Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
From: Stephen Boyd
Date: Fri Feb 28 2020 - 19:43:58 EST
Quoting Heikki Krogerus (2020-02-28 08:24:00)
> On Thu, Feb 27, 2020 at 02:07:53PM -0800, Stephen Boyd wrote:
> > Quoting Heikki Krogerus (2020-02-27 08:38:25)
> > > No.
> > >
> > > The above follows the usb-connector bindings, so it is correct:
> > > Documentation/devicetree/bindings/connector/usb-connector.txt
> > >
> > > So the connector is always a child of the "CC controller" with the USB
> > > Type-C connectors, which in this case is the EC (from operating systems
> > > perspective). The "CC controller" controls connectors, and it doesn't
> > > actually do anything else. So placing the connectors under the
> > > "connector controller" is also logically correct.
> >
> > Ah ok I see. The graph binding is for describing the data path, not the
> > control path. Makes sense.
> >
> > >
> > > > Yes, the connector is intimately involved with the EC here, but I would
> > > > think that we would have an OF graph connection from the USB controller
> > > > on the SoC to the USB connector, traversing through anything that may be
> > > > in that path, such as a USB hub. Maybe the connector node itself can
> > > > point to the EC type-c controller with some property like
> > >
> > > I think your idea here is that there should be only a single node for
> > > each connector that is then linked with every component that it is
> > > physically connected to (right?), but please note that that is not
> > > enough. Every component attached to the connector must have its own
> > > child node that represents the "port" that is physically connected to
> > > the USB Type-C connector.
> > >
> > > So for example, the USB controller nodes have child nodes for every
> > > USB2 port as well as for every USB3 port. Similarly, the GPU
> > > controllers have child node for every DisplayPort, etc. And I believe
> > > that is already how it has been done in DT (and also in ACPI).
> >
> > It looks like perhaps you're conflating ports in USB spec with the OF
> > graph port? I want there to be one node per type-c connector that I can
> > physically see on the device. Is that not sufficient?
>
> It is. We don't need more than one node that represents the physical
> connector (and we should not have more than one node for that). And
> actually, I was not mixing the OF graph ports and USB ports... I
> think I should be talking about PHY instead of "port". That is
> probable more clear.
>
> My point is that every PHY that is connected to a Type-C connector
> must still be represented with its own node in devicetree and ACPI. So
> there still needs to be a node for the USB2 PHY, USB3 PHY, DisplayPort
> PHY, etc., on top of the connector node. I got the picture that you
> are proposing that we don't need those PHY nodes anymore since we have
> the connector nodes, but maybe I misunderstood?
Alright. Maybe a full example will help. See below. I think I understand
how it's supposed to look.
>
> > Are there any examples of the type-c connector in DT? I see some
> > NXP/Freescale boards and one Renesas board so far. Maybe there are other
> > discussions I can read up on?
> >
> > >
> > > Those "port" nodes then just need to be linked with the "connector"
> > > node. I think for that the idea was to use OF graph, but I'm really
> > > sceptical about that. The problem is that with the USB Type-C
> > > connectors we have to be able to identify the connections, i.e. which
> > > endpoint is the USB2 port, which is the DisplayPort and so on, and OF
> > > graph does not give any means to do that on its own. We will have to
> > > rely on separate device properties in order to do the identification.
> > > Currently it is not documented anywhere which property should be used
> > > for that.
> >
> > I hope that this patch series can document this.
>
> Well, we do need that to be documented, but do we really need to block
> this series because of that? This driver does not depend on OF graph
> yet.
I don't know. I think this binding patch will go for another round so
maybe it's blocked in other ways?
>
> > Why can't that work by having multiple OF graph ports for USB2 port,
> > DisplayPort, USB3 port, etc? The data path goes to the connector and
> > we can attach more information to each port node to describe what
> > type of endpoint is there like a DisplayPort capable type-c
> > connector for example.
>
> The PHY nodes we must still always have. So the OF graph will always
> describe the connection between the PHY and the connector, and the
> connection between the PHY and the controller must be described
> separately.
Got it.
Here's the same example that hopefully shows how all this stuff can
work. I've added more nonsense to try and make it as complicated as
possible.
/ {
// Expand single usb2/usb3 from SoC to 4 port hub
usb-hub {
compatible = "vendor,usb-hub-4-port";
usb-vid = <0xaaad>;
usb-pid = <0xffed>;
vdd-supply = <&pp3300_usb>;
reset-gpios = <&gpio_controller 50 GPIO_ACTIVE_LOW>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
usb2_hub0: endpoint0 {
remote-endpoint = <&left_typec2>;
};
usb3_hub0: endpoint1 {
remote-endpoint = <&left_typec3>;
};
};
port@1 {
reg = <1>;
usb2_hub1: endpoint0 {
remote-endpoint = <&right_typec2>;
};
usb3_hub1: endpoint1 {
remote-endpoint = <&right_typec3>;
};
};
port@2 {
reg = <2>;
usb2_hub2: endpoint0 {
remote-endpoint = <&left_typea2>;
};
usb3_hub2: endpoint1 {
remote-endpoint = <&left_typea3>;
};
};
port@3 {
reg = <3>;
usb2_hub3: endpoint0 {
// Not connected
};
usb3_hub3: endpoint1 {
// Not connected
};
};
port@4 {
reg = <4>;
usb2_hub_in: endpoint0 {
remote-endpoint = <&usb2_phy_out>;
};
usb3_hub_in: endpoint1 {
remote-endpoint = <&usb3_phy_out>;
};
};
};
};
// Maybe this should go under EC node too if EC controls it
// somehow?
connector {
compatible = "usb-a-connector";
label = "type-A-left"
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
left_typea2: endpoint0 {
remote-endpoint = <&usb2_hub2>;
};
left_typea3: endpoint1 {
remote-endpoint = <&usb3_hub2>;
};
};
};
// Steer DP to either left or right type-c port
mux {
compatible = "vendor,dp-mux";
// Inactive: port 0
// Active: port 1
mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
mux_dp_0: endpoint {
remote-endpoint = <&right_typec_dp>;
};
};
port@1 {
reg = <1>;
mux_dp_1: endpoint {
remote-endpoint = <&left_typec_dp>;
};
};
port@2 {
reg = <1>;
mux_dp_in: endpoint {
remote-endpoint = <&dp_phy_out>;
};
};
};
};
soc@0 {
#address-cells = <1>;
#size-cells = <0>;
spi@a000000 {
compatible = "soc,spi-controller";
reg = <0xa000000 0x1000>;
cros_ec: ec@0 {
compatible = "google,cros-ec-spi";
reg = <0>;
connector@0 {
compatible = "usb-c-connector";
label = "type-c-left";
reg = <0>;
power-role = "dual";
...
ports { // Maybe ports is overkill with only one port?
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
left_typec2: endpoint0 {
remote-endpoint = <&usb2_hub0>;
};
left_typec3: endpoint1 {
remote-endpoint = <&usb3_hub0>;
};
left_typec_dp: endpoint2 {
remote-endpoint = <&mux_dp_1>;
};
};
};
};
connector@1 {
compatible = "usb-c-connector";
label = "type-c-right";
reg = <1>;
power-role = "dual";
...
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
right_typec2: endpoint0 {
remote-endpoint = <&usb2_hub1>;
};
right_typec3: endpoint1 {
remote-endpoint = <&usb3_hub1>;
};
right_typec_dp: endpoint2 {
remote-endpoint = <&mux_dp_0>;
};
};
};
};
};
};
usb2_phy: phy@da00000 {
compatible = "soc,usb2-phy";
reg = <0xda00000 0x1000>;
ports {
port@0 {
reg = <0>;
usb2_phy_out: endpoint {
remote-endpoint = <&usb2_hub_in>;
};
};
};
};
usb3_phy: phy@db00000 {
compatible = "soc,usb3-phy";
reg = <0xdb00000 0x1000>;
ports {
port@0 {
reg = <0>;
usb3_phy_out: endpoint {
remote-endpoint = <&usb3_hub_in>;
};
};
};
};
dp_phy: phy@dc00000 {
compatible = "soc,dp-phy";
reg = <0xdc00000 0x1000>;
ports {
port@0 {
reg = <0>;
dp_phy_out: endpoint {
remote-endpoint = <&mux_dp_in>;
};
};
};
};
usb@ea00000 {
compatible = "soc,dwc3-controller";
reg = <0xea00000 0x1000>;
phys = <&usb2_phy>, <&usb3_phy>;
};
display-controller@eb00000 {
compatible = "soc,dwc3-controller";
reg = <0xeb00000 0x1000>;
phys = <&dp_phy>;
// TODO: Connect audio to DP phy somehow
};
};
};