[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