[Qemu-devel] [PATCH] hw/arm/raspi: avoid reparenting the sd card during qbus tree reset

Damien Hedde posted 1 patch 4 years, 6 months ago
Test docker-clang@ubuntu failed
Test FreeBSD passed
Test checkpatch passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190904162247.24095-1-damien.hedde@greensocs.com
Maintainers: Andrew Baumann <Andrew.Baumann@microsoft.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
hw/arm/raspi.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
[Qemu-devel] [PATCH] hw/arm/raspi: avoid reparenting the sd card during qbus tree reset
Posted by Damien Hedde 4 years, 6 months ago
In the raspi machine, the sd card can be on several sd bus (in reality
there is one bus but several controllers). It is initially created in
the "sd-bus" child in the gpio peripheral. Then is moved (parent bus
changes) during machine reset in the "sdhci-bus". It can be moved again
by software between the "sdhci-bus" and another bus ("bcm2835-sdhost-bus").
Here's the corresponding qbus tree of the raspi machine:
 + sysbus
   * bcm2835_gpio
     + sd-bus
       * sd-card
   * bcm2835-sdhost
     + bcm2835-sdhost-bus
   * generic-sdhci
     + sdhci-bus

During the initial machine reset, the sd card is moved. Since reset is
based on qbus tree, moving the card during the reset seems odd (it changes
the qbus tree). In this case, because of the order the qbus tree is
walked, the sd card ends up being reset twice; the effective reset order call
follows:
 1 sd-card
 2 sd-bus
 3 bcm2835_gpio        -> move the sd-card to sdhci_bus
 4 bcm2835-sdhost-bus
 5 bcm2835-sdhost
 6 sd-card             (again)
 7 sdhci-bus
 8 generic-sdhci

This patch adds a raspi machine reset method which moves the sd card
to the sdhci-bus before doing the whole reset (which will try to do the
move too). By anticipating the move we avoid changing the qdev tree while
resetting it.

In consequence the step 1 is skipped in the previous list: when reset starts
the sd-card is already not a child of bcm2835_gpio.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
This is a follow-up of this discussion
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg06954.html

I did not have much comments about my last proposal so I thought I'd finalize
a corresponding patch since it is small instead of doing a ping.

Feel free to comment,
Damien
---
 hw/arm/raspi.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 74c062d05e..19b032546b 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -21,6 +21,7 @@
 #include "hw/loader.h"
 #include "hw/arm/boot.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/reset.h"
 
 #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS */
 #define MVBAR_ADDR      0x400 /* secure vectors */
@@ -214,6 +215,32 @@ static void raspi_init(MachineState *machine, int version)
     setup_boot(machine, version, machine->ram_size - vcram_size);
 }
 
+static void raspi_reset(MachineState *machine)
+{
+    BCM2835GpioState *gpio;
+
+    gpio = BCM2835_GPIO(object_resolve_path("gpio", NULL));
+
+    /*
+     * Put the sd-card on sdhci bus as the bcm2835_gpio's reset
+     * procedure will do during qemu_devices_reset().
+     *
+     * Note: we do this here to avoid doing it during following
+     * qemu_devices_reset() so that we don't modify the qbus tree during the
+     * reset (which is based on it).
+     *
+     * Note: sd-card can be on sdbus, sdbus_sdhci or sdbus_sdhost. So at least
+     * one of the following sdbus_reparent_card is useless. This is not a
+     * problem because sdbus_reparent_card is a no-op if the source does not
+     * has a card.
+     */
+    sdbus_reparent_card(&gpio->sdbus, gpio->sdbus_sdhci);
+    sdbus_reparent_card(gpio->sdbus_sdhost, gpio->sdbus_sdhci);
+
+    /* then do the classical reset */
+    qemu_devices_reset();
+}
+
 static void raspi2_init(MachineState *machine)
 {
     raspi_init(machine, 2);
@@ -223,6 +250,7 @@ static void raspi2_machine_init(MachineClass *mc)
 {
     mc->desc = "Raspberry Pi 2";
     mc->init = raspi2_init;
+    mc->reset = raspi_reset;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
     mc->no_floppy = 1;
@@ -245,6 +273,7 @@ static void raspi3_machine_init(MachineClass *mc)
 {
     mc->desc = "Raspberry Pi 3";
     mc->init = raspi3_init;
+    mc->reset = raspi_reset;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
     mc->no_floppy = 1;
-- 
2.22.0


Re: [Qemu-devel] [PATCH] hw/arm/raspi: avoid reparenting the sd card during qbus tree reset
Posted by Peter Maydell 4 years, 6 months ago
On Wed, 4 Sep 2019 at 17:23, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> In the raspi machine, the sd card can be on several sd bus (in reality
> there is one bus but several controllers). It is initially created in
> the "sd-bus" child in the gpio peripheral. Then is moved (parent bus
> changes) during machine reset in the "sdhci-bus". It can be moved again
> by software between the "sdhci-bus" and another bus ("bcm2835-sdhost-bus").
> Here's the corresponding qbus tree of the raspi machine:
>  + sysbus
>    * bcm2835_gpio
>      + sd-bus
>        * sd-card
>    * bcm2835-sdhost
>      + bcm2835-sdhost-bus
>    * generic-sdhci
>      + sdhci-bus
>
> During the initial machine reset, the sd card is moved. Since reset is
> based on qbus tree, moving the card during the reset seems odd (it changes
> the qbus tree). In this case, because of the order the qbus tree is
> walked, the sd card ends up being reset twice; the effective reset order call
> follows:
>  1 sd-card
>  2 sd-bus
>  3 bcm2835_gpio        -> move the sd-card to sdhci_bus
>  4 bcm2835-sdhost-bus
>  5 bcm2835-sdhost
>  6 sd-card             (again)
>  7 sdhci-bus
>  8 generic-sdhci
>
> This patch adds a raspi machine reset method which moves the sd card
> to the sdhci-bus before doing the whole reset (which will try to do the
> move too). By anticipating the move we avoid changing the qdev tree while
> resetting it.
>
> In consequence the step 1 is skipped in the previous list: when reset starts
> the sd-card is already not a child of bcm2835_gpio.

The solution proposed in this patch pushes something that should
really be the business just of the SoC model out to the machine
model level; it would be nice to be able to avoid that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/raspi: avoid reparenting the sd card during qbus tree reset
Posted by Damien Hedde 4 years, 6 months ago

On 9/6/19 12:36 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 17:23, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> In the raspi machine, the sd card can be on several sd bus (in reality
>> there is one bus but several controllers). It is initially created in
>> the "sd-bus" child in the gpio peripheral. Then is moved (parent bus
>> changes) during machine reset in the "sdhci-bus". It can be moved again
>> by software between the "sdhci-bus" and another bus ("bcm2835-sdhost-bus").
>> Here's the corresponding qbus tree of the raspi machine:
>>  + sysbus
>>    * bcm2835_gpio
>>      + sd-bus
>>        * sd-card
>>    * bcm2835-sdhost
>>      + bcm2835-sdhost-bus
>>    * generic-sdhci
>>      + sdhci-bus
>>
>> During the initial machine reset, the sd card is moved. Since reset is
>> based on qbus tree, moving the card during the reset seems odd (it changes
>> the qbus tree). In this case, because of the order the qbus tree is
>> walked, the sd card ends up being reset twice; the effective reset order call
>> follows:
>>  1 sd-card
>>  2 sd-bus
>>  3 bcm2835_gpio        -> move the sd-card to sdhci_bus
>>  4 bcm2835-sdhost-bus
>>  5 bcm2835-sdhost
>>  6 sd-card             (again)
>>  7 sdhci-bus
>>  8 generic-sdhci
>>
>> This patch adds a raspi machine reset method which moves the sd card
>> to the sdhci-bus before doing the whole reset (which will try to do the
>> move too). By anticipating the move we avoid changing the qdev tree while
>> resetting it.
>>
>> In consequence the step 1 is skipped in the previous list: when reset starts
>> the sd-card is already not a child of bcm2835_gpio.
> 
> The solution proposed in this patch pushes something that should
> really be the business just of the SoC model out to the machine
> model level; it would be nice to be able to avoid that.

The problem is sysbus is the only common "reset" ancestor of all
sd-card-buses. So we don't really have a lot of places to do this.

I could move the proposed code to the reset method of
bcm2835_peripherals which creates the bcm3825_gpio, bcm2835-sdhost and
generic-sdhci so we don't have to seek these.
It will works because the reset of bcm2835_peripherals is called before
the others.
But in terms of reset/qdev hiearchy, the 'bcm2835_peripherals' it at the
same level of bcm2835_gpio.

The reset order would be:

1 bcm2835_peripherals -> move the sd-card to sdhci_bus.
2   sd-bus
3 bcm2835_gpio
4   bcm2835-sdhost-bus
5 bcm2835-sdhost
6     sd-card
7   sdhci-bus
8 generic-sdhci

Would that be an acceptable solution ?

--
Thanks
Damien