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 7B3B6C021B2 for ; Sun, 23 Feb 2025 14:18:39 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9CAC280079; Sun, 23 Feb 2025 15:18:37 +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="B6cl08BR"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D660480087; Sun, 23 Feb 2025 15:18:36 +0100 (CET) Received: from freeshell.de (freeshell.de [IPv6:2a01:4f8:231:482b::2]) (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 8D40B80017 for ; Sun, 23 Feb 2025 15:18:34 +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 87E19B2202A2; Sun, 23 Feb 2025 15:18:31 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freeshell.de; s=mail; t=1740320314; bh=/vE/GgBiL26bDKe+Kq4YqweObHZaocH8cTHuzY255TE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=B6cl08BR6T8xNG8+87H+3CDhcK5XJq9vqFVfmZ/w1vSks7lvRBg4vuDwdtj9JJDCI Y8IufBailPKGJil1RPUThUCbpE/27z/IInYTVPTDRSJuQDFNFvT/0yakV7MYfcvRQh Y9eIOq/E9sCyLEFVocHI5KPF/C2L7Ire5q9HEskY= Message-ID: <55917f4e-b354-4ff4-9708-417b5b9aa670@freeshell.de> Date: Sun, 23 Feb 2025 06:18:29 -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> 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 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 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. -E