Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
	        };

        };
 };




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux