[PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

This patch simplifies the way the phy-samsung-usb code finds the correct
power management register to enable PHY clock gating. Previously, the
code would calculate the register address from a device tree supplied
base address and add an offset based on the PHY type.

Since every PHY has its own device tree entry and needs only one
register, we can just encode the address itself in the device tree and
remove the diffentiation in the code. The bitmask needed to specify the
bit within that register stays in place, allowing support for platforms
like s3c64xx that use different bits within the same register.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 .../devicetree/bindings/usb/samsung-usbphy.txt     | 26 +++++-----------------
 arch/arm/boot/dts/exynos5250.dtsi                  |  4 ++--
 drivers/usb/phy/phy-samsung-usb.c                  | 18 ++++-----------
 drivers/usb/phy/phy-samsung-usb.h                  | 23 +++++--------------
 drivers/usb/phy/phy-samsung-usb2.c                 | 11 ++++-----
 drivers/usb/phy/phy-samsung-usb3.c                 |  2 +-
 6 files changed, 22 insertions(+), 62 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
index 33fd354..1cf9b68 100644
--- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
+++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
@@ -34,14 +34,7 @@ Optional properties:
 - 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.
+  - reg : address of PHY_CONTROL register for this PHY.
 
 Example:
  - Exynos4210
@@ -57,8 +50,8 @@ Example:
 		clock-names = "xusbxti", "otg";
 
 		usbphy-sys {
-			/* USB device and host PHY_CONTROL registers */
-			reg = <0x10020704 0x8>;
+			/* USB device PHY_CONTROL register */
+			reg = <0x10020704 0x4>;
 		};
 	};
 
@@ -90,14 +83,7 @@ Optional properties:
 - 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.
+  - reg : address of PHY_CONTROL register for this PHY.
 
 Example:
 	usbphy@12100000 {
@@ -111,7 +97,7 @@ Example:
 		clock-names = "ext_xtal", "usbdrd30";
 
 		usbphy-sys {
-			/* USB device and host PHY_CONTROL registers */
-			reg = <0x10040704 0x8>;
+			/* USB DRD PHY_CONTROL register */
+			reg = <0x10040704 0x4>;
 		};
 	};
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index ef57277..5a754d7 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -473,7 +473,7 @@
 		ranges;
 
 		usbphy-sys {
-			reg = <0x10040704 0x8>;
+			reg = <0x10040704 0x4>;
 		};
 	};
 
@@ -505,7 +505,7 @@
 		ranges;
 
 		usbphy-sys {
-			reg = <0x10040704 0x8>,
+			reg = <0x10040708 0x4>,
 			      <0x10050230 0x4>;
 		};
 	};
diff --git a/drivers/usb/phy/phy-samsung-usb.c b/drivers/usb/phy/phy-samsung-usb.c
index ac025ca..51cad19 100644
--- a/drivers/usb/phy/phy-samsung-usb.c
+++ b/drivers/usb/phy/phy-samsung-usb.c
@@ -75,31 +75,21 @@ EXPORT_SYMBOL_GPL(samsung_usbphy_parse_dt);
  */
 void samsung_usbphy_set_isolation_4210(struct samsung_usbphy *sphy, bool on)
 {
-	void __iomem *reg = NULL;
 	u32 reg_val;
-	u32 en_mask = 0;
 
 	if (!sphy->pmuregs) {
 		dev_warn(sphy->dev, "Can't set pmu isolation\n");
 		return;
 	}
 
-	if (sphy->phy_type == USB_PHY_TYPE_DEVICE) {
-		reg = sphy->pmuregs + sphy->drv_data->devphy_reg_offset;
-		en_mask = sphy->drv_data->devphy_en_mask;
-	} else if (sphy->phy_type == USB_PHY_TYPE_HOST) {
-		reg = sphy->pmuregs + sphy->drv_data->hostphy_reg_offset;
-		en_mask = sphy->drv_data->hostphy_en_mask;
-	}
-
-	reg_val = readl(reg);
+	reg_val = readl(sphy->pmuregs);
 
 	if (on)
-		reg_val &= ~en_mask;
+		reg_val &= ~sphy->drv_data->phy_en_mask;
 	else
-		reg_val |= en_mask;
+		reg_val |= sphy->drv_data->phy_en_mask;
 
-	writel(reg_val, reg);
+	writel(reg_val, sphy->pmuregs);
 
 	if (sphy->drv_data->cpu_type == TYPE_EXYNOS4X12) {
 		writel(reg_val, sphy->pmuregs + EXYNOS4X12_PHY_HSIC_CTRL0);
diff --git a/drivers/usb/phy/phy-samsung-usb.h b/drivers/usb/phy/phy-samsung-usb.h
index 68771bf..a817af5 100644
--- a/drivers/usb/phy/phy-samsung-usb.h
+++ b/drivers/usb/phy/phy-samsung-usb.h
@@ -243,7 +243,6 @@
 #define KHZ (1000)
 #endif
 
-#define EXYNOS_USBHOST_PHY_CTRL_OFFSET		(0x4)
 #define S3C64XX_USBPHY_ENABLE			(0x1 << 16)
 #define EXYNOS_USBPHY_ENABLE			(0x1 << 0)
 #define EXYNOS_USB20PHY_CFG_HOST_LINK		(0x1 << 0)
@@ -260,27 +259,15 @@ struct samsung_usbphy;
 /*
  * 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
- * @hostphy_en_mask: host phy enable mask for PHY CONTROL register
- * @devphy_reg_offset: offset to DEVICE PHY CONTROL register from
- *		       mapped address of system controller.
- * @hostphy_reg_offset: offset to HOST PHY CONTROL register from
- *		       mapped address of system controller.
+ * @phy_en_mask: 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 position 0 and 1 respectively.
- *	Although for newer SoCs like exynos these bits belong to
- *	different registers altogether placed at position 0.
+ *	Here we have a mask for the phy control bit. This is required for SoCs
+ *	like S5PV210, in which different PHYs use different bits in the same
+ *	register.
  */
 struct samsung_usbphy_drvdata {
 	int cpu_type;
-	int devphy_en_mask;
-	int hostphy_en_mask;
-	u32 devphy_reg_offset;
-	u32 hostphy_reg_offset;
+	int phy_en_mask;
 	int (*rate_to_clksel)(struct samsung_usbphy *, unsigned long);
 	void (*set_isolation)(struct samsung_usbphy *, bool);
 	void (*phy_enable)(struct samsung_usbphy *);
diff --git a/drivers/usb/phy/phy-samsung-usb2.c b/drivers/usb/phy/phy-samsung-usb2.c
index 758b86d..adbaa8c 100644
--- a/drivers/usb/phy/phy-samsung-usb2.c
+++ b/drivers/usb/phy/phy-samsung-usb2.c
@@ -445,7 +445,7 @@ static int samsung_usb2phy_remove(struct platform_device *pdev)
 
 static const struct samsung_usbphy_drvdata usb2phy_s3c64xx = {
 	.cpu_type		= TYPE_S3C64XX,
-	.devphy_en_mask		= S3C64XX_USBPHY_ENABLE,
+	.phy_en_mask		= S3C64XX_USBPHY_ENABLE,
 	.rate_to_clksel		= samsung_usbphy_rate_to_clksel_64xx,
 	.set_isolation		= NULL, /* TODO */
 	.phy_enable		= samsung_usb2phy_enable,
@@ -454,8 +454,7 @@ static const struct samsung_usbphy_drvdata usb2phy_s3c64xx = {
 
 static const struct samsung_usbphy_drvdata usb2phy_exynos4 = {
 	.cpu_type		= TYPE_EXYNOS4210,
-	.devphy_en_mask		= EXYNOS_USBPHY_ENABLE,
-	.hostphy_en_mask	= EXYNOS_USBPHY_ENABLE,
+	.phy_en_mask		= EXYNOS_USBPHY_ENABLE,
 	.rate_to_clksel		= samsung_usbphy_rate_to_clksel_64xx,
 	.set_isolation		= samsung_usbphy_set_isolation_4210,
 	.phy_enable		= samsung_usb2phy_enable,
@@ -464,8 +463,7 @@ static const struct samsung_usbphy_drvdata usb2phy_exynos4 = {
 
 static const struct samsung_usbphy_drvdata usb2phy_exynos4x12 = {
 	.cpu_type		= TYPE_EXYNOS4X12,
-	.devphy_en_mask		= EXYNOS_USBPHY_ENABLE,
-	.hostphy_en_mask	= EXYNOS_USBPHY_ENABLE,
+	.phy_en_mask		= EXYNOS_USBPHY_ENABLE,
 	.rate_to_clksel		= samsung_usbphy_rate_to_clksel_4x12,
 	.set_isolation		= samsung_usbphy_set_isolation_4210,
 	.phy_enable		= samsung_usb2phy_enable,
@@ -474,8 +472,7 @@ static const struct samsung_usbphy_drvdata usb2phy_exynos4x12 = {
 
 static struct samsung_usbphy_drvdata usb2phy_exynos5 = {
 	.cpu_type		= TYPE_EXYNOS5250,
-	.hostphy_en_mask	= EXYNOS_USBPHY_ENABLE,
-	.hostphy_reg_offset	= EXYNOS_USBHOST_PHY_CTRL_OFFSET,
+	.phy_en_mask		= EXYNOS_USBPHY_ENABLE,
 	.rate_to_clksel		= samsung_usbphy_rate_to_clksel_4x12,
 	.set_isolation		= samsung_usbphy_set_isolation_4210,
 	.phy_enable		= samsung_exynos5_usb2phy_enable,
diff --git a/drivers/usb/phy/phy-samsung-usb3.c b/drivers/usb/phy/phy-samsung-usb3.c
index 300e0cf..3ad9579 100644
--- a/drivers/usb/phy/phy-samsung-usb3.c
+++ b/drivers/usb/phy/phy-samsung-usb3.c
@@ -302,7 +302,7 @@ static int samsung_usb3phy_remove(struct platform_device *pdev)
 
 static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
 	.cpu_type		= TYPE_EXYNOS5250,
-	.devphy_en_mask		= EXYNOS_USBPHY_ENABLE,
+	.phy_en_mask		= EXYNOS_USBPHY_ENABLE,
 	.rate_to_clksel		= samsung_usbphy_rate_to_clksel_4x12,
 	.set_isolation		= samsung_usbphy_set_isolation_4210,
 	.phy_enable		= samsung_exynos5_usb3phy_enable,
-- 
1.7.12.4

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Mark Rutland]

On Tue, Aug 06, 2013 at 07:00:17PM +0100, Julius Werner wrote:
> This patch simplifies the way the phy-samsung-usb code finds the correct
> power management register to enable PHY clock gating. Previously, the
> code would calculate the register address from a device tree supplied
> base address and add an offset based on the PHY type.
> 
> Since every PHY has its own device tree entry and needs only one
> register, we can just encode the address itself in the device tree and
> remove the diffentiation in the code. The bitmask needed to specify the
> bit within that register stays in place, allowing support for platforms
> like s3c64xx that use different bits within the same register.

This breaks compatibility, both for an old kernel and a new dt and a new
kernel with an old dt. Is anyone using these bindings?


> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  .../devicetree/bindings/usb/samsung-usbphy.txt     | 26 +++++-----------------
>  arch/arm/boot/dts/exynos5250.dtsi                  |  4 ++--
>  drivers/usb/phy/phy-samsung-usb.c                  | 18 ++++-----------
>  drivers/usb/phy/phy-samsung-usb.h                  | 23 +++++--------------
>  drivers/usb/phy/phy-samsung-usb2.c                 | 11 ++++-----
>  drivers/usb/phy/phy-samsung-usb3.c                 |  2 +-
>  6 files changed, 22 insertions(+), 62 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> index 33fd354..1cf9b68 100644
> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> @@ -34,14 +34,7 @@ Optional properties:
>  - 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.
> +  - reg : address of PHY_CONTROL register for this PHY.
>  
>  Example:
>   - Exynos4210
> @@ -57,8 +50,8 @@ Example:
>  		clock-names = "xusbxti", "otg";
>  
>  		usbphy-sys {
> -			/* USB device and host PHY_CONTROL registers */
> -			reg = <0x10020704 0x8>;
> +			/* USB device PHY_CONTROL register */
> +			reg = <0x10020704 0x4>;
>  		};
>  	};
>  

Why are we describing fewer registers now? Are they described elsewhere?

The dt should describe the device, not only the portion of it Linux
wants to use right now.

Thanks,
Mark.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Mark Rutland]

On Tue, Aug 06, 2013 at 07:00:17PM +0100, Julius Werner wrote:
> This patch simplifies the way the phy-samsung-usb code finds the correct
> power management register to enable PHY clock gating. Previously, the
> code would calculate the register address from a device tree supplied
> base address and add an offset based on the PHY type.
> 
> Since every PHY has its own device tree entry and needs only one
> register, we can just encode the address itself in the device tree and
> remove the diffentiation in the code. The bitmask needed to specify the
> bit within that register stays in place, allowing support for platforms
> like s3c64xx that use different bits within the same register.

This breaks compatibility, both for an old kernel and a new dt and a new
kernel with an old dt. Is anyone using these bindings?


> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  .../devicetree/bindings/usb/samsung-usbphy.txt     | 26 +++++-----------------
>  arch/arm/boot/dts/exynos5250.dtsi                  |  4 ++--
>  drivers/usb/phy/phy-samsung-usb.c                  | 18 ++++-----------
>  drivers/usb/phy/phy-samsung-usb.h                  | 23 +++++--------------
>  drivers/usb/phy/phy-samsung-usb2.c                 | 11 ++++-----
>  drivers/usb/phy/phy-samsung-usb3.c                 |  2 +-
>  6 files changed, 22 insertions(+), 62 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> index 33fd354..1cf9b68 100644
> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> @@ -34,14 +34,7 @@ Optional properties:
>  - 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.
> +  - reg : address of PHY_CONTROL register for this PHY.
>  
>  Example:
>   - Exynos4210
> @@ -57,8 +50,8 @@ Example:
>  		clock-names = "xusbxti", "otg";
>  
>  		usbphy-sys {
> -			/* USB device and host PHY_CONTROL registers */
> -			reg = <0x10020704 0x8>;
> +			/* USB device PHY_CONTROL register */
> +			reg = <0x10020704 0x4>;
>  		};
>  	};
>  

Why are we describing fewer registers now? Are they described elsewhere?

The dt should describe the device, not only the portion of it Linux
wants to use right now.

Thanks,
Mark.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

> This breaks compatibility, both for an old kernel and a new dt and a new
> kernel with an old dt. Is anyone using these bindings?

They only affect Samsung SoCs and have only been upstream for half a
year, so I doubt it's heavily used.

> Why are we describing fewer registers now? Are they described elsewhere?
>
> The dt should describe the device, not only the portion of it Linux
> wants to use right now.

This only ever described a small section of the huge set of PMU
registers anyway. Before it described up to three registers
controlling different PHYs (using hardcoded offsets in the code to
later find the right one)... with my patch every PHY's DT entry only
describes the one register concerning itself, which makes more sense
in my opinion. It will also prevent the register descriptions in
different DT entries from overlapping.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

> This breaks compatibility, both for an old kernel and a new dt and a new
> kernel with an old dt. Is anyone using these bindings?

They only affect Samsung SoCs and have only been upstream for half a
year, so I doubt it's heavily used.

> Why are we describing fewer registers now? Are they described elsewhere?
>
> The dt should describe the device, not only the portion of it Linux
> wants to use right now.

This only ever described a small section of the huge set of PMU
registers anyway. Before it described up to three registers
controlling different PHYs (using hardcoded offsets in the code to
later find the right one)... with my patch every PHY's DT entry only
describes the one register concerning itself, which makes more sense
in my opinion. It will also prevent the register descriptions in
different DT entries from overlapping.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Sylwester Nawrocki]

On 08/07/2013 07:06 PM, Julius Werner wrote:
>> This breaks compatibility, both for an old kernel and a new dt and a new
>> kernel with an old dt. Is anyone using these bindings?
> 
> They only affect Samsung SoCs and have only been upstream for half a
> year, so I doubt it's heavily used.

It probably wouldn't be of much concern, but I can't tell for sure.

>> Why are we describing fewer registers now? Are they described elsewhere?
>>
>> The dt should describe the device, not only the portion of it Linux
>> wants to use right now.
> 
> This only ever described a small section of the huge set of PMU
> registers anyway. 

The PMU registers are already scattered across various drivers, like
Power Domain, various PHYs (USB/HSIC/MIPI CSI-2/DSI, ADC, ...), Reset
and more. And it happens currently there is no central driver for
the Power Management Unit, that would, for example expose the regmap
interface that the interested drivers could use. The downside of this
would be that each, e.g. USB PHY driver would need to know SoC specific
offsets to its registers in the PMU.

Before device tree started to be used those PMU registers were controlled
by respective drivers, mostly through platform_data callbacks. As they
could be considered parts of given device.

                   Before it described up to three registers
> controlling different PHYs (using hardcoded offsets in the code to
> later find the right one)... with my patch every PHY's DT entry only
> describes the one register concerning itself, which makes more sense
> in my opinion. It will also prevent the register descriptions in
> different DT entries from overlapping.

Yes, the patch looks sensible. Since currently each USB PHY has its
own device tree node it looks wrong to have the reg property in the
usbphy-sys subnodes defining register region for all possible PHYs.
And we indeed and up with multiple reg properties pointing to same
register region.

However the biggest drawback is breaking backwards compatibility.
I'm not sure if those changes are worth it, especially that all those
USB PHY drivers are supposed to be rewritten to use the generic PHY API.


Thanks,
Sylwester

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Sylwester Nawrocki]

On 08/07/2013 07:06 PM, Julius Werner wrote:
>> This breaks compatibility, both for an old kernel and a new dt and a new
>> kernel with an old dt. Is anyone using these bindings?
> 
> They only affect Samsung SoCs and have only been upstream for half a
> year, so I doubt it's heavily used.

It probably wouldn't be of much concern, but I can't tell for sure.

>> Why are we describing fewer registers now? Are they described elsewhere?
>>
>> The dt should describe the device, not only the portion of it Linux
>> wants to use right now.
> 
> This only ever described a small section of the huge set of PMU
> registers anyway. 

The PMU registers are already scattered across various drivers, like
Power Domain, various PHYs (USB/HSIC/MIPI CSI-2/DSI, ADC, ...), Reset
and more. And it happens currently there is no central driver for
the Power Management Unit, that would, for example expose the regmap
interface that the interested drivers could use. The downside of this
would be that each, e.g. USB PHY driver would need to know SoC specific
offsets to its registers in the PMU.

Before device tree started to be used those PMU registers were controlled
by respective drivers, mostly through platform_data callbacks. As they
could be considered parts of given device.

                   Before it described up to three registers
> controlling different PHYs (using hardcoded offsets in the code to
> later find the right one)... with my patch every PHY's DT entry only
> describes the one register concerning itself, which makes more sense
> in my opinion. It will also prevent the register descriptions in
> different DT entries from overlapping.

Yes, the patch looks sensible. Since currently each USB PHY has its
own device tree node it looks wrong to have the reg property in the
usbphy-sys subnodes defining register region for all possible PHYs.
And we indeed and up with multiple reg properties pointing to same
register region.

However the biggest drawback is breaking backwards compatibility.
I'm not sure if those changes are worth it, especially that all those
USB PHY drivers are supposed to be rewritten to use the generic PHY API.


Thanks,
Sylwester

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Mark Rutland]

On Wed, Aug 07, 2013 at 06:06:05PM +0100, Julius Werner wrote:
> > This breaks compatibility, both for an old kernel and a new dt and a new
> > kernel with an old dt. Is anyone using these bindings?
> 
> They only affect Samsung SoCs and have only been upstream for half a
> year, so I doubt it's heavily used.

I'm not sure everyone will be happy with that line.

> 
> > Why are we describing fewer registers now? Are they described elsewhere?
> >
> > The dt should describe the device, not only the portion of it Linux
> > wants to use right now.
> 
> This only ever described a small section of the huge set of PMU
> registers anyway. Before it described up to three registers
> controlling different PHYs (using hardcoded offsets in the code to
> later find the right one)... with my patch every PHY's DT entry only
> describes the one register concerning itself, which makes more sense
> in my opinion. It will also prevent the register descriptions in
> different DT entries from overlapping.
> 

I'm not sure I understand. The old documentation referred to the
USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
your new version only refers to (usb device) PHY_CONTROL. Regardless of
multiple phys, you're suggesting that we describe less of each phy.
That seems like taking away usable information. Unless I've
misunderstood?

Ideally, we'd describe the whole set of registers and linkages to phys,
even if Linux doesn't ahppen to use that information right now.

Thanks,
Mark.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Mark Rutland]

On Wed, Aug 07, 2013 at 06:06:05PM +0100, Julius Werner wrote:
> > This breaks compatibility, both for an old kernel and a new dt and a new
> > kernel with an old dt. Is anyone using these bindings?
> 
> They only affect Samsung SoCs and have only been upstream for half a
> year, so I doubt it's heavily used.

I'm not sure everyone will be happy with that line.

> 
> > Why are we describing fewer registers now? Are they described elsewhere?
> >
> > The dt should describe the device, not only the portion of it Linux
> > wants to use right now.
> 
> This only ever described a small section of the huge set of PMU
> registers anyway. Before it described up to three registers
> controlling different PHYs (using hardcoded offsets in the code to
> later find the right one)... with my patch every PHY's DT entry only
> describes the one register concerning itself, which makes more sense
> in my opinion. It will also prevent the register descriptions in
> different DT entries from overlapping.
> 

I'm not sure I understand. The old documentation referred to the
USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
your new version only refers to (usb device) PHY_CONTROL. Regardless of
multiple phys, you're suggesting that we describe less of each phy.
That seems like taking away usable information. Unless I've
misunderstood?

Ideally, we'd describe the whole set of registers and linkages to phys,
even if Linux doesn't ahppen to use that information right now.

Thanks,
Mark.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Vivek Gautam]

On Thu, Aug 8, 2013 at 2:56 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Aug 07, 2013 at 06:06:05PM +0100, Julius Werner wrote:
>> > This breaks compatibility, both for an old kernel and a new dt and a new
>> > kernel with an old dt. Is anyone using these bindings?
>>
>> They only affect Samsung SoCs and have only been upstream for half a
>> year, so I doubt it's heavily used.
>
> I'm not sure everyone will be happy with that line.
>
>>
>> > Why are we describing fewer registers now? Are they described elsewhere?
>> >
>> > The dt should describe the device, not only the portion of it Linux
>> > wants to use right now.
>>
>> This only ever described a small section of the huge set of PMU
>> registers anyway. Before it described up to three registers
>> controlling different PHYs (using hardcoded offsets in the code to
>> later find the right one)... with my patch every PHY's DT entry only
>> describes the one register concerning itself, which makes more sense
>> in my opinion. It will also prevent the register descriptions in
>> different DT entries from overlapping.
>>
>
> I'm not sure I understand. The old documentation referred to the
> USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
> your new version only refers to (usb device) PHY_CONTROL. Regardless of
> multiple phys, you're suggesting that we describe less of each phy.
> That seems like taking away usable information. Unless I've
> misunderstood?

Just giving some pointers here:

As also mentioned in the documentation for samsung-usbphy, SoCs prior
to exynos4x had only one PMU
register handling power to the PHYs (USB 2.0 phy to be specific).
Exynos4x SoCs also had USB 2.0 PHY only
but device and host PHYs' power was being handled by two registers
namely - USBDEVICE_PHY_CONTROL and
USBHOST_PHY_CONTROL.

Exynos5x series of SoCs now have USB 2.0 type PHY (both device and
host PHY are power-handled by only one register)
and USB 3.0 type PHY (having a separate PMU register to handle power
to PHY); so in a total of two registers but both
handling entirely separate PHYs.

So, samsung-usb2 phy driver should be interacting with only one PMU
register (with an exception for exynos4x)
and furthermore samsung-usb3phy driver interact with its separate PMU register.

Sylwester,
Please correct me if i am wrong somewhere.

>
> Ideally, we'd describe the whole set of registers and linkages to phys,
> even if Linux doesn't ahppen to use that information right now.
>
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Vivek

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Vivek Gautam]

On Thu, Aug 8, 2013 at 2:56 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Aug 07, 2013 at 06:06:05PM +0100, Julius Werner wrote:
>> > This breaks compatibility, both for an old kernel and a new dt and a new
>> > kernel with an old dt. Is anyone using these bindings?
>>
>> They only affect Samsung SoCs and have only been upstream for half a
>> year, so I doubt it's heavily used.
>
> I'm not sure everyone will be happy with that line.
>
>>
>> > Why are we describing fewer registers now? Are they described elsewhere?
>> >
>> > The dt should describe the device, not only the portion of it Linux
>> > wants to use right now.
>>
>> This only ever described a small section of the huge set of PMU
>> registers anyway. Before it described up to three registers
>> controlling different PHYs (using hardcoded offsets in the code to
>> later find the right one)... with my patch every PHY's DT entry only
>> describes the one register concerning itself, which makes more sense
>> in my opinion. It will also prevent the register descriptions in
>> different DT entries from overlapping.
>>
>
> I'm not sure I understand. The old documentation referred to the
> USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
> your new version only refers to (usb device) PHY_CONTROL. Regardless of
> multiple phys, you're suggesting that we describe less of each phy.
> That seems like taking away usable information. Unless I've
> misunderstood?

Just giving some pointers here:

As also mentioned in the documentation for samsung-usbphy, SoCs prior
to exynos4x had only one PMU
register handling power to the PHYs (USB 2.0 phy to be specific).
Exynos4x SoCs also had USB 2.0 PHY only
but device and host PHYs' power was being handled by two registers
namely - USBDEVICE_PHY_CONTROL and
USBHOST_PHY_CONTROL.

Exynos5x series of SoCs now have USB 2.0 type PHY (both device and
host PHY are power-handled by only one register)
and USB 3.0 type PHY (having a separate PMU register to handle power
to PHY); so in a total of two registers but both
handling entirely separate PHYs.

So, samsung-usb2 phy driver should be interacting with only one PMU
register (with an exception for exynos4x)
and furthermore samsung-usb3phy driver interact with its separate PMU register.

Sylwester,
Please correct me if i am wrong somewhere.

>
> Ideally, we'd describe the whole set of registers and linkages to phys,
> even if Linux doesn't ahppen to use that information right now.
>
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Vivek

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

> I'm not sure I understand. The old documentation referred to the
> USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
> your new version only refers to (usb device) PHY_CONTROL. Regardless of
> multiple phys, you're suggesting that we describe less of each phy.
> That seems like taking away usable information. Unless I've
> misunderstood?

Well that's just the thing that's confusing right now, and which I am
trying to fix: every PHY is either DEVICE or HOST and thus has only
one PMU register. The current code describes the PMU register space
for all PHYs on the system in the DT entry of every PHY and then
calculates which register to use with hardcoded offsets. I think it
makes much more sense if every PHY only describes its own register and
doesn't need to do address arithmetic later on.

As Vivek said there is one exception in an old Exynos4, but that is
currently not implemented in the upstream kernel anyway, and if it
ever will be it's still much easier to special case one weird chip
than to have a super complicated and confusing mechanism for all of
them.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

> I'm not sure I understand. The old documentation referred to the
> USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
> your new version only refers to (usb device) PHY_CONTROL. Regardless of
> multiple phys, you're suggesting that we describe less of each phy.
> That seems like taking away usable information. Unless I've
> misunderstood?

Well that's just the thing that's confusing right now, and which I am
trying to fix: every PHY is either DEVICE or HOST and thus has only
one PMU register. The current code describes the PMU register space
for all PHYs on the system in the DT entry of every PHY and then
calculates which register to use with hardcoded offsets. I think it
makes much more sense if every PHY only describes its own register and
doesn't need to do address arithmetic later on.

As Vivek said there is one exception in an old Exynos4, but that is
currently not implemented in the upstream kernel anyway, and if it
ever will be it's still much easier to special case one weird chip
than to have a super complicated and confusing mechanism for all of
them.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Tomasz Figa]

Hi Julius,

On Thursday 08 of August 2013 11:06:54 Julius Werner wrote:
> > I'm not sure I understand. The old documentation referred to the
> > USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
> > your new version only refers to (usb device) PHY_CONTROL. Regardless
> > of
> > multiple phys, you're suggesting that we describe less of each phy.
> > That seems like taking away usable information. Unless I've
> > misunderstood?
> 
> Well that's just the thing that's confusing right now, and which I am
> trying to fix: every PHY is either DEVICE or HOST and thus has only
> one PMU register. The current code describes the PMU register space
> for all PHYs on the system in the DT entry of every PHY and then
> calculates which register to use with hardcoded offsets. I think it
> makes much more sense if every PHY only describes its own register and
> doesn't need to do address arithmetic later on.
> 
> As Vivek said there is one exception in an old Exynos4,

Not that old yet. :)

> but that is
> currently not implemented in the upstream kernel anyway

Sorry, I don't understand what is not implemented. Without your patch, the 
PHY driver handles both PMU registers of Exynos4.

Best regards,
Tomasz

> , and if it
> ever will be it's still much easier to special case one weird chip
> than to have a super complicated and confusing mechanism for all of
> them.
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Tomasz Figa]

Hi Julius,

On Thursday 08 of August 2013 11:06:54 Julius Werner wrote:
> > I'm not sure I understand. The old documentation referred to the
> > USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
> > your new version only refers to (usb device) PHY_CONTROL. Regardless
> > of
> > multiple phys, you're suggesting that we describe less of each phy.
> > That seems like taking away usable information. Unless I've
> > misunderstood?
> 
> Well that's just the thing that's confusing right now, and which I am
> trying to fix: every PHY is either DEVICE or HOST and thus has only
> one PMU register. The current code describes the PMU register space
> for all PHYs on the system in the DT entry of every PHY and then
> calculates which register to use with hardcoded offsets. I think it
> makes much more sense if every PHY only describes its own register and
> doesn't need to do address arithmetic later on.
> 
> As Vivek said there is one exception in an old Exynos4,

Not that old yet. :)

> but that is
> currently not implemented in the upstream kernel anyway

Sorry, I don't understand what is not implemented. Without your patch, the 
PHY driver handles both PMU registers of Exynos4.

Best regards,
Tomasz

> , and if it
> ever will be it's still much easier to special case one weird chip
> than to have a super complicated and confusing mechanism for all of
> them.
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

> Sorry, I don't understand what is not implemented. Without your patch, the
> PHY driver handles both PMU registers of Exynos4.

I don't have an Exynos4 to actually test this, so please let me know
if I'm missing something here... but in order to hit the right HOST
PHY register in the current upstream code, the Exynos4 code would need
to have a hostphy_reg_offset of 4 somewhere in its
samsung_usbphy_drvdata. In my latest checkout of Linus' tree (6c2580c)
it does not (only Exynos5 sets that attribute), so it would default to
0 (thereby actually hitting the DEVICE register).

If you want I can gladly provide another change on top of my patchset
to fix that in the future... but it looks to me like it had been
broken anyway for now.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

> Sorry, I don't understand what is not implemented. Without your patch, the
> PHY driver handles both PMU registers of Exynos4.

I don't have an Exynos4 to actually test this, so please let me know
if I'm missing something here... but in order to hit the right HOST
PHY register in the current upstream code, the Exynos4 code would need
to have a hostphy_reg_offset of 4 somewhere in its
samsung_usbphy_drvdata. In my latest checkout of Linus' tree (6c2580c)
it does not (only Exynos5 sets that attribute), so it would default to
0 (thereby actually hitting the DEVICE register).

If you want I can gladly provide another change on top of my patchset
to fix that in the future... but it looks to me like it had been
broken anyway for now.

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

*Ping!*

Are there still unanswered concerns left with this patch? I hope my
prior mails could clear up why I think that the PMU register
description in the device tree for a specific PHY is represents the
hardware more accurately after my patch, and my analysis of the
Exynos4 situation currently not being implemented (and therefore not
needing to be addressed by this patch) was correct. Please let me know
if you have further objections... and if not, could we agree to have
this picked up somewhere?

Thanks,
Julius

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Julius Werner]

*Ping!*

Are there still unanswered concerns left with this patch? I hope my
prior mails could clear up why I think that the PMU register
description in the device tree for a specific PHY is represents the
hardware more accurately after my patch, and my analysis of the
Exynos4 situation currently not being implemented (and therefore not
needing to be addressed by this patch) was correct. Please let me know
if you have further objections... and if not, could we agree to have
this picked up somewhere?

Thanks,
Julius

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Hi,

On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
> *Ping!*
> 
> Are there still unanswered concerns left with this patch? I hope my
> prior mails could clear up why I think that the PMU register
> description in the device tree for a specific PHY is represents the
> hardware more accurately after my patch, and my analysis of the
> Exynos4 situation currently not being implemented (and therefore not
> needing to be addressed by this patch) was correct. Please let me know
> if you have further objections... and if not, could we agree to have
> this picked up somewhere?

I need acks for DTS part...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Hi,

On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
> *Ping!*
> 
> Are there still unanswered concerns left with this patch? I hope my
> prior mails could clear up why I think that the PMU register
> description in the device tree for a specific PHY is represents the
> hardware more accurately after my patch, and my analysis of the
> Exynos4 situation currently not being implemented (and therefore not
> needing to be addressed by this patch) was correct. Please let me know
> if you have further objections... and if not, could we agree to have
> this picked up somewhere?

I need acks for DTS part...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Tomasz Figa]

Hi Felipe,

On Tuesday 17 of September 2013 10:36:16 Felipe Balbi wrote:
> Hi,
> 
> On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
> > *Ping!*
> > 
> > Are there still unanswered concerns left with this patch? I hope my
> > prior mails could clear up why I think that the PMU register
> > description in the device tree for a specific PHY is represents the
> > hardware more accurately after my patch, and my analysis of the
> > Exynos4 situation currently not being implemented (and therefore not
> > needing to be addressed by this patch) was correct. Please let me know
> > if you have further objections... and if not, could we agree to have
> > this picked up somewhere?
> 
> I need acks for DTS part...

This patch breaks Exynos 4, so it's a NAK from me.

Anyway, a new, completely redesigned PHY driver supporting most of the PHY 
features (as opposed to only the basic subset currently) developed by Kamil 
Debski should show up soon, so I don't think there is any reason to apply 
any patches to this old driver.

Best regards,
Tomasz

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Tomasz Figa]

Hi Felipe,

On Tuesday 17 of September 2013 10:36:16 Felipe Balbi wrote:
> Hi,
> 
> On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
> > *Ping!*
> > 
> > Are there still unanswered concerns left with this patch? I hope my
> > prior mails could clear up why I think that the PMU register
> > description in the device tree for a specific PHY is represents the
> > hardware more accurately after my patch, and my analysis of the
> > Exynos4 situation currently not being implemented (and therefore not
> > needing to be addressed by this patch) was correct. Please let me know
> > if you have further objections... and if not, could we agree to have
> > this picked up somewhere?
> 
> I need acks for DTS part...

This patch breaks Exynos 4, so it's a NAK from me.

Anyway, a new, completely redesigned PHY driver supporting most of the PHY 
features (as opposed to only the basic subset currently) developed by Kamil 
Debski should show up soon, so I don't think there is any reason to apply 
any patches to this old driver.

Best regards,
Tomasz

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

On Tue, Sep 17, 2013 at 05:53:31PM +0200, Tomasz Figa wrote:
> Hi Felipe,
> 
> On Tuesday 17 of September 2013 10:36:16 Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
> > > *Ping!*
> > > 
> > > Are there still unanswered concerns left with this patch? I hope my
> > > prior mails could clear up why I think that the PMU register
> > > description in the device tree for a specific PHY is represents the
> > > hardware more accurately after my patch, and my analysis of the
> > > Exynos4 situation currently not being implemented (and therefore not
> > > needing to be addressed by this patch) was correct. Please let me know
> > > if you have further objections... and if not, could we agree to have
> > > this picked up somewhere?
> > 
> > I need acks for DTS part...
> 
> This patch breaks Exynos 4, so it's a NAK from me.
> 
> Anyway, a new, completely redesigned PHY driver supporting most of the PHY 
> features (as opposed to only the basic subset currently) developed by Kamil 
> Debski should show up soon, so I don't think there is any reason to apply 
> any patches to this old driver.

awesome, then I espect to see a patch deleting the old driver shortly
:-)

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

On Tue, Sep 17, 2013 at 05:53:31PM +0200, Tomasz Figa wrote:
> Hi Felipe,
> 
> On Tuesday 17 of September 2013 10:36:16 Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
> > > *Ping!*
> > > 
> > > Are there still unanswered concerns left with this patch? I hope my
> > > prior mails could clear up why I think that the PMU register
> > > description in the device tree for a specific PHY is represents the
> > > hardware more accurately after my patch, and my analysis of the
> > > Exynos4 situation currently not being implemented (and therefore not
> > > needing to be addressed by this patch) was correct. Please let me know
> > > if you have further objections... and if not, could we agree to have
> > > this picked up somewhere?
> > 
> > I need acks for DTS part...
> 
> This patch breaks Exynos 4, so it's a NAK from me.
> 
> Anyway, a new, completely redesigned PHY driver supporting most of the PHY 
> features (as opposed to only the basic subset currently) developed by Kamil 
> Debski should show up soon, so I don't think there is any reason to apply 
> any patches to this old driver.

awesome, then I espect to see a patch deleting the old driver shortly
:-)

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]