Re: [PATCH v18 01/16] clk: Add RISC-V Canaan Kendryte K210 clock driver

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/02/10 10:24, Stephen Boyd wrote:
> Quoting Damien Le Moal (2021-02-09 04:29:59)
>> 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. 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@kernel.org>
>> Cc: Michael Turquette <mturquette@baylibre.com>
>> Cc: linux-clk@vger.kernel.org
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> Reviewed-by: Anup Patel <anup@brainfault.org>
>> ---
> 
> My inbox has this one patch multiple times and no changelog here. I also
> don't see the cover letter, so I guess I wasn't Cced? Can you please add
> a changelog so I know if anything has changed?

My apologies about this. I forgot to CC you on the cover letter. Will do for the
next version.

[...]
>> +static const struct clk_ops k210_clk_ops = {
>> +       .enable         = k210_clk_enable,
>> +       .disable        = k210_clk_disable,
>> +       .recalc_rate    = k210_clk_get_rate,
>> +};
> 
> From here...
> 
>> +
>> +static void k210_register_clk(struct k210_sysclk *ksc, int id,
>> +                             const struct clk_parent_data *parent_data,
>> +                             int num_parents, unsigned long flags)
>> +{
>> +       struct k210_clk *kclk = &ksc->clks[id];
>> +       struct clk_init_data init = {};
>> +       int ret;
>> +
>> +       init.name = k210_clk_cfgs[id].name;
>> +       init.flags = flags;
>> +       init.parent_data = parent_data;
>> +       init.num_parents = num_parents;
>> +       if (num_parents > 1)
>> +               init.ops = &k210_clk_mux_ops;
>> +       else
>> +               init.ops = &k210_clk_ops;
>> +
>> +       kclk->id = id;
>> +       kclk->ksc = ksc;
>> +       kclk->hw.init = &init;
>> +
>> +       ret = clk_hw_register(NULL, &kclk->hw);
>> +       if (ret) {
>> +               pr_err("%pOFP: register clock %s failed\n",
>> +                      ksc->node, k210_clk_cfgs[id].name);
>> +               kclk->id = -1;
>> +       }
>> +}
>> +
>> +/*
>> + * All muxed clocks have IN0 and PLL0 as parents.
>> + */
>> +static inline void k210_register_mux_clk(struct k210_sysclk *ksc,
>> +                                        const char *in0, int id)
>> +{
>> +       const struct clk_parent_data parent_data[2] = {
>> +               { .name = in0 },
>> +               { .hw = &ksc->plls[K210_PLL0].hw },
>> +       };
>> +
>> +       k210_register_clk(ksc, id, parent_data, 2, 0);
>> +}
>> +
>> +static inline void k210_register_in0_child(struct k210_sysclk *ksc,
>> +                                          const char *in0, int id)
>> +{
>> +       const struct clk_parent_data parent_data = {
>> +               .name = in0,
>> +       };
>> +
>> +       k210_register_clk(ksc, id, &parent_data, 1, 0);
>> +}
>> +
>> +static inline void k210_register_pll_child(struct k210_sysclk *ksc, int id,
>> +                                          enum k210_pll_id pllid,
>> +                                          unsigned long flags)
>> +{
>> +       const struct clk_parent_data parent_data = {
>> +               .hw = &ksc->plls[pllid].hw,
>> +       };
>> +
>> +       k210_register_clk(ksc, id, &parent_data, 1, flags);
>> +}
>> +
>> +static inline void k210_register_aclk_child(struct k210_sysclk *ksc, int id,
>> +                                           unsigned long flags)
>> +{
>> +       const struct clk_parent_data parent_data = {
>> +               .hw = &ksc->aclk,
>> +       };
>> +
>> +       k210_register_clk(ksc, id, &parent_data, 1, flags);
>> +}
>> +
>> +static inline void k210_register_clk_child(struct k210_sysclk *ksc, int id,
>> +                                          int parent_id)
>> +{
>> +       const struct clk_parent_data parent_data = {
>> +               .hw = &ksc->clks[parent_id].hw,
>> +       };
>> +
>> +       k210_register_clk(ksc, id, &parent_data, 1, 0);
>> +}
> 
> .. to here, shouldn't these all gain __init?

Yes indeed, they can. Fixed that.

>> +
>> +static struct clk_hw *k210_clk_hw_onecell_get(struct of_phandle_args *clkspec,
>> +                                             void *data)
>> +{
>> +       struct k210_sysclk *ksc = data;
>> +       unsigned int idx = clkspec->args[0];
>> +
>> +       if (idx >= K210_NUM_CLKS)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       return &ksc->clks[idx].hw;
>> +}
>> +
>> +static void __init k210_clk_init(struct device_node *np)
>> +{
>> +       struct device_node *sysctl_np;
>> +       struct k210_sysclk *ksc;
>> +       const char *in0;
>> +       int i, ret;
>> +
>> +       ksc = kzalloc(sizeof(*ksc), GFP_KERNEL);
>> +       if (!ksc)
>> +               return;
>> +
>> +       ksc->node = np;
>> +       spin_lock_init(&ksc->clk_lock);
>> +       sysctl_np = of_get_parent(np);
>> +       ksc->regs = of_iomap(sysctl_np, 0);
>> +       of_node_put(sysctl_np);
>> +       if (!ksc->regs) {
>> +               pr_err("%pOFP: failed to map registers\n", np);
>> +               return;
>> +       }
>> +
>> +       in0 = of_clk_get_parent_name(np, 0);
> 
> I'm still lost why we need to get the clk parent name here vs. using the
> index approach and using clk_parent_data. There were some comments about
> #clock-cells which didn't make sense to me. The fixed rate clk in DT
> should have #clock-cells as it is a clk.

It is like this because I could not get your suggested approach to work. I am
using clk_parent_data[]->hw for specifying the parents of the clocks registered
by this driver. But using this data structure, I failed to figure out how to
specify the "in0" clock as a parent without using the clock name itself. The
other option I see is using fw_name (I do not understand that one) and hw, but I
do not have that pointer since the clock is registered by the common framework.
What am I missing here ?

>> +       if (!in0) {
>> +               pr_err("%pOFP: undefined fixed-rate oscillator clock\n", np);
>> +               return;
>> +       }
>> +
>> +       ret = k210_register_plls(ksc, in0);
> 
> I suspect the 'np' should be passed to these functions instead, and then
> use of_clk_hw_register().

Done.
I also addressed all your nits. Thanks for the comments.


-- 
Damien Le Moal
Western Digital Research

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv