Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
Hi, On 05-05-16 18:00, Stephen Warren wrote: On 05/05/2016 02:05 AM, Hans de Goede wrote: Hi, On 04-05-16 22:25, Thierry Reding wrote: On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote: On 05/04/2016 08:40 AM, Thierry Reding wrote: From: Thierry Reding 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 Tegra SoCs have three EHCI controllers, each with a separate reset line. However the first controller contains UTMI pads configuration registers that are shared with its siblings and that are reset as part of the first controller's reset. There is special code in the driver to assert and deassert this shared reset at probe time, and it does so irrespective of which controller is probed first to ensure that these shared registers are reset before any of the controllers are initialized. Unfortunately this means that if the first controller gets probed first, it will request its own reset line and will 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 controller's reset if the controller happens to be the first controller. diff --git 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) +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; Isn't that just: has_utmi_pad_registers = of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"); ... and then you can remove " = false" from the declaration too? Yes. This is really only for aesthetics. The direct assignment doesn't fit within 80 columns, and wrapping it looks ugly whichever way you do it. if (!usb1_reset_attempted) { struct reset_control *usb1_reset; -usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); +if (!has_utmi_pad_registers) +usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); +else +usb1_reset = tegra->rst; ... usb1_reset_attempted = true; } This is a pre-existing issue, but what happens if the probes for two USB controllers run in parallel; there seems to be missing locking related to testing/setting usb1_reset_attempted, which could cause multiple parallel attempts to get the "utmi-pads" reset object, which would presumably cause essentially the same issue this patch is solving in other cases? Hah! Interestingly my initial attempt at fixing this was to introduce a lock to serialize these, because I thought that was what was going on. I don't think this function can ever run concurrently for different devices because the driver core already serializes probes (unless a driver specifically requests asynchronous probing, which this one doesn't). Why not just use the new shared reset functionality ? It is easy to use, that way you can drop some of the special handling in the driver and you're code actually reflects the hardware (which IMHO has a shared reset). Judging purely by the descriptions of the shared reset functionality in this thread, I doubt that will work. A varying number of USB controllers will be enabled in DT on a board-by-board basis, so anything that attempts to wait for "all devices to assert reset" can't be implemented, since it won't be known ahead of time how many reset assertions to wait for. Equally, if device probes are serialized, the reset will not happen at the right time since it can't happen until the nth probe (when each device has asserted reset) but we want it to happen during the 1st probe. Ah, so you actually want to reset the utmi-pad registers, not take them out of reset state ? Yeah then the shared reset support will not work. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
On 05/05/16 18:05, Jon Hunter wrote: [snip] > I spent a bit of time looking at this to figure out what it is doing. Can we > simply > this a bit as follows (limited testing so far) ... > > Cheers > Jon > > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c > index c1c1024a054c..70501053e1ec 100644 > --- a/drivers/usb/host/ehci-tegra.c > +++ b/drivers/usb/host/ehci-tegra.c > @@ -81,15 +81,25 @@ 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; > + dev_info(&pdev->dev, "%s-%d\n", __func__, __LINE__); Oops, some left over debug I meant to remove :-p Jon -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
On 04/05/16 15:40, Thierry Reding wrote: > From: Thierry Reding > > 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] [] (unwind_backtrace) from [] > (show_stack+0x10/0x14) > [3.406787] [] (show_stack) from [] > (dump_stack+0x90/0xa4) > [3.414007] [] (dump_stack) from [] (__warn+0xe8/0x100) > [3.420964] [] (__warn) from [] > (warn_slowpath_null+0x20/0x28) > [3.428525] [] (warn_slowpath_null) from [] > (__of_reset_control_get+0x16c/0x23c) > [3.437648] [] (__of_reset_control_get) from [] > (tegra_ehci_probe+0x394/0x518) > [3.446600] [] (tegra_ehci_probe) from [] > (platform_drv_probe+0x4c/0xb0) > [3.455029] [] (platform_drv_probe) from [] > (driver_probe_device+0x1ec/0x330) > [3.463892] [] (driver_probe_device) from [] > (__driver_attach+0xb8/0xbc) > [3.472320] [] (__driver_attach) from [] > (bus_for_each_dev+0x68/0x9c) > [3.480489] [] (bus_for_each_dev) from [] > (bus_add_driver+0x1a0/0x218) > [3.488743] [] (bus_add_driver) from [] > (driver_register+0x78/0xf8) > [3.496738] [] (driver_register) from [] > (do_one_initcall+0x40/0x170) > [3.504909] [] (do_one_initcall) from [] > (kernel_init_freeable+0x158/0x1f8) > [3.513600] [] (kernel_init_freeable) from [] > (kernel_init+0x8/0x114) > [3.521770] [] (kernel_init) from [] > (ret_from_fork+0x14/0x3c) > [3.529361] ---[ end trace 4bda87dbe4ecef8a ]--- > > The reason is that Tegra SoCs have three EHCI controllers, each with a > separate reset line. However the first controller contains UTMI pads > configuration registers that are shared with its siblings and that are > reset as part of the first controller's reset. There is special code in > the driver to assert and deassert this shared reset at probe time, and > it does so irrespective of which controller is probed first to ensure > that these shared registers are reset before any of the controllers are > initialized. Unfortunately this means that if the first controller gets > probed first, it will request its own reset line and will 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 controller's reset if the > controller happens to be the first controller. > > Cc: Philipp Zabel > Cc: Hans de Goede > Signed-off-by: Thierry Reding > --- > Changes in v3: > - reword commit message to more accurately describe the hardware design > > Changes in v2: > - restore has_utmi_pad_registers condition (Alan Stern) > > drivers/usb/host/ehci-tegra.c | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c > index c1c1024a054c..8396b622f238 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, "utmi-pads"); > + if (!has_utmi_pad_registers) > + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); > + 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) >
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
On 05/05/2016 02:05 AM, Hans de Goede wrote: Hi, On 04-05-16 22:25, Thierry Reding wrote: On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote: On 05/04/2016 08:40 AM, Thierry Reding wrote: From: Thierry Reding 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 Tegra SoCs have three EHCI controllers, each with a separate reset line. However the first controller contains UTMI pads configuration registers that are shared with its siblings and that are reset as part of the first controller's reset. There is special code in the driver to assert and deassert this shared reset at probe time, and it does so irrespective of which controller is probed first to ensure that these shared registers are reset before any of the controllers are initialized. Unfortunately this means that if the first controller gets probed first, it will request its own reset line and will 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 controller's reset if the controller happens to be the first controller. diff --git 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) +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; Isn't that just: has_utmi_pad_registers = of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"); ... and then you can remove " = false" from the declaration too? Yes. This is really only for aesthetics. The direct assignment doesn't fit within 80 columns, and wrapping it looks ugly whichever way you do it. if (!usb1_reset_attempted) { struct reset_control *usb1_reset; -usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); +if (!has_utmi_pad_registers) +usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); +else +usb1_reset = tegra->rst; ... usb1_reset_attempted = true; } This is a pre-existing issue, but what happens if the probes for two USB controllers run in parallel; there seems to be missing locking related to testing/setting usb1_reset_attempted, which could cause multiple parallel attempts to get the "utmi-pads" reset object, which would presumably cause essentially the same issue this patch is solving in other cases? Hah! Interestingly my initial attempt at fixing this was to introduce a lock to serialize these, because I thought that was what was going on. I don't think this function can ever run concurrently for different devices because the driver core already serializes probes (unless a driver specifically requests asynchronous probing, which this one doesn't). Why not just use the new shared reset functionality ? It is easy to use, that way you can drop some of the special handling in the driver and you're code actually reflects the hardware (which IMHO has a shared reset). Judging purely by the descriptions of the shared reset functionality in this thread, I doubt that will work. A varying number of USB controllers will be enabled in DT on a board-by-board basis, so anything that attempts to wait for "all devices to assert reset" can't be implemented, since it won't be known ahead of time how many reset assertions to wait for. Equally, if device probes are serialized, the reset will not happen at the right time since it can't happen until the nth probe (when each device has asserted reset) but we want it to happen during the 1st probe. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
Hi, On 04-05-16 22:25, Thierry Reding wrote: On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote: On 05/04/2016 08:40 AM, Thierry Reding wrote: From: Thierry Reding 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 Tegra SoCs have three EHCI controllers, each with a separate reset line. However the first controller contains UTMI pads configuration registers that are shared with its siblings and that are reset as part of the first controller's reset. There is special code in the driver to assert and deassert this shared reset at probe time, and it does so irrespective of which controller is probed first to ensure that these shared registers are reset before any of the controllers are initialized. Unfortunately this means that if the first controller gets probed first, it will request its own reset line and will 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 controller's reset if the controller happens to be the first controller. diff --git 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) + 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; Isn't that just: has_utmi_pad_registers = of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"); ... and then you can remove " = false" from the declaration too? Yes. This is really only for aesthetics. The direct assignment doesn't fit within 80 columns, and wrapping it looks ugly whichever way you do it. if (!usb1_reset_attempted) { struct reset_control *usb1_reset; - usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); + if (!has_utmi_pad_registers) + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); + else + usb1_reset = tegra->rst; ... usb1_reset_attempted = true; } This is a pre-existing issue, but what happens if the probes for two USB controllers run in parallel; there seems to be missing locking related to testing/setting usb1_reset_attempted, which could cause multiple parallel attempts to get the "utmi-pads" reset object, which would presumably cause essentially the same issue this patch is solving in other cases? Hah! Interestingly my initial attempt at fixing this was to introduce a lock to serialize these, because I thought that was what was going on. I don't think this function can ever run concurrently for different devices because the driver core already serializes probes (unless a driver specifically requests asynchronous probing, which this one doesn't). Why not just use the new shared reset functionality ? It is easy to use, that way you can drop some of the special handling in the driver and you're code actually reflects the hardware (which IMHO has a shared reset). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote: > On 05/04/2016 08:40 AM, Thierry Reding wrote: > > From: Thierry Reding > > > > 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 Tegra SoCs have three EHCI controllers, each with a > > separate reset line. However the first controller contains UTMI pads > > configuration registers that are shared with its siblings and that are > > reset as part of the first controller's reset. There is special code in > > the driver to assert and deassert this shared reset at probe time, and > > it does so irrespective of which controller is probed first to ensure > > that these shared registers are reset before any of the controllers are > > initialized. Unfortunately this means that if the first controller gets > > probed first, it will request its own reset line and will 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 controller's reset if the > > controller happens to be the first controller. > > > diff --git 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) > > > + 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; > > Isn't that just: > > has_utmi_pad_registers = of_property_read_bool(phy_np, > "nvidia,has-utmi-pad-registers"); > > ... and then you can remove " = false" from the declaration too? Yes. This is really only for aesthetics. The direct assignment doesn't fit within 80 columns, and wrapping it looks ugly whichever way you do it. > > if (!usb1_reset_attempted) { > > struct reset_control *usb1_reset; > > > > - usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); > > + if (!has_utmi_pad_registers) > > + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); > > + else > > + usb1_reset = tegra->rst; > ... > > usb1_reset_attempted = true; > > } > > This is a pre-existing issue, but what happens if the probes for two USB > controllers run in parallel; there seems to be missing locking related to > testing/setting usb1_reset_attempted, which could cause multiple parallel > attempts to get the "utmi-pads" reset object, which would presumably cause > essentially the same issue this patch is solving in other cases? Hah! Interestingly my initial attempt at fixing this was to introduce a lock to serialize these, because I thought that was what was going on. I don't think this function can ever run concurrently for different devices because the driver core already serializes probes (unless a driver specifically requests asynchronous probing, which this one doesn't). Thierry signature.asc Description: PGP signature
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
On Wed, May 04, 2016 at 07:22:54PM +0200, Philipp Zabel wrote: > Hi Thierry, > > Am Mittwoch, den 04.05.2016, 16:40 +0200 schrieb Thierry Reding: > > From: Thierry Reding > > > > 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] [] (unwind_backtrace) from [] > > (show_stack+0x10/0x14) > > [3.406787] [] (show_stack) from [] > > (dump_stack+0x90/0xa4) > > [3.414007] [] (dump_stack) from [] > > (__warn+0xe8/0x100) > > [3.420964] [] (__warn) from [] > > (warn_slowpath_null+0x20/0x28) > > [3.428525] [] (warn_slowpath_null) from [] > > (__of_reset_control_get+0x16c/0x23c) > > [3.437648] [] (__of_reset_control_get) from [] > > (tegra_ehci_probe+0x394/0x518) > > [3.446600] [] (tegra_ehci_probe) from [] > > (platform_drv_probe+0x4c/0xb0) > > [3.455029] [] (platform_drv_probe) from [] > > (driver_probe_device+0x1ec/0x330) > > [3.463892] [] (driver_probe_device) from [] > > (__driver_attach+0xb8/0xbc) > > [3.472320] [] (__driver_attach) from [] > > (bus_for_each_dev+0x68/0x9c) > > [3.480489] [] (bus_for_each_dev) from [] > > (bus_add_driver+0x1a0/0x218) > > [3.488743] [] (bus_add_driver) from [] > > (driver_register+0x78/0xf8) > > [3.496738] [] (driver_register) from [] > > (do_one_initcall+0x40/0x170) > > [3.504909] [] (do_one_initcall) from [] > > (kernel_init_freeable+0x158/0x1f8) > > [3.513600] [] (kernel_init_freeable) from [] > > (kernel_init+0x8/0x114) > > [3.521770] [] (kernel_init) from [] > > (ret_from_fork+0x14/0x3c) > > [3.529361] ---[ end trace 4bda87dbe4ecef8a ]--- > > > > The reason is that Tegra SoCs have three EHCI controllers, each with a > > separate reset line. However the first controller contains UTMI pads > > configuration registers that are shared with its siblings and that are > > reset as part of the first controller's reset. There is special code in > > the driver to assert and deassert this shared reset at probe time, and > > it does so irrespective of which controller is probed first to ensure > > that these shared registers are reset before any of the controllers are > > initialized. Unfortunately this means that if the first controller gets > > probed first, it will request its own reset line and will 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 controller's reset if the > > controller happens to be the first controller. > > > > Cc: Philipp Zabel > > Cc: Hans de Goede > > Signed-off-by: Thierry Reding > > --- > > Changes in v3: > > - reword commit message to more accurately describe the hardware design > > Reviewed-by: Philipp Zabel Thanks. > > Changes in v2: > > - restore has_utmi_pad_registers condition (Alan Stern) > > > > drivers/usb/host/ehci-tegra.c | 16 +--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c > > index c1c1024a054c..8396b622f238 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, "utmi-pads"); > > + if (!has_utmi_pad_registers) > > + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); > > +
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
On 05/04/2016 08:40 AM, Thierry Reding wrote: From: Thierry Reding 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 Tegra SoCs have three EHCI controllers, each with a separate reset line. However the first controller contains UTMI pads configuration registers that are shared with its siblings and that are reset as part of the first controller's reset. There is special code in the driver to assert and deassert this shared reset at probe time, and it does so irrespective of which controller is probed first to ensure that these shared registers are reset before any of the controllers are initialized. Unfortunately this means that if the first controller gets probed first, it will request its own reset line and will 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 controller's reset if the controller happens to be the first controller. diff --git 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) + 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; Isn't that just: has_utmi_pad_registers = of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"); ... and then you can remove " = false" from the declaration too? if (!usb1_reset_attempted) { struct reset_control *usb1_reset; - usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); + if (!has_utmi_pad_registers) + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); + else + usb1_reset = tegra->rst; ... usb1_reset_attempted = true; } This is a pre-existing issue, but what happens if the probes for two USB controllers run in parallel; there seems to be missing locking related to testing/setting usb1_reset_attempted, which could cause multiple parallel attempts to get the "utmi-pads" reset object, which would presumably cause essentially the same issue this patch is solving in other cases? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
Hi Thierry, Am Mittwoch, den 04.05.2016, 16:40 +0200 schrieb Thierry Reding: > From: Thierry Reding > > 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] [] (unwind_backtrace) from [] > (show_stack+0x10/0x14) > [3.406787] [] (show_stack) from [] > (dump_stack+0x90/0xa4) > [3.414007] [] (dump_stack) from [] (__warn+0xe8/0x100) > [3.420964] [] (__warn) from [] > (warn_slowpath_null+0x20/0x28) > [3.428525] [] (warn_slowpath_null) from [] > (__of_reset_control_get+0x16c/0x23c) > [3.437648] [] (__of_reset_control_get) from [] > (tegra_ehci_probe+0x394/0x518) > [3.446600] [] (tegra_ehci_probe) from [] > (platform_drv_probe+0x4c/0xb0) > [3.455029] [] (platform_drv_probe) from [] > (driver_probe_device+0x1ec/0x330) > [3.463892] [] (driver_probe_device) from [] > (__driver_attach+0xb8/0xbc) > [3.472320] [] (__driver_attach) from [] > (bus_for_each_dev+0x68/0x9c) > [3.480489] [] (bus_for_each_dev) from [] > (bus_add_driver+0x1a0/0x218) > [3.488743] [] (bus_add_driver) from [] > (driver_register+0x78/0xf8) > [3.496738] [] (driver_register) from [] > (do_one_initcall+0x40/0x170) > [3.504909] [] (do_one_initcall) from [] > (kernel_init_freeable+0x158/0x1f8) > [3.513600] [] (kernel_init_freeable) from [] > (kernel_init+0x8/0x114) > [3.521770] [] (kernel_init) from [] > (ret_from_fork+0x14/0x3c) > [3.529361] ---[ end trace 4bda87dbe4ecef8a ]--- > > The reason is that Tegra SoCs have three EHCI controllers, each with a > separate reset line. However the first controller contains UTMI pads > configuration registers that are shared with its siblings and that are > reset as part of the first controller's reset. There is special code in > the driver to assert and deassert this shared reset at probe time, and > it does so irrespective of which controller is probed first to ensure > that these shared registers are reset before any of the controllers are > initialized. Unfortunately this means that if the first controller gets > probed first, it will request its own reset line and will 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 controller's reset if the > controller happens to be the first controller. > > Cc: Philipp Zabel > Cc: Hans de Goede > Signed-off-by: Thierry Reding > --- > Changes in v3: > - reword commit message to more accurately describe the hardware design Reviewed-by: Philipp Zabel > Changes in v2: > - restore has_utmi_pad_registers condition (Alan Stern) > > drivers/usb/host/ehci-tegra.c | 16 +--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c > index c1c1024a054c..8396b622f238 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, "utmi-pads"); > + if (!has_utmi_pad_registers) > + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); > + 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_re
[PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice
From: Thierry Reding 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] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [3.406787] [] (show_stack) from [] (dump_stack+0x90/0xa4) [3.414007] [] (dump_stack) from [] (__warn+0xe8/0x100) [3.420964] [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [3.428525] [] (warn_slowpath_null) from [] (__of_reset_control_get+0x16c/0x23c) [3.437648] [] (__of_reset_control_get) from [] (tegra_ehci_probe+0x394/0x518) [3.446600] [] (tegra_ehci_probe) from [] (platform_drv_probe+0x4c/0xb0) [3.455029] [] (platform_drv_probe) from [] (driver_probe_device+0x1ec/0x330) [3.463892] [] (driver_probe_device) from [] (__driver_attach+0xb8/0xbc) [3.472320] [] (__driver_attach) from [] (bus_for_each_dev+0x68/0x9c) [3.480489] [] (bus_for_each_dev) from [] (bus_add_driver+0x1a0/0x218) [3.488743] [] (bus_add_driver) from [] (driver_register+0x78/0xf8) [3.496738] [] (driver_register) from [] (do_one_initcall+0x40/0x170) [3.504909] [] (do_one_initcall) from [] (kernel_init_freeable+0x158/0x1f8) [3.513600] [] (kernel_init_freeable) from [] (kernel_init+0x8/0x114) [3.521770] [] (kernel_init) from [] (ret_from_fork+0x14/0x3c) [3.529361] ---[ end trace 4bda87dbe4ecef8a ]--- The reason is that Tegra SoCs have three EHCI controllers, each with a separate reset line. However the first controller contains UTMI pads configuration registers that are shared with its siblings and that are reset as part of the first controller's reset. There is special code in the driver to assert and deassert this shared reset at probe time, and it does so irrespective of which controller is probed first to ensure that these shared registers are reset before any of the controllers are initialized. Unfortunately this means that if the first controller gets probed first, it will request its own reset line and will 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 controller's reset if the controller happens to be the first controller. Cc: Philipp Zabel Cc: Hans de Goede Signed-off-by: Thierry Reding --- Changes in v3: - reword commit message to more accurately describe the hardware design Changes in v2: - restore has_utmi_pad_registers condition (Alan Stern) drivers/usb/host/ehci-tegra.c | 16 +--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c index c1c1024a054c..8396b622f238 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, "utmi-pads"); + if (!has_utmi_pad_registers) + usb1_reset = of_reset_control_get(phy_np, "utmi-pads"); + 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); +