[PATCH RFC 0/4] phy: hisi-inno-phy: add support for hi3798mv200-usb2-phy

[PATCH RFC 0/4] phy: hisi-inno-phy: add support for hi3798mv200-usb2-phy

[Yang Xiwen via B4 Relay]

This should be considered a dirty hack. The proper solution would be
extracting write_reg logic to a separate regmap driver. Leaving only
"write BIT(2) to address 0x6" to the PHY driver.

The initial commit is already doing things wrong. The following patches
adding hi3798mv100 support is also very confusing. The name of the
enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
different across SoCs. But actually it's the bus (i.e. how to write to a
given address) which is different, not the PHY.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Yang Xiwen (4):
      dt-binding: phy: hisi-inno-usb2: convert to YAML
      phy: hisilicon: enable clocks for every ports
      phy: hisi-inno-usb2: add support for direct MMIO
      dt-binding: phy: hisi-inno-usb2: add compatible of hisilicon,hi3798mv200-usb2-phy

 .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 125 +++++++++++++++++++++
 .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 ------------
 drivers/phy/hisilicon/phy-hisi-inno-usb2.c         |  57 ++++++----
 3 files changed, 161 insertions(+), 92 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-inno-phy-a2d872f6b74b

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
compatible lists.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
 .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
 2 files changed, 115 insertions(+), 71 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
new file mode 100644
index 000000000000..73256eee10f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon HiSTB SoCs INNO USB2 PHY device
+
+maintainers:
+  - Yang Xiwen <forbidden405@outlook.com>
+
+properties:
+  compatible:
+    minItems: 1
+    items:
+      - enum:
+          - hisilicon,hi3798cv200-usb2-phy
+          - hisilicon,hi3798mv100-usb2-phy
+      - const: hisilicon,inno-usb2-phy
+
+  reg:
+    maxItems: 1
+    description: |
+      Should be the address space for PHY configuration register in peripheral
+      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
+      Or direct MMIO address space.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  clocks:
+    maxItems: 1
+    description: reference clock
+
+  resets:
+    maxItems: 1
+
+patternProperties:
+  'phy@[0-9a-f]+':
+    type: object
+    additionalProperties: false
+    description: individual ports provided by INNO PHY
+
+    properties:
+      reg:
+        maxItems: 1
+
+      '#phy-cells':
+        const: 0
+
+      resets:
+        maxItems: 1
+
+    required: [reg, '#phy-cells', resets]
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/histb-clock.h>
+
+    peripheral-controller@8a20000 {
+        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
+        reg = <0x8a20000 0x1000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges = <0x0 0x8a20000 0x1000>;
+
+        usb2-phy@120 {
+            compatible = "hisilicon,hi3798cv200-usb2-phy";
+            reg = <0x120 0x4>;
+            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
+            resets = <&crg 0xbc 4>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            phy@0 {
+                reg = <0>;
+                #phy-cells = <0>;
+                resets = <&crg 0xbc 8>;
+            };
+
+            phy@1 {
+                reg = <1>;
+                #phy-cells = <0>;
+                resets = <&crg 0xbc 9>;
+            };
+        };
+
+        usb2-phy@124 {
+            compatible = "hisilicon,hi3798cv200-usb2-phy";
+            reg = <0x124 0x4>;
+            clocks = <&crg HISTB_USB2_PHY2_REF_CLK>;
+            resets = <&crg 0xbc 6>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            phy@0 {
+                reg = <0>;
+                #phy-cells = <0>;
+                resets = <&crg 0xbc 10>;
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/phy/phy-hisi-inno-usb2.txt b/Documentation/devicetree/bindings/phy/phy-hisi-inno-usb2.txt
deleted file mode 100644
index 104953e849e7..000000000000
--- a/Documentation/devicetree/bindings/phy/phy-hisi-inno-usb2.txt
+++ /dev/null
@@ -1,71 +0,0 @@
-Device tree bindings for HiSilicon INNO USB2 PHY
-
-Required properties:
-- compatible: Should be one of the following strings:
-	"hisilicon,inno-usb2-phy",
-	"hisilicon,hi3798cv200-usb2-phy".
-- reg: Should be the address space for PHY configuration register in peripheral
-  controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
-- clocks: The phandle and clock specifier pair for INNO USB2 PHY device
-  reference clock.
-- resets: The phandle and reset specifier pair for INNO USB2 PHY device reset
-  signal.
-- #address-cells: Must be 1.
-- #size-cells: Must be 0.
-
-The INNO USB2 PHY device should be a child node of peripheral controller that
-contains the PHY configuration register, and each device supports up to 2 PHY
-ports which are represented as child nodes of INNO USB2 PHY device.
-
-Required properties for PHY port node:
-- reg: The PHY port instance number.
-- #phy-cells: Defined by generic PHY bindings.  Must be 0.
-- resets: The phandle and reset specifier pair for PHY port reset signal.
-
-Refer to phy/phy-bindings.txt for the generic PHY binding properties
-
-Example:
-
-perictrl: peripheral-controller@8a20000 {
-	compatible = "hisilicon,hi3798cv200-perictrl", "simple-mfd";
-	reg = <0x8a20000 0x1000>;
-	#address-cells = <1>;
-	#size-cells = <1>;
-	ranges = <0x0 0x8a20000 0x1000>;
-
-	usb2_phy1: usb2-phy@120 {
-		compatible = "hisilicon,hi3798cv200-usb2-phy";
-		reg = <0x120 0x4>;
-		clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
-		resets = <&crg 0xbc 4>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		usb2_phy1_port0: phy@0 {
-			reg = <0>;
-			#phy-cells = <0>;
-			resets = <&crg 0xbc 8>;
-		};
-
-		usb2_phy1_port1: phy@1 {
-			reg = <1>;
-			#phy-cells = <0>;
-			resets = <&crg 0xbc 9>;
-		};
-	};
-
-	usb2_phy2: usb2-phy@124 {
-		compatible = "hisilicon,hi3798cv200-usb2-phy";
-		reg = <0x124 0x4>;
-		clocks = <&crg HISTB_USB2_PHY2_REF_CLK>;
-		resets = <&crg 0xbc 6>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		usb2_phy2_port0: phy@0 {
-			reg = <0>;
-			#phy-cells = <0>;
-			resets = <&crg 0xbc 10>;
-		};
-	};
-};

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 2/4] phy: hisilicon: enable clocks for every ports

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

This is needed for port1 to work.

Fixes: ba8b0ee81fbb ("phy: add inno-usb2-phy driver for hi3798cv200 SoC")
Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
index c138cd4807d6..b7e740eb4752 100644
--- a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
+++ b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
@@ -86,8 +86,10 @@ static void hisi_inno_phy_write_reg(struct hisi_inno_phy_priv *priv,
 
 static void hisi_inno_phy_setup(struct hisi_inno_phy_priv *priv)
 {
+	int i;
 	/* The phy clk is controlled by the port0 register 0x06. */
-	hisi_inno_phy_write_reg(priv, 0, 0x06, PHY_CLK_ENABLE);
+	for (i = 0; i < INNO_PHY_PORT_NUM; i++)
+		hisi_inno_phy_write_reg(priv, i, 0x06, PHY_CLK_ENABLE);
 	msleep(PHY_CLK_STABLE_TIME);
 }
 

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 3/4] phy: hisi-inno-usb2: add support for direct MMIO

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

This method of resgiter access is used by Hi3798MV200. For other models,
of_iomap() returns 0 due to insufficient length. So they are unaffected.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 53 +++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
index b7e740eb4752..12a158fb749c 100644
--- a/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
+++ b/drivers/phy/hisilicon/phy-hisi-inno-usb2.c
@@ -10,6 +10,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
@@ -43,6 +44,7 @@
 #define PHY_CLK_ENABLE		BIT(2)
 
 struct hisi_inno_phy_port {
+	void __iomem *base;
 	struct reset_control *utmi_rst;
 	struct hisi_inno_phy_priv *priv;
 };
@@ -62,26 +64,31 @@ static void hisi_inno_phy_write_reg(struct hisi_inno_phy_priv *priv,
 	u32 val;
 	u32 value;
 
-	if (priv->type == PHY_TYPE_0)
-		val = (data & PHY_TEST_DATA) |
-		      ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
-		      ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
-		      PHY0_TEST_WREN | PHY0_TEST_RST;
-	else
-		val = (data & PHY_TEST_DATA) |
-		      ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
-		      ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
-		      PHY1_TEST_WREN | PHY1_TEST_RST;
-	writel(val, reg);
-
-	value = val;
-	if (priv->type == PHY_TYPE_0)
-		value |= PHY0_TEST_CLK;
-	else
-		value |= PHY1_TEST_CLK;
-	writel(value, reg);
-
-	writel(val, reg);
+	if (priv->ports[port].base)
+		// stride is 4
+		writel(data, (u32 *)priv->ports[port].base + addr);
+	else {
+		if (priv->type == PHY_TYPE_0)
+			val = (data & PHY_TEST_DATA) |
+			      ((addr << PHY_TEST_ADDR_OFFSET) & PHY0_TEST_ADDR) |
+			      ((port << PHY0_TEST_PORT_OFFSET) & PHY0_TEST_PORT) |
+			      PHY0_TEST_WREN | PHY0_TEST_RST;
+		else
+			val = (data & PHY_TEST_DATA) |
+			      ((addr << PHY_TEST_ADDR_OFFSET) & PHY1_TEST_ADDR) |
+			      ((port << PHY1_TEST_PORT_OFFSET) & PHY1_TEST_PORT) |
+			      PHY1_TEST_WREN | PHY1_TEST_RST;
+		writel(val, reg);
+
+		value = val;
+		if (priv->type == PHY_TYPE_0)
+			value |= PHY0_TEST_CLK;
+		else
+			value |= PHY1_TEST_CLK;
+		writel(value, reg);
+
+		writel(val, reg);
+	}
 }
 
 static void hisi_inno_phy_setup(struct hisi_inno_phy_priv *priv)
@@ -167,6 +174,7 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
 	for_each_child_of_node(np, child) {
 		struct reset_control *rst;
 		struct phy *phy;
+		void __iomem *base;
 
 		rst = of_reset_control_get_exclusive(child, NULL);
 		if (IS_ERR(rst)) {
@@ -174,7 +182,10 @@ static int hisi_inno_phy_probe(struct platform_device *pdev)
 			return PTR_ERR(rst);
 		}
 
+		base = of_iomap(child, 0);
+
 		priv->ports[i].utmi_rst = rst;
+		priv->ports[i].base = base;
 		priv->ports[i].priv = priv;
 
 		phy = devm_phy_create(dev, child, &hisi_inno_phy_ops);
@@ -205,6 +216,8 @@ static const struct of_device_id hisi_inno_phy_of_match[] = {
 	  .data = (void *) PHY_TYPE_0 },
 	{ .compatible = "hisilicon,hi3798mv100-usb2-phy",
 	  .data = (void *) PHY_TYPE_1 },
+	{ .compatible = "hisilicon,hi3798mv200-usb2-phy",
+	  .data = (void *) PHY_TYPE_0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, hisi_inno_phy_of_match);

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH RFC 4/4] dt-binding: phy: hisi-inno-usb2: add compatible of hisilicon,hi3798mv200-usb2-phy

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

It is accessed by direct MMIO, making "ranges" property mandatory for
it.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 .../devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml       | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
index 73256eee10f9..d702878b8e6e 100644
--- a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
@@ -16,6 +16,7 @@ properties:
       - enum:
           - hisilicon,hi3798cv200-usb2-phy
           - hisilicon,hi3798mv100-usb2-phy
+          - hisilicon,hi3798mv200-usb2-phy
       - const: hisilicon,inno-usb2-phy
 
   reg:
@@ -64,6 +65,15 @@ required:
   - '#size-cells'
   - resets
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: hisilicon,hi3798mv200-inno-usb2-phy
+then:
+  required:
+    - ranges
+
 additionalProperties: false
 
 examples:

-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Krzysztof Kozlowski]

On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
> compatible lists.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>  .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>  2 files changed, 115 insertions(+), 71 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> new file mode 100644
> index 000000000000..73256eee10f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
> +
> +maintainers:
> +  - Yang Xiwen <forbidden405@outlook.com>
> +
> +properties:
> +  compatible:
> +    minItems: 1

No, why? Compatibles must be fixed/constrained.

> +    items:
> +      - enum:
> +          - hisilicon,hi3798cv200-usb2-phy
> +          - hisilicon,hi3798mv100-usb2-phy

This wasn't here before. Not explained in commit msg.

> +      - const: hisilicon,inno-usb2-phy
> +
> +  reg:
> +    maxItems: 1
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Should be the address space for PHY configuration register in peripheral
> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
> +      Or direct MMIO address space.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  clocks:
> +    maxItems: 1
> +    description: reference clock
> +
> +  resets:
> +    maxItems: 1
> +
> +patternProperties:
> +  'phy@[0-9a-f]+':
> +    type: object
> +    additionalProperties: false
> +    description: individual ports provided by INNO PHY
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +      '#phy-cells':
> +        const: 0
> +
> +      resets:
> +        maxItems: 1
> +
> +    required: [reg, '#phy-cells', resets]

One item per line. Look at other bindings or example-schema.

> +
> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/histb-clock.h>
> +
> +    peripheral-controller@8a20000 {
> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
> +        reg = <0x8a20000 0x1000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges = <0x0 0x8a20000 0x1000>;

Drop the node, not related to this binding. If this binding is supposed
to be part of other device in case of MFD devices or some tightly
coupled ones, then could be included in the example there.

> +
> +        usb2-phy@120 {
> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
> +            reg = <0x120 0x4>;
> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
> +            resets = <&crg 0xbc 4>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            phy@0 {
> +                reg = <0>;
> +                #phy-cells = <0>;
> +                resets = <&crg 0xbc 8>;
> +            };
> +
> +            phy@1 {
> +                reg = <1>;
> +                #phy-cells = <0>;
> +                resets = <&crg 0xbc 9>;
> +            };
> +        };
> +
> +        usb2-phy@124 {
> +            compatible = "hisilicon,hi3798cv200-usb2-phy";

You can keep only one example, because they are basically the same.


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 4/4] dt-binding: phy: hisi-inno-usb2: add compatible of hisilicon,hi3798mv200-usb2-phy

[Krzysztof Kozlowski]

On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> It is accessed by direct MMIO, making "ranges" property mandatory for
> it.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  .../devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml       | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> index 73256eee10f9..d702878b8e6e 100644
> --- a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> @@ -16,6 +16,7 @@ properties:
>        - enum:
>            - hisilicon,hi3798cv200-usb2-phy
>            - hisilicon,hi3798mv100-usb2-phy
> +          - hisilicon,hi3798mv200-usb2-phy
>        - const: hisilicon,inno-usb2-phy
>  
>    reg:
> @@ -64,6 +65,15 @@ required:
>    - '#size-cells'
>    - resets
>  
> +if:

allOf: above.

> +  properties:
> +    compatible:
> +      contains:
> +        const: hisilicon,hi3798mv200-inno-usb2-phy
> +then:
> +  required:
> +    - ranges
So please test your DTS first.

> 

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 0/4] phy: hisi-inno-phy: add support for hi3798mv200-usb2-phy

[Krzysztof Kozlowski]

On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> This should be considered a dirty hack. The proper solution would be
> extracting write_reg logic to a separate regmap driver. Leaving only
> "write BIT(2) to address 0x6" to the PHY driver.
> 
> The initial commit is already doing things wrong. The following patches
> adding hi3798mv100 support is also very confusing. The name of the
> enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
> different across SoCs. But actually it's the bus (i.e. how to write to a
> given address) which is different, not the PHY.

I have many bounces from your emails. Please do not Cc unrelated,
non-working hisilicon emails.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Yang Xiwen]

On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>> compatible lists.
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
Will fix in next version
>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>>   .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>>   2 files changed, 115 insertions(+), 71 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> new file mode 100644
>> index 000000000000..73256eee10f9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> @@ -0,0 +1,115 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@outlook.com>
>> +
>> +properties:
>> +  compatible:
>> +    minItems: 1
> No, why? Compatibles must be fixed/constrained.
>
>> +    items:
>> +      - enum:
>> +          - hisilicon,hi3798cv200-usb2-phy
>> +          - hisilicon,hi3798mv100-usb2-phy
> This wasn't here before. Not explained in commit msg.
The commit 3940ffc65492 ("phy: hisilicon: Add inno-usb2-phy driver for 
Hi3798MV100") does not have dt-binding change commit along with. Will 
explain this in commit log.
>
>> +      - const: hisilicon,inno-usb2-phy
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: |
> Do not need '|' unless you need to preserve formatting.
>
>> +      Should be the address space for PHY configuration register in peripheral
>> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
>> +      Or direct MMIO address space.
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: reference clock
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  'phy@[0-9a-f]+':
>> +    type: object
>> +    additionalProperties: false
>> +    description: individual ports provided by INNO PHY
>> +
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +
>> +      '#phy-cells':
>> +        const: 0
>> +
>> +      resets:
>> +        maxItems: 1
>> +
>> +    required: [reg, '#phy-cells', resets]
> One item per line. Look at other bindings or example-schema.
>
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - resets
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/histb-clock.h>
>> +
>> +    peripheral-controller@8a20000 {
>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>> +        reg = <0x8a20000 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges = <0x0 0x8a20000 0x1000>;
> Drop the node, not related to this binding. If this binding is supposed
> to be part of other device in case of MFD devices or some tightly
> coupled ones, then could be included in the example there.
For CV200, this binding is supposed to be always inside the perictrl 
device. The PHY address space are accessed from a bus implemented by 
perictrl.
>
>> +
>> +        usb2-phy@120 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>> +            reg = <0x120 0x4>;
>> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
>> +            resets = <&crg 0xbc 4>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            phy@0 {
>> +                reg = <0>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 8>;
>> +            };
>> +
>> +            phy@1 {
>> +                reg = <1>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 9>;
>> +            };
>> +        };
>> +
>> +        usb2-phy@124 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
> You can keep only one example, because they are basically the same.
Will remove
>
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Krzysztof Kozlowski]

On 17/02/2024 11:24, Yang Xiwen wrote:

>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/histb-clock.h>
>>> +
>>> +    peripheral-controller@8a20000 {
>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>> +        reg = <0x8a20000 0x1000>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges = <0x0 0x8a20000 0x1000>;
>> Drop the node, not related to this binding. If this binding is supposed
>> to be part of other device in case of MFD devices or some tightly
>> coupled ones, then could be included in the example there.
> For CV200, this binding is supposed to be always inside the perictrl 
> device. The PHY address space are accessed from a bus implemented by 
> perictrl.

Every node is supposes to be somewhere in something, so with this logic
you would start from soc@. What's wrong in putting it in parent schema?

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Yang Xiwen]

On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 11:24, Yang Xiwen wrote:
>
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/clock/histb-clock.h>
>>>> +
>>>> +    peripheral-controller@8a20000 {
>>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>>> +        reg = <0x8a20000 0x1000>;
>>>> +        #address-cells = <1>;
>>>> +        #size-cells = <1>;
>>>> +        ranges = <0x0 0x8a20000 0x1000>;
>>> Drop the node, not related to this binding. If this binding is supposed
>>> to be part of other device in case of MFD devices or some tightly
>>> coupled ones, then could be included in the example there.
>> For CV200, this binding is supposed to be always inside the perictrl
>> device. The PHY address space are accessed from a bus implemented by
>> perictrl.
> Every node is supposes to be somewhere in something, so with this logic
> you would start from soc@. What's wrong in putting it in parent schema?

When the ports reg property only has an dummy address (no size), it must 
be inside the perictrl node, accessed from the bus implemented by perictrl.

But when the ports has its own MMIO address space (mv200), it should be 
located under a simple-bus node instead.

So it's either:

perictrl@8a20000 {

     usb2-phy@120: {

         reg = <0x120 0x4>; // this is the register that controls writes 
and reads to the phy, implemented by perictrl. (just like SPI/I2C)

         phy@0: {

             reg = <0>; // the reg is used as an index

         };

     };

};

or:

soc@0 {

     usb2-phy@f9865000 { // MMIO

         reg = <0xf9865000 0x1000>

         port0@0 {

             reg = <0x0 0x400>; // used as MMIO address space

         };

     };

};

So here is why i include perictrl node in the example. If the ports are 
not mmio, the phy must be under a perictrl node. Or if the ports has its 
own address space, it should not be under a perictrl node, but rather an 
simple-bus node.

>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 0/4] phy: hisi-inno-phy: add support for hi3798mv200-usb2-phy

[Yang Xiwen]

On 2/17/2024 6:18 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> This should be considered a dirty hack. The proper solution would be
>> extracting write_reg logic to a separate regmap driver. Leaving only
>> "write BIT(2) to address 0x6" to the PHY driver.
>>
>> The initial commit is already doing things wrong. The following patches
>> adding hi3798mv100 support is also very confusing. The name of the
>> enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
>> different across SoCs. But actually it's the bus (i.e. how to write to a
>> given address) which is different, not the PHY.
> I have many bounces from your emails. Please do not Cc unrelated,
> non-working hisilicon emails.
I can't recall why Tian is Cced here. Will remove next time.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 0/4] phy: hisi-inno-phy: add support for hi3798mv200-usb2-phy

[Yang Xiwen]

On 2/17/2024 6:18 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> This should be considered a dirty hack. The proper solution would be
>> extracting write_reg logic to a separate regmap driver. Leaving only
>> "write BIT(2) to address 0x6" to the PHY driver.
>>
>> The initial commit is already doing things wrong. The following patches
>> adding hi3798mv100 support is also very confusing. The name of the
>> enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
>> different across SoCs. But actually it's the bus (i.e. how to write to a
>> given address) which is different, not the PHY.
> I have many bounces from your emails. Please do not Cc unrelated,
> non-working hisilicon emails.
All except one email addresses are added by `b4 prep --auto-to-cc`. 
Anyway, i'll remove the email address that does not work in next version.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Yang Xiwen]

On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>> compatible lists.
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>>   .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>>   2 files changed, 115 insertions(+), 71 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> new file mode 100644
>> index 000000000000..73256eee10f9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>> @@ -0,0 +1,115 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@outlook.com>
>> +
>> +properties:
>> +  compatible:
>> +    minItems: 1
> No, why? Compatibles must be fixed/constrained.
Hi3798CV200 has only the first compatible listed in its device tree. But 
you are right i can add it to hi3798mv200.dtsi so that `minItems` can be 
removed
>
>> +    items:
>> +      - enum:
>> +          - hisilicon,hi3798cv200-usb2-phy
>> +          - hisilicon,hi3798mv100-usb2-phy
> This wasn't here before. Not explained in commit msg.
>
>> +      - const: hisilicon,inno-usb2-phy
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: |
> Do not need '|' unless you need to preserve formatting.
>
>> +      Should be the address space for PHY configuration register in peripheral
>> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
>> +      Or direct MMIO address space.
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: reference clock
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  'phy@[0-9a-f]+':
>> +    type: object
>> +    additionalProperties: false
>> +    description: individual ports provided by INNO PHY
>> +
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +
>> +      '#phy-cells':
>> +        const: 0
>> +
>> +      resets:
>> +        maxItems: 1
>> +
>> +    required: [reg, '#phy-cells', resets]
> One item per line. Look at other bindings or example-schema.
>
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - resets
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/histb-clock.h>
>> +
>> +    peripheral-controller@8a20000 {
>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>> +        reg = <0x8a20000 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges = <0x0 0x8a20000 0x1000>;
> Drop the node, not related to this binding. If this binding is supposed
> to be part of other device in case of MFD devices or some tightly
> coupled ones, then could be included in the example there.
>
>> +
>> +        usb2-phy@120 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>> +            reg = <0x120 0x4>;
>> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
>> +            resets = <&crg 0xbc 4>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            phy@0 {
>> +                reg = <0>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 8>;
>> +            };
>> +
>> +            phy@1 {
>> +                reg = <1>;
>> +                #phy-cells = <0>;
>> +                resets = <&crg 0xbc 9>;
>> +            };
>> +        };
>> +
>> +        usb2-phy@124 {
>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
> You can keep only one example, because they are basically the same.

It is listed here just because cv200 (and the upcoming mv200) actually 
has two INNO phys in the SoC. And coincidently for both SoCs, one with 
two ports is wired to USB2 controller(EHCI &OHCI), while the other one 
with only one port is wired to DWC3 controller. The example here is 
borrowed directly from hi3798cv200.dtsi.

>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Krzysztof Kozlowski]

On 17/02/2024 11:54, Yang Xiwen wrote:
> On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote:
>> On 17/02/2024 11:24, Yang Xiwen wrote:
>>
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/clock/histb-clock.h>
>>>>> +
>>>>> +    peripheral-controller@8a20000 {
>>>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>>>> +        reg = <0x8a20000 0x1000>;
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <1>;
>>>>> +        ranges = <0x0 0x8a20000 0x1000>;
>>>> Drop the node, not related to this binding. If this binding is supposed
>>>> to be part of other device in case of MFD devices or some tightly
>>>> coupled ones, then could be included in the example there.
>>> For CV200, this binding is supposed to be always inside the perictrl
>>> device. The PHY address space are accessed from a bus implemented by
>>> perictrl.
>> Every node is supposes to be somewhere in something, so with this logic
>> you would start from soc@. What's wrong in putting it in parent schema?
> 
> When the ports reg property only has an dummy address (no size), it must 
> be inside the perictrl node, accessed from the bus implemented by perictrl.
> 
> But when the ports has its own MMIO address space (mv200), it should be 
> located under a simple-bus node instead.
> 
> So it's either:
> 
> perictrl@8a20000 {
> 
>      usb2-phy@120: {
> 
>          reg = <0x120 0x4>; // this is the register that controls writes 
> and reads to the phy, implemented by perictrl. (just like SPI/I2C)
> 
>          phy@0: {
> 
>              reg = <0>; // the reg is used as an index
> 
>          };
> 
>      };
> 
> };
> 
> or:
> 
> soc@0 {
> 
>      usb2-phy@f9865000 { // MMIO
> 
>          reg = <0xf9865000 0x1000>
> 
>          port0@0 {
> 
>              reg = <0x0 0x400>; // used as MMIO address space
> 
>          };
> 
>      };
> 
> };
> 
> So here is why i include perictrl node in the example. If the ports are 
> not mmio, the phy must be under a perictrl node. Or if the ports has its 
> own address space, it should not be under a perictrl node, but rather an 
> simple-bus node.

I don't understand why you keep insisting and discussing this. You are
adding other compatibles to this schema example, which usually we try to
avoid. You entirely ignored my comment above and pasted DTS which is no
related to the topic we discuss here. I did not question whether this
can be or cannot be in some node.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Krzysztof Kozlowski]

On 17/02/2024 14:14, Yang Xiwen wrote:
> On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote:
>> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>>> From: Yang Xiwen <forbidden405@outlook.com>
>>>
>>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
>>> compatible lists.
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>>
>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>>> ---
>>>   .../bindings/phy/hisilicon,inno-usb2-phy.yaml      | 115 +++++++++++++++++++++
>>>   .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt |  71 -------------
>>>   2 files changed, 115 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>>> new file mode 100644
>>> index 000000000000..73256eee10f9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
>>> @@ -0,0 +1,115 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device
>>> +
>>> +maintainers:
>>> +  - Yang Xiwen <forbidden405@outlook.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    minItems: 1
>> No, why? Compatibles must be fixed/constrained.
> Hi3798CV200 has only the first compatible listed in its device tree. But 
> you are right i can add it to hi3798mv200.dtsi so that `minItems` can be 
> removed
>>
>>> +    items:
>>> +      - enum:
>>> +          - hisilicon,hi3798cv200-usb2-phy
>>> +          - hisilicon,hi3798mv100-usb2-phy
>> This wasn't here before. Not explained in commit msg.
>>
>>> +      - const: hisilicon,inno-usb2-phy
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +    description: |
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +      Should be the address space for PHY configuration register in peripheral
>>> +      controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC.
>>> +      Or direct MMIO address space.
>>> +
>>> +  '#address-cells':
>>> +    const: 1
>>> +
>>> +  '#size-cells':
>>> +    const: 0
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +    description: reference clock
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
>>> +patternProperties:
>>> +  'phy@[0-9a-f]+':
>>> +    type: object
>>> +    additionalProperties: false
>>> +    description: individual ports provided by INNO PHY
>>> +
>>> +    properties:
>>> +      reg:
>>> +        maxItems: 1
>>> +
>>> +      '#phy-cells':
>>> +        const: 0
>>> +
>>> +      resets:
>>> +        maxItems: 1
>>> +
>>> +    required: [reg, '#phy-cells', resets]
>> One item per line. Look at other bindings or example-schema.
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - clocks
>>> +  - reg
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +  - resets
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/histb-clock.h>
>>> +
>>> +    peripheral-controller@8a20000 {
>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>> +        reg = <0x8a20000 0x1000>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges = <0x0 0x8a20000 0x1000>;
>> Drop the node, not related to this binding. If this binding is supposed
>> to be part of other device in case of MFD devices or some tightly
>> coupled ones, then could be included in the example there.
>>
>>> +
>>> +        usb2-phy@120 {
>>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>>> +            reg = <0x120 0x4>;
>>> +            clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
>>> +            resets = <&crg 0xbc 4>;
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            phy@0 {
>>> +                reg = <0>;
>>> +                #phy-cells = <0>;
>>> +                resets = <&crg 0xbc 8>;
>>> +            };
>>> +
>>> +            phy@1 {
>>> +                reg = <1>;
>>> +                #phy-cells = <0>;
>>> +                resets = <&crg 0xbc 9>;
>>> +            };
>>> +        };
>>> +
>>> +        usb2-phy@124 {
>>> +            compatible = "hisilicon,hi3798cv200-usb2-phy";
>> You can keep only one example, because they are basically the same.
> 
> It is listed here just because cv200 (and the upcoming mv200) actually 
> has two INNO phys in the SoC. And coincidently for both SoCs, one with 
> two ports is wired to USB2 controller(EHCI &OHCI), while the other one 
> with only one port is wired to DWC3 controller. The example here is 
> borrowed directly from hi3798cv200.dtsi.
> 

I see the differences and one difference means they are basically the same.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH RFC 1/4] dt-binding: phy: hisi-inno-usb2: convert to YAML

[Yang Xiwen]

On 2/17/2024 9:39 PM, Krzysztof Kozlowski wrote:
> On 17/02/2024 11:54, Yang Xiwen wrote:
>> On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote:
>>> On 17/02/2024 11:24, Yang Xiwen wrote:
>>>
>>>>>> +
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    #include <dt-bindings/clock/histb-clock.h>
>>>>>> +
>>>>>> +    peripheral-controller@8a20000 {
>>>>>> +        compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd";
>>>>>> +        reg = <0x8a20000 0x1000>;
>>>>>> +        #address-cells = <1>;
>>>>>> +        #size-cells = <1>;
>>>>>> +        ranges = <0x0 0x8a20000 0x1000>;
>>>>> Drop the node, not related to this binding. If this binding is supposed
>>>>> to be part of other device in case of MFD devices or some tightly
>>>>> coupled ones, then could be included in the example there.
>>>> For CV200, this binding is supposed to be always inside the perictrl
>>>> device. The PHY address space are accessed from a bus implemented by
>>>> perictrl.
>>> Every node is supposes to be somewhere in something, so with this logic
>>> you would start from soc@. What's wrong in putting it in parent schema?
>> When the ports reg property only has an dummy address (no size), it must
>> be inside the perictrl node, accessed from the bus implemented by perictrl.
>>
>> But when the ports has its own MMIO address space (mv200), it should be
>> located under a simple-bus node instead.
>>
>> So it's either:
>>
>> perictrl@8a20000 {
>>
>>       usb2-phy@120: {
>>
>>           reg = <0x120 0x4>; // this is the register that controls writes
>> and reads to the phy, implemented by perictrl. (just like SPI/I2C)
>>
>>           phy@0: {
>>
>>               reg = <0>; // the reg is used as an index
>>
>>           };
>>
>>       };
>>
>> };
>>
>> or:
>>
>> soc@0 {
>>
>>       usb2-phy@f9865000 { // MMIO
>>
>>           reg = <0xf9865000 0x1000>
>>
>>           port0@0 {
>>
>>               reg = <0x0 0x400>; // used as MMIO address space
>>
>>           };
>>
>>       };
>>
>> };
>>
>> So here is why i include perictrl node in the example. If the ports are
>> not mmio, the phy must be under a perictrl node. Or if the ports has its
>> own address space, it should not be under a perictrl node, but rather an
>> simple-bus node.
> I don't understand why you keep insisting and discussing this. You are
> adding other compatibles to this schema example, which usually we try to
> avoid. You entirely ignored my comment above and pasted DTS which is no
> related to the topic we discuss here. I did not question whether this
> can be or cannot be in some node.
okay, I can remove the parent node if it's preferred. This is simply the 
most usual example that exists in real dts.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy