[2/2] pci: Add Rockchip dwc based PCIe controller driver

Add Rockchip dwc based PCIe controller driver for rk356x platform.
Driver support Gen3 by operating as a Root complex.

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

---

 drivers/pci/Kconfig            |   9 +
 drivers/pci/Makefile           |   1 +
 drivers/pci/pcie_dw_rockchip.c | 755 +++++++++++++++++++++++++++++++++
 3 files changed, 765 insertions(+)
 create mode 100644 drivers/pci/pcie_dw_rockchip.c

Hi Shawn,

On Thu, 14 Jan 2021 at 01:15, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Add Rockchip dwc based PCIe controller driver for rk356x platform.
> Driver support Gen3 by operating as a Root complex.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
>  drivers/pci/Kconfig            |   9 +
>  drivers/pci/Makefile           |   1 +
>  drivers/pci/pcie_dw_rockchip.c | 755 +++++++++++++++++++++++++++++++++
>  3 files changed, 765 insertions(+)
>  create mode 100644 drivers/pci/pcie_dw_rockchip.c
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 65498bce1d..b1de38f766 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -281,6 +281,15 @@ config PCIE_ROCKCHIP
>           Say Y here if you want to enable PCIe controller support on
>           Rockchip SoCs.
>
> +config PCIE_DW_ROCKCHIP
> +       bool "Rockchip DesignWare based PCIe controller"
> +       depends on ARCH_ROCKCHIP
> +       select DM_PCI
> +       select PHY_ROCKCHIP_SNPS_PCIE3
> +       help
> +         Say Y here if you want to enable DW PCIe controller support on
> +         Rockchip SoCs.
> +
>  config PCI_BRCMSTB
>         bool "Broadcom STB PCIe controller"
>         depends on DM_PCI
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 8b4d49a590..5ed94bc95c 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -48,5 +48,6 @@ obj-$(CONFIG_PCIE_INTEL_FPGA) += pcie_intel_fpga.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pcie_dw_ti.o
>  obj-$(CONFIG_PCIE_MEDIATEK) += pcie_mediatek.o
>  obj-$(CONFIG_PCIE_ROCKCHIP) += pcie_rockchip.o
> +obj-$(CONFIG_PCIE_DW_ROCKCHIP) += pcie_dw_rockchip.o
>  obj-$(CONFIG_PCI_BRCMSTB) += pcie_brcmstb.o
>  obj-$(CONFIG_PCI_OCTEONTX) += pci_octeontx.o
> diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c
> new file mode 100644
> index 0000000000..aa2cc15e15
> --- /dev/null
> +++ b/drivers/pci/pcie_dw_rockchip.c
> @@ -0,0 +1,755 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Rockchip DesignWare based PCIe host controller driver
> + *
> + * Copyright (c) 2021 Rockchip, Inc.
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <generic-phy.h>
> +#include <pci.h>
> +#include <power-domain.h>
> +#include <power/regulator.h>

that one should go at the end

> +#include <reset.h>
> +#include <syscon.h>
> +#include <asm/io.h>
> +#include <asm-generic/gpio.h>
> +#include <asm/arch-rockchip/clock.h>
> +#include <dm/device_compat.h>
> +#include <linux/iopoll.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct rk_pcie {
> +       struct udevice  *dev;
> +       struct udevice  *vpcie3v3;
> +       void            *dbi_base;
> +       void            *apb_base;
> +       void            *cfg_base;
> +       fdt_size_t      cfg_size;
> +       struct phy      phy;
> +       struct clk_bulk clks;
> +       int             first_busno;
> +       struct reset_ctl_bulk   rsts;
> +       struct gpio_desc        rst_gpio;
> +       struct pci_region       io;
> +       struct pci_region       mem;

these members need comments

> +};
> +
> +enum {
> +       PCIBIOS_SUCCESSFUL = 0x0000,
> +       PCIBIOS_UNSUPPORTED = -ENODEV,

Can you use a different error? This is used by drive model to know
when there is no device, but generally there is a device in a driver!

> +       PCIBIOS_NODEV = -ENODEV,
> +};
> +
> +#define msleep(a)              udelay((a) * 1000)

That is in linux/delay.h I think

> +
> +/* Parameters for the waiting for iATU enabled routine */
> +#define PCIE_CLIENT_GENERAL_DEBUG      0x104
> +#define PCIE_CLIENT_HOT_RESET_CTRL     0x180
> +#define PCIE_LTSSM_ENABLE_ENHANCE      BIT(4)
> +#define PCIE_CLIENT_LTSSM_STATUS       0x300
> +#define SMLH_LINKUP                    BIT(16)
> +#define RDLH_LINKUP                    BIT(17)
> +#define PCIE_CLIENT_DBG_FIFO_MODE_CON  0x310
> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320
> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324
> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328
> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c
> +#define PCIE_CLIENT_DBG_FIFO_STATUS    0x350
> +#define PCIE_CLIENT_DBG_TRANSITION_DATA        0xffff0000
> +#define PCIE_CLIENT_DBF_EN             0xffff0003
> +#define RK_PCIE_DBG                    0

Can you rely on DEBUG in this file instead?

> +
> +/* PCI DBICS registers */
> +#define PCIE_LINK_STATUS_REG           0x80
> +#define PCIE_LINK_STATUS_SPEED_OFF     16
> +#define PCIE_LINK_STATUS_SPEED_MASK    (0xf << PCIE_LINK_STATUS_SPEED_OFF)
> +#define PCIE_LINK_STATUS_WIDTH_OFF     20
> +#define PCIE_LINK_STATUS_WIDTH_MASK    (0xf << PCIE_LINK_STATUS_WIDTH_OFF)
> +
> +#define PCIE_LINK_CAPABILITY           0x7c
> +#define PCIE_LINK_CTL_2                        0xa0
> +#define TARGET_LINK_SPEED_MASK         0xf
> +#define LINK_SPEED_GEN_1               0x1
> +#define LINK_SPEED_GEN_2               0x2
> +#define LINK_SPEED_GEN_3               0x3
> +
> +#define PCIE_MISC_CONTROL_1_OFF                0x8bc
> +#define PCIE_DBI_RO_WR_EN              BIT(0)
> +
> +#define PCIE_LINK_WIDTH_SPEED_CONTROL  0x80c
> +#define PORT_LOGIC_SPEED_CHANGE                BIT(17)
> +
> +/*
> + * iATU Unroll-specific register definitions
> + * From 4.80 core version the address translation will be made by unroll.
> + * The registers are offset from atu_base
> + */
> +#define PCIE_ATU_UNR_REGION_CTRL1      0x00
> +#define PCIE_ATU_UNR_REGION_CTRL2      0x04
> +#define PCIE_ATU_UNR_LOWER_BASE                0x08
> +#define PCIE_ATU_UNR_UPPER_BASE                0x0c
> +#define PCIE_ATU_UNR_LIMIT             0x10
> +#define PCIE_ATU_UNR_LOWER_TARGET      0x14
> +#define PCIE_ATU_UNR_UPPER_TARGET      0x18
> +
> +#define PCIE_ATU_REGION_INDEX1         (0x1 << 0)
> +#define PCIE_ATU_REGION_INDEX0         (0x0 << 0)
> +#define PCIE_ATU_TYPE_MEM              (0x0 << 0)
> +#define PCIE_ATU_TYPE_IO               (0x2 << 0)
> +#define PCIE_ATU_TYPE_CFG0             (0x4 << 0)
> +#define PCIE_ATU_TYPE_CFG1             (0x5 << 0)
> +#define PCIE_ATU_ENABLE                        (0x1 << 31)
> +#define PCIE_ATU_BAR_MODE_ENABLE       (0x1 << 30)
> +#define PCIE_ATU_BUS(x)                        (((x) & 0xff) << 24)
> +#define PCIE_ATU_DEV(x)                        (((x) & 0x1f) << 19)
> +#define PCIE_ATU_FUNC(x)               (((x) & 0x7) << 16)
> +
> +/* Register address builder */
> +#define PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(region)        \
> +       ((0x3 << 20) | ((region) << 9))
> +#define PCIE_GET_ATU_INB_UNR_REG_OFFSET(region) \
> +       ((0x3 << 20) | ((region) << 9) | (0x1 << 8))

Perhaps these should be inline functions? The second one doesn't seem
to ue used, so could be dropped.


> +
> +/* Parameters for the waiting for iATU enabled routine */
> +#define LINK_WAIT_MAX_IATU_RETRIES     5
> +#define LINK_WAIT_IATU                 10000

If this is milliseconds, how about a _MS suffix?

> +
> +static int rk_pcie_read(void __iomem *addr, int size, u32 *val)
> +{
> +       if ((uintptr_t)addr & (size - 1)) {
> +               *val = 0;
> +               return PCIBIOS_UNSUPPORTED;
> +       }
> +
> +       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_NODEV;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int rk_pcie_write(void __iomem *addr, int size, u32 val)
> +{
> +       if ((uintptr_t)addr & (size - 1))
> +               return PCIBIOS_UNSUPPORTED;
> +
> +       if (size == 4)
> +               writel(val, addr);
> +       else if (size == 2)
> +               writew(val, addr);
> +       else if (size == 1)
> +               writeb(val, addr);
> +       else
> +               return PCIBIOS_NODEV;
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static u32 __rk_pcie_read_apb(struct rk_pcie *rk_pcie, void __iomem *base,
> +                             u32 reg, size_t size)

function comment. Many of the functions below need some sort of
comment as to what they do.

> +{
> +       int ret;
> +       u32 val;
> +
> +       ret = rk_pcie_read(base + reg, size, &val);
> +       if (ret)
> +               dev_err(rk_pcie->dev, "Read APB address failed\n");
> +
> +       return val;
> +}
> +
> +static void __rk_pcie_write_apb(struct rk_pcie *rk_pcie, void __iomem *base,
> +                               u32 reg, size_t size, u32 val)
> +{
> +       int ret;
> +
> +       ret = rk_pcie_write(base + reg, size, val);
> +       if (ret)
> +               dev_err(rk_pcie->dev, "Write APB address failed\n");
> +}
> +
> +static inline u32 rk_pcie_readl_apb(struct rk_pcie *rk_pcie, u32 reg)
> +{
> +       return __rk_pcie_read_apb(rk_pcie, rk_pcie->apb_base, reg, 0x4);
> +}
> +
> +static inline void rk_pcie_writel_apb(struct rk_pcie *rk_pcie, u32 reg,
> +                                     u32 val)
> +{
> +       __rk_pcie_write_apb(rk_pcie, rk_pcie->apb_base, reg, 0x4, val);
> +}
> +
> +static int rk_pcie_get_link_speed(struct rk_pcie *rk_pcie)
> +{
> +       return (readl(rk_pcie->dbi_base + PCIE_LINK_STATUS_REG) &
> +               PCIE_LINK_STATUS_SPEED_MASK) >> PCIE_LINK_STATUS_SPEED_OFF;
> +}
> +
> +static int rk_pcie_get_link_width(struct rk_pcie *rk_pcie)
> +{
> +       return (readl(rk_pcie->dbi_base + PCIE_LINK_STATUS_REG) &
> +               PCIE_LINK_STATUS_WIDTH_MASK) >> PCIE_LINK_STATUS_WIDTH_OFF;
> +}
> +
> +static void rk_pcie_writel_ob_unroll(struct rk_pcie *rk_pcie, u32 index,
> +                                    u32 reg, u32 val)
> +{
> +       u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> +       void __iomem *base = rk_pcie->dbi_base;
> +
> +       writel(val, base + offset + reg);
> +}
> +
> +static u32 rk_pcie_readl_ob_unroll(struct rk_pcie *rk_pcie, u32 index, u32 reg)
> +{
> +       u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> +       void __iomem *base = rk_pcie->dbi_base;
> +
> +       return readl(base + offset + reg);
> +}
> +
> +static inline void rk_pcie_dbi_write_enable(struct rk_pcie *rk_pcie, bool en)
> +{
> +       u32 val;
> +
> +       val = readl(rk_pcie->dbi_base + PCIE_MISC_CONTROL_1_OFF);
> +
> +       if (en)
> +               val |= PCIE_DBI_RO_WR_EN;
> +       else
> +               val &= ~PCIE_DBI_RO_WR_EN;
> +       writel(val, rk_pcie->dbi_base + PCIE_MISC_CONTROL_1_OFF);
> +}
> +
> +static void rk_pcie_setup_host(struct rk_pcie *rk_pcie)
> +{
> +       u32 val;
> +
> +       rk_pcie_dbi_write_enable(rk_pcie, true);
> +
> +       /* setup RC BARs */
> +       writel(PCI_BASE_ADDRESS_MEM_TYPE_64,
> +              rk_pcie->dbi_base + PCI_BASE_ADDRESS_0);
> +       writel(0x0, rk_pcie->dbi_base + PCI_BASE_ADDRESS_1);
> +
> +       /* setup interrupt pins */
> +       val = readl(rk_pcie->dbi_base + PCI_INTERRUPT_LINE);
> +       val &= 0xffff00ff;
> +       val |= 0x00000100;
> +       writel(val, rk_pcie->dbi_base + PCI_INTERRUPT_LINE);
> +
> +       /* setup bus numbers */
> +       val = readl(rk_pcie->dbi_base + PCI_PRIMARY_BUS);
> +       val &= 0xff000000;
> +       val |= 0x00ff0100;
> +       writel(val, rk_pcie->dbi_base + PCI_PRIMARY_BUS);

Please use clrsetbits_le32() as it does this sort of thing more easily

> +
> +       val = readl(rk_pcie->dbi_base + PCI_PRIMARY_BUS);
> +
> +       /* setup command register */
> +       val = readl(rk_pcie->dbi_base + PCI_COMMAND);
> +       val &= 0xffff0000;
> +       val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
> +               PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
> +       writel(val, rk_pcie->dbi_base + PCI_COMMAND);
> +
> +       /* program correct class for RC */
> +       writew(PCI_CLASS_BRIDGE_PCI, rk_pcie->dbi_base + PCI_CLASS_DEVICE);
> +       /* Better disable write permission right after the update */
> +
> +       val = readl(rk_pcie->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);
> +       val |= PORT_LOGIC_SPEED_CHANGE;
> +       writel(val, rk_pcie->dbi_base + PCIE_LINK_WIDTH_SPEED_CONTROL);

setbits_le32()

Please fix globally

> +
> +       rk_pcie_dbi_write_enable(rk_pcie, false);
> +}
> +
> +static void rk_pcie_configure(struct rk_pcie *pci, u32 cap_speed)

function comment

> +{
> +       u32 val;
> +
> +       rk_pcie_dbi_write_enable(pci, true);
> +
> +       val = readl(pci->dbi_base + PCIE_LINK_CAPABILITY);
> +       val &= ~TARGET_LINK_SPEED_MASK;
> +       val |= cap_speed;
> +       writel(val, pci->dbi_base + PCIE_LINK_CAPABILITY);
> +
> +       val = readl(pci->dbi_base + PCIE_LINK_CTL_2);
> +       val &= ~TARGET_LINK_SPEED_MASK;
> +       val |= cap_speed;
> +       writel(val, pci->dbi_base + PCIE_LINK_CTL_2);
> +
> +       rk_pcie_dbi_write_enable(pci, false);
> +}
> +
> +static void rk_pcie_prog_outbound_atu_unroll(struct rk_pcie *pci, int index,
> +                                            int type, u64 cpu_addr,
> +                                            u64 pci_addr, u32 size)
> +{

function comment. Also this can fail so should return an error and be
checked by callers.

> +       u32 retries, val;
> +
> +       dev_dbg(pci->dev, "ATU programmed with: index: %d, type: %d, cpu addr: %8llx, pci addr: %8llx, size: %8x\n",
> +               index, type, cpu_addr, pci_addr, size);
> +
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_BASE,
> +                                lower_32_bits(cpu_addr));
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_BASE,
> +                                upper_32_bits(cpu_addr));
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LIMIT,
> +                                lower_32_bits(cpu_addr + size - 1));
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_LOWER_TARGET,
> +                                lower_32_bits(pci_addr));
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_UPPER_TARGET,
> +                                upper_32_bits(pci_addr));
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL1,
> +                                type);
> +       rk_pcie_writel_ob_unroll(pci, index, PCIE_ATU_UNR_REGION_CTRL2,
> +                                PCIE_ATU_ENABLE);
> +
> +       /*
> +        * Make sure ATU enable takes effect before any subsequent config
> +        * and I/O accesses.
> +        */
> +       for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> +               val = rk_pcie_readl_ob_unroll(pci, index,
> +                                             PCIE_ATU_UNR_REGION_CTRL2);
> +               if (val & PCIE_ATU_ENABLE)
> +                       return;
> +
> +               udelay(LINK_WAIT_IATU);
> +       }
> +       dev_err(pci->dev, "outbound iATU is not being enabled\n");

return -EIO or something

> +}
> +
> +static int rk_pcie_addr_valid(pci_dev_t d, int first_busno)
> +{
> +       if ((PCI_BUS(d) == first_busno) && (PCI_DEV(d) > 0))
> +               return 0;
> +       if ((PCI_BUS(d) == first_busno + 1) && (PCI_DEV(d) > 0))
> +               return 0;
> +
> +       return 1;
> +}
> +
> +static uintptr_t set_cfg_address(struct rk_pcie *pcie,
> +                                pci_dev_t d, uint where)
> +{
> +       int bus = PCI_BUS(d) - pcie->first_busno;

This is strange because it is a relative bus number, not the actual
PCI bus number. Would it be better as rel_bus, perhaps? Or am I
misunderstanding this? It could use a comment.


> +       uintptr_t va_address;
> +       u32 atu_type;
> +
> +       /* Use dbi_base for own configuration read and write */
> +       if (!bus) {
> +               va_address = (uintptr_t)pcie->dbi_base;
> +               goto out;
> +       }
> +
> +       if (bus == 1)
> +               /*
> +                * For local bus whose primary bus number is root bridge,
> +                * change TLP Type field to 4.
> +                */
> +               atu_type = PCIE_ATU_TYPE_CFG0;
> +       else
> +               /* Otherwise, change TLP Type field to 5. */
> +               atu_type = PCIE_ATU_TYPE_CFG1;
> +
> +       /*
> +        * Not accessing root port configuration space?
> +        * Region #0 is used for Outbound CFG space access.
> +        * Direction = Outbound
> +        * Region Index = 0
> +        */
> +       d = PCI_MASK_BUS(d);
> +       d = PCI_ADD_BUS(bus, d);
> +       rk_pcie_prog_outbound_atu_unroll(pcie, PCIE_ATU_REGION_INDEX1,
> +                                        atu_type, (u64)pcie->cfg_base,
> +                                        d << 8, pcie->cfg_size);
> +
> +       va_address = (uintptr_t)pcie->cfg_base;
> +
> +out:
> +       va_address += where & ~0x3;
> +
> +       return va_address;
> +}
> +
> +static int rockchip_pcie_rd_conf(const struct udevice *bus, pci_dev_t bdf,
> +                                uint offset, ulong *valuep,
> +                                enum pci_size_t size)
> +{
> +       struct rk_pcie *pcie = dev_get_priv(bus);
> +       uintptr_t va_address;
> +       ulong value;
> +
> +       debug("PCIE CFG read: bdf=%2x:%2x:%2x\n",
> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> +
> +       if (!rk_pcie_addr_valid(bdf, pcie->first_busno)) {
> +               debug("- out of range\n");
> +               *valuep = pci_get_ff(size);
> +               return 0;
> +       }
> +
> +       va_address = set_cfg_address(pcie, bdf, offset);
> +
> +       value = readl(va_address);
> +
> +       debug("(addr,val)=(0x%04x, 0x%08lx)\n", offset, value);
> +       *valuep = pci_conv_32_to_size(value, offset, size);
> +
> +       rk_pcie_prog_outbound_atu_unroll(pcie, PCIE_ATU_REGION_INDEX1,
> +                                        PCIE_ATU_TYPE_IO, pcie->io.phys_start,
> +                                        pcie->io.bus_start, pcie->io.size);
> +
> +       return 0;
> +}
> +
> +static int rockchip_pcie_wr_conf(struct udevice *bus, pci_dev_t bdf,
> +                                uint offset, ulong value,
> +                                enum pci_size_t size)
> +{
> +       struct rk_pcie *pcie = dev_get_priv(bus);
> +       uintptr_t va_address;
> +       ulong old;
> +
> +       debug("PCIE CFG write: (b,d,f)=(%2d,%2d,%2d)\n",
> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> +       debug("(addr,val)=(0x%04x, 0x%08lx)\n", offset, value);
> +
> +       if (!rk_pcie_addr_valid(bdf, pcie->first_busno)) {
> +               debug("- out of range\n");
> +               return 0;
> +       }
> +
> +       va_address = set_cfg_address(pcie, bdf, offset);
> +
> +       old = readl(va_address);
> +       value = pci_conv_size_to_32(old, value, offset, size);
> +       writel(value, va_address);
> +
> +       rk_pcie_prog_outbound_atu_unroll(pcie, PCIE_ATU_REGION_INDEX1,
> +                                        PCIE_ATU_TYPE_IO, pcie->io.phys_start,
> +                                        pcie->io.bus_start, pcie->io.size);
> +
> +       return 0;
> +}
> +
> +static void rk_pcie_enable_debug(struct rk_pcie *rk_pcie)
> +{
> +#if RK_PCIE_DBG
> +       rk_pcie_writel_apb(rk_pcie, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0,
> +                          PCIE_CLIENT_DBG_TRANSITION_DATA);
> +       rk_pcie_writel_apb(rk_pcie, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1,
> +                          PCIE_CLIENT_DBG_TRANSITION_DATA);
> +       rk_pcie_writel_apb(rk_pcie, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0,
> +                          PCIE_CLIENT_DBG_TRANSITION_DATA);
> +       rk_pcie_writel_apb(rk_pcie, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1,
> +                          PCIE_CLIENT_DBG_TRANSITION_DATA);
> +       rk_pcie_writel_apb(rk_pcie, PCIE_CLIENT_DBG_FIFO_MODE_CON,
> +                          PCIE_CLIENT_DBF_EN);
> +#endif
> +}
> +
> +static void rk_pcie_debug_dump(struct rk_pcie *rk_pcie)
> +{
> +#if RK_PCIE_DBG
> +       u32 loop;
> +
> +       dev_info(rk_pcie->dev, "ltssm = 0x%x\n",
> +                rk_pcie_readl_apb(rk_pcie, PCIE_CLIENT_LTSSM_STATUS));
> +       for (loop = 0; loop < 64; loop++)
> +               dev_info(rk_pcie->dev, "fifo_status = 0x%x\n",
> +                        rk_pcie_readl_apb(rk_pcie, PCIE_CLIENT_DBG_FIFO_STATUS));
> +#endif
> +}
> +
> +static inline void rk_pcie_link_status_clear(struct rk_pcie *rk_pcie)
> +{
> +       rk_pcie_writel_apb(rk_pcie, PCIE_CLIENT_GENERAL_DEBUG, 0x0);
> +}
> +
> +static inline void rk_pcie_disable_ltssm(struct rk_pcie *rk_pcie)
> +{
> +       rk_pcie_writel_apb(rk_pcie, 0x0, 0xc0008);
> +}
> +
> +static inline void rk_pcie_enable_ltssm(struct rk_pcie *rk_pcie)
> +{
> +       rk_pcie_writel_apb(rk_pcie, 0x0, 0xc000c);
> +}
> +
> +static int is_link_up(struct rk_pcie *priv)
> +{
> +       u32 val;
> +
> +       val = rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS);
> +       if ((val & (RDLH_LINKUP | SMLH_LINKUP)) == 0x30000 &&
> +           (val & GENMASK(5, 0)) == 0x11)
> +               return 1;
> +
> +       return 0;
> +}
> +
> +static int rk_pcie_link_up(struct rk_pcie *priv, u32 cap_speed)

Needs function comment, with return values.

> +{
> +       int retries;
> +
> +       if (is_link_up(priv)) {
> +               printf("PCI Link already up before configuration!\n");
> +               return 1;
> +       }
> +
> +       /* DW pre link configurations */
> +       rk_pcie_configure(priv, cap_speed);
> +
> +       /* Rest the device */
> +       if (dm_gpio_is_valid(&priv->rst_gpio)) {
> +               dm_gpio_set_value(&priv->rst_gpio, 0);
> +               msleep(1000);

Make this a #define with a comment as to why so long. This is a huge
delay to have in a driver.

> +               dm_gpio_set_value(&priv->rst_gpio, 1);
> +       }
> +
> +       rk_pcie_disable_ltssm(priv);
> +       rk_pcie_link_status_clear(priv);
> +       rk_pcie_enable_debug(priv);
> +
> +       /* Enable LTSSM */
> +       rk_pcie_enable_ltssm(priv);
> +
> +       for (retries = 0; retries < 10000000; retries++) {

Too many retries...how about 5? This effectively hangs forever if
there is a problem.

> +               if (is_link_up(priv)) {
> +                       dev_info(priv->dev, "PCIe Link up, LTSSM is 0x%x\n",
> +                                rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
> +                       rk_pcie_debug_dump(priv);
> +                       return 0;
> +               }
> +
> +               dev_info(priv->dev, "PCIe Linking... LTSSM is 0x%x\n",
> +                        rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
> +               rk_pcie_debug_dump(priv);
> +               msleep(1000);
> +       }
> +
> +       dev_err(priv->dev, "PCIe-%d Link Fail\n", dev_seq(priv->dev));
> +       /* Link maybe in Gen switch recovery but we need to wait more 1s */
> +       msleep(1000);
> +       return -EINVAL;

-EIO or something like that. We use -EINVAL when we cannot read args
from the device tree, for example.

> +}
> +
> +static int rockchip_pcie_init_port(struct udevice *dev)
> +{
> +       int ret;
> +       u32 val;
> +       struct rk_pcie *priv = dev_get_priv(dev);
> +
> +       /* Set power and maybe external ref clk input */
> +       if (priv->vpcie3v3) {
> +               ret = regulator_set_value(priv->vpcie3v3, 3300000);

Is there an autoset option for this so this info can go in the device
tree instead?

> +               if (ret) {
> +                       dev_err(priv->dev, "failed to enable vpcie3v3 (ret=%d)\n",
> +                               ret);
> +                       return ret;
> +               }
> +       }
> +
> +       msleep(1000);
> +
> +       ret = generic_phy_init(&priv->phy);
> +       if (ret) {
> +               dev_err(dev, "failed to init phy (ret=%d)\n", ret);
> +               return ret;
> +       }
> +
> +       ret = generic_phy_power_on(&priv->phy);
> +       if (ret) {
> +               dev_err(dev, "failed to power on phy (ret=%d)\n", ret);
> +               goto err_exit_phy;
> +       }
> +
> +       ret = reset_deassert_bulk(&priv->rsts);
> +       if (ret) {
> +               dev_err(dev, "failed to deassert resets (ret=%d)\n", ret);
> +               goto err_power_off_phy;
> +       }
> +
> +       ret = clk_enable_bulk(&priv->clks);
> +       if (ret) {
> +               dev_err(dev, "failed to enable clks (ret=%d)\n", ret);
> +               goto err_deassert_bulk;
> +       }
> +
> +       /* LTSSM EN ctrl mode */
> +       val = rk_pcie_readl_apb(priv, PCIE_CLIENT_HOT_RESET_CTRL);
> +       val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16);
> +       rk_pcie_writel_apb(priv, PCIE_CLIENT_HOT_RESET_CTRL, val);
> +
> +       /* Set RC mode */
> +       rk_pcie_writel_apb(priv, 0x0, 0xf00040);
> +       rk_pcie_setup_host(priv);
> +
> +       ret = rk_pcie_link_up(priv, LINK_SPEED_GEN_3);
> +       if (ret < 0)
> +               goto err_link_up;
> +
> +       return 0;
> +err_link_up:
> +       clk_disable_bulk(&priv->clks);
> +err_deassert_bulk:
> +       reset_assert_bulk(&priv->rsts);
> +err_power_off_phy:
> +       generic_phy_power_off(&priv->phy);
> +err_exit_phy:
> +       generic_phy_exit(&priv->phy);

Blank line before final return

> +       return ret;
> +}
> +
> +static int rockchip_pcie_parse_dt(struct udevice *dev)
> +{
> +       struct rk_pcie *priv = dev_get_priv(dev);
> +       int ret;
> +
> +       priv->dbi_base = (void *)dev_read_addr_index(dev, 0);
> +       if (!priv->dbi_base)
> +               return -ENODEV;
> +
> +       dev_dbg(dev, "DBI address is 0x%p\n", priv->dbi_base);
> +
> +       priv->apb_base = (void *)dev_read_addr_index(dev, 1);
> +       if (!priv->apb_base)
> +               return -ENODEV;
> +
> +       dev_dbg(dev, "APB address is 0x%p\n", priv->apb_base);
> +
> +       ret = gpio_request_by_name(dev, "reset-gpios", 0,
> +                                  &priv->rst_gpio, GPIOD_IS_OUT);
> +       if (ret) {
> +               dev_err(dev, "failed to find reset-gpios property\n");
> +               return ret;
> +       }
> +
> +       ret = reset_get_bulk(dev, &priv->rsts);
> +       if (ret) {
> +               dev_err(dev, "Can't get reset: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = clk_get_bulk(dev, &priv->clks);
> +       if (ret) {
> +               dev_err(dev, "Can't get clock: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = device_get_supply_regulator(dev, "vpcie3v3-supply",
> +                                         &priv->vpcie3v3);
> +       if (ret && ret != -ENOENT) {
> +               dev_err(dev, "failed to get vpcie3v3 supply (ret=%d)\n", ret);
> +               return ret;
> +       }
> +
> +       ret = generic_phy_get_by_index(dev, 0, &priv->phy);
> +       if (ret) {
> +               dev_err(dev, "failed to get pcie phy (ret=%d)\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rockchip_pcie_probe(struct udevice *dev)
> +{
> +       struct rk_pcie *priv = dev_get_priv(dev);
> +       struct udevice *ctlr = pci_get_controller(dev);
> +       struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> +       int ret;
> +
> +       priv->first_busno = dev_seq(dev);
> +       priv->dev = dev;
> +
> +       ret = rockchip_pcie_parse_dt(dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = rockchip_pcie_init_port(dev);
> +       if (ret)
> +               return ret;
> +
> +       dev_info(dev, "PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n",
> +                dev_seq(dev), rk_pcie_get_link_speed(priv),
> +                rk_pcie_get_link_width(priv),
> +                hose->first_busno);
> +
> +       for (ret = 0; ret < hose->region_count; ret++) {
> +               if (hose->regions[ret].flags == PCI_REGION_IO) {
> +                       priv->io.phys_start = hose->regions[ret].phys_start; /* IO base */
> +                       priv->io.bus_start  = hose->regions[ret].bus_start;  /* IO_bus_addr */
> +                       priv->io.size       = hose->regions[ret].size;      /* IO size */
> +               } else if (hose->regions[ret].flags == PCI_REGION_MEM) {
> +                       priv->mem.phys_start = hose->regions[ret].phys_start; /* MEM base */
> +                       priv->mem.bus_start  = hose->regions[ret].bus_start;  /* MEM_bus_addr */
> +                       priv->mem.size       = hose->regions[ret].size;     /* MEM size */
> +               } else if (hose->regions[ret].flags == PCI_REGION_SYS_MEMORY) {
> +                       priv->cfg_base = (void *)(priv->io.phys_start - priv->io.size);
> +                       priv->cfg_size = priv->io.size;
> +               } else {
> +                       dev_err(dev, "invalid flags type!\n");
> +               }
> +       }
> +
> +       dev_dbg(dev, "Config space: [0x%p - 0x%p, size 0x%llx]\n",
> +               priv->cfg_base, priv->cfg_base + priv->cfg_size,
> +               priv->cfg_size);
> +
> +       dev_dbg(dev, "IO space: [0x%llx - 0x%llx, size 0x%lx]\n",
> +               priv->io.phys_start, priv->io.phys_start + priv->io.size,
> +               priv->io.size);
> +
> +       dev_dbg(dev, "IO bus:   [0x%lx - 0x%lx, size 0x%lx]\n",
> +               priv->io.bus_start, priv->io.bus_start + priv->io.size,
> +               priv->io.size);
> +
> +       dev_dbg(dev, "MEM space: [0x%llx - 0x%llx, size 0x%lx]\n",
> +               priv->mem.phys_start, priv->mem.phys_start + priv->mem.size,
> +               priv->mem.size);
> +
> +       dev_dbg(dev, "MEM bus:   [0x%lx - 0x%lx, size 0x%lx]\n",
> +               priv->mem.bus_start, priv->mem.bus_start + priv->mem.size,
> +               priv->mem.size);
> +
> +       rk_pcie_prog_outbound_atu_unroll(priv, PCIE_ATU_REGION_INDEX0,
> +                                        PCIE_ATU_TYPE_MEM,
> +                                        priv->mem.phys_start,
> +                                        priv->mem.bus_start, priv->mem.size);

Need to check error whenever this function is called

> +       return 0;
> +}
> +
> +static const struct dm_pci_ops rockchip_pcie_ops = {
> +       .read_config    = rockchip_pcie_rd_conf,
> +       .write_config   = rockchip_pcie_wr_conf,
> +};
> +
> +static const struct udevice_id rockchip_pcie_ids[] = {
> +       { .compatible = "rockchip,rk3568-pcie" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(rockchip_dw_pcie) = {
> +       .name                   = "pcie_dw_rockchip",
> +       .id                     = UCLASS_PCI,
> +       .of_match               = rockchip_pcie_ids,
> +       .ops                    = &rockchip_pcie_ops,
> +       .probe                  = rockchip_pcie_probe,
> +       .priv_auto              = sizeof(struct rk_pcie),
> +};
> --
> 2.17.1
>
>
>

Regards,
Simon

Hi Simon

Thanks you for  reviewing it.

在 2021/1/14 23:42, Simon Glass 写道:
> Hi Shawn,
> 
> On Thu, 14 Jan 2021 at 01:15, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>

----8<------

>> +
>> +static int rockchip_pcie_init_port(struct udevice *dev)
>> +{
>> +       int ret;
>> +       u32 val;
>> +       struct rk_pcie *priv = dev_get_priv(dev);
>> +
>> +       /* Set power and maybe external ref clk input */
>> +       if (priv->vpcie3v3) {
>> +               ret = regulator_set_value(priv->vpcie3v3, 3300000);
> 
> Is there an autoset option for this so this info can go in the device
> tree instead?

I think we should control this by driver as other PCIe drivers did, as
we can't guarantee all kernel dtbs doing the right thing and 3v3 power
is very important for both of devices and PHY block.

> 
>> +               if (ret) {
>> +                       dev_err(priv->dev, "failed to enable vpcie3v3 (ret=%d)\n",
>> +                               ret);
>> +                       return ret;

....

> 
> Regards,
> Simon
> 
> 
>

Hi Shawn,

On Fri, 15 Jan 2021 at 02:15, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> Hi Simon
>
> Thanks you for  reviewing it.
>
> 在 2021/1/14 23:42, Simon Glass 写道:
> > Hi Shawn,
> >
> > On Thu, 14 Jan 2021 at 01:15, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
>
> ----8<------
>
> >> +
> >> +static int rockchip_pcie_init_port(struct udevice *dev)
> >> +{
> >> +       int ret;
> >> +       u32 val;
> >> +       struct rk_pcie *priv = dev_get_priv(dev);
> >> +
> >> +       /* Set power and maybe external ref clk input */
> >> +       if (priv->vpcie3v3) {
> >> +               ret = regulator_set_value(priv->vpcie3v3, 3300000);
> >
> > Is there an autoset option for this so this info can go in the device
> > tree instead?
>
> I think we should control this by driver as other PCIe drivers did, as
> we can't guarantee all kernel dtbs doing the right thing and 3v3 power
> is very important for both of devices and PHY block.

That's OK. Note that U-Boot uses its own devicetree so you can make
sure it is correct in a separate patch if you like.

Regards,
Simon