Message-Id: <20241121-ahci-nonconsecutive-ports-v1-1-1a20f52816fb@solid-run.com>
Date: Thu, 21 Nov 2024 16:05:11 +0100
From: Josua Mayer <josua@...id-run.com>
To: Damien Le Moal <dlemoal@...nel.org>, Niklas Cassel <cassel@...nel.org>, 
 Hans de Goede <hdegoede@...hat.com>
Cc: Jon Nettleton <jon@...id-run.com>, 
 Mikhail Anikin <mikhail.anikin@...id-run.com>, 
 Yazan Shhady <yazan.shhady@...id-run.com>, 
 Rabeeh Khoury <rabeeh@...id-run.com>, linux-ide@...r.kernel.org, 
 linux-kernel@...r.kernel.org, Josua Mayer <josua@...id-run.com>
Subject: [PATCH RFC] ata: libahci_platform: support non-consecutive port
 numbers

So far ahci_platform relied on number of child nodes in firmware to
allocate arrays and expected port numbers to start from 0 without holes.
This number of ports is then set in private structure for use when
configuring phys and regulators.

Some platforms may not use every port of an ahci controller.
E.g. SolidRUN CN9130 Clearfog uses only port 1 but not port 0, leading
to the following errors during boot:
[    1.719476] ahci f2540000.sata: invalid port number 1
[    1.724562] ahci f2540000.sata: No port enabled

Remove from ahci_host_priv the property nports which only makes sense
when enabled ports are consecutive. It is replaced with AHCI_MAX_PORTS
and checks for hpriv->mask_port_map, which indicates each port that is
enabled.

Update ahci_host_priv properties target_pwrs and phys from dynamically
allocated arrays to statically allocated to size AHCI_MAX_PORTS.

Update ahci_platform_get_resources to ignore holes in the port numbers
and enable ports defined in firmware by their reg property only.

When firmware does not define children it is assumed that there is
exactly one port, using index 0.

I marked this RFC because it was only tested with Linux v6.1, Marvell
fork, CN9130 Clearfog Pro which has only port number 1 in device-tree.
Further I am not completely sure if it has severe side-effects on
other platforms.
I plan to submit it again after testing on v6.13-rc1, but do welcome
feedback in the meantime, particularly whether this idea of supporting
non-consecutive ports is acceptable.

Signed-off-by: Josua Mayer <josua@...id-run.com>
---
 drivers/ata/ahci.h             |  5 ++-
 drivers/ata/ahci_brcm.c        |  5 ++-
 drivers/ata/ahci_ceva.c        | 22 +++++++-----
 drivers/ata/libahci_platform.c | 81 ++++++++++++++----------------------------
 4 files changed, 46 insertions(+), 67 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 8f40f75ba08cff4deca261b1162ee9a0fb91fed6..544b6242c11131f75c724d15244a74588410ee05 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -348,15 +348,14 @@ struct ahci_host_priv {
 	struct clk_bulk_data	*clks;		/* Optional */
 	unsigned int		f_rsts;
 	struct reset_control	*rsts;		/* Optional */
-	struct regulator	**target_pwrs;	/* Optional */
+	struct regulator	*target_pwrs[AHCI_MAX_PORTS];	/* Optional */
 	struct regulator	*ahci_regulator;/* Optional */
 	struct regulator	*phy_regulator;/* Optional */
 	/*
 	 * If platform uses PHYs. There is a 1:1 relation between the port number and
 	 * the PHY position in this array.
 	 */
-	struct phy		**phys;
-	unsigned		nports;		/* Number of ports */
+	struct phy		*phys[AHCI_MAX_PORTS];
 	void			*plat_data;	/* Other platform data */
 	unsigned int		irq;		/* interrupt line */
 	/*
diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 2f16524c252629cf89014144b33fec85d6bca137..169519c4cde37fe8aea6a48fbef16fd10d4d402c 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -287,7 +287,10 @@ static unsigned int brcm_ahci_read_id(struct ata_device *dev,
 	brcm_sata_phy_enable(priv, ap->port_no);
 
 	/* Re-initialize and calibrate the PHY */
-	for (i = 0; i < hpriv->nports; i++) {
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_phys;
diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
index 11a2c199a7c24628e858f2fc8e88e69a60c8b94b..4f0b5674f68e62d6871207e82de16c8422232285 100644
--- a/drivers/ata/ahci_ceva.c
+++ b/drivers/ata/ahci_ceva.c
@@ -205,20 +205,24 @@ static int ceva_ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 	if (rc)
 		goto disable_clks;
 
-	for (i = 0; i < hpriv->nports; i++) {
-		rc = phy_init(hpriv->phys[i]);
-		if (rc)
-			goto disable_rsts;
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (hpriv->mask_port_map & (1 << i)) {
+			rc = phy_init(hpriv->phys[i]);
+			if (rc)
+				goto disable_rsts;
+		}
 	}
 
 	/* De-assert the controller reset */
 	ahci_platform_deassert_rsts(hpriv);
 
-	for (i = 0; i < hpriv->nports; i++) {
-		rc = phy_power_on(hpriv->phys[i]);
-		if (rc) {
-			phy_exit(hpriv->phys[i]);
-			goto disable_phys;
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (hpriv->mask_port_map & (1 << i)) {
+			rc = phy_power_on(hpriv->phys[i]);
+			if (rc) {
+				phy_exit(hpriv->phys[i]);
+				goto disable_phys;
+			}
 		}
 	}
 
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 7a8064520a35bd86a1fa82d05c1ecaa8a81b7010..e73dbdaa17604999561646f8d0b04f3e7314305c 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -48,7 +48,10 @@ int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 {
 	int rc, i;
 
-	for (i = 0; i < hpriv->nports; i++) {
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_phys;
@@ -70,8 +73,10 @@ int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 
 disable_phys:
 	while (--i >= 0) {
-		phy_power_off(hpriv->phys[i]);
-		phy_exit(hpriv->phys[i]);
+		if (hpriv->mask_port_map & (1 << i)) {
+			phy_power_off(hpriv->phys[i]);
+			phy_exit(hpriv->phys[i]);
+		}
 	}
 	return rc;
 }
@@ -87,9 +92,11 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 {
 	int i;
 
-	for (i = 0; i < hpriv->nports; i++) {
-		phy_power_off(hpriv->phys[i]);
-		phy_exit(hpriv->phys[i]);
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (hpriv->mask_port_map & (1 << i)) {
+			phy_power_off(hpriv->phys[i]);
+			phy_exit(hpriv->phys[i]);
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
@@ -209,13 +216,12 @@ int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
 	if (rc)
 		goto disable_ahci_pwrs;
 
-	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->target_pwrs[i])
-			continue;
-
-		rc = regulator_enable(hpriv->target_pwrs[i]);
-		if (rc)
-			goto disable_target_pwrs;
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (hpriv->mask_port_map & (1 << i)) {
+			rc = regulator_enable(hpriv->target_pwrs[i]);
+			if (rc)
+				goto disable_target_pwrs;
+		}
 	}
 
 	return 0;
@@ -243,10 +249,9 @@ void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
 {
 	int i;
 
-	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->target_pwrs[i])
-			continue;
-		regulator_disable(hpriv->target_pwrs[i]);
+	for (i = 0; i < AHCI_MAX_PORTS; i++) {
+		if (hpriv->mask_port_map & (1 << i))
+			regulator_disable(hpriv->target_pwrs[i]);
 	}
 
 	regulator_disable(hpriv->ahci_regulator);
@@ -343,8 +348,8 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
 	 * SATA device itself. So we can't use devm for automatically
 	 * releasing them. We have to do it manually here.
 	 */
-	for (c = 0; c < hpriv->nports; c++)
-		if (hpriv->target_pwrs && hpriv->target_pwrs[c])
+	for (c = 0; c < AHCI_MAX_PORTS; c++)
+		if ((hpriv->mask_port_map & (1 << c)) && hpriv->target_pwrs[c])
 			regulator_put(hpriv->target_pwrs[c]);
 
 	kfree(hpriv->target_pwrs);
@@ -539,41 +544,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		hpriv->f_rsts = flags & AHCI_PLATFORM_RST_TRIGGER;
 	}
 
-	/*
-	 * Too many sub-nodes most likely means having something wrong with
-	 * the firmware.
-	 */
 	child_nodes = of_get_child_count(dev->of_node);
-	if (child_nodes > AHCI_MAX_PORTS) {
-		rc = -EINVAL;
-		goto err_out;
-	}
-
-	/*
-	 * If no sub-node was found, we still need to set nports to
-	 * one in order to be able to use the
-	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
-	 */
-	if (child_nodes)
-		hpriv->nports = child_nodes;
-	else
-		hpriv->nports = 1;
-
-	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
-	if (!hpriv->phys) {
-		rc = -ENOMEM;
-		goto err_out;
-	}
-	/*
-	 * We cannot use devm_ here, since ahci_platform_put_resources() uses
-	 * target_pwrs after devm_ have freed memory
-	 */
-	hpriv->target_pwrs = kcalloc(hpriv->nports, sizeof(*hpriv->target_pwrs), GFP_KERNEL);
-	if (!hpriv->target_pwrs) {
-		rc = -ENOMEM;
-		goto err_out;
-	}
-
 	if (child_nodes) {
 		for_each_child_of_node_scoped(dev->of_node, child) {
 			u32 port;
@@ -587,7 +558,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 				goto err_out;
 			}
 
-			if (port >= hpriv->nports) {
+			if (port >= AHCI_MAX_PORTS) {
 				dev_warn(dev, "invalid port number %d\n", port);
 				continue;
 			}
@@ -625,6 +596,8 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		 * If no sub-node was found, keep this for device tree
 		 * compatibility
 		 */
+		hpriv->mask_port_map |= BIT(0);
+
 		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
 		if (rc)
 			goto err_out;

---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241121-ahci-nonconsecutive-ports-a8911b3255a7

Best regards,
-- 
Josua Mayer <josua@...id-run.com>

Powered by blists - more mailing lists