Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support
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 OTGEH2.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 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 3/3] usb: phy-generic: Add ULPI VBUS support
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 OTGEH2.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.
Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support
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 -- 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 3/3] usb: phy-generic: Add ULPI VBUS support
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 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 3/3] usb: phy-generic: Add ULPI VBUS support
On Mon, Dec 02, 2013 at 07:05:19AM +, 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.ru...@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 = (11) EXTVBUSIND = (12) CHRGVBUS = (13) + 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support
On Tuesday, December 03, 2013 02:22 AM, Mark Rutland wrote: On Mon, Dec 02, 2013 at 07:05:19AM +, 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_usbphy0pinctrl_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 Ruehlchris.ru...@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 = (11) EXTVBUSIND = (12) CHRGVBUS = (13) + 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