[PATCH] usb: phy-generic, phy-ulpi patch set

[PATCH] usb: phy-generic, phy-ulpi patch set

[Chris Ruehl]

This patch set add support 
* GPIO ChipSelect
* ULPI support to set VBUS flags for EXTVBUSIND,CHRGVBUS
* ULPI support to set VBUS flags from DT

[PATCH 1/3] usb: phy-generic: Add GPIO based ChipSelect
	The ULPI ISP1504 uses the CHIP_SELECT_N (low active) pin to active the
	chip. This patch add support for GPIO based ChipSelect almost the same
	way implemented for the Reset feature.

[PATCH 2/3] usb: phy-ulpi: Add EXTVBUSIND,CHRGVBUS flag support
	ULPI like ISP1504 support external vbus power indication
	used in combination with vbus switches mic2075.

[PATCH 3/3] 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.

Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
---

[PATCH 1/3] usb: phy-generic: Add GPIO based ChipSelect

[Chris Ruehl]

usb: phy-generic: Add GPIO based ChipSelect

The ULPI ISP1504 uses the CHIP_SELECT_N (low active) pin to active the
chip. This patch add support for GPIO based ChipSelect almost the same
way implemented for the Reset feature.

Sample DT configuration:
pinctrl_usbphy2: usbphy-2 {
fsl,pins = <
	/* chip select and reset gpio output */
	MX27_PAD_PC_CD1_B__GPIO6_20 0x0
	MX27_PAD_PC_CD2_B__GPIO6_19 0x0
	>;
};

&usbphy2 {
        cs-gpios = <&gpio6 20 1>;
	reset-gpios = <&gpio6 19 1>;
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_usbphy2>;
};

Signed-off-by: Chris Ruehl <chris.ruehl-CR359r9tUDPXPF5Rlphj1Q@public.gmane.org>
---
 drivers/usb/phy/phy-generic.c |   31 +++++++++++++++++++++++++++++++
 drivers/usb/phy/phy-generic.h |    2 ++
 2 files changed, 33 insertions(+)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index fce3a9e..ff4d68c 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -181,6 +181,24 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop,
 			return -EPROBE_DEFER;
 	}
 
+	if(gpio_is_valid(nop->gpio_chipselect)) {
+		unsigned long gpio_flags;
+		dev_dbg(dev, "Requesting Chipselect GPIO %d\n",
+				nop->gpio_chipselect);
+		if (nop->cs_active_low)
+			gpio_flags = GPIOF_OUT_INIT_LOW;
+		else
+			gpio_flags = GPIOF_OUT_INIT_HIGH;
+
+		err = devm_gpio_request_one(dev, nop->gpio_chipselect,
+				gpio_flags, dev_name(dev));
+		if (err) {
+			dev_err(dev, "Error requesting Chipselect GPIO %d err=%d\n",
+					nop->gpio_chipselect, err);
+			return err;
+		}
+	}
+
 	if (gpio_is_valid(nop->gpio_reset)) {
 		unsigned long gpio_flags;
 
@@ -231,27 +249,40 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	nop->reset_active_low = true;	/* default behaviour */
+	nop->cs_active_low = true;
 
 	if (dev->of_node) {
 		struct device_node *node = dev->of_node;
 		enum of_gpio_flags flags;
+		enum of_gpio_flags csflags;
 
 		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);
+
 		if (nop->gpio_reset == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 
 		nop->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
 
+		nop->gpio_chipselect = of_get_named_gpio_flags(node, "cs-gpios",
+								0, &csflags);
+		if (gpio_is_valid(nop->gpio_chipselect))
+			nop->cs_active_low = csflags & OF_GPIO_ACTIVE_LOW;
+
 	} else if (pdata) {
 		type = pdata->type;
 		clk_rate = pdata->clk_rate;
 		needs_vcc = pdata->needs_vcc;
 		nop->gpio_reset = pdata->gpio_reset;
+		nop->gpio_chipselect = pdata->gpio_chipselect;
+	} else {
+		nop->gpio_reset = -1;
+		nop->gpio_chipselect = -1;
 	}
 
 	err = usb_phy_gen_create_phy(dev, nop, type, clk_rate, needs_vcc);
diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h
index d2a220d..97eafc2 100644
--- a/drivers/usb/phy/phy-generic.h
+++ b/drivers/usb/phy/phy-generic.h
@@ -7,7 +7,9 @@ struct usb_phy_gen_xceiv {
 	struct clk *clk;
 	struct regulator *vcc;
 	int gpio_reset;
+	int gpio_chipselect;
 	bool reset_active_low;
+	bool cs_active_low;
 };
 
 int usb_gen_phy_init(struct usb_phy *phy);
-- 
1.7.10.4

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

[PATCH 2/3] usb: phy-ulpi: Add EXTVBUSIND,CHRGVBUS flag support

[Chris Ruehl]

usb: phy-ulpi: Add EXTVBUSIND,CHRGVBUS flag support

ULPI like ISP1504 support external vbus power indication
used in combination with vbus switches mic2075.

Signed-off-by: Chris Ruehl <chris.ruehl-CR359r9tUDPXPF5Rlphj1Q@public.gmane.org>
---
 drivers/usb/phy/phy-ulpi.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-ulpi.c b/drivers/usb/phy/phy-ulpi.c
index 217339d..e2f15c4 100644
--- a/drivers/usb/phy/phy-ulpi.c
+++ b/drivers/usb/phy/phy-ulpi.c
@@ -180,6 +180,8 @@ static int ulpi_init(struct usb_phy *phy)
 	int i, vid, pid, ret;
 	u32 ulpi_id = 0;
 
+	pr_info("ULPI Viewport 0x%p\n",phy->io_priv);
+
 	for (i = 0; i < 4; i++) {
 		ret = usb_phy_io_read(phy, ULPI_PRODUCT_ID_HIGH - i);
 		if (ret < 0)
@@ -237,7 +239,8 @@ static int ulpi_set_vbus(struct usb_otg *otg, bool on)
 	struct usb_phy *phy = otg->phy;
 	unsigned int flags = usb_phy_io_read(phy, ULPI_OTG_CTRL);
 
-	flags &= ~(ULPI_OTG_CTRL_DRVVBUS | ULPI_OTG_CTRL_DRVVBUS_EXT);
+	flags &= ~(ULPI_OTG_CTRL_DRVVBUS | ULPI_OTG_CTRL_DRVVBUS_EXT |
+			ULPI_OTG_CTRL_EXTVBUSIND | ULPI_OTG_CTRL_CHRGVBUS);
 
 	if (on) {
 		if (phy->flags & ULPI_OTG_DRVVBUS)
@@ -245,6 +248,12 @@ static int ulpi_set_vbus(struct usb_otg *otg, bool on)
 
 		if (phy->flags & ULPI_OTG_DRVVBUS_EXT)
 			flags |= ULPI_OTG_CTRL_DRVVBUS_EXT;
+
+		if (phy->flags & ULPI_OTG_EXTVBUSIND)
+			flags |= ULPI_OTG_CTRL_EXTVBUSIND;
+
+		if (phy->flags & ULPI_OTG_CHRGVBUS)
+			flags |= ULPI_OTG_CTRL_CHRGVBUS;
 	}
 
 	return usb_phy_io_write(phy, flags, ULPI_OTG_CTRL);
-- 
1.7.10.4

--
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

[PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Chris Ruehl]

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.

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@gtsys.com.hk>
---
 .../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.
+cs-gpios :	GPIO used to activate a ULPI like ISP1504 with
+		0 = reset acitive high; 1 = reset active low.
+ulpi_set_vbus :	ULPI configuation parameter to program the VBUS signaling of 
+		ISP1504 or similar chipsets.
+		Set the parameter:
+		DRVVBUS = (1) DRVVBUS_EXT = (1<<1) EXTVBUSIND = (1<<2) CHRGVBUS = (1<<3)
+		eg: DRVVBUS | DRVVBUS_EXT = 0x03
+		    ulpi_set_vbus = <0x03>
+
 Example 1:
 usb1: usb_otg_ss@xxx {
     compatible = "xxx";
@@ -44,6 +56,9 @@ usb1: usb_otg_ss@xxx {
     phy-names = "usb2phy", "usb3phy";
     .
     .
+    reset-gpios = <&gpio6 19 0> /*high active pin */
+    cs-gpios = <&gpio6 20 1> /* low active pin */
+    ulpi_set_vbus = <0x0f>;
 };
 
 This node represents a controller that uses two PHYs, one for usb2 and one for
diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index ff4d68c..9c26b58 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -30,6 +30,7 @@
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/ulpi.h>
 #include <linux/usb/usb_phy_gen_xceiv.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
@@ -99,6 +100,11 @@ int usb_gen_phy_init(struct usb_phy *phy)
 	/* De-assert RESET */
 	nop_reset_set(nop, 0);
 
+	if (nop->ulpi_vbus > 0) {
+		nop->ulpi->init(nop->ulpi);
+		nop->ulpi->otg->set_vbus(nop->ulpi->otg,true);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_gen_phy_init);
@@ -107,6 +113,10 @@ void usb_gen_phy_shutdown(struct usb_phy *phy)
 {
 	struct usb_phy_gen_xceiv *nop = dev_get_drvdata(phy->dev);
 
+	if (nop->ulpi_vbus > 0) {
+		nop->ulpi->otg->set_vbus(nop->ulpi->otg,false);
+	}
+
 	/* Assert RESET */
 	nop_reset_set(nop, 1);
 
@@ -154,6 +164,27 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop,
 {
 	int err;
 
+	if (nop->ulpi_vbus > 0) {
+		unsigned int flags = 0;
+
+		if (nop->ulpi_vbus & 0x1)
+			flags |= ULPI_OTG_DRVVBUS;
+		if (nop->ulpi_vbus & 0x2)
+			flags |= ULPI_OTG_DRVVBUS_EXT;
+		if (nop->ulpi_vbus & 0x4)
+			flags |= ULPI_OTG_EXTVBUSIND;
+		if (nop->ulpi_vbus & 0x8)
+			flags |= ULPI_OTG_CHRGVBUS;
+
+		nop->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, flags);
+		if (!nop->ulpi) {
+			dev_err(dev, "Failed create ULPI Phy\n");
+			return -ENOMEM;
+		}
+		dev_dbg(dev, "Create ULPI Phy\n");
+		nop->ulpi->io_priv =  nop->viewport;
+	}
+
 	nop->phy.otg = devm_kzalloc(dev, sizeof(*nop->phy.otg),
 			GFP_KERNEL);
 	if (!nop->phy.otg)
@@ -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);
 
@@ -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);
+			nop->viewport = devm_ioremap_resource(dev, res);
+			if (IS_ERR(nop->viewport)) {
+				dev_err(dev,"No IORESOURCE_MEM for ULPI Viewport");
+				return PTR_ERR(nop->viewport);
+			}
+		}
+
 	} else if (pdata) {
 		type = pdata->type;
 		clk_rate = pdata->clk_rate;
diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h
index 97eafc2..0f6f1fc 100644
--- a/drivers/usb/phy/phy-generic.h
+++ b/drivers/usb/phy/phy-generic.h
@@ -3,13 +3,16 @@
 
 struct usb_phy_gen_xceiv {
 	struct usb_phy phy;
+	struct usb_phy *ulpi;
 	struct device *dev;
 	struct clk *clk;
 	struct regulator *vcc;
 	int gpio_reset;
 	int gpio_chipselect;
+	int ulpi_vbus;
 	bool reset_active_low;
 	bool cs_active_low;
+	void __iomem *viewport;
 };
 
 int usb_gen_phy_init(struct usb_phy *phy);
-- 
1.7.10.4

Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Mark Rutland]

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-CR359r9tUDPXPF5Rlphj1Q@public.gmane.org>
> ---
>  .../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.

Thanks,
Mark.
--
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 2/3] usb: phy-ulpi: Add EXTVBUSIND,CHRGVBUS flag support

[Sergei Shtylyov]

On 12/02/2013 10:05 AM, Chris Ruehl wrote:

> usb: phy-ulpi: Add EXTVBUSIND,CHRGVBUS flag support

> ULPI like ISP1504 support external vbus power indication
> used in combination with vbus switches mic2075.

> Signed-off-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> ---
>   drivers/usb/phy/phy-ulpi.c |   11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)

> diff --git a/drivers/usb/phy/phy-ulpi.c b/drivers/usb/phy/phy-ulpi.c
> index 217339d..e2f15c4 100644
> --- a/drivers/usb/phy/phy-ulpi.c
> +++ b/drivers/usb/phy/phy-ulpi.c
> @@ -180,6 +180,8 @@ static int ulpi_init(struct usb_phy *phy)
>   	int i, vid, pid, ret;
>   	u32 ulpi_id = 0;
>
> +	pr_info("ULPI Viewport 0x%p\n",phy->io_priv);
> +

    You forgot space after comma.

WBR, Sergei

Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Chris Ruehl]

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?

Hi Mark,

If the parameters not set properly for this kind of board design
(ULPI + MIC) the 5V USB power is unstable and result errors while detecting and
running a usb-device.

>
>>
>> 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-CR359r9tUDPXPF5Rlphj1Q@public.gmane.org>
>> ---
>>   .../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.

I write this description because its takes me half a day to figure out that
the gpio definition must set <&gpioX num 1> to activate low rather then
<&gpioX num 0> which is active high! Only want to give people a hand. I don't
mind if you want to truncate it to:

reset-gpios :	GPIO used to reset ULPI
cs-gpios:	GPIO used to select a ULPI chipset

>
> 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?

We talk about board specific design, some designs need to set the VBUS flags 
others not, but its all run on the same generic kernel functions. FMHO this is
exactly what DT is designed for. The problem is how to encode the VBUS setup
a) easy to understand
b) easy to decode in the code

Drivers like phy-tegra-usb using there own hard coded ulpi setup, I think this
patch offers a way to work with the phy-generic.

>
> Also, s/_/-/ on property names please.

got it.

>
>> +		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

Sorry, but for the pinctrl (e.g fsl,imx27-pinctrl.txt) its done this way too, 
and for the code logic its becomes very simple. A solution with Boolean
ulpi-set-drvvbus
..
ulpi-set-chrgvbus
result in much more code in the kernel, by four times calling 
of_property_read_bool() vs. one time of_property_read_u32()
and the overhead the logic to encode it into a u32.

>
> Boolean properties are nicer for flags.
>
> What exactly do these flags mean?

The flags names related to the phy-ulpi.c used names ULPI_OTG_DRVVBUS ...
DRVVBUS 	enables the general logic for USB-VBUS in the ULPI
DRVVBUS_EXT	enables the external power out
DRVBUSIND	enables the logic for overcurrent protection and fault detection
CHRGVBUS 	enables the pump to stabilize the 5V output power level

>
> [...]
>
>> @@ -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?

Ups that's should be happen while I had a earlier release.. I missed it.

>
>>
>> @@ -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);
add spaces here
>> +		if (err) {
>> +			nop->ulpi_vbus = -1;
>> +			nop->viewport = NULL;

>> +			ulpi_vbus = 0;
remove this its unused

>> +		} else {
>> +			dev_dbg(dev,"ULPI ulpi_set_vbus 0x%02x",ulpi_vbus);
add spaces here
>> +			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

You right, we looking IORESOURCE_MEM for the ULPI-Viewport which I missed to 
mentioned.
The reg = <> for the current phy: phy@0 defines in the imx27.dtmi was unused.
1) I moved it to the AIPI2
2) merge them into the usb address range
to the the ULPI viewport available when needed.


> think we need a separate ulpi-phy binding that builds upon the generic
> one.
>
I agree with this.

> Thanks,
> Mark.

Thanks for the review.
Chris

> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3/3] usb: phy-generic: Add ULPI VBUS support

[Chris Ruehl]

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@gtsys.com.hk>
>> ---
>>   .../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@vger.kernel.org
> 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

Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Heikki Krogerus]

On Mon, Dec 02, 2013 at 03:05:19PM +0800, Chris Ruehl wrote:
> @@ -154,6 +164,27 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop,
>  {
>  	int err;
>  
> +	if (nop->ulpi_vbus > 0) {
> +		unsigned int flags = 0;
> +
> +		if (nop->ulpi_vbus & 0x1)
> +			flags |= ULPI_OTG_DRVVBUS;
> +		if (nop->ulpi_vbus & 0x2)
> +			flags |= ULPI_OTG_DRVVBUS_EXT;
> +		if (nop->ulpi_vbus & 0x4)
> +			flags |= ULPI_OTG_EXTVBUSIND;
> +		if (nop->ulpi_vbus & 0x8)
> +			flags |= ULPI_OTG_CHRGVBUS;
> +
> +		nop->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, flags);
> +		if (!nop->ulpi) {
> +			dev_err(dev, "Failed create ULPI Phy\n");
> +			return -ENOMEM;
> +		}
> +		dev_dbg(dev, "Create ULPI Phy\n");
> +		nop->ulpi->io_priv =  nop->viewport;
> +	}

This is so wrong. You are registering one kind of usb phy driver from
an other. Change drivers/usb/phy/ulpi.c to be a platform device. The
whole flag system in it is pretty horrid. While you are at it, change
that so it sets the values based on boolean flags from OF properties
or platform data.

NAK for the whole set.


-- 
heikki

Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Chris Ruehl]

On Tuesday, December 03, 2013 04:15 PM, Heikki Krogerus wrote:
> On Mon, Dec 02, 2013 at 03:05:19PM +0800, Chris Ruehl wrote:
>> @@ -154,6 +164,27 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop,
>>   {
>>   	int err;
>>
>> +	if (nop->ulpi_vbus>  0) {
>> +		unsigned int flags = 0;
>> +
>> +		if (nop->ulpi_vbus&  0x1)
>> +			flags |= ULPI_OTG_DRVVBUS;
>> +		if (nop->ulpi_vbus&  0x2)
>> +			flags |= ULPI_OTG_DRVVBUS_EXT;
>> +		if (nop->ulpi_vbus&  0x4)
>> +			flags |= ULPI_OTG_EXTVBUSIND;
>> +		if (nop->ulpi_vbus&  0x8)
>> +			flags |= ULPI_OTG_CHRGVBUS;
>> +
>> +		nop->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, flags);
>> +		if (!nop->ulpi) {
>> +			dev_err(dev, "Failed create ULPI Phy\n");
>> +			return -ENOMEM;
>> +		}
>> +		dev_dbg(dev, "Create ULPI Phy\n");
>> +		nop->ulpi->io_priv =  nop->viewport;
>> +	}
>
> This is so wrong. You are registering one kind of usb phy driver from
> an other. Change drivers/usb/phy/ulpi.c to be a platform device. The
> whole flag system in it is pretty horrid. While you are at it, change
> that so it sets the values based on boolean flags from OF properties
> or platform data.
>
> NAK for the whole set.
>
>

Heikki,

Thanks for your comments, even not much positive to me.. any how.
My intention on the "horrid" path was to reduce kernel code where
one of_read32 vs. four of_boolean. And mentioned logic is simple. But that's 
history.

On my way to find a solution for my board I'd look around and found using of
phy-ulpi.c functions in phy-tegra-usb.c and don't mind to use them too.

I accept your NAK and will work on a patch to make phy-ulpi.c working as 
platform device.

Last question to you. What you don't like on the patch to support chip-select 
gpio of my patch-set.. I ask because you NAK the whole set.
I really need the ChipSelect function to make my hardware work!

Chris

-- 
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 devicetree" 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 3/3] usb: phy-generic: Add ULPI VBUS support

[Heikki Krogerus]

Hi Chris,

On Wed, Dec 04, 2013 at 03:16:21PM +0800, Chris Ruehl wrote:
> On Tuesday, December 03, 2013 04:15 PM, Heikki Krogerus wrote:
> >On Mon, Dec 02, 2013 at 03:05:19PM +0800, Chris Ruehl wrote:
> >>@@ -154,6 +164,27 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop,
> >>  {
> >>  	int err;
> >>
> >>+	if (nop->ulpi_vbus>  0) {
> >>+		unsigned int flags = 0;
> >>+
> >>+		if (nop->ulpi_vbus&  0x1)
> >>+			flags |= ULPI_OTG_DRVVBUS;
> >>+		if (nop->ulpi_vbus&  0x2)
> >>+			flags |= ULPI_OTG_DRVVBUS_EXT;
> >>+		if (nop->ulpi_vbus&  0x4)
> >>+			flags |= ULPI_OTG_EXTVBUSIND;
> >>+		if (nop->ulpi_vbus&  0x8)
> >>+			flags |= ULPI_OTG_CHRGVBUS;
> >>+
> >>+		nop->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, flags);
> >>+		if (!nop->ulpi) {
> >>+			dev_err(dev, "Failed create ULPI Phy\n");
> >>+			return -ENOMEM;
> >>+		}
> >>+		dev_dbg(dev, "Create ULPI Phy\n");
> >>+		nop->ulpi->io_priv =  nop->viewport;
> >>+	}
> >
> >This is so wrong. You are registering one kind of usb phy driver from
> >an other. Change drivers/usb/phy/ulpi.c to be a platform device. The
> >whole flag system in it is pretty horrid. While you are at it, change
> >that so it sets the values based on boolean flags from OF properties
> >or platform data.
> >
> >NAK for the whole set.
> >
> >
> 
> Heikki,
> 
> Thanks for your comments, even not much positive to me.. any how.
> My intention on the "horrid" path was to reduce kernel code where
> one of_read32 vs. four of_boolean. And mentioned logic is simple.
> But that's history.

I should probable explain why I have problems with them. First of all,
things like driving the vbus should be a function that can be called
from upper layers. struct usb_otg has the set_vbus hook for that. You
can call it for example from your host controller's init routine. I'm
assuming you have a host controller since you are driving vbus.

You don't need to set the ULPI_OTG_CHRGVBUS. It's used for the VBUS
pulsing of SRP, which btw. is not anymore supported in OTG&EH2.0 spec,
so just don't use that bit even if you want to start SRP.

The only of_booleans you should need are for the DRV_VBUS_EXT and
USE_EXT_VBUS_IND. In my case I could not use even those. My controller
provides it's own control for them, so even if I set them to my ULPI
phy, the controller would simply override the values.

Secondly, why those silly flags in the first place. Those flags are
just bits in the registers. It would have been much easier and cleaner
to deliver a small struct with default values for the registers
instead.

> On my way to find a solution for my board I'd look around and found using of
> phy-ulpi.c functions in phy-tegra-usb.c and don't mind to use them too.

OK, IC. I have not followed what is happening with USB in linux for a
while.

The whole otg_ulpi_create() thing, and the flags with it, were
originally planned to be used from platform code. It's evil and it
should have never been accepted into upstream kernel. The time it was
introduced I was on vacation and nobody else seemed to care :(. All I
was able to do was to protest afterwards.

> I accept your NAK and will work on a patch to make phy-ulpi.c
> working as platform device.
> 
> Last question to you. What you don't like on the patch to support
> chip-select gpio of my patch-set.. I ask because you NAK the whole
> set.
> I really need the ChipSelect function to make my hardware work!

OK, I did not explain my problem with that patch. I'm sorry about
that. It also looks like I made wrong assumption with it. I thought
that your phy (is was ISP1504 right) is just like isp1704 that I have
worked with. On isp1704 you only have the chip_sel pin (no reset pin),
so I thought you can not have any reason to add handler for an other
gpio to this driver. After a quick look at isp1504 data sheet, it
looks like you have both reset and chip_sel pins on it, which I guess
are both connected to gpios on your platform.

So I don't have a problem with that. Though I'm not sure is this
driver the right place to handle things like these gpios, which are
pretty phy specific, in the first place. The phy-generic was
originally meant to be pure NOP phy driver.

One comment about how to handle the gpios. You should move to the new
gpio descriptor API. The legacy gpio API is now just a wrapper on top
of it.


> Chris
> 

Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3/3] usb: phy-generic: Add ULPI VBUS support

[Chris Ruehl]

On Wednesday, December 04, 2013 05:49 PM, Heikki Krogerus wrote:
> Hi Chris,
>
> On Wed, Dec 04, 2013 at 03:16:21PM +0800, Chris Ruehl wrote:
>> On Tuesday, December 03, 2013 04:15 PM, Heikki Krogerus wrote:
>>> On Mon, Dec 02, 2013 at 03:05:19PM +0800, Chris Ruehl wrote:
>>>> @@ -154,6 +164,27 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_gen_xceiv *nop,
>>>>   {
>>>>   	int err;
>>>>
>>>> +	if (nop->ulpi_vbus>   0) {
>>>> +		unsigned int flags = 0;
>>>> +
>>>> +		if (nop->ulpi_vbus&   0x1)
>>>> +			flags |= ULPI_OTG_DRVVBUS;
>>>> +		if (nop->ulpi_vbus&   0x2)
>>>> +			flags |= ULPI_OTG_DRVVBUS_EXT;
>>>> +		if (nop->ulpi_vbus&   0x4)
>>>> +			flags |= ULPI_OTG_EXTVBUSIND;
>>>> +		if (nop->ulpi_vbus&   0x8)
>>>> +			flags |= ULPI_OTG_CHRGVBUS;
>>>> +
>>>> +		nop->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, flags);
>>>> +		if (!nop->ulpi) {
>>>> +			dev_err(dev, "Failed create ULPI Phy\n");
>>>> +			return -ENOMEM;
>>>> +		}
>>>> +		dev_dbg(dev, "Create ULPI Phy\n");
>>>> +		nop->ulpi->io_priv =  nop->viewport;
>>>> +	}
>>>
>>> This is so wrong. You are registering one kind of usb phy driver from
>>> an other. Change drivers/usb/phy/ulpi.c to be a platform device. The
>>> whole flag system in it is pretty horrid. While you are at it, change
>>> that so it sets the values based on boolean flags from OF properties
>>> or platform data.
>>>
>>> NAK for the whole set.
>>>
>>>
>>
>> Heikki,
>>
>> Thanks for your comments, even not much positive to me.. any how.
>> My intention on the "horrid" path was to reduce kernel code where
>> one of_read32 vs. four of_boolean. And mentioned logic is simple.
>> But that's history.
>
> I should probable explain why I have problems with them. First of all,
> things like driving the vbus should be a function that can be called
> from upper layers. struct usb_otg has the set_vbus hook for that. You
> can call it for example from your host controller's init routine. I'm
> assuming you have a host controller since you are driving vbus.

My platform is Freescale imx27 and the host controller the ChipIdea, where I 
have already send some patches for. I uses the set_vbus it in the wrong place
nop->ulpi->otg->set_vbus(nop->ulpi->otg,true); (phy-generic:usb_gen_phy_init())

and now I start to understand where is the issue. I must tell chipidea to init 
the vbus using the platform....

>
> You don't need to set the ULPI_OTG_CHRGVBUS. It's used for the VBUS
> pulsing of SRP, which btw. is not anymore supported in OTG&EH2.0 spec,
> so just don't use that bit even if you want to start SRP.

OK, got it. Test it right away, yes my USB still works great even I omit the 
flag. The reason I introduced it was the fact that plat-mxc/isp1504xc.c of the 
2.6.22 with the freescale patches set this flag.

>
> The only of_booleans you should need are for the DRV_VBUS_EXT and
> USE_EXT_VBUS_IND. In my case I could not use even those. My controller
> provides it's own control for them, so even if I set them to my ULPI
> phy, the controller would simply override the values.
>
> Secondly, why those silly flags in the first place. Those flags are
> just bits in the registers. It would have been much easier and cleaner
> to deliver a small struct with default values for the registers
> instead.
>
>> On my way to find a solution for my board I'd look around and found using of
>> phy-ulpi.c functions in phy-tegra-usb.c and don't mind to use them too.
>
> OK, IC. I have not followed what is happening with USB in linux for a
> while.
>
> The whole otg_ulpi_create() thing, and the flags with it, were
> originally planned to be used from platform code. It's evil and it
> should have never been accepted into upstream kernel. The time it was
> introduced I was on vacation and nobody else seemed to care :(. All I
> was able to do was to protest afterwards.
>

Checked!


>> I accept your NAK and will work on a patch to make phy-ulpi.c
>> working as platform device.
>>
>> Last question to you. What you don't like on the patch to support
>> chip-select gpio of my patch-set.. I ask because you NAK the whole
>> set.
>> I really need the ChipSelect function to make my hardware work!
>
> OK, I did not explain my problem with that patch. I'm sorry about
> that. It also looks like I made wrong assumption with it. I thought
> that your phy (is was ISP1504 right) is just like isp1704 that I have
> worked with. On isp1704 you only have the chip_sel pin (no reset pin),
> so I thought you can not have any reason to add handler for an other
> gpio to this driver. After a quick look at isp1504 data sheet, it
> looks like you have both reset and chip_sel pins on it, which I guess
> are both connected to gpios on your platform.
>
Yes 1504, and my hardware guys make otg using the chipselect with gpio
and the usbh2 is fixed selected via pull down resistor.


> So I don't have a problem with that. Though I'm not sure is this
> driver the right place to handle things like these gpios, which are
> pretty phy specific, in the first place. The phy-generic was
> originally meant to be pure NOP phy driver.
May then change the meaning back to "generic" when support generic requirements
like chip-select(1704+1504) reset(1504).
If the 1504 missed a proper reset its ends up in weird errors ..

>
> One comment about how to handle the gpios. You should move to the new
> gpio descriptor API. The legacy gpio API is now just a wrapper on top
> of it.
>
Back to the Manuals.. :-) OK its on the list.


>
>> Chris
>>
>
> Thanks,
>

I thank you!
Chris

-- 
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-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 1/3] usb: phy-generic: Add GPIO based ChipSelect

[Felipe Balbi]

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

Hi,

On Mon, Dec 02, 2013 at 03:05:17PM +0800, Chris Ruehl wrote:
> @@ -231,27 +249,40 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	nop->reset_active_low = true;	/* default behaviour */
> +	nop->cs_active_low = true;
>  
>  	if (dev->of_node) {
>  		struct device_node *node = dev->of_node;
>  		enum of_gpio_flags flags;
> +		enum of_gpio_flags csflags;
>  
>  		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);
> +

two unrelated changes

>  		if (nop->gpio_reset == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
>  
>  		nop->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
>  
> +		nop->gpio_chipselect = of_get_named_gpio_flags(node, "cs-gpios",
> +								0, &csflags);
> +		if (gpio_is_valid(nop->gpio_chipselect))
> +			nop->cs_active_low = csflags & OF_GPIO_ACTIVE_LOW;
> +
>  	} else if (pdata) {
>  		type = pdata->type;
>  		clk_rate = pdata->clk_rate;
>  		needs_vcc = pdata->needs_vcc;
>  		nop->gpio_reset = pdata->gpio_reset;
> +		nop->gpio_chipselect = pdata->gpio_chipselect;
> +	} else {
> +		nop->gpio_reset = -1;

This line is already going upstream, please remove it, i'll handle the
conflict later.

-- 
balbi

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

Re: [PATCH 1/3] usb: phy-generic: Add GPIO based ChipSelect

[Chris Ruehl]

On Saturday, December 07, 2013 04:24 AM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Dec 02, 2013 at 03:05:17PM +0800, Chris Ruehl wrote:
>> @@ -231,27 +249,40 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
>>   		return -ENOMEM;
>>
>>   	nop->reset_active_low = true;	/* default behaviour */
>> +	nop->cs_active_low = true;
>>
>>   	if (dev->of_node) {
>>   		struct device_node *node = dev->of_node;
>>   		enum of_gpio_flags flags;
>> +		enum of_gpio_flags csflags;
>>
>>   		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);
>> +
>
> two unrelated changes
>
>>   		if (nop->gpio_reset == -EPROBE_DEFER)
>>   			return -EPROBE_DEFER;
>>
>>   		nop->reset_active_low = flags&  OF_GPIO_ACTIVE_LOW;
>>
>> +		nop->gpio_chipselect = of_get_named_gpio_flags(node, "cs-gpios",
>> +								0,&csflags);
>> +		if (gpio_is_valid(nop->gpio_chipselect))
>> +			nop->cs_active_low = csflags&  OF_GPIO_ACTIVE_LOW;
>> +
>>   	} else if (pdata) {
>>   		type = pdata->type;
>>   		clk_rate = pdata->clk_rate;
>>   		needs_vcc = pdata->needs_vcc;
>>   		nop->gpio_reset = pdata->gpio_reset;
>> +		nop->gpio_chipselect = pdata->gpio_chipselect;
>> +	} else {
>> +		nop->gpio_reset = -1;
>
> This line is already going upstream, please remove it, i'll handle the
> conflict later.
>

Beause the rest of the patch set is not ready to make it in the upstream, I will 
checkout latest linux-next and send the patch again as a single patch.

thanks
Chris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 1/3] usb: phy-generic: Add GPIO based ChipSelect

[Felipe Balbi]

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

Hi,

On Mon, Dec 09, 2013 at 09:45:30AM +0800, Chris Ruehl wrote:
> On Saturday, December 07, 2013 04:24 AM, Felipe Balbi wrote:
> >Hi,
> >
> >On Mon, Dec 02, 2013 at 03:05:17PM +0800, Chris Ruehl wrote:
> >>@@ -231,27 +249,40 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
> >>  		return -ENOMEM;
> >>
> >>  	nop->reset_active_low = true;	/* default behaviour */
> >>+	nop->cs_active_low = true;
> >>
> >>  	if (dev->of_node) {
> >>  		struct device_node *node = dev->of_node;
> >>  		enum of_gpio_flags flags;
> >>+		enum of_gpio_flags csflags;
> >>
> >>  		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);
> >>+
> >
> >two unrelated changes
> >
> >>  		if (nop->gpio_reset == -EPROBE_DEFER)
> >>  			return -EPROBE_DEFER;
> >>
> >>  		nop->reset_active_low = flags&  OF_GPIO_ACTIVE_LOW;
> >>
> >>+		nop->gpio_chipselect = of_get_named_gpio_flags(node, "cs-gpios",
> >>+								0,&csflags);
> >>+		if (gpio_is_valid(nop->gpio_chipselect))
> >>+			nop->cs_active_low = csflags&  OF_GPIO_ACTIVE_LOW;
> >>+
> >>  	} else if (pdata) {
> >>  		type = pdata->type;
> >>  		clk_rate = pdata->clk_rate;
> >>  		needs_vcc = pdata->needs_vcc;
> >>  		nop->gpio_reset = pdata->gpio_reset;
> >>+		nop->gpio_chipselect = pdata->gpio_chipselect;
> >>+	} else {
> >>+		nop->gpio_reset = -1;
> >
> >This line is already going upstream, please remove it, i'll handle the
> >conflict later.
> >
> 
> Beause the rest of the patch set is not ready to make it in the
> upstream, I will checkout latest linux-next and send the patch again
> as a single patch.

no, please *never* base any patches off of linux-next. That tree gets
recreated every day and can never be considered stable. Aim at using a
tag from Linus instead (v3.13-rc3, for example). It's a much better
development point than linux-next.

In case patch doesn't apply cleanly, different maintainers will have
their choice of rebasing it themselves or asking author to rebase on a
specific branch.

By default, however, use a tag from Linus.

cheers

-- 
balbi

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

Re: [PATCH 1/3] usb: phy-generic: Add GPIO based ChipSelect

[Chris Ruehl]

On Monday, December 09, 2013 12:07 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Dec 09, 2013 at 09:45:30AM +0800, Chris Ruehl wrote:
>> On Saturday, December 07, 2013 04:24 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Mon, Dec 02, 2013 at 03:05:17PM +0800, Chris Ruehl wrote:
>>>> @@ -231,27 +249,40 @@ static int usb_phy_gen_xceiv_probe(struct platform_device *pdev)
>>>>   		return -ENOMEM;
>>>>
>>>>   	nop->reset_active_low = true;	/* default behaviour */
>>>> +	nop->cs_active_low = true;
>>>>
>>>>   	if (dev->of_node) {
>>>>   		struct device_node *node = dev->of_node;
>>>>   		enum of_gpio_flags flags;
>>>> +		enum of_gpio_flags csflags;
>>>>
>>>>   		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);
>>>> +
>>> two unrelated changes
>>>
>>>>   		if (nop->gpio_reset == -EPROBE_DEFER)
>>>>   			return -EPROBE_DEFER;
>>>>
>>>>   		nop->reset_active_low = flags&  OF_GPIO_ACTIVE_LOW;
>>>>
>>>> +		nop->gpio_chipselect = of_get_named_gpio_flags(node, "cs-gpios",
>>>> +								0,&csflags);
>>>> +		if (gpio_is_valid(nop->gpio_chipselect))
>>>> +			nop->cs_active_low = csflags&  OF_GPIO_ACTIVE_LOW;
>>>> +
>>>>   	} else if (pdata) {
>>>>   		type = pdata->type;
>>>>   		clk_rate = pdata->clk_rate;
>>>>   		needs_vcc = pdata->needs_vcc;
>>>>   		nop->gpio_reset = pdata->gpio_reset;
>>>> +		nop->gpio_chipselect = pdata->gpio_chipselect;
>>>> +	} else {
>>>> +		nop->gpio_reset = -1;
>>> This line is already going upstream, please remove it, i'll handle the
>>> conflict later.
>>>
>> Beause the rest of the patch set is not ready to make it in the
>> upstream, I will checkout latest linux-next and send the patch again
>> as a single patch.
> no, please *never* base any patches off of linux-next. That tree gets
> recreated every day and can never be considered stable. Aim at using a
> tag from Linus instead (v3.13-rc3, for example). It's a much better
> development point than linux-next.
>
> In case patch doesn't apply cleanly, different maintainers will have
> their choice of rebasing it themselves or asking author to rebase on a
> specific branch.
>
> By default, however, use a tag from Linus.
>
> cheers
>

Thanks for the advice, I will follow :-)

Chris
--
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