diff mbox series

mmc: sdhci-esdhc-imx: correct the tuning start tap and step setting

Message ID 20221207112315.1812222-1-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci-esdhc-imx: correct the tuning start tap and step setting | expand

Commit Message

Bough Chen Dec. 7, 2022, 11:23 a.m. UTC 
From: Haibo Chen <haibo.chen@nxp.com>

Current code logic may be impacted by the setting of ROM/Bootloader,
so unmask these bits first, then setting these bits accordingly.

Fixes: 2b16cf326b70 ("mmc: sdhci-esdhc-imx: move tuning static configuration into hwinit function")
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Fabio Estevam Dec. 7, 2022, 11:49 a.m. UTC | #1 
[Adding Kevin]

Not sure if this solve the -84 write error when using ath10k that
Kevin reported.

On Wed, Dec 7, 2022 at 8:23 AM <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> Current code logic may be impacted by the setting of ROM/Bootloader,
> so unmask these bits first, then setting these bits accordingly.
>
> Fixes: 2b16cf326b70 ("mmc: sdhci-esdhc-imx: move tuning static configuration into hwinit function")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 89ef0c80ac37..9e73c34b6401 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -107,6 +107,7 @@
>  #define ESDHC_TUNING_START_TAP_DEFAULT 0x1
>  #define ESDHC_TUNING_START_TAP_MASK    0x7f
>  #define ESDHC_TUNING_CMD_CRC_CHECK_DISABLE     (1 << 7)
> +#define ESDHC_TUNING_STEP_DEFAULT      0x1
>  #define ESDHC_TUNING_STEP_MASK         0x00070000
>  #define ESDHC_TUNING_STEP_SHIFT                16
>
> @@ -1368,7 +1369,7 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>         struct cqhci_host *cq_host = host->mmc->cqe_private;
> -       int tmp;
> +       u32 tmp;
>
>         if (esdhc_is_usdhc(imx_data)) {
>                 /*
> @@ -1423,17 +1424,24 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>
>                 if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
>                         tmp = readl(host->ioaddr + ESDHC_TUNING_CTRL);
> -                       tmp |= ESDHC_STD_TUNING_EN |
> -                               ESDHC_TUNING_START_TAP_DEFAULT;
> -                       if (imx_data->boarddata.tuning_start_tap) {
> -                               tmp &= ~ESDHC_TUNING_START_TAP_MASK;
> +                       tmp |= ESDHC_STD_TUNING_EN;
> +
> +                       /*
> +                        * ROM code or bootloader may config the start tap
> +                        * and step, unmask them first.
> +                        */
> +                       tmp &= ~(ESDHC_TUNING_START_TAP_MASK | ESDHC_TUNING_STEP_MASK);
> +                       if (imx_data->boarddata.tuning_start_tap)
>                                 tmp |= imx_data->boarddata.tuning_start_tap;
> -                       }
> +                       else
> +                               tmp |= ESDHC_TUNING_START_TAP_DEFAULT;
>
>                         if (imx_data->boarddata.tuning_step) {
> -                               tmp &= ~ESDHC_TUNING_STEP_MASK;
>                                 tmp |= imx_data->boarddata.tuning_step
>                                         << ESDHC_TUNING_STEP_SHIFT;
> +                       } else {
> +                               tmp |= ESDHC_TUNING_STEP_DEFAULT
> +                                       << ESDHC_TUNING_STEP_SHIFT;
>                         }
>
>                         /* Disable the CMD CRC check for tuning, if not, need to
> --
> 2.34.1
>
Kevin Groeneveld Dec. 20, 2022, 2:55 p.m. UTC | #2 
On 2022-12-07 06:49, Fabio Estevam wrote:
> [Adding Kevin]
> 
> Not sure if this solve the -84 write error when using ath10k that
> Kevin reported.

Thanks for the suggestion and adding me. I have not had much time to 
work on this lately but I did dig into this patch a bit today.

This patch has no impact in my situation for a couple reasons:

1. I verified my boot loader is not changing the defaults (at least on 
the interface used for WiFi, it is changing them for eMMC interface).

2. The mainline imx8mm.dtsi file defines fsl,tuning-start-tap and 
fsl,tuning-step in which case I do not think this patch makes any 
difference as the code was already masking the bits in question in that 
case.


Kevin
Adrian Hunter Dec. 29, 2022, 6:44 a.m. UTC | #3 
On 7/12/22 13:23, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> Current code logic may be impacted by the setting of ROM/Bootloader,
> so unmask these bits first, then setting these bits accordingly.
> 
> Fixes: 2b16cf326b70 ("mmc: sdhci-esdhc-imx: move tuning static configuration into hwinit function")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 89ef0c80ac37..9e73c34b6401 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -107,6 +107,7 @@
>  #define ESDHC_TUNING_START_TAP_DEFAULT	0x1
>  #define ESDHC_TUNING_START_TAP_MASK	0x7f
>  #define ESDHC_TUNING_CMD_CRC_CHECK_DISABLE	(1 << 7)
> +#define ESDHC_TUNING_STEP_DEFAULT	0x1
>  #define ESDHC_TUNING_STEP_MASK		0x00070000
>  #define ESDHC_TUNING_STEP_SHIFT		16
>  
> @@ -1368,7 +1369,7 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>  	struct cqhci_host *cq_host = host->mmc->cqe_private;
> -	int tmp;
> +	u32 tmp;
>  
>  	if (esdhc_is_usdhc(imx_data)) {
>  		/*
> @@ -1423,17 +1424,24 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>  
>  		if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
>  			tmp = readl(host->ioaddr + ESDHC_TUNING_CTRL);
> -			tmp |= ESDHC_STD_TUNING_EN |
> -				ESDHC_TUNING_START_TAP_DEFAULT;
> -			if (imx_data->boarddata.tuning_start_tap) {
> -				tmp &= ~ESDHC_TUNING_START_TAP_MASK;
> +			tmp |= ESDHC_STD_TUNING_EN;
> +
> +			/*
> +			 * ROM code or bootloader may config the start tap
> +			 * and step, unmask them first.
> +			 */
> +			tmp &= ~(ESDHC_TUNING_START_TAP_MASK | ESDHC_TUNING_STEP_MASK);
> +			if (imx_data->boarddata.tuning_start_tap)
>  				tmp |= imx_data->boarddata.tuning_start_tap;
> -			}
> +			else
> +				tmp |= ESDHC_TUNING_START_TAP_DEFAULT;
>  
>  			if (imx_data->boarddata.tuning_step) {
> -				tmp &= ~ESDHC_TUNING_STEP_MASK;
>  				tmp |= imx_data->boarddata.tuning_step
>  					<< ESDHC_TUNING_STEP_SHIFT;
> +			} else {
> +				tmp |= ESDHC_TUNING_STEP_DEFAULT
> +					<< ESDHC_TUNING_STEP_SHIFT;
>  			}
>  
>  			/* Disable the CMD CRC check for tuning, if not, need to
Ulf Hansson Jan. 2, 2023, 3:06 p.m. UTC | #4 
On Wed, 7 Dec 2022 at 12:23, <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> Current code logic may be impacted by the setting of ROM/Bootloader,
> so unmask these bits first, then setting these bits accordingly.
>
> Fixes: 2b16cf326b70 ("mmc: sdhci-esdhc-imx: move tuning static configuration into hwinit function")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>

Applied for fixes and by adding a stable tag, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 89ef0c80ac37..9e73c34b6401 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -107,6 +107,7 @@
>  #define ESDHC_TUNING_START_TAP_DEFAULT 0x1
>  #define ESDHC_TUNING_START_TAP_MASK    0x7f
>  #define ESDHC_TUNING_CMD_CRC_CHECK_DISABLE     (1 << 7)
> +#define ESDHC_TUNING_STEP_DEFAULT      0x1
>  #define ESDHC_TUNING_STEP_MASK         0x00070000
>  #define ESDHC_TUNING_STEP_SHIFT                16
>
> @@ -1368,7 +1369,7 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>         struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>         struct cqhci_host *cq_host = host->mmc->cqe_private;
> -       int tmp;
> +       u32 tmp;
>
>         if (esdhc_is_usdhc(imx_data)) {
>                 /*
> @@ -1423,17 +1424,24 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>
>                 if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
>                         tmp = readl(host->ioaddr + ESDHC_TUNING_CTRL);
> -                       tmp |= ESDHC_STD_TUNING_EN |
> -                               ESDHC_TUNING_START_TAP_DEFAULT;
> -                       if (imx_data->boarddata.tuning_start_tap) {
> -                               tmp &= ~ESDHC_TUNING_START_TAP_MASK;
> +                       tmp |= ESDHC_STD_TUNING_EN;
> +
> +                       /*
> +                        * ROM code or bootloader may config the start tap
> +                        * and step, unmask them first.
> +                        */
> +                       tmp &= ~(ESDHC_TUNING_START_TAP_MASK | ESDHC_TUNING_STEP_MASK);
> +                       if (imx_data->boarddata.tuning_start_tap)
>                                 tmp |= imx_data->boarddata.tuning_start_tap;
> -                       }
> +                       else
> +                               tmp |= ESDHC_TUNING_START_TAP_DEFAULT;
>
>                         if (imx_data->boarddata.tuning_step) {
> -                               tmp &= ~ESDHC_TUNING_STEP_MASK;
>                                 tmp |= imx_data->boarddata.tuning_step
>                                         << ESDHC_TUNING_STEP_SHIFT;
> +                       } else {
> +                               tmp |= ESDHC_TUNING_STEP_DEFAULT
> +                                       << ESDHC_TUNING_STEP_SHIFT;
>                         }
>
>                         /* Disable the CMD CRC check for tuning, if not, need to
> --
> 2.34.1
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 89ef0c80ac37..9e73c34b6401 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -107,6 +107,7 @@ 
 #define ESDHC_TUNING_START_TAP_DEFAULT	0x1
 #define ESDHC_TUNING_START_TAP_MASK	0x7f
 #define ESDHC_TUNING_CMD_CRC_CHECK_DISABLE	(1 << 7)
+#define ESDHC_TUNING_STEP_DEFAULT	0x1
 #define ESDHC_TUNING_STEP_MASK		0x00070000
 #define ESDHC_TUNING_STEP_SHIFT		16
 
@@ -1368,7 +1369,7 @@  static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	struct cqhci_host *cq_host = host->mmc->cqe_private;
-	int tmp;
+	u32 tmp;
 
 	if (esdhc_is_usdhc(imx_data)) {
 		/*
@@ -1423,17 +1424,24 @@  static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
 
 		if (imx_data->socdata->flags & ESDHC_FLAG_STD_TUNING) {
 			tmp = readl(host->ioaddr + ESDHC_TUNING_CTRL);
-			tmp |= ESDHC_STD_TUNING_EN |
-				ESDHC_TUNING_START_TAP_DEFAULT;
-			if (imx_data->boarddata.tuning_start_tap) {
-				tmp &= ~ESDHC_TUNING_START_TAP_MASK;
+			tmp |= ESDHC_STD_TUNING_EN;
+
+			/*
+			 * ROM code or bootloader may config the start tap
+			 * and step, unmask them first.
+			 */
+			tmp &= ~(ESDHC_TUNING_START_TAP_MASK | ESDHC_TUNING_STEP_MASK);
+			if (imx_data->boarddata.tuning_start_tap)
 				tmp |= imx_data->boarddata.tuning_start_tap;
-			}
+			else
+				tmp |= ESDHC_TUNING_START_TAP_DEFAULT;
 
 			if (imx_data->boarddata.tuning_step) {
-				tmp &= ~ESDHC_TUNING_STEP_MASK;
 				tmp |= imx_data->boarddata.tuning_step
 					<< ESDHC_TUNING_STEP_SHIFT;
+			} else {
+				tmp |= ESDHC_TUNING_STEP_DEFAULT
+					<< ESDHC_TUNING_STEP_SHIFT;
 			}
 
 			/* Disable the CMD CRC check for tuning, if not, need to