[U-Boot] [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
Z.Q. Hou
zhiqiang.hou at nxp.com
Tue Nov 22 10:25:49 CET 2016
Hi Simon,
Sorry for my delay respond due to out of the office several days, and thanks a lot for your comments!
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: 2016年11月18日 9:15
> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Albert ARIBAUD
> <albert.u.boot at aribaud.net>; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Huan Wang-B18965
> <alison.wang at freescale.com>; Sumit Garg <sumit.garg at nxp.com>; Ruchika
> Gupta <ruchika.gupta at nxp.com>; Saksham Jain
> <saksham.jain at nxp.freescale.com>; york sun <york.sun at nxp.com>; M.H. Lian
> <minghuan.lian at nxp.com>; Bin Meng <bmeng.cn at gmail.com>; Mingkai Hu
> <mingkai.hu at nxp.com>
> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
>
> Hi,
>
> On 16 November 2016 at 02:48, Zhiqiang Hou <Zhiqiang.Hou at nxp.com>
> wrote:
> > From: Minghuan Lian <Minghuan.Lian at nxp.com>
> >
> > There are more than five kinds of Layerscape SoCs. unfortunately, PCIe
> > controller of each SoC is a little bit different. In order to avoid
> > too many macro definitions, the patch addes a new implementation of
> > PCIe driver based on DM. PCIe dts node is used to describe the
> > difference.
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian at nxp.com>
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
> > ---
> > V3:
> > - No change
> >
> > drivers/pci/Kconfig | 8 +
> > drivers/pci/pcie_layerscape.c | 761
> > ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 769 insertions(+)
> >
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index
> > b8376b4..07d21ea 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -61,4 +61,12 @@ config PCI_XILINX
> > Enable support for the Xilinx AXI bridge for PCI express, an IP
> block
> > which can be used on some generations of Xilinx FPGAs.
> >
> > +config PCIE_LAYERSCAPE
> > + bool "Layerscape PCIe support"
> > + depends on DM_PCI
> > + help
> > + Support Layerscape PCIe. The Layerscape SoC may have one or
> several
> > + PCIe controllers. The PCIe may works in RC or EP mode
> according to
> > + RCW setting.
>
> Can you please write out RC, EP, RCW in full since this is help?
>
Yes, will add the field of RCW to set the mode of PCIE.
> > +
> > endif
> > diff --git a/drivers/pci/pcie_layerscape.c
> > b/drivers/pci/pcie_layerscape.c index 2e6b986..f107d1c 100644
> > --- a/drivers/pci/pcie_layerscape.c
> > +++ b/drivers/pci/pcie_layerscape.c
> > @@ -11,11 +11,14 @@
> > #include <asm/io.h>
> > #include <errno.h>
> > #include <malloc.h>
> > +#include <dm.h>
> > #ifndef CONFIG_LS102XA
> > #include <asm/arch/fdt.h>
> > #include <asm/arch/soc.h>
> > #endif
>
> This is odd - drivers should not have board-specific code in them.
>
This 2 header files are unused for now, so will remove them.
> >
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > #ifndef CONFIG_SYS_PCI_MEMORY_BUS
> > #define CONFIG_SYS_PCI_MEMORY_BUS CONFIG_SYS_SDRAM_BASE
> #endif @@
> > -40,6 +43,7 @@
> > #define PCIE_ATU_REGION_INDEX1 (0x1 << 0)
> > #define PCIE_ATU_REGION_INDEX2 (0x2 << 0)
> > #define PCIE_ATU_REGION_INDEX3 (0x3 << 0)
> > +#define PCIE_ATU_REGION_NUM 6
> > #define PCIE_ATU_CR1 0x904
> > #define PCIE_ATU_TYPE_MEM (0x0 << 0)
> > #define PCIE_ATU_TYPE_IO (0x2 << 0)
> > @@ -58,6 +62,9 @@
> > #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16)
> > #define PCIE_ATU_UPPER_TARGET 0x91C
> >
> > +/* DBI registers */
> > +#define PCIE_SRIOV 0x178
> > +#define PCIE_STRFMR1 0x71c /* Symbol Timer & Filter Mask
> Register1 */
> > #define PCIE_DBI_RO_WR_EN 0x8bc
> >
> > #define PCIE_LINK_CAP 0x7c
> > @@ -88,6 +95,8 @@
> > #define PCIE_BAR2_SIZE (4 * 1024) /* 4K */
> > #define PCIE_BAR4_SIZE (1 * 1024 * 1024) /* 1M */
> >
> > +#ifndef CONFIG_DM_PCI
> > +
> > struct ls_pcie {
> > int idx;
> > void __iomem *dbi;
> > @@ -814,3 +823,755 @@ void ft_pci_setup(void *blob, bd_t *bd) { }
> > #endif
> > +
> > +#else
> > +
> > +/* LUT registers */
> > +#define PCIE_LUT_UDR(n) (0x800 + (n) * 8)
> > +#define PCIE_LUT_LDR(n) (0x804 + (n) * 8)
> > +#define PCIE_LUT_ENABLE (1 << 31)
> > +#define PCIE_LUT_ENTRY_COUNT 32
> > +
> > +/* PF Controll registers */
> > +#define PCIE_PF_VF_CTRL 0x7F8
> > +#define PCIE_PF_DBG 0x7FC
> > +
> > +#define PCIE_SRDS_PRTCL(idx) (PCIE1 + (idx))
> > +#define PCIE_SYS_BASE_ADDR 0x3400000
> > +#define PCIE_CCSR_SIZE 0x0100000
> > +
> > +/* CS2 */
> > +#define PCIE_CS2_OFFSET 0x1000 /* For PCIe without
> SR-IOV */
> > +
> > +#ifdef CONFIG_LS102XA
> > +/* LS1021a PCIE space */
> > +#define LS1021_PCIE_SPACE_OFFSET 0x4000000000ULL
> > +#define LS1021_PCIE_SPACE_SIZE 0x0800000000ULL
> > +
> > +/* LS1021a PEX1/2 Misc Ports Status Register */
> > +#define LS1021_PEXMSCPORTSR(pex_idx) (0x94 + (pex_idx) * 4)
> > +#define LS1021_LTSSM_STATE_SHIFT 20
> > +#endif
> > +
> > +struct ls_pcie {
> > + int idx;
> > + struct list_head list;
> > + struct udevice *bus;
> > + struct fdt_resource dbi_res;
> > + struct fdt_resource lut_res;
> > + struct fdt_resource ctrl_res;
> > + struct fdt_resource cfg_res;
> > + void __iomem *dbi;
> > + void __iomem *lut;
> > + void __iomem *ctrl;
> > + void __iomem *cfg0;
> > + void __iomem *cfg1;
> > + bool big_endian;
> > + bool enabled;
> > + int next_lut_index;
> > + struct pci_controller hose;
> > +};
> > +
> > +static LIST_HEAD(ls_pcie_list);
> > +
> > +static unsigned int dbi_readl(struct ls_pcie *pcie, unsigned int
> > +offset) {
> > + return in_le32(pcie->dbi + offset); }
> > +
> > +static void dbi_writel(struct ls_pcie *pcie, unsigned int value,
> > + unsigned int offset) {
> > + out_le32(pcie->dbi + offset, value); }
> > +
> > +#ifdef CONFIG_FSL_LSCH3
> > +static void lut_writel(struct ls_pcie *pcie, unsigned int value,
> > + unsigned int offset) {
> > + if (pcie->big_endian)
> > + out_be32(pcie->lut + offset, value);
> > + else
> > + out_le32(pcie->lut + offset, value); } #endif
> > +
> > +static unsigned int ctrl_readl(struct ls_pcie *pcie, unsigned int
> > +offset) {
> > + if (pcie->big_endian)
> > + return in_be32(pcie->ctrl + offset);
> > + else
> > + return in_le32(pcie->ctrl + offset); }
> > +
> > +static void ctrl_writel(struct ls_pcie *pcie, unsigned int value,
> > + unsigned int offset) {
> > + if (pcie->big_endian)
> > + out_be32(pcie->ctrl + offset, value);
> > + else
> > + out_le32(pcie->ctrl + offset, value); }
> > +
> > +#ifdef CONFIG_LS102XA
>
> Ick, should not have board-specific code here. Perhaps add a private run-time
> value indicating whether to do this or not.
Yes, totally agree, will consolidate this function.
>
> > +static int ls_pcie_ltssm(struct ls_pcie *pcie) {
> > + u32 state;
> > +
> > + state = ctrl_readl(pcie, LS1021_PEXMSCPORTSR(pcie->idx));
> > + state = (state >> LS1021_LTSSM_STATE_SHIFT) &
> > + LTSSM_STATE_MASK;
> > +
> > + return state;
> > +}
> > +#else
> > +static int ls_pcie_ltssm(struct ls_pcie *pcie) {
> > + return ctrl_readl(pcie, PCIE_PF_DBG) & LTSSM_STATE_MASK; }
> > +#endif
> > +
> > +static int ls_pcie_link_up(struct ls_pcie *pcie) {
> > + int ltssm;
> > +
> > + ltssm = ls_pcie_ltssm(pcie);
> > + if (ltssm < LTSSM_PCIE_L0)
> > + return 0;
> > +
> > + return 1;
> > +}
> > +
> > +static void ls_pcie_cfg0_set_busdev(struct ls_pcie *pcie, u32 busdev)
> > +{
> > + dbi_writel(pcie, PCIE_ATU_REGION_OUTBOUND |
> PCIE_ATU_REGION_INDEX0,
> > + PCIE_ATU_VIEWPORT);
> > + dbi_writel(pcie, busdev, PCIE_ATU_LOWER_TARGET); }
> > +
> > +static void ls_pcie_cfg1_set_busdev(struct ls_pcie *pcie, u32 busdev)
> > +{
> > + dbi_writel(pcie, PCIE_ATU_REGION_OUTBOUND |
> PCIE_ATU_REGION_INDEX1,
> > + PCIE_ATU_VIEWPORT);
> > + dbi_writel(pcie, busdev, PCIE_ATU_LOWER_TARGET); }
> > +
> > +static void ls_pcie_atu_outbound_set(struct ls_pcie *pcie, int idx, int type,
> > + u64 phys, u64 bus_addr,
> > +pci_size_t size) {
> > + dbi_writel(pcie, PCIE_ATU_REGION_OUTBOUND | idx,
> PCIE_ATU_VIEWPORT);
> > + dbi_writel(pcie, (u32)phys, PCIE_ATU_LOWER_BASE);
> > + dbi_writel(pcie, phys >> 32, PCIE_ATU_UPPER_BASE);
> > + dbi_writel(pcie, (u32)phys + size - 1, PCIE_ATU_LIMIT);
> > + dbi_writel(pcie, (u32)bus_addr, PCIE_ATU_LOWER_TARGET);
> > + dbi_writel(pcie, bus_addr >> 32, PCIE_ATU_UPPER_TARGET);
> > + dbi_writel(pcie, type, PCIE_ATU_CR1);
> > + dbi_writel(pcie, PCIE_ATU_ENABLE, PCIE_ATU_CR2); }
> > +
> > +/* Use bar match mode and MEM type as default */ static void
> > +ls_pcie_atu_inbound_set(struct ls_pcie *pcie, int idx,
> > + int bar, u64 phys) {
> > + dbi_writel(pcie, PCIE_ATU_REGION_INBOUND | idx,
> PCIE_ATU_VIEWPORT);
> > + dbi_writel(pcie, (u32)phys, PCIE_ATU_LOWER_TARGET);
> > + dbi_writel(pcie, phys >> 32, PCIE_ATU_UPPER_TARGET);
> > + dbi_writel(pcie, PCIE_ATU_TYPE_MEM, PCIE_ATU_CR1);
> > + dbi_writel(pcie, PCIE_ATU_ENABLE |
> PCIE_ATU_BAR_MODE_ENABLE |
> > + PCIE_ATU_BAR_NUM(bar), PCIE_ATU_CR2); }
> > +
> > +static void ls_pcie_dump_atu(struct ls_pcie *pcie) {
> > + int i;
> > +
> > + for (i = 0; i < PCIE_ATU_REGION_NUM; i++) {
> > + dbi_writel(pcie, PCIE_ATU_REGION_OUTBOUND | i,
> > + PCIE_ATU_VIEWPORT);
> > + debug("iATU%d:\n", i);
> > + debug("\tLOWER PHYS 0x%08x\n",
> > + dbi_readl(pcie, PCIE_ATU_LOWER_BASE));
> > + debug("\tUPPER PHYS 0x%08x\n",
> > + dbi_readl(pcie, PCIE_ATU_UPPER_BASE));
> > + debug("\tLOWER BUS 0x%08x\n",
> > + dbi_readl(pcie, PCIE_ATU_LOWER_TARGET));
> > + debug("\tUPPER BUS 0x%08x\n",
> > + dbi_readl(pcie, PCIE_ATU_UPPER_TARGET));
> > + debug("\tLIMIT 0x%08x\n",
> > + readl(pcie->dbi + PCIE_ATU_LIMIT));
> > + debug("\tCR1 0x%08x\n",
> > + dbi_readl(pcie, PCIE_ATU_CR1));
> > + debug("\tCR2 0x%08x\n",
> > + dbi_readl(pcie, PCIE_ATU_CR2));
> > + }
> > +}
> > +
> > +static void ls_pcie_setup_atu(struct ls_pcie *pcie) {
> > + struct pci_region *io, *mem, *pref;
> > + unsigned long long offset = 0;
> > + int idx = 0;
> > +
> > +#ifdef CONFIG_LS102XA
> > + offset = LS1021_PCIE_SPACE_OFFSET + LS1021_PCIE_SPACE_SIZE *
> > +pcie->idx; #endif
>
> Here also
>
Yes
> > +
> > + /* ATU 0 : OUTBOUND : CFG0 */
> > + ls_pcie_atu_outbound_set(pcie, PCIE_ATU_REGION_INDEX0,
> > + PCIE_ATU_TYPE_CFG0,
> > + pcie->cfg_res.start + offset,
> > + 0,
> > + fdt_resource_size(&pcie->cfg_res) /
> 2);
> > + /* ATU 1 : OUTBOUND : CFG1 */
> > + ls_pcie_atu_outbound_set(pcie, PCIE_ATU_REGION_INDEX1,
> > + PCIE_ATU_TYPE_CFG1,
> > + pcie->cfg_res.start + offset +
> > + fdt_resource_size(&pcie->cfg_res) /
> 2,
> > + 0,
> > + fdt_resource_size(&pcie->cfg_res) /
> > + 2);
> > +
> > + pci_get_regions(pcie->bus, &io, &mem, &pref);
> > + idx = PCIE_ATU_REGION_INDEX1 + 1;
> > +
> > + if (io)
> > + /* ATU : OUTBOUND : IO */
> > + ls_pcie_atu_outbound_set(pcie, idx++,
> > + PCIE_ATU_TYPE_IO,
> > + io->phys_start + offset,
> > + io->bus_start,
> > + io->size);
> > +
> > + if (mem)
> > + /* ATU : OUTBOUND : MEM */
> > + ls_pcie_atu_outbound_set(pcie, idx++,
> > + PCIE_ATU_TYPE_MEM,
> > + mem->phys_start + offset,
> > + mem->bus_start,
> > + mem->size);
> > +
> > + if (pref)
> > + /* ATU : OUTBOUND : pref */
> > + ls_pcie_atu_outbound_set(pcie, idx++,
> > + PCIE_ATU_TYPE_MEM,
> > + pref->phys_start + offset,
> > + pref->bus_start,
> > + pref->size);
> > +
> > + ls_pcie_dump_atu(pcie);
> > +}
> > +
> > +static int ls_pcie_addr_valid(struct ls_pcie *pcie, pci_dev_t bdf)
>
> Function comment, in particular the return value.
>
Yes, will add comments for this function.
> > +{
> > + struct udevice *bus = pcie->bus;
> > +
> > + if (!pcie->enabled)
> > + return -ENODEV;
>
> That means there is no device. Can it use -ENXIO instead since we should not
> be in the driver if there is no devices?
>
Yes, agree with you and will fix in next version.
> > +
> > + if (PCI_BUS(bdf) < bus->seq)
> > + return -EINVAL;
> > +
> > + if ((PCI_BUS(bdf) > bus->seq) && (!ls_pcie_link_up(pcie)))
> > + return -EINVAL;
> > +
> > + if (PCI_BUS(bdf) <= (bus->seq + 1) && (PCI_DEV(bdf) > 0))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +void *ls_pcie_conf_address(struct ls_pcie *pcie, pci_dev_t bdf,
> > + int offset) {
> > + struct udevice *bus = pcie->bus;
> > + u32 busdev;
> > +
> > + if (PCI_BUS(bdf) == bus->seq)
> > + return pcie->dbi + offset;
> > +
> > + busdev = PCIE_ATU_BUS(PCI_BUS(bdf)) |
> > + PCIE_ATU_DEV(PCI_DEV(bdf)) |
> > + PCIE_ATU_FUNC(PCI_FUNC(bdf));
> > +
> > + if (PCI_BUS(bdf) == bus->seq + 1) {
> > + ls_pcie_cfg0_set_busdev(pcie, busdev);
> > + return pcie->cfg0 + offset;
> > + } else {
> > + ls_pcie_cfg1_set_busdev(pcie, busdev);
> > + return pcie->cfg1 + offset;
> > + }
> > +}
> > +
> > +static int ls_pcie_read_config(struct udevice *bus, pci_dev_t bdf,
> > + uint offset, ulong *valuep,
> > + enum pci_size_t size) {
> > + struct ls_pcie *pcie = dev_get_priv(bus);
> > + void *address;
> > +
> > + if (ls_pcie_addr_valid(pcie, bdf)) {
> > + *valuep = pci_get_ff(size);
> > + return 0;
> > + }
> > +
> > + address = ls_pcie_conf_address(pcie, bdf, offset);
> > +
> > + switch (size) {
> > + case PCI_SIZE_8:
> > + *valuep = readb(address);
> > + return 0;
> > + case PCI_SIZE_16:
> > + *valuep = readw(address);
> > + return 0;
> > + case PCI_SIZE_32:
> > + *valuep = readl(address);
> > + return 0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ls_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
> > + uint offset, ulong value,
> > + enum pci_size_t size) {
> > + struct ls_pcie *pcie = dev_get_priv(bus);
> > + void *address;
> > +
> > + if (ls_pcie_addr_valid(pcie, bdf))
> > + return 0;
> > +
> > + address = ls_pcie_conf_address(pcie, bdf, offset);
> > +
> > + switch (size) {
> > + case PCI_SIZE_8:
> > + writeb(value, address);
> > + return 0;
> > + case PCI_SIZE_16:
> > + writew(value, address);
> > + return 0;
> > + case PCI_SIZE_32:
> > + writel(value, address);
> > + return 0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +/* Clear multi-function bit */
> > +static void ls_pcie_clear_multifunction(struct ls_pcie *pcie) {
> > + writeb(PCI_HEADER_TYPE_BRIDGE, pcie->dbi +
> PCI_HEADER_TYPE); }
> > +
> > +/* Fix class value */
> > +static void ls_pcie_fix_class(struct ls_pcie *pcie) {
> > + writew(PCI_CLASS_BRIDGE_PCI, pcie->dbi + PCI_CLASS_DEVICE); }
> > +
> > +/* Drop MSG TLP except for Vendor MSG */ static void
> > +ls_pcie_drop_msg_tlp(struct ls_pcie *pcie) {
> > + u32 val;
> > +
> > + val = dbi_readl(pcie, PCIE_STRFMR1);
> > + val &= 0xDFFFFFFF;
> > + dbi_writel(pcie, val, PCIE_STRFMR1); }
> > +
> > +/* Disable all bars in RC mode */
> > +static void ls_pcie_disable_bars(struct ls_pcie *pcie) {
> > + u32 sriov;
> > +
> > + sriov = in_le32(pcie->dbi + PCIE_SRIOV);
> > +
> > + if (PCI_EXT_CAP_ID(sriov) == PCI_EXT_CAP_ID_SRIOV)
> > + return;
>
> Comment this?
>
Will add comments for this condition.
> > +
> > + dbi_writel(pcie, 0, PCIE_CS2_OFFSET + PCI_BASE_ADDRESS_0);
> > + dbi_writel(pcie, 0, PCIE_CS2_OFFSET + PCI_BASE_ADDRESS_1);
> > + dbi_writel(pcie, 0, PCIE_CS2_OFFSET + PCI_ROM_ADDRESS1); }
> > +
> > +static void ls_pcie_setup_ctrl(struct ls_pcie *pcie) {
> > + ls_pcie_setup_atu(pcie);
> > +
> > + dbi_writel(pcie, 1, PCIE_DBI_RO_WR_EN);
> > + ls_pcie_fix_class(pcie);
> > + ls_pcie_clear_multifunction(pcie);
> > + ls_pcie_drop_msg_tlp(pcie);
> > + dbi_writel(pcie, 0, PCIE_DBI_RO_WR_EN);
> > +
> > + ls_pcie_disable_bars(pcie);
> > +}
> > +
> > +static void ls_pcie_ep_setup_atu(struct ls_pcie *pcie) {
> > + u64 phys = CONFIG_SYS_PCI_EP_MEMORY_BASE;
> > +
> > + /* ATU 0 : INBOUND : map BAR0 */
> > + ls_pcie_atu_inbound_set(pcie, 0, 0, phys);
> > + /* ATU 1 : INBOUND : map BAR1 */
> > + phys += PCIE_BAR1_SIZE;
> > + ls_pcie_atu_inbound_set(pcie, 1, 1, phys);
> > + /* ATU 2 : INBOUND : map BAR2 */
> > + phys += PCIE_BAR2_SIZE;
> > + ls_pcie_atu_inbound_set(pcie, 2, 2, phys);
> > + /* ATU 3 : INBOUND : map BAR4 */
> > + phys = CONFIG_SYS_PCI_EP_MEMORY_BASE + PCIE_BAR4_SIZE;
> > + ls_pcie_atu_inbound_set(pcie, 3, 4, phys);
> > +
> > + /* ATU 0 : OUTBOUND : map MEM */
> > + ls_pcie_atu_outbound_set(pcie, 0,
> > + PCIE_ATU_TYPE_MEM,
> > + pcie->cfg_res.start,
> > + 0,
> > + CONFIG_SYS_PCI_MEMORY_SIZE); }
> > +
> > +/* BAR0 and BAR1 are 32bit BAR2 and BAR4 are 64bit */ static void
> > +ls_pcie_ep_setup_bar(void *bar_base, int bar, u32 size) {
> > + if (size < 4 * 1024)
> > + return;
>
> Why? Comment?
Layerscape PCIe controller limited the least inbound window is 4KiB, and will add comments for this condition.
> > +
> > + switch (bar) {
> > + case 0:
> > + writel(size - 1, bar_base + PCI_BASE_ADDRESS_0);
> > + break;
> > + case 1:
> > + writel(size - 1, bar_base + PCI_BASE_ADDRESS_1);
> > + break;
> > + case 2:
> > + writel(size - 1, bar_base + PCI_BASE_ADDRESS_2);
> > + writel(0, bar_base + PCI_BASE_ADDRESS_3);
> > + break;
> > + case 4:
> > + writel(size - 1, bar_base + PCI_BASE_ADDRESS_4);
> > + writel(0, bar_base + PCI_BASE_ADDRESS_5);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +static void ls_pcie_ep_setup_bars(void *bar_base) {
> > + /* BAR0 - 32bit - 4K configuration */
> > + ls_pcie_ep_setup_bar(bar_base, 0, PCIE_BAR0_SIZE);
> > + /* BAR1 - 32bit - 8K MSIX*/
> > + ls_pcie_ep_setup_bar(bar_base, 1, PCIE_BAR1_SIZE);
> > + /* BAR2 - 64bit - 4K MEM desciptor */
> > + ls_pcie_ep_setup_bar(bar_base, 2, PCIE_BAR2_SIZE);
> > + /* BAR4 - 64bit - 1M MEM*/
> > + ls_pcie_ep_setup_bar(bar_base, 4, PCIE_BAR4_SIZE); }
> > +
> > +static void ls_pcie_setup_ep(struct ls_pcie *pcie) {
> > + u32 sriov;
> > +
> > + sriov = readl(pcie->dbi + PCIE_SRIOV);
> > + if (PCI_EXT_CAP_ID(sriov) == PCI_EXT_CAP_ID_SRIOV) {
> > + int pf, vf;
> > +
> > + for (pf = 0; pf < PCIE_PF_NUM; pf++) {
> > + for (vf = 0; vf <= PCIE_VF_NUM; vf++) {
> > + ctrl_writel(pcie, PCIE_LCTRL0_VAL(pf,
> vf),
> > + PCIE_PF_VF_CTRL);
> > +
> > + ls_pcie_ep_setup_bars(pcie->dbi);
> > + ls_pcie_ep_setup_atu(pcie);
> > + }
> > + }
> > + /* Disable CFG2 */
> > + ctrl_writel(pcie, 0, PCIE_PF_VF_CTRL);
> > + } else {
> > + ls_pcie_ep_setup_bars(pcie->dbi +
> PCIE_NO_SRIOV_BAR_BASE);
> > + ls_pcie_ep_setup_atu(pcie);
> > + }
> > +}
> > +
> > +#ifdef CONFIG_FSL_LSCH3
>
> Can this be a run-time check?
No, it is for Linux DT fixup and these functions is needed only by FSL_LSCH3 SoCs.
>
> > +/*
> > + * Return next available LUT index.
> > + */
> > +static int ls_pcie_next_lut_index(struct ls_pcie *pcie) {
> > + if (pcie->next_lut_index < PCIE_LUT_ENTRY_COUNT)
> > + return pcie->next_lut_index++;
> > + else
> > + return -1; /* LUT is full */
>
> -ENOSPC?
Yes, ENOSPC is more reasonable.
>
> > +}
> > +
> > +/*
> > + * Program a single LUT entry
> > + */
> > +static void ls_pcie_lut_set_mapping(struct ls_pcie *pcie, int index, u32
> devid,
> > + u32 streamid) {
> > + /* leave mask as all zeroes, want to match all bits */
> > + lut_writel(pcie, devid << 16, PCIE_LUT_UDR(index));
> > + lut_writel(pcie, streamid | PCIE_LUT_ENABLE,
> > +PCIE_LUT_LDR(index)); }
> > +
> > +/* returns the next available streamid */ static u32
> > +ls_pcie_next_streamid(void) {
> > + static int next_stream_id = FSL_PEX_STREAM_ID_START;
> > +
> > + if (next_stream_id > FSL_PEX_STREAM_ID_END)
> > + return 0xffffffff;
>
> Is FSL_PEX_STREAM_ID_END the maximum value, or the number of values?
The maximum value for PCIe.
> > +
> > + return next_stream_id++;
> > +}
> > +
> > +/*
> > + * An msi-map is a property to be added to the pci controller
> > + * node. It is a table, where each entry consists of 4 fields
> > + * e.g.:
> > + *
> > + * msi-map = <[devid] [phandle-to-msi-ctrl] [stream-id] [count]
> > + * [devid] [phandle-to-msi-ctrl] [stream-id] [count]>;
> > + */
> > +static void fdt_pcie_set_msi_map_entry(void *blob, struct ls_pcie *pcie,
> > + u32 devid, u32 streamid) {
> > + u32 *prop;
> > + u32 phandle;
> > + int nodeoffset;
> > +
> > + /* find pci controller node */
> > + nodeoffset = fdt_node_offset_by_compat_reg(blob, "fsl,ls-pcie",
> > +
> > + pcie->dbi_res.start);
>
> At this point I'm a bit lost, but if this is using driver model, you can use
> dev->of_offset
This function is used to fixup Linux Kernel DT instead of u-boot DT.
>
> > + if (nodeoffset < 0) {
> > + #ifdef FSL_PCIE_COMPAT /* Compatible with older version of dts
> > + node */
>
> Eek! Can't you detect this at run-time?
>
No, it's Kernel DT fixup, we plan to refactor Layerscape PCIe Linux driver using the compatible "fsl,ls-pcie",
but for now the macro FSL_PCIE_COMPAT must be defined to fixup Linux DT.
> > + nodeoffset = fdt_node_offset_by_compat_reg(blob,
> > +
> FSL_PCIE_COMPAT,
> > +
> pcie->dbi_res.start);
> > + if (nodeoffset < 0)
> > + return;
> > + #else
> > + return;
> > + #endif
> > + }
> > +
> > + /* get phandle to MSI controller */
> > + prop = (u32 *)fdt_getprop(blob, nodeoffset, "msi-parent", 0);
>
> fdtdec_getint()
The fdtdec_get_int() is not suit for this case, because the value of "msi-parent" is an index of gic-its, so there isn't a default value.
>
> > + if (prop == NULL) {
> > + printf("\n%s: ERROR: missing msi-parent: PCIe%d\n",
> > + __func__, pcie->idx);
> > + return;
>
> Return an error error and check it.
This function is used to fixup Linux DT, so this error won't block the u-boot process, and I think an error message is enough.
> > + }
> > + phandle = be32_to_cpu(*prop);
>
> fdt32_to_cpu()
>
Yes, better to use fdt32_to_cpu.
> > +
> > + /* set one msi-map row */
> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", devid);
> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", phandle);
> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", streamid);
> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", 1); }
> > +
> > +static void fdt_fixup_pcie(void *blob)
>
> This is a pretty horrible function. What is it for?
Kernel DT fixup.
> > +{
> > + struct udevice *dev, *bus;
> > + struct ls_pcie *pcie;
> > + u32 streamid;
> > + int index;
> > + pci_dev_t bdf;
> > +
> > + /* Scan all known buses */
> > + for (pci_find_first_device(&dev);
> > + dev;
> > + pci_find_next_device(&dev)) {
> > + for (bus = dev; device_is_on_pci_bus(bus);)
> > + bus = bus->parent;
> > + pcie = dev_get_priv(bus);
> > +
> > + streamid = ls_pcie_next_streamid();
> > + if (streamid == 0xffffffff) {
> > + printf("ERROR: no stream ids free\n");
> > + continue;
> > + }
> > +
> > + index = ls_pcie_next_lut_index(pcie);
> > + if (index < 0) {
> > + printf("ERROR: no LUT indexes free\n");
> > + continue;
> > + }
> > +
> > + /* the DT fixup must be relative to the hose first_busno
> */
> > + bdf = dm_pci_get_bdf(dev) - PCI_BDF(bus->seq, 0, 0);
> > + /* map PCI b.d.f to streamID in LUT */
> > + ls_pcie_lut_set_mapping(pcie, index, bdf >> 8,
> > + streamid);
> > + /* update msi-map in device tree */
> > + fdt_pcie_set_msi_map_entry(blob, pcie, bdf >> 8,
> > + streamid);
> > + }
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_OF_BOARD_SETUP
> > +#include <libfdt.h>
> > +#include <fdt_support.h>
> > +
> > +static void ft_pcie_ls_setup(void *blob, struct ls_pcie *pcie) {
> > + int off;
> > +
> > + off = fdt_node_offset_by_compat_reg(blob, "fsl,ls-pcie",
> > + pcie->dbi_res.start);
> > + if (off < 0) {
> > + #ifdef FSL_PCIE_COMPAT /* Compatible with older version of dts
> > + node */
>
> Run-time check?
No, it is still Kernel DT fixup.
> > + off = fdt_node_offset_by_compat_reg(blob,
> > +
> FSL_PCIE_COMPAT,
> > +
> pcie->dbi_res.start);
> > + if (off < 0)
> > + return;
> > + #else
> > + return;
> > + #endif
> > + }
> > +
> > + if (pcie->enabled)
> > + fdt_set_node_status(blob, off, FDT_STATUS_OKAY, 0);
> > + else
> > + fdt_set_node_status(blob, off, FDT_STATUS_DISABLED,
> > +0); }
> > +
> > +void ft_pci_setup(void *blob, bd_t *bd) {
> > + struct ls_pcie *pcie;
> > +
> > + list_for_each_entry(pcie, &ls_pcie_list, list)
> > + ft_pcie_ls_setup(blob, pcie);
> > +
> > + #ifdef CONFIG_FSL_LSCH3
>
> # in column one, but as mentioned, this should be a run-time check.
>
Yes, will correct the indent of #.
> > + fdt_fixup_pcie(blob);
> > + #endif
> > +}
> > +
> > +#else
> > +void ft_pci_setup(void *blob, bd_t *bd) { } #endif
> > +
> > +static int ls_pcie_probe(struct udevice *dev) {
> > + struct ls_pcie *pcie = dev_get_priv(dev);
> > + void *fdt = (void *)gd->fdt_blob;
>
> const void *fdt, then you don't need the cast.
>
Yes, will fix next version.
> > + int node = dev->of_offset;
> > + u8 header_type;
> > + u16 link_sta;
> > + bool ep_mode;
> > + int ret;
> > +
> > + pcie->bus = dev;
> > +
> > + ret = fdt_get_named_resource(fdt, node, "reg", "reg-names",
> > + "dbi", &pcie->dbi_res);
> > + if (ret) {
> > + printf("ls-pcie: resource \"dbi\" not found\n");
> > + return ret;
> > + }
> > +
> > + pcie->idx = (pcie->dbi_res.start - PCIE_SYS_BASE_ADDR) /
> > + PCIE_CCSR_SIZE;
> > +
> > + list_add(&pcie->list, &ls_pcie_list);
> > +
> > + pcie->enabled =
> is_serdes_configured(PCIE_SRDS_PRTCL(pcie->idx));
> > + if (!pcie->enabled) {
> > + printf("PCIe%d: %s disabled\n", pcie->idx, dev->name);
> > + return 0;
> > + }
> > +
> > + pcie->dbi = map_physmem(pcie->dbi_res.start,
> > + fdt_resource_size(&pcie->dbi_res),
> > + MAP_NOCACHE);
> > +
> > + ret = fdt_get_named_resource(fdt, node, "reg", "reg-names",
> > + "lut", &pcie->lut_res);
> > + if (!ret)
> > + pcie->lut = map_physmem(pcie->lut_res.start,
> > +
> fdt_resource_size(&pcie->lut_res),
> > + MAP_NOCACHE);
> > +
> > + ret = fdt_get_named_resource(fdt, node, "reg", "reg-names",
> > + "ctrl", &pcie->ctrl_res);
> > + if (!ret)
> > + pcie->ctrl = map_physmem(pcie->ctrl_res.start,
> > +
> fdt_resource_size(&pcie->ctrl_res),
> > + MAP_NOCACHE);
> > + if (!pcie->ctrl)
> > + pcie->ctrl = pcie->lut;
> > +
> > + if (!pcie->ctrl) {
> > + printf("%s: NOT find CTRL\n", dev->name);
> > + return 0;
>
> Return error?
Yes, it should return error.
> > + }
> > +
> > + ret = fdt_get_named_resource(fdt, node, "reg", "reg-names",
> > + "config", &pcie->cfg_res);
> > + if (ret) {
> > + printf("%s: resource \"config\" not found\n",
> dev->name);
> > + return 0;
>
> Return error?
Yes, will return error.
> > + }
> > +
> > + pcie->cfg0 = map_physmem(pcie->cfg_res.start,
> > + fdt_resource_size(&pcie->cfg_res),
> > + MAP_NOCACHE);
> > + pcie->cfg1 = pcie->cfg0 + fdt_resource_size(&pcie->cfg_res) /
> > + 2;
> > +
> > + pcie->big_endian = fdtdec_get_bool(fdt, node, "big-endian");
> > +
> > + debug("%s dbi:%lx lut:%lx ctrl:0x%lx cfg0:0x%lx, big-endian:%d\n",
> > + dev->name, (unsigned long)pcie->dbi, (unsigned
> long)pcie->lut,
> > + (unsigned long)pcie->ctrl, (unsigned long)pcie->cfg0,
> > + pcie->big_endian);
> > +
> > + header_type = readb(pcie->dbi + PCI_HEADER_TYPE);
> > + ep_mode = (header_type & 0x7f) == PCI_HEADER_TYPE_NORMAL;
> > + printf("PCIe%u: %s %s", pcie->idx, dev->name,
> > + ep_mode ? "Endpoint" : "Root Complex");
> > +
> > + if (ep_mode)
> > + ls_pcie_setup_ep(pcie);
> > + else
> > + ls_pcie_setup_ctrl(pcie);
> > +
> > + if (!ls_pcie_link_up(pcie)) {
> > + /* Let the user know there's no PCIe link */
> > + printf(": no link\n");
> > + return 0;
>
> Return error?
>
The no link condition is not an error, it is a info.
> > + }
> > +
> > + /* Print the negotiated PCIe link width */
> > + link_sta = readw(pcie->dbi + PCIE_LINK_STA);
> > + printf(": x%d gen%d\n", (link_sta & 0x3f0) >> 4, link_sta &
> > + 0xf);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dm_pci_ops ls_pcie_ops = {
> > + .read_config = ls_pcie_read_config,
> > + .write_config = ls_pcie_write_config,
> > +};
> > +
> > +static const struct udevice_id ls_pcie_ids[] = {
> > + { .compatible = "fsl,ls-pcie" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(pci_layerscape) = {
> > + .name = "pci_layerscape",
> > + .id = UCLASS_PCI,
> > + .of_match = ls_pcie_ids,
> > + .ops = &ls_pcie_ops,
> > + .probe = ls_pcie_probe,
> > + .priv_auto_alloc_size = sizeof(struct ls_pcie), };
> > +
> > +#endif /* CONFIG_DM_PCI */
> > --
> > 2.1.0.27.g96db324
> >
>
Thanks,
Zhiqiang
More information about the U-Boot
mailing list