linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller
@ 2016-06-16  1:50 Shawn Lin
  2016-06-16  7:00 ` Arnd Bergmann
  0 siblings, 1 reply; 6+ messages in thread
From: Shawn Lin @ 2016-06-16  1:50 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Marc Zyngier, linux-pci, Arnd Bergmann, linux-kernel,
	linux-rockchip, Heiko Stuebner, Doug Anderson, Wenrui Li,
	Rob Herring, devicetree, Shawn Lin

This patch adds a binding that describes the Rockchip PCIe controller
found on Rockchip SoCs PCIe interface.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v3:
- fix example dts code suggested by Rob and Marc
- remove driver's behaviour of regulator

Changes in v2:
- fix lots clk/reset stuff suggested by Heiko
- remove msi-parent and add msi-map suggested by Marc
- drop phy related stuff
- some others minor fixes

 .../devicetree/bindings/pci/rockchip-pcie.txt      | 86 ++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rockchip-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
new file mode 100644
index 0000000..1ffdc7c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -0,0 +1,86 @@
+* Rockchip AXI PCIe Root Port Bridge DT description
+
+Required properties:
+- #address-cells: Address representation for root ports, set to <3>
+- #size-cells: Size representation for root ports, set to <2>
+- #interrupt-cells: specifies the number of cells needed to encode an
+		interrupt source. The value must be 1.
+- compatible: Should contain "rockchip,rk3399-pcie"
+- reg: Two register ranges as listed in the reg-names property
+- reg-names: Must include the following names
+	- "axi-base"
+	- "apb-base"
+- clocks: Must contain an entry for each entry in clock-names.
+		See ../clocks/clock-bindings.txt for details.
+- clock-names: Must include the following entries:
+	- "aclk"
+	- "aclk-perf"
+	- "hclk"
+	- "pm"
+- msi-map: Maps a Requester ID to an MSI controller and associated.
+		See ./pci-msi.txt
+- phys: From PHY bindings: Phandle for the Generic PHY for PCIe.
+- phy-names:  MUST be "pcie-phy".
+- interrupts: Three interrupt entries must be specified.
+- interrupt-names: Must include the following names
+	- "sys"
+	- "legacy"
+	- "client"
+- resets: Must contain five entries for each entry in reset-names.
+	   See ../reset/reset.txt for details.
+- reset-names: Must include the following names
+	- "core"
+	- "mgmt"
+	- "mgmt-sticky"
+	- "pipe"
+- pinctrl-names : The pin control state names
+- pinctrl-0: The "default" pinctrl state
+- interrupt-map-mask and interrupt-map: standard PCI properties
+- interrupt-controller: identifies the node as an interrupt controller
+
+Optional Property:
+- ep-gpios: contain the entry for pre-reset gpio
+- num-lanes: number of lanes to use
+- vpcie3v3-supply: The phandle to the 3.3v regulator to use for pcie.
+- vpcie1v8-supply: The phandle to the 1.8v regulator to use for pcie.
+- vpcie0v9-supply: The phandle to the 0.9v regulator to use for pcie.
+
+Example:
+
+pcie0: pcie@f8000000 {
+	compatible = "rockchip,rk3399-pcie";
+	#address-cells = <3>;
+	#size-cells = <2>;
+	clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
+		 <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>;
+	clock-names = "aclk", "aclk-perf",
+		      "hclk", "pm";
+	bus-range = <0x0 0x1>;
+	interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
+	interrupt-names = "sys", "legacy", "client";
+	assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
+	assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
+	assigned-clock-rates = <100000000>;
+	ep-gpios = <&gpio3 13 GPIO_ACTIVE_HIGH>;
+	ranges = <0x83000000 0x0 0xfa000000 0x0 0xfa000000 0x0 0x600000
+		  0x81000000 0x0 0xfa600000 0x0 0xfa600000 0x0 0x100000>;
+	num-lanes = <4>;
+	msi-map = <0x0 &its 0x0 0x1000>;
+	reg = < 0x0 0xf8000000 0x0 0x2000000 >, < 0x0 0xfd000000 0x0 0x1000000 >;
+	reg-names = "axi-base", "apb-base";
+	resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
+		 <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>;
+	reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
+	phys = <&pcie_phy>;
+	phy-names = "pcie-phy";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_clkreq>;
+	#interrupt-cells = <1>;
+	interrupt-controller;
+	interrupt-map-mask = <0 0 0 7>;
+	interrupt-map = <0 0 0 1 &pcie0 1>,
+			<0 0 0 2 &pcie0 2>,
+			<0 0 0 3 &pcie0 3>,
+			<0 0 0 4 &pcie0 4>;
+};
-- 
2.3.7

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller
  2016-06-16  1:50 [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller Shawn Lin
@ 2016-06-16  7:00 ` Arnd Bergmann
  2016-06-16  8:01   ` Wenrui Li
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2016-06-16  7:00 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Bjorn Helgaas, Marc Zyngier, linux-pci, linux-kernel,
	linux-rockchip, Heiko Stuebner, Doug Anderson, Wenrui Li,
	Rob Herring, devicetree, Thomas Petazzoni

On Thursday, June 16, 2016 9:50:21 AM CEST Shawn Lin wrote:

> +	reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
> +	phys = <&pcie_phy>;
> +	phy-names = "pcie-phy";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie_clkreq>;
> +	#interrupt-cells = <1>;
> +	interrupt-controller;
> +	interrupt-map-mask = <0 0 0 7>;
> +	interrupt-map = <0 0 0 1 &pcie0 1>,
> +			<0 0 0 2 &pcie0 2>,
> +			<0 0 0 3 &pcie0 3>,
> +			<0 0 0 4 &pcie0 4>;
> +};
> 

One thing that came up in the review of the new Marvell PCIe driver is that it's
most likely invalid for a device node to have both "interrupt-controller"
and "interrupt-map" properties. I originally thought this was a nice way to
handle embedded irqchips within the PCIe host, but it only really works
by coincidence with the current kernel, and only as long as the hwirq number
of the irqchip matches the integer representation of the irq line in the root
bridge (which it does in the example above).

For that driver we concluded that it would be less of a hack to have the
irqchip as a child node of the PCIe host after all (just not with
device_type="pci" of course), and that makes the translation work as
expected.

	Arnd

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller
  2016-06-16  7:00 ` Arnd Bergmann
@ 2016-06-16  8:01   ` Wenrui Li
  2016-06-16  9:22     ` Arnd Bergmann
  0 siblings, 1 reply; 6+ messages in thread
From: Wenrui Li @ 2016-06-16  8:01 UTC (permalink / raw)
  To: Arnd Bergmann, Shawn Lin
  Cc: Bjorn Helgaas, Marc Zyngier, linux-pci, linux-kernel,
	linux-rockchip, Heiko Stuebner, Doug Anderson, Rob Herring,
	devicetree, Thomas Petazzoni



在 2016/6/16 15:00, Arnd Bergmann 写道:
> On Thursday, June 16, 2016 9:50:21 AM CEST Shawn Lin wrote:
>
>> +	reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
>> +	phys = <&pcie_phy>;
>> +	phy-names = "pcie-phy";
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pcie_clkreq>;
>> +	#interrupt-cells = <1>;
>> +	interrupt-controller;
>> +	interrupt-map-mask = <0 0 0 7>;
>> +	interrupt-map = <0 0 0 1 &pcie0 1>,
>> +			<0 0 0 2 &pcie0 2>,
>> +			<0 0 0 3 &pcie0 3>,
>> +			<0 0 0 4 &pcie0 4>;
>> +};
>>
>
> One thing that came up in the review of the new Marvell PCIe driver is that it's
> most likely invalid for a device node to have both "interrupt-controller"
> and "interrupt-map" properties. I originally thought this was a nice way to
> handle embedded irqchips within the PCIe host, but it only really works
> by coincidence with the current kernel, and only as long as the hwirq number
> of the irqchip matches the integer representation of the irq line in the root
> bridge (which it does in the example above).
>
> For that driver we concluded that it would be less of a hack to have the
> irqchip as a child node of the PCIe host after all (just not with
> device_type="pci" of course), and that makes the translation work as
> expected.
>
> 	Arnd
>

Original driver have an irqchip as child node. But Marc suggested don't 
need an intermediate node here.
Now the conclusion is to retain the child node?

>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller
  2016-06-16  8:01   ` Wenrui Li
@ 2016-06-16  9:22     ` Arnd Bergmann
  2016-06-16  9:58       ` Thomas Petazzoni
  2016-06-19 14:53       ` Rob Herring
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2016-06-16  9:22 UTC (permalink / raw)
  To: Wenrui Li
  Cc: Shawn Lin, Bjorn Helgaas, Marc Zyngier, linux-pci, linux-kernel,
	linux-rockchip, Heiko Stuebner, Doug Anderson, Rob Herring,
	devicetree, Thomas Petazzoni

On Thursday, June 16, 2016 4:01:12 PM CEST Wenrui Li wrote:
> 在 2016/6/16 15:00, Arnd Bergmann 写道:
> > On Thursday, June 16, 2016 9:50:21 AM CEST Shawn Lin wrote:
> >
> >> +    reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
> >> +    phys = <&pcie_phy>;
> >> +    phy-names = "pcie-phy";
> >> +    pinctrl-names = "default";
> >> +    pinctrl-0 = <&pcie_clkreq>;
> >> +    #interrupt-cells = <1>;
> >> +    interrupt-controller;
> >> +    interrupt-map-mask = <0 0 0 7>;
> >> +    interrupt-map = <0 0 0 1 &pcie0 1>,
> >> +                    <0 0 0 2 &pcie0 2>,
> >> +                    <0 0 0 3 &pcie0 3>,
> >> +                    <0 0 0 4 &pcie0 4>;
> >> +};
> >>
> >
> > One thing that came up in the review of the new Marvell PCIe driver is that it's
> > most likely invalid for a device node to have both "interrupt-controller"
> > and "interrupt-map" properties. I originally thought this was a nice way to
> > handle embedded irqchips within the PCIe host, but it only really works
> > by coincidence with the current kernel, and only as long as the hwirq number
> > of the irqchip matches the integer representation of the irq line in the root
> > bridge (which it does in the example above).
> >
> > For that driver we concluded that it would be less of a hack to have the
> > irqchip as a child node of the PCIe host after all (just not with
> > device_type="pci" of course), and that makes the translation work as
> > expected.
> >
> >       Arnd
> >
> 
> Original driver have an irqchip as child node. But Marc suggested don't 
> need an intermediate node here.
> Now the conclusion is to retain the child node?

That is at least my view of the situation, sorry for the mixed messages
you have been getting. Marc, Rob, do you agree with my finding?

If we want to allow having both interrupt-map and interrupt-controller
in the same node, we need to rewrite both the irq parsing function and
have extend the DT binding for the interrupt-map to explain what we
actually expect to happen in that case. At the moment, we walk up the
tree until we find either an interrupt-map or an interrupt-controller
property, and use that to map the interrupt number. If we find an
interrupt-controller, we ignore the interrupt-map.

	Arnd

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller
  2016-06-16  9:22     ` Arnd Bergmann
@ 2016-06-16  9:58       ` Thomas Petazzoni
  2016-06-19 14:53       ` Rob Herring
  1 sibling, 0 replies; 6+ messages in thread
From: Thomas Petazzoni @ 2016-06-16  9:58 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Wenrui Li, Shawn Lin, Bjorn Helgaas, Marc Zyngier, linux-pci,
	linux-kernel, linux-rockchip, Heiko Stuebner, Doug Anderson,
	Rob Herring, devicetree

Hello,

On Thu, 16 Jun 2016 11:22:02 +0200, Arnd Bergmann wrote:

> > >> +    reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
> > >> +    phys = <&pcie_phy>;
> > >> +    phy-names = "pcie-phy";
> > >> +    pinctrl-names = "default";
> > >> +    pinctrl-0 = <&pcie_clkreq>;
> > >> +    #interrupt-cells = <1>;
> > >> +    interrupt-controller;
> > >> +    interrupt-map-mask = <0 0 0 7>;
> > >> +    interrupt-map = <0 0 0 1 &pcie0 1>,
> > >> +                    <0 0 0 2 &pcie0 2>,
> > >> +                    <0 0 0 3 &pcie0 3>,
> > >> +                    <0 0 0 4 &pcie0 4>;
> > >> +};
> > >>  
> > >
> > > One thing that came up in the review of the new Marvell PCIe driver is that it's
> > > most likely invalid for a device node to have both "interrupt-controller"
> > > and "interrupt-map" properties. I originally thought this was a nice way to
> > > handle embedded irqchips within the PCIe host, but it only really works
> > > by coincidence with the current kernel, and only as long as the hwirq number
> > > of the irqchip matches the integer representation of the irq line in the root
> > > bridge (which it does in the example above).
> > >
> > > For that driver we concluded that it would be less of a hack to have the
> > > irqchip as a child node of the PCIe host after all (just not with
> > > device_type="pci" of course), and that makes the translation work as
> > > expected.
> > >
> > >       Arnd
> > >  
> > 
> > Original driver have an irqchip as child node. But Marc suggested don't 
> > need an intermediate node here.
> > Now the conclusion is to retain the child node?  
> 
> That is at least my view of the situation, sorry for the mixed messages
> you have been getting. Marc, Rob, do you agree with my finding?
> 
> If we want to allow having both interrupt-map and interrupt-controller
> in the same node, we need to rewrite both the irq parsing function and
> have extend the DT binding for the interrupt-map to explain what we
> actually expect to happen in that case. At the moment, we walk up the
> tree until we find either an interrupt-map or an interrupt-controller
> property, and use that to map the interrupt number. If we find an
> interrupt-controller, we ignore the interrupt-map.

I can confirm what Arnd said. If you have the following interrupt-map
property:

 +    interrupt-map = <0 0 0 1 &pcie0 0>,
 +                    <0 0 0 2 &pcie0 1>,
 +                    <0 0 0 3 &pcie0 2>,
 +                    <0 0 0 4 &pcie0 3>;

Then the hwirq you get in the ->map() function are [ 1 ; 4 ] instead of
the expected [ 0 ; 3 ]. I.e the translation to the interrupt number in
the "target" interrupt controller does not happen.

Now, the question is whether this is a bug *or* whether having
interrupt-map pointing to its own node is not a valid situation.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller
  2016-06-16  9:22     ` Arnd Bergmann
  2016-06-16  9:58       ` Thomas Petazzoni
@ 2016-06-19 14:53       ` Rob Herring
  1 sibling, 0 replies; 6+ messages in thread
From: Rob Herring @ 2016-06-19 14:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Wenrui Li, Shawn Lin, Bjorn Helgaas, Marc Zyngier, linux-pci,
	linux-kernel, linux-rockchip, Heiko Stuebner, Doug Anderson,
	devicetree, Thomas Petazzoni

On Thu, Jun 16, 2016 at 11:22:02AM +0200, Arnd Bergmann wrote:
> On Thursday, June 16, 2016 4:01:12 PM CEST Wenrui Li wrote:
> > 在 2016/6/16 15:00, Arnd Bergmann 写道:
> > > On Thursday, June 16, 2016 9:50:21 AM CEST Shawn Lin wrote:
> > >
> > >> +    reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
> > >> +    phys = <&pcie_phy>;
> > >> +    phy-names = "pcie-phy";
> > >> +    pinctrl-names = "default";
> > >> +    pinctrl-0 = <&pcie_clkreq>;
> > >> +    #interrupt-cells = <1>;
> > >> +    interrupt-controller;
> > >> +    interrupt-map-mask = <0 0 0 7>;
> > >> +    interrupt-map = <0 0 0 1 &pcie0 1>,
> > >> +                    <0 0 0 2 &pcie0 2>,
> > >> +                    <0 0 0 3 &pcie0 3>,
> > >> +                    <0 0 0 4 &pcie0 4>;
> > >> +};
> > >>
> > >
> > > One thing that came up in the review of the new Marvell PCIe driver is that it's
> > > most likely invalid for a device node to have both "interrupt-controller"
> > > and "interrupt-map" properties. I originally thought this was a nice way to
> > > handle embedded irqchips within the PCIe host, but it only really works
> > > by coincidence with the current kernel, and only as long as the hwirq number
> > > of the irqchip matches the integer representation of the irq line in the root
> > > bridge (which it does in the example above).
> > >
> > > For that driver we concluded that it would be less of a hack to have the
> > > irqchip as a child node of the PCIe host after all (just not with
> > > device_type="pci" of course), and that makes the translation work as
> > > expected.
> > >
> > >       Arnd
> > >
> > 
> > Original driver have an irqchip as child node. But Marc suggested don't 
> > need an intermediate node here.
> > Now the conclusion is to retain the child node?
> 
> That is at least my view of the situation, sorry for the mixed messages
> you have been getting. Marc, Rob, do you agree with my finding?

Yes, the OF interrupt mapping doc[1] parsing code treats things them as 
mutually exclusive.

Rob

[1] http://www.unixag-kl.fh-kl.de/~jkunz/tmp/imap0_9d.pdf

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-06-19 14:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-16  1:50 [PATCH v3 1/2] Documentation: bindings: add dt doc for Rockchip PCIe controller Shawn Lin
2016-06-16  7:00 ` Arnd Bergmann
2016-06-16  8:01   ` Wenrui Li
2016-06-16  9:22     ` Arnd Bergmann
2016-06-16  9:58       ` Thomas Petazzoni
2016-06-19 14:53       ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).