[PATCH 1/1] board: fix compatible property Milk-V Mars CM

[Heinrich Schuchardt]

On 7/21/24 23:27, E Shattow wrote:
> P.S. my suggestion below
> 
> On Fri, Jul 19, 2024 at 5:06 PM E Shattow <lucent at gmail.com> wrote:
>>
>> On Fri, Jul 19, 2024 at 4:12 PM Heinrich Schuchardt
>> <heinrich.schuchardt at canonical.com> wrote:
>>>
>>> For the Milk-V Mars CM (lite) we have only been copying sizeof(void *)
>>> bytes to the compatible property instead of the whole string list.
>>
>> This const char array must be so that we may get an accurate data
>> length with sizeof() but it highlights there are helper functions for
>> get of fdt stringlist and not for set of fdt stringlist.
>>
>>>
>>> Fixes: de3229599d4f ("board: add support for Milk-V Mars CM")
>>> Reported-by: E Shattow <lucent at gmail.com>
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>> ---
>>>   board/starfive/visionfive2/spl.c | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/visionfive2/spl.c
>>> index b794b73b6bd..f55c6b5d34c 100644
>>> --- a/board/starfive/visionfive2/spl.c
>>> +++ b/board/starfive/visionfive2/spl.c
>>> @@ -170,23 +170,32 @@ void spl_fdt_fixup_mars_cm(void *fdt)
>>>   {
>>>          const char *compat;
>>>          const char *model;
>>> +       int compat_size;
>>>
>>>          spl_fdt_fixup_mars(fdt);
>>>
>>>          if (!get_mmc_size_from_eeprom()) {
>>>                  int offset;
>>> +               static const char
>>> +               compat_cm_lite[] = "milkv,mars-cm-lite\0starfive,jh7110";
>>>
>>>                  model = "Milk-V Mars CM Lite";
>>> -               compat = "milkv,mars-cm-lite\0starfive,jh7110";
>>> +               compat = compat_cm_lite;
>>> +               compat_size = sizeof(compat_cm_lite);
>>>
>>>                  offset = fdt_path_offset(fdt, "/soc/pinctrl/mmc0-pins/mmc0-pins-rest");
>>>                  /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */
>>>                  fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016);
>>>          } else {
>>> +               static const char
>>> +               compat_cm[] = "milkv,mars-cm\0starfive,jh7110";
>>> +
>>>                  model = "Milk-V Mars CM";
>>> -               compat = "milkv,mars-cm\0starfive,jh7110";
>>> +               compat = compat_cm;
>>> +               compat_size = sizeof(compat_cm);
>>>          }
>>> -       fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible", compat, sizeof(compat));
>>> +       fdt_setprop(fdt, fdt_path_offset(fdt, "/"),
>>> +                   "compatible", compat, compat_size);
>>>          fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model);
>>>   }
>>>
>>> --
>>> 2.45.2
>>>
>>
>> -E
> 
> What about something like as follows?
> 
>   void spl_fdt_fixup_mars(void *fdt)
>   {
> -       static const char compat[] = "milkv,mars\0starfive,jh7110";
>          u32 phandle;
>          u8 i;
>          int offset;
>          int ret;
> 
> -       fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible",
> compat, sizeof(compat));
> -       fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model",
> -                          "Milk-V Mars");
> +       offset = fdt_path_offset(fdt, "/");
> +       fdt_setprop_string(fdt,    offset, "compatible", "milkv,mars");
> +       fdt_appendprop_string(fdt, offset, "compatible", "starfive,jh7110");
> +       fdt_setprop_string(fdt,    offset, "model",      "Milk-V Mars");
> 
>          /* gmac0 */
>          offset = fdt_path_offset(fdt, "/soc/clock-controller at 17000000");
> @@ -164,30 +226,31 @@ void spl_fdt_fixup_mars(void *fdt)
>                                  break;
>                  }
>          }
>   }
> 
>   void spl_fdt_fixup_mars_cm(void *fdt)
>   {
> -       const char *compat;
> -       const char *model;
> +       int offset;
> 
>          spl_fdt_fixup_mars(fdt);
> 
>          if (!get_mmc_size_from_eeprom()) {
> -               int offset;
> -
> -               model = "Milk-V Mars CM Lite";
> -               compat = "milkv,mars-cm-lite\0starfive,jh7110";
> +               offset = fdt_path_offset(fdt, "/");
> +               fdt_setprop_string(fdt, offset, "compatible",
> "milkv,mars-cm-lite");
> +               fdt_appendprop_string(fdt, offset, "compatible",
> "starfive,jh7110");
> +               fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM Lite");
> 
>                  offset = fdt_path_offset(fdt,
> "/soc/pinctrl/mmc0-pins/mmc0-pins-rest");
>                  /* GPIOMUX(22, GPOUT_SYS_SDIO0_RST, GPOEN_ENABLE, GPI_NONE) */
>                  fdt_setprop_u32(fdt, offset, "pinmux", 0xff130016);
>          } else {
> -               model = "Milk-V Mars CM";
> -               compat = "milkv,mars-cm\0starfive,jh7110";
> +               offset = fdt_path_offset(fdt, "/");
> +               fdt_setprop_string(fdt, offset, "compatible", "milkv,mars-cm");
> +               fdt_appendprop_string(fdt, offset, "compatible",
> "starfive,jh7110");
> +               fdt_setprop_string(fdt, offset, "model", "Milk-V Mars CM");
>          }
> -       fdt_setprop(fdt, fdt_path_offset(fdt, "/"), "compatible",
> compat, sizeof(compat));
> -       fdt_setprop_string(fdt, fdt_path_offset(fdt, "/"), "model", model);
>   }
> 
>   void spl_fdt_fixup_version_a(void *fdt)
> @@ -235,6 +298,7 @@ void spl_fdt_fixup_version_a(void *fdt)
>                                  break;
>                  }
>          }
> +
>   }
> 
> 
> And similar changes to the other "string1\0string2" settings in the
> same source file.
> 
> -E

You did not mention why you would prefer three function calls (including 
strlen()) instead of one.

Your code should work too but results in a larger code size.

Probably the shortest code would use a common function with parameters 
'model' and 'compatible_1st' for each of the boards and then use 
fdt_appendprop_string() to append the common 2nd compatible string.

Best regards

Heinrich