RE: [PATCH] mmc: sdhci: Rework SDHCI_QUIRK_BROKEN_R1B

["Jaehoon Chung" <jh80.chung@samsung.com>]

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