Re: [PATCH] tiny-printf: Add support for upper case hex values
Hi,
> >> The issue is that disabling TINY_PRINTF may not be possible (size
> >> constraints) and some code is compiled for different stages and people
> >> typically don't check whether the format used in printf is valid with
> >> tiny_printf. I've had this issue already in the past, I vaguely recall
> >> "complaining" about it on IRC.
> >
> > Yes, I've stumbled on "%pa" with tiny printf (i.e. in
> > drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
> > value, actually :) So printing anything unknown as '?' would really
> > help here.
>
> Something like that would help:
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index 48db7b1f78f..b918d6d7386 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -280,6 +280,12 @@ static int _vprintf(struct printf_info *info, const char
> *fmt, va_list va)
> while (isalnum(fmt[0]))
> fmt++;
> break;
> + } else {
> + if (fmt[0] != '\0' && (fmt[0] == 'a'
> || fmt[0] == 'm' ||
> + fmt[0] == 'M'
> || fmt[0] == 'I')) {
> + fmt++;
> + goto unsupported;
> + }
I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
*content* of the pointer which is really confusing. I.e. in
pinctrl-single.c the reg value pairs are printed like
dev_dbg(dev, "reg/val %pa/0x%08x\n", ®, val);
with reg being a pointer to a physical address. So with tiny_printf
the address of reg (which is a pointer to the stack) is printed in
this case.
I don't think we can print %p without putting more logic into the
decoding. I think the culprit here is the fallthrough to %x, which
then leads to the confusing behavior shown above. IMHO if we want to
avoid that, we'd have to make %p entirely unsupported.
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index faf55d7f327..8147ffa2c1b 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char
*fmt, va_list va)
div_out(info, &num, div);
}
break;
+#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
case 'p':
- if (CONFIG_IS_ENABLED(NET) ||
- CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG) {
- pointer(info, fmt, va_arg(va, void *));
- /*
-* Skip this because it pulls in _ctype
which is
-* 256 bytes, and we don't generally
implement
-* pointer anyway
-*/
- while (isalnum(fmt[0]))
- fmt++;
- break;
- }
- islong = true;
- /* no break */
+ pointer(info, fmt, va_arg(va, void *));
+ /*
+* Skip this because it pulls in _ctype which is
+* 256 bytes, and we don't generally implement
+* pointer anyway
+*/
+ while (isalnum(fmt[0]))
+ fmt++;
+ break;
+#endif
case 'x':
if (islong) {
num = va_arg(va, unsigned long);
@@ -310,6 +307,8 @@ static int _vprintf(struct printf_info *info, const char
*fmt, va_list va)
case '%':
out(info, '%');
default:
+ out(info, '?');
break;
}
-michael
signature.asc
Description: PGP signature
Re: [PATCH v2 5/6] dt-bindings: clock: drop NUM_CLOCKS define for EN7581
Sorry for jumping in late in this thread as it is still using my old Linaro email ID which should be disabled by now. On Tue, Apr 01, 2025 at 12:02:17PM -0600, Tom Rini wrote: > On Tue, Apr 01, 2025 at 07:28:36PM +0200, Krzysztof Kozlowski wrote: > > On 01/04/2025 18:40, Tom Rini wrote: > > > On Tue, Apr 01, 2025 at 05:27:30PM +0200, Krzysztof Kozlowski wrote: > > >> On 01/04/2025 16:44, Tom Rini wrote: > > >>> On Thu, Mar 27, 2025 at 03:58:52PM +0100, Krzysztof Kozlowski wrote: > > On 27/03/2025 15:50, Christian Marangi wrote: > > > On Thu, Mar 27, 2025 at 03:43:47PM +0100, Krzysztof Kozlowski wrote: > > >> On 14/03/2025 19:59, Christian Marangi wrote: > > >>> Drop NUM_CLOCKS define for EN7581 dts/upstream/src/include. This is > > >>> not a binding and > > >>> should not be placed here. Value is derived internally in the user > > >>> driver. > > >>> > > >>> Signed-off-by: Christian Marangi > > >>> Acked-by: Krzysztof Kozlowski > > >> Please drop my Ack. I have never acked such patch for uboot. If I > > >> did, > > >> it was by mistake - probably you CC-ed me for some reason. > > >> > > > > > > Some explaination, uboot introduced the concept of upstream where they > > > "import" linux patch for dts and dt-bindings. > > > > I expected OF_UPSTREAM to be taking the sources, not patches. > > > > > > > > This and the other patch are the exact upstream patch with only the > > > path > > > changed so I keep all the patch commit message with tags and added the > > > > > > [ upstream commit ] thing. > > > > > > Hope Tom can better suggest how this should be done. You were CC > > > probably because the git send-email included you as present in the > > > different tags. > > > > Well, Ack is still not valid because I did not Ack exactly that change. > > It does not matter for the ack, but for Reviewed-by it would matter, > > because it is a statement (of oversight...). I cannot control what you > > put into patches taken out of kernel, but at least do not Cc me on > > that. > > >>> > > >>> In specifics, yes, we should update doc/develop/devicetree/control.rst > > >>> and maybe doc/develop/sending_patches.rst to use --suppress-cc=all for > > >>> dts/upstream. > > >>> > > >>> But in general, what do you expect people to be doing with content from > > >>> devicetree-rebasing? We're doing some direct cherry-picks in between > > >>> merging of the tags. I think it would be weird to be dropping the tags > > >>> and un-attributing peoples work. > > >> > > >> > > >> I rather expected something like how kernel is importing dtc. You just > > >> list the commits you get. If you want the full git history, then I would > > >> expect simple git submodule. In both cases there will be no such patches > > >> on the lists. > > >> > > >> For the Ack it does not matter, but I would feel uncomfortable if people > > >> were sending stripped and modified patches with my Rb tag. > > > > > > I guess I'm confused. Looking at > > > https://patchwork.ozlabs.org/project/uboot/patch/20250314185941.27834-6-ansuels...@gmail.com/ > > > we're doing the normal thing of havig "[ upstream commit ]" after > > > the imported log. When I merge the subtree and tag it indeed gives what > > > you're expecting too. > > > > When you merge subtree, the patch is not modified and it lives in > > separate repo. No one sends them over lists, no one modifies them. > > Unlike here (even if modification did not happen, person was touching it > > so how can anyone be sure? That's not a scripted process). > > We merge the subtree on tags, and people cherry-pick commits in between > tags when needed. This is a case of the latter, which is why it says "[ > upstream commit ]" in the commit message, which is the usual case. Although we have tooling to pick patches from devicetree-rebasing tree but I can see Krzysztof's concerns here. We can't be sure if developer has touched the cherry picked patch or not but I suppose there would be similar concerns for the stable backports for Linux too. So IMHO, it's really upto maintainer applying those cherry-picked patches to see if there is any difference from upstream. However, there is an additional process change what we can do here is for the developer to list just the commit IDs for the patches to be cherry picked in dts/upstream in the cover letter. This way the maintainer can just directly use the tooling to cherry pick those patches before applying the patch-set. Tooling already available for cherry-picking subtree commits as: $ ./tools/update-subtree.sh pick dts -Sumit
Re: [PATCH] tiny-printf: Add support for upper case hex values
Hi, > The issue is that disabling TINY_PRINTF may not be possible (size > constraints) and some code is compiled for different stages and people > typically don't check whether the format used in printf is valid with > tiny_printf. I've had this issue already in the past, I vaguely recall > "complaining" about it on IRC. Yes, I've stumbled on "%pa" with tiny printf (i.e. in drivers/pinctrl/pinctrl-single.c) which is printing the very wrong value, actually :) So printing anything unknown as '?' would really help here. -michael signature.asc Description: PGP signature
Re: [PATCH 00/12] lwIP sandbox tests
Hi Simon, On 4/1/25 19:55, Simon Glass wrote: > Hi Ilias, > > On Wed, 2 Apr 2025 at 05:33, Ilias Apalodimas > wrote: >> >> Hi Simon >> >> On Tue, 1 Apr 2025 at 18:42, Simon Glass wrote: >>> >>> Hi Jerome, >>> >>> On Tue, 1 Apr 2025 at 03:14, Jerome Forissier >>> wrote: Hi Simon, Any feedback on this series? https://patchwork.ozlabs.org/project/uboot/list/?series=448553 >>> >>> No, not from me, but thanks for doing it! >> >> Aren't you maintaining the sandbox drivers? > > Yes, I'm listed as maintainer for lots of things. What sort of > feedback are you looking for? Well, you were pretty explicit that those tests were important to you ([1], [2] and especially [3]), so I was expecting some kind of review/ack on your part. The series is currently assigned to me in patchwork and I got zero comments, so I'm not sure what to do with it. Unless someone objects I will probably add it to my next PR to Tom for -next. [1] https://lists.denx.de/pipermail/u-boot/2025-February/581988.html [2] https://lists.denx.de/pipermail/u-boot/2025-March/582435.html [3] https://lists.denx.de/pipermail/u-boot/2025-March/583362.html Thanks, -- Jerome > > Regards. > > Simon
Re: [PATCH] acpi: Fix table length for QEMU
On 4/3/25 07:53, Ilias Apalodimas wrote:
On Thu, 3 Apr 2025 at 08:34, Ilias Apalodimas
wrote:
The end of the ACPI table is set to 'addr' instead of 'end'. The ACPI
code for QEMU relies on those values to mark memory as 'ACPI Reclaim'
and as a result the ACPI RSDP ends up in Boot services Data.
Reported-by: bj...@kernel.org
I obviously messed up my reported-by...
Reported-by: Björn Töpel
Reviewed-by: Heinrich Schuchardt
Fixes: commit 638cc363484b ("acpi: enable writing ACPI tables on QEMU")
Signed-off-by: Ilias Apalodimas
---
drivers/misc/qfw_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/qfw_acpi.c b/drivers/misc/qfw_acpi.c
index 0d0cf7646890..77cebae5e3fd 100644
--- a/drivers/misc/qfw_acpi.c
+++ b/drivers/misc/qfw_acpi.c
@@ -305,7 +305,7 @@ static int evt_write_acpi_tables(void)
/* Generate ACPI tables */
end = write_acpi_tables(addr);
gd->arch.table_start = addr;
- gd->arch.table_end = addr;
+ gd->arch.table_end = end;
return 0;
}
--
2.49.0
Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console
Hi Tom,
On Thu, 3 Apr 2025 at 08:57, Tom Rini wrote:
>
> On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> > On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 3 Apr 2025 at 03:29, Tom Rini wrote:
> > > >
> > > > On Sat, Mar 15, 2025 at 02:26:00PM +, Simon Glass wrote:
> > > > > Update the string to take account of UARTs which are connected on I/O
> > > > > ports, as on x86.
> > > > >
> > > > > Fix a typo in an error message in the same command, while we are here.
> > > > >
> > > > > Signed-off-by: Simon Glass
> > > > > ---
> > > > >
> > > > > (no changes since v3)
> > > > >
> > > > > Changes in v3:
> > > > > - Add new patch to support IO UARTs for earlycon and console
> > > > >
> > > > > boot/bootflow.c | 7 ---
> > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > index 58a1afa7a75..4054a966af8 100644
> > > > > --- a/boot/bootflow.c
> > > > > +++ b/boot/bootflow.c
> > > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow,
> > > > > const char *arg)
> > > > > *buf = '\0';
> > > > > if (!strcmp("earlycon", arg) && info.type ==
> > > > > SERIAL_CHIP_16550_COMPATIBLE) {
> > > > > snprintf(buf, sizeof(buf),
> > > > > - "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > > - info.baudrate);
> > > > > + "uart8250,%s,%#lx,%dn8",
> > > > > + info.addr_space == SERIAL_ADDRESS_SPACE_IO ?
> > > > > "io" :
> > > > > + "mmio", info.addr, info.baudrate);
> > > > > } else if (!strcmp("earlycon", arg) && info.type ==
> > > > > SERIAL_CHIP_PL01X) {
> > > > > snprintf(buf, sizeof(buf),
> > > > >"pl011,mmio32,%#lx,%dn8", info.addr,
> > > >
> > > > I suppose we're well past the point where we can delete
> > > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > > trouble down the line (5 years from now when the kernel adopts a new
> > > > preferred way to pass this info) and grows every platform by some amount
> > > > of space every time we add something new here.
> > >
> > > Well firstly, why would you want to delete this command? It is very
> > > useful to be able to change the cmdline.
> > >
> > > This command is only available with BOOTSTD_FULL, which is less than
> > > 10% of boards.
> >
> > Because it's automatic non-obvious stuff. We should not be modifying the
> > command line at all. Is it even documented that we're doing this?
Yes, the command is documented. It only happens when you run the
command. Otherwise the command line is not modified.
As you know U-Boot has bootargs but that doesn't work with extlinux
and the like, so we have this command, which does.
>
> To be clearer, the more I review your changes, the more I see a blurred
> line that I do not this is good between "I found this handy while
> doing something" and "This is a good generic design / feature".
>
> That it can be annoying at times to add the debug uart information is
> not a new unique problem. It's something that's generally normally
> solved. I assume you hit this on x86 where it's more annoying than most.
> But a generic feature it should not have been.
I'm not even sure why you wrote this email, then.
U-Boot has all the information needed to set up the UART correctly and
it is extremely tedious to do otherwise. This is a bootloader, so it
needs to support kernel development. I actually think this is a really
cool feature.
At this point, I really would encourage you to look at what you are
trying to achieve with all these 'no' emails. Collected together they
create a substantial corpus of negativity. Why not just start saying
'yes' instead of 'no' for a while and see how things go?
Regards,
Simon
Re: RISC-V UEFI/ACPI on QEMU regression?
On Wed, 2 Apr 2025 at 17:26, Ilias Apalodimas
wrote:
>
> Hey Bjorn
>
> Long time, hope all is well!
>
> On Wed, 2 Apr 2025 at 16:22, Björn Töpel wrote:
> >
> > Hi,
> >
> > I think I got a regression from commit 53d5a221632e ("emulation: Use
> > bloblist to hold tables"), and v2024.10 for
> > qemu-riscv64_smode_defconfig + acpi.config booting Linux with UEFI.
> >
> > TL;DR: It seems like the RSDP is placed in the wrong EFI memory map
> > type (it should be "ACPI Reclaim").
> >
> > I might also be misunderstanding what config fragments should be
> > used -- I'm out in the weeds here! ;-)
> >
> > When I was using v2024.10, ACPI RSDP was in ACPI Reclaim memory. All
> > good, and e.g. a kexec would properly work. However, when using u-boot
> > with commit 53d5a221632e ("emulation: Use bloblist to hold tables") I
> > get the following when booting, and then kexec:
> >
> > First kernel:
> > [0.00] ACPI: Early table checksum verification disabled
> > [0.00] ACPI:
> > RSDP 0x00047EED3000 24 (v02 BOCHS )
> > Kexec kernel:
> > [0.00] ACPI: Early table checksum verification disabled
> > [0.00] ACPI: 0x00047EED3000 00 (v00
> > )
> > [0.00] Oops - load access fault [#1]
> >
> > RSDP reside in:
> > [0.00] efi: 0x00047ded1000-0x00047eee3fff [Boot Code | |
> > | | | | | | | | | |WB| | | ]
> >
> > (Boot Code vs ACPI Reclaimed)
> >
> > Now to get qemu-riscv64_smode_defconfig + acpi.config to build
> > post-2024.10, I needed to add the following fragments:
> >
> > CONFIG_BLOBLIST=y
> > CONFIG_BLOBLIST_ALLOC=y
> > CONFIG_BLOBLIST_SIZE_RELOC=0x2
> >
> > which is really just a "make the build not complain", guessing game
> > from my side.
> >
> > My guess would be that it's related to the change in
> > evt_write_acpi_tables(), where:
> >
> > -ptr = memalign(SZ_4K, SZ_64K);
> > +ptr = bloblist_add(BLOBLISTT_ACPI_TABLES, SZ_64K, 12);
> >
> > is done.
> >
> > Is my config fragment broken, or is this a proper regression?
>
> I think it's a regression and I think what breaks it is commit cfb4aa2a754ed1
>
> Can you apply the diff below and see if it fixes it for you?
>
> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> index 4422b31ac6a7..afa5eee85484 100644
> --- a/lib/efi_loader/efi_acpi.c
> +++ b/lib/efi_loader/efi_acpi.c
> @@ -34,8 +34,8 @@ efi_status_t efi_acpi_register(void)
> * add_u_boot_and_runtime(). At some point that function could create
> a
> * more detailed map.
> */
> - if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> - return EFI_SUCCESS;
> + //if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> + return EFI_SUCCESS;
>
> /* Mark space used for tables */
> start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
>
> Simon, why that PR got sent with no ACKs from any of the EFI maintainers?
And please fix whatever tooling you use to CC the proper maintainers
next time [0] as I don't see my name in the CC list
[0] https://lore.kernel.org/u-boot/2025011129.245022-27-...@chromium.org/
>
> Thanks
> /Ilias
> >
> >
> > Thanks!
> > Björn
Re: [PATCH] acpi: Fix table length for QEMU
On Thu, 3 Apr 2025 at 08:34, Ilias Apalodimas
wrote:
>
> The end of the ACPI table is set to 'addr' instead of 'end'. The ACPI
> code for QEMU relies on those values to mark memory as 'ACPI Reclaim'
> and as a result the ACPI RSDP ends up in Boot services Data.
>
> Reported-by: bj...@kernel.org
I obviously messed up my reported-by...
> Fixes: commit 638cc363484b ("acpi: enable writing ACPI tables on QEMU")
> Signed-off-by: Ilias Apalodimas
> ---
> drivers/misc/qfw_acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/qfw_acpi.c b/drivers/misc/qfw_acpi.c
> index 0d0cf7646890..77cebae5e3fd 100644
> --- a/drivers/misc/qfw_acpi.c
> +++ b/drivers/misc/qfw_acpi.c
> @@ -305,7 +305,7 @@ static int evt_write_acpi_tables(void)
> /* Generate ACPI tables */
> end = write_acpi_tables(addr);
> gd->arch.table_start = addr;
> - gd->arch.table_end = addr;
> + gd->arch.table_end = end;
>
> return 0;
> }
> --
> 2.49.0
>
[PATCH 0/7] watchdog: at91sam9_wdt driver enhancement
It looks like cover letter was missing... This patch series includes some code refactor and adds new device support for at91sam9_wdt driver. Instead of add a new driver like Linux kernel, at91sam9_wdt driver is extended as new watchdog variant is similar to existing one, especially for the function subset used by u-boot. 1. Remove unused typedef and rename variables for readability. 2. Add SAMA5D4 compatible, it has the same watchdog as SAM9260 except a new lockout feature is added. Currently this feature is unimplemented. 3. SAM9X60, SAM9X7 and SAMA7 series have a new watchdog variant, some bitfields bof MR register shifted their position, a new register is added for timer value. 4. Add DT node to SAM9X60-Currently board It has been tested on SAM9X60-Currently board: - Start & stop - Set timeout from DT node - Reset kick in with a while(1) loop Zixun LI (7): arm: at91: wdt: Remove unused at91_wdt struct arm: at91: wdt: Rename regval in priv data to mr watchdog: at91sam9_wdt: Rename priv to wdt arm: at91: wdt: Add SAM9X60 register definition watchdog: at91sam9_wdt: Add SAM9X60 support ARM: dts: sam9x60: Add watchdog DT node. ARM: dts: at91: sam9x60-curiosity: Enable watchdog node arch/arm/dts/at91-sam9x60_curiosity.dts| 5 ++ arch/arm/dts/sam9x60.dtsi | 7 +++ arch/arm/mach-at91/include/mach/at91_wdt.h | 25 ++ drivers/watchdog/at91sam9_wdt.c| 55 +++--- 4 files changed, 68 insertions(+), 24 deletions(-) -- 2.49.0
Re: RISC-V UEFI/ACPI on QEMU regression?
Hi Ilias,
On Thu, 3 Apr 2025 at 08:27, Ilias Apalodimas
wrote:
>
> On Wed, 2 Apr 2025 at 22:19, Simon Glass wrote:
> >
> > Hi,
> >
> > On Thu, 3 Apr 2025 at 03:36, Ilias Apalodimas
> > wrote:
> > >
> > > On Wed, 2 Apr 2025 at 17:26, Ilias Apalodimas
> > > wrote:
> > > >
> > > > Hey Bjorn
> > > >
> > > > Long time, hope all is well!
> > > >
> > > > On Wed, 2 Apr 2025 at 16:22, Björn Töpel wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > I think I got a regression from commit 53d5a221632e ("emulation: Use
> > > > > bloblist to hold tables"), and v2024.10 for
> > > > > qemu-riscv64_smode_defconfig + acpi.config booting Linux with UEFI.
> > > > >
> > > > > TL;DR: It seems like the RSDP is placed in the wrong EFI memory map
> > > > > type (it should be "ACPI Reclaim").
> > > > >
> > > > > I might also be misunderstanding what config fragments should be
> > > > > used -- I'm out in the weeds here! ;-)
> > > > >
> > > > > When I was using v2024.10, ACPI RSDP was in ACPI Reclaim memory. All
> > > > > good, and e.g. a kexec would properly work. However, when using u-boot
> > > > > with commit 53d5a221632e ("emulation: Use bloblist to hold tables") I
> > > > > get the following when booting, and then kexec:
> > > > >
> > > > > First kernel:
> > > > > [0.00] ACPI: Early table checksum verification disabled
> > > > > [0.00] ACPI:
> > > > > RSDP 0x00047EED3000 24 (v02 BOCHS )
> > > > > Kexec kernel:
> > > > > [0.00] ACPI: Early table checksum verification disabled
> > > > > [0.00] ACPI: 0x00047EED3000 00 (v00
> > > > > )
> > > > > [0.00] Oops - load access fault [#1]
> > > > >
> > > > > RSDP reside in:
> > > > > [0.00] efi: 0x00047ded1000-0x00047eee3fff [Boot Code | |
> > > > > | | | | | | | | | |WB| | | ]
> > > > >
> > > > > (Boot Code vs ACPI Reclaimed)
> > > > >
> > > > > Now to get qemu-riscv64_smode_defconfig + acpi.config to build
> > > > > post-2024.10, I needed to add the following fragments:
> > > > >
> > > > > CONFIG_BLOBLIST=y
> > > > > CONFIG_BLOBLIST_ALLOC=y
> > > > > CONFIG_BLOBLIST_SIZE_RELOC=0x2
> > > > >
> > > > > which is really just a "make the build not complain", guessing game
> > > > > from my side.
> > > > >
> > > > > My guess would be that it's related to the change in
> > > > > evt_write_acpi_tables(), where:
> > > > >
> > > > > -ptr = memalign(SZ_4K, SZ_64K);
> > > > > +ptr = bloblist_add(BLOBLISTT_ACPI_TABLES, SZ_64K, 12);
> > > > >
> > > > > is done.
> > > > >
> > > > > Is my config fragment broken, or is this a proper regression?
> > > >
> > > > I think it's a regression and I think what breaks it is commit
> > > > cfb4aa2a754ed1
> >
> > Yes, that's right. It was reported by Patrick Rudolph in mid-Feb and I
> > sent a patch about a month ago [1]
> >
> > It seems to be in Heinrich's queue in patchwork.
> >
> > > >
> > > > Can you apply the diff below and see if it fixes it for you?
> > > >
> > > > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> > > > index 4422b31ac6a7..afa5eee85484 100644
> > > > --- a/lib/efi_loader/efi_acpi.c
> > > > +++ b/lib/efi_loader/efi_acpi.c
> > > > @@ -34,8 +34,8 @@ efi_status_t efi_acpi_register(void)
> > > > * add_u_boot_and_runtime(). At some point that function could
> > > > create a
> > > > * more detailed map.
> > > > */
> > > > - if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> > > > - return EFI_SUCCESS;
> > > > + //if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> > > > + return EFI_SUCCESS;
> > > >
> > > > /* Mark space used for tables */
> > > > start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
> > > >
> > > > Simon, why that PR got sent with no ACKs from any of the EFI
> > > > maintainers?
> >
> > I didn't send a PR, but it's not clear that anyone would have spotted
> > this bug as it is pretty subtle. It would be good to have testing for
> > this case. We can actually do that by expanding the bootflow_efi()
> > test.
> >
> > >
> > > And please fix whatever tooling you use to CC the proper maintainers
> > > next time [0] as I don't see my name in the CC list
> >
> > You had asked not to be cc'd on my patches anymore,
>
> Are you sure?
>
> > so I just cc
> > Heinrich now, unless I manually override it.
>
> I asked not to be cc'ed on patches that don't apply to -master or
> -next and are for your tree. I obviously want to be CC'ed for anything
> related to subsystems I help maintain.
>
> Thanks
> /Ilias
>
> > Let me know what you'd
> > like me to do.
OK, well unfortunately for now all of my patches are based on my tree.
I would love that to change, but so far it has been my only option for
landing things, like bootstd migrations, EFI-app improvements, PXE
improvements and so on.
I can manually override this for individual patches (as I did with the
TPM patch). I al
Re: [PATCH v2 00/15] Various toolchain compatibility fixes/improvements
On Sat, 15 Mar 2025 15:17:58 -0700, Sam Edwards wrote: > This is v2 of my "misc. fixes" series, sent to prepare the codebase for more > direct LLVM support in the near future. This series contains several fixes > that > I found in the process of preparing that support and which address issues > independent of any future feature or enhancement. I am sending these now, both > so that their inclusion is not delayed by discussion on my upcoming series and > to make the latter more manageable. > > [...] Applied to u-boot/next, thanks! [01/15] arm: Remove stray .mmutable reference in linker script commit: 805377b1f5ae1834e2b1a07dbb6fed94672e0954 [02/15] arm: Exclude eabi_compat from LTO commit: 6ba839a0f5473aac8dc038f8601071234c810119 [03/15] arm: Add __aeabi_memclr in eabi_compat commit: 828484a7740d4ebfed8db85e4b4569cfbfe66cde [04/15] arm: Add aligned-memory aliases to eabi_compat commit: deba40dd0ba452a3f15a359894e6b50494870d0e [05/15] arm: Discard unwanted sections in linker script commit: 16448c443c8cacba7dacb3e919c0b414f70b8a7c [06/15] arm: Replace 'adrl' in EFI crt0 commit: d5734b183c3d578fff1c1e81e46a1d04342edffe [07/15] x86: Fix call64's section flags commit: 8c39dc549b0de155c02a2b39f01dae19775f41a5 [08/15] makefile: Avoid objcopy --gap-fill for .hex/.srec commit: 86838a1ddc8a0e5b5f548a5051e5e68f90fb6660 [09/15] makefile: Add `norelro` linker option commit: 7a8121fe6d314b314531eee7487272601f469c1d [10/15] makefile: Add READELF command variable commit: 586bb720e776396208df399874665ae8c6eb81e8 [11/15] arm: riscv: efi: Export _start symbol from crt0_*_efi stubs commit: f692540b24a7775e43f1d315d3e13a5d3ed21dd4 [12/15] efi_loader: Move .dynamic out of .text in EFI commit: 1755071db7d95fa0b95e9f9bedd3785e2abc10cf [13/15] scripts/Makefile.lib: efi: Preserve the .dynstr section as well commit: 9ca475a6b5da06908a70d1eceb439d480137d69b [14/15] spl: riscv: opensbi: Error on misaligned FDT commit: 17d830cb4b6cdbac56d41938d455820fd7a96a89 [15/15] spl: Align FDT load address commit: 358d1cc232c30091767ce192e74169e7861ae58a -- Tom
Re: [PATCH] onenand: Remove ONENAND_BOOT option
On Thu, 13 Mar 2025 11:17:29 -0600, Tom Rini wrote: > The option ONENAND_BOOT is never set, so remove it. The option > SYS_ONENAND_BOOT was never migrated to Kconfig and any platforms which > supported that have long been removed from the code, so remove the > reference there as well. > > Applied to u-boot/next, thanks! [1/1] onenand: Remove ONENAND_BOOT option commit: 177d73dd17705e77b1830a7354425636f617666d -- Tom
[PATCH] acpi: Fix table length for QEMU
The end of the ACPI table is set to 'addr' instead of 'end'. The ACPI
code for QEMU relies on those values to mark memory as 'ACPI Reclaim'
and as a result the ACPI RSDP ends up in Boot services Data.
Reported-by: bj...@kernel.org
Fixes: commit 638cc363484b ("acpi: enable writing ACPI tables on QEMU")
Signed-off-by: Ilias Apalodimas
---
drivers/misc/qfw_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/qfw_acpi.c b/drivers/misc/qfw_acpi.c
index 0d0cf7646890..77cebae5e3fd 100644
--- a/drivers/misc/qfw_acpi.c
+++ b/drivers/misc/qfw_acpi.c
@@ -305,7 +305,7 @@ static int evt_write_acpi_tables(void)
/* Generate ACPI tables */
end = write_acpi_tables(addr);
gd->arch.table_start = addr;
- gd->arch.table_end = addr;
+ gd->arch.table_end = end;
return 0;
}
--
2.49.0
Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console
On Thu, Apr 03, 2025 at 12:54:42PM +1300, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 3 Apr 2025 at 08:57, Tom Rini wrote:
> >
> > On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> > > On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 3 Apr 2025 at 03:29, Tom Rini wrote:
> > > > >
> > > > > On Sat, Mar 15, 2025 at 02:26:00PM +, Simon Glass wrote:
> > > > > > Update the string to take account of UARTs which are connected on
> > > > > > I/O
> > > > > > ports, as on x86.
> > > > > >
> > > > > > Fix a typo in an error message in the same command, while we are
> > > > > > here.
> > > > > >
> > > > > > Signed-off-by: Simon Glass
> > > > > > ---
> > > > > >
> > > > > > (no changes since v3)
> > > > > >
> > > > > > Changes in v3:
> > > > > > - Add new patch to support IO UARTs for earlycon and console
> > > > > >
> > > > > > boot/bootflow.c | 7 ---
> > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > > > index 58a1afa7a75..4054a966af8 100644
> > > > > > --- a/boot/bootflow.c
> > > > > > +++ b/boot/bootflow.c
> > > > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow
> > > > > > *bflow, const char *arg)
> > > > > > *buf = '\0';
> > > > > > if (!strcmp("earlycon", arg) && info.type ==
> > > > > > SERIAL_CHIP_16550_COMPATIBLE) {
> > > > > > snprintf(buf, sizeof(buf),
> > > > > > - "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > > > - info.baudrate);
> > > > > > + "uart8250,%s,%#lx,%dn8",
> > > > > > + info.addr_space == SERIAL_ADDRESS_SPACE_IO ?
> > > > > > "io" :
> > > > > > + "mmio", info.addr, info.baudrate);
> > > > > > } else if (!strcmp("earlycon", arg) && info.type ==
> > > > > > SERIAL_CHIP_PL01X) {
> > > > > > snprintf(buf, sizeof(buf),
> > > > > >"pl011,mmio32,%#lx,%dn8", info.addr,
> > > > >
> > > > > I suppose we're well past the point where we can delete
> > > > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > > > trouble down the line (5 years from now when the kernel adopts a new
> > > > > preferred way to pass this info) and grows every platform by some
> > > > > amount
> > > > > of space every time we add something new here.
> > > >
> > > > Well firstly, why would you want to delete this command? It is very
> > > > useful to be able to change the cmdline.
> > > >
> > > > This command is only available with BOOTSTD_FULL, which is less than
> > > > 10% of boards.
> > >
> > > Because it's automatic non-obvious stuff. We should not be modifying the
> > > command line at all. Is it even documented that we're doing this?
>
> Yes, the command is documented. It only happens when you run the
> command. Otherwise the command line is not modified.
>
> As you know U-Boot has bootargs but that doesn't work with extlinux
> and the like, so we have this command, which does.
>
> >
> > To be clearer, the more I review your changes, the more I see a blurred
> > line that I do not this is good between "I found this handy while
> > doing something" and "This is a good generic design / feature".
> >
> > That it can be annoying at times to add the debug uart information is
> > not a new unique problem. It's something that's generally normally
> > solved. I assume you hit this on x86 where it's more annoying than most.
> > But a generic feature it should not have been.
>
> I'm not even sure why you wrote this email, then.
Because this is another little thing I'm being flexible on and not
naking, I just think it's wrong-headed. And you seem to think I'm never
being flexible. So I started this by saying I'm not naking it, I just
think it's wrong.
> U-Boot has all the information needed to set up the UART correctly and
> it is extremely tedious to do otherwise. This is a bootloader, so it
> needs to support kernel development. I actually think this is a really
> cool feature.
It's "cool" until it breaks. And grows platforms for features they
can't/won't support.
--
Tom
signature.asc
Description: PGP signature
[PATCH] configs: meson64: move DFU step at the end to give the board a chance to boot something on storage
The DFU was set to run at the beginning, but we may want to boot something over USB, MMC or Ethernet even if booted over USB, so move the DFU as the final fallback. This keeps the current Amlogic U-Boot CI working and makes the default config more generic. Signed-off-by: Neil Armstrong --- include/configs/meson64.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/configs/meson64.h b/include/configs/meson64.h index f3275b37a516da6befd41324ddb2a3fe1f36fc83..675df623311f361f3a55971c3894ffbe1d07fee6 100644 --- a/include/configs/meson64.h +++ b/include/configs/meson64.h @@ -118,13 +118,13 @@ #ifndef BOOT_TARGET_DEVICES #define BOOT_TARGET_DEVICES(func) \ func(ROMUSB, romusb, na) \ - func(USB_DFU, usbdfu, na) \ BOOT_TARGET_MMC(func) \ BOOT_TARGET_DEVICES_USB(func) \ BOOT_TARGET_NVME(func) \ BOOT_TARGET_SCSI(func) \ BOOT_TARGET_PXE(func) \ - BOOT_TARGET_DHCP(func) + BOOT_TARGET_DHCP(func) \ + func(USB_DFU, usbdfu, na) #endif #define BOOTM_SIZE __stringify(0x170) --- base-commit: c17f03a7e93dfbbe5d32f5738274187065d9493f change-id: 20250402-u-boot-meson64-move-dfu-end-eb9f5fe55608 Best regards, -- Neil Armstrong
Re: [PATCH 02/14] net: airoha: Add Airoha Ethernet driver
On Wed, Apr 02, 2025 at 12:51:34AM +0200, Christian Marangi wrote: > Add airoha Ethernet driver for Airoha AN7581 SoC. This is a majorly > rewritten and simplified version of the Linux airoha_eth.c driver. > > It's has been modified to support a single RX/TX ring to reflect U-Boot > implementation with recv and send API. > > The struct and the define are kept as similar as possible to upstream > one to not diverge too much. > > The AN7581 SoC include an Ethernet Switch based on the Mediatek MT753x > but doesn't require any modification aside from setting the CPU port and > applying the Flood configuration hence it can be handled entirely in the > Ethernet driver. > > Signed-off-by: Christian Marangi > --- > drivers/net/Kconfig |8 + > drivers/net/Makefile |1 + > drivers/net/airoha_eth.c | 1448 ++ > 3 files changed, 1457 insertions(+) > create mode 100644 drivers/net/airoha_eth.c checkpatch.pl has some macro warnings and it looks like some inconsistent spacing around '#define FOO' vs '#defineFOO' ? And are all of those defines needed? -- Tom signature.asc Description: PGP signature
Re: [PATCH] firmware: ti_sci: Scan all device instances when releasing exclusive devices
On 11:57-20250402, Dhruva Gole wrote:
[...]
> >
> > As a result of this, the cleanup we intent to do with
>
> s/intent/intend
Thanks. will pick it up in a respin.
>
[...]
> > To fix this, let us make ti_sci_cmd_release_exclusive_devices scan the
> > all registrations of tisci instances and cleanup all exclusive devices
> > that have ever been registered.
>
> Firstly, it seems to me like this issue is mostly specific to the
> AM64/ DM+TIFS combined core arch K3 devices? So it would be good if the
> commit message can specify that for better context. People who are more
> used to the R5 core running DM architecture may find this description
> slightly confusing as there's no TI SCI calls that the R5 SPL can make
> to DM since DM isn't up yet on those devices...
This is not related to TI_SCI. As described in commit message, it is just
the multi_fit_dtb handling challenges. Combined DMSC or split DM arch
devices are probably both susceptible to this. That said, i have tested
this on am64x.
[...]
> > index 190a1e3f5fc3..54d6689ce783 100644
> > --- a/drivers/firmware/ti_sci.c
> > +++ b/drivers/firmware/ti_sci.c
> > @@ -696,20 +696,25 @@ static int ti_sci_cmd_put_device(const struct
> > ti_sci_handle *handle, u32 id)
> >MSG_DEVICE_SW_STATE_AUTO_OFF);
> > }
> >
> > -static
> > -int ti_sci_cmd_release_exclusive_devices(const struct ti_sci_handle
> > *handle)
> > +static int ti_sci_cmd_release_exclusive_devices(void)
> > {
> > struct ti_sci_exclusive_dev *dev, *tmp;
> > struct ti_sci_info *info;
> > int i, cnt;
> >
> > - info = handle_to_ti_sci_info(handle);
> > -
> > - list_for_each_entry_safe(dev, tmp, &info->dev_list, list) {
> > - cnt = dev->count;
> > - debug("%s: id = %d, cnt = %d\n", __func__, dev->id, cnt);
> > - for (i = 0; i < cnt; i++)
> > - ti_sci_cmd_put_device(handle, dev->id);
> > + /*
> > +* Scan all ti_sci_list registrations, since with FIT images, we could
> > +* have started with one device tree registration and switched over
> > +* to a final version. This prevents exclusive devices identified
> > +* during the first probe to be left orphan.
> > +*/
> > + list_for_each_entry(info, &ti_sci_list, list) {
> > + list_for_each_entry_safe(dev, tmp, &info->dev_list, list) {
> > + cnt = dev->count;
> > + debug("%s: id = %d, cnt = %d\n", __func__, dev->id,
> > cnt);
> > + for (i = 0; i < cnt; i++)
> > + ti_sci_cmd_put_device(&info->handle, dev->id);
> > + }
>
> The other thing I am slightly not comfortable with is this.
> The release_exclusive call is going to be common to every K3 device.
> With your changes, will it result in any sort of increased boot time
> penalty? I feel like it might, unless I maybe mistaken.
It should not. for non-multi-dtb FIT, this logic just flows only 1 list
which is all that is present. for multi-dtb FIT images, it fixes a real
problem.
>
> Also, to fix just AM64 style devices, we're touching a K3 common piece
> of code like TI SCI. The issue that you described about the TI SCI
> probing the second time causing issues seems more fundamental than the
> fix being offered here.
As described in commit message, calling probe twice is Normal since
there are two dtbs involved in the FIT image (one with which we
identify what is the board and use the information to pick the correct
dtb).
> Quoting the commit message,
>
> > At this stage, the exclusive devices such as i2c instances used to
> > probe the board information is left in the old info->dev_list that is
> > no longer used actively by the system using the replaced dtb.
>
> Can we instead try and fix the TI SCI driver behaviour from the first time?
> Like is it possible to implement a .remove for the TI SCI driver such
> that when the old dtb is now out of context, the driver gets cleanly
> removed, and with that calls the release_exclusive of all devices that
> it requested?
Tried .remove and .unbind and enabling CONFIG_SPL_DM_DEVICE_REMOVE but
then we run into all kind of other dependency issues on dm_uninit
> That way the new instance of TI SCI driver doesn't need to worry about
> doing cleanups from any of it's previous instances right?
>
> To me that sounds like the right root cause to try and fix. Let me know
> what are your thoughts on this.
I am open to any approach :) - just dont seem to find one that
Re: [PATCH 02/14] net: airoha: Add Airoha Ethernet driver
On Wed, Apr 02, 2025 at 05:21:49PM +0200, Christian Marangi wrote: > On Wed, Apr 02, 2025 at 09:19:51AM -0600, Tom Rini wrote: > > On Wed, Apr 02, 2025 at 12:51:34AM +0200, Christian Marangi wrote: > > > > > Add airoha Ethernet driver for Airoha AN7581 SoC. This is a majorly > > > rewritten and simplified version of the Linux airoha_eth.c driver. > > > > > > It's has been modified to support a single RX/TX ring to reflect U-Boot > > > implementation with recv and send API. > > > > > > The struct and the define are kept as similar as possible to upstream > > > one to not diverge too much. > > > > > > The AN7581 SoC include an Ethernet Switch based on the Mediatek MT753x > > > but doesn't require any modification aside from setting the CPU port and > > > applying the Flood configuration hence it can be handled entirely in the > > > Ethernet driver. > > > > > > Signed-off-by: Christian Marangi > > > --- > > > drivers/net/Kconfig |8 + > > > drivers/net/Makefile |1 + > > > drivers/net/airoha_eth.c | 1448 ++ > > > 3 files changed, 1457 insertions(+) > > > create mode 100644 drivers/net/airoha_eth.c > > > > checkpatch.pl has some macro warnings and it looks like some > > inconsistent spacing around '#define FOO' vs '#defineFOO' ? And are > > all of those defines needed? > > > > No they are not all needed, I defined them for consistency with the > original driver. Should I drop them? > > For the checkpatch check, also comes from the original driver. I guess if it will make long term re-syncs easier, we can ignore them. But the macro thing should likely be addressed upstream too? -- Tom signature.asc Description: PGP signature
Re: [PATCH 02/14] net: airoha: Add Airoha Ethernet driver
On Wed, Apr 02, 2025 at 09:31:04AM -0600, Tom Rini wrote: > On Wed, Apr 02, 2025 at 05:21:49PM +0200, Christian Marangi wrote: > > On Wed, Apr 02, 2025 at 09:19:51AM -0600, Tom Rini wrote: > > > On Wed, Apr 02, 2025 at 12:51:34AM +0200, Christian Marangi wrote: > > > > > > > Add airoha Ethernet driver for Airoha AN7581 SoC. This is a majorly > > > > rewritten and simplified version of the Linux airoha_eth.c driver. > > > > > > > > It's has been modified to support a single RX/TX ring to reflect U-Boot > > > > implementation with recv and send API. > > > > > > > > The struct and the define are kept as similar as possible to upstream > > > > one to not diverge too much. > > > > > > > > The AN7581 SoC include an Ethernet Switch based on the Mediatek MT753x > > > > but doesn't require any modification aside from setting the CPU port and > > > > applying the Flood configuration hence it can be handled entirely in the > > > > Ethernet driver. > > > > > > > > Signed-off-by: Christian Marangi > > > > --- > > > > drivers/net/Kconfig |8 + > > > > drivers/net/Makefile |1 + > > > > drivers/net/airoha_eth.c | 1448 ++ > > > > 3 files changed, 1457 insertions(+) > > > > create mode 100644 drivers/net/airoha_eth.c > > > > > > checkpatch.pl has some macro warnings and it looks like some > > > inconsistent spacing around '#define FOO' vs '#defineFOO' ? And are > > > all of those defines needed? > > > > > > > No they are not all needed, I defined them for consistency with the > > original driver. Should I drop them? > > > > For the checkpatch check, also comes from the original driver. > > I guess if it will make long term re-syncs easier, we can ignore them. > But the macro thing should likely be addressed upstream too? > Ehh ethernet driver are always very different compared to the upstream version. I think I will just remove additional define not needed in v2. -- Ansuel
[PATCH 1/2] net-lwip: wget: add LMB and buffer checks
Legacy NET wget invokes a store_block() function which performs buffer
validation (LMB, address wrapping). Do the same with NET_LWIP.
Signed-off-by: Jerome Forissier
Suggested-by: Sughosh Ganu
---
net/lwip/wget.c | 48
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/net/lwip/wget.c b/net/lwip/wget.c
index 14f27d42998..51fe745f5cc 100644
--- a/net/lwip/wget.c
+++ b/net/lwip/wget.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include "lwip/altcp_tls.h"
#include
@@ -201,11 +202,44 @@ static int parse_legacy_arg(char *arg, char *nurl, size_t
rem)
return 0;
}
+static int store_block(struct wget_ctx *ctx, void *src, u16_t len)
+{
+ ulong store_addr = ctx->daddr;
+ uchar *ptr;
+
+ /* Avoid overflow */
+ if (wget_info->buffer_size && wget_info->buffer_size < ctx->size + len)
+ return -1;
+
+ if (CONFIG_IS_ENABLED(LMB) && wget_info->set_bootdev) {
+ if (store_addr + len < store_addr ||
+ lmb_read_check(store_addr, len)) {
+ printf("\nwget error: ");
+ printf("trying to overwrite reserved memory...\n");
+ return -1;
+ }
+ }
+
+ ptr = map_sysmem(store_addr, len);
+ memcpy(ptr, src, len);
+ unmap_sysmem(ptr);
+
+ ctx->daddr += len;
+ ctx->size += len;
+ if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
+ printf("#");
+ ctx->prevsize = ctx->size;
+ }
+
+ return 0;
+}
+
static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
err_t err)
{
struct wget_ctx *ctx = arg;
struct pbuf *buf;
+ err_t ret;
if (!pbuf)
return ERR_BUF;
@@ -214,18 +248,16 @@ static err_t httpc_recv_cb(void *arg, struct altcp_pcb
*pcb, struct pbuf *pbuf,
ctx->start_time = get_timer(0);
for (buf = pbuf; buf; buf = buf->next) {
- memcpy((void *)ctx->daddr, buf->payload, buf->len);
- ctx->daddr += buf->len;
- ctx->size += buf->len;
- if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
- printf("#");
- ctx->prevsize = ctx->size;
+ if (store_block(ctx, buf->payload, buf->len) < 0) {
+ ret = ERR_BUF;
+ goto out;
}
}
-
altcp_recved(pcb, pbuf->tot_len);
+ ret = ERR_OK;
+out:
pbuf_free(pbuf);
- return ERR_OK;
+ return ret;
}
static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
--
2.43.0
[PATCH 2/2] net-lwip: tftp: add LMB and buffer checks
Legacy NET tftp invokes a store_block() function which performs buffer
validation (LMB, address wrapping). Do the same with NET_LWIP.
Signed-off-by: Jerome Forissier
Suggested-by: Sughosh Ganu
---
net/lwip/tftp.c | 45 ++---
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/net/lwip/tftp.c b/net/lwip/tftp.c
index 123d66b5dba..38aa647df5c 100644
--- a/net/lwip/tftp.c
+++ b/net/lwip/tftp.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -31,6 +32,36 @@ struct tftp_ctx {
enum done_state done;
};
+static int store_block(struct tftp_ctx *ctx, void *src, u16_t len)
+{
+ ulong store_addr = ctx->daddr;
+ void *ptr;
+
+ if (CONFIG_IS_ENABLED(LMB)) {
+ if (store_addr + len < store_addr ||
+ lmb_read_check(store_addr, len)) {
+ puts("\nTFTP error: ");
+ puts("trying to overwrite reserved memory...\n");
+ return -1;
+ }
+ }
+
+ ptr = map_sysmem(store_addr, len);
+ memcpy(ptr, src, len);
+ unmap_sysmem(ptr);
+
+ ctx->daddr += len;
+ ctx->size += len;
+ ctx->block_count++;
+ if (ctx->block_count % 10 == 0) {
+ putc('#');
+ if (ctx->block_count % (65 * 10) == 0)
+ puts("\n\t ");
+ }
+
+ return 0;
+}
+
static void *tftp_open(const char *fname, const char *mode, u8_t is_write)
{
return NULL;
@@ -71,17 +102,9 @@ static int tftp_write(void *handle, struct pbuf *p)
struct tftp_ctx *ctx = handle;
struct pbuf *q;
- for (q = p; q; q = q->next) {
- memcpy((void *)ctx->daddr, q->payload, q->len);
- ctx->daddr += q->len;
- ctx->size += q->len;
- ctx->block_count++;
- if (ctx->block_count % 10 == 0) {
- putc('#');
- if (ctx->block_count % (65 * 10) == 0)
- puts("\n\t ");
- }
- }
+ for (q = p; q; q = q->next)
+ if (store_block(ctx, q->payload, q->len) < 0)
+ return -1;
return 0;
}
--
2.43.0
[PATCH 0/2] NET_LWIP LMB fixes
Two small patches fixing issues with tftp and wget when the network stack is NET_LWIP and LMB is enabled. Jerome Forissier (2): net-lwip: wget: add LMB and buffer checks net-lwip: tftp: add LMB and buffer checks net/lwip/tftp.c | 45 ++--- net/lwip/wget.c | 48 2 files changed, 74 insertions(+), 19 deletions(-) -- 2.43.0
Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console
On Sat, Mar 15, 2025 at 02:26:00PM +, Simon Glass wrote:
> Update the string to take account of UARTs which are connected on I/O
> ports, as on x86.
>
> Fix a typo in an error message in the same command, while we are here.
>
> Signed-off-by: Simon Glass
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Add new patch to support IO UARTs for earlycon and console
>
> boot/bootflow.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 58a1afa7a75..4054a966af8 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow, const
> char *arg)
> *buf = '\0';
> if (!strcmp("earlycon", arg) && info.type ==
> SERIAL_CHIP_16550_COMPATIBLE) {
> snprintf(buf, sizeof(buf),
> - "uart8250,mmio32,%#lx,%dn8", info.addr,
> - info.baudrate);
> + "uart8250,%s,%#lx,%dn8",
> + info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> + "mmio", info.addr, info.baudrate);
> } else if (!strcmp("earlycon", arg) && info.type == SERIAL_CHIP_PL01X) {
> snprintf(buf, sizeof(buf),
>"pl011,mmio32,%#lx,%dn8", info.addr,
I suppose we're well past the point where we can delete
bootflow_cmdline_auto() itself because that's just going to lead us to
trouble down the line (5 years from now when the kernel adopts a new
preferred way to pass this info) and grows every platform by some amount
of space every time we add something new here.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 00/13] video: Enhancements related to truetype and console
Hi Tom, On Thu, 3 Apr 2025 at 04:00, Tom Rini wrote: > > On Wed, Apr 02, 2025 at 01:08:56PM +1300, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 2 Apr 2025 at 10:41, Tom Rini wrote: > > > > > > On Wed, Apr 02, 2025 at 06:29:31AM +1300, Simon Glass wrote: > > > > > > > This series includes some precursor patches needed for forthcoming expo > > > > enhancements. > > > > > > > > - truetype support for multiple lines > > > > - make white-on-black a runtime option > > > > - support drawing a rectangle > > > > > > > > This series was split out from schd2 > > > [snip] > > > > base-commit: 3f76d803db9b500f43bc534465945a8d2836bb3e > > > > branch: schg > > > > > > I get as far as patch 4 before it doesn't apply to -next. > > > > Your tree is missing the video-damage series which adds some more > > tests. It is here if you want to pick it up: > > > > https://patchwork.ozlabs.org/project/uboot/list/?series=369663&state=* > > > > I can rebase and send a PR if you prefer. > > Reading the cover letter again, that doesn't seem ready to merge either. It seems OK to me. What are your concerns? Regards, Simon
Re: [PATCH] tpm: cr50: Support opening the TPM multiple times
HI Ilias,
On Wed, 2 Apr 2025 at 19:20, Ilias Apalodimas
wrote:
>
> Hi Simon,
>
> On Wed, 2 Apr 2025 at 00:28, Simon Glass wrote:
> >
> > The tpm_auto_start() function is used in tests and assumes that it can
> > open the TPM even if it is already open and a locality claimed. The cr50
> > driver does not use the common TPM2 TIS code so lacks a check for the
> > is_open field of struct tpm_chip and in fact it doesn't use that struct.
> >
> > Add an equivalent check to cr50_i2c_open().
> >
> > This fixes tpm_auto_start() on coral.
>
> Mind if I change this on the commit. it doesn't fix tpm_auto_start()
> it fixes *all* init on that TPM because tpm init && tpminit hangs as
> well
>
> Other than that
>
> Reviewed-by: Ilias Apalodimas
OK thanks, yes please go ahead.
>
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > drivers/tpm/cr50_i2c.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/tpm/cr50_i2c.c b/drivers/tpm/cr50_i2c.c
> > index 08ec179346e..5b2d5ccb146 100644
> > --- a/drivers/tpm/cr50_i2c.c
> > +++ b/drivers/tpm/cr50_i2c.c
> > @@ -737,9 +737,13 @@ static int cr50_i2c_report_state(struct udevice *dev,
> > char *str, int str_max)
> >
> > static int cr50_i2c_open(struct udevice *dev)
> > {
> > + struct cr50_priv *priv = dev_get_priv(dev);
> > char buf[80];
> > int ret;
> >
> > + if (priv->locality != -1)
> > + return -EBUSY;
> > +
> > ret = process_reset(dev);
> > if (ret)
> > return log_msg_ret("reset", ret);
> > --
> > 2.43.0
> >
> > base-commit: e0695b08a71e167bf5ce83ae25acfdebca554b8d
> > branch: tpm
Regards,
Simon
Re: Rate of innovation in the project (Was: Re: Rate of change in the project)
Hi Tom, On Wed, 2 Apr 2025 at 06:18, Tom Rini wrote: > > On Wed, Apr 02, 2025 at 04:45:37AM +1300, Simon Glass wrote: > > Hi Tom, > > > > On Tue, 1 Apr 2025 at 04:51, Tom Rini wrote: > > > > > > On Fri, Mar 28, 2025 at 11:42:20PM +, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Mon, 10 Mar 2025 at 09:53, Tom Rini wrote: > > > > > > > > > > On Fri, Mar 07, 2025 at 08:46:31AM -0600, Tom Rini wrote: > > > > > > On Thu, Mar 06, 2025 at 09:10:47AM -0700, Simon Glass wrote: > > > > > [snip] > > > > > > > Again, back to this thread, if you want me to migrate things, > > > > > > > consider > > > > > > > applying the sunxi patches as I have described above. I will then > > > > > > > look > > > > > > > at the next target for bootstd. But while you hold this up, I > > > > > > > cannot > > > > > > > move forward with more bootstd migration. It doesn't seem that > > > > > > > much to > > > > > > > ask. > > > > > > > > > > > > I will take another look at what's still relevant. But I believe > > > > > > it's > > > > > > still blocked on the fact that it changes behavior and breaks users. > > > > > > > > > > In details: > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-2-...@chromium.org/ > > > > > > > > > > Now that the underlying BLK problem is resolved, this can just be > > > > > dropped I believe. Removing the BLK dependency from BOOTSTD can happen > > > > > when you're supporting a flow that lacks a BLK device entirely. This > > > > > would be another reminder as to why putting unrelated changes in a > > > > > series is a problem. > > > > > > > > OK, then just don't apply this patch. Problem solved? > > > > > > Well, for a hypothetical v6 you would not include it, sure. > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-3-...@chromium.org/ > > > > > > > > > > This is fine. > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-4-...@chromium.org/ > > > > > > > > > > This is not fine. This is also not a sunxi problem. It means that we > > > > > should drop bootmgr from rockchip, where the conversion has already > > > > > taken place, and would need to drop it from future conversion too. > > > > > Neither of which are desirable changes. > > > > > > > > Why do you say that? I don't understand how this relates to rockchip > > > > or why we would need to drop bootmgr from that. > > > > > > Then you don't have enough of a grasp of the details of the area you're > > > trying to solve problems in? Or maybe you need to refresh yourself on > > > the details of the area you're trying to work in. > > > > Or perhaps there isn't a problem? The sunxi case is special because we > > have a FEL boothmeth. That isn't present on rockchip, for example. > > Again, you seem to have forgotten what the problems with the series > were. No, it's just that we disagree on the path forward. > > > > > > This patch in particular is > > > > > where we have the note: > > > > > > > > > > "Yes, the introduction of boot standard changed the boot order and > > > > > specifically deprioritizing scripts is unexpected." > > > > > > > > > > Which should have had more attention than it did. > > > > > > > > From memory the scripts are a fallback used when the other things > > > > don't work, so that was the decision I made at the time. > > > > > > The key point being we changed behavior that others depended on, and > > > didn't document it well and didn't make sure the change would work for > > > them either. > > > > > > > > This is the thread that to me spelled out in details where the > > > > > conversions are now blocked. We changed behavior and that in turn > > > > > breaks > > > > > users that have come to rely on ordering. > > > > > > > > I don't know what action to take on that comment. We cannot have 100% > > > > backwards compatibility with the scripts, which after all weren't even > > > > documented. But it is very close. > > > > > > You need to get feedback from the people you want to migrate from the > > > scripts and ordering and rely on to something else and see what works > > > for them. > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-5-...@chromium.org/ > > > > > > > > > > Is an alternative to the above which then turned in to a discussion > > > > > that > > > > > I would very briefly summarize as "no discussions were had between > > > > > stakeholders before integrating efi bootmgr with bootstd". > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-6-...@chromium.org/ > > > > > > > > > > This is fine, but only relevant once migration happens. > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-7-...@chromium.org/ > > > > > > > > > > If Andre is fine with this, this is fine. > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-8-...@chromium.org/ > >
Re: [RFC PATCH 1/2 v2] tools: binman: control.py: Propagate bootph-* properties to supernodes
Hi Moteen,
On Wed, 2 Apr 2025 at 22:01, Moteen Shah wrote:
>
> Hey Simon,
>
> On 29/03/25 05:17, Simon Glass wrote:
> > Hi Moteen,
> >
> > On Thu, 27 Mar 2025 at 08:06, Moteen Shah wrote:
> >> Add a function to scan through all the nodes in the device-tree
> >> recusively for bootph-* property. If found, propagate it to all
> >> of its parent nodes up the hierarchy.
> >>
> >> Signed-off-by: Moteen Shah
> >> ---
> >> tools/binman/control.py | 35 ++-
> >> 1 file changed, 34 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/binman/control.py b/tools/binman/control.py
> >> index e73c598298c..e739949d366 100644
> >> --- a/tools/binman/control.py
> >> +++ b/tools/binman/control.py
> >> @@ -526,6 +526,35 @@ def _RemoveTemplates(parent):
> >> if node.name.startswith('template'):
> >> node.Delete()
> >>
> >> +def prop_bootph_to_parent(node, prop, dtb):
> > I think the 'prop_' is a bit confusing, since you are dealing with
> > properties! How about 'add_' as the prefix?
>
> 'add_' should be more descriptive, will add that in v3.
>
> >
> >> +"""Propagates bootph-* property to all the parent
> >> +nodes up the hierarchy
> > First line should be a summary
> >
> > then blank line
> >
> > then describe the args...you can see this if you look at a few other
> > functions.
>
>
> Noted, will rectify in v3.
>
> >> +"""
> >> +parent = node.parent
> >> +if parent == None or parent.props.get(prop):
> > if not parent or ...
>
>
> Noted.
>
>
> >
> >> + return
> >> +
> >> +while parent:
> >> +parent.AddEmptyProp(prop, 0)
> >> +parent = parent.parent
> >> +
> >> +def scan_and_prop_bootph(node, dtb):
> >> +"""Scan the device tree and set the bootph-* property if its present
> >> +in subnode
> >> +
> >> +This is used to set the bootph-* property in the parent node if a
> >> +"bootph-*" property is found in any of the subnodes of the parent
> >> +node.
> > comment style again
> >
> >> +"""
> >> +bootph_prop = ['bootph-all', 'bootph-some-ram', 'bootph-pre-ram',
> >> 'bootph-pre-sram']
> > From my understanding the only ones that matter are bootph-all and
> > bootph-some-ram, since the SPL ones are handled by fdtgrep.
>
>
> Noted.
>
>
> >
> >> +
> >> +for props in bootph_prop:
> > for prop
> >
> > (since it is just one)
> >
> >> +if node.props.get(props):
> >> +prop_bootph_to_parent(node, props, dtb)
> >> +
> >> +for subnode in node.subnodes:
> >> + scan_and_prop_bootph(subnode, dtb)
> >> +
> >> def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt,
> >> use_expanded, indir):
> >> """Prepare the images to be processed and select the device tree
> >>
> >> @@ -563,7 +592,11 @@ def PrepareImagesAndDtbs(dtb_fname, select_images,
> >> update_fdt, use_expanded, ind
> >> indir = []
> >> dtb_fname = fdt_util.EnsureCompiled(dtb_fname, indir=indir)
> >> fname = tools.get_output_filename('u-boot.dtb.out')
> >> -tools.write_file(fname, tools.read_file(dtb_fname))
> >> +dtb = fdt.FdtScan(dtb_fname)
> >> +scan_and_prop_bootph(dtb.GetRoot(), dtb)
> >> +dtb.Sync(True)
> >> +tools.write_file(dtb_fname, dtb.GetContents())
> >> +tools.write_file(fname, dtb.GetContents())
> > You shouldn't write back to the file created by the build. Do you need this?
>
>
> Had the same thought, but the build fails in non clean builds[0]. Did a
> workaround[1] but then some essential template nodes ends up getting
> deleted from u-boot.dtb. Finally, had to write back to the file
> to resolve the issue.
Can you push a tree somewhere. This could be a bug that I could fix.
>
> >> dtb = fdt.FdtScan(fname)
> >>
> >> node = _FindBinmanNode(dtb)
> >> --
> >> 2.34.1
> >>
> > The code looks fine to me apart from the nits.
> >
> > This addition needs a test - see ftest.py for some examples there. But
> > basically just create a dts that has some props in it, then check that
> > they got added.
>
>
> Included a test as well in the patchset[3]. Do let me know if there are
> some changes required in it.
Somehow I'm not seeing that in patchwork. It looks good but please try
to keep lines <=80 cols.
>
> >
> > I think testSimpleFitEncryptedData() could be a good example?
> >
> > Regards,
> > Simon
>
> References:
> [0]
> https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-non-clean-build
> [1]
> https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-patch-tools-dtdoc-fdt-py
> [3] https://lore.kernel.org/u-boot/20250327080642.2269856-3-m-s...@ti.com/
>
> Thanks for the review.
Regards,
Simon
Re: [PATCH 3/6] test: Add a test for strim()
Hi Tom,
On Fri, 21 Mar 2025 at 03:15, Tom Rini wrote:
>
> On Thu, Mar 20, 2025 at 03:43:30AM +, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 19 Mar 2025 at 16:38, Tom Rini wrote:
> > >
> > > On Wed, Mar 19, 2025 at 03:04:16PM +, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 19 Mar 2025 at 15:24, Tom Rini wrote:
> > > > >
> > > > > On Wed, Mar 19, 2025 at 12:59:05PM +0100, Simon Glass wrote:
> > > > >
> > > > > > This function trims whitespace from the start and end of a string.
> > > > > > Add a
> > > > > > test for it.
> > > > > >
> > > > > > Signed-off-by: Simon Glass
> > > > > > ---
> > > > > >
> > > > > > test/lib/string.c | 31 +++
> > > > > > 1 file changed, 31 insertions(+)
> > > > >
> > > > > This got me to ask "What even is using strim and where did it come
> > > > > from?". To which the answer is:
> > > > > - A few places, but it's probably reasonable.
> > > > > - Linux, pre-2011.
> > > > >
> > > > > I say the latter because we're missing a bug fix to the strim function
> > > > > that's been there since 2011:
> > > > >
> > > > > commit 66f6958e69d8055277356d3cc2e7a1d734db1755
> > > > > Author: Michael Holzheu
> > > > > Date: Mon Oct 31 17:12:37 2011 -0700
> > > > >
> > > > > lib/string.c: fix strim() semantics for strings that have only
> > > > > blanks
> > > > >
> > > > > Commit 84c95c9acf0 ("string: on strstrip(), first remove leading
> > > > > spaces
> > > > > before running over str") improved the performance of the strim()
> > > > > function.
> > > > >
> > > > > Unfortunately this changed the semantics of strim() and broke my
> > > > > code.
> > > > > Before the patch it was possible to use strim() without using the
> > > > > return
> > > > > value for removing trailing spaces from strings that had either
> > > > > only
> > > > > blanks or only trailing blanks.
> > > > >
> > > > > Now this does not work any longer for strings that *only* have
> > > > > blanks.
> > > > >
> > > > > Before patch: " " -> ""(empty string)
> > > > > After patch: " " -> " " (no change)
> > > > >
> > > > > I think we should remove your patch to restore the old behavior.
> > > > >
> > > > > The description (lib/string.c):
> > > > >
> > > > > * Note that the first trailing whitespace is replaced with a
> > > > > %NUL-terminator
> > > > >
> > > > > => The first trailing whitespace of a string that only has
> > > > > whitespace
> > > > >characters is the first whitespace
> > > > >
> > > > > The patch restores the old strim() semantics.
> > > > >
> > > > > Signed-off-by: Michael Holzheu
> > > > > Cc: Andre Goddard Rosa
> > > > > Cc: Martin Schwidefsky
> > > > > Cc: Heiko Carstens
> > > > > Signed-off-by: Andrew Morton
> > > > > Signed-off-by: Linus Torvalds
> > > > >
> > > > > --
> > > > > Tom
> > > >
> > > > Here is the comment for the function, of note given Linux's disdain
> > > > for commenting code:
> > > >
> > > > /**
> > > > * strim - Removes leading and trailing whitespace from @s.
> > > > * @s: The string to be stripped.
> > > > *
> > > > * Note that the first trailing whitespace is replaced with a
> > > > %NUL-terminator
> > > > * in the given string @s. Returns a pointer to the first non-whitespace
> > > > * character in @s.
> > > > */
> > > >
> > > > Given that comment, I don't see a bug here. But of course we could add
> > > > a test for it and adjust the function too. PLMK.
> > >
> > > Did your test add a testcase for the situation described above?
> >
> > No. I didn't know about that case. The function comment does not
> > suggest that it handles a whitespace-only string in that way. In fact,
> > even the Linux commit which changes the behaviour doesn't update the
> > comment to mention that behaviour.
> >
> > Anyway, let me know what you'd like to do.
>
> Well, being a function we borrow from the linux kernel, we should follow
> how it works and backport the trivial change to match how it behaves,
> then add tests in 2/2.
OK.
Regards,
Simon
Re: [PATCH v5 38/46] boot: Consider non-bootable partitions
Hi Tom,
On Thu, 3 Apr 2025 at 03:28, Tom Rini wrote:
>
> On Sat, Mar 15, 2025 at 02:25:58PM +, Simon Glass wrote:
>
> > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > bootable partitions for that media. This can mean that extlinux.conf
> > files on the root disk are missed.
> >
> > Put this logic behind a flag and update the documentation.
> >
> > For now, the flag is enabled, to preserve existing behaviour. Future
> > work may provide a command (or some other mechanism) to control this.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > (no changes since v4)
> >
> > Changes in v4:
> > - Rewrite the commit message
> > - Enable the flag by default
> >
> > Changes in v3:
> > - Add new patch to consider non-bootable partitions
> >
> > boot/bootdev-uclass.c| 4 +++-
> > cmd/bootflow.c | 2 +-
> > doc/develop/bootstd/overview.rst | 5 +++--
> > include/bootflow.h | 2 ++
> > test/boot/bootdev.c | 1 +
> > test/boot/bootflow.c | 5 +++--
> > 6 files changed, 13 insertions(+), 6 deletions(-)
>
> Having spent another 10 minutes just now trying to understand things,
> again:
> - The commit message, and the implementation are either in disagreement
> or just too vague. Saying "to preserve existing behaviour" is unclear
> about which behavior is being preserved, since we're setting the flag
> to continue to only check bootable flagged partitions
> (BOOTFLOWIF_ONLY_BOOTABLE). But then we would continue to miss
> extlinux.conf files on root filesystems, which would seem to be the
> bug that needed fixing?
How about 'to preserve the existing behaviour of bootstd which is to
ignore non-bootable partitions so long as there is at least one
bootable partition on the disk" ?
> - It's still unclear if this makes bootstd match the exiting distro
> script behavior or not, and from your other emails it sounded like you
> were expecting someone else to dig around.
The distro scripts include the code below. My reading of it is that it
only considers bootable partitions, except that if there is none, then
it always scans partition 1.
"scan_dev_for_boot_part=" \
"part list ${devtype} ${devnum} -bootable devplist; " \
"env exists devplist || setenv devplist 1; " \
"for distro_bootpart in ${devplist}; do " \
"if fstype ${devtype} " \
"${devnum}:${distro_bootpart} " \
"bootfstype; then " \
"part uuid ${devtype} " \
"${devnum}:${distro_bootpart} " \
"distro_bootpart_uuid ; " \
"run scan_dev_for_boot; " \
"fi; "\
"done; " \
"setenv devplist\0" \
\
Regards,
Simon
Re: RISC-V UEFI/ACPI on QEMU regression?
On Wed, 2 Apr 2025 at 22:19, Simon Glass wrote:
>
> Hi,
>
> On Thu, 3 Apr 2025 at 03:36, Ilias Apalodimas
> wrote:
> >
> > On Wed, 2 Apr 2025 at 17:26, Ilias Apalodimas
> > wrote:
> > >
> > > Hey Bjorn
> > >
> > > Long time, hope all is well!
> > >
> > > On Wed, 2 Apr 2025 at 16:22, Björn Töpel wrote:
> > > >
> > > > Hi,
> > > >
> > > > I think I got a regression from commit 53d5a221632e ("emulation: Use
> > > > bloblist to hold tables"), and v2024.10 for
> > > > qemu-riscv64_smode_defconfig + acpi.config booting Linux with UEFI.
> > > >
> > > > TL;DR: It seems like the RSDP is placed in the wrong EFI memory map
> > > > type (it should be "ACPI Reclaim").
> > > >
> > > > I might also be misunderstanding what config fragments should be
> > > > used -- I'm out in the weeds here! ;-)
> > > >
> > > > When I was using v2024.10, ACPI RSDP was in ACPI Reclaim memory. All
> > > > good, and e.g. a kexec would properly work. However, when using u-boot
> > > > with commit 53d5a221632e ("emulation: Use bloblist to hold tables") I
> > > > get the following when booting, and then kexec:
> > > >
> > > > First kernel:
> > > > [0.00] ACPI: Early table checksum verification disabled
> > > > [0.00] ACPI:
> > > > RSDP 0x00047EED3000 24 (v02 BOCHS )
> > > > Kexec kernel:
> > > > [0.00] ACPI: Early table checksum verification disabled
> > > > [0.00] ACPI: 0x00047EED3000 00 (v00
> > > > )
> > > > [0.00] Oops - load access fault [#1]
> > > >
> > > > RSDP reside in:
> > > > [0.00] efi: 0x00047ded1000-0x00047eee3fff [Boot Code | |
> > > > | | | | | | | | | |WB| | | ]
> > > >
> > > > (Boot Code vs ACPI Reclaimed)
> > > >
> > > > Now to get qemu-riscv64_smode_defconfig + acpi.config to build
> > > > post-2024.10, I needed to add the following fragments:
> > > >
> > > > CONFIG_BLOBLIST=y
> > > > CONFIG_BLOBLIST_ALLOC=y
> > > > CONFIG_BLOBLIST_SIZE_RELOC=0x2
> > > >
> > > > which is really just a "make the build not complain", guessing game
> > > > from my side.
> > > >
> > > > My guess would be that it's related to the change in
> > > > evt_write_acpi_tables(), where:
> > > >
> > > > -ptr = memalign(SZ_4K, SZ_64K);
> > > > +ptr = bloblist_add(BLOBLISTT_ACPI_TABLES, SZ_64K, 12);
> > > >
> > > > is done.
> > > >
> > > > Is my config fragment broken, or is this a proper regression?
> > >
> > > I think it's a regression and I think what breaks it is commit
> > > cfb4aa2a754ed1
>
> Yes, that's right. It was reported by Patrick Rudolph in mid-Feb and I
> sent a patch about a month ago [1]
>
> It seems to be in Heinrich's queue in patchwork.
>
> > >
> > > Can you apply the diff below and see if it fixes it for you?
> > >
> > > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> > > index 4422b31ac6a7..afa5eee85484 100644
> > > --- a/lib/efi_loader/efi_acpi.c
> > > +++ b/lib/efi_loader/efi_acpi.c
> > > @@ -34,8 +34,8 @@ efi_status_t efi_acpi_register(void)
> > > * add_u_boot_and_runtime(). At some point that function could
> > > create a
> > > * more detailed map.
> > > */
> > > - if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> > > - return EFI_SUCCESS;
> > > + //if (IS_ENABLED(CONFIG_BLOBLIST_TABLES))
> > > + return EFI_SUCCESS;
> > >
> > > /* Mark space used for tables */
> > > start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
> > >
> > > Simon, why that PR got sent with no ACKs from any of the EFI maintainers?
>
> I didn't send a PR, but it's not clear that anyone would have spotted
> this bug as it is pretty subtle. It would be good to have testing for
> this case. We can actually do that by expanding the bootflow_efi()
> test.
>
> >
> > And please fix whatever tooling you use to CC the proper maintainers
> > next time [0] as I don't see my name in the CC list
>
> You had asked not to be cc'd on my patches anymore,
Are you sure?
> so I just cc
> Heinrich now, unless I manually override it.
I asked not to be cc'ed on patches that don't apply to -master or
-next and are for your tree. I obviously want to be CC'ed for anything
related to subsystems I help maintain.
Thanks
/Ilias
> Let me know what you'd
> like me to do.
>
> Regards,
> Simon
>
> >
> > [0]
> > https://lore.kernel.org/u-boot/2025011129.245022-27-...@chromium.org/
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20250306143134.2549815-1-...@chromium.org/
Re: [PATCH 00/13] video: Enhancements related to truetype and console
Hi Tom, On Thu, 3 Apr 2025 at 10:11, Tom Rini wrote: > > On Thu, Apr 03, 2025 at 08:19:03AM +1300, Simon Glass wrote: > > Hi Tom, > > > > On Thu, 3 Apr 2025 at 04:00, Tom Rini wrote: > > > > > > On Wed, Apr 02, 2025 at 01:08:56PM +1300, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Wed, 2 Apr 2025 at 10:41, Tom Rini wrote: > > > > > > > > > > On Wed, Apr 02, 2025 at 06:29:31AM +1300, Simon Glass wrote: > > > > > > > > > > > This series includes some precursor patches needed for forthcoming > > > > > > expo > > > > > > enhancements. > > > > > > > > > > > > - truetype support for multiple lines > > > > > > - make white-on-black a runtime option > > > > > > - support drawing a rectangle > > > > > > > > > > > > This series was split out from schd2 > > > > > [snip] > > > > > > base-commit: 3f76d803db9b500f43bc534465945a8d2836bb3e > > > > > > branch: schg > > > > > > > > > > I get as far as patch 4 before it doesn't apply to -next. > > > > > > > > Your tree is missing the video-damage series which adds some more > > > > tests. It is here if you want to pick it up: > > > > > > > > https://patchwork.ozlabs.org/project/uboot/list/?series=369663&state=* > > > > > > > > I can rebase and send a PR if you prefer. > > > > > > Reading the cover letter again, that doesn't seem ready to merge either. > > > > It seems OK to me. What are your concerns? > > The cover letter read to me like it should be further reworked to > address some of the problems it still has / exposed. That's likely why I > marked it as Changes Requested a year and a half ago. Which bit of it? Are you talking about my conversation with Alex, or the actual cover letter? Anyway, if you are willing to pick it up I could send a PR. Regards, Simon
Re: [PATCH 00/13] video: Enhancements related to truetype and console
On Thu, Apr 03, 2025 at 12:54:55PM +1300, Simon Glass wrote: > Hi Tom, > > On Thu, 3 Apr 2025 at 10:11, Tom Rini wrote: > > > > On Thu, Apr 03, 2025 at 08:19:03AM +1300, Simon Glass wrote: > > > Hi Tom, > > > > > > On Thu, 3 Apr 2025 at 04:00, Tom Rini wrote: > > > > > > > > On Wed, Apr 02, 2025 at 01:08:56PM +1300, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Wed, 2 Apr 2025 at 10:41, Tom Rini wrote: > > > > > > > > > > > > On Wed, Apr 02, 2025 at 06:29:31AM +1300, Simon Glass wrote: > > > > > > > > > > > > > This series includes some precursor patches needed for > > > > > > > forthcoming expo > > > > > > > enhancements. > > > > > > > > > > > > > > - truetype support for multiple lines > > > > > > > - make white-on-black a runtime option > > > > > > > - support drawing a rectangle > > > > > > > > > > > > > > This series was split out from schd2 > > > > > > [snip] > > > > > > > base-commit: 3f76d803db9b500f43bc534465945a8d2836bb3e > > > > > > > branch: schg > > > > > > > > > > > > I get as far as patch 4 before it doesn't apply to -next. > > > > > > > > > > Your tree is missing the video-damage series which adds some more > > > > > tests. It is here if you want to pick it up: > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/list/?series=369663&state=* > > > > > > > > > > I can rebase and send a PR if you prefer. > > > > > > > > Reading the cover letter again, that doesn't seem ready to merge either. > > > > > > It seems OK to me. What are your concerns? > > > > The cover letter read to me like it should be further reworked to > > address some of the problems it still has / exposed. That's likely why I > > marked it as Changes Requested a year and a half ago. > > Which bit of it? Are you talking about my conversation with Alex, or > the actual cover letter? Yes, I'm talking about the lengthy discussion in the cover letter which sounds to me as a summary of "changes requested". -- Tom signature.asc Description: PGP signature
Re: Rate of innovation in the project (Was: Re: Rate of change in the project)
Hi Tom, On Thu, 3 Apr 2025 at 11:35, Tom Rini wrote: > > On Thu, Apr 03, 2025 at 08:22:13AM +1300, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 2 Apr 2025 at 06:18, Tom Rini wrote: > > > > > > On Wed, Apr 02, 2025 at 04:45:37AM +1300, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Tue, 1 Apr 2025 at 04:51, Tom Rini wrote: > > > > > > > > > > On Fri, Mar 28, 2025 at 11:42:20PM +, Simon Glass wrote: > > > > > > Hi Tom, > > > > > > > > > > > > On Mon, 10 Mar 2025 at 09:53, Tom Rini wrote: > > > > > > > > > > > > > > On Fri, Mar 07, 2025 at 08:46:31AM -0600, Tom Rini wrote: > > > > > > > > On Thu, Mar 06, 2025 at 09:10:47AM -0700, Simon Glass wrote: > > > > > > > [snip] > > > > > > > > > Again, back to this thread, if you want me to migrate things, > > > > > > > > > consider > > > > > > > > > applying the sunxi patches as I have described above. I will > > > > > > > > > then look > > > > > > > > > at the next target for bootstd. But while you hold this up, I > > > > > > > > > cannot > > > > > > > > > move forward with more bootstd migration. It doesn't seem > > > > > > > > > that much to > > > > > > > > > ask. > > > > > > > > > > > > > > > > I will take another look at what's still relevant. But I > > > > > > > > believe it's > > > > > > > > still blocked on the fact that it changes behavior and breaks > > > > > > > > users. > > > > > > > > > > > > > > In details: > > > > > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-2-...@chromium.org/ > > > > > > > > > > > > > > Now that the underlying BLK problem is resolved, this can just be > > > > > > > dropped I believe. Removing the BLK dependency from BOOTSTD can > > > > > > > happen > > > > > > > when you're supporting a flow that lacks a BLK device entirely. > > > > > > > This > > > > > > > would be another reminder as to why putting unrelated changes in a > > > > > > > series is a problem. > > > > > > > > > > > > OK, then just don't apply this patch. Problem solved? > > > > > > > > > > Well, for a hypothetical v6 you would not include it, sure. > > > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-3-...@chromium.org/ > > > > > > > > > > > > > > This is fine. > > > > > > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20241113150938.1534931-4-...@chromium.org/ > > > > > > > > > > > > > > This is not fine. This is also not a sunxi problem. It means that > > > > > > > we > > > > > > > should drop bootmgr from rockchip, where the conversion has > > > > > > > already > > > > > > > taken place, and would need to drop it from future conversion too. > > > > > > > Neither of which are desirable changes. > > > > > > > > > > > > Why do you say that? I don't understand how this relates to rockchip > > > > > > or why we would need to drop bootmgr from that. > > > > > > > > > > Then you don't have enough of a grasp of the details of the area > > > > > you're > > > > > trying to solve problems in? Or maybe you need to refresh yourself on > > > > > the details of the area you're trying to work in. > > > > > > > > Or perhaps there isn't a problem? The sunxi case is special because we > > > > have a FEL boothmeth. That isn't present on rockchip, for example. > > > > > > Again, you seem to have forgotten what the problems with the series > > > were. > > > > No, it's just that we disagree on the path forward. > > Then why did you bring up FEL? That's the part of the migration which is > NOT a problem, I keep being reminded when I ask. FEL needs to get priority, that's all. It was a problem until I adjusted the priority. > > > > > > > > This patch in particular is > > > > > > > where we have the note: > > > > > > > > > > > > > > "Yes, the introduction of boot standard changed the boot order and > > > > > > > specifically deprioritizing scripts is unexpected." > > > > > > > > > > > > > > Which should have had more attention than it did. > > > > > > > > > > > > From memory the scripts are a fallback used when the other things > > > > > > don't work, so that was the decision I made at the time. > > > > > > > > > > The key point being we changed behavior that others depended on, and > > > > > didn't document it well and didn't make sure the change would work for > > > > > them either. > > > > > > > > > > > > This is the thread that to me spelled out in details where the > > > > > > > conversions are now blocked. We changed behavior and that in turn > > > > > > > breaks > > > > > > > users that have come to rely on ordering. > > > > > > > > > > > > I don't know what action to take on that comment. We cannot have > > > > > > 100% > > > > > > backwards compatibility with the scripts, which after all weren't > > > > > > even > > > > > > documented. But it is very close. > > > > > > > > > > You need to get feedback from the people you want to migrate from the > > > > > scripts and ordering and rely on to something else and see what works > > > > > for them.
Re: [RFC PATCH 1/2 v2] tools: binman: control.py: Propagate bootph-* properties to supernodes
Hey Simon,
On 03/04/25 00:52, Simon Glass wrote:
Hi Moteen,
On Wed, 2 Apr 2025 at 22:01, Moteen Shah wrote:
Hey Simon,
On 29/03/25 05:17, Simon Glass wrote:
Hi Moteen,
On Thu, 27 Mar 2025 at 08:06, Moteen Shah wrote:
Add a function to scan through all the nodes in the device-tree
recusively for bootph-* property. If found, propagate it to all
of its parent nodes up the hierarchy.
Signed-off-by: Moteen Shah
---
tools/binman/control.py | 35 ++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py
index e73c598298c..e739949d366 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -526,6 +526,35 @@ def _RemoveTemplates(parent):
if node.name.startswith('template'):
node.Delete()
+def prop_bootph_to_parent(node, prop, dtb):
I think the 'prop_' is a bit confusing, since you are dealing with
properties! How about 'add_' as the prefix?
'add_' should be more descriptive, will add that in v3.
+"""Propagates bootph-* property to all the parent
+nodes up the hierarchy
First line should be a summary
then blank line
then describe the args...you can see this if you look at a few other functions.
Noted, will rectify in v3.
+"""
+parent = node.parent
+if parent == None or parent.props.get(prop):
if not parent or ...
Noted.
+ return
+
+while parent:
+parent.AddEmptyProp(prop, 0)
+parent = parent.parent
+
+def scan_and_prop_bootph(node, dtb):
+"""Scan the device tree and set the bootph-* property if its present
+in subnode
+
+This is used to set the bootph-* property in the parent node if a
+"bootph-*" property is found in any of the subnodes of the parent
+node.
comment style again
+"""
+bootph_prop = ['bootph-all', 'bootph-some-ram', 'bootph-pre-ram',
'bootph-pre-sram']
From my understanding the only ones that matter are bootph-all and
bootph-some-ram, since the SPL ones are handled by fdtgrep.
Noted.
+
+for props in bootph_prop:
for prop
(since it is just one)
+if node.props.get(props):
+prop_bootph_to_parent(node, props, dtb)
+
+for subnode in node.subnodes:
+ scan_and_prop_bootph(subnode, dtb)
+
def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded,
indir):
"""Prepare the images to be processed and select the device tree
@@ -563,7 +592,11 @@ def PrepareImagesAndDtbs(dtb_fname, select_images,
update_fdt, use_expanded, ind
indir = []
dtb_fname = fdt_util.EnsureCompiled(dtb_fname, indir=indir)
fname = tools.get_output_filename('u-boot.dtb.out')
-tools.write_file(fname, tools.read_file(dtb_fname))
+dtb = fdt.FdtScan(dtb_fname)
+scan_and_prop_bootph(dtb.GetRoot(), dtb)
+dtb.Sync(True)
+tools.write_file(dtb_fname, dtb.GetContents())
+tools.write_file(fname, dtb.GetContents())
You shouldn't write back to the file created by the build. Do you need this?
Had the same thought, but the build fails in non clean builds[0]. Did a
workaround[1] but then some essential template nodes ends up getting
deleted from u-boot.dtb. Finally, had to write back to the file
to resolve the issue.
Can you push a tree somewhere. This could be a bug that I could fix.
You can use the tree here to recreate the non clean build issue mentioned:
https://github.com/Jamm02/U-Boot-patchwork/tree/bootph-patch-test
dtb = fdt.FdtScan(fname)
node = _FindBinmanNode(dtb)
--
2.34.1
The code looks fine to me apart from the nits.
This addition needs a test - see ftest.py for some examples there. But
basically just create a dts that has some props in it, then check that
they got added.
Included a test as well in the patchset[3]. Do let me know if there are
some changes required in it.
Somehow I'm not seeing that in patchwork. It looks good but please try
to keep lines <=80 cols.
Sure, will fix up in v3. Should I push a v3 or wait for you to fix the
issue discussed above?
I think testSimpleFitEncryptedData() could be a good example?
Regards,
Simon
References:
[0]https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-non-clean-build
[1]https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-patch-tools-dtdoc-fdt-py
[3]https://lore.kernel.org/u-boot/20250327080642.2269856-3-m-s...@ti.com/
Thanks for the review.
Regards,
Simon
Regards,
Moteen
Re: [PATCH 00/13] video: Enhancements related to truetype and console
On Wed, Apr 02, 2025 at 01:08:56PM +1300, Simon Glass wrote: > Hi Tom, > > On Wed, 2 Apr 2025 at 10:41, Tom Rini wrote: > > > > On Wed, Apr 02, 2025 at 06:29:31AM +1300, Simon Glass wrote: > > > > > This series includes some precursor patches needed for forthcoming expo > > > enhancements. > > > > > > - truetype support for multiple lines > > > - make white-on-black a runtime option > > > - support drawing a rectangle > > > > > > This series was split out from schd2 > > [snip] > > > base-commit: 3f76d803db9b500f43bc534465945a8d2836bb3e > > > branch: schg > > > > I get as far as patch 4 before it doesn't apply to -next. > > Your tree is missing the video-damage series which adds some more > tests. It is here if you want to pick it up: > > https://patchwork.ozlabs.org/project/uboot/list/?series=369663&state=* > > I can rebase and send a PR if you prefer. Reading the cover letter again, that doesn't seem ready to merge either. -- Tom signature.asc Description: PGP signature
[PATCH] hmibsc_defconfig: disable DM_USB_GADGET
As with the db410c this breaks linking as it conflicts with the USB controller used by these platforms. This fixes building after DM_USB_GADGET was enabled by default for mach-snapdragon. Fixes: 7235dbedfce3 (mach-snapdragon: enable DM_USB_GADGET by default) Signed-off-by: Caleb Connolly --- configs/hmibsc_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/configs/hmibsc_defconfig b/configs/hmibsc_defconfig index 88c9c70a6399..4550eba71ab2 100644 --- a/configs/hmibsc_defconfig +++ b/configs/hmibsc_defconfig @@ -69,8 +69,9 @@ CONFIG_DM_PMIC=y CONFIG_PMIC_QCOM=y CONFIG_MSM_SERIAL=y CONFIG_SPMI_MSM=y CONFIG_USB=y +# CONFIG_DM_USB_GADGET is not set CONFIG_USB_EHCI_HCD=y CONFIG_USB_EHCI_MSM=y CONFIG_USB_ULPI_VIEWPORT=y CONFIG_USB_ULPI=y -- 2.48.1
Re: [PATCH] env: Introduce support for SPI NAND flash
On Wed, Apr 02, 2025 at 12:57:57AM +0200, Christian Marangi wrote: > Introduce support for SPI NAND flash. Currently we only support SPI > flash based on the lagacy sf cmd that assume SPI flash are always NOR. > This is not the case as to SPI controller also NAND can be attached. Add > support for it by adding an env driver that base entirely on the MTD > api. > > Introduce a new kconfig ENV_IS_IN_SPI_NAND_FLASH and > CONFIG_SYS_SNAND_ENV_DEV to define the name of the SPI nand as exposed > by mtd list. > > Signed-off-by: Christian Marangi > --- > env/Kconfig| 41 - > env/Makefile | 1 + > env/env.c | 3 + > env/snand.c| 338 + > include/env_internal.h | 1 + > 5 files changed, 380 insertions(+), 4 deletions(-) > create mode 100644 env/snand.c Since this uses the generic mtd API, it can also support SPI NOR, and "regular" NAND too yes? If so, I'd like to see this named more generically (and my feedback about naming things from the UFS one applies here too), and then perhaps a follow-up converting some other platforms to use this? Thanks. -- Tom signature.asc Description: PGP signature
Re: [PATCH] env: Introduce support for SPI NAND flash
On Wed, Apr 02, 2025 at 09:05:42AM -0600, Tom Rini wrote: > On Wed, Apr 02, 2025 at 12:57:57AM +0200, Christian Marangi wrote: > > > Introduce support for SPI NAND flash. Currently we only support SPI > > flash based on the lagacy sf cmd that assume SPI flash are always NOR. > > This is not the case as to SPI controller also NAND can be attached. Add > > support for it by adding an env driver that base entirely on the MTD > > api. > > > > Introduce a new kconfig ENV_IS_IN_SPI_NAND_FLASH and > > CONFIG_SYS_SNAND_ENV_DEV to define the name of the SPI nand as exposed > > by mtd list. > > > > Signed-off-by: Christian Marangi > > --- > > env/Kconfig| 41 - > > env/Makefile | 1 + > > env/env.c | 3 + > > env/snand.c| 338 + > > include/env_internal.h | 1 + > > 5 files changed, 380 insertions(+), 4 deletions(-) > > create mode 100644 env/snand.c > > Since this uses the generic mtd API, it can also support SPI NOR, and > "regular" NAND too yes? If so, I'd like to see this named more > generically (and my feedback about naming things from the UFS one > applies here too), and then perhaps a follow-up converting some other > platforms to use this? Thanks. > I assume yes. So maybe we can change this to env/mtd.c? Any hint for a better name? Can you link the feedback from UFS, I can't find it. -- Ansuel
Re: [PATCH] env: Introduce support for SPI NAND flash
On Wed, Apr 02, 2025 at 05:10:31PM +0200, Christian Marangi wrote: > On Wed, Apr 02, 2025 at 09:05:42AM -0600, Tom Rini wrote: > > On Wed, Apr 02, 2025 at 12:57:57AM +0200, Christian Marangi wrote: > > > > > Introduce support for SPI NAND flash. Currently we only support SPI > > > flash based on the lagacy sf cmd that assume SPI flash are always NOR. > > > This is not the case as to SPI controller also NAND can be attached. Add > > > support for it by adding an env driver that base entirely on the MTD > > > api. > > > > > > Introduce a new kconfig ENV_IS_IN_SPI_NAND_FLASH and > > > CONFIG_SYS_SNAND_ENV_DEV to define the name of the SPI nand as exposed > > > by mtd list. > > > > > > Signed-off-by: Christian Marangi > > > --- > > > env/Kconfig| 41 - > > > env/Makefile | 1 + > > > env/env.c | 3 + > > > env/snand.c| 338 + > > > include/env_internal.h | 1 + > > > 5 files changed, 380 insertions(+), 4 deletions(-) > > > create mode 100644 env/snand.c > > > > Since this uses the generic mtd API, it can also support SPI NOR, and > > "regular" NAND too yes? If so, I'd like to see this named more > > generically (and my feedback about naming things from the UFS one > > applies here too), and then perhaps a follow-up converting some other > > platforms to use this? Thanks. > > > > I assume yes. So maybe we can change this to env/mtd.c? Any hint for a > better name? Sounds good to me. > Can you link the feedback from UFS, I can't find it. Here: https://lore.kernel.org/u-boot/7a5ac5c2-5bb7-45d2-838b-63fa07766...@linaro.org/T/#m90111fe8a18efad7fc4f4e03745fe5e7b00db028 -- Tom signature.asc Description: PGP signature
[PATCH] bootstd: android: avoid possible null pointer dereference
- avb_slot_verify_data_free() doesn't check its data parameter
- out_data can be null if avb_slot_verify() fails to allocate memory
Signed-off-by: Gary Bisson
---
Another approach would be to fix avb_slot_verify_data_free() to check
its paramter but I believe the goal is not to touch libavb to be closer
to upstream.
---
boot/bootmeth_android.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
index 3a59a4e3f6..f431b6ae58 100644
--- a/boot/bootmeth_android.c
+++ b/boot/bootmeth_android.c
@@ -481,7 +481,8 @@ static int run_avb_verification(struct bootflow *bflow)
if (result != AVB_SLOT_VERIFY_RESULT_OK) {
printf("Verification failed, reason: %s\n",
str_avb_slot_error(result));
- avb_slot_verify_data_free(out_data);
+ if (out_data)
+ avb_slot_verify_data_free(out_data);
return log_msg_ret("avb verify", -EIO);
}
boot_state = AVB_GREEN;
@@ -491,7 +492,8 @@ static int run_avb_verification(struct bootflow *bflow)
result != AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION) {
printf("Unlocked verification failed, reason: %s\n",
str_avb_slot_error(result));
- avb_slot_verify_data_free(out_data);
+ if (out_data)
+ avb_slot_verify_data_free(out_data);
return log_msg_ret("avb verify unlocked", -EIO);
}
boot_state = AVB_ORANGE;
--
2.47.2
Re: [PATCH] configs: meson64: move DFU step at the end to give the board a chance to boot something on storage
On Wed Apr 2, 2025 at 2:02 PM UTC, Neil Armstrong via groups.io wrote: > The DFU was set to run at the beginning, but we may want to > boot something over USB, MMC or Ethernet even if booted over > USB, so move the DFU as the final fallback. > > This keeps the current Amlogic U-Boot CI working and makes > the default config more generic. > > Signed-off-by: Neil Armstrong Acked-by: Ferass El Hafidi Thanks! > --- > include/configs/meson64.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/configs/meson64.h b/include/configs/meson64.h > index > f3275b37a516da6befd41324ddb2a3fe1f36fc83..675df623311f361f3a55971c3894ffbe1d07fee6 > 100644 > --- a/include/configs/meson64.h > +++ b/include/configs/meson64.h > @@ -118,13 +118,13 @@ > #ifndef BOOT_TARGET_DEVICES > #define BOOT_TARGET_DEVICES(func) \ > func(ROMUSB, romusb, na) \ > - func(USB_DFU, usbdfu, na) \ > BOOT_TARGET_MMC(func) \ > BOOT_TARGET_DEVICES_USB(func) \ > BOOT_TARGET_NVME(func) \ > BOOT_TARGET_SCSI(func) \ > BOOT_TARGET_PXE(func) \ > - BOOT_TARGET_DHCP(func) > + BOOT_TARGET_DHCP(func) \ > + func(USB_DFU, usbdfu, na) > #endif > > #define BOOTM_SIZE __stringify(0x170) > > --- > base-commit: c17f03a7e93dfbbe5d32f5738274187065d9493f > change-id: 20250402-u-boot-meson64-move-dfu-end-eb9f5fe55608 > > Best regards,
RISC-V UEFI/ACPI on QEMU regression?
Hi,
I think I got a regression from commit 53d5a221632e ("emulation: Use
bloblist to hold tables"), and v2024.10 for
qemu-riscv64_smode_defconfig + acpi.config booting Linux with UEFI.
TL;DR: It seems like the RSDP is placed in the wrong EFI memory map
type (it should be "ACPI Reclaim").
I might also be misunderstanding what config fragments should be
used -- I'm out in the weeds here! ;-)
When I was using v2024.10, ACPI RSDP was in ACPI Reclaim memory. All
good, and e.g. a kexec would properly work. However, when using u-boot
with commit 53d5a221632e ("emulation: Use bloblist to hold tables") I
get the following when booting, and then kexec:
First kernel:
[0.00] ACPI: Early table checksum verification disabled
[0.00] ACPI:
RSDP 0x00047EED3000 24 (v02 BOCHS )
Kexec kernel:
[0.00] ACPI: Early table checksum verification disabled
[0.00] ACPI: 0x00047EED3000 00 (v00
)
[0.00] Oops - load access fault [#1]
RSDP reside in:
[0.00] efi: 0x00047ded1000-0x00047eee3fff [Boot Code | |
| | | | | | | | | |WB| | | ]
(Boot Code vs ACPI Reclaimed)
Now to get qemu-riscv64_smode_defconfig + acpi.config to build
post-2024.10, I needed to add the following fragments:
CONFIG_BLOBLIST=y
CONFIG_BLOBLIST_ALLOC=y
CONFIG_BLOBLIST_SIZE_RELOC=0x2
which is really just a "make the build not complain", guessing game
from my side.
My guess would be that it's related to the change in
evt_write_acpi_tables(), where:
-ptr = memalign(SZ_4K, SZ_64K);
+ptr = bloblist_add(BLOBLISTT_ACPI_TABLES, SZ_64K, 12);
is done.
Is my config fragment broken, or is this a proper regression?
Thanks!
Björn
Re: [RFC] Upstreaming Orange Pi RV2 vendor patches - Seeking guidance
Hi Sungjoon Moon, On 4/1/25 3:41 PM, Sungjoon Moon wrote: [You don't often get email from sum...@seoulsaram.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] I'm working on upstreaming vendor patches for the Orange Pi RV2 board, currently available at: https://eur02.safelinks.protection.outlook.com/? url=https%3A%2F%2Fgithub.com%2FOctopusET%2Fu-boot- orangepi%2Fpull%2F1%2Fcommits&data=05%7C02%7Cquentin.schulz%40cherry.de%7C32ac8a27848840e80f4f08dd71657036%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638791402679712024%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=VhrD%2BgKD0cCW5pZIKhy7BoiGtj5fyTKukkdVj116jIY%3D&reserved=0 I'm rebasing these patches onto the latest U-Boot branch (v2025.04-rc5). To ensure the patches meet upstream standards, I have a few questions: 1. What specific steps should I take after rebasing to prepare these patches for upstream submission? Not sure what exactly you're after here. You need at the very least to run scripts/checkpatch.pl on all your patches and have them pass successfully. Some warnings may be ignored in some corner cases so ask if some warnings are odd or difficult to fix. This can be run automatically if you're using b4[1] with `b4 prep --check`. Your patches shouldn't break other boards, be it build time or runtime. For build time, you could build everything and check that it still compiles. I vaguely remember buildman tool is supposed to be able to do that but I've used it maybe once. 2. The current patch set is quite large. Should I split it into smaller, more focused patches? If so, what would be the recommended approach for splitting? https://docs.u-boot.org/en/latest/develop/sending_patches.html the whole page is worth reading but to answer this question specifically: """ - Patches should always contain exactly one complete logical change, i.e. - Changes that contain different, unrelated modifications shall be submitted as separate patches, one patch per changeset. - If one logical set of modifications affects or creates several files, all these changes shall be submitted in a single patch. - Non-functional changes, i.e. whitespace and reformatting changes, should be done in separate patches marked as cosmetic. This separation of functional and cosmetic changes greatly facilitates the review process. """ I'm of the opinion that it's easier to start with too many commits and squash them on request than splitting commits. If there are patches that the project could benefit from that would impact more boards than the upcoming OrangePi RV2, you may consider sending them in a separate series in advance. In short, series are rarely partially merged, so if there are controversial patches in a series, it could be beneficial to split into multiple, easier to review and merge, series. 3. Are there any particular coding style or documentation requirements I should be aware of for the Orange Pi RV2 board support? https://docs.u-boot.org/en/latest/develop/designprinciples.html https://docs.u-boot.org/en/latest/develop/codingstyle.html https://docs.u-boot.org/en/latest/develop/board_best_practices.html Add tests when adding code. Add documentation for your board to https://docs.u-boot.org/en/latest/board/index.html 4. How should I handle any vendor-specific code that may not be suitable for upstream? We all have different opinion on what may or may not be suitable for upstream. Anything not sent upstream, we don't care, that's your problem to handle. If you send patches that are unacceptable, we'll tell you and we'll try to figure out ways to deal with them (easier by outright rejecting them, or by suggesting ways to modify the changes so they are acceptable). What specifically do you have in mind as being not suitable for upstream? 5. Are there any maintainers or subsystem-specific mailing lists I should include when submitting these patches? Use scripts/get_maintainer.pl on your patches, and Cc/To most/all people and mailing list listed there. This is done automatically by b4[1] with `b4 prep --auto-to-cc`. [1] https://b4.docs.kernel.org/en/latest/contributor/overview.html Any guidance or best practices for upstreaming vendor patches would be greatly appreciated. Thank you for your time and assistance. Good luck, have fun and looking forward to seeing patches on the mailing list. Cheers, Quentin
Re: [RFC PATCH 1/2 v2] tools: binman: control.py: Propagate bootph-* properties to supernodes
Hey Simon,
On 29/03/25 05:17, Simon Glass wrote:
Hi Moteen,
On Thu, 27 Mar 2025 at 08:06, Moteen Shah wrote:
Add a function to scan through all the nodes in the device-tree
recusively for bootph-* property. If found, propagate it to all
of its parent nodes up the hierarchy.
Signed-off-by: Moteen Shah
---
tools/binman/control.py | 35 ++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/tools/binman/control.py b/tools/binman/control.py
index e73c598298c..e739949d366 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -526,6 +526,35 @@ def _RemoveTemplates(parent):
if node.name.startswith('template'):
node.Delete()
+def prop_bootph_to_parent(node, prop, dtb):
I think the 'prop_' is a bit confusing, since you are dealing with
properties! How about 'add_' as the prefix?
'add_' should be more descriptive, will add that in v3.
+"""Propagates bootph-* property to all the parent
+nodes up the hierarchy
First line should be a summary
then blank line
then describe the args...you can see this if you look at a few other functions.
Noted, will rectify in v3.
+"""
+parent = node.parent
+if parent == None or parent.props.get(prop):
if not parent or ...
Noted.
+ return
+
+while parent:
+parent.AddEmptyProp(prop, 0)
+parent = parent.parent
+
+def scan_and_prop_bootph(node, dtb):
+"""Scan the device tree and set the bootph-* property if its present
+in subnode
+
+This is used to set the bootph-* property in the parent node if a
+"bootph-*" property is found in any of the subnodes of the parent
+node.
comment style again
+"""
+bootph_prop = ['bootph-all', 'bootph-some-ram', 'bootph-pre-ram',
'bootph-pre-sram']
From my understanding the only ones that matter are bootph-all and
bootph-some-ram, since the SPL ones are handled by fdtgrep.
Noted.
+
+for props in bootph_prop:
for prop
(since it is just one)
+if node.props.get(props):
+prop_bootph_to_parent(node, props, dtb)
+
+for subnode in node.subnodes:
+ scan_and_prop_bootph(subnode, dtb)
+
def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded,
indir):
"""Prepare the images to be processed and select the device tree
@@ -563,7 +592,11 @@ def PrepareImagesAndDtbs(dtb_fname, select_images,
update_fdt, use_expanded, ind
indir = []
dtb_fname = fdt_util.EnsureCompiled(dtb_fname, indir=indir)
fname = tools.get_output_filename('u-boot.dtb.out')
-tools.write_file(fname, tools.read_file(dtb_fname))
+dtb = fdt.FdtScan(dtb_fname)
+scan_and_prop_bootph(dtb.GetRoot(), dtb)
+dtb.Sync(True)
+tools.write_file(dtb_fname, dtb.GetContents())
+tools.write_file(fname, dtb.GetContents())
You shouldn't write back to the file created by the build. Do you need this?
Had the same thought, but the build fails in non clean builds[0]. Did a
workaround[1] but then some essential template nodes ends up getting
deleted from u-boot.dtb. Finally, had to write back to the file
to resolve the issue.
dtb = fdt.FdtScan(fname)
node = _FindBinmanNode(dtb)
--
2.34.1
The code looks fine to me apart from the nits.
This addition needs a test - see ftest.py for some examples there. But
basically just create a dts that has some props in it, then check that
they got added.
Included a test as well in the patchset[3]. Do let me know if there are
some changes required in it.
I think testSimpleFitEncryptedData() could be a good example?
Regards,
Simon
References:
[0]
https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-non-clean-build
[1]
https://gist.github.com/Jamm02/2478a4f1186f3ba4a8cd7dbf1fb11e2f#file-patch-tools-dtdoc-fdt-py
[3] https://lore.kernel.org/u-boot/20250327080642.2269856-3-m-s...@ti.com/
Thanks for the review.
Regards,
Moteen
Re: RISC-V UEFI/ACPI on QEMU regression?
Hey! On Wed, 2 Apr 2025 at 16:27, Ilias Apalodimas wrote: > > Hey Bjorn > > Long time, hope all is well! Too long! Maybe next Plumbers? ;-) > I think it's a regression and I think what breaks it is commit cfb4aa2a754ed1 > > Can you apply the diff below and see if it fixes it for you? > > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c > index 4422b31ac6a7..afa5eee85484 100644 > --- a/lib/efi_loader/efi_acpi.c > +++ b/lib/efi_loader/efi_acpi.c > @@ -34,8 +34,8 @@ efi_status_t efi_acpi_register(void) > * add_u_boot_and_runtime(). At some point that function could create > a > * more detailed map. > */ > - if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) > - return EFI_SUCCESS; > + //if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) > + return EFI_SUCCESS; > > /* Mark space used for tables */ > start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK); Unfortunately, still the same with this patch: [0.00] efi: 0x00047eee3000-0x00047ef19fff [Boot Code | | | | | | | | | | | |WB| | | ] [0.00] efi: 0x00047ef1a000-0x00047ef49fff [ACPI Reclaim| | | | | | | | | | | |WB| | | ] [0.00] ACPI: RSDP 0x00047EED1000 24 (v02 BOCHS ) (RSDP resides in Boot Code) Björn
Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console
On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 3 Apr 2025 at 03:29, Tom Rini wrote:
> >
> > On Sat, Mar 15, 2025 at 02:26:00PM +, Simon Glass wrote:
> > > Update the string to take account of UARTs which are connected on I/O
> > > ports, as on x86.
> > >
> > > Fix a typo in an error message in the same command, while we are here.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > (no changes since v3)
> > >
> > > Changes in v3:
> > > - Add new patch to support IO UARTs for earlycon and console
> > >
> > > boot/bootflow.c | 7 ---
> > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > index 58a1afa7a75..4054a966af8 100644
> > > --- a/boot/bootflow.c
> > > +++ b/boot/bootflow.c
> > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow,
> > > const char *arg)
> > > *buf = '\0';
> > > if (!strcmp("earlycon", arg) && info.type ==
> > > SERIAL_CHIP_16550_COMPATIBLE) {
> > > snprintf(buf, sizeof(buf),
> > > - "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > - info.baudrate);
> > > + "uart8250,%s,%#lx,%dn8",
> > > + info.addr_space == SERIAL_ADDRESS_SPACE_IO ? "io" :
> > > + "mmio", info.addr, info.baudrate);
> > > } else if (!strcmp("earlycon", arg) && info.type ==
> > > SERIAL_CHIP_PL01X) {
> > > snprintf(buf, sizeof(buf),
> > >"pl011,mmio32,%#lx,%dn8", info.addr,
> >
> > I suppose we're well past the point where we can delete
> > bootflow_cmdline_auto() itself because that's just going to lead us to
> > trouble down the line (5 years from now when the kernel adopts a new
> > preferred way to pass this info) and grows every platform by some amount
> > of space every time we add something new here.
>
> Well firstly, why would you want to delete this command? It is very
> useful to be able to change the cmdline.
>
> This command is only available with BOOTSTD_FULL, which is less than
> 10% of boards.
Because it's automatic non-obvious stuff. We should not be modifying the
command line at all. Is it even documented that we're doing this?
--
Tom
signature.asc
Description: PGP signature
Merged v6.14-dts to next branch
As the subject says, I've merged v6.14-dts to dts/upstream in the next branch now. -- Tom signature.asc Description: PGP signature
Re: [PATCH v5 40/46] boot: Support IO UARTs for earlycon and console
On Wed, Apr 02, 2025 at 01:50:33PM -0600, Tom Rini wrote:
> On Thu, Apr 03, 2025 at 08:22:44AM +1300, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 3 Apr 2025 at 03:29, Tom Rini wrote:
> > >
> > > On Sat, Mar 15, 2025 at 02:26:00PM +, Simon Glass wrote:
> > > > Update the string to take account of UARTs which are connected on I/O
> > > > ports, as on x86.
> > > >
> > > > Fix a typo in an error message in the same command, while we are here.
> > > >
> > > > Signed-off-by: Simon Glass
> > > > ---
> > > >
> > > > (no changes since v3)
> > > >
> > > > Changes in v3:
> > > > - Add new patch to support IO UARTs for earlycon and console
> > > >
> > > > boot/bootflow.c | 7 ---
> > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/boot/bootflow.c b/boot/bootflow.c
> > > > index 58a1afa7a75..4054a966af8 100644
> > > > --- a/boot/bootflow.c
> > > > +++ b/boot/bootflow.c
> > > > @@ -942,8 +942,9 @@ int bootflow_cmdline_auto(struct bootflow *bflow,
> > > > const char *arg)
> > > > *buf = '\0';
> > > > if (!strcmp("earlycon", arg) && info.type ==
> > > > SERIAL_CHIP_16550_COMPATIBLE) {
> > > > snprintf(buf, sizeof(buf),
> > > > - "uart8250,mmio32,%#lx,%dn8", info.addr,
> > > > - info.baudrate);
> > > > + "uart8250,%s,%#lx,%dn8",
> > > > + info.addr_space == SERIAL_ADDRESS_SPACE_IO ?
> > > > "io" :
> > > > + "mmio", info.addr, info.baudrate);
> > > > } else if (!strcmp("earlycon", arg) && info.type ==
> > > > SERIAL_CHIP_PL01X) {
> > > > snprintf(buf, sizeof(buf),
> > > >"pl011,mmio32,%#lx,%dn8", info.addr,
> > >
> > > I suppose we're well past the point where we can delete
> > > bootflow_cmdline_auto() itself because that's just going to lead us to
> > > trouble down the line (5 years from now when the kernel adopts a new
> > > preferred way to pass this info) and grows every platform by some amount
> > > of space every time we add something new here.
> >
> > Well firstly, why would you want to delete this command? It is very
> > useful to be able to change the cmdline.
> >
> > This command is only available with BOOTSTD_FULL, which is less than
> > 10% of boards.
>
> Because it's automatic non-obvious stuff. We should not be modifying the
> command line at all. Is it even documented that we're doing this?
To be clearer, the more I review your changes, the more I see a blurred
line that I do not this is good between "I found this handy while
doing something" and "This is a good generic design / feature".
That it can be annoying at times to add the debug uart information is
not a new unique problem. It's something that's generally normally
solved. I assume you hit this on x86 where it's more annoying than most.
But a generic feature it should not have been.
--
Tom
signature.asc
Description: PGP signature
Re: U-Boot interferes with initrd binary — Revert "efi: Correct smbios-table installation" ?
On 31. Mar 2025, at 19:15, Michael Brown wrote: > > On 31/03/2025 17:49, Christian Kohlschütter wrote: >> # hexdump /.argh >> 000 >> * >> 0044100 15dc f9c8 6f16 e188 >> 0044110 00a0 52b0 a01c 6d59 >> 0044120 >> 0044130 e1a8 6097 > > This looks suspiciously like a broadcast Ethernet packet from a source MAC > address of dc:15:c8:f9:16:6f with an ethertype of 0x88e1. Some quick > research suggests that these are HomePlug Management packets generated by a > "Fritz!" home router. > >>> commit 06ef8089f876b6dabf56caba31a05c003f03c629 (HEAD) >>> Author: Simon Glass >>> Date: Sun Dec 31 08:25:55 2023 -0700 >>> >>> efi: Correct smbios-table installation >>> At present this code allocates memory when writing the tables and >>> then unnecessarily adds another memory map when installing it. >>> Adjust the code to allocate the tables using the normal U-Boot >>> mechanism. This avoids doing an EFI memory allocation early in >>> U-Boot, which may use memory that would be overwritten by a >>> 'load' command, for example. >>> Signed-off-by: Simon Glass > > No idea how that could relate to the packet you're seeing being written into > random memory locations, sorry. I'll leave it to one of the U-Boot > developers to respond. > > Thanks, > > Michael > Whoa! Good eyes, Michael! What is my Fritzbox doing to my initrd, and why does reverting the commit fix it? FWIW, I also have a capture with an ethernet frame from another device on my network (ARP, ethertype 0x0806), so this is probably the content of some ethernet receive buffer...
Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"
Am Dienstag, 1. April 2025, 18:13:35 MESZ schrieb Heinrich Schuchardt:
> On 01.04.25 17:51, Simon Glass wrote:
> > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner wrote:
> >>
> >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> >>
> >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> >> aims to reduce EFI boot times by disabling the dhcp_run when
> >> checking ethernet bootdevices, by preventing it from running double,
> >> with the reasoning
> >>
> >> We need to call eth_bootdev_hunt() when setting up the EFI sub-system
> >> to
> >> supply the simple network protocol. We don't need an IP address set
> >> up.
> >>
> >> That might by true for EFI, but not for everything else, because when
> >> running distro-boot and for example the PXE method in it, nothing will
> >> set up an IP address now.
>
> The removed call was dhcp_run(addr, NULL, true);
>
> We have:
>
> distro_efi_read_bootflow_net():
> boot/bootmeth_efi.c:205:ret = dhcp_run(addr, NULL, true);
>
> script_read_bootflow_net():
> boot/bootmeth_script.c:132: ret = dhcp_run(addr, fname, true);
>
> extlinux_pxe_read_bootflow() seems to be lacking the call.
>
> So instead of reverting
> 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> we should add the missing call in extlinux_pxe_read_bootflow().
doing
- 8< -
diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
index b91e61bcbc4..6e5e0f99ea4 100644
--- a/boot/bootmeth_pxe.c
+++ b/boot/bootmeth_pxe.c
@@ -73,6 +73,10 @@ static int extlinux_pxe_read_bootflow(struct udevice *dev,
return log_msg_ret("pxeb", -EPERM);
addr = simple_strtoul(addr_str, NULL, 16);
+ ret = dhcp_run(addr, NULL, false);
+ if (ret)
+ return log_msg_ret("dhc", ret);
+
log_debug("calling pxe_get()\n");
ret = pxe_get(addr, &bootdir, &size, false);
log_debug("pxe_get() returned %d\n", ret);
- 8< -
does seem to work in _my_ usecase and gets me loading stuff over pxe
again.
I guess this whole thing now becomes a strategy decision ;-) :
- it's very late in the release, do we revert to the known working state
or hope the above fixes all issues
- Simon's wish of not sprinkling dhcp_run calls in multiple places
I guess I'm fine with both way, as either fixes my use-case ;-)
Heiko
Re: [PATCH 02/14] net: airoha: Add Airoha Ethernet driver
On Wed, Apr 02, 2025 at 09:19:51AM -0600, Tom Rini wrote: > On Wed, Apr 02, 2025 at 12:51:34AM +0200, Christian Marangi wrote: > > > Add airoha Ethernet driver for Airoha AN7581 SoC. This is a majorly > > rewritten and simplified version of the Linux airoha_eth.c driver. > > > > It's has been modified to support a single RX/TX ring to reflect U-Boot > > implementation with recv and send API. > > > > The struct and the define are kept as similar as possible to upstream > > one to not diverge too much. > > > > The AN7581 SoC include an Ethernet Switch based on the Mediatek MT753x > > but doesn't require any modification aside from setting the CPU port and > > applying the Flood configuration hence it can be handled entirely in the > > Ethernet driver. > > > > Signed-off-by: Christian Marangi > > --- > > drivers/net/Kconfig |8 + > > drivers/net/Makefile |1 + > > drivers/net/airoha_eth.c | 1448 ++ > > 3 files changed, 1457 insertions(+) > > create mode 100644 drivers/net/airoha_eth.c > > checkpatch.pl has some macro warnings and it looks like some > inconsistent spacing around '#define FOO' vs '#defineFOO' ? And are > all of those defines needed? > No they are not all needed, I defined them for consistency with the original driver. Should I drop them? For the checkpatch check, also comes from the original driver. -- Ansuel
[PATCH] env: Introduce support for SPI NAND flash
Introduce support for SPI NAND flash. Currently we only support SPI
flash based on the lagacy sf cmd that assume SPI flash are always NOR.
This is not the case as to SPI controller also NAND can be attached. Add
support for it by adding an env driver that base entirely on the MTD
api.
Introduce a new kconfig ENV_IS_IN_SPI_NAND_FLASH and
CONFIG_SYS_SNAND_ENV_DEV to define the name of the SPI nand as exposed
by mtd list.
Signed-off-by: Christian Marangi
---
env/Kconfig| 41 -
env/Makefile | 1 +
env/env.c | 3 +
env/snand.c| 338 +
include/env_internal.h | 1 +
5 files changed, 380 insertions(+), 4 deletions(-)
create mode 100644 env/snand.c
diff --git a/env/Kconfig b/env/Kconfig
index 4438f0b392c..484786b5bd8 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -74,7 +74,7 @@ config ENV_IS_DEFAULT
!ENV_IS_IN_MMC && !ENV_IS_IN_NAND && \
!ENV_IS_IN_NVRAM && !ENV_IS_IN_ONENAND && \
!ENV_IS_IN_REMOTE && !ENV_IS_IN_SPI_FLASH && \
-!ENV_IS_IN_UBI
+!ENV_IS_IN_UBI && !ENV_IS_IN_SPI_NAND_FLASH
select ENV_IS_NOWHERE
config ENV_IS_NOWHERE
@@ -387,6 +387,33 @@ config ENV_IS_IN_SPI_FLASH
during a "saveenv" operation. CONFIG_ENV_OFFSET_REDUND must be
aligned to an erase sector boundary.
+config ENV_IS_IN_SPI_NAND_FLASH
+ bool "Environment is in SPI NAND flash"
+ depends on !CHAIN_OF_TRUST && (SPI_FLASH || DM_SPI_FLASH)
+ default y if ARMADA_XP
+ default y if INTEL_BAYTRAIL
+ default y if INTEL_BRASWELL
+ default y if INTEL_BROADWELL
+ default y if NORTHBRIDGE_INTEL_IVYBRIDGE
+ default y if INTEL_QUARK
+ default y if INTEL_QUEENSBAY
+ default y if ARCH_SUNXI
+ default y if ARCH_AIROHA
+ help
+ Define this if you have a SPI NAND Flash memory device which you
+ want to use for the environment.
+
+ - CONFIG_SYS_SNAND_ENV_DEV:
+
+ Specifies which SPI NAND device the environment is stored in.
+
+ - CONFIG_ENV_OFFSET:
+ - CONFIG_ENV_SIZE:
+
+ These two #defines specify the offset and size of the
+ environment area within the SPI NAND Flash.
+ CONFIG_ENV_OFFSET must be aligned to an erase sector boundary.
+
config ENV_SECT_SIZE_AUTO
bool "Use automatically detected sector size"
depends on ENV_IS_IN_SPI_FLASH
@@ -562,8 +589,8 @@ config ENV_EXT4_FILE
config ENV_ADDR
hex "Environment address"
depends on ENV_IS_IN_FLASH || ENV_IS_IN_NVRAM || ENV_IS_IN_ONENAND || \
-ENV_IS_IN_REMOTE || ENV_IS_IN_SPI_FLASH
- default 0x0 if ENV_IS_IN_SPI_FLASH
+ENV_IS_IN_REMOTE || ENV_IS_IN_SPI_FLASH ||
ENV_IS_IN_SPI_NAND_FLASH
+ default 0x0 if ENV_IS_IN_SPI_FLASH || ENV_IS_IN_SPI_NAND_FLASH
help
Offset from the start of the device (or partition)
@@ -577,7 +604,7 @@ config ENV_ADDR_REDUND
config ENV_OFFSET
hex "Environment offset"
depends on ENV_IS_IN_EEPROM || ENV_IS_IN_MMC || ENV_IS_IN_NAND || \
- ENV_IS_IN_SPI_FLASH
+ ENV_IS_IN_SPI_FLASH || ENV_IS_IN_SPI_NAND_FLASH
default 0x3f8000 if ARCH_ROCKCHIP && ENV_IS_IN_MMC
default 0x14 if ARCH_ROCKCHIP && ENV_IS_IN_SPI_FLASH
default 0xF if ARCH_SUNXI
@@ -666,6 +693,12 @@ config SYS_RELOC_GD_ENV_ADDR
Relocate the early env_addr pointer so we know it is not inside
the binary. Some systems need this and for the rest, it doesn't hurt.
+config SYS_SNAND_ENV_DEV
+ string "spi nand device name"
+ depends on ENV_IS_IN_SPI_NAND_FLASH
+ help
+ SPI NAND device name on the platform where the environment is stored.
+
config SYS_MMC_ENV_DEV
int "mmc device number"
depends on ENV_IS_IN_MMC || ENV_IS_IN_FAT || ENV_IS_IN_EXT4 || \
diff --git a/env/Makefile b/env/Makefile
index a54e924d419..14295a5fd4a 100644
--- a/env/Makefile
+++ b/env/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_$(PHASE_)ENV_IS_IN_FAT) += fat.o
obj-$(CONFIG_$(PHASE_)ENV_IS_IN_EXT4) += ext4.o
obj-$(CONFIG_$(PHASE_)ENV_IS_IN_NAND) += nand.o
obj-$(CONFIG_$(PHASE_)ENV_IS_IN_SPI_FLASH) += sf.o
+obj-$(CONFIG_$(PHASE_)ENV_IS_IN_SPI_NAND_FLASH) += snand.o
obj-$(CONFIG_$(PHASE_)ENV_IS_IN_FLASH) += flash.o
CFLAGS_embedded.o := -Wa,--no-warn -DENV_CRC=$(shell tools/envcrc 2>/dev/null)
diff --git a/env/env.c b/env/env.c
index bcc189e14db..b795cf24625 100644
--- a/env/env.c
+++ b/env/env.c
@@ -58,6 +58,9 @@ static enum env_location env_locations[] = {
#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
ENVL_SPI_FLASH,
#endif
+#ifdef CONFIG_ENV_IS_IN_SPI_NAND_FLASH
+ ENVL_SPI_NAND_FLASH,
+#endif
#ifdef CONFIG_ENV_IS_IN_UBI
ENVL_UBI,
#endif
diff --git a/env/snand.c b/env/snand.c
new file mode 100644
index 000..67388201bd3
--- /dev
[PATCH 14/14] configs: airoha: an7581_evb: Enable Airoha SNFI SPI config
Enable Airoha SNFI SPI config to enable support for SNAND flash. Signed-off-by: Christian Marangi --- configs/an7581_evb_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/configs/an7581_evb_defconfig b/configs/an7581_evb_defconfig index fa41a4bca97..c74247e13db 100644 --- a/configs/an7581_evb_defconfig +++ b/configs/an7581_evb_defconfig @@ -78,3 +78,4 @@ CONFIG_DM_SPI=y CONFIG_SHA512=y CONFIG_AIROHA_ETH=y CONFIG_MMC_MTK=y +CONFIG_AIROHA_SNFI_SPI=y -- 2.48.1
RE: [PATCH] tiny-printf: Add support for upper case hex values
From: Michael Walle
Sent: Wednesday, April 2, 2025 9:02 AM
The issue is that disabling TINY_PRINTF may not be possible (size
constraints) and some code is compiled for different stages and people
typically don't check whether the format used in printf is valid with
tiny_printf. I've had this issue already in the past, I vaguely recall
"complaining" about it on IRC.
>>>
>>> Yes, I've stumbled on "%pa" with tiny printf (i.e. in
>>> drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
>>> value, actually :) So printing anything unknown as '?' would really
>>> help here.
>>
>> Something like that would help:
>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
>> index 48db7b1f78f..b918d6d7386 100644
>> --- a/lib/tiny-printf.c
>> +++ b/lib/tiny-printf.c
>> @@ -280,6 +280,12 @@ static int _vprintf(struct printf_info *info, const
>> char *fmt, va_list va)
>> while (isalnum(fmt[0]))
>> fmt++;
>> break;
>> + } else {
>> + if (fmt[0] != '\0' && (fmt[0] == 'a'
>> || fmt[0] == 'm' ||
>> + fmt[0] == 'M'
>> || fmt[0] == 'I')) {
>> + fmt++;
>> + goto unsupported;
>> + }
>
> I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
> *content* of the pointer which is really confusing. I.e. in
> pinctrl-single.c the reg value pairs are printed like
>
> dev_dbg(dev, "reg/val %pa/0x%08x\n", ®, val);
>
> with reg being a pointer to a physical address. So with tiny_printf
> the address of reg (which is a pointer to the stack) is printed in
> this case.
>
> I don't think we can print %p without putting more logic into the
> decoding. I think the culprit here is the fallthrough to %x, which
> then leads to the confusing behavior shown above. IMHO if we want to
> avoid that, we'd have to make %p entirely unsupported.
>
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index faf55d7f327..8147ffa2c1b 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const
> char *fmt,
> va_list va)
> div_out(info, &num, div);
> }
> break;
> +#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
What if we fine-tune tinyprinf via config here?
For example SPL_USE_TINY_PRINTF_POINTER_SUPPORT and
select it by NET or NET_LWIP. If someone needs it,
the pointer output can be enabled, otherwise '?' for
unsupported is output.
> case 'p':
> - if (CONFIG_IS_ENABLED(NET) ||
> - CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG) {
> - pointer(info, fmt, va_arg(va, void *));
> - /*
> - * Skip this because it pulls in _ctype
> which is
> - * 256 bytes, and we don't generally
> implement
> - * pointer anyway
> - */
> - while (isalnum(fmt[0]))
> - fmt++;
> - break;
> - }
> - islong = true;
> - /* no break */
> + pointer(info, fmt, va_arg(va, void *));
> + /*
> + * Skip this because it pulls in _ctype which is
> + * 256 bytes, and we don't generally implement
> + * pointer anyway
> + */
> + while (isalnum(fmt[0]))
> + fmt++;
> + break;
> +#endif
> case 'x':
> if (islong) {
> num = va_arg(va, unsigned long);
> @@ -310,6 +307,8 @@ static int _vprintf(struct printf_info *info, const char
> *fmt, va_list va)
> case '%':
> out(info, '%');
> default:
> + out(info, '?');
> break;
> }
Regards
Christoph
Re: [PATCH v5] spl: remove usage of CMD_BOOT[IZ] from image parsing
On Tue Apr 1, 2025 at 4:33 AM IST, Tom Rini wrote: > On Fri, Mar 14, 2025 at 09:25:04AM +0530, Anshul Dalal wrote: > >> Using CMD_* configs from spl doesn't make logical sense. Therefore this >> patch replaces the checks for CMD_BOOT[IZ] with newly added configs >> SPL_HAS_BOOT[IZ]. >> >> SPL_HAS_BOOTZ is enabled by default for 32-bit ARM systems and >> SPL_HAS_BOOTI is enabled by default for 64-bit ARM and RISCV. This >> ensures configs relying on CMD_BOOT[IZ] in falcon boot still work. >> >> Signed-off-by: Anshul Dalal > > OK, so this needs to be introducing some library symbol which then both > CMD_BOOTx and SPL_...something select. I was thinking of chaning the Makefile with the following diff: -obj-$(CONFIG_CMD_BOOTI) += bootm.o image.o +obj-$(CONFIG_LIB_BOOTI) += image.o obj-$(CONFIG_CMD_BOOTM) += bootm.o -obj-$(CONFIG_CMD_BOOTZ) += bootm.o zimage.o +obj-$(CONFIG_LIB_BOOTZ) += zimage.o else obj-$(CONFIG_$(PHASE_)FRAMEWORK) += spl.o -obj-$(CONFIG_SPL_HAS_BOOTI) += image.o -obj-$(CONFIG_SPL_HAS_BOOTZ) += zimage.o obj-$(CONFIG_OF_LIBFDT) += bootm-fdt.o endif This would simplify the Makefile by not having duplicated configs for image/zimage and the removed bootm.o for non-spl builds should be fine since except for colibri_vf, all defconfigs with CMD_BOOTx already have CMD_BOOTM enabled. And for colibri_vf, we can safely enable CMD_BOOTM? > What we have here introduces > failure to build on some imx8 platforms such as > mx8mp_evk and then size growth on others such as imx28_xea. The imx8mp_evk build should not fail since the patch should only effect falcon mode. Only difference this patch makes is now image.o would not be compiled for the spl which is not used in non falcon boot anyways. For the imx28_xea and 3 other (imx6qdl_icore_mipi|mmc|rqs) defconfigs that use falcon mode but don't make use of CMD_BOOTx. Their size growth is expected since SPL_HAS_BOOTx is default y in falcon boot. To keep the same size, the SPL_HAS_BOOTx can be explicitly disabled for those 4 configs. For future reference is there any CI tests I can run to detect any regressions before posting patches upstream? Regards, Anshul
Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"
Am Mittwoch, 2. April 2025, 23:36:46 MESZ schrieb Heiko Stübner:
> Am Dienstag, 1. April 2025, 18:13:35 MESZ schrieb Heinrich Schuchardt:
> > On 01.04.25 17:51, Simon Glass wrote:
> > > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner wrote:
> > >>
> > >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> > >>
> > >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > >> aims to reduce EFI boot times by disabling the dhcp_run when
> > >> checking ethernet bootdevices, by preventing it from running double,
> > >> with the reasoning
> > >>
> > >> We need to call eth_bootdev_hunt() when setting up the EFI
> > >> sub-system to
> > >> supply the simple network protocol. We don't need an IP address set
> > >> up.
> > >>
> > >> That might by true for EFI, but not for everything else, because when
> > >> running distro-boot and for example the PXE method in it, nothing will
> > >> set up an IP address now.
> >
> > The removed call was dhcp_run(addr, NULL, true);
> >
> > We have:
> >
> > distro_efi_read_bootflow_net():
> > boot/bootmeth_efi.c:205:ret = dhcp_run(addr, NULL, true);
> >
> > script_read_bootflow_net():
> > boot/bootmeth_script.c:132: ret = dhcp_run(addr, fname, true);
> >
> > extlinux_pxe_read_bootflow() seems to be lacking the call.
> >
> > So instead of reverting
> > 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > we should add the missing call in extlinux_pxe_read_bootflow().
>
> doing
>
> - 8< -
> diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> index b91e61bcbc4..6e5e0f99ea4 100644
> --- a/boot/bootmeth_pxe.c
> +++ b/boot/bootmeth_pxe.c
> @@ -73,6 +73,10 @@ static int extlinux_pxe_read_bootflow(struct udevice *dev,
> return log_msg_ret("pxeb", -EPERM);
> addr = simple_strtoul(addr_str, NULL, 16);
>
> + ret = dhcp_run(addr, NULL, false);
> + if (ret)
> + return log_msg_ret("dhc", ret);
> +
> log_debug("calling pxe_get()\n");
> ret = pxe_get(addr, &bootdir, &size, false);
> log_debug("pxe_get() returned %d\n", ret);
> - 8< -
>
> does seem to work in _my_ usecase and gets me loading stuff over pxe
> again.
For reference, I implemented that now in
https://lore.kernel.org/u-boot/20250402215025.2655384-1-he...@sntech.de/
So people can decide which way to go :-)
Heiko
> I guess this whole thing now becomes a strategy decision ;-) :
>
> - it's very late in the release, do we revert to the known working state
> or hope the above fixes all issues
> - Simon's wish of not sprinkling dhcp_run calls in multiple places
>
> I guess I'm fine with both way, as either fixes my use-case ;-)
>
> Heiko
Re: [PATCH] Revert "net: eth_bootdev_hunt() should not run DHCP"
Hi Heiko,
On Thu, 3 Apr 2025 at 10:36, Heiko Stübner wrote:
>
> Am Dienstag, 1. April 2025, 18:13:35 MESZ schrieb Heinrich Schuchardt:
> > On 01.04.25 17:51, Simon Glass wrote:
> > > On Tue, 1 Apr 2025 at 21:38, Heiko Stuebner wrote:
> > >>
> > >> This reverts commit 1f68057e03206e6597ca8b2be8bb1c49d4bd47d0.
> > >>
> > >> Commit 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > >> aims to reduce EFI boot times by disabling the dhcp_run when
> > >> checking ethernet bootdevices, by preventing it from running double,
> > >> with the reasoning
> > >>
> > >> We need to call eth_bootdev_hunt() when setting up the EFI
> > >> sub-system to
> > >> supply the simple network protocol. We don't need an IP address set
> > >> up.
> > >>
> > >> That might by true for EFI, but not for everything else, because when
> > >> running distro-boot and for example the PXE method in it, nothing will
> > >> set up an IP address now.
> >
> > The removed call was dhcp_run(addr, NULL, true);
> >
> > We have:
> >
> > distro_efi_read_bootflow_net():
> > boot/bootmeth_efi.c:205:ret = dhcp_run(addr, NULL, true);
> >
> > script_read_bootflow_net():
> > boot/bootmeth_script.c:132: ret = dhcp_run(addr, fname, true);
> >
> > extlinux_pxe_read_bootflow() seems to be lacking the call.
> >
> > So instead of reverting
> > 1f68057e0320 ("net: eth_bootdev_hunt() should not run DHCP")
> > we should add the missing call in extlinux_pxe_read_bootflow().
>
> doing
>
> - 8< -
> diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> index b91e61bcbc4..6e5e0f99ea4 100644
> --- a/boot/bootmeth_pxe.c
> +++ b/boot/bootmeth_pxe.c
> @@ -73,6 +73,10 @@ static int extlinux_pxe_read_bootflow(struct udevice *dev,
> return log_msg_ret("pxeb", -EPERM);
> addr = simple_strtoul(addr_str, NULL, 16);
>
> + ret = dhcp_run(addr, NULL, false);
> + if (ret)
> + return log_msg_ret("dhc", ret);
> +
> log_debug("calling pxe_get()\n");
> ret = pxe_get(addr, &bootdir, &size, false);
> log_debug("pxe_get() returned %d\n", ret);
> - 8< -
>
> does seem to work in _my_ usecase and gets me loading stuff over pxe
> again.
>
> I guess this whole thing now becomes a strategy decision ;-) :
>
> - it's very late in the release, do we revert to the known working state
> or hope the above fixes all issues
> - Simon's wish of not sprinkling dhcp_run calls in multiple places
>
> I guess I'm fine with both way, as either fixes my use-case ;-)
For the reasons you patch seems fine to me. As you say, it is very
late in the cycle.
I already gave this feedback to Heinrich, so once the revert is in he
may have time to take a look.
Regards,
Simon
