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:
- 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.
- 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:
- On a blank board, we boot the bootloaders over DFU
- 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.