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

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(+)

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