* [RESEND PATCHv4 0/1] drivers/amba: add reset control to amba @ 2019-08-20 14:58 Dinh Nguyen 2019-08-20 14:58 ` [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe Dinh Nguyen 0 siblings, 1 reply; 6+ messages in thread From: Dinh Nguyen @ 2019-08-20 14:58 UTC (permalink / raw) To: linux-kernel Cc: robh, daniel.thompson, tony.luck, manivannan.sadhasivam, keescook, devicetree, linus.walleij, anton, linux, dinguyen, ccross, frowand.list, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe 2019-08-20 14:58 [RESEND PATCHv4 0/1] drivers/amba: add reset control to amba Dinh Nguyen @ 2019-08-20 14:58 ` Dinh Nguyen 2019-08-23 9:19 ` Linus Walleij 0 siblings, 1 reply; 6+ messages in thread From: Dinh Nguyen @ 2019-08-20 14:58 UTC (permalink / raw) To: linux-kernel Cc: robh, daniel.thompson, tony.luck, manivannan.sadhasivam, keescook, devicetree, linus.walleij, anton, linux, dinguyen, ccross, frowand.list, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe 2019-08-20 14:58 ` [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe Dinh Nguyen @ 2019-08-23 9:19 ` Linus Walleij 2019-08-23 15:42 ` Dinh Nguyen 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2019-08-23 9:19 UTC (permalink / raw) To: Dinh Nguyen, Philipp Zabel Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Daniel Thompson, Tony Luck, Manivannan Sadhasivam, Kees Cook, Rob Herring, Anton Vorontsov, Russell King, linux-kernel, Colin Cross, Frank Rowand, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe 2019-08-23 9:19 ` Linus Walleij @ 2019-08-23 15:42 ` Dinh Nguyen 2019-08-26 8:57 ` Philipp Zabel 0 siblings, 1 reply; 6+ messages in thread From: Dinh Nguyen @ 2019-08-23 15:42 UTC (permalink / raw) To: Linus Walleij, Philipp Zabel Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Daniel Thompson, Tony Luck, Manivannan Sadhasivam, Kees Cook, Rob Herring, Anton Vorontsov, Russell King, linux-kernel, Colin Cross, Frank Rowand, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe 2019-08-23 15:42 ` Dinh Nguyen @ 2019-08-26 8:57 ` Philipp Zabel 2019-08-26 15:27 ` Dinh Nguyen 0 siblings, 1 reply; 6+ messages in thread From: Philipp Zabel @ 2019-08-26 8:57 UTC (permalink / raw) To: Dinh Nguyen, Linus Walleij Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Daniel Thompson, Tony Luck, Manivannan Sadhasivam, Kees Cook, Rob Herring, Anton Vorontsov, Russell King, linux-kernel, Colin Cross, Frank Rowand, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe 2019-08-26 8:57 ` Philipp Zabel @ 2019-08-26 15:27 ` Dinh Nguyen 0 siblings, 0 replies; 6+ messages in thread From: Dinh Nguyen @ 2019-08-26 15:27 UTC (permalink / raw) To: Philipp Zabel, Linus Walleij Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Daniel Thompson, Tony Luck, Manivannan Sadhasivam, Kees Cook, Rob Herring, Anton Vorontsov, Russell King, linux-kernel, Colin Cross, Frank Rowand, Linux ARM 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-26 15:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-20 14:58 [RESEND PATCHv4 0/1] drivers/amba: add reset control to amba Dinh Nguyen 2019-08-20 14:58 ` [RESEND PATCHv4 1/1] drivers/amba: add reset control to amba bus probe Dinh Nguyen 2019-08-23 9:19 ` Linus Walleij 2019-08-23 15:42 ` Dinh Nguyen 2019-08-26 8:57 ` Philipp Zabel 2019-08-26 15:27 ` Dinh Nguyen
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).