| Message ID | 20210715182123.23372-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Renesas RZ/G2L CANFD support | expand |
Hi Prabhakar, On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Add CANFD binding documentation for Renesas RZ/G2L SoC. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > @@ -78,6 +79,38 @@ patternProperties: > node. Each child node supports the "status" property only, which > is used to enable/disable the respective channel. > > +if: > + properties: > + compatible: > + contains: > + enum: > + - renesas,rzg2l-canfd > +then: > + properties: > + interrupts: > + items: > + - description: CAN global error interrupt > + - description: CAN receive FIFO interrupt > + - description: CAN0 error interrupt > + - description: CAN0 transmit interrupt > + - description: CAN0 transmit/receive FIFO receive completion interrupt > + - description: CAN1 error interrupt > + - description: CAN1 transmit interrupt > + - description: CAN1 transmit/receive FIFO receive completion interrupt Does it make sense to add interrupt-names? > + > + resets: > + maxItems: 2 Same here, for reset-names? Or a list of descriptions, so we know which reset serves what purpose. > + > +else: > + properties: > + interrupts: > + items: > + - description: Channel interrupt > + - description: Global interrupt > + > + resets: > + maxItems: 1 > + > required: > - compatible > - reg The rest looks good to me. 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 Prabhakar, Thanks for your patch! On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Add P0_DIV2 core clock required for CANFD module. CANFD core clock is > sourced from P0_DIV2 referenced from HW manual Rev.0.50. OK. > Also add R9A07G044_LAST_CORE_CLK entry to avoid changes in > r9a07g044-cpg.c file. I'm not so fond of adding this. Unlike the other definitions, it is not really part of the bindings, but merely a convenience definition for the driver. Furthermore it has to change when a new definition is ever added. > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > include/dt-bindings/clock/r9a07g044-cpg.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h > index 0728ad07ff7a..2fd20db0b2f4 100644 > --- a/include/dt-bindings/clock/r9a07g044-cpg.h > +++ b/include/dt-bindings/clock/r9a07g044-cpg.h > @@ -30,6 +30,8 @@ > #define R9A07G044_CLK_P2 19 > #define R9A07G044_CLK_AT 20 > #define R9A07G044_OSCCLK 21 > +#define R9A07G044_CLK_P0_DIV2 22 > +#define R9A07G044_LAST_CORE_CLK 23 Third issue: off-by-one error, it should be 22 ;-) > > /* R9A07G044 Module Clocks */ > #define R9A07G044_CA55_SCLK 0 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 Prabhakar, On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Add entry for fixed core clock P0_DIV2 and assign LAST_DT_CORE_CLK > to R9A07G044_LAST_CORE_CLK. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/r9a07g044-cpg.c > +++ b/drivers/clk/renesas/r9a07g044-cpg.c > @@ -16,7 +16,7 @@ > > enum clk_ids { > /* Core Clock Outputs exported to DT */ > - LAST_DT_CORE_CLK = R9A07G044_OSCCLK, > + LAST_DT_CORE_CLK = R9A07G044_LAST_CORE_CLK, Please use R9A07G044_CLK_P0_DIV2 instead. > > /* External Input Clocks */ > CLK_EXTAL, > @@ -77,6 +77,7 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = { > DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1), > DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A, > dtable_1_32, CLK_DIVIDER_HIWORD_MASK), > + DEF_FIXED("P0_DIV2", R9A07G044_CLK_P0_DIV2, R9A07G044_CLK_P0, 1, 2), > DEF_FIXED("TSU", R9A07G044_CLK_TSU, CLK_PLL2_DIV20, 1, 1), > DEF_DIV("P1", R9A07G044_CLK_P1, CLK_PLL3_DIV2_4, > DIVPL3B, dtable_1_32, CLK_DIVIDER_HIWORD_MASK), The rest looks good to me. 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, Thank you for the review. On Fri, Jul 16, 2021 at 8:38 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add CANFD binding documentation for Renesas RZ/G2L SoC. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > @@ -78,6 +79,38 @@ patternProperties: > > node. Each child node supports the "status" property only, which > > is used to enable/disable the respective channel. > > > > +if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - renesas,rzg2l-canfd > > +then: > > + properties: > > + interrupts: > > + items: > > + - description: CAN global error interrupt > > + - description: CAN receive FIFO interrupt > > + - description: CAN0 error interrupt > > + - description: CAN0 transmit interrupt > > + - description: CAN0 transmit/receive FIFO receive completion interrupt > > + - description: CAN1 error interrupt > > + - description: CAN1 transmit interrupt > > + - description: CAN1 transmit/receive FIFO receive completion interrupt > > Does it make sense to add interrupt-names? > Agreed will drop this and add interrupt-names instead. Also I will update the driver to pick up the interrupts based on names. > > + > > + resets: > > + maxItems: 2 > > Same here, for reset-names? > Or a list of descriptions, so we know which reset serves what purpose. > OK I'll add the reset-names. Cheers, Prabhakar > > + > > +else: > > + properties: > > + interrupts: > > + items: > > + - description: Channel interrupt > > + - description: Global interrupt > > + > > + resets: > > + maxItems: 1 > > + > > required: > > - compatible > > - reg > > The rest looks good to me. > > 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, Thank you for the review. On Fri, Jul 16, 2021 at 9:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > Thanks for your patch! > > On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add P0_DIV2 core clock required for CANFD module. CANFD core clock is > > sourced from P0_DIV2 referenced from HW manual Rev.0.50. > > OK. > > > Also add R9A07G044_LAST_CORE_CLK entry to avoid changes in > > r9a07g044-cpg.c file. > > I'm not so fond of adding this. Unlike the other definitions, it is > not really part of the bindings, but merely a convenience definition > for the driver. Furthermore it has to change when a new definition > is ever added. > Agreed will drop this. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > include/dt-bindings/clock/r9a07g044-cpg.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h > > index 0728ad07ff7a..2fd20db0b2f4 100644 > > --- a/include/dt-bindings/clock/r9a07g044-cpg.h > > +++ b/include/dt-bindings/clock/r9a07g044-cpg.h > > @@ -30,6 +30,8 @@ > > #define R9A07G044_CLK_P2 19 > > #define R9A07G044_CLK_AT 20 > > #define R9A07G044_OSCCLK 21 > > +#define R9A07G044_CLK_P0_DIV2 22 > > +#define R9A07G044_LAST_CORE_CLK 23 > > Third issue: off-by-one error, it should be 22 ;-) > 23 was intentionally as these numbers aren't used for core clock count we use r9a07g044_core_clks[] instead. Said that I'll drop this. Cheers, Prabhakar > > > > /* R9A07G044 Module Clocks */ > > #define R9A07G044_CA55_SCLK 0 > > 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, Thank you for the review. On Fri, Jul 16, 2021 at 9:09 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Add entry for fixed core clock P0_DIV2 and assign LAST_DT_CORE_CLK > > to R9A07G044_LAST_CORE_CLK. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c > > @@ -16,7 +16,7 @@ > > > > enum clk_ids { > > /* Core Clock Outputs exported to DT */ > > - LAST_DT_CORE_CLK = R9A07G044_OSCCLK, > > + LAST_DT_CORE_CLK = R9A07G044_LAST_CORE_CLK, > > Please use R9A07G044_CLK_P0_DIV2 instead. > Ok, I will update it. Cheers, Prabhakar > > > > /* External Input Clocks */ > > CLK_EXTAL, > > @@ -77,6 +77,7 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = { > > DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1), > > DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A, > > dtable_1_32, CLK_DIVIDER_HIWORD_MASK), > > + DEF_FIXED("P0_DIV2", R9A07G044_CLK_P0_DIV2, R9A07G044_CLK_P0, 1, 2), > > DEF_FIXED("TSU", R9A07G044_CLK_TSU, CLK_PLL2_DIV20, 1, 1), > > DEF_DIV("P1", R9A07G044_CLK_P1, CLK_PLL3_DIV2_4, > > DIVPL3B, dtable_1_32, CLK_DIVIDER_HIWORD_MASK), > > The rest looks good to me. > > 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 Prabhakar, On Fri, Jul 16, 2021 at 10:45 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Fri, Jul 16, 2021 at 9:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Add P0_DIV2 core clock required for CANFD module. CANFD core clock is > > > sourced from P0_DIV2 referenced from HW manual Rev.0.50. > > > > OK. > > > > > Also add R9A07G044_LAST_CORE_CLK entry to avoid changes in > > > r9a07g044-cpg.c file. > > > > I'm not so fond of adding this. Unlike the other definitions, it is > > not really part of the bindings, but merely a convenience definition > > for the driver. Furthermore it has to change when a new definition > > is ever added. > > > Agreed will drop this. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- > > > include/dt-bindings/clock/r9a07g044-cpg.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h > > > index 0728ad07ff7a..2fd20db0b2f4 100644 > > > --- a/include/dt-bindings/clock/r9a07g044-cpg.h > > > +++ b/include/dt-bindings/clock/r9a07g044-cpg.h > > > @@ -30,6 +30,8 @@ > > > #define R9A07G044_CLK_P2 19 > > > #define R9A07G044_CLK_AT 20 > > > #define R9A07G044_OSCCLK 21 > > > +#define R9A07G044_CLK_P0_DIV2 22 > > > +#define R9A07G044_LAST_CORE_CLK 23 > > > > Third issue: off-by-one error, it should be 22 ;-) > > > 23 was intentionally as these numbers aren't used for core clock count > we use r9a07g044_core_clks[] instead. It ends up as an off-by-one bug in the range check in rzg2l_cpg_clk_src_twocell_get(). > Said that I'll drop this. OK. 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, On Fri, Jul 16, 2021 at 9:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Fri, Jul 16, 2021 at 10:45 AM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Fri, Jul 16, 2021 at 9:08 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Thu, Jul 15, 2021 at 8:21 PM Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > Add P0_DIV2 core clock required for CANFD module. CANFD core clock is > > > > sourced from P0_DIV2 referenced from HW manual Rev.0.50. > > > > > > OK. > > > > > > > Also add R9A07G044_LAST_CORE_CLK entry to avoid changes in > > > > r9a07g044-cpg.c file. > > > > > > I'm not so fond of adding this. Unlike the other definitions, it is > > > not really part of the bindings, but merely a convenience definition > > > for the driver. Furthermore it has to change when a new definition > > > is ever added. > > > > > Agreed will drop this. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > --- > > > > include/dt-bindings/clock/r9a07g044-cpg.h | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h > > > > index 0728ad07ff7a..2fd20db0b2f4 100644 > > > > --- a/include/dt-bindings/clock/r9a07g044-cpg.h > > > > +++ b/include/dt-bindings/clock/r9a07g044-cpg.h > > > > @@ -30,6 +30,8 @@ > > > > #define R9A07G044_CLK_P2 19 > > > > #define R9A07G044_CLK_AT 20 > > > > #define R9A07G044_OSCCLK 21 > > > > +#define R9A07G044_CLK_P0_DIV2 22 > > > > +#define R9A07G044_LAST_CORE_CLK 23 > > > > > > Third issue: off-by-one error, it should be 22 ;-) > > > > > 23 was intentionally as these numbers aren't used for core clock count > > we use r9a07g044_core_clks[] instead. > > It ends up as an off-by-one bug in the range check in > rzg2l_cpg_clk_src_twocell_get(). > Ooops missed that! Cheers, Prabhakar > > Said that I'll drop this. > > OK. > > 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