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

[Hans de Goede <hdegoede@redhat.com>]

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,