[PATCH 1/4] scripts/make_fit: factor dtb file name into configuration name

[PATCH 1/4] scripts/make_fit: factor dtb file name into configuration name

[Ahmad Fatoum]

Bootloaders often support booting a specific FIT configuration by name.
This is useful when the automatic boot fails, because the bootloader's
DT compatible differs from the upstream one included with the Linux
device trees.

Currently, configuration are numbered sequentially requiring the user to
dump the FIT image to determine what number the configuration has.

Improve upon this by naming configurations after the dtb name instead.
This is what OE-core kernel-fitimage.bbclass does for example.

Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 scripts/make_fit.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/scripts/make_fit.py b/scripts/make_fit.py
index 075b7c258ff2..2190da4c003c 100755
--- a/scripts/make_fit.py
+++ b/scripts/make_fit.py
@@ -145,11 +145,9 @@ def finish_fit(fsw, entries):
             str: Compatible stringlist
     """
     fsw.end_node()
-    seq = 0
     with fsw.add_node('configurations'):
-        for model, compat, files in entries:
-            seq += 1
-            with fsw.add_node(f'conf-{seq}'):
+        for dtbname, model, compat, files in entries:
+            with fsw.add_node(f'conf-{dtbname}'):
                 fsw.property('compatible', bytes(compat))
                 fsw.property_string('description', model)
                 fsw.property('fdt', bytes(''.join(f'fdt-{x}\x00' for x in files), "ascii"))
@@ -266,6 +264,7 @@ def build_fit(args):
     fsw = libfdt.FdtSw()
     setup_fit(fsw, args.name)
     entries = []
+    dtbs_seen = set()
     fdts = {}
 
     # Handle the kernel
@@ -290,7 +289,13 @@ def build_fit(args):
 
         files_seq = [fdts[fn] for fn in files]
 
-        entries.append([model, compat, files_seq])
+        dtbname = os.path.basename(fname)
+        ndtbs_seen = len(dtbs_seen)
+        dtbs_seen.add(dtbname)
+        if len(dtbs_seen) == ndtbs_seen:
+            raise RuntimeError(f"Duplicate file name '{dtbname}' during FIT creation")
+
+        entries.append([dtbname, model, compat, files_seq])
 
     finish_fit(fsw, entries)
 
-- 
2.39.5

[PATCH 2/4] amba: drop unused hepers for creating AMBA devices outside OF

[Ahmad Fatoum]

All amba devices created for upstream boards are instantiated from
device tree. The headers have functions that can be used from board
code, but are unused, so let's drop them and make AMBA OF-only.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/amba/bus.c       | 21 ---------------------
 include/linux/amba/bus.h | 23 -----------------------
 2 files changed, 44 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index cf43aaa76ea1..6463366fafc5 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -162,27 +162,6 @@ int amba_device_add(struct amba_device *dev)
 	return ret;
 }
 
-struct amba_device *
-amba_aphb_device_add(struct device *parent, const char *name, int id,
-		     resource_size_t base, size_t size,
-		     void *pdata, unsigned int periphid)
-{
-	struct amba_device *dev;
-	int ret;
-
-	dev = amba_device_alloc(name, id, base, size);
-
-	dev->periphid = periphid;
-	dev->dev.platform_data = pdata;
-	dev->dev.parent = parent;
-
-	ret = amba_device_add(dev);
-	if (ret)
-		return ERR_PTR(ret);
-
-	return dev;
-}
-
 /**
  *	amba_device_alloc - allocate an AMBA device
  *	@name: sysfs name of the AMBA device
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index cc24b38e8300..7cbc4e653eac 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -74,29 +74,6 @@ void amba_device_put(struct amba_device *);
 int amba_device_add(struct amba_device *);
 int amba_device_register(struct amba_device *, struct resource *);
 
-struct amba_device *
-amba_aphb_device_add(struct device *parent, const char *name, int id,
-		     resource_size_t base, size_t size,
-		     void *pdata, unsigned int periphid);
-
-static inline struct amba_device *
-amba_apb_device_add(struct device *parent, const char *name, int id,
-		    resource_size_t base, size_t size,
-		    void *pdata, unsigned int periphid)
-{
-	return amba_aphb_device_add(parent, name, id, base, size, pdata,
-				    periphid);
-}
-
-static inline struct amba_device *
-amba_ahb_device_add(struct device *parent, const char *name, int id,
-		    resource_size_t base, size_t size,
-		    void *pdata, unsigned int periphid)
-{
-	return amba_aphb_device_add(parent, name, id, base, size, pdata,
-				    periphid);
-}
-
 static inline void __iomem *amba_get_mem_region(struct amba_device *dev)
 {
 	return dev->base;
-- 
2.39.5

Re: [PATCH 2/4] amba: drop unused hepers for creating AMBA devices outside OF

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 62 bytes --]

Hello Ahmad,

$Subject ~= s/hepers/helpers/

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/4] amba: support masking data abort during identification

[Ahmad Fatoum]

Registration of an AMBA device requires reading its pid and cid
registers after turning on the device. This is the first I/O done to the
device and may end up crashing if the device tree doesn't describe
the correct clock and power domains or if a previous boot stage
should've enabled them, but didn't.

This is not a likely issue, but when it happens it crashes the system
annoyingly early during device tree populating.

Add a symbol that masks the data abort exception around the
identification code to turn the crash into an error message instead.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/amba/Kconfig | 19 +++++++++++++++++++
 drivers/amba/bus.c   | 12 +++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/amba/Kconfig b/drivers/amba/Kconfig
index 444d4ce7435c..be5e053f4b84 100644
--- a/drivers/amba/Kconfig
+++ b/drivers/amba/Kconfig
@@ -1,3 +1,22 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config ARM_AMBA
 	bool
+
+config ARM_AMBA_DABT_MASK
+	bool
+	prompt "Mask data abort while identifying AMBA devices" if COMPILE_TEST
+	depends on ARM_AMBA && ARCH_HAS_DATA_ABORT_MASK
+	help
+	  Registration of an AMBA device requires reading its pid and cid
+	  registers after turning on the device, which can involve enabling
+	  clocks, deasserting resets and powering on power domains.
+	  In some cases, these resources are not available to barebox running
+	  in the normal world and thus need to be setup by a previous
+	  stage bootloader running in the secure world.
+	  If such setup doesn't happen, accessing devices may lead to
+	  a data abort or complete system hang.
+
+	  As a workaround for the former case, this option can be enabled
+	  to gracefully handle data aborts during readout of the PID/CID
+	  registers. The proper solution is fixing your first stage
+	  bootloader or allow barebox access to the missing resources.
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 6463366fafc5..5ed8336e6124 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -10,6 +10,7 @@
 #include <linux/amba/bus.h>
 #include <io.h>
 #include <init.h>
+#include <abort.h>
 
 #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
 
@@ -132,6 +133,9 @@ int amba_device_add(struct amba_device *dev)
 	if (ret == 0) {
 		u32 pid, cid;
 
+		if (IS_ENABLED(CONFIG_ARM_AMBA_DABT_MASK))
+			data_abort_mask();
+
 		/*
 		 * Read pid and cid based on size of resource
 		 * they are located at end of region
@@ -139,8 +143,14 @@ int amba_device_add(struct amba_device *dev)
 		pid = amba_device_get_pid(tmp, size);
 		cid = amba_device_get_cid(tmp, size);
 
-		if (cid == AMBA_CID)
+		if (IS_ENABLED(CONFIG_ARM_AMBA_DABT_MASK) && data_abort_unmask()) {
+			dev_err(amba_bustype.dev,
+				"data abort during MMIO read of PID/CID for %pOF\n",
+				dev->dev.of_node);
+			ret = -EFAULT;
+		} else if (cid == AMBA_CID) {
 			dev->periphid = pid;
+		}
 
 		if (!dev->periphid)
 			ret = -ENODEV;
-- 
2.39.5

Re: [PATCH 3/4] amba: support masking data abort during identification

[Sascha Hauer]

On Tue, Mar 18, 2025 at 09:25:06AM +0100, Ahmad Fatoum wrote:
> Registration of an AMBA device requires reading its pid and cid
> registers after turning on the device. This is the first I/O done to the
> device and may end up crashing if the device tree doesn't describe
> the correct clock and power domains or if a previous boot stage
> should've enabled them, but didn't.
> 
> This is not a likely issue, but when it happens it crashes the system
> annoyingly early during device tree populating.
> 
> Add a symbol that masks the data abort exception around the
> identification code to turn the crash into an error message instead.

Do we need to make this optional? I could imagine I find out about the
existence of this option only after having traced the issue down to the
exact lines of code in which case I won't need the option anymore.

Sascha

> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/amba/Kconfig | 19 +++++++++++++++++++
>  drivers/amba/bus.c   | 12 +++++++++++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/amba/Kconfig b/drivers/amba/Kconfig
> index 444d4ce7435c..be5e053f4b84 100644
> --- a/drivers/amba/Kconfig
> +++ b/drivers/amba/Kconfig
> @@ -1,3 +1,22 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config ARM_AMBA
>  	bool
> +
> +config ARM_AMBA_DABT_MASK
> +	bool
> +	prompt "Mask data abort while identifying AMBA devices" if COMPILE_TEST
> +	depends on ARM_AMBA && ARCH_HAS_DATA_ABORT_MASK
> +	help
> +	  Registration of an AMBA device requires reading its pid and cid
> +	  registers after turning on the device, which can involve enabling
> +	  clocks, deasserting resets and powering on power domains.
> +	  In some cases, these resources are not available to barebox running
> +	  in the normal world and thus need to be setup by a previous
> +	  stage bootloader running in the secure world.
> +	  If such setup doesn't happen, accessing devices may lead to
> +	  a data abort or complete system hang.
> +
> +	  As a workaround for the former case, this option can be enabled
> +	  to gracefully handle data aborts during readout of the PID/CID
> +	  registers. The proper solution is fixing your first stage
> +	  bootloader or allow barebox access to the missing resources.
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 6463366fafc5..5ed8336e6124 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -10,6 +10,7 @@
>  #include <linux/amba/bus.h>
>  #include <io.h>
>  #include <init.h>
> +#include <abort.h>
>  
>  #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
>  
> @@ -132,6 +133,9 @@ int amba_device_add(struct amba_device *dev)
>  	if (ret == 0) {
>  		u32 pid, cid;
>  
> +		if (IS_ENABLED(CONFIG_ARM_AMBA_DABT_MASK))
> +			data_abort_mask();
> +
>  		/*
>  		 * Read pid and cid based on size of resource
>  		 * they are located at end of region
> @@ -139,8 +143,14 @@ int amba_device_add(struct amba_device *dev)
>  		pid = amba_device_get_pid(tmp, size);
>  		cid = amba_device_get_cid(tmp, size);
>  
> -		if (cid == AMBA_CID)
> +		if (IS_ENABLED(CONFIG_ARM_AMBA_DABT_MASK) && data_abort_unmask()) {
> +			dev_err(amba_bustype.dev,
> +				"data abort during MMIO read of PID/CID for %pOF\n",
> +				dev->dev.of_node);
> +			ret = -EFAULT;
> +		} else if (cid == AMBA_CID) {
>  			dev->periphid = pid;
> +		}
>  
>  		if (!dev->periphid)
>  			ret = -ENODEV;
> -- 
> 2.39.5
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] amba: support masking data abort during identification

[Ahmad Fatoum]

Hello Sascha,

On 3/18/25 12:04, Sascha Hauer wrote:
> On Tue, Mar 18, 2025 at 09:25:06AM +0100, Ahmad Fatoum wrote:
>> Registration of an AMBA device requires reading its pid and cid
>> registers after turning on the device. This is the first I/O done to the
>> device and may end up crashing if the device tree doesn't describe
>> the correct clock and power domains or if a previous boot stage
>> should've enabled them, but didn't.
>>
>> This is not a likely issue, but when it happens it crashes the system
>> annoyingly early during device tree populating.
>>
>> Add a symbol that masks the data abort exception around the
>> identification code to turn the crash into an error message instead.
> 
> Do we need to make this optional? I could imagine I find out about the
> existence of this option only after having traced the issue down to the
> exact lines of code in which case I won't need the option anymore.

If it turns out to be needed for more platforms, we can consider
making it visible by default, but currently I think its of
rather limited use case.

Another way would be disabling the node in the barebox DT,
but I don't want to the Qemu virt DTs to diverge.

Cheers,
Ahmad

> 
> Sascha
> 
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/amba/Kconfig | 19 +++++++++++++++++++
>>  drivers/amba/bus.c   | 12 +++++++++++-
>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/amba/Kconfig b/drivers/amba/Kconfig
>> index 444d4ce7435c..be5e053f4b84 100644
>> --- a/drivers/amba/Kconfig
>> +++ b/drivers/amba/Kconfig
>> @@ -1,3 +1,22 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  config ARM_AMBA
>>  	bool
>> +
>> +config ARM_AMBA_DABT_MASK
>> +	bool
>> +	prompt "Mask data abort while identifying AMBA devices" if COMPILE_TEST
>> +	depends on ARM_AMBA && ARCH_HAS_DATA_ABORT_MASK
>> +	help
>> +	  Registration of an AMBA device requires reading its pid and cid
>> +	  registers after turning on the device, which can involve enabling
>> +	  clocks, deasserting resets and powering on power domains.
>> +	  In some cases, these resources are not available to barebox running
>> +	  in the normal world and thus need to be setup by a previous
>> +	  stage bootloader running in the secure world.
>> +	  If such setup doesn't happen, accessing devices may lead to
>> +	  a data abort or complete system hang.
>> +
>> +	  As a workaround for the former case, this option can be enabled
>> +	  to gracefully handle data aborts during readout of the PID/CID
>> +	  registers. The proper solution is fixing your first stage
>> +	  bootloader or allow barebox access to the missing resources.
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index 6463366fafc5..5ed8336e6124 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/amba/bus.h>
>>  #include <io.h>
>>  #include <init.h>
>> +#include <abort.h>
>>  
>>  #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
>>  
>> @@ -132,6 +133,9 @@ int amba_device_add(struct amba_device *dev)
>>  	if (ret == 0) {
>>  		u32 pid, cid;
>>  
>> +		if (IS_ENABLED(CONFIG_ARM_AMBA_DABT_MASK))
>> +			data_abort_mask();
>> +
>>  		/*
>>  		 * Read pid and cid based on size of resource
>>  		 * they are located at end of region
>> @@ -139,8 +143,14 @@ int amba_device_add(struct amba_device *dev)
>>  		pid = amba_device_get_pid(tmp, size);
>>  		cid = amba_device_get_cid(tmp, size);
>>  
>> -		if (cid == AMBA_CID)
>> +		if (IS_ENABLED(CONFIG_ARM_AMBA_DABT_MASK) && data_abort_unmask()) {
>> +			dev_err(amba_bustype.dev,
>> +				"data abort during MMIO read of PID/CID for %pOF\n",
>> +				dev->dev.of_node);
>> +			ret = -EFAULT;
>> +		} else if (cid == AMBA_CID) {
>>  			dev->periphid = pid;
>> +		}
>>  
>>  		if (!dev->periphid)
>>  			ret = -ENODEV;
>> -- 
>> 2.39.5
>>
>>
>>
> 

[PATCH 4/4] ARM: qemu: mask data aborts during AMBA identification

[Ahmad Fatoum]

The barebox FIT image target is meant as an easy way to load barebox
second stage from other bootloaders.

Doing so with U-Boot 2025.01 compiled for qemu_arm64_defconfig
currently leads to a crash when accessing the PL061 GPIO controller
device for identification.

It seems that U-Boot (or TF-A, which wasn't included in my setup)
should have done some initial setup to allow access to the PL061.

barebox doesn't even have a driver for the peripheral and loading
barebox directly with -kernel works. In both cases, barebox runs in EL1.

Let's thus workaround the issue on the barebox side by using the new
CONFIG_ARM_AMBA_DABT_MASK option to turn the crash into an error
message.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index f5f9f3828782..7d46dcfb63dc 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -282,6 +282,7 @@ config BOARD_ARM_VIRT
 	select BOARD_GENERIC_DT
 	select BOARD_QEMU_VIRT
 	select OF_OVERLAY
+	select ARM_AMBA_DABT_MASK
 
 config BOARD_ARM_GENERIC_DT
 	def_bool BOARD_GENERIC_DT
-- 
2.39.5

Re: [PATCH 4/4] ARM: qemu: mask data aborts during AMBA identification

[Sascha Hauer]

On Tue, Mar 18, 2025 at 09:25:07AM +0100, Ahmad Fatoum wrote:
> The barebox FIT image target is meant as an easy way to load barebox
> second stage from other bootloaders.
> 
> Doing so with U-Boot 2025.01 compiled for qemu_arm64_defconfig
> currently leads to a crash when accessing the PL061 GPIO controller
> device for identification.
> 
> It seems that U-Boot (or TF-A, which wasn't included in my setup)
> should have done some initial setup to allow access to the PL061.
> 
> barebox doesn't even have a driver for the peripheral and loading
> barebox directly with -kernel works. In both cases, barebox runs in EL1.
> 
> Let's thus workaround the issue on the barebox side by using the new
> CONFIG_ARM_AMBA_DABT_MASK option to turn the crash into an error
> message.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index f5f9f3828782..7d46dcfb63dc 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -282,6 +282,7 @@ config BOARD_ARM_VIRT
>  	select BOARD_GENERIC_DT
>  	select BOARD_QEMU_VIRT
>  	select OF_OVERLAY
> +	select ARM_AMBA_DABT_MASK

Ah, ok. You select the option where you anticipate that it's useful.
Should have read this patch before commenting on #3.

Fine with me then.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/4] scripts/make_fit: factor dtb file name into configuration name

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]

Hello Ahmad,

On Tue, Mar 18, 2025 at 09:25:04AM +0100, Ahmad Fatoum wrote:
> Bootloaders often support booting a specific FIT configuration by name.
> This is useful when the automatic boot fails, because the bootloader's
> DT compatible differs from the upstream one included with the Linux
> device trees.
> 
> Currently, configuration are numbered sequentially requiring the user to
> dump the FIT image to determine what number the configuration has.
> 
> Improve upon this by naming configurations after the dtb name instead.
> This is what OE-core kernel-fitimage.bbclass does for example.
> 
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  scripts/make_fit.py | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> index 075b7c258ff2..2190da4c003c 100755
> --- a/scripts/make_fit.py
> +++ b/scripts/make_fit.py
> @@ -145,11 +145,9 @@ def finish_fit(fsw, entries):
>              str: Compatible stringlist
>      """
>      fsw.end_node()
> -    seq = 0
>      with fsw.add_node('configurations'):
> -        for model, compat, files in entries:
> -            seq += 1
> -            with fsw.add_node(f'conf-{seq}'):
> +        for dtbname, model, compat, files in entries:
> +            with fsw.add_node(f'conf-{dtbname}'):

Funny/surprising semantic of fsw.add_node (i.e. fsw refers to the new
node in the with-body?)

>                  fsw.property('compatible', bytes(compat))
>                  fsw.property_string('description', model)
>                  fsw.property('fdt', bytes(''.join(f'fdt-{x}\x00' for x in files), "ascii"))
> @@ -266,6 +264,7 @@ def build_fit(args):
>      fsw = libfdt.FdtSw()
>      setup_fit(fsw, args.name)
>      entries = []
> +    dtbs_seen = set()
>      fdts = {}
>  
>      # Handle the kernel
> @@ -290,7 +289,13 @@ def build_fit(args):
>  
>          files_seq = [fdts[fn] for fn in files]
>  
> -        entries.append([model, compat, files_seq])
> +        dtbname = os.path.basename(fname)
> +        ndtbs_seen = len(dtbs_seen)
> +        dtbs_seen.add(dtbname)
> +        if len(dtbs_seen) == ndtbs_seen:
> +            raise RuntimeError(f"Duplicate file name '{dtbname}' during FIT creation")
> +
> +        entries.append([dtbname, model, compat, files_seq])

	dtbname = os.path.basename(fname)
	if dtbname in dtbs_seen:
	    raise RuntimeError(...)
	dtbs_seen.add(dtbname)
	entries.append(...)

looks more pythonic (to me).

Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>

for the idea of this patch.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] scripts/make_fit: factor dtb file name into configuration name

[Ahmad Fatoum]

Hello Uwe,

On 3/18/25 12:03, Uwe Kleine-König wrote:
> On Tue, Mar 18, 2025 at 09:25:04AM +0100, Ahmad Fatoum wrote:
>>      fsw.end_node()
>> -    seq = 0
>>      with fsw.add_node('configurations'):
>> -        for model, compat, files in entries:
>> -            seq += 1
>> -            with fsw.add_node(f'conf-{seq}'):
>> +        for dtbname, model, compat, files in entries:
>> +            with fsw.add_node(f'conf-{dtbname}'):
> 
> Funny/surprising semantic of fsw.add_node (i.e. fsw refers to the new
> node in the with-body?)

Yes. I suspect it's to ensure that a node end tag is inserted
after the properties.

>> -        entries.append([model, compat, files_seq])
>> +        dtbname = os.path.basename(fname)
>> +        ndtbs_seen = len(dtbs_seen)
>> +        dtbs_seen.add(dtbname)
>> +        if len(dtbs_seen) == ndtbs_seen:
>> +            raise RuntimeError(f"Duplicate file name '{dtbname}' during FIT creation")
>> +
>> +        entries.append([dtbname, model, compat, files_seq])
> 
> 	dtbname = os.path.basename(fname)
> 	if dtbname in dtbs_seen:
> 	    raise RuntimeError(...)
> 	dtbs_seen.add(dtbname)
> 	entries.append(...)
> 
> looks more pythonic (to me).

I dislike iterating twice over the dtbs. I wrote it this way,
because I assumed calling len on the set is O(1).

> Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> 
> for the idea of this patch.

Thanks,
Ahmad

> 
> Best regards
> Uwe

Re: [PATCH 1/4] scripts/make_fit: factor dtb file name into configuration name

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

Hello,

On Tue, Mar 18, 2025 at 12:27:26PM +0100, Ahmad Fatoum wrote:
> Hello Uwe,
> 
> On 3/18/25 12:03, Uwe Kleine-König wrote:
> > On Tue, Mar 18, 2025 at 09:25:04AM +0100, Ahmad Fatoum wrote:
> >>      fsw.end_node()
> >> -    seq = 0
> >>      with fsw.add_node('configurations'):
> >> -        for model, compat, files in entries:
> >> -            seq += 1
> >> -            with fsw.add_node(f'conf-{seq}'):
> >> +        for dtbname, model, compat, files in entries:
> >> +            with fsw.add_node(f'conf-{dtbname}'):
> > 
> > Funny/surprising semantic of fsw.add_node (i.e. fsw refers to the new
> > node in the with-body?)
> 
> Yes. I suspect it's to ensure that a node end tag is inserted
> after the properties.
> 
> >> -        entries.append([model, compat, files_seq])
> >> +        dtbname = os.path.basename(fname)
> >> +        ndtbs_seen = len(dtbs_seen)
> >> +        dtbs_seen.add(dtbname)
> >> +        if len(dtbs_seen) == ndtbs_seen:
> >> +            raise RuntimeError(f"Duplicate file name '{dtbname}' during FIT creation")
> >> +
> >> +        entries.append([dtbname, model, compat, files_seq])
> > 
> > 	dtbname = os.path.basename(fname)
> > 	if dtbname in dtbs_seen:
> > 	    raise RuntimeError(...)
> > 	dtbs_seen.add(dtbname)
> > 	entries.append(...)
> > 
> > looks more pythonic (to me).
> 
> I dislike iterating twice over the dtbs. I wrote it this way,
> because I assumed calling len on the set is O(1).

I would expect that both `dtbname in dtbs_seen` and
`dtbs_seen.add(dtbname)` is O(1) (in the absense of hash collisions).

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] scripts/make_fit: factor dtb file name into configuration name

[Sascha Hauer]

On Tue, 18 Mar 2025 09:25:04 +0100, Ahmad Fatoum wrote:
> Bootloaders often support booting a specific FIT configuration by name.
> This is useful when the automatic boot fails, because the bootloader's
> DT compatible differs from the upstream one included with the Linux
> device trees.
> 
> Currently, configuration are numbered sequentially requiring the user to
> dump the FIT image to determine what number the configuration has.
> 
> [...]

Applied, thanks!

[1/4] scripts/make_fit: factor dtb file name into configuration name
      https://git.pengutronix.de/cgit/barebox/commit/?id=852e75a48dc7 (link may not be stable)
[2/4] amba: drop unused hepers for creating AMBA devices outside OF
      https://git.pengutronix.de/cgit/barebox/commit/?id=b12430033d05 (link may not be stable)
[3/4] amba: support masking data abort during identification
      https://git.pengutronix.de/cgit/barebox/commit/?id=c1395964b07f (link may not be stable)
[4/4] ARM: qemu: mask data aborts during AMBA identification
      https://git.pengutronix.de/cgit/barebox/commit/?id=2db7c84a8d0b (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>