diff mbox series

[v2] ata: libahci_platform: support non-consecutive port numbers

Message ID 20250101-ahci-nonconsecutive-ports-v2-1-38a48f357321@solid-run.com
State New
Headers show
Series [v2] ata: libahci_platform: support non-consecutive port numbers | expand

Commit Message

Josua Mayer Jan. 1, 2025, 12:13 p.m. UTC
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

Update all accessesors of ahci_host_priv phys and target_pwrs arrays to
support holes. Access is gated by hpriv->mask_port_map which has a bit
set for each enabled port.

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.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Changes in v2:
- reverted back to dynamically allocated arrays
  (Reported-by: Damien Le Moal <dlemoal@kernel.org>)
- added helper function to find maximum port id
  (Reported-by: Damien Le Moal <dlemoal@kernel.org>)
- reduced size of changes
- rebased on 6.13-rc1
- tested on 6.13-rc1 with CN9130 Clearfog Pro
- Link to v1: https://lore.kernel.org/r/20241121-ahci-nonconsecutive-ports-v1-1-1a20f52816fb@solid-run.com
---
 drivers/ata/ahci_brcm.c        |  3 +++
 drivers/ata/ahci_ceva.c        |  6 ++++++
 drivers/ata/libahci_platform.c | 40 ++++++++++++++++++++++++++++++++++------
 3 files changed, 43 insertions(+), 6 deletions(-)


---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241121-ahci-nonconsecutive-ports-a8911b3255a7

Best regards,

Comments

Hans de Goede Jan. 1, 2025, 1:17 p.m. UTC | #1
Hi,

On 1-Jan-25 1:13 PM, Josua Mayer wrote:
> 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
> 
> Update all accessesors of ahci_host_priv phys and target_pwrs arrays to
> support holes. Access is gated by hpriv->mask_port_map which has a bit
> set for each enabled port.
> 
> 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.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
> Changes in v2:
> - reverted back to dynamically allocated arrays
>   (Reported-by: Damien Le Moal <dlemoal@kernel.org>)
> - added helper function to find maximum port id
>   (Reported-by: Damien Le Moal <dlemoal@kernel.org>)
> - reduced size of changes
> - rebased on 6.13-rc1
> - tested on 6.13-rc1 with CN9130 Clearfog Pro
> - Link to v1: https://lore.kernel.org/r/20241121-ahci-nonconsecutive-ports-v1-1-1a20f52816fb@solid-run.com

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/ata/ahci_brcm.c        |  3 +++
>  drivers/ata/ahci_ceva.c        |  6 ++++++
>  drivers/ata/libahci_platform.c | 40 ++++++++++++++++++++++++++++++++++------
>  3 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index ef569eae4ce4625e92c24c7dd54e8704b9aff2c4..24c471b485ab8b43eca21909ea16cb47a2a95ee1 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -288,6 +288,9 @@ static unsigned int brcm_ahci_read_id(struct ata_device *dev,
>  
>  	/* Re-initialize and calibrate the PHY */
>  	for (i = 0; i < hpriv->nports; 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 1ec35778903ddc28aebdab7d72676a31e757e56f..f2e20ed11ec70f48cb5f2c12812996bb99872aa5 100644
> --- a/drivers/ata/ahci_ceva.c
> +++ b/drivers/ata/ahci_ceva.c
> @@ -206,6 +206,9 @@ static int ceva_ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
>  		goto disable_clks;
>  
>  	for (i = 0; i < hpriv->nports; i++) {
> +		if (!(hpriv->mask_port_map & (1 << i)))
> +			continue;
> +
>  		rc = phy_init(hpriv->phys[i]);
>  		if (rc)
>  			goto disable_rsts;
> @@ -215,6 +218,9 @@ static int ceva_ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
>  	ahci_platform_deassert_rsts(hpriv);
>  
>  	for (i = 0; i < hpriv->nports; i++) {
> +		if (!(hpriv->mask_port_map & (1 << i)))
> +			continue;
> +
>  		rc = phy_power_on(hpriv->phys[i]);
>  		if (rc) {
>  			phy_exit(hpriv->phys[i]);
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 7a8064520a35bd86a1fa82d05c1ecaa8a81b7010..b68777841f7a544b755a16a633b1a2a47b90da08 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -49,6 +49,9 @@ int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
>  	int rc, i;
>  
>  	for (i = 0; i < hpriv->nports; i++) {
> +		if (!(hpriv->mask_port_map & (1 << i)))
> +			continue;
> +
>  		rc = phy_init(hpriv->phys[i]);
>  		if (rc)
>  			goto disable_phys;
> @@ -70,6 +73,9 @@ int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
>  
>  disable_phys:
>  	while (--i >= 0) {
> +		if (!(hpriv->mask_port_map & (1 << i)))
> +			continue;
> +
>  		phy_power_off(hpriv->phys[i]);
>  		phy_exit(hpriv->phys[i]);
>  	}
> @@ -88,6 +94,9 @@ void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
>  	int i;
>  
>  	for (i = 0; i < hpriv->nports; i++) {
> +		if (!(hpriv->mask_port_map & (1 << i)))
> +			continue;
> +
>  		phy_power_off(hpriv->phys[i]);
>  		phy_exit(hpriv->phys[i]);
>  	}
> @@ -432,6 +441,20 @@ static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
>  	return 0;
>  }
>  
> +static u32 ahci_platform_find_max_port_id(struct device *dev)
> +{
> +	u32 max_port = 0;
> +
> +	for_each_child_of_node_scoped(dev->of_node, child) {
> +		u32 port;
> +
> +		if (!of_property_read_u32(child, "reg", &port))
> +			max_port = max(max_port, port);
> +	}
> +
> +	return max_port;
> +}
> +
>  /**
>   * ahci_platform_get_resources - Get platform resources
>   * @pdev: platform device to get resources for
> @@ -458,6 +481,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  	struct device *dev = &pdev->dev;
>  	struct ahci_host_priv *hpriv;
>  	u32 mask_port_map = 0;
> +	u32 max_port;
>  
>  	if (!devres_open_group(dev, NULL, GFP_KERNEL))
>  		return ERR_PTR(-ENOMEM);
> @@ -549,15 +573,17 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>  		goto err_out;
>  	}
>  
> +	/* find maximum port id for allocating structures */
> +	max_port = ahci_platform_find_max_port_id(dev);
>  	/*
> -	 * If no sub-node was found, we still need to set nports to
> -	 * one in order to be able to use the
> +	 * Set nports according to maximum port id. Clamp at
> +	 * AHCI_MAX_PORTS, warning message for invalid port id
> +	 * is generated later.
> +	 * When DT has no sub-nodes max_port is 0, nports is 1,
> +	 * 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->nports = min(AHCI_MAX_PORTS, max_port + 1);
>  
>  	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
>  	if (!hpriv->phys) {
> @@ -625,6 +651,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: 40384c840ea1944d7c5a392e8975ed088ecf0b37
> change-id: 20241121-ahci-nonconsecutive-ports-a8911b3255a7
> 
> Best regards,
Damien Le Moal Jan. 6, 2025, 5:27 a.m. UTC | #2
On 1/1/25 9:13 PM, Josua Mayer wrote:
> 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
> 
> Update all accessesors of ahci_host_priv phys and target_pwrs arrays to
> support holes. Access is gated by hpriv->mask_port_map which has a bit
> set for each enabled port.
> 
> 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.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>

Applied to for-6.14. Thanks !
Klaus Kudielka Feb. 5, 2025, 6:03 p.m. UTC | #3
On Wed, 2025-01-01 at 13:13 +0100, Josua Mayer wrote:
> 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
> 
> Update all accessesors of ahci_host_priv phys and target_pwrs arrays to
> support holes. Access is gated by hpriv->mask_port_map which has a bit
> set for each enabled port.
> 
> 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.
> 

[...]

> @@ -625,6 +651,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;
> 

This very last hunk (when firmware does not define children...) causes some change in behaviour on my Turris Omnia
(Armada 385):

6.13.0 bootlog
==============

Feb 05 18:30:45 xxxx kernel: ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
Feb 05 18:30:45 xxxx kernel: ahci-mvebu f10a8000.sata: 2/2 ports implemented (port mask 0x3)
Feb 05 18:30:45 xxxx kernel: ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
Feb 05 18:30:45 xxxx kernel: scsi host0: ahci-mvebu
Feb 05 18:30:45 xxxx kernel: scsi host1: ahci-mvebu
Feb 05 18:30:45 xxxx kernel: ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
Feb 05 18:30:45 xxxx kernel: ata2: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x180 irq 40 lpm-pol 0

Previously, both detected ports were automatically used, and no warning was emitted.
(hpriv->mask_port_map was 0x0)


6.14.0-rc1 bootlog
==================

Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1)
Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
Feb 05 18:36:40 xxxx kernel: scsi host0: ahci-mvebu
Feb 05 18:36:40 xxxx kernel: scsi host1: ahci-mvebu
Feb 05 18:36:40 xxxx kernel: ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
Feb 05 18:36:40 xxxx kernel: ata2: DUMMY

Now, hpriv->mask_port_map is forced to 0x1, resulting in a kernel warning, and no more ata2 available.
In my particular case it is not a big deal, since nothing is connected to the 2nd port, and I can live with this obscure warning.

But that might not be the case for everybody?

Regards, Klaus
Damien Le Moal Feb. 6, 2025, 1:34 a.m. UTC | #4
On 2/6/25 3:03 AM, Klaus Kudielka wrote:
> On Wed, 2025-01-01 at 13:13 +0100, Josua Mayer wrote:
>> 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
>>
>> Update all accessesors of ahci_host_priv phys and target_pwrs arrays to
>> support holes. Access is gated by hpriv->mask_port_map which has a bit
>> set for each enabled port.
>>
>> 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.
>>
> 
> [...]
> 
>> @@ -625,6 +651,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;
>>
> 
> This very last hunk (when firmware does not define children...) causes some change in behaviour on my Turris Omnia
> (Armada 385):
> 
> 6.13.0 bootlog
> ==============
> 
> Feb 05 18:30:45 xxxx kernel: ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
> Feb 05 18:30:45 xxxx kernel: ahci-mvebu f10a8000.sata: 2/2 ports implemented (port mask 0x3)
> Feb 05 18:30:45 xxxx kernel: ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> Feb 05 18:30:45 xxxx kernel: scsi host0: ahci-mvebu
> Feb 05 18:30:45 xxxx kernel: scsi host1: ahci-mvebu
> Feb 05 18:30:45 xxxx kernel: ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
> Feb 05 18:30:45 xxxx kernel: ata2: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x180 irq 40 lpm-pol 0
> 
> Previously, both detected ports were automatically used, and no warning was emitted.
> (hpriv->mask_port_map was 0x0)
> 
> 
> 6.14.0-rc1 bootlog
> ==================
> 
> Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
> Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
> Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1)
> Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> Feb 05 18:36:40 xxxx kernel: scsi host0: ahci-mvebu
> Feb 05 18:36:40 xxxx kernel: scsi host1: ahci-mvebu
> Feb 05 18:36:40 xxxx kernel: ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
> Feb 05 18:36:40 xxxx kernel: ata2: DUMMY
> 
> Now, hpriv->mask_port_map is forced to 0x1, resulting in a kernel warning, and no more ata2 available.
> In my particular case it is not a big deal, since nothing is connected to the 2nd port, and I can live with this obscure warning.
> 
> But that might not be the case for everybody?

Can you try this to see if it restores the probe for the second port:

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 53b2c7719dc5..91d44302eac9 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -651,8 +651,6 @@ 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;
Klaus Kudielka Feb. 6, 2025, 6:42 p.m. UTC | #5
On Thu, 2025-02-06 at 10:34 +0900, Damien Le Moal wrote:
> 
> Can you try this to see if it restores the probe for the second port:
> 
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 53b2c7719dc5..91d44302eac9 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -651,8 +651,6 @@ 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;
> 
> 

Yes, it does.

6.14.0-rc1 (plus patch above) bootlog
=====================================

Feb 06 19:31:51 spare kernel: ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
Feb 06 19:31:51 spare kernel: ahci-mvebu f10a8000.sata: 2/2 ports implemented (port mask 0x3)
Feb 06 19:31:51 spare kernel: ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
Feb 06 19:31:51 spare kernel: scsi host0: ahci-mvebu
Feb 06 19:31:51 spare kernel: scsi host1: ahci-mvebu
Feb 06 19:31:51 spare kernel: ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
Feb 06 19:31:51 spare kernel: ata2: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x180 irq 40 lpm-pol 0


Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
Josua Mayer Feb. 7, 2025, 11:22 a.m. UTC | #6
Am 06.02.25 um 19:42 schrieb Klaus Kudielka:
> On Thu, 2025-02-06 at 10:34 +0900, Damien Le Moal wrote:
>> Can you try this to see if it restores the probe for the second port:
>>
>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>> index 53b2c7719dc5..91d44302eac9 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -651,8 +651,6 @@ 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;
>>
>>
> Yes, it does.
>
> 6.14.0-rc1 (plus patch above) bootlog
> =====================================
>
> Feb 06 19:31:51 spare kernel: ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
> Feb 06 19:31:51 spare kernel: ahci-mvebu f10a8000.sata: 2/2 ports implemented (port mask 0x3)
> Feb 06 19:31:51 spare kernel: ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> Feb 06 19:31:51 spare kernel: scsi host0: ahci-mvebu
> Feb 06 19:31:51 spare kernel: scsi host1: ahci-mvebu
> Feb 06 19:31:51 spare kernel: ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
> Feb 06 19:31:51 spare kernel: ata2: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x180 irq 40 lpm-pol 0

Can you confirm the physical number of sata ports on your board?

Quick review of u-boot sources suggests it is only a single port,
muxed on serdes #0, while the controller has two:

https://source.denx.de/u-boot/u-boot/-/blob/v2025.01/board/CZ.NIC/turris_omnia/turris_omnia.c#L418

> Feb 05 18:36:40 xxxx kernel: ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1)
> ...
> Feb 05 18:36:40 xxxx kernel: ata2: DUMMY

This is clearly result of my patch masking only port 1.

I would be curious whether in another board that has two ports physically,
whether both of them were functional before my patch.

See e.g. Helios-4 DTS based on Armada 388 (similar Turris Omnia)
explicitly specifies two ports below the sata node:

            sata@a8000 {
                status = "okay";
                #address-cells = <1>;
                #size-cells = <0>;

                sata0: sata-port@0 {
                    reg = <0>;
                };

                sata1: sata-port@1 {
                    reg = <1>;
                };
            };

I propose that perhaps for the second port explicit node was required.
Klaus Kudielka Feb. 7, 2025, 7:45 p.m. UTC | #7
On Fri, 2025-02-07 at 11:22 +0000, Josua Mayer wrote:
> 
> Can you confirm the physical number of sata ports on your board?
> 

The second port indeed seems not wired on Turris Omnia.
If the "masking port_map 0x3 -> 0x1" kernel warning had not suddenly appeared, I would not have noticed this at all.

> I would be curious whether in another board that has two ports physically,
> whether both of them were functional before my patch.

I don't have such a board, but to me it seems the existing code was made exactly for that case.

For reference, my board later reports
  ata2: SATA link down (SStatus 0 SControl 300)
  ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)

Best regards, Klaus
Josua Mayer Feb. 8, 2025, 1:39 p.m. UTC | #8
Am 07.02.25 um 20:45 schrieb Klaus Kudielka:
> On Fri, 2025-02-07 at 11:22 +0000, Josua Mayer wrote:
>> Can you confirm the physical number of sata ports on your board?
>>
> The second port indeed seems not wired on Turris Omnia.
> If the "masking port_map 0x3 -> 0x1" kernel warning had not suddenly appeared, I would not have noticed this at all.
>
>> I would be curious whether in another board that has two ports physically,
>> whether both of them were functional before my patch.
> I don't have such a board, but to me it seems the existing code was made exactly for that case.

I have such a board and tested how it behaves with Linux 6.1.124 from Debian.

I modified the dtb removing the port subnodes from sata nodes and found
just one difference:

- [    3.225848] ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x3
  [    3.225882] ahci-mvebu f10a8000.sata: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl platform mode
  [    3.225891] ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs
...
- [    3.248678] ahci-mvebu f10e0000.sata: masking port_map 0x3 -> 0x3
  [    3.248714] ahci-mvebu f10e0000.sata: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl platform mode
  [    3.248723] ahci-mvebu f10e0000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs

So, only the masking message goes away.

When connecting drives to each port, both ports per controller were functional
contrary to my intuition.

>
> For reference, my board later reports
>   ata2: SATA link down (SStatus 0 SControl 300)
>   ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
Thanks!
diff mbox series

Patch

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index ef569eae4ce4625e92c24c7dd54e8704b9aff2c4..24c471b485ab8b43eca21909ea16cb47a2a95ee1 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -288,6 +288,9 @@  static unsigned int brcm_ahci_read_id(struct ata_device *dev,
 
 	/* Re-initialize and calibrate the PHY */
 	for (i = 0; i < hpriv->nports; 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 1ec35778903ddc28aebdab7d72676a31e757e56f..f2e20ed11ec70f48cb5f2c12812996bb99872aa5 100644
--- a/drivers/ata/ahci_ceva.c
+++ b/drivers/ata/ahci_ceva.c
@@ -206,6 +206,9 @@  static int ceva_ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 		goto disable_clks;
 
 	for (i = 0; i < hpriv->nports; i++) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_rsts;
@@ -215,6 +218,9 @@  static int ceva_ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 	ahci_platform_deassert_rsts(hpriv);
 
 	for (i = 0; i < hpriv->nports; i++) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		rc = phy_power_on(hpriv->phys[i]);
 		if (rc) {
 			phy_exit(hpriv->phys[i]);
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 7a8064520a35bd86a1fa82d05c1ecaa8a81b7010..b68777841f7a544b755a16a633b1a2a47b90da08 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -49,6 +49,9 @@  int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 	int rc, i;
 
 	for (i = 0; i < hpriv->nports; i++) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_phys;
@@ -70,6 +73,9 @@  int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 
 disable_phys:
 	while (--i >= 0) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		phy_power_off(hpriv->phys[i]);
 		phy_exit(hpriv->phys[i]);
 	}
@@ -88,6 +94,9 @@  void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 	int i;
 
 	for (i = 0; i < hpriv->nports; i++) {
+		if (!(hpriv->mask_port_map & (1 << i)))
+			continue;
+
 		phy_power_off(hpriv->phys[i]);
 		phy_exit(hpriv->phys[i]);
 	}
@@ -432,6 +441,20 @@  static int ahci_platform_get_firmware(struct ahci_host_priv *hpriv,
 	return 0;
 }
 
+static u32 ahci_platform_find_max_port_id(struct device *dev)
+{
+	u32 max_port = 0;
+
+	for_each_child_of_node_scoped(dev->of_node, child) {
+		u32 port;
+
+		if (!of_property_read_u32(child, "reg", &port))
+			max_port = max(max_port, port);
+	}
+
+	return max_port;
+}
+
 /**
  * ahci_platform_get_resources - Get platform resources
  * @pdev: platform device to get resources for
@@ -458,6 +481,7 @@  struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
 	u32 mask_port_map = 0;
+	u32 max_port;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
 		return ERR_PTR(-ENOMEM);
@@ -549,15 +573,17 @@  struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		goto err_out;
 	}
 
+	/* find maximum port id for allocating structures */
+	max_port = ahci_platform_find_max_port_id(dev);
 	/*
-	 * If no sub-node was found, we still need to set nports to
-	 * one in order to be able to use the
+	 * Set nports according to maximum port id. Clamp at
+	 * AHCI_MAX_PORTS, warning message for invalid port id
+	 * is generated later.
+	 * When DT has no sub-nodes max_port is 0, nports is 1,
+	 * 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->nports = min(AHCI_MAX_PORTS, max_port + 1);
 
 	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
 	if (!hpriv->phys) {
@@ -625,6 +651,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;