From: Lad Prabhakar <[email protected]>
Hi All,
This patch series updates DT binding doc and scif driver to add support
for the Renesas RZ/V2H(P) SoC. RZ/V2H(P) SoC supports one channel SCIF
interface.
v2->v3
- Included DT validation patches
- Added a new compat string for RZ/V2H(P) SoC
- Added driver changes for RZ/V2H(P) SoC
- Listed interrupts and interrupt-names for every SoC in if check
Cheers,
Prabhakar
Lad Prabhakar (4):
dt-bindings: serial: renesas,scif: Move ref for serial.yaml at the end
dt-bindings: serial: renesas,scif: Validate 'interrupts' and
'interrupt-names'
dt-bindings: serial: renesas,scif: Document R9A09G057 support
serial: sh-sci: Add support for RZ/V2H(P) SoC
.../bindings/serial/renesas,scif.yaml | 158 ++++++++++++------
drivers/tty/serial/sh-sci.c | 7 +-
2 files changed, 117 insertions(+), 48 deletions(-)
--
2.34.1
From: Lad Prabhakar <[email protected]>
In preparation for adding more validation checks move the ref for
'serial.yaml' to the end and also move reset check in 'allOf' block.
Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
v2->v3
- no change
---
.../bindings/serial/renesas,scif.yaml | 30 +++++++++----------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
index 4610a5bd580c..af72c3420453 100644
--- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
+++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
@@ -9,9 +9,6 @@ title: Renesas Serial Communication Interface with FIFO (SCIF)
maintainers:
- Geert Uytterhoeven <[email protected]>
-allOf:
- - $ref: serial.yaml#
-
properties:
compatible:
oneOf:
@@ -160,18 +157,21 @@ required:
- clock-names
- power-domains
-if:
- properties:
- compatible:
- contains:
- enum:
- - renesas,rcar-gen2-scif
- - renesas,rcar-gen3-scif
- - renesas,rcar-gen4-scif
- - renesas,scif-r9a07g044
-then:
- required:
- - resets
+allOf:
+ - $ref: serial.yaml#
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - renesas,rcar-gen2-scif
+ - renesas,rcar-gen3-scif
+ - renesas,rcar-gen4-scif
+ - renesas,scif-r9a07g044
+ then:
+ required:
+ - resets
unevaluatedProperties: false
--
2.34.1
From: Lad Prabhakar <[email protected]>
Add support to validate the 'interrupts' and 'interrupt-names' properties
for every supported SoC. This ensures proper handling and configuration of
interrupt-related properties across supported platforms.
Signed-off-by: Lad Prabhakar <[email protected]>
---
v2->v3
- Listed interrupts and interrupt-names for every SoC in if check
---
.../bindings/serial/renesas,scif.yaml | 95 ++++++++++++-------
1 file changed, 63 insertions(+), 32 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
index af72c3420453..53f18e9810fd 100644
--- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
+++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
@@ -82,38 +82,6 @@ properties:
reg:
maxItems: 1
- interrupts:
- oneOf:
- - items:
- - description: A combined interrupt
- - items:
- - description: Error interrupt
- - description: Receive buffer full interrupt
- - description: Transmit buffer empty interrupt
- - description: Break interrupt
- - 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
-
- interrupt-names:
- oneOf:
- - items:
- - const: eri
- - const: rxi
- - const: txi
- - const: bri
- - items:
- - const: eri
- - const: rxi
- - const: txi
- - const: bri
- - const: dri
- - const: tei
-
clocks:
minItems: 1
maxItems: 4
@@ -173,6 +141,69 @@ allOf:
required:
- resets
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - renesas,rcar-gen1-scif
+ - renesas,rcar-gen2-scif
+ - renesas,rcar-gen3-scif
+ - renesas,rcar-gen4-scif
+ then:
+ properties:
+ interrupts:
+ items:
+ - description: Single combined interrupt
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: renesas,scif-r7s72100
+ then:
+ properties:
+ interrupts:
+ items:
+ - description: Error interrupt
+ - description: Receive buffer full interrupt
+ - description: Transmit buffer empty interrupt
+ - description: Break interrupt
+
+ interrupt-names:
+ items:
+ - const: eri
+ - const: rxi
+ - const: txi
+ - const: bri
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - renesas,scif-r7s9210
+ - renesas,scif-r9a07g044
+ 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
+
+ interrupt-names:
+ items:
+ - const: eri
+ - const: rxi
+ - const: txi
+ - const: bri
+ - const: dri
+ - const: tei
+
unevaluatedProperties: false
examples:
--
2.34.1
From: Lad Prabhakar <[email protected]>
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 <[email protected]>
---
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)
+
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:
--
2.34.1
From: Lad Prabhakar <[email protected]>
Add serial support for RZ/V2H(P) SoC with earlycon.
Signed-off-by: Lad Prabhakar <[email protected]>
---
v2 - > v3
- new patch
---
drivers/tty/serial/sh-sci.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index a85e7b9a2e49..4a60d77257d6 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -290,7 +290,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
},
/*
- * The "SCIFA" that is in RZ/A2, RZ/G2L and RZ/T.
+ * The "SCIFA" that is in RZ/A2, RZ/G2L, RZ/T and RZ/V2H.
* It looks like a normal SCIF with FIFO data, but with a
* compressed address space. Also, the break out of interrupts
* are different: ERI/BRI, RXI, TXI, TEI, DRI.
@@ -3224,6 +3224,10 @@ static const struct of_device_id of_sci_match[] __maybe_unused = {
.compatible = "renesas,scif-r9a07g044",
.data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
},
+ {
+ .compatible = "renesas,scif-r9a09g057",
+ .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
+ },
/* Family-specific types */
{
.compatible = "renesas,rcar-gen1-scif",
@@ -3554,6 +3558,7 @@ OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
+OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a09g057", rzscifa_early_console_setup);
OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);
--
2.34.1
On 18/03/2024 18:21, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Add support to validate the 'interrupts' and 'interrupt-names' properties
> for every supported SoC. This ensures proper handling and configuration of
> interrupt-related properties across supported platforms.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> v2->v3
> - Listed interrupts and interrupt-names for every SoC in if check
> ---
> .../bindings/serial/renesas,scif.yaml | 95 ++++++++++++-------
> 1 file changed, 63 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> index af72c3420453..53f18e9810fd 100644
> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> @@ -82,38 +82,6 @@ properties:
> reg:
> maxItems: 1
>
> - interrupts:
I don't understand what is happening with this patchset. Interrupts must
stay here. Where did you receive any different feedback?
Best regards,
Krzysztof
On 19/03/2024 07:19, Krzysztof Kozlowski wrote:
> On 18/03/2024 18:21, Prabhakar wrote:
>> From: Lad Prabhakar <[email protected]>
>>
>> Add support to validate the 'interrupts' and 'interrupt-names' properties
>> for every supported SoC. This ensures proper handling and configuration of
>> interrupt-related properties across supported platforms.
>>
>> Signed-off-by: Lad Prabhakar <[email protected]>
>> ---
>> v2->v3
>> - Listed interrupts and interrupt-names for every SoC in if check
>> ---
>> .../bindings/serial/renesas,scif.yaml | 95 ++++++++++++-------
>> 1 file changed, 63 insertions(+), 32 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>> index af72c3420453..53f18e9810fd 100644
>> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>> @@ -82,38 +82,6 @@ properties:
>> reg:
>> maxItems: 1
>>
>> - interrupts:
>
> I don't understand what is happening with this patchset. Interrupts must
> stay here. Where did you receive any different feedback?
Look how it is done:
https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
Best regards,
Krzysztof
Hi Prabhakar,
On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> 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 <[email protected]>
> ---
> 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-m68korg
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 Mon, Mar 18, 2024 at 6:22 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> Add serial support for RZ/V2H(P) SoC with earlycon.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> v2 - > v3
> - new patch
Thanks for your patch!
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -290,7 +290,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
> },
>
> /*
> - * The "SCIFA" that is in RZ/A2, RZ/G2L and RZ/T.
> + * The "SCIFA" that is in RZ/A2, RZ/G2L, RZ/T and RZ/V2H.
> * It looks like a normal SCIF with FIFO data, but with a
> * compressed address space. Also, the break out of interrupts
> * are different: ERI/BRI, RXI, TXI, TEI, DRI.
and RZ/V2H has more interrupts than RZ/A1, RZ/G2L and RZ/T...
In addition, RZ/V2H does not support synchronous mode (does not matter
for the driver) and modem control signals.
Currently, sci_init_pins() does write ones to the SCPTR bits that are
reserved and marked as "write zero" on RZ/V2H. I am not sure how bad
that is. You could avoid that by adding a check for .hasrtscts, but
that may have impact on other SoCs/boards, where currently e.g. RTS#
is always programmed for output and active low...
So if you really need to avoid writing to these bits, the only safe
way may be to add a new SCIF type. But perhaps I'm over-cautious? ;-)
> @@ -3224,6 +3224,10 @@ static const struct of_device_id of_sci_match[] __maybe_unused = {
> .compatible = "renesas,scif-r9a07g044",
> .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> },
> + {
> + .compatible = "renesas,scif-r9a09g057",
> + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> + },
> /* Family-specific types */
> {
> .compatible = "renesas,rcar-gen1-scif",
> @@ -3554,6 +3558,7 @@ OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
> OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
> OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
> +OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a09g057", rzscifa_early_console_setup);
> OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
> OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
> OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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 Krzysztof,
On Tue, Mar 19, 2024 at 6:22 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 19/03/2024 07:19, Krzysztof Kozlowski wrote:
> > On 18/03/2024 18:21, Prabhakar wrote:
> >> From: Lad Prabhakar <[email protected]>
> >>
> >> Add support to validate the 'interrupts' and 'interrupt-names' properties
> >> for every supported SoC. This ensures proper handling and configuration of
> >> interrupt-related properties across supported platforms.
> >>
> >> Signed-off-by: Lad Prabhakar <[email protected]>
> >> ---
> >> v2->v3
> >> - Listed interrupts and interrupt-names for every SoC in if check
> >> ---
> >> .../bindings/serial/renesas,scif.yaml | 95 ++++++++++++-------
> >> 1 file changed, 63 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >> index af72c3420453..53f18e9810fd 100644
> >> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >> @@ -82,38 +82,6 @@ properties:
> >> reg:
> >> maxItems: 1
> >>
> >> - interrupts:
> >
> > I don't understand what is happening with this patchset. Interrupts must
> > stay here. Where did you receive any different feedback?
>
> Look how it is done:
> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
>
Thanks for the pointer, as the above binding doesn't have any
description items as compared to our case, to clarify I have updated
the binding is below. Is this the correct approach?
option #1
---------------
interrupts:
minItems: 1
maxItems: 6
interrupt-names:
minItems: 4
maxItems: 6
- if:
properties:
compatible:
contains:
enum:
- renesas,rcar-gen1-scif
- renesas,rcar-gen2-scif
- renesas,rcar-gen3-scif
- renesas,rcar-gen4-scif
then:
properties:
interrupts:
items:
- description: Single combined interrupt
interrupt-names: false
- if:
properties:
compatible:
contains:
const: renesas,scif-r7s72100
then:
properties:
interrupts:
items:
- description: Error interrupt
- description: Receive buffer full interrupt
- description: Transmit buffer empty interrupt
- description: Break interrupt
interrupt-names:
items:
- const: eri
- const: rxi
- const: txi
- const: bri
- if:
properties:
compatible:
contains:
enum:
- renesas,scif-r7s9210
- renesas,scif-r9a07g044
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
interrupt-names:
items:
- const: eri
- const: rxi
- const: txi
- const: bri
- const: dri
- const: tei
Cheers,
Prabhakar
Hi Geert,
Thank you for the review.
On Tue, Mar 19, 2024 at 8:12 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmailcom> wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
On 19/03/2024 13:43, Lad, Prabhakar wrote:
>>>> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>>>> index af72c3420453..53f18e9810fd 100644
>>>> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>>>> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>>>> @@ -82,38 +82,6 @@ properties:
>>>> reg:
>>>> maxItems: 1
>>>>
>>>> - interrupts:
>>>
>>> I don't understand what is happening with this patchset. Interrupts must
>>> stay here. Where did you receive any different feedback?
>>
>> Look how it is done:
>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
>>
> Thanks for the pointer, as the above binding doesn't have any
Yeah, that's just an example to point you the concept: top level
property comes with widest constraints (or widest matching items
description) and each variant narrows the choice.
> description items as compared to our case, to clarify I have updated
> the binding is below. Is this the correct approach?
>
> option #1
> ---------------
Yes, it looks correct.
Best regards,
Krzysztof
Hi Krzysztof,
On Tue, Mar 19, 2024 at 2:04 PM Krzysztof Kozlowski
<[email protected]> wrote:
> On 19/03/2024 13:43, Lad, Prabhakar wrote:
> >>>> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >>>> index af72c3420453..53f18e9810fd 100644
> >>>> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >>>> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >>>> @@ -82,38 +82,6 @@ properties:
> >>>> reg:
> >>>> maxItems: 1
> >>>>
> >>>> - interrupts:
> >>>
> >>> I don't understand what is happening with this patchset. Interrupts must
> >>> stay here. Where did you receive any different feedback?
> >>
> >> Look how it is done:
> >> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
> >>
> > Thanks for the pointer, as the above binding doesn't have any
>
> Yeah, that's just an example to point you the concept: top level
> property comes with widest constraints (or widest matching items
> description) and each variant narrows the choice.
>
> > description items as compared to our case, to clarify I have updated
> > the binding is below. Is this the correct approach?
> >
> > option #1
> > ---------------
>
>
> Yes, it looks correct.
Why duplicate all the descriptions? The only differences are the number
of valid interrupts?
What was wrong with "[PATCH v2 2/2] dt-bindings: serial: renesas,scif:
Validate 'interrupts' and 'interrupt-names'"[1]?
https://lore.kernel.org/r/[email protected]/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
On 19/03/2024 14:25, Geert Uytterhoeven wrote:
> Hi Krzysztof,
>
> On Tue, Mar 19, 2024 at 2:04 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 19/03/2024 13:43, Lad, Prabhakar wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>>>>>> index af72c3420453..53f18e9810fd 100644
>>>>>> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
>>>>>> @@ -82,38 +82,6 @@ properties:
>>>>>> reg:
>>>>>> maxItems: 1
>>>>>>
>>>>>> - interrupts:
>>>>>
>>>>> I don't understand what is happening with this patchset. Interrupts must
>>>>> stay here. Where did you receive any different feedback?
>>>>
>>>> Look how it is done:
>>>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
>>>>
>>> Thanks for the pointer, as the above binding doesn't have any
>>
>> Yeah, that's just an example to point you the concept: top level
>> property comes with widest constraints (or widest matching items
>> description) and each variant narrows the choice.
>>
>>> description items as compared to our case, to clarify I have updated
>>> the binding is below. Is this the correct approach?
>>>
>>> option #1
>>> ---------------
>>
>>
>> Yes, it looks correct.
>
> Why duplicate all the descriptions? The only differences are the number
> of valid interrupts?
> What was wrong with "[PATCH v2 2/2] dt-bindings: serial: renesas,scif:
> Validate 'interrupts' and 'interrupt-names'"[1]?
>
> https://lore.kernel.org/r/[email protected]/
I have impression that only two variants out of three have same
descriptions... but now I see mistake I made in above. I read that first
interrupt is "Error interrupt" but it is "error or combined". Sorry for
that, I think most of my comment there is not correct.
It could be made oneOf?
oneOf:
- items:
- description: A combined interrupt
- items:
- ....
minItems: 4
?
Best regards,
Krzysztof
Hi Krzysztof,
On Wed, Mar 20, 2024 at 9:06 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 19/03/2024 14:25, Geert Uytterhoeven wrote:
> > On Tue, Mar 19, 2024 at 2:04 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >> On 19/03/2024 13:43, Lad, Prabhakar wrote:
> >>>>>> diff --git a/Documentation/devicetree/bindings/serial/renesas,scifyaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >>>>>> index af72c3420453..53f18e9810fd 100644
> >>>>>> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> >>>>>> @@ -82,38 +82,6 @@ properties:
> >>>>>> reg:
> >>>>>> maxItems: 1
> >>>>>>
> >>>>>> - interrupts:
> >>>>>
> >>>>> I don't understand what is happening with this patchset. Interrupts must
> >>>>> stay here. Where did you receive any different feedback?
> >>>>
> >>>> Look how it is done:
> >>>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
> >>>>
> >>> Thanks for the pointer, as the above binding doesn't have any
> >>
> >> Yeah, that's just an example to point you the concept: top level
> >> property comes with widest constraints (or widest matching items
> >> description) and each variant narrows the choice.
> >>
> >>> description items as compared to our case, to clarify I have updated
> >>> the binding is below. Is this the correct approach?
> >>>
> >>> option #1
> >>> ---------------
> >>
> >>
> >> Yes, it looks correct.
> >
> > Why duplicate all the descriptions? The only differences are the number
> > of valid interrupts?
> > What was wrong with "[PATCH v2 2/2] dt-bindings: serial: renesas,scif:
> > Validate 'interrupts' and 'interrupt-names'"[1]?
> >
> > https://lore.kernel.org/r/[email protected]/
>
> I have impression that only two variants out of three have same
> descriptions... but now I see mistake I made in above. I read that first
> interrupt is "Error interrupt" but it is "error or combined". Sorry for
> that, I think most of my comment there is not correct.
>
> It could be made oneOf?
>
> oneOf:
> - items:
> - description: A combined interrupt
> - items:
> - ....
> minItems: 4
> ?
Yes, that would be even better.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
On Mon, Mar 18, 2024 at 05:21:01PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Hi Geert,
Thank you for the review.
On Tue, Mar 19, 2024 at 8:21 AM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmailcom> wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Add serial support for RZ/V2H(P) SoC with earlycon.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > v2 - > v3
> > - new patch
>
> Thanks for your patch!
>
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -290,7 +290,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
> > },
> >
> > /*
> > - * The "SCIFA" that is in RZ/A2, RZ/G2L and RZ/T.
> > + * The "SCIFA" that is in RZ/A2, RZ/G2L, RZ/T and RZ/V2H.
> > * It looks like a normal SCIF with FIFO data, but with a
> > * compressed address space. Also, the break out of interrupts
> > * are different: ERI/BRI, RXI, TXI, TEI, DRI.
>
> and RZ/V2H has more interrupts than RZ/A1, RZ/G2L and RZ/T...
>
> In addition, RZ/V2H does not support synchronous mode (does not matter
> for the driver) and modem control signals.
>
> Currently, sci_init_pins() does write ones to the SCPTR bits that are
> reserved and marked as "write zero" on RZ/V2H. I am not sure how bad
> that is. You could avoid that by adding a check for .hasrtscts, but
> that may have impact on other SoCs/boards, where currently e.g. RTS#
> is always programmed for output and active low...
>
Oops I had totally missed this difference, thanks for catching that.
> So if you really need to avoid writing to these bits, the only safe
> way may be to add a new SCIF type. But perhaps I'm over-cautious? ;-)
>
As we are adding a SoC specific compat string we can update this if we
see an issue, but I'm happy to do that change now too. Please let me
know what would you prefer.
Cheers,
Prabhakar
> > @@ -3224,6 +3224,10 @@ static const struct of_device_id of_sci_match[] __maybe_unused = {
> > .compatible = "renesas,scif-r9a07g044",
> > .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> > },
> > + {
> > + .compatible = "renesas,scif-r9a09g057",
> > + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> > + },
> > /* Family-specific types */
> > {
> > .compatible = "renesas,rcar-gen1-scif",
> > @@ -3554,6 +3558,7 @@ OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
> > OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
> > OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> > OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
> > +OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a09g057", rzscifa_early_console_setup);
> > OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
> > OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
> > OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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 Rob,
Thank you for the review.
On Wed, Mar 20, 2024 at 2:37 PM Rob Herring <[email protected]> wrote:
>
> On Mon, Mar 18, 2024 at 05:21:01PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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