Skip to contentSkip to conversation

Bring suspend to RAM support to MVEBU SATA

PATCH In review Miquel Raynal 4 participants 14 messages Nov 23, 2018 – Nov 30, 2018

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover Bring suspend to RAM support to MVEBU SATA

Hello,

As part of an effort to bring suspend to RAM support to Armada 3700 SoCs (main target: ESPRESSObin), this series handles the work around the SATA IP.

First, a change in the libahci platform adds support for the new PHY framework by following the phy_set_mode()/phy_power_on() sequence. Then, the AHCI MVEBU driver is a bit updated (patch 2 & 3) and a missing initialization is added for the A3700 in patch 4 (only done by the Bootloader before). Missing clock support is implemented in patch 5 to be sure the clock will be resumed before this driver (see [1] for the series adding device links to the clock core).

Finally, device trees are updated to reflect the hardware: the missing PHY is added to the ESPRESSObin DT, and the clock is added to the SoC DT (patch 6 & 7). Bindings already document the clock and the PHY so no update is needed on this regard.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html

Thanks,
Miqu?l

Miquel Raynal (7):
ata: libahci_platform: comply to PHY framework
ata: ahci: mvebu: remove stale comment
ata: ahci: mvebu: do Armada 38x configuration only on relevant SoCs
ata: ahci: mvebu: add Armada 3700 initialization needed for S2RAM
ata: ahci: mvebu: add clock support
ARM64: dts: marvell: armada-37xx: declare SATA clock
ARM64: dts: marvell: armada-3720-espressobin: declare SATA PHY
property

 .../dts/marvell/armada-3720-espressobin.dts   |  2 +
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |  1 +
 drivers/ata/ahci_mvebu.c                      | 93 ++++++++++++++-----
 drivers/ata/libahci_platform.c                | 11 +++
 4 files changed, 84 insertions(+), 23 deletions(-)
Signature · 1 line
2.19.1
#1 lore raw

PATCH 1/7 ata: libahci_platform: comply to PHY framework

Current implementation of the libahci does not take into account the new PHY framework. Correct the situation by adding a call to phy_set_mode() before phy_power_on() and by adding calls to ahci_platform_enable/disable_phys() at suspend/resume_host() time.

---
 drivers/ata/libahci_platform.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/ata/libahci_platform.c +11 -0
index 4b900fc659f7..9f33f72b674b 100644
@@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 		if (rc)
 			goto disable_phys;
 
+		rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA);
+		if (rc) {
+			phy_exit(hpriv->phys[i]);
+			goto disable_phys;
+		}
+
 		rc = phy_power_on(hpriv->phys[i]);
 		if (rc) {
 			phy_exit(hpriv->phys[i]);
@@ -738,6 +744,8 @@ int ahci_platform_suspend_host(struct device *dev)
 	writel(ctl, mmio + HOST_CTL);
 	readl(mmio + HOST_CTL); /* flush */
 
+	ahci_platform_disable_phys(hpriv);
+
 	return ata_host_suspend(host, PMSG_SUSPEND);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
@@ -756,6 +764,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
 int ahci_platform_resume_host(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
 	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
@@ -766,6 +775,8 @@ int ahci_platform_resume_host(struct device *dev)
 		ahci_init_controller(host);
 	}
 
+	ahci_platform_enable_phys(hpriv);
+
 	ata_host_resume(host);
 
 	return 0;
Signature · 1 line
2.19.1
#2 lore raw

PATCH 1/7 ata: libahci_platform: comply to PHY framework

Hi Miquel,

Quoted 15 lines · Miquel Raynal

Current implementation of the libahci does not take into account the new PHY framework. Correct the situation by adding a call to phy_set_mode() before phy_power_on() and by adding calls to ahci_platform_enable/disable_phys() at suspend/resume_host() time.

---
  drivers/ata/libahci_platform.c | 11 +++++++++++
  1 file changed, 11 insertions(+)
drivers/ata/libahci_platform.c +0 -0
index 4b900fc659f7..9f33f72b674b 100644

drivers/ata/libahci_platform.c static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) · line 64

Quoted 9 lines
drivers/ata/libahci_platform.c +6 -0
@@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
  		if (rc)
  			goto disable_phys;
 
+		rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA);
+		if (rc) {
+			phy_exit(hpriv->phys[i]);
+			goto disable_phys;
+		}
+

I see that phy_set_mode returns 0 for drivers which do not implement it, so this should be fine.

drivers/ata/libahci_platform.c int ahci_platform_suspend_host(struct device *dev) · line 751

Quoted 12 lines
drivers/ata/libahci_platform.c +2 -0
  		rc = phy_power_on(hpriv->phys[i]);
  		if (rc) {
  			phy_exit(hpriv->phys[i]);
@@ -738,6 +744,8 @@ int ahci_platform_suspend_host(struct device *dev)
  	writel(ctl, mmio + HOST_CTL);
  	readl(mmio + HOST_CTL); /* flush */
 
+	ahci_platform_disable_phys(hpriv);
+
  	return ata_host_suspend(host, PMSG_SUSPEND);
  }
  EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);

I'm afraid that this and the matching change in ahci_platform_suspend_host needs to be guarded by a flag, there are quite a few sata drivers using the libahci_platform functions as well as quite a few sata drivers combining this with using phy drivers.

I'm worried that doing this unconditionally on drivers which have not been tested with this change my break things.

I think it might be cleanest to extend the existing flags passed to ahci_platform_get_resources with a flag for this and storing them somewhere in ahci_host_priv so that the suspend/resume functions can get to them.

Regards,

Hans

Quoted 18 lines
drivers/ata/libahci_platform.c +3 -0
@@ -756,6 +764,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
  int ahci_platform_resume_host(struct device *dev)
  {
  	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
  	int rc;
 
  	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
@@ -766,6 +775,8 @@ int ahci_platform_resume_host(struct device *dev)
  		ahci_init_controller(host);
  	}
 
+	ahci_platform_enable_phys(hpriv);
+
  	ata_host_resume(host);
 
  	return 0;
#3 lore raw

Re: [PATCH 1/7] ata: libahci_platform: comply to PHY framework

Hi Hans,

Thanks for the review.

Hans de Goede <[email protected]> wrote on Fri, 23 Nov 2018 11:33:15 +0100:

Quoted 48 lines

Hi Miquel,

Quoted 15 lines · Miquel Raynal

Current implementation of the libahci does not take into account the new PHY framework. Correct the situation by adding a call to phy_set_mode() before phy_power_on() and by adding calls to ahci_platform_enable/disable_phys() at suspend/resume_host() time.

---
  drivers/ata/libahci_platform.c | 11 +++++++++++
  1 file changed, 11 insertions(+)
drivers/ata/libahci_platform.c +0 -0
index 4b900fc659f7..9f33f72b674b 100644

drivers/ata/libahci_platform.c static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) · line 63

Quoted 8 lines
drivers/ata/libahci_platform.c +5 -0
@@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
  		if (rc)
  			goto disable_phys;  
  > +		rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA);  
+		if (rc) {
+			phy_exit(hpriv->phys[i]);
+			goto disable_phys;
+		}
+  

I see that phy_set_mode returns 0 for drivers which do not implement it, so this should be fine.

drivers/ata/libahci_platform.c int ahci_platform_suspend_host(struct device *dev) · line 750

Quoted 11 lines
drivers/ata/libahci_platform.c +1 -0
  		rc = phy_power_on(hpriv->phys[i]);
  		if (rc) {
  			phy_exit(hpriv->phys[i]);
@@ -738,6 +744,8 @@ int ahci_platform_suspend_host(struct device *dev)
  	writel(ctl, mmio + HOST_CTL);
  	readl(mmio + HOST_CTL); /* flush */  
  > +	ahci_platform_disable_phys(hpriv);  
+
  	return ata_host_suspend(host, PMSG_SUSPEND);
  }
  EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);  

I'm afraid that this and the matching change in ahci_platform_suspend_host needs to be guarded by a flag, there are quite a few sata drivers using the libahci_platform functions as well as quite a few sata drivers combining this with using phy drivers.

Quoted 7 lines

I'm worried that doing this unconditionally on drivers which have not been tested with this change my break things.

I think it might be cleanest to extend the existing flags passed to ahci_platform_get_resources with a flag for this and storing them somewhere in ahci_host_priv so that the suspend/resume functions can get to them.

I understand your concern, please have a look at the v2 which addresses this the way you suggested.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

maintainer
#4 lore raw

PATCH 1/7 ata: libahci_platform: comply to PHY framework

In reply to Miquel Raynal
Quoted 15 lines · Miquel Raynal

Current implementation of the libahci does not take into account the new PHY framework. Correct the situation by adding a call to phy_set_mode() before phy_power_on() and by adding calls to ahci_platform_enable/disable_phys() at suspend/resume_host() time.

---
 drivers/ata/libahci_platform.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/ata/libahci_platform.c +0 -0
index 4b900fc659f7..9f33f72b674b 100644

drivers/ata/libahci_platform.c static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) · line 63

Quoted 8 lines
drivers/ata/libahci_platform.c +5 -0
@@ -56,6 +56,12 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 		if (rc)
 			goto disable_phys;
 
+		rc = phy_set_mode(hpriv->phys[i], PHY_MODE_SATA);
+		if (rc) {
+			phy_exit(hpriv->phys[i]);
+			goto disable_phys;
+		}

Hi Miquel

Russell King wrote a comphy driver for the Armada 3XX family. It only supports network PHYs. Did you check it does the right thing when passed PHY_MODE_SATA? This is slightly different to Hans's comment, in that i expect phy_set_mode() is implemented for the comphy driver, but it might not understand PHY_MODE_SATA and return an error?

Maybe you should look at rc and keep going if EOPNOTSUPP is returned?

Andrew

#5 lore raw

PATCH 1/7 ata: libahci_platform: comply to PHY framework

Quoted 5 lines · Andrew Lunn

Russell King wrote a comphy driver for the Armada 3XX family. It only supports network PHYs. Did you check it does the right thing when passed PHY_MODE_SATA? This is slightly different to Hans's comment, in that i expect phy_set_mode() is implemented for the comphy driver, but it might not understand PHY_MODE_SATA and return an error?

It makes no difference until the comphy is wired up to the SATA device in the appropriate DT files. Until then, there's no way that the comphy will ever see PHY_MODE_SATA.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
maintainer
#6 lore raw

PATCH 1/7 ata: libahci_platform: comply to PHY framework

Quoted 10 lines · Russell King - ARM Linux
Quoted 5 lines · Andrew Lunn

Russell King wrote a comphy driver for the Armada 3XX family. It only supports network PHYs. Did you check it does the right thing when passed PHY_MODE_SATA? This is slightly different to Hans's comment, in that i expect phy_set_mode() is implemented for the comphy driver, but it might not understand PHY_MODE_SATA and return an error?

It makes no difference until the comphy is wired up to the SATA device in the appropriate DT files. Until then, there's no way that the comphy will ever see PHY_MODE_SATA.

O.K, great.

Thanks
Andrew

#7 lore raw

PATCH 2/7 ata: ahci: mvebu: remove stale comment

In reply to Miquel Raynal

For Armada-38x (32-bit) SoCs, PM platform support has been added since:
commit 32f9494c9dfd ("ARM: mvebu: prepare pm-board.c for the
introduction of Armada 38x support")
commit 3cbd6a6ca81c ("ARM: mvebu: Add standby support")

For Armada 64-bit SoCs, like the A3700 also using this AHCI driver, PM platform support has always existed.

There are even suspend/resume hooks in this driver since:
commit d6ecf15814888 ("ata: ahci_mvebu: add suspend/resume support")

Remove the stale comment at the end of this driver stating that all the above does not exist yet.

Fixes d6ecf15814888 ("ata: ahci_mvebu: add suspend/resume support")
---
 drivers/ata/ahci_mvebu.c | 5 -----
 1 file changed, 5 deletions(-)
drivers/ata/ahci_mvebu.c +0 -5
index f9cb51be38eb..128d6f22926d 100644
@@ -197,11 +197,6 @@ static const struct of_device_id ahci_mvebu_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ahci_mvebu_of_match);
 
-/*
- * We currently don't provide power management related operations,
- * since there is no suspend/resume support at the platform level for
- * Armada 38x for the moment.
- */
 static struct platform_driver ahci_mvebu_driver = {
 	.probe = ahci_mvebu_probe,
 	.remove = ata_platform_remove_one,
Signature · 1 line
2.19.1
#8 lore raw

PATCH 3/7 ata: ahci: mvebu: do Armada 38x configuration only on relevant SoCs

In reply to Miquel Raynal

At the beginning, only Armada 38x SoCs where supported by the ahci_mvebu.c driver. Commit 15d3ce7b63bd ("ata: ahci_mvebu: add support for Armada 3700 variant") introduced Armada 3700 support. As opposed to Armada 38x SoCs, the 3700 variants do not have to configure mbus and the regret option. This patch took care of avoiding such configuration when not needed in the probe function, but failed to do the same in the resume path. While doing so looks harmless by experience, let's clean the driver logic and avoid doing this useless configuration with Armada 3700 SoCs.

Because the logic is very similar between these two places, it has been decided to factorize this code and put it in a "Armada 38x configuration function". This function is part of a new (per-compatible) platform data structure, so that the addition of such configuration function for Armada 3700 will be eased.

Fixes 15d3ce7b63bd ("ata: ahci_mvebu: add support for Armada 3700 variant")
---
 drivers/ata/ahci_mvebu.c | 66 +++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 17 deletions(-)
drivers/ata/ahci_mvebu.c +49 -17
index 128d6f22926d..a1fce848bba9 100644
@@ -28,6 +28,10 @@
 #define AHCI_WINDOW_BASE(win)	(0x64 + ((win) << 4))
 #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
 
+struct ahci_mvebu_plat_data {
+	int (*plat_config)(struct ahci_host_priv *hpriv);
+};
+
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
 				   const struct mbus_dram_target_info *dram)
 {
@@ -62,6 +66,22 @@ static void ahci_mvebu_regret_option(struct ahci_host_priv *hpriv)
 	writel(0x80, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA);
 }
 
+static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
+{
+	const struct mbus_dram_target_info *dram;
+	int rc = 0;
+
+	dram = mv_mbus_dram_info();
+	if (dram)
+		ahci_mvebu_mbus_config(hpriv, dram);
+	else
+		rc = -ENODEV;
+
+	ahci_mvebu_regret_option(hpriv);
+
+	return rc;
+}
+
 /**
  * ahci_mvebu_stop_engine
  *
@@ -126,13 +146,10 @@ static int ahci_mvebu_resume(struct platform_device *pdev)
 {
 	struct ata_host *host = platform_get_drvdata(pdev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	const struct mbus_dram_target_info *dram;
+	const struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
 
-	dram = mv_mbus_dram_info();
-	if (dram)
-		ahci_mvebu_mbus_config(hpriv, dram);
-
-	ahci_mvebu_regret_option(hpriv);
+	if (pdata->plat_config)
+		pdata->plat_config(hpriv);
 
 	return ahci_platform_resume_host(&pdev->dev);
 }
@@ -154,28 +171,29 @@ static struct scsi_host_template ahci_platform_sht = {
 
 static int ahci_mvebu_probe(struct platform_device *pdev)
 {
+	const struct ahci_mvebu_plat_data *pdata;
 	struct ahci_host_priv *hpriv;
-	const struct mbus_dram_target_info *dram;
 	int rc;
 
 	hpriv = ahci_platform_get_resources(pdev, 0);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
+	hpriv->plat_data = (void *)of_device_get_match_data(&pdev->dev);
+	if (!hpriv->plat_data)
+		return -EINVAL;
+
 	rc = ahci_platform_enable_resources(hpriv);
 	if (rc)
 		return rc;
 
 	hpriv->stop_engine = ahci_mvebu_stop_engine;
 
-	if (of_device_is_compatible(pdev->dev.of_node,
-				    "marvell,armada-380-ahci")) {
-		dram = mv_mbus_dram_info();
-		if (!dram)
-			return -ENODEV;
-
-		ahci_mvebu_mbus_config(hpriv, dram);
-		ahci_mvebu_regret_option(hpriv);
+	pdata = hpriv->plat_data;
+	if (pdata->plat_config) {
+		rc = pdata->plat_config(hpriv);
+		if (rc)
+			goto disable_resources;
 	}
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_mvebu_port_info,
@@ -190,9 +208,23 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	return rc;
 }
 
+static const struct ahci_mvebu_plat_data ahci_mvebu_armada_380_plat_data = {
+	.plat_config = ahci_mvebu_armada_380_config,
+};
+
+static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
+	.plat_config = NULL,
+};
+
 static const struct of_device_id ahci_mvebu_of_match[] = {
-	{ .compatible = "marvell,armada-380-ahci", },
-	{ .compatible = "marvell,armada-3700-ahci", },
+	{
+		.compatible = "marvell,armada-380-ahci",
+		.data = &ahci_mvebu_armada_380_plat_data,
+	},
+	{
+		.compatible = "marvell,armada-3700-ahci",
+		.data = &ahci_mvebu_armada_3700_plat_data,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ahci_mvebu_of_match);
Signature · 1 line
2.19.1
#9 lore raw

PATCH 4/7 ata: ahci: mvebu: add Armada 3700 initialization needed for S2RAM

In reply to Miquel Raynal

A3700 comphy initialization is done in the firmware (TF-A). Looking at the SATA PHY initialization routine, there is a comment about "vendor specific" registers. Two registers are mentioned. They are not initialized there in the firmware because they are AHCI related, while the firmware at this location does only PHY configuration. The solution to avoid doing such initialization is relying on U-Boot.

While this work at boot time, U-Boot is definitely not going to run during a resume after suspending to RAM.

Two possible solutions were considered:
* Fixing the firmware.
* Fixing the kernel driver.

The first solution would take ages to propagate, while the second solution is easy to implement as the driver as been a little bit reworked to prepare for such platform configuration. Hence, this patch adds an Armada 3700 configuration function to set these two registers both at boot time (in the probe) and after a suspend (in the resume path).

---
 drivers/ata/ahci_mvebu.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)
drivers/ata/ahci_mvebu.c +18 -8
index a1fce848bba9..902971bfe301 100644
@@ -82,6 +82,19 @@ static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
 	return rc;
 }
 
+static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv)
+{
+	u32 reg;
+
+	writel(0, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_ADDR);
+
+	reg = readl(hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA);
+	reg |= BIT(6);
+	writel(reg, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA);
+
+	return 0;
+}
+
 /**
  * ahci_mvebu_stop_engine
  *
@@ -148,8 +161,7 @@ static int ahci_mvebu_resume(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	const struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
 
-	if (pdata->plat_config)
-		pdata->plat_config(hpriv);
+	pdata->plat_config(hpriv);
 
 	return ahci_platform_resume_host(&pdev->dev);
 }
@@ -190,11 +202,9 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	hpriv->stop_engine = ahci_mvebu_stop_engine;
 
 	pdata = hpriv->plat_data;
-	if (pdata->plat_config) {
-		rc = pdata->plat_config(hpriv);
-		if (rc)
-			goto disable_resources;
-	}
+	rc = pdata->plat_config(hpriv);
+	if (rc)
+		goto disable_resources;
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_mvebu_port_info,
 				     &ahci_platform_sht);
@@ -213,7 +223,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_380_plat_data = {
 };
 
 static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
-	.plat_config = NULL,
+	.plat_config = ahci_mvebu_armada_3700_config,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
Signature · 1 line
2.19.1
#10 lore raw

PATCH 5/7 ata: ahci: mvebu: add clock support

In reply to Miquel Raynal

Add clock support to the ahci_mvebu.c driver. This is needed in order to get suspend to RAM working.

---
 drivers/ata/ahci_mvebu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/ata/ahci_mvebu.c +10 -0
index 902971bfe301..cad35806c3c2 100644
@@ -195,6 +195,16 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!hpriv->plat_data)
 		return -EINVAL;
 
+	/* The clock property is absent in old bindings */
+	hpriv->clks[0] = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(hpriv->clks[0])) {
+		if (PTR_ERR(hpriv->clks[0]) == -EPROBE_DEFER)
+			return PTR_ERR(hpriv->clks[0]);
+
+		dev_warn(&pdev->dev, "no clock available\n");
+		hpriv->clks[0] = NULL;
+	}
+
 	rc = ahci_platform_enable_resources(hpriv);
 	if (rc)
 		return rc;
Signature · 1 line
2.19.1
#11 lore raw

Re: [PATCH 5/7] ata: ahci: mvebu: add clock support

Hello,

Miquel Raynal <[email protected]> wrote on Fri, 23 Nov 2018 11:15:54 +0100:

Quoted 16 lines

Add clock support to the ahci_mvebu.c driver. This is needed in order to get suspend to RAM working.

---
 drivers/ata/ahci_mvebu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/ata/ahci_mvebu.c +0 -0
index 902971bfe301..cad35806c3c2 100644
@@ -195,6 +195,16 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!hpriv->plat_data)
 		return -EINVAL;
 

drivers/ata/ahci_mvebu.c static int ahci_mvebu_probe(struct platform_device *pdev) · line 207

Quoted 10 lines
drivers/ata/ahci_mvebu.c +10 -0
+	/* The clock property is absent in old bindings */
+	hpriv->clks[0] = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(hpriv->clks[0])) {
+		if (PTR_ERR(hpriv->clks[0]) == -EPROBE_DEFER)
+			return PTR_ERR(hpriv->clks[0]);
+
+		dev_warn(&pdev->dev, "no clock available\n");
+		hpriv->clks[0] = NULL;
+	}
+

During a debug session about clock device links I realized this patch was not needed. The clk_get() on hpriv->clks[*] is done in libahci_platform.c in ahci_platform_get_resources() so this is redundant.

I'll fix it in the v2.

drivers/ata/ahci_mvebu.c static int ahci_mvebu_probe(struct platform_device *pdev) · line 210

Quoted 3 lines
drivers/ata/ahci_mvebu.c +0 -0
 	rc = ahci_platform_enable_resources(hpriv);
 	if (rc)
 		return rc;

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#12 lore raw

PATCH 6/7 ARM64: dts: marvell: armada-37xx: declare SATA clock

In reply to Miquel Raynal

The SATA IP get its clock from the north-bridge.

---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/boot/dts/marvell/armada-37xx.dtsi +1 -0
index 036d6fd6c9ef..65bf774516ec 100644
@@ -389,6 +389,7 @@
 				compatible = "marvell,armada-3700-ahci";
 				reg = <0xe0000 0x178>;
 				interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&nb_periph_clk 1>;
 				status = "disabled";
 			};
 
Signature · 1 line
2.19.1
#13 lore raw

PATCH 7/7 ARM64: dts: marvell: armada-3720-espressobin: declare SATA PHY property

In reply to Miquel Raynal

The SATA node is wired to the third PHY of the COMPHY IP.

---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 2 ++
 1 file changed, 2 insertions(+)
arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts +2 -0
index 3ab25ad402b9..e947ef61d946 100644
@@ -51,6 +51,8 @@
 /* J6 */
 &sata {
 	status = "okay";
+	phys = <&comphy2 0>;
+	phy-names = "sata-phy";
 };
 
 /* J1 */
Signature · 1 line
2.19.1