| Message ID | 1391016574-25237-9-git-send-email-k.debski@samsung.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 29 Jan 2014, Kamil Debski wrote: > Change the phy provider used from the old one using the USB phy > framework to a new one using the Generic phy framework. > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > --- > .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ > drivers/usb/host/ehci-exynos.c | 97 +++++++++++++------- > 2 files changed, 76 insertions(+), 34 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt > index d967ba1..25e199a 100644 > --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt > +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt > @@ -12,6 +12,10 @@ Required properties: > - interrupts: interrupt number to the cpu. > - clocks: from common clock binding: handle to usb clock. > - clock-names: from common clock binding: Shall be "usbhost". > + - port: if in the SoC there are EHCI phys, they should be listed here. > +One phy per port. Each port should have its reg entry with a consecutive > +number. Also it should contain phys and phy-names entries specifying the > +phy used by the port. What is the reg entry number used for? As far as I can see, it isn't used for anything. In which case, why have it at all? > @@ -42,10 +42,10 @@ > static const char hcd_name[] = "ehci-exynos"; > static struct hc_driver __read_mostly exynos_ehci_hc_driver; > > +#define PHY_NUMBER 3 > struct exynos_ehci_hcd { > struct clk *clk; > - struct usb_phy *phy; > - struct usb_otg *otg; You have removed all the OTG stuff from the driver. This wasn't mentioned in the patch description, and it has no connection with the PHY work. > + struct phy *phy[PHY_NUMBER]; > }; > > #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv) > @@ -69,13 +69,43 @@ static void exynos_setup_vbus_gpio(struct platform_device *pdev) > dev_err(dev, "can't request ehci vbus gpio %d", gpio); > } > > +static int exynos_phys_on(struct phy *p[]) > +{ > + int i; > + int ret = 0; > + > + for (i = 0; ret == 0 && i < PHY_NUMBER; i++) > + if (p[i]) > + ret = phy_power_on(p[i]); > + if (ret) > + for (i--; i > 0; i--) > + if (p[i]) > + phy_power_off(p[i]); This loop runs while i > 0. Therefore you will never turn off the power to p[0]. > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct platform_device *pdev) > "samsung,exynos5440-ehci")) > goto skip_phy; > > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); > - if (IS_ERR(phy)) { > - usb_put_hcd(hcd); You omitted this line from the new error returns below. > - dev_warn(&pdev->dev, "no platform data or transceiver defined\n"); > - return -EPROBE_DEFER; > - } else { > - exynos_ehci->phy = phy; > - exynos_ehci->otg = phy->otg; > + for_each_available_child_of_node(pdev->dev.of_node, child) { > + err = of_property_read_u32(child, "reg", &phy_number); > + if (err) { > + dev_err(&pdev->dev, "Failed to parse device tree\n"); > + of_node_put(child); > + return err; Here, for example. Wouldn't it be better to goto fail_clk? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jan 29, 2014 at 9:29 AM, Kamil Debski <k.debski@samsung.com> wrote: > Change the phy provider used from the old one using the USB phy > framework to a new one using the Generic phy framework. > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > --- > .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ > drivers/usb/host/ehci-exynos.c | 97 +++++++++++++------- > 2 files changed, 76 insertions(+), 34 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt > index d967ba1..25e199a 100644 > --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt > +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt > @@ -12,6 +12,10 @@ Required properties: > - interrupts: interrupt number to the cpu. > - clocks: from common clock binding: handle to usb clock. > - clock-names: from common clock binding: Shall be "usbhost". > + - port: if in the SoC there are EHCI phys, they should be listed here. > +One phy per port. Each port should have its reg entry with a consecutive > +number. Also it should contain phys and phy-names entries specifying the > +phy used by the port. > > Optional properties: > - samsung,vbus-gpio: if present, specifies the GPIO that > @@ -27,6 +31,15 @@ Example: > > clocks = <&clock 285>; > clock-names = "usbhost"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + phys = <&usb2phy 1>; > + phy-names = "host"; > + status = "disabled"; > + }; > }; > > OHCI [...] > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct platform_device *pdev) > "samsung,exynos5440-ehci")) > goto skip_phy; > > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); > - if (IS_ERR(phy)) { > - usb_put_hcd(hcd); > - dev_warn(&pdev->dev, "no platform data or transceiver defined\n"); > - return -EPROBE_DEFER; > - } else { > - exynos_ehci->phy = phy; > - exynos_ehci->otg = phy->otg; > + for_each_available_child_of_node(pdev->dev.of_node, child) { > + err = of_property_read_u32(child, "reg", &phy_number); > + if (err) { > + dev_err(&pdev->dev, "Failed to parse device tree\n"); > + of_node_put(child); > + return err; > + } > + if (phy_number >= PHY_NUMBER) { > + dev_err(&pdev->dev, "Failed to parse device tree - number out of range\n"); > + of_node_put(child); > + return -EINVAL; > + } > + phy = devm_of_phy_get(&pdev->dev, child, 0); > + of_node_put(child); > + if (IS_ERR(phy)) { > + dev_err(&pdev->dev, "Failed to get phy number %d", > + phy_number); > + return PTR_ERR(phy); > + } > + exynos_ehci->phy[phy_number] = phy; this looks like it is now breaking older device trees, where ports might not be described. Since device tree interfaces need to be backwards compatible, you still need to handle the old case of not having ports described. There are two ways of doing this: 1. Fall back to the old behavior if there are no ports 2. Use a new compatible value for the new model with port subnodes, and if the old compatible value is used, then fall back to the old behavior. I'm guessing (1) might be easiest since you can check for the presence of #address-cells to tell if this is just an old style node, or if it's a new-style node without any ports below it. -Olof -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, On 29.01.2014 21:42, Alan Stern wrote: > On Wed, 29 Jan 2014, Kamil Debski wrote: > >> Change the phy provider used from the old one using the USB phy >> framework to a new one using the Generic phy framework. >> >> Signed-off-by: Kamil Debski <k.debski@samsung.com> >> --- >> .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ >> drivers/usb/host/ehci-exynos.c | 97 +++++++++++++------- >> 2 files changed, 76 insertions(+), 34 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt >> index d967ba1..25e199a 100644 >> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt >> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt >> @@ -12,6 +12,10 @@ Required properties: >> - interrupts: interrupt number to the cpu. >> - clocks: from common clock binding: handle to usb clock. >> - clock-names: from common clock binding: Shall be "usbhost". >> + - port: if in the SoC there are EHCI phys, they should be listed here. >> +One phy per port. Each port should have its reg entry with a consecutive >> +number. Also it should contain phys and phy-names entries specifying the >> +phy used by the port. > > What is the reg entry number used for? As far as I can see, it isn't > used for anything. In which case, why have it at all? The reg property is here to identify which EHCI port the node is describing. This should be mentioned in the documentation, though, as well as the whole description of port nodes should be written in a more structured manner, just as other properties. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, Thank you for your review. > From: Alan Stern [mailto:stern@rowland.harvard.edu] > Sent: Wednesday, January 29, 2014 9:43 PM > > On Wed, 29 Jan 2014, Kamil Debski wrote: > > > Change the phy provider used from the old one using the USB phy > > framework to a new one using the Generic phy framework. > > > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > > --- > > .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ > > drivers/usb/host/ehci-exynos.c | 97 > +++++++++++++------- > > 2 files changed, 76 insertions(+), 34 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt > > b/Documentation/devicetree/bindings/usb/exynos-usb.txt > > index d967ba1..25e199a 100644 > > --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt > > +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt > > @@ -12,6 +12,10 @@ Required properties: > > - interrupts: interrupt number to the cpu. > > - clocks: from common clock binding: handle to usb clock. > > - clock-names: from common clock binding: Shall be "usbhost". > > + - port: if in the SoC there are EHCI phys, they should be listed > here. > > +One phy per port. Each port should have its reg entry with a > > +consecutive number. Also it should contain phys and phy-names > entries > > +specifying the phy used by the port. > > What is the reg entry number used for? As far as I can see, it isn't > used for anything. In which case, why have it at all? Tomasz Figa already commented this. I agree with him, that this should be better described in the documentation. > > > @@ -42,10 +42,10 @@ > > static const char hcd_name[] = "ehci-exynos"; static struct > > hc_driver __read_mostly exynos_ehci_hc_driver; > > > > +#define PHY_NUMBER 3 > > struct exynos_ehci_hcd { > > struct clk *clk; > > - struct usb_phy *phy; > > - struct usb_otg *otg; > > You have removed all the OTG stuff from the driver. This wasn't > mentioned in the patch description, and it has no connection with the > PHY work. Maybe I'll explain more about what are we trying to achieve. The goal is to replace the old phy driver with the new one. In the old driver it was difficult to add support to new SoC. It also had issues with having device and host working together. You're right that until the old phy driver is removed support for the it should remain. To be able to remove the old driver both new USB2 and new USB3 phy drivers have to be ready. The USB3 driver is written by Vivek Gautam and as I see it, he is also close to completion. Regarding the otg part. The old phy driver is the only provider of the otg structure. It sets the host field of the structure. It is then used by samsung_usb2phy_init (drivers/usb/phy/phy-samsung-usb2.c) to check which driver is requesting the phy (is it host or device). In the new driver this is determined by the entry in device tree. So no need to check the otg struct and strstr (!) to check if dev_name is ehci, ohci or other, like the old driver. > > + struct phy *phy[PHY_NUMBER]; > > }; > > > > #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd > > *)(hcd_to_ehci(hcd)->priv) @@ -69,13 +69,43 @@ static void > exynos_setup_vbus_gpio(struct platform_device *pdev) > > dev_err(dev, "can't request ehci vbus gpio %d", gpio); } > > > > +static int exynos_phys_on(struct phy *p[]) { > > + int i; > > + int ret = 0; > > + > > + for (i = 0; ret == 0 && i < PHY_NUMBER; i++) > > + if (p[i]) > > + ret = phy_power_on(p[i]); > > + if (ret) > > + for (i--; i > 0; i--) > > + if (p[i]) > > + phy_power_off(p[i]); > > This loop runs while i > 0. Therefore you will never turn off the > power to p[0]. Ups, my bad. Thank you for spotting this. > > > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct > platform_device *pdev) > > "samsung,exynos5440-ehci")) > > goto skip_phy; > > > > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); > > - if (IS_ERR(phy)) { > > - usb_put_hcd(hcd); > > You omitted this line from the new error returns below. I see. > > > - dev_warn(&pdev->dev, "no platform data or transceiver > defined\n"); > > - return -EPROBE_DEFER; > > - } else { > > - exynos_ehci->phy = phy; > > - exynos_ehci->otg = phy->otg; > > + for_each_available_child_of_node(pdev->dev.of_node, child) { > > + err = of_property_read_u32(child, "reg", &phy_number); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to parse device tree\n"); > > + of_node_put(child); > > + return err; > > Here, for example. Wouldn't it be better to goto fail_clk? Right, I will fix this. Thank you. Best wishes,
Hi Olof, Thank you for your review. > From: Olof Johansson [mailto:olof@lixom.net] > Sent: Wednesday, January 29, 2014 9:55 PM > > Hi, > > On Wed, Jan 29, 2014 at 9:29 AM, Kamil Debski <k.debski@samsung.com> > wrote: > > Change the phy provider used from the old one using the USB phy > > framework to a new one using the Generic phy framework. > > > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > > --- > > .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ > > drivers/usb/host/ehci-exynos.c | 97 > +++++++++++++------- > > 2 files changed, 76 insertions(+), 34 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt > > b/Documentation/devicetree/bindings/usb/exynos-usb.txt > > index d967ba1..25e199a 100644 > > --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt > > +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt > > @@ -12,6 +12,10 @@ Required properties: > > - interrupts: interrupt number to the cpu. > > - clocks: from common clock binding: handle to usb clock. > > - clock-names: from common clock binding: Shall be "usbhost". > > + - port: if in the SoC there are EHCI phys, they should be listed > here. > > +One phy per port. Each port should have its reg entry with a > > +consecutive number. Also it should contain phys and phy-names > entries > > +specifying the phy used by the port. > > > > Optional properties: > > - samsung,vbus-gpio: if present, specifies the GPIO that @@ -27,6 > > +31,15 @@ Example: > > > > clocks = <&clock 285>; > > clock-names = "usbhost"; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + port@0 { > > + reg = <0>; > > + phys = <&usb2phy 1>; > > + phy-names = "host"; > > + status = "disabled"; > > + }; > > }; > > > > OHCI > > [...] > > > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct > platform_device *pdev) > > "samsung,exynos5440-ehci")) > > goto skip_phy; > > > > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); > > - if (IS_ERR(phy)) { > > - usb_put_hcd(hcd); > > - dev_warn(&pdev->dev, "no platform data or transceiver > defined\n"); > > - return -EPROBE_DEFER; > > - } else { > > - exynos_ehci->phy = phy; > > - exynos_ehci->otg = phy->otg; > > + for_each_available_child_of_node(pdev->dev.of_node, child) { > > + err = of_property_read_u32(child, "reg", > &phy_number); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to parse device > tree\n"); > > + of_node_put(child); > > + return err; > > + } > > + if (phy_number >= PHY_NUMBER) { > > + dev_err(&pdev->dev, "Failed to parse device > tree - number out of range\n"); > > + of_node_put(child); > > + return -EINVAL; > > + } > > + phy = devm_of_phy_get(&pdev->dev, child, 0); > > + of_node_put(child); > > + if (IS_ERR(phy)) { > > + dev_err(&pdev->dev, "Failed to get phy number > %d", > > + > phy_number); > > + return PTR_ERR(phy); > > + } > > + exynos_ehci->phy[phy_number] = phy; > > this looks like it is now breaking older device trees, where ports > might not be described. Since device tree interfaces need to be > backwards compatible, you still need to handle the old case of not > having ports described. > > There are two ways of doing this: > > 1. Fall back to the old behavior if there are no ports 2. Use a new > compatible value for the new model with port subnodes, and if the old > compatible value is used, then fall back to the old behavior. > > I'm guessing (1) might be easiest since you can check for the presence > of #address-cells to tell if this is just an old style node, or if it's > a new-style node without any ports below it. The ultimate goal is to remove the old phy driver. Unfortunately this has to be synced with the new USB3 phy driver by Vivek Gautam. I think he is also close to completion. What about this case? In the end the old driver will be removed and no longer be supported. Having backward compatibility in mind, it is possible to have the old and the new phy driver together in one kernel release. But do we want to have two drivers doing the same thing at the same time? Best wishes,
On Wed, Feb 5, 2014 at 7:57 AM, Kamil Debski <k.debski@samsung.com> wrote: > Hi Olof, > > Thank you for your review. > >> From: Olof Johansson [mailto:olof@lixom.net] >> Sent: Wednesday, January 29, 2014 9:55 PM >> >> Hi, >> >> On Wed, Jan 29, 2014 at 9:29 AM, Kamil Debski <k.debski@samsung.com> >> wrote: >> > Change the phy provider used from the old one using the USB phy >> > framework to a new one using the Generic phy framework. >> > >> > Signed-off-by: Kamil Debski <k.debski@samsung.com> >> > --- >> > .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ >> > drivers/usb/host/ehci-exynos.c | 97 >> +++++++++++++------- >> > 2 files changed, 76 insertions(+), 34 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt >> > b/Documentation/devicetree/bindings/usb/exynos-usb.txt >> > index d967ba1..25e199a 100644 >> > --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt >> > +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt >> > @@ -12,6 +12,10 @@ Required properties: >> > - interrupts: interrupt number to the cpu. >> > - clocks: from common clock binding: handle to usb clock. >> > - clock-names: from common clock binding: Shall be "usbhost". >> > + - port: if in the SoC there are EHCI phys, they should be listed >> here. >> > +One phy per port. Each port should have its reg entry with a >> > +consecutive number. Also it should contain phys and phy-names >> entries >> > +specifying the phy used by the port. >> > >> > Optional properties: >> > - samsung,vbus-gpio: if present, specifies the GPIO that @@ -27,6 >> > +31,15 @@ Example: >> > >> > clocks = <&clock 285>; >> > clock-names = "usbhost"; >> > + >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + port@0 { >> > + reg = <0>; >> > + phys = <&usb2phy 1>; >> > + phy-names = "host"; >> > + status = "disabled"; >> > + }; >> > }; >> > >> > OHCI >> >> [...] >> >> > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct >> platform_device *pdev) >> > "samsung,exynos5440-ehci")) >> > goto skip_phy; >> > >> > - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); >> > - if (IS_ERR(phy)) { >> > - usb_put_hcd(hcd); >> > - dev_warn(&pdev->dev, "no platform data or transceiver >> defined\n"); >> > - return -EPROBE_DEFER; >> > - } else { >> > - exynos_ehci->phy = phy; >> > - exynos_ehci->otg = phy->otg; >> > + for_each_available_child_of_node(pdev->dev.of_node, child) { >> > + err = of_property_read_u32(child, "reg", >> &phy_number); >> > + if (err) { >> > + dev_err(&pdev->dev, "Failed to parse device >> tree\n"); >> > + of_node_put(child); >> > + return err; >> > + } >> > + if (phy_number >= PHY_NUMBER) { >> > + dev_err(&pdev->dev, "Failed to parse device >> tree - number out of range\n"); >> > + of_node_put(child); >> > + return -EINVAL; >> > + } >> > + phy = devm_of_phy_get(&pdev->dev, child, 0); >> > + of_node_put(child); >> > + if (IS_ERR(phy)) { >> > + dev_err(&pdev->dev, "Failed to get phy number >> %d", >> > + >> phy_number); >> > + return PTR_ERR(phy); >> > + } >> > + exynos_ehci->phy[phy_number] = phy; >> >> this looks like it is now breaking older device trees, where ports >> might not be described. Since device tree interfaces need to be >> backwards compatible, you still need to handle the old case of not >> having ports described. >> >> There are two ways of doing this: >> >> 1. Fall back to the old behavior if there are no ports 2. Use a new >> compatible value for the new model with port subnodes, and if the old >> compatible value is used, then fall back to the old behavior. >> >> I'm guessing (1) might be easiest since you can check for the presence >> of #address-cells to tell if this is just an old style node, or if it's >> a new-style node without any ports below it. > > The ultimate goal is to remove the old phy driver. Unfortunately > this has to be synced with the new USB3 phy driver by Vivek Gautam. I think > he > is also close to completion. What about this case? In the end the old driver > will be removed and no longer be supported. Having backward compatibility in > mind, it is possible to have the old and the new phy driver together in one > kernel release. But do we want to have two drivers doing the same thing at > the same time? It is mostly irrelevant if there is a new driver or not -- the old device tree has to keep working. In this case it would mean that the new driver needs to work with older device trees as well, or people will see functionality regressing. The device tree is a description of the hardware, not an extension of the driver. -Olof -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 5 Feb 2014, Kamil Debski wrote: > Hi Alan, > > Thank you for your review. You're welcome. > > > > > Change the phy provider used from the old one using the USB phy > > You have removed all the OTG stuff from the driver. This wasn't > > mentioned in the patch description, and it has no connection with the > > PHY work. > > Maybe I'll explain more about what are we trying to achieve. The goal > is to replace the old phy driver with the new one. In the old driver it was > difficult to add support to new SoC. It also had issues with having device > and > host working together. > > You're right that until the old phy driver is removed support for the it > should > remain. To be able to remove the old driver both new USB2 and new USB3 phy > drivers > have to be ready. The USB3 driver is written by Vivek Gautam and as I see > it, he > is also close to completion. > > Regarding the otg part. The old phy driver is the only provider of the otg > structure. It sets the host field of the structure. It is then used by > samsung_usb2phy_init (drivers/usb/phy/phy-samsung-usb2.c) to check which > driver is requesting the phy (is it host or device). In the new driver this > is determined by the entry in device tree. So no need to check the otg > struct > and strstr (!) to check if dev_name is ehci, ohci or other, like the old > driver. Okay, that's fine. But please explain this in the patch description next time. Otherwise the connection between the phy driver and the otg structure is not at all clear; they look like two unrelated things. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.02.2014 18:30, Olof Johansson wrote: > On Wed, Feb 5, 2014 at 7:57 AM, Kamil Debski <k.debski@samsung.com> wrote: >> Hi Olof, >> >> Thank you for your review. >> >>> From: Olof Johansson [mailto:olof@lixom.net] >>> Sent: Wednesday, January 29, 2014 9:55 PM >>> >>> Hi, >>> >>> On Wed, Jan 29, 2014 at 9:29 AM, Kamil Debski <k.debski@samsung.com> >>> wrote: >>>> Change the phy provider used from the old one using the USB phy >>>> framework to a new one using the Generic phy framework. >>>> >>>> Signed-off-by: Kamil Debski <k.debski@samsung.com> >>>> --- >>>> .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ >>>> drivers/usb/host/ehci-exynos.c | 97 >>> +++++++++++++------- >>>> 2 files changed, 76 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt >>>> b/Documentation/devicetree/bindings/usb/exynos-usb.txt >>>> index d967ba1..25e199a 100644 >>>> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt >>>> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt >>>> @@ -12,6 +12,10 @@ Required properties: >>>> - interrupts: interrupt number to the cpu. >>>> - clocks: from common clock binding: handle to usb clock. >>>> - clock-names: from common clock binding: Shall be "usbhost". >>>> + - port: if in the SoC there are EHCI phys, they should be listed >>> here. >>>> +One phy per port. Each port should have its reg entry with a >>>> +consecutive number. Also it should contain phys and phy-names >>> entries >>>> +specifying the phy used by the port. >>>> >>>> Optional properties: >>>> - samsung,vbus-gpio: if present, specifies the GPIO that @@ -27,6 >>>> +31,15 @@ Example: >>>> >>>> clocks = <&clock 285>; >>>> clock-names = "usbhost"; >>>> + >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + port@0 { >>>> + reg = <0>; >>>> + phys = <&usb2phy 1>; >>>> + phy-names = "host"; >>>> + status = "disabled"; >>>> + }; >>>> }; >>>> >>>> OHCI >>> >>> [...] >>> >>>> @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct >>> platform_device *pdev) >>>> "samsung,exynos5440-ehci")) >>>> goto skip_phy; >>>> >>>> - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); >>>> - if (IS_ERR(phy)) { >>>> - usb_put_hcd(hcd); >>>> - dev_warn(&pdev->dev, "no platform data or transceiver >>> defined\n"); >>>> - return -EPROBE_DEFER; >>>> - } else { >>>> - exynos_ehci->phy = phy; >>>> - exynos_ehci->otg = phy->otg; >>>> + for_each_available_child_of_node(pdev->dev.of_node, child) { >>>> + err = of_property_read_u32(child, "reg", >>> &phy_number); >>>> + if (err) { >>>> + dev_err(&pdev->dev, "Failed to parse device >>> tree\n"); >>>> + of_node_put(child); >>>> + return err; >>>> + } >>>> + if (phy_number >= PHY_NUMBER) { >>>> + dev_err(&pdev->dev, "Failed to parse device >>> tree - number out of range\n"); >>>> + of_node_put(child); >>>> + return -EINVAL; >>>> + } >>>> + phy = devm_of_phy_get(&pdev->dev, child, 0); >>>> + of_node_put(child); >>>> + if (IS_ERR(phy)) { >>>> + dev_err(&pdev->dev, "Failed to get phy number >>> %d", >>>> + >>> phy_number); >>>> + return PTR_ERR(phy); >>>> + } >>>> + exynos_ehci->phy[phy_number] = phy; >>> >>> this looks like it is now breaking older device trees, where ports >>> might not be described. Since device tree interfaces need to be >>> backwards compatible, you still need to handle the old case of not >>> having ports described. >>> >>> There are two ways of doing this: >>> >>> 1. Fall back to the old behavior if there are no ports 2. Use a new >>> compatible value for the new model with port subnodes, and if the old >>> compatible value is used, then fall back to the old behavior. >>> >>> I'm guessing (1) might be easiest since you can check for the presence >>> of #address-cells to tell if this is just an old style node, or if it's >>> a new-style node without any ports below it. >> >> The ultimate goal is to remove the old phy driver. Unfortunately >> this has to be synced with the new USB3 phy driver by Vivek Gautam. I think >> he >> is also close to completion. What about this case? In the end the old driver >> will be removed and no longer be supported. Having backward compatibility in >> mind, it is possible to have the old and the new phy driver together in one >> kernel release. But do we want to have two drivers doing the same thing at >> the same time? > > It is mostly irrelevant if there is a new driver or not -- the old > device tree has to keep working. In this case it would mean that the > new driver needs to work with older device trees as well, or people > will see functionality regressing. > > The device tree is a description of the hardware, not an extension of > the driver. The problem with this case is that when the original driver was added there was no way to bind PHY providers and consumers together. Basically there was no generic PHY subsystem. Instead the hacky USB PHY subsystem was used with hardcoded PHY IDs (types) in both PHY driver and user drivers. In DT you only had nodes for the PHY controller and consumer IPs (e.g. EHCI, HSOTG), but no relation between them. This was broken, merged without proper review and I don't think we should be supporting this, along with a lot of completely messed up bindings we were supposed to fix after a lot of discussion that happened last year, but I haven't seen much movement in this direction, instead ending with such FUBDs being stable and a need to maintain support for them... Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt index d967ba1..25e199a 100644 --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt @@ -12,6 +12,10 @@ Required properties: - interrupts: interrupt number to the cpu. - clocks: from common clock binding: handle to usb clock. - clock-names: from common clock binding: Shall be "usbhost". + - port: if in the SoC there are EHCI phys, they should be listed here. +One phy per port. Each port should have its reg entry with a consecutive +number. Also it should contain phys and phy-names entries specifying the +phy used by the port. Optional properties: - samsung,vbus-gpio: if present, specifies the GPIO that @@ -27,6 +31,15 @@ Example: clocks = <&clock 285>; clock-names = "usbhost"; + + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + phys = <&usb2phy 1>; + phy-names = "host"; + status = "disabled"; + }; }; OHCI diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index d1d8c47..7c35501 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -19,12 +19,12 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_gpio.h> +#include <linux/phy/phy.h> #include <linux/platform_device.h> #include <linux/usb/phy.h> #include <linux/usb/samsung_usb_phy.h> #include <linux/usb.h> #include <linux/usb/hcd.h> -#include <linux/usb/otg.h> #include "ehci.h" @@ -42,10 +42,10 @@ static const char hcd_name[] = "ehci-exynos"; static struct hc_driver __read_mostly exynos_ehci_hc_driver; +#define PHY_NUMBER 3 struct exynos_ehci_hcd { struct clk *clk; - struct usb_phy *phy; - struct usb_otg *otg; + struct phy *phy[PHY_NUMBER]; }; #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd *)(hcd_to_ehci(hcd)->priv) @@ -69,13 +69,43 @@ static void exynos_setup_vbus_gpio(struct platform_device *pdev) dev_err(dev, "can't request ehci vbus gpio %d", gpio); } +static int exynos_phys_on(struct phy *p[]) +{ + int i; + int ret = 0; + + for (i = 0; ret == 0 && i < PHY_NUMBER; i++) + if (p[i]) + ret = phy_power_on(p[i]); + if (ret) + for (i--; i > 0; i--) + if (p[i]) + phy_power_off(p[i]); + + return ret; +} + +static int exynos_phys_off(struct phy *p[]) +{ + int i; + int ret = 0; + + for (i = 0; ret == 0 && i < PHY_NUMBER; i++) + if (p[i]) + ret = phy_power_off(p[i]); + + return ret; +} + static int exynos_ehci_probe(struct platform_device *pdev) { struct exynos_ehci_hcd *exynos_ehci; struct usb_hcd *hcd; struct ehci_hcd *ehci; struct resource *res; - struct usb_phy *phy; + struct phy *phy; + struct device_node *child; + int phy_number; int irq; int err; @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct platform_device *pdev) "samsung,exynos5440-ehci")) goto skip_phy; - phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2); - if (IS_ERR(phy)) { - usb_put_hcd(hcd); - dev_warn(&pdev->dev, "no platform data or transceiver defined\n"); - return -EPROBE_DEFER; - } else { - exynos_ehci->phy = phy; - exynos_ehci->otg = phy->otg; + for_each_available_child_of_node(pdev->dev.of_node, child) { + err = of_property_read_u32(child, "reg", &phy_number); + if (err) { + dev_err(&pdev->dev, "Failed to parse device tree\n"); + of_node_put(child); + return err; + } + if (phy_number >= PHY_NUMBER) { + dev_err(&pdev->dev, "Failed to parse device tree - number out of range\n"); + of_node_put(child); + return -EINVAL; + } + phy = devm_of_phy_get(&pdev->dev, child, 0); + of_node_put(child); + if (IS_ERR(phy)) { + dev_err(&pdev->dev, "Failed to get phy number %d", + phy_number); + return PTR_ERR(phy); + } + exynos_ehci->phy[phy_number] = phy; } skip_phy: @@ -149,11 +191,11 @@ skip_phy: goto fail_io; } - if (exynos_ehci->otg) - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); - - if (exynos_ehci->phy) - usb_phy_init(exynos_ehci->phy); + err = exynos_phys_on(exynos_ehci->phy); + if (err) { + dev_err(&pdev->dev, "Failed to enabled phys\n"); + goto fail_io; + } ehci = hcd_to_ehci(hcd); ehci->caps = hcd->regs; @@ -173,8 +215,7 @@ skip_phy: return 0; fail_add_hcd: - if (exynos_ehci->phy) - usb_phy_shutdown(exynos_ehci->phy); + exynos_phys_off(exynos_ehci->phy); fail_io: clk_disable_unprepare(exynos_ehci->clk); fail_clk: @@ -189,11 +230,7 @@ static int exynos_ehci_remove(struct platform_device *pdev) usb_remove_hcd(hcd); - if (exynos_ehci->otg) - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); - - if (exynos_ehci->phy) - usb_phy_shutdown(exynos_ehci->phy); + exynos_phys_off(exynos_ehci->phy); clk_disable_unprepare(exynos_ehci->clk); @@ -213,11 +250,7 @@ static int exynos_ehci_suspend(struct device *dev) rc = ehci_suspend(hcd, do_wakeup); - if (exynos_ehci->otg) - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); - - if (exynos_ehci->phy) - usb_phy_shutdown(exynos_ehci->phy); + exynos_phys_off(exynos_ehci->phy); clk_disable_unprepare(exynos_ehci->clk); @@ -231,11 +264,7 @@ static int exynos_ehci_resume(struct device *dev) clk_prepare_enable(exynos_ehci->clk); - if (exynos_ehci->otg) - exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); - - if (exynos_ehci->phy) - usb_phy_init(exynos_ehci->phy); + exynos_phys_on(exynos_ehci->phy); /* DMA burst Enable */ writel(EHCI_INSNREG00_ENABLE_DMA_BURST, EHCI_INSNREG00(hcd->regs));
Change the phy provider used from the old one using the USB phy framework to a new one using the Generic phy framework. Signed-off-by: Kamil Debski <k.debski@samsung.com> --- .../devicetree/bindings/usb/exynos-usb.txt | 13 +++ drivers/usb/host/ehci-exynos.c | 97 +++++++++++++------- 2 files changed, 76 insertions(+), 34 deletions(-)