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

Damien Le Moal
2021-02-10T03:15:57Z

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