Hi,
Arnd Bergmann <arnd@xxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
