RE: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi Anton, From: Anton Tikhomirov [mailto:av.tikhomi...@samsung.com] Sent: Tuesday, December 10, 2013 3:43 AM Hi Kamil, Same USB2.0 PHY may be used by several HCDs, for example EHCI and OHCI. Consider the situation, when EHCI stops using the PHY and calls power_off, then OHCI becomes non-operational. In other words, PHY power_on and power_off calls must be balanced. Shall we handle it in your driver? (usage count?) Please look in the drivers/phy/phy-core.c file. Usage count is handled there - see phy_power_on and phy_power_off functions. I understand that after both EHCI and OHCI power on the phy, the usage count is 2. So powering off one of them (EHCI for instance) the usage count is still 1, so the OHCI should still work properly. [snip] Best wishes, -- Kamil Debski Samsung RD Institute Poland -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi Kamil, Hi Anton, From: Anton Tikhomirov [mailto:av.tikhomi...@samsung.com] Sent: Tuesday, December 10, 2013 3:43 AM Hi Kamil, Same USB2.0 PHY may be used by several HCDs, for example EHCI and OHCI. Consider the situation, when EHCI stops using the PHY and calls power_off, then OHCI becomes non-operational. In other words, PHY power_on and power_off calls must be balanced. Shall we handle it in your driver? (usage count?) Please look in the drivers/phy/phy-core.c file. Usage count is handled there - see phy_power_on and phy_power_off functions. I understand that after both EHCI and OHCI power on the phy, the usage count is 2. So powering off one of them (EHCI for instance) the usage count is still 1, so the OHCI should still work properly. Oops, sorry, missed that. [snip] Best wishes, -- Kamil Debski Samsung RD Institute Poland -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi,
From: Kishon Vijay Abraham I [mailto:kis...@ti.com]
Sent: Monday, December 09, 2013 8:56 AM
Hi,
On Friday 06 December 2013 09:58 PM, Kamil Debski wrote:
Hi Kishon,
Thank you for the review.
From: Kishon Vijay Abraham I [mailto:kis...@ti.com]
Sent: Friday, December 06, 2013 11:59 AM
Hi,
On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
Add a new driver for the Exynos USB PHY. The new driver uses the
generic PHY framework. The driver includes support for the Exynos
4x10
and 4x12 SoC families.
Signed-off-by: Kamil Debski k.deb...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
snip
.
.
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
d0caae9..9f4befd 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -7,3 +7,6 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-
exynos-dp-
video.o
obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-
video.o
obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o
obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
+obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-samsung-usb2.o
+obj-$(CONFIG_PHY_EXYNOS4210_USB2)+= phy-exynos4210-usb2.o
+obj-$(CONFIG_PHY_EXYNOS4212_USB2)+= phy-exynos4212-usb2.o
diff --git a/drivers/phy/phy-exynos4210-usb2.c
b/drivers/phy/phy-exynos4210-usb2.c
new file mode 100644
index 000..a02e5c2
--- /dev/null
+++ b/drivers/phy/phy-exynos4210-usb2.c
@@ -0,0 +1,264 @@
+/*
+ * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Kamil Debski k.deb...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2
+as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/clk.h
+#include linux/delay.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/phy/phy.h
+#include linux/platform_device.h
+#include linux/regmap.h
+#include linux/spinlock.h
You've included most of the above header files in phy-samsung-usb2.h
which you are including below.
I agree that includes in phy-samsung-usb2.h could use a cleanup. On
the other hand my opinion is that a .c file should include all .h
files that are used in this .c file. Relaying on .h file to include
another .h doesn't seem good to me.
then remove it in .h file.
+#include phy-samsung-usb2.h
+
+/* Exynos USB PHY registers */
+
+/* PHY power control */
+#define EXYNOS_4210_UPHYPWR 0x0
+
+#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND (1 0)
use BIT() here and everywhere below.
snip
.
.
+#ifdef CONFIG_PHY_EXYNOS4212_USB2
+ {
+ .compatible = samsung,exynos4212-usb2-phy,
+ .data = exynos4212_usb2_phy_config,
+ },
+#endif
+ { },
+};
I think we've had enough discussion about this approach. Let's get
the opinion of others too. Felipe? Greg?
Good idea.
Summary:
We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
almost similar register map [1] and a samsung helper driver for
these
two drivers.
I would not call them separate drivers. It's a single USB 2.0 driver
with the option to include support for various SoCs. This patchset
adds:
Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that
another person is working on supporting S3C6410.
These two PHY drivers populate the function pointers in the helper
driver. So any phy_ops will first invoke the helper driver which
will
then invoke the corresponding phy driver.
[1] - http://www.diffchecker.com/7yno1uvk
Come on, this diff only includes the registers part of the file.
The following functions are also different:
- exynos421*_rate_to_clk
- exynos421*_isol
- exynos421*_phy_pwr
- exynos421*_power_on
- exynos421*_power_on
But most of the differences is because your 4212 has additional
features in HSIC and supports more clock rates.
It seems that the file is too large for the tool. But still this
makes
a false impression that only registers are different.
Advantages:
* (more) clean and readable
* helper driver can be used with other PHY drivers which will be
added soon
Disadvantages:
* code duplication
I would say that actually in this case less code is duplicated.
Having
Separate drivers would mean that most of the phy-samsung-usb2.c file
has
I actually meant a single driver for 4210 and 4212.
your current code has separate drivers for different versions of the
same IP. If you have a single driver for the different versions, it
will lead to a lot less code duplication (hint: I'v given the exact
'same'
comment at-least twice in this patch).
You wrote more than
Re: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi,
On Friday 06 December 2013 09:58 PM, Kamil Debski wrote:
Hi Kishon,
Thank you for the review.
From: Kishon Vijay Abraham I [mailto:kis...@ti.com]
Sent: Friday, December 06, 2013 11:59 AM
Hi,
On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
Add a new driver for the Exynos USB PHY. The new driver uses the
generic PHY framework. The driver includes support for the Exynos
4x10
and 4x12 SoC families.
Signed-off-by: Kamil Debski k.deb...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
snip
.
.
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index
d0caae9..9f4befd 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -7,3 +7,6 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-
video.o
obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o
obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
+obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-samsung-usb2.o
+obj-$(CONFIG_PHY_EXYNOS4210_USB2) += phy-exynos4210-usb2.o
+obj-$(CONFIG_PHY_EXYNOS4212_USB2) += phy-exynos4212-usb2.o
diff --git a/drivers/phy/phy-exynos4210-usb2.c
b/drivers/phy/phy-exynos4210-usb2.c
new file mode 100644
index 000..a02e5c2
--- /dev/null
+++ b/drivers/phy/phy-exynos4210-usb2.c
@@ -0,0 +1,264 @@
+/*
+ * Samsung SoC USB 1.1/2.0 PHY driver - Exynos 4210 support
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Kamil Debski k.deb...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/clk.h
+#include linux/delay.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of.h
+#include linux/of_address.h
+#include linux/phy/phy.h
+#include linux/platform_device.h
+#include linux/regmap.h
+#include linux/spinlock.h
You've included most of the above header files in phy-samsung-usb2.h
which you are including below.
I agree that includes in phy-samsung-usb2.h could use a cleanup. On the
other
hand my opinion is that a .c file should include all .h files that are used
in
this .c file. Relaying on .h file to include another .h doesn't seem good to
me.
then remove it in .h file.
+#include phy-samsung-usb2.h
+
+/* Exynos USB PHY registers */
+
+/* PHY power control */
+#define EXYNOS_4210_UPHYPWR0x0
+
+#define EXYNOS_4210_UPHYPWR_PHY0_SUSPEND (1 0)
use BIT() here and everywhere below.
snip
.
.
+#ifdef CONFIG_PHY_EXYNOS4212_USB2
+ {
+ .compatible = samsung,exynos4212-usb2-phy,
+ .data = exynos4212_usb2_phy_config,
+ },
+#endif
+ { },
+};
I think we've had enough discussion about this approach. Let's get the
opinion of others too. Felipe? Greg?
Good idea.
Summary:
We have two drivers PHY_EXYNOS4210_USB2 and PHY_EXYNOS4212_USB2 with
almost similar register map [1] and a samsung helper driver for these
two drivers.
I would not call them separate drivers. It's a single USB 2.0 driver with
the option to include support for various SoCs. This patchset adds:
Exynos 4210, Exynos 4212, Exynos 5250 and S5PCV210. I know that another
person is working on supporting S3C6410.
These two PHY drivers populate the function pointers in the helper
driver. So any phy_ops will first invoke the helper driver which will
then invoke the corresponding phy driver.
[1] - http://www.diffchecker.com/7yno1uvk
Come on, this diff only includes the registers part of the file.
The following functions are also different:
- exynos421*_rate_to_clk
- exynos421*_isol
- exynos421*_phy_pwr
- exynos421*_power_on
- exynos421*_power_on
But most of the differences is because your 4212 has additional features
in HSIC and supports more clock rates.
It seems that the file is too large for the tool. But still this makes a
false impression that only registers are different.
Advantages:
* (more) clean and readable
* helper driver can be used with other PHY drivers which will be added
soon
Disadvantages:
* code duplication
I would say that actually in this case less code is duplicated. Having
Separate drivers would mean that most of the phy-samsung-usb2.c file has
I actually meant a single driver for 4210 and 4212.
your current code has separate drivers for different versions of the
same IP. If you have a single driver for the different versions, it will
lead to a lot less code duplication (hint: I've given the exact 'same'
comment at-least twice in this patch). There are quite a few examples in
the kernel where the same driver is used for multiple versions of the IP.
to be repeated. That is 240 times 4 (and surely more in the future, as
this patchset adds support for 4 SoCs). Which is almost 1000 lines more.
Maybe having a helper driver makes sense when we have other samsung PHY
drivers
Re: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi,
On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
Add a new driver for the Exynos USB PHY. The new driver uses the generic
PHY framework. The driver includes support for the Exynos 4x10 and 4x12
SoC families.
Signed-off-by: Kamil Debski k.deb...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../devicetree/bindings/phy/samsung-usbphy.txt | 54
drivers/phy/Kconfig| 20 ++
drivers/phy/Makefile |3 +
drivers/phy/phy-exynos4210-usb2.c | 264 +
drivers/phy/phy-exynos4212-usb2.c | 312
drivers/phy/phy-samsung-usb2.c | 228 ++
drivers/phy/phy-samsung-usb2.h | 72 +
7 files changed, 953 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/samsung-usbphy.txt
create mode 100644 drivers/phy/phy-exynos4210-usb2.c
create mode 100644 drivers/phy/phy-exynos4212-usb2.c
create mode 100644 drivers/phy/phy-samsung-usb2.c
create mode 100644 drivers/phy/phy-samsung-usb2.h
diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
new file mode 100644
index 000..cadbf70
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
use the existing samsung-phy.txt.
@@ -0,0 +1,54 @@
+Samsung S5P/EXYNOS SoC series USB PHY
+-
+
+Required properties:
+- compatible : should be one of the listed compatibles:
+ - samsung,exynos4210-usb2-phy
+ - samsung,exynos4212-usb2-phy
+- reg : a list of registers used by phy driver
+ - first and obligatory is the location of phy modules registers
+- samsung,sysreg-phandle - handle to syscon used to control the system
registers
+- samsung,pmureg-phandle - handle to syscon used to control PMU registers
+- #phy-cells : from the generic phy bindings, must be 1;
+- clocks and clock-names:
+ - the phy clocks is required by the phy module
+ - next for each of the phys a clock has to be assidned, this clock
%s/assidned/assigned/
+ will be used to determine clocking frequency for the phys
+ (the labels are specified in the paragraph below)
+
+The first phandle argument in the PHY specifier identifies the PHY, its
+meaning is compatible dependent. For the currently supported SoCs (Exynos
4210
+and Exynos 4212) it is as follows:
+ 0 - USB device (device),
+ 1 - USB host (host),
+ 2 - HSIC0 (hsic0),
+ 3 - HSIC1 (hsic1),
+
+Exynos 4210 and Exynos 4212 use mode switching and require that mode switch
+register is supplied.
+
+Example:
+
+For Exynos 4412 (compatible with Exynos 4212):
+
+usbphy: phy@125B {
use lower case for address here...
+ compatible = samsung,exynos4212-usb2-phy;
+ reg = 0x125B 0x100 0x10020704 0x0c 0x1001021c 0x4;
and here..
+ clocks = clock 305, clock 2, clock 2, clock 2,
+ clock 2;
+ clock-names = phy, device, host, hsic0, hsic1;
+ status = okay;
+ #phy-cells = 1;
+ samsung,sysreg-phandle = sys_reg;
+ samsung,pmureg-phandle = pmu_reg;
+};
+
+Then the PHY can be used in other nodes such as:
+
+phy-consumer@1234 {
+ phys = usbphy 2;
+ phy-names = phy;
+};
+
+Refer to DT bindings documentation of particular PHY consumer devices for
more
+information about required PHYs and the way of specification.
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index a344f3d..b29018f 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO
help
Support for Display Port PHY found on Samsung EXYNOS SoCs.
+config PHY_SAMSUNG_USB2
+ tristate Samsung USB 2.0 PHY driver
+ help
+ Enable this to support Samsung USB phy helper driver for Samsung SoCs.
+ This driver provides common interface to interact, for Samsung
+ USB 2.0 PHY driver.
+
+config PHY_EXYNOS4210_USB2
+ bool Support for Exynos 4210
+ depends on PHY_SAMSUNG_USB2
+ depends on CPU_EXYNOS4210
select GENERIC_PHY here?
+ help
+ Enable USB PHY support for Exynos 4210
Add more explanation here and make checkpatch happy.
+
+config PHY_EXYNOS4212_USB2
+ bool Support for Exynos 4212
+ depends on PHY_SAMSUNG_USB2
+ depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
select GENERIC_PHY.
+ help
+ Enable USB PHY support for Exynos 4212
more explanation here too..
endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index d0caae9..9f4befd 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -7,3 +7,6 @@ obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-video.o
obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
RE: [PATCH v4 3/9] phy: Add new Exynos USB PHY driver
Hi Kishon,
Thank you for the review.
From: Kishon Vijay Abraham I [mailto:kis...@ti.com]
Sent: Friday, December 06, 2013 11:59 AM
Hi,
On Thursday 05 December 2013 05:59 PM, Kamil Debski wrote:
Add a new driver for the Exynos USB PHY. The new driver uses the
generic PHY framework. The driver includes support for the Exynos
4x10
and 4x12 SoC families.
Signed-off-by: Kamil Debski k.deb...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
.../devicetree/bindings/phy/samsung-usbphy.txt | 54
drivers/phy/Kconfig| 20 ++
drivers/phy/Makefile |3 +
drivers/phy/phy-exynos4210-usb2.c | 264
+
drivers/phy/phy-exynos4212-usb2.c | 312
drivers/phy/phy-samsung-usb2.c | 228
++
drivers/phy/phy-samsung-usb2.h | 72 +
7 files changed, 953 insertions(+)
create mode 100644
Documentation/devicetree/bindings/phy/samsung-usbphy.txt
create mode 100644 drivers/phy/phy-exynos4210-usb2.c create mode
100644 drivers/phy/phy-exynos4212-usb2.c create mode 100644
drivers/phy/phy-samsung-usb2.c create mode 100644
drivers/phy/phy-samsung-usb2.h
diff --git a/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
new file mode 100644
index 000..cadbf70
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/samsung-usbphy.txt
use the existing samsung-phy.txt.
Ok.
@@ -0,0 +1,54 @@
+Samsung S5P/EXYNOS SoC series USB PHY
+-
+
+Required properties:
+- compatible : should be one of the listed compatibles:
+ - samsung,exynos4210-usb2-phy
+ - samsung,exynos4212-usb2-phy
+- reg : a list of registers used by phy driver
+ - first and obligatory is the location of phy modules registers
+- samsung,sysreg-phandle - handle to syscon used to control the
+system registers
+- samsung,pmureg-phandle - handle to syscon used to control PMU
+registers
+- #phy-cells : from the generic phy bindings, must be 1;
+- clocks and clock-names:
+ - the phy clocks is required by the phy module
+ - next for each of the phys a clock has to be assidned, this
clock
%s/assidned/assigned/
Thank you for spotting this.
+ will be used to determine clocking frequency for the phys
+ (the labels are specified in the paragraph below)
+
+The first phandle argument in the PHY specifier identifies the PHY,
+its meaning is compatible dependent. For the currently supported
SoCs
+(Exynos 4210 and Exynos 4212) it is as follows:
+ 0 - USB device (device),
+ 1 - USB host (host),
+ 2 - HSIC0 (hsic0),
+ 3 - HSIC1 (hsic1),
+
+Exynos 4210 and Exynos 4212 use mode switching and require that mode
+switch register is supplied.
+
+Example:
+
+For Exynos 4412 (compatible with Exynos 4212):
+
+usbphy: phy@125B {
use lower case for address here...
Ok.
+ compatible = samsung,exynos4212-usb2-phy;
+ reg = 0x125B 0x100 0x10020704 0x0c 0x1001021c 0x4;
and here..
+ clocks = clock 305, clock 2, clock 2, clock 2,
+ clock 2;
+ clock-names = phy, device, host, hsic0, hsic1;
+ status = okay;
+ #phy-cells = 1;
+ samsung,sysreg-phandle = sys_reg;
+ samsung,pmureg-phandle = pmu_reg; };
+
+Then the PHY can be used in other nodes such as:
+
+phy-consumer@1234 {
+ phys = usbphy 2;
+ phy-names = phy;
+};
+
+Refer to DT bindings documentation of particular PHY consumer
devices
+for more information about required PHYs and the way of
specification.
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index
a344f3d..b29018f 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -51,4 +51,24 @@ config PHY_EXYNOS_DP_VIDEO
help
Support for Display Port PHY found on Samsung EXYNOS SoCs.
+config PHY_SAMSUNG_USB2
+ tristate Samsung USB 2.0 PHY driver
+ help
+ Enable this to support Samsung USB phy helper driver for
Samsung SoCs.
+ This driver provides common interface to interact, for Samsung
+ USB 2.0 PHY driver.
+
+config PHY_EXYNOS4210_USB2
+ bool Support for Exynos 4210
+ depends on PHY_SAMSUNG_USB2
+ depends on CPU_EXYNOS4210
select GENERIC_PHY here?
I think that depends on PHY_SAMSUNG_USB2 is better in this place.
However, I agree that I should add select GENERIC_PHY to PHY_SAMSUNG_USB2.
The reason why I am saying this is that I like how it looks in
menuconfig. Selecting PHY_SAMSUNG_USB2 expands more options and if
unselected the menu looks tidier.
+ help
+ Enable USB PHY support for Exynos 4210
Add more explanation here and make checkpatch happy.
Here I think we should not treat
