Date: Sat, 20 Apr 2024 09:39:18 +0800
From: Inochi Amaoto <inochiama@...look.com>
To: Conor Dooley <conor@...nel.org>, Inochi Amaoto <inochiama@...look.com>
Cc: Vinod Koul <vkoul@...nel.org>, 
	Kishon Vijay Abraham I <kishon@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Chen Wang <unicorn_wang@...look.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Jisheng Zhang <jszhang@...nel.org>, Liu Gui <kenneth.liu@...hgo.com>, 
	Jingbao Qiu <qiujingbao.dlmu@...il.com>, dlan@...too.org, linux-phy@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 1/2] dt-bindings: phy: Add Sophgo CV1800 USB phy

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@...look.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@...look.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
> > 

Powered by blists - more mailing lists