Re: [PATCH RFC/RFT 2/3] board: ti: j784s4: Initialize the ESM & PMIC ESM

[Andrew Halaney]

On Sat, Sep 07, 2024 at 11:12:05AM GMT, Kumar, Udit wrote:
> Hi Andrew,
> 
> On 9/7/2024 2:54 AM, Andrew Halaney wrote:
> > From: Keerthy <j-keer...@ti.com>
> > 
> > Initialize the ESM & PMIC ESM. This allows things like
> > the watchdog to reset the board when tripped.
> > 
> > Signed-off-by: Keerthy <j-keer...@ti.com>
> > Signed-off-by: Neha Malcom Francis <n-fran...@ti.com>
> > Link: 
> > https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/board/ti/j784s4/evm.c?h=ti-u-boot-2024.04&id=9d8b40958ce792808bc571d828197dbc2e7978d6
> > [halaney: add a line to the commit message, include header]
> > Signed-off-by: Andrew Halaney <ahala...@redhat.com>
> > ---
> >   board/ti/j784s4/evm.c | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> > 
> > diff --git a/board/ti/j784s4/evm.c b/board/ti/j784s4/evm.c
> > index 548dbd5925d..a0ef3a44536 100644
> > --- a/board/ti/j784s4/evm.c
> > +++ b/board/ti/j784s4/evm.c
> > @@ -7,6 +7,7 @@
> >    *
> >    */
> > +#include <dm.h>
> >   #include <efi_loader.h>
> >   #include <init.h>
> >   #include <spl.h>
> > @@ -72,4 +73,31 @@ int board_late_init(void)
> >   void spl_board_init(void)
> >   {
> > +   struct udevice *dev;
> > +   int ret;
> > +
> > +   if (IS_ENABLED(CONFIG_ESM_K3)) {
> > +           ret = uclass_get_device_by_name(UCLASS_MISC, "esm@700000",
> > +                                           &dev);
> > +           if (ret)
> > +                   printf("MISC init for esm@700000 failed: %d\n", ret);
> > +
> > +           ret = uclass_get_device_by_name(UCLASS_MISC, "esm@40800000",
> > +                                           &dev);
> > +           if (ret)
> > +                   printf("MISC init for esm@40800000 failed: %d\n", ret);
> > +
> > +           ret = uclass_get_device_by_name(UCLASS_MISC, "esm@42080000",
> > +                                           &dev);
> > +           if (ret)
> > +                   printf("MISC init for esm@42080000 failed: %d\n", ret);
> > +   }
> 
> Thanks for patch ,
> 
> IMO, if one of esm is failing then we can skip other esm probe,
> 
> > +
> > +   if (IS_ENABLED(CONFIG_ESM_PMIC)) {
> > +           ret = uclass_get_device_by_driver(UCLASS_MISC,
> > +                                             DM_DRIVER_GET(pmic_esm),
> > +                                             &dev);
> 
> Same here if esm probe is successful then only we should probe pmic_esm.
> 
> Reason is , if any of probe is failing functionality will not work and
> 
> there is no point to probe devices, if function is not working.

Any preference on how this is handled? I was going to do something like:

        if (IS_ENABLED(CONFIG_ESM_K3)) {
                ret = uclass_get_device_by_name(UCLASS_MISC, "esm@700000",
                                                &dev);
                if (ret) {
                        printf("MISC init for esm@700000 failed: %d\n", ret);
                        return;
                }

                ret = uclass_get_device_by_name(UCLASS_MISC, "esm@40800000",
                                                &dev);
                ...
                if (IS_ENABLED(CONFIG_ESM_PMIC)) {
                        ret = uclass_get_device_by_driver(UCLASS_MISC,
                                                          
DM_DRIVER_GET(pmic_esm),
                                                          &dev);
                        if (ret) {
                                printf("ESM PMIC init failed: %d\n", ret);
                                return;
                        }
                }
        }

but wasn't sure if a err_esm label and goto would be more in line with
what you imagined? Let me know otherwise I'll just submit this version
tomorrow or so.

Thanks,
Andrew