| Message ID | 1445006500-1859-3-git-send-email-Liviu.Dudau@foss.arm.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Fri, 16 Oct 2015 17:58:44 +0100 Ryan Harkin <ryan.harkin@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@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.Dudau@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@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@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 > > > >
On 19 October 2015 at 10:10, Liviu Dudau <liviu.dudau@foss.arm.com> wrote: > On Fri, 16 Oct 2015 17:58:44 +0100 > Ryan Harkin <ryan.harkin@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@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@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@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@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! / > --------------- > ¯\_(ツ)_/¯ >
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@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 + +#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 +/* 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); + 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); + /* 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"); + 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; }
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@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