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@xxxxxxxxxxx> > > > > --- > > > > .../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@xxxxxxxxxxx> > > > > + > > > > +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.
