Re: [PATCH v3 02/25] clk: versaclock3: Prepare for the addition of 5L35023 device

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Claudiu,

On Mon, Dec 9, 2024 at 1:22 PM Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> On 09.12.2024 14:16, Geert Uytterhoeven wrote:
> > On Mon, Dec 9, 2024 at 12:14 PM Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> >> On 09.12.2024 12:57, Geert Uytterhoeven wrote:
> >>> On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>>>
> >>>> The 5P35023 and 5L35035 Versa 3 clock generator variants are different but
> >>>> the versaclock3 driver could be used with small adjustments. The features
> >>>> that are implemented in driver and differs b/w variants are the PLL2 Fvco
> >>>> and clock sel bit for SE2 clock. Adjust the driver to prepare for the
> >>>> addition of 5L35023 device.
> >>>>
> >>>> Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>>
> >>> Thanks for your patch!
> >>>
> >>>> --- a/drivers/clk/clk-versaclock3.c
> >>>> +++ b/drivers/clk/clk-versaclock3.c
> >>>> @@ -166,12 +167,17 @@ struct vc3_div_data {
> >>>>  struct vc3_hw_data {
> >>>>         struct clk_hw hw;
> >>>>         struct regmap *regmap;
> >>>> -       const void *data;
> >>>> +       void *data;
> >>>>
> >>>>         u32 div_int;
> >>>>         u32 div_frc;
> >>>>  };
> >>>
> >>>> @@ -698,8 +706,6 @@ static struct vc3_hw_data clk_pll[] = {
> >>>>                         .num = VC3_PLL2,
> >>>>                         .int_div_msb_offs = VC3_PLL2_FB_INT_DIV_MSB,
> >>>>                         .int_div_lsb_offs = VC3_PLL2_FB_INT_DIV_LSB,
> >>>> -                       .vco_min = VC3_PLL2_VCO_MIN,
> >>>> -                       .vco_max = VC3_PLL2_VCO_MAX
> >>>>                 },
> >>>>                 .hw.init = &(struct clk_init_data) {
> >>>>                         .name = "pll2",
> >>>
> >>>> @@ -1029,9 +1037,16 @@ static int vc3_probe(struct i2c_client *client)
> >>>>                                              clk_pfd[i].hw.init->name);
> >>>>         }
> >>>>
> >>>> +       data = i2c_get_match_data(client);
> >>>> +
> >>>>         /* Register pll's */
> >>>>         for (i = 0; i < ARRAY_SIZE(clk_pll); i++) {
> >>>>                 clk_pll[i].regmap = regmap;
> >>>> +               if (i == VC3_PLL2) {
> >>>> +                       struct vc3_pll_data *pll_data = clk_pll[i].data;
> >>>> +
> >>>> +                       pll_data->vco = data->pll2_vco;
> >>>
> >>> You cannot modify the global clk_pll[] data, as it is shared when
> >>> there are multiple instances.
> >>
> >> By "multiple instances" do you mean, multiple versa3 devices using this
> >> driver? Do you know if we have such a board integrated, ATM?
> >
> > Exactly.
> > Currently there are no such (upstream) users for 5p35023,
> > but e.g. the Beacon RZ/G2M kit has two 5p49v6965 instances.
>
> Are you OK with keeping it as is for the RZ/G3S SSIF support and returning
> back later with a solution for the scenario you pointed out? Although, ATM,
> I don't have a board to test it.

Fine for me, as multiple instances were already broken before.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds