| Message ID | 20230504-b4-v6-3-topic-boards-imx8mp-evk-dual-role-usb-v2-3-3889b1b2050c@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add i.MX8MP-EVK USB Gadget Support | expand |
On 5/4/23 06:46, Marco Felsch wrote: > According the "USB Type-C Port Controller Interface Specification v2.0" > the TCPC sets the fault status register bit-7 > (AllRegistersResetToDefault) once the registers have been reseted to cleared ? set ? > their default values. > > This triggers an alert(-irq) on PTN5110 devices albeit we do mask the > fault-irq. Fix this gernally by writing a one to the correspondig generically ? corresponding > bit-7. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/usb/typec/tcpm/tcpci.c | 5 +++++ > include/linux/usb/tcpci.h | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c > index 8da23240afbe..15632d023e4c 100644 > --- a/drivers/usb/typec/tcpm/tcpci.c > +++ b/drivers/usb/typec/tcpm/tcpci.c > @@ -602,6 +602,11 @@ static int tcpci_init(struct tcpc_dev *tcpc) > if (time_after(jiffies, timeout)) > return -ETIMEDOUT; > > + regmap_read(tcpci->regmap, TCPC_FAULT_STATUS, ®); Needs error check. Also, I am not sure if this is the correct place for this code. The alert status is cleared after vendor initialization. Should the same be done here ? Also, why not just write the bit unconditionally, similar to TCPC_ALERT ? Thanks, Guenter > + if (reg & TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT) > + tcpci_write16(tcpci, TCPC_FAULT_STATUS, > + TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT); > + > /* Handle vendor init */ > if (tcpci->data->init) { > ret = tcpci->data->init(tcpci, tcpci->data); > diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h > index 85e95a3251d3..83376473ac76 100644 > --- a/include/linux/usb/tcpci.h > +++ b/include/linux/usb/tcpci.h > @@ -103,6 +103,7 @@ > #define TCPC_POWER_STATUS_SINKING_VBUS BIT(0) > > #define TCPC_FAULT_STATUS 0x1f > +#define TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT BIT(7) > > #define TCPC_ALERT_EXTENDED 0x21 > >
On 23-05-04, Guenter Roeck wrote: > On 5/4/23 06:46, Marco Felsch wrote: > > According the "USB Type-C Port Controller Interface Specification v2.0" > > the TCPC sets the fault status register bit-7 > > (AllRegistersResetToDefault) once the registers have been reseted to > > cleared ? set ? Sry. I don't get this. > > their default values. > > > > This triggers an alert(-irq) on PTN5110 devices albeit we do mask the > > fault-irq. Fix this gernally by writing a one to the correspondig > > generically ? Sure, thanks. > corresponding Of course! > > bit-7. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/usb/typec/tcpm/tcpci.c | 5 +++++ > > include/linux/usb/tcpci.h | 1 + > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c > > index 8da23240afbe..15632d023e4c 100644 > > --- a/drivers/usb/typec/tcpm/tcpci.c > > +++ b/drivers/usb/typec/tcpm/tcpci.c > > @@ -602,6 +602,11 @@ static int tcpci_init(struct tcpc_dev *tcpc) > > if (time_after(jiffies, timeout)) > > return -ETIMEDOUT; > > + regmap_read(tcpci->regmap, TCPC_FAULT_STATUS, ®); > > Needs error check. I will add this. > Also, I am not sure if this is the correct place for this code. The alert > status is cleared after vendor initialization. Should the same be done > here ? According the spec the bit must be cleared before the TCPC_ALERT is cleared. Of course the vendor-init can (re-)trigger the bit, therefore we should move behind the vendor init and right before the TCPC_ALERT clear. > Also, why not just write the bit unconditionally, similar > to TCPC_ALERT ? Thought about this too.. I will change it in the v3. Thanks for the feedback, Marco > > Thanks, > Guenter > > > + if (reg & TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT) > > + tcpci_write16(tcpci, TCPC_FAULT_STATUS, > > + TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT); > > + > > /* Handle vendor init */ > > if (tcpci->data->init) { > > ret = tcpci->data->init(tcpci, tcpci->data); > > diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h > > index 85e95a3251d3..83376473ac76 100644 > > --- a/include/linux/usb/tcpci.h > > +++ b/include/linux/usb/tcpci.h > > @@ -103,6 +103,7 @@ > > #define TCPC_POWER_STATUS_SINKING_VBUS BIT(0) > > #define TCPC_FAULT_STATUS 0x1f > > +#define TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT BIT(7) > > #define TCPC_ALERT_EXTENDED 0x21 > > > >
On 5/4/23 07:27, Marco Felsch wrote: > On 23-05-04, Guenter Roeck wrote: >> On 5/4/23 06:46, Marco Felsch wrote: >>> According the "USB Type-C Port Controller Interface Specification v2.0" >>> the TCPC sets the fault status register bit-7 >>> (AllRegistersResetToDefault) once the registers have been reseted to >> >> cleared ? set ? > > Sry. I don't get this. > instead of "reseted" which isn't really a word. >>> their default values. >>> >>> This triggers an alert(-irq) on PTN5110 devices albeit we do mask the >>> fault-irq. Fix this gernally by writing a one to the correspondig >> >> generically ? > > Sure, thanks. > >> corresponding > > Of course! > >>> bit-7. >>> >>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> >>> --- >>> drivers/usb/typec/tcpm/tcpci.c | 5 +++++ >>> include/linux/usb/tcpci.h | 1 + >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c >>> index 8da23240afbe..15632d023e4c 100644 >>> --- a/drivers/usb/typec/tcpm/tcpci.c >>> +++ b/drivers/usb/typec/tcpm/tcpci.c >>> @@ -602,6 +602,11 @@ static int tcpci_init(struct tcpc_dev *tcpc) >>> if (time_after(jiffies, timeout)) >>> return -ETIMEDOUT; >>> + regmap_read(tcpci->regmap, TCPC_FAULT_STATUS, ®); >> >> Needs error check. > > I will add this. > >> Also, I am not sure if this is the correct place for this code. The alert >> status is cleared after vendor initialization. Should the same be done >> here ? > > According the spec the bit must be cleared before the TCPC_ALERT is > cleared. Of course the vendor-init can (re-)trigger the bit, therefore > we should move behind the vendor init and right before the TCPC_ALERT > clear. > >> Also, why not just write the bit unconditionally, similar >> to TCPC_ALERT ? > > Thought about this too.. I will change it in the v3. > > Thanks for the feedback, > Marco > >> >> Thanks, >> Guenter >> >>> + if (reg & TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT) >>> + tcpci_write16(tcpci, TCPC_FAULT_STATUS, >>> + TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT); >>> + >>> /* Handle vendor init */ >>> if (tcpci->data->init) { >>> ret = tcpci->data->init(tcpci, tcpci->data); >>> diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h >>> index 85e95a3251d3..83376473ac76 100644 >>> --- a/include/linux/usb/tcpci.h >>> +++ b/include/linux/usb/tcpci.h >>> @@ -103,6 +103,7 @@ >>> #define TCPC_POWER_STATUS_SINKING_VBUS BIT(0) >>> #define TCPC_FAULT_STATUS 0x1f >>> +#define TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT BIT(7) >>> #define TCPC_ALERT_EXTENDED 0x21 >>> >> >>
On 23-05-04, Guenter Roeck wrote: > On 5/4/23 07:27, Marco Felsch wrote: > > On 23-05-04, Guenter Roeck wrote: > > > On 5/4/23 06:46, Marco Felsch wrote: > > > > According the "USB Type-C Port Controller Interface Specification v2.0" > > > > the TCPC sets the fault status register bit-7 > > > > (AllRegistersResetToDefault) once the registers have been reseted to > > > > > > cleared ? set ? > > > > Sry. I don't get this. > > > > instead of "reseted" which isn't really a word. Sure, thanks. Regards, Marco
Hi Marco, On Thu, May 4, 2023 at 11:27 AM Marco Felsch <m.felsch@pengutronix.de> wrote: > > Also, why not just write the bit unconditionally, similar > > to TCPC_ALERT ? > > Thought about this too.. I will change it in the v3. Was there ever a v3 for this patch? Thanks
On 23-08-16, Fabio Estevam wrote: > Hi Marco, > > On Thu, May 4, 2023 at 11:27 AM Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > Also, why not just write the bit unconditionally, similar > > > to TCPC_ALERT ? > > > > Thought about this too.. I will change it in the v3. > > Was there ever a v3 for this patch? Nope, thanks for sending it separate :) Regards, Marco
diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c index 8da23240afbe..15632d023e4c 100644 --- a/drivers/usb/typec/tcpm/tcpci.c +++ b/drivers/usb/typec/tcpm/tcpci.c @@ -602,6 +602,11 @@ static int tcpci_init(struct tcpc_dev *tcpc) if (time_after(jiffies, timeout)) return -ETIMEDOUT; + regmap_read(tcpci->regmap, TCPC_FAULT_STATUS, ®); + if (reg & TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT) + tcpci_write16(tcpci, TCPC_FAULT_STATUS, + TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT); + /* Handle vendor init */ if (tcpci->data->init) { ret = tcpci->data->init(tcpci, tcpci->data); diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h index 85e95a3251d3..83376473ac76 100644 --- a/include/linux/usb/tcpci.h +++ b/include/linux/usb/tcpci.h @@ -103,6 +103,7 @@ #define TCPC_POWER_STATUS_SINKING_VBUS BIT(0) #define TCPC_FAULT_STATUS 0x1f +#define TCPC_FAULT_STATUS_ALL_REG_RST_TO_DEFAULT BIT(7) #define TCPC_ALERT_EXTENDED 0x21
According the "USB Type-C Port Controller Interface Specification v2.0" the TCPC sets the fault status register bit-7 (AllRegistersResetToDefault) once the registers have been reseted to their default values. This triggers an alert(-irq) on PTN5110 devices albeit we do mask the fault-irq. Fix this gernally by writing a one to the correspondig bit-7. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/usb/typec/tcpm/tcpci.c | 5 +++++ include/linux/usb/tcpci.h | 1 + 2 files changed, 6 insertions(+)