| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFp+6iGe3Hz6OiVNDa=8c98CyNGaELfg-uRbCO0kFdpdv_gQpw@mail.gmail.com>
Date: Thu, 20 Dec 2012 12:07:19 +0530
From: Vivek Gautam <gautamvivek1987@...il.com>
To: Doug Anderson <dianders@...omium.org>
Cc: Vivek Gautam <gautam.vivek@...sung.com>, linux-usb@...r.kernel.org,
yulgon.kim@...sung.com, linux-samsung-soc@...r.kernel.org,
Praveen Paneri <p.paneri@...sung.com>,
gregkh@...uxfoundation.org, devicetree-discuss@...ts.ozlabs.org,
jg1.han@...sung.com,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
balbi@...com, kishon@...com, Kukjin Kim <kgene.kim@...sung.com>,
Alan Stern <stern@...land.harvard.edu>,
Rob Herring <rob.herring@...xeda.com>,
Sylwester Nawrocki <sylvester.nawrocki@...il.com>
Subject: Re: [PATCH v5 3/4] USB: ehci-s5p: Add phy driver support
Hi Doug,
On Thu, Dec 20, 2012 at 5:00 AM, Doug Anderson <dianders@...omium.org> wrote:
> Vivek,
>
> Again, not a high-level review, but...
>
Thanks for reviewing. :-)
>
> On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam <gautam.vivek@...sung.com> wrote:
>> Adding the phy driver to ehci-s5p. Keeping the platform data
>> for continuing the smooth operation for boards which still uses it
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@...sung.com>
>> Acked-by: Jingoo Han <jg1.han@...sung.com>
>> ---
>> drivers/usb/host/ehci-s5p.c | 70 ++++++++++++++++++++++++++++++-------------
>> 1 files changed, 49 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c
>> index 46ca5ef..50c93af 100644
>> --- a/drivers/usb/host/ehci-s5p.c
>> +++ b/drivers/usb/host/ehci-s5p.c
>> @@ -17,6 +17,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/of_gpio.h>
>> #include <linux/platform_data/usb-ehci-s5p.h>
>> +#include <linux/usb/phy.h>
>> #include <linux/usb/samsung_usb_phy.h>
>> #include <plat/usb-phy.h>
>>
>> @@ -33,6 +34,8 @@ struct s5p_ehci_hcd {
>> struct device *dev;
>> struct usb_hcd *hcd;
>> struct clk *clk;
>> + struct usb_phy *phy;
>> + struct s5p_ehci_platdata *pdata;
>> };
>>
>> static const struct hc_driver s5p_ehci_hc_driver = {
>> @@ -66,6 +69,30 @@ static const struct hc_driver s5p_ehci_hc_driver = {
>> .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
>> };
>>
>> +static void s5p_ehci_phy_enable(struct s5p_ehci_hcd *s5p_ehci)
>> +{
>> + struct platform_device *pdev = to_platform_device(s5p_ehci->dev);
>> +
>> + if (s5p_ehci->phy) {
>> + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST);
>
> This confuses me. Why are you setting the type to host here?
>
Being in host controller, before calling usb_phy_init() we set type to
Host since, with certain SOCs
like 4210, same register has different bit settings for HOST type and
device type. So setting this
to Host type here make the flow of usb_phy_init to go in the direction of Host.
>> + usb_phy_init(s5p_ehci->phy);
>> + } else if (s5p_ehci->pdata->phy_init) {
>> + s5p_ehci->pdata->phy_init(pdev, USB_PHY_TYPE_HOST);
>> + }
>> +}
>> +
>> +static void s5p_ehci_phy_disable(struct s5p_ehci_hcd *s5p_ehci)
>> +{
>> + struct platform_device *pdev = to_platform_device(s5p_ehci->dev);
>> +
>> + if (s5p_ehci->phy) {
>> + samsung_usbphy_set_type(s5p_ehci->phy, USB_PHY_TYPE_HOST);
>
> ...and why set it to host here?
>
Same here...
>> + usb_phy_shutdown(s5p_ehci->phy);
>> + } else if (s5p_ehci->pdata->phy_exit) {
>> + s5p_ehci->pdata->phy_exit(pdev, USB_PHY_TYPE_HOST);
>> + }
>> +}
>> +
>> static void s5p_setup_vbus_gpio(struct platform_device *pdev)
>> {
>> int err;
>> @@ -88,20 +115,15 @@ static u64 ehci_s5p_dma_mask = DMA_BIT_MASK(32);
>>
>> static int s5p_ehci_probe(struct platform_device *pdev)
>> {
>> - struct s5p_ehci_platdata *pdata;
>> + struct s5p_ehci_platdata *pdata = pdev->dev.platform_data;
>> struct s5p_ehci_hcd *s5p_ehci;
>> struct usb_hcd *hcd;
>> struct ehci_hcd *ehci;
>> struct resource *res;
>> + struct usb_phy *phy;
>> int irq;
>> int err;
>>
>> - pdata = pdev->dev.platform_data;
>> - if (!pdata) {
>> - dev_err(&pdev->dev, "No platform data defined\n");
>> - return -EINVAL;
>> - }
>> -
>> /*
>> * Right now device-tree probed devices don't get dma_mask set.
>> * Since shared usb code relies on it, set it here for now.
>> @@ -119,6 +141,19 @@ static int s5p_ehci_probe(struct platform_device *pdev)
>> if (!s5p_ehci)
>> return -ENOMEM;
>>
>> + phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
>> + if (IS_ERR_OR_NULL(phy)) {
>> + /* Fallback to pdata */
>> + if (!pdata) {
>> + dev_err(&pdev->dev, "no platform data or transceiver defined\n");
>> + return -EPROBE_DEFER;
>
> Shouldn't you return -EINVAL like the old code did?
We are deferring the probe since the usb-phy transceiver may get
probed after ehci/ohci controllers.
And if we return -EINVAL like the previous code, we would end up not
setting the phy.
>
>> + } else {
>> + s5p_ehci->pdata = pdata;
>> + }
>> + } else {
>> + s5p_ehci->phy = phy;
>> + }
>> +
>> s5p_ehci->dev = &pdev->dev;
>>
>> hcd = usb_create_hcd(&s5p_ehci_hc_driver, &pdev->dev,
>> @@ -164,8 +199,7 @@ static int s5p_ehci_probe(struct platform_device *pdev)
>> goto fail_io;
>> }
>>
>> - if (pdata->phy_init)
>> - pdata->phy_init(pdev, USB_PHY_TYPE_HOST);
>> + s5p_ehci_phy_enable(s5p_ehci);
>>
>> ehci = hcd_to_ehci(hcd);
>> ehci->caps = hcd->regs;
>> @@ -176,13 +210,15 @@ static int s5p_ehci_probe(struct platform_device *pdev)
>> err = usb_add_hcd(hcd, irq, IRQF_SHARED);
>> if (err) {
>> dev_err(&pdev->dev, "Failed to add USB HCD\n");
>> - goto fail_io;
>> + goto fail_add_hcd;
>> }
>>
>> platform_set_drvdata(pdev, s5p_ehci);
>>
>> return 0;
>>
>> +fail_add_hcd:
>> + s5p_ehci_phy_disable(s5p_ehci);
>> fail_io:
>> clk_disable_unprepare(s5p_ehci->clk);
>> fail_clk:
>> @@ -192,14 +228,12 @@ fail_clk:
>>
>> static int s5p_ehci_remove(struct platform_device *pdev)
>> {
>> - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data;
>> struct s5p_ehci_hcd *s5p_ehci = platform_get_drvdata(pdev);
>> struct usb_hcd *hcd = s5p_ehci->hcd;
>>
>> usb_remove_hcd(hcd);
>>
>> - if (pdata && pdata->phy_exit)
>> - pdata->phy_exit(pdev, USB_PHY_TYPE_HOST);
>> + s5p_ehci_phy_disable(s5p_ehci);
>>
>> clk_disable_unprepare(s5p_ehci->clk);
>>
>> @@ -223,14 +257,11 @@ static int s5p_ehci_suspend(struct device *dev)
>> struct s5p_ehci_hcd *s5p_ehci = dev_get_drvdata(dev);
>> struct usb_hcd *hcd = s5p_ehci->hcd;
>> bool do_wakeup = device_may_wakeup(dev);
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data;
>> int rc;
>>
>> rc = ehci_suspend(hcd, do_wakeup);
>>
>> - if (pdata && pdata->phy_exit)
>> - pdata->phy_exit(pdev, USB_PHY_TYPE_HOST);
>> + s5p_ehci_phy_disable(s5p_ehci);
>>
>> clk_disable_unprepare(s5p_ehci->clk);
>>
>> @@ -241,13 +272,10 @@ static int s5p_ehci_resume(struct device *dev)
>> {
>> struct s5p_ehci_hcd *s5p_ehci = dev_get_drvdata(dev);
>> struct usb_hcd *hcd = s5p_ehci->hcd;
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct s5p_ehci_platdata *pdata = pdev->dev.platform_data;
>>
>> clk_prepare_enable(s5p_ehci->clk);
>>
>> - if (pdata && pdata->phy_init)
>> - pdata->phy_init(pdev, USB_PHY_TYPE_HOST);
>> + s5p_ehci_phy_enable(s5p_ehci);
>>
>> /* DMA burst Enable */
>> writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
>> --
>> 1.7.6.5
>>
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss@...ts.ozlabs.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks & Regards
Vivek
--
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