Message-ID: <CAMuHMdWx97OnPWnQn78oL+vVuQXmeaJP-byc_4ZwBMZhMOorxw@mail.gmail.com>
Date: Thu, 19 Dec 2024 16:11:59 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>, gregkh@...uxfoundation.org, 
	jirislaby@...nel.org, prabhakar.mahadev-lad.rj@...renesas.com, 
	lethal@...ux-sh.org, g.liakhovetski@....de, groeck@...omium.org, 
	mka@...omium.org, ulrich.hecht+renesas@...il.com, ysato@...rs.sourceforge.jp, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

Hi Claudiu,

On Thu, Dec 5, 2024 at 9:39 AM Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> On 04.12.2024 23:38, Wolfram Sang wrote:
> >> in the following scenarios:
> >>
> >> 1/ "earlycon keep_bootcon" were present in bootargs
> >> 2/ only "earlycon" was present in bootargs
> >> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
> >>    bootargs
> > ...
> >> Please give it a try on your devices as well.
> >
> > Will happily do so. Is there something to look for? Except for "it
> > works"?
>
> As this code touches the earlycon functionality, of interest are the 3
> cases highlighted above:
>
> 1/ "earlycon keep_bootcon" are both present in bootargs
> 2/ only "earlycon" is present in bootargs
> 3/ none of the "earlycon" or "earlycon keep_bootcon" are present in
>    bootargs
>
> One other thing, that I was currently able to test only on RZ/G3S, is to
> see how it behaves when the debug serial is described in DT with an alias
> other than zero. E.g., on [1] the debug serial alias on RZ/G3S was changed
> from 0 to 3. With the new alias (3) there were issues that I've tried to
> fix with this series.

I gave this a try on Koelsch, which has two easily-accessible usb-serial
ports, for all three cases above.  Originally, I had CONFIG_VT_CONSOLE=y
(tty0 takes over from earlycon rather early), but I had to disable
that to exercise all code paths (ttySC0 takes over much later).

  A. CONFIG_VT_CONSOLE=y: OK
  B. CONFIG_VT_CONSOLE=y earlycon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Clearing sci_ports[0]
  C. CONFIG_VT_CONSOLE=n earlycon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [ttySC0] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Not clearing sci_ports[0]
  D. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled

So all good, but note that these cases worked fine without your
series, too.

The real troublesome cases involve using earlycon on a different
serial port than serial0.  As I don't have any Renesas boards where
chosen/stdout-path does not use serial0, I tried exchanging the serial0
and serial1 DT aliases, and updating chosen/stdout-path accordingly.

  E. CONFIG_VT_CONSOLE=y: OK
  F. CONFIG_VT_CONSOLE=y earlycon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Clearing sci_ports[0]
  G. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: SCIF1 missing
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled
       sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!
  H. CONFIG_VT_CONSOLE=n earlycon: SCIF1 missing
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [ttySC1] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Not clearing sci_ports[0]
       sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!

Case G gives a missing SCIF1, because sci_port[0] is still
used for earlycon, as expected.
Case H also gives a missing SCIF1, but should succeed IMHO, as earlycon
is no longer active.  I think early_console_exit() should clear the
earlycon flag regardless.

Note that before your series, cases E-F worked too, but cases G-H gave
an initialized but broken SCIF1 instead.

Now, can we improve?
  - Can we use a proper id instead of zero for earlycon, e.g.
    sci_probe_earlyprintk() does fill in early_serial_console.index?
  - Alternatively, can we use a separate sci_port structure instead
    of abusing sci_ports[0]?

Thanks!

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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

Powered by blists - more mailing lists