* 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).