On Fri, 13 Jul 2018 01:43:05 +0000
Marcel Ziswiler [off-list ref] wrote:

On Mon, 2018-07-02 at 15:16 +0200, Ulf Hansson wrote:

quoted

On 4 June 2018 at 17:35, Aapo Vienamo [off-list ref] wrote:  

quoted

The sdhci get_max_clock callback is set to
sdhci_pltfm_clk_get_max_clock
and tegra_sdhci_get_max_clock is removed. It appears that the
shdci-tegra specific callback was originally introduced due to the
requirement that the host clock has to be twice the bus clock on
DDR50
mode. As far as I can tell the only effect the removal has on DDR50
mode
is in cases where the parent clock is unable to supply the
requested
clock rate, causing the DDR50 mode to run at a lower frequency.
Currently the DDR50 mode isn't enabled on any of the SoCs and would
also
require configuring the SDHCI clock divider register to function
properly.

The problem with tegra_sdhci_get_max_clock is that it divides the
clock
rate by two and thus artificially limits the maximum frequency of
faster
signaling modes which don't have the host-bus frequency ratio
requirement
of DDR50 such as SDR104 and HS200. Furthermore, the call to
clk_round_rate() may return an error which isn't handled by
tegra_sdhci_get_max_clock.

Signed-off-by: Aapo Vienamo <redacted>  

Thanks, applied for next!

Kind regards
Uffe
  

quoted

---
 drivers/mmc/host/sdhci-tegra.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c

b/drivers/mmc/host/sdhci-tegra.c
index 970d38f6..c8745b5 100644

--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c

@@ -234,17 +234,6 @@ 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);
-
-       /*
-        * DDR modes require the host to run at double the card
frequency, so
-        * the maximum rate we can support is half of the module
input clock.
-        */
-       return clk_round_rate(pltfm_host->clk, UINT_MAX) / 2;
-}
-
 static void tegra_sdhci_set_tap(struct sdhci_host *host, unsigned
int tap)
 {
        u32 reg;

@@ -309,7 +298,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 = tegra_sdhci_get_max_clock,
+       .get_max_clock = sdhci_pltfm_clk_get_max_clock,
 };

 static const struct sdhci_pltfm_data sdhci_tegra20_pdata = {

@@ -357,7 +346,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 = tegra_sdhci_get_max_clock,
+       .get_max_clock = sdhci_pltfm_clk_get_max_clock,
 };

 static const struct sdhci_pltfm_data sdhci_tegra114_pdata = {
--
2.7.4  

Hm, for us this definitely breaks stuff. While using Stefan's patch set
[1] we may not only run eMMC at DDR52 even SD cards run stable at
SDR104. With this patch however the clock gets crippled to 45.33 resp.
48 MHz always. This is observed both on Apalis/Colibri T30 as well as
Apalis TK1.

Current next-20180712 just with Stefan's 3 patches:

root@apalis-t30:~# cat /sys/kernel/debug/mmc1/ios 
clock:          48000000 Hz
actual clock:   45333333 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: 218 MB in  3.03 seconds =  71.95 MB/sec

root@apalis-t30:~# cat /sys/kernel/debug/mmc2/ios 
clock:          48000000 Hz
actual clock:   48000000 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:      2 (4 bits)
timing spec:    6 (sd uhs SDR104)
signal voltage: 1 (1.80 V)
driver type:    0 (driver type B)
root@apalis-t30:~# hdparm -t /dev/mmcblk2

/dev/mmcblk2:
 Timing buffered disk reads:  66 MB in  3.06 seconds =  21.60 MB/sec

And with this patch reverted it works as expected again:

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: 240 MB in  3.02 seconds =  79.50 MB/sec

root@apalis-t30:~# cat /sys/kernel/debug/mmc2/ios 
clock:          204000000 Hz
actual clock:   204000000 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:      2 (4 bits)
timing spec:    6 (sd uhs SDR104)
signal voltage: 1 (1.80 V)
driver type:    0 (driver type B)
root@apalis-t30:~# hdparm -t /dev/mmcblk2

/dev/mmcblk2:
 Timing buffered disk reads: 258 MB in  3.01 seconds =  85.58 MB/sec

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

This happens because sdhci_pltfm_clk_get_max_clock() does not actually
return the maximum clock rate but the current one, leading to smaller
clock rates on some platforms. I'll send a patch that fixes this for
sdhci-tegra. Although this raises the question whether the current
behaviour of sdhci_pltfm_clk_get_max_clock() is desirable or not.

 -Aapo