Re: [PATCH] gpio: pca953x_gpio: support optional reset-gpios property
On 10/04/2024 14.24, Quentin Schulz wrote:
> Hi Rasmus,
>
>> @@ -321,6 +322,13 @@ static int pca953x_probe(struct udevice *dev)
>> driver_data = dev_get_driver_data(dev);
>> + /* If a reset-gpios property is present, take the device out of
>> reset. */
>> + ret = gpio_request_by_name(dev, "reset-gpios", 0, ,
>> GPIOD_IS_OUT);
>> + if (ret && ret != -ENOENT) {
>
> This seems to differ from the implementation we have for optionally
> getting gpios by index, c.f.
> https://elixir.bootlin.com/u-boot/latest/source/drivers/gpio/gpio-uclass.c#L1498
>
> """
> struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
> unsigned int index, int flags)
> {
> [...]
> rc = gpio_request_by_name(dev, propname, index, desc, flags);
>
> end:
> [...]
> if (rc)
> return ERR_PTR(rc);
> [...]
> return desc;
> }
>
> struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
> const char *id,
> unsigned int index,
> int flags)
Well, that doesn't seem to have a lot of users outside tests? We really
have way too many APIs for doing the same thing.
I copied the logic from some other driver that also had an optional
reset-gpios and checked for -ENOENT, so I assumed that was the right
thing to do.
> {
> struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags);
>
> if (IS_ERR(desc))
> return NULL;
>
> return desc;
> }
> """
>
> It seems we only need to check whether rc is non-zero, but it doesn't
> check that it's not ENOENT. I think we would benefit from having the
> same logic here.
What are you proposing exactly? That devm_gpiod_get_index_optional()
starts propagating errors which are not -ENOENT? That would make sense,
but requires that callers check three-ways (NULL, IS_ERR or valid), not
just two-ways. Dunno.
> Also, maybe we need a devm_gpio_get_by_name_optional implementation in
> the subsystem so we don't have to reimplement it in drivers that want to
> use this?
Maybe, but as I said, we already have too many helpers implemented in
terms of each other with drivers using some random one even if there
happens to be another helper that "helpfully" plugs in a 0 for the index
or whatnot.
I'm unsure just exactly what I should do from here?
Rasmus
[PATCH] gpio: pca953x_gpio: support optional reset-gpios property
The DT bindings for the pca953x family has an optional reset-gpios
property. If present, ensure that the device is taken out of reset
before attempting to read from it.
Signed-off-by: Rasmus Villemoes
---
drivers/gpio/pca953x_gpio.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpio/pca953x_gpio.c b/drivers/gpio/pca953x_gpio.c
index b0c66d18317..24b0732f89a 100644
--- a/drivers/gpio/pca953x_gpio.c
+++ b/drivers/gpio/pca953x_gpio.c
@@ -306,6 +306,7 @@ static int pca953x_probe(struct udevice *dev)
struct pca953x_info *info = dev_get_plat(dev);
struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
char name[32], label[8], *str;
+ struct gpio_desc reset;
int addr;
ulong driver_data;
int ret;
@@ -321,6 +322,13 @@ static int pca953x_probe(struct udevice *dev)
driver_data = dev_get_driver_data(dev);
+ /* If a reset-gpios property is present, take the device out of reset.
*/
+ ret = gpio_request_by_name(dev, "reset-gpios", 0, , GPIOD_IS_OUT);
+ if (ret && ret != -ENOENT) {
+ dev_err(dev, "requesting reset-gpios failed: %d\n", ret);
+ return ret;
+ }
+
info->gpio_count = driver_data & PCA_GPIO_MASK;
if (info->gpio_count > MAX_BANK * BANK_SZ) {
dev_err(dev, "Max support %d pins now\n", MAX_BANK * BANK_SZ);
--
2.40.1.1.g1c60b9335d
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
On 14/12/2023 08.33, Masahiro Yamada wrote:
> On Thu, Dec 14, 2023 at 3:12 PM Masahiro Yamada wrote:
>>
> One more question to confirm if I can use this
> for my practical use-cases.
>
> Is U-Boot able to handle FIT (includes kernel + DTs)
> and a separate initrd?
>
> # bootm :
>
>
> Presumably, it would be difficult to inject initramdisk
> into image.fit later, so I am hoping bootm would work like that,
> but I did not delve into U-Boot code.
I recently had occasion to use this, and it actually already works
out-of-the-box, but perhaps it could be better documented. Though you
need not only the ramdisk address but also the size, as in
:, and of course CONFIG_SUPPORT_RAW_INITRD.
My use case is bootstrapping: I have one FIT image (consisting of
kernel, dtbs and an initramfs) which is the one that will be written to
the target. But for bootstrapping, I (obviously) need to boot with a
different initramfs that contains the bootstrap logic. Since this
project uses fastboot, what I do is: upload the alternative initramfs,
move it out of the way ('cause fastboot only supports one single target
buffer), upload the FIT image, and "bootm $fitaddr $initrdaddr:$initrdsize".
> If it works, is it possible to verify the integrity of initrd?
No, I don't think so. In my case the FIT image is signed, and the kernel
and chosen dtb does get verified, but not the contents of the initrd.
I'm not sure how that should happen - in any case, in the fastboot case,
the host can run arbitrary shell commands so not much U-Boot can do.
Rasmus
[PATCH v2 8/8] doc: drop references to non-existing CONFIG_MEM_SUPPORT_64BIT_DATA
Such a config option does not exist. Rephrase, and avoid mentioning MEM_SUPPORT_64BIT_DATA, which is an implementation detail. Signed-off-by: Rasmus Villemoes --- doc/usage/cmd/cmp.rst | 2 +- doc/usage/cmd/cp.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/usage/cmd/cmp.rst b/doc/usage/cmd/cmp.rst index 8d196ee5786..66865ebd7ee 100644 --- a/doc/usage/cmd/cmp.rst +++ b/doc/usage/cmd/cmp.rst @@ -96,7 +96,7 @@ Configuration - The cmp command is only available if CONFIG_CMD_MEMORY=y. The cmp.q command is -only available if additionally CONFIG_MEM_SUPPORT_64BIT_DATA=y. +only available on 64-bit targets. Return value diff --git a/doc/usage/cmd/cp.rst b/doc/usage/cmd/cp.rst index 67360e30e41..bea379ff360 100644 --- a/doc/usage/cmd/cp.rst +++ b/doc/usage/cmd/cp.rst @@ -74,7 +74,7 @@ Configuration - The cp command is available if CONFIG_CMD_MEMORY=y. Support for 64 bit words -(cp.q) depends on CONFIG_MEM_SUPPORT_64BIT_DATA=y. Copying to flash depends on +(cp.q) is only available on 64-bit targets. Copying to flash depends on CONFIG_MTD_NOR_FLASH=y. Return value -- 2.40.1.1.g1c60b9335d
[PATCH v2 6/8] cmd/mem.c: fix wrong use of ifdef, drop pointless SUPPORT_64BIT_DATA macro
The macro MEM_SUPPORT_64BIT_DATA is always defined, as either 1 or 0,
so using "#ifdef MEM_SUPPORT_64BIT_DATA" doesn't do what one
expects.
This means that currently all 32 bit targets get compiled with the .q
suffix mentioned in the help text, while it doesn't actually work.
Use the proper "#if" instead.
There's really no point defining another similarly-named macro with
exactly the same value, so just use MEM_SUPPORT_64BIT_DATA throughout.
Signed-off-by: Rasmus Villemoes
---
cmd/mem.c | 38 ++
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/cmd/mem.c b/cmd/mem.c
index c696b92a274..768057e4d3f 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -35,11 +35,9 @@
DECLARE_GLOBAL_DATA_PTR;
/* Create a compile-time value */
-#ifdef MEM_SUPPORT_64BIT_DATA
-#define SUPPORT_64BIT_DATA 1
+#if MEM_SUPPORT_64BIT_DATA
#define HELP_Q ", .q"
#else
-#define SUPPORT_64BIT_DATA 0
#define HELP_Q ""
#endif
@@ -131,7 +129,7 @@ static int do_mem_nm(struct cmd_tbl *cmdtp, int flag, int
argc,
static int do_mem_mw(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
{
- ulong writeval; /* 64-bit if SUPPORT_64BIT_DATA */
+ ulong writeval; /* 64-bit if MEM_SUPPORT_64BIT_DATA */
ulong addr, count;
int size;
void *buf, *start;
@@ -152,7 +150,7 @@ static int do_mem_mw(struct cmd_tbl *cmdtp, int flag, int
argc,
/* Get the value to write.
*/
- if (SUPPORT_64BIT_DATA)
+ if (MEM_SUPPORT_64BIT_DATA)
writeval = simple_strtoull(argv[2], NULL, 16);
else
writeval = hextoul(argv[2], NULL);
@@ -170,7 +168,7 @@ static int do_mem_mw(struct cmd_tbl *cmdtp, int flag, int
argc,
while (count-- > 0) {
if (size == 4)
*((u32 *)buf) = (u32)writeval;
- else if (SUPPORT_64BIT_DATA && size == 8)
+ else if (MEM_SUPPORT_64BIT_DATA && size == 8)
*((ulong *)buf) = writeval;
else if (size == 2)
*((u16 *)buf) = (u16)writeval;
@@ -248,7 +246,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int
argc,
int rcode = 0;
const char *type;
const void *buf1, *buf2, *base;
- ulong word1, word2; /* 64-bit if SUPPORT_64BIT_DATA */
+ ulong word1, word2; /* 64-bit if MEM_SUPPORT_64BIT_DATA */
if (argc != 4)
return CMD_RET_USAGE;
@@ -276,7 +274,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int
argc,
if (size == 4) {
word1 = *(u32 *)buf1;
word2 = *(u32 *)buf2;
- } else if (SUPPORT_64BIT_DATA && size == 8) {
+ } else if (MEM_SUPPORT_64BIT_DATA && size == 8) {
word1 = *(ulong *)buf1;
word2 = *(ulong *)buf2;
} else if (size == 2) {
@@ -528,7 +526,7 @@ static int do_mem_loop(struct cmd_tbl *cmdtp, int flag, int
argc,
{
ulong addr, length, i, bytes;
int size;
- volatile ulong *llp; /* 64-bit if SUPPORT_64BIT_DATA */
+ volatile ulong *llp; /* 64-bit if MEM_SUPPORT_64BIT_DATA */
volatile u32 *longp;
volatile u16 *shortp;
volatile u8 *cp;
@@ -559,7 +557,7 @@ static int do_mem_loop(struct cmd_tbl *cmdtp, int flag, int
argc,
* If we have only one object, just run infinite loops.
*/
if (length == 1) {
- if (SUPPORT_64BIT_DATA && size == 8) {
+ if (MEM_SUPPORT_64BIT_DATA && size == 8) {
llp = (ulong *)buf;
for (;;)
i = *llp;
@@ -579,7 +577,7 @@ static int do_mem_loop(struct cmd_tbl *cmdtp, int flag, int
argc,
i = *cp;
}
- if (SUPPORT_64BIT_DATA && size == 8) {
+ if (MEM_SUPPORT_64BIT_DATA && size == 8) {
for (;;) {
llp = (ulong *)buf;
i = length;
@@ -620,8 +618,8 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag,
int argc,
{
ulong addr, length, i, bytes;
int size;
- volatile ulong *llp; /* 64-bit if SUPPORT_64BIT_DATA */
- ulong data;/* 64-bit if SUPPORT_64BIT_DATA */
+ volatile ulong *llp; /* 64-bit if MEM_SUPPORT_64BIT_DATA */
+ ulong data;/* 64-bit if MEM_SUPPORT_64BIT_DATA */
volatile u32 *longp;
volatile u16 *shortp;
volatile u8 *cp;
@@ -646,7 +644,7 @@ static int do_mem_loopw(struct cmd_tbl *cmdtp, int flag,
int argc,
length = hextoul(argv[2], NULL);
/* data to write */
- if (SUPPORT_64BIT_DATA)
+ if (MEM_SUPPORT_64BIT_DATA)
data = simple_strtoull(arg
[PATCH v2 7/8] README: drop mention of MEM_SUPPORT_64BIT_DATA
The first sentence is half-way true; the macro is always defined, but has the value 0 or 1. The second is outright false. A lot of code guarded by MEM_SUPPORT_64BIT_DATA uses a "ulong" to store values, so if sizeof(long) is not 8, that code would probably compile, but not work at all as expected. It would probably be possible to make all such code explicitly use u64 and thus make it work on 32 bit targets, but until that is done, do not pretend that it's ok to override the automatic value of MEM_SUPPORT_64BIT_DATA. Signed-off-by: Rasmus Villemoes --- README | 3 --- 1 file changed, 3 deletions(-) diff --git a/README b/README index 60c6b8a19db..bc7fb474348 100644 --- a/README +++ b/README @@ -1248,9 +1248,6 @@ typically in board_init_f() and board_init_r(). Configuration Settings: --- -- MEM_SUPPORT_64BIT_DATA: Defined automatically if compiled as 64-bit. - Optionally it can be defined to support 64-bit memory commands. - - CONFIG_SYS_LONGHELP: Defined when you want long help messages included; undefine this when you're short of memory. -- 2.40.1.1.g1c60b9335d
[PATCH v2 5/8] test: add test of "cp" shell command
Signed-off-by: Rasmus Villemoes
---
test/cmd/Makefile | 1 +
test/cmd/mem_copy.c | 169
2 files changed, 170 insertions(+)
create mode 100644 test/cmd/mem_copy.c
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index e296ba1192b..b26b5b1ea85 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CONSOLE_TRUETYPE) += font.o
obj-$(CONFIG_CMD_HISTORY) += history.o
obj-$(CONFIG_CMD_LOADM) += loadm.o
obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
+obj-$(CONFIG_CMD_MEMORY) += mem_copy.o
ifdef CONFIG_CMD_PCI
obj-$(CONFIG_CMD_PCI_MPS) += pci_mps.o
endif
diff --git a/test/cmd/mem_copy.c b/test/cmd/mem_copy.c
new file mode 100644
index 000..072159b2906
--- /dev/null
+++ b/test/cmd/mem_copy.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Tests for memory 'cp' command
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define BUF_SIZE 256
+
+/* Declare a new mem test */
+#define MEM_TEST(_name)UNIT_TEST(_name, 0, mem_test)
+
+struct param {
+ int d, s, count;
+};
+
+static int do_test(struct unit_test_state *uts,
+ const char *suffix, int d, int s, int count)
+{
+ const long addr = 0x1000;
+ u8 shadow[BUF_SIZE];
+ u8 *buf;
+ int i, w, bytes;
+
+ buf = map_sysmem(addr, BUF_SIZE);
+
+ /* Fill with distinct bytes. */
+ for (i = 0; i < BUF_SIZE; ++i)
+ buf[i] = shadow[i] = i;
+
+ /* Parameter sanity checking. */
+ w = cmd_get_data_size(suffix, 4);
+ ut_assert(w == 1 || w == 2 || w == 4 || (MEM_SUPPORT_64BIT_DATA && w ==
8));
+
+ bytes = count * w;
+ ut_assert(d < BUF_SIZE);
+ ut_assert(d + bytes <= BUF_SIZE);
+ ut_assert(s < BUF_SIZE);
+ ut_assert(s + bytes <= BUF_SIZE);
+
+ /* This is exactly what we expect to happen to "buf" */
+ memmove(shadow + d, shadow + s, bytes);
+
+ run_commandf("cp%s 0x%lx 0x%lx 0x%x", suffix, addr + s, addr + d,
count);
+
+ ut_asserteq(0, memcmp(buf, shadow, BUF_SIZE));
+
+ unmap_sysmem(buf);
+
+ return 0;
+}
+
+static int mem_test_cp_b(struct unit_test_state *uts)
+{
+ static const struct param tests[] = {
+ { 0, 128, 128 },
+ { 128, 0, 128 },
+ { 0, 16, 32 },
+ { 16, 0, 32 },
+ { 60, 100, 100 },
+ { 100, 60, 100 },
+ { 123, 54, 96 },
+ { 54, 123, 96 },
+ };
+ const struct param *p;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(tests); ++i) {
+ p = [i];
+ ret = do_test(uts, ".b", p->d, p->s, p->count);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+MEM_TEST(mem_test_cp_b);
+
+static int mem_test_cp_w(struct unit_test_state *uts)
+{
+ static const struct param tests[] = {
+ { 0, 128, 64 },
+ { 128, 0, 64 },
+ { 0, 16, 16 },
+ { 16, 0, 16 },
+ { 60, 100, 50 },
+ { 100, 60, 50 },
+ { 123, 54, 48 },
+ { 54, 123, 48 },
+ };
+ const struct param *p;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(tests); ++i) {
+ p = [i];
+ ret = do_test(uts, ".w", p->d, p->s, p->count);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+MEM_TEST(mem_test_cp_w);
+
+static int mem_test_cp_l(struct unit_test_state *uts)
+{
+ static const struct param tests[] = {
+ { 0, 128, 32 },
+ { 128, 0, 32 },
+ { 0, 16, 8 },
+ { 16, 0, 8 },
+ { 60, 100, 25 },
+ { 100, 60, 25 },
+ { 123, 54, 24 },
+ { 54, 123, 24 },
+ };
+ const struct param *p;
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(tests); ++i) {
+ p = [i];
+ ret = do_test(uts, ".l", p->d, p->s, p->count);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(tests); ++i) {
+ p = [i];
+ ret = do_test(uts, "", p->d, p->s, p->count);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+MEM_TEST(mem_test_cp_l);
+
+#if MEM_SUPPORT_64BIT_DATA
+static int mem_test_cp_q(struct unit_test_state *uts)
+{
+ static const struct param tests[] = {
+ { 0, 128, 16 },
+ { 128, 0, 16 },
+ { 0, 16, 8 },
+ { 16, 0, 8 },
+ { 60, 100, 15 },
+ { 100, 60, 15 },
+ { 123, 54, 12 },
+ { 54, 123, 12 },
+ };
+ const st
[PATCH v2 3/8] cmd/command.c: constify "arg" argument of cmd_get_data_size()
This function obviously does not and must not modify "arg". Change the
prototype to allow passing an argument of type "const char*" without
requiring a cast.
Signed-off-by: Rasmus Villemoes
---
common/command.c | 2 +-
include/command.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/command.c b/common/command.c
index 846e16e2ada..474ac98bc38 100644
--- a/common/command.c
+++ b/common/command.c
@@ -466,7 +466,7 @@ int cmd_auto_complete(const char *const prompt, char *buf,
int *np, int *colp)
#endif
#ifdef CMD_DATA_SIZE
-int cmd_get_data_size(char* arg, int default_size)
+int cmd_get_data_size(const char *arg, int default_size)
{
/* Check for a size specification .b, .w or .l.
*/
diff --git a/include/command.h b/include/command.h
index 6262365e128..6ea678fbbe6 100644
--- a/include/command.h
+++ b/include/command.h
@@ -153,7 +153,7 @@ int cmd_process_error(struct cmd_tbl *cmdtp, int err);
* Return: data size in bytes (1, 2, 4, 8) or CMD_DATA_SIZE_ERR for an invalid
* character, or CMD_DATA_SIZE_STR for a string
*/
-int cmd_get_data_size(char *arg, int default_size);
+int cmd_get_data_size(const char *arg, int default_size);
#endif
#ifdef CONFIG_CMD_BOOTD
--
2.40.1.1.g1c60b9335d
[PATCH v2 2/8] doc/usage/cmd/cp.rst: document that overlapping regions are supported
Now that the cp command is changed to use memmove() internally, update the documentation to explicitly state that overlapping regions are allowed. Signed-off-by: Rasmus Villemoes --- doc/usage/cmd/cp.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/usage/cmd/cp.rst b/doc/usage/cmd/cp.rst index 12a24e19fee..67360e30e41 100644 --- a/doc/usage/cmd/cp.rst +++ b/doc/usage/cmd/cp.rst @@ -19,7 +19,8 @@ Description The cp command is used to copy *count* chunks of memory from the *source* address to the *target* address. If the *target* address points to NOR flash, -the flash is programmed. +the flash is programmed. When the *target* address points at ordinary memory, +memmove() is used, so the two regions may overlap. The number bytes in one chunk is defined by the suffix defaulting to 4 bytes: -- 2.40.1.1.g1c60b9335d
[PATCH v2 1/8] cmd/mem.c: use memmove in do_mem_cp()
There's no 'mv' shell command for handling overlapping src and dst regions, and there's no point introducing one, when we can just make the existing 'cp' command DTRT in all cases. memmove() should at most be a few instructions more then memcpy() (to detect the appropriate direction to do the copy), which is of course completely in the noise with all the string processing that a shell command does. Reviewed-by: Simon Glass Signed-off-by: Rasmus Villemoes --- cmd/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/mem.c b/cmd/mem.c index 66c2d36a148..c696b92a274 100644 --- a/cmd/mem.c +++ b/cmd/mem.c @@ -361,7 +361,7 @@ static int do_mem_cp(struct cmd_tbl *cmdtp, int flag, int argc, } #endif - memcpy(dst, src, count * size); + memmove(dst, src, count * size); unmap_sysmem(src); unmap_sysmem(dst); -- 2.40.1.1.g1c60b9335d
[PATCH v2 4/8] cmd/command.c: relax length check in cmd_get_data_size()
Just check that the length is at least 2. This allows passing strings
like ".b", which can be convenient when constructing
tests (i.e. parametrizing the suffix used).
Signed-off-by: Rasmus Villemoes
---
common/command.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/command.c b/common/command.c
index 474ac98bc38..f0354f41644 100644
--- a/common/command.c
+++ b/common/command.c
@@ -471,7 +471,7 @@ int cmd_get_data_size(const char *arg, int default_size)
/* Check for a size specification .b, .w or .l.
*/
int len = strlen(arg);
- if (len > 2 && arg[len-2] == '.') {
+ if (len >= 2 && arg[len-2] == '.') {
switch (arg[len-1]) {
case 'b':
return 1;
--
2.40.1.1.g1c60b9335d
[PATCH v2 0/8] cmd/mem.c fixups
v2: Add Simon's R-b to patch 1. Patches 2-8 are new. This started as a simple one-liner (still patch 1), but when Simon asked me to update the documentation and add some test cases I stumbled on some things that might as well also get fixed. Patch 2 is the documentation update, might be folded into patch 1. 3 and 4 are preparations for the test cases added in patch 5. 6 is an actual fix; 32 bit targets wrongly list .q in their help texts. 7 and 8 fix stale documentation. Rasmus Villemoes (8): cmd/mem.c: use memmove in do_mem_cp() doc/usage/cmd/cp.rst: document that overlapping regions are supported cmd/command.c: constify "arg" argument of cmd_get_data_size() cmd/command.c: relax length check in cmd_get_data_size() test: add test of "cp" shell command cmd/mem.c: fix wrong use of ifdef, drop pointless SUPPORT_64BIT_DATA macro README: drop mention of MEM_SUPPORT_64BIT_DATA doc: drop references to non-existing CONFIG_MEM_SUPPORT_64BIT_DATA README| 3 - cmd/mem.c | 40 +- common/command.c | 4 +- doc/usage/cmd/cmp.rst | 2 +- doc/usage/cmd/cp.rst | 5 +- include/command.h | 2 +- test/cmd/Makefile | 1 + test/cmd/mem_copy.c | 169 ++ 8 files changed, 196 insertions(+), 30 deletions(-) create mode 100644 test/cmd/mem_copy.c -- 2.40.1.1.g1c60b9335d
Re: [PATCH v2 1/1] Makefile: pass -ansi option to cmd_gen_envp
On 20/12/2023 09.59, Sébastien Szymanski wrote: > Without the '-ansi' option, the 'linux' string in env. files is replaced > with the string '1 '. For example, in the > board/armadeus/opos6uldev/opos6uldev.env file, > > kernelimg=opos6ul-linux.bin > > becomes > > kernelimg=opos6ul-1 .bin > > in the include/generated/env.in file. > > That's because 'linux' is a System-specific Predefined Macros. [1] > > Pass the '-ansi' option as suggested by the GCC documentation. [1] So when the .dtb files are built, we use -x assembler-with-cpp -undef and that -undef seems to suppress a lot more than just the linux and unix defines. So I wonder if this case should also use that? I think that would also allow the c++ comments to remain. Rasmus
[PATCH] cmd/mem.c: use memmove in do_mem_cp()
There's no 'mv' shell command for handling overlapping src and dst regions, and there's no point introducing one, when we can just make the existing 'cp' command DTRT in all cases. memmove() should at most be a few instructions more then memcpy() (to detect the appropriate direction to do the copy), which is of course completely in the noise with all the string processing that a shell command does. Signed-off-by: Rasmus Villemoes --- cmd/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/mem.c b/cmd/mem.c index 66c2d36a148..c696b92a274 100644 --- a/cmd/mem.c +++ b/cmd/mem.c @@ -361,7 +361,7 @@ static int do_mem_cp(struct cmd_tbl *cmdtp, int flag, int argc, } #endif - memcpy(dst, src, count * size); + memmove(dst, src, count * size); unmap_sysmem(src); unmap_sysmem(dst); -- 2.40.1.1.g1c60b9335d
Re: [PATCH v2 6/6] led: add TI LP5562 LED driver
On 29/11/2023 16.23, Tom Rini wrote: > On Fri, Nov 17, 2023 at 12:38:11PM +0100, Rasmus Villemoes wrote: > >> --- >> doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++ >> drivers/led/Kconfig | 8 + >> drivers/led/Makefile | 1 + >> drivers/led/led_lp5562.c | 578 ++ >> 4 files changed, 650 insertions(+) >> create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt >> create mode 100644 drivers/led/led_lp5562.c > > If there's no further comments, I'll update the MAINTAINERS file to list > this driver under your name as well, OK? > Fine by me. Thanks, Rasmus
Re: [PATCH 1/2] global: Clean up arch/*/dts/Makefile
On 19/11/2023 20.09, Simon Glass wrote:
> On Fri, 17 Nov 2023 at 15:52, Tom Rini wrote:
>>
>> With commit 3609e1dc ("dts: automatically build necessary .dtb files")
>> we now have logic that will ensure that all device trees needed in the
>> binary are built automatically. Any device tree that the developer needs
>> while working can still be built normally via make arch/.../foo.dtb so
>> we can simply drop the rest of this logic.
>> 13 files changed, 6 insertions(+), 1570 deletions(-)
>
> I can see the motivation for this, but it ends up moving us away from
> driver model and devicetree, IMO.
No, it does not.
> For example, rk3399 and other
> rockchip boards use DT for their init so the same U-Boot can be used
> for all, mostly.
Huh? I just took a look at a few random _defconfigs that mention various
rk3399-* as CONFIG_DEFAULT_DEVICE_TREE. And they vary _wildly_ beyond
just that DEFAULT_DEVICE_TREE, so the U-Boot binary (as in the ELF file,
before being concat'ed with a .dtb or wrapped in some container) that
comes out of those must be similarly wildly different.
> Ideally that is the way all boards would be, as they
> are in Linux.
As Tom notes, that's just not gonna happen, ever, not even within a
single SOC. Something as simple as the amount of RAM may need to be
hardcoded by _some_ stage of the boot process. (Not all, I'm sure you
can find examples of SOCs where it is possible to actually build a quite
generic u-boot binary, but how that binary is then going to figure out
which .dtb is relevant either to itself or to the kernel its job is to
load is then hard to know... something like a top-level compatible
string would still need to be hard-coded somewhere in the binary).
> But here I have to manually add all the rk3399 boards to OF_LIST for
> each board
No, why? If you want to create such a generic U-Boot, you create one
single _defconfig that has all the options needed by all the various
rk3399 variants it's supposed to support, and add all the relevant .dtbs
to that defconfig's OF_LIST.
> So this is teaching people to use a board.c file, to use #ifdefs for
> device-specific differences, etc. It will be easier to do that than to
> do the right thing.
I'm sorry, but I really don't see how that follows. All this does is
eliminate duplication in our build system. Each defconfig already lists
the dtbs it works with (/is supposed to work with), which can be a wide
or narrow range of boards; some boards have multiple defconfigs simply
because the users may choose to store U-Boot (or its environment, or...)
in nand or emmc or SPI-NOR or... Sure, some of that info could come from
a .dtb, and the .dtb could somehow be chosen at run-time or some earlier
stage be tasked with figuring it out (which just leads to an elephants
all the way down problem).
> With Linux right now I can build an arm64 build which supports most of
> the boards...with about 990 devicetree files.
Well, the kernel can get away with a kitchen-sink defconfig build
resulting in a 40+MB kernel image plus hundreds of MB of modules on top.
And relying on an earlier stage picking the right .dtb for it. U-Boot
simply doesn't have that luxury, because U-Boot is that earlier stage.
Rasmus
[PATCH v2 5/6] led: led_pwm: use led_bind_generic() helper
Use the helper led_bind_generic() to reduce code duplication.
Signed-off-by: Rasmus Villemoes
---
drivers/led/led_pwm.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/led/led_pwm.c b/drivers/led/led_pwm.c
index 7c8eae9337b..ae6de3087ab 100644
--- a/drivers/led/led_pwm.c
+++ b/drivers/led/led_pwm.c
@@ -9,7 +9,6 @@
#include
#include
#include
-#include
#include
#define LEDS_PWM_DRIVER_NAME "led_pwm"
@@ -136,18 +135,7 @@ static int led_pwm_of_to_plat(struct udevice *dev)
static int led_pwm_bind(struct udevice *parent)
{
- struct udevice *dev;
- ofnode node;
- int ret;
-
- dev_for_each_subnode(node, parent) {
- ret = device_bind_driver_to_node(parent, LEDS_PWM_DRIVER_NAME,
-ofnode_get_name(node),
-node, );
- if (ret)
- return ret;
- }
- return 0;
+ return led_bind_generic(parent, LEDS_PWM_DRIVER_NAME);
}
static const struct led_ops led_pwm_ops = {
--
2.40.1.1.g1c60b9335d
[PATCH v2 6/6] led: add TI LP5562 LED driver
From: Doug Zobel
Driver for the TI LP5562 4 channel LED controller. Supports
independent on/off control of all 4 channels. Supports LED_BLINK on 3
independent channels: blue/green/red. The white channel can blink, but
shares the blue channel blink rate.
Heavily based on patch originally from Doug Zobel [1].
I have modified it so it matches the DT bindings in the linux tree,
and also follows the linux driver implementation more closely. This
should address Tom's concerns, and also matches my goal of making the
U-Boot driver work with our existing .dts which is known to work in
linux.
As our boards only have the R,G,B outputs connected, I have not
actually tested how the white channel behaves, but the R,G,B work
exactly as expected.
[1]
https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zo...@climate.com/
Cc: Doug Zobel
Signed-off-by: Rasmus Villemoes
---
doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++
drivers/led/Kconfig | 8 +
drivers/led/Makefile | 1 +
drivers/led/led_lp5562.c | 578 ++
4 files changed, 650 insertions(+)
create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
create mode 100644 drivers/led/led_lp5562.c
diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt
b/doc/device-tree-bindings/leds/leds-lp5562.txt
new file mode 100644
index 000..4e0c742959a
--- /dev/null
+++ b/doc/device-tree-bindings/leds/leds-lp5562.txt
@@ -0,0 +1,63 @@
+LEDs connected to TI LP5562 controller
+
+This driver works with a TI LP5562 4-channel LED controller.
+CONFIG_LED_BLINK is supported using the controller engines. However
+there are only 3 engines available for the 4 channels. This means
+that the blue and white channels share the same engine. When both
+blue and white LEDs are set to blink, they will share the same blink
+rate. Changing the blink rate of the blue LED will affect the white
+LED and vice-versa. Manual on/off is handled independently for all 4
+channels.
+
+Required properties:
+ - compatible : should be "ti,lp5562".
+ - #address-cells : must be 1.
+ - #size-cells : must be 0.
+ - reg : LP5562 LED controller I2C address.
+
+Optional properties:
+ - enable-gpios : Enable GPIO
+ - clock-mode : u8, configures the clock mode:
+ - 0 # automode
+ - 1 # internal
+ - 2 # external
+
+Each LED is represented as a sub-node of the ti,lp5562 device.
+
+LED sub-node required properties:
+ - reg : Zero-based channel identifier:
+- 0 red
+- 1 green
+- 2 blue
+- 3 white
+
+LED sub-node optional properties:
+ - chan-name : name of LED
+ - max-cur : LED current at max brightness in 100uA steps (0x00 - 0xFF)
+Default : 100 (10 mA)
+
+Example:
+leds0: lp5562@30 {
+compatible = "ti,lp5562";
+#address-cells = <1>;
+#size-cells = <0>;
+enable-gpios = < 9 GPIO_ACTIVE_HIGH>;
+reg = <0x30>;
+ clock-mode = /bits/8 <1>;
+
+led@0 {
+reg = <0>;
+chan-name = "red";
+max-cur = /bits/ 8 <200>; /* 20mA */
+};
+led@1 {
+reg = <1>;
+chan-name = "green";
+max-cur = /bits/ 8 <200>; /* 20mA */
+};
+led@2 {
+reg = <2>;
+chan-name = "blue";
+max-cur = /bits/ 8 <200>; /* 20mA */
+};
+};
diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 996b757e6d0..9837960198d 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -49,6 +49,14 @@ config LED_CORTINA
This option enables support for LEDs connected to the Cortina
Access CA SOCs.
+config LED_LP5562
+ bool "LED Support for LP5562"
+ depends on LED && DM_I2C
+ help
+ This option enables support for LEDs connected to the TI LP5562
+ 4 channel I2C LED controller. Driver fully supports blink on the
+ B/G/R LEDs. White LED can blink, but re-uses the period from blue.
+
config LED_PWM
bool "LED PWM"
depends on LED && DM_PWM
diff --git a/drivers/led/Makefile b/drivers/led/Makefile
index 49ae91961d5..2bcb8589087 100644
--- a/drivers/led/Makefile
+++ b/drivers/led/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_LED_BCM6858) += led_bcm6858.o
obj-$(CONFIG_LED_PWM) += led_pwm.o
obj-$(CONFIG_$(SPL_)LED_GPIO) += led_gpio.o
obj-$(CONFIG_LED_CORTINA) += led_cortina.o
+obj-$(CONFIG_LED_LP5562) += led_lp5562.o
diff --git a/drivers/led/led_lp5562.c b/drivers/led/led_lp5562.c
new file mode 100644
index 000..87479ec5515
--- /dev/null
+++
[PATCH v2 2/6] led-uclass: do not create fallback label for top-level node
Many existing drivers, and led-uclass itself, rely on uc_plat->label
being NULL for the device representing the top node, as opposed to the
child nodes representing individual LEDs. This means that the drivers
whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
Move OF "label" property parsing to core"), and also that the top node
wrongly shows up with 'led list'.
Binding the same driver to the top node as to the individual child
nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
for the top node is probably better - this has for example been done
in commits 01074697801b and 910b01c27c04.
Until remaining affected drivers are fixed, we can use a heuristic
that only sets the label to the fallback value derived from the node
name if the node does not have a "compatible" property - i.e., if it
has been bound to the LED driver explicitly via
device_bind_driver_to_node(). This is similar to what e3aa76644c2a did
for gpio_led, but that fix was then supplanted by 01074697801b.
Fixes: 83c63f0d1185 ("led: Move OF "label" property parsing to core")
Signed-off-by: Rasmus Villemoes
---
drivers/led/led-uclass.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 009f1080197..0232fa84def 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -74,7 +74,7 @@ static int led_post_bind(struct udevice *dev)
if (!uc_plat->label)
uc_plat->label = dev_read_string(dev, "label");
- if (!uc_plat->label)
+ if (!uc_plat->label && !dev_read_string(dev, "compatible"))
uc_plat->label = ofnode_get_name(dev_ofnode(dev));
uc_plat->default_state = LEDST_COUNT;
--
2.40.1.1.g1c60b9335d
[PATCH v2 4/6] led: led_gpio: use led_bind_generic() helper
Use the helper led_bind_generic() to reduce code duplication.
Signed-off-by: Rasmus Villemoes
---
drivers/led/led_gpio.c | 15 +--
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c
index fbed151b5d9..71421de628c 100644
--- a/drivers/led/led_gpio.c
+++ b/drivers/led/led_gpio.c
@@ -11,7 +11,6 @@
#include
#include
#include
-#include
struct led_gpio_priv {
struct gpio_desc gpio;
@@ -80,19 +79,7 @@ static int led_gpio_remove(struct udevice *dev)
static int led_gpio_bind(struct udevice *parent)
{
- struct udevice *dev;
- ofnode node;
- int ret;
-
- dev_for_each_subnode(node, parent) {
- ret = device_bind_driver_to_node(parent, "gpio_led",
-ofnode_get_name(node),
-node, );
- if (ret)
- return ret;
- }
-
- return 0;
+ return led_bind_generic(parent, "gpio_led");
}
static const struct led_ops gpio_led_ops = {
--
2.40.1.1.g1c60b9335d
[PATCH v2 3/6] led: introduce led_bind_generic()
All existing drivers in drivers/led/ contain a .bind method that does
exactly the same thing, with just the actual driver name
differing. Create a helper so all those individual methods can be
changed to one-liners.
Reviewed-by: Marek Vasut
Signed-off-by: Rasmus Villemoes
---
drivers/led/led-uclass.c | 18 ++
include/led.h| 8
2 files changed, 26 insertions(+)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 0232fa84def..a4be56fc258 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -11,9 +11,27 @@
#include
#include
#include
+#include
#include
#include
+int led_bind_generic(struct udevice *parent, const char *driver_name)
+{
+ struct udevice *dev;
+ ofnode node;
+ int ret;
+
+ dev_for_each_subnode(node, parent) {
+ ret = device_bind_driver_to_node(parent, driver_name,
+ofnode_get_name(node),
+node, );
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
int led_get_by_label(const char *label, struct udevice **devp)
{
struct udevice *dev;
diff --git a/include/led.h b/include/led.h
index 329041008c1..a6353166289 100644
--- a/include/led.h
+++ b/include/led.h
@@ -110,4 +110,12 @@ enum led_state_t led_get_state(struct udevice *dev);
*/
int led_set_period(struct udevice *dev, int period_ms);
+/**
+ * led_bind_generic() - bind children of parent to given driver
+ *
+ * @parent: Top-level LED device
+ * @driver_name: Driver for handling individual child nodes
+ */
+int led_bind_generic(struct udevice *parent, const char *driver_name);
+
#endif
--
2.40.1.1.g1c60b9335d
[PATCH v2 1/6] led-uclass: honour ->label field populated by driver's own .bind
If the driver's own .bind method has populated uc_plat->label, don't override that. This is necessary for an upcoming driver for ti,lp5562, where the DT binding unfortunately says to use "chan-name" and not "label". Reviewed-by: Christian Gmeiner Reviewed-by: Marek Vasut Signed-off-by: Rasmus Villemoes --- drivers/led/led-uclass.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 68ca3c29702..009f1080197 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev) struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; - uc_plat->label = dev_read_string(dev, "label"); + if (!uc_plat->label) + uc_plat->label = dev_read_string(dev, "label"); + if (!uc_plat->label) uc_plat->label = ofnode_get_name(dev_ofnode(dev)); -- 2.40.1.1.g1c60b9335d
[PATCH v2 0/6] some LED patches
I wanted to add support for ti,lp5562, and found an old submission from Doug. While trying to modify that to work in current U-Boot, I found a problem with the "move label handling to core" patches. Patch 1 is a prerequisite for the ti,lp5562 driver, which turned out to be needed by Christian as well. Patch 2 is an attempt at (quick-)fixing the mentioned "move label handling to core" problem. The real fix consists of changing remaining drivers to not bind the same driver to the top node as to the child nodes, but I can't test those other drivers. Patch 3 introduces a helper which should allow removing some boilerplate in most individual drivers, and 4,5 apply that in the gpio and pwm drivers. Converting remaining drivers is trivial, but left out for now. Finally patch 6 is the reworked lp5562 driver. While I've changed it to match existing DT bindings (with the goal of making it work with our .dts that is known to work with the linux driver), most of the logic is unchanged from Doug's original patch, so he is still listed as author. Changes in v2: Interchange order of patches 1 and 2, add a few R-bs, and try to trim down the commit message in patch 2. Doug Zobel (1): led: add TI LP5562 LED driver Rasmus Villemoes (5): led-uclass: honour ->label field populated by driver's own .bind led-uclass: do not create fallback label for top-level node led: introduce led_bind_generic() led: led_gpio: use led_bind_generic() helper led: led_pwm: use led_bind_generic() helper doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++ drivers/led/Kconfig | 8 + drivers/led/Makefile | 1 + drivers/led/led-uclass.c | 22 +- drivers/led/led_gpio.c| 15 +- drivers/led/led_lp5562.c | 578 ++ drivers/led/led_pwm.c | 14 +- include/led.h | 8 + 8 files changed, 681 insertions(+), 28 deletions(-) create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt create mode 100644 drivers/led/led_lp5562.c -- 2.40.1.1.g1c60b9335d
Re: [PATCH 0/4] bootm: Handle compressed arm64 images with bootm
On 05/11/2023 21.03, Simon Glass wrote: > This little series corrects a problem I noticed with arm64 images, > where the kernel is not recognised: The $subject is misleading, bootm works just fine with compressed arm64 images, with the type set to "kernel". > Type: Kernel Image (no loading done) > Compression: gzip compressed Isn't that a non-sensical combination to begin with? Decompressing the Image.gz kernel image to any location (however you determine that destination) _is_ loading it. If you want XIP, obviously the image must be uncompressed in the FIT. I don't understand what you're trying to do here. Rasmus
Re: [PATCH 5/4] mkimage: update man page and -h output
On 07/11/2023 08.30, Rasmus Villemoes wrote: > I'll send a revert to Tom for the prematurely applied fixup. Oh, I see that's already done. Good. Rasmus
Re: [PATCH 5/4] mkimage: update man page and -h output
On 07/11/2023 01.46, Sean Anderson wrote: > On 10/13/23 14:30, Rasmus Villemoes wrote: >> On 12/10/2023 04.17, Sean Anderson wrote: >> >>> I was hoping you would respond to my most-recent email regarding this >>> series. >>> In particular: >>> >>> | Why does mkimage have to do this? Can't you just use truncate or, in a >>> | binman context, align-size? >> >> In both cases, that just affects the size of the file, but does not >> affect the totalsize field in the fdt header. > > Use `dtc -a` then. Not possible, when the generator is binman which calls mkimage which calls dtc. If I was using dtc directly, sure. Anyway, I currently have more important things to deal with than this. Simon, please drop the series. I'll send a revert to Tom for the prematurely applied fixup. Rasmus
Re: [PATCH 1/4] mkimage: also honour -B even without external data
On 04/11/2023 20.43, Simon Glass wrote: > Hi Rasmus, > Are you planning a new version of this series? No. AFAICT there's nothing to be done on my end. Rasmus
Re: [PATCH v1 2/2] imx: spl_imx_romapi: fix emmc fast boot mode case
On 26/10/2023 09.32, Marcel Ziswiler wrote:
> From: Marcel Ziswiler
>
> This fixes a regression in the eMMC fast boot mode case where the buffer
> was missing 464 bytes.
>
> The code figures out how many bytes must at least be fetched to honor
> the current read, rounds that up to the ss->pagesize [which is a no-op
> in the USB download case because that has ->pagesize==1], fetches that
> many bytes, but then recorded the original upper bound as the new end of
> the valid data. However, this did not take into account the rounding up
> to the ss->pagesize. Fix this by recording the actual bytes downloaded.
>
> Fixes: 4b4472438f5a ("imx: spl_imx_romapi: avoid tricky use of
> spl_load_simple_fit() to get full FIT size")
> Signed-off-by: Marcel Ziswiler
>
Thanks for reporting and fixing this, and sorry for the trouble.
Acked-by: Rasmus Villemoes
Re: [REGRESSION] imx: spl_imx_romapi: boot loops
On 25/10/2023 18.01, Marcel Ziswiler wrote: > Hi Rasmus > > On Tue, 2023-10-24 at 16:32 +0200, Rasmus Villemoes wrote: >> What am I missing? > > Good question. Some more debugging revealed that we are missing 464 bytes at > the beginning of the buffer. Why > would that be? [slaps forehead] bytes = end - ss->end; bytes += ss->pagesize - 1; bytes /= ss->pagesize; bytes *= ss->pagesize; ret = rom_api_download_image(ss->end, 0, bytes); ... ss->end = end; So I figure out how many bytes must at least be fetched to honor the current read, round that up to the ss->pagesize [which is a no-op in the usb download case because that has ->pagesize==1, so I even considered leaving that rounding-up out], fetch that many bytes, but then record the original upper bound as the new end of the valid data. That can certainly account for losing something 0 < foo < pagesize number of bytes. So let's try if changing that last line to ss->end += bytes; works better. Rasmus
Re: [PATCH v3] cli: Consume invalid escape sequences early
On 10/10/2023 10.16, Yurii Monakov wrote: > Unexpected 'Esc' key presses are accumulated internally, even if it is > already clear that the current escape sequence is invalid. This results > in weird behaviour. For example, the next character after 'Esc' key > simply disappears from input and 'Unknown command' is printed > after 'Enter'. > > This commit fixes some issues with extra 'Esc' keys entered by user: > > 1. Sequence right after autoboot stop gives: > => > nknown command 'ry 'help' > => > 2. Sequence gives: > => ri > Unknown command 'ri' - try 'help' > => > 3. Extra 'Esc' key presses break backspace functionality. Thank you! This has been bugging me for years, since we have CONFIG_AUTOBOOT_DELAY_STR=" " CONFIG_AUTOBOOT_STOP_STR="\x1b" and pressing to stop autoboot did have that side-effect of swallowing the first following char. But I never found the time to dig into why or if it was even fixable. Tom has already applied this, but nevertheless Tested-by: Rasmus Villemoes Rasmus
Re: [REGRESSION] imx: spl_imx_romapi: boot loops
On 24/10/2023 15.15, Marcel Ziswiler wrote:
Hi Marcel
tl;dr: can you try
@@ -330,7 +335,7 @@ static int spl_romapi_load_image_stream(struct
spl_image_info *spl_image,
ss.base = phdr;
ss.end = p;
- ss.pagesize = pagesize;
+ ss.pagesize = 1024;
memset(, 0, sizeof(load));
load.bl_len = 1;
and also put 1024 in the binman node so u-boot.itb is also 1024-byte
aligned?
This will likely break usb boot (where pagesize==1), so if you want the
binary to work with usb you can make the RHS instead "(pagesize > 1 ?
1024 : 1)" or something like that.
I don't think it will work, but OTOH my analysis below doesn't find any
other (fundamental) difference between the old and new code.
>
> On Tue, 2023-10-24 at 13:17 +0200, Rasmus Villemoes wrote:
>
>>
>> and this works just fine. But in my case ss->pagesize is 1, whereas you
>> have 512.
>>
>> Just exactly how are you booting? It says "Boot Stage: Primary boot"
>> whereas I'm doing serial download with uuu (i.e. "Boot Stage: USB boot").
>
> Yes, regular eMMC boot.
[snip]
>
> At least I am not aware that we would be doing anything special for eMMC boot.
Well, I/we usually do boot from eMMC (except during bootstrap where we
use uuu), more precisely from the eMMC boot partitions, but that doesn't
end up using the spl_romapi_load_image_stream() but instead the expected
spl_romapi_load_image_seekable().
Perhaps some NXP folks can explain the logic behind
static int is_boot_from_stream_device(u32 boot)
{
u32 interface;
interface = boot >> 16;
if (interface >= BT_DEV_TYPE_USB)
return 1;
if (interface == BT_DEV_TYPE_MMC && (boot & 1))
return 1;
return 0;
}
Apparently that "boot & 1" is an indication that "eMMC fast boot mode"
is enabled (not to be confused with fastboot which is a USB protocol
that only becomes relevant once U-Boot proper is up and running).
As I've complained about previously, the ROM API is almost entirely
undocumented. There's something in imx93rm.pdf (which is where I have
that eMMC fast boot info from), but I have no idea if that applies to
imx8/imx8mp as well.
Are there any alignment requirements on the destination and/or the size
parameter? Is there some timing requirements so that if we read too late
the data is lost? So could e.g. a bunch of printf debugging break boot?
Because I really can't see the fundamental difference between now and
then. Both before and after the commit in question, the code does:
- read 1024 bytes at a time, search each chunk for FDT magic
This then gives the "Find img info 0x4802b000"
- make sure the leftover within that 1024 chunk is a full FDT header
(usually ok, so no output)
- read the FDT size from that header, round up to a multiple of 1024,
fetch that - that's the "Need continue download 1024".
And then the flow deviates.
The old code would do the fake spl_fit_load to figure out the maximum
offset (thus the real total .itb size), then take that remaining size,
round up to ->pagesize (512), and fetch that with one final
rom_api_download_image(p, 0, imagesize);.
My code instead does a number of rom_api_download_image() calls, where
each size argument is rounded up to the ->pagesize from the boot_info
query (i.e. 512). So it's possible that for one of those calls, the
destination is only 512-byte aligned (because a previous call fetched an
odd number of 512 byte blocks), and that doesn't happen in the previous
case where all but the last rom_api_download_image() have fetched 1024
byte aligned chunks.
What am I missing?
Rasmus
Re: [REGRESSION] imx: spl_imx_romapi: boot loops
On 24/10/2023 12.03, Marcel Ziswiler wrote:
> Hi Rasmus
>
> Thanks for your help.
>
> On Tue, 2023-10-24 at 11:18 +0200, Rasmus Villemoes wrote:
>
>
> [snip]
>
>> Hm. Can you show the result of 'fdtdump u-boot.itb | head'
>
> ⬢[zim@toolbox u-boot.git]$ fdtdump u-boot.itb | head
>
> fdtdump is a low-level debugging tool, not meant for general use.
> If you want to decompile a dtb, you probably want
> dtc -I dtb -O dts
>
> /dts-v1/;
> // magic: 0xd00dfeed
> // totalsize: 0x414 (1044)
> // off_dt_struct: 0x38
> // off_dt_strings:0x370
> // off_mem_rsvmap:0x28
> // version: 17
> // last_comp_version: 2
> // boot_cpuid_phys: 0x0
> // size_dt_strings: 0xa1
>
>> as well as
>> just the actual file sizes of u-boot.itb and flash.bin.
>
> ⬢[zim@toolbox u-boot.git]$ ls -l u-boot.itb u-boot.bin
> -rw-r--r--. 1 zim zim 882984 Oct 24 11:42 u-boot.bin
> -rw-r--r--. 1 zim zim 934504 Oct 24 11:42 u-boot.itb
>
>> Also, in spl_romapi_read_stream(), can you add a print out what
>> ss->pagesize is, and also an unconditional printf() after the
>> rom_api_download_image() - we apparently don't hit the "Failure download
>> %d", but I'd still like to see if we even return from that
>> rom_api_download_image() call.
>
> U-Boot SPL 2024.01-rc1-dirty (Oct 24 2023 - 11:56:41 +0200)
> Training FAILED
> DDR configured as single rank
> SEC0: RNG instantiated
> Normal Boot
> WDT: Started watchdog@3028 with servicing every 1000ms (60s timeout)
> Trying to boot from BOOTROM
> Boot Stage: Primary boot
> Find img info 0x4802b000, size 1044
> Need continue download 1024
> spl_romapi_read_stream ss->pagesize 0x200
> spl_romapi_read_stream ss->pagesize 0x200
> downloading another 0xc9000 bytes
> rom_api_download_image()=240
> spl_romapi_read_stream ss->pagesize 0x200
> downloading another 0x1ae00 bytes
> rom_api_download_image()=240
> spl_romapi_read_stream�
So this is interesting. I tried disabling CONFIG_IMX_HAB (because that
case doesn't create an .itb with external data) and boot on a board
which is not locked down, and I get
Trying to boot from BOOTROM
Boot Stage: USB boot
Find img info 0x4802df00, size 1504
already have 256 bytes
Need continue download 2048
spl_romapi_read_stream(): ss->pagesize = 0x1
spl_romapi_read_stream(): ss->pagesize = 0x1
downloading another 0x9d768 bytes
spl_romapi_read_stream(): rom_api_download_image -> 240
spl_romapi_read_stream(): ss->pagesize = 0x1
downloading another 0x1ff18 bytes
spl_romapi_read_stream(): rom_api_download_image -> 240
spl_romapi_read_stream(): ss->pagesize = 0x1
spl_romapi_read_stream(): ss->pagesize = 0x1
downloading another 0x1622 bytes
spl_romapi_read_stream(): rom_api_download_image -> 240
NOTICE: BL31: v2.6(release):lf-5.15.71-2.2.0-0-g3c1583ba0-dirty
NOTICE: BL31: Built : 11:00:38, Nov 21 2022
and this works just fine. But in my case ss->pagesize is 1, whereas you
have 512.
Just exactly how are you booting? It says "Boot Stage: Primary boot"
whereas I'm doing serial download with uuu (i.e. "Boot Stage: USB boot").
It seems weird that you even end up in the is_boot_from_stream_device()
if you're booting from eMMC (which would explain the 512 byte page size).
Also, the prints above reveal that the download of the 0x1ae00 succeeds,
but then we enter the function once more, and for some reason your
printf() gets cut short; in my boot, that's for fetching another 0x1622
bytes. My guess is that the board hangs because it tries to download
more data than the stream can supply, due to the rounding-up to
ss->pagesize. Can you try with this
diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi
b/arch/arm/dts/imx8mp-u-boot.dtsi
index e54df1766fa..4b5e9ff466f 100644
--- a/arch/arm/dts/imx8mp-u-boot.dtsi
+++ b/arch/arm/dts/imx8mp-u-boot.dtsi
@@ -100,6 +100,7 @@
itb {
filename = "u-boot.itb";
+ align-size = <512>;
fit {
description = "Configuration to load ATF before
U-Boot";
so that u-boot.itb is extended so it's a multiple of 512 bytes?
Rasmus
Re: [REGRESSION] imx: spl_imx_romapi: boot loops
On 24/10/2023 09.39, Marcel Ziswiler wrote:
> Hi
>
> On our weekly master upstream CI build we noticed Verdin iMX8M Plus boot
> looping.
>
> Bisecting pointed to the following commit:
>
> commit 4b4472438f5a ("imx: spl_imx_romapi: avoid tricky use of
> spl_load_simple_fit() to get full FIT size")
>
> It looks like this introduced a regression.
>
> Before and after console output (with DEBUG defined in
> arch/arm/mach-imx/spl_imx_romapi.c) below:
>
>
> After (e.g. commit 4b4472438f5 or master):
>
> U-Boot SPL 2024.01-rc1-dirty (Oct 24 2023 - 09:28:26 +0200)
> Training FAILED
> DDR configured as single rank
> SEC0: RNG instantiated
> Normal Boot
> WDT: Started watchdog@3028 with servicing every 1000ms (60s timeout)
> Trying to boot from BOOTROM
> Boot Stage: Primary boot
> Find img info 0x4802b000, size 1044
> Need continue download 1024
> downloading another 0xc9000 bytes
> downloading another 0x1ae00 bytes
Hm. Can you show the result of 'fdtdump u-boot.itb | head' as well as
just the actual file sizes of u-boot.itb and flash.bin.
Also, in spl_romapi_read_stream(), can you add a print out what
ss->pagesize is, and also an unconditional printf() after the
rom_api_download_image() - we apparently don't hit the "Failure download
%d", but I'd still like to see if we even return from that
rom_api_download_image() call.
Rasmus
Re: [PATCH 6/6] led: add TI LP5562 LED driver
On 23/10/2023 11.39, Marek Vasut wrote: > On 10/23/23 11:11, Rasmus Villemoes wrote: >> On 19/10/2023 15.58, Marek Vasut wrote: >>> On 10/19/23 11:58, Rasmus Villemoes wrote: >>>> From: Doug Zobel >>>> >>>> Driver for the TI LP5562 4 channel LED controller. Supports >>>> independent on/off control of all 4 channels. Supports LED_BLINK on 3 >>>> independent channels: blue/green/red. The white channel can blink, but >>>> shares the blue channel blink rate. >>>> >>>> Heavily based on patch originally from Doug Zobel [1]. >>>> >>>> I have modified it so it matches the DT bindings in the linux tree, >>>> and also follows the linux driver implementation more closely. This >>>> should address Tom's concerns, and also matches my goal of making the >>>> U-Boot driver work with our existing .dts which is known to work in >>>> linux. >>>> >>>> As our boards only have the R,G,B outputs connected, I have not >>>> actually tested how the white channel behaves, but the R,G,B work >>>> exactly as expected. >>>> >>>> [1] >>>> https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zo...@climate.com/ >>>> >>>> Cc: Doug Zobel >>>> Signed-off-by: Rasmus Villemoes >>>> --- >>>> doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++ >>>> drivers/led/Kconfig | 8 + >>>> drivers/led/Makefile | 1 + >>>> drivers/led/led_lp5562.c | 578 >>>> ++ >>>> 4 files changed, 650 insertions(+) >>>> create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt >>>> create mode 100644 drivers/led/led_lp5562.c >>>> >>>> diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt >>>> b/doc/device-tree-bindings/leds/leds-lp5562.txt >>>> new file mode 100644 >>>> index 00..4e0c742959 >>>> --- /dev/null >>>> +++ b/doc/device-tree-bindings/leds/leds-lp5562.txt >>> >>> Why not use Linux >>> Documentation/devicetree/bindings/leds/leds-lp55xx.yaml ? >> >> Because I'm not adding support for all the devices covered by that >> binding, nor for all the properties defined there (and some of those, I >> think, do not even make sense for the lp5562 but only apply to some of >> the other variants). > > Using old bindings like that will only cause divergence, please dont do > that. I am not doing that (i.e. using old bindings). Compared to Doug's original patch, I _have_ changed that .txt file to match the existing linux binding and hence my existing .dts. And made all the corresponding (mostly mechanical) changes in the driver. Which I also mentioned in the commit log: >>>> I have modified it so it matches the DT bindings in the linux tree, For example reading some property values using dev_read_u8, because for some reason the binding specifies that the values are /bits/8. If you want I can spell out all the changes I made. It just doesn't make sense to have that kind of info in the commit log. >>> [...] >>> >>>> --- /dev/null >>>> +++ b/drivers/led/led_lp5562.c >>>> @@ -0,0 +1,578 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> +/* >>>> + * Copyright (C) 2018 Doug Zobel >>> >>> Why not port Linux drivers/leds/leds-lp5562.c ? >> >> Because that would be way more work to make that fit into U-Boot's LED >> framework than to take Doug's initial patch and make it build against >> current U-Boot. > > Seems the drivers are just about the same complexity, so why not port > the new Linux one ? So how did you measure the complexity? LOC? If so, you may have missed drivers/leds/leds-lp55xx-common.[ch]. They are far from the same complexity, and the linux driver simply has tons of gunk that would have to be removed or #ifdef'ed out to compile in U-Boot, and I'd still need to add the U-Boot-specific glue for the struct led_ops etc. In the end I don't think much more than the register defines could actually be reused. And since I now have a driver that actually works I don't really want to try and see what it would take to port the linux driver. It even includes blink support, for which there is AFAIK no equivalent API in linux, so that would have to be implemented separately on top - I don't need it, but left it in (and tested it) because it was in Doug's original patch. If I removed that it would be cut at least 1/3 of the lines. Rasmus
Re: [PATCH 6/6] led: add TI LP5562 LED driver
On 19/10/2023 15.58, Marek Vasut wrote: > On 10/19/23 11:58, Rasmus Villemoes wrote: >> From: Doug Zobel >> >> Driver for the TI LP5562 4 channel LED controller. Supports >> independent on/off control of all 4 channels. Supports LED_BLINK on 3 >> independent channels: blue/green/red. The white channel can blink, but >> shares the blue channel blink rate. >> >> Heavily based on patch originally from Doug Zobel [1]. >> >> I have modified it so it matches the DT bindings in the linux tree, >> and also follows the linux driver implementation more closely. This >> should address Tom's concerns, and also matches my goal of making the >> U-Boot driver work with our existing .dts which is known to work in >> linux. >> >> As our boards only have the R,G,B outputs connected, I have not >> actually tested how the white channel behaves, but the R,G,B work >> exactly as expected. >> >> [1] >> https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zo...@climate.com/ >> >> Cc: Doug Zobel >> Signed-off-by: Rasmus Villemoes >> --- >> doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++ >> drivers/led/Kconfig | 8 + >> drivers/led/Makefile | 1 + >> drivers/led/led_lp5562.c | 578 ++ >> 4 files changed, 650 insertions(+) >> create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt >> create mode 100644 drivers/led/led_lp5562.c >> >> diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt >> b/doc/device-tree-bindings/leds/leds-lp5562.txt >> new file mode 100644 >> index 00..4e0c742959 >> --- /dev/null >> +++ b/doc/device-tree-bindings/leds/leds-lp5562.txt > > Why not use Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml ? Because I'm not adding support for all the devices covered by that binding, nor for all the properties defined there (and some of those, I think, do not even make sense for the lp5562 but only apply to some of the other variants). > [...] > >> --- /dev/null >> +++ b/drivers/led/led_lp5562.c >> @@ -0,0 +1,578 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (C) 2018 Doug Zobel > > Why not port Linux drivers/leds/leds-lp5562.c ? Because that would be way more work to make that fit into U-Boot's LED framework than to take Doug's initial patch and make it build against current U-Boot. Rasmus
Re: [PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind
On 19/10/2023 15.54, Marek Vasut wrote:
> On 10/19/23 11:58, Rasmus Villemoes wrote:
>> If the driver's own .bind method has populated uc_plat->label, don't
>> override that. This is necessary for an upcoming driver for ti,lp5562,
>> where the DT binding unfortunately says to use "chan-name" and not
>> "label".
>>
>> Signed-off-by: Rasmus Villemoes
>> ---
>> drivers/led/led-uclass.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>> index 5a5d07b9a7..0232fa84de 100644
>> --- a/drivers/led/led-uclass.c
>> +++ b/drivers/led/led-uclass.c
>> @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev)
>> struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> const char *default_state;
>> - uc_plat->label = dev_read_string(dev, "label");
>> + if (!uc_plat->label)
>> + uc_plat->label = dev_read_string(dev, "label");
>> +
>
> One thing I have to wonder about is, why does this controller have label
> property in the top-level node , what is that used for ?
>
> (see Linux Documentation/devicetree/bindings/leds/leds-lp55xx.yaml)
>
> Reviewed-by: Marek Vasut
Reading the linux driver, it seems that the top-level label, if any, is
used as part of the naming for individual channels if they don't have
individual chan-name properties:
if (pdata->led_config[chan].name) {
led->cdev.name = pdata->led_config[chan].name;
} else {
snprintf(name, sizeof(name), "%s:channel%d",
pdata->label ? : chip->cl->name, chan);
led->cdev.name = name;
}
but I think the rationale in d1188adb2dabc is a bit weak, since the only
example also does have individual chan-name properties.
[Complete aside: At first I thought it was related to the multi-color
LED work that has been ongoing for many many years (I think there was an
LWN article at some point), where this could be exposed as a single
multi-color LED, as opposed to the "traditional" three/four individual
LEDs. In the former case, there would only be one sysfs entry, but with
attributes exposing the multicolor functionality. I must admit I don't
know the status of that work, when something reaches v31,
http://archive.lwn.net:8080/linux-kernel/20200722071055.GA8984@amd/t/ ,
it's hard to know if it ever lands, or if pieces of it has landed.]
Rasmus
Re: [PATCH 1/6] led-uclass: do not create fallback label for top-level node
On 19/10/2023 15.51, Marek Vasut wrote:
> On 10/19/23 11:58, Rasmus Villemoes wrote:
>> Many existing drivers, and led-uclass itself, rely on uc_plat->label
>> being NULL for the device representing the top node, as opposed to the
>> child nodes representing individual LEDs. This means that the drivers
>> whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
>> Move OF "label" property parsing to core"), and also that the top node
>> wrongly shows up with 'led list'. Some drivers have since been fixed
>> up individually, e.g.
>>
>> e3aa76644c2a "led: gpio: Check device compatible string to determine
>> the top level node"
>> 01074697801b "led: gpio: Use NOP uclass driver for top-level node"
>> 910b01c27c04 "drivers: led: bcm6753: do not use null label to find the
>> top"
>>
>> Binding the same driver to the top node as to the individual child
>> nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
>> for the top node is probably better.
>
> Note that
> 83c63f0d1185 ("led: Move OF "label" property parsing to core")
> and
> 01074697801b ("led: gpio: Use NOP uclass driver for top-level node")
> were applied shortly after each other, so I don't see the point of the
> aforementioned rant.
What rant? I'm merely trying to write down what I found out while trying
to figure out why 83c63f0d1185 broke stuff, while acknowledging that the
fixes that have been applied to some drivers are probably the approach
in general.
> I sort-of understand what you are trying to do in this patch based on
> $SUBJECT of this email, but not from this wall of text, so can you
> abbreviate the commit message ?
Sure, I can try and make it shorter.
>> - if (!uc_plat->label)
>> + if (!uc_plat->label && !dev_read_string(dev, "compatible"))
>> uc_plat->label = ofnode_get_name(dev_ofnode(dev));
>
> Is there an existing driver which has a top-level DT node with "label"
> property ?
-ENOPARSE? A driver doesn't have a top-level DT node.
And regardless, this wouldn't change anything for a top-level DT node
with a "label" property, as we're still unconditionally first trying to
read a "label" property, this is merely avoiding adding a fallback value
for top-level DT nodes (using "having a "compatible" DT property as
proxy for that "is a top-level DT node"). So I really don't understand
what you are trying to ask.
Rasmus
Re: [PATCH v2] make: check if DTC variable is an absolute path
On 19/10/2023 15.04, Richard Marko wrote: > If we try to build using external dtc using > >> make DTC=dtc > > we get a confusing error like > >> make[2]: *** No rule to make target 'arch/x86/dts/bayleybay.dtb', >> needed by 'dtbs'. Stop. > > Workaround is to use > >> make DTC=$( which dtc ) > > which gives make a full path, so the dependency > is satisfied. > > This was introduced by commit d50af66 > kbuild: add dtc as dependency on .dtb file > > This patch checks that DTC is an absolute path > and fails early if not. > > We also replace `which` in `scripts/dtc-version.sh` > with POSIXy `command -v`. I think this patch is very hard to read because of the initial check that moves all the other checks a level down and increases the indent. I was more thinking you could let make do the work instead of doing it in embedded shell script. Something like diff --git a/Makefile b/Makefile index aeaa6987360..f313e9dba66 100644 --- a/Makefile +++ b/Makefile @@ -419,6 +419,11 @@ PYTHON3?= python3 DTC_INTREE := $(objtree)/scripts/dtc/dtc DTC?= $(DTC_INTREE) DTC_MIN_VERSION:= 010406 +ifneq ($(DTC),$(DTC_INTREE)) +ifneq ($(patsubst /%,,$(DTC)),) +$(error "$$(DTC) must be an absolute path") +endif +endif CHECK = sparse seems to work (though I have only tested it very lightly). One could also add ifeq ($(wildcard $(DTC)),) $(error "$$(DTC) = $(DTC) does not exist") endif inside the outer ifneq to make the failure very explicit. Rasmus
[PATCH 6/6] led: add TI LP5562 LED driver
From: Doug Zobel
Driver for the TI LP5562 4 channel LED controller. Supports
independent on/off control of all 4 channels. Supports LED_BLINK on 3
independent channels: blue/green/red. The white channel can blink, but
shares the blue channel blink rate.
Heavily based on patch originally from Doug Zobel [1].
I have modified it so it matches the DT bindings in the linux tree,
and also follows the linux driver implementation more closely. This
should address Tom's concerns, and also matches my goal of making the
U-Boot driver work with our existing .dts which is known to work in
linux.
As our boards only have the R,G,B outputs connected, I have not
actually tested how the white channel behaves, but the R,G,B work
exactly as expected.
[1]
https://lore.kernel.org/u-boot/1547150757-1561-1-git-send-email-douglas.zo...@climate.com/
Cc: Doug Zobel
Signed-off-by: Rasmus Villemoes
---
doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++
drivers/led/Kconfig | 8 +
drivers/led/Makefile | 1 +
drivers/led/led_lp5562.c | 578 ++
4 files changed, 650 insertions(+)
create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt
create mode 100644 drivers/led/led_lp5562.c
diff --git a/doc/device-tree-bindings/leds/leds-lp5562.txt
b/doc/device-tree-bindings/leds/leds-lp5562.txt
new file mode 100644
index 00..4e0c742959
--- /dev/null
+++ b/doc/device-tree-bindings/leds/leds-lp5562.txt
@@ -0,0 +1,63 @@
+LEDs connected to TI LP5562 controller
+
+This driver works with a TI LP5562 4-channel LED controller.
+CONFIG_LED_BLINK is supported using the controller engines. However
+there are only 3 engines available for the 4 channels. This means
+that the blue and white channels share the same engine. When both
+blue and white LEDs are set to blink, they will share the same blink
+rate. Changing the blink rate of the blue LED will affect the white
+LED and vice-versa. Manual on/off is handled independently for all 4
+channels.
+
+Required properties:
+ - compatible : should be "ti,lp5562".
+ - #address-cells : must be 1.
+ - #size-cells : must be 0.
+ - reg : LP5562 LED controller I2C address.
+
+Optional properties:
+ - enable-gpios : Enable GPIO
+ - clock-mode : u8, configures the clock mode:
+ - 0 # automode
+ - 1 # internal
+ - 2 # external
+
+Each LED is represented as a sub-node of the ti,lp5562 device.
+
+LED sub-node required properties:
+ - reg : Zero-based channel identifier:
+- 0 red
+- 1 green
+- 2 blue
+- 3 white
+
+LED sub-node optional properties:
+ - chan-name : name of LED
+ - max-cur : LED current at max brightness in 100uA steps (0x00 - 0xFF)
+Default : 100 (10 mA)
+
+Example:
+leds0: lp5562@30 {
+compatible = "ti,lp5562";
+#address-cells = <1>;
+#size-cells = <0>;
+enable-gpios = < 9 GPIO_ACTIVE_HIGH>;
+reg = <0x30>;
+ clock-mode = /bits/8 <1>;
+
+led@0 {
+reg = <0>;
+chan-name = "red";
+max-cur = /bits/ 8 <200>; /* 20mA */
+};
+led@1 {
+reg = <1>;
+chan-name = "green";
+max-cur = /bits/ 8 <200>; /* 20mA */
+};
+led@2 {
+reg = <2>;
+chan-name = "blue";
+max-cur = /bits/ 8 <200>; /* 20mA */
+};
+};
diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
index 996b757e6d..9837960198 100644
--- a/drivers/led/Kconfig
+++ b/drivers/led/Kconfig
@@ -49,6 +49,14 @@ config LED_CORTINA
This option enables support for LEDs connected to the Cortina
Access CA SOCs.
+config LED_LP5562
+ bool "LED Support for LP5562"
+ depends on LED && DM_I2C
+ help
+ This option enables support for LEDs connected to the TI LP5562
+ 4 channel I2C LED controller. Driver fully supports blink on the
+ B/G/R LEDs. White LED can blink, but re-uses the period from blue.
+
config LED_PWM
bool "LED PWM"
depends on LED && DM_PWM
diff --git a/drivers/led/Makefile b/drivers/led/Makefile
index 49ae91961d..2bcb858908 100644
--- a/drivers/led/Makefile
+++ b/drivers/led/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_LED_BCM6858) += led_bcm6858.o
obj-$(CONFIG_LED_PWM) += led_pwm.o
obj-$(CONFIG_$(SPL_)LED_GPIO) += led_gpio.o
obj-$(CONFIG_LED_CORTINA) += led_cortina.o
+obj-$(CONFIG_LED_LP5562) += led_lp5562.o
diff --git a/drivers/led/led_lp5562.c b/drivers/led/led_lp5562.c
new file mode 100644
index 00..87479ec551
--- /dev/null
+++ b/drivers/le
[PATCH 5/6] led: led_pwm: use led_bind_generic() helper
Signed-off-by: Rasmus Villemoes
---
drivers/led/led_pwm.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/led/led_pwm.c b/drivers/led/led_pwm.c
index 7c8eae9337..ae6de3087a 100644
--- a/drivers/led/led_pwm.c
+++ b/drivers/led/led_pwm.c
@@ -9,7 +9,6 @@
#include
#include
#include
-#include
#include
#define LEDS_PWM_DRIVER_NAME "led_pwm"
@@ -136,18 +135,7 @@ static int led_pwm_of_to_plat(struct udevice *dev)
static int led_pwm_bind(struct udevice *parent)
{
- struct udevice *dev;
- ofnode node;
- int ret;
-
- dev_for_each_subnode(node, parent) {
- ret = device_bind_driver_to_node(parent, LEDS_PWM_DRIVER_NAME,
-ofnode_get_name(node),
-node, );
- if (ret)
- return ret;
- }
- return 0;
+ return led_bind_generic(parent, LEDS_PWM_DRIVER_NAME);
}
static const struct led_ops led_pwm_ops = {
--
2.40.1.1.g1c60b9335d
[PATCH 4/6] led: led_gpio: use led_bind_generic() helper
Signed-off-by: Rasmus Villemoes
---
drivers/led/led_gpio.c | 15 +--
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/led/led_gpio.c b/drivers/led/led_gpio.c
index fbed151b5d..71421de628 100644
--- a/drivers/led/led_gpio.c
+++ b/drivers/led/led_gpio.c
@@ -11,7 +11,6 @@
#include
#include
#include
-#include
struct led_gpio_priv {
struct gpio_desc gpio;
@@ -80,19 +79,7 @@ static int led_gpio_remove(struct udevice *dev)
static int led_gpio_bind(struct udevice *parent)
{
- struct udevice *dev;
- ofnode node;
- int ret;
-
- dev_for_each_subnode(node, parent) {
- ret = device_bind_driver_to_node(parent, "gpio_led",
-ofnode_get_name(node),
-node, );
- if (ret)
- return ret;
- }
-
- return 0;
+ return led_bind_generic(parent, "gpio_led");
}
static const struct led_ops gpio_led_ops = {
--
2.40.1.1.g1c60b9335d
[PATCH 3/6] led: introduce led_bind_generic()
All existing drivers in drivers/led/ contain a .bind method that does
exactly the same thing, with just the actual driver name
differing. Create a helper so all those individual methods can be
changed to one-liners.
Signed-off-by: Rasmus Villemoes
---
drivers/led/led-uclass.c | 18 ++
include/led.h| 8
2 files changed, 26 insertions(+)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 0232fa84de..a4be56fc25 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -11,9 +11,27 @@
#include
#include
#include
+#include
#include
#include
+int led_bind_generic(struct udevice *parent, const char *driver_name)
+{
+ struct udevice *dev;
+ ofnode node;
+ int ret;
+
+ dev_for_each_subnode(node, parent) {
+ ret = device_bind_driver_to_node(parent, driver_name,
+ofnode_get_name(node),
+node, );
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
int led_get_by_label(const char *label, struct udevice **devp)
{
struct udevice *dev;
diff --git a/include/led.h b/include/led.h
index 329041008c..a635316628 100644
--- a/include/led.h
+++ b/include/led.h
@@ -110,4 +110,12 @@ enum led_state_t led_get_state(struct udevice *dev);
*/
int led_set_period(struct udevice *dev, int period_ms);
+/**
+ * led_bind_generic() - bind children of parent to given driver
+ *
+ * @parent: Top-level LED device
+ * @driver_name: Driver for handling individual child nodes
+ */
+int led_bind_generic(struct udevice *parent, const char *driver_name);
+
#endif
--
2.40.1.1.g1c60b9335d
[PATCH 2/6] led-uclass: honour ->label field populated by driver's own .bind
If the driver's own .bind method has populated uc_plat->label, don't override that. This is necessary for an upcoming driver for ti,lp5562, where the DT binding unfortunately says to use "chan-name" and not "label". Signed-off-by: Rasmus Villemoes --- drivers/led/led-uclass.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c index 5a5d07b9a7..0232fa84de 100644 --- a/drivers/led/led-uclass.c +++ b/drivers/led/led-uclass.c @@ -71,7 +71,9 @@ static int led_post_bind(struct udevice *dev) struct led_uc_plat *uc_plat = dev_get_uclass_plat(dev); const char *default_state; - uc_plat->label = dev_read_string(dev, "label"); + if (!uc_plat->label) + uc_plat->label = dev_read_string(dev, "label"); + if (!uc_plat->label && !dev_read_string(dev, "compatible")) uc_plat->label = ofnode_get_name(dev_ofnode(dev)); -- 2.40.1.1.g1c60b9335d
[PATCH 1/6] led-uclass: do not create fallback label for top-level node
Many existing drivers, and led-uclass itself, rely on uc_plat->label
being NULL for the device representing the top node, as opposed to the
child nodes representing individual LEDs. This means that the drivers
whose .probe methods rely on this were broken by 83c63f0d1185 ("led:
Move OF "label" property parsing to core"), and also that the top node
wrongly shows up with 'led list'. Some drivers have since been fixed
up individually, e.g.
e3aa76644c2a "led: gpio: Check device compatible string to determine the top
level node"
01074697801b "led: gpio: Use NOP uclass driver for top-level node"
910b01c27c04 "drivers: led: bcm6753: do not use null label to find the top"
Binding the same driver to the top node as to the individual child
nodes is arguably wrong, and the approach of using a UCLASS_NOP driver
for the top node is probably better.
But as a temporary work-around, we can use a heuristic that only sets
the label to the fallback value derived from the node name if the node
does not have a "compatible" property - i.e., if it has been bound to
the LED driver explicitly via device_bind_driver_to_node() [similar to
what e3aa76644c2a did, but that then vanished with the next commit.]
Fixes: 83c63f0d1185 ("led: Move OF "label" property parsing to core")
Signed-off-by: Rasmus Villemoes
---
drivers/led/led-uclass.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
index 68ca3c2970..5a5d07b9a7 100644
--- a/drivers/led/led-uclass.c
+++ b/drivers/led/led-uclass.c
@@ -72,7 +72,7 @@ static int led_post_bind(struct udevice *dev)
const char *default_state;
uc_plat->label = dev_read_string(dev, "label");
- if (!uc_plat->label)
+ if (!uc_plat->label && !dev_read_string(dev, "compatible"))
uc_plat->label = ofnode_get_name(dev_ofnode(dev));
uc_plat->default_state = LEDST_COUNT;
--
2.40.1.1.g1c60b9335d
[PATCH 0/6] some LED patches
I wanted to add support for ti,lp5562, and found an old submission from Doug. While trying to modify that to work in current U-Boot, I found a problem with the "move label handling to core" patches. Patch 1 is an attempt at (quick-)fixing that, though the real fix is probably to not bind the same driver to the top node as to the child nodes and using uc_plat->label to distinguish in .probe. Patch 2 touches the same area, and is needed for the lp5562 driver to work with existing DT bindings. Patch 3 introduces a helper which should allow removing some boilerplate in most individual drivers, and 4,5 apply that in the gpio and pwm drivers. Converting remaining drivers is trivial, but left out for now. Finally patch 6 is the reworked lp5562 driver. While I've changed it to match existing DT bindings (with the goal of making it work with our .dts that is known to work with the linux driver), most of the logic is unchanged from Doug's original patch, so he is still listed as author. Doug Zobel (1): led: add TI LP5562 LED driver Rasmus Villemoes (5): led-uclass: do not create fallback label for top-level node led-uclass: honour ->label field populated by driver's own .bind led: introduce led_bind_generic() led: led_gpio: use led_bind_generic() helper led: led_pwm: use led_bind_generic() helper doc/device-tree-bindings/leds/leds-lp5562.txt | 63 ++ drivers/led/Kconfig | 8 + drivers/led/Makefile | 1 + drivers/led/led-uclass.c | 22 +- drivers/led/led_gpio.c| 15 +- drivers/led/led_lp5562.c | 578 ++ drivers/led/led_pwm.c | 14 +- include/led.h | 8 + 8 files changed, 681 insertions(+), 28 deletions(-) create mode 100644 doc/device-tree-bindings/leds/leds-lp5562.txt create mode 100644 drivers/led/led_lp5562.c -- 2.40.1.1.g1c60b9335d
Re: commit 83c63f0d118 (led: Move OF "label" property parsing to core)
On 18/10/2023 09.43, Rasmus Villemoes wrote: > On 17/10/2023 17.33, Marek Vasut wrote: >> Which string ? The "bcm6753-led" is driver name , so that would have to >> be parametrized. > > Exactly. The only difference between the two examples (apart from the > scope of the variables) is the driver name, but I think a > generic_led_bind() could just do this inside the loop: > > device_bind_driver_to_node(parent, parent->driver->name, > ofnode_get_name(node), > node, ); > Ah, no, that won't work for the drivers that do distinguish between the top-level and child nodes, e.g. the gpio one. Oh well, a one-line wrapper in each driver is still better than what we have currently. Rasmus
Re: commit 83c63f0d118 (led: Move OF "label" property parsing to core)
On 17/10/2023 17.33, Marek Vasut wrote:
> On 10/17/23 15:29, Rasmus Villemoes wrote:
>> static int led_gpio_bind(struct udevice *parent)
>> {
...
>> ret = device_bind_driver_to_node(parent, "gpio_led",
>> ofnode_get_name(node),
>> node, );
>>
>> static int bcm6753_led_bind(struct udevice *parent)
>> {
>>
...
>> ret = device_bind_driver_to_node(parent, "bcm6753-led",
>> ofnode_get_name(node),
>> node, );
>>
>> and that string can, if I'm not mistaken, be gotten from
>> parent->driver->name.
>
> Which string ? The "bcm6753-led" is driver name , so that would have to
> be parametrized.
Exactly. The only difference between the two examples (apart from the
scope of the variables) is the driver name, but I think a
generic_led_bind() could just do this inside the loop:
device_bind_driver_to_node(parent, parent->driver->name,
ofnode_get_name(node),
node, );
and with that we could have most, if not all, existing .bind methods
assigned directly to generic_led_bind(), we don't even need one-line
wrapper functions in each driver passing that string explicitly.
> Make sure you test the 'led' command. The LEDs might be bound, but not
> probed (so label parsing in LED probe would be too late), and if I
> recall it right, the label parsing has to be in post-bind for the labels
> to correctly show in the 'led' command listing.
It's the led command I'm testing with (with my "new" driver for the
lp5562), and where I see the problems. Currently, the top node also gets
listed in "led list" as led-controller@30 (it's an i2c device), but
probe of that device fails because the logic in the .probe method thinks
this is a subnode (the pattern is the same as in e.g. cortina_led_probe).
Since I'm simply copying the pattern from existing drivers, I'm pretty
sure those other drivers are also currently broken (see also the fixups
I mentioned).
> I am all for generic_led_bind() .
I'll try to write some real patches.
Rasmus
commit 83c63f0d118 (led: Move OF "label" property parsing to core)
Hi,
I'm trying to resurrect an old submission of a driver for ti,lp5562, so
had occasion to dig into drivers/led/. And I think commit 83c63f0d118 is
buggy or at least incomplete.
Many of the drivers that were subsequently modified to not do that
"label" parsing rely, in their .probe method, on the top node being
distinguished by not having a ->label. And led-uclass itself does that
/* Ignore the top-level LED node */
if (uc_plat->label && !strcmp(label, uc_plat->label))
return uclass_get_device_tail(dev, 0, devp);
The drivers used to only do that label-parsing for subnodes of the top
node, so that worked, but the new code assigns some ->label for anything
bound to a LED uclass driver. There have been at least two patches to
individual drivers to fix this up since (the current top two patches
touching drivers/led/), but I think that's the wrong way to handle this.
At the same time, I actually think 83c63f0d118 didn't go far enough in
deduplicating, since all drivers are left with essentially the same loop:
static int led_gpio_bind(struct udevice *parent)
{
struct udevice *dev;
ofnode node;
int ret;
dev_for_each_subnode(node, parent) {
ret = device_bind_driver_to_node(parent, "gpio_led",
ofnode_get_name(node),
node, );
if (ret)
return ret;
}
return 0;
}
static int bcm6753_led_bind(struct udevice *parent)
{
ofnode node;
dev_for_each_subnode(node, parent) {
struct udevice *dev;
int ret;
ret = device_bind_driver_to_node(parent, "bcm6753-led",
ofnode_get_name(node),
node, );
if (ret)
return ret;
}
return 0;
}
and that string can, if I'm not mistaken, be gotten from
parent->driver->name. So we really should be able to create a
generic_led_bind() that does exactly this loop + the "label" parsing,
remove the label parsing from post_bind (so it doesn't apply to the top
nodes), and switch all drivers over to this generic_led_bind().
Alternatively, we can still create a generic_led_bind() that just does
the loop as above, but then somehow prevent the top nodes from gaining
->label, say by not adding the nodename fallback if the node has a
"compatible" property (though that seems like a hack, maybe there's a
cleaner way).
WDYT?
Rasmus
Re: [PATCH] kbuild: use which $(DTC) as a dependency
On 17/10/2023 12.44, Richard Marko wrote: > If we try to build using external dtc using > >> make DTC=dtc > > we get a confusing error like > >> make[2]: *** No rule to make target 'arch/x86/dts/bayleybay.dtb', >> needed by 'dtbs'. Stop. > > Workaround is to use > >> make DTC=$( which dtc ) > > which gives make a full path, so the dependency > is satisfied. > > This was introduced by commit > d50af66 kbuild: add dtc as dependency on .dtb files > > and we extend it so it calls which automatically > (similar to scripts/dtc-version.sh) Let's please not deviate from linux Kbuild unless we absolutely have to. In this case, passing an absolute path works just fine, and is AFAICT the only documented way to pass DTC. E.g. doc/build/gcc.rst has DTC=/usr/bin/dtc make as example. [And if we do this, then at the very least this should also be done for the .dtbo rule and wherever else $(DTC) is used. But I'm really not a fan of calling out to $(shell which ...) in the rule itself. Perhaps if it was done just once, near DTC/DTC_IN_TREE, something like (not real make syntax I think) ifneq ($(DTC), $(DTC_IN_TREE)) DTC := $(shell which $(DTC)) endif But I don't think we should be doing this at all.] Rasmus
[PATCH v2 2/2] serial: introduce CONFIG_CONSOLE_FLUSH_ON_NEWLINE
When debugging, one sometimes only gets partial output lines or
nothing at all from the last printf, because the uart has a largish
buffer, and the code after the printf() may cause the CPU to hang
before the uart IP has time to actually emit all the characters. That
can be very confusing, because one doesn't then know exactly where the
hang happens.
Introduce a config knob allowing one to wait for the uart fifo to
drain whenever a newline character is printed, roughly corresponding
to the effect of setvbuf(..., _IOLBF, ...) in ordinary C programs.
Since this uses IS_ENABLED() instead of cpp ifdef, we can remove the
ifdef around the _serial_flush() definition - if neither
CONSOLE_FLUSH_SUPPORT or CONSOLE_FLUSH_ON_NEWLINE are enabled, the
compiler elides _serial_flush(), but it won't warn about it being
unused.
Signed-off-by: Rasmus Villemoes
---
common/Kconfig | 10 ++
drivers/serial/serial-uclass.c | 8 ++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/common/Kconfig b/common/Kconfig
index 93c96f23b0..43701fe9e8 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -224,6 +224,16 @@ config CONSOLE_FLUSH_SUPPORT
help
This enables compilation of flush() function for console flush
support.
+config CONSOLE_FLUSH_ON_NEWLINE
+ bool "Flush console buffer on every newline character"
+ depends on DM_SERIAL
+ help
+ This makes the serial core code flush the console device
+ whenever a newline (\n) character has been emitted. This can
+ be especially useful when "printf debugging", as otherwise
+ lots of output could still be in the UART's FIFO by the time
+ one hits the code which causes the CPU to hang or reset.
+
config CONSOLE_MUX
bool "Enable console multiplexing"
default y if VIDEO || LCD
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 8bdcdd1eaa..df6a387284 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -182,7 +182,6 @@ int serial_initialize(void)
return serial_init();
}
-#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
static void _serial_flush(struct udevice *dev)
{
struct dm_serial_ops *ops = serial_get_ops(dev);
@@ -192,7 +191,6 @@ static void _serial_flush(struct udevice *dev)
while (ops->pending(dev, false) > 0)
;
}
-#endif
static void _serial_putc(struct udevice *dev, char ch)
{
@@ -205,6 +203,9 @@ static void _serial_putc(struct udevice *dev, char ch)
do {
err = ops->putc(dev, ch);
} while (err == -EAGAIN);
+
+ if (IS_ENABLED(CONFIG_CONSOLE_FLUSH_ON_NEWLINE) && ch == '\n')
+ _serial_flush(dev);
}
static int __serial_puts(struct udevice *dev, const char *str, size_t len)
@@ -243,6 +244,9 @@ static void _serial_puts(struct udevice *dev, const char
*str)
if (*newline && __serial_puts(dev, "\r\n", 2))
return;
+ if (IS_ENABLED(CONFIG_CONSOLE_FLUSH_ON_NEWLINE) && *newline)
+ _serial_flush(dev);
+
str += len + !!*newline;
} while (*str);
}
--
2.40.1.1.g1c60b9335d
[PATCH v2 0/2] serial: introduce CONFIG_CONSOLE_FLUSH_ON_NEWLINE
v2: - fix implementation (lacked CONFIG_ inside IS_ENABLED...). - drop dependency on CONSOLE_FLUSH_SUPPORT - that governs whether functions for explicit flushing should be built, and is not actually a requirement for allowing this implicit flushing, so the features should be selectable independently. - add Simon's R-b to 1/2 1/2 is trivial prep. Motivation from 2/2: When debugging, one sometimes only gets partial output lines or nothing at all from the last printf, because the uart has a largish buffer, and the code after the printf() may cause the CPU to hang before the uart IP has time to actually emit all the characters. That can be very confusing, because one doesn't then know exactly where the hang happens. Rasmus Villemoes (2): serial: serial-uclass.c: move definition of _serial_flush up a bit serial: introduce CONFIG_CONSOLE_FLUSH_ON_NEWLINE common/Kconfig | 10 ++ drivers/serial/serial-uclass.c | 28 2 files changed, 26 insertions(+), 12 deletions(-) -- 2.40.1.1.g1c60b9335d
[PATCH v2 1/2] serial: serial-uclass.c: move definition of _serial_flush up a bit
Preparation for next patch.
Reviewed-by: Simon Glass
Signed-off-by: Rasmus Villemoes
---
drivers/serial/serial-uclass.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 4a2da7a331..8bdcdd1eaa 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -182,6 +182,18 @@ int serial_initialize(void)
return serial_init();
}
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static void _serial_flush(struct udevice *dev)
+{
+ struct dm_serial_ops *ops = serial_get_ops(dev);
+
+ if (!ops->pending)
+ return;
+ while (ops->pending(dev, false) > 0)
+ ;
+}
+#endif
+
static void _serial_putc(struct udevice *dev, char ch)
{
struct dm_serial_ops *ops = serial_get_ops(dev);
@@ -235,18 +247,6 @@ static void _serial_puts(struct udevice *dev, const char
*str)
} while (*str);
}
-#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
-static void _serial_flush(struct udevice *dev)
-{
- struct dm_serial_ops *ops = serial_get_ops(dev);
-
- if (!ops->pending)
- return;
- while (ops->pending(dev, false) > 0)
- ;
-}
-#endif
-
static int __serial_getc(struct udevice *dev)
{
struct dm_serial_ops *ops = serial_get_ops(dev);
--
2.40.1.1.g1c60b9335d
Re: [PATCH 5/4] mkimage: update man page and -h output
On 12/10/2023 04.17, Sean Anderson wrote: > I was hoping you would respond to my most-recent email regarding this > series. > In particular: > > | Why does mkimage have to do this? Can't you just use truncate or, in a > | binman context, align-size? In both cases, that just affects the size of the file, but does not affect the totalsize field in the fdt header. Wrt. binman, just as I was somewhat misled by the short mkimage -h output into thinking that this already worked as I expected, binman's fit,align documentation can also be read that way - and the _implementation_ certainly currently unconditionally translates a fit,align property into a -B argument to mkimage. So if we don't want mkimage to support a -B by itself, binman documentation and code would probably need another patch to reject that as well. Rasmus
Re: [PATCH 5/4] mkimage: update man page and -h output
On 11/10/2023 20.37, Tom Rini wrote: > On Thu, Sep 28, 2023 at 10:02:57AM +0200, Rasmus Villemoes wrote: > >> The man page correctly said that -B was ignored without -E, while the >> `mkimage -h` output suggested otherwise. Now that -B can actually be >> used by itself, update the man page. >> >> While at it, also amend the `mkimage -h` line to mention the >> connection with -E. >> >> The FDT header is a fixed 40 bytes, so its size cannot (and is not) >> modified, while its alignment is a property of the address in RAM one >> loads the FIT to, so not something mkimage can affect in any way. (In >> the file itself, the header is of course at offset 0, which has all >> possible alignments already.) >> >> Reported-by: Sean Anderson >> Signed-off-by: Rasmus Villemoes >> Reviewed-by: Simon Glass > > Applied to u-boot/master, thanks! > Thanks, but I'm afraid that was premature. The original series which this was a fixup/followup for hasn't been applied, and Sean had reservations. I'm leaving it to Simon or Tom or whoever has final say to decide if they should eventually go in, but it would probably be good to get a verdict soonish (it really shouldn't be too controversial), and if it's a no, this should just be reverted. Rasmus
Re: [PATCH] Makefile: make u-boot-initial-env target depend explicitly on scripts_basic
On 04/10/2023 04.10, Simon Glass wrote: > On Tue, 3 Oct 2023 at 04:02, Rasmus Villemoes > wrote: >> >> We're seeing sporadic errors like >> >> ENVCinclude/generated/env.txt >> HOSTCC scripts/basic/fixdep >> ENVPinclude/generated/env.in >> ENVTinclude/generated/environment.h >> HOSTCC tools/printinitialenv >> /bin/sh: 1: scripts/basic/fixdep: not found >> make[1]: *** [scripts/Makefile.host:95: tools/printinitialenv] Error 127 >> make[1]: *** Deleting file 'tools/printinitialenv' >> make: *** [Makefile:2446: u-boot-initial-env] Error 2 >> make: *** Waiting for unfinished jobs >> >> where sometimes the "fixdep: not found" is instead "fixdep: Permission >> denied" and the Error 127 becomes 126. >> >> This smells like a race condition, and indeed it is: Currently, >> u-boot-initial-env is a prerequisite of the envtools target, which >> also lists scripts_basic as a prerequisite: >> >> envtools: u-boot-initial-env scripts_basic $(version_h) $(timestamp_h) >> tools/version.h >> $(Q)$(MAKE) $(build)=tools/env >> >> However, the u-boot-initial-env rule involves building the >> printinitialenv helper, which in turn is built using an if_changed_dep >> rule. That means we must ensure scripts/basic/fixdep is built and >> ready before trying to build printinitialenv, i.e. the >> u-boot-initial-env rule itself must depend on the phony scripts_basic >> target. >> >> Signed-off-by: Rasmus Villemoes >> --- >> Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> > > Reviewed-by: Simon Glass Thanks. Tom, can you pick this up, it's a pretty annoying race to hit in our CI, and I'd like an upstream sha1 to cherry-pick from. > I have wondered for a while if we could have a few tests of the form: > > - build sandbox > - delete an output file > - build again > - check that the build succeeds and the file is there Well, in this case one would have to delete two files I think, both the printinitialenv and the fixdep binaries. I dug a little further, and while I can reproduce with sandbox_defconfig by just building, deleting those two files, and building again, I think one reason we hit it much more easily in our actual setup (i.e. without such manual intervention) is that we set TOOLS_DEBUG=y, and because of the way that option ends up affecting the make rules (in particular, the .config is not always read in by make), fixdep ends up getting rebuilt several times due to command line change (the -g coming and going). I don't know if there's a way to fix that, or why we don't just always build the host tools with debug info (I added that option back when I had to debug some weird mkimage failure). But regardless, this patch is needed for correctness. Rasmus
[PATCH] Makefile: make u-boot-initial-env target depend explicitly on scripts_basic
We're seeing sporadic errors like ENVCinclude/generated/env.txt HOSTCC scripts/basic/fixdep ENVPinclude/generated/env.in ENVTinclude/generated/environment.h HOSTCC tools/printinitialenv /bin/sh: 1: scripts/basic/fixdep: not found make[1]: *** [scripts/Makefile.host:95: tools/printinitialenv] Error 127 make[1]: *** Deleting file 'tools/printinitialenv' make: *** [Makefile:2446: u-boot-initial-env] Error 2 make: *** Waiting for unfinished jobs where sometimes the "fixdep: not found" is instead "fixdep: Permission denied" and the Error 127 becomes 126. This smells like a race condition, and indeed it is: Currently, u-boot-initial-env is a prerequisite of the envtools target, which also lists scripts_basic as a prerequisite: envtools: u-boot-initial-env scripts_basic $(version_h) $(timestamp_h) tools/version.h $(Q)$(MAKE) $(build)=tools/env However, the u-boot-initial-env rule involves building the printinitialenv helper, which in turn is built using an if_changed_dep rule. That means we must ensure scripts/basic/fixdep is built and ready before trying to build printinitialenv, i.e. the u-boot-initial-env rule itself must depend on the phony scripts_basic target. Signed-off-by: Rasmus Villemoes --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 8af1fa9468..069c03696a 100644 --- a/Makefile +++ b/Makefile @@ -2447,7 +2447,7 @@ cmd_genenv = \ sed -e '/^\s*$$/d' | \ sort -t '=' -k 1,1 -s -o $@ -u-boot-initial-env: $(env_h) FORCE +u-boot-initial-env: scripts_basic $(env_h) FORCE $(Q)$(MAKE) $(build)=tools $(objtree)/tools/printinitialenv $(call if_changed,genenv) -- 2.40.1.1.g1c60b9335d
Re: [PATCH v2 1/1] input: avoid NULL dereference
On 03/10/2023 00.46, Tom Rini wrote:
> On Tue, Oct 03, 2023 at 12:27:25AM +0200, Heinrich Schuchardt wrote:
>>
>> error = stdio_register(dev);
>> -#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)
>> -/* check if this is the standard input device */
>> -if (!error && strcmp(env_get("stdin"), dev->name) == 0) {
>> -/* reassign the console */
>> -if (OVERWRITE_CONSOLE ||
>> -console_assign(stdin, dev->name))
>> -return -1;
>> +if ((IS_ENABLED(SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)) &&
>> +!error) {
>> +const char *cstdin;
>> +
>> +/* check if this is the standard input device */
>> +cstdin = env_get("stdin");
>> +if (cstdin && !strcmp(cstdin, dev->name)) {
>> +/* reassign the console */
>> +if (OVERWRITE_CONSOLE ||
>> +console_assign(stdin, dev->name))
>> +return -1;
>> +}
>> }
>> -#else
>> -error = error;
>> -#endif
>>
>> return 0;
>> }
>
> This is an example I think of where #if is more readable.
>
I kind of agree, but at the same time, this could just be rewritten to
avoid that extra indentation. Turn the condition around and make it an
early return, then the rest of the function need not be indented.
I also think the conversion above is buggy (loses a ! on the SPL part),
but also, the condition is needlessly convoluted to begin with.
!defined(CONFIG_SPL_BUILD) implies ENV_SUPPORT, so the condition is
equivalent to just CONFIG_IS_ENABLED(ENV_SUPPORT). So I'd add a
if (!CONFIG_IS_ENABLED(ENV_SUPPORT)
return 0;
Rasmus
Re: [PATCH] scripts/Makefile.lib: also consider $(CONFIG_SYS_BOARD)-u-boot.dtsi
On 02/10/2023 20.56, Simon Glass wrote: > Hi Tom, > > On Mon, 2 Oct 2023 at 10:22, Tom Rini wrote: >> >>> I'm really not sure that replacing build rules with a board CONFIG is >>> a good idea. I suppose part of my confusion is why the Makefile is >>> considered a problem? >> >> Because it's duplicative and as Rasmus points out, often wrong. > > To me, that is under the control of the board maintainers. They should > not put CONFIG_SYS_BOARD in the DT...it obviously goes against what DT > is for and I'm not sure what binding would allow it anyway. > > If it helps, I could do a little series to remove those? I think it is > just arch/arm/dts/rockchip-optee.dtsi ? > > This seems like a case of throwing the baby out with the bathwater. > Huh? I don't think you understood what Tom was referring to, and I certainly don't understand what you're talking about. (1) duplicative = the _defconfig already has the necessary information in *OF_LIST / DEFAULT_DEVICETREE (2) often wrong = people add .dts targets to dtb-y, not .dtb targets. They are harmless no-ops because people do get their .dtb files built due to (1). "often" is perhaps a bit of an overstatement. > We are trying to use DT (and not custom C code) to deal with the > differences between boards. So long as that is preserved, I am happy > enough. Of course. This has nothing at all to do with C code versus device tree(s). Just how we ensure that all relevant .dtbs that get built today with the massive [mostly redundant] lists will still be built if we remove all those dtb-$(FOO) += lists. >>> The OF_LIST option is a little vague but I think it means that the DTs >>> are packaged into a FIT in u-boot.img - is that right? But they >>> presumably have to be built first. >> >> No, they don't have to be built first because scripts/Makefile.dts >> ensures that we build everything in *OF_LIST. > > That is from this commit: > > 3609e1dc5f4 dts: automatically build necessary .dtb files > > which was for use by custom boards using a private branch. Did it have > a wider purpose that I didn't understand? Not at the time, but it turns out that since we now _do_ have it, new in-tree boards also don't have to modify an arch/*/dts/Makefile, and do it wrong. Rasmus
Re: [PATCH] scripts/Makefile.lib: also consider $(CONFIG_SYS_BOARD)-u-boot.dtsi
On 29/09/2023 18.02, Tom Rini wrote: > On Fri, Sep 29, 2023 at 09:15:00AM -0600, Simon Glass wrote: >> Honestly at this point I've forgotten what this is all about. Fair enough, let me try to recap, though even a summary is a bit long. (1) I wanted to do what $subject says, and you seemed to be ok with that https://lore.kernel.org/u-boot/capnjgz1b+3v_5rcsbnugz6cvo4odjjdfes0rx3imw4vjxuo...@mail.gmail.com/ (2) Tom pointed out that, unfortunately, as-is, the patch caused a bunch of build failures. With hindsight, that was inevitable. https://lore.kernel.org/u-boot/c3c94614-9916-7316-e009-04ddbdc20...@prevas.dk/ (3) I still very much would like the original patch to go in, and so I pointed out that most of the current arch/*/dts/Makefile are actually completely redundant, with the logic in scripts/Makefile.dts being in place. This is when the thread turned away from talking about the original patch, but rather the cleanup of the makefile logic that turns out to be a prerequisite for said patch to go anywhere. (4) Tom tested a patch that nuked most of the arch/arm/dts/Makefile, which revealed a few defects, partly in Makefile.dts (got fixed in 6923f49d3ac2) partly in a board defconfig (got fixed in 2d158d3c387d). >> Perhaps the easiest approach is to create a new Kconfig to control >> whether a board-level .dtsi is included in the list of wildcard >> searches. Then you can enable it for your board without affecting >> others. > > That's getting things backwards, from what this cleanup does. Today we > have messy lists of "build these device trees" and then don't use most > of them, and some of the list is just Wrong (listing dts files as an > output). (5) Then a few months passed. I'm still interested in the original patch, and also the cleanup. So I pinged Tom to redo that build test with most of the Makefile gone. While revisiting this and doing that mechanical strip-down of the Makefile, I noticed that the Makefile has grown not just one but two .dts files listed in dtb-y, which is of course bogus, and nobody noticed or cared because the scripts/Makefile.dts logic JustWorks. So, given that today, each board, in the form of the defconfig used to build for it, in 99.9% of cases already includes all the information the build system needs in order to ensure that all relevant .dtb files gets built [because there's only one and that's DEFAULT_DEVICETREE, or there's more and those are in OF_LIST or SPL_OF_LIST], and the Makefile is error-prone to maintain and when adding a new board one chooses some semi-random-list to add one's .dtbs (or .dts.) to, we want to simply stop having all those lists in arch/arm/dts/Makefile (and the other arch/*/dts/Makefile, but those are much smaller and can be handled later). No TARGET lists, no SOC lists, no nothing. Yes, there may still be some 0.1%. We're trying to figure out if they exist, which they are, and how best to handle them. An easy fix is to drop the condition on when OF_LIST is settable to something different from its default (which is DEFAULT_DEVICETREE). But without knowing just exactly which boards and which .dtbs we're talking about, it's hard to know what the best solution is or if there is actually anything that needs to be done. Rasmus
Re: [PATCH v2 1/6] Kbuild: Fix cleanup of generated sources in tools
On 20/06/2023 00.41, Tobias Deiminger wrote: > quiet_cmd_wrap = WRAP$@ > -cmd_wrap = echo "\#include <../$(patsubst $(obj)/%,%,$@)>" >$@ > +cmd_wrap = echo "\#include <../$(patsubst $(obj)/generated/%,%,$@)>" >$@ > > -$(obj)/boot/%.c $(obj)/common/%.c $(obj)/env/%.c $(obj)/lib/%.c: > +$(obj)/generated/%.c: > $(call cmd,wrap) FWIW, this change incidentally also fixes a warning (and future error!) from GNU Make. Building v2023.04 with GNU Make 4.4 produces WRAPtools/env/embedded.c tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/common/crc32.c'. tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/boot/crc32.c'. tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/lib/embedded.c'. tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/common/embedded.c'. tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/boot/embedded.c'. WRAPtools/lib/sha1.c HOSTCC tools/img2srec tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/env/sha1.c'. tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/common/sha1.c'. tools/Makefile:264: warning: pattern recipe did not update peer target 'tools/boot/sha1.c'. etc. See the first "WARNING: Future backward-incompatibility!" bullet in https://lists.gnu.org/archive/html/help-make/2022-10/msg00020.html . Rasmus
[PATCH 5/4] mkimage: update man page and -h output
The man page correctly said that -B was ignored without -E, while the `mkimage -h` output suggested otherwise. Now that -B can actually be used by itself, update the man page. While at it, also amend the `mkimage -h` line to mention the connection with -E. The FDT header is a fixed 40 bytes, so its size cannot (and is not) modified, while its alignment is a property of the address in RAM one loads the FIT to, so not something mkimage can affect in any way. (In the file itself, the header is of course at offset 0, which has all possible alignments already.) Reported-by: Sean Anderson Signed-off-by: Rasmus Villemoes --- doc/mkimage.1 | 6 -- tools/mkimage.c | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/doc/mkimage.1 b/doc/mkimage.1 index 76c7859bb0..1a4bc25936 100644 --- a/doc/mkimage.1 +++ b/doc/mkimage.1 @@ -281,8 +281,10 @@ properties. A \(oqdata-offset\(cq of 0 indicates that it starts in the first .BI \-B " alignment" .TQ .BI \-\-alignment " alignment" -The alignment, in hexadecimal, that external data will be aligned to. This -option only has an effect when \-E is specified. +The alignment, in hexadecimal, that the FDT structure will be aligned +to. With +.BR \-E , +also specifies the alignment for the external data. . .TP .BI \-p " external-position" diff --git a/tools/mkimage.c b/tools/mkimage.c index 6dfe3e1d42..a5979fa6fd 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -112,7 +112,7 @@ static void usage(const char *msg) " -f => input filename for FIT source\n" " -i => input filename for ramdisk file\n" " -E => place data outside of the FIT structure\n" - " -B => align size in hex for FIT structure and header\n" + " -B => align size in hex for FIT structure and, with -E, for the external data\n" " -b => append the device tree binary to the FIT\n" " -t => update the timestamp in the FIT\n"); #ifdef CONFIG_FIT_SIGNATURE -- 2.37.2
Re: [PATCH 1/4] mkimage: also honour -B even without external data
On 27/09/2023 21.02, Sean Anderson wrote: > On 9/19/23 07:37, Rasmus Villemoes wrote: >> In some cases, using the "external data" feature is impossible or >> undesirable, but one may still want (or need) the FIT image to have a >> certain alignment. Also, given the current 'mkimage -h' output, >> >> -B => align size in hex for FIT structure and header >> >> it is quite unexpected for -B to be effectively ignored without -E. > > FWIW, this behavior is documented in doc/mkimage.1 (which should also be > updated if this behavior is implemented): > > | The alignment, in hexadecimal, that external data will be aligned to. > | This option only has an effect when -E is specified. I'll send a follow-up to fix that, thanks. > And, for additional context, the documentation for -E is > > | After processing, move the image data outside the FIT and store a data > | offset in the FIT. Images will be placed one after the other > | immediately after the FIT, with each one aligned to a 4-byte boundary. > | The existing ‘data’ property in each image will be replaced with > | ‘data-offset’ and ‘data-size’ properties. A ‘data-offset’ of 0 > | indicates that it starts in the first (4-byte-aligned) byte after the > | FIT. > > Based on this documentation and my understanding of the code as-is, -B > controls the alignment of the images themselves, Yes, when -E is in effect. My patch does not affect the case where -E is present. > not the size multiple of the FIT. It is _also_ that, but mostly as a "necessary side effect" of getting the first image aligned. In order to achieve the desired alignment of the first external image, the FIT structure itself is aligned to the -B value, and each image's size similarly aligned up to that value so the next image ends on a -B multiple again. So with both -E and -B, the FIT structure itself is indeed also padded to the -B value, as the `mkimage -h` output suggests. What I want, and expected from `mkimage -h`, is to make the whole FIT have a certain size multiple, without using -E, so in that case the alignment of the FIT is the primary goal. > However, from what I can tell, this patch does not actually > affect the alignment of the images, No, because the images are (still) embedded within the FIT as ordinary values of the data= properties, and it's a basic property of the DTB format that those are always 4-byte aligned, and you can't (easily) change that [1]. Which, I suppose, is one of the reasons U-Boot invented the "external data" mechanism - so for example the .dtbs embedded in the FIT can all be guaranteed to be on an 8-byte boundary, and thus the selected one can be used in-place when passed to linux. > but rather adjusts the size of the > overall FIT to a certain alignment. I find this rather unexpected. Well, as I said, _I_ was surprised by -B having no effect based on the `mkimage -h` output, so the two sources of documentation were not in sync. The man page was/is correct wrt. the actual code. Rasmus [1] There's a nop tag one can insert, and I think I saw somebody actually suggesting to do that to align the embedded data= properties, but it's quite error-prone and inflexible, as any subsequent modification of the .dtb before that property will ruin the alignment.
Re: [PATCH 4/4] binman: update documentation for fit,align property
On 25/09/2023 17.14, Jonas Karlman wrote: >> fit,align >> -Indicates what alignment to use for the FIT and its external data, >> -and provides the alignment to use. This is passed to mkimage via >> -the -B flag. >> +Indicates what alignment to use for the FIT and, if applicable, >> +its external data. This is passed to mkimage via the -B flag. > > This only updates entries.rst, please update tools/binman/etype/fit.py > and re-generate entries.rst from output of running binman entry-docs. Ah, I didn't know the .rst was generated. Simon, want me to resend (this one or the whole series?), or can you fold in this when applying: diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 2c14b15b03..97d3cedaf5 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -71,9 +71,8 @@ class Entry_fit(Entry_section): external offset. This is passed to mkimage via the -E and -p flags. fit,align -Indicates what alignment to use for the FIT and its external data, -and provides the alignment to use. This is passed to mkimage via -the -B flag. +Indicates what alignment to use for the FIT and, if applicable, +its external data. This is passed to mkimage via the -B flag. fit,fdt-list Indicates the entry argument which provides the list of device tree Rasmus
Re: [PATCH] scripts/Makefile.lib: also consider $(CONFIG_SYS_BOARD)-u-boot.dtsi
On 25/09/2023 20.19, Tom Rini wrote: > On Mon, Sep 25, 2023 at 10:27:43AM +0200, Rasmus Villemoes wrote: >> On 04/05/2023 14.35, Rasmus Villemoes wrote: >>> On 03/05/2023 16.54, Tom Rini wrote: >> >>>> The one last problem now is on stm32mp15_dhcor_basic which is a >>>> defconfig missing one from OF_LIST but including it in the its file, so >>>> the above is the patch we need. >>>> >> >> Hi Tom >> >> Can I persuade you to try something like >> https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 >> again, but leaving the .dtbo targets in there? >> >> I could send a patch, but it's entirely mechanical, and not really meant >> for being applied until we know if there's more to be cleaned up. > > So what ended up being the problem I think is the case Simon pointed out > where we do take the output from "make all" and concatenate one of the > dtbs that was generated with u-boot.img or so, and it works. But maybe > that should just list all of the valid DTBs that it needs in the > defconfig to start with? I don't quite know, it was a case I hadn't > considered at the time. > Re-reading the thread, I can't see where that was mentioned. But yes, if some boards (still) need that, and have more than one possible .dtb, the board can't set an OF_LIST different from the default consisting of DEFAULT_DEVICE_TREE because changing OF_LIST requires SPL_LOAD_FIT || MULTI_DTB_FIT. How do we figure out if such boards even exist? Rasmus
Re: [PATCH 1/4] mkimage: also honour -B even without external data
On 25/09/2023 15.10, Simon Glass wrote: > Hi Rasmus, > > On Mon, 25 Sept 2023 at 02:47, Rasmus Villemoes > wrote: >> Since patches 2,3,4 touch binman code, could you take all four? > > Yes, will do. I didn't pick them up in the most recent PR as I try to > have things sit for a week before applying. Oh, absolutely, no rush :) Rasmus
[PATCH 2/2] serial: introduce CONFIG_CONSOLE_FLUSH_ON_NEWLINE
When debugging, one sometimes only gets partial output lines or
nothing at all from the last printf, because the uart has a largish
buffer, and the code after the printf() may cause the CPU to hang
before the uart IP has time to actually emit all the characters. That
can be very confusing, because one doesn't then know exactly where the
hang happens.
Introduce a config knob allowing one to wait for the uart fifo to
drain whenever a newline character is printed, roughly corresponding
to the effect of setvbuf(..., _IOLBF, ...) in ordinary C programs.
Signed-off-by: Rasmus Villemoes
---
common/Kconfig | 11 +++
drivers/serial/serial-uclass.c | 8 ++--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/common/Kconfig b/common/Kconfig
index cdb77a6a7d..49130f2a2b 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -224,6 +224,17 @@ config CONSOLE_FLUSH_SUPPORT
help
This enables compilation of flush() function for console flush
support.
+config CONSOLE_FLUSH_ON_NEWLINE
+ bool "Flush console buffer on every newline character"
+ depends on CONSOLE_FLUSH_SUPPORT
+ depends on DM_SERIAL
+ help
+ This makes the serial core code flush the console device
+ whenever a newline (\n) character has been emitted. This can
+ be especially useful when "printf debugging", as otherwise
+ lots of output could still be in the UART's FIFO by the time
+ one hits the code which causes the CPU to hang or reset.
+
config CONSOLE_MUX
bool "Enable console multiplexing"
default y if VIDEO || LCD
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 2f75ff878c..b41e3ebe7f 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -181,7 +181,6 @@ int serial_initialize(void)
return serial_init();
}
-#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
static void _serial_flush(struct udevice *dev)
{
struct dm_serial_ops *ops = serial_get_ops(dev);
@@ -191,7 +190,6 @@ static void _serial_flush(struct udevice *dev)
while (ops->pending(dev, false) > 0)
;
}
-#endif
static void _serial_putc(struct udevice *dev, char ch)
{
@@ -204,6 +202,9 @@ static void _serial_putc(struct udevice *dev, char ch)
do {
err = ops->putc(dev, ch);
} while (err == -EAGAIN);
+
+ if (IS_ENABLED(CONSOLE_FLUSH_ON_NEWLINE) && ch == '\n')
+ _serial_flush(dev);
}
static int __serial_puts(struct udevice *dev, const char *str, size_t len)
@@ -242,6 +243,9 @@ static void _serial_puts(struct udevice *dev, const char
*str)
if (*newline && __serial_puts(dev, "\r\n", 2))
return;
+ if (IS_ENABLED(CONSOLE_FLUSH_ON_NEWLINE) && *newline)
+ _serial_flush(dev);
+
str += len + !!*newline;
} while (*str);
}
--
2.37.2
[PATCH 1/2] serial: serial-uclass.c: move definition of _serial_flush up a bit
Preparation for next patch.
Signed-off-by: Rasmus Villemoes
---
drivers/serial/serial-uclass.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 067fae2614..2f75ff878c 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -181,6 +181,18 @@ int serial_initialize(void)
return serial_init();
}
+#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
+static void _serial_flush(struct udevice *dev)
+{
+ struct dm_serial_ops *ops = serial_get_ops(dev);
+
+ if (!ops->pending)
+ return;
+ while (ops->pending(dev, false) > 0)
+ ;
+}
+#endif
+
static void _serial_putc(struct udevice *dev, char ch)
{
struct dm_serial_ops *ops = serial_get_ops(dev);
@@ -234,18 +246,6 @@ static void _serial_puts(struct udevice *dev, const char
*str)
} while (*str);
}
-#ifdef CONFIG_CONSOLE_FLUSH_SUPPORT
-static void _serial_flush(struct udevice *dev)
-{
- struct dm_serial_ops *ops = serial_get_ops(dev);
-
- if (!ops->pending)
- return;
- while (ops->pending(dev, false) > 0)
- ;
-}
-#endif
-
static int __serial_getc(struct udevice *dev)
{
struct dm_serial_ops *ops = serial_get_ops(dev);
--
2.37.2
[PATCH 0/2] serial: introduce CONFIG_CONSOLE_FLUSH_ON_NEWLINE
1/2 is trivial prep. Commit log from that 2/2: When debugging, one sometimes only gets partial output lines or nothing at all from the last printf, because the uart has a largish buffer, and the code after the printf() may cause the CPU to hang before the uart IP has time to actually emit all the characters. That can be very confusing, because one doesn't then know exactly where the hang happens. Introduce a config knob allowing one to wait for the uart fifo to drain whenever a newline character is printed, roughly corresponding to the effect of setvbuf(..., _IOLBF, ...) in ordinary C programs. Rasmus Villemoes (2): serial: serial-uclass.c: move definition of _serial_flush up a bit serial: introduce CONFIG_CONSOLE_FLUSH_ON_NEWLINE common/Kconfig | 11 +++ drivers/serial/serial-uclass.c | 28 2 files changed, 27 insertions(+), 12 deletions(-) -- 2.37.2
Re: [PATCH 0/2] make CONFIG_SPL_SYS_MALLOC_SIMPLE && CONFIG_SYS_SPL_MALLOC actually work
On 21/09/2023 03.02, Simon Glass wrote: > Hi Rasmus, > > On Fri, 15 Sept 2023 at 11:51, Rasmus Villemoes > wrote: >> >> Currently, setting both CONFIG_SPL_SYS_MALLOC_SIMPLE and >> CONFIG_SYS_SPL_MALLOC (but not CONFIG_SPL_STACK_R) doesn't work as >> expected: The SIMPLE option means that all malloc etc. calls are >> directed at build-time to the implementation in malloc_simple.c, but > > Sort-of. It is control by both a CONFIG and GD_FLG_FULL_MALLOC_INIT. > Eh, no? The CONFIG option completely preempts any run-time gd flag checking #if CONFIG_IS_ENABLED(SYS_MALLOC_SIMPLE) #define malloc malloc_simple #define realloc realloc_simple #define memalign memalign_simple Yes, _without_ that CONFIG option, the "real" malloc() functions do check that gd flag and if not set fall back to the _simple variants. But what I wrote is not merely "sort-of" correct. Rasmus
Re: [PATCH 1/4] mkimage: also honour -B even without external data
On 22/09/2023 17.26, Simon Glass wrote: >>> Shouldn't this be fdt_open_into()? >> >> I honestly just copy-pasted fit_extract_data() and shaved it down to the >> part that does the "align the FDT part of the file". >> >> I don't really understand your question. Are you saying this doesn't >> work (or maybe doesn't work in some cases), or are you saying that >> there's a simpler way to do this? For the latter, sure, one doesn't >> really need to parse the whole FDT; we could just >> >> open() >> pread() length from FDT header, convert to cpu-endianness >> length = ALIGN(length) >> pwrite() the new length in fdt-endianness >> ftruncate() >> close() >> >> but I thought it was better to stay closer to how fit_extract_data() was >> done. > > I mean that fdt_open_into() does more than just set the size (from > what I can tell). But looking further I see other code which calls > fdt_set_totalsize() so perhaps it is fine. Yes, I think it's as it should be - as a I said, this is really just a trimmed-down copy of the function which moves the data externally, and also needs to make the size of the base fdt structure aligned. Since patches 2,3,4 touch binman code, could you take all four? Thanks, Rasmus
Re: [PATCH] scripts/Makefile.lib: also consider $(CONFIG_SYS_BOARD)-u-boot.dtsi
On 04/05/2023 14.35, Rasmus Villemoes wrote: > On 03/05/2023 16.54, Tom Rini wrote: >> The one last problem now is on stm32mp15_dhcor_basic which is a >> defconfig missing one from OF_LIST but including it in the its file, so >> the above is the patch we need. >> > > Hm, well, for now I think at least all .dtbo targets need to be left in > the Makefiles, since nothing in the defconfig tells us to build those. Hi Tom Can I persuade you to try something like https://source.denx.de/u-boot/u-boot/-/commit/a05e0d0e6b9103542a1076f9cab0005f400fa072 again, but leaving the .dtbo targets in there? I could send a patch, but it's entirely mechanical, and not really meant for being applied until we know if there's more to be cleaned up. Rasmus
[PATCH 5/5] doc: use .dtso as extension for device tree overlay sources
Moving towards using .dtso for overlay sources, update the documentation examples to follow that pattern. Signed-off-by: Rasmus Villemoes --- doc/develop/uefi/uefi.rst | 4 ++-- doc/usage/fdt_overlays.rst | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst index a7a41f2fac..34dab3c12c 100644 --- a/doc/develop/uefi/uefi.rst +++ b/doc/develop/uefi/uefi.rst @@ -563,10 +563,10 @@ To insert the lowest supported version into a dtb .. code-block:: console -$ dtc -@ -I dts -O dtb -o version.dtbo version.dts +$ dtc -@ -I dts -O dtb -o version.dtbo version.dtso $ fdtoverlay -i orig.dtb -o new.dtb -v version.dtbo -where version.dts looks like:: +where version.dtso looks like:: /dts-v1/; /plugin/; diff --git a/doc/usage/fdt_overlays.rst b/doc/usage/fdt_overlays.rst index 7f113edae3..81d0d37f3f 100644 --- a/doc/usage/fdt_overlays.rst +++ b/doc/usage/fdt_overlays.rst @@ -43,7 +43,7 @@ traditional binary device-tree. For example: $ dtc -@ -I dts -O dtb -o base.dtb base.dts -**overlay.dts** +**overlay.dtso** :: @@ -63,7 +63,7 @@ traditional binary device-tree. For example: .. code-block:: console - $ dtc -@ -I dts -O dtb -o overlay.dtbo overlay.dts + $ dtc -@ -I dts -O dtb -o overlay.dtbo overlay.dtso Ways to Utilize Overlays in U-Boot -- -- 2.37.2
[PATCH 4/5] sandbox: rename overlay sources to .dtso
Distinguish more clearly between source files meant for producing .dtb
from those meant for producing .dtbo. No functional change, as we
currently have rules for producing a foo.dtbo from either foo.dts or
foo.dtso.
Note that in the linux tree, all device tree overlay sources have been
renamed to .dtso, and the .dts->.dtbo rule is gone since v6.5 (commit
81d362732bac). So this is also a step towards staying closer to linux
with respect to both Kbuild and device tree sources.
Signed-off-by: Rasmus Villemoes
---
arch/sandbox/dts/{overlay0.dts => overlay0.dtso} | 0
arch/sandbox/dts/{overlay1.dts => overlay1.dtso} | 0
2 files changed, 0 insertions(+), 0 deletions(-)
rename arch/sandbox/dts/{overlay0.dts => overlay0.dtso} (100%)
rename arch/sandbox/dts/{overlay1.dts => overlay1.dtso} (100%)
diff --git a/arch/sandbox/dts/overlay0.dts b/arch/sandbox/dts/overlay0.dtso
similarity index 100%
rename from arch/sandbox/dts/overlay0.dts
rename to arch/sandbox/dts/overlay0.dtso
diff --git a/arch/sandbox/dts/overlay1.dts b/arch/sandbox/dts/overlay1.dtso
similarity index 100%
rename from arch/sandbox/dts/overlay1.dts
rename to arch/sandbox/dts/overlay1.dtso
--
2.37.2
[PATCH 3/5] arm64: zynqmp: rename overlay sources to .dtso
Distinguish more clearly between source files meant for producing .dtb
from those meant for producing .dtbo. No functional change, as we
currently have rules for producing a foo.dtbo from either foo.dts or
foo.dtso.
Note that in the linux tree, all device tree overlay sources have been
renamed to .dtso, and the .dts->.dtbo rule is gone since v6.5 (commit
81d362732bac). So this is also a step towards staying closer to linux
with respect to both Kbuild and device tree sources.
Signed-off-by: Rasmus Villemoes
---
.../dts/{zynqmp-sck-kr-g-revA.dts => zynqmp-sck-kr-g-revA.dtso} | 0
.../dts/{zynqmp-sck-kr-g-revB.dts => zynqmp-sck-kr-g-revB.dtso} | 0
.../dts/{zynqmp-sck-kv-g-revA.dts => zynqmp-sck-kv-g-revA.dtso} | 0
.../dts/{zynqmp-sck-kv-g-revB.dts => zynqmp-sck-kv-g-revB.dtso} | 0
4 files changed, 0 insertions(+), 0 deletions(-)
rename arch/arm/dts/{zynqmp-sck-kr-g-revA.dts => zynqmp-sck-kr-g-revA.dtso}
(100%)
rename arch/arm/dts/{zynqmp-sck-kr-g-revB.dts => zynqmp-sck-kr-g-revB.dtso}
(100%)
rename arch/arm/dts/{zynqmp-sck-kv-g-revA.dts => zynqmp-sck-kv-g-revA.dtso}
(100%)
rename arch/arm/dts/{zynqmp-sck-kv-g-revB.dts => zynqmp-sck-kv-g-revB.dtso}
(100%)
diff --git a/arch/arm/dts/zynqmp-sck-kr-g-revA.dts
b/arch/arm/dts/zynqmp-sck-kr-g-revA.dtso
similarity index 100%
rename from arch/arm/dts/zynqmp-sck-kr-g-revA.dts
rename to arch/arm/dts/zynqmp-sck-kr-g-revA.dtso
diff --git a/arch/arm/dts/zynqmp-sck-kr-g-revB.dts
b/arch/arm/dts/zynqmp-sck-kr-g-revB.dtso
similarity index 100%
rename from arch/arm/dts/zynqmp-sck-kr-g-revB.dts
rename to arch/arm/dts/zynqmp-sck-kr-g-revB.dtso
diff --git a/arch/arm/dts/zynqmp-sck-kv-g-revA.dts
b/arch/arm/dts/zynqmp-sck-kv-g-revA.dtso
similarity index 100%
rename from arch/arm/dts/zynqmp-sck-kv-g-revA.dts
rename to arch/arm/dts/zynqmp-sck-kv-g-revA.dtso
diff --git a/arch/arm/dts/zynqmp-sck-kv-g-revB.dts
b/arch/arm/dts/zynqmp-sck-kv-g-revB.dtso
similarity index 100%
rename from arch/arm/dts/zynqmp-sck-kv-g-revB.dts
rename to arch/arm/dts/zynqmp-sck-kv-g-revB.dtso
--
2.37.2
[PATCH 1/5] arm: dts: imx8mm-cl-iot-gate: rename overlay sources to .dtso
Distinguish more clearly between source files meant for producing .dtb
from those meant for producing .dtbo. No functional change, as we
currently have rules for producing a foo.dtbo from either foo.dts or
foo.dtso.
Note that in the linux tree, all device tree overlay sources have been
renamed to .dtso, and the .dts->.dtbo rule is gone since v6.5 (commit
81d362732bac). So this is also a step towards staying closer to linux
with respect to both Kbuild and device tree sources.
Signed-off-by: Rasmus Villemoes
---
...-cl-iot-gate-ied-adc0.dts => imx8mm-cl-iot-gate-ied-adc0.dtso} | 0
...-cl-iot-gate-ied-adc1.dts => imx8mm-cl-iot-gate-ied-adc1.dtso} | 0
...-cl-iot-gate-ied-can0.dts => imx8mm-cl-iot-gate-ied-can0.dtso} | 0
...-cl-iot-gate-ied-can1.dts => imx8mm-cl-iot-gate-ied-can1.dtso} | 0
...-cl-iot-gate-ied-tpm0.dts => imx8mm-cl-iot-gate-ied-tpm0.dtso} | 0
...-cl-iot-gate-ied-tpm1.dts => imx8mm-cl-iot-gate-ied-tpm1.dtso} | 0
.../{imx8mm-cl-iot-gate-ied.dts => imx8mm-cl-iot-gate-ied.dtso} | 0
7 files changed, 0 insertions(+), 0 deletions(-)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-adc0.dts =>
imx8mm-cl-iot-gate-ied-adc0.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-adc1.dts =>
imx8mm-cl-iot-gate-ied-adc1.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-can0.dts =>
imx8mm-cl-iot-gate-ied-can0.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-can1.dts =>
imx8mm-cl-iot-gate-ied-can1.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-tpm0.dts =>
imx8mm-cl-iot-gate-ied-tpm0.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-tpm1.dts =>
imx8mm-cl-iot-gate-ied-tpm1.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied.dts =>
imx8mm-cl-iot-gate-ied.dtso} (100%)
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied-adc0.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied-adc0.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied-adc0.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied-adc0.dtso
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied-adc1.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied-adc1.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied-adc1.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied-adc1.dtso
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied-can0.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied-can0.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied-can0.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied-can0.dtso
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied-can1.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied-can1.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied-can1.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied-can1.dtso
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dtso
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm1.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm1.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm1.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm1.dtso
diff --git a/arch/arm/dts/imx8mm-cl-iot-gate-ied.dts
b/arch/arm/dts/imx8mm-cl-iot-gate-ied.dtso
similarity index 100%
rename from arch/arm/dts/imx8mm-cl-iot-gate-ied.dts
rename to arch/arm/dts/imx8mm-cl-iot-gate-ied.dtso
--
2.37.2
[PATCH 2/5] iot2050: rename overlay sources to .dtso
Distinguish more clearly between source files meant for producing .dtb
from those meant for producing .dtbo. No functional change, as we
currently have rules for producing a foo.dtbo from either foo.dts or
foo.dtso.
Note that in the linux tree, all device tree overlay sources have been
renamed to .dtso, and the .dts->.dtbo rule is gone since v6.5 (commit
81d362732bac). So this is also a step towards staying closer to linux
with respect to both Kbuild and device tree sources.
Signed-off-by: Rasmus Villemoes
---
... => k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dtso} | 0
...y.dts => k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dtso} | 0
2 files changed, 0 insertions(+), 0 deletions(-)
rename arch/arm/dts/{k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dts
=> k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dtso} (100%)
rename arch/arm/dts/{k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dts =>
k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dtso} (100%)
diff --git
a/arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dts
b/arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dtso
similarity index 100%
rename from
arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dts
rename to arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dtso
diff --git a/arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dts
b/arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dtso
similarity index 100%
rename from arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dts
rename to arch/arm/dts/k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dtso
--
2.37.2
[PATCH 0/5] use .dtso for device tree overlay sources
All device tree overlay sources in the kernel tree have been renamed
to .dtso and, since v6.5 (commit 81d362732bac), the .dts -> .dtbo
rules is gone from linux' Kbuild.
Let's follow the lead of linux and do the renaming, and sooner rather
than later to reduce the amount of churn.
This is not quite complete as there are some files under test/ that
still need converting, but as they currently rely on being compiled to
.dtb (in order to, in turn, be applicable for the .dtb -> .dtb.S ->
.dtb.o rules), that's a bit more involved than simple renaming.
Rasmus Villemoes (5):
arm: dts: imx8mm-cl-iot-gate: rename overlay sources to .dtso
iot2050: rename overlay sources to .dtso
arm64: zynqmp: rename overlay sources to .dtso
sandbox: rename overlay sources to .dtso
doc: use .dtso as extension for device tree overlay sources
...iot-gate-ied-adc0.dts => imx8mm-cl-iot-gate-ied-adc0.dtso} | 0
...iot-gate-ied-adc1.dts => imx8mm-cl-iot-gate-ied-adc1.dtso} | 0
...iot-gate-ied-can0.dts => imx8mm-cl-iot-gate-ied-can0.dtso} | 0
...iot-gate-ied-can1.dts => imx8mm-cl-iot-gate-ied-can1.dtso} | 0
...iot-gate-ied-tpm0.dts => imx8mm-cl-iot-gate-ied-tpm0.dtso} | 0
...iot-gate-ied-tpm1.dts => imx8mm-cl-iot-gate-ied-tpm1.dtso} | 0
...imx8mm-cl-iot-gate-ied.dts => imx8mm-cl-iot-gate-ied.dtso} | 0
...k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dtso} | 0
...s => k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dtso} | 0
.../{zynqmp-sck-kr-g-revA.dts => zynqmp-sck-kr-g-revA.dtso} | 0
.../{zynqmp-sck-kr-g-revB.dts => zynqmp-sck-kr-g-revB.dtso} | 0
.../{zynqmp-sck-kv-g-revA.dts => zynqmp-sck-kv-g-revA.dtso} | 0
.../{zynqmp-sck-kv-g-revB.dts => zynqmp-sck-kv-g-revB.dtso} | 0
arch/sandbox/dts/{overlay0.dts => overlay0.dtso} | 0
arch/sandbox/dts/{overlay1.dts => overlay1.dtso} | 0
doc/develop/uefi/uefi.rst | 4 ++--
doc/usage/fdt_overlays.rst| 4 ++--
17 files changed, 4 insertions(+), 4 deletions(-)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-adc0.dts =>
imx8mm-cl-iot-gate-ied-adc0.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-adc1.dts =>
imx8mm-cl-iot-gate-ied-adc1.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-can0.dts =>
imx8mm-cl-iot-gate-ied-can0.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-can1.dts =>
imx8mm-cl-iot-gate-ied-can1.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-tpm0.dts =>
imx8mm-cl-iot-gate-ied-tpm0.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied-tpm1.dts =>
imx8mm-cl-iot-gate-ied-tpm1.dtso} (100%)
rename arch/arm/dts/{imx8mm-cl-iot-gate-ied.dts =>
imx8mm-cl-iot-gate-ied.dtso} (100%)
rename arch/arm/dts/{k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dts
=> k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie-overlay.dtso} (100%)
rename arch/arm/dts/{k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dts =>
k3-am6548-iot2050-advanced-m2-bkey-usb3-overlay.dtso} (100%)
rename arch/arm/dts/{zynqmp-sck-kr-g-revA.dts => zynqmp-sck-kr-g-revA.dtso}
(100%)
rename arch/arm/dts/{zynqmp-sck-kr-g-revB.dts => zynqmp-sck-kr-g-revB.dtso}
(100%)
rename arch/arm/dts/{zynqmp-sck-kv-g-revA.dts => zynqmp-sck-kv-g-revA.dtso}
(100%)
rename arch/arm/dts/{zynqmp-sck-kv-g-revB.dts => zynqmp-sck-kv-g-revB.dtso}
(100%)
rename arch/sandbox/dts/{overlay0.dts => overlay0.dtso} (100%)
rename arch/sandbox/dts/{overlay1.dts => overlay1.dtso} (100%)
--
2.37.2
Re: [PATCH 1/4] mkimage: also honour -B even without external data
On 21/09/2023 03.02, Simon Glass wrote:
> Hi Rasmus,
>
> On Tue, 19 Sept 2023 at 05:37, Rasmus Villemoes
> wrote:
>>
>> In some cases, using the "external data" feature is impossible or
>> undesirable, but one may still want (or need) the FIT image to have a
>> certain alignment. Also, given the current 'mkimage -h' output,
>>
>> -B => align size in hex for FIT structure and header
>>
>> it is quite unexpected for -B to be effectively ignored without -E.
>>
>> Signed-off-by: Rasmus Villemoes
>> ---
>> tools/fit_image.c | 40
>> 1 file changed, 40 insertions(+)
>
> Reviewed-by: Simon Glass
>
> Q below
>
>>
>> diff --git a/tools/fit_image.c b/tools/fit_image.c
>> index 9fe69ea0d9..2f5b25098a 100644
>> --- a/tools/fit_image.c
>> +++ b/tools/fit_image.c
>> @@ -712,6 +712,42 @@ err:
>> return ret;
>> }
>>
>> +/**
>> + * fit_align() - Ensure FIT image has certain alignment
>> + *
>> + * This takes a normal FIT file (with embedded data) and increases its
>> + * size so that it is a multiple of params->bl_len.
>> + */
>> +static int fit_align(struct image_tool_params *params, const char *fname)
>> +{
>> + int fit_size, new_size;
>> + int fd;
>> + struct stat sbuf;
>> + void *fdt;
>> + int ret = 0;
>> + int align_size;
>> +
>> + align_size = params->bl_len;
>> + fd = mmap_fdt(params->cmdname, fname, 0, , , false, false);
>> + if (fd < 0)
>> + return -EIO;
>> +
>> + fit_size = fdt_totalsize(fdt);
>> + new_size = ALIGN(fit_size, align_size);
>> + fdt_set_totalsize(fdt, new_size);
>
> Shouldn't this be fdt_open_into()?
I honestly just copy-pasted fit_extract_data() and shaved it down to the
part that does the "align the FDT part of the file".
I don't really understand your question. Are you saying this doesn't
work (or maybe doesn't work in some cases), or are you saying that
there's a simpler way to do this? For the latter, sure, one doesn't
really need to parse the whole FDT; we could just
open()
pread() length from FDT header, convert to cpu-endianness
length = ALIGN(length)
pwrite() the new length in fdt-endianness
ftruncate()
close()
but I thought it was better to stay closer to how fit_extract_data() was
done.
Rasmus
[PATCH v2] mx8m: csf.sh: pad csf blob for u-boot.itb to CSF_SIZE minus IVT header
When built with CONFIG_IMX_HAB, the full FIT image, including stuff
tacked on beyond the end of the fdt structure, is expected to be (fdt
size rounded up to 0x1000 boundary)+CONFIG_CSF_SIZE.
Now, when the FIT image is loaded from a storage device, it doesn't
really matter that the flash.bin that gets written to target isn't
quite that big - we will just load some garbage bytes that are never
read or used for anything. But when flash.bin is uploaded via uuu,
it's important that we actually serve at least as many bytes as the
target expects, or we will hang in rom_api_download_image().
Extend the logic in the csf.sh script so that the csf blob is padded
to CONFIG_CSF_SIZE minus the size of the IVT header.
Signed-off-by: Rasmus Villemoes
---
v2: use dd instead of truncate.
doc/imx/habv4/csf_examples/mx8m/csf.sh | 13 +
1 file changed, 13 insertions(+)
diff --git a/doc/imx/habv4/csf_examples/mx8m/csf.sh
b/doc/imx/habv4/csf_examples/mx8m/csf.sh
index 65c143073c..cd3b2614a2 100644
--- a/doc/imx/habv4/csf_examples/mx8m/csf.sh
+++ b/doc/imx/habv4/csf_examples/mx8m/csf.sh
@@ -75,5 +75,18 @@ dd if=ivt.bin of=flash.bin bs=1 seek=${ivt_block_offset}
conv=notrunc
# Generate CSF blob
cst -i csf_fit.tmp -o csf_fit.bin
+
+# When loading flash.bin via USB, we must ensure that the file being
+# served is as large as the target expects (see
+# board_spl_fit_size_align()), otherwise the target will hang in
+# rom_api_download_image() waiting for the remaining bytes.
+#
+# Note that in order for dd to actually extend the file, one must not
+# pass conv=notrunc here. With a non-zero seek= argument, dd is
+# documented to preserve the contents of the file seeked past; in
+# particular, dd does not open the file with O_TRUNC.
+CSF_SIZE=$(sed -n "/CONFIG_CSF_SIZE=/ s@.*=@@p" .config)
+dd if=/dev/null of=csf_fit.bin bs=1 seek=$((CSF_SIZE - 0x20)) count=0
+
# Patch CSF blob into flash.bin
dd if=csf_fit.bin of=flash.bin bs=1 seek=${csf_block_offset} conv=notrunc
--
2.37.2
Re: [PATCH] mx8m: csf.sh: pad csf blob for u-boot.itb to CSF_SIZE minus IVT header
On 19/09/2023 20.27, Marek Vasut wrote: > On 9/19/23 12:00, Rasmus Villemoes wrote: >> When built with CONFIG_IMX_HAB, the full FIT image, including stuff >> tacked on beyond the end of the fdt structure, is expected to be (fdt >> size rounded up to 0x1000 boundary)+CONFIG_CSF_SIZE. >> >> Now, when the FIT image is loaded from a storage device, it doesn't >> really matter that the flash.bin that gets written to target isn't >> quite that big - we will just load some garbage bytes that are never >> read or used for anything. But when flash.bin is uploaded via uuu, >> it's important that we actually serve at least as many bytes as the >> target expects, or we will hang in rom_api_download_image(). >> >> Extend the logic in the csf.sh script so that the csf blob is padded >> to CONFIG_CSF_SIZE minus the size of the IVT header. > > On which SoC do you trigger this stuff ? imx8mp > (or rather, which bootrom version, v1 or v2? they each use SDP or SDPS > respectively) Absolutely no idea. I just do "uuu flash.bin", which seems to DTRT automatically, I've never figured out the esoteric uuu language, and I've never found any actual documentation for it. It also doesn't work without https://lore.kernel.org/u-boot/20230919134932.134678-1-rasmus.villem...@prevas.dk/ due to the spl_load_simple_fit() abuse. >> + >> +# When loading flash.bin via USB, we must ensure that the file being >> +# served is as large as the target expects (see >> +# board_spl_fit_size_align()), otherwise the target will hang in >> +# rom_api_download_image() waiting for the remaining bytes. >> +CSF_SIZE=$(sed -n "/CONFIG_CSF_SIZE=/ s@.*=@@p" .config) >> +truncate -s $((CSF_SIZE - 0x20)) csf_fit.bin > > Can you use dd(1) instead ? I think dd(1) is more portable than > truncate(1) , at least I cannot find truncate(1) in opengroup specs. Certainly. Rasmus
[PATCH resend 2/2] imx: spl_imx_romapi.c: remove dead code
These IS_ENABLED(CONFIG_SPL_LOAD_FIT) cases can no longer be reached,
and thus get_fit_image_size() is also redundant.
Signed-off-by: Rasmus Villemoes
---
arch/arm/mach-imx/spl_imx_romapi.c | 25 ++---
1 file changed, 2 insertions(+), 23 deletions(-)
diff --git a/arch/arm/mach-imx/spl_imx_romapi.c
b/arch/arm/mach-imx/spl_imx_romapi.c
index b9dfb3d41d..c4a4185eed 100644
--- a/arch/arm/mach-imx/spl_imx_romapi.c
+++ b/arch/arm/mach-imx/spl_imx_romapi.c
@@ -184,23 +184,6 @@ static ulong spl_ram_load_read(struct spl_load_info *load,
ulong sector,
return count;
}
-static ulong get_fit_image_size(void *fit)
-{
- struct spl_image_info spl_image;
- struct spl_load_info spl_load_info;
- ulong last = (ulong)fit;
-
- memset(_load_info, 0, sizeof(spl_load_info));
- spl_load_info.bl_len = 1;
- spl_load_info.read = spl_ram_load_read;
- spl_load_info.priv =
-
- spl_load_simple_fit(_image, _load_info,
- (uintptr_t)fit, fit);
-
- return last - (ulong)fit;
-}
-
static u8 *search_fit_header(u8 *p, int size)
{
int i;
@@ -261,9 +244,7 @@ static int img_info_size(void *img_hdr)
static int img_total_size(void *img_hdr)
{
- if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
- return get_fit_image_size(img_hdr);
- } else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
+ if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
int total = get_container_size((ulong)img_hdr, NULL);
if (total < 0) {
@@ -386,9 +367,7 @@ static int spl_romapi_load_image_stream(struct
spl_image_info *spl_image,
load.bl_len = 1;
load.read = spl_ram_load_read;
- if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
- return spl_load_simple_fit(spl_image, , (ulong)phdr, phdr);
- else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
+ if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
return spl_load_imx_container(spl_image, , (ulong)phdr);
return -1;
--
2.37.2
[PATCH resend 0/2] imx: spl_imx_romapi: simplify stream loading
This is essentially a resend of something I sent four months ago [1]. The patches themselves are unchanged, but the commit log for 1/2 is updated to reflect the change wrt. IMX_HAB. Moreover, while they were previously "merely" a simplification, they are now required for making usb loading with IMX_HAB actually work. [1] https://lore.kernel.org/u-boot/20230523113651.292806-1-rasmus.villem...@prevas.dk/ Rasmus Villemoes (2): imx: spl_imx_romapi: avoid tricky use of spl_load_simple_fit() to get full FIT size imx: spl_imx_romapi.c: remove dead code arch/arm/mach-imx/spl_imx_romapi.c | 75 +- 1 file changed, 52 insertions(+), 23 deletions(-) -- 2.37.2
[PATCH resend 1/2] imx: spl_imx_romapi: avoid tricky use of spl_load_simple_fit() to get full FIT size
Currently, spl_imx_romapi uses a somewhat tricky workaround for the
fact that a FIT image with external data doesn't directly allow one to
know the full size of the file: It does a dummy spl_load_simple_fit(),
having the ->read callback remember the largest offset requested, and
then does a last call to rom_api_download_image() to fetch the
remaining part of the full FIT image.
We can avoid that by just keeping track of how much we have downloaded
already, and if the ->read() requests something outside the current
valid buffer, fetch up to the end of the current request.
The current method also suffers from not working when CONFIG_IMX_HAB
is enabled: While in that case u-boot.itb is not built with external
data, so the fdt header does contain the full size of the dtb
structure. However, it does not account for the extra CONFIG_CSF_SIZE
added by board_spl_fit_size_align(). And also, the data it hands out
during the first dummy spl_load_simple_fit() is of course garbage, and
wouldn't pass the verification.
So we really need to call spl_load_simple_fit() only once, let that
figure out just how big the FIT image is (including whatever data, CSF
or "ordinary" external data, has been tacked on beyond the fdt
structure), and always provide valid data from the ->read callback.
This only affects the CONFIG_SPL_LOAD_FIT case - I don't have any
hardware or experience with the CONFIG_SPL_LOAD_IMX_CONTAINER case, so
I leave that alone for now.
Signed-off-by: Rasmus Villemoes
---
arch/arm/mach-imx/spl_imx_romapi.c | 50 ++
1 file changed, 50 insertions(+)
diff --git a/arch/arm/mach-imx/spl_imx_romapi.c
b/arch/arm/mach-imx/spl_imx_romapi.c
index 4af4169967..b9dfb3d41d 100644
--- a/arch/arm/mach-imx/spl_imx_romapi.c
+++ b/arch/arm/mach-imx/spl_imx_romapi.c
@@ -133,6 +133,41 @@ err:
return -1;
}
+struct stream_state {
+ u8 *base;
+ u8 *end;
+ u32 pagesize;
+};
+
+static ulong spl_romapi_read_stream(struct spl_load_info *load, ulong sector,
+ ulong count, void *buf)
+{
+ struct stream_state *ss = load->priv;
+ u8 *end = (u8*)(sector + count);
+ u32 bytes;
+ int ret;
+
+ if (end > ss->end) {
+ bytes = end - ss->end;
+ bytes += ss->pagesize - 1;
+ bytes /= ss->pagesize;
+ bytes *= ss->pagesize;
+
+ debug("downloading another 0x%x bytes\n", bytes);
+ ret = rom_api_download_image(ss->end, 0, bytes);
+
+ if (ret != ROM_API_OKAY) {
+ printf("Failure download %d\n", bytes);
+ return 0;
+ }
+
+ ss->end = end;
+ }
+
+ memcpy(buf, (void *)(sector), count);
+ return count;
+}
+
static ulong spl_ram_load_read(struct spl_load_info *load, ulong sector,
ulong count, void *buf)
{
@@ -316,6 +351,21 @@ static int spl_romapi_load_image_stream(struct
spl_image_info *spl_image,
}
}
+ if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
+ struct stream_state ss;
+
+ ss.base = phdr;
+ ss.end = p;
+ ss.pagesize = pagesize;
+
+ memset(, 0, sizeof(load));
+ load.bl_len = 1;
+ load.read = spl_romapi_read_stream;
+ load.priv =
+
+ return spl_load_simple_fit(spl_image, , (ulong)phdr, phdr);
+ }
+
total = img_total_size(phdr);
total += 3;
total &= ~0x3;
--
2.37.2
[PATCH 3/4] tools: binman: add test case for fit, align without fit, external-offset
Signed-off-by: Rasmus Villemoes
---
tools/binman/ftest.py | 10 +
tools/binman/test/311_fit_align.dts | 58 +
2 files changed, 68 insertions(+)
create mode 100644 tools/binman/test/311_fit_align.dts
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index d26e7511f7..a3c465b3d3 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -7216,5 +7216,15 @@ fdt fdtmapExtract the devicetree
blob from the fdtmap
self.assertRegex(err,
"Image 'image'.*missing bintools.*: bootgen")
+def testFitAlign(self):
+"""Test a FIT image with a fit,align property"""
+data = self._DoReadFile('311_fit_align.dts')
+self.assertEqual(2048, len(data))
+
+dtb = fdt.Fdt.FromData(data)
+dtb.Scan()
+
+self.assertEqual(2048, dtb._fdt_obj.totalsize())
+
if __name__ == "__main__":
unittest.main()
diff --git a/tools/binman/test/311_fit_align.dts
b/tools/binman/test/311_fit_align.dts
new file mode 100644
index 00..4a9b95b8df
--- /dev/null
+++ b/tools/binman/test/311_fit_align.dts
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ binman {
+ fit {
+ description = "test desc";
+ #address-cells = <1>;
+ fit,align = <2048>;
+
+ images {
+ u-boot {
+ description = "test u-boot";
+ type = "standalone";
+ arch = "arm64";
+ os = "u-boot";
+ compression = "none";
+ load = <>;
+ entry = <>;
+
+ u-boot-nodtb {
+ };
+ };
+
+ fdt-1 {
+ description = "test fdt";
+ type = "flat_dt";
+ compression = "none";
+
+ u-boot-dtb {
+ };
+ };
+
+ fdt-2 {
+ description = "test fdt";
+ type = "flat_dt";
+ compression = "none";
+
+ u-boot-dtb {
+ };
+ };
+ };
+
+ configurations {
+ default = "config-1";
+ config-1 {
+ description = "test config";
+ fdt = "fdt-1";
+ firmware = "u-boot";
+ };
+ };
+ };
+ };
+};
--
2.37.2
[PATCH 4/4] binman: update documentation for fit,align property
Eliminate the repetition "what alignment to use ... and provides the alignment to use", and indicate that fit,align can both be used by itself and together with fit,external-offset. Signed-off-by: Rasmus Villemoes --- tools/binman/entries.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index e7dfe6b2a3..f9ad27ce8c 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -691,9 +691,8 @@ The top-level 'fit' node supports the following special properties: external offset. This is passed to mkimage via the -E and -p flags. fit,align -Indicates what alignment to use for the FIT and its external data, -and provides the alignment to use. This is passed to mkimage via -the -B flag. +Indicates what alignment to use for the FIT and, if applicable, +its external data. This is passed to mkimage via the -B flag. fit,fdt-list Indicates the entry argument which provides the list of device tree -- 2.37.2
[PATCH 2/4] binman: test: rename 275_fit_align.dts -> 275_fit_align_external.dts
In preparation for adding a test case for fit,align without
fit,external-offset present, rename the existing test case and
corresponding file.
Signed-off-by: Rasmus Villemoes
---
tools/binman/ftest.py | 4 ++--
.../test/{275_fit_align.dts => 275_fit_align_external.dts}| 0
2 files changed, 2 insertions(+), 2 deletions(-)
rename tools/binman/test/{275_fit_align.dts => 275_fit_align_external.dts}
(100%)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 1293e9dbf4..d26e7511f7 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6522,9 +6522,9 @@ fdt fdtmapExtract the devicetree
blob from the fdtmap
self.assertEqual(base + 8, inset.image_pos);
self.assertEqual(4, inset.size);
-def testFitAlign(self):
+def testFitAlignExternal(self):
"""Test an image with an FIT with aligned external data"""
-data = self._DoReadFile('275_fit_align.dts')
+data = self._DoReadFile('275_fit_align_external.dts')
self.assertEqual(4096, len(data))
dtb = fdt.Fdt.FromData(data)
diff --git a/tools/binman/test/275_fit_align.dts
b/tools/binman/test/275_fit_align_external.dts
similarity index 100%
rename from tools/binman/test/275_fit_align.dts
rename to tools/binman/test/275_fit_align_external.dts
--
2.37.2
[PATCH 1/4] mkimage: also honour -B even without external data
In some cases, using the "external data" feature is impossible or
undesirable, but one may still want (or need) the FIT image to have a
certain alignment. Also, given the current 'mkimage -h' output,
-B => align size in hex for FIT structure and header
it is quite unexpected for -B to be effectively ignored without -E.
Signed-off-by: Rasmus Villemoes
---
tools/fit_image.c | 40
1 file changed, 40 insertions(+)
diff --git a/tools/fit_image.c b/tools/fit_image.c
index 9fe69ea0d9..2f5b25098a 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -712,6 +712,42 @@ err:
return ret;
}
+/**
+ * fit_align() - Ensure FIT image has certain alignment
+ *
+ * This takes a normal FIT file (with embedded data) and increases its
+ * size so that it is a multiple of params->bl_len.
+ */
+static int fit_align(struct image_tool_params *params, const char *fname)
+{
+ int fit_size, new_size;
+ int fd;
+ struct stat sbuf;
+ void *fdt;
+ int ret = 0;
+ int align_size;
+
+ align_size = params->bl_len;
+ fd = mmap_fdt(params->cmdname, fname, 0, , , false, false);
+ if (fd < 0)
+ return -EIO;
+
+ fit_size = fdt_totalsize(fdt);
+ new_size = ALIGN(fit_size, align_size);
+ fdt_set_totalsize(fdt, new_size);
+ debug("Size extended from from %x to %x\n", fit_size, new_size);
+ munmap(fdt, sbuf.st_size);
+
+ if (ftruncate(fd, new_size)) {
+ debug("%s: Failed to truncate file: %s\n", __func__,
+ strerror(errno));
+ ret = -EIO;
+ }
+
+ close(fd);
+ return ret;
+}
+
/**
* fit_handle_file - main FIT file processing function
*
@@ -817,6 +853,10 @@ static int fit_handle_file(struct image_tool_params
*params)
ret = fit_extract_data(params, tmpfile);
if (ret)
goto err_system;
+ } else if (params->bl_len) {
+ ret = fit_align(params, tmpfile);
+ if (ret)
+ goto err_system;
}
if (rename (tmpfile, params->imagefile) == -1) {
--
2.37.2
[PATCH 0/4] mkimage: also honour -B even without external data
I was surprised to discover that mkimage effectively ignores -B when
used by itself - the help text suggests otherwise, and it's a
completely reasonable thing to have.
Also, binman already translates a fit,align property into a -B
argument, without requiring fit,external-offset to be set.
Rasmus Villemoes (4):
mkimage: also honour -B even without external data
binman: test: rename 275_fit_align.dts -> 275_fit_align_external.dts
tools: binman: add test case for fit,align without fit,external-offset
binman: update documentation for fit,align property
tools/binman/entries.rst | 5 +-
tools/binman/ftest.py | 14 -
...t_align.dts => 275_fit_align_external.dts} | 0
tools/binman/test/311_fit_align.dts | 58 +++
tools/fit_image.c | 40 +
5 files changed, 112 insertions(+), 5 deletions(-)
rename tools/binman/test/{275_fit_align.dts => 275_fit_align_external.dts}
(100%)
create mode 100644 tools/binman/test/311_fit_align.dts
--
2.37.2
[PATCH] mx8m: csf.sh: pad csf blob for u-boot.itb to CSF_SIZE minus IVT header
When built with CONFIG_IMX_HAB, the full FIT image, including stuff
tacked on beyond the end of the fdt structure, is expected to be (fdt
size rounded up to 0x1000 boundary)+CONFIG_CSF_SIZE.
Now, when the FIT image is loaded from a storage device, it doesn't
really matter that the flash.bin that gets written to target isn't
quite that big - we will just load some garbage bytes that are never
read or used for anything. But when flash.bin is uploaded via uuu,
it's important that we actually serve at least as many bytes as the
target expects, or we will hang in rom_api_download_image().
Extend the logic in the csf.sh script so that the csf blob is padded
to CONFIG_CSF_SIZE minus the size of the IVT header.
Signed-off-by: Rasmus Villemoes
---
doc/imx/habv4/csf_examples/mx8m/csf.sh | 8
1 file changed, 8 insertions(+)
diff --git a/doc/imx/habv4/csf_examples/mx8m/csf.sh
b/doc/imx/habv4/csf_examples/mx8m/csf.sh
index 65c143073c..80edc94aeb 100644
--- a/doc/imx/habv4/csf_examples/mx8m/csf.sh
+++ b/doc/imx/habv4/csf_examples/mx8m/csf.sh
@@ -75,5 +75,13 @@ dd if=ivt.bin of=flash.bin bs=1 seek=${ivt_block_offset}
conv=notrunc
# Generate CSF blob
cst -i csf_fit.tmp -o csf_fit.bin
+
+# When loading flash.bin via USB, we must ensure that the file being
+# served is as large as the target expects (see
+# board_spl_fit_size_align()), otherwise the target will hang in
+# rom_api_download_image() waiting for the remaining bytes.
+CSF_SIZE=$(sed -n "/CONFIG_CSF_SIZE=/ s@.*=@@p" .config)
+truncate -s $((CSF_SIZE - 0x20)) csf_fit.bin
+
# Patch CSF blob into flash.bin
dd if=csf_fit.bin of=flash.bin bs=1 seek=${csf_block_offset} conv=notrunc
--
2.37.2
[PATCH] imx8mp: binman: rename spl and u-boot nodes
The hab signing script doc/imx/habv4/csf_examples/mx8m/csf.sh does
fdtget -t x u-boot.dtb /binman/imx-boot/uboot offset
to figure out the offset of u-boot.itb inside flash.bin. That works
fine for imx8mm, imx8mn, imx8mq, but fails for imx8mp because in that
case 'uboot' is merely a label and not actually the node name.
Homogenize these cases and make imx8mp the same as the other imx8m*
variants. The binman type is explicitly given and no longer derived
from the node name, and the csf.sh script will work for all four SOCs.
Signed-off-by: Rasmus Villemoes
---
arch/arm/dts/imx8mp-u-boot.dtsi | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
index 200938a980..355af16cf3 100644
--- a/arch/arm/dts/imx8mp-u-boot.dtsi
+++ b/arch/arm/dts/imx8mp-u-boot.dtsi
@@ -171,14 +171,16 @@
filename = "flash.bin";
pad-byte = <0x00>;
- spl: blob-ext@1 {
+ spl {
filename = "spl.bin";
offset = <0x0>;
+ type = "blob-ext";
};
- uboot: blob-ext@2 {
+ binman_uboot: uboot {
filename = "u-boot.itb";
offset = <0x58000>;
+ type = "blob-ext";
};
};
};
--
2.37.2
[PATCH 2/2] malloc_simple: add mem_malloc_init_simple()
I was running out of malloc() in SPL, and the message told me to look
at CONFIG_SYS_SPL_MALLOC_SIZE. So I did, and bumped it quite a bit,
but that had no effect whatsoever.
The reason for that was that I also have
CONFIG_SPL_SYS_MALLOC_SIMPLE=y. So while board_init_r() in spl.c duly
calls
mem_malloc_init(SYS_SPL_MALLOC_START, CONFIG_SYS_SPL_MALLOC_SIZE);
that doesn't actually do anything, because that function just sets
some variables in dlmalloc.c, and (as the linker map shows), the rest
of dlmalloc.o has been garbage-collected.
I don't want to have the full dlmalloc implementation in SPL - code
size is still precious. However, once SDRAM is initialized, the heap
is practically infinite, if only CONFIG_SYS_SPL_MALLOC_SIZE actually
had an effect.
So just as CONFIG_SPL_SYS_MALLOC_SIMPLE redirects malloc() and friends
at build-time to the _simple variants, add a _simple variant of
mem_malloc_init() which will actually update the bookkeeping variables
relevant to the actual and active malloc() implementation.
Signed-off-by: Rasmus Villemoes
---
common/dlmalloc.c | 2 +-
common/malloc_simple.c | 7 +++
include/malloc.h | 7 +--
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/common/dlmalloc.c b/common/dlmalloc.c
index dcecdb8623..d42b26410f 100644
--- a/common/dlmalloc.c
+++ b/common/dlmalloc.c
@@ -619,7 +619,7 @@ void *sbrk(ptrdiff_t increment)
return (void *)old;
}
-void mem_malloc_init(ulong start, ulong size)
+void mem_dlmalloc_init(ulong start, ulong size)
{
mem_malloc_start = start;
mem_malloc_end = start + size;
diff --git a/common/malloc_simple.c b/common/malloc_simple.c
index 0a004d40e1..9ecf05cf2e 100644
--- a/common/malloc_simple.c
+++ b/common/malloc_simple.c
@@ -17,6 +17,13 @@
DECLARE_GLOBAL_DATA_PTR;
+void mem_malloc_init_simple(ulong start, ulong size)
+{
+ gd->malloc_base = start;
+ gd->malloc_ptr = 0;
+ gd->malloc_limit = size;
+}
+
static void *alloc_simple(size_t bytes, int align)
{
ulong addr, new_ptr;
diff --git a/include/malloc.h b/include/malloc.h
index 161ccbd129..f59942115b 100644
--- a/include/malloc.h
+++ b/include/malloc.h
@@ -899,6 +899,7 @@ void malloc_disable_testing(void);
#define malloc malloc_simple
#define realloc realloc_simple
#define memalign memalign_simple
+#define mem_malloc_init mem_malloc_init_simple
#if IS_ENABLED(CONFIG_VALGRIND)
#define free free_simple
#else
@@ -908,6 +909,9 @@ void *calloc(size_t nmemb, size_t size);
void *realloc_simple(void *ptr, size_t size);
#else
+#define mem_malloc_init mem_dlmalloc_init
+void mem_dlmalloc_init(ulong start, ulong size);
+
# ifdef USE_DL_PREFIX
# define cALLOcdlcalloc
# define fREe dlfree
@@ -955,6 +959,7 @@ int initf_malloc(void);
/* Simple versions which can be used when space is tight */
void *malloc_simple(size_t size);
void *memalign_simple(size_t alignment, size_t bytes);
+void mem_malloc_init_simple(ulong start, ulong size);
#pragma GCC visibility push(hidden)
# if __STD_C
@@ -997,8 +1002,6 @@ extern ulong mem_malloc_start;
extern ulong mem_malloc_end;
extern ulong mem_malloc_brk;
-void mem_malloc_init(ulong start, ulong size);
-
#ifdef __cplusplus
}; /* end of extern "C" */
#endif
--
2.37.2
[PATCH 0/2] make CONFIG_SPL_SYS_MALLOC_SIMPLE && CONFIG_SYS_SPL_MALLOC actually work
Currently, setting both CONFIG_SPL_SYS_MALLOC_SIMPLE and CONFIG_SYS_SPL_MALLOC (but not CONFIG_SPL_STACK_R) doesn't work as expected: The SIMPLE option means that all malloc etc. calls are directed at build-time to the implementation in malloc_simple.c, but the mem_alloc_init() call which is done in board_init_f() currently calls into dlmalloc.c, and updates the variables describing the dlmalloc arena - which is of course completely unused. And the malloc() implementation continues to hand out allocations from the initial SPL_SYS_MALLOC_F_LEN bytes. These two patches are an attempt at making the combination in $subject actually work as one would expect: define an area of sdram to be used as a malloc arena, but still manage that using the malloc_simple() implementation. Since this now changes the mem_alloc_init() from being a harmless no-op call to actually doing something, there's a risk of some boards breaking. The solution for those boards will probably be to just drop CONFIG_SYS_SPL_MALLOC, since that hasn't actually done anything. Rasmus Villemoes (2): spl: make SYS_SPL_MALLOC depend on !(SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE) malloc_simple: add mem_malloc_init_simple() common/dlmalloc.c | 2 +- common/malloc_simple.c | 7 +++ common/spl/Kconfig | 1 + include/malloc.h | 7 +-- 4 files changed, 14 insertions(+), 3 deletions(-) -- 2.37.2
[PATCH 1/2] spl: make SYS_SPL_MALLOC depend on !(SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE)
Currently, one can have both SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y. However, while the former does make board_init_r() in spl.c call mem_malloc_init(), that has no effect at all, because that just updates a few bookkeeping variables, but as the linker map shows, the latter setting has (as expected) caused most of dlmalloc.o to be garbage-collected. That is, those bookkeeping variables are not used for anything. IOWs, with SYS_SPL_MALLOC=y and SPL_SYS_MALLOC_SIMPLE=y, the value of CONFIG_SYS_SPL_MALLOC_SIZE is irrelevant, and one still only has the small, SRAM-backed, malloc arena available. Now I want to change that so that the mem_malloc_init() instead updates the gd->malloc* variables to point at the SDRAM area. However, there's a small complication, namely when SPL_STACK_R=y is also in the mix. In that case, the "simple" malloc arena is indeed updated to point at the SDRAM area carved out of the new stack (see spl_relocate_stack_gd()). So that case works in the sense that one does get a "large" "simple" malloc arena (of size SPL_STACK_R_MALLOC_SIMPLE_LEN) - but CONFIG_SYS_SPL_MALLOC_SIZE is still irrelevant. Once I change the mem_malloc_init() logic, this would then break, because the gd->malloc* variables would be updated again. Also, it doesn't really make sense to allow the .config to essentially specify two different SDRAM-backed malloc arenas. So since CONFIG_SYS_SPL_MALLOC and its dependent options are no-ops currently when SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE, simply forbid that combination. Simple grepping suggests that these boards/configs are affected, in that they become a tiny bit smaller, and the defconfig will need refreshing: am62ax_evm_r5_defconfig am62x_evm_r5_defconfig am64x_evm_a53_defconfig am64x_evm_r5_defconfig am65x_evm_a53_defconfig am65x_hs_evm_a53_defconfig iot2050_defconfig j7200_evm_a72_defconfig j721e_evm_a72_defconfig j721s2_evm_a72_defconfig j721s2_evm_r5_defconfig verdin-am62_r5_defconfig Signed-off-by: Rasmus Villemoes --- common/spl/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/common/spl/Kconfig b/common/spl/Kconfig index c5dd476db5..fca9ada337 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -405,6 +405,7 @@ config SPL_SEPARATE_BSS config SYS_SPL_MALLOC bool "Enable malloc pool in SPL" depends on SPL_FRAMEWORK + depends on !(SPL_STACK_R && SPL_SYS_MALLOC_SIMPLE) config HAS_CUSTOM_SPL_MALLOC_START bool "For the SPL malloc pool, define a custom starting address" -- 2.37.2
Re: USB: error messages on DWC3 gadget endpoint dequeue
On 15/09/2023 15.05, João Paulo Silva Gonçalves wrote:
> Hi Marek,
>
> I was testing fastboot image download over usb for imx8mp (from usb
> recovery patch of verdin-imx8mp) and i am having error messages on
> endpoint request dequeue function of DWC3 gadget controller. However,
> download is working fine, so this message may not be an error. They are
> happening because fastboot tx before sending a new usb request dequeue
> the same request, maybe to be sure it does not send it twice. Can I
> just ignore these messages? Maybe change its log level to dbg instead
> of error? What do you think? The messages I am seeing are below and are
> the ones with "... was not queued to ep1in-bulk".
We apply this internally (sorry if it's whitespace damaged), but I never
fully understood the problem nor how the referenced kernel thread was
resolved, which is why I haven't sent it upstream yet.
dwc3: gadget: Handle dequeuing of non queued request gracefully
Trying to dequeue an request that is currently not queued should be
a no-op
and be handled gracefully.
Checking on list/queue empty indicate whether the request is queue
or not.
Handling this gracefully allows for race condition free synchronization
between the complete callback being called to to a completed
transfer and
trying to call usb_ep_dequeue() at the same time.
Inspired by:
https://patchwork.kernel.org/project/linux-usb/patch/20191106144553.16956-1-alexandru.ardel...@analog.com/
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index eb416b832aa..378d19d8e99 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1113,6 +1113,9 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
spin_lock_irqsave(>lock, flags);
+ if (list_empty(>request_list) && list_empty(>req_queued))
+ goto out0;
+
list_for_each_entry(r, >request_list, list) {
if (r == req)
break;
Rasmus
Re: [PATCH 1/2] Revert "lib: string: Fix strlcpy return value", fix callers
On 14/07/2023 13.24, Matthias Schiffer wrote: > Both the Linux kernel and libbsd agree that strlcpy() should always > return strlen(src) and not include the NUL termination. The incorrect > U-Boot implementation makes it impossible to check the return value for > truncation, and breaks code written with the usual implementation in > mind (for example, fdtdec_add_reserved_memory() was subtly broken). So while we're fixing non-standard string function behaviour, can we _please_ finally fix strncpy() so it follows C/POSIX? https://lore.kernel.org/u-boot/52bc92d4-8651-48df-3577-043aa2e16...@prevas.dk/ Note in particular the very last paragraph. To rephrase and give a concrete example, what I'm worried about is us importing some file system code that (correctly) uses strncpy() to fully initialize some memory buffer, then writes that out to disk - but with our crippled strncpy(), the result is potential garbage on-disk, which both our own read implementation (which is likely also just copied) and the kernel's may subsequently choke on. Correctness first, please. If there are any performance problems, those should be identified individually and perhaps rewritten to not use strncpy() after verifying that not zeroing the tail is ok for that call site. Rasmus
[PATCH 1/2] imx: spl_imx_romapi: avoid tricky use of spl_load_simple_fit() to get full FIT size
Currently, spl_imx_romapi uses a somewhat tricky workaround for the
fact that a FIT image with external data doesn't directly allow one to
know the full size of the file: It does a dummy spl_load_simple_fit(),
having the ->read callback remember the largest offset requested, and
then does a last call to rom_api_download_image() to fetch the
remaining part of the full FIT image.
We can avoid that by just keeping track of how much we have downloaded
already, and if the ->read() requests something outside the current
valid buffer, fetch up to the end of the current request.
The current method also suffers from not working when CONFIG_IMX_HAB
is enabled, because the first call of spl_load_simple_fit() will have
written garbage to the various locations, so the
board_spl_fit_post_load() call panics. The downstream NXP kernel
carries a workaround for this, but that workaround should not be
necessary with this, since we only ever memcpy() valid image data.
This only affects the CONFIG_SPL_LOAD_FIT case - I don't have any
hardware or experience with the CONFIG_SPL_LOAD_IMX_CONTAINER case, so
I leave that alone for now.
Signed-off-by: Rasmus Villemoes
---
arch/arm/mach-imx/spl_imx_romapi.c | 50 ++
1 file changed, 50 insertions(+)
diff --git a/arch/arm/mach-imx/spl_imx_romapi.c
b/arch/arm/mach-imx/spl_imx_romapi.c
index 830d5d12c2..e4e2297ac0 100644
--- a/arch/arm/mach-imx/spl_imx_romapi.c
+++ b/arch/arm/mach-imx/spl_imx_romapi.c
@@ -126,6 +126,41 @@ static int spl_romapi_load_image_seekable(struct
spl_image_info *spl_image,
return 0;
}
+struct stream_state {
+ u8 *base;
+ u8 *end;
+ u32 pagesize;
+};
+
+static ulong spl_romapi_read_stream(struct spl_load_info *load, ulong sector,
+ ulong count, void *buf)
+{
+ struct stream_state *ss = load->priv;
+ u8 *end = (u8*)(sector + count);
+ u32 bytes;
+ int ret;
+
+ if (end > ss->end) {
+ bytes = end - ss->end;
+ bytes += ss->pagesize - 1;
+ bytes /= ss->pagesize;
+ bytes *= ss->pagesize;
+
+ debug("downloading another 0x%x bytes\n", bytes);
+ ret = rom_api_download_image(ss->end, 0, bytes);
+
+ if (ret != ROM_API_OKAY) {
+ printf("Failure download %d\n", bytes);
+ return 0;
+ }
+
+ ss->end = end;
+ }
+
+ memcpy(buf, (void *)(sector), count);
+ return count;
+}
+
static ulong spl_ram_load_read(struct spl_load_info *load, ulong sector,
ulong count, void *buf)
{
@@ -309,6 +344,21 @@ static int spl_romapi_load_image_stream(struct
spl_image_info *spl_image,
}
}
+ if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
+ struct stream_state ss;
+
+ ss.base = phdr;
+ ss.end = p;
+ ss.pagesize = pagesize;
+
+ memset(, 0, sizeof(load));
+ load.bl_len = 1;
+ load.read = spl_romapi_read_stream;
+ load.priv =
+
+ return spl_load_simple_fit(spl_image, , (ulong)phdr, phdr);
+ }
+
total = img_total_size(phdr);
total += 3;
total &= ~0x3;
--
2.37.2
[PATCH 2/2] imx: spl_imx_romapi.c: remove dead code
These IS_ENABLED(CONFIG_SPL_LOAD_FIT) cases can no longer be reached,
and thus get_fit_image_size() is also redundant.
Signed-off-by: Rasmus Villemoes
---
arch/arm/mach-imx/spl_imx_romapi.c | 25 ++---
1 file changed, 2 insertions(+), 23 deletions(-)
diff --git a/arch/arm/mach-imx/spl_imx_romapi.c
b/arch/arm/mach-imx/spl_imx_romapi.c
index e4e2297ac0..d34102990f 100644
--- a/arch/arm/mach-imx/spl_imx_romapi.c
+++ b/arch/arm/mach-imx/spl_imx_romapi.c
@@ -177,23 +177,6 @@ static ulong spl_ram_load_read(struct spl_load_info *load,
ulong sector,
return count;
}
-static ulong get_fit_image_size(void *fit)
-{
- struct spl_image_info spl_image;
- struct spl_load_info spl_load_info;
- ulong last = (ulong)fit;
-
- memset(_load_info, 0, sizeof(spl_load_info));
- spl_load_info.bl_len = 1;
- spl_load_info.read = spl_ram_load_read;
- spl_load_info.priv =
-
- spl_load_simple_fit(_image, _load_info,
- (uintptr_t)fit, fit);
-
- return last - (ulong)fit;
-}
-
static u8 *search_fit_header(u8 *p, int size)
{
int i;
@@ -254,9 +237,7 @@ static int img_info_size(void *img_hdr)
static int img_total_size(void *img_hdr)
{
- if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
- return get_fit_image_size(img_hdr);
- } else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
+ if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
int total = get_container_size((ulong)img_hdr, NULL);
if (total < 0) {
@@ -379,9 +360,7 @@ static int spl_romapi_load_image_stream(struct
spl_image_info *spl_image,
load.bl_len = 1;
load.read = spl_ram_load_read;
- if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
- return spl_load_simple_fit(spl_image, , (ulong)phdr, phdr);
- else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
+ if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
return spl_load_imx_container(spl_image, , (ulong)phdr);
return -1;
--
2.37.2
[PATCH] imx8m: soc.c: demote some printfs to debug
Getting
Found /vpu_g1@3830 node
Modify /vpu_g1@3830:status disabled
Found /vpu_g2@3831 node
Modify /vpu_g2@3831:status disabled
etc. on the console on every boot is needlessly verbose. Demote the
"Found ..." lines to debug(), which is consistent with other instances
in soc.c.
Signed-off-by: Rasmus Villemoes
---
arch/arm/mach-imx/imx8m/soc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 5a4f8358c9..f5c82dff35 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -736,7 +736,7 @@ static int disable_fdt_nodes(void *blob, const char *const
nodes_path[], int siz
if (nodeoff < 0)
continue; /* Not found, skip it */
- printf("Found %s node\n", nodes_path[i]);
+ debug("Found %s node\n", nodes_path[i]);
add_status:
rc = fdt_setprop(blob, nodeoff, "status", status,
strlen(status) + 1);
@@ -1265,7 +1265,7 @@ int ft_system_setup(void *blob, struct bd_info *bd)
if (nodeoff >= 0) {
const char *speed = "high-speed";
- printf("Found %s node\n", usb_dwc3_path[v]);
+ debug("Found %s node\n", usb_dwc3_path[v]);
usb_modify_speed:
--
2.37.2
Re: [PATCH 3/3] arm: dts: imx8mp: Sync with Linux 6.3
On 20/05/2023 00.26, Adam Ford wrote:
> On Fri, May 19, 2023 at 5:19 PM Tim Harvey wrote:
>>
>> On Wed, May 3, 2023 at 9:11 AM Tim Harvey wrote:
>>>
>> Fabio,
>>
>> Apparently I didn't do a very good job of testing this. This patch is
>> causing imx8mp-venice-* and imx8mp-evk boards to no longer boot with
>> no SPL banner. The specific change that causes breakage is the one
>> that encapsulates the spi/uart/flexcan children with
>> spba-bus@3080.
>
> The SPI, UART, and Flexcan are part of the spba-bus.
>
> We'll need to add the spba bus to imx8mp-u-boot.dtsi. Since it had no
> node name, it'll have to fall under aip3.
>
> Try this:
>
> diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
> index 18d1728e1d..0e6811b129 100644
> --- a/arch/arm/dts/imx8mp-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mp-u-boot.dtsi
> @@ -44,6 +44,10 @@
>
> {
> bootph-pre-ram;
> +
> + spba-bus@3080 {
> + bootph-pre-ram;
> + };
> };
>
> {
This begs the question: Why don't these tags just implicitly propagate
to parent nodes? It's a U-Boot specific tool (fdtgrep) that makes use of
them, no? So making the rule be "keep this node if it _or any
descendant_ has that tag" should be possible.
This has probably been answered somewhere before.
Rasmus
[PATCH] scripts/Makefile.lib: change spelling of $(srctree)/arch/$(ARCH)/dts in dtc_cpp_flags
Currently, all in-tree .dts files (apart from some under test/ and tools/), reside in arch/$ARCH/dts. However, in the linux kernel tree, dts files for arm64 boards, and probably in the not too distant future [1], arm boards as well, live in subdirectories of that. For private forks, using a vendor or project subdirectory is also more convenient to clearly separate private code from upstream - in the same way that code under board/ is also split and easy to maintain. In order to prepare for us to follow suit and do the splitting of the in-tree .dts files, and to make life a little easier for private forks that already place dts files not directly in arch/$ARCH/dts, change the $(srctree)/arch/$(ARCH)/dts path to instead refer to the directory of the .dts file being compiled. This should be a no-op for all existing cases. [1] https://lore.kernel.org/lkml/20220328000915.15041-1-ansuels...@gmail.com/ Signed-off-by: Rasmus Villemoes --- scripts/Makefile.lib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 7b27224b5d..b67da75ba1 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -186,7 +186,7 @@ u_boot_dtsi = $(strip $(u_boot_dtsi_options_debug) \ # Modified for U-Boot dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc\ $(UBOOTINCLUDE) \ --I$(srctree)/arch/$(ARCH)/dts \ +-I$(dir $<) \ -I$(srctree)/arch/$(ARCH)/dts/include \ -I$(srctree)/include\ -D__ASSEMBLY__ \ -- 2.37.2
