diff mbox series

[v3,3/4] dt-bindings: serial: renesas,scif: Document R9A09G057 support

Message ID 20240318172102.45549-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add SCIF support for Renesas RZ/V2H(P) SoC | expand

Commit Message

Lad, Prabhakar March 18, 2024, 5:21 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Document support for the Serial Communication Interface with FIFO (SCIF)
available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in
the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L
(R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has
three additional interrupts: one for Tx end/Rx ready and the other two for
Rx and Tx buffer full, which are edge-triggered.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
- Added SoC specific compat string
---
 .../bindings/serial/renesas,scif.yaml         | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Geert Uytterhoeven March 19, 2024, 8:12 a.m. UTC | #1 
Hi Prabhakar,

On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Document support for the Serial Communication Interface with FIFO (SCIF)
> available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in
> the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L
> (R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has
> three additional interrupts: one for Tx end/Rx ready and the other two for
> Rx and Tx buffer full, which are edge-triggered.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Added SoC specific compat string

Thanks for the update!

> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> @@ -79,6 +79,8 @@ properties:
>                - renesas,scif-r9a08g045      # RZ/G3S
>            - const: renesas,scif-r9a07g044   # RZ/G2{L,LC} fallback
>
> +      - const: renesas,scif-r9a09g057       # RZ/V2H(P)
> +
>    reg:
>      maxItems: 1
>
> @@ -204,6 +206,37 @@ allOf:
>              - const: dri
>              - const: tei
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,scif-r9a09g057
> +    then:
> +      properties:
> +        interrupts:
> +          items:
> +            - description: Error interrupt

[...]

These descriptions should be at the top level.  The SoC-specific rules
should just restrict the lower limit (interrupts/minItems).

> +
> +        interrupt-names:
> +          items:
> +            - const: eri

[...]

Likewise.

In addition, you should restrict clocks/maxItems to 1, as on RZ/V2H
only the UART functional clock is supported.

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
Lad, Prabhakar March 19, 2024, 12:48 p.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Tue, Mar 19, 2024 at 8:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Document support for the Serial Communication Interface with FIFO (SCIF)
> > available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in
> > the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L
> > (R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has
> > three additional interrupts: one for Tx end/Rx ready and the other two for
> > Rx and Tx buffer full, which are edge-triggered.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > - Added SoC specific compat string
>
> Thanks for the update!
>
> > --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > @@ -79,6 +79,8 @@ properties:
> >                - renesas,scif-r9a08g045      # RZ/G3S
> >            - const: renesas,scif-r9a07g044   # RZ/G2{L,LC} fallback
> >
> > +      - const: renesas,scif-r9a09g057       # RZ/V2H(P)
> > +
> >    reg:
> >      maxItems: 1
> >
> > @@ -204,6 +206,37 @@ allOf:
> >              - const: dri
> >              - const: tei
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: renesas,scif-r9a09g057
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          items:
> > +            - description: Error interrupt
>
> [...]
>
> These descriptions should be at the top level.  The SoC-specific rules
> should just restrict the lower limit (interrupts/minItems).
>
I think I'm misunderstanding here. As per patch 2/4 the DT maintainer
wants properties at top level with just minItems/maxItems and have SoC
specific listed in the checks (as pointed out to me like [0])

[0] https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L48

> > +
> > +        interrupt-names:
> > +          items:
> > +            - const: eri
>
> [...]
>
> Likewise.
>
> In addition, you should restrict clocks/maxItems to 1, as on RZ/V2H
> only the UART functional clock is supported.
>
Agreed.

Cheers,
Prabhakar
Rob Herring (Arm) March 20, 2024, 2:37 p.m. UTC | #3 
On Mon, Mar 18, 2024 at 05:21:01PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Document support for the Serial Communication Interface with FIFO (SCIF)
> available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in
> the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L
> (R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has
> three additional interrupts: one for Tx end/Rx ready and the other two for
> Rx and Tx buffer full, which are edge-triggered.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Added SoC specific compat string
> ---
>  .../bindings/serial/renesas,scif.yaml         | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> index 53f18e9810fd..e4ce13e20cd7 100644
> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> @@ -79,6 +79,8 @@ properties:
>                - renesas,scif-r9a08g045      # RZ/G3S
>            - const: renesas,scif-r9a07g044   # RZ/G2{L,LC} fallback
>  
> +      - const: renesas,scif-r9a09g057       # RZ/V2H(P)

I don't understand why there's not a fallback. Looks like the existing 
driver would work if you had one. It should be fine to ignore the new 
interrupts. Though with Geert's comments, it seems there are more 
differences than you say. 

Rob
Lad, Prabhakar March 21, 2024, 10:48 a.m. UTC | #4 
Hi Rob,

Thank you for the review.

On Wed, Mar 20, 2024 at 2:37 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 05:21:01PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Document support for the Serial Communication Interface with FIFO (SCIF)
> > available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in
> > the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L
> > (R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has
> > three additional interrupts: one for Tx end/Rx ready and the other two for
> > Rx and Tx buffer full, which are edge-triggered.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > - Added SoC specific compat string
> > ---
> >  .../bindings/serial/renesas,scif.yaml         | 33 +++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > index 53f18e9810fd..e4ce13e20cd7 100644
> > --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > @@ -79,6 +79,8 @@ properties:
> >                - renesas,scif-r9a08g045      # RZ/G3S
> >            - const: renesas,scif-r9a07g044   # RZ/G2{L,LC} fallback
> >
> > +      - const: renesas,scif-r9a09g057       # RZ/V2H(P)
>
> I don't understand why there's not a fallback. Looks like the existing
> driver would work if you had one. It should be fine to ignore the new
> interrupts. Though with Geert's comments, it seems there are more
> differences than you say.
>
Apart from the interrupt differences there are some register bit
differences too (as pointed by Geert in patch 4/4).

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
index 53f18e9810fd..e4ce13e20cd7 100644
--- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
+++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
@@ -79,6 +79,8 @@  properties:
               - renesas,scif-r9a08g045      # RZ/G3S
           - const: renesas,scif-r9a07g044   # RZ/G2{L,LC} fallback
 
+      - const: renesas,scif-r9a09g057       # RZ/V2H(P)
+
   reg:
     maxItems: 1
 
@@ -204,6 +206,37 @@  allOf:
             - const: dri
             - const: tei
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,scif-r9a09g057
+    then:
+      properties:
+        interrupts:
+          items:
+            - description: Error interrupt
+            - description: Receive buffer full interrupt
+            - description: Transmit buffer empty interrupt
+            - description: Break interrupt
+            - description: Data Ready interrupt
+            - description: Transmit End interrupt
+            - description: Transmit End/Data Ready interrupt
+            - description: Receive buffer full interrupt (EDGE trigger)
+            - description: Transmit buffer empty interrupt (EDGE trigger)
+
+        interrupt-names:
+          items:
+            - const: eri
+            - const: rxi
+            - const: txi
+            - const: bri
+            - const: dri
+            - const: tei
+            - const: tei-dri
+            - const: rxi-edge
+            - const: txi-edge
+
 unevaluatedProperties: false
 
 examples: