| Message ID | 20230824082501.87429-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Fix Versa3 clock mapping | expand |
Hi Biju, On Thu, Aug 24, 2023 at 10:25 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> This order should be documented in the DT bindings, too. Gr{oetje,eeting}s, Geert
Hi Geert Uytterhoeven, > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping > > Hi Biju, > > On Thu, Aug 24, 2023 at 10:25 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> > > This order should be documented in the DT bindings, too. Ok, will update the mapping in bindings like below. + assigned-clocks: + items: + - description: Link clock generator. + - description: Output clock index. The index is mapped to the clock + output in the hardware manual as follows + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. + Cheers, Biju
Hi Biju, On Thu, Aug 24, 2023 at 11:20 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping > > On Thu, Aug 24, 2023 at 10:25 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> > > > > This order should be documented in the DT bindings, too. > > Ok, will update the mapping in bindings like below. > > + assigned-clocks: > + items: > + - description: Link clock generator. > + - description: Output clock index. The index is mapped to the clock > + output in the hardware manual as follows > + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. > + There is no need to document assigned-clocks. The clock indices documentation belongs to the #clock-cells property. 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
Hi Geert Uytterhoeven, Thanks for the feedback. > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping > > Hi Biju, > > On Thu, Aug 24, 2023 at 11:20 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping On > > > Thu, Aug 24, 2023 at 10:25 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> > > > > > > This order should be documented in the DT bindings, too. > > > > Ok, will update the mapping in bindings like below. > > > > + assigned-clocks: > > + items: > > + - description: Link clock generator. > > + - description: Output clock index. The index is mapped to the > clock > > + output in the hardware manual as follows > > + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. > > + > > There is no need to document assigned-clocks. > The clock indices documentation belongs to the #clock-cells property. OK, I will update the main description as below --- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml +++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml @@ -25,6 +25,9 @@ description: | boots. Any configuration not supported by the common clock framework must be done via the full register map, including optimized settings. + The index in the assigned-clocks is mapped to the output clock as below + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. + Link to datasheet: Cheers, Biju
Hi Biju, On Thu, Aug 24, 2023 at 11:48 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping > > On Thu, Aug 24, 2023 at 11:20 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping On > > > > Thu, Aug 24, 2023 at 10:25 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> > > > > > > > > This order should be documented in the DT bindings, too. > > > > > > Ok, will update the mapping in bindings like below. > > > > > > + assigned-clocks: > > > + items: > > > + - description: Link clock generator. > > > + - description: Output clock index. The index is mapped to the > > clock > > > + output in the hardware manual as follows > > > + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. > > > + > > > > There is no need to document assigned-clocks. > > The clock indices documentation belongs to the #clock-cells property. > > OK, I will update the main description as below > > --- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml > +++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml > @@ -25,6 +25,9 @@ description: | > boots. Any configuration not supported by the common clock framework > must be done via the full register map, including optimized settings. > > + The index in the assigned-clocks is mapped to the output clock as below > + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. > + > Link to datasheet: Please add this to the #clocks-cells property instead. Gr{oetje,eeting}s, Geert
Hi Geert Uytterhoeven, > > On Thu, Aug 24, 2023 at 11:48 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping On > > > Thu, Aug 24, 2023 at 11:20 AM Biju Das <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > Subject: Re: [PATCH v4 3/4] clk: vc3: Fix output clock mapping > > > > > On Thu, Aug 24, 2023 at 10:25 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> > > > > > > > > > > This order should be documented in the DT bindings, too. > > > > > > > > Ok, will update the mapping in bindings like below. > > > > > > > > + assigned-clocks: > > > > + items: > > > > + - description: Link clock generator. > > > > + - description: Output clock index. The index is mapped to > > > > + the > > > clock > > > > + output in the hardware manual as follows > > > > + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. > > > > + > > > > > > There is no need to document assigned-clocks. > > > The clock indices documentation belongs to the #clock-cells property. > > > > OK, I will update the main description as below > > > > --- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml > > +++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml > > @@ -25,6 +25,9 @@ description: | > > boots. Any configuration not supported by the common clock framework > > must be done via the full register map, including optimized settings. > > > > + The index in the assigned-clocks is mapped to the output clock as > > + below > > + 0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2. > > + > > Link to datasheet: > > Please add this to the #clocks-cells property instead. Oops. Missed that. It makes sense. Cheers, Biju
diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c index b1a94db1f3c9..157cf510b23a 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 { @@ -896,33 +896,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 @@ -944,33 +944,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 @@ -1109,7 +1109,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> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3->v4: * No change. v2->v3: * Added Rb tag from Geert. v1->v2: * No change. --- drivers/clk/clk-versaclock3.c | 68 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 34 deletions(-)