Re: [Uclinux-dist-devel] [PATCH] spi/bfin_sport_spi: new driver to emulate a SPI bus using Blackfin SPORT peripheral

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Mon, Jan 10, 2011 at 09:39:40AM -0500, Mike Frysinger wrote:
> On Mon, Nov 1, 2010 at 01:56, Grant Likely wrote:
> > On Fri, Oct 22, 2010 at 02:53:35AM -0400, Mike Frysinger wrote:
> >> +     /* SPI framework hookup */
> >> +     struct spi_master *master;
> >> +
> >> +     /* Regs base of SPI controller */
> >> +     volatile struct sport_register __iomem *regs;
> >
> > Drop the volatile.  Registers must never be dereferenced directly
> > anyway.
> >
> >> +static void bfin_sport_spi_enable(struct master_data *drv_data)
> >> +{
> >> +     drv_data->regs->tcr1 |= TSPEN;
> >> +     drv_data->regs->rcr1 |= TSPEN;
> >
> > Illegal direct dereference; use ioread/iowrite instead.  Ditto through
> > rest of file.
> 
> that's not correct.  ioread/iowrite are fine if the devices were
> generically mapped through say the asynchronous memory bus, but these
> registers arent.  they're mapped directly into the Blackfin hardware
> system bus and have different semantics than the async memory bus.

It is not okay to use direct register dereference in Linux for well
documented reasons (see volatile-considered-harmful.txt).  In
particular, accessors prevent either the compiler or the CPU from
reordering or optimizing out accesses to registers in a predictable
way.  volatile is strongly discouraged.  I may have the specific
accessor routine name wrong for the blackfin use case, but direct
dereference is definitely not okay.

> 
> >> +     SSYNC();
> >
> > The SSYNC shouldn't be necessary with the correct accessors.
> 
> independently incorrect from the ioread/iowrite comment.  there's no
> way any register accessors function would ever impose a SSYNC on the
> Blackfin architecture.  this causes a *system* wide sync of *all*
> registers and as such, is used only when necessary.  if we did as you
> proposed and added them to every single read/write, it would kill
> performance.

Fair enough.

g.