[PATCH 0/5] starfive reboot cleanup & fix

[PATCH 0/5] starfive reboot cleanup & fix

[Nam Cao]

This series:
  - add gracefully error handling when reboot devices are not present
  - correct a device tree compatibility string
  - some minor cleanup

This series complements my other series:
http://lists.infradead.org/pipermail/opensbi/2024-January/006300.html

Nam Cao (5):
  platform: starfive: remove redundant compatibility check in pmic_ops
  platform: starfive: rename "stf,axp15060-regulator" ->
    "x-powers,axp15060"
  platform: starfive: drop "compatible" from struct pmic
  platform: starfive: return error if needed devices are not present
  platform: starfive: call starfive_jh7110_inst_init() in
    pm_reset_init()

 platform/generic/starfive/jh7110.c | 49 ++++++++++++++++--------------
 1 file changed, 27 insertions(+), 22 deletions(-)

-- 
2.39.2

[PATCH 1/5] platform: starfive: remove redundant compatibility check in pmic_ops

[Nam Cao]

pmic_ops() is only called if a compatible device is found in device
tree. It is redundant for this function to check the compability again.
Remove this check.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 platform/generic/starfive/jh7110.c | 36 ++++++++++++++----------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index dcd6306..72969e4 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -134,28 +134,26 @@ static void pmic_ops(struct pmic *pmic, int type)
 	if (ret)
 		return;
 
-	if (!sbi_strcmp("stf,axp15060-regulator", pmic->compatible)) {
-		ret = i2c_adapter_reg_read(pmic->adapter, pmic->dev_addr,
-					   AXP15060_POWER_REG, &val);
-
-		if (ret) {
-			sbi_printf("%s: cannot read pmic power register\n",
-				   __func__);
-			return;
-		}
+	ret = i2c_adapter_reg_read(pmic->adapter, pmic->dev_addr,
+					AXP15060_POWER_REG, &val);
+
+	if (ret) {
+		sbi_printf("%s: cannot read pmic power register\n",
+				__func__);
+		return;
+	}
 
+	val |= AXP15060_POWER_OFF_BIT;
+	if (type == SBI_SRST_RESET_TYPE_SHUTDOWN)
 		val |= AXP15060_POWER_OFF_BIT;
-		if (type == SBI_SRST_RESET_TYPE_SHUTDOWN)
-			val |= AXP15060_POWER_OFF_BIT;
-		else
-			val |= AXP15060_RESET_BIT;
+	else
+		val |= AXP15060_RESET_BIT;
 
-		ret = i2c_adapter_reg_write(pmic->adapter, pmic->dev_addr,
-					    AXP15060_POWER_REG, val);
-		if (ret)
-			sbi_printf("%s: cannot write pmic power register\n",
-				   __func__);
-	}
+	ret = i2c_adapter_reg_write(pmic->adapter, pmic->dev_addr,
+					AXP15060_POWER_REG, val);
+	if (ret)
+		sbi_printf("%s: cannot write pmic power register\n",
+				__func__);
 }
 
 static void pmic_i2c_clk_enable(void)
-- 
2.39.2

[PATCH 2/5] platform: starfive: rename "stf,axp15060-regulator" -> "x-powers,axp15060"

[Nam Cao]

OpenSBI uses the device tree compatible string "stf,axp15060-regulator"
for the regulator node. However, the string used by U-Boot (and Linux)
is actually "x-powers,axp15060". As OpenSBI gets the device tree from
U-Boot, this causes the regulator device to be undetected, and OpenSBI
does not use this device to perform board reset/shutdown.

Rename this device tree compatible string to match U-Boot (and Linux).

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
Actually "x-powers,axp15060" does not exist in U-Boot at the moment. But
it is being added:
https://lore.kernel.org/u-boot/47fd2b2225f52a2d5c3a91494ef7b9036e944199.1706517351.git.namcao at linutronix.de/

 platform/generic/starfive/jh7110.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index 72969e4..9f5261b 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -227,7 +227,7 @@ static int pm_reset_init(void *fdt, int nodeoff,
 }
 
 static const struct fdt_match pm_reset_match[] = {
-	{ .compatible = "stf,axp15060-regulator", .data = (void *)true },
+	{ .compatible = "x-powers,axp15060", .data = (void *)true },
 	{ },
 };
 
-- 
2.39.2

[PATCH 3/5] platform: starfive: drop "compatible" from struct pmic

[Nam Cao]

The member "compatible" of struct pmic is not used. Delete it.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 platform/generic/starfive/jh7110.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index 9f5261b..cb8c48d 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -23,7 +23,6 @@
 struct pmic {
 	struct i2c_adapter *adapter;
 	u32 dev_addr;
-	const char *compatible;
 };
 
 struct jh7110 {
@@ -208,7 +207,6 @@ static int pm_reset_init(void *fdt, int nodeoff,
 		return rc;
 
 	pmic_inst.dev_addr = addr;
-	pmic_inst.compatible = match->compatible;
 
 	i2c_bus = fdt_parent_offset(fdt, nodeoff);
 	if (i2c_bus < 0)
-- 
2.39.2

[PATCH 4/5] platform: starfive: return error if needed devices are not present

[Nam Cao]

Jh7110's reset driver needs power management device and clock controller
device to work. Currently, the driver proceed anyway without these
devices, and invalid addresses (jh7110_inst.pmu_reg_base and
jh7110_inst.clk_reg_base) are used during reboot, which causes
unpredictable broken behaviors.

If these devices are not present, return -SBI_ENODEV.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 platform/generic/starfive/jh7110.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index cb8c48d..f9942d3 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -246,6 +246,8 @@ static int starfive_jh7110_inst_init(void *fdt)
 		if (rc)
 			goto err;
 		jh7110_inst.pmu_reg_base = addr;
+	} else {
+		return -SBI_ENODEV;
 	}
 
 	noff = fdt_node_offset_by_compatible(fdt, -1, "starfive,jh7110-clkgen");
@@ -254,6 +256,8 @@ static int starfive_jh7110_inst_init(void *fdt)
 		if (rc)
 			goto err;
 		jh7110_inst.clk_reg_base = addr;
+	} else {
+		return -SBI_ENODEV;
 	}
 
 	if (pmic_inst.adapter) {
-- 
2.39.2

[PATCH 5/5] platform: starfive: call starfive_jh7110_inst_init() in pm_reset_init()

[Nam Cao]

The function starfive_jh7110_inst_init() initialize some power
management unit address and clock addresses, needed for the reset
driver. It doesn't do anything else, and also the reset driver doesn't
work without calling this function. Thus, it does not make much sense
that this function is independent from pm_reset_init().

Delete the separate call to starfive_jh7110_inst_init(), and instead
just call this function inside pm_reset_init().

Doing this also fixes another problem: if starfive_jh7110_inst_init()
returns an error code, it gets propagated to final_init() and OpenSBI
hangs. This hang is not necessary, because failures within
starfive_jh7110_inst_init() only mean OpenSBI cannot perform reboot or
shutdown, but the system can still function normally.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 platform/generic/starfive/jh7110.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/platform/generic/starfive/jh7110.c b/platform/generic/starfive/jh7110.c
index f9942d3..520bc8a 100644
--- a/platform/generic/starfive/jh7110.c
+++ b/platform/generic/starfive/jh7110.c
@@ -194,6 +194,8 @@ static struct sbi_system_reset_device pm_reset = {
 	.system_reset = pm_system_reset
 };
 
+static int starfive_jh7110_inst_init(void *fdt);
+
 static int pm_reset_init(void *fdt, int nodeoff,
 			 const struct fdt_match *match)
 {
@@ -219,6 +221,10 @@ static int pm_reset_init(void *fdt, int nodeoff,
 
 	pmic_inst.adapter = adapter;
 
+	rc = starfive_jh7110_inst_init(fdt);
+	if (rc)
+		return rc;
+
 	sbi_system_reset_add_device(&pm_reset);
 
 	return 0;
@@ -278,7 +284,6 @@ static int starfive_jh7110_final_init(bool cold_boot,
 
 	if (cold_boot) {
 		fdt_reset_driver_init(fdt, &fdt_reset_pmic);
-		return starfive_jh7110_inst_init(fdt);
 	}
 
 	return 0;
-- 
2.39.2

[PATCH 0/5] starfive reboot cleanup & fix

[Anup Patel]

On Wed, Jan 31, 2024 at 6:39?PM Nam Cao <namcao@linutronix.de> wrote:
>
> This series:
>   - add gracefully error handling when reboot devices are not present
>   - correct a device tree compatibility string
>   - some minor cleanup
>
> This series complements my other series:
> http://lists.infradead.org/pipermail/opensbi/2024-January/006300.html
>
> Nam Cao (5):
>   platform: starfive: remove redundant compatibility check in pmic_ops
>   platform: starfive: rename "stf,axp15060-regulator" ->
>     "x-powers,axp15060"
>   platform: starfive: drop "compatible" from struct pmic
>   platform: starfive: return error if needed devices are not present
>   platform: starfive: call starfive_jh7110_inst_init() in
>     pm_reset_init()

Can someone from StarFive review and test this series ?

Regards,
Anup

>
>  platform/generic/starfive/jh7110.c | 49 ++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
>
> --
> 2.39.2
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/5] starfive reboot cleanup & fix

[Minda Chen]

> On Wed, Jan 31, 2024 at 6:39?PM Nam Cao <namcao@linutronix.de> wrote:
> >
> > This series:
> >   - add gracefully error handling when reboot devices are not present
> >   - correct a device tree compatibility string
> >   - some minor cleanup
> >
> > This series complements my other series:
> > http://lists.infradead.org/pipermail/opensbi/2024-January/006300.html
> >
> > Nam Cao (5):
> >   platform: starfive: remove redundant compatibility check in pmic_ops
> >   platform: starfive: rename "stf,axp15060-regulator" ->
> >     "x-powers,axp15060"
> >   platform: starfive: drop "compatible" from struct pmic
> >   platform: starfive: return error if needed devices are not present
> >   platform: starfive: call starfive_jh7110_inst_init() in
> >     pm_reset_init()
> 
> Can someone from StarFive review and test this series ?
> 
> Regards,
> Anup
> 
Let me review these. But I don't receive this series patch.

Minda
> >
> >  platform/generic/starfive/jh7110.c | 49 ++++++++++++++++--------------
> >  1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > --
> > 2.39.2
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/5] starfive reboot cleanup & fix

[Nam Cao]

On Thu, 1 Feb 2024 05:38:16 +0000 Minda Chen <minda.chen@starfivetech.com> wrote:
> > On Wed, Jan 31, 2024 at 6:39?PM Nam Cao <namcao@linutronix.de> wrote:  
> > >
> > > This series:
> > >   - add gracefully error handling when reboot devices are not present
> > >   - correct a device tree compatibility string
> > >   - some minor cleanup
> > >
> > > This series complements my other series:
> > > http://lists.infradead.org/pipermail/opensbi/2024-January/006300.html
> > >
> > > Nam Cao (5):
> > >   platform: starfive: remove redundant compatibility check in pmic_ops
> > >   platform: starfive: rename "stf,axp15060-regulator" ->
> > >     "x-powers,axp15060"
> > >   platform: starfive: drop "compatible" from struct pmic
> > >   platform: starfive: return error if needed devices are not present
> > >   platform: starfive: call starfive_jh7110_inst_init() in
> > >     pm_reset_init()  
> > 
> > Can someone from StarFive review and test this series ?
> > 
> > Regards,
> > Anup
> >   
> Let me review these. But I don't receive this series patch.

I only sent it to the list. Can you grab the series from there?

Or I can also resend the series and include you as a recipient, if that's
more convenient.

Best regards,
Nam

[PATCH 0/5] starfive reboot cleanup & fix

[Minda Chen]

> 
> On Thu, 1 Feb 2024 05:38:16 +0000 Minda Chen
> <minda.chen@starfivetech.com> wrote:
> > > On Wed, Jan 31, 2024 at 6:39?PM Nam Cao <namcao@linutronix.de> wrote:
> > > >
> > > > This series:
> > > >   - add gracefully error handling when reboot devices are not present
> > > >   - correct a device tree compatibility string
> > > >   - some minor cleanup
> > > >
> > > > This series complements my other series:
> > > > http://lists.infradead.org/pipermail/opensbi/2024-January/006300.h
> > > > tml
> > > >
> > > > Nam Cao (5):
> > > >   platform: starfive: remove redundant compatibility check in pmic_ops
> > > >   platform: starfive: rename "stf,axp15060-regulator" ->
> > > >     "x-powers,axp15060"
> > > >   platform: starfive: drop "compatible" from struct pmic
> > > >   platform: starfive: return error if needed devices are not present
> > > >   platform: starfive: call starfive_jh7110_inst_init() in
> > > >     pm_reset_init()
> > >
> > > Can someone from StarFive review and test this series ?
> > >
> > > Regards,
> > > Anup
> > >
> > Let me review these. But I don't receive this series patch.
> 
> I only sent it to the list. Can you grab the series from there?
> 
> Or I can also resend the series and include you as a recipient, if that's more
> convenient.
> 
> Best regards,
> Nam

Resend the patchset is better. Thanks
Minda