[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

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

[Vivek Gautam]

Hi Doug,


On Tue, Jan 15, 2013 at 3:41 AM, Doug Anderson <dianders@chromium.org> wrote:
> Vivek,
>
> Sorry for being so absent from these reviews.  I'll try to look over a
> few patches today, but please don't hold up anything on account of my
> reviews.  I'm definitely a bit of an interested bystander in USB land.
>  ;)
>
> In general things look pretty good here.  :)  One last comment below...
>
> On Fri, Jan 11, 2013 at 12:08 AM, Vivek Gautam
> <gautam.vivek@samsung.com> wrote:> +static int
> samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
>> +{
>> +       struct device_node *usbphy_sys;
>> +
>> +       /* Getting node for system controller interface for usb-phy */
>> +       usbphy_sys = of_get_child_by_name(sphy->dev->of_node, "usbphy-sys");
>> +       if (!usbphy_sys)
>> +               dev_warn(sphy->dev, "No sys-controller interface for usb-phy\n");
>
> Seems like you ought to return with an error here.  Calling of_iomap()
> with a NULL value seems undesirable.
>

Yeah, true. This should have been returning error value alongwith dev_err().

>> +
>> +       sphy->pmuregs = of_iomap(usbphy_sys, 0);
>> +
>> +       of_node_put(usbphy_sys);
>> +
>> +       if (sphy->pmuregs == NULL) {
>> +               dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Set isolation here for phy.
>> + * Here 'on = true' would mean USB PHY block is isolated, hence
>> + * de-activated and vice-versa.
>> + */
>
> Thank you very much for this comment.  :)  This explains one of the
> confusions I had earlier...
>
Your welcome :-)
>
> Once you fix the one error condition above you can add my
> "Reviewed-by".  Thanks!
>
Sure, thanks !!



-- 
Thanks & Regards
Vivek

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

[Doug Anderson]

Vivek,

Sorry for being so absent from these reviews.  I'll try to look over a
few patches today, but please don't hold up anything on account of my
reviews.  I'm definitely a bit of an interested bystander in USB land.
 ;)

In general things look pretty good here.  :)  One last comment below...

On Fri, Jan 11, 2013 at 12:08 AM, Vivek Gautam
<gautam.vivek@samsung.com> wrote:> +static int
samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
> +{
> +       struct device_node *usbphy_sys;
> +
> +       /* Getting node for system controller interface for usb-phy */
> +       usbphy_sys = of_get_child_by_name(sphy->dev->of_node, "usbphy-sys");
> +       if (!usbphy_sys)
> +               dev_warn(sphy->dev, "No sys-controller interface for usb-phy\n");

Seems like you ought to return with an error here.  Calling of_iomap()
with a NULL value seems undesirable.

> +
> +       sphy->pmuregs = of_iomap(usbphy_sys, 0);
> +
> +       of_node_put(usbphy_sys);
> +
> +       if (sphy->pmuregs == NULL) {
> +               dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * Set isolation here for phy.
> + * Here 'on = true' would mean USB PHY block is isolated, hence
> + * de-activated and vice-versa.
> + */

Thank you very much for this comment.  :)  This explains one of the
confusions I had earlier...


Once you fix the one error condition above you can add my
"Reviewed-by".  Thanks!

-Doug

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

[Vivek Gautam]

Hi Sylwester,


On Fri, Jan 11, 2013 at 2:51 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 01/11/2013 09:08 AM, 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>
>
> Thanks for addressing my all comments,
>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>

Thanks for the review and help. :-)


-- 
Thanks & Regards
Vivek

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

[Sylwester Nawrocki]

On 01/11/2013 09:08 AM, 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>

Thanks for addressing my all comments,

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

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

Changes from v5:
 - Using a global spin_lock member in 'samsung_usbphy' structure to be
   used in samsung_usbphy_init() and samsung_usbphy_shutdown()
   to take care of all register initialization sequence.
 - Addressing few nits:
        - Using devphy_reg_offset instead of 'pmureg_devphy_offset'
        - Using if else block instead of ternary conditional statement
          in samsung_usbphy_set_isolation()
	- Using 'bool' type instead of 'int' for 'on' argument in
	  samsung_usbphy_set_isolation()
        - Amending few comments.

 .../devicetree/bindings/usb/samsung-usbphy.txt     |   36 +++++
 drivers/usb/phy/samsung-usbphy.c                   |  161 +++++++++++++++++---
 2 files changed, 175 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
index 7b26e2d..22d06cf 100644
--- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
+++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
@@ -9,3 +9,39 @@ Required properties:
 - compatible : should be "samsung,exynos4210-usbphy"
 - reg : base physical address of the phy registers and length of memory mapped
 	region.
+
+Optional properties:
+- #address-cells: should be '1' when usbphy node has a child node with 'reg'
+		  property.
+- #size-cells: should be '1' when usbphy node has a child node with 'reg'
+	       property.
+- ranges: allows valid translation between child's address space and parent's
+	  address space.
+
+- The child node 'usbphy-sys' to the node 'usbphy' is for the system controller
+  interface for usb-phy. It should provide the following information required by
+  usb-phy controller to control phy.
+  - reg : base physical address of PHY_CONTROL registers.
+	  The size of this register is the total sum of size of all PHY_CONTROL
+	  registers that the SoC has. For example, the size will be
+	  '0x4' in case we have only one PHY_CONTROL register (e.g.
+	  OTHERS register in S3C64XX or USB_PHY_CONTROL register in S5PV210)
+	  and, '0x8' in case we have two PHY_CONTROL registers (e.g.
+	  USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers in exynos4x).
+	  and so on.
+
+Example:
+ - Exynos4210
+
+	usbphy@125B0000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "samsung,exynos4210-usbphy";
+		reg = <0x125B0000 0x100>;
+		ranges;
+
+		usbphy-sys {
+			/* USB device and host PHY_CONTROL registers */
+			reg = <0x10020704 0x8>;
+		};
+	};
diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 5c5e1bb5..7eec7c3 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/usb/otg.h>
 #include <linux/platform_data/samsung-usbphy.h>
 
@@ -60,20 +61,46 @@
 #define MHZ (1000*1000)
 #endif
 
+#define S3C64XX_USBPHY_ENABLE			(0x1 << 16)
+#define EXYNOS_USBPHY_ENABLE			(0x1 << 0)
+
 enum samsung_cpu_type {
 	TYPE_S3C64XX,
 	TYPE_EXYNOS4210,
 };
 
 /*
+ * struct samsung_usbphy_drvdata - driver data for various SoC variants
+ * @cpu_type: machine identifier
+ * @devphy_en_mask: device phy enable mask for PHY CONTROL register
+ * @devphy_reg_offset: offset to DEVICE PHY CONTROL register from
+ *		       mapped address of system controller.
+ *
+ *	Here we have a separate mask for device type phy.
+ *	Having different masks for host and device type phy helps
+ *	in setting independent masks in case of SoCs like S5PV210,
+ *	in which PHY0 and PHY1 enable bits belong to same register
+ *	placed at position 0 and 1 respectively.
+ *	Although for newer SoCs like exynos these bits belong to
+ *	different registers altogether placed at position 0.
+ */
+struct samsung_usbphy_drvdata {
+	int cpu_type;
+	int devphy_en_mask;
+	u32 devphy_reg_offset;
+};
+
+/*
  * struct samsung_usbphy - transceiver driver state
  * @phy: transceiver structure
  * @plat: platform data
  * @dev: The parent device supplied to the probe function
  * @clk: usb phy clock
- * @regs: usb phy register memory base
+ * @regs: usb phy controller registers memory base
+ * @pmuregs: USB device PHY_CONTROL register memory base
  * @ref_clk_freq: reference clock frequency selection
- * @cpu_type: machine identifier
+ * @drv_data: driver data available for different SoCs
+ * @lock: lock for phy operations
  */
 struct samsung_usbphy {
 	struct usb_phy	phy;
@@ -81,12 +108,64 @@ struct samsung_usbphy {
 	struct device	*dev;
 	struct clk	*clk;
 	void __iomem	*regs;
+	void __iomem	*pmuregs;
 	int		ref_clk_freq;
-	int		cpu_type;
+	const struct samsung_usbphy_drvdata *drv_data;
+	spinlock_t	lock;
 };
 
 #define phy_to_sphy(x)		container_of((x), struct samsung_usbphy, phy)
 
+static int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
+{
+	struct device_node *usbphy_sys;
+
+	/* Getting node for system controller interface for usb-phy */
+	usbphy_sys = of_get_child_by_name(sphy->dev->of_node, "usbphy-sys");
+	if (!usbphy_sys)
+		dev_warn(sphy->dev, "No sys-controller interface for usb-phy\n");
+
+	sphy->pmuregs = of_iomap(usbphy_sys, 0);
+
+	of_node_put(usbphy_sys);
+
+	if (sphy->pmuregs == NULL) {
+		dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/*
+ * Set isolation here for phy.
+ * Here 'on = true' would mean USB PHY block is isolated, hence
+ * de-activated and vice-versa.
+ */
+static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, bool on)
+{
+	void __iomem *reg;
+	u32 reg_val;
+	u32 en_mask;
+
+	if (!sphy->pmuregs) {
+		dev_warn(sphy->dev, "Can't set pmu isolation\n");
+		return;
+	}
+
+	reg = sphy->pmuregs + sphy->drv_data->devphy_reg_offset;
+	en_mask = sphy->drv_data->devphy_en_mask;
+
+	reg_val = readl(reg);
+
+	if (on)
+		reg_val &= ~en_mask;
+	else
+		reg_val |= en_mask;
+
+	writel(reg_val, reg);
+}
+
 /*
  * Returns reference clock frequency selection value
  */
@@ -112,7 +191,7 @@ static int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy)
 		refclk_freq = PHYCLK_CLKSEL_48M;
 		break;
 	default:
-		if (sphy->cpu_type == TYPE_S3C64XX)
+		if (sphy->drv_data->cpu_type == TYPE_S3C64XX)
 			refclk_freq = PHYCLK_CLKSEL_48M;
 		else
 			refclk_freq = PHYCLK_CLKSEL_24M;
@@ -135,7 +214,7 @@ static void samsung_usbphy_enable(struct samsung_usbphy *sphy)
 	phypwr = readl(regs + SAMSUNG_PHYPWR);
 	rstcon = readl(regs + SAMSUNG_RSTCON);
 
-	switch (sphy->cpu_type) {
+	switch (sphy->drv_data->cpu_type) {
 	case TYPE_S3C64XX:
 		phyclk &= ~PHYCLK_COMMON_ON_N;
 		phypwr &= ~PHYPWR_NORMAL_MASK;
@@ -165,7 +244,7 @@ static void samsung_usbphy_disable(struct samsung_usbphy *sphy)
 
 	phypwr = readl(regs + SAMSUNG_PHYPWR);
 
-	switch (sphy->cpu_type) {
+	switch (sphy->drv_data->cpu_type) {
 	case TYPE_S3C64XX:
 		phypwr |= PHYPWR_NORMAL_MASK;
 		break;
@@ -185,6 +264,7 @@ static void samsung_usbphy_disable(struct samsung_usbphy *sphy)
 static int samsung_usbphy_init(struct usb_phy *phy)
 {
 	struct samsung_usbphy *sphy;
+	unsigned long flags;
 	int ret = 0;
 
 	sphy = phy_to_sphy(phy);
@@ -196,13 +276,19 @@ static int samsung_usbphy_init(struct usb_phy *phy)
 		return ret;
 	}
 
+	spin_lock_irqsave(&sphy->lock, flags);
+
 	/* 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);
 
+	spin_unlock_irqrestore(&sphy->lock, flags);
+
 	/* Disable the phy clock */
 	clk_disable_unprepare(sphy->clk);
 	return ret;
@@ -214,6 +300,7 @@ static int samsung_usbphy_init(struct usb_phy *phy)
 static void samsung_usbphy_shutdown(struct usb_phy *phy)
 {
 	struct samsung_usbphy *sphy;
+	unsigned long flags;
 
 	sphy = phy_to_sphy(phy);
 
@@ -222,44 +309,47 @@ static void samsung_usbphy_shutdown(struct usb_phy *phy)
 		return;
 	}
 
+	spin_lock_irqsave(&sphy->lock, flags);
+
 	/* De-initialize usb phy registers */
 	samsung_usbphy_disable(sphy);
 
 	/* Enable phy isolation */
 	if (sphy->plat && sphy->plat->pmu_isolation)
 		sphy->plat->pmu_isolation(true);
+	else
+		samsung_usbphy_set_isolation(sphy, true);
+
+	spin_unlock_irqrestore(&sphy->lock, flags);
 
 	clk_disable_unprepare(sphy->clk);
 }
 
 static const struct of_device_id samsung_usbphy_dt_match[];
 
-static inline int samsung_usbphy_get_driver_data(struct platform_device *pdev)
+static inline const struct samsung_usbphy_drvdata
+*samsung_usbphy_get_driver_data(struct platform_device *pdev)
 {
 	if (pdev->dev.of_node) {
 		const struct of_device_id *match;
 		match = of_match_node(samsung_usbphy_dt_match,
 							pdev->dev.of_node);
-		return (int) match->data;
+		return match->data;
 	}
 
-	return platform_get_device_id(pdev)->driver_data;
+	return (struct samsung_usbphy_drvdata *)
+				platform_get_device_id(pdev)->driver_data;
 }
 
 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 +373,19 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
 		return PTR_ERR(clk);
 	}
 
-	sphy->dev		= &pdev->dev;
+	sphy->dev = dev;
+
+	if (dev->of_node) {
+		ret = samsung_usbphy_parse_dt(sphy);
+		if (ret < 0)
+			return ret;
+	} else {
+		if (!pdata) {
+			dev_err(dev, "no platform data specified\n");
+			return -EINVAL;
+		}
+	}
+
 	sphy->plat		= pdata;
 	sphy->regs		= phy_base;
 	sphy->clk		= clk;
@@ -291,9 +393,11 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
 	sphy->phy.label		= "samsung-usbphy";
 	sphy->phy.init		= samsung_usbphy_init;
 	sphy->phy.shutdown	= samsung_usbphy_shutdown;
-	sphy->cpu_type		= samsung_usbphy_get_driver_data(pdev);
+	sphy->drv_data		= samsung_usbphy_get_driver_data(pdev);
 	sphy->ref_clk_freq	= samsung_usbphy_get_refclk_freq(sphy);
 
+	spin_lock_init(&sphy->lock);
+
 	platform_set_drvdata(pdev, sphy);
 
 	return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
@@ -305,17 +409,30 @@ static int __exit samsung_usbphy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&sphy->phy);
 
+	if (sphy->pmuregs)
+		iounmap(sphy->pmuregs);
+
 	return 0;
 }
 
+static const struct samsung_usbphy_drvdata usbphy_s3c64xx = {
+	.cpu_type		= TYPE_S3C64XX,
+	.devphy_en_mask		= S3C64XX_USBPHY_ENABLE,
+};
+
+static const struct samsung_usbphy_drvdata usbphy_exynos4 = {
+	.cpu_type		= TYPE_EXYNOS4210,
+	.devphy_en_mask		= EXYNOS_USBPHY_ENABLE,
+};
+
 #ifdef CONFIG_OF
 static const struct of_device_id samsung_usbphy_dt_match[] = {
 	{
 		.compatible = "samsung,s3c64xx-usbphy",
-		.data = (void *)TYPE_S3C64XX,
+		.data = &usbphy_s3c64xx,
 	}, {
 		.compatible = "samsung,exynos4210-usbphy",
-		.data = (void *)TYPE_EXYNOS4210,
+		.data = &usbphy_exynos4,
 	},
 	{},
 };
@@ -325,10 +442,10 @@ MODULE_DEVICE_TABLE(of, samsung_usbphy_dt_match);
 static struct platform_device_id samsung_usbphy_driver_ids[] = {
 	{
 		.name		= "s3c64xx-usbphy",
-		.driver_data	= TYPE_S3C64XX,
+		.driver_data	= (unsigned long)&usbphy_s3c64xx,
 	}, {
 		.name		= "exynos4210-usbphy",
-		.driver_data	= TYPE_EXYNOS4210,
+		.driver_data	= (unsigned long)&usbphy_exynos4,
 	},
 	{},
 };
-- 
1.7.6.5