[PATCH v18 01/16] clk: Add RISC-V Canaan Kendryte K210 clock driver
Damien Le Moal
Damien.LeMoal at wdc.com
Tue Feb 9 23:32:34 EST 2021
On 2021/02/10 12:15, Damien Le Moal wrote:
> 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 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>
>>> Reviewed-by: Anup Patel <anup at 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 ?
Revisiting this, I think I got it. All I need to to is set ".index = 0" in
clk_parent_data[] of the clocks that have in0 as a parent. Very simple indeed. I
removed the ino clock name stuff.
Testing and sending out v19.
Thanks !
>
>>> + 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
More information about the linux-riscv
mailing list