by Heikki Krogerus

[permalink] [raw]

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

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.
> > >
> > > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > > ---
> > > drivers/soc/qcom/Makefile | 1 -
> > > drivers/soc/qcom/pmic_glink_altmode.c | 546 ----------------------------------
> > > drivers/usb/typec/ucsi/ucsi_glink.c | 495 ++++++++++++++++++++++++++++--
> > > 3 files changed, 475 insertions(+), 567 deletions(-)
> > >
>
> [skipped the patch]
>
> > > +
> > > +static void pmic_glink_ucsi_register_altmode(struct ucsi_connector *con)
> > > +{
> > > + static const u8 all_assignments = BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D) |
> > > + BIT(DP_PIN_ASSIGN_E);
> > > + struct typec_altmode_desc desc;
> > > + struct typec_altmode *alt;
> > > +
> > > + mutex_lock(&con->lock);
> > > +
> > > + if (con->port_altmode[0])
> > > + goto out;
> > > +
> > > + memset(&desc, 0, sizeof(desc));
> > > + desc.svid = USB_TYPEC_DP_SID;
> > > + desc.mode = USB_TYPEC_DP_MODE;
> > > +
> > > + desc.vdo = DP_CAP_CAPABILITY(DP_CAP_DFP_D);
> > > +
> > > + /* We can't rely on the firmware with the capabilities. */
> > > + desc.vdo |= DP_CAP_DP_SIGNALLING(0) | DP_CAP_RECEPTACLE;
> > > +
> > > + /* Claiming that we support all pin assignments */
> > > + desc.vdo |= all_assignments << 8;
> > > + desc.vdo |= all_assignments << 16;
> > > +
> > > + alt = typec_port_register_altmode(con->port, &desc);
> >
> > alt = ucsi_register_displayport(con, 0, 0, &desc);
>
> Note, the existing UCSI displayport AltMode driver depends on the UCSI
> actually handling the altomode. It needs a partner, etc.
>
> > You need to export that function, but that should not be a problem:
> >
> > diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
> > index d9d3c91125ca..f2754d7b5876 100644
> > --- a/drivers/usb/typec/ucsi/displayport.c
> > +++ b/drivers/usb/typec/ucsi/displayport.c
> > @@ -315,11 +315,13 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
> > struct ucsi_dp *dp;
> >
> > /* We can't rely on the firmware with the capabilities. */
> > - desc->vdo |= DP_CAP_DP_SIGNALLING(0) | DP_CAP_RECEPTACLE;
> > + if (!desc->vdo) {
> > + desc->vdo = DP_CAP_DP_SIGNALLING(0) | DP_CAP_RECEPTACLE;
> >
> > - /* Claiming that we support all pin assignments */
> > - desc->vdo |= all_assignments << 8;
> > - desc->vdo |= all_assignments << 16;
> > + /* Claiming that we support all pin assignments */
> > + desc->vdo |= all_assignments << 8;
> > + desc->vdo |= all_assignments << 16;
> > + }
> >
> > alt = typec_port_register_altmode(con->port, desc);
> > if (IS_ERR(alt))
> > @@ -342,3 +344,4 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
> >
> > return alt;
> > }
> > +EXPORT_SYMBOL_GPL(ucsi_register_displayport);
> >
> > <snip>
> >
> > > +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.

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.

thanks,

--
heikki