Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755367AbcCBW7j (ORCPT ); Wed, 2 Mar 2016 17:59:39 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:33832 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753879AbcCBW7f (ORCPT ); Wed, 2 Mar 2016 17:59:35 -0500 MIME-Version: 1.0 In-Reply-To: <20160222220750.GI21240@tuxbot> References: <1456119133-16114-1-git-send-email-bjorn.andersson@linaro.org> <56CADCE9.9090305@linaro.org> <20160222220750.GI21240@tuxbot> Date: Wed, 2 Mar 2016 16:59:33 -0600 X-Google-Sender-Auth: v_c0o-RD0Q9JS_QeP2VbIkBwVk0 Message-ID: Subject: Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT From: Li Yang To: Bjorn Andersson Cc: Srinivas Kandagatla , Arnd Bergmann , Peter Chen , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" , linux-arm-msm , lkml , Rajesh Bhagat Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2915 Lines: 87 On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson wrote: > On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote: > >> >> >> On 22/02/16 05:32, Bjorn Andersson wrote: >> >On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set >> >to be able to do DMA allocations, so use the of_dma_configure() helper >> >to populate the dma properties and assign an appropriate dma_ops. We also hit the same issue with the dwc3 driver. >> > >> >Signed-off-by: Bjorn Andersson >> >--- >> > drivers/usb/chipidea/core.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> >diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c >> >index 7404064b9bbc..047b9d4e67aa 100644 >> >--- a/drivers/usb/chipidea/core.c >> >+++ b/drivers/usb/chipidea/core.c >> >@@ -62,6 +62,7 @@ >> > #include >> > #include >> > #include >> >+#include >> > #include >> > #include >> > #include >> >@@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct device *dev, >> > pdev->dev.dma_parms = dev->dma_parms; >> > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >> > >> >+ if (IS_ENABLED(CONFIG_OF) && dev->of_node) >> >+ of_dma_configure(&pdev->dev, dev->of_node); >> >+ >> Would we hit the same issue if we are on non Device tree platforms like ACPI >> or platform device style itself? >> > > As far as I can see, yes. > >> >> > ret = platform_device_add_resources(pdev, res, nres); >> > if (ret) >> > goto err; >> > >> >> I think this is the side effect of commit >> 1dccb598df549d892b6450c261da54cdd7af44b4(arm64: simplify dma_get_ops) >> > > I agree, before that we would have hit: > > __generic_dma_ops() { > .. > else if (acpi_disabled) > return dma_ops; > ... > } > > with dma_ops being swiotlb_dma_ops from arm64_dma_init(). > > > But this would not have saved us in the ACPI case, i.e. the result would > have been as with my suggested patch. Poking Arnd here to see if he has > any input. > >> None of the drivers call of_dma_configure() explicitly, which makes me feel >> that we are doing something wrong. TBH, this should be handled in more >> generic way rather than driver like this having an explicit call to >> of_dma_configure(). >> > > I agree, trying to figure out if it should be inherited or something. I also agree. We need address it in a more generic way. I did a search for platform_device_add()/platform_device_register() in the kernel source code. I found a lot of them and many could be also doing DMA. Looks like it is still too early to assume every device is already getting dma_ops set through bus probe. Otherwise, many drivers are potentially broken by this assumption. Regards, Leo