diff mbox series

[05/13] tty: serial: samsung: add gs101 earlycon support

Message ID 20231214105243.3707730-6-tudor.ambarus@linaro.org (mailing list archive)
State Accepted
Commit 5887cab232f7abf4ff2e7701dec38b8f0feb4cff
Headers show
Series GS101 Oriole: CMU_PERIC0 support and USI updates | expand

Commit Message

Tudor Ambarus Dec. 14, 2023, 10:52 a.m. UTC
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(+)

Comments

Arnd Bergmann Dec. 14, 2023, 12:01 p.m. UTC | #1
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?

      Arnd
Tudor Ambarus Dec. 14, 2023, 1:52 p.m. UTC | #2
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
Arnd Bergmann Dec. 14, 2023, 2:19 p.m. UTC | #3
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.

> 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.

     Arnd
Tudor Ambarus Dec. 14, 2023, 2:31 p.m. UTC | #4
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
Krzysztof Kozlowski Dec. 15, 2023, 8:01 a.m. UTC | #5
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
Tudor Ambarus Dec. 21, 2023, 7:41 a.m. UTC | #6
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 mbox series

Patch

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)