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

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
> <[email protected]> wrote:
> > - earlyprintk having fault.
> > - fix shortname conflict.
> > - fix SCI regshift in SH4.
> >
> > Signed-off-by: Yoshinori Sato <[email protected]>
> 
> 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 -- [email protected]
> 
> 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

-- 
Yosinori Sato