Re: [PATCH 00/11] of: Fix DMA configuration for non-DT masters

[Nicolas Saenz Julienne]

On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
> On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne
> <nsaenzjulienne@xxxxxxx> wrote:
> > Hi All,
> > this series tries to address one of the issues blocking us from
> > upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that
> > devices not represented in DT which sit behind a PCI bus fail to get the
> > bus' DMA addressing constraints.
> > 
> > This is due to the fact that of_dma_configure() assumes it's receiving a
> > DT node representing the device being configured, as opposed to the PCIe
> > bridge node we currently pass. This causes the code to directly jump
> > into PCI's parent node when checking for 'dma-ranges' and misses
> > whatever was set there.
> > 
> > To address this I create a new API in OF - inspired from Robin Murphys
> > original proposal[2] - which accepts a bus DT node as it's input in
> > order to configure a device's DMA constraints. The changes go deep into
> > of/address.c's implementation, as a device being having a DT node
> > assumption was pretty strong.
> > 
> > On top of this work, I also cleaned up of_dma_configure() removing its
> > redundant arguments and creating an alternative function for the special
> > cases
> > not applicable to either the above case or the default usage.
> > 
> > IMO the resulting functions are more explicit. They will probably
> > surface some hacky usages that can be properly fixed as I show with the
> > DT fixes on the Layerscape platform.
> > 
> > This was also tested on a Raspberry Pi 4 with a custom PCIe driver and
> > on a Seattle AMD board.
> 
> Humm, I've been working on this issue too. Looks similar though yours
> has a lot more churn and there's some other bugs I've found.

That's good news, and yes now that I see it, some stuff on my series is overly
complicated. Specially around of_translate_*().

On top of that, you removed in of_dma_get_range():

-	/*
-	 * At least empty ranges has to be defined for parent node if
-	 * DMA is supported
-	 */
-	if (!ranges)
-		break;

Which I assumed was bound to the standard and makes things easier.

> Can you test out this branch[1]. I don't have any h/w needing this,
> but wrote a unittest and tested with modified QEMU.

I reviewed everything, I did find a minor issue, see the patch attached.

Also I tested your branch both on an RPi4, with a PCI device that depends on
these changes and by comparing the OF debugs logs on a Layerscape board which
uses dma-ranges, dma-coherent and IOMMU. All works as expected.

Will you send this series for v5.5? Please keep me in the loop, I'll review and
test the final version.

Regards,
Nicolas

From 26d51853c25c04c28dbc09061951a93c102dabcd Mon Sep 17 00:00:00 2001
From: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
Date: Wed, 25 Sep 2019 16:26:57 +0200
Subject: [PATCH] of: device: do not bail of_dma_configure() when force_dma is
 set

Some Xen devices call of_dma_configure() without an actual DT node in
order for it to set its 'dma_ops'. That's the original intent of
'force_dma', honor that behaviour.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
---
 drivers/of/device.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index a45261e21144..7bc00f72468f 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -100,8 +100,6 @@ int of_dma_configure(struct device *dev, struct device_node *parent, bool force_
 	np = dev->of_node;
 	if (!np)
 		np = parent;
-	if (!np)
-		return -ENODEV;
 
 	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
 	if (ret < 0) {
-- 
2.23.0

Attachment: signature.asc
Description: This is a digitally signed message part