Date:	Thu, 14 Mar 2013 20:28:46 +0530
From:	Thomas Abraham <thomas.abraham@...aro.org>
To:	Alexander Graf <agraf@...e.de>
Cc:	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, dmueller@...e.de,
	Vivek Gautam <gautam.vivek@...sung.com>,
	Jingoo Han <jg1.han@...sung.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH v2] USB: ehci-s5p: Fix phy reset

On 14 March 2013 17:31, Alexander Graf <agraf@...e.de> wrote:
>
> On 14.03.2013, at 05:19, Thomas Abraham wrote:
>
>> On 14 March 2013 05:29, Alexander Graf <agraf@...e.de> wrote:
>>> On my Exynos 5 based Arndale system, I need to pull the reset line down
>>> and then let it go up again to actually perform a reset. Without that
>>> reset, I can't find any USB hubs on my bus, rendering the USB controller
>>> useless.
>>>
>>> We also only need to reset the line after the phy node has been found.
>>> This way we don't accidently reserve the vbus GPIO pin, but later on
>>> defer the creation of our controller, because the phy device tree node
>>> hasn't been probed yet.
>>>
>>> This patch implements the above logic, making EHCI and OHCI work on
>>> Arndale systems for me.
>>>
>>> Signed-off-by: Alexander Graf <agraf@...e.de>
>>> CC: Vivek Gautam <gautam.vivek@...sung.com>
>>> CC: Jingoo Han <jg1.han@...sung.com>
>>> CC: Alan Stern <stern@...land.harvard.edu>
>>> CC: Kukjin Kim <kgene.kim@...sung.com>
>>> CC: Felipe Balbi <balbi@...com>
>>> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>> CC: Doug Anderson <dianders@...omium.org>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>  - remove gpio_free call
>>>  - move reset logic after phy node search
>>>
>>> diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c
>>> index 20ebf6a..b29b2b8 100644
>>> --- a/drivers/usb/host/ehci-s5p.c
>>> +++ b/drivers/usb/host/ehci-s5p.c
>>> @@ -103,9 +103,14 @@ static void s5p_setup_vbus_gpio(struct platform_device *pdev)
>>>        if (!gpio_is_valid(gpio))
>>>                return;
>>>
>>> -       err = gpio_request_one(gpio, GPIOF_OUT_INIT_HIGH, "ehci_vbus_gpio");
>>> -       if (err)
>>> +       /* reset pulls the line down, then up again */
>>> +       err = gpio_request_one(gpio, GPIOF_OUT_INIT_LOW, "ehci_vbus_gpio");
>>> +       if (err) {
>>>                dev_err(&pdev->dev, "can't request ehci vbus gpio %d", gpio);
>>> +               return;
>>> +       }
>>> +       mdelay(1);
>>> +       __gpio_set_value(gpio, 1);
>>> }
>>>
>>> static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32);
>>> @@ -131,8 +136,6 @@ static int s5p_ehci_probe(struct platform_device *pdev)
>>>        if (!pdev->dev.coherent_dma_mask)
>>>                pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>>
>>> -       s5p_setup_vbus_gpio(pdev);
>>> -
>>>        s5p_ehci = devm_kzalloc(&pdev->dev, sizeof(struct s5p_ehci_hcd),
>>>                                GFP_KERNEL);
>>>        if (!s5p_ehci)
>>> @@ -152,6 +155,8 @@ static int s5p_ehci_probe(struct platform_device *pdev)
>>>                s5p_ehci->otg = phy->otg;
>>>        }
>>>
>>> +       s5p_setup_vbus_gpio(pdev);
>>> +
>>>        s5p_ehci->dev = &pdev->dev;
>>>
>>>        hcd = usb_create_hcd(&s5p_ehci_hc_driver, &pdev->dev,
>>
>> Hi Alexander,
>>
>> This change, though it works for Exynos5250 based Arndale board, does
>> not actually seem correct. On Arndale board, the on-board 4-port usb
>> hub is self powered and hence the vbus 'enable' gpio line from
>> Exynos5250 SoC is instead used as a "reset" signal for the on-board
>> usb hub (and not as the vbus enable signal).
>>
>> Whereas, the driver uses the gpio used in 's5p_setup_vbus_gpio'
>> function as just a mechanism to enable vbus for downstream devices. So
>> the driver should not be modified as above to handle the board
>> specific behavior.
>>
>> Instead, what needs to be done is, remove the "samsung,vbus-gpio"
>> property from the usb2.0 node in dts files (this property is optional)
>> for Arndale board. Then, during the machine_init, perform the reset
>> sequencing as required.
>>
>> Ideally, the reset sequencing for the on-board AX88760 usb hub should
>> have been handled in the driver for this device. I have not checked if
>> there is a driver for this in the kernel.
>
> I can see your point, but as I mentioned earlier there seems to be some timing issue here. By simply doing the reset a few ms earlier (in the first probe, before the driver detects that it needs to defer probing), I already can't find the hub on the bus later.
>
> So I'm assuming that the same thing would also happen if I put it even earlier in machine init.

True, I missed that point. The usb hub connected over hsic interface,
after power-on-reset, might have initiated the 'connect' state on
seeing the idle condition on the bus and since the host/phy controller
is not ready yet, the connect might have failed.

So the correct sequence would be, after the usb host controller and
the phy controllers are initialized, the 'reset' pin on the on-board
usb hub should be asserted. Upon releasing that reset, the usb hub
would initiate the 'connect' state on the HSIC bus.

>
> The change in this patch actually does a reset even on non-Arndale boards. By taking away power and returning power to the line, the chip will most likely have reset :). So even on non-Arndale boards, this should get the USB phy into a clean state regardless of where the bootloader left it, right?
>

No, the toggling of the vbus cannot ensure hardware-reset on self
powered devices. On Arndale board, since the usb hub is self powered
and being on HSIC interface, the dedicated vbus control gpio line is
instead used to assert the 'reset' pin of the on-board usb hub.

Using the dedicated vbus control line of Exynos5250 (XuhostPWREN) for
hardware resetting the usb hub is a board specific design of Arndale
board. The function ''s5p_setup_vbus_gpio' is meant to turn on the
vbus for downstream ports. Utilizing this function to hardware-reset
the usb hub is not right.

In fact, for Arndale board, there should not be a 'samsung,vbus-gpio'
property for usb2.0 host controller node. Because, there is no such
vbus control line on Arndale board which is using HSIC interface to
connect to USB devices. So this change is not correct. The assertion
of the hardware-reset for the on board usb hub should be handled
elsewhere.

Thanks,
Thomas.

>
> Alex
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists