| Message ID | 1462298755-20857-1-git-send-email-thierry.reding@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, May 03, 2016 at 08:05:55PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Starting with commit 0b52297f2288 ("reset: Add support for shared reset > controls") there is a reference count for reset control assertions. The > goal is to allow resets to be shared by multiple devices and an assert > will take effect only when all instances have asserted the reset. > > In order to preserve backwards-compatibility, all reset controls become > exclusive by default. This is to ensure that reset_control_assert() can > immediately assert in hardware. > > However, this new behaviour triggers the following warning in the EHCI > driver for Tegra: > > [ 3.365019] ------------[ cut here ]------------ > [ 3.369639] WARNING: CPU: 0 PID: 1 at drivers/reset/core.c:187 __of_reset_control_get+0x16c/0x23c > [ 3.382151] Modules linked in: > [ 3.385214] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.6.0-rc6-next-20160503 #140 > [ 3.392769] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) > [ 3.399046] [<c010fa50>] (unwind_backtrace) from [<c010b120>] (show_stack+0x10/0x14) > [ 3.406787] [<c010b120>] (show_stack) from [<c0347dcc>] (dump_stack+0x90/0xa4) > [ 3.414007] [<c0347dcc>] (dump_stack) from [<c011f4fc>] (__warn+0xe8/0x100) > [ 3.420964] [<c011f4fc>] (__warn) from [<c011f5c4>] (warn_slowpath_null+0x20/0x28) > [ 3.428525] [<c011f5c4>] (warn_slowpath_null) from [<c03cc8cc>] (__of_reset_control_get+0x16c/0x23c) > [ 3.437648] [<c03cc8cc>] (__of_reset_control_get) from [<c0526858>] (tegra_ehci_probe+0x394/0x518) > [ 3.446600] [<c0526858>] (tegra_ehci_probe) from [<c04516d8>] (platform_drv_probe+0x4c/0xb0) > [ 3.455029] [<c04516d8>] (platform_drv_probe) from [<c044fe78>] (driver_probe_device+0x1ec/0x330) > [ 3.463892] [<c044fe78>] (driver_probe_device) from [<c0450074>] (__driver_attach+0xb8/0xbc) > [ 3.472320] [<c0450074>] (__driver_attach) from [<c044e1ec>] (bus_for_each_dev+0x68/0x9c) > [ 3.480489] [<c044e1ec>] (bus_for_each_dev) from [<c044f338>] (bus_add_driver+0x1a0/0x218) > [ 3.488743] [<c044f338>] (bus_add_driver) from [<c0450768>] (driver_register+0x78/0xf8) > [ 3.496738] [<c0450768>] (driver_register) from [<c010178c>] (do_one_initcall+0x40/0x170) > [ 3.504909] [<c010178c>] (do_one_initcall) from [<c0c00ddc>] (kernel_init_freeable+0x158/0x1f8) > [ 3.513600] [<c0c00ddc>] (kernel_init_freeable) from [<c0810784>] (kernel_init+0x8/0x114) > [ 3.521770] [<c0810784>] (kernel_init) from [<c0107778>] (ret_from_fork+0x14/0x3c) > [ 3.529361] ---[ end trace 4bda87dbe4ecef8a ]--- > > The reason is that the EHCI implements three ports, each with a separate > reset line. However the first port's reset also serves as a means to > reset the UTMI pad for all ports. There is special code in the driver to > assert and deassert this shared reset at probe time. It needs to do this > regardless of which port is probed first. Unfortunately this means that > if the first port is probed first, it will request its own reset line > and subsequently request the same reset line again (temporarily) to > perform the reset. This used to work fine before the above-mentioned > commit, but now triggers the new WARN. > > Work around this by making sure we reuse the port's reset if it happens > to be the same as the UTMI pad reset. > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/usb/host/ehci-tegra.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) Looks like I sent this too quickly. It doesn't seem to get rid of the warning in all cases, so I'll need to investigate further. Sorry for the noise. Thierry
On Tue, 3 May 2016, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Starting with commit 0b52297f2288 ("reset: Add support for shared reset > controls") there is a reference count for reset control assertions. The > goal is to allow resets to be shared by multiple devices and an assert > will take effect only when all instances have asserted the reset. > > In order to preserve backwards-compatibility, all reset controls become > exclusive by default. This is to ensure that reset_control_assert() can > immediately assert in hardware. > > However, this new behaviour triggers the following warning in the EHCI > driver for Tegra: ... > The reason is that the EHCI implements three ports, each with a separate > reset line. However the first port's reset also serves as a means to > reset the UTMI pad for all ports. There is special code in the driver to > assert and deassert this shared reset at probe time. It needs to do this > regardless of which port is probed first. Unfortunately this means that > if the first port is probed first, it will request its own reset line > and subsequently request the same reset line again (temporarily) to > perform the reset. This used to work fine before the above-mentioned > commit, but now triggers the new WARN. > > Work around this by making sure we reuse the port's reset if it happens > to be the same as the UTMI pad reset. > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Thierry Reding <treding@nvidia.com> Possibly related to the problems you are still seeing, I noticed something odd about the patch. > --- a/drivers/usb/host/ehci-tegra.c > +++ b/drivers/usb/host/ehci-tegra.c > @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev) > struct usb_hcd *hcd = platform_get_drvdata(pdev); > struct tegra_ehci_hcd *tegra = > (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv; > + bool has_utmi_pad_registers = false; > > phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0); > if (!phy_np) > return -ENOENT; > > + if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) > + has_utmi_pad_registers = true; I would have writte simply: has_utmi_pad_registers = of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"); Regardless, notice that has_utmi_pad_registers gets set to true if the OF property value is true. ... > - if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) { > + if (has_utmi_pad_registers) { But down here the sense of the test is reversed. Now the conditional block gets executed if the property is true, whereas the original code executed it if the property was false. > reset_control_assert(tegra->rst); > udelay(1); > reset_control_deassert(tegra->rst); Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 03, 2016 at 03:16:56PM -0400, Alan Stern wrote: > On Tue, 3 May 2016, Thierry Reding wrote: > > > From: Thierry Reding <treding@nvidia.com> > > > > Starting with commit 0b52297f2288 ("reset: Add support for shared reset > > controls") there is a reference count for reset control assertions. The > > goal is to allow resets to be shared by multiple devices and an assert > > will take effect only when all instances have asserted the reset. > > > > In order to preserve backwards-compatibility, all reset controls become > > exclusive by default. This is to ensure that reset_control_assert() can > > immediately assert in hardware. > > > > However, this new behaviour triggers the following warning in the EHCI > > driver for Tegra: > > ... > > > The reason is that the EHCI implements three ports, each with a separate > > reset line. However the first port's reset also serves as a means to > > reset the UTMI pad for all ports. There is special code in the driver to > > assert and deassert this shared reset at probe time. It needs to do this > > regardless of which port is probed first. Unfortunately this means that > > if the first port is probed first, it will request its own reset line > > and subsequently request the same reset line again (temporarily) to > > perform the reset. This used to work fine before the above-mentioned > > commit, but now triggers the new WARN. > > > > Work around this by making sure we reuse the port's reset if it happens > > to be the same as the UTMI pad reset. > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > Possibly related to the problems you are still seeing, I noticed > something odd about the patch. > > > --- a/drivers/usb/host/ehci-tegra.c > > +++ b/drivers/usb/host/ehci-tegra.c > > @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev) > > struct usb_hcd *hcd = platform_get_drvdata(pdev); > > struct tegra_ehci_hcd *tegra = > > (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv; > > + bool has_utmi_pad_registers = false; > > > > phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0); > > if (!phy_np) > > return -ENOENT; > > > > + if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) > > + has_utmi_pad_registers = true; > > I would have writte simply: > > has_utmi_pad_registers = of_property_read_bool(phy_np, > "nvidia,has-utmi-pad-registers"); I wrote it that way specifically because all variants with direct assignment that I could come up with looked ugly if I wanted the lines to fit within 80 columns. > Regardless, notice that has_utmi_pad_registers gets set to true if the > OF property value is true. > > ... > > > - if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) { > > + if (has_utmi_pad_registers) { > > But down here the sense of the test is reversed. Now the conditional > block gets executed if the property is true, whereas the original code > executed it if the property was false. Good catch! Unfortunately this doesn't help getting rid of the WARN_ON. However I finally tracked this down, though it took me an embarrassingly long time because I kept looking at the wrong place. The reason why this patch doesn't fix it is because the original code is requesting the wrong reset control in the first place. It's requesting the control named "usb" to reset the UTMI pads registers where is really should be requesting the one named "utmi-pads". So the above patch only works if the first controller ends up being probed first (because that's when both resets are the same). In any other case the "usb" reset will still be requested twice. I've sent out a new patch that should fix all the warnings. I ran it through the same test system I sent these yesterday and I don't see any warnings anymore. Thierry
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c index 4031b372008e..c8fe0fe9de68 100644 --- a/drivers/usb/host/ehci-tegra.c +++ b/drivers/usb/host/ehci-tegra.c @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev) struct usb_hcd *hcd = platform_get_drvdata(pdev); struct tegra_ehci_hcd *tegra = (struct tegra_ehci_hcd *)hcd_to_ehci(hcd)->priv; + bool has_utmi_pad_registers = false; phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0); if (!phy_np) return -ENOENT; + if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) + has_utmi_pad_registers = true; + if (!usb1_reset_attempted) { struct reset_control *usb1_reset; - usb1_reset = of_reset_control_get(phy_np, "usb"); + if (!has_utmi_pad_registers) + usb1_reset = of_reset_control_get(phy_np, "usb"); + else + usb1_reset = tegra->rst; + if (IS_ERR(usb1_reset)) { dev_warn(&pdev->dev, "can't get utmi-pads reset from the PHY\n"); @@ -101,11 +109,13 @@ static int tegra_reset_usb_controller(struct platform_device *pdev) reset_control_deassert(usb1_reset); } - reset_control_put(usb1_reset); + if (!has_utmi_pad_registers) + reset_control_put(usb1_reset); + usb1_reset_attempted = true; } - if (!of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers")) { + if (has_utmi_pad_registers) { reset_control_assert(tegra->rst); udelay(1); reset_control_deassert(tegra->rst);