| Message ID | 201402270312.51588.sergei.shtylyov@cogentembedded.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 27/02/14 00:12, Sergei Shtylyov wrote: > Add support of the device tree probing for the Renesas R-Car generation 2 SoCs > documenting the device tree binding as necessary. So what happened w.r.t to my last set of patches for this?
Hello. On 27-02-2014 16:57, Ben Dooks wrote: >> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs >> documenting the device tree binding as necessary. > So what happened w.r.t to my last set of patches for this? Hm, I forgot you've posted the OF patch already (and I was tasked with the PHY driver OF support). I have re-read the thread now, and the issue was that Felipe wanted an ACK from one of DT maintainers. I had issues with the "compatible" props the driver reacts on. Also, I didn't see your bindings doc patch the last time you posted the driver patch. I have one more issue now and will follow up to the patch with it... WBR, Sergei -- 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 Thu, Feb 27, 2014 at 12:12:50AM +0000, Sergei Shtylyov wrote: > Add support of the device tree probing for the Renesas R-Car generation 2 SoCs > documenting the device tree binding as necessary. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo. > > Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++ > drivers/usb/phy/phy-rcar-gen2-usb.c | 64 ++++++++++++++-- > 2 files changed, 85 insertions(+), 8 deletions(-) > > Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt > =================================================================== > --- /dev/null > +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt > @@ -0,0 +1,29 @@ > +* Renesas R-Car generation 2 USB PHY > + > +This file provides information on what the device node for the R-Car generation > +2 USB PHY contains. > + > +Required properties: > +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC. > + "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC. Is the r8a7791's USB PHY known to be different to that of the r8a7790? If this is just to possibly handle the two differently in future, why not have "renesas,usb-phy-r8a7790" as a fallback in the compatible list? That was you only need it in the driver for now. > +- reg: offset and length of the register block. > +- clocks: clock phandle and specifier pair. > +- clock-names: string, clock input name, must be "usbhs". > + > +Optional properties: > +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected > + to PCI EHCI/OHCI; otherwise, it will be connected to the > + USBHS controller. > +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected > + to PCI EHCI/OHCI; otherwise, it will be connected to the > + USBSS controller (xHCI). When would you want this to connect to PCI, and when would you not? Why is this not a run-time decision? > + > +Example (Lager board): > + > + usb-phy@e6590100 { > + compatible = "renesas,usb-phy-r8a7790"; > + reg = <0 0xe6590100 0 0x100>; > + clocks = <&mstp7_clks R8A7790_CLK_HSUSB>; > + clock-names = "usbhs"; > + renesas,channel2-pci; > + }; We're not using the generic phy bindings? How is the linkage to the host controller expressed? [...] > - clk = devm_clk_get(dev, "usbhs"); > + if (np) > + clk = of_clk_get_by_name(np, "usbhs"); > + else > + clk = clk_get(dev, "usbhs"); Doesn't clk_get (and hence devm_clk_get) call of_clk_get_by_name? Cheers, Mark. -- 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 27/02/14 00:12, Sergei Shtylyov wrote: > Add support of the device tree probing for the Renesas R-Car generation 2 SoCs > documenting the device tree binding as necessary. You've popped in some fixes for the driver probe in here as well. Could you do the fixes as a patch and send those before the devicetree code is done? > + > static int rcar_gen2_usb_phy_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > struct rcar_gen2_phy_platform_data *pdata; > struct rcar_gen2_usb_phy_priv *priv; > struct resource *res; > @@ -177,13 +210,19 @@ static int rcar_gen2_usb_phy_probe(struc > struct clk *clk; > int retval; > > - pdata = dev_get_platdata(dev); > + if (np) > + pdata = rcar_gen2_usb_phy_parse_dt(dev); > + else > + pdata = dev_get_platdata(dev); > if (!pdata) { > dev_err(dev, "No platform data\n"); > return -EINVAL; > } > > - clk = devm_clk_get(dev, "usbhs"); > + if (np) > + clk = of_clk_get_by_name(np, "usbhs"); > + else > + clk = clk_get(dev, "usbhs"); Can be removed, just add a clock-name of usbhs in the device node. > if (IS_ERR(clk)) { > dev_err(dev, "Can't get the clock\n"); > return PTR_ERR(clk); > @@ -191,13 +230,16 @@ static int rcar_gen2_usb_phy_probe(struc > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(dev, res); > - if (IS_ERR(base)) > - return PTR_ERR(base); > + if (IS_ERR(base)) { > + retval = PTR_ERR(base); > + goto error; > + } > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) { > dev_err(dev, "Memory allocation failed\n"); > - return -ENOMEM; > + retval = -ENOMEM; > + goto error; > } Probably should be separate patch to fix probe issues. > > spin_lock_init(&priv->lock); > @@ -216,12 +258,16 @@ static int rcar_gen2_usb_phy_probe(struc > retval = usb_add_phy_dev(&priv->phy); > if (retval < 0) { > dev_err(dev, "Failed to add USB phy\n"); > - return retval; > + goto error; > } > > platform_set_drvdata(pdev, priv); > > return retval; > + > +error: > + clk_put(clk); > + return retval; > } Again, should have been rolled into fix patch.
On 27-02-2014 20:06, Ben Dooks wrote: >> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs >> documenting the device tree binding as necessary. > You've popped in some fixes for the driver probe in here as well. No, I didn't -- it's all the result of not using devm_clk_get(). [...] >> @@ -177,13 +210,19 @@ static int rcar_gen2_usb_phy_probe(struc [...] >> - clk = devm_clk_get(dev, "usbhs"); >> + if (np) >> + clk = of_clk_get_by_name(np, "usbhs"); >> + else >> + clk = clk_get(dev, "usbhs"); > Can be removed, just add a clock-name of usbhs in the device node. Ah, I haven't figured out I should check clk_get() first... Yes, I'm adding the "clock-names" prop. WBR, Sergei -- 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
Hello. On 02/27/2014 06:56 PM, Mark Rutland wrote: >> Add support of the device tree probing for the Renesas R-Car generation 2 SoCs >> documenting the device tree binding as necessary. >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> --- >> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo. >> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++ >> drivers/usb/phy/phy-rcar-gen2-usb.c | 64 ++++++++++++++-- >> 2 files changed, 85 insertions(+), 8 deletions(-) >> Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt >> =================================================================== >> --- /dev/null >> +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt >> @@ -0,0 +1,29 @@ >> +* Renesas R-Car generation 2 USB PHY >> + >> +This file provides information on what the device node for the R-Car generation >> +2 USB PHY contains. >> + >> +Required properties: >> +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC. >> + "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC. > Is the r8a7791's USB PHY known to be different to that of the r8a7790? Not at all, according to the preliminary manuals. The problem is the SH-Mobile maintainers don't trust the manuals (translated from Japanese, they are sometimes indeed of a questionable quality). > If this is just to possibly handle the two differently in future, why > not have "renesas,usb-phy-r8a7790" as a fallback in the compatible list? > That was you only need it in the driver for now. The SH-Mobile maintainers are against that approach. >> +- reg: offset and length of the register block. >> +- clocks: clock phandle and specifier pair. >> +- clock-names: string, clock input name, must be "usbhs". >> + >> +Optional properties: >> +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected >> + to PCI EHCI/OHCI; otherwise, it will be connected to the >> + USBHS controller. >> +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected >> + to PCI EHCI/OHCI; otherwise, it will be connected to the >> + USBSS controller (xHCI). > When would you want this to connect to PCI, and when would you not? Why > is this not a run-time decision? I think it's mostly connector type thing: A (host) vs B (device), USB 2.0 vs 3.0. With B connector you certainly want USBHS controller which is includes the function controller (testing has shown that it also has the host controller but the manuals don't quite say that). With A connector, user would probably prefer PCI EHCI/OHCI, unless you have a USB 3.0 port in which case the preference would probably be xHCI. So it appears to be the board specific thing. Some runtime switching (e.g. downgrading from xHCI to PCI EHCI/OHCI) might be considered too but I don't quite have a picture how that would work yet. >> + >> +Example (Lager board): >> + >> + usb-phy@e6590100 { >> + compatible = "renesas,usb-phy-r8a7790"; >> + reg = <0 0xe6590100 0 0x100>; >> + clocks = <&mstp7_clks R8A7790_CLK_HSUSB>; >> + clock-names = "usbhs"; >> + renesas,channel2-pci; >> + }; > We're not using the generic phy bindings? It's not a driver/phy/ but driver/usb/phy/ code, so I guess we're not. > How is the linkage to the host controller expressed? Looking at Documentation/devicetree/bindings/usb/, it's via "usb-phy" property in the host device nodes. > [...] >> - clk = devm_clk_get(dev, "usbhs"); >> + if (np) >> + clk = of_clk_get_by_name(np, "usbhs"); >> + else >> + clk = clk_get(dev, "usbhs"); > Doesn't clk_get (and hence devm_clk_get) call of_clk_get_by_name? Indeed, stupid me. I just didn't expect it, so didn't even look there. > Cheers, > Mark. WBR, Sergei -- 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
Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt =================================================================== --- /dev/null +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt @@ -0,0 +1,29 @@ +* Renesas R-Car generation 2 USB PHY + +This file provides information on what the device node for the R-Car generation +2 USB PHY contains. + +Required properties: +- compatible: "renesas,usb-phy-r8a7790" if the device is a part of R8A7790 SoC. + "renesas,usb-phy-r8a7791" if the device is a part of R8A7791 SoC. +- reg: offset and length of the register block. +- clocks: clock phandle and specifier pair. +- clock-names: string, clock input name, must be "usbhs". + +Optional properties: +- renesas,channel0-pci: boolean, specify when USB channel 0 should be connected + to PCI EHCI/OHCI; otherwise, it will be connected to the + USBHS controller. +- renesas,channel2-pci: boolean, specify when USB channel 2 should be connected + to PCI EHCI/OHCI; otherwise, it will be connected to the + USBSS controller (xHCI). + +Example (Lager board): + + usb-phy@e6590100 { + compatible = "renesas,usb-phy-r8a7790"; + reg = <0 0xe6590100 0 0x100>; + clocks = <&mstp7_clks R8A7790_CLK_HSUSB>; + clock-names = "usbhs"; + renesas,channel2-pci; + }; Index: usb/drivers/usb/phy/phy-rcar-gen2-usb.c =================================================================== --- usb.orig/drivers/usb/phy/phy-rcar-gen2-usb.c +++ usb/drivers/usb/phy/phy-rcar-gen2-usb.c @@ -1,8 +1,8 @@ /* * Renesas R-Car Gen2 USB phy driver * - * Copyright (C) 2013 Renesas Solutions Corp. - * Copyright (C) 2013 Cogent Embedded, Inc. + * Copyright (C) 2013-2014 Renesas Solutions Corp. + * Copyright (C) 2013-2014 Cogent Embedded, Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -13,6 +13,7 @@ #include <linux/delay.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/platform_data/usb-rcar-gen2-phy.h> #include <linux/platform_device.h> #include <linux/spinlock.h> @@ -167,9 +168,41 @@ out: spin_unlock_irqrestore(&priv->lock, flags); } +#ifdef CONFIG_OF +static struct rcar_gen2_phy_platform_data * +rcar_gen2_usb_phy_parse_dt(struct device *dev) +{ + struct device_node *np = dev->of_node; + struct rcar_gen2_phy_platform_data *pdata; + + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return NULL; + + pdata->chan0_pci = of_property_read_bool(np, "renesas,channel0-pci"); + pdata->chan2_pci = of_property_read_bool(np, "renesas,channel2-pci"); + + return pdata; +} + +static const struct of_device_id rcar_gen2_usb_phy_match_table[] = { + { .compatible = "renesas,usb-phy-r8a7790" }, + { .compatible = "renesas,usb-phy-r8a7791" }, + { } +}; +MODULE_DEVICE_TABLE(of, rcar_gen2_usb_phy_match_table); +#else +static inline struct rcar_gen2_phy_platform_data * +rcar_gen2_usb_phy_parse_dt(struct device *dev) +{ + return NULL; +} +#endif + static int rcar_gen2_usb_phy_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; struct rcar_gen2_phy_platform_data *pdata; struct rcar_gen2_usb_phy_priv *priv; struct resource *res; @@ -177,13 +210,19 @@ static int rcar_gen2_usb_phy_probe(struc struct clk *clk; int retval; - pdata = dev_get_platdata(dev); + if (np) + pdata = rcar_gen2_usb_phy_parse_dt(dev); + else + pdata = dev_get_platdata(dev); if (!pdata) { dev_err(dev, "No platform data\n"); return -EINVAL; } - clk = devm_clk_get(dev, "usbhs"); + if (np) + clk = of_clk_get_by_name(np, "usbhs"); + else + clk = clk_get(dev, "usbhs"); if (IS_ERR(clk)) { dev_err(dev, "Can't get the clock\n"); return PTR_ERR(clk); @@ -191,13 +230,16 @@ static int rcar_gen2_usb_phy_probe(struc res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_ioremap_resource(dev, res); - if (IS_ERR(base)) - return PTR_ERR(base); + if (IS_ERR(base)) { + retval = PTR_ERR(base); + goto error; + } priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) { dev_err(dev, "Memory allocation failed\n"); - return -ENOMEM; + retval = -ENOMEM; + goto error; } spin_lock_init(&priv->lock); @@ -216,12 +258,16 @@ static int rcar_gen2_usb_phy_probe(struc retval = usb_add_phy_dev(&priv->phy); if (retval < 0) { dev_err(dev, "Failed to add USB phy\n"); - return retval; + goto error; } platform_set_drvdata(pdev, priv); return retval; + +error: + clk_put(clk); + return retval; } static int rcar_gen2_usb_phy_remove(struct platform_device *pdev) @@ -229,6 +275,7 @@ static int rcar_gen2_usb_phy_remove(stru struct rcar_gen2_usb_phy_priv *priv = platform_get_drvdata(pdev); usb_remove_phy(&priv->phy); + clk_put(priv->clk); return 0; } @@ -236,6 +283,7 @@ static int rcar_gen2_usb_phy_remove(stru static struct platform_driver rcar_gen2_usb_phy_driver = { .driver = { .name = "usb_phy_rcar_gen2", + .of_match_table = of_match_ptr(rcar_gen2_usb_phy_match_table), }, .probe = rcar_gen2_usb_phy_probe, .remove = rcar_gen2_usb_phy_remove,
Add support of the device tree probing for the Renesas R-Car generation 2 SoCs documenting the device tree binding as necessary. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo. Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++ drivers/usb/phy/phy-rcar-gen2-usb.c | 64 ++++++++++++++-- 2 files changed, 85 insertions(+), 8 deletions(-) -- 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