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;
From:   Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To:     linux@roeck-us.net, heikki.krogerus@linux.intel.com,
        andersson@kernel.org, robh@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Subject: [RFC PATCH 0/1] Add TCPM connector role-switch endpoint binding
Date:   Sat, 25 Mar 2023 01:15:51 +0000
Message-Id: <20230325011552.2241155-1-bryan.odonoghue@linaro.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Speaking to Rob Herring and Bjorn Andersson about aligning the Qualcomm
PMIC Type-C code with the Qualcomm PMIC GLINK Type-C code I ended up in a
conversation with Bjorn about where various nodes are declared.

If we look at how GLINK declares its bindings we see the usb_role endpoint
is at the same scope as the connector. This is I believe the format the Rob
and Bjorn agreed on.

Documentation/devicetree/bindings/soc/qcom/qcom,pmic-glink.yaml

connector@0 {
    compatible = "usb-c-connector";
    reg = <0>;
    power-role = "dual";
    data-role = "dual";

    ports {
        #address-cells = <1>;
        #size-cells = <0>;

        port@0 {
            reg = <0>;
            glink_role_switch: endpoint {
                remote-endpoint = <&usb_role>;
            };
        };

        port@1 {
            reg = <1>;
            endpoint {
                remote-endpoint = <&ss_phy_out>;
            };
        };

        port@2 {
            reg = <2>;
            endpoint {
                remote-endpoint = <&sbu_mux>;
            };
        };
    };
};

Implicit in the above declaration is a node that does the following:

usb_controller {
    usb-role-switch;
    port {
        usb_role: endpoint {
            remote-endpoint = <&glink_role_switch>;
        }
    };
};

In TCPM what we have a situation where we document something like the above
Documentation/devicetree/bindings/usb/typec-tcpci.txt

ptn5110@50 {
    compatible = "nxp,ptn5110";
    reg = <0x50>;
    interrupt-parent = <&gpio3>;
    interrupts = <3 IRQ_TYPE_LEVEL_LOW>;

    usb_con: connector {
        compatible = "usb-c-connector";
        label = "USB-C";
        data-role = "dual";
        power-role = "dual";
        try-power-role = "sink";
        source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
        sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
                               PDO_VAR(5000, 12000, 2000)>;
        op-sink-microwatt = <10000000>;

        ports {
            #address-cells = <1>;
            #size-cells = <0>;

            port@1 {
                reg = <1>;
                usb_con_ss: endpoint {
                    remote-endpoint = <&usb3_data_ss>;
                };
            };
        };
    };
};

However because of how tcpm.c looks up the remote-endpoint this example
actually _declares_ like this with the role-switch node having to be
declared outside of the connector.

arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi

ptn5110: tcpc@50 {
    compatible = "nxp,ptn5110";
    pinctrl-names = "default";
    pinctrl-0 = <&pinctrl_typec1>;
    reg = <0x50>;
    interrupt-parent = <&gpio2>;
    interrupts = <11 8>;
    status = "okay";

    port {
        typec1_dr_sw: endpoint {
            remote-endpoint = <&usb1_drd_sw>;
        };
    };

    typec1_con: connector {
        compatible = "usb-c-connector";
        label = "USB-C";
        power-role = "dual";
        data-role = "dual";
        try-power-role = "sink";
        source-pdos = <PDO_FIXED(5000, 3000, PDO_FIXED_USB_COMM)>;
        sink-pdos = <PDO_FIXED(5000, 3000, PDO_FIXED_USB_COMM)
                     PDO_VAR(5000, 20000, 3000)>;
        op-sink-microwatt = <15000000>;
        self-powered;
    };
};

I''m sending the next patch as an RFC. What that patch does is make it
possible for the TCPM code to find the glink.yaml or the typec-tcpci.txt
way of declaring the role-switch remote-endpoint inside of the connector
like we document.

It doesn't get rid of the exiting one-level higher declaration since that's
extra work and I'm not sure of the provenance of the existing code i.e. why
it currently depends on the one-level up approach. Also I'm wondering about
dtb backwards compatibility etc.

In any case the RFC patch that comes next allows TCPM dts to declare their
role-switch remote endpoints in the connector as per my understanding of
what Bjorn and Rob had discussed.

Perhaps the approach of having the remote-endpoint outside of the connector
has some very good reason but as I read the above typec-tcpci.txt and then
the imx8mm-evk.dtsi it seems to me as if we documented how we would like
for it work only to implement it how it currently works.

That is my experience too developing TCPM for the pm8150b PMIC - I have to
declare the role-switch endpoint outside of the connector.

Anyway I'd appreciate thoughts on this one, it seems to me the right place
for the data role switch remote endpoint to go is into the connector{};

Bryan O'Donoghue (1):
  usb: typec: tcpm: Support role-switch remote endpoint in connector

 drivers/usb/typec/tcpm/tcpm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.39.2