mmc: host: sdhci-esdhc-imx: implement emmc hardware reset

Commit Message

Josua Mayer Oct. 27, 2024, 2:41 p.m. UTC

NXP ESDHC supports control of native emmc reset signal when pinmux is
set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
Documentation is available in NXP i.MX6Q Reference Manual.

Implement the hw_reset function in sdhci_ops asserting reset for at
least 10us and waiting an extra 300us after deassertion.
These particular delays were inspired by sunxi-mmc hw_reset function.

Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that eMMC is
still accessible after boot. eMMC extcsd has RST_N_FUNCTION=0x01, i.e.
reset input enabled, Linux v5.15.

Signed-off-by: Josua Mayer <[e-mail]>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)


---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241027-imx-emmc-reset-7127d311174c

Best regards,

Comments

Peng Fan Oct. 28, 2024, 2:09 a.m. UTC | #1

> Subject: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc
> hardware reset
> 
> NXP ESDHC supports control of native emmc reset signal when pinmux
> is set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
> Documentation is available in NXP i.MX6Q Reference Manual.

But this relies on the PAD been configured as RESET, should this
flow being default enabled whether the PAD is configured as RESET or
not?

> 
> Implement the hw_reset function in sdhci_ops asserting reset for at
> least 10us and waiting an extra 300us after deassertion.
> These particular delays were inspired by sunxi-mmc hw_reset function.
> 
> Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that
> eMMC is still accessible after boot. eMMC extcsd has
> RST_N_FUNCTION=0x01, i.e.
> reset input enabled, Linux v5.15.
> 
> Signed-off-by: Josua Mayer <[e-mail]>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> b/drivers/mmc/host/sdhci-esdhc-imx.c
> index
> 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791
> a1701a3515ef6515779aa4 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -33,6 +33,8 @@
>  #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
>  #define	ESDHC_CTRL_D3CD			0x08
>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
> +#define ESDHC_SYS_CTRL			0x2c
> +#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
>  /* VENDOR SPEC register */
>  #define ESDHC_VENDOR_SPEC		0xc0
>  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
> @@ -1402,6 +1404,15 @@ static u32 esdhc_cqhci_irq(struct
> sdhci_host *host, u32 intmask)
>  	return 0;
>  }
> 
> +static void esdhc_hw_reset(struct sdhci_host *host) {
> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0,
> ESDHC_SYS_CTRL);
> +	udelay(10);
> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
> +			ESDHC_SYS_CTRL_IPP_RST_N,
> ESDHC_SYS_CTRL);
> +	udelay(300);

Please add a comment on why 10us or 300us? This is board
related or soc related or card related?

Thanks,
Peng.

> +}
> +
>  static struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl_le,
>  	.read_w = esdhc_readw_le,
> @@ -1420,6 +1431,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.reset = esdhc_reset,
>  	.irq = esdhc_cqhci_irq,
>  	.dump_vendor_regs = esdhc_dump_debug_regs,
> +	.hw_reset = esdhc_hw_reset,
>  };
> 
>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> 
> ---
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> change-id: 20241027-imx-emmc-reset-7127d311174c
> 
> Best regards,
> --
> Josua Mayer <[e-mail]>
>

Bough Chen Oct. 28, 2024, 3:15 a.m. UTC | #2

> -----Original Message-----
> From: Peng Fan <[e-mail]>
> Sent: 2024年10月28日 10:10
> To: Josua Mayer <[e-mail]>; Adrian Hunter
> <[e-mail]>; Bough Chen <[e-mail]>; Ulf Hansson
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha Hauer
> <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>
> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
> <[e-mail]>; [e-mail]; [e-mail];
> dl-S32 <[e-mail]>; [e-mail];
> [e-mail]
> Subject: RE: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
> reset
> 
> > Subject: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
> > reset
> >
> > NXP ESDHC supports control of native emmc reset signal when pinmux is
> > set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
> > Documentation is available in NXP i.MX6Q Reference Manual.
> 
> But this relies on the PAD been configured as RESET, should this flow being
> default enabled whether the PAD is configured as RESET or not?

No, from my understanding, even the PAD is configured as RESET, still need SW to config IPP_RST_N to control the output of this pad.
Josua, you can double confirm this on your board. By the way, I check the code, when you do the test to support this reset operation on eMMC, did you add  "cap-mmc-hw-reset" in dts?
From current code logic, the callback you add here seems only can be called by eMMC device, so will be safe for sd and sdio device. And if your answer for my question is "yes", then your change will also be safe for eMMC device which do not use this reset function before.


> 
> >
> > Implement the hw_reset function in sdhci_ops asserting reset for at
> > least 10us and waiting an extra 300us after deassertion.
> > These particular delays were inspired by sunxi-mmc hw_reset function.
> >
> > Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that eMMC
> > is still accessible after boot. eMMC extcsd has RST_N_FUNCTION=0x01,
> > i.e.
> > reset input enabled, Linux v5.15.
> >
> > Signed-off-by: Josua Mayer <[e-mail]>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index
> > 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791
> > a1701a3515ef6515779aa4 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -33,6 +33,8 @@
> >  #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
> >  #define	ESDHC_CTRL_D3CD			0x08
> >  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
> > +#define ESDHC_SYS_CTRL			0x2c
> > +#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
> >  /* VENDOR SPEC register */
> >  #define ESDHC_VENDOR_SPEC		0xc0
> >  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
> > @@ -1402,6 +1404,15 @@ static u32 esdhc_cqhci_irq(struct sdhci_host
> > *host, u32 intmask)
> >  	return 0;
> >  }
> >
> > +static void esdhc_hw_reset(struct sdhci_host *host) {
> > +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0,
> > ESDHC_SYS_CTRL);
> > +	udelay(10);
> > +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
> > +			ESDHC_SYS_CTRL_IPP_RST_N,
> > ESDHC_SYS_CTRL);
> > +	udelay(300);
> 
> Please add a comment on why 10us or 300us? This is board related or soc
> related or card related?

Agree, please add comment and explain.

Regards
Haibo Chen
> 
> Thanks,
> Peng.
> 
> > +}
> > +
> >  static struct sdhci_ops sdhci_esdhc_ops = {
> >  	.read_l = esdhc_readl_le,
> >  	.read_w = esdhc_readw_le,
> > @@ -1420,6 +1431,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
> >  	.reset = esdhc_reset,
> >  	.irq = esdhc_cqhci_irq,
> >  	.dump_vendor_regs = esdhc_dump_debug_regs,
> > +	.hw_reset = esdhc_hw_reset,
> >  };
> >
> >  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> >
> > ---
> > base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> > change-id: 20241027-imx-emmc-reset-7127d311174c
> >
> > Best regards,
> > --
> > Josua Mayer <[e-mail]>
> >

Josua Mayer Oct. 28, 2024, 10:45 a.m. UTC | #3

Am 28.10.24 um 04:15 schrieb Bough Chen:
>> -----Original Message-----
>> From: Peng Fan <[e-mail]>
>> Sent: 2024年10月28日 10:10
>> To: Josua Mayer <[e-mail]>; Adrian Hunter
>> <[e-mail]>; Bough Chen <[e-mail]>; Ulf Hansson
>> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha Hauer
>> <[e-mail]>; Pengutronix Kernel Team
>> <[e-mail]>; Fabio Estevam <[e-mail]>
>> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
>> <[e-mail]>; [e-mail]; [e-mail];
>> dl-S32 <[e-mail]>; [e-mail];
>> [e-mail]
>> Subject: RE: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
>> reset
>>
>>> Subject: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
>>> reset
>>>
>>> NXP ESDHC supports control of native emmc reset signal when pinmux is
>>> set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
>>> Documentation is available in NXP i.MX6Q Reference Manual.
>> But this relies on the PAD been configured as RESET, should this flow being
>> default enabled whether the PAD is configured as RESET or not?
> No, from my understanding, even the PAD is configured as RESET, still need SW to config IPP_RST_N to control the output of this pad.
> Josua, you can double confirm this on your board.
Correct, only when a pad is configured for emmc reset function
does the reset signal affect any board circuit.

We tested this when we had wrong pinmux and reset did not toggle.

> By the way, I check the code, when you do the test to support this reset operation on eMMC, did you add  "cap-mmc-hw-reset" in dts?

Correct:

> From current code logic, the callback you add here seems only can be called by eMMC device, so will be safe for sd and sdio device. And if your answer for my question is "yes", then your change will also be safe for eMMC device which do not use this reset function before.
I believe so. Only when dts declares the capability will sdhci_hw_reset

get called, and then it will call esdhc-imx driver hw_reset.

See also drivers/mmc/core.c: _mmc_hw_reset

>
>
>>> Implement the hw_reset function in sdhci_ops asserting reset for at
>>> least 10us and waiting an extra 300us after deassertion.
>>> These particular delays were inspired by sunxi-mmc hw_reset function.
>>>
>>> Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that eMMC
>>> is still accessible after boot. eMMC extcsd has RST_N_FUNCTION=0x01,
>>> i.e.
>>> reset input enabled, Linux v5.15.
>>>
>>> Signed-off-by: Josua Mayer <[e-mail]>
>>> ---
>>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> index
>>> 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791
>>> a1701a3515ef6515779aa4 100644
>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> @@ -33,6 +33,8 @@
>>>  #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
>>>  #define	ESDHC_CTRL_D3CD			0x08
>>>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
>>> +#define ESDHC_SYS_CTRL			0x2c
>>> +#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
>>>  /* VENDOR SPEC register */
>>>  #define ESDHC_VENDOR_SPEC		0xc0
>>>  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
>>> @@ -1402,6 +1404,15 @@ static u32 esdhc_cqhci_irq(struct sdhci_host
>>> *host, u32 intmask)
>>>  	return 0;
>>>  }
>>>
>>> +static void esdhc_hw_reset(struct sdhci_host *host) {
>>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0,
>>> ESDHC_SYS_CTRL);
>>> +	udelay(10);
>>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
>>> +			ESDHC_SYS_CTRL_IPP_RST_N,
>>> ESDHC_SYS_CTRL);
>>> +	udelay(300);
>> Please add a comment on why 10us or 300us? This is board related or soc
>> related or card related?
> Agree, please add comment and explain.

They were copied from drivers/mmc/host/sunxi-mmc.c.

I was hoping somebody knows or recognises these magic numbers.
They are intended to be generic across all eMMC (not SoC).


sincerely
Josua Mayer

>
> Regards
> Haibo Chen
>> Thanks,
>> Peng.
>>
>>> +}
>>> +
>>>  static struct sdhci_ops sdhci_esdhc_ops = {
>>>  	.read_l = esdhc_readl_le,
>>>  	.read_w = esdhc_readw_le,
>>> @@ -1420,6 +1431,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>>>  	.reset = esdhc_reset,
>>>  	.irq = esdhc_cqhci_irq,
>>>  	.dump_vendor_regs = esdhc_dump_debug_regs,
>>> +	.hw_reset = esdhc_hw_reset,
>>>  };
>>>
>>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>>>
>>> ---
>>> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
>>> change-id: 20241027-imx-emmc-reset-7127d311174c
>>>
>>> Best regards,
>>> --
>>> Josua Mayer <[e-mail]>
>>>

Bough Chen Oct. 29, 2024, 2:33 a.m. UTC | #4

> -----Original Message-----
> From: Josua Mayer <[e-mail]>
> Sent: 2024年10月28日 18:46
> To: Bough Chen <[e-mail]>; Peng Fan <[e-mail]>;
> Adrian Hunter <[e-mail]>; Ulf Hansson
> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha Hauer
> <[e-mail]>; Pengutronix Kernel Team
> <[e-mail]>; Fabio Estevam <[e-mail]>
> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
> <[e-mail]>; [e-mail]; [e-mail];
> dl-S32 <[e-mail]>; [e-mail];
> [e-mail]
> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
> reset
> 
> Am 28.10.24 um 04:15 schrieb Bough Chen:
> >> -----Original Message-----
> >> From: Peng Fan <[e-mail]>
> >> Sent: 2024年10月28日 10:10
> >> To: Josua Mayer <[e-mail]>; Adrian Hunter
> >> <[e-mail]>; Bough Chen <[e-mail]>; Ulf
> >> Hansson <[e-mail]>; Shawn Guo <[e-mail]>;
> >> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
> >> <[e-mail]>; Fabio Estevam <[e-mail]>
> >> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
> >> <[e-mail]>; [e-mail];
> >> [e-mail];
> >> dl-S32 <[e-mail]>; [e-mail];
> >> [e-mail]
> >> Subject: RE: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc
> >> hardware reset
> >>
> >>> Subject: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
> >>> reset
> >>>
> >>> NXP ESDHC supports control of native emmc reset signal when pinmux
> >>> is set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
> >>> Documentation is available in NXP i.MX6Q Reference Manual.
> >> But this relies on the PAD been configured as RESET, should this flow
> >> being default enabled whether the PAD is configured as RESET or not?
> > No, from my understanding, even the PAD is configured as RESET, still need SW
> to config IPP_RST_N to control the output of this pad.
> > Josua, you can double confirm this on your board.
> Correct, only when a pad is configured for emmc reset function does the reset
> signal affect any board circuit.
> 
> We tested this when we had wrong pinmux and reset did not toggle.
> 
> > By the way, I check the code, when you do the test to support this reset
> operation on eMMC, did you add  "cap-mmc-hw-reset" in dts?
> 
> Correct:
> 
> > From current code logic, the callback you add here seems only can be called by
> eMMC device, so will be safe for sd and sdio device. And if your answer for my
> question is "yes", then your change will also be safe for eMMC device which do
> not use this reset function before.
> I believe so. Only when dts declares the capability will sdhci_hw_reset
> 
> get called, and then it will call esdhc-imx driver hw_reset.
> 
> See also drivers/mmc/core.c: _mmc_hw_reset
> 
> >
> >
> >>> Implement the hw_reset function in sdhci_ops asserting reset for at
> >>> least 10us and waiting an extra 300us after deassertion.
> >>> These particular delays were inspired by sunxi-mmc hw_reset function.
> >>>
> >>> Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that
> >>> eMMC is still accessible after boot. eMMC extcsd has
> >>> RST_N_FUNCTION=0x01, i.e.
> >>> reset input enabled, Linux v5.15.
> >>>
> >>> Signed-off-by: Josua Mayer <[e-mail]>
> >>> ---
> >>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> b/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> index
> >>> 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791
> >>> a1701a3515ef6515779aa4 100644
> >>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> @@ -33,6 +33,8 @@
> >>>  #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
> >>>  #define	ESDHC_CTRL_D3CD			0x08
> >>>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
> >>> +#define ESDHC_SYS_CTRL			0x2c
> >>> +#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
> >>>  /* VENDOR SPEC register */
> >>>  #define ESDHC_VENDOR_SPEC		0xc0
> >>>  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
> >>> @@ -1402,6 +1404,15 @@ static u32 esdhc_cqhci_irq(struct sdhci_host
> >>> *host, u32 intmask)
> >>>  	return 0;
> >>>  }
> >>>
> >>> +static void esdhc_hw_reset(struct sdhci_host *host) {
> >>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0,
> >>> ESDHC_SYS_CTRL);
> >>> +	udelay(10);
> >>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
> >>> +			ESDHC_SYS_CTRL_IPP_RST_N,
> >>> ESDHC_SYS_CTRL);
> >>> +	udelay(300);
> >> Please add a comment on why 10us or 300us? This is board related or
> >> soc related or card related?
> > Agree, please add comment and explain.
> 
> They were copied from drivers/mmc/host/sunxi-mmc.c.
> 
> I was hoping somebody knows or recognises these magic numbers.
> They are intended to be generic across all eMMC (not SoC).

I check the eMMC spec, refer to JEDEC Standard No. 84-B51,  6.15.10 H/W Reset Operation, on page 159
The min time for reset signal keep low is 1us, and then the min time for reset keep high is 200us. So your config should be okay, but please add commend for each delay.

Here is the example:
/* the eMMC spec require mix 1us, here delay 10us */
udelay(10);
...
/* the eMMC spec require mix 200us, here delay 300us */
udelay(300)

Best Regards
Haibo Chen

> 
> 
> sincerely
> Josua Mayer
> 
> >
> > Regards
> > Haibo Chen
> >> Thanks,
> >> Peng.
> >>
> >>> +}
> >>> +
> >>>  static struct sdhci_ops sdhci_esdhc_ops = {
> >>>  	.read_l = esdhc_readl_le,
> >>>  	.read_w = esdhc_readw_le,
> >>> @@ -1420,6 +1431,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
> >>>  	.reset = esdhc_reset,
> >>>  	.irq = esdhc_cqhci_irq,
> >>>  	.dump_vendor_regs = esdhc_dump_debug_regs,
> >>> +	.hw_reset = esdhc_hw_reset,
> >>>  };
> >>>
> >>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
> >>>
> >>> ---
> >>> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> >>> change-id: 20241027-imx-emmc-reset-7127d311174c
> >>>
> >>> Best regards,
> >>> --
> >>> Josua Mayer <[e-mail]>
> >>>

Adrian Hunter Oct. 29, 2024, 7:44 a.m. UTC | #5

On 29/10/24 04:33, Bough Chen wrote:
>> -----Original Message-----
>> From: Josua Mayer <[e-mail]>
>> Sent: 2024年10月28日 18:46
>> To: Bough Chen <[e-mail]>; Peng Fan <[e-mail]>;
>> Adrian Hunter <[e-mail]>; Ulf Hansson
>> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha Hauer
>> <[e-mail]>; Pengutronix Kernel Team
>> <[e-mail]>; Fabio Estevam <[e-mail]>
>> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
>> <[e-mail]>; [e-mail]; [e-mail];
>> dl-S32 <[e-mail]>; [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
>> reset
>>
>> Am 28.10.24 um 04:15 schrieb Bough Chen:
>>>> -----Original Message-----
>>>> From: Peng Fan <[e-mail]>
>>>> Sent: 2024年10月28日 10:10
>>>> To: Josua Mayer <[e-mail]>; Adrian Hunter
>>>> <[e-mail]>; Bough Chen <[e-mail]>; Ulf
>>>> Hansson <[e-mail]>; Shawn Guo <[e-mail]>;
>>>> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
>>>> <[e-mail]>; Fabio Estevam <[e-mail]>
>>>> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
>>>> <[e-mail]>; [e-mail];
>>>> [e-mail];
>>>> dl-S32 <[e-mail]>; [e-mail];
>>>> [e-mail]
>>>> Subject: RE: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc
>>>> hardware reset
>>>>
>>>>> Subject: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
>>>>> reset
>>>>>
>>>>> NXP ESDHC supports control of native emmc reset signal when pinmux
>>>>> is set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
>>>>> Documentation is available in NXP i.MX6Q Reference Manual.
>>>> But this relies on the PAD been configured as RESET, should this flow
>>>> being default enabled whether the PAD is configured as RESET or not?
>>> No, from my understanding, even the PAD is configured as RESET, still need SW
>> to config IPP_RST_N to control the output of this pad.
>>> Josua, you can double confirm this on your board.
>> Correct, only when a pad is configured for emmc reset function does the reset
>> signal affect any board circuit.
>>
>> We tested this when we had wrong pinmux and reset did not toggle.
>>
>>> By the way, I check the code, when you do the test to support this reset
>> operation on eMMC, did you add  "cap-mmc-hw-reset" in dts?
>>
>> Correct:
>>
>>> From current code logic, the callback you add here seems only can be called by
>> eMMC device, so will be safe for sd and sdio device. And if your answer for my
>> question is "yes", then your change will also be safe for eMMC device which do
>> not use this reset function before.
>> I believe so. Only when dts declares the capability will sdhci_hw_reset
>>
>> get called, and then it will call esdhc-imx driver hw_reset.
>>
>> See also drivers/mmc/core.c: _mmc_hw_reset
>>
>>>
>>>
>>>>> Implement the hw_reset function in sdhci_ops asserting reset for at
>>>>> least 10us and waiting an extra 300us after deassertion.
>>>>> These particular delays were inspired by sunxi-mmc hw_reset function.
>>>>>
>>>>> Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that
>>>>> eMMC is still accessible after boot. eMMC extcsd has
>>>>> RST_N_FUNCTION=0x01, i.e.
>>>>> reset input enabled, Linux v5.15.
>>>>>
>>>>> Signed-off-by: Josua Mayer <[e-mail]>
>>>>> ---
>>>>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>> b/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>> index
>>>>> 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791
>>>>> a1701a3515ef6515779aa4 100644
>>>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>> @@ -33,6 +33,8 @@
>>>>>  #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
>>>>>  #define	ESDHC_CTRL_D3CD			0x08
>>>>>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
>>>>> +#define ESDHC_SYS_CTRL			0x2c
>>>>> +#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
>>>>>  /* VENDOR SPEC register */
>>>>>  #define ESDHC_VENDOR_SPEC		0xc0
>>>>>  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
>>>>> @@ -1402,6 +1404,15 @@ static u32 esdhc_cqhci_irq(struct sdhci_host
>>>>> *host, u32 intmask)
>>>>>  	return 0;
>>>>>  }
>>>>>
>>>>> +static void esdhc_hw_reset(struct sdhci_host *host) {
>>>>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0,
>>>>> ESDHC_SYS_CTRL);
>>>>> +	udelay(10);
>>>>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
>>>>> +			ESDHC_SYS_CTRL_IPP_RST_N,
>>>>> ESDHC_SYS_CTRL);
>>>>> +	udelay(300);
>>>> Please add a comment on why 10us or 300us? This is board related or
>>>> soc related or card related?
>>> Agree, please add comment and explain.
>>
>> They were copied from drivers/mmc/host/sunxi-mmc.c.
>>
>> I was hoping somebody knows or recognises these magic numbers.
>> They are intended to be generic across all eMMC (not SoC).
> 
> I check the eMMC spec, refer to JEDEC Standard No. 84-B51,  6.15.10 H/W Reset Operation, on page 159
> The min time for reset signal keep low is 1us, and then the min time for reset keep high is 200us. So your config should be okay, but please add commend for each delay.
> 
> Here is the example:
> /* the eMMC spec require mix 1us, here delay 10us */
> udelay(10);
> ...
> /* the eMMC spec require mix 200us, here delay 300us */
> udelay(300)

At least that one should use usleep_range() rather than udelay().

> 
> Best Regards
> Haibo Chen
> 
>>
>>
>> sincerely
>> Josua Mayer
>>
>>>
>>> Regards
>>> Haibo Chen
>>>> Thanks,
>>>> Peng.
>>>>
>>>>> +}
>>>>> +
>>>>>  static struct sdhci_ops sdhci_esdhc_ops = {
>>>>>  	.read_l = esdhc_readl_le,
>>>>>  	.read_w = esdhc_readw_le,
>>>>> @@ -1420,6 +1431,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>>>>>  	.reset = esdhc_reset,
>>>>>  	.irq = esdhc_cqhci_irq,
>>>>>  	.dump_vendor_regs = esdhc_dump_debug_regs,
>>>>> +	.hw_reset = esdhc_hw_reset,
>>>>>  };
>>>>>
>>>>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>>>>>
>>>>> ---
>>>>> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
>>>>> change-id: 20241027-imx-emmc-reset-7127d311174c
>>>>>
>>>>> Best regards,
>>>>> --
>>>>> Josua Mayer <[e-mail]>
>>>>>

Josua Mayer Oct. 30, 2024, 12:30 p.m. UTC | #6

Am 29.10.24 um 08:44 schrieb Adrian Hunter:
> On 29/10/24 04:33, Bough Chen wrote:
>>> -----Original Message-----
>>> From: Josua Mayer <[e-mail]>
>>> Sent: 2024年10月28日 18:46
>>> To: Bough Chen <[e-mail]>; Peng Fan <[e-mail]>;
>>> Adrian Hunter <[e-mail]>; Ulf Hansson
>>> <[e-mail]>; Shawn Guo <[e-mail]>; Sascha Hauer
>>> <[e-mail]>; Pengutronix Kernel Team
>>> <[e-mail]>; Fabio Estevam <[e-mail]>
>>> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
>>> <[e-mail]>; [e-mail]; [e-mail];
>>> dl-S32 <[e-mail]>; [e-mail];
>>> [e-mail]
>>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
>>> reset
>>>
>>> Am 28.10.24 um 04:15 schrieb Bough Chen:
>>>>> -----Original Message-----
>>>>> From: Peng Fan <[e-mail]>
>>>>> Sent: 2024年10月28日 10:10
>>>>> To: Josua Mayer <[e-mail]>; Adrian Hunter
>>>>> <[e-mail]>; Bough Chen <[e-mail]>; Ulf
>>>>> Hansson <[e-mail]>; Shawn Guo <[e-mail]>;
>>>>> Sascha Hauer <[e-mail]>; Pengutronix Kernel Team
>>>>> <[e-mail]>; Fabio Estevam <[e-mail]>
>>>>> Cc: yazan.shhady <[e-mail]>; Rabeeh Khoury
>>>>> <[e-mail]>; [e-mail];
>>>>> [e-mail];
>>>>> dl-S32 <[e-mail]>; [e-mail];
>>>>> [e-mail]
>>>>> Subject: RE: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc
>>>>> hardware reset
>>>>>
>>>>>> Subject: [PATCH] mmc: host: sdhci-esdhc-imx: implement emmc hardware
>>>>>> reset
>>>>>>
>>>>>> NXP ESDHC supports control of native emmc reset signal when pinmux
>>>>>> is set accordingly, using uSDHCx_SYS_CTRL register IPP_RST_N bit.
>>>>>> Documentation is available in NXP i.MX6Q Reference Manual.
>>>>> But this relies on the PAD been configured as RESET, should this flow
>>>>> being default enabled whether the PAD is configured as RESET or not?
>>>> No, from my understanding, even the PAD is configured as RESET, still need SW
>>> to config IPP_RST_N to control the output of this pad.
>>>> Josua, you can double confirm this on your board.
>>> Correct, only when a pad is configured for emmc reset function does the reset
>>> signal affect any board circuit.
>>>
>>> We tested this when we had wrong pinmux and reset did not toggle.
>>>
>>>> By the way, I check the code, when you do the test to support this reset
>>> operation on eMMC, did you add  "cap-mmc-hw-reset" in dts?
>>>
>>> Correct:
>>>
>>>> From current code logic, the callback you add here seems only can be called by
>>> eMMC device, so will be safe for sd and sdio device. And if your answer for my
>>> question is "yes", then your change will also be safe for eMMC device which do
>>> not use this reset function before.
>>> I believe so. Only when dts declares the capability will sdhci_hw_reset
>>>
>>> get called, and then it will call esdhc-imx driver hw_reset.
>>>
>>> See also drivers/mmc/core.c: _mmc_hw_reset
>>>
>>>>
>>>>>> Implement the hw_reset function in sdhci_ops asserting reset for at
>>>>>> least 10us and waiting an extra 300us after deassertion.
>>>>>> These particular delays were inspired by sunxi-mmc hw_reset function.
>>>>>>
>>>>>> Tested on SolidRun i.MX8DXL SoM with a scope, and confirmed that
>>>>>> eMMC is still accessible after boot. eMMC extcsd has
>>>>>> RST_N_FUNCTION=0x01, i.e.
>>>>>> reset input enabled, Linux v5.15.
>>>>>>
>>>>>> Signed-off-by: Josua Mayer <[e-mail]>
>>>>>> ---
>>>>>>  drivers/mmc/host/sdhci-esdhc-imx.c | 12 ++++++++++++
>>>>>>  1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>>> b/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>>> index
>>>>>> 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791
>>>>>> a1701a3515ef6515779aa4 100644
>>>>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>>>>> @@ -33,6 +33,8 @@
>>>>>>  #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
>>>>>>  #define	ESDHC_CTRL_D3CD			0x08
>>>>>>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
>>>>>> +#define ESDHC_SYS_CTRL			0x2c
>>>>>> +#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
>>>>>>  /* VENDOR SPEC register */
>>>>>>  #define ESDHC_VENDOR_SPEC		0xc0
>>>>>>  #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
>>>>>> @@ -1402,6 +1404,15 @@ static u32 esdhc_cqhci_irq(struct sdhci_host
>>>>>> *host, u32 intmask)
>>>>>>  	return 0;
>>>>>>  }
>>>>>>
>>>>>> +static void esdhc_hw_reset(struct sdhci_host *host) {
>>>>>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0,
>>>>>> ESDHC_SYS_CTRL);
>>>>>> +	udelay(10);
>>>>>> +	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
>>>>>> +			ESDHC_SYS_CTRL_IPP_RST_N,
>>>>>> ESDHC_SYS_CTRL);
>>>>>> +	udelay(300);
>>>>> Please add a comment on why 10us or 300us? This is board related or
>>>>> soc related or card related?
>>>> Agree, please add comment and explain.
>>> They were copied from drivers/mmc/host/sunxi-mmc.c.
>>>
>>> I was hoping somebody knows or recognises these magic numbers.
>>> They are intended to be generic across all eMMC (not SoC).
>> I check the eMMC spec, refer to JEDEC Standard No. 84-B51,  6.15.10 H/W Reset Operation, on page 159
>> The min time for reset signal keep low is 1us, and then the min time for reset keep high is 200us. So your config should be okay, but please add commend for each delay.
>>
>> Here is the example:
>> /* the eMMC spec require mix 1us, here delay 10us */
>> udelay(10);
>> ...
>> /* the eMMC spec require mix 200us, here delay 300us */
>> udelay(300)
> At least that one should use usleep_range() rather than udelay().
Thank you for the verbose example! I will submit version two soon-ish.

Based on these recommendations I want to go with
usleep_range(1, 10);
usleep_range(200, 300);

>
>> Best Regards
>> Haibo Chen
>>
>>>
>>> sincerely
>>> Josua Mayer
>>>
>>>> Regards
>>>> Haibo Chen
>>>>> Thanks,
>>>>> Peng.
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>  static struct sdhci_ops sdhci_esdhc_ops = {
>>>>>>  	.read_l = esdhc_readl_le,
>>>>>>  	.read_w = esdhc_readw_le,
>>>>>> @@ -1420,6 +1431,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>>>>>>  	.reset = esdhc_reset,
>>>>>>  	.irq = esdhc_cqhci_irq,
>>>>>>  	.dump_vendor_regs = esdhc_dump_debug_regs,
>>>>>> +	.hw_reset = esdhc_hw_reset,
>>>>>>  };
>>>>>>
>>>>>>  static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
>>>>>>
>>>>>> ---
>>>>>> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
>>>>>> change-id: 20241027-imx-emmc-reset-7127d311174c
>>>>>>
>>>>>> Best regards,
>>>>>> --
>>>>>> Josua Mayer <[e-mail]>
>>>>>>

Patch

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 8f0bc6dca2b0402fd2a0695903cf261a5b4e19dc..ebcfa427cca6cc2791a1701a3515ef6515779aa4 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -33,6 +33,8 @@ 
 #define ESDHC_SYS_CTRL_DTOCV_MASK	0x0f
 #define	ESDHC_CTRL_D3CD			0x08
 #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
+#define ESDHC_SYS_CTRL			0x2c
+#define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
 /* VENDOR SPEC register */
 #define ESDHC_VENDOR_SPEC		0xc0
 #define  ESDHC_VENDOR_SPEC_SDIO_QUIRK	(1 << 1)
@@ -1402,6 +1404,15 @@  static u32 esdhc_cqhci_irq(struct sdhci_host *host, u32 intmask)
 	return 0;
 }
 
+static void esdhc_hw_reset(struct sdhci_host *host)
+{
+	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N, 0, ESDHC_SYS_CTRL);
+	udelay(10);
+	esdhc_clrset_le(host, ESDHC_SYS_CTRL_IPP_RST_N,
+			ESDHC_SYS_CTRL_IPP_RST_N, ESDHC_SYS_CTRL);
+	udelay(300);
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl_le,
 	.read_w = esdhc_readw_le,
@@ -1420,6 +1431,7 @@  static struct sdhci_ops sdhci_esdhc_ops = {
 	.reset = esdhc_reset,
 	.irq = esdhc_cqhci_irq,
 	.dump_vendor_regs = esdhc_dump_debug_regs,
+	.hw_reset = esdhc_hw_reset,
 };
 
 static const struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {