RE: [PATCH v2 2/3] drivers: clk: Add support for versa3 clock driver

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Stephen,

Thanks for the feedback.

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Tuesday, March 21, 2023 11:16 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>; Michael Turquette
> <mturquette@baylibre.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; linux-clk@vger.kernel.org; Geert
> Uytterhoeven <geert+renesas@glider.be>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v2 2/3] drivers: clk: Add support for versa3 clock
> driver
> 
> Quoting Biju Das (2023-03-09 08:55:28)
> > diff --git a/drivers/clk/clk-versaclock3.c
> > b/drivers/clk/clk-versaclock3.c new file mode 100644 index
> > 000000000000..6c5c8b37f6af
> > --- /dev/null
> > +++ b/drivers/clk/clk-versaclock3.c
> > @@ -0,0 +1,1139 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Renesas Versaclock 3
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + */
> > +
> [...]
> > +       [vc3_se1] = "se1",
> > +       [vc3_ref] = "ref"
> > +};
> > +
> > +static const struct clk_parent_data pfdmux_p[] = {
> > +       { .fw_name = vc3_fin_name, .name = vc3_fin_name },
> 
> New drivers should only have .fw_name here. I don't think you're migrating
> an existing driver to clk_parent_data so .name should be removed. And then
> maybe you'll want to simply use the index instead so that we don't have to
> do any string comparisons to find clk parents.

+ Rob and Krzysztof Kozlowski

Agreed. But it requires the below change in device tree. 
+		clock-names = "xtal";

Otherwise with just[1], devm_clk_hw_register is not assigning xtal as
Parent clock.

[1]
static const struct clk_parent_data pfdmux_p[] = {
{ .fw_name = "xtal"},
..
}

So I will update the bindings.


> 
> > +       { .fw_name = "div2", .name = "div2" } };
> > +
> [...]
> > +
> > +static unsigned long vc3_clk_out_recalc_rate(struct clk_hw *hw,
> > +                                            unsigned long
> > +parent_rate) {
> > +       return parent_rate;
> > +}
> > +
> > +static long vc3_clk_out_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                                  unsigned long *prate) {
> > +       *prate = clk_hw_round_rate(clk_hw_get_parent(hw), rate);
> > +
> > +       return *prate;
> > +}
> > +
> > +static int vc3_clk_out_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                               unsigned long parent_rate) {
> > +       /*
> > +        * We must report success. round_rate() propagates rate to the
> > +        * parent and based on the rate mux changes its parent.
> > +        */
> > +
> > +       return 0;
> > +}
> > +
> > +const struct clk_ops vc3_clk_out_ops = {
> > +       .recalc_rate = vc3_clk_out_recalc_rate,
> > +       .round_rate = vc3_clk_out_round_rate,
> > +       .set_rate = vc3_clk_out_set_rate, };
> 
> Are any of these clk ops necessary? They don't do anything besides pass up
> to the parent, so you can set CLK_SET_RATE_PARENT and be done?


Yes it is required, otherwise, I get Warn message [2] and it fails to register the clk hw.
[2] https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L4120


> 
> > +
> > +static bool vc3_regmap_is_writeable(struct device *dev, unsigned int
> > +reg) {
> > +       return true;
> > +}
> > +
> > +static const struct regmap_config vc3_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +       .cache_type = REGCACHE_RBTREE,
> > +       .max_register = 0x24,
> > +       .writeable_reg = vc3_regmap_is_writeable, };
> > +
> > +static struct clk_hw *vc3_of_clk_get(struct of_phandle_args *clkspec,
> > +                                    void *data) {
> > +       struct vc3_driver_data *vc3 = data;
> > +       unsigned int idx = clkspec->args[0];
> > +
> > +       if (idx >= ARRAY_SIZE(clk_out_data)) {
> > +               pr_err("invalid clk index %u for provider %pOF\n", idx,
> clkspec->np);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       return &vc3->clk_out[idx].hw;
> > +}
> > +
> > +static void vc3_divider_type_parse_dt(struct device *dev,
> > +                                     struct vc3_driver_data *vc3) {
> > +       struct device_node *np = dev->of_node;
> > +       struct property *prop;
> > +       const __be32 *p;
> > +       u32 i = 0;
> > +       u32 val;
> > +
> > +       of_property_for_each_u32(np, "assigned-clock-rates",
> 
> This is an interesting use of assigned-clock-rates.
> 
> > +                                prop, p, val) {
> > +               if (i >= ARRAY_SIZE(div_data))
> > +                       break;
> > +               if (val)
> > +                       vc3->clk_div[i].flags = CLK_DIVIDER_READ_ONLY;
> 
> Why would assigned clock rates change the read only flag?

I do not want to change the divider settings, so I thought of using
these properties to assign the read only flag to clock-divider flag.

I will remove this function in next version and Will use CLK_DIVIDER_READ_ONLY
flags for dividers.

Cheers,
Biju