| Message ID | 20191018150630.31099-13-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multi-phase reset mechanism | expand |
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
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
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
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
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 --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;
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(-)