From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 310D5C021B3 for ; Mon, 24 Feb 2025 10:01:07 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 95D4080079; Mon, 24 Feb 2025 11:01:05 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=freeshell.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; secure) header.d=freeshell.de header.i=@freeshell.de header.b="xa7OpayR"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 8ADAE805C3; Mon, 24 Feb 2025 11:01:04 +0100 (CET) Received: from freeshell.de (freeshell.de [116.202.128.144]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0240080017 for ; Mon, 24 Feb 2025 11:01:01 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=freeshell.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=e@freeshell.de Received: from [192.168.2.35] (unknown [98.97.26.251]) (Authenticated sender: e) by freeshell.de (Postfix) with ESMTPSA id 4CC55B2C21F7; Mon, 24 Feb 2025 11:00:59 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freeshell.de; s=mail; t=1740391261; bh=eRpheJLvk8uM9Rdeorbw0hVkmslsRa1+4lPSm6WtDTo=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=xa7OpayR8cPTGlk+ZFbiQ4Ffrgvw/LFRnO9b0g1kdHVxOGkqXG1C39taWg9nzOLea Ms2j/DnqYpieKiBICsjFUeutVZAY2BVlBtF+18cxW3uzX35Xz4zUajviwBvtix/09V uODE9HCPXjESdgqSt0TPii2lRzPlAjmsfZ/K/4Js= Message-ID: <9773425b-18c0-41ea-95e2-9d32f863c355@freeshell.de> Date: Mon, 24 Feb 2025 02:00:57 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/4] board: starfive: spl: strip off 'starfive/' prefix To: Heinrich Schuchardt Cc: Minda Chen , Hal Feng , Simon Glass , Yu-Chien Peter Lin , Sumit Garg , "Leon M . Busch-George" , Lukas Funke , H Bell , u-boot@lists.denx.de, Leo , Rick Chen References: <20250221095856.36530-1-heinrich.schuchardt@canonical.com> <20250221095856.36530-4-heinrich.schuchardt@canonical.com> <79f87309-a4a4-4332-8ea1-9a9df0174186@freeshell.de> <55917f4e-b354-4ff4-9708-417b5b9aa670@freeshell.de> Content-Language: en-US From: E Shattow In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On 2/23/25 07:39, Heinrich Schuchardt wrote: > On 2/23/25 15:18, E Shattow wrote: >> >> On 2/23/25 04:55, Heinrich Schuchardt wrote: >>> On 2/23/25 02:33, E Shattow wrote: >>>> >>>> >>>> On 2/21/25 01:58, Heinrich Schuchardt wrote: >>>>> The configuration descriptions generated by binman contain the vendor >>>>> device-tree directory. Instead of adding it to all match strings just >>>>> strip >>>>> it off. >>>>> >>>>> Signed-off-by: Heinrich Schuchardt >>>>> Reviewed-by: Leo Yu-Chi Liang >>>>> --- >>>>> v3: >>>>>      no change >>>>> v2: >>>>>      no change >>>>> --- >>>>>    board/starfive/visionfive2/spl.c | 4 ++++ >>>>>    1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/board/starfive/visionfive2/spl.c b/board/starfive/ >>>>> visionfive2/spl.c >>>>> index 22afd76c6b9..d63eb1abe6a 100644 >>>>> --- a/board/starfive/visionfive2/spl.c >>>>> +++ b/board/starfive/visionfive2/spl.c >>>>> @@ -118,6 +118,10 @@ int board_fit_config_name_match(const char *name) >>>>>          product_id = get_product_id_from_eeprom(); >>>>>    +    /* Strip off prefix */ >>>>> +    if (strncmp(name, "starfive/", 9)) >>>>> +        return -EINVAL; >>>>> +    name += 9; >>>>>        if (!strncmp(product_id, "VF7110", 6)) { >>>>>            version = get_pcb_revision_from_eeprom(); >>>>>            if ((version == 'b' || version == 'B') && >>>> >>>> Let's insist on logic statements in board_fit_config_name_match() >>>> callback that begin with literal items (no pointer math trickery) from >>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and in that >>>> order: >>> >>> Thank you for reviewing. >>> >>> Unfortunately your sentence starting with "Let's insist" does not >>> provide insight into your reasoning. >>> >> >> On first sight I do not like "code golf" of pointer math on the function >> parameter in-place. It seems to be okay here but does get my attention >> to look closer. >> >> On closer look what I do want to see is 1:1 continuity between what is >> in configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST and the overall >> form of this logic block so that it is uncomplicated to add more >> variants to this board target. We assume there is a pattern "starfive/" >> here but there is no such thing, this is wrong to do that. The >> originating list in the Makefile may contain literals that do not have a >> "starfive/" prefix. >> >>> Why do you want to add the 'starfive/' to each of the strings we compare >>> instead of checking the common prefix first and the remainder next which >>> results in a smaller binary? >> >> What are the limits on binary size, here? How important is the need to >> create this assumption of "starfive/" prefix? >> >>> >>>> >>>> #if CONFIG_IS_ENABLED(LOAD_FIT) >>>> int board_fit_config_name_match(const char *name) >>>> { >>>>      if(!strcmp(name, "starfive/jh7110-milkv-mars") && >>>>         !strncmp(get_product_id_from_eeprom(), "MARS", 4)) { >>>>          return 0; >>>>      } else if((!strcmp(name, "starfive/jh7110-pine64-star64")) && >>>>         !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) { >>>>          return 0; >>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2- >>>> v1.2a")) && >>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) { >>>>          switch (get_pcb_revision_from_eeprom()) { >>>>          case 'a': >>>>          case 'A': >>>>              return 0; >>>>          } >>>>      } else if((!strcmp(name, "starfive/jh7110-starfive-visionfive-2- >>>> v1.2b")) && >>>>         !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) { >>>>          switch (get_pcb_revision_from_eeprom()) { >>>>          case 'b': >>>>          case 'B': >>>>              return 0; >>>>          } >>>>      } >>>> >>>>      return -EINVAL; >>>> } >>>> #endif >>>> >>>> Not sure about code style so that is simply an example of keeping the >>>> sort order the same as how it exists in >>>> configs/starfive_visionfive2_defconfig:CONFIG_OF_LIST >>>> >>>> Mars CM (and CM Lite) logic may be dropped since those targets do not >>>> exist at the moment in starfive_visionfive2_defconfig:CONFIG_OF_LIST >>>> however, I do anticipate to submit for review into Linux upstream soon >>>> and to begin that process. A donation board was sent to me so I now >>>> have >>>> Mars CM to test as well as Mars CM Lite. >>> >>> As you are planning to upstream the boards I suggest to keep those >>> lines. You could already use them by manually copying the device-trees >>> into the upstream dtb folder and adding the files to CONFIG_OF_LIST. >> >> Yes, this dead code can be some removed with other cleanup, it does not >> have to be this series. >> >>> >>>> >>>> Are the duplicate string definitions and logic in >>>> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() etc. >>>> still appropriate, could those now be factored out? I think Simon's >>>> suggestion (in reply on IRC) of CONFIG_FIT_BEST_MATCH could replace >>>> that >>>> functionality? -E >>> >>> $fdtfile is used for loading a device-tree from the ESP. I can't see how >>> CONFIG_FIT_BEST_MATCH which controls choosing FIT configurations is >>> related. We cannot set $fdtfile from SPL. >>> >>> Best regards >>> >>> Heinrich >> >> I've now tested CONFIG_FIT_BEST_MATCH and it works as suggested: >> >> --- a/configs/starfive_visionfive2_defconfig >> +++ b/configs/starfive_visionfive2_defconfig >> @@ -33,6 +33,7 @@ CONFIG_RISCV_SMODE=y >>   # CONFIG_OF_BOARD_FIXUP is not set >>   # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set >>   CONFIG_FIT=y >> +CONFIG_FIT_BEST_MATCH=y >>   CONFIG_BOOTSTD_DEFAULTS=y >>   CONFIG_BOOTSTAGE=y >>   CONFIG_QSPI_BOOT=y >> >> also delete function >> board/starfive/visionfive2/starfive_visionfive2.c:set_fdtfile() and >> where it is called from within board_late_init(). Then $fdtfile is set >> not defined so the best fit is selected, or, if $fdtfile is user-defined >> then the user-defined path is loaded (same as before). The difference is >> there is not any $fdtfile env variable defined for the typical situation >> but it does apparently have some kind of heuristic and loads something > > From what you write above I take that variable $fdtfile has not been set. > > Have you successfully booted using the EFI boot manager and verified > that it is picking up the correct device-tree from the ESP and not the > one from u-boot.itb? > > I wouldn't know how that should work as in the EFI boot manager we rely > on environment variable $fdtfile: > > lib/efi_loader/efi_fdt.c:55: >     fdt_fname = env_get("fdtfile"); > > Best regards > > Heinrich > >> appropriate, I am not sure if it is from U-Boot or if it is from the >> search path. This makes sense to me since we're OF_UPSTREAM now. Anyhow >> we can drop a lot of unnecessary duplicate logic and code this way. That >> change could as part of this series or as a follow-up. >> > This works the same way, you set fdtfile to load device-tree from the ESP. If fdtfile is not set we are already have a valid fdt for Linux in u-boot.itb so it should not be any problem. Do I understand this correctly? -E