| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 23 Jun 2016 09:09:46 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Brian Norris <briannorris@...omium.org>
Cc: shawn.lin@...k-chips.com, Bjorn Helgaas <bhelgaas@...gle.com>,
devicetree@...r.kernel.org, Heiko Stuebner <heiko@...ech.de>,
Arnd Bergmann <arnd@...db.de>,
Marc Zyngier <marc.zyngier@....com>, linux-pci@...r.kernel.org,
Wenrui Li <wenrui.li@...k-chips.com>,
linux-kernel@...r.kernel.org,
Doug Anderson <dianders@...omium.org>,
linux-rockchip@...ts.infradead.org,
Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH v3 2/2] PCI: Rockchip: Add Rockchip PCIe controller
support
在 2016/6/23 8:29, Brian Norris 写道:
> Hi Shawn,
>
> I'm familiarizing myself a bit with your PCIe core, but I'm certainly no
> expert, so I may have some dumb comments. Also a few other general
> comments.
>
> On Thu, Jun 16, 2016 at 09:50:35AM +0800, Shawn Lin wrote:
>> This patch adds Rockchip PCIe controller support found
>> on RK3399 Soc platform.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@...k-chips.com>
>>
>> ---
>>
>> Changes in v3:
>> - remove header file
>> - remove struct msi_controller and move most of variables
>> of rockchip_pcie_port to become the local ones.
>> - allow legacy int even if enabling MSI
>> - drop regulator set voltage operation suggested by Doug
>>
>> Changes in v2:
>> - remove phy related stuff and call phy API
>> - add new head file and define lots of macro to make
>> the code more readable
>> - remove lots msi related code suggested by Marc
>> - add IO window address translation
>> - init_port and parse_dt reconstruction suggested by Bharat
>> - improve wr_own_conf suggested by Arnd
>> - make pcie as an interrupt controller and fix wrong int handler
>> suggested by Marc
>> - remove PCI_PROBE_ONLY suggested by Lorenzo
>>
>> drivers/pci/host/Kconfig | 11 +
>> drivers/pci/host/Makefile | 1 +
>> drivers/pci/host/pcie-rockchip.c | 1183 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1195 insertions(+)
>> create mode 100644 drivers/pci/host/pcie-rockchip.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 5d2374e..251f4ac 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -245,4 +245,15 @@ 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"
>
> Any good reason this can't be a module? I realize PCIe is something we'd
> likely want built in most of the time, but is there a code reason?
If you want it to be tristate, I will update it.
>
>> + depends on ARM64 && ARCH_ROCKCHIP
>> + depends on OF
>> + select MFD_SYSCON
>> + select PCI_MSI_IRQ_DOMAIN if PCI_MSI
>> + help
>> + Say Y here if you want internal PCI support on Rockchip SoC.
>> + There are 1 internal PCIe port available to support GEN2 with
>
> s/are/is/
will fix.
>
>> + 4 slots.
>> +
>> endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 9c8698e..c52985c 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..661d6e0
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-rockchip.c
>> @@ -0,0 +1,1183 @@
>> +/*
>> + * Rockchip AXI PCIe host controller driver
>> + *
>> + * Copyright (c) 2016 Rockchip, Inc.
>> + *
>> + * Author: Shawn Lin <shawn.lin@...k-chips.com>
>> + * Wenrui Li <wenrui.li@...k-chips.com>
>> + *
>> + * Bits taken from Synopsys Designware Host controller driver and
>> + * ARM PCI Host generic driver.
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>
> Add a blank line here?
will fix it.
>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.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/pci_ids.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +#include <linux/regmap.h>
>> +
>> +#define PCIE_CLIENT_BASE 0x0
>> +#define PCIE_RC_CONFIG_NORMAL_BASE 0x800000
>> +#define PCIE_RC_CONFIG_BASE 0xa00000
>> +#define PCIE_CORE_LINK_CTRL_STATUS 0x8000d0
>> +#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_RC_CONFIG_RID_CCR 0x8
>> +#define PCIR_RC_CONFIG_LCS 0xd0
>
> s/PCIR/PCIE/
>
> ...right?
Ahh... my slippy finger, I will rename it.:)
>
>> +#define PCIE_RC_BAR_CONF 0x300
>> +#define PCIE_CORE_OB_REGION_ADDR1 0x4
>> +#define PCIE_CORE_OB_REGION_DESC0 0x8
>> +#define PCIE_CORE_OB_REGION_DESC1 0xc
>> +#define PCIE_RP_IB_ADDR_TRANS 0x4
>> +#define PCIE_CORE_INT_MASK 0x900210
>> +#define PCIE_CORE_INT_STATUS 0x90020c
>> +
>> +/* Size of one AXI Region (not Region 0) */
>> +#define AXI_REGION_SIZE BIT(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_VENDOR_ID 0x1d87
>> +#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))
>> +
>> +/*
>> + * The higher 16-bit of this register is used for write protection
>> + * only if BIT(x + 16) set to 1 the BIT(x) can be written.
>> + */
>> +#define HIWORD_UPDATE(val, mask, shift) \
>> + ((val) << (shift) | (mask) << ((shift) + 16))
>> +
>> +#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_CORE_LCSR_RETAIN_LINK BIT(5)
>
> s/RETAIN/RETRAIN/
will fix.
>
>> +#define PCIE_CLIENT_CONF_ENABLE 1
>> +#define PCIE_CLIENT_LINK_TRAIN_ENABLE 1
>> +#define PCIE_CLIENT_ARI_ENABLE 1
>> +#define PCIE_CLIENT_CONF_LANE_NUM(x) (x / 2)
>> +#define PCIE_CLIENT_MODE_RC 1
>> +#define PCIE_CLIENT_GEN_SEL_2 1
>> +#define PCIE_CLIENT_GEN_SEL_1 0
>> +#define PCIE_CLIENT_CONF_ENABLE_SHIFT 0
>> +#define PCIE_CLIENT_CONF_ENABLE_MASK 0x1
>
> It's quite confusing (IMO) having the values (e.g.,
> PCIE_CLIENT_CONF_ENABLE) separated from the bitfield SHIFT/MASK macros.
> Can you reorganize so they're listed next to each other?
>
okay.
>> +#define PCIE_CLIENT_LINK_TRAIN_SHIFT 1
>> +#define PCIE_CLIENT_LINK_TRAIN_MASK 0x1
>> +#define PCIE_CLIENT_ARI_ENABLE_SHIFT 3
>> +#define PCIE_CLIENT_ARI_ENABLE_MASK 0x1
>> +#define PCIE_CLIENT_CONF_LANE_NUM_SHIFT 4
>> +#define PCIE_CLIENT_CONF_LANE_NUM_MASK 0x3
>> +#define PCIE_CLIENT_MODE_SHIFT 6
>> +#define PCIE_CLIENT_MODE_MASK 0x1
>> +#define PCIE_CLIENT_GEN_SEL_SHIFT 7
>> +#define PCIE_CLIENT_GEN_SEL_MASK 0x1
>> +#define PCIE_CLIENT_LINK_STATUS_UP 0x3
>> +#define PCIE_CLIENT_LINK_STATUS_SHIFT 20
>> +#define PCIE_CLIENT_LINK_STATUS_MASK 0x3
>> +#define PCIE_CORE_PL_CONF_SPEED_25G 0x0
>> +#define PCIE_CORE_PL_CONF_SPEED_50G 0x1
>> +#define PCIE_CORE_PL_CONF_SPEED_80G 0x2
>
> These speeds are not actually 25/50/80G, they're 2.5/5.0/8.0G. Maybe you
> can either add an underscore (_) where the decimal would be? And
> optionally drop the 0's? Like:
sounds good.
>
> #define PCIE_CORE_PL_CONF_SPEED_2_5G 0x0
> #define PCIE_CORE_PL_CONF_SPEED_5G 0x1
> #define PCIE_CORE_PL_CONF_SPEED_8G 0x2
>
> I'd be ashamed to admit how much time I spent confused as to where you
> were getting 25, 50, and 80 from...
>
>> +#define PCIE_CORE_PL_CONF_SPEED_SHIFT 3
>> +#define PCIE_CORE_PL_CONF_SPEED_MASK 0x3
>> +#define PCIE_CORE_PL_CONF_LANE_SHIFT 1
>> +#define PCIE_CORE_PL_CONF_LANE_MASK 0x3
>> +#define PCIE_CORE_RC_CONF_SCC_SHIFT 16
>> +
>> +/* PCIE_CLIENT_INT_STATUS */
>> +#define PCIE_CLIENT_INT_LEGACY_DONE BIT(15)
>> +#define PCIE_CLIENT_INT_MSG BIT(14)
>> +#define PCIE_CLIENT_INT_HOT_RST BIT(13)
>> +#define PCIE_CLIENT_INT_DPA BIT(12)
>> +#define PCIE_CLIENT_INT_FATAL_ERR BIT(11)
>> +#define PCIE_CLIENT_INT_NFATAL_ERR BIT(10)
>> +#define PCIE_CLIENT_INT_CORR_ERR BIT(9)
>> +#define PCIE_CLIENT_INT_INTD BIT(8)
>> +#define PCIE_CLIENT_INT_INTC BIT(7)
>> +#define PCIE_CLIENT_INT_INTB BIT(6)
>> +#define PCIE_CLIENT_INT_INTA BIT(5)
>> +#define PCIE_CLIENT_INT_LOCAL BIT(4)
>> +#define PCIE_CLIENT_INT_UDMA BIT(3)
>> +#define PCIE_CLIENT_INT_PHY BIT(2)
>> +#define PCIE_CLIENT_INT_HOT_PLUG BIT(1)
>> +#define PCIE_CLIENT_INT_PWR_STCG BIT(0)
>> +#define PCIE_CORE_INT_PRFPE BIT(0)
>> +#define PCIE_CORE_INT_CRFPE BIT(1)
>> +#define PCIE_CORE_INT_RRPE BIT(2)
>> +#define PCIE_CORE_INT_PRFO BIT(3)
>> +#define PCIE_CORE_INT_CRFO BIT(4)
>> +#define PCIE_CORE_INT_RT BIT(5)
>> +#define PCIE_CORE_INT_RTR BIT(6)
>> +#define PCIE_CORE_INT_PE BIT(7)
>> +#define PCIE_CORE_INT_MTR BIT(8)
>> +#define PCIE_CORE_INT_UCR BIT(9)
>> +#define PCIE_CORE_INT_FCE BIT(10)
>> +#define PCIE_CORE_INT_CT BIT(11)
>> +#define PCIE_CORE_INT_UTC BIT(18)
>> +#define PCIE_CORE_INT_MMVC BIT(19)
>> +
>> +#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK GENMASK(8, 5)
>> +#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT 5
>> +
>> +#define PCIE_CORE_INT \
>> + (PCIE_CORE_INT_PRFPE | PCIE_CORE_INT_CRFPE | \
>> + PCIE_CORE_INT_RRPE | PCIE_CORE_INT_CRFO | \
>> + PCIE_CORE_INT_RT | PCIE_CORE_INT_RTR | \
>> + PCIE_CORE_INT_PE | PCIE_CORE_INT_MTR | \
>> + PCIE_CORE_INT_UCR | PCIE_CORE_INT_FCE | \
>> + PCIE_CORE_INT_CT | PCIE_CORE_INT_UTC | \
>> + PCIE_CORE_INT_MMVC)
>> +
>> +#define PCIE_CLIENT_INT_SUBSYSTEM \
>> + (PCIE_CLIENT_INT_PWR_STCG | PCIE_CLIENT_INT_HOT_PLUG | \
>> + PCIE_CLIENT_INT_PHY | PCIE_CLIENT_INT_UDMA | \
>> + PCIE_CLIENT_INT_LOCAL)
>> +
>> +#define PCIE_CLIENT_INT_LEGACY \
>> + (PCIE_CLIENT_INT_INTA | PCIE_CLIENT_INT_INTB | \
>> + PCIE_CLIENT_INT_INTC | PCIE_CLIENT_INT_INTD)
>> +
>> +#define PCIE_CLIENT_INT_CLI \
>> + (PCIE_CLIENT_INT_CORR_ERR | PCIE_CLIENT_INT_NFATAL_ERR | \
>> + PCIE_CLIENT_INT_FATAL_ERR | PCIE_CLIENT_INT_DPA | \
>> + PCIE_CLIENT_INT_HOT_RST | PCIE_CLIENT_INT_MSG | \
>> + PCIE_CLIENT_INT_LEGACY_DONE | PCIE_CLIENT_INT_LEGACY)
>> +
>> +struct rockchip_pcie_port {
>> + void __iomem *reg_base;
>> + void __iomem *apb_base;
>> + struct phy *phy;
>> + 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_pcie_pm;
>> + struct regulator *vpcie3v3; /* 3.3V power supply */
>> + struct regulator *vpcie1v8; /* 1.8V power supply */
>> + struct regulator *vpcie0v9; /* 0.9V power supply */
>> + struct gpio_desc *ep_gpio;
>> + u32 lanes;
>> + u8 root_bus_nr;
>> + struct device *dev;
>> + struct irq_domain *irq_domain;
>> +};
>> +
>> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg);
>> +static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg);
>> +static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc);
>> +
>> +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 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 (!IS_ALIGNED((uintptr_t)addr, size)) {
>> + *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 mask, tmp, offset;
>> +
>> + offset = (where & (~0x3));
>> +
>> + if (size == 4) {
>> + writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
>> + return PCIBIOS_SUCCESSFUL;
>> + }
>> +
>> + mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
>> +
>> + tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset) & mask;
>> + tmp |= val << ((where & 0x3) * 8);
>> + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset);
>> +
>> + 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 (!IS_ALIGNED(busdev, size)) {
>> + *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 (!IS_ALIGNED(busdev, size))
>> + 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;
>> +
>> + gpiod_set_value(port->ep_gpio, 0);
>> +
>> + err = phy_init(port->phy);
>> + if (err < 0)
>> + dev_err(port->dev, "fail to init phy, err %d\n", err);
>
> Are you intentionally continuing, even on error? Seems like you should
> 'return ret'.
Good catch, thanks.
>
>> +
>> + 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,
>> + HIWORD_UPDATE(PCIE_CLIENT_CONF_ENABLE,
>> + PCIE_CLIENT_CONF_ENABLE_MASK,
>> + PCIE_CLIENT_CONF_ENABLE_SHIFT) |
>> + HIWORD_UPDATE(PCIE_CLIENT_CONF_LANE_NUM(port->lanes),
>> + PCIE_CLIENT_CONF_LANE_NUM_MASK,
>> + PCIE_CLIENT_CONF_LANE_NUM_SHIFT) |
>> + HIWORD_UPDATE(PCIE_CLIENT_MODE_RC,
>> + PCIE_CLIENT_MODE_MASK,
>> + PCIE_CLIENT_MODE_SHIFT) |
>> + HIWORD_UPDATE(PCIE_CLIENT_ARI_ENABLE,
>> + PCIE_CLIENT_ARI_ENABLE_MASK,
>> + PCIE_CLIENT_ARI_ENABLE_SHIFT) |
>> + HIWORD_UPDATE(PCIE_CLIENT_GEN_SEL_2,
>> + PCIE_CLIENT_GEN_SEL_MASK,
>> + PCIE_CLIENT_GEN_SEL_SHIFT),
>> + PCIE_CLIENT_BASE);
>> +
>> + err = phy_power_on(port->phy);
>> + if (err)
>> + dev_err(port->dev, "fail to power on phy, err %d\n", err);
>
> Same here.
>
>> +
>> + 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;
>> + }
>> +
>> + /* Enable Gen1 trainning */
>
> s/nn/n/
>
>> + pcie_write(port,
>> + HIWORD_UPDATE(PCIE_CLIENT_LINK_TRAIN_ENABLE,
>> + PCIE_CLIENT_LINK_TRAIN_MASK,
>> + PCIE_CLIENT_LINK_TRAIN_SHIFT),
>> + PCIE_CLIENT_BASE);
>> +
>> + gpiod_set_value(port->ep_gpio, 1);
>> +
>> + /* 500ms timeout value should be enough for gen1/2 taining */
>> + timeout = jiffies + msecs_to_jiffies(500);
>> +
>> + err = -ETIMEDOUT;
>> + while (time_before(jiffies, timeout)) {
>> + status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1);
>> + if (((status >> PCIE_CLIENT_LINK_STATUS_SHIFT) &
>> + PCIE_CLIENT_LINK_STATUS_MASK) ==
>> + PCIE_CLIENT_LINK_STATUS_UP) {
>> + dev_dbg(port->dev, "pcie link training gen1 pass!\n");
>> + err = 0;
>> + break;
>> + }
>> + msleep(20);
>> + }
>
> Technically, the above timeout loop is not quite correct. Possible error
> case: we can fail with a timeout after 500 ms if the training completes
> between the 480-500 ms time window. This can happen because you're
> doing:
>
> (1) read register: if complete, then terminate successfully
> (2) delay
> (3) check for timeout: if time is up, return error
>
> You actually need:
>
> (1) read register: if complete, then terminate successfully
> (2) delay
> (3) check for timeout: if time is up, repeat (1), and then report error
>
> You can examine the logic for readx_poll_timeout() in
> include/linux/iopoll.h to see an example of a proper timeout loop. You
> could even try to use one of the helpers there, except that your
> pcie_read() takes 2 args.
I see, thanks.
>
>> + if (err) {
>> + dev_err(port->dev, "pcie link training gen1 timeout!\n");
>> + return err;
>> + }
>> +
>> + /*
>> + * Enable retrain for gen2. This should be configured only after
>> + * gen1 finished.
>> + */
>> + status = pcie_read(port,
>> + PCIR_RC_CONFIG_LCS + PCIE_RC_CONFIG_BASE);
>> + status |= PCIE_CORE_LCSR_RETAIN_LINK;
>> + pcie_write(port, status,
>> + PCIR_RC_CONFIG_LCS + PCIE_RC_CONFIG_BASE);
>
> I'm not really an expert on this, but how are you actually "retraining
> for gen2"? Is that just the behavior of the core, that it retries at the
> higher rate on the 2nd training attempt? I'm just confused, since you
> set PCIE_CLIENT_GEN_SEL_2 above, so you've already allowed either gen1
> or gen2 negotiation AFAICT, and so seemingly you might already have
> negotiated at gen2.
Not really. I allow the core to enable gen2, but it needs a extra
setting of retrain after finishing gen1. It's not so strange as it
depends on the vendor's design. So I have to say it fits the
designer's expectation.
>
>> + err = -ETIMEDOUT;
>> +
>> + while (time_before(jiffies, timeout)) {
>
> You never reset 'timeout' when starting this loop. So you only have a
> cumulative 500 ms to do both the gen1 and gen2 loops. Is that
> intentional? (I feel like this comment was made on an earlier revision
> of this driver, though I haven't read everything thoroughly.)
yes, I don't have any docs to let me know how long should I wait for
gen1/2 to be finished. Maybe someday it will be augmented to a larger
value if finding a device actually need a longer time. But the only
thing I can say is that it's from my test for many pcie devices
currently.
Do you agree?
>
>> + status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
>> + if (((status >> PCIE_CORE_PL_CONF_SPEED_SHIFT) &
>> + PCIE_CORE_PL_CONF_SPEED_MASK) ==
>> + PCIE_CORE_PL_CONF_SPEED_50G) {
>> + dev_dbg(port->dev, "pcie link training gen2 pass!\n");
>> + err = 0;
>> + break;
>> + }
>> + msleep(20);
>> + }
>
> Similar problem with your timeout loop, as mentioned above. Although I
> confused about what you do in the error case here...
>
>> + if (err)
>> + dev_dbg(port->dev, "pcie link training gen2 timeout, force to gen1!\n");
>
> ... how are you forcing gen1? I don't see any code that indicates this.
> Should you at least be checking that there aren't some kind of training
> errors, and that we settled back on gen1/2.5G?
yes, when failing gen2, my pcie controller will fallback to gen1
automatically.
if we pass the gen1 then fail to train gen2, a dbg msg here is enough
here to let we know that we should check the HW signal. Of course we
should make sure that this device supports gen2.
>
>> +
>> + /* Check the final link with from negotiated lane counter from MGMT */
>> + status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
>> + status = 0x1 << ((status >> PCIE_CORE_PL_CONF_LANE_SHIFT) &
>> + PCIE_CORE_PL_CONF_LANE_MASK);
>> + dev_dbg(port->dev, "current link width is x%d\n", status);
>> +
>> + pcie_write(port, ROCKCHIP_VENDOR_ID, PCIE_RC_CONFIG_BASE);
>> + pcie_write(port, PCI_CLASS_BRIDGE_PCI << PCIE_CORE_RC_CONF_SCC_SHIFT,
>> + PCIE_RC_CONFIG_BASE + PCIE_RC_CONFIG_RID_CCR);
>> + pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + PCIE_RC_BAR_CONF);
>> +
>> + 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 + PCIE_CORE_OB_REGION_ADDR1);
>> + pcie_write(port, 0x0080000a,
>> + PCIE_CORE_AXI_CONF_BASE + PCIE_CORE_OB_REGION_DESC0);
>> + pcie_write(port, 0x0,
>> + PCIE_CORE_AXI_CONF_BASE + PCIE_CORE_OB_REGION_DESC1);
>> +
>> + 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 platform_device *pdev = to_platform_device(dev);
>> + struct device_node *node = dev->of_node;
>> + struct resource *regs;
>> + int irq;
>> + int err = -ENODEV;
>> +
>> + regs = platform_get_resource_byname(pdev,
>> + IORESOURCE_MEM,
>> + "axi-base");
>> + if (!regs) {
>> + dev_err(dev, "missing axi-base property\n");
>> + return err;
>> + }
>> +
>> + port->reg_base = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(port->reg_base))
>> + return PTR_ERR(port->reg_base);
>> +
>> + regs = platform_get_resource_byname(pdev,
>> + IORESOURCE_MEM,
>> + "apb-base");
>> + if (!regs) {
>> + dev_err(dev, "missing apb-base 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->phy = devm_phy_get(dev, "pcie-phy");
>> + if (IS_ERR_OR_NULL(port->phy)) {
>> + if (PTR_ERR_OR_ZERO(port->phy) != -EPROBE_DEFER)
>> + dev_err(dev, "Missing phy\n");
>> + return PTR_ERR(port->phy);
>
> You're handling the port->phy == NULL case kind of inconsistently. On
> one hand, I don't think phy_get() can return NULL, so you might not need
> to handle it (i.e., the IS_ERR_OR_NULL() -> IS_ERR()) -- and the phy API
> can just ignore NULL PHY. But on the other hand, if you do want to
> handle NULL here, you probably shouldn't return PTR_ERR(NULL) here
> (i.e., 0, which means success).
>
Brilliant.. I will fix it.
> My recommendation would be:
>
> s/IS_ERR_OR_NULL/IS_ERR/
> s/PTR_ERR_OR_ZERO/PTR_ERR/
>
> for the above 4 lines.
>
>> + }
>> +
>> + 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_warn(dev, "invalid num-lanes, default use one lane\n");
>> + port->lanes = 1;
>> + }
>> +
>> + port->core_rst = devm_reset_control_get(dev, "core");
>> + 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);
>> + return PTR_ERR(port->core_rst);
>> + }
>> +
>> + port->mgmt_rst = devm_reset_control_get(dev, "mgmt");
>> + 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);
>> + return PTR_ERR(port->mgmt_rst);
>> + }
>> +
>> + port->mgmt_sticky_rst = devm_reset_control_get(dev, "mgmt-sticky");
>> + 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);
>> + return PTR_ERR(port->mgmt_sticky_rst);
>> + }
>> +
>> + port->pipe_rst = devm_reset_control_get(dev, "pipe");
>> + 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);
>> + return PTR_ERR(port->pipe_rst);
>> + }
>> +
>> + port->ep_gpio = devm_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");
>> + if (IS_ERR(port->aclk_pcie)) {
>> + dev_err(dev, "aclk clock not found.\n");
>> + return PTR_ERR(port->aclk_pcie);
>> + }
>> +
>> + port->aclk_perf_pcie = devm_clk_get(dev, "aclk-perf");
>> + if (IS_ERR(port->aclk_perf_pcie)) {
>> + dev_err(dev, "aclk_perf clock not found.\n");
>> + return PTR_ERR(port->aclk_perf_pcie);
>> + }
>> +
>> + port->hclk_pcie = devm_clk_get(dev, "hclk");
>> + if (IS_ERR(port->hclk_pcie)) {
>> + dev_err(dev, "hclk clock not found.\n");
>> + return PTR_ERR(port->hclk_pcie);
>> + }
>> +
>> + port->clk_pcie_pm = devm_clk_get(dev, "pm");
>> + if (IS_ERR(port->clk_pcie_pm)) {
>> + dev_err(dev, "pm clock not found.\n");
>> + return PTR_ERR(port->clk_pcie_pm);
>> + }
>> +
>> + irq = platform_get_irq_byname(pdev, "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;
>> + }
>> +
>> + irq = platform_get_irq_byname(pdev, "legacy");
>> + if (irq < 0) {
>> + dev_err(dev, "missing pcie_legacy IRQ resource\n");
>> + return -EINVAL;
>> + }
>> +
>> + irq_set_chained_handler_and_data(irq,
>> + rockchip_pcie_legacy_int_handler,
>> + port);
>> +
>> + irq = platform_get_irq_byname(pdev, "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->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
>> + if (IS_ERR(port->vpcie3v3)) {
>> + if (PTR_ERR(port->vpcie3v3) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + dev_info(dev, "No vpcie3v3 regulator found.\n");
>> + }
>> +
>> + port->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>> + if (IS_ERR(port->vpcie1v8)) {
>> + if (PTR_ERR(port->vpcie1v8) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + dev_info(dev, "No vpcie1v8 regulator found.\n");
>> + }
>> +
>> + port->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>> + if (IS_ERR(port->vpcie0v9)) {
>> + if (PTR_ERR(port->vpcie0v9) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + dev_info(dev, "No vpcie0v9 regulator found.\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int rockchip_pcie_set_vpcie(struct rockchip_pcie_port *port)
>> +{
>> + int err;
>> +
>> + if (!IS_ERR(port->vpcie3v3)) {
>> + err = regulator_enable(port->vpcie3v3);
>> + if (err) {
>> + dev_err(port->dev, "Fail to enable vpcie3v3 regulator.\n");
>> + goto err_out;
>> + }
>> + }
>> +
>> + if (!IS_ERR(port->vpcie1v8)) {
>> + err = regulator_enable(port->vpcie1v8);
>> + if (err) {
>> + dev_err(port->dev, "Fail to enable vpcie1v8 regulator.\n");
>> + goto err_disable_3v3;
>> + }
>> + }
>> +
>> + if (!IS_ERR(port->vpcie0v9)) {
>> + err = regulator_enable(port->vpcie0v9);
>> + if (err) {
>> + dev_err(port->dev, "Fail to enable vpcie0v9 regulator.\n");
>> + goto err_disable_1v8;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err_disable_1v8:
>> + if (!IS_ERR(port->vpcie1v8))
>> + regulator_disable(port->vpcie1v8);
>> +err_disable_3v3:
>> + if (!IS_ERR(port->vpcie3v3))
>> + regulator_disable(port->vpcie3v3);
>> +err_out:
>> + return err;
>> +}
>> +
>> +static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *port)
>> +{
>> + pcie_write(port, (PCIE_CLIENT_INT_CLI << 16) &
>> + (~PCIE_CLIENT_INT_CLI), PCIE_CLIENT_INT_MASK);
>> + pcie_write(port, PCIE_CORE_INT, 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;
>> +
>> + pp->irq_domain = irq_domain_add_linear(dev->of_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 & PCIE_CLIENT_INT_LOCAL) {
>> + dev_dbg(pp->dev, "local interrupt recived\n");
>> + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
>> + if (sub_reg & PCIE_CORE_INT_PRFPE)
>> + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_CRFPE)
>> + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_RRPE)
>> + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_PRFO)
>> + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_CRFO)
>> + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_RT)
>> + dev_dbg(pp->dev, "Replay timer timed out\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_RTR)
>> + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_PE)
>> + dev_dbg(pp->dev, "Phy error detected on receive side\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_MTR)
>> + dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_UCR)
>> + dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_FCE)
>> + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_CT)
>> + dev_dbg(pp->dev, "A request timed out waiting for completion\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_UTC)
>> + dev_dbg(pp->dev, "Unmapped TC error\n");
>> +
>> + if (sub_reg & PCIE_CORE_INT_MMVC)
>> + dev_dbg(pp->dev, "MSI mask register changes\n");
>> +
>> + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
>> + }
>> +
>> + pcie_write(pp, PCIE_CLIENT_INT_SUBSYSTEM | PCIE_CLIENT_INT_LOCAL,
>> + 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 & PCIE_CLIENT_INT_LEGACY_DONE)
>> + dev_dbg(pp->dev, "legacy done interrupt recived\n");
>> +
>> + if (reg & PCIE_CLIENT_INT_MSG)
>> + dev_dbg(pp->dev, "message done interrupt recived\n");
>> +
>> + if (reg & PCIE_CLIENT_INT_HOT_RST)
>> + dev_dbg(pp->dev, "hot reset interrupt recived\n");
>> +
>> + if (reg & PCIE_CLIENT_INT_DPA)
>> + dev_dbg(pp->dev, "dpa interrupt recived\n");
>> +
>> + if (reg & PCIE_CLIENT_INT_FATAL_ERR)
>> + dev_dbg(pp->dev, "fatal error interrupt recived\n");
>> +
>> + if (reg & PCIE_CLIENT_INT_NFATAL_ERR)
>> + dev_dbg(pp->dev, "no fatal error interrupt recived\n");
>> +
>> + if (reg & PCIE_CLIENT_INT_CORR_ERR)
>> + dev_dbg(pp->dev, "correctable error interrupt recived\n");
>> +
>> + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc)
>> +{
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct rockchip_pcie_port *port;
>> + u32 reg;
>> +
>> + chained_irq_enter(chip, desc);
>> + port = irq_desc_get_handler_data(desc);
>> +
>> + reg = pcie_read(port, PCIE_CLIENT_INT_STATUS);
>> + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
>> + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
>> + if (reg)
>> + generic_handle_irq(irq_find_mapping(port->irq_domain,
>> + ffs(reg)));
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +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 + PCIE_CORE_OB_REGION_ADDR1);
>> + writel(ob_desc_0, aw_base + PCIE_CORE_OB_REGION_DESC0);
>> + writel(ob_desc_1, aw_base + PCIE_CORE_OB_REGION_DESC1);
>> +
>> + 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 + PCIE_RP_IB_ADDR_TRANS);
>> +
>> + 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;
>> + resource_size_t io_base;
>> + struct resource *busn;
>> + struct resource *mem;
>> + struct resource *io;
>> + phys_addr_t io_bus_addr;
>> + u32 io_size;
>> + phys_addr_t mem_bus_addr;
>> + u32 mem_size;
>> + int reg_no;
>> + int err = 0;
>> + int offset = 0;
>> +
>> + LIST_HEAD(res);
>> +
>> + if (!dev->of_node)
>> + return -ENODEV;
>> +
>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + return -ENOMEM;
>> +
>> + port->dev = dev;
>> +
>> + err = rockchip_pcie_parse_dt(port);
>> + if (err)
>> + return err;
>> +
>> + 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_pcie_pm);
>> + if (err) {
>> + dev_err(dev, "Unable to enable hclk_pcie clock.\n");
>> + goto err_pcie_pm;
>> + }
>> +
>> + err = rockchip_pcie_set_vpcie(port);
>> + if (err) {
>> + dev_err(port->dev, "Fail to set vpcie regulator.\n");
>> + goto err_set_vpcie;
>> + }
>> +
>> + err = rockchip_pcie_init_port(port);
>> + if (err)
>> + goto err_vpcie;
>> +
>> + platform_set_drvdata(pdev, port);
>> +
>> + rockchip_pcie_enable_interrupts(port);
>> +
>> + err = rockchip_pcie_init_irq_domain(port);
>> + if (err < 0)
>> + goto err_vpcie;
>> +
>> + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
>> + &res, &io_base);
>> + if (err)
>> + goto err_vpcie;
>> +
>> + /* Get the I/O and memory ranges from DT */
>> + resource_list_for_each_entry(win, &res) {
>> + switch (resource_type(win->res)) {
>> + case IORESOURCE_IO:
>> + io = win->res;
>> + io->name = "I/O";
>> + io_size = resource_size(io);
>> + io_bus_addr = io->start - win->offset;
>> + err = pci_remap_iospace(io, io_base);
>> + if (err) {
>> + dev_warn(port->dev, "error %d: failed to map resource %pR\n",
>> + err, io);
>> + continue;
>> + }
>> + break;
>> + case IORESOURCE_MEM:
>> + mem = win->res;
>> + mem->name = "MEM";
>> + mem_size = resource_size(mem);
>> + mem_bus_addr = mem->start - win->offset;
>> + break;
>> + case 0:
>
> Why case 0? That doesn't seem right.
I will remove it as it actually means configure space. But my controller
has the fixed cfg space inside the core, so we never need to get it
from parsing DT.
>
>> + break;
>> + case IORESOURCE_BUS:
>> + busn = win->res;
>> + break;
>> + default:
>> + continue;
>> + }
>> + }
>> +
>> + for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) {
>
> kbuildbot already complained about potential unused warnings here and
> elsewhere. One problem that this highlights:
>
> what happens if the expected resources aren't included in your device
> tree? Seems like you'll just keep going, with uninitialized data.
> Probably want some kind of checks to make sure we found the necessary
> resources.
yes, I saw the report from kbuildbot and I will fix it.
>
>> + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
>> + AXI_WRAPPER_MEM_WRITE,
>> + 20 - 1,
>> + mem_bus_addr +
>> + (reg_no << 20),
>> + 0);
>> + if (err) {
>> + dev_err(dev, "Program RC mem outbound atu failed\n");
>> + goto err_vpcie;
>> + }
>> + }
>> +
>> + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0);
>> + if (err) {
>> + dev_err(dev, "Program RC mem inbound atu failed\n");
>> + goto err_vpcie;
>> + }
>> +
>> + offset = mem_size >> 20;
>> +
>> + for (reg_no = 0; reg_no < (io_size >> 20); reg_no++) {
>> + err = rockchip_pcie_prog_ob_atu(port,
>> + reg_no + 1 + offset,
>> + AXI_WRAPPER_IO_WRITE,
>> + 20 - 1,
>> + io_bus_addr +
>> + (reg_no << 20),
>> + 0);
>> + if (err) {
>> + dev_err(dev, "Program RC io outbound atu failed\n");
>> + goto err_vpcie;
>> + }
>> + }
>> +
>> + port->root_bus_nr = busn->start;
>> +
>> + bus = pci_scan_root_bus(&pdev->dev, 0,
>> + &rockchip_pcie_ops, port, &res);
>> +
>> + if (!bus) {
>> + err = -ENOMEM;
>> + goto err_vpcie;
>> + }
>> +
>> + pci_bus_size_bridges(bus);
>> + pci_bus_assign_resources(bus);
>> + list_for_each_entry(child, &bus->children, node)
>> + pcie_bus_configure_settings(child);
>
> Bad indentation: the above line is part of a loop, so it should be
> indented one more.
>
My bad, will fix it.
>> +
>> + pci_bus_add_devices(bus);
>> +
>> + return err;
>> +
>> +err_vpcie:
>> + if (!IS_ERR(port->vpcie3v3))
>> + regulator_disable(port->vpcie3v3);
>> + if (!IS_ERR(port->vpcie1v8))
>> + regulator_disable(port->vpcie1v8);
>> + if (!IS_ERR(port->vpcie0v9))
>> + regulator_disable(port->vpcie0v9);
>> +err_set_vpcie:
>> + clk_disable_unprepare(port->clk_pcie_pm);
>> +err_pcie_pm:
>> + 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 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,
>> + },
>> + .probe = rockchip_pcie_probe,
>> +
>> +};
>> +module_platform_driver(rockchip_pcie_driver);
>> +
>> +MODULE_AUTHOR("Rockchip Inc");
>> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.3.7
>
> Brian
>
>
>
--
Best Regards
Shawn Lin
Powered by blists - more mailing lists