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

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@gmail.com wrote:

...

On Fri, Jul 19, 2024 at 4:12 PM Heinrich Schuchardt heinrich.schuchardt@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@gmail.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@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@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