Re: [PATCH v3 3/6] DT: PCI: qcom: Document PCIe devicetree bindings

[Stanimir Varbanov]

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 
>>
>> Document Qualcomm PCIe driver devicetree bindings.
>>
>> Signed-off-by: Stanimir Varbanov 
>> Signed-off-by: Stanimir Varbanov 
>> ---
>>  .../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 ..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: 
>> +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: 
>> +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.

> 
> [..]
>> +- -supply:
>> +Usage: required
>> +Value type: 
>> +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.

> 
> [..]
>> +- -gpio:
>> +Usage: optional
>> +Value type: 
>> +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 -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: 
>> +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: 
>> +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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
regards,
Stan
--
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 v3 3/6] DT: PCI: qcom: Document PCIe devicetree bindings

[Stanimir Varbanov]

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 
>>
>> Document Qualcomm PCIe driver devicetree bindings.
>>
>> Signed-off-by: Stanimir Varbanov 
>> Signed-off-by: Stanimir Varbanov 
>> ---
>>  .../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 ..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: 
>> +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.
> 
> [...]
> 
>> +
>> +- -gpio:
> 
> Use -gpios

Ok I will.

-- 
regards,
Stan
--
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 v3 3/6] DT: PCI: qcom: Document PCIe devicetree bindings

[Bjorn Andersson]

On Mon 23 Nov 01:29 PST 2015, Stanimir Varbanov wrote:

> From: Stanimir Varbanov 
> 
> Document Qualcomm PCIe driver devicetree bindings.
> 
> Signed-off-by: Stanimir Varbanov 
> Signed-off-by: Stanimir Varbanov 
> ---
>  .../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 ..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: 
> + 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: 
> + 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.

[..]
> +- -supply:
> + Usage: required
> + Value type: 
> + 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.

[..]
> +- -gpio:
> + Usage: optional
> + Value type: 
> + 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 -gpios.

Are these identifiers coming from some data sheet? Or could we simply
name them "reset" and "wakeup"?

> +
> +- pinctrl-0:
> + Usage: required
> + Value type: 
> + 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: 
> + 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/6] DT: PCI: qcom: Document PCIe devicetree bindings

[Stanimir Varbanov]

From: Stanimir Varbanov 

Document Qualcomm PCIe driver devicetree bindings.

Signed-off-by: Stanimir Varbanov 
Signed-off-by: Stanimir Varbanov 
---
 .../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 ..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: 
+   Definition: Value shall include
+   - "qcom,pcie-v0" for apq/ipq8064
+   - "qcom,pcie-v1" for apq8084
+
+- reg:
+   Usage: required
+   Value type: 
+   Definition: Register ranges as listed in the reg-names property
+
+- reg-names:
+   Usage: required
+   Value type: 
+   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: 
+   Definition: Should be "pci". As specified in designware-pcie.txt
+
+- #address-cells:
+   Usage: required
+   Value type: 
+   Definition: Should be set to 3. As specified in designware-pcie.txt
+
+- #size-cells:
+   Usage: required
+   Value type: 
+   Definition: Should be set 2. As specified in designware-pcie.txt
+
+- ranges:
+   Usage: required
+   Value type: 
+   Definition: As specified in designware-pcie.txt
+
+- interrupts:
+   Usage: required
+   Value type: 
+   Definition: MSI interrupt
+
+- interrupt-names:
+   Usage: required
+   Value type: 
+   Definition: Should contain "msi"
+
+- #interrupt-cells:
+   Usage: required
+   Value type: 
+   Definition: Should be 1. As specified in designware-pcie.txt
+
+- interrupt-map-mask:
+   Usage: required
+   Value type: 
+   Definition: As specified in designware-pcie.txt
+
+- interrupt-map:
+   Usage: required
+   Value type: 
+   Definition: As specified in designware-pcie.txt
+
+- clocks:
+   Usage: required
+   Value type: 
+   Definition: List of phandle and clock specifier pairs as listed
+   in clock-names property
+
+- clock-names:
+   Usage: required
+   Value type: 
+   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: 
+   Definition: List of phandle and reset specifier pairs as listed
+   in reset-names property
+
+- reset-names:
+   Usage: required
+   Value type: 
+   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: 
+   Definition: A phandle and power domain specifier pair to the
+   power domain which is responsible for collapsing
+   and restoring power to the peripheral
+
+- -supply:
+   Usage: required
+   Value type: 
+   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: 
+   Definition: List of phandle(s) as listed in phy-names property
+
+- phy-names:
+   Usage: required (for v1 only)
+   Value type: 
+   

Re: [PATCH v3 3/6] DT: PCI: qcom: Document PCIe devicetree bindings

[Rob Herring]

On Mon, Nov 23, 2015 at 11:29:00AM +0200, Stanimir Varbanov wrote:
> From: Stanimir Varbanov 
> 
> Document Qualcomm PCIe driver devicetree bindings.
> 
> Signed-off-by: Stanimir Varbanov 
> Signed-off-by: Stanimir Varbanov 
> ---
>  .../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 ..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: 
> + 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.

[...]

> +
> +- -gpio:

Use -gpios

> + Usage: optional
> + Value type: 
> + Definition: List of phandle and gpio specifier pairs. Should contain
> + - "perst"  PCIe endpoint reset signal line
> + - "pewake" PCIe endpoint wake signal line

--
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