[PATCH] reset: axs10x: Implement assert and deassert callbacks

[PATCH] reset: axs10x: Implement assert and deassert callbacks

[Vitor Soares]

Some custom IP-block connected to ARC AXS10x board need assert and
deassert functions to control reset signal of selected peripherals.

This patch improve AXS10x reset driver by adding assert and deassert
callbacks.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>

Cc: Eugeniy Paltsev <eugeniy.Paltsev@synopsys.com>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Jose Abreu <jose.abreu@synopsys.com>
Cc: Luis Oliveira <lolivei@synopsys.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Cc: Nelson Costa <nelson.costa@synopsys.com>
Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/reset/reset-axs10x.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/reset-axs10x.c b/drivers/reset/reset-axs10x.c
index a854ef41..12dac8b 100644
--- a/drivers/reset/reset-axs10x.c
+++ b/drivers/reset/reset-axs10x.c
@@ -37,8 +37,36 @@ static int axs10x_reset_reset(struct reset_controller_dev *rcdev,
 	return 0;
 }
 
+static int axs10x_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct axs10x_rst *rst = to_axs10x_rst(rcdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&rst->lock, flags);
+	writel(readl(rst->regs_rst) & ~BIT(id), rst->regs_rst);
+	spin_unlock_irqrestore(&rst->lock, flags);
+
+	return 0;
+}
+
+static int axs10x_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	struct axs10x_rst *rst = to_axs10x_rst(rcdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&rst->lock, flags);
+	writel(readl(rst->regs_rst) | BIT(id), rst->regs_rst);
+	spin_unlock_irqrestore(&rst->lock, flags);
+
+	return 0;
+}
+
 static const struct reset_control_ops axs10x_reset_ops = {
-	.reset	= axs10x_reset_reset,
+	.reset		= axs10x_reset_reset,
+	.assert		= axs10x_reset_assert,
+	.deassert	= axs10x_reset_deassert,
 };
 
 static int axs10x_reset_probe(struct platform_device *pdev)
-- 
2.7.4

RE: [PATCH] reset: axs10x: Implement assert and deassert callbacks

[Jose Abreu]

From: Vitor Soares <vitor.soares@synopsys.com>
Date: Mon, Apr 08, 2019 at 11:31:23

>  static const struct reset_control_ops axs10x_reset_ops = {
> -	.reset	= axs10x_reset_reset,

Eugeniy, Alexey,

I think the implementation of this callback is wrong: you should
readl_poll_timeout_atomic() until the reset bit is not cleared.

Thanks,
Jose Miguel Abreu

Re: [PATCH] reset: axs10x: Implement assert and deassert callbacks

[Eugeniy Paltsev]

Hi Vitor,

On Mon, 2019-04-08 at 12:31 +0200, Vitor Soares wrote:
> Some custom IP-block connected to ARC AXS10x board need assert and
> deassert functions to control reset signal of selected peripherals.
> 
> This patch improve AXS10x reset driver by adding assert and deassert
> callbacks.


In the AXS10x reset driver only 'reset' callback is intentionally implemented.
AXS10x is FPGA based boards and with our default firmware AXS10x reset register is implemented as self-deasserted.

Do you have somehow modified AXS10x firmware where reset register is not self-deasserted?
In that case "simple-reset" driver will be suitable for you, I guess.
Otherwise this implementation is incorrect - there should be no 'assert' for reset controller with self-deasserted logic.


> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> 
> Cc: Eugeniy Paltsev <eugeniy.Paltsev@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Jose Abreu <jose.abreu@synopsys.com>
> Cc: Luis Oliveira <lolivei@synopsys.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Nelson Costa <nelson.costa@synopsys.com>
> Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/reset/reset-axs10x.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/reset-axs10x.c b/drivers/reset/reset-axs10x.c
> index a854ef41..12dac8b 100644
> --- a/drivers/reset/reset-axs10x.c
> +++ b/drivers/reset/reset-axs10x.c
> @@ -37,8 +37,36 @@ static int axs10x_reset_reset(struct reset_controller_dev *rcdev,
>  	return 0;
>  }
>  
> +static int axs10x_reset_assert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct axs10x_rst *rst = to_axs10x_rst(rcdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rst->lock, flags);
> +	writel(readl(rst->regs_rst) & ~BIT(id), rst->regs_rst);
> +	spin_unlock_irqrestore(&rst->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int axs10x_reset_deassert(struct reset_controller_dev *rcdev,
> +				 unsigned long id)
> +{
> +	struct axs10x_rst *rst = to_axs10x_rst(rcdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rst->lock, flags);
> +	writel(readl(rst->regs_rst) | BIT(id), rst->regs_rst);
> +	spin_unlock_irqrestore(&rst->lock, flags);
> +
> +	return 0;
> +}
> +
>  static const struct reset_control_ops axs10x_reset_ops = {
> -	.reset	= axs10x_reset_reset,
> +	.reset		= axs10x_reset_reset,
> +	.assert		= axs10x_reset_assert,
> +	.deassert	= axs10x_reset_deassert,
>  };
>  
>  static int axs10x_reset_probe(struct platform_device *pdev)
-- 
 Eugeniy Paltsev

RE: [PATCH] reset: axs10x: Implement assert and deassert callbacks

[Vitor Soares]

Hi Eugeniy,

From: Eugeniy Paltsev <paltsev@synopsys.com>
Date: Mon, Apr 08, 2019 at 12:40:00

> Hi Vitor,
> 
> On Mon, 2019-04-08 at 12:31 +0200, Vitor Soares wrote:
> > Some custom IP-block connected to ARC AXS10x board need assert and
> > deassert functions to control reset signal of selected peripherals.
> > 
> > This patch improve AXS10x reset driver by adding assert and deassert
> > callbacks.
> 
> 
> In the AXS10x reset driver only 'reset' callback is intentionally 
> > implemented.
> AXS10x is FPGA based boards and with our default firmware AXS10x reset 
> > register is implemented as self-deasserted.
> 

I have another reset block connect through AXI.

> Do you have somehow modified AXS10x firmware where reset register is not 
> > self-deasserted?
> In that case "simple-reset" driver will be suitable for you, I guess.

I will try it.

> Otherwise this implementation is incorrect - there should be no 'assert' for 
> > reset controller with self-deasserted logic.

So the assert and reset callback are mutually exclusive?

> 
> 
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > 
> > Cc: Eugeniy Paltsev <eugeniy.Paltsev@synopsys.com>
> > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Joao Pinto <jpinto@synopsys.com>
> > Cc: Jose Abreu <jose.abreu@synopsys.com>
> > Cc: Luis Oliveira <lolivei@synopsys.com>
> > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > Cc: Nelson Costa <nelson.costa@synopsys.com>
> > Cc: Pedro Sousa <pedrom.sousa@synopsys.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/reset/reset-axs10x.c | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/reset/reset-axs10x.c b/drivers/reset/reset-axs10x.c
> > index a854ef41..12dac8b 100644
> > --- a/drivers/reset/reset-axs10x.c
> > +++ b/drivers/reset/reset-axs10x.c
> > @@ -37,8 +37,36 @@ static int axs10x_reset_reset(struct 
> > reset_controller_dev *rcdev,
> >  	return 0;
> >  }
> >  
> > +static int axs10x_reset_assert(struct reset_controller_dev *rcdev,
> > +			       unsigned long id)
> > +{
> > +	struct axs10x_rst *rst = to_axs10x_rst(rcdev);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&rst->lock, flags);
> > +	writel(readl(rst->regs_rst) & ~BIT(id), rst->regs_rst);
> > +	spin_unlock_irqrestore(&rst->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int axs10x_reset_deassert(struct reset_controller_dev *rcdev,
> > +				 unsigned long id)
> > +{
> > +	struct axs10x_rst *rst = to_axs10x_rst(rcdev);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&rst->lock, flags);
> > +	writel(readl(rst->regs_rst) | BIT(id), rst->regs_rst);
> > +	spin_unlock_irqrestore(&rst->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct reset_control_ops axs10x_reset_ops = {
> > -	.reset	= axs10x_reset_reset,
> > +	.reset		= axs10x_reset_reset,
> > +	.assert		= axs10x_reset_assert,
> > +	.deassert	= axs10x_reset_deassert,
> >  };
> >  
> >  static int axs10x_reset_probe(struct platform_device *pdev)
> -- 
>  Eugeniy Paltsev
> 

Best regards,
Vitor Soares

Re: [PATCH] reset: axs10x: Implement assert and deassert callbacks

[Eugeniy Paltsev]

Hi Vitor,

On Mon, 2019-04-08 at 13:25 +0000, Vitor Soares wrote:
> Hi Eugeniy,
> 
> From: Eugeniy Paltsev <paltsev@synopsys.com>
> Date: Mon, Apr 08, 2019 at 12:40:00
> 
> > Hi Vitor,
> > 
> > On Mon, 2019-04-08 at 12:31 +0200, Vitor Soares wrote:
> > > Some custom IP-block connected to ARC AXS10x board need assert and
> > > deassert functions to control reset signal of selected peripherals.
> > > 
> > > This patch improve AXS10x reset driver by adding assert and deassert
> > > callbacks.
> > 
> > 
> > In the AXS10x reset driver only 'reset' callback is intentionally 
> > > implemented.
> > 
> > AXS10x is FPGA based boards and with our default firmware AXS10x reset 
> > > register is implemented as self-deasserted.
> 
> I have another reset block connect through AXI.
> 
> > Do you have somehow modified AXS10x firmware where reset register is not 
> > > self-deasserted?
> > 
> > In that case "simple-reset" driver will be suitable for you, I guess.
> 
> I will try it.
> 
> > Otherwise this implementation is incorrect - there should be no 'assert' for 
> > > reset controller with self-deasserted logic.
> 
> So the assert and reset callback are mutually exclusive?

Not actually.

Adding 'assert' callback is incorrect for exclusive reset controls in
case of self-deasserted reset controller.
It will cause reset_control_assert() to return success for exclusive
reset controls, even though the .assert op failed to leave the reset
line asserted after the function returns.


[snap]
> 
> Best regards,
> Vitor Soares
> 
-- 
 Eugeniy Paltsev

Re: [PATCH] reset: axs10x: Implement assert and deassert callbacks

[Eugeniy Paltsev]

Hi Jose,

On Mon, 2019-04-08 at 11:23 +0000, Jose Abreu wrote:
> From: Vitor Soares <vitor.soares@synopsys.com>
> Date: Mon, Apr 08, 2019 at 11:31:23
> 
> >  static const struct reset_control_ops axs10x_reset_ops = {
> > -	.reset	= axs10x_reset_reset,
> 
> Eugeniy, Alexey,
> 
> I think the implementation of this callback is wrong: you should
> readl_poll_timeout_atomic() until the reset bit is not cleared.

I don't remember if this register is read/write or write-only. I'll check it.
Thanks for report.

> 
> Thanks,
> Jose Miguel Abreu
-- 
 Eugeniy Paltsev

RE: [PATCH] reset: axs10x: Implement assert and deassert callbacks

[Vitor Soares]

Hi Eugeniy,

From: Eugeniy Paltsev <paltsev@synopsys.com>
Date: Fri, Apr 12, 2019 at 14:22:33

> Hi Vitor,
> 
> On Mon, 2019-04-08 at 13:25 +0000, Vitor Soares wrote:
> > Hi Eugeniy,
> > 
> > From: Eugeniy Paltsev <paltsev@synopsys.com>
> > Date: Mon, Apr 08, 2019 at 12:40:00
> > 
> > > Hi Vitor,
> > > 
> > > On Mon, 2019-04-08 at 12:31 +0200, Vitor Soares wrote:
> > > > Some custom IP-block connected to ARC AXS10x board need assert and
> > > > deassert functions to control reset signal of selected peripherals.
> > > > 
> > > > This patch improve AXS10x reset driver by adding assert and deassert
> > > > callbacks.
> > > 
> > > 
> > > In the AXS10x reset driver only 'reset' callback is intentionally 
> > > > implemented.
> > > 
> > > AXS10x is FPGA based boards and with our default firmware AXS10x reset 
> > > > register is implemented as self-deasserted.
> > 
> > I have another reset block connect through AXI.
> > 
> > > Do you have somehow modified AXS10x firmware where reset register is not 
> > > > self-deasserted?
> > > 
> > > In that case "simple-reset" driver will be suitable for you, I guess.
> > 
> > I will try it.
> > 
> > > Otherwise this implementation is incorrect - there should be no 'assert' for 
> > > > reset controller with self-deasserted logic.
> > 
> > So the assert and reset callback are mutually exclusive?
> 
> Not actually.
> 
> Adding 'assert' callback is incorrect for exclusive reset controls in
> case of self-deasserted reset controller.
> It will cause reset_control_assert() to return success for exclusive
> reset controls, even though the .assert op failed to leave the reset
> line asserted after the function returns.
> 

Sorry I'm not understanding. Can you please explain?

What I see in reset_control_assert() when not shared is return -ENOTSUPP.

> 
> [snap]
> > 
> > Best regards,
> > Vitor Soares
> > 
> -- 
>  Eugeniy Paltsev

Thanks for your feedback.


Best regards,
Vitor Soares