diff mbox series

[2/3] reset: hisilicon: Extend reset operation type

Message ID 20191202144524.5391-3-jun.nie@linaro.org
State New
Headers show
Series Extend Hisilicon reset type | expand

Commit Message

Jun Nie Dec. 2, 2019, 2:45 p.m. UTC
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(-)

-- 
2.17.1

Comments

Philipp Zabel Dec. 2, 2019, 5:04 p.m. UTC | #1
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
Jun Nie Dec. 3, 2019, 3:53 a.m. UTC | #2
Philipp Zabel <p.zabel@pengutronix.de> 于2019年12月3日周二 上午1:04写道:
>

> 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?


Some reset is asserted on power on automatically. So only .deassert is needed.
>

> > +

> >       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.


Will fix the wrong value definition in next version patch. The same to
below comments.
>

> > +             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.


Will do.
>

> 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?


I do not know yet. Only extend this driver with all combination in logic.
>

> >       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.


All current dts file are modified accordingly. But existing dtb binary support
is an issue. Do you have any suggestion?

>

> >       rstc->rcdev.of_xlate = hisi_reset_of_xlate;

> >       reset_controller_register(&rstc->rcdev);

>

> regards

> Philipp

>
Philipp Zabel Dec. 3, 2019, 2:15 p.m. UTC | #3
Hi Jun,

On Tue, 2019-12-03 at 11:53 +0800, Jun Nie wrote:
[...]
> @@ -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?

> 

> Some reset is asserted on power on automatically. So only .deassert is needed.


But if the bit was set/cleared again after being deasserted, would it
assert the reset line? Basically, I wonder if those bits are write-once
or not.

[...]
> > > +             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?


This is missing an explanation.

[...]
> > > +     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?

> 

> I do not know yet. Only extend this driver with all combination in logic.


If this is not used, let's leave it out.

[...]
> > > @@ -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.

> 

> All current dts file are modified accordingly. But existing dtb binary support

> is an issue. Do you have any suggestion?


Since this is for a new SoC, you should keep using of_reset_n_cells = 2
for the current SoCs and only set it to 3 for a new compatible, for
example using of_device_get_match_data().

regards
Philipp
Jun Nie Dec. 4, 2019, 6:09 a.m. UTC | #4
Philipp Zabel <p.zabel@pengutronix.de> 于2019年12月3日周二 下午10:15写道:
>

> Hi Jun,

>

> On Tue, 2019-12-03 at 11:53 +0800, Jun Nie wrote:

> [...]

> > @@ -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?

> >

> > Some reset is asserted on power on automatically. So only .deassert is needed.

>

> But if the bit was set/cleared again after being deasserted, would it

> assert the reset line? Basically, I wonder if those bits are write-once

> or not.


I did not see vendor's code to clear it, seems the bit is only polled
to be set on every
device enablement.
>

> [...]

> > > > +             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?

>

> This is missing an explanation.


They are not mutually exclusive in logic. But I did not see the need
to poll for a assert yet.
>

> [...]

> > > > +     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?

> >

> > I do not know yet. Only extend this driver with all combination in logic.

>

> If this is not used, let's leave it out.

>

> [...]

> > > > @@ -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.

> >

> > All current dts file are modified accordingly. But existing dtb binary support

> > is an issue. Do you have any suggestion?

>

> Since this is for a new SoC, you should keep using of_reset_n_cells = 2

> for the current SoCs and only set it to 3 for a new compatible, for

> example using of_device_get_match_data().


Yeah, this should be the solution. I will check the detail, thanks!

Regards,
Jun
diff mbox series

Patch

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 */
+
 	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) {
+		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);
+	}
+
 	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);
+	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 */
+
 	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);
+	}
+
 	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);
+	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;
 	rstc->rcdev.of_xlate = hisi_reset_of_xlate;
 	reset_controller_register(&rstc->rcdev);