| Message ID | 1388963080-12544-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 06 January 2014, Hans de Goede wrote: > +Required properties: > + - compatible: Should be "platform-ohci" > + - reg: Address range of the ohci registers. > + - interrupts: Should contain the ohci interrupt. > + > +Optional properties: > + - clocks: array of clocks > + - clock-names: clock names "ahb" and/or "ohci" > + - phys: phy > + - phy-names: "usb_phy" Maybe just "usb"? It obviously is a phy, so that part of the name is a bit redundant. Actually, the "usb" part of the name also seems redundant. Is it possible to make it an anonymous phy reference without a phy-names property as we often do for clocks? > +static int ohci_platform_power_on(struct platform_device *dev) > +{ > + struct usb_hcd *hcd = platform_get_drvdata(dev); > + struct ohci_platform_priv *priv = > + (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv; > + int ret; > + > + if (!IS_ERR(priv->ohci_clk)) { > + ret = clk_prepare_enable(priv->ohci_clk); > + if (ret) > + return ret; > + } > + > + if (!IS_ERR(priv->ahb_clk)) { > + ret = clk_prepare_enable(priv->ahb_clk); > + if (ret) > + goto err_disable_ohci_clk; > + } > + > + if (!IS_ERR(priv->phy)) { > + ret = phy_init(priv->phy); > + if (ret) > + goto err_disable_ahb_clk; > + > + ret = phy_power_on(priv->phy); > + if (ret) > + goto err_exit_phy; > + } Style-wise, I think I'd prefer not storing error values in the ohci_platform_priv struct, but rather using NULL pointers for when the clocks or phy references are unused. > @@ -83,17 +171,30 @@ static int ohci_platform_probe(struct platform_device *dev) > return -ENXIO; > } > > + hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev, > + dev_name(&dev->dev)); > + if (!hcd) > + return -ENOMEM; > + > + if (pdata == &ohci_platform_defaults) { > + struct ohci_platform_priv *priv = (struct ohci_platform_priv *) > + hcd_to_ohci(hcd)->priv; > + > + priv->phy = devm_phy_get(&dev->dev, "usb_phy"); > + if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) { > + err = -EPROBE_DEFER; > + goto err_put_hcd; > + } > + > + priv->ohci_clk = devm_clk_get(&dev->dev, "ohci"); > + priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); > + } I think you have to check the clocks for -EPROBE_DEFER as well here, not just the PHY. Otherwise you don't know the difference between "no clock provided", "error getting the clock reference" and "clock controller not initialized yet, try again". > @@ -178,6 +277,12 @@ static int ohci_platform_resume(struct device *dev) > #define ohci_platform_resume NULL > #endif /* CONFIG_PM */ > > +static const struct of_device_id ohci_platform_ids[] = { > + { .compatible = "platform-ohci", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ohci_platform_ids); I never liked the "platform-*" naming for compatible properties, the name of the driver is just based on a linux implementation detail (the platform bus), which could change. How about just calling the generic one "ohci" or "usb-ohci"? Arnd
Hi, On 01/06/2014 08:16 AM, Arnd Bergmann wrote: > On Monday 06 January 2014, Hans de Goede wrote: >> +Required properties: >> + - compatible: Should be "platform-ohci" >> + - reg: Address range of the ohci registers. >> + - interrupts: Should contain the ohci interrupt. >> + >> +Optional properties: >> + - clocks: array of clocks >> + - clock-names: clock names "ahb" and/or "ohci" >> + - phys: phy >> + - phy-names: "usb_phy" > > Maybe just "usb"? It obviously is a phy, so that part of the name > is a bit redundant. Actually, the "usb" part of the name also seems > redundant. Is it possible to make it an anonymous phy reference > without a phy-names property as we often do for clocks? I'm not a big fan of anonymous references, IE currently the ahci-platform driver is using an anonymous clk reference, but for sunxi (and ie imx too) it needs to be extended to 2 clks, which means using names, while keeping compatibility with the older anonymous using dts (and board) files. So I can agree to dropping the _phy, but I would like to keep the "usb" name I realize the chances are slim of ever needing 2 phys but never say never ... > >> +static int ohci_platform_power_on(struct platform_device *dev) >> +{ >> + struct usb_hcd *hcd = platform_get_drvdata(dev); >> + struct ohci_platform_priv *priv = >> + (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv; >> + int ret; >> + >> + if (!IS_ERR(priv->ohci_clk)) { >> + ret = clk_prepare_enable(priv->ohci_clk); >> + if (ret) >> + return ret; >> + } >> + >> + if (!IS_ERR(priv->ahb_clk)) { >> + ret = clk_prepare_enable(priv->ahb_clk); >> + if (ret) >> + goto err_disable_ohci_clk; >> + } >> + >> + if (!IS_ERR(priv->phy)) { >> + ret = phy_init(priv->phy); >> + if (ret) >> + goto err_disable_ahb_clk; >> + >> + ret = phy_power_on(priv->phy); >> + if (ret) >> + goto err_exit_phy; >> + } > > Style-wise, I think I'd prefer not storing error values in the > ohci_platform_priv struct, but rather using NULL pointers for > when the clocks or phy references are unused. This is a style I picked up from the mmc code, ie do a grep for !IS_ERR in drivers/mmc/host/*.c, but I agree it is not the prettiest, and on looking in other subsystems it is not common, so I'll convert this to storing NULL on error to get the phy or clk. >> @@ -83,17 +171,30 @@ static int ohci_platform_probe(struct platform_device *dev) >> return -ENXIO; >> } >> >> + hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev, >> + dev_name(&dev->dev)); >> + if (!hcd) >> + return -ENOMEM; >> + >> + if (pdata == &ohci_platform_defaults) { >> + struct ohci_platform_priv *priv = (struct ohci_platform_priv *) >> + hcd_to_ohci(hcd)->priv; >> + >> + priv->phy = devm_phy_get(&dev->dev, "usb_phy"); >> + if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) { >> + err = -EPROBE_DEFER; >> + goto err_put_hcd; >> + } >> + >> + priv->ohci_clk = devm_clk_get(&dev->dev, "ohci"); >> + priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); >> + } > > I think you have to check the clocks for -EPROBE_DEFER as well here, not > just the PHY. So far I've never seen clk_get return -EPROBE_DEFER, but I guess on some platforms it can. > Otherwise you don't know the difference between "no clock > provided", "error getting the clock reference" and "clock controller not > initialized yet, try again". I guess of these 3 we really only want to continue on "no clock provided", so I think something like this (for both clks and the phy) would be best: priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); if (IS_ERR(priv->ahb_clk)) { err = PTR_ERR(priv->ahb_clk); if (err != -EINVAL && err != -ENODATA) goto err_put_hcd; priv->ahb_clk = NULL; /* No clock provided */ } To clarify -EINVAL will be returned when there is no clock-names, and -ENODATA when the specified name is not found in clock-names. > >> @@ -178,6 +277,12 @@ static int ohci_platform_resume(struct device *dev) >> #define ohci_platform_resume NULL >> #endif /* CONFIG_PM */ >> >> +static const struct of_device_id ohci_platform_ids[] = { >> + { .compatible = "platform-ohci", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, ohci_platform_ids); > > I never liked the "platform-*" naming for compatible properties, the > name of the driver is just based on a linux implementation detail > (the platform bus), which could change. How about just calling the > generic one "ohci" or "usb-ohci"? usb-ohci seems free but usb-ehci are already taken by drivers/usb/host/ehci-ppc-of.c which is as the name implies pretty ppc specific. And since ehci-platform.c can be build on ppc too, we could end up with 2 drivers claiming the same compatibility string on ppc. Also uhci-platform.c is already using platform-uhci, so using ohci-platform and ehci-platform seems consistent and avoids the conflict with drivers/usb/host/ehci-ppc-of.c Regards, Hans
On Sun, Jan 05, 2014 at 11:04:39PM +0000, Hans de Goede wrote: > Add support for ohci-platform instantiation from devicetree, including > optionally getting clks and a phy from devicetree, and enabling / disabling > those on power_on / off. > > This should allow using ohci-platform from devicetree in various cases. > Specifically after this commit it can be used for the ohci controller found > on Allwinner sunxi SoCs. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../devicetree/bindings/usb/platform-ohci.txt | 25 ++++ > drivers/usb/host/ohci-platform.c | 146 ++++++++++++++++++--- > 2 files changed, 151 insertions(+), 20 deletions(-) > create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt > > diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt > new file mode 100644 > index 0000000..6846f1c > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt > @@ -0,0 +1,25 @@ > +Generic Platform OHCI controller > + > +Required properties: > + - compatible: Should be "platform-ohci" To me, "platform-ohci" seems rather Linux-specific. Platform devices are a Linux abstraction that don't correspond to any particular bus type in reality. We have some "*-generic" bindings. A name of that form might be more appropriate. Or we could choose an arbitrary OHCI implementation's vendor,ochi string and everything else can declare compatibility with that. > + - reg: Address range of the ohci registers. > + - interrupts: Should contain the ohci interrupt. > + > +Optional properties: > + - clocks: array of clocks > + - clock-names: clock names "ahb" and/or "ohci" A description of what the clocks are might be helpful. How about something like: - clocks: a list of phandle + clock specifier pairs, one for each entry in clock-names. - clock-names: Should contain: * "ahb" - some description here. * "ohci" - some description here. Thanks, Mark.
On Mon, 6 Jan 2014, Hans de Goede wrote: > Add support for ohci-platform instantiation from devicetree, including > optionally getting clks and a phy from devicetree, and enabling / disabling > those on power_on / off. > > This should allow using ohci-platform from devicetree in various cases. > Specifically after this commit it can be used for the ohci controller found > on Allwinner sunxi SoCs. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../devicetree/bindings/usb/platform-ohci.txt | 25 ++++ > drivers/usb/host/ohci-platform.c | 146 ++++++++++++++++++--- > 2 files changed, 151 insertions(+), 20 deletions(-) > create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt > > diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt > new file mode 100644 > index 0000000..6846f1c > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt > @@ -0,0 +1,25 @@ > +Generic Platform OHCI controller > + > +Required properties: > + - compatible: Should be "platform-ohci" > + - reg: Address range of the ohci registers. > + - interrupts: Should contain the ohci interrupt. > + > +Optional properties: > + - clocks: array of clocks > + - clock-names: clock names "ahb" and/or "ohci" Where does "ahb" come from, what does it mean, and how is it relevant to generic platforms? What about platforms that use 3 clocks? Alan Stern
On Monday 06 January 2014, Hans de Goede wrote: > On 01/06/2014 08:16 AM, Arnd Bergmann wrote: > > On Monday 06 January 2014, Hans de Goede wrote: > >> +Required properties: > >> + - compatible: Should be "platform-ohci" > >> + - reg: Address range of the ohci registers. > >> + - interrupts: Should contain the ohci interrupt. > >> + > >> +Optional properties: > >> + - clocks: array of clocks > >> + - clock-names: clock names "ahb" and/or "ohci" > >> + - phys: phy > >> + - phy-names: "usb_phy" > > > > Maybe just "usb"? It obviously is a phy, so that part of the name > > is a bit redundant. Actually, the "usb" part of the name also seems > > redundant. Is it possible to make it an anonymous phy reference > > without a phy-names property as we often do for clocks? > > I'm not a big fan of anonymous references, IE currently the ahci-platform > driver is using an anonymous clk reference, but for sunxi (and ie imx too) > it needs to be extended to 2 clks, which means using names, while > keeping compatibility with the older anonymous using dts (and board) > files. > > So I can agree to dropping the _phy, but I would like to keep the "usb" > name I realize the chances are slim of ever needing 2 phys but never > say never ... Ok, fair enough. > > Style-wise, I think I'd prefer not storing error values in the > > ohci_platform_priv struct, but rather using NULL pointers for > > when the clocks or phy references are unused. > > This is a style I picked up from the mmc code, ie do a grep for > !IS_ERR in drivers/mmc/host/*.c, but I agree it is not the prettiest, > and on looking in other subsystems it is not common, so I'll convert > this to storing NULL on error to get the phy or clk. Ok, thanks. > >> @@ -83,17 +171,30 @@ static int ohci_platform_probe(struct platform_device *dev) > >> return -ENXIO; > >> } > >> > >> + hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev, > >> + dev_name(&dev->dev)); > >> + if (!hcd) > >> + return -ENOMEM; > >> + > >> + if (pdata == &ohci_platform_defaults) { > >> + struct ohci_platform_priv *priv = (struct ohci_platform_priv *) > >> + hcd_to_ohci(hcd)->priv; > >> + > >> + priv->phy = devm_phy_get(&dev->dev, "usb_phy"); > >> + if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) { > >> + err = -EPROBE_DEFER; > >> + goto err_put_hcd; > >> + } > >> + > >> + priv->ohci_clk = devm_clk_get(&dev->dev, "ohci"); > >> + priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); > >> + } > > > > I think you have to check the clocks for -EPROBE_DEFER as well here, not > > just the PHY. > > So far I've never seen clk_get return -EPROBE_DEFER, but I guess on some > platforms it can. Right. Most clock controllers tend to be initialized at early boot time, but some of them are on external PMICs that are only probed after i2c or some other subsystem is up. > > Otherwise you don't know the difference between "no clock > > provided", "error getting the clock reference" and "clock controller not > > initialized yet, try again". > > I guess of these 3 we really only want to continue on "no clock provided", > so I think something like this (for both clks and the phy) would be best: > > priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); > if (IS_ERR(priv->ahb_clk)) { > err = PTR_ERR(priv->ahb_clk); > if (err != -EINVAL && err != -ENODATA) > goto err_put_hcd; > priv->ahb_clk = NULL; /* No clock provided */ > } > > To clarify -EINVAL will be returned when there is no clock-names, and > -ENODATA when the specified name is not found in clock-names. Ok, looks good. > >> @@ -178,6 +277,12 @@ static int ohci_platform_resume(struct device *dev) > >> #define ohci_platform_resume NULL > >> #endif /* CONFIG_PM */ > >> > >> +static const struct of_device_id ohci_platform_ids[] = { > >> + { .compatible = "platform-ohci", }, > >> + { } > >> +}; > >> +MODULE_DEVICE_TABLE(of, ohci_platform_ids); > > > > I never liked the "platform-*" naming for compatible properties, the > > name of the driver is just based on a linux implementation detail > > (the platform bus), which could change. How about just calling the > > generic one "ohci" or "usb-ohci"? > > usb-ohci seems free but usb-ehci are already taken by > drivers/usb/host/ehci-ppc-of.c which is as the name implies pretty ppc > specific. And since ehci-platform.c can be build on ppc too, we could > end up with 2 drivers claiming the same compatibility string on ppc. > > Also uhci-platform.c is already using platform-uhci, so using > ohci-platform and ehci-platform seems consistent and avoids the > conflict with drivers/usb/host/ehci-ppc-of.c Hmm, that file seems to do two things that ehci-platform doesn't: It handles big-endian controllers and it has special initialization for compatible="ibm,usb-ehci-440epx". It also uses a different way to get to the resources because the driver dates back to before the unification of platform_bus and of_platform_bus, but that can be trivially changed. I would hope that we can eventually merge the two drivers, and I'm guessing that we will sooner or later need the big-endian support on non-ppc machines anyway. The question is what to do about the 440epx hack. It may be small enough that we can just leave it as a quirk inside #ifdef CONFIG_PPC in the unified driver, or it may end up being shared with arm64 X-Gene, which seems to share a common ancestry with some of the ppc4xx socs. Arnd
Hi, On 01/06/2014 04:45 PM, Mark Rutland wrote: > On Sun, Jan 05, 2014 at 11:04:39PM +0000, Hans de Goede wrote: >> Add support for ohci-platform instantiation from devicetree, including >> optionally getting clks and a phy from devicetree, and enabling / disabling >> those on power_on / off. >> >> This should allow using ohci-platform from devicetree in various cases. >> Specifically after this commit it can be used for the ohci controller found >> on Allwinner sunxi SoCs. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../devicetree/bindings/usb/platform-ohci.txt | 25 ++++ >> drivers/usb/host/ohci-platform.c | 146 ++++++++++++++++++--- >> 2 files changed, 151 insertions(+), 20 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt >> >> diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt >> new file mode 100644 >> index 0000000..6846f1c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt >> @@ -0,0 +1,25 @@ >> +Generic Platform OHCI controller >> + >> +Required properties: >> + - compatible: Should be "platform-ohci" > > To me, "platform-ohci" seems rather Linux-specific. Platform devices are > a Linux abstraction that don't correspond to any particular bus type in > reality. > > We have some "*-generic" bindings. A name of that form might be more > appropriate. Or we could choose an arbitrary OHCI implementation's > vendor,ochi string and everything else can declare compatibility with > that. With the ehci patch I'll go for your suggestion to simply keep via,vt8500-ehci as compat string for it, without adding a new generic name. So for ohci I'll also go with the first platform to use it and thus "allwinner,sun4i-ohci" > >> + - reg: Address range of the ohci registers. >> + - interrupts: Should contain the ohci interrupt. >> + >> +Optional properties: >> + - clocks: array of clocks >> + - clock-names: clock names "ahb" and/or "ohci" > > A description of what the clocks are might be helpful. > > How about something like: > > - clocks: a list of phandle + clock specifier pairs, one for each entry > in clock-names. > > - clock-names: Should contain: > * "ahb" - some description here. > * "ohci" - some description here. As Alan pointed out by asking "what is ahb" these names are too platform specific (arm platform in this case), for a generic driver. So I'm going to call them clk1, clk2 and instead. It may seem a bit silly to use names at all in this case, but having names makes things a lot easier implementation wise. Regards, Hans
Hi, On 01/06/2014 04:49 PM, Alan Stern wrote: > On Mon, 6 Jan 2014, Hans de Goede wrote: > >> Add support for ohci-platform instantiation from devicetree, including >> optionally getting clks and a phy from devicetree, and enabling / disabling >> those on power_on / off. >> >> This should allow using ohci-platform from devicetree in various cases. >> Specifically after this commit it can be used for the ohci controller found >> on Allwinner sunxi SoCs. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../devicetree/bindings/usb/platform-ohci.txt | 25 ++++ >> drivers/usb/host/ohci-platform.c | 146 ++++++++++++++++++--- >> 2 files changed, 151 insertions(+), 20 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt >> >> diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt >> new file mode 100644 >> index 0000000..6846f1c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt >> @@ -0,0 +1,25 @@ >> +Generic Platform OHCI controller >> + >> +Required properties: >> + - compatible: Should be "platform-ohci" >> + - reg: Address range of the ohci registers. >> + - interrupts: Should contain the ohci interrupt. >> + >> +Optional properties: >> + - clocks: array of clocks >> + - clock-names: clock names "ahb" and/or "ohci" > > Where does "ahb" come from, what does it mean, and how is it relevant > to generic platforms? ahb is an ARM specific thing, so your right it does not belong in a generic driver. I'll use clk1 and clk2 as names in my next version. > What about platforms that use 3 clocks? Ah yes I see some platforms have 3 clocks, I'll also add a clk3. Regards, Hans
On Tuesday 07 January 2014 22:03:11 Hans de Goede wrote: > >> + > >> +Optional properties: > >> + - clocks: array of clocks > >> + - clock-names: clock names "ahb" and/or "ohci" > > > > Where does "ahb" come from, what does it mean, and how is it relevant > > to generic platforms? > > ahb is an ARM specific thing, so your right it does not belong in a > generic driver. I'll use clk1 and clk2 as names in my next version. While AHB is a bus created by ARM Ltd, it's not actually specific to the ARM architecture. My guess is that it is in fact used on 95% of all SoCs, so I would leave it at that. For the other clock, I think that's actually the bus clock for the USB interface, so I would not call it "ohci" but rather just "usb" or "phy". I think it's important to distinguish the names and not just use "clk1" and "clk2", because the driver may actually want to access a particular clock in some scenario. > > What about platforms that use 3 clocks? > > Ah yes I see some platforms have 3 clocks, I'll also add a clk3. I guess we should try to find at least one hardware data sheet for an actual ohci implementation and look at what the clock inputs are really called. A lot of the drivers seem to incorrectly use the name for the clock signal inside of the soc, which tends to be named after who provides it, not what it's used for. Arnd
Hi, On 01/07/2014 10:16 PM, Arnd Bergmann wrote: > On Tuesday 07 January 2014 22:03:11 Hans de Goede wrote: >>>> + >>>> +Optional properties: >>>> + - clocks: array of clocks >>>> + - clock-names: clock names "ahb" and/or "ohci" >>> >>> Where does "ahb" come from, what does it mean, and how is it relevant >>> to generic platforms? >> >> ahb is an ARM specific thing, so your right it does not belong in a >> generic driver. I'll use clk1 and clk2 as names in my next version. > > While AHB is a bus created by ARM Ltd, it's not actually specific > to the ARM architecture. My guess is that it is in fact used on 95% > of all SoCs, so I would leave it at that. For the other clock, I > think that's actually the bus clock for the USB interface, so I would > not call it "ohci" but rather just "usb" or "phy". > > I think it's important to distinguish the names and not just use > "clk1" and "clk2", because the driver may actually want to access > a particular clock in some scenario. The idea here is to have a generic driver, if a driver needs to know about a specific clock, it will likely be another device specific driver and it can use its own dt-bindings and clock names. I believe that for a generic driver meant to cover common hardware configs, simply having X clks and then on power_on enabling clk1, then clk2, then clk3, etc. and on power off do the same in reverse other is a good approach. > >>> What about platforms that use 3 clocks? >> >> Ah yes I see some platforms have 3 clocks, I'll also add a clk3. > > I guess we should try to find at least one hardware data sheet > for an actual ohci implementation and look at what the clock > inputs are really called. A lot of the drivers seem to incorrectly > use the name for the clock signal inside of the soc, which tends > to be named after who provides it, not what it's used for. I don't know about data-sheets, but an example of a driver with 3 clocks is drivers/usb/host/ohci-at91.c fwiw it uses "uhpck", "hclk" and "usb_clk" as clk names. Regards, Hans
Hi, On 01/06/2014 08:50 AM, Hans de Goede wrote: <snip> >> Otherwise you don't know the difference between "no clock >> provided", "error getting the clock reference" and "clock controller not >> initialized yet, try again". > > I guess of these 3 we really only want to continue on "no clock provided", > so I think something like this (for both clks and the phy) would be best: > > priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); > if (IS_ERR(priv->ahb_clk)) { > err = PTR_ERR(priv->ahb_clk); > if (err != -EINVAL && err != -ENODATA) > goto err_put_hcd; > priv->ahb_clk = NULL; /* No clock provided */ > } > > To clarify -EINVAL will be returned when there is no clock-names, and > -ENODATA when the specified name is not found in clock-names. Ok, so I've got this wrong, if there is no clk by that name specified in dt -ENOENT will be returned. Actually -ENOENT is the only error clk_get and thus devm_clk_get will ever return. So it seems that clk_get currently is not properly passing along probe-deferral. To make things future proof I will add a probe deferral check to the next version of my patch. Regards, Hans
On Tue, 7 Jan 2014, Hans de Goede wrote: > Hi, > > On 01/07/2014 10:16 PM, Arnd Bergmann wrote: > > On Tuesday 07 January 2014 22:03:11 Hans de Goede wrote: > >>>> + > >>>> +Optional properties: > >>>> + - clocks: array of clocks > >>>> + - clock-names: clock names "ahb" and/or "ohci" > >>> > >>> Where does "ahb" come from, what does it mean, and how is it relevant > >>> to generic platforms? > >> > >> ahb is an ARM specific thing, so your right it does not belong in a > >> generic driver. I'll use clk1 and clk2 as names in my next version. > > > > While AHB is a bus created by ARM Ltd, it's not actually specific > > to the ARM architecture. My guess is that it is in fact used on 95% > > of all SoCs, so I would leave it at that. For the other clock, I > > think that's actually the bus clock for the USB interface, so I would > > not call it "ohci" but rather just "usb" or "phy". > > > > I think it's important to distinguish the names and not just use > > "clk1" and "clk2", because the driver may actually want to access > > a particular clock in some scenario. > > The idea here is to have a generic driver, if a driver needs to know > about a specific clock, it will likely be another device specific > driver and it can use its own dt-bindings and clock names. I believe > that for a generic driver meant to cover common hardware configs, > simply having X clks and then on power_on enabling clk1, then clk2, > then clk3, etc. and on power off do the same in reverse other is > a good approach. I agree. Alan Stern
diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt new file mode 100644 index 0000000..6846f1c --- /dev/null +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt @@ -0,0 +1,25 @@ +Generic Platform OHCI controller + +Required properties: + - compatible: Should be "platform-ohci" + - reg: Address range of the ohci registers. + - interrupts: Should contain the ohci interrupt. + +Optional properties: + - clocks: array of clocks + - clock-names: clock names "ahb" and/or "ohci" + - phys: phy + - phy-names: "usb_phy" + +Example: + + ohci0: ohci@0x01c14400 { + compatible = "platform-ohci"; + reg = <0x01c14400 0x100>; + interrupts = <0 64 4>; + clocks = <&ahb_gates 2>, <&usb_clk 6>; + clock-names = "ahb", "ohci"; + phys = <&usbphy 1>; + phy-names = "usb_phy"; + status = "disabled"; + }; diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c index f351ff5..b7abb44 100644 --- a/drivers/usb/host/ohci-platform.c +++ b/drivers/usb/host/ohci-platform.c @@ -14,11 +14,14 @@ * Licensed under the GNU/GPL. See COPYING for details. */ +#include <linux/clk.h> +#include <linux/dma-mapping.h> #include <linux/hrtimer.h> #include <linux/io.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/err.h> +#include <linux/phy/phy.h> #include <linux/platform_device.h> #include <linux/usb/ohci_pdriver.h> #include <linux/usb.h> @@ -28,6 +31,12 @@ #define DRIVER_DESC "OHCI generic platform driver" +struct ohci_platform_priv { + struct clk *ahb_clk; + struct clk *ohci_clk; + struct phy *phy; +}; + static const char hcd_name[] = "ohci-platform"; static int ohci_platform_reset(struct usb_hcd *hcd) @@ -48,11 +57,79 @@ static int ohci_platform_reset(struct usb_hcd *hcd) return ohci_setup(hcd); } +static int ohci_platform_power_on(struct platform_device *dev) +{ + struct usb_hcd *hcd = platform_get_drvdata(dev); + struct ohci_platform_priv *priv = + (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv; + int ret; + + if (!IS_ERR(priv->ohci_clk)) { + ret = clk_prepare_enable(priv->ohci_clk); + if (ret) + return ret; + } + + if (!IS_ERR(priv->ahb_clk)) { + ret = clk_prepare_enable(priv->ahb_clk); + if (ret) + goto err_disable_ohci_clk; + } + + if (!IS_ERR(priv->phy)) { + ret = phy_init(priv->phy); + if (ret) + goto err_disable_ahb_clk; + + ret = phy_power_on(priv->phy); + if (ret) + goto err_exit_phy; + } + + return 0; + +err_exit_phy: + phy_exit(priv->phy); +err_disable_ahb_clk: + if (!IS_ERR(priv->ahb_clk)) + clk_disable_unprepare(priv->ahb_clk); +err_disable_ohci_clk: + if (!IS_ERR(priv->ohci_clk)) + clk_disable_unprepare(priv->ohci_clk); + + return ret; +} + +static void ohci_platform_power_off(struct platform_device *dev) +{ + struct usb_hcd *hcd = platform_get_drvdata(dev); + struct ohci_platform_priv *priv = + (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv; + + if (!IS_ERR(priv->phy)) { + phy_power_off(priv->phy); + phy_exit(priv->phy); + } + + if (!IS_ERR(priv->ahb_clk)) + clk_disable_unprepare(priv->ahb_clk); + + if (!IS_ERR(priv->ohci_clk)) + clk_disable_unprepare(priv->ohci_clk); +} + static struct hc_driver __read_mostly ohci_platform_hc_driver; static const struct ohci_driver_overrides platform_overrides __initconst = { - .product_desc = "Generic Platform OHCI controller", - .reset = ohci_platform_reset, + .product_desc = "Generic Platform OHCI controller", + .reset = ohci_platform_reset, + .extra_priv_size = sizeof(struct ohci_platform_priv), +}; + +static struct usb_ohci_pdata ohci_platform_defaults = { + .power_on = ohci_platform_power_on, + .power_suspend = ohci_platform_power_off, + .power_off = ohci_platform_power_off, }; static int ohci_platform_probe(struct platform_device *dev) @@ -60,12 +137,23 @@ static int ohci_platform_probe(struct platform_device *dev) struct usb_hcd *hcd; struct resource *res_mem; struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev); - int irq; - int err = -ENOMEM; + int irq, err; + /* + * use reasonable defaults so platforms don't have to provide these. + * with DT probing on ARM. + */ if (!pdata) { - WARN_ON(1); - return -ENODEV; + pdata = dev->dev.platform_data = &ohci_platform_defaults; + /* + * Right now device-tree probed devices don't get dma_mask set. + * Since shared usb code relies on it, set it here for now. + * Once we have dma capability bindings this can go away. + */ + err = dma_coerce_mask_and_coherent(&dev->dev, + DMA_BIT_MASK(32)); + if (err) + return err; } if (usb_disabled()) @@ -83,17 +171,30 @@ static int ohci_platform_probe(struct platform_device *dev) return -ENXIO; } + hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev, + dev_name(&dev->dev)); + if (!hcd) + return -ENOMEM; + + if (pdata == &ohci_platform_defaults) { + struct ohci_platform_priv *priv = (struct ohci_platform_priv *) + hcd_to_ohci(hcd)->priv; + + priv->phy = devm_phy_get(&dev->dev, "usb_phy"); + if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) { + err = -EPROBE_DEFER; + goto err_put_hcd; + } + + priv->ohci_clk = devm_clk_get(&dev->dev, "ohci"); + priv->ahb_clk = devm_clk_get(&dev->dev, "ahb"); + } + + platform_set_drvdata(dev, hcd); if (pdata->power_on) { err = pdata->power_on(dev); if (err < 0) - return err; - } - - hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev, - dev_name(&dev->dev)); - if (!hcd) { - err = -ENOMEM; - goto err_power; + goto err_put_hcd; } hcd->rsrc_start = res_mem->start; @@ -102,21 +203,19 @@ static int ohci_platform_probe(struct platform_device *dev) hcd->regs = devm_ioremap_resource(&dev->dev, res_mem); if (IS_ERR(hcd->regs)) { err = PTR_ERR(hcd->regs); - goto err_put_hcd; + goto err_power; } err = usb_add_hcd(hcd, irq, IRQF_SHARED); if (err) - goto err_put_hcd; - - platform_set_drvdata(dev, hcd); + goto err_power; return err; -err_put_hcd: - usb_put_hcd(hcd); err_power: if (pdata->power_off) pdata->power_off(dev); +err_put_hcd: + usb_put_hcd(hcd); return err; } @@ -178,6 +277,12 @@ static int ohci_platform_resume(struct device *dev) #define ohci_platform_resume NULL #endif /* CONFIG_PM */ +static const struct of_device_id ohci_platform_ids[] = { + { .compatible = "platform-ohci", }, + { } +}; +MODULE_DEVICE_TABLE(of, ohci_platform_ids); + static const struct platform_device_id ohci_platform_table[] = { { "ohci-platform", 0 }, { } @@ -198,6 +303,7 @@ static struct platform_driver ohci_platform_driver = { .owner = THIS_MODULE, .name = "ohci-platform", .pm = &ohci_platform_pm_ops, + .of_match_table = ohci_platform_ids, } };
Add support for ohci-platform instantiation from devicetree, including optionally getting clks and a phy from devicetree, and enabling / disabling those on power_on / off. This should allow using ohci-platform from devicetree in various cases. Specifically after this commit it can be used for the ohci controller found on Allwinner sunxi SoCs. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../devicetree/bindings/usb/platform-ohci.txt | 25 ++++ drivers/usb/host/ohci-platform.c | 146 ++++++++++++++++++--- 2 files changed, 151 insertions(+), 20 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt