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

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

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

Signed-off-by: Stephan Linz <linz@li-pro.net>
---
 include/configs/microblaze-generic.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
index 6b3fd76..cd3d2a1 100644
--- a/include/configs/microblaze-generic.h
+++ b/include/configs/microblaze-generic.h
@@ -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)
-- 
1.7.0.4

[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

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


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

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

[Stephan Linz]

Am Montag, den 21.11.2011, 08:21 +0100 schrieb 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?


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.



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

[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

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

[Stephan Linz]

Am Mittwoch, den 23.11.2011, 15:40 +0100 schrieb 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.
> >>
> >> --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.

> 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

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

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

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

[Michal Simek]

Stephan Linz wrote:
> Am Mittwoch, den 23.11.2011, 15:40 +0100 schrieb 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.
>>>>
>>>> --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.
> 
>> 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 ...

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

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

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

[Stephan Linz]

Am Donnerstag, den 24.11.2011, 20:13 +0100 schrieb Michal Simek:
> Stephan Linz wrote:
> >--snip--
> >>
> >> 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.

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

> 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

> 
> Thanks,
> Michal
> 
> -- 
> 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 
-- 
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

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

[Michal Simek]

Stephan Linz wrote:
> Am Donnerstag, den 24.11.2011, 20:13 +0100 schrieb Michal Simek:
>> Stephan Linz wrote:
>>> --snip--
>>>> 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.

ok.

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

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

ok. Look forward for patches.

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


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