[U-Boot,PATCHv3,09/15] pci: layerscape: add pci driver based on DM

From: Minghuan Lian <Minghuan.Lian@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@nxp.com>
Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
V3:
 - No change

 drivers/pci/Kconfig           |   8 +
 drivers/pci/pcie_layerscape.c | 761 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 769 insertions(+)

Hi,

On 16 November 2016 at 02:48, Zhiqiang Hou <Zhiqiang.Hou@nxp.com> wrote:
> From: Minghuan Lian <Minghuan.Lian@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@nxp.com>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@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?

> +
>  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.

>
> +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.

> +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

> +
> +       /* 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.

> +{
> +       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?

> +
> +       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?

> +
> +       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?

> +
> +       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?

> +/*
> + * 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?

> +}
> +
> +/*
> + * 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?

> +
> +       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

> +       if (nodeoffset < 0) {
> +       #ifdef FSL_PCIE_COMPAT /* Compatible with older version of dts node */

Eek! Can't you detect this at run-time?

> +               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()

> +       if (prop == NULL) {
> +               printf("\n%s: ERROR: missing msi-parent: PCIe%d\n",
> +                      __func__, pcie->idx);
> +               return;

Return an error error and check it.

> +       }
> +       phandle = be32_to_cpu(*prop);

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?

> +{
> +       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?

> +               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.

> +       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.

> +       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?

> +       }
> +
> +       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?

> +       }
> +
> +       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?

> +       }
> +
> +       /* 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
>

Regards,
Simon

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@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> Sent: 2016年11月18日 9:15

> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika

> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian

> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu

> <mingkai.hu@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@nxp.com>

> wrote:

> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>

> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@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

Hi,

On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
> 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@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> Sent: 2016年11月18日 9:15
>> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika
>> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian
>> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu
>> <mingkai.hu@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@nxp.com>
>> wrote:
>> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>
>> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>> > ---
>> > V3:
>> >  - No change
>> >
>> >  drivers/pci/Kconfig           |   8 +
>> >  drivers/pci/pcie_layerscape.c | 761
>> > ++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 769 insertions(+)
>> >

>> > +#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.

I mean that you cannot have an #ifdef in a driver - it should be done
at run-time by looking at the compatible strings.

>
>>
>> > +/*
>> > + * 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.

They should use the same 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.

I'm still confused by this. I don't see it defined anywhere and it is
not a CONFIG. Can you not detect at run-time when you need to do the
fix-up?

>
>> > +               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.

Try:

   val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)
   if (val == -1) {
      debug(...);
      return -EINVAL;
   }

>
>>
>> > +       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.

If it is an error it should return an error. If it is just a warning
it should say so, ideally using debug(). As it is, it is very
confusing for the user to get this message.

>
>> > +       }
>> > +       phandle = be32_to_cpu(*prop);
>>
>> fdt32_to_cpu()
>>
>
> Yes, better to use fdt32_to_cpu.

But where do you use that value? Also. consider fdtdec_lookup_phandle().

>
>> > +
>> > +       /* 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.

OK, well please add some comments!

[...]

Regards,
Simon

Hi Simon,

Thanks for your comments!

> -----Original Message-----

> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> Sent: 2016年11月24日 10:21

> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika

> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian

> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu

> <mingkai.hu@nxp.com>

> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM

> 

> Hi,

> 

> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> > 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@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> >> Sent: 2016年11月18日 9:15

> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> Ruchika

> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.

> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> Mingkai

> >> Hu <mingkai.hu@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@nxp.com>

> >> wrote:

> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>

> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

> >> > ---

> >> > V3:

> >> >  - No change

> >> >

> >> >  drivers/pci/Kconfig           |   8 +

> >> >  drivers/pci/pcie_layerscape.c | 761

> >> > ++++++++++++++++++++++++++++++++++++++++++

> >> >  2 files changed, 769 insertions(+)

> >> >

> 

> >> > +#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.

> 

> I mean that you cannot have an #ifdef in a driver - it should be done at

> run-time by looking at the compatible strings.


This driver work for many platforms, but this fixup is only used by FSL_LSCH3 SoCs,
if check the compatible string at run-time, the fixup will be still compiled for the platform which doesn't need it.
Why compile it into the binary for the platform which doesn't need it?

> >

> >>

> >> > +/*

> >> > + * 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.

> 

> They should use the same DT.


Yes, Ideally they should, but up to now actually Kernel does not use the one u-boot
used, so we cannot make sure the offset of the nodes are the same.
So to ensure the fixup work, get the node offset from kernel 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.

> 

> I'm still confused by this. I don't see it defined anywhere and it is not a CONFIG.

> Can you not detect at run-time when you need to do the fix-up?


Ok, the process is find the node offset by "fsl,ls-pcie" first, if failed, find it again by FSL_PCIE_COMPAT.
But in the current kernel DT the name of PCIe controller node is NOT the "fsl,ls-pcie" which we will
refactor layerscape pcie kernel driver to use, so far it is the FSL_PCIE_COMPAT which is defined
according to the current kernel DT in header file include/configs/ls*.h.
So it is unable to be detected at run-time, but it will be removed when the kernel driver refactored.

> 

> >

> >> > +               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.

> 

> Try:

> 

>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)

>    if (val == -1) {

>       debug(...);

>       return -EINVAL;

>    }

> 


Any benefit compared with fdt_getprop? I'm confused by this function, what if the correct value equal to the given 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.

> 

> If it is an error it should return an error. If it is just a warning it should say so,

> ideally using debug(). As it is, it is very confusing for the user to get this

> message.


Will replace with debug().

> >

> >> > +       }

> >> > +       phandle = be32_to_cpu(*prop);

> >>

> >> fdt32_to_cpu()

> >>

> >

> > Yes, better to use fdt32_to_cpu.

> 

> But where do you use that value? Also. consider fdtdec_lookup_phandle().


Thanks for your tip, just the value of this phandle is used, see the lines below.
 
> >

> >> > +

> >> > +       /* 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.

> 

> OK, well please add some comments!


Will comment it.
 
> [...]

> 

> Regards,

> Simon

Hi,

On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
> Hi Simon,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> Sent: 2016年11月24日 10:21
>> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika
>> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian
>> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu
>> <mingkai.hu@nxp.com>
>> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
>>
>> Hi,
>>
>> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>> > 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@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> >> Sent: 2016年11月18日 9:15
>> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;
>> Ruchika
>> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.
>> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;
>> Mingkai
>> >> Hu <mingkai.hu@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@nxp.com>
>> >> wrote:
>> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>
>> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>> >> > ---
>> >> > V3:
>> >> >  - No change
>> >> >
>> >> >  drivers/pci/Kconfig           |   8 +
>> >> >  drivers/pci/pcie_layerscape.c | 761
>> >> > ++++++++++++++++++++++++++++++++++++++++++
>> >> >  2 files changed, 769 insertions(+)
>> >> >
>>
>> >> > +#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.
>>
>> I mean that you cannot have an #ifdef in a driver - it should be done at
>> run-time by looking at the compatible strings.
>
> This driver work for many platforms, but this fixup is only used by FSL_LSCH3 SoCs,
> if check the compatible string at run-time, the fixup will be still compiled for the platform which doesn't need it.
> Why compile it into the binary for the platform which doesn't need it?

Because that's how it works. Drivers are drivers for their hardware.
We cannot compile them differently depending on who might use them...

If this is a big problem you could split the driver into multiple
parts perhaps. But what exactly is the problem here?

>
>> >
>> >>
>> >> > +/*
>> >> > + * 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.
>>
>> They should use the same DT.
>
> Yes, Ideally they should, but up to now actually Kernel does not use the one u-boot
> used, so we cannot make sure the offset of the nodes are the same.
> So to ensure the fixup work, get the node offset from kernel DT.

Is it not possible to change U-Boot to use the kernel DT? It might be less work.

>
>>
>> >
>> >>
>> >> > +       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.
>>
>> I'm still confused by this. I don't see it defined anywhere and it is not a CONFIG.
>> Can you not detect at run-time when you need to do the fix-up?
>
> Ok, the process is find the node offset by "fsl,ls-pcie" first, if failed, find it again by FSL_PCIE_COMPAT.
> But in the current kernel DT the name of PCIe controller node is NOT the "fsl,ls-pcie" which we will
> refactor layerscape pcie kernel driver to use, so far it is the FSL_PCIE_COMPAT which is defined
> according to the current kernel DT in header file include/configs/ls*.h.
> So it is unable to be detected at run-time, but it will be removed when the kernel driver refactored.

OK, so how about making this a new CONFIG which you can turn on/off?

>
>>
>> >
>> >> > +               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.
>>
>> Try:
>>
>>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)
>>    if (val == -1) {
>>       debug(...);
>>       return -EINVAL;
>>    }
>>
>
> Any benefit compared with fdt_getprop? I'm confused by this function, what if the correct value equal to the given default value?

You choose an invalid default. If there isn't one then you cannot use
this function. The benefit is that it avoids the be32_to_cpu().
>
>> >
>> >>
>> >> > +       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.
>>
>> If it is an error it should return an error. If it is just a warning it should say so,
>> ideally using debug(). As it is, it is very confusing for the user to get this
>> message.
>
> Will replace with debug().
>
>> >
>> >> > +       }
>> >> > +       phandle = be32_to_cpu(*prop);
>> >>
>> >> fdt32_to_cpu()
>> >>
>> >
>> > Yes, better to use fdt32_to_cpu.
>>
>> But where do you use that value? Also. consider fdtdec_lookup_phandle().
>
> Thanks for your tip, just the value of this phandle is used, see the lines below.

OK I see.

>
>> >
>> >> > +
>> >> > +       /* 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.
>>
>> OK, well please add some comments!
>
> Will comment it.

Regards,
Simon

Hi Simon,

Thanks for your comments!

> -----Original Message-----

> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> Sent: 2016年11月28日 1:02

> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika

> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian

> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu

> <mingkai.hu@nxp.com>

> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM

> 

> Hi,

> 

> On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> > Hi Simon,

> >

> > Thanks for your comments!

> >

> >> -----Original Message-----

> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> >> Sent: 2016年11月24日 10:21

> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> Ruchika

> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.

> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> Mingkai

> >> Hu <mingkai.hu@nxp.com>

> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on

> >> DM

> >>

> >> Hi,

> >>

> >> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> >> > 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@google.com [mailto:sjg@google.com] On Behalf Of Simon

> >> >> Glass

> >> >> Sent: 2016年11月18日 9:15

> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> >> Ruchika

> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;

> M.H.

> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> >> Mingkai

> >> >> Hu <mingkai.hu@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@nxp.com>

> >> >> wrote:

> >> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>

> >> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

> >> >> > ---

> >> >> > V3:

> >> >> >  - No change

> >> >> >

> >> >> >  drivers/pci/Kconfig           |   8 +

> >> >> >  drivers/pci/pcie_layerscape.c | 761

> >> >> > ++++++++++++++++++++++++++++++++++++++++++

> >> >> >  2 files changed, 769 insertions(+)

> >> >> >

> >>

> >> >> > +#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.

> >>

> >> I mean that you cannot have an #ifdef in a driver - it should be done

> >> at run-time by looking at the compatible strings.

> >

> > This driver work for many platforms, but this fixup is only used by

> > FSL_LSCH3 SoCs, if check the compatible string at run-time, the fixup will be

> still compiled for the platform which doesn't need it.

> > Why compile it into the binary for the platform which doesn't need it?

> 

> Because that's how it works. Drivers are drivers for their hardware.

> We cannot compile them differently depending on who might use them...

> 

> If this is a big problem you could split the driver into multiple parts perhaps. But

> what exactly is the problem here?


It isn't a big problem, actually it is just kernel DT fixup function, and it doesn't affect the u-boot pcie driver.
But the fixup is LSCH3 SoC special, and some macros are only defined in header file of LSCH3, e.g. FSL_PEX_STREAM_ID_*.
So cannot removed the #ifdef CONFIG_FSL_LSCH3.

> 

> >

> >> >

> >> >>

> >> >> > +/*

> >> >> > + * 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.

> >>

> >> They should use the same DT.

> >

> > Yes, Ideally they should, but up to now actually Kernel does not use

> > the one u-boot used, so we cannot make sure the offset of the nodes are the

> same.

> > So to ensure the fixup work, get the node offset from kernel DT.

> 

> Is it not possible to change U-Boot to use the kernel DT? It might be less work.


Since this is used to fixup Kernel DT, and u-boot and Kernel use two copies of DT, until the u-boot and kernel use one copy of DT, we must fixup the one works for Kernel. 

> 

> >

> >>

> >> >

> >> >>

> >> >> > +       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.

> >>

> >> I'm still confused by this. I don't see it defined anywhere and it is not a

> CONFIG.

> >> Can you not detect at run-time when you need to do the fix-up?

> >

> > Ok, the process is find the node offset by "fsl,ls-pcie" first, if failed, find it

> again by FSL_PCIE_COMPAT.

> > But in the current kernel DT the name of PCIe controller node is NOT

> > the "fsl,ls-pcie" which we will refactor layerscape pcie kernel driver

> > to use, so far it is the FSL_PCIE_COMPAT which is defined according to the

> current kernel DT in header file include/configs/ls*.h.

> > So it is unable to be detected at run-time, but it will be removed when the

> kernel driver refactored.

> 

> OK, so how about making this a new CONFIG which you can turn on/off?


Yes, will move it to CONFIG_ FSL_PCIE_COMPAT.
 
> >

> >>

> >> >

> >> >> > +               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.

> >>

> >> Try:

> >>

> >>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)

> >>    if (val == -1) {

> >>       debug(...);

> >>       return -EINVAL;

> >>    }

> >>

> >

> > Any benefit compared with fdt_getprop? I'm confused by this function, what

> if the correct value equal to the given default value?

> 

> You choose an invalid default. If there isn't one then you cannot use this

> function. The benefit is that it avoids the be32_to_cpu().


The value of this property is a reference of other node and don't know which is the invalid value.
Do you have any suggestion about this case?

> >

> >> >

> >> >>

> >> >> > +       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.

> >>

> >> If it is an error it should return an error. If it is just a warning

> >> it should say so, ideally using debug(). As it is, it is very

> >> confusing for the user to get this message.

> >

> > Will replace with debug().

> >

> >> >

> >> >> > +       }

> >> >> > +       phandle = be32_to_cpu(*prop);

> >> >>

> >> >> fdt32_to_cpu()

> >> >>

> >> >

> >> > Yes, better to use fdt32_to_cpu.

> >>

> >> But where do you use that value? Also. consider fdtdec_lookup_phandle().

> >

> > Thanks for your tip, just the value of this phandle is used, see the lines below.

> 

> OK I see.

> 

> >

> >> >

> >> >> > +

> >> >> > +       /* 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.

> >>

> >> OK, well please add some comments!

> >

> > Will comment it.


Regards,
Zhiqiang

Hi,

On 27 November 2016 at 22:59, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
> Hi Simon,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> Sent: 2016年11月28日 1:02
>> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika
>> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian
>> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu
>> <mingkai.hu@nxp.com>
>> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
>>
>> Hi,
>>
>> On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>> > Hi Simon,
>> >
>> > Thanks for your comments!
>> >
>> >> -----Original Message-----
>> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> >> Sent: 2016年11月24日 10:21
>> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;
>> Ruchika
>> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.
>> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;
>> Mingkai
>> >> Hu <mingkai.hu@nxp.com>
>> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on
>> >> DM
>> >>
>> >> Hi,
>> >>
>> >> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>> >> > 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@google.com [mailto:sjg@google.com] On Behalf Of Simon
>> >> >> Glass
>> >> >> Sent: 2016年11月18日 9:15
>> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;
>> >> Ruchika
>> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;
>> M.H.
>> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;
>> >> Mingkai
>> >> >> Hu <mingkai.hu@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@nxp.com>
>> >> >> wrote:
>> >> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>
>> >> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>> >> >> > ---
>> >> >> > V3:
>> >> >> >  - No change
>> >> >> >
>> >> >> >  drivers/pci/Kconfig           |   8 +
>> >> >> >  drivers/pci/pcie_layerscape.c | 761
>> >> >> > ++++++++++++++++++++++++++++++++++++++++++
>> >> >> >  2 files changed, 769 insertions(+)
>> >> >> >
>> >>
>> >> >> > +#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.
>> >>
>> >> I mean that you cannot have an #ifdef in a driver - it should be done
>> >> at run-time by looking at the compatible strings.
>> >
>> > This driver work for many platforms, but this fixup is only used by
>> > FSL_LSCH3 SoCs, if check the compatible string at run-time, the fixup will be
>> still compiled for the platform which doesn't need it.
>> > Why compile it into the binary for the platform which doesn't need it?
>>
>> Because that's how it works. Drivers are drivers for their hardware.
>> We cannot compile them differently depending on who might use them...
>>
>> If this is a big problem you could split the driver into multiple parts perhaps. But
>> what exactly is the problem here?
>
> It isn't a big problem, actually it is just kernel DT fixup function, and it doesn't affect the u-boot pcie driver.
> But the fixup is LSCH3 SoC special, and some macros are only defined in header file of LSCH3, e.g. FSL_PEX_STREAM_ID_*.
> So cannot removed the #ifdef CONFIG_FSL_LSCH3.

Really there should be two separate drivers, with a shared common file
for common code.

Failing that, is it really impossible to include the extra macros regardless?

If we start putting board-specific #ifdefs in drivers, we have lost
the DM battle.

>
>>
>> >
>> >> >
>> >> >>
>> >> >> > +/*
>> >> >> > + * 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.
>> >>
>> >> They should use the same DT.
>> >
>> > Yes, Ideally they should, but up to now actually Kernel does not use
>> > the one u-boot used, so we cannot make sure the offset of the nodes are the
>> same.
>> > So to ensure the fixup work, get the node offset from kernel DT.
>>
>> Is it not possible to change U-Boot to use the kernel DT? It might be less work.
>
> Since this is used to fixup Kernel DT, and u-boot and Kernel use two copies of DT, until the u-boot and kernel use one copy of DT, we must fixup the one works for Kernel.

OK. Please add a TODO(email) prominently.

>
>>
>> >
>> >>
>> >> >
>> >> >>
>> >> >> > +       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.
>> >>
>> >> I'm still confused by this. I don't see it defined anywhere and it is not a
>> CONFIG.
>> >> Can you not detect at run-time when you need to do the fix-up?
>> >
>> > Ok, the process is find the node offset by "fsl,ls-pcie" first, if failed, find it
>> again by FSL_PCIE_COMPAT.
>> > But in the current kernel DT the name of PCIe controller node is NOT
>> > the "fsl,ls-pcie" which we will refactor layerscape pcie kernel driver
>> > to use, so far it is the FSL_PCIE_COMPAT which is defined according to the
>> current kernel DT in header file include/configs/ls*.h.
>> > So it is unable to be detected at run-time, but it will be removed when the
>> kernel driver refactored.
>>
>> OK, so how about making this a new CONFIG which you can turn on/off?
>
> Yes, will move it to CONFIG_ FSL_PCIE_COMPAT.
>
>> >
>> >>
>> >> >
>> >> >> > +               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.
>> >>
>> >> Try:
>> >>
>> >>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)
>> >>    if (val == -1) {
>> >>       debug(...);
>> >>       return -EINVAL;
>> >>    }
>> >>
>> >
>> > Any benefit compared with fdt_getprop? I'm confused by this function, what
>> if the correct value equal to the given default value?
>>
>> You choose an invalid default. If there isn't one then you cannot use this
>> function. The benefit is that it avoids the be32_to_cpu().
>
> The value of this property is a reference of other node and don't know which is the invalid value.
> Do you have any suggestion about this case?

Well, phandles cannot be < 0, so how about -1?

>
>> >
>> >> >
>> >> >>
>> >> >> > +       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.
>> >>
>> >> If it is an error it should return an error. If it is just a warning
>> >> it should say so, ideally using debug(). As it is, it is very
>> >> confusing for the user to get this message.
>> >
>> > Will replace with debug().
>> >
>> >> >
>> >> >> > +       }
>> >> >> > +       phandle = be32_to_cpu(*prop);
>> >> >>
>> >> >> fdt32_to_cpu()
>> >> >>
>> >> >
>> >> > Yes, better to use fdt32_to_cpu.
>> >>
>> >> But where do you use that value? Also. consider fdtdec_lookup_phandle().
>> >
>> > Thanks for your tip, just the value of this phandle is used, see the lines below.
>>
>> OK I see.
>>
>> >
>> >> >
>> >> >> > +
>> >> >> > +       /* 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.
>> >>
>> >> OK, well please add some comments!
>> >
>> > Will comment it.
>
> Regards,
> Zhiqiang

Regards,
Simon

Hi Simon,

Thanks for your comments!

> -----Original Message-----

> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> Sent: 2016年11月30日 5:41

> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika

> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian

> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu

> <mingkai.hu@nxp.com>

> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM

> 

> Hi,

> 

> On 27 November 2016 at 22:59, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> > Hi Simon,

> >

> > Thanks for your comments!

> >

> >> -----Original Message-----

> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> >> Sent: 2016年11月28日 1:02

> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> Ruchika

> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.

> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> Mingkai

> >> Hu <mingkai.hu@nxp.com>

> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on

> >> DM

> >>

> >> Hi,

> >>

> >> On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> >> > Hi Simon,

> >> >

> >> > Thanks for your comments!

> >> >

> >> >> -----Original Message-----

> >> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon

> >> >> Glass

> >> >> Sent: 2016年11月24日 10:21

> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> >> Ruchika

> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;

> M.H.

> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> >> Mingkai

> >> >> Hu <mingkai.hu@nxp.com>

> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based

> >> >> on DM

> >> >>

> >> >> Hi,

> >> >>

> >> >> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com>

> wrote:

> >> >> > 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@google.com [mailto:sjg@google.com] On Behalf Of Simon

> >> >> >> Glass

> >> >> >> Sent: 2016年11月18日 9:15

> >> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> >> >> Ruchika

> >> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;

> >> M.H.

> >> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> >> >> Mingkai

> >> >> >> Hu <mingkai.hu@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@nxp.com>

> >> >> >> wrote:

> >> >> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>

> >> >> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

> >> >> >> > ---

> >> >> >> > V3:

> >> >> >> >  - No change

> >> >> >> >

> >> >> >> >  drivers/pci/Kconfig           |   8 +

> >> >> >> >  drivers/pci/pcie_layerscape.c | 761

> >> >> >> > ++++++++++++++++++++++++++++++++++++++++++

> >> >> >> >  2 files changed, 769 insertions(+)

> >> >> >> >

> >> >>

> >> >> >> > +#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.

> >> >>

> >> >> I mean that you cannot have an #ifdef in a driver - it should be

> >> >> done at run-time by looking at the compatible strings.

> >> >

> >> > This driver work for many platforms, but this fixup is only used by

> >> > FSL_LSCH3 SoCs, if check the compatible string at run-time, the

> >> > fixup will be

> >> still compiled for the platform which doesn't need it.

> >> > Why compile it into the binary for the platform which doesn't need it?

> >>

> >> Because that's how it works. Drivers are drivers for their hardware.

> >> We cannot compile them differently depending on who might use them...

> >>

> >> If this is a big problem you could split the driver into multiple

> >> parts perhaps. But what exactly is the problem here?

> >

> > It isn't a big problem, actually it is just kernel DT fixup function, and it doesn't

> affect the u-boot pcie driver.

> > But the fixup is LSCH3 SoC special, and some macros are only defined in

> header file of LSCH3, e.g. FSL_PEX_STREAM_ID_*.

> > So cannot removed the #ifdef CONFIG_FSL_LSCH3.

> 

> Really there should be two separate drivers, with a shared common file for

> common code.

>

> Failing that, is it really impossible to include the extra macros regardless?

> 

> If we start putting board-specific #ifdefs in drivers, we have lost the DM battle.


Is it necessary to separate two drivers just for a fixup function?
The fixup is functionally independent with pcie driver, and it works for kernel pcie driver, if removed the fixup, u-boot pcie driver is still unabridged and works well but kernel pcie driver won't.
The #ifdefs isn't introduced by Minghuan's refactor based on DM, actually this refactor removed many #ifdefs. So we do not lost the DM battle.

> >

> >>

> >> >

> >> >> >

> >> >> >>

> >> >> >> > +/*

> >> >> >> > + * 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.

> >> >>

> >> >> They should use the same DT.

> >> >

> >> > Yes, Ideally they should, but up to now actually Kernel does not

> >> > use the one u-boot used, so we cannot make sure the offset of the

> >> > nodes are the

> >> same.

> >> > So to ensure the fixup work, get the node offset from kernel DT.

> >>

> >> Is it not possible to change U-Boot to use the kernel DT? It might be less

> work.

> >

> > Since this is used to fixup Kernel DT, and u-boot and Kernel use two copies of

> DT, until the u-boot and kernel use one copy of DT, we must fixup the one

> works for Kernel.

> 

> OK. Please add a TODO(email) prominently.


I'm afraid you're confused.
U-boot and kernel use two copies of DT whether they are the same or not, they locate in different addresses, and let's name the u-boot used A and kernel used B.
This function is used to fixup B, so the node-offset must be get from B instead of A. Because we cannot ensure A and B always are the same.

> >

> >>

> >> >

> >> >>

> >> >> >

> >> >> >>

> >> >> >> > +       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.

> >> >>

> >> >> I'm still confused by this. I don't see it defined anywhere and it

> >> >> is not a

> >> CONFIG.

> >> >> Can you not detect at run-time when you need to do the fix-up?

> >> >

> >> > Ok, the process is find the node offset by "fsl,ls-pcie" first, if

> >> > failed, find it

> >> again by FSL_PCIE_COMPAT.

> >> > But in the current kernel DT the name of PCIe controller node is

> >> > NOT the "fsl,ls-pcie" which we will refactor layerscape pcie kernel

> >> > driver to use, so far it is the FSL_PCIE_COMPAT which is defined

> >> > according to the

> >> current kernel DT in header file include/configs/ls*.h.

> >> > So it is unable to be detected at run-time, but it will be removed

> >> > when the

> >> kernel driver refactored.

> >>

> >> OK, so how about making this a new CONFIG which you can turn on/off?

> >

> > Yes, will move it to CONFIG_ FSL_PCIE_COMPAT.

> >

> >> >

> >> >>

> >> >> >

> >> >> >> > +               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.

> >> >>

> >> >> Try:

> >> >>

> >> >>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)

> >> >>    if (val == -1) {

> >> >>       debug(...);

> >> >>       return -EINVAL;

> >> >>    }

> >> >>

> >> >

> >> > Any benefit compared with fdt_getprop? I'm confused by this

> >> > function, what

> >> if the correct value equal to the given default value?

> >>

> >> You choose an invalid default. If there isn't one then you cannot use

> >> this function. The benefit is that it avoids the be32_to_cpu().

> >

> > The value of this property is a reference of other node and don't know which

> is the invalid value.

> > Do you have any suggestion about this case?

> 

> Well, phandles cannot be < 0, so how about -1?


No, it can be < 0.
Made an experiment that added "test = <0xffffffff>;" to DT then the fdtdec_get_int() return -1.
So, avoid to use it when didn't know an invalid 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.

> >> >>

> >> >> If it is an error it should return an error. If it is just a

> >> >> warning it should say so, ideally using debug(). As it is, it is

> >> >> very confusing for the user to get this message.

> >> >

> >> > Will replace with debug().

> >> >

> >> >> >

> >> >> >> > +       }

> >> >> >> > +       phandle = be32_to_cpu(*prop);

> >> >> >>

> >> >> >> fdt32_to_cpu()

> >> >> >>

> >> >> >

> >> >> > Yes, better to use fdt32_to_cpu.

> >> >>

> >> >> But where do you use that value? Also. consider

> fdtdec_lookup_phandle().

> >> >

> >> > Thanks for your tip, just the value of this phandle is used, see the lines

> below.

> >>

> >> OK I see.

> >>

> >> >

> >> >> >

> >> >> >> > +

> >> >> >> > +       /* 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.

> >> >>

> >> >> OK, well please add some comments!

> >> >

> >> > Will comment it.

> >


Regards,
Zhiqiang

Hi,

On 30 November 2016 at 01:14, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
> Hi Simon,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> Sent: 2016年11月30日 5:41
>> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika
>> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian
>> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu
>> <mingkai.hu@nxp.com>
>> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
>>
>> Hi,
>>
>> On 27 November 2016 at 22:59, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>> > Hi Simon,
>> >
>> > Thanks for your comments!
>> >
>> >> -----Original Message-----
>> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass
>> >> Sent: 2016年11月28日 1:02
>> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;
>> Ruchika
>> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.
>> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;
>> Mingkai
>> >> Hu <mingkai.hu@nxp.com>
>> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on
>> >> DM
>> >>
>> >> Hi,
>> >>
>> >> On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>> >> > Hi Simon,
>> >> >
>> >> > Thanks for your comments!
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon
>> >> >> Glass
>> >> >> Sent: 2016年11月24日 10:21
>> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;
>> >> Ruchika
>> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;
>> M.H.
>> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;
>> >> Mingkai
>> >> >> Hu <mingkai.hu@nxp.com>
>> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based
>> >> >> on DM
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com>
>> wrote:
>> >> >> > 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@google.com [mailto:sjg@google.com] On Behalf Of Simon
>> >> >> >> Glass
>> >> >> >> Sent: 2016年11月18日 9:15
>> >> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
>> >> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD
>> >> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha
>> >> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965
>> >> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;
>> >> >> Ruchika
>> >> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain
>> >> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;
>> >> M.H.
>> >> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;
>> >> >> Mingkai
>> >> >> >> Hu <mingkai.hu@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@nxp.com>
>> >> >> >> wrote:
>> >> >> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>
>> >> >> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>> >> >> >> > ---
>> >> >> >> > V3:
>> >> >> >> >  - No change
>> >> >> >> >
>> >> >> >> >  drivers/pci/Kconfig           |   8 +
>> >> >> >> >  drivers/pci/pcie_layerscape.c | 761
>> >> >> >> > ++++++++++++++++++++++++++++++++++++++++++
>> >> >> >> >  2 files changed, 769 insertions(+)
>> >> >> >> >
>> >> >>
>> >> >> >> > +#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.
>> >> >>
>> >> >> I mean that you cannot have an #ifdef in a driver - it should be
>> >> >> done at run-time by looking at the compatible strings.
>> >> >
>> >> > This driver work for many platforms, but this fixup is only used by
>> >> > FSL_LSCH3 SoCs, if check the compatible string at run-time, the
>> >> > fixup will be
>> >> still compiled for the platform which doesn't need it.
>> >> > Why compile it into the binary for the platform which doesn't need it?
>> >>
>> >> Because that's how it works. Drivers are drivers for their hardware.
>> >> We cannot compile them differently depending on who might use them...
>> >>
>> >> If this is a big problem you could split the driver into multiple
>> >> parts perhaps. But what exactly is the problem here?
>> >
>> > It isn't a big problem, actually it is just kernel DT fixup function, and it doesn't
>> affect the u-boot pcie driver.
>> > But the fixup is LSCH3 SoC special, and some macros are only defined in
>> header file of LSCH3, e.g. FSL_PEX_STREAM_ID_*.
>> > So cannot removed the #ifdef CONFIG_FSL_LSCH3.
>>
>> Really there should be two separate drivers, with a shared common file for
>> common code.
>>
>> Failing that, is it really impossible to include the extra macros regardless?
>>
>> If we start putting board-specific #ifdefs in drivers, we have lost the DM battle.
>
> Is it necessary to separate two drivers just for a fixup function?
> The fixup is functionally independent with pcie driver, and it works for kernel pcie driver, if removed the fixup, u-boot pcie driver is still unabridged and works well but kernel pcie driver won't.
> The #ifdefs isn't introduced by Minghuan's refactor based on DM, actually this refactor removed many #ifdefs. So we do not lost the DM battle.

OK so the #ifdef is only used for the fix-up function? In that case
can we move this into a separate file like pcie_layerscape_fixup.c?
Then we don't have #ifdef CONFIGs in the driver. Would that work?

>
>> >
>> >>
>> >> >
>> >> >> >
>> >> >> >>
>> >> >> >> > +/*
>> >> >> >> > + * 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.
>> >> >>
>> >> >> They should use the same DT.
>> >> >
>> >> > Yes, Ideally they should, but up to now actually Kernel does not
>> >> > use the one u-boot used, so we cannot make sure the offset of the
>> >> > nodes are the
>> >> same.
>> >> > So to ensure the fixup work, get the node offset from kernel DT.
>> >>
>> >> Is it not possible to change U-Boot to use the kernel DT? It might be less
>> work.
>> >
>> > Since this is used to fixup Kernel DT, and u-boot and Kernel use two copies of
>> DT, until the u-boot and kernel use one copy of DT, we must fixup the one
>> works for Kernel.
>>
>> OK. Please add a TODO(email) prominently.
>
> I'm afraid you're confused.
> U-boot and kernel use two copies of DT whether they are the same or not, they locate in different addresses, and let's name the u-boot used A and kernel used B.
> This function is used to fixup B, so the node-offset must be get from B instead of A. Because we cannot ensure A and B always are the same.

OK I think I am slowly understanding this. So you have a kernel DT
fix-up function that you are putting in this driver. The only calls
into drivers should be via driver model. As above I suggest putting
this in its own file.

>
>> >
>> >>
>> >> >
>> >> >>
>> >> >> >
>> >> >> >>
>> >> >> >> > +       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.
>> >> >>
>> >> >> I'm still confused by this. I don't see it defined anywhere and it
>> >> >> is not a
>> >> CONFIG.
>> >> >> Can you not detect at run-time when you need to do the fix-up?
>> >> >
>> >> > Ok, the process is find the node offset by "fsl,ls-pcie" first, if
>> >> > failed, find it
>> >> again by FSL_PCIE_COMPAT.
>> >> > But in the current kernel DT the name of PCIe controller node is
>> >> > NOT the "fsl,ls-pcie" which we will refactor layerscape pcie kernel
>> >> > driver to use, so far it is the FSL_PCIE_COMPAT which is defined
>> >> > according to the
>> >> current kernel DT in header file include/configs/ls*.h.
>> >> > So it is unable to be detected at run-time, but it will be removed
>> >> > when the
>> >> kernel driver refactored.
>> >>
>> >> OK, so how about making this a new CONFIG which you can turn on/off?
>> >
>> > Yes, will move it to CONFIG_ FSL_PCIE_COMPAT.
>> >
>> >> >
>> >> >>
>> >> >> >
>> >> >> >> > +               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.
>> >> >>
>> >> >> Try:
>> >> >>
>> >> >>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)
>> >> >>    if (val == -1) {
>> >> >>       debug(...);
>> >> >>       return -EINVAL;
>> >> >>    }
>> >> >>
>> >> >
>> >> > Any benefit compared with fdt_getprop? I'm confused by this
>> >> > function, what
>> >> if the correct value equal to the given default value?
>> >>
>> >> You choose an invalid default. If there isn't one then you cannot use
>> >> this function. The benefit is that it avoids the be32_to_cpu().
>> >
>> > The value of this property is a reference of other node and don't know which
>> is the invalid value.
>> > Do you have any suggestion about this case?
>>
>> Well, phandles cannot be < 0, so how about -1?
>
> No, it can be < 0.
> Made an experiment that added "test = <0xffffffff>;" to DT then the fdtdec_get_int() return -1.
> So, avoid to use it when didn't know an invalid value.

Yes but a phandle will never be -1. My point is that if the phandle is
missing, the function will return -1, so you can detect that case. All
valid phandles are >= 0. Anyway if you like the code as is, it's fine
with me. It just seems unnecessarily complicated.

>
>> >
>> >> >
>> >> >> >
>> >> >> >>
>> >> >> >> > +       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.
>> >> >>
>> >> >> If it is an error it should return an error. If it is just a
>> >> >> warning it should say so, ideally using debug(). As it is, it is
>> >> >> very confusing for the user to get this message.
>> >> >
>> >> > Will replace with debug().
>> >> >
>> >> >> >
>> >> >> >> > +       }
>> >> >> >> > +       phandle = be32_to_cpu(*prop);
>> >> >> >>
>> >> >> >> fdt32_to_cpu()
>> >> >> >>
>> >> >> >
>> >> >> > Yes, better to use fdt32_to_cpu.
>> >> >>
>> >> >> But where do you use that value? Also. consider
>> fdtdec_lookup_phandle().
>> >> >
>> >> > Thanks for your tip, just the value of this phandle is used, see the lines
>> below.
>> >>
>> >> OK I see.
>> >>
>> >> >
>> >> >> >
>> >> >> >> > +
>> >> >> >> > +       /* 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.
>> >> >>
>> >> >> OK, well please add some comments!
>> >> >
>> >> > Will comment it.
>> >
>
> Regards,
> Zhiqiang

Hi Simon,

Thanks for your comments!

> -----Original Message-----

> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> Sent: 2016年12月1日 10:20

> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>; Ruchika

> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H. Lian

> <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>; Mingkai Hu

> <mingkai.hu@nxp.com>

> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM

> 

> Hi,

> 

> On 30 November 2016 at 01:14, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> > Hi Simon,

> >

> > Thanks for your comments!

> >

> >> -----Original Message-----

> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass

> >> Sent: 2016年11月30日 5:41

> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> Ruchika

> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>; M.H.

> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> Mingkai

> >> Hu <mingkai.hu@nxp.com>

> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on

> >> DM

> >>

> >> Hi,

> >>

> >> On 27 November 2016 at 22:59, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:

> >> > Hi Simon,

> >> >

> >> > Thanks for your comments!

> >> >

> >> >> -----Original Message-----

> >> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon

> >> >> Glass

> >> >> Sent: 2016年11月28日 1:02

> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> >> Ruchika

> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;

> M.H.

> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> >> Mingkai

> >> >> Hu <mingkai.hu@nxp.com>

> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based

> >> >> on DM

> >> >>

> >> >> Hi,

> >> >>

> >> >> On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang.hou@nxp.com>

> wrote:

> >> >> > Hi Simon,

> >> >> >

> >> >> > Thanks for your comments!

> >> >> >

> >> >> >> -----Original Message-----

> >> >> >> From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon

> >> >> >> Glass

> >> >> >> Sent: 2016年11月24日 10:21

> >> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD

> >> >> >> <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> >> >> <alison.wang@freescale.com>; Sumit Garg <sumit.garg@nxp.com>;

> >> >> Ruchika

> >> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> >> >> <saksham.jain@nxp.freescale.com>; york sun <york.sun@nxp.com>;

> >> M.H.

> >> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng <bmeng.cn@gmail.com>;

> >> >> Mingkai

> >> >> >> Hu <mingkai.hu@nxp.com>

> >> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver

> >> >> >> based on DM

> >> >> >>

> >> >> >> Hi,

> >> >> >>

> >> >> >> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> wrote:

> >> >> >> > 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@google.com [mailto:sjg@google.com] On Behalf Of

> >> >> >> >> Simon Glass

> >> >> >> >> Sent: 2016年11月18日 9:15

> >> >> >> >> To: Z.Q. Hou <zhiqiang.hou@nxp.com>

> >> >> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert

> >> >> >> >> ARIBAUD <albert.u.boot@aribaud.net>; Prabhakar Kushwaha

> >> >> >> >> <prabhakar.kushwaha@nxp.com>; Huan Wang-B18965

> >> >> >> >> <alison.wang@freescale.com>; Sumit Garg

> >> >> >> >> <sumit.garg@nxp.com>;

> >> >> >> Ruchika

> >> >> >> >> Gupta <ruchika.gupta@nxp.com>; Saksham Jain

> >> >> >> >> <saksham.jain@nxp.freescale.com>; york sun

> >> >> >> >> <york.sun@nxp.com>;

> >> >> M.H.

> >> >> >> >> Lian <minghuan.lian@nxp.com>; Bin Meng

> <bmeng.cn@gmail.com>;

> >> >> >> Mingkai

> >> >> >> >> Hu <mingkai.hu@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@nxp.com>

> >> >> >> >> wrote:

> >> >> >> >> > From: Minghuan Lian <Minghuan.Lian@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@nxp.com>

> >> >> >> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

> >> >> >> >> > ---

> >> >> >> >> > V3:

> >> >> >> >> >  - No change

> >> >> >> >> >

> >> >> >> >> >  drivers/pci/Kconfig           |   8 +

> >> >> >> >> >  drivers/pci/pcie_layerscape.c | 761

> >> >> >> >> > ++++++++++++++++++++++++++++++++++++++++++

> >> >> >> >> >  2 files changed, 769 insertions(+)

> >> >> >> >> >

> >> >> >>

> >> >> >> >> > +#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.

> >> >> >>

> >> >> >> I mean that you cannot have an #ifdef in a driver - it should

> >> >> >> be done at run-time by looking at the compatible strings.

> >> >> >

> >> >> > This driver work for many platforms, but this fixup is only used

> >> >> > by

> >> >> > FSL_LSCH3 SoCs, if check the compatible string at run-time, the

> >> >> > fixup will be

> >> >> still compiled for the platform which doesn't need it.

> >> >> > Why compile it into the binary for the platform which doesn't need it?

> >> >>

> >> >> Because that's how it works. Drivers are drivers for their hardware.

> >> >> We cannot compile them differently depending on who might use

> them...

> >> >>

> >> >> If this is a big problem you could split the driver into multiple

> >> >> parts perhaps. But what exactly is the problem here?

> >> >

> >> > It isn't a big problem, actually it is just kernel DT fixup

> >> > function, and it doesn't

> >> affect the u-boot pcie driver.

> >> > But the fixup is LSCH3 SoC special, and some macros are only

> >> > defined in

> >> header file of LSCH3, e.g. FSL_PEX_STREAM_ID_*.

> >> > So cannot removed the #ifdef CONFIG_FSL_LSCH3.

> >>

> >> Really there should be two separate drivers, with a shared common

> >> file for common code.

> >>

> >> Failing that, is it really impossible to include the extra macros regardless?

> >>

> >> If we start putting board-specific #ifdefs in drivers, we have lost the DM

> battle.

> >

> > Is it necessary to separate two drivers just for a fixup function?

> > The fixup is functionally independent with pcie driver, and it works for kernel

> pcie driver, if removed the fixup, u-boot pcie driver is still unabridged and

> works well but kernel pcie driver won't.

> > The #ifdefs isn't introduced by Minghuan's refactor based on DM, actually

> this refactor removed many #ifdefs. So we do not lost the DM battle.

> 

> OK so the #ifdef is only used for the fix-up function? In that case can we move

> this into a separate file like pcie_layerscape_fixup.c?

> Then we don't have #ifdef CONFIGs in the driver. Would that work?


Ok, will add a new file for fixup.

> 

> >

> >> >

> >> >>

> >> >> >

> >> >> >> >

> >> >> >> >>

> >> >> >> >> > +/*

> >> >> >> >> > + * 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.

> >> >> >>

> >> >> >> They should use the same DT.

> >> >> >

> >> >> > Yes, Ideally they should, but up to now actually Kernel does not

> >> >> > use the one u-boot used, so we cannot make sure the offset of

> >> >> > the nodes are the

> >> >> same.

> >> >> > So to ensure the fixup work, get the node offset from kernel DT.

> >> >>

> >> >> Is it not possible to change U-Boot to use the kernel DT? It might

> >> >> be less

> >> work.

> >> >

> >> > Since this is used to fixup Kernel DT, and u-boot and Kernel use

> >> > two copies of

> >> DT, until the u-boot and kernel use one copy of DT, we must fixup the

> >> one works for Kernel.

> >>

> >> OK. Please add a TODO(email) prominently.

> >

> > I'm afraid you're confused.

> > U-boot and kernel use two copies of DT whether they are the same or not,

> they locate in different addresses, and let's name the u-boot used A and kernel

> used B.

> > This function is used to fixup B, so the node-offset must be get from B

> instead of A. Because we cannot ensure A and B always are the same.

> 

> OK I think I am slowly understanding this. So you have a kernel DT fix-up

> function that you are putting in this driver. The only calls into drivers should be

> via driver model. As above I suggest putting this in its own file.

> 

> >

> >> >

> >> >>

> >> >> >

> >> >> >>

> >> >> >> >

> >> >> >> >>

> >> >> >> >> > +       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.

> >> >> >>

> >> >> >> I'm still confused by this. I don't see it defined anywhere and

> >> >> >> it is not a

> >> >> CONFIG.

> >> >> >> Can you not detect at run-time when you need to do the fix-up?

> >> >> >

> >> >> > Ok, the process is find the node offset by "fsl,ls-pcie" first,

> >> >> > if failed, find it

> >> >> again by FSL_PCIE_COMPAT.

> >> >> > But in the current kernel DT the name of PCIe controller node is

> >> >> > NOT the "fsl,ls-pcie" which we will refactor layerscape pcie

> >> >> > kernel driver to use, so far it is the FSL_PCIE_COMPAT which is

> >> >> > defined according to the

> >> >> current kernel DT in header file include/configs/ls*.h.

> >> >> > So it is unable to be detected at run-time, but it will be

> >> >> > removed when the

> >> >> kernel driver refactored.

> >> >>

> >> >> OK, so how about making this a new CONFIG which you can turn on/off?

> >> >

> >> > Yes, will move it to CONFIG_ FSL_PCIE_COMPAT.

> >> >

> >> >> >

> >> >> >>

> >> >> >> >

> >> >> >> >> > +               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.

> >> >> >>

> >> >> >> Try:

> >> >> >>

> >> >> >>    val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1)

> >> >> >>    if (val == -1) {

> >> >> >>       debug(...);

> >> >> >>       return -EINVAL;

> >> >> >>    }

> >> >> >>

> >> >> >

> >> >> > Any benefit compared with fdt_getprop? I'm confused by this

> >> >> > function, what

> >> >> if the correct value equal to the given default value?

> >> >>

> >> >> You choose an invalid default. If there isn't one then you cannot

> >> >> use this function. The benefit is that it avoids the be32_to_cpu().

> >> >

> >> > The value of this property is a reference of other node and don't

> >> > know which

> >> is the invalid value.

> >> > Do you have any suggestion about this case?

> >>

> >> Well, phandles cannot be < 0, so how about -1?

> >

> > No, it can be < 0.

> > Made an experiment that added "test = <0xffffffff>;" to DT then the

> fdtdec_get_int() return -1.

> > So, avoid to use it when didn't know an invalid value.

> 

> Yes but a phandle will never be -1. My point is that if the phandle is missing, the

> function will return -1, so you can detect that case. All valid phandles are >= 0.

> Anyway if you like the code as is, it's fine with me. It just seems unnecessarily

> complicated.


Ok, thanks for your tips.

> 

> >

> >> >

> >> >> >

> >> >> >> >

> >> >> >> >>

> >> >> >> >> > +       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.

> >> >> >>

> >> >> >> If it is an error it should return an error. If it is just a

> >> >> >> warning it should say so, ideally using debug(). As it is, it

> >> >> >> is very confusing for the user to get this message.

> >> >> >

> >> >> > Will replace with debug().

> >> >> >

> >> >> >> >

> >> >> >> >> > +       }

> >> >> >> >> > +       phandle = be32_to_cpu(*prop);

> >> >> >> >>

> >> >> >> >> fdt32_to_cpu()

> >> >> >> >>

> >> >> >> >

> >> >> >> > Yes, better to use fdt32_to_cpu.

> >> >> >>

> >> >> >> But where do you use that value? Also. consider

> >> fdtdec_lookup_phandle().

> >> >> >

> >> >> > Thanks for your tip, just the value of this phandle is used, see

> >> >> > the lines

> >> below.

> >> >>

> >> >> OK I see.

> >> >>

> >> >> >

> >> >> >> >

> >> >> >> >> > +

> >> >> >> >> > +       /* 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.

> >> >> >>

> >> >> >> OK, well please add some comments!

> >> >> >

> >> >> > Will comment it.

> >> >

> >


Regards,
Zhiqiang