| Message ID | 20250324092336.2231385-1-ziniu.wang_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mmc: sdhci: convert sdhci_calc_timeout() to non-static | expand |
On 24/03/25 11:23, ziniu.wang_1@nxp.com wrote: > From: Luke Wang <ziniu.wang_1@nxp.com> > > Calclute data timeout value based on clock instead of using max value. And the subject: Calclute -> Calculate Presumably the driver has been working OK up until now with max value. Is there any particular reason to change it? > > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index ff78a7c6a04c..e7316ecff64e 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -31,6 +31,7 @@ > #include "cqhci.h" > > #define ESDHC_SYS_CTRL_DTOCV_MASK GENMASK(19, 16) > +#define ESDHC_SYS_CTRL_DTOCV_SHIFT 16 > #define ESDHC_SYS_CTRL_IPP_RST_N BIT(23) > #define ESDHC_CTRL_D3CD 0x08 > #define ESDHC_BURST_LEN_EN_INCR (1 << 27) > @@ -1387,12 +1388,16 @@ static unsigned int esdhc_get_max_timeout_count(struct sdhci_host *host) > > static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) > { > - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > - struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > + bool too_big = false; > + u8 count = sdhci_calc_timeout(host, cmd, &too_big); > > - /* use maximum timeout counter */ > + /* > + * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset > + * pin of the card. Write 0 to bit[23] will reset the card. > + * Only write DTOCV filed here. filed -> field ? > + */ > esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, > - esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000, > + count << ESDHC_SYS_CTRL_DTOCV_SHIFT, Could use FIELD_PREP() here > ESDHC_SYSTEM_CONTROL); Another way to do this could be to add SDHCI_TIMEOUT_CONTROL to esdhc_writeb_le() and remove esdhc_set_timeout(). That would avoid having to export sdhci_calc_timeout() and is perhaps slightly more consistent with other code in this driver. Probably look something like below: diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index ff78a7c6a04c..66477fc0ba82 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -870,6 +870,16 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg) esdhc_clrset_le(host, mask, new_val, reg); return; + case SDHCI_TIMEOUT_CONTROL: + /* + * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset + * pin of the card. Write 0 to bit[23] will reset the card. + * Only write DTOCV field here. + */ + esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, + FIELD_PREP(ESDHC_SYS_CTRL_DTOCV_MASK, val), + ESDHC_SYSTEM_CONTROL); + return; case SDHCI_SOFTWARE_RESET: if (val & SDHCI_RESET_DATA) new_val = readl(host->ioaddr + SDHCI_HOST_CONTROL); @@ -1385,17 +1395,6 @@ static unsigned int esdhc_get_max_timeout_count(struct sdhci_host *host) return esdhc_is_usdhc(imx_data) ? 1 << 29 : 1 << 27; } -static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) -{ - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); - - /* use maximum timeout counter */ - esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, - esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000, - ESDHC_SYSTEM_CONTROL); -} - static u32 esdhc_cqhci_irq(struct sdhci_host *host, u32 intmask) { int cmd_error = 0; @@ -1432,7 +1431,6 @@ static struct sdhci_ops sdhci_esdhc_ops = { .get_min_clock = esdhc_pltfm_get_min_clock, .get_max_timeout_count = esdhc_get_max_timeout_count, .get_ro = esdhc_pltfm_get_ro, - .set_timeout = esdhc_set_timeout, .set_bus_width = esdhc_pltfm_set_bus_width, .set_uhs_signaling = esdhc_set_uhs_signaling, .reset = esdhc_reset, > } > > @@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) > * to distinguish the card type. > */ > host->mmc_host_ops.init_card = usdhc_init_card; > + > + host->max_timeout_count = 0xF; > } > > if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING)
> -----Original Message----- > From: Adrian Hunter <adrian.hunter@intel.com> > Sent: Wednesday, April 9, 2025 1:59 AM > To: Luke Wang <ziniu.wang_1@nxp.com>; ulf.hansson@linaro.org; Bough > Chen <haibo.chen@nxp.com> > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; > kernel@pengutronix.de; festevam@gmail.com; imx@lists.linux.dev; linux- > mmc@vger.kernel.org; dl-S32 <S32@nxp.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout > value based on clock > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > On 24/03/25 11:23, ziniu.wang_1@nxp.com wrote: > > From: Luke Wang <ziniu.wang_1@nxp.com> > > > > Calclute data timeout value based on clock instead of using max value. > > And the subject: > > Calclute -> Calculate > > Presumably the driver has been working OK up until now with max value. > Is there any particular reason to change it? Hi Adrian, Yes, the max value works fine. We want the value to align with the spec recommendation. > > > > > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com> > > --- > > drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci- > esdhc-imx.c > > index ff78a7c6a04c..e7316ecff64e 100644 > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > @@ -31,6 +31,7 @@ > > #include "cqhci.h" > > > > #define ESDHC_SYS_CTRL_DTOCV_MASK GENMASK(19, 16) > > +#define ESDHC_SYS_CTRL_DTOCV_SHIFT 16 > > #define ESDHC_SYS_CTRL_IPP_RST_N BIT(23) > > #define ESDHC_CTRL_D3CD 0x08 > > #define ESDHC_BURST_LEN_EN_INCR (1 << 27) > > @@ -1387,12 +1388,16 @@ static unsigned int > esdhc_get_max_timeout_count(struct sdhci_host *host) > > > > static void esdhc_set_timeout(struct sdhci_host *host, struct > mmc_command *cmd) > > { > > - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > - struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > > + bool too_big = false; > > + u8 count = sdhci_calc_timeout(host, cmd, &too_big); > > > > - /* use maximum timeout counter */ > > + /* > > + * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset > > + * pin of the card. Write 0 to bit[23] will reset the card. > > + * Only write DTOCV filed here. > > filed -> field ? Yes, I will use --codespell to check next time. > > > + */ > > esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, > > - esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000, > > + count << ESDHC_SYS_CTRL_DTOCV_SHIFT, > > Could use FIELD_PREP() here OK > > > ESDHC_SYSTEM_CONTROL); > > Another way to do this could be to add SDHCI_TIMEOUT_CONTROL to > esdhc_writeb_le() and remove esdhc_set_timeout(). That would > avoid having to export sdhci_calc_timeout() and is perhaps > slightly more consistent with other code in this driver. > Probably look something like below: > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci- > esdhc-imx.c > index ff78a7c6a04c..66477fc0ba82 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -870,6 +870,16 @@ static void esdhc_writeb_le(struct sdhci_host *host, > u8 val, int reg) > > esdhc_clrset_le(host, mask, new_val, reg); > return; > + case SDHCI_TIMEOUT_CONTROL: > + /* > + * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset > + * pin of the card. Write 0 to bit[23] will reset the card. > + * Only write DTOCV field here. > + */ > + esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, > + FIELD_PREP(ESDHC_SYS_CTRL_DTOCV_MASK, val), > + ESDHC_SYSTEM_CONTROL); > + return; > case SDHCI_SOFTWARE_RESET: > if (val & SDHCI_RESET_DATA) > new_val = readl(host->ioaddr + SDHCI_HOST_CONTROL); > @@ -1385,17 +1395,6 @@ static unsigned int > esdhc_get_max_timeout_count(struct sdhci_host *host) > return esdhc_is_usdhc(imx_data) ? 1 << 29 : 1 << 27; > } > > -static void esdhc_set_timeout(struct sdhci_host *host, struct > mmc_command *cmd) > -{ > - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > - struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); > - > - /* use maximum timeout counter */ > - esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, > - esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000, > - ESDHC_SYSTEM_CONTROL); > -} > - > static u32 esdhc_cqhci_irq(struct sdhci_host *host, u32 intmask) > { > int cmd_error = 0; > @@ -1432,7 +1431,6 @@ static struct sdhci_ops sdhci_esdhc_ops = { > .get_min_clock = esdhc_pltfm_get_min_clock, > .get_max_timeout_count = esdhc_get_max_timeout_count, > .get_ro = esdhc_pltfm_get_ro, > - .set_timeout = esdhc_set_timeout, > .set_bus_width = esdhc_pltfm_set_bus_width, > .set_uhs_signaling = esdhc_set_uhs_signaling, > .reset = esdhc_reset, > > > > } > > > > @@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct > platform_device *pdev) > > * to distinguish the card type. > > */ > > host->mmc_host_ops.init_card = usdhc_init_card; > > + > > + host->max_timeout_count = 0xF; > > } > > > > if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING) It's indeed a better way. I will send v2 patch. Thanks Luke
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index ff78a7c6a04c..e7316ecff64e 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -31,6 +31,7 @@ #include "cqhci.h" #define ESDHC_SYS_CTRL_DTOCV_MASK GENMASK(19, 16) +#define ESDHC_SYS_CTRL_DTOCV_SHIFT 16 #define ESDHC_SYS_CTRL_IPP_RST_N BIT(23) #define ESDHC_CTRL_D3CD 0x08 #define ESDHC_BURST_LEN_EN_INCR (1 << 27) @@ -1387,12 +1388,16 @@ static unsigned int esdhc_get_max_timeout_count(struct sdhci_host *host) static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) { - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); - struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host); + bool too_big = false; + u8 count = sdhci_calc_timeout(host, cmd, &too_big); - /* use maximum timeout counter */ + /* + * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset + * pin of the card. Write 0 to bit[23] will reset the card. + * Only write DTOCV filed here. + */ esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK, - esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000, + count << ESDHC_SYS_CTRL_DTOCV_SHIFT, ESDHC_SYSTEM_CONTROL); } @@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev) * to distinguish the card type. */ host->mmc_host_ops.init_card = usdhc_init_card; + + host->max_timeout_count = 0xF; } if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING)