* [RESEND PATCHv2] Add SNVS clock support to rtc-snvs driver @ 2014-12-03 5:58 Sanchayan Maity 2014-12-03 5:58 ` [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support Sanchayan Maity 0 siblings, 1 reply; 9+ messages in thread From: Sanchayan Maity @ 2014-12-03 5:58 UTC (permalink / raw) To: a.zummo, rtc-linux; +Cc: stefan, shawn.guo, linux-kernel, Sanchayan Maity For v2: As such this patch remains the same, but is being resend and in isolation after the previous discussions here. https://lkml.org/lkml/2014/11/7/295 Since the changes on rtc-snvs.c will need to go through the rtc subsystem tree and the DT changes cannot be applied by Shawn, till this driver patch gets mainlined, this driver patch I am sending by itself without the DT changes send earlier. The first version of the patches were send quiet a while back. https://lkml.org/lkml/2014/9/26/492 The reason for sending this v2 of the patches late was to prevent any changes midway to the VF dts patches being tried with the below commit. https://lkml.org/lkml/2014/10/22/605 Changes since v1: The clock enable and disable functions for the SNVS are now optional and only enable/disable it, if it has been defined in the device tree node. Instead of failing and returning error as in the earlier set of patches, this makes sure that the driver does not break for the other i.MX devices for which the clock has not been defined and enabled elsewhere. Sanchayan Maity (1): drivers/rtc/rtc-snvs: Add clock support drivers/rtc/rtc-snvs.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-03 5:58 [RESEND PATCHv2] Add SNVS clock support to rtc-snvs driver Sanchayan Maity @ 2014-12-03 5:58 ` Sanchayan Maity 2014-12-04 9:36 ` Stefan Agner ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Sanchayan Maity @ 2014-12-03 5:58 UTC (permalink / raw) To: a.zummo, rtc-linux; +Cc: stefan, shawn.guo, linux-kernel, Sanchayan Maity This patch adds clock enable and disable support for the SNVS peripheral, which is required for using the RTC within the SNVS block. The clock is not strictly enforced, as this would break the i.MX devices. The clocking for the i.MX devices seems to be enabled elsewhere and enabling RTC SNVS for Vybrid results in a crash. This patch adds the clock support but also makes it optional so Vybrid platform can use the clock if defined while making sure not to break i.MX . Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com> --- drivers/rtc/rtc-snvs.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c index fa384fe..d4a6512 100644 --- a/drivers/rtc/rtc-snvs.c +++ b/drivers/rtc/rtc-snvs.c @@ -17,6 +17,7 @@ #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/rtc.h> +#include <linux/clk.h> /* These register offsets are relative to LP (Low Power) range */ #define SNVS_LPCR 0x04 @@ -39,6 +40,7 @@ struct snvs_rtc_data { void __iomem *ioaddr; int irq; spinlock_t lock; + struct clk *clk; }; static u32 rtc_read_lp_counter(void __iomem *ioaddr) @@ -260,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev) if (data->irq < 0) return data->irq; + data->clk = devm_clk_get(&pdev->dev, "snvs-rtc"); + if (IS_ERR(data->clk)) { + data->clk = NULL; + } else { + ret = clk_prepare_enable(data->clk); + if (ret) { + dev_err(&pdev->dev, + "Could not prepare or enable the snvs clock\n"); + return ret; + } + } + platform_set_drvdata(pdev, data); spin_lock_init(&data->lock); @@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev) if (ret) { dev_err(&pdev->dev, "failed to request irq %d: %d\n", data->irq, ret); - return ret; + goto error_rtc_device_register; } data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name, @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev) if (IS_ERR(data->rtc)) { ret = PTR_ERR(data->rtc); dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); - return ret; + goto error_rtc_device_register; } return 0; + +error_rtc_device_register: + if (data->clk) + clk_disable_unprepare(data->clk); + + return ret; } #ifdef CONFIG_PM_SLEEP @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev) if (device_may_wakeup(dev)) enable_irq_wake(data->irq); + if (data->clk) + clk_disable_unprepare(data->clk); + return 0; } static int snvs_rtc_resume(struct device *dev) { struct snvs_rtc_data *data = dev_get_drvdata(dev); + int ret; if (device_may_wakeup(dev)) disable_irq_wake(data->irq); + if (data->clk) { + ret = clk_prepare_enable(data->clk); + if (ret) + return ret; + } + return 0; } #endif -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-03 5:58 ` [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support Sanchayan Maity @ 2014-12-04 9:36 ` Stefan Agner 2014-12-04 10:05 ` Alessandro Zummo 2014-12-05 0:12 ` Andrew Morton 2 siblings, 0 replies; 9+ messages in thread From: Stefan Agner @ 2014-12-04 9:36 UTC (permalink / raw) To: Sanchayan Maity; +Cc: a.zummo, rtc-linux, shawn.guo, linux-kernel On 2014-12-03 06:58, Sanchayan Maity wrote: > This patch adds clock enable and disable support for > the SNVS peripheral, which is required for using the > RTC within the SNVS block. > > The clock is not strictly enforced, as this would > break the i.MX devices. The clocking for the i.MX > devices seems to be enabled elsewhere and enabling > RTC SNVS for Vybrid results in a crash. This patch > adds the clock support but also makes it optional > so Vybrid platform can use the clock if defined > while making sure not to break i.MX . > > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com> > --- > drivers/rtc/rtc-snvs.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c > index fa384fe..d4a6512 100644 > --- a/drivers/rtc/rtc-snvs.c > +++ b/drivers/rtc/rtc-snvs.c > @@ -17,6 +17,7 @@ > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/rtc.h> > +#include <linux/clk.h> > > /* These register offsets are relative to LP (Low Power) range */ > #define SNVS_LPCR 0x04 > @@ -39,6 +40,7 @@ struct snvs_rtc_data { > void __iomem *ioaddr; > int irq; > spinlock_t lock; > + struct clk *clk; > }; > > static u32 rtc_read_lp_counter(void __iomem *ioaddr) > @@ -260,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev) > if (data->irq < 0) > return data->irq; > > + data->clk = devm_clk_get(&pdev->dev, "snvs-rtc"); > + if (IS_ERR(data->clk)) { > + data->clk = NULL; > + } else { > + ret = clk_prepare_enable(data->clk); > + if (ret) { > + dev_err(&pdev->dev, > + "Could not prepare or enable the snvs clock\n"); > + return ret; > + } > + } > + > platform_set_drvdata(pdev, data); > > spin_lock_init(&data->lock); > @@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev) > if (ret) { > dev_err(&pdev->dev, "failed to request irq %d: %d\n", > data->irq, ret); > - return ret; > + goto error_rtc_device_register; > } > > data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name, > @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev) > if (IS_ERR(data->rtc)) { > ret = PTR_ERR(data->rtc); > dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); > - return ret; > + goto error_rtc_device_register; > } > > return 0; > + > +error_rtc_device_register: > + if (data->clk) > + clk_disable_unprepare(data->clk); > + > + return ret; > } > > #ifdef CONFIG_PM_SLEEP > @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev) > if (device_may_wakeup(dev)) > enable_irq_wake(data->irq); > > + if (data->clk) > + clk_disable_unprepare(data->clk); > + > return 0; > } > > static int snvs_rtc_resume(struct device *dev) > { > struct snvs_rtc_data *data = dev_get_drvdata(dev); > + int ret; > > if (device_may_wakeup(dev)) > disable_irq_wake(data->irq); > > + if (data->clk) { > + ret = clk_prepare_enable(data->clk); > + if (ret) > + return ret; > + } > + > return 0; > } > #endif Acked-by: Stefan Agner <stefan@agner.ch> -- Stefan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-03 5:58 ` [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support Sanchayan Maity 2014-12-04 9:36 ` Stefan Agner @ 2014-12-04 10:05 ` Alessandro Zummo 2014-12-04 12:29 ` Stefan Agner 2014-12-05 0:12 ` Andrew Morton 2 siblings, 1 reply; 9+ messages in thread From: Alessandro Zummo @ 2014-12-04 10:05 UTC (permalink / raw) To: Sanchayan Maity; +Cc: rtc-linux, stefan, shawn.guo, linux-kernel, akpm On Wed, 3 Dec 2014 11:28:55 +0530 Sanchayan Maity <maitysanchayan@gmail.com> wrote: > The clock is not strictly enforced, as this would > break the i.MX devices. The clocking for the i.MX > devices seems to be enabled elsewhere and enabling > RTC SNVS for Vybrid results in a crash. This patch > adds the clock support but also makes it optional > so Vybrid platform can use the clock if defined > while making sure not to break i.MX . > > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com> Acked-by: Alessandro Zummo <a.zummo@towertech.it> -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-04 10:05 ` Alessandro Zummo @ 2014-12-04 12:29 ` Stefan Agner 2014-12-04 16:54 ` Alessandro Zummo 0 siblings, 1 reply; 9+ messages in thread From: Stefan Agner @ 2014-12-04 12:29 UTC (permalink / raw) To: Alessandro Zummo Cc: Sanchayan Maity, rtc-linux, shawn.guo, linux-kernel, akpm On 2014-12-04 11:05, Alessandro Zummo wrote: > On Wed, 3 Dec 2014 11:28:55 +0530 > Sanchayan Maity <maitysanchayan@gmail.com> wrote: > >> The clock is not strictly enforced, as this would >> break the i.MX devices. The clocking for the i.MX >> devices seems to be enabled elsewhere and enabling >> RTC SNVS for Vybrid results in a crash. This patch >> adds the clock support but also makes it optional >> so Vybrid platform can use the clock if defined >> while making sure not to break i.MX . >> >> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com> > > > Acked-by: Alessandro Zummo <a.zummo@towertech.it> Through which tree is this going upstream? -- Stefan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-04 12:29 ` Stefan Agner @ 2014-12-04 16:54 ` Alessandro Zummo 0 siblings, 0 replies; 9+ messages in thread From: Alessandro Zummo @ 2014-12-04 16:54 UTC (permalink / raw) To: Stefan Agner; +Cc: Sanchayan Maity, rtc-linux, shawn.guo, linux-kernel, akpm On Thu, 04 Dec 2014 13:29:58 +0100 Stefan Agner <stefan@agner.ch> wrote: > > Acked-by: Alessandro Zummo <a.zummo@towertech.it> > > Through which tree is this going upstream? Andrew usually picks up those small patches, unless there's a more appropriate tree. -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-03 5:58 ` [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support Sanchayan Maity 2014-12-04 9:36 ` Stefan Agner 2014-12-04 10:05 ` Alessandro Zummo @ 2014-12-05 0:12 ` Andrew Morton 2014-12-05 14:54 ` Stefan Agner 2 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2014-12-05 0:12 UTC (permalink / raw) To: Sanchayan Maity; +Cc: a.zummo, rtc-linux, stefan, shawn.guo, linux-kernel On Wed, 3 Dec 2014 11:28:55 +0530 Sanchayan Maity <maitysanchayan@gmail.com> wrote: > This patch adds clock enable and disable support for > the SNVS peripheral, which is required for using the > RTC within the SNVS block. > > The clock is not strictly enforced, as this would > break the i.MX devices. The clocking for the i.MX > devices seems to be enabled elsewhere and enabling > RTC SNVS for Vybrid results in a crash. This patch > adds the clock support but also makes it optional > so Vybrid platform can use the clock if defined > while making sure not to break i.MX . > > ... I assume this hasn't been tested with CONFIG_HAVE_CLK=n. It all *looks* OK, but the clk API does its tests for clk==NULL in some very deep places. > --- a/drivers/rtc/rtc-snvs.c > +++ b/drivers/rtc/rtc-snvs.c . > ... > > @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev) > if (IS_ERR(data->rtc)) { > ret = PTR_ERR(data->rtc); > dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); > - return ret; > + goto error_rtc_device_register; > } > > return 0; > + > +error_rtc_device_register: > + if (data->clk) > + clk_disable_unprepare(data->clk); >From my reading, there's no need to test data->clk here - clk_disable_unprepare() handles that. It doesn't hurt to test it - a reasonable approach would be to look around at what other users of the clk API are doing, and do that. > + return ret; > } > > #ifdef CONFIG_PM_SLEEP > @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev) > if (device_may_wakeup(dev)) > enable_irq_wake(data->irq); > > + if (data->clk) > + clk_disable_unprepare(data->clk); Ditto. > return 0; > } > > static int snvs_rtc_resume(struct device *dev) > { > struct snvs_rtc_data *data = dev_get_drvdata(dev); > + int ret; > > if (device_may_wakeup(dev)) > disable_irq_wake(data->irq); > > + if (data->clk) { > + ret = clk_prepare_enable(data->clk); > + if (ret) > + return ret; > + } It could be omitted here also. > return 0; > } > #endif ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-05 0:12 ` Andrew Morton @ 2014-12-05 14:54 ` Stefan Agner 2014-12-05 21:44 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Stefan Agner @ 2014-12-05 14:54 UTC (permalink / raw) To: Andrew Morton Cc: Sanchayan Maity, a.zummo, rtc-linux, shawn.guo, linux-kernel On 2014-12-05 01:12, Andrew Morton wrote: > On Wed, 3 Dec 2014 11:28:55 +0530 Sanchayan Maity > <maitysanchayan@gmail.com> wrote: > >> This patch adds clock enable and disable support for >> the SNVS peripheral, which is required for using the >> RTC within the SNVS block. >> >> The clock is not strictly enforced, as this would >> break the i.MX devices. The clocking for the i.MX >> devices seems to be enabled elsewhere and enabling >> RTC SNVS for Vybrid results in a crash. This patch >> adds the clock support but also makes it optional >> so Vybrid platform can use the clock if defined >> while making sure not to break i.MX . >> >> ... > > I assume this hasn't been tested with CONFIG_HAVE_CLK=n. It all > *looks* OK, but the clk API does its tests for clk==NULL in some very > deep places. > >> --- a/drivers/rtc/rtc-snvs.c >> +++ b/drivers/rtc/rtc-snvs.c > . >> ... >> >> @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev) >> if (IS_ERR(data->rtc)) { >> ret = PTR_ERR(data->rtc); >> dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); >> - return ret; >> + goto error_rtc_device_register; >> } >> >> return 0; >> + >> +error_rtc_device_register: >> + if (data->clk) >> + clk_disable_unprepare(data->clk); > > From my reading, there's no need to test data->clk here - > clk_disable_unprepare() handles that. > > It doesn't hurt to test it - a reasonable approach would be to look > around at what other users of the clk API are doing, and do that. > > >> + return ret; >> } >> >> #ifdef CONFIG_PM_SLEEP >> @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev) >> if (device_may_wakeup(dev)) >> enable_irq_wake(data->irq); >> >> + if (data->clk) >> + clk_disable_unprepare(data->clk); > > Ditto. > >> return 0; >> } >> >> static int snvs_rtc_resume(struct device *dev) >> { >> struct snvs_rtc_data *data = dev_get_drvdata(dev); >> + int ret; >> >> if (device_may_wakeup(dev)) >> disable_irq_wake(data->irq); >> >> + if (data->clk) { >> + ret = clk_prepare_enable(data->clk); >> + if (ret) >> + return ret; >> + } > Just discovered issue with suspend/resume I am testing right now: The alarm interrupt handler also reads registers which are part of SNVS and need clocks enabled. However, the resume function is called after IRQ's have been enabled, hence this leads to a abort: [ 90.755222] Unhandled fault: external abort on non-linefetch (0x1008) at 0x908c604c [ 90.762892] Internal error: : 1008 [#1] ARM [ 90.767082] Modules linked in: [ 90.770174] CPU: 0 PID: 421 Comm: sh Not tainted 3.18.0-rc5-00135-g0689c67-dirty #1592 [ 90.778100] task: 8e03e800 ti: 8cad8000 task.ti: 8cad8000 [ 90.783530] PC is at snvs_rtc_irq_handler+0x14/0x74 [ 90.788424] LR is at handle_irq_event_percpu+0x3c/0x144 It can be fixed, with something like this: @@ -346,7 +347,10 @@ static int snvs_rtc_resume(struct device *dev) } #endif -static SIMPLE_DEV_PM_OPS(snvs_rtc_pm_ops, snvs_rtc_suspend, snvs_rtc_resume); +static const struct dev_pm_ops snvs_rtc_pm_ops = { + .suspend_noirq = snvs_rtc_suspend, + .resume_noirq = snvs_rtc_resume, +}; static const struct of_device_id snvs_dt_ids[] = { { .compatible = "fsl,sec-v4.0-mon-rtc-lp", }, Andrew, this patch is already merged, so it would need another fix for this, right? -- Stefan > It could be omitted here also. > >> return 0; >> } >> #endif ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support 2014-12-05 14:54 ` Stefan Agner @ 2014-12-05 21:44 ` Andrew Morton 0 siblings, 0 replies; 9+ messages in thread From: Andrew Morton @ 2014-12-05 21:44 UTC (permalink / raw) To: Stefan Agner; +Cc: Sanchayan Maity, a.zummo, rtc-linux, shawn.guo, linux-kernel On Fri, 05 Dec 2014 15:54:15 +0100 Stefan Agner <stefan@agner.ch> wrote: > It can be fixed, with something like this: > > @@ -346,7 +347,10 @@ static int snvs_rtc_resume(struct device *dev) > } > #endif > > -static SIMPLE_DEV_PM_OPS(snvs_rtc_pm_ops, snvs_rtc_suspend, > snvs_rtc_resume); > +static const struct dev_pm_ops snvs_rtc_pm_ops = { > + .suspend_noirq = snvs_rtc_suspend, > + .resume_noirq = snvs_rtc_resume, > +}; > > static const struct of_device_id snvs_dt_ids[] = { > { .compatible = "fsl,sec-v4.0-mon-rtc-lp", }, > > > > Andrew, this patch is already merged, so it would need another fix for > this, right? A replacement patch is OK. An incremental patch is OK too. I almost always turn the replacement into an incremental so that I and others can see what changed. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-12-05 21:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-03 5:58 [RESEND PATCHv2] Add SNVS clock support to rtc-snvs driver Sanchayan Maity 2014-12-03 5:58 ` [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support Sanchayan Maity 2014-12-04 9:36 ` Stefan Agner 2014-12-04 10:05 ` Alessandro Zummo 2014-12-04 12:29 ` Stefan Agner 2014-12-04 16:54 ` Alessandro Zummo 2014-12-05 0:12 ` Andrew Morton 2014-12-05 14:54 ` Stefan Agner 2014-12-05 21:44 ` Andrew Morton
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).