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

[Damien Le Moal]

On Wed, 2021-01-13 at 00:25 -0800, Stephen Boyd wrote:
> Quoting Damien Le Moal (2021-01-11 17:02:38)
> > Add a clock provider driver for the Canaan Kendryte K210 RISC-V SoC.
> > This new driver with the compatible string "canaan,k210-clk" implements
> > support for the full clock structure of the K210 SoC. Since it is
> > required for the correct operation of the SoC, this driver is
> > selected by default for compilation when the SOC_CANAAN option is
> > selected.
> > 
> > With this change, the k210-sysctl driver is turned into a simple
> > platform driver which enables its power bus clock and triggers
> > populating its child nodes. This driver is also automatically selected
> > for compilation with the selection of SOC_CANAAN.
> 
> That's stated twice.
> 
> > The sysctl driver
> > retains the SOC early initialization code, but the implementation now
> > relies on the new function k210_clk_early_init() provided by the new
> > clk-k210 driver.
> > 
> > The clock structure implemented and many of the coding ideas for the
> > driver come from the work by Sean Anderson on the K210 support for the
> > U-Boot project.
> > 
> > Cc: Stephen Boyd <sboyd at kernel.org>
> > Cc: Michael Turquette <mturquette at baylibre.com>
> > Cc: linux-clk at vger.kernel.org
> > Signed-off-by: Damien Le Moal <damien.lemoal at wdc.com>
> > ---
> >  MAINTAINERS                      |    1 +
> >  drivers/clk/Kconfig              |    8 +
> >  drivers/clk/Makefile             |    1 +
> >  drivers/clk/clk-k210.c           | 1005 ++++++++++++++++++++++++++++++
> >  drivers/soc/canaan/Kconfig       |   18 +-
> >  drivers/soc/canaan/Makefile      |    2 +-
> >  drivers/soc/canaan/k210-sysctl.c |  205 ++----
> >  include/soc/canaan/k210-sysctl.h |    2 +
> >  8 files changed, 1064 insertions(+), 178 deletions(-)
> >  create mode 100644 drivers/clk/clk-k210.c

[...]
> > +static void __init k210_clk_init(struct device_node *np)
> > +{
> > +       struct device_node *sysctl_np;
> > +       struct clk_hw **hws;
> > +       struct clk *in0;
> > +       int i, ret;
> > +
> > +       kcl = kzalloc(sizeof(*kcl), GFP_KERNEL);
> > +       if (!kcl)
> > +               return;
> > +
> > +       kcl->clk_data = kzalloc(struct_size(kcl->clk_data, hws, K210_NUM_CLKS),
> > +                               GFP_KERNEL);
> > +       if (!kcl->clk_data)
> > +               return;
> > +
> > +       sysctl_np = of_get_parent(np);
> > +       kcl->regs = of_iomap(sysctl_np, 0);
> > +       of_node_put(sysctl_np);
> > +       if (!kcl->regs) {
> > +               pr_err("%pOFP: failed to map registers\n", np);
> > +               return;
> > +       }
> > +
> > +       spin_lock_init(&kcl->clk_lock);
> > +       kcl->clk_data->num = K210_NUM_CLKS;
> > +       hws = kcl->clk_data->hws;
> > +       for (i = 0; i < K210_NUM_CLKS; i++)
> > +               hws[i] = ERR_PTR(-EPROBE_DEFER);
> > +
> > +       /* 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.


-- 
Damien Le Moal
Western Digital