[PATCH v3 0/2] hwrng: starfive: updates for jh7110-trng DT binding and driver fixes

[PATCH v3 0/2] hwrng: starfive: updates for jh7110-trng DT binding and driver fixes

[lianfeng.ouyang]

From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>

- the obsolete JH8100 compatible string is dropped, support for the new
  JHB100 SoC is added, and the schema is simplified to an enum.
- The driver is updated to handle the JHB100's specific hardware
  requirement where clocks must be disabled beforeasserting reset during
  teardown, to avoid glitches. This is implemented via a per-compatible
  data flag (SEQ_CLK_FIRSTfor JHB100, preserving SEQ_RST_FIRSTfor JH7110).
- Several Runtime Power Management (RPM) fixes are included to ensure
  proper autosuspend behavior, balanced runtime usage counting, and
  correct cleanup ordering on probe failure or device removal.

Lianfeng Ouyang (2):
  dt-bindings: rng: starfive,jh7110-trng: add jhb100, drop jh8100
  hwrng: starfive: rework clk/reset teardown order for JHB100 & fix RPM

 .../bindings/rng/starfive,jh7110-trng.yaml    |  10 +-
 MAINTAINERS                                   |   2 +-
 drivers/char/hw_random/jh7110-trng.c          | 191 ++++++++++++++----
 3 files changed, 152 insertions(+), 51 deletions(-)

--
2.43.0

[PATCH v3 1/2] dt-bindings: rng: starfive,jh7110-trng: add jhb100, drop jh8100

[lianfeng.ouyang]

From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>

- Drop "starfive,jh8100-trng" since JH8100 SoC is no longer
  supported
- Add "starfive,jhb100-trng" for the JHB100 SoC TRNG.
- Update maintainer to current owner

Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
---
 .../devicetree/bindings/rng/starfive,jh7110-trng.yaml  | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml b/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
index 4639247e9e51..d21769b7d54e 100644
--- a/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
+++ b/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
@@ -7,15 +7,13 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: StarFive SoC TRNG Module
 
 maintainers:
-  - Jia Jie Ho <jiajie.ho@starfivetech.com>
+  - Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
 
 properties:
   compatible:
-    oneOf:
-      - items:
-          - const: starfive,jh8100-trng
-          - const: starfive,jh7110-trng
-      - const: starfive,jh7110-trng
+    enum:
+      - starfive,jh7110-trng
+      - starfive,jhb100-trng
 
   reg:
     maxItems: 1
-- 
2.43.0

[PATCH v3 2/2] hwrng: starfive: rework clk/reset teardown order for JHB100 & fix RPM

[lianfeng.ouyang]

From: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>

JHB100: to avoid RDC glitch, assert reset only after clocks are gated.
  Introduce a per-compatible seq_rst_clk flag (RST_FIRST/CLK_FIRST)
  selected via of_device_get_match_data().

RPM fixes:
- Mark device RPM_ACTIVE in probe after clocks & reset deassert,
  before pm_runtime_enable(), so usage count can eventually drop to 0
  and autosuspend actually triggers.
- Balance pm_runtime_get/put in init/read/cleanup, and reject
  register access when RPM resume fails.
- Move low-level disable/reset into a devm action
  (starfive_trng_release()) so error-path unwind order is correct.

Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
---
 MAINTAINERS                          |   2 +-
 drivers/char/hw_random/jh7110-trng.c | 191 +++++++++++++++++++++------
 2 files changed, 148 insertions(+), 45 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d3a6b3f6b6a0..729b20ecc697 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25280,7 +25280,7 @@ F:	Documentation/devicetree/bindings/perf/starfive,jh8100-starlink-pmu.yaml
 F:	drivers/perf/starfive_starlink_pmu.c
 
 STARFIVE TRNG DRIVER
-M:	Jia Jie Ho <jiajie.ho@starfivetech.com>
+M:	Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
 S:	Supported
 F:	Documentation/devicetree/bindings/rng/starfive*
 F:	drivers/char/hw_random/jh7110-trng.c
diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
index 9776f4daa044..9c164a0011ae 100644
--- a/drivers/char/hw_random/jh7110-trng.c
+++ b/drivers/char/hw_random/jh7110-trng.c
@@ -92,20 +92,36 @@ enum mode {
 	PRNG_256BIT,
 };
 
+/*
+ * For JHB100, assert reset after disabling clocks to avoid
+ * reset-domain crossing (RDC) induced glitches that can affect
+ * downstream IPs.
+ */
+enum seq_rst_clk {
+	SEQ_RST_FIRST,
+	SEQ_CLK_FIRST,
+};
+
+struct starfive_trng_data {
+	enum seq_rst_clk	seq_rst_clk;
+};
+
 struct starfive_trng {
-	struct device		*dev;
-	void __iomem		*base;
-	struct clk		*hclk;
-	struct clk		*ahb;
-	struct reset_control	*rst;
-	struct hwrng		rng;
-	struct completion	random_done;
-	struct completion	reseed_done;
-	u32			mode;
-	u32			mission;
-	u32			reseed;
-	/* protects against concurrent write to ctrl register */
-	spinlock_t		write_lock;
+	struct device			*dev;
+	void __iomem			*base;
+	int				irq;
+	struct clk			*hclk;
+	struct clk			*ahb;
+	struct reset_control		*rst;
+	struct hwrng			rng;
+	struct completion		random_done;
+	struct completion		reseed_done;
+	const struct starfive_trng_data *data;
+	u32				mode;
+	u32				mission;
+	u32				reseed;
+	struct mutex			lock; /* protect trng cmd seq */
+	spinlock_t			write_lock; /* protects register access seq */
 };
 
 static u16 autoreq;
@@ -130,7 +146,7 @@ static inline int starfive_trng_wait_idle(struct starfive_trng *trng)
 					  10, 100000);
 }
 
-static inline void starfive_trng_irq_mask_clear(struct starfive_trng *trng)
+static inline void starfive_trng_irq_clear(struct starfive_trng *trng)
 {
 	/* clear register: ISTAT */
 	u32 data = readl(trng->base + STARFIVE_ISTAT);
@@ -138,6 +154,31 @@ static inline void starfive_trng_irq_mask_clear(struct starfive_trng *trng)
 	writel(data, trng->base + STARFIVE_ISTAT);
 }
 
+static void starfive_trng_release(void *data)
+{
+	struct starfive_trng *trng = data;
+
+	if (!pm_runtime_status_suspended(trng->dev)) {
+		writel(0, trng->base + STARFIVE_IE);
+		starfive_trng_irq_clear(trng);
+
+		if (trng->irq >= 0)
+			synchronize_irq(trng->irq);
+
+		if (trng->data->seq_rst_clk == SEQ_RST_FIRST)
+			reset_control_assert(trng->rst);
+
+		clk_disable_unprepare(trng->ahb);
+		clk_disable_unprepare(trng->hclk);
+
+		if (trng->data->seq_rst_clk == SEQ_CLK_FIRST)
+			reset_control_assert(trng->rst);
+	}
+
+	pm_runtime_dont_use_autosuspend(trng->dev);
+	pm_runtime_disable(trng->dev);
+}
+
 static int starfive_trng_cmd(struct starfive_trng *trng, u32 cmd, bool wait)
 {
 	int wait_time = 1000;
@@ -174,13 +215,22 @@ static int starfive_trng_init(struct hwrng *rng)
 {
 	struct starfive_trng *trng = to_trng(rng);
 	u32 mode, intr = 0;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(trng->dev);
+	if (ret < 0) {
+		dev_warn(trng->dev, "Failed to wake device for init: %d\n", ret);
+		return ret;
+	}
+
+	mutex_lock(&trng->lock);
 
 	/* setup Auto Request/Age register */
 	writel(autoage, trng->base + STARFIVE_AUTO_AGE);
 	writel(autoreq, trng->base + STARFIVE_AUTO_RQSTS);
 
 	/* clear register: ISTAT */
-	starfive_trng_irq_mask_clear(trng);
+	starfive_trng_irq_clear(trng);
 
 	intr |= STARFIVE_IE_ALL;
 	writel(intr, trng->base + STARFIVE_IE);
@@ -201,7 +251,13 @@ static int starfive_trng_init(struct hwrng *rng)
 
 	writel(mode, trng->base + STARFIVE_MODE);
 
-	return starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_RANDRESEED, 1);
+	ret = starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_RANDRESEED, 1);
+
+	mutex_unlock(&trng->lock);
+
+	pm_runtime_put_autosuspend(trng->dev);
+
+	return ret;
 }
 
 static irqreturn_t starfive_trng_irq(int irq, void *priv)
@@ -209,16 +265,17 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
 	u32 status;
 	struct starfive_trng *trng = (struct starfive_trng *)priv;
 
+	if (!pm_runtime_get_if_active(trng->dev)) {
+		dev_err_ratelimited(trng->dev, "pm is inactive in irq\n");
+		return IRQ_NONE;
+	}
+
 	status = readl(trng->base + STARFIVE_ISTAT);
-	if (status & STARFIVE_ISTAT_RAND_RDY) {
+	if (status & STARFIVE_ISTAT_RAND_RDY)
 		writel(STARFIVE_ISTAT_RAND_RDY, trng->base + STARFIVE_ISTAT);
-		complete(&trng->random_done);
-	}
 
-	if (status & STARFIVE_ISTAT_SEED_DONE) {
+	if (status & STARFIVE_ISTAT_SEED_DONE)
 		writel(STARFIVE_ISTAT_SEED_DONE, trng->base + STARFIVE_ISTAT);
-		complete(&trng->reseed_done);
-	}
 
 	if (status & STARFIVE_ISTAT_LFSR_LOCKUP) {
 		writel(STARFIVE_ISTAT_LFSR_LOCKUP, trng->base + STARFIVE_ISTAT);
@@ -228,18 +285,37 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
 		spin_unlock(&trng->write_lock);
 	}
 
+	if (status & STARFIVE_ISTAT_RAND_RDY)
+		complete(&trng->random_done);
+
+	if (status & STARFIVE_ISTAT_SEED_DONE)
+		complete(&trng->reseed_done);
+
+	pm_runtime_put_noidle(trng->dev);
+
 	return IRQ_HANDLED;
 }
 
 static void starfive_trng_cleanup(struct hwrng *rng)
 {
 	struct starfive_trng *trng = to_trng(rng);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(trng->dev);
+	if (ret < 0) {
+		dev_warn(trng->dev, "Failed to wake device for cleanup: %d\n", ret);
+		return;
+	}
+
+	writel(0, trng->base + STARFIVE_IE);
+	starfive_trng_irq_clear(trng);
+
+	if (trng->irq >= 0)
+		synchronize_irq(trng->irq);
 
 	writel(0, trng->base + STARFIVE_CTRL);
 
-	reset_control_assert(trng->rst);
-	clk_disable_unprepare(trng->hclk);
-	clk_disable_unprepare(trng->ahb);
+	pm_runtime_put_sync(trng->dev);
 }
 
 static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
@@ -247,7 +323,13 @@ static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wai
 	struct starfive_trng *trng = to_trng(rng);
 	int ret;
 
-	pm_runtime_get_sync(trng->dev);
+	ret = pm_runtime_resume_and_get(trng->dev);
+	if (ret < 0) {
+		dev_warn(trng->dev, "Failed to wake device for read: %d\n", ret);
+		return ret;
+	}
+
+	mutex_lock(&trng->lock);
 
 	if (trng->mode == PRNG_256BIT)
 		max = min_t(size_t, max, (STARFIVE_RAND_LEN * 8));
@@ -257,24 +339,28 @@ static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wai
 	if (wait) {
 		ret = starfive_trng_wait_idle(trng);
 		if (ret)
-			return -ETIMEDOUT;
+			goto end;
 	}
 
 	ret = starfive_trng_cmd(trng, STARFIVE_CTRL_GENE_RANDNUM, wait);
 	if (ret)
-		return ret;
+		goto end;
 
 	memcpy_fromio(buf, trng->base + STARFIVE_RAND0, max);
 
-	pm_runtime_put_sync_autosuspend(trng->dev);
+	ret = max;
+
+end:
+	mutex_unlock(&trng->lock);
 
-	return max;
+	pm_runtime_put_autosuspend(trng->dev);
+
+	return ret;
 }
 
 static int starfive_trng_probe(struct platform_device *pdev)
 {
 	int ret;
-	int irq;
 	struct starfive_trng *trng;
 
 	trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
@@ -282,22 +368,32 @@ static int starfive_trng_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, trng);
+
 	trng->dev = &pdev->dev;
+	trng->data = of_device_get_match_data(&pdev->dev);
+	if (!trng->data)
+		return -EINVAL;
+
+	if (trng->data->seq_rst_clk != SEQ_RST_FIRST && trng->data->seq_rst_clk != SEQ_CLK_FIRST) {
+		dev_err(&pdev->dev, "Unknown seq_rst_clk value\n");
+		return -EINVAL;
+	}
 
 	trng->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(trng->base))
 		return dev_err_probe(&pdev->dev, PTR_ERR(trng->base),
 				     "Error remapping memory for platform device.\n");
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	trng->irq = platform_get_irq(pdev, 0);
+	if (trng->irq < 0)
+		return trng->irq;
 
 	init_completion(&trng->random_done);
 	init_completion(&trng->reseed_done);
+	mutex_init(&trng->lock);
 	spin_lock_init(&trng->write_lock);
 
-	ret = devm_request_irq(&pdev->dev, irq, starfive_trng_irq, 0, pdev->name,
+	ret = devm_request_irq(&pdev->dev, trng->irq, starfive_trng_irq, 0, pdev->name,
 			       (void *)trng);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret,
@@ -333,18 +429,16 @@ static int starfive_trng_probe(struct platform_device *pdev)
 
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
+	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
-	if (ret) {
-		pm_runtime_disable(&pdev->dev);
-
-		reset_control_assert(trng->rst);
-		clk_disable_unprepare(trng->ahb);
-		clk_disable_unprepare(trng->hclk);
+	ret = devm_add_action_or_reset(&pdev->dev, starfive_trng_release, trng);
+	if (ret)
+		return ret;
 
+	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
+	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "Failed to register hwrng\n");
-	}
 
 	return 0;
 }
@@ -376,8 +470,17 @@ static const struct dev_pm_ops starfive_trng_pm_ops = {
 			   starfive_trng_resume, NULL)
 };
 
+static const struct starfive_trng_data jh7110_data = {
+	.seq_rst_clk = SEQ_RST_FIRST,
+};
+
+static const struct starfive_trng_data jhb100_data = {
+	.seq_rst_clk = SEQ_CLK_FIRST,
+};
+
 static const struct of_device_id trng_dt_ids[] __maybe_unused = {
-	{ .compatible = "starfive,jh7110-trng" },
+	{ .compatible = "starfive,jh7110-trng", .data = &jh7110_data },
+	{ .compatible = "starfive,jhb100-trng", .data = &jhb100_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, trng_dt_ids);
-- 
2.43.0

Re: [PATCH v3 2/2] hwrng: starfive: rework clk/reset teardown order for JHB100 & fix RPM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] IRQ handler incorrectly evaluates pm_runtime_get_if_active() return value, leading to PM usage counter underflow and fatal unclocked hardware access.

Pre-existing issues:
- [High] Missing return value checks for clk_prepare_enable() leads to potential kernel panics on resource activation failure.
--

commit b2fbed65d08f91e4f7234550dff66d9c5c318e6a
Author: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>

hwrng: starfive: rework clk/reset teardown order for JHB100 & fix RPM
    
JHB100: to avoid RDC glitch, assert reset only after clocks are gated.
Introduce a per-compatible seq_rst_clk flag (RST_FIRST/CLK_FIRST)
selected via of_device_get_match_data().

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> index 9776f4daa0445..9c164a0011ae6 100644
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c

[ ... ]

> @@ -209,16 +265,17 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
>  	u32 status;
>  	struct starfive_trng *trng = (struct starfive_trng *)priv;
>  
> +	if (!pm_runtime_get_if_active(trng->dev)) {

[Severity: High]
Does this evaluation properly handle negative error codes?

If pm_runtime_get_if_active() returns -EINVAL when Runtime PM is disabled
(for example, during probe before pm_runtime_enable() is called), the
!(-EINVAL) check evaluates to false.

Since devm_request_irq() is called before clocks are enabled in probe, a
pending interrupt from a previous boot stage could trigger this handler. It
would bypass the early return and perform an MMIO read on STARFIVE_ISTAT
while clocks are off, which might cause a synchronous external abort.

Could we check for ret <= 0 instead?

> +		dev_err_ratelimited(trng->dev, "pm is inactive in irq\n");
> +		return IRQ_NONE;
> +	}

[ ... ]

> @@ -228,18 +285,37 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
>  		spin_unlock(&trng->write_lock);
>  	}
>  
> +	if (status & STARFIVE_ISTAT_RAND_RDY)
> +		complete(&trng->random_done);
> +
> +	if (status & STARFIVE_ISTAT_SEED_DONE)
> +		complete(&trng->reseed_done);
> +
> +	pm_runtime_put_noidle(trng->dev);

[Severity: High]
Could this lead to a PM usage counter underflow?

If pm_runtime_get_if_active() returned -EINVAL earlier, we didn't increment
the usage counter, but we unconditionally decrement it here. This might steal
the reference held by the driver core during probe.

> +
>  	return IRQ_HANDLED;
>  }

[ ... ]

> @@ -333,18 +429,16 @@ static int starfive_trng_probe(struct platform_device *pdev)
>  
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> +	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> -	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
> -	if (ret) {
> -		pm_runtime_disable(&pdev->dev);
> -
> -		reset_control_assert(trng->rst);
> -		clk_disable_unprepare(trng->ahb);
> -		clk_disable_unprepare(trng->hclk);

[Severity: High]
This isn't a bug introduced by this patch, but should the return values of
the clock enable calls be verified?

In starfive_trng_probe() and starfive_trng_resume(), clk_prepare_enable()
is called for both the hclk and ahb clocks without checking for failure:

clk_prepare_enable(trng->hclk);
clk_prepare_enable(trng->ahb);

If clock activation fails under resource pressure, the driver will proceed as
if the hardware is powered. Subsequent MMIO accesses might trigger a
fatal bus error.

> +	ret = devm_add_action_or_reset(&pdev->dev, starfive_trng_release, trng);
> +	if (ret)
> +		return ret;
>  
> +	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
> +	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "Failed to register hwrng\n");
> -	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601093744.84210-1-lianfeng.ouyang@starfivetech.com?part=2