[PATCH 0/4] Add support for USB DFU boot on AM62Px

[PATCH 0/4] Add support for USB DFU boot on AM62Px

[Siddharth Vadapalli]

Hello,

This series add support for USB DFU boot on TI's AM62Px SoC which has
two instances of USB Controllers, of which the USB0 instance of USB
which is a DWC3 USB Controller is used for USB DFU Boot:
ROM => tiboot3.bin => tispl.bin => u-boot.img

The first patch of this series is based on the suggestion of
Jonathan Humphreys <j-humpreys@ti.com>
at:
https://patchwork.ozlabs.org/project/uboot/cover/20241124070828.617558-1-s-vadapalli@ti.com/#3419733
which prevents overwriting the "dfu_alt_info" environment variable which
is set by "spl_dfu_cmd()" for USB DFU boot.

The second patch adds USB DFU environment for AM62Px.

The third patch extends "am62x_r5_usbdfu.config" config fragment for
being re-used by AM62Px SoC by dropping configs which aren't required
for USB DFU boot. This is necessary to reduce the size of R5 SPL
(tiboot3.bin) built for USB DFU boot.

The fourth patch enables USB device-tree nodes for USB DFU functionality
on AM62Px. Since CONFIG_OF_UPSTREAM is not yet supported for AM62Px, the
changes are being made in arch/arm/dts/k3-am62p5-sk-u-boot.dtsi at the
moment, with the intent of making the same changes in the Linux
device-tree and eventually enabling CONFIG_OF_UPSTREAM to drop the
changes made in "k3-am62p5-sk-u-boot.dtsi".

Series is based on commit
3b3c7280b82 smbios: address build warning
of the next branch of U-Boot.

The USB DFU config fragments for AM62x are to be re-used for enabling
USB DFU boot on AM62Px as follows:
R5  => am62px_evm_r5_defconfig + am62x_r5_usbdfu.config
A53 => am62px_evm_a53_defconfig + am62x_a53_usbdfu.config

Series has been tested on AM62P5-SK. Logs validating USB DFU boot on
AM62P5-SK with this series:
https://gist.github.com/Siddharth-Vadapalli-at-TI/50198ee3348c2db73e0dbd3a14cb0f8b

Series has also been validated on AM625-SK to ensure that the changes
made to "am62x_r5_usbdfu.config" config fragment being used by AM625-SK
for USB DFU boot do not cause a regression on AM625-SK w.r.t. USB DFU
boot. Logs validating USB DFU Boot on AM625-SK with this series:
https://gist.github.com/Siddharth-Vadapalli-at-TI/85a7579ed2c95330ac71ee3a832cadfb

Regards,
Siddharth.

Jonathan Humphreys (1):
  dfu: do not overwrite "dfu_alt_info"

Siddharth Vadapalli (3):
  board: ti: am62px: env: include environment for DFU Boot
  configs: am62x_r5_usbdfu: extend for AM62Px
  arm: dts: k3-am62p5-sk-u-boot: enable USB0 for USB DFU boot

 arch/arm/dts/k3-am62p5-sk-u-boot.dtsi | 10 ++++++++++
 board/ti/am62px/am62px.env            |  5 +++--
 configs/am62x_r5_usbdfu.config        |  3 +++
 drivers/dfu/dfu.c                     |  7 +++++--
 4 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 1/4] dfu: do not overwrite "dfu_alt_info"

[Siddharth Vadapalli]

From: Jonathan Humphreys <j-humphreys@ti.com>

For use-cases such as USB DFU Boot, "spl_dfu_cmd()" will setup the
appropriate value for "dfu_alt_info". To facilitate such use-cases and
in order to avoid overwriting the value of "dfu_alt_info", invoke the
"set_dfu_alt_info()" function only when "dfu_alt_info" is not defined.

Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/dfu/dfu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 756569217bb..ab8abae1d89 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr)
 	dfu_reinit_needed = false;
 	dfu_alt_info_changed = false;
 
+	str_env = env_get("dfu_alt_info");
 #ifdef CONFIG_SET_DFU_ALT_INFO
-	set_dfu_alt_info(interface, devstr);
+	if (!str_env) {
+		set_dfu_alt_info(interface, devstr);
+		str_env = env_get("dfu_alt_info");
+	}
 #endif
-	str_env = env_get("dfu_alt_info");
 	if (!str_env) {
 		pr_err("\"dfu_alt_info\" env variable not defined!\n");
 		return -EINVAL;
-- 
2.43.0

Re: [PATCH 1/4] dfu: do not overwrite "dfu_alt_info"

[Jon Humphreys]

Siddharth Vadapalli <s-vadapalli@ti.com> writes:

> From: Jonathan Humphreys <j-humphreys@ti.com>
>
> For use-cases such as USB DFU Boot, "spl_dfu_cmd()" will setup the
> appropriate value for "dfu_alt_info". To facilitate such use-cases and
> in order to avoid overwriting the value of "dfu_alt_info", invoke the
> "set_dfu_alt_info()" function only when "dfu_alt_info" is not defined.
>
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/dfu/dfu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 756569217bb..ab8abae1d89 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr)
>  	dfu_reinit_needed = false;
>  	dfu_alt_info_changed = false;
>  
> +	str_env = env_get("dfu_alt_info");
>  #ifdef CONFIG_SET_DFU_ALT_INFO
> -	set_dfu_alt_info(interface, devstr);
> +	if (!str_env) {
> +		set_dfu_alt_info(interface, devstr);
> +		str_env = env_get("dfu_alt_info");
> +	}
>  #endif
> -	str_env = env_get("dfu_alt_info");
>  	if (!str_env) {
>  		pr_err("\"dfu_alt_info\" env variable not defined!\n");
>  		return -EINVAL;
> -- 
> 2.43.0

Hi Siddharth, thanks. I went ahead and just posted a separate patch to fix
this issue. I added a bit more explanation, and wanted it separate so that
it can hopefully make the 2025.01 release.

See https://lore.kernel.org/r/20241217204835.3312765-1-j-humphreys@ti.com.

Thanks
Jon

Re: [PATCH 1/4] dfu: do not overwrite "dfu_alt_info"

[Siddharth Vadapalli]

On Tue, Dec 17, 2024 at 02:55:13PM -0600, Jon Humphreys wrote:
> Siddharth Vadapalli <s-vadapalli@ti.com> writes:
> 
> > From: Jonathan Humphreys <j-humphreys@ti.com>
> >
> > For use-cases such as USB DFU Boot, "spl_dfu_cmd()" will setup the
> > appropriate value for "dfu_alt_info". To facilitate such use-cases and
> > in order to avoid overwriting the value of "dfu_alt_info", invoke the
> > "set_dfu_alt_info()" function only when "dfu_alt_info" is not defined.
> >
> > Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> >  drivers/dfu/dfu.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> > index 756569217bb..ab8abae1d89 100644
> > --- a/drivers/dfu/dfu.c
> > +++ b/drivers/dfu/dfu.c
> > @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr)
> >  	dfu_reinit_needed = false;
> >  	dfu_alt_info_changed = false;
> >  
> > +	str_env = env_get("dfu_alt_info");
> >  #ifdef CONFIG_SET_DFU_ALT_INFO
> > -	set_dfu_alt_info(interface, devstr);
> > +	if (!str_env) {
> > +		set_dfu_alt_info(interface, devstr);
> > +		str_env = env_get("dfu_alt_info");
> > +	}
> >  #endif
> > -	str_env = env_get("dfu_alt_info");
> >  	if (!str_env) {
> >  		pr_err("\"dfu_alt_info\" env variable not defined!\n");
> >  		return -EINVAL;
> > -- 
> > 2.43.0
> 
> Hi Siddharth, thanks. I went ahead and just posted a separate patch to fix
> this issue. I added a bit more explanation, and wanted it separate so that
> it can hopefully make the 2025.01 release.
> 
> See https://lore.kernel.org/r/20241217204835.3312765-1-j-humphreys@ti.com.

Thank you Jon. I will drop this patch in the v2 series. I have reviewed
your patch and shared feedback.

Regards,
Siddharth.

[PATCH 2/4] board: ti: am62px: env: include environment for DFU Boot

[Siddharth Vadapalli]

Include the TI K3 DFU environment to support DFU Boot and DFU Flash.
Also add "usb" to the list of "boot_targets".

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 board/ti/am62px/am62px.env | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/board/ti/am62px/am62px.env b/board/ti/am62px/am62px.env
index 7ef54079aa8..e0838196e3a 100644
--- a/board/ti/am62px/am62px.env
+++ b/board/ti/am62px/am62px.env
@@ -1,5 +1,6 @@
 #include <env/ti/ti_common.env>
 #include <env/ti/mmc.env>
+#include <env/ti/k3_dfu.env>
 
 name_kern=Image
 console=ttyS2,115200n8
@@ -7,7 +8,7 @@ args_all=setenv optargs ${optargs} earlycon=ns16550a,mmio32,0x02800000
 	${mtdparts}
 run_kern=booti ${loadaddr} ${rd_spec} ${fdtaddr}
 
-boot_targets=mmc1 mmc0 pxe dhcp
+boot_targets=mmc1 mmc0 usb pxe dhcp
 boot=mmc
 mmcdev=1
 bootpart=1:2
@@ -17,4 +18,4 @@ rd_spec=-
 #if CONFIG_BOOTMETH_ANDROID
 #include <env/ti/android.env>
 adtb_idx=3
-#endif
\ No newline at end of file
+#endif
-- 
2.43.0

Re: [PATCH 2/4] board: ti: am62px: env: include environment for DFU Boot

[Mattijs Korpershoek]

Hi Siddharth,

Thank you for the patch.

On mar., déc. 17, 2024 at 18:46, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:

> Include the TI K3 DFU environment to support DFU Boot and DFU Flash.
> Also add "usb" to the list of "boot_targets".
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  board/ti/am62px/am62px.env | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/board/ti/am62px/am62px.env b/board/ti/am62px/am62px.env
> index 7ef54079aa8..e0838196e3a 100644
> --- a/board/ti/am62px/am62px.env
> +++ b/board/ti/am62px/am62px.env
> @@ -1,5 +1,6 @@
>  #include <env/ti/ti_common.env>
>  #include <env/ti/mmc.env>
> +#include <env/ti/k3_dfu.env>
>
>  name_kern=Image
>  console=ttyS2,115200n8
> @@ -7,7 +8,7 @@ args_all=setenv optargs ${optargs} earlycon=ns16550a,mmio32,0x02800000
>  	${mtdparts}
>  run_kern=booti ${loadaddr} ${rd_spec} ${fdtaddr}
>
> -boot_targets=mmc1 mmc0 pxe dhcp
> +boot_targets=mmc1 mmc0 usb pxe dhcp
>  boot=mmc
>  mmcdev=1
>  bootpart=1:2
> @@ -17,4 +18,4 @@ rd_spec=-
>  #if CONFIG_BOOTMETH_ANDROID
>  #include <env/ti/android.env>
>  adtb_idx=3
> -#endif
> \ No newline at end of file
> +#endif

This change seems un-related, is it needed?

Also, looking at Martyn's/Sjoerd's series, I see a couple of things
missing:
1. Documentation. now that am62px is compatible with the
   am62x_r5_usbdfu.config fragment, we need to document this in the board
   docs. See: commit def64b493748 ("doc: board: Add document for DFU boot on am62x SoCs")

2. Including configs/am62x_a53_usbdfu.config in configs/am62px_evm_a53_defconfig.
   This is how it's done for am62x, see:
   commit dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support")

Note that If we don't do 2), we cannot use USB gadget from a U-Boot that
has been booted over DFU:

    => fastboot usb 0
    No USB device found
    USB init failed: -19
    => usb list
    USB is stopped. Please issue 'usb start' first.
    => usb start
    starting USB...
    No USB controllers found
    =>

For 2, this diff fixes it:

diff --git a/configs/am62px_evm_a53_defconfig b/configs/am62px_evm_a53_defconfig
index 9635beb1b27e..81f433c997b5 100644
--- a/configs/am62px_evm_a53_defconfig
+++ b/configs/am62px_evm_a53_defconfig
@@ -183,3 +183,4 @@ CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
 CONFIG_EFI_SET_TIME=y
 
 #include <configs/k3_efi_capsule.config>
+#include <configs/am62x_a53_usbdfu.config>

In my opinion, 2) is a valid use case:
1. On a blank board, we boot the bootloaders over DFU
2. Once U-Boot is started, we start fastboot to flash all images to eMMC.

Could this be added for v2, please?

Thanks,
Mattijs

> --
> 2.43.0

Re: [PATCH 2/4] board: ti: am62px: env: include environment for DFU Boot

[Siddharth Vadapalli]

On Wed, Dec 18, 2024 at 10:57:36AM +0100, Mattijs Korpershoek wrote:

Hello Mattijs,

> Hi Siddharth,
> 
> Thank you for the patch.
> 
> On mar., déc. 17, 2024 at 18:46, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> 
> > Include the TI K3 DFU environment to support DFU Boot and DFU Flash.
> > Also add "usb" to the list of "boot_targets".
> >
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> >  board/ti/am62px/am62px.env | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/board/ti/am62px/am62px.env b/board/ti/am62px/am62px.env
> > index 7ef54079aa8..e0838196e3a 100644
> > --- a/board/ti/am62px/am62px.env
> > +++ b/board/ti/am62px/am62px.env
> > @@ -1,5 +1,6 @@
> >  #include <env/ti/ti_common.env>
> >  #include <env/ti/mmc.env>
> > +#include <env/ti/k3_dfu.env>
> >
> >  name_kern=Image
> >  console=ttyS2,115200n8
> > @@ -7,7 +8,7 @@ args_all=setenv optargs ${optargs} earlycon=ns16550a,mmio32,0x02800000
> >  	${mtdparts}
> >  run_kern=booti ${loadaddr} ${rd_spec} ${fdtaddr}
> >
> > -boot_targets=mmc1 mmc0 pxe dhcp
> > +boot_targets=mmc1 mmc0 usb pxe dhcp
> >  boot=mmc
> >  mmcdev=1
> >  bootpart=1:2
> > @@ -17,4 +18,4 @@ rd_spec=-
> >  #if CONFIG_BOOTMETH_ANDROID
> >  #include <env/ti/android.env>
> >  adtb_idx=3
> > -#endif
> > \ No newline at end of file
> > +#endif
> 
> This change seems un-related, is it needed?

My editor automatically adds a newline, thereby fixing the warning/error
regarding the absence of a newline. I assume that a newline is expected.
Please let me know if you want me to undo this in the v2 series.

> 
> Also, looking at Martyn's/Sjoerd's series, I see a couple of things
> missing:
> 1. Documentation. now that am62px is compatible with the
>    am62x_r5_usbdfu.config fragment, we need to document this in the board
>    docs. See: commit def64b493748 ("doc: board: Add document for DFU boot on am62x SoCs")

I planned on documenting it once this series got merged. The reason
behind it is that I was unsure if the device-tree patch in this series
will be accepted or will be asked to be enabled via the Linux DT Sync
process. In the latter case, documenting this feature will be incorrect
in the event of a partial merge (i.e. the documentation patch gets
merged but the device-tree patch doesn't).

> 
> 2. Including configs/am62x_a53_usbdfu.config in configs/am62px_evm_a53_defconfig.
>    This is how it's done for am62x, see:
>    commit dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support")
> 
> Note that If we don't do 2), we cannot use USB gadget from a U-Boot that
> has been booted over DFU:
> 
>     => fastboot usb 0
>     No USB device found
>     USB init failed: -19
>     => usb list
>     USB is stopped. Please issue 'usb start' first.
>     => usb start
>     starting USB...
>     No USB controllers found
>     =>
> 
> For 2, this diff fixes it:
> 
> diff --git a/configs/am62px_evm_a53_defconfig b/configs/am62px_evm_a53_defconfig
> index 9635beb1b27e..81f433c997b5 100644
> --- a/configs/am62px_evm_a53_defconfig
> +++ b/configs/am62px_evm_a53_defconfig
> @@ -183,3 +183,4 @@ CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
>  CONFIG_EFI_SET_TIME=y
>  
>  #include <configs/k3_efi_capsule.config>
> +#include <configs/am62x_a53_usbdfu.config>
> 
> In my opinion, 2) is a valid use case:
> 1. On a blank board, we boot the bootloaders over DFU
> 2. Once U-Boot is started, we start fastboot to flash all images to eMMC.
> 
> Could this be added for v2, please?

Sure, I will include it in the v2 series. Thank you for reviewing this
patch and sharing feedback.

Regards,
Siddharth.

Re: [PATCH 2/4] board: ti: am62px: env: include environment for DFU Boot

[Mattijs Korpershoek]

On mer., déc. 18, 2024 at 15:44, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:

> On Wed, Dec 18, 2024 at 10:57:36AM +0100, Mattijs Korpershoek wrote:
>
> Hello Mattijs,
>
>> Hi Siddharth,
>> 
>> Thank you for the patch.
>> 
>> On mar., déc. 17, 2024 at 18:46, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>> 
>> > Include the TI K3 DFU environment to support DFU Boot and DFU Flash.
>> > Also add "usb" to the list of "boot_targets".
>> >
>> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> > ---
>> >  board/ti/am62px/am62px.env | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/board/ti/am62px/am62px.env b/board/ti/am62px/am62px.env
>> > index 7ef54079aa8..e0838196e3a 100644
>> > --- a/board/ti/am62px/am62px.env
>> > +++ b/board/ti/am62px/am62px.env
>> > @@ -1,5 +1,6 @@
>> >  #include <env/ti/ti_common.env>
>> >  #include <env/ti/mmc.env>
>> > +#include <env/ti/k3_dfu.env>
>> >
>> >  name_kern=Image
>> >  console=ttyS2,115200n8
>> > @@ -7,7 +8,7 @@ args_all=setenv optargs ${optargs} earlycon=ns16550a,mmio32,0x02800000
>> >  	${mtdparts}
>> >  run_kern=booti ${loadaddr} ${rd_spec} ${fdtaddr}
>> >
>> > -boot_targets=mmc1 mmc0 pxe dhcp
>> > +boot_targets=mmc1 mmc0 usb pxe dhcp
>> >  boot=mmc
>> >  mmcdev=1
>> >  bootpart=1:2
>> > @@ -17,4 +18,4 @@ rd_spec=-
>> >  #if CONFIG_BOOTMETH_ANDROID
>> >  #include <env/ti/android.env>
>> >  adtb_idx=3
>> > -#endif
>> > \ No newline at end of file
>> > +#endif
>> 
>> This change seems un-related, is it needed?
>
> My editor automatically adds a newline, thereby fixing the warning/error
> regarding the absence of a newline. I assume that a newline is expected.
> Please let me know if you want me to undo this in the v2 series.

This is editor dependant. I'm not sure if there is a coding style entry
for having a newline present/absent.
I'm in favor of keeping the newline addition, however, please mention
it in the commit message.

Something between the lines of "while at it, also add a missing newline
to the am62p.env file".

>
>> 
>> Also, looking at Martyn's/Sjoerd's series, I see a couple of things
>> missing:
>> 1. Documentation. now that am62px is compatible with the
>>    am62x_r5_usbdfu.config fragment, we need to document this in the board
>>    docs. See: commit def64b493748 ("doc: board: Add document for DFU boot on am62x SoCs")
>
> I planned on documenting it once this series got merged. The reason
> behind it is that I was unsure if the device-tree patch in this series
> will be accepted or will be asked to be enabled via the Linux DT Sync
> process. In the latter case, documenting this feature will be incorrect
> in the event of a partial merge (i.e. the documentation patch gets
> merged but the device-tree patch doesn't).

Hmm, I'm not the one deciding if [PATCH 4/4] will get applied so I can't
chime in on that. However, I think it's good practice to send the
documentation changes along with the code changes. Otherwise, doc
updates might be forgotten/de-prioritized.

I won't block the series if doc does not get send, though.

Maybe for future submissions, you can consider writing this in the cover
letter:

"Since i'm unsure that PATCH 4/4 will be accepted, I've decided to send
the documentation changes in a future series"

>
>> 
>> 2. Including configs/am62x_a53_usbdfu.config in configs/am62px_evm_a53_defconfig.
>>    This is how it's done for am62x, see:
>>    commit dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support")
>> 
>> Note that If we don't do 2), we cannot use USB gadget from a U-Boot that
>> has been booted over DFU:
>> 
>>     => fastboot usb 0
>>     No USB device found
>>     USB init failed: -19
>>     => usb list
>>     USB is stopped. Please issue 'usb start' first.
>>     => usb start
>>     starting USB...
>>     No USB controllers found
>>     =>
>> 
>> For 2, this diff fixes it:
>> 
>> diff --git a/configs/am62px_evm_a53_defconfig b/configs/am62px_evm_a53_defconfig
>> index 9635beb1b27e..81f433c997b5 100644
>> --- a/configs/am62px_evm_a53_defconfig
>> +++ b/configs/am62px_evm_a53_defconfig
>> @@ -183,3 +183,4 @@ CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
>>  CONFIG_EFI_SET_TIME=y
>>  
>>  #include <configs/k3_efi_capsule.config>
>> +#include <configs/am62x_a53_usbdfu.config>
>> 
>> In my opinion, 2) is a valid use case:
>> 1. On a blank board, we boot the bootloaders over DFU
>> 2. Once U-Boot is started, we start fastboot to flash all images to eMMC.
>> 
>> Could this be added for v2, please?
>
> Sure, I will include it in the v2 series. Thank you for reviewing this
> patch and sharing feedback.

Make sure to check that am62px_evm_a53_defconfig does not contain any
duplicate entries with the dfu fragment, like:

CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments"
CONFIG_USB_GADGET_VENDOR_NUM=0x0451
CONFIG_USB_GADGET_PRODUCT_NUM=0x6165

>
> Regards,
> Siddharth.

Re: [PATCH 2/4] board: ti: am62px: env: include environment for DFU Boot

[Siddharth Vadapalli]

On Wed, Dec 18, 2024 at 12:00:08PM +0100, Mattijs Korpershoek wrote:
> On mer., déc. 18, 2024 at 15:44, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> 
> > On Wed, Dec 18, 2024 at 10:57:36AM +0100, Mattijs Korpershoek wrote:
> >
> > Hello Mattijs,
> >
> >> Hi Siddharth,
> >> 
> >> Thank you for the patch.

[...]

> >> This change seems un-related, is it needed?
> >
> > My editor automatically adds a newline, thereby fixing the warning/error
> > regarding the absence of a newline. I assume that a newline is expected.
> > Please let me know if you want me to undo this in the v2 series.
> 
> This is editor dependant. I'm not sure if there is a coding style entry
> for having a newline present/absent.
> I'm in favor of keeping the newline addition, however, please mention
> it in the commit message.
> 
> Something between the lines of "while at it, also add a missing newline
> to the am62p.env file".

Sure, I will mention this in the commit message.

> 
> >
> >> 
> >> Also, looking at Martyn's/Sjoerd's series, I see a couple of things
> >> missing:
> >> 1. Documentation. now that am62px is compatible with the
> >>    am62x_r5_usbdfu.config fragment, we need to document this in the board
> >>    docs. See: commit def64b493748 ("doc: board: Add document for DFU boot on am62x SoCs")
> >
> > I planned on documenting it once this series got merged. The reason
> > behind it is that I was unsure if the device-tree patch in this series
> > will be accepted or will be asked to be enabled via the Linux DT Sync
> > process. In the latter case, documenting this feature will be incorrect
> > in the event of a partial merge (i.e. the documentation patch gets
> > merged but the device-tree patch doesn't).
> 
> Hmm, I'm not the one deciding if [PATCH 4/4] will get applied so I can't
> chime in on that. However, I think it's good practice to send the
> documentation changes along with the code changes. Otherwise, doc
> updates might be forgotten/de-prioritized.
> 
> I won't block the series if doc does not get send, though.
> 
> Maybe for future submissions, you can consider writing this in the cover
> letter:
> 
> "Since i'm unsure that PATCH 4/4 will be accepted, I've decided to send
> the documentation changes in a future series"

I will include the documentation in the v2 series, while also pointing
out that if the device-tree changes don't get in, then the documentation
patch should also be dropped.

> 
> >
> >> 
> >> 2. Including configs/am62x_a53_usbdfu.config in configs/am62px_evm_a53_defconfig.
> >>    This is how it's done for am62x, see:
> >>    commit dfc2dff5a844 ("configs: am62x_evm_*: Enable USB and DFU support")
> >> 
> >> Note that If we don't do 2), we cannot use USB gadget from a U-Boot that
> >> has been booted over DFU:
> >> 
> >>     => fastboot usb 0
> >>     No USB device found
> >>     USB init failed: -19
> >>     => usb list
> >>     USB is stopped. Please issue 'usb start' first.
> >>     => usb start
> >>     starting USB...
> >>     No USB controllers found
> >>     =>
> >> 
> >> For 2, this diff fixes it:
> >> 
> >> diff --git a/configs/am62px_evm_a53_defconfig b/configs/am62px_evm_a53_defconfig
> >> index 9635beb1b27e..81f433c997b5 100644
> >> --- a/configs/am62px_evm_a53_defconfig
> >> +++ b/configs/am62px_evm_a53_defconfig
> >> @@ -183,3 +183,4 @@ CONFIG_FS_FAT_MAX_CLUSTSIZE=16384
> >>  CONFIG_EFI_SET_TIME=y
> >>  
> >>  #include <configs/k3_efi_capsule.config>
> >> +#include <configs/am62x_a53_usbdfu.config>
> >> 
> >> In my opinion, 2) is a valid use case:
> >> 1. On a blank board, we boot the bootloaders over DFU
> >> 2. Once U-Boot is started, we start fastboot to flash all images to eMMC.
> >> 
> >> Could this be added for v2, please?
> >
> > Sure, I will include it in the v2 series. Thank you for reviewing this
> > patch and sharing feedback.
> 
> Make sure to check that am62px_evm_a53_defconfig does not contain any
> duplicate entries with the dfu fragment, like:
> 
> CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments"
> CONFIG_USB_GADGET_VENDOR_NUM=0x0451
> CONFIG_USB_GADGET_PRODUCT_NUM=0x6165

I will keep this in mind. Thank you.

Regards,
Siddharth.

[PATCH 3/4] configs: am62x_r5_usbdfu: extend for AM62Px

[Siddharth Vadapalli]

Disable configs which are not required for USB DFU functionality, in
order to allow reusing this fragment for AM62Px SoC.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 configs/am62x_r5_usbdfu.config | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configs/am62x_r5_usbdfu.config b/configs/am62x_r5_usbdfu.config
index 772bb2ab935..f094ed127e6 100644
--- a/configs/am62x_r5_usbdfu.config
+++ b/configs/am62x_r5_usbdfu.config
@@ -26,3 +26,6 @@ CONFIG_SPL_DFU=y
 # CONFIG_CMD_MMC is not set
 # CONFIG_CMD_FAT is not set
 # CONFIG_MMC_SDHCI is not set
+# CONFIG_SPL_MTD is not set
+# CONFIG_DMA_CHANNELS is not set
+# CONFIG_TI_K3_NAVSS_UDMA is not set
-- 
2.43.0

Re: [PATCH 3/4] configs: am62x_r5_usbdfu: extend for AM62Px

[Mattijs Korpershoek]

Hi Siddharth,

Thank you for the patch.

On mar., déc. 17, 2024 at 18:46, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:

> Disable configs which are not required for USB DFU functionality, in
> order to allow reusing this fragment for AM62Px SoC.
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

I confirm this is needed, without this, the tispl size is too big and we
can't build.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  configs/am62x_r5_usbdfu.config | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/configs/am62x_r5_usbdfu.config b/configs/am62x_r5_usbdfu.config
> index 772bb2ab935..f094ed127e6 100644
> --- a/configs/am62x_r5_usbdfu.config
> +++ b/configs/am62x_r5_usbdfu.config
> @@ -26,3 +26,6 @@ CONFIG_SPL_DFU=y
>  # CONFIG_CMD_MMC is not set
>  # CONFIG_CMD_FAT is not set
>  # CONFIG_MMC_SDHCI is not set
> +# CONFIG_SPL_MTD is not set
> +# CONFIG_DMA_CHANNELS is not set
> +# CONFIG_TI_K3_NAVSS_UDMA is not set
> -- 
> 2.43.0

[PATCH 4/4] arm: dts: k3-am62p5-sk-u-boot: enable USB0 for USB DFU boot

[Siddharth Vadapalli]

Enable USB0 instance of the USB controller for USB DFU boot functionality
which requires the "bootph-all" property in order to have USB enabled at
all stages of the USB DFU boot.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 arch/arm/dts/k3-am62p5-sk-u-boot.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi b/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
index cf087c6e343..c2091fc1a51 100644
--- a/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
+++ b/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
@@ -16,3 +16,13 @@
 &dmsc {
 	bootph-pre-ram;
 };
+
+/* Required for USB DFU boot */
+&usbss0 {
+	status = "okay";
+};
+
+/* Required for USB DFU boot */
+&usb0 {
+	bootph-all;
+};
-- 
2.43.0

Re: [PATCH 4/4] arm: dts: k3-am62p5-sk-u-boot: enable USB0 for USB DFU boot

[Mattijs Korpershoek]

Hi Siddharth,

Thank you for the patch.

On mar., déc. 17, 2024 at 18:46, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:

> Enable USB0 instance of the USB controller for USB DFU boot functionality
> which requires the "bootph-all" property in order to have USB enabled at
> all stages of the USB DFU boot.
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  arch/arm/dts/k3-am62p5-sk-u-boot.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi b/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> index cf087c6e343..c2091fc1a51 100644
> --- a/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> @@ -16,3 +16,13 @@
>  &dmsc {
>  	bootph-pre-ram;
>  };
> +
> +/* Required for USB DFU boot */
> +&usbss0 {
> +	status = "okay";
> +};
> +
> +/* Required for USB DFU boot */
> +&usb0 {
> +	bootph-all;
> +};
> -- 
> 2.43.0

Re: [PATCH 0/4] Add support for USB DFU boot on AM62Px

[Mattijs Korpershoek]

Hi Siddharth,

Thank you for the series.

On mar., déc. 17, 2024 at 18:46, Siddharth Vadapalli <s-vadapalli@ti.com> wrote:

> Hello,
>
> This series add support for USB DFU boot on TI's AM62Px SoC which has
> two instances of USB Controllers, of which the USB0 instance of USB
> which is a DWC3 USB Controller is used for USB DFU Boot:
> ROM => tiboot3.bin => tispl.bin => u-boot.img
>
> The first patch of this series is based on the suggestion of
> Jonathan Humphreys <j-humpreys@ti.com>
> at:
> https://patchwork.ozlabs.org/project/uboot/cover/20241124070828.617558-1-s-vadapalli@ti.com/#3419733
> which prevents overwriting the "dfu_alt_info" environment variable which
> is set by "spl_dfu_cmd()" for USB DFU boot.
>
> The second patch adds USB DFU environment for AM62Px.
>
> The third patch extends "am62x_r5_usbdfu.config" config fragment for
> being re-used by AM62Px SoC by dropping configs which aren't required
> for USB DFU boot. This is necessary to reduce the size of R5 SPL
> (tiboot3.bin) built for USB DFU boot.
>
> The fourth patch enables USB device-tree nodes for USB DFU functionality
> on AM62Px. Since CONFIG_OF_UPSTREAM is not yet supported for AM62Px, the
> changes are being made in arch/arm/dts/k3-am62p5-sk-u-boot.dtsi at the
> moment, with the intent of making the same changes in the Linux
> device-tree and eventually enabling CONFIG_OF_UPSTREAM to drop the
> changes made in "k3-am62p5-sk-u-boot.dtsi".
>
> Series is based on commit
> 3b3c7280b82 smbios: address build warning
> of the next branch of U-Boot.
>
> The USB DFU config fragments for AM62x are to be re-used for enabling
> USB DFU boot on AM62Px as follows:
> R5  => am62px_evm_r5_defconfig + am62x_r5_usbdfu.config
> A53 => am62px_evm_a53_defconfig + am62x_a53_usbdfu.config
>
> Series has been tested on AM62P5-SK. Logs validating USB DFU boot on
> AM62P5-SK with this series:
> https://gist.github.com/Siddharth-Vadapalli-at-TI/50198ee3348c2db73e0dbd3a14cb0f8b
>
> Series has also been validated on AM625-SK to ensure that the changes
> made to "am62x_r5_usbdfu.config" config fragment being used by AM625-SK
> for USB DFU boot do not cause a regression on AM625-SK w.r.t. USB DFU
> boot. Logs validating USB DFU Boot on AM625-SK with this series:
> https://gist.github.com/Siddharth-Vadapalli-at-TI/85a7579ed2c95330ac71ee3a832cadfb
>
> Regards,
> Siddharth.
>
> Jonathan Humphreys (1):
>   dfu: do not overwrite "dfu_alt_info"
>
> Siddharth Vadapalli (3):
>   board: ti: am62px: env: include environment for DFU Boot
>   configs: am62x_r5_usbdfu: extend for AM62Px
>   arm: dts: k3-am62p5-sk-u-boot: enable USB0 for USB DFU boot

Boot tested this on AM62Px SK EVM using snagrecover:

Boot logs are here: https://paste.debian.net/1340377/

Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

>
>  arch/arm/dts/k3-am62p5-sk-u-boot.dtsi | 10 ++++++++++
>  board/ti/am62px/am62px.env            |  5 +++--
>  configs/am62x_r5_usbdfu.config        |  3 +++
>  drivers/dfu/dfu.c                     |  7 +++++--
>  4 files changed, 21 insertions(+), 4 deletions(-)
>
> -- 
> 2.43.0