| Message ID | 20190927002455.13169-6-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Sep 30, 2019 at 8:32 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote: > > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote: > > > -int of_dma_configure(struct device *dev, struct device_node *np, bool > > > force_dma) > > > +int of_dma_configure(struct device *dev, struct device_node *parent, bool > > > force_dma) > > > > This creates a > 80 char line. > > > > > { > > > u64 dma_addr, paddr, size = 0; > > > int ret; > > > bool coherent; > > > unsigned long offset; > > > const struct iommu_ops *iommu; > > > + struct device_node *np; > > > u64 mask; > > > > > > + np = dev->of_node; > > > + if (!np) > > > + np = parent; > > > + if (!np) > > > + return -ENODEV; > > > > I have to say I find the older calling convention simpler to understand. > > If we want to enforce the invariant I'd rather do that explicitly: > > > > if (dev->of_node && np != dev->of_node) > > return -EINVAL; > > As is, this would break Freescale Layerscape fsl-mc bus' dma_configure(): This may break PCI too for devices that have a DT node. > static int fsl_mc_dma_configure(struct device *dev) > { > struct device *dma_dev = dev; > > while (dev_is_fsl_mc(dma_dev)) > dma_dev = dma_dev->parent; > > return of_dma_configure(dev, dma_dev->of_node, 0); > } > > But I think that with this series, given the fact that we now treat the lack of > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the function > like this: Now, I'm reconsidering allowing this abuse... It's better if the code which understands the bus structure in DT for a specific bus passes in the right thing. Maybe I should go back to Robin's version (below). OTOH, the existing assumption that 'dma-ranges' was in the immediate parent was an assumption on the bus structure which maybe doesn't always apply. diff --git a/drivers/of/device.c b/drivers/of/device.c index a45261e21144..6951450bb8f3 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct device_node *parent, bool force_ u64 mask; np = dev->of_node; - if (!np) - np = parent; + if (np) + parent = of_get_dma_parent(np); + else + np = of_node_get(parent); if (!np) return -ENODEV; - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); + of_node_put(parent); if (ret < 0) { /* * For legacy reasons, we have to assume some devices need
On Thu, 2019-10-03 at 20:53 -0500, Rob Herring wrote: > > > > But I think that with this series, given the fact that we now treat the > > > > lack > > > > of > > > > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the > > > > function > > > > like this: > > > > > > Now, I'm reconsidering allowing this abuse... It's better if the code > > > which understands the bus structure in DT for a specific bus passes in > > > the right thing. Maybe I should go back to Robin's version (below). > > > OTOH, the existing assumption that 'dma-ranges' was in the immediate > > > parent was an assumption on the bus structure which maybe doesn't > > > always apply. > > > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c > > > index a45261e21144..6951450bb8f3 100644 > > > --- a/drivers/of/device.c > > > +++ b/drivers/of/device.c > > > @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct > > > device_node *parent, bool force_ > > > u64 mask; > > > > > > np = dev->of_node; > > > - if (!np) > > > - np = parent; > > > + if (np) > > > + parent = of_get_dma_parent(np); > > > + else > > > + np = of_node_get(parent); > > > if (!np) > > > return -ENODEV; > > > > > > - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > > > + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); > > > + of_node_put(parent); > > > if (ret < 0) { > > > /* > > > * For legacy reasons, we have to assume some devices need > > > > I spent some time thinking about your comments and researching. I came to > > the > > realization that both these solutions break the usage in > > drivers/gpu/drm/sun4i/sun4i_backend.c:805. In that specific case both > > 'dev->of_node' and 'parent' exist yet the device receiving the configuration > > and 'parent' aren't related in any way. > > I knew there was some reason I didn't like those virtual DT nodes... > > That does seem to be the oddest case. Several of the others are just > non-DT child platform devices. Perhaps we need a "copy the DMA config > from another struct device (or parent struct device)" function to > avoid using a DT function on a non-DT device. > > > IOW we can't just use 'dev->of_node' as a starting point to walk upwards the > > tree. We always have to respect whatever DT node the bus provided, and start > > there. This clashes with the current solutions, as they are based on the > > fact > > that we can use dev->of_node when present. > > Yes, you are right. > > > My guess at this point, if we're forced to honor that behaviour, is that we > > have to create a new API for the PCI use case. Something the likes of > > of_dma_configure_parent(). > > I think of_dma_configure just has to work with the device_node of > either the device or the device parent and dev->of_node is never used > unless the caller sets it. Fine, so given the following two distinct uses of of_dma_configure(struct device *dev, struct device_node *np, bool ...): - dev->of_node == np: Platform bus' typical use, we imperatively have to start parsing dma-ranges from np's DMA parent, as the device we're configuring might be a bus containing dma-ranges himself. For example a platform PCIe bus. - dev->of_node != np: Here the bus is pulling some trick. The device might or might not be represented in DT and np might be a bus or a device. But one thing I realised is that device being configured never represents a memory mapped bus. Assuming this assumption is acceptable, we can traverse the DT tree starting from np and get a correct configuration as long as dma-ranges not being present is interpreted as a 1:1 mapping. The resulting code, which I tested on an RPi4, Freescale Layerscape and passes OF's unit tests, looks like this: int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) { u64 dma_addr, paddr, size = 0; struct device_node *parent; u64 mask; int ret; if (!np) return -ENODEV; parent = of_node_get(np); if (dev->of_node == parent) parent = of_get_next_dma_parent(np); ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); of_node_put(parent); [...] } Would that be acceptable? Regards, Nicolas
diff --git a/drivers/of/device.c b/drivers/of/device.c index da8158392010..a45261e21144 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -75,7 +75,8 @@ int of_device_add(struct platform_device *ofdev) /** * of_dma_configure - Setup DMA configuration * @dev: Device to apply DMA configuration - * @np: Pointer to OF node having DMA configuration + * @parent: OF node of parent device having DMA configuration, if + * @dev->of_node is NULL (ignored otherwise) * @force_dma: Whether device is to be set up by of_dma_configure() even if * DMA capability is not explicitly described by firmware. * @@ -86,15 +87,22 @@ int of_device_add(struct platform_device *ofdev) * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) +int of_dma_configure(struct device *dev, struct device_node *parent, bool force_dma) { u64 dma_addr, paddr, size = 0; int ret; bool coherent; unsigned long offset; const struct iommu_ops *iommu; + struct device_node *np; u64 mask; + np = dev->of_node; + if (!np) + np = parent; + if (!np) + return -ENODEV; + ret = of_dma_get_range(np, &dma_addr, &paddr, &size); if (ret < 0) { /* diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 8d31e39dd564..a4fe418e57f6 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -56,7 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) } int of_dma_configure(struct device *dev, - struct device_node *np, + struct device_node *parent, bool force_dma); #else /* CONFIG_OF */ @@ -107,7 +107,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) } static inline int of_dma_configure(struct device *dev, - struct device_node *np, + struct device_node *parent, bool force_dma) { return 0;