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

[Wills Wang]

On Saturday, January 09, 2016 10:30 PM, Daniel Schwierzeck wrote:
> Am Samstag, den 09.01.2016, 18:46 +0800 schrieb 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.
>>
> then try to use 8 bit access and find the correct register offset. You
> can configure that offset with the DT property "reg-shift".
>
CONFIG_SYS_BIG_ENDIAN and CONFIG_SYS_LITTLE_ENDIAN are used
just for MIPS in u-boot?

-- 
Best Regards
Wills