| Message ID | 20210719143811.2135-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Renesas RZ/G2L CANFD support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi Prabhakar, On Mon, Jul 19, 2021 at 4:39 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! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Just some bikeshedding on the exact naming below ;-) > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > @@ -91,6 +92,59 @@ required: > - channel0 > - channel1 > > +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 > + > + interrupt-names: > + items: > + - const: g_error > + - const: g_rx_fifo > + - const: can0_error s/error/err/? > + - const: can0_tx > + - const: can0_tx_rx_fifo_receive_completion > + - const: can1_error > + - const: can1_tx > + - const: can1_tx_rx_fifo_receive_completion s/receive/rx/? Some are also a bit long to type. Perhaps use naming closer to the User's Manual? INTRCANGERR => g_err INTRCANGRECC => g_recc INTRCAN0ERR => ch0_err INTRCAN0REC => ch0_rec INTRCAN0TRX => ch0_trx INTRCAN1ERR => ch1_err INTRCAN1REC => ch1_rec INTRCAN1TRX => ch1_trx These do not have "_int" suffixes... > + > + resets: > + items: > + - description: CANFD_RSTP_N > + - description: CANFD_RSTC_N > + > + required: > + - interrupt-names > +else: > + properties: > + interrupts: > + items: > + - description: Channel interrupt > + - description: Global interrupt > + > + interrupt-names: > + items: > + - const: ch_int > + - const: g_int ... and these do have "_int" suffixes. > + > + resets: > + items: > + - description: CANFD reset > + > unevaluatedProperties: false Gr{oetje,eeting}s, Geert
Hi Lad, On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar 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> > --- > .../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++-- > 1 file changed, 60 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > index 0b33ba9ccb47..4fb6dd370904 100644 > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > @@ -30,13 +30,15 @@ properties: > - renesas,r8a77995-canfd # R-Car D3 > - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2 > > + - items: > + - enum: > + - renesas,r9a07g044-canfd # RZ/G2{L,LC} > + - const: renesas,rzg2l-canfd # RZ/G2L family > + > reg: > maxItems: 1 > > - interrupts: > - items: > - - description: Channel interrupt > - - description: Global interrupt > + interrupts: true > > clocks: > maxItems: 3 > @@ -50,8 +52,7 @@ properties: > power-domains: > maxItems: 1 > > - resets: > - maxItems: 1 > + resets: true > > renesas,no-can-fd: > $ref: /schemas/types.yaml#/definitions/flag > @@ -91,6 +92,59 @@ required: > - channel0 > - channel1 > > +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 > + > + interrupt-names: > + items: > + - const: g_error > + - const: g_rx_fifo > + - const: can0_error > + - const: can0_tx > + - const: can0_tx_rx_fifo_receive_completion > + - const: can1_error > + - const: can1_tx > + - const: can1_tx_rx_fifo_receive_completion > + > + resets: > + items: > + - description: CANFD_RSTP_N > + - description: CANFD_RSTC_N Do you know what the "P" and "C" stands for? It would be nice if the description could tell us what the reset lines are used for. I would prefer if you used these names (or shortened versions, for example "rstp_n", "rstc_n") as "reset-names" and let the driver reference the resets by name instead of by index. regards Philipp
Hi Philipp, Thank you for the review. On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Lad, > > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar 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> > > --- > > .../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++-- > > 1 file changed, 60 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > index 0b33ba9ccb47..4fb6dd370904 100644 > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > @@ -30,13 +30,15 @@ properties: > > - renesas,r8a77995-canfd # R-Car D3 > > - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2 > > > > + - items: > > + - enum: > > + - renesas,r9a07g044-canfd # RZ/G2{L,LC} > > + - const: renesas,rzg2l-canfd # RZ/G2L family > > + > > reg: > > maxItems: 1 > > > > - interrupts: > > - items: > > - - description: Channel interrupt > > - - description: Global interrupt > > + interrupts: true > > > > clocks: > > maxItems: 3 > > @@ -50,8 +52,7 @@ properties: > > power-domains: > > maxItems: 1 > > > > - resets: > > - maxItems: 1 > > + resets: true > > > > renesas,no-can-fd: > > $ref: /schemas/types.yaml#/definitions/flag > > @@ -91,6 +92,59 @@ required: > > - channel0 > > - channel1 > > > > +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 > > + > > + interrupt-names: > > + items: > > + - const: g_error > > + - const: g_rx_fifo > > + - const: can0_error > > + - const: can0_tx > > + - const: can0_tx_rx_fifo_receive_completion > > + - const: can1_error > > + - const: can1_tx > > + - const: can1_tx_rx_fifo_receive_completion > > + > > + resets: > > + items: > > + - description: CANFD_RSTP_N > > + - description: CANFD_RSTC_N > > Do you know what the "P" and "C" stands for? It would be nice if the > description could tell us what the reset lines are used for. > unfortunately the HW manual does not mention anything about "P" and "C" :( > I would prefer if you used these names (or shortened versions, for > example "rstp_n", "rstc_n") as "reset-names" and let the driver > reference the resets by name instead of by index. > OK will do that and maxItems:2 for resets. @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset) sounds good for reset-names? Or do you have any other suggestions? Cheers, Prabhakar
Hi Geert, Thank you for the review. On Tue, Jul 20, 2021 at 11:21 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Jul 19, 2021 at 4:39 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! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Just some bikeshedding on the exact naming below ;-) > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > @@ -91,6 +92,59 @@ required: > > - channel0 > > - channel1 > > > > +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 > > + > > + interrupt-names: > > + items: > > + - const: g_error > > + - const: g_rx_fifo > > + - const: can0_error > > s/error/err/? > > > + - const: can0_tx > > + - const: can0_tx_rx_fifo_receive_completion > > + - const: can1_error > > + - const: can1_tx > > + - const: can1_tx_rx_fifo_receive_completion > > s/receive/rx/? > > Some are also a bit long to type. > Perhaps use naming closer to the User's Manual? > > INTRCANGERR => g_err > INTRCANGRECC => g_recc > INTRCAN0ERR => ch0_err > INTRCAN0REC => ch0_rec > INTRCAN0TRX => ch0_trx > INTRCAN1ERR => ch1_err > INTRCAN1REC => ch1_rec > INTRCAN1TRX => ch1_trx > > These do not have "_int" suffixes... > Agreed thanks for the input. > > + > > + resets: > > + items: > > + - description: CANFD_RSTP_N > > + - description: CANFD_RSTC_N > > + > > + required: > > + - interrupt-names > > +else: > > + properties: > > + interrupts: > > + items: > > + - description: Channel interrupt > > + - description: Global interrupt > > + > > + interrupt-names: > > + items: > > + - const: ch_int > > + - const: g_int > > ... and these do have "_int" suffixes. > indeed Cheers, Prabhakar > > + > > + resets: > > + items: > > + - description: CANFD reset > > + > > unevaluatedProperties: false > > 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 Tue, Jul 20, 2021 at 4:31 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar 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> > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > + resets: > > > + items: > > > + - description: CANFD_RSTP_N > > > + - description: CANFD_RSTC_N > > > > Do you know what the "P" and "C" stands for? It would be nice if the > > description could tell us what the reset lines are used for. > > > unfortunately the HW manual does not mention anything about "P" and "C" :( > > > I would prefer if you used these names (or shortened versions, for > > example "rstp_n", "rstc_n") as "reset-names" and let the driver > > reference the resets by name instead of by index. > > > OK will do that and maxItems:2 for resets. > > @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset) > sounds good for reset-names? Or do you have any other suggestions? I wouldn't bother with reset-names on R-Car, as there is only a single reset. BTW, does there exist a generally-accepted reset-equivalent of "fck" ("Functional ClocK")? Gr{oetje,eeting}s, Geert
Hi Geert, On Tue, Jul 20, 2021 at 4:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Tue, Jul 20, 2021 at 4:31 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar 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> > > > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > > > + resets: > > > > + items: > > > > + - description: CANFD_RSTP_N > > > > + - description: CANFD_RSTC_N > > > > > > Do you know what the "P" and "C" stands for? It would be nice if the > > > description could tell us what the reset lines are used for. > > > > > unfortunately the HW manual does not mention anything about "P" and "C" :( > > > > > I would prefer if you used these names (or shortened versions, for > > > example "rstp_n", "rstc_n") as "reset-names" and let the driver > > > reference the resets by name instead of by index. > > > > > OK will do that and maxItems:2 for resets. > > > > @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset) > > sounds good for reset-names? Or do you have any other suggestions? > > I wouldn't bother with reset-names on R-Car, as there is only a > single reset. > OK will keep "description: CANFD reset" for R-Car as done in the current patch and just add reset-names only for RZ/G2L SoC. > BTW, does there exist a generally-accepted reset-equivalent of "fck" > ("Functional ClocK")? > None that I am aware of (Couple of binding docs have "rst"), but maybe Philipp could have some suggestions. Cheers, Prabhakar > 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 Tue, 2021-07-20 at 15:31 +0100, Lad, Prabhakar wrote: > Hi Philipp, > > Thank you for the review. > > On Tue, Jul 20, 2021 at 11:22 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Lad, Sorry I mixed up your name. > > On Mon, 2021-07-19 at 15:38 +0100, Lad Prabhakar 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> > > > --- > > > .../bindings/net/can/renesas,rcar-canfd.yaml | 66 +++++++++++++++++-- > > > 1 file changed, 60 insertions(+), 6 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > index 0b33ba9ccb47..4fb6dd370904 100644 > > > --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml > > > @@ -30,13 +30,15 @@ properties: > > > - renesas,r8a77995-canfd # R-Car D3 > > > - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2 > > > > > > + - items: > > > + - enum: > > > + - renesas,r9a07g044-canfd # RZ/G2{L,LC} > > > + - const: renesas,rzg2l-canfd # RZ/G2L family > > > + > > > reg: > > > maxItems: 1 > > > > > > - interrupts: > > > - items: > > > - - description: Channel interrupt > > > - - description: Global interrupt > > > + interrupts: true > > > > > > clocks: > > > maxItems: 3 > > > @@ -50,8 +52,7 @@ properties: > > > power-domains: > > > maxItems: 1 > > > > > > - resets: > > > - maxItems: 1 > > > + resets: true > > > > > > renesas,no-can-fd: > > > $ref: /schemas/types.yaml#/definitions/flag > > > @@ -91,6 +92,59 @@ required: > > > - channel0 > > > - channel1 > > > > > > +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 > > > + > > > + interrupt-names: > > > + items: > > > + - const: g_error > > > + - const: g_rx_fifo > > > + - const: can0_error > > > + - const: can0_tx > > > + - const: can0_tx_rx_fifo_receive_completion > > > + - const: can1_error > > > + - const: can1_tx > > > + - const: can1_tx_rx_fifo_receive_completion > > > + > > > + resets: > > > + items: > > > + - description: CANFD_RSTP_N > > > + - description: CANFD_RSTC_N > > > > Do you know what the "P" and "C" stands for? It would be nice if the > > description could tell us what the reset lines are used for. > > > unfortunately the HW manual does not mention anything about "P" and "C" :( Yes, unfortunately this is all too common. > > I would prefer if you used these names (or shortened versions, for > > example "rstp_n", "rstc_n") as "reset-names" and let the driver > > reference the resets by name instead of by index. > > > OK will do that and maxItems:2 for resets. > > @Geert, for R-Car Gen3 does "canfd_rst" (as it's a module reset) > sounds good for reset-names? Or do you have any other suggestions? I agree with Geert here. Assuming no second reset will be discovered for R-Car Gen3 later, there is no need to invent a name. regards Philipp
Hi Geert, On Tue, 2021-07-20 at 17:11 +0200, Geert Uytterhoeven wrote: [...] > I wouldn't bother with reset-names on R-Car, as there is only a > single reset. > > BTW, does there exist a generally-accepted reset-equivalent of "fck" > ("Functional ClocK")? Not really. There is "rst", which seems to be slightly more popular than "reset". Some bindings use "core" to differentiate between the functional reset and peripheral resets like bus or phy resets. Ideally the reset-names would match the names of the reset inputs in the IP core documentation (not the global names of the reset signals in the SoC documentation). But more often than not they are not known. regards Philipp
diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml index 0b33ba9ccb47..4fb6dd370904 100644 --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml @@ -30,13 +30,15 @@ properties: - renesas,r8a77995-canfd # R-Car D3 - const: renesas,rcar-gen3-canfd # R-Car Gen3 and RZ/G2 + - items: + - enum: + - renesas,r9a07g044-canfd # RZ/G2{L,LC} + - const: renesas,rzg2l-canfd # RZ/G2L family + reg: maxItems: 1 - interrupts: - items: - - description: Channel interrupt - - description: Global interrupt + interrupts: true clocks: maxItems: 3 @@ -50,8 +52,7 @@ properties: power-domains: maxItems: 1 - resets: - maxItems: 1 + resets: true renesas,no-can-fd: $ref: /schemas/types.yaml#/definitions/flag @@ -91,6 +92,59 @@ required: - channel0 - channel1 +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 + + interrupt-names: + items: + - const: g_error + - const: g_rx_fifo + - const: can0_error + - const: can0_tx + - const: can0_tx_rx_fifo_receive_completion + - const: can1_error + - const: can1_tx + - const: can1_tx_rx_fifo_receive_completion + + resets: + items: + - description: CANFD_RSTP_N + - description: CANFD_RSTC_N + + required: + - interrupt-names +else: + properties: + interrupts: + items: + - description: Channel interrupt + - description: Global interrupt + + interrupt-names: + items: + - const: ch_int + - const: g_int + + resets: + items: + - description: CANFD reset + unevaluatedProperties: false examples: