[U-Boot] [PATCH v2 2/2] vexpress64: Juno: Add initialisation code for Juno R1 PCIe host bridge.
Ryan Harkin
ryan.harkin at linaro.org
Mon Oct 19 12:26:00 CEST 2015
On 19 October 2015 at 10:10, Liviu Dudau <liviu.dudau at foss.arm.com> wrote:
> On Fri, 16 Oct 2015 17:58:44 +0100
> Ryan Harkin <ryan.harkin at linaro.org> wrote:
>
> > 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>
>
> Hi Ryan,
>
> Thanks for testing this patchset.
>
> >
> > 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:
> > +
> > +
>
> Interesting, when running ./scripts/checkpatch.pl in my copy of U-Boot I
> get the 7 warnings about lines over 80 characters long, and the
> following message:
>
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE
> USLEEP_RANGE
>
> I see the same message when I run checkpatch.
> Not sure how to enable all the types by default and also the reason why
> I've missed them.
>
Er, I think I have done something wrong... I ran checkpatch.pl from my
kernel tree, not from the u-boot tree. U-boot's checkpatch.pl doesn't give
me those extra warnings. Sorry about that! It seems that u-boot is still
using an older checkpatch. Still, your v3 patch looks much nicer now.
Ahem.
> >
> > 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?
>
> Mostly historical, code was copied from Linux version where there was
> an #ifdef CONFIG_PHYS_ADDR_T_64BIT around the lines setting the _HIGH
> registers. I will respin with the logical grouping of registers.
>
> >
> >
> > + 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"?
>
> Well, this is embarrassing: when I've restructured the code I've
> dropped the +4 from the second writel. EDK2 is correct, as is the
> "reference" code from here:
>
>
> https://github.com/ARM-software/linux/blob/master/drivers/pci/host/pci-xr3.c
>
> I will send a v3 with the fixes incorporated.
>
> Best regards,
> Liviu
>
> >
> >
> >
> > > + /* 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
> > >
> > >
>
>
>
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
>
More information about the U-Boot
mailing list