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

[Daniel Schwierzeck]

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".

-- 
- Daniel