Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: >> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: >> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: >> >> > > >> >> > > I would be in favour of a dma_inherit() function as well. We could hack >> >> > > something up in the arch code (like below) but I would rather prefer an >> >> > > explicit dma_inherit() call by drivers creating such devices. >> >> > > >> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h >> >> > > index ba437f090a74..ea6fb9b0e8fa 100644 >> >> > > --- a/arch/arm64/include/asm/dma-mapping.h >> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h >> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; >> >> > > >> >> > > static inline struct dma_map_ops *__generic_dma_ops(struct device *dev) >> >> > > { >> >> > > - if (dev && dev->archdata.dma_ops) >> >> > > - return dev->archdata.dma_ops; >> >> > > + while (dev) { >> >> > > + if (dev->archdata.dma_ops) >> >> > > + return dev->archdata.dma_ops; >> >> > > + dev = dev->parent; >> >> > > + } >> >> > >> >> > I think this would be a very bad idea: we don't want to have random >> >> > devices be able to perform DMA just because their parent devices >> >> > have been set up that way. >> >> >> >> I agree, it's a big hack. It would be nice to have a simpler way to do >> >> this in driver code rather than explicitly calling >> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this >> >> thread. >> > >> > I haven't followed the entire discussion, but what's wrong with passing >> > around a pointer to a 'struct device *hwdev' that represents the physical >> > device that does the DMA? >> >> that will likely create duplicated solutions in several drivers and >> it'll be a pain to maintain. There's another complication, dwc3 can be >> integrated in many different ways. See the device child-parent tree >> representations below: >> >> a) with a parent PCI device: >> >> pci_bus_type >> - dwc3-pci >> - dwc3 >> - xhci-plat >> >> b) with a parent platform_device (OF): >> >> platform_bus_type >> - dwc3-${omap,st,of-simple,exynos,keystone} >> - dwc3 >> - xhci-plat >> >> c) without a parent at all (thanks Grygorii): >> >> platform_bus_type >> - dwc3 >> - xhci-plat >> >> (a) and (b) above are the common cases. The DMA-capable device is >> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only >> having proper DMA configuration in OF platforms (because of the >> unconditional of_dma_configure() during OF device creation) and >> xhci-plat not knowing about DMA at all and hardcoding some crappy >> defaults. >> >> (c) is the uncommon case which creates some problems. In this case, dwc3 >> itself is the DMA-capable device and dwc3->dev->parent is the >> platform_bus_type itself. Now consider the problem this creates: >> >> i. the patch that I wrote [1] becomes invalid for (c), thanks to >> Grygorii for pointing this out before it was too late. >> >> ii. xhci-plat can also be described directly in DT (and is in some >> cases). This means that assuming xhci-plat's parent's parent to be the >> DMA-capable device is also an invalid assumption. >> >> iii. one might argue that for DT-based platforms *with* a glue layer >> ((b) above), OF already "copies" some sensible DMA defaults during >> device creation. > > But that is exactly the point I was trying to make: instead of copying > all the data into the xhci-plat device, just assign one pointer > inside the xhci-plat device from whoever creates it. and how do you pass that pointer to xhci-plat from another driver ? No, platform_data is not an option ;-) >> The only clean way to fix this up is with a dma_inherit()-like API which >> would allow dwc3's glue layers ((a) and (b) above) to initialize child's >> (dwc3) DMA configuration during child's creation. Something like below: >> >> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c >> index adc1e8a624cb..74b599269e2c 100644 >> --- a/drivers/usb/dwc3/dwc3-pci.c >> +++ b/drivers/usb/dwc3/dwc3-pci.c >> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, >> return -ENOMEM; >> } >> >> + dma_inherit(&dwc->dev, dev); >> + >> memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res)); >> >> res[0].start = pci_resource_start(pci, 0); >> >> that's all I'm asking for :-) dma_inherit() should, probably, be >> arch-specific to handle details like IOMMU (which today sits under >> archdata). > > It's still wrong: the archdata in the device can contain all sorts of > additional information about how to do DMA, and some of that information yes, inherit all of that ;-) > is bus specific, e.g. when your dma_map_ops look like the on sparc: okay, let me be clear. The underlying bus is still pci_bus_type or platform_bus_type; that won't change. > static inline struct dma_map_ops *get_dma_ops(struct device *dev) > { > #ifdef CONFIG_SPARC_LEON > if (sparc_cpu_model == sparc_leon) > return leon_dma_ops; > #endif > #if defined(CONFIG_SPARC32) && defined(CONFIG_PCI) > if (dev->bus == &pci_bus_type) > return &pci32_dma_ops; > #endif > return dma_ops; > } this seems to be a rather fragile assumption, no ? Only works because *_bus_type declarations are exported. I wonder if this is the only reason *why* they are exported, probably not... > There is no way for a device to "inherit" the bus_type of its parent, I don't want to inherit bus_type, I just want DMA configuration to not depend on bus_type directly ;-) -- balbi