Re: [PATCH v7 5/9] PCI: Add pci_iomap_wc() variants

On Jul 2, 2015, at 11:49 AM, Luis R. Rodriguez [off-list ref] wrote:

On Sat, Jun 27, 2015 at 08:00:48AM +1000, Benjamin Herrenschmidt wrote:

quoted

On Fri, 2015-06-26 at 16:24 +0000, Casey Leedom wrote:

quoted

 Thanks for looking into this Ben.  As it stands now, it seems as
if Write Combined mappings simply aren't supported and/or all
driver writers trying to utilize Write Combined mappings have to
"hand roll" their own solutions which really isn't supportable.

 One thing that might be considered is simply to treat the desire
to utilize the Write Combining hardware as a separate issue and
develop writel_wc(), writeq_wc(), etc.

That seems rather sloppy and cumbersome, its best to just provide the
infrastructure for initial mapping for an area and let the hardware do it for
you. With the current design drivers would just use regular read/write on all
areas and the only thing that will set them apart will be the mapping.  With
what you propose we'd end up having to shift a whole bunch of reads/writes for
just the purpose of adding WC to an area that didn't have wc before. That's
a waste of code and time.

quoted

quoted

Those could be defined
as legal only for Write Combined Mappings and would "do the
right thing" for each architecture.  

Yuck.

  Yeah, probably.  But it sounds like from Ben’s response, we should really use write_relaxed() as an already existing API for this.  But most of the architectures just define this as writel() so some work would need to be done to get it to work.

quoted

The question then is what is "the right thing". In the powerpc case,
we'll have a non-garded mapping, which means we also get no ordering
between load and stores.

I don't follow, you *ordering* between load and stores for WC? We should
not need that for WC, its why WC is used for only very specific things
such as framebuffer and PIO (which BTw I still don't quite get all this
use case for infiniband to be honest, and I will note I do see some
proprietary hardware extensions like bursts but nothing covering all
this in a general doc, I think I think it all just has to do that this
is a hardware hack in reality, which we sell as a feature).

  I was talking about the work that our drivers (cxgb4, cxgb4vf, etc.) do to ensure correct ordering between writes to memory and I/O space.  For instance, issuing a wmb() between writes to a DMA buffer and the write to a register telling the hardware that the data is available.  This isn’t necessary on the Strongly Ordered Intel architectures but is necessary on other architectures.

Casey