Re: [Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
Hi,
On Tue, 2014-07-01 at 14:47 -0500, Rob Herring wrote:
On Tue, Jul 1, 2014 at 1:01 PM, Andy Gross agr...@codeaurora.org wrote:
On Tue, Jul 01, 2014 at 12:04:35AM -0500, Rob Herring wrote:
snip
snip
+
+ ranges;
+
+ status = disabled;
+
+ dwc3@1100 {
+ 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
On Wednesday 02 July 2014 11:43:25 Ivan T. Ivanov wrote:
snip
+
+ ranges;
+
+ status = disabled;
+
+ dwc3@1100 {
+ 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
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@1100 {
+ 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
--
balbi
signature.asc
Description: Digital signature
Re: [Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
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@1100 {
+ 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.
--
sent by an employee of the Qualcomm Innovation Center, Inc.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
From: Ivan T. Ivanov iiva...@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 iiva...@mm-sol.com
Signed-off-by: Andy Gross agr...@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 000..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:
+ refReference 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@1100 {
+ compatible = snps,dwc3;
+ reg = 0x1100 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
Re: [Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
On Jun 30, 2014, at 11:03 AM, Andy Gross agr...@codeaurora.org wrote:
From: Ivan T. Ivanov iiva...@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 iiva...@mm-sol.com
Signed-off-by: Andy Gross agr...@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 000..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:
+ ifaceSystem bus AXI clock. Not present on all platforms
+ sleepSleep 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@1100 {
+ compatible = snps,dwc3;
+ reg = 0x1100 0xcd00;
+ interrupts = 0 110 0x4;
+ usb-phy = hs_phy_0, ss_phy_0;
+
Re: [Patch v7 3/3] usb: dwc3: qcom: Add device tree binding
On Mon, Jun 30, 2014 at 11:03 AM, Andy Gross agr...@codeaurora.org wrote:
From: Ivan T. Ivanov iiva...@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 iiva...@mm-sol.com
Signed-off-by: Andy Gross agr...@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 000..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:
+ refReference 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@1100 {
+ 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
