* Re: [PATCH v2] ata: libahci_platform: Do not set mask_port_map when not needed
2025-02-07 23:29 ` [PATCH v2] ata: libahci_platform: Do not set mask_port_map when not needed Damien Le Moal
@ 2025-02-08 13:42 ` Josua Mayer
2025-02-10 10:55 ` Niklas Cassel
2025-02-25 9:34 ` Marek Szyprowski
2 siblings, 0 replies; 5+ messages in thread
From: Josua Mayer @ 2025-02-08 13:42 UTC (permalink / raw)
To: Damien Le Moal, linux-ide@vger.kernel.org, Niklas Cassel; +Cc: Klaus Kudielka
Am 08.02.25 um 00:29 schrieb Damien Le Moal:
> Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive
> port numbers") modified ahci_platform_get_resources() to allow
> identifying the ports of a controller that are defined as child nodes of
> the controller node in order to support non-consecutive port numbers (as
> defined by the platform device tree).
>
> However, this commit also erroneously sets bit 0 of
> hpriv->mask_port_map when the platform devices tree does not define port
> child nodes, to match the fact that the temporary default number of
> ports used in that case is 1 (which is also consistent with the fact
> that only index 0 of hpriv->phys[] is initialized with the call to
> ahci_platform_get_phy(). But doing so causes ahci_platform_init_host()
> to initialize and probe only the first port, even if this function
> determines that the controller has in fact multiple ports using the
> capability register of the controller (through a call to
> ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada
> 385 SoC) with the second port declared as "dummy":
>
> ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
> ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
> ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1)
> ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs
> scsi host0: ahci-mvebu
> scsi host1: ahci-mvebu
> ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
> ata2: DUMMY
>
> Fix this issue by removing setting bit 0 of hpriv->mask_port_map when
> the platform device tree does not define port child nodes.
>
> Reported-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> Fixes: 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
> Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
> Changes from v1:
> - Improved commit message (fixed typos, added example and removed
> cc-stable tag).
>
> drivers/ata/libahci_platform.c | 2 --
> 1 file changed, 2 deletions(-)
>
> 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;
After testing on Armada 388 Helios-4 board with and without port subnodes,
I can confirm that before my patch and without port subnodes
both controller ports were fully operational.
Therefore I believe restoring the original behaviour here is correct.
Acked-by: Josua Mayer <josua@solid-run.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] ata: libahci_platform: Do not set mask_port_map when not needed
2025-02-07 23:29 ` [PATCH v2] ata: libahci_platform: Do not set mask_port_map when not needed Damien Le Moal
2025-02-08 13:42 ` Josua Mayer
@ 2025-02-10 10:55 ` Niklas Cassel
2025-02-25 9:34 ` Marek Szyprowski
2 siblings, 0 replies; 5+ messages in thread
From: Niklas Cassel @ 2025-02-10 10:55 UTC (permalink / raw)
To: linux-ide, Damien Le Moal; +Cc: Klaus Kudielka, Josua Mayer
On Sat, 08 Feb 2025 08:29:15 +0900, Damien Le Moal wrote:
> Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive
> port numbers") modified ahci_platform_get_resources() to allow
> identifying the ports of a controller that are defined as child nodes of
> the controller node in order to support non-consecutive port numbers (as
> defined by the platform device tree).
>
> However, this commit also erroneously sets bit 0 of
> hpriv->mask_port_map when the platform devices tree does not define port
> child nodes, to match the fact that the temporary default number of
> ports used in that case is 1 (which is also consistent with the fact
> that only index 0 of hpriv->phys[] is initialized with the call to
> ahci_platform_get_phy(). But doing so causes ahci_platform_init_host()
> to initialize and probe only the first port, even if this function
> determines that the controller has in fact multiple ports using the
> capability register of the controller (through a call to
> ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada
> 385 SoC) with the second port declared as "dummy":
>
> [...]
Applied to libata/linux.git (for-6.14), thanks!
[1/1] ata: libahci_platform: Do not set mask_port_map when not needed
https://git.kernel.org/libata/linux/c/2c202e6c
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] ata: libahci_platform: Do not set mask_port_map when not needed
2025-02-07 23:29 ` [PATCH v2] ata: libahci_platform: Do not set mask_port_map when not needed Damien Le Moal
2025-02-08 13:42 ` Josua Mayer
2025-02-10 10:55 ` Niklas Cassel
@ 2025-02-25 9:34 ` Marek Szyprowski
2025-02-25 14:24 ` Niklas Cassel
2 siblings, 1 reply; 5+ messages in thread
From: Marek Szyprowski @ 2025-02-25 9:34 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Niklas Cassel; +Cc: Klaus Kudielka, Josua Mayer
Hi Damien,
On 08.02.2025 00:29, Damien Le Moal wrote:
> Commit 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive
> port numbers") modified ahci_platform_get_resources() to allow
> identifying the ports of a controller that are defined as child nodes of
> the controller node in order to support non-consecutive port numbers (as
> defined by the platform device tree).
>
> However, this commit also erroneously sets bit 0 of
> hpriv->mask_port_map when the platform devices tree does not define port
> child nodes, to match the fact that the temporary default number of
> ports used in that case is 1 (which is also consistent with the fact
> that only index 0 of hpriv->phys[] is initialized with the call to
> ahci_platform_get_phy(). But doing so causes ahci_platform_init_host()
> to initialize and probe only the first port, even if this function
> determines that the controller has in fact multiple ports using the
> capability register of the controller (through a call to
> ahci_nr_ports()). This can be seen with the ahci_mvebu driver (Armada
> 385 SoC) with the second port declared as "dummy":
>
> ahci-mvebu f10a8000.sata: masking port_map 0x3 -> 0x1
> ahci-mvebu f10a8000.sata: AHCI vers 0001.0000, 32 command slots, 6 Gbps, platform mode
> ahci-mvebu f10a8000.sata: 1/2 ports implemented (port mask 0x1)
> ahci-mvebu f10a8000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs
> scsi host0: ahci-mvebu
> scsi host1: ahci-mvebu
> ata1: SATA max UDMA/133 mmio [mem 0xf10a8000-0xf10a9fff] port 0x100 irq 40 lpm-pol 0
> ata2: DUMMY
>
> Fix this issue by removing setting bit 0 of hpriv->mask_port_map when
> the platform device tree does not define port child nodes.
>
> Reported-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> Fixes: 8c87215dd3a2 ("ata: libahci_platform: support non-consecutive port numbers")
> Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
This patch landed yesterday in linux-next as commit 2c202e6c4f4d ("ata:
libahci_platform: Do not set mask_port_map when not needed").
Unfortunately it breaks SATA operation on legacy ARM Exynos5250 Arndale
board (arch/arm/boot/dts/samsung/exynos5250-arndale.dts). I wonder if
this patch is really correct as if I understand code correctly, it
disables support for any legacy of-based, single port.
Here is the log from that board:
[ 0.937868] libata version 3.00 loaded.
[ 2.322611] ahci-dwc 122f0000.sata: supply ahci not found, using
dummy regulator
[ 2.329389] ahci-dwc 122f0000.sata: supply phy not found, using dummy
regulator
[ 2.336745] ahci-dwc 122f0000.sata: supply target not found, using
dummy regulator
[ 2.344254] ahci-dwc 122f0000.sata: forcing port_map 0x0 -> 0x1
[ 2.349517] ahci-dwc 122f0000.sata: AHCI vers 0001.0300, 32 command
slots, 6 Gbps, platform mode
[ 2.358128] ahci-dwc 122f0000.sata: 1/1 ports implemented (port mask
0x1)
[ 2.364921] ahci-dwc 122f0000.sata: flags: ncq sntf pm led clo only
pmp pio slum part ccc apst
[ 2.385620] ata1: SATA max UDMA/133 mmio [mem 0x122f0000-0x122f01fe]
port 0x100 irq 101 lpm-pol 0
[ 2.716379] ata1: SATA link down (SStatus 0 SControl 300)
with the $subject patch reverted:
[ 0.938268] libata version 3.00 loaded.
[ 2.283918] ahci-dwc 122f0000.sata: supply ahci not found, using
dummy regulator
[ 2.290714] ahci-dwc 122f0000.sata: supply phy not found, using dummy
regulator
[ 2.298064] ahci-dwc 122f0000.sata: supply target not found, using
dummy regulator
[ 2.312275] ahci-dwc 122f0000.sata: forcing port_map 0x0 -> 0x1
[ 2.316847] ahci-dwc 122f0000.sata: masking port_map 0x1 -> 0x1
[ 2.322791] ahci-dwc 122f0000.sata: AHCI vers 0001.0300, 32 command
slots, 6 Gbps, platform mode
[ 2.331464] ahci-dwc 122f0000.sata: 1/1 ports implemented (port mask
0x1)
[ 2.338258] ahci-dwc 122f0000.sata: flags: ncq sntf pm led clo only
pmp pio slum part ccc apst
[ 2.359896] ata1: SATA max UDMA/133 mmio [mem 0x122f0000-0x122f01fe]
port 0x100 irq 101 lpm-pol 0
[ 2.687374] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[ 2.744673] ata1.00: HPA detected: current 117229295, native 117231408
[ 2.757119] ata1.00: ATA-8: Corsair CSSD-F60GB2, 1.1, max UDMA/133
[ 2.772062] ata1.00: 117229295 sectors, multi 1: LBA48 NCQ (depth 32)
[ 2.834520] ata1.00: configured for UDMA/133
> ---
> Changes from v1:
> - Improved commit message (fixed typos, added example and removed
> cc-stable tag).
>
> drivers/ata/libahci_platform.c | 2 --
> 1 file changed, 2 deletions(-)
>
> 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;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 5+ messages in thread