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

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

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Hi Geert,

This is the result of the SDHI hackathon for a possible solution to the 
clock issue on early ES versions. It is based on the Gen2 solution where 
a row of the possible clock settings are ignored on the effected SoC+ES 
versions. The first row is not effected when reading settings left by 
the bootloader, only when the setting the clock.

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.

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 | 38 ++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 11 deletions(-)

-- 
2.19.1

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

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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>
---
 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 628b63b85d3f09c5..ff56ac6134111c38 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -233,13 +233,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 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

On H3 (ES1.0,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 vale for the SDn
clock. On the effected boards the one currently selected results in HS00
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>
---
 drivers/clk/renesas/rcar-gen3-cpg.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index ff56ac6134111c38..8cc524a29c855dd2 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -227,6 +227,7 @@ struct sd_clock {
 	unsigned int div_min;
 	unsigned int div_max;
 	unsigned int cur_div_idx;
+	bool skip_first;
 };
 
 /* SDn divider
@@ -243,6 +244,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) */
@@ -327,7 +332,7 @@ static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
 	u32 val;
 	unsigned int i;
 
-	for (i = 0; i < clock->div_num; i++)
+	for (i = clock->skip_first ? 1 : 0; i < clock->div_num; i++)
 		if (div == clock->div_table[i].div)
 			break;
 
@@ -355,7 +360,7 @@ static const struct clk_ops cpg_sd_clock_ops = {
 
 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)
+	struct raw_notifier_head *notifiers, bool skip_first)
 {
 	struct clk_init_data init;
 	struct sd_clock *clock;
@@ -377,6 +382,7 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
 	clock->hw.init = &init;
 	clock->div_table = cpg_sd_div_table;
 	clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
+	clock->skip_first = skip_first;
 
 	sd_fc = readl(clock->csn.reg) & CPG_SD_FC_MASK;
 	for (i = 0; i < clock->div_num; i++)
@@ -417,19 +423,28 @@ 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 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 */ }
 };
@@ -504,7 +519,8 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
 
 	case CLK_TYPE_GEN3_SD:
 		return cpg_sd_clk_register(core, base, __clk_get_name(parent),
-					   notifiers);
+					   notifiers,
+					   cpg_quirks & SD_SKIP_FIRST);
 
 	case CLK_TYPE_GEN3_R:
 		if (cpg_quirks & RCKCR_CKSEL) {
-- 
2.19.1

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

[Wolfram Sang]

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

On Thu, Nov 01, 2018 at 12:25:17AM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> 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>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

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

[Wolfram Sang]

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

On Thu, Nov 01, 2018 at 12:25:18AM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> On H3 (ES1.0,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 vale for the SDn
> clock. On the effected boards the one currently selected results in HS00
> 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>

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


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

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

[Wolfram Sang]

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


> This is the result of the SDHI hackathon for a possible solution to the 
> clock issue on early ES versions. It is based on the Gen2 solution where 
> a row of the possible clock settings are ignored on the effected SoC+ES 
> versions. The first row is not effected when reading settings left by 
> the bootloader, only when the setting the clock.

We really thought about exposing both clocks, SDn and SDHn. I got
convinced to use this less intrusive approach to get HS400 reliably to
work as a first step. "First make it work, then make it beautiful" so to
say...


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

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

[Sergei Shtylyov]

Hello!

On 11/01/2018 02:25 AM, Niklas Söderlund wrote:

> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> On H3 (ES1.0,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 vale for the SDn

   s/vale/value/.

> clock. On the effected boards the one currently selected results in HS00
> 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>
[...]

MBR, Sergei

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

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Nov 1, 2018 at 8:37 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Nov 01, 2018 at 12:25:17AM +0100, Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > 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>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Is a Tested-by appropriate for a comment-only patch?

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 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock

[Geert Uytterhoeven]

Hi Niklas,

On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> This is the result of the SDHI hackathon for a possible solution to the
> clock issue on early ES versions. It is based on the Gen2 solution where
> a row of the possible clock settings are ignored on the effected SoC+ES
> versions. The first row is not effected when reading settings left by
> the bootloader, only when the setting the clock.

To clarify, you decided not to describe the SDH clock in DT, but went for
heuristics with a quirk instead?

Is there any chance this can start to bite us in the future?

Thanks!

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 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Geert Uytterhoeven]

Hi Niklas,

Thanks for your patch!

On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> On H3 (ES1.0,ES2.0) and M3-W (ES1.0,ES1.1) the clock setting for HS400

H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1)

> needs a quirk to function properly. The reason for the quirk is that
> there are two settings which produces same divider vale for the SDn
> clock. On the effected boards the one currently selected results in HS00

HS200 or 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>
> ---
>  drivers/clk/renesas/rcar-gen3-cpg.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> index ff56ac6134111c38..8cc524a29c855dd2 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -227,6 +227,7 @@ struct sd_clock {
>         unsigned int div_min;
>         unsigned int div_max;
>         unsigned int cur_div_idx;
> +       bool skip_first;

I think you can do without adding this field...

>  };
>
>  /* SDn divider
> @@ -243,6 +244,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) */
> @@ -327,7 +332,7 @@ static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
>         u32 val;
>         unsigned int i;
>
> -       for (i = 0; i < clock->div_num; i++)
> +       for (i = clock->skip_first ? 1 : 0; i < clock->div_num; i++)

... and without this change (see below)

>                 if (div == clock->div_table[i].div)
>                         break;
>
> @@ -355,7 +360,7 @@ static const struct clk_ops cpg_sd_clock_ops = {
>
>  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)
> +       struct raw_notifier_head *notifiers, bool skip_first)

I think you can do without adding this parameter.

>  {
>         struct clk_init_data init;
>         struct sd_clock *clock;
> @@ -377,6 +382,7 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
>         clock->hw.init = &init;
>         clock->div_table = cpg_sd_div_table;
>         clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> +       clock->skip_first = skip_first;

What about

    if (cpg_quirks & SD_SKIP_FIRST) {
            clock->div_table++;
            clock->div_num--;
    }

instead?

It does require moving cpg_quirks and the quirk definitions up, but reduces
the actual code change, and makes the code easier to follow.

>
>         sd_fc = readl(clock->csn.reg) & CPG_SD_FC_MASK;
>         for (i = 0; i < clock->div_num; i++)
> @@ -417,19 +423,28 @@ 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 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 */ }
>  };
> @@ -504,7 +519,8 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>
>         case CLK_TYPE_GEN3_SD:
>                 return cpg_sd_clk_register(core, base, __clk_get_name(parent),
> -                                          notifiers);
> +                                          notifiers,
> +                                          cpg_quirks & SD_SKIP_FIRST);
>
>         case CLK_TYPE_GEN3_R:
>                 if (cpg_quirks & RCKCR_CKSEL) {

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 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2018-11-05 11:32:15 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Thu, Nov 1, 2018 at 12:26 AM Niklas S�derlund
> <niklas.soderlund@ragnatech.se> wrote:
> > This is the result of the SDHI hackathon for a possible solution to the
> > clock issue on early ES versions. It is based on the Gen2 solution where
> > a row of the possible clock settings are ignored on the effected SoC+ES
> > versions. The first row is not effected when reading settings left by
> > the bootloader, only when the setting the clock.
> 
> To clarify, you decided not to describe the SDH clock in DT, but went for
> heuristics with a quirk instead?
> 
> Is there any chance this can start to bite us in the future?

We discussed this at the SDHI hackfest and we could not think of any 
future situation where this could come back and bite us.

> 
> Thanks!
> 
> 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

-- 
Regards,
Niklas S�derlund

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

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> Thanks for your patch!
> 
> On Thu, Nov 1, 2018 at 12:26 AM Niklas S�derlund
> <niklas.soderlund@ragnatech.se> wrote:
> > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > On H3 (ES1.0,ES2.0) and M3-W (ES1.0,ES1.1) the clock setting for HS400
> 
> H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1)

Thanks.

> 
> > needs a quirk to function properly. The reason for the quirk is that
> > there are two settings which produces same divider vale for the SDn
> > clock. On the effected boards the one currently selected results in HS00
> 
> HS200 or HS400?

Wops, 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>
> > ---
> >  drivers/clk/renesas/rcar-gen3-cpg.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> > index ff56ac6134111c38..8cc524a29c855dd2 100644
> > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> > @@ -227,6 +227,7 @@ struct sd_clock {
> >         unsigned int div_min;
> >         unsigned int div_max;
> >         unsigned int cur_div_idx;
> > +       bool skip_first;
> 
> I think you can do without adding this field...
> 
> >  };
> >
> >  /* SDn divider
> > @@ -243,6 +244,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) */
> > @@ -327,7 +332,7 @@ static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
> >         u32 val;
> >         unsigned int i;
> >
> > -       for (i = 0; i < clock->div_num; i++)
> > +       for (i = clock->skip_first ? 1 : 0; i < clock->div_num; i++)
> 
> ... and without this change (see below)
> 
> >                 if (div == clock->div_table[i].div)
> >                         break;
> >
> > @@ -355,7 +360,7 @@ static const struct clk_ops cpg_sd_clock_ops = {
> >
> >  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)
> > +       struct raw_notifier_head *notifiers, bool skip_first)
> 
> I think you can do without adding this parameter.
> 
> >  {
> >         struct clk_init_data init;
> >         struct sd_clock *clock;
> > @@ -377,6 +382,7 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
> >         clock->hw.init = &init;
> >         clock->div_table = cpg_sd_div_table;
> >         clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> > +       clock->skip_first = skip_first;
> 
> What about
> 
>     if (cpg_quirks & SD_SKIP_FIRST) {
>             clock->div_table++;
>             clock->div_num--;
>     }
> 
> instead?

This was my first approach as well, unfortunately it won't work :-(

If the bootloader leaves the clock settings in a state which matches the 
first row in the table the clock fails to register as the check in 
cpg_sd_clk_register() makes sure it have a row for the state the 
hardware is in. If it can't find that state the clock fails to register. 
Whit this quirk the limitation of the effected boards only have an 
effect when setting the clock.

I thought this solution solved this quiet neatly with the robustness 
principle, 'Be conservative in what you do, be liberal in what you 
accept from others' :-)

I will wait for your feedback on this before sending a updated version 
of this series addressing the other comments.

> 
> It does require moving cpg_quirks and the quirk definitions up, but reduces
> the actual code change, and makes the code easier to follow.
> 
> >
> >         sd_fc = readl(clock->csn.reg) & CPG_SD_FC_MASK;
> >         for (i = 0; i < clock->div_num; i++)
> > @@ -417,19 +423,28 @@ 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 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 */ }
> >  };
> > @@ -504,7 +519,8 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
> >
> >         case CLK_TYPE_GEN3_SD:
> >                 return cpg_sd_clk_register(core, base, __clk_get_name(parent),
> > -                                          notifiers);
> > +                                          notifiers,
> > +                                          cpg_quirks & SD_SKIP_FIRST);
> >
> >         case CLK_TYPE_GEN3_R:
> >                 if (cpg_quirks & RCKCR_CKSEL) {
> 
> 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

-- 
Regards,
Niklas S�derlund

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

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Nov 5, 2018 at 4:07 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> > On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >
> > > On H3 (ES1.0,ES2.0) and M3-W (ES1.0,ES1.1) the clock setting for HS400
> >
> > H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1)
>
> Thanks.
>
> >
> > > needs a quirk to function properly. The reason for the quirk is that
> > > there are two settings which produces same divider vale for the SDn
> > > clock. On the effected boards the one currently selected results in HS00
> >
> > HS200 or HS400?
>
> Wops, 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>
> > > ---
> > >  drivers/clk/renesas/rcar-gen3-cpg.c | 28 ++++++++++++++++++++++------
> > >  1 file changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> > > index ff56ac6134111c38..8cc524a29c855dd2 100644
> > > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c

> > > @@ -377,6 +382,7 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
> > >         clock->hw.init = &init;
> > >         clock->div_table = cpg_sd_div_table;
> > >         clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> > > +       clock->skip_first = skip_first;
> >
> > What about
> >
> >     if (cpg_quirks & SD_SKIP_FIRST) {
> >             clock->div_table++;
> >             clock->div_num--;
> >     }
> >
> > instead?
>
> This was my first approach as well, unfortunately it won't work :-(
>
> If the bootloader leaves the clock settings in a state which matches the
> first row in the table the clock fails to register as the check in
> cpg_sd_clk_register() makes sure it have a row for the state the
> hardware is in. If it can't find that state the clock fails to register.
> Whit this quirk the limitation of the effected boards only have an
> effect when setting the clock.

IC...

> I thought this solution solved this quiet neatly with the robustness
> principle, 'Be conservative in what you do, be liberal in what you
> accept from others' :-)

Will it ever use the settings as inherited from boot loader or reset state?
If it does, I assume it will fail badly, due to an inconsistency between the
SD and SDH clock rates?
And what if the kernel is ever booted when the SDnSRCFC or SDnFC field
has an invalid value? Then the driver will fail, too?

Hence, isn't it safer to initialize the registers to a known safe value?

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 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock

[Wolfram Sang]

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

Hi Geert,

> Is there any chance this can start to bite us in the future?

Well, there is always a chance but to the best of our current knowledge,
we can't see it for Gen3. And even then, we can still fix it.

I was entering SDHI hackfest with the attitude of representing the
hardware which means exposing SDHn. Yet, I got convinced that getting
and keeping highspeed modes stable means turning a lot of knobs
currently. This is not only true for HS400, but also HS200 and SDR50 and
SDR104.

This simple approach here allows us to have a stable base on which we
can evaluate the other BSP patches on top. Especially those which modify
the tuning procedure.

So, because this change does not prevent SDHn exposure in the future, I
suggest to go this way for now, so we can spend our (limited) manpower
on evaluating the tuning next.

Kind regards,

   Wolfram


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Nov 5, 2018 at 7:08 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> > Is there any chance this can start to bite us in the future?
>
> Well, there is always a chance but to the best of our current knowledge,
> we can't see it for Gen3. And even then, we can still fix it.
>
> I was entering SDHI hackfest with the attitude of representing the
> hardware which means exposing SDHn. Yet, I got convinced that getting
> and keeping highspeed modes stable means turning a lot of knobs
> currently. This is not only true for HS400, but also HS200 and SDR50 and
> SDR104.
>
> This simple approach here allows us to have a stable base on which we
> can evaluate the other BSP patches on top. Especially those which modify
> the tuning procedure.
>
> So, because this change does not prevent SDHn exposure in the future, I
> suggest to go this way for now, so we can spend our (limited) manpower
> on evaluating the tuning next.

OK. Thanks!

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 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2018-11-05 16:45:39 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Mon, Nov 5, 2018 at 4:07 PM Niklas S�derlund
> <niklas.soderlund@ragnatech.se> wrote:
> > On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> > > On Thu, Nov 1, 2018 at 12:26 AM Niklas S�derlund
> > > <niklas.soderlund@ragnatech.se> wrote:
> > > > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > >
> > > > On H3 (ES1.0,ES2.0) and M3-W (ES1.0,ES1.1) the clock setting for HS400
> > >
> > > H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1)
> >
> > Thanks.
> >
> > >
> > > > needs a quirk to function properly. The reason for the quirk is that
> > > > there are two settings which produces same divider vale for the SDn
> > > > clock. On the effected boards the one currently selected results in HS00
> > >
> > > HS200 or HS400?
> >
> > Wops, 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>
> > > > ---
> > > >  drivers/clk/renesas/rcar-gen3-cpg.c | 28 ++++++++++++++++++++++------
> > > >  1 file changed, 22 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> > > > index ff56ac6134111c38..8cc524a29c855dd2 100644
> > > > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > > > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> 
> > > > @@ -377,6 +382,7 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
> > > >         clock->hw.init = &init;
> > > >         clock->div_table = cpg_sd_div_table;
> > > >         clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> > > > +       clock->skip_first = skip_first;
> > >
> > > What about
> > >
> > >     if (cpg_quirks & SD_SKIP_FIRST) {
> > >             clock->div_table++;
> > >             clock->div_num--;
> > >     }
> > >
> > > instead?
> >
> > This was my first approach as well, unfortunately it won't work :-(
> >
> > If the bootloader leaves the clock settings in a state which matches the
> > first row in the table the clock fails to register as the check in
> > cpg_sd_clk_register() makes sure it have a row for the state the
> > hardware is in. If it can't find that state the clock fails to register.
> > Whit this quirk the limitation of the effected boards only have an
> > effect when setting the clock.
> 
> IC...
> 
> > I thought this solution solved this quiet neatly with the robustness
> > principle, 'Be conservative in what you do, be liberal in what you
> > accept from others' :-)
> 
> Will it ever use the settings as inherited from boot loader or reset state?
> If it does, I assume it will fail badly, due to an inconsistency between the
> SD and SDH clock rates?
> And what if the kernel is ever booted when the SDnSRCFC or SDnFC field
> has an invalid value? Then the driver will fail, too?
> 
> Hence, isn't it safer to initialize the registers to a known safe value?

I agree with you that we should not trust the value fro the bootloader 
and the sdhi driver do not trust this and resets it. The problem is 
rcar-gen3-cpg. From cpg_sd_clk_register()

         sd_fc = readl(clock->csn.reg) & CPG_SD_FC_MASK;
         for (i = 0; i < clock->div_num; i++)
                 if (sd_fc == (clock->div_table[i].val & CPG_SD_FC_MASK))
                         break;

         if (WARN_ON(i >= clock->div_num)) {
                 kfree(clock);
                 return ERR_PTR(-EINVAL);
         }

         clock->cur_div_idx = i;

If the bootloader sets the register to a value not known by the clock 
driver or if we us the moth you prefer to modify clock->div_table and 
clock->div_num the clock driver would need to set a safe default. I 
think this would be fine as the SDHI driver could handle this (I need to 
test it tho). My proposal is therefor that I add a patch to this series 
where the clock driver try to set a known divider when it registers the 
clock.

My suggestion would be that the divider to be set to 4 as this appears 
to be what current boot loaders do. Would this work for you?


> 
> 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

-- 
Regards,
Niklas S�derlund

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

[Niklas Söderlund]

Hi Geert,

On 2018-11-28 19:02:33 +0100, Niklas S�derlund wrote:
> Hi Geert,
> 
> Thanks for your feedback.
> 
> On 2018-11-05 16:45:39 +0100, Geert Uytterhoeven wrote:
> > Hi Niklas,
> > 
> > On Mon, Nov 5, 2018 at 4:07 PM Niklas S�derlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > > On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> > > > On Thu, Nov 1, 2018 at 12:26 AM Niklas S�derlund
> > > > <niklas.soderlund@ragnatech.se> wrote:
> > > > > From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > > > >
> > > > > On H3 (ES1.0,ES2.0) and M3-W (ES1.0,ES1.1) the clock setting for HS400
> > > >
> > > > H3 (ES1.x, ES2.0) and M3-W (ES1.0, ES1.1)
> > >
> > > Thanks.
> > >
> > > >
> > > > > needs a quirk to function properly. The reason for the quirk is that
> > > > > there are two settings which produces same divider vale for the SDn
> > > > > clock. On the effected boards the one currently selected results in HS00
> > > >
> > > > HS200 or HS400?
> > >
> > > Wops, 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>
> > > > > ---
> > > > >  drivers/clk/renesas/rcar-gen3-cpg.c | 28 ++++++++++++++++++++++------
> > > > >  1 file changed, 22 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> > > > > index ff56ac6134111c38..8cc524a29c855dd2 100644
> > > > > --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> > > > > +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> > 
> > > > > @@ -377,6 +382,7 @@ static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
> > > > >         clock->hw.init = &init;
> > > > >         clock->div_table = cpg_sd_div_table;
> > > > >         clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> > > > > +       clock->skip_first = skip_first;
> > > >
> > > > What about
> > > >
> > > >     if (cpg_quirks & SD_SKIP_FIRST) {
> > > >             clock->div_table++;
> > > >             clock->div_num--;
> > > >     }
> > > >
> > > > instead?
> > >
> > > This was my first approach as well, unfortunately it won't work :-(
> > >
> > > If the bootloader leaves the clock settings in a state which matches the
> > > first row in the table the clock fails to register as the check in
> > > cpg_sd_clk_register() makes sure it have a row for the state the
> > > hardware is in. If it can't find that state the clock fails to register.
> > > Whit this quirk the limitation of the effected boards only have an
> > > effect when setting the clock.
> > 
> > IC...
> > 
> > > I thought this solution solved this quiet neatly with the robustness
> > > principle, 'Be conservative in what you do, be liberal in what you
> > > accept from others' :-)
> > 
> > Will it ever use the settings as inherited from boot loader or reset state?
> > If it does, I assume it will fail badly, due to an inconsistency between the
> > SD and SDH clock rates?
> > And what if the kernel is ever booted when the SDnSRCFC or SDnFC field
> > has an invalid value? Then the driver will fail, too?
> > 
> > Hence, isn't it safer to initialize the registers to a known safe value?
> 
> I agree with you that we should not trust the value fro the bootloader 
> and the sdhi driver do not trust this and resets it. The problem is 
> rcar-gen3-cpg. From cpg_sd_clk_register()
> 
>          sd_fc = readl(clock->csn.reg) & CPG_SD_FC_MASK;
>          for (i = 0; i < clock->div_num; i++)
>                  if (sd_fc == (clock->div_table[i].val & CPG_SD_FC_MASK))
>                          break;
> 
>          if (WARN_ON(i >= clock->div_num)) {
>                  kfree(clock);
>                  return ERR_PTR(-EINVAL);
>          }
> 
>          clock->cur_div_idx = i;
> 
> If the bootloader sets the register to a value not known by the clock 
> driver or if we us the moth you prefer to modify clock->div_table and 
> clock->div_num the clock driver would need to set a safe default. I 
> think this would be fine as the SDHI driver could handle this (I need to 
> test it tho). My proposal is therefor that I add a patch to this series 
> where the clock driver try to set a known divider when it registers the 
> clock.
> 
> My suggestion would be that the divider to be set to 4 as this appears 
> to be what current boot loaders do. Would this work for you?

I have now created a patch which do almost this [1]. But instead of 
setting the divider to 4 just use the first row of clock->div_table and 
make sure the clock is stopped. Any user of the clock needs to set the 
rate it expects before enabling the clock. I tested this on H3 ES1 and 
ES2 and M3-N and it works just fine.

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

-- 
Regards,
Niklas S�derlund