[lkml]   [2016]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

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).



 \ /
  Last update: 2016-05-05 10:21    [W:0.079 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site