| Message ID | 20230316160118.133182-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | tty: serial: sh-sci: Fix transmit end interrupt handler | expand |
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
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
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
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
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
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
+ 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
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
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
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
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
> This is all for a Fixes: tag, or? Is it that important?
Reading the other thread: yes, it is important!
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
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
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
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
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
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.
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(+)