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

[Daniel Schwierzeck]

Am Sonntag, den 17.01.2016, 00:15 +0800 schrieb 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?

no, that is a generic config option. E.g. PowerPC also has BE, most
other archs have LE, MIPS supports both variants.


-- 
- Daniel