diff mbox series

[v3,3/4] ata: ahci_imx: Enlarge RX water mark for i.MX8QM SATA

Message ID 1721099895-26098-4-git-send-email-hongxing.zhu@nxp.com (mailing list archive)
State Superseded
Headers show
Series Refine i.MX8QM SATA based on generic PHY callbacks | expand

Commit Message

Hongxing Zhu July 16, 2024, 3:18 a.m. UTC
The RXWM(RxWaterMark) sets the minimum number of free location within
the RX FIFO before the watermark is exceeded which in turn will cause
the Transport Layer to instruct the Link Layer to transmit HOLDS to
the transmitting end.

Based on the default RXWM value 0x20, RX FIFO overflow might be
observed on i.MX8QM MEK board, when some Gen3 SATA disks are used.

The FIFO overflow will result in CRC error, internal error and protocol
error, then the SATA link is not stable anymore.

To fix this issue, enlarge RX water mark setting from 0x20 to 0x29.

Fixes: 027fa4dee935 ("ahci: imx: add the imx8qm ahci sata support")
Cc: stable@vger.kernel.org
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/ata/ahci_imx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Niklas Cassel July 17, 2024, 6:17 p.m. UTC | #1
Hello Richard,

On Tue, Jul 16, 2024 at 11:18:14AM +0800, Richard Zhu wrote:
> The RXWM(RxWaterMark) sets the minimum number of free location within
> the RX FIFO before the watermark is exceeded which in turn will cause
> the Transport Layer to instruct the Link Layer to transmit HOLDS to
> the transmitting end.
> 
> Based on the default RXWM value 0x20, RX FIFO overflow might be
> observed on i.MX8QM MEK board, when some Gen3 SATA disks are used.
> 
> The FIFO overflow will result in CRC error, internal error and protocol
> error, then the SATA link is not stable anymore.
> 
> To fix this issue, enlarge RX water mark setting from 0x20 to 0x29.
> 
> Fixes: 027fa4dee935 ("ahci: imx: add the imx8qm ahci sata support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---

Looking at the title of this patch:
"ahci_imx: Enlarge RX water mark for i.MX8QM SATA"

This suggests that this fix is only needed for i.MX8QM.

Support for i.MX8QM was added to the device tree binding in patch 1/4 in
this series.

Doing a git grep in linux-next gives the following result:

$ git grep fsl,imx8qm-ahci linux-next/master
linux-next/master:drivers/ata/ahci_imx.c:       { .compatible = "fsl,imx8qm-ahci", .data = (void *)AHCI_IMX8QM },


This is interesting for two reasons:
1) drivers/ata/ahci_imx.c already has support for this compatible string,
even though this compatible string does not exist in any DT binding
(in linux-next).

2) There is not a single in-tree device tree (DTS) that uses this compatible
string ....and we do not care about out of tree device trees.


Considering 2) I do NOT think that we should have
Cc: stable@vger.kernel.org on this... we shouldn't just backport random driver
fixes is there are no in-tree users of this compatible string.

So I suggest that:
-Drop the CC: stable.
-I actually think that it is better that you drop the Fixes tag too, because if
you keep it, the stable bots will automatically select this for backporting,
and then we will need to reply and say that this should not be backported, so
better to avoid adding the Fixes tag in the first place.
(Since there are no users of this compatible string, there is nothing that is
broken, so there is nothing to fix.)


Damien, when applying this patch, I suggest that we apply it to for-6.12
together with the rest of the series (instead of applying it to
for-6.11-fixes).


Kind regards,
Niklas
Damien Le Moal July 17, 2024, 11:43 p.m. UTC | #2
On 7/18/24 3:17 AM, Niklas Cassel wrote:
> Hello Richard,
> 
> On Tue, Jul 16, 2024 at 11:18:14AM +0800, Richard Zhu wrote:
>> The RXWM(RxWaterMark) sets the minimum number of free location within
>> the RX FIFO before the watermark is exceeded which in turn will cause
>> the Transport Layer to instruct the Link Layer to transmit HOLDS to
>> the transmitting end.
>>
>> Based on the default RXWM value 0x20, RX FIFO overflow might be
>> observed on i.MX8QM MEK board, when some Gen3 SATA disks are used.
>>
>> The FIFO overflow will result in CRC error, internal error and protocol
>> error, then the SATA link is not stable anymore.
>>
>> To fix this issue, enlarge RX water mark setting from 0x20 to 0x29.
>>
>> Fixes: 027fa4dee935 ("ahci: imx: add the imx8qm ahci sata support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
>> ---
> 
> Looking at the title of this patch:
> "ahci_imx: Enlarge RX water mark for i.MX8QM SATA"
> 
> This suggests that this fix is only needed for i.MX8QM.
> 
> Support for i.MX8QM was added to the device tree binding in patch 1/4 in
> this series.
> 
> Doing a git grep in linux-next gives the following result:
> 
> $ git grep fsl,imx8qm-ahci linux-next/master
> linux-next/master:drivers/ata/ahci_imx.c:       { .compatible = "fsl,imx8qm-ahci", .data = (void *)AHCI_IMX8QM },
> 
> 
> This is interesting for two reasons:
> 1) drivers/ata/ahci_imx.c already has support for this compatible string,
> even though this compatible string does not exist in any DT binding
> (in linux-next).
> 
> 2) There is not a single in-tree device tree (DTS) that uses this compatible
> string ....and we do not care about out of tree device trees.
> 
> 
> Considering 2) I do NOT think that we should have
> Cc: stable@vger.kernel.org on this... we shouldn't just backport random driver
> fixes is there are no in-tree users of this compatible string.
> 
> So I suggest that:
> -Drop the CC: stable.
> -I actually think that it is better that you drop the Fixes tag too, because if
> you keep it, the stable bots will automatically select this for backporting,
> and then we will need to reply and say that this should not be backported, so
> better to avoid adding the Fixes tag in the first place.
> (Since there are no users of this compatible string, there is nothing that is
> broken, so there is nothing to fix.)
> 
> 
> Damien, when applying this patch, I suggest that we apply it to for-6.12
> together with the rest of the series (instead of applying it to
> for-6.11-fixes).

It was me who asked for the Fixes and Cc-stable tags, but I had not checked
that the compatible is not being used in any DT. So good catch.
I will apply everything to for-6.12.

> 
> 
> Kind regards,
> Niklas
diff mbox series

Patch

diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index e94c0fdea2260..b6ad0fe5fbbcc 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -45,6 +45,10 @@  enum {
 	/* Clock Reset Register */
 	IMX_CLOCK_RESET				= 0x7f3f,
 	IMX_CLOCK_RESET_RESET			= 1 << 0,
+	/* IMX8QM SATA specific control registers */
+	IMX8QM_SATA_AHCI_PTC			= 0xc8,
+	IMX8QM_SATA_AHCI_PTC_RXWM_MASK		= GENMASK(6, 0),
+	IMX8QM_SATA_AHCI_PTC_RXWM		= 0x29,
 };
 
 enum ahci_imx_type {
@@ -466,6 +470,12 @@  static int imx8_sata_enable(struct ahci_host_priv *hpriv)
 	phy_power_off(imxpriv->cali_phy0);
 	phy_exit(imxpriv->cali_phy0);
 
+	/* RxWaterMark setting */
+	val = readl(hpriv->mmio + IMX8QM_SATA_AHCI_PTC);
+	val &= ~IMX8QM_SATA_AHCI_PTC_RXWM_MASK;
+	val |= IMX8QM_SATA_AHCI_PTC_RXWM;
+	writel(val, hpriv->mmio + IMX8QM_SATA_AHCI_PTC);
+
 	return 0;
 
 err_sata_phy_exit: