| Message ID | 1404144233-17222-4-git-send-email-agross@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Jun 30, 2014, at 11:03 AM, Andy Gross <agross@codeaurora.org> wrote: > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > QCOM USB3.0 core wrapper consist of USB3.0 IP from Synopsys > (SNPS) and HS, SS PHY's control and configuration registers. > > It could operate in device mode (SS, HS, FS) and host > mode (SS, HS, FS, LS). > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > Signed-off-by: Andy Gross <agross@codeaurora.org> > --- > .../devicetree/bindings/usb/qcom,dwc3.txt | 104 ++++++++++++++++++++ > 1 file changed, 104 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,dwc3.txt > > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > new file mode 100644 > index 0000000..105b6b7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > @@ -0,0 +1,104 @@ > +Qualcomm SuperSpeed DWC3 USB SoC controller > + > + > +QCOM DWC3 Highspeed USB PHY > +======================== > +Required properities: > +- compatible: should contain "qcom,dwc3-hsphy"; > +- reg: offset and length of the register set in the memory map > +- clocks: A list of phandle + clock-specifier pairs for the > + clocks listed in clock-names > +- clock-names: Should contain the following: > + "utmi" UTMI clock > +- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. > +- v3p3-supply: phandle to the regulator for the 3.3v supply to HSPHY. > +- vbus-supply: phandle to the regulator for the vbus supply for host > + mode. > +- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY > + digital circuit operation. > + > +Optional clocks: > + "xo" External reference clock > + > + > +QCOM DWC3 Superspeed USB PHY > +========================= > +Required properities: > +- compatible: should contain "qcom,dwc3-ssphy"; > +- reg: offset and length of the register set in the memory map > +- clocks: A list of phandle + clock-specifier pairs for the > + clocks listed in clock-names > +- clock-names: Should contain the following: > + "ref" Reference clock used in host mode. > +- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. > +- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY > + digital circuit operation. > + > +Optional clocks: > + "xo" External reference clock > + > +QCOM DWC3 controller wrapper > +=========================== > +Required properties: > +- compatible: should contain "qcom,dwc3" > +- clocks: A list of phandle + clock-specifier pairs for the > + clocks listed in clock-names > +- clock-names: Should contain the following: > + "core" Master/Core clock, have to be >= 125 MHz for SS > + operation and >= 60MHz for HS operation > + > +Optional clocks: > + "iface" System bus AXI clock. Not present on all platforms > + "sleep" Sleep clock, used when USB3 core goes into low > + power mode (U3). We should encode the an optional port number here in the cases that we have multiple controllers and need to know about it so we can set TCSR correctly. > + > +Optional regulator: > +- gdsc-supply: phandle to the regulator from globally distributed > + switch controller > + > +Required child node: > +A child node must exist to represent the core DWC3 IP block. The name of > +the node is not important. The content of the node is defined in dwc3.txt. > + > +Example device nodes: > + > + hs_phy_0: phy@110f8800 { > + compatible = "qcom,dwc3-hsphy"; > + reg = <0x110f8800 0x30>; > + clocks = <&gcc USB30_0_UTMI_CLK>; > + clock-names = "utmi"; > + > + status = "disabled"; > + }; > + > + ss_phy_0: phy@110f8830 { > + compatible = "qcom,dwc3-ssphy"; > + reg = <0x110f8830 0x30>; > + > + clocks = <&gcc USB30_0_MASTER_CLK>; > + clock-names = "ref"; > + > + status = "disabled"; > + }; > + > + usb3_0: usb30@0 { > + compatible = "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + clocks = <&gcc USB30_0_MASTER_CLK>; > + clock-names = "core"; > + > + ranges; > + > + status = "disabled"; > + > + dwc3@11000000 { > + compatible = "snps,dwc3"; > + reg = <0x11000000 0xcd00>; > + interrupts = <0 110 0x4>; > + usb-phy = <&hs_phy_0>, <&ss_phy_0>; > + phy-names = "usb2-phy", "usb3-phy"; > + tx-fifo-resize; > + dr_mode = "host"; > + }; > + }; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 30, 2014 at 11:03 AM, Andy Gross <agross@codeaurora.org> wrote: > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> Please copy the right lists and maintainers. > > QCOM USB3.0 core wrapper consist of USB3.0 IP from Synopsys > (SNPS) and HS, SS PHY's control and configuration registers. > > It could operate in device mode (SS, HS, FS) and host > mode (SS, HS, FS, LS). > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > Signed-off-by: Andy Gross <agross@codeaurora.org> > --- > .../devicetree/bindings/usb/qcom,dwc3.txt | 104 ++++++++++++++++++++ > 1 file changed, 104 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,dwc3.txt > > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > new file mode 100644 > index 0000000..105b6b7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > @@ -0,0 +1,104 @@ > +Qualcomm SuperSpeed DWC3 USB SoC controller > + > + > +QCOM DWC3 Highspeed USB PHY > +======================== > +Required properities: > +- compatible: should contain "qcom,dwc3-hsphy"; > +- reg: offset and length of the register set in the memory map > +- clocks: A list of phandle + clock-specifier pairs for the > + clocks listed in clock-names > +- clock-names: Should contain the following: > + "utmi" UTMI clock > +- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. > +- v3p3-supply: phandle to the regulator for the 3.3v supply to HSPHY. > +- vbus-supply: phandle to the regulator for the vbus supply for host > + mode. > +- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY > + digital circuit operation. > + > +Optional clocks: > + "xo" External reference clock > + > + > +QCOM DWC3 Superspeed USB PHY > +========================= > +Required properities: > +- compatible: should contain "qcom,dwc3-ssphy"; > +- reg: offset and length of the register set in the memory map > +- clocks: A list of phandle + clock-specifier pairs for the > + clocks listed in clock-names > +- clock-names: Should contain the following: > + "ref" Reference clock used in host mode. > +- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. > +- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY > + digital circuit operation. > + > +Optional clocks: > + "xo" External reference clock > + > +QCOM DWC3 controller wrapper > +=========================== > +Required properties: > +- compatible: should contain "qcom,dwc3" > +- clocks: A list of phandle + clock-specifier pairs for the > + clocks listed in clock-names > +- clock-names: Should contain the following: > + "core" Master/Core clock, have to be >= 125 MHz for SS > + operation and >= 60MHz for HS operation > + > +Optional clocks: > + "iface" System bus AXI clock. Not present on all platforms Really?, some platforms have a clockless bus? > + "sleep" Sleep clock, used when USB3 core goes into low > + power mode (U3). > + > +Optional regulator: > +- gdsc-supply: phandle to the regulator from globally distributed > + switch controller The name should reflect the name of the input, not the source. > + > +Required child node: > +A child node must exist to represent the core DWC3 IP block. The name of > +the node is not important. The content of the node is defined in dwc3.txt. > + > +Example device nodes: > + > + hs_phy_0: phy@110f8800 { > + compatible = "qcom,dwc3-hsphy"; > + reg = <0x110f8800 0x30>; > + clocks = <&gcc USB30_0_UTMI_CLK>; > + clock-names = "utmi"; > + > + status = "disabled"; > + }; > + > + ss_phy_0: phy@110f8830 { > + compatible = "qcom,dwc3-ssphy"; > + reg = <0x110f8830 0x30>; > + > + clocks = <&gcc USB30_0_MASTER_CLK>; > + clock-names = "ref"; > + > + status = "disabled"; > + }; > + > + usb3_0: usb30@0 { > + compatible = "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + clocks = <&gcc USB30_0_MASTER_CLK>; > + clock-names = "core"; > + > + ranges; > + > + status = "disabled"; > + > + dwc3@11000000 { > + compatible = "snps,dwc3"; This sub-node is just wrong. Why can't you have a single node with ' "qcom,dwc3", "snps,dwc3" ' for the compatible property? All you are adding here is clocks. Does the Synopsys block have no clocks? I guess this is copied from other broken dwc3 bindings... That doesn't mean you have to copy it. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 01, 2014 at 12:04:35AM -0500, Rob Herring wrote: <snip> > > +- clock-names: Should contain the following: > > + "core" Master/Core clock, have to be >= 125 MHz for SS > > + operation and >= 60MHz for HS operation > > + > > +Optional clocks: > > + "iface" System bus AXI clock. Not present on all platforms > > Really?, some platforms have a clockless bus? Some platforms require core and interface. The specific platform I tested on does not have an iface clk. I'll take a look at the ipq block diagram to see if they did something cute, but i don't believe there is one. > > > + "sleep" Sleep clock, used when USB3 core goes into low > > + power mode (U3). > > + > > +Optional regulator: > > +- gdsc-supply: phandle to the regulator from globally distributed > > + switch controller > > The name should reflect the name of the input, not the source. Ok, I'll revisit this. I took this from the original patch set. <snip> > > + > > + ranges; > > + > > + status = "disabled"; > > + > > + dwc3@11000000 { > > + compatible = "snps,dwc3"; > > This sub-node is just wrong. Why can't you have a single node with ' > "qcom,dwc3", "snps,dwc3" ' for the compatible property? All you are > adding here is clocks. Does the Synopsys block have no clocks? > > I guess this is copied from other broken dwc3 bindings... That doesn't > mean you have to copy it. The dwc3 core does not deal with clocks. That is why everyone has a wrapper. That, in addition to pm, has to be handled from the wrapper. That's my take anyway. I am sure Felipe can speak more to this.
On Tue, Jul 1, 2014 at 1:01 PM, Andy Gross <agross@codeaurora.org> wrote: > On Tue, Jul 01, 2014 at 12:04:35AM -0500, Rob Herring wrote: > > <snip> > >> > +- clock-names: Should contain the following: >> > + "core" Master/Core clock, have to be >= 125 MHz for SS >> > + operation and >= 60MHz for HS operation >> > + >> > +Optional clocks: >> > + "iface" System bus AXI clock. Not present on all platforms >> >> Really?, some platforms have a clockless bus? > > Some platforms require core and interface. The specific platform I tested on > does not have an iface clk. I'll take a look at the ipq block diagram to see if > they did something cute, but i don't believe there is one. Usually, that difference just means all the clock inputs are connected to the same clock source. But the binding should describe the inputs. If the dwc3 core has 3 clocks and your wrapper logic varies, I would say you should follow the dwc3 core and ignore the wrapper for purposes of the binding (unless there is some complex clock tree there). > <snip> > >> > + >> > + ranges; >> > + >> > + status = "disabled"; >> > + >> > + dwc3@11000000 { >> > + compatible = "snps,dwc3"; >> >> This sub-node is just wrong. Why can't you have a single node with ' >> "qcom,dwc3", "snps,dwc3" ' for the compatible property? All you are >> adding here is clocks. Does the Synopsys block have no clocks? >> >> I guess this is copied from other broken dwc3 bindings... That doesn't >> mean you have to copy it. > > The dwc3 core does not deal with clocks. That is why everyone has a wrapper. > That, in addition to pm, has to be handled from the wrapper. That's my take > anyway. I am sure Felipe can speak more to this. That is a Linux driver issue which is irrelevant to the binding. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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, 2014-07-01 at 14:47 -0500, Rob Herring wrote: > On Tue, Jul 1, 2014 at 1:01 PM, Andy Gross <agross@codeaurora.org> wrote: > > On Tue, Jul 01, 2014 at 12:04:35AM -0500, Rob Herring wrote: <snip> > > <snip> > > > >> > + > >> > + ranges; > >> > + > >> > + status = "disabled"; > >> > + > >> > + dwc3@11000000 { > >> > + compatible = "snps,dwc3"; > >> > >> This sub-node is just wrong. Why can't you have a single node with ' > >> "qcom,dwc3", "snps,dwc3" ' for the compatible property? All you are > >> adding here is clocks. Does the Synopsys block have no clocks? > >> > >> I guess this is copied from other broken dwc3 bindings... That doesn't > >> mean you have to copy it. > > > > The dwc3 core does not deal with clocks. That is why everyone has a wrapper. > > That, in addition to pm, has to be handled from the wrapper. That's my take > > anyway. I am sure Felipe can speak more to this. > > That is a Linux driver issue which is irrelevant to the binding. DWC3 IP core from Synopsys is the same across SoC's (OMAP, Exynos..), vendors are adding glue IP to provide necessary clocks and voltages. I think that the DT bindings properly reflect this fact. Regards, Ivan > > Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 02 July 2014 11:43:25 Ivan T. Ivanov wrote: > > > <snip> > > > > > >> > + > > >> > + ranges; > > >> > + > > >> > + status = "disabled"; > > >> > + > > >> > + dwc3@11000000 { > > >> > + compatible = "snps,dwc3"; > > >> > > >> This sub-node is just wrong. Why can't you have a single node with ' > > >> "qcom,dwc3", "snps,dwc3" ' for the compatible property? All you are > > >> adding here is clocks. Does the Synopsys block have no clocks? > > >> > > >> I guess this is copied from other broken dwc3 bindings... That doesn't > > >> mean you have to copy it. > > > > > > The dwc3 core does not deal with clocks. That is why everyone has a wrapper. > > > That, in addition to pm, has to be handled from the wrapper. That's my take > > > anyway. I am sure Felipe can speak more to this. > > > > That is a Linux driver issue which is irrelevant to the binding. > > DWC3 IP core from Synopsys is the same across SoC's (OMAP, Exynos..), > vendors are adding glue IP to provide necessary clocks and voltages. > I think that the DT bindings properly reflect this fact. Not really. The proper way to do this is to have a single node that lists multiple compatible strings from the most specific (per SoC variant) to most generic (the underlying IP core) and have all properties in it. We have seen many cases before where it later turned out that whatever feature people thought was SoC specific actually was common to all of them and that we later want to change the code to handle it in a common way, and to reflect it in the common binding. The clocks that Rob mentioned are one example of that. If you have a binding that separates one IP block into two device nodes, you cannot later adapt the common driver to be more generic and handle all sorts of SoCs. See the usb-ehci.txt for an example: it started out really simple but the generic driver has been extended multiple times to the point where it handles a lot of SoCs by default. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Jul 02, 2014 at 02:48:17PM +0200, Arnd Bergmann wrote: > On Wednesday 02 July 2014 11:43:25 Ivan T. Ivanov wrote: > > > > <snip> > > > > > > > >> > + > > > >> > + ranges; > > > >> > + > > > >> > + status = "disabled"; > > > >> > + > > > >> > + dwc3@11000000 { > > > >> > + compatible = "snps,dwc3"; > > > >> > > > >> This sub-node is just wrong. Why can't you have a single node with ' > > > >> "qcom,dwc3", "snps,dwc3" ' for the compatible property? All you are > > > >> adding here is clocks. Does the Synopsys block have no clocks? > > > >> > > > >> I guess this is copied from other broken dwc3 bindings... That doesn't > > > >> mean you have to copy it. > > > > > > > > The dwc3 core does not deal with clocks. That is why everyone has a wrapper. everyone has a wrapper for several others reasons. PM from the point of view of the Synopsys IP is *completely* different from PM from the point of view the $current_soc. The wrapper helps abstract that. Currently, there's little PM implemented in the driver, but when we get to implementing the nice features Synopsys has given us (hibernation, for instance) it will start to be aparent why the driver was split like that. > > > > That, in addition to pm, has to be handled from the wrapper. That's my take > > > > anyway. I am sure Felipe can speak more to this. > > > > > > That is a Linux driver issue which is irrelevant to the binding. > > > > DWC3 IP core from Synopsys is the same across SoC's (OMAP, Exynos..), > > vendors are adding glue IP to provide necessary clocks and voltages. > > I think that the DT bindings properly reflect this fact. > > Not really. The proper way to do this is to have a single node that > lists multiple compatible strings from the most specific (per SoC variant) > to most generic (the underlying IP core) and have all properties in it. > > We have seen many cases before where it later turned out that whatever > feature people thought was SoC specific actually was common to all > of them and that we later want to change the code to handle it in a > common way, and to reflect it in the common binding. The clocks that > Rob mentioned are one example of that. > > If you have a binding that separates one IP block into two device nodes, > you cannot later adapt the common driver to be more generic and handle > all sorts of SoCs. See the usb-ehci.txt for an example: it started out > really simple but the generic driver has been extended multiple times > to the point where it handles a lot of SoCs by default. The fact is that you _DO_ have *TWO* IP blocks. The synopsys DWC3 is one IP and the wrapper is another IP which adapts the synopsys IP to the SoC's integration context. From the SoC point of view, the device only needs one (or maybe two) clocks, an IRQ line, etc... The wrapper then, sometimes, enables that particular memory region, enables IRQs and generates several other clocks which are needed for proper operation of the IP. Having this sort of "bridge" or "glue" driver also helps *HUGELY* in different PM methodologies different SoCs use. I certainly don't want any clock API calls in my dwc3 driver when I'm running on top of a PCI bus on an intel platform. Likewise, I don't want any pci_enable_device() calls in my dwc3 driver when I'm running on OMAP/Exynos/Qcom/etc. Now, you guys just decided to give crap to the authors for no aparent reason. If you really want to describe the HW then every single clock node and every single voltage rail would have to be described but that would just create a whole bunch of fixed clocks and fixed regulators which have no way of being controlled whatsoever and just waste memory due to several other instances of such drivers being needed. It's pointless to continue discussing if we should have one clock node or two clock nodes. If the wrapper doesn't need an interface clock, it just doesn't need an interface clock and it's not up to us to start *GUESSING* that it maybe has both clock inputs tied to the same clock node if that's not what's written in the documentation. Sure, Qcom folks could ask their HW designers, but what would that give us in practice ? Nothing, not a single damn benefit. So can we just move on from this pointless discussion now ? cheers
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt new file mode 100644 index 0000000..105b6b7 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt @@ -0,0 +1,104 @@ +Qualcomm SuperSpeed DWC3 USB SoC controller + + +QCOM DWC3 Highspeed USB PHY +======================== +Required properities: +- compatible: should contain "qcom,dwc3-hsphy"; +- reg: offset and length of the register set in the memory map +- clocks: A list of phandle + clock-specifier pairs for the + clocks listed in clock-names +- clock-names: Should contain the following: + "utmi" UTMI clock +- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. +- v3p3-supply: phandle to the regulator for the 3.3v supply to HSPHY. +- vbus-supply: phandle to the regulator for the vbus supply for host + mode. +- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY + digital circuit operation. + +Optional clocks: + "xo" External reference clock + + +QCOM DWC3 Superspeed USB PHY +========================= +Required properities: +- compatible: should contain "qcom,dwc3-ssphy"; +- reg: offset and length of the register set in the memory map +- clocks: A list of phandle + clock-specifier pairs for the + clocks listed in clock-names +- clock-names: Should contain the following: + "ref" Reference clock used in host mode. +- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. +- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY + digital circuit operation. + +Optional clocks: + "xo" External reference clock + +QCOM DWC3 controller wrapper +=========================== +Required properties: +- compatible: should contain "qcom,dwc3" +- clocks: A list of phandle + clock-specifier pairs for the + clocks listed in clock-names +- clock-names: Should contain the following: + "core" Master/Core clock, have to be >= 125 MHz for SS + operation and >= 60MHz for HS operation + +Optional clocks: + "iface" System bus AXI clock. Not present on all platforms + "sleep" Sleep clock, used when USB3 core goes into low + power mode (U3). + +Optional regulator: +- gdsc-supply: phandle to the regulator from globally distributed + switch controller + +Required child node: +A child node must exist to represent the core DWC3 IP block. The name of +the node is not important. The content of the node is defined in dwc3.txt. + +Example device nodes: + + hs_phy_0: phy@110f8800 { + compatible = "qcom,dwc3-hsphy"; + reg = <0x110f8800 0x30>; + clocks = <&gcc USB30_0_UTMI_CLK>; + clock-names = "utmi"; + + status = "disabled"; + }; + + ss_phy_0: phy@110f8830 { + compatible = "qcom,dwc3-ssphy"; + reg = <0x110f8830 0x30>; + + clocks = <&gcc USB30_0_MASTER_CLK>; + clock-names = "ref"; + + status = "disabled"; + }; + + usb3_0: usb30@0 { + compatible = "qcom,dwc3"; + #address-cells = <1>; + #size-cells = <1>; + clocks = <&gcc USB30_0_MASTER_CLK>; + clock-names = "core"; + + ranges; + + status = "disabled"; + + dwc3@11000000 { + compatible = "snps,dwc3"; + reg = <0x11000000 0xcd00>; + interrupts = <0 110 0x4>; + usb-phy = <&hs_phy_0>, <&ss_phy_0>; + phy-names = "usb2-phy", "usb3-phy"; + tx-fifo-resize; + dr_mode = "host"; + }; + };