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
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.
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
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.
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..
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
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
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
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
On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <[email protected]> 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
Rob,
On 18/02/2020 19:22, Rob Herring wrote:
> On Tue, Feb 18, 2020 at 2:28 AM Roger Quadros <[email protected]> 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
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 <[email protected]> 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/[email protected]/
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 <[email protected]> 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/[email protected]/
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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 <[email protected]> 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/[email protected]/
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
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 <[email protected]> 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/[email protected]/
>
> 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
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 */
>>
>
On Tue, Mar 3, 2020 at 8:06 AM Robin Murphy <[email protected]> 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 */
> >>
> >
Hi,
On 03/03/2020 21:26, Rob Herring wrote:
> On Tue, Mar 3, 2020 at 8:06 AM Robin Murphy <[email protected]> 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