[RFC v1 PATCH 0/2] PCI: get DMA configuration from parent device

[RFC v1 PATCH 0/2] PCI: get DMA configuration from parent device

[Murali Karicheri]

PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
and dma ops etc using the same information from DT. The prior RFCs and
discussions are available at [1] and [2] below.

This patch make use of a recent dma API addeded to latest master and I could
only compile this as the latest kernel doesn't boot up on Keystone platforms.
So I am continue to send RFC version so that this gets a wider review before
sending the formal patch.

[2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
[1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591

Change history:
	v1 - updates based on the comments against initial RFC.
	   - Added a helper function to get the OF node of the parent
	   - Added an API in of_pci.c to update DMA configuration of the pci
	     device.

Murali Karicheri (2):
  of/pci: add of_pci_dma_configure() update dma configuration
  PCI: update dma configuration from DT

 drivers/of/of_pci.c    |   71 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c    |    2 ++
 include/linux/of_pci.h |   12 ++++++++
 3 files changed, 85 insertions(+)

-- 
1.7.9.5

[RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

Add of_pci_dma_configure() to allow updating the dma configuration
of the pci device using the configuration from the parent of
the root bridge device.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/of/of_pci.c    |   71 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |   12 ++++++++
 2 files changed, 83 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 88471d3..966b418 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -229,6 +229,77 @@ parse_failed:
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
+
+/**
+ * of_get_pci_root_bridge_parent - Helper function to get the OF node of
+ *		the root bridge's parent
+ * @dev: ptr to pci_dev struct of the pci device
+ *
+ * This function will traverse the bus up to the root bus starting with
+ * the child and return the of node ptr to bridge device's parent device.
+ */
+struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct device *bridge;
+
+	while (!pci_is_root_bus(bus))
+		bus = bus->parent;
+	bridge = bus->bridge;
+
+	return bridge->parent->of_node;
+}
+EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
+
+/**
+ * of_pci_dma_configure - Setup DMA configuration
+ * @dev: ptr to pci_dev struct of the pci device
+ *
+ * Try to get PCI devices's DMA configuration from DT and update it
+ * accordingly. This is similar to of_dma_configure() in of/platform.c
+ */
+void of_pci_dma_configure(struct pci_dev *pci_dev)
+{
+	struct device *dev = &pci_dev->dev;
+	u64 dma_addr, paddr, size;
+	struct device_node *parent_np;
+	unsigned long offset;
+	bool coherent;
+	int ret;
+
+	parent_np = of_get_pci_root_bridge_parent(pci_dev);
+
+	/*
+	 * Set default dma-mask to 32 bit. Drivers are expected to setup
+	 * the correct supported dma_mask.
+	 */
+	dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+	/*
+	 * Set it to coherent_dma_mask by default if the architecture
+	 * code has not set it.
+	 */
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+
+	ret = of_dma_get_range(parent_np, &dma_addr, &paddr, &size);
+	if (ret < 0) {
+		dma_addr = offset = 0;
+		size = dev->coherent_dma_mask;
+	} else {
+		offset = PFN_DOWN(paddr - dma_addr);
+		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
+	}
+	dev->dma_pfn_offset = offset;
+
+	coherent = of_dma_is_coherent(parent_np);
+	dev_dbg(dev, "device is%sdma coherent\n",
+		coherent ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
+}
+EXPORT_SYMBOL_GPL(of_pci_dma_configure);
+
 #endif /* CONFIG_OF_ADDRESS */
 
 #ifdef CONFIG_PCI_MSI
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index ce0e5ab..0465a2a 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -16,6 +16,8 @@ int of_pci_get_devfn(struct device_node *np);
 int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
+struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev);
+void of_pci_dma_configure(struct pci_dev *pci_dev);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
@@ -50,6 +52,16 @@ of_get_pci_domain_nr(struct device_node *node)
 {
 	return -1;
 }
+
+static inline struct device_node
+*of_get_pci_root_bridge_parent(struct pci_dev *dev)
+{
+	return NULL;
+}
+
+static inline void of_pci_dma_configure(struct pci_dev *pci_dev)
+{
+}
 #endif
 
 #if defined(CONFIG_OF_ADDRESS)
-- 
1.7.9.5

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Thursday 18 December 2014 17:07:04 Murali Karicheri wrote:
> Add of_pci_dma_configure() to allow updating the dma configuration
> of the pci device using the configuration from the parent of
> the root bridge device.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

Much better!

There is one detail that we should get right from the start here,
which is currently wrong in the platform device code, but I have so
far not dared change it:

> +	/*
> +	 * Set default dma-mask to 32 bit. Drivers are expected to setup
> +	 * the correct supported dma_mask.
> +	 */
> +	dev->coherent_dma_mask = DMA_BIT_MASK(32);

The 32-bit mask is indeed correct as the default for almost all cases,
and we must not set it larger than DMA_BIT_MASK(32). There is one
corner case though, which happens on some shmobile machines that have
a 31-bit mask on the PCI host, and I think we should use that as the
default.

> +	/*
> +	 * Set it to coherent_dma_mask by default if the architecture
> +	 * code has not set it.
> +	 */
> +	if (!dev->dma_mask)
> +		dev->dma_mask = &dev->coherent_dma_mask;
> +
> +	ret = of_dma_get_range(parent_np, &dma_addr, &paddr, &size);
> +	if (ret < 0) {
> +		dma_addr = offset = 0;
> +		size = dev->coherent_dma_mask;

Can you check one thing here? I believe the size argument as returned
from of_dma_get_range is inclusive (e.g. 0x100000000), while the coherent
mask by definition is exlusive (e.g. 0xffffffff), so the size needs to
be adapted here. I haven't checked all the code here though, so I may
be wrong.

> +	} else {
> +		offset = PFN_DOWN(paddr - dma_addr);
> +		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
> +	}
> +	dev->dma_pfn_offset = offset;
> +
> +	coherent = of_dma_is_coherent(parent_np);
> +	dev_dbg(dev, "device is%sdma coherent\n",
> +		coherent ? " " : " not ");
> +
> +	arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);

Basically, I would use limit the size argument we pass into
arch_setup_dma_ops to the minimum of 'size' and 'dma_mask' here,
after converting into the same format. We should make sure we do the
same thing for platform_device as well, so it might be better to do
it inside of arch_setup_dma_ops instead.

	Arnd

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 12/18/2014 05:29 PM, Arnd Bergmann wrote:
> On Thursday 18 December 2014 17:07:04 Murali Karicheri wrote:
>> Add of_pci_dma_configure() to allow updating the dma configuration
>> of the pci device using the configuration from the parent of
>> the root bridge device.
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>
> Much better!
>
> There is one detail that we should get right from the start here,
> which is currently wrong in the platform device code, but I have so
> far not dared change it:
>
>> +	/*
>> +	 * Set default dma-mask to 32 bit. Drivers are expected to setup
>> +	 * the correct supported dma_mask.
>> +	 */
>> +	dev->coherent_dma_mask = DMA_BIT_MASK(32);
>
> The 32-bit mask is indeed correct as the default for almost all cases,
> and we must not set it larger than DMA_BIT_MASK(32). There is one
> corner case though, which happens on some shmobile machines that have
> a 31-bit mask on the PCI host, and I think we should use that as the
> default.
>
>> +	/*
>> +	 * Set it to coherent_dma_mask by default if the architecture
>> +	 * code has not set it.
>> +	 */
>> +	if (!dev->dma_mask)
>> +		dev->dma_mask =&dev->coherent_dma_mask;
>> +
>> +	ret = of_dma_get_range(parent_np,&dma_addr,&paddr,&size);
>> +	if (ret<  0) {
>> +		dma_addr = offset = 0;
>> +		size = dev->coherent_dma_mask;
>
> Can you check one thing here? I believe the size argument as returned
> from of_dma_get_range is inclusive (e.g. 0x100000000), while the coherent
> mask by definition is exlusive (e.g. 0xffffffff), so the size needs to
> be adapted here. I haven't checked all the code here though, so I may
> be wrong.

size returned by of_dma_get_range() is inclusive as you indicated. Fromt 
the grep of dma-ranges in arch/arm/boot/dts, I see

arch/arm/boot/dts/keystone.dtsi:		dma-ranges = <0x80000000 0x8 
0x00000000 0x80000000>;
arch/arm/boot/dts/keystone.dtsi:			dma-ranges;
arch/arm/boot/dts/r8a7790.dtsi:		dma-ranges = <0x42000000 0 0x40000000 0 
0x40000000 0 0x80000000
arch/arm/boot/dts/integratorap.dts:	dma-ranges = <0x80000000 0x0 
0x80000000>;
arch/arm/boot/dts/r8a7791.dtsi:		dma-ranges = <0x42000000 0 0x40000000 0 
0x40000000 0 0x80000000

So I guess I need to change the code to

 >> +	ret = of_dma_get_range(parent_np,&dma_addr,&paddr,&size);
 >> +	if (ret<  0) {
 >> +		dma_addr = offset = 0;
 >> +		size = dev->coherent_dma_mask + 1;

>
>> +	} else {
>> +		offset = PFN_DOWN(paddr - dma_addr);
>> +		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
>> +	}
>> +	dev->dma_pfn_offset = offset;
>> +
>> +	coherent = of_dma_is_coherent(parent_np);
>> +	dev_dbg(dev, "device is%sdma coherent\n",
>> +		coherent ? " " : " not ");
>> +
>> +	arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>
> Basically, I would use limit the size argument we pass into
> arch_setup_dma_ops to the minimum of 'size' and 'dma_mask' here,
> after converting into the same format. We should make sure we do the
> same thing for platform_device as well, so it might be better to do
> it inside of arch_setup_dma_ops instead.

Do you think following changes will work?

+++ b/arch/arm/mm/dma-mapping.c
@@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64 
dma_base, u64 size,
                         struct iommu_ops *iommu, bool coherent)
  {
         struct dma_map_ops *dma_ops;
+       u64 temp_size = min((*(dev->dma_mask) + 1), size);

         dev->archdata.dma_coherent = coherent;
-       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
+       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))

If you agree, I will post v1 of the patch with these updates. Let me 
know. I did some basic tests on Keystone with these changes and it works 
fine.

Murali

>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Monday 22 December 2014 12:46:12 Murali Karicheri wrote:
> On 12/18/2014 05:29 PM, Arnd Bergmann wrote:
> > On Thursday 18 December 2014 17:07:04 Murali Karicheri wrote:
> >> Add of_pci_dma_configure() to allow updating the dma configuration
> >> of the pci device using the configuration from the parent of
> >> the root bridge device.
> >> +	/*
> >> +	 * Set it to coherent_dma_mask by default if the architecture
> >> +	 * code has not set it.
> >> +	 */
> >> +	if (!dev->dma_mask)
> >> +		dev->dma_mask =&dev->coherent_dma_mask;
> >> +
> >> +	ret = of_dma_get_range(parent_np,&dma_addr,&paddr,&size);
> >> +	if (ret<  0) {
> >> +		dma_addr = offset = 0;
> >> +		size = dev->coherent_dma_mask;
> >
> > Can you check one thing here? I believe the size argument as returned
> > from of_dma_get_range is inclusive (e.g. 0x100000000), while the coherent
> > mask by definition is exlusive (e.g. 0xffffffff), so the size needs to
> > be adapted here. I haven't checked all the code here though, so I may
> > be wrong.
> 
> size returned by of_dma_get_range() is inclusive as you indicated. Fromt 
> the grep of dma-ranges in arch/arm/boot/dts, I see
> 
> arch/arm/boot/dts/keystone.dtsi:		dma-ranges = <0x80000000 0x8 
> 0x00000000 0x80000000>;
> arch/arm/boot/dts/keystone.dtsi:			dma-ranges;
> arch/arm/boot/dts/r8a7790.dtsi:		dma-ranges = <0x42000000 0 0x40000000 0 
> 0x40000000 0 0x80000000
> arch/arm/boot/dts/integratorap.dts:	dma-ranges = <0x80000000 0x0 
> 0x80000000>;
> arch/arm/boot/dts/r8a7791.dtsi:		dma-ranges = <0x42000000 0 0x40000000 0 
> 0x40000000 0 0x80000000
> 
> So I guess I need to change the code to
> 
>  >> +	ret = of_dma_get_range(parent_np,&dma_addr,&paddr,&size);
>  >> +	if (ret<  0) {
>  >> +		dma_addr = offset = 0;
>  >> +		size = dev->coherent_dma_mask + 1;

Right.

> >> +	} else {
> >> +		offset = PFN_DOWN(paddr - dma_addr);
> >> +		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
> >> +	}
> >> +	dev->dma_pfn_offset = offset;
> >> +
> >> +	coherent = of_dma_is_coherent(parent_np);
> >> +	dev_dbg(dev, "device is%sdma coherent\n",
> >> +		coherent ? " " : " not ");
> >> +
> >> +	arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
> >
> > Basically, I would use limit the size argument we pass into
> > arch_setup_dma_ops to the minimum of 'size' and 'dma_mask' here,
> > after converting into the same format. We should make sure we do the
> > same thing for platform_device as well, so it might be better to do
> > it inside of arch_setup_dma_ops instead.
> 
> Do you think following changes will work?
> 
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64 
> dma_base, u64 size,
>                          struct iommu_ops *iommu, bool coherent)
>   {
>          struct dma_map_ops *dma_ops;
> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
> 
>          dev->archdata.dma_coherent = coherent;
> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
> 
> If you agree, I will post v1 of the patch with these updates. Let me 
> know. I did some basic tests on Keystone with these changes and it works 
> fine.

It's not exactly what I meant. My main point was that we need to limit
dev->dma_mask to (size-1) here, but you are not touching that. We can
either change the two functions in which we first assign the dma_mask
(platform and pci bus), or set it again in arch_setup_dma_ops (on each
architecture implementing it), either way would work. Someone else
might have a stronger opinion on that matter.

For arm_setup_iommu_dma_ops, passing the original size is probably
best.

	Arnd

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 12/22/2014 02:43 PM, Arnd Bergmann wrote:
> On Monday 22 December 2014 12:46:12 Murali Karicheri wrote:
>> On 12/18/2014 05:29 PM, Arnd Bergmann wrote:
>>> On Thursday 18 December 2014 17:07:04 Murali Karicheri wrote:
>>>> Add of_pci_dma_configure() to allow updating the dma configuration
>>>> of the pci device using the configuration from the parent of
>>>> the root bridge device.
>>>> +	/*
>>>> +	 * Set it to coherent_dma_mask by default if the architecture
>>>> +	 * code has not set it.
>>>> +	 */
>>>> +	if (!dev->dma_mask)
>>>> +		dev->dma_mask =&dev->coherent_dma_mask;
>>>> +
>>>> +	ret = of_dma_get_range(parent_np,&dma_addr,&paddr,&size);
>>>> +	if (ret<   0) {
>>>> +		dma_addr = offset = 0;
>>>> +		size = dev->coherent_dma_mask;
>>>
>>> Can you check one thing here? I believe the size argument as returned
>>> from of_dma_get_range is inclusive (e.g. 0x100000000), while the coherent
>>> mask by definition is exlusive (e.g. 0xffffffff), so the size needs to
>>> be adapted here. I haven't checked all the code here though, so I may
>>> be wrong.
>>
>> size returned by of_dma_get_range() is inclusive as you indicated. Fromt
>> the grep of dma-ranges in arch/arm/boot/dts, I see
>>
>> arch/arm/boot/dts/keystone.dtsi:		dma-ranges =<0x80000000 0x8
>> 0x00000000 0x80000000>;
>> arch/arm/boot/dts/keystone.dtsi:			dma-ranges;
>> arch/arm/boot/dts/r8a7790.dtsi:		dma-ranges =<0x42000000 0 0x40000000 0
>> 0x40000000 0 0x80000000
>> arch/arm/boot/dts/integratorap.dts:	dma-ranges =<0x80000000 0x0
>> 0x80000000>;
>> arch/arm/boot/dts/r8a7791.dtsi:		dma-ranges =<0x42000000 0 0x40000000 0
>> 0x40000000 0 0x80000000
>>
>> So I guess I need to change the code to
>>
>>   >>  +	ret = of_dma_get_range(parent_np,&dma_addr,&paddr,&size);
>>   >>  +	if (ret<   0) {
>>   >>  +		dma_addr = offset = 0;
>>   >>  +		size = dev->coherent_dma_mask + 1;
>
> Right.
>
>>>> +	} else {
>>>> +		offset = PFN_DOWN(paddr - dma_addr);
>>>> +		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
>>>> +	}
>>>> +	dev->dma_pfn_offset = offset;
>>>> +
>>>> +	coherent = of_dma_is_coherent(parent_np);
>>>> +	dev_dbg(dev, "device is%sdma coherent\n",
>>>> +		coherent ? " " : " not ");
>>>> +
>>>> +	arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>>>
>>> Basically, I would use limit the size argument we pass into
>>> arch_setup_dma_ops to the minimum of 'size' and 'dma_mask' here,
>>> after converting into the same format. We should make sure we do the
>>> same thing for platform_device as well, so it might be better to do
>>> it inside of arch_setup_dma_ops instead.
>>
>> Do you think following changes will work?
>>
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64
>> dma_base, u64 size,
>>                           struct iommu_ops *iommu, bool coherent)
>>    {
>>           struct dma_map_ops *dma_ops;
>> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
>>
>>           dev->archdata.dma_coherent = coherent;
>> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
>> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
>>
>> If you agree, I will post v1 of the patch with these updates. Let me
>> know. I did some basic tests on Keystone with these changes and it works
>> fine.
>
> It's not exactly what I meant. My main point was that we need to limit
> dev->dma_mask to (size-1) here, but you are not touching that.

if you mean overriding the dev->dma_mask to min((*dev->dma_mask), 
size-1), then I am getting the error "Coherent DMA mask 0x7fffffff (pfn 
0x780000-0x800000) covers a smaller range of system memory than the DMA 
zone pfn 0x0-0x880000) when the devices on Keystone tries to set the dma 
mask. Something wrong and I need to look into the code.

> either change the two functions in which we first assign the dma_mask
> (platform and pci bus), or set it again in arch_setup_dma_ops (on each
> architecture implementing it), either way would work. Someone else
> might have a stronger opinion on that matter.
>
> For arm_setup_iommu_dma_ops, passing the original size is probably
> best.
>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Monday 22 December 2014 16:40:43 Murali Karicheri wrote:
> >> +++ b/arch/arm/mm/dma-mapping.c
> >> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64
> >> dma_base, u64 size,
> >>                           struct iommu_ops *iommu, bool coherent)
> >>    {
> >>           struct dma_map_ops *dma_ops;
> >> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
> >>
> >>           dev->archdata.dma_coherent = coherent;
> >> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> >> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
> >>
> >> If you agree, I will post v1 of the patch with these updates. Let me
> >> know. I did some basic tests on Keystone with these changes and it works
> >> fine.
> >
> > It's not exactly what I meant. My main point was that we need to limit
> > dev->dma_mask to (size-1) here, but you are not touching that.
> 
> if you mean overriding the dev->dma_mask to min((*dev->dma_mask), 
> size-1), then I am getting the error "Coherent DMA mask 0x7fffffff (pfn 
> 0x780000-0x800000) covers a smaller range of system memory than the DMA 
> zone pfn 0x0-0x880000) when the devices on Keystone tries to set the dma 
> mask. Something wrong and I need to look into the code.

Right, it sounds like the offset was applied incorrectly at some point.

What are the DMA zone size and the phys-offset?

	Arnd

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 12/22/2014 05:24 PM, Arnd Bergmann wrote:
> On Monday 22 December 2014 16:40:43 Murali Karicheri wrote:
>>>> +++ b/arch/arm/mm/dma-mapping.c
>>>> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64
>>>> dma_base, u64 size,
>>>>                            struct iommu_ops *iommu, bool coherent)
>>>>     {
>>>>            struct dma_map_ops *dma_ops;
>>>> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
>>>>
>>>>            dev->archdata.dma_coherent = coherent;
>>>> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
>>>> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
>>>>
>>>> If you agree, I will post v1 of the patch with these updates. Let me
>>>> know. I did some basic tests on Keystone with these changes and it works
>>>> fine.
>>>
>>> It's not exactly what I meant. My main point was that we need to limit
>>> dev->dma_mask to (size-1) here, but you are not touching that.
>>
>> if you mean overriding the dev->dma_mask to min((*dev->dma_mask),
>> size-1), then I am getting the error "Coherent DMA mask 0x7fffffff (pfn
>> 0x780000-0x800000) covers a smaller range of system memory than the DMA
>> zone pfn 0x0-0x880000) when the devices on Keystone tries to set the dma
>> mask. Something wrong and I need to look into the code.
>
> Right, it sounds like the offset was applied incorrectly at some point.
>
> What are the DMA zone size and the phys-offset?
2G and 0x8_0000_0000. This limit the usable DMA size to 2G on Keystone, 
I believe you shouldn't be limiting the dma mask to size-1 in this case, 
right? The DT setup the dma-range to have a size of 2G (0x80000000).

Murali
>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Monday 22 December 2014 17:40:30 Murali Karicheri wrote:
> On 12/22/2014 05:24 PM, Arnd Bergmann wrote:
> > On Monday 22 December 2014 16:40:43 Murali Karicheri wrote:
> >>>> +++ b/arch/arm/mm/dma-mapping.c
> >>>> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64
> >>>> dma_base, u64 size,
> >>>>                            struct iommu_ops *iommu, bool coherent)
> >>>>     {
> >>>>            struct dma_map_ops *dma_ops;
> >>>> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
> >>>>
> >>>>            dev->archdata.dma_coherent = coherent;
> >>>> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> >>>> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
> >>>>
> >>>> If you agree, I will post v1 of the patch with these updates. Let me
> >>>> know. I did some basic tests on Keystone with these changes and it works
> >>>> fine.
> >>>
> >>> It's not exactly what I meant. My main point was that we need to limit
> >>> dev->dma_mask to (size-1) here, but you are not touching that.
> >>
> >> if you mean overriding the dev->dma_mask to min((*dev->dma_mask),
> >> size-1), then I am getting the error "Coherent DMA mask 0x7fffffff (pfn
> >> 0x780000-0x800000) covers a smaller range of system memory than the DMA
> >> zone pfn 0x0-0x880000) when the devices on Keystone tries to set the dma
> >> mask. Something wrong and I need to look into the code.
> >
> > Right, it sounds like the offset was applied incorrectly at some point.
> >
> > What are the DMA zone size and the phys-offset?
> 2G and 0x8_0000_0000. This limit the usable DMA size to 2G on Keystone, 
> I believe you shouldn't be limiting the dma mask to size-1 in this case, 
> right? The DT setup the dma-range to have a size of 2G (0x80000000).

No, the problem is something else: the pfn range that we calculate for the
coherent DMA mask should have been 0x800000-0x880000, not 0x780000-0x800000,
so we would exactly match the ZONE_DMA.

	Arnd

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Monday 22 December 2014 23:44:52 Arnd Bergmann wrote:
> On Monday 22 December 2014 17:40:30 Murali Karicheri wrote:
> > On 12/22/2014 05:24 PM, Arnd Bergmann wrote:
> > > On Monday 22 December 2014 16:40:43 Murali Karicheri wrote:
> > >>>> +++ b/arch/arm/mm/dma-mapping.c
> > >>>> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64
> > >>>> dma_base, u64 size,
> > >>>>                            struct iommu_ops *iommu, bool coherent)
> > >>>>     {
> > >>>>            struct dma_map_ops *dma_ops;
> > >>>> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
> > >>>>
> > >>>>            dev->archdata.dma_coherent = coherent;
> > >>>> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> > >>>> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
> > >>>>
> > >>>> If you agree, I will post v1 of the patch with these updates. Let me
> > >>>> know. I did some basic tests on Keystone with these changes and it works
> > >>>> fine.
> > >>>
> > >>> It's not exactly what I meant. My main point was that we need to limit
> > >>> dev->dma_mask to (size-1) here, but you are not touching that.
> > >>
> > >> if you mean overriding the dev->dma_mask to min((*dev->dma_mask),
> > >> size-1), then I am getting the error "Coherent DMA mask 0x7fffffff (pfn
> > >> 0x780000-0x800000) covers a smaller range of system memory than the DMA
> > >> zone pfn 0x0-0x880000) when the devices on Keystone tries to set the dma
> > >> mask. Something wrong and I need to look into the code.
> > >
> > > Right, it sounds like the offset was applied incorrectly at some point.
> > >
> > > What are the DMA zone size and the phys-offset?
> > 2G and 0x8_0000_0000. This limit the usable DMA size to 2G on Keystone, 
> > I believe you shouldn't be limiting the dma mask to size-1 in this case, 
> > right? The DT setup the dma-range to have a size of 2G (0x80000000).
> 
> No, the problem is something else: the pfn range that we calculate for the
> coherent DMA mask should have been 0x800000-0x880000, not 0x780000-0x800000,
> so we would exactly match the ZONE_DMA.

I gave it some more thought, and concluded that the size that gets passed
down is not really the right value to be compared to a mask if the dma-capable
area starts at a nonzero bus address.

In your case, bus addresses 0x80000000-0xffffffff are valid for DMA, so
the mask must be 0xffffffff to forbid any DMA to addresses larger than
0x100000000, not 0x7fffffff which would not be enough to cover any RAM.

I guess the dma_mask should be 'min((*dev->dma_mask), dma_base + size - 1)'
here.

	Arnd

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 12/22/2014 06:07 PM, Arnd Bergmann wrote:
> On Monday 22 December 2014 23:44:52 Arnd Bergmann wrote:
>> On Monday 22 December 2014 17:40:30 Murali Karicheri wrote:
>>> On 12/22/2014 05:24 PM, Arnd Bergmann wrote:
>>>> On Monday 22 December 2014 16:40:43 Murali Karicheri wrote:
>>>>>>> +++ b/arch/arm/mm/dma-mapping.c
>>>>>>> @@ -2052,9 +2052,10 @@ void arch_setup_dma_ops(struct device *dev, u64
>>>>>>> dma_base, u64 size,
>>>>>>>                             struct iommu_ops *iommu, bool coherent)
>>>>>>>      {
>>>>>>>             struct dma_map_ops *dma_ops;
>>>>>>> +       u64 temp_size = min((*(dev->dma_mask) + 1), size);
>>>>>>>
>>>>>>>             dev->archdata.dma_coherent = coherent;
>>>>>>> -       if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
>>>>>>> +       if (arm_setup_iommu_dma_ops(dev, dma_base, temp_size, iommu))
>>>>>>>
>>>>>>> If you agree, I will post v1 of the patch with these updates. Let me
>>>>>>> know. I did some basic tests on Keystone with these changes and it works
>>>>>>> fine.
>>>>>>
>>>>>> It's not exactly what I meant. My main point was that we need to limit
>>>>>> dev->dma_mask to (size-1) here, but you are not touching that.
>>>>>
>>>>> if you mean overriding the dev->dma_mask to min((*dev->dma_mask),
>>>>> size-1), then I am getting the error "Coherent DMA mask 0x7fffffff (pfn
>>>>> 0x780000-0x800000) covers a smaller range of system memory than the DMA
>>>>> zone pfn 0x0-0x880000) when the devices on Keystone tries to set the dma
>>>>> mask. Something wrong and I need to look into the code.
>>>>
>>>> Right, it sounds like the offset was applied incorrectly at some point.
>>>>
>>>> What are the DMA zone size and the phys-offset?
>>> 2G and 0x8_0000_0000. This limit the usable DMA size to 2G on Keystone,
>>> I believe you shouldn't be limiting the dma mask to size-1 in this case,
>>> right? The DT setup the dma-range to have a size of 2G (0x80000000).
>>
>> No, the problem is something else: the pfn range that we calculate for the
>> coherent DMA mask should have been 0x800000-0x880000, not 0x780000-0x800000,
>> so we would exactly match the ZONE_DMA.
>
> I gave it some more thought, and concluded that the size that gets passed
> down is not really the right value to be compared to a mask if the dma-capable
> area starts at a nonzero bus address.
>
> In your case, bus addresses 0x80000000-0xffffffff are valid for DMA, so
> the mask must be 0xffffffff to forbid any DMA to addresses larger than
> 0x100000000, not 0x7fffffff which would not be enough to cover any RAM.
>
> I guess the dma_mask should be 'min((*dev->dma_mask), dma_base + size - 1)'
> here.

Arnd,

I guess so. Besides we need to keep the default coherent dma mask to 
32bit 0xffffffffull as well to work on Keystone and also in sync with 
current defaults used in pci_device_add() so that we don't break this 
behavior.

Here is the summary of changes need to make on top of my existing patch.

1. of_dma_configure() - change size = dev->coherent_dma_mask to size = 
dev->coherent_dma_mask + 1. This is a new patch to fix existing code.

2. Do the above change to of_pci_dma_configure() as well.

3. in arch_setup_dma_ops() update the DMA mask to min((*dev->dma_mask), 
dma_base + size - 1) as


diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c17f6a9..88b4769 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2053,8 +2053,13 @@ void arch_setup_dma_ops(struct device *dev, u64 
dma_base, u64 size,
  {
         struct dma_map_ops *dma_ops;

+
+       *dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));

I have tested this on keystone and it works fine with rootfs on PCI SATA 
harddisk. I will be doing more tests with this change. If you are in 
agreement with the above changes, I will re-spin the patch to accomodate 
them and send v1 of the same.

Regards,

Murali

>
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Murali Karicheri
Linux Kernel, Texas Instruments
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Tuesday 23 December 2014 12:42:05 Murali Karicheri wrote:
> > here.
> 
> Arnd,
> 
> I guess so. Besides we need to keep the default coherent dma mask to 
> 32bit 0xffffffffull as well to work on Keystone and also in sync with 
> current defaults used in pci_device_add() so that we don't break this 
> behavior.
> 
> Here is the summary of changes need to make on top of my existing patch.
> 
> 1. of_dma_configure() - change size = dev->coherent_dma_mask to size = 
> dev->coherent_dma_mask + 1. This is a new patch to fix existing code.

ok

> 2. Do the above change to of_pci_dma_configure() as well.

ok

> 3. in arch_setup_dma_ops() update the DMA mask to min((*dev->dma_mask), 
> dma_base + size - 1) as
> 
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index c17f6a9..88b4769 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2053,8 +2053,13 @@ void arch_setup_dma_ops(struct device *dev, u64 
> dma_base, u64 size,
>   {
>          struct dma_map_ops *dma_ops;
> 
> +
> +       *dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
> 
> I have tested this on keystone and it works fine with rootfs on PCI SATA 
> harddisk. I will be doing more tests with this change. If you are in 
> agreement with the above changes, I will re-spin the patch to accomodate 
> them and send v1 of the same.

Ok, sounds good. I noticed that you did not put Russell into the Cc list
for the emails. Please do so when you send it again, as he may have some
additional comments.

	Arnd

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 12/23/2014 05:42 PM, Arnd Bergmann wrote:
> On Tuesday 23 December 2014 12:42:05 Murali Karicheri wrote:
>>> here.
>>
>> Arnd,
>>
>> I guess so. Besides we need to keep the default coherent dma mask to
>> 32bit 0xffffffffull as well to work on Keystone and also in sync with
>> current defaults used in pci_device_add() so that we don't break this
>> behavior.
>>
>> Here is the summary of changes need to make on top of my existing patch.
>>
>> 1. of_dma_configure() - change size = dev->coherent_dma_mask to size =
>> dev->coherent_dma_mask + 1. This is a new patch to fix existing code.
>
> ok
>
>> 2. Do the above change to of_pci_dma_configure() as well.
>
> ok
>
>> 3. in arch_setup_dma_ops() update the DMA mask to min((*dev->dma_mask),
>> dma_base + size - 1) as
>>
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index c17f6a9..88b4769 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -2053,8 +2053,13 @@ void arch_setup_dma_ops(struct device *dev, u64
>> dma_base, u64 size,
>>    {
>>           struct dma_map_ops *dma_ops;
>>
>> +
>> +       *dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
>>
>> I have tested this on keystone and it works fine with rootfs on PCI SATA
>> harddisk. I will be doing more tests with this change. If you are in
>> agreement with the above changes, I will re-spin the patch to accomodate
>> them and send v1 of the same.
>
> Ok, sounds good. I noticed that you did not put Russell into the Cc list
> for the emails. Please do so when you send it again, as he may have some
> additional comments.
Ok. Will do.

Murali
>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 PATCH 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 12/23/2014 05:42 PM, Arnd Bergmann wrote:
> On Tuesday 23 December 2014 12:42:05 Murali Karicheri wrote:
>>> here.
>>
>> Arnd,
>>
>> I guess so. Besides we need to keep the default coherent dma mask to
>> 32bit 0xffffffffull as well to work on Keystone and also in sync with
>> current defaults used in pci_device_add() so that we don't break this
>> behavior.
>>
>> Here is the summary of changes need to make on top of my existing patch.
>>
>> 1. of_dma_configure() - change size = dev->coherent_dma_mask to size =
>> dev->coherent_dma_mask + 1. This is a new patch to fix existing code.
>
> ok
>
>> 2. Do the above change to of_pci_dma_configure() as well.
>
> ok
>
>> 3. in arch_setup_dma_ops() update the DMA mask to min((*dev->dma_mask),
>> dma_base + size - 1) as
>>
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index c17f6a9..88b4769 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -2053,8 +2053,13 @@ void arch_setup_dma_ops(struct device *dev, u64
>> dma_base, u64 size,
>>    {
>>           struct dma_map_ops *dma_ops;
>>
>> +
>> +       *dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
>>
>> I have tested this on keystone and it works fine with rootfs on PCI SATA
>> harddisk. I will be doing more tests with this change. If you are in
>> agreement with the above changes, I will re-spin the patch to accomodate
>> them and send v1 of the same.
>
> Ok, sounds good. I noticed that you did not put Russell into the Cc list
> for the emails. Please do so when you send it again, as he may have some
> additional comments.
>
> 	Arnd
Arnd,

Can I add "Reviewed-by: Arnd Bergmann <arnd@arndb.de>" when I send the 
v2 of the patch? I want to send this today. So please respond.

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

[RFC v1 PATCH 2/2] PCI: update dma configuration from DT

[Murali Karicheri]

if there is a DT node available for the root bridge's parent device,
use the dma configuration from that device node. For example, keystone
PCI devices would require dma_pfn_offset to be set correctly in the
device structure of the pci device in order to have the correct dma mask.
The DT node will have dma-ranges defined for this. This also support using
the DT property dma-coherent to allow coherent DMA operation by the
PCI device.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/pci/probe.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 23212f8..d7dcd6c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/of_pci.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -1520,6 +1521,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
 	dev->dev.coherent_dma_mask = 0xffffffffull;
+	of_pci_dma_configure(dev);
 
 	pci_set_dma_max_seg_size(dev, 65536);
 	pci_set_dma_seg_boundary(dev, 0xffffffff);
-- 
1.7.9.5