[PATCH v2 0/6] Bring suspend to RAM support to MVEBU SATA

[PATCH v2 0/6] Bring suspend to RAM support to MVEBU SATA

[Miquel Raynal]

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

Changes since v1:
=================
* The clock is automatically requested by the libahci_platform.c
  driver, doing it in the mvebu driver is redundant, remove the patch
  adding clock support as clock support already exists.
* Changed authorship of patch adding a SATA enum in the PHY core.
* Added Suggested-by tag to the patch fixing the SATA node scope in DT,
  to the patch adding PHY framework compliance to the
  libahci_platform driver and to the DT patch adding the SATA PHY
  property.
* Add a flag to do not disable/enable the PHY for compatibility
  reasons and to avoid to break untested boards with this change.
  The flag is called AHCI_HFLAG_MANAGE_PHYS.
* The mvebu ahci driver is edited to enable this flag only on A3700.


Miquel Raynal (6):
  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
  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.h                            |  2 +
 drivers/ata/ahci_mvebu.c                      | 88 ++++++++++++++-----
 drivers/ata/libahci_platform.c                | 19 +++-
 5 files changed, 87 insertions(+), 25 deletions(-)

-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/6] ata: libahci_platform: comply to PHY framework

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

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci.h             |  2 ++
 drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index ef356e70e6de..982638b37fee 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -254,6 +254,8 @@ enum {
 	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
 							SATA_MOBILE_LPM_POLICY
 							as default lpm_policy */
+	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
+							PHYs when relevant */
 
 	/* ap->flags bits */
 
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 4b900fc659f7..f5a64eb1fea8 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -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]);
@@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
  * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
  *    or for non devicetree enabled platforms a single clock
  * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
- * 5) phys (optional)
+ * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
+ *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
  *
  * RETURNS:
  * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
@@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		}
 	}
 
+	if (flags & AHCI_HFLAG_MANAGE_PHYS)
+		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
+
 	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
 
 	/*
@@ -738,6 +748,9 @@ int ahci_platform_suspend_host(struct device *dev)
 	writel(ctl, mmio + HOST_CTL);
 	readl(mmio + HOST_CTL); /* flush */
 
+	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
+		ahci_platform_disable_phys(hpriv);
+
 	return ata_host_suspend(host, PMSG_SUSPEND);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
@@ -756,6 +769,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 +780,9 @@ int ahci_platform_resume_host(struct device *dev)
 		ahci_init_controller(host);
 	}
 
+	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
+		ahci_platform_enable_phys(hpriv);
+
 	ata_host_resume(host);
 
 	return 0;
-- 
2.19.1

[PATCH v2 1/6] ata: libahci_platform: comply to PHY framework

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

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci.h             |  2 ++
 drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index ef356e70e6de..982638b37fee 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -254,6 +254,8 @@ enum {
 	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
 							SATA_MOBILE_LPM_POLICY
 							as default lpm_policy */
+	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
+							PHYs when relevant */
 
 	/* ap->flags bits */
 
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 4b900fc659f7..f5a64eb1fea8 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -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]);
@@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
  * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
  *    or for non devicetree enabled platforms a single clock
  * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
- * 5) phys (optional)
+ * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
+ *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
  *
  * RETURNS:
  * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
@@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		}
 	}
 
+	if (flags & AHCI_HFLAG_MANAGE_PHYS)
+		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
+
 	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
 
 	/*
@@ -738,6 +748,9 @@ int ahci_platform_suspend_host(struct device *dev)
 	writel(ctl, mmio + HOST_CTL);
 	readl(mmio + HOST_CTL); /* flush */
 
+	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
+		ahci_platform_disable_phys(hpriv);
+
 	return ata_host_suspend(host, PMSG_SUSPEND);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
@@ -756,6 +769,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 +780,9 @@ int ahci_platform_resume_host(struct device *dev)
 		ahci_init_controller(host);
 	}
 
+	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
+		ahci_platform_enable_phys(hpriv);
+
 	ata_host_resume(host);
 
 	return 0;
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/6] ata: ahci: mvebu: remove stale comment

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")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index f9cb51be38eb..128d6f22926d 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -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,
-- 
2.19.1

[PATCH v2 2/6] ata: ahci: mvebu: remove stale comment

[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")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index f9cb51be38eb..128d6f22926d 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -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,
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/6] ata: ahci: mvebu: do Armada 38x configuration only on relevant SoCs

[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")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 68 ++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 128d6f22926d..7839a5df1fd2 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -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,31 @@ 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;
 
+	pdata = of_device_get_match_data(&pdev->dev);
+	if (!pdata)
+		return -EINVAL;
+
 	hpriv = ahci_platform_get_resources(pdev, 0);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
+	hpriv->plat_data = (void *)pdata;
+
 	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 +210,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);
-- 
2.19.1

[PATCH v2 3/6] ata: ahci: mvebu: do Armada 38x configuration only on relevant SoCs

[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")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 68 ++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 128d6f22926d..7839a5df1fd2 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -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,31 @@ 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;
 
+	pdata = of_device_get_match_data(&pdev->dev);
+	if (!pdata)
+		return -EINVAL;
+
 	hpriv = ahci_platform_get_resources(pdev, 0);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
+	hpriv->plat_data = (void *)pdata;
+
 	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 +210,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);
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/6] ata: ahci: mvebu: add Armada 3700 initialization needed for S2RAM

[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).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 7839a5df1fd2..761d67c5bc31 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -30,6 +30,7 @@
 
 struct ahci_mvebu_plat_data {
 	int (*plat_config)(struct ahci_host_priv *hpriv);
+	unsigned int flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -82,6 +83,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 +162,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);
 }
@@ -179,7 +192,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!pdata)
 		return -EINVAL;
 
-	hpriv = ahci_platform_get_resources(pdev, 0);
+	hpriv = ahci_platform_get_resources(pdev, pdata->flags);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
@@ -191,12 +204,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);
@@ -215,7 +225,8 @@ 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,
+	.flags = AHCI_HFLAG_MANAGE_PHYS,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
-- 
2.19.1

[PATCH v2 4/6] ata: ahci: mvebu: add Armada 3700 initialization needed for S2RAM

[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).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 7839a5df1fd2..761d67c5bc31 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -30,6 +30,7 @@
 
 struct ahci_mvebu_plat_data {
 	int (*plat_config)(struct ahci_host_priv *hpriv);
+	unsigned int flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -82,6 +83,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 +162,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);
 }
@@ -179,7 +192,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!pdata)
 		return -EINVAL;
 
-	hpriv = ahci_platform_get_resources(pdev, 0);
+	hpriv = ahci_platform_get_resources(pdev, pdata->flags);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
@@ -191,12 +204,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);
@@ -215,7 +225,8 @@ 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,
+	.flags = AHCI_HFLAG_MANAGE_PHYS,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/6] ARM64: dts: marvell: armada-37xx: declare SATA clock

[Miquel Raynal]

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

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 036d6fd6c9ef..65bf774516ec 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -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";
 			};
 
-- 
2.19.1

[PATCH v2 5/6] ARM64: dts: marvell: armada-37xx: declare SATA clock

[Miquel Raynal]

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

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 036d6fd6c9ef..65bf774516ec 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -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";
 			};
 
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/6] ARM64: dts: marvell: armada-3720-espressobin: declare SATA PHY property

[Miquel Raynal]

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

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 3ab25ad402b9..e947ef61d946 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -51,6 +51,8 @@
 /* J6 */
 &sata {
 	status = "okay";
+	phys = <&comphy2 0>;
+	phy-names = "sata-phy";
 };
 
 /* J1 */
-- 
2.19.1

[PATCH v2 6/6] ARM64: dts: marvell: armada-3720-espressobin: declare SATA PHY property

[Miquel Raynal]

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

Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 3ab25ad402b9..e947ef61d946 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -51,6 +51,8 @@
 /* J6 */
 &sata {
 	status = "okay";
+	phys = <&comphy2 0>;
+	phy-names = "sata-phy";
 };
 
 /* J1 */
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Hans de Goede]

Hi,

On 30-11-18 16:38, Miquel Raynal wrote:
> 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.
> 
> Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Much better. Some remarks inline.

> ---
>   drivers/ata/ahci.h             |  2 ++
>   drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
>   2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index ef356e70e6de..982638b37fee 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -254,6 +254,8 @@ enum {
>   	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
>   							SATA_MOBILE_LPM_POLICY
>   							as default lpm_policy */
> +	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
> +							PHYs when relevant */

Using a hflag for this is a good idea, but the MANAGE_PHYS names is a bit
generic, even if this is not set then ahci_platform_enable_resources() will
still enable the phy. So I would name this AHCI_HFLAG_SUSPEND_PHYS to make
clear this only influences suspend/resume behavior.

Also see below.

>   	/* ap->flags bits */
>   
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 4b900fc659f7..f5a64eb1fea8 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -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]);
> @@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>    * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
>    *    or for non devicetree enabled platforms a single clock
>    * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
> - * 5) phys (optional)
> + * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
> + *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
>    *
>    * RETURNS:
>    * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
> @@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		}
>   	}
>   
> +	if (flags & AHCI_HFLAG_MANAGE_PHYS)
> +		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
> +
>   	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>   
>   	/*

Interpreting a flag passed to ahci_platform_get_resources using a
AHCI_HFLAG_* mask is asking for trouble in the future.

Since you are using a hflag for this, you can just drop this chunk / change
and in the caller of ahci_platform_get_resources() do:

	hpriv = ahci_platform_get_resources(pdev, ...);
	if (IS_ERR(hpriv))
		...

	hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;

Settting hflags like this after calling ahci_platform_get_resources()
is already done in several place.

Once this is changed, this patch looks good to go upstream to me.

Regards,

Hans




> @@ -738,6 +748,9 @@ int ahci_platform_suspend_host(struct device *dev)
>   	writel(ctl, mmio + HOST_CTL);
>   	readl(mmio + HOST_CTL); /* flush */
>   
> +	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
> +		ahci_platform_disable_phys(hpriv);
> +
>   	return ata_host_suspend(host, PMSG_SUSPEND);
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
> @@ -756,6 +769,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 +780,9 @@ int ahci_platform_resume_host(struct device *dev)
>   		ahci_init_controller(host);
>   	}
>   
> +	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
> +		ahci_platform_enable_phys(hpriv);
> +
>   	ata_host_resume(host);
>   
>   	return 0;
> 

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

[Hans de Goede]

Hi,

On 30-11-18 16:38, Miquel Raynal wrote:
> 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.
> 
> Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Much better. Some remarks inline.

> ---
>   drivers/ata/ahci.h             |  2 ++
>   drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
>   2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index ef356e70e6de..982638b37fee 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -254,6 +254,8 @@ enum {
>   	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
>   							SATA_MOBILE_LPM_POLICY
>   							as default lpm_policy */
> +	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
> +							PHYs when relevant */

Using a hflag for this is a good idea, but the MANAGE_PHYS names is a bit
generic, even if this is not set then ahci_platform_enable_resources() will
still enable the phy. So I would name this AHCI_HFLAG_SUSPEND_PHYS to make
clear this only influences suspend/resume behavior.

Also see below.

>   	/* ap->flags bits */
>   
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 4b900fc659f7..f5a64eb1fea8 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -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]);
> @@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>    * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
>    *    or for non devicetree enabled platforms a single clock
>    * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
> - * 5) phys (optional)
> + * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
> + *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
>    *
>    * RETURNS:
>    * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
> @@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		}
>   	}
>   
> +	if (flags & AHCI_HFLAG_MANAGE_PHYS)
> +		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
> +
>   	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>   
>   	/*

Interpreting a flag passed to ahci_platform_get_resources using a
AHCI_HFLAG_* mask is asking for trouble in the future.

Since you are using a hflag for this, you can just drop this chunk / change
and in the caller of ahci_platform_get_resources() do:

	hpriv = ahci_platform_get_resources(pdev, ...);
	if (IS_ERR(hpriv))
		...

	hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;

Settting hflags like this after calling ahci_platform_get_resources()
is already done in several place.

Once this is changed, this patch looks good to go upstream to me.

Regards,

Hans




> @@ -738,6 +748,9 @@ int ahci_platform_suspend_host(struct device *dev)
>   	writel(ctl, mmio + HOST_CTL);
>   	readl(mmio + HOST_CTL); /* flush */
>   
> +	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
> +		ahci_platform_disable_phys(hpriv);
> +
>   	return ata_host_suspend(host, PMSG_SUSPEND);
>   }
>   EXPORT_SYMBOL_GPL(ahci_platform_suspend_host);
> @@ -756,6 +769,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 +780,9 @@ int ahci_platform_resume_host(struct device *dev)
>   		ahci_init_controller(host);
>   	}
>   
> +	if (hpriv->flags & AHCI_HFLAG_MANAGE_PHYS)
> +		ahci_platform_enable_phys(hpriv);
> +
>   	ata_host_resume(host);
>   
>   	return 0;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Miquel Raynal]

Hi Hans,

Hans de Goede <hdegoede@redhat.com> wrote on Sun, 2 Dec 2018 14:19:49
+0100:

> Hi,
> 
> On 30-11-18 16:38, Miquel Raynal wrote:
> > 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.
> > 
> > Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Much better. Some remarks inline.
> 
> > ---
> >   drivers/ata/ahci.h             |  2 ++
> >   drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
> >   2 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> > index ef356e70e6de..982638b37fee 100644
> > --- a/drivers/ata/ahci.h
> > +++ b/drivers/ata/ahci.h
> > @@ -254,6 +254,8 @@ enum {
> >   	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
> >   							SATA_MOBILE_LPM_POLICY
> >   							as default lpm_policy */
> > +	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
> > +							PHYs when relevant */  
> 
> Using a hflag for this is a good idea, but the MANAGE_PHYS names is a bit
> generic, even if this is not set then ahci_platform_enable_resources() will
> still enable the phy. So I would name this AHCI_HFLAG_SUSPEND_PHYS to make
> clear this only influences suspend/resume behavior.

I understand and will modify the HFLAG name as you suggest.

> 
> Also see below.
> 
> >   	/* ap->flags bits */  
> >   > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c  
> > index 4b900fc659f7..f5a64eb1fea8 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -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]);
> > @@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> >    * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
> >    *    or for non devicetree enabled platforms a single clock
> >    * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
> > - * 5) phys (optional)
> > + * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
> > + *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
> >    *
> >    * RETURNS:
> >    * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
> > @@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >   		}
> >   	}  
> >   > +	if (flags & AHCI_HFLAG_MANAGE_PHYS)  
> > +		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
> > +
> >   	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);  
> >   >   	/*  
> 
> Interpreting a flag passed to ahci_platform_get_resources using a
> AHCI_HFLAG_* mask is asking for trouble in the future.
> 
> Since you are using a hflag for this, you can just drop this chunk / change
> and in the caller of ahci_platform_get_resources() do:
> 
> 	hpriv = ahci_platform_get_resources(pdev, ...);
> 	if (IS_ERR(hpriv))
> 		...
> 
> 	hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
> 
> Settting hflags like this after calling ahci_platform_get_resources()
> is already done in several place.

Indeed, this is close to my first draft but after re-reading your
previous review I understood you wanted this flag to be passed to
ahci_platform_get_resources().

I do agree that the current form is a bit redundant so I will let users
add the flag manually during the probe after the
ahci_platform_get_resources() call, as suggested.

> 
> Once this is changed, this patch looks good to go upstream to me.
> 
> Regards,
> 
> Hans
> 
> 
> 

Thanks for the review!
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Hans de Goede]

Hi,

On 03-12-18 15:46, Miquel Raynal wrote:
> Hi Hans,
> 
> Hans de Goede <hdegoede@redhat.com> wrote on Sun, 2 Dec 2018 14:19:49
> +0100:
> 
>> Hi,
>>
>> On 30-11-18 16:38, Miquel Raynal wrote:
>>> 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.
>>>
>>> Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> Much better. Some remarks inline.
>>
>>> ---
>>>    drivers/ata/ahci.h             |  2 ++
>>>    drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
>>>    2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>> index ef356e70e6de..982638b37fee 100644
>>> --- a/drivers/ata/ahci.h
>>> +++ b/drivers/ata/ahci.h
>>> @@ -254,6 +254,8 @@ enum {
>>>    	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
>>>    							SATA_MOBILE_LPM_POLICY
>>>    							as default lpm_policy */
>>> +	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
>>> +							PHYs when relevant */
>>
>> Using a hflag for this is a good idea, but the MANAGE_PHYS names is a bit
>> generic, even if this is not set then ahci_platform_enable_resources() will
>> still enable the phy. So I would name this AHCI_HFLAG_SUSPEND_PHYS to make
>> clear this only influences suspend/resume behavior.
> 
> I understand and will modify the HFLAG name as you suggest.
> 
>>
>> Also see below.
>>
>>>    	/* ap->flags bits */
>>>    > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index 4b900fc659f7..f5a64eb1fea8 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -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]);
>>> @@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>>>     * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
>>>     *    or for non devicetree enabled platforms a single clock
>>>     * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
>>> - * 5) phys (optional)
>>> + * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
>>> + *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
>>>     *
>>>     * RETURNS:
>>>     * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
>>> @@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>>    		}
>>>    	}
>>>    > +	if (flags & AHCI_HFLAG_MANAGE_PHYS)
>>> +		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
>>> +
>>>    	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>>>    >   	/*
>>
>> Interpreting a flag passed to ahci_platform_get_resources using a
>> AHCI_HFLAG_* mask is asking for trouble in the future.
>>
>> Since you are using a hflag for this, you can just drop this chunk / change
>> and in the caller of ahci_platform_get_resources() do:
>>
>> 	hpriv = ahci_platform_get_resources(pdev, ...);
>> 	if (IS_ERR(hpriv))
>> 		...
>>
>> 	hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
>>
>> Settting hflags like this after calling ahci_platform_get_resources()
>> is already done in several place.
> 
> Indeed, this is close to my first draft but after re-reading your
> previous review I understood you wanted this flag to be passed to
> ahci_platform_get_resources().

Yes that was my initial thought, but sine you are using a hflag for
it that is no longer a good idea.

> I do agree that the current form is a bit redundant so I will let users
> add the flag manually during the probe after the
> ahci_platform_get_resources() call, as suggested.

Sounds good, thanks.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 03-12-18 15:46, Miquel Raynal wrote:
> Hi Hans,
> 
> Hans de Goede <hdegoede@redhat.com> wrote on Sun, 2 Dec 2018 14:19:49
> +0100:
> 
>> Hi,
>>
>> On 30-11-18 16:38, Miquel Raynal wrote:
>>> 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.
>>>
>>> Suggested-by: Grzegorz Jaszczyk <jaz@semihalf.com>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> Much better. Some remarks inline.
>>
>>> ---
>>>    drivers/ata/ahci.h             |  2 ++
>>>    drivers/ata/libahci_platform.c | 19 ++++++++++++++++++-
>>>    2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>>> index ef356e70e6de..982638b37fee 100644
>>> --- a/drivers/ata/ahci.h
>>> +++ b/drivers/ata/ahci.h
>>> @@ -254,6 +254,8 @@ enum {
>>>    	AHCI_HFLAG_IS_MOBILE		= (1 << 25), /* mobile chipset, use
>>>    							SATA_MOBILE_LPM_POLICY
>>>    							as default lpm_policy */
>>> +	AHCI_HFLAG_MANAGE_PHYS		= (1 << 26), /* let the core manage the
>>> +							PHYs when relevant */
>>
>> Using a hflag for this is a good idea, but the MANAGE_PHYS names is a bit
>> generic, even if this is not set then ahci_platform_enable_resources() will
>> still enable the phy. So I would name this AHCI_HFLAG_SUSPEND_PHYS to make
>> clear this only influences suspend/resume behavior.
> 
> I understand and will modify the HFLAG name as you suggest.
> 
>>
>> Also see below.
>>
>>>    	/* ap->flags bits */
>>>    > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>>> index 4b900fc659f7..f5a64eb1fea8 100644
>>> --- a/drivers/ata/libahci_platform.c
>>> +++ b/drivers/ata/libahci_platform.c
>>> @@ -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]);
>>> @@ -378,7 +384,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>>>     * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
>>>     *    or for non devicetree enabled platforms a single clock
>>>     * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
>>> - * 5) phys (optional)
>>> + * 5) phys (optional), PHY handling during suspend/resume will be skipped if the
>>> + *    flag AHCI_HFLAG_MANAGE_PHYS is missing.
>>>     *
>>>     * RETURNS:
>>>     * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
>>> @@ -458,6 +465,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>>>    		}
>>>    	}
>>>    > +	if (flags & AHCI_HFLAG_MANAGE_PHYS)
>>> +		hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
>>> +
>>>    	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
>>>    >   	/*
>>
>> Interpreting a flag passed to ahci_platform_get_resources using a
>> AHCI_HFLAG_* mask is asking for trouble in the future.
>>
>> Since you are using a hflag for this, you can just drop this chunk / change
>> and in the caller of ahci_platform_get_resources() do:
>>
>> 	hpriv = ahci_platform_get_resources(pdev, ...);
>> 	if (IS_ERR(hpriv))
>> 		...
>>
>> 	hpriv->flags |= AHCI_HFLAG_MANAGE_PHYS;
>>
>> Settting hflags like this after calling ahci_platform_get_resources()
>> is already done in several place.
> 
> Indeed, this is close to my first draft but after re-reading your
> previous review I understood you wanted this flag to be passed to
> ahci_platform_get_resources().

Yes that was my initial thought, but sine you are using a hflag for
it that is no longer a good idea.

> I do agree that the current form is a bit redundant so I will let users
> add the flag manually during the probe after the
> ahci_platform_get_resources() call, as suggested.

Sounds good, thanks.

Regards,

Hans


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel