Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH 09/12] unicore32 machine related files: hardware registers | Date | Thu, 17 Feb 2011 17:59:45 +0100 |
| |
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.
- PKUNITY_IOSPACE_BASE seems to be both a virtual and a physical address, which is a bad idea, because it prevents a lot of the checks from working correctly.
- The __REG/__PREG macros seem to have the same purpose as io_p2v/io_v2p, you should not require both.
- The term IOSPACE is confusing, because it normally refers to the PCI PIO space, while you mean the SoC's MMIO region
I would recommend defining these as
#ifndef __ASSEMBLY__ #define PKUNITY_MMIO_VIRT ((void __iomem *)0x80000000) #else #define PKUNITY_MMIO_VIRT 0x80000000 #endif
#define io_p2v(x) ((x) - PKUNITY_MMIO_VIRT) #define io_v2p(x) ((x) + PKUNITY_MMIO_VIRT)
/* please remove the two macros below as soon as all users are changed to use io_p2v */ #define __REG(x) io_p2v(x) #define __PREG(x) io_v2p(x)
Arnd
| |