[PATCH 0/7] reset: add managned reset_controller_register()

[PATCH 0/7] reset: add managned reset_controller_register()

[Masahiro Yamada]

Masahiro Yamada (7):
  reset: add devm_reset_controller_register API
  reset: ath79: use devm_reset_controller_register()
  reset: lpc18xx: use devm_reset_controller_register()
  reset: pistachio: use devm_reset_controller_register()
  reset: sunxi: use devm_reset_controller_register()
  reset: socfpga: use devm_reset_controller_register()
  reset: zynq: use devm_reset_controller_register()

 Documentation/driver-model/devres.txt |  4 ++++
 drivers/reset/core.c                  | 37 +++++++++++++++++++++++++++++++++++
 drivers/reset/reset-ath79.c           |  3 +--
 drivers/reset/reset-lpc18xx.c         |  4 +---
 drivers/reset/reset-pistachio.c       | 12 +-----------
 drivers/reset/reset-socfpga.c         | 12 +-----------
 drivers/reset/reset-sunxi.c           | 12 +-----------
 drivers/reset/reset-zynq.c            | 12 +-----------
 include/linux/reset-controller.h      |  4 ++++
 9 files changed, 51 insertions(+), 49 deletions(-)

-- 
1.9.1

[PATCH 1/7] reset: add devm_reset_controller_register API

[Masahiro Yamada]

Add a device managed API for reset_controller_register().

This helps in reducing code in .remove callbacks and sometimes
dropping .remove callbacks entirely.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Documentation/driver-model/devres.txt |  4 ++++
 drivers/reset/core.c                  | 37 +++++++++++++++++++++++++++++++++++
 include/linux/reset-controller.h      |  4 ++++
 3 files changed, 45 insertions(+)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 108d455..5270435 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -340,6 +340,10 @@ REGULATOR
   devm_regulator_put()
   devm_regulator_register()
 
+RESET
+  devm_reset_control_get()
+  devm_reset_controller_register()
+
 SLAVE DMA ENGINE
   devm_acpi_dma_controller_register()
 
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index f15f150..181b05d 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -82,6 +82,43 @@ void reset_controller_unregister(struct reset_controller_dev *rcdev)
 }
 EXPORT_SYMBOL_GPL(reset_controller_unregister);
 
+static void devm_reset_controller_release(struct device *dev, void *res)
+{
+	reset_controller_unregister(*(struct reset_controller_dev **)res);
+}
+
+/**
+ * devm_reset_controller_register - resource managed reset_controller_register()
+ * @dev: device that is registering this reset controller
+ * @rcdev: a pointer to the initialized reset controller device
+ *
+ * Managed reset_controller_register(). For reset controllers registered by
+ * this function, reset_controller_unregister() is automatically called on
+ * driver detach. See reset_controller_register() for more information.
+ */
+int devm_reset_controller_register(struct device *dev,
+				   struct reset_controller_dev *rcdev)
+{
+	struct reset_controller_dev **rcdevp;
+	int ret;
+
+	rcdevp = devres_alloc(devm_reset_controller_release, sizeof(*rcdevp),
+			      GFP_KERNEL);
+	if (!rcdevp)
+		return -ENOMEM;
+
+	ret = reset_controller_register(rcdev);
+	if (!ret) {
+		*rcdevp = rcdev;
+		devres_add(dev, rcdevp);
+	} else {
+		devres_free(rcdevp);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_reset_controller_register);
+
 /**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index a3a5bcd..a4eaf1c 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -51,4 +51,8 @@ struct reset_controller_dev {
 int reset_controller_register(struct reset_controller_dev *rcdev);
 void reset_controller_unregister(struct reset_controller_dev *rcdev);
 
+struct device;
+int devm_reset_controller_register(struct device *dev,
+				   struct reset_controller_dev *rcdev);
+
 #endif
-- 
1.9.1

[PATCH 1/7] reset: add devm_reset_controller_register API

[Laxman Dewangan]

On Sunday 01 May 2016 04:06 PM, Masahiro Yamada wrote:
> Add a device managed API for reset_controller_register().
>
> This helps in reducing code in .remove callbacks and sometimes
> dropping .remove callbacks entirely.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>


I liked it.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>

[PATCH 1/7] reset: add devm_reset_controller_register API

[Philipp Zabel]

Hi Masahiro,

Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> Add a device managed API for reset_controller_register().
> 
> This helps in reducing code in .remove callbacks and sometimes
> dropping .remove callbacks entirely.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Thank you for these patches. Except for the issue with the lpc18xx patch
they all look good to me.
If you don't mind, I'll drop the lpc18xx patch for now and apply the
others.

regards
Philipp

[PATCH 1/7] reset: add devm_reset_controller_register API

[Masahiro Yamada]

2016-05-03 19:17 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Masahiro,
>
> Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> Add a device managed API for reset_controller_register().
>>
>> This helps in reducing code in .remove callbacks and sometimes
>> dropping .remove callbacks entirely.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Thank you for these patches. Except for the issue with the lpc18xx patch
> they all look good to me.
> If you don't mind, I'll drop the lpc18xx patch for now and apply the
> others.
>

It is OK with me, but I do not understand what is the problem.

Could you answer my question in the 3/7 thread?



-- 
Best Regards
Masahiro Yamada

[PATCH 1/7] reset: add devm_reset_controller_register API

[Masahiro Yamada]

2016-05-03 19:26 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> 2016-05-03 19:17 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> Hi Masahiro,
>>
>> Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>>> Add a device managed API for reset_controller_register().
>>>
>>> This helps in reducing code in .remove callbacks and sometimes
>>> dropping .remove callbacks entirely.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>
>> Thank you for these patches. Except for the issue with the lpc18xx patch
>> they all look good to me.
>> If you don't mind, I'll drop the lpc18xx patch for now and apply the
>> others.
>>
>
> It is OK with me, but I do not understand what is the problem.
>
> Could you answer my question in the 3/7 thread?


Now I am convinced.

I leave 3/7 to your decision.



-- 
Best Regards
Masahiro Yamada

[PATCH 1/7] reset: add devm_reset_controller_register API

[Philipp Zabel]

Am Dienstag, den 03.05.2016, 20:41 +0900 schrieb Masahiro Yamada:
> 2016-05-03 19:26 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> > 2016-05-03 19:17 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> >> Hi Masahiro,
> >>
> >> Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> >>> Add a device managed API for reset_controller_register().
> >>>
> >>> This helps in reducing code in .remove callbacks and sometimes
> >>> dropping .remove callbacks entirely.
> >>>
> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >>
> >> Thank you for these patches. Except for the issue with the lpc18xx patch
> >> they all look good to me.
> >> If you don't mind, I'll drop the lpc18xx patch for now and apply the
> >> others.
> >>
> >
> > It is OK with me, but I do not understand what is the problem.
> >
> > Could you answer my question in the 3/7 thread?
>
> Now I am convinced.
> 
> I leave 3/7 to your decision.

Ok, I applied all but 3/7 to my reset/next branch.

thanks
Philipp

[PATCH 2/7] reset: ath79: use devm_reset_controller_register()

[Masahiro Yamada]

Use devm_reset_controller_register() for the reset controller
registration and remove the unregister call from the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-ath79.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/reset/reset-ath79.c b/drivers/reset/reset-ath79.c
index ccb940a..16d410c 100644
--- a/drivers/reset/reset-ath79.c
+++ b/drivers/reset/reset-ath79.c
@@ -112,7 +112,7 @@ static int ath79_reset_probe(struct platform_device *pdev)
 	ath79_reset->rcdev.of_reset_n_cells = 1;
 	ath79_reset->rcdev.nr_resets = 32;
 
-	err = reset_controller_register(&ath79_reset->rcdev);
+	err = devm_reset_controller_register(&pdev->dev, &ath79_reset->rcdev);
 	if (err)
 		return err;
 
@@ -131,7 +131,6 @@ static int ath79_reset_remove(struct platform_device *pdev)
 	struct ath79_reset *ath79_reset = platform_get_drvdata(pdev);
 
 	unregister_restart_handler(&ath79_reset->restart_nb);
-	reset_controller_unregister(&ath79_reset->rcdev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Masahiro Yamada]

Use devm_reset_controller_register() for the reset controller
registration and remove the unregister call from the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-lpc18xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
index 3b8a4f5..dd4f27e 100644
--- a/drivers/reset/reset-lpc18xx.c
+++ b/drivers/reset/reset-lpc18xx.c
@@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rc);
 
-	ret = reset_controller_register(&rc->rcdev);
+	ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register device\n");
 		goto dis_clks;
@@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
 	if (ret)
 		dev_warn(&pdev->dev, "failed to unregister restart handler\n");
 
-	reset_controller_unregister(&rc->rcdev);
-
 	clk_disable_unprepare(rc->clk_delay);
 	clk_disable_unprepare(rc->clk_reg);
 
-- 
1.9.1

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Joachim Eastwood]

On 1 May 2016 at 12:36, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Use devm_reset_controller_register() for the reset controller
> registration and remove the unregister call from the .remove callback.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Acked-by: Joachim Eastwood <manabian@gmail.com>


regards,
Joachim Eastwood

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Philipp Zabel]

Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> Use devm_reset_controller_register() for the reset controller
> registration and remove the unregister call from the .remove callback.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  drivers/reset/reset-lpc18xx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> index 3b8a4f5..dd4f27e 100644
> --- a/drivers/reset/reset-lpc18xx.c
> +++ b/drivers/reset/reset-lpc18xx.c
> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, rc);
>  
> -	ret = reset_controller_register(&rc->rcdev);
> +	ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to register device\n");
>  		goto dis_clks;
> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>  	if (ret)
>  		dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>  
> -	reset_controller_unregister(&rc->rcdev);
> -
>  	clk_disable_unprepare(rc->clk_delay);
>  	clk_disable_unprepare(rc->clk_reg);
>  

Hmm, would this patch theoretically allow a window between the calls to
clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
reset_control_get() + reset_control_(de)assert() would access unclocked
registers?

regards
Philipp

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Masahiro Yamada]

2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> Use devm_reset_controller_register() for the reset controller
>> registration and remove the unregister call from the .remove callback.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  drivers/reset/reset-lpc18xx.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> index 3b8a4f5..dd4f27e 100644
>> --- a/drivers/reset/reset-lpc18xx.c
>> +++ b/drivers/reset/reset-lpc18xx.c
>> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, rc);
>>
>> -     ret = reset_controller_register(&rc->rcdev);
>> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>>       if (ret) {
>>               dev_err(&pdev->dev, "unable to register device\n");
>>               goto dis_clks;
>> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>>       if (ret)
>>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>>
>> -     reset_controller_unregister(&rc->rcdev);
>> -
>>       clk_disable_unprepare(rc->clk_delay);
>>       clk_disable_unprepare(rc->clk_reg);
>>
>
> Hmm, would this patch theoretically allow a window between the calls to
> clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
> reset_control_get() + reset_control_(de)assert() would access unclocked
> registers?

This is not clear to me.

Why reset_control_get() + reset_control_(de)assert() would happen here?


devm_reset_controller_release() just calls reset_controller_unregister().

It is just a manipulation of a linked list.


void reset_controller_unregister(struct reset_controller_dev *rcdev)
{
      mutex_lock(&reset_controller_list_mutex);
      list_del(&rcdev->list);
      mutex_unlock(&reset_controller_list_mutex);
}




-- 
Best Regards
Masahiro Yamada

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Philipp Zabel]

Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> >> Use devm_reset_controller_register() for the reset controller
> >> registration and remove the unregister call from the .remove callback.
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> ---
> >>
> >>  drivers/reset/reset-lpc18xx.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> >> index 3b8a4f5..dd4f27e 100644
> >> --- a/drivers/reset/reset-lpc18xx.c
> >> +++ b/drivers/reset/reset-lpc18xx.c
> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
> >>
> >>       platform_set_drvdata(pdev, rc);
> >>
> >> -     ret = reset_controller_register(&rc->rcdev);
> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
> >>       if (ret) {
> >>               dev_err(&pdev->dev, "unable to register device\n");
> >>               goto dis_clks;
> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
> >>       if (ret)
> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
> >>
> >> -     reset_controller_unregister(&rc->rcdev);
> >> -
> >>       clk_disable_unprepare(rc->clk_delay);
> >>       clk_disable_unprepare(rc->clk_reg);
> >>
> >
> > Hmm, would this patch theoretically allow a window between the calls to
> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
> > reset_control_get() + reset_control_(de)assert() would access unclocked
> > registers?
> 
> This is not clear to me.
> 
> Why reset_control_get() + reset_control_(de)assert() would happen here?

I suppose on a non-SMP device, without parallel probing this can't
really happen in practice.
It still seems weird that suddenly we disable the clocks before
unregistering the reset controller instead of afterwards.

regards
Philipp

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Masahiro Yamada]

Hi Philipp,

2016-05-03 18:05 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
>> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> >> Use devm_reset_controller_register() for the reset controller
>> >> registration and remove the unregister call from the .remove callback.
>> >>
>> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >> ---
>> >>
>> >>  drivers/reset/reset-lpc18xx.c | 4 +---
>> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> >> index 3b8a4f5..dd4f27e 100644
>> >> --- a/drivers/reset/reset-lpc18xx.c
>> >> +++ b/drivers/reset/reset-lpc18xx.c
>> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>> >>
>> >>       platform_set_drvdata(pdev, rc);
>> >>
>> >> -     ret = reset_controller_register(&rc->rcdev);
>> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>> >>       if (ret) {
>> >>               dev_err(&pdev->dev, "unable to register device\n");
>> >>               goto dis_clks;
>> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>> >>       if (ret)
>> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>> >>
>> >> -     reset_controller_unregister(&rc->rcdev);
>> >> -
>> >>       clk_disable_unprepare(rc->clk_delay);
>> >>       clk_disable_unprepare(rc->clk_reg);
>> >>
>> >
>> > Hmm, would this patch theoretically allow a window between the calls to
>> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
>> > reset_control_get() + reset_control_(de)assert() would access unclocked
>> > registers?
>>
>> This is not clear to me.
>>
>> Why reset_control_get() + reset_control_(de)assert() would happen here?
>
> I suppose on a non-SMP device, without parallel probing this can't
> really happen in practice.
> It still seems weird that suddenly we disable the clocks before
> unregistering the reset controller instead of afterwards.
>

I still do not understand what you mean.

This patch moves the reset_controller_unregister() call
after clk_disable_unprepare().


But, reset_controller_unregister() is just a manipulation of a liked list.
It does not trigger any hardware access.

Am I wrong?

void reset_controller_unregister(struct reset_controller_dev *rcdev)
{
        mutex_lock(&reset_controller_list_mutex);
        list_del(&rcdev->list);
        mutex_unlock(&reset_controller_list_mutex);
}



-- 
Best Regards
Masahiro Yamada

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Philipp Zabel]

Am Dienstag, den 03.05.2016, 19:25 +0900 schrieb Masahiro Yamada:
> Hi Philipp,
> 
> 2016-05-03 18:05 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
> >> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> >> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> >> >> Use devm_reset_controller_register() for the reset controller
> >> >> registration and remove the unregister call from the .remove callback.
> >> >>
> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> >> ---
> >> >>
> >> >>  drivers/reset/reset-lpc18xx.c | 4 +---
> >> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> >> >> index 3b8a4f5..dd4f27e 100644
> >> >> --- a/drivers/reset/reset-lpc18xx.c
> >> >> +++ b/drivers/reset/reset-lpc18xx.c
> >> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
> >> >>
> >> >>       platform_set_drvdata(pdev, rc);
> >> >>
> >> >> -     ret = reset_controller_register(&rc->rcdev);
> >> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
> >> >>       if (ret) {
> >> >>               dev_err(&pdev->dev, "unable to register device\n");
> >> >>               goto dis_clks;
> >> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
> >> >>       if (ret)
> >> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
> >> >>
> >> >> -     reset_controller_unregister(&rc->rcdev);
> >> >> -
> >> >>       clk_disable_unprepare(rc->clk_delay);
> >> >>       clk_disable_unprepare(rc->clk_reg);
> >> >>
> >> >
> >> > Hmm, would this patch theoretically allow a window between the calls to
> >> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
> >> > reset_control_get() + reset_control_(de)assert() would access unclocked
> >> > registers?
> >>
> >> This is not clear to me.
> >>
> >> Why reset_control_get() + reset_control_(de)assert() would happen here?
> >
> > I suppose on a non-SMP device, without parallel probing this can't
> > really happen in practice.
> > It still seems weird that suddenly we disable the clocks before
> > unregistering the reset controller instead of afterwards.
> >
> 
> I still do not understand what you mean.
> 
> This patch moves the reset_controller_unregister() call
> after clk_disable_unprepare().

And so the register access is made impossible before the reset
controller device actually vanishes from the publicly visible list.

> But, reset_controller_unregister() is just a manipulation of a liked list.
> It does not trigger any hardware access.
> 
> Am I wrong?

No, you are perfectly right. I don't see how this can be a real problem
unless at the same time another driver could try to request the still
available reset control.

regards
Philipp

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

[Masahiro Yamada]

2016-05-03 20:08 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Dienstag, den 03.05.2016, 19:25 +0900 schrieb Masahiro Yamada:
>> Hi Philipp,
>>
>> 2016-05-03 18:05 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
>> >> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> >> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> >> >> Use devm_reset_controller_register() for the reset controller
>> >> >> registration and remove the unregister call from the .remove callback.
>> >> >>
>> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >> >> ---
>> >> >>
>> >> >>  drivers/reset/reset-lpc18xx.c | 4 +---
>> >> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> >> >> index 3b8a4f5..dd4f27e 100644
>> >> >> --- a/drivers/reset/reset-lpc18xx.c
>> >> >> +++ b/drivers/reset/reset-lpc18xx.c
>> >> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>> >> >>
>> >> >>       platform_set_drvdata(pdev, rc);
>> >> >>
>> >> >> -     ret = reset_controller_register(&rc->rcdev);
>> >> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>> >> >>       if (ret) {
>> >> >>               dev_err(&pdev->dev, "unable to register device\n");
>> >> >>               goto dis_clks;
>> >> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>> >> >>       if (ret)
>> >> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>> >> >>
>> >> >> -     reset_controller_unregister(&rc->rcdev);
>> >> >> -
>> >> >>       clk_disable_unprepare(rc->clk_delay);
>> >> >>       clk_disable_unprepare(rc->clk_reg);
>> >> >>
>> >> >
>> >> > Hmm, would this patch theoretically allow a window between the calls to
>> >> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
>> >> > reset_control_get() + reset_control_(de)assert() would access unclocked
>> >> > registers?
>> >>
>> >> This is not clear to me.
>> >>
>> >> Why reset_control_get() + reset_control_(de)assert() would happen here?
>> >
>> > I suppose on a non-SMP device, without parallel probing this can't
>> > really happen in practice.
>> > It still seems weird that suddenly we disable the clocks before
>> > unregistering the reset controller instead of afterwards.
>> >
>>
>> I still do not understand what you mean.
>>
>> This patch moves the reset_controller_unregister() call
>> after clk_disable_unprepare().
>
> And so the register access is made impossible before the reset
> controller device actually vanishes from the publicly visible list.
>
>> But, reset_controller_unregister() is just a manipulation of a liked list.
>> It does not trigger any hardware access.
>>
>> Am I wrong?
>
> No, you are perfectly right. I don't see how this can be a real problem
> unless at the same time another driver could try to request the still
> available reset control.


Ah, now I understood.
Thanks!



-- 
Best Regards
Masahiro Yamada

[PATCH 4/7] reset: pistachio: use devm_reset_controller_register()

[Masahiro Yamada]

Use devm_reset_controller_register() for the reset controller
registration and drop the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-pistachio.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/reset/reset-pistachio.c b/drivers/reset/reset-pistachio.c
index 72a97a1..bbc4c06 100644
--- a/drivers/reset/reset-pistachio.c
+++ b/drivers/reset/reset-pistachio.c
@@ -121,16 +121,7 @@ static int pistachio_reset_probe(struct platform_device *pdev)
 	rd->rcdev.ops = &pistachio_reset_ops;
 	rd->rcdev.of_node = np;
 
-	return reset_controller_register(&rd->rcdev);
-}
-
-static int pistachio_reset_remove(struct platform_device *pdev)
-{
-	struct pistachio_reset_data *data = platform_get_drvdata(pdev);
-
-	reset_controller_unregister(&data->rcdev);
-
-	return 0;
+	return devm_reset_controller_register(dev, &rd->rcdev);
 }
 
 static const struct of_device_id pistachio_reset_dt_ids[] = {
@@ -141,7 +132,6 @@ MODULE_DEVICE_TABLE(of, pistachio_reset_dt_ids);
 
 static struct platform_driver pistachio_reset_driver = {
 	.probe	= pistachio_reset_probe,
-	.remove	= pistachio_reset_remove,
 	.driver = {
 		.name		= "pistachio-reset",
 		.of_match_table	= pistachio_reset_dt_ids,
-- 
1.9.1

[PATCH 5/7] reset: sunxi: use devm_reset_controller_register()

[Masahiro Yamada]

Use devm_reset_controller_register() for the reset controller
registration and drop the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-sunxi.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c
index 677f865..3080190 100644
--- a/drivers/reset/reset-sunxi.c
+++ b/drivers/reset/reset-sunxi.c
@@ -165,21 +165,11 @@ static int sunxi_reset_probe(struct platform_device *pdev)
 	data->rcdev.ops = &sunxi_reset_ops;
 	data->rcdev.of_node = pdev->dev.of_node;
 
-	return reset_controller_register(&data->rcdev);
-}
-
-static int sunxi_reset_remove(struct platform_device *pdev)
-{
-	struct sunxi_reset_data *data = platform_get_drvdata(pdev);
-
-	reset_controller_unregister(&data->rcdev);
-
-	return 0;
+	return devm_reset_controller_register(&pdev->dev, &data->rcdev);
 }
 
 static struct platform_driver sunxi_reset_driver = {
 	.probe	= sunxi_reset_probe,
-	.remove	= sunxi_reset_remove,
 	.driver = {
 		.name		= "sunxi-reset",
 		.of_match_table	= sunxi_reset_dt_ids,
-- 
1.9.1

[PATCH 6/7] reset: socfpga: use devm_reset_controller_register()

[Masahiro Yamada]

Use devm_reset_controller_register() for the reset controller
registration and drop the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-socfpga.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
index cd05a70..12add9b 100644
--- a/drivers/reset/reset-socfpga.c
+++ b/drivers/reset/reset-socfpga.c
@@ -134,16 +134,7 @@ static int socfpga_reset_probe(struct platform_device *pdev)
 	data->rcdev.ops = &socfpga_reset_ops;
 	data->rcdev.of_node = pdev->dev.of_node;
 
-	return reset_controller_register(&data->rcdev);
-}
-
-static int socfpga_reset_remove(struct platform_device *pdev)
-{
-	struct socfpga_reset_data *data = platform_get_drvdata(pdev);
-
-	reset_controller_unregister(&data->rcdev);
-
-	return 0;
+	return devm_reset_controller_register(dev, &data->rcdev);
 }
 
 static const struct of_device_id socfpga_reset_dt_ids[] = {
@@ -153,7 +144,6 @@ static const struct of_device_id socfpga_reset_dt_ids[] = {
 
 static struct platform_driver socfpga_reset_driver = {
 	.probe	= socfpga_reset_probe,
-	.remove	= socfpga_reset_remove,
 	.driver = {
 		.name		= "socfpga-reset",
 		.of_match_table	= socfpga_reset_dt_ids,
-- 
1.9.1

[PATCH 7/7] reset: zynq: use devm_reset_controller_register()

[Masahiro Yamada]

Use devm_reset_controller_register() for the reset controller
registration and drop the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-zynq.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
index a7e87bc..138f2f2 100644
--- a/drivers/reset/reset-zynq.c
+++ b/drivers/reset/reset-zynq.c
@@ -122,16 +122,7 @@ static int zynq_reset_probe(struct platform_device *pdev)
 	priv->rcdev.ops = &zynq_reset_ops;
 	priv->rcdev.of_node = pdev->dev.of_node;
 
-	return reset_controller_register(&priv->rcdev);
-}
-
-static int zynq_reset_remove(struct platform_device *pdev)
-{
-	struct zynq_reset_data *priv = platform_get_drvdata(pdev);
-
-	reset_controller_unregister(&priv->rcdev);
-
-	return 0;
+	return devm_reset_controller_register(&pdev->dev, &priv->rcdev);
 }
 
 static const struct of_device_id zynq_reset_dt_ids[] = {
@@ -141,7 +132,6 @@ static const struct of_device_id zynq_reset_dt_ids[] = {
 
 static struct platform_driver zynq_reset_driver = {
 	.probe	= zynq_reset_probe,
-	.remove	= zynq_reset_remove,
 	.driver = {
 		.name		= KBUILD_MODNAME,
 		.of_match_table	= zynq_reset_dt_ids,
-- 
1.9.1