* [PATCH v1 0/2] ESWIN EIC7700 sata driver
@ 2025-05-15 8:51 hehuan1
2025-05-15 8:57 ` [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC hehuan1
2025-05-15 9:00 ` [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver hehuan1
0 siblings, 2 replies; 9+ messages in thread
From: hehuan1 @ 2025-05-15 8:51 UTC (permalink / raw)
To: dlemoal, cassel, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel
Cc: ningyu, linmin, luyulin, Huan He
From: Huan He <hehuan1@eswincomputing.com>
Implements support for the Eswin eic7700 SoC sata controller.
Provides basic functionality to initialize and manage the sata
controller for the eic7700 SoC. Integrates with the Linux AHCI subsystem
for standardized SATA host control and scalability.
Supported chips:
Eswin eic7700 SoC.
Test:
I tested this patch on the Sifive HiFive Premier P550 (which uses the EIC7700 SoC),
Ensure the hard drive is properly connected via the SATA interface on the hardware,
and confirm that the SATA controller within the chip can read/write to the hard drive
normally. The SATA driver patch is working properly.
Huan He (2):
dt-bindings: sata: eswin: Document for EIC7700 SoC
sata: eswin: Add eic7700 sata driver
.../bindings/ata/eswin,eic7700-sata.yaml | 80 ++++++
drivers/ata/Kconfig | 12 +
drivers/ata/Makefile | 1 +
drivers/ata/ahci_eic7700.c | 248 ++++++++++++++++++
4 files changed, 341 insertions(+)
create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
create mode 100644 drivers/ata/ahci_eic7700.c
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC
2025-05-15 8:51 [PATCH v1 0/2] ESWIN EIC7700 sata driver hehuan1
@ 2025-05-15 8:57 ` hehuan1
2025-05-15 10:18 ` Rob Herring (Arm)
2025-05-15 11:25 ` Niklas Cassel
2025-05-15 9:00 ` [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver hehuan1
1 sibling, 2 replies; 9+ messages in thread
From: hehuan1 @ 2025-05-15 8:57 UTC (permalink / raw)
To: dlemoal, cassel, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel
Cc: ningyu, linmin, luyulin, Huan He
From: Huan He <hehuan1@eswincomputing.com>
Add eic7700 AHCI SATA controller device with single port support.
For the eic7700 SATA registers, it supports AHCI standard interface,
interrupt modes (INTx/MSI/PME), APB reset control,
and HSP_SP_CSR register configuration.
Co-developed-by: Yulin Lu <luyulin@eswincomputing.com>
Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
Signed-off-by: Huan He <hehuan1@eswincomputing.com>
---
.../bindings/ata/eswin,eic7700-sata.yaml | 80 +++++++++++++++++++
1 file changed, 80 insertions(+)
create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
diff --git a/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml b/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
new file mode 100644
index 000000000000..71e1b865ed2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.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-sata.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 binding describes the SATA controller integrated in the Eswin EIC7700 SoC.
+ The controller is compatible with the AHCI (Advanced Host Controller Interface)
+ specification and supports up to 1 port.
+
+properties:
+ compatible:
+ const: eswin,eic7700-ahci
+
+ reg:
+ maxItems: 1
+ description: Address range of the SATA registers
+
+ interrupt-names:
+ items:
+ - const: intrq
+ - const: msi
+ - const: pme
+
+ interrupts:
+ maxItems: 3
+ description: The SATA interrupt numbers
+
+ ports-implemented:
+ maximum: 0x1
+
+ resets:
+ maxItems: 1
+ description: resets to be used by the controller.
+
+ reset-names:
+ const: apb
+
+ '#address-cells':
+ const: 2
+
+ '#size-cells':
+ const: 2
+
+ eswin,hsp_sp_csr:
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ description: hsp_sp_csr regs to be used by the controller.
+
+required:
+ - compatible
+ - reg
+ - interrupt-names
+ - interrupts
+ - resets
+ - reset-names
+ - eswin,hsp_sp_csr
+
+additionalProperties: false
+
+examples:
+ - |
+ sata: sata@50420000 {
+ compatible = "eswin,eic7700-ahci";
+ reg = <0x50420000 0x10000>;
+ interrupt-parent = <&plic>;
+ interrupt-names = "intrq", "msi", "pme";
+ interrupts = <58>, <59>, <60>;
+ ports-implemented = <0x1>;
+ resets = <&reset 7 (1 << 27)>;
+ reset-names = "apb";
+ #size-cells = <2>;
+ eswin,hsp_sp_csr = <&hsp_sp_csr 0x1050>;
+ };
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver
2025-05-15 8:51 [PATCH v1 0/2] ESWIN EIC7700 sata driver hehuan1
2025-05-15 8:57 ` [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC hehuan1
@ 2025-05-15 9:00 ` hehuan1
2025-05-15 11:56 ` Niklas Cassel
2025-05-16 14:33 ` kernel test robot
1 sibling, 2 replies; 9+ messages in thread
From: hehuan1 @ 2025-05-15 9:00 UTC (permalink / raw)
To: dlemoal, cassel, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel
Cc: ningyu, linmin, luyulin, Huan He
From: Huan He <hehuan1@eswincomputing.com>
Add support for the AHCI SATA controller in Eswin's eic7700 soc,
which supports SATA PHY initialization, reset control,
and power management.
Co-developed-by: Yulin Lu <luyulin@eswincomputing.com>
Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
Signed-off-by: Huan He <hehuan1@eswincomputing.com>
---
drivers/ata/Kconfig | 12 ++
drivers/ata/Makefile | 1 +
drivers/ata/ahci_eic7700.c | 248 +++++++++++++++++++++++++++++++++++++
3 files changed, 261 insertions(+)
create mode 100644 drivers/ata/ahci_eic7700.c
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e00536b49552..474c09543006 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -185,6 +185,18 @@ config AHCI_DWC
If unsure, say N.
+config AHCI_EIC7700
+ tristate "Eswin AHCI SATA support"
+ depends on ARCH_ESWIN || COMPILE_TEST
+ select SATA_HOST
+ help
+ This enables the AHCI SATA controller driver for Eswin SoCs. This driver
+ is specific to Eswin SoCs and should only be enabled if using such hardware.
+ The driver supports eic7700 series chips. The controller supports up
+ to 1 port.
+
+ If unsure, say N.
+
config AHCI_ST
tristate "ST AHCI SATA support"
depends on ARCH_STI || COMPILE_TEST
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 20e6645ab737..af00e55fa593 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_AHCI_CEVA) += ahci_ceva.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_DA850) += ahci_da850.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_DM816) += ahci_dm816.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_DWC) += ahci_dwc.o libahci.o libahci_platform.o
+obj-$(CONFIG_AHCI_EIC7700) += ahci_eic7700.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_IMX) += ahci_imx.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_MTK) += ahci_mtk.o libahci.o libahci_platform.o
obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o libahci.o libahci_platform.o
diff --git a/drivers/ata/ahci_eic7700.c b/drivers/ata/ahci_eic7700.c
new file mode 100644
index 000000000000..d2b7cafbfdd7
--- /dev/null
+++ b/drivers/ata/ahci_eic7700.c
@@ -0,0 +1,248 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ESWIN EIC7700 AHCI SATA Driver
+ *
+ * Copyright 2024, Beijing ESWIN Computing Technology Co., Ltd.. All rights reserved.
+ *
+ * Authors: Yulin Lu <luyulin@eswincomputing.com>
+ * Huan He <hehuan1@eswincomputing.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/device.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/libata.h>
+#include <linux/ahci_platform.h>
+#include <linux/acpi.h>
+#include <linux/pci_ids.h>
+#include <linux/iommu.h>
+#include <linux/mfd/syscon.h>
+#include <linux/bitfield.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include "ahci.h"
+
+#define DRV_NAME "ahci"
+
+/* Register Definitions */
+#define SATA_REF_CTRL1 0x338
+#define SATA_PHY_CTRL0 0x328
+#define SATA_PHY_CTRL1 0x32c
+#define SATA_LOS_IDEN 0x33c
+#define SATA_AXI_LP_CTRL 0x308
+#define SATA_REG_CTRL 0x334
+#define SATA_MPLL_CTRL 0x320
+#define SATA_RESET_CTRL 0x340
+#define SATA_RESET_CTRL_ASSERT 0x3
+#define SATA_RESET_CTRL_DEASSERT 0x0
+#define SATA_PHY_RESET BIT(0)
+#define SATA_P0_RESET BIT(1)
+#define SATA_LOS_LEVEL 0x9
+#define SATA_LOS_BIAS (0x02 << 16)
+#define SATA_REF_REPEATCLK_EN BIT(0)
+#define SATA_REF_USE_PAD BIT(20)
+#define SATA_P0_AMPLITUDE_GEN1 0x42
+#define SATA_P0_AMPLITUDE_GEN2 (0x46 << 8)
+#define SATA_P0_AMPLITUDE_GEN3 (0x73 << 16)
+#define SATA_P0_PHY_TX_PREEMPH_GEN1 0x05
+#define SATA_P0_PHY_TX_PREEMPH_GEN2 (0x05 << 8)
+#define SATA_P0_PHY_TX_PREEMPH_GEN3 (0x23 << 16)
+#define SATA_MPLL_MULTIPLIER (0x3c << 16)
+#define SATA_M_CSYSREQ BIT(0)
+#define SATA_S_CSYSREQ BIT(16)
+
+struct eswin_ahci_plat {
+ struct reset_control *apb_rst;
+};
+
+static const struct ata_port_info ahci_port_info = {
+ .flags = AHCI_FLAG_COMMON,
+ .pio_mask = ATA_PIO4,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &ahci_platform_ops,
+};
+
+static const struct ata_port_info ahci_port_info_nolpm = {
+ .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_LPM,
+ .pio_mask = ATA_PIO4,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &ahci_platform_ops,
+};
+
+static struct scsi_host_template ahci_platform_sht = {
+ AHCI_SHT(DRV_NAME),
+};
+
+static int eswin_sata_init(struct device *dev)
+{
+ struct regmap *regmap;
+
+ regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "eswin,hsp_sp_csr");
+ if (IS_ERR(regmap)) {
+ dev_dbg(dev, "No hsp_sp_csr phandle specified\n");
+ return -1;
+ }
+
+ regmap_write(regmap, SATA_REF_CTRL1, 0x1);
+ regmap_write(regmap, SATA_PHY_CTRL0, (SATA_P0_AMPLITUDE_GEN1 |
+ SATA_P0_AMPLITUDE_GEN2 |
+ SATA_P0_AMPLITUDE_GEN3));
+ regmap_write(regmap, SATA_PHY_CTRL1, (SATA_P0_PHY_TX_PREEMPH_GEN1 |
+ SATA_P0_PHY_TX_PREEMPH_GEN2 |
+ SATA_P0_PHY_TX_PREEMPH_GEN3));
+ regmap_write(regmap, SATA_LOS_IDEN, SATA_LOS_LEVEL | SATA_LOS_BIAS);
+ regmap_write(regmap, SATA_AXI_LP_CTRL, SATA_M_CSYSREQ | SATA_S_CSYSREQ);
+ regmap_write(regmap, SATA_REG_CTRL, SATA_REF_REPEATCLK_EN | SATA_REF_USE_PAD);
+ regmap_write(regmap, SATA_MPLL_CTRL, SATA_MPLL_MULTIPLIER);
+ regmap_write(regmap, SATA_RESET_CTRL, 0x0);
+
+ return 0;
+}
+
+static int eswin_ahci_platform_resets(struct ahci_host_priv *hpriv,
+ struct device *dev)
+{
+ struct eswin_ahci_plat *plat = hpriv->plat_data;
+ struct regmap *regmap;
+ int ret;
+
+ regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "eswin,hsp_sp_csr");
+ if (IS_ERR(regmap)) {
+ dev_dbg(dev, "No hsp_sp_csr phandle specified\n");
+ return -1;
+ }
+
+ plat->apb_rst = devm_reset_control_get_optional(dev, "apb");
+ if (PTR_ERR(plat->apb_rst) == -EPROBE_DEFER)
+ return PTR_ERR(plat->apb_rst);
+
+ ret = reset_control_assert(plat->apb_rst);
+ if (ret) {
+ dev_err(dev, "failed to assert apb_rst\n");
+ return ret;
+ }
+ regmap_write(regmap, SATA_RESET_CTRL, SATA_RESET_CTRL_ASSERT);
+
+ regmap_write(regmap, SATA_RESET_CTRL, SATA_RESET_CTRL_DEASSERT);
+ ret = reset_control_deassert(plat->apb_rst);
+ if (ret) {
+ dev_err(dev, "failed to deassert apb_rst\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ahci_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ahci_host_priv *hpriv;
+ struct eswin_ahci_plat *plat;
+ const struct ata_port_info *port;
+ int ret;
+
+ plat = devm_kzalloc(dev, sizeof(*plat), GFP_KERNEL);
+ if (!plat)
+ return -ENOMEM;
+
+ hpriv = ahci_platform_get_resources(pdev, 0);
+ if (IS_ERR(hpriv))
+ return PTR_ERR(hpriv);
+
+ hpriv->plat_data = plat;
+ ret = eswin_ahci_platform_resets(hpriv, dev);
+ if (ret)
+ return ret;
+
+ ret = ahci_platform_enable_resources(hpriv);
+ if (ret)
+ return ret;
+
+ eswin_sata_init(dev);
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(41));
+ if (ret)
+ return ret;
+
+ of_property_read_u32(dev->of_node, "ports-implemented", &hpriv->saved_port_map);
+
+ if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
+ hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;
+
+ port = acpi_device_get_match_data(dev);
+ if (!port)
+ port = &ahci_port_info;
+
+ ret = ahci_platform_init_host(pdev, hpriv, port, &ahci_platform_sht);
+ if (ret)
+ goto disable_resources;
+
+ return 0;
+
+disable_resources:
+ ahci_platform_disable_resources(hpriv);
+ return ret;
+}
+
+static void ahci_remove(struct platform_device *pdev)
+{
+ ata_platform_remove_one(pdev);
+}
+
+static int eswin_ahci_suspend(struct device *dev)
+{
+ int ret;
+
+ ret = ahci_platform_suspend(dev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int eswin_ahci_resume(struct device *dev)
+{
+ int ret;
+
+ ret = ahci_platform_resume(dev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(ahci_pm_ops, eswin_ahci_suspend, eswin_ahci_resume);
+
+static const struct of_device_id ahci_of_match[] = {
+ { .compatible = "eswin,eic7700-ahci", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+
+static const struct acpi_device_id ahci_acpi_match[] = {
+ { "APMC0D33", (unsigned long)&ahci_port_info_nolpm },
+ { ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI, 0xffffff) },
+ {},
+};
+MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
+
+static struct platform_driver ahci_driver = {
+ .probe = ahci_probe,
+ .remove = ahci_remove,
+ .shutdown = ahci_platform_shutdown,
+ .driver = {
+ .name = DRV_NAME,
+ .of_match_table = ahci_of_match,
+ .acpi_match_table = ahci_acpi_match,
+ .pm = &ahci_pm_ops,
+ },
+};
+module_platform_driver(ahci_driver);
+
+MODULE_DESCRIPTION("ESWIN AHCI SATA driver");
+MODULE_AUTHOR("Yulin Lu <luyulin@eswincomputing.com>");
+MODULE_AUTHOR("Huan He <hehuan1@eswincomputing.com>");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC
2025-05-15 8:57 ` [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC hehuan1
@ 2025-05-15 10:18 ` Rob Herring (Arm)
2025-05-15 11:25 ` Niklas Cassel
1 sibling, 0 replies; 9+ messages in thread
From: Rob Herring (Arm) @ 2025-05-15 10:18 UTC (permalink / raw)
To: hehuan1
Cc: dlemoal, linmin, linux-kernel, p.zabel, linux-ide, conor+dt,
ningyu, krzk+dt, cassel, devicetree, luyulin
On Thu, 15 May 2025 16:57:23 +0800, hehuan1@eswincomputing.com wrote:
> From: Huan He <hehuan1@eswincomputing.com>
>
> Add eic7700 AHCI SATA controller device with single port support.
> For the eic7700 SATA registers, it supports AHCI standard interface,
> interrupt modes (INTx/MSI/PME), APB reset control,
> and HSP_SP_CSR register configuration.
>
> Co-developed-by: Yulin Lu <luyulin@eswincomputing.com>
> Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> Signed-off-by: Huan He <hehuan1@eswincomputing.com>
> ---
> .../bindings/ata/eswin,eic7700-sata.yaml | 80 +++++++++++++++++++
> 1 file changed, 80 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.example.dtb: sata@50420000 (eswin,eic7700-ahci): #size-cells: 0 was expected
from schema $id: http://devicetree.org/schemas/ata/sata-common.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.example.dtb: sata@50420000 (eswin,eic7700-ahci): '#address-cells' is a dependency of '#size-cells'
from schema $id: http://devicetree.org/schemas/reg.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.example.dtb: sata@50420000 (eswin,eic7700-ahci): 'eswin,hsp_sp_csr' does not match any of the regexes: '^#.*', '^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*', '^(keypad|m25p|max8952|max8997|max8998|mpmc),.*', '^(pciclass|pinctrl-single|#pinctrl-single|PowerPC),.*', '^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*', '^(simple-audio-card|st-plgpio|st-spics|ts),.*', '^100ask,.*', '^70mai,.*', '^8dev,.*', '^GEFanuc,.*', '^IBM,.*', '^ORCL,.*', '^SUNW,.*', '^[a-zA-Z0-9#_][a-zA-Z0-9+\\-._@]{0,63}$', '^[a-zA-Z0-9+\\-._]*@[0-9a-zA-Z,]*$', '^abb,.*', '^abilis,.*', '^abracon,.*', '^abt,.*', '^acbel,.*', '^acelink,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^adieng,.*', '^admatec,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^airoha,.*', '^al,.*', '^alcatel,.*', '^aldec,.*', '^alfa-network,.*', '^allegro,.*', '^allegromicro,.*', '^alliedvision,.*', '^allo,.*', '^allwinner,.*', '^alphascale,.*', '^alps,.*', '^alt,.*', '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^amphenol,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^anbernic,.*', '^andestech,.*', '^anvo,.*', '^aoly,.*', '^aosong,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arcom,.*', '^arctic,.*', '^arcx,.*', '^ariaboard,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^armsom,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^asix,.*', '^aspeed,.*', '^asrock,.*', '^asteralabs,.*', '^asus,.*', '^atheros,.*', '^atlas,.*', '^atmel,.*', '^auo,.*', '^auvidea,.*', '^avago,.*', '^avia,.*', '^avic,.*', '^avnet,.*', '^awinic,.*', '^axentia,.*', '^axis,.*', '^azoteq,.*', '^azw,.*', '^baikal,.*', '^bananapi,.*', '^beacon,.*', '^beagle,.*', '^belling,.*', '^bhf,.*', '^bigtreetech,.*', '^bitmain,.*', '^blaize,.*', '^blutek,.*', '^boe,.*', '^bosch,.*', '^boundary,.*', '^brcm,.*', '^broadmobi,.*', '^bsh,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^bytedance,.*', '^calamp,.*', '^calao,.*', '^calaosystems,.*', '^calxeda,.*', '^cameo,.*', '^canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalyst,.*', '^cavium,.*', '^cct,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^chargebyte,.*', '^checkpoint,.*', '^chefree,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chongzhou,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^clockwork,.*', '^cloos,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^comvetia,.*', '^congatec,.*', '^coolpi,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csot,.*', '^csq,.*', '^ctera,.*', '^ctu,.*', '^cubietech,.*', '^cudy,.*', '^cui,.*', '^cypress,.*', '^cyx,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^deepcomputing,.*', '^dell,.*', '^delta,.*', '^densitron,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dfrobot,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^dimonoff,.*', '^diodes,.*', '^dioo,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,.*', '^dragino,.*', '^dream,.*', '^ds,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebbg,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^econet,.*', '^edgeble,.*', '^edimax,.*', '^edt,.*', '^ees,.*', '^eeti,.*', '^einfochips,.*', '^eink,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^elpida,.*', '^embedfire,.*', '^embest,.*', '^emcraft,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^enclustra,.*', '^endless,.*', '^ene,.*', '^energymicro,.*', '^engicam,.*', '^engleder,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^exegin,.*', '^ezchip,.*', '^facebook,.*', '^fairchild,.*', '^fairphone,.*', '^faraday,.*', '^fascontek,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^forlinx,.*', '^freebox,.*', '^freecom,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^fxtec,.*', '^galaxycore,.*', '^gameforce,.*', '^gardena,.*', '^gateway,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying,.*', '^gef,.*', '^gehc,.*', '^gemei,.*', '^gemtek,.*', '^genesys,.*', '^genexis,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^glinet,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^gocontroll,.*', '^goldelico,.*', '^goodix,.*', '^google,.*', '^goramo,.*', '^gplus,.*', '^grinn,.*', '^grmn,.*', '^gumstix,.*', '^gw,.*', '^hannstar,.*', '^haochuangyi,.*', '^haoyu,.*', '^hardkernel,.*', '^hechuang,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisi,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperf,.*', '^hoperun,.*', '^hp,.*', '^hpe,.*', '^hsg,.*', '^htc,.*', '^huawei,.*', '^hugsun,.*', '^hwacom,.*', '^hxt,.*', '^hycon,.*', '^hydis,.*', '^hynitron,.*', '^hynix,.*', '^hyundai,.*', '^i2se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^iei,.*', '^ifi,.*', '^ilitek,.*', '^imagis,.*', '^img,.*', '^imi,.*', '^inanbo,.*', '^incircuit,.*', '^indiedroid,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^ingrasys,.*', '^injoinic,.*', '^innocomm,.*', '^innolux,.*', '^inside-secure,.*', '^insignal,.*', '^inspur,.*', '^intel,.*', '^intercontrol,.*', '^invensense,.*', '^inventec,.*', '^inversepath,.*', '^iom,.*', '^irondevice,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^itian,.*', '^ivo,.*', '^iwave,.*', '^jadard,.*', '^jasonic,.*', '^jdi,.*', '^jedec,.*', '^jenson,.*', '^jesurun,.*', '^jethome,.*', '^jianda,.*', '^jide,.*', '^joz,.*', '^kam,.*', '^karo,.*', '^keithkoep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^lckfb,.*', '^lctech,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^lincolntech,.*', '^lineartechnology,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^liontron,.*', '^liteon,.*', '^litex,.*', '^lltc,.*', '^logicpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongmasses,.*', '^loongson,.*', '^lsi,.*', '^lunzn,.*', '^luxul,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marantec,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^maxlinear,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsensing,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^methode,.*', '^micrel,.*', '^microchip,.*', '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^microtips,.*', '^mikroe,.*', '^mikrotik,.*', '^milkv,.*', '^miniand,.*', '^minix,.*', '^mips,.*', '^miramems,.*', '^mitsubishi,.*', '^mitsumi,.*', '^mixel,.*', '^miyoo,.*', '^mntre,.*', '^mobileye,.*', '^modtronix,.*', '^moortec,.*', '^mosaixtech,.*', '^motorcomm,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxic,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^neardi,.*', '^nec,.*', '^neofidelity,.*', '^neonode,.*', '^netcube,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.*', '^newvision,.*', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^nothing,.*', '^novatek,.*', '^novtech,.*', '^numonyx,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^ocs,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^oneplus,.*', '^onie,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^openailab,.*', '^opencores,.*', '^openembed,.*', '^openpandora,.*', '^openrisc,.*', '^openwrt,.*', '^option,.*', '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^osmc,.*', '^ouya,.*', '^overkiz,.*', '^ovti,.*', '^oxsemi,.*', '^ozzmaker,.*', '^panasonic,.*', '^parade,.*', '^parallax,.*', '^pda,.*', '^pegatron,.*', '^pericom,.*', '^pervasive,.*', '^phicomm,.*', '^phytec,.*', '^picochip,.*', '^pinctrl-[0-9]+$', '^pine64,.*', '^pineriver,.*', '^pixcir,.*', '^plantower,.*', '^plathome,.*', '^plda,.*', '^plx,.*', '^ply,.*', '^pni,.*', '^pocketbook,.*', '^polaroid,.*', '^polyhex,.*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^powkiddy,.*', '^pri,.*', '^primeview,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^puya,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^qihua,.*', '^qishenglong,.*', '^qnap,.*', '^quanta,.*', '^radxa,.*', '^raidsonic,.*', '^ralink,.*', '^ramtron,.*', '^raspberrypi,.*', '^raydium,.*', '^rda,.*', '^realtek,.*', '^relfor,.*', '^remarkable,.*', '^renesas,.*', '^rervision,.*', '^retronix,.*', '^revotics,.*', '^rex,.*', '^richtek,.*', '^ricoh,.*', '^rikomagic,.*', '^riot,.*', '^riscv,.*', '^rockchip,.*', '^rocktech,.*', '^rohm,.*', '^ronbo,.*', '^roofull,.*', '^roseapplepi,.*', '^rve,.*', '^saef,.*', '^samsung,.*', '^samtec,.*', '^sancloud,.*', '^sandisk,.*', '^satoz,.*', '^sbs,.*', '^schindler,.*', '^schneider,.*', '^sciosense,.*', '^seagate,.*', '^seeed,.*', '^seirobotics,.*', '^semtech,.*', '^senseair,.*', '^sensirion,.*', '^sensortek,.*', '^sercomm,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shift,.*', '^shimafuji,.*', '^shineworld,.*', '^shiratech,.*', '^si-en,.*', '^si-linux,.*', '^siemens,.*', '^sifive,.*', '^siflower,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silan,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sinowealth,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skov,.*', '^skyworks,.*', '^smartlabs,.*', '^smartrg,.*', '^smi,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^sophgo,.*', '^sourceparts,.*', '^spacemit,.*', '^spansion,.*', '^sparkfun,.*', '^spinalhdl,.*', '^sprd,.*', '^square,.*', '^ssi,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-ericsson,.*', '^starfive,.*', '^starry,.*', '^startek,.*', '^starterkit,.*', '^ste,.*', '^stericsson,.*', '^storlink,.*', '^storm,.*', '^storopack,.*', '^summit,.*', '^sunchip,.*', '^sundance,.*', '^sunplus,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^synopsys,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tcu,.*', '^tdo,.*', '^team-source-display,.*', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^techwell,.*', '^teejet,.*', '^teltonika,.*', '^tempo,.*', '^terasic,.*', '^tesla,.*', '^test,.*', '^tfc,.*', '^thead,.*', '^thine,.*', '^thingyjp,.*', '^thundercomm,.*', '^thwc,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^topic,.*', '^topland,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^transpeed,.*', '^traverse,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^turing,.*', '^tyan,.*', '^tyhx,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ufispace,.*', '^ugoos,.*', '^ultratronik,.*', '^uni-t,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^usr,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*', '^vamrs,.*', '^variscite,.*', '^vdl,.*', '^vertexcom,.*', '^via,.*', '^vialab,.*', '^vicor,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vivax,.*', '^vocore,.*', '^voipac,.*', '^voltafield,.*', '^vot,.*', '^vscom,.*', '^vxt,.*', '^wacom,.*', '^wanchanglong,.*', '^wand,.*', '^waveshare,.*', '^wd,.*', '^we,.*', '^welltech,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^widora,.*', '^wiligear,.*', '^willsemi,.*', '^winbond,.*', '^wingtech,.*', '^winlink,.*', '^winsen,.*', '^winstar,.*', '^wirelesstag,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^wolfvision,.*', '^x-powers,.*', '^xen,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yadro,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^yiming,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^yuridenki,.*', '^yuzukihd,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*'
from schema $id: http://devicetree.org/schemas/vendor-prefixes.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250515085723.1706-1-hehuan1@eswincomputing.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC
2025-05-15 8:57 ` [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC hehuan1
2025-05-15 10:18 ` Rob Herring (Arm)
@ 2025-05-15 11:25 ` Niklas Cassel
2025-08-14 9:51 ` luyulin
1 sibling, 1 reply; 9+ messages in thread
From: Niklas Cassel @ 2025-05-15 11:25 UTC (permalink / raw)
To: hehuan1
Cc: dlemoal, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel, ningyu, linmin, luyulin, Serge Semin
Hello Huan He,
On Thu, May 15, 2025 at 04:57:23PM +0800, hehuan1@eswincomputing.com wrote:
> From: Huan He <hehuan1@eswincomputing.com>
>
> Add eic7700 AHCI SATA controller device with single port support.
> For the eic7700 SATA registers, it supports AHCI standard interface,
> interrupt modes (INTx/MSI/PME), APB reset control,
> and HSP_SP_CSR register configuration.
>
> Co-developed-by: Yulin Lu <luyulin@eswincomputing.com>
> Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> Signed-off-by: Huan He <hehuan1@eswincomputing.com>
> ---
> .../bindings/ata/eswin,eic7700-sata.yaml | 80 +++++++++++++++++++
> 1 file changed, 80 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
>
> diff --git a/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml b/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
> new file mode 100644
> index 000000000000..71e1b865ed2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.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-sata.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 binding describes the SATA controller integrated in the Eswin EIC7700 SoC.
> + The controller is compatible with the AHCI (Advanced Host Controller Interface)
> + specification and supports up to 1 port.
> +
> +properties:
> + compatible:
> + const: eswin,eic7700-ahci
> +
> + reg:
> + maxItems: 1
> + description: Address range of the SATA registers
> +
> + interrupt-names:
> + items:
> + - const: intrq
> + - const: msi
> + - const: pme
> +
> + interrupts:
> + maxItems: 3
> + description: The SATA interrupt numbers
> +
> + ports-implemented:
> + maximum: 0x1
> +
> + resets:
> + maxItems: 1
> + description: resets to be used by the controller.
> +
> + reset-names:
> + const: apb
> +
> + '#address-cells':
> + const: 2
> +
> + '#size-cells':
> + const: 2
> +
> + eswin,hsp_sp_csr:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description: hsp_sp_csr regs to be used by the controller.
> +
> +required:
> + - compatible
> + - reg
> + - interrupt-names
> + - interrupts
> + - resets
> + - reset-names
> + - eswin,hsp_sp_csr
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + sata: sata@50420000 {
> + compatible = "eswin,eic7700-ahci";
> + reg = <0x50420000 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupt-names = "intrq", "msi", "pme";
> + interrupts = <58>, <59>, <60>;
> + ports-implemented = <0x1>;
> + resets = <&reset 7 (1 << 27)>;
> + reset-names = "apb";
> + #size-cells = <2>;
> + eswin,hsp_sp_csr = <&hsp_sp_csr 0x1050>;
> + };
> --
> 2.25.1
>
I'm surprised that you AHCI controller does not need any clocks ;)
When looking at the EIC7700X TRM:
https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases/download/v1.0.0-20250103/EIC7700X_SoC_Technical_Reference_Manual_Part2.pdf
It is obvious that this SoC integrates the DWC AHCI controller.
Thus, I would have expected your DT binding to have a:
$ref: snps,dwc-ahci-common.yaml#
Please have a look at these bindings:
baikal,bt1-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
baikal,bt1-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
rockchip,dwc-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
rockchip,dwc-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
snps,dwc-ahci-common.yaml:$id: http://devicetree.org/schemas/ata/snps,dwc-ahci-common.yaml#
snps,dwc-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
snps,dwc-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
The good news is that snps,dwc-ahci-common.yaml has defined and documented
all the SATA clocks and resets for your board already (a lot of them which
you missed to include in this binding).
Looking quickly at:
eswin,hsp_sp_csr = <&hsp_sp_csr 0x1050>;
I can't help to wonder if these regs shouldn't be in a SATA PHY binding
instead.
Do e.g. a
$ git grep -A 20 snps,dwc-ahci arch/
There are multiple examples that use a PHY driver.
If you were to implement a PHY driver, it is possible that you would
not need to create a new (AHCI) DT binding at all, you could probably
just add your compatible string to snps,dwc-ahci.yaml, as (from a quick)
glance, all the only platform specific things appear to be PHY related.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver
2025-05-15 9:00 ` [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver hehuan1
@ 2025-05-15 11:56 ` Niklas Cassel
2025-05-16 14:33 ` kernel test robot
1 sibling, 0 replies; 9+ messages in thread
From: Niklas Cassel @ 2025-05-15 11:56 UTC (permalink / raw)
To: hehuan1
Cc: dlemoal, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel, ningyu, linmin, luyulin, Serge Semin
Hello Huan He,
On Thu, May 15, 2025 at 05:00:18PM +0800, hehuan1@eswincomputing.com wrote:
> From: Huan He <hehuan1@eswincomputing.com>
>
> Add support for the AHCI SATA controller in Eswin's eic7700 soc,
> which supports SATA PHY initialization, reset control,
> and power management.
>
> Co-developed-by: Yulin Lu <luyulin@eswincomputing.com>
> Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> Signed-off-by: Huan He <hehuan1@eswincomputing.com>
> ---
> drivers/ata/Kconfig | 12 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/ahci_eic7700.c | 248 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 261 insertions(+)
> create mode 100644 drivers/ata/ahci_eic7700.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e00536b49552..474c09543006 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -185,6 +185,18 @@ config AHCI_DWC
>
> If unsure, say N.
>
> +config AHCI_EIC7700
> + tristate "Eswin AHCI SATA support"
> + depends on ARCH_ESWIN || COMPILE_TEST
> + select SATA_HOST
> + help
> + This enables the AHCI SATA controller driver for Eswin SoCs. This driver
> + is specific to Eswin SoCs and should only be enabled if using such hardware.
> + The driver supports eic7700 series chips. The controller supports up
> + to 1 port.
> +
> + If unsure, say N.
> +
> config AHCI_ST
> tristate "ST AHCI SATA support"
> depends on ARCH_STI || COMPILE_TEST
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 20e6645ab737..af00e55fa593 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_AHCI_CEVA) += ahci_ceva.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_DA850) += ahci_da850.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_DM816) += ahci_dm816.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_DWC) += ahci_dwc.o libahci.o libahci_platform.o
> +obj-$(CONFIG_AHCI_EIC7700) += ahci_eic7700.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_IMX) += ahci_imx.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_MTK) += ahci_mtk.o libahci.o libahci_platform.o
> obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o libahci.o libahci_platform.o
> diff --git a/drivers/ata/ahci_eic7700.c b/drivers/ata/ahci_eic7700.c
> new file mode 100644
> index 000000000000..d2b7cafbfdd7
> --- /dev/null
> +++ b/drivers/ata/ahci_eic7700.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ESWIN EIC7700 AHCI SATA Driver
> + *
> + * Copyright 2024, Beijing ESWIN Computing Technology Co., Ltd.. All rights reserved.
> + *
> + * Authors: Yulin Lu <luyulin@eswincomputing.com>
> + * Huan He <hehuan1@eswincomputing.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/device.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/acpi.h>
> +#include <linux/pci_ids.h>
> +#include <linux/iommu.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include "ahci.h"
> +
> +#define DRV_NAME "ahci"
> +
> +/* Register Definitions */
> +#define SATA_REF_CTRL1 0x338
> +#define SATA_PHY_CTRL0 0x328
> +#define SATA_PHY_CTRL1 0x32c
> +#define SATA_LOS_IDEN 0x33c
> +#define SATA_AXI_LP_CTRL 0x308
> +#define SATA_REG_CTRL 0x334
> +#define SATA_MPLL_CTRL 0x320
> +#define SATA_RESET_CTRL 0x340
> +#define SATA_RESET_CTRL_ASSERT 0x3
> +#define SATA_RESET_CTRL_DEASSERT 0x0
> +#define SATA_PHY_RESET BIT(0)
> +#define SATA_P0_RESET BIT(1)
> +#define SATA_LOS_LEVEL 0x9
> +#define SATA_LOS_BIAS (0x02 << 16)
> +#define SATA_REF_REPEATCLK_EN BIT(0)
> +#define SATA_REF_USE_PAD BIT(20)
> +#define SATA_P0_AMPLITUDE_GEN1 0x42
> +#define SATA_P0_AMPLITUDE_GEN2 (0x46 << 8)
> +#define SATA_P0_AMPLITUDE_GEN3 (0x73 << 16)
> +#define SATA_P0_PHY_TX_PREEMPH_GEN1 0x05
> +#define SATA_P0_PHY_TX_PREEMPH_GEN2 (0x05 << 8)
> +#define SATA_P0_PHY_TX_PREEMPH_GEN3 (0x23 << 16)
> +#define SATA_MPLL_MULTIPLIER (0x3c << 16)
> +#define SATA_M_CSYSREQ BIT(0)
> +#define SATA_S_CSYSREQ BIT(16)
> +
> +struct eswin_ahci_plat {
> + struct reset_control *apb_rst;
> +};
> +
> +static const struct ata_port_info ahci_port_info = {
> + .flags = AHCI_FLAG_COMMON,
> + .pio_mask = ATA_PIO4,
> + .udma_mask = ATA_UDMA6,
> + .port_ops = &ahci_platform_ops,
> +};
> +
> +static const struct ata_port_info ahci_port_info_nolpm = {
> + .flags = AHCI_FLAG_COMMON | ATA_FLAG_NO_LPM,
> + .pio_mask = ATA_PIO4,
> + .udma_mask = ATA_UDMA6,
> + .port_ops = &ahci_platform_ops,
> +};
> +
> +static struct scsi_host_template ahci_platform_sht = {
> + AHCI_SHT(DRV_NAME),
> +};
> +
> +static int eswin_sata_init(struct device *dev)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "eswin,hsp_sp_csr");
> + if (IS_ERR(regmap)) {
> + dev_dbg(dev, "No hsp_sp_csr phandle specified\n");
> + return -1;
> + }
> +
> + regmap_write(regmap, SATA_REF_CTRL1, 0x1);
> + regmap_write(regmap, SATA_PHY_CTRL0, (SATA_P0_AMPLITUDE_GEN1 |
> + SATA_P0_AMPLITUDE_GEN2 |
> + SATA_P0_AMPLITUDE_GEN3));
> + regmap_write(regmap, SATA_PHY_CTRL1, (SATA_P0_PHY_TX_PREEMPH_GEN1 |
> + SATA_P0_PHY_TX_PREEMPH_GEN2 |
> + SATA_P0_PHY_TX_PREEMPH_GEN3));
> + regmap_write(regmap, SATA_LOS_IDEN, SATA_LOS_LEVEL | SATA_LOS_BIAS);
> + regmap_write(regmap, SATA_AXI_LP_CTRL, SATA_M_CSYSREQ | SATA_S_CSYSREQ);
> + regmap_write(regmap, SATA_REG_CTRL, SATA_REF_REPEATCLK_EN | SATA_REF_USE_PAD);
> + regmap_write(regmap, SATA_MPLL_CTRL, SATA_MPLL_MULTIPLIER);
> + regmap_write(regmap, SATA_RESET_CTRL, 0x0);
> +
> + return 0;
> +}
As I wrote in patch 1/2, I can't help to wonder if these shouldn't be in a
PHY driver instead.
If it were, you probably wouldn't need this file, as most of it looks like
is it already handled by ahci_dwc.c / libahci_platform.c.
(phy_init() is called from ahci_platform_enable_phys(), which is called
from ahci_platform_enable_resources() which is called from
ahci_dwc_init_host() which is called from ahci_dwc_probe().)
> +
> +static int eswin_ahci_platform_resets(struct ahci_host_priv *hpriv,
> + struct device *dev)
> +{
> + struct eswin_ahci_plat *plat = hpriv->plat_data;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "eswin,hsp_sp_csr");
> + if (IS_ERR(regmap)) {
> + dev_dbg(dev, "No hsp_sp_csr phandle specified\n");
> + return -1;
> + }
> +
> + plat->apb_rst = devm_reset_control_get_optional(dev, "apb");
> + if (PTR_ERR(plat->apb_rst) == -EPROBE_DEFER)
> + return PTR_ERR(plat->apb_rst);
> +
> + ret = reset_control_assert(plat->apb_rst);
> + if (ret) {
> + dev_err(dev, "failed to assert apb_rst\n");
> + return ret;
> + }
> + regmap_write(regmap, SATA_RESET_CTRL, SATA_RESET_CTRL_ASSERT);
> +
> + regmap_write(regmap, SATA_RESET_CTRL, SATA_RESET_CTRL_DEASSERT);
> + ret = reset_control_deassert(plat->apb_rst);
> + if (ret) {
> + dev_err(dev, "failed to deassert apb_rst\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ahci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct ahci_host_priv *hpriv;
> + struct eswin_ahci_plat *plat;
> + const struct ata_port_info *port;
> + int ret;
> +
> + plat = devm_kzalloc(dev, sizeof(*plat), GFP_KERNEL);
> + if (!plat)
> + return -ENOMEM;
> +
> + hpriv = ahci_platform_get_resources(pdev, 0);
> + if (IS_ERR(hpriv))
> + return PTR_ERR(hpriv);
> +
> + hpriv->plat_data = plat;
> + ret = eswin_ahci_platform_resets(hpriv, dev);
> + if (ret)
> + return ret;
> +
> + ret = ahci_platform_enable_resources(hpriv);
> + if (ret)
> + return ret;
> +
> + eswin_sata_init(dev);
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(41));
This looks wrong.
If I'm not mistaken, the proper way is that the bus which your device
(in this case an AHCI controller) is located on in device tree should
specify 'dma-ranges', and that will set the DMA mask for the devices
on that bus automatically.
> + if (ret)
> + return ret;
> +
> + of_property_read_u32(dev->of_node, "ports-implemented", &hpriv->saved_port_map);
> +
> + if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci"))
> + hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ;
"hisilicon,hisi-ahci" seems copy pasted from ahci_platform.c, and seems unwanted.
> +
> + port = acpi_device_get_match_data(dev);
> + if (!port)
> + port = &ahci_port_info;
> +
> + ret = ahci_platform_init_host(pdev, hpriv, port, &ahci_platform_sht);
> + if (ret)
> + goto disable_resources;
> +
> + return 0;
> +
> +disable_resources:
> + ahci_platform_disable_resources(hpriv);
> + return ret;
> +}
> +
> +static void ahci_remove(struct platform_device *pdev)
> +{
> + ata_platform_remove_one(pdev);
> +}
> +
> +static int eswin_ahci_suspend(struct device *dev)
> +{
> + int ret;
> +
> + ret = ahci_platform_suspend(dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int eswin_ahci_resume(struct device *dev)
> +{
> + int ret;
> +
> + ret = ahci_platform_resume(dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ahci_pm_ops, eswin_ahci_suspend, eswin_ahci_resume);
> +
> +static const struct of_device_id ahci_of_match[] = {
> + { .compatible = "eswin,eic7700-ahci", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ahci_of_match);
> +
> +static const struct acpi_device_id ahci_acpi_match[] = {
> + { "APMC0D33", (unsigned long)&ahci_port_info_nolpm },
> + { ACPI_DEVICE_CLASS(PCI_CLASS_STORAGE_SATA_AHCI, 0xffffff) },
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, ahci_acpi_match);
This table seems copy pasted from ahci_platform.c, and seems unwanted.
(I assume that your platform does not support ACPI.)
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver
2025-05-15 9:00 ` [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver hehuan1
2025-05-15 11:56 ` Niklas Cassel
@ 2025-05-16 14:33 ` kernel test robot
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2025-05-16 14:33 UTC (permalink / raw)
To: hehuan1, dlemoal, cassel, robh, krzk+dt, conor+dt, linux-ide,
devicetree, linux-kernel, p.zabel
Cc: oe-kbuild-all, ningyu, linmin, luyulin, Huan He
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.15-rc6 next-20250516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/hehuan1-eswincomputing-com/dt-bindings-sata-eswin-Document-for-EIC7700-SoC/20250515-170607
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20250515090018.1720-1-hehuan1%40eswincomputing.com
patch subject: [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250516/202505162248.OhCrILxm-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250516/202505162248.OhCrILxm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505162248.OhCrILxm-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/ata/ahci_eic7700.c:206:12: warning: 'eswin_ahci_resume' defined but not used [-Wunused-function]
206 | static int eswin_ahci_resume(struct device *dev)
| ^~~~~~~~~~~~~~~~~
>> drivers/ata/ahci_eic7700.c:195:12: warning: 'eswin_ahci_suspend' defined but not used [-Wunused-function]
195 | static int eswin_ahci_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~~
vim +/eswin_ahci_resume +206 drivers/ata/ahci_eic7700.c
194
> 195 static int eswin_ahci_suspend(struct device *dev)
196 {
197 int ret;
198
199 ret = ahci_platform_suspend(dev);
200 if (ret)
201 return ret;
202
203 return 0;
204 }
205
> 206 static int eswin_ahci_resume(struct device *dev)
207 {
208 int ret;
209
210 ret = ahci_platform_resume(dev);
211 if (ret)
212 return ret;
213
214 return 0;
215 }
216
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC
2025-05-15 11:25 ` Niklas Cassel
@ 2025-08-14 9:51 ` luyulin
2025-08-14 14:32 ` Niklas Cassel
0 siblings, 1 reply; 9+ messages in thread
From: luyulin @ 2025-08-14 9:51 UTC (permalink / raw)
To: Niklas Cassel
Cc: hehuan1, dlemoal, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel, ningyu, linmin, Serge Semin
Hi Niklas,
Thank you very much for your constructive suggestions. Based on your advice,
I have optimized both the driver and the YAML program.
I sincerely apologize for the delayed response to your suggestions.
During this period, we have carefully studied the kernel specifications
and slowed down the pace of submitting patches.
>
> Hello Huan He,
>
> On Thu, May 15, 2025 at 04:57:23PM +0800, hehuan1@eswincomputing.com wrote:
> > From: Huan He <hehuan1@eswincomputing.com>
> >
> > Add eic7700 AHCI SATA controller device with single port support.
> > For the eic7700 SATA registers, it supports AHCI standard interface,
> > interrupt modes (INTx/MSI/PME), APB reset control,
> > and HSP_SP_CSR register configuration.
> >
> > Co-developed-by: Yulin Lu <luyulin@eswincomputing.com>
> > Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> > Signed-off-by: Huan He <hehuan1@eswincomputing.com>
> > ---
> > .../bindings/ata/eswin,eic7700-sata.yaml | 80 +++++++++++++++++++
> > 1 file changed, 80 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml b/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.yaml
> > new file mode 100644
> > index 000000000000..71e1b865ed2a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ata/eswin,eic7700-sata.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-sata.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 binding describes the SATA controller integrated in the Eswin EIC7700 SoC.
> > + The controller is compatible with the AHCI (Advanced Host Controller Interface)
> > + specification and supports up to 1 port.
> > +
> > +properties:
> > + compatible:
> > + const: eswin,eic7700-ahci
> > +
> > + reg:
> > + maxItems: 1
> > + description: Address range of the SATA registers
> > +
> > + interrupt-names:
> > + items:
> > + - const: intrq
> > + - const: msi
> > + - const: pme
> > +
> > + interrupts:
> > + maxItems: 3
> > + description: The SATA interrupt numbers
> > +
> > + ports-implemented:
> > + maximum: 0x1
> > +
> > + resets:
> > + maxItems: 1
> > + description: resets to be used by the controller.
> > +
> > + reset-names:
> > + const: apb
> > +
> > + '#address-cells':
> > + const: 2
> > +
> > + '#size-cells':
> > + const: 2
> > +
> > + eswin,hsp_sp_csr:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description: hsp_sp_csr regs to be used by the controller.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupt-names
> > + - interrupts
> > + - resets
> > + - reset-names
> > + - eswin,hsp_sp_csr
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + sata: sata@50420000 {
> > + compatible = "eswin,eic7700-ahci";
> > + reg = <0x50420000 0x10000>;
> > + interrupt-parent = <&plic>;
> > + interrupt-names = "intrq", "msi", "pme";
> > + interrupts = <58>, <59>, <60>;
> > + ports-implemented = <0x1>;
> > + resets = <&reset 7 (1 << 27)>;
> > + reset-names = "apb";
> > + #size-cells = <2>;
> > + eswin,hsp_sp_csr = <&hsp_sp_csr 0x1050>;
> > + };
> > --
> > 2.25.1
> >
>
> I'm surprised that you AHCI controller does not need any clocks ;)
>
>
> When looking at the EIC7700X TRM:
> https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases/download/v1.0.0-20250103/EIC7700X_SoC_Technical_Reference_Manual_Part2.pdf
>
> It is obvious that this SoC integrates the DWC AHCI controller.
>
> Thus, I would have expected your DT binding to have a:
> $ref: snps,dwc-ahci-common.yaml#
>
> Please have a look at these bindings:
> baikal,bt1-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
> baikal,bt1-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
> rockchip,dwc-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
> rockchip,dwc-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
> snps,dwc-ahci-common.yaml:$id: http://devicetree.org/schemas/ata/snps,dwc-ahci-common.yaml#
> snps,dwc-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
> snps,dwc-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
>
> The good news is that snps,dwc-ahci-common.yaml has defined and documented
> all the SATA clocks and resets for your board already (a lot of them which
> you missed to include in this binding).
>
>
> Looking quickly at:
> eswin,hsp_sp_csr = <&hsp_sp_csr 0x1050>;
>
> I can't help to wonder if these regs shouldn't be in a SATA PHY binding
> instead.
>
> Do e.g. a
> $ git grep -A 20 snps,dwc-ahci arch/
>
> There are multiple examples that use a PHY driver.
>
> If you were to implement a PHY driver, it is possible that you would
> not need to create a new (AHCI) DT binding at all, you could probably
> just add your compatible string to snps,dwc-ahci.yaml, as (from a quick)
> glance, all the only platform specific things appear to be PHY related.
>
Thank you very much for your expert advice. I have already implemented a
independent PHY driver, while the controller driver utilizes ahci_dwc.c.
Due to our hardware platform's SATA controller has specific constraints on clock, reset
and port resources, I think adding these to snps,dwc-ahci.yaml might compromise its readability.
Following reference implementations from other vendors in the Linux kernel,
such as rockchip,dwc-ahci.yaml, amlogic,axg-pcie.yaml and others, I plan to create
a new eswin,eic7700-ahci.yaml to describe these specifications.
Based on your professional experience, would you consider this approach acceptable?
Best regards,
Yulin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC
2025-08-14 9:51 ` luyulin
@ 2025-08-14 14:32 ` Niklas Cassel
0 siblings, 0 replies; 9+ messages in thread
From: Niklas Cassel @ 2025-08-14 14:32 UTC (permalink / raw)
To: luyulin
Cc: hehuan1, dlemoal, robh, krzk+dt, conor+dt, linux-ide, devicetree,
linux-kernel, p.zabel, ningyu, linmin, Serge Semin
Hello Yulin,
On Thu, Aug 14, 2025 at 05:51:59PM +0800, luyulin@eswincomputing.com wrote:
> Thank you very much for your constructive suggestions. Based on your advice,
> I have optimized both the driver and the YAML program.
> I sincerely apologize for the delayed response to your suggestions.
No need to apologize for anything :)
> > The good news is that snps,dwc-ahci-common.yaml has defined and documented
> > all the SATA clocks and resets for your board already (a lot of them which
> > you missed to include in this binding).
> >
> >
> > Looking quickly at:
> > eswin,hsp_sp_csr = <&hsp_sp_csr 0x1050>;
> >
> > I can't help to wonder if these regs shouldn't be in a SATA PHY binding
> > instead.
> >
> > Do e.g. a
> > $ git grep -A 20 snps,dwc-ahci arch/
> >
> > There are multiple examples that use a PHY driver.
> >
> > If you were to implement a PHY driver, it is possible that you would
> > not need to create a new (AHCI) DT binding at all, you could probably
> > just add your compatible string to snps,dwc-ahci.yaml, as (from a quick)
> > glance, all the only platform specific things appear to be PHY related.
> >
> Thank you very much for your expert advice. I have already implemented a
> independent PHY driver, while the controller driver utilizes ahci_dwc.c.
> Due to our hardware platform's SATA controller has specific constraints on clock, reset
> and port resources, I think adding these to snps,dwc-ahci.yaml might compromise its readability.
> Following reference implementations from other vendors in the Linux kernel,
> such as rockchip,dwc-ahci.yaml, amlogic,axg-pcie.yaml and others, I plan to create
> a new eswin,eic7700-ahci.yaml to describe these specifications.
> Based on your professional experience, would you consider this approach acceptable?
That sounds like a good approach.
When you create your device tree binding, make sure to reference
snps,dwc-ahci-common.yaml, like the other DWC based bindings:
$ git grep snps,dwc-ahci-common.yaml
baikal,bt1-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
baikal,bt1-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
rockchip,dwc-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
rockchip,dwc-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
snps,dwc-ahci.yaml: - $ref: snps,dwc-ahci-common.yaml#
snps,dwc-ahci.yaml: $ref: /schemas/ata/snps,dwc-ahci-common.yaml#/$defs/dwc-ahci-port
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-08-14 14:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-15 8:51 [PATCH v1 0/2] ESWIN EIC7700 sata driver hehuan1
2025-05-15 8:57 ` [PATCH v1 1/2] dt-bindings: sata: eswin: Document for EIC7700 SoC hehuan1
2025-05-15 10:18 ` Rob Herring (Arm)
2025-05-15 11:25 ` Niklas Cassel
2025-08-14 9:51 ` luyulin
2025-08-14 14:32 ` Niklas Cassel
2025-05-15 9:00 ` [PATCH v1 2/2] sata: eswin: Add eic7700 sata driver hehuan1
2025-05-15 11:56 ` Niklas Cassel
2025-05-16 14:33 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).