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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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.