linux-kernel - Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Jan 2017 16:57:10 +0100
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Geert Uytterhoeven <geert+renesas@...der.be>
Cc:     Simon Horman <horms@...ge.net.au>,
        Magnus Damm <magnus.damm@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset
 control

Hi Geert,

On Fri, 2017-01-20 at 15:08 +0100, Geert Uytterhoeven wrote:
> Add optional support for the Reset Control feature of the Renesas Clock
> Pulse Generator / Module Standby and Software Reset module on R-Car
> Gen2, R-Car Gen3, and RZ/G1 SoCs.

Is there a reason to make this optional?

> This allows to reset SoC devices using the Reset Controller API.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>

Looks good to me,

Acked-by: Philipp Zabel <p.zabel@...gutronix.de>

Just a small issue below,

> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 122 +++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index f1161a585c57e433..ea4af714ac14603a 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
[...]
> +static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> +			  unsigned long id)
> +{
> +	struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +	unsigned int reg = id / 32;
> +	unsigned int bit = id % 32;
> +	u32 bitmask = BIT(bit);

Here you have a bitmask = BIT(bit) variable.

> +	unsigned long flags;
> +	u32 value;
> +
> +	dev_dbg(priv->dev, "reset %u%02u\n", reg, bit);
> +
> +	/* Reset module */
> +	spin_lock_irqsave(&priv->rmw_lock, flags);
> +	value = readl(priv->base + SRCR(reg));
> +	value |= bitmask;

Here you use it.

> +	writel(value, priv->base + SRCR(reg));
> +	spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +
> +	/* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
> +	udelay(35);
> +
> +	/* Release module from reset state */
> +	writel(bitmask, priv->base + SRSTCLR(reg));
> +
> +	return 0;
> +}
> +
> +static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +	unsigned int reg = id / 32;
> +	unsigned int bit = id % 32;

Here you haven't.

> +	unsigned long flags;
> +	u32 value;
> +
> +	dev_dbg(priv->dev, "assert %u%02u\n", reg, bit);
> +
> +	spin_lock_irqsave(&priv->rmw_lock, flags);
> +	value = readl(priv->base + SRCR(reg));
> +	writel(value | BIT(bit), priv->base + SRCR(reg));

Here you don't.

> +	spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +	return 0;
> +}
> +
> +static int cpg_mssr_deassert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +	unsigned int reg = id / 32;
> +	unsigned int bit = id % 32;
> +
> +	dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit);
> +
> +	writel(BIT(bit), priv->base + SRSTCLR(reg));

And here ...

> +	return 0;
> +}
> +
> +static int cpg_mssr_status(struct reset_controller_dev *rcdev,
> +			   unsigned long id)
> +{
> +	struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev);
> +	unsigned int reg = id / 32;
> +	unsigned int bit = id % 32;
> +
> +	return !!(readl(priv->base + SRCR(reg)) & BIT(bit));

And here neither.

I'd choose one variant over the other for consistency.

regards
Philipp