Re: [PATCH 1/4] common/board_f: remove XTRN_DECLARE_GLOBAL_DATA_PTR dead code
Dear Ovidiu,
In message <20220911161052.2986264-1-ovpan...@gmail.com> you wrote:
> The XTRN_DECLARE_GLOBAL_DATA_PTR declarations in ppc code are permanently
> commented out, so there are no users for this macro:
> #if 1
> #define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm ("r2")
> #else
> #define XTRN_DECLARE_GLOBAL_DATA_PTR extern
> #define DECLARE_GLOBAL_DATA_PTR XTRN_DECLARE_GLOBAL_DATA_PTR \
> gd_t *gd
> #endif
>
> Remove all references.
Actually the commented out code contained some information, and I
feel it would be a pity if that got lost:
> -#if 1
> #define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm ("r2")
> -#else /* We could use plain global data, but the resulting code is bigger */
> -#define XTRN_DECLARE_GLOBAL_DATA_PTR extern
> -#define DECLARE_GLOBAL_DATA_PTR XTRN_DECLARE_GLOBAL_DATA_PTR \
> - gd_t *gd
> -#endif
Maybe we can keep the information that using global data for the GD
pointer would be possible too (and simpler, as it does not require
the reservation of a specific register for it), but that the
implementation uses a register nevertheless because this results in
smaller code?
Maybe add such a comment instead ?
Thanks!
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr. 5, 82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
All repairs tend to destroy the structure, to increase the entropy
and disorder of the system. Less and less effort is spent on fixing
original design flaws; more and more is spent on fixing flaws intro-
duced by earlier fixes. - Fred Brooks, "The Mythical Man Month"
Re: [PATCH] image: Ensure image header name is null terminated
Dear Joel,
In message <20220823055907.416060-1-j...@jms.id.au> you wrote:
> When building with GCC 12:
>
> ../include/image.h:779:9: warning: ‘strncpy’ specified bound 32 equals
> destination size [-Wstringop-truncation]
> 779 | strncpy(image_get_name(hdr), name, IH_NMLEN);
> | ^~~~
>
> Ensure the copied string is null terminated by always setting the final
> byte to 0. Shorten the strncpy to IH_NMLEN-1 as we will always overwrite
> the last byte.
>
> We can't use strlcpy as this is code is built on the host as well as the
> target.
>
> Fixes: b97a2a0a21f2 ("[new uImage] Define a API for image handling
> operations")
> Signed-off-by: Joel Stanley
> ---
> include/image.h | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/image.h b/include/image.h
> index e4c6a50b885f..665b2278b7fb 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -776,7 +776,10 @@ image_set_hdr_b(comp)/* image_set_comp */
>
> static inline void image_set_name(image_header_t *hdr, const char *name)
> {
> - strncpy(image_get_name(hdr), name, IH_NMLEN);
> + char *hdr_name = image_get_name(hdr);
> +
> + strncpy(hdr_name, name, IH_NMLEN - 1);
> + hdr_name[IH_NMLEN - 1] = '\0';
> }
Why don't you use strlcpy() instead? This covers exactly the
situation we have here.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr. 5, 82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
panic: can't find /
Re: [PATCH 1/3] Licenses: Clarify exceptions for standalone apps
Dear Paul Barker, In message <20220505153242.1598807-2-paul.bar...@sancloud.com> you wrote: > On 2010-01-27, an email [1] was sent to the mailing list by Wolfgang > Denk which clarified the intended licensing exceptions for standalone > applications. As the "export.h" header and the "stubs.c" source files > are required to implement a standalone application, the intention was > that these files be covered by the licensing exception. This is made > clear in the following quotes from that email: > > "exports.h" should be added to the "allowed" file list; there should > be no need to include "common.h". Eventually this needs fixing. > Patches are welcome. > > "examples/standalone/stubs.c" should be added to the "allowed" file > list (the ppc_*jmp.S files are LGPLed). > > There should be no doubts - the intention is clear, the current state > may need improvement. Help (read: patches) welcome. > > [1]: https://lists.denx.de/pipermail/u-boot/2010-January/067174.html > > Signed-off-by: Paul Barker > Cc: Wolfgang Denk Acked-by: Wolfgang Denk Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr. 5, 82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de It usually takes more than three weeks to prepare a good impromptu speech. - Mark Twain
Re: [PATCH 3/3] exports: Fix export of SPI access functions
Dear Paul, In message <20220505153242.1598807-4-paul.bar...@sancloud.com> you wrote: > * With CONFIG_DM_SPI defined, spi_get_bus_and_cs needs to be used > instead of spi_setup_slave to configure a SPI bus. As spi_setup_slave is > already present in the export list it is reasonable to also export > spi_get_bus_and_cs. > > * For the functions listed in the jump table to be callable they must > also be defined in the "exports.h" header. Define the various exported > SPI functions so that they can be used. > > Signed-off-by: Paul Barker > --- > include/_exports.h | 4 > include/exports.h | 15 ++- > 2 files changed, 18 insertions(+), 1 deletion(-) Sorry, but I disagree here. The SPI functions should have never been part of the export interface. As far as I can see now, they have been added by commit bedd8403f7 export SPI functions to standalone apps While we're here, fix the broken #ifdef handling in _exports.h. Signed-off-by: Mike Frysinger in 2009. Actually I must even have seen them, as I complained about incorrect comment style :-( But no, SPI support should not be inclluded. I2C was there because it was needed for reading the environment from an EEPROM, but then the interface was frozen ano no more new interfices / drivers should be allowed. Viele Grüße, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr. 5, 82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de If you can't explain it to a six year old, you don't understand it yourself. - Albert Einstein
Re: [PATCH 2/3] examples: hello_world: Drop inclusion of common header
Dear Paul, In message <20220505153242.1598807-3-paul.bar...@sancloud.com> you wrote: > The "common.h" header is not covered by the licensing exception for > standalone applications. Let's drop inclusion of this header from the > hello_world example to prove that a standalone app can be built without > it. > > Signed-off-by: Paul Barker > --- > examples/standalone/hello_world.c | 1 - > 1 file changed, 1 deletion(-) Acked-off-by: Wolfgang Denk Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr. 5, 82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de An expert is a person who avoids the small errors while sweeping on to the grand fallacy.
Re: [ANN] U-Boot v2021.04 released
40 (2.4%) Texas Instruments 39 (2.3%) Wind River 38 (2.3%) ... Top lines changed by employer (Unknown) 65153 (46.0%) Konsulko Group19605 (13.8%) Google, Inc. 17340 (12.2%) NXP 7103 (5.0%) Texas Instruments 5418 (3.8%) Linaro5391 (3.8%) BayLibre SAS 4113 (2.9%) DENX Software Engineering 3728 (2.6%) Phytec2827 (2.0%) Intel 2286 (1.6%) ... Employers with the most signoffs (total 245) (Unknown) 50 (20.4%) NXP 46 (18.8%) ARM 28 (11.4%) BayLibre SAS26 (10.6%) Texas Instruments 21 (8.6%) Xilinx 18 (7.3%) Novell 17 (6.9%) Intel 10 (4.1%) Samsung 10 (4.1%) ST Microelectronics 6 (2.4%) ... Employers with the most hackers (total 196) (Unknown) 110 (56.1%) NXP 19 (9.7%) Texas Instruments7 (3.6%) Xilinx 6 (3.1%) Intel5 (2.6%) ST Microelectronics 5 (2.6%) Samsung 4 (2.0%) Linaro 4 (2.0%) DENX Software Engineering4 (2.0%) Marvell 4 (2.0%) ... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "A great many people think they are thinking when they are merely re- arranging their prejudices." - William James
Re: [cmd] clarification on syntax of 'chpart' command
Dear Adarsh,
In message
you wrote:
>
> I was trying to verify and use 'chpart' command on Beaglebone black with an
> SDHC card ( with multiple partitions ) and am unable to get its exact
> syntax.Is this command applicable to
> any particular category of storage devices (viz. NOR flash,NAND flash etc)??
I agree that it is not obvious from the help output, but the fact
that the chpart command is implemented in the cmd/mtdparts.c source
file gives a clear hint: it works only for MTD devices, not for
general storage devides using DOS (or GPT) partitions.
> => mmc dev 0
> switch to partitions #0, OK
> mmc0 is current device
This is what you should look for. THe help output shoud tell you:
...
mmc dev [dev] [part] - show or set current mmc device [partition]
...
So instead of
> => chpart mmc0:2
> Device nand0 not found!
you would use:
=> mmc dev 0 2
Hope this helps.
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
A person without a sense of humor is like a wagon without springs --
jolted by every pebble in the road. - Henry Ward Beecher (1813-1887)
Re: U-Boot statistics update
307 (4.0%) NXP 4974 (2.4%) ST Microelectronics 4373 (2.1%) Linaro3759 (1.8%) Broadcom 3655 (1.8%) ... Employers with the most signoffs (total 252) (Unknown) 57 (22.6%) NXP 53 (21.0%) Xilinx 34 (13.5%) Broadcom17 (6.7%) Novell 17 (6.7%) Texas Instruments 15 (6.0%) Konsulko Group 15 (6.0%) Marvell 6 (2.4%) Amarula Solutions6 (2.4%) Google, Inc. 5 (2.0%) ... Employers with the most hackers (total 231) (Unknown) 109 (47.2%) NXP 28 (12.1%) Texas Instruments 14 (6.1%) DENX Software Engineering9 (3.9%) Xilinx 7 (3.0%) Linaro 7 (3.0%) Broadcom 6 (2.6%) Marvell 6 (2.6%) ST Microelectronics 5 (2.2%) Intel4 (1.7%) ... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The trouble with our times is that the future is not what it used to be. - Paul Valery
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
Dear Chris Packham,
In message <20200813013727.8186-1-judge.pack...@gmail.com> you wrote:
> When building envtools via tools-only_defconfig the builtin defaults
> are based on options in the defconfig. For example:
>
> bootcmd=bootp; setenv bootargs root=/dev/nfs
> nfsroot=${serverip}:${rootpath}
> ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; bootm
> bootdelay=2
> baudrate=115200
> stdin=serial,cros-ec-keyb,usbkbd
> stdout=serial,vidconsole
> stderr=serial,vidconsole
> ethaddr=00:00:11:22:33:44
> eth3addr=00:00:11:22:33:45
> eth5addr=00:00:11:22:33:46
> eth6addr=00:00:11:22:33:47
Having any specific MAC addresses set in the default environment has
always been a strict No-Go as it is just begging for trouble.
If this was a real example, that code should be fixed!!
> These may or may not be sensible for any particular target. Rather than
> trying to have a set of defaults that work for every target add a config
> option to make the default environment completely empty.
An empty default environment is even worse, as you are missing vital
settings like baudrate, bootdelay etc. There are good chances to
brick your board with such settings.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The word "fit", as I understand it, means "appropriate to a purpose",
and I would say the body of the Dean is supremely appropriate to the
purpose of sitting around all day and eating big heavy meals.
- Terry Pratchett, _Moving Pictures_
Re: [PATCH v4 1/9] CONFIG_NR_DRAM_BANKS: Remove unreferenced code as its always defined
Dear Stefan Roese,
In message <20200813054800.469284-2...@denx.de> you wrote:
> Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
Has there been any evaluation about the impact this change had on
both text and data sizes of the resulting U-Boot image?
Especially the default value of 4 makes no sense to me - whiy is
this not 1?
> It makes no sense to still carry code that is guarded with
> "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> all these unreferenced code paths.
...thus futher hiding where we just lost another lof of memory, for
no advantage.
Sic...
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
If programming was easy, they wouldn't need something as complicated
as a human being to do it, now would they?
- L. Wall & R. L. Schwartz, _Programming Perl_
Re: [PATCH v4 4/9] global: Move from bi_memstart/memsize -> gd->ram_base/ram_size
Dear Stefan,
In message <20200813054800.469284-5...@denx.de> you wrote:
> With the removal of bi_memstart & bi_memsize, this patch now moves the
> references to the better suiting gd->ram_base/ram_size variables.
This sounds as if bi_memstart & bi_memsize have already been
removed, in whih case the patch order would be wrong as it would
be unbisectable ; but if I see his right the actual removel will
only follow in later patches ("[PATCH v4 7/9] powerpc: Remove
bi_memstart & bi_memsize assignments in spl.c" etc.) ?
In this case it might be better to write something like:
"With the _planned_ removal ..."
or such?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
This was an amazing creation using small squares of compressed
vegetable matter on which letters were made by means of small carbon
particles from a stylus, giving an effect similar to the traditional
word-processor screen. It seemed amazingly portable and I never once
saw him have to change the batteries.
- Terry Pratchett & Stephen Briggs, _The Discworld Companion_
Re: U-Boot stats
5814 (4.3%) Xilinx5033 (3.7%) DENX Software Engineering 3566 (2.6%) ... Employers with the most signoffs (total 285) NXP100 (35.1%) (Unknown) 79 (27.7%) Xilinx 17 (6.0%) ST Microelectronics 12 (4.2%) BayLibre SAS11 (3.9%) Broadcom10 (3.5%) Novell 10 (3.5%) Texas Instruments9 (3.2%) Konsulko Group 8 (2.8%) Amarula Solutions7 (2.5%) ... Employers with the most hackers (total 207) (Unknown) 91 (44.0%) NXP 30 (14.5%) Texas Instruments 10 (4.8%) Xilinx 9 (4.3%) DENX Software Engineering8 (3.9%) ST Microelectronics 7 (3.4%) Toradex 6 (2.9%) Linaro 4 (1.9%) Wind River 4 (1.9%) Collabora Ltd. 4 (1.9%) ... Full statistics as usual at http://www.denx.de/wiki/U-Boot/UbootStat_2020_07 Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Sorry, but my karma just ran over your dogma.
Re: [PATCH v4 1/9] CONFIG_NR_DRAM_BANKS: Remove unreferenced code as its always defined
Dear Stefan, In message <8ddb672c-956c-68d2-ceb4-d77f96cdf...@denx.de> you wrote: > > > Especially the default value of 4 makes no sense to me - whiy is > > this not 1? > > I can't really tell. I can only assume, that it originates from this > patch: Yes, probably. I think the issue here resutlsfromt he fact that those who worked on the patches came primarily from architectures where this array of banks has been used; for Power Architecture systems this has (for a long, long time) not been the case - there we would map several memory banks (after determining their respecitve sizes) into one contiguous area of memory starting at address 0. So on all such system the definition of this array meand code and data overhead, as it is not needed. > > ...thus futher hiding where we just lost another lof of memory, for > > no advantage. > > > > Sic... > > I'm not sure, what you mean with "lost lot of memory"? We carried > referenced code for ~2 years, which this patch series now tries to > solve. See above. On most PPC systems this array of sizes and addresses is not needed at all, and _if_ code is changed to use such an array, it should not waste even more memory be defining room for 4 banks where there usually will be only one. Sorry, I know that you are not to blame for that... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Make it right before you make it faster.
Re: [RFC PATCH] tools: env: Add an option to have an empty default environment
Dear Chris, In message you wrote: > > What I think might be the best path forward is being able to supply > defaults via a file at run time. That way we can have a generic fw_setenv > but supply board specific defaults via a config file. This would indeed make sense (though I would not call this any longer "default" then). Patches welcome! > Content-Type: text/html; charset="UTF-8" > Content-Transfer-Encoding: quoted-printable And please - stop sending HTML to the list! Thanks. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "Pull the wool over your own eyes!"- J.R. "Bob" Dobbs
Re: Bug tracking
Dear Pratyush, In message <20210325193843.qniaryw2xxgkz...@ti.com> you wrote: > > You would need people to maintain the bugs that are reported in the > tracker. Asking for clear, reproducible info, closing duplicates or old > bugs, etc. Then you need people dedicated to fixing those bugs. Sure, > some can be taken up by subsystem maintainers or active devs, but what > to do with the ones that aren't? > > My point is, having a bug tracker needs volunteers to help maintain it. > Otherwise it would not be very useful and important bugs would get > drowned in the noise or be left stale. We can experiment with it but we > need to keep in mind the extra effort required. I think it is worth to start such an experiment. Eventually there are people out there who don;t have experience, time and resources for bigger development tasks, but who might help out with smaller tasks like bug fixing. > > Another option is to use source.denx.de but that would require > > allowing anyone to register so is probably a non-starter. > > Honestly, source.denx.de makes the most sense to me. I would expect the > Gitlab instance where the repo is hosted to also be where the bug > tracker is hosted. Makes it much easier to find. I fully agree here; this is also my suggestion. > But if allowing anyone to register is a no-go, then I would prefer > something decentralized/non-proprietary to keep the project independent > of the host. But people generally aren't very enthusiastic about those > because the proprietary solutions are just so easy to use... It is not a strict no-go, but we will have to think about some form of rate limiting - either in form of approvals (which would add to our work load) or allowing only OAUTH logins from other platforms as it's done for example here [1]. [1] https://gitlab.gnome.org/users/sign_in And we should make sure that only bugs against mainline versions can be reported - it makes no sense to collect bug information for out-of-tree code where we might not even have access to. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de In the realm of scientific observation, luck is granted only to those who are prepared. - Louis Pasteur
[IMPORTANT] gitlab downtime
Hi all, I just reeived a notification from our hoster that our gitlab server ( source.denx.de ) will be offline for approx. 2 hours between now and 15:45 CEST (urgent maintenance work). Sorry for the inconvenience, but there is nothing we can do... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A direct quote from the Boss: "We passed over a lot of good people to get the ones we hired."
Re: [PATCH] powerpc: remove WATCHDOG_RESET call from wait_ticks()
Dear Rasmus, In message <20200316212337.30204-1-rasmus.villem...@prevas.dk> you wrote: > wait_ticks() is only used by the ppc-specific __udelay() > function. Having the powerpc version of __udelay implicitly call > WATCHDOG_RESET() is inconsistent with other architectures' (and the > generic __udelay() in lib/time.c) implementations. It also means a > driver cannot use __udelay() as the raw primitive it is supposed to be > - e.g. a watchdog driver that needs to do a short delay between two > operations needed to perform a ping sequence. Many PPC processors implement the watchdog differently than other sysytems - for example, on many systems the watchdog is automatically enabled after power on / reset. > There are not that many __udelay() calls, so I doubt this causes a > regression for anyone. Callers of udelay() are not affected, since > udelay() itself does one WATCHDOG_RESET() per __udelay() call. Which exact platforms have you tested this on? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Diplomacy is the art of saying "nice doggy" until you can find a rock.
Re: [PATCH] powerpc: remove WATCHDOG_RESET call from wait_ticks()
Dear Rasmus, In message <1b6c7efd-8264-6cb5-0b39-3223bae5f...@prevas.dk> you wrote: > > Or do you not agree that __udelay is supposed to be a raw primitive that > does the delay and nothing else? I agree, and it does that - it converts the microseconds into ticks, and calls wait_ticks(), and does nothing else. It's the wait_ticks() implementation which references the macro WATCHDOG_RESET. > >> There are not that many __udelay() calls, so I doubt this causes a > >> regression for anyone. Callers of udelay() are not affected, since > >> udelay() itself does one WATCHDOG_RESET() per __udelay() call. > > > > Which exact platforms have you tested this on? > > An MPC8309-derived board which does not utilize the SOCs watchdog but > has an external gpio-triggered (always-running) watchdog circuit. This is not even close to global coverage. Are you aware that there is the PPC-global implementation of wait_ticks() in arch/powerpc/lib/ticks.S , plus another MPC83xx specific one in drivers/timer/mpc83xx_timer.c? I don't even understand why MPC83xx needs special treatment. In any case it seems to me a board specific redefinition of the WATCHDOG_RESET macro would be less intrusive and risky than changing code that has been there since the beginning of time (well, at least more than 18 years). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de I am not now, nor have I ever been, a member of the demigodic party. -- Dennis Ritchie
Re: [PATCH] powerpc: remove WATCHDOG_RESET call from wait_ticks()
Dear Rasmus, In message <01193803-be8f-865d-5fce-2c7cee0fd...@prevas.dk> you wrote: > > >> An MPC8309-derived board which does not utilize the SOCs watchdog but > >> has an external gpio-triggered (always-running) watchdog circuit. > > > > This is not even close to global coverage. > > No, of course not. Shall I list all __udelay() calls in the tree and > exclude the ones that are in board-specific code for non-ppc boards? I'm > pretty sure that leaves a very small handful. I can do that. No, I mean there is a pretty large numbe rof different processor families out there which were not covered by any of your tests. I think I still know a bit of the Power Architecture, but I would not dare to guarantee that your suggested change does not break any of these. Testing on a single processor / family is definitely not good enoug to give any confidence. > > Are you aware that there is the PPC-global implementation of > > wait_ticks() in arch/powerpc/lib/ticks.S , plus another MPC83xx > > specific one in drivers/timer/mpc83xx_timer.c? > > Yes, that latter one doesn't even compile in the presence of > CONFIG_WATCHDOG or CONFIG_HW_WATCHDOG. Maybe you focus on cleaning up this first? > It doesn't. I don't understand why powerpc's __udelay needs to be > different from all other architectures'. This is not really about the You misunderstand. The Power Architecture has been the first ever running U-Boot. In the beginning, two decades back, the project was not called U-Boot but PPCBoot - guess why. So you should rephrase your question into: why did all other architectures deviate from the reference implementation? > quirks of ppc SOCs' watchdogs or not, it's about providing a __udelay > primitive that generic code can rely on has certain properties, and in > particular, that watchdog drivers can use without risking being called > recursively. If any such recursive calls happen, then this is a bug in their implementation, isn't it? And it should be trivial to detect and to fix. > The point is, we're being told that everything is moving to DM and > better convert your board or else..., and nowadays CONFIG_WDT comes with > it's own watchdog_reset() which is a rather more complicated beast than > the board-specific ones that used to be sprinkled throughout (and > out-of) the tree. So yes, for the past 18 years, nothing bad has > probably come from doing a WATCHDOG_RESET even deep down in the guts of > arch-specific primitives. You certainly have a point here. But when you try to improve any code, the first thing you must do is to guarantee it continues to work on all affected systems. I don't dare to give even a prognosis before testing this on a number of different hardware configurations. Testing on a single platform (which apparently has aother problems, or you would not need any such change) is not convincing. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "A dirty mind is a joy forever." - Randy Kunkee
Re: [PATCH] powerpc: remove WATCHDOG_RESET call from wait_ticks()
Dear Rasmus, In message <56b724ef-8051-8a46-5df8-fc5b00bd8...@prevas.dk> you wrote: > > Yes, but I'm not talking about or using the SOC watchdog. Also, this is > not really about the particular watchdog device at all. It is about what > generic code can expect from the __udelay primitive, which is not > currently that on ppc. Maybe what you expect from __udelay() is not what other people expect(ed) from it? I don't think there is any formal description of that __udelay() is supposed to do in any existing U-Boot documentation... Please just face facts: for nearly two decades this code has been doing what it does, and I guess there were good reasons at that time (like memory footprint and resources to fix a specific problem). I agree that from a design point of view this is not nice, but that's not the point here. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de No, I'm not going to explain it. If you can't figure it out, you didn't want to know anyway... :-) - Larry Wall in <1991aug7.180856.2...@netlabs.com>
Re: [PATCH] powerpc: remove WATCHDOG_RESET call from wait_ticks()
Dear Rasmus, In message <86e1c45a-fc23-6794-7ca9-e96910af4...@prevas.dk> you wrote: > > > Testing on a single platform (which apparently has aother problems, > > or you would not need any such change) is not convincing. > > I don't, actually, need this change, but suggested it as a way towards > making the primitives available a little more consistent across > architectures since I stumbled on this implementation detail while > single-stepping my board to figure out why it would reset in the SPL. As > I'm unable to convince you that the benefits of that outweigh the risk > of introducing a regression, feel free to drop it. OK. > I do, however, need the other ppc patch I sent yesterday: > https://patchwork.ozlabs.org/patch/1255844/ . That one should satisfy > the requirement that it cannot possibly break anything for existing boards. This other patch lacks documentation - just from reading the code nobody will be able to understand what this piece of code is intended for, so there is a risk that it might be removed by some later "cleanup". Please add some comment in the code to explain the intentions. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Defaults are wonderful, just like fire. - Larry Wall in <1996mar6.004121.27...@netlabs.com>
Re: [PATCH 3/3] cmd: dm: Fixed/Added DM driver listing subcommands
Dear Sean,
In message you wrote:
>
> Can you print these dashes in a way which makes it obvious that they are
> the correct length? E.g. do something like
>
> puts("Driveruid uclass Devices\n");
> puts("--\n");
Or even less code:
puts("Driveruid uclass Devices\n"
"--\n");
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
A person with one watch knows what time it is; a person with two
watches is never sure. Proverb
Re: [PATCH 07/10] board: stm32mp1: add finished good in board identifier OTP
Dear Patrick, In message <20200212183744.5309-8-patrick.delau...@st.com> you wrote: > Update the command stboard to support the coding of OTP 59 with > finished good: Can you please explain what "finished good" means? I can't parse the sentence above, sorry. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Prof:So the American government went to IBM to come up with a data encryption standard and they came up with ... Student: EBCDIC!
Re: [RFC RFT PATCH] env: spl: filter the variables in default environment of SPL or TPL
Dear Patrick, In message <20200318143602.23253-1-patrick.delau...@st.com> you wrote: > Use a new option CONFIG_SPL_ENV_VARS to filter the variables included > in the default environment used in SPL (and TPL). > > That allows to reduce the size of default_environment[]. Sorry, but NAK. we had this discussion a couple of times before. It is mandatory that both SPL and U-Boot proper will use the _same_ environment, including the same default environment, or all kind of havoc may result. Just think of situations where Falcon Mode is being used and SPL and U-Boot proper would be using different settings. If your default environment is too big for the SPL, then make it smaller. If you need additional settings in U-Boot, there are many ways to load thise there. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "If that makes any sense to you, you have a big problem." -- C. Durance, Computer Science 234
Re: [PATCH v2] powerpc: allow opting out of WATCHDOG_RESET() from timer interrupt
Dear Rasmus Villemoes, In message <20200318151651.2510-1-rasmus.villem...@prevas.dk> you wrote: > When using CONFIG_(SPL_)WDT, the watchdog_reset function is a lot more > complicated than just poking a few SOC-specific registers - it > involves accessing all kinds of global data, and if the interrupt > happens at the wrong time (say, in the middle of an WATCHDOG_RESET() > call from ordinary code), that can end up corrupting said global data. > > Also, having WATCHDOG_RESET() called automatically from the timer > interrupt runs counter to the idea of a watchdog device - if the board > runs into an infinite loops with interrupts still enabled, the > watchdog will never fire. > > Allow the board to opt out of this behaviour by setting > CONFIG_SYS_WATCHDOG_FREQ to 0 - as that setting is currently > nonsensical, it cannot affect any existing boards. > > Signed-off-by: Rasmus Villemoes If such a change gets implemented, it should at least be made consistently, i. e. not only in arch/powerpc/lib/interrupts.c but also in the timer_interrupt() handler in drivers/timer/mpc83xx_timer.c and in dtimer_interrupt() in arch/m68k/lib/time.c and wherever else similar code may exist. Only changing one plase is not a good idea. And please add some documentation also to the README. Thanks. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Our OS who art in CPU, UNIX be thy name. Thy programs run, thy syscalls done, In kernel as it is in user!
Re: [PATCH 07/10] board: stm32mp1: add finished good in board identifier OTP
Dear Patrick, In message <07159b22a76a445089aa6cd646c0e...@sfhdag6node3.st.com> you wrote: > > > I can't parse the sentence above, sorry. > > It is a part of the codification used in production of ST board, sorry if it > is not clear. I see. Please add suich explanation to the commit message and maybe even comment in the code. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Everyone, in some small sacred sanctuary of the self, is nuts. - Leo Rosten
Re: [PATCH v3 0/3] Add command to display or save Linux PStore dumps
Dear Frédéric, In message <20200319175737.10166-1-frederic.da...@collabora.com> you wrote: > This serie of patches adds a new pstore command allowing to display or save > ramoops logs (oops, panic, console, ftrace and user) generated by a previous > kernel crash. > PStore parameters can be set in U-Boot configuration file, or at run-time > using "pstore set" command. For kernel using Device Tree, the parameters are > dynamically added to Device Tree. > Records size should be the same as the ones used by kernel, and should be a > power of 2. I wonder if we are reinventing the wheel here again? There is this feature in U-Boot which is called "shared log buffer"; a couple of years ago this was fully functional at least on Power and ARM architectures, but it was rarely used and probably has not been tested for years. A;so, the necessary tiny patch to have it supported in Linux as well never made it upstream (don't remember why, likely lack of time/interest). The functionality we had then was the following: - A memory area war reserved in U-Boot (typically at the upper end of memory) as a buffer that was shared between U-Boot and Linux. The format was as used for the kernel log buffer. - Upon boot, U-Boot would not re-initialize an existing log buffer, but keep it's content. That means, you could read and display the log buffer of the linux kernel that was running before the reset. After kernel crashes, pretty often this contained information that the kernel could not even print to the serial console any more. - In U-Boot, you could append log entries to that buffer. For example, this was used to record the results of the Power On Self Test (POST) routines (another feature that only few people still remember). - When booting Linux, the kernel syslog mechanism was used to extract the information from the log buffer in the usual way. The interesting fact here was that the Linux kernel was able to extract and save the kernel panic messages etc. from the crash before, plus any messages logged by U-Boot. To me this sounds very much like what you are adding here (plus a few features more). Does it make sense to unify such code? Added Heiko to Cc:, as he is currently working on fixes to get shared logbuffer working again for another project. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de I'm what passes for a Unix guru in my office. This is a frightening concept. - Lee Ann Goldstein, in <3k55ba$c...@butch.lmsc.lockheed.com>
Re: [PATCH v3 2/3] test: Add PStore command tests
Dear Frédéric, In message <20200319175737.10166-3-frederic.da...@collabora.com> you wrote: > > +++ b/test/py/tests/test_pstore.py > @@ -0,0 +1,73 @@ > +# SPDX-License-Identifier: GPL-2.0 Can we please make new code always GPL-2.0+ ? Thanks! Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de That's their goal, remember, a goal that's really contrary to that of the programmer or administrator. We just want to get our jobs done. $Bill just wants to become $$Bill. These aren't even marginallly congruent. -- Tom Christiansen in <6jhtqk$qls$1...@csnews.cs.colorado.edu>
Re: [PATCH] cmd: ubi: add a command to rename volume
Dear Philippe, In message <1584644739-10258-1-git-send-email-philippe.rey...@softathome.com> you wrote: > This commit add the command ubi rename to rename an ubi volume. > The format of the command is: ubi rename . Can we plase make this optional / configurable? Thanks! Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Every revolutionary idea - in science, politics, art, or whatever - evokes three stages of reaction in a hearer: 1. It is completely impossible - don't waste my time. 2. It is possible, but it is not worth doing. 3. I said it was a good idea all along.
Re: [PATCH v3 0/3] Add command to display or save Linux PStore dumps
Dear Heinrich, In message you wrote: > > > To me this sounds very much like what you are adding here (plus a > > few features more). Does it make sense to unify such code? > > It seems you are relating to > https://lore.kernel.org/lkml/844oyrqvvb@sauna.l.org/t/ No, I'm not. I was talking of my own code from many, many years ago. > ramoops in Linux is exactly doing what was suggested in 2009. You can > find the Documentation/admin-guide/ramoops.rst We had this in U-Boot long before that time. It was a key requirement when we added POST support in 2002. > git grep -GHrn 'shared log' finds nothing in U-Boot. So if any part of > the old implementation in U-Boot exists, could you, please, point us to > the coding. The shared log buffer support was added by commit 56f94be3ef63: commit 56f94be3ef63732384063e110277ed89701b6471 (tag: LABEL_2002_11_05_1735) Author: Wolfgang Denk Date: Tue Nov 5 16:35:14 2002 + * Add support for log buffer which can be passed to Linux kernel's syslog mechanism; used especially for POST results. * Patch by Klaus Heydeck, 31 Oct 2002: Add initial support for kup4k board The code was mainly in common/cmd_log.c, but this got heahily rewritten and is now in cmd/log.c ; apparently this got lost (like the original copyright, sic!) when Simon modified / rewrote this driver. For history, try: git log --follow -- common/cmd_log.c > If the original design never made it into Linux and there is an > established Linux interface since 2011, I would plead to eliminate any > remaining non-compliant coding from U-Boot should it exist. I understand what Linus has is one-way, only focussing on crash dump, i. e. it does not allow to pass information from U-Boot to Linux? Also, my understanding is that the changes needed in Linux are pretty small. Maybe Heiko can comment on that... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Ken Dodd’s dad’s dog’s dead.
Re: [PATCH v3 0/3] Add command to display or save Linux PStore dumps
Dear Frédéric, In message <03674974-2a2c-3804-ce02-4f3d38ff0...@collabora.com> you wrote: > > > I understand what Linus has is one-way, only focussing on crash > > dump, i. e. it does not allow to pass information from U-Boot to Linux? > > Yes, currently it is not possible to pass information from U-Boot to Linux. > But, PStore does not only store crash dump, but also console, ftrace and > user space logs. > > It may be possible to add a "bootloader" storage space to PStore. This does not make much sense to me. We should not try to push any possible functionality into U-Boot just because we can. U-Boot is a boot loader, and should only contain necessary functionality and things, that are impossible or difficult to solve in other ways. I would much rather see the opposite direction working (again): being able to pass information from U-Boot to Linux kernel and further into Linux user land in a standard way. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Panic, n.: The second time you can't do it the first time.
Re: [PATCH v2 1/2] spi: call WATCHDOG_RESET() in spi_nor_wait_till_ready_with_timeout()
Dear Vignesh,
In message <20200320101448.10714-1-rasmus.villem...@prevas.dk> Rasmus Villemoes
wrote:
> I have a board for which doing "sf erase 0x10 0x8"
> consistently causes the external watchdog circuit to reset the
> board. Make sure to pet the watchdog during slow operations such as
> erasing or writing large areas of a spi nor flash.
...
> drivers/mtd/spi/spi-nor-core.c | 2 ++
> drivers/mtd/spi/spi-nor-tiny.c | 2 ++
Rasmus' patch triggers a few interesting questions about the SPI NOR
code:
Is there any specific reason why spi-nor-core.c and spi-nor-tiny.c
contain large parts of absolutely identical code?
All the functions
spi_nor_read_write_reg()
spi_nor_read_reg()
spi_nor_write_reg()
spi_nor_read_data()
mtd_to_spi_nor()
spi_nor_convert_opcode()
spi_nor_ready()
spi_nor_wait_till_ready_with_timeout()
spi_nor_wait_till_ready()
macronix_quad_enable()
spansion_read_cr_quad_enable()
spi_nor_set_read_settings()
are absolutely identical;
functions
read_cr()
write_sr()
write_disable()
set_4byte()
spi_nor_read()
write_sr_cr()
are mostly identical, but I wonder if the differences (like
nor->write_reg() versus spi_nor_write_reg()) is indeed intended to
save memory footprint or lack an update to later code?
Function
spi_nor_convert_3to4_read()
spi_nor_set_4byte_opcodes()
looks like it has not been updated/synced for some time.
Function
spi_nor_sr_ready()
spi_nor_fsr_ready()
is lacking error handling in spi-nor-tiny.c; and the rror handling
code in spi-nor-core.c is also mostly duplicated a couple or times.
Function
spi_nor_read_id()
differs in "interesting" ways, i. e. we have
370 info = spi_nor_ids;
371 for (; info->sector_size != 0; info++) {
372 if (info->id_len) {
here, and
894 info = spi_nor_ids;
895 for (; info->name; info++) {
896 if (info->id_len) {
there, while all the rest is idential. Lack of synchronization?
Also the differences in
spi_nor_select_read()
make we wonder...
This extensive code duplication looks really painful and error prone
to me.
Do you have any intentions to clean this up any time soon?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Common sense and a sense of humor are the same thing, moving at
different speeds. A sense of humor is just common sense, dancing.
- Clive James
Re: [PATCH v3 1/2] timer: mpc83xx_timer: fix build with CONFIG_{HW_, }WATCHDOG
Dear Rasmus, In message <20200320105248.24518-1-rasmus.villem...@prevas.dk> you wrote: > The code, which is likely copied from arch/powerpc/lib/interrupts.c, > lacks a fallback definition of CONFIG_SYS_WATCHDOG_FREQ and refers to > a non-existing timestamp variable - obviously priv->timestamp is > meant. > > Signed-off-by: Rasmus Villemoes > --- > drivers/timer/mpc83xx_timer.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Could you _please_ get used to add some patch histroy below this line "---", too? I. e. some information so we can see easily what has changed between patch version and and 2, and between version 2 and 3? Thanks. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Boss, n.: According to the Oxford English Dictionary, in the Middle Ages the words "boss" and "botch" were largely synonymous, except that boss, in addition to meaning "a supervisor of workers" also meant "an ornamental stud."
Re: [PATCH v3 0/3] Add command to display or save Linux PStore dumps
Dear Frédéric, In message <9b0b5776-cb47-dfb2-ab77-36c89dc64...@collabora.com> you wrote: > > But, when debugging kernel crashes occurring either early in boot or > hard crashes later, it can be helpful to be able to retrieve logs using > the U-Boot session. Full agreement, and we had such a feature working and in mainline already 17+ years ago. It's sad that people rather reinvent the wheel instead of using existing functionality (or reviving it, if it has been broken over time). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Be wiser than other people if you can, but do not tell them so. -- Philip Earl of Chesterfield
Re: [PATCH] tools: fw_env: use erasesize from MEMGETINFO ioctl
Dear Rasmus, In message <20200320140414.19689-1-rasmus.villem...@prevas.dk> you wrote: > > Having different fw_env.config files for the different revisions is > highly impractical, and the correct information is already available > right at our fingertips. So use the erasesize returned by the > MEMGETINFO ioctl when the fourth column is absent or contains a 0. > > As I'm only testing this on a NOR flash, I'm only changing the logic > for that case, though I think it should be possible for the other > types as well. > > Signed-off-by: Rasmus Villemoes > --- > tools/env/fw_env.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c > index 381739d28d..87c3504315 100644 > --- a/tools/env/fw_env.c > +++ b/tools/env/fw_env.c > @@ -1647,6 +1647,8 @@ static int check_device_config(int dev) > goto err; > } > DEVTYPE(dev) = mtdinfo.type; > + if (DEVESIZE(dev) == 0 && mtdinfo.type == MTD_NORFLASH) > + DEVESIZE(dev) = mtdinfo.erasesize; > if (DEVESIZE(dev) == 0) > /* Assume the erase size is the same as the env-size */ > DEVESIZE(dev) = ENVSIZE(dev); What happens if you - say - have an environment size of 16 KiB and an erase block size of 4 KiB? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A memorandum is written not to inform the reader, but to protect the writer. -- Dean Acheson
Re: [U-Boot] Sharing a hardware lab
Dear Harald, In message you wrote: > > I added something similar to this in our DENX internal tbot configurations but > did not yet publish it anywhere (maybe I should add it to tbot_contrib?). > Just Yes, please do! Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Computers are not intelligent. They only think they are.
Re: Totally wrecked my ENV
Dear Udo, In message you wrote: > > while trying to set the mac-address of a device in u-boot I somehow > wrecked the whole thing. > > I think I did 'setenv eth0addr xx:xx:xx:xx:xx:xx' followed by 'saveenv' > After that, my device tells me "Unknown command 'run' - try 'help'" and > also "Unknown command 'env' - try 'help'" I can't imagine of a situation where a correctly running U-Boot would suffer in such a way from any misspelled environment settings. > I seemed to me that some wrong quoting in the above 'setenv' command > messed up all the strings. No, the cause of the problem must be elsewhere. > I'm lost now, how can I achienve a total reset of my ENV? Resetting your environment will not help. When U-Boot cannot find command names like run or printenv, it's internal data stuctures have been corrupted, and this will not go away if you clean up the environment. You must reflash the whole U-Boot image. If you can still boot into Linux, I would use this to write the new U-Boot image, as you cannot really trust your U-Boot any more. If not, and you can still load and flash an image in U-Boot, then try that. If this doesn't work either, then you probably have to use a JTAG debugger (if you have a JTAG connector on your board). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are two ways to write error-free programs. Only the third one works.
Re: [PATCH v2 1/2] spi: call WATCHDOG_RESET() in spi_nor_wait_till_ready_with_timeout()
These differences are tricial to handle using IS_ENABLED() for code
parts that are needed only when erase/write support is configured.
> > Function
> >
> > spi_nor_read_id()
> >
> > differs in "interesting" ways, i. e. we have
> >
> > 370 info = spi_nor_ids;
> > 371 for (; info->sector_size != 0; info++) {
> > 372 if (info->id_len) {
> >
> > here, and
>
> In case of tiny stack, we save space by not storing flash names in
> spi_nor_ids[] table (its a significant saving) and hence have to rely on
> another field to detect EOL.
You could still use the same method in both implementations, right?
> Duplication is to avoid feature creep leading to increase in code size.
> But I can factor out common code if there is a wider agreement.
Code duplication never evermakes sense to me. It is just a cause of
errors and mental pain.
I would really appreciate if youc ould clean this up.
Thanks!
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Veni, Vidi, VISA:
I came, I saw, I did a little shopping.
Re: [PATCH 0/5] CMD_SAVEENV ifdef cleanup
Dear Rasmus Villemoes, In message <9c03710e-5eec-da6e-6c15-2f8a14cfc...@prevas.dk> you wrote: > > Can I ask whether you request changes to this patch series or if my > answers to your various comments have been satisfactory? I think you did no really answer to some of my concerns. In Message <20200219132715.1f81a240...@gemini.denx.de> I asked: | Have you tested that this works? How do the sizes of the | images differe before and after applying your changes? You replied: ... Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my patches we get | $ size u-boot spl/u-boot-spl |textdata bss dec hex filename | 407173 45308 98352 550833 867b1 u-boot | 582983360 65860 127518 1f21e spl/u-boot-spl | | but without, | | $ size u-boot spl/u-boot-spl |textdata bss dec hex filename | 407173 45308 98352 550833 867b1 u-boot | 526593360 280 56299dbeb spl/u-boot-spl We can observe that - the text size of the SPL grows from 52659 to 58298, i. e. by about 5.5 kB or more than 10% - the BSS size explodes from 280 to 65860 bytes, i. e. it grows from a few hndet bytes to more than 64 kB I can see where the increase in text size is coming from - your removal of #ifdef's now unconditionally includes some code that was omitted before, for example functions env_fat_save(), env_ext4_save(), env_sf_save(), plus a few variables. It is not obvious to me but scary to see such an explosion of BSS size. It's difficult to comment here as it is not clear to me which exact configuration you reported about, and it's also not clear if this is a typical result, of if it's the only configuration you ever tested. Your patch description sounds as if it was just a #ifdef cleanup without actual impact on the generated code, but the SPL size differences above make it clear that it is not - or that your testing has issues. You also failed to comment on impact on other environment storage configurations (NOR flash, NAND flash, UBI volume, ...). If it's only #ifdef changes without impact on function, then we should get exactly the same images. You did not comment if you have verified that. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de All easy problems have already been solved.
Re: [PATCH 0/5] CMD_SAVEENV ifdef cleanup
n with your patches, and this must be fixed. > .bss.tmpbuf_cluster > 0x0x1 fs/built-in.o > > that gets discarded without my patches (but with the config options > chosen so one would _expect_ to have save support in SPL). So yes, of > course there's a price to pay for enabling environment save support in > the SPL, with some backends being more expensive (in terms of footprint) > than others. A price of more than 64 kB additional memory footprint in the SPL is a strict no-go. This must be fixed, and I'm surprised that you did not even spend a thought about this after I explicitly mentioned it. This all makes no sense to me, as tmpbuf_cluster[] comes from fs/fat/fat_write.c, which should not be used for the SPL when you don't enable both FAT and SAVEENV support together. > > and it's also not clear if this is > > a typical result, of if it's the only configuration you ever > > tested. You continue to ignore this question. > > Your patch description sounds as if it was just a #ifdef cleanup > > without actual impact on the generated code, but the SPL size > > differences above make it clear that it is not - or that your > > testing has issues. > > There is _no_ change in code size, u-boot or spl, when > CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where > CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely > intended, namely actually allowing one to save the environment. This makes no sense either. If you compare the SAME configuration with and without your patches above, then we have this unacceptable BSS explosing, which is unacceptable. If you compare two different configurations above, one with CONFIG_SPL_SAVEENV=n and one with CONFIG_SPL_SAVEENV=y, then the whole comparion makes no sense. As long as we stick with the same single board (wandboard_defconfig), plus the 4 lines changed, there would be 4 different cases to test: - CONFIG_SPL_SAVEENV=n without your patches - CONFIG_SPL_SAVEENV=n withyour patches - CONFIG_SPL_SAVEENV=y without your patches - CONFIG_SPL_SAVEENV=y withyour patches Anything else is comparing apples and bicycles. > > You also failed to comment on impact on other environment storage > > configurations (NOR flash, NAND flash, UBI volume, ...). > > I don't touch those files at all, so they are not affected. Some still > fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets > CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually > supported). You _say_ they are not affected, and I accept that this is your intention. But my question was if you actually _tested_ that your patches behave as intented? I think there have been cases before where code changes had ... let's say unexpected side effects... You should build a few (if not all!) such boards with and without your patches applied and _verify_ the the code does not change. Just guessing is not good enough. > To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change > anything, except get rid of a lot of pointless ifdefs. For > CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's > intention of actually being able to save the environment from SPL, at > least for fat, ext4 and sf. You continue to fail to prove that. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "We don't care. We don't have to. We're the Phone Company."
Re: [PATCH v2 1/2] spi: call WATCHDOG_RESET() in spi_nor_wait_till_ready_with_timeout()
Dear Vignesh, In message <9a1e75ac-135a-26aa-2ded-784fbe14b...@ti.com> you wrote: > > I fully understand your concerns and will work on unifying the two > stacks with IS_ENABLED() macro so that there will still be a tiny stack > with same memory footprint. Thanks!! > But I want to state that the differences currently in spi-nor-tiny.c vs > spi-nor-core.c are intentional. I don't think there have been any fixes > in the main code missing from tiny stack. > Features such as Octal mode and other stuff have been intentionally kept > out of spi-nor-tiny to avoid code size increase. Understood. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de As long as we're going to reinvent the wheel again, we might as well try making it round this time.- Mike Dennison
Re: [PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH
Dear Rasmus,
In message <20200326230200.12617-1-rasmus.villem...@prevas.dk> you wrote:
> Currently, CONFIG_SPL_SAVEENV is not very well supported by the
> various storage backends, as many of them contain variants of some
> logic that end up not compiling the .save method when
> CONFIG_SPL_BUILD.
...
> As far as I can tell, the only in-tree defconfig that sets both
> SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV is display5_defconfig, which
> also happens to be the only one setting SPL_SAVEENV at all. Let's see
> how these patches affect that:
>
> # avoid differences due to different git commit or wallclock time
> $ export SOURCE_DATE_EPOCH=1585252702
> $ echo 'test' > .scmversion
> $ export ARCH=arm
> $ export CROSS_COMPILE=arm-linux-gnueabi-
> $ git checkout master ; make display5_defconfig ; make -j8
> $ cp u-boot u-boot.1 ; cp spl/u-boot-spl u-boot-spl.1
> $ git checkout sf-spl-saveenv ; make display5_defconfig ; make -j8
> $ cp u-boot u-boot.2 ; cp spl/u-boot-spl u-boot-spl.2
> $ size u-boot{,-spl}.{1,2}
>textdata bss dec hex filename
> 377468 24620 66164 468252 7251c u-boot.1
> 377468 24620 66164 468252 7251c u-boot.2
> 584112020 116 60547ec83 u-boot-spl.1
> 599762028 116 62120f2a8 u-boot-spl.2
Thanks for the additional testing. As you can see here, it is
definitely worth the effort.
> So U-Boot proper is not affected (the files even yield identical
> objdump -d output), while the SPL grows by the ~1.5K necessary to
> implement saving the environment. Borrowing the bloat-o-meter script
> from linux, we can also see the functions/data items that are now
> included:
Does this not trigger questions to you? Why is the code growing?
It had SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV before!
> ../linux/scripts/bloat-o-meter u-boot-spl.1 u-boot-spl.2
> add/remove: 11/0 grow/shrink: 0/1 up/down: 1340/-24 (1316)
> Function old new delta
> hexport_r - 408+408
> env_sf_save- 332+332
> qsort - 144+144
> match_entry- 124+124
> env_export - 100+100
> match_string - 92 +92
> strstr - 64 +64
> setup_flash_device - 56 +56
> cmpkey - 12 +12
> env_offset - 4 +4
> env_new_offset - 4 +4
> env_sf_load 184 160 -24
> Total: Before=52986, After=54302, chg +2.48%
To me this triggers at least two questions:
- Why is this code included now, when it was not before?
- Iff the code was not included before, why did this not cause
problems when trying to save the environment in SPL, which was
apparently needed by this board?
Adding Lukasz on Cc:, who maintains this board.
After some initial talk to Lukasz it seems your testing indeed
discovered a bug - without your patch SPL_SAVEENV apparently had no
effect, and oard testing did not vdetect this failure, because
requirements changed during the project the the feature that was
once requested got later dropped, but the option was not removed.
Testing is _always_ worth the effort.
> Now, to check that other storage backends are not affected, and also
> that nothing (neither U-Boot or SPL) changes for ENV_IS_IN_SPI_FLASH
> when CONFIG_SPL_SAVEENV=n, I have repeated the above with
> am335x_shc_netboot_defconfig (MMC), pengwyn_defconfig (NAND),
> mccmon6_sd_defconfig (FLASH),
> ls1046ardb_qspi_spl_defconfig (SPI_FLASH):
>
> $ for c in am335x_shc_netboot_defconfig pengwyn_defconfig
> mccmon6_sd_defconfig ls1046ardb_qspi_spl_defconfig ; do
...
Actually this would have been easier using tbot, and it would have
been possible to cover many more / all boards, but I don't intend to
ask more from you. Thanks, both for the additional testing and your
patience.
Reviewed-by: Wolfgang Denk
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
I will also, for an appropriate fee, certify that your keyboard is
object-oriented, and that the bits on your hard disk are template-
compatible.- Jeffrey S. Haemer in <411akr$3...@cygnus.com>
Re: [PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH
Dear Rasmus, In message <893503e2-10a1-2d3e-e7ad-9d24163ad...@prevas.dk> you wrote: > ... > > Actually this would have been easier using tbot, > > Can you provide a pointer? Sounds like something I could use going forward. See [1] And/or see the thread "Sharing a hardware lab" [2] [1] https://tbot.tools/ [2] https://lists.denx.de/pipermail/u-boot/2020-February/399278.html Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There is a theory which states that if ever anyone discovers exactly what the Universe is for and why it is here, it will instantly dis- appear and be replaced by something even more bizarre and inexpli- cable. There is another theory which states that this has already happened.-- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"
Re: [PATCH 03/16] arm: stm32mp: reset to default environment when serial# change
Dear Patrick,
In message
<20200331180330.3.I8f6df6d28ce5b4b601ced711af3699d95e1576fb@changeid> you wrote:
> Serial number is first checked and, in case of mismatch, all
> environment variables are reset to their default value.
>
> This patch allows to detect that environment is saved in a removable
> device, as a SD card, and reused on a other board, potentially with
> incompatible variables.
>
> Signed-off-by: Patrick Delaunay
> ---
>
> arch/arm/mach-stm32mp/cpu.c | 20 +++-
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index 9aa5794334..365c2aa4f7 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -511,8 +511,9 @@ __weak int setup_mac_address(void)
> return -EINVAL;
> }
> pr_debug("OTP MAC address = %pM\n", enetaddr);
> - ret = !eth_env_set_enetaddr("ethaddr", enetaddr);
> - if (!ret)
> +
> + ret = eth_env_set_enetaddr("ethaddr", enetaddr);
> + if (ret)
> pr_err("Failed to set mac address %pM from OTP: %d\n",
> enetaddr, ret);
This is an unrelated and totally undocumented change. Please split
into separate commit.
> +
> + if (serial_env) {
> + if (!strcmp(serial_string, serial_env))
> + return 0;
> + /* For invalid enviromnent (serial# change), reset to default */
> + env_set_default("serial number mismatch", 0);
> + }
Resetting the enviropnment to the defaults means you drop all
setting sa user may have made. This is a very bad move - as a user
I find such things completely unacceptable. If I make any changes,
they must never ever be killed without my explicit confirmation.
I strongly advice against such a method. Please drop that.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"You ain't experienced..." "Well, nor are you." "That's true. But the
point is ... the point is ... the point is we've been not experienced
for a lot longer than you. We've got a lot of experience of not
having any experience." - Terry Pratchett, _Witches Abroad_
Re: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Dear Patrick Delaunay, In message <20200331160456.26254-1-patrick.delau...@st.com> you wrote: > Imply CONFIG_VERSION_VARIABLE for stm32mp1 target > and test U-Boot version ($env_ver) when the environment was > saved for the last time and to display warning trace. What is env_ver? Are you by chance reinventing the wheel? The U-Boot version is stored in the environment variable "ver"; there should be no need for something similar. Also. where is $env_ver coming from? It does not exist in mainline, nor in any of the 3 patches that preceed this patch # 4/16 ? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Computers are not intelligent. They only think they are.
Re: [PATCH 05/16] arm: stm32mp: spl: add bsec driver in SPL
Dear Patrick Delaunay,
In message
<20200331180330.5.I7a042a9ffbb5c2668034eddf5ace91271bb53c5f@changeid> you wrote:
> Add the bsec driver in SPL, as it is needed by SOC part number detection.
>
> Signed-off-by: Patrick Delaunay
> ---
>
> arch/arm/dts/stm32mp15-u-boot.dtsi | 2 +-
> arch/arm/mach-stm32mp/Makefile | 2 +-
> arch/arm/mach-stm32mp/bsec.c | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/dts/stm32mp15-u-boot.dtsi
> b/arch/arm/dts/stm32mp15-u-boot.dtsi
> index 8f9535a4db..e0b1223de8 100644
> --- a/arch/arm/dts/stm32mp15-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp15-u-boot.dtsi
> @@ -40,7 +40,7 @@
> };
>
> &bsec {
> - u-boot,dm-pre-proper;
> + u-boot,dm-pre-reloc;
> };
This seems to be an unrelated change? You should at least explain
why this is needed, and if it's unrelated, spilt into a separate
commit.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Good manners are the settled medium of social, as specie is of
commercial, life; returns are equally expected for both.
- Lord Chesterfield _Letters to his Son_, 25 December 1753
Re: [PATCH 06/16] arm: stm32mp: spl: display error in board_init_f
Dear Patrick Delaunay,
In message
<20200331180330.6.I41a641a07fd12da45b392920fc3407e608926396@changeid> you wrote:
> Update board_init_f and try to display error message
> when console is available.
>
> This patch adds trace to debug a spl boot issue when DEBUG
> and DEBUG_UART is not activated, after uart probe.
>
> Signed-off-by: Patrick Delaunay
> ---
>
> arch/arm/mach-stm32mp/spl.c | 33 -
> 1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
> index ca4231cd0d..dfdb5bb7e9 100644
> --- a/arch/arm/mach-stm32mp/spl.c
> +++ b/arch/arm/mach-stm32mp/spl.c
> @@ -79,37 +79,36 @@ void spl_display_print(void)
> void board_init_f(ulong dummy)
> {
> struct udevice *dev;
> - int ret;
> + int ret, clk, reset, pinctrl;
>
> arch_cpu_init();
>
> ret = spl_early_init();
> if (ret) {
> - debug("spl_early_init() failed: %d\n", ret);
> + debug("%s: spl_early_init() failed: %d\n", __func__, ret);
> hang();
> }
>
> - ret = uclass_get_device(UCLASS_CLK, 0, &dev);
> - if (ret) {
> - debug("Clock init failed: %d\n", ret);
> - return;
> - }
> + clk = uclass_get_device(UCLASS_CLK, 0, &dev);
> + if (clk)
> + debug("%s: Clock init failed: %d\n", __func__, clk);
>
> - ret = uclass_get_device(UCLASS_RESET, 0, &dev);
> - if (ret) {
> - debug("Reset init failed: %d\n", ret);
> - return;
> - }
> + reset = uclass_get_device(UCLASS_RESET, 0, &dev);
> + if (reset)
> + debug("%s: Reset init failed: %d\n", __func__, reset);
>
> - ret = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> - if (ret) {
> - debug("%s: Cannot find pinctrl device\n", __func__);
> - return;
> - }
> + pinctrl = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> + if (pinctrl)
> + debug("%s: Cannot find pinctrl device: %d\n",
> + __func__, pinctrl);
>
> /* enable console uart printing */
> preloader_console_init();
>
> + if (clk || reset || pinctrl)
> + printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
> +__func__, clk, reset, pinctrl);
> +
This change makes little sense to me/ If you want error messages,
then just turn above debug() into printf(), and be done with it.
As an additional benefit so see at once which step failed.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
A Chairman was as necessary to a Board planet as the zero was in
mathematics, but being a zero had big disadvantages...
- Terry Pratchett, _The Dark Side of the Sun_
Re: [PATCH 08/16] board: stm32mp1: update management of boot-led
Dear Patrick Delaunay,
In message
<20200331180330.8.I15cb0a6245fb4cd5d23371683c2697f794adf306@changeid> you wrote:
> Force boot-led ON and no more rely on default-state.
> This patch avoid device-tree modification for U-Boot.
...
> +#ifdef CONFIG_LED
> struct udevice *dev;
> int ret;
>
> @@ -292,8 +294,10 @@ static int setup_led(enum led_state_t cmd)
>
> ret = led_set_state(dev, cmd);
> return ret;
> -}
> +#else
> + return 0;
> #endif
> +}
>
> static void __maybe_unused led_error_blink(u32 nb_blink)
> {
> @@ -648,8 +652,10 @@ int board_init(void)
>
> sysconf_init();
>
> - if (CONFIG_IS_ENABLED(CONFIG_LED))
> + if (CONFIG_IS_ENABLED(CONFIG_LED)) {
> led_default_state();
> + setup_led(LEDST_ON);
> + }
This is inconsistent, please use CONFIG_IS_ENABLED() everywhere
instead of #ifdef's.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Q: Do you know what the death rate around here is?
A: One per person.
Re: [PATCH 09/16] board: stm32mp1: gt9147 IRQ before reset on EV1
Dear Patrick Delaunay,
In message
<20200331180330.9.I5d296f8fd82b60a592b51029e7e420672d0e855b@changeid> you wrote:
> Software workaround for I2C issue on EV1 board,
> configure the IRQ line for touchscreen before LCD reset
> to fix the used I2C address.
...
> + ret = uclass_get_device_by_driver(UCLASS_NOP, DM_GET_DRIVER(goodix),
> + &dev);
> + if (ret)
> + debug("goodix init failed: %d\n", ret);
If this is an error condition, you should use printf(), and not
paper over it with a debug().
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Don't have a battle of wits with an unarmed opponent.
Re: [PATCH 11/16] board: stm32mp1: check env_get result in board_late_init
Dear Patrick Delaunay,
In message
<20200331180330.11.Ic051e25812481db408f2431c7962da1db1f198fb@changeid> you
wrote:
> This patch avoids crash in strcmp when the boot_device is not
> present in environment (this case should be never occur)
>
> Signed-off-by: Patrick Delaunay
> ---
>
> board/st/stm32mp1/stm32mp1.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> index 89a088cd28..fff4cef2c2 100644
> --- a/board/st/stm32mp1/stm32mp1.c
> +++ b/board/st/stm32mp1/stm32mp1.c
> @@ -753,7 +753,8 @@ int board_late_init(void)
>
> /* Check the boot-source to disable bootdelay */
> boot_device = env_get("boot_device");
> - if (!strcmp(boot_device, "serial") || !strcmp(boot_device, "usb"))
> + if (boot_device &&
> + (!strcmp(boot_device, "serial") || !strcmp(boot_device, "usb")))
> env_set("bootdelay", "0");
I think this is generally a bad idea. You should have more respect
to the intentions of your users. If a user defines a specific
bootdelay setting in his environment, this must be respected.
I really hate vendors who believe they know better than me what I
want or what is good to me. Please get rid of such stuff.
"bootdelay" is an environment variable that is intentionally user
definable. You must never mess with those.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"Everything should be made as simple as possible, but not simpler."
- Albert Einstein
Re: [PATCH 02/18] common: Drop flash.h from common header
Dear Simon, In message <20200406203250.2.Ia0f8528ae0e9e3ead7d99891b46cccaef5859295@changeid> you wrote: > Move this uncommon header out of the common header. > > Fix up some style problems in flash.h while we are here. Actually your coding style fixes are anywhere else as well. I think this should be in a separate commit. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A rolling stone gathers momentum.
Re: [PATCH 03/16] arm: stm32mp: reset to default environment when serial# change
Dear Patrick, In message <60273317e5704581bef81c4beb28a...@sfhdag6node3.st.com> you wrote: > > > I strongly advice against such a method. Please drop that. > > Understood, I drops this patch Thanks. > I introduce this behavior after a internal request and to avoid issues during > tests: > > Some users use the same SD card (same rootfs) on several boards (EV1 and DK2 > for example) > and the U-Boot environment is saved on this SD card. OK. > When an user updates U-Boot binary to use this SD card on other board but not > erase the > environment file (save in ext4 file in bootfs partition), the boot can fail > because the > environment is not compatible between the 2 boards. Well, why would that fail on another board but not on the one that is currently in use? Where is the U-Boot image you are booting from? Not on the SDCard, too? Well, then this is a design problem (or you may even call it a design bug). It has always been a bad idea to use a fixed structure binary format if there are chances that provider (the env storage) and consumer (U-Boot) may get out of sync. The binary blob environment format (checksum, eventually redundancy flag, date with a fixed total size) is inherently only compatible with other U-Boot versions as long as redundancy and size settings are kept the same. If you cannot garantee this, you should use a different storage format - for example as plain text file. Of course you pay for added compatibility across U-Boot configurations with the price of reduced security (lack of checksum). But then, normally you do not change redundancy or environment size between U-Boot versions, so all this should be a theroretical problem only? > This patch try to avoid this issue when I detect that the removable device > (as SD card) is used on a > a different board (it is detected when saved serial number with different the > OTP). You see this only as a risk - try to see it as a chance, too. Imagine a user is trying to copy environment between systems. Why would you want to stop him? Any incompatibility issues can reliably be avoided when using binary blob format, as then you will get a checksum error, and the incorrect copy is ignored. > I make too many assumption on user usage and this patch can't be acceptable > in arch > (common for all board based on STM32MP15x). See signature below :-) Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "UNIX was not designed to stop you from doing stupid things, because that would also stop you from doing clever things." - Doug Gwyn
Re: [PATCH 04/16] arm: stm32mp: detect U-Boot version used to save environment
Dear Patrick, In message <8607d1778bcd4035807908e4a3a90...@sfhdag6node3.st.com> you wrote: > > To simplify the test: > > env_check = " if env info -p -d -q; then env save; fi;" All such automatical "env save" actions somewhere in the code give me the creeps. I've seen too often that they did things I nver intended to do or would have accepted if I had a chance to decide. Use extremely careful, please. >From a user point of view, it's me who owns the environment, and nobody should mess with my data without me confirming it. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "Well I don't see why I have to make one man miserable when I can make so many men happy." - Ellyn Mustard, about marriage
Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal
Dear Roland,
In message <20210622135042.133904-2-roland.gaudig-...@weidmueller.com> you
wrote:
>
> This patch extends the setexpr command with a dec operator to
> convert an input value to decimal.
...
> + /* hexadecimal to decimal conversion: "setexpr name dec value" */
> + if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
> + w = cmd_get_data_size(argv[3], 4);
> + a = get_arg(argv[3], w);
> + return env_set_ulong(argv[1], a);
> + }
Should there not be a test for 4 arguments and the third _not_ being
"dec" ? Like "setexpr foo hex 42" ?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
A wise man asks himself the reason for his mistakes, while a fool
will ask others.
Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal
Dear Simon,
In message
you wrote:
>
> > > 0m123 ? ('m' for deciMal).
> >
> > Perhaps 0d123? Though I would prefer to remove many of the implicit
> > assumptions of hex input.
>
> Right, we can't use 'd' because it is valid hex.
>
> I believe hex is the right default. We just need an easy way to use decimal.
Maybe we should make this more general and support an even wirder
range of formats? Instead of just converting to decimal, we could
pass a format string for sprintf() ?
Like:
# setexpr foo fmt %d $value
?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Question: How does one get fresh air into a Russian church?
Answer: One clicks on an icon, and a window opens!
Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal
Dear Roland,
In message you wrote:
>
> > ...
> >> + /* hexadecimal to decimal conversion: "setexpr name dec value" */
> >> + if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
> >> + w = cmd_get_data_size(argv[3], 4);
> >> + a = get_arg(argv[3], w);
> >> + return env_set_ulong(argv[1], a);
> >> + }
> >
> > Should there not be a test for 4 arguments and the third _not_ being
> > "dec" ? Like "setexpr foo hex 42" ?
>
> Yes it's possible to add further conversion operations. But I didn't saw a
> need for hex in the first place, as hex is currently the default within
You misunderstand. With your code, the incorect command "setexpr foo hex 42"
would not raise any error message.
Assume such a call:
setexpr foo kjkjkjlkj 42
This should raise an error, right?
Instead of
if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
...
}
you would need something like:
if (argc == 4) {
if strcmp((argv[2], "dec") != 0) {
print error message
bail our
}
...
}
But see my other suggestion anyway - why add just decimal format
when using sprintf() with a format string allows for all kinds of
fancy uses?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"Love is an ideal thing, marriage a real thing; a confusion of the
real with the ideal never goes unpunished." - Goethe
Re: [RFC PATCH 1/1] cmd: nvedit: Forbid key to be empty.
Dear Francis,
In message <20210628193145.796999-2-francis.lan...@amarulasolutions.com> you
wrote:
> Before this patch, it was possible to do the following using setenv:
> setenv '' foo
> Then, on next reboot, U-Boot will not be able to parse environment due to it
> having:
> =foo
>
> Now, if the above command is given, an error message is thrown and environment
> is not modified.
>
> Signed-off-by: Francis Laniel
> ---
> cmd/nvedit.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index d14ba10cef..64b7aef78d 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -262,6 +262,11 @@ static int _do_env_set(int flag, int argc, char *const
> argv[], int env_flag)
> return 1;
> }
>
> + if (!strlen(name)) {
Instead of calling a library function here you could use the same
shortcut as in the patch you referenced:
if (*name == 0) {
> + printf("## Error: variable name cannot be empty\n");
s/cannot/must not/ ??
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"The fundamental principle of science, the definition almost, is
this: the sole test of the validity of any idea is experiment."
- Richard P. Feynman
Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation
Dear Roland, In message <20210628151750.572837-1-roland.gaudig-...@weidmueller.com> you wrote: > > > U-Boot uses almost everywhere hexadecimal numbers. But some bootargs > passed to Linux are expecting decimal numbers. As long as the values > are in the range 0 to 9 it is sufficient to just strip 0x from the > number. But for greater values a method for converting numbers to > decimal is needed. > > This patch adds C like format string capabilities to the setexpr > command. Here are some examples: Thanks! > In contrast to the original C format strings the number of parameters > is limited to one. As the get_arg() function returns unsigned long > type, the format string commands are limited to those which are > operating on unsigned long type. These are two pretty unfortunate restrictions. I guess it should not be too hard to avoid both of these. Can you please give it a try? I think it is reasonable to assume (and specify) that, when the "fmt" option is used, _all_ following arguments will be passed (unchanged) to the sprintf() function. This was actually one of my intentions when making this suggestion - to be able to construct any kind of data from pieces; say, for example: => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Never ascribe to malice that which can adequately be explained by stupidity.
Re: [PATCH 1/3] cmd: setexpr: add fmt format string operation
Dear Roland,
In message <20210628151750.572837-2-roland.gaudig-...@weidmueller.com> you
wrote:
>
> + if (*format == '%') {
> + switch (*(format + 1)) {
> + case '%':
> + /* found '%%', not a format specifier, skip. */
> + format++;
> + break;
> + case '\0':
> + /* found '%' at end of string,
> + * incomplete format specifier.
> + */
> + return NULL;
> + default:
> + /* looks like a format specifier */
> + return format;
> + }
> + }
Why do you do that here? *printf() should be clever enough to parst
the format string.
> +static int setexpr_fmt(char *name, char *format, char *value)
> +{
> + struct expr_arg aval;
> + int data_size;
> + char str[MAX_STR_LEN];
> + int fmt_len = strlen(format);
> +
> + if (fmt_len < 2) {
> + printf("Error: missing format string");
> + return CMD_RET_FAILURE;
> + }
This is an arbitrary restriction that just limits the potential use.
Please don't do this. Maybe I want to use:
=> setexpr foo fmt X
This should not cause problems.
> + /* Exactly one format specifier is required */
> + if (!first || setexpr_fmt_spec_search(first + 1)) {
> + printf("Error: exactly one format specifier is required\n");
> + return CMD_RET_FAILURE;
> + }
Please get rid of this restriction.
> + /* Search type field of format specifier */
> + while (*first && !isalpha(*first))
> + first++;
> +
> + /* Test if type is supported */
> + if (!strchr("diouxX", *first)) {
> + printf("Error: format type not supported\n");
> + return CMD_RET_FAILURE;
> + }
Get rid of all these, please!
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Man did not weave the web of life; he is merely a strand in it.
Whatever he does to the web, he does to himself. - Seattle [1854]
Re: [PATCH 1/3] cmd: setexpr: add fmt format string operation
Dear Rasmus, In message <4c69688f-c187-ad14-5cfe-cdcddc71f...@prevas.dk> you wrote: > > I think this allows a lone % to appear at the end of the string after > the actual %d, i.e. it seems "%d bla %" would be accepted. there are a lot of highly unintuitive restrictions in that code, indeed. > I think allowing "arbitrary" format strings, restricted to those > expected exactly one integer argument, is too fragile and error-prone. Indeed. See for example the "printf" (bash) shell builtin - we should have a similar flexibility here. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Humanity has the stars in its future, and that future is too important to be lost under the burden of juvenile folly and ignorant superstition. - Isaac Asimov
Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation
Dear Roland, In message you wrote: > > > These are two pretty unfortunate restrictions. I guess it should > > not be too hard to avoid both of these. Can you please give it a > > try? > > I think it is possible to allow more than one format parameter or more > types. But it would make checking much more difficult. Maybe we need _less_ checking, not more - and maybe the needed checking is already done in the *printf() code? > I think just passing the format string directly to sprintf should be > avoided because it is unsafe. For example > > => setexpr foo fmt %s 0x > > would surely lead to access on memory location outside the variable > where 0x is stored. Only if you make the wrong assumptions. I would expect this to result in foo=0x in the same way as the bash builting gives $ printf '%s\n' 0x 0x > > => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d > > I think the only way to support such expressions in a save way would > be implementing an own format string parser for setexpr with Maybe it makes sense to have a look at the bash code? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de How many seconds are there in a year? If I tell you there are 3.155 x 10^7, you won't even try to remember it. On the other hand, who could forget that, to within half a percent, pi seconds is a nanocentury. -- Tom Duff, Bell Labs
Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation
Dear Sean, In message <19b6eeea-2aad-972b-aeeb-8959aab17...@gmail.com> you wrote: > > The issue with this is twofold. First, there is no portable way to > construct a va_list from C code. So the likely way to do this would be > to set an arbitrary limit, and then just pass the arguments in. E.g. > something like We already have an argument list: it's what's being passed to the "setexpr" command, minus the initial arguments. > snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc > */); Why this test on argc? If it's less than 4, argv[4] should be NULL anyway. > but of course there is no way to check that the format string matches > the correct number of arguments. This is a pretty big footgun. You have this problem always when you have user provided format strings and arguments. We don't have to re-invent the wheel here. I repeat myself: maybe we should have a look at bash's implementation of the printf builtin command? there I get for example this: $ printf "%d %d %d\n" 3 3 0 0 $ printf "%d %d %d\n" foo bar -bash: printf: foo: invalid number -bash: printf: bar: invalid number 0 0 0 > The other problem is that things like `%d` expect a number and not a > string. So you would have to reimplement snprintf anyway so that it > expects all of its arguments to be strings, and calls strtoul as > appropriate. And considering that the *printf functions take 5k > already, this reimplementation may add a significant amount of code. > For this reason, I'd much prefer to just have `hex` and `dec` functions > which do the appropriate conversions. Eventually the format checking can be kept out of the generic *printf() code; it could then be optional/configurable with the "fmt" option in the setexpr command. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Every program has at least one bug and can be shortened by at least one instruction -- from which, by induction, one can deduce that every program can be reduced to one instruction which doesn't work.
Re: [RFC PATCH v2 1/1] cmd: nvedit: Forbid key to be empty.
Dear Francis Laniel,
In message <20210629161859.298630-2-francis.lan...@amarulasolutions.com> you
wrote:
> Before this patch, it was possible to do the following using setenv:
> setenv '' foo
> Then, on next reboot, U-Boot will not be able to parse environment due to it
> having:
> =foo
>
> Now, if the above command is given, an error message is thrown and environment
> is not modified.
>
> Signed-off-by: Francis Laniel
> ---
> cmd/nvedit.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index d14ba10cef..6f99a85a9c 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -262,6 +262,11 @@ static int _do_env_set(int flag, int argc, char *const
> argv[], int env_flag)
> return 1;
> }
>
> + if (*name == '\0') {
> + printf("## Error: variable name must no be empty\n");
> + return 1;
> + }
> +
> env_id++;
>
> /* Delete only ? */
Reviewed-by: Wolfgang Denk
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"I can call spirits from the vasty deep."
"Why so can I, or so can any man; but will they come when you do call
for them?" - Shakespeare, 1 King Henry IV, Act III, Scene I.
Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation
Dear Sean, In message you wrote: > > >>snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc > >> */); > > > > Why this test on argc? If it's less than 4, argv[4] should be NULL > > anyway. > > snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, >argc >= 5 ? argv[5] : NULL, argc >= 6 ? argv[6] : NULL, /* etc > */); > > and you keep doing this until you get to whatever number of arguments > you'd like. I'm sorry, but this is not an acceptable way to implement variadic functions in C. Are you aware how va_arg works? > > Eventually the format checking can be kept out of the generic > > *printf() code; it could then be optional/configurable with the > > "fmt" option in the setexpr command. > > It's not a "checking" problem. The issue is that "123" cannot > be passed directly to %d. So you have dig into the guts of snprintf > anyway. Did you read my recommendation to have a look for example at the implementation of the printf bash builting? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The moral of the story is: "Don't stop to tighten your shoe laces during the Olympics 100m finals". - Kevin Jones in
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Sean, In message <20210701061611.957918-3-sean...@gmail.com> you wrote: > This is the LIL programming language [1] as originally written by Kostas > Michalopoulos . LIL is a stripped-down TCL > variant. Many syntax features are very similar to shell: Do you have a list of the exact differencec between LIL and a standard shell? I wonder, if we deviate from standard shell syntax anyway, we could also have a look at lua, for example? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de One essential to success is that you desire be an all-obsessing one, your thoughts and aims be co-ordinated, and your energy be concentra- ted and applied without letup.
Re: [RFC PATCH 00/28] cli: Add a new shell
Dear Tom, In message <20210701202155.GQ9516@bill-the-cat> you wrote: > > First, great! Thanks for doing this. A new shell really is the only > viable path forward here, and I appreciate you taking the time to > evaluate several and implement one. I disagree that a new shell is the _only_ way forward. AFAICT, all the raised concerns have long been fixed in upstream versions of hush; see for example [1]: ... //config: hush is a small shell. It handles the normal flow control //config: constructs such as if/then/elif/else/fi, for/in/do/done, while loops, //config: case/esac. Redirections, here documents, $((arithmetic)) //config: and functions are supported. //config: [1] https://git.busybox.net/busybox/tree/shell/hush.c#n98 My gut feeling is that updating to a recent version of hush is the most efficent _backward_compatible_ way. And if we drop that requirement, we might even take a bigger step and move to lua - which would allow for a complete new level of script based extensions. > > - There is a serious error handling problem. Most original LIL code never > > checked errors. In almost every case, errors were silently ignored, even > > malloc failures! While I have designed new code to handle errors properly, > > there still remains a significant amount of original code which just > > ignores > > errors. In particular, I would like to ensure that the following > > categories of > > error conditions are handled: This is something that scares me like hell. This in a shell? For me this is close to a killing point. > > - Running out of memory. > > - Access to a nonexistant variable. > > - Passing the wrong number of arguments to a function. > > - Interpreting a value as the wrong type (e.g. "foo" should not have a > > numeric > > representation, instead of just being treated as 1). Who says so? Bash says: -> printf "%d\n" foo -bash: printf: foo: invalid number 0 So it is _not_ 1 ... > > - There are many deviations from TCL with no purpose. For example, the list > > indexing function is named "index" and not "lindex". It is perfectly fine > > to > > drop features or change semantics to reduce code size, make parsing > > easier, > > or make execution easier. But changing things for the sake of it should be > > avoided. It's not a standard POSIX shell, it's not TCL (ick!), ... it's something new, incompatible... > Thanks for the evaluations, of these, lil does make the most sense. You mean, adding a complete new, incompatible and non-standard shell is a better approach than updating to a recent version of hush? What makes you think so? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Ernest asks Frank how long he has been working for the company. "Ever since they threatened to fire me."
Re: [RFC PATCH 03/28] cli: lil: Replace strclone with strdup
Dear Rasmus, In message you wrote: > > But that begs the question: What is the long-term plan for this? While > it does seem to be an improvement compared to hush, will we ever be able > to incorporate fixes&features from upstream, or will this code end up in > the same situation as hush? I would like to put the context of this right. As written, "improvement compared to hush", this could be misunderstood. What you likely mean is "compared to our ancient (nearly 20 years old) version of hush". Comparing to recent versions have probably much different results. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Ever try. Ever fail. No matter. Try again. Fail again. Fail better. -- S. Beckett
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Sean,
In message you wrote:
> >
> > Do you have a list of the exact differencec between LIL and a
> > standard shell?
>
> For a partial list, see
>
> [1] https://github.com/Forty-Bot/lil/commits/master
Hm, this list of commits is not exactly helpful, I'm afraid.
Where _exactly_ should I look?
> > I wonder, if we deviate from standard shell syntax anyway, we could
> > also have a look at lua, for example?
>
> I also looked at lua (see the cover letter), but I rejected it based on
> size constraints (eLua was around the size of U-Boot itself).
I have to admit that I never tried myself to build a lua based
system, optimizing for minimal size - but I'm surprised by your
results.
> Because of how often the shell is used to debug things, I wanted the
> candidate I picked to have similar syntax to the existing shell. For
> example,
>
> load mmc ${mmcdev}:${mmcpart} $loadaddr $image
>
> is a valid command in both Hush and LIL. Compare with lua, which might
> express the above as
Yes, I'm aware of this. But then, it would add another level of
powerful scripting capabilities - and as it was already suggested
before, _replacing_ the standard shell is only one way to use lua -
another would be to use lua as a command that can be started from
the shell when needed - assuming you want to pay the price in terms
of size.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
They're usually so busy thinking about what happens next that the
only time they ever find out what is happening now is when they come
to look back on it. - Terry Pratchett, _Wyrd Sisters_
Re: [RFC PATCH 03/28] cli: lil: Replace strclone with strdup
Dear Sean, In message you wrote: > > Well, since Hush was never updated, I don't believe LIL will be either. Let's please be exact here: Hus has never been updated _in_U-Boot_, but it has seen a lot of changes upstream, which apparently fix all the issues that motivated you to look for a replacement. > I think reducing the amount of ifdefs makes the code substantially > easier to maintain. My intention is to just use LIL as a starting point > which can be modified as needed to better suit U-Boot. > > The other half of this is that LIL is not particularly actively > developed. I believe the author sees his work as essentially > feature-complete, so I expect no major features which we might like to > backport. This sounds like an advantage, indeed, but then you can also interpret this as betting on a dead horse... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Never worry about theory as long as the machinery does what it's supposed to do. - R. A. Heinlein
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Sean,
In message <8bbdb7a1-5085-a3b7-614f-12ae9aee8...@gmail.com> you wrote:
>
> > For a partial list, see
> >
> > [1] https://github.com/Forty-Bot/lil/commits/master
>
> Whoops, looks like I completely misread what you were asking here. I
> don't have an exhaustive list of differences, but here are some similar
> things expressed in both languages:
>
> shtcl
>
> foo=bar set foo bar
> echo $foo echo $foo
>
> if [ 1 -gt 2 ]; then if {1 > 2} {
> echo a echo a
> else } {
> echo b echo b
> fi}
>
> foo() { proc foo {first second} {
> echo $1 $2 echo $first $second
> } }
>
> for file in $(ls *.c); do foreach file [glob *.c] {
> echo $file echo $file
> done }
>
> fact() {
> if [ $1 -eq 0 ]; then
> echo 1
> else
> echo $(($1 * $(fact $(($1 - 1)
> fi
> }
>
> proc fact {n} {
> if {$n} {
> expr {$n * [fact [expr {$n -
> 1}]]}
> } {
> return 1
> }
> }
>
> Hopefully this gives you a bit of a feel for the basic differences.
Well, I know TCL, and there has been a zillion of reasons to move
_from_ that language many, many years ago. Intoducing this as new
"shell" language in U-Boot does not look attractive to me.
Actually, it gives me the creeps.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Drawing on my fine command of language, I said nothing.
Re: [RFC PATCH 03/28] cli: lil: Replace strclone with strdup
t it is not suited for. If we are going to go to No, they are not. They are much more the consequence of no strict design guidelines for commands, so everybody implements what fits his purposes best. A cleaner approach does not require any of the additional features you've asked for - it would be possible as is. > the effort of porting a new language (which must be done no matter if > we use Hush or some other language), we should pick one which has better > support for single-threaded programming. In which way do you think "a new language" needs to be ported when switching from an old to a new version of hush? It would be still a (mostly) POSIX compatible shell, with some restrictions. I can fully understand that you defend your proposal, but let's be fair and stick with the facts. Thanks. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de To get something done, a committee should consist of no more than three men, two of them absent.
Re: [RFC PATCH 05/28] cli: lil: Rename some functions to be more like TCL
Dear Sean, In message you wrote: > > > Is your intent to create a fork of this in U-Boot? > > Yes. I believe some of the major additions I have made (especially "[RFC > PATCH 21/28] cli: lil: Add a distinct parsing step") would not be > accepted by upstream. Ouch... > > Could we not update things upstream, at least as an option, to avoid > > carrying these patches? > > For some of the smaller patches, that may be possible. However, I am not > a fan of the major amount of ifdefs that Hush has. For something as core > as a shell, I think we should be free to make changes as we see fit > without worrying about how it will affect a hypothetical backport. I'm afraind I cannot understand your thinking. You complain that the existing port of hus has a number of severe limitations or bugs which have long been fixed upstream, but cannot be easily fixed in U-Boot because we essentially created an unmaintained fork - and as a cure, you recommend to do the same thing again, but this time intentionally and deliberately? If you had not apparently already invested a lot of effort into this thing I would assume you must be joking... To me such an approach is unacceptable. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de If a train station is a place where a train stops, then what's a workstation?
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Tom,
In message <20210705191058.GB9516@bill-the-cat> you wrote:
>
> > > foo=bar set foo bar
> > > echo $foo echo $foo
> > >
> > > if [ 1 -gt 2 ]; then if {1 > 2} {
> > > echo a echo a
> > > else } {
> > > echo b echo b
> > > fi}
> > >
> > > foo() { proc foo {first second} {
> > > echo $1 $2 echo $first $second
> > > } }
> > >
> > > for file in $(ls *.c); do foreach file [glob *.c] {
> > > echo $file echo $file
> > > done }
> > >
> > > fact() {
> > > if [ $1 -eq 0 ]; then
> > > echo 1
> > > else
> > > echo $(($1 * $(fact $(($1 - 1)
> > > fi
> > > }
> > >
> > > proc fact {n} {
> > > if {$n} {
> > > expr {$n * [fact [expr {$n -
> > > 1}]]}
> > > } {
> > > return 1
> > > }
> > > }
> > >
> > > Hopefully this gives you a bit of a feel for the basic differences.
>
> Which of these things, from each column, can you do in the context of
> U-Boot? That's important too.
Well, with a current version of hush we can do:
-> foo=bar
-> echo $foo
bar
-> if [ 1 -gt 2 ]; then
> echo a
> else
> echo b
> fi
b
-> foo() {
> echo $1 $2
> }
-> foo bar baz
bar baz
-> for file in $(ls *.c); do
> echo $file
> done
ls: cannot access '*.c': No such file or directory
-> fact() {
> if [ $1 -eq 0 ]; then
> echo 1
> else
> echo $(($1 * $(fact $(($1 - 1)
> fi
> }
-> fact 4
24
Oh, in the contect of U-Boot? Well, there are of course
limitations, but not because of the shell, but because of the fact
that we have no concept of files, for example.
But another command interpreter will not fix this.
> This is I think the hard question. A draw of the current shell is that
> it it looks and acts like bash/sh/etc, for at least basic operations.
> That's something that's comfortable to a large audience. That has
> disadvantages when people want to start doing something complex. Sean
> has shown that several times and he's not the only one. LIL being
> tcl'ish is not.
Tcl is a horror of a language for anything that is above trivial
level.
Do you really think that replacing standard shell syntax with Tcl is
"something that's comfortable to a large audience"? I seriously
doubt that.
> Something that has "sh" syntax but also clear to the user errors when
> trying to do something not supported would also be interesting to see.
> It seems like a lot of the frustration from users with our shell is that
> it's not clear where the line between "this is an sh-like shell" and
> "no, not like that" is.
Did you run some tests on the version of hush as comes with recent
busybox releases? Which of our user's requirements does it fail to
meet?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The first thing we do is kill all the lawyers.
(Shakespeare. II Henry VI, Act IV, scene ii)
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Sean,
In message you wrote:
>
> >>> foo() { proc foo {first second} {
> >>> echo $1 $2 echo $first $second
> >>> } }
>
> This is not possible. We only have eval (run) as of today. I view adding
> functions as one of the most important usability improvements we can
> make.
Again: this is not an issue with hush as is, but only with our
resource-limited port of a nearly 20 year old version of it.
Updating to a current version would fix this, in an almost 100%
backward compatible way.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
If you don't have time to do it right, when will you have time to do
it over?- John Wooden
Re: [RFC PATCH 05/28] cli: lil: Rename some functions to be more like TCL
Dear Sean, In message <7143cb1e-4061-3034-57b9-1a12753fa...@gmail.com> you wrote: > > > > You complain that the existing port of hus has a number of severe > > limitations or bugs which have long been fixed upstream, > > The bugs are fairly minor. The particular characteristics of Hush have > not changed. These characteristics make Hush difficult to adapt to the > limitations of U-Boot. When we cannot support the basic abstractions > expected by Hush, the shell will necessarily change for the worse. This is not true. Just have a look what hush in a recent version of Busybox offers. > > but cannot be easily fixed in U-Boot > > Because they are core to the design of Hush (and other bourne derived > shells). Oh, this is an interesting opinion. I doubt if a majority (or even a significant percentage) of U-Boot users share it. If you were right, there would be far less users of bash (or other "bourne derived shells"). Guess which percentage of users of UNIX operating systems is using a Tcl based command interpreter as their login shell? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "There are three principal ways to lose money: wine, women, and engi- neers. While the first two are more pleasant, the third is by far the more certain." - Baron Rothschild, ca. 1800
Re: [RFC PATCH 05/28] cli: lil: Rename some functions to be more like TCL
Dear Tom, In message <20210705185141.GA9516@bill-the-cat> you wrote: > > I think I want to try and address this. While with "hush" we have > something that's in heavy active development outside of U-Boot, with LIL > we have something that's mature and "done". Mature? And still without consequent error checking? And done, i. e. this will never be fixed? > Tracking an active outside development is HARD and requires > constant resync. Based on that logic we should stop supporting Linux, and stop using DTs or file systems - all of these are under "active outside development". This is a bogus argiment. > Look at the last few > LIL releases. That could be easily re-worked in to our fork if needed. > I see that as a positive, not a negative. OK, this is your opinion. Mine differs. Please consider this as a full NAK from me when when you think of it as a _replacement_ (even an optional one) of the standard shell. If you like, have it added as an _additional_ command, of course fully optional and without impact on the rest of U-Boot if not intentionally selected. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Of all the things I've lost, I miss my mind the most.
Re: [ANN] U-Boot v2021.07 released
79 (4.6%) Xilinx 49 (2.8%) Linaro 47 (2.7%) BayLibre SAS41 (2.4%) ... Top lines changed by employer Marvell 241819 (53.4%) Texas Instruments 50334 (11.1%) Konsulko Group44582 (9.8%) (Unknown) 42381 (9.4%) Google, Inc. 14090 (3.1%) Linaro12952 (2.9%) NXP 10413 (2.3%) Xilinx8712 (1.9%) DENX Software Engineering 7161 (1.6%) Amarula Solutions 5089 (1.1%) ... Employers with the most signoffs (total 303) DENX Software Engineering 118 (38.9%) NXP 61 (20.1%) (Unknown) 31 (10.2%) ARM 22 (7.3%) Xilinx 20 (6.6%) Texas Instruments 18 (5.9%) Marvell 8 (2.6%) CompuLab 6 (2.0%) Collabora Ltd. 5 (1.7%) Konsulko Group 3 (1.0%) ... Employers with the most hackers (total 190) (Unknown) 85 (44.7%) NXP 27 (14.2%) Marvell 10 (5.3%) Texas Instruments9 (4.7%) Linaro 8 (4.2%) DENX Software Engineering7 (3.7%) Xilinx 7 (3.7%) ARM 3 (1.6%) Intel3 (1.6%) Toradex 3 (1.6%) ... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There's an old story about the person who wished his computer were as easy to use as his telephone. That wish has come true, since I no longer know how to use my telephone.
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Tom, In message <20210706145420.GQ9516@bill-the-cat> you wrote: > > > Updating to a current version would fix this, in an almost 100% > > backward compatible way. > > Let us cut to the chase then. Who is going to port a modern version of > hush over to U-Boot, and maintain it? If we fork and forget again, > we'll be in a bad place once again in 2-3 years. Would we really be better off if we switch to some exotic piece of code instead (I was not able to locate any user base, nor other developers), which has been reported to have poor or no error handling, and comes with an incompatible command line interface? There is a zillion of shell scripts in the field, from non-trivial boot sequences to complex download-and-upgrade scripts. You can't really even think of breaking compatibility on such a level. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are no data that cannot be plotted on a straight line if the axis are chosen correctly.
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Tom, In message <20210706154346.GT9516@bill-the-cat> you wrote: > > I'm pretty confident that exactly zero people have written complex > U-Boot scripts and then been happy about the experience. I have seen many U-Boot scripts which were pretty complex, but working absolutely reliably. > TCL has its fans. csh has it's fans. The question isn't what's the > best desktop shell or general scripting language, but what's the most > useful in our environment an use cases. Maybe you should try and do a poll of our user base which CLI they _want_? I doubt there will be any significant percentage voting for Tcl. I know of a large number of systems which offer a shell interface on their command line, and those who don't usually use completely proprietary code. I know of very few examples where Tcl is being used. > I don't know if it's right either. But drawing on my comment just now > and above about complex boot scripts, I also don't know if "it's sh but > quirky and incomplete, WHY DOESN'T THIS WORK RIGHT" is better than "It's > TCL? I don't know that, let me hit stackoverflow and do a little > reading" as would be the common experience. Especially if we document > up-front what the quirks we have are. Point taken. But if you think this to an end, the result is: lets write some documentation and explain the limitations of a shell in U-Boot environment, and document the warts and bugs of this (or an updated) version of hush. This should make more users happy than completely new and incompatible stuff. Frankly, I believe when you run into problems with hush in U-Boot (even the current version) you should lean back and think about what you are doing. U-Boot is a boot loader, and while it is powerful enough to do complex things, this is not necessarily the most clever approach. 15 years ago, I've written complex update scripts for U-Boot. This was easy enough to do, and worked perfectly. But there are so many limitations in a boot loader environment. We don't do this any more. Instead, we use an OS suitable for such tasks (Linux with SWUpdate). And talking about problems and limitations in U-Boot... Is the CLI really our biggest concern right now? None of our users (customers) has asked for a better command interpreter - the question we hear are more like: "When will you support IPv6?", "NFS does not work with recent Linux distros, will this be fixed?", "Can I download over WiFi?", "Can I download using HTTP/HTTPS?", "How can I harden U-Boot for security-critical environments?", etc. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Always try to do things in chronological order; it's less confusing that way.
Re: [RFC PATCH 05/28] cli: lil: Rename some functions to be more like TCL
Dear Tom, In message <20210706153327.GS9516@bill-the-cat> you wrote: > > > Mature? And still without consequent error checking? And done, > > i. e. this will never be fixed? > > Intentional design by upstream, and then for the actual problem part > (error checking, test suite), Sean is saying he'll fix it, and has > started on it. Seriously - any piece of software that omits error checking intentionally be design should be indented six feet downward and covered with dirt. We should not even consider looking at it. > OK, snark aside, I'm very serious here, any "we'll just import ..." > needs to have a plan to keep it up to date, or be easy enough to do such > that I can set a monthly reminder to check for and do the update. Every > area where we don't do this is a set of problems waiting to get worse, > as we can see with the hush shell right now as it's one of the oldest > things we stopped syncing with. Which exact _new_ problems do we see with hush right now? I can only see old ones, that have been known (and worked around) for nearly two decades. The limitations and bugs have all been there since the beginning - the limitations actually being intentional due to the typical resource situation at that time. > But I > really think we want a shell environment that is not actively adding new > features is a good thing, for the default. Just how much stuff should > we be doing or need to be doing before we hand things over to the OS? You are shooting yourself in the knee here. If you think out CLI should not be adding new features, then we should just stick with our ancient hush and neither update it nor replace it with something else that adds not only new features, but breaks backward compatibility, hard. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de God runs electromagnetics by wave theory on Monday, Wednesday, and Friday, and the Devil runs them by quantum theory on Tuesday, Thurs- day, and Saturday. -- William Bragg
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Tom, In message <20210707135839.GW9516@bill-the-cat> you wrote: > > As I've said a few times in this thread, this not being an sh-style > interpreter is a strike against it. And if we're going to insist on a > bug-for-bug upgrade to our hush (so that all of the hugely complex > existing scripts keep working) we might as well not upgrade. Frankly I > suspect that down the line IF a new cli interpreter comes in to U-Boot > we will have to keep the old one around as a "use this instead" option > for another long number of years, so that if there are any systems with > non-trivial scripts but upgrade U-Boot and don't / won't / can't > re-validate their entire sequence, they can just use the old cli. Do you actually have an example where code working on our ancient port of hush would fail on the current upstream version? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de If you can't explain it to a six year old, you don't understand it yourself. - Albert Einstein
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Tom, In message <20210707141418.GZ9516@bill-the-cat> you wrote: > > Have you validated one of those exceedingly complex boot scripts with a > modern hush (and some fakery for u-boot commands) ? No. No, I havent. I also don't claim that I know all the warts and issues with our old hus, but for the ones I am aware the woraround usually splitting complex or nested expressions into a sequence of simpler steps. I cannot remember any cases where the resulting code should be incompatible to a shell without that specific bug. And yes, I am aware of the problems with the distro_bootcmd stuff. Thisis exactly what I had in mind when I wrote: if you run into such situations you should lean back and reflect a bit. I can undrstand the intentions of all this stuff, but the implementation is a horrible mess. > I'm just > saying I expect there to be enough risk-adverse groups that just > dropping our old hush entirely might not be possible right away. Of > course, if all of the current in-tree complex cases Just Work, that > might be a good argument against needing to keep such levels of > backwards compatibility. There is only one way to test this. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The inappropriate cannot be beautiful. - Frank Lloyd Wright _The Future of Architecture_ (1953)
Re: [RFC PATCH 03/28] cli: lil: Replace strclone with strdup
mpletely unrecognizable compared to what we have in > U-Boot today. Any "updating" effort would be akin to going over the > entire upstream codebase and porting it from scratch. Agreed. But this means porting some code, which still implements the very same language (i. e. "shell"). There would be no new language in this case - just a bigger subset, less restrictions, more options, probably. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de God made machine language; all the rest is the work of man.
Re: [RFC PATCH 05/28] cli: lil: Rename some functions to be more like TCL
Dear Sean, In message you wrote: > ... > And yet, this is not the field we compete in. While bourne-style shells > can take advantage of a multi-threaded environment, embedded shells tend > to implement a much wider set of languages. See [1] for a survey of > examples. > > [1] https://github.com/dbohdan/embedded-scripting-languages "Embedded scripting languages" is probably not a good match for what we are discussing. I don't think this comparison was made for restricted boot loader environments, but for use in general purpose / embedded operating systems. Hey, they even list Forth there. Maybe somebody should port OpenBoot, so we can have a Forth interpreter as new commandline language? We could implement device trees in the old, traditional way then, too :-) Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are bugs and then there are bugs. And then there are bugs. - Karl Lehenbauer
Re: [RFC PATCH 02/28] cli: Add LIL shell
Dear Sean, In message <43880bf0-baa6-0cb2-80fe-0fe50d43b...@gmail.com> you wrote: > > > Tcl is a horror of a language for anything that is above trivial > > level. > > Can you please elaborate on this? You've made this claim several times, > but haven't explained your reasoning for it. A long, long time ago I I was looking for an efficient way to do regression testing for embedded systems, mainly U-Boot and Linux. I found GNU gnats to bee too difficult to handle und started writing my own test system (you can still find this ancient stuff here [1]) based on expect/Tcl. At that time, I could not find any other solution, so I had to accept Tcl as programming language for this tool. And at the beginning it usually worked just fine. Mostly. Coming from a C programming environment I always found Tcl ... strange. For example it's scoping rules (see [2] for an example). Things got worse when the system grew and we had to deal with strings or parameters which might contail quotes or other special characters. This required fancy quoting, which depended of the calling depth - if there was only on specific level. you might find a solution after some (cumbersome) experimenting. But if the same string was used in several places in the code, and different levels, you really lost. At a certain point all our developers refused to write new test cases for the system, because it always was the same nightmare of fighting unforeseeable language problems. That was when we abandoned DUTS, and some time later Heiko came up with an initial implementation of tbot, soon to be joined by Harald to make a production quality test system out of it [4]. IMO, Tcl has a large number of situations where it's actual behaviours is not what you expect from it - it is good enough for simple things, but any more complex processing of arbitrary text data quickly drives you insane - things like quotes or braces and other special characters. [1] http://www.denx.de/wiki/DUTS/DUTSDocs [2] https://wiki.tcl-lang.org/page/Commonly+encountered+problems+in+Tcl [3] https://wiki.tcl-lang.org/page/Why+can+I+not+place+unmatched+braces+in+Tcl+comments [4] https://tbot.tools/ Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The number you have dialed is imaginary. Please divide by 0 and try again.
Re: [PATCH] efidebug: Introduce bootmgr command
Dear Ilias, In message <20210210105425.356131-1-ilias.apalodi...@linaro.org> you wrote: > Up to now we've been adding all the efi related configuration to > 'efidebug' command. The command name feels a bit weird to configure boot > manager related commands. Since the bootmanager is growing and we intend > to extend it with features like defining the initrd we want to expose to > the kernel, it would make sense to split it on a command of it's own. > > So let's introduce a new command called bootmgr and move all of the > existing Boot manager functionality there. As this is EFI specific, I would appreciate to have "efi" in the command name, too. Maybe all EFi related commands should be collected as "efi " like we did it with the "env" commands long ago. For backward compatibility e. g. 'efidebug' could be kept, but the new name would be 'efi debug'; likewise, your new command would be 'efi bootmgr' [or just 'efi boot' ?] Thanks! Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de In my experience the best way to get something done is to give it to someone who is busy. - Terry Pratchett, _Going_Postal_
Re: [PATCH] efidebug: Introduce bootmgr command
Dear Ilias,
In message you wrote:
>
> The efidebug for boot options wasn't introduced that long ago and I don't
> think anyone uses it in production. If someone would want to have it
> backwards
> compatible, please shout and we'll see what we can do, but I'd strongly
> prefer
> replacing it overall. If we truly want backwards compatibility though we must
> keep
> efidebug, changing the name to something like 'efi debug' just for the name
> similarity wouldn't help much as it would break things regardless.
In this case, "debug" would just be a sub-command of the "efi"
command, with more sub-commands under efi (like bootmgr or such)
following, same as we did for example with "env" (where many
commands maintain backward compatibility, but here this was
necessary because these have been in use forever):
env print -> printenv
env save-> saveenv
env set -> setenv
etc.
Maybe a similar approach makes sense for "efi" (with or without
backward compatibility, as you like - after all, this is just a
little name space pollution ;-) :
efi:
efi debug
efi bootmgr
efi ...
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
If you believe that feeling bad or worrying long enough will change a
past or future event, then you are residing on another planet with a
different reality system.
Re: [PATCH] efidebug: Introduce bootmgr command
Dear Ilias, In message you wrote: > > > In this case, "debug" would just be a sub-command of the "efi" > > command, with more sub-commands under efi (like bootmgr or such) > > following, same as we did for example with "env" (where many > > commands maintain backward compatibility, but here this was > > necessary because these have been in use forever): > > > > env print -> printenv > > env save-> saveenv > > env set -> setenv > > > > Yes, but it would still look a bit strange, because the efidebug command was > overloaded in our case. So you could test capsules, set EFI bootmgr variables, > dump EFI tables amongst other things. The saveenv & friends had a tightly > defined scope already. > So that would require a lot of code to keep the convention running, but since > it's rarely used, I don't think it's worth the effort or the additional > complexity (once again unless someone has a valid reason), hence my suggestion > to define it properly while we still have time. Indeed - if it was rarely used before, it makes a lot of sense to break combined functionality into separate subcommands (probably also with separate Kconfig options so you can select what you really need only). Thanks! Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The biggest difference between time and space is that you can't reuse time. - Merrick Furst
Re: [PATCH] efidebug: Introduce bootmgr command
Dear Heinrich, In message <9966b14a-43ef-42ad-dda4-d40b5e3b9...@gmx.de> you wrote: > > > For backward compatibility e. g. 'efidebug' could be kept, but the > > new name would be 'efi debug'; likewise, your new command would be > > 'efi bootmgr' [or just 'efi boot' ?] ... > > what is the benefit for the user to have a single command with an > endless list of options? > > I can't see any. You miss the point. The complexisty for the user will be the same; as is, we already have a single command whichimplements a number of different functions as one big monolithig block of code. What we are discussing here is to split this into logical components, each implementing one specific function only. The suggested change will not make things any harder to the user, on contrary, it has the chance to make it easier andmore consistent to use. Also, it's not options, but separate commands grouped into one topic. Advantages are: - maintenance: it is much easier to maintain and debug a number of separate functions than monster spaghetti code - configurability: you can select needed / unselect unnecessary sub commands - memory footprint: comes with configurability - style: it follows existing practise. Just to name a few. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de When the tide of life turns against you And the current upsets your boat Don't waste tears on what might have been Just lie on your back and float.
Re: [PATCH 00/15] lib: Add support for a decimal 0m prefix for numbers
Dear Simon, In message <20210720132940.1171011-1-...@chromium.org> you wrote: > U-Boot mostly uses hex for value input, largely because addresses are much > easier to understand in hex. > > But in some cases a hex value is requested, but it is more convenient to > provide a decimal value. This may be because the value comes from another > source, where its base cannot be controlled. > > This series adds support for a 0m prefix to indicate a decimal number. The > letter 'm' is chosen because: What is the impact of this change on the U-Boot size for typical configurations? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Sorry, but my karma just ran over your dogma.
Re: [PATCH 00/15] lib: Add support for a decimal 0m prefix for numbers
Hi, In message <20210720160547.GM9379@bill-the-cat> you wrote: > > > So for example (10)123 would mean decimal 123? I don't know how we > > would parse brackets separately from expressions though. > > (123)10 would be "123" in decimal. Which is indeed a mouthful. But it > would also be generic and (123)16 would be 0x123. So the parsing > shouldn't be too hard, for most commands. But then yes, expressions > become quite hard. Come on, guys, be serious! This is a boot loader. Size matters. Do we _really_ need all this, and is it worth the code size? Simon's patches include some cleanup, which probably even reduces the size, so good. But whether it's 0m123 or 0t123 or 0!123 or ... is pretty much irrelevant - chose one symbol, use it, and be done with that. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Do you suppose the reason the ends of the `Intel Inside' logo don't match up is that it was drawn on a Pentium?
Re: [PATCH 13/15] RFC: lib: Support a binary prefix 0y
Dear Simon, In message <20210720132940.1171011-14-...@chromium.org> you wrote: > In some cases it is useful to be able to supply a binary value to a > command. Use the '0y' prefix for this (binarY). We also don't handle octal input yet, and also miss a number of other interesting numberbases, like 42. But ... do we really *need* all this stuff? %% (signatures) Perfection is reached, not when there is no longer anything to add, but when there is no longer anything to take away. - Antoine de Saint-Exupery Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de PROGRAM - n. A magic spell cast over a computer allowing it to turn one's input into error messages. v. tr. - To engage in a pastime similar to banging one's head against a wall, but with fewer opportunities for reward.
Re: [PATCH 13/15] RFC: lib: Support a binary prefix 0y
Dear Sean, In message <9e1797ea-c1bf-5749-03a6-cf0e9430f...@gmail.com> you wrote: > On 7/20/21 9:29 AM, Simon Glass wrote: > > In some cases it is useful to be able to supply a binary value to a > > command. Use the '0y' prefix for this (binarY). > > To bikeshed: 0b please That doesn't work, as 0b010101 is in U-Boot in alost all contexts interpreted as 0x0b010101 (hex) = 184615169 (dec) Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Microsoft Multitasking: several applications can crash at the same time.
Re: [PATCH 13/15] RFC: lib: Support a binary prefix 0y
Dear Simon, In message you wrote: > > > But ... do we really *need* all this stuff? > > No... > > I added binary as an RFC because I have found a few cases where it is > nice to be able to specify the bits (e.g. programming GPIOs). We could > update 'md' to support it too. Yes, we could, but U-Boot size is continuously growing, even for constant configurations, i. e. when no nmew features are wanted/needed. > I added octal as an RFC since the current impl is almost never > available (only when 0 is parted to simple_strtoul()) which seems odd. I can't see how this should work... > Well, yes. Perhaps we should just drop octal? Drop? AFAICT we never supported octal. Something like "md 040" will start dumping at 0x0040, right? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de There are three things I always forget. Names, faces - the third I can't remember. - Italo Svevo
Re: [PATCH 13/15] RFC: lib: Support a binary prefix 0y
Dear Simon, In message you wrote: > > > Drop? AFAICT we never supported octal. Something like "md 040" will > > start dumping at 0x0040, right? > > There are quite a few places where simple_strtoul() is called with a > base of 0. In that case, if the string starts with 0 it is interpreted > as octal. That's a bug, then. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de panic: can't find /
Re: [PATCH 13/15] RFC: lib: Support a binary prefix 0y
Dear Simon, In message you wrote: > > > > There are quite a few places where simple_strtoul() is called with a > > > base of 0. In that case, if the string starts with 0 it is interpreted > > > as octal. > > > > That's a bug, then. > > If so it is a 19-year-old bug: (was lib_generic/vsprintc.c) > > 153d511e369 Initial revision Mea culpa. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The trouble with our times is that the future is not what it used to be. - Paul Valery
Re: [PATCH] lib: rsa: Add debug message on algo mismatch
Dear Sean Anderson,
In message <20210216164016.635125-1-sean.ander...@seco.com> you wrote:
> Currently we fail silently if there is an algorithm mismatch. To help
> distinguish this failure condition.
>
> Signed-off-by: Sean Anderson
> ---
>
> lib/rsa/rsa-verify.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> index e34d3293d1..aee76f42d5 100644
> --- a/lib/rsa/rsa-verify.c
> +++ b/lib/rsa/rsa-verify.c
> @@ -447,8 +447,11 @@ static int rsa_verify_with_keynode(struct
> image_sign_info *info,
> }
>
> algo = fdt_getprop(blob, node, "algo", NULL);
> - if (strcmp(info->name, algo))
> + if (strcmp(info->name, algo)) {
> + debug("%s: Wrong algo: have %s, expected %s", __func__,
> + info->name, algo);
> return -EFAULT;
> + }
If this is considered an error, should that not be a printf() then
instead of a debug() which users will never see?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
It is impractical for the standard to attempt to constrain the
behavior of code that does not obey the constraints of the standard.
- Doug Gwyn
[IMPORTANT] gitlab relocation / rename
Hi all, I'm sorry that I have to inform you about a disruption of the gitlab services. This has become necessary for a number of reasons: - repeated cases of poor performance - difficulties with CI setup - security issues We have to move all public projects off gitlab.denx.de to a new gitlab instance; the new host name is then source.denx.de . We are fully aware that this will cause inconveniences, and I can only assure you that we try hard to keep these as limited as possible. # Details Around noon on Saturday, 2021-02-27, we will freeze all public repositories on gitlab.denx.de. They will still be readable/ cloneable but any attempt to push new commits after this point will fail. We will then copy them over to the now host, source.denx.de . All custodian accounts will also be transferred, the same login credentials from gitlab.denx.de will work on source.denx.de . The URLs for the repositories will stay the same, except of course for the host name. So, for example, the U-Boot mainline repository will then be hosted at https://source.denx.de/u-boot/u-boot.git We will install redirects to forward HTTP accesses from the old repository URLs to the new host. This should make the transition mostly transparent, but does not cover everything. The following needs to be taken care of manually: - Update the SSH URI for pushing to the repositories. Just replace gitlab.denx.de with source.denx.de . We will transfer all the SSH-keys to the new host so nothing else should be needed. - Make sure you're logging in on source.denx.de . Access to gitlab.denx.de will no longer work! - CI runners connected to gitlab.denx.de need to be re-registered with source.denx.de . The new host should hopefully be operational sometime on Sunday, 2021-02-28. We will send a further announcement once everything is back up and running. Thanks for your understanding and patience! Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de At the source of every error which is blamed on the computer you will find at least two human errors, including the error of blaming it on the computer.
Re: [PATCH 1/1] Correct U-Boot upstream repository
Dear Tom, In message <20210224134257.GJ10169@bill-the-cat> you wrote: > > > Where is this information posted? > > https://lists.denx.de/pipermail/u-boot/2021-February/442175.html > > Which yes, we need to figure out how to get more widely seen most > likely. Well, I posted it on the U-Boot list, and on the Custodian and Board Mainteiner lists, too (plus on the Xenomai list). I also update the link on the web page. What else can be done? If people don't even read messages flagged as important ? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de To be sure of hitting the target, shoot first and, whatever you hit, call it the target.
Re: [PATCH 1/1] Correct U-Boot upstream repository
Dear Heinrich, In message you wrote: > > If we can have a redirect on the old server, we should be fine. I wrote: | We will install redirects to forward HTTP accesses from the old | repository URLs to the new host. This should make the transition | mostly transparent, but does not cover everything. The following | needs to be taken care of manually: | | - Update the SSH URI for pushing to the repositories. Just replace | gitlab.denx.de with source.denx.de . We will transfer all the | SSH-keys to the new host so nothing else should be needed. | | - Make sure you're logging in on source.denx.de . Access to | gitlab.denx.de will no longer work! | | - CI runners connected to gitlab.denx.de need to be re-registered | with source.denx.de . And just to avoid misunderstandings: MTTPS too, of course. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Imagination is more important than knowledge. -- Albert Einstein
