[PATCH v3 0/3] Add driver support for Eswin EIC7700 SoC SATA Controller and PHY

[PATCH v3 0/3] Add driver support for Eswin EIC7700 SoC SATA Controller and PHY

[Yulin Lu]

This series depends on the config option patch [1].

[1] https://lore.kernel.org/all/20250825132427.1618089-3-pinkesh.vaghela@einfochips.com/

Updates:
  v2 -> v3:
    - Use full name in "From" and "Signed-off-by" fields information.
    - eswin,eic7700-ahci.yaml
      - Remove the introduction to the reg, interrupts, phys, and phy-names fields.
      - Modify the usage of the clocks field in the examples.
      - Corrected the order of dt properties.
    - phy-eic7700-sata.c
      - Register operations use the GENMASK macro and FIELD_PREP instead of
        the original bit offset method, and add "#include <linux/bitfield.h>".
      - Modified some macro definition names.
      - Remove the redundant initialization assignments for "ret" and "val".
      - Delete ".suppress_bind_attrs = true".
      - Modify the driver name.
      - Add "#include <linux/io.h>" to fix the robot test issue.
    - Link to v2: https://lore.kernel.org/lkml/20250819134722.220-1-luyulin@eswincomputing.com/

  v2 -> v1:
    - Delete the original controller driver and use ahci_dwc.c instead.
    - Add eswin,eic7700-ahci.yaml
      - Correct the descriptions of reset, interrupt and other
        hardware resources for the sata controller on EIC7700 SoC.
      - The clocks for both sata controller and sata PHY are controlled
        via a register bit in the HSP bus and are not registered in the
        clock tree. Clock are managed within the PHY driver, therefore
        it is not described in this document.
      - Add $ref: snps,dwc-ahci-common.yaml#.
    - Add eswin,eic7700-sata-phy.yaml
      - Add this file to include the description of the PHY on EIC7700 SoC.
    - Add an eswin directory under the PHY driver path, and include the SATA
      PHY driver code for EIC7700 SoC.
    - Link to v1: https://lore.kernel.org/all/20250515085114.1692-1-hehuan1@eswincomputing.com/

Yulin Lu (3):
  dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
  dt-bindings: phy: eswin: Document for EIC7700 SoC SATA PHY
  phy: eswin: Create eswin directory and add EIC7700 SATA PHY driver

 .../bindings/ata/eswin,eic7700-ahci.yaml      |  80 +++++++
 .../bindings/phy/eswin,eic7700-sata-phy.yaml  |  36 +++
 drivers/phy/Kconfig                           |   1 +
 drivers/phy/Makefile                          |   1 +
 drivers/phy/eswin/Kconfig                     |  14 ++
 drivers/phy/eswin/Makefile                    |   2 +
 drivers/phy/eswin/phy-eic7700-sata.c          | 205 ++++++++++++++++++
 7 files changed, 339 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/eswin,eic7700-sata-phy.yaml
 create mode 100644 drivers/phy/eswin/Kconfig
 create mode 100644 drivers/phy/eswin/Makefile
 create mode 100644 drivers/phy/eswin/phy-eic7700-sata.c

-- 
2.25.1

[PATCH v3 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci

[Yulin Lu]

Add document for the SATA AHCI controller on the EIC7700 SoC platform,
including descriptions of its hardware configurations.

Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
---
 .../bindings/ata/eswin,eic7700-ahci.yaml      | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml

diff --git a/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml b/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
new file mode 100644
index 000000000000..9266f8f2543e
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/ata/eswin,eic7700-ahci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Eswin EIC7700 SoC SATA Controller
+
+maintainers:
+  - Yulin Lu <luyulin@eswincomputing.com>
+  - Huan He <hehuan1@eswincomputing.com>
+
+description:
+  This document defines device tree bindings for the Synopsys DWC
+  implementation of the AHCI SATA controller found in Eswin's
+  Eic7700 SoC platform.
+
+select:
+  properties:
+    compatible:
+      const: eswin,eic7700-ahci
+  required:
+    - compatible
+
+allOf:
+  - $ref: snps,dwc-ahci-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: eswin,eic7700-ahci
+      - const: snps,dwc-ahci
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: aclk
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: arst
+
+  ports-implemented:
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - phys
+  - phy-names
+  - ports-implemented
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    sata@50420000 {
+        compatible = "eswin,eic7700-ahci", "snps,dwc-ahci";
+        reg = <0x50420000 0x10000>;
+        interrupt-parent = <&plic>;
+        interrupts = <58>;
+        clocks = <&clock 171>, <&clock 186>;
+        clock-names = "pclk", "aclk";
+        phys = <&sata_phy>;
+        phy-names = "sata-phy";
+        ports-implemented = <0x1>;
+        resets = <&reset 96>;
+        reset-names = "arst";
+    };
-- 
2.25.1

[PATCH v3 2/3] dt-bindings: phy: eswin: Document for EIC7700 SoC SATA PHY

[Yulin Lu]

Add document for the SATA phy on the EIC7700 SoC platform,
describing its usage.

Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
---
 .../bindings/phy/eswin,eic7700-sata-phy.yaml  | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/eswin,eic7700-sata-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/eswin,eic7700-sata-phy.yaml b/Documentation/devicetree/bindings/phy/eswin,eic7700-sata-phy.yaml
new file mode 100644
index 000000000000..d914cb4402d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/eswin,eic7700-sata-phy.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/eswin,eic7700-sata-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Eswin EIC7700 SoC SATA PHY
+
+maintainers:
+  - Yulin Lu <luyulin@eswincomputing.com>
+  - Huan He <hehuan1@eswincomputing.com>
+
+properties:
+  compatible:
+    const: eswin,eic7700-sata-phy
+
+  "#phy-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - "#phy-cells"
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    sata-phy@50440300 {
+        compatible = "eswin,eic7700-sata-phy";
+        reg = <0x50440300 0x40>;
+        #phy-cells = <0>;
+    };
-- 
2.25.1

[PATCH v3 3/3] phy: eswin: Create eswin directory and add EIC7700 SATA PHY driver

[Yulin Lu]

Created the eswin phy driver directory and added support for
the SATA phy driver on the EIC7700 SoC platform.

Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
---
 drivers/phy/Kconfig                  |   1 +
 drivers/phy/Makefile                 |   1 +
 drivers/phy/eswin/Kconfig            |  14 ++
 drivers/phy/eswin/Makefile           |   2 +
 drivers/phy/eswin/phy-eic7700-sata.c | 205 +++++++++++++++++++++++++++
 5 files changed, 223 insertions(+)
 create mode 100644 drivers/phy/eswin/Kconfig
 create mode 100644 drivers/phy/eswin/Makefile
 create mode 100644 drivers/phy/eswin/phy-eic7700-sata.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 58c911e1b2d2..e82ebcfe534a 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -105,6 +105,7 @@ source "drivers/phy/allwinner/Kconfig"
 source "drivers/phy/amlogic/Kconfig"
 source "drivers/phy/broadcom/Kconfig"
 source "drivers/phy/cadence/Kconfig"
+source "drivers/phy/eswin/Kconfig"
 source "drivers/phy/freescale/Kconfig"
 source "drivers/phy/hisilicon/Kconfig"
 source "drivers/phy/ingenic/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index c670a8dac468..ed7444949259 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -17,6 +17,7 @@ obj-y					+= allwinner/	\
 					   amlogic/	\
 					   broadcom/	\
 					   cadence/	\
+					   eswin/	\
 					   freescale/	\
 					   hisilicon/	\
 					   ingenic/	\
diff --git a/drivers/phy/eswin/Kconfig b/drivers/phy/eswin/Kconfig
new file mode 100644
index 000000000000..3fcd76582c3b
--- /dev/null
+++ b/drivers/phy/eswin/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Phy drivers for Eswin platforms
+#
+config PHY_EIC7700_SATA
+	tristate "eic7700 Sata SerDes/PHY driver"
+	depends on ARCH_ESWIN || COMPILE_TEST
+	depends on HAS_IOMEM
+	select GENERIC_PHY
+	help
+	  Enable this to support SerDes/Phy found on ESWIN's
+	  EIC7700 SoC.This Phy supports SATA 1.5 Gb/s,
+	  SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds.
+	  It supports one SATA host port to accept one SATA device.
diff --git a/drivers/phy/eswin/Makefile b/drivers/phy/eswin/Makefile
new file mode 100644
index 000000000000..db08c66be812
--- /dev/null
+++ b/drivers/phy/eswin/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_EIC7700_SATA)	+= phy-eic7700-sata.o
diff --git a/drivers/phy/eswin/phy-eic7700-sata.c b/drivers/phy/eswin/phy-eic7700-sata.c
new file mode 100644
index 000000000000..cf97365bf841
--- /dev/null
+++ b/drivers/phy/eswin/phy-eic7700-sata.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ESWIN SATA PHY driver
+ *
+ * Copyright 2024, Beijing ESWIN Computing Technology Co., Ltd..
+ * All rights reserved.
+ *
+ * Authors: Yulin Lu <luyulin@eswincomputing.com>
+ */
+
+#include <linux/io.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+
+#define SATA_CLK_CTRL				0x0
+#define SATA_AXI_LP_CTRL			0x08
+#define SATA_MPLL_CTRL				0x20
+#define SATA_P0_PHY_STAT			0x24
+#define SATA_PHY_CTRL0				0x28
+#define SATA_PHY_CTRL1				0x2c
+#define SATA_REG_CTRL				0x34
+#define SATA_REF_CTRL1				0x38
+#define SATA_LOS_IDEN				0x3c
+#define SATA_RESET_CTRL				0x40
+
+#define SATA_SYS_CLK_EN				BIT(28)
+#define SATA_PHY_RESET				BIT(0)
+#define SATA_PORT_RESET				BIT(1)
+#define SATA_CLK_RST_SOURCE_PHY			BIT(0)
+#define SATA_P0_PHY_TX_AMPLITUDE_GEN1_MASK	GENMASK(6, 0)
+#define SATA_P0_PHY_TX_AMPLITUDE_GEN2_MASK	GENMASK(14, 8)
+#define SATA_P0_PHY_TX_AMPLITUDE_GEN3_MASK	GENMASK(22, 16)
+#define SATA_P0_PHY_TX_PREEMPH_GEN1_MASK	GENMASK(5, 0)
+#define SATA_P0_PHY_TX_PREEMPH_GEN2_MASK	GENMASK(13, 8)
+#define SATA_P0_PHY_TX_PREEMPH_GEN3_MASK	GENMASK(21, 16)
+#define SATA_LOS_LEVEL_MASK			GENMASK(4, 0)
+#define SATA_LOS_BIAS_MASK			GENMASK(18, 16)
+#define SATA_M_CSYSREQ				BIT(0)
+#define SATA_S_CSYSREQ				BIT(16)
+#define SATA_REF_REPEATCLK_EN			BIT(0)
+#define SATA_REF_USE_PAD			BIT(20)
+#define SATA_MPLL_MULTIPLIER_MASK		GENMASK(22, 16)
+#define SATA_P0_PHY_READY			BIT(0)
+
+#define PHY_READY_TIMEOUT			(usecs_to_jiffies(4000))
+
+struct eic7700_sata_phy {
+	struct phy *phy;
+	void __iomem *regs;
+};
+
+static int wait_for_phy_ready(void __iomem *base, u32 reg, u32 checkbit,
+			      u32 status)
+{
+	unsigned long start = jiffies;
+	unsigned long timeout = start + PHY_READY_TIMEOUT;
+
+	while (time_before(start, timeout)) {
+		if ((readl(base + reg) & checkbit) == status)
+			return 0;
+		usleep_range(50, 70);
+	}
+
+	return -EFAULT;
+}
+
+static int eic7700_sata_phy_init(struct phy *phy)
+{
+	struct eic7700_sata_phy *sata_phy = phy_get_drvdata(phy);
+	u32 val;
+	int ret;
+
+	/*
+	 * The SATA_CLK_CTRL register offset controls the pmalive, rxoob,
+	 * and rbc clocks gate provided by the PHY through the HSP bus,
+	 * and it is not registered in the clock tree.
+	 */
+	val = readl(sata_phy->regs + SATA_CLK_CTRL);
+	val |= SATA_SYS_CLK_EN;
+	writel(val, sata_phy->regs + SATA_CLK_CTRL);
+
+	writel(SATA_CLK_RST_SOURCE_PHY, sata_phy->regs + SATA_REF_CTRL1);
+	writel(FIELD_PREP(SATA_P0_PHY_TX_AMPLITUDE_GEN1_MASK, 0x42) |
+	       FIELD_PREP(SATA_P0_PHY_TX_AMPLITUDE_GEN2_MASK, 0x46) |
+	       FIELD_PREP(SATA_P0_PHY_TX_AMPLITUDE_GEN3_MASK, 0x73),
+	       sata_phy->regs + SATA_PHY_CTRL0);
+	writel(FIELD_PREP(SATA_P0_PHY_TX_PREEMPH_GEN1_MASK, 0x5) |
+	       FIELD_PREP(SATA_P0_PHY_TX_PREEMPH_GEN2_MASK, 0x5) |
+	       FIELD_PREP(SATA_P0_PHY_TX_PREEMPH_GEN3_MASK, 0x8),
+	       sata_phy->regs + SATA_PHY_CTRL1);
+	writel(FIELD_PREP(SATA_LOS_LEVEL_MASK, 0x9) |
+	       FIELD_PREP(SATA_LOS_BIAS_MASK, 0x2),
+	       sata_phy->regs + SATA_LOS_IDEN);
+	writel(SATA_M_CSYSREQ | SATA_S_CSYSREQ,
+	       sata_phy->regs + SATA_AXI_LP_CTRL);
+	writel(SATA_REF_REPEATCLK_EN | SATA_REF_USE_PAD,
+	       sata_phy->regs + SATA_REG_CTRL);
+	writel(FIELD_PREP(SATA_MPLL_MULTIPLIER_MASK, 0x3c),
+	       sata_phy->regs + SATA_MPLL_CTRL);
+	usleep_range(15, 20);
+
+	/*
+	 * The SATA_RESET_CTRL register offset controls reset/deassert
+	 * for both the port and the PHY through the HSP bus,
+	 * and it is not registered in the reset tree.
+	 */
+	val = readl(sata_phy->regs + SATA_RESET_CTRL);
+	val &= ~(SATA_PHY_RESET | SATA_PORT_RESET);
+	writel(val, sata_phy->regs + SATA_RESET_CTRL);
+
+	ret = wait_for_phy_ready(sata_phy->regs, SATA_P0_PHY_STAT,
+				 SATA_P0_PHY_READY, 1);
+	if (ret < 0)
+		dev_err(&sata_phy->phy->dev,
+			"PHY READY check failed\n");
+	return ret;
+}
+
+static int eic7700_sata_phy_exit(struct phy *phy)
+{
+	struct eic7700_sata_phy *sata_phy = phy_get_drvdata(phy);
+	u32 val;
+
+	val = readl(sata_phy->regs + SATA_RESET_CTRL);
+	val |= SATA_PHY_RESET | SATA_PORT_RESET;
+	writel(val, sata_phy->regs + SATA_RESET_CTRL);
+
+	val = readl(sata_phy->regs + SATA_CLK_CTRL);
+	val &= ~SATA_SYS_CLK_EN;
+	writel(val, sata_phy->regs + SATA_CLK_CTRL);
+
+	return 0;
+}
+
+static const struct phy_ops eic7700_sata_phy_ops = {
+	.init		= eic7700_sata_phy_init,
+	.exit		= eic7700_sata_phy_exit,
+	.owner		= THIS_MODULE,
+};
+
+static int eic7700_sata_phy_probe(struct platform_device *pdev)
+{
+	struct eic7700_sata_phy *sata_phy;
+	struct device *dev = &pdev->dev;
+	struct phy_provider *phy_provider;
+	u32 val;
+	int ret;
+
+	sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
+	if (!sata_phy)
+		return -ENOMEM;
+
+	sata_phy->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sata_phy->regs))
+		return PTR_ERR(sata_phy->regs);
+
+	dev_set_drvdata(dev, sata_phy);
+
+	sata_phy->phy = devm_phy_create(dev, NULL, &eic7700_sata_phy_ops);
+	if (IS_ERR(sata_phy->phy)) {
+		dev_err(dev, "failed to create PHY\n");
+		ret = PTR_ERR(sata_phy->phy);
+		goto clk_disable;
+	}
+
+	phy_set_drvdata(sata_phy->phy, sata_phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		goto clk_disable;
+	}
+
+	return 0;
+
+clk_disable:
+	val = readl(sata_phy->regs + SATA_CLK_CTRL);
+	val &= ~SATA_SYS_CLK_EN;
+	writel(val, sata_phy->regs + SATA_CLK_CTRL);
+
+	return ret;
+}
+
+static const struct of_device_id eic7700_sata_phy_of_match[] = {
+	{ .compatible = "eswin,eic7700-sata-phy" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, eic7700_sata_phy_of_match);
+
+static struct platform_driver eic7700_sata_phy_driver = {
+	.probe	= eic7700_sata_phy_probe,
+	.driver = {
+		.of_match_table	= eic7700_sata_phy_of_match,
+		.name  = "eic7700-sata-phy",
+	}
+};
+module_platform_driver(eic7700_sata_phy_driver);
+
+MODULE_DESCRIPTION("SATA PHY driver for the ESWIN EIC7700 SoC");
+MODULE_AUTHOR("Yulin Lu <luyulin@eswincomputing.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci

[Krzysztof Kozlowski]

On 04/09/2025 08:37, Yulin Lu wrote:
> Add document for the SATA AHCI controller on the EIC7700 SoC platform,

Subject: drop "for", wrong grammar. "Document" is a verb.

> including descriptions of its hardware configurations.
> 
> Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>


...

> +
> +maintainers:
> +  - Yulin Lu <luyulin@eswincomputing.com>
> +  - Huan He <hehuan1@eswincomputing.com>
> +
> +description:
> +  This document defines device tree bindings for the Synopsys DWC

Describe the hardware, not the document. Entire line is completely
redundant.

> +  implementation of the AHCI SATA controller found in Eswin's
> +  Eic7700 SoC platform.
> +



...

> +  clock-names:
> +    items:
> +      - const: pclk
> +      - const: aclk
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: arst
> +
> +  ports-implemented:
> +    const: 1

I do not see how you addressed request about firmware. Nothing changed
here, no explanation in the commit msg.


Best regards,
Krzysztof

Re: [PATCH v3 2/3] dt-bindings: phy: eswin: Document for EIC7700 SoC SATA PHY

[Krzysztof Kozlowski]

On 04/09/2025 08:38, Yulin Lu wrote:

Same comment about subject.

> +  compatible:
> +    const: eswin,eic7700-sata-phy
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1


reg is the second property.

> +
> +required:
> +  - compatible
> +  - "#phy-cells"
> +  - reg


Same here

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci

[Niklas Cassel]

Hello Krzysztof, Rob,

On Thu, Sep 04, 2025 at 09:10:34AM +0200, Krzysztof Kozlowski wrote:
> > +
> > +  ports-implemented:
> > +    const: 1
> 
> I do not see how you addressed request about firmware. Nothing changed
> here, no explanation in the commit msg.

In Yulin's defence, he did comment that when having the Ports Implemented
register initialized by firmware, the Ports Implemented register apparently
gets cleared to zero when rmmoding the driver (probably because it disables
the clocks and regulators to the controller), thus this suggestion breaks
the use case of being able to reload the driver (rmmod + insmod).

He mentioned this, and asked for advice here:
https://lore.kernel.org/linux-ide/2cc9f2ff.6a2.198e04fd36e.Coremail.luyulin@eswincomputing.com/

After no reply he asked the same question again:
https://lore.kernel.org/linux-ide/692e11ca.843.198f0337528.Coremail.luyulin@eswincomputing.com/

I assume that Rob simply missed those messages.

Anyway, I provided my 50 cents here:
https://lore.kernel.org/linux-ide/aLBUC116MdJqDGIJ@flawful.org/

(I would like to add that I think it is the disabling of clocks and
regulators that causes the register to be cleared, since we do call
ahci_platform_assert_rsts() during the first probe, so if it was the reset
that cleared the register, the first probe should also not have worked.)


Not sure if it relevant to mention this reply to Rob's review comment in the
commit message, but perhaps it should have been mentioned in the change log.


Kind regards,
Niklas

Re: [PATCH v3 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci

[Krzysztof Kozlowski]

On 04/09/2025 11:14, Niklas Cassel wrote:
> Hello Krzysztof, Rob,
> 
> On Thu, Sep 04, 2025 at 09:10:34AM +0200, Krzysztof Kozlowski wrote:
>>> +
>>> +  ports-implemented:
>>> +    const: 1
>>
>> I do not see how you addressed request about firmware. Nothing changed
>> here, no explanation in the commit msg.
> 
> In Yulin's defence, he did comment that when having the Ports Implemented
> register initialized by firmware, the Ports Implemented register apparently
> gets cleared to zero when rmmoding the driver (probably because it disables
> the clocks and regulators to the controller), thus this suggestion breaks
> the use case of being able to reload the driver (rmmod + insmod).
> 
> He mentioned this, and asked for advice here:
> https://lore.kernel.org/linux-ide/2cc9f2ff.6a2.198e04fd36e.Coremail.luyulin@eswincomputing.com/
> 
> After no reply he asked the same question again:
> https://lore.kernel.org/linux-ide/692e11ca.843.198f0337528.Coremail.luyulin@eswincomputing.com/
> 
> I assume that Rob simply missed those messages.
> 
> Anyway, I provided my 50 cents here:
> https://lore.kernel.org/linux-ide/aLBUC116MdJqDGIJ@flawful.org/
> 
> (I would like to add that I think it is the disabling of clocks and
> regulators that causes the register to be cleared, since we do call
> ahci_platform_assert_rsts() during the first probe, so if it was the reset
> that cleared the register, the first probe should also not have worked.)
> 
> 
> Not sure if it relevant to mention this reply to Rob's review comment in the
> commit message, but perhaps it should have been mentioned in the change log.

Reviewer questions for more serious stuff happen for a reason, so when
discussion is resolved somehow differently than reviewer suggested, it
pretty often deserves explanation in commit msg.

Well, in changelog as absolute minimum. No explanation happened here in
the changelog, nor in the commit msg.

Best regards,
Krzysztof

Re: Re: [PATCH v3 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci

[luyulin]

Hello Krzysztof, Niklas, Rob,

Thank you very much for your suggestions and reply.

> 
> On 04/09/2025 11:14, Niklas Cassel wrote:
> > Hello Krzysztof, Rob,
> > 
> > On Thu, Sep 04, 2025 at 09:10:34AM +0200, Krzysztof Kozlowski wrote:
> >>> +
> >>> +  ports-implemented:
> >>> +    const: 1
> >>
> >> I do not see how you addressed request about firmware. Nothing changed
> >> here, no explanation in the commit msg.
> > 

...

> > 
> > Anyway, I provided my 50 cents here:
> > https://lore.kernel.org/linux-ide/aLBUC116MdJqDGIJ@flawful.org/
> > 
> > (I would like to add that I think it is the disabling of clocks and
> > regulators that causes the register to be cleared, since we do call
> > ahci_platform_assert_rsts() during the first probe, so if it was the reset
> > that cleared the register, the first probe should also not have worked.)
> > 

Thank you very much for your explanation. To add some context:
In our system, the ports-implemented register has already been configured by the firmware
(which is U-Boot on the HiFive Premier P550 board).
Therefore, when entering the kernel, the value of this register is correctly set to 0x1.

During probe, ahci_platform_enable_resources → ahci_platform_deassert_rsts is called.
And when the driver is removed, ahci_platform_disable_resources
→ ahci_platform_assert_rsts is triggered.
This reset operation causes the register to be restored to 0.
According to the IP databook, this register is indeed set to 0 after reset.

This is my understanding. I'd greatly appreciate it if you point out any issues.

> > 
> > Not sure if it relevant to mention this reply to Rob's review comment in the
> > commit message, but perhaps it should have been mentioned in the change log.
> 
> Reviewer questions for more serious stuff happen for a reason, so when
> discussion is resolved somehow differently than reviewer suggested, it
> pretty often deserves explanation in commit msg.
> 
> Well, in changelog as absolute minimum. No explanation happened here in
> the changelog, nor in the commit msg.

Thank you very much for your suggestion.
I'll add explanations in the commit message and changelogs for the next patch.

Best regards,
Yulin