[PATCH v4] spl: Add support for Device Firmware Upgrade (DFU) over PCIe

[Hrushikesh Salunke]

On 31/10/25 20:22, Mattijs Korpershoek wrote:
> Hi Hrushikesh,
> 
> Thank you for the patch.
> 
> v4 is in good shape already. I have a couple of small remarks below.
> Please have a look.
> 
> On Thu, Oct 23, 2025 at 13:39, Hrushikesh Salunke <h-salunke at ti.com> wrote:
> 
>> Introduces support for Device Firmware Upgrade (DFU) over PCIe in
>> U-Boot. Traditionally, the DFU protocol is used over USB, where a

[...]

>>   
>> +#ifdef CONFIG_SPL_PCI_DFU
>> +	if (spl_boot_device() == BOOT_DEVICE_PCIE)
>> +		return dfu_over_pcie();
>> +#endif
>> +
>>   	/* set default environment */
>>   	env_set_default(NULL, 0);
>>   	str_env = env_get(dfu_alt_info);
>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>> index 71b7a8374bb..0c501cf02f2 100644
>> --- a/common/spl/spl_ram.c
>> +++ b/common/spl/spl_ram.c
>> @@ -27,6 +27,11 @@ static ulong spl_ram_load_read(struct spl_load_info *load, ulong sector,
>>   	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
>>   		addr = IF_ENABLED_INT(CONFIG_SPL_LOAD_FIT,
>>   				      CONFIG_SPL_LOAD_FIT_ADDRESS);
>> +
>> +#ifdef	CONFIG_SPL_PCI_DFU
>> +		if (spl_boot_device() == BOOT_DEVICE_PCIE)
>> +			addr = CONFIG_SPL_PCI_DFU_SPL_LOAD_FIT_ADDRESS;
>> +#endif
> 
> Can't we rewrite this using IF_ENABLED_INT() similarly to what has been
> done for CONFIG_SPL_LOAD_FIT_ADDRESS ?
> 
> This should allow us to get rid of the #ifdef CONFIG_SPL_PCI_DFU part.
> 
>>   	}
>>   	addr += sector;
>>   	if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD))
>> @@ -47,6 +52,11 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
>>   	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
>>   		addr = IF_ENABLED_INT(CONFIG_SPL_LOAD_FIT,
>>   				      CONFIG_SPL_LOAD_FIT_ADDRESS);
>> +
>> +#ifdef CONFIG_SPL_PCI_DFU
>> +		if (spl_boot_device() == BOOT_DEVICE_PCIE)
>> +			addr = CONFIG_SPL_PCI_DFU_SPL_LOAD_FIT_ADDRESS;
>> +#endif
> 
> Same here
> 
>>   	}
>>   
>>   	if (CONFIG_IS_ENABLED(IMAGE_PRE_LOAD)) {
>> @@ -64,6 +74,11 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
>>   		spl_dfu_cmd(0, "dfu_alt_info_ram", "ram", "0");
>>   #endif
>>   
>> +#if CONFIG_IS_ENABLED(PCI_DFU)
>> +	if (bootdev->boot_device == BOOT_DEVICE_PCIE)
>> +		spl_dfu_cmd(0, "dfu_alt_info_ram", "ram", "0");
>> +#endif
>> +
> 
> Same here. Please rewrite using if (IS_ENABLED(CONFIG_SPL_PCI_DFU)).
> This way we are consistent with the rest of the file.
> 


Thank you for the feedback. However, using IS_ENABLED() instead of
#ifdef in these locations causes compilation errors on platforms where
BOOT_DEVICE_PCIE is not defined. The BOOT_DEVICE_PCIE macro is only
defined in platform-specific headers, and is not available universally
across all architectures. With IS_ENABLED(CONFIG_SPL_PCI_DFU), the code
inside the conditional is still visible to the compiler even when the
config is disabled. This means the compiler will try to evaluate
BOOT_DEVICE_PCIE on platforms where it doesn't exist, resulting in a
compilation error like:

error: 'BOOT_DEVICE_PCIE' undeclared

The #ifdef CONFIG_SPL_PCI_DFU approach ensures that the code referencing
BOOT_DEVICE_PCIE is completely removed during preprocessing on platforms
that don't support PCIe DFU, preventing the compilation error. This is
similar to how other boot device checks are handled in U-Boot when the
boot device macro may not be universally defined across all platforms.

Regards,
Hrushikesh.



>>   	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>>   	    image_get_magic(header) == FDT_MAGIC) {
>>   		struct spl_load_info load;
>> @@ -102,3 +117,6 @@ SPL_LOAD_IMAGE_METHOD("RAM", 0, BOOT_DEVICE_RAM, spl_ram_load_image);
>>   #if CONFIG_IS_ENABLED(DFU)
>>   SPL_LOAD_IMAGE_METHOD("DFU", 0, BOOT_DEVICE_DFU, spl_ram_load_image);
>>   #endif
>> +#if CONFIG_IS_ENABLED(PCI_DFU)
>> +SPL_LOAD_IMAGE_METHOD("PCIE", 0, BOOT_DEVICE_PCIE, spl_ram_load_image);
>> +#endif
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 7560008a842..77fc66eb8ba 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_MULTIPLEXER) += mux/
>>   obj-$(CONFIG_$(PHASE_)ETH) += net/
>>   obj-$(CONFIG_$(PHASE_)PCH) += pch/
>>   obj-$(CONFIG_$(PHASE_)PCI) += pci/
>> +obj-$(CONFIG_$(PHASE_)PCI_ENDPOINT) += pci_endpoint/
>>   obj-$(CONFIG_$(PHASE_)PHY) += phy/
>>   obj-$(CONFIG_$(PHASE_)PINCTRL) += pinctrl/
>>   obj-$(CONFIG_$(PHASE_)POWER) += power/
>> -- 
>> 2.34.1