[PATCH v1] riscv: dts: thead: Fix TH1520 emmc and shdci clock rate

[PATCH v1] riscv: dts: thead: Fix TH1520 emmc and shdci clock rate

[bigunclemax]

From: Maksim Kiselev <bigunclemax@gmail.com>

In accordance with LicheePi 4A BSP the clock that comes to emmc/sdhci
is 198Mhz.

But changing from fixed-clock to CLK_EMMC_SDIO leads to increasing input
clock from 198Mhz to 792Mhz. Because the CLK_EMMC_SDIO is actually 792Mhz.

Therefore calculation of output SDCLK is incorrect now.
The mmc driver sets the divisor to 4 times larger than it should be
and emmc/sd works 4 times slower.

This can be confirmed with fio test:
Sequential read of emmc with fixed 198Mz clock:
READ: bw=289MiB/s (303MB/s)

Sequential read with CLK_EMMC_SDIO clock:
READ: bw=82.6MiB/s (86.6MB/s)

Let's fix this issue by providing fixed-factor-clock that divides
CLK_EMMC_SDIO by 4 for emmc/sd nodes.

Fixes: 03a20182e1e0 ("riscv: dts: thead: change TH1520 mmc nodes to use clock controller")
Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index acfe030e803a..6c20965cd10c 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -229,6 +229,14 @@ stmmac_axi_config: stmmac-axi-config {
 		snps,blen = <0 0 64 32 0 0 0>;
 	};
 
+	sdhci_clk: sdhci-clock {
+		compatible = "fixed-factor-clock";
+		clocks = <&clk CLK_EMMC_SDIO>;
+		#clock-cells = <0>;
+		clock-div = <4>;
+		clock-mult = <1>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;
@@ -328,7 +336,7 @@ emmc: mmc@ffe7080000 {
 			compatible = "thead,th1520-dwcmshc";
 			reg = <0xff 0xe7080000 0x0 0x10000>;
 			interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&clk CLK_EMMC_SDIO>;
+			clocks = <&sdhci_clk>;
 			clock-names = "core";
 			status = "disabled";
 		};
@@ -337,7 +345,7 @@ sdio0: mmc@ffe7090000 {
 			compatible = "thead,th1520-dwcmshc";
 			reg = <0xff 0xe7090000 0x0 0x10000>;
 			interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&clk CLK_EMMC_SDIO>;
+			clocks = <&sdhci_clk>;
 			clock-names = "core";
 			status = "disabled";
 		};
@@ -346,7 +354,7 @@ sdio1: mmc@ffe70a0000 {
 			compatible = "thead,th1520-dwcmshc";
 			reg = <0xff 0xe70a0000 0x0 0x10000>;
 			interrupts = <71 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&clk CLK_EMMC_SDIO>;
+			clocks = <&sdhci_clk>;
 			clock-names = "core";
 			status = "disabled";
 		};
-- 
2.45.2

Re: [PATCH v1] riscv: dts: thead: Fix TH1520 emmc and shdci clock rate

[Emil Renner Berthing]

bigunclemax@ wrote:
> From: Maksim Kiselev <bigunclemax@gmail.com>
>
> In accordance with LicheePi 4A BSP the clock that comes to emmc/sdhci
> is 198Mhz.
>
> But changing from fixed-clock to CLK_EMMC_SDIO leads to increasing input
> clock from 198Mhz to 792Mhz. Because the CLK_EMMC_SDIO is actually 792Mhz.
>
> Therefore calculation of output SDCLK is incorrect now.
> The mmc driver sets the divisor to 4 times larger than it should be
> and emmc/sd works 4 times slower.
>
> This can be confirmed with fio test:
> Sequential read of emmc with fixed 198Mz clock:
> READ: bw=289MiB/s (303MB/s)
>
> Sequential read with CLK_EMMC_SDIO clock:
> READ: bw=82.6MiB/s (86.6MB/s)
>
> Let's fix this issue by providing fixed-factor-clock that divides
> CLK_EMMC_SDIO by 4 for emmc/sd nodes.

Thanks for finding this bug!

However, this feels like a work-around for a bug in the clock driver, and even
if there is a fixed factor divider somewhere this should probably be modelled
by the clock driver. Did you look into the documentation[1] and try to figure
out where eMMC clock comes from and where the /4 is missing?

There is also a vendor tree somewhere with a much more complete clock driver.
Drew do you remember where it is? Maybe it's worth looking at how that driver
models the eMMC clocks.

[1]: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf

/Emil

>
> Fixes: 03a20182e1e0 ("riscv: dts: thead: change TH1520 mmc nodes to use clock controller")
> Signed-off-by: Maksim Kiselev <bigunclemax@gmail.com>
> ---
>  arch/riscv/boot/dts/thead/th1520.dtsi | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index acfe030e803a..6c20965cd10c 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -229,6 +229,14 @@ stmmac_axi_config: stmmac-axi-config {
>  		snps,blen = <0 0 64 32 0 0 0>;
>  	};
>
> +	sdhci_clk: sdhci-clock {
> +		compatible = "fixed-factor-clock";
> +		clocks = <&clk CLK_EMMC_SDIO>;
> +		#clock-cells = <0>;
> +		clock-div = <4>;
> +		clock-mult = <1>;
> +	};
> +
>  	soc {
>  		compatible = "simple-bus";
>  		interrupt-parent = <&plic>;
> @@ -328,7 +336,7 @@ emmc: mmc@ffe7080000 {
>  			compatible = "thead,th1520-dwcmshc";
>  			reg = <0xff 0xe7080000 0x0 0x10000>;
>  			interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&clk CLK_EMMC_SDIO>;
> +			clocks = <&sdhci_clk>;
>  			clock-names = "core";
>  			status = "disabled";
>  		};
> @@ -337,7 +345,7 @@ sdio0: mmc@ffe7090000 {
>  			compatible = "thead,th1520-dwcmshc";
>  			reg = <0xff 0xe7090000 0x0 0x10000>;
>  			interrupts = <64 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&clk CLK_EMMC_SDIO>;
> +			clocks = <&sdhci_clk>;
>  			clock-names = "core";
>  			status = "disabled";
>  		};
> @@ -346,7 +354,7 @@ sdio1: mmc@ffe70a0000 {
>  			compatible = "thead,th1520-dwcmshc";
>  			reg = <0xff 0xe70a0000 0x0 0x10000>;
>  			interrupts = <71 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&clk CLK_EMMC_SDIO>;
> +			clocks = <&sdhci_clk>;
>  			clock-names = "core";
>  			status = "disabled";
>  		};
> --
> 2.45.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1] riscv: dts: thead: Fix TH1520 emmc and shdci clock rate

[Drew Fustini]

On Wed, Dec 04, 2024 at 03:19:28PM +0000, Emil Renner Berthing wrote:
> bigunclemax@ wrote:
> > From: Maksim Kiselev <bigunclemax@gmail.com>
> >
> > In accordance with LicheePi 4A BSP the clock that comes to emmc/sdhci
> > is 198Mhz.
> >
> > But changing from fixed-clock to CLK_EMMC_SDIO leads to increasing input
> > clock from 198Mhz to 792Mhz. Because the CLK_EMMC_SDIO is actually 792Mhz.
> >
> > Therefore calculation of output SDCLK is incorrect now.
> > The mmc driver sets the divisor to 4 times larger than it should be
> > and emmc/sd works 4 times slower.
> >
> > This can be confirmed with fio test:
> > Sequential read of emmc with fixed 198Mz clock:
> > READ: bw=289MiB/s (303MB/s)
> >
> > Sequential read with CLK_EMMC_SDIO clock:
> > READ: bw=82.6MiB/s (86.6MB/s)
> >
> > Let's fix this issue by providing fixed-factor-clock that divides
> > CLK_EMMC_SDIO by 4 for emmc/sd nodes.
> 
> Thanks for finding this bug!
> 
> However, this feels like a work-around for a bug in the clock driver, and even
> if there is a fixed factor divider somewhere this should probably be modelled
> by the clock driver. Did you look into the documentation[1] and try to figure
> out where eMMC clock comes from and where the /4 is missing?
> 
> There is also a vendor tree somewhere with a much more complete clock driver.
> Drew do you remember where it is? Maybe it's worth looking at how that driver
> models the eMMC clocks.

Sorry for the delay, I'm travelling until tomorrow.

Maksim, thanks for finding this issue and sending a patch.

That is a good point about checking the thead vendor kernel. I normally
look at revy's thead-kernel repo [1] which is 5.10. revy also has a 6.6
lts branch in th1520-linux-kernel [2].

https://github.com/revyos/thead-kernel/tree/lpi4a/drivers/clk/thead


Looking at line 454 in drivers/clk/thead/clk-light-fm.c [3]:

  clks[EMMC_SDIO_REF_CLK] =
  thead_light_clk_fixed_factor("emmc_sdio_ref_clk",
                               "video_pll_foutpostdiv", 1, 4)
                               /* Note: base clk is div 4 to 198M*/

Which derives from line 373:

  clks[VIDEO_PLL_FOUTPOSTDIV] =
  thead_clk_fixed("video_pll_foutpostdiv", 792000000);

Thanks,
Drew

[1] https://github.com/revyos/thead-kernel
[2] https://github.com/revyos/th1520-linux-kernel
[3] https://github.com/revyos/thead-kernel/blob/lpi4a/drivers/clk/thead/clk-light-fm.c

Re: [PATCH v1] riscv: dts: thead: Fix TH1520 emmc and shdci clock rate

[Maxim Kiselev]

Hi Emil, Drew

> Did you look into the documentation

Yes, I looked into th1520 user manual but found only
mention of emmc sdio ref clk which is 792Mhz.

> That is a good point about checking the thead vendor kernel.

Drew, thanks for the suggestion to look at Revy's BSP.
I'll make a patch for the clk controller in v2.

Best wishes,
Maksim