Re: [PATCH] boot: fdt: Turn all addresses and sizes into u64
On Sun, Apr 14, 2024 at 11:25:06PM +0200, Marek Vasut wrote: > On 4/14/24 9:29 PM, Laurent Pinchart wrote: > > Hi Marek, > > > > Thank you for the patch. > > > > On Sun, Apr 14, 2024 at 08:37:20PM +0200, Marek Vasut wrote: > >> In case of systems where DRAM bank ends at the edge of 32bit boundary, > >> start + size calculations would overflow. This happens on STM32MP15xx > >> with 1 DRAM bank starting at 0xc000 and 1 GiB of DRAM. This is a > >> usual 32bit system DRAM size overflow, fix it by doing all DRAM size > >> and offset calculations using u64 types. > > > > I'm not sure I like this much, as it removes a useful indication > > regarding what the variables store. > > That's what the variable name is for, not variable type. > > > Wouldn't it be better if the code's > > logic could be modified to avoid those overflows ? > > I'd prefer to keep the code simple and blanket avoid the overflows for a > very long time, rather than play whack-a-mole with various odd corner > cases here. Up to you. > Note that this is a fix for a previous series which changed from > u64/ulong to phys_addr/size_t , which clearly was incorrect . > > >> This also covers a case where > >> a 32bit PAE system might be able to address up to 36bits of DRAM. > > > > Shouldn't phys_addr_t be a 64-bit type on PAE systems ? > > That depends on CONFIG_PHYS_64BIT , on am57xx this is not set for > example, so there phys_addr_t is 32bit . The system won't be able to address more than 32 bits of memory in that case, would it ? -- Regards, Laurent Pinchart
Re: [PATCH] boot: fdt: Turn all addresses and sizes into u64
Hi Marek,
Thank you for the patch.
On Sun, Apr 14, 2024 at 08:37:20PM +0200, Marek Vasut wrote:
> In case of systems where DRAM bank ends at the edge of 32bit boundary,
> start + size calculations would overflow. This happens on STM32MP15xx
> with 1 DRAM bank starting at 0xc000 and 1 GiB of DRAM. This is a
> usual 32bit system DRAM size overflow, fix it by doing all DRAM size
> and offset calculations using u64 types.
I'm not sure I like this much, as it removes a useful indication
regarding what the variables store. Wouldn't it be better if the code's
logic could be modified to avoid those overflows ?
> This also covers a case where
> a 32bit PAE system might be able to address up to 36bits of DRAM.
Shouldn't phys_addr_t be a 64-bit type on PAE systems ?
> Fixes: a4df06e41fa2 ("boot: fdt: Change type of env_get_bootm_low() to
> phys_addr_t")
> Signed-off-by: Marek Vasut
> ---
> Cc: Laurent Pinchart
> Cc: Matthias Schiffer
> Cc: Simon Glass
> Cc: Tom Rini
> ---
> boot/image-fdt.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 2b92bdaff16..f09716cba30 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -158,13 +158,10 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void
> *fdt_blob)
> */
> int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size)
> {
> + u64 start, size, usable, addr, low, mapsize;
> void*fdt_blob = *of_flat_tree;
> void*of_start = NULL;
> - phys_addr_t start, size, usable;
> char*fdt_high;
> - phys_addr_t addr;
> - phys_addr_t low;
> - phys_size_t mapsize;
> ulong of_len = 0;
> int bank;
> int err;
--
Regards,
Laurent Pinchart
Re: [PATCH v2 0/3] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Drive-by fixes
Hi Florian, On Tue, Apr 02, 2024 at 01:18:35PM -0700, Florian Fainelli wrote: > On 4/2/24 13:08, Laurent Pinchart wrote: > > On Tue, Apr 02, 2024 at 09:52:06PM +0200, Stefan Wahren wrote: > >> Am 02.04.24 um 10:58 schrieb Ivan T. Ivanov: > >>> On 2024-03-28 01:37, Laurent Pinchart wrote: > >>>> On Wed, Mar 27, 2024 at 07:49:38AM +0100, Stefan Wahren wrote: > >>>>> Hi, > >>>>> > >>>>> [add Peter and Ivan] > >>>>> > >>>>> Am 26.03.24 um 20:58 schrieb Laurent Pinchart: > >>>>>> Hello, > >>>>>> > >>>>>> This small series includes a few drive-by fixes for DT validation > >>>>>> errors. > >>>>>> > >>>>>> The first patch has been posted previously in v1 ([1], and now > >>>>>> addresses > >>>>>> a small review comment. I think it's good to go. > >>>>>> > >>>>>> The next two patches address the same issue as "[PATCH 1/2] > >>>>>> dt-bindings: > >>>>>> arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties" ([2]), > >>>>>> but this time with a (hopefully) correct approach. Patch 2/3 starts by > >>>>>> fixing the raspberrypi-bcm2835-firmware driver, removing the need for > >>>>>> DT > >>>>>> properties that are specified in bcm2835-rpi.dtsi but not documented in > >>>>>> the corresponding bindings. Patch 3/3 can then drop those properties, > >>>>>> getting rid of the warnings. > >>>>> > >>>>> since this series drops properties from the device tree, does anyone > >>>>> have the chance to test it with a recent U-Boot? > >>>> > >>>> I don't have U-Boot running with my RPi, so I would appreciate if > >>>> someone could help :-) > >>> > >>> Sorry for taking me so long to verify this. > >>> > >>> I think on RPi U-Boot side we are fine. API used when accessing Mbox > >>> device do not follow DM model and do not use DMA, but just access > >>> device directly using this nice macros phys_to_bus/bus_to_phys. > >>> > >>> I build new DTB files with this patch included and U-Boot build > >>> from the latest sources. No obvious issues on RPi3 and RPi4. > >>> Devices boot fine. > > > > Thank you for testing Ivan. > > > >> Thanks you, Laurent and Ivan > >> > >> Reviewed-by: Stefan Wahren > > > > Stefan, I'm quite unfamiliar with the Raspberry Pi upstreaming process > > (despite having sent patches for ages :-)), do I understand correctly > > that this patch will go through your tree, or do I need to work with > > someone else to get it merged upstream ? > > I will be taking those via the Broadcom SoC tree. Thank you. If there's a chance to include patches 05/10, 07/10, 08/10 and 09/10 from [1] at the same time, that would be great :-) [1] https://lore.kernel.org/linux-media/20240402000424.4650-1-laurent.pinch...@ideasonboard.com -- Regards, Laurent Pinchart
Re: [PATCH v2 0/3] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Drive-by fixes
Hello, On Tue, Apr 02, 2024 at 09:52:06PM +0200, Stefan Wahren wrote: > Am 02.04.24 um 10:58 schrieb Ivan T. Ivanov: > > On 2024-03-28 01:37, Laurent Pinchart wrote: > >> On Wed, Mar 27, 2024 at 07:49:38AM +0100, Stefan Wahren wrote: > >>> Hi, > >>> > >>> [add Peter and Ivan] > >>> > >>> Am 26.03.24 um 20:58 schrieb Laurent Pinchart: > >>> > Hello, > >>> > > >>> > This small series includes a few drive-by fixes for DT validation > >>> > errors. > >>> > > >>> > The first patch has been posted previously in v1 ([1], and now addresses > >>> > a small review comment. I think it's good to go. > >>> > > >>> > The next two patches address the same issue as "[PATCH 1/2] dt-bindings: > >>> > arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties" ([2]), > >>> > but this time with a (hopefully) correct approach. Patch 2/3 starts by > >>> > fixing the raspberrypi-bcm2835-firmware driver, removing the need for DT > >>> > properties that are specified in bcm2835-rpi.dtsi but not documented in > >>> > the corresponding bindings. Patch 3/3 can then drop those properties, > >>> > getting rid of the warnings. > >>> > >>> since this series drops properties from the device tree, does anyone > >>> have the chance to test it with a recent U-Boot? > >> > >> I don't have U-Boot running with my RPi, so I would appreciate if > >> someone could help :-) > > > > Sorry for taking me so long to verify this. > > > > I think on RPi U-Boot side we are fine. API used when accessing Mbox > > device do not follow DM model and do not use DMA, but just access > > device directly using this nice macros phys_to_bus/bus_to_phys. > > > > I build new DTB files with this patch included and U-Boot build > > from the latest sources. No obvious issues on RPi3 and RPi4. > > Devices boot fine. Thank you for testing Ivan. > Thanks you, Laurent and Ivan > > Reviewed-by: Stefan Wahren Stefan, I'm quite unfamiliar with the Raspberry Pi upstreaming process (despite having sent patches for ages :-)), do I understand correctly that this patch will go through your tree, or do I need to work with someone else to get it merged upstream ? -- Regards, Laurent Pinchart
Re: [PATCH v2 0/3] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Drive-by fixes
On Wed, Mar 27, 2024 at 07:49:38AM +0100, Stefan Wahren wrote: > Hi, > > [add Peter and Ivan] > > Am 26.03.24 um 20:58 schrieb Laurent Pinchart: > > Hello, > > > > This small series includes a few drive-by fixes for DT validation > > errors. > > > > The first patch has been posted previously in v1 ([1], and now addresses > > a small review comment. I think it's good to go. > > > > The next two patches address the same issue as "[PATCH 1/2] dt-bindings: > > arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties" ([2]), > > but this time with a (hopefully) correct approach. Patch 2/3 starts by > > fixing the raspberrypi-bcm2835-firmware driver, removing the need for DT > > properties that are specified in bcm2835-rpi.dtsi but not documented in > > the corresponding bindings. Patch 3/3 can then drop those properties, > > getting rid of the warnings. > > since this series drops properties from the device tree, does anyone > have the chance to test it with a recent U-Boot? I don't have U-Boot running with my RPi, so I would appreciate if someone could help :-) > > [1] > > https://lore.kernel.org/linux-arm-kernel/20240326004902.17054-3-laurent.pinch...@ideasonboard.com/ > > [2] > > https://lore.kernel.org/linux-arm-kernel/20240326004902.17054-2-laurent.pinch...@ideasonboard.com/ > > > > Laurent Pinchart (3): > >dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add gpio child > > node > >firmware: raspberrypi: Use correct device for DMA mappings > >ARM: dts: bcm283x: Drop unneeded properties in the bcm2835-firmware > > node > > > > .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 30 +++ > > .../gpio/raspberrypi,firmware-gpio.txt| 30 --- > > arch/arm/boot/dts/broadcom/bcm2835-rpi.dtsi | 4 --- > > drivers/firmware/raspberrypi.c| 7 +++-- > > 4 files changed, 34 insertions(+), 37 deletions(-) > > delete mode 100644 > > Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt > > -- Regards, Laurent Pinchart
Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()
On Wed, Mar 20, 2024 at 09:52:34PM +0100, Marek Vasut wrote:
> On 3/18/24 5:18 PM, Laurent Pinchart wrote:
>
> >> @@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
> >>
> >>s = env_get("bootm_low");
> >>if (s)
> >> - tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
> >> + tmp = simple_strtoull(s, NULL, 16);
> >>else
> >>tmp = start;
> >>
> >
> > Maybe you could even drop the tmp variable completely by writing this
> >
> > if (s)
> > size -= simple_strtoull(s, NULL, 16) - start;
> >
> > return size;
> >
> > I've never liked variables named tmp :-)
>
> No, let's not do this. With this FDT part, the code should be verbose
> and as easy to understand at first glance as possible, no subtraction
> assignments and other shenanigans please.
How about this ?
s = env_get("bootm_low");
if (s) {
phys_addr_t low_addr = simple_strtoull(s, NULL, 16);
size -= low_addr - start;
}
return size;
If you're going for readability, that's clearer than
return size - (tmp - start);
and it also interestingly points out that tmp/low_addr should be a
phys_addr_t, not a phys_size_t.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 3/4] boot: fdt: Drop lmb_alloc*() typecasts
Hi Marek,
Thank you for the patch.
On Mon, Mar 18, 2024 at 04:00:46PM +0100, Marek Vasut wrote:
> The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts
> phys_addr_t as first parameter. Declare 'addr' as phys_addr_t and
> get rid of the casts.
>
> Reported-by: Laurent Pinchart
> Signed-off-by: Marek Vasut
Reviewed-by: Laurent Pinchart
> ---
> Cc: Heinrich Schuchardt
> Cc: Kuninori Morimoto
> Cc: Laurent Pinchart
> Cc: Simon Glass
> Cc: Tom Rini
> ---
> V2: - Replace $addr with 'addr' to somehow delimit the variable name
> ---
> boot/image-fdt.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index c2571b22244..c37442c9130 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> void*of_start = NULL;
> phys_addr_t start, size, usable;
> char*fdt_high;
> + phys_addr_t addr;
> phys_addr_t low;
> phys_size_t mapsize;
> ulong of_len = 0;
> @@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> fdt_high = env_get("fdt_high");
> if (fdt_high) {
> ulong desired_addr = hextoul(fdt_high, NULL);
> - ulong addr;
>
> if (desired_addr == ~0UL) {
> /* All ones means use fdt in place */
> @@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
>* At least part of this DRAM bank is usable, try
>* using it for LMB allocation.
>*/
> - of_start = map_sysmem((ulong)lmb_alloc_base(lmb,
> - of_len, 0x1000, usable), of_len);
> + addr = lmb_alloc_base(lmb, of_len, 0x1000, usable);
> + of_start = map_sysmem(addr, of_len);
> /* Allocation succeeded, use this block. */
> if (of_start != NULL)
> break;
--
Regards,
Laurent Pinchart
Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()
Hi Marek,
Thank you for the patch.
On Mon, Mar 18, 2024 at 04:00:45PM +0100, Marek Vasut wrote:
> Clean up tmp variable use and type cast in env_get_bootm_size().
> No functional change.
The code change looks fine, but you may want to expand the commit
message to explain why this patch improves the code.
> Signed-off-by: Marek Vasut
> ---
> Cc: Heinrich Schuchardt
> Cc: Kuninori Morimoto
> Cc: Laurent Pinchart
> Cc: Simon Glass
> Cc: Tom Rini
> ---
> V2: - New patch
> ---
> boot/image-board.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/boot/image-board.c b/boot/image-board.c
> index 3263497a1d5..e3d63745299 100644
> --- a/boot/image-board.c
> +++ b/boot/image-board.c
> @@ -129,10 +129,8 @@ phys_size_t env_get_bootm_size(void)
> phys_addr_t start;
> char *s = env_get("bootm_size");
>
> - if (s) {
> - tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
> - return tmp;
> - }
> + if (s)
> + return simple_strtoull(s, NULL, 16);
>
> start = gd->ram_base;
> size = gd->ram_size;
> @@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
>
> s = env_get("bootm_low");
> if (s)
> - tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
> + tmp = simple_strtoull(s, NULL, 16);
> else
> tmp = start;
>
Maybe you could even drop the tmp variable completely by writing this
if (s)
size -= simple_strtoull(s, NULL, 16) - start;
return size;
I've never liked variables named tmp :-)
--
Regards,
Laurent Pinchart
Re: [PATCH 3/3] boot: fdt: Move usable variable below updated comment
Hi Marek, Thank you for the patch. On Sun, Mar 17, 2024 at 07:16:31AM +0100, Marek Vasut wrote: > Move the variable below comment which explains what the variable means. > Update the comment. No functional change. > > Suggested-by: Laurent Pinchart > Signed-off-by: Marek Vasut > --- > Cc: Heinrich Schuchardt > Cc: Kuninori Morimoto > Cc: Laurent Pinchart > Cc: Simon Glass > Cc: Tom Rini > --- > boot/image-fdt.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/boot/image-fdt.c b/boot/image-fdt.c > index c37442c9130..16cd15e7e44 100644 > --- a/boot/image-fdt.c > +++ b/boot/image-fdt.c > @@ -218,12 +218,12 @@ int boot_relocate_fdt(struct lmb *lmb, char > **of_flat_tree, ulong *of_size) > if (start + size < low) > continue; > > - usable = min(start + size, low + mapsize); > - > /* >* At least part of this DRAM bank is usable, try > - * using it for LMB allocation. > + * using the the DRAM bank up to $usable address s/the the/the/ Is it a u-boot convention to write $variable to refer to a variable in comments ? Reviewed-by: Laurent Pinchart > + * limit for LMB allocation. >*/ > + usable = min(start + size, low + mapsize); > addr = lmb_alloc_base(lmb, of_len, 0x1000, usable); > of_start = map_sysmem(addr, of_len); > /* Allocation succeeded, use this block. */ -- Regards, Laurent Pinchart
Re: [PATCH 2/3] boot: fdt: Drop lmb_alloc*() typecasts
Hi Marek,
Thank you for the patch.
On Sun, Mar 17, 2024 at 07:16:30AM +0100, Marek Vasut wrote:
> The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts
> phys_addr_t as first parameter. Declare $addr as phys_addr_t and
s/$addr/addr/ ?
> get rid of the casts.
>
> Reported-by: Laurent Pinchart
> Signed-off-by: Marek Vasut
> ---
> Cc: Heinrich Schuchardt
> Cc: Kuninori Morimoto
> Cc: Laurent Pinchart
> Cc: Simon Glass
> Cc: Tom Rini
> ---
> boot/image-fdt.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index c2571b22244..c37442c9130 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> void*of_start = NULL;
> phys_addr_t start, size, usable;
> char*fdt_high;
> + phys_addr_t addr;
> phys_addr_t low;
> phys_size_t mapsize;
> ulong of_len = 0;
> @@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> fdt_high = env_get("fdt_high");
> if (fdt_high) {
> ulong desired_addr = hextoul(fdt_high, NULL);
> - ulong addr;
I would keep addr declared here. With that,
Reviewed-by: Laurent Pinchart
>
> if (desired_addr == ~0UL) {
> /* All ones means use fdt in place */
> @@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
>* At least part of this DRAM bank is usable, try
>* using it for LMB allocation.
>*/
> - of_start = map_sysmem((ulong)lmb_alloc_base(lmb,
> - of_len, 0x1000, usable), of_len);
> + addr = lmb_alloc_base(lmb, of_len, 0x1000, usable);
> + of_start = map_sysmem(addr, of_len);
> /* Allocation succeeded, use this block. */
> if (of_start != NULL)
> break;
--
Regards,
Laurent Pinchart
Re: [PATCH 1/3] boot: fdt: Change type of env_get_bootm_low() to phys_addr_t
Hi Marek,
Thank you for the patch.
On Sun, Mar 17, 2024 at 07:16:29AM +0100, Marek Vasut wrote:
> Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low().
> The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t,
> while the function itself still returns ulong. This is potentially dangerous
> on 64bit systems, where ulong might not be large enough to hold the content
> of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to
> what env_get_bootm_size() does, which returns phys_size_t .
>
> Reported-by: Laurent Pinchart
> Signed-off-by: Marek Vasut
> ---
> Cc: Heinrich Schuchardt
> Cc: Kuninori Morimoto
> Cc: Laurent Pinchart
> Cc: Simon Glass
> Cc: Tom Rini
> ---
> boot/bootm.c | 2 +-
> boot/image-board.c | 11 ++-
> boot/image-fdt.c | 9 +
> include/image.h| 2 +-
> 4 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/boot/bootm.c b/boot/bootm.c
> index d071537d692..2e818264f5f 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -242,7 +242,7 @@ static int boot_get_kernel(const char *addr_fit, struct
> bootm_headers *images,
> #ifdef CONFIG_LMB
> static void boot_start_lmb(struct bootm_headers *images)
> {
> - ulong mem_start;
> + phys_addr_t mem_start;
> phys_size_t mem_size;
>
> mem_start = env_get_bootm_low();
> diff --git a/boot/image-board.c b/boot/image-board.c
> index 75f6906cd56..447ced2678a 100644
> --- a/boot/image-board.c
> +++ b/boot/image-board.c
> @@ -107,12 +107,13 @@ static int on_loadaddr(const char *name, const char
> *value, enum env_op op,
> }
> U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr);
>
> -ulong env_get_bootm_low(void)
> +phys_addr_t env_get_bootm_low(void)
> {
> char *s = env_get("bootm_low");
> + phys_size_t tmp;
>
> if (s) {
> - ulong tmp = hextoul(s, NULL);
> + tmp = (phys_addr_t)simple_strtoull(s, NULL, 16);
> return tmp;
Can't you write
return (phys_addr_t)simple_strtoull(s, NULL, 16);
and avoid the temporary variable ?
Reviewed-by: Laurent Pinchart
> }
>
> @@ -538,7 +539,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data,
> ulong rd_len,
> ulong *initrd_start, ulong *initrd_end)
> {
> char*s;
> - ulong initrd_high;
> + phys_addr_t initrd_high;
> int initrd_copy_to_ram = 1;
>
> s = env_get("initrd_high");
> @@ -553,8 +554,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data,
> ulong rd_len,
> initrd_high = env_get_bootm_mapsize() + env_get_bootm_low();
> }
>
> - debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n",
> - initrd_high, initrd_copy_to_ram);
> + debug("## initrd_high = 0x%p, copy_to_ram = %d\n",
> + (void *)(uintptr_t)initrd_high, initrd_copy_to_ram);
>
> if (rd_data) {
> if (!initrd_copy_to_ram) { /* zero-copy ramdisk support */
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 5e4aa9de0d2..c2571b22244 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -160,9 +160,10 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> {
> void*fdt_blob = *of_flat_tree;
> void*of_start = NULL;
> - u64 start, size, usable;
> + phys_addr_t start, size, usable;
> char*fdt_high;
> - ulong mapsize, low;
> + phys_addr_t low;
> + phys_size_t mapsize;
> ulong of_len = 0;
> int bank;
> int err;
> @@ -217,7 +218,7 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> if (start + size < low)
> continue;
>
> - usable = min(start + size, (u64)(low + mapsize));
> + usable = min(start + size, low + mapsize);
>
> /*
>* At least part of this DRAM bank is usable, try
> @@ -233,7 +234,7 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
>* Reduce the mapping size in the next bank
>* by the size of attempt in current bank.
>*/
> - mapsize -= usable - max(start, (u64)low);
> + mapsize -= usable - max(start, low);
> if (!mapsize)
> break;
> }
> diff --git a/include/image.h b/include/image
Re: [PATCH] fdt: Fix bootm_low handling
Hi Marek,
Thank you for the patch.
On Sat, Mar 02, 2024 at 11:54:02PM +0100, Marek Vasut wrote:
> According to README CFG_SYS_BOOTMAPSZ section, in case both "bootm_low" and
> "bootm_size" variables are defined, "bootm_mapsize" variable is not defined
> and CFG_SYS_BOOTMAPSZ macro is not defined, all data for the Linux kernel
> must be between "bootm_low" and "bootm_low" + "bootm_size".
>
> Currently, for systems with DRAM between 0x4000_..0x7fff_ and with
> e.g. bootm_low=0x6000 and bootm_size=0x1000, the code will attempt
> to reserve memory from 0x4000_..0x4fff_, which is incorrect. This
> is because "bootm_low" is not taken into consideration correctly.
>
> The last parameter of lmb_alloc_base() is the maximum physical address of
> the to be reserved LMB area. Currently this is the start of DRAM bank that
> is considered for LMB area reservation + min(DRAM bank size, bootm_size).
> In case bootm_low is set to non-zero, this maximum physical address has to
> be shifted upward, to min(DRAM bank start + size, bootm_low + bootm_size),
> otherwise the reserved memory may be below bootm_low address.
>
> In case of multiple DRAM banks, the current change reserves top part of
> the first bank, and reserves the rest of memory in the follow up banks.
>
> Signed-off-by: Marek Vasut
> ---
> Cc: Geert Uytterhoeven
> Cc: Hans Verkuil
> Cc: Heinrich Schuchardt
> Cc: Kuninori Morimoto
> Cc: Laurent Pinchart
> Cc: Niklas Söderlund
> Cc: Simon Glass
> Cc: Tom Rini
> Cc: Wolfram Sang
> ---
> boot/image-fdt.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 75bdd55f326..5e4aa9de0d2 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -217,14 +217,14 @@ int boot_relocate_fdt(struct lmb *lmb, char
> **of_flat_tree, ulong *of_size)
> if (start + size < low)
> continue;
>
> - usable = min(size, (u64)mapsize);
> + usable = min(start + size, (u64)(low + mapsize));
low and mapsize are ulong, is there a risk this could overflow on 32-bit
platforms ?
I would rename usable to usable_end to make it clearer.
>
> /*
>* At least part of this DRAM bank is usable, try
>* using it for LMB allocation.
>*/
> of_start = map_sysmem((ulong)lmb_alloc_base(lmb,
> - of_len, 0x1000, start + usable), of_len);
> + of_len, 0x1000, usable), of_len);
> /* Allocation succeeded, use this block. */
> if (of_start != NULL)
> break;
The code continue with
/*
* Reduce the mapping size in the next bank
* by the size of attempt in current bank.
*/
mapsize -= usable - max(start, (u64)low);
if (!mapsize)
break;
which seems to be correct now, but interestingly was not before :-)
--
Regards,
Laurent Pinchart
Re: [PATCH] arm64: zynqmp: Remove snps,enable_guctl1_ipd_quirk property
Hi Michal, Thank you for the patch. On Mon, Feb 26, 2024 at 04:54:13PM +0100, Michal Simek wrote: > Remove undocumented DT property. Suggested solution was to apply quirk > via glue logic driver that's why make no sense to have it listed in DT. > > Signed-off-by: Michal Simek Reviewed-by: Laurent Pinchart > --- > > For more information please take a look at: > https://lore.kernel.org/r/1708023665-1441674-1-git-send-email-radhey.shyam.pan...@amd.com > https://lore.kernel.org/r/1708717523-4006664-1-git-send-email-radhey.shyam.pan...@amd.com > --- > arch/arm/dts/zynqmp.dtsi | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi > index b50b83b7723f..daae23c12b79 100644 > --- a/arch/arm/dts/zynqmp.dtsi > +++ b/arch/arm/dts/zynqmp.dtsi > @@ -1008,7 +1008,6 @@ > /* iommus = < 0x860>; */ > snps,quirk-frame-length-adjustment = <0x20>; > clock-names = "ref"; > - snps,enable_guctl1_ipd_quirk; > snps,resume-hs-terminations; > /* dma-coherent; */ > }; > @@ -1040,7 +1039,6 @@ > /* iommus = < 0x861>; */ > snps,quirk-frame-length-adjustment = <0x20>; > clock-names = "ref"; > - snps,enable_guctl1_ipd_quirk; > snps,resume-hs-terminations; > /* dma-coherent; */ > }; -- Regards, Laurent Pinchart
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
On Sat, Dec 09, 2023 at 10:13:59PM +0900, Chen-Yu Tsai wrote:
> On Thu, Dec 7, 2023 at 11:38 PM Laurent Pinchart
> wrote:
> >
> > On Thu, Dec 07, 2023 at 10:27:23PM +0800, Chen-Yu Tsai wrote:
> > > On Sun, Dec 03, 2023 at 05:34:01PM +0200, Laurent Pinchart wrote:
> > > > Hi Simon,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Fri, Dec 01, 2023 at 08:54:42PM -0700, Simon Glass wrote:
> > > > > Add a script which produces a Flat Image Tree (FIT), a single file
> > > > > containing the built kernel and associated devicetree files.
> > > > > Compression defaults to gzip which gives a good balance of size and
> > > > > performance.
> > > > >
> > > > > The files compress from about 86MB to 24MB using this approach.
> > > > >
> > > > > The FIT can be used by bootloaders which support it, such as U-Boot
> > > > > and Linuxboot. It permits automatic selection of the correct
> > > > > devicetree, matching the compatible string of the running board with
> > > > > the closest compatible string in the FIT. There is no need for
> > > > > filenames or other workarounds.
> > > > >
> > > > > Add a 'make image.fit' build target for arm64, as well. Use
> > > > > FIT_COMPRESSION to select a different algorithm.
> > > > >
> > > > > The FIT can be examined using 'dumpimage -l'.
> > > > >
> > > > > This features requires pylibfdt (use 'pip install libfdt'). It also
> > > > > requires compression utilities for the algorithm being used. Supported
> > > > > compression options are the same as the Image.xxx files. For now there
> > > > > is no way to change the compression other than by editing the rule for
> > > > > $(obj)/image.fit
> > > > >
> > > > > While FIT supports a ramdisk / initrd, no attempt is made to support
> > > > > this here, since it must be built separately from the Linux build.
> > > >
> > > > FIT images are very useful, so I think this is a very welcome addition
> > > > to the kernel build system. It can get tricky though: given the
> > > > versatile nature of FIT images, there can't be any
> > > > one-size-fits-them-all solution to build them, and striking the right
> > > > balance between what makes sense for the kernel and the features that
> > > > users may request will probably lead to bikeshedding. As we all love
> > > > bikeshedding, I thought I would start selfishly, with a personal use
> > > > case :-) This isn't a yak-shaving request though, I don't see any reason
> > > > to delay merging this series.
> > > >
> > > > Have you envisioned building FIT images with a subset of DTBs, or adding
> > > > DTBOs ? Both would be fairly trivial extensions to this script by
> > > > extending the supported command line arguments. It would perhaps be more
> > > > difficult to integrate in the kernel build system though. This leads me
> > > > to a second question: would you consider merging extensions to this
> > > > script if they are not used by the kernel build system, but meant for
> > > > users who manually invoke the script ? More generally, is the script
> > >
> > > We'd also be interested in some customization, though in a different way.
> > > We imagine having a rule file that says X compatible string should map
> > > to A base DTB, plus B and C DTBO for the configuration section. The base
> > > DTB would carry all common elements of some device, while the DTBOs
> > > carry all the possible second source components, like different display
> > > panels or MIPI cameras for instance. This could drastically reduce the
> > > size of FIT images in ChromeOS by deduplicating all the common stuff.
> >
> > Do you envision the "mapping" compatible string mapping to a config
> > section in the FIT image, that would bundle the base DTB and the DTBOs ?
>
> That's exactly the idea. The mapping compatible string could be untied
> from the base board's compatible string if needed (which we probably do).
>
> So something like:
>
> config {
> config-1 {
> compatible = "google,krane-sku0";
> fdt = "krane-baseboard", "krane-sku0-overlay";
> };
> };
>
> With "krane-sku0-overlay" being an overlay that holds the differences
> between the SKUs, in this case the display panel and MIPI camera (not
> upstreamed) that applies to SKU0 in particular.
The kernel DT makefiles already contain information on what overlays to
apply to what base boards, in order to test the overlays and produce
"full" DTBs. Maybe that information could be leveraged to create the
configurations in the FIT image ?
> Sorry for not giving a more concrete idea.
>
> > > > meant to be used stand-alone as well, in which case its command line
> > > > arguments need to remain backward-compatible, or do you see it as being
> > > > internal to the kernel ?
> > >
> > > [...]
--
Regards,
Laurent Pinchart
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
On Thu, Dec 07, 2023 at 01:52:53PM -0700, Simon Glass wrote: > On Thu, 7 Dec 2023 at 07:38, Laurent Pinchart wrote: > > On Thu, Dec 07, 2023 at 10:27:23PM +0800, Chen-Yu Tsai wrote: > > > On Sun, Dec 03, 2023 at 05:34:01PM +0200, Laurent Pinchart wrote: > > > > Hi Simon, > > > > > > > > Thank you for the patch. > > > > > > > > On Fri, Dec 01, 2023 at 08:54:42PM -0700, Simon Glass wrote: > > > > > Add a script which produces a Flat Image Tree (FIT), a single file > > > > > containing the built kernel and associated devicetree files. > > > > > Compression defaults to gzip which gives a good balance of size and > > > > > performance. > > > > > > > > > > The files compress from about 86MB to 24MB using this approach. > > > > > > > > > > The FIT can be used by bootloaders which support it, such as U-Boot > > > > > and Linuxboot. It permits automatic selection of the correct > > > > > devicetree, matching the compatible string of the running board with > > > > > the closest compatible string in the FIT. There is no need for > > > > > filenames or other workarounds. > > > > > > > > > > Add a 'make image.fit' build target for arm64, as well. Use > > > > > FIT_COMPRESSION to select a different algorithm. > > > > > > > > > > The FIT can be examined using 'dumpimage -l'. > > > > > > > > > > This features requires pylibfdt (use 'pip install libfdt'). It also > > > > > requires compression utilities for the algorithm being used. Supported > > > > > compression options are the same as the Image.xxx files. For now there > > > > > is no way to change the compression other than by editing the rule for > > > > > $(obj)/image.fit > > > > > > > > > > While FIT supports a ramdisk / initrd, no attempt is made to support > > > > > this here, since it must be built separately from the Linux build. > > > > > > > > FIT images are very useful, so I think this is a very welcome addition > > > > to the kernel build system. It can get tricky though: given the > > > > versatile nature of FIT images, there can't be any > > > > one-size-fits-them-all solution to build them, and striking the right > > > > balance between what makes sense for the kernel and the features that > > > > users may request will probably lead to bikeshedding. As we all love > > > > bikeshedding, I thought I would start selfishly, with a personal use > > > > case :-) This isn't a yak-shaving request though, I don't see any reason > > > > to delay merging this series. > > > > > > > > Have you envisioned building FIT images with a subset of DTBs, or adding > > > > DTBOs ? Both would be fairly trivial extensions to this script by > > > > extending the supported command line arguments. It would perhaps be more > > > > difficult to integrate in the kernel build system though. This leads me > > > > to a second question: would you consider merging extensions to this > > > > script if they are not used by the kernel build system, but meant for > > > > users who manually invoke the script ? More generally, is the script > > > > > > We'd also be interested in some customization, though in a different way. > > > We imagine having a rule file that says X compatible string should map > > > to A base DTB, plus B and C DTBO for the configuration section. The base > > > DTB would carry all common elements of some device, while the DTBOs > > > carry all the possible second source components, like different display > > > panels or MIPI cameras for instance. This could drastically reduce the > > > size of FIT images in ChromeOS by deduplicating all the common stuff. > > > > Do you envision the "mapping" compatible string mapping to a config > > section in the FIT image, that would bundle the base DTB and the DTBOs ? > > > > > > meant to be used stand-alone as well, in which case its command line > > > > arguments need to remain backward-compatible, or do you see it as being > > > > internal to the kernel ? > > It is great to see all this discussion! I did send a proposal to the > U-Boot ML about extensions but it was mixed up with other things, so > I'll start a new thread. > > For now, I am really just waiting for this to be applied, before > talking too much about future possibilities. Sure, I don't see any reason to delay this series until you have fixed everybody's system images problems :-) -- Regards, Laurent Pinchart
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
On Thu, Dec 07, 2023 at 10:27:23PM +0800, Chen-Yu Tsai wrote: > On Sun, Dec 03, 2023 at 05:34:01PM +0200, Laurent Pinchart wrote: > > Hi Simon, > > > > Thank you for the patch. > > > > On Fri, Dec 01, 2023 at 08:54:42PM -0700, Simon Glass wrote: > > > Add a script which produces a Flat Image Tree (FIT), a single file > > > containing the built kernel and associated devicetree files. > > > Compression defaults to gzip which gives a good balance of size and > > > performance. > > > > > > The files compress from about 86MB to 24MB using this approach. > > > > > > The FIT can be used by bootloaders which support it, such as U-Boot > > > and Linuxboot. It permits automatic selection of the correct > > > devicetree, matching the compatible string of the running board with > > > the closest compatible string in the FIT. There is no need for > > > filenames or other workarounds. > > > > > > Add a 'make image.fit' build target for arm64, as well. Use > > > FIT_COMPRESSION to select a different algorithm. > > > > > > The FIT can be examined using 'dumpimage -l'. > > > > > > This features requires pylibfdt (use 'pip install libfdt'). It also > > > requires compression utilities for the algorithm being used. Supported > > > compression options are the same as the Image.xxx files. For now there > > > is no way to change the compression other than by editing the rule for > > > $(obj)/image.fit > > > > > > While FIT supports a ramdisk / initrd, no attempt is made to support > > > this here, since it must be built separately from the Linux build. > > > > FIT images are very useful, so I think this is a very welcome addition > > to the kernel build system. It can get tricky though: given the > > versatile nature of FIT images, there can't be any > > one-size-fits-them-all solution to build them, and striking the right > > balance between what makes sense for the kernel and the features that > > users may request will probably lead to bikeshedding. As we all love > > bikeshedding, I thought I would start selfishly, with a personal use > > case :-) This isn't a yak-shaving request though, I don't see any reason > > to delay merging this series. > > > > Have you envisioned building FIT images with a subset of DTBs, or adding > > DTBOs ? Both would be fairly trivial extensions to this script by > > extending the supported command line arguments. It would perhaps be more > > difficult to integrate in the kernel build system though. This leads me > > to a second question: would you consider merging extensions to this > > script if they are not used by the kernel build system, but meant for > > users who manually invoke the script ? More generally, is the script > > We'd also be interested in some customization, though in a different way. > We imagine having a rule file that says X compatible string should map > to A base DTB, plus B and C DTBO for the configuration section. The base > DTB would carry all common elements of some device, while the DTBOs > carry all the possible second source components, like different display > panels or MIPI cameras for instance. This could drastically reduce the > size of FIT images in ChromeOS by deduplicating all the common stuff. Do you envision the "mapping" compatible string mapping to a config section in the FIT image, that would bundle the base DTB and the DTBOs ? > > meant to be used stand-alone as well, in which case its command line > > arguments need to remain backward-compatible, or do you see it as being > > internal to the kernel ? > > [...] > > ChenYu -- Regards, Laurent Pinchart
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
cdir:
> +# Handle devicetree files
> +if os.path.isdir(path):
> +for dirpath, _, fnames in os.walk(path):
> +for fname in fnames:
> +if os.path.splitext(fname)[1] != '.dtb':
> +continue
> +pathname = os.path.join(dirpath, fname)
> +seq += 1
> +size += os.path.getsize(pathname)
> +model, compat = output_dtb(fsw, seq, pathname,
> + args.arch, args.compress)
> +entries.append([model, compat])
> +
> +finish_fit(fsw, entries)
> +
> +# Include the kernel itself in the returned file count
> +return fsw.as_fdt().as_bytearray(), seq + 1, size
> +
> +
> +def run_make_fit():
> +"""Run the tool's main logic"""
> +args = parse_args()
> +
> +out_data, count, size = build_fit(args)
> +with open(args.fit, 'wb') as outf:
> +outf.write(out_data)
> +
> +ext_fit_size = None
> +if args.external:
> +mkimage = os.environ.get('MKIMAGE', 'mkimage')
> +subprocess.check_call([mkimage, '-E', '-F', args.fit],
> + stdout=subprocess.DEVNULL)
> +
> +with open(args.fit, 'rb') as inf:
> +data = inf.read()
> +ext_fit = libfdt.FdtRo(data)
> +ext_fit_size = ext_fit.totalsize()
> +
> +comp_size = len(out_data)
> +print(f'FIT size {comp_size:#x}/{comp_size / 1024 / 1024:.1f} MB',
> end='')
> +if ext_fit_size:
> +print(f', header {ext_fit_size:#x}/{ext_fit_size / 1024:.1f} KB',
> end='')
> +print(f', {count} files, uncompressed {size / 1024 / 1024:.1f} MB')
> +
> +
> +if __name__ == "__main__":
> +sys.exit(run_make_fit())
--
Regards,
Laurent Pinchart
Re: [RFC/PATCH] lib/Kconfig: Enable OF_LIBFDT_OVERLAY by default when FIT is enabled
Hi Tom, On Fri, Mar 10, 2023 at 01:12:24PM -0500, Tom Rini wrote: > On Sun, Jan 29, 2023 at 06:30:22PM +0200, Laurent Pinchart wrote: > > > FIT image support is commonly used to bundle a kernel image, a device > > tree, and device tree overlays. Applying overlays requires the > > OF_LIBFDT_OVERLAY config option to be set, which lots of boards fail to > > select, most likely because developers never noticed. This leads to an > > error when trying to apply overlays: > > > > "config with overlays but CONFIG_OF_LIBFDT_OVERLAY not set" > > > > TI ARM boards select the option by default. Extend this to all systems > > that select the FIT option. This only affects the default, overlay > > support can still be disabled manually in the configuration. > > > > Signed-off-by: Laurent Pinchart > > Reviewed-by: Marek Vasut > > Reviewed-by: Simon Glass > > --- > > I'm posting this as an RFC to get feedback. If the idea is generally > > appreciated, I'll update the defconfig files accordingly. > > Alright, so, I put this through a world build, and most platforms grow > by 4-5kB. Thank you for testing this, despite the patch falling off my radar. > I think that means what I'd really like to see as a starting > point is more SoCs doing an "imply OF_LIBFDT_OVERLAY if OF_LIBFDT && FIT" > or adding to the default y list below, or similar. If that brings us to > the point where a good number of ARM boards with FIT are enabling it, we > can default y if ARM, for example. But right now it's more like several > hundred boards growing in size, which is uncomfortable, given the size > it's growing by. I'm fine with that. I've submitted the original patch because I had to update a vendor-supplied U-Boot binary to get overlay support, which ended up being a bit rabbit hole for various reasons. I thought it would be nice to save users from this kind of trouble. I can send patches to enable the option for SoC I care about, but generally speaking, who should decide which SoC(s) should imply OF_LIBFDT_OVERLAY ? -- Regards, Laurent Pinchart
[RFC/PATCH] lib/Kconfig: Enable OF_LIBFDT_OVERLAY by default when FIT is enabled
FIT image support is commonly used to bundle a kernel image, a device tree, and device tree overlays. Applying overlays requires the OF_LIBFDT_OVERLAY config option to be set, which lots of boards fail to select, most likely because developers never noticed. This leads to an error when trying to apply overlays: "config with overlays but CONFIG_OF_LIBFDT_OVERLAY not set" TI ARM boards select the option by default. Extend this to all systems that select the FIT option. This only affects the default, overlay support can still be disabled manually in the configuration. Signed-off-by: Laurent Pinchart --- I'm posting this as an RFC to get feedback. If the idea is generally appreciated, I'll update the defconfig files accordingly. --- lib/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Kconfig b/lib/Kconfig index 549bd3577851..d309ccf83c03 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -823,7 +823,7 @@ config OF_LIBFDT_ASSUME_MASK config OF_LIBFDT_OVERLAY bool "Enable the FDT library overlay support" depends on OF_LIBFDT - default y if ARCH_OMAP2PLUS || ARCH_KEYSTONE + default y if FIT help This enables the FDT library (libfdt) overlay support. base-commit: f147aa80f52989c7455022ca1ab959e8545feccc -- Regards, Laurent Pinchart
Re: TFTP hangs with fragmented IP packets
Hi Ramon, On Sun, Apr 03, 2022 at 02:17:26AM +0300, Ramon Fried wrote: > On Thu, Mar 31, 2022 at 8:43 AM Ramon Fried wrote: > > On Tue, Mar 29, 2022 at 11:28 PM Laurent Pinchart wrote: > > > > > > Hello, > > > > > > I've banged my head a few days ago trying to debug an issue with a TFTP > > > transfer hanging in the middle. > > > > > > I'm testing U-Boot 2022-rc5 on a Toradex Verdin i.MX8MP module (using > > > the verdin-imx8mp defconfig). My local network MTU is 1500 bytes, and > > > the board uses the EQoS ethernet controller. > > > > > > The problem started occurring after rebuilding a kernel image. U-Boot > > > started transferring the image, and stopped in the middle, eventually > > > timing out. Capture network traffic showed that U-Boot was continuously > > > asking for retransmit of the same block, and eventually timed out. > > > > > > U-Boot is configured with a default TFTP block size of 4096 bytes, which > > > results in the TFTP blocks being sent in one UDP packet split in three > > > IP packets. U-Boot is configured with IP fragmentation supprot enabled. > > > This works fine for all TFTP blocks until a paticular one in the middle > > > of the kernel image. > > > > > > I've narrowed it down to a file of 1472 that can't be transferred at > > > all (I have attached the binary to this e-mail). Changing the value of > > > any of the last two bytes of the file allows transferring it correctly, > > > so I suspect a CRC issue, likely related to IP fragmentation. Lowering > > > the TFTP block size to avoid fragmentation works around the problem. > > > > > > Arguably a TFTP block size of 4096 bytes should probably not be used > > > with a 1500 bytes MTU network, but I thought it would be useful to fix > > > the issue nonetheless. > > > > > > I can test patches. > > > > > Interesting. > > I will try to reproduce. > > Thanks for reporting. > > Hi. > I couldn't reproduce the issue with the file you provided. > I used sandbox64, and changed the TFTP BLOCK size to 4096. > I managed to TFTP the file successfully. > > => setenv autoload no > => setenv ethrotate no > => setenv ethact eth0 > => setenv ipaddr > => setenv serverip 172.23.1.137 > => env set ipaddr 172.23.1.100 > => tftpboot 0x10 test.bin > Using host_eno1 device > TFTP from server 172.23.1.137; our IP address is 172.23.1.100 > Filename 'test.bin'. > Load address: 0x10 > Loading: # > 718.8 KiB/s > done > Bytes transferred = 1472 (5c0 hex) > => H... I wonder if it could be specific to the EQoS ethernet controller in the i.MX8, maybe caused by issues in hardware CRC calculation ? I don't have any other board I can use for testing this at the moment as I'm travelling. > Can you perhaps send me a network PCAP of your device failing to > digest the file ? I'll try to do that tomorrow. -- Regards, Laurent Pinchart
TFTP hangs with fragmented IP packets
Hello, I've banged my head a few days ago trying to debug an issue with a TFTP transfer hanging in the middle. I'm testing U-Boot 2022-rc5 on a Toradex Verdin i.MX8MP module (using the verdin-imx8mp defconfig). My local network MTU is 1500 bytes, and the board uses the EQoS ethernet controller. The problem started occurring after rebuilding a kernel image. U-Boot started transferring the image, and stopped in the middle, eventually timing out. Capture network traffic showed that U-Boot was continuously asking for retransmit of the same block, and eventually timed out. U-Boot is configured with a default TFTP block size of 4096 bytes, which results in the TFTP blocks being sent in one UDP packet split in three IP packets. U-Boot is configured with IP fragmentation supprot enabled. This works fine for all TFTP blocks until a paticular one in the middle of the kernel image. I've narrowed it down to a file of 1472 that can't be transferred at all (I have attached the binary to this e-mail). Changing the value of any of the last two bytes of the file allows transferring it correctly, so I suspect a CRC issue, likely related to IP fragmentation. Lowering the TFTP block size to avoid fragmentation works around the problem. Arguably a TFTP block size of 4096 bytes should probably not be used with a 1500 bytes MTU network, but I thought it would be useful to fix the issue nonetheless. I can test patches. -- Regards, Laurent Pinchart test.bin Description: Binary data
Re: [U-Boot] [RFC] ARM: rmobile: create DT memory nodes for R8A7795 3.0 and newer
Hi Geert, On Tuesday, 19 June 2018 09:58:59 EEST Geert Uytterhoeven wrote: > On Tue, Jun 19, 2018 at 4:15 AM Laurent Pinchart wrote: > > On Sunday, 17 June 2018 03:08:02 EEST Marek Vasut wrote: > >> On 06/16/2018 05:44 PM, Laurent Pinchart wrote: > >>> On Saturday, 16 June 2018 02:42:30 EEST Marek Vasut wrote: > >>>> On 06/16/2018 01:21 AM, Laurent Pinchart wrote: > >>>>> On Friday, 15 June 2018 15:00:31 EEST Marek Vasut wrote: > >>>>>> On 06/15/2018 01:43 PM, Marek Vasut wrote: [snip] > >>>>>>>>> Obvious design question is -- since you're adding new SMC call > >>>>>>>>> anyway, can't the call just return the memory layout table > >>>>>>>>> itself, so that it won't be duplicated both in U-Boot and ATF ? > >>>>>>>> > >>>>>>>> My gut feeling was to go with the smallest interface possible. > >>>>>>> > >>>>>>> But this doesn't scale. The API here uses some ad-hoc constants to > >>>>>>> identify memory layout tables which have to be encoded both in ATF > >>>>>>> and U-Boot, both of which must be kept in sync. > >>>>>>> > >>>>>>> The ATF already has those memory layout tables, it's only a matter > >>>>>>> of passing them to U-Boot. If you do just that, the ad-hoc > >>>>>>> constants and encoding of tables into U-Boot goes away and in fact > >>>>>>> simplifies the design. > >>>>>>> > >>>>>>> Yet, I have to wonder if ATF doesn't already contain some sort of > >>>>>>> standard SMC call to get memory topology. It surprises me that it > >>>>>>> wouldn't. > >>>>>> > >>>>>> In fact, Laurent (CCed) was solving some similar issue with lossy > >>>>>> decomp and I think this involved some passing of memory layout > >>>>>> information from ATF to U-Boot too, or am I mistaken ? > >>>>> > >>>>> That's correct, ATF stores information about the memory layout at a > >>>>> fixed address in system memory, and U-Boot can read it. > >>>> > >>>> Well, that sounds good ! Maybe we can avoid adding SMC call > >>>> altogether then? :) > >>> > >>> I'd prefer that, yes. > >>> > >>> Let's not duplicate the mechanism used to pass FCNL information from > >>> ATF to U- Boot, but instead create a data table format that can store > >>> all the information we need, in an easily extensible way. > >>> > >>> To see how the mechanism is implemented for FCNL, search for 47FD7000 > >>> in the Renesas ATF sources > >>> (git://github.com/renesas-rcar/arm-trusted-firmware.git). > >> > >> For everyone involved, can you explain what FCNL is ? ;-) > > > > FCNL is Frame Compression Near Lossless. It's a way to reduce memory > > bandwidth by transparent compression and decompression of video frames. > > Compression is handled by an IP core called FCP, and decompression is > > handled by the DRAM controller. ATF programs the DRAM controller with > > ranges of memory addresses that will be dynamically decompressed. The > > registers containing those ranges are accessible in secure mode only, so > > neither U-Boot nor Linux can read them. That's why ATF has to pass the > > information to U-Boot, in order to add the ranges as reserved memory in > > DT. > > > >> Any yes, I agree this sounds good. I had a discussion on the U-Boot IRC > >> about passing the memory configuration around and the result is > >> basically the same -- pass a table from ATF to U-Boot. If there's > >> already something, great. > > Pass a "table"? Or an FDT containing the /memory nodes? > The latter allows for easier future extension. ATF passes a table to U-Boot, and U-Boot then updates the FDT accordingly before starting Linux. -- Regards, Laurent Pinchart ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ARM: rmobile: create DT memory nodes for R8A7795 3.0 and newer
Hi Magnus, On Tuesday, 19 June 2018 08:43:31 EEST Magnus Damm wrote: > On Tue, Jun 19, 2018 at 11:15 AM, Laurent Pinchart wrote: > > On Sunday, 17 June 2018 03:08:02 EEST Marek Vasut wrote: > >> On 06/16/2018 05:44 PM, Laurent Pinchart wrote: > >>> On Saturday, 16 June 2018 02:42:30 EEST Marek Vasut wrote: > >>>> On 06/16/2018 01:21 AM, Laurent Pinchart wrote: > >>>>> On Friday, 15 June 2018 15:00:31 EEST Marek Vasut wrote: > >>>>>> On 06/15/2018 01:43 PM, Marek Vasut wrote: [snip] > >>>>>>> Yet, I have to wonder if ATF doesn't already contain some sort of > >>>>>>> standard SMC call to get memory topology. It surprises me that it > >>>>>>> wouldn't. > >>>>>> > >>>>>> In fact, Laurent (CCed) was solving some similar issue with lossy > >>>>>> decomp and I think this involved some passing of memory layout > >>>>>> information from ATF to U-Boot too, or am I mistaken ? > >>>>> > >>>>> That's correct, ATF stores information about the memory layout at a > >>>>> fixed address in system memory, and U-Boot can read it. > >>>> > >>>> Well, that sounds good ! Maybe we can avoid adding SMC call altogether > >>>> then? :) > >>> > >>> I'd prefer that, yes. > >>> > >>> Let's not duplicate the mechanism used to pass FCNL information from > >>> ATF to U- Boot, but instead create a data table format that can store > >>> all the information we need, in an easily extensible way. > >>> > >>> To see how the mechanism is implemented for FCNL, search for 47FD7000 > >>> in the Renesas ATF sources > >>> (git://github.com/renesas-rcar/arm-trusted-firmware.git). > >> > >> For everyone involved, can you explain what FCNL is ? ;-) > > > > FCNL is Frame Compression Near Lossless. It's a way to reduce memory > > bandwidth by transparent compression and decompression of video frames. > > Compression is handled by an IP core called FCP, and decompression is > > handled by the DRAM controller. ATF programs the DRAM controller with > > ranges of memory addresses that will be dynamically decompressed. The > > registers containing those ranges are accessible in secure mode only, so > > neither U-Boot nor Linux can read them. That's why ATF has to pass the > > information to U-Boot, in order to add the ranges as reserved memory in > > DT. > > Thanks for the clarification. I wonder how much of the split between > ATF and "the rest" can be adjusted. It smells like just a software > architecture policy, my gut feeling is that LIFEC can be programmed to > adjust the assignment between secure side and "regular". At least it > can do so for a wide range of bus masters. However if this is the case > for FCNL remains to be seen. > > As a side note, for FCNL I personally would prefer a more dynamic > solution where we manage physically contiguous ranges from Linux > during run time instead of relying of statically setup windows. So would I, but whether we can be allowed to access those registers from Linux isn't my decision :-/ > >> Any yes, I agree this sounds good. I had a discussion on the U-Boot IRC > >> about passing the memory configuration around and the result is > >> basically the same -- pass a table from ATF to U-Boot. If there's > >> already something, great. -- Regards, Laurent Pinchart ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ARM: rmobile: create DT memory nodes for R8A7795 3.0 and newer
On Sunday, 17 June 2018 03:08:02 EEST Marek Vasut wrote:
> On 06/16/2018 05:44 PM, Laurent Pinchart wrote:
> > Hi Marek,
> >
> > On Saturday, 16 June 2018 02:42:30 EEST Marek Vasut wrote:
> >> On 06/16/2018 01:21 AM, Laurent Pinchart wrote:
> >>> On Friday, 15 June 2018 15:00:31 EEST Marek Vasut wrote:
> >>>> On 06/15/2018 01:43 PM, Marek Vasut wrote:
> >>>>> On 06/15/2018 12:37 PM, Ulrich Hecht wrote:
> >>>>>> On Fri, Jun 15, 2018 at 12:09 PM, Marek Vasut wrote:
> >>>>>>>> + arm_smccc_smc(ARM_SMCCC_RENESAS_MEMCONF,
> >>>>>>>> + 0, 0, 0, 0, 0, 0, 0, );
> >>>>>>>
> >>>>>>> Will this call work on platforms without patched ATF ?
> >>>>>>> (I think not, don't you need to handle return value?)
> >>>>>>
> >>>>>> I have not actually tested that, but if I understand the ATF code
> >>>>>> correctly, unimplemented calls return
> >>>>>> SMC_UNK (0x), which should be handled by the default case
> >>>>>> (NOP)
> >>>>>> below.
> >>>>>
> >>>>> Which means the board has a memory size of 0 and fails to boot ?
> >>>>>
> >>>>>>>> + switch (res.a0) {
> >>>>>>>> + case 1:
> >>>>>>>> + base[0] = 0x04800ULL;
> >>>>>>>> + size[0] = 0x03800ULL;
> >>>>>>>> + base[1] = 0x5ULL;
> >>>>>>>> + size[1] = 0x04000ULL;
> >>>>>>>> + base[2] = 0x6ULL;
> >>>>>>>> + size[2] = 0x04000ULL;
> >>>>>>>> + base[3] = 0x7ULL;
> >>>>>>>> + size[3] = 0x04000ULL;
> >>>>>>>> + fdt_fixup_memory_banks(blob, base, size, 4);
> >>>>>>>> + break;
> >>>>>>>> + case 2:
> >>>>>>>> + base[0] = 0x04800ULL;
> >>>>>>>> + size[0] = 0x07800ULL;
> >>>>>>>> + base[1] = 0x5ULL;
> >>>>>>>> + size[1] = 0x08000ULL;
> >>>>>>>> + fdt_fixup_memory_banks(blob, base, size, 2);
> >>>>>>>> + break;
> >>>>>>>> + case 3:
> >>>>>>>> + base[0] = 0x04800ULL;
> >>>>>>>> + size[0] = 0x07800ULL;
> >>>>>>>> + base[1] = 0x5ULL;
> >>>>>>>> + size[1] = 0x08000ULL;
> >>>>>>>> + base[2] = 0x6ULL;
> >>>>>>>> + size[2] = 0x08000ULL;
> >>>>>>>> + base[3] = 0x7ULL;
> >>>>>>>> + size[3] = 0x08000ULL;
> >>>>>>>> + fdt_fixup_memory_banks(blob, base, size, 4);
> >>>>>>>> + break;
> >>>>>>>
> >>>>>>> Obvious design question is -- since you're adding new SMC call
> >>>>>>> anyway,
> >>>>>>> can't the call just return the memory layout table itself, so that
> >>>>>>> it
> >>>>>>> won't be duplicated both in U-Boot and ATF ?
> >>>>>>
> >>>>>> My gut feeling was to go with the smallest interface possible.
> >>>>>
> >>>>> But this doesn't scale. The API here uses some ad-hoc constants to
> >>>>> identify memory layout tables which have to be encoded both in ATF and
> >>>>> U-Boot, both of which must be kept in sync.
> >>>>>
> >>>>> The ATF already has those memory layout tables, it's only a matter of
> >>>>> passing them to U-Boot. If you do just that, the ad-hoc constants and
>
Re: [U-Boot] [RFC] ARM: rmobile: create DT memory nodes for R8A7795 3.0 and newer
Hi Marek,
On Saturday, 16 June 2018 02:42:30 EEST Marek Vasut wrote:
> On 06/16/2018 01:21 AM, Laurent Pinchart wrote:
> > On Friday, 15 June 2018 15:00:31 EEST Marek Vasut wrote:
> >> On 06/15/2018 01:43 PM, Marek Vasut wrote:
> >>> On 06/15/2018 12:37 PM, Ulrich Hecht wrote:
> >>>> On Fri, Jun 15, 2018 at 12:09 PM, Marek Vasut wrote:
> >>>>>> + arm_smccc_smc(ARM_SMCCC_RENESAS_MEMCONF,
> >>>>>> + 0, 0, 0, 0, 0, 0, 0, );
> >>>>>
> >>>>> Will this call work on platforms without patched ATF ?
> >>>>> (I think not, don't you need to handle return value?)
> >>>>
> >>>> I have not actually tested that, but if I understand the ATF code
> >>>> correctly, unimplemented calls return
> >>>> SMC_UNK (0x), which should be handled by the default case (NOP)
> >>>> below.
> >>>
> >>> Which means the board has a memory size of 0 and fails to boot ?
> >>>
> >>>>>> + switch (res.a0) {
> >>>>>> + case 1:
> >>>>>> + base[0] = 0x04800ULL;
> >>>>>> + size[0] = 0x03800ULL;
> >>>>>> + base[1] = 0x5ULL;
> >>>>>> + size[1] = 0x04000ULL;
> >>>>>> + base[2] = 0x6ULL;
> >>>>>> + size[2] = 0x04000ULL;
> >>>>>> + base[3] = 0x7ULL;
> >>>>>> + size[3] = 0x04000ULL;
> >>>>>> + fdt_fixup_memory_banks(blob, base, size, 4);
> >>>>>> + break;
> >>>>>> + case 2:
> >>>>>> + base[0] = 0x04800ULL;
> >>>>>> + size[0] = 0x07800ULL;
> >>>>>> + base[1] = 0x5ULL;
> >>>>>> + size[1] = 0x08000ULL;
> >>>>>> + fdt_fixup_memory_banks(blob, base, size, 2);
> >>>>>> + break;
> >>>>>> + case 3:
> >>>>>> + base[0] = 0x04800ULL;
> >>>>>> + size[0] = 0x07800ULL;
> >>>>>> + base[1] = 0x5ULL;
> >>>>>> + size[1] = 0x08000ULL;
> >>>>>> + base[2] = 0x6ULL;
> >>>>>> + size[2] = 0x08000ULL;
> >>>>>> + base[3] = 0x7ULL;
> >>>>>> + size[3] = 0x08000ULL;
> >>>>>> + fdt_fixup_memory_banks(blob, base, size, 4);
> >>>>>> + break;
> >>>>>
> >>>>> Obvious design question is -- since you're adding new SMC call anyway,
> >>>>> can't the call just return the memory layout table itself, so that it
> >>>>> won't be duplicated both in U-Boot and ATF ?
> >>>>
> >>>> My gut feeling was to go with the smallest interface possible.
> >>>
> >>> But this doesn't scale. The API here uses some ad-hoc constants to
> >>> identify memory layout tables which have to be encoded both in ATF and
> >>> U-Boot, both of which must be kept in sync.
> >>>
> >>> The ATF already has those memory layout tables, it's only a matter of
> >>> passing them to U-Boot. If you do just that, the ad-hoc constants and
> >>> encoding of tables into U-Boot goes away and in fact simplifies the
> >>> design.
> >>>
> >>> Yet, I have to wonder if ATF doesn't already contain some sort of
> >>> standard SMC call to get memory topology. It surprises me that it
> >>> wouldn't.
> >>
> >> In fact, Laurent (CCed) was solving some similar issue with lossy decomp
> >> and I think this involved some passing of memory layout information from
> >> ATF to U-Boot too, or am I mistaken ?
> >
> > That's correct, ATF stores information about the memory layout at a fixed
> > address in system memory, and U-Boot can read it.
>
> Well, that sounds good ! Maybe we can avoid adding SMC call altogether
> then? :)
I'd prefer that, yes.
Let's not duplicate the mechanism used to pass FCNL information from ATF to U-
Boot, but instead create a data table format that can store all the
information we need, in an easily extensible way.
To see how the mechanism is implemented for FCNL, search for 47FD7000 in the
Renesas ATF sources (git://github.com/renesas-rcar/arm-trusted-firmware.git).
--
Regards,
Laurent Pinchart
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ARM: rmobile: create DT memory nodes for R8A7795 3.0 and newer
Hi Marek,
On Friday, 15 June 2018 15:00:31 EEST Marek Vasut wrote:
> On 06/15/2018 01:43 PM, Marek Vasut wrote:
> > On 06/15/2018 12:37 PM, Ulrich Hecht wrote:
> >> On Fri, Jun 15, 2018 at 12:09 PM, Marek Vasut
wrote:
> >>>> + arm_smccc_smc(ARM_SMCCC_RENESAS_MEMCONF,
> >>>> + 0, 0, 0, 0, 0, 0, 0, );
> >>>
> >>> Will this call work on platforms without patched ATF ?
> >>> (I think not, don't you need to handle return value?)
> >>
> >> I have not actually tested that, but if I understand the ATF code
> >> correctly, unimplemented calls return
> >> SMC_UNK (0x), which should be handled by the default case (NOP)
> >> below.>
> > Which means the board has a memory size of 0 and fails to boot ?
> >
> >>>> + switch (res.a0) {
> >>>> + case 1:
> >>>> + base[0] = 0x04800ULL;
> >>>> + size[0] = 0x03800ULL;
> >>>> + base[1] = 0x5ULL;
> >>>> + size[1] = 0x04000ULL;
> >>>> + base[2] = 0x6ULL;
> >>>> + size[2] = 0x04000ULL;
> >>>> + base[3] = 0x7ULL;
> >>>> + size[3] = 0x04000ULL;
> >>>> + fdt_fixup_memory_banks(blob, base, size, 4);
> >>>> + break;
> >>>> + case 2:
> >>>> + base[0] = 0x04800ULL;
> >>>> + size[0] = 0x07800ULL;
> >>>> + base[1] = 0x5ULL;
> >>>> + size[1] = 0x08000ULL;
> >>>> + fdt_fixup_memory_banks(blob, base, size, 2);
> >>>> + break;
> >>>> + case 3:
> >>>> + base[0] = 0x04800ULL;
> >>>> + size[0] = 0x07800ULL;
> >>>> + base[1] = 0x5ULL;
> >>>> + size[1] = 0x08000ULL;
> >>>> + base[2] = 0x6ULL;
> >>>> + size[2] = 0x08000ULL;
> >>>> + base[3] = 0x7ULL;
> >>>> + size[3] = 0x08000ULL;
> >>>> + fdt_fixup_memory_banks(blob, base, size, 4);
> >>>> + break;
> >>>
> >>> Obvious design question is -- since you're adding new SMC call anyway,
> >>> can't the call just return the memory layout table itself, so that it
> >>> won't be duplicated both in U-Boot and ATF ?
> >>
> >> My gut feeling was to go with the smallest interface possible.
> >
> > But this doesn't scale. The API here uses some ad-hoc constants to
> > identify memory layout tables which have to be encoded both in ATF and
> > U-Boot, both of which must be kept in sync.
> >
> > The ATF already has those memory layout tables, it's only a matter of
> > passing them to U-Boot. If you do just that, the ad-hoc constants and
> > encoding of tables into U-Boot goes away and in fact simplifies the
> > design.
> >
> > Yet, I have to wonder if ATF doesn't already contain some sort of
> > standard SMC call to get memory topology. It surprises me that it
> > wouldn't.
>
> In fact, Laurent (CCed) was solving some similar issue with lossy decomp
> and I think this involved some passing of memory layout information from
> ATF to U-Boot too, or am I mistaken ?
That's correct, ATF stores information about the memory layout at a fixed
address in system memory, and U-Boot can read it.
--
Regards,
Laurent Pinchart
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] cmd/fdt: support single value replacement within an array
Hi Hannes,
On Friday 18 Aug 2017 10:07:19 Hannes Schmelzer wrote:
> Laurent Pinchart schrieb am 04.08.2017 23:23:19:
>
> Hi Laurent,
>
> as told a few days ago i'm now coming back to this issue.
Thank you.
> > (I'm not subscribed to the list, please keep me CC'ed on replies)
>
> subscribing to the list would be a great idea, so you're always up to date
> whats going on ;-)
I know, but I've already stopped reading mailing lists I'm subscribed to due
to the amount of traffic, so I figured out that subscribing to new ones
wouldn't be a good idea :-)
> > On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> >> With this commit we can modify single values within an array of a dts
> >> property.
> >
> > But with this commit U-Boot crashes if you try to create a new property
> > with the fdt set command :-/
> >
> > I've tested v2017.07 with the commit reverted, and fdt set works again
> > for me. The issue is that your fdt_getprop() call fails and return NULL
> > with len set to -1. You can easily imagine what the memcpy() following it
> > will do.
>
> Yes. Your'e right with that.
>
> I just checked most current source (...), there were changes regarding
> this issue.
> Tom introduced a check against the fail of fdt_getprop(...) call.
>
> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=99bb38e2cce9d99238458e0f6d18
> 80c6d2e80a4d
>
> Can you please try with your testcase with most current source again?
>
> please let us know if the problem is fixed with this.
I can't test that right now as I don't have access to my hardware at the
moment, but I doubt it will work.
The code now reads as
ptmp = fdt_getprop(working_fdt, nodeoffset, prop, );
if (!ptmp) {
printf("prop (%s) not found!\n", prop);
return 1;
}
The new !ptmp check should prevent the crash, but it will also prevent the fdt
set command from operating correctly, as it will return an error if the
property isn't found.
--
Regards,
Laurent Pinchart
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] cmd/fdt: support single value replacement within an array
Hi Hannes,
(I'm not subscribed to the list, please keep me CC'ed on replies)
On Tuesday 30 May 2017 13:05:00 Hannes Schmelzer wrote:
> With this commit we can modify single values within an array of a dts
> property.
But with this commit U-Boot crashes if you try to create a new property with
the fdt set command :-/
I've tested v2017.07 with the commit reverted, and fdt set works again for me.
The issue is that your fdt_getprop() call fails and return NULL with len set
to -1. You can easily imagine what the memcpy() following it will do.
> This is useful if we have for example a pwm-backlight where we want to
> modifiy the pwm frequency per u-boot script.
>
> The pwm is described in dts like this:
>
> backlight {
> pwms = <0x002b 0x 0x004c4b40>;
> };
>
> For changing the frequency, here the 3rd parameter, we simply type:
>
> fdt set /backlight pwms ;
>
> For doing all this we:
> - backup the property content into our 'SCRATCHPAD'
> - only modify the array-cell if the new content doesn't start with '?'
>
> Signed-off-by: Hannes Schmelzer <hannes.schmel...@br-automation.com>
>
> ---
>
> cmd/fdt.c | 29 +
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index a21415d..e55102a 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -257,6 +257,7 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc,
> char * const argv[])>
> char *prop; /* property */
> int nodeoffset;/* node offset from libfdt */
> static char data[SCRATCHPAD]; /* storage for the property */
> + const void *ptmp;
> int len; /* new length of the property */
> int ret; /* return value */
>
> @@ -268,13 +269,6 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[])>
> pathp = argv[2];
> prop = argv[3];
>
> - if (argc == 4) {
> - len = 0;
> - } else {
> - ret = fdt_parse_prop([4], argc - 4, data, );
> - if (ret != 0)
> - return ret;
> - }
>
> nodeoffset = fdt_path_offset (working_fdt, pathp);
> if (nodeoffset < 0) {
> @@ -286,6 +280,21 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[])
> return 1;
> }
>
> + if (argc == 4) {
> + len = 0;
> + } else {
> + ptmp = fdt_getprop(working_fdt, nodeoffset, prop,
> );
> + if (len > SCRATCHPAD) {
> + printf("prop (%d) doesn't fit in scratchpad!
> \n",
> +len);
> + return 1;
> + }
> + memcpy(data, ptmp, len);
> + ret = fdt_parse_prop([4], argc - 4, data, );
> + if (ret != 0)
> + return ret;
> + }
> +
> ret = fdt_setprop(working_fdt, nodeoffset, prop, data, len);
> if (ret < 0) {
> printf ("libfdt fdt_setprop(): %s\n",
> fdt_strerror(ret));
> @@ -766,7 +775,11 @@ static int fdt_parse_prop(char * const *newval, int
> count, char *data, int *len)
> cp = newp;
> tmp = simple_strtoul(cp, , 0);
>
> - *(fdt32_t *)data = cpu_to_fdt32(tmp);
> + if (*cp != '?')
> + *(fdt32_t *)data = cpu_to_fdt32(tmp);
> + else
> + newp++;
> +
>
> data += 4;
> *len += 4;
--
Regards,
Laurent Pinchart
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] Remove duplicate Spartan-3E definition.
Signed-off-by: Laurent Pinchart [EMAIL PROTECTED]
---
include/spartan3.h |9 -
1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/include/spartan3.h b/include/spartan3.h
index 235dbc0..2262278 100644
--- a/include/spartan3.h
+++ b/include/spartan3.h
@@ -81,9 +81,6 @@ typedef struct {
#defineXILINX_XC3S1200E_SIZE 3841184/8
#defineXILINX_XC3S1600E_SIZE 5969696/8
-/* Spartan-IIIE (1.2V) */
-#define XILINX_XC3S1200E_SIZE 3841184/8
-
/* Descriptor Macros
*/
/* Spartan-III devices */
@@ -111,7 +108,6 @@ typedef struct {
#define XILINX_XC3S5000_DESC(iface, fn_table, cookie) \
{ Xilinx_Spartan3, iface, XILINX_XC3S5000E_SIZE, fn_table, cookie }
-
/* Spartan-3E devices */
#define XILINX_XC3S100E_DESC(iface, fn_table, cookie) \
{ Xilinx_Spartan3, iface, XILINX_XC3S100E_SIZE, fn_table, cookie }
@@ -128,9 +124,4 @@ typedef struct {
#define XILINX_XC3S1600E_DESC(iface, fn_table, cookie) \
{ Xilinx_Spartan3, iface, XILINX_XC3S1600E_SIZE, fn_table, cookie }
-
-/* Spartan-IIIE devices */
-#define XILINX_XC3S1200E_DESC(iface, fn_table, cookie) \
-{ Xilinx_Spartan3, iface, XILINX_XC3S1200E_SIZE, fn_table, cookie }
-
#endif /* _SPARTAN3_H_ */
--
1.5.0
--
Laurent Pinchart
CSE Semaphore Belgium
Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium
T +32 (2) 387 42 59
F +32 (2) 387 42 75
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Fix Spartan-3 definitions.
A few Spartan-3 definitions erroneously use Spartan-3E size constants. This
patch fixes them.
Signed-off-by: Laurent Pinchart [EMAIL PROTECTED]
---
include/spartan3.h |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/spartan3.h b/include/spartan3.h
index c203eeb..235dbc0 100644
--- a/include/spartan3.h
+++ b/include/spartan3.h
@@ -103,13 +103,13 @@ typedef struct {
{ Xilinx_Spartan3, iface, XILINX_XC3S1500_SIZE, fn_table, cookie }
#define XILINX_XC3S2000_DESC(iface, fn_table, cookie) \
-{ Xilinx_Spartan3, iface, XILINX_XC3S2000E_SIZE, fn_table, cookie }
+{ Xilinx_Spartan3, iface, XILINX_XC3S2000_SIZE, fn_table, cookie }
#define XILINX_XC3S4000_DESC(iface, fn_table, cookie) \
-{ Xilinx_Spartan3, iface, XILINX_XC3S4000E_SIZE, fn_table, cookie }
+{ Xilinx_Spartan3, iface, XILINX_XC3S4000_SIZE, fn_table, cookie }
#define XILINX_XC3S5000_DESC(iface, fn_table, cookie) \
-{ Xilinx_Spartan3, iface, XILINX_XC3S5000E_SIZE, fn_table, cookie }
+{ Xilinx_Spartan3, iface, XILINX_XC3S5000_SIZE, fn_table, cookie }
/* Spartan-3E devices */
--
1.5.0
--
Laurent Pinchart
CSE Semaphore Belgium
Chaussee de Bruxelles, 732A
B-1410 Waterloo
Belgium
T +32 (2) 387 42 59
F +32 (2) 387 42 75
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
