linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Jun Nie <jun.nie@linaro.org>,
	mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
	mark.rutland@arm.com, xuwei5@hisilicon.com,
	opensource@jilayne.com, swinslow@gmail.com, allison@lohutok.net,
	yuehaibing@huawei.com, tglx@linutronix.de,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, xuejiancheng@hisilicon.com
Subject: Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type
Date: Mon, 02 Dec 2019 18:04:19 +0100	[thread overview]
Message-ID: <449968d8f085a1d1fcf4018bb8efe454fa35b3e3.camel@pengutronix.de> (raw)
In-Reply-To: <20191202144524.5391-3-jun.nie@linaro.org>

On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> Extend reset operations to support combination of three type flags:
> ASSERT/DEASSERT SET/CLEAR POLL.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/clk/hisilicon/reset.c | 58 ++++++++++++++++++++++++++++++++---
>  1 file changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> index 93cee17db8b1..de7d186b0894 100644
> --- a/drivers/clk/hisilicon/reset.c
> +++ b/drivers/clk/hisilicon/reset.c
> @@ -2,20 +2,25 @@
>  /*
>   * Hisilicon Reset Controller Driver
>   *
> - * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
> + * Copyright (c) 2015-2019 HiSilicon Technologies Co., Ltd.
>   */
>  
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +
> +#include <dt-bindings/reset/hisilicon-resets.h>
>  #include "reset.h"
>  
>  #define	HISI_RESET_BIT_MASK	0x1f
>  #define	HISI_RESET_OFFSET_SHIFT	8
>  #define	HISI_RESET_OFFSET_MASK	0xffff00
> +#define	HISI_RESET_FLAG_SHIFT	24
> +#define	HISI_RESET_FLAG_MASK	0xff000000
>  
>  struct hisi_reset_controller {
>  	spinlock_t	lock;
> @@ -30,14 +35,17 @@ struct hisi_reset_controller {
>  static int hisi_reset_of_xlate(struct reset_controller_dev *rcdev,
>  			const struct of_phandle_args *reset_spec)
>  {
> +	unsigned long flags;
>  	u32 offset;
>  	u8 bit;
>  
> +	flags = (reset_spec->args[2] << HISI_RESET_FLAG_SHIFT)
> +		& HISI_RESET_FLAG_MASK;
>  	offset = (reset_spec->args[0] << HISI_RESET_OFFSET_SHIFT)
>  		& HISI_RESET_OFFSET_MASK;
>  	bit = reset_spec->args[1] & HISI_RESET_BIT_MASK;
>  
> -	return (offset | bit);
> +	return (flags | offset | bit);
>  }
>  
>  static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
>  	u32 offset, reg;
>  	u8 bit;
>  
> +	flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> +	if (flags & HISI_ASSERT_NONE)
> +		return -ENOTSUPP; /* assert not supported for this reset */

As long as .assert and .deassert are the only implemented operations for
this reset controller, this does not make any sense to me. Are there
resets that can only be deasserted?

> +
>  	offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
>  	bit = id & HISI_RESET_BIT_MASK;
>  
> +	pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> +		flags & HISI_ASSERT_POLL ? "poll" : "",
> +		flags & HISI_ASSERT_SET ? "set":"clear", offset, bit);
> +
> +	if (flags & HISI_ASSERT_POLL) {

Since HISI_ASSERT_POLL is 0, this is always false.

> +		if (flags & HISI_ASSERT_SET)
> +			return readl_poll_timeout(rstc->membase + offset,
> +						  reg, reg & BIT(bit), 0, 5000);
> +		else
> +			return readl_poll_timeout(rstc->membase + offset,
> +						  reg, !(reg & BIT(bit)),
> +						  0, 5000);

And this is effectively dead code. Do you really have hardware that
asserts or asserts a reset line in reaction to a read access?

Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?

> +	}
> +
>  	spin_lock_irqsave(&rstc->lock, flags);
>  
>  	reg = readl(rstc->membase + offset);
> -	writel(reg | BIT(bit), rstc->membase + offset);
> +	/* Default is setting to assert for no flag case. */
> +	reg = (flags & HISI_ASSERT_CLEAR) ? reg & ~BIT(bit) : reg | BIT(bit);

Consider moving this into a helper function with a boolean set/clear
parameter.

As long as HISI_ASSERT_CLEAR is 0, the first branch is dead code.

> +	writel(reg, rstc->membase + offset);
>  
>  	spin_unlock_irqrestore(&rstc->lock, flags);
>  
> @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
>  	u32 offset, reg;
>  	u8 bit;
>  
> +	flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> +	if (flags & HISI_DEASSERT_NONE)
> +		return -ENOTSUPP; /* deassert not supported for this reset */
> +

Are there resets that can only ever be asserted?

>  	offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
>  	bit = id & HISI_RESET_BIT_MASK;
>  
> +	pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> +		flags & HISI_DEASSERT_POLL ? "poll" : "",
> +		flags & HISI_DEASSERT_SET ? "clear":"set", offset, bit);
> +
> +	if (flags & HISI_DEASSERT_POLL) {
> +		if (flags & HISI_DEASSERT_SET)
> +			return readl_poll_timeout(rstc->membase + offset,
> +						  reg, reg & BIT(bit), 0, 5000);
> +		else
> +			return readl_poll_timeout(rstc->membase + offset,
> +						  reg, !(reg & BIT(bit)),
> +						  0, 5000);

See above, this code currently can never be reached.

> +	}
> +
>  	spin_lock_irqsave(&rstc->lock, flags);
>  
>  	reg = readl(rstc->membase + offset);
> -	writel(reg & ~BIT(bit), rstc->membase + offset);
> +	/* Default is clearing to deasseart for no flag case. */
> +	reg = (flags & HISI_DEASSERT_SET) ? reg | BIT(bit) : reg & ~BIT(bit);

Same as above, the read-modify-write for set/clear could be split out
into a helper.

> +	writel(reg, rstc->membase + offset);
>  
>  	spin_unlock_irqrestore(&rstc->lock, flags);
>  
> @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
>  	rstc->rcdev.owner = THIS_MODULE;
>  	rstc->rcdev.ops = &hisi_reset_ops;
>  	rstc->rcdev.of_node = pdev->dev.of_node;
> -	rstc->rcdev.of_reset_n_cells = 2;
> +	rstc->rcdev.of_reset_n_cells = 3;

This breaks current device trees (before patch 3). You can make sure
device trees with #reset-cells = <2> keep working by parsing the #reset-
cells and setting this value to 2 for old DTs.

>  	rstc->rcdev.of_xlate = hisi_reset_of_xlate;
>  	reset_controller_register(&rstc->rcdev);

regards
Philipp


  reply	other threads:[~2019-12-02 17:04 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 14:45 [PATCH 0/3] Extend Hisilicon reset type Jun Nie
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
2019-12-02 17:04   ` Philipp Zabel
2019-12-03  3:11     ` Jun Nie
2019-12-03 14:12       ` Philipp Zabel
2019-12-02 14:45 ` [PATCH 2/3] reset: hisilicon: Extend reset operation type Jun Nie
2019-12-02 17:04   ` Philipp Zabel [this message]
2019-12-03  3:53     ` Jun Nie
2019-12-03 14:15       ` Philipp Zabel
2019-12-04  6:09         ` Jun Nie
2019-12-02 14:45 ` [PATCH 3/3] ARM: dts: Update reset for hi3519 and hi3798cv200 Jun Nie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=449968d8f085a1d1fcf4018bb8efe454fa35b3e3.camel@pengutronix.de \
    --to=p.zabel@pengutronix.de \
    --cc=allison@lohutok.net \
    --cc=devicetree@vger.kernel.org \
    --cc=jun.nie@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=opensource@jilayne.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=swinslow@gmail.com \
    --cc=tglx@linutronix.de \
    --cc=xuejiancheng@hisilicon.com \
    --cc=xuwei5@hisilicon.com \
    --cc=yuehaibing@huawei.com \
    --subject='Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).