* RE: [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary
2022-02-22 3:28 ` [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary haibo.chen
@ 2022-02-23 5:28 ` Peng Fan (OSS)
2022-02-24 0:03 ` Jaehoon Chung
2022-03-09 16:10 ` Tom Rini
2 siblings, 0 replies; 5+ messages in thread
From: Peng Fan (OSS) @ 2022-02-23 5:28 UTC (permalink / raw)
To: Bough Chen, jh80.chung@samsung.com, festevam@gmail.com,
sean.anderson@seco.com, u-boot@lists.denx.de, marex@denx.de,
aford173@gmail.com, tharvey@gateworks.com,
andrey.zhizhikin@leica-geosystems.com
Cc: dl-uboot-imx, Bough Chen
> Subject: [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON
> when necessary
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> After commit f132aab40327 ("Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output""), it involve issue
> in mmc_switch_voltage(), because of the special design of usdhc.
>
> For FSL_USDHC, it do not implement
> VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN,
> these are reserved bits(Though RM contain the definition of these bits, but
> actually internal IC logic do not implement, already confirm with IC team).
> Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card clock
> output. Here is the definition of this bit in RM:
>
> [8] FRC_SDCLK_ON
> Force CLK output active
> Do not set this bit to 1 unless it is necessary. Also, make sure that this bit is
> cleared when uSDHC’s clock is about to be changed (frequency change, clock
> source change, or delay chain tuning).
> 0b - CLK active or inactive is fully controlled by the hardware.
> 1b - Force CLK active
>
> In default, the FRC_SDCLK_ON is 0. This means, when there is no command or
> data transfer on bus, hardware will gate off the card clock. But in some case,
> we need the card clock keep on. Take IO voltage 1.8v switch as example, after
> IO voltage change to 1.8v, spec require gate off the card clock for 5ms, and
> gate on the clock back, once detect the card clock on, then the card will draw
> the dat0 to high immediately. If there is not clock gate off/on behavior, some
> card will keep the dat0 to low level. This is the reason we fail in
> mmc_switch_voltage().
>
> To fix this issue, and concern that this is only the fsl usdhc hardware design
> limitation, set the bit FRC_SDCLK_ON in the beginning of the
> wait_dat0() and clear it in the end. To make sure the 1.8v IO voltage switch
> process align with SD specification.
>
> For standard tuning process, usdhc specification also require the card clock
> keep on, so also add these behavior in fsl_esdhc_execute_tuning().
>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/mmc/fsl_esdhc_imx.c | 25 ++++++++++++++++++++++---
> include/fsl_esdhc_imx.h | 2 ++
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index 9299635f50..e0108144e7 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -831,13 +831,16 @@ static int fsl_esdhc_execute_tuning(struct udevice
> *dev, uint32_t opcode)
> struct mmc *mmc = &plat->mmc;
> u32 irqstaten = esdhc_read32(®s->irqstaten);
> u32 irqsigen = esdhc_read32(®s->irqsigen);
> - int i, ret = -ETIMEDOUT;
> - u32 val, mixctrl;
> + int i, err, ret = -ETIMEDOUT;
> + u32 val, mixctrl, tmp;
>
> /* clock tuning is not needed for upto 52MHz */
> if (mmc->clock <= 52000000)
> return 0;
>
> + /* make sure the card clock keep on */
> + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +
> /* This is readw/writew SDHCI_HOST_CONTROL2 when tuning */
> if (priv->flags & ESDHC_FLAG_STD_TUNING) {
> val = esdhc_read32(®s->autoc12err); @@ -897,6 +900,12 @@
> static int fsl_esdhc_execute_tuning(struct udevice *dev, uint32_t opcode)
>
> esdhc_stop_tuning(mmc);
>
> + /* change to default setting, let host control the card clock */
> + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> + err = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100);
> + if (err)
> + dev_warn(dev, "card clock not gate off as expect.\n");
> +
> return ret;
> }
> #endif
> @@ -1555,14 +1564,24 @@ static int __maybe_unused
> fsl_esdhc_set_enhanced_strobe(struct udevice *dev) static int
> fsl_esdhc_wait_dat0(struct udevice *dev, int state,
> int timeout_us)
> {
> - int ret;
> + int ret, err;
> u32 tmp;
> struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> struct fsl_esdhc *regs = priv->esdhc_regs;
>
> + /* make sure the card clock keep on */
> + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +
> ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp,
> !!(tmp & PRSSTAT_DAT0) == !!state,
> timeout_us);
> +
> + /* change to default setting, let host control the card clock */
> + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> + err = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100);
> + if (err)
> + dev_warn(dev, "card clock not gate off as expect.\n");
> +
> return ret;
> }
>
> diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index
> 2153f29bef..b8efd2a166 100644
> --- a/include/fsl_esdhc_imx.h
> +++ b/include/fsl_esdhc_imx.h
> @@ -37,6 +37,7 @@
> #define VENDORSPEC_HCKEN 0x00001000
> #define VENDORSPEC_IPGEN 0x00000800
> #define VENDORSPEC_INIT 0x20007809
> +#define VENDORSPEC_FRC_SDCLK_ON 0x00000100
>
> #define IRQSTAT 0x0002e030
> #define IRQSTAT_DMAE (0x10000000)
> @@ -94,6 +95,7 @@
> #define PRSSTAT_CINS (0x00010000)
> #define PRSSTAT_BREN (0x00000800)
> #define PRSSTAT_BWEN (0x00000400)
> +#define PRSSTAT_SDOFF (0x00000080)
> #define PRSSTAT_SDSTB (0X00000008)
> #define PRSSTAT_DLA (0x00000004)
> #define PRSSTAT_CICHB (0x00000002)
> --
> 2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary
2022-02-22 3:28 ` [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary haibo.chen
2022-02-23 5:28 ` Peng Fan (OSS)
@ 2022-02-24 0:03 ` Jaehoon Chung
2022-03-09 16:10 ` Tom Rini
2 siblings, 0 replies; 5+ messages in thread
From: Jaehoon Chung @ 2022-02-24 0:03 UTC (permalink / raw)
To: haibo.chen, peng.fan, festevam, sean.anderson, u-boot, marex,
aford173, tharvey, andrey.zhizhikin
Cc: uboot-imx
On 2/22/22 12:28, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
>
> After commit f132aab40327 ("Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output""), it
> involve issue in mmc_switch_voltage(), because of the special
> design of usdhc.
>
> For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN,
> these are reserved bits(Though RM contain the definition of these bits,
> but actually internal IC logic do not implement, already confirm with
> IC team). Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card
> clock output. Here is the definition of this bit in RM:
>
> [8] FRC_SDCLK_ON
> Force CLK output active
> Do not set this bit to 1 unless it is necessary. Also, make sure that
> this bit is cleared when uSDHC’s clock is about to be changed (frequency
> change, clock source change, or delay chain tuning).
> 0b - CLK active or inactive is fully controlled by the hardware.
> 1b - Force CLK active
>
> In default, the FRC_SDCLK_ON is 0. This means, when there is no command
> or data transfer on bus, hardware will gate off the card clock. But in
> some case, we need the card clock keep on. Take IO voltage 1.8v switch
> as example, after IO voltage change to 1.8v, spec require gate off the
> card clock for 5ms, and gate on the clock back, once detect the card
> clock on, then the card will draw the dat0 to high immediately. If there
> is not clock gate off/on behavior, some card will keep the dat0 to low
> level. This is the reason we fail in mmc_switch_voltage().
>
> To fix this issue, and concern that this is only the fsl usdhc hardware
> design limitation, set the bit FRC_SDCLK_ON in the beginning of the
> wait_dat0() and clear it in the end. To make sure the 1.8v IO voltage
> switch process align with SD specification.
>
> For standard tuning process, usdhc specification also require the card
> clock keep on, so also add these behavior in fsl_esdhc_execute_tuning().
>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Best Regards,
Jaehoon Chung
> ---
> drivers/mmc/fsl_esdhc_imx.c | 25 ++++++++++++++++++++++---
> include/fsl_esdhc_imx.h | 2 ++
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index 9299635f50..e0108144e7 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -831,13 +831,16 @@ static int fsl_esdhc_execute_tuning(struct udevice *dev, uint32_t opcode)
> struct mmc *mmc = &plat->mmc;
> u32 irqstaten = esdhc_read32(®s->irqstaten);
> u32 irqsigen = esdhc_read32(®s->irqsigen);
> - int i, ret = -ETIMEDOUT;
> - u32 val, mixctrl;
> + int i, err, ret = -ETIMEDOUT;
> + u32 val, mixctrl, tmp;
>
> /* clock tuning is not needed for upto 52MHz */
> if (mmc->clock <= 52000000)
> return 0;
>
> + /* make sure the card clock keep on */
> + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +
> /* This is readw/writew SDHCI_HOST_CONTROL2 when tuning */
> if (priv->flags & ESDHC_FLAG_STD_TUNING) {
> val = esdhc_read32(®s->autoc12err);
> @@ -897,6 +900,12 @@ static int fsl_esdhc_execute_tuning(struct udevice *dev, uint32_t opcode)
>
> esdhc_stop_tuning(mmc);
>
> + /* change to default setting, let host control the card clock */
> + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> + err = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100);
> + if (err)
> + dev_warn(dev, "card clock not gate off as expect.\n");
> +
> return ret;
> }
> #endif
> @@ -1555,14 +1564,24 @@ static int __maybe_unused fsl_esdhc_set_enhanced_strobe(struct udevice *dev)
> static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
> int timeout_us)
> {
> - int ret;
> + int ret, err;
> u32 tmp;
> struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> struct fsl_esdhc *regs = priv->esdhc_regs;
>
> + /* make sure the card clock keep on */
> + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +
> ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp,
> !!(tmp & PRSSTAT_DAT0) == !!state,
> timeout_us);
> +
> + /* change to default setting, let host control the card clock */
> + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> + err = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100);
> + if (err)
> + dev_warn(dev, "card clock not gate off as expect.\n");
> +
> return ret;
> }
>
> diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h
> index 2153f29bef..b8efd2a166 100644
> --- a/include/fsl_esdhc_imx.h
> +++ b/include/fsl_esdhc_imx.h
> @@ -37,6 +37,7 @@
> #define VENDORSPEC_HCKEN 0x00001000
> #define VENDORSPEC_IPGEN 0x00000800
> #define VENDORSPEC_INIT 0x20007809
> +#define VENDORSPEC_FRC_SDCLK_ON 0x00000100
>
> #define IRQSTAT 0x0002e030
> #define IRQSTAT_DMAE (0x10000000)
> @@ -94,6 +95,7 @@
> #define PRSSTAT_CINS (0x00010000)
> #define PRSSTAT_BREN (0x00000800)
> #define PRSSTAT_BWEN (0x00000400)
> +#define PRSSTAT_SDOFF (0x00000080)
> #define PRSSTAT_SDSTB (0X00000008)
> #define PRSSTAT_DLA (0x00000004)
> #define PRSSTAT_CICHB (0x00000002)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary
2022-02-22 3:28 ` [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary haibo.chen
2022-02-23 5:28 ` Peng Fan (OSS)
2022-02-24 0:03 ` Jaehoon Chung
@ 2022-03-09 16:10 ` Tom Rini
2022-03-14 9:31 ` Jaehoon Chung
2 siblings, 1 reply; 5+ messages in thread
From: Tom Rini @ 2022-03-09 16:10 UTC (permalink / raw)
To: haibo.chen, Jaehoon Chung, Peng Fan, Stefano Babic
Cc: peng.fan, jh80.chung, festevam, sean.anderson, u-boot, marex,
aford173, tharvey, andrey.zhizhikin, uboot-imx
[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]
On Tue, Feb 22, 2022 at 11:28:18AM +0800, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
>
> After commit f132aab40327 ("Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output""), it
> involve issue in mmc_switch_voltage(), because of the special
> design of usdhc.
>
> For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN,
> these are reserved bits(Though RM contain the definition of these bits,
> but actually internal IC logic do not implement, already confirm with
> IC team). Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card
> clock output. Here is the definition of this bit in RM:
>
> [8] FRC_SDCLK_ON
> Force CLK output active
> Do not set this bit to 1 unless it is necessary. Also, make sure that
> this bit is cleared when uSDHC’s clock is about to be changed (frequency
> change, clock source change, or delay chain tuning).
> 0b - CLK active or inactive is fully controlled by the hardware.
> 1b - Force CLK active
>
> In default, the FRC_SDCLK_ON is 0. This means, when there is no command
> or data transfer on bus, hardware will gate off the card clock. But in
> some case, we need the card clock keep on. Take IO voltage 1.8v switch
> as example, after IO voltage change to 1.8v, spec require gate off the
> card clock for 5ms, and gate on the clock back, once detect the card
> clock on, then the card will draw the dat0 to high immediately. If there
> is not clock gate off/on behavior, some card will keep the dat0 to low
> level. This is the reason we fail in mmc_switch_voltage().
>
> To fix this issue, and concern that this is only the fsl usdhc hardware
> design limitation, set the bit FRC_SDCLK_ON in the beginning of the
> wait_dat0() and clear it in the end. To make sure the 1.8v IO voltage
> switch process align with SD specification.
>
> For standard tuning process, usdhc specification also require the card
> clock keep on, so also add these behavior in fsl_esdhc_execute_tuning().
>
> Reviewed-by: Marek Vasut <marex@denx.de>
> Tested-by: Fabio Estevam <festevam@gmail.com>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> ---
> drivers/mmc/fsl_esdhc_imx.c | 25 ++++++++++++++++++++++---
> include/fsl_esdhc_imx.h | 2 ++
> 2 files changed, 24 insertions(+), 3 deletions(-)
Is this going to get picked up in the imx or mmc trees soon, or should I
take this directly? Thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary
2022-03-09 16:10 ` Tom Rini
@ 2022-03-14 9:31 ` Jaehoon Chung
0 siblings, 0 replies; 5+ messages in thread
From: Jaehoon Chung @ 2022-03-14 9:31 UTC (permalink / raw)
To: Tom Rini, haibo.chen, Peng Fan, Stefano Babic
Cc: festevam, sean.anderson, u-boot, marex, aford173, tharvey,
andrey.zhizhikin, uboot-imx
On 3/10/22 01:10, Tom Rini wrote:
> On Tue, Feb 22, 2022 at 11:28:18AM +0800, haibo.chen@nxp.com wrote:
>
>> From: Haibo Chen <haibo.chen@nxp.com>
>>
>> After commit f132aab40327 ("Revert "mmc: fsl_esdhc_imx: use
>> VENDORSPEC_FRC_SDCLK_ON to control card clock output""), it
>> involve issue in mmc_switch_voltage(), because of the special
>> design of usdhc.
>>
>> For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN,
>> these are reserved bits(Though RM contain the definition of these bits,
>> but actually internal IC logic do not implement, already confirm with
>> IC team). Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card
>> clock output. Here is the definition of this bit in RM:
>>
>> [8] FRC_SDCLK_ON
>> Force CLK output active
>> Do not set this bit to 1 unless it is necessary. Also, make sure that
>> this bit is cleared when uSDHC’s clock is about to be changed (frequency
>> change, clock source change, or delay chain tuning).
>> 0b - CLK active or inactive is fully controlled by the hardware.
>> 1b - Force CLK active
>>
>> In default, the FRC_SDCLK_ON is 0. This means, when there is no command
>> or data transfer on bus, hardware will gate off the card clock. But in
>> some case, we need the card clock keep on. Take IO voltage 1.8v switch
>> as example, after IO voltage change to 1.8v, spec require gate off the
>> card clock for 5ms, and gate on the clock back, once detect the card
>> clock on, then the card will draw the dat0 to high immediately. If there
>> is not clock gate off/on behavior, some card will keep the dat0 to low
>> level. This is the reason we fail in mmc_switch_voltage().
>>
>> To fix this issue, and concern that this is only the fsl usdhc hardware
>> design limitation, set the bit FRC_SDCLK_ON in the beginning of the
>> wait_dat0() and clear it in the end. To make sure the 1.8v IO voltage
>> switch process align with SD specification.
>>
>> For standard tuning process, usdhc specification also require the card
>> clock keep on, so also add these behavior in fsl_esdhc_execute_tuning().
>>
>> Reviewed-by: Marek Vasut <marex@denx.de>
>> Tested-by: Fabio Estevam <festevam@gmail.com>
>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>> ---
>> drivers/mmc/fsl_esdhc_imx.c | 25 ++++++++++++++++++++++---
>> include/fsl_esdhc_imx.h | 2 ++
>> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> Is this going to get picked up in the imx or mmc trees soon, or should I
> take this directly? Thanks!
Applied to u-boot-mmc. Thanks! Sorry for late.
Best Regards,
Jaehoon Chung
>
^ permalink raw reply [flat|nested] 5+ messages in thread