Re: [PATCH V4 1/5] phy: Add new Exynos5 USB 3.0 PHY driver

From: Tomasz Figa
Date: Mon Apr 14 2014 - 09:50:15 EST

On 14.04.2014 15:40, Vivek Gautam wrote:
On Mon, Apr 14, 2014 at 6:56 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:


On Monday 14 April 2014 06:50 PM, Vivek Gautam wrote:
On Mon, Apr 14, 2014 at 6:29 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:


On Monday 14 April 2014 06:12 PM, Vivek Gautam wrote:
Hi,


On Mon, Apr 14, 2014 at 5:57 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
Hi,

On Tuesday 08 April 2014 08:06 PM, Vivek Gautam wrote:
Add a new driver for the USB 3.0 PHY on Exynos5 series of SoCs.
The new driver uses the generic PHY framework and will interact
with DWC3 controller present on Exynos5 series of SoCs.
Thereby, removing old phy-samsung-usb3 driver and related code
used untill now which was based on usb/phy framework.

Signed-off-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
---
.../devicetree/bindings/phy/samsung-phy.txt | 42 ++
drivers/phy/Kconfig | 11 +
drivers/phy/Makefile | 1 +
drivers/phy/phy-exynos5-usbdrd.c | 668 ++++++++++++++++++++
4 files changed, 722 insertions(+)
create mode 100644 drivers/phy/phy-exynos5-usbdrd.c

diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index 28f9edb..6d99ba9 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -74,3 +74,45 @@ phy-consumer@12340000 {

Refer to DT bindings documentation of particular PHY consumer devices for more
information about required PHYs and the way of specification.
+
+Samsung Exynos5 SoC series USB DRD PHY controller
+--------------------------------------------------
+
+Required properties:
+- compatible : Should be set to one of the following supported values:
+ - "samsung,exynos5250-usbdrd-phy" - for exynos5250 SoC,
+ - "samsung,exynos5420-usbdrd-phy" - for exynos5420 SoC.
+- reg : Register offset and length of USB DRD PHY register set;
+- clocks: Clock IDs array as required by the controller
+- clock-names: names of clocks correseponding to IDs in the clock property;
+ Required clocks:
+ - phy: main PHY clock (same as USB DRD controller i.e. DWC3 IP clock),
+ used for register access.
+ - ref: PHY's reference clock (usually crystal clock), associated by
+ phy name, used to determine bit values for clock settings
+ register.
+ Additional clock required for Exynos5420:
+ - usb30_sclk_100m: Additional special clock used for PHY operation
+ depicted as 'sclk_usbphy30' in CMU of Exynos5420.
+- samsung,syscon-phandle: phandle for syscon interface, which is used to
+ control pmu registers for power isolation.
+- samsung,pmu-offset: phy power control register offset to pmu-system-controller
+ base.
+- #phy-cells : from the generic PHY bindings, must be 1;
+
+For "samsung,exynos5250-usbdrd-phy" and "samsung,exynos5420-usbdrd-phy"
+compatible PHYs, the second cell in the PHY specifier identifies the
+PHY id, which is interpreted as follows:
+ 0 - UTMI+ type phy,
+ 1 - PIPE3 type phy,
+
+Example:
+ usb3_phy: usbphy@12100000 {
+ compatible = "samsung,exynos5250-usbdrd-phy";
+ reg = <0x12100000 0x100>;
+ clocks = <&clock 286>, <&clock 1>;
+ clock-names = "phy", "usb3phy_refclk";
+ samsung,syscon-phandle = <&pmu_syscon>;
+ samsung,pmu-offset = <0x704>;
+ #phy-cells = <1>;
+ };
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 8d3c49c..d955a05 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -166,4 +166,15 @@ config PHY_XGENE
help
This option enables support for APM X-Gene SoC multi-purpose PHY.

+config PHY_EXYNOS5_USBDRD
+ tristate "Exynos5 SoC series USB DRD PHY driver"
+ depends on ARCH_EXYNOS5 && OF
+ depends on HAS_IOMEM
+ select GENERIC_PHY
+ select MFD_SYSCON

Lets try to avoid select in Kconfig. We've got enough problems with that.

I hope you meant with "select MFD_SYSCON".
We are referencing the syscon for accessing pmu reg, for which we need
this config to be selected.
Other Exynos phy drivers also need this config and for that they have
selected this.

Do you want me to do it any other way ?

depends on is one option.

Ok, i can see there are places where depends_on MFD_SYSCON is used.
"drivers/gpu/drm/exynos/Kconfig:60"

so, do you want me to fix at other places too ?

But i also have a question here.
MFD_SYSCON is a subsystem that's facilitating us in getting our work
done here by giving an access to pmu_system_controller.
So unless MFD_SYSCON is exposed, the phy driver will not be exposed to us.
So is that valid enough really ?

maybe in the Kconfig for MFD_SYSCON, we can select it if PHY_EXYNOS5_USBDRD is
enabled?

config MFD_SYSCON
default y if PHY_EXYNOS5_USBDRD

Yes, that seems to be one option. But we will end up adding this
dependency for all the drivers which want to use syscon.

Adding few people who worked on MFS_SYSCON, just to gather ideas.

Re-quoting our problem here:
"Inside our phy driver we are accessing power mgt unit registers,
which we register using syscon in ut device tree.
Now we don't want to select MFD_SYSCON from our phy driver, instead
want to go 'depend_on' way."

So one solution seems to be enabling MFD_SYSCON by default, if any of
driver using syscon is enabled.
Can we use some other way here ?

This is an awful idea from maintainability perspective... This means that every driver depending on MFD_SYSCON would have to add new "default y if" to MFD_SYSCON Kconfig entry. Even with respect to readability, it is more logical to have all dependencies of a driver listed in its Kconfig entry.

Anyway, PHY_EXYNOS5_USBDRD can't work without MFD_SYSCON and somehow this needs to be stated. I believe select is the right thing here and I'm not sure why it could be a problem. The biggest reason for going this way is that users wouldn't have to care about internal dependencies, instead they would simply enable drivers for their hardware.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/