Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
Christian Zigotzky writes: > On 24 January 2020 at 12:42 pm, Michael Ellerman wrote: >> Ulf Hansson writes: >>> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky >>> wrote: Hi All, We still need the attached patch for our onboard SD card interface [1,2]. Could you please add this patch to the tree? >>> No, because according to previous discussion that isn't the correct >>> solution and more importantly it will break other archs (if I recall >>> correctly). >>> >>> Looks like someone from the ppc community needs to pick up the ball. >> That's a pretty small community these days :) :/ >> >> Christian can you test this please? I think I got the polarity of all >> the tests right, but it's Friday night so maybe I'm wrong :) >> >> cheers > Michael, > > Thanks a lot for the new patch! I compiled the RC7 of kernel 5.5 with > your patch again yesterday and the kernel works without any problems > with our onboard SD cards. [1] Thanks for testing. cheers
Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On 24 January 2020 at 12:42 pm, Michael Ellerman wrote:
Ulf Hansson writes:
On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky wrote:
Hi All,
We still need the attached patch for our onboard SD card interface
[1,2]. Could you please add this patch to the tree?
No, because according to previous discussion that isn't the correct
solution and more importantly it will break other archs (if I recall
correctly).
Looks like someone from the ppc community needs to pick up the ball.
That's a pretty small community these days :) :/
Christian can you test this please? I think I got the polarity of all
the tests right, but it's Friday night so maybe I'm wrong :)
cheers
Michael,
Thanks a lot for the new patch! I compiled the RC7 of kernel 5.5 with
your patch again yesterday and the kernel works without any problems
with our onboard SD cards. [1]
Cheers,
Christian
[1]
http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4384&p=49697#p49693
>From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
From: Michael Ellerman
Date: Fri, 24 Jan 2020 22:26:59 +1100
Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc
There's an OF helper called of_dma_is_coherent(), which checks if a
device has a "dma-coherent" property to see if the device is coherent
for DMA.
But on some platforms devices are coherent by default, and on some
platforms it's not possible to update existing device trees to add the
"dma-coherent" property.
So add a Kconfig symbol to allow arch code to tell
of_dma_is_coherent() that devices are coherent by default, regardless
of the presence of the property.
Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
when the system has a coherent cache.
Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
Cc: sta...@vger.kernel.org # v3.16+
Signed-off-by: Michael Ellerman
---
arch/powerpc/Kconfig | 1 +
drivers/of/Kconfig | 4
drivers/of/address.c | 6 +-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 62752c3bfabf..460678ab2375 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,6 +235,7 @@ config PPC
select NEED_DMA_MAP_STATE if PPC64 || NOT_COHERENT_CACHE
select NEED_SG_DMA_LENGTH
select OF
+ select OF_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE
select OF_EARLY_FLATTREE
select OLD_SIGACTIONif PPC32
select OLD_SIGSUSPEND
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 37c2ccbefecd..d91618641be6 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -103,4 +103,8 @@ config OF_OVERLAY
config OF_NUMA
bool
+config OF_DMA_DEFAULT_COHERENT
+ # arches should select this if DMA is coherent by default for OF devices
+ bool
+
endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 99c1b8058559..e8a39c3ec4d4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64
*dma_addr, u64 *paddr, u64 *siz
* @np: device node
*
* It returns true if "dma-coherent" property was found
- * for this device in DT.
+ * for this device in the DT, or if DMA is coherent by
+ * default for OF devices on the current platform.
*/
bool of_dma_is_coherent(struct device_node *np)
{
struct device_node *node = of_node_get(np);
+ if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
+ return true;
+
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
of_node_put(node);
Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
Ulf Hansson writes:
> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky
> wrote:
>>
>> Hi All,
>>
>> We still need the attached patch for our onboard SD card interface
>> [1,2]. Could you please add this patch to the tree?
>
> No, because according to previous discussion that isn't the correct
> solution and more importantly it will break other archs (if I recall
> correctly).
>
> Looks like someone from the ppc community needs to pick up the ball.
That's a pretty small community these days :) :/
Christian can you test this please? I think I got the polarity of all
the tests right, but it's Friday night so maybe I'm wrong :)
cheers
>From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
From: Michael Ellerman
Date: Fri, 24 Jan 2020 22:26:59 +1100
Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc
There's an OF helper called of_dma_is_coherent(), which checks if a
device has a "dma-coherent" property to see if the device is coherent
for DMA.
But on some platforms devices are coherent by default, and on some
platforms it's not possible to update existing device trees to add the
"dma-coherent" property.
So add a Kconfig symbol to allow arch code to tell
of_dma_is_coherent() that devices are coherent by default, regardless
of the presence of the property.
Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
when the system has a coherent cache.
Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
Cc: sta...@vger.kernel.org # v3.16+
Signed-off-by: Michael Ellerman
---
arch/powerpc/Kconfig | 1 +
drivers/of/Kconfig | 4
drivers/of/address.c | 6 +-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 62752c3bfabf..460678ab2375 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,6 +235,7 @@ config PPC
select NEED_DMA_MAP_STATE if PPC64 || NOT_COHERENT_CACHE
select NEED_SG_DMA_LENGTH
select OF
+ select OF_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE
select OF_EARLY_FLATTREE
select OLD_SIGACTIONif PPC32
select OLD_SIGSUSPEND
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 37c2ccbefecd..d91618641be6 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -103,4 +103,8 @@ config OF_OVERLAY
config OF_NUMA
bool
+config OF_DMA_DEFAULT_COHERENT
+ # arches should select this if DMA is coherent by default for OF devices
+ bool
+
endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 99c1b8058559..e8a39c3ec4d4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64
*dma_addr, u64 *paddr, u64 *siz
* @np:device node
*
* It returns true if "dma-coherent" property was found
- * for this device in DT.
+ * for this device in the DT, or if DMA is coherent by
+ * default for OF devices on the current platform.
*/
bool of_dma_is_coherent(struct device_node *np)
{
struct device_node *node = of_node_get(np);
+ if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
+ return true;
+
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
of_node_put(node);
--
2.21.1
Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Mon, 20 Jan 2020 at 10:17, Christian Zigotzky wrote: > > Am 16.01.20 um 16:46 schrieb Ulf Hansson: > > On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky > > wrote: > >> Hi All, > >> > >> We still need the attached patch for our onboard SD card interface > >> [1,2]. Could you please add this patch to the tree? > > No, because according to previous discussion that isn't the correct > > solution and more importantly it will break other archs (if I recall > > correctly). > > > > Looks like someone from the ppc community needs to pick up the ball. > I am not sure if the ppc community have to fix this issue because your > updates (mmc-v5.4-2) are responsible for this issue. If nobody wants to > fix this issue then we will lost the onboard SD card support in the > future. PLEASE check the 'mmc-v5.4-2' updates again. Applying your suggested fix breaks other archs/boards. It's really not a good situation, but I will not take a step back when it's quite easy to take a step forward instead. Someone just need to care and send a patch, it doesn't look that hard to me, but maybe I am wrong. Apologies if this isn't the answer you wanted, but that's all I can do for now, sorry. Kind regards Uffe
[FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
Am 16.01.20 um 16:46 schrieb Ulf Hansson: On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky wrote: Hi All, We still need the attached patch for our onboard SD card interface [1,2]. Could you please add this patch to the tree? No, because according to previous discussion that isn't the correct solution and more importantly it will break other archs (if I recall correctly). Looks like someone from the ppc community needs to pick up the ball. I am not sure if the ppc community have to fix this issue because your updates (mmc-v5.4-2) are responsible for this issue. If nobody wants to fix this issue then we will lost the onboard SD card support in the future. PLEASE check the 'mmc-v5.4-2' updates again. Thanks, Christian [1] https://www.spinics.net/lists/linux-mmc/msg56211.html I think this discussion even suggested some viable solutions, so it just be a matter of sending a patch :-) [2] http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012 Kind regards Uffe
Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky wrote: > > Hi All, > > We still need the attached patch for our onboard SD card interface > [1,2]. Could you please add this patch to the tree? No, because according to previous discussion that isn't the correct solution and more importantly it will break other archs (if I recall correctly). Looks like someone from the ppc community needs to pick up the ball. > > Thanks, > Christian > > [1] https://www.spinics.net/lists/linux-mmc/msg56211.html I think this discussion even suggested some viable solutions, so it just be a matter of sending a patch :-) > [2] > http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012 > Kind regards Uffe
[FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
Hi All,
We still need the attached patch for our onboard SD card interface
[1,2]. Could you please add this patch to the tree?
Thanks,
Christian
[1] https://www.spinics.net/lists/linux-mmc/msg56211.html
[2]
http://forum.hyperion-entertainment.com/viewtopic.php?f=58&t=4349&start=20#p49012
diff -rupN a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
--- a/arch/powerpc/kernel/setup-common.c 2019-12-03 18:05:52.436070217 +0100
+++ b/arch/powerpc/kernel/setup-common.c 2019-12-03 18:03:20.316629696 +0100
@@ -780,6 +780,22 @@ static int __init check_cache_coherency(
late_initcall(check_cache_coherency);
#endif /* CONFIG_CHECK_CACHE_COHERENCY */
+#ifndef CONFIG_NOT_COHERENT_CACHE
+/*
+ * For historical reasons powerpc kernels are built with hard wired knowledge of
+ * whether or not DMA accesses are cache coherent. Additionally device trees on
+ * powerpc do not typically support the dma-coherent property.
+ *
+ * So when we know that DMA is coherent, override arch_of_dma_is_coherent() to
+ * tell the drivers/of code that all devices are coherent regardless of whether
+ * they have a dma-coherent property.
+ */
+bool arch_of_dma_is_coherent(struct device_node *np)
+{
+ return true;
+}
+#endif
+
#ifdef CONFIG_DEBUG_FS
struct dentry *powerpc_debugfs_root;
EXPORT_SYMBOL(powerpc_debugfs_root);
diff -rupN a/drivers/of/address.c b/drivers/of/address.c
--- a/drivers/of/address.c 2019-12-03 18:05:57.332052212 +0100
+++ b/drivers/of/address.c 2019-12-03 18:03:20.320629681 +0100
@@ -990,6 +990,14 @@ out:
return ret;
}
+/*
+ * arch_of_dma_is_coherent - Arch hook to determine if device is coherent for DMA
+ */
+bool __weak arch_of_dma_is_coherent(struct device_node *np)
+{
+ return false;
+}
+
/**
* of_dma_is_coherent - Check if device is coherent
* @np: device node
@@ -999,8 +1007,12 @@ out:
*/
bool of_dma_is_coherent(struct device_node *np)
{
- struct device_node *node = of_node_get(np);
+ struct device_node *node;
+
+ if (arch_of_dma_is_coherent(np))
+ return true;
+ np = of_node_get(np);
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
of_node_put(node);
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
FYI: The onboard SD card works also with the RC6 of kernel 5.4 with the
patch below.
-- Christian
On 23 October 2019 at 4:20pm, Christian Zigotzky wrote:
Hello,
The patch below works. I compiled the RC4 of kernel 5.4 with this patch today
and the onboard SD card works without any problems.
Thanks!
Christian
On 23. Oct 2019, at 07:42, Michael Ellerman wrote:
Russell King - ARM Linux admin writes:
On Tue, Oct 15, 2019 at 03:12:49PM +0200, Christian Zigotzky wrote:
Hello Russell,
You asked me about "dma-coherent" in the Cyrus device tree. Unfortunately I
don't find the property "dma-coherent" in the dtb source files.
Output of "fdtdump cyrus_p5020_eth_poweroff.dtb | grep dma":
dma0 = "/soc@ffe00/dma@100300";
dma1 = "/soc@ffe00/dma@101300";
dma@100300 {
compatible = "fsl,eloplus-dma";
dma-channel@0 {
compatible = "fsl,eloplus-dma-channel";
dma-channel@80 {
compatible = "fsl,eloplus-dma-channel";
dma-channel@100 {
compatible = "fsl,eloplus-dma-channel";
dma-channel@180 {
compatible = "fsl,eloplus-dma-channel";
dma@101300 {
compatible = "fsl,eloplus-dma";
dma-channel@0 {
compatible = "fsl,eloplus-dma-channel";
dma-channel@80 {
compatible = "fsl,eloplus-dma-channel";
dma-channel@100 {
compatible = "fsl,eloplus-dma-channel";
dma-channel@180 {
compatible = "fsl,eloplus-dma-channel";
Hmm, so it looks like PowerPC doesn't mark devices that are dma
coherent with a property that describes them as such.
I think this opens a wider question - what should of_dma_is_coherent()
return for PowerPC? It seems right now that it returns false for
devices that are DMA coherent, which seems to me to be a recipe for
future mistakes.
Right, it seems of_dma_is_coherent() has baked in the assumption that
devices are non-coherent unless explicitly marked as coherent.
Which is wrong on all or at least most existing powerpc systems
according to Ben.
Any ideas from the PPC maintainers?
Fixing it at the source seems like the best option to prevent future
breakage.
So I guess that would mean making of_dma_is_coherent() return true/false
based on CONFIG_NOT_COHERENT_CACHE on powerpc.
We could do it like below, which would still allow the dma-coherent
property to work if it ever makes sense on a future powerpc platform.
I don't really know any of this embedded stuff well, so happy to take
other suggestions on how to handle this mess.
cheers
diff --git a/arch/powerpc/kernel/setup-common.c
b/arch/powerpc/kernel/setup-common.c
index 25aaa3903000..b96c9010acb6 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -760,6 +760,22 @@ static int __init check_cache_coherency(void)
late_initcall(check_cache_coherency);
#endif /* CONFIG_CHECK_CACHE_COHERENCY */
+#ifndef CONFIG_NOT_COHERENT_CACHE
+/*
+ * For historical reasons powerpc kernels are built with hard wired knowledge
of
+ * whether or not DMA accesses are cache coherent. Additionally device trees on
+ * powerpc do not typically support the dma-coherent property.
+ *
+ * So when we know that DMA is coherent, override arch_of_dma_is_coherent() to
+ * tell the drivers/of code that all devices are coherent regardless of whether
+ * they have a dma-coherent property.
+ */
+bool arch_of_dma_is_coherent(struct device_node *np)
+{
+return true;
+}
+#endif
+
#ifdef CONFIG_DEBUG_FS
struct dentry *powerpc_debugfs_root;
EXPORT_SYMBOL(powerpc_debugfs_root);
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 978427a9d5e6..3a4b2949a322 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -993,6 +993,14 @@ int of_dma_get_range(struct device_node *np, u64
*dma_addr, u64 *paddr, u64 *siz
}
EXPORT_SYMBOL_GPL(of_dma_get_range);
+/*
+ * arch_of_dma_is_coherent - Arch hook to determine if device is coherent for
DMA
+ */
+bool __weak arch_of_dma_is_coherent(struct device_node *np)
+{
+return false;
+}
+
/**
* of_dma_is_coherent - Check if device is coherent
* @np:device node
@@ -1002,8 +1010,12 @@ EXPORT_SYMBOL_GPL(of_dma_get_range);
*/
bool of_dma_is_coherent(struct device_node *np)
{
-struct device_node *node = of_node_get(np);
+struct device_node *node;
+
+if (arch_of_dma_is_coherent(np))
+return true;
+np = of_node_get(np);
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
of_node_put(node);
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Fri, 2019-10-25 at 23:39 -0700, Christoph Hellwig wrote: > On Fri, Oct 25, 2019 at 05:28:45PM -0500, Rob Herring wrote: > > This doesn't work?: > > > > if (IS_ENABLED(CONFIG_PPC) || of_dma_is_coherent(dev- > > >of_node)) > > value |= ESDHC_DMA_SNOOP; > > else > > value &= ~ESDHC_DMA_SNOOP; > > > > While I said use the compatibles, using the kconfig symbol is > > easier > > than sorting out which compatibles are PPC SoCs. Though if that's > > already done elsewhere in the driver, you could set a flag and use > > that here. I'd be surprised if this was the only difference between > > ARM and PPC SoCs for this block. > > I think the right thing is a Kconfig variable that the architectures > selects which says if OF is by default coherent or incoherent, and > then use that in of_dma_is_coherent. That too. We could also define properties for both ways so we can override the default either way. Cheers, Ben.
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Fri, 2019-10-25 at 17:28 -0500, Rob Herring wrote: > This doesn't work?: > > if (IS_ENABLED(CONFIG_PPC) || of_dma_is_coherent(dev- > >of_node)) > value |= ESDHC_DMA_SNOOP; > else > value &= ~ESDHC_DMA_SNOOP; CONFIG_PPC is restrictive. What about sparc64 ? There could be others .. .we can't suddenly requests people to add new properties for what was implied behaviours before hand, esp since it's not in the base 1275 spec, no ? I would suggest of_dma_is_coherent is *true* by default unless overriden to do something else. Cheers, Ben.
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Fri, Oct 25, 2019 at 05:28:45PM -0500, Rob Herring wrote: > This doesn't work?: > > if (IS_ENABLED(CONFIG_PPC) || of_dma_is_coherent(dev->of_node)) > value |= ESDHC_DMA_SNOOP; > else > value &= ~ESDHC_DMA_SNOOP; > > While I said use the compatibles, using the kconfig symbol is easier > than sorting out which compatibles are PPC SoCs. Though if that's > already done elsewhere in the driver, you could set a flag and use > that here. I'd be surprised if this was the only difference between > ARM and PPC SoCs for this block. I think the right thing is a Kconfig variable that the architectures selects which says if OF is by default coherent or incoherent, and then use that in of_dma_is_coherent.
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, Oct 23, 2019 at 9:32 AM Russell King - ARM Linux admin wrote: > > On Wed, Oct 23, 2019 at 08:52:33AM -0500, Rob Herring wrote: > > > I think this should have been done the other way around and default to > > > coherent since most traditional OF platforms are coherent, and you > > > can't just require those DTs to change. > > > > You can blame me. This was really only intended for cases where > > coherency is configurable on a per peripheral basis and can't be > > determined in other ways. > > > > The simple solution here is simply to use the compatible string for > > the device to determine coherent or not. > > It really isn't that simple. This doesn't work?: if (IS_ENABLED(CONFIG_PPC) || of_dma_is_coherent(dev->of_node)) value |= ESDHC_DMA_SNOOP; else value &= ~ESDHC_DMA_SNOOP; While I said use the compatibles, using the kconfig symbol is easier than sorting out which compatibles are PPC SoCs. Though if that's already done elsewhere in the driver, you could set a flag and use that here. I'd be surprised if this was the only difference between ARM and PPC SoCs for this block. > There are two aspects to coherency, both of which must match: > > 1) The configuration of the device > 2) The configuration of the kernel's DMA API > > (1) is controlled by the driver, which can make the decision any way > it pleases. > > (2) on ARM64 is controlled depending on whether or not "dma-coherent" > is specified in the device tree, since ARM64 can have a mixture of > DMA coherent and non-coherent devices. > > A mismatch between (1) and (2) results in data corruption, potentially > eating your filesystem. So, it's very important that the two match. > > These didn't match for the LX2160A, but, due to the way CMA was working, > we sort of got away with it, but it was very dangerous as far as data > safety went. > > Then, a change to CMA happened which moved where it was located, which > caused a regression. Reverting the CMA changes didn't seem to be an > option, so another solution had to be found. > > I started a discussion on how best to solve this: > > https://archive.armlinux.org.uk/lurker/thread/20190919.041320.1e53541f.en.html > > and the solution that the discussion came out with was the one that has > been merged - which we now know caused a regression on PPC. > > Using compatible strings doesn't solve the issue: there is no way to > tell the DMA API from the driver that the device is coherent. The > only way to do that is via the "dma-coherent" property in DT on ARM64. > > To say that this is a mess is an under-statement, but we seem to have > ended up here because of a series of piece-meal changes that don't seem > to have been thought through enough. > > So, what's the right way to solve this, and ensure that the DMA API and > device match as far as their coherency expectations go? Revert all the > changes for sdhci-of-esdhc and CMA back to 5.0 or 5.1 state? The other option is similar to earlier in the thread and just add to of_dma_is_coherent(): /* Powerpc is normally cache coherent DMA */ if (IS_ENABLED(CONFIG_PPC) && !IS_ENABLED(CONFIG_NOT_COHERENT_CACHE)) return true; We could do the all the weak arch hooks, but that seems like overkill to me at this point. Rob
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, Oct 23, 2019 at 08:52:33AM -0500, Rob Herring wrote: > > I think this should have been done the other way around and default to > > coherent since most traditional OF platforms are coherent, and you > > can't just require those DTs to change. > > You can blame me. This was really only intended for cases where > coherency is configurable on a per peripheral basis and can't be > determined in other ways. > > The simple solution here is simply to use the compatible string for > the device to determine coherent or not. It really isn't that simple. There are two aspects to coherency, both of which must match: 1) The configuration of the device 2) The configuration of the kernel's DMA API (1) is controlled by the driver, which can make the decision any way it pleases. (2) on ARM64 is controlled depending on whether or not "dma-coherent" is specified in the device tree, since ARM64 can have a mixture of DMA coherent and non-coherent devices. A mismatch between (1) and (2) results in data corruption, potentially eating your filesystem. So, it's very important that the two match. These didn't match for the LX2160A, but, due to the way CMA was working, we sort of got away with it, but it was very dangerous as far as data safety went. Then, a change to CMA happened which moved where it was located, which caused a regression. Reverting the CMA changes didn't seem to be an option, so another solution had to be found. I started a discussion on how best to solve this: https://archive.armlinux.org.uk/lurker/thread/20190919.041320.1e53541f.en.html and the solution that the discussion came out with was the one that has been merged - which we now know caused a regression on PPC. Using compatible strings doesn't solve the issue: there is no way to tell the DMA API from the driver that the device is coherent. The only way to do that is via the "dma-coherent" property in DT on ARM64. To say that this is a mess is an under-statement, but we seem to have ended up here because of a series of piece-meal changes that don't seem to have been thought through enough. So, what's the right way to solve this, and ensure that the DMA API and device match as far as their coherency expectations go? Revert all the changes for sdhci-of-esdhc and CMA back to 5.0 or 5.1 state? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, Oct 23, 2019 at 08:52:33AM -0500, Rob Herring wrote:
> On Wed, Oct 23, 2019 at 1:41 AM Benjamin Herrenschmidt
> wrote:
> >
> > On Wed, 2019-10-23 at 16:42 +1100, Michael Ellerman wrote:
> > >
> > > Right, it seems of_dma_is_coherent() has baked in the assumption that
> > > devices are non-coherent unless explicitly marked as coherent.
> > >
> > > Which is wrong on all or at least most existing powerpc systems
> > > according to Ben.
> >
> > This is probably broken on sparc(64) as well and whatever else uses
> > DT and is an intrinsicly coherent architecture (did we ever have
> > DT enabled x86s ? Wasn't OLPC such a beast ?).
>
> Only if those platforms use one of the 5 drivers that call this function:
>
> drivers/ata/ahci_qoriq.c: qoriq_priv->is_dmacoherent =
> of_dma_is_coherent(np);
> drivers/crypto/ccree/cc_driver.c: new_drvdata->coherent =
> of_dma_is_coherent(np);
> drivers/iommu/arm-smmu-v3.c:if (of_dma_is_coherent(dev->of_node))
> drivers/iommu/arm-smmu.c: if (of_dma_is_coherent(dev->of_node))
> drivers/mmc/host/sdhci-of-esdhc.c: if (of_dma_is_coherent(dev->of_node))
>
> Curious that a PPC specific driver (ahci_qoriq) calls it...
Maybe because it is not PPC specific:
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi:
compatible = "fsl,lx2160a-ahci";
drivers/ata/ahci_qoriq.c: { .compatible = "fsl,lx2160a-ahci", .data =
(void *)AHCI_LX2160A},
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
Hello,
The patch below works. I compiled the RC4 of kernel 5.4 with this patch today
and the onboard SD card works without any problems.
Thanks!
Christian
> On 23. Oct 2019, at 07:42, Michael Ellerman wrote:
>
> Russell King - ARM Linux admin writes:
>>> On Tue, Oct 15, 2019 at 03:12:49PM +0200, Christian Zigotzky wrote:
>>> Hello Russell,
>>>
>>> You asked me about "dma-coherent" in the Cyrus device tree. Unfortunately I
>>> don't find the property "dma-coherent" in the dtb source files.
>>>
>>> Output of "fdtdump cyrus_p5020_eth_poweroff.dtb | grep dma":
>>>
>>> dma0 = "/soc@ffe00/dma@100300";
>>> dma1 = "/soc@ffe00/dma@101300";
>>> dma@100300 {
>>> compatible = "fsl,eloplus-dma";
>>> dma-channel@0 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma-channel@80 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma-channel@100 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma-channel@180 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma@101300 {
>>> compatible = "fsl,eloplus-dma";
>>> dma-channel@0 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma-channel@80 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma-channel@100 {
>>> compatible = "fsl,eloplus-dma-channel";
>>> dma-channel@180 {
>>> compatible = "fsl,eloplus-dma-channel";
>>
>> Hmm, so it looks like PowerPC doesn't mark devices that are dma
>> coherent with a property that describes them as such.
>>
>> I think this opens a wider question - what should of_dma_is_coherent()
>> return for PowerPC? It seems right now that it returns false for
>> devices that are DMA coherent, which seems to me to be a recipe for
>> future mistakes.
>
> Right, it seems of_dma_is_coherent() has baked in the assumption that
> devices are non-coherent unless explicitly marked as coherent.
>
> Which is wrong on all or at least most existing powerpc systems
> according to Ben.
>
>> Any ideas from the PPC maintainers?
>
> Fixing it at the source seems like the best option to prevent future
> breakage.
>
> So I guess that would mean making of_dma_is_coherent() return true/false
> based on CONFIG_NOT_COHERENT_CACHE on powerpc.
>
> We could do it like below, which would still allow the dma-coherent
> property to work if it ever makes sense on a future powerpc platform.
>
> I don't really know any of this embedded stuff well, so happy to take
> other suggestions on how to handle this mess.
>
> cheers
>
>
> diff --git a/arch/powerpc/kernel/setup-common.c
> b/arch/powerpc/kernel/setup-common.c
> index 25aaa3903000..b96c9010acb6 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -760,6 +760,22 @@ static int __init check_cache_coherency(void)
> late_initcall(check_cache_coherency);
> #endif /* CONFIG_CHECK_CACHE_COHERENCY */
>
> +#ifndef CONFIG_NOT_COHERENT_CACHE
> +/*
> + * For historical reasons powerpc kernels are built with hard wired
> knowledge of
> + * whether or not DMA accesses are cache coherent. Additionally device trees
> on
> + * powerpc do not typically support the dma-coherent property.
> + *
> + * So when we know that DMA is coherent, override arch_of_dma_is_coherent()
> to
> + * tell the drivers/of code that all devices are coherent regardless of
> whether
> + * they have a dma-coherent property.
> + */
> +bool arch_of_dma_is_coherent(struct device_node *np)
> +{
> +return true;
> +}
> +#endif
> +
> #ifdef CONFIG_DEBUG_FS
> struct dentry *powerpc_debugfs_root;
> EXPORT_SYMBOL(powerpc_debugfs_root);
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 978427a9d5e6..3a4b2949a322 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -993,6 +993,14 @@ int of_dma_get_range(struct device_node *np, u64
> *dma_addr, u64 *paddr, u64 *siz
> }
> EXPORT_SYMBOL_GPL(of_dma_get_range);
>
> +/*
> + * arch_of_dma_is_coherent - Arch hook to determine if device is coherent
> for DMA
> + */
> +bool __weak arch_of_dma_is_coherent(struct device_node *np)
> +{
> +return false;
> +}
> +
> /**
> * of_dma_is_coherent - Check if device is coherent
> * @np:device node
> @@ -1002,8 +1010,12 @@ EXPORT_SYMBOL_GPL(of_dma_get_range);
> */
> bool of_dma_is_coherent(struct device_node *np)
> {
> -struct device_node *node = of_node_get(np);
> +struct device_node *node;
> +
> +if (arch_of_dma_is_coherent(np))
> +return true;
>
> +np = of_node_get(np);
>while (node) {
>if (of_property_read_bool(node, "dma-coherent")) {
>of_node_put(node);
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, Oct 23, 2019 at 1:41 AM Benjamin Herrenschmidt wrote: > > On Wed, 2019-10-23 at 16:42 +1100, Michael Ellerman wrote: > > > > Right, it seems of_dma_is_coherent() has baked in the assumption that > > devices are non-coherent unless explicitly marked as coherent. > > > > Which is wrong on all or at least most existing powerpc systems > > according to Ben. > > This is probably broken on sparc(64) as well and whatever else uses > DT and is an intrinsicly coherent architecture (did we ever have > DT enabled x86s ? Wasn't OLPC such a beast ?). Only if those platforms use one of the 5 drivers that call this function: drivers/ata/ahci_qoriq.c: qoriq_priv->is_dmacoherent = of_dma_is_coherent(np); drivers/crypto/ccree/cc_driver.c: new_drvdata->coherent = of_dma_is_coherent(np); drivers/iommu/arm-smmu-v3.c:if (of_dma_is_coherent(dev->of_node)) drivers/iommu/arm-smmu.c: if (of_dma_is_coherent(dev->of_node)) drivers/mmc/host/sdhci-of-esdhc.c: if (of_dma_is_coherent(dev->of_node)) Curious that a PPC specific driver (ahci_qoriq) calls it... Note that the value is also passed to arch_setup_dma_ops(), but only arc, arm, arm64, and mips implement arch_setup_dma_ops. > I think this should have been done the other way around and default to > coherent since most traditional OF platforms are coherent, and you > can't just require those DTs to change. You can blame me. This was really only intended for cases where coherency is configurable on a per peripheral basis and can't be determined in other ways. The simple solution here is simply to use the compatible string for the device to determine coherent or not. Rob
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
On Wed, 2019-10-23 at 16:42 +1100, Michael Ellerman wrote: > > Right, it seems of_dma_is_coherent() has baked in the assumption that > devices are non-coherent unless explicitly marked as coherent. > > Which is wrong on all or at least most existing powerpc systems > according to Ben. This is probably broken on sparc(64) as well and whatever else uses DT and is an intrinsicly coherent architecture (did we ever have DT enabled x86s ? Wasn't OLPC such a beast ?). I think this should have been done the other way around and default to coherent since most traditional OF platforms are coherent, and you can't just require those DTs to change. > > Any ideas from the PPC maintainers? > > Fixing it at the source seems like the best option to prevent future > breakage. > > So I guess that would mean making of_dma_is_coherent() return true/false > based on CONFIG_NOT_COHERENT_CACHE on powerpc. > > We could do it like below, which would still allow the dma-coherent > property to work if it ever makes sense on a future powerpc platform. > > I don't really know any of this embedded stuff well, so happy to take > other suggestions on how to handle this mess.
Re: Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
Russell King - ARM Linux admin writes:
> On Tue, Oct 15, 2019 at 03:12:49PM +0200, Christian Zigotzky wrote:
>> Hello Russell,
>>
>> You asked me about "dma-coherent" in the Cyrus device tree. Unfortunately I
>> don't find the property "dma-coherent" in the dtb source files.
>>
>> Output of "fdtdump cyrus_p5020_eth_poweroff.dtb | grep dma":
>>
>> dma0 = "/soc@ffe00/dma@100300";
>> dma1 = "/soc@ffe00/dma@101300";
>> dma@100300 {
>> compatible = "fsl,eloplus-dma";
>> dma-channel@0 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma-channel@80 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma-channel@100 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma-channel@180 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma@101300 {
>> compatible = "fsl,eloplus-dma";
>> dma-channel@0 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma-channel@80 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma-channel@100 {
>> compatible = "fsl,eloplus-dma-channel";
>> dma-channel@180 {
>> compatible = "fsl,eloplus-dma-channel";
>
> Hmm, so it looks like PowerPC doesn't mark devices that are dma
> coherent with a property that describes them as such.
>
> I think this opens a wider question - what should of_dma_is_coherent()
> return for PowerPC? It seems right now that it returns false for
> devices that are DMA coherent, which seems to me to be a recipe for
> future mistakes.
Right, it seems of_dma_is_coherent() has baked in the assumption that
devices are non-coherent unless explicitly marked as coherent.
Which is wrong on all or at least most existing powerpc systems
according to Ben.
> Any ideas from the PPC maintainers?
Fixing it at the source seems like the best option to prevent future
breakage.
So I guess that would mean making of_dma_is_coherent() return true/false
based on CONFIG_NOT_COHERENT_CACHE on powerpc.
We could do it like below, which would still allow the dma-coherent
property to work if it ever makes sense on a future powerpc platform.
I don't really know any of this embedded stuff well, so happy to take
other suggestions on how to handle this mess.
cheers
diff --git a/arch/powerpc/kernel/setup-common.c
b/arch/powerpc/kernel/setup-common.c
index 25aaa3903000..b96c9010acb6 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -760,6 +760,22 @@ static int __init check_cache_coherency(void)
late_initcall(check_cache_coherency);
#endif /* CONFIG_CHECK_CACHE_COHERENCY */
+#ifndef CONFIG_NOT_COHERENT_CACHE
+/*
+ * For historical reasons powerpc kernels are built with hard wired knowledge
of
+ * whether or not DMA accesses are cache coherent. Additionally device trees on
+ * powerpc do not typically support the dma-coherent property.
+ *
+ * So when we know that DMA is coherent, override arch_of_dma_is_coherent() to
+ * tell the drivers/of code that all devices are coherent regardless of whether
+ * they have a dma-coherent property.
+ */
+bool arch_of_dma_is_coherent(struct device_node *np)
+{
+ return true;
+}
+#endif
+
#ifdef CONFIG_DEBUG_FS
struct dentry *powerpc_debugfs_root;
EXPORT_SYMBOL(powerpc_debugfs_root);
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 978427a9d5e6..3a4b2949a322 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -993,6 +993,14 @@ int of_dma_get_range(struct device_node *np, u64
*dma_addr, u64 *paddr, u64 *siz
}
EXPORT_SYMBOL_GPL(of_dma_get_range);
+/*
+ * arch_of_dma_is_coherent - Arch hook to determine if device is coherent for
DMA
+ */
+bool __weak arch_of_dma_is_coherent(struct device_node *np)
+{
+ return false;
+}
+
/**
* of_dma_is_coherent - Check if device is coherent
* @np:device node
@@ -1002,8 +1010,12 @@ EXPORT_SYMBOL_GPL(of_dma_get_range);
*/
bool of_dma_is_coherent(struct device_node *np)
{
- struct device_node *node = of_node_get(np);
+ struct device_node *node;
+
+ if (arch_of_dma_is_coherent(np))
+ return true;
+ np = of_node_get(np);
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
of_node_put(node);
