[U-Boot] [PATCH] microblaze: usable uart16550 for big endian systems

[Michal Simek]

Hi Stephan,

>> Stephan Linz wrote:
>>> As a result of the commit 6833260 the uart16550 driver
>>> is broken for Microblaze big endian systems, because of
>>> the missing 3 byte offset. Other than as described, the
>>> U-Boot BSP does not treat properly the 3 byte offset.
>>>
>>> However, with the new 32 bit access to ns16550 registers
>>> we can enable correct register access for Microblaze big
>>> and little endian systems in the same manner.
>> The reason why I have applied that patch is that baseaddress generation
>> was moved to u-boot BSP out of u-boot configs.
>>
>> Here is example how addresses are generated.
>> BE system:
>> #define XILINX_UART16550
>> #define XILINX_UART16550_BASEADDR	0x83e00003
> 
> Hi Michal,
> 
> Who is generating this entry (especially incl. this offset)? Was it the
> MDL environment from PetaLinux? If so, which version?

u-boot BSP.

> I use my own MDL environment from TPOS, and the generator for
> xparameters.h does not add this offset, see proc put_uart16550_cfg():
> 
> https://gitorious.org/mbref/mbref/blobs/master/edk-repository/ThirdParty/lib/tpos_misclib.tcl#line1384
> 
> And seriously we never need this offset. With a sane endianess handling
> in software we will access the right bytes in uart16550. The Xilinx FPGA
> synthesis produce results that are good enough for us. All NS16550 8 bit
> registers alligned on 32 bit memory access: 0x000000rr on BE and
> 0xrr000000 in LE.
> 
> 
> The BSP generator (Xilinx MDL part) may never knows specifics about
> software or unclean code. Moreover we have to change the code ;-)
> 
> 
> Till now we have set CONFIG_SYS_NS16550_REG_SIZE to -4 and a offset of 3
> to the NS16550 base address for Microblaze BE systems. As I can see in
> ns16550.h that was completely wrong, or not? See:
> 
> #if !defined(CONFIG_SYS_NS16550_REG_SIZE)
> #error "Please define NS16550 registers size."
> #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
> #define UART_REG(x)                                               \
>         unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1];\
>         unsigned char x;
> #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
> #define UART_REG(x)     \
>         unsigned char x;\
>         unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
> #endif
> 
> struct NS16550 {
>         UART_REG(rbr);          /* 0 */
>         UART_REG(ier);          /* 1 */
> 
> ... and so on. For BE systems we should use CONFIG_SYS_NS16550_REG_SIZE
> set to 4 --> have a 3 byte gap on NS16550 base address and then point to
> the right byte on offset 3, or not? On LE systems we need to set -4 for
> *_REG_SIZE --> have a 3 byte gap after and betweeen each 8 bit
> registers.
> 
>> Anyway you solution looks interesting and I will test it.
> 
> However since commit 79df120 we can use direct 32 bit access to 8 bit
> NS16550 registers without gap generation in ns16550.h ... we need sane
> in_*/out_* implementation.
> 

I have look at it and tested on BE/LE. For 32bit accesses we need to implement
in/out_le32 functions which we don't have right now that's why please remove this macro
from your patch.

Our BSP generates/ed +3 offset that's why I prefer to mask it in the same patch
to be sure that baseaddr is correct and compatible with old versions.

Here is patch I have used. Please add that changes to v2 patch.

Thanks,
Michal

diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
index b740a28..8085130 100644
--- a/include/configs/microblaze-generic.h
+++ b/include/configs/microblaze-generic.h
@@ -55,10 +55,16 @@
  #elif XILINX_UART16550_BASEADDR
  # define CONFIG_SYS_NS16550            1
  # define CONFIG_SYS_NS16550_SERIAL
+
+#if defined(__MICROBLAZEEL__)
  # define CONFIG_SYS_NS16550_REG_SIZE   -4
+#else
+# define CONFIG_SYS_NS16550_REG_SIZE   4
+#endif
+
  # define CONFIG_CONS_INDEX             1
  # define CONFIG_SYS_NS16550_COM1 \
-                       (XILINX_UART16550_BASEADDR + 0x1000)
+                       ((XILINX_UART16550_BASEADDR & ~0xF) + 0x1000)
  # define CONFIG_SYS_NS16550_CLK        XILINX_UART16550_CLOCK_HZ
  # define CONFIG_BAUDRATE       115200



-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian