[PATCH v3 0/4] Refine i.MX8QM SATA based on generic PHY callbacks

[PATCH v3 0/4] Refine i.MX8QM SATA based on generic PHY callbacks

[Richard Zhu]

V3 main changes:
- Use GENMASK() macro to define the _MASK.
- Refine the macro names.

V2 main changes:
- Add Rob's reviewed-by in the binding patch.
- Re-name the error out lables and new RXWM macro more descriptive.
- In #3 patch, add one fix tag, and CC stable kernel.

Based on i.MX8QM HSIO PHY driver, refine i.MX8QM SATA driver by using PHY
interface.

[PATCH v3 1/4] dt-bindings: ata: Add i.MX8QM AHCI compatible string
[PATCH v3 2/4] ata: ahci_imx: Clean up code by using i.MX8Q HSIO PHY
[PATCH v3 3/4] ata: ahci_imx: Enlarge RX water mark for i.MX8QM SATA
[PATCH v3 4/4] ata: ahci_imx: Correct the email address

Documentation/devicetree/bindings/ata/imx-sata.yaml |  47 +++++++++++
drivers/ata/ahci_imx.c                              | 406 ++++++++++++++++++++++++-----------------------------------------------------------------
2 files changed, 155 insertions(+), 298 deletions(-)

[PATCH v3 1/4] dt-bindings: ata: Add i.MX8QM AHCI compatible string

[Richard Zhu]

Add i.MX8QM AHCI "fsl,imx8qm-ahci" compatible strings.

i.MX8QM AHCI SATA doesn't require AHB clock rate to set the vendor
specified TIMER1MS register. ahb clock is not required by i.MX8QM AHCI.

Update the description of clocks in the dt-binding accordingly.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../devicetree/bindings/ata/imx-sata.yaml     | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/imx-sata.yaml b/Documentation/devicetree/bindings/ata/imx-sata.yaml
index 68ffb97ddc9b2..f4eb3550a0960 100644
--- a/Documentation/devicetree/bindings/ata/imx-sata.yaml
+++ b/Documentation/devicetree/bindings/ata/imx-sata.yaml
@@ -19,6 +19,7 @@ properties:
       - fsl,imx53-ahci
       - fsl,imx6q-ahci
       - fsl,imx6qp-ahci
+      - fsl,imx8qm-ahci
 
   reg:
     maxItems: 1
@@ -27,12 +28,14 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 2
     items:
       - description: sata clock
       - description: sata reference clock
       - description: ahb clock
 
   clock-names:
+    minItems: 2
     items:
       - const: sata
       - const: sata_ref
@@ -58,6 +61,25 @@ properties:
     $ref: /schemas/types.yaml#/definitions/flag
     description: if present, disable spread-spectrum clocking on the SATA link.
 
+  phys:
+    items:
+      - description: phandle to SATA PHY.
+          Since "REXT" pin is only present for first lane of i.MX8QM PHY, it's
+          calibration result will be stored, passed through second lane, and
+          shared with all three lanes PHY. The first two lanes PHY are used as
+          calibration PHYs, although only the third lane PHY is used by SATA.
+      - description: phandle to the first lane PHY of i.MX8QM.
+      - description: phandle to the second lane PHY of i.MX8QM.
+
+  phy-names:
+    items:
+      - const: sata-phy
+      - const: cali-phy0
+      - const: cali-phy1
+
+  power-domains:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -65,6 +87,31 @@ required:
   - clocks
   - clock-names
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx53-ahci
+              - fsl,imx6q-ahci
+              - fsl,imx6qp-ahci
+    then:
+      properties:
+        clock-names:
+          minItems: 3
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - fsl,imx8qm-ahci
+    then:
+      properties:
+        clock-names:
+          minItems: 2
+
 additionalProperties: false
 
 examples:
-- 
2.37.1

Re: [PATCH v3 1/4] dt-bindings: ata: Add i.MX8QM AHCI compatible string

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1

Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree.
Subject: [PATCH v3 1/4] dt-bindings: ata: Add i.MX8QM AHCI compatible string
Link: https://lore.kernel.org/stable/1721099895-26098-2-git-send-email-hongxing.zhu%40nxp.com

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 2/4] ata: ahci_imx: Clean up code by using i.MX8Q HSIO PHY driver

[Richard Zhu]

Clean up code by using PHY interface.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/ata/ahci_imx.c | 396 ++++++++++-------------------------------
 1 file changed, 98 insertions(+), 298 deletions(-)

diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index cb768f66f0a70..e94c0fdea2260 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -19,6 +19,7 @@
 #include <linux/libata.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <linux/phy/phy.h>
 #include <linux/thermal.h>
 #include "ahci.h"
 
@@ -44,42 +45,6 @@ enum {
 	/* Clock Reset Register */
 	IMX_CLOCK_RESET				= 0x7f3f,
 	IMX_CLOCK_RESET_RESET			= 1 << 0,
-	/* IMX8QM HSIO AHCI definitions */
-	IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET	= 0x03,
-	IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET	= 0x09,
-	IMX8QM_SATA_PHY_IMPED_RATIO_85OHM	= 0x6c,
-	IMX8QM_LPCG_PHYX2_OFFSET		= 0x00000,
-	IMX8QM_CSR_PHYX2_OFFSET			= 0x90000,
-	IMX8QM_CSR_PHYX1_OFFSET			= 0xa0000,
-	IMX8QM_CSR_PHYX_STTS0_OFFSET		= 0x4,
-	IMX8QM_CSR_PCIEA_OFFSET			= 0xb0000,
-	IMX8QM_CSR_PCIEB_OFFSET			= 0xc0000,
-	IMX8QM_CSR_SATA_OFFSET			= 0xd0000,
-	IMX8QM_CSR_PCIE_CTRL2_OFFSET		= 0x8,
-	IMX8QM_CSR_MISC_OFFSET			= 0xe0000,
-
-	IMX8QM_LPCG_PHYX2_PCLK0_MASK		= (0x3 << 16),
-	IMX8QM_LPCG_PHYX2_PCLK1_MASK		= (0x3 << 20),
-	IMX8QM_PHY_APB_RSTN_0			= BIT(0),
-	IMX8QM_PHY_MODE_SATA			= BIT(19),
-	IMX8QM_PHY_MODE_MASK			= (0xf << 17),
-	IMX8QM_PHY_PIPE_RSTN_0			= BIT(24),
-	IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0		= BIT(25),
-	IMX8QM_PHY_PIPE_RSTN_1			= BIT(26),
-	IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1		= BIT(27),
-	IMX8QM_STTS0_LANE0_TX_PLL_LOCK		= BIT(4),
-	IMX8QM_MISC_IOB_RXENA			= BIT(0),
-	IMX8QM_MISC_IOB_TXENA			= BIT(1),
-	IMX8QM_MISC_PHYX1_EPCS_SEL		= BIT(12),
-	IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1	= BIT(24),
-	IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0	= BIT(25),
-	IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1	= BIT(28),
-	IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0	= BIT(29),
-	IMX8QM_SATA_CTRL_RESET_N		= BIT(12),
-	IMX8QM_SATA_CTRL_EPCS_PHYRESET_N	= BIT(7),
-	IMX8QM_CTRL_BUTTON_RST_N		= BIT(21),
-	IMX8QM_CTRL_POWER_UP_RST_N		= BIT(23),
-	IMX8QM_CTRL_LTSSM_ENABLE		= BIT(4),
 };
 
 enum ahci_imx_type {
@@ -95,14 +60,10 @@ struct imx_ahci_priv {
 	struct clk *sata_clk;
 	struct clk *sata_ref_clk;
 	struct clk *ahb_clk;
-	struct clk *epcs_tx_clk;
-	struct clk *epcs_rx_clk;
-	struct clk *phy_apbclk;
-	struct clk *phy_pclk0;
-	struct clk *phy_pclk1;
-	void __iomem *phy_base;
-	struct gpio_desc *clkreq_gpiod;
 	struct regmap *gpr;
+	struct phy *sata_phy;
+	struct phy *cali_phy0;
+	struct phy *cali_phy1;
 	bool no_device;
 	bool first_time;
 	u32 phy_params;
@@ -450,201 +411,73 @@ ATTRIBUTE_GROUPS(fsl_sata_ahci);
 
 static int imx8_sata_enable(struct ahci_host_priv *hpriv)
 {
-	u32 val, reg;
-	int i, ret;
+	u32 val;
+	int ret;
 	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 	struct device *dev = &imxpriv->ahci_pdev->dev;
 
-	/* configure the hsio for sata */
-	ret = clk_prepare_enable(imxpriv->phy_pclk0);
-	if (ret < 0) {
-		dev_err(dev, "can't enable phy_pclk0.\n");
+	/*
+	 * Since "REXT" pin is only present for first lane of i.MX8QM
+	 * PHY, its calibration results will be stored, passed through
+	 * to the second lane PHY, and shared with all three lane PHYs.
+	 *
+	 * Initialize the first two lane PHYs here, although only the
+	 * third lane PHY is used by SATA.
+	 */
+	ret = phy_init(imxpriv->cali_phy0);
+	if (ret) {
+		dev_err(dev, "cali PHY init failed\n");
 		return ret;
 	}
-	ret = clk_prepare_enable(imxpriv->phy_pclk1);
-	if (ret < 0) {
-		dev_err(dev, "can't enable phy_pclk1.\n");
-		goto disable_phy_pclk0;
+	ret = phy_power_on(imxpriv->cali_phy0);
+	if (ret) {
+		dev_err(dev, "cali PHY power on failed\n");
+		goto err_cali_phy0_exit;
 	}
-	ret = clk_prepare_enable(imxpriv->epcs_tx_clk);
-	if (ret < 0) {
-		dev_err(dev, "can't enable epcs_tx_clk.\n");
-		goto disable_phy_pclk1;
+	ret = phy_init(imxpriv->cali_phy1);
+	if (ret) {
+		dev_err(dev, "cali PHY1 init failed\n");
+		goto err_cali_phy0_off;
 	}
-	ret = clk_prepare_enable(imxpriv->epcs_rx_clk);
-	if (ret < 0) {
-		dev_err(dev, "can't enable epcs_rx_clk.\n");
-		goto disable_epcs_tx_clk;
+	ret = phy_power_on(imxpriv->cali_phy1);
+	if (ret) {
+		dev_err(dev, "cali PHY1 power on failed\n");
+		goto err_cali_phy1_exit;
 	}
-	ret = clk_prepare_enable(imxpriv->phy_apbclk);
-	if (ret < 0) {
-		dev_err(dev, "can't enable phy_apbclk.\n");
-		goto disable_epcs_rx_clk;
+	ret = phy_init(imxpriv->sata_phy);
+	if (ret) {
+		dev_err(dev, "sata PHY init failed\n");
+		goto err_cali_phy1_off;
 	}
-	/* Configure PHYx2 PIPE_RSTN */
-	regmap_read(imxpriv->gpr, IMX8QM_CSR_PCIEA_OFFSET +
-			IMX8QM_CSR_PCIE_CTRL2_OFFSET, &val);
-	if ((val & IMX8QM_CTRL_LTSSM_ENABLE) == 0) {
-		/* The link of the PCIEA of HSIO is down */
-		regmap_update_bits(imxpriv->gpr,
-				IMX8QM_CSR_PHYX2_OFFSET,
-				IMX8QM_PHY_PIPE_RSTN_0 |
-				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0,
-				IMX8QM_PHY_PIPE_RSTN_0 |
-				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0);
+	ret = phy_set_mode(imxpriv->sata_phy, PHY_MODE_SATA);
+	if (ret) {
+		dev_err(dev, "unable to set SATA PHY mode\n");
+		goto err_sata_phy_exit;
 	}
-	regmap_read(imxpriv->gpr, IMX8QM_CSR_PCIEB_OFFSET +
-			IMX8QM_CSR_PCIE_CTRL2_OFFSET, &reg);
-	if ((reg & IMX8QM_CTRL_LTSSM_ENABLE) == 0) {
-		/* The link of the PCIEB of HSIO is down */
-		regmap_update_bits(imxpriv->gpr,
-				IMX8QM_CSR_PHYX2_OFFSET,
-				IMX8QM_PHY_PIPE_RSTN_1 |
-				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1,
-				IMX8QM_PHY_PIPE_RSTN_1 |
-				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1);
-	}
-	if (((reg | val) & IMX8QM_CTRL_LTSSM_ENABLE) == 0) {
-		/* The links of both PCIA and PCIEB of HSIO are down */
-		regmap_update_bits(imxpriv->gpr,
-				IMX8QM_LPCG_PHYX2_OFFSET,
-				IMX8QM_LPCG_PHYX2_PCLK0_MASK |
-				IMX8QM_LPCG_PHYX2_PCLK1_MASK,
-				0);
-	}
-
-	/* set PWR_RST and BT_RST of csr_pciea */
-	val = IMX8QM_CSR_PCIEA_OFFSET + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
-	regmap_update_bits(imxpriv->gpr,
-			val,
-			IMX8QM_CTRL_BUTTON_RST_N,
-			IMX8QM_CTRL_BUTTON_RST_N);
-	regmap_update_bits(imxpriv->gpr,
-			val,
-			IMX8QM_CTRL_POWER_UP_RST_N,
-			IMX8QM_CTRL_POWER_UP_RST_N);
-
-	/* PHYX1_MODE to SATA */
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_PHYX1_OFFSET,
-			IMX8QM_PHY_MODE_MASK,
-			IMX8QM_PHY_MODE_SATA);
-
-	/*
-	 * BIT0 RXENA 1, BIT1 TXENA 0
-	 * BIT12 PHY_X1_EPCS_SEL 1.
-	 */
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_MISC_OFFSET,
-			IMX8QM_MISC_IOB_RXENA,
-			IMX8QM_MISC_IOB_RXENA);
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_MISC_OFFSET,
-			IMX8QM_MISC_IOB_TXENA,
-			0);
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_MISC_OFFSET,
-			IMX8QM_MISC_PHYX1_EPCS_SEL,
-			IMX8QM_MISC_PHYX1_EPCS_SEL);
-	/*
-	 * It is possible, for PCIe and SATA are sharing
-	 * the same clock source, HPLL or external oscillator.
-	 * When PCIe is in low power modes (L1.X or L2 etc),
-	 * the clock source can be turned off. In this case,
-	 * if this clock source is required to be toggling by
-	 * SATA, then SATA functions will be abnormal.
-	 * Set the override here to avoid it.
-	 */
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_MISC_OFFSET,
-			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 |
-			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 |
-			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 |
-			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0,
-			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 |
-			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 |
-			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 |
-			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0);
-
-	/* clear PHY RST, then set it */
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_SATA_OFFSET,
-			IMX8QM_SATA_CTRL_EPCS_PHYRESET_N,
-			0);
-
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_SATA_OFFSET,
-			IMX8QM_SATA_CTRL_EPCS_PHYRESET_N,
-			IMX8QM_SATA_CTRL_EPCS_PHYRESET_N);
-
-	/* CTRL RST: SET -> delay 1 us -> CLEAR -> SET */
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_SATA_OFFSET,
-			IMX8QM_SATA_CTRL_RESET_N,
-			IMX8QM_SATA_CTRL_RESET_N);
-	udelay(1);
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_SATA_OFFSET,
-			IMX8QM_SATA_CTRL_RESET_N,
-			0);
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_SATA_OFFSET,
-			IMX8QM_SATA_CTRL_RESET_N,
-			IMX8QM_SATA_CTRL_RESET_N);
-
-	/* APB reset */
-	regmap_update_bits(imxpriv->gpr,
-			IMX8QM_CSR_PHYX1_OFFSET,
-			IMX8QM_PHY_APB_RSTN_0,
-			IMX8QM_PHY_APB_RSTN_0);
-
-	for (i = 0; i < 100; i++) {
-		reg = IMX8QM_CSR_PHYX1_OFFSET +
-			IMX8QM_CSR_PHYX_STTS0_OFFSET;
-		regmap_read(imxpriv->gpr, reg, &val);
-		val &= IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
-		if (val == IMX8QM_STTS0_LANE0_TX_PLL_LOCK)
-			break;
-		udelay(1);
+	ret = phy_power_on(imxpriv->sata_phy);
+	if (ret) {
+		dev_err(dev, "sata PHY power up failed\n");
+		goto err_sata_phy_exit;
 	}
 
-	if (val != IMX8QM_STTS0_LANE0_TX_PLL_LOCK) {
-		dev_err(dev, "TX PLL of the PHY is not locked\n");
-		ret = -ENODEV;
-	} else {
-		writeb(imxpriv->imped_ratio, imxpriv->phy_base +
-				IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET);
-		writeb(imxpriv->imped_ratio, imxpriv->phy_base +
-				IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET);
-		reg = readb(imxpriv->phy_base +
-				IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET);
-		if (unlikely(reg != imxpriv->imped_ratio))
-			dev_info(dev, "Can't set PHY RX impedance ratio.\n");
-		reg = readb(imxpriv->phy_base +
-				IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET);
-		if (unlikely(reg != imxpriv->imped_ratio))
-			dev_info(dev, "Can't set PHY TX impedance ratio.\n");
-		usleep_range(50, 100);
+	/* The cali_phy# can be turned off after SATA PHY is initialized. */
+	phy_power_off(imxpriv->cali_phy1);
+	phy_exit(imxpriv->cali_phy1);
+	phy_power_off(imxpriv->cali_phy0);
+	phy_exit(imxpriv->cali_phy0);
 
-		/*
-		 * To reduce the power consumption, gate off
-		 * the PHY clks
-		 */
-		clk_disable_unprepare(imxpriv->phy_apbclk);
-		clk_disable_unprepare(imxpriv->phy_pclk1);
-		clk_disable_unprepare(imxpriv->phy_pclk0);
-		return ret;
-	}
+	return 0;
 
-	clk_disable_unprepare(imxpriv->phy_apbclk);
-disable_epcs_rx_clk:
-	clk_disable_unprepare(imxpriv->epcs_rx_clk);
-disable_epcs_tx_clk:
-	clk_disable_unprepare(imxpriv->epcs_tx_clk);
-disable_phy_pclk1:
-	clk_disable_unprepare(imxpriv->phy_pclk1);
-disable_phy_pclk0:
-	clk_disable_unprepare(imxpriv->phy_pclk0);
+err_sata_phy_exit:
+	phy_exit(imxpriv->sata_phy);
+err_cali_phy1_off:
+	phy_power_off(imxpriv->cali_phy1);
+err_cali_phy1_exit:
+	phy_exit(imxpriv->cali_phy1);
+err_cali_phy0_off:
+	phy_power_off(imxpriv->cali_phy0);
+err_cali_phy0_exit:
+	phy_exit(imxpriv->cali_phy0);
 
 	return ret;
 }
@@ -698,6 +531,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 		}
 	} else if (imxpriv->type == AHCI_IMX8QM) {
 		ret = imx8_sata_enable(hpriv);
+		if (ret)
+			goto disable_clk;
+
 	}
 
 	usleep_range(1000, 2000);
@@ -736,8 +572,10 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
 		break;
 
 	case AHCI_IMX8QM:
-		clk_disable_unprepare(imxpriv->epcs_rx_clk);
-		clk_disable_unprepare(imxpriv->epcs_tx_clk);
+		if (imxpriv->sata_phy) {
+			phy_power_off(imxpriv->sata_phy);
+			phy_exit(imxpriv->sata_phy);
+		}
 		break;
 
 	default:
@@ -760,6 +598,9 @@ static void ahci_imx_error_handler(struct ata_port *ap)
 
 	ahci_error_handler(ap);
 
+	if (imxpriv->type == AHCI_IMX8QM)
+		return;
+
 	if (!(imxpriv->first_time) || ahci_imx_hotplug)
 		return;
 
@@ -986,65 +827,22 @@ static const struct scsi_host_template ahci_platform_sht = {
 
 static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv)
 {
-	struct resource *phy_res;
-	struct platform_device *pdev = imxpriv->ahci_pdev;
-	struct device_node *np = dev->of_node;
-
-	if (of_property_read_u32(np, "fsl,phy-imp", &imxpriv->imped_ratio))
-		imxpriv->imped_ratio = IMX8QM_SATA_PHY_IMPED_RATIO_85OHM;
-	phy_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy");
-	if (phy_res) {
-		imxpriv->phy_base = devm_ioremap(dev, phy_res->start,
-					resource_size(phy_res));
-		if (!imxpriv->phy_base) {
-			dev_err(dev, "error with ioremap\n");
-			return -ENOMEM;
-		}
-	} else {
-		dev_err(dev, "missing *phy* reg region.\n");
-		return -ENOMEM;
-	}
-	imxpriv->gpr =
-		 syscon_regmap_lookup_by_phandle(np, "hsio");
-	if (IS_ERR(imxpriv->gpr)) {
-		dev_err(dev, "unable to find gpr registers\n");
-		return PTR_ERR(imxpriv->gpr);
-	}
-
-	imxpriv->epcs_tx_clk = devm_clk_get(dev, "epcs_tx");
-	if (IS_ERR(imxpriv->epcs_tx_clk)) {
-		dev_err(dev, "can't get epcs_tx_clk clock.\n");
-		return PTR_ERR(imxpriv->epcs_tx_clk);
-	}
-	imxpriv->epcs_rx_clk = devm_clk_get(dev, "epcs_rx");
-	if (IS_ERR(imxpriv->epcs_rx_clk)) {
-		dev_err(dev, "can't get epcs_rx_clk clock.\n");
-		return PTR_ERR(imxpriv->epcs_rx_clk);
-	}
-	imxpriv->phy_pclk0 = devm_clk_get(dev, "phy_pclk0");
-	if (IS_ERR(imxpriv->phy_pclk0)) {
-		dev_err(dev, "can't get phy_pclk0 clock.\n");
-		return PTR_ERR(imxpriv->phy_pclk0);
-	}
-	imxpriv->phy_pclk1 = devm_clk_get(dev, "phy_pclk1");
-	if (IS_ERR(imxpriv->phy_pclk1)) {
-		dev_err(dev, "can't get phy_pclk1 clock.\n");
-		return PTR_ERR(imxpriv->phy_pclk1);
-	}
-	imxpriv->phy_apbclk = devm_clk_get(dev, "phy_apbclk");
-	if (IS_ERR(imxpriv->phy_apbclk)) {
-		dev_err(dev, "can't get phy_apbclk clock.\n");
-		return PTR_ERR(imxpriv->phy_apbclk);
-	}
-
-	/* Fetch GPIO, then enable the external OSC */
-	imxpriv->clkreq_gpiod = devm_gpiod_get_optional(dev, "clkreq",
-				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
-	if (IS_ERR(imxpriv->clkreq_gpiod))
-		return PTR_ERR(imxpriv->clkreq_gpiod);
-	if (imxpriv->clkreq_gpiod)
-		gpiod_set_consumer_name(imxpriv->clkreq_gpiod, "SATA CLKREQ");
-
+	if (!(dev->bus_dma_limit))
+		dev->bus_dma_limit = DMA_BIT_MASK(32);
+
+	imxpriv->sata_phy = devm_phy_get(dev, "sata-phy");
+	if (IS_ERR(imxpriv->sata_phy))
+		return dev_err_probe(dev, PTR_ERR(imxpriv->sata_phy),
+				     "Failed to get sata_phy\n");
+
+	imxpriv->cali_phy0 = devm_phy_get(dev, "cali-phy0");
+	if (IS_ERR(imxpriv->cali_phy0))
+		return dev_err_probe(dev, PTR_ERR(imxpriv->cali_phy0),
+				     "Failed to get cali_phy0\n");
+	imxpriv->cali_phy1 = devm_phy_get(dev, "cali-phy1");
+	if (IS_ERR(imxpriv->cali_phy1))
+		return dev_err_probe(dev, PTR_ERR(imxpriv->cali_phy1),
+				     "Failed to get cali_phy1\n");
 	return 0;
 }
 
@@ -1077,12 +875,6 @@ static int imx_ahci_probe(struct platform_device *pdev)
 		return PTR_ERR(imxpriv->sata_ref_clk);
 	}
 
-	imxpriv->ahb_clk = devm_clk_get(dev, "ahb");
-	if (IS_ERR(imxpriv->ahb_clk)) {
-		dev_err(dev, "can't get ahb clock.\n");
-		return PTR_ERR(imxpriv->ahb_clk);
-	}
-
 	if (imxpriv->type == AHCI_IMX6Q || imxpriv->type == AHCI_IMX6QP) {
 		u32 reg_value;
 
@@ -1142,11 +934,8 @@ static int imx_ahci_probe(struct platform_device *pdev)
 		goto disable_clk;
 
 	/*
-	 * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
-	 * and IP vendor specific register IMX_TIMER1MS.
-	 * Configure CAP_SSS (support stagered spin up).
-	 * Implement the port0.
-	 * Get the ahb clock rate, and configure the TIMER1MS register.
+	 * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL.
+	 * Set CAP_SSS (support stagered spin up) and Implement the port0.
 	 */
 	reg_val = readl(hpriv->mmio + HOST_CAP);
 	if (!(reg_val & HOST_CAP_SSS)) {
@@ -1159,8 +948,19 @@ static int imx_ahci_probe(struct platform_device *pdev)
 		writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL);
 	}
 
-	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
-	writel(reg_val, hpriv->mmio + IMX_TIMER1MS);
+	if (imxpriv->type != AHCI_IMX8QM) {
+		/*
+		 * Get AHB clock rate and configure the vendor specified
+		 * TIMER1MS register on i.MX53, i.MX6Q and i.MX6QP only.
+		 */
+		imxpriv->ahb_clk = devm_clk_get(dev, "ahb");
+		if (IS_ERR(imxpriv->ahb_clk)) {
+			dev_err(dev, "Failed to get ahb clock\n");
+			goto disable_sata;
+		}
+		reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
+		writel(reg_val, hpriv->mmio + IMX_TIMER1MS);
+	}
 
 	ret = ahci_platform_init_host(pdev, hpriv, &ahci_imx_port_info,
 				      &ahci_platform_sht);
-- 
2.37.1

Re: [PATCH v3 2/4] ata: ahci_imx: Clean up code by using i.MX8Q HSIO PHY driver

[Niklas Cassel]

Hello Richard,

On Tue, Jul 16, 2024 at 11:18:13AM +0800, Richard Zhu wrote:
> Clean up code by using PHY interface.

This commit message needs some improvement.

You are removing a bunch of code, but you are not even stating which
PHY driver has the equivalent code to the code you are removing.

The minimum here is to write the full file path to the PHY driver in the
kernel tree that has the equivalent code to the code that you are removing.


> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/ata/ahci_imx.c | 396 ++++++++++-------------------------------
>  1 file changed, 98 insertions(+), 298 deletions(-)
> 
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index cb768f66f0a70..e94c0fdea2260 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -19,6 +19,7 @@
>  #include <linux/libata.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> +#include <linux/phy/phy.h>
>  #include <linux/thermal.h>
>  #include "ahci.h"
>  
> @@ -44,42 +45,6 @@ enum {
>  	/* Clock Reset Register */
>  	IMX_CLOCK_RESET				= 0x7f3f,
>  	IMX_CLOCK_RESET_RESET			= 1 << 0,
> -	/* IMX8QM HSIO AHCI definitions */
> -	IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET	= 0x03,
> -	IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET	= 0x09,
> -	IMX8QM_SATA_PHY_IMPED_RATIO_85OHM	= 0x6c,
> -	IMX8QM_LPCG_PHYX2_OFFSET		= 0x00000,
> -	IMX8QM_CSR_PHYX2_OFFSET			= 0x90000,
> -	IMX8QM_CSR_PHYX1_OFFSET			= 0xa0000,
> -	IMX8QM_CSR_PHYX_STTS0_OFFSET		= 0x4,
> -	IMX8QM_CSR_PCIEA_OFFSET			= 0xb0000,
> -	IMX8QM_CSR_PCIEB_OFFSET			= 0xc0000,
> -	IMX8QM_CSR_SATA_OFFSET			= 0xd0000,
> -	IMX8QM_CSR_PCIE_CTRL2_OFFSET		= 0x8,
> -	IMX8QM_CSR_MISC_OFFSET			= 0xe0000,
> -
> -	IMX8QM_LPCG_PHYX2_PCLK0_MASK		= (0x3 << 16),
> -	IMX8QM_LPCG_PHYX2_PCLK1_MASK		= (0x3 << 20),
> -	IMX8QM_PHY_APB_RSTN_0			= BIT(0),
> -	IMX8QM_PHY_MODE_SATA			= BIT(19),
> -	IMX8QM_PHY_MODE_MASK			= (0xf << 17),
> -	IMX8QM_PHY_PIPE_RSTN_0			= BIT(24),
> -	IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0		= BIT(25),
> -	IMX8QM_PHY_PIPE_RSTN_1			= BIT(26),
> -	IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1		= BIT(27),
> -	IMX8QM_STTS0_LANE0_TX_PLL_LOCK		= BIT(4),
> -	IMX8QM_MISC_IOB_RXENA			= BIT(0),
> -	IMX8QM_MISC_IOB_TXENA			= BIT(1),
> -	IMX8QM_MISC_PHYX1_EPCS_SEL		= BIT(12),
> -	IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1	= BIT(24),
> -	IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0	= BIT(25),
> -	IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1	= BIT(28),
> -	IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0	= BIT(29),
> -	IMX8QM_SATA_CTRL_RESET_N		= BIT(12),
> -	IMX8QM_SATA_CTRL_EPCS_PHYRESET_N	= BIT(7),
> -	IMX8QM_CTRL_BUTTON_RST_N		= BIT(21),
> -	IMX8QM_CTRL_POWER_UP_RST_N		= BIT(23),
> -	IMX8QM_CTRL_LTSSM_ENABLE		= BIT(4),
>  };
>  
>  enum ahci_imx_type {
> @@ -95,14 +60,10 @@ struct imx_ahci_priv {
>  	struct clk *sata_clk;
>  	struct clk *sata_ref_clk;
>  	struct clk *ahb_clk;
> -	struct clk *epcs_tx_clk;
> -	struct clk *epcs_rx_clk;
> -	struct clk *phy_apbclk;
> -	struct clk *phy_pclk0;
> -	struct clk *phy_pclk1;
> -	void __iomem *phy_base;
> -	struct gpio_desc *clkreq_gpiod;
>  	struct regmap *gpr;
> +	struct phy *sata_phy;
> +	struct phy *cali_phy0;
> +	struct phy *cali_phy1;
>  	bool no_device;
>  	bool first_time;
>  	u32 phy_params;
> @@ -450,201 +411,73 @@ ATTRIBUTE_GROUPS(fsl_sata_ahci);
>  
>  static int imx8_sata_enable(struct ahci_host_priv *hpriv)
>  {
> -	u32 val, reg;
> -	int i, ret;
> +	u32 val;
> +	int ret;
>  	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
>  	struct device *dev = &imxpriv->ahci_pdev->dev;
>  
> -	/* configure the hsio for sata */
> -	ret = clk_prepare_enable(imxpriv->phy_pclk0);
> -	if (ret < 0) {
> -		dev_err(dev, "can't enable phy_pclk0.\n");
> +	/*
> +	 * Since "REXT" pin is only present for first lane of i.MX8QM
> +	 * PHY, its calibration results will be stored, passed through
> +	 * to the second lane PHY, and shared with all three lane PHYs.
> +	 *
> +	 * Initialize the first two lane PHYs here, although only the
> +	 * third lane PHY is used by SATA.
> +	 */
> +	ret = phy_init(imxpriv->cali_phy0);
> +	if (ret) {
> +		dev_err(dev, "cali PHY init failed\n");
>  		return ret;
>  	}
> -	ret = clk_prepare_enable(imxpriv->phy_pclk1);
> -	if (ret < 0) {
> -		dev_err(dev, "can't enable phy_pclk1.\n");
> -		goto disable_phy_pclk0;
> +	ret = phy_power_on(imxpriv->cali_phy0);
> +	if (ret) {
> +		dev_err(dev, "cali PHY power on failed\n");
> +		goto err_cali_phy0_exit;
>  	}
> -	ret = clk_prepare_enable(imxpriv->epcs_tx_clk);
> -	if (ret < 0) {
> -		dev_err(dev, "can't enable epcs_tx_clk.\n");
> -		goto disable_phy_pclk1;
> +	ret = phy_init(imxpriv->cali_phy1);
> +	if (ret) {
> +		dev_err(dev, "cali PHY1 init failed\n");
> +		goto err_cali_phy0_off;
>  	}
> -	ret = clk_prepare_enable(imxpriv->epcs_rx_clk);
> -	if (ret < 0) {
> -		dev_err(dev, "can't enable epcs_rx_clk.\n");
> -		goto disable_epcs_tx_clk;
> +	ret = phy_power_on(imxpriv->cali_phy1);
> +	if (ret) {
> +		dev_err(dev, "cali PHY1 power on failed\n");
> +		goto err_cali_phy1_exit;
>  	}
> -	ret = clk_prepare_enable(imxpriv->phy_apbclk);
> -	if (ret < 0) {
> -		dev_err(dev, "can't enable phy_apbclk.\n");
> -		goto disable_epcs_rx_clk;
> +	ret = phy_init(imxpriv->sata_phy);
> +	if (ret) {
> +		dev_err(dev, "sata PHY init failed\n");
> +		goto err_cali_phy1_off;
>  	}
> -	/* Configure PHYx2 PIPE_RSTN */
> -	regmap_read(imxpriv->gpr, IMX8QM_CSR_PCIEA_OFFSET +
> -			IMX8QM_CSR_PCIE_CTRL2_OFFSET, &val);
> -	if ((val & IMX8QM_CTRL_LTSSM_ENABLE) == 0) {
> -		/* The link of the PCIEA of HSIO is down */
> -		regmap_update_bits(imxpriv->gpr,
> -				IMX8QM_CSR_PHYX2_OFFSET,
> -				IMX8QM_PHY_PIPE_RSTN_0 |
> -				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0,
> -				IMX8QM_PHY_PIPE_RSTN_0 |
> -				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0);
> +	ret = phy_set_mode(imxpriv->sata_phy, PHY_MODE_SATA);
> +	if (ret) {
> +		dev_err(dev, "unable to set SATA PHY mode\n");
> +		goto err_sata_phy_exit;
>  	}
> -	regmap_read(imxpriv->gpr, IMX8QM_CSR_PCIEB_OFFSET +
> -			IMX8QM_CSR_PCIE_CTRL2_OFFSET, &reg);
> -	if ((reg & IMX8QM_CTRL_LTSSM_ENABLE) == 0) {
> -		/* The link of the PCIEB of HSIO is down */
> -		regmap_update_bits(imxpriv->gpr,
> -				IMX8QM_CSR_PHYX2_OFFSET,
> -				IMX8QM_PHY_PIPE_RSTN_1 |
> -				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1,
> -				IMX8QM_PHY_PIPE_RSTN_1 |
> -				IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1);
> -	}
> -	if (((reg | val) & IMX8QM_CTRL_LTSSM_ENABLE) == 0) {
> -		/* The links of both PCIA and PCIEB of HSIO are down */
> -		regmap_update_bits(imxpriv->gpr,
> -				IMX8QM_LPCG_PHYX2_OFFSET,
> -				IMX8QM_LPCG_PHYX2_PCLK0_MASK |
> -				IMX8QM_LPCG_PHYX2_PCLK1_MASK,
> -				0);
> -	}
> -
> -	/* set PWR_RST and BT_RST of csr_pciea */
> -	val = IMX8QM_CSR_PCIEA_OFFSET + IMX8QM_CSR_PCIE_CTRL2_OFFSET;
> -	regmap_update_bits(imxpriv->gpr,
> -			val,
> -			IMX8QM_CTRL_BUTTON_RST_N,
> -			IMX8QM_CTRL_BUTTON_RST_N);
> -	regmap_update_bits(imxpriv->gpr,
> -			val,
> -			IMX8QM_CTRL_POWER_UP_RST_N,
> -			IMX8QM_CTRL_POWER_UP_RST_N);
> -
> -	/* PHYX1_MODE to SATA */
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_PHYX1_OFFSET,
> -			IMX8QM_PHY_MODE_MASK,
> -			IMX8QM_PHY_MODE_SATA);
> -
> -	/*
> -	 * BIT0 RXENA 1, BIT1 TXENA 0
> -	 * BIT12 PHY_X1_EPCS_SEL 1.
> -	 */
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_MISC_OFFSET,
> -			IMX8QM_MISC_IOB_RXENA,
> -			IMX8QM_MISC_IOB_RXENA);
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_MISC_OFFSET,
> -			IMX8QM_MISC_IOB_TXENA,
> -			0);
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_MISC_OFFSET,
> -			IMX8QM_MISC_PHYX1_EPCS_SEL,
> -			IMX8QM_MISC_PHYX1_EPCS_SEL);
> -	/*
> -	 * It is possible, for PCIe and SATA are sharing
> -	 * the same clock source, HPLL or external oscillator.
> -	 * When PCIe is in low power modes (L1.X or L2 etc),
> -	 * the clock source can be turned off. In this case,
> -	 * if this clock source is required to be toggling by
> -	 * SATA, then SATA functions will be abnormal.
> -	 * Set the override here to avoid it.
> -	 */
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_MISC_OFFSET,
> -			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 |
> -			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 |
> -			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 |
> -			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0,
> -			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 |
> -			IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 |
> -			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 |
> -			IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0);
> -
> -	/* clear PHY RST, then set it */
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_SATA_OFFSET,
> -			IMX8QM_SATA_CTRL_EPCS_PHYRESET_N,
> -			0);
> -
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_SATA_OFFSET,
> -			IMX8QM_SATA_CTRL_EPCS_PHYRESET_N,
> -			IMX8QM_SATA_CTRL_EPCS_PHYRESET_N);
> -
> -	/* CTRL RST: SET -> delay 1 us -> CLEAR -> SET */
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_SATA_OFFSET,
> -			IMX8QM_SATA_CTRL_RESET_N,
> -			IMX8QM_SATA_CTRL_RESET_N);
> -	udelay(1);
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_SATA_OFFSET,
> -			IMX8QM_SATA_CTRL_RESET_N,
> -			0);
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_SATA_OFFSET,
> -			IMX8QM_SATA_CTRL_RESET_N,
> -			IMX8QM_SATA_CTRL_RESET_N);
> -
> -	/* APB reset */
> -	regmap_update_bits(imxpriv->gpr,
> -			IMX8QM_CSR_PHYX1_OFFSET,
> -			IMX8QM_PHY_APB_RSTN_0,
> -			IMX8QM_PHY_APB_RSTN_0);
> -
> -	for (i = 0; i < 100; i++) {
> -		reg = IMX8QM_CSR_PHYX1_OFFSET +
> -			IMX8QM_CSR_PHYX_STTS0_OFFSET;
> -		regmap_read(imxpriv->gpr, reg, &val);
> -		val &= IMX8QM_STTS0_LANE0_TX_PLL_LOCK;
> -		if (val == IMX8QM_STTS0_LANE0_TX_PLL_LOCK)
> -			break;
> -		udelay(1);
> +	ret = phy_power_on(imxpriv->sata_phy);
> +	if (ret) {
> +		dev_err(dev, "sata PHY power up failed\n");
> +		goto err_sata_phy_exit;
>  	}
>  
> -	if (val != IMX8QM_STTS0_LANE0_TX_PLL_LOCK) {
> -		dev_err(dev, "TX PLL of the PHY is not locked\n");
> -		ret = -ENODEV;
> -	} else {
> -		writeb(imxpriv->imped_ratio, imxpriv->phy_base +
> -				IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET);
> -		writeb(imxpriv->imped_ratio, imxpriv->phy_base +
> -				IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET);
> -		reg = readb(imxpriv->phy_base +
> -				IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET);
> -		if (unlikely(reg != imxpriv->imped_ratio))
> -			dev_info(dev, "Can't set PHY RX impedance ratio.\n");
> -		reg = readb(imxpriv->phy_base +
> -				IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET);
> -		if (unlikely(reg != imxpriv->imped_ratio))
> -			dev_info(dev, "Can't set PHY TX impedance ratio.\n");
> -		usleep_range(50, 100);
> +	/* The cali_phy# can be turned off after SATA PHY is initialized. */
> +	phy_power_off(imxpriv->cali_phy1);
> +	phy_exit(imxpriv->cali_phy1);
> +	phy_power_off(imxpriv->cali_phy0);
> +	phy_exit(imxpriv->cali_phy0);
>  
> -		/*
> -		 * To reduce the power consumption, gate off
> -		 * the PHY clks
> -		 */
> -		clk_disable_unprepare(imxpriv->phy_apbclk);
> -		clk_disable_unprepare(imxpriv->phy_pclk1);
> -		clk_disable_unprepare(imxpriv->phy_pclk0);
> -		return ret;
> -	}
> +	return 0;
>  
> -	clk_disable_unprepare(imxpriv->phy_apbclk);
> -disable_epcs_rx_clk:
> -	clk_disable_unprepare(imxpriv->epcs_rx_clk);
> -disable_epcs_tx_clk:
> -	clk_disable_unprepare(imxpriv->epcs_tx_clk);
> -disable_phy_pclk1:
> -	clk_disable_unprepare(imxpriv->phy_pclk1);
> -disable_phy_pclk0:
> -	clk_disable_unprepare(imxpriv->phy_pclk0);
> +err_sata_phy_exit:
> +	phy_exit(imxpriv->sata_phy);
> +err_cali_phy1_off:
> +	phy_power_off(imxpriv->cali_phy1);
> +err_cali_phy1_exit:
> +	phy_exit(imxpriv->cali_phy1);
> +err_cali_phy0_off:
> +	phy_power_off(imxpriv->cali_phy0);
> +err_cali_phy0_exit:
> +	phy_exit(imxpriv->cali_phy0);
>  
>  	return ret;
>  }
> @@ -698,6 +531,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
>  		}
>  	} else if (imxpriv->type == AHCI_IMX8QM) {
>  		ret = imx8_sata_enable(hpriv);
> +		if (ret)
> +			goto disable_clk;
> +
>  	}
>  
>  	usleep_range(1000, 2000);
> @@ -736,8 +572,10 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
>  		break;
>  
>  	case AHCI_IMX8QM:
> -		clk_disable_unprepare(imxpriv->epcs_rx_clk);
> -		clk_disable_unprepare(imxpriv->epcs_tx_clk);
> +		if (imxpriv->sata_phy) {
> +			phy_power_off(imxpriv->sata_phy);
> +			phy_exit(imxpriv->sata_phy);
> +		}
>  		break;
>  
>  	default:
> @@ -760,6 +598,9 @@ static void ahci_imx_error_handler(struct ata_port *ap)
>  
>  	ahci_error_handler(ap);
>  
> +	if (imxpriv->type == AHCI_IMX8QM)
> +		return;
> +
>  	if (!(imxpriv->first_time) || ahci_imx_hotplug)
>  		return;
>  
> @@ -986,65 +827,22 @@ static const struct scsi_host_template ahci_platform_sht = {
>  
>  static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv)
>  {
> -	struct resource *phy_res;
> -	struct platform_device *pdev = imxpriv->ahci_pdev;
> -	struct device_node *np = dev->of_node;
> -
> -	if (of_property_read_u32(np, "fsl,phy-imp", &imxpriv->imped_ratio))
> -		imxpriv->imped_ratio = IMX8QM_SATA_PHY_IMPED_RATIO_85OHM;
> -	phy_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy");
> -	if (phy_res) {
> -		imxpriv->phy_base = devm_ioremap(dev, phy_res->start,
> -					resource_size(phy_res));
> -		if (!imxpriv->phy_base) {
> -			dev_err(dev, "error with ioremap\n");
> -			return -ENOMEM;
> -		}
> -	} else {
> -		dev_err(dev, "missing *phy* reg region.\n");
> -		return -ENOMEM;
> -	}
> -	imxpriv->gpr =
> -		 syscon_regmap_lookup_by_phandle(np, "hsio");
> -	if (IS_ERR(imxpriv->gpr)) {
> -		dev_err(dev, "unable to find gpr registers\n");
> -		return PTR_ERR(imxpriv->gpr);
> -	}
> -
> -	imxpriv->epcs_tx_clk = devm_clk_get(dev, "epcs_tx");
> -	if (IS_ERR(imxpriv->epcs_tx_clk)) {
> -		dev_err(dev, "can't get epcs_tx_clk clock.\n");
> -		return PTR_ERR(imxpriv->epcs_tx_clk);
> -	}
> -	imxpriv->epcs_rx_clk = devm_clk_get(dev, "epcs_rx");
> -	if (IS_ERR(imxpriv->epcs_rx_clk)) {
> -		dev_err(dev, "can't get epcs_rx_clk clock.\n");
> -		return PTR_ERR(imxpriv->epcs_rx_clk);
> -	}
> -	imxpriv->phy_pclk0 = devm_clk_get(dev, "phy_pclk0");
> -	if (IS_ERR(imxpriv->phy_pclk0)) {
> -		dev_err(dev, "can't get phy_pclk0 clock.\n");
> -		return PTR_ERR(imxpriv->phy_pclk0);
> -	}
> -	imxpriv->phy_pclk1 = devm_clk_get(dev, "phy_pclk1");
> -	if (IS_ERR(imxpriv->phy_pclk1)) {
> -		dev_err(dev, "can't get phy_pclk1 clock.\n");
> -		return PTR_ERR(imxpriv->phy_pclk1);
> -	}
> -	imxpriv->phy_apbclk = devm_clk_get(dev, "phy_apbclk");
> -	if (IS_ERR(imxpriv->phy_apbclk)) {
> -		dev_err(dev, "can't get phy_apbclk clock.\n");
> -		return PTR_ERR(imxpriv->phy_apbclk);
> -	}
> -
> -	/* Fetch GPIO, then enable the external OSC */
> -	imxpriv->clkreq_gpiod = devm_gpiod_get_optional(dev, "clkreq",
> -				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> -	if (IS_ERR(imxpriv->clkreq_gpiod))
> -		return PTR_ERR(imxpriv->clkreq_gpiod);
> -	if (imxpriv->clkreq_gpiod)
> -		gpiod_set_consumer_name(imxpriv->clkreq_gpiod, "SATA CLKREQ");
> -
> +	if (!(dev->bus_dma_limit))
> +		dev->bus_dma_limit = DMA_BIT_MASK(32);
> +
> +	imxpriv->sata_phy = devm_phy_get(dev, "sata-phy");
> +	if (IS_ERR(imxpriv->sata_phy))
> +		return dev_err_probe(dev, PTR_ERR(imxpriv->sata_phy),
> +				     "Failed to get sata_phy\n");
> +
> +	imxpriv->cali_phy0 = devm_phy_get(dev, "cali-phy0");
> +	if (IS_ERR(imxpriv->cali_phy0))
> +		return dev_err_probe(dev, PTR_ERR(imxpriv->cali_phy0),
> +				     "Failed to get cali_phy0\n");
> +	imxpriv->cali_phy1 = devm_phy_get(dev, "cali-phy1");
> +	if (IS_ERR(imxpriv->cali_phy1))
> +		return dev_err_probe(dev, PTR_ERR(imxpriv->cali_phy1),
> +				     "Failed to get cali_phy1\n");
>  	return 0;
>  }
>  
> @@ -1077,12 +875,6 @@ static int imx_ahci_probe(struct platform_device *pdev)
>  		return PTR_ERR(imxpriv->sata_ref_clk);
>  	}
>  
> -	imxpriv->ahb_clk = devm_clk_get(dev, "ahb");
> -	if (IS_ERR(imxpriv->ahb_clk)) {
> -		dev_err(dev, "can't get ahb clock.\n");
> -		return PTR_ERR(imxpriv->ahb_clk);
> -	}
> -
>  	if (imxpriv->type == AHCI_IMX6Q || imxpriv->type == AHCI_IMX6QP) {
>  		u32 reg_value;
>  
> @@ -1142,11 +934,8 @@ static int imx_ahci_probe(struct platform_device *pdev)
>  		goto disable_clk;
>  
>  	/*
> -	 * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL,
> -	 * and IP vendor specific register IMX_TIMER1MS.
> -	 * Configure CAP_SSS (support stagered spin up).
> -	 * Implement the port0.
> -	 * Get the ahb clock rate, and configure the TIMER1MS register.
> +	 * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL.
> +	 * Set CAP_SSS (support stagered spin up) and Implement the port0.
>  	 */
>  	reg_val = readl(hpriv->mmio + HOST_CAP);
>  	if (!(reg_val & HOST_CAP_SSS)) {
> @@ -1159,8 +948,19 @@ static int imx_ahci_probe(struct platform_device *pdev)
>  		writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL);
>  	}
>  
> -	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
> -	writel(reg_val, hpriv->mmio + IMX_TIMER1MS);
> +	if (imxpriv->type != AHCI_IMX8QM) {
> +		/*
> +		 * Get AHB clock rate and configure the vendor specified
> +		 * TIMER1MS register on i.MX53, i.MX6Q and i.MX6QP only.
> +		 */
> +		imxpriv->ahb_clk = devm_clk_get(dev, "ahb");
> +		if (IS_ERR(imxpriv->ahb_clk)) {
> +			dev_err(dev, "Failed to get ahb clock\n");
> +			goto disable_sata;
> +		}
> +		reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
> +		writel(reg_val, hpriv->mmio + IMX_TIMER1MS);
> +	}

This chunk has nothing to do with:
"ata: ahci_imx: Clean up code by using i.MX8Q HSIO PHY driver"
so it should be in a separate commit, with a proper commit message.


Kind regards,
Niklas

Re: [PATCH v3 2/4] ata: ahci_imx: Clean up code by using i.MX8Q HSIO PHY driver

[Niklas Cassel]

On Tue, Jul 16, 2024 at 11:18:13AM +0800, Richard Zhu wrote:
> Clean up code by using PHY interface.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/ata/ahci_imx.c | 396 ++++++++++-------------------------------
>  1 file changed, 98 insertions(+), 298 deletions(-)
> 
> diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
> index cb768f66f0a70..e94c0fdea2260 100644
> --- a/drivers/ata/ahci_imx.c
> +++ b/drivers/ata/ahci_imx.c
> @@ -986,65 +827,22 @@ static const struct scsi_host_template ahci_platform_sht = {
>  
>  static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv)
>  {
> -	struct resource *phy_res;
> -	struct platform_device *pdev = imxpriv->ahci_pdev;
> -	struct device_node *np = dev->of_node;
> -
> -	if (of_property_read_u32(np, "fsl,phy-imp", &imxpriv->imped_ratio))
> -		imxpriv->imped_ratio = IMX8QM_SATA_PHY_IMPED_RATIO_85OHM;
> -	phy_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy");
> -	if (phy_res) {
> -		imxpriv->phy_base = devm_ioremap(dev, phy_res->start,
> -					resource_size(phy_res));
> -		if (!imxpriv->phy_base) {
> -			dev_err(dev, "error with ioremap\n");
> -			return -ENOMEM;
> -		}
> -	} else {
> -		dev_err(dev, "missing *phy* reg region.\n");
> -		return -ENOMEM;
> -	}
> -	imxpriv->gpr =
> -		 syscon_regmap_lookup_by_phandle(np, "hsio");
> -	if (IS_ERR(imxpriv->gpr)) {
> -		dev_err(dev, "unable to find gpr registers\n");
> -		return PTR_ERR(imxpriv->gpr);
> -	}
> -
> -	imxpriv->epcs_tx_clk = devm_clk_get(dev, "epcs_tx");
> -	if (IS_ERR(imxpriv->epcs_tx_clk)) {
> -		dev_err(dev, "can't get epcs_tx_clk clock.\n");
> -		return PTR_ERR(imxpriv->epcs_tx_clk);
> -	}
> -	imxpriv->epcs_rx_clk = devm_clk_get(dev, "epcs_rx");
> -	if (IS_ERR(imxpriv->epcs_rx_clk)) {
> -		dev_err(dev, "can't get epcs_rx_clk clock.\n");
> -		return PTR_ERR(imxpriv->epcs_rx_clk);
> -	}
> -	imxpriv->phy_pclk0 = devm_clk_get(dev, "phy_pclk0");
> -	if (IS_ERR(imxpriv->phy_pclk0)) {
> -		dev_err(dev, "can't get phy_pclk0 clock.\n");
> -		return PTR_ERR(imxpriv->phy_pclk0);
> -	}
> -	imxpriv->phy_pclk1 = devm_clk_get(dev, "phy_pclk1");
> -	if (IS_ERR(imxpriv->phy_pclk1)) {
> -		dev_err(dev, "can't get phy_pclk1 clock.\n");
> -		return PTR_ERR(imxpriv->phy_pclk1);
> -	}
> -	imxpriv->phy_apbclk = devm_clk_get(dev, "phy_apbclk");
> -	if (IS_ERR(imxpriv->phy_apbclk)) {
> -		dev_err(dev, "can't get phy_apbclk clock.\n");
> -		return PTR_ERR(imxpriv->phy_apbclk);
> -	}
> -
> -	/* Fetch GPIO, then enable the external OSC */
> -	imxpriv->clkreq_gpiod = devm_gpiod_get_optional(dev, "clkreq",
> -				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> -	if (IS_ERR(imxpriv->clkreq_gpiod))
> -		return PTR_ERR(imxpriv->clkreq_gpiod);
> -	if (imxpriv->clkreq_gpiod)
> -		gpiod_set_consumer_name(imxpriv->clkreq_gpiod, "SATA CLKREQ");
> -
> +	if (!(dev->bus_dma_limit))
> +		dev->bus_dma_limit = DMA_BIT_MASK(32);

These two lines look like a unrelated change, should be in a separate commit
with a proper commit message.


Kind regards,
Niklas

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

[Richard Zhu]

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..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:
-- 
2.37.1

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

[Niklas Cassel]

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

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

[Damien Le Moal]

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

-- 
Damien Le Moal
Western Digital Research

[PATCH v3 4/4] ata: ahci_imx: Correct the email address

[Richard Zhu]

Correct the email address of driver author.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/ata/ahci_imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index b6ad0fe5fbbcc..965466c7a27e5 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -1039,6 +1039,6 @@ static struct platform_driver imx_ahci_driver = {
 module_platform_driver(imx_ahci_driver);
 
 MODULE_DESCRIPTION("Freescale i.MX AHCI SATA platform driver");
-MODULE_AUTHOR("Richard Zhu <Hong-Xing.Zhu@freescale.com>");
+MODULE_AUTHOR("Richard Zhu <hongxing.zhu@nxp.com>");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.37.1