| Message ID | 20230504-b4-v6-3-topic-boards-imx8mp-evk-dual-role-usb-v2-0-3889b1b2050c@pengutronix.de |
|---|---|
| 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
On Thu, May 04, 2023 at 03:46:49PM +0200, Marco Felsch wrote: > Hi all, > > this adds the usb gadget support to the i.MX8MP-EVK. This Series is > based on [1] and therefore it is already a v2. Thanks to Li and Andreas > for the very useful feedback. > > Patch1-3: Add the mssing support for USB-SS GPIO muxes. This is required > to have proper USB-SS support on the EVK. > > Patch4: Adds the devicetree integration. Please send the DTS change separately afterwards, as we do not want Greg's tool to pick it up into USB tree. Shawn > > [1] https://lore.kernel.org/all/20230323105826.2058003-1-m.felsch@pengutronix.de/ > > Regards, > Marco > > --- > Marco Felsch (4): > dt-bindings: usb: gpio-sbu-mux: add support for ss-data lanes mux > usb: typec: mux: gpio-sbu-mux: add support for ss data lane muxing > usb: typec: tcpci: clear the fault status bit > arm64: dts: imx8mp-evk: add dual-role usb port1 support > > .../devicetree/bindings/usb/gpio-sbu-mux.yaml | 82 +++++++++++++++++--- > arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 88 ++++++++++++++++++++++ > drivers/usb/typec/mux/Kconfig | 5 +- > drivers/usb/typec/mux/gpio-sbu-mux.c | 18 ++++- > drivers/usb/typec/tcpm/tcpci.c | 5 ++ > include/linux/usb/tcpci.h | 1 + > 6 files changed, 185 insertions(+), 14 deletions(-) > --- > base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4 > change-id: 20230504-b4-v6-3-topic-boards-imx8mp-evk-dual-role-usb-8dcf6274d9df > > Best regards, > -- > Marco Felsch <m.felsch@pengutronix.de> >
On Sun, May 14, 2023 at 09:21:22PM +0800, Shawn Guo wrote: > On Thu, May 04, 2023 at 03:46:49PM +0200, Marco Felsch wrote: > > Hi all, > > > > this adds the usb gadget support to the i.MX8MP-EVK. This Series is > > based on [1] and therefore it is already a v2. Thanks to Li and Andreas > > for the very useful feedback. > > > > Patch1-3: Add the mssing support for USB-SS GPIO muxes. This is required > > to have proper USB-SS support on the EVK. > > > > Patch4: Adds the devicetree integration. > > Please send the DTS change separately afterwards, as we do not want > Greg's tool to pick it up into USB tree. "Greg's tool" is the standard `b4` tool that lots of kernel maintainers are now using. So this isn't some magic thing that is unique to me... thanks, greg k-h
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
Hi all, this adds the usb gadget support to the i.MX8MP-EVK. This Series is based on [1] and therefore it is already a v2. Thanks to Li and Andreas for the very useful feedback. Patch1-3: Add the mssing support for USB-SS GPIO muxes. This is required to have proper USB-SS support on the EVK. Patch4: Adds the devicetree integration. [1] https://lore.kernel.org/all/20230323105826.2058003-1-m.felsch@pengutronix.de/ Regards, Marco --- Marco Felsch (4): dt-bindings: usb: gpio-sbu-mux: add support for ss-data lanes mux usb: typec: mux: gpio-sbu-mux: add support for ss data lane muxing usb: typec: tcpci: clear the fault status bit arm64: dts: imx8mp-evk: add dual-role usb port1 support .../devicetree/bindings/usb/gpio-sbu-mux.yaml | 82 +++++++++++++++++--- arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 88 ++++++++++++++++++++++ drivers/usb/typec/mux/Kconfig | 5 +- drivers/usb/typec/mux/gpio-sbu-mux.c | 18 ++++- drivers/usb/typec/tcpm/tcpci.c | 5 ++ include/linux/usb/tcpci.h | 1 + 6 files changed, 185 insertions(+), 14 deletions(-) --- base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4 change-id: 20230504-b4-v6-3-topic-boards-imx8mp-evk-dual-role-usb-8dcf6274d9df Best regards,