[PATCH linux] mtd: spi-nor: aspeed-smc: Use io string accessors to fifo

[Andrew Jeffery]

On Thu, 2016-02-18 at 22:50 +0000, Milton Miller II wrote:
> Around 02/11/2016 08:59PM in some timezone,  Andrew Jeffery wrote:
> > Hi Milton,
> > 
> > Excuse the unnecessary quote markers that might appear through the
> > patch, my mail client (Evolution) seems to have a mind of its own
> > sometimes. Starting to shave that yak. Anyway.
> 
> It can't be worse than me manually quoting in smart cloud notes client.
> 
> > 
> > As an aside I've tested an earlier version of the code and confirmed
> > the approach works (i.e. no JFFS2 corruption on re-mount).
> 
> Yea.
> 
> 
> > On Wed, 2016-02-10 at 21:50 -0600, OpenBMC Patches wrote:
> > > From: "Milton D. Miller II" <miltonm at us.ibm.com>
> > > 
> > > Make and use new routines to access the fifo when in user mode.
> > > Instead of using memcpy_fromio, base the new routines on the io
> > >  port string accessors.
> > > 
> ...
> > typo: unsigned
> fixed
> 
> 
> > > +static void *ins_align(unsigned long io, void *buf, size_t len)
> > > +{
> > > +> > > > 	> > > > BUILD_BUG_ON(0 & len & ~3);
> > 
> > Should just be BUG_ON()? Given that you disabled it with '0 &', do we
> > want it at all?
> 
> It should definitively not be a driver BUG that crashes the whole system.  If 
> you need I can search for Linus' quote about the use of BUG in drivers.

Sure - but if we needed to use a bug macro then it needs to be one that
triggers at runtime, as you've used a formal parameter whose value
won't be known at compile time right? That's what I was (poorly) trying
to express.

Unless the optimiser gets involved and reasons about actual parameters
from callers? Anyway, this is largely a distraction.

> 
> It can be killed.

SGTM

> 
> > > + */
> > > +
> > > +static void aspeed_smc_from_fifo(void *buf, const void __iomem *iop, size_t len)
> > > +{
> > > +> > > > 	> > > > unsigned long io = (__force unsigned long)iop;
> > > +
> ...
> > > +> > > > 	> > > > > > > 	> > > > buf = ins_align(io, buf, len & 1);
> > > +> > > > 	> > > > } else {
> > > +> > > > 	> > > > > > > 	> > > > size_t l = (4 - (unsigned long)buf) & 3;
> > > +
> > > +> > > > 	> > > > > > > 	> > > > l = min(l, len);
> > > +> > > > 	> > > > > > > 	> > > > buf = ins_align(io, buf, l);
> > > +> > > > 	> > > > > > > 	> > > > len -= l;
> > > +
> > > +> > > > 	> > > > > > > 	> > > > if (len >= 4) {
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > insl(io, buf, len >> 2);
> > > +> > > > 	> > > > > > > 	> > > > > > > 	> > > > buf += len & ~3;
> > > +> > > > 	> > > > > > > 	> > > > }
> > > +> > > > 	> > > > > > > 	> > > > buf = ins_align(io, buf, len & 3);
> > > +> > > > 	> > > > }
> > 
> > So I've tried deduplicating this function - the code in the latter two
> > cases is largely the same bar the magic numbers and insw vs insl. I've
> > come up with the code below (note that I haven't actually probed the
> > modifications), but I'm not convinced it's much of a win in terms of
> > clarity:
> > 
> > /* Is this available somewhere else? Rusty doesn't think so.
> >  * Probably needs a better name (first set value) */
> > #define FS_VAL(x) ((x) & ~((x) - 1))
> 
> That is a horrible name!  What is this magic file system value about?

Yes, I admitted that in the comment!

> 
> > 
> > static void aspeed_smc_from_fifo(void *buf, const void __iomem *iop, size_t len)
> > {
> > 	> > unsigned long io = (__force unsigned long)iop;
> >         u8 align = FS_VAL((io & 7) | 4); /* align is one of 1, 2 or 4 */
> 
> I see masking & 7 and comment about being 1, 2, or 4 and think:
> 
> That's not true, the port bit doesn't always have bit 4 on when
> the others are off.
> 
> Only after a long pause coming back do I see the | 4, and then I have
> to go back to a macro above to figure out is it before or after the
> fancy math.

Yeah, adding further magic to what was there already was a dicey idea.
The code was more something to spark discussion of alternatives than a
genuine proposal.

> 
> 
> >         u8 mask = align - 1;
> >         size_t to_align = align - ((unsigned long)buf) & mask);
> > 
> > 	> > if (align == 1) {
> > 	> > 	> > insb(io, buf, len);
> >                 return;
> > 	> > }
> >         to_align = min(to_align, len);
> >         buf = ins_align(io, buf, to_align);
> >         len -= to_align;
> > 	> > if (len >= align) {
> > 	> > 	> > size_t nr_blocks = len / align;
> > 	> > 	> > if (align == 2) {
> > 	> > 	> > 	> > insw(io, buf, nr_blocks);
> > 	> > 	> > } else {
> > 	> > 	> > 	> > insl(io, buf, nr_blocks);
> > 	> > 	> > }
> > 	> > 	> > buf += (nr_blocks * align);
> > 	> > }
> >         buf = ins_align(io, buf, len & mask);
> > 
> > Is it worth it? A similar approach can be taken for
> > aspeed_smc_to_fifo() if so.
> 
> So I think this approach hides what is supposed to happen
> in the case we are actually using.
> 
> I decided on a different approach to the remove the duplication:

Cool, as mentioned above I mainly posted my hack to trigger thoughts
about alternatives, so I'm happy it's done that :)

> 
> Don't add code that will not be used.
> 
> We always pass a 4 byte aligned port.  Rather than bug, 
> just call insb if the io port is not aligned.  
> 
> Then remove the ins_align and outs_align function and 
> bring them inline, but expand top and bottom call sites 
> to order the byte vs (half)word transfers to remove
> the get/put_unaligned pessimisation since we are testing 
> the alignemnt and can choose the order by context.
> 
> I then removed the updates to buf and len that were dead
> in the tail code.
> 
> With 2 one-line comments and no white space it fits in 
> 80x25 which means you can see all the code after the 
> preliminary 0 length and unaligned checks on one screen.
> 
> 
> > 
> > As for whether there's a better approach short of fixing mmiocpy(), I
> > don't know, so no further suggestions really (i.e. I guess we just live
> > with the IO_SPACE_LIMIT jiggery-pokery).
> > 
> 
> When I started to go back and comment the assembly I figured out the
> memcpy has 2 places it can stutter:
> 
> It first aligns source to a word boundary with byte copies.  After that 
> it works on aligning the destination.  It will do an extra load at this
> point if the destination is not aligned to the source.  Then it sometimes 
> aligns to cache lines (depends on core if it helps), then transfers
> blocks, back to words, then if there are upto 3 bytes it backs up and
> switches back to bytes (reading bytes a second time here).
> 
> So there are two places to fix, one stutters output and one input.
> 
> I thought about switching read/write to engine mode.  I probably still
> will sometime.  The stutters will cause it to stop and resend the 
> address slowing down the access.  Hmm, that might cause double write 
> attempt at the stutter...

Ugh

> 
> Anyways, see what you think of the current code.

Will do, thanks for the detailed response!

Andrew

> 
> Thanks,
> milton
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160219/4bb43bb3/attachment.sig>