[PATCH v11 01/10] clk: Add RISC-V Canaan Kendryte K210 clock driver

[Damien Le Moal]

On Wed, 2021-01-13 at 17:47 -0800, Stephen Boyd wrote:
> Quoting Damien Le Moal (2021-01-13 17:12:06)
> > On Wed, 2021-01-13 at 12:52 -0800, Stephen Boyd wrote:
> > > Quoting Damien Le Moal (2021-01-13 03:29:36)
> > > > On Wed, 2021-01-13 at 00:25 -0800, Stephen Boyd wrote:
> > > > > Quoting Damien Le Moal (2021-01-11 17:02:38)
> > > > > > +
> > > > > > +       /* Get the system base fixed-rate 26MHz oscillator clock */
> > > > > > +       in0 = of_clk_get(np, 0);
> > > > > > +       if (IS_ERR(in0)) {
> > > > > > +               pr_err("%pOFP: failed to get base fixed-rate oscillator\n", np);
> > > > > > +               return;
> > > > > > +       }
> > > > > > +       kcl->parents[K210_PARENT_IN0] = __clk_get_hw(in0);
> > > > > 
> > > > > This shouldn't be necessary. Specify the DT node index in the
> > > > > parent_data structure instead.
> > > > 
> > > > OK. I understood what you mean here. However, IN0 is not created by this
> > > > driver. It has its own fixed-oscillator DT node that creates the clock. So a
> > > > different provider. Hence I think we cannot use an index into clk_data->hws,
> > > > unless I am not understanding something.
> > > > 
> > > 
> > > Can you use clk_parent_data::index?
> > 
> > Not for the base oscillator "in0" as that clock is created through the device
> > tree (it has its own fixed-clock node) and so is not provided by this driver. 
> > 
> > But I can use parent_data::hw. That works.
> > So I ended up rewriting all parents setup using the clk_parent_data struct to
> > define all clocks parents using the parent_data::hw field. That removes the
> > need for the parents array that I had.
> 
> The driver should not be calling of_clk_get(). It should register clks
> with a device node or a device that has a DT binding with a clocks
> property and then the index and/or fw_name can indicate what that fixed
> clock node is.
> 
> In DT:
> 
> 	this_files_associated_device_node {
> 		clocks = <&phandle_to_fixed_clock_node>;
> 	};
> 
> And then clk_parent_data::index == 0 to indicate that it should use that
> clk. If the clk_hw is registered by this driver then using clk_hw is
> appropriate, but otherwise we shouldn't need to get the hw pointer for
> any clk that is created outside of this file.

I cannot get this to work. The sysclk node has "clocks = <&in0>;" has the sole
input/parent clock. But using clk_parent_data::index = 0 to specify in0 as a
parent clock for the clocks registered by the k210-clk driver does not work (no
boot). However, if I use

clk_parent_data::name =  of_clk_get_parent_name(np, 0)

where np is the device node for the sysclk driver, things work.

Looking at clk_core_fill_parent_index() and clk_core_get(), using index should
work. Not sure what I am doing wrong here. Digging...



-- 
Damien Le Moal
Western Digital