diff mbox series

[v5,12/13] hw/gpio/bcm2835_gpio: Isolate sdbus reparenting

Message ID 20191018150630.31099-13-damien.hedde@greensocs.com (mailing list archive)
State New, archived
Headers show
Series Multi-phase reset mechanism | expand

Commit Message

Damien Hedde Oct. 18, 2019, 3:06 p.m. UTC
Split gpfsel_set() in 2 so that the sdbus reparenting is done
in a dedicated function.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: qemu-arm@nongnu.org
---
 hw/gpio/bcm2835_gpio.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Peter Maydell Nov. 29, 2019, 7:05 p.m. UTC | #1
On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Split gpfsel_set() in 2 so that the sdbus reparenting is done
> in a dedicated function.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/gpio/bcm2835_gpio.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
> index 91ce3d10cc..81fe07132f 100644
> --- a/hw/gpio/bcm2835_gpio.c
> +++ b/hw/gpio/bcm2835_gpio.c
> @@ -75,7 +75,10 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>              s->fsel[index] = fsel;
>          }
>      }
> +}
>
> +static void gpfsel_update_sdbus(BCM2835GpioState *s)
> +{
>      /* SD controller selection (48-53) */
>      if (s->sd_fsel != 0
>              && (s->fsel[48] == 0) /* SD_CLK_R */
> @@ -86,6 +89,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>              && (s->fsel[53] == 0) /* SD_DATA3_R */
>              ) {
>          /* SDHCI controller selected */
> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
>          sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
>          s->sd_fsel = 0;>      } else if (s->sd_fsel != 4
> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>              && (s->fsel[53] == 4) /* SD_DATA3_R */
>              ) {
>          /* SDHost controller selected */
> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);

The commit message says it's just splitting the function in two,
but these two hunks are adding extra calls to sdbus_reparent_card().
Why do we need to call it twice ?

>          s->sd_fsel = 4;
>      }

thanks
-- PMM
Damien Hedde Dec. 2, 2019, 12:27 p.m. UTC | #2
On 11/29/19 8:05 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> Split gpfsel_set() in 2 so that the sdbus reparenting is done
>> in a dedicated function.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: qemu-arm@nongnu.org
>> ---
>>  hw/gpio/bcm2835_gpio.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
>> index 91ce3d10cc..81fe07132f 100644
>> --- a/hw/gpio/bcm2835_gpio.c
>> +++ b/hw/gpio/bcm2835_gpio.c
>> @@ -75,7 +75,10 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>              s->fsel[index] = fsel;
>>          }
>>      }
>> +}
>>
>> +static void gpfsel_update_sdbus(BCM2835GpioState *s)
>> +{
>>      /* SD controller selection (48-53) */
>>      if (s->sd_fsel != 0
>>              && (s->fsel[48] == 0) /* SD_CLK_R */
>> @@ -86,6 +89,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>              && (s->fsel[53] == 0) /* SD_DATA3_R */
>>              ) {
>>          /* SDHCI controller selected */
>> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
>>          sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
>>          s->sd_fsel = 0;>      } else if (s->sd_fsel != 4
>> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>              && (s->fsel[53] == 4) /* SD_DATA3_R */
>>              ) {
>>          /* SDHost controller selected */
>> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
>>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
> 
> The commit message says it's just splitting the function in two,
> but these two hunks are adding extra calls to sdbus_reparent_card().
> Why do we need to call it twice ?

You're right. I forgot to update the commit message. The patch also
refactor a little the reset procedure and move the call to
sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci)
which was in there to this part of the code.

raspi machines create the sd in &s->sdbus. So there is need for a first
reparenting from this bus.

With this addition "gpfsel_update_sdbus" always do the expected effect
of putting the sd card onto the right bus.

sdbus_reparent_card(src,dst) only do something if the _src_ bus has a
card. So only one of the 2 sdbus_reparent_card will have an effect. If
the card is already onto the _dst_, both calls will be nop-op.

What about rewording the commit message like this ?
| hw/gpio/bcm2835_gpio: Refactor sdbus reparenting
|
| Split gpfsel_set() in 2 so that the sdbus reparenting is done in a
| dedicated function gpfsel_update_sdbus() and update call sites.
| Also make gpfsel_update_sdbus() handle the case where the sdcard is in
| BCM2835GpioState.sdbus (initial sd card holding bus at machine
| creation).
| Refactor the reset procedure in consequence.
|
| This patch is a preparation step for the migration to multi-phases
| reset which will be done in a following commit

Thanks,
--
Damien
Peter Maydell Dec. 2, 2019, 12:33 p.m. UTC | #3
On Mon, 2 Dec 2019 at 12:27, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 11/29/19 8:05 PM, Peter Maydell wrote:
> > On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
> >> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
> >>              && (s->fsel[53] == 4) /* SD_DATA3_R */
> >>              ) {
> >>          /* SDHost controller selected */
> >> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
> >>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
> >
> > The commit message says it's just splitting the function in two,
> > but these two hunks are adding extra calls to sdbus_reparent_card().
> > Why do we need to call it twice ?
>
> You're right. I forgot to update the commit message. The patch also
> refactor a little the reset procedure and move the call to
> sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci)
> which was in there to this part of the code.
>
> raspi machines create the sd in &s->sdbus. So there is need for a first
> reparenting from this bus.
>
> With this addition "gpfsel_update_sdbus" always do the expected effect
> of putting the sd card onto the right bus.
>
> sdbus_reparent_card(src,dst) only do something if the _src_ bus has a
> card. So only one of the 2 sdbus_reparent_card will have an effect. If
> the card is already onto the _dst_, both calls will be nop-op

The intention of sdbus_reparent_card() is that it moves
something from the 'src' bus to the 'dst' bus. So one
call is supposed to do the whole job of the move. If
it doesn't, then that's a bug.

I thought the raspi machines had an sd card that could
either be connected to one of the controllers, or the
other. Why would the sd card ever be somewhere else
than on one of those two buses? If the machine creation
puts the sdcard somewhere wrong then we should probably
just fix that.

thanks
-- PMM
Damien Hedde Dec. 2, 2019, 1:05 p.m. UTC | #4
On 12/2/19 1:33 PM, Peter Maydell wrote:
> On Mon, 2 Dec 2019 at 12:27, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>>
>>
>> On 11/29/19 8:05 PM, Peter Maydell wrote:
>>> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>> @@ -97,6 +101,7 @@ static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
>>>>              && (s->fsel[53] == 4) /* SD_DATA3_R */
>>>>              ) {
>>>>          /* SDHost controller selected */
>>>> +        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
>>>>          sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
>>>
>>> The commit message says it's just splitting the function in two,
>>> but these two hunks are adding extra calls to sdbus_reparent_card().
>>> Why do we need to call it twice ?
>>
>> You're right. I forgot to update the commit message. The patch also
>> refactor a little the reset procedure and move the call to
>> sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci)
>> which was in there to this part of the code.
>>
>> raspi machines create the sd in &s->sdbus. So there is need for a first
>> reparenting from this bus.
>>
>> With this addition "gpfsel_update_sdbus" always do the expected effect
>> of putting the sd card onto the right bus.
>>
>> sdbus_reparent_card(src,dst) only do something if the _src_ bus has a
>> card. So only one of the 2 sdbus_reparent_card will have an effect. If
>> the card is already onto the _dst_, both calls will be nop-op
> 
> The intention of sdbus_reparent_card() is that it moves
> something from the 'src' bus to the 'dst' bus. So one
> call is supposed to do the whole job of the move. If
> it doesn't, then that's a bug.
> 
> I thought the raspi machines had an sd card that could
> either be connected to one of the controllers, or the
> other. Why would the sd card ever be somewhere else
> than on one of those two buses? If the machine creation
> puts the sdcard somewhere wrong then we should probably
> just fix that.
> 

I don't know why it has been implemented like this but right now the
raspi_init() does the following during machine creation:
| bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
| [...]
| carddev = qdev_create(bus, TYPE_SD_CARD);
which put the sd in the BCM2835GpioState.sdbus .

Then the reset procedure of the BCM2835Gpio move the sd card
to one of the two usable controllers and the sd card can never go back
to the initial BCM2835GpioState.sdbus.
As far as I understand, it is theorically possible to have the sd card
on no controller at all (it's maybe the reason for the .sdbus "useless"
bus) (for example if the BCM2835Gpio is badly configured) but this is
not implemented in qemu.

Anyway I can add some plumbing to only call sdbus_reparent_card() when
really needed by:
+ not duplicating the sdbus_reparent_card() in gpfsel_update_sdbus()
+ adding needed test in reset() method to only do the initial
  sdbus_reparent_card() if needed (first time we call reset).

--
Damien
Peter Maydell Dec. 2, 2019, 1:10 p.m. UTC | #5
On Mon, 2 Dec 2019 at 13:05, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> I don't know why it has been implemented like this but right now the
> raspi_init() does the following during machine creation:
> | bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> | [...]
> | carddev = qdev_create(bus, TYPE_SD_CARD);
> which put the sd in the BCM2835GpioState.sdbus .
>
> Then the reset procedure of the BCM2835Gpio move the sd card
> to one of the two usable controllers and the sd card can never go back
> to the initial BCM2835GpioState.sdbus.

This seems like it's just an oversight. The code in raspi_init()
which creates the SD card was added in 2016, a year before
the gpio device was added, so when it was written there was
only ever one place the SD card could be, I think. We should
fix it so it puts the card in the right place to start with.

> As far as I understand, it is theorically possible to have the sd card
> on no controller at all (it's maybe the reason for the .sdbus "useless"
> bus) (for example if the BCM2835Gpio is badly configured) but this is
> not implemented in qemu.
>
> Anyway I can add some plumbing to only call sdbus_reparent_card() when
> really needed by:
> + not duplicating the sdbus_reparent_card() in gpfsel_update_sdbus()
> + adding needed test in reset() method to only do the initial
>   sdbus_reparent_card() if needed (first time we call reset).

I don't think you need the latter if we make the machine model
put the card in the right place to start with.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
index 91ce3d10cc..81fe07132f 100644
--- a/hw/gpio/bcm2835_gpio.c
+++ b/hw/gpio/bcm2835_gpio.c
@@ -75,7 +75,10 @@  static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
             s->fsel[index] = fsel;
         }
     }
+}
 
+static void gpfsel_update_sdbus(BCM2835GpioState *s)
+{
     /* SD controller selection (48-53) */
     if (s->sd_fsel != 0
             && (s->fsel[48] == 0) /* SD_CLK_R */
@@ -86,6 +89,7 @@  static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
             && (s->fsel[53] == 0) /* SD_DATA3_R */
             ) {
         /* SDHCI controller selected */
+        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
         sdbus_reparent_card(s->sdbus_sdhost, s->sdbus_sdhci);
         s->sd_fsel = 0;
     } else if (s->sd_fsel != 4
@@ -97,6 +101,7 @@  static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
             && (s->fsel[53] == 4) /* SD_DATA3_R */
             ) {
         /* SDHost controller selected */
+        sdbus_reparent_card(&s->sdbus, s->sdbus_sdhost);
         sdbus_reparent_card(s->sdbus_sdhci, s->sdbus_sdhost);
         s->sd_fsel = 4;
     }
@@ -210,6 +215,7 @@  static void bcm2835_gpio_write(void *opaque, hwaddr offset,
     case GPFSEL4:
     case GPFSEL5:
         gpfsel_set(s, offset / 4, value);
+        gpfsel_update_sdbus(s);
         break;
     case GPSET0:
         gpset(s, value, 0, 32, &s->lev0);
@@ -265,10 +271,12 @@  static void bcm2835_gpio_reset(DeviceState *dev)
         gpfsel_set(s, i, 0);
     }
 
-    s->sd_fsel = 0;
-
-    /* SDHCI is selected by default */
-    sdbus_reparent_card(&s->sdbus, s->sdbus_sdhci);
+    /*
+     * Setup the right sdbus (put 1 in sd_fsel to force reparenting
+     * the sd). It will be SDHCI because of the gpfsel_set() above.
+     */
+    s->sd_fsel = 1;
+    gpfsel_update_sdbus(s);
 
     s->lev0 = 0;
     s->lev1 = 0;