[PATCH 0/2] Add Rockchip PCIe RC controller support

[PATCH 0/2] Add Rockchip PCIe RC controller support

[Shawn Lin]

Hi all,

This patchset gonna support Rockchip PCIe controller integrated
in RK3399 Soc supporting Gen2 with max 4 lanes.

Also it supports:
(1) Support Single-root I/O virtualization(SR-IOV)
(2) Support Legacy Interrupt
(3) Support MSI and MSI-X interrupt
(4) Support ECRC Generation and Checking
(5) Support 8 Virtual Functions attached to Physical Function
(6) Support Outbound and Inbound address translation
(7) Support ASPM state L0s and L1
(8) Support L1 Power Management Substate
(9) Support PCI Function power states D0, D1 and D3, and the corresponding
link power states L0, L1 and L2


Please review, test and comment. Thanks.



Shawn Lin (2):
  Documentation: add binding description of Rockchip PCIe controller
  pci: Add PCIe driver for Rockchip Soc

 .../devicetree/bindings/pci/rockchip-pcie.txt      |   93 ++
 drivers/pci/host/Kconfig                           |   12 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-rockchip.c                   | 1181 ++++++++++++++++++++
 4 files changed, 1287 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rockchip-pcie.txt
 create mode 100644 drivers/pci/host/pcie-rockchip.c

-- 
2.3.7

[PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Shawn Lin]

This patch add some required and optional properties for Rockchip
PCIe controller. Also we add a example for how to use it.

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

---

 .../devicetree/bindings/pci/rockchip-pcie.txt      | 93 ++++++++++++++++++++++
 1 file changed, 93 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..69a0804
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
@@ -0,0 +1,93 @@
+* 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: The first entry must be "axi-base" for the core registers
+	The second entry must be "apb-base" for the client pcie registers
+- 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_pcie"
+	- "aclk_perf_pcie"
+	- "hclk_pcie"
+	- "clk_pciephy_ref"
+- interrupts: Three interrupt entries must be specified.
+- interrupt-names: Must include the following names
+	- "pcie-sys"
+	- "pcie-legacy"
+	- "pcie-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
+	- "phy-rst"
+	- "core-rst"
+	- "mgmt-rst"
+	- "mgmt-sticky-rst"
+	- "pipe-rst"
+- rockchip,grf: phandle to the syscon managing the "general register files"
+- pcie-conf: offset of pcie client block for  configuration
+- pcie-status: offset of pcie client block for status
+- pcie-laneoff: offset of pcie client block for lane
+- msi-parent: Link to the hardware entity that serves as the Message
+- 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
+- assigned-clocks, assigned-clock-parents and assigned-clock-rates: standard
+		   clock bindings. See ../clock/clock-bindings.txt
+
+Example:
+
+pci_express: axi-pcie@f8000000 {
+	#address-cells = <3>;
+	#size-cells = <2>;
+	compatible = "rockchip,rk3399-pcie";
+	clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
+		 <&cru PCLK_PCIE>, <&cru SCLK_PCIEPHY_REF>;
+	clock-names = "aclk_pcie", "aclk_perf_pcie",
+		      "hclk_pcie", "clk_pciephy_ref";
+	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: "pcie-sys", "pcie-legacy", "pcie-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 = < 0x82000000 0 0xfa000000 0x0 0xfa000000 0 0x600000
+		   0x81000000 0 0xfa600000 0x0 0xfa600000 0 0x100000 >;
+	num-lanes = <4>;
+	reg = < 0x0 0xf8000000 0x0 0x2000000 >, < 0x0 0xfd000000 0x0 0x1000000 >;
+	reg-name = "axi-base", "apb-base";
+	resets = <&cru SRST_PCIEPHY>, <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
+		 <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>;
+	reset-names = "phy-rst", "core-rst", "mgmt-rst", "mgmt-sticky-rst", "pipe-rst";
+	rockchip,grf = <&grf>;
+	pcie-conf = <0xe220>;
+	pcie-status = <0xe2a4>;
+	pcie-laneoff = <0xe214>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_clkreq>;
+	msi-parent = <&its>;
+	#interrupt-cells = <1>;
+	interrupt-map-mask = <0 0 0 7>;
+	interrupt-map = <0 0 0 1 &pcie_0 1>,
+			<0 0 0 2 &pcie_0 2>,
+			<0 0 0 3 &pcie_0 3>,
+			<0 0 0 4 &pcie_0 4>;
+	pcie_0: interrupt-controller {
+		interrupt-controller;
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+	};
+
+};
-- 
2.3.7

Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Heiko Stuebner]

Hi Shawn,

Am Freitag, 20. Mai 2016, 18:29:06 schrieb Shawn Lin:
> This patch add some required and optional properties for Rockchip
> PCIe controller. Also we add a example for how to use it.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      | 93
> ++++++++++++++++++++++ 1 file changed, 93 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..69a0804
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -0,0 +1,93 @@
> +* 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: The first entry must be "axi-base" for the core registers
> +	The second entry must be "apb-base" for the client pcie registers
> +- 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_pcie"
> +	- "aclk_perf_pcie"
> +	- "hclk_pcie"
> +	- "clk_pciephy_ref"

clock names are always in the scope of the node/driver, so the names could 
easily be "aclk", "aclk-perf", "hclk", "phy".

Although from my cursory-glance at the phy-related code, it looks more like 
it shouldn't be in the pcie-driver itself, but in a an actual phy-driver 
(together with the phy reset and clock)?

> +- interrupts: Three interrupt entries must be specified.
> +- interrupt-names: Must include the following names
> +	- "pcie-sys"
> +	- "pcie-legacy"
> +	- "pcie-client"

Same as above, names could simply be "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
> +	- "phy-rst"
> +	- "core-rst"
> +	- "mgmt-rst"
> +	- "mgmt-sticky-rst"
> +	- "pipe-rst"

and again (= without the "-rst")


> +- rockchip,grf: phandle to the syscon managing the "general register
> files" 

> +- pcie-conf: offset of pcie client block for  configuration
> +- pcie-status: offset of pcie client block for status
> +- pcie-laneoff: offset of pcie client block for lane

These are GRF offsets (GRF_SOC_CON8, GRF_SOC_CON5_PCIE, GRF_SOC_STATUS1)
Those registers are generally prone to change (even their layout) for future 
socs and I'd suggest instead of declaring them in the devicetree, move them 
to the per-soc data in the rockchip_pcie_of_match struct.

But it looks as if they're pretty phy-specific, so see comment about possible 
separate phy driver above.


> +- msi-parent: Link to the hardware entity that serves as the Message
> +- pinctrl-names : The pin control state names
> +- pinctrl-0: The "default" pinctrl state

I'm not sure if pinctrl-properties need to be described when you don't need 
special handling in the form of additional pin states. The pcie part does 
not do any pin-handling of its own.


> +- 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
> +- assigned-clocks, assigned-clock-parents and assigned-clock-rates:
> standard +		   clock bindings. See ../clock/clock-bindings.txt

Again that (assigned-clocks handling) is not actual part of the pci-
controllers actions, but other parts and also described already elsewhere.


Heiko

Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Shawn Lin]

On 2016/5/20 19:20, Heiko Stuebner wrote:
> Hi Shawn,
>
> Am Freitag, 20. Mai 2016, 18:29:06 schrieb Shawn Lin:
>> This patch add some required and optional properties for Rockchip
>> PCIe controller. Also we add a example for how to use it.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>>  .../devicetree/bindings/pci/rockchip-pcie.txt      | 93
>> ++++++++++++++++++++++ 1 file changed, 93 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..69a0804
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>> @@ -0,0 +1,93 @@
>> +* 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: The first entry must be "axi-base" for the core registers
>> +	The second entry must be "apb-base" for the client pcie registers
>> +- 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_pcie"
>> +	- "aclk_perf_pcie"
>> +	- "hclk_pcie"
>> +	- "clk_pciephy_ref"
>
> clock names are always in the scope of the node/driver, so the names could
> easily be "aclk", "aclk-perf", "hclk", "phy".
>
> Although from my cursory-glance at the phy-related code, it looks more like
> it shouldn't be in the pcie-driver itself, but in a an actual phy-driver
> (together with the phy reset and clock)?
>
>> +- interrupts: Three interrupt entries must be specified.
>> +- interrupt-names: Must include the following names
>> +	- "pcie-sys"
>> +	- "pcie-legacy"
>> +	- "pcie-client"
>
> Same as above, names could simply be "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
>> +	- "phy-rst"
>> +	- "core-rst"
>> +	- "mgmt-rst"
>> +	- "mgmt-sticky-rst"
>> +	- "pipe-rst"
>
> and again (= without the "-rst")
>

okay, will simplify clk, int and rst mentioned above.

>
>> +- rockchip,grf: phandle to the syscon managing the "general register
>> files"
>
>> +- pcie-conf: offset of pcie client block for  configuration
>> +- pcie-status: offset of pcie client block for status
>> +- pcie-laneoff: offset of pcie client block for lane
>
> These are GRF offsets (GRF_SOC_CON8, GRF_SOC_CON5_PCIE, GRF_SOC_STATUS1)
> Those registers are generally prone to change (even their layout) for future
> socs and I'd suggest instead of declaring them in the devicetree, move them
> to the per-soc data in the rockchip_pcie_of_match struct.

sure.

>
> But it looks as if they're pretty phy-specific, so see comment about possible
> separate phy driver above.
>
>
>> +- msi-parent: Link to the hardware entity that serves as the Message
>> +- pinctrl-names : The pin control state names
>> +- pinctrl-0: The "default" pinctrl state
>
> I'm not sure if pinctrl-properties need to be described when you don't need
> special handling in the form of additional pin states. The pcie part does
> not do any pin-handling of its own.
>

We need it in prevention of any firmwares change the default state
of #CLKREQ which is useful for ASPM. Also we have a backup pin for
clkreqn called clkreqnb, which should be taken more consideration since
when refering to any one of these two, pinctrl should configure the
bit[14] of GRF_SOC_CON7 automatically. But it is unfortunately beyound
the implementation of pinctrl-rk3399.

BTW, I don't know if we wanna support this action inside the pinctrl
code?

>
>> +- 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
>> +- assigned-clocks, assigned-clock-parents and assigned-clock-rates:
>> standard +		   clock bindings. See ../clock/clock-bindings.txt
>
> Again that (assigned-clocks handling) is not actual part of the pci-
> controllers actions, but other parts and also described already elsewhere.
>

Basically it does. But this is an alternative choice for pcie-phy to
generate the ref_clk. When we want 100MHz src clk for PLL inside the
pcie-phy，we should add them, otherwise it's taken from xin 24MHz.

This is useful for SI testing or some others special cases. So should we
add it as an option and leave a sample here?

>
> Heiko
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Heiko Stuebner]

Am Samstag, 21. Mai 2016, 11:55:35 schrieb Shawn Lin:
> On 2016/5/20 19:20, Heiko Stuebner wrote:
> > Hi Shawn,
> > 
> > Am Freitag, 20. Mai 2016, 18:29:06 schrieb Shawn Lin:
> >> This patch add some required and optional properties for Rockchip
> >> PCIe controller. Also we add a example for how to use it.
> >> 
> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> 
> >> ---

[...]

> >> +- msi-parent: Link to the hardware entity that serves as the Message
> >> +- pinctrl-names : The pin control state names
> >> +- pinctrl-0: The "default" pinctrl state
> > 
> > I'm not sure if pinctrl-properties need to be described when you don't
> > need special handling in the form of additional pin states. The pcie
> > part does not do any pin-handling of its own.
> 
> We need it in prevention of any firmwares change the default state
> of #CLKREQ which is useful for ASPM. Also we have a backup pin for
> clkreqn called clkreqnb, which should be taken more consideration since
> when refering to any one of these two, pinctrl should configure the
> bit[14] of GRF_SOC_CON7 automatically. But it is unfortunately beyound
> the implementation of pinctrl-rk3399.
> 
> BTW, I don't know if we wanna support this action inside the pinctrl
> code?

The TRM says for me for that bit only "pcie_clkreq_sel port control" and 
that naming really suggests that it is a property of the pcie controller, 
not the generic pinctrl. So if this needs to be touched the pcie controller 
needs to do it.


> >> +- 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
> >> +- assigned-clocks, assigned-clock-parents and assigned-clock-rates:
> >> standard +		   clock bindings. See ../clock/clock-bindings.txt
> > 
> > Again that (assigned-clocks handling) is not actual part of the pci-
> > controllers actions, but other parts and also described already
> > elsewhere.
> Basically it does. But this is an alternative choice for pcie-phy to
> generate the ref_clk. When we want 100MHz src clk for PLL inside the
> pcie-phy，we should add them, otherwise it's taken from xin 24MHz.
> 
> This is useful for SI testing or some others special cases. So should we
> add it as an option and leave a sample here?

What I meant was that while clock handling is important when looking at the 
whole system, the pcie controller itself does only care that it gets a 
clock, but not that much where you get it from.

So while assigned-clocks has its place in the real devicetree, I don't think 
it is an element of the actual pcie-controller binding.


Heiko

Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Shawn Lin]

On 2016/5/24 3:53, Heiko Stuebner wrote:
> Am Samstag, 21. Mai 2016, 11:55:35 schrieb Shawn Lin:
>> On 2016/5/20 19:20, Heiko Stuebner wrote:
>>> Hi Shawn,
>>>
>>> Am Freitag, 20. Mai 2016, 18:29:06 schrieb Shawn Lin:
>>>> This patch add some required and optional properties for Rockchip
>>>> PCIe controller. Also we add a example for how to use it.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>
>>>> ---
>
> [...]
>
>>>> +- msi-parent: Link to the hardware entity that serves as the Message
>>>> +- pinctrl-names : The pin control state names
>>>> +- pinctrl-0: The "default" pinctrl state
>>>
>>> I'm not sure if pinctrl-properties need to be described when you don't
>>> need special handling in the form of additional pin states. The pcie
>>> part does not do any pin-handling of its own.
>>
>> We need it in prevention of any firmwares change the default state
>> of #CLKREQ which is useful for ASPM. Also we have a backup pin for
>> clkreqn called clkreqnb, which should be taken more consideration since
>> when refering to any one of these two, pinctrl should configure the
>> bit[14] of GRF_SOC_CON7 automatically. But it is unfortunately beyound
>> the implementation of pinctrl-rk3399.
>>
>> BTW, I don't know if we wanna support this action inside the pinctrl
>> code?
>
> The TRM says for me for that bit only "pcie_clkreq_sel port control" and
> that naming really suggests that it is a property of the pcie controller,
> not the generic pinctrl. So if this needs to be touched the pcie controller
> needs to do it.

I don't agree that pcie controller should do it. As a common driver, it
should not care two much setting related to io selection which is very
likely to be changed in the future Socs. Shuld it always keep a
reference to bit[ABC] of GRF_SOC_CONXYZ, and should it adds some code
to see which IO is selected for #CLKREQ?

Currently I do it in firmware, but it's worth to make some discussion
as there are also some IO backup slelections the GRF of RK3399. Anyway,
let's skip this topic from the $SUBJECT patch.

>
>
>>>> +- 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
>>>> +- assigned-clocks, assigned-clock-parents and assigned-clock-rates:
>>>> standard +		   clock bindings. See ../clock/clock-bindings.txt
>>>
>>> Again that (assigned-clocks handling) is not actual part of the pci-
>>> controllers actions, but other parts and also described already
>>> elsewhere.
>> Basically it does. But this is an alternative choice for pcie-phy to
>> generate the ref_clk. When we want 100MHz src clk for PLL inside the
>> pcie-phy，we should add them, otherwise it's taken from xin 24MHz.
>>
>> This is useful for SI testing or some others special cases. So should we
>> add it as an option and leave a sample here?
>
> What I meant was that while clock handling is important when looking at the
> whole system, the pcie controller itself does only care that it gets a
> clock, but not that much where you get it from.
>
> So while assigned-clocks has its place in the real devicetree, I don't think
> it is an element of the actual pcie-controller binding.

Oh, I see.. So it seems good to keep all the assigned-clocks in on
place. I will remove it from this patch.

>
>
> Heiko
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Marc Zyngier]

On Fri, 20 May 2016 18:29:06 +0800
Shawn Lin <shawn.lin@rock-chips.com> wrote:

> This patch add some required and optional properties for Rockchip
> PCIe controller. Also we add a example for how to use it.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
>  .../devicetree/bindings/pci/rockchip-pcie.txt      | 93 ++++++++++++++++++++++
>  1 file changed, 93 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..69a0804
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -0,0 +1,93 @@
> +* 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: The first entry must be "axi-base" for the core registers
> +	The second entry must be "apb-base" for the client pcie registers
> +- 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_pcie"
> +	- "aclk_perf_pcie"
> +	- "hclk_pcie"
> +	- "clk_pciephy_ref"
> +- interrupts: Three interrupt entries must be specified.
> +- interrupt-names: Must include the following names
> +	- "pcie-sys"
> +	- "pcie-legacy"
> +	- "pcie-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
> +	- "phy-rst"
> +	- "core-rst"
> +	- "mgmt-rst"
> +	- "mgmt-sticky-rst"
> +	- "pipe-rst"
> +- rockchip,grf: phandle to the syscon managing the "general register files"
> +- pcie-conf: offset of pcie client block for  configuration
> +- pcie-status: offset of pcie client block for status
> +- pcie-laneoff: offset of pcie client block for lane
> +- msi-parent: Link to the hardware entity that serves as the Message
> +- 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
> +- assigned-clocks, assigned-clock-parents and assigned-clock-rates: standard
> +		   clock bindings. See ../clock/clock-bindings.txt
> +
> +Example:
> +
> +pci_express: axi-pcie@f8000000 {
> +	#address-cells = <3>;
> +	#size-cells = <2>;
> +	compatible = "rockchip,rk3399-pcie";
> +	clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
> +		 <&cru PCLK_PCIE>, <&cru SCLK_PCIEPHY_REF>;
> +	clock-names = "aclk_pcie", "aclk_perf_pcie",
> +		      "hclk_pcie", "clk_pciephy_ref";
> +	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: "pcie-sys", "pcie-legacy", "pcie-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 = < 0x82000000 0 0xfa000000 0x0 0xfa000000 0 0x600000
> +		   0x81000000 0 0xfa600000 0x0 0xfa600000 0 0x100000 >;
> +	num-lanes = <4>;
> +	reg = < 0x0 0xf8000000 0x0 0x2000000 >, < 0x0 0xfd000000 0x0 0x1000000 >;
> +	reg-name = "axi-base", "apb-base";
> +	resets = <&cru SRST_PCIEPHY>, <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
> +		 <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>;
> +	reset-names = "phy-rst", "core-rst", "mgmt-rst", "mgmt-sticky-rst", "pipe-rst";
> +	rockchip,grf = <&grf>;
> +	pcie-conf = <0xe220>;
> +	pcie-status = <0xe2a4>;
> +	pcie-laneoff = <0xe214>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie_clkreq>;
> +	msi-parent = <&its>;

We've moved away from bare msi-parent for the ITS (notice that the
binding mandates #msi-cells = <1>). So what you need here is an msi-map
(as described in Documentation/devicetree/bindings/pci/pci-msi.txt).

Do you have an IOMMU (or some other remapping HW) between the RC and the
GIC ITS? If so, yo may have to describe the mappings between the PCIe
RID and the ITS DevID in this msi-map.

> +	#interrupt-cells = <1>;
> +	interrupt-map-mask = <0 0 0 7>;
> +	interrupt-map = <0 0 0 1 &pcie_0 1>,
> +			<0 0 0 2 &pcie_0 2>,
> +			<0 0 0 3 &pcie_0 3>,
> +			<0 0 0 4 &pcie_0 4>;
> +	pcie_0: interrupt-controller {
> +		interrupt-controller;
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +	};
> +
> +};

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH 1/2] Documentation: add binding description of Rockchip PCIe controller

[Marc Zyngier]

On 31/05/16 10:48, Shawn Lin wrote:
> Hi Marc,
> 
> 在 2016/5/30 19:08, Marc Zyngier 写道:
>> On Fri, 20 May 2016 18:29:06 +0800
>> Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>>> This patch add some required and optional properties for Rockchip
>>> PCIe controller. Also we add a example for how to use it.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>
>>> ---
>>>
>>>  .../devicetree/bindings/pci/rockchip-pcie.txt      | 93 ++++++++++++++++++++++
>>>  1 file changed, 93 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..69a0804
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
>>> @@ -0,0 +1,93 @@
>>> +* 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: The first entry must be "axi-base" for the core registers
>>> +	The second entry must be "apb-base" for the client pcie registers
>>> +- 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_pcie"
>>> +	- "aclk_perf_pcie"
>>> +	- "hclk_pcie"
>>> +	- "clk_pciephy_ref"
>>> +- interrupts: Three interrupt entries must be specified.
>>> +- interrupt-names: Must include the following names
>>> +	- "pcie-sys"
>>> +	- "pcie-legacy"
>>> +	- "pcie-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
>>> +	- "phy-rst"
>>> +	- "core-rst"
>>> +	- "mgmt-rst"
>>> +	- "mgmt-sticky-rst"
>>> +	- "pipe-rst"
>>> +- rockchip,grf: phandle to the syscon managing the "general register files"
>>> +- pcie-conf: offset of pcie client block for  configuration
>>> +- pcie-status: offset of pcie client block for status
>>> +- pcie-laneoff: offset of pcie client block for lane
>>> +- msi-parent: Link to the hardware entity that serves as the Message
>>> +- 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
>>> +- assigned-clocks, assigned-clock-parents and assigned-clock-rates: standard
>>> +		   clock bindings. See ../clock/clock-bindings.txt
>>> +
>>> +Example:
>>> +
>>> +pci_express: axi-pcie@f8000000 {
>>> +	#address-cells = <3>;
>>> +	#size-cells = <2>;
>>> +	compatible = "rockchip,rk3399-pcie";
>>> +	clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
>>> +		 <&cru PCLK_PCIE>, <&cru SCLK_PCIEPHY_REF>;
>>> +	clock-names = "aclk_pcie", "aclk_perf_pcie",
>>> +		      "hclk_pcie", "clk_pciephy_ref";
>>> +	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: "pcie-sys", "pcie-legacy", "pcie-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 = < 0x82000000 0 0xfa000000 0x0 0xfa000000 0 0x600000
>>> +		   0x81000000 0 0xfa600000 0x0 0xfa600000 0 0x100000 >;
>>> +	num-lanes = <4>;
>>> +	reg = < 0x0 0xf8000000 0x0 0x2000000 >, < 0x0 0xfd000000 0x0 0x1000000 >;
>>> +	reg-name = "axi-base", "apb-base";
>>> +	resets = <&cru SRST_PCIEPHY>, <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
>>> +		 <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>;
>>> +	reset-names = "phy-rst", "core-rst", "mgmt-rst", "mgmt-sticky-rst", "pipe-rst";
>>> +	rockchip,grf = <&grf>;
>>> +	pcie-conf = <0xe220>;
>>> +	pcie-status = <0xe2a4>;
>>> +	pcie-laneoff = <0xe214>;
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&pcie_clkreq>;
>>> +	msi-parent = <&its>;
>>
>> We've moved away from bare msi-parent for the ITS (notice that the
>> binding mandates #msi-cells = <1>). So what you need here is an msi-map
>> (as described in Documentation/devicetree/bindings/pci/pci-msi.txt).
>>
>> Do you have an IOMMU (or some other remapping HW) between the RC and the
>> GIC ITS? If so, yo may have to describe the mappings between the PCIe
>> RID and the ITS DevID in this msi-map.
>>
> 
> yes, we don't need msi-parent here. And we don't need msi-map as well,
> since we don't have a IOMMU block between RC and ITS which need to be
> described in msi-map.

Well, if have neither msi-parent nor msi-map, you won't get MSI at all.

Maybe that's good enough for you, but in that case, make sure you drop
all the MSI code from your patches! ;-) More seriously, you do need an
msi-map, even if that's with a single entry.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Shawn Lin]

RK3399 has a PCIe controller which can be used as Root Complex.
This driver supports a PCIe controller as Root Complex mode.

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

 drivers/pci/host/Kconfig         |   12 +
 drivers/pci/host/Makefile        |    1 +
 drivers/pci/host/pcie-rockchip.c | 1181 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 1194 insertions(+)
 create mode 100644 drivers/pci/host/pcie-rockchip.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 8e4f038..76447a8 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -245,4 +245,16 @@ config PCIE_ARMADA_8K
 	  Designware hardware and therefore the driver re-uses the
 	  Designware core functions to implement the driver.
 
+config PCIE_ROCKCHIP
+	bool "Rockchip PCIe controller"
+	depends on ARM64 && ARCH_ROCKCHIP
+	depends on OF
+	select MFD_SYSCON
+	select PCI_MSI
+	select PCI_MSI_IRQ_DOMAIN
+	help
+	  Say Y here if you want internal PCI support on Rockchip SoC.
+	  There are 1 internal PCIe port available to support GEN2 with
+	  4 slots.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index a6f85e3..fb3871e 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
 obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
+obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
new file mode 100644
index 0000000..4063fd3
--- /dev/null
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -0,0 +1,1181 @@
+/*
+ * Rockchip AXI PCIe host controller driver
+ *
+ * Copyright (c) 2016 Rockchip, Inc.
+ *
+ * Author: Shawn Lin <shawn.lin@rock-chips.com>
+ *         Wenrui Li <wenrui.li@rock-chips.com>
+ * Bits taken from Synopsys Designware Host controller driver and
+ * ARM PCI Host generic driver.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+
+#define REF_CLK_100MHZ			(100 * 1000 * 1000)
+#define PCIE_CLIENT_BASE		0x0
+#define PCIE_RC_CONFIG_BASE		0xa00000
+#define PCIE_CORE_CTRL_MGMT_BASE	0x900000
+#define PCIE_CORE_AXI_CONF_BASE		0xc00000
+#define PCIE_CORE_AXI_INBOUND_BASE	0xc00800
+
+#define PCIE_CLIENT_BASIC_STATUS0	0x44
+#define PCIE_CLIENT_BASIC_STATUS1	0x48
+#define PCIE_CLIENT_INT_MASK		0x4c
+#define PCIE_CLIENT_INT_STATUS		0x50
+#define PCIE_CORE_INT_MASK		0x900210
+#define PCIE_CORE_INT_STATUS		0x90020c
+
+/** Size of one AXI Region (not Region 0) */
+#define	AXI_REGION_SIZE			(0x1 << 20)
+/** Overall size of AXI area */
+#define	AXI_OVERALL_SIZE		(64 * (0x1 << 20))
+/** Size of Region 0, equal to sum of sizes of other regions */
+#define	AXI_REGION_0_SIZE		(32 * (0x1 << 20))
+#define OB_REG_SIZE_SHIFT		5
+#define IB_ROOT_PORT_REG_SIZE_SHIFT	3
+
+#define AXI_WRAPPER_IO_WRITE		0x6
+#define AXI_WRAPPER_MEM_WRITE		0x2
+#define MAX_AXI_IB_ROOTPORT_REGION_NUM	3
+#define	MIN_AXI_ADDR_BITS_PASSED	8
+
+#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK	GENMASK(8, 5)
+#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT	5
+#define CLIENT_INTERRUPTS \
+		(LOC_INT | INTA | INTB | INTC | INTD |\
+		 CORR_ERR | NFATAL_ERR | FATAL_ERR | DPA_INT | \
+		 HOT_RESET | MSG_DONE | LEGACY_DONE)
+#define CORE_INTERRUPTS	\
+		(PRFPE | CRFPE | RRPE | CRFO | RT | RTR | \
+		 PE | MTR | UCR | FCE | CT | UTC | MMVC)
+#define PWR_STCG			BIT(0)
+#define HOT_PLUG			BIT(1)
+#define PHY_INT				BIT(2)
+#define UDMA_INT			BIT(3)
+#define LOC_INT				BIT(4)
+#define INTA				BIT(5)
+#define INTB				BIT(6)
+#define INTC				BIT(7)
+#define INTD				BIT(8)
+#define CORR_ERR			BIT(9)
+#define NFATAL_ERR			BIT(10)
+#define FATAL_ERR			BIT(11)
+#define DPA_INT				BIT(12)
+#define HOT_RESET			BIT(13)
+#define MSG_DONE			BIT(14)
+#define LEGACY_DONE			BIT(15)
+#define PRFPE				BIT(0)
+#define CRFPE				BIT(1)
+#define RRPE				BIT(2)
+#define PRFO				BIT(3)
+#define CRFO				BIT(4)
+#define RT				BIT(5)
+#define RTR				BIT(6)
+#define PE				BIT(7)
+#define MTR				BIT(8)
+#define UCR				BIT(9)
+#define FCE				BIT(10)
+#define CT				BIT(11)
+#define UTC				BIT(18)
+#define MMVC				BIT(19)
+
+#define PCIE_ECAM_BUS(x)		(((x) & 0xFF) << 20)
+#define PCIE_ECAM_DEV(x)		(((x) & 0x1F) << 15)
+#define PCIE_ECAM_FUNC(x)		(((x) & 0x7) << 12)
+#define PCIE_ECAM_REG(x)		(((x) & 0xFFF) << 0)
+#define PCIE_ECAM_ADDR(bus, dev, func, reg) \
+	  (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \
+	   PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg))
+
+#define RC_REGION_0_ADDR_TRANS_H	0x00000000
+#define RC_REGION_0_ADDR_TRANS_L	0x00000000
+#define RC_REGION_0_PASS_BITS		(25 - 1)
+#define RC_REGION_1_ADDR_TRANS_H	0x00000000
+#define RC_REGION_1_ADDR_TRANS_L	0x00400000
+#define RC_REGION_1_PASS_BITS		(20 - 1)
+#define MAX_AXI_WRAPPER_REGION_NUM	33
+#define PCIE_CLIENT_CONF_ENABLE		BIT(0)
+#define PCIE_CLIENT_CONF_LANE_NUM(x)	((x / 2) << 4)
+#define PCIE_CLIENT_MODE_RC		BIT(6)
+#define PCIE_CLIENT_GEN_SEL_2		BIT(7)
+#define PCIE_CLIENT_GEN_SEL_1		0x0
+
+struct rockchip_pcie_port {
+	void __iomem *reg_base;
+	void __iomem *apb_base;
+	struct regmap *grf;
+	unsigned int pcie_conf;
+	unsigned int pcie_status;
+	unsigned int pcie_laneoff;
+	struct reset_control *phy_rst;
+	struct reset_control *core_rst;
+	struct reset_control *mgmt_rst;
+	struct reset_control *mgmt_sticky_rst;
+	struct reset_control *pipe_rst;
+	struct clk *aclk_pcie;
+	struct clk *aclk_perf_pcie;
+	struct clk *hclk_pcie;
+	struct clk *clk_pciephy_ref;
+	struct gpio_desc *ep_gpio;
+	u32 lanes;
+	resource_size_t		io_base;
+	struct resource		*cfg;
+	struct resource		*io;
+	struct resource		*mem;
+	struct resource		*busn;
+	phys_addr_t		io_bus_addr;
+	u32			io_size;
+	phys_addr_t		mem_bus_addr;
+	u32			mem_size;
+	u8	root_bus_nr;
+	int irq;
+	struct msi_controller *msi;
+
+	struct device *dev;
+	struct irq_domain *irq_domain;
+};
+
+static inline u32 pcie_read(struct rockchip_pcie_port *port, u32 reg)
+{
+	return readl(port->apb_base + reg);
+}
+
+static inline void pcie_write(struct rockchip_pcie_port *port,
+			      u32 val, u32 reg)
+{
+	writel(val, port->apb_base + reg);
+}
+
+static inline void pcie_pb_wr_cfg(struct rockchip_pcie_port *port,
+				  u32 addr, u32 data)
+{
+	regmap_write(port->grf, port->pcie_conf,
+		     (0x3ff << 17) | (data << 7) | (addr << 1));
+	udelay(1);
+	regmap_write(port->grf, port->pcie_conf,
+		     (0x1 << 16) | (0x1 << 0));
+	udelay(1);
+	regmap_write(port->grf, port->pcie_conf,
+		     (0x1 << 16) | (0x0 << 0));
+}
+
+static inline u32 pcie_pb_rd_cfg(struct rockchip_pcie_port *port,
+				 u32 addr)
+{
+	u32 val;
+
+	regmap_write(port->grf, port->pcie_conf,
+		     (0x3ff << 17) | (addr << 1));
+	regmap_read(port->grf, port->pcie_status, &val);
+	return val;
+}
+
+static int rockchip_pcie_valid_config(struct rockchip_pcie_port *pp,
+				      struct pci_bus *bus, int dev)
+{
+	/* access only one slot on each root port */
+	if (bus->number == pp->root_bus_nr && dev > 0)
+		return 0;
+
+	/*
+	 * do not read more than one device on the bus directly attached
+	 * to RC's (Virtual Bridge's) DS side.
+	 */
+	if (bus->primary == pp->root_bus_nr && dev > 0)
+		return 0;
+
+	return 1;
+}
+
+static int rockchip_pcie_rd_own_conf(struct rockchip_pcie_port *pp,
+				     int where, int size,
+				     u32 *val)
+{
+	void __iomem *addr = pp->apb_base + PCIE_RC_CONFIG_BASE + where;
+
+	if ((uintptr_t)addr & (size - 1)) {
+		*val = 0;
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	if (size == 4) {
+		*val = readl(addr);
+	} else if (size == 2) {
+		*val = readw(addr);
+	} else if (size == 1) {
+		*val = readb(addr);
+	} else {
+		*val = 0;
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp,
+				     int where, int size, u32 val)
+{
+	u32 tmp;
+	int offset;
+
+	offset = (where & (~0x3));
+	tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
+	if (size == 4) {
+		writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where);
+	} else if (size == 2) {
+		if (where & 0x2)
+			tmp = ((tmp & 0xffff) | (val << 16));
+		else
+			tmp = ((tmp & 0xffff0000) | val);
+
+		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
+	} else if (size == 1) {
+		if ((where & 0x3) == 0)
+			tmp = ((tmp & (~0xff)) | val);
+		else if ((where & 0x3) == 1)
+			tmp = ((tmp & (~0xff00)) | (val << 8));
+		else if ((where & 0x3) == 2)
+			tmp = ((tmp & (~0xff0000)) | (val << 16));
+		else if ((where & 0x3) == 3)
+			tmp = ((tmp & (~0xff000000)) | (val << 24));
+
+		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
+	} else {
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
+				       struct pci_bus *bus, u32 devfn,
+				       int where, int size, u32 *val)
+{
+	u32 busdev;
+
+	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), where);
+
+	if (busdev & (size - 1)) {
+		*val = 0;
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	if (size == 4) {
+		*val = readl(pp->reg_base + busdev);
+	} else if (size == 2) {
+		*val = readw(pp->reg_base + busdev);
+	} else if (size == 1) {
+		*val = readb(pp->reg_base + busdev);
+	} else {
+		*val = 0;
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int rockchip_pcie_wr_other_conf(struct rockchip_pcie_port *pp,
+				       struct pci_bus *bus, u32 devfn,
+				       int where, int size, u32 val)
+{
+	u32 busdev;
+
+	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), where);
+	if (busdev & (size - 1))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	if (size == 4)
+		writel(val, pp->reg_base + busdev);
+	else if (size == 2)
+		writew(val, pp->reg_base + busdev);
+	else if (size == 1)
+		writeb(val, pp->reg_base + busdev);
+	else
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
+				 int size, u32 *val)
+{
+	struct rockchip_pcie_port *pp = bus->sysdata;
+	int ret;
+
+	if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
+		*val = 0xffffffff;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	if (bus->number != pp->root_bus_nr)
+		ret = rockchip_pcie_rd_other_conf(pp, bus, devfn,
+						  where, size, val);
+	else
+		ret = rockchip_pcie_rd_own_conf(pp, where, size, val);
+
+	return ret;
+}
+
+static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
+				 int where, int size, u32 val)
+{
+	struct rockchip_pcie_port *pp = bus->sysdata;
+	int ret;
+
+	if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	if (bus->number != pp->root_bus_nr)
+		ret = rockchip_pcie_wr_other_conf(pp, bus, devfn,
+						  where, size, val);
+	else
+		ret = rockchip_pcie_wr_own_conf(pp, where, size, val);
+
+	return ret;
+}
+
+static struct pci_ops rockchip_pcie_ops = {
+	.read = rockchip_pcie_rd_conf,
+	.write = rockchip_pcie_wr_conf,
+};
+
+/**
+ * rockchip_pcie_init_port - Initialize hardware
+ * @port: PCIe port information
+ */
+static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
+{
+	int err;
+	u32 status;
+	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
+
+	gpiod_set_value(port->ep_gpio, 0);
+
+	/* Make sure PCIe relate block is in reset state */
+	err = reset_control_assert(port->phy_rst);
+	if (err) {
+		dev_err(port->dev, "assert phy_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_assert(port->core_rst);
+	if (err) {
+		dev_err(port->dev, "assert core_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_assert(port->mgmt_rst);
+	if (err) {
+		dev_err(port->dev, "assert mgmt_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_assert(port->mgmt_sticky_rst);
+	if (err) {
+		dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_assert(port->pipe_rst);
+	if (err) {
+		dev_err(port->dev, "assert pipe_rst err %d\n", err);
+		return err;
+	}
+
+	pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 |
+			  (0x1 << 19) | (0x1 << 3) |
+			  PCIE_CLIENT_MODE_RC |
+			  PCIE_CLIENT_CONF_LANE_NUM(port->lanes) |
+			  PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE);
+
+	err = reset_control_deassert(port->phy_rst);
+	if (err) {
+		dev_err(port->dev, "deassert phy_rst err %d\n", err);
+		return err;
+	}
+	regmap_write(port->grf, port->pcie_conf,
+		     (0x3f << 17) | (0x10 << 1));
+	err = -EINVAL;
+	while (time_before(jiffies, timeout)) {
+		regmap_read(port->grf, port->pcie_status, &status);
+		if ((status & (1 << 9))) {
+			dev_info(port->dev, "pll locked!\n");
+			err = 0;
+			break;
+		}
+	}
+	if (err) {
+		dev_err(port->dev, "pll lock timeout!\n");
+		return err;
+	}
+	pcie_pb_wr_cfg(port, 0x10, 0x8);
+	pcie_pb_wr_cfg(port, 0x12, 0x8);
+
+	err = -ETIMEDOUT;
+	while (time_before(jiffies, timeout)) {
+		regmap_read(port->grf, port->pcie_status, &status);
+		if (!(status & (1 << 10))) {
+			dev_info(port->dev, "pll output enable done!\n");
+			err = 0;
+			break;
+		}
+	}
+
+	if (err) {
+		dev_err(port->dev, "pll output enable timeout!\n");
+		return err;
+	}
+
+	regmap_write(port->grf, port->pcie_conf,
+		     (0x3f << 17) | (0x10 << 1));
+	err = -EINVAL;
+	while (time_before(jiffies, timeout)) {
+		regmap_read(port->grf, port->pcie_status, &status);
+		if ((status & (1 << 9))) {
+			dev_info(port->dev, "pll relocked!\n");
+			err = 0;
+			break;
+		}
+	}
+	if (err) {
+		dev_err(port->dev, "pll relock timeout!\n");
+		return err;
+	}
+
+	err = reset_control_deassert(port->core_rst);
+	if (err) {
+		dev_err(port->dev, "deassert core_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_deassert(port->mgmt_rst);
+	if (err) {
+		dev_err(port->dev, "deassert mgmt_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_deassert(port->mgmt_sticky_rst);
+	if (err) {
+		dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err);
+		return err;
+	}
+	err = reset_control_deassert(port->pipe_rst);
+	if (err) {
+		dev_err(port->dev, "deassert pipe_rst err %d\n", err);
+		return err;
+	}
+
+	pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE);
+
+	gpiod_set_value(port->ep_gpio, 1);
+	err = -ETIMEDOUT;
+	while (time_before(jiffies, timeout)) {
+		status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1);
+		if (((status >> 20) & 0x3) == 0x3) {
+			dev_info(port->dev, "pcie link training gen1 pass!\n");
+			err = 0;
+			break;
+		}
+	}
+	if (err) {
+		dev_err(port->dev, "pcie link training gen1 timeout!\n");
+		return err;
+	}
+
+	status = pcie_read(port, 0x9000d0);
+	status |= 0x20;
+	pcie_write(port, status, 0x9000d0);
+	err = -ETIMEDOUT;
+	while (time_before(jiffies, timeout)) {
+		status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
+		if (((status >> 3) & 0x3) == 0x1) {
+			dev_info(port->dev, "pcie link training gen2 pass!\n");
+			err = 0;
+			break;
+		}
+	}
+	if (err)
+		dev_dbg(port->dev, "pcie link training gen2 timeout, force to gen1!\n");
+
+	if (((status >> 3) & 0x3) == 0x0)
+		dev_info(port->dev, "pcie link 2.5!\n");
+	if (((status >> 3) & 0x3) == 0x1)
+		dev_info(port->dev, "pcie link 5.0!\n");
+
+	status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
+	status =  0x1 << ((status >> 1) & 0x3);
+	dev_info(port->dev, "current link width is x%d\n", status);
+
+	status = pcie_pb_rd_cfg(port, 0x30);
+	if (!((status >> 11) & 0x1))
+		dev_dbg(port->dev, "lane A is used\n");
+	else
+		regmap_write(port->grf, port->pcie_laneoff,
+			     (0x1 << 19) | (0x1 << 3));
+
+	status = pcie_pb_rd_cfg(port, 0x31);
+	if (!((status >> 11) & 0x1))
+		dev_dbg(port->dev, "lane B is used\n");
+	else
+		regmap_write(port->grf, port->pcie_laneoff,
+			     (0x2 << 19) | (0x2 << 3));
+
+	status = pcie_pb_rd_cfg(port, 0x32);
+	if (!((status >> 11) & 0x1))
+		dev_dbg(port->dev, "lane C is used\n");
+	else
+		regmap_write(port->grf, port->pcie_laneoff,
+			     (0x4 << 19) | (0x4 << 3));
+
+	status = pcie_pb_rd_cfg(port, 0x33);
+	if (!((status >> 11) & 0x1))
+		dev_dbg(port->dev, "lane D is used\n");
+	else
+		regmap_write(port->grf, port->pcie_laneoff,
+			     (0x8 << 19) | (0x8 << 3));
+	return 0;
+}
+
+/**
+ * rockchip_pcie_parse_dt - Parse Device tree
+ * @port: PCIe port information
+ *
+ * Return: '0' on success and error value on failure
+ */
+static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port)
+{
+	struct device *dev = port->dev;
+	struct device_node *node = dev->of_node;
+	struct resource regs;
+	unsigned int pcie_conf;
+	unsigned int pcie_status;
+	unsigned int pcie_laneoff;
+	int err;
+
+	err = of_address_to_resource(node, 0, &regs);
+	if (err) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return err;
+	}
+
+	port->reg_base = devm_ioremap_resource(dev, &regs);
+	if (IS_ERR(port->reg_base))
+		return PTR_ERR(port->reg_base);
+
+	err = of_address_to_resource(node, 1, &regs);
+	if (err) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return err;
+	}
+
+	port->apb_base = devm_ioremap_resource(dev, &regs);
+	if (IS_ERR(port->apb_base))
+		return PTR_ERR(port->apb_base);
+
+	port->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
+						    "rockchip,grf");
+	if (IS_ERR(port->grf)) {
+		dev_err(dev, "Missing rockchip,grf property\n");
+		return PTR_ERR(port->grf);
+	}
+
+	if (of_property_read_u32(node, "pcie-conf", &pcie_conf)) {
+		dev_err(dev, "missing pcie-conf property in node %s\n",
+			node->name);
+		return -EINVAL;
+	}
+
+	port->pcie_conf = pcie_conf;
+
+	if (of_property_read_u32(node, "pcie-status", &pcie_status)) {
+		dev_err(dev, "missing pcie-status property in node %s\n",
+			node->name);
+		return -EINVAL;
+	}
+
+	port->pcie_status = pcie_status;
+
+	if (of_property_read_u32(node, "pcie-laneoff", &pcie_laneoff)) {
+		dev_err(dev, "missing pcie-laneoff property in node %s\n",
+			node->name);
+		return -EINVAL;
+	}
+
+	port->pcie_laneoff = pcie_laneoff;
+
+	port->lanes = 1;
+	err = of_property_read_u32(node, "num-lanes", &port->lanes);
+	if (!err && ((port->lanes == 0) ||
+		     (port->lanes == 3) ||
+		     (port->lanes > 4))) {
+		dev_info(dev, "invalid num-lanes, default use one lane\n");
+		port->lanes = 1;
+	}
+
+	port->phy_rst = devm_reset_control_get(dev, "phy-rst");
+	if (IS_ERR(port->phy_rst)) {
+		if (PTR_ERR(port->phy_rst) != -EPROBE_DEFER)
+			dev_err(dev, "missing phy-rst property in node %s\n",
+				node->name);
+		err = PTR_ERR(port->phy_rst);
+		goto err_aclk_pcie;
+	}
+
+	port->core_rst = devm_reset_control_get(dev, "core-rst");
+	if (IS_ERR(port->core_rst)) {
+		if (PTR_ERR(port->core_rst) != -EPROBE_DEFER)
+			dev_err(dev, "missing core-rst property in node %s\n",
+				node->name);
+		err = PTR_ERR(port->core_rst);
+		goto err_aclk_pcie;
+	}
+
+	port->mgmt_rst = devm_reset_control_get(dev, "mgmt-rst");
+	if (IS_ERR(port->mgmt_rst)) {
+		if (PTR_ERR(port->mgmt_rst) != -EPROBE_DEFER)
+			dev_err(dev, "missing mgmt-rst property in node %s\n",
+				node->name);
+		err = PTR_ERR(port->mgmt_rst);
+		goto err_aclk_pcie;
+	}
+
+	port->mgmt_sticky_rst = devm_reset_control_get(dev, "mgmt-sticky-rst");
+	if (IS_ERR(port->mgmt_sticky_rst)) {
+		if (PTR_ERR(port->mgmt_sticky_rst) != -EPROBE_DEFER)
+			dev_err(dev, "missing mgmt-sticky-rst property in node %s\n",
+				node->name);
+		err = PTR_ERR(port->mgmt_sticky_rst);
+		goto err_aclk_pcie;
+	}
+
+	port->pipe_rst = devm_reset_control_get(dev, "pipe-rst");
+	if (IS_ERR(port->pipe_rst)) {
+		if (PTR_ERR(port->pipe_rst) != -EPROBE_DEFER)
+			dev_err(dev, "missing pipe-rst property in node %s\n",
+				node->name);
+		err = PTR_ERR(port->pipe_rst);
+		goto err_aclk_pcie;
+	}
+
+	port->ep_gpio = gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
+	if (IS_ERR(port->ep_gpio)) {
+		dev_err(dev, "missing ep-gpios property in node %s\n",
+			node->name);
+		return PTR_ERR(port->ep_gpio);
+	}
+
+	port->aclk_pcie = devm_clk_get(dev, "aclk_pcie");
+	if (IS_ERR(port->aclk_pcie)) {
+		dev_err(dev, "aclk_pcie clock not found.\n");
+		return PTR_ERR(port->aclk_pcie);
+	}
+
+	port->aclk_perf_pcie = devm_clk_get(dev, "aclk_perf_pcie");
+	if (IS_ERR(port->aclk_perf_pcie)) {
+		dev_err(dev, "aclk_perf_pcie clock not found.\n");
+		return PTR_ERR(port->aclk_perf_pcie);
+	}
+
+	port->hclk_pcie = devm_clk_get(dev, "hclk_pcie");
+	if (IS_ERR(port->hclk_pcie)) {
+		dev_err(dev, "hclk_pcie clock not found.\n");
+		return PTR_ERR(port->hclk_pcie);
+	}
+
+	port->clk_pciephy_ref = devm_clk_get(dev, "clk_pciephy_ref");
+	if (IS_ERR(port->clk_pciephy_ref)) {
+		dev_err(dev, "clk_pciephy_ref clock not found.\n");
+		return PTR_ERR(port->clk_pciephy_ref);
+	}
+
+	err = clk_prepare_enable(port->aclk_pcie);
+	if (err) {
+		dev_err(dev, "Unable to enable aclk_pcie clock.\n");
+		goto err_aclk_pcie;
+	}
+
+	err = clk_prepare_enable(port->aclk_perf_pcie);
+	if (err) {
+		dev_err(dev, "Unable to enable aclk_perf_pcie clock.\n");
+		goto err_aclk_perf_pcie;
+	}
+
+	err = clk_prepare_enable(port->hclk_pcie);
+	if (err) {
+		dev_err(dev, "Unable to enable hclk_pcie clock.\n");
+		goto err_hclk_pcie;
+	}
+
+	err = clk_prepare_enable(port->clk_pciephy_ref);
+	if (err) {
+		dev_err(dev, "Unable to enable hclk_pcie clock.\n");
+		goto err_pciephy_ref;
+	}
+
+	return 0;
+
+err_pciephy_ref:
+	clk_disable_unprepare(port->hclk_pcie);
+err_hclk_pcie:
+	clk_disable_unprepare(port->aclk_perf_pcie);
+err_aclk_perf_pcie:
+	clk_disable_unprepare(port->aclk_pcie);
+err_aclk_pcie:
+	return err;
+}
+
+static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp)
+{
+	struct device_node *msi_node;
+
+	msi_node = of_parse_phandle(pp->dev->of_node,
+				    "msi-parent", 0);
+	if (!msi_node)
+		return;
+
+	pp->msi = of_pci_find_msi_chip_by_node(msi_node);
+	of_node_put(msi_node);
+
+	if (pp->msi)
+		pp->msi->dev = pp->dev;
+}
+
+static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *pp)
+{
+	pcie_write(pp, (CLIENT_INTERRUPTS << 16) &
+		   (~CLIENT_INTERRUPTS), PCIE_CLIENT_INT_MASK);
+	pcie_write(pp, CORE_INTERRUPTS, PCIE_CORE_INT_MASK);
+}
+
+static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+				  irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = rockchip_pcie_intx_map,
+};
+
+static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp)
+{
+	struct device *dev = pp->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *pcie_intc_node =  of_get_next_child(node, NULL);
+
+	if (!pcie_intc_node) {
+		dev_err(dev, "No PCIe Intc node found\n");
+		return PTR_ERR(pcie_intc_node);
+	}
+	pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4,
+					       &intx_domain_ops, pp);
+	if (!pp->irq_domain) {
+		dev_err(dev, "Failed to get a INTx IRQ domain\n");
+		return PTR_ERR(pp->irq_domain);
+	}
+
+	return 0;
+}
+
+static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
+{
+	struct rockchip_pcie_port *pp = arg;
+	u32 reg;
+	u32 sub_reg;
+
+	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
+	if (reg & LOC_INT) {
+		dev_dbg(pp->dev, "local interrupt recived\n");
+		sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
+		if (sub_reg & PRFPE)
+			dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
+
+		if (sub_reg & CRFPE)
+			dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
+
+		if (sub_reg & RRPE)
+			dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
+
+		if (sub_reg & PRFO)
+			dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
+
+		if (sub_reg & CRFO)
+			dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
+
+		if (sub_reg & RT)
+			dev_dbg(pp->dev, "Replay timer timed out\n");
+
+		if (sub_reg & RTR)
+			dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
+
+		if (sub_reg & PE)
+			dev_dbg(pp->dev, "Phy error detected on receive side\n");
+
+		if (sub_reg & MTR)
+			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
+
+		if (sub_reg & UCR)
+			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
+
+		if (sub_reg & FCE)
+			dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
+
+		if (sub_reg & CT)
+			dev_dbg(pp->dev, "A request timed out waiting for completion\n");
+
+		if (sub_reg & UTC)
+			dev_dbg(pp->dev, "Unmapped TC error\n");
+
+		if (sub_reg & MMVC)
+			dev_dbg(pp->dev, "MSI mask register changes\n");
+
+		pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
+	}
+
+	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg)
+{
+	struct rockchip_pcie_port *pp = arg;
+	u32 reg;
+
+	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
+	if (reg & LEGACY_DONE)
+		dev_dbg(pp->dev, "legacy done interrupt recived\n");
+
+	if (reg & MSG_DONE)
+		dev_dbg(pp->dev, "message done interrupt recived\n");
+
+	if (reg & HOT_RESET)
+		dev_dbg(pp->dev, "hot reset interrupt recived\n");
+
+	if (reg & DPA_INT)
+		dev_dbg(pp->dev, "dpa interrupt recived\n");
+
+	if (reg & FATAL_ERR)
+		dev_dbg(pp->dev, "fatal error interrupt recived\n");
+
+	if (reg & DPA_INT)
+		dev_dbg(pp->dev, "no fatal error interrupt recived\n");
+
+	if (reg & CORR_ERR)
+		dev_dbg(pp->dev, "correctable error interrupt recived\n");
+
+	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg)
+{
+	struct rockchip_pcie_port *pp = arg;
+	u32 reg;
+
+	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
+	reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
+	       ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
+	generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));
+
+	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
+	return IRQ_HANDLED;
+}
+
+static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie_port *pp,
+				     int region_no,
+				     int type, u8 num_pass_bits,
+				     u32 lower_addr, u32 upper_addr)
+{
+	u32 ob_addr_0 = 0;
+	u32 ob_addr_1 = 0;
+	u32 ob_desc_0 = 0;
+	u32 ob_desc_1 = 0;
+	void __iomem *aw_base;
+
+	if (!pp)
+		return -EINVAL;
+	if (region_no >= MAX_AXI_WRAPPER_REGION_NUM)
+		return -EINVAL;
+	if ((num_pass_bits + 1) < 8)
+		return -EINVAL;
+	if (num_pass_bits > 63)
+		return -EINVAL;
+	if (region_no == 0) {
+		if (AXI_REGION_0_SIZE < (2ULL << num_pass_bits))
+		return -EINVAL;
+	}
+	if (region_no != 0) {
+		if (AXI_REGION_SIZE < (2ULL << num_pass_bits))
+		return -EINVAL;
+	}
+	aw_base = pp->apb_base + PCIE_CORE_AXI_CONF_BASE;
+	aw_base += (region_no << OB_REG_SIZE_SHIFT);
+
+	ob_addr_0 = (ob_addr_0 &
+		     ~0x0000003fU) | (num_pass_bits &
+		     0x0000003fU);
+	ob_addr_0 = (ob_addr_0 &
+		     ~0xffffff00U) | (lower_addr & 0xffffff00U);
+	ob_addr_1 = upper_addr;
+	ob_desc_0 = (1 << 23 | type);
+
+	writel(ob_addr_0, aw_base);
+	writel(ob_addr_1, aw_base + 0x4);
+	writel(ob_desc_0, aw_base + 0x8);
+	writel(ob_desc_1, aw_base + 0xc);
+
+	return 0;
+}
+
+static int rockchip_pcie_prog_ib_atu(struct rockchip_pcie_port *pp,
+				     int region_no,
+				     u8 num_pass_bits,
+				     u32 lower_addr,
+				     u32 upper_addr)
+{
+	u32 ib_addr_0 = 0;
+	u32 ib_addr_1 = 0;
+	void __iomem *aw_base;
+
+	if (!pp)
+		return -EINVAL;
+	if (region_no > MAX_AXI_IB_ROOTPORT_REGION_NUM)
+		return -EINVAL;
+	if ((num_pass_bits + 1) < MIN_AXI_ADDR_BITS_PASSED)
+		return -EINVAL;
+	if (num_pass_bits > 63)
+		return -EINVAL;
+	aw_base = pp->apb_base + PCIE_CORE_AXI_INBOUND_BASE;
+	aw_base += (region_no << IB_ROOT_PORT_REG_SIZE_SHIFT);
+	ib_addr_0 = (ib_addr_0 &
+		     ~0x0000003fU) | (num_pass_bits &
+		     0x0000003fU);
+
+	ib_addr_0 = (ib_addr_0 & ~0xffffff00U) |
+		     ((lower_addr << 8) & 0xffffff00U);
+	ib_addr_1 = upper_addr;
+	writel(ib_addr_0, aw_base);
+	writel(ib_addr_1, aw_base + 0x4);
+
+	return 0;
+}
+
+static int rockchip_pcie_probe(struct platform_device *pdev)
+{
+	struct rockchip_pcie_port *port;
+	struct device *dev = &pdev->dev;
+	struct pci_bus *bus, *child;
+	struct resource_entry *win;
+	int reg_no;
+	int err = 0;
+	int irq;
+	LIST_HEAD(res);
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	irq = platform_get_irq_byname(pdev, "pcie-sys");
+	if (irq < 0) {
+		dev_err(dev, "missing pcie_sys IRQ resource\n");
+		return -EINVAL;
+	}
+	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
+			       IRQF_SHARED, "pcie-sys", port);
+	if (err) {
+		dev_err(dev, "failed to request pcie subsystem irq\n");
+		return err;
+	}
+
+	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
+	if (port->irq < 0) {
+		dev_err(dev, "missing pcie_legacy IRQ resource\n");
+		return -EINVAL;
+	}
+	err = devm_request_irq(dev, port->irq,
+			       rockchip_pcie_legacy_int_handler,
+			       IRQF_SHARED,
+			       "pcie-legacy",
+			       port);
+	if (err) {
+		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
+		return err;
+	}
+
+	irq = platform_get_irq_byname(pdev, "pcie-client");
+	if (irq < 0) {
+		dev_err(dev, "missing pcie-client IRQ resource\n");
+		return -EINVAL;
+	}
+	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
+			       IRQF_SHARED, "pcie-client", port);
+	if (err) {
+		dev_err(dev, "failed to request pcie client irq\n");
+		return err;
+	}
+
+	port->dev = dev;
+	err = rockchip_pcie_parse_dt(port);
+	if (err) {
+		dev_err(dev, "Parsing DT failed\n");
+		return err;
+	}
+
+	err = rockchip_pcie_init_port(port);
+	if (err)
+		return err;
+
+	platform_set_drvdata(pdev, port);
+
+	rockchip_pcie_enable_interrupts(port);
+	if (!IS_ENABLED(CONFIG_PCI_MSI)) {
+		err = rockchip_pcie_init_irq_domain(port);
+		if (err < 0)
+			return err;
+	}
+
+	err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
+					       &res, &port->io_base);
+	if (err)
+		return err;
+	/* Get the I/O and memory ranges from DT */
+	resource_list_for_each_entry(win, &res) {
+		switch (resource_type(win->res)) {
+		case IORESOURCE_IO:
+			port->io = win->res;
+			port->io->name = "I/O";
+			port->io_size = resource_size(port->io);
+			port->io_bus_addr = port->io->start - win->offset;
+			err = pci_remap_iospace(port->io, port->io_base);
+			if (err) {
+				dev_warn(port->dev, "error %d: failed to map resource %pR\n",
+					 err, port->io);
+				continue;
+			}
+			break;
+		case IORESOURCE_MEM:
+			port->mem = win->res;
+			port->mem->name = "MEM";
+			port->mem_size = resource_size(port->mem);
+			port->mem_bus_addr = port->mem->start - win->offset;
+			break;
+		case 0:
+			port->cfg = win->res;
+			break;
+		case IORESOURCE_BUS:
+			port->busn = win->res;
+			break;
+		default:
+			continue;
+		}
+	}
+
+	pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8);
+	pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300);
+
+	pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + RC_REGION_0_PASS_BITS),
+		   PCIE_CORE_AXI_CONF_BASE);
+	pcie_write(port, RC_REGION_0_ADDR_TRANS_H,
+		   PCIE_CORE_AXI_CONF_BASE + 0x4);
+	pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8);
+	pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc);
+
+	for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) {
+		err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
+						AXI_WRAPPER_MEM_WRITE,
+						20 - 1,
+						port->mem_bus_addr +
+							(reg_no << 20),
+						0);
+		if (err) {
+			dev_err(dev, "Program RC outbound atu failed\n");
+			return err;
+		}
+	}
+
+	err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0);
+	if (err) {
+		dev_err(dev, "Program RC inbound atu failed\n");
+		return err;
+	}
+
+	rockchip_pcie_msi_enable(port);
+
+	port->root_bus_nr = port->busn->start;
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr,
+					    &rockchip_pcie_ops, port, &res,
+					    port->msi);
+	} else {
+		bus = pci_scan_root_bus(&pdev->dev, 0,
+					&rockchip_pcie_ops, port, &res);
+	}
+	if (!bus)
+		return -ENOMEM;
+
+	if (!pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_bus_size_bridges(bus);
+		pci_bus_assign_resources(bus);
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+	}
+
+	pci_bus_add_devices(bus);
+
+	return err;
+}
+
+static int rockchip_pcie_remove(struct platform_device *pdev)
+{
+	struct rockchip_pcie_port *port = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(port->hclk_pcie);
+	clk_disable_unprepare(port->aclk_perf_pcie);
+	clk_disable_unprepare(port->aclk_pcie);
+	clk_disable_unprepare(port->clk_pciephy_ref);
+
+	return 0;
+}
+
+static const struct of_device_id rockchip_pcie_of_match[] = {
+	{ .compatible = "rockchip,rk3399-pcie", },
+	{}
+};
+
+static struct platform_driver rockchip_pcie_driver = {
+	.driver = {
+		.name = "rockchip-pcie",
+		.of_match_table = rockchip_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = rockchip_pcie_probe,
+	.remove = rockchip_pcie_remove,
+};
+module_platform_driver(rockchip_pcie_driver);
+
+MODULE_AUTHOR("Rockchip Inc");
+MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
+MODULE_LICENSE("GPL v2");
-- 
2.3.7

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Heiko Stuebner]

Hi Shawn,

I haven't had any contact with PCI yet, so my comments below will likely 
address more generic findings only.

As you might've guessed from the binding comments, to me it looks like the 
phy handling should be in a separate phy driver and looking below all phy 
accesses seem very separate from the actual pci controller interactions - 
they are even in port_init only as well.

While I already added some comments about that below, the driver seems full 
of raw register bit handling (including wild shifts). Please abstract that 
using contants, so that stuff stays readable for the future.


Am Freitag, 20. Mai 2016, 18:29:16 schrieb Shawn Lin:
> RK3399 has a PCIe controller which can be used as Root Complex.
> This driver supports a PCIe controller as Root Complex mode.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---

[...]

> diff --git a/drivers/pci/host/pcie-rockchip.c
> b/drivers/pci/host/pcie-rockchip.c new file mode 100644
> index 0000000..4063fd3
> --- /dev/null
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -0,0 +1,1181 @@
> +/*
> + * Rockchip AXI PCIe host controller driver
> + *
> + * Copyright (c) 2016 Rockchip, Inc.
> + *
> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
> + *         Wenrui Li <wenrui.li@rock-chips.com>
> + * Bits taken from Synopsys Designware Host controller driver and
> + * ARM PCI Host generic driver.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/regmap.h>
> +
> +#define REF_CLK_100MHZ			(100 * 1000 * 1000)

seems unused

> +#define PCIE_CLIENT_BASE		0x0
> +#define PCIE_RC_CONFIG_BASE		0xa00000
> +#define PCIE_CORE_CTRL_MGMT_BASE	0x900000
> +#define PCIE_CORE_AXI_CONF_BASE		0xc00000
> +#define PCIE_CORE_AXI_INBOUND_BASE	0xc00800
> +
> +#define PCIE_CLIENT_BASIC_STATUS0	0x44
> +#define PCIE_CLIENT_BASIC_STATUS1	0x48
> +#define PCIE_CLIENT_INT_MASK		0x4c
> +#define PCIE_CLIENT_INT_STATUS		0x50
> +#define PCIE_CORE_INT_MASK		0x900210
> +#define PCIE_CORE_INT_STATUS		0x90020c
> +
> +/** Size of one AXI Region (not Region 0) */
> +#define	AXI_REGION_SIZE			(0x1 << 20)

for those generic (1 << x) things please use BIT(x) instead
Also constants intertwined with constants is hard to read,
so ideally add a blank line above each comment and comment style for single 
line comments only has one * in the beginning
	/* foo */

> +/** Overall size of AXI area */
> +#define	AXI_OVERALL_SIZE		(64 * (0x1 << 20))
> +/** Size of Region 0, equal to sum of sizes of other regions */
> +#define	AXI_REGION_0_SIZE		(32 * (0x1 << 20))
> +#define OB_REG_SIZE_SHIFT		5
> +#define IB_ROOT_PORT_REG_SIZE_SHIFT	3
> +
> +#define AXI_WRAPPER_IO_WRITE		0x6
> +#define AXI_WRAPPER_MEM_WRITE		0x2
> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM	3
> +#define	MIN_AXI_ADDR_BITS_PASSED	8

strange spacing after #define

[...]

> +/**
> + * rockchip_pcie_init_port - Initialize hardware
> + * @port: PCIe port information
> + */
> +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
> +{
> +	int err;
> +	u32 status;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(1000);

this timeout only seems to be initialized once here but used for multiple 
loops below resulting in all waitloops combined together being allowed to 
take 1 second ... is this intended?

> +	gpiod_set_value(port->ep_gpio, 0);
> +
> +	/* Make sure PCIe relate block is in reset state */
> +	err = reset_control_assert(port->phy_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert phy_rst err %d\n", err);
> +		return err;
> +	}

should move to phy driver probe or so

> +	err = reset_control_assert(port->core_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert core_rst err %d\n", err);
> +		return err;
> +	}

blank line

> +	err = reset_control_assert(port->mgmt_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert mgmt_rst err %d\n", err);
> +		return err;
> +	}

blank line

> +	err = reset_control_assert(port->mgmt_sticky_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err);
> +		return err;
> +	}

blank line

> +	err = reset_control_assert(port->pipe_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert pipe_rst err %d\n", err);
> +		return err;
> +	}
> +
> +	pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 |
> +			  (0x1 << 19) | (0x1 << 3) |
> +			  PCIE_CLIENT_MODE_RC |
> +			  PCIE_CLIENT_CONF_LANE_NUM(port->lanes) |
> +			  PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE);
> +

------ phy_enable begin ------

As mentioned above, the whole phy handling seems pretty separate, so should 
be easily being able to live in a real phy driver?

This of course also needs to handle the phy clock in it.

> +	err = reset_control_deassert(port->phy_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert phy_rst err %d\n", err);
> +		return err;
> +	}
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x3f << 17) | (0x10 << 1));

the bits above should use some sort of constant / description. Also see 
HIWORD_UPDATE in other drivers for the write-enable mask

also add blank line here

> +	err = -EINVAL;
> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if ((status & (1 << 9))) {

use a constant for the (1 << 9) [aka BIT(9)] please

> +			dev_info(port->dev, "pll locked!\n");

dev_dbg instead?

> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err) {
> +		dev_err(port->dev, "pll lock timeout!\n");
> +		return err;
> +	}
> +	pcie_pb_wr_cfg(port, 0x10, 0x8);
> +	pcie_pb_wr_cfg(port, 0x12, 0x8);
> +
> +	err = -ETIMEDOUT;
> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if (!(status & (1 << 10))) {

constant again

> +			dev_info(port->dev, "pll output enable done!\n");

dev_dbg or leave it out

> +			err = 0;
> +			break;
> +		}
> +	}
> +
> +	if (err) {
> +		dev_err(port->dev, "pll output enable timeout!\n");
> +		return err;
> +	}
> +
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x3f << 17) | (0x10 << 1));
> +	err = -EINVAL;
> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if ((status & (1 << 9))) {
> +			dev_info(port->dev, "pll relocked!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err) {
> +		dev_err(port->dev, "pll relock timeout!\n");
> +		return err;
> +	}

------ phy_enable end ------


> +	err = reset_control_deassert(port->core_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert core_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_deassert(port->mgmt_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert mgmt_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_deassert(port->mgmt_sticky_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_deassert(port->pipe_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert pipe_rst err %d\n", err);
> +		return err;
> +	}
> +
> +	pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE);
> +
> +	gpiod_set_value(port->ep_gpio, 1);
> +	err = -ETIMEDOUT;
> +	while (time_before(jiffies, timeout)) {
> +		status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1);
> +		if (((status >> 20) & 0x3) == 0x3) {
> +			dev_info(port->dev, "pcie link training gen1 pass!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err) {
> +		dev_err(port->dev, "pcie link training gen1 timeout!\n");
> +		return err;
> +	}
> +
> +	status = pcie_read(port, 0x9000d0);
> +	status |= 0x20;
> +	pcie_write(port, status, 0x9000d0);

just to mention it again, bit handling as descriptive constant please and 
also please give that register a name :-)

[...]

> +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port)
> +{

[...]

> +	port->lanes = 1;
> +	err = of_property_read_u32(node, "num-lanes", &port->lanes);
> +	if (!err && ((port->lanes == 0) ||
> +		     (port->lanes == 3) ||
> +		     (port->lanes > 4))) {
> +		dev_info(dev, "invalid num-lanes, default use one lane\n");

dev_warn might be more appropriate

> +		port->lanes = 1;
> +	}

[...]

> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp)
> +{
> +	struct device_node *msi_node;
> +
> +	msi_node = of_parse_phandle(pp->dev->of_node,
> +				    "msi-parent", 0);

I would assume this should live in the general parse_dt function?
Also is that MSI enablement really allow to fail silently without any affect 
on the PCI functionality?

> +	if (!msi_node)
> +		return;
> +
> +	pp->msi = of_pci_find_msi_chip_by_node(msi_node);
> +	of_node_put(msi_node);
> +
> +	if (pp->msi)
> +		pp->msi->dev = pp->dev;
> +}

[...]

> +static int rockchip_pcie_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_pcie_port *port;
> +	struct device *dev = &pdev->dev;
> +	struct pci_bus *bus, *child;
> +	struct resource_entry *win;
> +	int reg_no;
> +	int err = 0;
> +	int irq;
> +	LIST_HEAD(res);
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-sys");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie_sys IRQ resource\n");
> +		return -EINVAL;
> +	}

blank line

> +	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
> +			       IRQF_SHARED, "pcie-sys", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie subsystem irq\n");
> +		return err;
> +	}
> +
> +	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
> +	if (port->irq < 0) {
> +		dev_err(dev, "missing pcie_legacy IRQ resource\n");
> +		return -EINVAL;
> +	}

blank line

> +	err = devm_request_irq(dev, port->irq,
> +			       rockchip_pcie_legacy_int_handler,
> +			       IRQF_SHARED,
> +			       "pcie-legacy",
> +			       port);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
> +		return err;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-client");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie-client IRQ resource\n");
> +		return -EINVAL;
> +	}

blank line

> +	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
> +			       IRQF_SHARED, "pcie-client", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie client irq\n");
> +		return err;
> +	}
> +
> +	port->dev = dev;
> +	err = rockchip_pcie_parse_dt(port);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}

rockchip_pcie_parse_dt already emits error messages that are a lot more 
specific to the actual problem, so you don't need another error message here 
and can just return the error code.


> +	err = rockchip_pcie_init_port(port);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, port);
> +
> +	rockchip_pcie_enable_interrupts(port);
> +	if (!IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = rockchip_pcie_init_irq_domain(port);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
> +					       &res, &port->io_base);
> +	if (err)
> +		return err;

add blank line


Heiko

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Shawn Lin]

On 2016/5/21 5:13, Heiko Stuebner wrote:
> Hi Shawn,
>
> I haven't had any contact with PCI yet, so my comments below will likely
> address more generic findings only.
>
> As you might've guessed from the binding comments, to me it looks like the
> phy handling should be in a separate phy driver and looking below all phy
> accesses seem very separate from the actual pci controller interactions -
> they are even in port_init only as well.
>

yes, the main reason for not to seperate a new pcie-phy driver for this
prototype design is that I just wonder whether it's worth to create a
new driver for just a small piece of code. And a bit more forward is
that I think phy api is no so scalable for just init/power_on in
case of too much interactions between controller and phy from which I
suffer a bit for emmc.

Should I really need to seperate the phy part into pcie-phy? ;)

> While I already added some comments about that below, the driver seems full
> of raw register bit handling (including wild shifts). Please abstract that
> using contants, so that stuff stays readable for the future.

okay, let me migrate these magic number and raw reg bit handling into a
new header file.

>
>
> Am Freitag, 20. Mai 2016, 18:29:16 schrieb Shawn Lin:
>> RK3399 has a PCIe controller which can be used as Root Complex.
>> This driver supports a PCIe controller as Root Complex mode.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>
> [...]
>
>> diff --git a/drivers/pci/host/pcie-rockchip.c
>> b/drivers/pci/host/pcie-rockchip.c new file mode 100644
>> index 0000000..4063fd3
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -0,0 +1,1181 @@
>> +/*
>> + * Rockchip AXI PCIe host controller driver
>> + *
>> + * Copyright (c) 2016 Rockchip, Inc.
>> + *
>> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
>> + *         Wenrui Li <wenrui.li@rock-chips.com>
>> + * Bits taken from Synopsys Designware Host controller driver and
>> + * ARM PCI Host generic driver.
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/msi.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/regmap.h>
>> +
>> +#define REF_CLK_100MHZ			(100 * 1000 * 1000)
>
> seems unused

will be removed.

>
>> +#define PCIE_CLIENT_BASE		0x0
>> +#define PCIE_RC_CONFIG_BASE		0xa00000
>> +#define PCIE_CORE_CTRL_MGMT_BASE	0x900000
>> +#define PCIE_CORE_AXI_CONF_BASE		0xc00000
>> +#define PCIE_CORE_AXI_INBOUND_BASE	0xc00800
>> +
>> +#define PCIE_CLIENT_BASIC_STATUS0	0x44
>> +#define PCIE_CLIENT_BASIC_STATUS1	0x48
>> +#define PCIE_CLIENT_INT_MASK		0x4c
>> +#define PCIE_CLIENT_INT_STATUS		0x50
>> +#define PCIE_CORE_INT_MASK		0x900210
>> +#define PCIE_CORE_INT_STATUS		0x90020c
>> +
>> +/** Size of one AXI Region (not Region 0) */
>> +#define	AXI_REGION_SIZE			(0x1 << 20)
>
> for those generic (1 << x) things please use BIT(x) instead
> Also constants intertwined with constants is hard to read,
> so ideally add a blank line above each comment and comment style for single
> line comments only has one * in the beginning

Ahh yep.

> 	/* foo */
>
>> +/** Overall size of AXI area */
>> +#define	AXI_OVERALL_SIZE		(64 * (0x1 << 20))
>> +/** Size of Region 0, equal to sum of sizes of other regions */
>> +#define	AXI_REGION_0_SIZE		(32 * (0x1 << 20))
>> +#define OB_REG_SIZE_SHIFT		5
>> +#define IB_ROOT_PORT_REG_SIZE_SHIFT	3
>> +
>> +#define AXI_WRAPPER_IO_WRITE		0x6
>> +#define AXI_WRAPPER_MEM_WRITE		0x2
>> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM	3
>> +#define	MIN_AXI_ADDR_BITS_PASSED	8
>
> strange spacing after #define

Generally I use one space after #define, but I donnot somehow...
I will check it twice.

>
> [...]
>
>> +/**
>> + * rockchip_pcie_init_port - Initialize hardware
>> + * @port: PCIe port information
>> + */
>> +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
>> +{
>> +	int err;
>> +	u32 status;
>> +	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>
> this timeout only seems to be initialized once here but used for multiple
> loops below resulting in all waitloops combined together being allowed to
> take 1 second ... is this intended?

a big timeout value to make sure we leave enough margin. No any data
is provided about how long should we wait for pll lock/re-lock/tainning
finish stuff. As it also related to the Socs/devices. Currently I
test pci-ethernet/wifi/SATA-bridge/USB-bridge, and it seems can be
reduced. But as you know, I cannot guarantee not to augment it once we
find some SSD/GPU in the failure state of trainning in the future.
Anyway I think here we use loop-break method, so it should be not
harmful?

>
>> +	gpiod_set_value(port->ep_gpio, 0);
>> +
>> +	/* Make sure PCIe relate block is in reset state */
>> +	err = reset_control_assert(port->phy_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "assert phy_rst err %d\n", err);
>> +		return err;
>> +	}
>
> should move to phy driver probe or so
>
>> +	err = reset_control_assert(port->core_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "assert core_rst err %d\n", err);
>> +		return err;
>> +	}
>
> blank line
>
>> +	err = reset_control_assert(port->mgmt_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "assert mgmt_rst err %d\n", err);
>> +		return err;
>> +	}
>
> blank line
>
>> +	err = reset_control_assert(port->mgmt_sticky_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err);
>> +		return err;
>> +	}
>
> blank line
>
>> +	err = reset_control_assert(port->pipe_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "assert pipe_rst err %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 |
>> +			  (0x1 << 19) | (0x1 << 3) |
>> +			  PCIE_CLIENT_MODE_RC |
>> +			  PCIE_CLIENT_CONF_LANE_NUM(port->lanes) |
>> +			  PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE);
>> +
>
> ------ phy_enable begin ------
>
> As mentioned above, the whole phy handling seems pretty separate, so should
> be easily being able to live in a real phy driver?
>
> This of course also needs to handle the phy clock in it.
>
>> +	err = reset_control_deassert(port->phy_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "deassert phy_rst err %d\n", err);
>> +		return err;
>> +	}
>> +	regmap_write(port->grf, port->pcie_conf,
>> +		     (0x3f << 17) | (0x10 << 1));
>
> the bits above should use some sort of constant / description. Also see
> HIWORD_UPDATE in other drivers for the write-enable mask
>
> also add blank line here
>
>> +	err = -EINVAL;
>> +	while (time_before(jiffies, timeout)) {
>> +		regmap_read(port->grf, port->pcie_status, &status);
>> +		if ((status & (1 << 9))) {
>
> use a constant for the (1 << 9) [aka BIT(9)] please
>
>> +			dev_info(port->dev, "pll locked!\n");
>
> dev_dbg instead?

yep

>
>> +			err = 0;
>> +			break;
>> +		}
>> +	}
>> +	if (err) {
>> +		dev_err(port->dev, "pll lock timeout!\n");
>> +		return err;
>> +	}
>> +	pcie_pb_wr_cfg(port, 0x10, 0x8);
>> +	pcie_pb_wr_cfg(port, 0x12, 0x8);
>> +
>> +	err = -ETIMEDOUT;
>> +	while (time_before(jiffies, timeout)) {
>> +		regmap_read(port->grf, port->pcie_status, &status);
>> +		if (!(status & (1 << 10))) {
>
> constant again
>
>> +			dev_info(port->dev, "pll output enable done!\n");
>
> dev_dbg or leave it out

dev_dbg should be fine.

>
>> +			err = 0;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (err) {
>> +		dev_err(port->dev, "pll output enable timeout!\n");
>> +		return err;
>> +	}
>> +
>> +	regmap_write(port->grf, port->pcie_conf,
>> +		     (0x3f << 17) | (0x10 << 1));
>> +	err = -EINVAL;
>> +	while (time_before(jiffies, timeout)) {
>> +		regmap_read(port->grf, port->pcie_status, &status);
>> +		if ((status & (1 << 9))) {
>> +			dev_info(port->dev, "pll relocked!\n");
>> +			err = 0;
>> +			break;
>> +		}
>> +	}
>> +	if (err) {
>> +		dev_err(port->dev, "pll relock timeout!\n");
>> +		return err;
>> +	}
>
> ------ phy_enable end ------
>
>
>> +	err = reset_control_deassert(port->core_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "deassert core_rst err %d\n", err);
>> +		return err;
>> +	}
>> +	err = reset_control_deassert(port->mgmt_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "deassert mgmt_rst err %d\n", err);
>> +		return err;
>> +	}
>> +	err = reset_control_deassert(port->mgmt_sticky_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err);
>> +		return err;
>> +	}
>> +	err = reset_control_deassert(port->pipe_rst);
>> +	if (err) {
>> +		dev_err(port->dev, "deassert pipe_rst err %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE);
>> +
>> +	gpiod_set_value(port->ep_gpio, 1);
>> +	err = -ETIMEDOUT;
>> +	while (time_before(jiffies, timeout)) {
>> +		status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1);
>> +		if (((status >> 20) & 0x3) == 0x3) {
>> +			dev_info(port->dev, "pcie link training gen1 pass!\n");
>> +			err = 0;
>> +			break;
>> +		}
>> +	}
>> +	if (err) {
>> +		dev_err(port->dev, "pcie link training gen1 timeout!\n");
>> +		return err;
>> +	}
>> +
>> +	status = pcie_read(port, 0x9000d0);
>> +	status |= 0x20;
>> +	pcie_write(port, status, 0x9000d0);
>
> just to mention it again, bit handling as descriptive constant please and
> also please give that register a name :-)

will address this magic number all above using a new header file :)

>
> [...]
>
>> +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port)
>> +{
>
> [...]
>
>> +	port->lanes = 1;
>> +	err = of_property_read_u32(node, "num-lanes", &port->lanes);
>> +	if (!err && ((port->lanes == 0) ||
>> +		     (port->lanes == 3) ||
>> +		     (port->lanes > 4))) {
>> +		dev_info(dev, "invalid num-lanes, default use one lane\n");
>
> dev_warn might be more appropriate

sure.

>
>> +		port->lanes = 1;
>> +	}
>
> [...]
>
>> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp)
>> +{
>> +	struct device_node *msi_node;
>> +
>> +	msi_node = of_parse_phandle(pp->dev->of_node,
>> +				    "msi-parent", 0);
>
> I would assume this should live in the general parse_dt function?
> Also is that MSI enablement really allow to fail silently without any affect
> on the PCI functionality?

yes, we should check this as well as CONFIG_PCI_MSI.
Will fix it.


>
>> +	if (!msi_node)
>> +		return;
>> +
>> +	pp->msi = of_pci_find_msi_chip_by_node(msi_node);
>> +	of_node_put(msi_node);
>> +
>> +	if (pp->msi)
>> +		pp->msi->dev = pp->dev;
>> +}
>
> [...]
>
>> +static int rockchip_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct rockchip_pcie_port *port;
>> +	struct device *dev = &pdev->dev;
>> +	struct pci_bus *bus, *child;
>> +	struct resource_entry *win;
>> +	int reg_no;
>> +	int err = 0;
>> +	int irq;
>> +	LIST_HEAD(res);
>> +
>> +	if (!dev->of_node)
>> +		return -ENODEV;
>> +
>> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> +	if (!port)
>> +		return -ENOMEM;
>> +
>> +	irq = platform_get_irq_byname(pdev, "pcie-sys");
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing pcie_sys IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>
> blank line
>
>> +	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
>> +			       IRQF_SHARED, "pcie-sys", port);
>> +	if (err) {
>> +		dev_err(dev, "failed to request pcie subsystem irq\n");
>> +		return err;
>> +	}
>> +
>> +	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
>> +	if (port->irq < 0) {
>> +		dev_err(dev, "missing pcie_legacy IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>
> blank line
>
>> +	err = devm_request_irq(dev, port->irq,
>> +			       rockchip_pcie_legacy_int_handler,
>> +			       IRQF_SHARED,
>> +			       "pcie-legacy",
>> +			       port);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
>> +		return err;
>> +	}
>> +
>> +	irq = platform_get_irq_byname(pdev, "pcie-client");
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing pcie-client IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>
> blank line
>
>> +	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
>> +			       IRQF_SHARED, "pcie-client", port);
>> +	if (err) {
>> +		dev_err(dev, "failed to request pcie client irq\n");
>> +		return err;
>> +	}
>> +
>> +	port->dev = dev;
>> +	err = rockchip_pcie_parse_dt(port);
>> +	if (err) {
>> +		dev_err(dev, "Parsing DT failed\n");
>> +		return err;
>> +	}
>
> rockchip_pcie_parse_dt already emits error messages that are a lot more
> specific to the actual problem, so you don't need another error message here
> and can just return the error code.
>

okay.

>
>> +	err = rockchip_pcie_init_port(port);
>> +	if (err)
>> +		return err;
>> +
>> +	platform_set_drvdata(pdev, port);
>> +
>> +	rockchip_pcie_enable_interrupts(port);
>> +	if (!IS_ENABLED(CONFIG_PCI_MSI)) {
>> +		err = rockchip_pcie_init_irq_domain(port);
>> +		if (err < 0)
>> +			return err;
>> +	}
>> +
>> +	err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
>> +					       &res, &port->io_base);
>> +	if (err)
>> +		return err;
>
> add blank line
>
>
> Heiko
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Shawn Lin]

On 2016/5/23 8:48, Shawn Lin wrote:
> On 2016/5/21 5:13, Heiko Stuebner wrote:
>> Hi Shawn,
>>
>> I haven't had any contact with PCI yet, so my comments below will likely
>> address more generic findings only.
>>
>> As you might've guessed from the binding comments, to me it looks like
>> the
>> phy handling should be in a separate phy driver and looking below all phy
>> accesses seem very separate from the actual pci controller interactions -
>> they are even in port_init only as well.
>>
>
> yes, the main reason for not to seperate a new pcie-phy driver for this
> prototype design is that I just wonder whether it's worth to create a
> new driver for just a small piece of code. And a bit more forward is
> that I think phy api is no so scalable for just init/power_on in
> case of too much interactions between controller and phy from which I
> suffer a bit for emmc.
>
> Should I really need to seperate the phy part into pcie-phy? ;)

Currently, We still need some more interactions between phy and 
controller here, not just what you point out in the comments. so if we
need a seperate phy driver, could it allows me to export some functions
for pcie driver?


>
>> While I already added some comments about that below, the driver seems
>> full
>> of raw register bit handling (including wild shifts). Please abstract
>> that
>> using contants, so that stuff stays readable for the future.
>
> okay, let me migrate these magic number and raw reg bit handling into a
> new header file.
>
>>
>>
>> Am Freitag, 20. Mai 2016, 18:29:16 schrieb Shawn Lin:
>>> RK3399 has a PCIe controller which can be used as Root Complex.
>>> This driver supports a PCIe controller as Root Complex mode.
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>> ---
>>
>> [...]
>>
>>> diff --git a/drivers/pci/host/pcie-rockchip.c
>>> b/drivers/pci/host/pcie-rockchip.c new file mode 100644
>>> index 0000000..4063fd3
>>> --- /dev/null
>>> +++ b/drivers/pci/host/pcie-rockchip.c
>>> @@ -0,0 +1,1181 @@
>>> +/*
>>> + * Rockchip AXI PCIe host controller driver
>>> + *
>>> + * Copyright (c) 2016 Rockchip, Inc.
>>> + *
>>> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
>>> + *         Wenrui Li <wenrui.li@rock-chips.com>
>>> + * Bits taken from Synopsys Designware Host controller driver and
>>> + * ARM PCI Host generic driver.
>>> + *
>>> + * This program is free software: you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation, either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + */
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/msi.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_pci.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/reset.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +#define REF_CLK_100MHZ            (100 * 1000 * 1000)
>>
>> seems unused
>
> will be removed.
>
>>
>>> +#define PCIE_CLIENT_BASE        0x0
>>> +#define PCIE_RC_CONFIG_BASE        0xa00000
>>> +#define PCIE_CORE_CTRL_MGMT_BASE    0x900000
>>> +#define PCIE_CORE_AXI_CONF_BASE        0xc00000
>>> +#define PCIE_CORE_AXI_INBOUND_BASE    0xc00800
>>> +
>>> +#define PCIE_CLIENT_BASIC_STATUS0    0x44
>>> +#define PCIE_CLIENT_BASIC_STATUS1    0x48
>>> +#define PCIE_CLIENT_INT_MASK        0x4c
>>> +#define PCIE_CLIENT_INT_STATUS        0x50
>>> +#define PCIE_CORE_INT_MASK        0x900210
>>> +#define PCIE_CORE_INT_STATUS        0x90020c
>>> +
>>> +/** Size of one AXI Region (not Region 0) */
>>> +#define    AXI_REGION_SIZE            (0x1 << 20)
>>
>> for those generic (1 << x) things please use BIT(x) instead
>> Also constants intertwined with constants is hard to read,
>> so ideally add a blank line above each comment and comment style for
>> single
>> line comments only has one * in the beginning
>
> Ahh yep.
>
>>     /* foo */
>>
>>> +/** Overall size of AXI area */
>>> +#define    AXI_OVERALL_SIZE        (64 * (0x1 << 20))
>>> +/** Size of Region 0, equal to sum of sizes of other regions */
>>> +#define    AXI_REGION_0_SIZE        (32 * (0x1 << 20))
>>> +#define OB_REG_SIZE_SHIFT        5
>>> +#define IB_ROOT_PORT_REG_SIZE_SHIFT    3
>>> +
>>> +#define AXI_WRAPPER_IO_WRITE        0x6
>>> +#define AXI_WRAPPER_MEM_WRITE        0x2
>>> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM    3
>>> +#define    MIN_AXI_ADDR_BITS_PASSED    8
>>
>> strange spacing after #define
>
> Generally I use one space after #define, but I donnot somehow...
> I will check it twice.
>
>>
>> [...]
>>
>>> +/**
>>> + * rockchip_pcie_init_port - Initialize hardware
>>> + * @port: PCIe port information
>>> + */
>>> +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
>>> +{
>>> +    int err;
>>> +    u32 status;
>>> +    unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>>
>> this timeout only seems to be initialized once here but used for multiple
>> loops below resulting in all waitloops combined together being allowed to
>> take 1 second ... is this intended?
>
> a big timeout value to make sure we leave enough margin. No any data
> is provided about how long should we wait for pll lock/re-lock/tainning
> finish stuff. As it also related to the Socs/devices. Currently I
> test pci-ethernet/wifi/SATA-bridge/USB-bridge, and it seems can be
> reduced. But as you know, I cannot guarantee not to augment it once we
> find some SSD/GPU in the failure state of trainning in the future.
> Anyway I think here we use loop-break method, so it should be not
> harmful?
>
>>
>>> +    gpiod_set_value(port->ep_gpio, 0);
>>> +
>>> +    /* Make sure PCIe relate block is in reset state */
>>> +    err = reset_control_assert(port->phy_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "assert phy_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>
>> should move to phy driver probe or so
>>
>>> +    err = reset_control_assert(port->core_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "assert core_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>
>> blank line
>>
>>> +    err = reset_control_assert(port->mgmt_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "assert mgmt_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>
>> blank line
>>
>>> +    err = reset_control_assert(port->mgmt_sticky_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>
>> blank line
>>
>>> +    err = reset_control_assert(port->pipe_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "assert pipe_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>> +
>>> +    pcie_write(port, (0xf << 20) | (0x1 << 16) |
>>> PCIE_CLIENT_GEN_SEL_2 |
>>> +              (0x1 << 19) | (0x1 << 3) |
>>> +              PCIE_CLIENT_MODE_RC |
>>> +              PCIE_CLIENT_CONF_LANE_NUM(port->lanes) |
>>> +              PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE);
>>> +
>>
>> ------ phy_enable begin ------
>>
>> As mentioned above, the whole phy handling seems pretty separate, so
>> should
>> be easily being able to live in a real phy driver?
>>
>> This of course also needs to handle the phy clock in it.
>>
>>> +    err = reset_control_deassert(port->phy_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "deassert phy_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>> +    regmap_write(port->grf, port->pcie_conf,
>>> +             (0x3f << 17) | (0x10 << 1));
>>
>> the bits above should use some sort of constant / description. Also see
>> HIWORD_UPDATE in other drivers for the write-enable mask
>>
>> also add blank line here
>>
>>> +    err = -EINVAL;
>>> +    while (time_before(jiffies, timeout)) {
>>> +        regmap_read(port->grf, port->pcie_status, &status);
>>> +        if ((status & (1 << 9))) {
>>
>> use a constant for the (1 << 9) [aka BIT(9)] please
>>
>>> +            dev_info(port->dev, "pll locked!\n");
>>
>> dev_dbg instead?
>
> yep
>
>>
>>> +            err = 0;
>>> +            break;
>>> +        }
>>> +    }
>>> +    if (err) {
>>> +        dev_err(port->dev, "pll lock timeout!\n");
>>> +        return err;
>>> +    }
>>> +    pcie_pb_wr_cfg(port, 0x10, 0x8);
>>> +    pcie_pb_wr_cfg(port, 0x12, 0x8);
>>> +
>>> +    err = -ETIMEDOUT;
>>> +    while (time_before(jiffies, timeout)) {
>>> +        regmap_read(port->grf, port->pcie_status, &status);
>>> +        if (!(status & (1 << 10))) {
>>
>> constant again
>>
>>> +            dev_info(port->dev, "pll output enable done!\n");
>>
>> dev_dbg or leave it out
>
> dev_dbg should be fine.
>
>>
>>> +            err = 0;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (err) {
>>> +        dev_err(port->dev, "pll output enable timeout!\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    regmap_write(port->grf, port->pcie_conf,
>>> +             (0x3f << 17) | (0x10 << 1));
>>> +    err = -EINVAL;
>>> +    while (time_before(jiffies, timeout)) {
>>> +        regmap_read(port->grf, port->pcie_status, &status);
>>> +        if ((status & (1 << 9))) {
>>> +            dev_info(port->dev, "pll relocked!\n");
>>> +            err = 0;
>>> +            break;
>>> +        }
>>> +    }
>>> +    if (err) {
>>> +        dev_err(port->dev, "pll relock timeout!\n");
>>> +        return err;
>>> +    }
>>
>> ------ phy_enable end ------
>>
>>
>>> +    err = reset_control_deassert(port->core_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "deassert core_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>> +    err = reset_control_deassert(port->mgmt_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "deassert mgmt_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>> +    err = reset_control_deassert(port->mgmt_sticky_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>> +    err = reset_control_deassert(port->pipe_rst);
>>> +    if (err) {
>>> +        dev_err(port->dev, "deassert pipe_rst err %d\n", err);
>>> +        return err;
>>> +    }
>>> +
>>> +    pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE);
>>> +
>>> +    gpiod_set_value(port->ep_gpio, 1);
>>> +    err = -ETIMEDOUT;
>>> +    while (time_before(jiffies, timeout)) {
>>> +        status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1);
>>> +        if (((status >> 20) & 0x3) == 0x3) {
>>> +            dev_info(port->dev, "pcie link training gen1 pass!\n");
>>> +            err = 0;
>>> +            break;
>>> +        }
>>> +    }
>>> +    if (err) {
>>> +        dev_err(port->dev, "pcie link training gen1 timeout!\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    status = pcie_read(port, 0x9000d0);
>>> +    status |= 0x20;
>>> +    pcie_write(port, status, 0x9000d0);
>>
>> just to mention it again, bit handling as descriptive constant please and
>> also please give that register a name :-)
>
> will address this magic number all above using a new header file :)
>
>>
>> [...]
>>
>>> +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port)
>>> +{
>>
>> [...]
>>
>>> +    port->lanes = 1;
>>> +    err = of_property_read_u32(node, "num-lanes", &port->lanes);
>>> +    if (!err && ((port->lanes == 0) ||
>>> +             (port->lanes == 3) ||
>>> +             (port->lanes > 4))) {
>>> +        dev_info(dev, "invalid num-lanes, default use one lane\n");
>>
>> dev_warn might be more appropriate
>
> sure.
>
>>
>>> +        port->lanes = 1;
>>> +    }
>>
>> [...]
>>
>>> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp)
>>> +{
>>> +    struct device_node *msi_node;
>>> +
>>> +    msi_node = of_parse_phandle(pp->dev->of_node,
>>> +                    "msi-parent", 0);
>>
>> I would assume this should live in the general parse_dt function?
>> Also is that MSI enablement really allow to fail silently without any
>> affect
>> on the PCI functionality?
>
> yes, we should check this as well as CONFIG_PCI_MSI.
> Will fix it.
>
>
>>
>>> +    if (!msi_node)
>>> +        return;
>>> +
>>> +    pp->msi = of_pci_find_msi_chip_by_node(msi_node);
>>> +    of_node_put(msi_node);
>>> +
>>> +    if (pp->msi)
>>> +        pp->msi->dev = pp->dev;
>>> +}
>>
>> [...]
>>
>>> +static int rockchip_pcie_probe(struct platform_device *pdev)
>>> +{
>>> +    struct rockchip_pcie_port *port;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct pci_bus *bus, *child;
>>> +    struct resource_entry *win;
>>> +    int reg_no;
>>> +    int err = 0;
>>> +    int irq;
>>> +    LIST_HEAD(res);
>>> +
>>> +    if (!dev->of_node)
>>> +        return -ENODEV;
>>> +
>>> +    port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>>> +    if (!port)
>>> +        return -ENOMEM;
>>> +
>>> +    irq = platform_get_irq_byname(pdev, "pcie-sys");
>>> +    if (irq < 0) {
>>> +        dev_err(dev, "missing pcie_sys IRQ resource\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> blank line
>>
>>> +    err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
>>> +                   IRQF_SHARED, "pcie-sys", port);
>>> +    if (err) {
>>> +        dev_err(dev, "failed to request pcie subsystem irq\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
>>> +    if (port->irq < 0) {
>>> +        dev_err(dev, "missing pcie_legacy IRQ resource\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> blank line
>>
>>> +    err = devm_request_irq(dev, port->irq,
>>> +                   rockchip_pcie_legacy_int_handler,
>>> +                   IRQF_SHARED,
>>> +                   "pcie-legacy",
>>> +                   port);
>>> +    if (err) {
>>> +        dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    irq = platform_get_irq_byname(pdev, "pcie-client");
>>> +    if (irq < 0) {
>>> +        dev_err(dev, "missing pcie-client IRQ resource\n");
>>> +        return -EINVAL;
>>> +    }
>>
>> blank line
>>
>>> +    err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
>>> +                   IRQF_SHARED, "pcie-client", port);
>>> +    if (err) {
>>> +        dev_err(dev, "failed to request pcie client irq\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    port->dev = dev;
>>> +    err = rockchip_pcie_parse_dt(port);
>>> +    if (err) {
>>> +        dev_err(dev, "Parsing DT failed\n");
>>> +        return err;
>>> +    }
>>
>> rockchip_pcie_parse_dt already emits error messages that are a lot more
>> specific to the actual problem, so you don't need another error
>> message here
>> and can just return the error code.
>>
>
> okay.
>
>>
>>> +    err = rockchip_pcie_init_port(port);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    platform_set_drvdata(pdev, port);
>>> +
>>> +    rockchip_pcie_enable_interrupts(port);
>>> +    if (!IS_ENABLED(CONFIG_PCI_MSI)) {
>>> +        err = rockchip_pcie_init_irq_domain(port);
>>> +        if (err < 0)
>>> +            return err;
>>> +    }
>>> +
>>> +    err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
>>> +                           &res, &port->io_base);
>>> +    if (err)
>>> +        return err;
>>
>> add blank line
>>
>>
>> Heiko
>>
>>
>>
>
>


-- 
Best Regards
Shawn Lin

RE: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Bharat Kumar Gogada]

> +
> +	irq = platform_get_irq_byname(pdev, "pcie-sys");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie_sys IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
> +			       IRQF_SHARED, "pcie-sys", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie subsystem irq\n");
> +		return err;
> +	}
> +
> +	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
> +	if (port->irq < 0) {
> +		dev_err(dev, "missing pcie_legacy IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, port->irq,
> +			       rockchip_pcie_legacy_int_handler,
> +			       IRQF_SHARED,
> +			       "pcie-legacy",
> +			       port);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
> +		return err;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-client");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie-client IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
> +			       IRQF_SHARED, "pcie-client", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie client irq\n");
> +		return err;
> +	}
> +

All of the above interrupt number details are being obtained from device tree, why not move above API's to rockchip_pcie_parse_dt.

> +	port->dev = dev;
> +	err = rockchip_pcie_parse_dt(port);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +
> +	pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8);
> +	pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300);
> +
> +	pcie_write(port, (RC_REGION_0_ADDR_TRANS_L +
> RC_REGION_0_PASS_BITS),
> +		   PCIE_CORE_AXI_CONF_BASE);
> +	pcie_write(port, RC_REGION_0_ADDR_TRANS_H,
> +		   PCIE_CORE_AXI_CONF_BASE + 0x4);
> +	pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8);
> +	pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc);

Why not move the above steps to rockchip_pcie_init_port function ? 
Please use macros instead of using values.

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Shawn Lin]

Hi Bharat,

On 2016/5/23 23:15, Bharat Kumar Gogada wrote:
>> +
>> +	irq = platform_get_irq_byname(pdev, "pcie-sys");
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing pcie_sys IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>> +	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
>> +			       IRQF_SHARED, "pcie-sys", port);
>> +	if (err) {
>> +		dev_err(dev, "failed to request pcie subsystem irq\n");
>> +		return err;
>> +	}
>> +
>> +	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
>> +	if (port->irq < 0) {
>> +		dev_err(dev, "missing pcie_legacy IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>> +	err = devm_request_irq(dev, port->irq,
>> +			       rockchip_pcie_legacy_int_handler,
>> +			       IRQF_SHARED,
>> +			       "pcie-legacy",
>> +			       port);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
>> +		return err;
>> +	}
>> +
>> +	irq = platform_get_irq_byname(pdev, "pcie-client");
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing pcie-client IRQ resource\n");
>> +		return -EINVAL;
>> +	}
>> +	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
>> +			       IRQF_SHARED, "pcie-client", port);
>> +	if (err) {
>> +		dev_err(dev, "failed to request pcie client irq\n");
>> +		return err;
>> +	}
>> +
>
> All of the above interrupt number details are being obtained from device tree, why not move above API's to rockchip_pcie_parse_dt.

Ok, I will move these above into parse_dt function.

>
>> +	port->dev = dev;
>> +	err = rockchip_pcie_parse_dt(port);
>> +	if (err) {
>> +		dev_err(dev, "Parsing DT failed\n");
>> +		return err;
>> +	}
>> +
>> +	pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8);
>> +	pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300);
>> +
>> +	pcie_write(port, (RC_REGION_0_ADDR_TRANS_L +
>> RC_REGION_0_PASS_BITS),
>> +		   PCIE_CORE_AXI_CONF_BASE);
>> +	pcie_write(port, RC_REGION_0_ADDR_TRANS_H,
>> +		   PCIE_CORE_AXI_CONF_BASE + 0x4);
>> +	pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8);
>> +	pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc);
>
> Why not move the above steps to rockchip_pcie_init_port function ?
> Please use macros instead of using values.

okay, will improve them.

Thanks for comment.

>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Arnd Bergmann]

On Friday, May 20, 2016 6:29:16 PM CEST Shawn Lin wrote:

> +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp,
> +				     int where, int size, u32 val)
> +{
> +	u32 tmp;
> +	int offset;
> +
> +	offset = (where & (~0x3));
> +	tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> +	if (size == 4) {
> +		writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where);
> +	} else if (size == 2) {
> +		if (where & 0x2)
> +			tmp = ((tmp & 0xffff) | (val << 16));
> +		else
> +			tmp = ((tmp & 0xffff0000) | val);
> +
> +		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> +	} else if (size == 1) {
> +		if ((where & 0x3) == 0)
> +			tmp = ((tmp & (~0xff)) | val);
> +		else if ((where & 0x3) == 1)
> +			tmp = ((tmp & (~0xff00)) | (val << 8));
> +		else if ((where & 0x3) == 2)
> +			tmp = ((tmp & (~0xff0000)) | (val << 16));
> +		else if ((where & 0x3) == 3)
> +			tmp = ((tmp & (~0xff000000)) | (val << 24));
> +
> +		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> +	} else {
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return PCIBIOS_SUCCESSFUL;
> +}

Why can't you access the individual sub-word registers here?

> +
> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
> +				       struct pci_bus *bus, u32 devfn,
> +				       int where, int size, u32 *val)
> +{
> +	u32 busdev;
> +
> +	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), where);
> +
> +	if (busdev & (size - 1)) {
> +		*val = 0;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	if (size == 4) {
> +		*val = readl(pp->reg_base + busdev);
> +	} else if (size == 2) {
> +		*val = readw(pp->reg_base + busdev);
> +	} else if (size == 1) {
> +		*val = readb(pp->reg_base + busdev);
> +	} else {
> +		*val = 0;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +

This looks like the normal ECAM operations, you could just call those.

> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if ((status & (1 << 9))) {
> +			dev_info(port->dev, "pll locked!\n");
> +			err = 0;
> +			break;
> +		}
> +	}

Maybe add an msleep(1) here to avoid busy-looping?


> +	for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) {
> +		err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
> +						AXI_WRAPPER_MEM_WRITE,
> +						20 - 1,
> +						port->mem_bus_addr +
> +							(reg_no << 20),
> +						0);
> +		if (err) {
> +			dev_err(dev, "Program RC outbound atu failed\n");
> +			return err;
> +		}
> +	}

What if there is more than one outbound memory window, e.g. prefetchable
and non-prefetchable?

Where do you set the I/O window?

> +	err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0);
> +	if (err) {
> +		dev_err(dev, "Program RC inbound atu failed\n");
> +		return err;
> +	}

And this doesn't seem to reflect the DMA ranges.

> +
> +	port->root_bus_nr = port->busn->start;
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr,
> +					    &rockchip_pcie_ops, port, &res,
> +					    port->msi);
> +	} else {
> +		bus = pci_scan_root_bus(&pdev->dev, 0,
> +					&rockchip_pcie_ops, port, &res);


PCI_MSI is selected unconditionally from Kconfig for this driver, so no
need for the compile-time check here.

> +
> +static int rockchip_pcie_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_pcie_port *port = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(port->hclk_pcie);
> +	clk_disable_unprepare(port->aclk_perf_pcie);
> +	clk_disable_unprepare(port->aclk_pcie);
> +	clk_disable_unprepare(port->clk_pciephy_ref);
> +
> +	return 0;
> +}

You don't seem to remove the child devices here. Have you tried unloading the module?

	Arnd

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Wenrui Li]

Hi,

On 2016/5/24 21:03, Arnd Bergmann wrote:
> On Friday, May 20, 2016 6:29:16 PM CEST Shawn Lin wrote:
>
>> +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp,
>> +				     int where, int size, u32 val)
>> +{
>> +	u32 tmp;
>> +	int offset;
>> +
>> +	offset = (where & (~0x3));
>> +	tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
>> +	if (size == 4) {
>> +		writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where);
>> +	} else if (size == 2) {
>> +		if (where & 0x2)
>> +			tmp = ((tmp & 0xffff) | (val << 16));
>> +		else
>> +			tmp = ((tmp & 0xffff0000) | val);
>> +
>> +		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
>> +	} else if (size == 1) {
>> +		if ((where & 0x3) == 0)
>> +			tmp = ((tmp & (~0xff)) | val);
>> +		else if ((where & 0x3) == 1)
>> +			tmp = ((tmp & (~0xff00)) | (val << 8));
>> +		else if ((where & 0x3) == 2)
>> +			tmp = ((tmp & (~0xff0000)) | (val << 16));
>> +		else if ((where & 0x3) == 3)
>> +			tmp = ((tmp & (~0xff000000)) | (val << 24));
>> +
>> +		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
>> +	} else {
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	}
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
>
> Why can't you access the individual sub-word registers here?

Our soc not support byte and word write in these region. so we have to 
use writel() to write these registers.

>
>> +
>> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
>> +				       struct pci_bus *bus, u32 devfn,
>> +				       int where, int size, u32 *val)
>> +{
>> +	u32 busdev;
>> +
>> +	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
>> +				PCI_FUNC(devfn), where);
>> +
>> +	if (busdev & (size - 1)) {
>> +		*val = 0;
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	}
>> +
>> +	if (size == 4) {
>> +		*val = readl(pp->reg_base + busdev);
>> +	} else if (size == 2) {
>> +		*val = readw(pp->reg_base + busdev);
>> +	} else if (size == 1) {
>> +		*val = readb(pp->reg_base + busdev);
>> +	} else {
>> +		*val = 0;
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>> +	}
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>
> This looks like the normal ECAM operations, you could just call those.

I read ECAM reference code, I found it not support ioremap config space 
for each bus individually on 64-bit systems. Our soc is 64-bit system, 
and bus0 config space base address is 0xfda00000, bus1 base address is 
0xf8100000. So I think it is not normal ECAM operations, I do not know 
if I have understood correctly?

>
>> +	while (time_before(jiffies, timeout)) {
>> +		regmap_read(port->grf, port->pcie_status, &status);
>> +		if ((status & (1 << 9))) {
>> +			dev_info(port->dev, "pll locked!\n");
>> +			err = 0;
>> +			break;
>> +		}
>> +	}
>
> Maybe add an msleep(1) here to avoid busy-looping?

Yeah, its a good suggestion.

>
>
>> +	for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) {
>> +		err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
>> +						AXI_WRAPPER_MEM_WRITE,
>> +						20 - 1,
>> +						port->mem_bus_addr +
>> +							(reg_no << 20),
>> +						0);
>> +		if (err) {
>> +			dev_err(dev, "Program RC outbound atu failed\n");
>> +			return err;
>> +		}
>> +	}
>
> What if there is more than one outbound memory window, e.g. prefetchable
> and non-prefetchable?
>
> Where do you set the I/O window?


Yeah, our soc support these outbound windows, we will support these 
windows next patch.

>
>> +	err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0);
>> +	if (err) {
>> +		dev_err(dev, "Program RC inbound atu failed\n");
>> +		return err;
>> +	}
>
> And this doesn't seem to reflect the DMA ranges.

Our soc support all the ram region use as DMA zone. SO I set all the 32 
bits bypass for the inbound ATS.

>
>> +
>> +	port->root_bus_nr = port->busn->start;
>> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> +		bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr,
>> +					    &rockchip_pcie_ops, port, &res,
>> +					    port->msi);
>> +	} else {
>> +		bus = pci_scan_root_bus(&pdev->dev, 0,
>> +					&rockchip_pcie_ops, port, &res);
>
>
> PCI_MSI is selected unconditionally from Kconfig for this driver, so no
> need for the compile-time check here.

Yeah, we will delete the compile-time check in next patch.

>
>> +
>> +static int rockchip_pcie_remove(struct platform_device *pdev)
>> +{
>> +	struct rockchip_pcie_port *port = platform_get_drvdata(pdev);
>> +
>> +	clk_disable_unprepare(port->hclk_pcie);
>> +	clk_disable_unprepare(port->aclk_perf_pcie);
>> +	clk_disable_unprepare(port->aclk_pcie);
>> +	clk_disable_unprepare(port->clk_pciephy_ref);
>> +
>> +	return 0;
>> +}
>
> You don't seem to remove the child devices here. Have you tried unloading the module?
>

I don't seem the remove function in other pcie host driver. I think this 
function could be deleted.

> 	Arnd
>
>
>
>
>

Best Regards
Wenrui Li

RE: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Bharat Kumar Gogada]

> >
> >> +
> >> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
> >> +				       struct pci_bus *bus, u32 devfn,
> >> +				       int where, int size, u32 *val)
> >> +{
> >> +	u32 busdev;
> >> +
> >> +	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
> >> +				PCI_FUNC(devfn), where);
> >> +
> >> +	if (busdev & (size - 1)) {
> >> +		*val = 0;
> >> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> >> +	}
> >> +
> >> +	if (size == 4) {
> >> +		*val = readl(pp->reg_base + busdev);
> >> +	} else if (size == 2) {
> >> +		*val = readw(pp->reg_base + busdev);
> >> +	} else if (size == 1) {
> >> +		*val = readb(pp->reg_base + busdev);
> >> +	} else {
> >> +		*val = 0;
> >> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> >> +	}
> >> +	return PCIBIOS_SUCCESSFUL;
> >> +}
> >> +
> >
> > This looks like the normal ECAM operations, you could just call those.
> 
> I read ECAM reference code, I found it not support ioremap config space
> for each bus individually on 64-bit systems. Our soc is 64-bit system,
> and bus0 config space base address is 0xfda00000, bus1 base address is
> 0xf8100000. So I think it is not normal ECAM operations, I do not know
> if I have understood correctly?
> 
Hi,

I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write
which does above functionality.

Bharat 

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Wenrui Li]

Hi,

On 2016/5/27 15:13, Bharat Kumar Gogada wrote:
>>>
>>>> +
>>>> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
>>>> +				       struct pci_bus *bus, u32 devfn,
>>>> +				       int where, int size, u32 *val)
>>>> +{
>>>> +	u32 busdev;
>>>> +
>>>> +	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
>>>> +				PCI_FUNC(devfn), where);
>>>> +
>>>> +	if (busdev & (size - 1)) {
>>>> +		*val = 0;
>>>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>>>> +	}
>>>> +
>>>> +	if (size == 4) {
>>>> +		*val = readl(pp->reg_base + busdev);
>>>> +	} else if (size == 2) {
>>>> +		*val = readw(pp->reg_base + busdev);
>>>> +	} else if (size == 1) {
>>>> +		*val = readb(pp->reg_base + busdev);
>>>> +	} else {
>>>> +		*val = 0;
>>>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>>>> +	}
>>>> +	return PCIBIOS_SUCCESSFUL;
>>>> +}
>>>> +
>>>
>>> This looks like the normal ECAM operations, you could just call those.
>>
>> I read ECAM reference code, I found it not support ioremap config space
>> for each bus individually on 64-bit systems. Our soc is 64-bit system,
>> and bus0 config space base address is 0xfda00000, bus1 base address is
>> 0xf8100000. So I think it is not normal ECAM operations, I do not know
>> if I have understood correctly?
>>
> Hi,
>
> I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write
> which does above functionality.

Yeah, I seem the pci_generic_config_write use writew/writeb for byte and 
word write. but our SOC not support byte and word write in RC config 
spcace. So I redefine the the pci_ops.write

>
> Bharat
>
>
>

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Arnd Bergmann]

On Friday, May 27, 2016 6:31:28 PM CEST Wenrui Li wrote:
> Hi,
> 
> On 2016/5/27 15:13, Bharat Kumar Gogada wrote:
> >>>
> >>>> +
> >>>> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
> >>>> +                                 struct pci_bus *bus, u32 devfn,
> >>>> +                                 int where, int size, u32 *val)
> >>>> +{
> >>>> +  u32 busdev;
> >>>> +
> >>>> +  busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
> >>>> +                          PCI_FUNC(devfn), where);
> >>>> +
> >>>> +  if (busdev & (size - 1)) {
> >>>> +          *val = 0;
> >>>> +          return PCIBIOS_BAD_REGISTER_NUMBER;
> >>>> +  }
> >>>> +
> >>>> +  if (size == 4) {
> >>>> +          *val = readl(pp->reg_base + busdev);
> >>>> +  } else if (size == 2) {
> >>>> +          *val = readw(pp->reg_base + busdev);
> >>>> +  } else if (size == 1) {
> >>>> +          *val = readb(pp->reg_base + busdev);
> >>>> +  } else {
> >>>> +          *val = 0;
> >>>> +          return PCIBIOS_BAD_REGISTER_NUMBER;
> >>>> +  }
> >>>> +  return PCIBIOS_SUCCESSFUL;
> >>>> +}
> >>>> +
> >>>
> >>> This looks like the normal ECAM operations, you could just call those.
> >>
> >> I read ECAM reference code, I found it not support ioremap config space
> >> for each bus individually on 64-bit systems. Our soc is 64-bit system,
> >> and bus0 config space base address is 0xfda00000, bus1 base address is
> >> 0xf8100000. So I think it is not normal ECAM operations, I do not know
> >> if I have understood correctly?
> >>
> > Hi,
> >
> > I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write
> > which does above functionality.
> 
> Yeah, I seem the pci_generic_config_write use writew/writeb for byte and 
> word write. but our SOC not support byte and word write in RC config 
> spcace. So I redefine the the pci_ops.write

Rihgt, that bug should be documented somewhere. You can also use
the pci_generic_config_read32/pci_generic_config_write32 functions
for this.

I wonder if we should add a more general way of treating type 1
config space accesses differently, as a lot of host bridges have
similar requirements, accessing the registers in a different place,
different layout, or other constraints.

The patch below would make that very easy to do. Suggestions for
better naming welcome.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d11cdbb8fba3..2f7dcaa63e1d 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -34,9 +34,12 @@ int pci_bus_read_config_##size \
 	u32 data = 0;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	raw_spin_lock_irqsave(&pci_lock, flags);			\
-	res = bus->ops->read(bus, devfn, pos, len, &data);		\
+	if (bus->number == 0 && bus->ops->read0)			\
+		res = bus->ops->read0(bus, devfn, pos, len, &data);	\
+	else								\
+		res = bus->ops->read(bus, devfn, pos, len, &data);	\
 	*value = (type)data;						\
-	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
+	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
@@ -48,8 +51,11 @@ int pci_bus_write_config_##size \
 	unsigned long flags;						\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	raw_spin_lock_irqsave(&pci_lock, flags);			\
-	res = bus->ops->write(bus, devfn, pos, len, value);		\
-	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
+	if (bus->number == 0 && bus->ops->write0)			\
+		res = bus->ops->write0(bus, devfn, pos, len, value);	\
+	else								\
+		res = bus->ops->write(bus, devfn, pos, len, value);	\
+	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
@@ -72,7 +78,11 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
 {
 	void __iomem *addr;
 
-	addr = bus->ops->map_bus(bus, devfn, where);
+	if (bus->number == 0 && bus->ops->map_bus0)
+		addr = bus->ops->map_bus0(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where);
+
 	if (!addr) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -94,7 +104,10 @@ int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
 {
 	void __iomem *addr;
 
-	addr = bus->ops->map_bus(bus, devfn, where);
+	if (bus->number == 0 && bus->ops->map_bus0)
+		addr = bus->ops->map_bus0(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where);
 	if (!addr)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
@@ -114,7 +127,10 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
 {
 	void __iomem *addr;
 
-	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (bus->number == 0 && bus->ops->map_bus0)
+		addr = bus->ops->map_bus0(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
 	if (!addr) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -135,7 +151,10 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
 	void __iomem *addr;
 	u32 mask, tmp;
 
-	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (bus->number == 0 && bus->ops->map_bus0)
+		addr = bus->ops->map_bus0(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
 	if (!addr)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index df41c4645911..1caae3bd7115 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -580,6 +580,9 @@ struct pci_ops {
 	void __iomem *(*map_bus)(struct pci_bus *bus, unsigned int devfn, int where);
 	int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
 	int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
+	void __iomem *(*map_bus0)(struct pci_bus *bus, unsigned int devfn, int where);
+	int (*read0)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
+	int (*write0)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
 };
 
 /*

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Shawn Lin]

Hi Arnd,

On 2016/6/1 16:24, Arnd Bergmann wrote:
> On Friday, May 27, 2016 6:31:28 PM CEST Wenrui Li wrote:
>> Hi,
>>
>> On 2016/5/27 15:13, Bharat Kumar Gogada wrote:
>>>>>
>>>>>> +
>>>>>> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
>>>>>> +                                 struct pci_bus *bus, u32 devfn,
>>>>>> +                                 int where, int size, u32 *val)
>>>>>> +{
>>>>>> +  u32 busdev;
>>>>>> +
>>>>>> +  busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
>>>>>> +                          PCI_FUNC(devfn), where);
>>>>>> +
>>>>>> +  if (busdev & (size - 1)) {
>>>>>> +          *val = 0;
>>>>>> +          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>> +  }
>>>>>> +
>>>>>> +  if (size == 4) {
>>>>>> +          *val = readl(pp->reg_base + busdev);
>>>>>> +  } else if (size == 2) {
>>>>>> +          *val = readw(pp->reg_base + busdev);
>>>>>> +  } else if (size == 1) {
>>>>>> +          *val = readb(pp->reg_base + busdev);
>>>>>> +  } else {
>>>>>> +          *val = 0;
>>>>>> +          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>> +  }
>>>>>> +  return PCIBIOS_SUCCESSFUL;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> This looks like the normal ECAM operations, you could just call those.
>>>>
>>>> I read ECAM reference code, I found it not support ioremap config space
>>>> for each bus individually on 64-bit systems. Our soc is 64-bit system,
>>>> and bus0 config space base address is 0xfda00000, bus1 base address is
>>>> 0xf8100000. So I think it is not normal ECAM operations, I do not know
>>>> if I have understood correctly?
>>>>
>>> Hi,
>>>
>>> I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write
>>> which does above functionality.
>>
>> Yeah, I seem the pci_generic_config_write use writew/writeb for byte and
>> word write. but our SOC not support byte and word write in RC config
>> spcace. So I redefine the the pci_ops.write
>
> Rihgt, that bug should be documented somewhere. You can also use
> the pci_generic_config_read32/pci_generic_config_write32 functions
> for this.
>
> I wonder if we should add a more general way of treating type 1
> config space accesses differently, as a lot of host bridges have
> similar requirements, accessing the registers in a different place,
> different layout, or other constraints.

yes, that is what we need and we believe other host bridges have
this requirements.  With your patch applied, we only need to
implement our map0 callback here. Thanks for improving it.

BTW, would you mind that we pack your patch into my patchset for
the next version if possible, or maybe you wanna upstream it
by yourself? :)


Thanks.

>
> The patch below would make that very easy to do. Suggestions for
> better naming welcome.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d11cdbb8fba3..2f7dcaa63e1d 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -34,9 +34,12 @@ int pci_bus_read_config_##size \
>  	u32 data = 0;							\
>  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
>  	raw_spin_lock_irqsave(&pci_lock, flags);			\
> -	res = bus->ops->read(bus, devfn, pos, len, &data);		\
> +	if (bus->number == 0 && bus->ops->read0)			\
> +		res = bus->ops->read0(bus, devfn, pos, len, &data);	\
> +	else								\
> +		res = bus->ops->read(bus, devfn, pos, len, &data);	\
>  	*value = (type)data;						\
> -	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
> +	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
>  	return res;							\
>  }
>
> @@ -48,8 +51,11 @@ int pci_bus_write_config_##size \
>  	unsigned long flags;						\
>  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
>  	raw_spin_lock_irqsave(&pci_lock, flags);			\
> -	res = bus->ops->write(bus, devfn, pos, len, value);		\
> -	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
> +	if (bus->number == 0 && bus->ops->write0)			\
> +		res = bus->ops->write0(bus, devfn, pos, len, value);	\
> +	else								\
> +		res = bus->ops->write(bus, devfn, pos, len, value);	\
> +	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
>  	return res;							\
>  }
>
> @@ -72,7 +78,11 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>  {
>  	void __iomem *addr;
>
> -	addr = bus->ops->map_bus(bus, devfn, where);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where);
> +
>  	if (!addr) {
>  		*val = ~0;
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -94,7 +104,10 @@ int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
>  {
>  	void __iomem *addr;
>
> -	addr = bus->ops->map_bus(bus, devfn, where);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where);
>  	if (!addr)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>
> @@ -114,7 +127,10 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
>  {
>  	void __iomem *addr;
>
> -	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>  	if (!addr) {
>  		*val = ~0;
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -135,7 +151,10 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
>  	void __iomem *addr;
>  	u32 mask, tmp;
>
> -	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> +	if (bus->number == 0 && bus->ops->map_bus0)
> +		addr = bus->ops->map_bus0(bus, devfn, where);
> +	else
> +		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>  	if (!addr)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index df41c4645911..1caae3bd7115 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -580,6 +580,9 @@ struct pci_ops {
>  	void __iomem *(*map_bus)(struct pci_bus *bus, unsigned int devfn, int where);
>  	int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
>  	int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
> +	void __iomem *(*map_bus0)(struct pci_bus *bus, unsigned int devfn, int where);
> +	int (*read0)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
> +	int (*write0)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
>  };
>
>  /*
>
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Arnd Bergmann]

On Wednesday, June 1, 2016 5:57:25 PM CEST Shawn Lin wrote:
> yes, that is what we need and we believe other host bridges have
> this requirements.  With your patch applied, we only need to
> implement our map0 callback here. Thanks for improving it.
> 
> BTW, would you mind that we pack your patch into my patchset for
> the next version if possible, or maybe you wanna upstream it
> by yourself? 

I think the patch still needs a little improvement and discussion.
I've just done a second version, and tried out converting two
of the existing drivers to it, with good results.

We might want to add another set of callbacks for the first indirect
level, as that could help designware, mvebu and rcar at the minimum.
We then end up with 'root' config, 'port type 1' config and
'device type 0' config, with the first two falling back on the third
when unavailable.

I'm sending out what I have now.

	Arnd

[2/2] pci: Add PCIe driver for Rockchip Soc

[Rajat Jain]

> RK3399 has a PCIe controller which can be used as Root Complex.
> This driver supports a PCIe controller as Root Complex mode.

1 general feedback I have is that there are a lot of magic constants.
I'd appreciate if they could be converted into macros for easy readability.

> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/pci/host/Kconfig         |   12 +
>  drivers/pci/host/Makefile        |    1 +
>  drivers/pci/host/pcie-rockchip.c | 1181 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1194 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-rockchip.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 8e4f038..76447a8 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -245,4 +245,16 @@ config PCIE_ARMADA_8K
>  	  Designware hardware and therefore the driver re-uses the
>  	  Designware core functions to implement the driver.
>  
> +config PCIE_ROCKCHIP
> +	bool "Rockchip PCIe controller"
> +	depends on ARM64 && ARCH_ROCKCHIP
> +	depends on OF
> +	select MFD_SYSCON
> +	select PCI_MSI
> +	select PCI_MSI_IRQ_DOMAIN
> +	help
> +	  Say Y here if you want internal PCI support on Rockchip SoC.
> +	  There are 1 internal PCIe port available to support GEN2 with
> +	  4 slots.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index a6f85e3..fb3871e 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> new file mode 100644
> index 0000000..4063fd3
> --- /dev/null
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -0,0 +1,1181 @@
> +/*
> + * Rockchip AXI PCIe host controller driver
> + *
> + * Copyright (c) 2016 Rockchip, Inc.
> + *
> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
> + *         Wenrui Li <wenrui.li@rock-chips.com>
> + * Bits taken from Synopsys Designware Host controller driver and
> + * ARM PCI Host generic driver.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/regmap.h>
> +
> +#define REF_CLK_100MHZ			(100 * 1000 * 1000)
> +#define PCIE_CLIENT_BASE		0x0
> +#define PCIE_RC_CONFIG_BASE		0xa00000
> +#define PCIE_CORE_CTRL_MGMT_BASE	0x900000
> +#define PCIE_CORE_AXI_CONF_BASE		0xc00000
> +#define PCIE_CORE_AXI_INBOUND_BASE	0xc00800
> +
> +#define PCIE_CLIENT_BASIC_STATUS0	0x44
> +#define PCIE_CLIENT_BASIC_STATUS1	0x48
> +#define PCIE_CLIENT_INT_MASK		0x4c
> +#define PCIE_CLIENT_INT_STATUS		0x50
> +#define PCIE_CORE_INT_MASK		0x900210
> +#define PCIE_CORE_INT_STATUS		0x90020c
> +
> +/** Size of one AXI Region (not Region 0) */
> +#define	AXI_REGION_SIZE			(0x1 << 20)
> +/** Overall size of AXI area */
> +#define	AXI_OVERALL_SIZE		(64 * (0x1 << 20))

Not used?

> +/** Size of Region 0, equal to sum of sizes of other regions */
> +#define	AXI_REGION_0_SIZE		(32 * (0x1 << 20))
> +#define OB_REG_SIZE_SHIFT		5
> +#define IB_ROOT_PORT_REG_SIZE_SHIFT	3
> +
> +#define AXI_WRAPPER_IO_WRITE		0x6
> +#define AXI_WRAPPER_MEM_WRITE		0x2
> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM	3
> +#define	MIN_AXI_ADDR_BITS_PASSED	8
> +
> +#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK	GENMASK(8, 5)
> +#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT	5
> +#define CLIENT_INTERRUPTS \
> +		(LOC_INT | INTA | INTB | INTC | INTD |\
> +		 CORR_ERR | NFATAL_ERR | FATAL_ERR | DPA_INT | \
> +		 HOT_RESET | MSG_DONE | LEGACY_DONE)
> +#define CORE_INTERRUPTS	\
> +		(PRFPE | CRFPE | RRPE | CRFO | RT | RTR | \
> +		 PE | MTR | UCR | FCE | CT | UTC | MMVC)
> +#define PWR_STCG			BIT(0)
> +#define HOT_PLUG			BIT(1)
> +#define PHY_INT				BIT(2)
> +#define UDMA_INT			BIT(3)
> +#define LOC_INT				BIT(4)
> +#define INTA				BIT(5)
> +#define INTB				BIT(6)
> +#define INTC				BIT(7)
> +#define INTD				BIT(8)
> +#define CORR_ERR			BIT(9)
> +#define NFATAL_ERR			BIT(10)
> +#define FATAL_ERR			BIT(11)
> +#define DPA_INT				BIT(12)
> +#define HOT_RESET			BIT(13)
> +#define MSG_DONE			BIT(14)
> +#define LEGACY_DONE			BIT(15)
> +#define PRFPE				BIT(0)
> +#define CRFPE				BIT(1)
> +#define RRPE				BIT(2)
> +#define PRFO				BIT(3)
> +#define CRFO				BIT(4)
> +#define RT				BIT(5)
> +#define RTR				BIT(6)
> +#define PE				BIT(7)
> +#define MTR				BIT(8)
> +#define UCR				BIT(9)
> +#define FCE				BIT(10)
> +#define CT				BIT(11)
> +#define UTC				BIT(18)
> +#define MMVC				BIT(19)
> +
> +#define PCIE_ECAM_BUS(x)		(((x) & 0xFF) << 20)
> +#define PCIE_ECAM_DEV(x)		(((x) & 0x1F) << 15)
> +#define PCIE_ECAM_FUNC(x)		(((x) & 0x7) << 12)
> +#define PCIE_ECAM_REG(x)		(((x) & 0xFFF) << 0)
> +#define PCIE_ECAM_ADDR(bus, dev, func, reg) \
> +	  (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \
> +	   PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg))
> +
> +#define RC_REGION_0_ADDR_TRANS_H	0x00000000
> +#define RC_REGION_0_ADDR_TRANS_L	0x00000000
> +#define RC_REGION_0_PASS_BITS		(25 - 1)
> +#define RC_REGION_1_ADDR_TRANS_H	0x00000000
> +#define RC_REGION_1_ADDR_TRANS_L	0x00400000
> +#define RC_REGION_1_PASS_BITS		(20 - 1)
> +#define MAX_AXI_WRAPPER_REGION_NUM	33
> +#define PCIE_CLIENT_CONF_ENABLE		BIT(0)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)	((x / 2) << 4)
> +#define PCIE_CLIENT_MODE_RC		BIT(6)
> +#define PCIE_CLIENT_GEN_SEL_2		BIT(7)
> +#define PCIE_CLIENT_GEN_SEL_1		0x0
> +
> +struct rockchip_pcie_port {
> +	void __iomem *reg_base;
> +	void __iomem *apb_base;
> +	struct regmap *grf;
> +	unsigned int pcie_conf;
> +	unsigned int pcie_status;
> +	unsigned int pcie_laneoff;
> +	struct reset_control *phy_rst;
> +	struct reset_control *core_rst;
> +	struct reset_control *mgmt_rst;
> +	struct reset_control *mgmt_sticky_rst;
> +	struct reset_control *pipe_rst;
> +	struct clk *aclk_pcie;
> +	struct clk *aclk_perf_pcie;
> +	struct clk *hclk_pcie;
> +	struct clk *clk_pciephy_ref;
> +	struct gpio_desc *ep_gpio;
> +	u32 lanes;
> +	resource_size_t		io_base;
> +	struct resource		*cfg;
> +	struct resource		*io;
> +	struct resource		*mem;

Nit: It seems to me that these resource fields (cfg/io/mem) aren't really
used, except for assignment and as temporary variables, so possibly
can be get rid of, (unless you intend to use them in later patches, if any)

> +	struct resource		*busn;
> +	phys_addr_t		io_bus_addr;
> +	u32			io_size;
> +	phys_addr_t		mem_bus_addr;
> +	u32			mem_size;
> +	u8	root_bus_nr;
> +	int irq;
> +	struct msi_controller *msi;
> +
> +	struct device *dev;
> +	struct irq_domain *irq_domain;
> +};
> +
> +static inline u32 pcie_read(struct rockchip_pcie_port *port, u32 reg)
> +{
> +	return readl(port->apb_base + reg);
> +}
> +
> +static inline void pcie_write(struct rockchip_pcie_port *port,
> +			      u32 val, u32 reg)
> +{
> +	writel(val, port->apb_base + reg);
> +}
> +
> +static inline void pcie_pb_wr_cfg(struct rockchip_pcie_port *port,
> +				  u32 addr, u32 data)
> +{
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x3ff << 17) | (data << 7) | (addr << 1));
> +	udelay(1);
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x1 << 16) | (0x1 << 0));
> +	udelay(1);
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x1 << 16) | (0x0 << 0));
> +}
> +
> +static inline u32 pcie_pb_rd_cfg(struct rockchip_pcie_port *port,
> +				 u32 addr)
> +{
> +	u32 val;
> +
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x3ff << 17) | (addr << 1));

Taking cue from pcie_pb_wr_cfg(), I'm wondering if there is any
delay required here?

> +	regmap_read(port->grf, port->pcie_status, &val);
> +	return val;
> +}
> +
> +static int rockchip_pcie_valid_config(struct rockchip_pcie_port *pp,
> +				      struct pci_bus *bus, int dev)
> +{
> +	/* access only one slot on each root port */
> +	if (bus->number == pp->root_bus_nr && dev > 0)
> +		return 0;
> +
> +	/*
> +	 * do not read more than one device on the bus directly attached
> +	 * to RC's (Virtual Bridge's) DS side.
> +	 */
> +	if (bus->primary == pp->root_bus_nr && dev > 0)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int rockchip_pcie_rd_own_conf(struct rockchip_pcie_port *pp,
> +				     int where, int size,
> +				     u32 *val)
> +{
> +	void __iomem *addr = pp->apb_base + PCIE_RC_CONFIG_BASE + where;
> +
> +	if ((uintptr_t)addr & (size - 1)) {

Use IS_ALIGNED()?

> +		*val = 0;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	if (size == 4) {
> +		*val = readl(addr);
> +	} else if (size == 2) {
> +		*val = readw(addr);
> +	} else if (size == 1) {
> +		*val = readb(addr);
> +	} else {
> +		*val = 0;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp,
> +				     int where, int size, u32 val)
> +{
> +	u32 tmp;
> +	int offset;
> +
> +	offset = (where & (~0x3));
> +	tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> +	if (size == 4) {
> +		writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where);
> +	} else if (size == 2) {
> +		if (where & 0x2)
> +			tmp = ((tmp & 0xffff) | (val << 16));
> +		else
> +			tmp = ((tmp & 0xffff0000) | val);
> +
> +		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
> +	} else if (size == 1) {
> +		if ((where & 0x3) == 0)
> +			tmp = ((tmp & (~0xff)) | val);
> +		else if ((where & 0x3) == 1)
> +			tmp = ((tmp & (~0xff00)) | (val << 8));
> +		else if ((where & 0x3) == 2)
> +			tmp = ((tmp & (~0xff0000)) | (val << 16));
> +		else if ((where & 0x3) == 3)
> +			tmp = ((tmp & (~0xff000000)) | (val << 24));
> +
> +		writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);

Why not register sized writes instead of writels?

> +	} else {
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp,
> +				       struct pci_bus *bus, u32 devfn,
> +				       int where, int size, u32 *val)
> +{
> +	u32 busdev;
> +
> +	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), where);
> +
> +	if (busdev & (size - 1)) {

Use IS_ALIGNED() for here and the other occurences in
above and below functions.

> +		*val = 0;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	if (size == 4) {
> +		*val = readl(pp->reg_base + busdev);
> +	} else if (size == 2) {
> +		*val = readw(pp->reg_base + busdev);
> +	} else if (size == 1) {
> +		*val = readb(pp->reg_base + busdev);
> +	} else {
> +		*val = 0;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int rockchip_pcie_wr_other_conf(struct rockchip_pcie_port *pp,
> +				       struct pci_bus *bus, u32 devfn,
> +				       int where, int size, u32 val)
> +{
> +	u32 busdev;
> +
> +	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), where);
> +	if (busdev & (size - 1))
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	if (size == 4)
> +		writel(val, pp->reg_base + busdev);
> +	else if (size == 2)
> +		writew(val, pp->reg_base + busdev);
> +	else if (size == 1)
> +		writeb(val, pp->reg_base + busdev);
> +	else
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
> +				 int size, u32 *val)
> +{
> +	struct rockchip_pcie_port *pp = bus->sysdata;
> +	int ret;
> +
> +	if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
> +		*val = 0xffffffff;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	if (bus->number != pp->root_bus_nr)
> +		ret = rockchip_pcie_rd_other_conf(pp, bus, devfn,
> +						  where, size, val);
> +	else
> +		ret = rockchip_pcie_rd_own_conf(pp, where, size, val);
> +
> +	return ret;
> +}
> +
> +static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> +				 int where, int size, u32 val)
> +{
> +	struct rockchip_pcie_port *pp = bus->sysdata;
> +	int ret;
> +
> +	if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	if (bus->number != pp->root_bus_nr)
> +		ret = rockchip_pcie_wr_other_conf(pp, bus, devfn,
> +						  where, size, val);
> +	else
> +		ret = rockchip_pcie_wr_own_conf(pp, where, size, val);
> +
> +	return ret;
> +}
> +
> +static struct pci_ops rockchip_pcie_ops = {
> +	.read = rockchip_pcie_rd_conf,
> +	.write = rockchip_pcie_wr_conf,
> +};
> +
> +/**
> + * rockchip_pcie_init_port - Initialize hardware
> + * @port: PCIe port information
> + */
> +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
> +{
> +	int err;
> +	u32 status;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(1000);

The way you have currently implemented, would mean that you have set
the cumulative timeout of 1000 msec for all the operations. So if PLL
takes 990 msec to lock, there is only 10 msec for other steps. It may
be better to have individual timeouts I think since you'd know from
HW docs how much should the time out should be?

> +
> +	gpiod_set_value(port->ep_gpio, 0);
> +
> +	/* Make sure PCIe relate block is in reset state */
> +	err = reset_control_assert(port->phy_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert phy_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_assert(port->core_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert core_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_assert(port->mgmt_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert mgmt_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_assert(port->mgmt_sticky_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_assert(port->pipe_rst);
> +	if (err) {
> +		dev_err(port->dev, "assert pipe_rst err %d\n", err);
> +		return err;
> +	}
> +
> +	pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 |
> +			  (0x1 << 19) | (0x1 << 3) |
> +			  PCIE_CLIENT_MODE_RC |
> +			  PCIE_CLIENT_CONF_LANE_NUM(port->lanes) |
> +			  PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE);
> +
> +	err = reset_control_deassert(port->phy_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert phy_rst err %d\n", err);
> +		return err;
> +	}
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x3f << 17) | (0x10 << 1));
> +	err = -EINVAL;
> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if ((status & (1 << 9))) {
> +			dev_info(port->dev, "pll locked!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err) {
> +		dev_err(port->dev, "pll lock timeout!\n");
> +		return err;
> +	}
> +	pcie_pb_wr_cfg(port, 0x10, 0x8);
> +	pcie_pb_wr_cfg(port, 0x12, 0x8);
> +
> +	err = -ETIMEDOUT;
> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if (!(status & (1 << 10))) {
> +			dev_info(port->dev, "pll output enable done!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +
> +	if (err) {
> +		dev_err(port->dev, "pll output enable timeout!\n");
> +		return err;
> +	}
> +
> +	regmap_write(port->grf, port->pcie_conf,
> +		     (0x3f << 17) | (0x10 << 1));
> +	err = -EINVAL;
> +	while (time_before(jiffies, timeout)) {
> +		regmap_read(port->grf, port->pcie_status, &status);
> +		if ((status & (1 << 9))) {
> +			dev_info(port->dev, "pll relocked!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err) {
> +		dev_err(port->dev, "pll relock timeout!\n");
> +		return err;
> +	}

There's a bunch of these "regmap_write, loop waiting for a bit, error check"(s)
May be have an inline function for it?

> +
> +	err = reset_control_deassert(port->core_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert core_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_deassert(port->mgmt_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert mgmt_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_deassert(port->mgmt_sticky_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err);
> +		return err;
> +	}
> +	err = reset_control_deassert(port->pipe_rst);
> +	if (err) {
> +		dev_err(port->dev, "deassert pipe_rst err %d\n", err);
> +		return err;
> +	}
> +
> +	pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE);
> +
> +	gpiod_set_value(port->ep_gpio, 1);
> +	err = -ETIMEDOUT;
> +	while (time_before(jiffies, timeout)) {
> +		status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1);
> +		if (((status >> 20) & 0x3) == 0x3) {
> +			dev_info(port->dev, "pcie link training gen1 pass!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err) {
> +		dev_err(port->dev, "pcie link training gen1 timeout!\n");
> +		return err;
> +	}
> +
> +	status = pcie_read(port, 0x9000d0);
> +	status |= 0x20;
> +	pcie_write(port, status, 0x9000d0);
> +	err = -ETIMEDOUT;
> +	while (time_before(jiffies, timeout)) {
> +		status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
> +		if (((status >> 3) & 0x3) == 0x1) {
> +			dev_info(port->dev, "pcie link training gen2 pass!\n");
> +			err = 0;
> +			break;
> +		}
> +	}
> +	if (err)
> +		dev_dbg(port->dev, "pcie link training gen2 timeout, force to gen1!\n");
> +
> +	if (((status >> 3) & 0x3) == 0x0)
> +		dev_info(port->dev, "pcie link 2.5!\n");
> +	if (((status >> 3) & 0x3) == 0x1)
> +		dev_info(port->dev, "pcie link 5.0!\n");
> +
> +	status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
> +	status =  0x1 << ((status >> 1) & 0x3);
> +	dev_info(port->dev, "current link width is x%d\n", status);
> +
> +	status = pcie_pb_rd_cfg(port, 0x30);
> +	if (!((status >> 11) & 0x1))
> +		dev_dbg(port->dev, "lane A is used\n");
> +	else
> +		regmap_write(port->grf, port->pcie_laneoff,
> +			     (0x1 << 19) | (0x1 << 3));
> +
> +	status = pcie_pb_rd_cfg(port, 0x31);
> +	if (!((status >> 11) & 0x1))
> +		dev_dbg(port->dev, "lane B is used\n");
> +	else
> +		regmap_write(port->grf, port->pcie_laneoff,
> +			     (0x2 << 19) | (0x2 << 3));
> +
> +	status = pcie_pb_rd_cfg(port, 0x32);
> +	if (!((status >> 11) & 0x1))
> +		dev_dbg(port->dev, "lane C is used\n");
> +	else
> +		regmap_write(port->grf, port->pcie_laneoff,
> +			     (0x4 << 19) | (0x4 << 3));
> +
> +	status = pcie_pb_rd_cfg(port, 0x33);
> +	if (!((status >> 11) & 0x1))
> +		dev_dbg(port->dev, "lane D is used\n");
> +	else
> +		regmap_write(port->grf, port->pcie_laneoff,
> +			     (0x8 << 19) | (0x8 << 3));

Can we have a loop looping over all the lanes instead of this?
Also, you seem to be checking and initializing all lanes instead
of referring to port->lanes?

> +	return 0;
> +}
> +
> +/**
> + * rockchip_pcie_parse_dt - Parse Device tree
> + * @port: PCIe port information
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *node = dev->of_node;
> +	struct resource regs;
> +	unsigned int pcie_conf;
> +	unsigned int pcie_status;
> +	unsigned int pcie_laneoff;
> +	int err;
> +
> +	err = of_address_to_resource(node, 0, &regs);
> +	if (err) {
> +		dev_err(dev, "missing \"reg\" property\n");

This error message is exactly the same as the one below.
May be say reg0 or reg1?

> +		return err;
> +	}
> +
> +	port->reg_base = devm_ioremap_resource(dev, &regs);
> +	if (IS_ERR(port->reg_base))
> +		return PTR_ERR(port->reg_base);
> +
> +	err = of_address_to_resource(node, 1, &regs);
> +	if (err) {
> +		dev_err(dev, "missing \"reg\" property\n");
> +		return err;
> +	}
> +
> +	port->apb_base = devm_ioremap_resource(dev, &regs);
> +	if (IS_ERR(port->apb_base))
> +		return PTR_ERR(port->apb_base);
> +
> +	port->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> +						    "rockchip,grf");
> +	if (IS_ERR(port->grf)) {
> +		dev_err(dev, "Missing rockchip,grf property\n");
> +		return PTR_ERR(port->grf);
> +	}
> +
> +	if (of_property_read_u32(node, "pcie-conf", &pcie_conf)) {
> +		dev_err(dev, "missing pcie-conf property in node %s\n",
> +			node->name);
> +		return -EINVAL;
> +	}
> +
> +	port->pcie_conf = pcie_conf;
> +
> +	if (of_property_read_u32(node, "pcie-status", &pcie_status)) {
> +		dev_err(dev, "missing pcie-status property in node %s\n",
> +			node->name);
> +		return -EINVAL;
> +	}
> +
> +	port->pcie_status = pcie_status;
> +
> +	if (of_property_read_u32(node, "pcie-laneoff", &pcie_laneoff)) {
> +		dev_err(dev, "missing pcie-laneoff property in node %s\n",
> +			node->name);
> +		return -EINVAL;
> +	}
> +
> +	port->pcie_laneoff = pcie_laneoff;
> +
> +	port->lanes = 1;
> +	err = of_property_read_u32(node, "num-lanes", &port->lanes);
> +	if (!err && ((port->lanes == 0) ||
> +		     (port->lanes == 3) ||
> +		     (port->lanes > 4))) {
> +		dev_info(dev, "invalid num-lanes, default use one lane\n");
> +		port->lanes = 1;
> +	}
> +
> +	port->phy_rst = devm_reset_control_get(dev, "phy-rst");
> +	if (IS_ERR(port->phy_rst)) {
> +		if (PTR_ERR(port->phy_rst) != -EPROBE_DEFER)
> +			dev_err(dev, "missing phy-rst property in node %s\n",
> +				node->name);
> +		err = PTR_ERR(port->phy_rst);
> +		goto err_aclk_pcie;
> +	}
> +
> +	port->core_rst = devm_reset_control_get(dev, "core-rst");
> +	if (IS_ERR(port->core_rst)) {
> +		if (PTR_ERR(port->core_rst) != -EPROBE_DEFER)
> +			dev_err(dev, "missing core-rst property in node %s\n",
> +				node->name);
> +		err = PTR_ERR(port->core_rst);
> +		goto err_aclk_pcie;
> +	}
> +
> +	port->mgmt_rst = devm_reset_control_get(dev, "mgmt-rst");
> +	if (IS_ERR(port->mgmt_rst)) {
> +		if (PTR_ERR(port->mgmt_rst) != -EPROBE_DEFER)
> +			dev_err(dev, "missing mgmt-rst property in node %s\n",
> +				node->name);
> +		err = PTR_ERR(port->mgmt_rst);
> +		goto err_aclk_pcie;
> +	}
> +
> +	port->mgmt_sticky_rst = devm_reset_control_get(dev, "mgmt-sticky-rst");
> +	if (IS_ERR(port->mgmt_sticky_rst)) {
> +		if (PTR_ERR(port->mgmt_sticky_rst) != -EPROBE_DEFER)
> +			dev_err(dev, "missing mgmt-sticky-rst property in node %s\n",
> +				node->name);
> +		err = PTR_ERR(port->mgmt_sticky_rst);
> +		goto err_aclk_pcie;
> +	}
> +
> +	port->pipe_rst = devm_reset_control_get(dev, "pipe-rst");
> +	if (IS_ERR(port->pipe_rst)) {
> +		if (PTR_ERR(port->pipe_rst) != -EPROBE_DEFER)
> +			dev_err(dev, "missing pipe-rst property in node %s\n",
> +				node->name);
> +		err = PTR_ERR(port->pipe_rst);
> +		goto err_aclk_pcie;
> +	}
> +
> +	port->ep_gpio = gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> +	if (IS_ERR(port->ep_gpio)) {
> +		dev_err(dev, "missing ep-gpios property in node %s\n",
> +			node->name);
> +		return PTR_ERR(port->ep_gpio);
> +	}
> +
> +	port->aclk_pcie = devm_clk_get(dev, "aclk_pcie");
> +	if (IS_ERR(port->aclk_pcie)) {
> +		dev_err(dev, "aclk_pcie clock not found.\n");
> +		return PTR_ERR(port->aclk_pcie);
> +	}
> +
> +	port->aclk_perf_pcie = devm_clk_get(dev, "aclk_perf_pcie");
> +	if (IS_ERR(port->aclk_perf_pcie)) {
> +		dev_err(dev, "aclk_perf_pcie clock not found.\n");
> +		return PTR_ERR(port->aclk_perf_pcie);
> +	}
> +
> +	port->hclk_pcie = devm_clk_get(dev, "hclk_pcie");
> +	if (IS_ERR(port->hclk_pcie)) {
> +		dev_err(dev, "hclk_pcie clock not found.\n");
> +		return PTR_ERR(port->hclk_pcie);
> +	}
> +
> +	port->clk_pciephy_ref = devm_clk_get(dev, "clk_pciephy_ref");
> +	if (IS_ERR(port->clk_pciephy_ref)) {
> +		dev_err(dev, "clk_pciephy_ref clock not found.\n");
> +		return PTR_ERR(port->clk_pciephy_ref);
> +	}
> +
> +	err = clk_prepare_enable(port->aclk_pcie);
> +	if (err) {
> +		dev_err(dev, "Unable to enable aclk_pcie clock.\n");
> +		goto err_aclk_pcie;
> +	}
> +
> +	err = clk_prepare_enable(port->aclk_perf_pcie);
> +	if (err) {
> +		dev_err(dev, "Unable to enable aclk_perf_pcie clock.\n");
> +		goto err_aclk_perf_pcie;
> +	}
> +
> +	err = clk_prepare_enable(port->hclk_pcie);
> +	if (err) {
> +		dev_err(dev, "Unable to enable hclk_pcie clock.\n");
> +		goto err_hclk_pcie;
> +	}
> +
> +	err = clk_prepare_enable(port->clk_pciephy_ref);
> +	if (err) {
> +		dev_err(dev, "Unable to enable hclk_pcie clock.\n");
> +		goto err_pciephy_ref;
> +	}

IMHO, the code in rockchip_pcie_parse_dt() is doing much more than
what the function name says i.e. "parsing the dt". May be move the
clk_prepare_enable() calls out (and possibly others).

> +
> +	return 0;
> +
> +err_pciephy_ref:
> +	clk_disable_unprepare(port->hclk_pcie);
> +err_hclk_pcie:
> +	clk_disable_unprepare(port->aclk_perf_pcie);
> +err_aclk_perf_pcie:
> +	clk_disable_unprepare(port->aclk_pcie);
> +err_aclk_pcie:
> +	return err;
> +}
> +
> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp)
> +{

This function seems to be (only) parsing the dt? If so we can move its
code to parse_dt function?

> +	struct device_node *msi_node;
> +
> +	msi_node = of_parse_phandle(pp->dev->of_node,
> +				    "msi-parent", 0);
> +	if (!msi_node)
> +		return;
> +
> +	pp->msi = of_pci_find_msi_chip_by_node(msi_node);
> +	of_node_put(msi_node);
> +
> +	if (pp->msi)
> +		pp->msi->dev = pp->dev;
> +}
> +
> +static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *pp)
> +{
> +	pcie_write(pp, (CLIENT_INTERRUPTS << 16) &
> +		   (~CLIENT_INTERRUPTS), PCIE_CLIENT_INT_MASK);
> +	pcie_write(pp, CORE_INTERRUPTS, PCIE_CORE_INT_MASK);
> +}
> +
> +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
> +				  irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = rockchip_pcie_intx_map,
> +};
> +
> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp)
> +{
> +	struct device *dev = pp->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node =  of_get_next_child(node, NULL);
> +
> +	if (!pcie_intc_node) {
> +		dev_err(dev, "No PCIe Intc node found\n");
> +		return PTR_ERR(pcie_intc_node);
> +	}
> +	pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4,
> +					       &intx_domain_ops, pp);
> +	if (!pp->irq_domain) {
> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> +		return PTR_ERR(pp->irq_domain);
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
> +{
> +	struct rockchip_pcie_port *pp = arg;
> +	u32 reg;
> +	u32 sub_reg;
> +
> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
> +	if (reg & LOC_INT) {
> +		dev_dbg(pp->dev, "local interrupt recived\n");
> +		sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
> +		if (sub_reg & PRFPE)
> +			dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
> +
> +		if (sub_reg & CRFPE)
> +			dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
> +
> +		if (sub_reg & RRPE)
> +			dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
> +
> +		if (sub_reg & PRFO)
> +			dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
> +
> +		if (sub_reg & CRFO)
> +			dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
> +
> +		if (sub_reg & RT)
> +			dev_dbg(pp->dev, "Replay timer timed out\n");
> +
> +		if (sub_reg & RTR)
> +			dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
> +
> +		if (sub_reg & PE)
> +			dev_dbg(pp->dev, "Phy error detected on receive side\n");
> +
> +		if (sub_reg & MTR)
> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
> +
> +		if (sub_reg & UCR)
> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
> +
> +		if (sub_reg & FCE)
> +			dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
> +
> +		if (sub_reg & CT)
> +			dev_dbg(pp->dev, "A request timed out waiting for completion\n");
> +
> +		if (sub_reg & UTC)
> +			dev_dbg(pp->dev, "Unmapped TC error\n");
> +
> +		if (sub_reg & MMVC)
> +			dev_dbg(pp->dev, "MSI mask register changes\n");
> +
> +		pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
> +	}
> +
> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg)
> +{
> +	struct rockchip_pcie_port *pp = arg;
> +	u32 reg;
> +
> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
> +	if (reg & LEGACY_DONE)
> +		dev_dbg(pp->dev, "legacy done interrupt recived\n");
> +
> +	if (reg & MSG_DONE)
> +		dev_dbg(pp->dev, "message done interrupt recived\n");
> +
> +	if (reg & HOT_RESET)
> +		dev_dbg(pp->dev, "hot reset interrupt recived\n");
> +
> +	if (reg & DPA_INT)
> +		dev_dbg(pp->dev, "dpa interrupt recived\n");
> +
> +	if (reg & FATAL_ERR)
> +		dev_dbg(pp->dev, "fatal error interrupt recived\n");
> +
> +	if (reg & DPA_INT)
> +		dev_dbg(pp->dev, "no fatal error interrupt recived\n");
> +
> +	if (reg & CORR_ERR)
> +		dev_dbg(pp->dev, "correctable error interrupt recived\n");
> +
> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);

This is a curosity question. You have 3 interrupt handlers, and in all 3 of them
you are checking the same register (PCIE_CLIENT_INT_STATUS) and unconditionally
clearing all the interrupts in that register. So I think if pci-sys and legacy
are raised at the same time, the 1st interrupt handler that gets called may clear
all interrupts and the second may find none to service. This may be OK, I'm wondering
if this is the intention?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg)
> +{
> +	struct rockchip_pcie_port *pp = arg;
> +	u32 reg;
> +
> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
> +	reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
> +	       ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
> +	generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));
> +
> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
> +	return IRQ_HANDLED;
> +}
> +
> +static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie_port *pp,
> +				     int region_no,
> +				     int type, u8 num_pass_bits,
> +				     u32 lower_addr, u32 upper_addr)
> +{
> +	u32 ob_addr_0 = 0;
> +	u32 ob_addr_1 = 0;
> +	u32 ob_desc_0 = 0;
> +	u32 ob_desc_1 = 0;
> +	void __iomem *aw_base;
> +
> +	if (!pp)
> +		return -EINVAL;
> +	if (region_no >= MAX_AXI_WRAPPER_REGION_NUM)
> +		return -EINVAL;
> +	if ((num_pass_bits + 1) < 8)
> +		return -EINVAL;
> +	if (num_pass_bits > 63)
> +		return -EINVAL;
> +	if (region_no == 0) {
> +		if (AXI_REGION_0_SIZE < (2ULL << num_pass_bits))
> +		return -EINVAL;
> +	}
> +	if (region_no != 0) {
> +		if (AXI_REGION_SIZE < (2ULL << num_pass_bits))
> +		return -EINVAL;
> +	}
> +	aw_base = pp->apb_base + PCIE_CORE_AXI_CONF_BASE;
> +	aw_base += (region_no << OB_REG_SIZE_SHIFT);
> +
> +	ob_addr_0 = (ob_addr_0 &
> +		     ~0x0000003fU) | (num_pass_bits &
> +		     0x0000003fU);
> +	ob_addr_0 = (ob_addr_0 &
> +		     ~0xffffff00U) | (lower_addr & 0xffffff00U);
> +	ob_addr_1 = upper_addr;
> +	ob_desc_0 = (1 << 23 | type);
> +
> +	writel(ob_addr_0, aw_base);
> +	writel(ob_addr_1, aw_base + 0x4);
> +	writel(ob_desc_0, aw_base + 0x8);
> +	writel(ob_desc_1, aw_base + 0xc);
> +
> +	return 0;
> +}
> +
> +static int rockchip_pcie_prog_ib_atu(struct rockchip_pcie_port *pp,
> +				     int region_no,
> +				     u8 num_pass_bits,
> +				     u32 lower_addr,
> +				     u32 upper_addr)
> +{
> +	u32 ib_addr_0 = 0;
> +	u32 ib_addr_1 = 0;
> +	void __iomem *aw_base;
> +
> +	if (!pp)
> +		return -EINVAL;
> +	if (region_no > MAX_AXI_IB_ROOTPORT_REGION_NUM)
> +		return -EINVAL;
> +	if ((num_pass_bits + 1) < MIN_AXI_ADDR_BITS_PASSED)
> +		return -EINVAL;
> +	if (num_pass_bits > 63)
> +		return -EINVAL;
> +	aw_base = pp->apb_base + PCIE_CORE_AXI_INBOUND_BASE;
> +	aw_base += (region_no << IB_ROOT_PORT_REG_SIZE_SHIFT);
> +	ib_addr_0 = (ib_addr_0 &
> +		     ~0x0000003fU) | (num_pass_bits &
> +		     0x0000003fU);
> +
> +	ib_addr_0 = (ib_addr_0 & ~0xffffff00U) |
> +		     ((lower_addr << 8) & 0xffffff00U);
> +	ib_addr_1 = upper_addr;
> +	writel(ib_addr_0, aw_base);
> +	writel(ib_addr_1, aw_base + 0x4);
> +
> +	return 0;
> +}
> +
> +static int rockchip_pcie_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_pcie_port *port;
> +	struct device *dev = &pdev->dev;
> +	struct pci_bus *bus, *child;
> +	struct resource_entry *win;
> +	int reg_no;
> +	int err = 0;
> +	int irq;
> +	LIST_HEAD(res);
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-sys");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie_sys IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
> +			       IRQF_SHARED, "pcie-sys", port);

I am a little worried because you are installing all the 3
interrupt handlers, passing "port *" as the argument - even
though the struct port is not initialized yet. Specifically the
pointers (port->apb_base) that irq handler would dereference. 
Also, the irq domain is not initialized. May be I'm
just cynical, but if there is an interrupt pending, I'm wondering
if your interrupt handler can get called even before you get a
chance to initialize your struct rockchip_pcie_port *port or irq domain?
If so, it may crash. So may be better to first parse dt and initialize
your structures before registering the interrupt handler(s)?

> +	if (err) {
> +		dev_err(dev, "failed to request pcie subsystem irq\n");
> +		return err;
> +	}
> +
> +	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
> +	if (port->irq < 0) {
> +		dev_err(dev, "missing pcie_legacy IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, port->irq,
> +			       rockchip_pcie_legacy_int_handler,
> +			       IRQF_SHARED,
> +			       "pcie-legacy",
> +			       port);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
> +		return err;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-client");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie-client IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
> +			       IRQF_SHARED, "pcie-client", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie client irq\n");
> +		return err;
> +	}
> +
> +	port->dev = dev;
> +	err = rockchip_pcie_parse_dt(port);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +
> +	err = rockchip_pcie_init_port(port);
> +	if (err)
> +		return err;

What about clocks? Do you need to clk_disable_unprepare() them
on this failure or any subsequent failure in probe?

> +
> +	platform_set_drvdata(pdev, port);
> +
> +	rockchip_pcie_enable_interrupts(port);
> +	if (!IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = rockchip_pcie_init_irq_domain(port);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
> +					       &res, &port->io_base);
> +	if (err)
> +		return err;
> +	/* Get the I/O and memory ranges from DT */
> +	resource_list_for_each_entry(win, &res) {
> +		switch (resource_type(win->res)) {
> +		case IORESOURCE_IO:
> +			port->io = win->res;
> +			port->io->name = "I/O";
> +			port->io_size = resource_size(port->io);
> +			port->io_bus_addr = port->io->start - win->offset;
> +			err = pci_remap_iospace(port->io, port->io_base);
> +			if (err) {
> +				dev_warn(port->dev, "error %d: failed to map resource %pR\n",
> +					 err, port->io);
> +				continue;
> +			}
> +			break;
> +		case IORESOURCE_MEM:
> +			port->mem = win->res;
> +			port->mem->name = "MEM";
> +			port->mem_size = resource_size(port->mem);
> +			port->mem_bus_addr = port->mem->start - win->offset;
> +			break;
> +		case 0:
> +			port->cfg = win->res;
> +			break;
> +		case IORESOURCE_BUS:
> +			port->busn = win->res;
> +			break;
> +		default:
> +			continue;
> +		}
> +	}
> +
> +	pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8);
> +	pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300);
> +
> +	pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + RC_REGION_0_PASS_BITS),
> +		   PCIE_CORE_AXI_CONF_BASE);
> +	pcie_write(port, RC_REGION_0_ADDR_TRANS_H,
> +		   PCIE_CORE_AXI_CONF_BASE + 0x4);
> +	pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8);
> +	pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc);
> +
> +	for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) {
> +		err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
> +						AXI_WRAPPER_MEM_WRITE,
> +						20 - 1,
> +						port->mem_bus_addr +
> +							(reg_no << 20),
> +						0);
> +		if (err) {
> +			dev_err(dev, "Program RC outbound atu failed\n");
> +			return err;
> +		}
> +	}
> +
> +	err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0);
> +	if (err) {
> +		dev_err(dev, "Program RC inbound atu failed\n");
> +		return err;
> +	}
> +
> +	rockchip_pcie_msi_enable(port);
> +
> +	port->root_bus_nr = port->busn->start;
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr,
> +					    &rockchip_pcie_ops, port, &res,
> +					    port->msi);
> +	} else {
> +		bus = pci_scan_root_bus(&pdev->dev, 0,
> +					&rockchip_pcie_ops, port, &res);
> +	}
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		pci_bus_size_bridges(bus);
> +		pci_bus_assign_resources(bus);
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	pci_bus_add_devices(bus);
> +
> +	return err;
> +}
> +
> +static int rockchip_pcie_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_pcie_port *port = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(port->hclk_pcie);
> +	clk_disable_unprepare(port->aclk_perf_pcie);
> +	clk_disable_unprepare(port->aclk_pcie);
> +	clk_disable_unprepare(port->clk_pciephy_ref);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rockchip_pcie_of_match[] = {
> +	{ .compatible = "rockchip,rk3399-pcie", },
> +	{}
> +};

Please add a MODULE_DEVICE_TABLE() here.

> +
> +static struct platform_driver rockchip_pcie_driver = {
> +	.driver = {
> +		.name = "rockchip-pcie",
> +		.of_match_table = rockchip_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = rockchip_pcie_probe,
> +	.remove = rockchip_pcie_remove,
> +};
> +module_platform_driver(rockchip_pcie_driver);
> +
> +MODULE_AUTHOR("Rockchip Inc");
> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Marc Zyngier]

[+Lorenzo]

On 20/05/16 11:29, Shawn Lin wrote:
> RK3399 has a PCIe controller which can be used as Root Complex.
> This driver supports a PCIe controller as Root Complex mode.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/pci/host/Kconfig         |   12 +
>  drivers/pci/host/Makefile        |    1 +
>  drivers/pci/host/pcie-rockchip.c | 1181 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1194 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-rockchip.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 8e4f038..76447a8 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -245,4 +245,16 @@ config PCIE_ARMADA_8K
>  	  Designware hardware and therefore the driver re-uses the
>  	  Designware core functions to implement the driver.
>  
> +config PCIE_ROCKCHIP
> +	bool "Rockchip PCIe controller"
> +	depends on ARM64 && ARCH_ROCKCHIP
> +	depends on OF
> +	select MFD_SYSCON
> +	select PCI_MSI
> +	select PCI_MSI_IRQ_DOMAIN
> +	help
> +	  Say Y here if you want internal PCI support on Rockchip SoC.
> +	  There are 1 internal PCIe port available to support GEN2 with
> +	  4 slots.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index a6f85e3..fb3871e 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
>  obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> new file mode 100644
> index 0000000..4063fd3
> --- /dev/null
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -0,0 +1,1181 @@
> +/*
> + * Rockchip AXI PCIe host controller driver
> + *
> + * Copyright (c) 2016 Rockchip, Inc.
> + *
> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
> + *         Wenrui Li <wenrui.li@rock-chips.com>
> + * Bits taken from Synopsys Designware Host controller driver and
> + * ARM PCI Host generic driver.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/regmap.h>
> +
> +#define REF_CLK_100MHZ			(100 * 1000 * 1000)
> +#define PCIE_CLIENT_BASE		0x0
> +#define PCIE_RC_CONFIG_BASE		0xa00000
> +#define PCIE_CORE_CTRL_MGMT_BASE	0x900000
> +#define PCIE_CORE_AXI_CONF_BASE		0xc00000
> +#define PCIE_CORE_AXI_INBOUND_BASE	0xc00800
> +
> +#define PCIE_CLIENT_BASIC_STATUS0	0x44
> +#define PCIE_CLIENT_BASIC_STATUS1	0x48
> +#define PCIE_CLIENT_INT_MASK		0x4c
> +#define PCIE_CLIENT_INT_STATUS		0x50
> +#define PCIE_CORE_INT_MASK		0x900210
> +#define PCIE_CORE_INT_STATUS		0x90020c
> +
> +/** Size of one AXI Region (not Region 0) */
> +#define	AXI_REGION_SIZE			(0x1 << 20)
> +/** Overall size of AXI area */
> +#define	AXI_OVERALL_SIZE		(64 * (0x1 << 20))
> +/** Size of Region 0, equal to sum of sizes of other regions */
> +#define	AXI_REGION_0_SIZE		(32 * (0x1 << 20))
> +#define OB_REG_SIZE_SHIFT		5
> +#define IB_ROOT_PORT_REG_SIZE_SHIFT	3
> +
> +#define AXI_WRAPPER_IO_WRITE		0x6
> +#define AXI_WRAPPER_MEM_WRITE		0x2
> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM	3
> +#define	MIN_AXI_ADDR_BITS_PASSED	8
> +
> +#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK	GENMASK(8, 5)
> +#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT	5
> +#define CLIENT_INTERRUPTS \
> +		(LOC_INT | INTA | INTB | INTC | INTD |\
> +		 CORR_ERR | NFATAL_ERR | FATAL_ERR | DPA_INT | \
> +		 HOT_RESET | MSG_DONE | LEGACY_DONE)
> +#define CORE_INTERRUPTS	\
> +		(PRFPE | CRFPE | RRPE | CRFO | RT | RTR | \
> +		 PE | MTR | UCR | FCE | CT | UTC | MMVC)
> +#define PWR_STCG			BIT(0)
> +#define HOT_PLUG			BIT(1)
> +#define PHY_INT				BIT(2)
> +#define UDMA_INT			BIT(3)
> +#define LOC_INT				BIT(4)
> +#define INTA				BIT(5)
> +#define INTB				BIT(6)
> +#define INTC				BIT(7)
> +#define INTD				BIT(8)
> +#define CORR_ERR			BIT(9)
> +#define NFATAL_ERR			BIT(10)
> +#define FATAL_ERR			BIT(11)
> +#define DPA_INT				BIT(12)
> +#define HOT_RESET			BIT(13)
> +#define MSG_DONE			BIT(14)
> +#define LEGACY_DONE			BIT(15)
> +#define PRFPE				BIT(0)
> +#define CRFPE				BIT(1)
> +#define RRPE				BIT(2)
> +#define PRFO				BIT(3)
> +#define CRFO				BIT(4)
> +#define RT				BIT(5)
> +#define RTR				BIT(6)
> +#define PE				BIT(7)
> +#define MTR				BIT(8)
> +#define UCR				BIT(9)
> +#define FCE				BIT(10)
> +#define CT				BIT(11)
> +#define UTC				BIT(18)
> +#define MMVC				BIT(19)
> +
> +#define PCIE_ECAM_BUS(x)		(((x) & 0xFF) << 20)
> +#define PCIE_ECAM_DEV(x)		(((x) & 0x1F) << 15)
> +#define PCIE_ECAM_FUNC(x)		(((x) & 0x7) << 12)
> +#define PCIE_ECAM_REG(x)		(((x) & 0xFFF) << 0)
> +#define PCIE_ECAM_ADDR(bus, dev, func, reg) \
> +	  (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \
> +	   PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg))
> +
> +#define RC_REGION_0_ADDR_TRANS_H	0x00000000
> +#define RC_REGION_0_ADDR_TRANS_L	0x00000000
> +#define RC_REGION_0_PASS_BITS		(25 - 1)
> +#define RC_REGION_1_ADDR_TRANS_H	0x00000000
> +#define RC_REGION_1_ADDR_TRANS_L	0x00400000
> +#define RC_REGION_1_PASS_BITS		(20 - 1)
> +#define MAX_AXI_WRAPPER_REGION_NUM	33
> +#define PCIE_CLIENT_CONF_ENABLE		BIT(0)
> +#define PCIE_CLIENT_CONF_LANE_NUM(x)	((x / 2) << 4)
> +#define PCIE_CLIENT_MODE_RC		BIT(6)
> +#define PCIE_CLIENT_GEN_SEL_2		BIT(7)
> +#define PCIE_CLIENT_GEN_SEL_1		0x0
> +
> +struct rockchip_pcie_port {
> +	void __iomem *reg_base;
> +	void __iomem *apb_base;
> +	struct regmap *grf;
> +	unsigned int pcie_conf;
> +	unsigned int pcie_status;
> +	unsigned int pcie_laneoff;
> +	struct reset_control *phy_rst;
> +	struct reset_control *core_rst;
> +	struct reset_control *mgmt_rst;
> +	struct reset_control *mgmt_sticky_rst;
> +	struct reset_control *pipe_rst;
> +	struct clk *aclk_pcie;
> +	struct clk *aclk_perf_pcie;
> +	struct clk *hclk_pcie;
> +	struct clk *clk_pciephy_ref;
> +	struct gpio_desc *ep_gpio;
> +	u32 lanes;
> +	resource_size_t		io_base;
> +	struct resource		*cfg;
> +	struct resource		*io;
> +	struct resource		*mem;
> +	struct resource		*busn;
> +	phys_addr_t		io_bus_addr;
> +	u32			io_size;
> +	phys_addr_t		mem_bus_addr;
> +	u32			mem_size;
> +	u8	root_bus_nr;
> +	int irq;
> +	struct msi_controller *msi;

Don't. struct msi_controller shouldn't be used in new code, and
certainly not for an arm64 system.

But even worse: your SoC seems to use a GICv3 ITS. Great. Which means
you do not need any of that at all.

> +
> +	struct device *dev;
> +	struct irq_domain *irq_domain;
> +};

[...]


> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp)
> +{
> +	struct device_node *msi_node;
> +
> +	msi_node = of_parse_phandle(pp->dev->of_node,
> +				    "msi-parent", 0);
> +	if (!msi_node)
> +		return;
> +
> +	pp->msi = of_pci_find_msi_chip_by_node(msi_node);

How funny. The ITS is never registered there (actually, nobody seems to
register anything there anymore), so this will always return NULL. This
whole function is thus completely useless.

> +	of_node_put(msi_node);
> +
> +	if (pp->msi)
> +		pp->msi->dev = pp->dev;
> +}
> +
> +static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *pp)
> +{
> +	pcie_write(pp, (CLIENT_INTERRUPTS << 16) &
> +		   (~CLIENT_INTERRUPTS), PCIE_CLIENT_INT_MASK);
> +	pcie_write(pp, CORE_INTERRUPTS, PCIE_CORE_INT_MASK);
> +}
> +
> +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
> +				  irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> +	.map = rockchip_pcie_intx_map,
> +};
> +
> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp)
> +{
> +	struct device *dev = pp->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node =  of_get_next_child(node, NULL);

That's really ugly, as it depends on the layout of your DT.

> +
> +	if (!pcie_intc_node) {
> +		dev_err(dev, "No PCIe Intc node found\n");
> +		return PTR_ERR(pcie_intc_node);
> +	}
> +	pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4,
> +					       &intx_domain_ops, pp);

Why can't you just register your host controller as the interrupt
controller? You don't need an intermediate node for that.

> +	if (!pp->irq_domain) {
> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> +		return PTR_ERR(pp->irq_domain);
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
> +{
> +	struct rockchip_pcie_port *pp = arg;
> +	u32 reg;
> +	u32 sub_reg;
> +
> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
> +	if (reg & LOC_INT) {
> +		dev_dbg(pp->dev, "local interrupt recived\n");
> +		sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
> +		if (sub_reg & PRFPE)
> +			dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
> +
> +		if (sub_reg & CRFPE)
> +			dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
> +
> +		if (sub_reg & RRPE)
> +			dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
> +
> +		if (sub_reg & PRFO)
> +			dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
> +
> +		if (sub_reg & CRFO)
> +			dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
> +
> +		if (sub_reg & RT)
> +			dev_dbg(pp->dev, "Replay timer timed out\n");
> +
> +		if (sub_reg & RTR)
> +			dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
> +
> +		if (sub_reg & PE)
> +			dev_dbg(pp->dev, "Phy error detected on receive side\n");
> +
> +		if (sub_reg & MTR)
> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
> +
> +		if (sub_reg & UCR)
> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
> +
> +		if (sub_reg & FCE)
> +			dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
> +
> +		if (sub_reg & CT)
> +			dev_dbg(pp->dev, "A request timed out waiting for completion\n");
> +
> +		if (sub_reg & UTC)
> +			dev_dbg(pp->dev, "Unmapped TC error\n");
> +
> +		if (sub_reg & MMVC)
> +			dev_dbg(pp->dev, "MSI mask register changes\n");
> +
> +		pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
> +	}
> +
> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg)
> +{
> +	struct rockchip_pcie_port *pp = arg;
> +	u32 reg;
> +
> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
> +	if (reg & LEGACY_DONE)
> +		dev_dbg(pp->dev, "legacy done interrupt recived\n");
> +
> +	if (reg & MSG_DONE)
> +		dev_dbg(pp->dev, "message done interrupt recived\n");
> +
> +	if (reg & HOT_RESET)
> +		dev_dbg(pp->dev, "hot reset interrupt recived\n");
> +
> +	if (reg & DPA_INT)
> +		dev_dbg(pp->dev, "dpa interrupt recived\n");
> +
> +	if (reg & FATAL_ERR)
> +		dev_dbg(pp->dev, "fatal error interrupt recived\n");
> +
> +	if (reg & DPA_INT)
> +		dev_dbg(pp->dev, "no fatal error interrupt recived\n");
> +
> +	if (reg & CORR_ERR)
> +		dev_dbg(pp->dev, "correctable error interrupt recived\n");
> +
> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg)
> +{
> +	struct rockchip_pcie_port *pp = arg;
> +	u32 reg;
> +
> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
> +	reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
> +	       ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
> +	generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));

NAK. What you have here is a chained interrupt controller, please
implement it as such.

> +
> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
> +	return IRQ_HANDLED;
> +}
> +
> +static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie_port *pp,
> +				     int region_no,
> +				     int type, u8 num_pass_bits,
> +				     u32 lower_addr, u32 upper_addr)
> +{
> +	u32 ob_addr_0 = 0;
> +	u32 ob_addr_1 = 0;
> +	u32 ob_desc_0 = 0;
> +	u32 ob_desc_1 = 0;
> +	void __iomem *aw_base;
> +
> +	if (!pp)
> +		return -EINVAL;
> +	if (region_no >= MAX_AXI_WRAPPER_REGION_NUM)
> +		return -EINVAL;
> +	if ((num_pass_bits + 1) < 8)
> +		return -EINVAL;
> +	if (num_pass_bits > 63)
> +		return -EINVAL;
> +	if (region_no == 0) {
> +		if (AXI_REGION_0_SIZE < (2ULL << num_pass_bits))
> +		return -EINVAL;
> +	}
> +	if (region_no != 0) {
> +		if (AXI_REGION_SIZE < (2ULL << num_pass_bits))
> +		return -EINVAL;
> +	}
> +	aw_base = pp->apb_base + PCIE_CORE_AXI_CONF_BASE;
> +	aw_base += (region_no << OB_REG_SIZE_SHIFT);
> +
> +	ob_addr_0 = (ob_addr_0 &
> +		     ~0x0000003fU) | (num_pass_bits &
> +		     0x0000003fU);
> +	ob_addr_0 = (ob_addr_0 &
> +		     ~0xffffff00U) | (lower_addr & 0xffffff00U);
> +	ob_addr_1 = upper_addr;
> +	ob_desc_0 = (1 << 23 | type);
> +
> +	writel(ob_addr_0, aw_base);
> +	writel(ob_addr_1, aw_base + 0x4);
> +	writel(ob_desc_0, aw_base + 0x8);
> +	writel(ob_desc_1, aw_base + 0xc);
> +
> +	return 0;
> +}
> +
> +static int rockchip_pcie_prog_ib_atu(struct rockchip_pcie_port *pp,
> +				     int region_no,
> +				     u8 num_pass_bits,
> +				     u32 lower_addr,
> +				     u32 upper_addr)
> +{
> +	u32 ib_addr_0 = 0;
> +	u32 ib_addr_1 = 0;
> +	void __iomem *aw_base;
> +
> +	if (!pp)
> +		return -EINVAL;
> +	if (region_no > MAX_AXI_IB_ROOTPORT_REGION_NUM)
> +		return -EINVAL;
> +	if ((num_pass_bits + 1) < MIN_AXI_ADDR_BITS_PASSED)
> +		return -EINVAL;
> +	if (num_pass_bits > 63)
> +		return -EINVAL;
> +	aw_base = pp->apb_base + PCIE_CORE_AXI_INBOUND_BASE;
> +	aw_base += (region_no << IB_ROOT_PORT_REG_SIZE_SHIFT);
> +	ib_addr_0 = (ib_addr_0 &
> +		     ~0x0000003fU) | (num_pass_bits &
> +		     0x0000003fU);
> +
> +	ib_addr_0 = (ib_addr_0 & ~0xffffff00U) |
> +		     ((lower_addr << 8) & 0xffffff00U);
> +	ib_addr_1 = upper_addr;
> +	writel(ib_addr_0, aw_base);
> +	writel(ib_addr_1, aw_base + 0x4);
> +
> +	return 0;
> +}
> +
> +static int rockchip_pcie_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_pcie_port *port;
> +	struct device *dev = &pdev->dev;
> +	struct pci_bus *bus, *child;
> +	struct resource_entry *win;
> +	int reg_no;
> +	int err = 0;
> +	int irq;
> +	LIST_HEAD(res);
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-sys");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie_sys IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler,
> +			       IRQF_SHARED, "pcie-sys", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie subsystem irq\n");
> +		return err;
> +	}
> +
> +	port->irq = platform_get_irq_byname(pdev, "pcie-legacy");
> +	if (port->irq < 0) {
> +		dev_err(dev, "missing pcie_legacy IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, port->irq,
> +			       rockchip_pcie_legacy_int_handler,
> +			       IRQF_SHARED,
> +			       "pcie-legacy",
> +			       port);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to request pcie-legacy irq\n");
> +		return err;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "pcie-client");
> +	if (irq < 0) {
> +		dev_err(dev, "missing pcie-client IRQ resource\n");
> +		return -EINVAL;
> +	}
> +	err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler,
> +			       IRQF_SHARED, "pcie-client", port);
> +	if (err) {
> +		dev_err(dev, "failed to request pcie client irq\n");
> +		return err;
> +	}
> +
> +	port->dev = dev;
> +	err = rockchip_pcie_parse_dt(port);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +
> +	err = rockchip_pcie_init_port(port);
> +	if (err)
> +		return err;
> +
> +	platform_set_drvdata(pdev, port);
> +
> +	rockchip_pcie_enable_interrupts(port);
> +	if (!IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = rockchip_pcie_init_irq_domain(port);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
> +					       &res, &port->io_base);
> +	if (err)
> +		return err;
> +	/* Get the I/O and memory ranges from DT */
> +	resource_list_for_each_entry(win, &res) {
> +		switch (resource_type(win->res)) {
> +		case IORESOURCE_IO:
> +			port->io = win->res;
> +			port->io->name = "I/O";
> +			port->io_size = resource_size(port->io);
> +			port->io_bus_addr = port->io->start - win->offset;
> +			err = pci_remap_iospace(port->io, port->io_base);
> +			if (err) {
> +				dev_warn(port->dev, "error %d: failed to map resource %pR\n",
> +					 err, port->io);
> +				continue;
> +			}
> +			break;
> +		case IORESOURCE_MEM:
> +			port->mem = win->res;
> +			port->mem->name = "MEM";
> +			port->mem_size = resource_size(port->mem);
> +			port->mem_bus_addr = port->mem->start - win->offset;
> +			break;
> +		case 0:
> +			port->cfg = win->res;
> +			break;
> +		case IORESOURCE_BUS:
> +			port->busn = win->res;
> +			break;
> +		default:
> +			continue;
> +		}
> +	}
> +
> +	pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8);
> +	pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300);
> +
> +	pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + RC_REGION_0_PASS_BITS),
> +		   PCIE_CORE_AXI_CONF_BASE);
> +	pcie_write(port, RC_REGION_0_ADDR_TRANS_H,
> +		   PCIE_CORE_AXI_CONF_BASE + 0x4);
> +	pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8);
> +	pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc);
> +
> +	for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) {
> +		err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
> +						AXI_WRAPPER_MEM_WRITE,
> +						20 - 1,
> +						port->mem_bus_addr +
> +							(reg_no << 20),
> +						0);
> +		if (err) {
> +			dev_err(dev, "Program RC outbound atu failed\n");
> +			return err;
> +		}
> +	}
> +
> +	err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0);
> +	if (err) {
> +		dev_err(dev, "Program RC inbound atu failed\n");
> +		return err;
> +	}
> +
> +	rockchip_pcie_msi_enable(port);
> +
> +	port->root_bus_nr = port->busn->start;
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr,
> +					    &rockchip_pcie_ops, port, &res,
> +					    port->msi);

You don't need this. The infrastructure will do the right thing for you.

> +	} else {
> +		bus = pci_scan_root_bus(&pdev->dev, 0,
> +					&rockchip_pcie_ops, port, &res);
> +	}
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	if (!pci_has_flag(PCI_PROBE_ONLY)) {

Why do you have catter for the PCI_PROBE_ONLY case? Nobody should ever
use that for properly implemented HW.

> +		pci_bus_size_bridges(bus);
> +		pci_bus_assign_resources(bus);
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +
> +	pci_bus_add_devices(bus);
> +
> +	return err;
> +}
> +
> +static int rockchip_pcie_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_pcie_port *port = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(port->hclk_pcie);
> +	clk_disable_unprepare(port->aclk_perf_pcie);
> +	clk_disable_unprepare(port->aclk_pcie);
> +	clk_disable_unprepare(port->clk_pciephy_ref);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rockchip_pcie_of_match[] = {
> +	{ .compatible = "rockchip,rk3399-pcie", },
> +	{}
> +};
> +
> +static struct platform_driver rockchip_pcie_driver = {
> +	.driver = {
> +		.name = "rockchip-pcie",
> +		.of_match_table = rockchip_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = rockchip_pcie_probe,
> +	.remove = rockchip_pcie_remove,
> +};
> +module_platform_driver(rockchip_pcie_driver);
> +
> +MODULE_AUTHOR("Rockchip Inc");
> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
> +MODULE_LICENSE("GPL v2");
> 

This definitely requires some rework for both the interrupt and MSI
parts. I'll leave Lorenzo to comment on the PCI side of things.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Wenrui Li]

Hi:

On 2016/5/27 20:25, Marc Zyngier Wrote:
> [+Lorenzo]
>
> On 20/05/16 11:29, Shawn Lin wrote:
>> RK3399 has a PCIe controller which can be used as Root Complex.
>> This driver supports a PCIe controller as Root Complex mode.
>>

[....]

>> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp)
>> +{
>> +	struct device *dev = pp->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct device_node *pcie_intc_node =  of_get_next_child(node, NULL);
>
> That's really ugly, as it depends on the layout of your DT.
>
>> +
>> +	if (!pcie_intc_node) {
>> +		dev_err(dev, "No PCIe Intc node found\n");
>> +		return PTR_ERR(pcie_intc_node);
>> +	}
>> +	pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4,
>> +					       &intx_domain_ops, pp);
>
> Why can't you just register your host controller as the interrupt
> controller? You don't need an intermediate node for that.

OK, the child node is really no need here, we will use the host 
controller as interrupt controller next patch. Thanks!

>
>> +	if (!pp->irq_domain) {
>> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
>> +		return PTR_ERR(pp->irq_domain);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
>> +{
>> +	struct rockchip_pcie_port *pp = arg;
>> +	u32 reg;
>> +	u32 sub_reg;
>> +
>> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
>> +	if (reg & LOC_INT) {
>> +		dev_dbg(pp->dev, "local interrupt recived\n");
>> +		sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
>> +		if (sub_reg & PRFPE)
>> +			dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
>> +
>> +		if (sub_reg & CRFPE)
>> +			dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
>> +
>> +		if (sub_reg & RRPE)
>> +			dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
>> +
>> +		if (sub_reg & PRFO)
>> +			dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
>> +
>> +		if (sub_reg & CRFO)
>> +			dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
>> +
>> +		if (sub_reg & RT)
>> +			dev_dbg(pp->dev, "Replay timer timed out\n");
>> +
>> +		if (sub_reg & RTR)
>> +			dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
>> +
>> +		if (sub_reg & PE)
>> +			dev_dbg(pp->dev, "Phy error detected on receive side\n");
>> +
>> +		if (sub_reg & MTR)
>> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>> +
>> +		if (sub_reg & UCR)
>> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>> +
>> +		if (sub_reg & FCE)
>> +			dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
>> +
>> +		if (sub_reg & CT)
>> +			dev_dbg(pp->dev, "A request timed out waiting for completion\n");
>> +
>> +		if (sub_reg & UTC)
>> +			dev_dbg(pp->dev, "Unmapped TC error\n");
>> +
>> +		if (sub_reg & MMVC)
>> +			dev_dbg(pp->dev, "MSI mask register changes\n");
>> +
>> +		pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
>> +	}
>> +
>> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
>> +
>> +	return IRQ_HANDLED;
>> +}

[....]

>> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg)
>> +{
>> +	struct rockchip_pcie_port *pp = arg;
>> +	u32 reg;
>> +
>> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
>> +	reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
>> +	       ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
>> +	generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));
>
> NAK. What you have here is a chained interrupt controller, please
> implement it as such.

Your mean is use handle_nested_irq instead of generic_handle_irq here?
But, I found all other pci host controller drivers use this api.

>
>> +
>> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
>> +	return IRQ_HANDLED;
>> +}
>> +

[...]

>> +static struct platform_driver rockchip_pcie_driver = {
>> +	.driver = {
>> +		.name = "rockchip-pcie",
>> +		.of_match_table = rockchip_pcie_of_match,
>> +		.suppress_bind_attrs = true,
>> +	},
>> +	.probe = rockchip_pcie_probe,
>> +	.remove = rockchip_pcie_remove,
>> +};
>> +module_platform_driver(rockchip_pcie_driver);
>> +
>> +MODULE_AUTHOR("Rockchip Inc");
>> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
> This definitely requires some rework for both the interrupt and MSI
> parts. I'll leave Lorenzo to comment on the PCI side of things.
>
> Thanks,
>
> 	M.
>

Best Regards,
Li

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Marc Zyngier]

On 01/06/16 03:56, Wenrui Li wrote:
> Hi:
> 
> On 2016/5/27 20:25, Marc Zyngier Wrote:
>> [+Lorenzo]
>>
>> On 20/05/16 11:29, Shawn Lin wrote:
>>> RK3399 has a PCIe controller which can be used as Root Complex.
>>> This driver supports a PCIe controller as Root Complex mode.
>>>
> 
> [....]
> 
>>> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp)
>>> +{
>>> +	struct device *dev = pp->dev;
>>> +	struct device_node *node = dev->of_node;
>>> +	struct device_node *pcie_intc_node =  of_get_next_child(node, NULL);
>>
>> That's really ugly, as it depends on the layout of your DT.
>>
>>> +
>>> +	if (!pcie_intc_node) {
>>> +		dev_err(dev, "No PCIe Intc node found\n");
>>> +		return PTR_ERR(pcie_intc_node);
>>> +	}
>>> +	pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4,
>>> +					       &intx_domain_ops, pp);
>>
>> Why can't you just register your host controller as the interrupt
>> controller? You don't need an intermediate node for that.
> 
> OK, the child node is really no need here, we will use the host 
> controller as interrupt controller next patch. Thanks!
> 
>>
>>> +	if (!pp->irq_domain) {
>>> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
>>> +		return PTR_ERR(pp->irq_domain);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
>>> +{
>>> +	struct rockchip_pcie_port *pp = arg;
>>> +	u32 reg;
>>> +	u32 sub_reg;
>>> +
>>> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
>>> +	if (reg & LOC_INT) {
>>> +		dev_dbg(pp->dev, "local interrupt recived\n");
>>> +		sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
>>> +		if (sub_reg & PRFPE)
>>> +			dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
>>> +
>>> +		if (sub_reg & CRFPE)
>>> +			dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
>>> +
>>> +		if (sub_reg & RRPE)
>>> +			dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
>>> +
>>> +		if (sub_reg & PRFO)
>>> +			dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
>>> +
>>> +		if (sub_reg & CRFO)
>>> +			dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
>>> +
>>> +		if (sub_reg & RT)
>>> +			dev_dbg(pp->dev, "Replay timer timed out\n");
>>> +
>>> +		if (sub_reg & RTR)
>>> +			dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
>>> +
>>> +		if (sub_reg & PE)
>>> +			dev_dbg(pp->dev, "Phy error detected on receive side\n");
>>> +
>>> +		if (sub_reg & MTR)
>>> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>>> +
>>> +		if (sub_reg & UCR)
>>> +			dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>>> +
>>> +		if (sub_reg & FCE)
>>> +			dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
>>> +
>>> +		if (sub_reg & CT)
>>> +			dev_dbg(pp->dev, "A request timed out waiting for completion\n");
>>> +
>>> +		if (sub_reg & UTC)
>>> +			dev_dbg(pp->dev, "Unmapped TC error\n");
>>> +
>>> +		if (sub_reg & MMVC)
>>> +			dev_dbg(pp->dev, "MSI mask register changes\n");
>>> +
>>> +		pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
>>> +	}
>>> +
>>> +	pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
> 
> [....]
> 
>>> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg)
>>> +{
>>> +	struct rockchip_pcie_port *pp = arg;
>>> +	u32 reg;
>>> +
>>> +	reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
>>> +	reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
>>> +	       ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
>>> +	generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));
>>
>> NAK. What you have here is a chained interrupt controller, please
>> implement it as such.
> 
> Your mean is use handle_nested_irq instead of generic_handle_irq here?

No, handle_nested_irq() is for threaded interrupts. What I mean is that
you need to configure the interrupt as a cascaded interrupt, and use the
chained_irq_enter/exit helpers. As a rule of thumb, if you're calling
generic_handle_irq() in an interrupt handler, you're doing something wrong.

If you don't do that, you may end up with interrupts that are not EOIed
properly, RCU violations, and double accounting of interrupts.

> But, I found all other pci host controller drivers use this api.

I'm afraid that doesn't make it any better. Buggy code is everywhere,
and it does spread faster than properly written code. Have a look at
drivers/pci/host/pcie-altera.c as an example of what it should look like.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Lorenzo Pieralisi]

On Fri, May 27, 2016 at 01:25:14PM +0100, Marc Zyngier wrote:

[...]

> > +	} else {
> > +		bus = pci_scan_root_bus(&pdev->dev, 0,
> > +					&rockchip_pcie_ops, port, &res);
> > +	}
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> 
> Why do you have catter for the PCI_PROBE_ONLY case? Nobody should ever
> use that for properly implemented HW.

I think that's just copy and paste and it is a useless check given
that the only way we can set that flag on ARM/ARM64 is through DT
(of_pci_check_probe_only()) and I doubt that systems probing this
driver really require a PCI_PROBE_ONLY set-up.

So, unless you can explain to us why it is really needed, please
remove the:

if (!pci_has_flag(PCI_PROBE_ONLY))

check.

Lorenzo

Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Marc Zyngier]

On 03/06/16 09:55, Lorenzo Pieralisi wrote:
> On Fri, May 27, 2016 at 01:25:14PM +0100, Marc Zyngier wrote:
> 
> [...]
> 
>>> +	} else {
>>> +		bus = pci_scan_root_bus(&pdev->dev, 0,
>>> +					&rockchip_pcie_ops, port, &res);
>>> +	}
>>> +	if (!bus)
>>> +		return -ENOMEM;
>>> +
>>> +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
>>
>> Why do you have catter for the PCI_PROBE_ONLY case? Nobody should ever
>> use that for properly implemented HW.
> 
> I think that's just copy and paste and it is a useless check given
> that the only way we can set that flag on ARM/ARM64 is through DT
> (of_pci_check_probe_only()) and I doubt that systems probing this
> driver really require a PCI_PROBE_ONLY set-up.
> 
> So, unless you can explain to us why it is really needed, please
> remove the:
> 
> if (!pci_has_flag(PCI_PROBE_ONLY))
> 
> check.

Agreed. Maybe we should add a comment somewhere saying that this is
deprecated on arm/arm64, and only useful to slightly dumb virtualization
environments (kvmtool being the prime example).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...