On Fri, 16 Oct 2015 17:58:44 +0100
Ryan Harkin <ryan.har...@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.har...@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
Not sure how to enable all the types by default and also the reason why
I've missed them.
>
> 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.du...@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.du...@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.du...@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! /
---------------
¯\_(ツ)_/¯
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
