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

[Damien Hedde]

On 11/29/19 8:05 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <address@hidden> wrote:
>>
>> Split gpfsel_set() in 2 so that the sdbus reparenting is done
>> in a dedicated function.
>>
>> Signed-off-by: Damien Hedde <address@hidden>
>> ---
>> Cc: Peter Maydell <address@hidden>
>> Cc: Andrew Baumann <address@hidden>
>> Cc: Philippe Mathieu-Daudé <address@hidden>
>> Cc: address@hidden
>> ---
>>  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