Re: [PATCH v2 7/7] OF: Don't set default coherent DMA mask

[Grygorii Strashko via iommu <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>]

On 07/27/2018 01:13 PM, Russell King - ARM Linux wrote:
> On Fri, Jul 27, 2018 at 12:36:00PM +0100, Robin Murphy wrote:
>> 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.
> 
> I think you're missing the point.
> 
> When OF creates platform devices, they are created without any DMA masks
> what so ever.  It is only during device probe that dma_configure() gets
> called, which then calls of_dma_configure(), where the DMA mask for any
> DT declared device is setup.
> 
> What this means is that your change has broken all existing DT devices
> that are trying to use DMA, because they now end up with a zero coherent
> DMA mask and/or streaming DMA mask.
> 
> Your original idea behind the patch may be a sound one, but since the
> implementation has caused a regression, the implementation is obviously
> broken, and that needs fixing or reworking in some way.
> 

There was additional patch [1] which fixes an issue.

But I have a question to all:
- The patch [1] sets default DMA mask to DMA_BIT_MASK(32)
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	if (!dev->dev.dma_mask)
+		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;

this will also work the same way for ARM64

- But of_dma_configure() still have code which does:
	dev->coherent_dma_mask &= mask;
	*dev->dma_mask &= mask;

As result, DMA mask supplied from DT will always be truncated
 to 32 bit for platform devices. for example:

soc0: soc0 {
        compatible = "simple-bus";
        #address-cells = <2>;
        #size-cells = <2>;
        ranges;
+        dma-ranges = <0 0 0 0 0x10000 0>; 

^ 48 bit DMA mask expected to be generated and assigned.

But real mask will be DMA_BIT_MASK(32). As result, any 
DMA capable driver will have be modified to do dma_set_xxx_mask(),
which disregards DT DMA configuration (especially for HW modules
which are used on ARM32 and ARM64).

Could it be considered to do one the changes below?

1) assign mask in case of dt
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);

2) use BITS_PER_LONG for default DMA mask for of_platform devices

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 7ba90c2..3f326e2 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -185,7 +185,7 @@ static struct platform_device *of_platform_device_create_pdata(
        if (!dev)
                goto err_clear_flag;
 
-       dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+       dev->dev.coherent_dma_mask = DMA_BIT_MASK(BITS_PER_LONG);
        if (!dev->dev.dma_mask)
                dev->dev.dma_mask = &dev->dev.coherent_dma_mask;

3) ...

[1] https://www.spinics.net/lists/arm-kernel/msg668934.html
-- 
regards,
-grygorii