| Message ID | 87d1keirlu.fsf@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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?
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
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
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