diff mbox series

[v7] ucsi_ccg: Refine the UCSI Interrupt handling

Message ID 20240119023409.659452-1-haotienh@nvidia.com (mailing list archive)
State Superseded
Headers show
Series [v7] ucsi_ccg: Refine the UCSI Interrupt handling | expand

Commit Message

HaoTien Hsu Jan. 19, 2024, 2:34 a.m. UTC 
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@nvidia.com>
Signed-off-by: Haotien Hsu <haotienh@nvidia.com>

---
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(-)

Comments

Heikki Krogerus Jan. 24, 2024, 1:21 p.m. UTC | #1 
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@nvidia.com>
> Signed-off-by: Haotien Hsu <haotienh@nvidia.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 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
diff mbox series

Patch

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;
 }