Hi,
On Fri, Jan 19, 2024 at 10:34:09AM +0800, Haotien Hsu wrote:
> With the Cypress CCGx Type-C controller the following error is
> sometimes observed on boot:
> [ 16.087147] ucsi_ccg 1-0008: failed to reset PPM!
> [ 16.087319] ucsi_ccg 1-0008: PPM init failed (-110)
>
> When the above timeout occurs the following happens:
> 1. The function ucsi_reset_ppm() is called to reset UCSI controller.
> This function performs an async write to start reset and then
> polls for completion.
> 2. An interrupt occurs when the reset completes. In the interrupt
> handler, the OPM field in the INTR_REG is cleared and this clears
> the CCI data in the PPM. Hence, the reset completion status is
> cleared.
> 3. The function ucsi_reset_ppm() continues to poll for the reset
> completion, but has missed the reset completion event and
> eventually timeouts.
>
> In this patch, we store CCI when handling the interrupt and make
> reading after async write gets the correct value.
>
> To align with the CCGx UCSI interface guide, this patch updates the
> driver to copy CCI and MESSAGE_IN before they are reset when UCSI
> interrupt acknowledged.
>
> When a new command is sent, the driver will clear the old CCI to avoid
> ucsi_ccg_read() getting wrong CCI after ucsi_ccg_async_write() when
> the UCSI interrupt is not handled.
>
> Finally, acking the UCSI_READ_INT interrupt before calling complete()
> in ISR to ensure that the ucsi_ccg_sync_write() would wait for the
> interrupt handling to complete.
>
> Signed-off-by: Sing-Han Chen <singhanc@xxxxxxxxxx>
> Signed-off-by: Haotien Hsu <haotienh@xxxxxxxxxx>
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> V1->V2
> - Fix uninitialized symbol 'cci'
> v2->v3
> - Remove misusing Reported-by tags
> v3->v4
> - Add comments for op_lock
> v4->v5
> - Specify the endianness of registers in struct op_region
> - Replace ccg_op_region_read() with inline codes
> - Replace ccg_op_region_clean() with inline codes
> - Replace stack memory with GFP_ATOMIC allocated memory in ccg_op_region_update()
> - Add comments for resetting CCI in ucsi_ccg_async_write()
> v5->v6
> - Fix sparse warning: incorrect type in assignment
> v6->v7
> - Move changelog to below
> ---
> drivers/usb/typec/ucsi/ucsi_ccg.c | 92 ++++++++++++++++++++++++++++---
> 1 file changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 449c125f6f87..dda7c7c94e08 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -192,6 +192,12 @@ struct ucsi_ccg_altmode {
> bool checked;
> } __packed;
>
> +#define CCGX_MESSAGE_IN_MAX 4
> +struct op_region {
> + __le32 cci;
> + __le32 message_in[CCGX_MESSAGE_IN_MAX];
> +};
> +
> struct ucsi_ccg {
> struct device *dev;
> struct ucsi *ucsi;
> @@ -222,6 +228,13 @@ struct ucsi_ccg {
> bool has_multiple_dp;
> struct ucsi_ccg_altmode orig[UCSI_MAX_ALTMODES];
> struct ucsi_ccg_altmode updated[UCSI_MAX_ALTMODES];
> +
> + /*
> + * This spinlock protects op_data which includes CCI and MESSAGE_IN that
> + * will be updated in ISR
> + */
> + spinlock_t op_lock;
> + struct op_region op_data;
> };
>
> static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> @@ -305,12 +318,42 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, const u8 *data, u32 len)
> return 0;
> }
>
> +static int ccg_op_region_update(struct ucsi_ccg *uc, u32 cci)
> +{
> + u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_MESSAGE_IN);
> + struct op_region *data = &uc->op_data;
> + unsigned char *buf;
> + size_t size = sizeof(data->message_in);
> +
> + buf = kzalloc(size, GFP_ATOMIC);
> + if (!buf)
> + return -ENOMEM;
> + if (UCSI_CCI_LENGTH(cci)) {
> + int ret = ccg_read(uc, reg, (void *)buf, size);
> +
> + if (ret) {
> + kfree(buf);
> + return ret;
> + }
> + }
> +
> + spin_lock(&uc->op_lock);
> + data->cci = cpu_to_le32(cci);
> + if (UCSI_CCI_LENGTH(cci))
> + memcpy(&data->message_in, buf, size);
> + spin_unlock(&uc->op_lock);
> + kfree(buf);
> + return 0;
> +}
> +
> static int ucsi_ccg_init(struct ucsi_ccg *uc)
> {
> unsigned int count = 10;
> u8 data;
> int status;
>
> + spin_lock_init(&uc->op_lock);
> +
> data = CCGX_RAB_UCSI_CONTROL_STOP;
> status = ccg_write(uc, CCGX_RAB_UCSI_CONTROL, &data, sizeof(data));
> if (status < 0)
> @@ -520,9 +563,20 @@ static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
> u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
> struct ucsi_capability *cap;
> struct ucsi_altmode *alt;
> - int ret;
> + int ret = 0;
> +
> + if (offset == UCSI_CCI) {
> + spin_lock(&uc->op_lock);
> + memcpy(val, &(uc->op_data).cci, val_len);
> + spin_unlock(&uc->op_lock);
> + } else if (offset == UCSI_MESSAGE_IN) {
> + spin_lock(&uc->op_lock);
> + memcpy(val, &(uc->op_data).message_in, val_len);
> + spin_unlock(&uc->op_lock);
> + } else {
> + ret = ccg_read(uc, reg, val, val_len);
> + }
>
> - ret = ccg_read(uc, reg, val, val_len);
> if (ret)
> return ret;
>
> @@ -559,9 +613,18 @@ static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
> static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
> const void *val, size_t val_len)
> {
> + struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
> u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
>
> - return ccg_write(ucsi_get_drvdata(ucsi), reg, val, val_len);
> + /*
> + * UCSI may read CCI instantly after async_write,
> + * clear CCI to avoid caller getting wrong data before we get CCI from ISR
> + */
> + spin_lock(&uc->op_lock);
> + uc->op_data.cci = 0;
> + spin_unlock(&uc->op_lock);
> +
> + return ccg_write(uc, reg, val, val_len);
> }
>
> static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
> @@ -615,13 +678,18 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
> u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_CCI);
> struct ucsi_ccg *uc = data;
> u8 intr_reg;
> - u32 cci;
> - int ret;
> + u32 cci = 0;
> + int ret = 0;
>
> ret = ccg_read(uc, CCGX_RAB_INTR_REG, &intr_reg, sizeof(intr_reg));
> if (ret)
> return ret;
>
> + if (!intr_reg)
> + return IRQ_HANDLED;
> + else if (!(intr_reg & UCSI_READ_INT))
> + goto err_clear_irq;
> +
> ret = ccg_read(uc, reg, (void *)&cci, sizeof(cci));
> if (ret)
> goto err_clear_irq;
> @@ -629,13 +697,21 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
> if (UCSI_CCI_CONNECTOR(cci))
> ucsi_connector_change(uc->ucsi, UCSI_CCI_CONNECTOR(cci));
>
> - if (test_bit(DEV_CMD_PENDING, &uc->flags) &&
> - cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
> - complete(&uc->complete);
> + /*
> + * As per CCGx UCSI interface guide, copy CCI and MESSAGE_IN
> + * to the OpRegion before clear the UCSI interrupt
> + */
> + ret = ccg_op_region_update(uc, cci);
> + if (ret)
> + goto err_clear_irq;
>
> err_clear_irq:
> ccg_write(uc, CCGX_RAB_INTR_REG, &intr_reg, sizeof(intr_reg));
>
> + if (!ret && test_bit(DEV_CMD_PENDING, &uc->flags) &&
> + cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
> + complete(&uc->complete);
> +
> return IRQ_HANDLED;
> }
>
> --
> 2.34.1
--
heikki
