Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1474745 > unrolled thread
| Started by | Leo Li <pku.leo@gmail.com> |
|---|---|
| First post | 2016-09-02 00:20 +0200 |
| Last post | 2016-09-09 04:00 +0200 |
| Articles | 20 on this page of 60 — 9 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Leo Li <pku.leo@gmail.com> - 2016-09-02 00:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-02 12:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-02 13:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-02 13:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-02 16:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Alan Stern <stern@rowland.harvard.edu> - 2016-09-02 16:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-02 18:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Roger Quadros <rogerq@ti.com> - 2016-09-07 09:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-07 10:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Roger Quadros <rogerq@ti.com> - 2016-09-07 15:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-07 16:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-02 18:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-02 13:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Robin Murphy <robin.murphy@arm.com> - 2016-09-02 14:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-02 15:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-02 15:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Leo Li <pku.leo@gmail.com> - 2016-09-03 00:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-05 17:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-06 08:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-06 08:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-06 12:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-06 13:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-06 15:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-07 09:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-07 09:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-07 11:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-07 11:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-07 11:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-07 12:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-06 12:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-07 08:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-07 10:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-07 12:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Robin Murphy <robin.murphy@arm.com> - 2016-09-07 12:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-07 12:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-07 12:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-07 17:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Alan Stern <stern@rowland.harvard.edu> - 2016-09-07 18:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-07 21:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-08 03:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-08 10:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-08 10:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-08 11:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 12:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-08 13:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 13:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-08 13:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 13:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi <balbi@kernel.org> - 2016-09-08 14:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 14:50 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-08 14:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 14:20 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-08 14:30 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Arnd Bergmann <arnd@arndb.de> - 2016-09-08 15:00 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-09 03:40 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Grygorii Strashko <grygorii.strashko@ti.com> - 2016-09-08 15:10 +0200
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Peter Chen <hzpeterchen@gmail.com> - 2016-09-09 04:00 +0200
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 10:10 +0200 |
| Message-ID | <sf2zf-3eC-3@gated-at.bofh.it> |
| In reply to | #1478719 |
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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-08 10:10 +0200 |
| Message-ID | <sf2zg-3eC-27@gated-at.bofh.it> |
| In reply to | #1478410 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Arnd Bergmann <arnd@arndb.de> writes:
>> Arnd Bergmann <arnd@arndb.de> 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 <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/platform_device.h>
> +#include <linux/pci.h>
actually, we don't want the core to know what it's attached to.
> #include <linux/pm_runtime.h>
> #include <linux/interrupt.h>
> #include <linux/ioport.h>
> @@ -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 <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/platform_device.h>
> -#include <linux/dma-mapping.h>
> #include <linux/clk.h>
> #include <linux/usb/otg.h>
> #include <linux/usb/usb_phy_generic.h>
> @@ -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?
> 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.
--
balbi
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 10:30 +0200 |
| Message-ID | <sf2SC-3oQ-25@gated-at.bofh.it> |
| In reply to | #1478900 |
On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> >> Arnd Bergmann <arnd@arndb.de> 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 <linux/slab.h>
> > #include <linux/spinlock.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pci.h>
>
> 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 <linux/pm_runtime.h>
> > #include <linux/interrupt.h>
> > #include <linux/ioport.h>
> > @@ -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 <linux/kernel.h>
> > #include <linux/slab.h>
> > #include <linux/platform_device.h>
> > -#include <linux/dma-mapping.h>
> > #include <linux/clk.h>
> > #include <linux/usb/otg.h>
> > #include <linux/usb/usb_phy_generic.h>
> > @@ -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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-08 10:40 +0200 |
| Message-ID | <sf32i-3sy-19@gated-at.bofh.it> |
| In reply to | #1478916 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Arnd Bergmann <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 10:50 +0200 |
| Message-ID | <sf3bX-3vN-5@gated-at.bofh.it> |
| In reply to | #1478920 |
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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-08 11:50 +0200 |
| Message-ID | <sf481-44o-13@gated-at.bofh.it> |
| In reply to | #1478929 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Arnd Bergmann <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 12:20 +0200 |
| Message-ID | <sf4B3-4xM-9@gated-at.bofh.it> |
| In reply to | #1478981 |
On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> Arnd Bergmann <arnd@arndb.de> 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@48880000 {
compatible = "ti,dwc3";
#address-cells = <1>;
#size-cells = <1>;
ranges;
usb1: usb@48890000 {
compatible = "snps,dwc3";
reg = <0x48890000 0x17000>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
<GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-08 13:10 +0200 |
| Message-ID | <sf5nt-53y-41@gated-at.bofh.it> |
| In reply to | #1479033 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Arnd Bergmann <arnd@arndb.de> writes:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann <arnd@arndb.de> 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@48880000 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@48890000 {
> compatible = "snps,dwc3";
> reg = <0x48890000 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> 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
+ dev->coherent_dma_mask = dev->parent->coherent_dma_mask;
+ }
/*
* Set it to coherent_dma_mask by default if the architecture
--
balbi
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 13:20 +0200 |
| Message-ID | <sf5x8-56T-19@gated-at.bofh.it> |
| In reply to | #1479103 |
On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >> Arnd Bergmann <arnd@arndb.de> 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@48880000 {
> > compatible = "ti,dwc3";
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges;
> > usb1: usb@48890000 {
> > compatible = "snps,dwc3";
> > reg = <0x48890000 0x17000>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> > interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > 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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-08 13:30 +0200 |
| Message-ID | <sf5GO-5a7-25@gated-at.bofh.it> |
| In reply to | #1479110 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Arnd Bergmann <arnd@arndb.de> writes:
> On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
>> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> >> Arnd Bergmann <arnd@arndb.de> 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@48880000 {
>> > compatible = "ti,dwc3";
>> > #address-cells = <1>;
>> > #size-cells = <1>;
>> > ranges;
>> > usb1: usb@48890000 {
>> > compatible = "snps,dwc3";
>> > reg = <0x48890000 0x17000>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> > interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
>> > <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
>> > <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> > 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 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.
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.
--
balbi
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 13:50 +0200 |
| Message-ID | <sf609-5gF-1@gated-at.bofh.it> |
| In reply to | #1479120 |
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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-08 14:00 +0200 |
| Message-ID | <sf69Q-5kf-31@gated-at.bofh.it> |
| In reply to | #1479128 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
Arnd Bergmann <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 14:50 +0200 |
| Message-ID | <sf6We-5Sk-9@gated-at.bofh.it> |
| In reply to | #1479148 |
On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote:
> Arnd Bergmann <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2016-09-08 14:10 +0200 |
| Subject | Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev |
| Message-ID | <sf6jv-5D5-23@gated-at.bofh.it> |
| In reply to | #1479103 |
On 09/08/2016 02:00 PM, Felipe Balbi wrote:
>
> Hi,
>
> Arnd Bergmann <arnd@arndb.de> writes:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann <arnd@arndb.de> 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@48880000 {
>> compatible = "ti,dwc3";
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges;
>> usb1: usb@48890000 {
>> compatible = "snps,dwc3";
>> reg = <0x48890000 0x17000>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
>> <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
>> <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> 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
> + dev->coherent_dma_mask = dev->parent->coherent_dma_mask;
> + }
>
> /*
> * Set it to coherent_dma_mask by default if the architecture
>
>
I'd like to clarify few points here:
- the default dma_mask = DMA_BIT_MASK(32); assigned here to keep
backward compatibility with existing DT files at the moment when
of_dma_configure() has been introduced and it satisfies most of the cases
- if HW require specific DMA configuration then "dma-ranges" property have to be
defined and of_dma_configure() will take care of it just few lines down.
including parent-child case - as it will try to find "dma-ranes" prop in parent node
when called for child dev.
Personally, I think Arnd's approach should work, if the problem of selecting of proper
sysdev/dma_dev device will be solved.
Wouldn't it work if is_device_dma_capable() will be used?
For DT-case, the device DMA properties have to be configured from DT. So, now
there are 2 cases for dwc3:
1) dwc3-glue (of_dma)
|- dwc3 (of_dma)
|- xhci-plat (manual)
better to use dwc3-glue as sysdev, but can use dwc3 also
2) (arch/arm/boot/dts/ls1021a.dtsi)
|- dwc3 (of_dma)
|- xhci-plat (manual)
need to use dwc3 as sysdev
dwc3: probe()
if (!&pdev->dev->of_node)
legacy case - hard-code DMA props
dwc->sysdev = &pdev->dev;
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)
if (!dwc->sysdev)
oops;
xhci_plat_probe:
do the same
Wouldn't above work for other cases PCI/ACPI?
--
regards,
-grygorii
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 14:20 +0200 |
| Message-ID | <sf6tc-5If-9@gated-at.bofh.it> |
| In reply to | #1479152 |
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
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-09-08 14:30 +0200 |
| Subject | Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev |
| Message-ID | <sf6CS-5Lv-21@gated-at.bofh.it> |
| In reply to | #1479033 |
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 <arnd@arndb.de> 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@48880000 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@48890000 {
> compatible = "snps,dwc3";
> reg = <0x48890000 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> 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>;
> };
> };
> };
> };
>
With my above changes, the hub of_node should be found since it is
child of root hub's of_node which is the dwc3's of_node.
> It's also the node that contains the "phys" properties and
> presumably other properties like "otg-rev", "maximum-speed"
> etc.
>
This information is described at dwc3 core device of_node, and be
handled at dwc3/core.c
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-09-08 15:00 +0200 |
| Message-ID | <sf75T-5Vt-5@gated-at.bofh.it> |
| In reply to | #1479169 |
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 <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-09-09 03:40 +0200 |
| Subject | Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev |
| Message-ID | <sfiXn-4V9-5@gated-at.bofh.it> |
| In reply to | #1479185 |
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 <arnd@arndb.de> 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
[toc] | [prev] | [next] | [standalone]
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2016-09-08 15:10 +0200 |
| Subject | Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev |
| Message-ID | <sf7fA-6dR-1@gated-at.bofh.it> |
| In reply to | #1479169 |
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 <arnd@arndb.de> 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 0x20000>,<0x5b880 0x80>;
interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&gateclk 16>;
phys = <&usbcluster PHY_TYPE_USB3>;
phy-names = "usb";
status = "disabled";
};
What will be the parent dev in above case?
--
regards,
-grygorii
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-09-09 04:00 +0200 |
| Subject | Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev |
| Message-ID | <sfjgJ-51k-3@gated-at.bofh.it> |
| In reply to | #1479193 |
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 <arnd@arndb.de> 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 0x20000>,<0x5b880 0x80>;
> interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> 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
[toc] | [prev] | [standalone]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web