lkml.org 
[lkml]   [2011]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 09/12] unicore32 machine related files: hardware registers
Date
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


\
 
 \ /
  Last update: 2011-02-18 11:45    [W:0.143 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site