On Mon, Mar 11, 2019 at 09:33:16AM +0000, Z.q. Hou wrote:

"Layerscape"

It would be nice to make "NXP Layerscape SoCs" a little more specific
so we can tell how it's different from the existing pci-layerscape.c
that says "Freescale Layerscape SoCs".  I assume this driver only
works with gen4 parts.

+ * Copyright 2018 NXP

s/2018/2019/

+ *
+ * Author: Zhiqiang Hou [off-list ref]
+ */
+
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#include "pcie-mobiveil.h"
+
+/* LUT and PF control registers */
+#define PCIE_LUT_OFF			(0x80000)
+#define PCIE_PF_OFF			(0xc0000)
+#define PCIE_PF_INT_STAT		(0x18)
+#define PF_INT_STAT_PABRST		(31)

You mix constants that are

  - the bit position (like this), and
  - the actual bit mask, like PAB_INTP_RESET

Pick one and use a single style.  My personal preference is a plain
hex bit mask, e.g.,

  #define PF_INT_STAT_PABRST    0x80000000
  #define PAB_INTP_RESET        0x00000002

But some people like BIT() or the style you used for PAB_INTP_RESET:

  #define PAB_INTP_RESET        BIT(1)
  #define PAB_INTP_RESET        (0x1 << 1)

I definitely don't like the simple bit position like this:

  #define PF_INT_STAT_PABRST    31

because that means you have to repeat things like
"1 << PF_INT_STAT_PABRST" everywhere you use it.

+#define PCIE_PF_DBG			(0x7fc)
+#define PF_DBG_LTSSM_MASK		(0x3f)
+#define PF_DBG_WE			(31)
+#define PF_DBG_PABR			(27)

Parens are not needed around single constants like these.

+#define LS_PCIE_G4_LTSSM_L0		0x2d /* L0 state */
+
+#define to_ls_pcie_g4(x)		platform_get_drvdata((x)->pdev)
+
+struct ls_pcie_g4 {
+	struct mobiveil_pcie *pci;
+	struct delayed_work dwork;
+	int irq;
+};
+
+static inline u32 ls_pcie_g4_lut_readl(struct ls_pcie_g4 *pcie, u32 off)
+{
+	return ioread32(pcie->pci->csr_axi_slave_base + PCIE_LUT_OFF + off);
+}
+
+static inline void ls_pcie_g4_lut_writel(struct ls_pcie_g4 *pcie,
+					 u32 off, u32 val)
+{
+	iowrite32(val, pcie->pci->csr_axi_slave_base + PCIE_LUT_OFF + off);
+}
+
+static inline u32 ls_pcie_g4_pf_readl(struct ls_pcie_g4 *pcie, u32 off)
+{
+	return ioread32(pcie->pci->csr_axi_slave_base + PCIE_PF_OFF + off);
+}
+
+static inline void ls_pcie_g4_pf_writel(struct ls_pcie_g4 *pcie,
+					u32 off, u32 val)
+{
+	iowrite32(val, pcie->pci->csr_axi_slave_base + PCIE_PF_OFF + off);
+}
+
+static bool ls_pcie_g4_is_bridge(struct ls_pcie_g4 *pcie)
+{
+	struct mobiveil_pcie *mv_pci = pcie->pci;
+	u32 header_type;
+
+	header_type = csr_readb(mv_pci, PCI_HEADER_TYPE);
+	header_type &= 0x7f;
+
+	return header_type == PCI_HEADER_TYPE_BRIDGE;
+}
+
+static int ls_pcie_g4_link_up(struct mobiveil_pcie *pci)
+{
+	struct ls_pcie_g4 *pcie = to_ls_pcie_g4(pci);
+	u32 state;
+
+	state = ls_pcie_g4_pf_readl(pcie, PCIE_PF_DBG);
+	state =	state & PF_DBG_LTSSM_MASK;
+
+	if (state == LS_PCIE_G4_LTSSM_L0)
+		return 1;
+
+	return 0;
+}
+
+static void ls_pcie_g4_reinit_hw(struct ls_pcie_g4 *pcie)
+{
+	struct mobiveil_pcie *mv_pci = pcie->pci;
+	u32 val, act_stat;
+	int to = 100;
+
+	/* Poll for pab_csb_reset to set and PAB activity to clear */
+	do {
+		usleep_range(10, 15);
+		val = ls_pcie_g4_pf_readl(pcie, PCIE_PF_INT_STAT);
+		act_stat = csr_readl(mv_pci, PAB_ACTIVITY_STAT);
+	} while (((val & 1 << PF_INT_STAT_PABRST) == 0 || act_stat) && to--);
+	if (to < 0) {
+		dev_err(&mv_pci->pdev->dev, "poll PABRST&PABACT timeout\n");
+		return;
+	}
+
+	/* clear PEX_RESET bit in PEX_PF0_DBG register */
+	val = ls_pcie_g4_pf_readl(pcie, PCIE_PF_DBG);
+	val |= 1 << PF_DBG_WE;
+	ls_pcie_g4_pf_writel(pcie, PCIE_PF_DBG, val);
+
+	val = ls_pcie_g4_pf_readl(pcie, PCIE_PF_DBG);
+	val |= 1 << PF_DBG_PABR;
+	ls_pcie_g4_pf_writel(pcie, PCIE_PF_DBG, val);
+
+	val = ls_pcie_g4_pf_readl(pcie, PCIE_PF_DBG);
+	val &= ~(1 << PF_DBG_WE);
+	ls_pcie_g4_pf_writel(pcie, PCIE_PF_DBG, val);
+
+	mobiveil_host_init(mv_pci, true);
+
+	to = 100;
+	while (!ls_pcie_g4_link_up(mv_pci) && to--)
+		usleep_range(200, 250);
+	if (to < 0)
+		dev_err(&mv_pci->pdev->dev, "PCIe link trainning timeout\n");

s/trainning/training/

+}
+
+static irqreturn_t ls_pcie_g4_handler(int irq, void *dev_id)

Function name should include "irq_handler" or "isr".  Run

  git grep request_irq drivers/pci/controller/

and follow the existing conventions.

+{
+	struct ls_pcie_g4 *pcie = (struct ls_pcie_g4 *)dev_id;
+	struct mobiveil_pcie *mv_pci = pcie->pci;
+	u32 val;
+
+	val = csr_readl(mv_pci, PAB_INTP_AMBA_MISC_STAT);
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & PAB_INTP_RESET)
+		schedule_delayed_work(&pcie->dwork, msecs_to_jiffies(1));
+
+	csr_writel(mv_pci, val, PAB_INTP_AMBA_MISC_STAT);
+
+	return IRQ_HANDLED;
+}
+
+static int ls_pcie_g4_interrupt_init(struct mobiveil_pcie *mv_pci)
+{
+	struct ls_pcie_g4 *pcie = to_ls_pcie_g4(mv_pci);
+	u32 val;
+	int ret;
+
+	pcie->irq = platform_get_irq_byname(mv_pci->pdev, "intr");
+	if (pcie->irq < 0) {
+		dev_err(&mv_pci->pdev->dev, "Can't get 'intr' irq.\n");

When you use &mv_pci->pdev->dev more than once, as you do here, please
add

  struct device *dev = &mv_pci->pdev->dev;

and use "dev" instead of repeating the whole "&mv_pci->pdev->dev"
expression.

s/irq./IRQ/ (Capitalize IRQ and omit period)

+		return pcie->irq;
+	}
+	ret = devm_request_irq(&mv_pci->pdev->dev, pcie->irq,
+			       ls_pcie_g4_handler, IRQF_SHARED,
+			       mv_pci->pdev->name, pcie);
+	if (ret) {
+		dev_err(&mv_pci->pdev->dev, "Can't register PCIe IRQ.\n");

Omit period from message and maybe include the IRQ number.

All your messages should start with lower-case or all should start
with upper-case (the two in this function are capitalized, but the
others are not).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel