| Message ID | 56CADCE9.9090305@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 22, 2016 at 02:07:50PM -0800, Bjorn Andersson wrote: > On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote: > > > I'm still puzzled to why the chipidea lives as a child device of the msm > device; but as this is a rather common structure I believe this still > needs to be figured out. > Chipidea is core driver; msm driver is glue layer, it has something platform related. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson <bjorn.andersson@linaro.org> 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 <bjorn.andersson@linaro.org> >> >--- >> > 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 <linux/usb/chipidea.h> >> > #include <linux/usb/of.h> >> > #include <linux/of.h> >> >+#include <linux/of_device.h> >> > #include <linux/phy.h> >> > #include <linux/regulator/consumer.h> >> > #include <linux/usb/ehci_def.h> >> >@@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote: > On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote: >> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson >> <bjorn.andersson@linaro.org> 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. [..] >>>> 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. > > Any further comment on this topic? I added the linux-arm mailing list > which was missing from previous discussion. > I had the chance to go through this with Arnd and the verdict is that devices not described in DT should not do DMA (or allocate buffers for doing DMA). So I believe the solution is to fall back on Peter's description; the chipidea driver is the core driver and the Qualcomm code should just be a platform layer. My suggestion is that we turn the chipidea core into a set of APIs that can called by the platform specific pieces. That way we will have the chipidea core be the device described in the DT. I'll try to find some time to prototype this after Connect. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 05:16:50PM -0600, Li Yang wrote: > On Tue, Mar 8, 2016 at 9:40 PM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote: > >> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote: > >>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson > >>> <bjorn.andersson@linaro.org> 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. > > [..] > >>>>> 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. > >> > >> Any further comment on this topic? I added the linux-arm mailing list > >> which was missing from previous discussion. > >> > > > > I had the chance to go through this with Arnd and the verdict is that > > devices not described in DT should not do DMA (or allocate buffers for > > doing DMA). > > > > So I believe the solution is to fall back on Peter's description; the > > chipidea driver is the core driver and the Qualcomm code should just > > be a platform layer. > > > > My suggestion is that we turn the chipidea core into a set of APIs > > that can called by the platform specific pieces. That way we will have > > the chipidea core be the device described in the DT. > > But like I said, this problem is not just existing for chipidea > driver. We already found that the dwc3 driver is also suffering from > the same issue. I don't know how many other drivers are impacted by > this change, but I suspect there will be some. A grep of > platform_device_add() in driver/ directory returns many possible > drivers to be impacted. As far as I know, the > drivers/net/ethernet/freescale/fman/mac.c is registering child > ethernet devices that definitely will do dma. If you want to do this > kind of rework to all these drivers, it will be a really big effort. > +1 Yes, I think this DMA things should be covered by driver core too. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 07:40:08PM -0800, Bjorn Andersson wrote: > On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote: > > On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote: > >> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson > >> <bjorn.andersson@linaro.org> 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. > [..] > >>>> 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 had the chance to go through this with Arnd and the verdict is that > devices not described in DT should not do DMA (or allocate buffers for > doing DMA). > > So I believe the solution is to fall back on Peter's description; the > chipidea driver is the core driver and the Qualcomm code should just > be a platform layer. > > My suggestion is that we turn the chipidea core into a set of APIs > that can called by the platform specific pieces. That way we will have > the chipidea core be the device described in the DT. > Hi Bjorn, After reading the DMA documentation Russell supplied and several related DMA APIs, would you please try below patch on your ARM64 platform? Since the core device has no device node at all, I don't know why your patch can work, or am I missing something? From bcf7eaf694d29fb7557a9406fb6c89213216069c Mon Sep 17 00:00:00 2001 From: Peter Chen <peter.chen@freescale.com> Date: Fri, 25 Mar 2016 11:54:21 +0800 Subject: [PATCH 1/1] usb: chipidea: add DMA mask configuration API Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/ci_hdrc_msm.c | 6 ++++++ drivers/usb/chipidea/core.c | 25 +++++++++++++++++++++++++ include/linux/usb/chipidea.h | 2 ++ 3 files changed, 33 insertions(+) -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.htmldiff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c index 3889809..43ceb38 100644 --- a/drivers/usb/chipidea/ci_hdrc_msm.c +++ b/drivers/usb/chipidea/ci_hdrc_msm.c @@ -5,6 +5,7 @@ * only version 2 as published by the Free Software Foundation. */ +#include <linux/dma-mapping.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> @@ -56,6 +57,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) { struct platform_device *plat_ci; struct usb_phy *phy; + int ret; dev_dbg(&pdev->dev, "ci_hdrc_msm_probe\n"); @@ -70,6 +72,10 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) ci_hdrc_msm_platdata.usb_phy = phy; + ret = ci_hdrc_set_dma_mask(&pdev->dev, DMA_BIT_MASK(64)); + if (ret) + return ret; + plat_ci = ci_hdrc_add_device(&pdev->dev, pdev->resource, pdev->num_resources, &ci_hdrc_msm_platdata); diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 69426e6..b8ca5e3 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -62,6 +62,7 @@ #include <linux/usb/chipidea.h> #include <linux/usb/of.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/phy.h> #include <linux/regulator/consumer.h> #include <linux/usb/ehci_def.h> @@ -811,6 +812,30 @@ static void ci_extcon_unregister(struct ci_hdrc *ci) static DEFINE_IDA(ci_ida); +/** + * ci_hdrc_set_dma_mask + * + * Set dma mask and coherent dma mask for glue layer device, and the core + * device will inherit these values. If the 'dma-ranges' is specified at + * DT, it will use this value for both dma mask and coherent dma mask. + * + * @dev: a pointer to the device struct of glue layer device + * @ci_coherent_dma_mask: the mask for both dma_mask and cohrent_dma_mask + */ +int ci_hdrc_set_dma_mask(struct device *dev, u64 ci_coherent_dma_mask) +{ + int ret = dma_set_mask_and_coherent(dev, ci_coherent_dma_mask); + if (ret) { + dev_err(dev, "dma_set_mask_and_coherent fails\n"); + return ret; + } + + if (dev_of_node(dev)) + of_dma_configure(dev, dev->of_node); + + return ret; +} + struct platform_device *ci_hdrc_add_device(struct device *dev, struct resource *res, int nres, struct ci_hdrc_platform_data *platdata) diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index 5dd75fa..8649930 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -84,4 +84,6 @@ struct platform_device *ci_hdrc_add_device(struct device *dev, /* Remove ci hdrc device */ void ci_hdrc_remove_device(struct platform_device *pdev); +int ci_hdrc_set_dma_mask(struct device *dev, u64 ci_coherent_dma_mask); + #endif
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 6e53c24..0293ed5 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -1844,13 +1844,13 @@ static int udc_start(struct ci_hdrc *ci) INIT_LIST_HEAD(&ci->gadget.ep_list); /* alloc resources */ - ci->qh_pool = dma_pool_create("ci_hw_qh", dev, + ci->qh_pool = dma_pool_create("ci_hw_qh", dev->parent, sizeof(struct ci_hw_qh), 64, CI_HDRC_PAGE_SIZE); if (ci->qh_pool == NULL) return -ENOMEM; - ci->td_pool = dma_pool_create("ci_hw_td", dev, + ci->td_pool = dma_pool_create("ci_hw_td", dev->parent, sizeof(struct ci_hw_td), 64, CI_HDRC_PAGE_SIZE); if (ci->td_pool == NULL) {