| Message ID | 20240328-chipidea-phy-misc-v1-1-907d9de5d4df@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 22ffd399e6e7aa18ae0314278ed0b7f05f8ab679 |
| Headers | show |
| Series | usb: chipidea: move ci_ulpi_init after the phy initialization | expand |
On 24-04-02 08:23:43, Michael Grzeschik wrote: > The function ci_usb_phy_init is already handling the > hw_phymode_configure path which is also only possible after we have > a valid phy. So we move the ci_ulpi_init after the phy initialization > to be really sure to be able to communicate with the ulpi phy. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Acked-by: Peter Chen <peter.chen@kernel.org> > --- > drivers/usb/chipidea/core.c | 8 ++++---- > drivers/usb/chipidea/ulpi.c | 5 ----- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 835bf2428dc6e..bada13f704b62 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - ret = ci_ulpi_init(ci); > - if (ret) > - return ret; > - > if (ci->platdata->phy) { > ci->phy = ci->platdata->phy; > } else if (ci->platdata->usb_phy) { > @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev) > goto ulpi_exit; > } > > + ret = ci_ulpi_init(ci); > + if (ret) > + return ret; > + > ci->hw_bank.phys = res->start; > > ci->irq = platform_get_irq(pdev, 0); > diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c > index dfec07e8ae1d2..89fb51e2c3ded 100644 > --- a/drivers/usb/chipidea/ulpi.c > +++ b/drivers/usb/chipidea/ulpi.c > @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci) > if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) > return 0; > > - /* > - * Set PORTSC correctly so we can read/write ULPI registers for > - * identification purposes > - */ > - hw_phymode_configure(ci); > > ci->ulpi_ops.read = ci_ulpi_read; > ci->ulpi_ops.write = ci_ulpi_write; > > --- > base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848 > change-id: 20240328-chipidea-phy-misc-b3f2bc814784 > > Best regards, > -- > Michael Grzeschik <m.grzeschik@pengutronix.de> >
Dear All, On 02.04.2024 08:23, Michael Grzeschik wrote: > The function ci_usb_phy_init is already handling the > hw_phymode_configure path which is also only possible after we have > a valid phy. So we move the ci_ulpi_init after the phy initialization > to be really sure to be able to communicate with the ulpi phy. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> This patch landed in linux-next some time ago as commit 22ffd399e6e7 ("usb: chipidea: move ci_ulpi_init after the phy initialization"). Unfortunately it breaks host USB operation on DragonBoard410c (arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor warning in the kernel log besides the information about deferred probe on the chipidea controller: platform ci_hdrc.0: deferred probe pending: (reason unknown) Tomorrow I will try to investigate which operation during driver probe triggers it. If there is anything else to check that might help fixing this issue, let me know. > --- > drivers/usb/chipidea/core.c | 8 ++++---- > drivers/usb/chipidea/ulpi.c | 5 ----- > 2 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 835bf2428dc6e..bada13f704b62 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - ret = ci_ulpi_init(ci); > - if (ret) > - return ret; > - > if (ci->platdata->phy) { > ci->phy = ci->platdata->phy; > } else if (ci->platdata->usb_phy) { > @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev) > goto ulpi_exit; > } > > + ret = ci_ulpi_init(ci); > + if (ret) > + return ret; > + > ci->hw_bank.phys = res->start; > > ci->irq = platform_get_irq(pdev, 0); > diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c > index dfec07e8ae1d2..89fb51e2c3ded 100644 > --- a/drivers/usb/chipidea/ulpi.c > +++ b/drivers/usb/chipidea/ulpi.c > @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci) > if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) > return 0; > > - /* > - * Set PORTSC correctly so we can read/write ULPI registers for > - * identification purposes > - */ > - hw_phymode_configure(ci); > > ci->ulpi_ops.read = ci_ulpi_read; > ci->ulpi_ops.write = ci_ulpi_write; > > --- > base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848 > change-id: 20240328-chipidea-phy-misc-b3f2bc814784 > > Best regards, Best regards
On 2024-04-25 15:40, Marek Szyprowski wrote: > Dear All, > > On 02.04.2024 08:23, Michael Grzeschik wrote: >> The function ci_usb_phy_init is already handling the >> hw_phymode_configure path which is also only possible after we have >> a valid phy. So we move the ci_ulpi_init after the phy initialization >> to be really sure to be able to communicate with the ulpi phy. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > This patch landed in linux-next some time ago as commit 22ffd399e6e7 > ("usb: chipidea: move ci_ulpi_init after the phy initialization"). > Unfortunately it breaks host USB operation on DragonBoard410c > (arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor > warning in the kernel log besides the information about deferred probe > on the chipidea controller: > > platform ci_hdrc.0: deferred probe pending: (reason unknown) > > Tomorrow I will try to investigate which operation during driver probe > triggers it. If there is anything else to check that might help fixing > this issue, let me know. I am also seeing this issue on a msm8974-based device. There is also a report and analysis by Wouter Franken here: https://lore.kernel.org/all/ecb8d3e8-d525-4a2d-a868-803202c16296@franken-peeters.be/ Reverting the commit does fix the issue for me. Adding to regzbot for tracking: #regzbot introduced: 22ffd399e6e7 ^ Thanks, Alexandre > >> --- >> drivers/usb/chipidea/core.c | 8 ++++---- >> drivers/usb/chipidea/ulpi.c | 5 ----- >> 2 files changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c >> index 835bf2428dc6e..bada13f704b62 100644 >> --- a/drivers/usb/chipidea/core.c >> +++ b/drivers/usb/chipidea/core.c >> @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> - ret = ci_ulpi_init(ci); >> - if (ret) >> - return ret; >> - >> if (ci->platdata->phy) { >> ci->phy = ci->platdata->phy; >> } else if (ci->platdata->usb_phy) { >> @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev) >> goto ulpi_exit; >> } >> >> + ret = ci_ulpi_init(ci); >> + if (ret) >> + return ret; >> + >> ci->hw_bank.phys = res->start; >> >> ci->irq = platform_get_irq(pdev, 0); >> diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c >> index dfec07e8ae1d2..89fb51e2c3ded 100644 >> --- a/drivers/usb/chipidea/ulpi.c >> +++ b/drivers/usb/chipidea/ulpi.c >> @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci) >> if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) >> return 0; >> >> - /* >> - * Set PORTSC correctly so we can read/write ULPI registers for >> - * identification purposes >> - */ >> - hw_phymode_configure(ci); >> >> ci->ulpi_ops.read = ci_ulpi_read; >> ci->ulpi_ops.write = ci_ulpi_write; >> >> --- >> base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848 >> change-id: 20240328-chipidea-phy-misc-b3f2bc814784 >> >> Best regards, > > Best regards
On 30.05.24 08:38, Alexandre Messier wrote: > On 2024-04-25 15:40, Marek Szyprowski wrote: >> On 02.04.2024 08:23, Michael Grzeschik wrote: >>> The function ci_usb_phy_init is already handling the >>> hw_phymode_configure path which is also only possible after we have >>> a valid phy. So we move the ci_ulpi_init after the phy initialization >>> to be really sure to be able to communicate with the ulpi phy. >>> >>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> >> This patch landed in linux-next some time ago as commit 22ffd399e6e7 >> ("usb: chipidea: move ci_ulpi_init after the phy initialization"). >> Unfortunately it breaks host USB operation on DragonBoard410c >> (arch/arm64/boot/dts/qcom/apq8016-sbc.dts). There is no error nor >> warning in the kernel log besides the information about deferred probe >> on the chipidea controller: > > I am also seeing this issue on a msm8974-based device. > > There is also a report and analysis by Wouter Franken here: > https://lore.kernel.org/all/ecb8d3e8-d525-4a2d-a868-803202c16296@franken-peeters.be/ TWIMC, as this is easy to miss (I was extremely close to writing a status inquiry instead of writing what I'm typing now): Greg applied that revert two days ago and I assume he'll soon send it to Linus. Ciao, Thorsten #regzbot fix: 718d4a63c0a62d16af1d0425d515d7e76f35681e #regzbot ignore-activity
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 835bf2428dc6e..bada13f704b62 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -1084,10 +1084,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) return -ENODEV; } - ret = ci_ulpi_init(ci); - if (ret) - return ret; - if (ci->platdata->phy) { ci->phy = ci->platdata->phy; } else if (ci->platdata->usb_phy) { @@ -1142,6 +1138,10 @@ static int ci_hdrc_probe(struct platform_device *pdev) goto ulpi_exit; } + ret = ci_ulpi_init(ci); + if (ret) + return ret; + ci->hw_bank.phys = res->start; ci->irq = platform_get_irq(pdev, 0); diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c index dfec07e8ae1d2..89fb51e2c3ded 100644 --- a/drivers/usb/chipidea/ulpi.c +++ b/drivers/usb/chipidea/ulpi.c @@ -68,11 +68,6 @@ int ci_ulpi_init(struct ci_hdrc *ci) if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) return 0; - /* - * Set PORTSC correctly so we can read/write ULPI registers for - * identification purposes - */ - hw_phymode_configure(ci); ci->ulpi_ops.read = ci_ulpi_read; ci->ulpi_ops.write = ci_ulpi_write;
The function ci_usb_phy_init is already handling the hw_phymode_configure path which is also only possible after we have a valid phy. So we move the ci_ulpi_init after the phy initialization to be really sure to be able to communicate with the ulpi phy. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/usb/chipidea/core.c | 8 ++++---- drivers/usb/chipidea/ulpi.c | 5 ----- 2 files changed, 4 insertions(+), 9 deletions(-) --- base-commit: 5bab5dc780c9ed0c69fc2f828015532acf4a7848 change-id: 20240328-chipidea-phy-misc-b3f2bc814784 Best regards,