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@xxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxx/
> > > 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@xxxxxxx/
> 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
