diff mbox

[RFC,1/2] common: dma-mapping: introduce dma_get_parent_cfg() helper

Message ID 1418839344-14393-2-git-send-email-m-karicheri2@ti.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Murali Karicheri Dec. 17, 2014, 6:02 p.m. UTC
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(+)

Comments

Arnd Bergmann Dec. 17, 2014, 9:56 p.m. UTC | #1
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
--
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
Murali Karicheri Dec. 17, 2014, 11:24 p.m. UTC | #2
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.
Arnd Bergmann Dec. 18, 2014, 12:09 a.m. UTC | #3
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
--
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
Murali Karicheri Dec. 18, 2014, 7:02 p.m. UTC | #4
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 mbox

Patch

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