| Message ID | 1721099895-26098-4-git-send-email-hongxing.zhu@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refine i.MX8QM SATA based on generic PHY callbacks | expand |
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
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 --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:
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(+)