[PATCH v2 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock

[PATCH v2 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock

[Niklas Söderlund]

Hi Geert,

This series aims to solve the clock quirk needed to enabled HS400 on 
SoCs needing special clock handeling. It uses the same method as v1 of 
this series and which was discussed during the SDHI hackathon. However 
patch 2/2 have been completely rewritten to take your comments from v1 
into account. Due to this change this series now depends on [1].

This is tested on H3 (ES1.0, ES2.0), M3-W (ES1.0) and M3-N together with
patches to enable HS400 with great results. No regressions found for
eMMC HS200/HS400 modes nor for SDR{25,50,104} on any of the SoCs.

Patch 1/2 adds documentation on which settings is used while 2/2 is the
real change where the quirk is implemented.

1. [PATCH] clk: renesas: rcar-gen3: set state when registering SD clocks

Niklas Söderlund (2):
  clk: renesas: rcar-gen3: add documentation for SD clocks
  clk: renesas: rcar-gen3: add HS400 quirk for SD clock

 drivers/clk/renesas/rcar-gen3-cpg.c | 43 +++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 12 deletions(-)

-- 
2.19.1

[PATCH v2 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks

[Niklas Söderlund]

Document the known use cases of the different clock settings. This is
useful as different SoC and ES versions uses different settings to do
the same thing as there are more then one combination to achieve the
same SDn clock speed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index f6335357c85505df..bca6c7f51de18db7 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -232,13 +232,13 @@ struct sd_clock {
  *                     sd_srcfc   sd_fc   div
  * stp_hck   stp_ck    (div)      (div)     = sd_srcfc x sd_fc
  *-------------------------------------------------------------------
- *  0         0         0 (1)      1 (4)      4
- *  0         0         1 (2)      1 (4)      8
- *  1         0         2 (4)      1 (4)     16
- *  1         0         3 (8)      1 (4)     32
+ *  0         0         0 (1)      1 (4)      4 : SDR104 / HS200 / HS400 (8 TAP)
+ *  0         0         1 (2)      1 (4)      8 : SDR50
+ *  1         0         2 (4)      1 (4)     16 : HS / SDR25
+ *  1         0         3 (8)      1 (4)     32 : NS / SDR12
  *  1         0         4 (16)     1 (4)     64
  *  0         0         0 (1)      0 (2)      2
- *  0         0         1 (2)      0 (2)      4
+ *  0         0         1 (2)      0 (2)      4 : SDR104 / HS200 / HS400 (4 TAP)
  *  1         0         2 (4)      0 (2)      8
  *  1         0         3 (8)      0 (2)     16
  *  1         0         4 (16)     0 (2)     32
-- 
2.19.1

[PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Niklas Söderlund]

On H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1) the clock setting for HS400
needs a quirk to function properly. The reason for the quirk is that
there are two settings which produces same divider value for the SDn
clock. On the effected boards the one currently selected results in
HS400 not working.

This change uses the same method as the Gen2 CPG driver and simply
ignores the first clock setting as this is the offending one when
selecting the settings. Which of the two possible settings is used have
no effect for SDR104.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

---
* Changes since v1
- Fixed spelling in commit message, thanks Sergei and Geert!
- Reworked the whole patch per Geerts suggestion. Instead of only
  skipping the first row on the effected boards when setting the clock
  rete totally ignore it. This is made possible by another change to the
  clock driver posted separately from this series and which this patch
  now depends on [1].

  1. [PATCH] clk: renesas: rcar-gen3: set state when registering SD clocks
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 33 +++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index bca6c7f51de18db7..bad062150cd486f6 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -242,6 +242,10 @@ struct sd_clock {
  *  1         0         2 (4)      0 (2)      8
  *  1         0         3 (8)      0 (2)     16
  *  1         0         4 (16)     0 (2)     32
+ *
+ *  NOTE: There is a quirk option to ignore the first row of the dividers
+ *  table when searching for suitable settings. This is because HS400 on
+ *  early ES versions of H3 and M3-W requires a specific setting to work.
  */
 static const struct sd_div_table cpg_sd_div_table[] = {
 /*	CPG_SD_DIV_TABLE_DATA(stp_hck,  stp_ck,   sd_srcfc,   sd_fc,  sd_div) */
@@ -352,6 +356,12 @@ static const struct clk_ops cpg_sd_clock_ops = {
 	.set_rate = cpg_sd_clock_set_rate,
 };
 
+static u32 cpg_quirks __initdata;
+
+#define PLL_ERRATA	BIT(0)		/* Missing PLL0/2/4 post-divider */
+#define RCKCR_CKSEL	BIT(1)		/* Manual RCLK parent selection */
+#define SD_SKIP_FIRST	BIT(2)		/* Skip first clock in SD table */
+
 static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
 	void __iomem *base, const char *parent_name,
 	struct raw_notifier_head *notifiers)
@@ -377,6 +387,11 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
 	clock->div_table = cpg_sd_div_table;
 	clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
 
+	if (cpg_quirks & SD_SKIP_FIRST) {
+		clock->div_table++;
+		clock->div_num--;
+	}
+
 	val = readl(clock->csn.reg) & ~CPG_SD_FC_MASK;
 	val |= CPG_SD_STP_MASK | (clock->div_table[0].val & CPG_SD_FC_MASK);
 	writel(val, clock->csn.reg);
@@ -406,23 +421,27 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
 static const struct rcar_gen3_cpg_pll_config *cpg_pll_config __initdata;
 static unsigned int cpg_clk_extalr __initdata;
 static u32 cpg_mode __initdata;
-static u32 cpg_quirks __initdata;
-
-#define PLL_ERRATA	BIT(0)		/* Missing PLL0/2/4 post-divider */
-#define RCKCR_CKSEL	BIT(1)		/* Manual RCLK parent selection */
 
 static const struct soc_device_attribute cpg_quirks_match[] __initconst = {
 	{
 		.soc_id = "r8a7795", .revision = "ES1.0",
-		.data = (void *)(PLL_ERRATA | RCKCR_CKSEL),
+		.data = (void *)(PLL_ERRATA | RCKCR_CKSEL | SD_SKIP_FIRST),
 	},
 	{
 		.soc_id = "r8a7795", .revision = "ES1.*",
-		.data = (void *)RCKCR_CKSEL,
+		.data = (void *)(RCKCR_CKSEL | SD_SKIP_FIRST),
+	},
+	{
+		.soc_id = "r8a7795", .revision = "ES2.0",
+		.data = (void *)SD_SKIP_FIRST,
 	},
 	{
 		.soc_id = "r8a7796", .revision = "ES1.0",
-		.data = (void *)RCKCR_CKSEL,
+		.data = (void *)(RCKCR_CKSEL | SD_SKIP_FIRST),
+	},
+	{
+		.soc_id = "r8a7796", .revision = "ES1.1",
+		.data = (void *)SD_SKIP_FIRST,
 	},
 	{ /* sentinel */ }
 };
-- 
2.19.1

Re: [PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Wolfram Sang]

Hi Niklas,

thanks for the patches!

On Thu, Nov 29, 2018 at 01:39:49AM +0100, Niklas Söderlund wrote:
> On H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1) the clock setting for HS400
> needs a quirk to function properly. The reason for the quirk is that
> there are two settings which produces same divider value for the SDn
> clock. On the effected boards the one currently selected results in
> HS400 not working.
> 
> This change uses the same method as the Gen2 CPG driver and simply
> ignores the first clock setting as this is the offending one when
> selecting the settings. Which of the two possible settings is used have
> no effect for SDR104.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> ---
> * Changes since v1
> - Fixed spelling in commit message, thanks Sergei and Geert!
> - Reworked the whole patch per Geerts suggestion. Instead of only
>   skipping the first row on the effected boards when setting the clock
>   rete totally ignore it. This is made possible by another change to the

"rete"? I don't get this sentence and I think it is important to
understand when reviewing these patches :)

>   clock driver posted separately from this series and which this patch
>   now depends on [1].

Hmm, why didn't you add it to the series then?

Still, all in all, seems we are on a nice track for having HS400 in the
next release \o/ Now, if that doesn't justify the 5.0 jump... ;D

Regards,

   Wolfram

Re: [PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Niklas Söderlund]

Hi Wolfram,

Thanks for your feedback.

On 2018-11-29 17:54:34 +0100, Wolfram Sang wrote:
> Hi Niklas,
> 
> thanks for the patches!
> 
> On Thu, Nov 29, 2018 at 01:39:49AM +0100, Niklas S�derlund wrote:
> > On H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1) the clock setting for HS400
> > needs a quirk to function properly. The reason for the quirk is that
> > there are two settings which produces same divider value for the SDn
> > clock. On the effected boards the one currently selected results in
> > HS400 not working.
> > 
> > This change uses the same method as the Gen2 CPG driver and simply
> > ignores the first clock setting as this is the offending one when
> > selecting the settings. Which of the two possible settings is used have
> > no effect for SDR104.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > ---
> > * Changes since v1
> > - Fixed spelling in commit message, thanks Sergei and Geert!
> > - Reworked the whole patch per Geerts suggestion. Instead of only
> >   skipping the first row on the effected boards when setting the clock
> >   rete totally ignore it. This is made possible by another change to the
> 
> "rete"? I don't get this sentence and I think it is important to
> understand when reviewing these patches :)

That should have been rate :-) To elaborate a bit more:

The patch is different from v1 as a different approach to solve the 
issue have been found. Instead of only ignoring the first row of the 
list of possible settings when selecting which divider to use also 
ignore it when examining which state the hardware is in. That is the 
driver is no longer aware the first row exists with this patch.

This was in v1 not possible as the first row might be a state the 
bootloader left the hardware in and then the clock failed to register as 
it would need to update its own state to match the hardware.

As the driver needed to know about the state the hardware was in when 
probing but not use it when selecting a divider the more complex v1 was 
needed. When selecting a divider we wish for it to select the second 
option for the divider value '4' when running on a SoC which needs the 
quirk.

With v2 which depends on [1] this is not needed as the clock driver now 
sets a know state when registering the clock so this patch can be made 
much simpler by simply 'removing' the first row from all operations.

> 
> >   clock driver posted separately from this series and which this patch
> >   now depends on [1].
> 
> Hmm, why didn't you add it to the series then?

Since it is unrelated to this series I thought it best to post it as a 
separate patch as I think it has value to create a known starting state 
disregarding where this series ends up :-)

> 
> Still, all in all, seems we are on a nice track for having HS400 in the
> next release \o/ Now, if that doesn't justify the 5.0 jump... ;D

I sure hope so!

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Wolfram Sang]

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

Hi Niklas,

> That should have been rate :-) To elaborate a bit more:
> 
> The patch is different from v1 as a different approach to solve the 
> issue have been found. Instead of only ignoring the first row of the 
> list of possible settings when selecting which divider to use also 
> ignore it when examining which state the hardware is in. That is the 
> driver is no longer aware the first row exists with this patch.
> 
> This was in v1 not possible as the first row might be a state the 
> bootloader left the hardware in and then the clock failed to register as 
> it would need to update its own state to match the hardware.
> 
> As the driver needed to know about the state the hardware was in when 
> probing but not use it when selecting a divider the more complex v1 was 
> needed. When selecting a divider we wish for it to select the second 
> option for the divider value '4' when running on a SoC which needs the 
> quirk.
> 
> With v2 which depends on [1] this is not needed as the clock driver now 
> sets a know state when registering the clock so this patch can be made 
> much simpler by simply 'removing' the first row from all operations.

Thanks for the elaboration. I still think the above is the way to go and
I still wonder a little why it was implemented differently beforehand.

> > Hmm, why didn't you add it to the series then?
> 
> Since it is unrelated to this series I thought it best to post it as a 
> separate patch as I think it has value to create a known starting state 
> disregarding where this series ends up :-)

Yes, can be argued.

Thanks,

   Wolfram

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

Re: [PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Nov 30, 2018 at 12:47 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > That should have been rate :-) To elaborate a bit more:
> >
> > The patch is different from v1 as a different approach to solve the
> > issue have been found. Instead of only ignoring the first row of the
> > list of possible settings when selecting which divider to use also
> > ignore it when examining which state the hardware is in. That is the
> > driver is no longer aware the first row exists with this patch.
> >
> > This was in v1 not possible as the first row might be a state the
> > bootloader left the hardware in and then the clock failed to register as
> > it would need to update its own state to match the hardware.
> >
> > As the driver needed to know about the state the hardware was in when
> > probing but not use it when selecting a divider the more complex v1 was
> > needed. When selecting a divider we wish for it to select the second
> > option for the divider value '4' when running on a SoC which needs the
> > quirk.
> >
> > With v2 which depends on [1] this is not needed as the clock driver now
> > sets a know state when registering the clock so this patch can be made
> > much simpler by simply 'removing' the first row from all operations.
>
> Thanks for the elaboration. I still think the above is the way to go and
> I still wonder a little why it was implemented differently beforehand.

Probably the initial implementation didn't care at all about selecting clock
speeds (no fancy SDR/HS modes supported), so just used whatever U-Boot
had set up.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Wolfram Sang]

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

On Thu, Nov 29, 2018 at 01:39:49AM +0100, Niklas Söderlund wrote:
> On H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1) the clock setting for HS400
> needs a quirk to function properly. The reason for the quirk is that
> there are two settings which produces same divider value for the SDn
> clock. On the effected boards the one currently selected results in
> HS400 not working.
> 
> This change uses the same method as the Gen2 CPG driver and simply
> ignores the first clock setting as this is the offending one when
> selecting the settings. Which of the two possible settings is used have
> no effect for SDR104.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Tested on H3 ES1.0 and ES2.0, M3W ES1.0, and M3N with eMMC and UHS-SD
cards. Proper clock speeds were selected and performance matches.

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v2 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Geert Uytterhoeven]

On Thu, Nov 29, 2018 at 1:41 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1) the clock setting for HS400
> needs a quirk to function properly. The reason for the quirk is that
> there are two settings which produces same divider value for the SDn
> clock. On the effected boards the one currently selected results in
> HS400 not working.
>
> This change uses the same method as the Gen2 CPG driver and simply
> ignores the first clock setting as this is the offending one when
> selecting the settings. Which of the two possible settings is used have
> no effect for SDR104.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> ---
> * Changes since v1
> - Fixed spelling in commit message, thanks Sergei and Geert!
> - Reworked the whole patch per Geerts suggestion. Instead of only
>   skipping the first row on the effected boards when setting the clock
>   rete totally ignore it. This is made possible by another change to the
>   clock driver posted separately from this series and which this patch
>   now depends on [1].
>
>   1. [PATCH] clk: renesas: rcar-gen3: set state when registering SD clocks

Thanks for the update!

With s/rete/rate/:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds