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@xxxxxxxxxxxx> wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx
>
> 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
