RE: [PATCH v1 3/4] ata: ahci_imx: Enlarge RX water mark for i.MX8QM SATA

[Hongxing Zhu]

> -----Original Message-----
> From: Damien Le Moal <dlemoal@xxxxxxxxxx>
> Sent: 2024年7月11日 18:08
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>; tj@xxxxxxxxxx; cassel@xxxxxxxxxx;
> robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx
> Cc: linux-ide@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 3/4] ata: ahci_imx: Enlarge RX water mark for i.MX8QM
> SATA
> 
> On 7/11/24 17:11, 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 vaulue 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.
> 
> 2 remarks:
> 
> 1) this seems to be a bug/problem fix, so this likely needs a backport to stable
> (Cc: stable tag) and a Fixes tag.
> 
Okay, I see. Would CC stable kernel later. Thanks.

> 2) What are these magic values 0x20 and 0x29 ? Where are they defined (SoC
> specs) ? Can you define the new value using a macro with a self-descriptive
> name ?
This register is one vender specific register, and defined in the SoC
 RM document. The default value of RXWM setting is 0x20.
Enlarge it to 0x29 recommended by the IP designer to fix the issue.
So, the RXWM is reconfigured from default value 0x20 to 0x29 here.

How about using " IMX8QM_SATA_AHCI_VEND_PTC_NEWRXWM " 
to replace " IMX8QM_SATA_AHCI_VEND_PTC_RXWM "?
I'm not sure the new name is more self-descriptive or not.
Can you help to recommend one?

Any recommends are appreciated.

Best Regards
Richard Zhu
> 
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@xxxxxxx>
> > ---
> >  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
> > 0e9fddd02ee5f..9147cd14f587e 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_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_VEND_PTC);
> > +	val &= ~IMX8QM_SATA_AHCI_VEND_PTC_RXWM_MASK;
> > +	val |= IMX8QM_SATA_AHCI_VEND_PTC_RXWM;
> > +	writel(val, hpriv->mmio + IMX8QM_SATA_AHCI_VEND_PTC);
> > +
> >  	return 0;
> >
> >  err_sata_phy_power_on:
> 
> --
> Damien Le Moal
> Western Digital Research