microblaze: usable uart16550 for big endian systems

PATCH New Stephan Linz 2 participants 8 messages Nov 19, 2011 – Nov 25, 2011

Share & tools

Stephan Linz

author

2011-11-19 12:03 UTC #0 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

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.

Signed-off-by Stephan Linz <[email protected]>

---
 include/configs/microblaze-generic.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

include/configs/microblaze-generic.h +5 -0

index 6b3fd76..cd3d2a1 100644
@@ -41,7 +41,12 @@
 #elif XILINX_UART16550_BASEADDR
 # define CONFIG_SYS_NS16550		1
 # define CONFIG_SYS_NS16550_SERIAL
+# define CONFIG_SYS_NS16550_MEM32
+#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)

Signature · 1 line

1.7.0.4

Michal Simek

2011-11-21 07:21 UTC #1 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Hi Stephan,

Quoted 8 lines · Stephan Linz

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

LE system:
#define XILINX_UART16550
#define XILINX_UART16550_BASEADDR 0x83e00000

Then you can use origin config file and change is only in xparameters.h in board folder.

Anyway you solution looks interesting and I will test it.

Thanks,
Michal

Signature · 4 lines

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

Stephan Linz

author

2011-11-21 18:18 UTC #2 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Am Montag, den 21.11.2011, 08:21 +0100 schrieb Michal Simek:

Quoted 12 lines

Hi Stephan,
Quoted 8 lines · Stephan Linz
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.

Quoted 7 lines

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?

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.

Quoted 1 line

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.

Signature · 8 lines

Best regards,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz

Michal Simek

2011-11-23 14:40 UTC #3 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Hi Stephan,

Quoted 17 lines

Quoted 9 lines
Quoted 8 lines · Stephan Linz
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.
Quoted 7 lines
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

Quoted 4 lines

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.

Quoted 42 lines

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.

Quoted 6 lines

Quoted 1 line
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

include/configs/microblaze-generic.h +7 -1

index b740a28..8085130 100644
@@ -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

Signature · 4 lines

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

Stephan Linz

author

2011-11-24 18:29 UTC #4 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Am Mittwoch, den 23.11.2011, 15:40 +0100 schrieb Michal Simek:

Quoted 18 lines

Hi Stephan,
Quoted 16 lines
Quoted 15 lines
Quoted 8 lines · Stephan Linz
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.
--snip--
Anyway you solution looks interesting and I will test it.

Quoted 7 lines

Quoted 4 lines
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

Hi Michal,

Oh yes, of course. There are no *_le32 operations, not yet.

Quoted 7 lines

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.

I'll do it this way. Give me a little time to change and test it. Currently I am sill working on refactoring of the LL TEMAC driver. I hope, the refactored driver than can merge in mainline ...

Thanks,
Stephan

Quoted 24 lines

include/configs/microblaze-generic.h +7 -1

index b740a28..8085130 100644
@@ -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

Signature · 8 lines

Viele Gr??e,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz

Michal Simek

2011-11-24 19:13 UTC #5 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Quoted 31 lines · Stephan Linz

Am Mittwoch, den 23.11.2011, 15:40 +0100 schrieb Michal Simek:
Quoted 14 lines
Hi Stephan,
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines · Stephan Linz
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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
--snip--
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
Hi Michal,
Oh yes, of course. There are no *_le32 operations, not yet.
Quoted 3 lines
that's why please remove this macro
from your patch.

Quoted 8 lines

Quoted 4 lines
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.
I'll do it this way. Give me a little time to change and test it. Currently I am sill working on refactoring of the LL TEMAC driver. I hope, the refactored driver than can merge in mainline ...

I have done it some time ago and look at "[PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot thread". I have attached the latest version I have and I am not going to change it to follow
"new" u-boot network driver style because I would like to keep ppc dcr support.

Thanks,
Michal

Signature · 11 lines

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xilinx_ll_temac.c
Type: text/x-csrc
Size: 17099 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111124/af4f8704/attachment.c>

Stephan Linz

author

2011-11-24 20:30 UTC #6 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Am Donnerstag, den 24.11.2011, 20:13 +0100 schrieb Michal Simek:

Quoted 11 lines

Quoted 7 lines · Stephan Linz
--snip--
Quoted 2 lines
Here is patch I have used. Please add that changes to v2 patch.
I'll do it this way. Give me a little time to change and test it. Currently I am sill working on refactoring of the LL TEMAC driver. I hope, the refactored driver than can merge in mainline ...
I have done it some time ago and look at "[PATCH v3] net: ll_temac:
Add LL TEMAC driver to u-boot thread".

Hi Michal,

yes, I've read the whole thread.

Quoted 2 lines

I have attached the latest version I have and I am not going to change it to follow "new" u-boot network driver style

Hm, but it is possible ...

Quoted 1 line

because I would like to keep ppc dcr support.

Even this was the main work I've done last week. I've split the driver code into differnt sub modules (xilinx_ll_temac_fifo and xilinx_ll_temac_sdram), introduce some call back functions into the sub modules to harmonize the main driver code and divide the sdma code into the two different bus access methodes: direct 32 bit memory access (Microblaze) and indirect access via DCR (Xilinx PPC4xx).

Please, give me some time to finish the refactoring. I'll release all results as fast as possible her in U-Boot list.

But, one issue was uncleare to me. Some functions, for example xps_ll_temac_hostif_set(), were prepared to set bit 10 of CTL register. But CTL bit 10 is not defined by any LL TEMAC documentation. Why did you done this?

Thanks,
Stephan

Quoted 10 lines

Thanks,
Michal

Signature · 5 lines

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

Signature · 8 lines

Viele Gr??e,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz

Michal Simek

2011-11-25 10:28 UTC #7 lore raw

PATCH 1/1 microblaze: usable uart16550 for big endian systems

Quoted 7 lines · Stephan Linz

Am Donnerstag, den 24.11.2011, 20:13 +0100 schrieb Michal Simek:
Quoted 6 lines
Quoted 5 lines · Stephan Linz
--snip--
Quoted 1 line
Here is patch I have used. Please add that changes to v2 patch.
I'll do it this way. Give me a little time to change and test it. Currently I am sill working on refactoring of the LL TEMAC driver. I hope, the refactored driver than can merge in mainline ...

Quoted 6 lines

Quoted 2 lines
I have done it some time ago and look at "[PATCH v3] net: ll_temac:
Add LL TEMAC driver to u-boot thread".
Hi Michal,
yes, I've read the whole thread.

ok.

Quoted 5 lines

Quoted 3 lines
I have attached the latest version I have and I am not going to change it to follow "new" u-boot network driver style
Hm, but it is possible ...

sure, it is software almost everything is possible.

Quoted 10 lines

Quoted 2 lines
because I would like to keep ppc dcr support.
Even this was the main work I've done last week. I've split the driver code into differnt sub modules (xilinx_ll_temac_fifo and xilinx_ll_temac_sdram), introduce some call back functions into the sub modules to harmonize the main driver code and divide the sdma code into the two different bus access methodes: direct 32 bit memory access (Microblaze) and indirect access via DCR (Xilinx PPC4xx).

Quoted 2 lines

Please, give me some time to finish the refactoring. I'll release all results as fast as possible her in U-Boot list.

ok. Look forward for patches.

Quoted 5 lines

But, one issue was uncleare to me. Some functions, for example xps_ll_temac_hostif_set(), were prepared to set bit 10 of CTL register. But CTL bit 10 is not defined by any LL TEMAC documentation. Why did you done this?

I was in origin Yoshio Kashiwagi's driver. emac parameter should be possible to remove entirely as below.

static unsigned int xps_ll_temac_hostif_get(struct eth_device *dev,
int phy_addr, int reg_addr)

{
	struct temac_reg *regs = (struct temac_reg *)dev->iobase;

	out_be32(&regs->lsw, (phy_addr << 5) | reg_addr);
	out_be32(&regs->ctl, MIIMAI);
	xps_ll_temac_check_status(regs, XTE_RSE_MIIM_RR_MASK);
	return in_be32(&regs->lsw);
}

Thanks,
Michal

Signature · 4 lines

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