Re: [PATCH v7] ucsi_ccg: Refine the UCSI Interrupt handling

[Heikki Krogerus]

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