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

[Damien Le Moal]

On Thu, 2021-01-14 at 12:15 +0900, Damien Le Moal wrote:
> 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...

After more code reading, I think that the clk_parent_data::index method for
specifying a clock parent works only for nodes that do not have a #clock-cells
property. Since this driver has one, it looks like the clk_parent_data::name
for the external in0 clock and clk_parent_data::hw for internally registered
clocks would be the best method to specify parents. Can you confirm please ?



-- 
Damien Le Moal
Western Digital