[PATCH] usb: phy: samsung: Add support to set pmu isolation

Based on patches for samsung-usbphy driver available at:
https://patchwork.kernel.org/patch/1794651/

In this patch we are adding support to parse device tree data for
samsung-usbphy driver and further setting pmu_isolation to
enable/disable phy as and when needed.
This further chucks out the need of platform data for samsung-usbphy on
DT enabled system and hence serves the purpose of the discussion
in the thread for:
[PATCH v8 2/2] usb: s3c-hsotg: Adding phy driver support

Vivek Gautam (1):
  usb: phy: samsung: Add support to set pmu isolation

 .../devicetree/bindings/usb/samsung-usbphy.txt     |   10 +++
 drivers/usb/phy/samsung-usbphy.c                   |   80 ++++++++++++++++++--
 2 files changed, 82 insertions(+), 8 deletions(-)

-- 
1.7.6.5

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>
---
 .../devicetree/bindings/usb/samsung-usbphy.txt     |   10 +++
 drivers/usb/phy/samsung-usbphy.c                   |   80 ++++++++++++++++++--
 2 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
index 7b26e2d..112eaa6 100644
--- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
+++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt

@@ -9,3 +9,13 @@ Required properties:
 - compatible : should be "samsung,exynos4210-usbphy"
 - reg : base physical address of the phy registers and length of memory mapped
 	region.
+- samsung,usb-phyctrl : should point to usb-phyctrl 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,phyctrl-reg: base physical address of
+						PHY_CONTROL register in PMU.
+- samsung,enable-mask : should be '1'

diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 5c5e1bb5..ef394c3 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 phy-control pmu register memory base
+ * @en_mask: enable mask
  * @ref_clk_freq: reference clock frequency selection
  * @cpu_type: machine identifier
  */

@@ -81,12 +83,62 @@ 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;
+
+	if (!sphy->dev->of_node) {
+		sphy->devctrl_reg = NULL;
+		return -ENODEV;
+	}
+
+	usb_phyctrl = of_parse_phandle(sphy->dev->of_node,
+					"samsung,usb-phyctrl", 0);
+	if (!usb_phyctrl) {
+		dev_dbg(sphy->dev, "Can't get usb-phy control node\n");
+		sphy->devctrl_reg = NULL;
+		return -ENODEV;
+	}
+
+	of_property_read_u32(usb_phyctrl, "samsung,phyctrl-reg", &reg);
+
+	sphy->devctrl_reg = ioremap(reg, SZ_4);
+
+	of_property_read_u32(sphy->dev->of_node, "samsung,enable-mask",
+							&sphy->en_mask);
+
+	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;
+
+	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 +251,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 +282,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 +305,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 +334,20 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
 		return PTR_ERR(clk);
 	}
 
-	sphy->dev		= &pdev->dev;
+	sphy->dev = &pdev->dev;
+
+	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__);
+			return -ENODEV;
+		} else {
+			sphy->plat = pdata;
+		}
+	}
+
 	sphy->plat		= pdata;
 	sphy->regs		= phy_base;
 	sphy->clk		= clk;

-- 
1.7.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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'

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
+ * @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);
+
+		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;
+
+	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__);
+			return -ENODEV;
+		} else {
+			sphy->plat = pdata;
+		}
+	}
+
 	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;
 }
 

-- 
1.7.6.5

Hi Vivek,

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

This should only be 1 for Exynos4210+ SoCs, right ?
S5PV210 uses bit 0 for OTG and bit1 for USB host, doesn't it ? And for 
s3c64xx
it seems to be bit 16.

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

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>;
	};
  };

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

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 ?

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

	sphy->dev = dev;

+
+	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.

+			return -ENODEV;

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

+		} 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.

--

Regards,
Sylwester

CC: Doug Anderson


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

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 ?
S5PV210 uses bit 0 for OTG and bit1 for USB host, doesn't it ? And for
s3c64xx
it seems to be bit 16.

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

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>;
        };
 };

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

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 ?

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'
?

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;

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.

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-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

-- 
Thanks & Regards
Vivek

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

Hi,

On 12/19/2012 02:44 PM, Vivek Gautam wrote:

quoted

quoted

quoted

--- 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

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 ??

Fair enough. Yes, let's not make any future addition of the device tree
support for the older Samsung platforms unnecessarily difficult. It doesn't
take much effort, and there is many drivers that are shared by multiple 
SoCs
already, starting from s3c64xx to exynos4 series.

quoted

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

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

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.

You could still specify multiple <address, size> pairs in 'reg' property
or perhaps use separate node for SYSREG. And if on some SoCs PHY_CONTROL
registers do not immediately follow each other in memory it might make
sense to define the bindings so that each register is specified separately,
e.g.

reg = <0x10040704 4>, <0x10040708 4>, <0x10050230 4>;

However AFAICS single region for the PHY_CONTROL registers should be
sufficient for all existing SoCs.

--

Thanks,
Sylwester