Received-SPF: pass (google.com: domain of linux-kernel+bounces-137679-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240409-qcom-ucsi-fixes-bis-v2-0-6d3a09faec90@linaro.org>
 <20240409-qcom-ucsi-fixes-bis-v2-3-6d3a09faec90@linaro.org> <ZhWWYQMluJCvYFKF@cae.in-ulm.de>
In-Reply-To: <ZhWWYQMluJCvYFKF@cae.in-ulm.de>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Wed, 10 Apr 2024 01:58:58 +0300
Message-ID: <CAA8EJprTbtTSkZ18dejEgvhJOEQKQiwpE+6JkbHiO4H-yeKuhg@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] usb: typec: ucsi_glink: drop special handling for CCI_BUSY
To: "Christian A. Ehrhardt" <lk@c--e.de>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Neil Armstrong <neil.armstrong@linaro.org>, 
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-arm-msm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Tue, 9 Apr 2024 at 22:26, Christian A. Ehrhardt <lk@c--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@linux.intel.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.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.

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. 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 don't have the hardware to do this myself.

-- 
With best wishes
Dmitry