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