Message-ID: <s2w7qfrbm3lq46hp4r4grmljg7cs24d6lmpiqr2mllxszte6wc@w7rh47o3z6rc>
Date: Thu, 11 Apr 2024 00:09:31 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: "Christian A. Ehrhardt" <lk@...e.de>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 3/3] usb: typec: ucsi_glink: drop special handling for
 CCI_BUSY

On Wed, Apr 10, 2024 at 09:41:39AM +0200, Christian A. Ehrhardt wrote:
> 
> Hi Dmitry,
> 
> On Wed, Apr 10, 2024 at 01:58:58AM +0300, Dmitry Baryshkov wrote:
> > On Tue, 9 Apr 2024 at 22:26, Christian A. Ehrhardt <lk@...e.de> wrote:
> > >
> > >
> > > Hi Dmitry,
> > >
> > > On Tue, Apr 09, 2024 at 06:29:18PM +0300, Dmitry Baryshkov wrote:
> > > > Newer Qualcomm platforms (sm8450+) successfully handle busy state and
> > > > send the Command Completion after sending the Busy state. Older devices
> > > > have firmware bug and can not continue after sending the CCI_BUSY state,
> > > > but the command that leads to CCI_BUSY is already forbidden by the
> > > > NO_PARTNER_PDOS quirk.
> > > >
> > > > Follow other UCSI glue drivers and drop special handling for CCI_BUSY
> > > > event. Let the UCSI core properly handle this state.
> > > >
> > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > > > ---
> > > >  drivers/usb/typec/ucsi/ucsi_glink.c | 10 ++++------
> > > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > index 9ffea20020e7..fe9b951f5228 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > @@ -176,7 +176,8 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
> > > >       left = wait_for_completion_timeout(&ucsi->sync_ack, 5 * HZ);
> > > >       if (!left) {
> > > >               dev_err(ucsi->dev, "timeout waiting for UCSI sync write response\n");
> > > > -             ret = -ETIMEDOUT;
> > > > +             /* return 0 here and let core UCSI code handle the CCI_BUSY */
> > > > +             ret = 0;
> > > >       } else if (ucsi->sync_val) {
> > > >               dev_err(ucsi->dev, "sync write returned: %d\n", ucsi->sync_val);
> > > >       }
> > > > @@ -243,11 +244,8 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
> > > >               ucsi_connector_change(ucsi->ucsi, con_num);
> > > >       }
> > > >
> > > > -     if (ucsi->sync_pending && cci & UCSI_CCI_BUSY) {
> > > > -             ucsi->sync_val = -EBUSY;
> > > > -             complete(&ucsi->sync_ack);
> > > > -     } else if (ucsi->sync_pending &&
> > > > -                (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
> > > > +     if (ucsi->sync_pending &&
> > > > +         (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
> > > >               complete(&ucsi->sync_ack);
> > > >       }
> > > >  }
> > >
> > > This handling of the command completion turned out to be racy in
> > > the ACPI case: If a normal command was sent one should wait for
> > > UCSI_CCI_COMMAND_COMPLETE only. In case of an UCSI_ACK_CC_CI
> > > command the completion is indicated by UCSI_CCI_ACK_COMPLETE.
> > >
> > > While not directly related, a port of this
> > >     https://lore.kernel.org/all/20240121204123.275441-3-lk@c--e.de/
> > > would nicely fit into this series.
> > 
> > Ack, I'll take a look.
> 
> Thanks.
> 
> > However... I can not stop but notice that CCG and STM32 glue drivers
> > use the same old approach as we do. Which probably means that they
> > might have the same issue.
> 
> I did ping the ccg people wrt. this but they have a different
> workaround that saves them at least most of the time, so I let
> this drop.
> 
> > Could you please consider pulling up that
> > code into the UCSI driver? Maybe the low-level code really should just
> > read/write the messages, leaving all completions and CCI parsing to
> > the core layer?
> 
> I did consider that but one of the ideas behind the new API for
> UCSI backends was that backends can send commands (e.g. as part of
> a quirk) even in the middle of a ->sync_write() call. Currently,
> I don't really see how to combine this with completion handling
> in the UCSI core.
> 
> > > I don't have the hardware to do this myself.
> 
> I did propose other changes to the API with little respone here:
>     https://lore.kernel.org/all/20240218222039.822040-1-lk@c--e.de/
> That could possibly be extended to achieve this. But again, that
> would require testers for all the backends.

Well, I think that the patchset is too intrusive and (from the
pmic-glink perspective) is too low-level.

I'd start by pulling the sync_write() into the core layer, leaving just
async_write in the glue layer. The async_write() then can be renamed to
something like send_cmd(). Once required we can add the data pointer to
this callback.

I liked the idea of getting the CCI from the notification (in case of
pmic-glink it works this way on all platforms except sc8180x).

So what about having a really simple interface:

sruct ucsi_operations {
	/*
	 * send the command without waiting for the result
	 * can be extended with u8 *data, size_t data_len once
	 * necessary.
	 * maybe use u8 control[8] instead of u64 control.
	 */
	int send_command(struct ucsi *, u64 control);

	int read_data(struct ucsi *, void *buf, size_t len);
	int read_version(struct ucsi *, u16 *version);
	/* to be used only for reset handling */
	int read_cci(struct ucsi *, u32 cci);

	// other ops like update_altmode, as is
};

/* to be called by the glue driver once it gets the notification from
 * PPM */
void ucsi_notify(struct ucsi *ucsi, u32 cci);

This way we can pull all the common ACK/connection_changed/completion
code into the core, while keeping glue layers flexible enough.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists