[U-Boot] [PATCH v6 06/10] ns16550: add support for mips

[Wills Wang]

On 01/09/2016 12:23 AM, Daniel Schwierzeck wrote:
> Am Montag, den 04.01.2016, 19:14 +0800 schrieb Wills Wang:
>> MIPS archtecture have no "in_le32/in_be32/out_le32/out_be32" macro,
>> but usually define CONFIG_SYS_BIG_ENDIAN, this patch use readl/writel
>> for register operation in mips when define CONFIG_SYS_NS16550_MEM32.
>>
>> Signed-off-by: Wills Wang <wills.wang at live.com>
>> ---
>>
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/serial/ns16550.c | 24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 3fab3f1..3b24af0 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -64,12 +64,16 @@ static inline void serial_out_shift(void *addr,
>> int shift, int value)
>>   {
>>   #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>   	outb(value, (ulong)addr);
>> -#elif defined(CONFIG_SYS_NS16550_MEM32) &&
>> !defined(CONFIG_SYS_BIG_ENDIAN)
>> -	out_le32(addr, value);
>> -#elif defined(CONFIG_SYS_NS16550_MEM32) &&
>> defined(CONFIG_SYS_BIG_ENDIAN)
>> -	out_be32(addr, value);
>>   #elif defined(CONFIG_SYS_NS16550_MEM32)
>> +#ifdef CONFIG_MIPS
>>   	writel(value, addr);
>> +#else
>> +#ifndef CONFIG_SYS_BIG_ENDIAN
>> +	out_le32(addr, value);
>> +#else
>> +	out_be32(addr, value);
>> +#endif
>> +#endif
>>   #elif defined(CONFIG_SYS_BIG_ENDIAN)
>>   	writeb(value, addr + (1 << shift) - 1);
>>   #else
>> @@ -81,12 +85,16 @@ static inline int serial_in_shift(void *addr, int
>> shift)
>>   {
>>   #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
>>   	return inb((ulong)addr);
>> -#elif defined(CONFIG_SYS_NS16550_MEM32) &&
>> !defined(CONFIG_SYS_BIG_ENDIAN)
>> -	return in_le32(addr);
>> -#elif defined(CONFIG_SYS_NS16550_MEM32) &&
>> defined(CONFIG_SYS_BIG_ENDIAN)
>> -	return in_be32(addr);
>>   #elif defined(CONFIG_SYS_NS16550_MEM32)
>> +#ifdef CONFIG_MIPS
>>   	return readl(addr);
>> +#else
>> +#ifndef CONFIG_SYS_BIG_ENDIAN
>> +	return in_le32(addr);
>> +#else
>> +	return in_be32(addr);
>> +#endif
>> +#endif
>>   #elif defined(CONFIG_SYS_BIG_ENDIAN)
>>   	return readb(addr + (1 << shift) - 1);
>>   #else
> Could you tell us, if you need port IO or MMIO. In case of MMIO do you
> need 8 bit or 32 bit I/O access?
>
> Becasue your CPU is running in Big Endian and the NS16550 is Little
> Endian, you need endianess conversion in the 32 bit case.
>
> The 8 bit access should already work without changing anything. The
> MIPS Malta board also uses NS16550 and already works in Bit Endian
> mode.
>
> To make the 32 bit case working, you need to select
> CONFIG_SWAP_IO_SPACE in your SoC Kconfig code. Then all readl/writel
> accessors do an endianess conversion to Little Endian if the CPU is
> running in Big Endian.
>
> Anyway I think the current implementation is wrong:
>
> static inline void serial_out_shift(void *addr, int shift, int value)
> {
> #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> 	outb(value, (ulong)addr);
> #elif defined(CONFIG_SYS_NS16550_MEM32) &&
> !defined(CONFIG_SYS_BIG_ENDIAN)
> 	out_le32(addr, value);
> #elif defined(CONFIG_SYS_NS16550_MEM32) &&
> defined(CONFIG_SYS_BIG_ENDIAN)
> 	out_be32(addr, value);
> #elif defined(CONFIG_SYS_NS16550_MEM32)
> 	writel(value, addr);
> #elif defined(CONFIG_SYS_BIG_ENDIAN)
> 	writeb(value, addr + (1 << shift) - 1);
> #else
> 	writeb(value, addr);
> #endif
> }
>
> The branch with writel() will never be taken because the two preceding
> branches already catch all conditions for CONFIG_SYS_NS16550_MEM32.
> Also if the NS16550 is Little Endian, the branch with out_be32 makes no
> sense. All IO accessors must convert from CPU endianess to NS16550's
> Little Endian, but out_be32 converts from CPU endianess to Big Endian.
>
> To handle port IO, 32 Bit MMIO and 8 Bit MMIO, following code should be
> enough:
>
> static inline void serial_out_shift(void *addr, int shift, int value)
> {
> #ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> 	outb(value, (ulong)addr);
> #elif defined(CONFIG_SYS_NS16550_MEM32)
> 	writel(value, addr);
> #elif defined(CONFIG_SYS_BIG_ENDIAN)
> 	writeb(value, addr + (1 << shift) - 1);
> #else
> 	writeb(value, addr);
> #endif
> }
>
> The arch-specific implementation of readl/writel should be responsible
> for the endianess conversion and not the driver. What do you think?
>
I wholly agree with Daniel, driver should not care the endianness, we
need a general mechanism to deal with this situation, no matter the
endianness of CPU and peripheral IP Core.
But CONFIG_SWAP_IO_SPACE don't resolve this issue, NS16550 is just
one of many peripherals for CPU.
NS16550 use only 8bit register field even if in 32 bit MMIO. In actual,
driver may just concern the register offset beause the bit field of
register in chip datasheet is coincident with CPU endianness, even
though hardware support both big-endian and little-endian, so the
optimal code should be the following:
static inline void serial_out_shift(void *addr, int shift, int value)
{
#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
     outb(value, (ulong)addr);
#elif defined(CONFIG_SYS_NS16550_MEM32)
     writel(value, addr);
#else
     writeb(value, addr);
#endif
}
I use 32bit MMIO to access uart currently, i think that 8 Bit MMIO
should work fine if adjust the register offset.

-- 
Best Regards
Wills