Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Mark Rutland]

[adding devicetree]

On Sun, Jun 05, 2016 at 11:20:29PM -0400, Bill Mills wrote:
> Keystone2 can do DMA coherency but only if:
> 1) DDR3A DMA buffers are in high physical addresses (0x8_0000_0000)
>     (DDR3B does not have this constraint)
> 2) Memory is marked outer shared
> 3) DMA Master marks transactions as outer shared
>     (This is taken care of in bootloader)
> 
> Use outer shared instead of inner shared.
> This choice is done at early init time and uses the attr_mod facility
> 
> If the kernel is not configured for LPAE and using high PA, or if the
> switch to outer shared fails, then we fail to meet this criteria.
> Under any of these conditions we veto any dma-coherent attributes in
> the DTB.

I very much do not like this. As I previously mentioned [1],
dma-coherent has de-facto semantics today. This series deliberately
changes that, and inverts the relationship between DT and kernel (as the
describption in the DT would now depend on the configuration of the
kernel).

I would prefer that we have a separate property (e.g.
"dma-outer-coherent") to describe when a device can be coherent with
Normal, Outer Shareable, Inner Write-Back, Outer Write-Back memory.
Then the kernel can figure out whether or not device can be used
coherently, depending on how it is configured.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/421470.html

> 
> Signed-off-by: Bill Mills <wmills@ti.com>
> ---
>  arch/arm/mach-keystone/keystone.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
> index a33a296..d10adaf 100644
> --- a/arch/arm/mach-keystone/keystone.c
> +++ b/arch/arm/mach-keystone/keystone.c
> @@ -28,6 +28,7 @@
>  #include "keystone.h"
>  
>  static unsigned long keystone_dma_pfn_offset __read_mostly;
> +static bool keystone_dma_coherent;
>  
>  static int keystone_platform_notifier(struct notifier_block *nb,
>  				      unsigned long event, void *data)
> @@ -52,21 +53,53 @@ static struct notifier_block platform_nb = {
>  	.notifier_call = keystone_platform_notifier,
>  };
>  
> +void veto_dma_coherent(void)
> +{
> +	struct device_node	*node, *start_node;
> +	struct property		*prop;
> +
> +	for (start_node = NULL;
> +	     (node = of_find_node_with_property(start_node, "dma-coherent"));
> +	     start_node = node) {
> +		prop = of_find_property(node, "dma-coherent", NULL);
> +		if (prop)
> +			of_remove_property(node, prop);
> +	}
> +}
> +
>  static void __init keystone_init(void)
>  {
> +	/* If we are running from the high physical addresses then adjust
> +	 * addresses we give to the device's DMA.  They will be seeing this
> +	 * memory through the MSMC address translation which makes the first 2GB
> +	 * of high memory appear in the low 4GB space.
> +	 * (DMA masters on keystone2 have 32 bit address buses)
> +	 */
>  	if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) {
>  		keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
>  						   KEYSTONE_LOW_PHYS_START);
>  		bus_register_notifier(&platform_bus_type, &platform_nb);
>  	}
> +
> +	/* if the kernel has not been configured to meet the keystone
> +	 * platform requirements to achieve DMA coherency, then ignore any
> +	 * device tree configuration for this
> +	 */
> +	if (!keystone_dma_coherent)
> +		veto_dma_coherent();
> +
>  	keystone_pm_runtime_init();
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  }
>  
>  static long long __init keystone_pv_fixup(void)
>  {
> +#ifdef CONFIG_ARM_LPAE
>  	long long offset;
>  	phys_addr_t mem_start, mem_end;
> +	bool dma_ok;
> +
> +	dma_ok = use_outer_shared();
>  
>  	mem_start = memblock_start_of_DRAM();
>  	mem_end = memblock_end_of_DRAM();
> @@ -84,11 +117,15 @@ static long long __init keystone_pv_fixup(void)
>  	}
>  
>  	offset = KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START;
> +	keystone_dma_coherent = dma_ok;
>  
>  	/* Populate the arch idmap hook */
>  	arch_phys_to_idmap_offset = -offset;
>  
>  	return offset;
> +#else
> +	return 0;
> +#endif
>  }
>  
>  static const char *const keystone_match[] __initconst = {
> -- 
> 1.9.1
> 

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Arnd Bergmann]

On Monday, June 6, 2016 9:56:27 AM CEST Mark Rutland wrote:
> [adding devicetree]
> 
> On Sun, Jun 05, 2016 at 11:20:29PM -0400, Bill Mills wrote:
> > Keystone2 can do DMA coherency but only if:
> > 1) DDR3A DMA buffers are in high physical addresses (0x8_0000_0000)
> >     (DDR3B does not have this constraint)
> > 2) Memory is marked outer shared
> > 3) DMA Master marks transactions as outer shared
> >     (This is taken care of in bootloader)
> > 
> > Use outer shared instead of inner shared.
> > This choice is done at early init time and uses the attr_mod facility
> > 
> > If the kernel is not configured for LPAE and using high PA, or if the
> > switch to outer shared fails, then we fail to meet this criteria.
> > Under any of these conditions we veto any dma-coherent attributes in
> > the DTB.
> 
> I very much do not like this. As I previously mentioned [1],
> dma-coherent has de-facto semantics today. This series deliberately
> changes that, and inverts the relationship between DT and kernel (as the
> describption in the DT would now depend on the configuration of the
> kernel).
> 
> I would prefer that we have a separate property (e.g.
> "dma-outer-coherent") to describe when a device can be coherent with
> Normal, Outer Shareable, Inner Write-Back, Outer Write-Back memory.
> Then the kernel can figure out whether or not device can be used
> coherently, depending on how it is configured.

I share your concern, but I don't think the dma-outer-coherent attribute
would be a good solution either.

The problem really is that keystone is a platform that is sometimes
coherent, depending purely on what kernel we run, and not at all on
anything we can describe in devicetree, and I don't see any good way
to capture the behavior of the hardware in generic DT bindings.

So far, the assumption has been:

- when running a non-LPAE kernel, keystone is not coherent, and we
  must ignore both the dma-coherent properties in devices and the
  dma-ranges properties in bus nodes.
- when running an LPAE kernel, keystone is coherent, and we must
  respect both of those.

My interpretation of Bill's description above is that we now have
an additional requirement that at least I was not aware of before,
regarding the outer-sharable attribute. I don't think there is
much value in making this a boot-time option, since everyone would
want to run this platform in a cache-coherent way if at all possible.

We already have special hacks to detect the case of keystone running
in LPAE mode, in order to do the special rewrite-all-page-tables
hack at boot time for relocating the physical address, and we could
use the same hack to change the page table attributes.

The question is how to communicate the requirement for outer-sharable
for a platform. If we think it's a safe assumption that there will
not be future 32-bit platforms with this requirement (or maybe one
or two more at most), we could leave it in the special keystone hack.
Alternatively, a DT property in an appropriate node could indicate
that a particular platform requires it.

	Arnd

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Mark Rutland]

On Mon, Jun 06, 2016 at 11:09:07AM +0200, Arnd Bergmann wrote:
> On Monday, June 6, 2016 9:56:27 AM CEST Mark Rutland wrote:
> > [adding devicetree]
> > 
> > On Sun, Jun 05, 2016 at 11:20:29PM -0400, Bill Mills wrote:
> > > Keystone2 can do DMA coherency but only if:
> > > 1) DDR3A DMA buffers are in high physical addresses (0x8_0000_0000)
> > >     (DDR3B does not have this constraint)
> > > 2) Memory is marked outer shared
> > > 3) DMA Master marks transactions as outer shared
> > >     (This is taken care of in bootloader)
> > > 
> > > Use outer shared instead of inner shared.
> > > This choice is done at early init time and uses the attr_mod facility
> > > 
> > > If the kernel is not configured for LPAE and using high PA, or if the
> > > switch to outer shared fails, then we fail to meet this criteria.
> > > Under any of these conditions we veto any dma-coherent attributes in
> > > the DTB.
> > 
> > I very much do not like this. As I previously mentioned [1],
> > dma-coherent has de-facto semantics today. This series deliberately
> > changes that, and inverts the relationship between DT and kernel (as the
> > describption in the DT would now depend on the configuration of the
> > kernel).
> > 
> > I would prefer that we have a separate property (e.g.
> > "dma-outer-coherent") to describe when a device can be coherent with
> > Normal, Outer Shareable, Inner Write-Back, Outer Write-Back memory.
> > Then the kernel can figure out whether or not device can be used
> > coherently, depending on how it is configured.
> 
> I share your concern, but I don't think the dma-outer-coherent attribute
> would be a good solution either.
> 
> The problem really is that keystone is a platform that is sometimes
> coherent, depending purely on what kernel we run, and not at all on
> anything we can describe in devicetree, and I don't see any good way
> to capture the behavior of the hardware in generic DT bindings.

I think that above doesn't quite capture the situation:

Some DMA masters can be cache-coherent (only) with Outer Shareable
transactions. That is a property we could capture inthe DT (e.g.
dma-outer-coherent), and is independent of the kernel configuration.

Whether or not the devices are coherent with the kernel's chosen memory
attributes certainly depends on the kernel configuration, but that is
not what we capture in the DT.

> So far, the assumption has been:
> 
> - when running a non-LPAE kernel, keystone is not coherent, and we
>   must ignore both the dma-coherent properties in devices and the
>   dma-ranges properties in bus nodes.

I wasn't able to spot if/where that was enforced. Is it possible to boot
Keystone UP, !LPAE?

> - when running an LPAE kernel, keystone is coherent, and we must
>   respect both of those.

Similarly this has not been enforced either way. Currently I cannot see
how devices with dma-coherent could possibly work with Keystone.

I think we also need to be clear as to what we mean by "keystone is
coherent". With LPAE, CPUs can be coherent with each other, so long as
the appropriate physical addresses are used (with the magic to handle
that).

Devices being cache-coherent with CPUs is already something we manage
per-device.

> My interpretation of Bill's description above is that we now have
> an additional requirement that at least I was not aware of before,
> regarding the outer-sharable attribute. I don't think there is
> much value in making this a boot-time option, since everyone would
> want to run this platform in a cache-coherent way if at all possible.
>
> We already have special hacks to detect the case of keystone running
> in LPAE mode, in order to do the special rewrite-all-page-tables
> hack at boot time for relocating the physical address, and we could
> use the same hack to change the page table attributes.
> 
> The question is how to communicate the requirement for outer-sharable
> for a platform. If we think it's a safe assumption that there will
> not be future 32-bit platforms with this requirement (or maybe one
> or two more at most), we could leave it in the special keystone hack.
> Alternatively, a DT property in an appropriate node could indicate
> that a particular platform requires it.

As above, I think this must be specified per-device, following the usual
manner in which we describe Inner Shareable coherency using
dma-coherent.

Thanks,
Mark.

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Russell King - ARM Linux]

On Mon, Jun 06, 2016 at 09:56:27AM +0100, Mark Rutland wrote:
> I very much do not like this. As I previously mentioned [1],
> dma-coherent has de-facto semantics today. This series deliberately
> changes that, and inverts the relationship between DT and kernel (as the
> describption in the DT would now depend on the configuration of the
> kernel).

dma-coherent's semantics are not very well defined - just grep for it
in Documention/devicetree/ and you'll find several different wordings
for what this property means.

Anyway, my point here is that all of these merely say that the hardware
is coherent in _some regard_ - it doesn't specify under what conditions
DMA coherency is guaranteed by the hardware.  It happens that on ARM,
most platforms give that guarantee when using inner shared mappings.  If
we were to use some other sharing, or disable sharing altogether (eg, by
disabling SMP support) then all these platforms would immediately break.

In other words, DMA coherence today already depends on the kernel's setup
of the page tables corresponding to the requirements of the hardware.

Keystone II is just slightly different - and as I understand it, TI
followed one of the early specifications that ARM Ltd produced.  That
specification may have contained errors, but unfortunately, we now have
a situation where there is hardware out there which followed in good
faith.

So, it seems to me to be entirely reasonable that Keystone II should
mark devices with the "dma-coherent" property - just the same way as
every other platform does.  It also seems to be entirely appropriate for
a platform to remove this property if it determines that the conditions
for DMA coherency are not met - in order to save the users data from
corruption.

TI Keystone II is not the only platform with issues here: there are
Marvell Armada platforms out there which have DMA coherence, but are
uniprocessor, we don't set the shared bit (which they require for
DMA coherence) and so we omit the dma-coherent property from the
device tree at the moment.  And they're inner-shared coherent.  We
just don't set the page tables up so that they can work.

So, I think to require a whole new property is absurd.  The existing
property means "if the rest of the system is appropriately configured,
this device can be dma-coherent".  So, what I think we need is a way
to communicate whether the rest of the system has been appropriately
configured, so the property can be attached to devices which meet the
criteria, but the arch/platform level can signal whether the conditions
for device DMA coherence have been met.  That's not a DT property,
that's a matter of how the kernel has setup the system.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Mark Rutland]

On Mon, Jun 06, 2016 at 12:43:21PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 06, 2016 at 09:56:27AM +0100, Mark Rutland wrote:
> > I very much do not like this. As I previously mentioned [1],
> > dma-coherent has de-facto semantics today. This series deliberately
> > changes that, and inverts the relationship between DT and kernel (as the
> > describption in the DT would now depend on the configuration of the
> > kernel).
> 
> dma-coherent's semantics are not very well defined - just grep for it
> in Documention/devicetree/ and you'll find several different wordings
> for what this property means.

Indeed. This is the tip of the iceberg w.r.t. under-specification of
memory attribute usage.

> Anyway, my point here is that all of these merely say that the hardware
> is coherent in _some regard_ - it doesn't specify under what conditions
> DMA coherency is guaranteed by the hardware.  It happens that on ARM,
> most platforms give that guarantee when using inner shared mappings.  If
> we were to use some other sharing, or disable sharing altogether (eg, by
> disabling SMP support) then all these platforms would immediately break.
> 
> In other words, DMA coherence today already depends on the kernel's setup
> of the page tables corresponding to the requirements of the hardware.

I agree that whether or not devices are coherent in practice depends on
the kernel's configuration. The flip side, as you point out, is that
devices are coherent when a specific set of attributes are used.

i.e. that if you read dma-coherent as meaning "coherent iff Normal,
Inner Shareable, Inner WB Cacheable, Outer WB Cacheable", then
dma-coherent consistently describes the same thing, rather than
depending on the configuration of the OS.

DT is a datastructure provided to the kernel, potentially without deep
internal knowledge of that kernel configuration. Having a consistent
rule that is independent of the kernel configuration seems worth aiming
for.

A dma-outer-coherent property would allow us to accurately describe the
keystone case in the same way, independent of kernel configuration.

> Keystone II is just slightly different - and as I understand it, TI
> followed one of the early specifications that ARM Ltd produced.  That
> specification may have contained errors, but unfortunately, we now have
> a situation where there is hardware out there which followed in good
> faith.

To be clear, I am not arguing against supporting keystone. I just wish
to avoid muddying the waters further w.r.t. the semantics of
dma-coherent, which I believe can be salvaged and made consistent.

Clearly, those semantics are the point of contention here.

Thanks,
Mark.

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[William Mills]

On 06/06/2016 07:59 AM, Mark Rutland wrote:
> On Mon, Jun 06, 2016 at 12:43:21PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Jun 06, 2016 at 09:56:27AM +0100, Mark Rutland wrote:
>>> I very much do not like this. As I previously mentioned [1],
>>> dma-coherent has de-facto semantics today. This series deliberately
>>> changes that, and inverts the relationship between DT and kernel (as the
>>> describption in the DT would now depend on the configuration of the
>>> kernel).
>>
>> dma-coherent's semantics are not very well defined - just grep for it
>> in Documention/devicetree/ and you'll find several different wordings
>> for what this property means.
> 
> Indeed. This is the tip of the iceberg w.r.t. under-specification of
> memory attribute usage.
> 
>> Anyway, my point here is that all of these merely say that the hardware
>> is coherent in _some regard_ - it doesn't specify under what conditions
>> DMA coherency is guaranteed by the hardware.  It happens that on ARM,
>> most platforms give that guarantee when using inner shared mappings.  If
>> we were to use some other sharing, or disable sharing altogether (eg, by
>> disabling SMP support) then all these platforms would immediately break.
>>
>> In other words, DMA coherence today already depends on the kernel's setup
>> of the page tables corresponding to the requirements of the hardware.
> 
> I agree that whether or not devices are coherent in practice depends on
> the kernel's configuration. The flip side, as you point out, is that
> devices are coherent when a specific set of attributes are used.
> 
> i.e. that if you read dma-coherent as meaning "coherent iff Normal,
> Inner Shareable, Inner WB Cacheable, Outer WB Cacheable", then
> dma-coherent consistently describes the same thing, rather than
> depending on the configuration of the OS.
>

Even w/o inner / outer it seems to me it is under specified.
What about a system where main DDR memory is DMA coherent but an on chip
SRAM needs manual flush/invalidate?  Right now dma coherency is all or
nothing.  In the above case you would need to ensure that the device
never tried to use that SRAM for dma transactions even though the device
is perfectly capable with the right hand-holding.  Alternatively you
could use the SRAM but penalize the normal case of DDR.

> DT is a datastructure provided to the kernel, potentially without deep
> internal knowledge of that kernel configuration. Having a consistent
> rule that is independent of the kernel configuration seems worth aiming
> for.
> 
> A dma-outer-coherent property would allow us to accurately describe the
> keystone case in the same way, independent of kernel configuration.
> 
>> Keystone II is just slightly different - and as I understand it, TI
>> followed one of the early specifications that ARM Ltd produced.  That
>> specification may have contained errors, but unfortunately, we now have
>> a situation where there is hardware out there which followed in good
>> faith.
> 
> To be clear, I am not arguing against supporting keystone. I just wish
> to avoid muddying the waters further w.r.t. the semantics of
> dma-coherent, which I believe can be salvaged and made consistent.
> 
> Clearly, those semantics are the point of contention here.
>

To me this seems like a choice between embracing outer-shared or
treating it like a quirk of some early armv7 devices.  With ARM's
current recommendations to use inner-shared for everything in many
contexts (like ARM servers etc), I was assuming you would want the latter.

Thanks for looking.  This is not the patch that I expected to generate
the most discussion.  :)

-- Bill

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Russell King - ARM Linux]

On Mon, Jun 06, 2016 at 12:59:18PM +0100, Mark Rutland wrote:
> I agree that whether or not devices are coherent in practice depends on
> the kernel's configuration. The flip side, as you point out, is that
> devices are coherent when a specific set of attributes are used.
> 
> i.e. that if you read dma-coherent as meaning "coherent iff Normal,
> Inner Shareable, Inner WB Cacheable, Outer WB Cacheable", then
> dma-coherent consistently describes the same thing, rather than
> depending on the configuration of the OS.
> 
> DT is a datastructure provided to the kernel, potentially without deep
> internal knowledge of that kernel configuration. Having a consistent
> rule that is independent of the kernel configuration seems worth aiming
> for.

I think you've missed the point.  dma-coherent is _already_ dependent on
the kernel configuration.  "Having a consistent rule that is independent
of the kernel configuration" is already an impossibility, as I illustrated
in my previous message concerning Marvell Armada SoCs, and you also said
in your preceding paragraph!

For example, if you clear the shared bit in the page tables on non-LPAE
SoCs, devices are no longer coherent.

DMA coherence on ARM _is_ already tightly linked with the kernel
configuration.  You already can't get away from that, so I think you
should give up trying to argue that point. :)

Whether devices are DMA coherent is a combination of two things:
 * is the device connected to a coherent bus.
 * is the system setup to allow coherency on that bus to work.

We capture the first through the dma-coherent property, which is clearly
a per-device property.  We ignore the second because we assume everyone
is going to configure the CPU side correctly.  That's untrue today, and
it's untrue not only because of Keystone II, but also because of other
SoCs as well which pre-date Keystone II.  We currently miss out on
considering that, because if we ignore it, we get something that works
for most platforms.

I don't see that adding a dma-outer-coherent property helps this - it's
muddying the waters somewhat - and it's also forcing additional complexity
into places where we shouldn't have it.  We would need to parse two
properties in the DMA API code, and then combine it with knowledge as
to how the system page tables have been setup.  If they've been setup
as inner sharable, then dma-coherent identifies whether the device is
coherent.  If they've been setup as outer sharable, then
dma-outer-coherent specifies that and dma-coherent is meaningless.

Sounds like a recipe for confusion.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Arnd Bergmann]

On Monday, June 6, 2016 12:42:56 PM CEST Mark Rutland wrote:
> On Mon, Jun 06, 2016 at 11:09:07AM +0200, Arnd Bergmann wrote:
> > On Monday, June 6, 2016 9:56:27 AM CEST Mark Rutland wrote:
> >
> > So far, the assumption has been:
> > 
> > - when running a non-LPAE kernel, keystone is not coherent, and we
> >   must ignore both the dma-coherent properties in devices and the
> >   dma-ranges properties in bus nodes.
> 
> I wasn't able to spot if/where that was enforced. Is it possible to boot
> Keystone UP, !LPAE?

With !LPAE, no devices are coherent, so that should work with both
SMP and and UP. Not sure about LPAE with coherent devices on UP,
IIRC we had a bug in that configuration on Armada XP, as the memory
was not marked as sharable at all there, and ended up not being
coherent with DMA masters.

My first guess is that uniprocessor mode on keystone has not been
tested at all (TI's QA seems to test only a very limited number of
configurations), so it may or may not work.

	Arnd

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[William Mills]

On 06/06/2016 07:42 AM, Mark Rutland wrote:
> On Mon, Jun 06, 2016 at 11:09:07AM +0200, Arnd Bergmann wrote:
>> On Monday, June 6, 2016 9:56:27 AM CEST Mark Rutland wrote:
>>> [adding devicetree]
>>>
>>> On Sun, Jun 05, 2016 at 11:20:29PM -0400, Bill Mills wrote:
>>>> Keystone2 can do DMA coherency but only if:
>>>> 1) DDR3A DMA buffers are in high physical addresses (0x8_0000_0000)
>>>>     (DDR3B does not have this constraint)
>>>> 2) Memory is marked outer shared
>>>> 3) DMA Master marks transactions as outer shared
>>>>     (This is taken care of in bootloader)
>>>>
>>>> Use outer shared instead of inner shared.
>>>> This choice is done at early init time and uses the attr_mod facility
>>>>
>>>> If the kernel is not configured for LPAE and using high PA, or if the
>>>> switch to outer shared fails, then we fail to meet this criteria.
>>>> Under any of these conditions we veto any dma-coherent attributes in
>>>> the DTB.
>>>
>>> I very much do not like this. As I previously mentioned [1],
>>> dma-coherent has de-facto semantics today. This series deliberately
>>> changes that, and inverts the relationship between DT and kernel (as the
>>> describption in the DT would now depend on the configuration of the
>>> kernel).
>>>
>>> I would prefer that we have a separate property (e.g.
>>> "dma-outer-coherent") to describe when a device can be coherent with
>>> Normal, Outer Shareable, Inner Write-Back, Outer Write-Back memory.
>>> Then the kernel can figure out whether or not device can be used
>>> coherently, depending on how it is configured.
>>
>> I share your concern, but I don't think the dma-outer-coherent attribute
>> would be a good solution either.
>>
>> The problem really is that keystone is a platform that is sometimes
>> coherent, depending purely on what kernel we run, and not at all on
>> anything we can describe in devicetree, and I don't see any good way
>> to capture the behavior of the hardware in generic DT bindings.
> 
> I think that above doesn't quite capture the situation:
> 
> Some DMA masters can be cache-coherent (only) with Outer Shareable
> transactions. That is a property we could capture inthe DT (e.g.
> dma-outer-coherent), and is independent of the kernel configuration.
> 
> Whether or not the devices are coherent with the kernel's chosen memory
> attributes certainly depends on the kernel configuration, but that is
> not what we capture in the DT.
> 
>> So far, the assumption has been:
>>
>> - when running a non-LPAE kernel, keystone is not coherent, and we
>>   must ignore both the dma-coherent properties in devices and the
>>   dma-ranges properties in bus nodes.
> 
> I wasn't able to spot if/where that was enforced. Is it possible to boot
> Keystone UP, !LPAE?
> 

Yes ...  with the right combination of DTB, u-boot, u-boot vars, and
kernel config.  Mismatches either fail hard or use dma-coherent ops
without actually providing coherency. I am attempting to make this less
fragile.

Mis-configured coherency can be dead-wrong and still only fail 1
transaction in 1,000,000.  I have seen customers run for weeks or months
w/o detecting the issue.  Thats why I wanted the veto logic.

There are 3 cases to cover:
LPAE w/ high PA:
	this is the normal mode for KS2.  Uses coherent dma-ops.
!LPAE:
	obviously uses low PA and must use non-coherent dma-ops.
LPAE w/ low PA:
	This happens with an LPAE kernel but the user has passed a low
	PA memory DTB and u-boot has not fixed it up.
	This case must also use non-coherent dma-ops

Upstream DTS has keystone memory at the low PA.  I agree with that.
U-boot and kernel opt-in to the use of high PA.

If you give high PA to a non-LPAE kernel I believe it will fail hard and
fast.  I can check.

Thanks,
Bill

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Santosh Shilimkar]

On 6/6/2016 5:50 AM, William Mills wrote:
>
>
I saw only v2 but seems like it already generated
discussion(s)

> On 06/06/2016 07:42 AM, Mark Rutland wrote:
>> On Mon, Jun 06, 2016 at 11:09:07AM +0200, Arnd Bergmann wrote:
>>> On Monday, June 6, 2016 9:56:27 AM CEST Mark Rutland wrote:
>>>> [adding devicetree]
>>>>
>>>> On Sun, Jun 05, 2016 at 11:20:29PM -0400, Bill Mills wrote:
>>>>> Keystone2 can do DMA coherency but only if:
>>>>> 1) DDR3A DMA buffers are in high physical addresses (0x8_0000_0000)
>>>>>     (DDR3B does not have this constraint)
>>>>> 2) Memory is marked outer shared
>>>>> 3) DMA Master marks transactions as outer shared
>>>>>     (This is taken care of in bootloader)
>>>>>
>>>>> Use outer shared instead of inner shared.
>>>>> This choice is done at early init time and uses the attr_mod facility
>>>>>
>>>>> If the kernel is not configured for LPAE and using high PA, or if the
>>>>> switch to outer shared fails, then we fail to meet this criteria.
>>>>> Under any of these conditions we veto any dma-coherent attributes in
>>>>> the DTB.
>>>>
>>>> I very much do not like this. As I previously mentioned [1],
>>>> dma-coherent has de-facto semantics today. This series deliberately
>>>> changes that, and inverts the relationship between DT and kernel (as the
>>>> describption in the DT would now depend on the configuration of the
>>>> kernel).
>>>>
>>>> I would prefer that we have a separate property (e.g.
>>>> "dma-outer-coherent") to describe when a device can be coherent with
>>>> Normal, Outer Shareable, Inner Write-Back, Outer Write-Back memory.
>>>> Then the kernel can figure out whether or not device can be used
>>>> coherently, depending on how it is configured.
>>>
>>> I share your concern, but I don't think the dma-outer-coherent attribute
>>> would be a good solution either.
>>>
>>> The problem really is that keystone is a platform that is sometimes
>>> coherent, depending purely on what kernel we run, and not at all on
>>> anything we can describe in devicetree, and I don't see any good way
>>> to capture the behavior of the hardware in generic DT bindings.
>>
>> I think that above doesn't quite capture the situation:
>>
>> Some DMA masters can be cache-coherent (only) with Outer Shareable
>> transactions. That is a property we could capture inthe DT (e.g.
>> dma-outer-coherent), and is independent of the kernel configuration.
>>
>> Whether or not the devices are coherent with the kernel's chosen memory
>> attributes certainly depends on the kernel configuration, but that is
>> not what we capture in the DT.
>>
>>> So far, the assumption has been:
>>>
>>> - when running a non-LPAE kernel, keystone is not coherent, and we
>>>   must ignore both the dma-coherent properties in devices and the
>>>   dma-ranges properties in bus nodes.

Correct.
>>
>> I wasn't able to spot if/where that was enforced. Is it possible to boot
>> Keystone UP, !LPAE?
>>
>
> Yes ...  with the right combination of DTB, u-boot, u-boot vars, and
> kernel config.  Mismatches either fail hard or use dma-coherent ops
> without actually providing coherency. I am attempting to make this less
> fragile.
>
> Mis-configured coherency can be dead-wrong and still only fail 1
> transaction in 1,000,000.  I have seen customers run for weeks or months
> w/o detecting the issue.  Thats why I wanted the veto logic.
>
> There are 3 cases to cover:
> LPAE w/ high PA:
> 	this is the normal mode for KS2.  Uses coherent dma-ops.
> !LPAE:
> 	obviously uses low PA and must use non-coherent dma-ops.
> LPAE w/ low PA:
> 	This happens with an LPAE kernel but the user has passed a low
> 	PA memory DTB and u-boot has not fixed it up.
> 	This case must also use non-coherent dma-ops
>
> Upstream DTS has keystone memory at the low PA.  I agree with that.
> U-boot and kernel opt-in to the use of high PA.
>
> If you give high PA to a non-LPAE kernel I believe it will fail hard and
> fast.  I can check.
>
UP will mostly boot from boot view the memory. The keystone_pv_fixup()
will bail out for higher PA. Let me know if you see otherwise.

Regards,
Santosh

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Santosh Shilimkar]

(Joining discussion late since only this thread showed up in my
inbox)

On 6/6/2016 5:32 AM, Russell King - ARM Linux wrote:
> On Mon, Jun 06, 2016 at 12:59:18PM +0100, Mark Rutland wrote:
>> I agree that whether or not devices are coherent in practice depends on
>> the kernel's configuration. The flip side, as you point out, is that
>> devices are coherent when a specific set of attributes are used.
>>
>> i.e. that if you read dma-coherent as meaning "coherent iff Normal,
>> Inner Shareable, Inner WB Cacheable, Outer WB Cacheable", then
>> dma-coherent consistently describes the same thing, rather than
>> depending on the configuration of the OS.
>>
I think there is a bit of miss-understanding with 'dma-coherent'
DT property and as RMK pointed out "dma-coherent-outer" isn't
right direction either.

>> DT is a datastructure provided to the kernel, potentially without deep
>> internal knowledge of that kernel configuration. Having a consistent
>> rule that is independent of the kernel configuration seems worth aiming
>> for.
>
> I think you've missed the point.  dma-coherent is _already_ dependent on
> the kernel configuration.  "Having a consistent rule that is independent
> of the kernel configuration" is already an impossibility, as I illustrated
> in my previous message concerning Marvell Armada SoCs, and you also said
> in your preceding paragraph!
>
> For example, if you clear the shared bit in the page tables on non-LPAE
> SoCs, devices are no longer coherent.
>
> DMA coherence on ARM _is_ already tightly linked with the kernel
> configuration.  You already can't get away from that, so I think you
> should give up trying to argue that point. :)
>
> Whether devices are DMA coherent is a combination of two things:
>  * is the device connected to a coherent bus.
>  * is the system setup to allow coherency on that bus to work.
>
> We capture the first through the dma-coherent property, which is clearly
> a per-device property.  We ignore the second because we assume everyone
> is going to configure the CPU side correctly.  That's untrue today, and
> it's untrue not only because of Keystone II, but also because of other
> SoCs as well which pre-date Keystone II.  We currently miss out on
> considering that, because if we ignore it, we get something that works
> for most platforms.
>
I agree with Russell. When I added "dma-coherent" per device DT
property, the intention was to distinguish certain devices which may
not be coherent sitting on coherent fabric for some hardware reasons.

> I don't see that adding a dma-outer-coherent property helps this - it's
> muddying the waters somewhat - and it's also forcing additional complexity
> into places where we shouldn't have it.  We would need to parse two
> properties in the DMA API code, and then combine it with knowledge as
> to how the system page tables have been setup.  If they've been setup
> as inner sharable, then dma-coherent identifies whether the device is
> coherent.  If they've been setup as outer sharable, then
> dma-outer-coherent specifies that and dma-coherent is meaningless.
>
> Sounds like a recipe for confusion.
>
Exactly. We should leave the "dma-coherent" property to mark coherent
vs non coherent device(s).

The inner vs outer is really page table ARCH setup issue and should
be handled exactly the way it was done first place to handle the
special memory view(outside 4 GB).

Keystone needs outer shared bit set while setting up MMU pages
which is best done in MMU off mode while recreating the new
page tables.

Regards,
Santosh

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Mark Rutland]

On Mon, Jun 06, 2016 at 01:32:10PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 06, 2016 at 12:59:18PM +0100, Mark Rutland wrote:
> > I agree that whether or not devices are coherent in practice depends on
> > the kernel's configuration. The flip side, as you point out, is that
> > devices are coherent when a specific set of attributes are used.
> > 
> > i.e. that if you read dma-coherent as meaning "coherent iff Normal,
> > Inner Shareable, Inner WB Cacheable, Outer WB Cacheable", then
> > dma-coherent consistently describes the same thing, rather than
> > depending on the configuration of the OS.
> > 
> > DT is a datastructure provided to the kernel, potentially without deep
> > internal knowledge of that kernel configuration. Having a consistent
> > rule that is independent of the kernel configuration seems worth aiming
> > for.
> 
> I think you've missed the point.  dma-coherent is _already_ dependent on
> the kernel configuration. 

I understood this point. Please, allow me to clarify below, as I've
evidently not done a good job so far.

> "Having a consistent rule that is independent of the kernel
> configuration" is already an impossibility, as I illustrated in my
> previous message concerning Marvell Armada SoCs, and you also said in
> your preceding paragraph!

That's not quite what I said. What I said was that whether or not you
end up with coherency depends on the kernel's configuration. That's why
I pointed out that in practice, the only cases that work with today's
mainline kernels are this for which the devices which are coherent with
the kernel's usual memory attributes in an SMP configuration.

If you grep for dma-coherent in arch/arm/boot/dts, you'll find that
appears in:

arch/arm/boot/dts/artpec6.dtsi
arch/arm/boot/dts/ecx-common.dtsi
arch/arm/boot/dts/ls1021a.dtsi

Which are all SMP Cortex-{A7,A9,A15} platforms, and:

arch/arm/boot/dts/k2e.dtsi
arch/arm/boot/dts/k2e-netcp.dtsi
arch/arm/boot/dts/k2hk-netcp.dtsi
arch/arm/boot/dts/k2l-netcp.dtsi
arch/arm/boot/dts/keystone.dtsi

For which, as far as I am aware, the dma-coherent property does not
yield coherency with a mainline kernel, due to the requirement of Outer
Shareable attributes.

So, if we codify the dma-coherent semantics as only matching the working
case today, then it becomes consistent and independent of kernel
configuration, and we can add properties to cater for the other cases,
independent of kernel configuration.

> For example, if you clear the shared bit in the page tables on non-LPAE
> SoCs, devices are no longer coherent.

Yes. This is a problem, but one that we already face. If we clarified
the semantics as above, we would know that the device is simply not
coherent.

> DMA coherence on ARM _is_ already tightly linked with the kernel
> configuration.  You already can't get away from that, so I think you
> should give up trying to argue that point. :)

I hope that I've clarified my position w.r.t. coherence vs specification
thereof. :)

> Whether devices are DMA coherent is a combination of two things:
>  * is the device connected to a coherent bus.
>  * is the system setup to allow coherency on that bus to work.
> 
> We capture the first through the dma-coherent property, which is clearly
> a per-device property.  We ignore the second because we assume everyone
> is going to configure the CPU side correctly.  That's untrue today, and
> it's untrue not only because of Keystone II, but also because of other
> SoCs as well which pre-date Keystone II.  We currently miss out on
> considering that, because if we ignore it, we get something that works
> for most platforms.
> 
> I don't see that adding a dma-outer-coherent property helps this - it's
> muddying the waters somewhat - and it's also forcing additional complexity
> into places where we shouldn't have it.  We would need to parse two
> properties in the DMA API code, and then combine it with knowledge as
> to how the system page tables have been setup.  If they've been setup
> as inner sharable, then dma-coherent identifies whether the device is
> coherent.  If they've been setup as outer sharable, then
> dma-outer-coherent specifies that and dma-coherent is meaningless.

I think that at minimum, the attributes devices require needs to be
describe to the kernel, rather than being something we hope just
happened to match.

> Sounds like a recipe for confusion.

Unfortunately, I think everything in this area leads to confusion. :(

Thanks,
Mark.
--
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

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Russell King - ARM Linux]

On Tue, Jun 07, 2016 at 11:01:43AM +0100, Mark Rutland wrote:
> So, if we codify the dma-coherent semantics as only matching the working
> case today, then it becomes consistent and independent of kernel
> configuration, and we can add properties to cater for the other cases,
> independent of kernel configuration.

That's where our points of view differ.  You claim that it becomes
independent of the kernel configuration.  I'm saying that's total
rubbish, because it's dependent on the kernel setting the CPU page
tables up as it does today.

If we set them up differently, then it doesn't work so well.  This
is evidenced by Marvell Armada uniprocessor platforms, where they
are DMA coherent provided that the S bit is set.  However, because
they are uniprocessor platforms, the kernel sets the page tables up
with the S bit clear.  That means that the kernel configures the
system in a way which results in it being non-coherent.

So here, we have an example of why your position is actually incorrect.
dma-coherent does *not* give a "consistent and independent of kernel
configuration" property - it's inherently tied to how the kernel has
setup the page tables.

So, dma-coherent is coherent _provided_ the kernel sets the page tables
up as we currently expect it to - the S bit set on non-LPAE systems, on
LPAE systems, inner-sharable.  If we deviate from that, (eg by clearing
the S bit on non-LPAE systems) we end up with a non-coherent system,
even if dma-coherent is specified.

The Keystone II case is no different - Keystone II is coherent if the
correct conditions are met with the CPU page tables.  The only difference
is that it's a slightly different set of conditions.

> > For example, if you clear the shared bit in the page tables on non-LPAE
> > SoCs, devices are no longer coherent.
> 
> Yes. This is a problem, but one that we already face. If we clarified
> the semantics as above, we would know that the device is simply not
> coherent.

How?  We would need to introduce some flag which is passed from the
architecture code into the OF code to disable the effect of dma-coherent,
making of_dma_is_coherent() return false if the S bit is clear.

> > Whether devices are DMA coherent is a combination of two things:
> >  * is the device connected to a coherent bus.
> >  * is the system setup to allow coherency on that bus to work.
> > 
> > We capture the first through the dma-coherent property, which is clearly
> > a per-device property.  We ignore the second because we assume everyone
> > is going to configure the CPU side correctly.  That's untrue today, and
> > it's untrue not only because of Keystone II, but also because of other
> > SoCs as well which pre-date Keystone II.  We currently miss out on
> > considering that, because if we ignore it, we get something that works
> > for most platforms.
> > 
> > I don't see that adding a dma-outer-coherent property helps this - it's
> > muddying the waters somewhat - and it's also forcing additional complexity
> > into places where we shouldn't have it.  We would need to parse two
> > properties in the DMA API code, and then combine it with knowledge as
> > to how the system page tables have been setup.  If they've been setup
> > as inner sharable, then dma-coherent identifies whether the device is
> > coherent.  If they've been setup as outer sharable, then
> > dma-outer-coherent specifies that and dma-coherent is meaningless.
> 
> I think that at minimum, the attributes devices require needs to be
> describe to the kernel, rather than being something we hope just
> happened to match.

Yuck.  Seriously?  What happens when we have two devices which have
different required attributes for the CPU mapping?  Should architecture
code have to parse the entire DT tree to work out what attributes each
device needs, and try to then work out how the CPU page tables should
be setup?

I really don't think that's a good idea.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC v2 4/4] ARM: keystone: dma-coherent with safe fallback

[Mark Rutland]

On Tue, Jun 07, 2016 at 01:32:48PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 07, 2016 at 11:01:43AM +0100, Mark Rutland wrote:
> > So, if we codify the dma-coherent semantics as only matching the working
> > case today, then it becomes consistent and independent of kernel
> > configuration, and we can add properties to cater for the other cases,
> > independent of kernel configuration.
> 
> That's where our points of view differ.  You claim that it becomes
> independent of the kernel configuration.  I'm saying that's total
> rubbish, because it's dependent on the kernel setting the CPU page
> tables up as it does today.

The key point is that *description* of the requirements for coherency
becomes independent of kernel configuration. Yes, whether or not a
kernel can support those depends on the configuration.

> If we set them up differently, then it doesn't work so well.  This
> is evidenced by Marvell Armada uniprocessor platforms, where they
> are DMA coherent provided that the S bit is set.  However, because
> they are uniprocessor platforms, the kernel sets the page tables up
> with the S bit clear.  That means that the kernel configures the
> system in a way which results in it being non-coherent.
> 
> So here, we have an example of why your position is actually incorrect.
> dma-coherent does *not* give a "consistent and independent of kernel
> configuration" property - it's inherently tied to how the kernel has
> setup the page tables.

Sorry, but that is not quite what I said.

I said that if you read dma-coherent as specifying *the requirements*
for coherency (i.e. "coherent iff Normal, Inner Shareable, Inner WB
Cacheable, Outer WB Cacheable"), rather than specifying that there is
coherency given some unspecified requirements, then it is possible to
use it in a manner which is consistent and independent of kernel
configuration. If the kernel uses memory attributes that don't meet
those requirements, it can know that the device cannot be used in a
coherent manner.

If we take that stance, then we can cater for other requirements (e.g.
Outer Shareable on Keystone) by having properties to specify those
requirements (e.g. dma-outer-coherent). The tricky part is how the
kernel decides how best to use that information, but that is a problem
regardless.

> > > For example, if you clear the shared bit in the page tables on non-LPAE
> > > SoCs, devices are no longer coherent.
> > 
> > Yes. This is a problem, but one that we already face. If we clarified
> > the semantics as above, we would know that the device is simply not
> > coherent.
> 
> How?  We would need to introduce some flag which is passed from the
> architecture code into the OF code to disable the effect of dma-coherent,
> making of_dma_is_coherent() return false if the S bit is clear.

Yes, we would need to either alter the OF code, or some code which makes
use of this. Surely it's possible to have this logic in an arch
callback?

> > > Whether devices are DMA coherent is a combination of two things:
> > >  * is the device connected to a coherent bus.
> > >  * is the system setup to allow coherency on that bus to work.
> > > 
> > > We capture the first through the dma-coherent property, which is clearly
> > > a per-device property.  We ignore the second because we assume everyone
> > > is going to configure the CPU side correctly.  That's untrue today, and
> > > it's untrue not only because of Keystone II, but also because of other
> > > SoCs as well which pre-date Keystone II.  We currently miss out on
> > > considering that, because if we ignore it, we get something that works
> > > for most platforms.
> > > 
> > > I don't see that adding a dma-outer-coherent property helps this - it's
> > > muddying the waters somewhat - and it's also forcing additional complexity
> > > into places where we shouldn't have it.  We would need to parse two
> > > properties in the DMA API code, and then combine it with knowledge as
> > > to how the system page tables have been setup.  If they've been setup
> > > as inner sharable, then dma-coherent identifies whether the device is
> > > coherent.  If they've been setup as outer sharable, then
> > > dma-outer-coherent specifies that and dma-coherent is meaningless.
> > 
> > I think that at minimum, the attributes devices require needs to be
> > describe to the kernel, rather than being something we hope just
> > happened to match.
> 
> Yuck.  Seriously?  What happens when we have two devices which have
> different required attributes for the CPU mapping?  Should architecture
> code have to parse the entire DT tree to work out what attributes each
> device needs, and try to then work out how the CPU page tables should
> be setup?

No, we do not necessarily have to try to dynamically handle every
possible case, especially as the vastly common case is the one I called
out above.

For those boards where we're going to have some code special-casing
those regardless, automatically deciding to have the kernel use the
preferred set of attributes is fine. However, to do this I don't think
we should provide board+kernel specific semantics to dma-coherent, and
should at least precisely specify the coherency requirements.

Thanks,
Mark.
--
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