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

2018-11-28 Thread 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
> >  wrote:
> > > On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> > > > On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
> > > >  wrote:
> > > > > From: Niklas Söderlund 
> > > > >
> > > > > 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 
> > > > > 
> > > > > ---
> > > > >  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 = 
> > > > > 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


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

2018-11-28 Thread 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
>  wrote:
> > On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> > > On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
> > >  wrote:
> > > > From: Niklas Söderlund 
> > > >
> > > > 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 
> > > > ---
> > > >  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 = 
> > > > 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 -- 
> ge...@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

2018-11-05 Thread Geert Uytterhoeven
Hi Niklas,

On Mon, Nov 5, 2018 at 4:07 PM Niklas Söderlund
 wrote:
> On 2018-11-05 11:43:24 +0100, Geert Uytterhoeven wrote:
> > On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
> >  wrote:
> > > From: Niklas Söderlund 
> > >
> > > 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 
> > > ---
> > >  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 = 
> > > 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 -- ge...@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

2018-11-05 Thread 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
>  wrote:
> > From: Niklas Söderlund 
> >
> > 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 
> > ---
> >  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 = 
> > 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_CKSELBIT(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",
> > -  

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

2018-11-05 Thread Geert Uytterhoeven
Hi Niklas,

Thanks for your patch!

On Thu, Nov 1, 2018 at 12:26 AM Niklas Söderlund
 wrote:
> From: Niklas Söderlund 
>
> 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 
> ---
>  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 = 
> 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_CKSELBIT(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),
> -  

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

2018-11-02 Thread Sergei Shtylyov
Hello!

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

> From: Niklas Söderlund 
> 
> 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 
[...]

MBR, Sergei


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

2018-11-01 Thread Wolfram Sang
On Thu, Nov 01, 2018 at 12:25:18AM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund 
> 
> 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 

Reviewed-by: Wolfram Sang 
Tested-by: Wolfram Sang 



signature.asc
Description: PGP signature