| Message ID | 20231214105243.3707730-6-tudor.ambarus@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GS101 Oriole: CMU_PERIC0 support and USI updates | expand |
On 12/14/23 12:01, Arnd Bergmann wrote: Hi, Arnd, Thanks for the review! > On Thu, Dec 14, 2023, at 11:52, Tudor Ambarus wrote: >> +static int __init gs101_early_console_setup(struct earlycon_device *device, >> + const char *opt) >> +{ >> + /* gs101 always expects MMIO32 register accesses. */ >> + device->port.iotype = UPIO_MEM32; >> + >> + return s5pv210_early_console_setup(device, opt); >> +} >> + >> +OF_EARLYCON_DECLARE(gs101, "google,gs101-uart", gs101_early_console_setup); > > It looks like this is already done by of_setup_earlycon() based on > the reg-io-width property. Any idea why it doesn't work with the > normal s5pv210_early_console_setup() function? > It works if in device tree one specifies the reg-io-width property and sets it to 4. If the reg-io-width is not specified, the iotype defaults to UPIO_MEM causing the SError interrupt on gs101 which makes the system unusable. Also, if the earlycon comes specified from the kernel params, the of_setup_earlycon() is no longer called and the earlycon will be set solely based on the kernel params buffer, thus allowing users to crash the kernel on wrong earlycon definitions. If you think the change is fine, I can amend the commit message with the description from above. Cheers, ta
On 12/14/23 14:19, Arnd Bergmann wrote: > On Thu, Dec 14, 2023, at 13:52, Tudor Ambarus wrote: >> On 12/14/23 12:01, Arnd Bergmann wrote: >>> On Thu, Dec 14, 2023, at 11:52, Tudor Ambarus wrote: >>>> +static int __init gs101_early_console_setup(struct earlycon_device *device, >>> >> >> It works if in device tree one specifies the reg-io-width property and >> sets it to 4. If the reg-io-width is not specified, the iotype defaults >> to UPIO_MEM causing the SError interrupt on gs101 which makes the system >> unusable. > > In the case of incorrect DT data like a missing reg-io-width property, > I would expect it to still fail once the regular console or tty takes > over from earlycon. > >> Also, if the earlycon comes specified from the kernel params, the >> of_setup_earlycon() is no longer called and the earlycon will be set >> solely based on the kernel params buffer, thus allowing users to crash >> the kernel on wrong earlycon definitions. > > But that in turn is the same as specifying any other incorrect earlycon. I don't think you can crash the kernel if you use other earlycon as you don't make accesses on the 32bit restricted bus. But I agree that if using the correct earlycon name, and mmio instead mmio32, is equivalent to not specifying reg-io-width in dt. > >> If you think the change is fine, I can amend the commit message with the >> description from above. > > I'm still not convinced we need a special case here when everything else > just requires passing the correct data. > Well, I made this patch because I used a wrong bootargs earlycon configuration and I ended up crashing the kernel. I couldn't see what happens as kgdb is not available at that stage. Figuring out what was going on made me spend some time. I hoped I'll be helpful and spare others of the same mistakes and wasted time. I'm ok to drop the patch as well, no pushing here. Please ignore. Thanks for the review! Cheers, ta
On 14/12/2023 15:31, Tudor Ambarus wrote: > > > On 12/14/23 14:19, Arnd Bergmann wrote: >> On Thu, Dec 14, 2023, at 13:52, Tudor Ambarus wrote: >>> On 12/14/23 12:01, Arnd Bergmann wrote: >>>> On Thu, Dec 14, 2023, at 11:52, Tudor Ambarus wrote: >>>>> +static int __init gs101_early_console_setup(struct earlycon_device *device, >>>> >>> >>> It works if in device tree one specifies the reg-io-width property and >>> sets it to 4. If the reg-io-width is not specified, the iotype defaults >>> to UPIO_MEM causing the SError interrupt on gs101 which makes the system >>> unusable. >> >> In the case of incorrect DT data like a missing reg-io-width property, >> I would expect it to still fail once the regular console or tty takes >> over from earlycon. >> >>> Also, if the earlycon comes specified from the kernel params, the >>> of_setup_earlycon() is no longer called and the earlycon will be set >>> solely based on the kernel params buffer, thus allowing users to crash >>> the kernel on wrong earlycon definitions. >> >> But that in turn is the same as specifying any other incorrect earlycon. > > I don't think you can crash the kernel if you use other earlycon as you > don't make accesses on the 32bit restricted bus. But I agree that if > using the correct earlycon name, and mmio instead mmio32, is equivalent > to not specifying reg-io-width in dt. > >> >>> If you think the change is fine, I can amend the commit message with the >>> description from above. >> >> I'm still not convinced we need a special case here when everything else >> just requires passing the correct data. We shouldn't need any data from DT for this case, because this property apparently can be inferred from the compatible. IOW, GS101 SoC requires reg-io-width=4, everywhere, for each node, thus there is no need to specify this property. It should be deduced from the compatible. Best regards, Krzysztof
On 12/15/23 08:01, Krzysztof Kozlowski wrote: > On 14/12/2023 15:31, Tudor Ambarus wrote: >> >> >> On 12/14/23 14:19, Arnd Bergmann wrote: >>> On Thu, Dec 14, 2023, at 13:52, Tudor Ambarus wrote: >>>> On 12/14/23 12:01, Arnd Bergmann wrote: >>>>> On Thu, Dec 14, 2023, at 11:52, Tudor Ambarus wrote: >>>>>> +static int __init gs101_early_console_setup(struct earlycon_device *device, >>>>> >>>> >>>> It works if in device tree one specifies the reg-io-width property and >>>> sets it to 4. If the reg-io-width is not specified, the iotype defaults >>>> to UPIO_MEM causing the SError interrupt on gs101 which makes the system >>>> unusable. >>> >>> In the case of incorrect DT data like a missing reg-io-width property, >>> I would expect it to still fail once the regular console or tty takes >>> over from earlycon. >>> >>>> Also, if the earlycon comes specified from the kernel params, the >>>> of_setup_earlycon() is no longer called and the earlycon will be set >>>> solely based on the kernel params buffer, thus allowing users to crash >>>> the kernel on wrong earlycon definitions. >>> >>> But that in turn is the same as specifying any other incorrect earlycon. >> >> I don't think you can crash the kernel if you use other earlycon as you >> don't make accesses on the 32bit restricted bus. But I agree that if >> using the correct earlycon name, and mmio instead mmio32, is equivalent >> to not specifying reg-io-width in dt. >> >>> >>>> If you think the change is fine, I can amend the commit message with the >>>> description from above. >>> >>> I'm still not convinced we need a special case here when everything else >>> just requires passing the correct data. > > We shouldn't need any data from DT for this case, because this property > apparently can be inferred from the compatible. IOW, GS101 SoC requires > reg-io-width=4, everywhere, for each node, thus there is no need to > specify this property. It should be deduced from the compatible. > The entire peric0/1 block requires 32 bit data widths indeed. PERIC is used by the Universal Serial Interface (USI) and I3C. I've checked few other hardware blocks and all require 32 bit data widths (G3D, TPU, TNR, PERIC, PDP, MFC, MCSC, IPP, HSI, GSA and I stopped here). If the reg-io-width shall be inferred from the compatible in the gs101 case, then this patch stands. I'll update the serial driver as well. Thanks, ta
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 66bd6c090ace..19cd3e6a9b6e 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -2787,6 +2787,17 @@ OF_EARLYCON_DECLARE(exynos4210, "samsung,exynos4210-uart", OF_EARLYCON_DECLARE(artpec8, "axis,artpec8-uart", s5pv210_early_console_setup); +static int __init gs101_early_console_setup(struct earlycon_device *device, + const char *opt) +{ + /* gs101 always expects MMIO32 register accesses. */ + device->port.iotype = UPIO_MEM32; + + return s5pv210_early_console_setup(device, opt); +} + +OF_EARLYCON_DECLARE(gs101, "google,gs101-uart", gs101_early_console_setup); + /* Apple S5L */ static int __init apple_s5l_early_console_setup(struct earlycon_device *device, const char *opt)
GS101 only allows 32-bit register accesses. If using 8-bit register accesses on gs101, a SError Interrupt is raised causing the system unusable. Force the reg accesses to MMIO32 regardless of the user's settings. This is a common practice for such earlycons (bcm2835aux, uniphier, lpuart32), in order to avoid crashing the kernel with a wrong early console definition. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/tty/serial/samsung_tty.c | 11 +++++++++++ 1 file changed, 11 insertions(+)