mbox series

[v3,0/2] tcpm: AMS and Collision Avoidance

Message ID 20190920032437.242187-1-kyletso@google.com (mailing list archive)
Headers show
Series tcpm: AMS and Collision Avoidance | expand

Message

Kyle Tso Sept. 20, 2019, 3:24 a.m. UTC 
*** BLURB HERE ***

Kyle Tso (2):
  usb: typec: tcpm: AMS and Collision Avoidance
  usb: typec: tcpm: AMS for PD2.0

 drivers/usb/typec/tcpm/tcpm.c | 523 ++++++++++++++++++++++++++++++----
 include/linux/usb/pd.h        |   1 +
 include/linux/usb/tcpm.h      |   4 +
 3 files changed, 468 insertions(+), 60 deletions(-)

Comments

Hans de Goede Sept. 20, 2019, 8:02 a.m. UTC | #1 
Hi Kyle,

On 20-09-2019 05:24, Kyle Tso wrote:
> *** BLURB HERE ***
> 
> Kyle Tso (2):
>    usb: typec: tcpm: AMS and Collision Avoidance
>    usb: typec: tcpm: AMS for PD2.0

May I ask how and on which hardware you have tested this?

And specifically if you have tested this in combination with pwr-role swapping?

Regards,

Hans
Kyle Tso Sept. 20, 2019, 8:19 a.m. UTC | #2 
Hi Hans,

I have tested these on an Android device (ARM64).
All the swap operations work fine (Power Role/Data Role/Vconn Swap).
(except for Fast Role Swap because it is still not supported in TCPM)

Regards,
Kyle Tso


On Fri, Sep 20, 2019 at 4:02 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kyle,
>
> On 20-09-2019 05:24, Kyle Tso wrote:
> > *** BLURB HERE ***
> >
> > Kyle Tso (2):
> >    usb: typec: tcpm: AMS and Collision Avoidance
> >    usb: typec: tcpm: AMS for PD2.0
>
> May I ask how and on which hardware you have tested this?
>
> And specifically if you have tested this in combination with pwr-role swapping?
>
> Regards,
>
> Hans
>
Hans de Goede Sept. 20, 2019, 8:25 a.m. UTC | #3 
Hi Kyle,

On 20-09-2019 10:19, Kyle Tso wrote:
> Hi Hans,
> 
> I have tested these on an Android device (ARM64).
> All the swap operations work fine (Power Role/Data Role/Vconn Swap).
> (except for Fast Role Swap because it is still not supported in TCPM)

May I ask which type-c controller are these devices using?

Regards,

Hans



> 
> Regards,
> Kyle Tso
> 
> 
> On Fri, Sep 20, 2019 at 4:02 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Kyle,
>>
>> On 20-09-2019 05:24, Kyle Tso wrote:
>>> *** BLURB HERE ***
>>>
>>> Kyle Tso (2):
>>>     usb: typec: tcpm: AMS and Collision Avoidance
>>>     usb: typec: tcpm: AMS for PD2.0
>>
>> May I ask how and on which hardware you have tested this?
>>
>> And specifically if you have tested this in combination with pwr-role swapping?
>>
>> Regards,
>>
>> Hans
>>
Kyle Tso Sept. 20, 2019, 8:54 a.m. UTC | #4 
On Fri, Sep 20, 2019 at 4:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kyle,
>
> On 20-09-2019 10:19, Kyle Tso wrote:
> > Hi Hans,
> >
> > I have tested these on an Android device (ARM64).
> > All the swap operations work fine (Power Role/Data Role/Vconn Swap).
> > (except for Fast Role Swap because it is still not supported in TCPM)
>
> May I ask which type-c controller are these devices using?
>
> Regards,
>
> Hans
>

It's a Type-C/PD controller embedded in Qualcomm PMIC PM8150.

Regards,
Kyle Tso
Hans de Goede Oct. 3, 2019, 9:47 a.m. UTC | #5 
Hi Kyle,

On 20-09-2019 05:24, Kyle Tso wrote:
> *** BLURB HERE ***
> 
> Kyle Tso (2):
>    usb: typec: tcpm: AMS and Collision Avoidance
>    usb: typec: tcpm: AMS for PD2.0

I've finally gotten a chance to test this on one of my own devices
which uses the tcpm framework for its Type-c port.

I am afraid that this series breaks DP altmode support,
specifically, the dp_altmode_configure() function no longer
seems to get called, leading to no pin-assignment being
selected by default and DP thus not working.

So sorry, but I have to NACK this series since it causes
regressions.

It might be easiest if you can get yourself some hardware
which supports DP altmode and uses the fusb302 Type-C
controller (which unlike your controller is actually
supported by the mainline kernel).

2 devices which have this are the original (version 1)
of the GPD win and the GPD pocket. Since the version
is not always clearly marked, make sure you get one which
has a X7-Z8750 CPU, those are the version 1 models, you
can still get these e.g. here:

https://www.geekbuying.com/item/GPD-Pocket-7-Inch-Tablet-PC-Intel-Atom-X7-Z8750-8GB-128GB-375711.html
https://www.geekbuying.com/item/GPD-Win-Intel-Z8750-Windows-10-4GB-64GB-Gamepad-Tablet-PC---Black-378018.html

These 2 devices still need 2 minor patches to hookup the DP
support, I have just finished these 2 patches up and I'm
submitting them upstream today, I will Cc you on these.

If you combine these with one of the many DP-charging pass-through
+ USB-3 out + HDMI out dongles, e.g.:
https://www.aliexpress.com/item/32953320909.html

And then after plugging in do:

cat /sys/class/typec/port0-partner/port0-partner.0/displayport/pin_assignment

This should print:

C [D]

But when I add your patches into the mix it prints just:

C D

And these debug pr_err calls never happen:

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 7845df030b72..d14f94078dd9 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -106,6 +106,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
  		break;
  	}

+	pr_err("dp_altmode_configure pin_assign %08x conf %08x\n", pin_assign, DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
  	/* Determining the initial pin assignment. */
  	if (!DP_CONF_GET_PIN_ASSIGN(dp->data.conf)) {
  		/* Is USB together with DP preferred */
@@ -115,6 +116,8 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
  		else if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK)
  			pin_assign &= DP_PIN_ASSIGN_DP_ONLY_MASK;

+		pr_err("dp_altmode_configure masked pin_assign %08x\n", pin_assign);
+
  		if (!pin_assign)
  			return -EINVAL;


Regards,

Hans
Kyle Tso Oct. 3, 2019, 10:04 a.m. UTC | #6 
Hi Hans

Could you append the TCPM log?

On Thu, Oct 3, 2019 at 5:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kyle,
>
> On 20-09-2019 05:24, Kyle Tso wrote:
> > *** BLURB HERE ***
> >
> > Kyle Tso (2):
> >    usb: typec: tcpm: AMS and Collision Avoidance
> >    usb: typec: tcpm: AMS for PD2.0
>
> I've finally gotten a chance to test this on one of my own devices
> which uses the tcpm framework for its Type-c port.
>
> I am afraid that this series breaks DP altmode support,
> specifically, the dp_altmode_configure() function no longer
> seems to get called, leading to no pin-assignment being
> selected by default and DP thus not working.
>
> So sorry, but I have to NACK this series since it causes
> regressions.
>
> It might be easiest if you can get yourself some hardware
> which supports DP altmode and uses the fusb302 Type-C
> controller (which unlike your controller is actually
> supported by the mainline kernel).
>
> 2 devices which have this are the original (version 1)
> of the GPD win and the GPD pocket. Since the version
> is not always clearly marked, make sure you get one which
> has a X7-Z8750 CPU, those are the version 1 models, you
> can still get these e.g. here:
>
> https://www.geekbuying.com/item/GPD-Pocket-7-Inch-Tablet-PC-Intel-Atom-X7-Z8750-8GB-128GB-375711.html
> https://www.geekbuying.com/item/GPD-Win-Intel-Z8750-Windows-10-4GB-64GB-Gamepad-Tablet-PC---Black-378018.html
>
> These 2 devices still need 2 minor patches to hookup the DP
> support, I have just finished these 2 patches up and I'm
> submitting them upstream today, I will Cc you on these.
>
> If you combine these with one of the many DP-charging pass-through
> + USB-3 out + HDMI out dongles, e.g.:
> https://www.aliexpress.com/item/32953320909.html
>
> And then after plugging in do:
>
> cat /sys/class/typec/port0-partner/port0-partner.0/displayport/pin_assignment
>
> This should print:
>
> C [D]
>
> But when I add your patches into the mix it prints just:
>
> C D
>
> And these debug pr_err calls never happen:
>
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 7845df030b72..d14f94078dd9 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -106,6 +106,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
>                 break;
>         }
>
> +       pr_err("dp_altmode_configure pin_assign %08x conf %08x\n", pin_assign, DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
>         /* Determining the initial pin assignment. */
>         if (!DP_CONF_GET_PIN_ASSIGN(dp->data.conf)) {
>                 /* Is USB together with DP preferred */
> @@ -115,6 +116,8 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
>                 else if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK)
>                         pin_assign &= DP_PIN_ASSIGN_DP_ONLY_MASK;
>
> +               pr_err("dp_altmode_configure masked pin_assign %08x\n", pin_assign);
> +
>                 if (!pin_assign)
>                         return -EINVAL;
>
>
> Regards,
>
> Hans
>
Hans de Goede Oct. 3, 2019, 10:38 a.m. UTC | #7 
Hi,

On 03-10-2019 12:04, Kyle Tso wrote:
> Hi Hans
> 
> Could you append the TCPM log?

I've attached both good and bad logs, both start with plugging in
one of these PD charging pass-through + USB-3 + HDMI out dongles.

at a quick glance the problem
seems to be that with the 2 AMS patches added we stop transmitting
after:

[  137.751964] Rx VDM cmd 0xff018144 type 1 cmd 4 len 1

Where as the good logs still transmits (and receives) a couple of
packets extra after this:

[ 4475.965108] Rx VDM cmd 0xff018144 type 1 cmd 4 len 1
[ 4475.965224] PD TX, header: 0x2f6f
[ 4475.968979] PD TX complete, status: 0
[ 4475.980811] PD RX, header: 0x2a4f [1]
[ 4475.980816] Rx VDM cmd 0xff018150 type 1 cmd 16 len 2
[ 4475.980929] PD TX, header: 0x216f
[ 4475.984093] PD TX complete, status: 0
[ 4475.996798] PD RX, header: 0x1c4f [1]
[ 4475.996803] Rx VDM cmd 0xff018151 type 1 cmd 17 len 1

Regards,

Hans


> 
> On Thu, Oct 3, 2019 at 5:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Kyle,
>>
>> On 20-09-2019 05:24, Kyle Tso wrote:
>>> *** BLURB HERE ***
>>>
>>> Kyle Tso (2):
>>>     usb: typec: tcpm: AMS and Collision Avoidance
>>>     usb: typec: tcpm: AMS for PD2.0
>>
>> I've finally gotten a chance to test this on one of my own devices
>> which uses the tcpm framework for its Type-c port.
>>
>> I am afraid that this series breaks DP altmode support,
>> specifically, the dp_altmode_configure() function no longer
>> seems to get called, leading to no pin-assignment being
>> selected by default and DP thus not working.
>>
>> So sorry, but I have to NACK this series since it causes
>> regressions.
>>
>> It might be easiest if you can get yourself some hardware
>> which supports DP altmode and uses the fusb302 Type-C
>> controller (which unlike your controller is actually
>> supported by the mainline kernel).
>>
>> 2 devices which have this are the original (version 1)
>> of the GPD win and the GPD pocket. Since the version
>> is not always clearly marked, make sure you get one which
>> has a X7-Z8750 CPU, those are the version 1 models, you
>> can still get these e.g. here:
>>
>> https://www.geekbuying.com/item/GPD-Pocket-7-Inch-Tablet-PC-Intel-Atom-X7-Z8750-8GB-128GB-375711.html
>> https://www.geekbuying.com/item/GPD-Win-Intel-Z8750-Windows-10-4GB-64GB-Gamepad-Tablet-PC---Black-378018.html
>>
>> These 2 devices still need 2 minor patches to hookup the DP
>> support, I have just finished these 2 patches up and I'm
>> submitting them upstream today, I will Cc you on these.
>>
>> If you combine these with one of the many DP-charging pass-through
>> + USB-3 out + HDMI out dongles, e.g.:
>> https://www.aliexpress.com/item/32953320909.html
>>
>> And then after plugging in do:
>>
>> cat /sys/class/typec/port0-partner/port0-partner.0/displayport/pin_assignment
>>
>> This should print:
>>
>> C [D]
>>
>> But when I add your patches into the mix it prints just:
>>
>> C D
>>
>> And these debug pr_err calls never happen:
>>
>> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
>> index 7845df030b72..d14f94078dd9 100644
>> --- a/drivers/usb/typec/altmodes/displayport.c
>> +++ b/drivers/usb/typec/altmodes/displayport.c
>> @@ -106,6 +106,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
>>                  break;
>>          }
>>
>> +       pr_err("dp_altmode_configure pin_assign %08x conf %08x\n", pin_assign, DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
>>          /* Determining the initial pin assignment. */
>>          if (!DP_CONF_GET_PIN_ASSIGN(dp->data.conf)) {
>>                  /* Is USB together with DP preferred */
>> @@ -115,6 +116,8 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
>>                  else if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK)
>>                          pin_assign &= DP_PIN_ASSIGN_DP_ONLY_MASK;
>>
>> +               pr_err("dp_altmode_configure masked pin_assign %08x\n", pin_assign);
>> +
>>                  if (!pin_assign)
>>                          return -EINVAL;
>>
>>
>> Regards,
>>
>> Hans
>>
Kyle Tso Oct. 4, 2019, 1:47 p.m. UTC | #8 
Hi Hans,

It seems that there is a bug in the patch. Thank you for catching this!
I will revise the patch and upload it again.





On Thu, Oct 3, 2019 at 6:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 03-10-2019 12:04, Kyle Tso wrote:
> > Hi Hans
> >
> > Could you append the TCPM log?
>
> I've attached both good and bad logs, both start with plugging in
> one of these PD charging pass-through + USB-3 + HDMI out dongles.
>
> at a quick glance the problem
> seems to be that with the 2 AMS patches added we stop transmitting
> after:
>
> [  137.751964] Rx VDM cmd 0xff018144 type 1 cmd 4 len 1
>
> Where as the good logs still transmits (and receives) a couple of
> packets extra after this:
>
> [ 4475.965108] Rx VDM cmd 0xff018144 type 1 cmd 4 len 1
> [ 4475.965224] PD TX, header: 0x2f6f
> [ 4475.968979] PD TX complete, status: 0
> [ 4475.980811] PD RX, header: 0x2a4f [1]
> [ 4475.980816] Rx VDM cmd 0xff018150 type 1 cmd 16 len 2
> [ 4475.980929] PD TX, header: 0x216f
> [ 4475.984093] PD TX complete, status: 0
> [ 4475.996798] PD RX, header: 0x1c4f [1]
> [ 4475.996803] Rx VDM cmd 0xff018151 type 1 cmd 17 len 1
>
> Regards,
>
> Hans
>
>
> >
> > On Thu, Oct 3, 2019 at 5:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Kyle,
> >>
> >> On 20-09-2019 05:24, Kyle Tso wrote:
> >>> *** BLURB HERE ***
> >>>
> >>> Kyle Tso (2):
> >>>     usb: typec: tcpm: AMS and Collision Avoidance
> >>>     usb: typec: tcpm: AMS for PD2.0
> >>
> >> I've finally gotten a chance to test this on one of my own devices
> >> which uses the tcpm framework for its Type-c port.
> >>
> >> I am afraid that this series breaks DP altmode support,
> >> specifically, the dp_altmode_configure() function no longer
> >> seems to get called, leading to no pin-assignment being
> >> selected by default and DP thus not working.
> >>
> >> So sorry, but I have to NACK this series since it causes
> >> regressions.
> >>
> >> It might be easiest if you can get yourself some hardware
> >> which supports DP altmode and uses the fusb302 Type-C
> >> controller (which unlike your controller is actually
> >> supported by the mainline kernel).
> >>
> >> 2 devices which have this are the original (version 1)
> >> of the GPD win and the GPD pocket. Since the version
> >> is not always clearly marked, make sure you get one which
> >> has a X7-Z8750 CPU, those are the version 1 models, you
> >> can still get these e.g. here:
> >>
> >> https://www.geekbuying.com/item/GPD-Pocket-7-Inch-Tablet-PC-Intel-Atom-X7-Z8750-8GB-128GB-375711.html
> >> https://www.geekbuying.com/item/GPD-Win-Intel-Z8750-Windows-10-4GB-64GB-Gamepad-Tablet-PC---Black-378018.html
> >>
> >> These 2 devices still need 2 minor patches to hookup the DP
> >> support, I have just finished these 2 patches up and I'm
> >> submitting them upstream today, I will Cc you on these.
> >>
> >> If you combine these with one of the many DP-charging pass-through
> >> + USB-3 out + HDMI out dongles, e.g.:
> >> https://www.aliexpress.com/item/32953320909.html
> >>
> >> And then after plugging in do:
> >>
> >> cat /sys/class/typec/port0-partner/port0-partner.0/displayport/pin_assignment
> >>
> >> This should print:
> >>
> >> C [D]
> >>
> >> But when I add your patches into the mix it prints just:
> >>
> >> C D
> >>
> >> And these debug pr_err calls never happen:
> >>
> >> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> >> index 7845df030b72..d14f94078dd9 100644
> >> --- a/drivers/usb/typec/altmodes/displayport.c
> >> +++ b/drivers/usb/typec/altmodes/displayport.c
> >> @@ -106,6 +106,7 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
> >>                  break;
> >>          }
> >>
> >> +       pr_err("dp_altmode_configure pin_assign %08x conf %08x\n", pin_assign, DP_CONF_GET_PIN_ASSIGN(dp->data.conf));
> >>          /* Determining the initial pin assignment. */
> >>          if (!DP_CONF_GET_PIN_ASSIGN(dp->data.conf)) {
> >>                  /* Is USB together with DP preferred */
> >> @@ -115,6 +116,8 @@ static int dp_altmode_configure(struct dp_altmode *dp, u8 con)
> >>                  else if (pin_assign & DP_PIN_ASSIGN_DP_ONLY_MASK)
> >>                          pin_assign &= DP_PIN_ASSIGN_DP_ONLY_MASK;
> >>
> >> +               pr_err("dp_altmode_configure masked pin_assign %08x\n", pin_assign);
> >> +
> >>                  if (!pin_assign)
> >>                          return -EINVAL;
> >>
> >>
> >> Regards,
> >>
> >> Hans
> >>