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

[Dmitry Baryshkov]

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 <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >  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.

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.

> 
> This will also allow the user space to see the altmode normally in
> sysfs.
> 
> > +	else
> > +		typec_set_mode(con->port, mode);
> > +
> > +	if (port->bridge)
> > +		drm_aux_hpd_bridge_notify(&port->bridge->dev,
> > +					  port->hpd_state ?
> > +					  connector_status_connected :
> > +					  connector_status_disconnected);
> > +
> > +}
> 
> thanks,
> 
> -- 
> heikki

-- 
With best wishes
Dmitry