Re: [PATCH v2] tools/mxsimage: Remove fclose on empty FILE pointer

2021-11-24 Thread Wolfgang Denk
Dear Mattias Hansson,

In message <20211124121049.24054-1-hansson.matt...@gmail.com> you wrote:
> If `sb_load_cmdfile()` fails to open the configuration file it will jump
> to error handling where the code will try to `fclose()` the FILE pointer
> which is NULL causing `mkimage` to segfault.
>
> This patch removes the label for error handling and instead returns
> immediately which skips the `fclose()` and prevents the segfault. The
> errno is also described in the error message to guide users.
>
> Signed-off-by: Mattias Hansson 

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
Time is a drug. Too much of it kills you.
  - Terry Pratchett, _Small Gods_


Re: [PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

2021-11-24 Thread Wolfgang Denk
Dear Mattias Hansson,

In message <20211123080633.8318-1-hansson.matt...@gmail.com> you wrote:
> If `sb_load_cmdfile()` fails to open the configuration file it will jump
> to error handling where the code will try to `fclose()` the FILE pointer
> which is NULL causing `mkimage` to segfault.
>
> This patch removes the `fclose()` since `fopen()` always returns NULL
> instead of the file descriptor when failing.
> ---
>  tools/mxsimage.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tools/mxsimage.c b/tools/mxsimage.c
> index 002f4b525a..c7bd86ce52 100644
> --- a/tools/mxsimage.c
> +++ b/tools/mxsimage.c
> @@ -1618,7 +1618,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
>   return 0;
>  
>  err_file:
> - fclose(fp);
>   fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
>   ictx->cfg_filename);
>   return -EINVAL;

The whole code with the goto is ugly.  Such an error exit may make
sense when used in several places, but here it just makes reading
the code more difficult.  I suggest to fix this like this:


diff --git a/tools/mxsimage.c b/tools/mxsimage.c
index 002f4b525a..801aaa62ce 100644
--- a/tools/mxsimage.c
+++ b/tools/mxsimage.c
@@ -1595,8 +1595,11 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
size_t len;
 
fp = fopen(ictx->cfg_filename, "r");
-   if (!fp)
-   goto err_file;
+   if (!fp) {
+   fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
+   ictx->cfg_filename);
+   return -EINVAL;
+   }
 
while ((rlen = getline(, , fp)) > 0) {
memset(, 0, sizeof(cmd));
@@ -1616,12 +1619,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
fclose(fp);
 
return 0;
-
-err_file:
-   fclose(fp);
-   fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
-   ictx->cfg_filename);
-   return -EINVAL;
 }
 
 static int sb_build_tree_from_cfg(struct sb_image_ctx *ictx)



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
Applying computer technology is simply finding the  right  wrench  to
pound in the correct screw.


Re: [PATCH v2 1/2] lib/circbuf: Make circbuf selectable symbol

2021-11-22 Thread Wolfgang Denk
Dear Loic Poulain,

In message <1637584252-15617-1-git-send-email-loic.poul...@linaro.org> you 
wrote:
> It is currenly only used from usbtty driver but make it properly
> selectable via Kconfig symbol, for future usage.
>
> Signed-off-by: Loic Poulain 
> ---
>  v2: no change
>
>  lib/Kconfig  | 3 +++
>  lib/Makefile | 8 +++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index c535147..1bd54d8 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -290,6 +290,9 @@ config TRACE_EARLY_ADDR
> the size is too small then the message which says the amount of early
> data being coped will the the same as the
>  
> +config CIRCBUF
> +bool
> +
>  source lib/dhry/Kconfig

Please add a description what this symbos is doing.

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
Old programmers never die, they just become managers.


Re: [PATCHv2] net: uclass: Save ethernet MAC address when generated

2021-11-22 Thread Wolfgang Denk
Dear Tom,

In message <20211120155358.376540-1-tr...@konsulko.com> you wrote:
>
> When MAC address is randomly generated it should be also saved to
> variables. This step is there when MAC address is passed via pdata but not
> when it is randomly generated.

"saved to variables" ? Which variables? In C code?  In the
environment?

This is not clear here.

> Selecting this will allow the Ethernet interface to function
> -   even when the ethaddr variable for that interface is unset.
> -   A new MAC address will be generated on every boot and it will
> -   not be added to the environment.
> +   even when the ethaddr variable for that interface is unset by
> +   generating a new MAC address in the locally administered address
> +   space and setting the appropriate environment variable.

Same here: "ethaddr variable" - I guess this refers to the
environment variable?  Then we should write that.

Hmmm: "when the ethaddr variable ... is unset by generating ... and
setting the appropriate environment variable."  this is misleading,
as the "ethaddr variable" is not unset by setting the env var.

Suggestion:

Selecting this will allow the Ethernet interface to function
even when the ethaddr variable for that interface is unset.
In this case, a random MAC address in the locally
administered address space is generated. It will be saved to
    the appropriate environment variable, 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
A conservative is a man with two perfectly good legs  who  has  never
learned to walk.  - Franklin D. Roosevelt


Re: [PATCH v2 0/2] env: setenv add resolve value option

2021-11-22 Thread Wolfgang Denk
Dear Artem,

In message  
you wrote:
> > >
> > > next examples just demonstrate how its works for already defined env
> > > variables which contain other variables (like storred env variables)
> >
> > Which next examples?
>
> Usage examples (from commit message):
>
> => setenv a hello; setenv b world; setenv c '${a} ${b}'
> => setenv -r d '${c}! ${a}...'
> => printenv d
> d=hello world! hello...

This is a very simple example, and I showed you how you can solve
this one by just omitting the apostrophes:

=> setenv a hello; setenv b world; setenv c ${a} ${b}
=> setenv d ${c}! ${a}...
=> printenv d
d=hello world! hello...


I _think_ what you actually have in mind is something like this:

=> setenv a hello
=> setenv b world
=> setenv c '${a} ${b}'
=> setenv a goodbye
=> setenv b sunshine

something to set d to: '${c}! ${a}...'

=> printenv d

Here my simple approach does not show what you want to have:

=> setenv a hello
=> setenv b world
=> setenv c ${a} ${b}
=> setenv a goodbye
=> setenv b sunshine
=> setenv d ${c}! ${a}...
=> printenv d
d=hello world! goodbye...

That's because here evaluation takes place at assignment, but you
want it when used - but being recursive here is neither a good idea
nor standard.

How would you do it in a standard posix shell?  You would have to
use "eval", like that:

$ a=hello
$ b=world
$ c='${a} ${b}'
$ a=goodbye
$ b=sunshine
$ d=$(eval echo $c)
$ echo $d
goodbye sunshine

But please note that "eval" is _not_ recursive!!

$ a='$b'
$ eval echo $c
$b sunshine

And this is why I object against this patch.

Oh, and in U-Boot you could write this as:

=> setenv a hello
=> setenv b world
=> setenv c '${a} ${b}'
=> setenv a goodbye
=> setenv b sunshine
=> setenv foo "setenv d ${c}! ${a}..."
=> run foo
=> printenv d 
d=goodbye sunshine! goodbye...

And yes, here you have to be careful about using ' or " as there is
no recursion like you might expect.

So yes, it would be nice if we had "eval" (which will ocme with the
hush update), and no, "eval" does not recurse either.



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
Real computer scientists despise the idea of actual  hardware.  Hard-
ware has limitations, software doesn't. It's a real shame that Turing
machines are so poor at I/O.


Re: [PATCH v2 0/2] env: setenv add resolve value option

2021-11-20 Thread Wolfgang Denk
Dear Artem,

In message  
you wrote:
>
> next examples just demonstrate how its works for already defined env
> variables which contain other variables (like storred env variables)

Which next examples?

> sure I know about this ! see my prev message please .

Which exact message are you referring to here?

> Why not have this new opportunity ?

I think the suggested code is adding more problems than it solves.

> > have in mind, as you speak of "deep resolve". But then, I'm first
> > missing an explanation (and documentation) of what "deep resolve"
>
> recurrent  resolving for variables

Your implementation of recursion has an arbiotrary and undocumented
depth limit. Also, I cannot see a way to prevent resolving in case I
want to keep something like "$foo" in the result.

But that's to be expected from such a non-standard way.

Why don't you stick with what "eval" in a standard shell does?

> > actually means in this context, i. e. how many levels down you
> > evaluat.   Oh...  the code has "int max_loop = 32;" - this is a
>
> i think its will be enough

It is a reallybad habt to implement code with arbitrary limits, as
it will blow into your face (or more likely that of an innocent
user) rather sooner than later.  It's even worse that this limit is
nowhere documented.

> 1) this option did not broke any exist compatibilities
> 2) there we talk not only about uboot shell, same time will be useful
> to have env_resolve for internal c usage, because env_set dont have
> this feature

I did not say that an "eval" like construct would not be useful.
But uncontrolled recursion with an undocumented depth limit is
a problem.

> yes i'm informed  about this plans  (and think its happens  not  so
> soon - but i provide some simple elegant solution already)
> but again we dont have env_resolve for internal c usage which must be
> very useful

On the CLI, we use the "run" command to get the desired effect. Yes,
this is neither perfect nor elegant. But you can use that in C code
as well.

> will be easy get useful features via simple solution ( deep resolve
> all vars by one line )

I understand what you want, but this is not a good way to solve the
problem.  I'd really rather see such efforts invested in helping
Francis with the hush update - which will make such code unnecessary.

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
Success in marriage is not so much finding the right person as it  is
being the right person.


Re: [PATCH v2 0/2] env: setenv add resolve value option

2021-11-18 Thread Wolfgang Denk
Dear Artem,

In message <2029043647.1251416-1-...@khadas.com> you wrote:
> Add possibility setup env variable with additional resolving vars inside
> value.

Hm... if you want to evaluate variables, you should not prevent the
shell to do that by enclosing them in apostrophes?

> Usage examples:
>
> => setenv a hello; setenv b world; setenv c '${a} ${b}'
> => setenv -r d '${c}! ${a}...'
> => printenv d
> d=hello world! hello...

Without any new code added:

=> setenv a hello; setenv b world; setenv c ${a} ${b}
=> setenv d ${c}! ${a}...
=> printenv d
d=hello world! hello...


I know very well that this does not cover all use cases you might
have in mind, as you speak of "deep resolve". But then, I'm first
missing an explanation (and documentation) of what "deep resolve"
actually means in this context, i. e. how many levels down you
evaluat.   Oh...  the code has "int max_loop = 32;" - this is a
limitation mentioned nowhere.  And are you really sure this is a
clever idea?  I am not convinced.  If we do something like this, we
should not invent a new non-standard way.  We should implement
standard shell behaviour instead (i. e. for example something like
the eval command).

> Artem Lapkin (2):
>   env: setenv add resolve value option
>   test: env: deep resolve value testing

This is one more of the patches that actually try to fix existing
problems with our ancient verion of the hush shell.  As far as I
understand, Francis (added to Cc:) has started working on an update
of hush to a current version.  We should rather help him with that
instead of implementing non-standard workarounds.

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 choosing between two evils, I always like to take the  one  I've
never tried before. -- Mae West, "Klondike Annie"


Re: [PATCH] net: uclass: Save ethernet MAC address when generated

2021-11-18 Thread Wolfgang Denk
Dear Tom,

In message <2028162920.GH24579@bill-the-cat> you wrote:
> 
> > It is perfectly OK for U-Boot to start with a random MAC address,
> > use this for a while, and change it so something else later.  This
> > is what may happen at production: say the MAC address is stored in
> > some EEPROM or fuses, which are initially empty, so U-Boot will use
> > a random MAC address, download it's board specific date (serial#,
> > MAC address, ...) over network, programm it into the respective
> > storage devices, and switch to using the new "official" MAC address.
>
> Yes.  And up until this patch saying I want to use this random MAC with
> Linux required user intervention.

Correct - this is a bug in the implementation of thispatch, and
apparently the few people that ever used it did not notice it or
care enough about it to submit fixes.

> And I dare say that half the time or
> more that was probably just not noticed (everything comes up with
> dynamic host names/dns these days, noticing the IP changed between
> U-Boot and Linux is easy to miss in those cases).

Agreed.

> > CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
> > automatically, except that it falls short of setting the "ethaddr"
> > environment variable.  I consider this a bug.
>
> Since the code isn't that old, it shouldn't be hard to pull up the
> thread / patch on introducing it.  So, lets:
> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-1-git-sen=
> d-email-joe.hershber...@ni.com/
> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-2-git-sen=
> d-email-joe.hershber...@ni.com/
> https://patchwork.ozlabs.org/project/uboot/patch/1430769315-27109-3-git-sen=
> d-email-joe.hershber...@ni.com/
>
> And from there we can take away I think two important things:
> 1: Not setting ethaddr with NET_RANDOM_ETHADDR is intentional

Ummm... From which part of the patches or the comments do you take
this conclusion?  Not a single line of code, or comments in the code
or commit messages, nor any of the review comments refer to the
"ethadddr" environment variable.  I thing it was just an oversight
to set it here, which was not detected during testing as everything
appeared to work.

I actually see the opposite - Joes initial commit messages speaks
about "Implement the random *ethaddr* fallback", and as his
following patches clean up the use of hard codes definitions of the
"ethaddr" environment variable, I see it more likely that he means
the environment variable and not MAC address.

> 2: It was widely inconsistent before!  Lots of platforms were generating
>a random MAC and then setting ethaddr.  I think in fact most were and
>it's just a few (which coincidentally are some of Michael's other
>platforms) that were not.

Agreed.

> > Yes, and it should be clear that this is not a reasonable approach.
> > It thwarts the original intent of the NET_RANDOM_ETHADDR patch to do
> > thing in an automated, scriptable way. I see this actually as a
> > manifestation of the bug that ethaddr does not get set. At this
> > point the problem was recognized, but instead of properly fixing it,
> > a poor workaround was documented.
>
> Well, for the record, with the above patch links, NET_RANDOM_ETHADDR
> _is_ working as intended.  That said..

No, it works only half.  It fails to set the "ethaddr" environment
variable which is mandatory for correct funktion (query by the user,
passing to Linux).

> So, yes, OK, this is a bug fix, in that NET_RANDOM_ETHADDR was incorrect
> at introduction.  That means we do need a v2 of this patch that updates
> the Kconfig help text as that currently says it will not update the
> environment.

This makes no sense to me.  Instead of documenting the bug we should
fix it and add the missing eth_env_set_enetaddr().

If I prepare such patches, will you accept these?


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] net: uclass: Save ethernet MAC address when generated

2021-11-18 Thread Wolfgang Denk
Dear Tom,

In message <1889944.1637219...@gemini.denx.de> I wrote:
>
> > We're about to
> > introduce the 3rd variant.  I'd feel a whole lot better about taking in
> > a v2 of this patch that correct the help (and maybe updates
> > doc/README.enetaddr!) if someone could report back on what's going on
> > with the layerscape, imx* and socfpga platforms.  There's in fact a
> > number of platforms enabling it that I'm pretty sure DENX has or had the
> > hardware on, so can we get some spot checking done there?
>
> I will check and provide an update later, but from the best of my
> knowledge none of the boards we ported actually need or use this
> feature.  This is just a copy mess.

We could not find even a single board maintained or upstreamed
by DENX where NET_RANDOM_ETHADDR was even mentioned in the
specifications, and certainly not a mandatory feature.

That's all copy & paste garbage.

Viele Grüße,

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 human mind treats a new idea the way the body  treats  a  strange
protein - it rejects it. - P. Medawar


Re: [PATCH] net: uclass: Save ethernet MAC address when generated

2021-11-17 Thread Wolfgang Denk
Dear Tom,

In message <2027161545.GA24579@bill-the-cat> you wrote:
> 
> Yes, you're changing behavior by requiring this change, and fwiw I
> suggested a slightly different fix-up here of deleting the device tree
> properties if it's a random MAC, in Michael's patch just disabling the
> feature on the platforms he cares about.

Of course fixing a bug will change behaviour; that's the intention
of the fix.

Technically there is no difference between the user setting
"ethaddr" manually to a locally administered MAC address or doing
this automatically in some code or script.  In all cases setting
"ethaddr" means: hey, this is my MAC address, please use it.

> I've asked Neil to chime in on the amlogic cases after talking on IRC,
> but the short answer was for prior to fuse/serial# reading support for a
> non-random MAC.  Possibly other SoCs in a similar situation.

It is perfectly OK for U-Boot to start with a random MAC address,
use this for a while, and change it so something else later.  This
is what may happen at production: say the MAC address is stored in
some EEPROM or fuses, which are initially empty, so U-Boot will use
a random MAC address, download it's board specific date (serial#,
MAC address, ...) over network, programm it into the respective
storage devices, and switch to using the new "official" MAC address.

> I don't mean this in a super snarky way, but I'm more concerned about
> the implications of changing our documented albeit slightly inconsistent
> behavior than I am about some of the myriad of technically feasible
> environment variable names we've also in theory supported.  For about
> half the time we've supported device trees, the solution to "I need to
> use a random MAC" was "run tools/gen_eth_addr, setenv, saveenv".

This is indeed what seems a straightforward approach to me.  The
problem here is that this needs to be done manually.
CONFIG_NET_RANDOM_ETHADDR is the attempt to do the same
automatically, except that it falls short of setting the "ethaddr"
environment variable.  I consider this a bug.

> For
> the second half of the time, it was the same, but with a side of "or note
> the random MAC from console logs and use that".

Yes, and it should be clear that this is not a reasonable approach.
It thwarts the original intent of the NET_RANDOM_ETHADDR patch to do
thing in an automated, scriptable way. I see this actually as a
manifestation of the bug that ethaddr does not get set. At this
point the problem was recognized, but instead of properly fixing it,
a poor workaround was documented.

> We're about to
> introduce the 3rd variant.  I'd feel a whole lot better about taking in
> a v2 of this patch that correct the help (and maybe updates
> doc/README.enetaddr!) if someone could report back on what's going on
> with the layerscape, imx* and socfpga platforms.  There's in fact a
> number of platforms enabling it that I'm pretty sure DENX has or had the
> hardware on, so can we get some spot checking done there?

I will check and provide an update later, but from the best of my
knowledge none of the boards we ported actually need or use this
feature.  This is just a copy mess.

> If we can
> drop from 250 platforms to 50 platforms with some level of confidence we
> understand why are setting NET_RANDOM_ETHADDR, I'd be a lot less worried
> we're about to introduce another change that we won't found out messed
> up a bunch of people on until some new work-around is accepted in to
> Linux or something.

Well, this work-around in Linux is because there have been
incnsistent ways how to store the MAC address in the device tree.
This has nothing to do with our problem here - Linux has no way to
know whether a locally administered MAC address has been assigned by
the user for a specific purpose, or if it has been generated
randomly.  Actually it does not even care.


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 know that feeling when you're leaning back  on  a  stool  and  it
starts to tip over? Well, that's how I feel all the time.
- Steven Wright


Re: [PATCH] net: uclass: Save ethernet MAC address when generated

2021-11-17 Thread Wolfgang Denk
Dear Tom,

In message <2027123548.GX24579@bill-the-cat> you wrote:
> 
> > Why would you expect any "do not use" note?  Locally adminitered MAC
> > addresses (even when randomly chosen) have been created for good
> > reason, so they should be usable.
>
> Because as been noted in either this thread, or the other long thread,
> the user does not want to confuse Linux with this emergency random MAC?

Classical answer: Don't do it, then.

If the user does not want to pass a MAC address to Linux, he should
simply not do it.  Deleting "ethaddr" from the environment should be
all that's needed.  [And if this does not work, then I consider this
a bug that should be fixed.]

> > I'm afraid I do not understand what exactly you are proposing here?
>
> I'm objecting to changing our long standing behavior of NOT
> automatically passing the random MAC to the OS.  That has always been
> user opt-in by using tools/gen_eth_addr and "setenv ethaddr ... ;
> saveenv".  Especially since I don't know how many of those 200+ boards
> that enable CONFIG_NET_RANDOM_ETHADDR today are "enabled, but never
> used" vs "enabled, random MAC in U-Boot since we don't care/didn't
> notice, real MAC in Linux".  So yes, another worry is that we have a
> class of boards in U-Boot where a random MAC is fine enough since it's
> rarely used/needed, but Linux can get the real MAC and now we'll be
> blowing that away.  Or maybe we just have a ton of boards that
> copypasta'd that option in and didn't really think why.

Unfortunately we can only speculate about that [my guess is that
most of them are just copy/paste while brain in low power mode].

> > But I object to using a MAC address in U-Boot in a way that makes it
> > invisible to the user who uses documented APIs ("printenv ethaddr").
>
> Well, that's the API we've had for over 10 years, and it was a common
> problem in those earlier days with lots of SBCs where it was cheap to
> toss in a USB eth controller that didn't store a MAC anywhere.  Now
> we're I believe hitting this due to FPGA stuff.

CONFIG_NET_RANDOM_ETHADDR has been added in 2015 with commit bef1014
"net: Implement random ethaddr fallback in eth.c"; from the changes
then I'm not sure if this was even USB related.

> > If we use some MAC address, it shall be possible to read it using
> > "printenv ethaddr" and to set it using "setenv ethaddr".  Anything
> > else is inconsistent crap.
>
> Well, we've been inconsistent about the former for forever and there's
> a lot of implications to changing it now.

Yes.  However, being bug-compatible is something we should not rate
too high.

[non-random signature used]

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 net result is a system that is not only binary  compatible  with
4.3  BSD, but is even bug for bug compatible in almost all features."
- Avadit  Tevanian,  Jr.,  "Architecture-Independent  Virtual  Memory
Management  for  Parallel  and  Distributed  Environments:  The  Mach
Approach"


Re: [PATCH] net: uclass: Save ethernet MAC address when generated

2021-11-17 Thread Wolfgang Denk
Dear Tom,

In message <2027115025.GV24579@bill-the-cat> you wrote:
> 
> > If the MAC address is not placed in the environment, then how can a
> > user query the currently used MAC address?  All documentation says
> > basically: run "printenv ethaddr".
>
> Update the documentation and say to use "net list" or read the previous
> part of console log that says we're using a random mac in this case?
> The more I look here the more I see we're changing part of the API with
> the OS, and it's not something that should be done without consulting
> the consumers too.

Well... "printenv ethaddr" has been here ever since U-Boot (or
PPCBoot, as it was still called at that time) got network support.
i. e. more than 21 years ago.

"net list: has only been added very recently, just 5 months ago in
commit 8a3987f47 "cmd: net: add a 'net list' command to list network
devs".

I bet that an overwhelming majority of users would use "printenv
ethaddr" when asked to check the MAC address of a system.


> Next, pulling up
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-c=
> ontroller.yaml
> there's two important things.  First, there's no way to flag "this is a
> random MAC, do not use" (if after all, that's what the user wants, such
> as in Michael's case).  And second, yes we probably ought to have
> enforced "mac-address" being the random one we had been using, all
> along.

Why would you expect any "do not use" note?  Locally adminitered MAC
addresses (even when randomly chosen) have been created for good
reason, so they should be usable.

> So no, in sum, I'm not convinced that the best path forward right here
> and now is to put the random MAC in the environment, not correct the now
> incorrect help text and not even poke the binding maintainer nor
> relevant lists to see how exactly they think it would be best to handle
> a locally administered MAC being used as there are both valid use cases
> for "use this in the kernel" and "disregard this in the kernel".

I'm afraid I do not understand what exactly you are proposing here?

But I object to using a MAC address in U-Boot in a way that makes it
invisible to the user who uses documented APIs ("printenv ethaddr").

If we use some MAC address, it shall be possible to read it using
"printenv ethaddr" and to set it using "setenv ethaddr".  Anything
else is inconsistent crap.

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
Success in marriage is not so much finding the right person as it  is
being the right person.


Re: [PATCH] net: uclass: Save ethernet MAC address when generated

2021-11-16 Thread Wolfgang Denk
Dear Tom,

In message <2026184146.GF24579@bill-the-cat> you wrote:
> 
> Because honestly, the more I read this, the more I think
> https://patchwork.ozlabs.org/project/uboot/patch/2025121152.3470910-1-m=
> ich...@walle.cc/
> is essentially the right direction.  There's no reason for 'net list' to
> be using the environment here when ->enetaddr is what's being used by
> the stack.  The use case of "I want to make my locally administered MAC
> persist because my USB ethernet adapter lacks a MAC address" is solved
> via the environment already.

If the MAC address is not placed in the environment, then how can a
user query the currently used MAC address?  All documentation says
basically: run "printenv ethaddr".

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
"Plan to throw one away. You will anyway."
  - Fred Brooks, "The Mythical Man Month"


Re: [PATCH] net: uclass: Save ethernet MAC address when generated

2021-11-16 Thread Wolfgang Denk
Dear Tom,

In message <2026141855.GD24579@bill-the-cat> you wrote:
> 
> So, to quote lib/Kconfig:
> config NET_RANDOM_ETHADDR
> bool "Random ethaddr if unset"
> help
>   Selecting this will allow the Ethernet interface to function
>   even when the ethaddr variable for that interface is unset.
>   A new MAC address will be generated on every boot and it will
>   not be added to the environment.

This description is at least incomplete, because it makes no
difference between the persistent copy of the environment and it's
in-memory copy.  For network to function, I think the MAC address
must be stored in the in-memory copy of the environment.

> We need either a re-spin or follow-up as we're changing the documented
> behavior.  And as I mentioned in the other thread related on-going
> thread, perhaps "ethmacskip" should play a role in preserving existing
> behavior?

We have way too many ways to do the same thing - nearly, just a
little different :-(

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 little knowledge is a dangerous thing."- Doug Gwyn


Re: [PATCH RFC] cmd: fix net list command

2021-11-16 Thread Wolfgang Denk
Dear Tom,

In message <2026141030.GC24579@bill-the-cat> you wrote:
> 
> One thing I'm not totally sure on yet is, looking at the README I see:
> "If Ethernet drivers implement the 'write_hwaddr' function, valid MAC
> addresses will be programmed into hardware as part of the initialization
> process.  This may be skipped by setting the appropriate 'ethmacskip'
> environment variable.  The naming convention is as follows: "ethmacskip"
> (=>eth0), "eth1macskip" (=>eth1) etc."

I have to admit that until now I was not even aware of these
variables...

> As I'm sure that predates device trees being used to the extent they are
> now, should 'ethmacskip' be involved in the "fixup the device tree"
> logic, and appropriate rST / Kconfig help text updated?  My first
> reaction is that in spirit, this is how to solve Michael's use case and
> README / doc/README.enetaddr do not specify when/why the "also fixup the
> device tree if it exists" is done.

Apparently this logic was added more than 10 years ago in commit
ecee9324d "Program net device MAC addresses after initializing".

I think it does not change anything to the logic that Linux uses the
MAC address passed by U-Boot. Whether U-Boot also writes some MAC
address to the device's persistent storage is an independent act.
If there already was an addressed stored there befoire it would not
have been read by Linux, so the same should happen here.

[Here it is even less problematic as U-Boot has the very same MAC
address in it's environment.]

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
Everyting looks interesting until you do it. Then you find it's  just
another job. - Terry Pratchett, _Moving Pictures_


Re: [PATCH RFC] cmd: fix net list command

2021-11-16 Thread Wolfgang Denk
Dear Ramon,

In message  
you wrote:
>
> It appears that Michael has some board and a scenario that *this* bug
> was working for his best interest, where he can have two distinct MAC
> addresses, one in U-boot and one in Linux.

If the Linux environment is supposed to be a specific MAC address
(which may be different from the one passed by U-Boot), then there
are standard tools available to set the address.  I don't see a
problem here.

> Mark and I already provided work around for Michael to delete the
> environment variable before he boots. or implement the necessary OTP
> driver in U-Boot.

As Marek pointed out, in this specific case it would be best to
provide such a driver.

> From my end, I think we can conclude the discussion.

ACK.

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
My challenge to the goto-less programmer  is  to  recode  tcp_input()
without any gotos ... without any loss of efficiency (there has to be
a catch). - W. R. Stevens


Re: [PATCH RFC] cmd: fix net list command

2021-11-16 Thread Wolfgang Denk
Dear Michal,

In message  you wrote:
> 
> In perfect world u-boot will just get it and use. And if manufacturer 
> doesn't do it properly but want to enable ethernet in u-boot random 
> address could be a solution for them.
> And then in Linux where drivers are available you will use the right one.

You miss the point.  The whole argument is about who is control of
the system - can I as user do what I want, or are there some stupid
limitations in the system that prevent me from doing it.

But if the user of the system decides to use a specific MAC address
(for whatever reason ever), the software should just do what he
requests, and use this MAC address on no other one.  It is up to the
user to decide which is "the right one".

> But back to this patch. I think it is good that net list will show all 
> mac addresses even they are not saved in environment.

Agreed.

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 girl with a future avoids the man with a past.
   -- Evan Esar, "The Humor of Humor"


Re: [PATCH RFC] cmd: fix net list command

2021-11-16 Thread Wolfgang Denk
Dear Tom,

In message <2025154516.GR24579@bill-the-cat> you wrote:
> 
> > One practical use case is board provisioning in the factory, which
> > includes setting up valid produt data like serial number, MAC
> > address etc. Yes, this can be done over serial consone as well, but
> > 1) this is slower than network and 2) many commecial products don't
> > have the connector etc. populated.
> 
> Is that the case for the layerscape or imx8 families?  I know on the TI
> side these values are derived from chip-specific fuses that are blown at
> chip production so we don't need an initial random one.  I'd have to
> pull up some NXP manuals to see what's the case there, but I think some
> other people on the thread know the answers here already.

This is actually independent of the hardware, I think.

It is more or less a management decision how board bringup / inital
commissioning is organized.

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 a business, marketroids, salespukes, and  lawyers  have  different
goals from those who actually do work and produce something. Usually,
is  is the former who triumph over the latter, due to the simple rule
that those who print the money make the rules.
 -- Tom Christiansen in <5jdcls$b04$2...@csnews.cs.colorado.edu>


Re: [PATCH RFC] cmd: fix net list command

2021-11-15 Thread Wolfgang Denk
Dear Tom,

In message <2025151956.GN24579@bill-the-cat> you wrote:
> 
> Well, what I mean is, where are the real MAC addresses?  What is the
> rationale for setting NET_RANDOM_ETHADDR?

One practical use case is board provisioning in the factory, which
includes setting up valid produt data like serial number, MAC
address etc. Yes, this can be done over serial consone as well, but
1) this is slower than network and 2) many commecial products don't
have the connector etc. populated.

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 Unix hacks does it take to change a light bulb?  Let's  see,
   can you use a shell script for that or does it need a C program?


Re: [PATCH RFC] cmd: fix net list command

2021-11-15 Thread Wolfgang Denk
Dear Tom,

In message <2025150753.GL24579@bill-the-cat> you wrote:
> 
> It might also be the case that in U-Boot some systems that are using
> NET_RANDOM_ETHADDR that really probably shouldn't?  A quick grep shows
> 253 platforms enabling it today, which feels high for the number of
> boards in the "we'll never have a valid MAC, need a random one"
> category.  Thinking out loud, perhaps we need an option for "no possible
> valid MAC on board" and a different option for "random MAC for fall-back
> only" ?

This is indeed obvious abuse of a feature which has been designed
for a few specific use cases.  It shpould be made clear that this is
nothing to select without thinking.

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 fail to plan, plan to fail.


Re: [PATCH RFC] cmd: fix net list command

2021-11-15 Thread Wolfgang Denk
Dear Michal,

In message <2a51974b-41cf-56e4-c9c9-e6b699f27...@xilinx.com> you wrote:
> 
> As we discussed in previous thread. I think there shouldn't be a problem 
> when u-boot passes random mac address (in whatever way) but if Linux 
> driver know how to get correct one it should be tried first.

No, I strongly disagree here.  If I set a MAC address in U-Boot and
pass it to the kernel, I want that the kernel uses this address.
there may be very good reasons to chose some other address - even if
the "correct" one is known in U-Boot (say, by accident there are
several boards using the same MAC address).

> And it is up to board designer to decide when it is the right time. I 
> can imagine that it can be the part of kernel driver or can be purely 
> done in user space.

Again, no.  It must not be done automatically in the kernel driver,
as this would restrict the user's capabilities to decide what
happens.  If the user want's to change the address in Linux, he can
do this in user space - but again, it is then a (hopufully concious)
decision of the user, and not some automagic code in a driver which
ruins all my attempts to use the address I want to have used.

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
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


Re: [PATCH RFC] cmd: fix net list command

2021-11-15 Thread Wolfgang Denk
Dear Michael,

In message  you wrote:
>
> What is the will of the user in this case?

In which case?  When the user does not bother to set a specific MAC
address and let the system gernerate a random one?  Well it is his
(maybe concious, maybe not) decision...

> It is the will of the
> developer to make the board more robust.

Maybe, maybe not.  Often this is just a convenient method for the
board manufacturer to provision his boards with valid data. If it
makes sense to ship boards in such a state to the end user is
another question.

> > Maybe you explain what exactly the ``error with "net list"'' is,
>
> Michal mentioned it here [1].

Yes, and he also provided a valuable and correct comment:

| And if you don't want to use this feature just don't enable it via
| CONFIG_NET_RANDOM_ETHADDR.

This say all, no patches needed.

> > considering the fact that it is U-Boot which determins the
> > "correct" MAC address and passes it to Linux.  And if the user
> > configures U-Boot such that a random MAC address is used, then this
> > _is_ the correct MAC address, as normally (*) U-Boot is just a tool
> > that does what the user requests.
>
> Only that the user doesn't do it but the board developer/OEM. I.e.
> there is no valid MAC address to pass to linux.

Why do you continue to claim that the MAC address used in U-Boot is
not valid?  Of course it is.


This is similar to the situation where appliances sip with default
passwords like ADMIN/ADMIN.  The user can ignore the documentation
which has bright red warning notes in CAPITAL LETTRRS that he *must*
configure secure passwords...

> It is really just
> for having netconsole running (or maybe you'll need networking for
> to rescue your failed EEPROM or whatever).

This is one of many possible use cases. Board provisioning is
another one.

> I think, we have to distiguish between two use cases here:
>   (1) make networking in u-boot work "somehow", to have a last
>   resort recovery, esp. required for netconsole where
>   the user cannot set the mac address by hand.
>   (2) normal use case, where drivers simply doesn't have any
>   other source for the mac address and need to fall back
>   to a random one.

There are many more use cases.  And it may be intentional to use a
random MAC address.

There is this old rule:

"UNIX was not designed to stop you from doing stupid things,  because
that would also stop you from doing clever things."   - Doug Gwyn


If you don't like it, then disable the feature. It is nonstandard
anyway, i. e. only intended for special cases / qualified users.

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 day-to-day travails of the IBM programmer are so amusing to  most
of us who are fortunate enough never to have been one - like watching
Charlie Chaplin trying to cook a shoe.


Re: [PATCH RFC] cmd: fix net list command

2021-11-15 Thread Wolfgang Denk
Dear Michael,

In message  you wrote:
>
> And again you're masking the error and possible fixes by linux itself.
> Seems like this isn't an argument.

Respecting the explicit will of the user (i. e using what he
configured in U-Boot and passed to the kernel) is not an error.  it
is intended and documented behaviour.

> Sorry, if the original intention was to make "net list" work
> correctly, then why is there now such a huge fuzz around that
> CONFIG_NET_RANDOM_ETHADDR config option and why can't we just
> fix the error with "net list" and leave the behavior like it
> was for years.

Maybe you explain what exactly the ``error with "net list"'' is,
considering the fact that it is U-Boot which determins the
"correct" MAC address and passes it to Linux.  And if the user
configures U-Boot such that a random MAC address is used, then this
_is_ the correct MAC address, as normally (*) U-Boot is just a tool
that does what the user requests.

(*) Exceptions are situations where U-Boot's user interfaces are
locked down for example for security reasons.

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 modem is a baudy house.


Re: [RFC 0/2] Do not stop with an error when mkimage fails

2021-11-11 Thread Wolfgang Denk
Dear Rasmus,

In message <3253160d-b2e1-2101-5cd4-b8549b5ac...@prevas.dk> you wrote:
>
> > Yes, I believe the build system must be taught some env var (or other
> > means) for opting in to this behavior. 
>
> Oh, and it should of course only paper over missing binary blobs, not
> arbitrary errors from mkimage or other tools. The easiest way to do that
> is probably to create some dummy blob(s) [only when CREATE_BROKEN_IMAGES
> is set of course] before calling the tool that will consume the blobs.

Would it not make much more sense that the CI actually creates the
needed data, at least dummy versions of 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
:   I've tried (in vi) "g/[a-z]\n[a-z]/s//_/"...but that doesn't
: cut it.  Any ideas?  (I take it that it may be a two-pass sort of solution).
In the first pass, install perl. :-) Larry Wall <6...@jpl-devvax.jpl.nasa.gov>


Re: [RFC 0/2] Do not stop with an error when mkimage fails

2021-11-11 Thread Wolfgang Denk
Dear Tom,

In message <20211109194224.GB24579@bill-the-cat> you wrote:
> 
> > The only reason I want to introduce this is because I want to have my
> > imx8mq board built by CI. This board needs an external HDMI firmware
> > which is used by mkimage. But because this firmware is not available
> > in the CI build, it comes to the abort. With other boards it is also
> > so that in the CI external blobs are not available and these make
> > nevertheless without error a binman run. In this case only a warning
> > is output.
...
> Unfortunately in these days of needing multiple inputs to create a
> functional image and also needing to have CI be able to be at all
> useful, what we do in many many many cases is yell loudly to the user
> that the resulting file here will NOT work and why.  So yes, some "yell
> it won't work but not return non-zero exit status" is the norm.

This is a terrible degradtion from standard programming style, then.

> I would be very much open however to some way to handle this
> differently.  Some environment variable our tools check for and then
> yell-but-succeed?  Some other idea?  I'm just thinking out loud here.

Well, why not fix the root cause?

Heiko writes that "an external HDMI firmware" is needed - so the fix
is to provide one, or at least a dummy file which is good enough for
the build to succeed.  It should be trivial to create a dummy file
in the CI context, no?

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 little knowledge is a dangerous thing."- Doug Gwyn


Re: [RFC 0/2] Do not stop with an error when mkimage fails

2021-11-11 Thread Wolfgang Denk
Dear Heiko,

In message  
you wrote:
>
> > Sorry, but this makes no sense to me.  If there is an error, it
> > should be reported and - if possible - handled.  If this is not
> > possible, then the correct thing is to abort.  Ignoring errors and
> > trying to continue is always the worst thing to do.
>
> The only reason I want to introduce this is because I want to have my
> imx8mq board built by CI. This board needs an external HDMI firmware
> which is used by mkimage. But because this firmware is not available
> in the CI build, it comes to the abort. With other boards it is also
> so that in the CI external blobs are not available and these make
> nevertheless without error a binman run. In this case only a warning
> is output.
>
> I know this is not a perfect solution but I don't know how to get my
> board merged without doing this kind of workaround for the U-Boot CI.

It's not only not a perfect solution, it is the intentional
introduction of a bug, and thus totally unacceptable.

If there is a file missing in the CI, then add/create it there.

But do remove necessary error handling which would cause hard to
detec failures when for normal use.

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 0/2] Do not stop with an error when mkimage fails

2021-11-07 Thread Wolfgang Denk
Dear Heiko,

In message  
you wrote:
>
> > > While converting to binman for an imx8mq board, it has been found that
> > > building in the u-boot CI fails. This is because an imx8mq requires an
> > > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > > mkimage fails. To work around the problem the exception is caught, an
> > > error message is printed and binman continues.
> >
> > But how can you continue, when mkimage fails and cannot generate the
> > needed image?

Let me rephrase:

How can can you continue, when mkimage fails and cannot
_succesfully_ generate the needed image?

> > In your patch 2/2 we have this:
> >
> > +tools.Run('mkimage', '-d', input_fname, *self._args, 
> > output_fname)
> > +except Exception as e:
> > +tout.Error("mkimage failed: %s" % e)
> > +
> >  self.SetContents(tools.ReadFile(output_fname))
> >
> > mkimage is supposed to create an output file which name is in
> > output_fname; if mkimage fails and you continue, the next step is
> > tools.ReadFile(output_fname) trying to read that file.  How is this
> > possible?
>
> # ls -al mkimage*
> -rw-r--r-- 1 hthiery hthiery  0 Nov  4 20:28 mkimage-out.spl.mkimage
> -rw-r--r-- 1 hthiery hthiery 180392 Nov  4 20:28 mkimage.spl.mkimage
>
> The file (mkimage-out.spl.mkimage) with size 0 seems to be created.  I
> assume mkimage will create that.

So in this situation we know that mkimage failed, and it generated
an empty output file.

I guess the output file is _not_ empty when no errors occur?

which reasonable results can you expect when you ignore an error and
just continue with garbage data as if nothing happened?

Sorry, but this makes no sense to me.  If there is an error, it
should be reported and - if possible - handled.  If this is not
possible, then the correct thing is to abort.  Ignoring errors and
trying to continue is always the worst thing to 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
"I go on working for the same reason a hen goes on laying eggs."
- H. L. Mencken


Re: [RFC 0/2] Do not stop with an error when mkimage fails

2021-11-04 Thread Wolfgang Denk
Dear Heiko,

In message <20211104185231.2927-1-heiko.thi...@gmail.com> you wrote:
> While converting to binman for an imx8mq board, it has been found that
> building in the u-boot CI fails. This is because an imx8mq requires an
> external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> mkimage fails. To work around the problem the exception is caught, an
> error message is printed and binman continues.

But how can you continue, when mkimage fails and cannot generate the
needed image?

In your patch 2/2 we have this:

+tools.Run('mkimage', '-d', input_fname, *self._args, output_fname)
+except Exception as e:
+tout.Error("mkimage failed: %s" % e)
+
 self.SetContents(tools.ReadFile(output_fname))

mkimage is supposed to create an output file which name is in
output_fname; if mkimage fails and you continue, the next step is
tools.ReadFile(output_fname) trying to read that file.  How is this
possible?


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 day," said a dull voice from down below, "I'm going to  be  back
in  form again and you're going to be very sorry you said that. For a
very long time. I might even go so far as to make even more Time just
for you to be sorry in."  - Terry Pratchett, _Small Gods_


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-26 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > > We need the space between the bootargs.
> >
> > Exactly like that, and for the case where you want to append something
> > _without_ an extra space there's the .=3D operator I also suggested.
>
> Do you have a link to the docs for that?
>
> Perhaps we should get this initial thing in and we can take it from
> there. I expect that as we start to convert more environments we'll
> find more things we need.

I think it is not a good idea to use two different operators for the
same appand operation, just to add a space in one case.

So assume I want to start the appended part with a TAB character,
would we define another operator then?

We have problems with escaping characters for the variable _name_
part, but not for the value. We can for example use standard shell
escape rules, like:

foo += bar
foo += \ bar
    foo += ' bar'

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 no statute of limitations on stupidity."
- Randomly produced by a computer program called Markov3.


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-25 Thread Wolfgang Denk
Dear Tom,

In message <20211024164404.GQ3577824@bill-the-cat> you wrote:
> 
> > It is a convenience tool, and it is OK if it has a few restrictions,
> > like for the character set of supported variable names.
> > 
> > But:
> > 
> > 1) These restrictions must be clearly documented, both in the commit
> >message and in the related documentation/readme.
> > 2) There should be another, more primitive way to generate
> >environment settings without these restrictions..
>
> First, in that we don't have tests today for any of the "interesting"
> possible variable options, I have no clue which ones even work as
> intended.
>
> Second, yes, an end result here should be that yes, the default
> environment should be more easily buildable and integrated with
> arbitrary tools, so if something else can parse it (libubootenv?) it can
> be done.

Actually I have an even more low-level approach in mind, like the
capability to include (or rather import) binary U-Boot environment
data given in the usual

=\0...=\0\0

form.  This might come in handy if your data comes from exporting
the environmentof a running system.

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
An expert is a person who avoids the small errors while  sweeping  on
to the grand fallacy.


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-24 Thread Wolfgang Denk
Dear Tom,

In message <20211022144759.GG3577824@bill-the-cat> you wrote:
> 
> > Any escape character is also a legal name character.
>
> I am struggling to have a non-meme reaction to this.  Perhaps the best
> step is just earlier on in the series note that variable names need to
> fit within the broadly and commonly used set of characters and assorted
> funny business you can do historically needs to be migrated.

Indeed I think this is the most reasonable approach.

Like you cannot write any aritrary code in plain C and have to fall
back to assembler in a few places, this patch series should simply
not claim to be able to support all legal environment settings.

It is a convenience tool, and it is OK if it has a few restrictions,
like for the character set of supported variable names.

But:

1) These restrictions must be clearly documented, both in the commit
   message and in the related documentation/readme.
2) There should be another, more primitive way to generate
   environment settings without these restrictions..

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
Whom the gods would destroy, they first teach BASIC.


Re: [PATCH v10 3/9] env: Allow U-Boot scripts to be placed in a .env file

2021-10-24 Thread Wolfgang Denk
Dear Tom,

In message <20211022142912.GF3577824@bill-the-cat> you wrote:
> 
> > However, '\' is also a legal character in a variable name (and
> > doubled backslashes or apostrophes etc. are legal, too), so
> > above line should actually set the environment variable "maximum\+"
> > to "value".
>
> I feel I should preface this with "I am cranky".  Now I see that I can
> indeed do:
> => setenv foo\\bar baz
> => printenv foo\\bar
> foo\bar=baz
>
> on a system today.  To what value, I know not.

That's simple: historical reasons.

I already explained that: when I wrote the environment code, memory
was a very precious resource, so I implemented absolutely no
checking that could be avoided.  '=' was nevessary to separate name
from value, and NUL was necessary to terminate an entry. All other
characters where legal.

Yes, this can be misused to have all kinds of fun, like embedded
terminal control sequences or "invisible" variable names:

=> setenv 'foo^H^H^H' bar
=> printenv
=bar
arch=sandbox
baudrate=115200
...

You don't like it? Don't do it, then.

Yes, robust programming is something different, but at that time we
were fighting for 10 or 20 byte code size - there were so many
systems where U-Boot, Linux, and root file system had to fit into
4 MB flash or 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
Any sufficiently advanced bug is indistinguishable from a feature.
  - Rich Kulawiec


Re: [PATCH v10 3/9] env: Allow U-Boot scripts to be placed in a .env file

2021-10-22 Thread Wolfgang Denk
Dear Simon,

In message 
<20211021210847.v10.3.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
> At present U-Boot environment variables, and thus scripts, are defined
> by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
> to this file and dealing with quoting and newlines is harder than it
> should be. It would be better if we could just type the script into a
> text file and have it included by U-Boot.
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file in a board/
> directory, typically called .env and controlled by the
> CONFIG_ENV_SOURCE_FILE option.
>
> The environment variables should be of the form "var=value". Values can
> extend to multiple lines. See the README under 'Environment Variables:'
> for more information and an example.

The README does not contain this information as it has been moved
into doc/usage/environment.rst

I think the documentation is lacking a hint that multiline
definitions will always be separated by spaces.

> Also support += to allow variables to be appended to. This is needed when
> using the preprocessor.

I cannot see what the preprocessor has to do with this feature.  It
would be useful in any case, even without the preporcessor.


The documentation reads:

"Variables can contain `+` characters but in the unlikely
event that you want to have a variable name ending in plus, put a backslash
before the `+` so that the script knows you are not adding to an existing
variable but assigning to a new one::
 
maximum\+=value
"

However, '\' is also a legal character in a variable name (and
doubled backslashes or apostrophes etc. are legal, too), so
above line should actually set the environment variable "maximum\+"
to "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
Steal five dollars and you were a petty  thief.  Steal  thousands  of
dollars and you are either a government or a hero.
   - Terry Pratchett, _Going_Postal_


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-22 Thread Wolfgang Denk
Dear Tom,

In message <20211021160311.GC3577824@bill-the-cat> you wrote:
> 
> How bad does it make the parser look if we allow trailing + in variable
> names, by escaping them?  It's seemingly the substantive objection at
> this point.

Any escape character is also a legal name character.

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:  How many IBM CPU's does it take to execute a job?
A:  Four; three to hold it down, and one to rip its head off.


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-22 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > > i.e.
> > >   var+=value
> > > appends value to var, while
> > >   var\+=value
> > > sets variable with name "var+"
>
> My first preference is to disallow + at the end of an end var. Perhaps
> we can start printing a warning if people do it, for a few releases.

This might seem to be a harmless change, but it is actually a
fundamental one.  And it breaks backward compatiility.  And all this
without need, as a list of alternatives have been suggested.

> My distance second preference is what Marek has here, using a
> backslash to escape the + character.

Actually this has the same problem, as the backslash is also a legal
character in a variable name:

=> setenv foo\\+ bar
=> printenv foo\\+  
foo\+=bar


Yes, it was probably not a good idea not to restrict the allowed
character set when I implemented this stuff 21 years ago, but then
code size was critical - we had U-Boot running from 128 kB EPROM
(you remember these huge chips which were erased under UV light?).

The fact is, '=' and NUL are the only characters that cannot be used
in a variable name.


> As for =+ ...while I can see how people might parse it (we are setting
> the var equal to what it has with an appending string) I think it is a
> terrible idea as it is just not what people expect.

What do people expect? This is a totally new feature, so people will
use what they find in the documentation and in example code.

> Also, putting the
> + after the = places (similarly unlikely) restrictions on the
> expression.

There is a fundamental difference here.

For the '+=' case, there is no way to escape the '+', as all
commonly used escapes are valid characters in the variable name,
too.

With '=+', the '=' defines where the variable name ends, and from
here you can define your own rules as where the value part begins -
this is just a matter how you implement your parser.

> The current format is basically the same as 'print'. So if I can't
> have the first preference, we could ensure that it prints a \ in the
> case that the var ends with +

But '\' is a legal character in the variable name, too. Anything but
'=' and NUL is a legal char. And this makes escaping impossible:

=> setenv \'foo\\-\' foobar
=> printenv \'foo\\-\'
'foo\-'=foobar

> > Also, I think that it would be better if spaces and tabs were allowed
> > to indent the .env file, i.e.
> >
> > var_a   =   3
> > var_bcd =   7
> >
> > should set "var_a" to "3", "var_bcd" to "7".
> >
> > If special character are needed in either name or value, they could be
> > escaped and/or quoted.
>
> They are allowed in the value but are reduced to a single space in the
> front. We need this for multi-line strings (but I'm a bit worried
> about it).

You mean this automatically insert a newline between parts? ugh...
I didn't realize this. Did I miss it in the documentation?

> We could update it to skip any leading space after the = I think.

So what if you need a leading space?


> I don't like spaces before the = though. It doesn't match the 'print'
> output (which has no space) and it is confusing:

env print also does not add any spaces after the '='.

> I think we need strict rules so it is easy for people to get exactly
> the env they want.

Strict rules, proper documentation, and a set of examples.


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
Any time things appear to be going better, you have overlooked  some-
thing.


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-21 Thread Wolfgang Denk
Dear Marek,

In message <20211021152831.15524883@thinkpad> you wrote:
>
>
> > I think =+ will confuse far more people than + as last character of var

I still fail to see why '=+' could be confusing if properly
documented to be the append operator.

I mean, it is not a new invention of mine.

OpenEmbedded / Yocto uses '=+' a lot, like in

meta/recipes-kernel/dtc/dtc.inc:

PACKAGES =+ "${PN}-misc"

Actually they use both '+=' and '=+', like

RESULT+=${ERRORS}


> > name working weirdly. But I also think that + should be supported as
> > last character. Therefore I propose backslash escaping in variable name,
> > i.e.
> >   var+=value
> > appends value to var, while
> >   var\+=value
> > sets variable with name "var+"

Yes, this is yet another alternative for handling this problem.

> Also, I think that it would be better if spaces and tabs were allowed
> to indent the .env file, i.e.
>
>   var_a   =   3
>   var_bcd =   7
>
> should set "var_a" to "3", "var_bcd" to "7".

Why not...

In the end it boils down that the file format should be properly
defined and have a clear syntax description.  Apparently the
"=\0" format as used in U-Boot persistent storage
should not be taken literally 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
If you're out of tree, you don't exist.
 - David Woodhouse in <1304620350.2398.29.ca...@i7.infradead.org>


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-21 Thread Wolfgang Denk
Dear Tom,

In message <20211021122325.GX7964@bill-the-cat> you wrote:
> 
> Do you have any other feedback on the entire rest of the series?

I already wrote that I support the concept, and the few nit I saw
have been fixed, I think.  Except this unneeded breaking of backward
compatibility.

> Because I'm not sure the benefit of "we can still support '+' at the end
> of a variable name, if anyone uses that" outweighs "we can more easily
> append variables in constructing our environment without relying on
> uncommon operators".

We introduce a new feature here. Defining an append operator is a
convenience thing. It could probably also solved using the
preprocessor, likely in a more ugly way.

In any case, the feature is new, and the operator is new.

For the implementation it does not matter if we define this operator
as "+=" or '=+" or "=." or something else.

the only difference is that any ioperator starting with an equal
sign is inherently backward compatible without need for arbitrary
new restrictions.

> To me "=+" as the append syntax is worse than "no
> + at the end of your variables".

In which way is it worse? For esthetic reasons?

I confirm that '+=' looks better.  But '+=" is technically broken.



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
Brain: an apparatus with which we think we think.- Ambrose Bierce


Re: [PATCH v9 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-21 Thread Wolfgang Denk
Dear Simon,

In message 
<20211019164418.v9.3.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
> At present U-Boot environment variables, and thus scripts, are defined
> by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
> to this file and dealing with quoting and newlines is harder than it
> should be. It would be better if we could just type the script into a
> text file and have it included by U-Boot.
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file in a board/
> directory, typically called .env and controlled by the
> CONFIG_ENV_SOURCE_FILE option.
>
> The environment variables should be of the form "var=value". Values can
> extend to multiple lines. See the README under 'Environment Variables:'
> for more information and an example. Note that variables names may
> not end in + due to the += syntax below.

I still object to placing new, arbitrary restrictions on what may or
may not be used in environment variable names.

We have discussed alternative implementations, and trivial changes
like using "=+" instead of "+=" as append operator do not require
such restictions.

Thus, and only for the restictions on variable names:

Naked-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
There is is no reason for any individual to have a computer in  their
home.  -- Ken Olsen (President of Digital Equipment Corporation),
  Convention of the World Future Society, in Boston, 1977


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-21 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > Wait saying we'll add "+SOMETHING" is a common case?
>
> Yes we have places where we add to env vars depending on CONFIG settings.

Yes, but how many places are there where the appended value starts
with a '+' ?

> The way I have this, is it fairly trivial to convert an existing
> script to a text file. I suspect it can be done automatically but I
> have not actually tried it. I'd really like to keep it simple. I also
> want to invoke the 'if you are not in mainline you don't exist' maxim
> at this point.

In which way is "+=" easier to parse than "=+"?  The only extension
you have to make is for the "=[^+]" case to check the first charater
of the value: if it is a backslash, then ignore it.

This has zero influence on doing this manually or automatic.

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 see that Microsoft's campaign  to  destroy  all  knowledge  of  any
operating   environment   but  its  own  environment-of-the-year  has
succeeded in creating a generation of users who don't understand  the
concept of a shell...
-- L. Peter Deutsch in 


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-21 Thread Wolfgang Denk
Dear Tom,

In message <20211019163000.GI7964@bill-the-cat> you wrote:
> 
> I'm not sure I like saying the operator is "=+" rather than "+=" because 
> "=+" is a less commonly seen operator and tends to be an alternative
> appends for special cases / side-effects / position in parsing.

I fully agree that "+=" would be nicer.  But it has issues, which
"=+" does not have.

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
CONSUMER NOTICE:  Because  of  the  "Uncertainty  Principle,"  It  Is
Impossible  for  the  Consumer  to  Find  Out  at  the Same Time Both
Precisely Where This Product Is and How Fast It Is Moving.


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-21 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > > var++=fred
> > >
> > > is unambiguous but very confusing. I think it would be better to disallow 
> > > +
> >
> > It's neither unambiguous nor confusing.  It is assigning to "var++".
>
> What? Can you read that again?

Did so, but didn't get what you might mean?

In the "" syntax there is no way to define an
operator that starts with a character that is legal at the end o a
name. Especially since usual escape characters like backslash are
also valid characters in a name.

So if you want to maintain the status quo for variable names, you
must use an operator that has no such needs.

> > It is much easier to change what is new and can be defined at will.

i. e. define an operator that is stil trivial to parse (as in an awk
script) and does not clash with name rules.

> > If we define for example that "=+" appends, then we can
> > also define our own escape rules, for example:
> >
> > var=fredassigns
> > var=+fred   appends "fred"
> > var=\+fred  assignes the value "+fred"
> > var=++fred  appends "+fred"
>
> I don't like that at all.

Why not?

Yes, "=+" may be less intuitive than "+=", but then, it's a new
feature, it is esy to use, and it does not clash with any
potentially existing environments.

> It requires an escape for a common case and

Well, I think if appending a value that starts with a '+' charecter
is a "common case", then variable names ending ith a '+' are common
case, too.

And at least my proposal can handle all situations I can think of in
a somewhat reasonable way, and it does not need to place new
restrictions on variable names.

> is very confusing.

Is it?

> Since people will be converting their out-of-tree scripts anyway, they
> can check for this sort of madness at the time. There should be no
> problem.

Agreed.  There should be no problem with my proposal - the last 2 of
the 4 example aboves are pathological situations which will not
happen often.

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 Compatibility:
 your old Windows 3.11 application crash exactly as the new ones.


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> But how do we handle this?
>
> var+=fred
>
> Is this appending to var or assigning to var+  ?

It is assigning to "var+".
>
> var++=fred
>
> is unambiguous but very confusing. I think it would be better to disallow +

It's neither unambiguous nor confusing.  It is assigning to "var++".


I think we should not change what is old and might be in use.

It is much easier to change what is new and can be defined at will.

If we define for example that "=+" appends, then we can
also define our own escape rules, for example:

var=fredassigns
var=+fred   appends "fred"
var=\+fred  assignes the value "+fred"
var=++fred  appends "+fred"

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
Monday is an awful way to spend one seventh of your life.


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> I thought you were trying to use + at the end of a variable.
>
> I used:
>
> fred\+=aaa
>
> and got
>
> cc1: warning: unknown escape sequence: '\=''
>
> You can try it yourself by editing sandbox.env in the
> u-boot-dm/env-working tree.

Hmmm...

-> cat foo.c
#define A ampersand
#define B berta

foo\+=B;
-> gcc -E foo.c
# 0 "foo.c"
# 0 ""
# 0 ""
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "" 2
# 1 "foo.c"



foo\+=berta;



-> cat bar.c
fred\+=aaa
-> gcc -E bar.c
# 0 "bar.c"
# 0 ""
# 0 ""
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "" 2
# 1 "bar.c"
fred\+=aaa


I do not see this problem...

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 don't know if it's what you want, but it's what you get.  :-)
  - Larry Wall in <10...@jpl-devvax.jpl.nasa.gov>


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> Well ideally I'd like to avoid Python in this case as it is in the
> compilation path. I am not sure yet what Wolfgang actually wants,
> apart from variable names ending with + which I would like to
> disallow.
>
> So if we can clearly understand the goal, then we might be able to do
> it in awk, but, again, can we just disallow '+' in var names ?

My goal is to have a parser that does not place new restrictions on
an ancient interface.  The first step would probably be to define a
clear syntax description.

Eventually this would be also more allowing for variations in white
space, so that "=" could also be written as " =
".  which in turn would allow for

foo+ = bar
vs.
foo += bar
.

Just my 2¢...

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 universe is a fragile house of atoms, held together by the mortar
of cause-and-effect. One magician would be two too many.
- Terry Pratchett, _The Dark Side of the Sun_


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Tom,

In message <20211019140711.GC7964@bill-the-cat> you wrote:
> 
> As much as I and others appreciate that you've written the parser here
> in a classic UNIX tool, awk, since a lot of the problems also seem to
> stem from having the parser be able to handle previously valid
> environment variables, if this was written in Python say, would we have
> this problem?

A classic tool would be lex ...


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
Doing the good deeds is like the grass in the garden. You  don't  see
its  growth. But, it does by days. Doing the wicked deeds is like the
hone. You don't see its damage. But, it does by days.- Buddha


Re: [PATCH v8 4/8] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message 
<20211018121315.v8.4.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
>
> The environment variables should be of the form "var=value". Values can
> extend to multiple lines. See the README under 'Environment Variables:'
> for more information and an example. Note that environment variables may
> not end in + 

This makes not really clear that the restriction is on the name (and
not the value) of the environment variable.

> but can start with other strange characters, including
> underscore, comma and slash.

I would omit this as it does not provide any relevant information
here, and in addition it is misleading as it could be interpreted
that such characters are only legal at the start of the variable
name. But you can do:

=> setenv .-^-. foo
=> printenv .-^-.
.-^-.=foo



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 secure program has to be robust: it  must  be  able  to  deal  with
conditions  that "can't happen", whether user input, program error or
library/etc. This is basic damage  control.  Buffer  overflow  errors
have nothing to do with security, but everything with stupidity.
 -- Wietse Venema in <5cnqm3$8...@spike.porcupine.org>


Re: [PATCH v8 0/8] env: Allow environment in text files

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message <20211018181322.1181847-1-...@chromium.org> you wrote:
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file board/.env or
> use CONFIG_ENV_SOURCE_FILE to set a filename.

Again, slash missing.  This should be board//.env  , right?

But that's only the cover letter...

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 don't need a weatherman to know which way the wind blows.
  - Bob Dylan


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> Can we just ban + ?

Why?  Just because your awk script is a lousy parser?

> In the above, foo\+ gives an unknown escape sequence from the C
> preprocessor, then the whole line is ignored by the script

Really?

-> cat foo.c
#define A ampersand
#define B berta

int foo = A \+ B ;
-> gcc -E foo.c
# 0 "foo.c"
# 0 ""
# 0 ""
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "" 2
# 1 "foo.c"



int foo = ampersand \+ berta ;

I do not see any such error message?


> foo+ = bar   produes a variable called "foo+ " in the environment with
> the value " bar" so you probably don't want that.

This is only because your "parser" is very primitive and sensitive
even to minimal white space changes.

> My original motivation was the complexity of getting the env you want
> using #define
>
> My current motivation is to complete the CONFIG migration, now in its 8th 
> year.

I fully understand your motivation and appreciate your efforts.

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
Thought for the day: What if there were no hypothetical situations?


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > Hm... I can't find it right now but did I not also read about other
> > restrictions to variable names, like they must noch begin with '_'
> > when using this new tool?
>
> Yes but I took that out (I think in v6). I'll update the commit message.

Why exactly is this now forbidden, too?

> > I feel it is wrong to place new restrictions on something that was
> > constant for 21 years, just because our parser cannot parse it...
>
> We need the + thing and perhaps we should ask people to avoid
> punctuation, etc? But for now I'm not requiring it in this series,
> apart from +

Punctuation?  Well, we already have ".flags", so at least here we do
have a real life use case.


I really would like to avoid new restictions of variable names,
especially as I cannot see any good reason for it - yes, your awk
script cannot handle this situation, but I tend to belive the fix is
in a better parser than in placing restrictions on the input data.

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
Half of the people in the world are below average.


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-19 Thread Wolfgang Denk
Dear Tom,

In message <20211018142404.GR7964@bill-the-cat> you wrote:
> 
> > > Perhaps we should just make "+" an illegal character in the variable
> > > name, for consistency?
> > 
> > And break backward compatibility?  I'd rather see a better
> > definition of the syntax of the environment files, plus maybe a more
> > powerful parser.
> 
> Are there examples today of scripts that use "+" in the variable names?

None that I know of.

> That maybe someone wrote a custom an private thing that uses + in the
> name isn't the best argument.  Someone saying that did would be better.

Yes, I know.  But then, changing existing APIs is not nice.

> Of course yes, if we can just make the parser handle it, without it also
> being a tricky nightmare, that's the better solution.

Exactly.

> > Hm... I can't find it right now but did I not also read about other
> > restrictions to variable names, like they must noch begin with '_'
> > when using this new tool?
> 
> Any invalid characters need to be clearly documented, if they aren't,
> yes.

So far, only NUL and '=' were impossible to use in a variable name.

> > I feel it is wrong to place new restrictions on something that was
> > constant for 21 years, just because our parser cannot parse it...
> 
> Sure.  But if it's also the case that for 21 years no one has been using
> foo+bar, baz+, etc, in their variable names, maybe we just document
> that's not valid and move on?

We cannot know what people have been using in their environemnts.
Even for those boards that are in mainline, the environment settings
used in real life are often totally different.

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
Save yourself!  Reboot in 5 seconds!


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-18 Thread Wolfgang Denk
Dear Tom,

In message <20211018133728.GQ7964@bill-the-cat> you wrote:
> 
> > And please see also my comments about changing the autostart
> > functionality for the user.
>
> Perhaps we should just make "+" an illegal character in the variable
> name, for consistency?

And break backward compatibility?  I'd rather see a better
definition of the syntax of the environment files, plus maybe a more
powerful parser.

I mean, there is no technical reason to forbid the '+' character -
and then it's only at the end of the variable name.

Hm... I can't find it right now but did I not also read about other
restrictions to variable names, like they must noch begin with '_'
when using this new tool?


I feel it is wrong to place new restrictions on something that was
constant for 21 years, just because our parser cannot parse 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
"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 v7 5/7] doc: Mention CONFIG_DEFAULT_ENV_FILE

2021-10-18 Thread Wolfgang Denk
Dear Simon,

In message <20211016003339.723169-4-...@chromium.org> you wrote:
> Add mention of this option this it does a similar thing to the text
> environment.
>
> Suggested-by: Rasmus Villemoes 
>
> Signed-off-by: Simon Glass 
...

> +The format is the same as accepted by the mkenvimage tool: lines containing
> +key=value pairs, blank lines and lines beginning with # are ignored.

This is misleading. It can be read as: "lines containing key=value
pairs ... are ignored."

This reminds me of a sign in the door of a shop in Cornwall which
read: "No prams or dogs or food being eaten!"  I mean eating dogs is
bad enough, but eating prams? ? ?

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 only one way to have a happy marriage and as soon as I learn
what it is I'll get married again."  - Clint Eastwood


Re: [PATCH v7 3/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-18 Thread Wolfgang Denk
Dear Simon,

In message 
<20211015183321.v7.3.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file in a board//env
> directory called .env (or common.env if you want the same
> environment for all boards).

The cover letter does not mention a separate "env" directory - and
do we really need it?  I will almost always contain just a single
file - so env/.env is an just unnecessary complication.

> Also support += to allow variables to be appended to. This is needed when
> using the preprocessor.
...
> - Explain why variables starting with _ , and / are not supported
...
> - Move use of += to this patch
> - Explain that environment variables may not end in +

I feel all these restrictions should also be mentioned in the commit
message, not only 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
Command, n.:
Statement presented by a human and accepted by a computer
in such a manner as to make the human feel as if he is in control.


Re: [PATCH v7 0/7] env: Allow environment in text files

2021-10-18 Thread Wolfgang Denk
Dear Simon,

In message <20211016003339.723169-1-...@chromium.org> you wrote:
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file board/.env or
> use CONFIG_ENV_SOURCE_FILE to set a filename.

That should be board//.env , right?


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
READ THIS BEFORE OPENING PACKAGE: According to Certain Suggested Ver-
sions of the Grand Unified Theory, the Primary Particles Constituting
this Product May Decay to Nothingness Within the  Next  Four  Hundred
Million Years.


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-18 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > I really think your fixed filename proposal does not work well in
> > reality.  The file name should be Kconfig configurable. See [1]
> > for details.
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2021-October/462668.html
>
> Yes I saw that but I forgot to look at it. I think it makes sense - we
> do that with devicetree, for example.
>
> Is that the only thing holding you back?

Basically yes - the only other concerns I have is about this +=
construct which makes the '+' character an illegal character for
environment variable names, but only when used at the end of the
variable.  This is anything but nice or consistent. Iwonder what
happens with notations like these:

foo+=bar-> "bar" gets appended to current value of "foo"
But what for:
foo\+=bar
or
foo+ = bar

?

And please see also my comments about changing the autostart
functionality for the user.

> I haven't seen any positive comments to this series yet...

Maybe many long-term users of U-Boot don't see the current situation
as such a big problem?  I have no idea.

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
Real Programmers always confuse Christmas and Halloween because
OCT 31 == DEC 25 !  - Andrew Rutherford (andr...@ucs.adelaide.edu.au)


Re: [PATCH v6 7/7] bootm: Tidy up use of autostart env var

2021-10-15 Thread Wolfgang Denk
Dear Simon Glass,

In message <20211014182257.468649-6-...@chromium.org> you wrote:
> This has different semantics in different places. Go with the bootm method
> and put it in a common function so that the behaviour is consistent in
> U-Boot. Update the docs.
>
> Signed-off-by: Simon Glass 
> Suggested-by: Wolfgang Denk 

It should be noted that this commit changes the behaviour of U-Boot
for "autostart" users, thus it has the potential of breaking
existent systems.

The problematic cases are in do_bootelf() [cmd/elf.c] and
do_bootm_standalone() [common/bootm_os.c]; the 3rd place where this
is used - bootm_maybe_autostart() [cmd/bootm.c] - does not change.

Or am I missing something?

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 express preference for a chronological  sequence  of  events  which
precludes a violence.   - Terry Pratchett, _The Dark Side of the Sun_


Re: [PATCH v6 4/7] env: Allow U-Boot scripts to be placed in a .env file

2021-10-15 Thread Wolfgang Denk
Dear Simon Glass,

In message 
<20211014122254.v6.4.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
> At present U-Boot environment variables, and thus scripts, are defined
> by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text
> to this file and dealing with quoting and newlines is harder than it
> should be. It would be better if we could just type the script into a
> text file and have it included by U-Boot.
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file in a board//env
> directory called .env (or common.env if you want the same
> environment for all boards).

Argh... did you bother to read my comments?  Apparently not.

Thus:

NAKed by: Wolfgang Denk 


I really think your fixed filename proposal does not work well in
reality.  The file name should be Kconfig configurable. See [1]
for details.

[1] https://lists.denx.de/pipermail/u-boot/2021-October/462668.html


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
"There's always been Tower of Babel sort of  bickering  inside  Unix,
but  this  is the most extreme form ever. This means at least several
years of confusion." - Bill Gates, founder and chairman of Microsoft,
about the Open Systems Foundation


Re: [PATCH v5 08/10] include/configs: apalis-imx8/verdin-imx8mm: rename kernel image variable

2021-10-09 Thread Wolfgang Denk
Dear Marcel Ziswiler,

In message <20211008002815.870313-9-mar...@ziswiler.com> you wrote:
> From: Oleksandr Suvorov 
>
> Variable "kernel_image" is used in boot.scr script only, that sets its
> own default value to the constant string @@KERNEL_IMAGETYPE@@ in case
> "kernel_image" is not set.
> The default name of the kernel image shipped with BSP 5.x is "Image.gz".
> Setting kernel_image="Image" as a pre-defined u-boot variable
> breaks booting systems with modern versions of boot.scr, whereas
> renaming it fixes booting with modern scripts and does not break working
> of earlier versions of boot.scr.

Same comment as before: please document all changes, i. e. the FDT
related changes.

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
Certainly there are things in life that money  can't  buy,  but  it's
very funny - Did you ever try buying them without money? - Ogden Nash


Re: [PATCH v4 08/10] include/configs: apalis-imx8/verdin-imx8mm: rename kernel image variable

2021-10-09 Thread Wolfgang Denk
Dear Marcel Ziswiler,

In message <20211006212757.464740-9-mar...@ziswiler.com> you wrote:
> From: Oleksandr Suvorov 
>
> Variable "kernel_image" is used in boot.scr script only, that sets its
> own default value to the constant string @@KERNEL_IMAGETYPE@@ in case
> "kernel_image" is not set.
> The default name of the kernel image shipped with BSP 5.x is "Image.gz".
> Setting kernel_image="Image" as a pre-defined u-boot variable
> breaks booting systems with modern versions of boot.scr, whereas
> renaming it fixes booting with modern scripts and does not break working
> of earlier versions of boot.scr.

The patch also changes FST handling without mentioning it in the
description.  Please fix.

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
Bei genauerem Hinsehen ist die  Arbeit  weniger  langweilig  als  das
Vergnügen.  -- Charles Baudelaire


Re: [PATCH v5 3/5] env: Allow U-Boot scripts to be placed in a .env file

2021-10-06 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > 1) This requires that the .env files are run through CPP, which is
> >only added in a later patch.
>
> OK perhaps I should just merge the patches. It is a bit artificial
> having two and it seems that people agree we need the += syntax.

Yes, some way to append to the environment is useful, and using '+='
is an intuitive syntax for it.  I'm just not happy about sneeking in
a new, undocumented restriction on U-Boot variable names.  Yes,
there are probably not many systems around (if any) where a variable
name ends in a plus sign, but we should be consistent.

I have to admit that I don't have any nice alternative suggestion.
If we stick with the rule that only NUL and '=' cannot be used in a
variable name, we could write

variable=+value

instead.  But this does not look as nice as '+=', and we need an
escape mechanism for the case where we want a simple assignment of a
value that starts with a '+'.


BTW: If we would add such a feature to U-Boot (which seems to be no
bad idea to me) we would probably implement an "env append" command?


> > 2) Even if I add an "#include board//env/common.env" in my
> >.env files, your logic would trigger on the existence of
> >the common.env file and ignore the .env files.
>
> OK, so I if reverse that, are you happy? What do you think about my
> explanation above?

The longer I think about it the more I wonder if any hard coded file
names are really a good way to handle this.  For example there might
be the case where we have 4 boards A, B, C and D, and boards A and B
would use one env file, and C and D would use another.  this does
not match the ".env" scheme, and "common.env" does not fit
either, as we have two "common" files.

I wonder if there should rather be a Kconfig option so each board
can select it's env file name; default would be ".env".
what do you think?

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
Life would be so much easier if we could  just  look  at  the  source
code.   -- Dave Olson


Re: [ANN] U-Boot v2021.10 released

2021-10-06 Thread Wolfgang Denk
Dear Matthias,

In message <2738187.1633445...@gemini.denx.de> I wrote:
>
> > Are the script to generate this email available somewhere?
>
> It is...
>
> ...oh, it was.  It's still on our gitlab.denx.de server. It was
> not moved to source.denx.de

It has been on source.denx.de all te time, I just didn't look at the
right place.

See https://source.denx.de/u-boot/gitdm

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 course life is  bizarre.  The  more  bizarre  it  gets,  the  more
interesting  it  is.  The only way to approach it is to make yourself
some popcorn and enjoy the show.  - David Gerrold


Re: [PATCH v5 3/5] env: Allow U-Boot scripts to be placed in a .env file

2021-10-05 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> > > Add a feature that brings in a .env file associated with the board
> > > config, if present. To use it, create a file in a board//env
> > > directory called .env (or common.env if you want the same
> > > environment for all boards).
> >
> > This should be no exclusive "or" here. If a common.env exists, it
> > should be used for all boards, and if additionally one ore more
> > .env files exist, these should ALSO be applied to the
> > respective boards.
>
> Is it not enough to use #include in the main file? We have a similar
> feature with the u-boot.dtsi files and in that case we only choose the
> most specific.

1) This requires that the .env files are run through CPP, which is
   only added in a later patch.
 
2) Even if I add an "#include board//env/common.env" in my
   .env files, your logic would trigger on the existence of
   the common.env file and ignore the .env files.

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
Work 8 hours, sleep 8 hours; but not the same 8 hours.


Re: [ANN] U-Boot v2021.10 released

2021-10-05 Thread Wolfgang Denk
Dear Matthias,

In message <9dd349a3-75ff-ecce-4f4d-957d15a15...@suse.com> you wrote:
>
> I wasn't able to find any commits from Novell email addresses. I wonder if 
> SUSE 
> still maps to Novell, which would be a long-time oversight :)

The fllowing domain names are mapped to Novell:

novell.com  Novell
suse.comNovell
suse.cz Novell
suse.de Novell

> Are the script to generate this email available somewhere?

It is...

...oh, it was.  It's still on our gitlab.denx.de server. It was
not moved to source.denx.de

I will fix this ASAP...


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
Chapter 1 -- The story so  far:
In the beginning the Universe was created. This has  made  a  lot  of
people very angry and been widely regarded as a bad move.


Re: [ANN] U-Boot v2021.10 released

2021-10-05 Thread Wolfgang Denk
  11240 (7.1%)
NXP   10782 (6.8%)
Linaro4905 (3.1%)
Xilinx4608 (2.9%)
Intel 4507 (2.8%)
ST Microelectronics   3472 (2.2%)
...

Employers with the most signoffs (total 325)
Texas Instruments  137 (42.2%)
Xilinx  44 (13.5%)
(Unknown)   39 (12.0%)
BayLibre SAS35 (10.8%)
NXP 15 (4.6%)
Konsulko Group  13 (4.0%)
ARM 10 (3.1%)
Rockchip 6 (1.8%)
Toradex  4 (1.2%)
Novell   4 (1.2%)
...

Employers with the most hackers (total 182)
(Unknown)   95 (52.2%)
Texas Instruments   15 (8.2%)
NXP 12 (6.6%)
Xilinx   9 (4.9%)
Linaro   8 (4.4%)
Rockchip 4 (2.2%)
Toradex  4 (2.2%)
DENX Software Engineering4 (2.2%)
ARM  3 (1.6%)
Intel3 (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
It is easier to write an incorrect program than understand a  correct
one.


Re: [PATCH v5 4/5] env: Allow environment files to use the C preprocessor

2021-10-04 Thread Wolfgang Denk
Dear Simon Glass,

In message 
<20211001183842.v5.4.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
> In many cases environment variables need access to the U-Boot CONFIG
> variables to select different options. Enable this so that the environment
> scripts can be as useful as the ones currently in the board config files.
>
> Also support += to allow variables to be appended to. This is needed when
> using the preprocessor.
>
> Signed-off-by: Simon Glass 
> ---
>
> (no changes since v4)
>
> Changes in v4:
> - Add documentation in rST format instead of README
> - Move use of += to this patch
> - Explain that environment variables may not end in +

Sorry, I disagree here.  There was intentionally only very little
restrictions on what a environment variable name should look like -
the only exceptions were the '=' and the NUL characters.

Adding artificial restrictions now just to enable your custom
notation for appending seems not acceptable to me.  You might want
to chose a different notation or implement a proper parser instead.

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
Never call a man a fool.  Borrow from him.


Re: [PATCH v5 3/5] env: Allow U-Boot scripts to be placed in a .env file

2021-10-04 Thread Wolfgang Denk
Dear Simon,

In message 
<20211001183842.v5.3.If789ba3e2667c46c03eda3386ca84a863baeda55@changeid> you 
wrote:
>
> Add a feature that brings in a .env file associated with the board
> config, if present. To use it, create a file in a board//env
> directory called .env (or common.env if you want the same
> environment for all boards).

This should be no exclusive "or" here. If a common.env exists, it
should be used for all boards, and if additionally one ore more
.env files exist, these should ALSO be applied to the
respective boards.

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
Save yourself!  Reboot in 5 seconds!


Re: [PATCH v5 2/5] doc: Move environment documentation to rST

2021-10-04 Thread Wolfgang Denk
Dear Simon,

In message <20211002003848.1803446-3-...@chromium.org> you wrote:
>
> +baudrate
> +see CONFIG_BAUDRATE
> +
> +bootdelay
> +see CONFIG_BOOTDELAY
> +
> +bootcmd
> +see CONFIG_BOOTCOMMAND

I know you only copied this, but the text is actually not helpful,
as it does not even mention where to look for documentation for
these CONFIG_* options

> +autoload
> +if set to "no" (any string beginning with 'n'),
> +"bootp" will just load perform a lookup of the
> +configuration from the BOOTP server, but not try to
> +load any image using TFTP

We should add "dhcp" here, too.

> +autostart
> +if set to "yes", an image loaded using the "bootp",
> +"rarpboot", "tftpboot" or "diskboot" commands will
> +be automatically started (by internally calling
> +"bootm")
> +
> +If set to "no", a standalone image passed to the
> +"bootm" command will be copied to the load address
> +(and eventually uncompressed), but NOT be started.
> +This can be used to load and uncompress arbitrary
> +data.

There is a consistency problem here.

In "bootm" (cmd/bootm.c), the "yes" part applies - any other value
or the variable not being defined omitting the call of do_bootm().

This also applies to nandboot, btw.

In "bootelf" (cmd/elf.c), the "no" part applies - any other value or
the variable not being defined will cause do_bootelf_exec() to be
called.

This should be both documented and fixed.

> +ipaddr
> +IP address; needed for tftpboot command

...and for ping etc.

> +vlan
> +When set to a value < 4095 the traffic over
> +Ethernet is encapsulated/received over 802.1q
> +VLAN tagged frames.

Really?  I can't see where "vlan" is actually used in the code.
[I looked for it because I could not remember if the values is
interpreted as decimal or hex number...]


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
Systems programmers are the high priests of a low cult.
   -- R.S. Barton


Re: [PATCH v2] cmd: ubi: add a command to swap volumes

2021-09-28 Thread Wolfgang Denk
Dear Ayoub,

In message <20210909115257.14147-1-ayoub.z...@embexus.com> you wrote:
> This commit adds the command ubi swap to swap an ubi volumes.
> The format of the command is: ubi swap  .
> To enable this command, the option CMD_UBI_SWAPVOL must be selected.

May I ask what this command is good for, i. e. what is the intended
use case?

I have the feeling that you might want to do something which
requires an atomic operation, but you should be aware that the
implementation is NOT atomic - even worse: in case of problems there
is not even a way to fall back to the old state.

So if you think to use this for example for reliable software
updates, then you have a big problem here.

Or am I missing something?

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
Niklaus Wirth has lamented that, whereas Europeans pronounce his name
correctly  (Ni-klows  Virt),  Americans  invariably  mangle  it  into
(Nick-les  Worth).  Which  is to say that Europeans call him by name,
but Americans call him by value.


Re: [PATCH] ARM: omap3_logic: Cleanup usage of MUX_VAL

2021-09-28 Thread Wolfgang Denk
Dear Adam Ford,

In message <20210927183030.506532-1-aford...@gmail.com> you wrote:
> The macro called MUX_VAL generates a writel instruction with
> semicolon at the end.  This table was written to use semicolons,
> however one was missed:
>
>MUX_VAL(CP(SYS_BOOT2),  (IEN  | PTD | DIS | M4))/* GPIO_4 */
>
> Since the extra semicolon is unnecessary with the use of the macro,
> remove all of them.

Thanks.


> + MUX_VAL(CP(MCSPI2_SOMI),(IEN  | PTD | DIS | M0))
> /*HSUSB2_DATA5*/
> + MUX_VAL(CP(MCSPI2_CS0), (IEN  | PTD | EN  | M0))
> /*HSUSB2_DATA6*/
> + MUX_VAL(CP(MCSPI2_CLK), (IEN  | PTD | DIS | M0))
> /*HSUSB2_DATA7*/
>   MUX_VAL(CP(SYS_BOOT2),  (IEN  | PTD | DIS | M4))/* GPIO_4 */

This line stands out because indentation is different from all the
other lines.  You might want to fix that, too, while we are at it?

Reviewd-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
When a program is being  tested,  it  is  too  late  to  make  design
changes.  -- Geoffrey James, "The Tao of Programming"


Re: Bug in board/logicpd/omap3som/omap3logic.h ???

2021-09-27 Thread Wolfgang Denk
Dear Adam,

In message  
you wrote:
> > >
> > > commit 25e4ff45b17 "ARM: omap3_logic: Enable OMAP EHCI support for
> > > SOM-LV Boards" added (among other things) these lines:
> > >
> > > ...
> > > 243 MUX_VAL(CP(MCSPI2_SOMI),(IEN  | PTD | DIS | M0)); 
> > >   /*HSUSB2_DATA5*/
> > > 244 MUX_VAL(CP(MCSPI2_CS0), (IEN  | PTD | EN  | M0)); 
> > >   /*HSUSB2_DATA6*/
> > > 245 MUX_VAL(CP(MCSPI2_CLK), (IEN  | PTD | DIS | M0)); 
> > >   /*HSUSB2_DATA7*/
> > > 246 MUX_VAL(CP(SYS_BOOT2),  (IEN  | PTD | DIS | M4))/* 
> > > GPIO_4 */
> > > 247 MUX_VAL(CP(ETK_D10_ES2),(IDIS | PTU | DIS | M3)); 
> > >   /*HSUSB2_CLK*/
> > > 248 MUX_VAL(CP(ETK_D11_ES2),(IDIS | PTU | DIS | M3)); 
> > >   /*HSUSB2_STP*/
> > > 249 MUX_VAL(CP(ETK_D12_ES2),(IEN  | PTU | DIS | M3)); 
> > >   /*HSUSB2_DIR*/
> > > ...
> > >
> > > All the entries in set_muxconf_regs() have a terminating semicolon -
> > > all except the entry in line # 246.
> > >
> > > I reckon this is a mistake and should be fixed?
> >
> > Interesting.  MUX_VAL is from arch/arm/include/asm/arch-omap3/mux.h and
> > includes a semicolon, so that's how this didn't trip anything up.  Most
> > of the time it's not used with one either, from a quick grep.
>
> I am guessing it was left out by accident.  I would not have
> intentionally done that.  I can submit a patch to make it consistent
> if you want.

Yes, it should be made xonsistent, please - otherwise everybody
seeing this wonders what is going on here.

Tom says there is already a semicolon in the macro and most use
cases don;t have one, so the fix should probably be to keep that
line and remove the other semicolons ?

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 human race is faced with a cruel choice: work  or  daytime  tele-
vision.


[PATCH 1/4] WS cleanup: remove trailing empty lines

2021-09-27 Thread Wolfgang Denk
Signed-off-by: Wolfgang Denk 
---
 arch/arm/cpu/arm926ejs/cache.c| 1 -
 arch/arm/cpu/armv7/psci-common.c  | 1 -
 arch/arm/cpu/armv8/hisilicon/pinmux.c | 2 --
 arch/arm/cpu/armv8/xen/hypercall.S| 1 -
 arch/arm/dts/sama7g5-pinfunc.h| 1 -
 arch/arm/include/asm/arch-imxrt/imxrt.h   | 1 -
 arch/arm/include/asm/arch-rockchip/f_rockusb.h| 1 -
 arch/arm/include/asm/arch-stm32/stm32f.h  | 1 -
 arch/arm/include/asm/arch-stv0991/stv0991_defs.h  | 1 -
 arch/arm/include/asm/xen.h| 1 -
 arch/arm/lib/ccn504.S | 1 -
 arch/arm/mach-at91/armv7/sama7g5_devices.c| 1 -
 arch/arm/mach-at91/atmel_sfr.c| 1 -
 arch/arm/mach-bcm283x/msg.c   | 1 -
 arch/arm/mach-imx/mx7/soc.c   | 1 -
 arch/arm/mach-kirkwood/cpu.c  | 1 -
 arch/arm/mach-octeontx/Makefile   | 1 -
 arch/arm/mach-octeontx2/Makefile  | 1 -
 arch/arm/mach-omap2/pipe3-phy.c   | 1 -
 arch/arm/mach-snapdragon/dram.c   | 1 -
 arch/arm/mach-tegra/tegra20/display.c | 1 -
 arch/arm/mach-versatile/timer.c   | 1 -
 arch/m68k/cpu/mcf530x/Makefile| 1 -
 arch/m68k/include/asm/immap_5307.h| 1 -
 arch/m68k/include/asm/m5307.h | 1 -
 arch/mips/mach-jz47xx/include/mach/jz4780_dram.h  | 1 -
 arch/powerpc/cpu/mpc85xx/fsl_corenet2_serdes.c| 1 -
 arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c | 1 -
 arch/powerpc/lib/ppccache.S   | 1 -
 arch/sh/lib/time.c| 1 -
 arch/xtensa/include/asm/arch-dc232b/core.h| 1 -
 arch/xtensa/include/asm/arch-dc232b/tie-asm.h | 1 -
 arch/xtensa/include/asm/arch-dc232b/tie.h | 1 -
 arch/xtensa/include/asm/arch-dc233c/core.h| 1 -
 arch/xtensa/include/asm/arch-dc233c/tie-asm.h | 1 -
 arch/xtensa/include/asm/arch-dc233c/tie.h | 1 -
 arch/xtensa/include/asm/arch-de212/core.h | 1 -
 arch/xtensa/include/asm/arch-de212/tie-asm.h  | 1 -
 arch/xtensa/include/asm/arch-de212/tie.h  | 1 -
 arch/xtensa/include/asm/regs.h| 1 -
 arch/xtensa/lib/bootm.c   | 1 -
 arch/xtensa/lib/relocate.c| 1 -
 board/Marvell/octeontx/smc.c  | 1 -
 board/Marvell/octeontx2/soc-utils.c   | 1 -
 board/atmark-techno/armadillo-800eva/Makefile | 1 -
 board/atmel/sama7g5ek/sama7g5ek.c | 1 -
 board/beacon/imx8mn/README| 1 -
 board/beacon/imx8mn/lpddr4_timing.c   | 1 -
 board/boundary/nitrogen6x/README.mx6qsabrelite| 1 -
 board/cadence/xtfpga/README   | 1 -
 board/compulab/cm_t43/spl.c   | 1 -
 .../imx8mm-cl-iot-gate/ddr/lpddr4_timing_01061010.1_2.c   | 1 -
 board/freescale/common/zm7300.c   | 1 -
 board/freescale/imx8mn_evk/ddr4_timing.c  | 1 -
 board/freescale/imx8mn_evk/ddr4_timing_ld.c   | 1 -
 board/freescale/ls2080aqds/README | 1 -
 board/freescale/ls2080ardb/README | 1 -
 board/freescale/mx6memcal/mx6memcal.c | 1 -
 board/freescale/t4rdb/cpld.h  | 1 -
 board/gateworks/gw_ventana/Makefile   | 1 -
 board/gateworks/gw_ventana/gsc.h  | 1 -
 board/gdsys/a38x/dt_helpers.c | 1 -
 board/ge/common/ge_rtc.c  | 1 -
 board/ge/mx53ppd/mx53ppd_video.c  | 1 -
 board/hisilicon/poplar/poplar.c   | 1 -
 board/keymile/common/qrio.c   | 1 -
 board/logicpd/imx6/Makefile   | 1 -
 board/logicpd/imx6/README | 1 -
 board/mediatek/mt7622/Makefile| 1 -
 board/mqmaker/miqi_rk3288/miqi-rk3288.c   | 1 -
 board/mscc/jr2/Makefile   | 1 -
 board/mscc/ocelot/Makefile

[PATCH 3/4] WS cleanup: remove trailing white space

2021-09-27 Thread Wolfgang Denk
Signed-off-by: Wolfgang Denk 
---
 Licenses/lgpl-2.0.txt |  2 +-
 Makefile  |  4 ++--
 arch/arm/include/asm/arch-am33xx/cpu.h|  2 +-
 arch/arm/lib/lib1funcs.S  |  6 +++---
 arch/xtensa/include/asm/arch-de212/core.h |  2 +-
 board/freescale/common/sys_eeprom.c   |  2 +-
 .../dragonboard820c/dragonboard820c.c |  6 +++---
 board/warp7/README|  2 +-
 common/spl/spl_mmc.c  |  2 +-
 doc/README.distro |  2 +-
 doc/imx/common/imx6.txt   |  2 +-
 drivers/misc/atsha204a-i2c.c  |  2 +-
 drivers/mtd/mtdcore.c |  4 ++--
 drivers/mtd/nand/raw/nand_base.c  |  2 +-
 drivers/mtd/nand/raw/nand_util.c  |  2 +-
 drivers/mtd/ubi/build.c   |  2 +-
 drivers/net/phy/meson-gxl.c   |  2 +-
 drivers/ram/stm32_sdram.c |  2 +-
 drivers/reset/reset-meson.c   | 10 +-
 drivers/serial/serial_msm.c   |  2 +-
 drivers/usb/dwc3/dwc3-meson-gxl.c |  2 +-
 drivers/usb/musb-new/musb_regs.h  |  2 +-
 drivers/usb/musb-new/musb_uboot.c |  2 +-
 drivers/usb/musb/musb_hcd.c   |  2 +-
 drivers/video/anx9804.c   | 10 +-
 drivers/video/tdo-tl070wsh30.c|  2 +-
 include/configs/imx6-engicam.h|  2 +-
 include/linux/mtd/rawnand.h   |  2 +-
 include/linux/serial_reg.h|  4 ++--
 scripts/kconfig/gconf.c   |  2 +-
 tools/asn1_compiler.c |  2 +-
 tools/patman/test/test01.txt  | 20 +--
 32 files changed, 56 insertions(+), 56 deletions(-)

diff --git a/Licenses/lgpl-2.0.txt b/Licenses/lgpl-2.0.txt
index 5bc8fb2c8f..12735e6c21 100644
--- a/Licenses/lgpl-2.0.txt
+++ b/Licenses/lgpl-2.0.txt
@@ -133,7 +133,7 @@ such a program is covered only if its contents constitute a 
work based
 on the Library (independent of the use of the Library in a tool for
 writing it).  Whether that is true depends on what the Library does
 and what the program that uses the Library does.
-  
+
   1. You may copy and distribute verbatim copies of the Library's
 complete source code as you receive it, in any medium, provided that
 you conspicuously and appropriately publish on each copy an
diff --git a/Makefile b/Makefile
index 732d702947..e2f5d47d1f 100644
--- a/Makefile
+++ b/Makefile
@@ -327,14 +327,14 @@ os_x_before   = $(shell if [ $(DARWIN_MAJOR_VERSION) 
-le $(1) -a \
$(DARWIN_MINOR_VERSION) -le $(2) ] ; then echo "$(3)"; else echo 
"$(4)"; fi ;)
 
 os_x_after = $(shell if [ $(DARWIN_MAJOR_VERSION) -ge $(1) -a \
-   $(DARWIN_MINOR_VERSION) -ge $(2) ] ; then echo "$(3)"; else echo 
"$(4)"; fi ;)  
+   $(DARWIN_MINOR_VERSION) -ge $(2) ] ; then echo "$(3)"; else echo 
"$(4)"; fi ;)
 
 # Snow Leopards build environment has no longer restrictions as described above
 HOSTCC   = $(call os_x_before, 10, 5, "cc", "gcc")
 KBUILD_HOSTCFLAGS  += $(call os_x_before, 10, 4, "-traditional-cpp")
 KBUILD_HOSTLDFLAGS += $(call os_x_before, 10, 5, "-multiply_defined suppress")
 
-# macOS Mojave (10.14.X) 
+# macOS Mojave (10.14.X)
 # Undefined symbols for architecture x86_64: "_PyArg_ParseTuple"
 KBUILD_HOSTLDFLAGS += $(call os_x_after, 10, 14, "-lpython -dynamclib", "")
 endif
diff --git a/arch/arm/include/asm/arch-am33xx/cpu.h 
b/arch/arm/include/asm/arch-am33xx/cpu.h
index 79081de700..b33e6f7fd1 100644
--- a/arch/arm/include/asm/arch-am33xx/cpu.h
+++ b/arch/arm/include/asm/arch-am33xx/cpu.h
@@ -408,7 +408,7 @@ struct cm_dpll {
unsigned int resv1;
unsigned int clktimer2clk;  /* offset 0x04 */
unsigned int resv2[11];
-   unsigned int clkselmacclk;  /* offset 0x34 */ 
+   unsigned int clkselmacclk;  /* offset 0x34 */
 };
 #endif /* CONFIG_AM43XX */
 
diff --git a/arch/arm/lib/lib1funcs.S b/arch/arm/lib/lib1funcs.S
index 0798d098af..700eee5fbb 100644
--- a/arch/arm/lib/lib1funcs.S
+++ b/arch/arm/lib/lib1funcs.S
@@ -34,7 +34,7 @@
mov \divisor, \divisor, lsl \result
mov \curbit, \curbit, lsl \result
mov \result, #0
-   
+
 #else
 
@ Initially shift the divisor left 3 bits if possible,
@@ -48,7 +48,7 @@
 
@ Unless the divisor is very big, shift it up in multiples of
@ four bits, since this is the amount of unwinding in the main
-   @ division loop.  Continue shifting until the divisor is 
+   @ division loop.  Continue shifting until the divisor is
@ larger than the dividend.
 1: cmp \divisor, #0x1000
cmplo   \divisor

[PATCH 2/4] WS cleanup: remove excessive empty lines

2021-09-27 Thread Wolfgang Denk
Signed-off-by: Wolfgang Denk 
---
 README| 1 -
 arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c | 2 --
 arch/xtensa/include/asm/arch-dc232b/core.h| 2 --
 arch/xtensa/include/asm/arch-dc232b/tie-asm.h | 4 
 arch/xtensa/include/asm/arch-dc233c/core.h| 3 ---
 arch/xtensa/include/asm/arch-dc233c/tie-asm.h | 4 
 arch/xtensa/include/asm/arch-de212/core.h | 3 ---
 arch/xtensa/include/asm/cacheasm.h| 2 --
 board/freescale/ls1088a/ddr.c | 2 --
 board/freescale/t208xqds/README   | 1 -
 board/xilinx/zynq/zynq-microzed/ps7_init_gpl.c| 4 
 board/xilinx/zynq/zynq-zc702/ps7_init_gpl.c   | 4 
 board/xilinx/zynq/zynq-zc706/ps7_init_gpl.c   | 4 
 board/xilinx/zynq/zynq-zed/ps7_init_gpl.c | 4 
 cmd/bedbug.c  | 4 
 doc/README.dfutftp| 2 --
 doc/device-tree-bindings/firmware/linaro,optee-tz.txt | 1 -
 drivers/gpio/hi6220_gpio.c| 2 --
 drivers/i2c/mxc_i2c.c | 2 --
 drivers/mtd/nand/raw/nand_util.c  | 2 --
 drivers/net/e1000.c   | 4 
 drivers/video/stb_truetype.h  | 1 -
 fs/btrfs/kernel-shared/btrfs_tree.h   | 1 -
 include/configs/etamin.h  | 1 -
 include/fsl-mc/fsl_dpni.h | 2 --
 include/linux/mtd/flashchip.h | 1 -
 include/linux/serial_reg.h| 1 -
 27 files changed, 64 deletions(-)

diff --git a/README b/README
index a3f81e4aed..91bf2a4e30 100644
--- a/README
+++ b/README
@@ -300,7 +300,6 @@ board_init_r():
- loads U-Boot or (in falcon mode) Linux
 
 
-
 Configuration Options:
 --
 
diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c 
b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
index 3b41c7d49b..bb7d24b4b7 100644
--- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
+++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
@@ -14,8 +14,6 @@
 #include "sys_env_lib.h"
 #include "ctrl_pex.h"
 
-
-
 /*
  * serdes_seq_db - holds all serdes sequences, their size and the
  * relevant index in the data array initialized in serdes_seq_init
diff --git a/arch/xtensa/include/asm/arch-dc232b/core.h 
b/arch/xtensa/include/asm/arch-dc232b/core.h
index 92ea0dfe35..c1453f719e 100644
--- a/arch/xtensa/include/asm/arch-dc232b/core.h
+++ b/arch/xtensa/include/asm/arch-dc232b/core.h
@@ -127,8 +127,6 @@
 #define XCHAL_DCACHE_IS_WRITEBACK  1   /* writeback feature */
 
 
-
-
 /
 Parameters Useful for PRIVILEGED (Supervisory or Non-Virtualized) Code
  /
diff --git a/arch/xtensa/include/asm/arch-dc232b/tie-asm.h 
b/arch/xtensa/include/asm/arch-dc232b/tie-asm.h
index 7003cad40d..35a26dca7c 100644
--- a/arch/xtensa/include/asm/arch-dc232b/tie-asm.h
+++ b/arch/xtensa/include/asm/arch-dc232b/tie-asm.h
@@ -26,7 +26,6 @@
 #define XTHAL_SAS_ALL  0x  /* include all default NCP contents */
 
 
-
 /* Macro to save all non-coprocessor (extra) custom TIE and optional state
  * (not including zero-overhead loop registers).
  * Save area ptr (clobbered):  ptr  (1 byte aligned)
@@ -109,11 +108,8 @@
.endif
.endm   // xchal_ncp_load
 
-
-
 #define XCHAL_NCP_NUM_ATMPS2
 
-
 #define XCHAL_SA_NUM_ATMPS 2
 
 #endif /*_XTENSA_CORE_TIE_ASM_H*/
diff --git a/arch/xtensa/include/asm/arch-dc233c/core.h 
b/arch/xtensa/include/asm/arch-dc233c/core.h
index ca07d8ee21..4646cdbfb4 100644
--- a/arch/xtensa/include/asm/arch-dc233c/core.h
+++ b/arch/xtensa/include/asm/arch-dc233c/core.h
@@ -149,13 +149,10 @@
 #define XCHAL_HAVE_PREFETCH0   /* PREFCTL register */
 
 
-
-
 /
 Parameters Useful for PRIVILEGED (Supervisory or Non-Virtualized) Code
  /
 
-
 #ifndef XTENSA_HAL_NON_PRIVILEGED_ONLY
 
 /*--
diff --git a/arch/xtensa/include/asm/arch-dc233c/tie-asm.h 
b/arch/xtensa/include/asm/arch-dc233c/tie-asm.h
index 317d4e1312..7b3d1f3c57 100644
--- a/arch/xtensa/include/asm/arch-dc233c/tie-asm.h
+++ b/arch/xtensa/include/asm/arch-dc233c/tie-asm.h
@@ -31,8 +31,6 @@
| ((ccuse) & XTHAL_SAS_ANYCC)  \
| ((abi)   & XTHAL_SAS_ANYABI) )
 
-
-
 /*
  *  Macro to save all non-coprocessor (extra) custom TIE and optional state
  *  (not 

[PATCH 0/4] White Space cleanup

2021-09-27 Thread Wolfgang Denk
The following is an attempt of a minimal-invasive white space
cleaup.  It performs the following actions:

- Remove empty lines at the end of a file
- Remove white space at the end of lines
- Remove excessive empty lines: there is almost never a good reason
  to have more than 2 consecutive empty lines in a file
- Remove SPACE characters that are followed by TAB characters.

Care has been taken not to:
- change any actual code
- change the formatting of the files (assuming you use a monospace font);
  no attempts were made to clean up broken formatting.
- change any files that were imported from other projects, where we
  might want to re-import more recent versions; for imports from the
  Linux kernel, current Linux versions were taken as reference, so
  for example some files in scripts/kconfig were fixed to fix
  differences from the Linux versions.

This is all purely cosmetic.  No code changes result from this.


I am aware that such a global cosmetic cleanup is always kind of
disruptive, but we are at -rc5, so the code should be pretty stable
for now, and I've done a few rebases before, with little and easy to
fix conflicts.


Wolfgang Denk (4):
  WS cleanup: remove trailing empty lines
  WS cleanup: remove excessive empty lines
  WS cleanup: remove trailing white space
  WS cleanup: remove SPACE(s) followed by TAB

 Licenses/lgpl-2.0.txt |   2 +-
 Makefile  |   6 +-
 README|   1 -
 arch/arc/lib/libgcc2.h|   2 +-
 arch/arm/cpu/arm926ejs/armada100/timer.c  |   2 +-
 arch/arm/cpu/arm926ejs/cache.c|   1 -
 arch/arm/cpu/armv7/psci-common.c  |   1 -
 arch/arm/cpu/armv8/fel_utils.S|  20 +-
 .../cpu/armv8/fsl-layerscape/doc/README.lsch3 |  36 +-
 arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S  |   4 +-
 arch/arm/cpu/armv8/hisilicon/pinmux.c |   2 -
 arch/arm/cpu/armv8/xen/hypercall.S|   1 -
 arch/arm/dts/sama7g5-pinfunc.h|   1 -
 arch/arm/dts/vf610-pinfunc.h  |   2 +-
 arch/arm/include/asm/arch-am33xx/cpu.h|   2 +-
 arch/arm/include/asm/arch-armada100/mfp.h |   2 +-
 arch/arm/include/asm/arch-imxrt/imxrt.h   |   1 -
 arch/arm/include/asm/arch-mx25/imx-regs.h |  22 +-
 arch/arm/include/asm/arch-mx5/imx-regs.h  |   4 +-
 arch/arm/include/asm/arch-mx6/mx6_plugin.S|   4 +-
 arch/arm/include/asm/arch-mx7/mx7_plugin.S|   2 +-
 .../include/asm/arch-mx7ulp/mx7ulp_plugin.S   |   2 +-
 .../include/asm/arch-rockchip/cru_rk3368.h|  16 +-
 .../arm/include/asm/arch-rockchip/f_rockusb.h |   1 -
 arch/arm/include/asm/arch-stm32/stm32f.h  |   1 -
 .../include/asm/arch-stv0991/stv0991_defs.h   |   1 -
 arch/arm/include/asm/arch-vf610/iomux-vf610.h |   8 +-
 arch/arm/include/asm/macro.h  |   2 +-
 arch/arm/include/asm/ti-common/davinci_nand.h |   6 +-
 arch/arm/include/asm/xen.h|   1 -
 arch/arm/lib/ccn504.S |   3 +-
 arch/arm/lib/div64.S  |  10 +-
 arch/arm/lib/lib1funcs.S  |   6 +-
 arch/arm/mach-at91/armv7/sama7g5_devices.c|   1 -
 arch/arm/mach-at91/atmel_sfr.c|   1 -
 arch/arm/mach-at91/include/mach/at91_mc.h |  24 +-
 arch/arm/mach-at91/include/mach/at91_st.h |   2 +-
 arch/arm/mach-bcm283x/msg.c   |   1 -
 .../arm/mach-davinci/include/mach/da8xx-usb.h |  36 +-
 .../mach-davinci/include/mach/davinci_misc.h  |   2 +-
 arch/arm/mach-imx/Makefile|   4 +-
 arch/arm/mach-imx/mx7/soc.c   |   1 -
 .../arm/mach-keystone/include/mach/hardware.h |   6 +-
 arch/arm/mach-kirkwood/cpu.c  |   1 -
 .../serdes/a38x/high_speed_env_spec.c |   2 -
 arch/arm/mach-octeontx/Makefile   |   1 -
 arch/arm/mach-octeontx2/Makefile  |   1 -
 arch/arm/mach-omap2/clocks-common.c   |   2 +-
 arch/arm/mach-omap2/omap5/prcm-regs.c |   2 +-
 arch/arm/mach-omap2/pipe3-phy.c   |   1 -
 arch/arm/mach-orion5x/timer.c |   2 +-
 arch/arm/mach-rmobile/pfc-r8a7790.h   |   2 +-
 arch/arm/mach-rockchip/rk3368/Makefile|   2 +-
 arch/arm/mach-s5pc1xx/include/mach/sromc.h|   4 +-
 arch/arm/mach-snapdragon/dram.c   |   1 -
 .../include/mach/fpga_manager_arria10.h   |  16 +-
 arch/arm/mach-sunxi/dram_sun4i.c  |   2 +-
 arch/arm/mach-sunxi/dram_sun8i_a33.c  |   4 +-
 .../mach-sunxi/dram_timings/h6_ddr3_1333.c|   2 +-
 arch/arm/mach-tegra/tegra20/display.c |   1 -
 arch/arm/mach-versatile/timer.c   |   1 -
 arch/m68k/cpu/mcf5227x/start.S|   4 +-
 arch/m68k/cpu/mcf523x/cpu.c   |   2 +-
 arch/m68k/cpu/mcf52x2/cpu.c   |   2 +-
 arch/m68k/cpu/mcf530x/Makefile|   1 -
 arch/m68k/cpu/mcf532x/cpu.c

Bug in board/logicpd/omap3som/omap3logic.h ???

2021-09-27 Thread Wolfgang Denk
Dear Adam,

commit 25e4ff45b17 "ARM: omap3_logic: Enable OMAP EHCI support for
SOM-LV Boards" added (among other things) these lines:

...
243 MUX_VAL(CP(MCSPI2_SOMI),(IEN  | PTD | DIS | M0));   
/*HSUSB2_DATA5*/
244 MUX_VAL(CP(MCSPI2_CS0), (IEN  | PTD | EN  | M0));   
/*HSUSB2_DATA6*/
245 MUX_VAL(CP(MCSPI2_CLK), (IEN  | PTD | DIS | M0));   
/*HSUSB2_DATA7*/
246 MUX_VAL(CP(SYS_BOOT2),  (IEN  | PTD | DIS | M4))/* GPIO_4 */
247 MUX_VAL(CP(ETK_D10_ES2),(IDIS | PTU | DIS | M3));   
/*HSUSB2_CLK*/
248 MUX_VAL(CP(ETK_D11_ES2),(IDIS | PTU | DIS | M3));   
/*HSUSB2_STP*/
249 MUX_VAL(CP(ETK_D12_ES2),(IEN  | PTU | DIS | M3));   
/*HSUSB2_DIR*/
...

All the entries in set_muxconf_regs() have a terminating semicolon -
all except the entry in line # 246.

I reckon this is a mistake and should be fixed?

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 more bored than you could ever possibly be.  Go back to work.


Re: [PATCH v4 4/5] env: Allow environment files to use the C preprocessor

2021-09-20 Thread Wolfgang Denk
Dear Simon,

In message 
<20210919125937.v4.4.Ie78bfbfca0d01d9cba501e127f446ec48e1f7afe@changeid> you 
wrote:
>
> +To add additional text to a variable you can use var+=value. This text is
> +merged into the variable during the make process and made available as a
> +single value to U-Boot.

Explained after use.  Move documentation to previous patch?

> + # Deal with +=
> + if (match(var, "(.*)[+]$", var_arr)) {
> + var = var_arr[1]
> + env = vars[var] env
> + }


Hm...  this is (so far) a legal command:

=> setenv foobar+ foo+=bar


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
"Summit meetings tend to be like panda matings. The expectations  are
always high, and the results usually disappointing."   - Robert Orben


Re: [PATCH v4 3/5] env: Allow U-Boot scripts to be placed in a .env file

2021-09-20 Thread Wolfgang Denk
Dear Simon,

In message 
<20210919125937.v4.3.If789ba3e2667c46c03eda3386ca84a863baeda55@changeid> you 
wrote:
>
...
> +It is also possible to create an environment file with the name
> +`board//env/.env` for your board. If that file is not present
> +then U-Boot will look for `oard//env/common.env` so that you can

a/oard/board/

> +have a common environment for all vendor boards.

Actually it would be nice to look for `board//env/common.env`
first, and then for `board//env/.env` - and if both
exost, they should be concatenated, so a vendor can keep all common
definitions in the common.env, and have only board specific
definitions in the .env files - otherwise he would have to
repeat everything in the board files, and the common file makes
little sense.

> +This is a plain text file where you can type your environment variables in
> +the form `var=value`. Blank lines and multi-line variables are supported.
> +The conversion script looks for a line that starts with a letter or number
> +and has an equals sign immediately afterwards. Spaces before the = are not
> +permitted. It is a good idea to indent your scripts so that only the 'var='
> +appears at the start of a line.

This is not correct.  Variable names can be more complex:

=> setenv _foo 1
=> setenv ,bar 2
=> setenv /baz 3

etc.

> +For example, for snapper9260 you would create a text file called
> +`board/bluewater/env/snapper9260.env` containing the environment text.
> +
> +Example::
> +
> +stdout=serial
> +#ifdef CONFIG_LCD
> +stdout+=,lcd

Is that a new feature?  I didn't see it documented anywhere?

> +#endif
> +bootcmd=
> +/* U-Boot script for booting */
> +
> +if [ -z ${tftpserverip} ]; then
> +echo "Use 'setenv tftpserverip a.b.c.d' to set IP address."
> +fi
> +
> +usb start; setenv autoload n; bootp;
> +tftpboot ${tftpserverip}:
> +bootm
> +failed=
> +/* Print a message when boot fails */
> +echo CONFIG_SYS_BOARD boot failed - please check your image
> +echo Load address is CONFIG_SYS_LOAD_ADDR

You _must_ define a clear syntax for the file, including indentation
rules.  Otherwise there is plenty chance for incorrect arsing.


> + # Is this the start of a new environment variable?
> + if (match($0, "^([^ =][^ =]*)=(.*)", arr)) {

This is not what you document above.

> + if (length(env) != 0) {
> + # Record the value of the variable now completed
> + vars[var] = env
> + }

What in case of length == 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
Imitation is the sincerest form of plagarism.


Re: [PATCH v4 2/5] doc: Move environment documentation to rST

2021-09-20 Thread Wolfgang Denk
Dear Simon,

In message <20210919185950.3813952-3-...@chromium.org> you wrote:
> Move this from the README to rST format.

Just a few nitpicks...

> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> new file mode 100644
> index 000..be785a8f717
> --- /dev/null
> +++ b/doc/usage/environment.rst
> @@ -0,0 +1,382 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +Environment Variables
> +=
> +
> +
> +U-Boot supports user configuration using Environment Variables which
> +can be made persistent by saving to Flash memory.

...by saving to persistent storage, for example flash memory.

> +Environment Variables are set using "setenv", printed using
> +"printenv", and saved to Flash using "saveenv". Using "setenv"

Environment Variables are set using "env set" (alias "setenv"),
printed using "env print" (alias "printenv"), and saved to
persistent storage using "env save" (alias "saveenv").
Using "env set" ...

> +Some configuration options can be set using Environment Variables.

This probably needs an explanation what "configuration options'
means here?


> +Image locations
> +---
> +
> +The following image location variables contain the location of images
> +used in booting. The "Image" column gives the role of the image and is
> +not an environment variable name. The other columns are environment
> +variable names. "File Name" gives the name of the file on a TFTP
> +server, "RAM Address" gives the location in RAM the image will be
> +loaded to, and "Flash Location" gives the image's address in NOR
> +flash or offset in NAND flash.
> +
> +*Note* - these variables don't have to be defined for all boards, some
> +boards currently use other variables for these purposes, and some
> +boards use these variables for other purposes.
> +
> += ==  ==
> +Image File Name  RAM Address  Flash Location
> += ==  ==
> +u-bootu-boot u-boot_addr_ru-boot_addr
> +Linux kernel  bootfile   kernel_addr_rkernel_addr
> +device tree blob  fdtfilefdt_addr_r   fdt_addr
> +ramdisk   ramdiskfileramdisk_addr_r   ramdisk_addr
> += ==  ==

Maybe it should be pointed out that this is just a commonly used set
of variable names, used in some other variable definitions, but is
in no way hard coded anywhere in U-Boot code.

OK, I see that "bootfile" and ""fdtfile" are now actually used in
some code - but IMO this is a bad idea and should be fixed.


> +Automatically updated variables
> +---
> +
> +The following environment variables may be used and automatically
> +updated by the network boot commands ("bootp" and "rarpboot"),
> +depending the information provided by your boot server:
> +
> +=  ===
> +Variable   Notes
> +=  ===
> +bootfile   see above
> +dnsip  IP address of your Domain Name Server
> +dnsip2 IP address of your secondary Domain Name Server
> +gatewayip  IP address of the Gateway (Router) to use
> +hostname   Target hostname
> +ipaddr See above
> +netmaskSubnet Mask
> +rootpath   Pathname of the root filesystem on the NFS server
> +serverip   see above
> +=  ===

This list is incomplete; for example, sysboot sets bootfile, too.

For completeness, .flags etc. should be documented, too [but yes,
this is a separate task 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
I wrote my name at the top of the page. I wrote down  the  number  of
the  question  ``1''.  After much reflection I put a bracket round it
thus ``(1)''. But thereafter I could not think of anything  connected
with it that was either relevant or true.
- Sir Winston Churchill _My Early Life_ ch. 2


Re: U-BOOT missing Security support

2021-09-15 Thread Wolfgang Denk
Dear Cedrick,

In message 

 you wrote:
> 
> I was attempting to enable CONFIG_CMD_AES and I though it would be
> under security support. Is there any reason why that option would
> not be enabled or available?

Search under "Security commands"...

from "cmd/Kconfig":


1975 menu "Security commands"
1976 config CMD_AES
1977 bool "Enable the 'aes' command"
1978 select AES
1979 help
1980   This provides a means to encrypt and decrypt data using the AES
1981   (Advanced Encryption Standard). This algorithm uses a symetric 
key
1982   and is widely used as a streaming cipher. Different key lengths 
are
1983   supported by the algorithm but this command only supports 128 
bits
1984   at present.
1985


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 Stanford research group advertised for participants in a  study  of
obsessive-compulsive  disorder. They were looking for therapy clients
who had been diagnosed with this disorder. The  response  was  grati-
fying;  they  got  3,000 responses about three days after the ad came
out. All from the same person.


Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

2021-09-13 Thread Wolfgang Denk
Dear Pali Rohár,

In message <20210913122245.my6ik4yjy7rwlh65@pali> you wrote:
>
> Timeout is not too slow, but sometimes user is (when is interrupted by
> other things during selecting file). And then it is not obvious why
> sx/sb command is failing... compared to transfer via gkermit which do
> not have this "problem".

I see.

> > > So... I do not know what is better if current behavior or this new which
> > > changes UI interaction.
> > 
> > We can do both, and still solve your problem: make the timeout
> > adjustable so you can set it to something you can conveniently work
> > with.
>
> Good point! env with timeout (or easier would be retry count?) seems
> like a ideal solution how to "define" behavior without changing default
> one. And e.g. negative value can mean infinite to support all possible
> combinations.

I would recommend to define a timeout (in seconds), this is easier
to understand for the end user - without looking at the source code
they cannot know how a retry count translates into a time interval.
And yes, setting the timeout to 0 could mean it waits forever.

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 optimum committee has no members.
   - Norman Augustine


Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

2021-09-13 Thread Wolfgang Denk
Dear Pali Rohár,

In message <20210913110806.27hc36n6gmhw6uq4@pali> you wrote:
>
> > If you use loady in any kind of scripts, this would now hard hang
> > the system, while until now it was possible to recover from the
> > error.
>
> Yes, this is a good point. But on the other hand, 'loadb' and 'loads'
> commands already have this behavior. So question is if it is better to
> have same behavior in all 'load?' commands or each 'load?' would behave
> differently... Because for software which transmit files and supports
> more protocols (e.g. both x-modem and kermit) it may be a nightmare if
> receiver behaves differently...

Yes, you are right, there is an unlucky difference in behaviour.
But all these interfaces are pretty old, and I would not invest
efforts to fix a aproblem nobody ever noticed before, at the risk
of breaking existing stuff.

I wonder if there are any users of 'loads' left - the Motorola
S-record format is close to 50 years old and cumbersome to use.  I
can't even remeber when I used it the last time - must be 15+ years
or such.

'loadb" is a different thing, but there you usually have kermit on
the other side, and usually run an interactive session (or an
automated one using something like  tbot ) - in any case, you
normally have to interact on both sides.

I never had a problem wih the current behaviour there.


> If you do not have integrated y-modem support in your terminal you have
> to do:
>
> 1) open terminal and write 'loady' into U-Boot console
> 2) disconnect terminal
> 3) start y-modem software
> 4) choose file to transmit
> 5) instruct y-modem software to start transfer
>
> And if 'loady' timeouts between 2) - 5) then it returns back to the

If this happens, the timeout is inconveniently short of you are too
slow.  I think what would be helpful is to make the timeout
adjustable (env var).

> So... I do not know what is better if current behavior or this new which
> changes UI interaction.

We can do both, and still solve your problem: make the timeout
adjustable so you can set it to something you can conveniently work
with.

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 2/2] xyz-modem: Wait infinitely for initial x-modem packet

2021-09-13 Thread Wolfgang Denk
Dear Pali Rohár,

In message <20210910204653.3066-2-p...@kernel.org> you wrote:
> Implement same thing also for x-modem protocol. As x-modem protocol does
> not have header packet, first packet is directly first data packet.
>
> Signed-off-by: Pali Rohár 
> ---
>  common/xyzModem.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Ditto here.   Please don't.

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 believe the use of noise to make  music  will  increase  until  we
reach  a  music  produced  through  the aid of electrical instruments
which will make available for musical purposes  any  and  all  sounds
that can be heard."- composer John Cage, 1937


Re: [PATCH 1/2] xyz-modem: Wait infinitely for initial y-modem packet

2021-09-13 Thread Wolfgang Denk
Dear Pali Rohár,

In message <20210910204653.3066-1-p...@kernel.org> you wrote:
> Now when command loady can be aborted / cancelled by CTRL+C, change wait
> timeout for initial packet to infinite. This would allow user to not be
> hurry when locating file which want to send. Commands loadb and loads
> already waits infinitely too.

I'm not sure if this is a good idea.

If you use loady in any kind of scripts, this would now hard hang
the system, while until now it was possible to recover from the
error.

This is a change to the user interface that is not really necessary,
so I recommend NOT to change the behaviour here, especially as it
does not hurt.

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've finally learned what `upward compatible' means. It means we get
to keep all our old mistakes." - Dennie van Tassel


Re: [PATCH v2] powerpc: mpc: Put U-Boot version string at correct place by linker script

2021-08-24 Thread Wolfgang Denk
Dear Christophe,

In message <75ae39aa-3762-6b90-c39a-7595fc417...@csgroup.eu> you wrote:
> 
> > Also, could you look at this, why version string is at specific location
> > of u-boot.bin header?
>
> I have no idea. AFAIKS it's been like that since the first version
> in git in 2002. Maybe Wolfgang has the answer ?

The Power Architecture was the very first where PPCBoot / U-Boot were
running, more than 20 years ago. At that time, resources were tight.
Systems would typically boot from a small NOR flash.  Typical
configurations came with 4 MB NOR / 16 MB RAM or such.

One problem was that you needed always at least one (or better two,
with redundancy) full sectors of the flash for the environment, even
though this was typically only a few hundred bytes long.  Many NOR
flash chips of that time did not have uniform sector sizes; instead,
they would come with mixed sector sizes.

For example, there were NOR flash chips which came with sector sizes
of 8, 4, 4, 16, 32, 32, 32, 32, ... kB.

Here you would naturally want to use the two small 4 kB sectors for
the environment, but this results in the need to have the
environment storage "embedded" within the U-Boot image.  This
resulted in the need to come up with special linker scripts.  And of
course you would try to use the first 8 kB sector as good as
possible, which was achieved by used manually optimized linker
scripts, collecting functions or other objects such that the wasted
gap at the end of the first sector was minimal - usually less than
100 bytes were wasted.

As part of such optimizations, it might have made sense to put the
version string there, too - it has a fixed size of a few ten bytes,
so it is a good candidate to fill the gap.


Today, such boot sector NOR flashes are no longer found in new
designs, nor does anybody care to keep U-Boot small and tidy.
As a result, knowledge about such optimizations is disappearing.


Hope this helps.



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 want to eat hippopotamus, you've got to pay the freight."   -
attributed to an IBM guy, about why IBM software uses so much memory


Re: Remove the gitlab-ci-runner on source.denx.de?

2021-08-23 Thread Wolfgang Denk
Dear Tom,

In message <20210823140128.GS858@bill-the-cat> you wrote:
> 
> > As the Dockerfile used for U-Boot CI testing is now integrated into
> > the U-Boot source tree, I suggest we remote the gitlab-ci-runner repo
> > to avoid confusion.
> >
> > https://source.denx.de/u-boot/gitlab-ci-runner
>
> Good idea.  Wolfgang, you're listed as Owner in GitLab so you've got the
> permissions for that and I don't, please delete it.  Thanks!

Instead of removing it, I have archived it for now.  If noody asks
for it for some time (say, a month or two), I will finally remove
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
How much net work could a network work, if a network could net work?


Re: How should we deal with actual hush odd behavior?

2021-08-23 Thread Wolfgang Denk
Dear Francis,

In message <2787647.e9J7NaK4W3@pwmachine> you wrote:
>
> Porting 2021 Busybox hush to U-Boot seems, for me, to be a good idea as we 
> would benefit from Busybox bug fixes as well as being compatible with actual 
> hush (in theory).
> We could also add new features to U-Boot hush, like functions, as they were 
> added to Busybox.

Thanks a lot, much appreciated.

> Nonetheless, the idea of this port is to be compatible.

My recommendation is not to try to be bug-compatible, i. e. is the
current U-Boot version of hush behaves different than the recent
one, we should check how other POSIX compatible shells behave.
I would expect that in most cases the old hush in U-Boot has a bug,
which has been fixed in the recent version.

In such a situation, we should accept the bug fix and not try tokeep
the old, buggy behaviour of the old version.


> In practice, I noted some cases when this is actually not the case.
> The first one can be related to how && and || operators were handled in hush.
> So, the following: false && false || true
> Returns 0 on Busybox 2021 hush and 1 on U-Boot.
> The behavior of 2021 is coherent with the definition of these operators [4]:
> >The return status of AND and OR lists is the exit 
> >status of the last command executed in the list.

This is a clear bug in the old version.  It's good to see it fixed
in the recent code.

> An other example concerns variable expansion, where foo='bar "quux" is 
> expanded to bar quux in U-Boot and bar "quux in Busybox.

I guess you mean   foo='bar "quux'   ?  [ foo='bar "quux"  is
missing the closing apostrophe.]

Again, this is a bug in U-Boots command line parsing.


> I do not have a real opinion on the second one, as I think local variable set 
> in U-Boot scripts are quite simple as people do not try to do: foo="bar 
> \"quux 
> 'quuz' \"\"\"corge".
> The first one is maybe more problematic.
> Grepping "if test" shows me that the more complex if condition seems to be 
> under the form:
> if first_test_ AND/OR second_test
> Here also, people seems to no try to write complex expression like: foo || 
> bar; echo quux && quuz.

We should port the recent version of hush without hesitating about
bug compatibility of expected use cases.  We can'tknoww if peple
didn't use afeature because the ran into problems with it, or
because it's just not their programming style.

> So, porting Busybox 2021 hush can solve bugs we have currently in U-Boot, but 
> what if fixing these bugs lead to a board script failing and so a device not 
> booting...

The new version should be optional in any case, at least for some
longer migration period to give users time to test their scripts.
If someone selects the new version in his board configuration, he
probably has run enough tests to make sure his scripts are working
fine in the new environment.


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 try to outweird me, three-eyes. I get stranger things than you
free with my breakfast cereal."
   - Zaphod Beeblebrox in  "Hitchhiker's Guide to the Galaxy"


Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

2021-08-23 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
> >
> > Wow.
> >
> > I think I'll add this to my signature database:
> >
> > | "Trying to catch [programming errors] in the field at runtime is not
> > | very kind to your users."
> > |
> > | - Simon Glass in 
> > 
>
> Well you've taken it out of context :-) It would make more sense if
> you mention the need for tests

Testing can show the presense of bugs, but not their absence.
   -- Edsger Dijkstr

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: [PATCH v6 10/12] watchdog: add gpio watchdog driver

2021-08-19 Thread Wolfgang Denk
Dear Simon,

In message  
you wrote:
>
> - programming errors
> - security errors where user input is insufficiently checked
>
> IMO the former should not be present if you have sufficient tests and
> trying to catch them in the field at runtime is not very kind to your
> users.

Wow.

I think I'll add this to my signature database:

| "Trying to catch [programming errors] in the field at runtime is not
| very kind to your users."
| 
| - Simon Glass in 


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 wrong always, everywhere and for everyone to  believe  anything
upon  insufficient  evidence.  - W. K. Clifford, British philosopher,
circa 1876


Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

2021-08-19 Thread Wolfgang Denk
Dear Tom,

In message <20210819130806.GW858@bill-the-cat> you wrote:
> 
> > So we have now a policy to wave through code, and ask others to
> > clean it up later?  That's ... sad.
>
> No, we continue to have the policy of expecting reviewers to follow the
> whole discussion and relevant subsystems.

Once upon a time there has also been a policy that if a function
might return error codes, these need to be checked and handled.

> Changing _every_ caller of dev_get_priv to check for NULL and
> then, what? is clearly not the right answer.

Then what is the right answer in your opinion?

I mean, look at the implementation of dev_get_priv():

 628 void *dev_get_priv(const struct udevice *dev)
 629 {
 630 if (!dev) {
 631 dm_warn("%s: null device\n", __func__);
 632 return NULL;
 633 }
 634
 635 return dm_priv_to_rw(dev->priv_);
 636 }

If there is guaranteed no way that dev_get_priv() can return a NULL
pointer, that means that it must be guaranteed that the "dev"
argument can never be a NULL pointer, either.  So why do we check it
at all?

The same applies for all functions in "drivers/core/device.c" - they
all check for valid input parameters, like any code should do.

> If you think you see a problem here please go audit the DM code
> itself more and propose some changes.

I can see that the DM code itself implements proper error checking
and reporting; it's the callers where negligent code prevails.


If you are consequent, you must decide what you want:

- Either we want robust and reliable code - then we have to handle
  the error codes which functions like dev_get_priv() etc. return.

- Or you don't care about software quality, then we can omit such
  handling, but then it would also be consequent to remove all the
  error checking from "drivers/core/device.c" etc. - hey, that would
  even save a few hundred bytes of code size.


Sugarcoating code which fails to handle error codes because "these
errors can never happen" does not seem to be a clever approach to
software engineering to me.


I stop here.  You know my opinion.  You are the maintainer.


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 witty saying proves nothing, but saying  something  pointless  gets
people's attention.


Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

2021-08-19 Thread Wolfgang Denk
Dear Tom,

In message <20210819123540.GV858@bill-the-cat> you wrote:
> 
> Since I literally just sent this in another email you couldn't have seen
> yet, I'll repeat it here.  Feel free to follow up to this with a series
> to further update things, Wolfgang.

So we have now a policy to wave through code, and ask others to
clean it up later?  That's ... sad.

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
"Consistency requires you to be as ignorant today as you were a  year
ago."  - Bernard Berenson


Re: [PATCH v6 10/12] watchdog: add gpio watchdog driver

2021-08-19 Thread Wolfgang Denk
Dear Rasmus,

In message <62540f7b-0e07-8759-8e12-125527c2e...@prevas.dk> you wrote:
>
> >> +static int gpio_wdt_reset(struct udevice *dev)
> >> +{
> >> +  struct gpio_wdt_priv *priv = dev_get_priv(dev);
> >> +
> >> +  priv->state = !priv->state;
> > 
> > Potential NULL pointer dereference.
>
> No, no and no. If allocation of the (driver or uclass) private data
> fails, the device probe would have failed, so this code can never get
> called with such a struct udevice.

Famous last words...

> Perhaps try doing a
>
> git grep -10 -E 'dev_get(_uclass)?_priv'
>
> and see how many cases you can find where that is followed by a NULL check?

The existence of bad code is not a justification to add more of 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
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 v6 10/12] watchdog: add gpio watchdog driver

2021-08-19 Thread Wolfgang Denk
Dear Rasmus,

again: error handling.

In message <20210819095706.3585923-11-rasmus.villem...@prevas.dk> you wrote:
>
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> new file mode 100644
> index 00..982a66b3f9
> --- /dev/null
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct gpio_wdt_priv {
> + struct gpio_desc gpio;
> + bool always_running;
> + int state;
> +};
> +
> +static int gpio_wdt_reset(struct udevice *dev)
> +{
> + struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +
> + priv->state = !priv->state;

Potential NULL pointer dereference.

> +static int gpio_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> + struct gpio_wdt_priv *priv = dev_get_priv(dev);
> +
> + if (priv->always_running)
> + return 0;

Ditto.

> +static int dm_probe(struct udevice *dev)
> +{
> + struct gpio_wdt_priv *priv = dev_get_priv(dev);
> + int ret;
> +
> + priv->always_running = dev_read_bool(dev, "always-running");

Ditto.

> + ret = gpio_request_by_name(dev, "gpios", 0, >gpio, GPIOD_IS_OUT);
> + if (ret < 0) {
> + dev_err(dev, "Request for wdt gpio failed: %d\n", ret);
> + return ret;
> + }
> +
> + if (priv->always_running)
> + ret = gpio_wdt_reset(dev);

Ditto.


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
Even if you aren't in doubt, consider the mental welfare of the  per-
son who has to maintain the code after you, and who will probably put
parens in the wrong place.  - Larry Wall in the perl man page


Re: [PATCH v6 09/12] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

2021-08-19 Thread Wolfgang Denk
Dear Rasmus,

In message <20210819095706.3585923-10-rasmus.villem...@prevas.dk> you wrote:
>
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 5b1c0df5d6..7570710c4d 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -61,20 +61,24 @@ static void init_watchdog_dev(struct udevice *dev)
...
> + ret = uclass_get(UCLASS_WDT, );
> + if (ret) {
> + log_debug("Error getting UCLASS_WDT: %d\n", ret);
> + return 0;
> + }
> +
> + uclass_foreach_dev(dev, uc) {
> + ret = device_probe(dev);
> + if (ret) {
> + log_debug("Error probing %s: %d\n", dev->name, ret);
> + continue;
>   }

As discussed - errors need to be shown to the user, and not only in
images with debugging enabled.

> @@ -182,22 +186,34 @@ void watchdog_reset(void)
>  {
>   struct wdt_priv *priv;
>   struct udevice *dev;
> + struct uclass *uc;
>   ulong now;
>  
>   /* Exit if GD is not ready or watchdog is not initialized yet */
>   if (!gd || !(gd->flags & GD_FLG_WDT_READY))
>   return;
>  
> - dev = gd->watchdog_dev;
> - priv = dev_get_uclass_priv(dev);
> - if (!priv->running)
> + if (uclass_get(UCLASS_WDT, ))
>   return;


Do I see this crrectly that you remove here the code which you just
added in patch 02 of this series?

Why not doing it right from the beginning?

> + uclass_foreach_dev(dev, uc) {
> + if (!device_active(dev))
> + continue;
> + priv = dev_get_uclass_priv(dev);
> + if (!priv->running)
> +     continue;

Potential NULL pointer dereference.


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
An Ada exception is when a routine gets in trouble and says
'Beam me up, Scotty'.


Re: [PATCH v6 07/12] watchdog: wdt-uclass.c: add wdt_stop_all() helper

2021-08-19 Thread Wolfgang Denk
Dear Rasmus,

again: error handling.

In message <20210819095706.3585923-8-rasmus.villem...@prevas.dk> you wrote:
>
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>   return ret;
>  }
>  
> +int wdt_stop_all(void)
> +{
> + struct wdt_priv *priv;
> + struct udevice *dev;
> + struct uclass *uc;
> + int ret, err;
> +
> + ret = uclass_get(UCLASS_WDT, );
> + if (ret)
> + return ret;
> +
> + uclass_foreach_dev(dev, uc) {
> + if (!device_active(dev))
> + continue;
> + priv = dev_get_uclass_priv(dev);
> + if (!priv->running)
> +     continue;

Potential NULL pointer dereferencing.


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 paid too much for it, but its worth it.


Re: [PATCH v6 05/12] watchdog: wdt-uclass.c: keep track of each device's running state

2021-08-19 Thread Wolfgang Denk
Dear Rasmus,

please check your patches for proper error handling.

In message <20210819095706.3585923-6-rasmus.villem...@prevas.dk> you wrote:
>
...
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 0a1f43771c..358fc68e27 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
...
> @@ -86,8 +89,11 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong 
> flags)
>   return -ENOSYS;
>  
>   ret = ops->start(dev, timeout_ms, flags);
> - if (ret == 0)
> - gd->flags |= GD_FLG_WDT_READY;
> + if (ret == 0) {
> + struct wdt_priv *priv = dev_get_uclass_priv(dev);
> +
> + priv->running = true;

dev_get_uclass_priv() can return NULL, in which case you would be
dereferencing a NULL pointer...

> @@ -101,8 +107,11 @@ int wdt_stop(struct udevice *dev)
>   return -ENOSYS;
>  
>   ret = ops->stop(dev);
> - if (ret == 0)
> - gd->flags &= ~GD_FLG_WDT_READY;
> + if (ret == 0) {
> + struct wdt_priv *priv = dev_get_uclass_priv(dev);
> +
> + priv->running = false;

Same here.

> @@ -156,6 +165,9 @@ void watchdog_reset(void)
>  
>   dev = gd->watchdog_dev;
>   priv = dev_get_uclass_priv(dev);
> + if (!priv->running)
> +     return;
> +

And here again.

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 used to be indecisive, now I'm not sure.


Re: [PATCH] configs: Layerscape: Remove the 'fdt_addr' env

2021-08-19 Thread Wolfgang Denk
Dear Priyanka,

In message 

 you wrote:
> 
> I agree we cant add copyright for minor changes.
> 
> But this file already contains Freescale (now merged into NXP) copyright.
> Zhiqiang is updating to latest copyright text used by NXP(Freescale) with 
> proper year.
> I hope this is fine.

Hm... I cannot see this.

> diff --git a/include/configs/ls1012afrdm.h b/include/configs/ls1012afrdm.h
> index 2711f651d7..c7fdd10cf5 100644
> --- a/include/configs/ls1012afrdm.h
> +++ b/include/configs/ls1012afrdm.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0+ */
>  /*
>   * Copyright 2016 Freescale Semiconductor, Inc.
> + * Copyright 2021 NXP
>   */

This is adding a NEW copyright entry - for deleting one line of
data (not even code).


> diff --git a/include/configs/ls1021atsn.h b/include/configs/ls1021atsn.h
> index 58c2d97a32..97fd61f6f9 100644
> --- a/include/configs/ls1021atsn.h
> +++ b/include/configs/ls1021atsn.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0
> - * Copyright 2016-2019 NXP Semiconductors
> + * Copyright 2016-2019, 2021 NXP Semiconductors
>   * Copyright 2019 Vladimir Oltean 
>   */

Same here, again for deleting one line of data.

> diff --git a/include/configs/ls1021atwr.h b/include/configs/ls1021atwr.h
> index ba308c514b..5be179a36b 100644
> --- a/include/configs/ls1021atwr.h
> +++ b/include/configs/ls1021atwr.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0+ */
>  /*
>   * Copyright 2014 Freescale Semiconductor, Inc.
> - * Copyright 2019 NXP
> + * Copyright 2019, 2021 NXP
>   */

Again, for deleting 2 lines of data.

And so on.


None of these changes has substance to claim a copyright for.

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
"Plan to throw one away. You will anyway."
  - Fred Brooks, "The Mythical Man Month"


Re: [PATCH v5 09/12] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

2021-08-19 Thread Wolfgang Denk
Dear Rasmus,

In message <4798abb5-07d9-fa88-931f-dbaff951e...@prevas.dk> you wrote:
> >>
> >> +  ret = uclass_get(UCLASS_WDT, );
> >> +  if (ret) {
> >> +  log_debug("Error getting UCLASS_WDT: %d\n", ret);
> >> +  return 0;
> >> +  }
> > 
> > Here the error goes silent, so we should fix the callers to report
> > it.
>
> The caller (singular) is the initr sequence, so returning an error is
> effectively the same as halting the boot process, and as I've already
> explained, I'm not going to change the semantics of initr_watchdog in
> this regard.

In this case you must print an error message here.

> Feel free to submit a patch if you feel a change in this area is in
> order. That's completely unrelated to what these patches are trying to
> achieve.

You add new code here, so please make sure not to add known issues.

> >> +  uclass_foreach_dev(dev, uc) {
> >> +  ret = device_probe(dev);
> >> +  if (ret) {
> >> +  log_debug("Error probing %s: %d\n", dev->name, ret);
> >> +  continue;
> >>}
> > 
> > Here the situation is different.  The probing error is never
> > reported anywhere.  Is it really a normal condition that a
> > device_probe() fails here?
>
> No, it is not a normal condition. I added the log_debug() after a
> request from Simon.

But log_debug() is nothing any user will see in the field.  We need
an error message here, 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
If the odds are a million to one against something occuring,  chances
are 50-50 it will.


Re: [PATCH] cmd: boot: Update reset usage message

2021-08-13 Thread Wolfgang Denk
Dear Michal,

In message <90e6c670-9e11-beb8-bcb5-9d22ba00f...@xilinx.com> you wrote:
> 
> > In case of the hard (cold) reset - is it really only a reset of the
> > CPU, or of the whole board hardware?
>
> If you look at sysreset headers you will find these levels
>  11 SYSRESET_WARM,  /* Reset CPU, keep GPIOs active */
>  12 SYSRESET_COLD,  /* Reset CPU and GPIOs */
>  13 SYSRESET_POWER, /* Reset PMIC (remove and restore power) */
>  14 SYSRESET_POWER_OFF, /* Turn off power */
>
> When you call reset sysreset uclass is calling sysreset_walk which is
> request to drivers with type passed.
> I see we have mixed drivers which deals with levels and especially in
> gpio case it is question how you connect it.
> I develop this for microblaze where gpio is connected reset logic which
> is normally only for CPU and subsystem.
> But in general you can connect whatever you want. It means it doesn't
> need to be only cpu which is reset.

Thanks a lot for the explanation.

> Do you want me to update that line and remove CPU from it?

I don't know :-)

What the "reset" command _should_ do is a hard cold boot including
the reset of all peripherals - for example, when booting from SPI
NOR flash it is mandatory to reset this flash to make sure the ROM
boot loader can actually read it.

I would appreciate if the help message documents what it actually
does.  Also, what the difference between "cold" and "warm" reset is.
My expectation (without knowing any hardware details) would be that
a warm reset is just a restart of the already loaded code? Or is it
just a reset of the CPU (without external reset)? This might then
hang the system if it's attempting to boot from a flash which is in
the wrong state.

So my problem with this is primarily that I don't understand what
the command really does, and the help command is of no help either.

[And if I understand correctly, this is even board dependent?]

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 NASA managers does it take to screw in a lightbulb?  "That's
a known problem... don't worry about it."


Re: [PATCH v4 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

2021-08-13 Thread Wolfgang Denk
Dear Tom,

In message <20210812162034.GY858@bill-the-cat> you wrote:
> 
> > So if "the system is on fire" is one of the cases where an error
> > message should be omitted to save maybe 50 or 100 bytes of image
> > size?  This sounds wrong to me.
>
> It sounds right to me because it's unlikely everything caught fire
> because of this call right here and likely it's because of one of the
> messages much further up on the console log.  Hopefully we haven't
> caused that message to be unavailable now due to unhelpful failure
> messages.

Omitting code to handle situations that are unlikely to happen is
definitely not what I consider robust programming, and nothing which
I would let pass a code review.

But if you insist, there is no more to do for me here that to note
that fact that we disagree.


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
People are very flexible and learn to adjust to strange  surroundings
--  they can become accustomed to read Lisp and Fortran programs, for
example.   - Leon Sterling and Ehud Shapiro, Art of Prolog, MIT Press


Re: [PATCH] cmd: boot: Update reset usage message

2021-08-13 Thread Wolfgang Denk
Dear Michal,

In message 
<82e0d7efdbd9f8c62f46c7e1a8913ffa52de5a1e.1628676265.git.michal.si...@xilinx.com>
 you wrote:
> The commit 573a3811edc8 ("sysreset: psci: support system reset in a generic
> way with PSCI") has added support for warm reset via PSCI but this hasn't
> been reflected in usage message and user has to look at the code how to run
> it. That's why update usage text to make this clear.
>
> Here is full help with updated usage:
> ZynqMP> help reset
> reset - Perform RESET of the CPU
>
> Usage:
> reset - cold boot without level specifier
> reset -w - warm reset if implemented

In case of the hard (cold) reset - is it really only a reset of the
CPU, or of the whole board hardware?

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 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.


  1   2   3   4   5   6   7   8   9   10   >