On 30 November 2017 at 17:42, David Miller [off-list ref] wrote:
From: Ard Biesheuvel <redacted>
Date: Thu, 30 Nov 2017 17:37:27 +0000
quoted
Well, the whole point of using memremap() instead of ioremap() is that
the region has memory semantics, i.e., we read the MAC address and the
DMA engine microcode from it. If memremap() itself is flawed in this
regard, I agree we should fix it. But as I understand it, this is
really an implementation problem in memremap() [the fact that it falls
back to ioremap()] and not a problem in this driver.
So what alternative would you propose in this case?
BTW, this should be IOREMAP_WC not IOREMAP_WT, because the EEPROM on
the platform in question does not tolerate cached mappings (or rather,
shareable mappings). ioremap_wt() happens to result in device mappings
rather than writethrough cacheable mappings, but this is another
problem in itself. Once arm64 fixes ioremap_wt(), this code will no
longer work on the SynQuacer SoC.
It doesn't "fall back", it directly uses ioremap_wt() for non-RAM
mappings.
It you look, most architectures do a "#define iomrep_wt ioremap"
OK, but that still means the implementation of memremap() is flawed,
not its use in this driver.
memremap() exists to allow non-DRAM regions to be mapped with memory
semantics, and if we currently implement that incorrectly, we should
fix it. But that still does not mean we should have __iomem
annotations and special accessors in this case, precisely because it
has memory semantics, and so it is a virtual pointer that may be
dereferenced normally.
So if memremap() is unsuitable, how should we map memory that is not DRAM?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ard Biesheuvel <redacted>
Date: Thu, 30 Nov 2017 17:48:44 +0000
On 30 November 2017 at 17:42, David Miller [off-list ref] wrote:
quoted
From: Ard Biesheuvel <redacted>
Date: Thu, 30 Nov 2017 17:37:27 +0000
quoted
Well, the whole point of using memremap() instead of ioremap() is that
the region has memory semantics, i.e., we read the MAC address and the
DMA engine microcode from it. If memremap() itself is flawed in this
regard, I agree we should fix it. But as I understand it, this is
really an implementation problem in memremap() [the fact that it falls
back to ioremap()] and not a problem in this driver.
So what alternative would you propose in this case?
BTW, this should be IOREMAP_WC not IOREMAP_WT, because the EEPROM on
the platform in question does not tolerate cached mappings (or rather,
shareable mappings). ioremap_wt() happens to result in device mappings
rather than writethrough cacheable mappings, but this is another
problem in itself. Once arm64 fixes ioremap_wt(), this code will no
longer work on the SynQuacer SoC.
It doesn't "fall back", it directly uses ioremap_wt() for non-RAM
mappings.
It you look, most architectures do a "#define iomrep_wt ioremap"
OK, but that still means the implementation of memremap() is flawed,
not its use in this driver.
memremap() exists to allow non-DRAM regions to be mapped with memory
semantics, and if we currently implement that incorrectly, we should
fix it. But that still does not mean we should have __iomem
annotations and special accessors in this case, precisely because it
has memory semantics, and so it is a virtual pointer that may be
dereferenced normally.
Memory semantics and how to access that memory are two different things.
On sparc64 the cacheability etc. can be specified in the load and
store instructions.
That's why I have all of the ioremap*() routines return a physical
address. The value returned from all of the ioremap*() interfaces is
completely opaque. That's why the return type is __iomem.
The iomem accessors on sparc64 use the appropriate access bits in the
load and store instructions as needed.
So in order for this to work properly, new sets of acessors need to be
implemented to let me give you the access semantics you want yet at
the same time allowing architectures to have the flexibility of
still returning an opaque "pointer".
There is no reason for me to make a virtual mapping on sparc64 because
it is completely unnecessary overhead since I can give you whatever
cacheability/IO/etc. attributes needed in the load and store
instructions themselves.
Thanks.
On 30 November 2017 at 17:58, David Miller [off-list ref] wrote:
From: Ard Biesheuvel <redacted>
Date: Thu, 30 Nov 2017 17:48:44 +0000
quoted
On 30 November 2017 at 17:42, David Miller [off-list ref] wrote:
quoted
From: Ard Biesheuvel <redacted>
Date: Thu, 30 Nov 2017 17:37:27 +0000
quoted
Well, the whole point of using memremap() instead of ioremap() is that
the region has memory semantics, i.e., we read the MAC address and the
DMA engine microcode from it. If memremap() itself is flawed in this
regard, I agree we should fix it. But as I understand it, this is
really an implementation problem in memremap() [the fact that it falls
back to ioremap()] and not a problem in this driver.
So what alternative would you propose in this case?
BTW, this should be IOREMAP_WC not IOREMAP_WT, because the EEPROM on
the platform in question does not tolerate cached mappings (or rather,
shareable mappings). ioremap_wt() happens to result in device mappings
rather than writethrough cacheable mappings, but this is another
problem in itself. Once arm64 fixes ioremap_wt(), this code will no
longer work on the SynQuacer SoC.
It doesn't "fall back", it directly uses ioremap_wt() for non-RAM
mappings.
It you look, most architectures do a "#define iomrep_wt ioremap"
OK, but that still means the implementation of memremap() is flawed,
not its use in this driver.
memremap() exists to allow non-DRAM regions to be mapped with memory
semantics, and if we currently implement that incorrectly, we should
fix it. But that still does not mean we should have __iomem
annotations and special accessors in this case, precisely because it
has memory semantics, and so it is a virtual pointer that may be
dereferenced normally.
Memory semantics and how to access that memory are two different things.
On sparc64 the cacheability etc. can be specified in the load and
store instructions.
That's why I have all of the ioremap*() routines return a physical
address. The value returned from all of the ioremap*() interfaces is
completely opaque. That's why the return type is __iomem.
The iomem accessors on sparc64 use the appropriate access bits in the
load and store instructions as needed.
So in order for this to work properly, new sets of acessors need to be
implemented to let me give you the access semantics you want yet at
the same time allowing architectures to have the flexibility of
still returning an opaque "pointer".
There is no reason for me to make a virtual mapping on sparc64 because
it is completely unnecessary overhead since I can give you whatever
cacheability/IO/etc. attributes needed in the load and store
instructions themselves.
But the whole point of memremap() is obtaining a virtual mapping that
does not require accessors, but can be used like ordinary memory. The
fact that Sparc64 can do nifty things using special accessors but
without a virtual mapping is interesting, but not really relevant
IMHO.
So I will work with Jassi to reimplement this using ioremap(), given
that it is not performance critical, and given the fact that your
criticism appears to be directed to memremap() in general, and not the
way it is being used here.
Thanks,
Ard.