[RESEND PATCHv4 0/1] drivers/amba: add reset control to amba

[RESEND PATCHv4 0/1] drivers/amba: add reset control to amba

[Dinh Nguyen]

Hello,

Even though this patch is a V4, I'm including more people in this review
cycle because I found that there was previous patch[1] that was discussed.

Thanks,
Dinh


[1] https://patchwork.kernel.org/patch/10845695/

Dinh Nguyen (1):
  drivers/amba: add reset control to amba bus probe

 drivers/amba/bus.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

-- 
2.20.0

[RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe

[Dinh Nguyen]

The primecell controller on some SoCs, i.e. SoCFPGA, is held in reset by
default. Until recently, the DMA controller was brought out of reset by the
bootloader(i.e. U-Boot). But a recent change in U-Boot, the peripherals
that are not used are held in reset and are left to Linux to bring them
out of reset.

Add a mechanism for getting the reset property and de-assert the primecell
module from reset if found. This is a not a hard fail if the reset properti
is not present in the device tree node, so the driver will continue to
probe.

Because there are different variants of the controller that may have
multiple reset signals, the code will find all reset(s) specified and
de-assert them.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v4: cleaned up indentation in loop
    fix up a few checkpatch warnings
    add Reviewed-by:
v3: add a reset_control_put()
    add error handling
v2: move reset control to bus code
    find all reset properties and de-assert them
---
 drivers/amba/bus.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 100e798a5c82..76a1cd56a1ab 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -18,6 +18,7 @@
 #include <linux/limits.h>
 #include <linux/clk/clk-conf.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 
 #include <asm/irq.h>
 
@@ -401,6 +402,26 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 	ret = amba_get_enable_pclk(dev);
 	if (ret == 0) {
 		u32 pid, cid;
+		int count;
+		struct reset_control *rstc;
+
+		/*
+		 * Find reset control(s) of the amba bus and de-assert them.
+		 */
+		count = reset_control_get_count(&dev->dev);
+		while (count > 0) {
+			rstc = of_reset_control_get_shared_by_index(dev->dev.of_node, count - 1);
+			if (IS_ERR(rstc)) {
+				if (PTR_ERR(rstc) == -EPROBE_DEFER)
+					ret = -EPROBE_DEFER;
+				else
+					dev_err(&dev->dev, "Can't get amba reset!\n");
+				break;
+			}
+			reset_control_deassert(rstc);
+			reset_control_put(rstc);
+			count--;
+		}
 
 		/*
 		 * Read pid and cid based on size of resource
-- 
2.20.0

Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe

[Linus Walleij]

On Tue, Aug 20, 2019 at 4:58 PM Dinh Nguyen <dinguyen@kernel.org> wrote:

> @@ -401,6 +402,26 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>         ret = amba_get_enable_pclk(dev);
>         if (ret == 0) {
>                 u32 pid, cid;
> +               int count;
> +               struct reset_control *rstc;
> +
> +               /*
> +                * Find reset control(s) of the amba bus and de-assert them.
> +                */
> +               count = reset_control_get_count(&dev->dev);
> +               while (count > 0) {
> +                       rstc = of_reset_control_get_shared_by_index(dev->dev.of_node, count - 1);
> +                       if (IS_ERR(rstc)) {
> +                               if (PTR_ERR(rstc) == -EPROBE_DEFER)
> +                                       ret = -EPROBE_DEFER;
> +                               else
> +                                       dev_err(&dev->dev, "Can't get amba reset!\n");
> +                               break;
> +                       }
> +                       reset_control_deassert(rstc);
> +                       reset_control_put(rstc);
> +                       count--;
> +               }

I'm not normally a footprint person, but the looks of the stubs in
<linux/reset.h> makes me suspicious whether this will have zero impact
in size on platforms without reset controllers.

Can you just ls -al on the kernel without CONFIG_RESET_CONTROLLER
before and after this patch and ascertain that it has zero footprint effect?

If it doesn't I'd sure like to break this into its own function and
stick a if (!IS_ENABLED(CONFIG_RESET_CONTROLLER)) return 0;
in there to make sure the compiler drops it.

Also it'd be nice to get Philipp's ACK on the semantics, though they
look correct to me.

Yours,
Linus Walleij

Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe

[Dinh Nguyen]

On 8/23/19 4:19 AM, Linus Walleij wrote:
> On Tue, Aug 20, 2019 at 4:58 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> 
>> @@ -401,6 +402,26 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>>         ret = amba_get_enable_pclk(dev);
>>         if (ret == 0) {
>>                 u32 pid, cid;
>> +               int count;
>> +               struct reset_control *rstc;
>> +
>> +               /*
>> +                * Find reset control(s) of the amba bus and de-assert them.
>> +                */
>> +               count = reset_control_get_count(&dev->dev);
>> +               while (count > 0) {
>> +                       rstc = of_reset_control_get_shared_by_index(dev->dev.of_node, count - 1);
>> +                       if (IS_ERR(rstc)) {
>> +                               if (PTR_ERR(rstc) == -EPROBE_DEFER)
>> +                                       ret = -EPROBE_DEFER;
>> +                               else
>> +                                       dev_err(&dev->dev, "Can't get amba reset!\n");
>> +                               break;
>> +                       }
>> +                       reset_control_deassert(rstc);
>> +                       reset_control_put(rstc);
>> +                       count--;
>> +               }
> 
> I'm not normally a footprint person, but the looks of the stubs in
> <linux/reset.h> makes me suspicious whether this will have zero impact
> in size on platforms without reset controllers.
> 
> Can you just ls -al on the kernel without CONFIG_RESET_CONTROLLER
> before and after this patch and ascertain that it has zero footprint effect?

Thanks for the review. I checked it, and indeed, it does have a zero
footprint effect.

> 
> If it doesn't I'd sure like to break this into its own function and
> stick a if (!IS_ENABLED(CONFIG_RESET_CONTROLLER)) return 0;
> in there to make sure the compiler drops it.
> 
> Also it'd be nice to get Philipp's ACK on the semantics, though they
> look correct to me.
> 

Dinh

Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe

[Philipp Zabel]

Hi Dinh, Linus,

On Fri, 2019-08-23 at 10:42 -0500, Dinh Nguyen wrote:
> 
> On 8/23/19 4:19 AM, Linus Walleij wrote:
> > On Tue, Aug 20, 2019 at 4:58 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
> > 
> > > @@ -401,6 +402,26 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
> > >         ret = amba_get_enable_pclk(dev);
> > >         if (ret == 0) {
> > >                 u32 pid, cid;
> > > +               int count;
> > > +               struct reset_control *rstc;
> > > +
> > > +               /*
> > > +                * Find reset control(s) of the amba bus and de-assert them.
> > > +                */
> > > +               count = reset_control_get_count(&dev->dev);

The reset_control_get_count() inline stub returns -ENOENT, so the
compiler can throw away the complete loop.

> > > +               while (count > 0) {
> > > +                       rstc = of_reset_control_get_shared_by_index(dev->dev.of_node, count - 1);

Since resets are looked up with of_reset_control_get, I'd use
of_reset_control_get_count() above for consistency. But see below:

> > > +                       if (IS_ERR(rstc)) {
> > > +                               if (PTR_ERR(rstc) == -EPROBE_DEFER)
> > > +                                       ret = -EPROBE_DEFER;
> > > +                               else
> > > +                                       dev_err(&dev->dev, "Can't get amba reset!\n");
> > > +                               break;
> > > +                       }
> > > +                       reset_control_deassert(rstc);
> > > +                       reset_control_put(rstc);
> > > +                       count--;
> > > +               }

It looks like the order of deassertions is irrelevant. If so,
You can use of_reset_control_array_get() to simplify this:

+		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
+		if (IS_ERR(rstc)) {
+			if (PTR_ERR(rstc) != -EPROBE_DEFER)
+				dev_err(&dev->dev, "Can't get amba reset!\n");
+			return PTR_ERR(rstc);
+		}
+		reset_control_deassert(rstc);
+		reset_control_put(rstc);

> > I'm not normally a footprint person, but the looks of the stubs in
> > <linux/reset.h> makes me suspicious whether this will have zero impact
> > in size on platforms without reset controllers.
> > 
> > Can you just ls -al on the kernel without CONFIG_RESET_CONTROLLER
> > before and after this patch and ascertain that it has zero footprint effect?
> 
> Thanks for the review. I checked it, and indeed, it does have a zero
> footprint effect.
> 
> > 
> > If it doesn't I'd sure like to break this into its own function and
> > stick a if (!IS_ENABLED(CONFIG_RESET_CONTROLLER)) return 0;
> > in there to make sure the compiler drops it.
> > 
> > Also it'd be nice to get Philipp's ACK on the semantics, though they
> > look correct to me.

regards
Philipp

Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe

[Dinh Nguyen]

Hi Philipp,

On 8/26/19 3:57 AM, Philipp Zabel wrote:
> Hi Dinh, Linus,
> 
> On Fri, 2019-08-23 at 10:42 -0500, Dinh Nguyen wrote:
>>
>> On 8/23/19 4:19 AM, Linus Walleij wrote:
>>> On Tue, Aug 20, 2019 at 4:58 PM Dinh Nguyen <dinguyen@kernel.org> wrote:
>>>
>>>> @@ -401,6 +402,26 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>>>>         ret = amba_get_enable_pclk(dev);
>>>>         if (ret == 0) {
>>>>                 u32 pid, cid;
>>>> +               int count;
>>>> +               struct reset_control *rstc;
>>>> +
>>>> +               /*
>>>> +                * Find reset control(s) of the amba bus and de-assert them.
>>>> +                */
>>>> +               count = reset_control_get_count(&dev->dev);
> 
> The reset_control_get_count() inline stub returns -ENOENT, so the
> compiler can throw away the complete loop.
> 
>>>> +               while (count > 0) {
>>>> +                       rstc = of_reset_control_get_shared_by_index(dev->dev.of_node, count - 1);
> 
> Since resets are looked up with of_reset_control_get, I'd use
> of_reset_control_get_count() above for consistency. But see below:
> 

reset_control_get_count() ultimately calls of_reset_control_get_count()
and it looks like of_reset_control_get_count() is not exported.

>>>> +                       if (IS_ERR(rstc)) {
>>>> +                               if (PTR_ERR(rstc) == -EPROBE_DEFER)
>>>> +                                       ret = -EPROBE_DEFER;
>>>> +                               else
>>>> +                                       dev_err(&dev->dev, "Can't get amba reset!\n");
>>>> +                               break;
>>>> +                       }
>>>> +                       reset_control_deassert(rstc);
>>>> +                       reset_control_put(rstc);
>>>> +                       count--;
>>>> +               }
> 
> It looks like the order of deassertions is irrelevant. If so,
> You can use of_reset_control_array_get() to simplify this:
> 
> +		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
> +		if (IS_ERR(rstc)) {
> +			if (PTR_ERR(rstc) != -EPROBE_DEFER)
> +				dev_err(&dev->dev, "Can't get amba reset!\n");
> +			return PTR_ERR(rstc);
> +		}
> +		reset_control_deassert(rstc);
> +		reset_control_put(rstc);
> 

Thanks for the review! I'll post v5 shortly.

Dinh