* [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock
@ 2018-10-31 23:25 Niklas Söderlund
2018-10-31 23:25 ` [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks Niklas Söderlund
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Niklas Söderlund @ 2018-10-31 23:25 UTC (permalink / raw)
To: Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc; +Cc: 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks
2018-10-31 23:25 [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
@ 2018-10-31 23:25 ` Niklas Söderlund
2018-11-01 19:37 ` Wolfram Sang
2018-10-31 23:25 ` [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Niklas Söderlund @ 2018-10-31 23:25 UTC (permalink / raw)
To: Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-10-31 23:25 [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
2018-10-31 23:25 ` [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks Niklas Söderlund
@ 2018-10-31 23:25 ` Niklas Söderlund
2018-11-01 19:38 ` Wolfram Sang
` (2 more replies)
2018-11-01 19:43 ` [PATCH 0/2] " Wolfram Sang
2018-11-05 10:32 ` Geert Uytterhoeven
3 siblings, 3 replies; 17+ messages in thread
From: Niklas Söderlund @ 2018-10-31 23:25 UTC (permalink / raw)
To: Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks
2018-10-31 23:25 ` [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks Niklas Söderlund
@ 2018-11-01 19:37 ` Wolfram Sang
2018-11-05 10:28 ` Geert Uytterhoeven
0 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2018-11-01 19:37 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc,
Niklas Söderlund
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-10-31 23:25 ` [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
@ 2018-11-01 19:38 ` Wolfram Sang
2018-11-02 16:54 ` Sergei Shtylyov
2018-11-05 10:43 ` Geert Uytterhoeven
2 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-11-01 19:38 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc,
Niklas Söderlund
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock
2018-10-31 23:25 [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
2018-10-31 23:25 ` [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks Niklas Söderlund
2018-10-31 23:25 ` [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
@ 2018-11-01 19:43 ` Wolfram Sang
2018-11-05 10:32 ` Geert Uytterhoeven
3 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-11-01 19:43 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Geert Uytterhoeven, Wolfram Sang, linux-renesas-soc,
Niklas Söderlund
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-10-31 23:25 ` [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
2018-11-01 19:38 ` Wolfram Sang
@ 2018-11-02 16:54 ` Sergei Shtylyov
2018-11-05 10:43 ` Geert Uytterhoeven
2 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2018-11-02 16:54 UTC (permalink / raw)
To: Niklas Söderlund, Geert Uytterhoeven, Wolfram Sang,
linux-renesas-soc
Cc: Niklas Söderlund
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks
2018-11-01 19:37 ` Wolfram Sang
@ 2018-11-05 10:28 ` Geert Uytterhoeven
0 siblings, 0 replies; 17+ messages in thread
From: Geert Uytterhoeven @ 2018-11-05 10:28 UTC (permalink / raw)
To: Wolfram Sang
Cc: Niklas Söderlund, Geert Uytterhoeven, Wolfram Sang,
Linux-Renesas, Niklas Söderlund
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock
2018-10-31 23:25 [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
` (2 preceding siblings ...)
2018-11-01 19:43 ` [PATCH 0/2] " Wolfram Sang
@ 2018-11-05 10:32 ` Geert Uytterhoeven
2018-11-05 14:58 ` Niklas Söderlund
2018-11-05 18:08 ` Wolfram Sang
3 siblings, 2 replies; 17+ messages in thread
From: Geert Uytterhoeven @ 2018-11-05 10:32 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas, Niklas Söderlund
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-10-31 23:25 ` [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
2018-11-01 19:38 ` Wolfram Sang
2018-11-02 16:54 ` Sergei Shtylyov
@ 2018-11-05 10:43 ` Geert Uytterhoeven
2018-11-05 15:07 ` Niklas Söderlund
2 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2018-11-05 10:43 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas, Niklas Söderlund
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-05 10:32 ` Geert Uytterhoeven
@ 2018-11-05 14:58 ` Niklas Söderlund
2018-11-05 18:08 ` Wolfram Sang
1 sibling, 0 replies; 17+ messages in thread
From: Niklas Söderlund @ 2018-11-05 14:58 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-05 10:43 ` Geert Uytterhoeven
@ 2018-11-05 15:07 ` Niklas Söderlund
2018-11-05 15:45 ` Geert Uytterhoeven
0 siblings, 1 reply; 17+ messages in thread
From: Niklas Söderlund @ 2018-11-05 15:07 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-05 15:07 ` Niklas Söderlund
@ 2018-11-05 15:45 ` Geert Uytterhoeven
2018-11-28 18:02 ` Niklas Söderlund
0 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2018-11-05 15:45 UTC (permalink / raw)
To: Niklas Söderlund; +Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-05 10:32 ` Geert Uytterhoeven
2018-11-05 14:58 ` Niklas Söderlund
@ 2018-11-05 18:08 ` Wolfram Sang
2018-11-05 18:23 ` Geert Uytterhoeven
1 sibling, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2018-11-05 18:08 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Niklas Söderlund, Geert Uytterhoeven, Wolfram Sang,
Linux-Renesas, Niklas Söderlund
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-05 18:08 ` Wolfram Sang
@ 2018-11-05 18:23 ` Geert Uytterhoeven
0 siblings, 0 replies; 17+ messages in thread
From: Geert Uytterhoeven @ 2018-11-05 18:23 UTC (permalink / raw)
To: Wolfram Sang
Cc: Niklas Söderlund, Geert Uytterhoeven, Wolfram Sang,
Linux-Renesas, Niklas Söderlund
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-05 15:45 ` Geert Uytterhoeven
@ 2018-11-28 18:02 ` Niklas Söderlund
2018-11-29 0:43 ` Niklas Söderlund
0 siblings, 1 reply; 17+ messages in thread
From: Niklas Söderlund @ 2018-11-28 18:02 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock
2018-11-28 18:02 ` Niklas Söderlund
@ 2018-11-29 0:43 ` Niklas Söderlund
0 siblings, 0 replies; 17+ messages in thread
From: Niklas Söderlund @ 2018-11-29 0:43 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Geert Uytterhoeven, Wolfram Sang, Linux-Renesas
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-11-29 11:47 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-31 23:25 [PATCH 0/2] renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
2018-10-31 23:25 ` [PATCH 1/2] clk: renesas: rcar-gen3: add documentation for SD clocks Niklas Söderlund
2018-11-01 19:37 ` Wolfram Sang
2018-11-05 10:28 ` Geert Uytterhoeven
2018-10-31 23:25 ` [PATCH 2/2] clk: renesas: rcar-gen3: add HS400 quirk for SD clock Niklas Söderlund
2018-11-01 19:38 ` Wolfram Sang
2018-11-02 16:54 ` Sergei Shtylyov
2018-11-05 10:43 ` Geert Uytterhoeven
2018-11-05 15:07 ` Niklas Söderlund
2018-11-05 15:45 ` Geert Uytterhoeven
2018-11-28 18:02 ` Niklas Söderlund
2018-11-29 0:43 ` Niklas Söderlund
2018-11-01 19:43 ` [PATCH 0/2] " Wolfram Sang
2018-11-05 10:32 ` Geert Uytterhoeven
2018-11-05 14:58 ` Niklas Söderlund
2018-11-05 18:08 ` Wolfram Sang
2018-11-05 18:23 ` Geert Uytterhoeven
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.