RE: [PATCH 09/12] unicore32 machine related files: hardware registers

[Guan Xuetao]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> Sent: Friday, February 18, 2011 6:43 PM
> To: Guan Xuetao
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-arch@xxxxxxxxxxxxxxx; 'Greg KH'
> Subject: Re: [PATCH 09/12] unicore32 machine related files: hardware registers
> 
> 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().
I see.
I apply the patch as following:
arch/unicore32/include/mach/hardware.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/unicore32/include/mach/hardware.h b/arch/unicore32/include/mach/hardware.h
index ebce7de..b71405a 100644
--- a/arch/unicore32/include/mach/hardware.h
+++ b/arch/unicore32/include/mach/hardware.h
@@ -22,8 +22,7 @@
 
 #ifndef __ASSEMBLY__
 
-# define __REG(x)	(*((volatile unsigned long *)io_p2v(x)))
-# define __PREG(x)	(io_v2p((unsigned long)&(x)))
+# define __REG(x)	(void __iomem *)io_p2v(x)
 
 #endif

---
__REG reserved only for all registers prefix.
And all using for __REG converted to readl/writel.

> 
> 	Arnd

Thanks Arnd

Guan Xuetao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/