| Message ID | 20230830092314.624212-1-mathieu.moneyron@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: Fix force card detect in sdhci | expand |
+ Eugen Hristev On 30/08/23 12:23, mathieu wrote: > From: Mathieu Moneyron <mathieu.moneyron@gmail.com> > > On the ATMEL at91 when using the non-removable flag in device tree and not > using the card-detect pin inside the device-tree pinctrl, the card detect > pin is physically still used which can cause unknown behaviour when this > pin is used for other purposes. > > From my interpretation this seems to be caused by a hardware design flaw > and the real hardware is not working as intended by the documentation. > > Signed-off-by: Mathieu Moneyron <mathieu.moneyron@gmail.com> > --- > drivers/mmc/host/sdhci-of-at91.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c > index 69fef88e7..4fd6bfbf6 100644 > --- a/drivers/mmc/host/sdhci-of-at91.c > +++ b/drivers/mmc/host/sdhci-of-at91.c > @@ -51,10 +51,15 @@ struct sdhci_at91_priv { > static void sdhci_at91_set_force_card_detect(struct sdhci_host *host) > { > u8 mc1r; > + u8 ctrl; > > mc1r = readb(host->ioaddr + SDMMC_MC1R); > mc1r |= SDMMC_MC1R_FCD; > writeb(mc1r, host->ioaddr + SDMMC_MC1R); > + > + ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL); > + ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN; > + writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); > } > > static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned int clock)
On 04/09/2023 09:38, Adrian Hunter wrote: > + Eugen Hristev > > On 30/08/23 12:23, mathieu wrote: >> On the ATMEL at91 when using the non-removable flag in device tree >> and not using the card-detect pin inside the device-tree pinctrl, >> the card detect pin is physically still used which can cause >> unknown behaviour when this pin is used for other purposes. Hi Mathieu, On which SoC(s) exactly, has this behaviour been observed? Also, has this issue been discussed in any separate support request, in such case we could retrieve some background from it? By "unknown behaviour", do you mean "the card insertion status would follow whatever electrical level is seen on the card-detect pin"? >> >> From my interpretation this seems to be caused by a hardware design >> flaw and the real hardware is not working as intended by the >> documentation. >> >> >> Signed-off-by: Mathieu Moneyron <mathieu.moneyron@gmail.com> >> >> --- >> drivers/mmc/host/sdhci-of-at91.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-of-at91.c >> b/drivers/mmc/host/sdhci-of-at91.c >> index 69fef88e7..4fd6bfbf6 100644 >> --- a/drivers/mmc/host/sdhci-of-at91.c >> +++ b/drivers/mmc/host/sdhci-of-at91.c >> @@ -51,10 +51,15 @@ struct sdhci_at91_priv { >> static void sdhci_at91_set_force_card_detect(struct sdhci_host *host) >> { >> u8 mc1r; >> + u8 ctrl; >> >> mc1r = readb(host->ioaddr + SDMMC_MC1R); >> mc1r |= SDMMC_MC1R_FCD; >> writeb(mc1r, host->ioaddr + SDMMC_MC1R); >> + >> + ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL); >> + ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN; >> + writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); >> } >> >> static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned >> int clock) Kind regards, Aubin
Hi Aubin, Thanks for your feedback. I'll try my best to give clear explanations. On Sat, Sep 23, 2023 at 2:44 PM Aubin Constans <aubin.constans@microchip.com> wrote: > > On 04/09/2023 09:38, Adrian Hunter wrote: > > + Eugen Hristev > > On 30/08/23 12:23, mathieu wrote: >> On the ATMEL at91 when using > the non-removable flag in device tree >> and not using the card-detect > pin inside the device-tree pinctrl, >> the card detect pin is physically > still used which can cause >> unknown behaviour when this pin is used > for other purposes. > Hi Mathieu, > > On which SoC(s) exactly, has this behaviour been observed? This has been observed on SAMA5D27. > > > Also, has this issue been discussed in any separate support request, in such > case we could retrieve some background from it? > > By "unknown behaviour", do you mean "the card insertion status would > follow whatever electrical level is seen on the card-detect pin"? For instance our board design has the PA13 (card detect) pin wired to drive a 3v3 power supply. The fixed regulator driver configure this pin as output (described in the device tree). But what I observed is that the sdmmc driver still want to use PA13 as the card detect signal which is then configured as an input. This cause the 3v3 power to drop. By "unknown behavior" I mean the configuration of this pin as card detect sets unwanted electrical levels to what's connected to this pin. > > > >> >> From my interpretation this seems to be caused by a hardware design > >> flaw and the real hardware is not working as intended by the >> > documentation. >> > >> > >> Signed-off-by: Mathieu Moneyron <mathieu.moneyron@gmail.com> > >> > >> --- > >> drivers/mmc/host/sdhci-of-at91.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/mmc/host/sdhci-of-at91.c > >> b/drivers/mmc/host/sdhci-of-at91.c > >> index 69fef88e7..4fd6bfbf6 100644 > >> --- a/drivers/mmc/host/sdhci-of-at91.c > >> +++ b/drivers/mmc/host/sdhci-of-at91.c > >> @@ -51,10 +51,15 @@ struct sdhci_at91_priv { > >> static void sdhci_at91_set_force_card_detect(struct sdhci_host *host) > >> { > >> u8 mc1r; > >> + u8 ctrl; > >> > >> mc1r = readb(host->ioaddr + SDMMC_MC1R); > >> mc1r |= SDMMC_MC1R_FCD; > >> writeb(mc1r, host->ioaddr + SDMMC_MC1R); > >> + > >> + ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL); > >> + ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN; > >> + writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); > >> } > >> > >> static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned > >> int clock) > > Kind regards, > Aubin > Kind regards, Mathieu
On 23/09/2023 16:23, Mathieu Moneyron wrote: > Hi Aubin, > > Thanks for your feedback. > I'll try my best to give clear explanations. > > On Sat, Sep 23, 2023 at 2:44 PM Aubin Constans > <aubin.constans@microchip.com> wrote: >> >> On 04/09/2023 09:38, Adrian Hunter wrote: >>> + Eugen Hristev >>> On 30/08/23 12:23, mathieu wrote: >>>> On the ATMEL at91 when using the non-removable flag in device tree and not >>>> using the card-detect pin inside the device-tree pinctrl, the card detect >>>> pin is physically still used which can cause unknown behaviour when this >>>> pin is used for other purposes. >> Hi Mathieu, >> >> On which SoC(s) exactly, has this behaviour been observed? > > This has been observed on SAMA5D27. >> >> >> Also, has this issue been discussed in any separate support request, in such >> case we could retrieve some background from it? >> >> By "unknown behaviour", do you mean "the card insertion status would >> follow whatever electrical level is seen on the card-detect pin"? > > For instance our board design has the PA13 (card detect) pin wired to drive a > 3v3 power supply. > The fixed regulator driver configure this pin as output (described in the device > tree). > But what I observed is that the sdmmc driver still want to use PA13 as the card > detect signal which is then configured as an input. This cause the 3v3 power to > drop. In the device tree, has PIN_PA13__SDMMC0_CD been removed from the pinctrl for sdmmc0? In case the above behaviour was due to hardware, it would likely point out a flaw in the Parallel Input/Output Controller (PIO) rather than in the SDMMC Controller. Would you read back the relevant PIO_CFGRx register, with only "PA13" set in PIO_MSKRx, and provide me with the value read? Assuming PA13 is Non-Secure; if PA13 is configured as Secure, S_PIO_CFGRx and S_PIO_MSKRx should be accessed instead. > By "unknown behavior" I mean the configuration of this pin as card detect sets > unwanted electrical levels to what's connected to this pin. >> >> >>>> From my interpretation this seems to be caused by a hardware design flaw >>>> and the real hardware is not working as intended by the documentation. >>>> >>>> Signed-off-by: Mathieu Moneyron <mathieu.moneyron@gmail.com> >>>> >>>> --- >>>> drivers/mmc/host/sdhci-of-at91.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-of-at91.c >>>> b/drivers/mmc/host/sdhci-of-at91.c >>>> index 69fef88e7..4fd6bfbf6 100644 >>>> --- a/drivers/mmc/host/sdhci-of-at91.c >>>> +++ b/drivers/mmc/host/sdhci-of-at91.c >>>> @@ -51,10 +51,15 @@ struct sdhci_at91_priv { >>>> static void sdhci_at91_set_force_card_detect(struct sdhci_host *host) >>>> { >>>> u8 mc1r; >>>> + u8 ctrl; >>>> >>>> mc1r = readb(host->ioaddr + SDMMC_MC1R); >>>> mc1r |= SDMMC_MC1R_FCD; >>>> writeb(mc1r, host->ioaddr + SDMMC_MC1R); >>>> + >>>> + ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL); >>>> + ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN; >>>> + writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); >>>> } >>>> >>>> static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned >>>> int clock) >> >> Kind regards, >> Aubin >> > Kind regards, > Mathieu Thank you, Aubin
On Fri, Nov 10, 2023 at 8:57 PM Aubin Constans <aubin.constans@microchip.com> wrote: > > On 23/09/2023 16:23, Mathieu Moneyron wrote: > > In the device tree, has PIN_PA13__SDMMC0_CD been removed from the > pinctrl for sdmmc0? Yes the pin is not present in the pinctrl. Only PA0 to PA10 configured as SDMMC0 function. > > In case the above behaviour was due to hardware, it would likely point > out a flaw in the Parallel Input/Output Controller (PIO) rather than in > the SDMMC Controller. > > Would you read back the relevant PIO_CFGRx register, with only "PA13" > set in PIO_MSKRx, and provide me with the value read? > Assuming PA13 is Non-Secure; if PA13 is configured as Secure, > S_PIO_CFGRx and S_PIO_MSKRx should be accessed instead. I'm not sure if that's the best way to achieve this but I enabled debug messages in pinctrl-at91-pio4 driver. The messages I got regarding the PA13 pin is the following: 'enable pin 13 as GPIO' With PIO_CFGR0 register for PA13 only is 0x0400. Also from PA0 to PA10 the pins used for the in the mmc pinctrl: 'enable function A group PA0' For all these pins funcion A is SDMMC0. Also as a side note, I will no longer have access to the hardware from the beginning of december as I have resigned from my current job. Best regards, Mathieu
diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c index 69fef88e7..4fd6bfbf6 100644 --- a/drivers/mmc/host/sdhci-of-at91.c +++ b/drivers/mmc/host/sdhci-of-at91.c @@ -51,10 +51,15 @@ struct sdhci_at91_priv { static void sdhci_at91_set_force_card_detect(struct sdhci_host *host) { u8 mc1r; + u8 ctrl; mc1r = readb(host->ioaddr + SDMMC_MC1R); mc1r |= SDMMC_MC1R_FCD; writeb(mc1r, host->ioaddr + SDMMC_MC1R); + + ctrl = readb(host->ioaddr + SDHCI_HOST_CONTROL); + ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN; + writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); } static void sdhci_at91_set_clock(struct sdhci_host *host, unsigned int clock)