| Message ID | 20210215121713.57687-24-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
On Mon, Feb 15, 2021 at 09:17:11PM +0900, Hector Martin wrote: > Earlycon support is identical to S3C2410, but Apple SoCs also need > MMIO mapped as nGnRnE. This is handled generically for normal drivers > including the normal UART path here, but earlycon uses fixmap and > runs before that scaffolding is ready. > > Since this is the only case where we need this fix, it makes more > sense to do it here in the UART driver instead of introducing a > whole fdt nonposted-mmio resolver just for earlycon/fixmap. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/tty/serial/samsung_tty.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index e7ab0b9d89a7..00262f0e704b 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -2988,6 +2988,23 @@ OF_EARLYCON_DECLARE(s5pv210, "samsung,s5pv210-uart", > s5pv210_early_console_setup); > OF_EARLYCON_DECLARE(exynos4210, "samsung,exynos4210-uart", > s5pv210_early_console_setup); > + > +/* Apple S5L */ > +static int __init apple_s5l_early_console_setup(struct earlycon_device *device, > + const char *opt) > +{ > + /* Close enough to S3C2410 for earlycon... */ > + device->port.private_data = &s3c2410_early_console_data; > + > +#ifdef CONFIG_ARM64 if IS_ENABLED() (unless it cannot be used due to missing symbol?) > + /* ... but we need to override the existing fixmap entry as nGnRnE */ > + __set_fixmap(FIX_EARLYCON_MEM_BASE, device->port.mapbase, > + __pgprot(PROT_DEVICE_nGnRnE)); > +#endif > + return samsung_early_console_setup(device, opt); Don't you need to handle the error code - set PROT_DEFAULT() or whatever was there before? Best regards, Krzysztof
On Mon, Feb 15, 2021 at 8:19 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Mon, Feb 15, 2021 at 09:17:11PM +0900, Hector Martin wrote: > > + > > +/* Apple S5L */ > > +static int __init apple_s5l_early_console_setup(struct earlycon_device *device, > > + const char *opt) > > +{ > > + /* Close enough to S3C2410 for earlycon... */ > > + device->port.private_data = &s3c2410_early_console_data; > > + > > +#ifdef CONFIG_ARM64 > > if IS_ENABLED() > (unless it cannot be used due to missing symbol?) > > > + /* ... but we need to override the existing fixmap entry as nGnRnE */ > > + __set_fixmap(FIX_EARLYCON_MEM_BASE, device->port.mapbase, > > + __pgprot(PROT_DEVICE_nGnRnE)); > > +#endif It has to be a preprocessor conditional because PROT_DEVICE_nGnRnE is only defined on arm64. We could add a FIXMAP_PAGE_NONPOSTED alias for it that defaults to FIXMAP_PAGE_IO, but in the end this is really an architecture specific thing and I think leaving it guarded by the architecture is appropriate. > > + return samsung_early_console_setup(device, opt); > > Don't you need to handle the error code - set PROT_DEFAULT() or whatever > was there before? __set_fixmap() has no return value, it just writes a page table entry and does not fail. Arnd
On Tue, 16 Feb 2021 at 11:19, Arnd Bergmann <arnd@kernel.org> wrote: > > > + return samsung_early_console_setup(device, opt); > > > > Don't you need to handle the error code - set PROT_DEFAULT() or whatever > > was there before? > > __set_fixmap() has no return value, it just writes a page table entry and > does not fail. I meant, handle samsung_early_console_setup() error code (NULL). Best regards, Krzysztof
On Tue, Feb 16, 2021 at 11:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Tue, 16 Feb 2021 at 11:19, Arnd Bergmann <arnd@kernel.org> wrote: > > > > + return samsung_early_console_setup(device, opt); > > > > > > Don't you need to handle the error code - set PROT_DEFAULT() or whatever > > > was there before? > > > > __set_fixmap() has no return value, it just writes a page table entry and > > does not fail. > > I meant, handle samsung_early_console_setup() error code (NULL). Ah, I see. I don't think it makes a difference -- if ->setup() fails, the page table entry is just left in place unused, and the type of the unused mapping doesn't matter. If earlycon tried to unmap the page, the type also would not change anything. With earlycon, I'd generally lean towards keeping things as simple as possible, in order to increase the chance of seeing anything at all. It clearly wouldn't hurt to try to add minimal error handling here. Arnd
On 16/02/2021 19.29, Arnd Bergmann wrote: > On Tue, Feb 16, 2021 at 11:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On Tue, 16 Feb 2021 at 11:19, Arnd Bergmann <arnd@kernel.org> wrote: >>>>> + return samsung_early_console_setup(device, opt); >>>> >>>> Don't you need to handle the error code - set PROT_DEFAULT() or whatever >>>> was there before? >>> >>> __set_fixmap() has no return value, it just writes a page table entry and >>> does not fail. >> >> I meant, handle samsung_early_console_setup() error code (NULL). > > Ah, I see. > > I don't think it makes a difference -- if ->setup() fails, the page table entry > is just left in place unused, and the type of the unused mapping doesn't > matter. If earlycon tried to unmap the page, the type also would not > change anything. > > With earlycon, I'd generally lean towards keeping things as simple as possible, > in order to increase the chance of seeing anything at all. It clearly wouldn't > hurt to try to add minimal error handling here. There's no logic to clean this up in earlycon itself anyway, so there's no point in trying to do it for the override. If another earlycon driver ends up getting instantiated for some reason, it will override the mapping with a normal one again.
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index e7ab0b9d89a7..00262f0e704b 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -2988,6 +2988,23 @@ OF_EARLYCON_DECLARE(s5pv210, "samsung,s5pv210-uart", s5pv210_early_console_setup); OF_EARLYCON_DECLARE(exynos4210, "samsung,exynos4210-uart", s5pv210_early_console_setup); + +/* Apple S5L */ +static int __init apple_s5l_early_console_setup(struct earlycon_device *device, + const char *opt) +{ + /* Close enough to S3C2410 for earlycon... */ + device->port.private_data = &s3c2410_early_console_data; + +#ifdef CONFIG_ARM64 + /* ... but we need to override the existing fixmap entry as nGnRnE */ + __set_fixmap(FIX_EARLYCON_MEM_BASE, device->port.mapbase, + __pgprot(PROT_DEVICE_nGnRnE)); +#endif + return samsung_early_console_setup(device, opt); +} + +OF_EARLYCON_DECLARE(s5l, "apple,s5l-uart", apple_s5l_early_console_setup); #endif MODULE_ALIAS("platform:samsung-uart");
Earlycon support is identical to S3C2410, but Apple SoCs also need MMIO mapped as nGnRnE. This is handled generically for normal drivers including the normal UART path here, but earlycon uses fixmap and runs before that scaffolding is ready. Since this is the only case where we need this fix, it makes more sense to do it here in the UART driver instead of introducing a whole fdt nonposted-mmio resolver just for earlycon/fixmap. Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/tty/serial/samsung_tty.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)