[2/2] serial: sh-sci: Document r7s9210 bindings

Message ID 20180711144131.73552-3-chris.brandt@renesas.com
State Changes Requested
Headers show
Series
  • serial: sh-sci: Add support for RZ/A2
Related show

Commit Message

Chris Brandt July 11, 2018, 2:41 p.m.
Add R7S9210 (RZ/A2) support

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 1 +
 1 file changed, 1 insertion(+)

Comments

Geert Uytterhoeven July 13, 2018, 3:50 p.m. | #1
On Wed, Jul 11, 2018 at 4:42 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Add R7S9210 (RZ/A2) support
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven July 17, 2018, 8:35 a.m. | #2
Hi Chris,

On Fri, Jul 13, 2018 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Jul 11, 2018 at 4:42 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> > Add R7S9210 (RZ/A2) support
> >
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Sorry, I spoke too soon.
It seems the bindings were never updated for the use of multiple interrupts
on RZ/A1.  As RZ/A2 adds one new interrupt, can you please document which
interrupts are required?
Thanks!

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt July 18, 2018, 10:19 p.m. | #3
Hi Geert,

On Tuesday, July 17, 2018, Geert Uytterhoeven wrote:
> On Fri, Jul 13, 2018 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org>
> wrote:
> > On Wed, Jul 11, 2018 at 4:42 PM Chris Brandt <chris.brandt@renesas.com>
> wrote:
> > > Add R7S9210 (RZ/A2) support
> > >
> > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Sorry, I spoke too soon.
> It seems the bindings were never updated for the use of multiple
> interrupts
> on RZ/A1.  As RZ/A2 adds one new interrupt, can you please document which
> interrupts are required?
> Thanks!

The issue that I ran into was the device driver assumed some signals 
were muxed together (TXI and DRI), and that other signals were individual.

The existing driver wanted interrupts to be specified in this order:
  1. Error
  2. RX
  3. TX (assumes DRI)
  4. Break

However, for the SCIF that is present in the RZ/A2M, Error and Break are
muxed together, and then DRI is not muxed with TX. This is different 
than any other SCIF supported by the driver.

My solution was to list the Error/Break twice, and then add a new 
interrupt for DRI.

As reference, here is what the DT node would look like:

	scif0: serial@e8007000 {
		compatible = "renesas,scif-r7s9210", "renesas,scif";
		reg = <0xe8007000 18>;
		interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI0/BRI0 */
			     <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,   /* RXI0 */
			     <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>,   /* TXI0 */
			     <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,   /* ERI0/BRI0 */
			     <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;   /* TEI/DRI0 */
		clocks = <&mstp4_clks R7S9210_CLK_SCIF0>;
		clock-names = "fck";
		power-domains = <&cpg_clocks>;
		status = "disabled";
	};

Of course I have no problem documenting all this, but I first I just 
wanted to make sure I was not going to get push back when I submit a DT 
later that lists the same interrupt twice.

Thanks,
Chris
Chris Brandt July 18, 2018, 10:23 p.m. | #4
Hi Geert,

On Tuesday, July 17, 2018, Geert Uytterhoeven wrote:
> On Fri, Jul 13, 2018 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org>
> wrote:
> > On Wed, Jul 11, 2018 at 4:42 PM Chris Brandt <chris.brandt@renesas.com>
> wrote:
> > > Add R7S9210 (RZ/A2) support
> > >
> > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Sorry, I spoke too soon.
> It seems the bindings were never updated for the use of multiple
> interrupts
> on RZ/A1.  As RZ/A2 adds one new interrupt, can you please document which
> interrupts are required?
> Thanks!

The issue that I ran into was the device driver assumed some signals 
were muxed together (TXI and DRI), and that other signals were individual.

The existing driver wanted interrupts to be specified in this order:
  1. Error
  2. RX
  3. TX (assumes DRI)
  4. Break

However, for the SCIF that is present in the RZ/A2M, Error and Break are
muxed together, and then DRI is not muxed with TX. This is different 
than any other SCIF supported by the driver.

My solution was to list the Error/Break twice, and then add a new 
interrupt for DRI.

As reference, here is what the DT node would look like:

	scif0: serial@e8007000 {
		compatible = "renesas,scif-r7s9210", "renesas,scif";
		reg = <0xe8007000 18>;
		interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI0/BRI0 */
			     <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,   /* RXI0 */
			     <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>,   /* TXI0 */
			     <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,   /* ERI0/BRI0 */
			     <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;   /* TEI/DRI0 */
		clocks = <&mstp4_clks R7S9210_CLK_SCIF0>;
		clock-names = "fck";
		power-domains = <&cpg_clocks>;
		status = "disabled";
	};

Of course I have no problem documenting all this, but I first I just 
wanted to make sure I was not going to get push back when I submit a DT 
later that lists the same interrupt twice.

Thanks,
Chris
Geert Uytterhoeven July 19, 2018, 8:13 a.m. | #5
Hi Chris,

On Thu, Jul 19, 2018 at 12:19 AM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tuesday, July 17, 2018, Geert Uytterhoeven wrote:
> > On Fri, Jul 13, 2018 at 5:50 PM Geert Uytterhoeven <geert@linux-m68k.org>
> > wrote:
> > > On Wed, Jul 11, 2018 at 4:42 PM Chris Brandt <chris.brandt@renesas.com>
> > wrote:
> > > > Add R7S9210 (RZ/A2) support
> > > >
> > > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Sorry, I spoke too soon.
> > It seems the bindings were never updated for the use of multiple
> > interrupts
> > on RZ/A1.  As RZ/A2 adds one new interrupt, can you please document which
> > interrupts are required?
> > Thanks!
>
> The issue that I ran into was the device driver assumed some signals
> were muxed together (TXI and DRI), and that other signals were individual.
>
> The existing driver wanted interrupts to be specified in this order:
>   1. Error
>   2. RX
>   3. TX (assumes DRI)
>   4. Break

Yes, that matches the RZ/A1 SCIFA and interrupt controller docs.

> However, for the SCIF that is present in the RZ/A2M, Error and Break are
> muxed together, and then DRI is not muxed with TX. This is different
> than any other SCIF supported by the driver.

Right, the RZ/A2 SCIFA variant is documented to provide 6 interrupt sources:
  1. TEI,
  2. TXI,
  3. RXI,
  4. DRI,
  5. ERI,
  6. BRI.

> My solution was to list the Error/Break twice, and then add a new
> interrupt for DRI.
>
> As reference, here is what the DT node would look like:
>
>         scif0: serial@e8007000 {
>                 compatible = "renesas,scif-r7s9210", "renesas,scif";
>                 reg = <0xe8007000 18>;
>                 interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI0/BRI0 */
>                              <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>,   /* RXI0 */
>                              <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>,   /* TXI0 */
>                              <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>,   /* ERI0/BRI0 */
>                              <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;   /* TEI/DRI0 */
>                 clocks = <&mstp4_clks R7S9210_CLK_SCIF0>;
>                 clock-names = "fck";
>                 power-domains = <&cpg_clocks>;
>                 status = "disabled";
>         };
>
> Of course I have no problem documenting all this, but I first I just
> wanted to make sure I was not going to get push back when I submit a DT
> later that lists the same interrupt twice.

Listing them twice does make sense to me, as the interrupt controller
source list in the RZ/A2 docs has only four, and explicitly lists how they are
multiplexed:
  base + 0 = ERI/BRI,
  base + 1 = RXI,
  base + 2 = TXI,
  base + 3 = TEI/DRI.
But future SoCS with the same SCIFA variant may wire them differently?

For DT backwards compatibility, we have to keep support for the following
2 schemes:
  1. Single "interrupts" value, no "interrupt-names", for fully multiplexed
     interrupts (SH/R-Mobile, R-Car).
  2. Four "interrupts" values, no "interrupt-names", for ERI/RXI/TXI/TEI
     (RZ/A1, H8/300).

For RZ/A2, I suggest extending the bindings with interrupt-names,
documenting all 6 interrupt sources, and let the driver handle that.
That means there should be 6, not 5, "interrupts" values.
Whether the driver implements all possible combinations, or only what you
need for RZ/A2, is up to you. I agree the interrupt handling in the driver
is already sufficiently complex.
Ideally, you would document support for RZ/A1 with interrupt-names too,
and handle that as well.

Does this make sense?
Thanks!


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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven July 20, 2018, 8:16 a.m. | #6
Hi Chris,

On Thu, Jul 19, 2018 at 2:59 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Thursday, July 19, 2018, Geert Uytterhoeven wrote:
> > > The issue that I ran into was the device driver assumed some signals
> > > were muxed together (TXI and DRI), and that other signals were
> > individual.
> > >
> > > The existing driver wanted interrupts to be specified in this order:
> > >   1. Error
> > >   2. RX
> > >   3. TX (assumes DRI)
> > >   4. Break
>
> First, sorry for my mis-type.
> DRI mean 'Data Ready Interrupt' and for SCIF it is normally muxed with RX (not TX).

OK.

> > > Of course I have no problem documenting all this, but I first I just
> > > wanted to make sure I was not going to get push back when I submit a DT
> > > later that lists the same interrupt twice.
> >
> > Listing them twice does make sense to me, as the interrupt controller
> > source list in the RZ/A2 docs has only four, and explicitly lists how they
> > are
> > multiplexed:
> >   base + 0 = ERI/BRI,
> >   base + 1 = RXI,
> >   base + 2 = TXI,
> >   base + 3 = TEI/DRI.
> > But future SoCS with the same SCIFA variant may wire them differently?
>
> This SCIF seems to be related to ones used in H8S devices. It's also been
> used in RX and RZ/T devices. So I think the order seems to be stable.
>
>
> > For DT backwards compatibility, we have to keep support for the following
> > 2 schemes:
> >   1. Single "interrupts" value, no "interrupt-names", for fully
> > multiplexed
> >      interrupts (SH/R-Mobile, R-Car).
> >   2. Four "interrupts" values, no "interrupt-names", for ERI/RXI/TXI/TEI
> >      (RZ/A1, H8/300).
> >
> > For RZ/A2, I suggest extending the bindings with interrupt-names,
> > documenting all 6 interrupt sources, and let the driver handle that.
> > That means there should be 6, not 5, "interrupts" values.
> > Whether the driver implements all possible combinations, or only what you
> > need for RZ/A2, is up to you. I agree the interrupt handling in the driver
> > is already sufficiently complex.
> > Ideally, you would document support for RZ/A1 with interrupt-names too,
> > and handle that as well.
> >
> > Does this make sense?
>
> What about this idea:
> Since we can't break any old DTs, what if we just say that you simply
> list all the interrupts in DT, and the driver just registers all those
> vectors with the same ISR function (sci_mpxed_interrupt) and process
> everything the same way R-Car devices do with their single interrupt.
>
> For this class of device, having separate interrupt vectors probably
> doesn't buy you that much extra performance.
>
> The driver could also check to see if "interrupt-names" was specified,
> and if it was, the driver could simply use those names when registering
> the interrupts so everything shows up nicely in /proc/interrupts.
>
> What's your thoughts on that idea?

I don't know if using the same handler for all separate interrupts won't cause
problems. What if two interrupts fire, the first one calls the common handler,
handles both, and the second one returns IRQ_NONE? Spurious interrupts
detected by the core interrupt, leading to "Disabling IRQ #n" once it has seen
too many of them?

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
index 106808b55b6d..5c002bce8f42 100644
--- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
+++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
@@ -5,6 +5,7 @@  Required properties:
   - compatible: Must contain one or more of the following:
 
     - "renesas,scif-r7s72100" for R7S72100 (RZ/A1H) SCIF compatible UART.
+    - "renesas,scif-r7s9210" for R7S9210 (RZ/A2) SCIF compatible UART.
     - "renesas,scifa-r8a73a4" for R8A73A4 (R-Mobile APE6) SCIFA compatible UART.
     - "renesas,scifb-r8a73a4" for R8A73A4 (R-Mobile APE6) SCIFB compatible UART.
     - "renesas,scifa-r8a7740" for R8A7740 (R-Mobile A1) SCIFA compatible UART.