Re: [Qemu-devel] [RFC PATCH] hw/sd/sdhci: Move PCI-related code into a separate file

[Philippe Mathieu-Daudé]

On 2/20/19 5:31 PM, Paolo Bonzini wrote:
> On 20/02/19 13:26, Philippe Mathieu-Daudé wrote:
>> On 2/20/19 1:06 PM, Thomas Huth wrote:
>>> Some machines have an SDHCI device, but no PCI. To be able to
>>> compile hw/sd/sdhci.c without CONFIG_PCI, we must not call functions
>>> like pci_get_address_space() and pci_allocate_irq() there. Thus
>>> move the PCI-related code into a separate file.
>>>
>>> This is required for the upcoming Kconfig-like build system, e.g. it
>>> is needed if a user wants to compile a QEMU binary with just one machine
>>> that has SDHCI, but no PCI, like the ARM "raspi" machines for example.
>>>
>>> Signed-off-by: Thomas Huth <address@hidden>
>>
>> I wonder if we should add a F: entry into the "PC Chipset" section of
>> MAINTAINERS.
> 
> No, I don't think so.  It's not part of PC.

You are correct, it might be used by any board provinding a PCI bus.

What I'm looking for is a list of potential users or maintainers as in
"is this device supported/maintainted?".

Currently the only explicit user of the codebase qtest.

> Paolo
> 
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> Tested-by: Philippe Mathieu-Daudé <address@hidden>
>>
>>> ---
>>>  Note: Once we've got Kconfig in place, the "land" in the Makefile
>>>  should be replaced with a proper new CONFIG_SDHCI_PCI switch instead
>>>
>>>  hw/sd/Makefile.objs    |  1 +
>>>  hw/sd/sdhci-internal.h | 34 ++++++++++++++++++
>>>  hw/sd/sdhci-pci.c      | 87 ++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/sd/sdhci.c          | 98 
>>> +++-----------------------------------------------
>>>  4 files changed, 127 insertions(+), 93 deletions(-)
>>>  create mode 100644 hw/sd/sdhci-pci.c
>>>
>>> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
>>> index a99d9fb..58be7b3 100644
>>> --- a/hw/sd/Makefile.objs
>>> +++ b/hw/sd/Makefile.objs
>>> @@ -2,6 +2,7 @@ common-obj-$(CONFIG_PL181) += pl181.o
>>>  common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
>>>  common-obj-$(CONFIG_SD) += sd.o core.o sdmmc-internal.o
>>>  common-obj-$(CONFIG_SDHCI) += sdhci.o
>>> +common-obj-$(call land,$(CONFIG_SDHCI),$(CONFIG_PCI)) += sdhci-pci.o
>>>  
>>>  obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
>>>  obj-$(CONFIG_OMAP) += omap_mmc.o
>>> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
>>> index 19665fd..3414140 100644
>>> --- a/hw/sd/sdhci-internal.h
>>> +++ b/hw/sd/sdhci-internal.h
>>> @@ -304,4 +304,38 @@ extern const VMStateDescription sdhci_vmstate;
>>>  
>>>  #define ESDHC_PRNSTS_SDSTB              (1 << 3)
>>>  
>>> +/*
>>> + * Default SD/MMC host controller features information, which will be
>>> + * presented in CAPABILITIES register of generic SD host controller at 
>>> reset.
>>> + *
>>> + * support:
>>> + * - 3.3v and 1.8v voltages
>>> + * - SDMA/ADMA1/ADMA2
>>> + * - high-speed
>>> + * max host controller R/W buffers size: 512B
>>> + * max clock frequency for SDclock: 52 MHz
>>> + * timeout clock frequency: 52 MHz
>>> + *
>>> + * does not support:
>>> + * - 3.0v voltage
>>> + * - 64-bit system bus
>>> + * - suspend/resume
>>> + */
>>> +#define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>> +
>>> +#define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>> +    DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>> +    DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>> +    \
>>> +    /* Capabilities registers provide information on supported
>>> +     * features of this specific host controller implementation */ \
>>> +    DEFINE_PROP_UINT64("capareg", _state, capareg, 
>>> SDHC_CAPAB_REG_DEFAULT), \
>>> +    DEFINE_PROP_UINT64("maxcurr", _state, maxcurr, 0)
>>> +
>>> +void sdhci_initfn(SDHCIState *s);
>>> +void sdhci_uninitfn(SDHCIState *s);
>>> +void sdhci_common_realize(SDHCIState *s, Error **errp);
>>> +void sdhci_common_unrealize(SDHCIState *s, Error **errp);
>>> +void sdhci_common_class_init(ObjectClass *klass, void *data);
>>> +
>>>  #endif
>>> diff --git a/hw/sd/sdhci-pci.c b/hw/sd/sdhci-pci.c
>>> new file mode 100644
>>> index 0000000..f884661
>>> --- /dev/null
>>> +++ b/hw/sd/sdhci-pci.c
>>> @@ -0,0 +1,87 @@
>>> +/*
>>> + * SDHCI device on PCI
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License as published by the
>>> + * Free Software Foundation; either version 2 of the License, or (at your
>>> + * option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>> + * See the GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along
>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "hw/hw.h"
>>> +#include "hw/sd/sdhci.h"
>>> +#include "sdhci-internal.h"
>>> +
>>> +static Property sdhci_pci_properties[] = {
>>> +    DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>> +{
>>> +    SDHCIState *s = PCI_SDHCI(dev);
>>> +    Error *local_err = NULL;
>>> +
>>> +    sdhci_initfn(s);
>>> +    sdhci_common_realize(s, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
>>> +    dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
>>> +    s->irq = pci_allocate_irq(dev);
>>> +    s->dma_as = pci_get_address_space(dev);
>>> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->iomem);
>>> +}
>>> +
>>> +static void sdhci_pci_exit(PCIDevice *dev)
>>> +{
>>> +    SDHCIState *s = PCI_SDHCI(dev);
>>> +
>>> +    sdhci_common_unrealize(s, &error_abort);
>>> +    sdhci_uninitfn(s);
>>> +}
>>> +
>>> +static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> +
>>> +    k->realize = sdhci_pci_realize;
>>> +    k->exit = sdhci_pci_exit;
>>> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>> +    k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
>>> +    k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>> +    dc->props = sdhci_pci_properties;
>>> +
>>> +    sdhci_common_class_init(klass, data);
>>> +}
>>> +
>>> +static const TypeInfo sdhci_pci_info = {
>>> +    .name = TYPE_PCI_SDHCI,
>>> +    .parent = TYPE_PCI_DEVICE,
>>> +    .instance_size = sizeof(SDHCIState),
>>> +    .class_init = sdhci_pci_class_init,
>>> +    .interfaces = (InterfaceInfo[]) {
>>> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>> +        { },
>>> +    },
>>> +};
>>> +
>>> +static void sdhci_pci_register_type(void)
>>> +{
>>> +    type_register_static(&sdhci_pci_info);
>>> +}
>>> +
>>> +type_init(sdhci_pci_register_type)
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 83f1574..17ad546 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -40,24 +40,6 @@
>>>  
>>>  #define MASKED_WRITE(reg, mask, val)  (reg = (reg & (mask)) | (val))
>>>  
>>> -/* Default SD/MMC host controller features information, which will be
>>> - * presented in CAPABILITIES register of generic SD host controller at 
>>> reset.
>>> - *
>>> - * support:
>>> - * - 3.3v and 1.8v voltages
>>> - * - SDMA/ADMA1/ADMA2
>>> - * - high-speed
>>> - * max host controller R/W buffers size: 512B
>>> - * max clock frequency for SDclock: 52 MHz
>>> - * timeout clock frequency: 52 MHz
>>> - *
>>> - * does not support:
>>> - * - 3.0v voltage
>>> - * - 64-bit system bus
>>> - * - suspend/resume
>>> - */
>>> -#define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>>> -
>>>  static inline unsigned int sdhci_get_fifolen(SDHCIState *s)
>>>  {
>>>      return 1 << (9 + FIELD_EX32(s->capareg, SDHC_CAPAB, MAXBLOCKLENGTH));
>>> @@ -1328,16 +1310,7 @@ static void sdhci_init_readonly_registers(SDHCIState 
>>> *s, Error **errp)
>>>  
>>>  /* --- qdev common --- */
>>>  
>>> -#define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
>>> -    DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
>>> -    DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
>>> -    \
>>> -    /* Capabilities registers provide information on supported
>>> -     * features of this specific host controller implementation */ \
>>> -    DEFINE_PROP_UINT64("capareg", _state, capareg, 
>>> SDHC_CAPAB_REG_DEFAULT), \
>>> -    DEFINE_PROP_UINT64("maxcurr", _state, maxcurr, 0)
>>> -
>>> -static void sdhci_initfn(SDHCIState *s)
>>> +void sdhci_initfn(SDHCIState *s)
>>>  {
>>>      qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
>>>                          TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
>>> @@ -1348,7 +1321,7 @@ static void sdhci_initfn(SDHCIState *s)
>>>      s->io_ops = &sdhci_mmio_ops;
>>>  }
>>>  
>>> -static void sdhci_uninitfn(SDHCIState *s)
>>> +void sdhci_uninitfn(SDHCIState *s)
>>>  {
>>>      timer_del(s->insert_timer);
>>>      timer_free(s->insert_timer);
>>> @@ -1359,7 +1332,7 @@ static void sdhci_uninitfn(SDHCIState *s)
>>>      s->fifo_buffer = NULL;
>>>  }
>>>  
>>> -static void sdhci_common_realize(SDHCIState *s, Error **errp)
>>> +void sdhci_common_realize(SDHCIState *s, Error **errp)
>>>  {
>>>      Error *local_err = NULL;
>>>  
>>> @@ -1375,7 +1348,7 @@ static void sdhci_common_realize(SDHCIState *s, Error 
>>> **errp)
>>>                            SDHC_REGISTERS_MAP_SIZE);
>>>  }
>>>  
>>> -static void sdhci_common_unrealize(SDHCIState *s, Error **errp)
>>> +void sdhci_common_unrealize(SDHCIState *s, Error **errp)
>>>  {
>>>      /* This function is expected to be called only once for each class:
>>>       * - SysBus:    via DeviceClass->unrealize(),
>>> @@ -1445,7 +1418,7 @@ const VMStateDescription sdhci_vmstate = {
>>>      },
>>>  };
>>>  
>>> -static void sdhci_common_class_init(ObjectClass *klass, void *data)
>>> +void sdhci_common_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>  
>>> @@ -1454,66 +1427,6 @@ static void sdhci_common_class_init(ObjectClass 
>>> *klass, void *data)
>>>      dc->reset = sdhci_poweron_reset;
>>>  }
>>>  
>>> -/* --- qdev PCI --- */
>>> -
>>> -static Property sdhci_pci_properties[] = {
>>> -    DEFINE_SDHCI_COMMON_PROPERTIES(SDHCIState),
>>> -    DEFINE_PROP_END_OF_LIST(),
>>> -};
>>> -
>>> -static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>> -{
>>> -    SDHCIState *s = PCI_SDHCI(dev);
>>> -    Error *local_err = NULL;
>>> -
>>> -    sdhci_initfn(s);
>>> -    sdhci_common_realize(s, &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        return;
>>> -    }
>>> -
>>> -    dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */
>>> -    dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
>>> -    s->irq = pci_allocate_irq(dev);
>>> -    s->dma_as = pci_get_address_space(dev);
>>> -    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->iomem);
>>> -}
>>> -
>>> -static void sdhci_pci_exit(PCIDevice *dev)
>>> -{
>>> -    SDHCIState *s = PCI_SDHCI(dev);
>>> -
>>> -    sdhci_common_unrealize(s, &error_abort);
>>> -    sdhci_uninitfn(s);
>>> -}
>>> -
>>> -static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>> -{
>>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> -
>>> -    k->realize = sdhci_pci_realize;
>>> -    k->exit = sdhci_pci_exit;
>>> -    k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>> -    k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
>>> -    k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>> -    dc->props = sdhci_pci_properties;
>>> -
>>> -    sdhci_common_class_init(klass, data);
>>> -}
>>> -
>>> -static const TypeInfo sdhci_pci_info = {
>>> -    .name = TYPE_PCI_SDHCI,
>>> -    .parent = TYPE_PCI_DEVICE,
>>> -    .instance_size = sizeof(SDHCIState),
>>> -    .class_init = sdhci_pci_class_init,
>>> -    .interfaces = (InterfaceInfo[]) {
>>> -        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>> -        { },
>>> -    },
>>> -};
>>> -
>>>  /* --- qdev SysBus --- */
>>>  
>>>  static Property sdhci_sysbus_properties[] = {
>>> @@ -1846,7 +1759,6 @@ static const TypeInfo imx_usdhc_info = {
>>>  
>>>  static void sdhci_register_types(void)
>>>  {
>>> -    type_register_static(&sdhci_pci_info);
>>>      type_register_static(&sdhci_sysbus_info);
>>>      type_register_static(&sdhci_bus_info);
>>>      type_register_static(&imx_usdhc_info);
>>>