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

[Peter Maydell]

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.

thanks
-- PMM