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
Right now in TCPM when we want to send a role-switch message the
remote-endpoint must appear inside of the TCPM bound node, not in the
connector associated with TCPM.
&typec {
status = "okay";
port {
typec_role_switch: endpoint {
remote-endpoint = <&dwc3_role_switch>;
};
};
connector {
compatible = "usb-c-connector";
power-role = "source";
data-role = "dual";
self-powered;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
typec_mux: endpoint {
remote-endpoint = <&phy_typec_mux>;
};
};
};
};
};
This change makes it possible to declare the remote-endpoint inside of the
connector of the TCPM e.g.
&typec {
status = "okay";
connector {
compatible = "usb-c-connector";
power-role = "source";
data-role = "dual";
self-powered;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
typec_role_switch: endpoint {
remote-endpoint = <&dwc3_role_switch>;
};
};
port@1 {
reg = <1>;
typec_mux: endpoint {
remote-endpoint = <&phy_typec_mux>;
};
};
};
};
};
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/usb/typec/tcpm/tcpm.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 1ee774c263f08..a62fecf3bb44c 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -6515,6 +6515,7 @@ static enum hrtimer_restart send_discover_timer_handler(struct hrtimer *timer)
struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
{
struct tcpm_port *port;
+ struct fwnode_handle *fwnode;
int err;
if (!dev || !tcpc ||
@@ -6582,6 +6583,14 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
goto out_destroy_wq;
}
+ if (!port->role_sw) {
+ fwnode = device_get_named_child_node(port->dev, "connector");
+ if (fwnode) {
+ port->role_sw = fwnode_usb_role_switch_get(fwnode);
+ fwnode_handle_put(fwnode);
+ }
+ }
+
err = devm_tcpm_psy_register(port);
if (err)
goto out_role_sw_put;
--
2.39.2
On Sat, Mar 25, 2023 at 01:15:52AM +0000, Bryan O'Donoghue wrote:
> Right now in TCPM when we want to send a role-switch message the
> remote-endpoint must appear inside of the TCPM bound node, not in the
> connector associated with TCPM.
>
> &typec {
> status = "okay";
>
> port {
> typec_role_switch: endpoint {
> remote-endpoint = <&dwc3_role_switch>;
> };
> };
>
> connector {
> compatible = "usb-c-connector";
>
> power-role = "source";
> data-role = "dual";
> self-powered;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> typec_mux: endpoint {
> remote-endpoint = <&phy_typec_mux>;
> };
> };
> };
> };
> };
>
> This change makes it possible to declare the remote-endpoint inside of the
> connector of the TCPM e.g.
>
> &typec {
> status = "okay";
>
> connector {
> compatible = "usb-c-connector";
>
> power-role = "source";
> data-role = "dual";
> self-powered;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> typec_role_switch: endpoint {
> remote-endpoint = <&dwc3_role_switch>;
> };
> };
> port@1 {
> reg = <1>;
> typec_mux: endpoint {
> remote-endpoint = <&phy_typec_mux>;
> };
> };
> };
> };
> };
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/usb/typec/tcpm/tcpm.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 1ee774c263f08..a62fecf3bb44c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -6515,6 +6515,7 @@ static enum hrtimer_restart send_discover_timer_handler(struct hrtimer *timer)
> struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> {
> struct tcpm_port *port;
> + struct fwnode_handle *fwnode;
> int err;
>
> if (!dev || !tcpc ||
> @@ -6582,6 +6583,14 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> goto out_destroy_wq;
> }
>
> + if (!port->role_sw) {
> + fwnode = device_get_named_child_node(port->dev, "connector");
You already have that assigned to tcpc->fwnode, no?
At least drivers/usb/typec/tcpm/tcpci.c assignes it there, and I think
so do the other port controller drivers.
> + if (fwnode) {
> + port->role_sw = fwnode_usb_role_switch_get(fwnode);
> + fwnode_handle_put(fwnode);
> + }
> + }
If that is the case, then I think this is enough:
if (!port->role_sw)
port->role_sw = fwnode_usb_role_switch_get(tcpc->fwnode);
Or maybe I missed something. If I did then sorry for the noise :-)
thanks,
--
heikki