[PATCH 0/4] serial: sh-sci: Clean up previous RZ/A2 support

[PATCH 0/4] serial: sh-sci: Clean up previous RZ/A2 support

[Chris Brandt]

This patch series doesn't really provide much new functionality, but
rather provides a cleaner solution for adding RZ/A2 support.


Chris Brandt (4):
  serial: sh-sci: Improve interrupts description
  serial: sh-sci: Allow for compressed SCIF address
  serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE
  serial: sh-sci: Improve support for separate TEI and DRI interrupts

 .../bindings/serial/renesas,sci-serial.txt         |  16 ++-
 drivers/tty/serial/sh-sci.c                        | 143 ++++++++-------------
 include/linux/serial_sci.h                         |   1 -
 3 files changed, 68 insertions(+), 92 deletions(-)

-- 
2.16.1

[PATCH 1/4] serial: sh-sci: Improve interrupts description

[Chris Brandt]

Describe interrupts property in more detail, especially when there are
more than one interrupt.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 .../devicetree/bindings/serial/renesas,sci-serial.txt    | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
index a7cda6550100..eaca9da79d83 100644
--- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
+++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
@@ -73,7 +73,21 @@ Required properties:
     family-specific and/or generic versions.
 
   - reg: Base address and length of the I/O registers used by the UART.
-  - interrupts: Must contain an interrupt-specifier for the SCIx interrupt.
+  - interrupts: Must contain one or more interrupt-specifiers for the SCIx.
+                If a single interrupt is expressed, then all events are
+                multiplexed into this single interrupt.
+
+                If multiple interrupts are provided by the hardware, the order
+                in which the interrupts are listed must match order below. Note
+                that some HW interrupt events may be muxed together resulting
+                in duplicate entries.
+                The interrupt order is as follows:
+                  1. Error (ERI)
+                  2. Receive buffer full (RXI)
+                  3. Transmit buffer empty (TXI)
+                  4. Break (BRI)
+                  5. Data Ready (DRI)
+                  6. Transmit End (TEI)
 
   - clocks: Must contain a phandle and clock-specifier pair for each entry
     in clock-names.
-- 
2.16.1

[PATCH 2/4] serial: sh-sci: Allow for compressed SCIF address

[Chris Brandt]

Some devices with SCIx_SH4_SCIF_REGTYPE have no space between registers.
Use the register area size to determine the spacing between register.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/tty/serial/sh-sci.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 6ff6f2bf3b9b..138296ec9a7d 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -348,15 +348,15 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
 	[SCIx_SH4_SCIF_REGTYPE] = {
 		.regs = {
 			[SCSMR]		= { 0x00, 16 },
-			[SCBRR]		= { 0x04,  8 },
-			[SCSCR]		= { 0x08, 16 },
-			[SCxTDR]	= { 0x0c,  8 },
-			[SCxSR]		= { 0x10, 16 },
-			[SCxRDR]	= { 0x14,  8 },
-			[SCFCR]		= { 0x18, 16 },
-			[SCFDR]		= { 0x1c, 16 },
-			[SCSPTR]	= { 0x20, 16 },
-			[SCLSR]		= { 0x24, 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,
@@ -2939,6 +2939,10 @@ static int sci_init_single(struct platform_device *dev,
 			port->regshift = 1;
 	}
 
+	if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
+		if (sci_port->reg_size >= 0x20)
+			port->regshift = 1;
+
 	/*
 	 * The UART port needs an IRQ value, so we peg this to the RX IRQ
 	 * for the multi-IRQ ports, which is where we are primarily
-- 
2.16.1

[PATCH 3/4] serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE

[Chris Brandt]

There is no more need for SCIx_RZ_SCIFA_REGTYPE now that
SCIx_SH4_SCIF_REGTYPE can provide the same register/address definitions.

Also, R7S9210 no longer needs a special compatible since the standard
"renesas,scif" will work just fine.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/tty/serial/sh-sci.c | 31 -------------------------------
 include/linux/serial_sci.h  |  1 -
 2 files changed, 32 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 138296ec9a7d..cb4f7d3e7192 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -293,33 +293,6 @@ 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.
 	 */
@@ -3147,10 +3120,6 @@ 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 1c89611e0e06..c0e795d95477 100644
--- a/include/linux/serial_sci.h
+++ b/include/linux/serial_sci.h
@@ -36,7 +36,6 @@ enum {
 	SCIx_SH4_SCIF_FIFODATA_REGTYPE,
 	SCIx_SH7705_SCIF_REGTYPE,
 	SCIx_HSCIF_REGTYPE,
-	SCIx_RZ_SCIFA_REGTYPE,
 
 	SCIx_NR_REGTYPES,
 };
-- 
2.16.1

[PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Chris Brandt]

Some SCIF versions mux error and break interrupts together and then provide
a separate interrupt ID for just TEI/DRI.

Allow all 6 types of interrupts to be specified via platform data (or DT)
and for any signals that are muxed together (have the same interrupt
number) simply register one handler.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/tty/serial/sh-sci.c | 90 ++++++++++++++++++++-------------------------
 1 file changed, 40 insertions(+), 50 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index cb4f7d3e7192..a3519e8e4d13 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -65,7 +65,8 @@ enum {
 	SCIx_RXI_IRQ,
 	SCIx_TXI_IRQ,
 	SCIx_BRI_IRQ,
-	SCIx_TEIDRI_IRQ,
+	SCIx_DRI_IRQ,
+	SCIx_TEI_IRQ,
 	SCIx_NR_IRQS,
 
 	SCIx_MUX_IRQ = SCIx_NR_IRQS,	/* special case */
@@ -77,9 +78,6 @@ 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 */
@@ -1685,14 +1683,23 @@ 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_br_interrupt(int irq, void *ptr)
+{
+	struct uart_port *port = ptr;
+
+	/* Handle BREAKs */
+	sci_handle_breaks(port);
+	sci_clear_SCxSR(port, SCxSR_BREAK_CLEAR(port));
+
+	return IRQ_HANDLED;
+}
 
 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)) {
+	if (s->irqs[SCIx_ERI_IRQ] == s->irqs[SCIx_BRI_IRQ]) {
 		/* Break and Error interrupts are muxed */
 		unsigned short ssr_status = serial_port_in(port, SCxSR);
 
@@ -1727,17 +1734,6 @@ static irqreturn_t sci_er_interrupt(int irq, void *ptr)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t sci_br_interrupt(int irq, void *ptr)
-{
-	struct uart_port *port = ptr;
-
-	/* Handle BREAKs */
-	sci_handle_breaks(port);
-	sci_clear_SCxSR(port, SCxSR_BREAK_CLEAR(port));
-
-	return IRQ_HANDLED;
-}
-
 static irqreturn_t sci_mpxed_interrupt(int irq, void *ptr)
 {
 	unsigned short ssr_status, scr_status, err_enabled, orer_status = 0;
@@ -1811,6 +1807,16 @@ static const struct sci_irq_desc {
 		.handler = sci_br_interrupt,
 	},
 
+	[SCIx_DRI_IRQ] = {
+		.desc = "rx ready",
+		.handler = sci_rx_interrupt,
+	},
+
+	[SCIx_TEI_IRQ] = {
+		.desc = "tx end",
+		.handler = sci_tx_interrupt,
+	},
+
 	/*
 	 * Special muxed handler.
 	 */
@@ -1823,12 +1829,19 @@ static const struct sci_irq_desc {
 static int sci_request_irq(struct sci_port *port)
 {
 	struct uart_port *up = &port->port;
-	int i, j, ret = 0;
+	int i, j, w, ret = 0;
 
 	for (i = j = 0; i < SCIx_NR_IRQS; i++, j++) {
 		const struct sci_irq_desc *desc;
 		int irq;
 
+		/* Check if already registered (muxed) */
+		for (w = 0; w < i; w++)
+			if (port->irqs[w] == port->irqs[i])
+				w = i + 1;
+		if (w > i)
+			continue;
+
 		if (SCIx_IRQ_IS_MUXED(port)) {
 			i = SCIx_MUX_IRQ;
 			irq = up->irq;
@@ -1845,31 +1858,8 @@ static int sci_request_irq(struct sci_port *port)
 
 		desc = sci_irq_desc + i;
 		port->irqstr[j] = NULL;
-		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);
+		port->irqstr[j] = kasprintf(GFP_KERNEL, "%s:%s",
+					    dev_name(up->dev), desc->desc);
 		if (!port->irqstr[j]) {
 			ret = -ENOMEM;
 			goto out_nomem;
@@ -2842,17 +2832,17 @@ static int sci_init_single(struct platform_device *dev,
 
 	/* The SCI generates several interrupts. They can be muxed together or
 	 * connected to different interrupt lines. In the muxed case only one
-	 * interrupt resource is specified. In the non-muxed case three or four
-	 * interrupt resources are specified, as the BRI interrupt is optional.
+	 * interrupt resource is specified as there is only one interrupt ID.
+	 * In the non-muxed case, up to 6 interrupt signals might be generated
+	 * from the SCI, however those signals might have their own individual
+	 * interrupt ID numbers, or muxed together with another interrupt.
 	 */
 	if (sci_port->irqs[0] < 0)
 		return -ENXIO;
 
-	if (sci_port->irqs[1] < 0) {
-		sci_port->irqs[1] = sci_port->irqs[0];
-		sci_port->irqs[2] = sci_port->irqs[0];
-		sci_port->irqs[3] = sci_port->irqs[0];
-	}
+	if (sci_port->irqs[1] < 0)
+		for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
+			sci_port->irqs[i] = sci_port->irqs[0];
 
 	sci_port->params = sci_probe_regmap(p);
 	if (unlikely(sci_port->params == NULL))
-- 
2.16.1

Re: [PATCH 2/4] serial: sh-sci: Allow for compressed SCIF address

[Geert Uytterhoeven]

Hi Chris,

On Fri, Jul 27, 2018 at 11:09 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Some devices with SCIx_SH4_SCIF_REGTYPE have no space between registers.
> Use the register area size to determine the spacing between register.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/tty/serial/sh-sci.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 6ff6f2bf3b9b..138296ec9a7d 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -348,15 +348,15 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>         [SCIx_SH4_SCIF_REGTYPE] = {
>                 .regs = {
>                         [SCSMR]         = { 0x00, 16 },
> -                       [SCBRR]         = { 0x04,  8 },
> -                       [SCSCR]         = { 0x08, 16 },
> -                       [SCxTDR]        = { 0x0c,  8 },
> -                       [SCxSR]         = { 0x10, 16 },
> -                       [SCxRDR]        = { 0x14,  8 },
> -                       [SCFCR]         = { 0x18, 16 },
> -                       [SCFDR]         = { 0x1c, 16 },
> -                       [SCSPTR]        = { 0x20, 16 },
> -                       [SCLSR]         = { 0x24, 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,
> @@ -2939,6 +2939,10 @@ static int sci_init_single(struct platform_device *dev,
>                         port->regshift = 1;
>         }
>
> +       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> +               if (sci_port->reg_size >= 0x20)
> +                       port->regshift = 1;

I've accidentally discovered this breaks SCIF on SH7751R2D (QEMU), as the
board code doesn't fill in regtype, so it is 0 = SCIx_PROBE_REGTYPE.
The proper (default) regtype of SCIx_SH4_SCIF_REGTYPE is derived by
sci_probe_regmap(). However, that value is never fed back to sci_init_single(),
as plat_sci_port is const. So regshift ends up being wrong.

I made it work by changing the check to:

-       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
+       if (p->regtype == SCIx_SH4_SCIF_REGTYPE ||
+           (p->regtype == SCIx_PROBE_REGTYPE && port->type == PORT_SCIF))

Perhaps there's a better way?

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

Re: [PATCH 2/4] serial: sh-sci: Allow for compressed SCIF address

[Geert Uytterhoeven]

Hi Chris,

On Sun, Jul 29, 2018 at 1:11 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Jul 27, 2018 at 11:09 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> > Some devices with SCIx_SH4_SCIF_REGTYPE have no space between registers.
> > Use the register area size to determine the spacing between register.
> >
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > ---
> >  drivers/tty/serial/sh-sci.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 6ff6f2bf3b9b..138296ec9a7d 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -348,15 +348,15 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
> >         [SCIx_SH4_SCIF_REGTYPE] = {
> >                 .regs = {
> >                         [SCSMR]         = { 0x00, 16 },
> > -                       [SCBRR]         = { 0x04,  8 },
> > -                       [SCSCR]         = { 0x08, 16 },
> > -                       [SCxTDR]        = { 0x0c,  8 },
> > -                       [SCxSR]         = { 0x10, 16 },
> > -                       [SCxRDR]        = { 0x14,  8 },
> > -                       [SCFCR]         = { 0x18, 16 },
> > -                       [SCFDR]         = { 0x1c, 16 },
> > -                       [SCSPTR]        = { 0x20, 16 },
> > -                       [SCLSR]         = { 0x24, 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,
> > @@ -2939,6 +2939,10 @@ static int sci_init_single(struct platform_device *dev,
> >                         port->regshift = 1;
> >         }
> >
> > +       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> > +               if (sci_port->reg_size >= 0x20)
> > +                       port->regshift = 1;
>
> I've accidentally discovered this breaks SCIF on SH7751R2D (QEMU), as the
> board code doesn't fill in regtype, so it is 0 = SCIx_PROBE_REGTYPE.
> The proper (default) regtype of SCIx_SH4_SCIF_REGTYPE is derived by
> sci_probe_regmap(). However, that value is never fed back to sci_init_single(),
> as plat_sci_port is const. So regshift ends up being wrong.
>
> I made it work by changing the check to:
>
> -       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> +       if (p->regtype == SCIx_SH4_SCIF_REGTYPE ||
> +           (p->regtype == SCIx_PROBE_REGTYPE && port->type == PORT_SCIF))
>
> Perhaps there's a better way?

Like (whitespace-damaged-gmail):

--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3340,7 +3340,7 @@ static int sci_init_single(struct platform_device *dev,
 {
        struct uart_port *port = &sci_port->port;
        const struct resource *res;
-       unsigned int i;
+       unsigned int i, regtype;
        int ret;

        sci_port->cfg   = p;
@@ -3381,6 +3381,7 @@ static int sci_init_single(struct platform_device *dev,
        if (unlikely(sci_port->params == NULL))
                return -EINVAL;

+       regtype = sci_port->params - sci_port_params;
        switch (p->type) {
        case PORT_SCIFB:
                sci_port->rx_trigger = 48;
@@ -3435,7 +3436,7 @@ static int sci_init_single(struct platform_device *dev,
                        port->regshift = 1;
        }

-       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
+       if (regtype == SCIx_SH4_SCIF_REGTYPE)
                if (sci_port->reg_size >= 0x20)
                        port->regshift = 1;

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

Re: [PATCH 3/4] serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE

[Geert Uytterhoeven]

On Fri, Jul 27, 2018 at 11:09 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> There is no more need for SCIx_RZ_SCIFA_REGTYPE now that
> SCIx_SH4_SCIF_REGTYPE can provide the same register/address definitions.
>
> Also, R7S9210 no longer needs a special compatible since the standard
> "renesas,scif" will work just fine.
>
> 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

Re: [PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Geert Uytterhoeven]

Hi Chris,

On Fri, Jul 27, 2018 at 11:09 PM Chris Brandt <chris.brandt@renesas.com> wrote:
> Some SCIF versions mux error and break interrupts together and then provide
> a separate interrupt ID for just TEI/DRI.
>
> Allow all 6 types of interrupts to be specified via platform data (or DT)
> and for any signals that are muxed together (have the same interrupt
> number) simply register one handler.
>
> 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

> @@ -2842,17 +2832,17 @@ static int sci_init_single(struct platform_device *dev,
>
>         /* The SCI generates several interrupts. They can be muxed together or
>          * connected to different interrupt lines. In the muxed case only one
> -        * interrupt resource is specified. In the non-muxed case three or four
> -        * interrupt resources are specified, as the BRI interrupt is optional.
> +        * interrupt resource is specified as there is only one interrupt ID.
> +        * In the non-muxed case, up to 6 interrupt signals might be generated
> +        * from the SCI, however those signals might have their own individual
> +        * interrupt ID numbers, or muxed together with another interrupt.
>          */
>         if (sci_port->irqs[0] < 0)
>                 return -ENXIO;
>
> -       if (sci_port->irqs[1] < 0) {
> -               sci_port->irqs[1] = sci_port->irqs[0];
> -               sci_port->irqs[2] = sci_port->irqs[0];
> -               sci_port->irqs[3] = sci_port->irqs[0];
> -       }
> +       if (sci_port->irqs[1] < 0)
> +               for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)

Shouldn't the "- 1" be dropped?

> +                       sci_port->irqs[i] = sci_port->irqs[0];
>
>         sci_port->params = sci_probe_regmap(p);
>         if (unlikely(sci_port->params == NULL))

With the above fixed:
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

RE: [PATCH 2/4] serial: sh-sci: Allow for compressed SCIF address

[Chris Brandt]

Hi Geert,

On Sunday, July 29, 2018, Geert Uytterhoeven wrote:
> I've accidentally discovered this breaks SCIF on SH7751R2D (QEMU), as the
> board code doesn't fill in regtype, so it is 0 = SCIx_PROBE_REGTYPE.

Oh that pesky SH7751!!


On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> > I made it work by changing the check to:
> >
> > -       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> > +       if (p->regtype == SCIx_SH4_SCIF_REGTYPE ||
> > +           (p->regtype == SCIx_PROBE_REGTYPE && port->type ==
> PORT_SCIF))
> >
> > Perhaps there's a better way?
> 
> Like (whitespace-damaged-gmail):
> 
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3340,7 +3340,7 @@ static int sci_init_single(struct platform_device
> *dev,
>  {
>         struct uart_port *port = &sci_port->port;
>         const struct resource *res;
> -       unsigned int i;
> +       unsigned int i, regtype;
>         int ret;
> 
>         sci_port->cfg   = p;
> @@ -3381,6 +3381,7 @@ static int sci_init_single(struct platform_device
> *dev,
>         if (unlikely(sci_port->params == NULL))
>                 return -EINVAL;
> 
> +       regtype = sci_port->params - sci_port_params;
>         switch (p->type) {
>         case PORT_SCIFB:
>                 sci_port->rx_trigger = 48;
> @@ -3435,7 +3436,7 @@ static int sci_init_single(struct platform_device
> *dev,
>                         port->regshift = 1;
>         }
> 
> -       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> +       if (regtype == SCIx_SH4_SCIF_REGTYPE)
>                 if (sci_port->reg_size >= 0x20)
>                         port->regshift = 1;

That's clever.

If it worked for SH7751, I'll go test it on RZ/A2 as well and resubmit 
the patch.


Chris

RE: [PATCH 2/4] serial: sh-sci: Allow for compressed SCIF address

[Chris Brandt]

Hi Geert,

On Sunday, July 29, 2018, Geert Uytterhoeven wrote:
> I've accidentally discovered this breaks SCIF on SH7751R2D (QEMU), as the
> board code doesn't fill in regtype, so it is 0 = SCIx_PROBE_REGTYPE.

Oh that pesky SH7751!!


On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> > I made it work by changing the check to:
> >
> > -       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> > +       if (p->regtype == SCIx_SH4_SCIF_REGTYPE ||
> > +           (p->regtype == SCIx_PROBE_REGTYPE && port->type ==
> PORT_SCIF))
> >
> > Perhaps there's a better way?
> 
> Like (whitespace-damaged-gmail):
> 
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3340,7 +3340,7 @@ static int sci_init_single(struct platform_device
> *dev,
>  {
>         struct uart_port *port = &sci_port->port;
>         const struct resource *res;
> -       unsigned int i;
> +       unsigned int i, regtype;
>         int ret;
> 
>         sci_port->cfg   = p;
> @@ -3381,6 +3381,7 @@ static int sci_init_single(struct platform_device
> *dev,
>         if (unlikely(sci_port->params == NULL))
>                 return -EINVAL;
> 
> +       regtype = sci_port->params - sci_port_params;
>         switch (p->type) {
>         case PORT_SCIFB:
>                 sci_port->rx_trigger = 48;
> @@ -3435,7 +3436,7 @@ static int sci_init_single(struct platform_device
> *dev,
>                         port->regshift = 1;
>         }
> 
> -       if (p->regtype == SCIx_SH4_SCIF_REGTYPE)
> +       if (regtype == SCIx_SH4_SCIF_REGTYPE)
>                 if (sci_port->reg_size >= 0x20)
>                         port->regshift = 1;

That's clever.

If it worked for SH7751, I'll go test it on RZ/A2 as well and resubmit 
the patch.


Chris

RE: [PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Chris Brandt]

Hi Geert,

On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> >         if (sci_port->irqs[0] < 0)
> >                 return -ENXIO;
> >
> > -       if (sci_port->irqs[1] < 0) {
> > -               sci_port->irqs[1] = sci_port->irqs[0];
> > -               sci_port->irqs[2] = sci_port->irqs[0];
> > -               sci_port->irqs[3] = sci_port->irqs[0];
> > -       }
> > +       if (sci_port->irqs[1] < 0)
> > +               for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
> 
> Shouldn't the "- 1" be dropped?

In reality, the last entry of the array is 'SCIx_NR_IRQS', so it's not 
really used anywhere.

The original array was:
enum {
	SCIx_ERI_IRQ,   (the only IRQ specified in DT)
	SCIx_RXI_IRQ,   << copy from irqs[0]
	SCIx_TXI_IRQ,   << copy from irqs[0]
	SCIx_BRI_IRQ,   << copy from irqs[0]
	SCIx_NR_IRQS,   (didn’t' touch)

	SCIx_MUX_IRQ = SCIx_NR_IRQS,	/* special case */
};


So the new for loop used "- "1 in order to create the same outcome.

But as far as I can tell irqs[SCIx_NR_IRQS] is never used anywhere, it 
doesn't really matter.


> With the above fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I have no problem taking the "- 1" out.

But...here's the funny part: It was you that suggested the "- 1"  ;)

On Thursday, July 26, 2018, Geert Uytterhoeven wrote:
> > @@ -2809,6 +2845,8 @@ static int sci_init_single(struct platform_device
> *dev,
> >                 sci_port->irqs[1] = sci_port->irqs[0];
> >                 sci_port->irqs[2] = sci_port->irqs[0];
> >                 sci_port->irqs[3] = sci_port->irqs[0];
> > +               sci_port->irqs[4] = sci_port->irqs[0];
> > +               sci_port->irqs[5] = sci_port->irqs[0];
> 
> You may want to start using a loop from 1 to ARRAY_SIZE(sci_port->irqs) - 1
> instead.


Cheers

Chris

RE: [PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Chris Brandt]

Hi Geert,

On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> >         if (sci_port->irqs[0] < 0)
> >                 return -ENXIO;
> >
> > -       if (sci_port->irqs[1] < 0) {
> > -               sci_port->irqs[1] = sci_port->irqs[0];
> > -               sci_port->irqs[2] = sci_port->irqs[0];
> > -               sci_port->irqs[3] = sci_port->irqs[0];
> > -       }
> > +       if (sci_port->irqs[1] < 0)
> > +               for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
> 
> Shouldn't the "- 1" be dropped?

In reality, the last entry of the array is 'SCIx_NR_IRQS', so it's not 
really used anywhere.

The original array was:
enum {
	SCIx_ERI_IRQ,   (the only IRQ specified in DT)
	SCIx_RXI_IRQ,   << copy from irqs[0]
	SCIx_TXI_IRQ,   << copy from irqs[0]
	SCIx_BRI_IRQ,   << copy from irqs[0]
	SCIx_NR_IRQS,   (didn’t' touch)

	SCIx_MUX_IRQ = SCIx_NR_IRQS,	/* special case */
};


So the new for loop used "- "1 in order to create the same outcome.

But as far as I can tell irqs[SCIx_NR_IRQS] is never used anywhere, it 
doesn't really matter.


> With the above fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I have no problem taking the "- 1" out.

But...here's the funny part: It was you that suggested the "- 1"  ;)

On Thursday, July 26, 2018, Geert Uytterhoeven wrote:
> > @@ -2809,6 +2845,8 @@ static int sci_init_single(struct platform_device
> *dev,
> >                 sci_port->irqs[1] = sci_port->irqs[0];
> >                 sci_port->irqs[2] = sci_port->irqs[0];
> >                 sci_port->irqs[3] = sci_port->irqs[0];
> > +               sci_port->irqs[4] = sci_port->irqs[0];
> > +               sci_port->irqs[5] = sci_port->irqs[0];
> 
> You may want to start using a loop from 1 to ARRAY_SIZE(sci_port->irqs) - 1
> instead.


Cheers

Chris

Re: [PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Geert Uytterhoeven]

Hi Chris,

On Mon, Jul 30, 2018 at 2:33 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> > >         if (sci_port->irqs[0] < 0)
> > >                 return -ENXIO;
> > >
> > > -       if (sci_port->irqs[1] < 0) {
> > > -               sci_port->irqs[1] = sci_port->irqs[0];
> > > -               sci_port->irqs[2] = sci_port->irqs[0];
> > > -               sci_port->irqs[3] = sci_port->irqs[0];
> > > -       }
> > > +       if (sci_port->irqs[1] < 0)
> > > +               for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
> >
> > Shouldn't the "- 1" be dropped?
>
> In reality, the last entry of the array is 'SCIx_NR_IRQS', so it's not
> really used anywhere.
>
> The original array was:
> enum {
>         SCIx_ERI_IRQ,   (the only IRQ specified in DT)

=0

>         SCIx_RXI_IRQ,   << copy from irqs[0]

= 1

>         SCIx_TXI_IRQ,   << copy from irqs[0]

= 2

>         SCIx_BRI_IRQ,   << copy from irqs[0]

= 3

>         SCIx_NR_IRQS,   (didn’t' touch)

= 4

>
>         SCIx_MUX_IRQ = SCIx_NR_IRQS,    /* special case */
> };

That's not the array, but the enum.

The array is in struct sci_port:

        int                     irqs[SCIx_NR_IRQS];

It has four entries, at indices 0..3.

> So the new for loop used "- "1 in order to create the same outcome.
>
> But as far as I can tell irqs[SCIx_NR_IRQS] is never used anywhere, it
> doesn't really matter.

irqs[SCIx_NR_IRQS] does not exist!

sci_irq_desc[SCIx_NR_IRQS] aka sci_irq_desc[SCIx_MUX_IRQ] does exit,
but that's a different array.

> > With the above fixed:
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I have no problem taking the "- 1" out.
>
> But...here's the funny part: It was you that suggested the "- 1"  ;)
>
> On Thursday, July 26, 2018, Geert Uytterhoeven wrote:
> > > @@ -2809,6 +2845,8 @@ static int sci_init_single(struct platform_device
> > *dev,
> > >                 sci_port->irqs[1] = sci_port->irqs[0];
> > >                 sci_port->irqs[2] = sci_port->irqs[0];
> > >                 sci_port->irqs[3] = sci_port->irqs[0];
> > > +               sci_port->irqs[4] = sci_port->irqs[0];
> > > +               sci_port->irqs[5] = sci_port->irqs[0];
> >
> > You may want to start using a loop from 1 to ARRAY_SIZE(sci_port->irqs) - 1
> > instead.

Your loop is:

    for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)

It loops over 1..ARRAY_SIZE(sci_port->irqs) - 2.
Note the "<" and the "- 1".

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

RE: [PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Chris Brandt]

Hi Geert,

On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> That's not the array, but the enum.
> 
> The array is in struct sci_port:
> 
>         int                     irqs[SCIx_NR_IRQS];
> 
> It has four entries, at indices 0..3.

> irqs[SCIx_NR_IRQS] does not exist!
> 
> sci_irq_desc[SCIx_NR_IRQS] aka sci_irq_desc[SCIx_MUX_IRQ] does exit,
> but that's a different array.


> Your loop is:
> 
>     for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
> 
> It loops over 1..ARRAY_SIZE(sci_port->irqs) - 2.
> Note the "<" and the "- 1".


Ahhhh, you're right!
Sorry about that.

Thanks,
Chris

RE: [PATCH 4/4] serial: sh-sci: Improve support for separate TEI and DRI interrupts

[Chris Brandt]

Hi Geert,

On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> That's not the array, but the enum.
> 
> The array is in struct sci_port:
> 
>         int                     irqs[SCIx_NR_IRQS];
> 
> It has four entries, at indices 0..3.

> irqs[SCIx_NR_IRQS] does not exist!
> 
> sci_irq_desc[SCIx_NR_IRQS] aka sci_irq_desc[SCIx_MUX_IRQ] does exit,
> but that's a different array.


> Your loop is:
> 
>     for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
> 
> It loops over 1..ARRAY_SIZE(sci_port->irqs) - 2.
> Note the "<" and the "- 1".


Ahhhh, you're right!
Sorry about that.

Thanks,
Chris