| Message ID | 1418839344-14393-2-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 17 December 2014 13:02:23 Murali Karicheri wrote: > Now, in Kernel, parent device's DMA parameters has to be applied to > the child as is - to enable DMA support for the device. Usually this > is happened in places where parent device manually instantiates child > device such as in drivers/pci/probe.c (pci_device_add() for example). > > Now DMA configuration is represented in device data structure not only > by DMA mask and DMA params, it also includes dma_pfn_offset at least. > Hence introduce common dma_get_parent_cfg() helper to apply dma > configuration from parent to child, and use __weak to allow arch to > override it if needed. > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/base/dma-mapping.c | 18 ++++++++++++++++++ > include/linux/dma-mapping.h | 3 +++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c > index 9e8bbdd..5322426 100644 > --- a/drivers/base/dma-mapping.c > +++ b/drivers/base/dma-mapping.c > @@ -339,3 +339,21 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) > vunmap(cpu_addr); > } > #endif > + > +int __weak dma_get_parent_cfg(struct device *dev, struct device *parent) > +{ > + struct device *temp = parent; > + > + if (!temp) > + temp = dev->parent; > + > + if (temp && is_device_dma_capable(temp)) { > + dev->dma_mask = temp->dma_mask; > + dev->coherent_dma_mask = temp->coherent_dma_mask; As discussed, setting the pointers like this is always wrong, so don't do it. What's wrong with using arch_setup_dma_ops() from PCI as suggested previously? Arnd
On 12/17/2014 04:56 PM, Arnd Bergmann wrote: > On Wednesday 17 December 2014 13:02:23 Murali Karicheri wrote: >> Now, in Kernel, parent device's DMA parameters has to be applied to >> the child as is - to enable DMA support for the device. Usually this >> is happened in places where parent device manually instantiates child >> device such as in drivers/pci/probe.c (pci_device_add() for example). >> >> Now DMA configuration is represented in device data structure not only >> by DMA mask and DMA params, it also includes dma_pfn_offset at least. >> Hence introduce common dma_get_parent_cfg() helper to apply dma >> configuration from parent to child, and use __weak to allow arch to >> override it if needed. >> >> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> >> --- >> drivers/base/dma-mapping.c | 18 ++++++++++++++++++ >> include/linux/dma-mapping.h | 3 +++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c >> index 9e8bbdd..5322426 100644 >> --- a/drivers/base/dma-mapping.c >> +++ b/drivers/base/dma-mapping.c >> @@ -339,3 +339,21 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) >> vunmap(cpu_addr); >> } >> #endif >> + >> +int __weak dma_get_parent_cfg(struct device *dev, struct device *parent) >> +{ >> + struct device *temp = parent; >> + >> + if (!temp) >> + temp = dev->parent; >> + >> + if (temp&& is_device_dma_capable(temp)) { >> + dev->dma_mask = temp->dma_mask; >> + dev->coherent_dma_mask = temp->coherent_dma_mask; > > As discussed, setting the pointers like this is always wrong, > so don't do it. > > What's wrong with using arch_setup_dma_ops() from PCI as suggested > previously? > > Arnd Arnd, Thanks for the review. I had originally written a code based on that line as below. But dma-ranges property is also used by ppc and other architectures in the pci device DT node. So I wasn't sure how this code impact PCI driver functionality on those platforms. Hence used a simpler change as all that is needed for keystone is to get the dma_pfn_offset rightly set in the pci slave device. Initially I had a function implemented as below for this in of_pci.c. + * of_pci_dma_configure - Setup DMA configuration for a pci device + * @dev: pci device to apply DMA configuration + * + * Try to get devices's DMA configuration from DT and update it + * accordingly. This is a similar to of_dma_configure() for platform + * devices. + * + */ +void of_pci_dma_configure(struct pci_dev *dev) +{ + struct device *host_bridge, *parent; + struct pci_bus *bus = dev->bus; + u64 dma_addr, paddr, size; + int ret; + + while (!pci_is_root_bus(bus)) + bus = bus->parent; + host_bridge = bus->bridge; + + parent = host_bridge->parent; + if (parent->of_node) { + /* + * if dma-coherent property exist, call arch hook to setup + * dma coherent operations. + */ + if (of_dma_is_coherent(parent->of_node)) { + set_arch_dma_coherent_ops(&dev->dev); + dev_info(&dev->dev, "device is dma coherent\n"); + } + + /* + * if dma-ranges property doesn't exist - just return else + * setup the dma offset + */ + ret = of_dma_get_range(parent->of_node, &dma_addr, &paddr, &size); + if (ret < 0) { + dev_info(&dev->dev, "no dma range information to setup\n"); + printk("no dma range information to setup\n"); + return; + } + + /* DMA ranges found. Calculate and set dma_pfn_offset */ + dev->dev.dma_pfn_offset = PFN_DOWN(paddr - dma_addr); + dev_info(&dev->dev, "dma_pfn_offset(%#08lx)\n", dev->dev.dma_pfn_offset); + } +} +EXPORT_SYMBOL_GPL(of_pci_dma_configure); and then called from pci/probe.c as @@ -1527,6 +1528,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) dev->dev.dma_parms = &dev->dma_parms; dev->dev.coherent_dma_mask = 0xffffffffull; + /* Get the DMA configuration from root bridge's parent if present */ + of_pci_dma_configure(dev); If you think this is a better way to implement this, I can work on a patch set using this approach. Let me know.
On Wednesday 17 December 2014 18:24:43 Murali Karicheri wrote: > On 12/17/2014 04:56 PM, Arnd Bergmann wrote: > > On Wednesday 17 December 2014 13:02:23 Murali Karicheri wrote: > > > > What's wrong with using arch_setup_dma_ops() from PCI as suggested > > previously? > > +Will Deacon > I had originally written a code based on that line as below. But > dma-ranges property is also used by ppc and other architectures in the > pci device DT node. So I wasn't sure how this code impact PCI driver > functionality on those platforms. Hence used a simpler change as all > that is needed for keystone is to get the dma_pfn_offset rightly set in > the pci slave device. But in your patch, you don't call arch_setup_dma_ops() at all. > Initially I had a function implemented as below for this in of_pci.c. > > + * of_pci_dma_configure - Setup DMA configuration for a pci device > + * @dev: pci device to apply DMA configuration > + * > + * Try to get devices's DMA configuration from DT and update it > + * accordingly. This is a similar to of_dma_configure() for platform > + * devices. > + * > + */ > +void of_pci_dma_configure(struct pci_dev *dev) > +{ > + struct device *host_bridge, *parent; > + struct pci_bus *bus = dev->bus; > + u64 dma_addr, paddr, size; > + int ret; > + > + while (!pci_is_root_bus(bus)) > + bus = bus->parent; > + host_bridge = bus->bridge; > + > + parent = host_bridge->parent; > + if (parent->of_node) { so far it looks good, although we may want to introduce a helper function to get the of_node. > + /* > + * if dma-coherent property exist, call arch hook to setup > + * dma coherent operations. > + */ > + if (of_dma_is_coherent(parent->of_node)) { > + set_arch_dma_coherent_ops(&dev->dev); > + dev_info(&dev->dev, "device is dma coherent\n"); > + } set_arch_dma_coherent_ops no longer exists. Just keep the flag in a local variable > + /* > + * if dma-ranges property doesn't exist - just return else > + * setup the dma offset > + */ > + ret = of_dma_get_range(parent->of_node, &dma_addr, > &paddr, &size); > + if (ret < 0) { > + dev_info(&dev->dev, "no dma range information to > setup\n"); > + printk("no dma range information to setup\n"); > + return; > + } > + > + /* DMA ranges found. Calculate and set dma_pfn_offset */ > + dev->dev.dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > + dev_info(&dev->dev, "dma_pfn_offset(%#08lx)\n", > dev->dev.dma_pfn_offset); Same for the offset and size here, then pass all of the above into arch_setup_dma_ops. This is also where we need to hook up the iommu support once we decide how to make that work with the ARM SMMU. Will, I think we may have a problem on ARM64 now, since we only replaced set_arch_dma_coherent_ops on ARM32 but not ARM64. Can you send a fix for this? Without that, we don't have any coherent operations on ARM64 any more, unless I'm missing something. Arnd
On 12/17/2014 07:09 PM, Arnd Bergmann wrote: > On Wednesday 17 December 2014 18:24:43 Murali Karicheri wrote: >> On 12/17/2014 04:56 PM, Arnd Bergmann wrote: >>> On Wednesday 17 December 2014 13:02:23 Murali Karicheri wrote: >>> >>> What's wrong with using arch_setup_dma_ops() from PCI as suggested >>> previously? >>> > > +Will Deacon > >> I had originally written a code based on that line as below. But >> dma-ranges property is also used by ppc and other architectures in the >> pci device DT node. So I wasn't sure how this code impact PCI driver >> functionality on those platforms. Hence used a simpler change as all >> that is needed for keystone is to get the dma_pfn_offset rightly set in >> the pci slave device. > > But in your patch, you don't call arch_setup_dma_ops() at all. > >> Initially I had a function implemented as below for this in of_pci.c. >> >> + * of_pci_dma_configure - Setup DMA configuration for a pci device >> + * @dev: pci device to apply DMA configuration >> + * >> + * Try to get devices's DMA configuration from DT and update it >> + * accordingly. This is a similar to of_dma_configure() for platform >> + * devices. >> + * >> + */ >> +void of_pci_dma_configure(struct pci_dev *dev) >> +{ >> + struct device *host_bridge, *parent; >> + struct pci_bus *bus = dev->bus; >> + u64 dma_addr, paddr, size; >> + int ret; >> + >> + while (!pci_is_root_bus(bus)) >> + bus = bus->parent; >> + host_bridge = bus->bridge; >> + >> + parent = host_bridge->parent; >> + if (parent->of_node) { > > so far it looks good, although we may want to introduce > a helper function to get the of_node. Will add > >> + /* >> + * if dma-coherent property exist, call arch hook to setup >> + * dma coherent operations. >> + */ >> + if (of_dma_is_coherent(parent->of_node)) { >> + set_arch_dma_coherent_ops(&dev->dev); >> + dev_info(&dev->dev, "device is dma coherent\n"); >> + } > > set_arch_dma_coherent_ops no longer exists. Just keep the flag in a > local variable Ok. > >> + /* >> + * if dma-ranges property doesn't exist - just return else >> + * setup the dma offset >> + */ >> + ret = of_dma_get_range(parent->of_node,&dma_addr, >> &paddr,&size); >> + if (ret< 0) { >> + dev_info(&dev->dev, "no dma range information to >> setup\n"); >> + printk("no dma range information to setup\n"); >> + return; >> + } >> + >> + /* DMA ranges found. Calculate and set dma_pfn_offset */ >> + dev->dev.dma_pfn_offset = PFN_DOWN(paddr - dma_addr); >> + dev_info(&dev->dev, "dma_pfn_offset(%#08lx)\n", >> dev->dev.dma_pfn_offset); > > Same for the offset and size here, then pass all of the above into > arch_setup_dma_ops. This is also where we need to hook up the iommu > support once we decide how to make that work with the ARM SMMU. Ok. I will make this similar to of_dma_configure() that can be called from pci/probe.c Murali > > Will, I think we may have a problem on ARM64 now, since we only replaced > set_arch_dma_coherent_ops on ARM32 but not ARM64. Can you send a fix for > this? Without that, we don't have any coherent operations on ARM64 any > more, unless I'm missing something. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index 9e8bbdd..5322426 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -339,3 +339,21 @@ void dma_common_free_remap(void *cpu_addr, size_t size, unsigned long vm_flags) vunmap(cpu_addr); } #endif + +int __weak dma_get_parent_cfg(struct device *dev, struct device *parent) +{ + struct device *temp = parent; + + if (!temp) + temp = dev->parent; + + if (temp && is_device_dma_capable(temp)) { + dev->dma_mask = temp->dma_mask; + dev->coherent_dma_mask = temp->coherent_dma_mask; + dev->dma_parms = temp->dma_parms; + dev->dma_pfn_offset = temp->dma_pfn_offset; + return 0; + } + return -EINVAL; +} +EXPORT_SYMBOL(dma_get_parent_cfg); diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index d5d3881..eb080d6 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -127,6 +127,9 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask) return dma_set_mask_and_coherent(dev, mask); } +extern int __weak dma_get_parent_cfg(struct device *dev, + struct device *parent); + extern u64 dma_get_required_mask(struct device *dev); #ifndef set_arch_dma_coherent_ops
Now, in Kernel, parent device's DMA parameters has to be applied to the child as is - to enable DMA support for the device. Usually this is happened in places where parent device manually instantiates child device such as in drivers/pci/probe.c (pci_device_add() for example). Now DMA configuration is represented in device data structure not only by DMA mask and DMA params, it also includes dma_pfn_offset at least. Hence introduce common dma_get_parent_cfg() helper to apply dma configuration from parent to child, and use __weak to allow arch to override it if needed. Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> --- drivers/base/dma-mapping.c | 18 ++++++++++++++++++ include/linux/dma-mapping.h | 3 +++ 2 files changed, 21 insertions(+)