ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Subsystems (2)
PATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Doing suspend/resume on imx6q and imx53 boards with no SATA disk attached will trigger the following warning.
------------[ cut here ]------------ WARNING: CPU: 0 PID: 661 at drivers/ata/libahci.c:224 ahci_enable_ahci+0x74/0x8) Modules linked in: CPU: 0 PID: 661 Comm: sh Tainted: G W 3.15.0-rc5-next-20140521-000027 Backtrace: [<80011c90>] (dump_backtrace) from [<80011e2c>] (show_stack+0x18/0x1c) r6:803a22f4 r5:00000000 r4:00000000 r3:00000000 [<80011e14>] (show_stack) from [<80661e60>] (dump_stack+0x88/0xa4) [<80661dd8>] (dump_stack) from [<80028fdc>] (warn_slowpath_common+0x70/0x94) r5:00000009 r4:00000000 [<80028f6c>] (warn_slowpath_common) from [<80029024>] (warn_slowpath_null+0x24/) r8:808f68c4 r7:00000000 r6:00000000 r5:00000000 r4:e0810004 [<80029000>] (warn_slowpath_null) from [<803a22f4>] (ahci_enable_ahci+0x74/0x80) [<803a2280>] (ahci_enable_ahci) from [<803a2324>] (ahci_reset_controller+0x24/0) r8:ddcd9410 r7:80351178 r6:ddcd9444 r5:dde8b850 r4:e0810000 r3:ddf35e90
[<803a2300>] (ahci_reset_controller) from [<803a2c68>] (ahci_platform_resume_ho)
r7:80351178 r6:ddcd9444 r5:dde8b850 r4:ddcd9410
[<803a2c30>] (ahci_platform_resume_host) from [<803a38f0>] (imx_ahci_resume+0x2)
r5:00000000 r4:ddcd9410
[<803a38c4>] (imx_ahci_resume) from [<803511ac>] (platform_pm_resume+0x34/0x54)
....
The reason is that the SATA controller has no working clock at this point, and thus ahci_enable_ahci() fails to enable the controller. In case that there is no SATA disk attached, the imx_sata_disable() gets called in ahci_imx_error_handler(), and both sata_clk and sata_ref_clk will be disabled there. Because all the imx_sata_enable() calls afterward will return immediately due to imxpriv->no_device check, the SATA controller working clock sata_clk will never get any chance to be enabled again.
This is a regression caused by commit 90870d79d4f2 (ahci-imx: Port to library-ised ahci_platform). Before the commit, only sata_ref_clk is managed by the driver in enable/disable function. But after the commit, all the clocks are enabled/disabled in a row by ahci platform helpers ahci_platform_enable[disable]_clks. Since ahb_clk is a bus clock which does not have gate at all, and i.MX low-power hardware module already manages sata_clk across suspend/resume cycle, the only clock that needs to be managed by software is sata_ref_clk.
So instead of using ahci_platform_enable[disable]_clks to manage all the clocks in a row from imx_sata_enable[disable], we should manage only sata_ref_clk in there.
--- drivers/ata/ahci_imx.c | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-)
drivers/ata/ahci_imx.c +31 -4
@@ -58,6 +58,8 @@ enum ahci_imx_type {struct imx_ahci_priv {struct platform_device *ahci_pdev;enum ahci_imx_type type; +struct clk *sata_clk; +struct clk *sata_ref_clk;struct clk *ahb_clk;struct regmap *gpr;bool no_device;
@@ -224,7 +226,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)return ret; } - ret =ahci_platform_enable_clks (hpriv); + ret =clk_prepare_enable (imxpriv->sata_ref_clk);if (ret <0 ) goto disable_regulator;
@@ -291,7 +293,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv) !IMX6Q_GPR13_SATA_MPLL_CLK_EN); } -ahci_platform_disable_clks (hpriv); +clk_disable_unprepare (imxpriv->sata_ref_clk);if (hpriv->target_pwr)regulator_disable (hpriv->target_pwr);
@@ -385,6 +387,19 @@ static int imx_ahci_probe(struct platform_device *pdev) imxpriv->no_device = false; imxpriv->first_time = true; imxpriv->type = (enum ahci_imx_type )of_id->data; + + imxpriv->sata_clk =devm_clk_get (dev,"sata" ); +if (IS_ERR (imxpriv->sata_clk)) { +dev_err (dev,"can't get sata clock.\n" ); +return PTR_ERR (imxpriv->sata_clk); + } + + imxpriv->sata_ref_clk =devm_clk_get (dev,"sata_ref" ); +if (IS_ERR (imxpriv->sata_ref_clk)) { +dev_err (dev,"can't get sata_ref clock.\n" ); +return PTR_ERR (imxpriv->sata_ref_clk); + } + imxpriv->ahb_clk =devm_clk_get (dev,"ahb" );if (IS_ERR (imxpriv->ahb_clk)) {dev_err (dev,"can't get ahb clock.\n" );
@@ -407,10 +422,14 @@ static int imx_ahci_probe(struct platform_device *pdev) hpriv->plat_data = imxpriv; - ret =imx_sata_enable (hpriv); + ret =clk_prepare_enable (imxpriv->sata_clk);if (ret)return ret; + ret =imx_sata_enable (hpriv); +if (ret) + goto disable_clk; +/* * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL, * and IP vendor specific register IMX_TIMER1MS.
@@ -435,16 +454,24 @@ static int imx_ahci_probe(struct platform_device *pdev) ret =ahci_platform_init_host (pdev, hpriv, &ahci_imx_port_info,0 ,0 ,0 );if (ret) -imx_sata_disable (hpriv); + goto disable_sata; + +return 0 ; +disable_sata: +imx_sata_disable (hpriv); +disable_clk: +clk_disable_unprepare (imxpriv->sata_clk);return ret; }static void ahci_imx_host_stop (struct ata_host *host) {struct ahci_host_priv *hpriv = host->private_data; +struct imx_ahci_priv *imxpriv = hpriv->plat_data;imx_sata_disable (hpriv); +clk_disable_unprepare (imxpriv->sata_clk); }#ifdef CONFIG_PM_SLEEP
Signature · 1 line
1.8.3.2
PATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Hi,
Quoted 43 lines · Shawn Guo
Doing suspend/resume on imx6q and imx53 boards with no SATA disk attached will trigger the following warning.
------------[ cut here ]------------ WARNING: CPU: 0 PID: 661 at drivers/ata/libahci.c:224 ahci_enable_ahci+0x74/0x8) Modules linked in: CPU: 0 PID: 661 Comm: sh Tainted: G W 3.15.0-rc5-next-20140521-000027 Backtrace: [<80011c90>] (dump_backtrace) from [<80011e2c>] (show_stack+0x18/0x1c) r6:803a22f4 r5:00000000 r4:00000000 r3:00000000 [<80011e14>] (show_stack) from [<80661e60>] (dump_stack+0x88/0xa4) [<80661dd8>] (dump_stack) from [<80028fdc>] (warn_slowpath_common+0x70/0x94) r5:00000009 r4:00000000 [<80028f6c>] (warn_slowpath_common) from [<80029024>] (warn_slowpath_null+0x24/) r8:808f68c4 r7:00000000 r6:00000000 r5:00000000 r4:e0810004 [<80029000>] (warn_slowpath_null) from [<803a22f4>] (ahci_enable_ahci+0x74/0x80) [<803a2280>] (ahci_enable_ahci) from [<803a2324>] (ahci_reset_controller+0x24/0) r8:ddcd9410 r7:80351178 r6:ddcd9444 r5:dde8b850 r4:e0810000 r3:ddf35e90[<
803a2300>] (ahci_reset_controller) from [<803a2c68>] (ahci_platform_resume_ho)
r7:80351178 r6:ddcd9444 r5:dde8b850 r4:ddcd9410
[<803a2c30>] (ahci_platform_resume_host) from [<803a38f0>] (imx_ahci_resume+0x2)
r5:00000000 r4:ddcd9410
[<803a38c4>] (imx_ahci_resume) from [<803511ac>] (platform_pm_resume+0x34/0x54)
....The reason is that the SATA controller has no working clock at this point, and thus
ahci_enable_ahci()fails to enable the controller. In case that there is no SATA disk attached, theimx_sata_disable()gets called inahci_imx_error_handler(), and bothsata_clkandsata_ref_clkwill be disabled there. Because all theimx_sata_enable()calls afterward will return immediately due toimxpriv->no_devicecheck, the SATA controller working clocksata_clkwill never get any chance to be enabled again.This is a regression caused by commit
90870d79d4f2(ahci-imx: Port to library-isedahci_platform). Before the commit, onlysata_ref_clkis managed by the driver in enable/disable function. But after the commit, all the clocks are enabled/disabled in a row by ahci platform helpers ahci_platform_enable[disable]_clks. Sinceahb_clkis a bus clock which does not have gate at all, and i.MX low-power hardware module already managessata_clkacross suspend/resume cycle, the only clock that needs to be managed by software issata_ref_clk.
Quoted 3 lines
So instead of using ahci_platform_enable[disable]_clks to manage all the clocks in a row from imx_sata_enable[disable], we should manage only
sata_ref_clkin there.
I disagree, the problem here is not the clk disable / enable, with your patch the sata_clk never gets disabled, unless the driver gets unloaded which clearly is the wrong thing to do.
The real problem is the weird error handling which is unique to the ahci_imx code where if no disk is present it semi-permanently shuts down (breaking later hotplug of sata without a reboot).
I assume that this weird error handling is done to save power in the case no disk is attached. But if that is the case the surely disabling the sata_clk in this scenario too is the right thing to do.
As for the "ahb_clk is a bus clock which does not have gate at all" argument, if that were the case then the disabling of the sata_clk would not cause any issues at all, so I'm not buying that argument.
The proper fix is obvious, if we've shutdown everything due to there not being a device present at the initial probe, don't call ahci_platform_resume_host on resume, as is done in the attached patch.
Regards,
Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ahci_imx-Fix-oops-on-suspend-resume.patch
Type: text/x-patch
Size: 2809 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140528/36aaedd0/attachment-0001.bin>
Fabio Estevam
reviewerPATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Hans,
Quoted 3 lines · Hans de Goede
The proper fix is obvious, if we've shutdown everything due to there not being a device present at the initial probe, don't call
ahci_platform_resume_hoston resume, as is done in the attached patch.
I tested your attached patch. It does not build as it lacks to add the definition of imxpriv inside the suspend/resume functions. After fixing it I get:
root at freescale /$ echo mem > /sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.002 seconds) done. Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
PM: suspend of devices complete after 91.109 msecs
PM: suspend devices took 0.100 seconds
PM: late suspend of devices complete after 9.086 msecs
PM: noirq suspend of devices complete after 8.780 msecs Disabling non-boot CPUs ...
CPU1: shutdown
CPU2: shutdown
CPU3: shutdown
Enabling non-boot CPUs ...
CPU1: Booted secondary processor
CPU1 is up
CPU2: Booted secondary processor
CPU2 is up
CPU3: Booted secondary processor
CPU3 is up
PM: noirq resume of devices complete after 6.914 msecs
PM: early resume of devices complete after 5.160 msecs
PM: resume of devices complete after 167.025 msecs
PM: resume devices took 0.180 seconds
Restarting tasks ... done.
ata1: failed to resume link (SControl 2003)
random: nonblocking pool is initialized
root at freescale /$ ata1: softreset failed (1st FIS failed)
ata1: failed to resume link (SControl 2003)
ata1: softreset failed (1st FIS failed)
ata1: failed to resume link (SControl 2003)
ata1: softreset failed (1st FIS failed)
ata1: limiting SATA link speed to 1.5 Gbps
ata1: failed to resume link (SControl 2003)
ata1: softreset failed (1st FIS failed)
ata1: reset failed, giving up
With Shawn's version I do not get the suspend error messages:
...
CPU1: Booted secondary processor
CPU1 is up
CPU2: Booted secondary processor
CPU2 is up
CPU3: Booted secondary processor
CPU3 is up
PM: noirq resume of devices complete after 5.836 msecs
PM: early resume of devices complete after 5.334 msecs
PM: resume of devices complete after 176.763 msecs
PM: resume devices took 0.190 seconds
Restarting tasks ... done.
ata1: SATA link down (SStatus 0 SControl 300)
random: nonblocking pool is initialized
PATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Hi,
Quoted 48 lines · Fabio Estevam
Hans,
Quoted 3 lines · Hans de Goede
The proper fix is obvious, if we've shutdown everything due to there not being a device present at the initial probe, don't call
ahci_platform_resume_hoston resume, as is done in the attached patch.I tested your attached patch. It does not build as it lacks to add the definition of imxpriv inside the suspend/resume functions. After fixing it I get:
root at freescale /$ echo mem >
/sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.002 seconds) done. Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
PM: suspend of devices complete after 91.109 msecs
PM: suspend devices took 0.100 seconds
PM: late suspend of devices complete after 9.086 msecs
PM: noirq suspend of devices complete after 8.780 msecs Disabling non-boot CPUs ...
CPU1: shutdown
CPU2: shutdown
CPU3: shutdown
Enabling non-boot CPUs ...
CPU1: Booted secondary processor
CPU1 is up
CPU2: Booted secondary processor
CPU2 is up
CPU3: Booted secondary processor
CPU3 is up
PM: noirq resume of devices complete after 6.914 msecs
PM: early resume of devices complete after 5.160 msecs
PM: resume of devices complete after 167.025 msecs
PM: resume devices took 0.180 seconds
Restarting tasks ... done.
ata1: failed to resume link (SControl 2003)
random: nonblocking pool is initialized
root at freescale /$ ata1: softreset failed (1st FIS failed)
ata1: failed to resume link (SControl 2003)
ata1: softreset failed (1st FIS failed)
ata1: failed to resume link (SControl 2003)
ata1: softreset failed (1st FIS failed)
ata1: limiting SATA link speed to 1.5 Gbps
ata1: failed to resume link (SControl 2003)
ata1: softreset failed (1st FIS failed)
ata1: reset failed, giving up
Quoted 16 lines
With Shawn's version I do not get the suspend error messages:
...
CPU1: Booted secondary processor
CPU1 is up
CPU2: Booted secondary processor
CPU2 is up
CPU3: Booted secondary processor
CPU3 is up
PM: noirq resume of devices complete after 5.836 msecs
PM: early resume of devices complete after 5.334 msecs
PM: resume of devices complete after 176.763 msecs
PM: resume devices took 0.190 seconds
Restarting tasks ... done.
ata1: SATA link down (SStatus 0 SControl 300)
random: nonblocking pool is initialized
Ah yes I see, the ahci_imx suspend/resume handler now no longer touches the controller, but the ata core is still trying to use it.
Ok, in that case I agree that your original patch is the best solution.
Your original patch is:
Regards,
Hans
Fabio Estevam
reviewerPATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Quoted 9 lines · Hans de Goede
Ah yes I see, the
ahci_imxsuspend/resume handler now no longer touches the controller, but the ata core is still trying to use it.Ok, in that case I agree that your original patch is the best solution.
Your original patch is:
Acked-by Hans de Goede <[email protected]>
I think you meant Shawn's patch, not mine :-)
PATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
Hi,
Quoted 5 lines · Fabio Estevam
Quoted 3 lines · Hans de Goede
Ah yes I see, the
ahci_imxsuspend/resume handler now no longer touches the controller, but the ata core is still trying to use it.
Quoted 8 lines
Quoted 6 lines
Ok, in that case I agree that your original patch is the best solution.
Your original patch is:
Acked-by Hans de Goede <[email protected]>I think you meant Shawn's patch, not mine :-)
Right, so to be extra clear, the patch titled:
"ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]"
is:
Regards,
Hans
Fabio Estevam
reviewerPATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
In reply to Shawn GuoQuoted 43 lines · Shawn Guo
Doing suspend/resume on imx6q and imx53 boards with no SATA disk attached will trigger the following warning.
------------[ cut here ]------------ WARNING: CPU: 0 PID: 661 at drivers/ata/libahci.c:224 ahci_enable_ahci+0x74/0x8) Modules linked in: CPU: 0 PID: 661 Comm: sh Tainted: G W 3.15.0-rc5-next-20140521-000027 Backtrace: [<80011c90>] (dump_backtrace) from [<80011e2c>] (show_stack+0x18/0x1c) r6:803a22f4 r5:00000000 r4:00000000 r3:00000000 [<80011e14>] (show_stack) from [<80661e60>] (dump_stack+0x88/0xa4) [<80661dd8>] (dump_stack) from [<80028fdc>] (warn_slowpath_common+0x70/0x94) r5:00000009 r4:00000000 [<80028f6c>] (warn_slowpath_common) from [<80029024>] (warn_slowpath_null+0x24/) r8:808f68c4 r7:00000000 r6:00000000 r5:00000000 r4:e0810004 [<80029000>] (warn_slowpath_null) from [<803a22f4>] (ahci_enable_ahci+0x74/0x80) [<803a2280>] (ahci_enable_ahci) from [<803a2324>] (ahci_reset_controller+0x24/0) r8:ddcd9410 r7:80351178 r6:ddcd9444 r5:dde8b850 r4:e0810000 r3:ddf35e90[<
803a2300>] (ahci_reset_controller) from [<803a2c68>] (ahci_platform_resume_ho)
r7:80351178 r6:ddcd9444 r5:dde8b850 r4:ddcd9410
[<803a2c30>] (ahci_platform_resume_host) from [<803a38f0>] (imx_ahci_resume+0x2)
r5:00000000 r4:ddcd9410
[<803a38c4>] (imx_ahci_resume) from [<803511ac>] (platform_pm_resume+0x34/0x54)
....The reason is that the SATA controller has no working clock at this point, and thus
ahci_enable_ahci()fails to enable the controller. In case that there is no SATA disk attached, theimx_sata_disable()gets called inahci_imx_error_handler(), and bothsata_clkandsata_ref_clkwill be disabled there. Because all theimx_sata_enable()calls afterward will return immediately due toimxpriv->no_devicecheck, the SATA controller working clocksata_clkwill never get any chance to be enabled again.This is a regression caused by commit
90870d79d4f2(ahci-imx: Port to library-isedahci_platform). Before the commit, onlysata_ref_clkis managed by the driver in enable/disable function. But after the commit, all the clocks are enabled/disabled in a row by ahci platform helpers ahci_platform_enable[disable]_clks. Sinceahb_clkis a bus clock which does not have gate at all, and i.MX low-power hardware module already managessata_clkacross suspend/resume cycle, the only clock that needs to be managed by software issata_ref_clk.
Quoted 7 lines
So instead of using ahci_platform_enable[disable]_clks to manage all the clocks in a row from imx_sata_enable[disable], we should manage only
sata_ref_clkin there.Reported-by Fabio Estevam <[email protected]>Fixes 90870d79d4f2 (ahci-imx: Port to library-ised ahci_platform)Signed-off-by Shawn Guo <[email protected]>
Tejun Heo
AppliedPATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]
In reply to Shawn GuoQuoted 43 lines · Shawn Guo
Doing suspend/resume on imx6q and imx53 boards with no SATA disk attached will trigger the following warning.
------------[ cut here ]------------ WARNING: CPU: 0 PID: 661 at drivers/ata/libahci.c:224 ahci_enable_ahci+0x74/0x8) Modules linked in: CPU: 0 PID: 661 Comm: sh Tainted: G W 3.15.0-rc5-next-20140521-000027 Backtrace: [<80011c90>] (dump_backtrace) from [<80011e2c>] (show_stack+0x18/0x1c) r6:803a22f4 r5:00000000 r4:00000000 r3:00000000 [<80011e14>] (show_stack) from [<80661e60>] (dump_stack+0x88/0xa4) [<80661dd8>] (dump_stack) from [<80028fdc>] (warn_slowpath_common+0x70/0x94) r5:00000009 r4:00000000 [<80028f6c>] (warn_slowpath_common) from [<80029024>] (warn_slowpath_null+0x24/) r8:808f68c4 r7:00000000 r6:00000000 r5:00000000 r4:e0810004 [<80029000>] (warn_slowpath_null) from [<803a22f4>] (ahci_enable_ahci+0x74/0x80) [<803a2280>] (ahci_enable_ahci) from [<803a2324>] (ahci_reset_controller+0x24/0) r8:ddcd9410 r7:80351178 r6:ddcd9444 r5:dde8b850 r4:e0810000 r3:ddf35e90[<
803a2300>] (ahci_reset_controller) from [<803a2c68>] (ahci_platform_resume_ho)
r7:80351178 r6:ddcd9444 r5:dde8b850 r4:ddcd9410
[<803a2c30>] (ahci_platform_resume_host) from [<803a38f0>] (imx_ahci_resume+0x2)
r5:00000000 r4:ddcd9410
[<803a38c4>] (imx_ahci_resume) from [<803511ac>] (platform_pm_resume+0x34/0x54)
....The reason is that the SATA controller has no working clock at this point, and thus
ahci_enable_ahci()fails to enable the controller. In case that there is no SATA disk attached, theimx_sata_disable()gets called inahci_imx_error_handler(), and bothsata_clkandsata_ref_clkwill be disabled there. Because all theimx_sata_enable()calls afterward will return immediately due toimxpriv->no_devicecheck, the SATA controller working clocksata_clkwill never get any chance to be enabled again.This is a regression caused by commit
90870d79d4f2(ahci-imx: Port to library-isedahci_platform). Before the commit, onlysata_ref_clkis managed by the driver in enable/disable function. But after the commit, all the clocks are enabled/disabled in a row by ahci platform helpers ahci_platform_enable[disable]_clks. Sinceahb_clkis a bus clock which does not have gate at all, and i.MX low-power hardware module already managessata_clkacross suspend/resume cycle, the only clock that needs to be managed by software issata_ref_clk.
Quoted 7 lines
So instead of using ahci_platform_enable[disable]_clks to manage all the clocks in a row from imx_sata_enable[disable], we should manage only
sata_ref_clkin there.Reported-by Fabio Estevam <[email protected]>Fixes 90870d79d4f2 (ahci-imx: Port to library-ised ahci_platform)Signed-off-by Shawn Guo <[email protected]>
Applied to libata/for-3.16-fixes.
Thanks.
Signature · 1 line
tejun