mmc: tegra: Add and use tegra_sdhci_get_max_clock()

Implement and use tegra_sdhci_get_max_clock() which returns the true
maximum host clock rate. The issue with tegra_sdhci_get_max_clock() is
that it returns the current clock rate of the host instead of the
maximum one, which can lead to unnecessarily small clock rates.

This differs from the previous implementation of
tegra_sdhci_get_max_clock() in that it doesn't divide the result by two.

Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>
---
 drivers/mmc/host/sdhci-tegra.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

On 13/07/18 14:17, Aapo Vienamo wrote:
> Implement and use tegra_sdhci_get_max_clock() which returns the true
> maximum host clock rate. The issue with tegra_sdhci_get_max_clock() is

Don't you mean sdhci_pltfm_clk_get_max_clock above? Does this function
need fixing then? Or at least should there be another variant added
because there is nothing Tegra specific we are doing below?

> that it returns the current clock rate of the host instead of the
> maximum one, which can lead to unnecessarily small clock rates.
> 
> This differs from the previous implementation of
> tegra_sdhci_get_max_clock() in that it doesn't divide the result by two.

Why?

Fixes tag?
Cheers
Jon

On Fri, 13 Jul 2018 15:01:21 +0100
Jon Hunter <jonathanh@nvidia.com> wrote:

> On 13/07/18 14:17, Aapo Vienamo wrote:
> > Implement and use tegra_sdhci_get_max_clock() which returns the true
> > maximum host clock rate. The issue with tegra_sdhci_get_max_clock() is  
> 
> Don't you mean sdhci_pltfm_clk_get_max_clock above? Does this function
> need fixing then? Or at least should there be another variant added
> because there is nothing Tegra specific we are doing below?

There are several drivers which use sdhci_pltfm_clk_get_max_clock() in
its current state and changing it could have unforeseen side-effects on
drivers which I'm unable to test. Adding another variant of 
sdhci_pltfm_clk_get_max_clock() would probably be a more feasible
approach.

> > that it returns the current clock rate of the host instead of the
> > maximum one, which can lead to unnecessarily small clock rates.
> > 
> > This differs from the previous implementation of
> > tegra_sdhci_get_max_clock() in that it doesn't divide the result by two.  
> 
> Why?

As far as I can tell the original tegra_sdhci_get_max_clock() was
implemented this way in order to force sdhci_calc_clk() to always set
the SDHCI clock divider to two on sdhci_set_clock(). The requirement to
configure the SDHCI divider to two is specific to DDR50/52 modes on
Tegra.

The .get_max_clock() callback retuning half of the actual maximum will
result in HS200 and HS400 modes not being able to run at full speed.
Another mechanism to enforce the divider requirement has to be figured
out in order to enable DDR50/52 modes on Tegra SoCs.

 -Aapo
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 2018-07-13 at 16:17 +0300, Aapo Vienamo wrote:
> Implement and use tegra_sdhci_get_max_clock() which returns the true
> maximum host clock rate. The issue with tegra_sdhci_get_max_clock()
> is
> that it returns the current clock rate of the host instead of the
> maximum one, which can lead to unnecessarily small clock rates.
> 
> This differs from the previous implementation of
> tegra_sdhci_get_max_clock() in that it doesn't divide the result by
> two.
> 
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>

The whole series stress-tested on various Apalis T30 as well as Colibri
T30 modules:

Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---
>  drivers/mmc/host/sdhci-tegra.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-
> tegra.c
> index 28b98e2..ddf00166 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -235,6 +235,13 @@ static void tegra_sdhci_set_uhs_signaling(struct
> sdhci_host *host,
>  	sdhci_set_uhs_signaling(host, timing);
>  }
>  
> +static unsigned int tegra_sdhci_get_max_clock(struct sdhci_host
> *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +	return clk_round_rate(pltfm_host->clk, UINT_MAX);
> +}
> +
>  static void tegra_sdhci_set_tap(struct sdhci_host *host, unsigned
> int tap)
>  {
>  	u32 reg;
> @@ -299,7 +306,7 @@ static const struct sdhci_ops tegra_sdhci_ops = {
>  	.platform_execute_tuning = tegra_sdhci_execute_tuning,
>  	.set_uhs_signaling = tegra_sdhci_set_uhs_signaling,
>  	.voltage_switch = tegra_sdhci_voltage_switch,
> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +	.get_max_clock = tegra_sdhci_get_max_clock,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_tegra20_pdata = {
> @@ -356,7 +363,7 @@ static const struct sdhci_ops tegra114_sdhci_ops
> = {
>  	.platform_execute_tuning = tegra_sdhci_execute_tuning,
>  	.set_uhs_signaling = tegra_sdhci_set_uhs_signaling,
>  	.voltage_switch = tegra_sdhci_voltage_switch,
> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +	.get_max_clock = tegra_sdhci_get_max_clock,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {

Works quite nicely on today's next together with Stefan's patch set [1]:

root@apalis-t30:~# cat /sys/kernel/debug/mmc1/ios 
clock:          52000000 Hz
actual clock:   51000000 Hz
vdd:            21 (3.3 ~ 3.4 V)
bus mode:       2 (push-pull)
chip select:    0 (don't care)
power mode:     2 (on)
bus width:      3 (8 bits)
timing spec:    8 (mmc DDR52)
signal voltage: 1 (1.80 V)
driver type:    0 (driver type B)
root@apalis-t30:~# hdparm -t /dev/mmcblk1

/dev/mmcblk1:
 Timing buffered disk reads: 236 MB in  3.01 seconds =  78.42 MB/sec

root@colibri-t30:~# cat /sys/kernel/debug/mmc0/ios 
clock:          52000000 Hz
actual clock:   51000000 Hz
vdd:            21 (3.3 ~ 3.4 V)
bus mode:       2 (push-pull)
chip select:    0 (don't care)
power mode:     2 (on)
bus width:      3 (8 bits)
timing spec:    8 (mmc DDR52)
signal voltage: 1 (1.80 V)
driver type:    0 (driver type B)
root@colibri-t30:~# hdparm -t /dev/mmcblk0

/dev/mmcblk0:
 Timing buffered disk reads: 168 MB in  3.00 seconds =  55.99 MB/sec

[1] https://lore.kernel.org/lkml/20180712073904.4705-1-stefan@agner.ch

+Stefan

On 13 July 2018 at 15:17, Aapo Vienamo <avienamo@nvidia.com> wrote:
> Implement and use tegra_sdhci_get_max_clock() which returns the true
> maximum host clock rate. The issue with tegra_sdhci_get_max_clock() is
> that it returns the current clock rate of the host instead of the
> maximum one, which can lead to unnecessarily small clock rates.
>
> This differs from the previous implementation of
> tegra_sdhci_get_max_clock() in that it doesn't divide the result by two.
>
> Signed-off-by: Aapo Vienamo <avienamo@nvidia.com>

Thanks, applied for next!

I realize there are some discussions and tests going on in regards to
the changes for tegra driver. However, I decided to apply all pending
patches from Stefan and you, to get them tested in next for a while to
see how it plays.

Please tell if you want me to drop some changes - or if you prefer to
send patches on top to fix further related issues.

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-tegra.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 28b98e2..ddf00166 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -235,6 +235,13 @@ static void tegra_sdhci_set_uhs_signaling(struct sdhci_host *host,
>         sdhci_set_uhs_signaling(host, timing);
>  }
>
> +static unsigned int tegra_sdhci_get_max_clock(struct sdhci_host *host)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +       return clk_round_rate(pltfm_host->clk, UINT_MAX);
> +}
> +
>  static void tegra_sdhci_set_tap(struct sdhci_host *host, unsigned int tap)
>  {
>         u32 reg;
> @@ -299,7 +306,7 @@ static const struct sdhci_ops tegra_sdhci_ops = {
>         .platform_execute_tuning = tegra_sdhci_execute_tuning,
>         .set_uhs_signaling = tegra_sdhci_set_uhs_signaling,
>         .voltage_switch = tegra_sdhci_voltage_switch,
> -       .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +       .get_max_clock = tegra_sdhci_get_max_clock,
>  };
>
>  static const struct sdhci_pltfm_data sdhci_tegra20_pdata = {
> @@ -356,7 +363,7 @@ static const struct sdhci_ops tegra114_sdhci_ops = {
>         .platform_execute_tuning = tegra_sdhci_execute_tuning,
>         .set_uhs_signaling = tegra_sdhci_set_uhs_signaling,
>         .voltage_switch = tegra_sdhci_voltage_switch,
> -       .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +       .get_max_clock = tegra_sdhci_get_max_clock,
>  };
>
>  static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 13/07/18 16:39, Aapo Vienamo wrote:

...

>>> that it returns the current clock rate of the host instead of the
>>> maximum one, which can lead to unnecessarily small clock rates.
>>>
>>> This differs from the previous implementation of
>>> tegra_sdhci_get_max_clock() in that it doesn't divide the result by two.  
>>
>> Why?
> 
> As far as I can tell the original tegra_sdhci_get_max_clock() was
> implemented this way in order to force sdhci_calc_clk() to always set
> the SDHCI clock divider to two on sdhci_set_clock(). The requirement to
> configure the SDHCI divider to two is specific to DDR50/52 modes on
> Tegra.
> 
> The .get_max_clock() callback retuning half of the actual maximum will
> result in HS200 and HS400 modes not being able to run at full speed.
> Another mechanism to enforce the divider requirement has to be figured
> out in order to enable DDR50/52 modes on Tegra SoCs.

OK, thanks. Ah I see Stefan's patch for doubling the clock. OK so that's fine.

Cheers
Jon