From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Fri, 25 Nov 2016 14:56:35 +0100 Subject: [U-Boot] [PATCH 12/15] pci: mvebu: Add PCIe driver for Armada-8K In-Reply-To: References: <20161115090831.8426-1-sr@denx.de> <20161115090831.8426-12-sr@denx.de> Message-ID: <9019d1cd-05aa-484d-ab48-f045fa5bbe2d@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 18.11.2016 02:14, Simon Glass wrote: > On 15 November 2016 at 02:08, Stefan Roese wrote: >> From: Shadi Ammouri >> >> 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 >> Signed-off-by: Yehuda Yitschak >> Signed-off-by: Stefan Roese >> Cc: Simon Glass >> Cc: Nadav Haklai >> Cc: Neta Zur Hershkovits >> Cc: Kostya Porotchkin >> Cc: Omri Itach >> Cc: Igal Liberman >> Cc: Haim Boot >> Cc: Hanna Hawa >> --- >> 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 >> + * >> + * Based on: >> + * - drivers/pci/pcie_imx.c >> + * - drivers/pci/pci_mvebu.c >> + * - drivers/pci/pcie_xilinx.c >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +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