| Message ID | 20191228203358.23490-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | NVIDIA Tegra USB2 drivers clean up | expand |
On 19-12-28 23:33:56, Dmitry Osipenko wrote: > Tegra's USB PHY driver now provides generic PHY init/shutdown callbacks > and thus the custom PHY management could be removed from Tegra-specific > part of the ChipIdea driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/usb/chipidea/ci_hdrc_tegra.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c > index 0c9911d44ee5..7455df0ede49 100644 > --- a/drivers/usb/chipidea/ci_hdrc_tegra.c > +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c > @@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev) > return err; > } > > - /* > - * Tegra's USB PHY driver doesn't implement optional phy_init() > - * hook, so we have to power on UDC controller before ChipIdea > - * driver initialization kicks in. > - */ > - usb_phy_set_suspend(udc->phy, 0); > - > /* setup and register ChipIdea HDRC device */ > udc->data.name = "tegra-udc"; > udc->data.flags = soc->flags; > @@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev) > return 0; > > fail_power_off: > - usb_phy_set_suspend(udc->phy, 1); > clk_disable_unprepare(udc->clk); > return err; > } > @@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev) > struct tegra_udc *udc = platform_get_drvdata(pdev); > > ci_hdrc_remove_device(udc->dev); > - usb_phy_set_suspend(udc->phy, 1); > clk_disable_unprepare(udc->clk); > > return 0; > -- > 2.24.0 > Acked-by: Peter Chen <peter.chen@nxp.com>
On 19-12-28 23:33:57, Dmitry Osipenko wrote: > Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb module > is loaded too regardless of kernel's configuration. Previously this > problem was masked because Tegra's EHCI driver is usually enabled in > kernel's config and thus PHY driver was getting loaded because of it, but > now I was making some more thorough testing and noticed that PHY's module > isn't getting auto-loaded without the host driver. > > Note that ChipIdea's driver doesn't use any of the exported functions of > phy_tegra_usb module and thus the module needs to be requested explicitly. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/usb/chipidea/Kconfig | 1 + > drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig > index ae850b3fddf2..d53db520e209 100644 > --- a/drivers/usb/chipidea/Kconfig > +++ b/drivers/usb/chipidea/Kconfig > @@ -7,6 +7,7 @@ config USB_CHIPIDEA > select RESET_CONTROLLER > select USB_ULPI_BUS > select USB_ROLE_SWITCH > + select USB_TEGRA_PHY if ARCH_TEGRA > help > Say Y here if your system has a dual role high speed USB > controller based on ChipIdea silicon IP. It supports: > diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c > index 7455df0ede49..8bc11100245d 100644 > --- a/drivers/usb/chipidea/ci_hdrc_tegra.c > +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c > @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev) > struct tegra_udc *udc; > int err; > > + if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) { > + err = request_module("phy_tegra_usb"); > + if (err) > + return err; > + } > + > udc = devm_kzalloc(&pdev->dev, sizeof(*udc), GFP_KERNEL); > if (!udc) > return -ENOMEM; > -- > 2.24.0 > Acked-by: Peter Chen <peter.chen@nxp.com>
On Sat, Dec 28, 2019 at 11:33:55PM +0300, Dmitry Osipenko wrote: > There is no good reason for disabling of CPU interrupts in order to > protect the utmip_pad_count modification. Since there are sleeping functions called outside of the locked sections, this should be a mutex instead. OTOH if the spin_lock is to protect register write against IRQ handler, then the patch is wrong. [...] > - spin_unlock_irqrestore(&utmip_pad_lock, flags); > + spin_unlock(&utmip_pad_lock); > > clk_disable_unprepare(phy->pad_clk); Best Regards, Michał Mirosław
30.12.2019 23:36, Michał Mirosław пишет: > On Sat, Dec 28, 2019 at 11:33:55PM +0300, Dmitry Osipenko wrote: >> There is no good reason for disabling of CPU interrupts in order to >> protect the utmip_pad_count modification. > > Since there are sleeping functions called outside of the locked sections, > this should be a mutex instead. OTOH if the spin_lock is to protect register > write against IRQ handler, then the patch is wrong. > > [...] >> - spin_unlock_irqrestore(&utmip_pad_lock, flags); >> + spin_unlock(&utmip_pad_lock); >> >> clk_disable_unprepare(phy->pad_clk); Hello Michał, This spinlock isn't for protecting from the IRQ handler, it's used solely to protect modification of the registers that are shared by all USB controllers. It's possible to use mutex instead of spinlock here, but it doesn't bring any benefits because mutex is more useful when protected code could block for a long time due to sleep or whatever, while spinlock is much more efficient when protected code doesn't sleep and takes no more than dozens microseconds to execute (which is the case here). In this particular case of the Tegra USB PHY driver, the chance of getting a block on taking the utmip_pad_lock is zero unless USB controller drivers will start to use asynchronous probing. So this patch does a very minor clean-up change and nothing more.
On Thu, Jan 02, 2020 at 05:40:48PM +0300, Dmitry Osipenko wrote: > 30.12.2019 23:36, Michał Mirosław пишет: > > On Sat, Dec 28, 2019 at 11:33:55PM +0300, Dmitry Osipenko wrote: > >> There is no good reason for disabling of CPU interrupts in order to > >> protect the utmip_pad_count modification. > > > > Since there are sleeping functions called outside of the locked sections, > > this should be a mutex instead. OTOH if the spin_lock is to protect register > > write against IRQ handler, then the patch is wrong. > > > > [...] > >> - spin_unlock_irqrestore(&utmip_pad_lock, flags); > >> + spin_unlock(&utmip_pad_lock); > >> > >> clk_disable_unprepare(phy->pad_clk); > > Hello Michał, > > This spinlock isn't for protecting from the IRQ handler, it's used > solely to protect modification of the registers that are shared by all > USB controllers. > > It's possible to use mutex instead of spinlock here, but it doesn't > bring any benefits because mutex is more useful when protected code > could block for a long time due to sleep or whatever, while spinlock is > much more efficient when protected code doesn't sleep and takes no more > than dozens microseconds to execute (which is the case here). > > In this particular case of the Tegra USB PHY driver, the chance of > getting a block on taking the utmip_pad_lock is zero unless USB > controller drivers will start to use asynchronous probing. So this patch > does a very minor clean-up change and nothing more. I was concerned that this change allows the kernel to switch away to another task, but I can see now that spin_lock() does disable preemtion. So it looks OK after all. Would be nice to see the explanation in the commit message (that the spinlock is only used to serialize probe()s). Best Regards, Michał Mirosław
On Sat, Dec 28, 2019 at 11:33:52PM +0300, Dmitry Osipenko wrote: [...] > static int ulpi_open(struct tegra_usb_phy *phy) > { > - int err; > - > - err = gpio_direction_output(phy->reset_gpio, 0); > - if (err) { > - dev_err(phy->u_phy.dev, > - "ULPI reset GPIO %d direction not deasserted: %d\n", > - phy->reset_gpio, err); > - return err; > - } > + gpiod_set_value_cansleep(phy->reset_gpio, 1); > > return 0; > } The message now removed seems inverted to the meaning of the code. Is this a bug, or the reset really should be asserted here? I can see that it is deasserted in phy_power_up, but that goes before or after ulpi_open()? After the change below, the reset is asserted at probe() time now. [...] > - err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio, > - "ulpi_phy_reset_b"); > + gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np, > + "nvidia,phy-reset-gpio", > + 0, GPIOD_OUT_HIGH, > + "ulpi_phy_reset_b"); > + err = PTR_ERR_OR_ZERO(gpiod); > if (err) { > - dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n", > - tegra_phy->reset_gpio, err); > + dev_err(&pdev->dev, > + "Request failed for reset GPIO: %d\n", err); > return err; > } > + tegra_phy->reset_gpio = gpiod; A nice extension to kernel's printf - "%pe" format - has just landed in Linus' master tree. Best Regards, Michał Mirosław
03.01.2020 10:58, Michał Mirosław пишет: > On Sat, Dec 28, 2019 at 11:33:52PM +0300, Dmitry Osipenko wrote: > [...] >> static int ulpi_open(struct tegra_usb_phy *phy) >> { >> - int err; >> - >> - err = gpio_direction_output(phy->reset_gpio, 0); >> - if (err) { >> - dev_err(phy->u_phy.dev, >> - "ULPI reset GPIO %d direction not deasserted: %d\n", >> - phy->reset_gpio, err); >> - return err; >> - } >> + gpiod_set_value_cansleep(phy->reset_gpio, 1); >> >> return 0; >> } > > The message now removed seems inverted to the meaning of the code. Is > this a bug, or the reset really should be asserted here? The removed message was added in patch #2 and indeed it should say "asserted". Good catch, thanks! > I can see that > it is deasserted in phy_power_up, but that goes before or after ulpi_open()? The ulpi_phy_power_on happens after the ulpi_open, please take a look at tegra_usb_phy_init(). > After the change below, the reset is asserted at probe() time now. Yes, the probe now asserts the reset. It is an intended change because it should be a bit better to explicitly per-initialize the GPIO state to an expected state during of the GPIO retrieval, like most of other drivers do and which should be a "generic/common way". Actually, the reset assertion of ulpi_open() could be removed safely now since it doesn't do anything useful, given that probe asserts the reset. > [...] >> - err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio, >> - "ulpi_phy_reset_b"); >> + gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np, >> + "nvidia,phy-reset-gpio", >> + 0, GPIOD_OUT_HIGH, >> + "ulpi_phy_reset_b"); >> + err = PTR_ERR_OR_ZERO(gpiod); >> if (err) { >> - dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n", >> - tegra_phy->reset_gpio, err); >> + dev_err(&pdev->dev, >> + "Request failed for reset GPIO: %d\n", err); >> return err; >> } >> + tegra_phy->reset_gpio = gpiod; > > A nice extension to kernel's printf - "%pe" format - has just landed in > Linus' master tree. Thank you very much, I didn't know about that. I'll prepare v4 with the above things addressed, thank you again and please let me know if you'll spot anything else!
04.01.2020 02:53, Dmitry Osipenko пишет: > 03.01.2020 10:58, Michał Mirosław пишет: [snip] >> [...] >>> - err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio, >>> - "ulpi_phy_reset_b"); >>> + gpiod = devm_gpiod_get_from_of_node(&pdev->dev, np, >>> + "nvidia,phy-reset-gpio", >>> + 0, GPIOD_OUT_HIGH, >>> + "ulpi_phy_reset_b"); >>> + err = PTR_ERR_OR_ZERO(gpiod); >>> if (err) { >>> - dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n", >>> - tegra_phy->reset_gpio, err); >>> + dev_err(&pdev->dev, >>> + "Request failed for reset GPIO: %d\n", err); >>> return err; >>> } >>> + tegra_phy->reset_gpio = gpiod; >> >> A nice extension to kernel's printf - "%pe" format - has just landed in >> Linus' master tree. > > Thank you very much, I didn't know about that. In this particular case PTR_ERR_OR_ZERO() results in a bit more cleaner code than with IS_ERR() and PTR_ERR() [IMHO], so I'll probably keep it as-is. Anyways, thanks again for the pointer to "%pe", it could come handy later on.
Hello, This patch series brings the NVIDIA Tegra USB2 PHY driver into a better shape by refactoring code to match upstream standards, the ChipIdea/Tegra UDC driver also gets a minor update. Please review and apply, thanks in advance! Changelog: v3: - The "Perform general clean up of the code" patch now cleans up couple more minor crumbs. - Added more "clean up" patches: usb: phy: tegra: Disable VBUS regulator on tegra_usb_phy_init failure usb: phy: tegra: Move utmip_pad_count checking under lock usb: phy: tegra: Keep CPU interrupts enabled usb: host: ehci-tegra: Remove unused fields from tegra_ehci_hcd - I noticed that ULPI's reset is getting erroneously deasserted on PHY's shutdown, this is fixed in these new patches: usb: phy: tegra: Assert reset on ULPI close instead of deasserting it usb: phy: tegra: Use device-tree notion of reset-GPIO's active-state v2: - The "usb: phy: tegra: Hook up init/shutdown callbacks" patch was updated and now it does a better job in regards to checking whether PHY is initialized before it is started to be used and whether there is a double init/shutdown. This allows to factor out the ChipIdea's driver change into a separate patch, which was requested by Peter Chen in a review comment to v1. In a result there is this new patch: usb: chipidea: tegra: Stop managing PHY's power - Added few more new patches: usb: phy: tegra: Use generic stub for a missing VBUS regulator I noticed that VBUS regulator usage could be cleaned up a tad as well. usb: ulpi: Add resource-managed variant of otg_ulpi_create() usb: phy: tegra: Use devm_otg_ulpi_create() usb: phy: tegra: Use u32 for hardware register variables These patches are made in response to review comments that were made by Thierry Reding to v1. I also noticed that phy_tegra_usb isn't getting auto-loaded while it should be. This is fixed in this new patch: usb: chipidea: tegra: Add USB_TEGRA_PHY to driver's dependencies Dmitry Osipenko (16): dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support usb: phy: tegra: Hook up init/shutdown callbacks usb: phy: tegra: Perform general clean up of the code usb: phy: tegra: Use relaxed versions of readl/writel usb: phy: tegra: Use generic stub for a missing VBUS regulator usb: ulpi: Add resource-managed variant of otg_ulpi_create() usb: phy: tegra: Use devm_otg_ulpi_create() usb: phy: tegra: Use u32 for hardware register variables usb: phy: tegra: Assert reset on ULPI close instead of deasserting it usb: phy: tegra: Use device-tree notion of reset-GPIO's active-state usb: phy: tegra: Disable VBUS regulator on tegra_usb_phy_init failure usb: phy: tegra: Move utmip_pad_count checking under lock usb: phy: tegra: Keep CPU interrupts enabled usb: chipidea: tegra: Stop managing PHY's power usb: chipidea: tegra: Add USB_TEGRA_PHY to driver's dependencies usb: host: ehci-tegra: Remove unused fields from tegra_ehci_hcd .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 + drivers/usb/chipidea/Kconfig | 1 + drivers/usb/chipidea/ci_hdrc_tegra.c | 15 +- drivers/usb/host/ehci-tegra.c | 2 - drivers/usb/phy/phy-tegra-usb.c | 830 +++++++++--------- drivers/usb/phy/phy-ulpi.c | 48 +- include/linux/usb/tegra_usb_phy.h | 3 +- include/linux/usb/ulpi.h | 11 + 8 files changed, 496 insertions(+), 418 deletions(-)