| Message ID | 1319449593-2427-3-git-send-email-macpaul@andestech.com |
|---|---|
| State | Superseded |
| Headers | show |
> 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! > Thanks for your comment. I'll try to do it tomorrow (GMT +0800).
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/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 *). Well please fix. You can send a cleanup series? > > 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. > > And there are also problems when translating readb from macro to > inline function. Why? > 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 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. 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? 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?
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 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? However I have no idea is there any other architecture like x86. >> 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. >> 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) > 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'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; }
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
diff --git a/arch/nds32/include/asm/io.h b/arch/nds32/include/asm/io.h index 2504c2b..a221a4d 100644 --- a/arch/nds32/include/asm/io.h +++ b/arch/nds32/include/asm/io.h @@ -98,13 +98,21 @@ extern void __raw_readsl(unsigned int addr, void *data, int longlen); #define __raw_readw(a) __arch_getw(a) #define __raw_readl(a) __arch_getl(a) -#define writeb(v, a) __arch_putb(v, a) -#define writew(v, a) __arch_putw(v, a) -#define writel(v, a) __arch_putl(v, a) +/* + * TODO: The kernel offers some more advanced versions of barriers, it might + * have some advantages to use them instead of the simple one here. + */ +#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 writew(v, c) ({ u16 __v = v; __iowmb(); __arch_putw(__v, c); __v; }) +#define writel(v, c) ({ u32 __v = v; __iowmb(); __arch_putl(__v, c); __v; }) -#define readb(a) __arch_getb(a) -#define readw(a) __arch_getw(a) -#define readl(a) __arch_getl(a) +#define readb(c) ({ u8 __v = __arch_getb(c); __iormb(); __v; }) +#define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) +#define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) /* * The compiler seems to be incapable of optimising constants
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> --- arch/nds32/include/asm/io.h | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)