| Message ID | 20241113133540.2005850-3-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add audio support for the Renesas RZ/G3S SoC | expand |
Hi Claudiu, On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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@bp.renesas.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> 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. Oops, the driver already did this before, as the .regmap, .div_int, and .div_frc fields are modified during operation. > + } > ret = devm_clk_hw_register(dev, &clk_pll[i].hw); > if (ret) > return dev_err_probe(dev, ret, "%s failed\n", > @@ -1059,6 +1074,11 @@ static int vc3_probe(struct i2c_client *client) > /* Register clk muxes */ > for (i = 0; i < ARRAY_SIZE(clk_mux); i++) { > clk_mux[i].regmap = regmap; > + if (i == VC3_SE2_MUX) { > + struct vc3_clk_data *clk_data = clk_mux[i].data; > + > + clk_data->bitmsk = data->se2_clk_sel_msk; > + } Likewise. > ret = devm_clk_hw_register(dev, &clk_mux[i].hw); > if (ret) > return dev_err_probe(dev, ret, "%s failed\n", Gr{oetje,eeting}s, Geert
Hi, Geert, On 09.12.2024 12:57, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> 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@bp.renesas.com> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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? Thank you, Claudiu > Oops, the driver already did this before, as the .regmap, .div_int, > and .div_frc fields are modified during operation. > >> + } >> ret = devm_clk_hw_register(dev, &clk_pll[i].hw); >> if (ret) >> return dev_err_probe(dev, ret, "%s failed\n", >> @@ -1059,6 +1074,11 @@ static int vc3_probe(struct i2c_client *client) >> /* Register clk muxes */ >> for (i = 0; i < ARRAY_SIZE(clk_mux); i++) { >> clk_mux[i].regmap = regmap; >> + if (i == VC3_SE2_MUX) { >> + struct vc3_clk_data *clk_data = clk_mux[i].data; >> + >> + clk_data->bitmsk = data->se2_clk_sel_msk; >> + } > > Likewise. > >> ret = devm_clk_hw_register(dev, &clk_mux[i].hw); >> if (ret) >> return dev_err_probe(dev, ret, "%s failed\n", > > Gr{oetje,eeting}s, > > Geert >
Hi CLaudiu, On Mon, Dec 9, 2024 at 12:14 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > On 09.12.2024 12:57, Geert Uytterhoeven wrote: > > On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> 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@bp.renesas.com> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > 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. Gr{oetje,eeting}s, Geert
Hi, Geert, On 09.12.2024 14:16, Geert Uytterhoeven wrote: > Hi CLaudiu, > > On Mon, Dec 9, 2024 at 12:14 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: >> On 09.12.2024 12:57, Geert Uytterhoeven wrote: >>> On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> 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@bp.renesas.com> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> 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. Thank you, Claudiu > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Mon, Dec 9, 2024 at 1:22 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > On 09.12.2024 14:16, Geert Uytterhoeven wrote: > > On Mon, Dec 9, 2024 at 12:14 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > >> On 09.12.2024 12:57, Geert Uytterhoeven wrote: > >>> On Wed, Nov 13, 2024 at 2:35 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>>> > >>>> 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@bp.renesas.com> > >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>> > >>> 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
diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c index 76d7ea1964c3..1398d16df5d0 100644 --- a/drivers/clk/clk-versaclock3.c +++ b/drivers/clk/clk-versaclock3.c @@ -78,9 +78,6 @@ #define VC3_PLL1_VCO_MIN 300000000UL #define VC3_PLL1_VCO_MAX 600000000UL -#define VC3_PLL2_VCO_MIN 400000000UL -#define VC3_PLL2_VCO_MAX 1200000000UL - #define VC3_PLL3_VCO_MIN 300000000UL #define VC3_PLL3_VCO_MAX 800000000UL @@ -147,9 +144,13 @@ struct vc3_pfd_data { u8 mdiv2_bitmsk; }; +struct vc3_vco { + unsigned long min; + unsigned long max; +}; + struct vc3_pll_data { - unsigned long vco_min; - unsigned long vco_max; + struct vc3_vco vco; u8 num; u8 int_div_msb_offs; u8 int_div_lsb_offs; @@ -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; }; +struct vc3_hw_cfg { + struct vc3_vco pll2_vco; + u32 se2_clk_sel_msk; +}; + static const struct clk_div_table div1_divs[] = { { .val = 0, .div = 1, }, { .val = 1, .div = 4, }, { .val = 2, .div = 5, }, { .val = 3, .div = 6, }, @@ -386,10 +392,10 @@ static long vc3_pll_round_rate(struct clk_hw *hw, unsigned long rate, const struct vc3_pll_data *pll = vc3->data; u64 div_frc; - if (rate < pll->vco_min) - rate = pll->vco_min; - if (rate > pll->vco_max) - rate = pll->vco_max; + if (rate < pll->vco.min) + rate = pll->vco.min; + if (rate > pll->vco.max) + rate = pll->vco.max; vc3->div_int = rate / *parent_rate; @@ -680,8 +686,10 @@ static struct vc3_hw_data clk_pll[] = { .num = VC3_PLL1, .int_div_msb_offs = VC3_PLL1_LOOP_FILTER_N_DIV_MSB, .int_div_lsb_offs = VC3_PLL1_VCO_N_DIVIDER, - .vco_min = VC3_PLL1_VCO_MIN, - .vco_max = VC3_PLL1_VCO_MAX + .vco = { + .min = VC3_PLL1_VCO_MIN, + .max = VC3_PLL1_VCO_MAX + } }, .hw.init = &(struct clk_init_data) { .name = "pll1", @@ -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", @@ -716,8 +722,10 @@ static struct vc3_hw_data clk_pll[] = { .num = VC3_PLL3, .int_div_msb_offs = VC3_PLL3_LOOP_FILTER_N_DIV_MSB, .int_div_lsb_offs = VC3_PLL3_N_DIVIDER, - .vco_min = VC3_PLL3_VCO_MIN, - .vco_max = VC3_PLL3_VCO_MAX + .vco = { + .min = VC3_PLL3_VCO_MIN, + .max = VC3_PLL3_VCO_MAX + } }, .hw.init = &(struct clk_init_data) { .name = "pll3", @@ -901,7 +909,6 @@ static struct vc3_hw_data clk_mux[] = { [VC3_SE2_MUX] = { .data = &(struct vc3_clk_data) { .offs = VC3_SE2_CTRL_REG0, - .bitmsk = VC3_SE2_CTRL_REG0_SE2_CLK_SEL }, .hw.init = &(struct clk_init_data) { .name = "se2_mux", @@ -982,6 +989,7 @@ static int vc3_probe(struct i2c_client *client) { struct device *dev = &client->dev; u8 settings[NUM_CONFIG_REGISTERS]; + const struct vc3_hw_cfg *data; struct regmap *regmap; const char *name; int ret, i; @@ -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; + } ret = devm_clk_hw_register(dev, &clk_pll[i].hw); if (ret) return dev_err_probe(dev, ret, "%s failed\n", @@ -1059,6 +1074,11 @@ static int vc3_probe(struct i2c_client *client) /* Register clk muxes */ for (i = 0; i < ARRAY_SIZE(clk_mux); i++) { clk_mux[i].regmap = regmap; + if (i == VC3_SE2_MUX) { + struct vc3_clk_data *clk_data = clk_mux[i].data; + + clk_data->bitmsk = data->se2_clk_sel_msk; + } ret = devm_clk_hw_register(dev, &clk_mux[i].hw); if (ret) return dev_err_probe(dev, ret, "%s failed\n", @@ -1108,8 +1128,13 @@ static int vc3_probe(struct i2c_client *client) return ret; } +static const struct vc3_hw_cfg vc3_5p = { + .pll2_vco = { .min = 400000000UL, .max = 1200000000UL }, + .se2_clk_sel_msk = BIT(6), +}; + static const struct of_device_id dev_ids[] = { - { .compatible = "renesas,5p35023" }, + { .compatible = "renesas,5p35023", .data = &vc3_5p }, { /* Sentinel */ } }; MODULE_DEVICE_TABLE(of, dev_ids);