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

[Daniel Schwierzeck]

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?

-- 
- Daniel