diff mbox series

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

Message ID 1721008436-24288-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 15, 2024, 1:53 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

Damien Le Moal July 15, 2024, 10:01 p.m. UTC | #1
On 7/15/24 10:53, 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>
> ---
>  drivers/ata/ahci_imx.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index e94c0fdea2260..12d69a6429b6a 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_VEND_PTC		= 0xc8,
> +	IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK	= 0x7f,

Please use GENMASK() macro to define this.

> +	IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM	= 0x29,

Here too. And I do not see the point of the "NEW" in the macro name.

Also, what is "_VEND_" supposed to mean in the macro names ? "Vendor" ?
If that is the case, remove that too, which will give you shorter macro names.
If not, then spell it out because that is not clear.

>  };
>  
>  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_VEND_PTC);
> +	val &= ~IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK;
> +	val |= IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM;
> +	writel(val, hpriv->mmio + IMX8QM_SATA_AHCI_VEND_PTC);
> +
>  	return 0;
>  
>  err_sata_phy_exit:
Hongxing Zhu July 16, 2024, 2:52 a.m. UTC | #2
> -----Original Message-----
> From: Damien Le Moal <dlemoal@kernel.org>
> Sent: 2024年7月16日 6:01
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; tj@kernel.org; cassel@kernel.org;
> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com
> Cc: linux-ide@vger.kernel.org; stable@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev;
> kernel@pengutronix.de
> Subject: Re: [PATCH v2 3/4] ata: ahci_imx: Enlarge RX water mark for i.MX8QM
> SATA
> 
> On 7/15/24 10:53, 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>
> > ---
> >  drivers/ata/ahci_imx.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index
> > e94c0fdea2260..12d69a6429b6a 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_VEND_PTC		= 0xc8,
> > +	IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK	= 0x7f,
> 
> Please use GENMASK() macro to define this.
Thanks for your comments.
Okay.
> 
> > +	IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM	= 0x29,
> 
> Here too. And I do not see the point of the "NEW" in the macro name.
> 
Okay, "NEW" would be removed.
> Also, what is "_VEND_" supposed to mean in the macro names ? "Vendor" ?
> If that is the case, remove that too, which will give you shorter macro names.
> If not, then spell it out because that is not clear.
Yes, you're right.
"_VEND_" means "Vendor". I would remove them later.

Best Regards
Richard Zhu

> 
> >  };
> >
> >  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_VEND_PTC);
> > +	val &= ~IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK;
> > +	val |= IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM;
> > +	writel(val, hpriv->mmio + IMX8QM_SATA_AHCI_VEND_PTC);
> > +
> >  	return 0;
> >
> >  err_sata_phy_exit:
> 
> --
> Damien Le Moal
> Western Digital Research
diff mbox series

Patch

diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index e94c0fdea2260..12d69a6429b6a 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_VEND_PTC		= 0xc8,
+	IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK	= 0x7f,
+	IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM	= 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_VEND_PTC);
+	val &= ~IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK;
+	val |= IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM;
+	writel(val, hpriv->mmio + IMX8QM_SATA_AHCI_VEND_PTC);
+
 	return 0;
 
 err_sata_phy_exit: