Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support
From: Chris Ruehl
Date: Tue Dec 03 2013 - 00:25:50 EST
On Tuesday, December 03, 2013 02:22 AM, Mark Rutland wrote:
On Mon, Dec 02, 2013 at 07:05:19AM +0000, Chris Ruehl wrote:
usb: phy-generic: Add ULPI VBUS support
Some platforms need to set the VBUS parameters of the ULPI
like ISP1504 which interact with overcurrent protection and
power switch MIC2575. Therefore it requires to set
* DRVVBUS
* DRVVBUS_EXT
* EXTVBUSIND
* CHRGVBUS
of the ULPI.
This patch add support for it.
Could you elaborate on when we need to do this and why?
Devicetree configuration example:
usbphy0: usbphy@0x10024170 {
compatible = "usb-nop-xceiv";
reg =<0x10024170 0x4>; /* ULPI Viewport OTG */
clocks =<&clks 75>;
clock-names = "main_clk";
};
&usbphy0 {
reset-gpios =<&gpio1 31 1>;
pinctrl-names = "default";
pinctrl-0 =<&pinctrl_usbphy0&pinctrl_usbotg1>;
ulpi_set_vbus =<0x0f>;
};
Please refer to the phy-bindings.txt for the value of ulpi_set_vbus commit
with this patch.
Signed-off-by: Chris Ruehl<chris.ruehl@xxxxxxxxxxxx>
---
.../devicetree/bindings/phy/phy-bindings.txt | 15 ++++++
drivers/usb/phy/phy-generic.c | 50 +++++++++++++++++++-
drivers/usb/phy/phy-generic.h | 3 ++
3 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
index 8ae844f..b109b2f 100644
--- a/Documentation/devicetree/bindings/phy/phy-bindings.txt
+++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
@@ -34,6 +34,18 @@ phys : the phandle for the PHY device (used by the PHY subsystem)
phy-names : the names of the PHY corresponding to the PHYs present in the
*phys* phandle
+Optional Properties:
+reset-gpios : GPIO used to reset ULPI like ISP1504 with
+ 0 = reset active high ; 1 = reset active low.
The format of the gpio-specifier doesn't matter here.
Why do you need to mention the ISP1504? Either this is generic, or it
doesn't belong here.
Perhaps we need a ulpi-phy binding document. This and the rest of the
patch is strongly tied to ULPI.
+cs-gpios : GPIO used to activate a ULPI like ISP1504 with
+ 0 = reset acitive high; 1 = reset active low.
Again, the format of the gpio-specifier is not a concern here. I'm also
a little confused as to the name "cs-gpios" for something that activates
the ULPI.
+ulpi_set_vbus : ULPI configuation parameter to program the VBUS signaling of
+ ISP1504 or similar chipsets.
Could you elaborate on this? Is this applicable to any ULPI PHY? The
description implies that it's somewhat tied to ISP1504 (if it's not,
drop the mention of ISP1504).
Why exactly do we need this, and why should it live in the DT?
Why can se not figure this out automatically, or have defaults that work
in more places?
Also, s/_/-/ on property names please.
+ Set the parameter:
+ DRVVBUS = (1) DRVVBUS_EXT = (1<<1) EXTVBUSIND = (1<<2) CHRGVBUS = (1<<3)
+ eg: DRVVBUS | DRVVBUS_EXT = 0x03
+ ulpi_set_vbus =<0x03>
I don't like putting arbitrary bitfields like this in DT. They're
illegible and easy to abuse.
Boolean properties are nicer for flags.
What exactly do these flags mean?
[...]
@@ -253,14 +284,15 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
if (dev->of_node) {
struct device_node *node = dev->of_node;
+ struct resource *res;
enum of_gpio_flags flags;
enum of_gpio_flags csflags;
+ u32 ulpi_vbus;
if (of_property_read_u32(node, "clock-frequency",&clk_rate))
clk_rate = 0;
needs_vcc = of_property_read_bool(node, "vcc-supply");
-
nop->gpio_reset = of_get_named_gpio_flags(node, "reset-gpios",
0,&flags);
Why the random line deletion?
@@ -274,6 +306,22 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
if (gpio_is_valid(nop->gpio_chipselect))
nop->cs_active_low = csflags& OF_GPIO_ACTIVE_LOW;
+ err = of_property_read_u32(node, "ulpi_set_vbus",&ulpi_vbus);
+ if (err) {
+ nop->ulpi_vbus = -1;
+ nop->viewport = NULL;
+ ulpi_vbus = 0;
+ } else {
+ dev_dbg(dev,"ULPI ulpi_set_vbus 0x%02x",ulpi_vbus);
+ nop->ulpi_vbus = ulpi_vbus;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
This wasn't described in the binding document, and it's ULPI specific. I
think we need a separate ulpi-phy binding that builds upon the generic
one.
Mark, before commit an other patch, is it this what you looking for?
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-ulpi.txt
@@ -0,0 +1,36 @@
+This document explains only the device tree data binding for phy-ulpi nodes.
For general
+information about PHY subsystem refer to Documentation/phy.txt
+
+PHY device node
+===============
+
+Required Properties:
+reg: ULPI Viewport
+reset-gpios : GPIO used to reset ULPI chipset
+
+Optional Properties:
+cs-gpios : GPIO used to activate a ULPI chipset
+ulpi-set-vbus : ULPI configuation parameter to program the VBUS logic
+ Set the parameter:
+ DRVVBUS = (1<<0) Enable VBUS logic
+ DRVVBUS_EXT = (1<<1) Enable External VBUS logic
+ EXTVBUSIND = (1<<2) Enable Ext. VBUS indication and
fault handler
+ CHRGVBUS = (1<<3) Enable VBUS charge pump logic
+ eg: DRVVBUS | DRVVBUS_EXT = 0x03
+ ulpi_set_vbus = <0x03>
+
+For example:
+
+phys: phy@p {
+ compatible = "xxx";
+ reg = <p>;
+ .
+ .
+ #phy-cells = <1>;
+ .
+ .
+ reset-gpios = <&gpioX nn n>
+ cs-gpios = <&gpioX nn n>
+ ulpi-set-vbus = <0xXX>;
+};
+
Thanks
Chris
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
GTSYS Limited RFID Technology
A01 24/F Gold King Industrial Bld
35-41 Tai Lin Pai Road, Kwai Chung, Hong Kong
Fax (852) 8167 4060 - Tel (852) 3598 9488
Disclaimer: http://www.gtsys.com.hk/email/classified.html
--
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/