Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thu, Sep 22, 2016 at 12:02 AM, Sriram Dash wrote: >>From: Arnd Bergmann [mailto:a...@arndb.de] >>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote: >>> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September >>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote: >>> >>> == >>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001 >>> From: Sriram Dash >>> Date: Wed, 21 Sep 2016 11:39:30 +0530 >>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration >>> from parent dev >>> >>> Fixes the patch https://patchwork.kernel.org/patch/9319527/ >>> ("usb: dwc3: host: inherit dma configuration from parent dev"). >>> >>> Signed-off-by: Sriram Dash >>> --- >>> drivers/usb/host/xhci-mem.c | 12 ++-- >>> drivers/usb/host/xhci.c | 20 ++-- >>> 2 files changed, 16 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >>> index 6afe323..79608df 100644 >>> --- a/drivers/usb/host/xhci-mem.c >>> +++ b/drivers/usb/host/xhci-mem.c >> >>All the changes you did to this file seem fine, I completely missed that part. >> >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index >>> 01d96c9..9a1ff09 100644 >>> --- a/drivers/usb/host/xhci.c >>> +++ b/drivers/usb/host/xhci.c > > Yes. Some sanity is required over this patch. Hi Sriram, Have you been able to do the sanity check on the patch? I will be good to have the formal patch submitted for integration as soon as possible because the dwc3 USB functionality has been broken for a while in upstream kernel. Regards, Leo
RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
>From: Arnd Bergmann [mailto:a...@arndb.de] >On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote: >> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September >> >21, 2016 11:06:47 AM CEST Sriram Dash wrote: >> >> == >> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001 >> From: Sriram Dash >> Date: Wed, 21 Sep 2016 11:39:30 +0530 >> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration >> from parent dev >> >> Fixes the patch https://patchwork.kernel.org/patch/9319527/ >> ("usb: dwc3: host: inherit dma configuration from parent dev"). >> >> Signed-off-by: Sriram Dash >> --- >> drivers/usb/host/xhci-mem.c | 12 ++-- >> drivers/usb/host/xhci.c | 20 ++-- >> 2 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >> index 6afe323..79608df 100644 >> --- a/drivers/usb/host/xhci-mem.c >> +++ b/drivers/usb/host/xhci-mem.c > >All the changes you did to this file seem fine, I completely missed that part. > >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index >> 01d96c9..9a1ff09 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c Yes. Some sanity is required over this patch. >> @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci) >> static int xhci_setup_msi(struct xhci_hcd *xhci) { >> int ret; >> -struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); >> +struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev); >> >> ret = pci_enable_msi(pdev); >> if (ret) { > >This one is interesting as I stumbled over some code comment mentioning that >for >dwc3-pci, we don't support MSI. I think with this change, we /can/ actually >support >MSI, but this could be a separate patch and we'd have to test it on dwc3-pci >hardware. Same for most of this file. > >> @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd) >> struct xhci_hcd *xhci = hcd_to_xhci(hcd); >> >> if (xhci->quirks & XHCI_SPURIOUS_REBOOT) >> -usb_disable_xhci_ports(to_pci_dev(hcd->self.controller)); >> +usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); >> >> spin_lock_irq(&xhci->lock); >> xhci_halt(xhci); > >This seems obviously correct, but I don't yet see why it currently works. We >probably don't call this function on dwc3. > >> #ifdef CONFIG_PM >> @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct >usb_device *udev) >> * if no devices remain. >> */ >> if (xhci->quirks & XHCI_RESET_ON_RESUME) >> -pm_runtime_put_noidle(hcd->self.controller); >> +pm_runtime_put_noidle(hcd->self.sysdev); >> #endif >> >> ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__); > >I suspect this one is wrong, based on what Felipe explained earlier: >the power management should propagate down from the child to the parent >device. > >Someone who understands this better than I do should look at it. > >> @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct >usb_device *udev) >> * suspend if there is a device attached. >> */ >> if (xhci->quirks & XHCI_RESET_ON_RESUME) >> -pm_runtime_get_noresume(hcd->self.controller); >> +pm_runtime_get_noresume(hcd->self.sysdev); >> #endif >> >> > >Same here. > >> @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd) int >> xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) { >> struct xhci_hcd *xhci; >> -struct device *dev = hcd->self.controller; >> +struct device *dev = hcd->self.sysdev; >> int retval; > > >This one calls > >get_quirks(dev, xhci); > >not sure whether this should be called with self.controller or self.sysdev, we >should >audit every one of the callers here to be sure: > >drivers/usb/host/xhci-mtk.c:return xhci_gen_setup(hcd, xhci_mtk_quirks); >drivers/usb/host/xhci-pci.c:retval = xhci_gen_setup(hcd, xhci_pci_quirks); >drivers/usb/host/xhci-plat.c: return xhci_gen_setup(hcd, xhci_plat_quirks); >drivers/usb/host/xhci-rcar.c:* xhci_gen_setup(). >drivers/usb/host/xhci-tegra.c: return xhci_gen_setup(hcd, tegra_xhci_quirks); > > Arnd
RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
>From: Arnd Bergmann [mailto:a...@arndb.de] >On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote: >> >> Hello Arnd, >> >> We tried this patch on NXP platforms (ls2085 and ls1043) which use >> dwc3 controller without any glue layer. On first go, this did not >> work. But after minimal reworks mention snippet below, we are able to >> verify that the USB was working OK. >> >> drivers/usb/host/xhci-mem.c | 12 ++-- >> drivers/usb/host/xhci.c | 20 ++-- >> >> - struct device *dev = xhci_to_hcd(xhci)->self.controller; >> + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; >> >> We believe the patch needs little modification to work or there might >> be chances we may have missed something. Any idea? > > >I had not tried the patch, it was just sent for clarification what I meant, so >I'm glad >you got it working with just minimal changes. > >Unfortunately, I can't tell from your lines above what exactly you changed, >can you >send that again as a proper patch? > Sure. == >From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001 From: Sriram Dash Date: Wed, 21 Sep 2016 11:39:30 +0530 Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from parent dev Fixes the patch https://patchwork.kernel.org/patch/9319527/ ("usb: dwc3: host: inherit dma configuration from parent dev"). Signed-off-by: Sriram Dash --- drivers/usb/host/xhci-mem.c | 12 ++-- drivers/usb/host/xhci.c | 20 ++-- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 6afe323..79608df 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -586,7 +586,7 @@ static void xhci_free_stream_ctx(struct xhci_hcd *xhci, unsigned int num_stream_ctxs, struct xhci_stream_ctx *stream_ctx, dma_addr_t dma) { - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs; if (size > MEDIUM_STREAM_ARRAY_SIZE) @@ -614,7 +614,7 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci, unsigned int num_stream_ctxs, dma_addr_t *dma, gfp_t mem_flags) { - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs; if (size > MEDIUM_STREAM_ARRAY_SIZE) @@ -1644,7 +1644,7 @@ void xhci_slot_copy(struct xhci_hcd *xhci, static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags) { int i; - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; int num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2); xhci_dbg_trace(xhci, trace_xhci_dbg_init, @@ -1716,7 +1716,7 @@ static void scratchpad_free(struct xhci_hcd *xhci) { int num_sp; int i; - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; if (!xhci->scratchpad) return; @@ -1792,7 +1792,7 @@ void xhci_free_command(struct xhci_hcd *xhci, void xhci_mem_cleanup(struct xhci_hcd *xhci) { - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; int size; int i, j, num_ports; @@ -2334,7 +2334,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) { dma_addr_t dma; - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; unsigned intval, val2; u64 val_64; struct xhci_segment *seg; diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 01d96c9..9a1ff09 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci) static int xhci_setup_msi(struct xhci_hcd *xhci) { int ret; - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev); ret = pci_enable_msi(pdev); if (ret) { @@ -257,7 +257,7 @@ static int xhci_setup_msi(struct xhci_hcd *xhci) */ static void xhci_free_irq(struct xhci_hcd *xhci) { - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev); int ret; /* return if using legacy interrupt */ @@ -280,7 +280,7 @@ static int xhci_setup_msix(struct xhci_hcd *xhci) {
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote: > >From: Arnd Bergmann [mailto:a...@arndb.de] > >On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote: > > == > From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001 > From: Sriram Dash > Date: Wed, 21 Sep 2016 11:39:30 +0530 > Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from > parent dev > > Fixes the patch https://patchwork.kernel.org/patch/9319527/ > ("usb: dwc3: host: inherit dma configuration from parent dev"). > > Signed-off-by: Sriram Dash > --- > drivers/usb/host/xhci-mem.c | 12 ++-- > drivers/usb/host/xhci.c | 20 ++-- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 6afe323..79608df 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c All the changes you did to this file seem fine, I completely missed that part. > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 01d96c9..9a1ff09 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci) > static int xhci_setup_msi(struct xhci_hcd *xhci) > { > int ret; > - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); > + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev); > > ret = pci_enable_msi(pdev); > if (ret) { This one is interesting as I stumbled over some code comment mentioning that for dwc3-pci, we don't support MSI. I think with this change, we /can/ actually support MSI, but this could be a separate patch and we'd have to test it on dwc3-pci hardware. Same for most of this file. > @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd) > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > if (xhci->quirks & XHCI_SPURIOUS_REBOOT) > - usb_disable_xhci_ports(to_pci_dev(hcd->self.controller)); > + usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); > > spin_lock_irq(&xhci->lock); > xhci_halt(xhci); This seems obviously correct, but I don't yet see why it currently works. We probably don't call this function on dwc3. > #ifdef CONFIG_PM > @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct > usb_device *udev) >* if no devices remain. >*/ > if (xhci->quirks & XHCI_RESET_ON_RESUME) > - pm_runtime_put_noidle(hcd->self.controller); > + pm_runtime_put_noidle(hcd->self.sysdev); > #endif > > ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__); I suspect this one is wrong, based on what Felipe explained earlier: the power management should propagate down from the child to the parent device. Someone who understands this better than I do should look at it. > @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct > usb_device *udev) >* suspend if there is a device attached. >*/ > if (xhci->quirks & XHCI_RESET_ON_RESUME) > - pm_runtime_get_noresume(hcd->self.controller); > + pm_runtime_get_noresume(hcd->self.sysdev); > #endif > > Same here. > @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd) > int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) > { > struct xhci_hcd *xhci; > - struct device *dev = hcd->self.controller; > + struct device *dev = hcd->self.sysdev; > int retval; This one calls get_quirks(dev, xhci); not sure whether this should be called with self.controller or self.sysdev, we should audit every one of the callers here to be sure: drivers/usb/host/xhci-mtk.c:return xhci_gen_setup(hcd, xhci_mtk_quirks); drivers/usb/host/xhci-pci.c:retval = xhci_gen_setup(hcd, xhci_pci_quirks); drivers/usb/host/xhci-plat.c: return xhci_gen_setup(hcd, xhci_plat_quirks); drivers/usb/host/xhci-rcar.c:* xhci_gen_setup(). drivers/usb/host/xhci-tegra.c: return xhci_gen_setup(hcd, tegra_xhci_quirks); Arnd
RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
>From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] >On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote: >> >> Hi, >> >> Arnd Bergmann writes: >> >> [...] >> >> > Regarding the DMA configuration that you mention in >> > ci_hdrc_add_device(), I think we should replace >> > >> > pdev->dev.dma_mask = dev->dma_mask; >> > pdev->dev.dma_parms = dev->dma_parms; >> > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >> > >> > with of_dma_configure(), which has the chance to configure more than >> > just those three, as the dma API might look into different aspects: >> > >> > - iommu specific configuration >> > - cache coherency information >> > - bus type >> > - dma offset >> > - dma_map_ops pointer >> > >> > We try to handle everything in of_dma_configure() at configuration >> > time, and that would be the place to add anything else that we might >> > need in the future. >> >> There are a couple problems with this: >> >> 1) won't work for PCI-based systems. >> >> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX >> platform (FPGA that appears like a PCI card to host PC) > >Right, I was specifically talking about the code in chipidea here, which I >think is >never used on the PCI bus, and how the current code is broken. We can probably >do >better than of_dma_configure() (see below), but it would be an improvement. > >> 2) not very robust solution >> >> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat >> because that's not created by DT. The only reason why this works at >> all is because of the default 32-bit dma mask thing :-) So, how is it >> any different than copying 32-bit dma mask from parent? > >The idea here is that you pass in the parent of_node along with the child >device >pointer, so it would behave exactly like the parent already does. >The difference is that it also handles all the other attributes besides the >mask. > >However, to summarize the discussion so far, I agree that >of_dma_configure() is not the solution to these problems, and I think we can do >much better: > >Splitting the usb_bus->controller field into the Linux-internal device (used >for the >sysfs hierarchy, for printks and for power management) and a new pointer (used >for >DMA, DT enumeration and phy lookup) probably covers all that we really need. > >I've prototyped it below, with the dwc3, xhci and chipidea changes together >with >the core changes. I've surely made mistakes there and don't expect it to work >out >of the box, but this should give an idea of how I think this can all be solved >in the >least invasive way. > Hello Arnd, We tried this patch on NXP platforms (ls2085 and ls1043) which use dwc3 controller without any glue layer. On first go, this did not work. But after minimal reworks mention snippet below, we are able to verify that the USB was working OK. drivers/usb/host/xhci-mem.c | 12 ++-- drivers/usb/host/xhci.c | 20 ++-- - struct device *dev = xhci_to_hcd(xhci)->self.controller; + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; We believe the patch needs little modification to work or there might be chances we may have missed something. Any idea? Regards, Sriram >I noticed that the gadget interface already has a way to handle the DMA >allocation >by device, so I added that in as well. > >Signed-off-by: Arnd Bergmann > > drivers/usb/chipidea/core.c| 4 > drivers/usb/chipidea/host.c| 3 ++- > drivers/usb/chipidea/udc.c | 8 > drivers/usb/core/buffer.c | 12 ++-- > drivers/usb/core/hcd.c | 48 > +-- >- > drivers/usb/core/usb.c | 16 > drivers/usb/dwc3/core.c| 28 +++- > drivers/usb/dwc3/core.h| 1 + > drivers/usb/dwc3/dwc3-exynos.c | 10 -- > drivers/usb/dwc3/dwc3-st.c | 1 - > drivers/usb/dwc3/ep0.c | 8 > drivers/usb/dwc3/gadget.c | 34 +- > drivers/usb/dwc3/host.c| 13 - > drivers/usb/host/ehci-fsl.c| 4 ++-- > drivers/usb/host/xhci-plat.c | 32 +--- > include/linux/usb.h| 1 + > include/linux/usb/hcd.h| 3 +++ > >diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index >69426e644d17..dff69837b349 100644 >--- a/drivers/usb/chipidea/core.c >+++ b/drivers/usb/chipidea/core.c >@@ -833,10 +833,6 @@ struct platform_device *ci_hdrc_add_device(struct device >*dev, > } > > pdev->dev.parent = dev; >- pdev->dev.dma_mask = dev->dma_mask; >- pdev->dev.dma_parms = dev->dma_parms; >- dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >- > ret = platform_device_add_resources(pdev, res, nres); > if (ret) > goto err; >diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c ind
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote: > > Hello Arnd, > > We tried this patch on NXP platforms (ls2085 and ls1043) which use dwc3 > controller without any glue layer. On first go, this did not work. But after > minimal reworks mention snippet below, we are able to verify that the USB > was working OK. > > drivers/usb/host/xhci-mem.c | 12 ++-- > drivers/usb/host/xhci.c | 20 ++-- > > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > > We believe the patch needs little modification to work or there might be > chances > we may have missed something. Any idea? I had not tried the patch, it was just sent for clarification what I meant, so I'm glad you got it working with just minimal changes. Unfortunately, I can't tell from your lines above what exactly you changed, can you send that again as a proper patch? I think I also had some minimal changes that I did myself in order to fix a build regression I introduced. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 14, 2016 5:31:36 PM CEST Lorenzo Pieralisi wrote: > On Wed, Sep 07, 2016 at 01:47:22PM +0300, Felipe Balbi wrote: > > > > Hi, > > > > Robin Murphy writes: > > > On 07/09/16 10:55, Peter Chen wrote: > > > [...] > > >>> Regarding the DMA configuration that you mention in > > >>> ci_hdrc_add_device(), > > >>> I think we should replace > > >>> > > >>> pdev->dev.dma_mask = dev->dma_mask; > > >>> pdev->dev.dma_parms = dev->dma_parms; > > >>> dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > >>> > > >>> with of_dma_configure(), which has the chance to configure more than > > >>> just those three, as the dma API might look into different aspects: > > >>> > > >>> - iommu specific configuration > > >>> - cache coherency information > > >>> - bus type > > >>> - dma offset > > >>> - dma_map_ops pointer > > >>> > > >>> We try to handle everything in of_dma_configure() at configuration > > >>> time, and that would be the place to add anything else that we might > > >>> need in the future. > > >>> > > >> > > >> Yes, I agree with you, but just like Felipe mentioned, we also need to > > >> consider PCI device, can we do something like gpiod_get_index does? Are > > >> there any similar APIs like of_dma_configure for ACPI? > > > > > > Not yet, but Lorenzo has one in progress[1], primarily for the sake of > > > abstracting away the IOMMU configuration. > > > > > > Robin. > > > > > > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html > > > > not exported for drivers to use. If Lorenzo is trying to making a > > matching API for ACPI systems, then it needs to follow what > > of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL() > > That's easy enough, not sure I understand though why > of_dma_deconfigure() is not exported then. The second question mark > is about the dma-ranges equivalent in ACPI world; the _DMA method > seems to be the exact equivalent but to the best of my knowledge > it is ignored by the kernel, to really have an of_dma_configure() > equivalent that's really necessary, unless we want to resort to > arch specific methods (is that what x86 is currently doing ?) to > retrieve/build the dma masks. Please see the follow-up emails after my proposed patch: if we add a pointer to the device that firmware knows about in the USB core layer, there is no longer a problem to be solved and the DMA operations will do the right thing. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Sep 07, 2016 at 01:47:22PM +0300, Felipe Balbi wrote: > > Hi, > > Robin Murphy writes: > > On 07/09/16 10:55, Peter Chen wrote: > > [...] > >>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(), > >>> I think we should replace > >>> > >>> pdev->dev.dma_mask = dev->dma_mask; > >>> pdev->dev.dma_parms = dev->dma_parms; > >>> dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > >>> > >>> with of_dma_configure(), which has the chance to configure more than > >>> just those three, as the dma API might look into different aspects: > >>> > >>> - iommu specific configuration > >>> - cache coherency information > >>> - bus type > >>> - dma offset > >>> - dma_map_ops pointer > >>> > >>> We try to handle everything in of_dma_configure() at configuration > >>> time, and that would be the place to add anything else that we might > >>> need in the future. > >>> > >> > >> Yes, I agree with you, but just like Felipe mentioned, we also need to > >> consider PCI device, can we do something like gpiod_get_index does? Are > >> there any similar APIs like of_dma_configure for ACPI? > > > > Not yet, but Lorenzo has one in progress[1], primarily for the sake of > > abstracting away the IOMMU configuration. > > > > Robin. > > > > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html > > not exported for drivers to use. If Lorenzo is trying to making a > matching API for ACPI systems, then it needs to follow what > of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL() That's easy enough, not sure I understand though why of_dma_deconfigure() is not exported then. The second question mark is about the dma-ranges equivalent in ACPI world; the _DMA method seems to be the exact equivalent but to the best of my knowledge it is ignored by the kernel, to really have an of_dma_configure() equivalent that's really necessary, unless we want to resort to arch specific methods (is that what x86 is currently doing ?) to retrieve/build the dma masks. Lorenzo
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thu, Sep 08, 2016 at 03:59:19PM +0300, Grygorii Strashko wrote: > On 09/08/2016 03:28 PM, Peter Chen wrote: > > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote: > >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > >>> Arnd Bergmann writes: > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: > >> If we do that, we have to put child devices of the dwc3 devices into > >> the platform glue, and it also breaks those dwc3 devices that don't > >> have a parent driver. > > > > Well, this is easy to fix: > > > > if (dwc->dev->parent) { > > dwc->sysdev = dwc->dev->parent; > > } else { > > dev_info(dwc->dev, "Please provide a glue layer!\n"); > > dwc->sysdev = dwc->dev; > > } > > I don't understand. Do you mean we should have an extra level of > stacking and splitting "static struct platform_driver dwc3_driver" > in two so instead of > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) > > we do this? > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> > "xhci" (usb_bus.dev) > >>> > >>> no > >>> > >>> If we have a parent device, use that as sysdev, otherwise use self as > >>> sysdev. > >> > >> But there is often a parent device in DT, as the xhci device is > >> attached to some internal bus that gets turned into a platform_device > >> as well, so checking whether there is a parent will get the wrong > >> device node. > > > > From my point, all platform and firmware information at dwc3 are > > correct, so we don't need to change dwc3/core.c, only changing for > > xhci-plat.c is ok. > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > index ed56bf9..fd57c0d 100644 > > --- a/drivers/usb/host/xhci-plat.c > > +++ b/drivers/usb/host/xhci-plat.c > > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev) > > struct clk *clk; > > int ret; > > int irq; > > + struct device *dev = &pdev->dev, *sysdev; > > > > if (usb_disabled()) > > return -ENODEV; > > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device > > *pdev) > > if (irq < 0) > > return -ENODEV; > > > > + if (dev->parent) { > > + sysdev = dev->parent; > > + } else { > > + sysdev = dev; > > + } > > + > > Shouldn't we be more careful with that? > Above code does not consider pci device case, Arnd's patch covers all cases. > armada-375.dtsi > > soc { > compatible = "marvell,armada375-mbus", "simple-bus"; > > internal-regs { > compatible = "simple-bus"; > > usb3@58000 { > compatible = "marvell,armada-375-xhci"; > reg = <0x58000 0x2>,<0x5b880 0x80>; > interrupts = ; > clocks = <&gateclk 16>; > phys = <&usbcluster PHY_TYPE_USB3>; > phy-names = "usb"; > status = "disabled"; > }; > > > What will be the parent dev in above case? > In this case, no parent dev for above case, it will use itself as sysdev since it has of_node at dts. -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thu, Sep 08, 2016 at 02:52:29PM +0200, Arnd Bergmann wrote: > On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote: > > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote: > > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > > > > Arnd Bergmann writes: > > > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: > > > > If we have a parent device, use that as sysdev, otherwise use self as > > > > sysdev. > > > > > > But there is often a parent device in DT, as the xhci device is > > > attached to some internal bus that gets turned into a platform_device > > > as well, so checking whether there is a parent will get the wrong > > > device node. > > > > From my point, all platform and firmware information at dwc3 are > > correct, so we don't need to change dwc3/core.c, only changing for > > xhci-plat.c is ok. > > Ok, thanks. That leaves the PCI glue, right? If pci's firmware information can only get from dwc3-pci, I was wrong. I am almost sure your patch covers all 3 cases. dwc3->sysdev covers dwc3 core and gadget side, hcd->self.sysdev cover host side. The only possible improvement may be how to detect pci device. > > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > > index d2e3f65..563600b 100644 > > --- a/drivers/usb/core/hcd.c > > +++ b/drivers/usb/core/hcd.c > > @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd) > > /* Did the HC die before the root hub was registered? */ > > if (HCD_DEAD(hcd)) > > usb_hc_died (hcd); /* This time clean up */ > > - usb_dev->dev.of_node = parent_dev->of_node; > > + usb_dev->dev.of_node = parent_dev->sysdev->of_node; > > } > > mutex_unlock(&usb_bus_idr_lock); > > > > At above changes, the root hub's of_node equals to xhci-hcd sysdev's > > of_node, which is from firmware or from its parent (it is dwc3 core > > device). > > Just to make sure I understand you right: > > in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the > dwc3 device, not the qcom,dwc3 device. > Yes, since there is a DT node for dwc3, and firmware information is there, that's why the original patch (Grygorii Strashko's) can work. -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 09/08/2016 03:28 PM, Peter Chen wrote: > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote: >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >>> Arnd Bergmann writes: On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: >> If we do that, we have to put child devices of the dwc3 devices into >> the platform glue, and it also breaks those dwc3 devices that don't >> have a parent driver. > > Well, this is easy to fix: > > if (dwc->dev->parent) { > dwc->sysdev = dwc->dev->parent; > } else { > dev_info(dwc->dev, "Please provide a glue layer!\n"); > dwc->sysdev = dwc->dev; > } I don't understand. Do you mean we should have an extra level of stacking and splitting "static struct platform_driver dwc3_driver" in two so instead of "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) we do this? "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" (usb_bus.dev) >>> >>> no >>> >>> If we have a parent device, use that as sysdev, otherwise use self as >>> sysdev. >> >> But there is often a parent device in DT, as the xhci device is >> attached to some internal bus that gets turned into a platform_device >> as well, so checking whether there is a parent will get the wrong >> device node. > > From my point, all platform and firmware information at dwc3 are > correct, so we don't need to change dwc3/core.c, only changing for > xhci-plat.c is ok. > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index ed56bf9..fd57c0d 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev) > struct clk *clk; > int ret; > int irq; > + struct device *dev = &pdev->dev, *sysdev; > > if (usb_disabled()) > return -ENODEV; > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev) > if (irq < 0) > return -ENODEV; > > + if (dev->parent) { > + sysdev = dev->parent; > + } else { > + sysdev = dev; > + } > + Shouldn't we be more careful with that? armada-375.dtsi soc { compatible = "marvell,armada375-mbus", "simple-bus"; internal-regs { compatible = "simple-bus"; usb3@58000 { compatible = "marvell,armada-375-xhci"; reg = <0x58000 0x2>,<0x5b880 0x80>; interrupts = ; clocks = <&gateclk 16>; phys = <&usbcluster PHY_TYPE_USB3>; phy-names = "usb"; status = "disabled"; }; What will be the parent dev in above case? -- regards, -grygorii
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote: > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote: > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > > > Arnd Bergmann writes: > > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: > > > If we have a parent device, use that as sysdev, otherwise use self as > > > sysdev. > > > > But there is often a parent device in DT, as the xhci device is > > attached to some internal bus that gets turned into a platform_device > > as well, so checking whether there is a parent will get the wrong > > device node. > > From my point, all platform and firmware information at dwc3 are > correct, so we don't need to change dwc3/core.c, only changing for > xhci-plat.c is ok. Ok, thanks. That leaves the PCI glue, right? > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index d2e3f65..563600b 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd) > /* Did the HC die before the root hub was registered? */ > if (HCD_DEAD(hcd)) > usb_hc_died (hcd); /* This time clean up */ > - usb_dev->dev.of_node = parent_dev->of_node; > + usb_dev->dev.of_node = parent_dev->sysdev->of_node; > } > mutex_unlock(&usb_bus_idr_lock); > > At above changes, the root hub's of_node equals to xhci-hcd sysdev's > of_node, which is from firmware or from its parent (it is dwc3 core > device). Just to make sure I understand you right: in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the dwc3 device, not the qcom,dwc3 device. > > > > That sounds a bit clumsy for the sake of consistency with PCI. > > > > The advantage is that xhci can always use the grandparent device > > > > as sysdev whenever it isn't probed through PCI or firmware > > > > itself, but the purpose of the dwc3-glue is otherwise questionable. > > > > > > > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > > > > device when that is created from the PCI driver and checking for that > > > > with the device property interface instead? If it's "snps,dwc3" > > > > we use the device itself while for "snps,dwc3-pci", we use the parent? > > > > > For pci glue device, it is always the parent for dwc3 core device. > In your patch, you may not need to split pci or non-pci, just using > if (dev->parent). Here we have the pci-dwc3 -> dwc3 -> xhci hierarchy, and we want sysdev to point to pci-dwc3, not dwc3! The point is that the pci_dev is where we have the dma settings and (optionally) additional DT or ACPI data for that device. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > >> If we make dwc3.ko a library which glue calls directly then all these > >> problems are solved but we break all current DTs and fall into the trap > >> of having another MUSB. > > > > I don't see how we'd break the current DTs, I'm fairly sure we could turn > > dwc3 > > well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to > look at possible children for their own quirks and properties. > > > into a library without changing the DT representation. However the parts > > that I think would change are > > > > - The sysfs representation for dwc3-pci, as we would no longer have > > a parent-child relationship there. > > that's a no-brainer, I think > > > - The power management handling might need a rework, since you currently > > rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning > > power on and off > > simple enough to do as well. > > > - turning dwc3 into a library probably implies also turning xhci into > > a library, in part for consistency. > > yeah, I considered that too. We could still do it in parts, though. > > > - if we don't do the whole usb_bus->sysdev thing, we need to not just > > do this for dwc3 but also chipidea and maybe a couple of others. > > MUSB comes to mind Right. > > There should not be any show-stoppers here, but it's a lot of work. > > I think the biggest work will making sure people don't abuse functions > just because they're now part of a single binary. Having them as > separate modules helped a lot reducing the maintenance overhead. There > was only one occasion where someone sent a glue layer which iterated > over its children to find struct dwc3 * from child's drvdata. This is where it get a bit philosophical ;-) I understand that you like the strict separation that the current model provides, and I agree that can be an advantage. Changing the abstraction model to a set of library modules the way that other drivers (e.g. ehci, sdhci, or libata) work to me means changing this separation model into a different model and once we do that I would not consider it a mistake for the platform specific driver to take advantage of that. You still get a bit of separation since the drivers would be in separate modules that can only access exported symbols, and the library can still hide its data structures (to some degree). I still think that turning xhci (and dwc3) into a library would be an overall win, but if we solve the problems of DMA settings and usb_device DT properties without it, I'd prefer not to fight over that with you again ;-) > >> If we try to pass DMA bits from parent to child, then we have the fact > >> that DT ends up, in practice, always having a parent device. > > > > I don't understand what you mean here, but I agree that the various ways > > well, we can't simply use what I pointed out a few emails back: > > if (dwc->dev->parent) > dwc->sysdev = dwc->dev->parent > else > dwc->sysdev = dwc->dev Ok, I see. > > we discussed for copying the DMA flags from one 'struct device' to another > > all turned out to be flawed in at least one way. > > > > Do you see any problems with the patch I posted other than the ugliness > > of the dwc3 and xhci drivers finding out which pointer to use for > > usb_bus->sysdev? If we can solve this, we shouldn't need any new > > of_dma_configure/acpi_dma_configure calls and we won't have to > > turn the drivers into a library, so maybe let's try to come up with > > better ideas for that sub-problem. > > No big problems with that, no. Just the ifdef looking for a PCI bus in > the parent. How about passing a flag via device_properties? I don't > wanna change dwc3 core's device name with a platform_device_id because > there probably already are scripts relying on the names to enable > pm_runtime for example. Sounds ok to me. Grygorii's solution might a be a bit more elegant, but also a bit more error-prone: If we get a platform that mistakenly sets the dma_mask pointer of the child device, or a platform that does not set the dma_mask pointer of the parent, things break. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote: > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > > Arnd Bergmann writes: > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: > > >> > If we do that, we have to put child devices of the dwc3 devices into > > >> > the platform glue, and it also breaks those dwc3 devices that don't > > >> > have a parent driver. > > >> > > >> Well, this is easy to fix: > > >> > > >> if (dwc->dev->parent) { > > >> dwc->sysdev = dwc->dev->parent; > > >> } else { > > >> dev_info(dwc->dev, "Please provide a glue layer!\n"); > > >> dwc->sysdev = dwc->dev; > > >> } > > > > > > I don't understand. Do you mean we should have an extra level of > > > stacking and splitting "static struct platform_driver dwc3_driver" > > > in two so instead of > > > > > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) > > > > > > we do this? > > > > > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> > > > "xhci" (usb_bus.dev) > > > > no > > > > If we have a parent device, use that as sysdev, otherwise use self as > > sysdev. > > But there is often a parent device in DT, as the xhci device is > attached to some internal bus that gets turned into a platform_device > as well, so checking whether there is a parent will get the wrong > device node. >From my point, all platform and firmware information at dwc3 are correct, so we don't need to change dwc3/core.c, only changing for xhci-plat.c is ok. diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index ed56bf9..fd57c0d 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev) struct clk *clk; int ret; int irq; + struct device *dev = &pdev->dev, *sysdev; if (usb_disabled()) return -ENODEV; @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev) if (irq < 0) return -ENODEV; + if (dev->parent) { + sysdev = dev->parent; + } else { + sysdev = dev; + } + /* Try to set 64-bit DMA first */ if (WARN_ON(!pdev->dev.dma_mask)) /* Platform did not initialize dma_mask */ @@ -170,7 +177,8 @@ static int xhci_plat_probe(struct platform_device *pdev) return ret; } - hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev)); + hcd = __usb_create_hcd(driver, sysdev, &pdev->dev, + dev_name(&pdev->dev), NULL); if (!hcd) return -ENOMEM; diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index d2e3f65..563600b 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd) /* Did the HC die before the root hub was registered? */ if (HCD_DEAD(hcd)) usb_hc_died (hcd); /* This time clean up */ - usb_dev->dev.of_node = parent_dev->of_node; + usb_dev->dev.of_node = parent_dev->sysdev->of_node; } mutex_unlock(&usb_bus_idr_lock); At above changes, the root hub's of_node equals to xhci-hcd sysdev's of_node, which is from firmware or from its parent (it is dwc3 core device). > > > > That sounds a bit clumsy for the sake of consistency with PCI. > > > The advantage is that xhci can always use the grandparent device > > > as sysdev whenever it isn't probed through PCI or firmware > > > itself, but the purpose of the dwc3-glue is otherwise questionable. > > > > > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > > > device when that is created from the PCI driver and checking for that > > > with the device property interface instead? If it's "snps,dwc3" > > > we use the device itself while for "snps,dwc3-pci", we use the parent? > > For pci glue device, it is always the parent for dwc3 core device. In your patch, you may not need to split pci or non-pci, just using if (dev->parent). > > Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? > > That would be incompatible with the USB binding, as the sysdev > is assumed to be a USB host controller with #address-cells=<1> > and #size-cells=<0> in order to hold the child devices, for > example: > > / { > omap_dwc3_1: omap_dwc3_1@4888 { > compatible = "ti,dwc3"; > #address-cells = <1>; > #size-cells = <1>; > ranges; > usb1: usb@4889 { > compatible = "snps,dwc3"; > reg = <0x4889 0x17000>; > #address-cells = <1>; > #size-cells = <0>; > interrupts = , >
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 3:02:56 PM CEST Grygorii Strashko wrote: > dwc3: probe() > if (!&pdev->dev->of_node) > legacy case - hard-code DMA props > dwc->sysdev = &pdev->dev; The PCI case will fall into this too, as we almost never have an ->of_node pointer for a PCI device. Do we actually have any legacy dwc3 users in Linux that are neither DT nor PCI based? Maybe we can just skip that. > else > dev = &pdev->dev; > do { > if (is_device_dma_capable(dev)) { > dwc->sysdev = dev; > break; > } >dev = dev->parent; > while (dev); > ^this cycle can be limited in depth (2 for PCI) Right, this could work by itself and looks generic enough. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 09/08/2016 02:00 PM, Felipe Balbi wrote: > > Hi, > > Arnd Bergmann writes: >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >>> Arnd Bergmann writes: On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: >> If we do that, we have to put child devices of the dwc3 devices into >> the platform glue, and it also breaks those dwc3 devices that don't >> have a parent driver. > > Well, this is easy to fix: > > if (dwc->dev->parent) { > dwc->sysdev = dwc->dev->parent; > } else { > dev_info(dwc->dev, "Please provide a glue layer!\n"); > dwc->sysdev = dwc->dev; > } I don't understand. Do you mean we should have an extra level of stacking and splitting "static struct platform_driver dwc3_driver" in two so instead of "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) we do this? "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" (usb_bus.dev) >>> >>> no >>> >>> If we have a parent device, use that as sysdev, otherwise use self as >>> sysdev. >> >> But there is often a parent device in DT, as the xhci device is >> attached to some internal bus that gets turned into a platform_device >> as well, so checking whether there is a parent will get the wrong >> device node. > > oh, that makes things more interesting :-s > That sounds a bit clumsy for the sake of consistency with PCI. The advantage is that xhci can always use the grandparent device as sysdev whenever it isn't probed through PCI or firmware itself, but the purpose of the dwc3-glue is otherwise questionable. How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 device when that is created from the PCI driver and checking for that with the device property interface instead? If it's "snps,dwc3" we use the device itself while for "snps,dwc3-pci", we use the parent? >>> >>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? >> >> That would be incompatible with the USB binding, as the sysdev >> is assumed to be a USB host controller with #address-cells=<1> >> and #size-cells=<0> in order to hold the child devices, for >> example: >> >> / { >> omap_dwc3_1: omap_dwc3_1@4888 { >> compatible = "ti,dwc3"; >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> usb1: usb@4889 { >> compatible = "snps,dwc3"; >> reg = <0x4889 0x17000>; >> #address-cells = <1>; >> #size-cells = <0>; >> interrupts = , >> , >> ; >> interrupt-names = "peripheral", >> "host", >> "otg"; >> phys = <&usb2_phy1>, <&usb3_phy1>; >> phy-names = "usb2-phy", "usb3-phy"; >> >> hub@1 { >> compatible = "usb5e3,608"; >> reg = <1>; >> #address-cells = <1>; >> #size-cells = <0>; >> >> ethernet@1 { >> compatible = "usb424,ec00"; >> mac-address = [00 11 22 33 44 55]; >> reg = <1>; >> }; >> }; >> }; >> }; >> >> It's also the node that contains the "phys" properties and >> presumably other properties like "otg-rev", "maximum-speed" >> etc. >> >> If we make the sysdev point to the parent, then we can no longer >> look up those properties and child devices from the USB core code >> by looking at "sysdev->of_node". > > this also makes things more interesting. I can't of anything other than > having some type of flag passed via e.g. device_properties by dwc3-pci.c > :-s > > It's quite a hack, though. I still think that inheriting DMA (or > manually initializing a child with parent's DMA bits and pieces) is the > best way to go. So we're back to of_dma_configure() and > acpi_dma_configure(), right? > > But this needs to be done before dwc3_probe() executes. For dwc3-pci > that's easy, but for DT devices, seems like it should be in of > core. Below is, clearly, not enough but should show the idea: > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad7c403..a54610198946 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct > device_node *np) > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > * setup the correct supported mask. > */ > - if (!dev->coherent_dma_mask) > - dev->coherent_dma_mask = DMA_BIT_MASK(32); > + if (!dev->
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: >> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci >> >> that's easy, but for DT devices, seems like it should be in of >> >> core. Below is, clearly, not enough but should show the idea: >> >> >> >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> >> index fd5cfad7c403..a54610198946 100644 >> >> --- a/drivers/of/device.c >> >> +++ b/drivers/of/device.c >> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct >> >> device_node *np) >> >> * Set default coherent_dma_mask to 32 bit. Drivers are expected >> >> to >> >> * setup the correct supported mask. >> >> */ >> >> - if (!dev->coherent_dma_mask) >> >> - dev->coherent_dma_mask = DMA_BIT_MASK(32); >> >> + if (!dev->coherent_dma_mask) { >> >> + if (!dev->parent->coherent_dma_mask) >> >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> >> + else >> >> + dev->coherent_dma_mask = >> >> dev->parent->coherent_dma_mask; >> >> + } >> >> >> > >> > As the comment above that code says, the default 32-bit mask is >> > intentional, >> > and you need the driver to ask for the mask it wants using >> > dma_set_mask_and_coherent(), while the platform code should be able to use >> > dev->of_node to figure out whether that mask is supported. >> > >> > Just setting the initial mask to something else based on what the parent >> > supports will not do the right thing elsewhere. >> >> oh man, it gets more and more complex. Seems like either path we take >> will cause problems somewhere >> >> If we make dwc3.ko a library which glue calls directly then all these >> problems are solved but we break all current DTs and fall into the trap >> of having another MUSB. > > I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3 well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to look at possible children for their own quirks and properties. > into a library without changing the DT representation. However the parts > that I think would change are > > - The sysfs representation for dwc3-pci, as we would no longer have > a parent-child relationship there. that's a no-brainer, I think > - The power management handling might need a rework, since you currently > rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning > power on and off simple enough to do as well. > - turning dwc3 into a library probably implies also turning xhci into > a library, in part for consistency. yeah, I considered that too. We could still do it in parts, though. > - if we don't do the whole usb_bus->sysdev thing, we need to not just > do this for dwc3 but also chipidea and maybe a couple of others. MUSB comes to mind > There should not be any show-stoppers here, but it's a lot of work. I think the biggest work will making sure people don't abuse functions just because they're now part of a single binary. Having them as separate modules helped a lot reducing the maintenance overhead. There was only one occasion where someone sent a glue layer which iterated over its children to find struct dwc3 * from child's drvdata. >> If we try to pass DMA bits from parent to child, then we have the fact >> that DT ends up, in practice, always having a parent device. > > I don't understand what you mean here, but I agree that the various ways well, we can't simply use what I pointed out a few emails back: if (dwc->dev->parent) dwc->sysdev = dwc->dev->parent else dwc->sysdev = dwc->dev > we discussed for copying the DMA flags from one 'struct device' to another > all turned out to be flawed in at least one way. > > Do you see any problems with the patch I posted other than the ugliness > of the dwc3 and xhci drivers finding out which pointer to use for > usb_bus->sysdev? If we can solve this, we shouldn't need any new > of_dma_configure/acpi_dma_configure calls and we won't have to > turn the drivers into a library, so maybe let's try to come up with > better ideas for that sub-problem. No big problems with that, no. Just the ifdef looking for a PCI bus in the parent. How about passing a flag via device_properties? I don't wanna change dwc3 core's device name with a platform_device_id because there probably already are scripts relying on the names to enable pm_runtime for example. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 2:20:58 PM CEST Felipe Balbi wrote: > >> It's quite a hack, though. I still think that inheriting DMA (or > >> manually initializing a child with parent's DMA bits and pieces) is the > >> best way to go. So we're back to of_dma_configure() and > >> acpi_dma_configure(), right? > > > > That won't solve the problems with the DT properties or the > > dma configuration for PCI devices though. > > acpi_dma_configure() is supposed to pass along DMA bits from PCI to > child devices, no? I don't know, haven't looked at that code. > >> But this needs to be done before dwc3_probe() executes. For dwc3-pci > >> that's easy, but for DT devices, seems like it should be in of > >> core. Below is, clearly, not enough but should show the idea: > >> > >> diff --git a/drivers/of/device.c b/drivers/of/device.c > >> index fd5cfad7c403..a54610198946 100644 > >> --- a/drivers/of/device.c > >> +++ b/drivers/of/device.c > >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct > >> device_node *np) > >> * Set default coherent_dma_mask to 32 bit. Drivers are expected > >> to > >> * setup the correct supported mask. > >> */ > >> - if (!dev->coherent_dma_mask) > >> - dev->coherent_dma_mask = DMA_BIT_MASK(32); > >> + if (!dev->coherent_dma_mask) { > >> + if (!dev->parent->coherent_dma_mask) > >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); > >> + else > >> + dev->coherent_dma_mask = > >> dev->parent->coherent_dma_mask; > >> + } > >> > > > > As the comment above that code says, the default 32-bit mask is intentional, > > and you need the driver to ask for the mask it wants using > > dma_set_mask_and_coherent(), while the platform code should be able to use > > dev->of_node to figure out whether that mask is supported. > > > > Just setting the initial mask to something else based on what the parent > > supports will not do the right thing elsewhere. > > oh man, it gets more and more complex. Seems like either path we take > will cause problems somewhere > > If we make dwc3.ko a library which glue calls directly then all these > problems are solved but we break all current DTs and fall into the trap > of having another MUSB. I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3 into a library without changing the DT representation. However the parts that I think would change are - The sysfs representation for dwc3-pci, as we would no longer have a parent-child relationship there. - The power management handling might need a rework, since you currently rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning power on and off - turning dwc3 into a library probably implies also turning xhci into a library, in part for consistency. - if we don't do the whole usb_bus->sysdev thing, we need to not just do this for dwc3 but also chipidea and maybe a couple of others. There should not be any show-stoppers here, but it's a lot of work. > If we try to pass DMA bits from parent to child, then we have the fact > that DT ends up, in practice, always having a parent device. I don't understand what you mean here, but I agree that the various ways we discussed for copying the DMA flags from one 'struct device' to another all turned out to be flawed in at least one way. Do you see any problems with the patch I posted other than the ugliness of the dwc3 and xhci drivers finding out which pointer to use for usb_bus->sysdev? If we can solve this, we shouldn't need any new of_dma_configure/acpi_dma_configure calls and we won't have to turn the drivers into a library, so maybe let's try to come up with better ideas for that sub-problem. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >> >> Arnd Bergmann writes: >> >> > That sounds a bit clumsy for the sake of consistency with PCI. >> >> > The advantage is that xhci can always use the grandparent device >> >> > as sysdev whenever it isn't probed through PCI or firmware >> >> > itself, but the purpose of the dwc3-glue is otherwise questionable. >> >> > >> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 >> >> > device when that is created from the PCI driver and checking for that >> >> > with the device property interface instead? If it's "snps,dwc3" >> >> > we use the device itself while for "snps,dwc3-pci", we use the parent? >> >> >> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? >> > >> > That would be incompatible with the USB binding, as the sysdev >> > is assumed to be a USB host controller with #address-cells=<1> >> > and #size-cells=<0> in order to hold the child devices, for >> > example: >> > >> > / { >> > omap_dwc3_1: omap_dwc3_1@4888 { >> > compatible = "ti,dwc3"; >> > #address-cells = <1>; >> > #size-cells = <1>; >> > ranges; >> > usb1: usb@4889 { >> > compatible = "snps,dwc3"; >> > reg = <0x4889 0x17000>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > interrupts = , >> > , >> > ; >> > interrupt-names = "peripheral", >> > "host", >> > "otg"; >> > phys = <&usb2_phy1>, <&usb3_phy1>; >> > phy-names = "usb2-phy", "usb3-phy"; >> > >> > hub@1 { >> > compatible = "usb5e3,608"; >> > reg = <1>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > >> > ethernet@1 { >> > compatible = "usb424,ec00"; >> > mac-address = [00 11 22 33 44 55]; >> > reg = <1>; >> > }; >> > }; >> > }; >> > }; >> > >> > It's also the node that contains the "phys" properties and >> > presumably other properties like "otg-rev", "maximum-speed" >> > etc. >> > >> > If we make the sysdev point to the parent, then we can no longer >> > look up those properties and child devices from the USB core code >> > by looking at "sysdev->of_node". >> >> this also makes things more interesting. I can't of anything other than >> having some type of flag passed via e.g. device_properties by dwc3-pci.c >> :-s > > Ok. man, I have been skipping words rather frequently when typing lately. I meant "I can't THINK of anything other " >> It's quite a hack, though. I still think that inheriting DMA (or >> manually initializing a child with parent's DMA bits and pieces) is the >> best way to go. So we're back to of_dma_configure() and >> acpi_dma_configure(), right? > > That won't solve the problems with the DT properties or the > dma configuration for PCI devices though. acpi_dma_configure() is supposed to pass along DMA bits from PCI to child devices, no? >> But this needs to be done before dwc3_probe() executes. For dwc3-pci >> that's easy, but for DT devices, seems like it should be in of >> core. Below is, clearly, not enough but should show the idea: >> >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index fd5cfad7c403..a54610198946 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct >> device_node *np) >> * Set default coherent_dma_mask to 32 bit. Drivers are expected to >> * setup the correct supported mask. >> */ >> - if (!dev->coherent_dma_mask) >> - dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + if (!dev->coherent_dma_mask) { >> + if (!dev->parent->coherent_dma_mask) >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + else >> + dev->coherent_dma_mask = >> dev->parent->coherent_dma_mask; >> + } >> > > As the comment above that code says, the default 32-bit mask is intentional, > and you need the driver to ask for the mask it wants using > dma_set_mask_and_coherent(), while the platform code should be able to use > dev->of_node to figure out whether that mask is supported. > > Just setting the initial mask to something else based on what the parent > supports will not do the right thing elsewhere. oh man, it gets more and more complex. Seems like either path we take will cause problems somewhere :-s If we mak
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > >> Arnd Bergmann writes: > >> > That sounds a bit clumsy for the sake of consistency with PCI. > >> > The advantage is that xhci can always use the grandparent device > >> > as sysdev whenever it isn't probed through PCI or firmware > >> > itself, but the purpose of the dwc3-glue is otherwise questionable. > >> > > >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > >> > device when that is created from the PCI driver and checking for that > >> > with the device property interface instead? If it's "snps,dwc3" > >> > we use the device itself while for "snps,dwc3-pci", we use the parent? > >> > >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? > > > > That would be incompatible with the USB binding, as the sysdev > > is assumed to be a USB host controller with #address-cells=<1> > > and #size-cells=<0> in order to hold the child devices, for > > example: > > > > / { > > omap_dwc3_1: omap_dwc3_1@4888 { > > compatible = "ti,dwc3"; > > #address-cells = <1>; > > #size-cells = <1>; > > ranges; > > usb1: usb@4889 { > > compatible = "snps,dwc3"; > > reg = <0x4889 0x17000>; > > #address-cells = <1>; > > #size-cells = <0>; > > interrupts = , > > , > > ; > > interrupt-names = "peripheral", > > "host", > > "otg"; > > phys = <&usb2_phy1>, <&usb3_phy1>; > > phy-names = "usb2-phy", "usb3-phy"; > > > > hub@1 { > > compatible = "usb5e3,608"; > > reg = <1>; > > #address-cells = <1>; > > #size-cells = <0>; > > > > ethernet@1 { > > compatible = "usb424,ec00"; > > mac-address = [00 11 22 33 44 55]; > > reg = <1>; > > }; > > }; > > }; > > }; > > > > It's also the node that contains the "phys" properties and > > presumably other properties like "otg-rev", "maximum-speed" > > etc. > > > > If we make the sysdev point to the parent, then we can no longer > > look up those properties and child devices from the USB core code > > by looking at "sysdev->of_node". > > this also makes things more interesting. I can't of anything other than > having some type of flag passed via e.g. device_properties by dwc3-pci.c > :-s Ok. > It's quite a hack, though. I still think that inheriting DMA (or > manually initializing a child with parent's DMA bits and pieces) is the > best way to go. So we're back to of_dma_configure() and > acpi_dma_configure(), right? That won't solve the problems with the DT properties or the dma configuration for PCI devices though. > But this needs to be done before dwc3_probe() executes. For dwc3-pci > that's easy, but for DT devices, seems like it should be in of > core. Below is, clearly, not enough but should show the idea: > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad7c403..a54610198946 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct > device_node *np) > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > * setup the correct supported mask. > */ > - if (!dev->coherent_dma_mask) > - dev->coherent_dma_mask = DMA_BIT_MASK(32); > + if (!dev->coherent_dma_mask) { > + if (!dev->parent->coherent_dma_mask) > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + else > + dev->coherent_dma_mask = > dev->parent->coherent_dma_mask; > + } > As the comment above that code says, the default 32-bit mask is intentional, and you need the driver to ask for the mask it wants using dma_set_mask_and_coherent(), while the platform code should be able to use dev->of_node to figure out whether that mask is supported. Just setting the initial mask to something else based on what the parent supports will not do the right thing elsewhere. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: >> >> > If we do that, we have to put child devices of the dwc3 devices into >> >> > the platform glue, and it also breaks those dwc3 devices that don't >> >> > have a parent driver. >> >> >> >> Well, this is easy to fix: >> >> >> >> if (dwc->dev->parent) { >> >> dwc->sysdev = dwc->dev->parent; >> >> } else { >> >> dev_info(dwc->dev, "Please provide a glue layer!\n"); >> >> dwc->sysdev = dwc->dev; >> >> } >> > >> > I don't understand. Do you mean we should have an extra level of >> > stacking and splitting "static struct platform_driver dwc3_driver" >> > in two so instead of >> > >> > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) >> > >> > we do this? >> > >> > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" >> > (usb_bus.dev) >> >> no >> >> If we have a parent device, use that as sysdev, otherwise use self as >> sysdev. > > But there is often a parent device in DT, as the xhci device is > attached to some internal bus that gets turned into a platform_device > as well, so checking whether there is a parent will get the wrong > device node. oh, that makes things more interesting :-s >> > That sounds a bit clumsy for the sake of consistency with PCI. >> > The advantage is that xhci can always use the grandparent device >> > as sysdev whenever it isn't probed through PCI or firmware >> > itself, but the purpose of the dwc3-glue is otherwise questionable. >> > >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 >> > device when that is created from the PCI driver and checking for that >> > with the device property interface instead? If it's "snps,dwc3" >> > we use the device itself while for "snps,dwc3-pci", we use the parent? >> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? > > That would be incompatible with the USB binding, as the sysdev > is assumed to be a USB host controller with #address-cells=<1> > and #size-cells=<0> in order to hold the child devices, for > example: > > / { > omap_dwc3_1: omap_dwc3_1@4888 { > compatible = "ti,dwc3"; > #address-cells = <1>; > #size-cells = <1>; > ranges; > usb1: usb@4889 { > compatible = "snps,dwc3"; > reg = <0x4889 0x17000>; > #address-cells = <1>; > #size-cells = <0>; > interrupts = , > , > ; > interrupt-names = "peripheral", > "host", > "otg"; > phys = <&usb2_phy1>, <&usb3_phy1>; > phy-names = "usb2-phy", "usb3-phy"; > > hub@1 { > compatible = "usb5e3,608"; > reg = <1>; > #address-cells = <1>; > #size-cells = <0>; > > ethernet@1 { > compatible = "usb424,ec00"; > mac-address = [00 11 22 33 44 55]; > reg = <1>; > }; > }; > }; > }; > > It's also the node that contains the "phys" properties and > presumably other properties like "otg-rev", "maximum-speed" > etc. > > If we make the sysdev point to the parent, then we can no longer > look up those properties and child devices from the USB core code > by looking at "sysdev->of_node". this also makes things more interesting. I can't of anything other than having some type of flag passed via e.g. device_properties by dwc3-pci.c :-s It's quite a hack, though. I still think that inheriting DMA (or manually initializing a child with parent's DMA bits and pieces) is the best way to go. So we're back to of_dma_configure() and acpi_dma_configure(), right? But this needs to be done before dwc3_probe() executes. For dwc3-pci that's easy, but for DT devices, seems like it should be in of core. Below is, clearly, not enough but should show the idea: diff --git a/drivers/of/device.c b/drivers/of/device.c index fd5cfad7c403..a54610198946 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node *np) * Set default coherent_dma_mask to 32 bit. Drivers are expected to * setup the correct supported mask. */ - if (!dev->coherent_dma_mask) - dev->coherent_dma_mask = DMA_BIT_MASK(32); + if (!dev->coherent_dma_mask) { + if (!dev->parent->coherent_dma_mask) + dev->coherent_dma_mask = DMA_BIT_MASK(32); + else +
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: > >> > If we do that, we have to put child devices of the dwc3 devices into > >> > the platform glue, and it also breaks those dwc3 devices that don't > >> > have a parent driver. > >> > >> Well, this is easy to fix: > >> > >> if (dwc->dev->parent) { > >> dwc->sysdev = dwc->dev->parent; > >> } else { > >> dev_info(dwc->dev, "Please provide a glue layer!\n"); > >> dwc->sysdev = dwc->dev; > >> } > > > > I don't understand. Do you mean we should have an extra level of > > stacking and splitting "static struct platform_driver dwc3_driver" > > in two so instead of > > > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) > > > > we do this? > > > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" > > (usb_bus.dev) > > no > > If we have a parent device, use that as sysdev, otherwise use self as > sysdev. But there is often a parent device in DT, as the xhci device is attached to some internal bus that gets turned into a platform_device as well, so checking whether there is a parent will get the wrong device node. > > That sounds a bit clumsy for the sake of consistency with PCI. > > The advantage is that xhci can always use the grandparent device > > as sysdev whenever it isn't probed through PCI or firmware > > itself, but the purpose of the dwc3-glue is otherwise questionable. > > > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > > device when that is created from the PCI driver and checking for that > > with the device property interface instead? If it's "snps,dwc3" > > we use the device itself while for "snps,dwc3-pci", we use the parent? > > Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? That would be incompatible with the USB binding, as the sysdev is assumed to be a USB host controller with #address-cells=<1> and #size-cells=<0> in order to hold the child devices, for example: / { omap_dwc3_1: omap_dwc3_1@4888 { compatible = "ti,dwc3"; #address-cells = <1>; #size-cells = <1>; ranges; usb1: usb@4889 { compatible = "snps,dwc3"; reg = <0x4889 0x17000>; #address-cells = <1>; #size-cells = <0>; interrupts = , , ; interrupt-names = "peripheral", "host", "otg"; phys = <&usb2_phy1>, <&usb3_phy1>; phy-names = "usb2-phy", "usb3-phy"; hub@1 { compatible = "usb5e3,608"; reg = <1>; #address-cells = <1>; #size-cells = <0>; ethernet@1 { compatible = "usb424,ec00"; mac-address = [00 11 22 33 44 55]; reg = <1>; }; }; }; }; It's also the node that contains the "phys" properties and presumably other properties like "otg-rev", "maximum-speed" etc. If we make the sysdev point to the parent, then we can no longer look up those properties and child devices from the USB core code by looking at "sysdev->of_node". Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: >> > If we do that, we have to put child devices of the dwc3 devices into >> > the platform glue, and it also breaks those dwc3 devices that don't >> > have a parent driver. >> >> Well, this is easy to fix: >> >> if (dwc->dev->parent) { >> dwc->sysdev = dwc->dev->parent; >> } else { >> dev_info(dwc->dev, "Please provide a glue layer!\n"); >> dwc->sysdev = dwc->dev; >> } > > I don't understand. Do you mean we should have an extra level of > stacking and splitting "static struct platform_driver dwc3_driver" > in two so instead of > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) > > we do this? > > "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" > (usb_bus.dev) no :-) If we have a parent device, use that as sysdev, otherwise use self as sysdev. > That sounds a bit clumsy for the sake of consistency with PCI. > The advantage is that xhci can always use the grandparent device > as sysdev whenever it isn't probed through PCI or firmware > itself, but the purpose of the dwc3-glue is otherwise questionable. > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 > device when that is created from the PCI driver and checking for that > with the device property interface instead? If it's "snps,dwc3" > we use the device itself while for "snps,dwc3-pci", we use the parent? Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote: > > If we do that, we have to put child devices of the dwc3 devices into > > the platform glue, and it also breaks those dwc3 devices that don't > > have a parent driver. > > Well, this is easy to fix: > > if (dwc->dev->parent) { > dwc->sysdev = dwc->dev->parent; > } else { > dev_info(dwc->dev, "Please provide a glue layer!\n"); > dwc->sysdev = dwc->dev; > } I don't understand. Do you mean we should have an extra level of stacking and splitting "static struct platform_driver dwc3_driver" in two so instead of "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev) we do this? "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" (usb_bus.dev) That sounds a bit clumsy for the sake of consistency with PCI. The advantage is that xhci can always use the grandparent device as sysdev whenever it isn't probed through PCI or firmware itself, but the purpose of the dwc3-glue is otherwise questionable. How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 device when that is created from the PCI driver and checking for that with the device property interface instead? If it's "snps,dwc3" we use the device itself while for "snps,dwc3-pci", we use the parent? Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: >> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 >> > *dwc) >> > static void dwc3_free_one_event_buffer(struct dwc3 *dwc, >> >struct dwc3_event_buffer *evt) >> > { >> > - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); >> > + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma); >> >> how about "dma_dev" instead? Is this used for anything other than DMA? > > The two other things we have discussed in this thread are: > > - connecting of_node pointers to usb_device structures for children > of sysdev->of_node. Note that this can happen even for PCI devices > in case you have a USB ethernet device hardwired to a PCI-USB bridge > and put the mac address in DT. > > - finding the PHY device for a HCD > > There might be others. Basically sysdev here is what the USB core code > can use for looking up any kind of properties provided by the firmware. fair enough >> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev) >> >dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1); >> >dwc->mem = mem; >> >dwc->dev = dev; >> > +#ifdef CONFIG_PCI >> > + /* TODO: or some other way of detecting this? */ >> > + if (dwc->dev->parent && dwc->dev->parent->bus == &pci_bus_type) >> > + dwc->sysdev = dwc->dev->parent; >> > + else >> > +#endif >> > + dwc->sysdev = dwc->dev; >> >> Well, we can remove this ifdef and *always* use the parent. We will just >> require that dwc3 users provide a glue layer. In that case, your check >> becomes: >> >> if (dwc->dev->parent) >> dwc->sysdev = dwc->dev->parent; >> else >> dev_info(dwc->dev, "Please provide a glue layer!\n"); > > If we do that, we have to put child devices of the dwc3 devices into > the platform glue, and it also breaks those dwc3 devices that don't > have a parent driver. Well, this is easy to fix: if (dwc->dev->parent) { dwc->sysdev = dwc->dev->parent; } else { dev_info(dwc->dev, "Please provide a glue layer!\n"); dwc->sysdev = dwc->dev; } >> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c >> > index 89a2f712fdfe..4d7439cb8cd8 100644 >> > --- a/drivers/usb/dwc3/dwc3-st.c >> > +++ b/drivers/usb/dwc3/dwc3-st.c >> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev) >> >if (IS_ERR(regmap)) >> >return PTR_ERR(regmap); >> > >> > - dma_set_coherent_mask(dev, dev->coherent_dma_mask); >> >> so is this. >> >> All in all, I like where you're going with this, we just need a matching >> acpi_dma_configure() and problems will be sorted out. > > With this patch, I don't think we even need that any more, as the device > that we use the dma-mapping API is the one that already gets configured > correctly by the platform code for all cases: PCI, OF, ACPI and combinations > of those. sounds good to me -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > >> Arnd Bergmann writes: > just look at the history of the file, you'll see that an Intel employee > was a maintainer of chipidea driver. Also: > > $ git ls-files drivers/usb/chipidea/ | grep pci > drivers/usb/chipidea/ci_hdrc_pci.c Right, Peter pointed that one out too. > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 35d092456bec..08db66c64c66 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -25,6 +25,7 @@ > > #include > > #include > > #include > > +#include > > actually, we don't want the core to know what it's attached to. Agreed. This was just a first draft and I couldn't come up with a better way to detect the case in which the parent device is probed from another HW bus and the child is not known to the firmware. > > #include > > #include > > #include > > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 > > *dwc) > > static void dwc3_free_one_event_buffer(struct dwc3 *dwc, > > struct dwc3_event_buffer *evt) > > { > > - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); > > + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma); > > how about "dma_dev" instead? Is this used for anything other than DMA? The two other things we have discussed in this thread are: - connecting of_node pointers to usb_device structures for children of sysdev->of_node. Note that this can happen even for PCI devices in case you have a USB ethernet device hardwired to a PCI-USB bridge and put the mac address in DT. - finding the PHY device for a HCD There might be others. Basically sysdev here is what the USB core code can use for looking up any kind of properties provided by the firmware. > > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1); > > dwc->mem = mem; > > dwc->dev = dev; > > +#ifdef CONFIG_PCI > > + /* TODO: or some other way of detecting this? */ > > + if (dwc->dev->parent && dwc->dev->parent->bus == &pci_bus_type) > > + dwc->sysdev = dwc->dev->parent; > > + else > > +#endif > > + dwc->sysdev = dwc->dev; > > Well, we can remove this ifdef and *always* use the parent. We will just > require that dwc3 users provide a glue layer. In that case, your check > becomes: > > if (dwc->dev->parent) > dwc->sysdev = dwc->dev->parent; > else > dev_info(dwc->dev, "Please provide a glue layer!\n"); If we do that, we have to put child devices of the dwc3 devices into the platform glue, and it also breaks those dwc3 devices that don't have a parent driver. > > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c > > index 2f1fb7e7aa54..e27899bb5706 100644 > > --- a/drivers/usb/dwc3/dwc3-exynos.c > > +++ b/drivers/usb/dwc3/dwc3-exynos.c > > @@ -20,7 +20,6 @@ > > #include > > #include > > #include > > -#include > > #include > > #include > > #include > > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device > > *pdev) > > if (!exynos) > > return -ENOMEM; > > > > - /* > > -* Right now device-tree probed devices don't get dma_mask set. > > -* Since shared usb code relies on it, set it here for now. > > -* Once we move to full device tree support this will vanish off. > > -*/ > > - ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > > - if (ret) > > - return ret; > > this is a separate patch, right? Yes, this is probably just a cleanup that we can apply regardless. We have not needed this in a long time. > > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c > > index 89a2f712fdfe..4d7439cb8cd8 100644 > > --- a/drivers/usb/dwc3/dwc3-st.c > > +++ b/drivers/usb/dwc3/dwc3-st.c > > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev) > > if (IS_ERR(regmap)) > > return PTR_ERR(regmap); > > > > - dma_set_coherent_mask(dev, dev->coherent_dma_mask); > > so is this. > > All in all, I like where you're going with this, we just need a matching > acpi_dma_configure() and problems will be sorted out. With this patch, I don't think we even need that any more, as the device that we use the dma-mapping API is the one that already gets configured correctly by the platform code for all cases: PCI, OF, ACPI and combinations of those. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: >> Arnd Bergmann writes: >> >> [...] >> >> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(), >> > I think we should replace >> > >> > pdev->dev.dma_mask = dev->dma_mask; >> > pdev->dev.dma_parms = dev->dma_parms; >> > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >> > >> > with of_dma_configure(), which has the chance to configure more than >> > just those three, as the dma API might look into different aspects: >> > >> > - iommu specific configuration >> > - cache coherency information >> > - bus type >> > - dma offset >> > - dma_map_ops pointer >> > >> > We try to handle everything in of_dma_configure() at configuration >> > time, and that would be the place to add anything else that we might >> > need in the future. >> >> There are a couple problems with this: >> >> 1) won't work for PCI-based systems. >> >> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX >> platform (FPGA that appears like a PCI card to host PC) > > Right, I was specifically talking about the code in chipidea here, > which I think is never used on the PCI bus, and how the current just look at the history of the file, you'll see that an Intel employee was a maintainer of chipidea driver. Also: $ git ls-files drivers/usb/chipidea/ | grep pci drivers/usb/chipidea/ci_hdrc_pci.c > code is broken. We can probably do better than of_dma_configure() > (see below), but it would be an improvement. > >> 2) not very robust solution >> >> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because >> that's not created by DT. The only reason why this works at all is >> because of the default 32-bit dma mask thing :-) So, how is it any >> different than copying 32-bit dma mask from parent? > > The idea here is that you pass in the parent of_node along with the child > device pointer, so it would behave exactly like the parent already does. > The difference is that it also handles all the other attributes besides the > mask. Now we're talking :-) I like that. We just need a matching API for ACPI/PCI-based systems. > However, to summarize the discussion so far, I agree that > of_dma_configure() is not the solution to these problems, and I think > we can do much better: > > Splitting the usb_bus->controller field into the Linux-internal device > (used for the sysfs hierarchy, for printks and for power management) > and a new pointer (used for DMA, DT enumeration and phy lookup) probably > covers all that we really need. > > I've prototyped it below, with the dwc3, xhci and chipidea changes > together with the core changes. I've surely made mistakes there and > don't expect it to work out of the box, but this should give an > idea of how I think this can all be solved in the least invasive > way. > > I noticed that the gadget interface already has a way to handle the > DMA allocation by device, so I added that in as well. yeah, I wanna use that :-) > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 35d092456bec..08db66c64c66 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include actually, we don't want the core to know what it's attached to. > #include > #include > #include > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc) > static void dwc3_free_one_event_buffer(struct dwc3 *dwc, > struct dwc3_event_buffer *evt) > { > - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); > + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma); how about "dma_dev" instead? Is this used for anything other than DMA? > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev) > dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1); > dwc->mem = mem; > dwc->dev = dev; > +#ifdef CONFIG_PCI > + /* TODO: or some other way of detecting this? */ > + if (dwc->dev->parent && dwc->dev->parent->bus == &pci_bus_type) > + dwc->sysdev = dwc->dev->parent; > + else > +#endif > + dwc->sysdev = dwc->dev; Well, we can remove this ifdef and *always* use the parent. We will just require that dwc3 users provide a glue layer. In that case, your check becomes: if (dwc->dev->parent) dwc->sysdev = dwc->dev->parent; else dev_info(dwc->dev, "Please provide a glue layer!\n"); > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c > index 2f1fb7e7aa54..e27899bb5706 100644 > --- a/drivers/usb/dwc3/dwc3-exynos.c > +++ b/drivers/usb/dwc3/dwc3-exynos.c > @@ -20,7 +20,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device > *pdev) > if (!exynos) > return -ENOMEM; > > - /* > - * Right now device-tree p
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 8, 2016 9:15:36 AM CEST Peter Chen wrote: > > > > Right, I was specifically talking about the code in chipidea here, > > which I think is never used on the PCI bus, and how the current > > code is broken. We can probably do better than of_dma_configure() > > (see below), but it would be an improvement. > > Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c > Ok, I see. The experimental patch I posted should actually handle this just fine, as it simply assumes that dev->parent is the device used for the DMA API in chipidea, and I think this holds true for both the PCI and the DT based uses of this driver. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Sep 07, 2016 at 05:24:08PM +0200, Arnd Bergmann wrote: > On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote: > > > > Hi, > > > > Arnd Bergmann writes: > > > > [...] > > > > > Regarding the DMA configuration that you mention in ci_hdrc_add_device(), > > > I think we should replace > > > > > > pdev->dev.dma_mask = dev->dma_mask; > > > pdev->dev.dma_parms = dev->dma_parms; > > > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > > > > > with of_dma_configure(), which has the chance to configure more than > > > just those three, as the dma API might look into different aspects: > > > > > > - iommu specific configuration > > > - cache coherency information > > > - bus type > > > - dma offset > > > - dma_map_ops pointer > > > > > > We try to handle everything in of_dma_configure() at configuration > > > time, and that would be the place to add anything else that we might > > > need in the future. > > > > There are a couple problems with this: > > > > 1) won't work for PCI-based systems. > > > > DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX > > platform (FPGA that appears like a PCI card to host PC) > > Right, I was specifically talking about the code in chipidea here, > which I think is never used on the PCI bus, and how the current > code is broken. We can probably do better than of_dma_configure() > (see below), but it would be an improvement. Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 7, 2016 12:08:20 PM CEST Alan Stern wrote: > On Wed, 7 Sep 2016, Arnd Bergmann wrote: > > > drivers/usb/host/ehci-fsl.c| 4 ++-- > > How did this driver end up in the patch? > > > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c > > index 9f5ffb629973..b2419950221f 100644 > > --- a/drivers/usb/host/ehci-fsl.c > > +++ b/drivers/usb/host/ehci-fsl.c > > @@ -96,8 +96,8 @@ static int fsl_ehci_drv_probe(struct platform_device > > *pdev) > > } > > irq = res->start; > > > > - hcd = usb_create_hcd(&fsl_ehci_hc_driver, &pdev->dev, > > - dev_name(&pdev->dev)); > > + hcd = __usb_create_hcd(&fsl_ehci_hc_driver, &pdev->dev.parent, > > + &pdev->dev, dev_name(&pdev->dev), NULL); > > Based on the > > if (of_device_is_compatible(dev->parent->of_node, > "fsl,mpc5121-usb2-dr")) { > > lines in the driver? No, based on the "fsl-ehci" name, which is only used as a child of the drivers/usb/host/fsl-mph-dr-of.c dual-role driver. I looked for drivers that call platform_device_add() and manipulate the dma_mask of that child. Sorry for missing this one when I did the description. I believe it is correct though. The DMA settings on powerpc are probably correct in this case, but the existing code doesn't allow you to describe on-board USB devices with additional properties as children of the dual-role device node. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, 7 Sep 2016, Arnd Bergmann wrote: > However, to summarize the discussion so far, I agree that > of_dma_configure() is not the solution to these problems, and I think > we can do much better: > > Splitting the usb_bus->controller field into the Linux-internal device > (used for the sysfs hierarchy, for printks and for power management) > and a new pointer (used for DMA, DT enumeration and phy lookup) probably > covers all that we really need. > > I've prototyped it below, with the dwc3, xhci and chipidea changes > together with the core changes. I've surely made mistakes there and > don't expect it to work out of the box, but this should give an > idea of how I think this can all be solved in the least invasive > way. > > I noticed that the gadget interface already has a way to handle the > DMA allocation by device, so I added that in as well. > > Signed-off-by: Arnd Bergmann > > drivers/usb/chipidea/core.c| 4 > drivers/usb/chipidea/host.c| 3 ++- > drivers/usb/chipidea/udc.c | 8 > drivers/usb/core/buffer.c | 12 ++-- > drivers/usb/core/hcd.c | 48 > +--- > drivers/usb/core/usb.c | 16 > drivers/usb/dwc3/core.c| 28 +++- > drivers/usb/dwc3/core.h| 1 + > drivers/usb/dwc3/dwc3-exynos.c | 10 -- > drivers/usb/dwc3/dwc3-st.c | 1 - > drivers/usb/dwc3/ep0.c | 8 > drivers/usb/dwc3/gadget.c | 34 +- > drivers/usb/dwc3/host.c| 13 - > drivers/usb/host/ehci-fsl.c| 4 ++-- How did this driver end up in the patch? > drivers/usb/host/xhci-plat.c | 32 +--- > include/linux/usb.h| 1 + > include/linux/usb/hcd.h| 3 +++ > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c > index 9f5ffb629973..b2419950221f 100644 > --- a/drivers/usb/host/ehci-fsl.c > +++ b/drivers/usb/host/ehci-fsl.c > @@ -96,8 +96,8 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev) > } > irq = res->start; > > - hcd = usb_create_hcd(&fsl_ehci_hc_driver, &pdev->dev, > - dev_name(&pdev->dev)); > + hcd = __usb_create_hcd(&fsl_ehci_hc_driver, &pdev->dev.parent, > + &pdev->dev, dev_name(&pdev->dev), NULL); Based on the if (of_device_is_compatible(dev->parent->of_node, "fsl,mpc5121-usb2-dr")) { lines in the driver? > if (!hcd) { > retval = -ENOMEM; > goto err1; Alan Stern
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote: > > Hi, > > Arnd Bergmann writes: > > [...] > > > Regarding the DMA configuration that you mention in ci_hdrc_add_device(), > > I think we should replace > > > > pdev->dev.dma_mask = dev->dma_mask; > > pdev->dev.dma_parms = dev->dma_parms; > > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > > > with of_dma_configure(), which has the chance to configure more than > > just those three, as the dma API might look into different aspects: > > > > - iommu specific configuration > > - cache coherency information > > - bus type > > - dma offset > > - dma_map_ops pointer > > > > We try to handle everything in of_dma_configure() at configuration > > time, and that would be the place to add anything else that we might > > need in the future. > > There are a couple problems with this: > > 1) won't work for PCI-based systems. > > DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX > platform (FPGA that appears like a PCI card to host PC) Right, I was specifically talking about the code in chipidea here, which I think is never used on the PCI bus, and how the current code is broken. We can probably do better than of_dma_configure() (see below), but it would be an improvement. > 2) not very robust solution > > of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because > that's not created by DT. The only reason why this works at all is > because of the default 32-bit dma mask thing :-) So, how is it any > different than copying 32-bit dma mask from parent? The idea here is that you pass in the parent of_node along with the child device pointer, so it would behave exactly like the parent already does. The difference is that it also handles all the other attributes besides the mask. However, to summarize the discussion so far, I agree that of_dma_configure() is not the solution to these problems, and I think we can do much better: Splitting the usb_bus->controller field into the Linux-internal device (used for the sysfs hierarchy, for printks and for power management) and a new pointer (used for DMA, DT enumeration and phy lookup) probably covers all that we really need. I've prototyped it below, with the dwc3, xhci and chipidea changes together with the core changes. I've surely made mistakes there and don't expect it to work out of the box, but this should give an idea of how I think this can all be solved in the least invasive way. I noticed that the gadget interface already has a way to handle the DMA allocation by device, so I added that in as well. Signed-off-by: Arnd Bergmann drivers/usb/chipidea/core.c| 4 drivers/usb/chipidea/host.c| 3 ++- drivers/usb/chipidea/udc.c | 8 drivers/usb/core/buffer.c | 12 ++-- drivers/usb/core/hcd.c | 48 +--- drivers/usb/core/usb.c | 16 drivers/usb/dwc3/core.c| 28 +++- drivers/usb/dwc3/core.h| 1 + drivers/usb/dwc3/dwc3-exynos.c | 10 -- drivers/usb/dwc3/dwc3-st.c | 1 - drivers/usb/dwc3/ep0.c | 8 drivers/usb/dwc3/gadget.c | 34 +- drivers/usb/dwc3/host.c| 13 - drivers/usb/host/ehci-fsl.c| 4 ++-- drivers/usb/host/xhci-plat.c | 32 +--- include/linux/usb.h| 1 + include/linux/usb/hcd.h| 3 +++ diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 69426e644d17..dff69837b349 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -833,10 +833,6 @@ struct platform_device *ci_hdrc_add_device(struct device *dev, } pdev->dev.parent = dev; - pdev->dev.dma_mask = dev->dma_mask; - pdev->dev.dma_parms = dev->dma_parms; - dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); - ret = platform_device_add_resources(pdev, res, nres); if (ret) goto err; diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 053bac9d983c..40d29c4d7772 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -113,7 +113,8 @@ static int host_start(struct ci_hdrc *ci) if (usb_disabled()) return -ENODEV; - hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev)); + hcd = __usb_create_hcd(&ci_ehci_hc_driver, ci->dev->parent, + ci->dev, dev_name(ci->dev), NULL); if (!hcd) return -ENOMEM; diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 0f692fcda638..4cbfff7934c6 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -424,7 +424,7 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) hwreq->req.status = -EALREADY; -
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 7, 2016 4:04:52 PM CEST Roger Quadros wrote: > > The main use for it is to let you specify a MAC address for on-board > > ethernet devices that lack an EPROM, but any other information can be > > added that way too. > > > >> There is a bug in the USB core because of which the ISB device and > >> interfaces > >> do not inherit dma_pfn_offset correctly for which I've sent a patch > >> https://lkml.org/lkml/2016/8/17/275 > > > > I'm a bit skeptical about this. Clearly if we set the dma_mask, we should > > also set the dma_pfn_offset, but what exactly is this used for in USB > > devices? > > Consider the mass storage device case. > USB storage driver creates a scsi host for the mass storage interface in > drivers/usb/storage/usb.c > The scsi host parent device is nothing but the the USB interface device. > > Now, __scsi_init_queue() calls scsi_calculate_bounce_limit() to find out > and set the block layer bounce limit. > > scsi_calculate_bounce_limit() uses dma_max_pfn(host_dev) to get the > bounce_limit. > > host_dev is nothing but the device representing the mass storage interface. > > If that device doesn't have the right dma_pfn_offset, then dma_max_pfn() > is messed up and the bounce buffer limit is wrong. I see. The same thing probably happens in the network and mmc subsystems, which have similar code. This shows the inconsistencies we have in the handling for bounce buffers in the kernel, which are sometimes handled by subsystems but sometimes rely on swiotlb instead. I don't have any better idea than your patch here, but maybe we should add a comment explaining that next to the code. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 07/09/16 11:29, Arnd Bergmann wrote: > On Wednesday, September 7, 2016 10:17:31 AM CEST Roger Quadros wrote: >>> >>> Speaking of that flag, I suppose we need the same logic to know where >>> to look for USB devices attached to a dwc3 host when we need to describe >>> them in DT. By default we look for child device nodes under the >>> node of the HCD device node, but that would be wrong here too. >> >> I didn't get this part. Information about USB devices attached to a USB host >> is never provided in DT because they are always dynamically created via >> usb_new_device(), whether they are hard-wired on the board or hot-plugged. >> >> These USB devices inherit their DMA masks in the usb_alloc_dev() routine >> whereas each interface within the USB device inherits its DMA mask in >> usb_set_configuration(). > > We had talked about adding support for this for at least six years (probably > much more), but Peter Chen finally added it this year in commit 69bec72598 > ("USB: core: let USB device know device node"). OK. Thanks for this pointer. > > The main use for it is to let you specify a MAC address for on-board > ethernet devices that lack an EPROM, but any other information can be > added that way too. > >> There is a bug in the USB core because of which the ISB device and >> interfaces >> do not inherit dma_pfn_offset correctly for which I've sent a patch >> https://lkml.org/lkml/2016/8/17/275 > > I'm a bit skeptical about this. Clearly if we set the dma_mask, we should > also set the dma_pfn_offset, but what exactly is this used for in USB > devices? Consider the mass storage device case. USB storage driver creates a scsi host for the mass storage interface in drivers/usb/storage/usb.c The scsi host parent device is nothing but the the USB interface device. Now, __scsi_init_queue() calls scsi_calculate_bounce_limit() to find out and set the block layer bounce limit. scsi_calculate_bounce_limit() uses dma_max_pfn(host_dev) to get the bounce_limit. host_dev is nothing but the device representing the mass storage interface. If that device doesn't have the right dma_pfn_offset, then dma_max_pfn() is messed up and the bounce buffer limit is wrong. > > As I understand it, the dma_mask/dma_pfn_offset etc is used for the DMA > mapping interface, but that can't really be used on USB devices, which > I assume use usb_alloc_coherent() and the URB interfaces for passing data > between a USB driver and the HCD. My knowledge of USB device drivers > is a bit lacking, so it's possible I'm misunderstanding things here. > cheers, -roger
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Robin Murphy writes: > On 07/09/16 10:55, Peter Chen wrote: > [...] >>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(), >>> I think we should replace >>> >>> pdev->dev.dma_mask = dev->dma_mask; >>> pdev->dev.dma_parms = dev->dma_parms; >>> dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >>> >>> with of_dma_configure(), which has the chance to configure more than >>> just those three, as the dma API might look into different aspects: >>> >>> - iommu specific configuration >>> - cache coherency information >>> - bus type >>> - dma offset >>> - dma_map_ops pointer >>> >>> We try to handle everything in of_dma_configure() at configuration >>> time, and that would be the place to add anything else that we might >>> need in the future. >>> >> >> Yes, I agree with you, but just like Felipe mentioned, we also need to >> consider PCI device, can we do something like gpiod_get_index does? Are >> there any similar APIs like of_dma_configure for ACPI? > > Not yet, but Lorenzo has one in progress[1], primarily for the sake of > abstracting away the IOMMU configuration. > > Robin. > > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html not exported for drivers to use. If Lorenzo is trying to making a matching API for ACPI systems, then it needs to follow what of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL() -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 07/09/16 10:55, Peter Chen wrote: [...] >> Regarding the DMA configuration that you mention in ci_hdrc_add_device(), >> I think we should replace >> >> pdev->dev.dma_mask = dev->dma_mask; >> pdev->dev.dma_parms = dev->dma_parms; >> dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >> >> with of_dma_configure(), which has the chance to configure more than >> just those three, as the dma API might look into different aspects: >> >> - iommu specific configuration >> - cache coherency information >> - bus type >> - dma offset >> - dma_map_ops pointer >> >> We try to handle everything in of_dma_configure() at configuration >> time, and that would be the place to add anything else that we might >> need in the future. >> > > Yes, I agree with you, but just like Felipe mentioned, we also need to > consider PCI device, can we do something like gpiod_get_index does? Are > there any similar APIs like of_dma_configure for ACPI? Not yet, but Lorenzo has one in progress[1], primarily for the sake of abstracting away the IOMMU configuration. Robin. [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: [...] > Regarding the DMA configuration that you mention in ci_hdrc_add_device(), > I think we should replace > > pdev->dev.dma_mask = dev->dma_mask; > pdev->dev.dma_parms = dev->dma_parms; > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > with of_dma_configure(), which has the chance to configure more than > just those three, as the dma API might look into different aspects: > > - iommu specific configuration > - cache coherency information > - bus type > - dma offset > - dma_map_ops pointer > > We try to handle everything in of_dma_configure() at configuration > time, and that would be the place to add anything else that we might > need in the future. There are a couple problems with this: 1) won't work for PCI-based systems. DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX platform (FPGA that appears like a PCI card to host PC) 2) not very robust solution of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because that's not created by DT. The only reason why this works at all is because of the default 32-bit dma mask thing :-) So, how is it any different than copying 32-bit dma mask from parent? -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Russell King - ARM Linux writes: > On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote: >> On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote: >> > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote: >> > > >> > > The pre-condition of DT function at USB HCD core works is the host >> > > controller device has of_node, since it is the root node for USB tree >> > > described at DT. If the host controller device is not at DT, it needs >> > > to try to get its of_node, the chipidea driver gets it through its >> > > parent node [1] >> > >> > > >> > > [1] https://lkml.org/lkml/2016/8/8/119 >> > > >> > >> > Ah, this is what I was referring to in the other mail. >> > >> > However, the way you set the of_node might be dangerous too: >> > We should generally not have two platform_device structures with >> > the same of_node pointer, most importantly it may cause the >> > child device to be bound to the same driver as the parent >> > device since the probing is done by compatible string. >> > >> > As you tested it successfully, it must work at the moment on your >> > machine, but it could easily break depending on deferred probing >> > or module load order. >> > >> >> Currently, I work around above problems by setting core device of_node >> as NULL at both probe error path and platform driver .remove routine. >> >> I admit it is not a good way, but if we only have of_node at device's >> life periods after probe, it seems ok currently. It is hard to create >> of_node dynamically when create device, and keep some contents >> of parent's of_node, and some are not. > > How about turning dwc3 into a library which can be used by a range of > platform devices? Wouldn't that solve all the current problems, and > completely avoid the need to copy resources from one platform device > to another? This will break all existing DTs out there. Also, there are other benefits from keeping current design, these have been discussed before but here's a short summary: . PM callbacks are kept simple . We avoid abuse of internal dwc3 functions . It's a lot less work to "port" dwc3 to "your SoC" . We prevent another MUSB (drivers/usb/musb/) And few others. Sure, they are rather subjective benefits, but it has worked well so far. Also, breaking DT ABI is kind of a big deal. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Sep 07, 2016 at 10:48:06AM +0200, Arnd Bergmann wrote: > On Wednesday, September 7, 2016 2:33:13 PM CEST Peter Chen wrote: > > > > > > Right, that should make it work with iommu as well. However, it does > > > not solve the other issue I mentioned above, with boards that have > > > USB devices hardwired to a chipidea host controller that need > > > configuration from DT. For that, we still need to come up with another > > > way to associate the DT hierarchy in the host bridge node with > > > the Linux platform_device. > > > > > > > Why? The DMA configuration is for host controller, not for USB device. > > No matter there is hardwired or hotplug devices, the DMA configuration > > for host controller are both inherited from glue layer platform devices, > > current implementation is at function ci_hdrc_add_device, > > drivers/usb/chipidea/core.c. > > I wasn't referring to DMA configuration there, but only to how we > set the of_node pointer in register_root_hub, which you added in > dc5878abf49 ("usb: core: move root hub's device node assignment after > it is added to bus") as: > > usb_dev->dev.of_node = parent_dev->of_node; > > As I understand, parent_dev (aka hcd->self.controller) here > refers to the device that you add in ci_hdrc_add_device(), > which does not have an of_node pointer, so we actually > want parent_dev->parent->of_node. For platform devices, like chipidea and dwc3, it is correct, since they don't have of_node. If the host controller has of_node, it is controller's of_node. In order to let USB HCD core life be easy, we need to let hcd->self.controller own of_node when it is added to HCD core, no matter it has from the dts or get it dynamically. > > I'm sure you understand that code better than me, so let me > know what my mistake is if this indeed works correctly. > Your understanding is correct, just some have of_node from dts, some (chipidea/dwc3) need to have assignment at its driver. > > > Regarding the DMA configuration that you mention in ci_hdrc_add_device(), > I think we should replace > > pdev->dev.dma_mask = dev->dma_mask; > pdev->dev.dma_parms = dev->dma_parms; > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > with of_dma_configure(), which has the chance to configure more than > just those three, as the dma API might look into different aspects: > > - iommu specific configuration > - cache coherency information > - bus type > - dma offset > - dma_map_ops pointer > > We try to handle everything in of_dma_configure() at configuration > time, and that would be the place to add anything else that we might > need in the future. > Yes, I agree with you, but just like Felipe mentioned, we also need to consider PCI device, can we do something like gpiod_get_index does? Are there any similar APIs like of_dma_configure for ACPI? -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote: > On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote: > > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote: > > > > > > The pre-condition of DT function at USB HCD core works is the host > > > controller device has of_node, since it is the root node for USB tree > > > described at DT. If the host controller device is not at DT, it needs > > > to try to get its of_node, the chipidea driver gets it through its > > > parent node [1] > > > > > > > > [1] https://lkml.org/lkml/2016/8/8/119 > > > > > > > Ah, this is what I was referring to in the other mail. > > > > However, the way you set the of_node might be dangerous too: > > We should generally not have two platform_device structures with > > the same of_node pointer, most importantly it may cause the > > child device to be bound to the same driver as the parent > > device since the probing is done by compatible string. > > > > As you tested it successfully, it must work at the moment on your > > machine, but it could easily break depending on deferred probing > > or module load order. > > > > Currently, I work around above problems by setting core device of_node > as NULL at both probe error path and platform driver .remove routine. > > I admit it is not a good way, but if we only have of_node at device's > life periods after probe, it seems ok currently. It is hard to create > of_node dynamically when create device, and keep some contents > of parent's of_node, and some are not. How about turning dwc3 into a library which can be used by a range of platform devices? Wouldn't that solve all the current problems, and completely avoid the need to copy resources from one platform device to another? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote: > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote: > > > > The pre-condition of DT function at USB HCD core works is the host > > controller device has of_node, since it is the root node for USB tree > > described at DT. If the host controller device is not at DT, it needs > > to try to get its of_node, the chipidea driver gets it through its > > parent node [1] > > > > > [1] https://lkml.org/lkml/2016/8/8/119 > > > > Ah, this is what I was referring to in the other mail. > > However, the way you set the of_node might be dangerous too: > We should generally not have two platform_device structures with > the same of_node pointer, most importantly it may cause the > child device to be bound to the same driver as the parent > device since the probing is done by compatible string. > > As you tested it successfully, it must work at the moment on your > machine, but it could easily break depending on deferred probing > or module load order. > Currently, I work around above problems by setting core device of_node as NULL at both probe error path and platform driver .remove routine. I admit it is not a good way, but if we only have of_node at device's life periods after probe, it seems ok currently. It is hard to create of_node dynamically when create device, and keep some contents of parent's of_node, and some are not. -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote: > > The pre-condition of DT function at USB HCD core works is the host > controller device has of_node, since it is the root node for USB tree > described at DT. If the host controller device is not at DT, it needs > to try to get its of_node, the chipidea driver gets it through its > parent node [1] > > [1] https://lkml.org/lkml/2016/8/8/119 > Ah, this is what I was referring to in the other mail. However, the way you set the of_node might be dangerous too: We should generally not have two platform_device structures with the same of_node pointer, most importantly it may cause the child device to be bound to the same driver as the parent device since the probing is done by compatible string. As you tested it successfully, it must work at the moment on your machine, but it could easily break depending on deferred probing or module load order. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 7, 2016 2:33:13 PM CEST Peter Chen wrote: > > > > Right, that should make it work with iommu as well. However, it does > > not solve the other issue I mentioned above, with boards that have > > USB devices hardwired to a chipidea host controller that need > > configuration from DT. For that, we still need to come up with another > > way to associate the DT hierarchy in the host bridge node with > > the Linux platform_device. > > > > Why? The DMA configuration is for host controller, not for USB device. > No matter there is hardwired or hotplug devices, the DMA configuration > for host controller are both inherited from glue layer platform devices, > current implementation is at function ci_hdrc_add_device, > drivers/usb/chipidea/core.c. I wasn't referring to DMA configuration there, but only to how we set the of_node pointer in register_root_hub, which you added in dc5878abf49 ("usb: core: move root hub's device node assignment after it is added to bus") as: usb_dev->dev.of_node = parent_dev->of_node; As I understand, parent_dev (aka hcd->self.controller) here refers to the device that you add in ci_hdrc_add_device(), which does not have an of_node pointer, so we actually want parent_dev->parent->of_node. I'm sure you understand that code better than me, so let me know what my mistake is if this indeed works correctly. Regarding the DMA configuration that you mention in ci_hdrc_add_device(), I think we should replace pdev->dev.dma_mask = dev->dma_mask; pdev->dev.dma_parms = dev->dma_parms; dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); with of_dma_configure(), which has the chance to configure more than just those three, as the dma API might look into different aspects: - iommu specific configuration - cache coherency information - bus type - dma offset - dma_map_ops pointer We try to handle everything in of_dma_configure() at configuration time, and that would be the place to add anything else that we might need in the future. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday, September 7, 2016 10:17:31 AM CEST Roger Quadros wrote: > > > > Speaking of that flag, I suppose we need the same logic to know where > > to look for USB devices attached to a dwc3 host when we need to describe > > them in DT. By default we look for child device nodes under the > > node of the HCD device node, but that would be wrong here too. > > I didn't get this part. Information about USB devices attached to a USB host > is never provided in DT because they are always dynamically created via > usb_new_device(), whether they are hard-wired on the board or hot-plugged. > > These USB devices inherit their DMA masks in the usb_alloc_dev() routine > whereas each interface within the USB device inherits its DMA mask in > usb_set_configuration(). We had talked about adding support for this for at least six years (probably much more), but Peter Chen finally added it this year in commit 69bec72598 ("USB: core: let USB device know device node"). The main use for it is to let you specify a MAC address for on-board ethernet devices that lack an EPROM, but any other information can be added that way too. > There is a bug in the USB core because of which the ISB device and interfaces > do not inherit dma_pfn_offset correctly for which I've sent a patch > https://lkml.org/lkml/2016/8/17/275 I'm a bit skeptical about this. Clearly if we set the dma_mask, we should also set the dma_pfn_offset, but what exactly is this used for in USB devices? As I understand it, the dma_mask/dma_pfn_offset etc is used for the DMA mapping interface, but that can't really be used on USB devices, which I assume use usb_alloc_coherent() and the URB interfaces for passing data between a USB driver and the HCD. My knowledge of USB device drivers is a bit lacking, so it's possible I'm misunderstanding things here. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Tue, Sep 06, 2016 at 03:27:43PM +0200, Arnd Bergmann wrote: > On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote: > > Hi, > > > > Arnd Bergmann writes: > > > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote: > > >> > > >> this only solves the problem for DT devices. Legacy devices and > > >> PCI-based systems will still suffer from the same problem. At least for > > >> dwc3, I will only be taking patches that solve the problem for all > > >> users, not a subset of them. > > > > > > I don't think legacy devices are a worry, because they wouldn't > > > have this problem. For the PCI case, you are right that it cannot > > > work, in particular for machines that have complex IOMMU setup. > > > > > > Some architectures (at least arm64 and sparc) check the bus_type of > > > a device in order to find the correct set of dma_map_ops for that > > > device, so there is no real way to handle this as long as you > > > pass a platform_device into an API that expects a pci_device. > > > > Then I guess we're left with adding a "struct device *dma_dev" to struct > > dwc3 and trying to figure out if we should use parent or self. Does > > anybody see any problems with that? > > I think we actually need the device pointer in the usb_hcd structure, > so it can be passed in these API calls from the USB core > > drivers/usb/core/buffer.c: return > dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags); > drivers/usb/core/buffer.c: dma_free_coherent(hcd->self.controller, size, > addr, dma); > drivers/usb/core/buffer.c: (!hcd->self.controller->dma_mask && > drivers/usb/core/buffer.c: hcd->pool[i] = dma_pool_create(name, > hcd->self.controller, > drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single( > drivers/usb/core/hcd.c: if > (dma_mapping_error(hcd->self.controller, > drivers/usb/core/hcd.c: n = dma_map_sg( > drivers/usb/core/hcd.c: urb->transfer_dma = > dma_map_page( > drivers/usb/core/hcd.c: if > (dma_mapping_error(hcd->self.controller, > drivers/usb/core/hcd.c: urb->transfer_dma = > dma_map_single( > drivers/usb/core/hcd.c: if > (dma_mapping_error(hcd->self.controller, > drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller, > drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents, > drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller, > drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller, > drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents, > > as these are all called on behalf of the host controller node. The USB HCD core uses the struct device pointer passed by usb_create_hcd which is called by each host controller driver, the host controller driver needs to make sure the information (DMA configurations, of_node, etc) in struct device are correct before calling usb_create_hcd. > Looking for more instances of hcd->self.controller, I find this > instance: > > drivers/usb/core/hcd.c: struct usb_phy *phy = > usb_get_phy_dev(hcd->self.controller, 0); > drivers/usb/core/hcd.c: struct phy *phy = > phy_get(hcd->self.controller, "usb"); > > I'm unsure which device pointer we want here, but I suspect this also > needs to be the one that has the device node in order to make the lookup > of the phy structure by device node work right. Can you clarify how > this works today? > The above codes are only called when the host controller driver does not the code which try to get USB PHY. Once the PHY drivers is probed, the above code can work no matter DT or non-DT. But by looking at the code, I am wondering how dwc3 host get its USB PHY at xhci-platform.c, it seems there is no of_node at xhci-hcd for dwc3. > We probably also need to add the of_node of the host controller device > to struct usb_hcd in order to make usb_of_get_child_node() work > in the case where the hcd itself is not device that is listed > in DT. The pre-condition of DT function at USB HCD core works is the host controller device has of_node, since it is the root node for USB tree described at DT. If the host controller device is not at DT, it needs to try to get its of_node, the chipidea driver gets it through its parent node [1] > It might be a good idea to use 'struct fwnode_handle' for that, > so we can in the future also allow ACPI platforms to specify > > > Note, we would NOT be passing device pointers are platform_data, we > > would have dwc3.ko figure out if it should use self or its parent device > > for dma. > > Ok, sounds good. > > Arnd [1] https://lkml.org/lkml/2016/8/8/119 -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi Arnd, On 02/09/16 18:51, Arnd Bergmann wrote: > On Friday, September 2, 2016 10:21:23 AM CEST Alan Stern wrote: >> On Fri, 2 Sep 2016, Felipe Balbi wrote: >> >>> Hi, >>> >>> Russell King - ARM Linux writes: On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: > On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: >> >> Hi Felipe and Arnd, >> >> It has been a while since the last response to this discussion, but we >> haven't reached an agreement yet! Can we get to a conclusion on if it >> is valid to create child platform device for abstraction purpose? If >> yes, can this child device do DMA by itself? > > I'd say it's no problem for a driver to create child devices in order > to represent different aspects of a device, but you should not rely on > those devices working when used with the dma-mapping interfaces. That's absolutely right. Consider the USB model - only the USB host controller can perform DMA, not the USB devices themselves. All DMA mappings need to be mapped using the USB host controller device struct not the USB device struct. The same _should_ be true everywhere else: the struct device representing the device performing DMA must be the one used to map the transfer. >>> >>> How do we fix dwc3 in dual-role, then? >>> >>> Peripheral-side dwc3 is easy, we just require a glue-layer to be present >>> and use dwc3.ko's parent device (which will be the PCI device or OF >>> device). But for host side dwc3, the problem is slightly more complex >>> because we're using xhci-plat.ko by just instantiating a xhci-platform >>> device so xhci-plat can probe. >>> >>> xhci core has no means to know if its own device or the parent of its >>> parent should be used for DMA. Any ideas? >> >> In theory, you can store a flag somewhere in the platform device, >> something that would tell xhci-hcd that it has to use the parent's >> parent for DMA purposes. >> >> I know it would be somewhat of a hack, but ought to work. > > Speaking of that flag, I suppose we need the same logic to know where > to look for USB devices attached to a dwc3 host when we need to describe > them in DT. By default we look for child device nodes under the > node of the HCD device node, but that would be wrong here too. I didn't get this part. Information about USB devices attached to a USB host is never provided in DT because they are always dynamically created via usb_new_device(), whether they are hard-wired on the board or hot-plugged. These USB devices inherit their DMA masks in the usb_alloc_dev() routine whereas each interface within the USB device inherits its DMA mask in usb_set_configuration(). There is a bug in the USB core because of which the ISB device and interfaces do not inherit dma_pfn_offset correctly for which I've sent a patch https://lkml.org/lkml/2016/8/17/275 cheers, -roger
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Arnd Bergmann writes: > On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote: >> Hi, >> >> Arnd Bergmann writes: >> > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote: >> >> >> >> this only solves the problem for DT devices. Legacy devices and >> >> PCI-based systems will still suffer from the same problem. At least for >> >> dwc3, I will only be taking patches that solve the problem for all >> >> users, not a subset of them. >> > >> > I don't think legacy devices are a worry, because they wouldn't >> > have this problem. For the PCI case, you are right that it cannot >> > work, in particular for machines that have complex IOMMU setup. >> > >> > Some architectures (at least arm64 and sparc) check the bus_type of >> > a device in order to find the correct set of dma_map_ops for that >> > device, so there is no real way to handle this as long as you >> > pass a platform_device into an API that expects a pci_device. >> >> Then I guess we're left with adding a "struct device *dma_dev" to struct >> dwc3 and trying to figure out if we should use parent or self. Does >> anybody see any problems with that? > > I think we actually need the device pointer in the usb_hcd structure, > so it can be passed in these API calls from the USB core that's for host side. I'm concerned about peripheral side > as these are all called on behalf of the host controller node. > Looking for more instances of hcd->self.controller, I find this > instance: > > drivers/usb/core/hcd.c: struct usb_phy *phy = > usb_get_phy_dev(hcd->self.controller, 0); > drivers/usb/core/hcd.c: struct phy *phy = > phy_get(hcd->self.controller, "usb"); > > I'm unsure which device pointer we want here, but I suspect this also > needs to be the one that has the device node in order to make the lookup > of the phy structure by device node work right. Can you clarify how > this works today? sounds correct to me. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Tue, Sep 06, 2016 at 12:38:29PM +0200, Arnd Bergmann wrote: > On Tuesday, September 6, 2016 2:35:29 PM CEST Peter Chen wrote: > > On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote: > > > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: > > > > > > > Most of these are probably never used with any nonstandard > > > DMA settings (IOMMU, cache coherency, offset, ...). > > > > > > One thing we could possibly do is to go through these and > > > replace the hardcoded dma mask setup with of_dma_configure() > > > in all cases in which we actually use DT for probing, which > > > should cover the interesting cases. > > > > > > > One case I am going to work is to let USB chipidea driver support iommu, > > the chipidea core device is no of_node, and created by > > platform_add_device on the runtime. Using of_dma_configure with parent > > of_node is a solution from my point, like [1]. > > > > https://lkml.org/lkml/2016/2/22/7 > > Right, that should make it work with iommu as well. However, it does > not solve the other issue I mentioned above, with boards that have > USB devices hardwired to a chipidea host controller that need > configuration from DT. For that, we still need to come up with another > way to associate the DT hierarchy in the host bridge node with > the Linux platform_device. > Why? The DMA configuration is for host controller, not for USB device. No matter there is hardwired or hotplug devices, the DMA configuration for host controller are both inherited from glue layer platform devices, current implementation is at function ci_hdrc_add_device, drivers/usb/chipidea/core.c. -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote: > Hi, > > Arnd Bergmann writes: > > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote: > >> > >> this only solves the problem for DT devices. Legacy devices and > >> PCI-based systems will still suffer from the same problem. At least for > >> dwc3, I will only be taking patches that solve the problem for all > >> users, not a subset of them. > > > > I don't think legacy devices are a worry, because they wouldn't > > have this problem. For the PCI case, you are right that it cannot > > work, in particular for machines that have complex IOMMU setup. > > > > Some architectures (at least arm64 and sparc) check the bus_type of > > a device in order to find the correct set of dma_map_ops for that > > device, so there is no real way to handle this as long as you > > pass a platform_device into an API that expects a pci_device. > > Then I guess we're left with adding a "struct device *dma_dev" to struct > dwc3 and trying to figure out if we should use parent or self. Does > anybody see any problems with that? I think we actually need the device pointer in the usb_hcd structure, so it can be passed in these API calls from the USB core drivers/usb/core/buffer.c: return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags); drivers/usb/core/buffer.c: dma_free_coherent(hcd->self.controller, size, addr, dma); drivers/usb/core/buffer.c: (!hcd->self.controller->dma_mask && drivers/usb/core/buffer.c: hcd->pool[i] = dma_pool_create(name, hcd->self.controller, drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single( drivers/usb/core/hcd.c: if (dma_mapping_error(hcd->self.controller, drivers/usb/core/hcd.c: n = dma_map_sg( drivers/usb/core/hcd.c: urb->transfer_dma = dma_map_page( drivers/usb/core/hcd.c: if (dma_mapping_error(hcd->self.controller, drivers/usb/core/hcd.c: urb->transfer_dma = dma_map_single( drivers/usb/core/hcd.c: if (dma_mapping_error(hcd->self.controller, drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller, drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents, drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller, drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller, drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents, as these are all called on behalf of the host controller node. Looking for more instances of hcd->self.controller, I find this instance: drivers/usb/core/hcd.c: struct usb_phy *phy = usb_get_phy_dev(hcd->self.controller, 0); drivers/usb/core/hcd.c: struct phy *phy = phy_get(hcd->self.controller, "usb"); I'm unsure which device pointer we want here, but I suspect this also needs to be the one that has the device node in order to make the lookup of the phy structure by device node work right. Can you clarify how this works today? We probably also need to add the of_node of the host controller device to struct usb_hcd in order to make usb_of_get_child_node() work in the case where the hcd itself is not device that is listed in DT. It might be a good idea to use 'struct fwnode_handle' for that, so we can in the future also allow ACPI platforms to specify > Note, we would NOT be passing device pointers are platform_data, we > would have dwc3.ko figure out if it should use self or its parent device > for dma. Ok, sounds good. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote: >> >> this only solves the problem for DT devices. Legacy devices and >> PCI-based systems will still suffer from the same problem. At least for >> dwc3, I will only be taking patches that solve the problem for all >> users, not a subset of them. > > I don't think legacy devices are a worry, because they wouldn't > have this problem. For the PCI case, you are right that it cannot > work, in particular for machines that have complex IOMMU setup. > > Some architectures (at least arm64 and sparc) check the bus_type of > a device in order to find the correct set of dma_map_ops for that > device, so there is no real way to handle this as long as you > pass a platform_device into an API that expects a pci_device. Then I guess we're left with adding a "struct device *dma_dev" to struct dwc3 and trying to figure out if we should use parent or self. Does anybody see any problems with that? Note, we would NOT be passing device pointers are platform_data, we would have dwc3.ko figure out if it should use self or its parent device for dma. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote: > > this only solves the problem for DT devices. Legacy devices and > PCI-based systems will still suffer from the same problem. At least for > dwc3, I will only be taking patches that solve the problem for all > users, not a subset of them. I don't think legacy devices are a worry, because they wouldn't have this problem. For the PCI case, you are right that it cannot work, in particular for machines that have complex IOMMU setup. Some architectures (at least arm64 and sparc) check the bus_type of a device in order to find the correct set of dma_map_ops for that device, so there is no real way to handle this as long as you pass a platform_device into an API that expects a pci_device. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Tuesday, September 6, 2016 2:35:29 PM CEST Peter Chen wrote: > On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote: > > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: > > > > Most of these are probably never used with any nonstandard > > DMA settings (IOMMU, cache coherency, offset, ...). > > > > One thing we could possibly do is to go through these and > > replace the hardcoded dma mask setup with of_dma_configure() > > in all cases in which we actually use DT for probing, which > > should cover the interesting cases. > > > > One case I am going to work is to let USB chipidea driver support iommu, > the chipidea core device is no of_node, and created by > platform_add_device on the runtime. Using of_dma_configure with parent > of_node is a solution from my point, like [1]. > > https://lkml.org/lkml/2016/2/22/7 Right, that should make it work with iommu as well. However, it does not solve the other issue I mentioned above, with boards that have USB devices hardwired to a chipidea host controller that need configuration from DT. For that, we still need to come up with another way to associate the DT hierarchy in the host bridge node with the Linux platform_device. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Peter Chen writes: > On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote: >> On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: >> > >> > Can we use the firmware or bootloader information to provide the >> > default dma-mapping attributes for devices that doesn't have an >> > of_node pointer or ACPI data? This will at least restore what we had >> > previously provided . I'm concerned that changing all the drivers >> > that are creating child device will be a big effort. Like I mentioned >> > in another thread, there are many instances of platform_device_add() >> > under the drivers/ directory. >> >> Fortunately, there are not too many drivers that call platform_device_add >> *and* try to set up a dma mask for the child device: >> >> git grep -wl dma_mask drivers | xargs grep -wl >> 'platform_device_\(add\|register\)' >> >> drivers/base/platform.c >> drivers/bcma/main.c >> drivers/eisa/virtual_root.c >> drivers/mfd/mfd-core.c >> drivers/mfd/omap-usb-host.c >> drivers/misc/mic/card/mic_x100.c >> drivers/platform/goldfish/pdev_bus.c >> drivers/ssb/main.c >> drivers/usb/chipidea/core.c >> drivers/usb/dwc3/dwc3-exynos.c >> drivers/usb/dwc3/host.c >> drivers/usb/gadget/udc/bdc/bdc_pci.c >> drivers/usb/host/bcma-hcd.c >> drivers/usb/host/fsl-mph-dr-of.c >> drivers/usb/host/ssb-hcd.c >> drivers/usb/misc/ftdi-elan.c >> drivers/usb/musb/blackfin.c >> drivers/usb/musb/musb_dsps.c >> drivers/usb/musb/omap2430.c >> drivers/usb/musb/ux500.c >> >> Most of these are probably never used with any nonstandard >> DMA settings (IOMMU, cache coherency, offset, ...). >> >> One thing we could possibly do is to go through these and >> replace the hardcoded dma mask setup with of_dma_configure() >> in all cases in which we actually use DT for probing, which >> should cover the interesting cases. >> > > One case I am going to work is to let USB chipidea driver support iommu, > the chipidea core device is no of_node, and created by > platform_add_device on the runtime. Using of_dma_configure with parent > of_node is a solution from my point, like [1]. > > https://lkml.org/lkml/2016/2/22/7 this only solves the problem for DT devices. Legacy devices and PCI-based systems will still suffer from the same problem. At least for dwc3, I will only be taking patches that solve the problem for all users, not a subset of them. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote: > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: > > > > Can we use the firmware or bootloader information to provide the > > default dma-mapping attributes for devices that doesn't have an > > of_node pointer or ACPI data? This will at least restore what we had > > previously provided . I'm concerned that changing all the drivers > > that are creating child device will be a big effort. Like I mentioned > > in another thread, there are many instances of platform_device_add() > > under the drivers/ directory. > > Fortunately, there are not too many drivers that call platform_device_add > *and* try to set up a dma mask for the child device: > > git grep -wl dma_mask drivers | xargs grep -wl > 'platform_device_\(add\|register\)' > > drivers/base/platform.c > drivers/bcma/main.c > drivers/eisa/virtual_root.c > drivers/mfd/mfd-core.c > drivers/mfd/omap-usb-host.c > drivers/misc/mic/card/mic_x100.c > drivers/platform/goldfish/pdev_bus.c > drivers/ssb/main.c > drivers/usb/chipidea/core.c > drivers/usb/dwc3/dwc3-exynos.c > drivers/usb/dwc3/host.c > drivers/usb/gadget/udc/bdc/bdc_pci.c > drivers/usb/host/bcma-hcd.c > drivers/usb/host/fsl-mph-dr-of.c > drivers/usb/host/ssb-hcd.c > drivers/usb/misc/ftdi-elan.c > drivers/usb/musb/blackfin.c > drivers/usb/musb/musb_dsps.c > drivers/usb/musb/omap2430.c > drivers/usb/musb/ux500.c > > Most of these are probably never used with any nonstandard > DMA settings (IOMMU, cache coherency, offset, ...). > > One thing we could possibly do is to go through these and > replace the hardcoded dma mask setup with of_dma_configure() > in all cases in which we actually use DT for probing, which > should cover the interesting cases. > One case I am going to work is to let USB chipidea driver support iommu, the chipidea core device is no of_node, and created by platform_add_device on the runtime. Using of_dma_configure with parent of_node is a solution from my point, like [1]. https://lkml.org/lkml/2016/2/22/7 -- Best Regards, Peter Chen
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: > > Can we use the firmware or bootloader information to provide the > default dma-mapping attributes for devices that doesn't have an > of_node pointer or ACPI data? This will at least restore what we had > previously provided . I'm concerned that changing all the drivers > that are creating child device will be a big effort. Like I mentioned > in another thread, there are many instances of platform_device_add() > under the drivers/ directory. Fortunately, there are not too many drivers that call platform_device_add *and* try to set up a dma mask for the child device: git grep -wl dma_mask drivers | xargs grep -wl 'platform_device_\(add\|register\)' drivers/base/platform.c drivers/bcma/main.c drivers/eisa/virtual_root.c drivers/mfd/mfd-core.c drivers/mfd/omap-usb-host.c drivers/misc/mic/card/mic_x100.c drivers/platform/goldfish/pdev_bus.c drivers/ssb/main.c drivers/usb/chipidea/core.c drivers/usb/dwc3/dwc3-exynos.c drivers/usb/dwc3/host.c drivers/usb/gadget/udc/bdc/bdc_pci.c drivers/usb/host/bcma-hcd.c drivers/usb/host/fsl-mph-dr-of.c drivers/usb/host/ssb-hcd.c drivers/usb/misc/ftdi-elan.c drivers/usb/musb/blackfin.c drivers/usb/musb/musb_dsps.c drivers/usb/musb/omap2430.c drivers/usb/musb/ux500.c Most of these are probably never used with any nonstandard DMA settings (IOMMU, cache coherency, offset, ...). One thing we could possibly do is to go through these and replace the hardcoded dma mask setup with of_dma_configure() in all cases in which we actually use DT for probing, which should cover the interesting cases. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Fri, Sep 2, 2016 at 5:43 AM, Arnd Bergmann wrote: > On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: >> >> Hi Felipe and Arnd, >> >> It has been a while since the last response to this discussion, but we >> haven't reached an agreement yet! Can we get to a conclusion on if it >> is valid to create child platform device for abstraction purpose? If >> yes, can this child device do DMA by itself? > > I'd say it's no problem for a driver to create child devices in order > to represent different aspects of a device, but you should not rely on > those devices working when used with the dma-mapping interfaces. > > This used to be simpler back when we could configure the kernel for > only one SoC platform at a time, and the platforms could provide their > own overrides for the dma-mapping interfaces. These days, we rely on > firmware or bootloader to describe various aspects of how DMA is done, > so you can't assume that passing a device without an of_node pointer > or ACPI data into those functions will do the right thing. Can we use the firmware or bootloader information to provide the default dma-mapping attributes for devices that doesn't have an of_node pointer or ACPI data? This will at least restore what we had previously provided . I'm concerned that changing all the drivers that are creating child device will be a big effort. Like I mentioned in another thread, there are many instances of platform_device_add() under the drivers/ directory. - Leo
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 09/02/2016 02:08 PM, Felipe Balbi wrote: > > Hi, > > Russell King - ARM Linux writes: >> On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: >>> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: Hi Felipe and Arnd, It has been a while since the last response to this discussion, but we haven't reached an agreement yet! Can we get to a conclusion on if it is valid to create child platform device for abstraction purpose? If yes, can this child device do DMA by itself? >>> >>> I'd say it's no problem for a driver to create child devices in order >>> to represent different aspects of a device, but you should not rely on >>> those devices working when used with the dma-mapping interfaces. >> >> That's absolutely right. Consider the USB model - only the USB host >> controller can perform DMA, not the USB devices themselves. All DMA >> mappings need to be mapped using the USB host controller device struct >> not the USB device struct. >> >> The same _should_ be true everywhere else: the struct device representing >> the device performing DMA must be the one used to map the transfer. > > How do we fix dwc3 in dual-role, then? > > Peripheral-side dwc3 is easy, we just require a glue-layer to be present > and use dwc3.ko's parent device (which will be the PCI device or OF > device). But for host side dwc3, the problem is slightly more complex > because we're using xhci-plat.ko by just instantiating a xhci-platform > device so xhci-plat can probe. > > xhci core has no means to know if its own device or the parent of its > parent should be used for DMA. Any ideas? > Wouldn't be possible to use dma_mask for such purposes? Like, case 1: dwc3-omap (dma_mask=X) -> dwc3 (dma_mask = NULL) -> xhci-plat (NULL) and then it might be possible to find proper parent by traversing DD hierarchy. or : dwc3 (dma_mask = X) -> xhci-plat (NULL) or : xhci-plat (dma_mask = X) of course, it might be needed to skip DMA configuration for devices which parents have been configured for dma already (easy for xhci-plat, but can be not easy for dwc3). just thinking.. Also, I'd like to note that problem become more complex when scsi layer is used on top USB ;( -- regards, -grygorii
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Friday, September 2, 2016 10:21:23 AM CEST Alan Stern wrote: > On Fri, 2 Sep 2016, Felipe Balbi wrote: > > > Hi, > > > > Russell King - ARM Linux writes: > > > On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: > > >> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: > > >> > > > >> > Hi Felipe and Arnd, > > >> > > > >> > It has been a while since the last response to this discussion, but we > > >> > haven't reached an agreement yet! Can we get to a conclusion on if it > > >> > is valid to create child platform device for abstraction purpose? If > > >> > yes, can this child device do DMA by itself? > > >> > > >> I'd say it's no problem for a driver to create child devices in order > > >> to represent different aspects of a device, but you should not rely on > > >> those devices working when used with the dma-mapping interfaces. > > > > > > That's absolutely right. Consider the USB model - only the USB host > > > controller can perform DMA, not the USB devices themselves. All DMA > > > mappings need to be mapped using the USB host controller device struct > > > not the USB device struct. > > > > > > The same _should_ be true everywhere else: the struct device representing > > > the device performing DMA must be the one used to map the transfer. > > > > How do we fix dwc3 in dual-role, then? > > > > Peripheral-side dwc3 is easy, we just require a glue-layer to be present > > and use dwc3.ko's parent device (which will be the PCI device or OF > > device). But for host side dwc3, the problem is slightly more complex > > because we're using xhci-plat.ko by just instantiating a xhci-platform > > device so xhci-plat can probe. > > > > xhci core has no means to know if its own device or the parent of its > > parent should be used for DMA. Any ideas? > > In theory, you can store a flag somewhere in the platform device, > something that would tell xhci-hcd that it has to use the parent's > parent for DMA purposes. > > I know it would be somewhat of a hack, but ought to work. Speaking of that flag, I suppose we need the same logic to know where to look for USB devices attached to a dwc3 host when we need to describe them in DT. By default we look for child device nodes under the node of the HCD device node, but that would be wrong here too. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Fri, 2 Sep 2016, Felipe Balbi wrote: > Hi, > > Russell King - ARM Linux writes: > > On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: > >> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: > >> > > >> > Hi Felipe and Arnd, > >> > > >> > It has been a while since the last response to this discussion, but we > >> > haven't reached an agreement yet! Can we get to a conclusion on if it > >> > is valid to create child platform device for abstraction purpose? If > >> > yes, can this child device do DMA by itself? > >> > >> I'd say it's no problem for a driver to create child devices in order > >> to represent different aspects of a device, but you should not rely on > >> those devices working when used with the dma-mapping interfaces. > > > > That's absolutely right. Consider the USB model - only the USB host > > controller can perform DMA, not the USB devices themselves. All DMA > > mappings need to be mapped using the USB host controller device struct > > not the USB device struct. > > > > The same _should_ be true everywhere else: the struct device representing > > the device performing DMA must be the one used to map the transfer. > > How do we fix dwc3 in dual-role, then? > > Peripheral-side dwc3 is easy, we just require a glue-layer to be present > and use dwc3.ko's parent device (which will be the PCI device or OF > device). But for host side dwc3, the problem is slightly more complex > because we're using xhci-plat.ko by just instantiating a xhci-platform > device so xhci-plat can probe. > > xhci core has no means to know if its own device or the parent of its > parent should be used for DMA. Any ideas? In theory, you can store a flag somewhere in the platform device, something that would tell xhci-hcd that it has to use the parent's parent for DMA purposes. I know it would be somewhat of a hack, but ought to work. Alan Stern
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Felipe Balbi writes: > Hi, > > Russell King - ARM Linux writes: >> On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: >>> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: >>> > >>> > Hi Felipe and Arnd, >>> > >>> > It has been a while since the last response to this discussion, but we >>> > haven't reached an agreement yet! Can we get to a conclusion on if it >>> > is valid to create child platform device for abstraction purpose? If >>> > yes, can this child device do DMA by itself? >>> >>> I'd say it's no problem for a driver to create child devices in order >>> to represent different aspects of a device, but you should not rely on >>> those devices working when used with the dma-mapping interfaces. >> >> That's absolutely right. Consider the USB model - only the USB host >> controller can perform DMA, not the USB devices themselves. All DMA >> mappings need to be mapped using the USB host controller device struct >> not the USB device struct. >> >> The same _should_ be true everywhere else: the struct device representing >> the device performing DMA must be the one used to map the transfer. > > How do we fix dwc3 in dual-role, then? > > Peripheral-side dwc3 is easy, we just require a glue-layer to be present > and use dwc3.ko's parent device (which will be the PCI device or OF > device). But for host side dwc3, the problem is slightly more complex > because we're using xhci-plat.ko by just instantiating a xhci-platform > device so xhci-plat can probe. > > xhci core has no means to know if its own device or the parent of its > parent should be used for DMA. Any ideas? another thing to consider is that dwc3 only works on omap because DT defaults to 32-bit DMA mask for anything described in DT that doesn't provide dma-ranges. Isn't that somewhat odd as well? Based on your reply, Russell, dwc3-omap should be the DMA device, but dwc3 works just as well because of the whole 32-bit default. -- balbi
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Friday, September 2, 2016 12:55:33 PM CEST Robin Murphy wrote: > > Huh? There's only no DMA description in DT if the device can be assumed > to be happy with the defaults. Anything else should be using > "dma-ranges", "dma-coherent", etc. to describe non-default integration > aspects. For devices with an inherent fixed addressing capability !=32 > bits, then it's down to the driver to call dma_set_mask() appropriately > to override the default 32-bit mask (which is not unique to OF-probed > devices either). The iommu configuration would be the main other one worth mentioning. Note that there is a known bug with dma_set_mask(), which always succeeds at the moment, even if the dma-ranges limit the possible addresses in a way that should fail. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Robin Murphy writes: It has been a while since the last response to this discussion, but we haven't reached an agreement yet! Can we get to a conclusion on if it is valid to create child platform device for abstraction purpose? If yes, can this child device do DMA by itself? >>> >>> I'd say it's no problem for a driver to create child devices in order >>> to represent different aspects of a device, but you should not rely on >>> those devices working when used with the dma-mapping interfaces. >> >> heh, that looks like an excuse to me :-) >> >> This will always be a problem for e.g. MFD, for example. Are you saying >> MFD child-devices shouldn't be allowed to do DMA? It becomes silly when >> you read it that way, right? >> >>> This used to be simpler back when we could configure the kernel for >>> only one SoC platform at a time, and the platforms could provide their >>> own overrides for the dma-mapping interfaces. These days, we rely on >> >> right, so we have a very old regression that just took a complex driver >> such as dwc3 to trigger ;-) >> >>> firmware or bootloader to describe various aspects of how DMA is done, >> >> there's no DMA description in DT. Every OF device gets the same 32-bit >> DMA mask and that is, itself, wrong for several devices. > > Huh? There's only no DMA description in DT if the device can be assumed > to be happy with the defaults. Anything else should be using > "dma-ranges", "dma-coherent", etc. to describe non-default integration heh, guilty as charged. I never noticed we had dma-ranges or dma-coherent. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 02/09/16 11:53, Felipe Balbi wrote: > > Hi, > > Arnd Bergmann writes: >> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: >>> >>> Hi Felipe and Arnd, >>> >>> It has been a while since the last response to this discussion, but we >>> haven't reached an agreement yet! Can we get to a conclusion on if it >>> is valid to create child platform device for abstraction purpose? If >>> yes, can this child device do DMA by itself? >> >> I'd say it's no problem for a driver to create child devices in order >> to represent different aspects of a device, but you should not rely on >> those devices working when used with the dma-mapping interfaces. > > heh, that looks like an excuse to me :-) > > This will always be a problem for e.g. MFD, for example. Are you saying > MFD child-devices shouldn't be allowed to do DMA? It becomes silly when > you read it that way, right? > >> This used to be simpler back when we could configure the kernel for >> only one SoC platform at a time, and the platforms could provide their >> own overrides for the dma-mapping interfaces. These days, we rely on > > right, so we have a very old regression that just took a complex driver > such as dwc3 to trigger ;-) > >> firmware or bootloader to describe various aspects of how DMA is done, > > there's no DMA description in DT. Every OF device gets the same 32-bit > DMA mask and that is, itself, wrong for several devices. Huh? There's only no DMA description in DT if the device can be assumed to be happy with the defaults. Anything else should be using "dma-ranges", "dma-coherent", etc. to describe non-default integration aspects. For devices with an inherent fixed addressing capability !=32 bits, then it's down to the driver to call dma_set_mask() appropriately to override the default 32-bit mask (which is not unique to OF-probed devices either). Sure, it's by no means a perfect API, but you're railing against untruths here. Robin. >> so you can't assume that passing a device without an of_node pointer >> or ACPI data into those functions will do the right thing. > > That's not the problem, however. We can very easily pass along > ACPI_COMPANION() to any platform_device we want, but that's not enough > because DMA-related bits are passed along with archdata; but archdata > isn't generic in any way. Some arches (like x86) _do_ use it for DMA, > but some don't. > > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Russell King - ARM Linux writes: > On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: >> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: >> > >> > Hi Felipe and Arnd, >> > >> > It has been a while since the last response to this discussion, but we >> > haven't reached an agreement yet! Can we get to a conclusion on if it >> > is valid to create child platform device for abstraction purpose? If >> > yes, can this child device do DMA by itself? >> >> I'd say it's no problem for a driver to create child devices in order >> to represent different aspects of a device, but you should not rely on >> those devices working when used with the dma-mapping interfaces. > > That's absolutely right. Consider the USB model - only the USB host > controller can perform DMA, not the USB devices themselves. All DMA > mappings need to be mapped using the USB host controller device struct > not the USB device struct. > > The same _should_ be true everywhere else: the struct device representing > the device performing DMA must be the one used to map the transfer. How do we fix dwc3 in dual-role, then? Peripheral-side dwc3 is easy, we just require a glue-layer to be present and use dwc3.ko's parent device (which will be the PCI device or OF device). But for host side dwc3, the problem is slightly more complex because we're using xhci-plat.ko by just instantiating a xhci-platform device so xhci-plat can probe. xhci core has no means to know if its own device or the parent of its parent should be used for DMA. Any ideas? -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: >> >> Hi Felipe and Arnd, >> >> It has been a while since the last response to this discussion, but we >> haven't reached an agreement yet! Can we get to a conclusion on if it >> is valid to create child platform device for abstraction purpose? If >> yes, can this child device do DMA by itself? > > I'd say it's no problem for a driver to create child devices in order > to represent different aspects of a device, but you should not rely on > those devices working when used with the dma-mapping interfaces. heh, that looks like an excuse to me :-) This will always be a problem for e.g. MFD, for example. Are you saying MFD child-devices shouldn't be allowed to do DMA? It becomes silly when you read it that way, right? > This used to be simpler back when we could configure the kernel for > only one SoC platform at a time, and the platforms could provide their > own overrides for the dma-mapping interfaces. These days, we rely on right, so we have a very old regression that just took a complex driver such as dwc3 to trigger ;-) > firmware or bootloader to describe various aspects of how DMA is done, there's no DMA description in DT. Every OF device gets the same 32-bit DMA mask and that is, itself, wrong for several devices. > so you can't assume that passing a device without an of_node pointer > or ACPI data into those functions will do the right thing. That's not the problem, however. We can very easily pass along ACPI_COMPANION() to any platform_device we want, but that's not enough because DMA-related bits are passed along with archdata; but archdata isn't generic in any way. Some arches (like x86) _do_ use it for DMA, but some don't. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote: > On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: > > > > Hi Felipe and Arnd, > > > > It has been a while since the last response to this discussion, but we > > haven't reached an agreement yet! Can we get to a conclusion on if it > > is valid to create child platform device for abstraction purpose? If > > yes, can this child device do DMA by itself? > > I'd say it's no problem for a driver to create child devices in order > to represent different aspects of a device, but you should not rely on > those devices working when used with the dma-mapping interfaces. That's absolutely right. Consider the USB model - only the USB host controller can perform DMA, not the USB devices themselves. All DMA mappings need to be mapped using the USB host controller device struct not the USB device struct. The same _should_ be true everywhere else: the struct device representing the device performing DMA must be the one used to map the transfer. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote: > > Hi Felipe and Arnd, > > It has been a while since the last response to this discussion, but we > haven't reached an agreement yet! Can we get to a conclusion on if it > is valid to create child platform device for abstraction purpose? If > yes, can this child device do DMA by itself? I'd say it's no problem for a driver to create child devices in order to represent different aspects of a device, but you should not rely on those devices working when used with the dma-mapping interfaces. This used to be simpler back when we could configure the kernel for only one SoC platform at a time, and the platforms could provide their own overrides for the dma-mapping interfaces. These days, we rely on firmware or bootloader to describe various aspects of how DMA is done, so you can't assume that passing a device without an of_node pointer or ACPI data into those functions will do the right thing. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thu, Apr 28, 2016 at 9:27 AM, Felipe Balbi wrote: > > Hi, > > Arnd Bergmann writes: >> On Thursday 28 April 2016 15:16:12 Russell King - ARM Linux wrote: >>> On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote: >>> > >>> > Hi, >>> > >>> > Arnd Bergmann writes: >>> > >pointer and pass that in platform_data. This is really easy, it's >>> > >>> > Sorry but passing a struct device pointer in platform_data is >>> > ridiculous. Not to mention that, as I said before, we can't assume which >>> > device to pass to xhci_plat in the first place. It might be dwc->dev and >>> > it might be dwc->dev->parent. >>> >>> +1. Passing an unref-counted struct device through platform data is >>> totally mad, Arnd you're off your rocker if you think that's a good >>> idea. What's more is that there's no way to properly refcount the >>> thing. >> >> It's the parent device (or NULL), there is no way it can ever go away as >> it's already refcounted through the device subsystem by the creation >> of the child device. > > you're assuming that based on what we have today. We could get into a > situation where we need to use a completely unrelated device and the > problem exists again. > >> I do realize that it's a hack, but the idea is to get rid of that >> as soon as possibly by fixing the way the xhci device is probe so >> we no longer need to fake a platform_device as the child here and >> can just use the device itself. > > okay, let me try to be extra clear here: > > We will *not* remove the extra platform_device because it actually > *does* exist and helps me hide/abstract a bunch of details and make > assumptions about order of certain events. We have already gone through > that in the past when I explained why I wrote dwc3 the way it is; if you > need a refresher, there are mailing list archives for that. > > Moreover, this same problem exists for anything under drivers/mfd. It > just so happens that they're usually some i2c or spi device which don't > do DMA by themselves. Hi Felipe and Arnd, It has been a while since the last response to this discussion, but we haven't reached an agreement yet! Can we get to a conclusion on if it is valid to create child platform device for abstraction purpose? If yes, can this child device do DMA by itself? - Leo
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Thursday 28 April 2016 15:16:12 Russell King - ARM Linux wrote: >> On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote: >> > >> > Hi, >> > >> > Arnd Bergmann writes: >> > >pointer and pass that in platform_data. This is really easy, it's >> > >> > Sorry but passing a struct device pointer in platform_data is >> > ridiculous. Not to mention that, as I said before, we can't assume which >> > device to pass to xhci_plat in the first place. It might be dwc->dev and >> > it might be dwc->dev->parent. >> >> +1. Passing an unref-counted struct device through platform data is >> totally mad, Arnd you're off your rocker if you think that's a good >> idea. What's more is that there's no way to properly refcount the >> thing. > > It's the parent device (or NULL), there is no way it can ever go away as > it's already refcounted through the device subsystem by the creation > of the child device. you're assuming that based on what we have today. We could get into a situation where we need to use a completely unrelated device and the problem exists again. > I do realize that it's a hack, but the idea is to get rid of that > as soon as possibly by fixing the way the xhci device is probe so > we no longer need to fake a platform_device as the child here and > can just use the device itself. okay, let me try to be extra clear here: We will *not* remove the extra platform_device because it actually *does* exist and helps me hide/abstract a bunch of details and make assumptions about order of certain events. We have already gone through that in the past when I explained why I wrote dwc3 the way it is; if you need a refresher, there are mailing list archives for that. Moreover, this same problem exists for anything under drivers/mfd. It just so happens that they're usually some i2c or spi device which don't do DMA by themselves. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thursday 28 April 2016 15:16:12 Russell King - ARM Linux wrote: > On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote: > > > > Hi, > > > > Arnd Bergmann writes: > > >pointer and pass that in platform_data. This is really easy, it's > > > > Sorry but passing a struct device pointer in platform_data is > > ridiculous. Not to mention that, as I said before, we can't assume which > > device to pass to xhci_plat in the first place. It might be dwc->dev and > > it might be dwc->dev->parent. > > +1. Passing an unref-counted struct device through platform data is > totally mad, Arnd you're off your rocker if you think that's a good > idea. What's more is that there's no way to properly refcount the > thing. It's the parent device (or NULL), there is no way it can ever go away as it's already refcounted through the device subsystem by the creation of the child device. I do realize that it's a hack, but the idea is to get rid of that as soon as possibly by fixing the way the xhci device is probe so we no longer need to fake a platform_device as the child here and can just use the device itself. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote: > > Hi, > > Arnd Bergmann writes: > >pointer and pass that in platform_data. This is really easy, it's > > Sorry but passing a struct device pointer in platform_data is > ridiculous. Not to mention that, as I said before, we can't assume which > device to pass to xhci_plat in the first place. It might be dwc->dev and > it might be dwc->dev->parent. +1. Passing an unref-counted struct device through platform data is totally mad, Arnd you're off your rocker if you think that's a good idea. What's more is that there's no way to properly refcount the thing. -- 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.
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote: >> >> On Wed, 27 Apr 2016, Arnd Bergmann wrote: >> >> >> >> > I've looked at the usb HCD code now and see this: >> >> > >> >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, >> >> > struct device *dev, const char *bus_name, >> >> > struct usb_hcd *primary_hcd) >> >> > { >> >> > ... >> >> > hcd->self.controller = dev; >> >> > hcd->self.uses_dma = (dev->dma_mask != NULL); >> >> > ... >> >> > } >> >> > >> >> > What I think we need to do here is ensure that the device that gets >> >> > passed here and assigned to hcd->self.controller is the actual DMA >> >> > master device, i.e. the pci_device or platform_device that was created >> >> > from outside of the xhci stack. This is after all the pointer that >> >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... >> >> > functions. >> >> >> >> It would be better to add a new field, since self.controller is also >> >> used for lots of other purposes. Something like hcd->self.dma_dev. >> > >> > Ok, fair enough. I only took a brief look and all uses I found were >> > either for the DMA mapping API or some printk logging. >> >> I have a feeling you guys are not considering how the patch to implement >> this will look like. >> >> How are you expecting dwc3 to pass a pointer to the DMA device from >> dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible > > Not any worse than it already is really. It already uses platform_data > for the exact case that is causing the problem here. there's no use of platform_data for passing around DMA configuration. By platform_data I really mean platform_device_add_data(). >> Also, remember that the DMA device for dwc3 is not always >> dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting >> us to figure that one out ? > > Do you have an example for this? The ones I found here either > create the dwc3 device from PCI or from a platform glue driver. arch/arm64/boot/dts/xilinx/zynqmp.dtsi >> I still think dma_inherit() (or something along those lines) is >> necessary. Specially when you consider that, as I said previously, >> that's pretty much what of_dma_configure() does. > > As I said, this is not an API that can work in general, because > it makes the assumption that everything related to DMA in a > device can be set in that device itself. > > The simplest case where this does not work is a PCI device behind > an IOMMU: when you call dma_map_single() or dma_alloc_coherent(), > the dma_map_ops implementation for the IOMMU has to look at the > PCI device to find out the association with an IOMMU context, > and on most architectures you cannot bind an IOMMU context to > a platform device at all. no, it relies on dev->archdata for IOMMU. In fact, the first "patch" (more of a hack) I wrote to fix IOMMU with dwc3 on Intel platforms was to literally memcpy() pci's archdata to dwc3->dev and it worked just fine with and without IOMMU enabled. >> Anyway, I'd really like to see a patch implementing this >> hcd->self.dma_dev logic. Consider all the duplication with this >> approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its >> own. Will that be passed to dwc3 as platform_data from glue layer ? What >> about platforms which don't even use a glue layer ? > > Let's separate the three problems here. > > a) dwc3 creating a "xhci-hcd" platform_device that is not connected >to any proper bus. We can work around that by adding the "self.dma_dev" platform_bus_type *is* a proper bus. >pointer and pass that in platform_data. This is really easy, it's Sorry but passing a struct device pointer in platform_data is ridiculous. Not to mention that, as I said before, we can't assume which device to pass to xhci_plat in the first place. It might be dwc->dev and it might be dwc->dev->parent. >actually less code (and less iffy) than the current implementation of >copying the parent dma mask. >In the long run, this could be solved by doing away with the extra >platform_device, by calling a variant of xhci_probe() from >xhci_plat_probe() like we do for the normal *HCI drivers. no, that's not something I'll do for dwc3. We have had this talk before and I'm not giving up the benefits of splitting things to separate devices. > b) dwc3-pci creating a "dwc3" platform_device under the covers. This it's not under the covers at all. It's pretty similar to what MFD drivers do. It's just not wrapped in a nice API because there's no need for that. >is pretty much the exact same problem as a) on another layer. In >the short run, we can pass the device pointer as part of >struct dwc3_platform_data (dwc3-pci is the only such user anway), It's incredible that you'd even suggest this at all. Did we not have a big trou
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote: > Arnd Bergmann writes: > > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote: > >> On Wed, 27 Apr 2016, Arnd Bergmann wrote: > >> > >> > I've looked at the usb HCD code now and see this: > >> > > >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, > >> > struct device *dev, const char *bus_name, > >> > struct usb_hcd *primary_hcd) > >> > { > >> > ... > >> > hcd->self.controller = dev; > >> > hcd->self.uses_dma = (dev->dma_mask != NULL); > >> > ... > >> > } > >> > > >> > What I think we need to do here is ensure that the device that gets > >> > passed here and assigned to hcd->self.controller is the actual DMA > >> > master device, i.e. the pci_device or platform_device that was created > >> > from outside of the xhci stack. This is after all the pointer that > >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... > >> > functions. > >> > >> It would be better to add a new field, since self.controller is also > >> used for lots of other purposes. Something like hcd->self.dma_dev. > > > > Ok, fair enough. I only took a brief look and all uses I found were > > either for the DMA mapping API or some printk logging. > > I have a feeling you guys are not considering how the patch to implement > this will look like. > > How are you expecting dwc3 to pass a pointer to the DMA device from > dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible Not any worse than it already is really. It already uses platform_data for the exact case that is causing the problem here. > Also, remember that the DMA device for dwc3 is not always > dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting > us to figure that one out ? Do you have an example for this? The ones I found here either create the dwc3 device from PCI or from a platform glue driver. > I still think dma_inherit() (or something along those lines) is > necessary. Specially when you consider that, as I said previously, > that's pretty much what of_dma_configure() does. As I said, this is not an API that can work in general, because it makes the assumption that everything related to DMA in a device can be set in that device itself. The simplest case where this does not work is a PCI device behind an IOMMU: when you call dma_map_single() or dma_alloc_coherent(), the dma_map_ops implementation for the IOMMU has to look at the PCI device to find out the association with an IOMMU context, and on most architectures you cannot bind an IOMMU context to a platform device at all. > Anyway, I'd really like to see a patch implementing this > hcd->self.dma_dev logic. Consider all the duplication with this > approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its > own. Will that be passed to dwc3 as platform_data from glue layer ? What > about platforms which don't even use a glue layer ? Let's separate the three problems here. a) dwc3 creating a "xhci-hcd" platform_device that is not connected to any proper bus. We can work around that by adding the "self.dma_dev" pointer and pass that in platform_data. This is really easy, it's actually less code (and less iffy) than the current implementation of copying the parent dma mask. In the long run, this could be solved by doing away with the extra platform_device, by calling a variant of xhci_probe() from xhci_plat_probe() like we do for the normal *HCI drivers. b) dwc3-pci creating a "dwc3" platform_device under the covers. This is pretty much the exact same problem as a) on another layer. In the short run, we can pass the device pointer as part of struct dwc3_platform_data (dwc3-pci is the only such user anway), and in the long run, it should be easy enough to get rid of the extra platform device by just calling a variant of dwc3_probe, which will again simplify the driver c) some SoCs that have two separate device nodes to describe their dwc3 xhci. I don't think this is causing any additional problems, but if we want to make this behave more like other drivers in the long run or deal with machines that are missing a "dma-ranges" property in the parent node, we can kill off the of_platform_populate() hack and instead call dwc3_probe() directly from the glue drivers as in b), and have that do for_each_child_of_node() or similar to find the child node. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: >> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: >> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: >> >> > > >> >> > > I would be in favour of a dma_inherit() function as well. We could >> >> > > hack >> >> > > something up in the arch code (like below) but I would rather prefer >> >> > > an >> >> > > explicit dma_inherit() call by drivers creating such devices. >> >> > > >> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h >> >> > > b/arch/arm64/include/asm/dma-mapping.h >> >> > > index ba437f090a74..ea6fb9b0e8fa 100644 >> >> > > --- a/arch/arm64/include/asm/dma-mapping.h >> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h >> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; >> >> > > >> >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device >> >> > > *dev) >> >> > > { >> >> > > - if (dev && dev->archdata.dma_ops) >> >> > > - return dev->archdata.dma_ops; >> >> > > + while (dev) { >> >> > > + if (dev->archdata.dma_ops) >> >> > > + return dev->archdata.dma_ops; >> >> > > + dev = dev->parent; >> >> > > + } >> >> > >> >> > I think this would be a very bad idea: we don't want to have random >> >> > devices be able to perform DMA just because their parent devices >> >> > have been set up that way. >> >> >> >> I agree, it's a big hack. It would be nice to have a simpler way to do >> >> this in driver code rather than explicitly calling >> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this >> >> thread. >> > >> > I haven't followed the entire discussion, but what's wrong with passing >> > around a pointer to a 'struct device *hwdev' that represents the physical >> > device that does the DMA? >> >> that will likely create duplicated solutions in several drivers and >> it'll be a pain to maintain. There's another complication, dwc3 can be >> integrated in many different ways. See the device child-parent tree >> representations below: >> >> a) with a parent PCI device: >> >> pci_bus_type >> - dwc3-pci >>- dwc3 >> - xhci-plat >> >> b) with a parent platform_device (OF): >> >> platform_bus_type >> - dwc3-${omap,st,of-simple,exynos,keystone} >>- dwc3 >> - xhci-plat >> >> c) without a parent at all (thanks Grygorii): >> >> platform_bus_type >> - dwc3 >>- xhci-plat >> >> (a) and (b) above are the common cases. The DMA-capable device is >> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only >> having proper DMA configuration in OF platforms (because of the >> unconditional of_dma_configure() during OF device creation) and >> xhci-plat not knowing about DMA at all and hardcoding some crappy >> defaults. >> >> (c) is the uncommon case which creates some problems. In this case, dwc3 >> itself is the DMA-capable device and dwc3->dev->parent is the >> platform_bus_type itself. Now consider the problem this creates: >> >> i. the patch that I wrote [1] becomes invalid for (c), thanks to >> Grygorii for pointing this out before it was too late. >> >> ii. xhci-plat can also be described directly in DT (and is in some >> cases). This means that assuming xhci-plat's parent's parent to be the >> DMA-capable device is also an invalid assumption. >> >> iii. one might argue that for DT-based platforms *with* a glue layer >> ((b) above), OF already "copies" some sensible DMA defaults during >> device creation. > > But that is exactly the point I was trying to make: instead of copying > all the data into the xhci-plat device, just assign one pointer > inside the xhci-plat device from whoever creates it. and how do you pass that pointer to xhci-plat from another driver ? No, platform_data is not an option ;-) >> The only clean way to fix this up is with a dma_inherit()-like API which >> would allow dwc3's glue layers ((a) and (b) above) to initialize child's >> (dwc3) DMA configuration during child's creation. Something like below: >> >> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c >> index adc1e8a624cb..74b599269e2c 100644 >> --- a/drivers/usb/dwc3/dwc3-pci.c >> +++ b/drivers/usb/dwc3/dwc3-pci.c >> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, >> return -ENOMEM; >> } >> >> +dma_inherit(&dwc->dev, dev); >> + >> memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res)); >> >> res[0].start= pci_resource_start(pci, 0); >> >> that's all I'm asking for :-) dma_inherit() should, probably, be >> arch-specific to handle details like IOMMU (which today sits under >> archdata). > > It's still wrong: the archdata in the device can contain all sorts of > additional information about how to do DMA, and some of that information yes, inherit
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote: >> On Wed, 27 Apr 2016, Arnd Bergmann wrote: >> >> > I've looked at the usb HCD code now and see this: >> > >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, >> > struct device *dev, const char *bus_name, >> > struct usb_hcd *primary_hcd) >> > { >> > ... >> > hcd->self.controller = dev; >> > hcd->self.uses_dma = (dev->dma_mask != NULL); >> > ... >> > } >> > >> > What I think we need to do here is ensure that the device that gets >> > passed here and assigned to hcd->self.controller is the actual DMA >> > master device, i.e. the pci_device or platform_device that was created >> > from outside of the xhci stack. This is after all the pointer that >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... >> > functions. >> >> It would be better to add a new field, since self.controller is also >> used for lots of other purposes. Something like hcd->self.dma_dev. > > Ok, fair enough. I only took a brief look and all uses I found were > either for the DMA mapping API or some printk logging. I have a feeling you guys are not considering how the patch to implement this will look like. How are you expecting dwc3 to pass a pointer to the DMA device from dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible :-) Also, remember that the DMA device for dwc3 is not always dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting us to figure that one out ? I still think dma_inherit() (or something along those lines) is necessary. Specially when you consider that, as I said previously, that's pretty much what of_dma_configure() does. Anyway, I'd really like to see a patch implementing this hcd->self.dma_dev logic. Consider all the duplication with this approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its own. Will that be passed to dwc3 as platform_data from glue layer ? What about platforms which don't even use a glue layer ? -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday 27 April 2016 13:59:13 Alan Stern wrote: > On Wed, 27 Apr 2016, Arnd Bergmann wrote: > > > I've looked at the usb HCD code now and see this: > > > > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, > > struct device *dev, const char *bus_name, > > struct usb_hcd *primary_hcd) > > { > > ... > > hcd->self.controller = dev; > > hcd->self.uses_dma = (dev->dma_mask != NULL); > > ... > > } > > > > What I think we need to do here is ensure that the device that gets > > passed here and assigned to hcd->self.controller is the actual DMA > > master device, i.e. the pci_device or platform_device that was created > > from outside of the xhci stack. This is after all the pointer that > > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... > > functions. > > It would be better to add a new field, since self.controller is also > used for lots of other purposes. Something like hcd->self.dma_dev. > Ok, fair enough. I only took a brief look and all uses I found were either for the DMA mapping API or some printk logging. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, 27 Apr 2016, Arnd Bergmann wrote: > I've looked at the usb HCD code now and see this: > > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, > struct device *dev, const char *bus_name, > struct usb_hcd *primary_hcd) > { > ... > hcd->self.controller = dev; > hcd->self.uses_dma = (dev->dma_mask != NULL); > ... > } > > What I think we need to do here is ensure that the device that gets > passed here and assigned to hcd->self.controller is the actual DMA > master device, i.e. the pci_device or platform_device that was created > from outside of the xhci stack. This is after all the pointer that > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... > functions. It would be better to add a new field, since self.controller is also used for lots of other purposes. Something like hcd->self.dma_dev. Alan Stern
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote: > Arnd Bergmann writes: > > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: > >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: > >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: > >> > > > >> > > I would be in favour of a dma_inherit() function as well. We could hack > >> > > something up in the arch code (like below) but I would rather prefer an > >> > > explicit dma_inherit() call by drivers creating such devices. > >> > > > >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h > >> > > b/arch/arm64/include/asm/dma-mapping.h > >> > > index ba437f090a74..ea6fb9b0e8fa 100644 > >> > > --- a/arch/arm64/include/asm/dma-mapping.h > >> > > +++ b/arch/arm64/include/asm/dma-mapping.h > >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; > >> > > > >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device > >> > > *dev) > >> > > { > >> > > - if (dev && dev->archdata.dma_ops) > >> > > - return dev->archdata.dma_ops; > >> > > + while (dev) { > >> > > + if (dev->archdata.dma_ops) > >> > > + return dev->archdata.dma_ops; > >> > > + dev = dev->parent; > >> > > + } > >> > > >> > I think this would be a very bad idea: we don't want to have random > >> > devices be able to perform DMA just because their parent devices > >> > have been set up that way. > >> > >> I agree, it's a big hack. It would be nice to have a simpler way to do > >> this in driver code rather than explicitly calling > >> of_dma_configure/arch_setup_dma_ops as per the original patch in this > >> thread. > > > > I haven't followed the entire discussion, but what's wrong with passing > > around a pointer to a 'struct device *hwdev' that represents the physical > > device that does the DMA? > > that will likely create duplicated solutions in several drivers and > it'll be a pain to maintain. There's another complication, dwc3 can be > integrated in many different ways. See the device child-parent tree > representations below: > > a) with a parent PCI device: > > pci_bus_type > - dwc3-pci >- dwc3 > - xhci-plat > > b) with a parent platform_device (OF): > > platform_bus_type > - dwc3-${omap,st,of-simple,exynos,keystone} >- dwc3 > - xhci-plat > > c) without a parent at all (thanks Grygorii): > > platform_bus_type > - dwc3 >- xhci-plat > > (a) and (b) above are the common cases. The DMA-capable device is > clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only > having proper DMA configuration in OF platforms (because of the > unconditional of_dma_configure() during OF device creation) and > xhci-plat not knowing about DMA at all and hardcoding some crappy > defaults. > > (c) is the uncommon case which creates some problems. In this case, dwc3 > itself is the DMA-capable device and dwc3->dev->parent is the > platform_bus_type itself. Now consider the problem this creates: > > i. the patch that I wrote [1] becomes invalid for (c), thanks to > Grygorii for pointing this out before it was too late. > > ii. xhci-plat can also be described directly in DT (and is in some > cases). This means that assuming xhci-plat's parent's parent to be the > DMA-capable device is also an invalid assumption. > > iii. one might argue that for DT-based platforms *with* a glue layer > ((b) above), OF already "copies" some sensible DMA defaults during > device creation. But that is exactly the point I was trying to make: instead of copying all the data into the xhci-plat device, just assign one pointer inside the xhci-plat device from whoever creates it. > The only clean way to fix this up is with a dma_inherit()-like API which > would allow dwc3's glue layers ((a) and (b) above) to initialize child's > (dwc3) DMA configuration during child's creation. Something like below: > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c > index adc1e8a624cb..74b599269e2c 100644 > --- a/drivers/usb/dwc3/dwc3-pci.c > +++ b/drivers/usb/dwc3/dwc3-pci.c > @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, > return -ENOMEM; > } > > + dma_inherit(&dwc->dev, dev); > + > memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res)); > > res[0].start= pci_resource_start(pci, 0); > > that's all I'm asking for :-) dma_inherit() should, probably, be > arch-specific to handle details like IOMMU (which today sits under > archdata). It's still wrong: the archdata in the device can contain all sorts of additional information about how to do DMA, and some of that information is bus specific, e.g. when your dma_map_ops look like the on sparc: static inline struct dma_map_ops *get_dma_ops(struct device *dev) { #ifdef CONFIG_SPARC_LEON if (sparc_cpu_model == sparc_leon) return leon_dma_ops; #endif #if defined(CONFIG_SPARC32) && defined(
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: >> > > >> > > I would be in favour of a dma_inherit() function as well. We could hack >> > > something up in the arch code (like below) but I would rather prefer an >> > > explicit dma_inherit() call by drivers creating such devices. >> > > >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h >> > > b/arch/arm64/include/asm/dma-mapping.h >> > > index ba437f090a74..ea6fb9b0e8fa 100644 >> > > --- a/arch/arm64/include/asm/dma-mapping.h >> > > +++ b/arch/arm64/include/asm/dma-mapping.h >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; >> > > >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) >> > > { >> > > - if (dev && dev->archdata.dma_ops) >> > > - return dev->archdata.dma_ops; >> > > + while (dev) { >> > > + if (dev->archdata.dma_ops) >> > > + return dev->archdata.dma_ops; >> > > + dev = dev->parent; >> > > + } >> > >> > I think this would be a very bad idea: we don't want to have random >> > devices be able to perform DMA just because their parent devices >> > have been set up that way. >> >> I agree, it's a big hack. It would be nice to have a simpler way to do >> this in driver code rather than explicitly calling >> of_dma_configure/arch_setup_dma_ops as per the original patch in this >> thread. > > I haven't followed the entire discussion, but what's wrong with passing > around a pointer to a 'struct device *hwdev' that represents the physical > device that does the DMA? that will likely create duplicated solutions in several drivers and it'll be a pain to maintain. There's another complication, dwc3 can be integrated in many different ways. See the device child-parent tree representations below: a) with a parent PCI device: pci_bus_type - dwc3-pci - dwc3 - xhci-plat b) with a parent platform_device (OF): platform_bus_type - dwc3-${omap,st,of-simple,exynos,keystone} - dwc3 - xhci-plat c) without a parent at all (thanks Grygorii): platform_bus_type - dwc3 - xhci-plat (a) and (b) above are the common cases. The DMA-capable device is clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only having proper DMA configuration in OF platforms (because of the unconditional of_dma_configure() during OF device creation) and xhci-plat not knowing about DMA at all and hardcoding some crappy defaults. (c) is the uncommon case which creates some problems. In this case, dwc3 itself is the DMA-capable device and dwc3->dev->parent is the platform_bus_type itself. Now consider the problem this creates: i. the patch that I wrote [1] becomes invalid for (c), thanks to Grygorii for pointing this out before it was too late. ii. xhci-plat can also be described directly in DT (and is in some cases). This means that assuming xhci-plat's parent's parent to be the DMA-capable device is also an invalid assumption. iii. one might argue that for DT-based platforms *with* a glue layer ((b) above), OF already "copies" some sensible DMA defaults during device creation. PCI-based systems just don't have the luxury of creating random PCI devices like that :-) I say it copies because I can pass *any* struct device_node pointer and it'll just copy that to the struct device argument. Here's of_dma_configure() to make your life easier: void of_dma_configure(struct device *dev, struct device_node *np) { u64 dma_addr, paddr, size; int ret; bool coherent; unsigned long offset; struct iommu_ops *iommu; /* * Set default coherent_dma_mask to 32 bit. Drivers are expected to * setup the correct supported mask. */ if (!dev->coherent_dma_mask) dev->coherent_dma_mask = DMA_BIT_MASK(32); /* * Set it to coherent_dma_mask by default if the architecture * code has not set it. */ if (!dev->dma_mask) dev->dma_mask = &dev->coherent_dma_mask; ret = of_dma_get_range(np, &dma_addr, &paddr, &size); if (ret < 0) { dma_addr = offset = 0; size = dev->coherent_dma_mask + 1; } else { offset = PFN_DOWN(paddr - dma_addr); /* * Add a work around to treat the size as mask + 1 in case * it is defined in DT as a mask. */ if (size & 1) { dev_warn(dev, "Invalid size 0x%llx for dma-range\n", size); size = size + 1; } if (!size) { dev_err(dev, "Adjusted size 0x%llx invalid\n", size); return; }
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: > On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: > > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: > > > > > > I would be in favour of a dma_inherit() function as well. We could hack > > > something up in the arch code (like below) but I would rather prefer an > > > explicit dma_inherit() call by drivers creating such devices. > > > > > > diff --git a/arch/arm64/include/asm/dma-mapping.h > > > b/arch/arm64/include/asm/dma-mapping.h > > > index ba437f090a74..ea6fb9b0e8fa 100644 > > > --- a/arch/arm64/include/asm/dma-mapping.h > > > +++ b/arch/arm64/include/asm/dma-mapping.h > > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; > > > > > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) > > > { > > > - if (dev && dev->archdata.dma_ops) > > > - return dev->archdata.dma_ops; > > > + while (dev) { > > > + if (dev->archdata.dma_ops) > > > + return dev->archdata.dma_ops; > > > + dev = dev->parent; > > > + } > > > > I think this would be a very bad idea: we don't want to have random > > devices be able to perform DMA just because their parent devices > > have been set up that way. > > I agree, it's a big hack. It would be nice to have a simpler way to do > this in driver code rather than explicitly calling > of_dma_configure/arch_setup_dma_ops as per the original patch in this > thread. > I haven't followed the entire discussion, but what's wrong with passing around a pointer to a 'struct device *hwdev' that represents the physical device that does the DMA? Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: > > > > I would be in favour of a dma_inherit() function as well. We could hack > > something up in the arch code (like below) but I would rather prefer an > > explicit dma_inherit() call by drivers creating such devices. > > > > diff --git a/arch/arm64/include/asm/dma-mapping.h > > b/arch/arm64/include/asm/dma-mapping.h > > index ba437f090a74..ea6fb9b0e8fa 100644 > > --- a/arch/arm64/include/asm/dma-mapping.h > > +++ b/arch/arm64/include/asm/dma-mapping.h > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; > > > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) > > { > > - if (dev && dev->archdata.dma_ops) > > - return dev->archdata.dma_ops; > > + while (dev) { > > + if (dev->archdata.dma_ops) > > + return dev->archdata.dma_ops; > > + dev = dev->parent; > > + } > > I think this would be a very bad idea: we don't want to have random > devices be able to perform DMA just because their parent devices > have been set up that way. I agree, it's a big hack. It would be nice to have a simpler way to do this in driver code rather than explicitly calling of_dma_configure/arch_setup_dma_ops as per the original patch in this thread. -- Catalin
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wed, Apr 27, 2016 at 08:41:06AM +0300, Felipe Balbi wrote: > Grygorii Strashko writes: > > On 04/26/2016 09:17 AM, Felipe Balbi wrote: > >> Grygorii Strashko writes: > >>> Now not all DMA paremters configured properly for "xhci-hcd" platform > >>> device which is created manually. For example: dma_pfn_offset, dam_ops > >>> and iommu configuration will not corresponds "dwc3" devices > >>> configuration. As result, this will cause problems like wrong DMA > >>> addresses translation on platforms with LPAE enabled like Keystone 2. > >>> > >>> When platform is using DT boot mode the DMA configuration will be > >>> parsed and applied from DT, so, to fix this issue, reuse > >>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd" > >>> from DWC3 device node. > >> > >> patch is incomplete. You left out non-DT users which might suffer from > >> the same problem. > > > > Honestly, I don't know how to fix it gracefully for non-DT case. > > I can update commit message to mention that this is fix for DT case only. > > no, that won't do :-) There are other users for this driver and they are > all "out-of-compliance" when it comes to DMA usage. Apparently, the > desired behavior is to pass correct device to DMA API which the gadget > side is already doing (see below). For the host side, the fix has to be > more involved. > > Frankly, I'd prefer that DMA setup could be inherited from parent > device, then it wouldn't really matter and a bunch of this could be > simplified. Some sort of dma_inherit(struct device *dev, struct device > *parent) would go a long way, IMHO. I would be in favour of a dma_inherit() function as well. We could hack something up in the arch code (like below) but I would rather prefer an explicit dma_inherit() call by drivers creating such devices. diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ba437f090a74..ea6fb9b0e8fa 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) { - if (dev && dev->archdata.dma_ops) - return dev->archdata.dma_ops; + while (dev) { + if (dev->archdata.dma_ops) + return dev->archdata.dma_ops; + dev = dev->parent; + } /* * We expect no ISA devices, and all other DMA masters are expected to -- Catalin
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 04/27/2016 04:59 PM, Catalin Marinas wrote: On Wed, Apr 27, 2016 at 08:41:06AM +0300, Felipe Balbi wrote: Grygorii Strashko writes: On 04/26/2016 09:17 AM, Felipe Balbi wrote: Grygorii Strashko writes: Now not all DMA paremters configured properly for "xhci-hcd" platform device which is created manually. For example: dma_pfn_offset, dam_ops and iommu configuration will not corresponds "dwc3" devices configuration. As result, this will cause problems like wrong DMA addresses translation on platforms with LPAE enabled like Keystone 2. When platform is using DT boot mode the DMA configuration will be parsed and applied from DT, so, to fix this issue, reuse of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd" from DWC3 device node. patch is incomplete. You left out non-DT users which might suffer from the same problem. Honestly, I don't know how to fix it gracefully for non-DT case. I can update commit message to mention that this is fix for DT case only. no, that won't do :-) There are other users for this driver and they are all "out-of-compliance" when it comes to DMA usage. Apparently, the desired behavior is to pass correct device to DMA API which the gadget side is already doing (see below). For the host side, the fix has to be more involved. Frankly, I'd prefer that DMA setup could be inherited from parent device, then it wouldn't really matter and a bunch of this could be simplified. Some sort of dma_inherit(struct device *dev, struct device *parent) would go a long way, IMHO. I would be in favour of a dma_inherit() function as well. We could hack something up in the arch code (like below) but I would rather prefer an explicit dma_inherit() call by drivers creating such devices. diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h index ba437f090a74..ea6fb9b0e8fa 100644 --- a/arch/arm64/include/asm/dma-mapping.h +++ b/arch/arm64/include/asm/dma-mapping.h @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) { - if (dev && dev->archdata.dma_ops) - return dev->archdata.dma_ops; + while (dev) { + if (dev->archdata.dma_ops) + return dev->archdata.dma_ops; + dev = dev->parent; + } /* * We expect no ISA devices, and all other DMA masters are expected to It's no enough to W/A just dma_ops :( dma_inherit()... FYI: http://www.spinics.net/lists/arm-kernel/msg384012.html Maybe you'll be able to find the way to make it acceptable. -- regards, -grygorii
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: > > I would be in favour of a dma_inherit() function as well. We could hack > something up in the arch code (like below) but I would rather prefer an > explicit dma_inherit() call by drivers creating such devices. > > diff --git a/arch/arm64/include/asm/dma-mapping.h > b/arch/arm64/include/asm/dma-mapping.h > index ba437f090a74..ea6fb9b0e8fa 100644 > --- a/arch/arm64/include/asm/dma-mapping.h > +++ b/arch/arm64/include/asm/dma-mapping.h > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) > { > - if (dev && dev->archdata.dma_ops) > - return dev->archdata.dma_ops; > + while (dev) { > + if (dev->archdata.dma_ops) > + return dev->archdata.dma_ops; > + dev = dev->parent; > + } I think this would be a very bad idea: we don't want to have random devices be able to perform DMA just because their parent devices have been set up that way. Arnd
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 04/27/2016 08:41 AM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko writes: >> On 04/26/2016 09:17 AM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Grygorii Strashko writes: Now not all DMA paremters configured properly for "xhci-hcd" platform device which is created manually. For example: dma_pfn_offset, dam_ops and iommu configuration will not corresponds "dwc3" devices configuration. As result, this will cause problems like wrong DMA addresses translation on platforms with LPAE enabled like Keystone 2. When platform is using DT boot mode the DMA configuration will be parsed and applied from DT, so, to fix this issue, reuse of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd" from DWC3 device node. >>> >>> patch is incomplete. You left out non-DT users which might suffer from >>> the same problem. >>> >> >> Honestly, I don't know how to fix it gracefully for non-DT case. >> I can update commit message to mention that this is fix for DT case only. > > no, that won't do :-) There are other users for this driver and they are > all "out-of-compliance" when it comes to DMA usage. Apparently, the > desired behavior is to pass correct device to DMA API which the gadget > side is already doing (see below). For the host side, the fix has to be > more involved. > > Frankly, I'd prefer that DMA setup could be inherited from parent > device, then it wouldn't really matter and a bunch of this could be > simplified. Some sort of dma_inherit(struct device *dev, struct device > *parent) would go a long way, IMHO. > > 8< cut here > commit 2725d6f974c4c268ae5fb746f8e3b33b76135aa8 > Author: Felipe Balbi > Date: Tue Apr 19 16:18:12 2016 +0300 > > usb: dwc3: use parent device for DMA > > our parent device is the one which was initialized > by either PCI or DeviceTree and that's the one which > is configured properly for DMA access. > > Instead of copying DMA bits from parent to child, > let's just rely on parent device for the entire DMA > API. 1) Patch I've sent fixes "xhci-hcd" platform device and not dwc3 core. On TI boards dwc3 core devices are created from DT and so, I do not see any problems with dwc3 core. Problem is with xhci. 2) there is minimum one dtsi file where dwc3 ("snps,dwc3") does not have parent device ls1021a.dtsi (and 5 dtsi in arm64 folder) > > Signed-off-by: Felipe Balbi > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index c050a88c16d4..09e4ff71a50f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -180,7 +180,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 > *dwc, u32 fladj) > static void dwc3_free_one_event_buffer(struct dwc3 *dwc, > struct dwc3_event_buffer *evt) > { > - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); > + dma_free_coherent(dwc->dev->parent, evt->length, evt->buf, evt->dma); > } [...] -- regards, -grygorii
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Grygorii Strashko writes: > On 04/26/2016 09:17 AM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko writes: >>> Now not all DMA paremters configured properly for "xhci-hcd" platform >>> device which is created manually. For example: dma_pfn_offset, dam_ops >>> and iommu configuration will not corresponds "dwc3" devices >>> configuration. As result, this will cause problems like wrong DMA >>> addresses translation on platforms with LPAE enabled like Keystone 2. >>> >>> When platform is using DT boot mode the DMA configuration will be >>> parsed and applied from DT, so, to fix this issue, reuse >>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd" >>> from DWC3 device node. >> >> patch is incomplete. You left out non-DT users which might suffer from >> the same problem. >> > > Honestly, I don't know how to fix it gracefully for non-DT case. > I can update commit message to mention that this is fix for DT case only. no, that won't do :-) There are other users for this driver and they are all "out-of-compliance" when it comes to DMA usage. Apparently, the desired behavior is to pass correct device to DMA API which the gadget side is already doing (see below). For the host side, the fix has to be more involved. Frankly, I'd prefer that DMA setup could be inherited from parent device, then it wouldn't really matter and a bunch of this could be simplified. Some sort of dma_inherit(struct device *dev, struct device *parent) would go a long way, IMHO. 8< cut here commit 2725d6f974c4c268ae5fb746f8e3b33b76135aa8 Author: Felipe Balbi Date: Tue Apr 19 16:18:12 2016 +0300 usb: dwc3: use parent device for DMA our parent device is the one which was initialized by either PCI or DeviceTree and that's the one which is configured properly for DMA access. Instead of copying DMA bits from parent to child, let's just rely on parent device for the entire DMA API. Signed-off-by: Felipe Balbi diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index c050a88c16d4..09e4ff71a50f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -180,7 +180,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) static void dwc3_free_one_event_buffer(struct dwc3 *dwc, struct dwc3_event_buffer *evt) { - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); + dma_free_coherent(dwc->dev->parent, evt->length, evt->buf, evt->dma); } /** @@ -202,7 +202,7 @@ static struct dwc3_event_buffer *dwc3_alloc_one_event_buffer(struct dwc3 *dwc, evt->dwc= dwc; evt->length = length; - evt->buf= dma_alloc_coherent(dwc->dev, length, + evt->buf= dma_alloc_coherent(dwc->dev->parent, length, &evt->dma, GFP_KERNEL); if (!evt->buf) return ERR_PTR(-ENOMEM); diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 143deb420481..c78238dc76fc 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -967,7 +967,7 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, u32 transfer_size = 0; u32 maxpacket; - ret = usb_gadget_map_request(&dwc->gadget, &req->request, + ret = usb_gadget_map_request_by_dev(dwc->dev->parent, &req->request, dep->number); if (ret) { dwc3_trace(trace_dwc3_ep0, "failed to map request\n"); @@ -995,7 +995,7 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc, dwc->ep0_bounce_addr, transfer_size, DWC3_TRBCTL_CONTROL_DATA, false); } else { - ret = usb_gadget_map_request(&dwc->gadget, &req->request, + ret = usb_gadget_map_request_by_dev(dwc->dev->parent, &req->request, dep->number); if (ret) { dwc3_trace(trace_dwc3_ep0, "failed to map request\n"); diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 88fd30bf0c46..6929775bde57 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -191,7 +191,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req, if (dwc->ep0_bounced && dep->number == 0) dwc->ep0_bounced = false; else - usb_gadget_unmap_request(&dwc->gadget, &req->request, + usb_gadget_unmap_request_by_dev(dwc->dev->parent, &req->request, req->direction); trace_dwc3_gadget_giveback(req); @@ -335,7 +335,7 @@ static int dwc3_alloc_trb_pool(struct dwc3_ep *dep) if (dep->trb_pool) return 0; - dep->trb_pool = dma_alloc_coherent(dwc->dev, + dep->trb_pool = dma_alloc_coherent(dwc->dev->p
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On 04/26/2016 09:17 AM, Felipe Balbi wrote: > > Hi, > > Grygorii Strashko writes: >> Now not all DMA paremters configured properly for "xhci-hcd" platform >> device which is created manually. For example: dma_pfn_offset, dam_ops >> and iommu configuration will not corresponds "dwc3" devices >> configuration. As result, this will cause problems like wrong DMA >> addresses translation on platforms with LPAE enabled like Keystone 2. >> >> When platform is using DT boot mode the DMA configuration will be >> parsed and applied from DT, so, to fix this issue, reuse >> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd" >> from DWC3 device node. > > patch is incomplete. You left out non-DT users which might suffer from > the same problem. > Honestly, I don't know how to fix it gracefully for non-DT case. I can update commit message to mention that this is fix for DT case only. -- regards, -grygorii
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Grygorii Strashko writes: > Now not all DMA paremters configured properly for "xhci-hcd" platform > device which is created manually. For example: dma_pfn_offset, dam_ops > and iommu configuration will not corresponds "dwc3" devices > configuration. As result, this will cause problems like wrong DMA > addresses translation on platforms with LPAE enabled like Keystone 2. > > When platform is using DT boot mode the DMA configuration will be > parsed and applied from DT, so, to fix this issue, reuse > of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd" > from DWC3 device node. patch is incomplete. You left out non-DT users which might suffer from the same problem. -- balbi signature.asc Description: PGP signature