linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* dma_mask limited to 32-bits with OF platform device
@ 2020-02-12 10:49 Roger Quadros
  2020-02-12 11:37 ` Robin Murphy
  0 siblings, 1 reply; 18+ messages in thread
From: Roger Quadros @ 2020-02-12 10:49 UTC (permalink / raw)
  To: robin.murphy, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman
  Cc: stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon

Hi,

I'd like to understand why of_dma_configure() is limiting the dma and coherent masks
instead of overriding them.

see commits
a5516219b102 ("of/platform: Initialise default DMA masks")
ee7b1f31200d ("of: fix DMA mask generation")

In of_platform_device_create_pdata(), we initialize both masks to 32-bits unconditionally,
which is fine to support legacy cases.

	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
         if (!dev->dev.dma_mask)
                 dev->dev.dma_mask = &dev->dev.coherent_dma_mask;

Then in of_dma_configure() we limit it like so.

         dev->coherent_dma_mask &= mask;
         *dev->dma_mask &= mask;

This way, legitimate devices which correctly set dma-ranges in DT
will never get a dma_mask above 32-bits at all. How is this expected to work?

For a test, I added this in dra7.dtsi sata node. (NOTE: CONFIG_ARM_LPAE=y)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 93aa65c75b45..cd8c6cea23d5 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -571,6 +571,8 @@
  		sata: sata@4a141100 {
  			compatible = "snps,dwc-ahci";
  			reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
+			#size-cells = <2>;
+			dma-ranges = <0x00000000 0x00000000 0x10 0x00000000>;
  			interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
  			phys = <&sata_phy>;
  			phy-names = "sata-phy";

----------------------------- drivers/of/device.c -----------------------------
index e9127db7b067..1072cebad57a 100644
@@ -95,6 +95,7 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
  	const struct iommu_ops *iommu;
  	u64 mask, end;
  
+	dev_info(dev, "of_dma_configure\n");
  	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
  	if (ret < 0) {
  		/*
@@ -123,7 +124,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
  			dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
  			return -EINVAL;
  		}
-		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
+		dev_info(dev, "dma %llx paddr %llx size %llx\n", dma_addr, paddr, size);
+		dev_info(dev, "dma_pfn_offset(%#08lx)\n", offset);
  	}
  
  	/*
@@ -152,6 +154,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
  	mask = DMA_BIT_MASK(ilog2(end) + 1);
  	dev->coherent_dma_mask &= mask;
  	*dev->dma_mask &= mask;
+
+	dev_info(dev, "end %llx, mask %llx\n", end, mask);
  	/* ...but only set bus limit if we found valid dma-ranges earlier */
  	if (!ret)
  		dev->bus_dma_limit = end;

And I see.

[    1.134294]  4a140000.sata: of_platform
[   13.203917] ahci 4a140000.sata: of_dma_configure
[   13.225635] ahci 4a140000.sata: dma 0 paddr 0 size 1000000000
[   13.266178] ahci 4a140000.sata: dma_pfn_offset(0x000000)
[   13.297621] ahci 4a140000.sata: end fffffffff, mask fffffffff
[   13.585499] ahci 4a140000.sata: dma_mask 0xffffffff, coherent_mask 0xffffffff
[   13.599082] ahci 4a140000.sata: setting 64-bit mask ffffffffffffffff

Truncation of dma_mask and coherent_mask is undesired in this case.

How about fixing it like so?

- 	dev->coherent_dma_mask &= mask;
-	*dev->dma_mask &= mask;
+ 	dev->coherent_dma_mask = mask;
+ 	*dev->dma_mask = mask;

Also this comment doesn't make sense anymore?

         /*
          * Limit coherent and dma mask based on size and default mask
          * set by the driver.
          */

cheers,
-roger
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-12 10:49 dma_mask limited to 32-bits with OF platform device Roger Quadros
@ 2020-02-12 11:37 ` Robin Murphy
  2020-02-12 12:33   ` Roger Quadros
  0 siblings, 1 reply; 18+ messages in thread
From: Robin Murphy @ 2020-02-12 11:37 UTC (permalink / raw)
  To: Roger Quadros, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman
  Cc: stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon

On 2020-02-12 10:49 am, Roger Quadros wrote:
> Hi,
> 
> I'd like to understand why of_dma_configure() is limiting the dma and 
> coherent masks
> instead of overriding them.
> 
> see commits
> a5516219b102 ("of/platform: Initialise default DMA masks")
> ee7b1f31200d ("of: fix DMA mask generation")
> 
> In of_platform_device_create_pdata(), we initialize both masks to 
> 32-bits unconditionally,
> which is fine to support legacy cases.
> 
>      dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>          if (!dev->dev.dma_mask)
>                  dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> 
> Then in of_dma_configure() we limit it like so.
> 
>          dev->coherent_dma_mask &= mask;
>          *dev->dma_mask &= mask;
> 
> This way, legitimate devices which correctly set dma-ranges in DT
> will never get a dma_mask above 32-bits at all. How is this expected to 
> work?

Because these are still just the *default* masks - although drivers are 
all expected to call dma_set_mask() and friends explicitly nowadays, 
there are sure to be some out there for 32-bit devices still assuming 
the default 32-bit masks are in place. Thus if platform code secretly 
makes them larger, Bad Things ensue. Making them *smaller* where there 
are platform limitations shouldn't really matter now that we have the 
bus_dma_limit mechanism working well, but also doesn't do any harm, so 
it was left in for good measure.

The current paradigm is that the device masks represent the inherent 
capability of the device as far as the driver knows, and external 
interconnect constraints are kept separately as private DMA API 
internals via the bus limit.

> For a test, I added this in dra7.dtsi sata node. (NOTE: CONFIG_ARM_LPAE=y)
> 
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index 93aa65c75b45..cd8c6cea23d5 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -571,6 +571,8 @@
>           sata: sata@4a141100 {
>               compatible = "snps,dwc-ahci";
>               reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
> +            #size-cells = <2>;
> +            dma-ranges = <0x00000000 0x00000000 0x10 0x00000000>;
>               interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>               phys = <&sata_phy>;
>               phy-names = "sata-phy";
> 
> ----------------------------- drivers/of/device.c 
> -----------------------------
> index e9127db7b067..1072cebad57a 100644
> @@ -95,6 +95,7 @@ int of_dma_configure(struct device *dev, struct 
> device_node *np, bool force_dma)
>       const struct iommu_ops *iommu;
>       u64 mask, end;
> 
> +    dev_info(dev, "of_dma_configure\n");
>       ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>       if (ret < 0) {
>           /*
> @@ -123,7 +124,8 @@ int of_dma_configure(struct device *dev, struct 
> device_node *np, bool force_dma)
>               dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
>               return -EINVAL;
>           }
> -        dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> +        dev_info(dev, "dma %llx paddr %llx size %llx\n", dma_addr, 
> paddr, size);
> +        dev_info(dev, "dma_pfn_offset(%#08lx)\n", offset);
>       }
> 
>       /*
> @@ -152,6 +154,8 @@ int of_dma_configure(struct device *dev, struct 
> device_node *np, bool force_dma)
>       mask = DMA_BIT_MASK(ilog2(end) + 1);
>       dev->coherent_dma_mask &= mask;
>       *dev->dma_mask &= mask;
> +
> +    dev_info(dev, "end %llx, mask %llx\n", end, mask);
>       /* ...but only set bus limit if we found valid dma-ranges earlier */
>       if (!ret)
>           dev->bus_dma_limit = end;
> 
> And I see.
> 
> [    1.134294]  4a140000.sata: of_platform
> [   13.203917] ahci 4a140000.sata: of_dma_configure
> [   13.225635] ahci 4a140000.sata: dma 0 paddr 0 size 1000000000
> [   13.266178] ahci 4a140000.sata: dma_pfn_offset(0x000000)
> [   13.297621] ahci 4a140000.sata: end fffffffff, mask fffffffff
> [   13.585499] ahci 4a140000.sata: dma_mask 0xffffffff, coherent_mask 
> 0xffffffff
> [   13.599082] ahci 4a140000.sata: setting 64-bit mask ffffffffffffffff
> 
> Truncation of dma_mask and coherent_mask is undesired in this case.

Again, it's only the initial default masks that are truncated, and the 
driver appropriately replaces them with its 64-bit masks anyway once it 
probes. However, bus_dma_limit should still reflect that 36-bit upstream 
constraint, and that's what really matters. If you've found a path 
through a DMA API implementation which is subsequently failing to 
respect that limit, that's a bug in that implementation.

> How about fixing it like so?
> 
> -     dev->coherent_dma_mask &= mask;
> -    *dev->dma_mask &= mask;
> +     dev->coherent_dma_mask = mask;
> +     *dev->dma_mask = mask;

As above, making the "32-bit default" larger than 32 bits stands to 
break 32-bit devices with old drivers, and there's nothing to "fix" at 
this point anyway.

> Also this comment doesn't make sense anymore?
> 
>          /*
>           * Limit coherent and dma mask based on size and default mask
>           * set by the driver.
>           */

TBH that's never made much sense, unless "driver" was supposed to refer 
to bus code. Its continued presence is down to inertia more than any 
other reason :)

Robin.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-12 11:37 ` Robin Murphy
@ 2020-02-12 12:33   ` Roger Quadros
  2020-02-12 14:04     ` Robin Murphy
  0 siblings, 1 reply; 18+ messages in thread
From: Roger Quadros @ 2020-02-12 12:33 UTC (permalink / raw)
  To: Robin Murphy, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman
  Cc: stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon, hdegoede

On 12/02/2020 13:37, Robin Murphy wrote:
> On 2020-02-12 10:49 am, Roger Quadros wrote:
>> Hi,
>>
>> I'd like to understand why of_dma_configure() is limiting the dma and coherent masks
>> instead of overriding them.
>>
>> see commits
>> a5516219b102 ("of/platform: Initialise default DMA masks")
>> ee7b1f31200d ("of: fix DMA mask generation")
>>
>> In of_platform_device_create_pdata(), we initialize both masks to 32-bits unconditionally,
>> which is fine to support legacy cases.
>>
>>      dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>          if (!dev->dev.dma_mask)
>>                  dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>>
>> Then in of_dma_configure() we limit it like so.
>>
>>          dev->coherent_dma_mask &= mask;
>>          *dev->dma_mask &= mask;
>>
>> This way, legitimate devices which correctly set dma-ranges in DT
>> will never get a dma_mask above 32-bits at all. How is this expected to work?
> 
> Because these are still just the *default* masks - although drivers are all expected to call dma_set_mask() and friends explicitly nowadays, there are sure to be some out there for 32-bit devices still assuming the default 32-bit masks are in place. Thus if platform code secretly makes them larger, Bad Things ensue. Making them *smaller* where there are platform limitations shouldn't really matter now that we have the bus_dma_limit mechanism working well, but also doesn't do any harm, so it was left in for good measure.
> 
> The current paradigm is that the device masks represent the inherent capability of the device as far as the driver knows, and external interconnect constraints are kept separately as private DMA API internals via the bus limit.
> 
>> For a test, I added this in dra7.dtsi sata node. (NOTE: CONFIG_ARM_LPAE=y)
>>
>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>> index 93aa65c75b45..cd8c6cea23d5 100644
>> --- a/arch/arm/boot/dts/dra7.dtsi
>> +++ b/arch/arm/boot/dts/dra7.dtsi
>> @@ -571,6 +571,8 @@
>>           sata: sata@4a141100 {
>>               compatible = "snps,dwc-ahci";
>>               reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>> +            #size-cells = <2>;
>> +            dma-ranges = <0x00000000 0x00000000 0x10 0x00000000>;
>>               interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>>               phys = <&sata_phy>;
>>               phy-names = "sata-phy";
>>
>> ----------------------------- drivers/of/device.c -----------------------------
>> index e9127db7b067..1072cebad57a 100644
>> @@ -95,6 +95,7 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>>       const struct iommu_ops *iommu;
>>       u64 mask, end;
>>
>> +    dev_info(dev, "of_dma_configure\n");
>>       ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>>       if (ret < 0) {
>>           /*
>> @@ -123,7 +124,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>>               dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
>>               return -EINVAL;
>>           }
>> -        dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> +        dev_info(dev, "dma %llx paddr %llx size %llx\n", dma_addr, paddr, size);
>> +        dev_info(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>       }
>>
>>       /*
>> @@ -152,6 +154,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
>>       mask = DMA_BIT_MASK(ilog2(end) + 1);
>>       dev->coherent_dma_mask &= mask;
>>       *dev->dma_mask &= mask;
>> +
>> +    dev_info(dev, "end %llx, mask %llx\n", end, mask);
>>       /* ...but only set bus limit if we found valid dma-ranges earlier */
>>       if (!ret)
>>           dev->bus_dma_limit = end;
>>
>> And I see.
>>
>> [    1.134294]  4a140000.sata: of_platform
>> [   13.203917] ahci 4a140000.sata: of_dma_configure
>> [   13.225635] ahci 4a140000.sata: dma 0 paddr 0 size 1000000000
>> [   13.266178] ahci 4a140000.sata: dma_pfn_offset(0x000000)
>> [   13.297621] ahci 4a140000.sata: end fffffffff, mask fffffffff
>> [   13.585499] ahci 4a140000.sata: dma_mask 0xffffffff, coherent_mask 0xffffffff
>> [   13.599082] ahci 4a140000.sata: setting 64-bit mask ffffffffffffffff
>>
>> Truncation of dma_mask and coherent_mask is undesired in this case.
> 
> Again, it's only the initial default masks that are truncated, and the driver appropriately replaces them with its 64-bit masks anyway once it probes. However, bus_dma_limit should still reflect that 36-bit upstream constraint, and that's what really matters. If you've found a path through a DMA API implementation which is subsequently failing to respect that limit, that's a bug in that implementation.
> 

For now, let's say that we limit dma-ranges to 4GB size. with "dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;"
Then, dma_bus_limit is set correctly to 0xffffffff, SATA driver sets masks to 64-bit as IP supports that.

[   13.306847] ahci 4a140000.sata: dma_mask 0xffffffffffffffff, coherent_mask 0xffffffffffffffff, dma_bus_limit 0xffffffff

However, the SATA controller still tries to do DMA above 32-bits.
dma_alloc() doesn't seem to be taking dma_bus_limit into account?

>> How about fixing it like so?
>>
>> -     dev->coherent_dma_mask &= mask;
>> -    *dev->dma_mask &= mask;
>> +     dev->coherent_dma_mask = mask;
>> +     *dev->dma_mask = mask;
> 
> As above, making the "32-bit default" larger than 32 bits stands to break 32-bit devices with old drivers, and there's nothing to "fix" at this point anyway.
> 
>> Also this comment doesn't make sense anymore?
>>
>>          /*
>>           * Limit coherent and dma mask based on size and default mask
>>           * set by the driver.
>>           */
> 
> TBH that's never made much sense, unless "driver" was supposed to refer to bus code. Its continued presence is down to inertia more than any other reason :)
> 
> Robin.

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-12 12:33   ` Roger Quadros
@ 2020-02-12 14:04     ` Robin Murphy
  2020-02-12 17:57       ` Christoph Hellwig
  2020-02-17 13:21       ` Christoph Hellwig
  0 siblings, 2 replies; 18+ messages in thread
From: Robin Murphy @ 2020-02-12 14:04 UTC (permalink / raw)
  To: Roger Quadros, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman
  Cc: stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon, hdegoede

On 12/02/2020 12:33 pm, Roger Quadros wrote:
[...]
> For now, let's say that we limit dma-ranges to 4GB size. with 
> "dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;"
> Then, dma_bus_limit is set correctly to 0xffffffff, SATA driver sets 
> masks to 64-bit as IP supports that.
> 
> [   13.306847] ahci 4a140000.sata: dma_mask 0xffffffffffffffff, 
> coherent_mask 0xffffffffffffffff, dma_bus_limit 0xffffffff
> 
> However, the SATA controller still tries to do DMA above 32-bits.
> dma_alloc() doesn't seem to be taking dma_bus_limit into account?

Yay ARM LPAE... Peter and Christoph have already been playing 
whack-a-mole with other bugs under that config - is this with or without 
SWIOTLB? (and whichever way, does the other work any better?)

Robin.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-12 14:04     ` Robin Murphy
@ 2020-02-12 17:57       ` Christoph Hellwig
  2020-02-17 13:21       ` Christoph Hellwig
  1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2020-02-12 17:57 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Roger Quadros, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman, stefan.wahren,
	afaerber, hverkuil, Rob Herring, devicetree, linux-kernel,
	Nishanth Menon, hdegoede

On Wed, Feb 12, 2020 at 02:04:31PM +0000, Robin Murphy wrote:
>> For now, let's say that we limit dma-ranges to 4GB size. with "dma-ranges 
>> = <0x00000000 0x00000000 0x1 0x00000000>;"
>> Then, dma_bus_limit is set correctly to 0xffffffff, SATA driver sets masks 
>> to 64-bit as IP supports that.
>>
>> [   13.306847] ahci 4a140000.sata: dma_mask 0xffffffffffffffff, 
>> coherent_mask 0xffffffffffffffff, dma_bus_limit 0xffffffff
>>
>> However, the SATA controller still tries to do DMA above 32-bits.
>> dma_alloc() doesn't seem to be taking dma_bus_limit into account?
>
> Yay ARM LPAE... Peter and Christoph have already been playing whack-a-mole 
> with other bugs under that config - is this with or without SWIOTLB? (and 
> whichever way, does the other work any better?)

Hmm.  ARM LPAE still uses the arm legacy dma alloc coherent, and that
might not be taking the dma_bus_limit into account.  Let me check..

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-12 14:04     ` Robin Murphy
  2020-02-12 17:57       ` Christoph Hellwig
@ 2020-02-17 13:21       ` Christoph Hellwig
  2020-02-17 14:54         ` Peter Ujfalusi
  2020-02-18  8:28         ` Roger Quadros
  1 sibling, 2 replies; 18+ messages in thread
From: Christoph Hellwig @ 2020-02-17 13:21 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Roger Quadros, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman, stefan.wahren,
	afaerber, hverkuil, Rob Herring, devicetree, linux-kernel,
	Nishanth Menon, hdegoede

Roger,

can you try the branch below and check if that helps?

    git://git.infradead.org/users/hch/misc.git arm-dma-bus-limit

Gitweb:

    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-dma-bus-limit

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-17 13:21       ` Christoph Hellwig
@ 2020-02-17 14:54         ` Peter Ujfalusi
  2020-02-18  7:26           ` Peter Ujfalusi
  2020-02-18  8:28         ` Roger Quadros
  1 sibling, 1 reply; 18+ messages in thread
From: Peter Ujfalusi @ 2020-02-17 14:54 UTC (permalink / raw)
  To: Christoph Hellwig, Robin Murphy
  Cc: Roger Quadros, Murali Karicheri, Nori, Sekhar, Anna, Suman,
	stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon, hdegoede

Christoph,

On 17/02/2020 15.21, Christoph Hellwig wrote:
> Roger,
> 
> can you try the branch below and check if that helps?
> 
>     git://git.infradead.org/users/hch/misc.git arm-dma-bus-limit
> 
> Gitweb:
> 
>     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-dma-bus-limit

I'll test them on k2 tomorrow ;)

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-17 14:54         ` Peter Ujfalusi
@ 2020-02-18  7:26           ` Peter Ujfalusi
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Ujfalusi @ 2020-02-18  7:26 UTC (permalink / raw)
  To: Christoph Hellwig, Robin Murphy
  Cc: Roger Quadros, Murali Karicheri, Nori, Sekhar, Anna, Suman,
	stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon, hdegoede

Christoph,

On 17/02/2020 16.54, Peter Ujfalusi wrote:
> Christoph,
> 
> On 17/02/2020 15.21, Christoph Hellwig wrote:
>> Roger,
>>
>> can you try the branch below and check if that helps?
>>
>>     git://git.infradead.org/users/hch/misc.git arm-dma-bus-limit
>>
>> Gitweb:
>>
>>     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-dma-bus-limit
> 
> I'll test them on k2 tomorrow ;)

fwiw, k2g works fine with the three patch applied. MMC uses ADMA, EDMA
probes and audio works.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-17 13:21       ` Christoph Hellwig
  2020-02-17 14:54         ` Peter Ujfalusi
@ 2020-02-18  8:28         ` Roger Quadros
  2020-02-18 17:22           ` Rob Herring
  1 sibling, 1 reply; 18+ messages in thread
From: Roger Quadros @ 2020-02-18  8:28 UTC (permalink / raw)
  To: Christoph Hellwig, Robin Murphy
  Cc: Péter Ujfalusi, Murali Karicheri, Nori, Sekhar, Anna, Suman,
	stefan.wahren, afaerber, hverkuil, Rob Herring, devicetree,
	linux-kernel, Nishanth Menon, hdegoede, Vignesh Raghavendra

Chrishtoph,

The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
have the below DT fix.

Do you intend to send these fixes to -stable?

------------------------- arch/arm/boot/dts/dra7.dtsi -------------------------
index d78b684e7fca..853ecf3cfb37 100644
@@ -645,6 +645,8 @@
  		sata: sata@4a141100 {
  			compatible = "snps,dwc-ahci";
  			reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
+			#size-cells = <2>;
+			dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;
  			interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
  			phys = <&sata_phy>;
  			phy-names = "sata-phy";


cheers,
-roger

On 17/02/2020 15:21, Christoph Hellwig wrote:
> Roger,
> 
> can you try the branch below and check if that helps?
> 
>      git://git.infradead.org/users/hch/misc.git arm-dma-bus-limit
> 
> Gitweb:
> 
>      http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-dma-bus-limit
> 

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-18  8:28         ` Roger Quadros
@ 2020-02-18 17:22           ` Rob Herring
  2020-02-19 14:29             ` Roger Quadros
  0 siblings, 1 reply; 18+ messages in thread
From: Rob Herring @ 2020-02-18 17:22 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Christoph Hellwig, Robin Murphy, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman, Stefan Wahren,
	Andreas Färber, Hans Verkuil, devicetree, linux-kernel,
	Nishanth Menon, hdegoede, Vignesh Raghavendra

On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <rogerq@ti.com> wrote:
>
> Chrishtoph,
>
> The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
> have the below DT fix.
>
> Do you intend to send these fixes to -stable?
>
> ------------------------- arch/arm/boot/dts/dra7.dtsi -------------------------
> index d78b684e7fca..853ecf3cfb37 100644
> @@ -645,6 +645,8 @@
>                 sata: sata@4a141100 {
>                         compatible = "snps,dwc-ahci";
>                         reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
> +                       #size-cells = <2>;
> +                       dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;

dma-ranges should be in the parent (bus) node, not the device node.

>                         interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>                         phys = <&sata_phy>;
>                         phy-names = "sata-phy";
>
>
> cheers,
> -roger
>
> On 17/02/2020 15:21, Christoph Hellwig wrote:
> > Roger,
> >
> > can you try the branch below and check if that helps?
> >
> >      git://git.infradead.org/users/hch/misc.git arm-dma-bus-limit
> >
> > Gitweb:
> >
> >      http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-dma-bus-limit
> >
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-18 17:22           ` Rob Herring
@ 2020-02-19 14:29             ` Roger Quadros
  2020-02-19 15:25               ` Robin Murphy
  0 siblings, 1 reply; 18+ messages in thread
From: Roger Quadros @ 2020-02-19 14:29 UTC (permalink / raw)
  To: Rob Herring
  Cc: Christoph Hellwig, Robin Murphy, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman, Stefan Wahren,
	Andreas Färber, Hans Verkuil, devicetree, linux-kernel,
	Nishanth Menon, hdegoede, Vignesh Raghavendra

Rob,

On 18/02/2020 19:22, Rob Herring wrote:
> On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <rogerq@ti.com> wrote:
>>
>> Chrishtoph,
>>
>> The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
>> have the below DT fix.
>>
>> Do you intend to send these fixes to -stable?
>>
>> ------------------------- arch/arm/boot/dts/dra7.dtsi -------------------------
>> index d78b684e7fca..853ecf3cfb37 100644
>> @@ -645,6 +645,8 @@
>>                  sata: sata@4a141100 {
>>                          compatible = "snps,dwc-ahci";
>>                          reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>> +                       #size-cells = <2>;
>> +                       dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;
> 
> dma-ranges should be in the parent (bus) node, not the device node.

I didn't understand why.

There are many devices on the parent bus node and all devices might not have the 32-bit DMA limit
the SATA controller has.

SATA controller is the bus master and the ATA devices are children of the SATA controller.

 From Documentation/devicetree/booting-without-of.txt

* DMA Bus master
Optional property:
- dma-ranges: <prop-encoded-array> encoded as arbitrary number of triplets of
         (child-bus-address, parent-bus-address, length). Each triplet specified
         describes a contiguous DMA address range.
         The dma-ranges property is used to describe the direct memory access (DMA)
         structure of a memory-mapped bus whose device tree parent can be accessed
         from DMA operations originating from the bus. It provides a means of
         defining a mapping or translation between the physical address space of
         the bus and the physical address space of the parent of the bus.
         (for more information see the Devicetree Specification)

* DMA Bus child
Optional property:
- dma-ranges: <empty> value. if present - It means that DMA addresses
         translation has to be enabled for this device.
- dma-coherent: Present if dma operations are coherent



> 
>>                          interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>>                          phys = <&sata_phy>;
>>                          phy-names = "sata-phy";
>>
>>
>> cheers,
>> -roger
>>
>> On 17/02/2020 15:21, Christoph Hellwig wrote:
>>> Roger,
>>>
>>> can you try the branch below and check if that helps?
>>>
>>>       git://git.infradead.org/users/hch/misc.git arm-dma-bus-limit
>>>
>>> Gitweb:
>>>
>>>       http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/arm-dma-bus-limit
>>>
>>
>> --
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-19 14:29             ` Roger Quadros
@ 2020-02-19 15:25               ` Robin Murphy
  2020-02-19 15:40                 ` Roger Quadros
  2020-02-26 11:33                 ` Roger Quadros
  0 siblings, 2 replies; 18+ messages in thread
From: Robin Murphy @ 2020-02-19 15:25 UTC (permalink / raw)
  To: Roger Quadros, Rob Herring
  Cc: Christoph Hellwig, Péter Ujfalusi, Murali Karicheri, Nori,
	Sekhar, Anna, Suman, Stefan Wahren, Andreas Färber,
	Hans Verkuil, devicetree, linux-kernel, Nishanth Menon, hdegoede,
	Vignesh Raghavendra

On 19/02/2020 2:29 pm, Roger Quadros wrote:
> Rob,
> 
> On 18/02/2020 19:22, Rob Herring wrote:
>> On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <rogerq@ti.com> wrote:
>>>
>>> Chrishtoph,
>>>
>>> The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
>>> have the below DT fix.
>>>
>>> Do you intend to send these fixes to -stable?
>>>
>>> ------------------------- arch/arm/boot/dts/dra7.dtsi 
>>> -------------------------
>>> index d78b684e7fca..853ecf3cfb37 100644
>>> @@ -645,6 +645,8 @@
>>>                  sata: sata@4a141100 {
>>>                          compatible = "snps,dwc-ahci";
>>>                          reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>>> +                       #size-cells = <2>;
>>> +                       dma-ranges = <0x00000000 0x00000000 0x1 
>>> 0x00000000>;
>>
>> dma-ranges should be in the parent (bus) node, not the device node.
> 
> I didn't understand why.
> 
> There are many devices on the parent bus node and all devices might not 
> have the 32-bit DMA limit
> the SATA controller has.
> 
> SATA controller is the bus master and the ATA devices are children of 
> the SATA controller.

But SATA is not a memory-mapped bus - in the context of MMIO, the AHCI 
is the bus-master device, not a bridge or level of interconnect. The 
DeviceTree spec[1] clearly defines dma-ranges as an address translation 
between a "parent bus" and a "child bus".

If in the worst case this address-limited interconnect really only 
exists between the AHCI's master interface and everything else in the 
system, then you'll have to describe it explicitly to meet DT's 
expectation of a "bus" (e.g. [2]). Yes, it's a bit clunky, but any 
scheme has its edge cases.

>  From Documentation/devicetree/booting-without-of.txt
> 
> * DMA Bus master
> Optional property:
> - dma-ranges: <prop-encoded-array> encoded as arbitrary number of 
> triplets of
>          (child-bus-address, parent-bus-address, length). Each triplet 
> specified
>          describes a contiguous DMA address range.
>          The dma-ranges property is used to describe the direct memory 
> access (DMA)
>          structure of a memory-mapped bus whose device tree parent can 
> be accessed
>          from DMA operations originating from the bus. It provides a 
> means of
>          defining a mapping or translation between the physical address 
> space of
>          the bus and the physical address space of the parent of the bus.
>          (for more information see the Devicetree Specification)
> 
> * DMA Bus child
> Optional property:
> - dma-ranges: <empty> value. if present - It means that DMA addresses
>          translation has to be enabled for this device.

Disregarding that this was apparently never in ePAPR, so not 
grandfathered in to DTSpec, and effectively nobody ever has actually 
followed it (oh, if only...), note "<empty>" - that still doesn't imply 
that a *non-empty* dma-ranges would be valid on device nodes.

Robin.

[1] https://www.devicetree.org/specifications/
[2] 
https://lore.kernel.org/lkml/20181010120737.30300-20-laurentiu.tudor@nxp.com/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-19 15:25               ` Robin Murphy
@ 2020-02-19 15:40                 ` Roger Quadros
  2020-02-26 11:33                 ` Roger Quadros
  1 sibling, 0 replies; 18+ messages in thread
From: Roger Quadros @ 2020-02-19 15:40 UTC (permalink / raw)
  To: Robin Murphy, Rob Herring
  Cc: Christoph Hellwig, Péter Ujfalusi, Murali Karicheri, Nori,
	Sekhar, Anna, Suman, Stefan Wahren, Andreas Färber,
	Hans Verkuil, devicetree, linux-kernel, Nishanth Menon, hdegoede,
	Vignesh Raghavendra



On 19/02/2020 17:25, Robin Murphy wrote:
> On 19/02/2020 2:29 pm, Roger Quadros wrote:
>> Rob,
>>
>> On 18/02/2020 19:22, Rob Herring wrote:
>>> On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <rogerq@ti.com> wrote:
>>>>
>>>> Chrishtoph,
>>>>
>>>> The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
>>>> have the below DT fix.
>>>>
>>>> Do you intend to send these fixes to -stable?
>>>>
>>>> ------------------------- arch/arm/boot/dts/dra7.dtsi -------------------------
>>>> index d78b684e7fca..853ecf3cfb37 100644
>>>> @@ -645,6 +645,8 @@
>>>>                  sata: sata@4a141100 {
>>>>                          compatible = "snps,dwc-ahci";
>>>>                          reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>>>> +                       #size-cells = <2>;
>>>> +                       dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;
>>>
>>> dma-ranges should be in the parent (bus) node, not the device node.
>>
>> I didn't understand why.
>>
>> There are many devices on the parent bus node and all devices might not have the 32-bit DMA limit
>> the SATA controller has.
>>
>> SATA controller is the bus master and the ATA devices are children of the SATA controller.
> 
> But SATA is not a memory-mapped bus - in the context of MMIO, the AHCI is the bus-master device, not a bridge or level of interconnect. The DeviceTree spec[1] clearly defines dma-ranges as an address translation between a "parent bus" and a "child bus".
> 
> If in the worst case this address-limited interconnect really only exists between the AHCI's master interface and everything else in the system, then you'll have to describe it explicitly to meet DT's expectation of a "bus" (e.g. [2]). Yes, it's a bit clunky, but any scheme has its edge cases.

I understand now. Thanks.

> 
>>  From Documentation/devicetree/booting-without-of.txt
>>
>> * DMA Bus master
>> Optional property:
>> - dma-ranges: <prop-encoded-array> encoded as arbitrary number of triplets of
>>          (child-bus-address, parent-bus-address, length). Each triplet specified
>>          describes a contiguous DMA address range.
>>          The dma-ranges property is used to describe the direct memory access (DMA)
>>          structure of a memory-mapped bus whose device tree parent can be accessed
>>          from DMA operations originating from the bus. It provides a means of
>>          defining a mapping or translation between the physical address space of
>>          the bus and the physical address space of the parent of the bus.
>>          (for more information see the Devicetree Specification)
>>
>> * DMA Bus child
>> Optional property:
>> - dma-ranges: <empty> value. if present - It means that DMA addresses
>>          translation has to be enabled for this device.
> 
> Disregarding that this was apparently never in ePAPR, so not grandfathered in to DTSpec, and effectively nobody ever has actually followed it (oh, if only...), note "<empty>" - that still doesn't imply that a *non-empty* dma-ranges would be valid on device nodes.
> 
> Robin.
> 
> [1] https://www.devicetree.org/specifications/
> [2] https://lore.kernel.org/lkml/20181010120737.30300-20-laurentiu.tudor@nxp.com/

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-19 15:25               ` Robin Murphy
  2020-02-19 15:40                 ` Roger Quadros
@ 2020-02-26 11:33                 ` Roger Quadros
  2020-03-03  8:27                   ` Roger Quadros
  1 sibling, 1 reply; 18+ messages in thread
From: Roger Quadros @ 2020-02-26 11:33 UTC (permalink / raw)
  To: Robin Murphy, Rob Herring
  Cc: Christoph Hellwig, Péter Ujfalusi, Murali Karicheri, Nori,
	Sekhar, Anna, Suman, Stefan Wahren, Andreas Färber,
	Hans Verkuil, devicetree, linux-kernel, Nishanth Menon, hdegoede,
	Vignesh Raghavendra

Hi,

On 19/02/2020 17:25, Robin Murphy wrote:
> On 19/02/2020 2:29 pm, Roger Quadros wrote:
>> Rob,
>>
>> On 18/02/2020 19:22, Rob Herring wrote:
>>> On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <rogerq@ti.com> wrote:
>>>>
>>>> Chrishtoph,
>>>>
>>>> The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
>>>> have the below DT fix.
>>>>
>>>> Do you intend to send these fixes to -stable?
>>>>
>>>> ------------------------- arch/arm/boot/dts/dra7.dtsi -------------------------
>>>> index d78b684e7fca..853ecf3cfb37 100644
>>>> @@ -645,6 +645,8 @@
>>>>                  sata: sata@4a141100 {
>>>>                          compatible = "snps,dwc-ahci";
>>>>                          reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>>>> +                       #size-cells = <2>;
>>>> +                       dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;
>>>
>>> dma-ranges should be in the parent (bus) node, not the device node.
>>
>> I didn't understand why.
>>
>> There are many devices on the parent bus node and all devices might not have the 32-bit DMA limit
>> the SATA controller has.
>>
>> SATA controller is the bus master and the ATA devices are children of the SATA controller.
> 
> But SATA is not a memory-mapped bus - in the context of MMIO, the AHCI is the bus-master device, not a bridge or level of interconnect. The DeviceTree spec[1] clearly defines dma-ranges as an address translation between a "parent bus" and a "child bus".
> 
> If in the worst case this address-limited interconnect really only exists between the AHCI's master interface and everything else in the system, then you'll have to describe it explicitly to meet DT's expectation of a "bus" (e.g. [2]). Yes, it's a bit clunky, but any scheme has its edge cases.
> 
>>  From Documentation/devicetree/booting-without-of.txt
>>
>> * DMA Bus master
>> Optional property:
>> - dma-ranges: <prop-encoded-array> encoded as arbitrary number of triplets of
>>          (child-bus-address, parent-bus-address, length). Each triplet specified
>>          describes a contiguous DMA address range.
>>          The dma-ranges property is used to describe the direct memory access (DMA)
>>          structure of a memory-mapped bus whose device tree parent can be accessed
>>          from DMA operations originating from the bus. It provides a means of
>>          defining a mapping or translation between the physical address space of
>>          the bus and the physical address space of the parent of the bus.
>>          (for more information see the Devicetree Specification)
>>
>> * DMA Bus child
>> Optional property:
>> - dma-ranges: <empty> value. if present - It means that DMA addresses
>>          translation has to be enabled for this device.
> 
> Disregarding that this was apparently never in ePAPR, so not grandfathered in to DTSpec, and effectively nobody ever has actually followed it (oh, if only...), note "<empty>" - that still doesn't imply that a *non-empty* dma-ranges would be valid on device nodes.
> 
> Robin.
> 
> [1] https://www.devicetree.org/specifications/
> [2] https://lore.kernel.org/lkml/20181010120737.30300-20-laurentiu.tudor@nxp.com/

With the patch (in the end). dev->bus_dma_limit is still set to 0 and so is not being used.

from of_dma_configure()
         ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
...
         /* ...but only set bus limit if we found valid dma-ranges earlier */
         if (!ret)
                 dev->bus_dma_limit = end;

There is no other place bus_dma_limit is set. Looks like every device should inherit that
from it's parent right?

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 64a0f90f5b52..5418c31d4da7 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -680,15 +680,22 @@
  		};
  
  		/* OCP2SCP3 */
-		sata: sata@4a141100 {
-			compatible = "snps,dwc-ahci";
-			reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
-			interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
-			phys = <&sata_phy>;
-			phy-names = "sata-phy";
-			clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
-			ti,hwmods = "sata";
-			ports-implemented = <0x1>;
+		sata_aux_bus {
+			#address-cells = <2>;
+			#size-cells = <2>;
+			compatible = "simple-bus";
+			ranges = <0x0 0x0 0x4a140000 0x0 0x1200>;
+			dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x00000000>;
+			sata: sata@4a141100 {
+				compatible = "snps,dwc-ahci";
+				reg = <0x0 0x0 0x0 0x1100>, <0x0 0x1100 0x0 0x7>;
+				interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+				phys = <&sata_phy>;
+				phy-names = "sata-phy";
+				clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
+				ti,hwmods = "sata";
+				ports-implemented = <0x1>;
+			};
  		};
  
  		/* OCP2SCP1 */

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-02-26 11:33                 ` Roger Quadros
@ 2020-03-03  8:27                   ` Roger Quadros
  2020-03-03 14:06                     ` Robin Murphy
  0 siblings, 1 reply; 18+ messages in thread
From: Roger Quadros @ 2020-03-03  8:27 UTC (permalink / raw)
  To: Robin Murphy, Rob Herring, Christoph Hellwig
  Cc: Péter Ujfalusi, Murali Karicheri, Nori, Sekhar, Anna, Suman,
	Stefan Wahren, Andreas Färber, Hans Verkuil, devicetree,
	linux-kernel, Nishanth Menon, hdegoede, Vignesh Raghavendra

Robin, Christoph,

On 26/02/2020 13:33, Roger Quadros wrote:
> Hi,
> 
> On 19/02/2020 17:25, Robin Murphy wrote:
>> On 19/02/2020 2:29 pm, Roger Quadros wrote:
>>> Rob,
>>>
>>> On 18/02/2020 19:22, Rob Herring wrote:
>>>> On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <rogerq@ti.com> wrote:
>>>>>
>>>>> Chrishtoph,
>>>>>
>>>>> The branch works fine for SATA on DRA7 with CONFIG_LPAE once I
>>>>> have the below DT fix.
>>>>>
>>>>> Do you intend to send these fixes to -stable?
>>>>>
>>>>> ------------------------- arch/arm/boot/dts/dra7.dtsi -------------------------
>>>>> index d78b684e7fca..853ecf3cfb37 100644
>>>>> @@ -645,6 +645,8 @@
>>>>>                  sata: sata@4a141100 {
>>>>>                          compatible = "snps,dwc-ahci";
>>>>>                          reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>>>>> +                       #size-cells = <2>;
>>>>> +                       dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;
>>>>
>>>> dma-ranges should be in the parent (bus) node, not the device node.
>>>
>>> I didn't understand why.
>>>
>>> There are many devices on the parent bus node and all devices might not have the 32-bit DMA limit
>>> the SATA controller has.
>>>
>>> SATA controller is the bus master and the ATA devices are children of the SATA controller.
>>
>> But SATA is not a memory-mapped bus - in the context of MMIO, the AHCI is the bus-master device, not a bridge or level of interconnect. The DeviceTree spec[1] clearly defines dma-ranges as an address translation between a "parent bus" and a "child bus".
>>
>> If in the worst case this address-limited interconnect really only exists between the AHCI's master interface and everything else in the system, then you'll have to describe it explicitly to meet DT's expectation of a "bus" (e.g. [2]). Yes, it's a bit clunky, but any scheme has its edge cases.
>>
>>>  From Documentation/devicetree/booting-without-of.txt
>>>
>>> * DMA Bus master
>>> Optional property:
>>> - dma-ranges: <prop-encoded-array> encoded as arbitrary number of triplets of
>>>          (child-bus-address, parent-bus-address, length). Each triplet specified
>>>          describes a contiguous DMA address range.
>>>          The dma-ranges property is used to describe the direct memory access (DMA)
>>>          structure of a memory-mapped bus whose device tree parent can be accessed
>>>          from DMA operations originating from the bus. It provides a means of
>>>          defining a mapping or translation between the physical address space of
>>>          the bus and the physical address space of the parent of the bus.
>>>          (for more information see the Devicetree Specification)
>>>
>>> * DMA Bus child
>>> Optional property:
>>> - dma-ranges: <empty> value. if present - It means that DMA addresses
>>>          translation has to be enabled for this device.
>>
>> Disregarding that this was apparently never in ePAPR, so not grandfathered in to DTSpec, and effectively nobody ever has actually followed it (oh, if only...), note "<empty>" - that still doesn't imply that a *non-empty* dma-ranges would be valid on device nodes.
>>
>> Robin.
>>
>> [1] https://www.devicetree.org/specifications/
>> [2] https://lore.kernel.org/lkml/20181010120737.30300-20-laurentiu.tudor@nxp.com/
> 
> With the patch (in the end). dev->bus_dma_limit is still set to 0 and so is not being used.
> 
> from of_dma_configure()
>          ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> ...
>          /* ...but only set bus limit if we found valid dma-ranges earlier */
>          if (!ret)
>                  dev->bus_dma_limit = end;
> 
> There is no other place bus_dma_limit is set. Looks like every device should inherit that
> from it's parent right?

Any ideas how to expect this to work?

> 
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index 64a0f90f5b52..5418c31d4da7 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -680,15 +680,22 @@
>           };
> 
>           /* OCP2SCP3 */
> -        sata: sata@4a141100 {
> -            compatible = "snps,dwc-ahci";
> -            reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
> -            interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> -            phys = <&sata_phy>;
> -            phy-names = "sata-phy";
> -            clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
> -            ti,hwmods = "sata";
> -            ports-implemented = <0x1>;
> +        sata_aux_bus {
> +            #address-cells = <2>;
> +            #size-cells = <2>;
> +            compatible = "simple-bus";
> +            ranges = <0x0 0x0 0x4a140000 0x0 0x1200>;
> +            dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x00000000>;
> +            sata: sata@4a141100 {
> +                compatible = "snps,dwc-ahci";
> +                reg = <0x0 0x0 0x0 0x1100>, <0x0 0x1100 0x0 0x7>;
> +                interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> +                phys = <&sata_phy>;
> +                phy-names = "sata-phy";
> +                clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
> +                ti,hwmods = "sata";
> +                ports-implemented = <0x1>;
> +            };
>           };
> 
>           /* OCP2SCP1 */
> 

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-03-03  8:27                   ` Roger Quadros
@ 2020-03-03 14:06                     ` Robin Murphy
  2020-03-03 19:26                       ` Rob Herring
  0 siblings, 1 reply; 18+ messages in thread
From: Robin Murphy @ 2020-03-03 14:06 UTC (permalink / raw)
  To: Roger Quadros, Rob Herring, Christoph Hellwig
  Cc: Péter Ujfalusi, Murali Karicheri, Nori, Sekhar, Anna, Suman,
	Stefan Wahren, Andreas Färber, Hans Verkuil, devicetree,
	linux-kernel, Nishanth Menon, hdegoede, Vignesh Raghavendra

On 03/03/2020 8:27 am, Roger Quadros wrote:
[...]
>> With the patch (in the end). dev->bus_dma_limit is still set to 0 and 
>> so is not being used.
>>
>> from of_dma_configure()
>>          ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> ...
>>          /* ...but only set bus limit if we found valid dma-ranges 
>> earlier */
>>          if (!ret)
>>                  dev->bus_dma_limit = end;
>>
>> There is no other place bus_dma_limit is set. Looks like every device 
>> should inherit that
>> from it's parent right?
> 
> Any ideas how to expect this to work?

Is of_dma_get_range() actually succeeding, or is it tripping up on some 
aspect of the DT (in which case there should be errors in the log)?

Looking again at the fragment below, are you sure it's correct? It 
appears to me like it might actually be defining a 1-byte-long DMA 
range, which indeed I wouldn't really expect to work.

Robin.

>>
>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>> index 64a0f90f5b52..5418c31d4da7 100644
>> --- a/arch/arm/boot/dts/dra7.dtsi
>> +++ b/arch/arm/boot/dts/dra7.dtsi
>> @@ -680,15 +680,22 @@
>>           };
>>
>>           /* OCP2SCP3 */
>> -        sata: sata@4a141100 {
>> -            compatible = "snps,dwc-ahci";
>> -            reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
>> -            interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>> -            phys = <&sata_phy>;
>> -            phy-names = "sata-phy";
>> -            clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
>> -            ti,hwmods = "sata";
>> -            ports-implemented = <0x1>;
>> +        sata_aux_bus {
>> +            #address-cells = <2>;
>> +            #size-cells = <2>;
>> +            compatible = "simple-bus";
>> +            ranges = <0x0 0x0 0x4a140000 0x0 0x1200>;
>> +            dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x00000000>;
>> +            sata: sata@4a141100 {
>> +                compatible = "snps,dwc-ahci";
>> +                reg = <0x0 0x0 0x0 0x1100>, <0x0 0x1100 0x0 0x7>;
>> +                interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>> +                phys = <&sata_phy>;
>> +                phy-names = "sata-phy";
>> +                clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
>> +                ti,hwmods = "sata";
>> +                ports-implemented = <0x1>;
>> +            };
>>           };
>>
>>           /* OCP2SCP1 */
>>
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-03-03 14:06                     ` Robin Murphy
@ 2020-03-03 19:26                       ` Rob Herring
  2020-03-04  8:28                         ` Roger Quadros
  0 siblings, 1 reply; 18+ messages in thread
From: Rob Herring @ 2020-03-03 19:26 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Roger Quadros, Christoph Hellwig, Péter Ujfalusi,
	Murali Karicheri, Nori, Sekhar, Anna, Suman, Stefan Wahren,
	Andreas Färber, Hans Verkuil, devicetree, linux-kernel,
	Nishanth Menon, hdegoede, Vignesh Raghavendra

On Tue, Mar 3, 2020 at 8:06 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 03/03/2020 8:27 am, Roger Quadros wrote:
> [...]
> >> With the patch (in the end). dev->bus_dma_limit is still set to 0 and
> >> so is not being used.
> >>
> >> from of_dma_configure()
> >>          ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> >> ...
> >>          /* ...but only set bus limit if we found valid dma-ranges
> >> earlier */
> >>          if (!ret)
> >>                  dev->bus_dma_limit = end;
> >>
> >> There is no other place bus_dma_limit is set. Looks like every device
> >> should inherit that
> >> from it's parent right?
> >
> > Any ideas how to expect this to work?
>
> Is of_dma_get_range() actually succeeding, or is it tripping up on some
> aspect of the DT (in which case there should be errors in the log)?
>
> Looking again at the fragment below, are you sure it's correct? It
> appears to me like it might actually be defining a 1-byte-long DMA
> range, which indeed I wouldn't really expect to work.

Indeed, though it took me a minute to see why.

>
> Robin.
>
> >>
> >> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> >> index 64a0f90f5b52..5418c31d4da7 100644
> >> --- a/arch/arm/boot/dts/dra7.dtsi
> >> +++ b/arch/arm/boot/dts/dra7.dtsi
> >> @@ -680,15 +680,22 @@
> >>           };
> >>
> >>           /* OCP2SCP3 */
> >> -        sata: sata@4a141100 {
> >> -            compatible = "snps,dwc-ahci";
> >> -            reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;

Based on this, the parent address size is 1 cell...

> >> -            interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> >> -            phys = <&sata_phy>;
> >> -            phy-names = "sata-phy";
> >> -            clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
> >> -            ti,hwmods = "sata";
> >> -            ports-implemented = <0x1>;
> >> +        sata_aux_bus {
> >> +            #address-cells = <2>;
> >> +            #size-cells = <2>;
> >> +            compatible = "simple-bus";
> >> +            ranges = <0x0 0x0 0x4a140000 0x0 0x1200>;
> >> +            dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x00000000>;

So this is:
child addr: 0x0 0x0
parent addr: 0x0
size: 0x0 0x1

The last cell is just ignored I guess if you aren't seeing any errors.
We check this in dtc for ranges, but not dma-ranges. So I'm fixing
that.

> >> +            sata: sata@4a141100 {
> >> +                compatible = "snps,dwc-ahci";
> >> +                reg = <0x0 0x0 0x0 0x1100>, <0x0 0x1100 0x0 0x7>;
> >> +                interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> >> +                phys = <&sata_phy>;
> >> +                phy-names = "sata-phy";
> >> +                clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
> >> +                ti,hwmods = "sata";
> >> +                ports-implemented = <0x1>;
> >> +            };
> >>           };
> >>
> >>           /* OCP2SCP1 */
> >>
> >

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: dma_mask limited to 32-bits with OF platform device
  2020-03-03 19:26                       ` Rob Herring
@ 2020-03-04  8:28                         ` Roger Quadros
  0 siblings, 0 replies; 18+ messages in thread
From: Roger Quadros @ 2020-03-04  8:28 UTC (permalink / raw)
  To: Rob Herring, Robin Murphy
  Cc: Christoph Hellwig, Péter Ujfalusi, Murali Karicheri, Nori,
	Sekhar, Anna, Suman, Stefan Wahren, Andreas Färber,
	Hans Verkuil, devicetree, linux-kernel, Nishanth Menon, hdegoede,
	Vignesh Raghavendra

Hi,

On 03/03/2020 21:26, Rob Herring wrote:
> On Tue, Mar 3, 2020 at 8:06 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 03/03/2020 8:27 am, Roger Quadros wrote:
>> [...]
>>>> With the patch (in the end). dev->bus_dma_limit is still set to 0 and
>>>> so is not being used.
>>>>
>>>> from of_dma_configure()
>>>>           ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>>>> ...
>>>>           /* ...but only set bus limit if we found valid dma-ranges
>>>> earlier */
>>>>           if (!ret)
>>>>                   dev->bus_dma_limit = end;
>>>>
>>>> There is no other place bus_dma_limit is set. Looks like every device
>>>> should inherit that
>>>> from it's parent right?
>>>
>>> Any ideas how to expect this to work?
>>
>> Is of_dma_get_range() actually succeeding, or is it tripping up on some
>> aspect of the DT (in which case there should be errors in the log)?
>>

of_dma_get_range() was failing but no errors in the log.

>> Looking again at the fragment below, are you sure it's correct? It
>> appears to me like it might actually be defining a 1-byte-long DMA
>> range, which indeed I wouldn't really expect to work.
> 
> Indeed, though it took me a minute to see why.
> 
>>
>> Robin.
>>
>>>>
>>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>>> index 64a0f90f5b52..5418c31d4da7 100644
>>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>>> @@ -680,15 +680,22 @@
>>>>            };
>>>>
>>>>            /* OCP2SCP3 */
>>>> -        sata: sata@4a141100 {
>>>> -            compatible = "snps,dwc-ahci";
>>>> -            reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
> 
> Based on this, the parent address size is 1 cell...
> 
>>>> -            interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>>>> -            phys = <&sata_phy>;
>>>> -            phy-names = "sata-phy";
>>>> -            clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
>>>> -            ti,hwmods = "sata";
>>>> -            ports-implemented = <0x1>;
>>>> +        sata_aux_bus {
>>>> +            #address-cells = <2>;
>>>> +            #size-cells = <2>;
>>>> +            compatible = "simple-bus";
>>>> +            ranges = <0x0 0x0 0x4a140000 0x0 0x1200>;
>>>> +            dma-ranges = <0x0 0x0 0x0 0x0 0x1 0x00000000>;
> 
> So this is:
> child addr: 0x0 0x0
> parent addr: 0x0
> size: 0x0 0x1

Good catch.
So I fixed it to

	dma-ranges = <0x0 0x0 0x0 0x1 0x00000000>;

And now it works :).
Thanks!

> 
> The last cell is just ignored I guess if you aren't seeing any errors.
> We check this in dtc for ranges, but not dma-ranges. So I'm fixing
> that.

Great!

> 
>>>> +            sata: sata@4a141100 {
>>>> +                compatible = "snps,dwc-ahci";
>>>> +                reg = <0x0 0x0 0x0 0x1100>, <0x0 0x1100 0x0 0x7>;
>>>> +                interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                phys = <&sata_phy>;
>>>> +                phy-names = "sata-phy";
>>>> +                clocks = <&l3init_clkctrl DRA7_L3INIT_SATA_CLKCTRL 8>;
>>>> +                ti,hwmods = "sata";
>>>> +                ports-implemented = <0x1>;
>>>> +            };
>>>>            };
>>>>
>>>>            /* OCP2SCP1 */
>>>>
>>>

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-03-04  8:28 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 10:49 dma_mask limited to 32-bits with OF platform device Roger Quadros
2020-02-12 11:37 ` Robin Murphy
2020-02-12 12:33   ` Roger Quadros
2020-02-12 14:04     ` Robin Murphy
2020-02-12 17:57       ` Christoph Hellwig
2020-02-17 13:21       ` Christoph Hellwig
2020-02-17 14:54         ` Peter Ujfalusi
2020-02-18  7:26           ` Peter Ujfalusi
2020-02-18  8:28         ` Roger Quadros
2020-02-18 17:22           ` Rob Herring
2020-02-19 14:29             ` Roger Quadros
2020-02-19 15:25               ` Robin Murphy
2020-02-19 15:40                 ` Roger Quadros
2020-02-26 11:33                 ` Roger Quadros
2020-03-03  8:27                   ` Roger Quadros
2020-03-03 14:06                     ` Robin Murphy
2020-03-03 19:26                       ` Rob Herring
2020-03-04  8:28                         ` Roger Quadros

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).