diff mbox

[U-Boot] powerpc: Retain compatible property for L2 cache

Message ID 20161129031044.26089-1-judge.packham@gmail.com
State Superseded
Delegated to: York Sun
Headers show

Commit Message

Chris Packham Nov. 29, 2016, 3:10 a.m. UTC 
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(-)

Comments

York Sun Nov. 30, 2016, 5:18 p.m. UTC | #1 
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
Chris Packham Dec. 1, 2016, 7:47 a.m. UTC | #2 
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
York Sun Dec. 1, 2016, 5:34 p.m. UTC | #3 
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
Chris Packham Dec. 2, 2016, 7:35 a.m. UTC | #4 
On 2/12/2016 6:34 AM, "york sun" <york.sun@nxp.com> wrote:
>
> 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?
>

Ok. I'll post a v2 based on the long version. As it's a repeat of code in
another block I'll see if i can extract it to a helper function.
diff mbox

Patch

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) {