| Message ID | 20230817090810.203900-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix Versa3 clock mapping | expand |
Hi Biju, On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > According to Table 3. ("Output Source") in the 5P35023 datasheet, > the output clock mapping should be 0=REF, 1=SE1, 2=SE2, 3=SE3, > 4=DIFF1, 5=DIFF2. But the code uses inverse. Fix this mapping issue. > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > Closes: https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0Y2ZpLCMNg@mail.gmail.com/ > Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver") > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> One suggestion for future improvement (which can be a separate patch) below... > --- a/drivers/clk/clk-versaclock3.c > +++ b/drivers/clk/clk-versaclock3.c > @@ -119,20 +119,20 @@ enum vc3_div { > }; > > enum vc3_clk_mux { > - VC3_DIFF2_MUX, > - VC3_DIFF1_MUX, > - VC3_SE3_MUX, > - VC3_SE2_MUX, > VC3_SE1_MUX, > + VC3_SE2_MUX, > + VC3_SE3_MUX, > + VC3_DIFF1_MUX, > + VC3_DIFF2_MUX, > }; > > enum vc3_clk { > - VC3_DIFF2, > - VC3_DIFF1, > - VC3_SE3, > - VC3_SE2, > - VC3_SE1, > VC3_REF, > + VC3_SE1, > + VC3_SE2, > + VC3_SE3, > + VC3_DIFF1, > + VC3_DIFF2, > }; > > struct vc3_clk_data { > @@ -1110,7 +1110,7 @@ static int vc3_probe(struct i2c_client *client) > name, 0, CLK_SET_RATE_PARENT, 1, 1); > else > clk_out[i] = devm_clk_hw_register_fixed_factor_parent_hw(dev, > - name, &clk_mux[i].hw, CLK_SET_RATE_PARENT, 1, 1); > + name, &clk_mux[i - 1].hw, CLK_SET_RATE_PARENT, 1, 1); This is (and was before) fragile, as it implies a strict relation between the vc3_clk_mux and vc3_clk enum values. To avoid accidental breakage, I think it would be wise to make this explicit, e.g. enum vc3_clk_mux { VC3_SE1_MUX = VC3_SE1 - 1, VC3_SE2_MUX = VC3_SE2 - 1, [...] }; > > if (IS_ERR(clk_out[i])) > return PTR_ERR(clk_out[i]); Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v2 2/3] clk: vc3: Fix output clock mapping > > Hi Biju, > > On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > According to Table 3. ("Output Source") in the 5P35023 datasheet, the > > output clock mapping should be 0=REF, 1=SE1, 2=SE2, 3=SE3, 4=DIFF1, > > 5=DIFF2. But the code uses inverse. Fix this mapping issue. > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver") > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > One suggestion for future improvement (which can be a separate patch) > below... Ok. > > > --- a/drivers/clk/clk-versaclock3.c > > +++ b/drivers/clk/clk-versaclock3.c > > @@ -119,20 +119,20 @@ enum vc3_div { > > }; > > > > enum vc3_clk_mux { > > - VC3_DIFF2_MUX, > > - VC3_DIFF1_MUX, > > - VC3_SE3_MUX, > > - VC3_SE2_MUX, > > VC3_SE1_MUX, > > + VC3_SE2_MUX, > > + VC3_SE3_MUX, > > + VC3_DIFF1_MUX, > > + VC3_DIFF2_MUX, > > }; > > > > enum vc3_clk { > > - VC3_DIFF2, > > - VC3_DIFF1, > > - VC3_SE3, > > - VC3_SE2, > > - VC3_SE1, > > VC3_REF, > > + VC3_SE1, > > + VC3_SE2, > > + VC3_SE3, > > + VC3_DIFF1, > > + VC3_DIFF2, > > }; > > > > struct vc3_clk_data { > > > @@ -1110,7 +1110,7 @@ static int vc3_probe(struct i2c_client *client) > > name, 0, CLK_SET_RATE_PARENT, 1, 1); > > else > > clk_out[i] = > devm_clk_hw_register_fixed_factor_parent_hw(dev, > > - name, &clk_mux[i].hw, > CLK_SET_RATE_PARENT, 1, 1); > > + name, &clk_mux[i - 1].hw, > > + CLK_SET_RATE_PARENT, 1, 1); > > This is (and was before) fragile, as it implies a strict relation between > the vc3_clk_mux and vc3_clk enum values. To avoid accidental breakage, I > think it would be wise to make this explicit, e.g. > > enum vc3_clk_mux { > VC3_SE1_MUX = VC3_SE1 - 1, > VC3_SE2_MUX = VC3_SE2 - 1, > [...] > }; Agreed. Cheers, Biju > > > > > if (IS_ERR(clk_out[i])) > > return PTR_ERR(clk_out[i]); > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > 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
diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c index 7ab2447bd203..4ee9078f4bf4 100644 --- a/drivers/clk/clk-versaclock3.c +++ b/drivers/clk/clk-versaclock3.c @@ -119,20 +119,20 @@ enum vc3_div { }; enum vc3_clk_mux { - VC3_DIFF2_MUX, - VC3_DIFF1_MUX, - VC3_SE3_MUX, - VC3_SE2_MUX, VC3_SE1_MUX, + VC3_SE2_MUX, + VC3_SE3_MUX, + VC3_DIFF1_MUX, + VC3_DIFF2_MUX, }; enum vc3_clk { - VC3_DIFF2, - VC3_DIFF1, - VC3_SE3, - VC3_SE2, - VC3_SE1, VC3_REF, + VC3_SE1, + VC3_SE2, + VC3_SE3, + VC3_DIFF1, + VC3_DIFF2, }; struct vc3_clk_data { @@ -897,33 +897,33 @@ static struct vc3_hw_data clk_div[] = { }; static struct vc3_hw_data clk_mux[] = { - [VC3_DIFF2_MUX] = { + [VC3_SE1_MUX] = { .data = &(struct vc3_clk_data) { - .offs = VC3_DIFF2_CTRL_REG, - .bitmsk = VC3_DIFF2_CTRL_REG_DIFF2_CLK_SEL + .offs = VC3_SE1_DIV4_CTRL, + .bitmsk = VC3_SE1_DIV4_CTRL_SE1_CLK_SEL }, .hw.init = &(struct clk_init_data){ - .name = "diff2_mux", + .name = "se1_mux", .ops = &vc3_clk_mux_ops, .parent_hws = (const struct clk_hw *[]) { - &clk_div[VC3_DIV1].hw, - &clk_div[VC3_DIV3].hw + &clk_div[VC3_DIV5].hw, + &clk_div[VC3_DIV4].hw }, .num_parents = 2, .flags = CLK_SET_RATE_PARENT } }, - [VC3_DIFF1_MUX] = { + [VC3_SE2_MUX] = { .data = &(struct vc3_clk_data) { - .offs = VC3_DIFF1_CTRL_REG, - .bitmsk = VC3_DIFF1_CTRL_REG_DIFF1_CLK_SEL + .offs = VC3_SE2_CTRL_REG0, + .bitmsk = VC3_SE2_CTRL_REG0_SE2_CLK_SEL }, .hw.init = &(struct clk_init_data){ - .name = "diff1_mux", + .name = "se2_mux", .ops = &vc3_clk_mux_ops, .parent_hws = (const struct clk_hw *[]) { - &clk_div[VC3_DIV1].hw, - &clk_div[VC3_DIV3].hw + &clk_div[VC3_DIV5].hw, + &clk_div[VC3_DIV4].hw }, .num_parents = 2, .flags = CLK_SET_RATE_PARENT @@ -945,33 +945,33 @@ static struct vc3_hw_data clk_mux[] = { .flags = CLK_SET_RATE_PARENT } }, - [VC3_SE2_MUX] = { + [VC3_DIFF1_MUX] = { .data = &(struct vc3_clk_data) { - .offs = VC3_SE2_CTRL_REG0, - .bitmsk = VC3_SE2_CTRL_REG0_SE2_CLK_SEL + .offs = VC3_DIFF1_CTRL_REG, + .bitmsk = VC3_DIFF1_CTRL_REG_DIFF1_CLK_SEL }, .hw.init = &(struct clk_init_data){ - .name = "se2_mux", + .name = "diff1_mux", .ops = &vc3_clk_mux_ops, .parent_hws = (const struct clk_hw *[]) { - &clk_div[VC3_DIV5].hw, - &clk_div[VC3_DIV4].hw + &clk_div[VC3_DIV1].hw, + &clk_div[VC3_DIV3].hw }, .num_parents = 2, .flags = CLK_SET_RATE_PARENT } }, - [VC3_SE1_MUX] = { + [VC3_DIFF2_MUX] = { .data = &(struct vc3_clk_data) { - .offs = VC3_SE1_DIV4_CTRL, - .bitmsk = VC3_SE1_DIV4_CTRL_SE1_CLK_SEL + .offs = VC3_DIFF2_CTRL_REG, + .bitmsk = VC3_DIFF2_CTRL_REG_DIFF2_CLK_SEL }, .hw.init = &(struct clk_init_data){ - .name = "se1_mux", + .name = "diff2_mux", .ops = &vc3_clk_mux_ops, .parent_hws = (const struct clk_hw *[]) { - &clk_div[VC3_DIV5].hw, - &clk_div[VC3_DIV4].hw + &clk_div[VC3_DIV1].hw, + &clk_div[VC3_DIV3].hw }, .num_parents = 2, .flags = CLK_SET_RATE_PARENT @@ -1110,7 +1110,7 @@ static int vc3_probe(struct i2c_client *client) name, 0, CLK_SET_RATE_PARENT, 1, 1); else clk_out[i] = devm_clk_hw_register_fixed_factor_parent_hw(dev, - name, &clk_mux[i].hw, CLK_SET_RATE_PARENT, 1, 1); + name, &clk_mux[i - 1].hw, CLK_SET_RATE_PARENT, 1, 1); if (IS_ERR(clk_out[i])) return PTR_ERR(clk_out[i]);
According to Table 3. ("Output Source") in the 5P35023 datasheet, the output clock mapping should be 0=REF, 1=SE1, 2=SE2, 3=SE3, 4=DIFF1, 5=DIFF2. But the code uses inverse. Fix this mapping issue. Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> Closes: https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0Y2ZpLCMNg@mail.gmail.com/ Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver") Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v1->v2: * No change. --- drivers/clk/clk-versaclock3.c | 68 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 34 deletions(-)