diff mbox series

[v2,04/20] sdhci: refactor same sysbus/pci properties into a common one

Message ID 20171215031547.31006-5-f4bug@amsat.org
State Superseded, archived
Headers show
Series SDHCI: housekeeping, add a qtest and fix few issues | expand

Commit Message

Philippe Mathieu-Daudé Dec. 15, 2017, 3:15 a.m. UTC 
add sysbus/pci/sdbus separator comments to keep it clearer

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Alistair Francis Dec. 19, 2017, 1:13 a.m. UTC | #1 
On Thu, Dec 14, 2017 at 7:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> add sysbus/pci/sdbus separator comments to keep it clearer
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I'm still unsure about this. Won't this leave us with properties that
have no impact on the device? That seems very confusing to me.

Alistair

> ---
>  hw/sd/sdhci.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 2823da00da..dbdfd54350 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1265,13 +1265,17 @@ const VMStateDescription sdhci_vmstate = {
>
>  /* Capabilities registers provide information on supported features of this
>   * specific host controller implementation */
> -static Property sdhci_pci_properties[] = {
> +static Property sdhci_properties[] = {
>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>              SDHC_CAPAB_REG_DEFAULT),
>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
> +                     false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +/* --- qdev PCI --- */
> +
>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>  {
>      SDHCIState *s = PCI_SDHCI(dev);
> @@ -1304,7 +1308,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      dc->vmsd = &sdhci_vmstate;
> -    dc->props = sdhci_pci_properties;
> +    dc->props = sdhci_properties;
>      dc->reset = sdhci_poweron_reset;
>  }
>
> @@ -1319,14 +1323,7 @@ static const TypeInfo sdhci_pci_info = {
>      },
>  };
>
> -static Property sdhci_sysbus_properties[] = {
> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
> -            SDHC_CAPAB_REG_DEFAULT),
> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
> -                     false),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> +/* --- qdev SysBus --- */
>
>  static void sdhci_sysbus_init(Object *obj)
>  {
> @@ -1359,7 +1356,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->vmsd = &sdhci_vmstate;
> -    dc->props = sdhci_sysbus_properties;
> +    dc->props = sdhci_properties;
>      dc->realize = sdhci_sysbus_realize;
>      dc->reset = sdhci_poweron_reset;
>  }
> @@ -1373,6 +1370,8 @@ static const TypeInfo sdhci_sysbus_info = {
>      .class_init = sdhci_sysbus_class_init,
>  };
>
> +/* --- qdev bus master --- */
> +
>  static void sdhci_bus_class_init(ObjectClass *klass, void *data)
>  {
>      SDBusClass *sbc = SD_BUS_CLASS(klass);
> --
> 2.15.1
>
>
Philippe Mathieu-Daudé Dec. 29, 2017, 5:21 p.m. UTC | #2 
Hi Alistair,

On 12/18/2017 10:13 PM, Alistair Francis wrote:
> On Thu, Dec 14, 2017 at 7:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> add sysbus/pci/sdbus separator comments to keep it clearer
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I'm still unsure about this. Won't this leave us with properties that
> have no impact on the device? That seems very confusing to me.

(from previous series)
> [...] but aren't we now going to
> have device properties that aren't actually connected to anything?

I'm also confused :)

My understanding is the "pending-insert-quirk" worries you.
This property is dependent of the HCI IP, so regardless the HCI is
accessed through a MMIO sysbus or a PCI bus the quirk might exists (for
this property, only the BCM implementation).

With v3 series applied, the monitor 'qtree' output is:

$ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S
(qemu) info qtree
  dev: generic-sdhci, id ""
    gpio-out "sysbus-irq" 1
    sd-spec-version = 3 (0x3)
    timeout-freq = 0 (0x0)
    freq-in-mhz = true
    clock-freq = 0 (0x0)
    max-block-length = 512 (0x200)
    dma = true
    sdma = true
    adma1 = false
    adma2 = true
    suspend = true
    high-speed = true
    3v3 = true
    3v0 = false
    1v8 = true
    64bit = true
    slot-type = 0 (0x0)
    bus-speed = 7 (0x7)
    driver-strength = 7 (0x7)
    maxcurr = 0 (0x0)
    pending-insert-quirk = false
    dma-memory = ""
    mmio 00000000ff170000/0000000000000100
    bus: sd-bus
      type sdhci-bus
      dev: sd-card, id ""
        drive = ""
        spi = false

Are you worried about this output?

>> ---
>>  hw/sd/sdhci.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 2823da00da..dbdfd54350 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1265,13 +1265,17 @@ const VMStateDescription sdhci_vmstate = {
>>
>>  /* Capabilities registers provide information on supported features of this
>>   * specific host controller implementation */
>> -static Property sdhci_pci_properties[] = {
>> +static Property sdhci_properties[] = {
>>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>              SDHC_CAPAB_REG_DEFAULT),
>>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>> +                     false),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> +/* --- qdev PCI --- */
>> +
>>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>  {
>>      SDHCIState *s = PCI_SDHCI(dev);
>> @@ -1304,7 +1308,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>      dc->vmsd = &sdhci_vmstate;
>> -    dc->props = sdhci_pci_properties;
>> +    dc->props = sdhci_properties;
>>      dc->reset = sdhci_poweron_reset;
>>  }
>>
>> @@ -1319,14 +1323,7 @@ static const TypeInfo sdhci_pci_info = {
>>      },
>>  };
>>
>> -static Property sdhci_sysbus_properties[] = {
>> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>> -            SDHC_CAPAB_REG_DEFAULT),
>> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>> -                     false),
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>> +/* --- qdev SysBus --- */
>>
>>  static void sdhci_sysbus_init(Object *obj)
>>  {
>> @@ -1359,7 +1356,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>
>>      dc->vmsd = &sdhci_vmstate;
>> -    dc->props = sdhci_sysbus_properties;
>> +    dc->props = sdhci_properties;
>>      dc->realize = sdhci_sysbus_realize;
>>      dc->reset = sdhci_poweron_reset;
>>  }
>> @@ -1373,6 +1370,8 @@ static const TypeInfo sdhci_sysbus_info = {
>>      .class_init = sdhci_sysbus_class_init,
>>  };
>>
>> +/* --- qdev bus master --- */
>> +
>>  static void sdhci_bus_class_init(ObjectClass *klass, void *data)
>>  {
>>      SDBusClass *sbc = SD_BUS_CLASS(klass);
>> --
>> 2.15.1
>>
>>
Alistair Francis Jan. 3, 2018, 7:36 p.m. UTC | #3 
On Fri, Dec 29, 2017 at 9:21 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Alistair,
>
> On 12/18/2017 10:13 PM, Alistair Francis wrote:
>> On Thu, Dec 14, 2017 at 7:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> add sysbus/pci/sdbus separator comments to keep it clearer
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> I'm still unsure about this. Won't this leave us with properties that
>> have no impact on the device? That seems very confusing to me.
>
> (from previous series)
>> [...] but aren't we now going to
>> have device properties that aren't actually connected to anything?
>
> I'm also confused :)
>
> My understanding is the "pending-insert-quirk" worries you.

Exactly, my worry is having properties that aren't connected. As a
user I expect that if I set a property that will have some effect on
how QEMU runs. A un-connected property is just confusing.

> This property is dependent of the HCI IP, so regardless the HCI is
> accessed through a MMIO sysbus or a PCI bus the quirk might exists (for
> this property, only the BCM implementation).

So it has always been configuration dependent?

Alistair

>
> With v3 series applied, the monitor 'qtree' output is:
>
> $ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S
> (qemu) info qtree
>   dev: generic-sdhci, id ""
>     gpio-out "sysbus-irq" 1
>     sd-spec-version = 3 (0x3)
>     timeout-freq = 0 (0x0)
>     freq-in-mhz = true
>     clock-freq = 0 (0x0)
>     max-block-length = 512 (0x200)
>     dma = true
>     sdma = true
>     adma1 = false
>     adma2 = true
>     suspend = true
>     high-speed = true
>     3v3 = true
>     3v0 = false
>     1v8 = true
>     64bit = true
>     slot-type = 0 (0x0)
>     bus-speed = 7 (0x7)
>     driver-strength = 7 (0x7)
>     maxcurr = 0 (0x0)
>     pending-insert-quirk = false
>     dma-memory = ""
>     mmio 00000000ff170000/0000000000000100
>     bus: sd-bus
>       type sdhci-bus
>       dev: sd-card, id ""
>         drive = ""
>         spi = false
>
> Are you worried about this output?
>
>>> ---
>>>  hw/sd/sdhci.c | 21 ++++++++++-----------
>>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 2823da00da..dbdfd54350 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -1265,13 +1265,17 @@ const VMStateDescription sdhci_vmstate = {
>>>
>>>  /* Capabilities registers provide information on supported features of this
>>>   * specific host controller implementation */
>>> -static Property sdhci_pci_properties[] = {
>>> +static Property sdhci_properties[] = {
>>>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>>              SDHC_CAPAB_REG_DEFAULT),
>>>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>>> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>>> +                     false),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> +/* --- qdev PCI --- */
>>> +
>>>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>>  {
>>>      SDHCIState *s = PCI_SDHCI(dev);
>>> @@ -1304,7 +1308,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>>      dc->vmsd = &sdhci_vmstate;
>>> -    dc->props = sdhci_pci_properties;
>>> +    dc->props = sdhci_properties;
>>>      dc->reset = sdhci_poweron_reset;
>>>  }
>>>
>>> @@ -1319,14 +1323,7 @@ static const TypeInfo sdhci_pci_info = {
>>>      },
>>>  };
>>>
>>> -static Property sdhci_sysbus_properties[] = {
>>> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>> -            SDHC_CAPAB_REG_DEFAULT),
>>> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>>> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
>>> -                     false),
>>> -    DEFINE_PROP_END_OF_LIST(),
>>> -};
>>> +/* --- qdev SysBus --- */
>>>
>>>  static void sdhci_sysbus_init(Object *obj)
>>>  {
>>> @@ -1359,7 +1356,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>
>>>      dc->vmsd = &sdhci_vmstate;
>>> -    dc->props = sdhci_sysbus_properties;
>>> +    dc->props = sdhci_properties;
>>>      dc->realize = sdhci_sysbus_realize;
>>>      dc->reset = sdhci_poweron_reset;
>>>  }
>>> @@ -1373,6 +1370,8 @@ static const TypeInfo sdhci_sysbus_info = {
>>>      .class_init = sdhci_sysbus_class_init,
>>>  };
>>>
>>> +/* --- qdev bus master --- */
>>> +
>>>  static void sdhci_bus_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      SDBusClass *sbc = SD_BUS_CLASS(klass);
>>> --
>>> 2.15.1
>>>
>>>
>
diff mbox series

Patch

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 2823da00da..dbdfd54350 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1265,13 +1265,17 @@  const VMStateDescription sdhci_vmstate = {
 
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
-static Property sdhci_pci_properties[] = {
+static Property sdhci_properties[] = {
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* --- qdev PCI --- */
+
 static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
 {
     SDHCIState *s = PCI_SDHCI(dev);
@@ -1304,7 +1308,7 @@  static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_pci_properties;
+    dc->props = sdhci_properties;
     dc->reset = sdhci_poweron_reset;
 }
 
@@ -1319,14 +1323,7 @@  static const TypeInfo sdhci_pci_info = {
     },
 };
 
-static Property sdhci_sysbus_properties[] = {
-    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
-            SDHC_CAPAB_REG_DEFAULT),
-    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
-    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert_quirk,
-                     false),
-    DEFINE_PROP_END_OF_LIST(),
-};
+/* --- qdev SysBus --- */
 
 static void sdhci_sysbus_init(Object *obj)
 {
@@ -1359,7 +1356,7 @@  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &sdhci_vmstate;
-    dc->props = sdhci_sysbus_properties;
+    dc->props = sdhci_properties;
     dc->realize = sdhci_sysbus_realize;
     dc->reset = sdhci_poweron_reset;
 }
@@ -1373,6 +1370,8 @@  static const TypeInfo sdhci_sysbus_info = {
     .class_init = sdhci_sysbus_class_init,
 };
 
+/* --- qdev bus master --- */
+
 static void sdhci_bus_class_init(ObjectClass *klass, void *data)
 {
     SDBusClass *sbc = SD_BUS_CLASS(klass);