[U-Boot] [PATCH v2 2/2] vexpress64: Juno: Add initialisation code for Juno R1 PCIe host bridge.
Ryan Harkin
ryan.harkin at linaro.org
Fri Oct 16 18:58:44 CEST 2015
Hi Liviu,
These patches work well for me, so at the very least, you can add my
Test-by for both:
Tested-by: Ryan Harkin <ryan.harkin at linaro.org>
But...
I ran checkpatch.pl across your patches and this one throws a few trivial
warnings, mostly about 80 character lines and a few like this:
CHECK: Prefer using the BIT macro
#97: FILE: board/armltd/vexpress64/pcie.c:60:
+#define JUNO_RESET_CTRL_PHY (1 << 0)
But also these:
WARNING: long udelay - prefer mdelay; see arch/arm/include/asm/delay.h
#208: FILE: board/armltd/vexpress64/pcie.c:171:
+ udelay(20000);
CHECK: Please don't use multiple blank lines
#227: FILE: board/armltd/vexpress64/pcie.c:190:
+
+
CHECK: Please don't use multiple blank lines
#251: FILE: board/armltd/vexpress64/pcie.h:12:
+
+
I'm not sure who strict the rules are followed for these types of warning.
But if you are fixing up, I also noticed a couple of whitespace nits below.
On 16 October 2015 at 15:41, Liviu Dudau <Liviu.Dudau at foss.arm.com> wrote:
> Juno R1 has an XpressRICH3 PCIe host bridge that needs to be initialised
> in order for the Linux kernel to be able to enumerate the bus. Add
> support code here that enables the host bridge, trains the links and
> sets up the Address Translation Tables.
>
> Signed-off-by: Liviu Dudau <Liviu.Dudau at foss.arm.com>
> ---
> board/armltd/vexpress64/Makefile | 2 +-
> board/armltd/vexpress64/pcie.c | 196
> +++++++++++++++++++++++++++++++++++
> board/armltd/vexpress64/pcie.h | 13 +++
> board/armltd/vexpress64/vexpress64.c | 2 +
> 4 files changed, 212 insertions(+), 1 deletion(-)
> create mode 100644 board/armltd/vexpress64/pcie.c
> create mode 100644 board/armltd/vexpress64/pcie.h
>
> diff --git a/board/armltd/vexpress64/Makefile
> b/board/armltd/vexpress64/Makefile
> index e009141..a35db40 100644
> --- a/board/armltd/vexpress64/Makefile
> +++ b/board/armltd/vexpress64/Makefile
> @@ -5,4 +5,4 @@
> # SPDX-License-Identifier: GPL-2.0+
> #
>
> -obj-y := vexpress64.o
> +obj-y := vexpress64.o pcie.o
> diff --git a/board/armltd/vexpress64/pcie.c
> b/board/armltd/vexpress64/pcie.c
> new file mode 100644
> index 0000000..367efd4
> --- /dev/null
> +++ b/board/armltd/vexpress64/pcie.c
> @@ -0,0 +1,196 @@
> +/*
> + * Copyright (C) ARM Ltd 2015
> + *
> + * Author: Liviu Dudau <Liviu.Dudau at arm.com>
> + *
> + * SPDX-Licence-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <pci_ids.h>
> +#include "pcie.h"
> +
> +/* XpressRICH3 support */
> +#define XR3_CONFIG_BASE 0x7ff30000
> +#define XR3_RESET_BASE 0x7ff20000
> +
> +#define XR3_PCI_ECAM_START 0x40000000
> +#define XR3_PCI_ECAM_SIZE 28 /* as power of 2 =
> 0x10000000 */
> +#define XR3_PCI_IOSPACE_START 0x5f800000
> +#define XR3_PCI_IOSPACE_SIZE 23 /* as power of 2 =
> 0x800000 */
> +#define XR3_PCI_MEMSPACE_START 0x50000000
> +#define XR3_PCI_MEMSPACE_SIZE 27 /* as power of 2 =
> 0x8000000 */
> +#define XR3_PCI_MEMSPACE64_START 0x4000000000
> +#define XR3_PCI_MEMSPACE64_SIZE 33 /* as power of 2 =
> 0x200000000 */
> +
> +#define JUNO_V2M_MSI_START 0x2c1c0000
> +#define JUNO_V2M_MSI_SIZE 12 /* as power of 2 = 4096 */
> +
> +#define XR3PCI_BASIC_STATUS 0x18
> +#define XR3PCI_BS_GEN_MASK (0xf << 8)
> +#define XR3PCI_BS_LINK_MASK 0xff
>
There's some strange extra whitespace in there...
+
> +#define XR3PCI_VIRTCHAN_CREDITS 0x90
> +#define XR3PCI_BRIDGE_PCI_IDS 0x9c
> +#define XR3PCI_PEX_SPC2 0xd8
> +
> +#define XR3PCI_ATR_PCIE_WIN0 0x600
> +#define XR3PCI_ATR_PCIE_WIN1 0x700
> +#define XR3PCI_ATR_AXI4_SLV0 0x800
> +
> +#define XR3PCI_ATR_TABLE_SIZE 0x20
> +#define XR3PCI_ATR_SRC_ADDR_LOW 0x0
> +#define XR3PCI_ATR_SRC_ADDR_HIGH 0x4
> +#define XR3PCI_ATR_TRSL_ADDR_LOW 0x8
> +#define XR3PCI_ATR_TRSL_ADDR_HIGH 0xc
> +#define XR3PCI_ATR_TRSL_PARAM 0x10
>
Perhaps add a newline here if you need to respin?
> +/* IDs used in the XR3PCI_ATR_TRSL_PARAM */
> +#define XR3PCI_ATR_TRSLID_AXIDEVICE (0x420004)
> +#define XR3PCI_ATR_TRSLID_AXIMEMORY (0x4e0004) /* Write-through,
> read/write allocate */
> +#define XR3PCI_ATR_TRSLID_PCIE_CONF (0x000001)
> +#define XR3PCI_ATR_TRSLID_PCIE_IO (0x020000)
> +#define XR3PCI_ATR_TRSLID_PCIE_MEMORY (0x000000)
> +
> +#define XR3PCI_ECAM_OFFSET(b, d, o) (((b) << 20) | \
> + (PCI_SLOT(d) << 15) | \
> + (PCI_FUNC(d) << 12) | o)
> +
> +#define JUNO_RESET_CTRL 0x1004
> +#define JUNO_RESET_CTRL_PHY (1 << 0)
> +#define JUNO_RESET_CTRL_RC (1 << 1)
> +
> +#define JUNO_RESET_STATUS 0x1008
> +#define JUNO_RESET_STATUS_PLL (1 << 0)
> +#define JUNO_RESET_STATUS_PHY (1 << 1)
> +#define JUNO_RESET_STATUS_RC (1 << 2)
> +#define JUNO_RESET_STATUS_MASK (JUNO_RESET_STATUS_PLL | \
> + JUNO_RESET_STATUS_PHY | \
> + JUNO_RESET_STATUS_RC)
> +
> +void xr3pci_set_atr_entry(unsigned long base, unsigned long src_addr,
> + unsigned long trsl_addr, int window_size,
> + int trsl_param)
> +{
> + /* X3PCI_ATR_SRC_ADDR_LOW:
> + - bit 0: enable entry,
> + - bits 1-6: ATR window size: total size in bytes:
> 2^(ATR_WSIZE + 1)
> + - bits 7-11: reserved
> + - bits 12-31: start of source address
> + */
> + writel((u32)(src_addr & 0xfffff000) | (window_size - 1) << 1 | 1,
> + base + XR3PCI_ATR_SRC_ADDR_LOW);
> + writel((u32)(trsl_addr & 0xfffff000), base +
> XR3PCI_ATR_TRSL_ADDR_LOW);
> + writel((u32)(src_addr >> 32), base + XR3PCI_ATR_SRC_ADDR_HIGH);
> + writel((u32)(trsl_addr >> 32), base + XR3PCI_ATR_TRSL_ADDR_HIGH);
>
It seems odd to split TRSL_ADDR_LOW and HIGH with SRC_ADDR_HIGH in between,
rather than doing SRC_ADDR_HIGH right after SRC_ADDR_LOW. Is there a
reason for that?
+ writel(trsl_param, base + XR3PCI_ATR_TRSL_PARAM);
> +
> + printf("ATR entry: 0x%010lx %s 0x%010lx [0x%010llx] (param:
> 0x%06x)\n",
> + src_addr, (trsl_param & 0x400000) ? "<-" : "->", trsl_addr,
> + ((u64)1) << window_size, trsl_param);
> +}
> +
> +void xr3pci_setup_atr(void)
> +{
> + /* setup PCIe to CPU address translation tables */
> + unsigned long base = XR3_CONFIG_BASE + XR3PCI_ATR_PCIE_WIN0;
> +
> + /* forward all writes from PCIe to GIC V2M (used for MSI) */
> + xr3pci_set_atr_entry(base, JUNO_V2M_MSI_START, JUNO_V2M_MSI_START,
> + JUNO_V2M_MSI_SIZE,
> XR3PCI_ATR_TRSLID_AXIDEVICE);
> +
> + base += XR3PCI_ATR_TABLE_SIZE;
> +
> + /* PCIe devices can write anywhere in memory */
> + xr3pci_set_atr_entry(base, PHYS_SDRAM_1, PHYS_SDRAM_1,
> + 31 /* grant access to all RAM under 4GB */,
> + XR3PCI_ATR_TRSLID_AXIMEMORY);
> + base += XR3PCI_ATR_TABLE_SIZE;
> + xr3pci_set_atr_entry(base, PHYS_SDRAM_2, PHYS_SDRAM_2,
> + XR3_PCI_MEMSPACE64_SIZE,
> + XR3PCI_ATR_TRSLID_AXIMEMORY);
> +
> +
> + /* setup CPU to PCIe address translation table */
> + base = XR3_CONFIG_BASE + XR3PCI_ATR_AXI4_SLV0;
> +
> + /* setup ECAM space to bus configuration interface */
> + xr3pci_set_atr_entry(base, XR3_PCI_ECAM_START, 0,
> XR3_PCI_ECAM_SIZE,
> + XR3PCI_ATR_TRSLID_PCIE_CONF);
> +
> + base += XR3PCI_ATR_TABLE_SIZE;
> +
> + /* setup IO space translation */
> + xr3pci_set_atr_entry(base, XR3_PCI_IOSPACE_START,
> XR3_PCI_IOSPACE_START,
> + XR3_PCI_IOSPACE_SIZE,
> XR3PCI_ATR_TRSLID_PCIE_IO);
> +
> + base += XR3PCI_ATR_TABLE_SIZE;
> +
> + /* setup 32bit MEM space translation */
> + xr3pci_set_atr_entry(base, XR3_PCI_MEMSPACE_START,
> XR3_PCI_MEMSPACE_START,
> + XR3_PCI_MEMSPACE_SIZE,
> XR3PCI_ATR_TRSLID_PCIE_MEMORY);
> +
> + base += XR3PCI_ATR_TABLE_SIZE;
> +
> + /* setup 64bit MEM space translation */
> + xr3pci_set_atr_entry(base, XR3_PCI_MEMSPACE64_START,
> XR3_PCI_MEMSPACE64_START,
> + XR3_PCI_MEMSPACE64_SIZE,
> XR3PCI_ATR_TRSLID_PCIE_MEMORY);
> +}
> +
> +void xr3pci_init(void)
> +{
> + u32 val;
> + int timeout = 200;
> +
> + /* Initialise the XpressRICH3 PCIe host bridge */
> +
> + /* add credits */
> + writel(0x00f0b818, XR3_CONFIG_BASE + XR3PCI_VIRTCHAN_CREDITS);
> + writel(0x1, XR3_CONFIG_BASE + XR3PCI_VIRTCHAN_CREDITS);
>
The equivalent code in EDK2 looks like this:
// Add credits
PCIE_ROOTPORT_WRITE32 (PCIE_VC_CRED, 0x00f0b818);
PCIE_ROOTPORT_WRITE32 (PCIE_VC_CRED + 4, 0x1);
I'm wondering if you were supposed to use the "+ 4" or if EDK2 should drop
the "+ 4"?
> + /* allow ECRC */
> + writel(0x6006, XR3_CONFIG_BASE + XR3PCI_PEX_SPC2);
>
+ /* setup the correct class code for the host bridge */
> + writel(PCI_CLASS_BRIDGE_PCI << 16, XR3_CONFIG_BASE +
> XR3PCI_BRIDGE_PCI_IDS);
> +
> + /* reset phy and root complex */
> + writel(JUNO_RESET_CTRL_PHY | JUNO_RESET_CTRL_RC,
> + XR3_RESET_BASE + JUNO_RESET_CTRL);
> +
> + do {
> + udelay(1000);
> + val = readl(XR3_RESET_BASE + JUNO_RESET_STATUS);
> + } while (--timeout &&
> + (val & JUNO_RESET_STATUS_MASK) != JUNO_RESET_STATUS_MASK);
> +
> + if (!timeout) {
> + printf("PCI XR3 Root complex reset timed out\n");
> + return;
> + }
> +
> + /* Wait for the link to train */
> + udelay(20000);
> + timeout = 20;
> +
> + do {
> + udelay(1000);
> + val = readl(XR3_CONFIG_BASE + XR3PCI_BASIC_STATUS);
> + } while (--timeout && !(val & XR3PCI_BS_LINK_MASK));
> +
> + if (!(val & XR3PCI_BS_LINK_MASK)) {
> + printf("Failed to negociate a link!\n");
>
"negotiate"
> + return;
> + }
> +
> + printf("PCIe XR3 Host Bridge enabled: x%d link (Gen %d)\n",
> + val & XR3PCI_BS_LINK_MASK, (val & XR3PCI_BS_GEN_MASK) >> 8);
> +
> + xr3pci_setup_atr();
> +}
> +
> +
> +void vexpress64_pcie_init(void)
> +{
> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
> + xr3pci_init();
> +#endif
> +}
> diff --git a/board/armltd/vexpress64/pcie.h
> b/board/armltd/vexpress64/pcie.h
> new file mode 100644
> index 0000000..d645385
> --- /dev/null
> +++ b/board/armltd/vexpress64/pcie.h
> @@ -0,0 +1,13 @@
> +#ifndef __VEXPRESS64_PCIE_H__
> +#define __VEXPRESS64_PCIE_H__
> +
> +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO
> +void vexpress64_pcie_init(void);
> +#else
> +static inline void vexpress64_pcie_init(void)
> +{
> +}
> +#endif
> +
> +
> +#endif /* __VEXPRESS64_PCIE_H__ */
> diff --git a/board/armltd/vexpress64/vexpress64.c
> b/board/armltd/vexpress64/vexpress64.c
> index 6df9d60..f4e8084 100644
> --- a/board/armltd/vexpress64/vexpress64.c
> +++ b/board/armltd/vexpress64/vexpress64.c
> @@ -13,6 +13,7 @@
> #include <linux/compiler.h>
> #include <dm/platdata.h>
> #include <dm/platform_data/serial_pl01x.h>
> +#include "pcie.h"
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -29,6 +30,7 @@ U_BOOT_DEVICE(vexpress_serials) = {
>
> int board_init(void)
> {
> + vexpress64_pcie_init();
> return 0;
> }
>
> --
> 2.6.0
>
>
More information about the U-Boot
mailing list