* [PATCH 0/2] serial: sh-sci: Add support for RZ/A2 @ 2018-07-11 14:41 Chris Brandt 2018-07-11 14:41 ` [PATCH 1/2] serial: sh-sci: Add support for R7S9210 Chris Brandt 2018-07-11 14:41 ` [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings Chris Brandt 0 siblings, 2 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-11 14:41 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland Cc: linux-serial, devicetree, linux-renesas-soc, Simon Horman, Geert Uytterhoeven, Chris Brandt The RZ/A2 uses a modified SCIF that until recently was only used in Renesas MCU devices (not MPU devices). So, while it functions mostly the same as a normal SCIF, some things needed to be shifted around. Chris Brandt (2): serial: sh-sci: Add support for R7S9210 serial: sh-sci: Document r7s9210 bindings .../bindings/serial/renesas,sci-serial.txt | 1 + drivers/tty/serial/sh-sci.c | 77 +++++++++++++++++++++- include/linux/serial_sci.h | 1 + 3 files changed, 77 insertions(+), 2 deletions(-) -- 2.16.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/2] serial: sh-sci: Add support for R7S9210 2018-07-11 14:41 [PATCH 0/2] serial: sh-sci: Add support for RZ/A2 Chris Brandt @ 2018-07-11 14:41 ` Chris Brandt 2018-07-20 8:09 ` Geert Uytterhoeven 2018-07-11 14:41 ` [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings Chris Brandt 1 sibling, 1 reply; 22+ messages in thread From: Chris Brandt @ 2018-07-11 14:41 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland Cc: linux-serial, devicetree, linux-renesas-soc, Simon Horman, Geert Uytterhoeven, Chris Brandt Add support for a "RZ_SCIFA" which is different than a traditional SCIFA. It looks like a normal SCIF with FIFO data, but with a compressed address space. Also, the break out of interrupts are different then traditinal SCIF: ERI/BRI, RXI, TXI, TEI, DRI. The R7S9210 (RZ/A2) contains this type of SCIF. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/tty/serial/sh-sci.c | 77 +++++++++++++++++++++++++++++++++++++++++++-- include/linux/serial_sci.h | 1 + 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index c181eb37f985..d3435976ba7e 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -65,6 +65,7 @@ enum { SCIx_RXI_IRQ, SCIx_TXI_IRQ, SCIx_BRI_IRQ, + SCIx_TEIDRI_IRQ, SCIx_NR_IRQS, SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ @@ -76,6 +77,9 @@ enum { ((port)->irqs[SCIx_ERI_IRQ] && \ ((port)->irqs[SCIx_RXI_IRQ] < 0)) +#define SCIx_TEIDRI_IRQ_EXISTS(port) \ + ((port)->irqs[SCIx_TEIDRI_IRQ] > 0) + enum SCI_CLKS { SCI_FCK, /* Functional Clock */ SCI_SCK, /* Optional External Clock */ @@ -287,6 +291,33 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { .error_clear = SCIF_ERROR_CLEAR, }, + /* + * The "SCIFA" that is in RZ/T and RZ/A2. + * 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. + */ + [SCIx_RZ_SCIFA_REGTYPE] = { + .regs = { + [SCSMR] = { 0x00, 16 }, + [SCBRR] = { 0x02, 8 }, + [SCSCR] = { 0x04, 16 }, + [SCxTDR] = { 0x06, 8 }, + [SCxSR] = { 0x08, 16 }, + [SCxRDR] = { 0x0A, 8 }, + [SCFCR] = { 0x0C, 16 }, + [SCFDR] = { 0x0E, 16 }, + [SCSPTR] = { 0x10, 16 }, + [SCLSR] = { 0x12, 16 }, + }, + .fifosize = 16, + .overrun_reg = SCLSR, + .overrun_mask = SCLSR_ORER, + .sampling_rate_mask = SCI_SR(32), + .error_mask = SCIF_DEFAULT_ERROR_MASK, + .error_clear = SCIF_ERROR_CLEAR, + }, + /* * Common SH-3 SCIF definitions. */ @@ -1683,11 +1714,26 @@ static irqreturn_t sci_tx_interrupt(int irq, void *ptr) return IRQ_HANDLED; } +static irqreturn_t sci_br_interrupt(int irq, void *ptr); + static irqreturn_t sci_er_interrupt(int irq, void *ptr) { struct uart_port *port = ptr; struct sci_port *s = to_sci_port(port); + if (SCIx_TEIDRI_IRQ_EXISTS(s)) { + /* Break and Error interrupts are muxed */ + unsigned short ssr_status = serial_port_in(port, SCxSR); + + /* Break Interrupt */ + if (ssr_status & SCxSR_BRK(port)) + sci_br_interrupt(irq, ptr); + + /* Break only? */ + if (!(ssr_status & SCxSR_ERRORS(port))) + return IRQ_HANDLED; + } + /* Handle errors */ if (port->type == PORT_SCI) { if (sci_handle_errors(port)) { @@ -1827,8 +1873,31 @@ static int sci_request_irq(struct sci_port *port) } desc = sci_irq_desc + i; - port->irqstr[j] = kasprintf(GFP_KERNEL, "%s:%s", - dev_name(up->dev), desc->desc); + if (SCIx_TEIDRI_IRQ_EXISTS(port)) { + /* + * ERI and BRI are muxed, just register ERI and + * ignore BRI. + * TEI and DRI are muxed, but only DRI + * is enabled, so use RXI handler + */ + if (i == SCIx_ERI_IRQ) + port->irqstr[j] = kasprintf(GFP_KERNEL, + "%s:err + break", + dev_name(up->dev)); + if (i == SCIx_BRI_IRQ) + continue; + if (i == SCIx_TEIDRI_IRQ) { + port->irqstr[j] = kasprintf(GFP_KERNEL, + "%s:tx end + rx ready", + dev_name(up->dev)); + desc = sci_irq_desc + SCIx_RXI_IRQ; + } + } + + if (!port->irqstr[j]) + port->irqstr[j] = kasprintf(GFP_KERNEL, "%s:%s", + dev_name(up->dev), + desc->desc); if (!port->irqstr[j]) { ret = -ENOMEM; goto out_nomem; @@ -3073,6 +3142,10 @@ static const struct of_device_id of_sci_match[] = { .compatible = "renesas,scif-r7s72100", .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH2_SCIF_FIFODATA_REGTYPE), }, + { + .compatible = "renesas,scif-r7s9210", + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), + }, /* Family-specific types */ { .compatible = "renesas,rcar-gen1-scif", diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h index c0e795d95477..1c89611e0e06 100644 --- a/include/linux/serial_sci.h +++ b/include/linux/serial_sci.h @@ -36,6 +36,7 @@ enum { SCIx_SH4_SCIF_FIFODATA_REGTYPE, SCIx_SH7705_SCIF_REGTYPE, SCIx_HSCIF_REGTYPE, + SCIx_RZ_SCIFA_REGTYPE, SCIx_NR_REGTYPES, }; -- 2.16.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] serial: sh-sci: Add support for R7S9210 2018-07-11 14:41 ` [PATCH 1/2] serial: sh-sci: Add support for R7S9210 Chris Brandt @ 2018-07-20 8:09 ` Geert Uytterhoeven 0 siblings, 0 replies; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-20 8:09 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Chris, On Wed, Jul 11, 2018 at 4:42 PM Chris Brandt <chris.brandt@renesas.com> wrote: > Add support for a "RZ_SCIFA" which is different than a traditional > SCIFA. It looks like a normal SCIF with FIFO data, but with a > compressed address space. Also, the break out of interrupts > are different then traditinal SCIF: ERI/BRI, RXI, TXI, TEI, DRI. > The R7S9210 (RZ/A2) contains this type of SCIF. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -65,6 +65,7 @@ enum { > SCIx_RXI_IRQ, > SCIx_TXI_IRQ, > SCIx_BRI_IRQ, > + SCIx_TEIDRI_IRQ, Why not separate enum values for TEI and DRI? According to the RZ/A2 docs, there are 6 separate interrupts, but they are multiplexed at the interrupt controller level. > SCIx_NR_IRQS, If you add interrupt value heres, you have to update the comment in sci_init_single(), which talks about "three or four" interrupts. sci_init_single() also fills in all unused sci_port->irqs[], so that code must be updated for the new interrupts. > > SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ > @@ -76,6 +77,9 @@ enum { > ((port)->irqs[SCIx_ERI_IRQ] && \ > ((port)->irqs[SCIx_RXI_IRQ] < 0)) > > +#define SCIx_TEIDRI_IRQ_EXISTS(port) \ > + ((port)->irqs[SCIx_TEIDRI_IRQ] > 0) > + > enum SCI_CLKS { > SCI_FCK, /* Functional Clock */ > SCI_SCK, /* Optional External Clock */ > @@ -287,6 +291,33 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { > .error_clear = SCIF_ERROR_CLEAR, > }, > > + /* > + * The "SCIFA" that is in RZ/T and RZ/A2. > + * 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. > + */ > + [SCIx_RZ_SCIFA_REGTYPE] = { > + .regs = { > + [SCSMR] = { 0x00, 16 }, > + [SCBRR] = { 0x02, 8 }, > + [SCSCR] = { 0x04, 16 }, > + [SCxTDR] = { 0x06, 8 }, > + [SCxSR] = { 0x08, 16 }, > + [SCxRDR] = { 0x0A, 8 }, > + [SCFCR] = { 0x0C, 16 }, > + [SCFDR] = { 0x0E, 16 }, > + [SCSPTR] = { 0x10, 16 }, > + [SCLSR] = { 0x12, 16 }, > + }, > + .fifosize = 16, > + .overrun_reg = SCLSR, > + .overrun_mask = SCLSR_ORER, > + .sampling_rate_mask = SCI_SR(32), > + .error_mask = SCIF_DEFAULT_ERROR_MASK, > + .error_clear = SCIF_ERROR_CLEAR, > + }, "Compressed addres space", like SCIx_SH4_SCIF_REGTYPE with regshift -1? ;-) Perhaps we should adjust all offsets in SCIx_SH4_SCIF_REGTYPE and let the current parts use regshift 1, so RZ/A2 can use the same definition with regshift 0? For DT systems, the regshift value could be stored in of_device_id.data by adjusting the SCI_OF_DATA() macro. For legacy SH, probably lots of platform data must be updated. > + > /* > * Common SH-3 SCIF definitions. > */ > @@ -1683,11 +1714,26 @@ static irqreturn_t sci_tx_interrupt(int irq, void *ptr) > return IRQ_HANDLED; > } > > +static irqreturn_t sci_br_interrupt(int irq, void *ptr); > + For interrupts, please see my response to the DT binding patch. 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-11 14:41 [PATCH 0/2] serial: sh-sci: Add support for RZ/A2 Chris Brandt 2018-07-11 14:41 ` [PATCH 1/2] serial: sh-sci: Add support for R7S9210 Chris Brandt @ 2018-07-11 14:41 ` Chris Brandt 2018-07-13 15:50 ` Geert Uytterhoeven 1 sibling, 1 reply; 22+ messages in thread From: Chris Brandt @ 2018-07-11 14:41 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland Cc: linux-serial, devicetree, linux-renesas-soc, Simon Horman, Geert Uytterhoeven, Chris Brandt 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(+) 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. -- 2.16.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-11 14:41 ` [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings Chris Brandt @ 2018-07-13 15:50 ` Geert Uytterhoeven 2018-07-17 8:35 ` Geert Uytterhoeven 0 siblings, 1 reply; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-13 15:50 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman 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 -- 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-13 15:50 ` Geert Uytterhoeven @ 2018-07-17 8:35 ` Geert Uytterhoeven 2018-07-17 13:43 ` Chris Brandt ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-17 8:35 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-17 8:35 ` Geert Uytterhoeven @ 2018-07-17 13:43 ` Chris Brandt 2018-07-18 22:19 ` Chris Brandt 2018-07-18 22:23 ` Chris Brandt 2 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-17 13:43 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Linux-Renesas, Simon Horman Hi Geert, On Tuesday, July 17, 2018, Geert Uytterhoeven wrote: > 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! OK. I will sent and updated patch in a bit. Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings @ 2018-07-17 13:43 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-17 13:43 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Geert, On Tuesday, July 17, 2018, Geert Uytterhoeven wrote: > 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! OK. I will sent and updated patch in a bit. Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-17 8:35 ` Geert Uytterhoeven @ 2018-07-18 22:19 ` Chris Brandt 2018-07-18 22:19 ` Chris Brandt 2018-07-18 22:23 ` Chris Brandt 2 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-18 22:19 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Linux-Renesas, Simon Horman 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings @ 2018-07-18 22:19 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-18 22:19 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-18 22:19 ` Chris Brandt (?) @ 2018-07-19 8:13 ` Geert Uytterhoeven 2018-07-19 12:58 ` Chris Brandt 2018-07-25 1:37 ` Chris Brandt -1 siblings, 2 replies; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-19 8:13 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-19 8:13 ` Geert Uytterhoeven @ 2018-07-19 12:58 ` Chris Brandt 2018-07-25 1:37 ` Chris Brandt 1 sibling, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-19 12:58 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Linux-Renesas, Simon Horman Hi Geert, 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). > > 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? Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings @ 2018-07-19 12:58 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-19 12:58 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Geert, 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). > > 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? Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-19 12:58 ` Chris Brandt (?) @ 2018-07-20 8:16 ` Geert Uytterhoeven -1 siblings, 0 replies; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-20 8:16 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman 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 -- 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-19 8:13 ` Geert Uytterhoeven @ 2018-07-25 1:37 ` Chris Brandt 2018-07-25 1:37 ` Chris Brandt 1 sibling, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-25 1:37 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Linux-Renesas, Simon Horman Hi Geert, I have one last clarification about your idea regarding documenting the interrupts separately for RZ/A2. On Thursday, July 19, 2018, Geert Uytterhoeven wrote: > 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. So for backward compatibility, no existing DT or setup-xxx.c (SH) file should change and the driver needs to assume the order of interrupts. However, if "interrupt-names" is specified in DT, then the driver determines what the interrupt are based on their names, not the order in which they are listed. Correct? So for example, RZ/A1 DT will stay the same and since "interrupt-names" was never used and the interrupt order will be assumed as ERI/RXI/TXI/BRI. RZ/A1 [ r7s72100.dtsi ] scif0: serial@e8007000 { compatible = "renesas,scif-r7s72100", "renesas,scif"; reg = <0xe8007000 64>; interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp4_clks R7S72100_CLK_SCIF0>; clock-names = "fck"; power-domains = <&cpg_clocks>; status = "disabled"; }; However, for RZ/A2, "interrupt-names" will be used and then the driver will sort things out however it needs to (figuring out what signals are muxed together an such). RZ/A2 [ r7s9210.dtsi ] scif0: serial@e8007000 { compatible = "renesas,scif-r7s9210", "renesas,scif"; reg = <0xe8007000 18>; interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI/BRI */ <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>, /* RXI */ <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>, /* TXI */ <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI/BRI */ <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>, /* TEI/DRI */ <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>; /* TEI/DRI */ interrupt-names = "eri", "rxi", "txi", "bri", "tei", "dri"; clocks = <&mstp4_clks R7S9210_CLK_SCIF0>; clock-names = "fck"; power-domains = <&cpg_clocks>; status = "disabled"; }; Are we on the same page? Thanks! Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings @ 2018-07-25 1:37 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-25 1:37 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Geert, I have one last clarification about your idea regarding documenting the interrupts separately for RZ/A2. On Thursday, July 19, 2018, Geert Uytterhoeven wrote: > 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. So for backward compatibility, no existing DT or setup-xxx.c (SH) file should change and the driver needs to assume the order of interrupts. However, if "interrupt-names" is specified in DT, then the driver determines what the interrupt are based on their names, not the order in which they are listed. Correct? So for example, RZ/A1 DT will stay the same and since "interrupt-names" was never used and the interrupt order will be assumed as ERI/RXI/TXI/BRI. RZ/A1 [ r7s72100.dtsi ] scif0: serial@e8007000 { compatible = "renesas,scif-r7s72100", "renesas,scif"; reg = <0xe8007000 64>; interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp4_clks R7S72100_CLK_SCIF0>; clock-names = "fck"; power-domains = <&cpg_clocks>; status = "disabled"; }; However, for RZ/A2, "interrupt-names" will be used and then the driver will sort things out however it needs to (figuring out what signals are muxed together an such). RZ/A2 [ r7s9210.dtsi ] scif0: serial@e8007000 { compatible = "renesas,scif-r7s9210", "renesas,scif"; reg = <0xe8007000 18>; interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI/BRI */ <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>, /* RXI */ <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>, /* TXI */ <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI/BRI */ <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>, /* TEI/DRI */ <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>; /* TEI/DRI */ interrupt-names = "eri", "rxi", "txi", "bri", "tei", "dri"; clocks = <&mstp4_clks R7S9210_CLK_SCIF0>; clock-names = "fck"; power-domains = <&cpg_clocks>; status = "disabled"; }; Are we on the same page? Thanks! Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-25 1:37 ` Chris Brandt (?) @ 2018-07-25 7:06 ` Geert Uytterhoeven 2018-07-25 12:04 ` Chris Brandt -1 siblings, 1 reply; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-25 7:06 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Chris, On Wed, Jul 25, 2018 at 3:38 AM Chris Brandt <Chris.Brandt@renesas.com> wrote: > I have one last clarification about your idea regarding documenting the > interrupts separately for RZ/A2. > > On Thursday, July 19, 2018, Geert Uytterhoeven wrote: > > 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. > > So for backward compatibility, no existing DT or setup-xxx.c (SH) file > should change and the driver needs to assume the order of interrupts. You can change the setup-xxx.c (SH) files, if needed. There's no in-kernel stable ABI. > However, if "interrupt-names" is specified in DT, then the driver > determines what the interrupt are based on their names, not the order in which > they are listed. > > Correct? Correct. > So for example, RZ/A1 DT will stay the same and since "interrupt-names" > was never used and the interrupt order will be assumed as > ERI/RXI/TXI/BRI. > > RZ/A1 [ r7s72100.dtsi ] > scif0: serial@e8007000 { > compatible = "renesas,scif-r7s72100", "renesas,scif"; > reg = <0xe8007000 64>; > interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&mstp4_clks R7S72100_CLK_SCIF0>; > clock-names = "fck"; > power-domains = <&cpg_clocks>; > status = "disabled"; > }; Correct. > However, for RZ/A2, "interrupt-names" will be used and then the driver > will sort things out however it needs to (figuring out what signals > are muxed together an such). > > RZ/A2 [ r7s9210.dtsi ] > scif0: serial@e8007000 { > compatible = "renesas,scif-r7s9210", "renesas,scif"; > reg = <0xe8007000 18>; > interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI/BRI */ > <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>, /* RXI */ > <GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>, /* TXI */ > <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>, /* ERI/BRI */ > <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>, /* TEI/DRI */ > <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>; /* TEI/DRI */ > interrupt-names = "eri", "rxi", "txi", "bri", "tei", "dri"; > > clocks = <&mstp4_clks R7S9210_CLK_SCIF0>; > clock-names = "fck"; > power-domains = <&cpg_clocks>; > status = "disabled"; > }; Correct. > Are we on the same page? I think so. 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-25 7:06 ` Geert Uytterhoeven @ 2018-07-25 12:04 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-25 12:04 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Linux-Renesas, Simon Horman Hi Geert, On Wednesday, July 25, 2018, Geert Uytterhoeven wrote: > > However, if "interrupt-names" is specified in DT, then the driver > > determines what the interrupt are based on their names, not the order in > which > > they are listed. > > > > Correct? > > Correct. One final note on this before I submit v2 of the patch. I just coded up something that works and is more simple and I don't even need "interrupt-names". Basically by using your suggestion from the code review made everything work. On Friday, July 20, 2018, Geert Uytterhoeven wrote: > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -65,6 +65,7 @@ enum { > > SCIx_RXI_IRQ, > > SCIx_TXI_IRQ, > > SCIx_BRI_IRQ, > > + SCIx_TEIDRI_IRQ, > > Why not separate enum values for TEI and DRI? According to the RZ/A2 docs, > there are 6 separate interrupts, but they are multiplexed at the interrupt > controller level. enum { SCIx_ERI_IRQ, SCIx_RXI_IRQ, SCIx_TXI_IRQ, SCIx_BRI_IRQ, + SCIx_DRI_IRQ, + SCIx_TEI_IRQ, SCIx_NR_IRQS, SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ }; Listing the same interrupt ID number twice in the DT (because it is muxed) is fine because the driver will check for that. This seems to satisfy all the SCI/SCIF variants in all the SH and ARM SoCs in the kernel today (DT and non-DT). So as long as I describe the interrupt order in the DT Documentation, all seems good. Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings @ 2018-07-25 12:04 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-25 12:04 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Geert, On Wednesday, July 25, 2018, Geert Uytterhoeven wrote: > > However, if "interrupt-names" is specified in DT, then the driver > > determines what the interrupt are based on their names, not the order in > which > > they are listed. > > > > Correct? > > Correct. One final note on this before I submit v2 of the patch. I just coded up something that works and is more simple and I don't even need "interrupt-names". Basically by using your suggestion from the code review made everything work. On Friday, July 20, 2018, Geert Uytterhoeven wrote: > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -65,6 +65,7 @@ enum { > > SCIx_RXI_IRQ, > > SCIx_TXI_IRQ, > > SCIx_BRI_IRQ, > > + SCIx_TEIDRI_IRQ, > > Why not separate enum values for TEI and DRI? According to the RZ/A2 docs, > there are 6 separate interrupts, but they are multiplexed at the interrupt > controller level. enum { SCIx_ERI_IRQ, SCIx_RXI_IRQ, SCIx_TXI_IRQ, SCIx_BRI_IRQ, + SCIx_DRI_IRQ, + SCIx_TEI_IRQ, SCIx_NR_IRQS, SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ }; Listing the same interrupt ID number twice in the DT (because it is muxed) is fine because the driver will check for that. This seems to satisfy all the SCI/SCIF variants in all the SH and ARM SoCs in the kernel today (DT and non-DT). So as long as I describe the interrupt order in the DT Documentation, all seems good. Chris ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-25 12:04 ` Chris Brandt (?) @ 2018-07-25 12:34 ` Geert Uytterhoeven -1 siblings, 0 replies; 22+ messages in thread From: Geert Uytterhoeven @ 2018-07-25 12:34 UTC (permalink / raw) To: Chris Brandt Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman Hi Chris, On Wed, Jul 25, 2018 at 2:05 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Wednesday, July 25, 2018, Geert Uytterhoeven wrote: > > > However, if "interrupt-names" is specified in DT, then the driver > > > determines what the interrupt are based on their names, not the order in > > which > > > they are listed. > > > > > > Correct? > > > > Correct. > > One final note on this before I submit v2 of the patch. > > I just coded up something that works and is more simple and I don't even > need "interrupt-names". > > Basically by using your suggestion from the code review made everything > work. > > On Friday, July 20, 2018, Geert Uytterhoeven wrote: > > > --- a/drivers/tty/serial/sh-sci.c > > > +++ b/drivers/tty/serial/sh-sci.c > > > @@ -65,6 +65,7 @@ enum { > > > SCIx_RXI_IRQ, > > > SCIx_TXI_IRQ, > > > SCIx_BRI_IRQ, > > > + SCIx_TEIDRI_IRQ, > > > > Why not separate enum values for TEI and DRI? According to the RZ/A2 docs, > > there are 6 separate interrupts, but they are multiplexed at the interrupt > > controller level. > > > enum { > SCIx_ERI_IRQ, > SCIx_RXI_IRQ, > SCIx_TXI_IRQ, > SCIx_BRI_IRQ, > + SCIx_DRI_IRQ, > + SCIx_TEI_IRQ, > SCIx_NR_IRQS, > > SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */ > }; > > Listing the same interrupt ID number twice in the DT (because it is > muxed) is fine because the driver will check for that. > > This seems to satisfy all the SCI/SCIF variants in all the SH and ARM > SoCs in the kernel today (DT and non-DT). > > So as long as I describe the interrupt order in the DT Documentation, > all seems good. Yes, that should work too. "interrupt-names" is handy for DTS writers, so they don't have to look up the order in the bindings, and it's less likely they make mistakes in the order of the interrupts. However, as soon as there will be an SoC where you will need a "hole" in the list, you're gonna need interrupt-names anyway. 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings 2018-07-17 8:35 ` Geert Uytterhoeven @ 2018-07-18 22:23 ` Chris Brandt 2018-07-18 22:19 ` Chris Brandt 2018-07-18 22:23 ` Chris Brandt 2 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-18 22:23 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Linux-Renesas, Simon Horman 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings @ 2018-07-18 22:23 ` Chris Brandt 0 siblings, 0 replies; 22+ messages in thread From: Chris Brandt @ 2018-07-18 22:23 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Greg KH, Rob Herring, Mark Rutland, open list:SERIAL DRIVERS, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Simon Horman 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-07-25 13:16 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-11 14:41 [PATCH 0/2] serial: sh-sci: Add support for RZ/A2 Chris Brandt 2018-07-11 14:41 ` [PATCH 1/2] serial: sh-sci: Add support for R7S9210 Chris Brandt 2018-07-20 8:09 ` Geert Uytterhoeven 2018-07-11 14:41 ` [PATCH 2/2] serial: sh-sci: Document r7s9210 bindings Chris Brandt 2018-07-13 15:50 ` Geert Uytterhoeven 2018-07-17 8:35 ` Geert Uytterhoeven 2018-07-17 13:43 ` Chris Brandt 2018-07-17 13:43 ` Chris Brandt 2018-07-18 22:19 ` Chris Brandt 2018-07-18 22:19 ` Chris Brandt 2018-07-19 8:13 ` Geert Uytterhoeven 2018-07-19 12:58 ` Chris Brandt 2018-07-19 12:58 ` Chris Brandt 2018-07-20 8:16 ` Geert Uytterhoeven 2018-07-25 1:37 ` Chris Brandt 2018-07-25 1:37 ` Chris Brandt 2018-07-25 7:06 ` Geert Uytterhoeven 2018-07-25 12:04 ` Chris Brandt 2018-07-25 12:04 ` Chris Brandt 2018-07-25 12:34 ` Geert Uytterhoeven 2018-07-18 22:23 ` Chris Brandt 2018-07-18 22:23 ` Chris Brandt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.