Re: [PATCH] ARM: OMAP3: USB: Fix the EHCI ULPI PHY reset fix issues.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 14, 2012 at 5:59 AM, Igor Grinberg <grinberg@xxxxxxxxxxxxxx> wrote:
> Hi Russ,
>
> On 06/14/12 14:31, Russ Dill wrote:
>> 'ARM: OMAP3: USB: Fix the EHCI ULPI PHY reset issue' (1fcb57d0) fixes
>> an issue where the ULPI PHYs were not held in reset while initializing
>> the EHCI controller. However, it also changes behavior in
>> omap-usb-host.c omap_usbhs_init by releasing reset while the
>> configuration in that function was done.
>>
>> This change caused a regression on BB-xM where USB would not function
>> if 'usb start' had been run from u-boot before booting. A change was
>> made to release reset a little bit earlier which fixed the issue on
>> BB-xM and did not cause any regressions on 3430 sdp, the board for
>> which the fix was originally made.
>>
>> This new fix, 'USB: EHCI: OMAP: Finish ehci omap phy reset cycle
>> before adding hcd.', (3aa2ae74) caused a regression on OMAP5.
>>
>> The original fix to hold the EHCI controller in reset during
>> initialization was correct, however it appears that changing
>> omap_usbhs_init to not hold the PHYs in reset during it's
>> configuration was incorrect. This patch first reverts both fixes, and
>> then changes ehci_hcd_omap_probe in ehci-omap.c to hold the PHYs in
>> reset as the original patch had done. It also is sure to incorporate
>> the _cansleep change that has been made in the meantime.
>>
>> I've tested this on Beagleboard xM, I'd really like to get an ack from
>> the 3430 sdp and OMAP5 guys before getting this merged.
>>
>> Signed-off-by: Russ Dill <Russ.Dill@xxxxxxxxx>
>> ---
>>  drivers/mfd/omap-usb-host.c  |   45 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/host/ehci-omap.c |   18 ++++++++---------
>>  2 files changed, 53 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>> index 7e96bb2..10984e2 100644
>> --- a/drivers/mfd/omap-usb-host.c
>> +++ b/drivers/mfd/omap-usb-host.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/clk.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/spinlock.h>
>> +#include <linux/gpio.h>
>>  #include <plat/cpu.h>
>>  #include <plat/usb.h>
>>  #include <linux/pm_runtime.h>
>> @@ -502,6 +503,19 @@ static void omap_usbhs_init(struct device *dev)
>>       pm_runtime_get_sync(dev);
>>       spin_lock_irqsave(&omap->lock, flags);
>>
>> +     if (pdata->ehci_data->phy_reset) {
>> +             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
>> +                     gpio_request_one(pdata->ehci_data->reset_gpio_port[0],
>> +                                      GPIOF_OUT_INIT_LOW, "USB1 PHY reset");
>> +
>> +             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
>> +                     gpio_request_one(pdata->ehci_data->reset_gpio_port[1],
>> +                                      GPIOF_OUT_INIT_LOW, "USB2 PHY reset");
>> +
>> +             /* Hold the PHY in RESET for enough time till DIR is high */
>> +             udelay(10);
>> +     }
>
> Hmmm.... I think this hunk should come before the spin_lock_irqsave() call,
> as it might sleep with I2C GPIO extenders...
>
>> +
>>       omap->usbhs_rev = usbhs_read(omap->uhh_base, OMAP_UHH_REVISION);
>>       dev_dbg(dev, "OMAP UHH_REVISION 0x%x\n", omap->usbhs_rev);
>>
>> @@ -580,10 +594,39 @@ static void omap_usbhs_init(struct device *dev)
>>                       usbhs_omap_tll_init(dev, OMAP_TLL_CHANNEL_COUNT);
>>       }
>>
>> +     if (pdata->ehci_data->phy_reset) {
>> +             /* Hold the PHY in RESET for enough time till
>> +              * PHY is settled and ready
>> +              */
>> +             udelay(10);
>> +
>> +             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
>> +                     gpio_set_value_cansleep
>> +                             (pdata->ehci_data->reset_gpio_port[0], 1);
>> +
>> +             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
>> +                     gpio_set_value_cansleep
>> +                             (pdata->ehci_data->reset_gpio_port[1], 1);
>> +     }
>
> Same here, after the the spin_unlock_irqrestore() call?

That's embarrassing, thanks, I'll resend.

>> +
>>       spin_unlock_irqrestore(&omap->lock, flags);
>>       pm_runtime_put_sync(dev);
>>  }
>>
>> +static void omap_usbhs_deinit(struct device *dev)
>> +{
>> +     struct usbhs_hcd_omap           *omap = dev_get_drvdata(dev);
>> +     struct usbhs_omap_platform_data *pdata = &omap->platdata;
>> +
>> +     if (pdata->ehci_data->phy_reset) {
>> +             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
>> +                     gpio_free(pdata->ehci_data->reset_gpio_port[0]);
>> +
>> +             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
>> +                     gpio_free(pdata->ehci_data->reset_gpio_port[1]);
>> +     }
>> +}
>> +
>>
>>  /**
>>   * usbhs_omap_probe - initialize TI-based HCDs
>> @@ -767,6 +810,7 @@ static int __devinit usbhs_omap_probe(struct platform_device *pdev)
>>       goto end_probe;
>>
>>  err_alloc:
>> +     omap_usbhs_deinit(&pdev->dev);
>>       iounmap(omap->tll_base);
>>
>>  err_tll:
>> @@ -818,6 +862,7 @@ static int __devexit usbhs_omap_remove(struct platform_device *pdev)
>>  {
>>       struct usbhs_hcd_omap *omap = platform_get_drvdata(pdev);
>>
>> +     omap_usbhs_deinit(&pdev->dev);
>>       iounmap(omap->tll_base);
>>       iounmap(omap->uhh_base);
>>       clk_put(omap->init_60m_fclk);
>> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
>> index a44294d..d0815b6 100644
>> --- a/drivers/usb/host/ehci-omap.c
>> +++ b/drivers/usb/host/ehci-omap.c
>> @@ -192,14 +192,13 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
>>               }
>>       }
>>
>> +     /* Hold PHYs in reset while initializing EHCI controller */
>>       if (pdata->phy_reset) {
>>               if (gpio_is_valid(pdata->reset_gpio_port[0]))
>> -                     gpio_request_one(pdata->reset_gpio_port[0],
>> -                                      GPIOF_OUT_INIT_LOW, "USB1 PHY reset");
>> +                     gpio_set_value_cansleep(pdata->reset_gpio_port[0], 0);
>>
>>               if (gpio_is_valid(pdata->reset_gpio_port[1]))
>> -                     gpio_request_one(pdata->reset_gpio_port[1],
>> -                                      GPIOF_OUT_INIT_LOW, "USB2 PHY reset");
>> +                     gpio_set_value_cansleep(pdata->reset_gpio_port[1], 0);
>>
>>               /* Hold the PHY in RESET for enough time till DIR is high */
>>               udelay(10);
>> @@ -241,6 +240,11 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
>>       omap_ehci->hcs_params = readl(&omap_ehci->caps->hcs_params);
>>
>>       ehci_reset(omap_ehci);
>> +     ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>> +     if (ret) {
>> +             dev_err(dev, "failed to add hcd with err %d\n", ret);
>> +             goto err_add_hcd;
>> +     }
>>
>>       if (pdata->phy_reset) {
>>               /* Hold the PHY in RESET for enough time till
>> @@ -255,12 +259,6 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
>>                       gpio_set_value_cansleep(pdata->reset_gpio_port[1], 1);
>>       }
>>
>> -     ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>> -     if (ret) {
>> -             dev_err(dev, "failed to add hcd with err %d\n", ret);
>> -             goto err_add_hcd;
>> -     }
>> -
>>       /* root ports should always stay powered */
>>       ehci_port_power(omap_ehci, 1);
>>
>
> --
> Regards,
> Igor.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux