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

[Damien Hedde]

On 12/2/19 1:33 PM, Peter Maydell wrote:
> On Mon, 2 Dec 2019 at 12:27, Damien Hedde <address@hidden> wrote:
>>
>>
>>
>> On 11/29/19 8:05 PM, Peter Maydell wrote:
>>> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <address@hidden> 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