| Message ID | 1466041821-1649-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
在 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? > >
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
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
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
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>; +};
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