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

[Damien Le Moal]

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;


-- 
Damien Le Moal
Western Digital Research