| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250106092224.251115-1-mitltlatltl@gmail.com>
Date: Mon, 6 Jan 2025 17:22:17 +0800
From: Pengyu Luo <mitltlatltl@...il.com>
To: dmitry.baryshkov@...aro.org
Cc: andersson@...nel.org,
bryan.odonoghue@...aro.org,
conor+dt@...nel.org,
devicetree@...r.kernel.org,
gregkh@...uxfoundation.org,
hdegoede@...hat.com,
heikki.krogerus@...ux.intel.com,
ilpo.jarvinen@...ux.intel.com,
konradybcio@...nel.org,
krzk+dt@...nel.org,
linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org,
linux-usb@...r.kernel.org,
mitltlatltl@...il.com,
nikita@...n.ru,
platform-driver-x86@...r.kernel.org,
robh@...nel.org,
sre@...nel.org
Subject: Re: [PATCH 3/5] usb: typec: ucsi: add Huawei Matebook E Go (sc8280xp) ucsi driver
Please ignore the last email, I sent the wrong archive.
On Mon, Jan 6, 2025 at 11:33 AM Dmitry Baryshkov <dmitry.baryshkov@...aro.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@...aro.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@...il.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
Powered by blists - more mailing lists