[v5] ucsi_ccg: Refine the UCSI Interrupt handling
Commit Message
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.
---
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()
Signed-off-by: Sing-Han Chen <singhanc@nvidia.com>
Signed-off-by: Haotien Hsu <haotienh@nvidia.com>
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 92 ++++++++++++++++++++++++++++---
1 file changed, 84 insertions(+), 8 deletions(-)
Comments
Hi Haotien,
kernel test robot noticed the following build warnings:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.7 next-20240117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Haotien-Hsu/ucsi_ccg-Refine-the-UCSI-Interrupt-handling/20240116-140628
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20240116060323.844959-1-haotienh%40nvidia.com
patch subject: [PATCH v5] ucsi_ccg: Refine the UCSI Interrupt handling
config: loongarch-randconfig-r131-20240117 (https://download.01.org/0day-ci/archive/20240118/202401180801.WsI6B2pq-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240118/202401180801.WsI6B2pq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401180801.WsI6B2pq-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/usb/typec/ucsi/ucsi_ccg.c:341:19: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __le32 [usertype] cci @@ got unsigned int [usertype] cci @@
drivers/usb/typec/ucsi/ucsi_ccg.c:341:19: sparse: expected restricted __le32 [usertype] cci
drivers/usb/typec/ucsi/ucsi_ccg.c:341:19: sparse: got unsigned int [usertype] cci
vim +341 drivers/usb/typec/ucsi/ucsi_ccg.c
320
321 static int ccg_op_region_update(struct ucsi_ccg *uc, u32 cci)
322 {
323 u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_MESSAGE_IN);
324 struct op_region *data = &uc->op_data;
325 unsigned char *buf;
326 size_t size = sizeof(data->message_in);
327
328 buf = kzalloc(size, GFP_ATOMIC);
329 if (!buf)
330 return -ENOMEM;
331 if (UCSI_CCI_LENGTH(cci)) {
332 int ret = ccg_read(uc, reg, (void *)buf, size);
333
334 if (ret) {
335 kfree(buf);
336 return ret;
337 }
338 }
339
340 spin_lock(&uc->op_lock);
> 341 data->cci = cci;
342 if (UCSI_CCI_LENGTH(cci))
343 memcpy(&data->message_in, buf, size);
344 spin_unlock(&uc->op_lock);
345 kfree(buf);
346 return 0;
347 }
348
@@ -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 = 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;
}