[v1] Fixes around device realization

[PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

Posted by Markus Armbruster 5 years, 6 months ago

stm32f405_soc_initfn() creates six such devices, but
stm32f405_soc_realize() realizes only one.  Affects machine
netduinoplus2.

I wonder how this ever worked.  If the "device becomes real only on
realize" thing actually works, then we've always been missing five of
six such devices, yet nobody noticed.

Fix stm32f405_soc_realize() to realize all six.  Visible in "info
qtree":

     bus: main-system-bus
       type System
       dev: stm32f405-soc, id ""
         cpu-type = "cortex-m4-arm-cpu"
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
         mmio 0000000040012000/00000000000000ff
       dev: stm32f2xx-adc, id ""
         gpio-out "sysbus-irq" 1
    -    mmio ffffffffffffffff/00000000000000ff
    +    mmio 0000000040012000/00000000000000ff
       dev: armv7m, id ""

The mmio addresses look suspicious.

Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
Cc: Alistair Francis <alistair@alistair23.me>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/stm32f405_soc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index 4f10ce6176..4649502711 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -185,16 +185,18 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
     qdev_connect_gpio_out(DEVICE(&s->adc_irqs), 0,
                           qdev_get_gpio_in(armv7m, ADC_IRQ));
 
-    dev = DEVICE(&(s->adc[i]));
-    object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
-    if (err != NULL) {
-        error_propagate(errp, err);
-        return;
+    for (i = 0; i < STM_NUM_ADCS; i++) {
+        dev = DEVICE(&(s->adc[i]));
+        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, ADC_ADDR);
+        sysbus_connect_irq(busdev, 0,
+                           qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
     }
-    busdev = SYS_BUS_DEVICE(dev);
-    sysbus_mmio_map(busdev, 0, ADC_ADDR);
-    sysbus_connect_irq(busdev, 0,
-                       qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
 
     /* SPI devices */
     for (i = 0; i < STM_NUM_SPIS; i++) {
-- 
2.21.1

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

Posted by Alistair Francis 5 years, 6 months ago

On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> stm32f405_soc_initfn() creates six such devices, but
> stm32f405_soc_realize() realizes only one.  Affects machine
> netduinoplus2.
>
> I wonder how this ever worked.  If the "device becomes real only on
> realize" thing actually works, then we've always been missing five of
> six such devices, yet nobody noticed.

I must have just been testing the first ADC.

>
> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
> qtree":
>
>      bus: main-system-bus
>        type System
>        dev: stm32f405-soc, id ""
>          cpu-type = "cortex-m4-arm-cpu"
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>          mmio 0000000040012000/00000000000000ff
>        dev: stm32f2xx-adc, id ""
>          gpio-out "sysbus-irq" 1
>     -    mmio ffffffffffffffff/00000000000000ff
>     +    mmio 0000000040012000/00000000000000ff
>        dev: armv7m, id ""
>
> The mmio addresses look suspicious.

Good catch, thanks :)

>
> Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/stm32f405_soc.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
> index 4f10ce6176..4649502711 100644
> --- a/hw/arm/stm32f405_soc.c
> +++ b/hw/arm/stm32f405_soc.c
> @@ -185,16 +185,18 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
>      qdev_connect_gpio_out(DEVICE(&s->adc_irqs), 0,
>                            qdev_get_gpio_in(armv7m, ADC_IRQ));
>
> -    dev = DEVICE(&(s->adc[i]));
> -    object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
> -    if (err != NULL) {
> -        error_propagate(errp, err);
> -        return;
> +    for (i = 0; i < STM_NUM_ADCS; i++) {
> +        dev = DEVICE(&(s->adc[i]));
> +        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        busdev = SYS_BUS_DEVICE(dev);
> +        sysbus_mmio_map(busdev, 0, ADC_ADDR);
> +        sysbus_connect_irq(busdev, 0,
> +                           qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
>      }
> -    busdev = SYS_BUS_DEVICE(dev);
> -    sysbus_mmio_map(busdev, 0, ADC_ADDR);
> -    sysbus_connect_irq(busdev, 0,
> -                       qdev_get_gpio_in(DEVICE(&s->adc_irqs), i));
>
>      /* SPI devices */
>      for (i = 0; i < STM_NUM_SPIS; i++) {
> --
> 2.21.1
>
>

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

Posted by Markus Armbruster 5 years, 5 months ago

Alistair Francis <alistair23@gmail.com> writes:

> On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> stm32f405_soc_initfn() creates six such devices, but
>> stm32f405_soc_realize() realizes only one.  Affects machine
>> netduinoplus2.
>>
>> I wonder how this ever worked.  If the "device becomes real only on
>> realize" thing actually works, then we've always been missing five of
>> six such devices, yet nobody noticed.
>
> I must have just been testing the first ADC.
>
>>
>> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
>> qtree":
>>
>>      bus: main-system-bus
>>        type System
>>        dev: stm32f405-soc, id ""
>>          cpu-type = "cortex-m4-arm-cpu"
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>          mmio 0000000040012000/00000000000000ff
>>        dev: stm32f2xx-adc, id ""
>>          gpio-out "sysbus-irq" 1
>>     -    mmio ffffffffffffffff/00000000000000ff
>>     +    mmio 0000000040012000/00000000000000ff
>>        dev: armv7m, id ""
>>
>> The mmio addresses look suspicious.
>
> Good catch, thanks :)

I'd love to squash in corrections, but I don't know the correct
addresses.  Can you help?

>>
>> Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Thanks!

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

Posted by Alistair Francis 5 years, 5 months ago

On Mon, May 18, 2020 at 10:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> > On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> stm32f405_soc_initfn() creates six such devices, but
> >> stm32f405_soc_realize() realizes only one.  Affects machine
> >> netduinoplus2.
> >>
> >> I wonder how this ever worked.  If the "device becomes real only on
> >> realize" thing actually works, then we've always been missing five of
> >> six such devices, yet nobody noticed.
> >
> > I must have just been testing the first ADC.
> >
> >>
> >> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
> >> qtree":
> >>
> >>      bus: main-system-bus
> >>        type System
> >>        dev: stm32f405-soc, id ""
> >>          cpu-type = "cortex-m4-arm-cpu"
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>          mmio 0000000040012000/00000000000000ff
> >>        dev: stm32f2xx-adc, id ""
> >>          gpio-out "sysbus-irq" 1
> >>     -    mmio ffffffffffffffff/00000000000000ff
> >>     +    mmio 0000000040012000/00000000000000ff
> >>        dev: armv7m, id ""
> >>
> >> The mmio addresses look suspicious.
> >
> > Good catch, thanks :)
>
> I'd love to squash in corrections, but I don't know the correct
> addresses.  Can you help?

Yep, thanks for squashing it in.

The three addresses are:

0x40012000
0x40012100
0x40012200

and they all share interrupt number 18.

Let me know if you want me to do it.

Alistair

>
> >>
> >> Fixes: 529fc5fd3e18ace8f739afd02dc0953354f39442
> >> Cc: Alistair Francis <alistair@alistair23.me>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: qemu-arm@nongnu.org
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Thanks!
>

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

Posted by Markus Armbruster 5 years, 5 months ago

Alistair Francis <alistair23@gmail.com> writes:

> On Mon, May 18, 2020 at 10:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Alistair Francis <alistair23@gmail.com> writes:
>>
>> > On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> stm32f405_soc_initfn() creates six such devices, but
>> >> stm32f405_soc_realize() realizes only one.  Affects machine
>> >> netduinoplus2.
>> >>
>> >> I wonder how this ever worked.  If the "device becomes real only on
>> >> realize" thing actually works, then we've always been missing five of
>> >> six such devices, yet nobody noticed.
>> >
>> > I must have just been testing the first ADC.
>> >
>> >>
>> >> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
>> >> qtree":
>> >>
>> >>      bus: main-system-bus
>> >>        type System
>> >>        dev: stm32f405-soc, id ""
>> >>          cpu-type = "cortex-m4-arm-cpu"
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>          mmio 0000000040012000/00000000000000ff
>> >>        dev: stm32f2xx-adc, id ""
>> >>          gpio-out "sysbus-irq" 1
>> >>     -    mmio ffffffffffffffff/00000000000000ff
>> >>     +    mmio 0000000040012000/00000000000000ff
>> >>        dev: armv7m, id ""
>> >>
>> >> The mmio addresses look suspicious.
>> >
>> > Good catch, thanks :)
>>
>> I'd love to squash in corrections, but I don't know the correct
>> addresses.  Can you help?
>
> Yep, thanks for squashing it in.
>
> The three addresses are:
>
> 0x40012000
> 0x40012100
> 0x40012200
>
> and they all share interrupt number 18.

An the other three?  There are six devices in total...

> Let me know if you want me to do it.

Re: [PATCH 01/24] arm/stm32f405: Fix realization of "stm32f2xx-adc" devices

Posted by Philippe Mathieu-Daudé 5 years, 5 months ago

On 5/27/20 11:27 AM, Markus Armbruster wrote:
> Alistair Francis <alistair23@gmail.com> writes:
> 
>> On Mon, May 18, 2020 at 10:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Alistair Francis <alistair23@gmail.com> writes:
>>>
>>>> On Sun, May 17, 2020 at 10:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>> stm32f405_soc_initfn() creates six such devices, but
>>>>> stm32f405_soc_realize() realizes only one.  Affects machine
>>>>> netduinoplus2.
>>>>>
>>>>> I wonder how this ever worked.  If the "device becomes real only on
>>>>> realize" thing actually works, then we've always been missing five of
>>>>> six such devices, yet nobody noticed.
>>>>
>>>> I must have just been testing the first ADC.
>>>>
>>>>>
>>>>> Fix stm32f405_soc_realize() to realize all six.  Visible in "info
>>>>> qtree":
>>>>>
>>>>>      bus: main-system-bus
>>>>>        type System
>>>>>        dev: stm32f405-soc, id ""
>>>>>          cpu-type = "cortex-m4-arm-cpu"
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>          mmio 0000000040012000/00000000000000ff
>>>>>        dev: stm32f2xx-adc, id ""
>>>>>          gpio-out "sysbus-irq" 1
>>>>>     -    mmio ffffffffffffffff/00000000000000ff
>>>>>     +    mmio 0000000040012000/00000000000000ff
>>>>>        dev: armv7m, id ""
>>>>>
>>>>> The mmio addresses look suspicious.
>>>>
>>>> Good catch, thanks :)
>>>
>>> I'd love to squash in corrections, but I don't know the correct
>>> addresses.  Can you help?
>>
>> Yep, thanks for squashing it in.
>>
>> The three addresses are:
>>
>> 0x40012000
>> 0x40012100
>> 0x40012200
>>
>> and they all share interrupt number 18.
> 
> An the other three?  There are six devices in total...

Alistair looked at the stm32f205, which has 3 ADCs (and seems correct).

> 
>> Let me know if you want me to do it.

Alistair, the problem is the stm32f405. I guess the bug come from
copy/pasting then modifying for the shared IRQ.

    /* Timer 2 to 5 */
    for (i = 0; i < STM_NUM_TIMERS; i++) {
       ...
    }
    ...
    /* ADC device, the IRQs are ORed together */
    ...
    dev = DEVICE(&(s->adc[i])); <=== i = STM_NUM_TIMERS = 4
    object_property_set_bool(OBJECT(&s->adc[i]),
                             true, "realized", &err);

Only ADC#3 is realized, then mapped at 0x40012000.

You need to unparent the others anyway, so it is easier to realize
adc[0] and unparent the rest:

    for (i = 1; i < STM_NUM_ADCS; i++) {
       ...
    }