[RFC PATCH] of: Fix DMA configuration for non-DT masters

On Wed, Mar 29, 2017 at 11:12 PM, Robin Murphy [off-list ref] wrote:

On 29/03/17 06:46, Oza Oza wrote:

quoted

On Wed, Mar 29, 2017 at 10:23 AM, Oza Oza [off-list ref] wrote:

quoted

On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy [off-list ref] wrote:

quoted

For PCI masters not represented in DT, we pass the OF node of their
associated host bridge to of_dma_configure(), such that they can inherit
the appropriate DMA configuration from whatever is described there.
Unfortunately, whilst this has worked for the "dma-coherent" property,
it turns out to miss the case where the host bridge node has a non-empty
"dma-ranges", since nobody is expecting the 'device' to be a bus itself.

It transpires, though, that the de-facto interface since the prototype
change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help
re-use") is very clear-cut: either the master_np argument is redundant
with dev->of_node, or dev->of_node is NULL and master_np is the relevant
parent bus. Let's ratify that behaviour, then teach the whole
of_dma_configure() pipeline to cope with both cases properly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>

[...]

quoted

quoted

pci and memory mapped device world is different.

???

No they aren't. There is nothing magic about PCI. PCI *is* a
memory-mapped bus.

The only PCI-specific aspect here is the Linux code path which passes a
host controller node into of_dma_configure() where the latter expects a
node for the actual endpoint device. It managed to work for
"dma-coherent", because that may appear either directly on a device node
or on any of its parent buses, but "dma-ranges" is *only* valid for DT
nodes representing buses, thus reveals that using a parent bus to stand
in for a device isn't actually correct. I only posted that horrible hack
patch to prove the point that having a child node to represent the
actual device is indeed sufficient to make of_dma_configure() work
correctly for PCI masters as-is (modulo the other issues).

quoted

of course if you talk

quoted

from iommu perspective, all the master are same for IOMMU

I don't understand what you mean by that. There's nothing about IOMMUs
here, it's purely about parsing DT properties correctly.

quoted

quoted

but the original intention of the patch is to try to solve 2 problems.
please refer to https://lkml.org/lkml/2017/3/29/10

One patch should not solve two separate problems anyway. Taking a step
back, there are at least 3 things that this discussion has brought up:

1) The way in which we call of_dma_configure() for PCI devices causes
the "dma-ranges" property on PCI host controllers to be ignored.

2) of_dma_get_range() only considers the first entry in any "dma-ranges"
property.

3) When of_dma_configure() does set a DMA mask, there is nothing on
arm64 and other architectures to prevent drivers overriding that with a
wider mask later.

Those are 3 separate problems, to solve with 3 or more separate patches,
and I have deliberately put the second and third to one side for the
moment. This patch fixes problem 1.

quoted

quoted

1) expose generic API for pci world clients to configure their
dma-ranges. right now there is none.
2) same API can be used by IOMMU to derive dma_mask.

while the current patch posted to handle dma-ranges for both memory
mapped and pci devices, which I think is overdoing.

No, of_dma_configure() handles the "dma-ranges" property as it is
defined in the DT spec to describe the mapping between a child bus
address space and a parent bus address space. Whether those
memory-mapped parent and child buses are PCI, ISA, AMBA, HyperTransport,
or anything else is irrelevant other than for the encoding of the
address specifiers. All this patch does is sort out the cases where we
have a real device node to start at, from the cases where we don't and
so start directly at the device's parent bus node instead.

quoted

quoted

besides we have different configuration of dma-ranges based on iommu
is enabled or not enabled.

That doesn't sound right, unless you mean the firmware actually programs
the host controller's AXI bridge differently for system configurations
where the IOMMU is expected to be used or not? (and even then, I don't
really see why it would be necessary to do that...)

quoted

quoted

 #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00
0x80000000 0x00 0x80000000 \
                                      0x43000000 0x08 0x00000000 0x08
0x00000000 0x08 0x00000000 \
                                      0x43000000 0x80 0x00000000 0x80
0x00000000 0x40 0x00000000>;
Not sure if this patch will consider above dma-ranges.

my suggestion is to leave existing of_dma_get_range as is, and have
new function for pci world as discussed in
please refer to https://lkml.org/lkml/2017/3/29/10

And then we keep adding new functions to do the exact same thing for
every other discoverable bus type whose bridge is be described in DT? I
fail to see how that is in any way better than simply fixing the
existing code to work as it was intended to.

of_dma_get_ranges() uses of_translate_dma_address() just the same way as
existing PowerPC PCI code does, which further disproves your assertion
that parsing PCI addresses is somehow special - it's really only a
matter of getting the right number of address cells in order to to read
a child address to give to of_translate_dma_address() in the first
place. Incidentally, I now notice that the proposed
of_pci_get_dma_ranges() is incomplete as it doesn't use
of_translate_dma_address() or otherwise traverse upwards through the
dma-ranges of any further parent buses.

quoted

quoted

Regards,
Oza.

also I see that, in case of multiple ranges of_dma_get_range doesnt handle that.
and also it is not meant to handle.

Yes, the existing code doesn't handle multiple dma-ranges entries,
because nobody's had the need to implement that so far, and this patch
does not change that because it's fixing a separate problem.

Now, of course of_dma_get_range() *should* be capable of handling
multiple entries regardless of this patch, and I'm going to write *that*
patch right now (it's going to be a case of adding a handful of lines
which probably won't even conflict with this one at all). If we had a
bunch of different range parsing functions, we'd then have to duplicate
the equivalent logic across all of them, which is clearly undesirable
when it can easily be avoided altogether.

Robin.

quoted

so with this patch will return wrong size and hence wrong dma_mask.
having said, I think it is better to separate pci world dma-ranges
function on of_pci.c

for which my discussion with Rob already, (same thread)
https://lkml.org/lkml/2017/3/29/10
Waiting for Rob's viewpoint on this.


Regards,
Oza.

In my opinion NAKing to this, because

1) some reasons already mentioned in https://lkml.org/lkml/2017/3/29/10

2) also of_dma_get_range is supposed to handle traditional dma-ranges
(not pci one)

3) we need separate function for pci users such as iproc or rcar SOCs
to have their dma-ranges parsed,
which can be extended later for e.g. pci flags have some meanings.

besides of_dma_configure is written to pass only single out parameter
(..., *dma_addr, *size)
handling multiple ranges here, and passing only one range out is not desirable.

also you would not like to handle pci flags (the first portion of DT
entry) in this function if required in future.

this new function will be similar to of_pci_get_host_bridge_resources
https://lkml.org/lkml/2017/3/29/10
which I am writing/improving right now..


Regards,
Oza.