| Message ID | 1390088935-7193-4-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Jan 19, 2014 at 12:48:45AM +0100, Hans de Goede wrote: > For devices where ahci_platform_data provides suspend() there is an unbalance > in clk enable/disable calls. The suspend path does not disable the clk, but > the resume path enables it. This commit fixes it by always disabling the clk > in the suspend path independent of there being an ahci_platform_data suspend > method. > > On all drivers currently using a suspend method, the suspend method always > succeeds, so change its return type to void, to avoid having the introduce > somewhat complex error handling paths. > > The disabling of the clock on suspend is a functional change, to ensure this > is ok I've audited all drivers using ahci_platform_data: > > arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method > arch/arm/mach-spear/spear1340.c: Does not use the clock framework > drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these > disable / enable another clock, so likely it is ok to disable / enable the > clock at of-node index 0 as well, I've ordered a wandboard to be able to > test these changes. This isn't your fault but similarly to the previous patch, I'd much prefer if drivers which need custom ops just override the whole operation and are allowed to use the default platform from their custom implementations as they see fit. Allowing partial overrides seems like an efficient thing to do at the beginning but if you continue to stack them, you eventually end up with giant pile of methods where figuring out which code paths are actually executed takes quite a bit of effort. I'd really like to avoid that. Thanks.
Hi, On 01/19/2014 12:14 PM, Tejun Heo wrote: > On Sun, Jan 19, 2014 at 12:48:45AM +0100, Hans de Goede wrote: >> For devices where ahci_platform_data provides suspend() there is an unbalance >> in clk enable/disable calls. The suspend path does not disable the clk, but >> the resume path enables it. This commit fixes it by always disabling the clk >> in the suspend path independent of there being an ahci_platform_data suspend >> method. >> >> On all drivers currently using a suspend method, the suspend method always >> succeeds, so change its return type to void, to avoid having the introduce >> somewhat complex error handling paths. >> >> The disabling of the clock on suspend is a functional change, to ensure this >> is ok I've audited all drivers using ahci_platform_data: >> >> arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method >> arch/arm/mach-spear/spear1340.c: Does not use the clock framework >> drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these >> disable / enable another clock, so likely it is ok to disable / enable the >> clock at of-node index 0 as well, I've ordered a wandboard to be able to >> test these changes. > > This isn't your fault but similarly to the previous patch, I'd much > prefer if drivers which need custom ops just override the whole > operation and are allowed to use the default platform from their > custom implementations as they see fit. Allowing partial overrides > seems like an efficient thing to do at the beginning but if you > continue to stack them, you eventually end up with giant pile of > methods where figuring out which code paths are actually executed > takes quite a bit of effort. I'd really like to avoid that. I disagree, if we were to follow this reasoning then the init and exit overrides would have to logically also be all or nothing propositions, currently ahci_probe and ahci_stop do all the clks, regulator and sata-core setup / teardown needed with the init / exit pdata callbacks doing any implementation specific register frobbing needed. As I see it either doing clks, regulator and sata-core things in a common place makes sense, and then it goes for suspend and resume too, or we opt for always following the complete override model, and which point it becomes more sensible to just do a separate platform driver per ahci implementation. After this patch, suspend / resume exactly follow init / stop in that the ahci_platform.c bits take care of the common stuff, while calling into a platform_data callback for implementation specific setup. Also if you look at both the imx and sunxi implementations doing things this way works well in practice. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote: > As I see it either doing clks, regulator and sata-core things in a common > place makes sense, and then it goes for suspend and resume too, or we > opt for always following the complete override model, and which point > it becomes more sensible to just do a separate platform driver per > ahci implementation. It makes sense in light of those specific cases, but there are gonna be cases where the placement of the callback is slightly wrong and we end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on. Please make the whole op overridable and then export the default op and use it as library. Thanks.
Hi, On 01/19/2014 08:15 PM, Tejun Heo wrote: > Hey, > > On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote: >> As I see it either doing clks, regulator and sata-core things in a common >> place makes sense, and then it goes for suspend and resume too, or we >> opt for always following the complete override model, and which point >> it becomes more sensible to just do a separate platform driver per >> ahci implementation. > > It makes sense in light of those specific cases, but there are gonna > be cases where the placement of the callback is slightly wrong and we > end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on. > Please make the whole op overridable and then export the default op > and use it as library. If we were to put a generic implementation in ahci_platform.c and export it for use from overrides to avoid copy and pasting common bits everywhere, then we still have the ordering problem you are talking about. How do you envision all of this fitting together, I can imagine ie a ahci_platform_resume_controller which has the bits of what is currently ahci_resume stating at: "if (dev->power.power_state.event == PM_EVENT_SUSPEND) {" And having ahci_resume in ahci_platform.c still doing the clk and power enabling before calling into ahci_platform_resume, and drivers overriding the resume method need to do their own clk + regulator + whatever setup before calling into ahci_platform_resume_controller ? Also how do you see overriding the entire op, does that mean that pdata->suspend will be deprecated (we will need to keep it around for now to avoid breaking existing drivers using it), and all of: ahci_probe ahci_suspend ahci_resume Will get exported from ahci_platform.c and drivers needing to override any of them will provide their own platform_driver struct, pointing either to their overrides, or for driver methods they don't need to override to the exported function from ahci_platform.c ? This would also work nicely for the of_device_id data stuff, since if drivers have their own platform_driver struct these bits could just go inside the driver file instead of being in #ifdef CONFIG_FOO in ahci_platform.c Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, On Sun, Jan 19, 2014 at 08:52:21PM +0100, Hans de Goede wrote: > And having ahci_resume in ahci_platform.c still doing the clk and power enabling > before calling into ahci_platform_resume, and drivers overriding the resume method > need to do their own clk + regulator + whatever setup > before calling into ahci_platform_resume_controller ? If the use cases are significant enough, split the base function to two parts? The thing which gets really messy down the road is when common code and callbacks intertwine arbitrarily. A driver wants an early exit after overriding a subpart, so if that callback exists, it's an early exit. The next driver wants to override something at slightly different point but still want to proceed with the rest of the common code, so it adds a new callback which doesn't do early exit, and so on. Pretty soon, when you're looking at a driver, it becomes really difficult what it actually does. We *HAD* this problem over and over again with ide and it was a nightmare. Providing larger, logical overriding points while providing common lib routines makes understanding what a given driver does a lot easier. Also, it forces people to think about how the overall API looks and whether a split of an existing library which require giving the splits sensible names and semantics is justifiable or if the case at hand is an one-off thing which can be better served by just open coding it. > Will get exported from ahci_platform.c and drivers needing to override any of them > will provide their own platform_driver struct, pointing either to their overrides, > or for driver methods they don't need to override to the exported function from > ahci_platform.c ? Yeah, makes sense to me. Thanks.
diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c index 3fb6834..c4cbbd2 100644 --- a/arch/arm/mach-spear/spear1340.c +++ b/arch/arm/mach-spear/spear1340.c @@ -107,14 +107,12 @@ void sata_miphy_exit(struct device *dev) msleep(20); } -int sata_suspend(struct device *dev) +void sata_suspend(struct device *dev) { if (dev->power.power_state.event == PM_EVENT_FREEZE) - return 0; + return; sata_miphy_exit(dev); - - return 0; } int sata_resume(struct device *dev) diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 3e23e99..30568d3 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -171,7 +171,7 @@ static void imx6q_sata_exit(struct device *dev) clk_disable_unprepare(imxpriv->sata_ref_clk); } -static int imx_ahci_suspend(struct device *dev) +static void imx_ahci_suspend(struct device *dev) { struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent); @@ -185,8 +185,6 @@ static int imx_ahci_suspend(struct device *dev) !IMX6Q_GPR13_SATA_MPLL_CLK_EN); clk_disable_unprepare(imxpriv->sata_ref_clk); } - - return 0; } static int imx_ahci_resume(struct device *dev) diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c index 4b231ba..dc1ef73 100644 --- a/drivers/ata/ahci_platform.c +++ b/drivers/ata/ahci_platform.c @@ -275,7 +275,7 @@ static int ahci_suspend(struct device *dev) return rc; if (pdata && pdata->suspend) - return pdata->suspend(dev); + pdata->suspend(dev); if (!IS_ERR(hpriv->clk)) clk_disable_unprepare(hpriv->clk); diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h index 73a2500..a641cb6 100644 --- a/include/linux/ahci_platform.h +++ b/include/linux/ahci_platform.h @@ -23,7 +23,7 @@ struct ata_port_info; struct ahci_platform_data { int (*init)(struct device *dev, void __iomem *addr); void (*exit)(struct device *dev); - int (*suspend)(struct device *dev); + void (*suspend)(struct device *dev); int (*resume)(struct device *dev); const struct ata_port_info *ata_port_info; unsigned int force_port_map;
For devices where ahci_platform_data provides suspend() there is an unbalance in clk enable/disable calls. The suspend path does not disable the clk, but the resume path enables it. This commit fixes it by always disabling the clk in the suspend path independent of there being an ahci_platform_data suspend method. On all drivers currently using a suspend method, the suspend method always succeeds, so change its return type to void, to avoid having the introduce somewhat complex error handling paths. The disabling of the clock on suspend is a functional change, to ensure this is ok I've audited all drivers using ahci_platform_data: arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method arch/arm/mach-spear/spear1340.c: Does not use the clock framework drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these disable / enable another clock, so likely it is ok to disable / enable the clock at of-node index 0 as well, I've ordered a wandboard to be able to test these changes. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/arm/mach-spear/spear1340.c | 6 ++---- drivers/ata/ahci_imx.c | 4 +--- drivers/ata/ahci_platform.c | 2 +- include/linux/ahci_platform.h | 2 +- 4 files changed, 5 insertions(+), 9 deletions(-)