diff mbox series

[RESEND,RFC,10/12] serial/sh-sci: Fix earlyprintk / earlycon.

Message ID 8f12d3b4b5fdeae4e465fc8fbf843e2878f17b55.1693444193.git.ysato@users.sourceforge.jp (mailing list archive)
State New, archived
Headers show
Series DeviceTree support for SH7751 based boards. | expand

Commit Message

Yoshinori Sato Aug. 31, 2023, 1:11 a.m. UTC
- earlyprintk having fault.
- fix shortname conflict.
- fix SCI regshift in SH4.

Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 drivers/tty/serial/sh-sci.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven Sept. 1, 2023, 1:26 p.m. UTC | #1
Hi Sato-san,

On Thu, Aug 31, 2023 at 11:23 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> - earlyprintk having fault.
> - fix shortname conflict.
> - fix SCI regshift in SH4.
>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2721,7 +2721,7 @@ static int sci_remap_port(struct uart_port *port)
>         if (port->membase)
>                 return 0;
>
> -       if (port->dev->of_node || (port->flags & UPF_IOREMAP)) {
> +       if ((port->dev && port->dev->of_node) || (port->flags & UPF_IOREMAP)) {

Why is this needed? It works fine on arm32, arm64, and riscv.

>                 port->membase = ioremap(port->mapbase, sport->reg_size);
>                 if (unlikely(!port->membase)) {
>                         dev_err(port->dev, "can't remap port#%d\n", port->line);
> @@ -3507,6 +3507,10 @@ static int __init early_console_setup(struct earlycon_device *device,
>         if (!device->port.membase)
>                 return -ENODEV;
>
> +       if (type == PORT_SCI &&
> +           (IS_ENABLED(CONFIG_CPU_SH3) || IS_ENABLED(CONFIG_CPU_SH4))) {
> +               device->port.regshift = 2;
> +       }

How do you run into this, given you don't have any SCI ports enabled
in your DTS, only SCIF?

Anyway, I think this should be fixed in sci_init_single(), by extending
the driver to support the more-or-less standard "reg-shift" DT property
(as I said before in
https://lore.kernel.org/all/CAMuHMdW2gxDzYbP_0Z90o8mHdUm_BV6e+gMHpELJx_g=ezAbdw@mail.gmail.com).

>         device->port.serial_in = sci_serial_in;
>         device->port.serial_out = sci_serial_out;
>         device->port.type = type;
> @@ -3556,8 +3560,8 @@ static int __init hscif_early_console_setup(struct earlycon_device *device,
>
>  OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
>  OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
> -OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> -OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
> +OF_EARLYCON_DECLARE(rzscif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> +OF_EARLYCON_DECLARE(rzscif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);

Why?

>  OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
>  OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
>  OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);

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
Yoshinori Sato Sept. 6, 2023, 7:27 a.m. UTC | #2
On Fri, 01 Sep 2023 22:26:14 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> On Thu, Aug 31, 2023 at 11:23 AM Yoshinori Sato
> <ysato@users.sourceforge.jp> wrote:
> > - earlyprintk having fault.
> > - fix shortname conflict.
> > - fix SCI regshift in SH4.
> >
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> Thanks for your patch!
> 
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -2721,7 +2721,7 @@ static int sci_remap_port(struct uart_port *port)
> >         if (port->membase)
> >                 return 0;
> >
> > -       if (port->dev->of_node || (port->flags & UPF_IOREMAP)) {
> > +       if ((port->dev && port->dev->of_node) || (port->flags & UPF_IOREMAP)) {
> 
> Why is this needed? It works fine on arm32, arm64, and riscv.

earlycon works fine, but the old earlyprintk was stuck.
I don't think it's used much, but it's a problem if it doesn't work
when checking old sh targets.

> >                 port->membase = ioremap(port->mapbase, sport->reg_size);
> >                 if (unlikely(!port->membase)) {
> >                         dev_err(port->dev, "can't remap port#%d\n", port->line);
> > @@ -3507,6 +3507,10 @@ static int __init early_console_setup(struct earlycon_device *device,
> >         if (!device->port.membase)
> >                 return -ENODEV;
> >
> > +       if (type == PORT_SCI &&
> > +           (IS_ENABLED(CONFIG_CPU_SH3) || IS_ENABLED(CONFIG_CPU_SH4))) {
> > +               device->port.regshift = 2;
> > +       }
> 
> How do you run into this, given you don't have any SCI ports enabled
> in your DTS, only SCIF?
> 
> Anyway, I think this should be fixed in sci_init_single(), by extending
> the driver to support the more-or-less standard "reg-shift" DT property
> (as I said before in
> https://lore.kernel.org/all/CAMuHMdW2gxDzYbP_0Z90o8mHdUm_BV6e+gMHpELJx_g=ezAbdw@mail.gmail.com).

earlycon cannot reference DeviceTree.
If you give the value of reg-shift dynamically, you need to use a kernel
parameter.
I don't think this is necessary either, but it's a problem if it doesn't work.

> >         device->port.serial_in = sci_serial_in;
> >         device->port.serial_out = sci_serial_out;
> >         device->port.type = type;
> > @@ -3556,8 +3560,8 @@ static int __init hscif_early_console_setup(struct earlycon_device *device,
> >
> >  OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
> >  OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
> > -OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> > -OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
> > +OF_EARLYCON_DECLARE(rzscif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
> > +OF_EARLYCON_DECLARE(rzscif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
> 
> Why?

The earlycon parameter uses the first name.
If sh4 and rz use the same name, one of them will not work properly.

> 
> >  OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
> >  OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
> >  OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);
> 
> 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
diff mbox series

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 7c9457962a3d..541466b23b45 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2721,7 +2721,7 @@  static int sci_remap_port(struct uart_port *port)
 	if (port->membase)
 		return 0;
 
-	if (port->dev->of_node || (port->flags & UPF_IOREMAP)) {
+	if ((port->dev && port->dev->of_node) || (port->flags & UPF_IOREMAP)) {
 		port->membase = ioremap(port->mapbase, sport->reg_size);
 		if (unlikely(!port->membase)) {
 			dev_err(port->dev, "can't remap port#%d\n", port->line);
@@ -3507,6 +3507,10 @@  static int __init early_console_setup(struct earlycon_device *device,
 	if (!device->port.membase)
 		return -ENODEV;
 
+	if (type == PORT_SCI &&
+	    (IS_ENABLED(CONFIG_CPU_SH3) || IS_ENABLED(CONFIG_CPU_SH4))) {
+		device->port.regshift = 2;
+	}
 	device->port.serial_in = sci_serial_in;
 	device->port.serial_out	= sci_serial_out;
 	device->port.type = type;
@@ -3556,8 +3560,8 @@  static int __init hscif_early_console_setup(struct earlycon_device *device,
 
 OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup);
 OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup);
-OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
-OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
+OF_EARLYCON_DECLARE(rzscif, "renesas,scif-r7s9210", rzscifa_early_console_setup);
+OF_EARLYCON_DECLARE(rzscif, "renesas,scif-r9a07g044", rzscifa_early_console_setup);
 OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup);
 OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup);
 OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);