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

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

[Vivek Gautam]

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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 .../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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---

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

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

[Sylwester Nawrocki]

Hi Vivek,

On 12/18/2012 02:56 PM, Vivek Gautam wrote:
> 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<gautam.vivek@samsung.com>
> ---
>
> 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.

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

> + * @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' ?

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

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

>   	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

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

[Vivek Gautam]

CC: Doug Anderson


On Wed, Dec 19, 2012 at 4:49 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi Vivek,
>
>
> On 12/18/2012 02:56 PM, Vivek Gautam wrote:
>>
>> 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<gautam.vivek@samsung.com>
>> ---
>>
>> 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.
>
>
>> 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 ?
>
>
>> + * @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'
> ?
>
>> +               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;
>
>
>> +
>> +       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.
>
>
>>         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

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

[Vivek Gautam]

Hi Sylwester,


On Wed, Dec 19, 2012 at 11:05 AM, Vivek Gautam
<gautamvivek1987@gmail.com> wrote:
> CC: Doug Anderson
>
>
> On Wed, Dec 19, 2012 at 4:49 AM, Sylwester Nawrocki
> <sylvester.nawrocki@gmail.com> wrote:
>> Hi Vivek,
>>
>>
>> On 12/18/2012 02:56 PM, Vivek Gautam wrote:
>>>
>>> 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<gautam.vivek@samsung.com>
>>> ---
>>>
>>> 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.

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

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

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

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

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

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

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

>>
>>> +
>>> +       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.
>>
Sure, will amend this.

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

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

[Sylwester Nawrocki]

Hi,

On 12/19/2012 02:44 PM, Vivek Gautam wrote:
>>>> --- 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.
>
>>> 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.

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