Re: [PATCH] Revert "clk: fix __clk_init_parent() for single parent clocks"

[Jerome Brunet]

On Tue, 2018-12-04 at 10:05 -0800, Stephen Boyd wrote:
> Quoting Jerome Brunet (2018-12-04 08:32:57)
> > This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.
> > 
> > From the original commit message:
> >   It turned out a problem because there are some single-parent clocks
> >   that implement .get_parent() callback and return non-zero index.
> >   The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.
> > 
> > It is wrong for a clock to return an index >= num_parents. CCF checks
> > for this condition in clk_core_get_parent_by_index(). This function sets
> > the parent to NULL if index is incoherent, which seems like the only
> > sane choice.
> > 
> > commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent
> > clocks")
> > appears to be a work around installed in the core framework for a problem
> > that is platform specific, and should probably be fixed in the platform
> > code.
> 
> Ouch. I see that I even pointed that out in 2016 but never got a reply
> or a fix patch[1].
> 
> > Further more, it introduces a problem in a corner case of the mux clock.
> > Take mux with multiple parents, but only one is known, the rest being
> > undocumented. The register reset has one of unknown parent set.
> > 
> > Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single
> > parent clocks"):
> >  * get_parent() is called, register is read and give an unknown index.
> >    -> the mux is orphaned.
> >  * a call to set_rate() will reparent the mux to the only known parent.
> > 
> > With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent
> > clocks"):
> >  * the register is never read.
> >  * parent is wrongly assumed to be the only known one.
> >    As a consequence, all the calculation deriving from the mux will be
> >    wrong
> >  * Since we believe the only know parent to be set, set_parent() won't
> >    ever be called and the register won't be set with the correct value.
> 
> Isn't this the broken bad case all over again? Why register a clk as a
> mux and then only say it has one parent?

I understand it is a bit odd but as I explained it is a corner case.

We are really trying to drive a mux here, applying a values will change the
clock signal we get. Documentation being what it is, we only know one the
parent. The other parent could anything or maybe not connected at all, who
know. That is not the important part actually

If such mux was already set to the known entry by default, it would be OK to
ignore it. But if it is not, then we CCF to realise that and change the
setting accordingly.

This the case of the 'ao_cts_cec' clock in the following patch:
https://lore.kernel.org/patchwork/patch/1021028/

by default the value in the register is 0, but the only one that makes sense
for us is 1.

> 
> > Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> > ---
> 
> Is this related to the other patch you sent? Can you send series for
> related patches please?
> 
> [1] https://lkml.kernel.org/r/20160209181833.GA24167@xxxxxxxxxxxxxx

Actually I was intially doing a series, and stopped when my cover letter
started with "those are two unrelated patches ..." ;)

I found these things while debugging the same thing but there is no deps
between them.