Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

From: Pin-yen Lin
Date: Sun Oct 02 2022 - 23:51:20 EST

Next message: Ira Weiny: "Re: [PATCH][CFT] [coredump] don't use __kernel_write() on kmap_local_page()"
Previous message: Eric Biggers: "[GIT PULL] STATX_DIOALIGN for 6.1"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi all,

Are there any thoughts or comments about this proposal?

On Sat, Sep 17, 2022 at 2:22 AM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
>
> Hi folks,
>
> On Fri, Sep 2, 2022 at 12:41 AM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > On Jul 12 11:45, Rob Herring wrote:
> > >
> > > That's not the right interpretation. There should not be some Type-C
> > > specific child mux/switch node because the device has no such h/w within
> > > it. Assuming all the possibilities Stephen outlined are valid, it's
> > > clear this lane selection has nothing to do with Type-C. It does have an
> > > output port for its DP output already and using that to describe the
> > > connection to DP connector(s) and/or Type-C connector(s) should be
> > > handled.
> > > Rob
> >
> > Below I've listed the proposal binding (for the Type-C connector) along
> > with 2 sample hardware diagrams and corresponding DT.
>
> Any thoughts about this proposal?
>
> >
> > The updated binding in usb-c-connector would be as follows:
> >
> > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > index ae515651fc6b..a043b09cb8ec 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > @@ -183,6 +183,30 @@ properties:
> > port@1:
> > $ref: /schemas/graph.yaml#/properties/port
> > description: Super Speed (SS), present in SS capable connectors.
> > + properties:
> > + '#address-cells':
> > + const: 1
> > +
> > + '#size-cells':
> > + const: 0
> > +
> > + patternProperties:
> > + "^endpoint@[0-1]$":
> > + $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > + description:
> > + Endpoints for the two SS lanes. endpoint@0 refers to SSTRX1 (A2,A3,B10,B11)
> > + and endpoint@1 refers to SSTRX2 (B2,B3,A10,A11).
> > + additionalProperties: false
> > +
> > + properties:
> > + reg:
> > + maxItems: 1
> > +
> > + remote-endpoint: true
> > +
> > + required:
> > + - reg
> > + - remote-endpoint
> >
> > port@2:
> > $ref: /schemas/graph.yaml#/properties/port
> >
> > Here are 2 examples of how that would look on some existing hardware:
> >
> > Example 1. 2 usb-c-connectors connecting to 1 drm bridge / DP switch:
> >
> > Here is the diagram we are using on the MTK platform:
> >
> > SOC
> > +---------------------+ C0
> > | | +----------+ 2 lane +--------+
> > | | | +---------/---------+ SSTRX1 |
> > | | | | | |
> > | MIPI DPI | | | 2 lane | |
> > | +------------+ ANX 7625 +---/-----+ +----+ SSTRX2 |
> > | | | | | | +--------+
> > | | +----------+ | |
> > +---------------------+ | |
> > | | +----------+ 2 lane | | C1
> > | | | +----/----C----+ +--------+
> > | USB3 HC | 2 lane | | | | SSTRX1 |
> > | +-----/------+ USB3 HUB | +---------+ |
> > | (host controller) | | | 2 lane | |
> > | | | +---------/---------+ SSTRX2 |
> > +---------------------+ | | | |
> > +----------+ +--------+
> >
> > Some platforms use it6505, so that can be swapped in for anx7625
> > without any change to the rest of the hardware diagram.
> >
> > From the above, we can see that it is helpful to describe the
> > Type-C SS lines as 2 endpoints:
> > - 1 for SSTX1+SSRX1 (A2,A3 + B10,B11)
> > - 1 for SSTX2+SSRX2 (B2,B3 + A10, A11)
> >
> > A device tree for this would look as follows:
> >
> > // Type-C port driver
> > ec {
> > ...
> > cros_ec_typec {
> > ...
> > usb-c0 {
> > compatible = "usb-c-connector";
> > ports {
> > hs : port@0 {
> > ...
> > };
> > ss: port@1 {
> > reg = <1>;
> > c0_sstrx1: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&anx7625_out0>;
> > };
> > c0_sstrx2: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&usb3hub_out0>;
> > };
> > };
> > sbu : port@2 {
> > ...
> > };
> > };
> > };
> > usb-c1 {
> > compatible = "usb-c-connector";
> > ports {
> > hs : port@0 {
> > ...
> > };
> > ss: port@1 {
> > reg = <1>;
> > c1_sstrx1: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&anx7625_out1>;
> > };
> > c1_sstrx2: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&usb3hub_out1>;
> > };
> > };
> > sbu : port@2 {
> > ...
> > };
> > };
> > };
> > };
> > };
> >
> > // DRM bridge / Type-C mode switch
> > anx_bridge: anx7625@58 {
> > compatible = "analogix,anx7625";
> > reg = <0x58>;
> > ...
> > // Input from DP controller
> > port@0 {
> > reg = <0>;
> > ...
> > };
> >
> > // Output to Type-C connector / DP panel
> > port@1 {
> > reg = <1>;
> >
> > anx7625_out0: endpoint@0 {
> > reg = <0>;
> > mode-switch;
> > remote-endpoint = <&c0_sstrx1>;
> > };
> > anx7625_out1: endpoint@1 {
> > reg = <1>;
> > mode-switch;
> > remote-endpoint = <&c1_sstrx1>;
> > };
> > };
> > };
> >
> > // USB3 hub
> > usb3hub: foo_hub {
> > ...
> > ports@0 {
> > // End point connected to USB3 host controller on SOC.
> > };
> > port@1 {
> > reg = <1>;
> >
> > foo_hub_out0: endpoint@0 {
> > reg = <0>;
> > mode-switch; ---> See c.) later
> > remote-endpoint = <&c0_sstrx2>;
> > };
> > foo_hub_out1: endpoint@1 {
> > reg = <1>;
> > mode-switch;
> > remote-endpoint = <&c1_sstrx2>;
> > };
> > };
> > };
> >
> > Notes:
> > - On the Chrome OS platform, the USB3 Hub is controlled by
> > the EC, so we don't really need to describe that connection,
> > but I've added a minimal one here just to show how the graph
> > connection would work if the HUB was controlled by the SoC.
> > - The above assumes that other hardware is controlling orientation.
> > We can add "orientation-switch" drivers along the graph path
> > if there is other hardware which controls orientation.
> >
> > Example 2: 1 USB-C connector connected to 1 drm-bridge/ mode-switch
> >
> > I've tried to use Bjorn's example [1], but I might have made
> > some mistakes since I don't have access to the schematic.
> >
> >
> > SoC
> > +------------------------------------------+
> > | |
> > | +---------------+ |
> > | | | |
> > | | DP ctrllr | +---------+ | C0
> > | | +-------+ | | 2 lane +----------+
> > | +---------------+ | QMP +-----+-----/--------+ SSTRX1 |
> > | | PHY | | | |
> > | +-------------+ 2 lane | | | 2 lane | |
> > | | +----/----+ +-----+-----/--------+ SSTRX2 |
> > | | dwc3 | +---------+ | | |
> > | | | | | |
> > | | | +---------+ | | |
> > | | +---------+ HS PHY | | HS lanes | |
> > | +-------------+ | +-----+----/---------+ D +/- |
> > | | | | +----------+
> > | +---------+ |
> > | |
> > +------------------------------------------+
> >
> > The DT would look something like this (borrowing from Stephen's example [2]):
> >
> > qmp {
> > mode-switch; ----> See b.) later.
> > orientation-switch;
> > ports {
> > qmp_usb_in: port@0 {
> > reg = <0>;
> > remote-endpoint = <&usb3_phy_out>;
> > };
> > qmp_dp_in: port@1 {
> > reg = <1>;
> > remote-endpoint = <&dp_phy_out>;
> > };
> > port@2 {
> > reg = <2>;
> > qmp_usb_dp_out0: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&c0_sstrx1>;
> > };
> > qmp_usb_dp_out1: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&c0_sstrx2>;
> > };
> > };
> > };
> >
> > dp-phy {
> > ports {
> > dp_phy_out: port {
> > remote-endpoint = <&qmp_dp_in>;
> > };
> > };
> > };
> >
> > dwc3: usb-phy {
> > ports {
> > usb3_phy_out: port@0 {
> > reg = <0>;
> > remote-endpoint = <&qmp_usb_in>;
> > };
> > };
> > };
> >
> > glink {
> > c0: usb-c-connector {
> > compatible = "usb-c-connector";
> > ports {
> > hs: port@0 {
> > reg = <0>;
> > endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&hs_phy_out>;
> > };
> > };
> >
> > ss: port@1 {
> > reg = <1>;
> > c0_sstrx1: endpoint@0 {
> > reg = <0>;
> > remote-endpoint = <&qmp_usb_dp_out0>;
> > };
> > c0_sstrx2: endpoint@1 {
> > reg = <1>;
> > remote-endpoint = <&qmp_usb_dp_out1>;
> > };
> > };
> > };
> > };
> > };
> >
> > Notes:
> > a. This proposal doesn't deal with the DRM bridge HPD forwarding; I
> > believe that is covered by Stephen's example/proposal in [2], and
> > can be addressed separately. That said, this binding is compatible
> > with the proposal in [2], that is, make the "mode-switch" driver a
> > drm-bridge and forward the HPD info to the upstream DRM-bridge (DP controller).
> > The driver implementing "mode-switch" will be able to do that, since
> > it gets DP status/attention VDOS with HPD info from the Type-C port driver.
> > b. If both SSTRX pairs from a connector are routed to the same
> > hardware block (example 2) then the device would keep "mode-switch"
> > as a top level property (and the fwnode associated with "mode-switch"
> > is the drm-bridge device).
> > c. If SSTRX pairs from 2 connectors are routed to the same
> > hardware block (example 1), then each end-point which is connected to
> > the USB-C connector will have a "mode-switch" property in its end-point.
> > There will be 2 mode switches registered here, and the fwnode for each
> > "mode-switch" is the end-point node.
> >
> > b.) and c.) can be handled by Type C mux registration and matching
> > code. We already have 3 mux devs for each mux [3].
> >
> > For the single mode-switch case, mux_dev[1] will just refer to the top-level
> > mode-switch registered by the DRM bridge / switch driver (example 1).
> > For the 2 mode-switch case, typec_mux_dev[1] will have 2 child
> > typec_mux_dev's, each of which represents the mode-switches
> > registered by the DRM bridge / switch driver. Introducing this
> > indirection means the port driver / alternate mode driver don't
> > need to care about how the connectors are routed; the framework
> > will just call the mux_set() function on the mux_dev() or its
> > children if it has any.
> >
> > The benefit of this approach is existing bindings (which just
> > assume 1 endpoint from usb-c-connector/port@1) should continue to
> > work without any changes.
> >
> > Why don't we use data lanes for the usb-c-connector
> > endpoints? I guess we could, but I am not a fan of adding the
> > extra data-lane parsing logic to the Type-C framework (I
> > don't think drivers need that level of detail from the connector
> > binding). And even then, we will still need an extra end-point
> > if the lanes of the USB-C connector are routed to different hardware blocks.
> >
> > The Type-C connector spec doesn't specify any alternate modes
> > with < 1 SSTRX pair, so the most we can ever have (short of a
> > major change to the spec) is 2 SSTRX end points for a
> > connector each being routed to different hardware blocks.
> > Codifying these as endpoint@0 and endpoint@1 in the usb-c-connector
> > binding seems to line up nicely with this detail of the spec.
> >
> > Thanks,
> >
> > -Prashant
> >
> > [1] https://lore.kernel.org/linux-usb/Yv1y9Wjp16CstJvK@baldur/
> > [2] https://lore.kernel.org/linux-usb/CAE-0n52-QVeUVCB1qZzPbYyrb1drrbJf6H2DEEW9bOE6mh7egw@xxxxxxxxxxxxxx/
> > [3] https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/usb/typec/mux.c#L259

Next message: Ira Weiny: "Re: [PATCH][CFT] [coredump] don't use __kernel_write() on kmap_local_page()"
Previous message: Eric Biggers: "[GIT PULL] STATX_DIOALIGN for 6.1"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]