Re: Adding tps65986 to your tps6598x driver

Re: Adding tps65986 to your tps6598x driver

[Heikki Krogerus]

+linux-usb ml

On Wed, Apr 22, 2020 at 12:17:14PM +0100, Bryan O'Donoghue wrote:
> On 14/04/2020 16:15, Heikki Krogerus wrote:
> > Hi Bryan,
> > 
> > On Tue, Apr 14, 2020 at 03:42:24PM +0100, Bryan O'Donoghue wrote:
> > > Hi Heikki.
> > > 
> > > I'm Bryan working with Linaro on a Qualcomm project.
> > > 
> > > We have a tps65986, which needs some upstreaming work.
> > > 
> > > http://www.ti.com/lit/gpn/tps65982
> > > http://www.ti.com/lit/gpn/tps65986
> > > 
> > > As you can see the two parts are very similar except the 65986 does data
> > > role switching but unlike the 65982 doesn't support 20v 5a power.
> > > 
> > > I was going to add
> > > 
> > > - OF probe support
> > > - USB role switching support
> > 
> > By USB role switching you mean the USB role switch class, right?
> > 
> > > to your existing driver. Do you know of anybody who has done any work for
> > > the tps65986 already ? Also does the above extension seem right/sensible to
> > > you ?
> > 
> > There isn't anybody adding support for tps65986 to the driver that I
> > know of, so please go for it. Both tasks make sense to me.
> > 
> > thanks,
> > 
> 
> What about tcpm ?

What about it?

> As I understand it for your platform, you just want the power controller
> stuff in the chip, without the data role.

Role swapping is actually the only thing that we need the driver for
on our platforms.

> In my case - we have a point-of-sale terminal, which basically looks like a
> phone - we want to be able to do data role switching.
> 
> What's your feeling. Add role switch into the driver you have here, or try
> to get that driver working with tcpm ?

So what you are proposing here is that you want to use tps65986 as
just a port controller (so PHY), right? I don't think that's possible.

TCPM (port manager) is the software that implements the USB Type-C and
PD state machines. The USB PD controllers are running their own state
machines, and the thing is that you can't turn off that part of them.
So basically the USB PD controllers are supplying the TCPM
functionality internally.

> Something else ? It's important we get the changes upstream, so I'd
> appreciate any thoughts you have on the right way to go about this.

So what exactly is the problem here?

Which USB controller are you using? Is it dual-role capable, or do you
have separate xHCI controller and separate USB device controller plus
a mux between them?

Is it clear to you that the Type-C driver, so tps6598x.c in this case,
is the consumer of the USB role switch? If you have a dual-role
capable USB controller, then that will be the supplier of the switch.
If you have separate USB host and devices IPs on your board, then the
mux between them is the switch supplier.

I'm attaching a diff that has all the changes that you should need to
do to the tps6598x.c in order to get the role swapping working on your
board.

The next step is to figure out how to describe the connection between
your USB role switch (which I'm guessing is the USB controller on your
board) and the USB PD controller. You are using devicetree, right?

> Could you recommend an Intel platform I could pick up to validate my changes
> don't break existing code ?

All -S variants of our SOCs can be used for testing the tps6598x.c,
for example Coffee Lake -S, but unfortunately I don't know which
products on the market actually use those SOCs. I do my development
with the reference boards.


thanks,

-- 
heikki

Re: Adding tps65986 to your tps6598x driver

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

On Wed, Apr 22, 2020 at 04:25:36PM +0300, Heikki Krogerus wrote:
> I'm attaching a diff that has all the changes that you should need to
> do to the tps6598x.c in order to get the role swapping working on your
> board.

Trying again...

-- 
heikki

[-- Attachment #2: role_switch_for_tps6598x.diff --]
[-- Type: text/plain, Size: 2580 bytes --]

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 0698addd1185..5f51397f0007 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/interrupt.h>
+#include <linux/usb/role.h>
 #include <linux/usb/typec.h>
 
 /* Register offsets */
@@ -94,6 +95,8 @@ struct tps6598x {
 	struct typec_port *port;
 	struct typec_partner *partner;
 	struct usb_pd_identity partner_identity;
+
+	struct usb_role_switch *role_sw;
 };
 
 /*
@@ -175,6 +178,18 @@ tps6598x_write_4cc(struct tps6598x *tps, u8 reg, const char *val)
 	return tps6598x_block_write(tps, reg, val, 4);
 }
 
+static int tps6598x_set_data_role(struct tps6598x *tps,
+				  enum typec_data_role role)
+{
+	enum usb_role usb_role;
+
+	usb_role = role == TYPEC_HOST ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+
+	typec_set_data_role(tps->port, role);
+
+	return usb_role_switch_set_role(tps->role_sw, usb_role);
+}
+
 static int tps6598x_read_partner_identity(struct tps6598x *tps)
 {
 	struct tps6598x_rx_identity_reg id;
@@ -220,7 +235,7 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
 	typec_set_pwr_opmode(tps->port, mode);
 	typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
 	typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
-	typec_set_data_role(tps->port, TPS_STATUS_DATAROLE(status));
+	tps6598x_set_data_role(tps, TPS_STATUS_DATAROLE(status));
 
 	tps->partner = typec_register_partner(tps->port, &desc);
 	if (IS_ERR(tps->partner))
@@ -241,6 +256,7 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 	typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
 	typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
 	typec_set_data_role(tps->port, TPS_STATUS_DATAROLE(status));
+	usb_role_switch_set_role(tps->role_sw, USB_ROLE_NONE);
 }
 
 static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
@@ -328,7 +344,7 @@ static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
 		goto out_unlock;
 	}
 
-	typec_set_data_role(tps->port, role);
+	ret = tps6598x_set_data_role(tps, role);
 
 out_unlock:
 	mutex_unlock(&tps->lock);
@@ -472,6 +488,10 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret < 0 || !vid)
 		return -ENODEV;
 
+	tps->role_sw = usb_role_switch_get(&client->dev);
+	if (IS_ERR(tps->role_sw))
+		return PTR_ERR(tps->role_sw);
+
 	/*
 	 * Checking can the adapter handle SMBus protocol. If it can not, the
 	 * driver needs to take care of block reads separately.

Re: Adding tps65986 to your tps6598x driver

[Bryan O'Donoghue]

On 22/04/2020 14:25, Heikki Krogerus wrote:
> +linux-usb ml
> 
> On Wed, Apr 22, 2020 at 12:17:14PM +0100, Bryan O'Donoghue wrote:
>> On 14/04/2020 16:15, Heikki Krogerus wrote: > So what you are proposing here is that you want to use tps65986 as
> just a port controller (so PHY), right? I don't think that's possible.

Both. Sorry I wasn't clear :)

> TCPM (port manager) is the software that implements the USB Type-C and
> PD state machines. The USB PD controllers are running their own state
> machines, and the thing is that you can't turn off that part of them.
> So basically the USB PD controllers are supplying the TCPM
> functionality internally.

OK. So forget about the above then. That answers the question why you 
didn't implement tps6598x as a tcpm bound device.

What's your feeling on the following.

In DT if we find a child connector then we can determine the state we 
are supposed to be in ?

That way it _shouldn't_ change the logic you already depend on the ACPI 
systems.

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;
};

we need to control :

http://www.ti.com/lit/an/slva843a/slva843a.pdf

- InitiateSwapToDPF
- InitiateSwapToUFP
- process-swap-to-dfp;
- process-swap-to-ufp;
- InitiateVconnSwap;
- ProcessVconnSwap;

The documentation makes clear some of these settings are mutually exclusive.

You can capture the logic of that with the connector

- power-role = "dual";
- data-role = "dual";

from the connector declaration. Absent the connector declaration the 
ACPI launched code should still work as-is.

Alternatively it could be something specific to the chip - as opposed to 
the connector.

My thought is you either have these two at the type-c controller level 
or inside a child connector node.

Either way tps6598x would consume them.

> 
>> Something else ? It's important we get the changes upstream, so I'd
>> appreciate any thoughts you have on the right way to go about this.
> 
> So what exactly is the problem here?
> 
> Which USB controller are you using? Is it dual-role capable, or do you
> have separate xHCI controller and separate USB device controller plus
> a mux between them?

Err, checks notes.
Its a ChipIdea HDRC. That part just works.

As I suggested above,

tps6598x: tps6598x@38 {
     compatible = "ti,tps6598x";
     reg = <0x38>;

     interrupt-parent = <&gpio>;
     interrupts = <107 IRQ_TYPE_LEVEL_LOW>;
     interrupt-names = "irq";

     pinctrl-names = "default";
     pinctrl-0 = <&typec_pins>;

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

     /* Example A */
     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>;
     };

     /* Example B */
     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>;
};

I think connector is probably the right way to go.

---
bod

Re: Adding tps65986 to your tps6598x driver

[Heikki Krogerus]

On Wed, Apr 22, 2020 at 03:50:08PM +0100, Bryan O'Donoghue wrote:
> What's your feeling on the following.
> 
> In DT if we find a child connector then we can determine the state we are
> supposed to be in ?
> 
> That way it _shouldn't_ change the logic you already depend on the ACPI
> systems.
> 
> 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;
> };
> 
> we need to control :
> 
> http://www.ti.com/lit/an/slva843a/slva843a.pdf
> 
> - InitiateSwapToDPF
> - InitiateSwapToUFP
> - process-swap-to-dfp;
> - process-swap-to-ufp;
> - InitiateVconnSwap;
> - ProcessVconnSwap;
> 
> The documentation makes clear some of these settings are mutually exclusive.
> 
> You can capture the logic of that with the connector
> 
> - power-role = "dual";
> - data-role = "dual";
> 
> from the connector declaration. Absent the connector declaration the ACPI
> launched code should still work as-is.
> 
> Alternatively it could be something specific to the chip - as opposed to the
> connector.
> 
> My thought is you either have these two at the type-c controller level or
> inside a child connector node.
> 
> Either way tps6598x would consume them.

You should always have a child node for every connector. Please Note
that we usually have one for each connector in ACPI as well.

For now on every platform the application code (the PD controler FW)
has been platform specific, which means we have not needed to
configure things like the System Configuration register, because we've
known that the application code has already done that for us.

In your case I'm assuming you do not have a platform specific PD
controler FW, so you are going to need to use device properties, which
is the correct thing to do.

So we can always check those properties in tps6598x.c. If we have
them, we configure the System Configuration, and what have you,
according to those. If we don't have them, then we know the PD
controller FW is platform specific.

> > > Something else ? It's important we get the changes upstream, so I'd
> > > appreciate any thoughts you have on the right way to go about this.
> > 
> > So what exactly is the problem here?
> > 
> > Which USB controller are you using? Is it dual-role capable, or do you
> > have separate xHCI controller and separate USB device controller plus
> > a mux between them?
> 
> Err, checks notes.
> Its a ChipIdea HDRC. That part just works.
> 
> As I suggested above,
> 
> tps6598x: tps6598x@38 {
>     compatible = "ti,tps6598x";
>     reg = <0x38>;
> 
>     interrupt-parent = <&gpio>;
>     interrupts = <107 IRQ_TYPE_LEVEL_LOW>;
>     interrupt-names = "irq";
> 
>     pinctrl-names = "default";
>     pinctrl-0 = <&typec_pins>;
> 
>     port {
>         typec1_dr_sw: endpoint {
>             remote-endpoint = <&usb1_drd_sw>;
>         };
>     };

That looks a bit odd to me. I think you want to place that under the
connector node too, however, OF graph is a bit problematic with
Type-C. The problem is that there is no way to identify the OF graph
ports/endpoints. It means that there does not seem to be any way to
know which port/endpoint/remote-endpoint is for the DisplayPort, the
mux, USB port, TBT 3 port, retimer #1, retimer #2, etc.

There is a proposal to define device property that simply contains
reference to the correct node for every type of connection. For USB
role switch the property is named "usb-role-switch":

        connector {
                ...
                usb-role-switch = <the remote port parent of &usb1_drd_sw>;
                ...
        };

>     /* Example A */
>     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>;
>     };
> 
>     /* Example B */
>     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>;
> };
> 
> I think connector is probably the right way to go.
> 
> ---
> bod

thanks,

-- 
heikki

Re: Adding tps65986 to your tps6598x driver

[Bryan O'Donoghue]

On 29/04/2020 15:03, Heikki Krogerus wrote:
> On Wed, Apr 22, 2020 at 03:50:08PM +0100, Bryan O'Donoghue wrote:
>> What's your feeling on the following.
>>
>> In DT if we find a child connector then we can determine the state we are
>> supposed to be in ?
>>
>> That way it _shouldn't_ change the logic you already depend on the ACPI
>> systems.
>>
>> 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;
>> };
>>
>> we need to control :
>>
>> http://www.ti.com/lit/an/slva843a/slva843a.pdf
>>
>> - InitiateSwapToDPF
>> - InitiateSwapToUFP
>> - process-swap-to-dfp;
>> - process-swap-to-ufp;
>> - InitiateVconnSwap;
>> - ProcessVconnSwap;
>>
>> The documentation makes clear some of these settings are mutually exclusive.
>>
>> You can capture the logic of that with the connector
>>
>> - power-role = "dual";
>> - data-role = "dual";
>>
>> from the connector declaration. Absent the connector declaration the ACPI
>> launched code should still work as-is.
>>
>> Alternatively it could be something specific to the chip - as opposed to the
>> connector.
>>
>> My thought is you either have these two at the type-c controller level or
>> inside a child connector node.
>>
>> Either way tps6598x would consume them.
> 
> You should always have a child node for every connector. Please Note
> that we usually have one for each connector in ACPI as well.
> 
> For now on every platform the application code (the PD controler FW)
> has been platform specific, which means we have not needed to
> configure things like the System Configuration register, because we've
> known that the application code has already done that for us.
> 
> In your case I'm assuming you do not have a platform specific PD
> controler FW, so you are going to need to use device properties, which
> is the correct thing to do.

On our platform I found the settings "just worked" asking around it 
turns out our FW has been configured with the TI configuration tool, so 
at this moment in time I don't think the stuff I'm working on has a 
specific need to configure these options.


> So we can always check those properties in tps6598x.c. If we have
> them, we configure the System Configuration, and what have you,
> according to those. If we don't have them, then we know the PD
> controller FW is platform specific.
> 
>>>> Something else ? It's important we get the changes upstream, so I'd
>>>> appreciate any thoughts you have on the right way to go about this.
>>>
>>> So what exactly is the problem here?
>>>
>>> Which USB controller are you using? Is it dual-role capable, or do you
>>> have separate xHCI controller and separate USB device controller plus
>>> a mux between them?
>>
>> Err, checks notes.
>> Its a ChipIdea HDRC. That part just works.
>>
>> As I suggested above,
>>
>> tps6598x: tps6598x@38 {
>>      compatible = "ti,tps6598x";
>>      reg = <0x38>;
>>
>>      interrupt-parent = <&gpio>;
>>      interrupts = <107 IRQ_TYPE_LEVEL_LOW>;
>>      interrupt-names = "irq";
>>
>>      pinctrl-names = "default";
>>      pinctrl-0 = <&typec_pins>;
>>
>>      port {
>>          typec1_dr_sw: endpoint {
>>              remote-endpoint = <&usb1_drd_sw>;
>>          };
>>      };
> 
> That looks a bit odd to me. I think you want to place that under the
> connector node too, however, OF graph is a bit problematic with
> Type-C. 


I discovered that as I came to implement, also looking at the other 
typec controllers like the fusb302 and hd3ss3220 it became obvious a 
connector {} should be included.

tps6598x: tps6598x@38 {
         compatible = "ti,tps6598x";
         reg = <0x38>;

         interrupt-parent = <&msmgpio>;
         interrupts = <107 IRQ_TYPE_LEVEL_LOW>;
         interrupt-names = "irq";

         reset-gpios = <&msmgpio 116 GPIO_ACTIVE_LOW>;

         pinctrl-names = "default";
         pinctrl-0 = <&typec_pins>;

         typec_con: connector {
                 compatible = "usb-c-connector";
                 label = "USB-C";
                 port {
                         typec_ep: endpoint {
                                 remote-endpoint = <&otg_ep>;
                         };
                 };
         };
};

&otg {
         status = "okay";
         usb-role-switch;
         ulpi {
                 usb_hs_phy: phy {
                         v1p8-supply = <&pm8916_l7>;
                         v3p3-supply = <&pm8916_l13>;
                 };
         };
         port {
                 otg_ep: endpoint {
                         remote-endpoint = <&typec_ep>;
                 };
         };
};

with no more change than adding OF binding and the role-switch stuff we 
discussed.

Not quite ready to send out yet.

> The problem is that there is no way to identify the OF graph
> ports/endpoints. It means that there does not seem to be any way to
> know which port/endpoint/remote-endpoint is for the DisplayPort, the
> mux, USB port, TBT 3 port, retimer #1, retimer #2, etc.


> There is a proposal to define device property that simply contains
> reference to the correct node for every type of connection. For USB
> role switch the property is named "usb-role-switch":
> 
>          connector {
>                  ...
>                  usb-role-switch = <the remote port parent of &usb1_drd_sw>;
>                  ...
>          };

I'll take a look.

Thanks for the feedback.

---
bod