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