| Message ID | 8fd3e7d1662f37abe549127d179c315dff3210d8.1448270813.git.stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon 23 Nov 01:29 PST 2015, Stanimir Varbanov wrote: > From: Stanimir Varbanov <svarbanov@mm-sol.com> > > Document Qualcomm PCIe driver devicetree bindings. > > Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > .../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++ > 1 file changed, 231 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt > new file mode 100644 > index 000000000000..d7640d45fa31 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt > @@ -0,0 +1,231 @@ > +* Qualcomm PCI express root complex > + > +- compatible: > + Usage: required > + Value type: <stringlist> > + Definition: Value shall include > + - "qcom,pcie-v0" for apq/ipq8064 > + - "qcom,pcie-v1" for apq8084 Do you know if we have the same v1 of this block in 8994? [..] > +- clock-names: > + Usage: required > + Value type: <stringlist> > + Definition: Should contain the following entries > + * should be populated for v0 and v1 > + - "iface" Configuration AHB clock > + > + * should be populated for v0 > + - "core" Clocks the pcie hw block > + - "phy" Clocks the pcie PHY block > + > + * should be populated for v1 > + - "aux" Auxiliary (AUX) clock > + - "bus_master" Master AXI clock > + - "bus_slave" Slave AXI clock You have white spaces among your tabs here. [..] > +- <name>-supply: > + Usage: required > + Value type: <phandle> > + Definition: List of phandles to the power supply regulator(s) > + * should be populated for v0 and v1 > + - "vdda" core analog power supply > + > + * should be populated for v0 > + - "vdda_phy" analog power supply for PHY > + - "vdda_refclk" analog power supply for IC which generate > + reference clock Exploding these into 3 different property descriptions would make it easier to read, and you can say "required for v0" for the latter two and simply "required" on the vdda. [..] > +- <name>-gpio: > + Usage: optional > + Value type: <prop-encoded-array> > + Definition: List of phandle and gpio specifier pairs. Should contain > + - "perst" PCIe endpoint reset signal line > + - "pewake" PCIe endpoint wake signal line This property should be pluralized, i.e. it's <name>-gpios. Are these identifiers coming from some data sheet? Or could we simply name them "reset" and "wakeup"? > + > +- pinctrl-0: > + Usage: required > + Value type: <phandle> > + Definition: List of phandles pointing at a pin(s) configuration This is not required and as it's a property applicable to all devices we normally don't mention them. > + > +- pinctrl-names > + Usage: required > + Value type: <stringlist> > + Definition: List of names of pinctrl-0 state > + dito Regards, Bjorn
On Mon, Nov 23, 2015 at 11:29:00AM +0200, Stanimir Varbanov wrote: > From: Stanimir Varbanov <svarbanov@mm-sol.com> > > Document Qualcomm PCIe driver devicetree bindings. > > Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > .../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++ > 1 file changed, 231 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt > new file mode 100644 > index 000000000000..d7640d45fa31 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt > @@ -0,0 +1,231 @@ > +* Qualcomm PCI express root complex > + > +- compatible: > + Usage: required > + Value type: <stringlist> > + Definition: Value shall include > + - "qcom,pcie-v0" for apq/ipq8064 > + - "qcom,pcie-v1" for apq8084 Use chip part numbers, not versions. Also, shouldn't you have the Designware compatible in addition. [...] > + > +- <name>-gpio: Use -gpios > + Usage: optional > + Value type: <prop-encoded-array> > + Definition: List of phandle and gpio specifier pairs. Should contain > + - "perst" PCIe endpoint reset signal line > + - "pewake" PCIe endpoint wake signal line
Bjorn, thanks for the comments! On 11/23/2015 08:13 PM, Bjorn Andersson wrote: > On Mon 23 Nov 01:29 PST 2015, Stanimir Varbanov wrote: > >> From: Stanimir Varbanov <svarbanov@mm-sol.com> >> >> Document Qualcomm PCIe driver devicetree bindings. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> .../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++ >> 1 file changed, 231 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt >> new file mode 100644 >> index 000000000000..d7640d45fa31 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt >> @@ -0,0 +1,231 @@ >> +* Qualcomm PCI express root complex >> + >> +- compatible: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Value shall include >> + - "qcom,pcie-v0" for apq/ipq8064 >> + - "qcom,pcie-v1" for apq8084 > > Do you know if we have the same v1 of this block in 8994? I have no idea, but looking in caf msm-3.18 it should be possible to reuse the code for v1. > > [..] >> +- clock-names: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Should contain the following entries >> + * should be populated for v0 and v1 >> + - "iface" Configuration AHB clock >> + >> + * should be populated for v0 >> + - "core" Clocks the pcie hw block >> + - "phy" Clocks the pcie PHY block >> + >> + * should be populated for v1 >> + - "aux" Auxiliary (AUX) clock >> + - "bus_master" Master AXI clock >> + - "bus_slave" Slave AXI clock > > You have white spaces among your tabs here. Ops, I forgot to remove the white spaces. > > [..] >> +- <name>-supply: >> + Usage: required >> + Value type: <phandle> >> + Definition: List of phandles to the power supply regulator(s) >> + * should be populated for v0 and v1 >> + - "vdda" core analog power supply >> + >> + * should be populated for v0 >> + - "vdda_phy" analog power supply for PHY >> + - "vdda_refclk" analog power supply for IC which generate >> + reference clock > > Exploding these into 3 different property descriptions would make it > easier to read, and you can say "required for v0" for the latter > two and simply "required" on the vdda. yes, that is a good idea. > > [..] >> +- <name>-gpio: >> + Usage: optional >> + Value type: <prop-encoded-array> >> + Definition: List of phandle and gpio specifier pairs. Should contain >> + - "perst" PCIe endpoint reset signal line >> + - "pewake" PCIe endpoint wake signal line > > This property should be pluralized, i.e. it's <name>-gpios. I hope you mean : - "perst-gpios" PCIe endpoint reset signal line - "pewake-gpios" PCIe endpoint wake signal line > > Are these identifiers coming from some data sheet? Or could we simply > name them "reset" and "wakeup"? In the pcie express card electromechanical specification we have signal names PERST# and WAKE#, so I'd like to keep as they are in the specification, thus the wake# is wrong and I will rename it to wake-gpios. > >> + >> +- pinctrl-0: >> + Usage: required >> + Value type: <phandle> >> + Definition: List of phandles pointing at a pin(s) configuration > > This is not required and as it's a property applicable to all devices we > normally don't mention them. agreed. > >> + >> +- pinctrl-names >> + Usage: required >> + Value type: <stringlist> >> + Definition: List of names of pinctrl-0 state >> + > > dito > > Regards, > Bjorn > -- > 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 >
Thanks for the comments! On 11/24/2015 01:17 AM, Rob Herring wrote: > On Mon, Nov 23, 2015 at 11:29:00AM +0200, Stanimir Varbanov wrote: >> From: Stanimir Varbanov <svarbanov@mm-sol.com> >> >> Document Qualcomm PCIe driver devicetree bindings. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> .../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++ >> 1 file changed, 231 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt >> new file mode 100644 >> index 000000000000..d7640d45fa31 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt >> @@ -0,0 +1,231 @@ >> +* Qualcomm PCI express root complex >> + >> +- compatible: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Value shall include >> + - "qcom,pcie-v0" for apq/ipq8064 >> + - "qcom,pcie-v1" for apq8084 > > Use chip part numbers, not versions. Sure. > > Also, shouldn't you have the Designware compatible in addition. > > [...] > >> + >> +- <name>-gpio: > > Use -gpios Ok I will.
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt new file mode 100644 index 000000000000..d7640d45fa31 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt @@ -0,0 +1,231 @@ +* Qualcomm PCI express root complex + +- compatible: + Usage: required + Value type: <stringlist> + Definition: Value shall include + - "qcom,pcie-v0" for apq/ipq8064 + - "qcom,pcie-v1" for apq8084 + +- reg: + Usage: required + Value type: <prop-encoded-array> + Definition: Register ranges as listed in the reg-names property + +- reg-names: + Usage: required + Value type: <stringlist> + Definition: Must include the following entries + - "parf" Qualcomm specific registers + - "dbi" Designware PCIe registers + - "elbi" External local bus interface registers + - "config" PCIe configuration space + +- device_type: + Usage: required + Value type: <string> + Definition: Should be "pci". As specified in designware-pcie.txt + +- #address-cells: + Usage: required + Value type: <u32> + Definition: Should be set to 3. As specified in designware-pcie.txt + +- #size-cells: + Usage: required + Value type: <u32> + Definition: Should be set 2. As specified in designware-pcie.txt + +- ranges: + Usage: required + Value type: <prop-encoded-array> + Definition: As specified in designware-pcie.txt + +- interrupts: + Usage: required + Value type: <prop-encoded-array> + Definition: MSI interrupt + +- interrupt-names: + Usage: required + Value type: <stringlist> + Definition: Should contain "msi" + +- #interrupt-cells: + Usage: required + Value type: <u32> + Definition: Should be 1. As specified in designware-pcie.txt + +- interrupt-map-mask: + Usage: required + Value type: <prop-encoded-array> + Definition: As specified in designware-pcie.txt + +- interrupt-map: + Usage: required + Value type: <prop-encoded-array> + Definition: As specified in designware-pcie.txt + +- clocks: + Usage: required + Value type: <prop-encoded-array> + Definition: List of phandle and clock specifier pairs as listed + in clock-names property + +- clock-names: + Usage: required + Value type: <stringlist> + Definition: Should contain the following entries + * should be populated for v0 and v1 + - "iface" Configuration AHB clock + + * should be populated for v0 + - "core" Clocks the pcie hw block + - "phy" Clocks the pcie PHY block + + * should be populated for v1 + - "aux" Auxiliary (AUX) clock + - "bus_master" Master AXI clock + - "bus_slave" Slave AXI clock + +- resets: + Usage: required + Value type: <prop-encoded-array> + Definition: List of phandle and reset specifier pairs as listed + in reset-names property + +- reset-names: + Usage: required + Value type: <stringlist> + Definition: Should contain the following entries + * should be populated for v0 + - "axi" AXI reset + - "ahb" AHB reset + - "por" POR reset + - "pci" PCI reset + - "phy" PHY reset + + * should be populated for v1 + - "core" Core reset + +- power-domains: + Usage: required (for v1 only) + Value type: <prop-encoded-array> + Definition: A phandle and power domain specifier pair to the + power domain which is responsible for collapsing + and restoring power to the peripheral + +- <name>-supply: + Usage: required + Value type: <phandle> + Definition: List of phandles to the power supply regulator(s) + * should be populated for v0 and v1 + - "vdda" core analog power supply + + * should be populated for v0 + - "vdda_phy" analog power supply for PHY + - "vdda_refclk" analog power supply for IC which generate + reference clock + +- phys: + Usage: required (for v1 only) + Value type: <phandle> + Definition: List of phandle(s) as listed in phy-names property + +- phy-names: + Usage: required (for v1 only) + Value type: <stringlist> + Definition: Should contain "pciephy" + +- <name>-gpio: + Usage: optional + Value type: <prop-encoded-array> + Definition: List of phandle and gpio specifier pairs. Should contain + - "perst" PCIe endpoint reset signal line + - "pewake" PCIe endpoint wake signal line + +- pinctrl-0: + Usage: required + Value type: <phandle> + Definition: List of phandles pointing at a pin(s) configuration + +- pinctrl-names + Usage: required + Value type: <stringlist> + Definition: List of names of pinctrl-0 state + +* Example for v0 + pcie0: pci@1b500000 { + compatible = "qcom,pcie-v0", "snps,dw-pcie"; + reg = <0x1b500000 0x1000 + 0x1b502000 0x80 + 0x1b600000 0x100 + 0x0ff00000 0x100000>; + reg-names = "dbi", "elbi", "parf", "config"; + device_type = "pci"; + linux,pci-domain = <0>; + bus-range = <0x00 0xff>; + num-lanes = <1>; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000 /* I/O */ + 0x82000000 0 0x00000000 0x08000000 0 0x07e00000>; /* memory */ + interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>; + interrupt-names = "msi"; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, /* int_a */ + <0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, /* int_b */ + <0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, /* int_c */ + <0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */ + clocks = <&gcc PCIE_A_CLK>, + <&gcc PCIE_H_CLK>, + <&gcc PCIE_PHY_CLK>; + clock-names = "core", "iface", "phy"; + resets = <&gcc PCIE_ACLK_RESET>, + <&gcc PCIE_HCLK_RESET>, + <&gcc PCIE_POR_RESET>, + <&gcc PCIE_PCI_RESET>, + <&gcc PCIE_PHY_RESET>; + reset-names = "axi", "ahb", "por", "pci", "phy"; + }; + +* Example for v1 + pcie0@fc520000 { + compatible = "qcom,pcie-v1", "snps,dw-pcie"; + reg = <0xfc520000 0x2000>, + <0xff000000 0x1000>, + <0xff001000 0x1000>, + <0xff002000 0x2000>; + reg-names = "parf", "dbi", "elbi", "config"; + device_type = "pci"; + linux,pci-domain = <0>; + bus-range = <0x00 0xff>; + num-lanes = <1>; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x81000000 0 0 0xff200000 0 0x00100000 /* I/O */ + 0x82000000 0 0x00300000 0xff300000 0 0x00d00000>; /* memory */ + interrupts = <GIC_SPI 243 IRQ_TYPE_NONE>; + interrupt-names = "msi"; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = <0 0 0 1 &intc 0 244 IRQ_TYPE_LEVEL_HIGH>, /* int_a */ + <0 0 0 2 &intc 0 245 IRQ_TYPE_LEVEL_HIGH>, /* int_b */ + <0 0 0 3 &intc 0 247 IRQ_TYPE_LEVEL_HIGH>, /* int_c */ + <0 0 0 4 &intc 0 248 IRQ_TYPE_LEVEL_HIGH>; /* int_d */ + clocks = <&gcc GCC_PCIE_0_CFG_AHB_CLK>, + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>, + <&gcc GCC_PCIE_0_SLV_AXI_CLK>, + <&gcc GCC_PCIE_0_AUX_CLK>; + clock-names = "iface", "master_bus", "slave_bus", "aux"; + resets = <&gcc GCC_PCIE_0_BCR>; + reset-names = "core"; + power-domains = <&gcc PCIE0_GDSC>; + vdda-supply = <&pma8084_l3>; + phys = <&pciephy0>; + phy-names = "pciephy"; + perst-gpio = <&tlmm 70 GPIO_ACTIVE_LOW>; + pinctrl-0 = <&pcie0_pins_default>; + pinctrl-names = "default"; + };