ARM: OMAP2+: l2c: squelch warning dump on power control setting

ROM code on AM437x does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Reported-by Nishanth Menon <[email protected]>

Signed-off-by Sekhar Nori <[email protected]>

Quoted 11 lines · Sekhar Nori

ROM code on AM437x does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Quoted 2 lines

Reported-by Nishanth Menon <[email protected]>

Signed-off-by Sekhar Nori <[email protected]>

Tested with today's linux-next (5f295cdf5c5dbbb0c40f10f2ddae02ff46bbf773) with am437x-sk, I can see that the WARNING goes away.

Tested-by Felipe Balbi <[email protected]>

Quoted 15 lines · Felipe Balbi

Quoted 13 lines · Sekhar Nori

ROM code on AM437x does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Reported-by Nishanth Menon <[email protected]>

Signed-off-by Sekhar Nori <[email protected]>

Quoted 5 lines

Tested with today's linux-next (5f295cdf5c5dbbb0c40f10f2ddae02ff46bbf773) with am437x-sk, I can see that the WARNING goes away.

Tested-by Felipe Balbi <[email protected]>

ping here, I can't see this patch onl linus/master or next/master yet.

Quoted 20 lines · Felipe Balbi

Quoted 19 lines · Felipe Balbi

Quoted 7 lines · Sekhar Nori

ROM code on AM437x does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Quoted 6 lines

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Reported-by Nishanth Menon <[email protected]>

Signed-off-by Sekhar Nori <[email protected]>

Tested with today's linux-next (5f295cdf5c5dbbb0c40f10f2ddae02ff46bbf773) with am437x-sk, I can see that the WARNING goes away.

Quoted 3 lines

Quoted 1 line

Tested-by Felipe Balbi <[email protected]>

ping here, I can't see this patch onl linus/master or next/master yet.

Sorry I've been waiting for my pull request against -rc1 to get merged first, no idea why that is still pending.

That's now merged into v3.16-rc4, so applying this into omap-for-v3.16/fixes.

From the comments in the patch, it seems that firmware doesn't provide

Quoted 2 lines · Tony Lindgren

That's now merged into v3.16-rc4, so applying this into omap-for-v3.16/fixes.

I intentionally left the warning in the hope that someone would update it to write to the register.

From the comments in the patch, it seems that firmware doesn't provide a method to do that, which is a tad annoying.

Quoted 7 lines · Russell King - ARM Linux

Quoted 2 lines · Tony Lindgren

That's now merged into v3.16-rc4, so applying this into omap-for-v3.16/fixes.

I intentionally left the warning in the hope that someone would update it to write to the register.

Quoted 2 lines

From the comments in the patch, it seems that firmware doesn't provide a method to do that, which is a tad annoying.

So it seems.. Santosh, might be worth checking if this is set up the same way for all omaps? Or do some have extra commands for OMAP4_MON_L2X0_* for ROM code calls?

OMAP4430 had L2 cache controller version r2p0 (per the public TRM) which does not have this register. So unless there is a ROM API that was introduced after OMAP4430, this would not be there even for other OMAP4s. Public TRM of OMAP4470 does not indicate an API for this.

Before creating the patch, I checked with ROM team handling AM437x and they denied an API to write to this register was present in AM437x ROM.

Quoted 7 lines · Sekhar Nori

OMAP4430 had L2 cache controller version r2p0 (per the public TRM) which does not have this register. So unless there is a ROM API that was introduced after OMAP4430, this would not be there even for other OMAP4s. Public TRM of OMAP4470 does not indicate an API for this.

Before creating the patch, I checked with ROM team handling AM437x and they denied an API to write to this register was present in AM437x ROM.

Okay, so why are we trying to write to this register then...

Ah, we have a bug in cache-l2x0.c:

#define L2X0_CACHE_ID_PART_MASK         (0xf << 6)
#define L2X0_CACHE_ID_RTL_MASK          0x3f
#define L310_CACHE_ID_RTL_R3P0          0x05

        unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;

        if (rev >= L310_CACHE_ID_RTL_R2P0) {
...
        if (rev >= L310_CACHE_ID_RTL_R3P0) {
                l2c_write_sec(L310_DYNAMIC_CLK_GATING_EN | L310_STNDBY_MODE_EN,
                              base, L310_POWER_CTRL);

So, because we're masking the wrong bits, we end up with these tests always succeeding.

So that's a NACK for the original patch, it's the wrong fix. The right fix is to avoid writing this register by fixing the RTL masking.

Quoted 25 lines · Russell King - ARM Linux

Quoted 7 lines · Sekhar Nori

OMAP4430 had L2 cache controller version r2p0 (per the public TRM) which does not have this register. So unless there is a ROM API that was introduced after OMAP4430, this would not be there even for other OMAP4s. Public TRM of OMAP4470 does not indicate an API for this.

Before creating the patch, I checked with ROM team handling AM437x and they denied an API to write to this register was present in AM437x ROM.

Okay, so why are we trying to write to this register then...

Ah, we have a bug in cache-l2x0.c:

#define L2X0_CACHE_ID_PART_MASK         (0xf << 6)
#define L2X0_CACHE_ID_RTL_MASK          0x3f
#define L310_CACHE_ID_RTL_R3P0          0x05

        unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;

        if (rev >= L310_CACHE_ID_RTL_R2P0) {
...
        if (rev >= L310_CACHE_ID_RTL_R3P0) {
                l2c_write_sec(L310_DYNAMIC_CLK_GATING_EN | L310_STNDBY_MODE_EN,
                              base, L310_POWER_CTRL);

Quoted 5 lines

So, because we're masking the wrong bits, we end up with these tests always succeeding.

So that's a NACK for the original patch, it's the wrong fix. The right fix is to avoid writing this register by fixing the RTL masking.

Okie dokie, dropping the omap specific fix.

Quoted 26 lines · Tony Lindgren

Quoted 25 lines · Russell King - ARM Linux

Quoted 7 lines · Sekhar Nori

OMAP4430 had L2 cache controller version r2p0 (per the public TRM) which does not have this register. So unless there is a ROM API that was introduced after OMAP4430, this would not be there even for other OMAP4s. Public TRM of OMAP4470 does not indicate an API for this.

Before creating the patch, I checked with ROM team handling AM437x and they denied an API to write to this register was present in AM437x ROM.

Okay, so why are we trying to write to this register then...

Ah, we have a bug in cache-l2x0.c:

#define L2X0_CACHE_ID_PART_MASK         (0xf << 6)
#define L2X0_CACHE_ID_RTL_MASK          0x3f
#define L310_CACHE_ID_RTL_R3P0          0x05

        unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;

        if (rev >= L310_CACHE_ID_RTL_R2P0) {
...
        if (rev >= L310_CACHE_ID_RTL_R3P0) {
                l2c_write_sec(L310_DYNAMIC_CLK_GATING_EN | L310_STNDBY_MODE_EN,
                              base, L310_POWER_CTRL);

Quoted 7 lines

Quoted 5 lines

So, because we're masking the wrong bits, we end up with these tests always succeeding.

So that's a NACK for the original patch, it's the wrong fix. The right fix is to avoid writing this register by fixing the RTL masking.

Okie dokie, dropping the omap specific fix.

Here's the revision mask fix - with the existing code, the revision checks are all useless since they would all pass irrespective of the actual revision. (Had the L2C series been better tested rather than being largely ignored, this may have been noticed before it was merged...) Anyway, what isn't clear from Sekhar's message is which revision L2C310 is in the AM437x.

From: Russell King <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] ARM: l2c: fix revision checking

The revision checking in l2c310_enable() was not correct; we were masking the part number rather than the revision number. Fix this to use the correct macro.

Fixes 4374d64933b1 ("ARM: l2c: add automatic enable of early BRESP")

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mm/cache-l2x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mm/cache-l2x0.c +0 -0

index 948f12cf6180..0b5068256baf 100644

Hi,

Quoted 61 lines · Russell King - ARM Linux

Quoted 26 lines · Tony Lindgren

Quoted 25 lines · Russell King - ARM Linux

Quoted 7 lines · Sekhar Nori

OMAP4430 had L2 cache controller version r2p0 (per the public TRM) which does not have this register. So unless there is a ROM API that was introduced after OMAP4430, this would not be there even for other OMAP4s. Public TRM of OMAP4470 does not indicate an API for this.

Before creating the patch, I checked with ROM team handling AM437x and they denied an API to write to this register was present in AM437x ROM.

Okay, so why are we trying to write to this register then...

Ah, we have a bug in cache-l2x0.c:

#define L2X0_CACHE_ID_PART_MASK         (0xf << 6)
#define L2X0_CACHE_ID_RTL_MASK          0x3f
#define L310_CACHE_ID_RTL_R3P0          0x05

        unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;

        if (rev >= L310_CACHE_ID_RTL_R2P0) {
...
        if (rev >= L310_CACHE_ID_RTL_R3P0) {
                l2c_write_sec(L310_DYNAMIC_CLK_GATING_EN | L310_STNDBY_MODE_EN,
                              base, L310_POWER_CTRL);

Quoted 7 lines

Quoted 5 lines

So, because we're masking the wrong bits, we end up with these tests always succeeding.

So that's a NACK for the original patch, it's the wrong fix. The right fix is to avoid writing this register by fixing the RTL masking.

Okie dokie, dropping the omap specific fix.

Here's the revision mask fix - with the existing code, the revision checks are all useless since they would all pass irrespective of the actual revision. (Had the L2C series been better tested rather than being largely ignored, this may have been noticed before it was merged...) Anyway, what isn't clear from Sekhar's message is which revision L2C310 is in the AM437x.

From: Russell King <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] ARM: l2c: fix revision checking

The revision checking in l2c310_enable() was not correct; we were masking the part number rather than the revision number. Fix this to use the correct macro.

Fixes 4374d64933b1 ("ARM: l2c: add automatic enable of early BRESP")

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mm/cache-l2x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mm/cache-l2x0.c +0 -0

index 948f12cf6180..0b5068256baf 100644
@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +1 -1

 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Quoted 5 lines · Felipe Balbi

Quoted 4 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +0 -0

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 

Quoted 12 lines

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +1 -1

 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

Quoted 19 lines · Sekhar Nori

Quoted 17 lines · Felipe Balbi

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 9 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +1 -1

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 
 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Quoted 3 lines

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

In that case I'm fine with the original patch in this series. Russell, got any better ideas?

Quoted 24 lines · Tony Lindgren

Quoted 22 lines · Sekhar Nori

Quoted 5 lines · Felipe Balbi

Quoted 4 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +0 -0

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 

Quoted 12 lines

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +1 -1

 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

Quoted 2 lines

In that case I'm fine with the original patch in this series. Russell, got any better ideas?

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

From the public TRM available for OMAP4470, even on that device, ROM

Quoted 25 lines · Tony Lindgren

Quoted 24 lines · Tony Lindgren

Quoted 22 lines · Sekhar Nori

Quoted 5 lines · Felipe Balbi

Quoted 4 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +0 -0

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 

Quoted 12 lines

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +1 -1

 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

Quoted 6 lines

Quoted 2 lines

In that case I'm fine with the original patch in this series. Russell, got any better ideas?

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

How does the following sound:

---
AM437x has L2C-310 version r3p2 and ROM code on that device does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

From the public TRM available for OMAP4470, even on that device, ROM does not support writing to this register even though it uses a version of L2C-310 which has the register implemented. So this patch should take care of all variants of existing OMAPs.
---

Two things that I have added are the version of L2C on AM437x and the fact that OMAP4470 also has the same issue (at least from the public TRM). Let me know if you would like to see anything else mentioned.

Quoted 3 lines · Tony Lindgren

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

How does the following sound:

---
AM437x has L2C-310 version r3p2 and ROM code on that device does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Quoted 1 line

From the public TRM available for OMAP4470, even on that device, ROM

does not support writing to this register even though it uses a version of L2C-310 which has the register implemented. So this patch should take care of all variants of existing OMAPs.
---

Quoted 7 lines · Sekhar Nori

Quoted 3 lines · Tony Lindgren

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

How does the following sound:

Quoted 8 lines

---
AM437x has L2C-310 version r3p2 and ROM code on that device does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Quoted 4 lines

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Quoted 5 lines

Quoted 1 line

From the public TRM available for OMAP4470, even on that device, ROM

does not support writing to this register even though it uses a version of L2C-310 which has the register implemented. So this patch should take care of all variants of existing OMAPs.
---

That sounds perfect, and explains why the change has to exist, and why it can't be fixed elsewhere. Thanks for providing the full reasoning in the commit message.

Quoted 13 lines · Russell King - ARM Linux

Quoted 12 lines · Sekhar Nori

Quoted 3 lines · Tony Lindgren

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

How does the following sound:

---
AM437x has L2C-310 version r3p2 and ROM code on that device does not support writing to L2C-310 power control register. The L2C driver, however, tries writing to this register for all revisions >= r3p0.

Quoted 3 lines

Quoted 3 lines

This leads to a warning dump on boot which leads most users to believe that L2 cache is non-functional.

Power controller register setting doesn't make cache controller functional but it is for really clock gating and standby. So please reword, the above statement accordingly.

Quoted 5 lines

Quoted 5 lines

Since the problem is understood, and cannot be addressed through software, replace the warning with a pr_info() while maintaining the WARN_ON() for other truly unexpected scenarios.

Instead of being vague here and below, I will just make it very simple as below.

On OMAP SOCs using PL310 controllers, Power_ctrl register is not accessible from non-secure software on PL310 versions which supports it. The secure code takes care of setting it up correctly and the power transitions are proven on these devices.

So lets add the ignore write check for PL310 Power_ctrl register write.

Quoted 28 lines · Tony Lindgren

Quoted 26 lines · Tony Lindgren

Quoted 19 lines · Sekhar Nori

Quoted 5 lines · Felipe Balbi

Quoted 4 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +0 -0

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 

Quoted 12 lines

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +1 -1

 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Quoted 3 lines

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

In that case I'm fine with the original patch in this series. Russell, got any better ideas?

Quoted 3 lines

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

How does the following sound:

---
AM437x has L2C-310 version r3p2 and ROM code on that device does not

Quoted 33 lines · Sekhar Nori

Quoted 31 lines · Tony Lindgren

Quoted 20 lines · Tony Lindgren

Quoted 19 lines · Sekhar Nori

Quoted 17 lines · Felipe Balbi

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 9 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +1 -1

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 
 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Quoted 6 lines

Quoted 3 lines

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

In that case I'm fine with the original patch in this series. Russell, got any better ideas?

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

Quoted 4 lines

How does the following sound:

---
AM437x has L2C-310 version r3p2 and ROM code on that device does not

at least my SoC has r3p3 (cache ID 0x9), other than that, commit log looks good to my eyes.

Quoted 24 lines · Tony Lindgren

Quoted 22 lines · Sekhar Nori

Quoted 5 lines · Felipe Balbi

Quoted 4 lines · Russell King - ARM Linux

arch/arm/mm/cache-l2x0.c +0 -0

@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 

Quoted 12 lines

arch/arm/mm/cache-l2x0.c static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v · line 736

Quoted 5 lines

arch/arm/mm/cache-l2x0.c +1 -1

 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;

even with this change, l2c still tries to write to power control register, at least on AM437x. Looking a little deeper here, AM437x identifies itself as l2c PL310 r3p3, which should have power control register, but aparentely there's no way to write that register. I'll file a bug to our ROM team, but we will certainly need a way to workaround this inside omap4-common.c

Looks like we need both my patch as well as Russell's patch. I can respin my patch with the pr_info_once() dropped if it helps further reduce boot noise.

Quoted 2 lines

In that case I'm fine with the original patch in this series. Russell, got any better ideas?

I guess no more comments. Took a look at the patch again, Sekhar, can you please update the description with what has been discovered in this thread and repost?

Quoted 32 lines · Tony Lindgren

Quoted 30 lines · Russell King - ARM Linux

Quoted 7 lines · Sekhar Nori

OMAP4430 had L2 cache controller version r2p0 (per the public TRM) which does not have this register. So unless there is a ROM API that was introduced after OMAP4430, this would not be there even for other OMAP4s. Public TRM of OMAP4470 does not indicate an API for this.

Before creating the patch, I checked with ROM team handling AM437x and they denied an API to write to this register was present in AM437x ROM.

Okay, so why are we trying to write to this register then...

Ah, we have a bug in cache-l2x0.c:

#define L2X0_CACHE_ID_PART_MASK         (0xf << 6)
#define L2X0_CACHE_ID_RTL_MASK          0x3f
#define L310_CACHE_ID_RTL_R3P0          0x05

        unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;

        if (rev >= L310_CACHE_ID_RTL_R2P0) {
...
        if (rev >= L310_CACHE_ID_RTL_R3P0) {
                l2c_write_sec(L310_DYNAMIC_CLK_GATING_EN | L310_STNDBY_MODE_EN,
                              base, L310_POWER_CTRL);

So, because we're masking the wrong bits, we end up with these tests always succeeding.

So that's a NACK for the original patch, it's the wrong fix. The right fix is to avoid writing this register by fixing the RTL masking.

Quoted 1 line

Okie dokie, dropping the omap specific fix.

Here's the revision mask fix - with the existing code, the revision checks are all useless since they would all pass irrespective of the actual revision. (Had the L2C series been better tested rather than being largely ignored, this may have been noticed before it was merged...) Anyway, what isn't clear from Sekhar's message is which revision L2C310 is in the AM437x.

From: Russell King <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] ARM: l2c: fix revision checking

The revision checking in l2c310_enable() was not correct; we were masking the part number rather than the revision number. Fix this to use the correct macro.

Fixes 4374d64933b1 ("ARM: l2c: add automatic enable of early BRESP")

Signed-off-by Russell King <[email protected]>

---

 arch/arm/mm/cache-l2x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mm/cache-l2x0.c +1 -1

index 948f12cf6180..0b5068256baf 100644
@@ -732,7 +732,7 @@ static int l2c310_cpu_enable_flz(struct notifier_block *nb, unsigned long act, v
 
 static void __init l2c310_enable(void __iomem *base, u32 aux, unsigned num_lock)
 {
-	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_PART_MASK;
+	unsigned rev = readl_relaxed(base + L2X0_CACHE_ID) & L2X0_CACHE_ID_RTL_MASK;
 	bool cortex_a9 = read_cpuid_part() == ARM_CPU_PART_CORTEX_A9;
 
 	if (rev >= L310_CACHE_ID_RTL_R2P0) {