Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
From: Stephen Boyd <hidden>
Date: 2020-02-29 00:43:59
Also in:
lkml
Quoting Heikki Krogerus (2020-02-28 08:24:00)
On Thu, Feb 27, 2020 at 02:07:53PM -0800, Stephen Boyd wrote:quoted
Quoting Heikki Krogerus (2020-02-27 08:38:25)quoted
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.quoted
quoted
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 likeI 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.
quoted
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?quoted
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?
quoted
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
};
};
};