On Fri, 10 Feb 2006, Stephen Street wrote:
> On Fri, 2006-02-10 at 12:40 -0500, Nicolas Pitre wrote:
> > [...]
> >
> > +#define SSP_REG(x) (*((volatile unsigned long *)x))
> >
> > Don't do that. Instead, please use:
> >
> > #define DEFINE_SSP_REG(reg, off) \
> > static inline u32 read_##reg(void *p) { return __raw_readl(p + (off)); \
> > static inline void write_##reg(u32 v, void *p) { __raw_writel(v, p + (off)); }
> >
> > DEFINE_SSP_REG(SSCR0, 0x00)
> > DEFINE_SSP_REG(SSCR1, 0x04)
> > DEFINE_SSP_REG(SSSR, 0x08)
> > DEFINE_SSP_REG(SSITR, 0x0c)
> > DEFINE_SSP_REG(SSDR, 0x10)
> > DEFINE_SSP_REG(SSTO, 0x28)
> > DEFINE_SSP_REG(SSPSP, 0x2c)
> >
> Ok, I'll do this (Andrew made a similar comment). But...
>
> I modeled my SSP_REG on the contents of include/asm-
> arm/arch_pxa/pxa_reg.h which makes extensive use of __REG defined in
> include/asm-arm/arch_pxa/hardware.h as
>
> #define __REG(x) (*((volatile u32 *)io_p2v(x)))
>
> which is effectively my SSP_REG without the io_p2v because I preloaded
> the virtual SSP register addresses in the pxa2xx_spi_probe function.
True. But you should pretend to never have seen that. This macro is low
level stuff and if it changes for whatever reason it is better if driver
code didn't reimplement it. Furthermore the argument to __REG() is
always a constant not a variable making it a simple absolute address
that the compiler can use and optimize for pretty effectively.
> For my education:
>
> Why are __raw_readl and friends better than exploiting the memory map
> I/O nature of the PXA2xx and other SOCs?
Actually, it will do almost the same as you originally did if you look
at its definition. However, because a solution needs to be implemented
to support multiple ports then using __raw_readl() at the driver level
is more familiar to other kernel people.
> Especially since something like
>
> SSP_REG(sscr1) &= ~SSCR1_TIE;
>
> becomes
>
> write_SSCR1(read_SSCR1(reg) & ~SSCR_TIE, reg);
>
> Further what should I do with the DMA register accesses (i.e. DCSR,
> DCMD, etc)?
They are fine already.
In fact, if there was only one SSP port, I'd have asked you to use
SSPCR0, SSPDR, etc. directly. But the fact that the driver can handle
multiple ports makes it rather messy (and the SSP*_P() macros in
pxa-regs.h are an abomination IMHO).
Nicolas
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
