| Message ID | IA1PR20MB4953040232A4D41E41F2D2A9BB042@IA1PR20MB4953.namprd20.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: sophgo: add USB phy support for CV18XX series | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 <inochiama@outlook.com> > --- > .../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 <inochiama@outlook.com> > + > +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 <inochiama@outlook.com> > > --- > > .../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 <inochiama@outlook.com> > > + > > +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 > >
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 <inochiama@outlook.com> > > > --- > > > .../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 <inochiama@outlook.com> > > > + > > > +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 <inochiama@outlook.com> > > > > --- > > > > .../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 <inochiama@outlook.com> > > > > + > > > > +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.
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 <inochiama@outlook.com> + +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>; + };
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 <inochiama@outlook.com> --- .../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 -- 2.44.0