Hi Stephen,
Thanks for the feedback.
> -----Original Message-----
> From: Stephen Boyd <sboyd@xxxxxxxxxx>
> Sent: Tuesday, March 21, 2023 11:16 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Michael Turquette
> <mturquette@xxxxxxxxxxxx>
> Cc: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx; Geert
> Uytterhoeven <geert+renesas@xxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> 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
