On Friday 18 February 2011 10:52:12 Guan Xuetao wrote: > > On Wednesday 16 February 2011, Guan Xuetao wrote: > > > +#define io_p2v(x) ((x) - PKUNITY_IOSPACE_BASE) > > > +#define io_v2p(x) ((x) + PKUNITY_IOSPACE_BASE) > > > + > > > +#ifndef __ASSEMBLY__ > > > + > > > +# define __REG(x) (*((volatile unsigned long *)io_p2v(x))) > > > +# define __PREG(x) (io_v2p((unsigned long)&(x))) > > > + > > > +#else > > > + > > > +# define __REG(x) io_p2v(x) > > > +# define __PREG(x) io_v2p(x) > > > > > #define PKUNITY_IOSPACE_BASE 0x80000000 /* 0x80000000 - 0xFFFFFFFF 2GB */ > > > > The typecasts look wrong here: > > > > - "volatile unsigned long*" is not the right pointer type to do I/O on. It > > should instead be "void __iomem *". Please use the "sparse" tool with "make C=1" > > to get warnings about incorrect pointer type accesses. > __REG() macro could be used in both left and right sides of assignment sentence. > This idea is borrowed from arm/sa1100. > > When used in left side, __REG is a register port, and when used in right side, > __REG is just the value of the register. > It is a trick, but very useful. I'd like to remain the macros. Ok, I didn't realize this. However, this is a really bad idea and I would strongly advise removing this throughout the code. The only way we do MMIO accesses in Linux is through the readl()/writel() functions. On many architectures, these contain more than just a pointer dereference, and if you need that, you can change it in just one place. Things that can go wrong with volatile pointer dereferences include: * compiler turns 32 bit access into four 8-bit accesses * I/O bus reorders access and it moves outside of a spinlock * non-volatile accesses to system RAM get reordered around MMIO access, which may break if the MMIO triggers a DMA. > diff --git a/arch/unicore32/include/mach/PKUnity.h b/arch/unicore32/include/mach/PKUnity.h > index 1e13208..39f27f4 100644 > --- a/arch/unicore32/include/mach/PKUnity.h > +++ b/arch/unicore32/include/mach/PKUnity.h > @@ -21,7 +21,7 @@ > * Memory Definitions > */ > #define PKUNITY_SDRAM_BASE 0x00000000 /* 0x00000000 - 0x7FFFFFFF 2GB */ > -#define PKUNITY_IOSPACE_BASE 0x80000000 /* 0x80000000 - 0xFFFFFFFF 2GB */ > +#define PKUNITY_MMIO_BASE 0x80000000 /* 0x80000000 - 0xFFFFFFFF 2GB */ > #define PKUNITY_PCI_BASE 0x80000000 /* 0x80000000 - 0xBFFFFFFF 1GB */ > #include "regs-pci.h" > #define PKUNITY_BOOT_ROM2_BASE 0xF4000000 /* 0xF4000000 - 0xF7FFFFFF 64MB */ This change is ok. > diff --git a/arch/unicore32/include/mach/hardware.h b/arch/unicore32/include/mach/hardware.h > index 3fb7236..ebce7de 100644 > --- a/arch/unicore32/include/mach/hardware.h > +++ b/arch/unicore32/include/mach/hardware.h > @@ -17,19 +17,14 @@ > > #include "PKUnity.h" > > -#define io_p2v(x) ((x) - PKUNITY_IOSPACE_BASE) > -#define io_v2p(x) ((x) + PKUNITY_IOSPACE_BASE) > +#define io_p2v(x) ((x) - PKUNITY_MMIO_BASE) > +#define io_v2p(x) ((x) + PKUNITY_MMIO_BASE) > > #ifndef __ASSEMBLY__ The result of io_p2v really needs to be a (void __iomem *) pointer, not an integer, so the accesses can work. > # define __REG(x) (*((volatile unsigned long *)io_p2v(x))) > # define __PREG(x) (io_v2p((unsigned long)&(x))) And as explained, these should be removed and all users converted to use readl()/writel(). Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
