| Message ID | 1375789991-30041-3-git-send-email-iivanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2013-08-06 at 12:53 +0100, Ivan T. Ivanov wrote: > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> The same comment as for the RFC 1/2 here... > .../devicetree/bindings/usb/msm-ssusb.txt | 39 +++++ > drivers/usb/dwc3/Kconfig | 8 + > drivers/usb/dwc3/Makefile | 1 + > drivers/usb/dwc3/dwc3-msm.c | 175 ++++++++++++++++++++ > 4 files changed, 223 insertions(+) > create mode 100644 drivers/usb/dwc3/dwc3-msm.c > > diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > index 550b496..313ae0d 100644 > --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt > +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > @@ -22,6 +22,23 @@ Required "supply-name" examples are: > "v1p8" : 1.8v supply for SS-PHY > "vddcx" : vdd supply for SS-PHY digital circuit operation > > +Required properties : > +- compatible : should be "qcom,dwc-usb3-msm" > +- reg : offset and length of the register set in the memory map > + offset and length of the TCSR register for routing USB > + signals to either picoPHY0 or picoPHY1. > +- clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; > +- clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; > + > +Optional properties : > +- gdsc-supply : phandle to the globally distributed switch controller > + regulator node to the USB controller. > + > +Sub nodes: > +- Sub node for "DWC3- USB3 controller". > + This sub node is required property for device node. The properties of this subnode > + are specified in dwc3.txt. Ah, this answers one of my questions - DWC3 comes from Synopsys. > Example device nodes: > > dwc3_usb2: phy@f92f8800 { > @@ -47,3 +64,25 @@ Example device nodes: > vddcx-supply = <&supply>; > v1p8-supply = <&supply>; > }; > + > + usb@fd4ab000 { > + compatible = "qcom,dwc-usb3-msm"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0xfd4ab000 0x4>; > + > + clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; > + clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; > + > + gdsc-supply = <&supply>; > + ranges; > + > + dwc3@f9200000 { > + compatible = "snps,dwc3"; Note that the Documentation/devicetree/bindings/usb/dwc3.txt is mentioning "synopsys,dwc3" (which is clearly in opposition to the vendor-prefixes.txt file) and this compatible value is used in the drivers/usb/dwc3/core.c and omap5.dtsi. Unless it has been already fixed recently, could you take care of that? > + reg = <0xf9200000 0xcd00>; > + interrupts = <0 131 0>; > + interrupt-names = "irq"; > + usb-phy = <&dwc3_usb2>, <&dwc3_usb3>; > + tx-fifo-resize; > + }; > + }; And now I'm really confused... Maybe it's just lack of documentation... How does the "qcom,dwc-usb3-msm" relate to "qcom,dwc-usb3"? Pawe? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-08-06 at 13:21 +0100, Pawel Moll wrote: > On Tue, 2013-08-06 at 12:53 +0100, Ivan T. Ivanov wrote: > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > The same comment as for the RFC 1/2 here... Will fix this. > > > .../devicetree/bindings/usb/msm-ssusb.txt | 39 +++++ > > drivers/usb/dwc3/Kconfig | 8 + > > drivers/usb/dwc3/Makefile | 1 + > > drivers/usb/dwc3/dwc3-msm.c | 175 ++++++++++++++++++++ > > 4 files changed, 223 insertions(+) > > create mode 100644 drivers/usb/dwc3/dwc3-msm.c > > > > diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > > index 550b496..313ae0d 100644 > > --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt > > +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > > @@ -22,6 +22,23 @@ Required "supply-name" examples are: > > "v1p8" : 1.8v supply for SS-PHY > > "vddcx" : vdd supply for SS-PHY digital circuit operation > > > > +Required properties : > > +- compatible : should be "qcom,dwc-usb3-msm" > > +- reg : offset and length of the register set in the memory map > > + offset and length of the TCSR register for routing USB > > + signals to either picoPHY0 or picoPHY1. > > +- clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; > > +- clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; > > + > > +Optional properties : > > +- gdsc-supply : phandle to the globally distributed switch controller > > + regulator node to the USB controller. > > + > > +Sub nodes: > > +- Sub node for "DWC3- USB3 controller". > > + This sub node is required property for device node. The properties of this subnode > > + are specified in dwc3.txt. > > Ah, this answers one of my questions - DWC3 comes from Synopsys. Yes, sorry. > > > Example device nodes: > > > > dwc3_usb2: phy@f92f8800 { > > @@ -47,3 +64,25 @@ Example device nodes: > > vddcx-supply = <&supply>; > > v1p8-supply = <&supply>; > > }; > > + > > + usb@fd4ab000 { > > + compatible = "qcom,dwc-usb3-msm"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + reg = <0xfd4ab000 0x4>; > > + > > + clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; > > + clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; > > + > > + gdsc-supply = <&supply>; > > + ranges; > > + > > + dwc3@f9200000 { > > + compatible = "snps,dwc3"; > > Note that the Documentation/devicetree/bindings/usb/dwc3.txt is > mentioning "synopsys,dwc3" (which is clearly in opposition to the > vendor-prefixes.txt file) and this compatible value is used in the > drivers/usb/dwc3/core.c and omap5.dtsi. Unless it has been already fixed > recently, could you take care of that? Yes, it is fixed already. Patch is in linux-next "usb: dwc3: core: switch to snps,dwc3" > > > + reg = <0xf9200000 0xcd00>; > > + interrupts = <0 131 0>; > > + interrupt-names = "irq"; > > + usb-phy = <&dwc3_usb2>, <&dwc3_usb3>; > > + tx-fifo-resize; > > + }; > > + }; > > And now I'm really confused... Maybe it's just lack of documentation... > > How does the "qcom,dwc-usb3-msm" relate to "qcom,dwc-usb3"? Not sure from where you get this "qcom,dwc-usb3", but now I think that "qcom,dwc-usb3" should be enough for compatible. Thanks, Ivan > > Pawe? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 06, 2013 at 12:53:11PM +0100, Ivan T. Ivanov wrote: > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> What does the "glue layer" do? Is it an actual piece of hardware, or just some platform-specific code? > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > --- > .../devicetree/bindings/usb/msm-ssusb.txt | 39 +++++ > drivers/usb/dwc3/Kconfig | 8 + > drivers/usb/dwc3/Makefile | 1 + > drivers/usb/dwc3/dwc3-msm.c | 175 ++++++++++++++++++++ > 4 files changed, 223 insertions(+) > create mode 100644 drivers/usb/dwc3/dwc3-msm.c > > diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > index 550b496..313ae0d 100644 > --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt > +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > @@ -22,6 +22,23 @@ Required "supply-name" examples are: > "v1p8" : 1.8v supply for SS-PHY > "vddcx" : vdd supply for SS-PHY digital circuit operation > > +Required properties : > +- compatible : should be "qcom,dwc-usb3-msm" > +- reg : offset and length of the register set in the memory map > + offset and length of the TCSR register for routing USB > + signals to either picoPHY0 or picoPHY1. > +- clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; Similarly to my comment on patch 1, these need to be described better. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, 2013-08-06 at 15:07 +0100, Mark Rutland wrote: > On Tue, Aug 06, 2013 at 12:53:11PM +0100, Ivan T. Ivanov wrote: > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > What does the "glue layer" do? Is it an actual piece of hardware, or > just some platform-specific code? > It is hardware layer around Synopsys DesignWare USB3 core. The term 'glue layer' is what is used in TI OMAP's and Samsung Exynos drivers implementations. > > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > > --- > > .../devicetree/bindings/usb/msm-ssusb.txt | 39 +++++ > > drivers/usb/dwc3/Kconfig | 8 + > > drivers/usb/dwc3/Makefile | 1 + > > drivers/usb/dwc3/dwc3-msm.c | 175 ++++++++++++++++++++ > > 4 files changed, 223 insertions(+) > > create mode 100644 drivers/usb/dwc3/dwc3-msm.c > > > > diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > > index 550b496..313ae0d 100644 > > --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt > > +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt > > @@ -22,6 +22,23 @@ Required "supply-name" examples are: > > "v1p8" : 1.8v supply for SS-PHY > > "vddcx" : vdd supply for SS-PHY digital circuit operation > > > > +Required properties : > > +- compatible : should be "qcom,dwc-usb3-msm" > > +- reg : offset and length of the register set in the memory map > > + offset and length of the TCSR register for routing USB > > + signals to either picoPHY0 or picoPHY1. > > +- clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; > > Similarly to my comment on patch 1, these need to be described better. Sure, will fix this. Thanks, Ivan > > Thanks, > Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-08-06 at 14:46 +0100, Ivan T. Ivanov wrote: > > > + reg = <0xf9200000 0xcd00>; > > > + interrupts = <0 131 0>; > > > + interrupt-names = "irq"; > > > + usb-phy = <&dwc3_usb2>, <&dwc3_usb3>; > > > + tx-fifo-resize; > > > + }; > > > + }; > > > > And now I'm really confused... Maybe it's just lack of documentation... > > > > How does the "qcom,dwc-usb3-msm" relate to "qcom,dwc-usb3"? > > Not sure from where you get this "qcom,dwc-usb3", but now I think > that "qcom,dwc-usb3" should be enough for compatible. The other patch introduces "qcom,dwc3-usb3" compatible value... Pawe? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-08-06 at 16:15 +0100, Pawel Moll wrote: > On Tue, 2013-08-06 at 14:46 +0100, Ivan T. Ivanov wrote: > > > > + reg = <0xf9200000 0xcd00>; > > > > + interrupts = <0 131 0>; > > > > + interrupt-names = "irq"; > > > > + usb-phy = <&dwc3_usb2>, <&dwc3_usb3>; > > > > + tx-fifo-resize; > > > > + }; > > > > + }; > > > > > > And now I'm really confused... Maybe it's just lack of documentation... > > > > > > How does the "qcom,dwc-usb3-msm" relate to "qcom,dwc-usb3"? > > > > Not sure from where you get this "qcom,dwc-usb3", but now I think > > that "qcom,dwc-usb3" should be enough for compatible. > > The other patch introduces "qcom,dwc3-usb3" compatible value... Oh, of course. Intention was that "qcom,dwc-usb3" will handle SS-PHY, while "qcom,dwc-usb2" will handle HS-PHY. Probably it will better if I rename them to "qcom,dwc-ssphy" and "qcom,dwc-hsphy". Regards, Ivan > > Pawe? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Aug 06, 2013 at 02:53:11PM +0300, Ivan T. Ivanov wrote: > diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c > new file mode 100644 > index 0000000..e509abc > --- /dev/null > +++ b/drivers/usb/dwc3/dwc3-msm.c > @@ -0,0 +1,175 @@ > +#undef CONFIG_REGULATOR why ?????? > +static int dwc3_msm_probe(struct platform_device *pdev) > +{ > + struct device_node *node = pdev->dev.of_node; > + struct dwc3_msm *mdwc; > + struct resource *res; > + void __iomem *tcsr; > + int ret = 0; > + > + dev_info(&pdev->dev, "MSM DWC3\n"); please don't. This is hardly necessary. > + mdwc = devm_kzalloc(&pdev->dev, sizeof(*mdwc), GFP_KERNEL); > + if (!mdwc) { > + dev_err(&pdev->dev, "not enough memory\n"); this message isn't needed either > + /* > + * DWC3 Core requires its CORE CLK (aka master / bus clk) to > + * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode. > + */ > + clk_set_rate(mdwc->core_clk, 125000000); if this is dwc3's core clock, why don't we teach dwc3.ko about this requirement ? Just make sure to have it optional, since x86 and OMAP wouldn't need direct fiddling with the clocks. > + clk_prepare_enable(mdwc->core_clk); > + clk_prepare_enable(mdwc->iface_clk); > + clk_prepare_enable(mdwc->sleep_clk); > + clk_prepare_enable(mdwc->utmi_clk); do you really need to enable your clocks here ? Why don't you enable them on runtime_resume and disable on runtime_suspend ? > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + tcsr = devm_ioremap_resource(&pdev->dev, res); > + if (!tcsr) { > + dev_dbg(&pdev->dev, "tcsr ioremap failed\n"); no need to ioremap, also you're likely leaking clocks and regulators enabled here. Make sure to have something like: if (!tcsr) goto err_disable_clocks; /* TODO This has to be revised */\ [...] > + } else { > + /* TODO: This has to be revised */ > + > + /* Enable USB3 on the primary USB port. */ > + writel_relaxed(0x1, tcsr); > + /* > + * Ensure that TCSR write is completed before > + * USB registers initialization. > + */ > + mb(); why don't you use writel() instead. It will add the memory barrier for you.
Hi, On Tue, Aug 06, 2013 at 01:21:38PM +0100, Pawel Moll wrote: > > @@ -47,3 +64,25 @@ Example device nodes: > > vddcx-supply = <&supply>; > > v1p8-supply = <&supply>; > > }; > > + > > + usb@fd4ab000 { > > + compatible = "qcom,dwc-usb3-msm"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + reg = <0xfd4ab000 0x4>; > > + > > + clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; > > + clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; > > + > > + gdsc-supply = <&supply>; > > + ranges; > > + > > + dwc3@f9200000 { > > + compatible = "snps,dwc3"; > > Note that the Documentation/devicetree/bindings/usb/dwc3.txt is > mentioning "synopsys,dwc3" (which is clearly in opposition to the I have a patch converting to snps,dwc3.
Hi, On Fri, 2013-08-09 at 16:23 +0300, Felipe Balbi wrote: > Hi, > > On Tue, Aug 06, 2013 at 02:53:11PM +0300, Ivan T. Ivanov wrote: > > diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c > > new file mode 100644 > > index 0000000..e509abc > > --- /dev/null > > +++ b/drivers/usb/dwc3/dwc3-msm.c > > @@ -0,0 +1,175 @@ > > +#undef CONFIG_REGULATOR > > why ?????? > 1. This shows that driver is still not fully tested Regulators support is still missing in the MSM 2. Helps me to load driver successfully. > > +static int dwc3_msm_probe(struct platform_device *pdev) > > +{ > > + struct device_node *node = pdev->dev.of_node; > > + struct dwc3_msm *mdwc; > > + struct resource *res; > > + void __iomem *tcsr; > > + int ret = 0; > > + > > + dev_info(&pdev->dev, "MSM DWC3\n"); > > please don't. This is hardly necessary. Sure, this will be removed. > > > + mdwc = devm_kzalloc(&pdev->dev, sizeof(*mdwc), GFP_KERNEL); > > + if (!mdwc) { > > + dev_err(&pdev->dev, "not enough memory\n"); > > this message isn't needed either ok. > > > + /* > > + * DWC3 Core requires its CORE CLK (aka master / bus clk) to > > + * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode. > > + */ > > + clk_set_rate(mdwc->core_clk, 125000000); > > if this is dwc3's core clock, why don't we teach dwc3.ko about this > requirement ? Just make sure to have it optional, since x86 and OMAP > wouldn't need direct fiddling with the clocks. I have to check whether DWC3 core or Qcom wrapper requires this clock. > > > + clk_prepare_enable(mdwc->core_clk); > > + clk_prepare_enable(mdwc->iface_clk); > > + clk_prepare_enable(mdwc->sleep_clk); > > + clk_prepare_enable(mdwc->utmi_clk); > > do you really need to enable your clocks here ? Why don't you enable > them on runtime_resume and disable on runtime_suspend ? I will like to make it working first and then will improve power management. > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + tcsr = devm_ioremap_resource(&pdev->dev, res); > > + if (!tcsr) { > > + dev_dbg(&pdev->dev, "tcsr ioremap failed\n"); > > no need to ioremap, also you're likely leaking clocks and regulators > enabled here. > > Make sure to have something like: > > if (!tcsr) > goto err_disable_clocks; > > /* TODO This has to be revised */\ > > [...] > Sure. > > + } else { > > + /* TODO: This has to be revised */ > > + > > + /* Enable USB3 on the primary USB port. */ > > + writel_relaxed(0x1, tcsr); > > + /* > > + * Ensure that TCSR write is completed before > > + * USB registers initialization. > > + */ > > + mb(); > > why don't you use writel() instead. It will add the memory barrier for > you. Ok. Thanks Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 09, 2013 at 07:09:18PM +0300, Ivan T. Ivanov wrote: > Hi, > > On Fri, 2013-08-09 at 16:23 +0300, Felipe Balbi wrote: > > Hi, > > > > On Tue, Aug 06, 2013 at 02:53:11PM +0300, Ivan T. Ivanov wrote: > > > diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c > > > new file mode 100644 > > > index 0000000..e509abc > > > --- /dev/null > > > +++ b/drivers/usb/dwc3/dwc3-msm.c > > > @@ -0,0 +1,175 @@ > > > +#undef CONFIG_REGULATOR > > > > why ?????? > > > > 1. This shows that driver is still not fully tested > Regulators support is still missing in the MSM > 2. Helps me to load driver successfully. Then remove all the regulator-related code from this. > > > + clk_prepare_enable(mdwc->core_clk); > > > + clk_prepare_enable(mdwc->iface_clk); > > > + clk_prepare_enable(mdwc->sleep_clk); > > > + clk_prepare_enable(mdwc->utmi_clk); > > > > do you really need to enable your clocks here ? Why don't you enable > > them on runtime_resume and disable on runtime_suspend ? > > I will like to make it working first and then will improve > power management. alright, makes sense. > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + tcsr = devm_ioremap_resource(&pdev->dev, res); > > > + if (!tcsr) { > > > + dev_dbg(&pdev->dev, "tcsr ioremap failed\n"); > > > > no need to ioremap, also you're likely leaking clocks and regulators > > enabled here. > > > > Make sure to have something like: > > > > if (!tcsr) > > goto err_disable_clocks; > > > > /* TODO This has to be revised */\ > > > > [...] > > > > Sure. just to make it clear, I meant to say that you don't need to print the error message :-)
Hi, On Mon, 2013-08-12 at 13:24 -0500, Felipe Balbi wrote: > On Fri, Aug 09, 2013 at 07:09:18PM +0300, Ivan T. Ivanov wrote: > > Hi, > > > > On Fri, 2013-08-09 at 16:23 +0300, Felipe Balbi wrote: > > > Hi, > > > > > > On Tue, Aug 06, 2013 at 02:53:11PM +0300, Ivan T. Ivanov wrote: > > > > diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c > > > > new file mode 100644 > > > > index 0000000..e509abc > > > > --- /dev/null > > > > +++ b/drivers/usb/dwc3/dwc3-msm.c > > > > @@ -0,0 +1,175 @@ > > > > +#undef CONFIG_REGULATOR > > > > > > why ?????? > > > > > > > 1. This shows that driver is still not fully tested > > Regulators support is still missing in the MSM > > 2. Helps me to load driver successfully. > > Then remove all the regulator-related code from this. I would like to keep them. I will just remove #undef line. Code will continue to build fine. And once regulators drivers are upstreamed this driver 'will not' require further modifications. > > > > > + clk_prepare_enable(mdwc->core_clk); > > > > + clk_prepare_enable(mdwc->iface_clk); > > > > + clk_prepare_enable(mdwc->sleep_clk); > > > > + clk_prepare_enable(mdwc->utmi_clk); > > > > > > do you really need to enable your clocks here ? Why don't you enable > > > them on runtime_resume and disable on runtime_suspend ? > > > > I will like to make it working first and then will improve > > power management. > > alright, makes sense. > > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + tcsr = devm_ioremap_resource(&pdev->dev, res); > > > > + if (!tcsr) { > > > > + dev_dbg(&pdev->dev, "tcsr ioremap failed\n"); > > > > > > no need to ioremap, also you're likely leaking clocks and regulators > > > enabled here. > > > > > > Make sure to have something like: > > > > > > if (!tcsr) > > > goto err_disable_clocks; > > > > > > /* TODO This has to be revised */\ > > > > > > [...] > > > > > > > Sure. > > just to make it clear, I meant to say that you don't need to print the > error message :-) Yes. I got it. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, 2013-08-09 at 16:23 +0300, Felipe Balbi wrote: <snip> > > > + /* > > + * DWC3 Core requires its CORE CLK (aka master / bus clk) to > > + * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode. > > + */ > > + clk_set_rate(mdwc->core_clk, 125000000); > > if this is dwc3's core clock, why don't we teach dwc3.ko about this > requirement ? Just make sure to have it optional, since x86 and OMAP > wouldn't need direct fiddling with the clocks. I believe this is Qualcomm specific requirement. Something is modified inside DWC in respect of the clocks. I will like to keep this here, in the glue layer driver. Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 14, 2013 at 12:28:17PM +0300, Ivan T. Ivanov wrote: > > Hi, > > On Mon, 2013-08-12 at 13:24 -0500, Felipe Balbi wrote: > > On Fri, Aug 09, 2013 at 07:09:18PM +0300, Ivan T. Ivanov wrote: > > > Hi, > > > > > > On Fri, 2013-08-09 at 16:23 +0300, Felipe Balbi wrote: > > > > Hi, > > > > > > > > On Tue, Aug 06, 2013 at 02:53:11PM +0300, Ivan T. Ivanov wrote: > > > > > diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c > > > > > new file mode 100644 > > > > > index 0000000..e509abc > > > > > --- /dev/null > > > > > +++ b/drivers/usb/dwc3/dwc3-msm.c > > > > > @@ -0,0 +1,175 @@ > > > > > +#undef CONFIG_REGULATOR > > > > > > > > why ?????? > > > > > > > > > > 1. This shows that driver is still not fully tested > > > Regulators support is still missing in the MSM > > > 2. Helps me to load driver successfully. > > > > Then remove all the regulator-related code from this. > > I would like to keep them. I will just remove #undef line. > Code will continue to build fine. And once regulators drivers > are upstreamed this driver 'will not' require further > modifications. fair enough.
diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt index 550b496..313ae0d 100644 --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt @@ -22,6 +22,23 @@ Required "supply-name" examples are: "v1p8" : 1.8v supply for SS-PHY "vddcx" : vdd supply for SS-PHY digital circuit operation +Required properties : +- compatible : should be "qcom,dwc-usb3-msm" +- reg : offset and length of the register set in the memory map + offset and length of the TCSR register for routing USB + signals to either picoPHY0 or picoPHY1. +- clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; +- clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; + +Optional properties : +- gdsc-supply : phandle to the globally distributed switch controller + regulator node to the USB controller. + +Sub nodes: +- Sub node for "DWC3- USB3 controller". + This sub node is required property for device node. The properties of this subnode + are specified in dwc3.txt. + Example device nodes: dwc3_usb2: phy@f92f8800 { @@ -47,3 +64,25 @@ Example device nodes: vddcx-supply = <&supply>; v1p8-supply = <&supply>; }; + + usb@fd4ab000 { + compatible = "qcom,dwc-usb3-msm"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0xfd4ab000 0x4>; + + clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>, <&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>; + clock-names = "core_clk", "iface_clk", "sleep_clk", "utmi_clk"; + + gdsc-supply = <&supply>; + ranges; + + dwc3@f9200000 { + compatible = "snps,dwc3"; + reg = <0xf9200000 0xcd00>; + interrupts = <0 131 0>; + interrupt-names = "irq"; + usb-phy = <&dwc3_usb2>, <&dwc3_usb3>; + tx-fifo-resize; + }; + }; diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 3e225d5..e2032c4 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -70,6 +70,14 @@ config USB_DWC3_PCI One such PCIe-based platform is Synopsys' PCIe HAPS model of this IP. +config USB_DWC3_MSM + tristate "Qualcomm MSM/APQ Platforms" + default USB_DWC3 + select USB_MSM_DWC3_PHYS + help + Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside, + say 'Y' or 'M' if you have one such device. + comment "Debugging features" config USB_DWC3_DEBUG diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile index dd17601..5226681 100644 --- a/drivers/usb/dwc3/Makefile +++ b/drivers/usb/dwc3/Makefile @@ -32,3 +32,4 @@ endif obj-$(CONFIG_USB_DWC3_OMAP) += dwc3-omap.o obj-$(CONFIG_USB_DWC3_EXYNOS) += dwc3-exynos.o obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o +obj-$(CONFIG_USB_DWC3_MSM) += dwc3-msm.o diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c new file mode 100644 index 0000000..e509abc --- /dev/null +++ b/drivers/usb/dwc3/dwc3-msm.c @@ -0,0 +1,175 @@ +/* Copyright (c) 2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#undef CONFIG_REGULATOR + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/usb/phy.h> + +struct dwc3_msm { + struct device *dev; + void __iomem *base; + + struct clk *core_clk; + struct clk *iface_clk; + struct clk *sleep_clk; + struct clk *utmi_clk; + + struct regulator *gdsc; +}; + +static int dwc3_msm_probe(struct platform_device *pdev) +{ + struct device_node *node = pdev->dev.of_node; + struct dwc3_msm *mdwc; + struct resource *res; + void __iomem *tcsr; + int ret = 0; + + dev_info(&pdev->dev, "MSM DWC3\n"); + + mdwc = devm_kzalloc(&pdev->dev, sizeof(*mdwc), GFP_KERNEL); + if (!mdwc) { + dev_err(&pdev->dev, "not enough memory\n"); + return -ENOMEM; + } + + platform_set_drvdata(pdev, mdwc); + mdwc->dev = &pdev->dev; + + mdwc->gdsc = devm_regulator_get(mdwc->dev, "gbsc"); + + mdwc->core_clk = devm_clk_get(&pdev->dev, "core_clk"); + if (IS_ERR(mdwc->core_clk)) { + dev_err(&pdev->dev, "failed to get core_clk\n"); + return PTR_ERR(mdwc->core_clk); + } + + mdwc->iface_clk = devm_clk_get(&pdev->dev, "iface_clk"); + if (IS_ERR(mdwc->iface_clk)) { + dev_err(&pdev->dev, "failed to get iface_clk\n"); + return PTR_ERR(mdwc->iface_clk); + } + + mdwc->sleep_clk = devm_clk_get(&pdev->dev, "sleep_clk"); + if (IS_ERR(mdwc->sleep_clk)) { + dev_err(&pdev->dev, "failed to get sleep_clk\n"); + return PTR_ERR(mdwc->sleep_clk); + } + + mdwc->utmi_clk = devm_clk_get(&pdev->dev, "utmi_clk"); + if (IS_ERR(mdwc->utmi_clk)) { + dev_err(&pdev->dev, "failed to get utmi_clk\n"); + return PTR_ERR(mdwc->utmi_clk); + } + + if (!IS_ERR(mdwc->gdsc)) { + ret = regulator_enable(mdwc->gdsc); + if (ret) + dev_err(mdwc->dev, "unable to enable usb3 gdsc\n"); + } + + /* + * DWC3 Core requires its CORE CLK (aka master / bus clk) to + * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode. + */ + clk_set_rate(mdwc->core_clk, 125000000); + clk_prepare_enable(mdwc->core_clk); + clk_prepare_enable(mdwc->iface_clk); + clk_prepare_enable(mdwc->sleep_clk); + clk_prepare_enable(mdwc->utmi_clk); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + tcsr = devm_ioremap_resource(&pdev->dev, res); + if (!tcsr) { + dev_dbg(&pdev->dev, "tcsr ioremap failed\n"); + } else { + /* TODO: This has to be revised */ + + /* Enable USB3 on the primary USB port. */ + writel_relaxed(0x1, tcsr); + /* + * Ensure that TCSR write is completed before + * USB registers initialization. + */ + mb(); + } + + ret = of_platform_populate(node, NULL, NULL, &pdev->dev); + if (ret) { + dev_err(&pdev->dev, "failed to add create dwc3 core\n"); + goto dis_clks; + } + + return 0; + +dis_clks: + clk_disable_unprepare(mdwc->utmi_clk); + clk_disable_unprepare(mdwc->sleep_clk); + clk_disable_unprepare(mdwc->iface_clk); + clk_disable_unprepare(mdwc->core_clk); + if (!IS_ERR(mdwc->gdsc)) { + ret = regulator_disable(mdwc->gdsc); + if (ret) + dev_err(mdwc->dev, "cannot disable usb3 gdsc\n"); + } + + return ret; +} + +static int dwc3_msm_remove(struct platform_device *pdev) +{ + struct dwc3_msm *mdwc = platform_get_drvdata(pdev); + int ret; + + clk_disable_unprepare(mdwc->utmi_clk); + clk_disable_unprepare(mdwc->sleep_clk); + clk_disable_unprepare(mdwc->iface_clk); + clk_disable_unprepare(mdwc->core_clk); + + if (!IS_ERR(mdwc->gdsc)) { + ret = regulator_disable(mdwc->gdsc); + if (ret) + dev_err(mdwc->dev, "cannot disable usb3 gdsc\n"); + } + + return 0; +} + +static const struct of_device_id of_dwc3_matach[] = { + { + .compatible = "qcom,dwc-usb3-msm", + }, + { }, +}; +MODULE_DEVICE_TABLE(of, of_dwc3_matach); + +static struct platform_driver dwc3_msm_driver = { + .probe = dwc3_msm_probe, + .remove = dwc3_msm_remove, + .driver = { + .name = "msm-dwc3", + .owner = THIS_MODULE, + .of_match_table = of_dwc3_matach, + }, +}; + +module_platform_driver(dwc3_msm_driver); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("DesignWare USB3 MSM Glue Layer");