[PATCH] mmc: rtsx: add 74 Clocks in power on flow

[PATCH] mmc: rtsx: add 74 Clocks in power on flow

[Ricky WU]

After 1ms stabilizing the voltage time
add "Host provides at least 74 Clocks
before issuing first command" that is
spec definition

Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
---
 drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
index 2a3f14afe9f8..e016d720e453 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -940,10 +940,17 @@ static int sd_power_on(struct realtek_pci_sdmmc *host)
 	if (err < 0)
 		return err;
 
+	mdelay(1);
+
 	err = rtsx_pci_write_register(pcr, CARD_OE, SD_OUTPUT_EN, SD_OUTPUT_EN);
 	if (err < 0)
 		return err;
 
+	/* send init 74 clocks */
+	rtsx_pci_write_register(pcr, SD_BUS_STAT, SD_CLK_TOGGLE_EN, SD_CLK_TOGGLE_EN);
+	mdelay(5);
+	rtsx_pci_write_register(pcr, SD_BUS_STAT, SD_CLK_TOGGLE_EN, 0);
+
 	if (PCI_PID(pcr) == PID_5261) {
 		/*
 		 * If test mode is set switch to SD Express mandatorily,
-- 
2.25.1

Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[gregkh]

On Tue, Feb 22, 2022 at 07:27:52AM +0000, Ricky WU wrote:
> After 1ms stabilizing the voltage time
> add "Host provides at least 74 Clocks
> before issuing first command" that is
> spec definition

You do have 72 columns to use here, no need to wrap this so tightly.

> 
> Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> ---
>  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 2a3f14afe9f8..e016d720e453 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -940,10 +940,17 @@ static int sd_power_on(struct realtek_pci_sdmmc *host)
>  	if (err < 0)
>  		return err;
>  
> +	mdelay(1);

What is this delay for?

thanks,

greg k-h

RE: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[Ricky WU]

> -----Original Message-----
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Tuesday, February 22, 2022 3:42 PM
> To: Ricky WU <ricky_wu@realtek.com>
> Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> 
> On Tue, Feb 22, 2022 at 07:27:52AM +0000, Ricky WU wrote:
> > After 1ms stabilizing the voltage time add "Host provides at least 74
> > Clocks before issuing first command" that is spec definition
> 
> You do have 72 columns to use here, no need to wrap this so tightly.
>

Ok...
so I need to have next patch to fix this format?

> >
> > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > ---
> >  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > index 2a3f14afe9f8..e016d720e453 100644
> > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > @@ -940,10 +940,17 @@ static int sd_power_on(struct realtek_pci_sdmmc
> *host)
> >  	if (err < 0)
> >  		return err;
> >
> > +	mdelay(1);
> 
> What is this delay for?
>

Spec definition, need to wait 1ms for voltage stable
and below mdelay(5) is our device send 74 Clocks time
 
> thanks,
> 
> greg k-h
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[gregkh]

On Tue, Feb 22, 2022 at 08:48:30AM +0000, Ricky WU wrote:
> > -----Original Message-----
> > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > Sent: Tuesday, February 22, 2022 3:42 PM
> > To: Ricky WU <ricky_wu@realtek.com>
> > Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> > tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> > 
> > On Tue, Feb 22, 2022 at 07:27:52AM +0000, Ricky WU wrote:
> > > After 1ms stabilizing the voltage time add "Host provides at least 74
> > > Clocks before issuing first command" that is spec definition
> > 
> > You do have 72 columns to use here, no need to wrap this so tightly.
> >
> 
> Ok...
> so I need to have next patch to fix this format?

Please do, because:

> 
> > >
> > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > > ---
> > >  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > index 2a3f14afe9f8..e016d720e453 100644
> > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > @@ -940,10 +940,17 @@ static int sd_power_on(struct realtek_pci_sdmmc
> > *host)
> > >  	if (err < 0)
> > >  		return err;
> > >
> > > +	mdelay(1);
> > 
> > What is this delay for?
> >
> 
> Spec definition, need to wait 1ms for voltage stable
> and below mdelay(5) is our device send 74 Clocks time

Clock cycles and mdelay() are not going to always stay the same, right?

I really have no idea what "74 clocks time" means, is this some specific
timing value for this hardware?  What is the units?  This needs to be
documented better in both the changelog and in the code.

thanks,

greg k-h

RE: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[Ricky WU]

> -----Original Message-----
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Tuesday, February 22, 2022 4:59 PM
> To: Ricky WU <ricky_wu@realtek.com>
> Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> 
> On Tue, Feb 22, 2022 at 08:48:30AM +0000, Ricky WU wrote:
> > > -----Original Message-----
> > > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > > Sent: Tuesday, February 22, 2022 3:42 PM
> > > To: Ricky WU <ricky_wu@realtek.com>
> > > Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> > > tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> > >
> > > On Tue, Feb 22, 2022 at 07:27:52AM +0000, Ricky WU wrote:
> > > > After 1ms stabilizing the voltage time add "Host provides at least
> > > > 74 Clocks before issuing first command" that is spec definition
> > >
> > > You do have 72 columns to use here, no need to wrap this so tightly.
> > >
> >
> > Ok...
> > so I need to have next patch to fix this format?
> 
> Please do, because:
> 
> >
> > > >
> > > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > > > ---
> > > >  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > index 2a3f14afe9f8..e016d720e453 100644
> > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > @@ -940,10 +940,17 @@ static int sd_power_on(struct
> > > > realtek_pci_sdmmc
> > > *host)
> > > >  	if (err < 0)
> > > >  		return err;
> > > >
> > > > +	mdelay(1);
> > >
> > > What is this delay for?
> > >
> >
> > Spec definition, need to wait 1ms for voltage stable and below
> > mdelay(5) is our device send 74 Clocks time
> 
> Clock cycles and mdelay() are not going to always stay the same, right?
> 
> I really have no idea what "74 clocks time" means, is this some specific timing
> value for this hardware?  What is the units?  This needs to be documented
> better in both the changelog and in the code.
> 
> thanks,
> 

Please ref: https://www.sdcard.org/downloads/pls/ Version8
And see the 6.4.1.2 Power Up Time of Host Figure 6-5: Power Up Diagram of Host
mdelay(1) corresponds to Stable supply voltage
mdelay(5) corresponds to Host provides at least 74 Clocks before issuing first command
our device need 5ms to issue 74 Clocks


> greg k-h
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[gregkh]

On Tue, Feb 22, 2022 at 12:31:10PM +0000, Ricky WU wrote:
> > -----Original Message-----
> > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > Sent: Tuesday, February 22, 2022 4:59 PM
> > To: Ricky WU <ricky_wu@realtek.com>
> > Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> > tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> > 
> > On Tue, Feb 22, 2022 at 08:48:30AM +0000, Ricky WU wrote:
> > > > -----Original Message-----
> > > > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > > > Sent: Tuesday, February 22, 2022 3:42 PM
> > > > To: Ricky WU <ricky_wu@realtek.com>
> > > > Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> > > > tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> > > >
> > > > On Tue, Feb 22, 2022 at 07:27:52AM +0000, Ricky WU wrote:
> > > > > After 1ms stabilizing the voltage time add "Host provides at least
> > > > > 74 Clocks before issuing first command" that is spec definition
> > > >
> > > > You do have 72 columns to use here, no need to wrap this so tightly.
> > > >
> > >
> > > Ok...
> > > so I need to have next patch to fix this format?
> > 
> > Please do, because:
> > 
> > >
> > > > >
> > > > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > > > > ---
> > > > >  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > index 2a3f14afe9f8..e016d720e453 100644
> > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > @@ -940,10 +940,17 @@ static int sd_power_on(struct
> > > > > realtek_pci_sdmmc
> > > > *host)
> > > > >  	if (err < 0)
> > > > >  		return err;
> > > > >
> > > > > +	mdelay(1);
> > > >
> > > > What is this delay for?
> > > >
> > >
> > > Spec definition, need to wait 1ms for voltage stable and below
> > > mdelay(5) is our device send 74 Clocks time
> > 
> > Clock cycles and mdelay() are not going to always stay the same, right?
> > 
> > I really have no idea what "74 clocks time" means, is this some specific timing
> > value for this hardware?  What is the units?  This needs to be documented
> > better in both the changelog and in the code.
> > 
> > thanks,
> > 
> 
> Please ref: https://www.sdcard.org/downloads/pls/ Version8

I can not download those specifications according to the license that
they ask me to abide by.

> And see the 6.4.1.2 Power Up Time of Host Figure 6-5: Power Up Diagram of Host
> mdelay(1) corresponds to Stable supply voltage
> mdelay(5) corresponds to Host provides at least 74 Clocks before issuing first command
> our device need 5ms to issue 74 Clocks

What is a "clock"?  The kernel works with time units, how does "5" equal
to 74?

thanks,

greg k-h

RE: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[Ricky WU]

> -----Original Message-----
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Tuesday, February 22, 2022 10:18 PM
> To: Ricky WU <ricky_wu@realtek.com>
> Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> 
> On Tue, Feb 22, 2022 at 12:31:10PM +0000, Ricky WU wrote:
> > > -----Original Message-----
> > > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > > Sent: Tuesday, February 22, 2022 4:59 PM
> > > To: Ricky WU <ricky_wu@realtek.com>
> > > Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> > > tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> > >
> > > On Tue, Feb 22, 2022 at 08:48:30AM +0000, Ricky WU wrote:
> > > > > -----Original Message-----
> > > > > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > > > > Sent: Tuesday, February 22, 2022 3:42 PM
> > > > > To: Ricky WU <ricky_wu@realtek.com>
> > > > > Cc: ulf.hansson@linaro.org; kai.heng.feng@canonical.com;
> > > > > tommyhebb@gmail.com; linux-mmc@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow
> > > > >
> > > > > On Tue, Feb 22, 2022 at 07:27:52AM +0000, Ricky WU wrote:
> > > > > > After 1ms stabilizing the voltage time add "Host provides at
> > > > > > least
> > > > > > 74 Clocks before issuing first command" that is spec
> > > > > > definition
> > > > >
> > > > > You do have 72 columns to use here, no need to wrap this so tightly.
> > > > >
> > > >
> > > > Ok...
> > > > so I need to have next patch to fix this format?
> > >
> > > Please do, because:
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > > > > > ---
> > > > > >  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > index 2a3f14afe9f8..e016d720e453 100644
> > > > > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> > > > > > @@ -940,10 +940,17 @@ static int sd_power_on(struct
> > > > > > realtek_pci_sdmmc
> > > > > *host)
> > > > > >  	if (err < 0)
> > > > > >  		return err;
> > > > > >
> > > > > > +	mdelay(1);
> > > > >
> > > > > What is this delay for?
> > > > >
> > > >
> > > > Spec definition, need to wait 1ms for voltage stable and below
> > > > mdelay(5) is our device send 74 Clocks time
> > >
> > > Clock cycles and mdelay() are not going to always stay the same, right?
> > >
> > > I really have no idea what "74 clocks time" means, is this some
> > > specific timing value for this hardware?  What is the units?  This
> > > needs to be documented better in both the changelog and in the code.
> > >
> > > thanks,
> > >
> >
> > Please ref: https://www.sdcard.org/downloads/pls/ Version8
> 
> I can not download those specifications according to the license that they ask
> me to abide by.
> 
> > And see the 6.4.1.2 Power Up Time of Host Figure 6-5: Power Up Diagram
> > of Host
> > mdelay(1) corresponds to Stable supply voltage
> > mdelay(5) corresponds to Host provides at least 74 Clocks before
> > issuing first command our device need 5ms to issue 74 Clocks
> 
> What is a "clock"?  The kernel works with time units, how does "5" equal to
> 74?
> 

spec original is
"Host provides at least 74 Clocks before issuing first command"
Let me clarify it, 
74 Clocks means 74 clock signals, our Card Reader need to issue more than 74 Clock signals to Card before first command
So mdelay(5) , our device will keep issue signal to card in this 5ms make sure over 74 

> thanks,
> 
> greg k-h
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] mmc: rtsx: add 74 Clocks in power on flow

[Ulf Hansson]

On Tue, 22 Feb 2022 at 08:28, Ricky WU <ricky_wu@realtek.com> wrote:
>
> After 1ms stabilizing the voltage time
> add "Host provides at least 74 Clocks
> before issuing first command" that is
> spec definition
>
> Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> ---
>  drivers/mmc/host/rtsx_pci_sdmmc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 2a3f14afe9f8..e016d720e453 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -940,10 +940,17 @@ static int sd_power_on(struct realtek_pci_sdmmc *host)
>         if (err < 0)
>                 return err;
>
> +       mdelay(1);
> +
>         err = rtsx_pci_write_register(pcr, CARD_OE, SD_OUTPUT_EN, SD_OUTPUT_EN);
>         if (err < 0)
>                 return err;
>
> +       /* send init 74 clocks */
> +       rtsx_pci_write_register(pcr, SD_BUS_STAT, SD_CLK_TOGGLE_EN, SD_CLK_TOGGLE_EN);
> +       mdelay(5);
> +       rtsx_pci_write_register(pcr, SD_BUS_STAT, SD_CLK_TOGGLE_EN, 0);
> +
>         if (PCI_PID(pcr) == PID_5261) {
>                 /*
>                  * If test mode is set switch to SD Express mandatorily,

As you probably are aware of, the mmc core uses three power states
(MMC_POWER_ON, MMC_POWER_UP and MMC_POWER_OFF) to manage the
initialization, while it invokes the ->set_ios() callback for the mmc
host driver. During these steps the core also tries to manage the
different delays that are needed according to the eMMC/SD specs. You
may have a look at mmc_power_up() in drivers/mmc/core/core.c. In the
rtsx case, MMC_POWER_ON and MMC_POWER_UP are treated as one single
step.

Moreover, it has turned out that some mmc HWs are actually controlling
these delays during the initialization themselves, which makes the
delays in the core superfluous. Therefore we have made the delays
configurable for host drivers. For DT based platforms, we have the DT
property "post-power-on-delay-ms" and for others, it's perfectly fine
to update host->power_delay_ms before calling mmc_add_host().

Would it be possible to take advantage of the above "features" from
the core, to avoid hard coded and superfluous delays?

Kind regards
Uffe