Message-ID: <b48fafecc821d2099ff7e84477f36161bae13293.camel@pengutronix.de>
Date: Fri, 05 Sep 2025 16:46:53 +0200
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Prabhakar <prabhakar.csengg@...il.com>, Geert Uytterhoeven
	 <geert+renesas@...der.be>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>
Cc: linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org, 
 linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
 Fabrizio Castro <fabrizio.castro.jz@...esas.com>, Lad Prabhakar
 <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v3] clk: renesas: cpg-mssr: Add module reset support for
 RZ/T2H

On Fr, 2025-09-05 at 12:45 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> 
> Add support for module reset handling on the RZ/T2H SoC. Unlike earlier
> CPG/MSSR variants, RZ/T2H uses a unified set of Module Reset Control
> Registers (MRCR) where both reset and deassert actions are done via
> read-modify-write (RMW) to the same register.
> 
> Introduce a new MRCR offset table (mrcr_for_rzt2h) for RZ/T2H and assign
> it to reset_regs. For this SoC, the number of resets is based on the
> number of MRCR registers rather than the number of module clocks. Also
> add cpg_mrcr_reset_ops to implement reset, assert, and deassert using RMW
> while holding the spinlock. This follows the RZ/T2H requirements, where
> processing after releasing a module reset must be secured by performing
> seven dummy reads of the same register, and where a module that is reset
> and released again must ensure the target bit in the Module Reset Control
> Register is set to 1.
> 
> Update the reset controller registration to select cpg_mrcr_reset_ops for
> RZ/T2H, while keeping the existing cpg_mssr_reset_ops for other SoCs.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> v2->v3:
> - Simplifed the code by adding a common function cpg_mrcr_set_bit() to handle
>   set/clear of bits with options for verify and dummy reads.
> - Added a macro for the number of dummy reads required.
>   
> v1->v2:
> - Added cpg_mrcr_reset_ops for RZ/T2H specific handling
> - Updated commit message
> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 112 ++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 5ff6ee1f7d4b..81206db2b873 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -40,6 +40,8 @@
>  #define WARN_DEBUG(x)	do { } while (0)
>  #endif
>  
> +#define RZT2H_RESET_REG_READ_COUNT	7
> +
>  /*
>   * Module Standby and Software Reset register offets.
>   *
> @@ -137,6 +139,22 @@ static const u16 srcr_for_gen4[] = {
>  	0x2C60, 0x2C64, 0x2C68, 0x2C6C, 0x2C70, 0x2C74,
>  };
>  
> +static const u16 mrcr_for_rzt2h[] = {
> +	0x240,	/* MRCTLA */
> +	0x244,	/* Reserved */
> +	0x248,	/* Reserved */
> +	0x24C,	/* Reserved */
> +	0x250,	/* MRCTLE */
> +	0x254,	/* Reserved */
> +	0x258,	/* Reserved */
> +	0x25C,	/* Reserved */
> +	0x260,	/* MRCTLI */
> +	0x264,	/* Reserved */
> +	0x268,	/* Reserved */
> +	0x26C,	/* Reserved */
> +	0x270,	/* MRCTLM */
> +};
> +
>  /*
>   * Software Reset Clearing Register offsets
>   */
> @@ -736,6 +754,73 @@ static int cpg_mssr_status(struct reset_controller_dev *rcdev,
>  	return !!(readl(priv->pub.base0 + priv->reset_regs[reg]) & bitmask);
>  }
>  
> +static int cpg_mrcr_set_bit(struct reset_controller_dev *rcdev, unsigned long id,
> +			    bool set, bool verify, bool dummy_reads, const char *op_name)

This function is inappropriately named, it does more than just set a
bit.

Why are there three boolean parameters if there are only ever two
combinations of them used? Just have a single bool assert.

Drop the op_name parameter.

> +{
> +	struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +	unsigned int reg = id / 32;
> +	unsigned int bit = id % 32;
> +	u32 bitmask = BIT(bit);
> +	void __iomem *reg_addr;
> +	unsigned long flags;
> +	unsigned int i;
> +	u32 val;
> +
> +	dev_dbg(priv->dev, "%s %u%02u\n", op_name, reg, bit);

Replace op_name with set ? "assert" : "deassert".

You could later add a str_assert_deassert() helper in string_choices.h.

> +
> +	spin_lock_irqsave(&priv->pub.rmw_lock, flags);
> +
> +	reg_addr = priv->pub.base0 + priv->reset_regs[reg];
> +	/* Read current value and modify */
> +	val = readl(reg_addr);
> +	if (set)
> +		val |= bitmask;
> +	else
> +		val &= ~bitmask;
> +	writel(val, reg_addr);
> +
> +	/* Verify the operation if requested */
> +	if (verify) {
> +		val = readl(reg_addr);
> +		if ((set && !(bitmask & val)) || (!set && (bitmask & val))) {
> +			dev_err(priv->dev, "Reset register %u%02u operation failed\n", reg, bit);
> +			spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
> +			return -EIO;
> +		}
> +	}
> +
> +	/* Perform dummy reads if required */
> +	for (i = 0; dummy_reads && i < RZT2H_RESET_REG_READ_COUNT; i++)
> +		readl(reg_addr);

Both verify and dummy reads could just live in a single if (!assert) {}
block.

> +
> +	spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int cpg_mrcr_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	int ret;
> +
> +	/* Assert reset */
> +	ret = cpg_mrcr_set_bit(rcdev, id, true, true, false, "reset");
> +	if (ret)
> +		return ret;

No delay necessary for any of the modules that can be reset?

> +	/* Deassert reset with dummy reads */
> +	return cpg_mrcr_set_bit(rcdev, id, false, false, true, "reset");

Copy & paste error in op_name, but that is fixed if you drop it.

> +}
> +
> +static int cpg_mrcr_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	return cpg_mrcr_set_bit(rcdev, id, true, true, false, "assert");
> +}
> +
> +static int cpg_mrcr_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	return cpg_mrcr_set_bit(rcdev, id, false, false, true, "deassert");
> +}
> +
>  static const struct reset_control_ops cpg_mssr_reset_ops = {
>  	.reset = cpg_mssr_reset,
>  	.assert = cpg_mssr_assert,
> @@ -743,6 +828,13 @@ static const struct reset_control_ops cpg_mssr_reset_ops = {
>  	.status = cpg_mssr_status,
>  };
>  
> +static const struct reset_control_ops cpg_mrcr_reset_ops = {
> +	.reset = cpg_mrcr_reset,
> +	.assert = cpg_mrcr_assert,
> +	.deassert = cpg_mrcr_deassert,
> +	.status = cpg_mssr_status,
> +};
> +
>  static int cpg_mssr_reset_xlate(struct reset_controller_dev *rcdev,
>  				const struct of_phandle_args *reset_spec)
>  {
> @@ -760,11 +852,23 @@ static int cpg_mssr_reset_xlate(struct reset_controller_dev *rcdev,
>  
>  static int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv)
>  {
> -	priv->rcdev.ops = &cpg_mssr_reset_ops;
> +	/*
> +	 * RZ/T2H (and family) has the Module Reset Control Registers
> +	 * which allows control resets of certain modules.
> +	 * The number of resets is not equal to the number of module clocks.
> +	 */
> +	if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
> +		priv->rcdev.ops = &cpg_mrcr_reset_ops;
> +		priv->rcdev.nr_resets = ARRAY_SIZE(mrcr_for_rzt2h) * 32;
> +	} else {
> +		priv->rcdev.ops = &cpg_mssr_reset_ops;
> +		priv->rcdev.nr_resets = priv->num_mod_clks;
> +	}
> +
>  	priv->rcdev.of_node = priv->dev->of_node;
>  	priv->rcdev.of_reset_n_cells = 1;
>  	priv->rcdev.of_xlate = cpg_mssr_reset_xlate;
> -	priv->rcdev.nr_resets = priv->num_mod_clks;
> +
>  	return devm_reset_controller_register(priv->dev, &priv->rcdev);
>  }
>  
> @@ -1166,6 +1270,7 @@ static int __init cpg_mssr_common_init(struct device *dev,
>  		priv->control_regs = stbcr;
>  	} else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
>  		priv->control_regs = mstpcr_for_rzt2h;
> +		priv->reset_regs = mrcr_for_rzt2h;
>  	} else if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4) {
>  		priv->status_regs = mstpsr_for_gen4;
>  		priv->control_regs = mstpcr_for_gen4;
> @@ -1262,8 +1367,7 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
>  		goto reserve_exit;
>  
>  	/* Reset Controller not supported for Standby Control SoCs */
> -	if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A ||
> -	    priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> +	if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
>  		goto reserve_exit;
>  
>  	error = cpg_mssr_reset_controller_register(priv);

regards
Philipp

Powered by blists - more mailing lists