[U-Boot] T2080 l2-cache-controller compatible string overwritten by ft_fixup_l2cache
Hi,
I was just looking at what it would take to add the T2080 L2 cache to the mpc85xx_edac driver in Linux. At a cursory glance all the registers appear to be there so I figured I'd just add the appropriate entry to the of match table.
To my surprise I found that the compatible string in my device tree was overwritten with "cache". I've tracked this down to the CONFIG_SYS_FSL_QORIQ_CHASSIS2 implementation of ft_fixup_l2cache in u-boot.
The CONFIG_L2_CACHE version seems to take care to update the device tree node to
compatible = "fsl,t2080-l2-cache-controller", "cache";
but the CONFIG_SYS_FSL_QORIQ_CHASSIS2 version just sets this to
compatible = "cache";
Is this an over-site or is it intentional?
Thanks, Chris
On 11/23/2016 01:43 AM, Chris Packham wrote:
Hi,
I was just looking at what it would take to add the T2080 L2 cache to the mpc85xx_edac driver in Linux. At a cursory glance all the registers appear to be there so I figured I'd just add the appropriate entry to the of match table.
To my surprise I found that the compatible string in my device tree was overwritten with "cache". I've tracked this down to the CONFIG_SYS_FSL_QORIQ_CHASSIS2 implementation of ft_fixup_l2cache in u-boot.
The CONFIG_L2_CACHE version seems to take care to update the device tree node to
compatible = "fsl,t2080-l2-cache-controller", "cache";
but the CONFIG_SYS_FSL_QORIQ_CHASSIS2 version just sets this to
compatible = "cache";
Is this an over-site or is it intentional?
I don't have any record of this discussion. Kumar wrote and committed this change. I hope he remembers.
York
Instead of setting the compatible property to "cache", append the desired value retaining what may already be set in the current property.
Signed-off-by: Chris Packham judge.packham@gmail.com --- On Thu, Nov 24, 2016 at 6:41 AM, york sun york.sun@nxp.com wrote:
On 11/23/2016 01:43 AM, Chris Packham wrote:
Hi,
I was just looking at what it would take to add the T2080 L2 cache to the mpc85xx_edac driver in Linux. At a cursory glance all the registers appear to be there so I figured I'd just add the appropriate entry to the of match table.
To my surprise I found that the compatible string in my device tree was overwritten with "cache". I've tracked this down to the CONFIG_SYS_FSL_QORIQ_CHASSIS2 implementation of ft_fixup_l2cache in u-boot.
The CONFIG_L2_CACHE version seems to take care to update the device tree node to
compatible = "fsl,t2080-l2-cache-controller", "cache";
but the CONFIG_SYS_FSL_QORIQ_CHASSIS2 version just sets this to
compatible = "cache";
Is this an over-site or is it intentional?
I don't have any record of this discussion. Kumar wrote and committed this change. I hope he remembers.
Here's a patch that retains the compatible property from the original dtb and adds the "cache" value if required. This gets the value I need through to the kernel.
arch/powerpc/cpu/mpc85xx/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 047c972ac78e..f31df41836d5 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -337,7 +337,8 @@ static inline void ft_fixup_l2cache(void *blob) fdt_setprop_cell(blob, l2_off, "cache-size", size); fdt_setprop_cell(blob, l2_off, "cache-sets", num_sets); fdt_setprop_cell(blob, l2_off, "cache-level", 2); - fdt_setprop(blob, l2_off, "compatible", "cache", 6); + if (fdt_node_check_compatible(blob, l2_off, "cache") == 1) + fdt_appendprop_string(blob, l2_off, "compatible", "cache"); }
if (l3_off < 0) {
Instead of setting the compatible property to "cache", append the desired value retaining what may already be set in the current property.
Signed-off-by: Chris Packham judge.packham@gmail.com --- On Thu, Nov 24, 2016 at 6:41 AM, york sun york.sun@nxp.com wrote:
On 11/23/2016 01:43 AM, Chris Packham wrote:
Hi,
I was just looking at what it would take to add the T2080 L2 cache to the mpc85xx_edac driver in Linux. At a cursory glance all the registers appear to be there so I figured I'd just add the appropriate entry to the of match table.
To my surprise I found that the compatible string in my device tree was overwritten with "cache". I've tracked this down to the CONFIG_SYS_FSL_QORIQ_CHASSIS2 implementation of ft_fixup_l2cache in u-boot.
The CONFIG_L2_CACHE version seems to take care to update the device tree node to
compatible = "fsl,t2080-l2-cache-controller", "cache";
but the CONFIG_SYS_FSL_QORIQ_CHASSIS2 version just sets this to
compatible = "cache";
Is this an over-site or is it intentional?
I don't have any record of this discussion. Kumar wrote and committed this change. I hope he remembers.
(re-sent because I flubbed the subject line and got bounced for Ccing all arch mainatiners, sorry for the spam)
Here's a patch that retains the compatible property from the original dtb and adds the "cache" value if required. This gets the value I need through to the kernel.
If it helps this is also consistent with the Linux documentation for this binding[1] which states that the compatible property should have both "<chip>-l2-cache-controller" and "cache" as values
[1] - Documentation/devicetree/bindings/powerpc/fsl/l2cache.txt
arch/powerpc/cpu/mpc85xx/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 047c972ac78e..f31df41836d5 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -337,7 +337,8 @@ static inline void ft_fixup_l2cache(void *blob) fdt_setprop_cell(blob, l2_off, "cache-size", size); fdt_setprop_cell(blob, l2_off, "cache-sets", num_sets); fdt_setprop_cell(blob, l2_off, "cache-level", 2); - fdt_setprop(blob, l2_off, "compatible", "cache", 6); + if (fdt_node_check_compatible(blob, l2_off, "cache") == 1) + fdt_appendprop_string(blob, l2_off, "compatible", "cache"); }
if (l3_off < 0) {
On 11/28/2016 07:10 PM, Chris Packham wrote:
Instead of setting the compatible property to "cache", append the desired value retaining what may already be set in the current property.
Signed-off-by: Chris Packham judge.packham@gmail.com
<snip>
arch/powerpc/cpu/mpc85xx/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 047c972ac78e..f31df41836d5 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -337,7 +337,8 @@ static inline void ft_fixup_l2cache(void *blob) fdt_setprop_cell(blob, l2_off, "cache-size", size); fdt_setprop_cell(blob, l2_off, "cache-sets", num_sets); fdt_setprop_cell(blob, l2_off, "cache-level", 2);
fdt_setprop(blob, l2_off, "compatible", "cache", 6);
if (fdt_node_check_compatible(blob, l2_off, "cache") == 1)fdt_appendprop_string(blob, l2_off, "compatible", "cache");}
if (l3_off < 0) {
You drop fdt_setprop, check the compatible "cache" and append it with "cache" again? I thought you wanted
compatible = "fsl,t2080-l2-cache-controller", "cache";
York
On Thu, Dec 1, 2016 at 6:18 AM, york sun york.sun@nxp.com wrote:
On 11/28/2016 07:10 PM, Chris Packham wrote:
Instead of setting the compatible property to "cache", append the desired value retaining what may already be set in the current property.
Signed-off-by: Chris Packham judge.packham@gmail.com
<snip>
arch/powerpc/cpu/mpc85xx/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 047c972ac78e..f31df41836d5 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -337,7 +337,8 @@ static inline void ft_fixup_l2cache(void *blob) fdt_setprop_cell(blob, l2_off, "cache-size", size); fdt_setprop_cell(blob, l2_off, "cache-sets", num_sets); fdt_setprop_cell(blob, l2_off, "cache-level", 2);
fdt_setprop(blob, l2_off, "compatible", "cache", 6);
if (fdt_node_check_compatible(blob, l2_off, "cache") == 1)fdt_appendprop_string(blob, l2_off, "compatible", "cache"); } if (l3_off < 0) {You drop fdt_setprop, check the compatible "cache" and append it with "cache" again? I thought you wanted
compatible = "fsl,t2080-l2-cache-controller", "cache";
I already have "fsl,t2080-l2-cache-controller" in my dts. Really I just want
fdt_appendprop_string(blob, l2_off, "compatible", "cache");
But the check is necessary because we run through this block multiple times (once per CPU). My initial version was
struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr())); int len; char buf[40];
len = sprintf(buf, "fsl,%c%s-l2-cache-controller",tolower(cpu->name[0]), cpu->name + 1) + 1; len += sprintf(buf + len, "cache") + 1;
fdt_setprop(blob, l2_off, "compatible", buf, len);
But that's more code.
York
On 11/30/2016 11:47 PM, Chris Packham wrote:
On Thu, Dec 1, 2016 at 6:18 AM, york sun york.sun@nxp.com wrote:
On 11/28/2016 07:10 PM, Chris Packham wrote:
Instead of setting the compatible property to "cache", append the desired value retaining what may already be set in the current property.
Signed-off-by: Chris Packham judge.packham@gmail.com
<snip>
arch/powerpc/cpu/mpc85xx/fdt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 047c972ac78e..f31df41836d5 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -337,7 +337,8 @@ static inline void ft_fixup_l2cache(void *blob) fdt_setprop_cell(blob, l2_off, "cache-size", size); fdt_setprop_cell(blob, l2_off, "cache-sets", num_sets); fdt_setprop_cell(blob, l2_off, "cache-level", 2);
fdt_setprop(blob, l2_off, "compatible", "cache", 6);
if (fdt_node_check_compatible(blob, l2_off, "cache") == 1)fdt_appendprop_string(blob, l2_off, "compatible", "cache"); } if (l3_off < 0) {You drop fdt_setprop, check the compatible "cache" and append it with "cache" again? I thought you wanted
compatible = "fsl,t2080-l2-cache-controller", "cache";
I already have "fsl,t2080-l2-cache-controller" in my dts. Really I just want
fdt_appendprop_string(blob, l2_off, "compatible", "cache");
I see.
But the check is necessary because we run through this block multiple times (once per CPU). My initial version was
struct cpu_type *cpu = identify_cpu(SVR_SOC_VER(get_svr())); int len; char buf[40];
len = sprintf(buf, "fsl,%c%s-l2-cache-controller",tolower(cpu->name[0]), cpu->name + 1) + 1; len += sprintf(buf + len, "cache") + 1;
fdt_setprop(blob, l2_off, "compatible", buf, len);
But that's more code.
Ideally we don't have to fix up dts. Since if we have to do it, I like the long version better. If the dts doesn't have correct compatible, the kernel won't take it, right?
York
participants (2)
-
Chris Packham -
york sun