On 07/27/2018 06:36 AM, Robin Murphy wrote:

On 27/07/18 01:22, Grygorii Strashko wrote:
[...]

quoted

quoted

the result of this change is pretty strange as for me :(
Resulted code:

????/*
???? * If @dev is expected to be DMA-capable then the bus code that 
created
???? * it should have initialised its dma_mask pointer by this point. 
For
???? * now, we'll continue the legacy behaviour of coercing it to the
???? * coherent mask if not, but we'll no longer do so quietly.
???? */
????if (!dev->dma_mask) {
??????? dev_warn(dev, "DMA mask not set\n");
??????? dev->dma_mask = &dev->coherent_dma_mask;
^this will always produce warning in case of platform-bus or if there 
are no bus driver.
even if DT contains correct definition of dma-range
????}

????if (!size && dev->coherent_dma_mask)

^ coherent_dma_mask is zero, so size will not be calculated

pls, ignore this particular comment

quoted

??????? size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
????else if (!size)
??????? size = 1ULL << 32;

????dev->dma_pfn_offset = offset;

????/*
???? * Limit coherent and dma mask based on size and default mask
???? * set by the driver.
???? */
????mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
????dev->bus_dma_mask = mask;
????dev->coherent_dma_mask &= mask;

^^ if nobody set coherent_dma_mask before it will stay null forever 
unless drivers
will overwrite it. Again even if DT has correct definition for 
dma-range.

That is intentional. Consider a 32-bit device on a bus with an upstream 
DMA range of 40 bits (which could easily happen with e.g. PCI). If the 
firmware code gives that device 40-bit DMA masks and the driver doesn't 
change them, then DMA is not going to work correctly. Therefore the 
firmware code cannot safely make {coherent_}dma_mask larger than the 
default value passed in, and if the default is 0 then that should be 
fixed in whatever code created the device, not worked around later.

Could you clarify what do you mean "firmware" here?

On most ARM32 platforms there is only DT + kernel.
And DT is usually only one source of information about DT configuration.
Sry, but seems this changes makes impossible using DT for DMA parameters 
configuration any more.

Robin.

quoted

quoted

????*dev->dma_mask &= mask;

????coherent = of_dma_is_coherent(np);
????dev_dbg(dev, "device is%sdma coherent\n",
??????? coherent ? " " : " not ");

FYI, with below diff i can boot at least:

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 5957cd4..f7dc121 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c

@@ -150,8 +150,8 @@ int of_dma_configure(struct device *dev, struct 

device_node *np, bool force_dma)
????????? */
???????? mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
???????? dev->bus_dma_mask = mask;
-?????? dev->coherent_dma_mask &= mask;
-?????? *dev->dma_mask &= mask;
+?????? dev->coherent_dma_mask = mask;
+?????? *dev->dma_mask = mask;
???????? coherent = of_dma_is_coherent(np);
???????? dev_dbg(dev, "device is%sdma coherent\n",

-- 
regards,
-grygorii