diff mbox series

mmc: usdhi6rol0: Implement card_busy function

Message ID 20210812145056.11916-1-marten.lindahl@axis.com
State New
Headers show
Series mmc: usdhi6rol0: Implement card_busy function | expand

Commit Message

Mårten Lindahl Aug. 12, 2021, 2:50 p.m. UTC
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(+)

Comments

Ulf Hansson Aug. 13, 2021, 9:25 a.m. UTC | #1
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
Marten Lindahl Aug. 16, 2021, 12:26 p.m. UTC | #2
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 mbox series

Patch

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				*/