RE: [PATCH v5 4/9] usb: ehci-s5p: Change to use phy provided by thegeneric phy framework

From: Kamil Debski
Date: Mon Dec 30 2013 - 08:43:35 EST

Next message: Daniel Lezcano: "[PATCH 0/7] sched: cleanup trigger_load_balance"
Previous message: Julian Andres Klode: "Re: [PATCH 1/4] thinkpad_acpi: Add support for controlling chargethresholds"
In reply to: Vivek Gautam: "Re: [PATCH v5 4/9] usb: ehci-s5p: Change to use phy provided by thegeneric phy framework"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Vivek,

> From: Vivek Gautam [mailto:gautamvivek1987@xxxxxxxxx]
> Sent: Thursday, December 26, 2013 11:14 AM
>
> Hi Kamil,
>
>
> On Fri, Dec 20, 2013 at 6:54 PM, Kamil Debski <k.debski@xxxxxxxxxxx>
> wrote:
> > Change the phy provider used from the old one using the USB phy
> > framework to a new one using the Generic phy framework.
> >
> > Signed-off-by: Kamil Debski <k.debski@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>
> Commit title:
> s/ehci-s5p/ehci-exynos

Thank you for spotting this.

>
> > ---
> > Documentation/devicetree/bindings/usb/usb-ehci.txt | 35 +++++++
> > drivers/usb/host/ehci-exynos.c | 97
> +++++++++++++-------
> > 2 files changed, 98 insertions(+), 34 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> > b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> > index fa18612..413f7cd 100644
> > --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> > @@ -14,6 +14,10 @@ If controller implementation operates with big
> > endian descriptors, If both big endian registers and descriptors are
> > used by the controller implementation, "big-endian" property can be
> > specified instead of having both "big-endian-regs" and "big-endian-
> desc".
> > + - port: if in the SoC there are EHCI phys, they should be listed
> here.
> > +One phy per port. Each port should have its reg entry with a
> > +consecutive number. Also it should contain phys and phy-names
> entries
> > +specifying the phy used by the port.
> >
> > Example (Sequoia 440EPx):
> > ehci@e0000300 {
> > @@ -23,3 +27,34 @@ Example (Sequoia 440EPx):
> > reg = <0 e0000300 90 0 e0000390 70>;
> > big-endian;
> > };
> > +
> > +Example (Exynos 4212):
> > + ehci@12580000 {
> > + compatible = "samsung,exynos4210-ehci";
> > + reg = <0x12580000 0x20000>;
> > + interrupts = <0 70 0>;
> > + clocks = <&clock 304>, <&clock 305>;
> > + clock-names = "usbhost", "otg";
> > + status = "disabled";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + port@0 {
> > + reg = <0>;
> > + phys = <&usb2phy 1>;
> > + phy-names = "host";
> > + status = "disabled";
> > + };
> > + port@1 {
> > + reg = <1>;
> > + phys = <&usb2phy 2>;
> > + phy-names = "hsic0";
> > + status = "disabled";
> > + };
> > + port@2 {
> > + reg = <2>;
> > + phys = <&usb2phy 3>;
> > + phy-names = "hsic1";
> > + status = "disabled";
> > + };
> > + };
>
> Should we place above documentation in
> "Documentation/devicetree/bindings/usb/exynos-usb.txt" ?
> or is it something that i am missing.

Indeed, this should go to exynos-usb.txt instead of usb-ehci.txt.
Thanks!

> > +
> > diff --git a/drivers/usb/host/ehci-exynos.c
> > b/drivers/usb/host/ehci-exynos.c index d1d8c47..7c35501 100644
> > --- a/drivers/usb/host/ehci-exynos.c
> > +++ b/drivers/usb/host/ehci-exynos.c
> > @@ -19,12 +19,12 @@
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_gpio.h>
> > +#include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > #include <linux/usb/phy.h>
> > #include <linux/usb/samsung_usb_phy.h> #include <linux/usb.h>
> > #include <linux/usb/hcd.h> -#include <linux/usb/otg.h>
> >
> > #include "ehci.h"
> >
> > @@ -42,10 +42,10 @@
> > static const char hcd_name[] = "ehci-exynos"; static struct
> > hc_driver __read_mostly exynos_ehci_hc_driver;
> >
> > +#define PHY_NUMBER 3
> > struct exynos_ehci_hcd {
> > struct clk *clk;
> > - struct usb_phy *phy;
> > - struct usb_otg *otg;
> > + struct phy *phy[PHY_NUMBER];
> > };
> >
> > #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
> > *)(hcd_to_ehci(hcd)->priv) @@ -69,13 +69,43 @@ static void
> exynos_setup_vbus_gpio(struct platform_device *pdev)
> > dev_err(dev, "can't request ehci vbus gpio %d",
> gpio);
> > }
> >
> > +static int exynos_phys_on(struct phy *p[]) {
> > + int i;
> > + int ret = 0;
> > +
> > + for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> > + if (p[i])
> > + ret = phy_power_on(p[i]);
> > + if (ret)
> > + for (i--; i > 0; i--)
> > + if (p[i])
> > + phy_power_off(p[i]);
>
> So we are turning off, say, port0 phy in case port1 phy power_on fails;
> can't we still leave a usb2.0 phy(a normal host phy) 'on' in case the
> HSIC phy fails ?

Currently all phys are can be either switched on or off. So if powering on
one phy fails (and exynos_phy_on returns an error code), I would expect
that no phy is switched on. I think that doing otherwise could leave
the phys in strange state - some phys are on, some are off and the power_on
call returned an error.

> > +
> > + return ret;
> > +}
> > +
> > +static int exynos_phys_off(struct phy *p[]) {
> > + int i;
> > + int ret = 0;
> > +
> > + for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> > + if (p[i])
> > + ret = phy_power_off(p[i]);
> > +
> > + return ret;
> > +}
> > +
> > static int exynos_ehci_probe(struct platform_device *pdev) {
> > struct exynos_ehci_hcd *exynos_ehci;
> > struct usb_hcd *hcd;
> > struct ehci_hcd *ehci;
> > struct resource *res;
> > - struct usb_phy *phy;
> > + struct phy *phy;
> > + struct device_node *child;
> > + int phy_number;
> > int irq;
> > int err;
> >
> > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct
> platform_device *pdev)
> > "samsung,exynos5440-ehci"))
> > goto skip_phy;
> >
> > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
> > - if (IS_ERR(phy)) {
> > - usb_put_hcd(hcd);
> > - dev_warn(&pdev->dev, "no platform data or transceiver
> defined\n");
> > - return -EPROBE_DEFER;
> > - } else {
> > - exynos_ehci->phy = phy;
> > - exynos_ehci->otg = phy->otg;
> > + for_each_available_child_of_node(pdev->dev.of_node, child) {
> > + err = of_property_read_u32(child, "reg",
> &phy_number);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to parse device
> tree\n");
> > + of_node_put(child);
> > + return err;
> > + }
> > + if (phy_number >= PHY_NUMBER) {
> > + dev_err(&pdev->dev, "Failed to parse device
> tree - number out of range\n");
> > + of_node_put(child);
> > + return -EINVAL;
> > + }
> > + phy = devm_of_phy_get(&pdev->dev, child, 0);
> > + of_node_put(child);
> > + if (IS_ERR(phy)) {
> > + dev_err(&pdev->dev, "Failed to get phy number
> %d",
> > +
> phy_number);
> > + return PTR_ERR(phy);
> > + }
> > + exynos_ehci->phy[phy_number] = phy;
> > }
> >
> > skip_phy:
> > @@ -149,11 +191,11 @@ skip_phy:
> > goto fail_io;
> > }
> >
> > - if (exynos_ehci->otg)
> > - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd-
> >self);
> > -
> > - if (exynos_ehci->phy)
> > - usb_phy_init(exynos_ehci->phy);
> > + err = exynos_phys_on(exynos_ehci->phy);
> > + if (err) {
> > + dev_err(&pdev->dev, "Failed to enabled phys\n");
> > + goto fail_io;
> > + }
> >
> > ehci = hcd_to_ehci(hcd);
> > ehci->caps = hcd->regs;
> > @@ -173,8 +215,7 @@ skip_phy:
> > return 0;
> >
> > fail_add_hcd:
> > - if (exynos_ehci->phy)
> > - usb_phy_shutdown(exynos_ehci->phy);
> > + exynos_phys_off(exynos_ehci->phy);
> > fail_io:
> > clk_disable_unprepare(exynos_ehci->clk);
> > fail_clk:
> > @@ -189,11 +230,7 @@ static int exynos_ehci_remove(struct
> > platform_device *pdev)
> >
> > usb_remove_hcd(hcd);
> >
> > - if (exynos_ehci->otg)
> > - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd-
> >self);
> > -
> > - if (exynos_ehci->phy)
> > - usb_phy_shutdown(exynos_ehci->phy);
> > + exynos_phys_off(exynos_ehci->phy);
> >
> > clk_disable_unprepare(exynos_ehci->clk);
> >
> > @@ -213,11 +250,7 @@ static int exynos_ehci_suspend(struct device
> > *dev)
> >
> > rc = ehci_suspend(hcd, do_wakeup);
> >
> > - if (exynos_ehci->otg)
> > - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd-
> >self);
> > -
> > - if (exynos_ehci->phy)
> > - usb_phy_shutdown(exynos_ehci->phy);
> > + exynos_phys_off(exynos_ehci->phy);
> >
> > clk_disable_unprepare(exynos_ehci->clk);
> >
> > @@ -231,11 +264,7 @@ static int exynos_ehci_resume(struct device
> *dev)
> >
> > clk_prepare_enable(exynos_ehci->clk);
> >
> > - if (exynos_ehci->otg)
> > - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd-
> >self);
> > -
> > - if (exynos_ehci->phy)
> > - usb_phy_init(exynos_ehci->phy);
> > + exynos_phys_on(exynos_ehci->phy);
> >
> > /* DMA burst Enable */
> > writel(EHCI_INSNREG00_ENABLE_DMA_BURST,
> > EHCI_INSNREG00(hcd->regs));
> > --
>
> Rest all looks good. :-)
> I tested this patch along with other patches in the series on smdk5250.

Thank you :)

> Tested-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
>

Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland

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

Next message: Daniel Lezcano: "[PATCH 0/7] sched: cleanup trigger_load_balance"
Previous message: Julian Andres Klode: "Re: [PATCH 1/4] thinkpad_acpi: Add support for controlling chargethresholds"
In reply to: Vivek Gautam: "Re: [PATCH v5 4/9] usb: ehci-s5p: Change to use phy provided by thegeneric phy framework"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]