Re: [RESEND] USB PD broken on Lenovo P15gen2

[Nikolay Borisov]

On 31.08.23 г. 16:41 ч., Heikki Krogerus wrote:
> Hi Nikolay,
>
> Thanks for the report.
>
> On Wed, Aug 30, 2023 at 04:07:55PM +0300, Nikolay Borisov wrote:
> > On 28.08.23 г. 17:52 ч., Nikolay Borisov wrote:
> > > [Resending as I had initially attached  a full acpi dump and it got
> > > bounced from the usb mailing list]
> > >
> > > Hello,
> > >
> > > I'm not able to use usb PD on a Lenovo Thinkpad P15gen2 laptop. It's
> > > equipped with 2 thunderbolt ports and a usb 3.2 gen2 usb port, all of
> > > which are supposed to support PD 2.0:
> >
> > <snip>
> > So I've been debugging this and what the PPM reports is the following:
> >
> > modprobe-529501  [004] ..... 33507.058332: ucsi_register: Supported UCSI spec: 100
> >       kworker/4:0-524223  [004] ..... 33507.486591: ucsi_init_work: Connectors supported: 3
> >       kworker/4:0-524223  [004] ..... 33507.486592: ucsi_init_work: GET_CAP: USB_PD: 0 TYPEC_CURRENT: 1 POWER_VBUS: 0, POWER_OTHER: 0, POWER_AC_SUPPLY: 1, BATTERY_CHARGING: 0 bcVersion: 0x102 typec_version: 0x100 pd_version: 0x200 PDO_DETAILS: 0
> >       kworker/4:0-524223  [004] ..... 33507.682726: ucsi_init_work: [Register port 1]: OPMODE: E4 flag:1
> >       kworker/4:0-524223  [004] ..... 33508.850438: ucsi_init_work: [Register port 2]: OPMODE: E4 flag:1
> >       kworker/4:0-524223  [004] ..... 33509.986672: ucsi_init_work: [Register port 3]: OPMODE: E4 flag:1
> >
> >
> > So all three ports support DRP/USB2/USB3/ALT_MODE and they can be a provider.
> >
> >
> > I find it strange that USB_PD is reported as 0 yet pd_version is reported as 2. I contacted Lenovo's support and they confirmed that this particular model indeed supports PD 3.0 on all USBC ports.
> >
> > I see a couple of problems with the current upstream code:
> >
> > 1. It assumes that USB_PD is valid because the PD version from pd_version is being propagated to several places (like in ucsi_register_port() cap->pd_revision = ucsi->cap.pd_version;)
>
> This part should be fixed.
>
> > 2. When typec_register_port() is called from ucsi_register_port() cap->pd is 0 hence the port->pd = cap->pd; assignment in typec_register_port is a noop. In fact I don't see where cap->pd is being initialized since we initialize con->pd when we call usb_power_delivery_register in ucsi_register_port().
>
> That "pd" member in struct typec_capability is optional. It can be
> used if the driver has a set of USB PD capabilities meant for USB
> Type-C port ready before the port is registered, but in UCSI driver
> the PD stuff are registered after the port.
>
> So I'm not sure there is anything wrong here.
>
> > Is it mandatory that GET_PDOS is supported if PD is supported, the UCSI spec doesn't say anything other than GET_PDOS is optional and signaled by bit in the GET_CAP call ?
>
> It looks like nobody ever checked is the command supported or not
> before using it. That's a bug.


If we assume support of this command is checked based on the respective 
bit in the optional capabilities member of GET_CAPS message. Is this 
command actually mandatory to properly support PD. I'm currently in 
contact with Lenovo as it seems there might be problems in their 
firmware as well i.e what they are reporting.
> thanks,