lkml.org 
[lkml]   [2020]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props
    On Fri, Jun 12, 2020 at 6:46 AM Heikki Krogerus
    <heikki.krogerus@linux.intel.com> wrote:
    >
    > On Wed, Jun 10, 2020 at 10:53:45AM -0600, Rob Herring wrote:
    > > On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
    > > <heikki.krogerus@linux.intel.com> wrote:
    > > >
    > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
    > > > > Hi Rob,
    > > > >
    > > > > Thanks again for the comments and feedback. Kindly see responses inline:
    > > > >
    > > > > (Trimming unrelated text from thread):
    > > > >
    > > > > On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
    > > > > > On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
    > > > > > >
    > > > > > > Nodes truncated and unrelated fields omitted in the interest of brevity:
    > > > > > >
    > > > > > > // Chrome OS EC Type C Port Manager.
    > > > > > > typec {
    > > > > > > compatible = "google,cros-ec-typec";
    > > > > > > #address-cells = <1>;
    > > > > > > #size-cells = <0>;
    > > > > > >
    > > > > > > connector@0 {
    > > > > > > compatible = "usb-c-connector";
    > > > > > > reg = <0>;
    > > > > > > power-role = "dual";
    > > > > > > data-role = "dual";
    > > > > > > try-power-role = "source";
    > > > > > > mode-switch = <&foo_mux>;
    > > > > > > // Other switches can point to the same mux.
    > > > > > > ....
    > > > > >
    > > > > > The connector is supposed to have 'ports' for USB2, USB3, and Aux
    > > > > > unless the parent is the USB controller.
    > > > > Understood; so, coupled with Heikki's explanation (see below for where
    > > > > I've pasted it), would it be something like so? (adding inline to the connector
    > > > > node definition):
    > > > >
    > > > > ports {
    > > > > #address-cells = <1>;
    > > > > #size-cells = <0>;
    > > > >
    > > > > port@0 {
    > > > > reg = <0>;
    > > > > usb_con_hs: endpoint {
    > > > > remote-endpoint = <&foo_usb_hs_controller>;
    > > > > };
    > > > > };
    > > > >
    > > > > port@1 {
    > > > > reg = <1>;
    > > > > usb_con0_ss: endpoint@0 {
    > > > > remote-endpoint = <&mode_mux_in>;
    > > > > };
    > > > > };
    > > > >
    > > > > port@2 {
    > > > > reg = <2>;
    > > > > usb_con_sbu: endpoint {
    > > > > remote-endpoint = <&foo_dp_aux>;
    > > > > };
    > > > > };
    > > > > };
    > > >
    > > > The pins that can be reassigned can in practice go anywhere. We can't
    > > > group them in any way. What do we do for example when the two sideband
    > > > pins go to different locations?
    > >
    > > The sideband pins from the connector go to multiple places or the
    > > sideband signal from a controller go to multiple connectors? Either
    > > way, that's solved with multiple endpoints. In the former case, port@2
    > > would have multiple endpoints with all the possible connections. The
    > > general model of the graph is each port is a separate data channel and
    > > multiple endpoints are either a mux or fanout depending on the data
    > > direction.
    >
    > No, that's not what I'm trying to ask here. Bad example, sorry. I'm
    > trying to understand why is it necessary to slit the connector into
    > three separate interfaces?

    Because it is easily 3 separate h/w components (nodes) that have a
    link to the connector.

    > There does not seem to be anything in the
    > kernel that would benefit from that. Why isn't the connector described
    > as a single interface in devicetree?

    The connector was designed pretty much before there was any TypeC
    support in the kernel. Bindings shouldn't be designed around the
    *current* needs of a particular OS.

    The simplest case for the connector would be:

    usb@1234 {
    compatible = "vendor,some-usb-2and3-with-typec-controller";
    ...
    connector {
    compatible = "usb-type-c-connector";
    /* No ports! */
    };

    In this case, the h/w for
    "vendor,some-usb-2and3-with-typec-controller" can handle everything
    for the connector. Doesn't need anything for alt modes because either
    it is not supported or there's only one possible source.

    > My concern with the three separate interfaces is that they may force
    > us to know in kernel which of the three interfaces are association
    > with a mode, and actually not just the mode, but the possible the pin
    > configurations of the mode. That is something that we may end up
    > having to hard code into the drivers, even though it does not provide
    > any useful information to us, and that would not be OK.

    Either you hard-code things in DT with "generic", low-level binding or
    you hard-code things in a driver. Or maybe in your case, things are
    hard-coded in the EC? But most platforms don't have that.

    > Right now they also allow making assumptions regarding the alternate
    > modes since there are no "bindings" for those, for example, if these
    > OF graph ports have an endpoint to the DP, it means DP alt mode is
    > supported. But that is of course not true. DisplayPort is used also
    > with other alternate modes. We must never make any assumptions based
    > on those interfaces. So again, why do we have them?

    I'm pretty sure we have cases where the alt mode is HDMI. Maybe
    there's not yet been a case of multiple alt modes til now. So now the
    binding needs to be extended.

    > Either I'm missing something, or the devicetree description of the
    > Type-C connectors really is way too complex, way too "low level",
    > causing us potential problems without providing anything that we could
    > actually ever use in the operating system.

    Well, all bindings are a balancing act of being flexible enough vs.
    high-level enough to be stable. What I need is something that's going
    to work for everyone, not just CrOS. Adding a new property at time is
    death by 1000 cuts and usually a sign of someone only fixing their own
    immediate problem.

    Rob

    \
     
     \ /
      Last update: 2020-06-12 16:21    [W:4.849 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site