[PATCH] lib: reset: Add early_init api for fdt_reset_drv

Guo Ren guoren at kernel.org
Sun Jun 18 08:11:01 PDT 2023


On Sat, Jun 17, 2023 at 2:43 AM Anup Patel <apatel at ventanamicro.com> wrote:
>
> On Sat, Jun 17, 2023 at 10:57 AM <guoren at kernel.org> wrote:
> >
> > From: Guo Ren <guoren at linux.alibaba.com>
> >
> > The fdt_reset_thead driver needs to modify the __reset_thead_csr_stub
> > text region for the secondary harts booting. After that, the
> > sbi_hart_pmp_configure may lock down the text region with M_READABLE &
> > M_EXECUTABLE attributes in the future. Currently, the M_READABLE &
> > M_EXECUtABLE have no effect on m-mode, the L-bit in pmpcfg csr is
> > useless for the current opensbi scenario. See:
> >
> > Priv-isa-spec 3.7.1.2. Locking and Privilege Mode
> > When the L bit is clear, any M-mode access matching the PMP entry will
> > succeed; the R/W/X permissions apply only to S and U modes.
> >
> > That's why current fdt_reset_thead could still work well after commit:
> > 230278dcf127 ("lib: sbi: Add separate entries for firmware RX and RW
> > regions"). So this patch fixes up a fake bug for the M-mode permission
> > setting of the future.
> >
> > Fixes: 230278dcf127 ("lib: sbi: Add separate entries for firmware RX and RW regions")
> > Link: http://lists.infradead.org/pipermail/opensbi/2023-June/005176.html
> > Reported-by: Jessica Clarke <jrtc27 at jrtc27.com>
> > Signed-off-by: Guo Ren <guoren at linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren at kernel.org>
> > ---
> >  include/sbi_utils/reset/fdt_reset.h |  9 +++++----
> >  lib/utils/reset/fdt_reset.c         | 20 +++++++++++++-------
> >  lib/utils/reset/fdt_reset_thead.c   |  2 +-
> >  platform/generic/platform.c         |  5 ++++-
> >  platform/generic/sifive/fu740.c     |  2 +-
> >  platform/generic/starfive/jh7110.c  |  2 +-
> >  6 files changed, 25 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/sbi_utils/reset/fdt_reset.h b/include/sbi_utils/reset/fdt_reset.h
> > index ea8063b8a90e..2b1849534e2b 100644
> > --- a/include/sbi_utils/reset/fdt_reset.h
> > +++ b/include/sbi_utils/reset/fdt_reset.h
> > @@ -15,6 +15,7 @@
> >  struct fdt_reset {
> >         const struct fdt_match *match_table;
> >         int (*init)(void *fdt, int nodeoff, const struct fdt_match *match);
> > +       int (*early_init)(void *fdt, int nodeoff, const struct fdt_match *match);
> >  };
> >
> >  #ifdef CONFIG_FDT_RESET
> > @@ -22,22 +23,22 @@ struct fdt_reset {
> >  /**
> >   * fdt_reset_driver_init() - initialize reset driver based on the device-tree
> >   */
> > -int fdt_reset_driver_init(void *fdt, struct fdt_reset *drv);
> > +int fdt_reset_driver_init(void *fdt, struct fdt_reset *drv, bool early);
> >
> >  /**
> >   * fdt_reset_init() - initialize reset drivers based on the device-tree
> >   *
> >   * This function shall be invoked in final init.
> >   */
> > -void fdt_reset_init(void);
> > +void fdt_reset_init(bool early);
> >
> >  #else
> >
> > -static inline int fdt_reset_driver_init(void *fdt, struct fdt_reset *drv)
> > +static inline int fdt_reset_driver_init(void *fdt, struct fdt_reset *drv, bool early)
> >  {
> >         return 0;
> >  }
> > -static inline void fdt_reset_init(void) { }
> > +static inline void fdt_reset_init(bool early) { }
> >
> >  #endif
> >
> > diff --git a/lib/utils/reset/fdt_reset.c b/lib/utils/reset/fdt_reset.c
> > index 4334586d39f7..a99d8ecc5907 100644
> > --- a/lib/utils/reset/fdt_reset.c
> > +++ b/lib/utils/reset/fdt_reset.c
> > @@ -17,31 +17,37 @@
> >  extern struct fdt_reset *fdt_reset_drivers[];
> >  extern unsigned long fdt_reset_drivers_size;
> >
> > -int fdt_reset_driver_init(void *fdt, struct fdt_reset *drv)
> > +int fdt_reset_driver_init(void *fdt, struct fdt_reset *drv, bool early)
> >  {
> >         int noff, rc = SBI_ENODEV;
> >         const struct fdt_match *match;
> > +       int (*init)(void *fdt, int nodeoff, const struct fdt_match *match);
> >
> >         noff = fdt_find_match(fdt, -1, drv->match_table, &match);
> >         if (noff < 0)
> >                 return SBI_ENODEV;
> >
> > -       if (drv->init) {
> > -               rc = drv->init(fdt, noff, match);
> > +       if (early)
> > +               init = drv->early_init;
> > +       else
> > +               init = drv->init;
> > +
> > +       if (init) {
> > +               rc = init(fdt, noff, match);
> >                 if (rc && rc != SBI_ENODEV) {
> > -                       sbi_printf("%s: %s init failed, %d\n",
> > -                                  __func__, match->compatible, rc);
> > +                       sbi_printf("%s: %s %s-init failed, %d\n",
> > +                                  __func__, match->compatible, early?"early":"final", rc);
> >                 }
> >         }
> >
> >         return rc;
> >  }
> >
> > -void fdt_reset_init(void)
> > +void fdt_reset_init(bool early)
> >  {
> >         int pos;
> >         void *fdt = fdt_get_address();
> >
> >         for (pos = 0; pos < fdt_reset_drivers_size; pos++)
> > -               fdt_reset_driver_init(fdt, fdt_reset_drivers[pos]);
> > +               fdt_reset_driver_init(fdt, fdt_reset_drivers[pos], early);
> >  }
> > diff --git a/lib/utils/reset/fdt_reset_thead.c b/lib/utils/reset/fdt_reset_thead.c
> > index e1d6885debae..4cae037b1db7 100644
> > --- a/lib/utils/reset/fdt_reset_thead.c
> > +++ b/lib/utils/reset/fdt_reset_thead.c
> > @@ -130,5 +130,5 @@ static const struct fdt_match thead_reset_match[] = {
> >
> >  struct fdt_reset fdt_reset_thead = {
> >         .match_table = thead_reset_match,
> > -       .init = thead_reset_init
> > +       .early_init = thead_reset_init
> >  };
> > diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> > index eeefef4c9533..65134e2c3895 100644
> > --- a/platform/generic/platform.c
> > +++ b/platform/generic/platform.c
> > @@ -146,6 +146,9 @@ static int generic_early_init(bool cold_boot)
> >         if (!generic_plat || !generic_plat->early_init)
> >                 return 0;
> >
> > +       if (cold_boot)
> > +               fdt_reset_init(true);
> > +
> >         return generic_plat->early_init(cold_boot, generic_plat_match);
> >  }
> >
> > @@ -155,7 +158,7 @@ static int generic_final_init(bool cold_boot)
> >         int rc;
> >
> >         if (cold_boot)
> > -               fdt_reset_init();
> > +               fdt_reset_init(false);
>
> We don't need this "early" flag business in fdt_reset_init().
>
> Just move the fdt_reset_init() call to generic_early_init() before
> "if (!generic_plat || !generic_plat->early_init)".
I agree and that would be more direct.
>
> Regards,
> Anup
>
> >
> >         if (generic_plat && generic_plat->final_init) {
> >                 rc = generic_plat->final_init(cold_boot, generic_plat_match);
> > diff --git a/platform/generic/sifive/fu740.c b/platform/generic/sifive/fu740.c
> > index fe71ce6a9ea5..5d3f1388beef 100644
> > --- a/platform/generic/sifive/fu740.c
> > +++ b/platform/generic/sifive/fu740.c
> > @@ -233,7 +233,7 @@ static int sifive_fu740_final_init(bool cold_boot,
> >         void *fdt = fdt_get_address();
> >
> >         if (cold_boot) {
> > -               rc = fdt_reset_driver_init(fdt, &fdt_reset_da9063);
> > +               rc = fdt_reset_driver_init(fdt, &fdt_reset_da9063, false);
> >                 if (rc)
> >                         sbi_printf("%s: failed to find da9063 for reset\n",
> >                                    __func__);
> > diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
> > index dcd6306be5fc..f21fe85c6560 100644
> > --- a/platform/generic/starfive/jh7110.c
> > +++ b/platform/generic/starfive/jh7110.c
> > @@ -277,7 +277,7 @@ static int starfive_jh7110_final_init(bool cold_boot,
> >         void *fdt = fdt_get_address();
> >
> >         if (cold_boot) {
> > -               fdt_reset_driver_init(fdt, &fdt_reset_pmic);
> > +               fdt_reset_driver_init(fdt, &fdt_reset_pmic, false);
> >                 return starfive_jh7110_inst_init(fdt);
> >         }
> >
> > --
> > 2.36.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi



-- 
Best Regards
 Guo Ren



More information about the opensbi mailing list