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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Hrushikesh,

On Mon, Nov 03, 2025 at 11:10, Hrushikesh Salunke <h-salunke@ti.com> wrote:

> 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@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

I see. Sorry I missed that.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

I'll pick this up shortly to try to make it for v2026.01

>
> 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