Re: [PATCH 3/5] usb: typec: ucsi: add Huawei Matebook E Go (sc8280xp) ucsi driver

[Pengyu Luo <mitltlatltl@gmail.com>]

Please ignore the last email, I sent the wrong archive.

On Mon, Jan 6, 2025 at 11:33 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Sun, Dec 29, 2024 at 05:05:47PM +0800, Pengyu Luo wrote:
> > On Sun, Dec 29, 2024 at 12:40 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > > On Sat, Dec 28, 2024 at 01:13:51AM +0800, Pengyu Luo wrote:
> > > > The Huawei Matebook E Go (sc8280xp) tablet provides implements UCSI
> > > > interface in the onboard EC. Add the glue driver to interface the
> > > > platform's UCSI implementation.
> > > >
> > > > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > > > ---
> > > >  drivers/usb/typec/ucsi/Kconfig              |   9 +
> > > >  drivers/usb/typec/ucsi/Makefile             |   1 +
> > > >  drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c | 481 ++++++++++++++++++++
> > > >  3 files changed, 491 insertions(+)
> > > >  create mode 100644 drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > > > index 680e1b87b..0d0f07488 100644
> > > > --- a/drivers/usb/typec/ucsi/Kconfig
> > > > +++ b/drivers/usb/typec/ucsi/Kconfig
> > > > @@ -78,4 +78,13 @@ config UCSI_LENOVO_YOGA_C630
> > > >         To compile the driver as a module, choose M here: the module will be
> > > >         called ucsi_yoga_c630.

[...]

> > > > +
> > > > +     spin_lock_irqsave(&port->lock, flags);
> > > > +
> > > > +     port->ccx = FIELD_GET(GAOKUN_CCX_MASK, dcc);
> > > > +     port->mux = FIELD_GET(GAOKUN_MUX_MASK, dcc);
> > > > +     port->mode = FIELD_GET(GAOKUN_DPAM_MASK, ddi);
> > > > +     port->hpd_state = FIELD_GET(GAOKUN_HPD_STATE_MASK, ddi);
> > > > +     port->hpd_irq = FIELD_GET(GAOKUN_HPD_IRQ_MASK, ddi);
> > > > +
> > > > +     switch (port->mux) {
> > > > +     case USBC_MUX_NONE:
> > > > +             port->svid = 0;
> > > > +             break;
> > > > +     case USBC_MUX_USB_2L:
> > > > +             port->svid = USB_SID_PD;
> > > > +             break;
> > > > +     case USBC_MUX_DP_4L:
> > > > +     case USBC_MUX_USB_DP:
> > > > +             port->svid = USB_SID_DISPLAYPORT;
> > > > +             if (port->ccx == USBC_CCX_REVERSE)
> > > > +                     port->mode -= 6;
> > >
> > > I'd prefer it this were more explicit about what is happening.
> > >
> >
> > If orientation is reverse, then we should minus 6, EC's logic.
> > I will add a comment for it. Actually, this field is unused, I don't
> > find the mux yet, so I cannot set it with this field. But I don't want
> > to make things imcomplete, so keep it.
>
> Which values are you expecting / getting there? The -6 is a pure magic.
> Please replace this with a switch-case or something more obvious.
>

In v2, I have deduced their meaning, with a switch to map them.

> > Let me go off the topic, on my device, I can just use drm_aux_hpd_bridge_notify
> > to enable altmode, usb functions well after I pluged out, I don't need set mode
> > switch(orientation switch is required if orientation is reverse), which is quiet
> > similar to Acer aspire 1. Is mux controlled also by QMP combo phy(see [1])?
> >
> > > > +             break;
> > > > +     default:
> > > > +             break;
> > > > +     }
> > > > +
> > > > +     spin_unlock_irqrestore(&port->lock, flags);
> > > > +}
> > > > +
> > > > +static int gaokun_ucsi_refresh(struct gaokun_ucsi *uec)
> > > > +{
> > > > +     struct gaokun_ucsi_reg ureg;
> > > > +     int ret, idx;
> > > > +
> > > > +     ret = gaokun_ec_ucsi_get_reg(uec->ec, (u8 *)&ureg);
> > > > +     if (ret)
> > > > +             return -EIO;
> > > > +
> > > > +     uec->port_num = ureg.port_num;
> > > > +     idx = GET_IDX(ureg.port_updt);
> > > > +
> > > > +     if (idx >= 0 && idx < ureg.port_num)
> > > > +             gaokun_ucsi_port_update(&uec->ports[idx], ureg.port_data);
> > > > +
> > > > +     return idx;
> > > > +}
> > > > +
> > > > +static void gaokun_ucsi_handle_altmode(struct gaokun_ucsi_port *port)
> > > > +{
> > > > +     struct gaokun_ucsi *uec = port->ucsi;
> > > > +     int idx = port->idx;
> > > > +
> > > > +     if (idx >= uec->ucsi->cap.num_connectors || !uec->ucsi->connector) {
> > > > +             dev_warn(uec->ucsi->dev, "altmode port out of range: %d\n", idx);
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     /* UCSI callback .connector_status() have set orientation */
> > > > +     if (port->bridge)
> > > > +             drm_aux_hpd_bridge_notify(&port->bridge->dev,
> > > > +                                       port->hpd_state ?
> > > > +                                       connector_status_connected :
> > > > +                                       connector_status_disconnected);
> > >
> > > Does your platform report any altmodes? What do you see in
> > > /sys/class/typec/port0/port0.*/ ?
> > >
> >
> > /sys/class/typec/port0/port0.0:
> > active  mode  mode1  power  svid  uevent  vdo
> >
> > /sys/class/typec/port0/port0.1:
> > active  mode  mode1  power  svid  uevent  vdo
> >
> > /sys/class/typec/port0/port0.2:
> > active  mode  mode1  power  svid  uevent  vdo
> >
> > /sys/class/typec/port0/port0.3:
> > active  mode  mode2  power  svid  uevent  vdo
> >
> > /sys/class/typec/port0/port0.4:
> > active  mode  mode3  power  svid  uevent  vdo
>
> please:
>
> cat /sys/class/typec/port0/port0*/svid
> cat /sys/class/typec/port0/port0*/vdo
>

svid:
8087
ff01
12d1
12d1
12d1

vdo:
0xff000001
0xff1c1c46
0xff000001
0xff000002
0xff000003

> If DP is reported as one the altmodes, then it should be using the
> DisplayPort AltMode driver, as suggested by Heikki.
>

But this paltform cannot access to the partner device, related API
requires a partner.

BTW, it is unnecessary that implementing/call a DP Altmode driver for
this platform. Currently, we can enter altmode with a HPD event notify.
This point is quiet similar to Acer aspire 1. I mentioned this when we
last talked about minus 6.

> > > > +
> > > > +     gaokun_ec_ucsi_pan_ack(uec->ec, port->idx);
> > > > +}
> > > > +
> > > > +static void gaokun_ucsi_altmode_notify_ind(struct gaokun_ucsi *uec)
> > > > +{
> > > > +     int idx;
> > > > +
> > > > +     idx = gaokun_ucsi_refresh(uec);
> > > > +     if (idx < 0)
> > > > +             gaokun_ec_ucsi_pan_ack(uec->ec, idx);
> > > > +     else
> > > > +             gaokun_ucsi_handle_altmode(&uec->ports[idx]);
> > > > +}
> > > > +
> > > > +/*
> > > > + * USB event is necessary for enabling altmode, the event should follow
> > > > + * UCSI event, if not after timeout(this notify may be disabled somehow),
> > > > + * then force to enable altmode.
> > > > + */
> > > > +static void gaokun_ucsi_handle_no_usb_event(struct gaokun_ucsi *uec, int idx)
> > > > +{
> > > > +     struct gaokun_ucsi_port *port;
> > > > +
> > > > +     port = &uec->ports[idx];
> > > > +     if (!wait_for_completion_timeout(&port->usb_ack, 2 * HZ)) {
> > > > +             dev_warn(uec->dev, "No USB EVENT, triggered by UCSI EVENT");
> > > > +             gaokun_ucsi_altmode_notify_ind(uec);
> > > > +     }
> > > > +}
> > > > +

[...]

> > > > +
> > > > +static void gaokun_ucsi_register_worker(struct work_struct *work)
> > > > +{
> > > > +     struct gaokun_ucsi *uec;
> > > > +     struct ucsi *ucsi;
> > > > +     int ret;
> > > > +
> > > > +     uec = container_of(work, struct gaokun_ucsi, work);
> > > > +     ucsi = uec->ucsi;
> > > > +
> > > > +     ucsi->quirks = UCSI_NO_PARTNER_PDOS | UCSI_DELAY_DEVICE_PDOS;
> > >
> > > Does it crash in the same way as GLINK crashes (as you've set
> > > UCSI_NO_PARTNER_PDOS)?
> > >
> >
> > Yes, no partner can be detected, I checked. I think it is also handled by
> > the firmware As you said in [2]
> > > In some obscure cases (Qualcomm PMIC Glink) altmode is completely
> > > handled by the firmware. Linux does not get proper partner altmode info.
>
> This is a separate topic. Those two flags were added for a very
> particular reason:
>
> - To workaround firmware crash on requesting PDOs for a partner
> - To delay requeting PDOs for the device because in the unconnected
>   state the GET_PDOS returns incorrect information
>
> Are you sure that those two flags are necessary for your platform?
>

Alright, I think I got things mixed up. Actually PDO requires UCSI only,
not a partner device.

I think I will remove it in v3 if it works well during the time. Rencetly,
this platform works well without it. Thanks for pointing out.

> >
> > > > +
> > > > +     ssleep(3); /* EC can't handle UCSI properly in the early stage */
> > > > +
> > > > +     ret = gaokun_ec_register_notify(uec->ec, &uec->nb);
> > > > +     if (ret) {
> > > > +             dev_err_probe(ucsi->dev, ret, "notifier register failed\n");
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     ret = ucsi_register(ucsi);
> > > > +     if (ret)
> > > > +             dev_err_probe(ucsi->dev, ret, "ucsi register failed\n");
> > > > +}
> > > > +
> > > > +static int gaokun_ucsi_register(struct gaokun_ucsi *uec)
> > >
> > > Please inline
> > >
> >
> > I see.
> >
> > Best wishes
> > Pengyu
> >
> > [1] https://elixir.bootlin.com/linux/v6.12.5/source/drivers/phy/qualcomm/phy-qcom-qmp-combo.c#L2679
> > [2] https://lore.kernel.org/lkml/20240416-ucsi-glink-altmode-v1-0-890db00877ac@linaro.org


Best Wishes,
Pengyu