by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [PATCH 7/8] usb: typec: ucsi: glink: merge pmic_glink_altmode driver

Hi Heikki,

On Sat, May 04, 2024 at 09:49:42AM +0300, Dmitry Baryshkov wrote:
> On Mon, 22 Apr 2024 at 18:02, Heikki Krogerus
> <[email protected]> wrote:
> >
> > Hi Dmitry,
> >
> > On Mon, Apr 22, 2024 at 03:45:22PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Apr 22, 2024 at 01:59:10PM +0300, Heikki Krogerus wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Tue, Apr 16, 2024 at 05:20:56AM +0300, Dmitry Baryshkov wrote:
> > > > > Move handling of USB Altmode to the ucsi_glink driver. This way the
> > > > > altmode is properly registered in the Type-C framework, the altmode
> > > > > handlers can use generic typec calls, the UCSI driver can use
> > > > > orientation information from altmode messages and vice versa, the
> > > > > altmode handlers can use GPIO-based orientation inormation from UCSI
> > > > > GLINK driver.
> > > > >

[skipped]

> > > Note, the existing UCSI displayport AltMode driver depends on the UCSI
> > > actually handling the altomode. It needs a partner, etc.
> > >

[skipped the patch]

> > > > > +static void pmic_glink_ucsi_set_state(struct ucsi_connector *con,
> > > > > + struct pmic_glink_ucsi_port *port)
> > > > > +{
> > > > > + struct typec_displayport_data dp_data = {};
> > > > > + struct typec_altmode *altmode = NULL;
> > > > > + unsigned long flags;
> > > > > + void *data = NULL;
> > > > > + int mode;
> > > > > +
> > > > > + spin_lock_irqsave(&port->lock, flags);
> > > > > +
> > > > > + if (port->svid == USB_SID_PD) {
> > > > > + mode = TYPEC_STATE_USB;
> > > > > + } else if (port->svid == USB_TYPEC_DP_SID && port->mode == DPAM_HPD_OUT) {
> > > > > + mode = TYPEC_STATE_SAFE;
> > > > > + } else if (port->svid == USB_TYPEC_DP_SID) {
> > > > > + altmode = find_altmode(con, port->svid);
> > > > > + if (!altmode) {
> > > > > + dev_err(con->ucsi->dev, "altmode woth SVID 0x%04x not found\n",
> > > > > + port->svid);
> > > > > + spin_unlock_irqrestore(&port->lock, flags);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + mode = TYPEC_MODAL_STATE(port->mode - DPAM_HPD_A);
> > > > > +
> > > > > + dp_data.status = DP_STATUS_ENABLED;
> > > > > + dp_data.status |= DP_STATUS_CON_DFP_D;
> > > > > + if (port->hpd_state)
> > > > > + dp_data.status |= DP_STATUS_HPD_STATE;
> > > > > + if (port->hpd_irq)
> > > > > + dp_data.status |= DP_STATUS_IRQ_HPD;
> > > > > + dp_data.conf = DP_CONF_SET_PIN_ASSIGN(port->mode - DPAM_HPD_A);
> > > > > +
> > > > > + data = &dp_data;
> > > > > + } else {
> > > > > + dev_err(con->ucsi->dev, "Unsupported SVID 0x%04x\n", port->svid);
> > > > > + spin_unlock_irqrestore(&port->lock, flags);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + spin_unlock_irqrestore(&port->lock, flags);
> > > > > +
> > > > > + if (altmode)
> > > > > + typec_altmode_set_port(altmode, mode, data);
> > > >
> > > > So if the port altmode is using the ucsi_displayport_ops, you can
> > > > simply register the partner altmode here instead. That should
> > > > guarantee that it'll bind to the DP altmode driver which will take
> > > > care of typec_altmode_enter() etc.
> > >
> > > In our case the altmode is unfortunately completely hidden inside the
> > > firmware. It is not exported via the native UCSI interface. Even if I
> > > plug the DP dongle, there is no partner / altmode being reported by the
> > > PPM. All DP events are reported via additional GLINK messages.
> >
> > I understand that there is no alt mode being reported, but I assumed
> > that there is a notification about connections.
> >
> > If that's not the case, then you need to use this code path to
> > register the partner device as well I think. The partner really has to
> > be registered somehow.
> >
> > > The goal is to use the core Type-C altmode handling, while keeping UCSI
> > > out of the altmode business.
> > >
> > > This allows the core to handle switches / muxes / retimers, report the
> > > altmode to the userspace via sysfs, keep the link between the DP part of
> > > the stack and the typec port, but at the same time we don't get errors
> > > from UCSI because of the PPM reporting unsupported commands, etc.
> >
> > I understand, and just to be clear, I don't have a problem with
> > bypassing UCSI. But that does not mean you can skip the alt mode
> > registration.
> >
> > The primary purpose of drivers/usb/typec/ucsi/displayport.c is to
> > emulate the partner DP alt mode device a little so that the actual DP
> > alt mode driver drivers/usb/typec/altmodes/displayport.c is happy. The
> > altmode driver will then make sure that all the muxes, switches and
> > what have you, are configured as they should, and more importantly,
> > make sure the DP alt mode is exposed to the user space exactly the
> > same way as it's exposed on all the other systems.
> >
> > There are a couple of UCSI commands that are being used there yes, but
> > by modifying it so that those UCSI commands are executed conditionally
> > - by checking the ALT_MODE_DETAILS feature - you should be able to use
> > it also in this case.
>
> I have played with the DP AltMode driver. I got it somewhat working,
> but I think I'm facing a control issue.
> Basically, the altmode driver wants to control pin assignment on its
> own. It works with the software TCPM, as we control it.
> It works with the normal UCSI, because it still can configure pin
> config. However with PMIC GLINK implementation there is no way to
> control pin assignment from the Linux side. The firmware does that for
> us.
> What would be the recommended way to handle it? Is it okay to override
> status_update to return just the selected pin config? Or is there any
> other (better) way to handle such an issue?

Any suggestions or further comments? Is it better to extend the
DisplayPort Altmode driver with the 'forced' transitions? Or it would be
fine to just register a partner device, emulate the userspace events,
but completely ignore the existing displayport driver?

>
> >
> > You really need to register the partner alt mode(s) one way or the
> > other in any case, and the partner device itself you absolutely must
> > register. The user space interface needs to be consistent.
>
> For reference, the partner is being reported and registered by the
> UCSI firmware. It's only the altmode itself where I'm facing the
> issue.

--
With best wishes
Dmitry

2024-05-16 12:05:40

by Heikki Krogerus

[permalink] [raw]

Subject: Re: [PATCH 7/8] usb: typec: ucsi: glink: merge pmic_glink_altmode driver

Hi Dmitry,

On Wed, May 15, 2024 at 06:01:48PM +0300, Dmitry Baryshkov wrote:
> > I have played with the DP AltMode driver. I got it somewhat working,
> > but I think I'm facing a control issue.
> > Basically, the altmode driver wants to control pin assignment on its
> > own. It works with the software TCPM, as we control it.
> > It works with the normal UCSI, because it still can configure pin
> > config. However with PMIC GLINK implementation there is no way to
> > control pin assignment from the Linux side. The firmware does that for
> > us.
> > What would be the recommended way to handle it? Is it okay to override
> > status_update to return just the selected pin config? Or is there any
> > other (better) way to handle such an issue?
>
> Any suggestions or further comments? Is it better to extend the
> DisplayPort Altmode driver with the 'forced' transitions? Or it would be
> fine to just register a partner device, emulate the userspace events,
> but completely ignore the existing displayport driver?

I may have to look at the DP alt mode with the TI host interface
(drivers/usb/typec/tipd/) at some point, because on some systems the
firmware does not seem to automatically enter the mode, but the forced
transition option would probable work there as well...

I would prefer that we don't ignore the DP alt mode driver, but just
to be clear, I'm also not completely against it if you feel that would
be the most reasonable option in your case.

br,

--
heikki