| Message ID | 20210812145056.11916-1-marten.lindahl@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: usdhi6rol0: Implement card_busy function | expand |
On Thu, 12 Aug 2021 at 16:51, Mårten Lindahl <marten.lindahl@axis.com> wrote: > > When switching card voltage to UHS voltage the mmc framework tries to > check that the card is driving pins CMD, and DAT[0-3] low before the > switch is made. Drivers that does not implement the card_busy function > will manage to do the switch anyway, but the framework will print a > warning about not being able to verify the voltage signal. > > Implement card_busy function. Renesas host interface only exposes pins > DAT[0] and DAT[3] for reading the busy state, which is why only these > two pins are checked. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > drivers/mmc/host/usdhi6rol0.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c > index b9b79b1089a0..e400a646e675 100644 > --- a/drivers/mmc/host/usdhi6rol0.c > +++ b/drivers/mmc/host/usdhi6rol0.c > @@ -77,6 +77,7 @@ > #define USDHI6_SD_INFO1_WP BIT(7) > #define USDHI6_SD_INFO1_D3_CARD_OUT BIT(8) > #define USDHI6_SD_INFO1_D3_CARD_IN BIT(9) > +#define USDHI6_SD_INFO1_SDDAT3 BIT(10) > > #define USDHI6_SD_INFO2_CMD_ERR BIT(0) > #define USDHI6_SD_INFO2_CRC_ERR BIT(1) > @@ -1186,6 +1187,21 @@ static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) > return ret; > } > > +static int usdhi6_card_busy(struct mmc_host *mmc) > +{ > + struct usdhi6_host *host = mmc_priv(mmc); > + u32 info1 = usdhi6_read(host, USDHI6_SD_INFO1); > + u32 info2 = usdhi6_read(host, USDHI6_SD_INFO2); > + > + /* Check if the SD bus is processing a command */ Hmm, this sounds a bit confusing to me. Don't you want to say something like "the clock logic is enabled". The point is, from the mmc core point of view, ->card_busy() is called to poll for busy completion *after* a command has been processed. > + if (!(info2 & USDHI6_SD_INFO2_SCLKDIVEN)) > + return 0; > + > + /* Card is busy if it is pulling dat[0] & dat[3] low */ > + return !(info2 & USDHI6_SD_INFO2_SDDAT0 || > + info1 & USDHI6_SD_INFO1_SDDAT3); In fact, it's sufficient to monitor any one of the DATA lines to check for busy for the UHS-I switch, according to the SD spec. However, since we are using ->card_busy() to monitor also other busy signal scenarios, like for some commands with R1B responses that assert only DAT0 to signal busy (at least according to specs), I think it's better to monitor solely DAT0 here. I realize that the comment for the ->card_busy() callback in include/linux/mmc/host.h needs to be updated to clarify this as well. > +} > + > static const struct mmc_host_ops usdhi6_ops = { > .request = usdhi6_request, > .set_ios = usdhi6_set_ios, > @@ -1193,6 +1209,7 @@ static const struct mmc_host_ops usdhi6_ops = { > .get_ro = usdhi6_get_ro, > .enable_sdio_irq = usdhi6_enable_sdio_irq, > .start_signal_voltage_switch = usdhi6_sig_volt_switch, > + .card_busy = usdhi6_card_busy, > }; > > /* State machine handlers */ > -- > 2.20.1 > Kind regards Uffe
Hi Ulf! Thank you for your comments! I will update the patch. On Fri, Aug 13, 2021 at 11:25:35AM +0200, Ulf Hansson wrote: > On Thu, 12 Aug 2021 at 16:51, Mårten Lindahl <marten.lindahl@axis.com> wrote: > > > > When switching card voltage to UHS voltage the mmc framework tries to > > check that the card is driving pins CMD, and DAT[0-3] low before the > > switch is made. Drivers that does not implement the card_busy function > > will manage to do the switch anyway, but the framework will print a > > warning about not being able to verify the voltage signal. > > > > Implement card_busy function. Renesas host interface only exposes pins > > DAT[0] and DAT[3] for reading the busy state, which is why only these > > two pins are checked. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > drivers/mmc/host/usdhi6rol0.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c > > index b9b79b1089a0..e400a646e675 100644 > > --- a/drivers/mmc/host/usdhi6rol0.c > > +++ b/drivers/mmc/host/usdhi6rol0.c > > @@ -77,6 +77,7 @@ > > #define USDHI6_SD_INFO1_WP BIT(7) > > #define USDHI6_SD_INFO1_D3_CARD_OUT BIT(8) > > #define USDHI6_SD_INFO1_D3_CARD_IN BIT(9) > > +#define USDHI6_SD_INFO1_SDDAT3 BIT(10) > > > > #define USDHI6_SD_INFO2_CMD_ERR BIT(0) > > #define USDHI6_SD_INFO2_CRC_ERR BIT(1) > > @@ -1186,6 +1187,21 @@ static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) > > return ret; > > } > > > > +static int usdhi6_card_busy(struct mmc_host *mmc) > > +{ > > + struct usdhi6_host *host = mmc_priv(mmc); > > + u32 info1 = usdhi6_read(host, USDHI6_SD_INFO1); > > + u32 info2 = usdhi6_read(host, USDHI6_SD_INFO2); > > + > > + /* Check if the SD bus is processing a command */ > > Hmm, this sounds a bit confusing to me. Don't you want to say > something like "the clock logic is enabled". > > The point is, from the mmc core point of view, ->card_busy() is called > to poll for busy completion *after* a command has been processed. > Thanks. After reading the SD specification my understanding is that it should be enough to solely check DAT0 in this case. I will remove the check for SCLKDIVEN. > > + if (!(info2 & USDHI6_SD_INFO2_SCLKDIVEN)) > > + return 0; > > + > > + /* Card is busy if it is pulling dat[0] & dat[3] low */ > > + return !(info2 & USDHI6_SD_INFO2_SDDAT0 || > > + info1 & USDHI6_SD_INFO1_SDDAT3); > > In fact, it's sufficient to monitor any one of the DATA lines to check > for busy for the UHS-I switch, according to the SD spec. > > However, since we are using ->card_busy() to monitor also other busy > signal scenarios, like for some commands with R1B responses that > assert only DAT0 to signal busy (at least according to specs), I think > it's better to monitor solely DAT0 here. > I will remove the check for DAT3. > I realize that the comment for the ->card_busy() callback in > include/linux/mmc/host.h needs to be updated to clarify this as well. > I will update the card_busy callback comment to DAT0 only. Kind regards Mårten > > +} > > + > > static const struct mmc_host_ops usdhi6_ops = { > > .request = usdhi6_request, > > .set_ios = usdhi6_set_ios, > > @@ -1193,6 +1209,7 @@ static const struct mmc_host_ops usdhi6_ops = { > > .get_ro = usdhi6_get_ro, > > .enable_sdio_irq = usdhi6_enable_sdio_irq, > > .start_signal_voltage_switch = usdhi6_sig_volt_switch, > > + .card_busy = usdhi6_card_busy, > > }; > > > > /* State machine handlers */ > > -- > > 2.20.1 > > > > Kind regards > Uffe
diff --git a/drivers/mmc/host/usdhi6rol0.c b/drivers/mmc/host/usdhi6rol0.c index b9b79b1089a0..e400a646e675 100644 --- a/drivers/mmc/host/usdhi6rol0.c +++ b/drivers/mmc/host/usdhi6rol0.c @@ -77,6 +77,7 @@ #define USDHI6_SD_INFO1_WP BIT(7) #define USDHI6_SD_INFO1_D3_CARD_OUT BIT(8) #define USDHI6_SD_INFO1_D3_CARD_IN BIT(9) +#define USDHI6_SD_INFO1_SDDAT3 BIT(10) #define USDHI6_SD_INFO2_CMD_ERR BIT(0) #define USDHI6_SD_INFO2_CRC_ERR BIT(1) @@ -1186,6 +1187,21 @@ static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios) return ret; } +static int usdhi6_card_busy(struct mmc_host *mmc) +{ + struct usdhi6_host *host = mmc_priv(mmc); + u32 info1 = usdhi6_read(host, USDHI6_SD_INFO1); + u32 info2 = usdhi6_read(host, USDHI6_SD_INFO2); + + /* Check if the SD bus is processing a command */ + if (!(info2 & USDHI6_SD_INFO2_SCLKDIVEN)) + return 0; + + /* Card is busy if it is pulling dat[0] & dat[3] low */ + return !(info2 & USDHI6_SD_INFO2_SDDAT0 || + info1 & USDHI6_SD_INFO1_SDDAT3); +} + static const struct mmc_host_ops usdhi6_ops = { .request = usdhi6_request, .set_ios = usdhi6_set_ios, @@ -1193,6 +1209,7 @@ static const struct mmc_host_ops usdhi6_ops = { .get_ro = usdhi6_get_ro, .enable_sdio_irq = usdhi6_enable_sdio_irq, .start_signal_voltage_switch = usdhi6_sig_volt_switch, + .card_busy = usdhi6_card_busy, }; /* State machine handlers */
When switching card voltage to UHS voltage the mmc framework tries to check that the card is driving pins CMD, and DAT[0-3] low before the switch is made. Drivers that does not implement the card_busy function will manage to do the switch anyway, but the framework will print a warning about not being able to verify the voltage signal. Implement card_busy function. Renesas host interface only exposes pins DAT[0] and DAT[3] for reading the busy state, which is why only these two pins are checked. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/mmc/host/usdhi6rol0.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)