On Tue, Apr 30, 2019 at 1:08 AM Chen Yu <chenyu56@xxxxxxxxxx> wrote: > > Hi Rob, > > On 2019/4/26 5:35, Rob Herring wrote: > > On Sat, Apr 20, 2019 at 02:40:08PM +0800, Yu Chen wrote: > >> This patch adds binding documentation to support usb hub and usb > >> data role switch of Hisilicon HiKey960 Board. > > > > Sorry I've been slow to really review this, but I needed to look at the > > schematics to see what exactly is going on here. > > > > I think this needs some changes to better reflect the h/w and utilize > > existing bindings. It should really be designed ignoring the muxing to > > start with. Define the binding for the TypeC connector and then the host > > hub and make sure they can coexist. Then overlay what you need to switch > > between the 2 modes which AFAICT is just a single GPIO. > > > >> > >> Cc: Kishon Vijay Abraham I <kishon@xxxxxx> > >> Cc: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx> > >> Cc: Rob Herring <robh+dt@xxxxxxxxxx> > >> Cc: Mark Rutland <mark.rutland@xxxxxxx> > >> Cc: John Stultz <john.stultz@xxxxxxxxxx> > >> Cc: Binghui Wang <wangbinghui@xxxxxxxxxxxxx> > >> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx> > >> --- > >> v1: > >> * Fix some format errors as suggested by Sergei. > >> * Modify gpio description to use gpiod API. > >> v2: > >> * Remove information about Hikey. > >> * Fix gpio description. > >> * Remove device_type of endpoint. > >> v3: > >> * Remove property typec-vbus-enable-val. > >> * Add description of pinctrl-names. > >> * Add example for "hisilicon,gpio-hubv1" > >> * Add flag in gpiod properties. > >> --- > >> --- > >> .../bindings/misc/hisilicon-hikey-usb.txt | 52 ++++++++++++++++++++++ > >> 1 file changed, 52 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt > >> > >> diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt > >> new file mode 100644 > >> index 000000000000..422e844df719 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt > >> @@ -0,0 +1,52 @@ > >> +Support usb hub and usb data role switch of Hisilicon HiKey960 Board. > >> + > >> +----------------------------- > >> + > >> +Required properties: > >> +- compatible: "hisilicon,gpio-hubv1","hisilicon,hikey960-usb" > >> +- typec-vbus-gpios: gpio to control the vbus of typeC port > > > > This should be a gpio regulator and then connected to 'vbus-supply' in a > > usb-connector node (see .../bindings/connectors/usb-connector.txt). > Currently usb-connector node has no "vbus-supply" property and > I do not find process that handles vbus-supply in RT1711H TypeC driver. The patch[1] adding that is posted to the list and may not have landed yet. Whether the RT1711H TypeC driver handles it or not is not a binding problem. > > Then you also need the RT1711HWSC TypeC controller in DT. That is > > typically the parent device of the connector node. > > > >> +- otg-switch-gpios: gpio to switch DP & DM between the hub and typeC port > > > > This probably belongs in USB controller node. > > > The otg-switch-gpios controls a mux like fsusb30mux. It is related to > the board design of HiKey960. And the state of the mux is decided by > the typeC port state. So I think it is not so good to make it belongs > in USB controller node. Let me put it this way. The gpio property belongs wherever the mux is represented. In this case, I would expect the graph port representing the HS port to have 2 endpoints representing the 2 mux outputs. We don't generally put properties in the endpoint or port nodes, but the parent nodes. > >> +- hub-vdd33-en-gpios: gpio to enable the power of hub > > > > This too should be a gpio regulator and then in a hub node. We have 2 > > ways to represent hubs. Either as an I2C device or as a child of the > > host controller. The latter is preferred, but I'm not too sure how the > > OF graph connection linking the controller to the TypeC connector will > > work with the usb bus binding. > > > There is no particular code except the power control for the hub. > The i2c on the hub is not used. So it can not be an I2C device. > Is there such an example that make the hub as a child of the host controller > and control its power? Yes, bindings/usb/usb-device.txt. Rob [1] https://www.spinics.net/lists/kernel/msg3089136.html
