lib: reset: Add early_init api for fdt_reset_drv

PATCH New [email protected] 2 participants 3 messages Jun 17, 2023 – Jun 18, 2023 SiFive drivers

lore

Share & tools

lore

[email protected]

author

2023-06-17 05:26 UTC #0 lore raw

PATCH 1/1 lib: reset: Add early_init api for fdt_reset_drv

From: Guo Ren <[email protected]>

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 <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
 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(-)

include/sbi_utils/reset/fdt_reset.h +5 -4

index ea8063b8a90e..2b1849534e2b 100644
@@ -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

lib/utils/reset/fdt_reset.c +13 -7

index 4334586d39f7..a99d8ecc5907 100644
@@ -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);
 }

lib/utils/reset/fdt_reset_thead.c +1 -1

index e1d6885debae..4cae037b1db7 100644
@@ -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
 };

platform/generic/platform.c +4 -1

index eeefef4c9533..65134e2c3895 100644
@@ -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);
 
 	if (generic_plat && generic_plat->final_init) {
 		rc = generic_plat->final_init(cold_boot, generic_plat_match);

platform/generic/sifive/fu740.c +1 -1

index fe71ce6a9ea5..5d3f1388beef 100644
@@ -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__);

platform/generic/starfive/jh7110.c +1 -1

index dcd6306be5fc..f21fe85c6560 100644
@@ -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);
 	}

Signature · 1 line

2.36.1

Anup Patel

2023-06-17 06:43 UTC #1 lore raw

PATCH 1/1 lib: reset: Add early_init api for fdt_reset_drv

Quoted 147 lines · On Sat, Jun 17, 2023 at 10:57?AM <[email protected]> wrote:

From: Guo Ren <[email protected]>

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.

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 <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
 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(-)

include/sbi_utils/reset/fdt_reset.h +5 -4

index ea8063b8a90e..2b1849534e2b 100644
@@ -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

lib/utils/reset/fdt_reset.c +13 -7

index 4334586d39f7..a99d8ecc5907 100644
@@ -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);
 }

lib/utils/reset/fdt_reset_thead.c +1 -1

index e1d6885debae..4cae037b1db7 100644
@@ -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
 };

platform/generic/platform.c +3 -0

index eeefef4c9533..65134e2c3895 100644
@@ -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);
 }

platform/generic/platform.c static int generic_final_init(bool cold_boot) · line 161

Quoted 5 lines

platform/generic/platform.c +1 -1

@@ -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)".

Regards,
Anup

Quoted 37 lines

platform/generic/platform.c +0 -0

        if (generic_plat && generic_plat->final_init) {
                rc = generic_plat->final_init(cold_boot, generic_plat_match);

platform/generic/sifive/fu740.c +1 -1

index fe71ce6a9ea5..5d3f1388beef 100644
@@ -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__);

platform/generic/starfive/jh7110.c +1 -2

index dcd6306be5fc..f21fe85c6560 100644
@@ -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

Guo Ren

author

2023-06-18 15:11 UTC #2 lore raw

PATCH 1/1 lib: reset: Add early_init api for fdt_reset_drv

Quoted 151 lines · On Sat, Jun 17, 2023 at 2:43?AM Anup Patel <[email protected]> wrote:

Quoted 149 lines · On Sat, Jun 17, 2023 at 10:57?AM <[email protected]> wrote:

From: Guo Ren <[email protected]>

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.

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 <[email protected]>

Signed-off-by Guo Ren <[email protected]>

---
 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(-)

include/sbi_utils/reset/fdt_reset.h +5 -4

index ea8063b8a90e..2b1849534e2b 100644
@@ -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

lib/utils/reset/fdt_reset.c +13 -7

index 4334586d39f7..a99d8ecc5907 100644
@@ -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);
 }

lib/utils/reset/fdt_reset_thead.c +1 -1

index e1d6885debae..4cae037b1db7 100644
@@ -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
 };

platform/generic/platform.c +3 -0

index eeefef4c9533..65134e2c3895 100644
@@ -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;

Quoted 8 lines

platform/generic/platform.c static int generic_final_init(bool cold_boot) · line 161
Quoted 3 lines
platform/generic/platform.c +1 -1
        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.

Quoted 41 lines

Regards,
Anup

Quoted 37 lines

platform/generic/platform.c +0 -0

        if (generic_plat && generic_plat->final_init) {
                rc = generic_plat->final_init(cold_boot, generic_plat_match);

platform/generic/sifive/fu740.c +1 -1

index fe71ce6a9ea5..5d3f1388beef 100644
@@ -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__);

platform/generic/starfive/jh7110.c +1 -2

index dcd6306be5fc..f21fe85c6560 100644
@@ -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

Signature · 2 lines

Best Regards
 Guo Ren