On Friday 28 October 2011 00:46:38 馬克泡 wrote:

Since the NDS32 has been come into mainline. Could I pick up this kind of simple changes of reviewed patches which are related to NDS32 only by myself, and then send pull-request to you? Hope this could reducing your loading and gain some benefits

you've got a maintainer tree now: http://git.denx.de/?p=u-boot/u-boot-nds32.git

so you should be gathering all the NDS32 patches and pushing to wolfgang. please see the documentation here for the process: http://www.denx.de/wiki/U-Boot/DevelopmentProcess

short answer: - fixes should get pushed at any time - everything else should get pushed during merge window -mike

Hi Macpaul,

2011/10/25 Anton Staaf robotboy@chromium.org:

On Mon, Oct 24, 2011 at 2:46 AM, Macpaul Lin macpaul@andestech.com wrote:

...

Add ARCH_DMA_MINALIGN definition to asm/cache.h

Signed-off-by: Macpaul Lin macpaul@andestech.com

Acked-by: Anton Staaf robotboy@chromium.org

Applied to u-boot-nds32/master, thanks.

Best regards, Macpaul Lin

This patch add required __iormb and __iowmb to io.h. This also fix some misbehavior to periphal drivers.

This io.h has been fixed with referencing arm/include/asm/io.h.

Signed-off-by: Macpaul Lin macpaul@andestech.com

Hi,

can you possibly implement those as inline functions ? It'd be also great if you could convert the other macros to inline functions.

Thanks!

Hi Marek,

2011/10/24 Marek Vasut marek.vasut@gmail.com

...

Hi,

can you possibly implement those as inline functions ? It'd be also great if you could convert the other macros to inline functions.

Thanks!

I've found there are some different data type warning occur when I'm translating inline function since all the driver originately call write and read marco.

Most of them is easy to deal with sicne the address is usually a 32-bit address "unsigned int *". However, the driver/serial/ns16550.c used readb/writeb but the "address" passed in ns16550.c is byte-wised (unsigned char *).

And there are also address casted in (ulong) in ns16550.c. serial_out(UART_LCR_BKSE | UART_LCRVAL, (ulong)&com_port->lcr); I'm not sure to fix this way would affect ns16550 in other architecutre?. #define serial_out(x, y) writeb(x, (unsigned int *)y)

And there are also problems when translating readb from macro to inline function. Some driver cannot be compiled with a return value inside the readb. I think I can post the fix of writeb for NDS32 architecture and see if you give some comments then I do for other architecture later.

Hi Wolfgang, Could you please pickup these 2 patches since it was independent to r/w inline functions? I'm not sure if the merge windows has been closed. Or should I pick these 2 patches into nds32 repo?

nds32: Use getenv_ulong() in place of getenv(), strtoul http://patchwork.ozlabs.org/patch/121312/

nds32: cache: define ARCH_DMA_MINALIGN for DMA buffer alignment http://patchwork.ozlabs.org/patch/121313/

Thanks!

Hi Marek,

2011/10/25 Marek Vasut marek.vasut@gmail.com:

[snip]

...

Hi Marek,

And there are also address casted in (ulong) in ns16550.c. serial_out(UART_LCR_BKSE | UART_LCRVAL, (ulong)&com_port->lcr); I'm not sure to fix this way would affect ns16550 in other architecutre?. #define serial_out(x, y)       writeb(x, (unsigned int *)y)

This is definitelly wrong. I'd like to see others comment on this one.

I'm agree with you but I'm not sure why dose it need a ulong casting (64 bit data?) since I'm not familiar with ns16550 hardware.

Please check ns16550.c, current code has another such kind of casting #ifdef CONFIG_SYS_NS16550_PORT_MAPPED #define serial_out(x, y) outb(x, (ulong)y) #define serial_in(y) inb((ulong)y) [...] #else #define serial_out(x, y) writeb(x, y) #define serial_in(y) readb(y) #endif

...

And there are also problems when translating readb from macro to inline function.

Why?

Oh I've just found where the problem is inside the io.h There are the same problem in other io.h (and in linux) if the specific macro wasn't defined, the header will use a kind of default macro. I'll post the patch for NDS32 later which also fix this problem.

Plesse give comment to the PATCH v2 later and see if everyone like such kind of implement.

Hi Macpaul

On Wed, Oct 26, 2011 at 4:19 PM, 馬克泡 macpaul@gmail.com wrote:

Hi Graeme,

2011/10/25 馬克泡 macpaul@gmail.com:

...

Hi Marek,

2011/10/25 Marek Vasut marek.vasut@gmail.com:

[snip]

Please check ns16550.c, current code has another such kind of casting #ifdef CONFIG_SYS_NS16550_PORT_MAPPED #define serial_out(x, y) outb(x, (ulong)y) #define serial_in(y) inb((ulong)y) [...] #else #define serial_out(x, y) writeb(x, y) #define serial_in(y) readb(y) #endif

I'm going to change writeb/readb series functions from macro into inline functions.

For all arches? Have you seen /arch/x86/include/asm/io.h?

However, I've found data type casting problem in NS16550. Only here the serial_out with CONFIG_SYS_NS16550_PORT_MAPPED is used for board "eNET". Is this a necessary for this machine to access 8-bytes data?

outb/inb access 8-bit data in a 16-bit address space and the ns16650 has (for the x86 architecture at least) byte aligned registers

Since inline functions are type sensitive, I've suggest to replace the macro for other machines like writeb(x, (uint)y) but Marek think this is not good enough. Could you give some comments?

Hmm, but eNET (well, all x86 really) would not use writeb/readb - outb/inb would be used instead so if you are only touching the writeb/readb, eNET would never see this. So I don't understand where your type conflicts are occuring

Looking at the ulong cast, I was wondering why it was there, and the commit diff says I wrote it which is a bit of a worry :) x86 only has 65535 ports accessible via outb and inb, so I would have thought the case should have been a ushort, not a ulong.

I'm sorry, but without a patch to see what you are actually doing, I'm having a bit of difficulty picturing what is happening and what could be going wrong...

Regards,

Graeme

Hi Macpaul,

2011/10/27 馬克泡 macpaul@gmail.com:

Hi Graeme,

在 2011年10月26日下午1:55，Graeme Russ graeme.russ@gmail.com 寫道：

...

Hi Macpaul

[snip]

...

...

I'm going to change writeb/readb series functions from macro into inline functions.

For all arches? Have you seen /arch/x86/include/asm/io.h?

Currently, I have take a looked into arm, mips, and blackfin. Because the most probably common problem lead with the function replacement might be NS16550, so I check NS16550 at first.

According to your reminding, it seem there is no necessary for x86 to replace the macro. Is that correct?

Yes, I think so - x86 will use inb/outb not readb/writeb. For all other arch's, I suspect these may be one and the same

However I have no idea is there any other architecture like x86.

No, x86 is unique - It define 'port mapped I/O' to free up the 64kB of memory that would otherwise be taken up by device I/O - that was 10% of the memory on a 640kB PC :)

...

...

Is this a necessary for this machine to access 8-bytes data?

outb/inb access 8-bit data in a 16-bit address space and the ns16650 has (for the x86 architecture at least) byte aligned registers

Okay, so it's not actually an "ulong" casting.

No

...

...

Since inline functions are type sensitive, I've suggest to replace the macro for other machines like writeb(x, (uint)y) but Marek think this is not good enough. Could you give some comments?

Hmm, but eNET (well, all x86 really) would not use writeb/readb - outb/inb would be used instead so if you are only touching the writeb/readb, eNET would never see this. So I don't understand where your type conflicts are occuring

Because the the address passed into writeb and readb in ns16550.c will be "unsigned char" if they defined CONFIG_SYS_NS16550_REG_SIZE as "1" or "-4" and then will lead compile warnings when the address of inline function of writeb/readb is "unsigned int". (Please refer to include/ns16550.c)

Hmm, I can't provide any insight there

...

Looking at the ulong cast, I was wondering why it was there, and the commit diff says I wrote it which is a bit of a worry :) x86 only has 65535 ports accessible via outb and inb, so I would have thought the case should have been a ushort, not a ulong.

Okay, so we can double confirmed it is an ushort not an ulong.

I think so - I'll test if it build when cast as a ushort

...

I'm sorry, but without a patch to see what you are actually doing, I'm having a bit of difficulty picturing what is happening and what could be going wrong...

Sorry not to add you at the beginning when the discussion begin. Because when I sent this patch, I thought it is only related to NDS32 arch at first.

For examples, the origin implementation in macro is this. #define __arch_getb(a) (*(unsigned char *)(a)) #define __arch_putb(v, a) (*(unsigned char *)(a) = (v))

#define dmb() __asm__ __volatile__ ("" : : : "memory") #define __iormb() dmb() #define __iowmb() dmb()

#define writeb(v, c) ({ u8 __v = v; __iowmb(); __arch_putb(__v, c); __v; }) #define readb(a) __arch_getb(a)

The new implementation in inline function will be like this.

static inline void writeb(unsigned char val, unsigned char *addr) { __iowmb(); __arch_putb(val, addr); } static inline unsigned char readb(unsigned char *addr) { u8 val;

   val = __arch_getb(addr);
   __iormb();
   return val;

}

In arch/x86/include/io.h:

#define readb(addr) (*(volatile unsigned char *) (addr))

Feel free to change

inb/outb are already inline funtions (via some very ugly macros)

Regards,

Graeme

Hi Macpaul,

2011/10/25 Macpaul Lin macpaul@andestech.com:

1. This patch add required __iormb and __iowmb to io.h.

This also fix some misbehavior to periphal drivers.   This io.h has been fixed with referencing arm/include/asm/io.h. 2. This patch replaced macro writeb and readb into inline function.

Signed-off-by: Macpaul Lin macpaul@andestech.com

Changes for v2:  - translate writeb macro into inline function.

arch/nds32/include/asm/io.h |   72 +++++++++++++++++++++++++++++++------------  1 files changed, 52 insertions(+), 20 deletions(-)

Applied to u-boot-nds32/master, thanks.

Best regards, Macpaul Lin