| Message ID | 201403010407.54639.sergei.shtylyov@cogentembedded.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, On Sat, Mar 01, 2014 at 04:07:53AM +0300, 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> Unless someone on devicetree@vger gives me an ACK pretty soon, I'm afraid this patch will miss v3.15. > > --- > This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo. > > Changes in version 2: > - restored devm_clk_get() call and the error handling logic in the probe() > method, removed clk_put() call in the remove() method. > > Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ > drivers/usb/phy/phy-rcar-gen2-usb.c | 42 ++++++++++++++-- > 2 files changed, 68 insertions(+), 3 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. > +- 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). I wonder if these two properties should be taken care by pinctrl framework instead. > + > +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,6 +168,37 @@ 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; > @@ -177,7 +209,10 @@ static int rcar_gen2_usb_phy_probe(struc > struct clk *clk; > int retval; > > - pdata = dev_get_platdata(dev); > + if (dev->of_node) > + 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; > @@ -236,6 +271,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,
Hi, On Mon, Mar 03, 2014 at 08:50:33PM +0300, Sergei Shtylyov wrote: > Hello. > > On 03/03/2014 07:44 PM, Felipe Balbi 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> > > >Unless someone on devicetree@vger gives me an ACK pretty soon, I'm > >afraid this patch will miss v3.15. > > Ugh, the USB acceptance window is getting very short... well, -rc5 is already out... I want to have at least 2 weeks of "linux-next soakage" (yes, I just coined a new term heh). Since most releases go up to -rc7, it's about time to close the tree ;-) > >>--- > >>This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo. > >> > >>Changes in version 2: > >>- restored devm_clk_get() call and the error handling logic in the probe() > >> method, removed clk_put() call in the remove() method. > > >> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ > >> drivers/usb/phy/phy-rcar-gen2-usb.c | 42 ++++++++++++++-- > >> 2 files changed, 68 insertions(+), 3 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. > >>+- 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). > > >I wonder if these two properties should be taken care by pinctrl > >framework instead. > > No, the internal port multiplexing is controlled by the PHY > itself, see the driver source. The same way it was with USB port 1 of > the generation 1 R-Car SoCs. ok, fair enough.
Hello. On 03/03/2014 07:44 PM, Felipe Balbi 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> > Unless someone on devicetree@vger gives me an ACK pretty soon, I'm > afraid this patch will miss v3.15. Ugh, the USB acceptance window is getting very short... >> --- >> This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo. >> >> Changes in version 2: >> - restored devm_clk_get() call and the error handling logic in the probe() >> method, removed clk_put() call in the remove() method. >> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ >> drivers/usb/phy/phy-rcar-gen2-usb.c | 42 ++++++++++++++-- >> 2 files changed, 68 insertions(+), 3 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. >> +- 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). > I wonder if these two properties should be taken care by pinctrl > framework instead. No, the internal port multiplexing is controlled by the PHY itself, see the driver source. The same way it was with USB port 1 of the generation 1 R-Car SoCs. 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 06/03/14 19:39, Sergei Shtylyov wrote: > Hello. > > On 03/03/2014 07:44 PM, Felipe Balbi 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> > >> Unless someone on devicetree@vger gives me an ACK pretty soon, I'm >> afraid this patch will miss v3.15. > > Mark, you've commented on v1 of the patch, I hope I cleared things > up for you in my reply, how about an ACK (or at least new portion of > comments)? And whose patches are we going to use? :-p
Hello. On 03/03/2014 07:44 PM, Felipe Balbi 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> > Unless someone on devicetree@vger gives me an ACK pretty soon, I'm > afraid this patch will miss v3.15. Mark, you've commented on v1 of the patch, I hope I cleared things up for you in my reply, how about an ACK (or at least new portion of comments)? 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 Sat, Mar 1, 2014 at 10:07 AM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> 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. > > Changes in version 2: > - restored devm_clk_get() call and the error handling logic in the probe() > method, removed clk_put() call in the remove() method. > > Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ > drivers/usb/phy/phy-rcar-gen2-usb.c | 42 ++++++++++++++-- > 2 files changed, 68 insertions(+), 3 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. > +- 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). Thanks for your efforts here, but this DT binding looks like a mix of software configuration and hardware description. I would like to omit the software configuration portion if possible and only describe the hardware. > +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; > + }; As an example, instead of relying on "renesas,channel0-pci" or "renesas,channel2-pci" to specify which hard coded software configuration you want please rework this to expose 3 separate channels and use DT to point each host controller to the right channel. That will give us the opportunity to perform any kind of run time selection and not be limited by software policy set by DT. So this is a NAK on my side. I expect better. Thanks, / magnus -- 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 07-03-2014 9:46, Magnus Damm 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. >> Changes in version 2: >> - restored devm_clk_get() call and the error handling logic in the probe() >> method, removed clk_put() call in the remove() method. >> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ >> drivers/usb/phy/phy-rcar-gen2-usb.c | 42 ++++++++++++++-- >> 2 files changed, 68 insertions(+), 3 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. >> +- 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). > Thanks for your efforts here, but this DT binding looks like a mix of > software configuration and hardware description. I disagree. I have already explained to Mark Rutland how selecting a particular controller for a particular USB port should depend on the kind of the USB connector used, i.e. the board hardware. >> +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; >> + }; > As an example, instead of relying on "renesas,channel0-pci" or > "renesas,channel2-pci" to specify which hard coded software > configuration you want please rework this to expose 3 separate > channels and use DT to point each host controller to the right > channel. So you want to expose the channels as "virtual devices"? Or what do you want? Also, it seems almost impossible to represent some USB controllers in DT -- such as USBHS in particular. > That will give us the opportunity to perform any kind of run time > selection and not be limited by software policy set by DT. Sorry, but I don't see how using DT could help with run-time selection. Could you please explain? > So this is a NAK on my side. I expect better. Thanks for your long awaited feedback. > Thanks, > / magnus 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 03/07/2014 05:27 PM, 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. >>> Changes in version 2: >>> - restored devm_clk_get() call and the error handling logic in the probe() >>> method, removed clk_put() call in the remove() method. >>> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ >>> drivers/usb/phy/phy-rcar-gen2-usb.c | 42 >>> ++++++++++++++-- >>> 2 files changed, 68 insertions(+), 3 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. >>> +- 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). >> Thanks for your efforts here, but this DT binding looks like a mix of >> software configuration and hardware description. > I disagree. I have already explained to Mark Rutland how selecting a > particular controller for a particular USB port should depend on the kind of > the USB connector used, i.e. the board hardware. >>> +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; >>> + }; >> As an example, instead of relying on "renesas,channel0-pci" or >> "renesas,channel2-pci" to specify which hard coded software >> configuration you want please rework this to expose 3 separate >> channels and use DT to point each host controller to the right >> channel. > So you want to expose the channels as "virtual devices"? Or what do you > want? OK, let's assume you want either "virtual" PHYs or channel subnodes in the PHY node we have now. Now what the PHY driver should do with all these phandles pointing from the controller nodes to the PHY nodes or the channel subnodes? Scan all device tree in search of the nodes having certain prop and get info from such nodes on how to program the channel MUXing register using their "compatible" props? Sorry, that doesn't appeal to me at all. Now if we got the phandles pointing from the PHY node back to the selected controllers, things would look somewhat better. Yet it still looks like an unnecessary complication of what I'm proposing and so doesn't appeal to me much either. So, what's your intent here (other than to make my life harder :-)? > Also, it seems almost impossible to represent some USB controllers in DT > -- such as USBHS in particular. I've spent several days staring at the various USBHS code and I'm going to send you a more detailed report on USBHS vs DT issues. [...] >> So this is a NAK on my side. I expect better. > Thanks for your long awaited feedback. I'm not convinced that what you're proposing is indeed better though. >> Thanks, >> / magnus 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
Hi Sergei, On Sat, Mar 8, 2014 at 8:57 AM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > > On 03/07/2014 05:27 PM, 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. > > >>>> Changes in version 2: >>>> - restored devm_clk_get() call and the error handling logic in the >>>> probe() >>>> method, removed clk_put() call in the remove() method. > > >>>> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 >>>> +++++++++++ >>>> drivers/usb/phy/phy-rcar-gen2-usb.c | 42 >>>> ++++++++++++++-- >>>> 2 files changed, 68 insertions(+), 3 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. >>>> +- 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). > > >>> Thanks for your efforts here, but this DT binding looks like a mix of >>> software configuration and hardware description. > > >> I disagree. I have already explained to Mark Rutland how selecting a >> particular controller for a particular USB port should depend on the kind >> of >> the USB connector used, i.e. the board hardware. Just to clarify, I'm talking about the selection of USB IP that is happening inside the SoC. You are right that there are also board specific dependencies, but that's a different issue. As an example, on Lager it is possible to use either USB 2.0 Host device functionality or USB 3.0 Host functionality using the same USB 3.0-compatible connector that is being exposed by the Lager board. In this case the board needs to describe that it has a USB3.0-compatbile connector on r8a7790 USB2, while the r8a7790 PHY driver and USB Host controller glue needs to represent which internal devices that are mapped to USB2. I imagine that there are board-specific DT bits for Lager that describe USB2 as USB3.0 compatible. Then there are r8a7790 specific bits that show how the USB2.0 host controller and the USB3.0 host controller both point to the USB2 channel included in the USB PHY. The last portion is not modified by the board-level integrator, only the board specific bits. >>>> +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; >>>> + }; > > >>> As an example, instead of relying on "renesas,channel0-pci" or >>> "renesas,channel2-pci" to specify which hard coded software >>> configuration you want please rework this to expose 3 separate >>> channels and use DT to point each host controller to the right >>> channel. > > >> So you want to expose the channels as "virtual devices"? Or what do >> you >> want? I'm not sure what "virtual devices" would be, but channel sub nodes sounds like one possibility. > OK, let's assume you want either "virtual" PHYs or channel subnodes in > the PHY node we have now. Now what the PHY driver should do with all these > phandles pointing from the controller nodes to the PHY nodes or the channel > subnodes? Scan all device tree in search of the nodes having certain prop > and get info from such nodes on how to program the channel MUXing register > using their "compatible" props? Sorry, that doesn't appeal to me at all. I can't say that I'm 100% sure what is the best solution for this, but I'm quite OK with basically anything that describes the hardware mapping between PHY ports and host controllers in a sane way. Exactly how to represent this in a sane way is however not trivial. But how fun would that be? =) One example from my side (which may be bad, not sure) is to point from each USB Host controller to a subnode in the USB PHY device. But then you will have to scan all though devices to find the link which may not be what you want. > Now if we got the phandles pointing from the PHY node back to the > selected controllers, things would look somewhat better. Yet it still looks > like an unnecessary complication of what I'm proposing and so doesn't appeal > to me much either. So my other example is probably same as your proposal above, which would be to point all USB Host controllers to the same USB PHY, but as DT properties in the PHY device have one list per USB port that points back to all possible USB Host controllers. That would make scanning easier. > So, what's your intent here (other than to make my life harder :-)? Of course, only to make your life harder. =) My goal is to describe the hardware connections between the PHY ports and the USB Host controllers so we eventually in software would be able to switch between USBHS for Function (or maybe Host) and USB PCI for USB Host on a board where the hardware allows us to do so. I want to be able to over time improve the kernel and do this without having to modify the DTS. Actually, I'd like the DTS to show us how we can switch the hardware. This is quite different from hard coding one connection in the DTS. >> Also, it seems almost impossible to represent some USB controllers in DT >> -- such as USBHS in particular. I'm not sure if impossible is the word I would pick, but it is indeed difficult. My opinion is that we cannot rush this kind of development. It is fine to take shortcuts with platform devices, but with DT we want to get it right. > I've spent several days staring at the various USBHS code and I'm going > to send you a more detailed report on USBHS vs DT issues. Sure, if that makes you happy, but I'm more interested in patches myself. =) >>> So this is a NAK on my side. I expect better. > >> Thanks for your long awaited feedback. > > I'm not convinced that what you're proposing is indeed better though. Well, I'm looking forward to a better proposal then! =) Thanks, / magnus -- 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 03/10/2014 02:00 PM, Magnus Damm 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. >>>>> Changes in version 2: >>>>> - restored devm_clk_get() call and the error handling logic in the >>>>> probe() >>>>> method, removed clk_put() call in the remove() method. >>>>> Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 >>>>> +++++++++++ >>>>> drivers/usb/phy/phy-rcar-gen2-usb.c | 42 >>>>> ++++++++++++++-- >>>>> 2 files changed, 68 insertions(+), 3 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. >>>>> +- 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). >>>> Thanks for your efforts here, but this DT binding looks like a mix of >>>> software configuration and hardware description. >>> I disagree. I have already explained to Mark Rutland how selecting a >>> particular controller for a particular USB port should depend on the kind >>> of >>> the USB connector used, i.e. the board hardware. > Just to clarify, I'm talking about the selection of USB IP that is > happening inside the SoC. I'm talking about the same thing. > You are right that there are also board > specific dependencies, but that's a different issue. I don't really think so. > As an example, on > Lager it is possible to use either USB 2.0 Host device functionality > or USB 3.0 Host functionality using the same USB 3.0-compatible > connector that is being exposed by the Lager board. Note that USB 3.0 controller (xHCI) knows how to talk USB 2.0 and 1.1 protocols, and I see little sense enabling EHCI/OHCI for USB 3.0 port (as it's done now), unless the xHCI hardware/driver is very buggy. > In this case the > board needs to describe that it has a USB3.0-compatbile connector on Hm, I think we'll then be the first trying to describe the board connector types in the device tree, at least I can't remember any example off the top of my head. > r8a7790 USB2, while the r8a7790 PHY driver and USB Host controller > glue needs to represent which internal devices that are mapped to > USB2. > I imagine that there are board-specific DT bits for Lager that > describe USB2 as USB3.0 compatible. Then there are r8a7790 specific > bits that show how the USB2.0 host controller and the USB3.0 host > controller both point to the USB2 channel included in the USB PHY. The > last portion is not modified by the board-level integrator, only the > board specific bits. So, those "board specific bits" would be considered the ultimate source of information of how to program USB channel MUXes, right? Or you mean to only provide the USB connector info there? My main concern is where the data to program MUXes would be placed and, maybe more importantly, in what way they would be conveyed to the PHY driver, while you seem mainly concerning about the topology representation. >>>>> +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; >>>>> + }; >>>> As an example, instead of relying on "renesas,channel0-pci" or >>>> "renesas,channel2-pci" to specify which hard coded software >>>> configuration you want please rework this to expose 3 separate >>>> channels and use DT to point each host controller to the right >>>> channel. >>> So you want to expose the channels as "virtual devices"? Or what do >>> you want? > I'm not sure what "virtual devices" would be, Devices without the "reg" property. By the way, from the manuals it didn't became crystal clear to me whether USBHS internal PHY (that is programmable and controls USB0/2 channel MUX among other things) and USB 2.0/3.0 PHY (that is not programmable but USB 3.0 part of which can be used alternatively as SATA0 PHY) is even one and the same thing. The PHY registers we control thru the dedicated PHY driver are documented along with other USBHS registers, so one might have an impression this PHY is dedicated to USBHS only... If this is not so, the USB PHY design and the way it's documented seems quite questionable to me. > but channel sub nodes sounds like one possibility. OK. >> OK, let's assume you want either "virtual" PHYs or channel subnodes in >> the PHY node we have now. Now what the PHY driver should do with all these >> phandles pointing from the controller nodes to the PHY nodes or the channel >> subnodes? Scan all device tree in search of the nodes having certain prop >> and get info from such nodes on how to program the channel MUXing register >> using their "compatible" props? Sorry, that doesn't appeal to me at all. > I can't say that I'm 100% sure what is the best solution for this, but > I'm quite OK with basically anything that describes the hardware > mapping between PHY ports and host controllers in a sane way. > Exactly how to represent this in a sane way is however not trivial. > But how fun would that be? =) I'm afraid I'm not a huge fan of device trees (although I had to acquaint myself with them back in 2006, my first experience wasn't quite successful), so the fun for me seems somewhat dubious... I'm of the opinion that plain old platform devices are far more flexible than DT, and seeing DT more as a necessary evil used to bring the ARM kernel to the more reasonable sizes and follow the x86 suit with the multiplatform builds... > One example from my side (which may be bad, not sure) is to point from > each USB Host controller to a subnode in the USB PHY device. But then > you will have to scan all though devices to find the link which may > not be what you want. Yes, I would like to avoid that. >> Now if we got the phandles pointing from the PHY node back to the >> selected controllers, things would look somewhat better. Yet it still looks >> like an unnecessary complication of what I'm proposing and so doesn't appeal >> to me much either. > So my other example is probably same as your proposal above, which > would be to point all USB Host controllers to the same USB PHY, but as > DT properties in the PHY device have one list per USB port that points > back to all possible USB Host controllers. That would make scanning > easier. OK... >> So, what's your intent here (other than to make my life harder :-)? > Of course, only to make your life harder. =) Well, so far you seem to succeed. :-) > My goal is to describe the hardware connections between the PHY ports > and the USB Host controllers so we eventually in software would be > able to switch between USBHS for Function (or maybe Host) and USB PCI > for USB Host on a board where the hardware allows us to do so. I want > to be able to over time improve the kernel and do this without having > to modify the DTS. Not sure I understand what you're imagining here... > Actually, I'd like the DTS to show us how we can > switch the hardware. That's clear. I'm only not sure we really need that information in the device tree in order to switch the hardware. Oh well... > This is quite different from hard coding one connection in the DTS. Well, having such limited platform data, I had little choice but mimic it in DT as it's usually done, at least for a first try... >>> Also, it seems almost impossible to represent some USB controllers in DT >>> -- such as USBHS in particular. > I'm not sure if impossible is the word I would pick, but it is indeed > difficult. Let's agree on "very difficult". :-) > My opinion is that we cannot rush this kind of development. > It is fine to take shortcuts with platform devices, but with DT we > want to get it right. OK... but that makes the PHY DT work a matter of quite distant future, doesn't it? I mean unless we use much avoided so far auxdata thing for USBHS. >> I've spent several days staring at the various USBHS code and I'm going >> to send you a more detailed report on USBHS vs DT issues. > Sure, if that makes you happy, but I'm more interested in patches myself. =) No, it won't, so let's skip it. >>>> So this is a NAK on my side. I expect better. >>> Thanks for your long awaited feedback. >> I'm not convinced that what you're proposing is indeed better though. > Well, I'm looking forward to a better proposal then! =) Unfortunately, I'm lacking your imagination here. :-) > Thanks, > / magnus 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,6 +168,37 @@ 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; @@ -177,7 +209,10 @@ static int rcar_gen2_usb_phy_probe(struc struct clk *clk; int retval; - pdata = dev_get_platdata(dev); + if (dev->of_node) + 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; @@ -236,6 +271,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. Changes in version 2: - restored devm_clk_get() call and the error handling logic in the probe() method, removed clk_put() call in the remove() method. Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt | 29 +++++++++++ drivers/usb/phy/phy-rcar-gen2-usb.c | 42 ++++++++++++++-- 2 files changed, 68 insertions(+), 3 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