Add USB PHY support for CV18XX/SG200X series
Inochi Amaoto (2):
dt-bindings: phy: Add Sophgo CV1800 USB phy
phy: sophgo: Add USB 2.0 PHY driver for Sophgo CV18XX/SG200X
.../bindings/phy/sophgo,cv1800-usb-phy.yaml | 90 +++++
drivers/phy/Kconfig | 1 +
drivers/phy/Makefile | 1 +
drivers/phy/sophgo/Kconfig | 19 +
drivers/phy/sophgo/Makefile | 2 +
drivers/phy/sophgo/phy-cv1800-usb.c | 381 ++++++++++++++++++
6 files changed, 494 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
create mode 100644 drivers/phy/sophgo/Kconfig
create mode 100644 drivers/phy/sophgo/Makefile
create mode 100644 drivers/phy/sophgo/phy-cv1800-usb.c
--
2.44.0
The USB phy of Sophgo CV18XX series SoC needs to sense a pin called
"VBUS_DET" to get the right operation mode. If this pin is not
connected, it only supports setting the mode manually.
Add USB phy bindings for Sophgo CV18XX/SG200X series SoC.
Signed-off-by: Inochi Amaoto <[email protected]>
---
.../bindings/phy/sophgo,cv1800-usb-phy.yaml | 90 +++++++++++++++++++
1 file changed, 90 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
diff --git a/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
new file mode 100644
index 000000000000..cb394ac5d8c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/sophgo,cv1800-usb-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo CV18XX/SG200X USB 2.0 PHY
+
+maintainers:
+ - Inochi Amaoto <[email protected]>
+
+properties:
+ compatible:
+ const: sophgo,cv1800-usb-phy
+
+ reg:
+ maxItems: 1
+
+ "#phy-cells":
+ const: 0
+
+ clocks:
+ items:
+ - description: PHY clock
+ - description: PHY app clock
+ - description: PHY stb clock
+ - description: PHY lpm clock
+
+ clock-names:
+ items:
+ - const: phy
+ - const: app
+ - const: stb
+ - const: lpm
+
+ dr_mode:
+ description: PHY device mode when initing.
+ enum: [host, peripheral, otg]
+
+ vbus_det-gpios:
+ description: GPIO to the USB OTG VBUS detect pin. This should not be
+ defined if vbus_det gpio and switch gpio are connected.
+ maxItems: 1
+
+ sophgo,switch-gpios:
+ description: GPIO for the phy to control connected switch.
+ maxItems: 2
+
+required:
+ - compatible
+ - "#phy-cells"
+ - clocks
+ - clock-names
+ - dr_mode
+
+allOf:
+ - if:
+ properties:
+ dr_mode:
+ const: otg
+ then:
+ required:
+ - vbus_det-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ phy@48 {
+ compatible = "sophgo,cv1800-usb-phy";
+ reg = <0x48 0x4>;
+ #phy-cells = <0>;
+ clocks = <&clk 92>, <&clk 93>,
+ <&clk 94>, <&clk 95>;
+ clock-names = "phy", "app", "stb", "lpm";
+ dr_mode = "host";
+ };
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ phy@54 {
+ compatible = "sophgo,cv1800-usb-phy";
+ reg = <0x54 0x4>;
+ #phy-cells = <0>;
+ clocks = <&clk 92>, <&clk 93>,
+ <&clk 94>, <&clk 95>;
+ clock-names = "phy", "app", "stb", "lpm";
+ dr_mode = "otg";
+ vbus_det-gpios = <&portb 6 GPIO_ACTIVE_HIGH>;
+ };
--
2.44.0
Add USB 2.0 PHY driver for Sophgo CV18XX/SG200X.
Signed-off-by: Inochi Amaoto <[email protected]>
---
drivers/phy/Kconfig | 1 +
drivers/phy/Makefile | 1 +
drivers/phy/sophgo/Kconfig | 19 ++
drivers/phy/sophgo/Makefile | 2 +
drivers/phy/sophgo/phy-cv1800-usb.c | 381 ++++++++++++++++++++++++++++
5 files changed, 404 insertions(+)
create mode 100644 drivers/phy/sophgo/Kconfig
create mode 100644 drivers/phy/sophgo/Makefile
create mode 100644 drivers/phy/sophgo/phy-cv1800-usb.c
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 787354b849c7..596b37ab3191 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -92,6 +92,7 @@ source "drivers/phy/renesas/Kconfig"
source "drivers/phy/rockchip/Kconfig"
source "drivers/phy/samsung/Kconfig"
source "drivers/phy/socionext/Kconfig"
+source "drivers/phy/sophgo/Kconfig"
source "drivers/phy/st/Kconfig"
source "drivers/phy/starfive/Kconfig"
source "drivers/phy/sunplus/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 868a220ed0f6..7ff32f0ae08a 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -31,6 +31,7 @@ obj-y += allwinner/ \
rockchip/ \
samsung/ \
socionext/ \
+ sophgo/ \
st/ \
starfive/ \
sunplus/ \
diff --git a/drivers/phy/sophgo/Kconfig b/drivers/phy/sophgo/Kconfig
new file mode 100644
index 000000000000..b1c558de6616
--- /dev/null
+++ b/drivers/phy/sophgo/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Phy drivers for Sophgo platforms
+#
+
+if ARCH_SOPHGO || COMPILE_TEST
+
+config PHY_SOPHGO_CV1800_USB
+ tristate "Sophgo CV18XX/SG200X USB 2.0 PHY support"
+ depends on MFD_SYSCON
+ depends on USB_SUPPORT
+ select GENERIC_PHY
+ help
+ Enable this to support the USB 2.0 PHY used with
+ the DWC2 USB controller in Sophgo CV18XX/SG200X
+ series SoC.
+ If unsure, say N.
+
+endif # ARCH_SOPHGO || COMPILE_TEST
diff --git a/drivers/phy/sophgo/Makefile b/drivers/phy/sophgo/Makefile
new file mode 100644
index 000000000000..659537054cd4
--- /dev/null
+++ b/drivers/phy/sophgo/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_SOPHGO_CV1800_USB) += phy-cv1800-usb.o
diff --git a/drivers/phy/sophgo/phy-cv1800-usb.c b/drivers/phy/sophgo/phy-cv1800-usb.c
new file mode 100644
index 000000000000..a9c7f41406b4
--- /dev/null
+++ b/drivers/phy/sophgo/phy-cv1800-usb.c
@@ -0,0 +1,381 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Inochi Amaoto <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/debugfs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <soc/sophgo/cv1800-sysctl.h>
+
+#define PHY_IDPAD_C_OW BIT(6)
+#define PHY_IDPAD_C_SW BIT(7)
+
+#define PHY_BASE_CLK_RATE 300000000
+#define PHY_APP_CLK_RATE 125000000
+#define PHY_LPM_CLK_RATE 12000000
+#define PHY_STB_CLK_RATE 333334
+
+#define PHY_VBUS_DET_DEBOUNCE_TIME usecs_to_jiffies(100)
+
+struct cv1800_usb_phy {
+ struct phy *phy;
+ struct regmap *syscon;
+ spinlock_t lock;
+ struct clk *usb_phy_clk;
+ struct clk *usb_app_clk;
+ struct clk *usb_lpm_clk;
+ struct clk *usb_stb_clk;
+ struct gpio_descs *switch_gpios;
+ struct gpio_desc *vbus_det_gpio;
+ int vbus_det_irq;
+ struct delayed_work vbus_work;
+ bool enable_otg;
+};
+
+static int cv1800_usb_phy_set_idpad(struct cv1800_usb_phy *phy,
+ bool is_host)
+{
+ unsigned long flags;
+ unsigned int regval = 0;
+ int ret;
+
+ if (is_host)
+ regval = PHY_IDPAD_C_OW;
+ else
+ regval = PHY_IDPAD_C_OW | PHY_IDPAD_C_SW;
+
+ spin_lock_irqsave(&phy->lock, flags);
+ ret = regmap_update_bits(phy->syscon, CV1800_USB_PHY_CTRL_REG,
+ PHY_IDPAD_C_OW | PHY_IDPAD_C_SW,
+ regval);
+ spin_unlock_irqrestore(&phy->lock, flags);
+
+ return ret;
+}
+
+static void cv1800_usb_phy_set_gpio(struct cv1800_usb_phy *phy,
+ bool is_host)
+{
+ unsigned int i, ndescs;
+ struct gpio_desc **gpios;
+
+ if (!phy->switch_gpios)
+ return;
+
+ ndescs = phy->switch_gpios->ndescs;
+ gpios = phy->switch_gpios->desc;
+
+ if (is_host) {
+ for (i = 0; i < ndescs; i++)
+ gpiod_set_value_cansleep(gpios[i], 0);
+ } else {
+ for (i = 0; i < ndescs; i++)
+ gpiod_set_value_cansleep(gpios[ndescs - 1 - i], 1);
+ }
+}
+
+static int cv1800_usb_phy_set_mode_internal(struct cv1800_usb_phy *phy,
+ bool is_host)
+{
+ int ret = cv1800_usb_phy_set_idpad(phy, is_host);
+
+ if (ret < 0)
+ return ret;
+
+ cv1800_usb_phy_set_gpio(phy, is_host);
+
+ return 0;
+}
+
+static ssize_t dr_mode_write(struct file *file, const char __user *_buf,
+ size_t count, loff_t *ppos)
+{
+ struct seq_file *seq = file->private_data;
+ struct cv1800_usb_phy *phy = seq->private;
+ bool is_host;
+ char buf[16];
+
+ if (copy_from_user(&buf, _buf, min_t(size_t, sizeof(buf) - 1, count)))
+ return -EFAULT;
+
+ if (sysfs_streq(buf, "host")) {
+ phy->enable_otg = false;
+ is_host = true;
+ } else if (sysfs_streq(buf, "peripheral")) {
+ phy->enable_otg = false;
+ is_host = false;
+ } else if (sysfs_streq(buf, "otg") && phy->vbus_det_irq > 0) {
+ phy->enable_otg = true;
+ } else
+ return -EINVAL;
+
+ if (phy->enable_otg)
+ queue_delayed_work(system_wq, &phy->vbus_work,
+ PHY_VBUS_DET_DEBOUNCE_TIME);
+ else
+ cv1800_usb_phy_set_mode_internal(phy, is_host);
+
+ return count;
+}
+
+static int dr_mode_show(struct seq_file *seq, void *v)
+{
+ struct cv1800_usb_phy *phy = seq->private;
+ unsigned long flags;
+ unsigned int regval;
+ bool is_host = true;
+ int ret;
+
+ spin_lock_irqsave(&phy->lock, flags);
+ ret = regmap_read(phy->syscon, CV1800_USB_PHY_CTRL_REG, ®val);
+ spin_unlock_irqrestore(&phy->lock, flags);
+
+ if (ret)
+ return ret;
+
+ if (regval & PHY_IDPAD_C_SW)
+ is_host = false;
+
+ if (phy->enable_otg)
+ seq_puts(seq, "otg: ");
+ seq_puts(seq, is_host ? "host\n" : "peripheral\n");
+
+ return 0;
+}
+
+DEFINE_SHOW_STORE_ATTRIBUTE(dr_mode);
+
+static int cv1800_usb_phy_set_clock(struct cv1800_usb_phy *phy)
+{
+ int ret;
+
+ ret = clk_set_rate(phy->usb_phy_clk, PHY_BASE_CLK_RATE);
+ if (ret)
+ return ret;
+
+ ret = clk_set_rate(phy->usb_app_clk, PHY_APP_CLK_RATE);
+ if (ret)
+ return ret;
+
+ ret = clk_set_rate(phy->usb_lpm_clk, PHY_LPM_CLK_RATE);
+ if (ret)
+ return ret;
+
+ ret = clk_set_rate(phy->usb_stb_clk, PHY_STB_CLK_RATE);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int cv1800_usb_phy_set_mode(struct phy *_phy,
+ enum phy_mode mode, int submode)
+{
+ struct cv1800_usb_phy *phy = phy_get_drvdata(_phy);
+ bool is_host;
+
+ switch (mode) {
+ case PHY_MODE_USB_DEVICE:
+ is_host = false;
+ phy->enable_otg = false;
+ break;
+ case PHY_MODE_USB_HOST:
+ is_host = true;
+ phy->enable_otg = false;
+ break;
+ case PHY_MODE_USB_OTG:
+ /* phy only supports soft OTG when VBUS_DET pin is connected. */
+ if (phy->vbus_det_irq > 0) {
+ phy->enable_otg = true;
+ queue_delayed_work(system_wq, &phy->vbus_work,
+ PHY_VBUS_DET_DEBOUNCE_TIME);
+ }
+ return 0;
+ default:
+ return -EINVAL;
+ }
+
+ return cv1800_usb_phy_set_mode_internal(phy, is_host);
+}
+
+static const struct phy_ops cv1800_usb_phy_ops = {
+ .set_mode = cv1800_usb_phy_set_mode,
+ .owner = THIS_MODULE,
+};
+
+static void cv1800_usb_phy_vbus_switch(struct work_struct *work)
+{
+ struct cv1800_usb_phy *phy =
+ container_of(work, struct cv1800_usb_phy, vbus_work.work);
+ int state = gpiod_get_value_cansleep(phy->vbus_det_gpio);
+
+ cv1800_usb_phy_set_mode_internal(phy, state == 0);
+}
+
+static irqreturn_t cv1800_usb_phy_vbus_det_irq(int irq, void *dev_id)
+{
+ struct cv1800_usb_phy *phy = dev_id;
+
+ if (phy->enable_otg)
+ queue_delayed_work(system_wq, &phy->vbus_work,
+ PHY_VBUS_DET_DEBOUNCE_TIME);
+ return IRQ_HANDLED;
+}
+
+static void cv1800_usb_phy_init_mode(struct device *dev,
+ struct cv1800_usb_phy *phy)
+{
+ const char *mode;
+ bool is_host;
+
+ phy->enable_otg = false;
+
+ device_property_read_string(dev, "dr_mode", &mode);
+ if (!mode)
+ mode = "host";
+
+ if (!strcmp(mode, "host"))
+ is_host = true;
+ else if (!strcmp(mode, "peripheral"))
+ is_host = false;
+ else if (phy->vbus_det_irq > 0)
+ phy->enable_otg = true;
+
+ if (phy->enable_otg)
+ queue_delayed_work(system_wq, &phy->vbus_work, 0);
+ else
+ cv1800_usb_phy_set_mode_internal(phy, is_host);
+}
+
+static int cv1800_usb_phy_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device *parent = dev->parent;
+ struct cv1800_usb_phy *phy;
+ struct phy_provider *phy_provider;
+ int ret;
+
+ if (!parent)
+ return -ENODEV;
+
+ phy = devm_kmalloc(dev, sizeof(*phy), GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->syscon = syscon_node_to_regmap(parent->of_node);
+ if (IS_ERR_OR_NULL(phy->syscon))
+ return -ENODEV;
+
+ spin_lock_init(&phy->lock);
+
+ phy->usb_phy_clk = devm_clk_get_enabled(dev, "phy");
+ if (IS_ERR(phy->usb_phy_clk))
+ return dev_err_probe(dev, PTR_ERR(phy->usb_phy_clk),
+ "Failed to get phy clock\n");
+
+ phy->usb_app_clk = devm_clk_get_enabled(dev, "app");
+ if (IS_ERR(phy->usb_app_clk))
+ return dev_err_probe(dev, PTR_ERR(phy->usb_app_clk),
+ "Failed to get app clock\n");
+
+ phy->usb_lpm_clk = devm_clk_get_enabled(dev, "lpm");
+ if (IS_ERR(phy->usb_lpm_clk))
+ return dev_err_probe(dev, PTR_ERR(phy->usb_lpm_clk),
+ "Failed to get lpm clock\n");
+
+ phy->usb_stb_clk = devm_clk_get_enabled(dev, "stb");
+ if (IS_ERR(phy->usb_stb_clk))
+ return dev_err_probe(dev, PTR_ERR(phy->usb_stb_clk),
+ "Failed to get stb clock\n");
+
+ phy->phy = devm_phy_create(dev, NULL, &cv1800_usb_phy_ops);
+ if (IS_ERR(phy->phy))
+ return dev_err_probe(dev, PTR_ERR(phy->phy),
+ "Failed to create phy\n");
+
+ ret = cv1800_usb_phy_set_clock(phy);
+ if (ret)
+ return ret;
+
+ phy->switch_gpios = devm_gpiod_get_array_optional(dev, "sophgo,switch",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(phy->switch_gpios))
+ return dev_err_probe(dev, PTR_ERR(phy->switch_gpios),
+ "Failed to get switch pin\n");
+
+ phy->vbus_det_gpio = devm_gpiod_get_optional(dev, "vbus_det", GPIOD_IN);
+ if (IS_ERR(phy->vbus_det_gpio))
+ return dev_err_probe(dev, PTR_ERR(phy->vbus_det_gpio),
+ "Failed to process vbus pin\n");
+
+ phy->vbus_det_irq = gpiod_to_irq(phy->vbus_det_gpio);
+ if (phy->vbus_det_irq > 0) {
+ ret = devm_request_irq(dev, phy->vbus_det_irq,
+ cv1800_usb_phy_vbus_det_irq,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ "usb-vbus-det", phy);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to request vbus irq\n");
+ }
+
+ INIT_DELAYED_WORK(&phy->vbus_work, cv1800_usb_phy_vbus_switch);
+
+ debugfs_create_file("dr_mode", 0644, phy->phy->debugfs,
+ phy, &dr_mode_fops);
+
+ phy_set_drvdata(phy->phy, phy);
+ phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+ /*
+ * phy needs to change mode twice after initialization,
+ * otherwise the controller can not found devices attached
+ * to the phy.
+ */
+ cv1800_usb_phy_set_idpad(phy, false);
+ cv1800_usb_phy_set_idpad(phy, true);
+ cv1800_usb_phy_init_mode(dev, phy);
+
+ return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static void cv1800_usb_phy_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cv1800_usb_phy *phy = platform_get_drvdata(pdev);
+
+ if (phy->vbus_det_irq > 0)
+ devm_free_irq(dev, phy->vbus_det_irq, phy);
+
+ cancel_delayed_work_sync(&phy->vbus_work);
+}
+
+static const struct of_device_id cv1800_usb_phy_ids[] = {
+ { .compatible = "sophgo,cv1800-usb-phy" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, cv1800_usb_phy_ids);
+
+static struct platform_driver cv1800_usb_phy_driver = {
+ .probe = cv1800_usb_phy_probe,
+ .remove_new = cv1800_usb_phy_remove,
+ .driver = {
+ .name = "cv1800-usb-phy",
+ .of_match_table = cv1800_usb_phy_ids,
+ },
+};
+module_platform_driver(cv1800_usb_phy_driver);
+MODULE_DESCRIPTION("CV1800/SG2000 SoC USB 2.0 PHY driver");
+MODULE_LICENSE("GPL");
--
2.44.0
On Fri, Apr 12, 2024 at 03:21:26PM +0800, Inochi Amaoto wrote:
> Add USB PHY support for CV18XX/SG200X series
>
> Inochi Amaoto (2):
> dt-bindings: phy: Add Sophgo CV1800 USB phy
> phy: sophgo: Add USB 2.0 PHY driver for Sophgo CV18XX/SG200X
>
I forgot to mention this patch 2 depends a header from
the following patch:
https://lore.kernel.org/all/IA1PR20MB4953BAA0F8E06CB202C5C2FBBB062@IA1PR20MB4953.namprd20.prod.outlook.com/
On Fri, Apr 12, 2024 at 03:22:24PM +0800, Inochi Amaoto wrote:
> The USB phy of Sophgo CV18XX series SoC needs to sense a pin called
> "VBUS_DET" to get the right operation mode. If this pin is not
> connected, it only supports setting the mode manually.
>
> Add USB phy bindings for Sophgo CV18XX/SG200X series SoC.
>
> Signed-off-by: Inochi Amaoto <[email protected]>
> ---
> .../bindings/phy/sophgo,cv1800-usb-phy.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> new file mode 100644
> index 000000000000..cb394ac5d8c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/sophgo,cv1800-usb-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo CV18XX/SG200X USB 2.0 PHY
> +
> +maintainers:
> + - Inochi Amaoto <[email protected]>
> +
> +properties:
> + compatible:
> + const: sophgo,cv1800-usb-phy
> +
> + reg:
> + maxItems: 1
> +
> + "#phy-cells":
> + const: 0
> +
> + clocks:
> + items:
> + - description: PHY clock
> + - description: PHY app clock
> + - description: PHY stb clock
> + - description: PHY lpm clock
> +
> + clock-names:
> + items:
> + - const: phy
> + - const: app
> + - const: stb
> + - const: lpm
> +
> + dr_mode:
> + description: PHY device mode when initing.
"initing" isn't a word, "initialising" is the correct word. Or
"initializing" if you speak American. But if it is just the value during
initialisation, why do we need to know - we can just overwrite it in
software, right?
Are you sure that this is limited to initialisation? I would have
thought that it describes the configuration that the board is in, and is
a fixed property of the system?
> + enum: [host, peripheral, otg]
Rob, I know this is a phy rather than a controller, so referencing
usb-drd.yaml doesn't really make sense, but there are several PHYs using
dr_mode so it feels like there should be something to reference here
rather than defining the property anew.
> +
> + vbus_det-gpios:
> + description: GPIO to the USB OTG VBUS detect pin. This should not be
> + defined if vbus_det gpio and switch gpio are connected.
I don't understand the second sentence here.
> + maxItems: 1
> +
> + sophgo,switch-gpios:
> + description: GPIO for the phy to control connected switch.
> + maxItems: 2
You've got two items here, they should be described. /But/ the property
above says "switch gpio", which is singular. Which is it?
Cheers,
Conor.
> +
> +required:
> + - compatible
> + - "#phy-cells"
> + - clocks
> + - clock-names
> + - dr_mode
> +
> +allOf:
> + - if:
> + properties:
> + dr_mode:
> + const: otg
> + then:
> + required:
> + - vbus_det-gpios
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + phy@48 {
> + compatible = "sophgo,cv1800-usb-phy";
> + reg = <0x48 0x4>;
> + #phy-cells = <0>;
> + clocks = <&clk 92>, <&clk 93>,
> + <&clk 94>, <&clk 95>;
> + clock-names = "phy", "app", "stb", "lpm";
> + dr_mode = "host";
> + };
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + phy@54 {
> + compatible = "sophgo,cv1800-usb-phy";
> + reg = <0x54 0x4>;
> + #phy-cells = <0>;
> + clocks = <&clk 92>, <&clk 93>,
> + <&clk 94>, <&clk 95>;
> + clock-names = "phy", "app", "stb", "lpm";
> + dr_mode = "otg";
> + vbus_det-gpios = <&portb 6 GPIO_ACTIVE_HIGH>;
> + };
> --
> 2.44.0
>
On Fri, Apr 19, 2024 at 03:26:53PM GMT, Conor Dooley wrote:
> On Fri, Apr 12, 2024 at 03:22:24PM +0800, Inochi Amaoto wrote:
> > The USB phy of Sophgo CV18XX series SoC needs to sense a pin called
> > "VBUS_DET" to get the right operation mode. If this pin is not
> > connected, it only supports setting the mode manually.
> >
> > Add USB phy bindings for Sophgo CV18XX/SG200X series SoC.
> >
> > Signed-off-by: Inochi Amaoto <[email protected]>
> > ---
> > .../bindings/phy/sophgo,cv1800-usb-phy.yaml | 90 +++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > new file mode 100644
> > index 000000000000..cb394ac5d8c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/sophgo,cv1800-usb-phy.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sophgo CV18XX/SG200X USB 2.0 PHY
> > +
> > +maintainers:
> > + - Inochi Amaoto <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + const: sophgo,cv1800-usb-phy
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + "#phy-cells":
> > + const: 0
> > +
> > + clocks:
> > + items:
> > + - description: PHY clock
> > + - description: PHY app clock
> > + - description: PHY stb clock
> > + - description: PHY lpm clock
> > +
> > + clock-names:
> > + items:
> > + - const: phy
> > + - const: app
> > + - const: stb
> > + - const: lpm
> > +
> > + dr_mode:
> > + description: PHY device mode when initing.
>
> "initing" isn't a word, "initialising" is the correct word. Or
> "initializing" if you speak American. But if it is just the value during
> initialisation, why do we need to know - we can just overwrite it in
> software, right?
>
> Are you sure that this is limited to initialisation? I would have
> thought that it describes the configuration that the board is in, and is
> a fixed property of the system?
>
> > + enum: [host, peripheral, otg]
>
> Rob, I know this is a phy rather than a controller, so referencing
> usb-drd.yaml doesn't really make sense, but there are several PHYs using
> dr_mode so it feels like there should be something to reference here
> rather than defining the property anew.
>
Yes, you are right. Using dr_mode in initialisation is not necessary.
We can just let it go and using the default mode. In fact, for most
boards with this SoC, host mode is just enough. And it is just easy
to overwrite the mode value in the driver if we want another mode.
For the OTG, it can just check the `vbus_det-gpios`. I will remove
this property, thanks.
> > +
> > + vbus_det-gpios:
> > + description: GPIO to the USB OTG VBUS detect pin. This should not be
> > + defined if vbus_det gpio and switch gpio are connected.
>
> I don't understand the second sentence here.
>
> > + maxItems: 1
> > +
> > + sophgo,switch-gpios:
> > + description: GPIO for the phy to control connected switch.
> > + maxItems: 2
>
> You've got two items here, they should be described. /But/ the property
> above says "switch gpio", which is singular. Which is it?
>
`switch-gpios` is gpios to controll USB switch connected to the
phy. Sophgo recommends that phy use a switch to separate device
port and host port if it supports both. I know this is weird,
but many board follows this design, such as Huashan PI and
Milkv Duo S. As for item number, it is just an array of gpios
to process one by one, I mark it as two just because Milkv
Duo S use two gpios to controll the USB switch.
For vbus_det gpio description, There is because the design of
Milk-v Duo S, which connect the switch gpio and VBUS detect
gpio. In this case the OTG function is broken and we can just
change the mode by toggling switch gpios. So I suggest not
defining this property.
> Cheers,
> Conor.
>
> > +
> > +required:
> > + - compatible
> > + - "#phy-cells"
> > + - clocks
> > + - clock-names
> > + - dr_mode
> > +
> > +allOf:
> > + - if:
> > + properties:
> > + dr_mode:
> > + const: otg
> > + then:
> > + required:
> > + - vbus_det-gpios
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + phy@48 {
> > + compatible = "sophgo,cv1800-usb-phy";
> > + reg = <0x48 0x4>;
> > + #phy-cells = <0>;
> > + clocks = <&clk 92>, <&clk 93>,
> > + <&clk 94>, <&clk 95>;
> > + clock-names = "phy", "app", "stb", "lpm";
> > + dr_mode = "host";
> > + };
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + phy@54 {
> > + compatible = "sophgo,cv1800-usb-phy";
> > + reg = <0x54 0x4>;
> > + #phy-cells = <0>;
> > + clocks = <&clk 92>, <&clk 93>,
> > + <&clk 94>, <&clk 95>;
> > + clock-names = "phy", "app", "stb", "lpm";
> > + dr_mode = "otg";
> > + vbus_det-gpios = <&portb 6 GPIO_ACTIVE_HIGH>;
> > + };
> > --
> > 2.44.0
> >
Hi Inochi,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9-rc4 next-20240419]
[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/Inochi-Amaoto/dt-bindings-phy-Add-Sophgo-CV1800-USB-phy/20240412-152532
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/IA1PR20MB4953EB2E2BF9D5F1EE30E2D4BB042%40IA1PR20MB4953.namprd20.prod.outlook.com
patch subject: [PATCH 2/2] phy: sophgo: Add USB 2.0 PHY driver for Sophgo CV18XX/SG200X
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240421/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7089c359a3845323f6f30c44a47dd901f2edfe63)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240421/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/phy/sophgo/phy-cv1800-usb.c:14:
In file included from include/linux/of_address.h:7:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/phy/sophgo/phy-cv1800-usb.c:14:
In file included from include/linux/of_address.h:7:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/phy/sophgo/phy-cv1800-usb.c:14:
In file included from include/linux/of_address.h:7:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/phy/sophgo/phy-cv1800-usb.c:17:
In file included from include/linux/phy/phy.h:17:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2208:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/phy/sophgo/phy-cv1800-usb.c:20:10: fatal error: 'soc/sophgo/cv1800-sysctl.h' file not found
20 | #include <soc/sophgo/cv1800-sysctl.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
7 warnings and 1 error generated.
vim +20 drivers/phy/sophgo/phy-cv1800-usb.c
> 20 #include <soc/sophgo/cv1800-sysctl.h>
21
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Inochi,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9-rc4 next-20240419]
[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/Inochi-Amaoto/dt-bindings-phy-Add-Sophgo-CV1800-USB-phy/20240412-152532
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/IA1PR20MB4953EB2E2BF9D5F1EE30E2D4BB042%40IA1PR20MB4953.namprd20.prod.outlook.com
patch subject: [PATCH 2/2] phy: sophgo: Add USB 2.0 PHY driver for Sophgo CV18XX/SG200X
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240421/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240421/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/phy/sophgo/phy-cv1800-usb.c:20:10: fatal error: soc/sophgo/cv1800-sysctl.h: No such file or directory
20 | #include <soc/sophgo/cv1800-sysctl.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +20 drivers/phy/sophgo/phy-cv1800-usb.c
> 20 #include <soc/sophgo/cv1800-sysctl.h>
21
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Sat, Apr 20, 2024 at 09:39:18AM +0800, Inochi Amaoto wrote:
> On Fri, Apr 19, 2024 at 03:26:53PM GMT, Conor Dooley wrote:
> > On Fri, Apr 12, 2024 at 03:22:24PM +0800, Inochi Amaoto wrote:
> > > The USB phy of Sophgo CV18XX series SoC needs to sense a pin called
> > > "VBUS_DET" to get the right operation mode. If this pin is not
> > > connected, it only supports setting the mode manually.
> > >
> > > Add USB phy bindings for Sophgo CV18XX/SG200X series SoC.
> > >
> > > Signed-off-by: Inochi Amaoto <[email protected]>
> > > ---
> > > .../bindings/phy/sophgo,cv1800-usb-phy.yaml | 90 +++++++++++++++++++
> > > 1 file changed, 90 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > new file mode 100644
> > > index 000000000000..cb394ac5d8c4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > @@ -0,0 +1,90 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/sophgo,cv1800-usb-phy.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sophgo CV18XX/SG200X USB 2.0 PHY
> > > +
> > > +maintainers:
> > > + - Inochi Amaoto <[email protected]>
> > > +
> > > +properties:
> > > + compatible:
> > > + const: sophgo,cv1800-usb-phy
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + "#phy-cells":
> > > + const: 0
> > > +
> > > + clocks:
> > > + items:
> > > + - description: PHY clock
> > > + - description: PHY app clock
> > > + - description: PHY stb clock
> > > + - description: PHY lpm clock
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: phy
> > > + - const: app
> > > + - const: stb
> > > + - const: lpm
> > > +
> > > + dr_mode:
> > > + description: PHY device mode when initing.
> >
> > "initing" isn't a word, "initialising" is the correct word. Or
> > "initializing" if you speak American. But if it is just the value during
> > initialisation, why do we need to know - we can just overwrite it in
> > software, right?
> >
> > Are you sure that this is limited to initialisation? I would have
> > thought that it describes the configuration that the board is in, and is
> > a fixed property of the system?
> >
> > > + enum: [host, peripheral, otg]
> >
> > Rob, I know this is a phy rather than a controller, so referencing
> > usb-drd.yaml doesn't really make sense, but there are several PHYs using
> > dr_mode so it feels like there should be something to reference here
> > rather than defining the property anew.
> >
>
> Yes, you are right. Using dr_mode in initialisation is not necessary.
> We can just let it go and using the default mode. In fact, for most
> boards with this SoC, host mode is just enough. And it is just easy
> to overwrite the mode value in the driver if we want another mode.
> For the OTG, it can just check the `vbus_det-gpios`. I will remove
> this property, thanks.
Just to be clear, it's valid to have a dr_mode property in cases that
you cannot detect what the mode is dynamically. What I was questioning
was the wording about only valid for init.
> > > + vbus_det-gpios:
> > > + description: GPIO to the USB OTG VBUS detect pin. This should not be
> > > + defined if vbus_det gpio and switch gpio are connected.
> >
> > I don't understand the second sentence here.
Ah, with your explanation below I now understand what you mean here. I
think this needs to be re-written - I think it would be easier to
understand with s/gpio/pin/ in the second line.
> > > + maxItems: 1
> > > +
> > > + sophgo,switch-gpios:
> > > + description: GPIO for the phy to control connected switch.
> > > + maxItems: 2
> >
> > You've got two items here, they should be described. /But/ the property
> > above says "switch gpio", which is singular. Which is it?
> >
>
> `switch-gpios` is gpios to controll USB switch connected to the
> phy. Sophgo recommends that phy use a switch to separate device
> port and host port if it supports both. I know this is weird,
> but many board follows this design, such as Huashan PI and
> Milkv Duo S. As for item number, it is just an array of gpios
> to process one by one, I mark it as two just because Milkv
> Duo S use two gpios to controll the USB switch.
Right, but what I'm looking for is a description for what each GPIO
does, so that someone can know how the dts should be written.
> For vbus_det gpio description, There is because the design of
> Milk-v Duo S, which connect the switch gpio and VBUS detect
> gpio. In this case the OTG function is broken and we can just
> change the mode by toggling switch gpios. So I suggest not
> defining this property.
Okay. See my comment on it above.
Thanks,
Conor.
On Mon, Apr 22, 2024 at 05:21:26PM GMT, Conor Dooley wrote:
> On Sat, Apr 20, 2024 at 09:39:18AM +0800, Inochi Amaoto wrote:
> > On Fri, Apr 19, 2024 at 03:26:53PM GMT, Conor Dooley wrote:
> > > On Fri, Apr 12, 2024 at 03:22:24PM +0800, Inochi Amaoto wrote:
> > > > The USB phy of Sophgo CV18XX series SoC needs to sense a pin called
> > > > "VBUS_DET" to get the right operation mode. If this pin is not
> > > > connected, it only supports setting the mode manually.
> > > >
> > > > Add USB phy bindings for Sophgo CV18XX/SG200X series SoC.
> > > >
> > > > Signed-off-by: Inochi Amaoto <[email protected]>
> > > > ---
> > > > .../bindings/phy/sophgo,cv1800-usb-phy.yaml | 90 +++++++++++++++++++
> > > > 1 file changed, 90 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > > new file mode 100644
> > > > index 000000000000..cb394ac5d8c4
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > > @@ -0,0 +1,90 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/sophgo,cv1800-usb-phy.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Sophgo CV18XX/SG200X USB 2.0 PHY
> > > > +
> > > > +maintainers:
> > > > + - Inochi Amaoto <[email protected]>
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + const: sophgo,cv1800-usb-phy
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > +
> > > > + "#phy-cells":
> > > > + const: 0
> > > > +
> > > > + clocks:
> > > > + items:
> > > > + - description: PHY clock
> > > > + - description: PHY app clock
> > > > + - description: PHY stb clock
> > > > + - description: PHY lpm clock
> > > > +
> > > > + clock-names:
> > > > + items:
> > > > + - const: phy
> > > > + - const: app
> > > > + - const: stb
> > > > + - const: lpm
> > > > +
> > > > + dr_mode:
> > > > + description: PHY device mode when initing.
> > >
> > > "initing" isn't a word, "initialising" is the correct word. Or
> > > "initializing" if you speak American. But if it is just the value during
> > > initialisation, why do we need to know - we can just overwrite it in
> > > software, right?
> > >
> > > Are you sure that this is limited to initialisation? I would have
> > > thought that it describes the configuration that the board is in, and is
> > > a fixed property of the system?
> > >
> > > > + enum: [host, peripheral, otg]
> > >
> > > Rob, I know this is a phy rather than a controller, so referencing
> > > usb-drd.yaml doesn't really make sense, but there are several PHYs using
> > > dr_mode so it feels like there should be something to reference here
> > > rather than defining the property anew.
> > >
> >
> > Yes, you are right. Using dr_mode in initialisation is not necessary.
> > We can just let it go and using the default mode. In fact, for most
> > boards with this SoC, host mode is just enough. And it is just easy
> > to overwrite the mode value in the driver if we want another mode.
> > For the OTG, it can just check the `vbus_det-gpios`. I will remove
> > this property, thanks.
>
> Just to be clear, it's valid to have a dr_mode property in cases that
> you cannot detect what the mode is dynamically. What I was questioning
> was the wording about only valid for init.
>
OK, for the USB phy of CV1800, it always needs to start at host mode.
Because it needs to switch to both mode when initializing. As a result,
the "dr_mode" property is just added to decide which mode is set after
initializing (mostly for functionality). This is why I say it is for
initializing.
Now, it is clean that setting dr_mode is just a function hint, and user
can just overwrite the mode. So I decided to remove this.
> > > > + vbus_det-gpios:
> > > > + description: GPIO to the USB OTG VBUS detect pin. This should not be
> > > > + defined if vbus_det gpio and switch gpio are connected.
> > >
> > > I don't understand the second sentence here.
>
> Ah, with your explanation below I now understand what you mean here. I
> think this needs to be re-written - I think it would be easier to
> understand with s/gpio/pin/ in the second line.
>
> > > > + maxItems: 1
> > > > +
> > > > + sophgo,switch-gpios:
> > > > + description: GPIO for the phy to control connected switch.
> > > > + maxItems: 2
> > >
> > > You've got two items here, they should be described. /But/ the property
> > > above says "switch gpio", which is singular. Which is it?
> > >
> >
> > `switch-gpios` is gpios to controll USB switch connected to the
> > phy. Sophgo recommends that phy use a switch to separate device
> > port and host port if it supports both. I know this is weird,
> > but many board follows this design, such as Huashan PI and
> > Milkv Duo S. As for item number, it is just an array of gpios
> > to process one by one, I mark it as two just because Milkv
> > Duo S use two gpios to controll the USB switch.
>
> Right, but what I'm looking for is a description for what each GPIO
> does, so that someone can know how the dts should be written.
>
> > For vbus_det gpio description, There is because the design of
> > Milk-v Duo S, which connect the switch gpio and VBUS detect
> > gpio. In this case the OTG function is broken and we can just
> > change the mode by toggling switch gpios. So I suggest not
> > defining this property.
>
> Okay. See my comment on it above.
>
Thanks, I will add some necessary comments for these two properties.
> Thanks,
> Conor.