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

Yulin Lu posted 3 patches 1 month, 2 weeks ago
[PATCH v2 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
Posted by Yulin Lu 1 month, 2 weeks ago
From: luyulin <luyulin@eswincomputing.com>

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

Signed-off-by: luyulin <luyulin@eswincomputing.com>
---
 .../bindings/ata/eswin,eic7700-ahci.yaml      | 92 +++++++++++++++++++
 1 file changed, 92 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..9ef58c9c2f28
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
@@ -0,0 +1,92 @@
+# 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
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  ports-implemented:
+    const: 1
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: aclk
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: arst
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: sata-phy
+
+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>;
+        ports-implemented = <0x1>;
+        clocks = <&gate_clk_hsp_cfgclk>, <&gate_clk_hsp_aclk>;
+        clock-names = "pclk", "aclk";
+        resets = <&reset 96>;
+        reset-names = "arst";
+        phys = <&sata_phy>;
+        phy-names = "sata-phy";
+    };
-- 
2.25.1
Re: [PATCH v2 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
Posted by Rob Herring 1 month, 2 weeks ago
On Tue, Aug 19, 2025 at 8:54 AM Yulin Lu <luyulin@eswincomputing.com> wrote:
>
> From: luyulin <luyulin@eswincomputing.com>

Please fix your name.

>
> Add document for the SATA AHCI controller on the EIC7700 SoC platform,
> including descriptions of its hardware configurations.
>
> Signed-off-by: luyulin <luyulin@eswincomputing.com>

And here.

> ---
>  .../bindings/ata/eswin,eic7700-ahci.yaml      | 92 +++++++++++++++++++
>  1 file changed, 92 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..9ef58c9c2f28
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
> @@ -0,0 +1,92 @@
> +# 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
> +
> +  reg:
> +    maxItems: 1

Drop. snps,dwc-ahci-common.yaml already defines this.

> +
> +  interrupts:
> +    maxItems: 1

Drop. snps,dwc-ahci-common.yaml already defines this.

> +
> +  ports-implemented:
> +    const: 1

Really, your firmware should initialize the DWC specific register that
sets this and is discoverable via a standard AHCI register.

> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: pclk
> +      - const: aclk
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: arst
> +
> +  phys:
> +    maxItems: 1

Drop. ahci-common.yaml already defines this.

> +
> +  phy-names:
> +    const: sata-phy

Drop. ahci-common.yaml already defines this.

> +
> +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>;
> +        ports-implemented = <0x1>;
> +        clocks = <&gate_clk_hsp_cfgclk>, <&gate_clk_hsp_aclk>;
> +        clock-names = "pclk", "aclk";
> +        resets = <&reset 96>;
> +        reset-names = "arst";
> +        phys = <&sata_phy>;
> +        phy-names = "sata-phy";
> +    };
> --
> 2.25.1
>
>
Re: Re: [PATCH v2 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
Posted by luyulin@eswincomputing.com 1 month ago
Hello, Rob

Thank you very much for your reply.
I have a question that I would like to seek your advice on and clarify.

> 
> On Tue, Aug 19, 2025 at 8:54 AM Yulin Lu <luyulin@eswincomputing.com> wrote:
> >
> > From: luyulin <luyulin@eswincomputing.com>
> 
> Please fix your name.
> 
> >
> > Add document for the SATA AHCI controller on the EIC7700 SoC platform,
> > including descriptions of its hardware configurations.
> >
> > Signed-off-by: luyulin <luyulin@eswincomputing.com>
> 
> And here.
> 
> > ---
> >  .../bindings/ata/eswin,eic7700-ahci.yaml      | 92 +++++++++++++++++++
> >  1 file changed, 92 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..9ef58c9c2f28
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
> > @@ -0,0 +1,92 @@
> > +# 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
> > +
> > +  reg:
> > +    maxItems: 1
> 
> Drop. snps,dwc-ahci-common.yaml already defines this.
> 
> > +
> > +  interrupts:
> > +    maxItems: 1
> 
> Drop. snps,dwc-ahci-common.yaml already defines this.
> 
> > +
> > +  ports-implemented:
> > +    const: 1
> 
> Really, your firmware should initialize the DWC specific register that
> sets this and is discoverable via a standard AHCI register.
> 

Accord to my understanding, if ports-implemented is configured in the dts,
this register will be set by the platform driver in libahci_platform.c.

Do you mean that ports-implemented should be removed from the dts,
and the corresponding register should be configured by the firmware
(which is U-Boot on the HiFive Premier P550 board)? Is this understanding correct?
If so, when the driver is removed, a reset will be triggered,
causing the configuration of this register to be lost,
which will result in an error when insmod the driver again.

Best regards,
Yulin
Re: Re: [PATCH v2 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
Posted by Niklas Cassel 1 month ago
On Thu, Aug 28, 2025 at 06:22:40PM +0800, luyulin@eswincomputing.com wrote:
> 
> Do you mean that ports-implemented should be removed from the dts,
> and the corresponding register should be configured by the firmware
> (which is U-Boot on the HiFive Premier P550 board)? Is this understanding correct?
> If so, when the driver is removed, a reset will be triggered,
> causing the configuration of this register to be lost,
> which will result in an error when insmod the driver again.

My 50 cents,

if the ports implemented register gets reset from the reset_control_reset()
in ahci_platform_assert_rsts(), then it seems like having ports-implemented
in device tree is acceptable.

There are a bunch of device trees that have this already:
arch/arm/boot/dts/qcom/qcom-apq8064.dtsi:                       ports-implemented = <0x1>;
arch/arm/boot/dts/qcom/qcom-ipq8064-v1.0.dtsi:                  ports-implemented = <0x1>;
arch/arm/boot/dts/qcom/qcom-ipq8064-v2.0.dtsi:  ports-implemented = <0x1>;
arch/arm/boot/dts/samsung/exynos5250.dtsi:                      ports-implemented = <0x1>;
arch/arm/boot/dts/socionext/uniphier-pro4.dtsi:                 ports-implemented = <1>;
arch/arm/boot/dts/socionext/uniphier-pro4.dtsi:                 ports-implemented = <1>;
arch/arm/boot/dts/socionext/uniphier-pxs2.dtsi:                 ports-implemented = <1>;
arch/arm/boot/dts/st/stih407-family.dtsi:                       ports-implemented = <0x1>;
arch/arm/boot/dts/st/stih407-family.dtsi:                       ports-implemented = <0x1>;
arch/arm/boot/dts/ti/omap/dra7-l4.dtsi:                         ports-implemented = <0x1>;
arch/arm/boot/dts/ti/omap/omap5-l4.dtsi:                                ports-implemented = <0x1>;
arch/arm64/boot/dts/mediatek/mt7622.dtsi:               ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk3568.dtsi:               ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk356x-base.dtsi:          ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk356x-base.dtsi:          ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk3576.dtsi:                       ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk3576.dtsi:                       ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:          ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:          ports-implemented = <0x1>;
arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi:         ports-implemented = <0x1>;
arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi:                       ports-implemented = <1>;
arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi:                       ports-implemented = <1>;


Sure, if the ports implemented register was sticky (kept its value after a
reset), then I think Rob's suggestion would make sense.



Kind regards,
Niklas
Re: Re: Re: [PATCH v2 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
Posted by luyulin@eswincomputing.com 1 month ago
> On Thu, Aug 28, 2025 at 06:22:40PM +0800, luyulin@eswincomputing.com wrote:
> > 
> > Do you mean that ports-implemented should be removed from the dts,
> > and the corresponding register should be configured by the firmware
> > (which is U-Boot on the HiFive Premier P550 board)? Is this understanding correct?
> > If so, when the driver is removed, a reset will be triggered,
> > causing the configuration of this register to be lost,
> > which will result in an error when insmod the driver again.
> 
> My 50 cents,
> 
> if the ports implemented register gets reset from the reset_control_reset()
> in ahci_platform_assert_rsts(), then it seems like having ports-implemented
> in device tree is acceptable.
> 

In our design, the ports implemented register gets reset from the ahci_platform_assert_rsts().

> There are a bunch of device trees that have this already:
> arch/arm/boot/dts/qcom/qcom-apq8064.dtsi:                       ports-implemented = <0x1>;
> arch/arm/boot/dts/qcom/qcom-ipq8064-v1.0.dtsi:                  ports-implemented = <0x1>;
> arch/arm/boot/dts/qcom/qcom-ipq8064-v2.0.dtsi:  ports-implemented = <0x1>;
> arch/arm/boot/dts/samsung/exynos5250.dtsi:                      ports-implemented = <0x1>;
> arch/arm/boot/dts/socionext/uniphier-pro4.dtsi:                 ports-implemented = <1>;
> arch/arm/boot/dts/socionext/uniphier-pro4.dtsi:                 ports-implemented = <1>;
> arch/arm/boot/dts/socionext/uniphier-pxs2.dtsi:                 ports-implemented = <1>;
> arch/arm/boot/dts/st/stih407-family.dtsi:                       ports-implemented = <0x1>;
> arch/arm/boot/dts/st/stih407-family.dtsi:                       ports-implemented = <0x1>;
> arch/arm/boot/dts/ti/omap/dra7-l4.dtsi:                         ports-implemented = <0x1>;
> arch/arm/boot/dts/ti/omap/omap5-l4.dtsi:                                ports-implemented = <0x1>;
> arch/arm64/boot/dts/mediatek/mt7622.dtsi:               ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk3568.dtsi:               ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk356x-base.dtsi:          ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk356x-base.dtsi:          ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk3576.dtsi:                       ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk3576.dtsi:                       ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:          ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:          ports-implemented = <0x1>;
> arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi:         ports-implemented = <0x1>;
> arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi:                       ports-implemented = <1>;
> arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi:                       ports-implemented = <1>;
> 
> 
> Sure, if the ports implemented register was sticky (kept its value after a
> reset), then I think Rob's suggestion would make sense.
> 
> 

Thank you very much for the clarification.

> 
> Kind regards,
> Niklas

Best regards,
Yulin
Re: Re: [PATCH v2 1/3] dt-bindings: ata: eswin: Document for EIC7700 SoC ahci
Posted by luyulin@eswincomputing.com 1 month, 1 week ago
Hi, Rob
Thank you very much for your professional response and suggestions.
Among the suggestions you mentioned, 
one point is not entirely clear, and I would like to seek your advice on it.
> 
> On Tue, Aug 19, 2025 at 8:54 AM Yulin Lu <luyulin@eswincomputing.com> wrote:
> >
> > From: luyulin <luyulin@eswincomputing.com>
> 
> Please fix your name.
> 
> >
> > Add document for the SATA AHCI controller on the EIC7700 SoC platform,
> > including descriptions of its hardware configurations.
> >
> > Signed-off-by: luyulin <luyulin@eswincomputing.com>
> 
> And here.
> 
> > ---
> >  .../bindings/ata/eswin,eic7700-ahci.yaml      | 92 +++++++++++++++++++
> >  1 file changed, 92 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..9ef58c9c2f28
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-ahci.yaml
> > @@ -0,0 +1,92 @@
> > +# 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
> > +
> > +  reg:
> > +    maxItems: 1
> 
> Drop. snps,dwc-ahci-common.yaml already defines this.
> 
> > +
> > +  interrupts:
> > +    maxItems: 1
> 
> Drop. snps,dwc-ahci-common.yaml already defines this.
> 
> > +
> > +  ports-implemented:
> > +    const: 1
> 
> Really, your firmware should initialize the DWC specific register that
> sets this and is discoverable via a standard AHCI register.
> 
Yes, I initialized the relevant registers during the uboot stage,
and they worked correctly after entering the Linux system.
However, after unloading and then reloading the ko driver,
a reset operation causes these registers to be initialized to 0,
leading to initialization errors when reloading the ko driver.
If I want to implement it this way, I need to modify the ahci_dwc.c driver
and add program handling tailored to our SoC design.

I searched the kernel for "snps,dwc-ahci" and found that
in manufacturers' dts files, such as rk3588-base.dtsi, dra7-l4.dtsi,
exynos5250.dtsi, etc., all include the ports-implemented field in their sata nodes. 
only the sata node in amd-seattle-soc.dtsi lacks the ports-implemented field
and does not have reset resources.
Additionally, in the YAML files under the /Documentation/ata directory that
integrate the DWC AHCI controller, the ports-implemented field has been implemented,
as seen in examples such as rockchip,dwc-ahci.yaml and baikal,bt1-ahci.yaml.

Therefore, I have questions I would like to confirm with you:
1. In your previous feedback, were you suggesting that I remove the ports-implemented field?
And what is the reason for doing so?
Based on your professional advice and considering our SoC design, do you think
it is acceptable to retain the ports-implemented field in the dts instead of
removing it and modifying the ahci_dwc.c driver?
2. I noticed that in the arch/ directory, chips like dra7-l4.dtsi and
exynos5250.dtsi integrate the DWC AHCI controller and include corresponding
hardware resource designs such as clocks and resets.
Why do they not have independent yaml files implemented in the kernel?

Best regards,
Yulin Lu