| Message ID | 1456119133-16114-1-git-send-email-bjorn.andersson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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. > > 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? > 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) 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(). On the other hand, I think we could also solve the issue by using correct device(parent device) while allocating dma/dma-pool. ------------------------>cut<----------------------------- ------------------------>cut<----------------------------- --srini -- 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/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) {
On Monday 14 March 2016 18:51:08 Peter Chen wrote: > 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: > > > > > > 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. > I don't think it's a very widespread problem, there are only very few developers that intentionally use this method, and some use the platform_device_register_full() call to create a device with a known mask, which is generally ok for the limited case where the driver is only ever going to run on a single platform, but not in the more general case that of_dma_configure is designed to handle. I think we should fix the drivers to consistently use the device that was created by the platform (DT or ACPI or board file) to pass that into the DMA API, anything else will just cause more subtle bugs. Arnd -- 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 Thu, Mar 17, 2016 at 04:52:55PM +0100, Arnd Bergmann wrote: > On Monday 14 March 2016 18:51:08 Peter Chen wrote: > > 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: > > > > > > > > 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. > > > > I don't think it's a very widespread problem, there are only very few > developers that intentionally use this method, and some use the > platform_device_register_full() call to create a device with a known > mask, which is generally ok for the limited case where the driver > is only ever going to run on a single platform, but not in the > more general case that of_dma_configure is designed to handle. Even only for qualcomm platforms, it may be possible have different DMA masks at ARM64 platforms, so we may can't use a fixed value at glue layer driver. So, using of_dma_configure is suitable choice for DT platforms for this case, right? > > I think we should fix the drivers to consistently use the device > that was created by the platform (DT or ACPI or board file) > to pass that into the DMA API, anything else will just cause > more subtle bugs. > Although I don't know what kinds of bugs it may have, it may be met before, otherwise, why most of platform drivers need to call dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly -- 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 Friday 18 March 2016 09:54:14 Peter Chen wrote: > > > > I don't think it's a very widespread problem, there are only very few > > developers that intentionally use this method, and some use the > > platform_device_register_full() call to create a device with a known > > mask, which is generally ok for the limited case where the driver > > is only ever going to run on a single platform, but not in the > > more general case that of_dma_configure is designed to handle. > > Even only for qualcomm platforms, it may be possible have different > DMA masks at ARM64 platforms, so we may can't use a fixed value > at glue layer driver. So, using of_dma_configure is suitable choice > for DT platforms for this case, right? Yes. > > I think we should fix the drivers to consistently use the device > > that was created by the platform (DT or ACPI or board file) > > to pass that into the DMA API, anything else will just cause > > more subtle bugs. > > > > Although I don't know what kinds of bugs it may have, it may be > met before, otherwise, why most of platform drivers need to call > dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly Any driver that wants to do 64-bit addressing on DMA should call dma_set_mask()/dma_set_coherent_mask() on its device and check the return code. No driver should call dma_coerce_mask_and_coherent() on its own device, it's basically always a bug and we named the function to make that more obvious. The problem with dma_coerce_mask_and_coherent() is that it just overrides whatever the platform knows about the device when the driver thinks it knows better. The reason for having those calls in a lot of drivers is that traditionally, ARM platforms booting with DT did not set up any DMA mask and the drivers worked around it by manually setting up a mask that happened to work for them (almost all 32-bit ARM devices need a 32-bit mask without coherency or offset or iommu, so that's easy). We now call of_dma_configure() for all devices that get probed from DT, so we should be removing those calls. Arnd -- 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 Fri, Mar 18, 2016 at 09:54:14AM +0800, Peter Chen wrote: > Although I don't know what kinds of bugs it may have, it may be > met before, otherwise, why most of platform drivers need to call > dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly See Documentation/DMA-API.txt, specifically the section starting Part Ic - DMA addressing limitations ------------------------------------ and also Documentation/DMA-API-HOWTO.txt, the section on DMA addressing limitations which provides further information. Drivers using DMA should be using dma_set_mask_and_coherent() _or_ one of dma_set_mask() and dma_set_coherent_mask() depending on which types of DMA they wish to perform. Drivers should not use dma_coerce_mask_and_coherent() except in exceptional circumstances: that function is more a marker that they or some bus/platform code is doing something wrong. -- RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- 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
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); + ret = platform_device_add_resources(pdev, res, nres); if (ret) goto err;
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. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/usb/chipidea/core.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.5.0 -- 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