[PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
is transmit end interrupt, so shuffle the interrupts to fix the
transmit end interrupt handler for these IPs.

Fixes: 392fb8df528b ("serial: sh-sci: Use platform_get_irq_optional() for optional interrupts")
Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
Tested the SCI0 interface on RZ/G2UL by connecting to PMOD USBUART.
 39:          0     GICv3 437 Level     1004d000.serial:rx err
 40:         12     GICv3 438 Edge      1004d000.serial:rx full
 41:         70     GICv3 439 Edge      1004d000.serial:tx empty
 42:         18     GICv3 440 Level     1004d000.serial:tx end
---
 drivers/tty/serial/sh-sci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 7bd080720929..9b07b45a6948 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -31,6 +31,7 @@
 #include <linux/ioport.h>
 #include <linux/ktime.h>
 #include <linux/major.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/of.h>
@@ -2841,6 +2842,7 @@ static int sci_init_single(struct platform_device *dev,
 	struct uart_port *port = &sci_port->port;
 	const struct resource *res;
 	unsigned int i;
+	int irq_cnt;
 	int ret;
 
 	sci_port->cfg	= p;
@@ -2864,6 +2866,14 @@ static int sci_init_single(struct platform_device *dev,
 			sci_port->irqs[i] = platform_get_irq(dev, i);
 	}
 
+	/*
+	 * RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
+	 * is transmit end interrupt, so shuffle the interrupts.
+	 */
+	irq_cnt = platform_irq_count(dev);
+	if (irq_cnt == 4)
+		swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);
+
 	/* 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 as there is only one interrupt ID.
-- 
2.25.1

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

On Thu, Mar 16, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> is transmit end interrupt, so shuffle the interrupts to fix the
> transmit end interrupt handler for these IPs.
>
> Fixes: 392fb8df528b ("serial: sh-sci: Use platform_get_irq_optional() for optional interrupts")

I don't think that's the right bad commit.

> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -31,6 +31,7 @@
>  #include <linux/ioport.h>
>  #include <linux/ktime.h>
>  #include <linux/major.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/mm.h>
>  #include <linux/of.h>
> @@ -2841,6 +2842,7 @@ static int sci_init_single(struct platform_device *dev,
>         struct uart_port *port = &sci_port->port;
>         const struct resource *res;
>         unsigned int i;
> +       int irq_cnt;
>         int ret;
>
>         sci_port->cfg   = p;
> @@ -2864,6 +2866,14 @@ static int sci_init_single(struct platform_device *dev,
>                         sci_port->irqs[i] = platform_get_irq(dev, i);
>         }
>
> +       /*
> +        * RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> +        * is transmit end interrupt, so shuffle the interrupts.
> +        */
> +       irq_cnt = platform_irq_count(dev);
> +       if (irq_cnt == 4)
> +               swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);
> +

I think it's simpler to just check if SCIx_TEI_IRQ is missing:

    if (sci_port->irqs[SCIx_TEI_IRQ] < 0)
            swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);

We already rely on "make dtbs_check" to catch invalid combinations
(anything different from 1/4/6 interrupts).

And please move that code below, together with the other checks for
non-existing interrupts.

>         /* 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 as there is only one interrupt ID.

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> On Thu, Mar 16, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt is
> > transmit end interrupt, so shuffle the interrupts to fix the transmit
> > end interrupt handler for these IPs.
> >
> > Fixes: 392fb8df528b ("serial: sh-sci: Use platform_get_irq_optional()
> > for optional interrupts")
> 
> I don't think that's the right bad commit.

OK. I will use below commit as fixes one, 
that is the commit which added RZ/A1 SCIF with 4 interrupts.

commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for R7S9210")


> 
> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/ioport.h>
> >  #include <linux/ktime.h>
> >  #include <linux/major.h>
> > +#include <linux/minmax.h>
> >  #include <linux/module.h>
> >  #include <linux/mm.h>
> >  #include <linux/of.h>
> > @@ -2841,6 +2842,7 @@ static int sci_init_single(struct platform_device
> *dev,
> >         struct uart_port *port = &sci_port->port;
> >         const struct resource *res;
> >         unsigned int i;
> > +       int irq_cnt;
> >         int ret;
> >
> >         sci_port->cfg   = p;
> > @@ -2864,6 +2866,14 @@ static int sci_init_single(struct platform_device
> *dev,
> >                         sci_port->irqs[i] = platform_get_irq(dev, i);
> >         }
> >
> > +       /*
> > +        * RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> > +        * is transmit end interrupt, so shuffle the interrupts.
> > +        */
> > +       irq_cnt = platform_irq_count(dev);
> > +       if (irq_cnt == 4)
> > +               swap(sci_port->irqs[SCIx_BRI_IRQ],
> > + sci_port->irqs[SCIx_TEI_IRQ]);
> > +
> 
> I think it's simpler to just check if SCIx_TEI_IRQ is missing:
> 
>     if (sci_port->irqs[SCIx_TEI_IRQ] < 0)
>             swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port-
> >irqs[SCIx_TEI_IRQ]);

OK.

> 
> We already rely on "make dtbs_check" to catch invalid combinations (anything
> different from 1/4/6 interrupts).
> 
> And please move that code below, together with the other checks for non-
> existing interrupts.

OK, Will add below code in probe

+       irq_cnt = platform_irq_count(dev);
+       if (irq_cnt != 1 && irq_cnt != 4 && irq_cnt != 6)
+               return -EINVAL;
+

Cheers,
Biju

> 
> >         /* 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 as there is only one interrupt
> ID.
> 
> 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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

On Thu, Mar 16, 2023 at 5:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

> > On Thu, Mar 16, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt is
> > > transmit end interrupt, so shuffle the interrupts to fix the transmit
> > > end interrupt handler for these IPs.
> > >
> > > Fixes: 392fb8df528b ("serial: sh-sci: Use platform_get_irq_optional()
> > > for optional interrupts")
> >
> > I don't think that's the right bad commit.
>
> OK. I will use below commit as fixes one,
> that is the commit which added RZ/A1 SCIF with 4 interrupts.
>
> commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for R7S9210")

That one added support for RZ/A2, and is also not the bad
commit?

> > > --- a/drivers/tty/serial/sh-sci.c
> > > +++ b/drivers/tty/serial/sh-sci.c
> > > @@ -31,6 +31,7 @@
> > >  #include <linux/ioport.h>
> > >  #include <linux/ktime.h>
> > >  #include <linux/major.h>
> > > +#include <linux/minmax.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/of.h>
> > > @@ -2841,6 +2842,7 @@ static int sci_init_single(struct platform_device
> > *dev,
> > >         struct uart_port *port = &sci_port->port;
> > >         const struct resource *res;
> > >         unsigned int i;
> > > +       int irq_cnt;
> > >         int ret;
> > >
> > >         sci_port->cfg   = p;
> > > @@ -2864,6 +2866,14 @@ static int sci_init_single(struct platform_device
> > *dev,
> > >                         sci_port->irqs[i] = platform_get_irq(dev, i);
> > >         }
> > >
> > > +       /*
> > > +        * RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> > > +        * is transmit end interrupt, so shuffle the interrupts.
> > > +        */
> > > +       irq_cnt = platform_irq_count(dev);
> > > +       if (irq_cnt == 4)
> > > +               swap(sci_port->irqs[SCIx_BRI_IRQ],
> > > + sci_port->irqs[SCIx_TEI_IRQ]);
> > > +
> >
> > I think it's simpler to just check if SCIx_TEI_IRQ is missing:
> >
> >     if (sci_port->irqs[SCIx_TEI_IRQ] < 0)
> >             swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port-
> > >irqs[SCIx_TEI_IRQ]);
>
> OK.
>
> >
> > We already rely on "make dtbs_check" to catch invalid combinations (anything
> > different from 1/4/6 interrupts).
> >
> > And please move that code below, together with the other checks for non-
> > existing interrupts.
>
> OK, Will add below code in probe
>
> +       irq_cnt = platform_irq_count(dev);
> +       if (irq_cnt != 1 && irq_cnt != 4 && irq_cnt != 6)
> +               return -EINVAL;
> +

IMHO none of these checks are needed. "make dtbs_check" takes
care of that.

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> On Thu, Mar 16, 2023 at 5:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler
> 
> > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> > > > is transmit end interrupt, so shuffle the interrupts to fix the
> > > > transmit end interrupt handler for these IPs.
> > > >
> > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > platform_get_irq_optional() for optional interrupts")
> > >
> > > I don't think that's the right bad commit.
> >
> > OK. I will use below commit as fixes one, that is the commit which
> > added RZ/A1 SCIF with 4 interrupts.
> >
> > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for R7S9210")
> 
> That one added support for RZ/A2, and is also not the bad commit?

OK will use below one,

Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to dtsi")

> 
> > > > --- a/drivers/tty/serial/sh-sci.c
> > > > +++ b/drivers/tty/serial/sh-sci.c
> > > > @@ -31,6 +31,7 @@
> > > >  #include <linux/ioport.h>
> > > >  #include <linux/ktime.h>
> > > >  #include <linux/major.h>
> > > > +#include <linux/minmax.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/mm.h>
> > > >  #include <linux/of.h>
> > > > @@ -2841,6 +2842,7 @@ static int sci_init_single(struct
> > > > platform_device
> > > *dev,
> > > >         struct uart_port *port = &sci_port->port;
> > > >         const struct resource *res;
> > > >         unsigned int i;
> > > > +       int irq_cnt;
> > > >         int ret;
> > > >
> > > >         sci_port->cfg   = p;
> > > > @@ -2864,6 +2866,14 @@ static int sci_init_single(struct
> > > > platform_device
> > > *dev,
> > > >                         sci_port->irqs[i] = platform_get_irq(dev, i);
> > > >         }
> > > >
> > > > +       /*
> > > > +        * RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> interrupt
> > > > +        * is transmit end interrupt, so shuffle the interrupts.
> > > > +        */
> > > > +       irq_cnt = platform_irq_count(dev);
> > > > +       if (irq_cnt == 4)
> > > > +               swap(sci_port->irqs[SCIx_BRI_IRQ],
> > > > + sci_port->irqs[SCIx_TEI_IRQ]);
> > > > +
> > >
> > > I think it's simpler to just check if SCIx_TEI_IRQ is missing:
> > >
> > >     if (sci_port->irqs[SCIx_TEI_IRQ] < 0)
> > >             swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port-
> > > >irqs[SCIx_TEI_IRQ]);
> >
> > OK.
> >
> > >
> > > We already rely on "make dtbs_check" to catch invalid combinations
> > > (anything different from 1/4/6 interrupts).
> > >
> > > And please move that code below, together with the other checks for
> > > non- existing interrupts.
> >
> > OK, Will add below code in probe
> >
> > +       irq_cnt = platform_irq_count(dev);
> > +       if (irq_cnt != 1 && irq_cnt != 4 && irq_cnt != 6)
> > +               return -EINVAL;
> > +
> 
> IMHO none of these checks are needed. "make dtbs_check" takes care of that.

Agreed. Will remove.

Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

On Fri, Mar 17, 2023 at 8:59 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > On Thu, Mar 16, 2023 at 5:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > > handler
> >
> > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth interrupt
> > > > > is transmit end interrupt, so shuffle the interrupts to fix the
> > > > > transmit end interrupt handler for these IPs.
> > > > >
> > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > platform_get_irq_optional() for optional interrupts")
> > > >
> > > > I don't think that's the right bad commit.
> > >
> > > OK. I will use below commit as fixes one, that is the commit which
> > > added RZ/A1 SCIF with 4 interrupts.
> > >
> > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for R7S9210")
> >
> > That one added support for RZ/A2, and is also not the bad commit?
>
> OK will use below one,
>
> Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to dtsi")

This really starts to look like a guessing game... Beep ;-)

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Geert,

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> On Fri, Mar 17, 2023 at 8:59 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler
> > >
> > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> > > > > > interrupt is transmit end interrupt, so shuffle the interrupts
> > > > > > to fix the transmit end interrupt handler for these IPs.
> > > > > >
> > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > platform_get_irq_optional() for optional interrupts")
> > > > >
> > > > > I don't think that's the right bad commit.
> > > >
> > > > OK. I will use below commit as fixes one, that is the commit which
> > > > added RZ/A1 SCIF with 4 interrupts.
> > > >
> > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > R7S9210")
> > >
> > > That one added support for RZ/A2, and is also not the bad commit?
> >
> > OK will use below one,
> >
> > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to
> > dtsi")
> 
> This really starts to look like a guessing game... Beep ;-)

Already there is a generic compatible in driver, where we started introducing RZ/A1 SoC
With 4 interrupts. So addition of this SoC has this issue. Am I missing anything here?

Please let me know.

Cheers,
Biju

RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

+ Wolfram

> Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Geert,
> 
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > handler
> >
> > Hi Biju,
> >
> > On Fri, Mar 17, 2023 at 8:59 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > interrupt handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > interrupt handler
> > > >
> > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> > > > > > > interrupt is transmit end interrupt, so shuffle the
> > > > > > > interrupts to fix the transmit end interrupt handler for these
> IPs.
> > > > > > >
> > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > >
> > > > > > I don't think that's the right bad commit.
> > > > >
> > > > > OK. I will use below commit as fixes one, that is the commit
> > > > > which added RZ/A1 SCIF with 4 interrupts.
> > > > >
> > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > R7S9210")
> > > >
> > > > That one added support for RZ/A2, and is also not the bad commit?
> > >
> > > OK will use below one,
> > >
> > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to
> > > dtsi")
> >
> > This really starts to look like a guessing game... Beep ;-)
> 
> Already there is a generic compatible in driver, where we started
> introducing RZ/A1 SoC With 4 interrupts. So addition of this SoC has this
> issue. Am I missing anything here?

Wolfram, Can you please confirm transmit end interrupt handler worked for you
with the commit 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to dtsi")

Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

On Fri, Mar 17, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler
> > > On Fri, Mar 17, 2023 at 8:59 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > > interrupt handler
> > > > >
> > > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> > > > > > > > interrupt is transmit end interrupt, so shuffle the
> > > > > > > > interrupts to fix the transmit end interrupt handler for these
> > IPs.
> > > > > > > >
> > > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > > >
> > > > > > > I don't think that's the right bad commit.
> > > > > >
> > > > > > OK. I will use below commit as fixes one, that is the commit
> > > > > > which added RZ/A1 SCIF with 4 interrupts.
> > > > > >
> > > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > > R7S9210")
> > > > >
> > > > > That one added support for RZ/A2, and is also not the bad commit?
> > > >
> > > > OK will use below one,
> > > >
> > > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to
> > > > dtsi")
> > >
> > > This really starts to look like a guessing game... Beep ;-)
> >
> > Already there is a generic compatible in driver, where we started
> > introducing RZ/A1 SoC With 4 interrupts. So addition of this SoC has this
> > issue. Am I missing anything here?
>
> Wolfram, Can you please confirm transmit end interrupt handler worked for you
> with the commit 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to dtsi")

That issue is moot: the fourth IRQ on RZ/A1 is BRI, not TEI, cfr. my
previous email.

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

Hi Biju,

> Wolfram, Can you please confirm transmit end interrupt handler worked for you
> with the commit 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to dtsi")

That was nearly 10 years ago :) I can't recall if a specific interrupt
worked. But SCIF worked. So, if it was needed for proper operation, then
it probably worked. If it was not needed, no idea.

This is all for a Fixes: tag, or? Is it that important?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 109 bytes --]


> This is all for a Fixes: tag, or? Is it that important?

Reading the other thread: yes, it is important!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> > Wolfram, Can you please confirm transmit end interrupt handler worked
> > for you with the commit 4c84c1b3acca ("ARM: shmobile: r7s72100: add
> > scif nodes to dtsi")
> 
> That was nearly 10 years ago :) I can't recall if a specific interrupt
> worked. But SCIF worked. So, if it was needed for proper operation, then it
> probably worked. If it was not needed, no idea.
> 
> This is all for a Fixes: tag, or? Is it that important?

Yes, I guess for backporting to stable fixes tag is important.

Now RZ/A1 is ruled out, as it uses BRI.

So RZ/A2, RZ/G2L alike SoCs and SH has this broken feature.

If SH SCI support is broken, then we must backport up to 4.14 stable.

If it is after RZ/A2, then we must backport up to 4.19 stable.

Else RZ/G2L alike SoCs then we must backport up to 6.1 stable.


Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

On Fri, Mar 17, 2023 at 9:08 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > On Fri, Mar 17, 2023 at 8:59 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > > handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > interrupt handler
> > > >
> > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> > > > > > > interrupt is transmit end interrupt, so shuffle the interrupts
> > > > > > > to fix the transmit end interrupt handler for these IPs.
> > > > > > >
> > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > >
> > > > > > I don't think that's the right bad commit.
> > > > >
> > > > > OK. I will use below commit as fixes one, that is the commit which
> > > > > added RZ/A1 SCIF with 4 interrupts.
> > > > >
> > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > R7S9210")
> > > >
> > > > That one added support for RZ/A2, and is also not the bad commit?
> > >
> > > OK will use below one,
> > >
> > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to
> > > dtsi")
> >
> > This really starts to look like a guessing game... Beep ;-)
>
> Already there is a generic compatible in driver, where we started introducing RZ/A1 SoC
> With 4 interrupts. So addition of this SoC has this issue. Am I missing anything here?

The rabbit hole seems to be deeper than I thought...

Looking at the code, the driver always assumed the fourth interrupt
is BRI, which matches the RZ/A1 datasheet for SCIF.
So the 4 IRQ case is really a subset of the 6 IRQ case, and
Documentation/devicetree/bindings/serial/renesas,scif.yaml
is wrong.

However, SCI(g) is the odd one (also on SH): it has TEI as the fourth
IRQ, which I probably missed when doing the json-schema conversion
in commit 384d00fae8e51f8f ("dt-bindings: serial: sh-sci: Convert to
json-schema"), leading to the bug in scif.yaml.

Note that the driver never looks at the interrupt names, but uses
indices exclusively.

So I guess SCI has been broken on SH since forever, too?

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> On Fri, Mar 17, 2023 at 9:08 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler On Fri, Mar 17, 2023 at 8:59 AM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > > interrupt handler
> > > > >
> > > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> > > > > > > > interrupt is transmit end interrupt, so shuffle the
> > > > > > > > interrupts to fix the transmit end interrupt handler for these
> IPs.
> > > > > > > >
> > > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > > >
> > > > > > > I don't think that's the right bad commit.
> > > > > >
> > > > > > OK. I will use below commit as fixes one, that is the commit
> > > > > > which added RZ/A1 SCIF with 4 interrupts.
> > > > > >
> > > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > > R7S9210")
> > > > >
> > > > > That one added support for RZ/A2, and is also not the bad commit?
> > > >
> > > > OK will use below one,
> > > >
> > > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to
> > > > dtsi")
> > >
> > > This really starts to look like a guessing game... Beep ;-)
> >
> > Already there is a generic compatible in driver, where we started
> > introducing RZ/A1 SoC With 4 interrupts. So addition of this SoC has this
> issue. Am I missing anything here?
> 
> The rabbit hole seems to be deeper than I thought...
> 
> Looking at the code, the driver always assumed the fourth interrupt is BRI,
> which matches the RZ/A1 datasheet for SCIF.
> So the 4 IRQ case is really a subset of the 6 IRQ case, and
> Documentation/devicetree/bindings/serial/renesas,scif.yaml
> is wrong.

OK.

> 
> However, SCI(g) is the odd one (also on SH): it has TEI as the fourth IRQ,
> which I probably missed when doing the json-schema conversion in commit
> 384d00fae8e51f8f ("dt-bindings: serial: sh-sci: Convert to json-schema"),
> leading to the bug in scif.yaml.
> 
> Note that the driver never looks at the interrupt names, but uses indices
> exclusively.
> 
> So I guess SCI has been broken on SH since forever, too?

I think so, by looking at the changes done in tx to make it work on RZ/G2UL.
On RZ/G2UL both rx and tx is broken.

Not sure SCI is tested ever on SH platform??

Can any SH platform person confirm this?

Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

CC linux-sh

On Fri, Mar 17, 2023 at 10:15 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > On Fri, Mar 17, 2023 at 9:08 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > > handler On Fri, Mar 17, 2023 at 8:59 AM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > interrupt handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > > > interrupt handler
> > > > > >
> > > > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > wrote:
> > > > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The fourth
> > > > > > > > > interrupt is transmit end interrupt, so shuffle the
> > > > > > > > > interrupts to fix the transmit end interrupt handler for these
> > IPs.
> > > > > > > > >
> > > > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > > > >
> > > > > > > > I don't think that's the right bad commit.
> > > > > > >
> > > > > > > OK. I will use below commit as fixes one, that is the commit
> > > > > > > which added RZ/A1 SCIF with 4 interrupts.
> > > > > > >
> > > > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > > > R7S9210")
> > > > > >
> > > > > > That one added support for RZ/A2, and is also not the bad commit?
> > > > >
> > > > > OK will use below one,
> > > > >
> > > > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes to
> > > > > dtsi")
> > > >
> > > > This really starts to look like a guessing game... Beep ;-)
> > >
> > > Already there is a generic compatible in driver, where we started
> > > introducing RZ/A1 SoC With 4 interrupts. So addition of this SoC has this
> > issue. Am I missing anything here?
> >
> > The rabbit hole seems to be deeper than I thought...
> >
> > Looking at the code, the driver always assumed the fourth interrupt is BRI,
> > which matches the RZ/A1 datasheet for SCIF.
> > So the 4 IRQ case is really a subset of the 6 IRQ case, and
> > Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > is wrong.
>
> OK.
>
> >
> > However, SCI(g) is the odd one (also on SH): it has TEI as the fourth IRQ,
> > which I probably missed when doing the json-schema conversion in commit
> > 384d00fae8e51f8f ("dt-bindings: serial: sh-sci: Convert to json-schema"),
> > leading to the bug in scif.yaml.
> >
> > Note that the driver never looks at the interrupt names, but uses indices
> > exclusively.
> >
> > So I guess SCI has been broken on SH since forever, too?
>
> I think so, by looking at the changes done in tx to make it work on RZ/G2UL.
> On RZ/G2UL both rx and tx is broken.
>
> Not sure SCI is tested ever on SH platform??
>
> Can any SH platform person confirm this?

SCI is only supported on
  - sh770x,
  - sh7750 (excluding rts7751r2d)
    I know it's not exposed on my landisk,
  - sh7760, for the SIM-port, which I doubt anyone uses.

FTR, I tried the "obvious" thing (remove the rts7751r2d-checks in
arch/sh/kernel/cpu/sh4/setup-sh7750.c, and replace sci_br_interrupt
as the interrupt handler by sci_tx_interrupt in sh-sci.c), but that
didn't make ttySC0 work on qemu/rts7751r2d.

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> CC linux-sh
> 
> On Fri, Mar 17, 2023 at 10:15 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler On Fri, Mar 17, 2023 at 9:08 AM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler On Fri, Mar 17, 2023 at 8:59 AM Biju Das
> > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > > interrupt handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit
> > > > > > > > > end interrupt handler
> > > > > > >
> > > > > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > > wrote:
> > > > > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The
> > > > > > > > > > fourth interrupt is transmit end interrupt, so shuffle
> > > > > > > > > > the interrupts to fix the transmit end interrupt
> > > > > > > > > > handler for these
> > > IPs.
> > > > > > > > > >
> > > > > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > > > > >
> > > > > > > > > I don't think that's the right bad commit.
> > > > > > > >
> > > > > > > > OK. I will use below commit as fixes one, that is the
> > > > > > > > commit which added RZ/A1 SCIF with 4 interrupts.
> > > > > > > >
> > > > > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > > > > R7S9210")
> > > > > > >
> > > > > > > That one added support for RZ/A2, and is also not the bad
> commit?
> > > > > >
> > > > > > OK will use below one,
> > > > > >
> > > > > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes
> > > > > > to
> > > > > > dtsi")
> > > > >
> > > > > This really starts to look like a guessing game... Beep ;-)
> > > >
> > > > Already there is a generic compatible in driver, where we started
> > > > introducing RZ/A1 SoC With 4 interrupts. So addition of this SoC
> > > > has this
> > > issue. Am I missing anything here?
> > >
> > > The rabbit hole seems to be deeper than I thought...
> > >
> > > Looking at the code, the driver always assumed the fourth interrupt
> > > is BRI, which matches the RZ/A1 datasheet for SCIF.
> > > So the 4 IRQ case is really a subset of the 6 IRQ case, and
> > > Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > > is wrong.
> >
> > OK.
> >
> > >
> > > However, SCI(g) is the odd one (also on SH): it has TEI as the
> > > fourth IRQ, which I probably missed when doing the json-schema
> > > conversion in commit 384d00fae8e51f8f ("dt-bindings: serial: sh-sci:
> > > Convert to json-schema"), leading to the bug in scif.yaml.
> > >
> > > Note that the driver never looks at the interrupt names, but uses
> > > indices exclusively.
> > >
> > > So I guess SCI has been broken on SH since forever, too?
> >
> > I think so, by looking at the changes done in tx to make it work on
> RZ/G2UL.
> > On RZ/G2UL both rx and tx is broken.
> >
> > Not sure SCI is tested ever on SH platform??
> >
> > Can any SH platform person confirm this?
> 
> SCI is only supported on
>   - sh770x,
>   - sh7750 (excluding rts7751r2d)
>     I know it's not exposed on my landisk,
>   - sh7760, for the SIM-port, which I doubt anyone uses.
> 
> FTR, I tried the "obvious" thing (remove the rts7751r2d-checks in
> arch/sh/kernel/cpu/sh4/setup-sh7750.c, and replace sci_br_interrupt as the
> interrupt handler by sci_tx_interrupt in sh-sci.c), but that didn't make
> ttySC0 work on qemu/rts7751r2d.

I am not seeing any SH platform SoCs in[1] and RZ/A2 does not have any SCI nodes
defined in dts, 

So Shall I use the below fixes tag instead, as it is documented in dt bindings and is the first
SoC with broken irq handler??

Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")

With below check in driver.

+	/*
+	 * The fourth interrupt on SCI port is transmit end interrupt, so
+	 * shuffle the interrupts.
+	 */
+	if (p->type == PORT_SCI)
+		swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);


[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/serial/renesas,sci.yaml?h=next-20230317

Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Geert Uytterhoeven]

Hi Biju,

On Fri, Mar 17, 2023 at 2:47 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > On Fri, Mar 17, 2023 at 10:15 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > > handler On Fri, Mar 17, 2023 at 9:08 AM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > interrupt handler On Fri, Mar 17, 2023 at 8:59 AM Biju Das
> > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > > > interrupt handler On Thu, Mar 16, 2023 at 5:34 PM Biju Das
> > > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit
> > > > > > > > > > end interrupt handler
> > > > > > > >
> > > > > > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > > > wrote:
> > > > > > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The
> > > > > > > > > > > fourth interrupt is transmit end interrupt, so shuffle
> > > > > > > > > > > the interrupts to fix the transmit end interrupt
> > > > > > > > > > > handler for these
> > > > IPs.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > > > > > platform_get_irq_optional() for optional interrupts")
> > > > > > > > > >
> > > > > > > > > > I don't think that's the right bad commit.
> > > > > > > > >
> > > > > > > > > OK. I will use below commit as fixes one, that is the
> > > > > > > > > commit which added RZ/A1 SCIF with 4 interrupts.
> > > > > > > > >
> > > > > > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support for
> > > > > > > > > R7S9210")
> > > > > > > >
> > > > > > > > That one added support for RZ/A2, and is also not the bad
> > commit?
> > > > > > >
> > > > > > > OK will use below one,
> > > > > > >
> > > > > > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif nodes
> > > > > > > to
> > > > > > > dtsi")
> > > > > >
> > > > > > This really starts to look like a guessing game... Beep ;-)
> > > > >
> > > > > Already there is a generic compatible in driver, where we started
> > > > > introducing RZ/A1 SoC With 4 interrupts. So addition of this SoC
> > > > > has this
> > > > issue. Am I missing anything here?
> > > >
> > > > The rabbit hole seems to be deeper than I thought...
> > > >
> > > > Looking at the code, the driver always assumed the fourth interrupt
> > > > is BRI, which matches the RZ/A1 datasheet for SCIF.
> > > > So the 4 IRQ case is really a subset of the 6 IRQ case, and
> > > > Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > > > is wrong.
> > >
> > > OK.
> > >
> > > >
> > > > However, SCI(g) is the odd one (also on SH): it has TEI as the
> > > > fourth IRQ, which I probably missed when doing the json-schema
> > > > conversion in commit 384d00fae8e51f8f ("dt-bindings: serial: sh-sci:
> > > > Convert to json-schema"), leading to the bug in scif.yaml.
> > > >
> > > > Note that the driver never looks at the interrupt names, but uses
> > > > indices exclusively.
> > > >
> > > > So I guess SCI has been broken on SH since forever, too?
> > >
> > > I think so, by looking at the changes done in tx to make it work on
> > RZ/G2UL.
> > > On RZ/G2UL both rx and tx is broken.
> > >
> > > Not sure SCI is tested ever on SH platform??
> > >
> > > Can any SH platform person confirm this?
> >
> > SCI is only supported on
> >   - sh770x,
> >   - sh7750 (excluding rts7751r2d)
> >     I know it's not exposed on my landisk,
> >   - sh7760, for the SIM-port, which I doubt anyone uses.
> >
> > FTR, I tried the "obvious" thing (remove the rts7751r2d-checks in
> > arch/sh/kernel/cpu/sh4/setup-sh7750.c, and replace sci_br_interrupt as the
> > interrupt handler by sci_tx_interrupt in sh-sci.c), but that didn't make
> > ttySC0 work on qemu/rts7751r2d.
>
> I am not seeing any SH platform SoCs in[1] and RZ/A2 does not have any SCI nodes
> defined in dts,

Most SH platforms have not been converted to DT yet:

$ git grep -w PORT_SCI -- arch/sh
arch/sh/kernel/cpu/sh3/setup-sh770x.c:  .type           = PORT_SCI,
arch/sh/kernel/cpu/sh4/setup-sh7750.c:  .type           = PORT_SCI,
arch/sh/kernel/cpu/sh4/setup-sh7760.c:  .type           = PORT_SCI,

But I just realized the above are not affected, as they define either
1 or 3 interrupts for the SCI port instead of.

> So Shall I use the below fixes tag instead, as it is documented in dt bindings and is the first
> SoC with broken irq handler??
>
> Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")

That's a DTS change, while the bug is in the driver?

The bug seems to be present in all versions since modern git of the
sh-sci serial driver.
More archaeology shows early versions used hardcoded lists of 3
interrupts for SCI, avoiding the issue. The even older sh-sci character
device driver registered only 3 interrupt handlers when built with
SCI support only.

So the issue only started to appear (if anyone noticed at all) with the
(removed) DT-based H8/300 architecture, which described 4 interrupts
in DT, which the sh-sci driver handles incorrectly.

So if you insist on a Fixes line:
Fixes: e1d0be616186906d ("sh-sci: Add h8300 SCI")

> With below check in driver.
>
> +       /*
> +        * The fourth interrupt on SCI port is transmit end interrupt, so
> +        * shuffle the interrupts.
> +        */
> +       if (p->type == PORT_SCI)
> +               swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);

Thanks, LGTM.

Now back to the present time, I had enough archaeology ;-)

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] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Geert,

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Biju,
> 
> On Fri, Mar 17, 2023 at 2:47 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler On Fri, Mar 17, 2023 at 10:15 AM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler On Fri, Mar 17, 2023 at 9:08 AM Biju Das
> > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > > interrupt handler On Fri, Mar 17, 2023 at 8:59 AM Biju Das
> > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit
> > > > > > > > > end interrupt handler On Thu, Mar 16, 2023 at 5:34 PM
> > > > > > > > > Biju Das
> > > > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix
> > > > > > > > > > > transmit end interrupt handler
> > > > > > > > >
> > > > > > > > > > > On Thu, Mar 16, 2023 at 5:01 PM Biju Das
> > > > > > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > > > > wrote:
> > > > > > > > > > > > The RZ SCI/ RZ/A1 SCIF has only 4 interrupts. The
> > > > > > > > > > > > fourth interrupt is transmit end interrupt, so
> > > > > > > > > > > > shuffle the interrupts to fix the transmit end
> > > > > > > > > > > > interrupt handler for these
> > > > > IPs.
> > > > > > > > > > > >
> > > > > > > > > > > > Fixes: 392fb8df528b ("serial: sh-sci: Use
> > > > > > > > > > > > platform_get_irq_optional() for optional
> > > > > > > > > > > > interrupts")
> > > > > > > > > > >
> > > > > > > > > > > I don't think that's the right bad commit.
> > > > > > > > > >
> > > > > > > > > > OK. I will use below commit as fixes one, that is the
> > > > > > > > > > commit which added RZ/A1 SCIF with 4 interrupts.
> > > > > > > > > >
> > > > > > > > > > commit 8b0bbd956228ae87 ("serial: sh-sci: Add support
> > > > > > > > > > for
> > > > > > > > > > R7S9210")
> > > > > > > > >
> > > > > > > > > That one added support for RZ/A2, and is also not the
> > > > > > > > > bad
> > > commit?
> > > > > > > >
> > > > > > > > OK will use below one,
> > > > > > > >
> > > > > > > > Fixes: 4c84c1b3acca ("ARM: shmobile: r7s72100: add scif
> > > > > > > > nodes to
> > > > > > > > dtsi")
> > > > > > >
> > > > > > > This really starts to look like a guessing game... Beep ;-)
> > > > > >
> > > > > > Already there is a generic compatible in driver, where we
> > > > > > started introducing RZ/A1 SoC With 4 interrupts. So addition
> > > > > > of this SoC has this
> > > > > issue. Am I missing anything here?
> > > > >
> > > > > The rabbit hole seems to be deeper than I thought...
> > > > >
> > > > > Looking at the code, the driver always assumed the fourth
> > > > > interrupt is BRI, which matches the RZ/A1 datasheet for SCIF.
> > > > > So the 4 IRQ case is really a subset of the 6 IRQ case, and
> > > > > Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > > > > is wrong.
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > However, SCI(g) is the odd one (also on SH): it has TEI as the
> > > > > fourth IRQ, which I probably missed when doing the json-schema
> > > > > conversion in commit 384d00fae8e51f8f ("dt-bindings: serial: sh-sci:
> > > > > Convert to json-schema"), leading to the bug in scif.yaml.
> > > > >
> > > > > Note that the driver never looks at the interrupt names, but
> > > > > uses indices exclusively.
> > > > >
> > > > > So I guess SCI has been broken on SH since forever, too?
> > > >
> > > > I think so, by looking at the changes done in tx to make it work
> > > > on
> > > RZ/G2UL.
> > > > On RZ/G2UL both rx and tx is broken.
> > > >
> > > > Not sure SCI is tested ever on SH platform??
> > > >
> > > > Can any SH platform person confirm this?
> > >
> > > SCI is only supported on
> > >   - sh770x,
> > >   - sh7750 (excluding rts7751r2d)
> > >     I know it's not exposed on my landisk,
> > >   - sh7760, for the SIM-port, which I doubt anyone uses.
> > >
> > > FTR, I tried the "obvious" thing (remove the rts7751r2d-checks in
> > > arch/sh/kernel/cpu/sh4/setup-sh7750.c, and replace sci_br_interrupt
> > > as the interrupt handler by sci_tx_interrupt in sh-sci.c), but that
> > > didn't make
> > > ttySC0 work on qemu/rts7751r2d.
> >
> > I am not seeing any SH platform SoCs in[1] and RZ/A2 does not have any
> > SCI nodes defined in dts,
> 
> Most SH platforms have not been converted to DT yet:
> 
> $ git grep -w PORT_SCI -- arch/sh
> arch/sh/kernel/cpu/sh3/setup-sh770x.c:  .type           = PORT_SCI,
> arch/sh/kernel/cpu/sh4/setup-sh7750.c:  .type           = PORT_SCI,
> arch/sh/kernel/cpu/sh4/setup-sh7760.c:  .type           = PORT_SCI,
> 
> But I just realized the above are not affected, as they define either
> 1 or 3 interrupts for the SCI port instead of.

OK.

> 
> > So Shall I use the below fixes tag instead, as it is documented in dt
> > bindings and is the first SoC with broken irq handler??
> >
> > Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1]
> > nodes")
> 
> That's a DTS change, while the bug is in the driver?
> 
> The bug seems to be present in all versions since modern git of the sh-sci
> serial driver.
> More archaeology shows early versions used hardcoded lists of 3 interrupts
> for SCI, avoiding the issue. The even older sh-sci character device driver
> registered only 3 interrupt handlers when built with SCI support only.
> 
> So the issue only started to appear (if anyone noticed at all) with the
> (removed) DT-based H8/300 architecture, which described 4 interrupts in DT,
> which the sh-sci driver handles incorrectly.
> 
> So if you insist on a Fixes line:
> Fixes: e1d0be616186906d ("sh-sci: Add h8300 SCI")

Thanks, I will use this.

> 
> > With below check in driver.
> >
> > +       /*
> > +        * The fourth interrupt on SCI port is transmit end interrupt, so
> > +        * shuffle the interrupts.
> > +        */
> > +       if (p->type == PORT_SCI)
> > +               swap(sci_port->irqs[SCIx_BRI_IRQ],
> > + sci_port->irqs[SCIx_TEI_IRQ]);
> 
> Thanks, LGTM.

OK, Will send next version with these changes.

Cheers,
Biju

[PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.

The fourth interrupt on SCI port is transmit end interrupt compared to
the break interrupt on other port types. So, shuffle the interrupts to fix
the transmit end interrupt handler.

Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
Cc: stable <stable@kernel.org>
Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Link: https://lore.kernel.org/r/20230317150403.154094-1-biju.das.jz@bp.renesas.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[biju: manually fixed the conflicts]
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
Resending to 4.14 with confilcts [1] fixed.
[1] https://lore.kernel.org/stable/2023041046-synthetic-urgent-3126@gregkh/T/#u
---
 drivers/tty/serial/sh-sci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index f7dd843a3eff..7cf95ffad4c9 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -37,6 +37,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/major.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/of.h>
@@ -2775,6 +2776,13 @@ static int sci_init_single(struct platform_device *dev,
 	for (i = 0; i < ARRAY_SIZE(sci_port->irqs); ++i)
 		sci_port->irqs[i] = platform_get_irq(dev, i);
 
+	/*
+	 * The fourth interrupt on SCI port is transmit end interrupt, so
+	 * shuffle the interrupts.
+	 */
+	if (p->type == PORT_SCI)
+		swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);
+
 	/* 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
-- 
2.25.1

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Greg Kroah-Hartman]

On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> 
> The fourth interrupt on SCI port is transmit end interrupt compared to
> the break interrupt on other port types. So, shuffle the interrupts to fix
> the transmit end interrupt handler.
> 
> Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> Cc: stable <stable@kernel.org>
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Link: https://lore.kernel.org/r/20230317150403.154094-1-biju.das.jz@bp.renesas.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> [biju: manually fixed the conflicts]
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> Resending to 4.14 with confilcts [1] fixed.
> [1] https://lore.kernel.org/stable/2023041046-synthetic-urgent-3126@gregkh/T/#u

You did not actually build your patch, as it breaks the build :(

RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Greg,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> > commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> >
> > The fourth interrupt on SCI port is transmit end interrupt compared to
> > the break interrupt on other port types. So, shuffle the interrupts to
> > fix the transmit end interrupt handler.
> >
> > Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> > Cc: stable <stable@kernel.org>
> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Link:
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > [biju: manually fixed the conflicts]
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > Resending to 4.14 with confilcts [1] fixed.
> > [1]
> 
> You did not actually build your patch, as it breaks the build :(

Actually, I build the patch, but did not test it on target as I don't have the platform
to test it.

I got some issues while building modules which is unrelated to this change.

Anyway, I will double check again and confirm.

Cheers,
Biju

RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Greg,

> Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> Hi Greg,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > handler
> >
> > On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> > > commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> > >
> > > The fourth interrupt on SCI port is transmit end interrupt compared
> > > to the break interrupt on other port types. So, shuffle the
> > > interrupts to fix the transmit end interrupt handler.
> > >
> > > Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> > > Cc: stable <stable@kernel.org>
> > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Link:
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > [biju: manually fixed the conflicts]
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > Resending to 4.14 with confilcts [1] fixed.
> > > [1]
> >
> > You did not actually build your patch, as it breaks the build :(
> 
> Actually, I build the patch, but did not test it on target as I don't have
> the platform to test it.
> 
> I got some issues while building modules which is unrelated to this change.
> 
> Anyway, I will double check again and confirm.

I confirm, there is an issue with this patch.

I disabled building modules from my build script and it showed the below error. 
So I would like to drop this patch for 4.14 as this header file does not exist for 4.14.

drivers/tty/serial/sh-sci.c:40:10: fatal error: linux/minmax.h: No such file or directory
   40 | #include <linux/minmax.h>
      |          ^~~~~~~~~~~~~~~~

Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Greg Kroah-Hartman]

On Tue, Apr 11, 2023 at 02:40:52PM +0000, Biju Das wrote:
> Hi Greg,
> 
> > Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > 
> > Hi Greg,
> > 
> > Thanks for the feedback.
> > 
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler
> > >
> > > On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> > > > commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> > > >
> > > > The fourth interrupt on SCI port is transmit end interrupt compared
> > > > to the break interrupt on other port types. So, shuffle the
> > > > interrupts to fix the transmit end interrupt handler.
> > > >
> > > > Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> > > > Cc: stable <stable@kernel.org>
> > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Link:
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > [biju: manually fixed the conflicts]
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > ---
> > > > Resending to 4.14 with confilcts [1] fixed.
> > > > [1]
> > >
> > > You did not actually build your patch, as it breaks the build :(
> > 
> > Actually, I build the patch, but did not test it on target as I don't have
> > the platform to test it.
> > 
> > I got some issues while building modules which is unrelated to this change.
> > 
> > Anyway, I will double check again and confirm.
> 
> I confirm, there is an issue with this patch.
> 
> I disabled building modules from my build script and it showed the below error. 
> So I would like to drop this patch for 4.14 as this header file does not exist for 4.14.
> 
> drivers/tty/serial/sh-sci.c:40:10: fatal error: linux/minmax.h: No such file or directory
>    40 | #include <linux/minmax.h>
>       |          ^~~~~~~~~~~~~~~~

Yes, minmax is not there, but the function needed by that is there
(hint, I had to remove that include in 4.19).  Remove it and see the
next error you get :)

thanks,

greg k-h

RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Greg,

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> On Tue, Apr 11, 2023 at 02:40:52PM +0000, Biju Das wrote:
> > Hi Greg,
> >
> > > Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler
> > >
> > > Hi Greg,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > interrupt handler
> > > >
> > > > On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> > > > > commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> > > > >
> > > > > The fourth interrupt on SCI port is transmit end interrupt
> > > > > compared to the break interrupt on other port types. So, shuffle
> > > > > the interrupts to fix the transmit end interrupt handler.
> > > > >
> > > > > Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> > > > > Cc: stable <stable@kernel.org>
> > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Link:
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > [biju: manually fixed the conflicts]
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > ---
> > > > > Resending to 4.14 with confilcts [1] fixed.
> > > > > [1]
> > > >
> > > > You did not actually build your patch, as it breaks the build :(
> > >
> > > Actually, I build the patch, but did not test it on target as I
> > > don't have the platform to test it.
> > >
> > > I got some issues while building modules which is unrelated to this
> change.
> > >
> > > Anyway, I will double check again and confirm.
> >
> > I confirm, there is an issue with this patch.
> >
> > I disabled building modules from my build script and it showed the below
> error.
> > So I would like to drop this patch for 4.14 as this header file does not
> exist for 4.14.
> >
> > drivers/tty/serial/sh-sci.c:40:10: fatal error: linux/minmax.h: No such
> file or directory
> >    40 | #include <linux/minmax.h>
> >       |          ^~~~~~~~~~~~~~~~
> 
> Yes, minmax is not there, but the function needed by that is there (hint, I
> had to remove that include in 4.19).  Remove it and see the next error you
> get :)
> 

OK got it, The SCIx_TEI_IRQ  is introduced after 4.18 by patch [1]. 

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/tty/serial/sh-sci.c?h=linux-5.4.y&id=628c534ae73581fd21a09a27b7a4222b01a44d64

So, 4.14 does not require this patch, as it have combined interrupt.

Cheers,
Biju

Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Greg Kroah-Hartman]

On Tue, Apr 11, 2023 at 03:36:04PM +0000, Biju Das wrote:
> 
> Hi Greg,
> 
> > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> > 
> > On Tue, Apr 11, 2023 at 02:40:52PM +0000, Biju Das wrote:
> > > Hi Greg,
> > >
> > > > Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > > handler
> > > >
> > > > Hi Greg,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler
> > > > >
> > > > > On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> > > > > > commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> > > > > >
> > > > > > The fourth interrupt on SCI port is transmit end interrupt
> > > > > > compared to the break interrupt on other port types. So, shuffle
> > > > > > the interrupts to fix the transmit end interrupt handler.
> > > > > >
> > > > > > Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> > > > > > Cc: stable <stable@kernel.org>
> > > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > Link:
> > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > [biju: manually fixed the conflicts]
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > ---
> > > > > > Resending to 4.14 with confilcts [1] fixed.
> > > > > > [1]
> > > > >
> > > > > You did not actually build your patch, as it breaks the build :(
> > > >
> > > > Actually, I build the patch, but did not test it on target as I
> > > > don't have the platform to test it.
> > > >
> > > > I got some issues while building modules which is unrelated to this
> > change.
> > > >
> > > > Anyway, I will double check again and confirm.
> > >
> > > I confirm, there is an issue with this patch.
> > >
> > > I disabled building modules from my build script and it showed the below
> > error.
> > > So I would like to drop this patch for 4.14 as this header file does not
> > exist for 4.14.
> > >
> > > drivers/tty/serial/sh-sci.c:40:10: fatal error: linux/minmax.h: No such
> > file or directory
> > >    40 | #include <linux/minmax.h>
> > >       |          ^~~~~~~~~~~~~~~~
> > 
> > Yes, minmax is not there, but the function needed by that is there (hint, I
> > had to remove that include in 4.19).  Remove it and see the next error you
> > get :)
> > 
> 
> OK got it, The SCIx_TEI_IRQ  is introduced after 4.18 by patch [1]. 
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/tty/serial/sh-sci.c?h=linux-5.4.y&id=628c534ae73581fd21a09a27b7a4222b01a44d64
> 
> So, 4.14 does not require this patch, as it have combined interrupt.

Great, thanks for checking, it turns out that the Fixes: tag was wrong :(

RE: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

Hi Greg,

Thanks for the feedback.

> Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt handler
> 
> On Tue, Apr 11, 2023 at 03:36:04PM +0000, Biju Das wrote:
> >
> > Hi Greg,
> >
> > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end interrupt
> > > handler
> > >
> > > On Tue, Apr 11, 2023 at 02:40:52PM +0000, Biju Das wrote:
> > > > Hi Greg,
> > > >
> > > > > Subject: RE: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > interrupt handler
> > > > >
> > > > > Hi Greg,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH] tty: serial: sh-sci: Fix transmit end
> > > > > > interrupt handler
> > > > > >
> > > > > > On Tue, Apr 11, 2023 at 11:08:59AM +0100, Biju Das wrote:
> > > > > > > commit b43a18647f03c87e77d50d6fe74904b61b96323e upstream.
> > > > > > >
> > > > > > > The fourth interrupt on SCI port is transmit end interrupt
> > > > > > > compared to the break interrupt on other port types. So,
> > > > > > > shuffle the interrupts to fix the transmit end interrupt
> handler.
> > > > > > >
> > > > > > > Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
> > > > > > > Cc: stable <stable@kernel.org>
> > > > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > Link:
> > > > > > > Signed-off-by: Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org>
> > > > > > > [biju: manually fixed the conflicts]
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > ---
> > > > > > > Resending to 4.14 with confilcts [1] fixed.
> > > > > > > [1]
> > > > > >
> > > > > > You did not actually build your patch, as it breaks the build
> > > > > > :(
> > > > >
> > > > > Actually, I build the patch, but did not test it on target as I
> > > > > don't have the platform to test it.
> > > > >
> > > > > I got some issues while building modules which is unrelated to
> > > > > this
> > > change.
> > > > >
> > > > > Anyway, I will double check again and confirm.
> > > >
> > > > I confirm, there is an issue with this patch.
> > > >
> > > > I disabled building modules from my build script and it showed the
> > > > below
> > > error.
> > > > So I would like to drop this patch for 4.14 as this header file
> > > > does not
> > > exist for 4.14.
> > > >
> > > > drivers/tty/serial/sh-sci.c:40:10: fatal error: linux/minmax.h: No
> > > > such
> > > file or directory
> > > >    40 | #include <linux/minmax.h>
> > > >       |          ^~~~~~~~~~~~~~~~
> > >
> > > Yes, minmax is not there, but the function needed by that is there
> > > (hint, I had to remove that include in 4.19).  Remove it and see the
> > > next error you get :)
> > >
> >
> > OK got it, The SCIx_TEI_IRQ  is introduced after 4.18 by patch [1].
> >
> > [1]
> >
> > So, 4.14 does not require this patch, as it have combined interrupt.
> 
> Great, thanks for checking, it turns out that the Fixes: tag was wrong :(

I agree. It is my fault, next time I will take care this.

Cheers,
Biju