[U-Boot] [PATCH 12/15] pci: mvebu: Add PCIe driver for Armada-8K
Stefan Roese
sr at denx.de
Fri Nov 25 14:56:35 CET 2016
Hi Simon,
On 18.11.2016 02:14, Simon Glass wrote:
> On 15 November 2016 at 02:08, Stefan Roese <sr at denx.de> wrote:
>> From: Shadi Ammouri <shadi at marvell.com>
>>
>> This patch adds a driver for the PCIe controller integrated in the
>> Marvell Armada-8K SoC. This controller is based on the DesignWare
>> IP core.
>>
>> The original version was written by Shadi and Yehuda. I ported this
>> driver to the latest mainline U-Boot version with DM support.
>>
>> Tested on the Marvell DB-88F8040 Armada-8K eval board.
>>
>> Signed-off-by: Shadi Ammouri <shadi at marvell.com>
>> Signed-off-by: Yehuda Yitschak <yehuday at marvell.com>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Nadav Haklai <nadavh at marvell.com>
>> Cc: Neta Zur Hershkovits <neta at marvell.com>
>> Cc: Kostya Porotchkin <kostap at marvell.com>
>> Cc: Omri Itach <omrii at marvell.com>
>> Cc: Igal Liberman <igall at marvell.com>
>> Cc: Haim Boot <hayim at marvell.com>
>> Cc: Hanna Hawa <hannah at marvell.com>
>> ---
>> drivers/pci/Kconfig | 10 ++
>> drivers/pci/Makefile | 1 +
>> drivers/pci/pcie_dw_mvebu.c | 415 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 426 insertions(+)
>> create mode 100644 drivers/pci/pcie_dw_mvebu.c
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index b8376b4..ff2c370 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -33,6 +33,16 @@ config PCI_PNP
>> help
>> Enable PCI memory and I/O space resource allocation and assignment.
>>
>> +config PCIE_DW_MVEBU
>> + bool "Enable Armada-8K PCIe driver (DesignWare core)"
>> + default n
>> + depends on DM_PCI
>> + depends on ARMADA_8K
>> + help
>> + Say Y here if you want to enable PCIe controller support on
>> + Armada-8K SoCs. The PCIe controller on Armada-8K is based on
>> + DesignWare hardware.
>> +
>> config PCI_SANDBOX
>> bool "Sandbox PCI support"
>> depends on SANDBOX && DM_PCI
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 9583e91..86717a4 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -30,5 +30,6 @@ obj-$(CONFIG_SH7780_PCI) +=pci_sh7780.o
>> obj-$(CONFIG_PCI_TEGRA) += pci_tegra.o
>> obj-$(CONFIG_TSI108_PCI) += tsi108_pci.o
>> obj-$(CONFIG_WINBOND_83C553) += w83c553f.o
>> +obj-$(CONFIG_PCIE_DW_MVEBU) += pcie_dw_mvebu.o
>> obj-$(CONFIG_PCIE_LAYERSCAPE) += pcie_layerscape.o
>> obj-$(CONFIG_PCI_XILINX) += pcie_xilinx.o
>> diff --git a/drivers/pci/pcie_dw_mvebu.c b/drivers/pci/pcie_dw_mvebu.c
>> new file mode 100644
>> index 0000000..dd5ba34
>> --- /dev/null
>> +++ b/drivers/pci/pcie_dw_mvebu.c
>> @@ -0,0 +1,415 @@
>> +/*
>> + * Copyright (C) 2015 Marvell International Ltd.
>> + *
>> + * Copyright (C) 2016 Stefan Roese <sr at denx.de>
>> + *
>> + * Based on:
>> + * - drivers/pci/pcie_imx.c
>> + * - drivers/pci/pci_mvebu.c
>> + * - drivers/pci/pcie_xilinx.c
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <pci.h>
>> +#include <asm/io.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +/* PCI Config space registers */
>> +#define PCIE_CONFIG_BAR0 0x10
>> +#define PCIE_LINK_STATUS_REG 0x80
>> +#define PCIE_LINK_STATUS_SPEED_OFF 16
>> +#define PCIE_LINK_STATUS_SPEED_MASK 0xf
>> +#define PCIE_LINK_STATUS_WIDTH_OFF 20
>> +#define PCIE_LINK_STATUS_WIDTH_MASK 0xf
>> +
>> +/* Resizable bar capability registers */
>> +#define RESIZABLE_BAR_CAP 0x250
>> +#define RESIZABLE_BAR_CTL0 0x254
>> +#define RESIZABLE_BAR_CTL1 0x258
>> +
>> +/* iATU registers */
>> +#define PCIE_ATU_VIEWPORT 0x900
>> +#define PCIE_ATU_REGION_INBOUND (0x1 << 31)
>> +#define PCIE_ATU_REGION_OUTBOUND (0x0 << 31)
>> +#define PCIE_ATU_REGION_INDEX1 (0x1 << 0)
>> +#define PCIE_ATU_REGION_INDEX0 (0x0 << 0)
>> +#define PCIE_ATU_CR1 0x904
>> +#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_CR2 0x908
>> +#define PCIE_ATU_ENABLE (0x1 << 31)
>> +#define PCIE_ATU_BAR_MODE_ENABLE (0x1 << 30)
>> +#define PCIE_ATU_LOWER_BASE 0x90C
>> +#define PCIE_ATU_UPPER_BASE 0x910
>> +#define PCIE_ATU_LIMIT 0x914
>> +#define PCIE_ATU_LOWER_TARGET 0x918
>> +#define PCIE_ATU_BUS(x) (((x) & 0xff) << 24)
>> +#define PCIE_ATU_DEV(x) (((x) & 0x1f) << 19)
>> +#define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16)
>> +#define PCIE_ATU_UPPER_TARGET 0x91C
>> +
>> +#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_GEN3_RELATED 0x890
>> +#define GEN3_EQU_DISABLE (1 << 16)
>> +#define GEN3_ZRXDC_NON_COMP (1 << 0)
>> +
>> +#define PCIE_GEN3_EQU_CTRL 0x8A8
>> +#define GEN3_EQU_EVAL_2MS_DISABLE (1 << 5)
>> +
>> +#define PCIE_ROOT_COMPLEX_MODE_MASK (0xF << 4)
>> +
>> +#define PCIE_LINK_UP_TIMEOUT_MS 100
>> +
>> +#define PCIE_GLOBAL_CONTROL 0x8000
>> +#define PCIE_APP_LTSSM_EN (1 << 2)
>> +#define PCIE_DEVICE_TYPE_OFFSET (4)
>> +#define PCIE_DEVICE_TYPE_MASK (0xF)
>> +#define PCIE_DEVICE_TYPE_EP (0x0) /* Endpoint */
>> +#define PCIE_DEVICE_TYPE_LEP (0x1) /* Legacy endpoint */
>> +#define PCIE_DEVICE_TYPE_RC (0x4) /* Root complex */
>> +
>> +#define PCIE_GLOBAL_STATUS 0x8008
>> +#define PCIE_GLB_STS_RDLH_LINK_UP (1 << 1)
>> +#define PCIE_GLB_STS_PHY_LINK_UP (1 << 9)
>> +
>> +#define PCIE_ARCACHE_TRC 0x8050
>> +#define PCIE_AWCACHE_TRC 0x8054
>> +#define ARCACHE_SHAREABLE_CACHEABLE 0x3511
>> +#define AWCACHE_SHAREABLE_CACHEABLE 0x5311
>> +
>> +#define LINK_SPEED_GEN_1 0x1
>> +#define LINK_SPEED_GEN_2 0x2
>> +#define LINK_SPEED_GEN_3 0x3
>> +
>> +struct pcie_dw_mvebu {
>> + struct pci_controller hose;
>
> Why is this here? It should be accessible with dev_get_uclass_priv(bus).
Changed for v2.
>> + void *ctrl_base;
>
> It would be nice if this were a C struct rather than offsets.
If I'm not mistaken, we're moving away from using C structs in U-Boot
as its done in Linux as well.
>> + void *cfg_base;
>> + fdt_size_t cfg_size;
>> + int first_busno;
>
> What are these two for? Needs comments.
Will add in v2.
>> +};
>> +
>> +static int pcie_dw_get_link_speed(void *regs_base)
>> +{
>> + return ((readl(regs_base + PCIE_LINK_STATUS_REG)) >>
>> + PCIE_LINK_STATUS_SPEED_OFF) & PCIE_LINK_STATUS_SPEED_MASK;
>
> I suggest making the MASK a shifted value, so that you mask and then shift.
Yes, thats cleaner. Will change in v2.
>> +}
>> +
>> +static int pcie_dw_get_link_width(void *regs_base)
>> +{
>> + return ((readl(regs_base + PCIE_LINK_STATUS_REG)) >>
>> + PCIE_LINK_STATUS_WIDTH_OFF) & PCIE_LINK_STATUS_WIDTH_MASK;
>> +}
>> +
>> +static uintptr_t set_cfg_address(struct pcie_dw_mvebu *pcie,
>> + pci_dev_t d, int where)
>
> Function comment? Much of this file could do with function comments.
Sure. Let me see, what I can come up with in v2.
>> +{
>> + uintptr_t va_address;
>> +
>> + /*
>> + * Region #0 is used for Outbound CFG space access.
>> + * Direction = Outbound
>> + * Region Index = 0
>> + */
>> + writel(0, pcie->ctrl_base + PCIE_ATU_VIEWPORT);
>> +
>> + if (PCI_BUS(d) == (pcie->first_busno + 1))
>> + /* For local bus, change TLP Type field to 4. */
>> + writel(PCIE_ATU_TYPE_CFG0, pcie->ctrl_base + PCIE_ATU_CR1);
>> + else
>> + /* Otherwise, change TLP Type field to 5. */
>> + writel(PCIE_ATU_TYPE_CFG1, pcie->ctrl_base + PCIE_ATU_CR1);
>> +
>> + if (PCI_BUS(d) == pcie->first_busno) {
>> + /* Accessing root port configuration space. */
>> + va_address = (uintptr_t)pcie->ctrl_base;
>> + } else {
>> + writel(d << 8, pcie->ctrl_base + PCIE_ATU_LOWER_TARGET);
>> + va_address = (uintptr_t)pcie->cfg_base;
>> + }
>> +
>> + va_address += (where & ~0x3);
>> +
>> + return va_address;
>> +}
>> +
>> +static int pcie_dw_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 int pcie_dw_mvebu_read_config(struct udevice *bus, pci_dev_t bdf,
>> + uint offset, ulong *valuep,
>> + enum pci_size_t size)
>> +{
>> + struct pcie_dw_mvebu *pcie = dev_get_priv(bus);
>> + uintptr_t va_address;
>> + ulong value;
>> +
>> + debug("PCIE CFG read: (b,d,f)=(%2d,%2d,%2d) ",
>> + PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
>> +
>> + if (!pcie_dw_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);
>> +
>> + return 0;
>> +}
>> +
>> +static int pcie_dw_mvebu_write_config(struct udevice *bus, pci_dev_t bdf,
>> + uint offset, ulong value,
>> + enum pci_size_t size)
>> +{
>> + struct pcie_dw_mvebu *pcie = dev_get_priv(bus);
>> + uintptr_t va_address;
>> + ulong old;
>> +
>> + debug("PCIE CFG write: (b,d,f)=(%2d,%2d,%2d) ",
>> + PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
>> + debug("(addr,val)=(0x%04x, 0x%08lx)\n", offset, value);
>> +
>> + if (!pcie_dw_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);
>> +
>> + return 0;
>> +}
>> +
>> +static void pcie_dw_configure(void *regs_base, u32 cap_speed)
>> +{
>> + u32 reg;
>> +
>> + /* TODO: need to read the serdes speed from the dts
>> + and according to it configure the PCIe gen */
>
> /*
> * TODO(email): ..
> * ...
> */
Okay.
>> +
>> + /* Set link to GEN 3 */
>> + reg = readl(regs_base + PCIE_LINK_CTL_2);
>> + reg &= ~TARGET_LINK_SPEED_MASK;
>> + reg |= cap_speed;
>> + writel(reg, regs_base + PCIE_LINK_CTL_2);
>
> clrsetbits_le32()
Okay.
>> +
>> + reg = readl(regs_base + PCIE_LINK_CAPABILITY);
>> + reg &= ~TARGET_LINK_SPEED_MASK;
>> + reg |= cap_speed;
>> + writel(reg, regs_base + PCIE_LINK_CAPABILITY);
>> +
>> + reg = readl(regs_base + PCIE_GEN3_EQU_CTRL);
>> + reg |= GEN3_EQU_EVAL_2MS_DISABLE;
>> + writel(reg, regs_base + PCIE_GEN3_EQU_CTRL);
>
> setbits_le32()
Okay.
>> +}
>> +
>> +static int is_link_up(void *regs_base)
>
> const void? It would be better as a struct though
Again, I really think that we are moving away from the "old"
U-Boot rule to use structs and are more and more going with
offsets as macros - exactly what the kernel promotes.
>> +{
>> + u32 mask = PCIE_GLB_STS_RDLH_LINK_UP | PCIE_GLB_STS_PHY_LINK_UP;
>> + u32 reg;
>> +
>> + reg = readl(regs_base + PCIE_GLOBAL_STATUS);
>> + if ((reg & mask) == mask)
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +static int wait_link_up(void *regs_base)
>
> Better to return 0 if OK, -ETIMEDOUT if not.
The function is named "is_link_up()", so returning "1" for "true"
makes the calling code clearer for my taste. I can change the
function to use "bool" instead of "int" and use "false / true"
as well if you prefer this.
>> +{
>> + unsigned long timeout;
>> +
>> + timeout = get_timer(0) + PCIE_LINK_UP_TIMEOUT_MS;
>> + while (!is_link_up(regs_base)) {
>> + if (get_timer(0) > timeout)
>> + return 0;
>> + };
>> +
>> + return 1;
>> +}
>> +
>> +static int pcie_dw_mvebu_pcie_link_up(void *regs_base, u32 cap_speed)
>
> Better to return 0 if OK, -ve error if not
Changing this function to 0 / -ETIMEDOUT makes more sense
in this function in contrast to the one above, IMHO. But
this makes the use of these functions inconsistant.
>> +{
>> + u32 reg;
>> +
>> + if (!is_link_up(regs_base)) {
>> + /* Disable LTSSM state machine to enable configuration */
>> + reg = readl(regs_base + PCIE_GLOBAL_CONTROL);
>> + reg &= ~(PCIE_APP_LTSSM_EN);
>> + writel(reg, regs_base + PCIE_GLOBAL_CONTROL);
>> + }
>> +
>> + reg = readl(regs_base + PCIE_GLOBAL_CONTROL);
>> + reg &= ~(PCIE_DEVICE_TYPE_MASK << PCIE_DEVICE_TYPE_OFFSET);
>> + reg |= (PCIE_DEVICE_TYPE_RC << PCIE_DEVICE_TYPE_OFFSET);
>> + writel(reg, regs_base + PCIE_GLOBAL_CONTROL);
>> +
>> + /* Set the PCIe master AXI attributes */
>> + writel(ARCACHE_SHAREABLE_CACHEABLE, regs_base + PCIE_ARCACHE_TRC);
>> + writel(AWCACHE_SHAREABLE_CACHEABLE, regs_base + PCIE_AWCACHE_TRC);
>> +
>> + /* DW pre link configurations */
>> +
> (regs_base, cap_speed);
>> +
>> + if (!is_link_up(regs_base)) {
>> + /* Configuration done. Start LTSSM */
>> + reg = readl(regs_base + PCIE_GLOBAL_CONTROL);
>> + reg |= PCIE_APP_LTSSM_EN;
>> + writel(reg, regs_base + PCIE_GLOBAL_CONTROL);
>> + }
>> +
>> + /* Check that link was established */
>> + if (!wait_link_up(regs_base))
>> + return 0;
>> +
>> + /*
>> + * Link can be established in Gen 1. still need to wait
>> + * till MAC nagaotiation is completed
>> + */
>> + udelay(100);
>> +
>> + return 1;
>> +}
>> +
>> +/*
>> + * iATU region setup
>> + */
>> +static int pcie_dw_regions_setup(struct pcie_dw_mvebu *pcie)
>> +{
>> + /*
>> + * Region #0 is used for Outbound CFG space access.
>> + * Direction = Outbound
>> + * Region Index = 0
>> + */
>> + writel(0, pcie->ctrl_base + PCIE_ATU_VIEWPORT);
>> +
>> + writel((u32)(uintptr_t)pcie->cfg_base, pcie->ctrl_base
>> + + PCIE_ATU_LOWER_BASE);
>> + writel(0, pcie->ctrl_base + PCIE_ATU_UPPER_BASE);
>> + writel((u32)(uintptr_t)pcie->cfg_base + pcie->cfg_size,
>> + pcie->ctrl_base + PCIE_ATU_LIMIT);
>> +
>> + writel(0, pcie->ctrl_base + PCIE_ATU_LOWER_TARGET);
>> + writel(0, pcie->ctrl_base + PCIE_ATU_UPPER_TARGET);
>> + writel(PCIE_ATU_TYPE_CFG0, pcie->ctrl_base + PCIE_ATU_CR1);
>> + writel(PCIE_ATU_ENABLE, pcie->ctrl_base + PCIE_ATU_CR2);
>> +
>> + return 0;
>> +}
>> +
>> +static void pcie_dw_set_host_bars(void *regs_base)
>> +{
>> + u32 size = gd->ram_size;
>> + u64 max_size;
>> + u32 reg;
>> + u32 bar0;
>> +
>> + /* Verify the maximal BAR size */
>> + reg = readl(regs_base + RESIZABLE_BAR_CAP);
>> + max_size = 1ULL << (5 + (reg + (1 << 4)));
>> +
>> + if (size > max_size) {
>> + size = max_size;
>> + printf("Warning: PCIe BARs can't map all DRAM space\n");
>
> Isn't this an error? If so I suggest returning it.
No, its a warning as the message explains. The PCIe drivers should
still be able to work correctly, AFAICTO. If later tests show
that this is a problem which needs to get addresses, I'll come
back and will either return with error here or add the missing
fixes.
>> + }
>> +
>> + /* Set the BAR base and size towards DDR */
>> + bar0 = CONFIG_SYS_SDRAM_BASE & ~0xf;
>> + bar0 |= PCI_BASE_ADDRESS_MEM_TYPE_32;
>> + writel(CONFIG_SYS_SDRAM_BASE, regs_base + PCIE_CONFIG_BAR0);
>> +
>> + reg = ((size >> 20) - 1) << 12;
>> + writel(size, regs_base + RESIZABLE_BAR_CTL0);
>> +}
>> +
>> +static int pcie_dw_mvebu_probe(struct udevice *dev)
>> +{
>> + struct pcie_dw_mvebu *pcie = dev_get_priv(dev);
>> +
>> + pcie->first_busno = dev->seq;
>
> OK I see what that variable is now. But please comment it at the top.
Will do.
>> +
>> + /* Don't register host if link is down */
>> + if (!pcie_dw_mvebu_pcie_link_up(pcie->ctrl_base, LINK_SPEED_GEN_3)) {
>> + printf("PCIE-%d: Link down\n", dev->seq);
>> + return -ENODEV;
>> + }
>> +
>> + printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev->seq,
>> + pcie_dw_get_link_speed(pcie->ctrl_base),
>> + pcie_dw_get_link_width(pcie->ctrl_base), pcie->hose.first_busno);
>> +
>> + pcie_dw_regions_setup(pcie);
>> + pcie_dw_set_host_bars(pcie->ctrl_base);
>> +
>> + return 0;
>> +}
>> +
>> +static int pcie_dw_mvebu_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + struct pcie_dw_mvebu *pcie = dev_get_priv(dev);
>> +
>> + pcie->ctrl_base = (void *)dev_get_addr_index(dev, 0);
>> + if (IS_ERR(pcie->ctrl_base))
>> + return PTR_ERR(pcie->ctrl_base);
>> +
>> + /*
>> + * We need to know the size for the cfg space as well from the
>> + * DT "reg" property. dev_get_addr_index() doesn't provide this
>> + * size, so we need to use a different method to acquire the
>> + * address and size here.
>> + */
>> + pcie->cfg_base = (void *)fdtdec_get_addr_size_auto_noparent(
>> + gd->fdt_blob, dev->of_offset, "reg", 1, &pcie->cfg_size, true);
>
> This function returns FDT_ADDR_T_NONE on failure. Do we need to add a
> dev_get_addr_size()?
Yes, this might be better. Let me see if I can cook up a patch for
such a new function.
Thanks for your review,
Stefan
More information about the U-Boot
mailing list