| Message ID | 20230103081531.423017-1-haotienh@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] ucsi_ccg: Refine the UCSI Interrupt handling | expand |
On Tue, Jan 03, 2023 at 04:15:31PM +0800, Haotien Hsu wrote: > From: Sing-Han Chen <singhanc@nvidia.com> > > For the CCGx, when the OPM field in the INTR_REG is cleared, then the > CCI data in the PPM is reset. > > To align with the CCGx UCSI interface guide, this patch updates the > driver to copy CCI and MESSAGE_IN before clearing UCSI interrupt. > When a new command is sent, the driver will clear the old CCI and > MESSAGE_IN copy. > > Finally, clear UCSI_READ_INT before calling complete() to ensure that > the ucsi_ccg_sync_write() would wait for the interrupt handling to > complete. > It prevents the driver from resetting CCI prematurely. > > 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 wrong Reported-by tags > --- > drivers/usb/typec/ucsi/ucsi_ccg.c | 86 ++++++++++++++++++++++++++++--- > 1 file changed, 79 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c > index eab3012e1b01..b35a3a97c9fb 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 { > + u32 cci; > + u32 message_in[CCGX_MESSAGE_IN_MAX]; > +}; > + > struct ucsi_ccg { > struct device *dev; > struct ucsi *ucsi; > @@ -222,6 +228,9 @@ struct ucsi_ccg { > bool has_multiple_dp; > struct ucsi_ccg_altmode orig[UCSI_MAX_ALTMODES]; > struct ucsi_ccg_altmode updated[UCSI_MAX_ALTMODES]; > + > + spinlock_t op_lock; What does this lock protect? Please document that so that we can verify if this really is a correct change _AND_ so we know what future changes need to take the lock or not. > + struct op_region op_data; > }; > > static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) > @@ -305,12 +314,57 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, const u8 *data, u32 len) > return 0; > } > > +static void ccg_op_region_read(struct ucsi_ccg *uc, unsigned int offset, > + void *val, size_t val_len) > +{ > + struct op_region *data = &uc->op_data; > + > + spin_lock(&uc->op_lock); > + if (offset == UCSI_CCI) > + memcpy(val, &data->cci, val_len); > + else if (offset == UCSI_MESSAGE_IN) > + memcpy(val, &data->message_in, val_len); > + spin_unlock(&uc->op_lock); > +} > + > +static void 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; > + u32 message_in[CCGX_MESSAGE_IN_MAX]; > + > + if (UCSI_CCI_LENGTH(cci)) > + if (ccg_read(uc, reg, (void *)&message_in, > + sizeof(message_in))) { > + dev_err(uc->dev, "failed to read MESSAGE_IN\n"); What can userspace do with this error? Will it be repeated a lot? thanks, greg k-h
On 1/6/23 16:10, Greg Kroah-Hartman wrote: > External email: Use caution opening links or attachments > > > On Tue, Jan 03, 2023 at 04:15:31PM +0800, Haotien Hsu wrote: >> From: Sing-Han Chen <singhanc@nvidia.com> >> >> For the CCGx, when the OPM field in the INTR_REG is cleared, then the >> CCI data in the PPM is reset. >> >> To align with the CCGx UCSI interface guide, this patch updates the >> driver to copy CCI and MESSAGE_IN before clearing UCSI interrupt. >> When a new command is sent, the driver will clear the old CCI and >> MESSAGE_IN copy. >> >> Finally, clear UCSI_READ_INT before calling complete() to ensure that >> the ucsi_ccg_sync_write() would wait for the interrupt handling to >> complete. >> It prevents the driver from resetting CCI prematurely. >> >> 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 wrong Reported-by tags >> --- >> drivers/usb/typec/ucsi/ucsi_ccg.c | 86 ++++++++++++++++++++++++++++--- >> 1 file changed, 79 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c >> index eab3012e1b01..b35a3a97c9fb 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 { >> + u32 cci; >> + u32 message_in[CCGX_MESSAGE_IN_MAX]; >> +}; >> + >> struct ucsi_ccg { >> struct device *dev; >> struct ucsi *ucsi; >> @@ -222,6 +228,9 @@ struct ucsi_ccg { >> bool has_multiple_dp; >> struct ucsi_ccg_altmode orig[UCSI_MAX_ALTMODES]; >> struct ucsi_ccg_altmode updated[UCSI_MAX_ALTMODES]; >> + >> + spinlock_t op_lock; > > What does this lock protect? Please document that so that we can verify > if this really is a correct change _AND_ so we know what future changes > need to take the lock or not. > /* This spinlock protects op_data which includes CCI and MESSAGE_IN that * will be updated in ISR */ spinlock_t op_lock; I will add this comment into v4. >> + struct op_region op_data; >> }; >> >> static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len) >> @@ -305,12 +314,57 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, const u8 *data, u32 len) >> return 0; >> } >> >> +static void ccg_op_region_read(struct ucsi_ccg *uc, unsigned int offset, >> + void *val, size_t val_len) >> +{ >> + struct op_region *data = &uc->op_data; >> + >> + spin_lock(&uc->op_lock); >> + if (offset == UCSI_CCI) >> + memcpy(val, &data->cci, val_len); >> + else if (offset == UCSI_MESSAGE_IN) >> + memcpy(val, &data->message_in, val_len); >> + spin_unlock(&uc->op_lock); >> +} >> + >> +static void 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; >> + u32 message_in[CCGX_MESSAGE_IN_MAX]; >> + >> + if (UCSI_CCI_LENGTH(cci)) >> + if (ccg_read(uc, reg, (void *)&message_in, >> + sizeof(message_in))) { >> + dev_err(uc->dev, "failed to read MESSAGE_IN\n"); > > What can userspace do with this error? Will it be repeated a lot? > If driver is okay to read CCI but failed to read MESSAGE_IN, it's an critical error in CCGx and in this situation, this log could repeat a lot. We don't seem to have a good way to help this situation from userspace. In our case, the failure on ccg_read() is mostly because of the I2C issue. When there is an I2C issue between CCGx & Tegra, the failure is more likely happening in the earlier stage (e.g., INTR read or CCI read). Please let me know if you have more questions and concerns. > thanks, > > greg k-h >
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c index eab3012e1b01..b35a3a97c9fb 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 { + u32 cci; + u32 message_in[CCGX_MESSAGE_IN_MAX]; +}; + struct ucsi_ccg { struct device *dev; struct ucsi *ucsi; @@ -222,6 +228,9 @@ struct ucsi_ccg { bool has_multiple_dp; struct ucsi_ccg_altmode orig[UCSI_MAX_ALTMODES]; struct ucsi_ccg_altmode updated[UCSI_MAX_ALTMODES]; + + 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 +314,57 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, const u8 *data, u32 len) return 0; } +static void ccg_op_region_read(struct ucsi_ccg *uc, unsigned int offset, + void *val, size_t val_len) +{ + struct op_region *data = &uc->op_data; + + spin_lock(&uc->op_lock); + if (offset == UCSI_CCI) + memcpy(val, &data->cci, val_len); + else if (offset == UCSI_MESSAGE_IN) + memcpy(val, &data->message_in, val_len); + spin_unlock(&uc->op_lock); +} + +static void 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; + u32 message_in[CCGX_MESSAGE_IN_MAX]; + + if (UCSI_CCI_LENGTH(cci)) + if (ccg_read(uc, reg, (void *)&message_in, + sizeof(message_in))) { + dev_err(uc->dev, "failed to read MESSAGE_IN\n"); + return; + } + + spin_lock(&uc->op_lock); + memcpy(&data->cci, &cci, sizeof(cci)); + if (UCSI_CCI_LENGTH(cci)) + memcpy(&data->message_in, &message_in, sizeof(message_in)); + spin_unlock(&uc->op_lock); +} + +static void ccg_op_region_clean(struct ucsi_ccg *uc) +{ + struct op_region *data = &uc->op_data; + + spin_lock(&uc->op_lock); + memset(&data->cci, 0, sizeof(data->cci)); + memset(&data->message_in, 0, sizeof(data->message_in)); + spin_unlock(&uc->op_lock); +} + 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 +574,13 @@ 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) || (offset == UCSI_MESSAGE_IN)) + ccg_op_region_read(uc, offset, val, val_len); + else + ret = ccg_read(uc, reg, val, val_len); - ret = ccg_read(uc, reg, val, val_len); if (ret) return ret; @@ -559,9 +617,13 @@ 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); + if (offset == UCSI_CONTROL) + ccg_op_region_clean(uc); + + return ccg_write(uc, reg, val, val_len); } static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset, @@ -616,12 +678,17 @@ static irqreturn_t ccg_irq_handler(int irq, void *data) struct ucsi_ccg *uc = data; u8 intr_reg; u32 cci; - int ret; + 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 +696,18 @@ 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 + */ + ccg_op_region_update(uc, cci); 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; }