Hi Sylwester,


On Wed, Dec 19, 2012 at 11:05 AM, Vivek Gautam
[off-list ref] wrote:

CC: Doug Anderson


On Wed, Dec 19, 2012 at 4:49 AM, Sylwester Nawrocki
[off-list ref] wrote:

quoted

Hi Vivek,


On 12/18/2012 02:56 PM, Vivek Gautam wrote:

quoted

Adding support to parse device node data in order to get
required properties to set pmu isolation for usb-phy.

Signed-off-by: Vivek Gautam<redacted>
---

Changes from v1:
  - Changed the name of property for phy handler from'samsung,usb-phyctrl'
    to 'samsung,usb-phyhandle' to make it look more generic.
  - Similarly 'samsung,phyctrl-reg' is changed to 'samsung,phyhandle-reg'
  - Added a check for 'samsung,usb-phyhandle' before getting node from
    phandle.
  - Putting the node using 'of_node_put()' which had been missed.
  - Adding necessary check for the pointer in
'samsung_usbphy_set_isolation()'
    to avoid any NULL pointer dereferencing.
  - Unmapping the register ioremapped in
'samsung_usbphy_parse_dt_param()'.


  .../devicetree/bindings/usb/samsung-usbphy.txt     |   12 +++
  drivers/usb/phy/samsung-usbphy.c                   |   94
++++++++++++++++++--
  2 files changed, 98 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt

b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
index 7b26e2d..a7b28b2 100644

--- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
+++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt

@@ -9,3 +9,15 @@ Required properties:
  - compatible : should be "samsung,exynos4210-usbphy"
  - reg : base physical address of the phy registers and length of memory

mapped
        region.
+
+Optional properties:
+- samsung,usb-phyhandle : should point to usb-phyhandle sub-node which
provides
+                       binding data to enable/disable device PHY handled
by
+                       PMU register.
+
+                       Required properties:
+                       - compatible : should be "samsung,usbdev-phyctrl"
for
+                                       DEVICE type phy.
+                       - samsung,phyhandle-reg: base physical address of
+                                               PHY_CONTROL register in
PMU.
+- samsung,enable-mask : should be '1'

This should only be 1 for Exynos4210+ SoCs, right ?

Yes that's true Exynso4210+ SoCs have only single PHY for both host and device
which gets enabled by single bit.

quoted

S5PV210 uses bit 0 for OTG and bit1 for USB host, doesn't it ? And for
s3c64xx
it seems to be bit 16.

True, S5PV210 uses two bits separately for OTG and HOST, so in that
case we would
require to set both these bits. Thanks for pointing out !!

I couldn't see device tree support for S5PV210 and S3C64xx so thought
of going for
4210+ SoCs. Better we make this more generic so that once these SoCs
are moved to
device tree we don't face any issue. Right ??

quoted

How about deriving this information from 'compatible' property instead ?

It will definitely be good to use the compatible property to derive
such information,
Need to amend the current approach.

quoted

Maybe you could just encode the USB PMU registers (I assume those aren't
touched by anything but the usb drivers) in a regular 'reg' property in
an usbphy subnode. Then the driver could interpret it also with help
of 'compatible' property. And you could just use of_iomap(). E.g.

 usbphy@12130000 {
        compatible = "samsung,exynos5250-usbphy";
        reg = <0x12130000 0x100>, <0x12100000 0x100>;
        usbphy-pmu {
                /* USB device and host PHY_CONTROL registers */
                reg = <0x10040704 8>;
        };
 };

This approach seems nice.

quoted

Your "samsung,usb-phyhandle" approach seems over-engineered to me.
I might be missing something though.

The idea behind using phandles for usb-phy was to get the multiple
registers (2 in PMU
and 1 in SYSREG) and program them separately as required.

quoted

quoted

diff --git a/drivers/usb/phy/samsung-usbphy.c

b/drivers/usb/phy/samsung-usbphy.c
index 5c5e1bb5..4ceabe3 100644

--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c

@@ -72,6 +72,8 @@ enum samsung_cpu_type {
   * @dev: The parent device supplied to the probe function
   * @clk: usb phy clock
   * @regs: usb phy register memory base
+ * @devctrl_reg: usb device phy-control pmu register memory base

hum, this name is not really helpful in understanding what's going
on here.

Looking at arch/arm/mach-s5pv210/setup-usb-phy.c, there is only one
PHY_CONTROL (Power Management Unit) register for both OTG and USB host
PHYs. So are you really taking care of that case as well ?

This doesn't take care of s3pv210. Will  amend this to ensure that.

quoted

quoted

+ * @en_mask: enable mask
   * @ref_clk_freq: reference clock frequency selection
   * @cpu_type: machine identifier
   */

@@ -81,12 +83,73 @@ struct samsung_usbphy {
        struct device   *dev;
        struct clk      *clk;
        void __iomem    *regs;
+       void __iomem    *devctrl_reg;
+       u32             en_mask;
        int             ref_clk_freq;
        int             cpu_type;
  };

  #define phy_to_sphy(x)                container_of((x), struct

samsung_usbphy, phy)

+static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
+{
+       struct device_node *usb_phyctrl;
+       u32 reg;
+       int lenp;
+
+       if (!sphy->dev->of_node) {
+               sphy->devctrl_reg = NULL;
+               return -ENODEV;
+       }
+
+       if (of_get_property(sphy->dev->of_node,
"samsung,usb-phyhandle",&lenp)) {
+               usb_phyctrl = of_parse_phandle(sphy->dev->of_node,
+                                               "samsung,usb-phyhandle",
0);
+               if (!usb_phyctrl) {
+                       dev_warn(sphy->dev, "Can't get usb-phy handle\n");
+                       sphy->devctrl_reg = NULL;
+               }
+
+               of_property_read_u32(usb_phyctrl,
"samsung,phyhandle-reg",&reg);
+
+               sphy->devctrl_reg = ioremap(reg, SZ_4);

What happens if invalid address value is assigned to 'samsung,phyhandle-reg'
?

Oops!! need to add an pointer check here.

quoted

quoted

+               of_property_read_u32(sphy->dev->of_node,
"samsung,enable-mask",
+                                                       &sphy->en_mask);
+               of_node_put(usb_phyctrl);
+       } else {
+               dev_warn(sphy->dev, "Can't get usb-phy handle\n");
+               sphy->devctrl_reg = NULL;
+       }
+
+       return 0;
+}
+
+/*
+ * Set isolation here for phy.
+ * SOCs control this by controlling corresponding PMU registers
+ */
+static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int
on)
+{
+       void __iomem *usb_phyctrl_reg;
+       u32 en_mask = sphy->en_mask;
+       u32 reg;
+
+       usb_phyctrl_reg = sphy->devctrl_reg;
+
+       if (!usb_phyctrl_reg) {
+               dev_warn(sphy->dev, "Can't set pmu isolation\n");
+               return;
+       }
+
+       reg = readl(usb_phyctrl_reg);
+
+       if (on)
+               writel(reg&  ~en_mask, usb_phyctrl_reg);

+       else
+               writel(reg | en_mask, usb_phyctrl_reg);
+}
+
  /*
   * Returns reference clock frequency selection value
   */

@@ -199,6 +262,8 @@ static int samsung_usbphy_init(struct usb_phy *phy)
        /* Disable phy isolation */
        if (sphy->plat&&  sphy->plat->pmu_isolation)

                sphy->plat->pmu_isolation(false);
+       else
+               samsung_usbphy_set_isolation(sphy, false);

        /* Initialize usb phy registers */
        samsung_usbphy_enable(sphy);

@@ -228,6 +293,8 @@ static void samsung_usbphy_shutdown(struct usb_phy

*phy)
        /* Enable phy isolation */
        if (sphy->plat&&  sphy->plat->pmu_isolation)

                sphy->plat->pmu_isolation(true);
+       else
+               samsung_usbphy_set_isolation(sphy, true);

        clk_disable_unprepare(sphy->clk);
  }

@@ -249,17 +316,12 @@ static inline int

samsung_usbphy_get_driver_data(struct platform_device *pdev)
  static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
  {
        struct samsung_usbphy *sphy;
-       struct samsung_usbphy_data *pdata;
+       struct samsung_usbphy_data *pdata = pdev->dev.platform_data;
        struct device *dev =&pdev->dev;

        struct resource *phy_mem;
        void __iomem    *phy_base;
        struct clk *clk;
-
-       pdata = pdev->dev.platform_data;
-       if (!pdata) {
-               dev_err(&pdev->dev, "%s: no platform data defined\n",
__func__);
-               return -EINVAL;
-       }
+       int ret;

        phy_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!phy_mem) {

@@ -283,7 +345,20 @@ static int __devinit samsung_usbphy_probe(struct

platform_device *pdev)
                return PTR_ERR(clk);
        }

-       sphy->dev               =&pdev->dev;
+       sphy->dev =&pdev->dev;

        sphy->dev = dev;

Right, will amend this.

quoted

quoted

+
+       ret = samsung_usbphy_parse_dt_param(sphy);
+       if (ret) {
+               /* fallback to pdata */
+               if (!pdata) {
+                       dev_err(&pdev->dev,
+                               "%s: no device data found\n", __func__);

I find term "device data" a bit confusing here.

quoted

+                       return -ENODEV;

In the original code -EINVAL was returned when platform_data was not set.

quoted

+               } else {
+                       sphy->plat = pdata;
+               }
+       }
+

How about rewriting this to:

        if (dev->of_node) {
                ret = samsung_usbphy_parse_dt_param(sphy);
                if (ret < 0)
                        return ret;
        } else {
                if (!pdata) {
                        dev_err(dev, "no platform data specified\n");
                        return -EINVAL;
                }
        }

This way we won't be obfuscating any error code returned from the
OF parsing function.

Sure, will amend this.

quoted

quoted

        sphy->plat              = pdata;
        sphy->regs              = phy_base;
        sphy->clk               = clk;

@@ -305,6 +380,9 @@ static int __exit samsung_usbphy_remove(struct

platform_device *pdev)

        usb_remove_phy(&sphy->phy);

+       if (sphy->devctrl_reg)
+               iounmap(sphy->devctrl_reg);
+
        return 0;
  }

--

Regards,
Sylwester

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

--
Thanks & Regards
Vivek

-- 
Thanks & Regards
Vivek