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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[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


Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>

---

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


Reply | Threaded
Open this post in threaded view
|

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

Shawn Lin
In reply to this post by 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 <[hidden email]>
---

 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 <[hidden email]>
+ *         Wenrui Li <[hidden email]>
+ * 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


Reply | Threaded
Open this post in threaded view
|

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

Heiko Stuebner
In reply to this post by Shawn Lin
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 <[hidden email]>
>
> ---
>
>  .../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
Reply | Threaded
Open this post in threaded view
|

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

Heiko Stuebner
In reply to this post by Shawn Lin
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 <[hidden email]>
> ---

[...]

> 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 <[hidden email]>
> + *         Wenrui Li <[hidden email]>
> + * 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
Reply | Threaded
Open this post in threaded view
|

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

Shawn Lin
In reply to this post by Heiko Stuebner
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 <[hidden email]>
>>
>> ---
>>
>>  .../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

Reply | Threaded
Open this post in threaded view
|

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

Shawn Lin
In reply to this post by Heiko Stuebner
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 <[hidden email]>
>> ---
>
> [...]
>
>> 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 <[hidden email]>
>> + *         Wenrui Li <[hidden email]>
>> + * 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

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
>>> ---
>>
>> [...]
>>
>>> 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 <[hidden email]>
>>> + *         Wenrui Li <[hidden email]>
>>> + * 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

Reply | Threaded
Open this post in threaded view
|

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

Bharat Kumar Gogada
In reply to this post by Shawn Lin
> +
> + 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.
Reply | Threaded
Open this post in threaded view
|

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

Heiko Stuebner
In reply to this post by Shawn Lin
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 <[hidden email]>
> >>
> >> ---

[...]

> >> +- 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
Reply | Threaded
Open this post in threaded view
|

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

Shawn Lin
In reply to this post by Bharat Kumar Gogada
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

Reply | Threaded
Open this post in threaded view
|

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

Shawn Lin
In reply to this post by Heiko Stuebner
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 <[hidden email]>
>>>>
>>>> ---
>
> [...]
>
>>>> +- 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

Reply | Threaded
Open this post in threaded view
|

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

Arnd Bergmann
In reply to this post by Shawn Lin
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


Reply | Threaded
Open this post in threaded view
|

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

Rajat Jain-6
In reply to this post by Shawn Lin
> 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 <[hidden email]>
> ---
>
>  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 <[hidden email]>
> + *         Wenrui Li <[hidden email]>
> + * 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");