diff mbox series

mmc: sdhci: Rework SDHCI_QUIRK_BROKEN_R1B

Message ID 20231027205702.1549693-1-sean.anderson@seco.com
State Accepted
Commit 21c84bb1112695f9bd49379f7e32c251b55a3cad
Delegated to: Jaehoon Chung
Headers show
Series mmc: sdhci: Rework SDHCI_QUIRK_BROKEN_R1B | expand

Commit Message

Sean Anderson Oct. 27, 2023, 8:57 p.m. UTC 
As noted in commit 3a6383207be ("mmc: sdhci: add the quirk for broken
r1b response"), some MMC controllers don't always set the transfer
complete bit with R1b responses.

According to the SD Host Controller Simplified Specification v4.20,

> In the case of a command pairing with response-with-busy[, Transfer
> Complete] is set when busy is de-asserted. Refer to DAT Line Active
> and Command Inhibit (DAT) in the Present State register.

By polling the DAT Line Active bit in the present state register, we can
detect when we are no longer busy, without waiting for a long timeout.
This results in much faster reads/writes on buggy controllers.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
I've CC'd a few people who've added this quirk to controllers recently.
Please let me know if your hardware still works. It's possible that my
hardware is buggy in a different way.

 drivers/mmc/sdhci.c | 19 ++++++++++++-------
 include/sdhci.h     |  1 +
 2 files changed, 13 insertions(+), 7 deletions(-)

Comments

Henrik Grimler Oct. 28, 2023, 8:31 p.m. UTC | #1 
Hi Sean,

Tested on odroid-u2 (exynos4412-odroid) on top of 2024.01-rc1, still
works fine, so feel free to add:

Tested-by: Henrik Grimler <henrik@grimler.se>

Best regards,
Henrik Grimler

On Fri, Oct 27, 2023 at 04:57:03PM -0400, Sean Anderson wrote:
> As noted in commit 3a6383207be ("mmc: sdhci: add the quirk for broken
> r1b response"), some MMC controllers don't always set the transfer
> complete bit with R1b responses.
> 
> According to the SD Host Controller Simplified Specification v4.20,
> 
> > In the case of a command pairing with response-with-busy[, Transfer
> > Complete] is set when busy is de-asserted. Refer to DAT Line Active
> > and Command Inhibit (DAT) in the Present State register.
> 
> By polling the DAT Line Active bit in the present state register, we can
> detect when we are no longer busy, without waiting for a long timeout.
> This results in much faster reads/writes on buggy controllers.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> I've CC'd a few people who've added this quirk to controllers recently.
> Please let me know if your hardware still works. It's possible that my
> hardware is buggy in a different way.
> 
>  drivers/mmc/sdhci.c | 19 ++++++++++++-------
>  include/sdhci.h     |  1 +
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index fc9c6c37996..0178ed8a11e 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -306,14 +306,19 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
>  		if (stat & SDHCI_INT_ERROR)
>  			break;
>  
> -		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
> -			if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) {
> +		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B &&
> +		    cmd->resp_type & MMC_RSP_BUSY && !data) {
> +			unsigned int state =
> +				sdhci_readl(host, SDHCI_PRESENT_STATE);
> +
> +			if (!(state & SDHCI_DAT_ACTIVE))
>  				return 0;
> -			} else {
> -				printf("%s: Timeout for status update!\n",
> -				       __func__);
> -				return -ETIMEDOUT;
> -			}
> +		}
> +
> +		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
> +			printf("%s: Timeout for status update: %08x %08x\n",
> +			       __func__, stat, mask);
> +			return -ETIMEDOUT;
>  		}
>  	} while ((stat & mask) != mask);
>  
> diff --git a/include/sdhci.h b/include/sdhci.h
> index 70fefca2a97..a1b74e3bd79 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -57,6 +57,7 @@
>  #define SDHCI_PRESENT_STATE	0x24
>  #define  SDHCI_CMD_INHIBIT	BIT(0)
>  #define  SDHCI_DATA_INHIBIT	BIT(1)
> +#define  SDHCI_DAT_ACTIVE	BIT(2)
>  #define  SDHCI_DOING_WRITE	BIT(8)
>  #define  SDHCI_DOING_READ	BIT(9)
>  #define  SDHCI_SPACE_AVAILABLE	BIT(10)
> -- 
> 2.35.1.1320.gc452695387.dirty
>
Jaehoon Chung Nov. 1, 2023, 1:08 a.m. UTC | #2 
Hi,

> -----Original Message-----
> From: Henrik Grimler <henrik@grimler.se>
> Sent: Sunday, October 29, 2023 5:31 AM
> To: Sean Anderson <sean.anderson@seco.com>
> Cc: u-boot@lists.denx.de; Jaehoon Chung <jh80.chung@samsung.com>; Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com>; Bharat Kumar Reddy Gooty <bharat.gooty@broadcom.com>
> Subject: Re: [PATCH] mmc: sdhci: Rework SDHCI_QUIRK_BROKEN_R1B
> 
> Hi Sean,
> 
> Tested on odroid-u2 (exynos4412-odroid) on top of 2024.01-rc1, still
> works fine, so feel free to add:
> 
> Tested-by: Henrik Grimler <henrik@grimler.se>
> 
> Best regards,
> Henrik Grimler
> 
> On Fri, Oct 27, 2023 at 04:57:03PM -0400, Sean Anderson wrote:
> > As noted in commit 3a6383207be ("mmc: sdhci: add the quirk for broken
> > r1b response"), some MMC controllers don't always set the transfer
> > complete bit with R1b responses.
> >
> > According to the SD Host Controller Simplified Specification v4.20,
> >
> > > In the case of a command pairing with response-with-busy[, Transfer
> > > Complete] is set when busy is de-asserted. Refer to DAT Line Active
> > > and Command Inhibit (DAT) in the Present State register.
> >
> > By polling the DAT Line Active bit in the present state register, we can
> > detect when we are no longer busy, without waiting for a long timeout.
> > This results in much faster reads/writes on buggy controllers.
> >
> > Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> > ---
> > I've CC'd a few people who've added this quirk to controllers recently.
> > Please let me know if your hardware still works. It's possible that my
> > hardware is buggy in a different way.
> >
> >  drivers/mmc/sdhci.c | 19 ++++++++++++-------
> >  include/sdhci.h     |  1 +
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> > index fc9c6c37996..0178ed8a11e 100644
> > --- a/drivers/mmc/sdhci.c
> > +++ b/drivers/mmc/sdhci.c
> > @@ -306,14 +306,19 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> >  		if (stat & SDHCI_INT_ERROR)
> >  			break;
> >
> > -		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
> > -			if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) {
> > +		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B &&
> > +		    cmd->resp_type & MMC_RSP_BUSY && !data) {
> > +			unsigned int state =
> > +				sdhci_readl(host, SDHCI_PRESENT_STATE);
> > +
> > +			if (!(state & SDHCI_DAT_ACTIVE))
> >  				return 0;
> > -			} else {
> > -				printf("%s: Timeout for status update!\n",
> > -				       __func__);
> > -				return -ETIMEDOUT;
> > -			}
> > +		}
> > +
> > +		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
> > +			printf("%s: Timeout for status update: %08x %08x\n",
> > +			       __func__, stat, mask);
> > +			return -ETIMEDOUT;
> >  		}
> >  	} while ((stat & mask) != mask);
> >
> > diff --git a/include/sdhci.h b/include/sdhci.h
> > index 70fefca2a97..a1b74e3bd79 100644
> > --- a/include/sdhci.h
> > +++ b/include/sdhci.h
> > @@ -57,6 +57,7 @@
> >  #define SDHCI_PRESENT_STATE	0x24
> >  #define  SDHCI_CMD_INHIBIT	BIT(0)
> >  #define  SDHCI_DATA_INHIBIT	BIT(1)
> > +#define  SDHCI_DAT_ACTIVE	BIT(2)
> >  #define  SDHCI_DOING_WRITE	BIT(8)
> >  #define  SDHCI_DOING_READ	BIT(9)
> >  #define  SDHCI_SPACE_AVAILABLE	BIT(10)
> > --
> > 2.35.1.1320.gc452695387.dirty
> >
Jaehoon Chung Nov. 1, 2023, 5:45 a.m. UTC | #3 
> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Jaehoon Chung
> Sent: Wednesday, November 1, 2023 10:09 AM
> To: 'Henrik Grimler' <henrik@grimler.se>; 'Sean Anderson' <sean.anderson@seco.com>
> Cc: u-boot@lists.denx.de; 'Rayagonda Kokatanur' <rayagonda.kokatanur@broadcom.com>; 'Bharat Kumar
> Reddy Gooty' <bharat.gooty@broadcom.com>
> Subject: RE: [PATCH] mmc: sdhci: Rework SDHCI_QUIRK_BROKEN_R1B
> 
> Hi,
> 
> > -----Original Message-----
> > From: Henrik Grimler <henrik@grimler.se>
> > Sent: Sunday, October 29, 2023 5:31 AM
> > To: Sean Anderson <sean.anderson@seco.com>
> > Cc: u-boot@lists.denx.de; Jaehoon Chung <jh80.chung@samsung.com>; Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com>; Bharat Kumar Reddy Gooty <bharat.gooty@broadcom.com>
> > Subject: Re: [PATCH] mmc: sdhci: Rework SDHCI_QUIRK_BROKEN_R1B
> >
> > Hi Sean,
> >
> > Tested on odroid-u2 (exynos4412-odroid) on top of 2024.01-rc1, still
> > works fine, so feel free to add:
> >
> > Tested-by: Henrik Grimler <henrik@grimler.se>
> >
> > Best regards,
> > Henrik Grimler
> >
> > On Fri, Oct 27, 2023 at 04:57:03PM -0400, Sean Anderson wrote:
> > > As noted in commit 3a6383207be ("mmc: sdhci: add the quirk for broken
> > > r1b response"), some MMC controllers don't always set the transfer
> > > complete bit with R1b responses.
> > >
> > > According to the SD Host Controller Simplified Specification v4.20,
> > >
> > > > In the case of a command pairing with response-with-busy[, Transfer
> > > > Complete] is set when busy is de-asserted. Refer to DAT Line Active
> > > > and Command Inhibit (DAT) in the Present State register.
> > >
> > > By polling the DAT Line Active bit in the present state register, we can
> > > detect when we are no longer busy, without waiting for a long timeout.
> > > This results in much faster reads/writes on buggy controllers.
> > >
> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> 
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Applied to u-boot-mmc/master. Thanks!

Best Regards,
Jaehoon Chung

> 
> Best Regards,
> Jaehoon Chung
> 
> > > ---
> > > I've CC'd a few people who've added this quirk to controllers recently.
> > > Please let me know if your hardware still works. It's possible that my
> > > hardware is buggy in a different way.
> > >
> > >  drivers/mmc/sdhci.c | 19 ++++++++++++-------
> > >  include/sdhci.h     |  1 +
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> > > index fc9c6c37996..0178ed8a11e 100644
> > > --- a/drivers/mmc/sdhci.c
> > > +++ b/drivers/mmc/sdhci.c
> > > @@ -306,14 +306,19 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
> > >  		if (stat & SDHCI_INT_ERROR)
> > >  			break;
> > >
> > > -		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
> > > -			if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) {
> > > +		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B &&
> > > +		    cmd->resp_type & MMC_RSP_BUSY && !data) {
> > > +			unsigned int state =
> > > +				sdhci_readl(host, SDHCI_PRESENT_STATE);
> > > +
> > > +			if (!(state & SDHCI_DAT_ACTIVE))
> > >  				return 0;
> > > -			} else {
> > > -				printf("%s: Timeout for status update!\n",
> > > -				       __func__);
> > > -				return -ETIMEDOUT;
> > > -			}
> > > +		}
> > > +
> > > +		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
> > > +			printf("%s: Timeout for status update: %08x %08x\n",
> > > +			       __func__, stat, mask);
> > > +			return -ETIMEDOUT;
> > >  		}
> > >  	} while ((stat & mask) != mask);
> > >
> > > diff --git a/include/sdhci.h b/include/sdhci.h
> > > index 70fefca2a97..a1b74e3bd79 100644
> > > --- a/include/sdhci.h
> > > +++ b/include/sdhci.h
> > > @@ -57,6 +57,7 @@
> > >  #define SDHCI_PRESENT_STATE	0x24
> > >  #define  SDHCI_CMD_INHIBIT	BIT(0)
> > >  #define  SDHCI_DATA_INHIBIT	BIT(1)
> > > +#define  SDHCI_DAT_ACTIVE	BIT(2)
> > >  #define  SDHCI_DOING_WRITE	BIT(8)
> > >  #define  SDHCI_DOING_READ	BIT(9)
> > >  #define  SDHCI_SPACE_AVAILABLE	BIT(10)
> > > --
> > > 2.35.1.1320.gc452695387.dirty
> > >
diff mbox series

Patch

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index fc9c6c37996..0178ed8a11e 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -306,14 +306,19 @@  static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		if (stat & SDHCI_INT_ERROR)
 			break;
 
-		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
-			if (host->quirks & SDHCI_QUIRK_BROKEN_R1B) {
+		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B &&
+		    cmd->resp_type & MMC_RSP_BUSY && !data) {
+			unsigned int state =
+				sdhci_readl(host, SDHCI_PRESENT_STATE);
+
+			if (!(state & SDHCI_DAT_ACTIVE))
 				return 0;
-			} else {
-				printf("%s: Timeout for status update!\n",
-				       __func__);
-				return -ETIMEDOUT;
-			}
+		}
+
+		if (get_timer(start) >= SDHCI_READ_STATUS_TIMEOUT) {
+			printf("%s: Timeout for status update: %08x %08x\n",
+			       __func__, stat, mask);
+			return -ETIMEDOUT;
 		}
 	} while ((stat & mask) != mask);
 
diff --git a/include/sdhci.h b/include/sdhci.h
index 70fefca2a97..a1b74e3bd79 100644
--- a/include/sdhci.h
+++ b/include/sdhci.h
@@ -57,6 +57,7 @@ 
 #define SDHCI_PRESENT_STATE	0x24
 #define  SDHCI_CMD_INHIBIT	BIT(0)
 #define  SDHCI_DATA_INHIBIT	BIT(1)
+#define  SDHCI_DAT_ACTIVE	BIT(2)
 #define  SDHCI_DOING_WRITE	BIT(8)
 #define  SDHCI_DOING_READ	BIT(9)
 #define  SDHCI_SPACE_AVAILABLE	BIT(10)