[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