Skip to contentSkip to conversation

ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]

PATCH New Shawn Guo 4 participants 8 messages May 28, 2014 – Jun 19, 2014

Subsystems (2)
lore
Share & tools
lore

Shawn Guo

author
#0 lore raw

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.

Fixes 90870d79d4f2 (ahci-imx: Port to library-ised ahci_platform)
---
 drivers/ata/ahci_imx.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)
drivers/ata/ahci_imx.c +31 -4
index 3a901520c62b..4384a7d72133 100644
@@ -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
#1 lore raw

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, 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.

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_clk in 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>

reviewer
#2 lore raw

PATCH 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_host on 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

#3 lore raw

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_host on 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

reviewer
#4 lore raw

PATCH 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_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:

I think you meant Shawn's patch, not mine :-)

#5 lore raw

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_imx suspend/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:

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

reviewer
#6 lore raw

PATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]

In reply to Shawn Guo
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, 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.

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_clk in there.

Fixes 90870d79d4f2 (ahci-imx: Port to library-ised ahci_platform)
Applied
#7 lore raw

PATCH 1/1 ahci: imx: manage only sata_ref_clk in imx_sata_enable[disable]

In reply to Shawn Guo
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, 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.

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_clk in there.

Fixes 90870d79d4f2 (ahci-imx: Port to library-ised ahci_platform)

Applied to libata/for-3.16-fixes.

Thanks.

Signature · 1 line
tejun