On Tue Feb 3, 2026 at 12:06 PM IST, Siddharth Vadapalli wrote:
> On Tue, 2026-02-03 at 11:38 +0530, Anshul Dalal wrote:
>> On Tue Feb 3, 2026 at 11:11 AM IST, Siddharth Vadapalli wrote:
>> > On Tue, 2026-02-03 at 10:51 +0530, Anshul Dalal wrote:
>> > > On Mon Feb 2, 2026 at 7:40 PM IST, Siddharth Vadapalli wrote:
>> > > > Since commit 27cc5951c862 ("include: env: ti: add default for
>> > > > do_main_cpsw0_qsgmii_phyinit"), the value of the environment variable
>> > > > do_main_cpsw0_qsgmii_phyinit happened to remain '0' and couldn't be
>> > > > changed without user intervention. This behavior is due to the
>> > > > following
>> > > > cyclic dependency:
>> > > > A) ti_common.env sets do_main_cpsw0_qsgmii_phyinit to '0' and its value
>> > > > can only be updated automatically by main_cpsw0_qsgmii_phyinit.
>> > > > B) main_cpsw0_qsgmii_phyinit is defined in j721e.env and it can run
>> > > > only
>> > > > if 'do_main_cpsw0_qsgmii_phyinit' is already '1' which isn't
>> > > > possible
>> > > > unless the user manually assigns the value.
>> > > >
>> > > > Fix the aforementioned cyclic dependency by using board_late_init() to
>> > > > detect the QSGMII Daughtercard and set do_main_cpsw0_qsgmii_phyinit.
>> > > >
>> > > > Additionally, to address the issue of do_main_cpsw0_qsgmii_phyinit
>> > > > being
>> > > > 'undefined' for other platforms, replace:
>> > > > if test ${do_main_cpsw0_qsgmii_phyinit} -eq 1;
>> > > > with:
>> > > > if env exists do_main_cpsw0_qsgmii_phyinit;
>> > > > in ti_common.env.
>> > > >
>> > > > Fixes: 27cc5951c862 ("include: env: ti: add default for
>> > > > do_main_cpsw0_qsgmii_phyinit")
>> > > > Signed-off-by: Siddharth Vadapalli <[email protected]>
>> > > > ---
>> > > >
>> >
>> > [TRIMMED]
>> >
>> > > > diff --git a/include/env/ti/ti_common.env
>> > > > b/include/env/ti/ti_common.env
>> > > > index 03e3267ef8a..a0ed83f52ac 100644
>> > > > --- a/include/env/ti/ti_common.env
>> > > > +++ b/include/env/ti/ti_common.env
>> > > > @@ -22,11 +22,10 @@ get_fit_overlaystring=
>> > > > done;
>> > > > get_fit_config=setexpr name_fit_config gsub / _ conf-${fdtfile}
>> > > > run_fit=run get_fit_config; bootm
>> > > > ${addr_fit}#${name_fit_config}${overlaystring}
>> > > > -do_main_cpsw0_qsgmii_phyinit=0
>> > > > bootcmd_ti_mmc=
>> > > > run init_${boot};
>> > > > #if CONFIG_CMD_REMOTEPROC
>> > > > - if test ${do_main_cpsw0_qsgmii_phyinit} -eq 1;
>> > > > + if env exists do_main_cpsw0_qsgmii_phyinit;
>> > >
>> > > The patch looks good to me however I wonder if we should factor out
>> > > cpsw0_qsgmii related envs out of ti_common.env since as far as I can
>> > > tell we only seem to be using it for the j721e/j7200 devices.
>> >
>> > Please refer to the following commit from 22 July 2023:
>> > https://github.com/u-boot/u-boot/commit/4ae1a2470ce7895b747c85a140aaf8b647ae6a32
>> > The commit added:
>> > run main_cpsw0_qsgmii_phyinit
>> > in include/environment/ti/ti_armv7_common.env and ti_armv7_common.env
>> > eventually got renamed to ti_common.env by:
>> > https://github.com/u-boot/u-boot/commit/bf9c61acb6caed97114029d2dc1e91b148cd9b8a
>> >
>> > Although I agree with your feedback in that it will make more sense for
>> > do_main_cpsw0_qsgmii_phyinit to be present in the board specific env, given
>> > the background pointed out above, it seems to me that we are simply going
>> > in circles by undoing the changes that were previously made. Nevertheless,
>> > if you think that it should be moved to the board environment, I will do so
>> > in a future cleanup patch.
>>
>> The patch that added main_cpsw0_qsgmii_phyinit was actually based on the
>> commit 14439cd71c1a ("configs: k3: make consistent bootcmd across all k3
>> socs") where main_cpsw0_qsgmii_phyinit was factored out form the
>> j721e/j7200 defconfigs to the common env. I think this could be better
>> handled by having a board level hook such as follows:
>>
>> --- a/include/env/ti/ti_common.env
>> +++ b/include/env/ti/ti_common.env
>> @@ -22,14 +22,10 @@ get_fit_overlaystring=
>> done;
>> get_fit_config=setexpr name_fit_config gsub / _ conf-${fdtfile}
>> run_fit=run get_fit_config; bootm
>> ${addr_fit}#${name_fit_config}${overlaystring}
>> +init_board=
>> bootcmd_ti_mmc=
>> run init_${boot};
>> -#if CONFIG_CMD_REMOTEPROC
>> - if env exists do_main_cpsw0_qsgmii_phyinit;
>> - then run main_cpsw0_qsgmii_phyinit;
>> - fi;
>> - run boot_rprocs;
>> -#endif
>> + run init_board;
>
> This assumes that it is defined for all boards and we will run into the
> following error for those boards which don't have it in their env:
> ## Error: "init_board" not defined
> So it needs to be updated to:
> if env exists init_board;
> then run init_board;
> fi;
>
I thought setting init_board to empty string above would've taken care
of that but I can incorporate your suggestion as well, thanks :)
>> if test ${boot_fit} -eq 1;
>> then run get_fit_${boot}; run get_fit_overlaystring;
>> run run_fit;
>> else;
>>
>> with init_board being used as required in the board's env as follows:
>>
>> --- a/board/ti/j7200/j7200.env
>> +++ b/board/ti/j7200/j7200.env
>> @@ -34,6 +34,13 @@ main_cpsw0_qsgmii_phyinit=
>> fi;
>> #endif
>>
>> +#if CONFIG_CMD_REMOTEPROC
>> +init_board=
>> + if env exists do_main_cpsw0_qsgmii_phyinit;
>> + then run main_cpsw0_qsgmii_phyinit;
>> + fi;
>> + run boot_rprocs;
>> +#endif
>> #if CONFIG_TARGET_J7200_A72_EVM
>> rproc_fw_binaries= 1 /lib/firmware/j7200-mcu-r5f0_1-fw 2
>> /lib/firmware/j7200-main-r5f0_0-fw 3 /lib/firmware/j7200-main-r5f0_1-fw
>> #endif
>
> With the minor comment above, this diff looks good to me. Please let me
> know if you plan on posting the patch for it.
>
> Additionally, since this is unrelated to the current patch, please
> acknowledge that you have no concerns with the current patch.
>
Yeah, I can post the refactor once your patch is merged to which I have
no objections to.
Reviewed-by: Anshul Dalal <[email protected]>
