[PATCH 0/2] sunxi: clk/mmc: Fix T113-s3 MMC performance

[PATCH 0/2] sunxi: clk/mmc: Fix T113-s3 MMC performance

[Andre Przywara]

Hi,

as Kuba reported, the MMC performance of the Allwinner T113-s3 chips in
mainline Linux is only half of what's expected. Checking that in U-Boot
revealed that's actually only one fourth there, since on top of the same
bug as in the kernel, we also calculate the base PLL frequency wrongly.

Fix that by halving the frequency as reported by the clock function, and
apply an extra divider of two in the MMC code - to compensate for that
missing hidden divider (which is the same bug as in Linux).
Seeing only one fourth of the base frequency makes the calculated
internal divider go up by the factor of 4, so we get the MMC performance
back its expected levels: from around 5.8 MB/s to ~23 MB/s.

Cheers,
Andre

Andre Przywara (2):
  sunxi: mmc: Fix T113-s3 MMC clock divider
  sunxi: sun50i_h6: clock: fix PLL_PERIPH0 rate calculation

 arch/arm/mach-sunxi/clock_sun50i_h6.c | 15 ++++++++++-----
 drivers/mmc/sunxi_mmc.c               |  7 +++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider

[Andre Przywara]

On the Allwinner D1/R528/T113-s3 SoCs the MMC clock source selected by
mux value 1 is PLL_PERIPH0(1x), not (2x), as in the other SoCs.
But we have still the hidden divisor of 2 in the MMC mod clock, so
need to explicitly compensate for that on those SoCs.

This leads to the actually programmed clock rate to be double compared
to before, which increases the MMC performance on those SoCs.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 drivers/mmc/sunxi_mmc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
index 0b56d1405be..8f72d758e46 100644
--- a/drivers/mmc/sunxi_mmc.c
+++ b/drivers/mmc/sunxi_mmc.c
@@ -92,6 +92,13 @@ static int mmc_set_mod_clk(struct sunxi_mmc_priv *priv, unsigned int hz)
 		pll = CCM_MMC_CTRL_PLL6;
 		pll_hz = clock_get_pll6();
 #endif
+		/*
+		 * On the D1/R528/T113 mux source 1 refers to PLL_PERIPH0(1x),
+		 * like for the older SoCs. However we still have the hidden
+		 * divider of 2x, so compensate for that here.
+		 */
+		if (IS_ENABLED(CONFIG_MACH_SUN8I_R528))
+			pll_hz /= 2;
 	}
 
 	div = pll_hz / hz;
-- 
2.25.1

[PATCH 2/2] sunxi: sun50i_h6: clock: fix PLL_PERIPH0 rate calculation

[Andre Przywara]

On the Allwinner D1/R528/T113-s3 SoCs (NCAT2) the factors encoded in
the PLL register describe the doubled clock rate, as in the other SoCs.

Correct for that by always dividing the calculated rate by 2, except on
the H6, where we need a divisor of 4 (no change here).

This corrects the PERIPH0 clock rate as read by the MMC driver, and
actually doubles the MMC performance on those NCAT2 chips.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 arch/arm/mach-sunxi/clock_sun50i_h6.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-sunxi/clock_sun50i_h6.c b/arch/arm/mach-sunxi/clock_sun50i_h6.c
index b424a7893ea..359513d1669 100644
--- a/arch/arm/mach-sunxi/clock_sun50i_h6.c
+++ b/arch/arm/mach-sunxi/clock_sun50i_h6.c
@@ -147,15 +147,20 @@ unsigned int clock_get_pll6(void)
 	if (IS_ENABLED(CONFIG_SUNXI_GEN_NCAT2)) {
 		div1 = ((rval & CCM_PLL6_CTRL_P0_MASK) >>
 			CCM_PLL6_CTRL_P0_SHIFT) + 1;
-		m = 1;
 	} else {
 		div1 = ((rval & CCM_PLL6_CTRL_DIV1_MASK) >>
 			CCM_PLL6_CTRL_DIV1_SHIFT) + 1;
-		if (IS_ENABLED(CONFIG_MACH_SUN50I_H6))
-			m = 4;
-		else
-			m = 2;
 	}
 
+	/*
+	 * The factors encoded in the register describe the doubled clock
+	 * frequency, expect for the H6, where it's the quadrupled frequency.
+	 * Compensate for that here.
+	 */
+	if (IS_ENABLED(CONFIG_MACH_SUN50I_H6))
+		m = 4;
+	else
+		m = 2;
+
 	return 24000000U * n / m / div1 / div2;
 }
-- 
2.25.1

Re: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider

[Jernej Škrabec]

Dne sreda, 26. februar 2025 ob 12:37:11 Srednjeevropski standardni čas je Andre Przywara napisal(a):
> On the Allwinner D1/R528/T113-s3 SoCs the MMC clock source selected by
> mux value 1 is PLL_PERIPH0(1x), not (2x), as in the other SoCs.
> But we have still the hidden divisor of 2 in the MMC mod clock, so
> need to explicitly compensate for that on those SoCs.
> 
> This leads to the actually programmed clock rate to be double compared
> to before, which increases the MMC performance on those SoCs.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

Re: [PATCH 2/2] sunxi: sun50i_h6: clock: fix PLL_PERIPH0 rate calculation

[Jernej Škrabec]

Dne sreda, 26. februar 2025 ob 12:37:12 Srednjeevropski standardni čas je Andre Przywara napisal(a):
> On the Allwinner D1/R528/T113-s3 SoCs (NCAT2) the factors encoded in
> the PLL register describe the doubled clock rate, as in the other SoCs.
> 
> Correct for that by always dividing the calculated rate by 2, except on
> the H6, where we need a divisor of 4 (no change here).
> 
> This corrects the PERIPH0 clock rate as read by the MMC driver, and
> actually doubles the MMC performance on those NCAT2 chips.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

RE: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider

[Peng Fan]

Hi Andre,

> Subject: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider
> 
> On the Allwinner D1/R528/T113-s3 SoCs the MMC clock source
> selected by mux value 1 is PLL_PERIPH0(1x), not (2x), as in the other
> SoCs.
> But we have still the hidden divisor of 2 in the MMC mod clock, so
> need to explicitly compensate for that on those SoCs.
> 
> This leads to the actually programmed clock rate to be double
> compared to before, which increases the MMC performance on those
> SoCs.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  drivers/mmc/sunxi_mmc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index 0b56d1405be..8f72d758e46 100644
> --- a/drivers/mmc/sunxi_mmc.c
> +++ b/drivers/mmc/sunxi_mmc.c
> @@ -92,6 +92,13 @@ static int mmc_set_mod_clk(struct
> sunxi_mmc_priv *priv, unsigned int hz)
>  		pll = CCM_MMC_CTRL_PLL6;
>  		pll_hz = clock_get_pll6();
>  #endif
> +		/*
> +		 * On the D1/R528/T113 mux source 1 refers to
> PLL_PERIPH0(1x),
> +		 * like for the older SoCs. However we still have the
> hidden
> +		 * divider of 2x, so compensate for that here.
> +		 */
> +		if (IS_ENABLED(CONFIG_MACH_SUN8I_R528))

Use CONFIG_IS_ENABLED(MACH_SUN8I_R528)?

Regards,
Peng.

> +			pll_hz /= 2;
>  	}
> 
>  	div = pll_hz / hz;
> --
> 2.25.1

RE: [PATCH 2/2] sunxi: sun50i_h6: clock: fix PLL_PERIPH0 rate calculation

[Peng Fan]

> Subject: [PATCH 2/2] sunxi: sun50i_h6: clock: fix PLL_PERIPH0 rate
> calculation
> 
> On the Allwinner D1/R528/T113-s3 SoCs (NCAT2) the factors encoded
> in the PLL register describe the doubled clock rate, as in the other SoCs.
> 
> Correct for that by always dividing the calculated rate by 2, except on
> the H6, where we need a divisor of 4 (no change here).
> 
> This corrects the PERIPH0 clock rate as read by the MMC driver, and
> actually doubles the MMC performance on those NCAT2 chips.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  arch/arm/mach-sunxi/clock_sun50i_h6.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/clock_sun50i_h6.c b/arch/arm/mach-
> sunxi/clock_sun50i_h6.c
> index b424a7893ea..359513d1669 100644
> --- a/arch/arm/mach-sunxi/clock_sun50i_h6.c
> +++ b/arch/arm/mach-sunxi/clock_sun50i_h6.c
> @@ -147,15 +147,20 @@ unsigned int clock_get_pll6(void)
>  	if (IS_ENABLED(CONFIG_SUNXI_GEN_NCAT2)) {
>  		div1 = ((rval & CCM_PLL6_CTRL_P0_MASK) >>
>  			CCM_PLL6_CTRL_P0_SHIFT) + 1;
> -		m = 1;
>  	} else {
>  		div1 = ((rval & CCM_PLL6_CTRL_DIV1_MASK) >>
>  			CCM_PLL6_CTRL_DIV1_SHIFT) + 1;
> -		if (IS_ENABLED(CONFIG_MACH_SUN50I_H6))
> -			m = 4;
> -		else
> -			m = 2;
>  	}
> 
> +	/*
> +	 * The factors encoded in the register describe the doubled
> clock
> +	 * frequency, expect for the H6, where it's the quadrupled
> frequency.
> +	 * Compensate for that here.
> +	 */
> +	if (IS_ENABLED(CONFIG_MACH_SUN50I_H6))
> +		m = 4;
> +	else
> +		m = 2;
> +

Nit: not sure you need CONFIG_IS_ENABLED or not. 

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Regards
Peng

>  	return 24000000U * n / m / div1 / div2;  }
> --
> 2.25.1

Re: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Thu, Feb 27, 2025 at 12:31:59AM +0000, Peng Fan wrote:
> Hi Andre,
> 
> > Subject: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider
> > 
> > On the Allwinner D1/R528/T113-s3 SoCs the MMC clock source
> > selected by mux value 1 is PLL_PERIPH0(1x), not (2x), as in the other
> > SoCs.
> > But we have still the hidden divisor of 2 in the MMC mod clock, so
> > need to explicitly compensate for that on those SoCs.
> > 
> > This leads to the actually programmed clock rate to be double
> > compared to before, which increases the MMC performance on those
> > SoCs.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> > ---
> >  drivers/mmc/sunxi_mmc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> > index 0b56d1405be..8f72d758e46 100644
> > --- a/drivers/mmc/sunxi_mmc.c
> > +++ b/drivers/mmc/sunxi_mmc.c
> > @@ -92,6 +92,13 @@ static int mmc_set_mod_clk(struct
> > sunxi_mmc_priv *priv, unsigned int hz)
> >  		pll = CCM_MMC_CTRL_PLL6;
> >  		pll_hz = clock_get_pll6();
> >  #endif
> > +		/*
> > +		 * On the D1/R528/T113 mux source 1 refers to
> > PLL_PERIPH0(1x),
> > +		 * like for the older SoCs. However we still have the
> > hidden
> > +		 * divider of 2x, so compensate for that here.
> > +		 */
> > +		if (IS_ENABLED(CONFIG_MACH_SUN8I_R528))
> 
> Use CONFIG_IS_ENABLED(MACH_SUN8I_R528)?

No, either is fine, IS_ENABLED(CONFIG_FOO) is less confusing for the
case where there's never a CONFIG_xPL_FOO.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider

[Peng Fan]

> Subject: Re: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider
> 
> On Thu, Feb 27, 2025 at 12:31:59AM +0000, Peng Fan wrote:
> > Hi Andre,
> >
> > > Subject: [PATCH 1/2] sunxi: mmc: Fix T113-s3 MMC clock divider
> > >
> > > On the Allwinner D1/R528/T113-s3 SoCs the MMC clock source
> selected
> > > by mux value 1 is PLL_PERIPH0(1x), not (2x), as in the other SoCs.
> > > But we have still the hidden divisor of 2 in the MMC mod clock, so
> > > need to explicitly compensate for that on those SoCs.
> > >
> > > This leads to the actually programmed clock rate to be double
> > > compared to before, which increases the MMC performance on
> those
> > > SoCs.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > Reported-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> > > ---
> > >  drivers/mmc/sunxi_mmc.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index
> > > 0b56d1405be..8f72d758e46 100644
> > > --- a/drivers/mmc/sunxi_mmc.c
> > > +++ b/drivers/mmc/sunxi_mmc.c
> > > @@ -92,6 +92,13 @@ static int mmc_set_mod_clk(struct
> sunxi_mmc_priv
> > > *priv, unsigned int hz)
> > >  		pll = CCM_MMC_CTRL_PLL6;
> > >  		pll_hz = clock_get_pll6();
> > >  #endif
> > > +		/*
> > > +		 * On the D1/R528/T113 mux source 1 refers to
> > > PLL_PERIPH0(1x),
> > > +		 * like for the older SoCs. However we still have the
> > > hidden
> > > +		 * divider of 2x, so compensate for that here.
> > > +		 */
> > > +		if (IS_ENABLED(CONFIG_MACH_SUN8I_R528))
> >
> > Use CONFIG_IS_ENABLED(MACH_SUN8I_R528)?
> 
> No, either is fine, IS_ENABLED(CONFIG_FOO) is less confusing for the
> case where there's never a CONFIG_xPL_FOO.

I see.

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> --
> Tom