Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220622173605.1168416-1-pmalani@chromium.org>
 <20220622173605.1168416-2-pmalani@chromium.org> <CAE-0n51kcr3VGdR2Kf8j1JaBbLcCmWo9GYhhvkUQ4+jn2iEKLg@mail.gmail.com>
 <CACeCKac4eL9++QwbDBKrVTpUzhes=WczqZfh+cFiVgoO4py4MQ@mail.gmail.com> <CAE-0n51E1TLMRNWnqiV-jU_qg15BF4D6A+0G1y1SRTu1zNs2Dg@mail.gmail.com>
In-Reply-To: <CAE-0n51E1TLMRNWnqiV-jU_qg15BF4D6A+0G1y1SRTu1zNs2Dg@mail.gmail.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Thu, 23 Jun 2022 17:35:38 -0700
Message-ID: <CACeCKacGZFY-_yn1R33OVcsdG47oqNTGBA43L5hrH2zyhK=cRw@mail.gmail.com>
Subject: Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        bleung@chromium.org, heikki.krogerus@linux.intel.com,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        =?UTF-8?B?TsOtY29sYXMgRiAuIFIgLiBBIC4gUHJhZG8=?= 
        <nfraprado@collabora.com>, Allen Chen <allen.chen@ite.com.tw>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>, devicetree@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hsin-Yi Wang <hsinyi@chromium.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        =?UTF-8?B?Sm9zw6kgRXhww7NzaXRv?= <jose.exposito89@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Maxime Ripard <maxime@cerno.tech>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Pin-Yen Lin <treapking@chromium.org>,
        Robert Foss <robert.foss@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Sam Ravnborg <sam@ravnborg.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Xin Ji <xji@analogixsemi.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Prashant Malani (2022-06-23 12:08:21)
> > On Thu, Jun 23, 2022 at 11:30 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Prashant Malani (2022-06-22 10:34:30)
> > > > diff --git a/Documentation/devicetree/bindings/usb/typec-switch.yaml b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > new file mode 100644
> > > > index 000000000000..78b0190c8543
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch.yaml
> > > > @@ -0,0 +1,74 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> [...]
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +    description: OF graph binding modelling data lines to the Type-C switch.
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/properties/port
> > > > +        description: Link between the switch and a Type-C connector.
> > >
> > > Is there an update to the usb-c-connector binding to accept this port
> > > connection?
> >
> > Not at this time. I don't think we should enforce that either.
> > (Type-C data-lines could theoretically be routed through intermediate
> > hardware like retimers/repeaters)
>
> I'm mostly wondering if having such a connection to the usb-c-connector,
> or even through some retimer/repeater, would be sufficient to detect how
> many type-c ports are connected to the device. If the type-c pin
> assignments only support two or four lanes for DP then it seems like we
> should describe the two lanes or four lanes as one graph endpoint
> "output" and then have some 'data-lanes' property in case the DP lanes
> are flipped while being sent to the retimer or usb-c-connector. This
> would of course depend on the capability of the device, i.e. if it can
> remap DP lanes or only has 2 lanes of DP, etc.
>
> > > > +  - |
> > > > +    drm-bridge {
> > > > +        usb-switch {
> > > > +            compatible = "typec-switch";
> > >
> > > I still don't understand the subnode design here. usb-switch as a
> > > container node indicates to me that this is a bus, but in earlier rounds
> > > of this series it was stated this isn't a bus.
> >
> > I am not aware of this as a requirement. Can you please point me to the
> > documentation that states this needs to be the case?
>
> I'm not aware of any documentation for the dos and don'ts here. Are
> there any examples in the bindings directory that split up a device into
> subnodes that isn't in bindings/mfd?

usb-c-connector [3] and its users is an example.

>  I just know from experience that
> any time I try to make a child node of an existing node that I'm
> supposed to be describing a bus, unless I'm adding some sort of
> exception node like a graph binding or an opp table. Typically a node
> corresponds 1:1 with a device in the kernel. I'll defer to Rob for any
> citations.
>
> >
> > > Why doesn't it work to
> > > merge everything inside usb-switch directly into the drm-bridge node?
> >
> > I attempted to explain the rationale in the previous version [1], but
> > using a dedicated sub-node means the driver doesn't haven't to
> > inspect individual ports to determine which of them need switches
> > registered for them. If it sees a `typec-switch`, it registers a
> > mode-switch and/or orientation-switch. IMO it simplifies the hardware
> > device binding too.
>
> How is that any harder than hard-coding that detail into the driver
> about which port and endpoint is possibly connected to the
> usb-c-connector (or retimer)? All of that logic could be behind some API
> that registers a typec-switch based on a graph port number that's passed
> in, ala drm_of_find_panel_or_bridge()'s design.

If each driver has to do it (and the port specifics vary for each driver),
it becomes an avoidable overhead for each of them.
I prefer hard-coding such details if avoidable. I suppose both approaches
require modifications to the binding and the driver code.

>
> Coming from a DT writer's perspective, I just want to go through the
> list of output pins in the datasheet and match them up to the ports
> binding for this device. If it's a pure DP bridge, where USB hardware
> isn't an input or an output like the ITE chip, then I don't want to have
> to describe a port graph binding for the case when it's connected to a
> dp-connector (see dp-connector.yaml) in the top-level node and then have
> to make an entirely different subnode for the usb-c-connector case with
> a whole other set of graph ports.

This approach still allows for that, if the driver has any use for it
(AFAICT these drivers don't).
Iff that driver uses it, one can (optionally) route their output
(top-level) ports through the
"typec-switch" sub-node (and onwards as required).
If it's being used in a "pure-DP" configuration, the "typec-switch" just
goes away (the top level ports can be routed as desired by the driver).
(Again, I must reiterate that neither this driver or the anx driver
utilizes this)

>
> How would I even know which two differential pairs correspond to port0
> or port1 in this binding in the ITE case?

Why do we need to know that? It doesn't affect this or the other
driver or hardware's
functioning in a perceivable way.

> Ideally we make the graph
> binding more strict for devices by enforcing that their graph ports
> exist. Otherwise we're not fully describing the connections between
> devices and our dtb checkers are going to let things through where the
> driver most likely will fail because it can't figure out what to do,
> e.g. display DP on 4 lanes or play some DP lane rerouting games to act
> as a mux.

How is the current binding enforcing this? The typec-switch binding
as a first step ensures that the DT is connecting the hardware(anx,ite
etc) to something
that at least "claims" to be a Type-C switch.

>
> >
> > It also maps with the internal block diagram for these hardware
> > components (for ex. the anx7625 crosspoint switch is a separate
> > sub-block within anx7625).
>
> We don't make DT bindings for sub-components like this very often. It
> would make more sense to me to have a subnode if a typec switch was some
> sort of off the shelf hard macro that the hardware engineer placed down
> inside the IC that they delivered. Then we could have a completely
> generic driver that binds to the generic binding that knows how to drive
> the hardware, because it's an unchangeable hard macro with a well
> defined programming interface.
>
> >
> > [1] https://lore.kernel.org/linux-usb/CACeCKaeH6qTTdG_huC4yw0xxG8TYEOtfPW3tiVNwYs=P4QVPXg@mail.gmail.com/
>
> I looked at the fsa4480 driver and the device has a publicly available
> datasheet[2]. That device is designed for "audio accessory mode" but I
> guess it's being used to simply mux SBU lines? There isn't an upstream
> user of the binding so far, but it also doesn't look like a complete
> binding. I'd expect to see DN_L/R as a graph output connected to the
> usb-c-connector and probably have a usb2.0 input port and a 'sound-dai'
> property to represent the input audio path.
>
> Finally, simply connecting to the typec controller node isn't sufficient
> because a typec controller can be controlling many usb-c-connectors so I
> don't see how the graph binding would be able to figure out how many
> usb-c-connectors are connected to a mux like device, unless we took the
> approach of this patch.

It can follow the endpoint of the typec-switch port (the port parent
of the remote
end-point would be a 'usb-c-connector'). That is if the graph binding
(I'm assuming you mean the switch device here) wants to figure this
out in the first place.

> Is that why you're proposing this binding? To
> avoid describing a graph binding in the usb-c-connector and effectively
> "pushing" the port count up to the mux?

No, that is not the intention behind this series. The
'usb-c-connector' still needs the
graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)

>
> [2] https://www.onsemi.com/pdf/datasheet/fsa4480-d.pdf

[3] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/connector/usb-connector.yaml#L23