[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 at 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.