mbox series

[v4,0/3] tty: serial: samsung: Serial fixes for Apple A7-A11 SoCs

Message ID 20240909084222.3209-1-towinchenmi@gmail.com
Headers show
Series tty: serial: samsung: Serial fixes for Apple A7-A11 SoCs | expand

Message

Nick Chan Sept. 9, 2024, 8:37 a.m. UTC 
Hi,

This series fixes issues with serial on A7-A11 SoCs. The changes do not
seem to affect existing M1 and up users so they can be applied
unconditionally.

Firstly, these SoCs require 32-bit writes on the serial port. This only
manifested in earlycon as reg-io-width in device tree is consulted for
normal serial writes.

Secondly, A7-A9 SoCs seems to use different bits for RXTO and RXTO
enable. Accessing these bits in addition to the original RXTO and RXTO
enable bits will allow serial rx to work correctly on those SoCs.

Changes in v4:
  - Removed fake Reviewed-by tag added by accident... need to stop
    making stupid mistakes that wastes everyone's time. The remaining
    Reviewed-by is real as far as I am aware.

Changes in v3:
  - v2 did not declare itself as v2 in subject line... resend as v3.

Changes in v2:
  - Mention A7-A11 in the comment about changing register accesses to
    MMIO32.

  - Use BIT() macro for new entries, and change the existing APPLE_S5L_*
    entries for consistency.

v1: https://lore.kernel.org/linux-samsung-soc/20240907111431.2970-1-towinchenmi@gmail.com
v2: https://lore.kernel.org/linux-samsung-soc/20240908075904.12133-1-towinchenmi@gmail.com
v3: https://lore.kernel.org/linux-samsung-soc/20240908090939.2745-1-towinchenmi@gmail.com

Nick Chan
---

Nick Chan (3):
  tty: serial: samsung: Use BIT() macro for APPLE_S5L_*
  tty: serial: samsung: Fix A7-A11 serial earlycon SError
  tty: serial: samsung: Fix serial rx on Apple A7-A9

 drivers/tty/serial/samsung_tty.c | 22 ++++++++++++++++------
 include/linux/serial_s3c.h       | 24 ++++++++++++++----------
 2 files changed, 30 insertions(+), 16 deletions(-)


base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8

Comments

Nick Chan Sept. 9, 2024, 9:51 a.m. UTC | #1 
On 9/9/2024 17:43, Kwanghoon Son wrote:
> On Mon, 2024-09-09 at 16:37 +0800, Nick Chan wrote:
>> Apple's older A7-A9 SoCs seems to use bit 3 in UTRSTAT as RXTO, which is
>> enabled by bit 11 in UCON.
>>
>> Access these bits in addition to the original RXTO and RXTO enable bits,
>> to allow serial rx to function on A7-A9 SoCs. This change does not
>> appear to affect the A10 SoC and up.
>>
>> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
>>
> 
> [snip]
> 
>> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
>> index 1e8686695487..964a4fbf2626 100644
>> --- a/include/linux/serial_s3c.h
>> +++ b/include/linux/serial_s3c.h
>> @@ -246,24 +246,28 @@
>>  				 S5PV210_UFCON_TXTRIG4 |	\
>>  				 S5PV210_UFCON_RXTRIG4)
>>  
>> -#define APPLE_S5L_UCON_RXTO_ENA		9
>> -#define APPLE_S5L_UCON_RXTHRESH_ENA	12
>> -#define APPLE_S5L_UCON_TXTHRESH_ENA	13
>> -#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
>> -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
>> -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
>> +#define APPLE_S5L_UCON_RXTO_ENA			9
>> +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA		11
>> +#define APPLE_S5L_UCON_RXTHRESH_ENA		12
>> +#define APPLE_S5L_UCON_TXTHRESH_ENA		13
>> +#define APPLE_S5L_UCON_RXTO_ENA_MSK		BIT(APPLE_S5L_UCON_RXTO_ENA)
>> +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_LEGACY_ENA)
>> +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
>> +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> 
> Small thing, but other diff is not needed except
> APPLE_S5L_UCON_RXTO_LEGACY_ENA.
> 
> Kwang.
The other diffs are there to keep everything aligned, it looks like a
jumbled mess here in the email, but in an editor like nano it is all
aligned, before or after this series.

> 
>>  
>>  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
>>  					 S3C2410_UCON_RXIRQMODE | \
>>  					 S3C2410_UCON_RXFIFO_TOI)
>>  #define APPLE_S5L_UCON_MASK		(APPLE_S5L_UCON_RXTO_ENA_MSK | \
>> +					 APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK | \
>>  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
>>  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
>>  
>> +#define APPLE_S5L_UTRSTAT_RXTO_LEGACY	BIT(3)
>>  #define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
>>  #define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
>>  #define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
>> -#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
>> +#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f8)
>>  
>>  #ifndef __ASSEMBLY__
>>  
> 

Nick Chan
Janne Grunau Sept. 9, 2024, 1:10 p.m. UTC | #2 
Hej,

On Mon, Sep 9, 2024, at 10:37, Nick Chan wrote:
> Hi,
>
> This series fixes issues with serial on A7-A11 SoCs. The changes do not
> seem to affect existing M1 and up users so they can be applied
> unconditionally.
>
> Firstly, these SoCs require 32-bit writes on the serial port. This only
> manifested in earlycon as reg-io-width in device tree is consulted for
> normal serial writes.
>
> Secondly, A7-A9 SoCs seems to use different bits for RXTO and RXTO
> enable. Accessing these bits in addition to the original RXTO and RXTO
> enable bits will allow serial rx to work correctly on those SoCs.
>
> Changes in v4:
>   - Removed fake Reviewed-by tag added by accident... need to stop
>     making stupid mistakes that wastes everyone's time. The remaining
>     Reviewed-by is real as far as I am aware.
>
> Changes in v3:
>   - v2 did not declare itself as v2 in subject line... resend as v3.
>
> Changes in v2:
>   - Mention A7-A11 in the comment about changing register accesses to
>     MMIO32.
>
>   - Use BIT() macro for new entries, and change the existing APPLE_S5L_*
>     entries for consistency.
>
> v1: 
> https://lore.kernel.org/linux-samsung-soc/20240907111431.2970-1-towinchenmi@gmail.com
> v2: 
> https://lore.kernel.org/linux-samsung-soc/20240908075904.12133-1-towinchenmi@gmail.com
> v3: 
> https://lore.kernel.org/linux-samsung-soc/20240908090939.2745-1-towinchenmi@gmail.com
>
> Nick Chan
> ---
>
> Nick Chan (3):
>   tty: serial: samsung: Use BIT() macro for APPLE_S5L_*
>   tty: serial: samsung: Fix A7-A11 serial earlycon SError
>   tty: serial: samsung: Fix serial rx on Apple A7-A9
>
>  drivers/tty/serial/samsung_tty.c | 22 ++++++++++++++++------
>  include/linux/serial_s3c.h       | 24 ++++++++++++++----------
>  2 files changed, 30 insertions(+), 16 deletions(-)

whole series tested on M1 Max and M2 Pro

Tested-by: Janne Grunau <j@jannau.net>

best regards
janne
Kwanghoon Son Sept. 10, 2024, 1:59 a.m. UTC | #3 
On Mon, 2024-09-09 at 17:51 +0800, Nick Chan wrote:
> 
> On 9/9/2024 17:43, Kwanghoon Son wrote:
> > On Mon, 2024-09-09 at 16:37 +0800, Nick Chan wrote:
> > > Apple's older A7-A9 SoCs seems to use bit 3 in UTRSTAT as RXTO, which is
> > > enabled by bit 11 in UCON.
> > > 
> > > Access these bits in addition to the original RXTO and RXTO enable bits,
> > > to allow serial rx to function on A7-A9 SoCs. This change does not
> > > appear to affect the A10 SoC and up.
> > > 
> > > Signed-off-by: Nick Chan <towinchenmi@gmail.com>
> > > 
> > 
> > [snip]
> > 
> > > diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> > > index 1e8686695487..964a4fbf2626 100644
> > > --- a/include/linux/serial_s3c.h
> > > +++ b/include/linux/serial_s3c.h
> > > @@ -246,24 +246,28 @@
> > >  				 S5PV210_UFCON_TXTRIG4 |	\
> > >  				 S5PV210_UFCON_RXTRIG4)
> > >  
> > > -#define APPLE_S5L_UCON_RXTO_ENA		9
> > > -#define APPLE_S5L_UCON_RXTHRESH_ENA	12
> > > -#define APPLE_S5L_UCON_TXTHRESH_ENA	13
> > > -#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
> > > -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> > > -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> > > +#define APPLE_S5L_UCON_RXTO_ENA			9
> > > +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA		11
> > > +#define APPLE_S5L_UCON_RXTHRESH_ENA		12
> > > +#define APPLE_S5L_UCON_TXTHRESH_ENA		13
> > > +#define APPLE_S5L_UCON_RXTO_ENA_MSK		BIT(APPLE_S5L_UCON_RXTO_ENA)
> > > +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_LEGACY_ENA)
> > > +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> > > +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> > 
> > Small thing, but other diff is not needed except
> > APPLE_S5L_UCON_RXTO_LEGACY_ENA.
> > 
> > Kwang.
> The other diffs are there to keep everything aligned, it looks like a
> jumbled mess here in the email, but in an editor like nano it is all
> aligned, before or after this series.

I know why you did. But still there is way keep aligned and only one
line added. 

you just added one more tab to other lines.
If one tab with APPLE_S5L_UCON_RXTO_LEGACY_ENA, then everything will
fine.

I think less changes better when see git show or blame.

Best regards,
Kwang.

> 
> > 
> > >  
> > >  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
> > >  					 S3C2410_UCON_RXIRQMODE | \
> > >  					 S3C2410_UCON_RXFIFO_TOI)
> > >  #define APPLE_S5L_UCON_MASK		(APPLE_S5L_UCON_RXTO_ENA_MSK | \
> > > +					 APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK | \
> > >  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
> > >  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
> > >  
> > > +#define APPLE_S5L_UTRSTAT_RXTO_LEGACY	BIT(3)
> > >  #define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
> > >  #define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
> > >  #define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
> > > -#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
> > > +#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f8)
> > >  
> > >  #ifndef __ASSEMBLY__
> > >  
> > 
> 
> Nick Chan
Nick Chan Sept. 10, 2024, 2:59 a.m. UTC | #4 
On 10/9/2024 09:59, Kwanghoon Son wrote:
> On Mon, 2024-09-09 at 17:51 +0800, Nick Chan wrote:
>>
>> On 9/9/2024 17:43, Kwanghoon Son wrote:
>>> On Mon, 2024-09-09 at 16:37 +0800, Nick Chan wrote:
>>>> Apple's older A7-A9 SoCs seems to use bit 3 in UTRSTAT as RXTO, which is
>>>> enabled by bit 11 in UCON.
>>>>
>>>> Access these bits in addition to the original RXTO and RXTO enable bits,
>>>> to allow serial rx to function on A7-A9 SoCs. This change does not
>>>> appear to affect the A10 SoC and up.
>>>>
>>>> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
>>>>
>>>
>>> [snip]
>>>
>>>> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
>>>> index 1e8686695487..964a4fbf2626 100644
>>>> --- a/include/linux/serial_s3c.h
>>>> +++ b/include/linux/serial_s3c.h
>>>> @@ -246,24 +246,28 @@
>>>>  				 S5PV210_UFCON_TXTRIG4 |	\
>>>>  				 S5PV210_UFCON_RXTRIG4)
>>>>  
>>>> -#define APPLE_S5L_UCON_RXTO_ENA		9
>>>> -#define APPLE_S5L_UCON_RXTHRESH_ENA	12
>>>> -#define APPLE_S5L_UCON_TXTHRESH_ENA	13
>>>> -#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
>>>> -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
>>>> -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
>>>> +#define APPLE_S5L_UCON_RXTO_ENA			9
>>>> +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA		11
>>>> +#define APPLE_S5L_UCON_RXTHRESH_ENA		12
>>>> +#define APPLE_S5L_UCON_TXTHRESH_ENA		13
>>>> +#define APPLE_S5L_UCON_RXTO_ENA_MSK		BIT(APPLE_S5L_UCON_RXTO_ENA)
>>>> +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_LEGACY_ENA)
>>>> +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
>>>> +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
>>>
>>> Small thing, but other diff is not needed except
>>> APPLE_S5L_UCON_RXTO_LEGACY_ENA.
>>>
>>> Kwang.
>> The other diffs are there to keep everything aligned, it looks like a
>> jumbled mess here in the email, but in an editor like nano it is all
>> aligned, before or after this series.
> 
> I know why you did. But still there is way keep aligned and only one
> line added. 
> 
> you just added one more tab to other lines.
> If one tab with APPLE_S5L_UCON_RXTO_LEGACY_ENA, then everything will
> fine.
> 
> I think less changes better when see git show or blame.
While as you said, APPLE_S5L_UCON_RXTO_LEGACY_ENA would be fine,
APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK is too long for that to be aligned, see
below without +, - or > distorting everything.

Before:
#define APPLE_S5L_UCON_RXTO_ENA		9
#define APPLE_S5L_UCON_RXTHRESH_ENA	12
#define APPLE_S5L_UCON_TXTHRESH_ENA	13
#define APPLE_S5L_UCON_RXTO_ENA_MSK	(1 << APPLE_S5L_UCON_RXTO_ENA)
#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_RXTHRESH_ENA)
#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_TXTHRESH_ENA)

Patch 1:
#define APPLE_S5L_UCON_RXTO_ENA		9
#define APPLE_S5L_UCON_RXTHRESH_ENA	12
#define APPLE_S5L_UCON_TXTHRESH_ENA	13
#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)

After:
#define APPLE_S5L_UCON_RXTO_ENA			9
#define APPLE_S5L_UCON_RXTO_LEGACY_ENA		11
#define APPLE_S5L_UCON_RXTHRESH_ENA		12
#define APPLE_S5L_UCON_TXTHRESH_ENA		13
#define APPLE_S5L_UCON_RXTO_ENA_MSK		BIT(APPLE_S5L_UCON_RXTO_ENA)
#define APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK
BIT(APPLE_S5L_UCON_RXTO_LEGACY_ENA)
#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_TXTHRESH_ENA)

> 
> Best regards,
> Kwang.
> 
>>
>>>
>>>>  
>>>>  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
>>>>  					 S3C2410_UCON_RXIRQMODE | \
>>>>  					 S3C2410_UCON_RXFIFO_TOI)
>>>>  #define APPLE_S5L_UCON_MASK		(APPLE_S5L_UCON_RXTO_ENA_MSK | \
>>>> +					 APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK | \
>>>>  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
>>>>  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
>>>>  
>>>> +#define APPLE_S5L_UTRSTAT_RXTO_LEGACY	BIT(3)
>>>>  #define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
>>>>  #define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
>>>>  #define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
>>>> -#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
>>>> +#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f8)
>>>>  
>>>>  #ifndef __ASSEMBLY__
>>>>  
>>>
>>
>> Nick Chan
> 

Nick Chan
Kwanghoon Son Sept. 10, 2024, 4:15 a.m. UTC | #5 
On Tue, 2024-09-10 at 10:59 +0800, Nick Chan wrote:
> 
> On 10/9/2024 09:59, Kwanghoon Son wrote:
> > On Mon, 2024-09-09 at 17:51 +0800, Nick Chan wrote:
> > > 
> > > On 9/9/2024 17:43, Kwanghoon Son wrote:
> > > > On Mon, 2024-09-09 at 16:37 +0800, Nick Chan wrote:
> > > > > Apple's older A7-A9 SoCs seems to use bit 3 in UTRSTAT as RXTO, which is
> > > > > enabled by bit 11 in UCON.
> > > > > 
> > > > > Access these bits in addition to the original RXTO and RXTO enable bits,
> > > > > to allow serial rx to function on A7-A9 SoCs. This change does not
> > > > > appear to affect the A10 SoC and up.
> > > > > 
> > > > > Signed-off-by: Nick Chan <towinchenmi@gmail.com>
> > > > > 
> > > > 
> > > > [snip]
> > > > 
> > > > > diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> > > > > index 1e8686695487..964a4fbf2626 100644
> > > > > --- a/include/linux/serial_s3c.h
> > > > > +++ b/include/linux/serial_s3c.h
> > > > > @@ -246,24 +246,28 @@
> > > > >  				 S5PV210_UFCON_TXTRIG4 |	\
> > > > >  				 S5PV210_UFCON_RXTRIG4)
> > > > >  
> > > > > -#define APPLE_S5L_UCON_RXTO_ENA		9
> > > > > -#define APPLE_S5L_UCON_RXTHRESH_ENA	12
> > > > > -#define APPLE_S5L_UCON_TXTHRESH_ENA	13
> > > > > -#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
> > > > > -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> > > > > -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> > > > > +#define APPLE_S5L_UCON_RXTO_ENA			9
> > > > > +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA		11
> > > > > +#define APPLE_S5L_UCON_RXTHRESH_ENA		12
> > > > > +#define APPLE_S5L_UCON_TXTHRESH_ENA		13
> > > > > +#define APPLE_S5L_UCON_RXTO_ENA_MSK		BIT(APPLE_S5L_UCON_RXTO_ENA)
> > > > > +#define APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_LEGACY_ENA)
> > > > > +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> > > > > +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> > > > 
> > > > Small thing, but other diff is not needed except
> > > > APPLE_S5L_UCON_RXTO_LEGACY_ENA.
> > > > 
> > > > Kwang.
> > > The other diffs are there to keep everything aligned, it looks like a
> > > jumbled mess here in the email, but in an editor like nano it is all
> > > aligned, before or after this series.
> > 
> > I know why you did. But still there is way keep aligned and only one
> > line added. 
> > 
> > you just added one more tab to other lines.
> > If one tab with APPLE_S5L_UCON_RXTO_LEGACY_ENA, then everything will
> > fine.
> > 
> > I think less changes better when see git show or blame.
> While as you said, APPLE_S5L_UCON_RXTO_LEGACY_ENA would be fine,
> APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK is too long for that to be aligned, see
> below without +, - or > distorting everything.
> 
> Before:
> #define APPLE_S5L_UCON_RXTO_ENA		9
> #define APPLE_S5L_UCON_RXTHRESH_ENA	12
> #define APPLE_S5L_UCON_TXTHRESH_ENA	13
> #define APPLE_S5L_UCON_RXTO_ENA_MSK	(1 << APPLE_S5L_UCON_RXTO_ENA)
> #define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_RXTHRESH_ENA)
> #define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_TXTHRESH_ENA)
> 
> Patch 1:
> #define APPLE_S5L_UCON_RXTO_ENA		9
> #define APPLE_S5L_UCON_RXTHRESH_ENA	12
> #define APPLE_S5L_UCON_TXTHRESH_ENA	13
> #define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
> #define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> #define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> 
> After:
> #define APPLE_S5L_UCON_RXTO_ENA			9
> #define APPLE_S5L_UCON_RXTO_LEGACY_ENA		11
> #define APPLE_S5L_UCON_RXTHRESH_ENA		12
> #define APPLE_S5L_UCON_TXTHRESH_ENA		13
> #define APPLE_S5L_UCON_RXTO_ENA_MSK		BIT(APPLE_S5L_UCON_RXTO_ENA)
> #define APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK
> BIT(APPLE_S5L_UCON_RXTO_LEGACY_ENA)
> #define APPLE_S5L_UCON_RXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> #define APPLE_S5L_UCON_TXTHRESH_ENA_MSK		BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> 

Okay I got it.
Thanks for check!

Kwang.

> > 
> > Best regards,
> > Kwang.
> > 
> > > 
> > > > 
> > > > >  
> > > > >  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
> > > > >  					 S3C2410_UCON_RXIRQMODE | \
> > > > >  					 S3C2410_UCON_RXFIFO_TOI)
> > > > >  #define APPLE_S5L_UCON_MASK		(APPLE_S5L_UCON_RXTO_ENA_MSK | \
> > > > > +					 APPLE_S5L_UCON_RXTO_LEGACY_ENA_MSK | \
> > > > >  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
> > > > >  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
> > > > >  
> > > > > +#define APPLE_S5L_UTRSTAT_RXTO_LEGACY	BIT(3)
> > > > >  #define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
> > > > >  #define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
> > > > >  #define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
> > > > > -#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
> > > > > +#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f8)
> > > > >  
> > > > >  #ifndef __ASSEMBLY__
> > > > >  
> > > > 
> > > 
> > > Nick Chan
> > 
> 
> Nick Chan
>
Neal Gompa Sept. 10, 2024, 12:21 p.m. UTC | #6 
On Mon, Sep 9, 2024 at 10:42 AM Nick Chan <towinchenmi@gmail.com> wrote:
>
> Hi,
>
> This series fixes issues with serial on A7-A11 SoCs. The changes do not
> seem to affect existing M1 and up users so they can be applied
> unconditionally.
>
> Firstly, these SoCs require 32-bit writes on the serial port. This only
> manifested in earlycon as reg-io-width in device tree is consulted for
> normal serial writes.
>
> Secondly, A7-A9 SoCs seems to use different bits for RXTO and RXTO
> enable. Accessing these bits in addition to the original RXTO and RXTO
> enable bits will allow serial rx to work correctly on those SoCs.
>
> Changes in v4:
>   - Removed fake Reviewed-by tag added by accident... need to stop
>     making stupid mistakes that wastes everyone's time. The remaining
>     Reviewed-by is real as far as I am aware.
>
> Changes in v3:
>   - v2 did not declare itself as v2 in subject line... resend as v3.
>
> Changes in v2:
>   - Mention A7-A11 in the comment about changing register accesses to
>     MMIO32.
>
>   - Use BIT() macro for new entries, and change the existing APPLE_S5L_*
>     entries for consistency.
>
> v1: https://lore.kernel.org/linux-samsung-soc/20240907111431.2970-1-towinchenmi@gmail.com
> v2: https://lore.kernel.org/linux-samsung-soc/20240908075904.12133-1-towinchenmi@gmail.com
> v3: https://lore.kernel.org/linux-samsung-soc/20240908090939.2745-1-towinchenmi@gmail.com
>
> Nick Chan
> ---
>
> Nick Chan (3):
>   tty: serial: samsung: Use BIT() macro for APPLE_S5L_*
>   tty: serial: samsung: Fix A7-A11 serial earlycon SError
>   tty: serial: samsung: Fix serial rx on Apple A7-A9
>
>  drivers/tty/serial/samsung_tty.c | 22 ++++++++++++++++------
>  include/linux/serial_s3c.h       | 24 ++++++++++++++----------
>  2 files changed, 30 insertions(+), 16 deletions(-)
>
>
> base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8
> --
> 2.46.0
>
>

Whole series LGTM.

Reviewed-by: Neal Gompa <neal@gompa.dev>
Andi Shyti Sept. 10, 2024, 12:41 p.m. UTC | #7 
Hi Nick,

> +#define APPLE_S5L_UTRSTAT_RXTO_LEGACY	BIT(3)
>  #define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
>  #define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
>  #define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
> -#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
> +#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f8)

As you are here, you could use genmask

GENMASK(0xff, 3)

Andi
Andi Shyti Sept. 10, 2024, 12:48 p.m. UTC | #8 
Hi Nick,

On Mon, Sep 09, 2024 at 04:37:25PM GMT, Nick Chan wrote:
> New entries using BIT() will be added soon, so change the existing ones
> for consistency.
> 
> Signed-off-by: Nick Chan <towinchenmi@gmail.com>

I think this is:

Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>

> ---
>  include/linux/serial_s3c.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> index 1672cf0810ef..1e8686695487 100644
> --- a/include/linux/serial_s3c.h
> +++ b/include/linux/serial_s3c.h
> @@ -249,9 +249,9 @@
>  #define APPLE_S5L_UCON_RXTO_ENA		9
>  #define APPLE_S5L_UCON_RXTHRESH_ENA	12
>  #define APPLE_S5L_UCON_TXTHRESH_ENA	13
> -#define APPLE_S5L_UCON_RXTO_ENA_MSK	(1 << APPLE_S5L_UCON_RXTO_ENA)
> -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_RXTHRESH_ENA)
> -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_TXTHRESH_ENA)
> +#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
> +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
>  
>  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
>  					 S3C2410_UCON_RXIRQMODE | \
> @@ -260,9 +260,9 @@
>  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
>  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
>  
> -#define APPLE_S5L_UTRSTAT_RXTHRESH	(1<<4)
> -#define APPLE_S5L_UTRSTAT_TXTHRESH	(1<<5)
> -#define APPLE_S5L_UTRSTAT_RXTO		(1<<9)
> +#define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
> +#define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
> +#define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
>  #define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)

You could make this GENMASK(0x3f, 4)

Andi
Andi Shyti Sept. 10, 2024, 12:49 p.m. UTC | #9 
On Tue, Sep 10, 2024 at 02:41:31PM GMT, Andi Shyti wrote:
> > +#define APPLE_S5L_UTRSTAT_RXTO_LEGACY	BIT(3)
> >  #define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
> >  #define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
> >  #define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
> > -#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
> > +#define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f8)
> 
> As you are here, you could use genmask
> 
> GENMASK(0xff, 3)

GENMASK(..., 2), of course :-)

Andi
Nick Chan Sept. 10, 2024, 2:26 p.m. UTC | #10 
On 10/9/2024 20:48, Andi Shyti wrote:
> Hi Nick,
> 
> On Mon, Sep 09, 2024 at 04:37:25PM GMT, Nick Chan wrote:
>> New entries using BIT() will be added soon, so change the existing ones
>> for consistency.
>>
>> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
> 
> I think this is:
> 
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
We will see... Got a bit paranoid after bad things happened with v2 and v3.

> 
>> ---
>>  include/linux/serial_s3c.h | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
>> index 1672cf0810ef..1e8686695487 100644
>> --- a/include/linux/serial_s3c.h
>> +++ b/include/linux/serial_s3c.h
>> @@ -249,9 +249,9 @@
>>  #define APPLE_S5L_UCON_RXTO_ENA		9
>>  #define APPLE_S5L_UCON_RXTHRESH_ENA	12
>>  #define APPLE_S5L_UCON_TXTHRESH_ENA	13
>> -#define APPLE_S5L_UCON_RXTO_ENA_MSK	(1 << APPLE_S5L_UCON_RXTO_ENA)
>> -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_RXTHRESH_ENA)
>> -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_TXTHRESH_ENA)
>> +#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
>> +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
>> +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
>>  
>>  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
>>  					 S3C2410_UCON_RXIRQMODE | \
>> @@ -260,9 +260,9 @@
>>  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
>>  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
>>  
>> -#define APPLE_S5L_UTRSTAT_RXTHRESH	(1<<4)
>> -#define APPLE_S5L_UTRSTAT_TXTHRESH	(1<<5)
>> -#define APPLE_S5L_UTRSTAT_RXTO		(1<<9)
>> +#define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
>> +#define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
>> +#define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
>>  #define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
> 
> You could make this GENMASK(0x3f, 4)
Good idea, given the above context I think I may add

Suggested-by: Andi Shyti <andi.shyti@kernel.org>

too. And actually it should be GENMASK(9, 3)

> 
> Andi

Nick Chan
Andi Shyti Sept. 10, 2024, 3:33 p.m. UTC | #11 
Hi Nick,

On Tue, Sep 10, 2024 at 10:26:44PM GMT, Nick Chan wrote:
> On 10/9/2024 20:48, Andi Shyti wrote:
> > On Mon, Sep 09, 2024 at 04:37:25PM GMT, Nick Chan wrote:
> >> New entries using BIT() will be added soon, so change the existing ones
> >> for consistency.
> >>
> >> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
> > 
> > I think this is:
> > 
> > Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> We will see... Got a bit paranoid after bad things happened with v2 and v3.

ahaha... in this case it's fine. The whole patch was suggested by
Krzysztof, so that it makes sense to add this tag.

What Krzysztof complained about is that you accidentally added
his r-b without him telling you explicitely.

> > 
> >> ---
> >>  include/linux/serial_s3c.h | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> >> index 1672cf0810ef..1e8686695487 100644
> >> --- a/include/linux/serial_s3c.h
> >> +++ b/include/linux/serial_s3c.h
> >> @@ -249,9 +249,9 @@
> >>  #define APPLE_S5L_UCON_RXTO_ENA		9
> >>  #define APPLE_S5L_UCON_RXTHRESH_ENA	12
> >>  #define APPLE_S5L_UCON_TXTHRESH_ENA	13
> >> -#define APPLE_S5L_UCON_RXTO_ENA_MSK	(1 << APPLE_S5L_UCON_RXTO_ENA)
> >> -#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_RXTHRESH_ENA)
> >> -#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_S5L_UCON_TXTHRESH_ENA)
> >> +#define APPLE_S5L_UCON_RXTO_ENA_MSK	BIT(APPLE_S5L_UCON_RXTO_ENA)
> >> +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_RXTHRESH_ENA)
> >> +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK	BIT(APPLE_S5L_UCON_TXTHRESH_ENA)
> >>  
> >>  #define APPLE_S5L_UCON_DEFAULT		(S3C2410_UCON_TXIRQMODE | \
> >>  					 S3C2410_UCON_RXIRQMODE | \
> >> @@ -260,9 +260,9 @@
> >>  					 APPLE_S5L_UCON_RXTHRESH_ENA_MSK | \
> >>  					 APPLE_S5L_UCON_TXTHRESH_ENA_MSK)
> >>  
> >> -#define APPLE_S5L_UTRSTAT_RXTHRESH	(1<<4)
> >> -#define APPLE_S5L_UTRSTAT_TXTHRESH	(1<<5)
> >> -#define APPLE_S5L_UTRSTAT_RXTO		(1<<9)
> >> +#define APPLE_S5L_UTRSTAT_RXTHRESH	BIT(4)
> >> +#define APPLE_S5L_UTRSTAT_TXTHRESH	BIT(5)
> >> +#define APPLE_S5L_UTRSTAT_RXTO		BIT(9)
> >>  #define APPLE_S5L_UTRSTAT_ALL_FLAGS	(0x3f0)
> > 
> > You could make this GENMASK(0x3f, 4)
> Good idea, given the above context I think I may add
> 
> Suggested-by: Andi Shyti <andi.shyti@kernel.org>

ehm... not in this case. Mine is a suggestion as reviewer and
this little comment does not deserve a "Suggested-by" like
in Krzysztof's case.

> too. And actually it should be GENMASK(9, 3)

You find out the right parameters :-)

Thanks,
Andi

> > 
> > Andi
> 
> Nick Chan
>