| Message ID | BLU436-SMTP726117884F5FCC94F2F72EFFF20@phx.gbl |
|---|---|
| State | Superseded |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
Hi Wills, On 2016年01月04日 19:14, Wills Wang wrote: > 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@live.com> > --- As Marek might have suggested, you should consider add these macro first. Best regards, Thomas
On Monday, January 04, 2016 at 02:12:08 PM, Thomas Chou wrote: > Hi Wills, > > On 2016年01月04日 19:14, Wills Wang wrote: > > 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@live.com> > > --- > > As Marek might have suggested, you should consider add these macro first. I think Marek's suggestions are being ignored :-) Best regards, Marek Vasut
On 01/04/2016 09:25 PM, Marek Vasut wrote: > On Monday, January 04, 2016 at 02:12:08 PM, Thomas Chou wrote: >> Hi Wills, >> >> On 2016年01月04日 19:14, Wills Wang wrote: >>> 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@live.com> >>> --- >> As Marek might have suggested, you should consider add these macro first. > I think Marek's suggestions are being ignored :-) I don't really know why u-boot and kernel don't both support these macros. In addition, should it be considered in the scope of these patch series? > > Best regards, > Marek Vasut
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@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?
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@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.
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@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".
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@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?
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@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.
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
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@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(-)