| Message ID | 20180421142344.25944-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Miquel, On sam., avril 21 2018, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Add suspend/resume hooks in Armada 37xx peripheral clocks driver to > handle S2RAM operations. > > One can think that these hooks are useless by comparing the register > values before and after a suspend/resume cycle: they will look the same > anyway. This is because of some scripts executed by the Cortex-M3 core > during ATF operations to init both the clocks and the DDR. These values > could be modified by the BL33 stage or by Linux itself and should be > preserved. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/clk/mvebu/armada-37xx-periph.c | 48 ++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c > index 723bb7f6cea1..cd560bdcb8a9 100644 > --- a/drivers/clk/mvebu/armada-37xx-periph.c > +++ b/drivers/clk/mvebu/armada-37xx-periph.c > @@ -59,6 +59,15 @@ struct clk_periph_driver_data { > struct clk_hw_onecell_data *hw_data; > spinlock_t lock; > void __iomem *reg; > +#if defined(CONFIG_PM) > + /* Storage registers for suspend/resume operations */ > + u32 tbg_sel; > + u32 div_sel0; > + u32 div_sel1; > + u32 div_sel2; > + u32 clk_sel; > + u32 clk_dis; > +#endif /* CONFIG_PM */ > }; > > struct clk_double_div { > @@ -642,6 +651,42 @@ static int armada_3700_add_composite_clk(const struct clk_periph_data *data, > return PTR_ERR_OR_ZERO(*hw); > } > > +#if defined(CONFIG_PM) I think you could get rid of this conditional code. Have a look on what is done in others drivers around DEV_PM_OPS and __maybe_unused. Gregory > +static int armada_3700_periph_clock_suspend(struct device *dev) > +{ > + struct clk_periph_driver_data *data = dev_get_drvdata(dev); > + > + data->tbg_sel = readl(data->reg + TBG_SEL); > + data->div_sel0 = readl(data->reg + DIV_SEL0); > + data->div_sel1 = readl(data->reg + DIV_SEL1); > + data->div_sel2 = readl(data->reg + DIV_SEL2); > + data->clk_sel = readl(data->reg + CLK_SEL); > + data->clk_dis = readl(data->reg + CLK_DIS); > + > + return 0; > +} > + > +static int armada_3700_periph_clock_resume(struct device *dev) > +{ > + struct clk_periph_driver_data *data = dev_get_drvdata(dev); > + > + /* Follow the same order than what the Cortex-M3 does (ATF code) */ > + writel(data->clk_dis, data->reg + CLK_DIS); > + writel(data->div_sel0, data->reg + DIV_SEL0); > + writel(data->div_sel1, data->reg + DIV_SEL1); > + writel(data->div_sel2, data->reg + DIV_SEL2); > + writel(data->tbg_sel, data->reg + TBG_SEL); > + writel(data->clk_sel, data->reg + CLK_SEL); > + > + return 0; > +} > + > +static const struct dev_pm_ops armada_3700_periph_clock_pm_ops = { > + .suspend = armada_3700_periph_clock_suspend, > + .resume = armada_3700_periph_clock_resume, > +}; > +#endif /* CONFIG_PM */ > + > static int armada_3700_periph_clock_probe(struct platform_device *pdev) > { > struct clk_periph_driver_data *driver_data; > @@ -716,6 +761,9 @@ static struct platform_driver armada_3700_periph_clock_driver = { > .driver = { > .name = "marvell-armada-3700-periph-clock", > .of_match_table = armada_3700_periph_clock_of_match, > +#if defined(CONFIG_PM) > + .pm = &armada_3700_periph_clock_pm_ops, > +#endif /* CONFIG_PM */ > }, > }; > > -- > 2.14.1 >
Hi Gregory, On Wed, 02 May 2018 18:18:37 +0200, Gregory CLEMENT <gregory.clement@bootlin.com> wrote: > Hi Miquel, > > On sam., avril 21 2018, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Add suspend/resume hooks in Armada 37xx peripheral clocks driver to > > handle S2RAM operations. > > > > One can think that these hooks are useless by comparing the register > > values before and after a suspend/resume cycle: they will look the same > > anyway. This is because of some scripts executed by the Cortex-M3 core > > during ATF operations to init both the clocks and the DDR. These values > > could be modified by the BL33 stage or by Linux itself and should be > > preserved. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/clk/mvebu/armada-37xx-periph.c | 48 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c > > index 723bb7f6cea1..cd560bdcb8a9 100644 > > --- a/drivers/clk/mvebu/armada-37xx-periph.c > > +++ b/drivers/clk/mvebu/armada-37xx-periph.c > > @@ -59,6 +59,15 @@ struct clk_periph_driver_data { > > struct clk_hw_onecell_data *hw_data; > > spinlock_t lock; > > void __iomem *reg; > > +#if defined(CONFIG_PM) > > + /* Storage registers for suspend/resume operations */ > > + u32 tbg_sel; > > + u32 div_sel0; > > + u32 div_sel1; > > + u32 div_sel2; > > + u32 clk_sel; > > + u32 clk_dis; > > +#endif /* CONFIG_PM */ > > }; > > > > struct clk_double_div { > > @@ -642,6 +651,42 @@ static int armada_3700_add_composite_clk(const struct clk_periph_data *data, > > return PTR_ERR_OR_ZERO(*hw); > > } > > > > +#if defined(CONFIG_PM) > > I think you could get rid of this conditional code. Have a look on what > is done in others drivers around DEV_PM_OPS and __maybe_unused. Thanks for pointing it. Actually most implementations do not use __maybe_unused but do something like: #if defined(CONFIG_PM) static int xxx_pm_suspend() { ... } static void xxx_pm_resume() { ... } static struct pm_ops ops; #define XXX_DEV_PM_OPS &ops #else #define XXX_DEV_PM_OPS NULL #endif /* CONFIG_PM */ And then in the platform_driver structure: .pm = XXX_DEV_PM_OPS, This way, the only #if/#endif is "out" of the current code and the ".pm =" line is free of preprocessor macros. Thanks, Miquèl
Quoting Miquel Raynal (2018-06-26 02:09:18) > Hi Gregory, > > On Wed, 02 May 2018 18:18:37 +0200, Gregory CLEMENT > <gregory.clement@bootlin.com> wrote: > > On sam., avril 21 2018, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > +#if defined(CONFIG_PM) > > > > I think you could get rid of this conditional code. Have a look on what > > is done in others drivers around DEV_PM_OPS and __maybe_unused. > > Thanks for pointing it. > > Actually most implementations do not use __maybe_unused but do > something like: > > #if defined(CONFIG_PM) > > static int xxx_pm_suspend() { ... } > > static void xxx_pm_resume() { ... } > > static struct pm_ops ops; > #define XXX_DEV_PM_OPS &ops > #else > #define XXX_DEV_PM_OPS NULL > #endif /* CONFIG_PM */ > > And then in the platform_driver structure: > > .pm = XXX_DEV_PM_OPS, > > This way, the only #if/#endif is "out" of the current code and the > ".pm =" line is free of preprocessor macros. > Yes, less preprocessor conditionals throughout the code is better for code readability and maintainability. Please remove the conditionals and use __maybe_unused instead. Sadly, we'll waste 48 bytes on the save/restore registers, but that's OK.
Hi Stephen, Stephen Boyd <sboyd@kernel.org> wrote on Fri, 06 Jul 2018 08:58:38 -0700: > Quoting Miquel Raynal (2018-06-26 02:09:18) > > Hi Gregory, > > > > On Wed, 02 May 2018 18:18:37 +0200, Gregory CLEMENT > > <gregory.clement@bootlin.com> wrote: > > > On sam., avril 21 2018, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > +#if defined(CONFIG_PM) > > > > > > I think you could get rid of this conditional code. Have a look on what > > > is done in others drivers around DEV_PM_OPS and __maybe_unused. > > > > Thanks for pointing it. > > > > Actually most implementations do not use __maybe_unused but do > > something like: > > > > #if defined(CONFIG_PM) > > > > static int xxx_pm_suspend() { ... } > > > > static void xxx_pm_resume() { ... } > > > > static struct pm_ops ops; > > #define XXX_DEV_PM_OPS &ops > > #else > > #define XXX_DEV_PM_OPS NULL > > #endif /* CONFIG_PM */ > > > > And then in the platform_driver structure: > > > > .pm = XXX_DEV_PM_OPS, > > > > This way, the only #if/#endif is "out" of the current code and the > > ".pm =" line is free of preprocessor macros. > > > > Yes, less preprocessor conditionals throughout the code is better for > code readability and maintainability. Please remove the conditionals and > use __maybe_unused instead. Sadly, we'll waste 48 bytes on the > save/restore registers, but that's OK. > Mmmh. Looking at current code I thought the above was the preferred way. Ok then, I'll repost with the conditionals removed and __maybe_unused on the suspend/resume functions as initially suggested by Gregory. Thanks, Miquèl
diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c index 723bb7f6cea1..cd560bdcb8a9 100644 --- a/drivers/clk/mvebu/armada-37xx-periph.c +++ b/drivers/clk/mvebu/armada-37xx-periph.c @@ -59,6 +59,15 @@ struct clk_periph_driver_data { struct clk_hw_onecell_data *hw_data; spinlock_t lock; void __iomem *reg; +#if defined(CONFIG_PM) + /* Storage registers for suspend/resume operations */ + u32 tbg_sel; + u32 div_sel0; + u32 div_sel1; + u32 div_sel2; + u32 clk_sel; + u32 clk_dis; +#endif /* CONFIG_PM */ }; struct clk_double_div { @@ -642,6 +651,42 @@ static int armada_3700_add_composite_clk(const struct clk_periph_data *data, return PTR_ERR_OR_ZERO(*hw); } +#if defined(CONFIG_PM) +static int armada_3700_periph_clock_suspend(struct device *dev) +{ + struct clk_periph_driver_data *data = dev_get_drvdata(dev); + + data->tbg_sel = readl(data->reg + TBG_SEL); + data->div_sel0 = readl(data->reg + DIV_SEL0); + data->div_sel1 = readl(data->reg + DIV_SEL1); + data->div_sel2 = readl(data->reg + DIV_SEL2); + data->clk_sel = readl(data->reg + CLK_SEL); + data->clk_dis = readl(data->reg + CLK_DIS); + + return 0; +} + +static int armada_3700_periph_clock_resume(struct device *dev) +{ + struct clk_periph_driver_data *data = dev_get_drvdata(dev); + + /* Follow the same order than what the Cortex-M3 does (ATF code) */ + writel(data->clk_dis, data->reg + CLK_DIS); + writel(data->div_sel0, data->reg + DIV_SEL0); + writel(data->div_sel1, data->reg + DIV_SEL1); + writel(data->div_sel2, data->reg + DIV_SEL2); + writel(data->tbg_sel, data->reg + TBG_SEL); + writel(data->clk_sel, data->reg + CLK_SEL); + + return 0; +} + +static const struct dev_pm_ops armada_3700_periph_clock_pm_ops = { + .suspend = armada_3700_periph_clock_suspend, + .resume = armada_3700_periph_clock_resume, +}; +#endif /* CONFIG_PM */ + static int armada_3700_periph_clock_probe(struct platform_device *pdev) { struct clk_periph_driver_data *driver_data; @@ -716,6 +761,9 @@ static struct platform_driver armada_3700_periph_clock_driver = { .driver = { .name = "marvell-armada-3700-periph-clock", .of_match_table = armada_3700_periph_clock_of_match, +#if defined(CONFIG_PM) + .pm = &armada_3700_periph_clock_pm_ops, +#endif /* CONFIG_PM */ }, };
Add suspend/resume hooks in Armada 37xx peripheral clocks driver to handle S2RAM operations. One can think that these hooks are useless by comparing the register values before and after a suspend/resume cycle: they will look the same anyway. This is because of some scripts executed by the Cortex-M3 core during ATF operations to init both the clocks and the DDR. These values could be modified by the BL33 stage or by Linux itself and should be preserved. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/clk/mvebu/armada-37xx-periph.c | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)