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

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

[Vivek Gautam]

Changes form v3:
 - Removing the hostphy_en_mas since this gets used in forthcoming patches
   only when host phy support is added.
 - Resolving few nits:
	- using 'const' specifier for driver data structures.
	- using ARRAY_SIZE() instead of giving magic number
	  for of_property_read_u32_array()

Changes from v2:
 - Removed the phandle type of device node properties, instead using
   sub-nodes now.
 - Removed the property 'samsung,enable-mask' since it is SoC dependent
   (SoCs like S5PV210 and S3C64XX have different bits to enable/disable
   phy controller in comparison to exysno4210 onwards).
 - Maintaining the phy enable mask (which is SoC dependent) for device type phy
   and host type phy in the driver data.
 - Re-structuring to get device properties using sub-nodes for 'usbphy-pmu'

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()'.

Based on usb-next branch with Praveen Paneri's patches on top of it.
- http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/134476.html
- http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/131763.html

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

 .../devicetree/bindings/usb/samsung-usbphy.txt     |   31 ++++
 drivers/usb/phy/samsung-usbphy.c                   |  145 +++++++++++++++++---
 2 files changed, 155 insertions(+), 21 deletions(-)

-- 
1.7.6.5

[PATCH v4] 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     |   31 ++++
 drivers/usb/phy/samsung-usbphy.c                   |  145 +++++++++++++++++---
 2 files changed, 155 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
index 7b26e2d..6b873fd 100644
--- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
+++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
@@ -9,3 +9,34 @@ 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.
+
+- The child node 'usbphy-pmu' to the node usbphy should provide the following
+  information required by usb-phy controller to enable/disable the phy.
+   - reg : base physical address of PHY control register in PMU which
+           enables/disables the phy controller.
+           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 (like in S3C64XX) or
+           '0x8' in case we have two phy-control registers (like in Exynos4210)
+           and so on.
+
+Example:
+ - Exysno4210
+
+	usbphy@125B0000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "samsung,exynos4210-usbphy";
+		reg = <0x125B0000 0x100>;
+
+		usbphy-pmu {
+			/* 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..b9f4f42 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c
@@ -60,20 +60,42 @@
 #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
+ *
+ *	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 [0] and [1] respectively.
+ *	Although for newer SoCs like exynos these bits belong to
+ *	different registers altogether placed at [0].
+ */
+struct samsung_usbphy_drvdata {
+	int cpu_type;
+	int devphy_en_mask;
+};
+
+/*
  * 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
+ * @phyctrl_pmureg: usb device phy-control pmu register memory base
  * @ref_clk_freq: reference clock frequency selection
- * @cpu_type: machine identifier
+ * @drv_data: driver data available for different cpu types
  */
 struct samsung_usbphy {
 	struct usb_phy	phy;
@@ -81,12 +103,67 @@ struct samsung_usbphy {
 	struct device	*dev;
 	struct clk	*clk;
 	void __iomem	*regs;
+	void __iomem	*phyctrl_pmureg;
 	int		ref_clk_freq;
-	int		cpu_type;
+	const struct samsung_usbphy_drvdata *drv_data;
 };
 
 #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 *usbphy_pmu;
+	u32 reg[2];
+	int ret;
+
+	if (!sphy->dev->of_node) {
+		dev_err(sphy->dev, "Can't get usb-phy node\n");
+		return -ENODEV;
+	}
+
+	usbphy_pmu = of_get_child_by_name(sphy->dev->of_node, "usbphy-pmu");
+	if (!usbphy_pmu)
+		dev_warn(sphy->dev, "Can't get usb-phy pmu control node\n");
+
+	ret = of_property_read_u32_array(usbphy_pmu, "reg", reg,
+						ARRAY_SIZE(reg));
+	if (!ret)
+		sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
+
+	of_node_put(usbphy_pmu);
+
+	if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {
+		dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");
+		return -ENODEV;
+	}
+
+	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)
+{
+	u32 reg;
+	int en_mask;
+
+	if (!sphy->phyctrl_pmureg) {
+		dev_warn(sphy->dev, "Can't set pmu isolation\n");
+		return;
+	}
+
+	reg = readl(sphy->phyctrl_pmureg);
+
+	en_mask = sphy->drv_data->devphy_en_mask;
+
+	if (on)
+		writel(reg & ~en_mask, sphy->phyctrl_pmureg);
+	else
+		writel(reg | en_mask, sphy->phyctrl_pmureg);
+}
+
 /*
  * Returns reference clock frequency selection value
  */
@@ -112,7 +189,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 +212,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 +242,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;
@@ -199,6 +276,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,38 +307,37 @@ 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);
 }
 
 static const struct of_device_id samsung_usbphy_dt_match[];
 
-static inline int samsung_usbphy_get_driver_data(struct platform_device *pdev)
+static inline 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 (struct samsung_usbphy_drvdata *) 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 +361,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_param(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,7 +381,7 @@ 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);
 
 	platform_set_drvdata(pdev, sphy);
@@ -305,17 +395,30 @@ static int __exit samsung_usbphy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&sphy->phy);
 
+	if (sphy->phyctrl_pmureg)
+		iounmap(sphy->phyctrl_pmureg);
+
 	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 = (void *)&usbphy_s3c64xx,
 	}, {
 		.compatible = "samsung,exynos4210-usbphy",
-		.data = (void *)TYPE_EXYNOS4210,
+		.data = (void *)&usbphy_exynos4,
 	},
 	{},
 };
@@ -325,10 +428,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

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

[Vivek Gautam]

Hi Sylwester,


On Wed, Dec 26, 2012 at 5:58 PM, Vivek Gautam <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---

Hope these changes align with what architectural changes you had suggested ?



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

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

[Sylwester Nawrocki]

On 12/26/2012 01:28 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>
> ---
>   .../devicetree/bindings/usb/samsung-usbphy.txt     |   31 ++++
>   drivers/usb/phy/samsung-usbphy.c                   |  145 +++++++++++++++++---
>   2 files changed, 155 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> index 7b26e2d..6b873fd 100644
> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> @@ -9,3 +9,34 @@ 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.
> +
> +- The child node 'usbphy-pmu' to the node usbphy should provide the following
> +  information required by usb-phy controller to enable/disable the phy.
> +   - reg : base physical address of PHY control register in PMU which
> +           enables/disables the phy controller.
> +           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 (like in S3C64XX) or
> +           '0x8' in case we have two phy-control registers (like in Exynos4210)
> +           and so on.
> +
> +Example:
> + - Exysno4210

s/Exysno/Exynos

> +
> +	usbphy@125B0000 {
> +		#address-cells =<1>;
> +		#size-cells =<1>;
> +		compatible = "samsung,exynos4210-usbphy";
> +		reg =<0x125B0000 0x100>;
> +
> +		usbphy-pmu {
> +			/* 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..b9f4f42 100644
> --- a/drivers/usb/phy/samsung-usbphy.c
> +++ b/drivers/usb/phy/samsung-usbphy.c
> @@ -60,20 +60,42 @@
>   #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
> + *
> + *	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 [0] and [1] respectively.

"and are placed at positions 0 and 1 respectively" ?

> + *	Although for newer SoCs like exynos these bits belong to
> + *	different registers altogether placed at [0].
> + */
> +struct samsung_usbphy_drvdata {
> +	int cpu_type;
> +	int devphy_en_mask;
> +};
> +
> +/*
>    * 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
> + * @phyctrl_pmureg: usb device phy-control pmu register memory base

nit: Perhaps we could just call it "pmureg' ?

>    * @ref_clk_freq: reference clock frequency selection
> - * @cpu_type: machine identifier
> + * @drv_data: driver data available for different cpu types

Actually it's for different SoCs, not CPUs, right ?

>    */
>   struct samsung_usbphy {
>   	struct usb_phy	phy;
> @@ -81,12 +103,67 @@ struct samsung_usbphy {
>   	struct device	*dev;
>   	struct clk	*clk;
>   	void __iomem	*regs;
> +	void __iomem	*phyctrl_pmureg;
>   	int		ref_clk_freq;
> -	int		cpu_type;
> +	const struct samsung_usbphy_drvdata *drv_data;
>   };
>
>   #define phy_to_sphy(x)		container_of((x), struct samsung_usbphy, phy)
>
> +static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)

nit: s/samsung_usbphy_parse_dt_param/samsung_usbphy_parse_dt ?

> +{
> +	struct device_node *usbphy_pmu;
> +	u32 reg[2];
> +	int ret;
> +
> +	if (!sphy->dev->of_node) {
> +		dev_err(sphy->dev, "Can't get usb-phy node\n");
> +		return -ENODEV;

The function is called only when dev->of_node is not NULL, so this path
is never executed, AFAICS. I would just drop this redundant check.

> +	}
> +
> +	usbphy_pmu = of_get_child_by_name(sphy->dev->of_node, "usbphy-pmu");
> +	if (!usbphy_pmu)
> +		dev_warn(sphy->dev, "Can't get usb-phy pmu control node\n");

> +	ret = of_property_read_u32_array(usbphy_pmu, "reg", reg,
> +						ARRAY_SIZE(reg));
> +	if (!ret)
> +		sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);

I don't think this is correct. reg is not really an array of u32 type,
it's an array of address/size tuples. I thought you would use of_iomap()
here. Any reason why you didn't do so ? It would also make it easier to
handle multiple address/size pairs if required.

> +
> +	of_node_put(usbphy_pmu);
> +
> +	if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {

When is sphy->phyctrl_pmureg assigned a ERR_PTR() value ? Wouldn't it
be enough to simply check for NULL ?

> +		dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");
> +		return -ENODEV;
> +	}
> +
> +	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)
> +{
> +	u32 reg;
> +	int en_mask;
> +
> +	if (!sphy->phyctrl_pmureg) {
> +		dev_warn(sphy->dev, "Can't set pmu isolation\n");
> +		return;
> +	}
> +
> +	reg = readl(sphy->phyctrl_pmureg);

To make it more generic maybe it's worth to store an offset to the actual
register somewhere, e.g. in the driver_data struct ? This function is
supposed to handle both device and host PHYs, isn't it ?

> +	en_mask = sphy->drv_data->devphy_en_mask;
> +
> +	if (on)
> +		writel(reg&  ~en_mask, sphy->phyctrl_pmureg);
> +	else
> +		writel(reg | en_mask, sphy->phyctrl_pmureg);

nit: This could be also written as:

	reg = on ? reg & ~mask : reg | mask;
	writel(reg, sphy->phyctrl_pmureg);
> +}
> +
>   /*
>    * Returns reference clock frequency selection value
>    */
> @@ -112,7 +189,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 +212,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 +242,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;
> @@ -199,6 +276,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,38 +307,37 @@ 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);
>   }
>
>   static const struct of_device_id samsung_usbphy_dt_match[];
>
> -static inline int samsung_usbphy_get_driver_data(struct platform_device *pdev)
> +static inline 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 (struct samsung_usbphy_drvdata *) 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 +361,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_param(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;

--

Thanks,
Sylwester

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

[Sylwester Nawrocki]

Hi,

On 12/26/2012 02:56 PM, Vivek Gautam wrote:
> On Wed, Dec 26, 2012 at 5:58 PM, Vivek Gautam<gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>  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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>
> Hope these changes align with what architectural changes you had suggested ?

It looks much better now, thanks! I had a few additional comments, please
see my other reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Russell King - ARM Linux]

On Wed, Dec 26, 2012 at 05:58:32PM +0530, Vivek Gautam wrote:
> +	if (!ret)
> +		sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
> +
> +	of_node_put(usbphy_pmu);
> +
> +	if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {

No.  Learn what the error return values are from functions.  Using the
wrong ones is buggy.  ioremap() only ever returns NULL on error.  You
must check against NULL, and not use the IS_ERR stuff.

> +/*
> + * 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)
> +{
> +	u32 reg;
> +	int en_mask;
> +
> +	if (!sphy->phyctrl_pmureg) {
> +		dev_warn(sphy->dev, "Can't set pmu isolation\n");
> +		return;
> +	}
> +
> +	reg = readl(sphy->phyctrl_pmureg);
> +
> +	en_mask = sphy->drv_data->devphy_en_mask;
> +
> +	if (on)
> +		writel(reg & ~en_mask, sphy->phyctrl_pmureg);
> +	else
> +		writel(reg | en_mask, sphy->phyctrl_pmureg);

What guarantees that this read-modify-write sequence of this register safe?
And, btw, this can't be optimised very well because of the barrier inside
writel().  This would be better:

	if (on)
		reg &= ~en_mask;
	else
		reg |= en_mask;

	writel(reg, sphy->phyctrl_pmureg);

> +static inline 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 (struct samsung_usbphy_drvdata *) match->data;

match->data is a const void pointer.  Is there a reason you need this
cast here?  What if you made the returned pointer from this function
also const and fixed up all its users (no user should modify this
data.)

>  #ifdef CONFIG_OF
>  static const struct of_device_id samsung_usbphy_dt_match[] = {
>  	{
>  		.compatible = "samsung,s3c64xx-usbphy",
> -		.data = (void *)TYPE_S3C64XX,
> +		.data = (void *)&usbphy_s3c64xx,

Why do you need this cast?

>  	}, {
>  		.compatible = "samsung,exynos4210-usbphy",
> -		.data = (void *)TYPE_EXYNOS4210,
> +		.data = (void *)&usbphy_exynos4,

Ditto.

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

[Vivek Gautam]

Hi Russell,


On Thu, Dec 27, 2012 at 5:56 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Dec 26, 2012 at 05:58:32PM +0530, Vivek Gautam wrote:
>> +     if (!ret)
>> +             sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
>> +
>> +     of_node_put(usbphy_pmu);
>> +
>> +     if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {
>
> No.  Learn what the error return values are from functions.  Using the
> wrong ones is buggy.  ioremap() only ever returns NULL on error.  You
> must check against NULL, and not use the IS_ERR stuff.
>
True, i should have checked the things. :-(
ioremap() won't return error. Will amend this to check against NULL.

>> +/*
>> + * 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)
>> +{
>> +     u32 reg;
>> +     int en_mask;
>> +
>> +     if (!sphy->phyctrl_pmureg) {
>> +             dev_warn(sphy->dev, "Can't set pmu isolation\n");
>> +             return;
>> +     }
>> +
>> +     reg = readl(sphy->phyctrl_pmureg);
>> +
>> +     en_mask = sphy->drv_data->devphy_en_mask;
>> +
>> +     if (on)
>> +             writel(reg & ~en_mask, sphy->phyctrl_pmureg);
>> +     else
>> +             writel(reg | en_mask, sphy->phyctrl_pmureg);
>
> What guarantees that this read-modify-write sequence of this register safe?
> And, btw, this can't be optimised very well because of the barrier inside
> writel().  This would be better:
>
>         if (on)
>                 reg &= ~en_mask;
>         else
>                 reg |= en_mask;
>
>         writel(reg, sphy->phyctrl_pmureg);
>
Sure will amend this.
A similar way suggested by Sylwester in the earlier mail in this thread:

reg = on ? reg & ~mask : reg | mask;
writel(reg, sphy->phyctrl_pmureg);

Does this look fine ?

>> +static inline 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 (struct samsung_usbphy_drvdata *) match->data;
>
> match->data is a const void pointer.  Is there a reason you need this
> cast here?  What if you made the returned pointer from this function
> also const and fixed up all its users (no user should modify this
> data.)
>
Right, we won't need this cast since match->data is a void pointer.
Will also make the returned pointer const.

>>  #ifdef CONFIG_OF
>>  static const struct of_device_id samsung_usbphy_dt_match[] = {
>>       {
>>               .compatible = "samsung,s3c64xx-usbphy",
>> -             .data = (void *)TYPE_S3C64XX,
>> +             .data = (void *)&usbphy_s3c64xx,
>
> Why do you need this cast?
>
True we don't need this cast. Will remove this one.

>>       }, {
>>               .compatible = "samsung,exynos4210-usbphy",
>> -             .data = (void *)TYPE_EXYNOS4210,
>> +             .data = (void *)&usbphy_exynos4,
>
> Ditto.

True we don't need this cast. Will remove this one.

Thanks for the review :-)


-- 
Regards
Vivek

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

[Vivek Gautam]

Hi Sylwester,


On Thu, Dec 27, 2012 at 4:00 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 12/26/2012 01:28 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>
>> ---
>>   .../devicetree/bindings/usb/samsung-usbphy.txt     |   31 ++++
>>   drivers/usb/phy/samsung-usbphy.c                   |  145
>> +++++++++++++++++---
>>   2 files changed, 155 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> index 7b26e2d..6b873fd 100644
>> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> @@ -9,3 +9,34 @@ 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.
>> +
>> +- The child node 'usbphy-pmu' to the node usbphy should provide the
>> following
>> +  information required by usb-phy controller to enable/disable the phy.
>> +   - reg : base physical address of PHY control register in PMU which
>> +           enables/disables the phy controller.
>> +           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 (like in
>> S3C64XX) or
>> +           '0x8' in case we have two phy-control registers (like in
>> Exynos4210)
>> +           and so on.
>> +
>> +Example:
>> + - Exysno4210
>
>
> s/Exysno/Exynos
>
Sure will amend this.
>
>> +
>> +       usbphy@125B0000 {
>> +               #address-cells =<1>;
>> +               #size-cells =<1>;
>> +               compatible = "samsung,exynos4210-usbphy";
>> +               reg =<0x125B0000 0x100>;
>> +
>> +               usbphy-pmu {
>> +                       /* 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..b9f4f42 100644
>> --- a/drivers/usb/phy/samsung-usbphy.c
>> +++ b/drivers/usb/phy/samsung-usbphy.c
>> @@ -60,20 +60,42 @@
>>   #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
>> + *
>> + *     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 [0] and [1] respectively.
>
>
> "and are placed at positions 0 and 1 respectively" ?
>
Ok, will change this.
>
>> + *     Although for newer SoCs like exynos these bits belong to
>> + *     different registers altogether placed at [0].
>> + */
>> +struct samsung_usbphy_drvdata {
>> +       int cpu_type;
>> +       int devphy_en_mask;
>> +};
>> +
>> +/*
>>    * 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
>> + * @phyctrl_pmureg: usb device phy-control pmu register memory base
>
>
> nit: Perhaps we could just call it "pmureg' ?
>
Sure we can use the name 'pmureg'.
>
>>    * @ref_clk_freq: reference clock frequency selection
>> - * @cpu_type: machine identifier
>> + * @drv_data: driver data available for different cpu types
>
>
> Actually it's for different SoCs, not CPUs, right ?
>
Right, will change this.

>
>>    */
>>   struct samsung_usbphy {
>>         struct usb_phy  phy;
>> @@ -81,12 +103,67 @@ struct samsung_usbphy {
>>         struct device   *dev;
>>         struct clk      *clk;
>>         void __iomem    *regs;
>> +       void __iomem    *phyctrl_pmureg;
>>         int             ref_clk_freq;
>> -       int             cpu_type;
>> +       const struct samsung_usbphy_drvdata *drv_data;
>>   };
>>
>>   #define phy_to_sphy(x)                container_of((x), struct
>> samsung_usbphy, phy)
>>
>> +static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
>
>
> nit: s/samsung_usbphy_parse_dt_param/samsung_usbphy_parse_dt ?
>
Ok, will change this.
>
>> +{
>> +       struct device_node *usbphy_pmu;
>> +       u32 reg[2];
>> +       int ret;
>> +
>> +       if (!sphy->dev->of_node) {
>> +               dev_err(sphy->dev, "Can't get usb-phy node\n");
>> +               return -ENODEV;
>
>
> The function is called only when dev->of_node is not NULL, so this path
> is never executed, AFAICS. I would just drop this redundant check.
>
Yes, thanks for pointing out. I missed that. Will remove this check.

>
>> +       }
>> +
>> +       usbphy_pmu = of_get_child_by_name(sphy->dev->of_node,
>> "usbphy-pmu");
>> +       if (!usbphy_pmu)
>> +               dev_warn(sphy->dev, "Can't get usb-phy pmu control
>> node\n");
>
>
>> +       ret = of_property_read_u32_array(usbphy_pmu, "reg", reg,
>>
>> +                                               ARRAY_SIZE(reg));
>> +       if (!ret)
>> +               sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
>
>
> I don't think this is correct. reg is not really an array of u32 type,
> it's an array of address/size tuples. I thought you would use of_iomap()
> here. Any reason why you didn't do so ? It would also make it easier to
> handle multiple address/size pairs if required.
>
True we should in fact be using of_iomap. Will change this accordingly.

>
>> +
>> +       of_node_put(usbphy_pmu);
>> +
>> +       if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {
>
>
> When is sphy->phyctrl_pmureg assigned a ERR_PTR() value ? Wouldn't it
> be enough to simply check for NULL ?
>
Yes, true no error returned here, will check this against NULL.

>
>> +               dev_err(sphy->dev, "Can't get usb-phy pmu control
>> register\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       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)
>> +{
>> +       u32 reg;
>> +       int en_mask;
>> +
>> +       if (!sphy->phyctrl_pmureg) {
>> +               dev_warn(sphy->dev, "Can't set pmu isolation\n");
>> +               return;
>> +       }
>> +
>> +       reg = readl(sphy->phyctrl_pmureg);
>
>
> To make it more generic maybe it's worth to store an offset to the actual
> register somewhere, e.g. in the driver_data struct ? This function is
> supposed to handle both device and host PHYs, isn't it ?
>
True we can make put an offset for host and device PHYs.
So we iomap the memory base address of the first register (i.e. device
PHY in exynos4210),
and then use an offset for other register (i.e.host PHY). Right?

>> +       en_mask = sphy->drv_data->devphy_en_mask;
>> +
>> +       if (on)
>> +               writel(reg&  ~en_mask, sphy->phyctrl_pmureg);
>>
>> +       else
>> +               writel(reg | en_mask, sphy->phyctrl_pmureg);
>
>
> nit: This could be also written as:
>
>         reg = on ? reg & ~mask : reg | mask;
>         writel(reg, sphy->phyctrl_pmureg);

Sure will amend this.


-- 
Thanks & Regards
Vivek

[PATCH v5] 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 v4:
 - Added 'ranges' property to usbphy node, to iomap the child nodes's
   address space.
 - Using offset for device phy control register to make it more generic.
 - Using of_iomap() to map register for child node.
 - Removing buggy check for IS_ERR_OR_NULL for ioremapped address, instead
   checking it against NULL.
 - Adding spin_lock around read-modify-write block in
   samsung_usbphy_set_isolation().
 - removing unnecessary casting for match->data.

 .../devicetree/bindings/usb/samsung-usbphy.txt     |   35 +++++
 drivers/usb/phy/samsung-usbphy.c                   |  145 +++++++++++++++++---
 2 files changed, 159 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
index 7b26e2d..1b97765 100644
--- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
+++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
@@ -9,3 +9,38 @@ 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 register in PMU which
+           enables/disables the phy controller.
+           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 (like in S3C64XX) or
+           '0x8' in case we have two phy-control registers (like in Exynos4210)
+           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..99e16e9 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,45 @@
 #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
+ * @pmureg_devphy_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 pmureg_devphy_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
+ * @pmureg: usb device phy-control pmu register memory base
  * @ref_clk_freq: reference clock frequency selection
- * @cpu_type: machine identifier
+ * @drv_data: driver data available for different SoCs
  */
 struct samsung_usbphy {
 	struct usb_phy	phy;
@@ -81,12 +107,63 @@ struct samsung_usbphy {
 	struct device	*dev;
 	struct clk	*clk;
 	void __iomem	*regs;
+	void __iomem	*pmureg;
 	int		ref_clk_freq;
-	int		cpu_type;
+	const struct samsung_usbphy_drvdata *drv_data;
 };
 
 #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->pmureg = of_iomap(usbphy_sys, 0);
+
+	of_node_put(usbphy_sys);
+
+	if (sphy->pmureg == NULL) {
+		dev_err(sphy->dev, "Can't get usb-phy pmu control register\n");
+		return -ENODEV;
+	}
+
+	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)
+{
+	static DEFINE_SPINLOCK(lock);
+	unsigned long flags;
+	void __iomem *reg;
+	u32 reg_val;
+	u32 en_mask;
+
+	if (!sphy->pmureg) {
+		dev_warn(sphy->dev, "Can't set pmu isolation\n");
+		return;
+	}
+
+	reg = sphy->pmureg + sphy->drv_data->pmureg_devphy_offset;
+	en_mask = sphy->drv_data->devphy_en_mask;
+
+	spin_lock_irqsave(&lock, flags);
+
+	reg_val = readl(reg);
+	reg_val = on ? (reg_val & ~en_mask) : (reg_val | en_mask);
+	writel(reg_val, reg);
+
+	spin_unlock_irqrestore(&lock, flags);
+}
+
 /*
  * Returns reference clock frequency selection value
  */
@@ -112,7 +189,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 +212,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 +242,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;
@@ -199,6 +276,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,38 +307,37 @@ 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);
 }
 
 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 +361,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,7 +381,7 @@ 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);
 
 	platform_set_drvdata(pdev, sphy);
@@ -305,17 +395,30 @@ static int __exit samsung_usbphy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&sphy->phy);
 
+	if (sphy->pmureg)
+		iounmap(sphy->pmureg);
+
 	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 +428,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

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

[Vivek Gautam]

Hi,


On Fri, Dec 28, 2012 at 2:43 PM, Vivek Gautam <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>

Any further comments on this ? Or does this seem fine ?


-- 
Thanks & Regards
Vivek

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

[Sylwester Nawrocki]

Hi,

On 12/28/2012 10:13 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>
...
> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> @@ -9,3 +9,38 @@ 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 register in PMU which
> +           enables/disables the phy controller.

On some SoCs USB_PHY_CONTROL registers don't belong to PMU. So maybe you 
should
drop references to PMU, or list all SoC entities where USB_PHY_CONTROL 
appears:
PMU, "MISC REGISTER", etc.

I would just rephrase this to:

     - reg : base physical address of PHY_CONTROL registers

"phy controller" might be confusing, since PHY CONTROLLER is an entity 
separate
from PHY 0 and PHY 1 ?

> +           The size of this register is the total sum of size of all phy-control

And what about using PHY_CONTROL name as per the User Manuals ? That would
perhaps make it a bit easier to follow.

> +           registers that the SoC has. For example, the size will be
> +           '0x4' in case we have only one phy-control register (like in S3C64XX) or
> +           '0x8' in case we have two phy-control registers (like in Exynos4210)
> +           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>;
> + };
> + };
...
>   /*
> + * 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
> + * @pmureg_devphy_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 pmureg_devphy_offset;

Perhaps just "devphy_reg_offset" would do ?

> +};
> +
> +/*
>    * 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

Is this more precisely:

       * @regs: usb phy controller registers memory base
?
> + * @pmureg: usb device phy-control pmu register memory base

Maybe something like this would be more clear:

@pmureg: USB device PHY_CONTROL registers memory region base

Note, that not on all SoCs USB_PHY_CONTROL registers belong to PMU.
Haven't you considered changing "pmureg" to ctrl_regs or something
else more generic ?

>    * @ref_clk_freq: reference clock frequency selection
> - * @cpu_type: machine identifier
> + * @drv_data: driver data available for different SoCs
>    */
>   struct samsung_usbphy {
>   struct usb_phy phy;
> @@ -81,12 +107,63 @@ struct samsung_usbphy {
>   struct device *dev;
>   struct clk *clk;
>   void __iomem *regs;
> + void __iomem *pmureg;
>   int ref_clk_freq;
> - int cpu_type;
> + const struct samsung_usbphy_drvdata *drv_data;
>   };
...
> +/*
> + * Set isolation here for phy.
> + * SOCs control this by controlling corresponding PMU registers

Hmm, it's not always PMU registers. I would remove this sentence and
instead explain what's the meaning of 'on' argument, so it is clear
the PHY is deactivated when on != 0.

> + */
> +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on)
> +{
> + static DEFINE_SPINLOCK(lock);

You probably don't need a global spinlock. Couldn't the spinlock be added
as struct samsung_usbhy field instead ?

> + unsigned long flags;
> + void __iomem *reg;
> + u32 reg_val;
> + u32 en_mask;
> +
> + if (!sphy->pmureg) {
> + dev_warn(sphy->dev, "Can't set pmu isolation\n");
> + return;
> + }
> +
> + reg = sphy->pmureg + sphy->drv_data->pmureg_devphy_offset;
> + en_mask = sphy->drv_data->devphy_en_mask;
> +
> + spin_lock_irqsave(&lock, flags);
> +
> + reg_val = readl(reg);
> + reg_val = on ? (reg_val&  ~en_mask) : (reg_val | en_mask);

Might be a good idea to use in this case plain if/else instead..

> + writel(reg_val, reg);
> +
> + spin_unlock_irqrestore(&lock, flags);
> +}

Thanks,
Sylwester

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

[Vivek Gautam]

Hi Sylwester,


On Thu, Jan 10, 2013 at 3:12 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> Hi,
>
>
> On 12/28/2012 10:13 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>
>
> ...
>
>> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> @@ -9,3 +9,38 @@ 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 register in PMU which
>> +           enables/disables the phy controller.
>
>
> On some SoCs USB_PHY_CONTROL registers don't belong to PMU. So maybe you
> should
> drop references to PMU, or list all SoC entities where USB_PHY_CONTROL
> appears:
> PMU, "MISC REGISTER", etc.
>

On S3C64XX "USB_SIG_MASK" bit is in "OTHERS" register, which is actually part of
system controller register map (clock controller + PM controller), and
then S5P64XX onward
this falls under PM controller's memory map. So, i thought of using
reference to PMU.
May be i am missing something here ?
Will change this if you suggest.

> I would just rephrase this to:
>
>     - reg : base physical address of PHY_CONTROL registers
>
> "phy controller" might be confusing, since PHY CONTROLLER is an entity
> separate
> from PHY 0 and PHY 1 ?
>
Ok, will change this as suggested.

>
>> +           The size of this register is the total sum of size of all
>> phy-control
>
>
> And what about using PHY_CONTROL name as per the User Manuals ? That would
> perhaps make it a bit easier to follow.
>
Sure this is better. We can use PHY_CONTROL to align with user manuals and avoid
any confusions.

>
>> +           registers that the SoC has. For example, the size will be
>> +           '0x4' in case we have only one phy-control register (like in
>> S3C64XX) or
>> +           '0x8' in case we have two phy-control registers (like in
>> Exynos4210)
>> +           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>;
>> + };
>> + };
>
> ...
>
>>   /*
>> + * 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
>> + * @pmureg_devphy_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 pmureg_devphy_offset;
>
>
> Perhaps just "devphy_reg_offset" would do ?
>
Sure, will amend this as suggested.

>
>> +};
>> +
>> +/*
>>    * 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
>
>
> Is this more precisely:
>
>       * @regs: usb phy controller registers memory base
>
> ?
this had been a part of Praveen's work submitted for initial
samsung-usbphy driver,
so didn't change anything in that. ;-)
Will amend this as suggested.

>>
>> + * @pmureg: usb device phy-control pmu register memory base
>
>
> Maybe something like this would be more clear:
>
> @pmureg: USB device PHY_CONTROL registers memory region base
>
Sure, will amend this.

> Note, that not on all SoCs USB_PHY_CONTROL registers belong to PMU.
> Haven't you considered changing "pmureg" to ctrl_regs or something
> else more generic ?
>
Same as mentioned in above comment in this context  for PMU register.
Thought this could be self-explanatory for pmu interface to control phy.
Will change this if you suggest.

>
>>    * @ref_clk_freq: reference clock frequency selection
>> - * @cpu_type: machine identifier
>> + * @drv_data: driver data available for different SoCs
>>    */
>>   struct samsung_usbphy {
>>   struct usb_phy phy;
>> @@ -81,12 +107,63 @@ struct samsung_usbphy {
>>   struct device *dev;
>>   struct clk *clk;
>>   void __iomem *regs;
>> + void __iomem *pmureg;
>>   int ref_clk_freq;
>> - int cpu_type;
>> + const struct samsung_usbphy_drvdata *drv_data;
>>   };
>
> ...
>
>> +/*
>> + * Set isolation here for phy.
>> + * SOCs control this by controlling corresponding PMU registers
>
>
> Hmm, it's not always PMU registers. I would remove this sentence and
> instead explain what's the meaning of 'on' argument, so it is clear
> the PHY is deactivated when on != 0.
>

Ditto for PMU register comment,
Will put an explanation for 'on' argument.

>
>> + */
>> +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int
>> on)
>> +{
>> + static DEFINE_SPINLOCK(lock);
>
>
> You probably don't need a global spinlock. Couldn't the spinlock be added
> as struct samsung_usbhy field instead ?
>
True, will add a spinlock in the samsung_usbphy structure.

>
>> + unsigned long flags;
>> + void __iomem *reg;
>> + u32 reg_val;
>> + u32 en_mask;
>> +
>> + if (!sphy->pmureg) {
>> + dev_warn(sphy->dev, "Can't set pmu isolation\n");
>> + return;
>> + }
>> +
>> + reg = sphy->pmureg + sphy->drv_data->pmureg_devphy_offset;
>> + en_mask = sphy->drv_data->devphy_en_mask;
>> +
>> + spin_lock_irqsave(&lock, flags);
>> +
>> + reg_val = readl(reg);
>> + reg_val = on ? (reg_val&  ~en_mask) : (reg_val | en_mask);
>
>
> Might be a good idea to use in this case plain if/else instead..
>
ok, will amend this.


-- 
Thanks & Regards
Vivek