Message-ID: <20150918203736.GK25767@google.com>
Date:	Fri, 18 Sep 2015 15:37:36 -0500
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Bharat Kumar Gogada <bharat.kumar.gogada@...inx.com>
Cc:	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	michal.simek@...inx.com, soren.brinkmann@...inx.com, arnd@...db.de,
	tinamdar@....com, treding@...dia.com, rjui@...adcom.com,
	Minghuan.Lian@...escale.com, m-karicheri2@...com, hauke@...ke-m.de,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	Bharat Kumar Gogada <bharatku@...inx.com>,
	Ravi Kiran Gummaluri <rgummal@...inx.com>
Subject: Re: [PATCH] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe
 Host Controller

Hi Bharat,

On Thu, Aug 27, 2015 at 05:14:20PM +0530, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.

> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..0f3a789 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> +obj-$(CONFIG_PCI_XILINX_NWL) += pci-xilinx-nwl.o

The other Xilinx driver uses "CONFIG_PCIE_*" and "pcie-xilinx.o".  Please
use "pcie" similarly for this driver.

> +/**
> + * struct nwl_msi - MSI information
> + *
> + * @chip: MSI controller
> + * @used: Declare Bitmap for MSI
> + * @domain: IRQ domain pointer
> + * @pages: MSI pages
> + * @lock: mutex lock
> + * @irq_msi0: msi0 interrupt number
> + * @irq_msi1: msi1 interrupt number

Please put these short comments inside the struct definition, on the same
line as the element they describe.  It's needlessly repetitive to do it
separately.

> + */
> +struct nwl_msi {
> +	struct msi_controller chip;
> +	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> +	struct irq_domain *domain;
> +	unsigned long pages;
> +	struct mutex lock;
> +	int irq_msi0;
> +	int irq_msi1;
> +};

> +static inline bool nwl_pcie_is_link_up(struct nwl_pcie *pcie, u32 check_bit)

Please name this "nwl_pcie_link_up" so it's consistent with most others
(xilinx_pcie_link_is_up() is an exception).  I'm not sure the "check_bit"
argument is really an improvement, since it makes the code a bit ugly and
more different from other drivers than it would otherwise be.

> +{
> +	unsigned int status = -EINVAL;
> +
> +	if (check_bit == PCIE_USER_LINKUP)
> +		status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +			  PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> +	else if (check_bit == PHY_RDY_LINKUP)
> +		status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +			  PHY_RDY_LINKUP_BIT) ? 1 : 0;
> +	return status;
> +}

> +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> +{
> +	unsigned int status;
> +	int check = 0;
> +
> +	do {
> +		status = nwl_bridge_readl(pcie, TX_PCIE_MSG) & MSG_BUSY_BIT;
> +		if (!status) {
> +			/*
> +			 * Generate the TLP message for a single EP
> +			 * [TODO] Add a multi-endpoint code
> +			 */
> +			nwl_bridge_writel(pcie, 0x0,
> +					  TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> +			nwl_bridge_writel(pcie, 0x0,
> +					  TX_PCIE_MSG + TX_PCIE_MSG_SPEC_LO);
> +			nwl_bridge_writel(pcie, 0x0,
> +					  TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> +			nwl_bridge_writel(pcie, 0x0,
> +					  TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> +			/* Pattern to generate SSLP TLP */
> +			nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> +					  TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> +			nwl_bridge_writel(pcie, RANDOM_DIGIT,
> +					  TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> +			nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> +					  TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> +			status = 0;
> +			do {
> +				status = nwl_bridge_readl(pcie, TX_PCIE_MSG) &
> +							  MSG_DONE_BIT;
> +				if (!status && (check < 1)) {
> +					mdelay(1);
> +					check++;
> +				} else {
> +					return false;
> +				}
> +
> +			} while (!status);
> +			status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +						  & MSG_DONE_STATUS_BIT;
> +		}
> +	} while (status);
> +
> +	return true;

This function is defined to return "int."  It should not return "true" or
"false."

I'm really confused about what this function does.  I can see it does
at least:

  - wait until MSG_BUSY_BIT is clear
  - write a bunch of registers
  - some magic handshake involving MSG_DONE_BIT and
    MSG_DONE_STATUS_BIT (I really don't understand this)
  - possibly repeat if MSG_DONE_STATUS_BIT is set?

Can you clean this up and straighten it out somehow?  My head hurts
from trying to understand it.

> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> +				unsigned int devfn,
> +				int where,
> +				int size,
> +				u32 val)
> +{
> +	void __iomem *addr;
> +	int err = 0;
> +	struct nwl_pcie *pcie = bus->sysdata;
> +
> +	if (!bus->number && devfn > 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = nwl_pcie_get_config_base(bus, devfn, where);
> +
> +	switch (size) {
> +	case 1:
> +		writeb(val, addr);
> +		break;
> +	case 2:
> +		writew(val, addr);
> +		break;
> +	default:
> +		writel(val, addr);
> +		break;
> +	}
> +	if (addr == (pcie->ecam_base + PCI_EXP_SLTCAP)) {

This only intercepts writes to bus 0.  I assume that's what you want, and
that's fine, but using pcie->ecam_base here is totally non-obvious.  If you
want to check for bus 0, use "bus->number == 0".

I think offset PCI_EXP_SLTCAP is wrong.  That's the offset of SLTCAP from
the beginning of the PCIe capability, not the address in the device's
config space.  You're intercepting writes 20 (0x14), which is BAR1.

> +		err = nwl_setup_sspl(pcie);
> +		if (!err)
> +			return PCIBIOS_SET_FAILED;

This doesn't make sense to me.  First, "err" doesn't need to be
initialized above.  Second, this basically reads "if no error, return
failure."

> +static void __nwl_pcie_msi_handler(struct nwl_pcie *pcie,
> +					unsigned long reg, u32 val)
> +{
> +	struct nwl_msi *msi = &pcie->msi;
> +	unsigned int offset, index;
> +	int irq_msi;
> +
> +	offset = find_first_bit(&reg, 32);
> +	index = offset;
> +
> +	/* Clear the interrupt */
> +	nwl_bridge_writel(pcie, 1 << offset, val);
> +
> +	/* Handle the msi virtual interrupt */
> +	irq_msi = irq_find_mapping(msi->domain, index);
> +	if (irq_msi) {
> +		if (test_bit(index, msi->used))
> +			generic_handle_irq(irq_msi);
> +		else
> +			dev_info(pcie->dev, "unhandled MSI\n");
> +	} else {
> +		/* that's weird who triggered this? just clear it */
> +		dev_info(pcie->dev, "unexpected MSI\n");

The comment says "just clear it," but there's nothing in this block
that actually clears it.  Maybe you meant "we cleared it above; just
note it"?

> +static int nwl_msi_setup_irq(struct msi_controller *chip, struct pci_dev *pdev,
> +			       struct msi_desc *desc)
> +{
> +	struct nwl_msi *msi = to_nwl_msi(chip);
> +	struct msi_msg msg;
> +	unsigned int irq;
> +	int hwirq;
> +
> +	if (desc->msi_attrib.is_msix) {
> +		/* currently we are not supporting MSIx */
> +		return -ENOSPC;
> +	}

Remove the braces here so it's the same style as below
(single-statement blocks don't need braces).  You can move the comment
above the "if" to make it more obvious that it's a single-statement
block.

> +
> +	hwirq = nwl_msi_alloc(msi);
> +	if (hwirq < 0)
> +		return hwirq;
> +
> +	irq = irq_create_mapping(msi->domain, hwirq);
> +	if (!irq)
> +		return -EINVAL;

> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus *bus)
> +{
> +	struct platform_device *pdev = to_platform_device(pcie->dev);
> +	struct nwl_msi *msi = &pcie->msi;
> +	unsigned long base;
> +	int ret;
> +
> +	mutex_init(&msi->lock);
> +
> +	/* Assign msi chip hooks */
> +	msi->chip.dev = pcie->dev;
> +	msi->chip.setup_irq = nwl_msi_setup_irq;
> +	msi->chip.teardown_irq = nwl_msi_teardown_irq;
> +
> +	bus->msi = &msi->chip;
> +	/* Allocate linear irq domain */
> +	msi->domain = irq_domain_add_linear(pcie->dev->of_node, INT_PCI_MSI_NR,
> +					    &msi_domain_ops, &msi->chip);
> +	if (!msi->domain) {
> +		dev_err(&pdev->dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Check for msii_present bit */
> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) & MSII_PRESENT;
> +	if (!ret) {
> +		dev_err(pcie->dev, "MSI not present\n");
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	/* Enable MSII */
> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> +			  MSII_ENABLE, I_MSII_CONTROL);
> +	if (!pcie->enable_msi_fifo)
> +		/* Enable MSII status */
> +		nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> +				  MSII_STATUS_ENABLE, I_MSII_CONTROL);
> +
> +	/* setup AFI/FPCI range */
> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> +	base = virt_to_phys((void *)msi->pages);
> +	/* Write base to MSII_BASE_LO */
> +	nwl_bridge_writel(pcie, base, I_MSII_BASE_LO);
> +
> +	/* Write 0x0 to MSII_BASE_HI */

These comments ("Allocate linear irq domain," "write xx to yy," etc.)
are really just repetitive and you might as well remove them because
the code says the same thing.  Sometimes it's not obvious what the
code is doing, and then a comment is worthwhile.

> +static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> +{
> +	struct platform_device *pdev = to_platform_device(pcie->dev);
> +	u32 breg_val, ecam_val, first_busno = 0;
> +	int err;
> +	int check_link_up = 0;
> +
> +	/* Check for BREG present bit */
> +	breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
> +	if (!breg_val) {
> +		dev_err(pcie->dev, "BREG is not present\n");
> +		return breg_val;
> +	}
> +	/* Write bridge_off to breg base */
> +	nwl_bridge_writel(pcie, (u32)(pcie->phys_breg_base),
> +			  E_BREG_BASE_LO);
> +
> +	/* Enable BREG */
> +	nwl_bridge_writel(pcie, ~BREG_ENABLE_FORCE & BREG_ENABLE,
> +			  E_BREG_CONTROL);
> +
> +	/* Disable DMA channel registers */
> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_PCIE_RX0) |
> +			  CFG_DMA_REG_BAR, BRCFG_PCIE_RX0);
> +
> +	/* Enable the bridge config interrupt */
> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_INTERRUPT) |
> +			  BRCFG_INTERRUPT_MASK, BRCFG_INTERRUPT);
> +	/* Enable Ingress subtractive decode translation */
> +	nwl_bridge_writel(pcie, SET_ISUB_CONTROL, I_ISUB_CONTROL);
> +
> +	/* Enable msg filtering details */
> +	nwl_bridge_writel(pcie, CFG_ENABLE_MSG_FILTER_MASK,
> +			  BRCFG_PCIE_RX_MSG_FILTER);
> +	do {
> +		err = nwl_pcie_is_link_up(pcie, PHY_RDY_LINKUP);
> +		if (err != 1) {
> +			check_link_up++;
> +			if (check_link_up > LINKUP_ITER_CHECK)
> +				return -ENODEV;
> +		}
> +	} while (!err);

Most drivers use usleep_range() or mdelay() in this kind of loop.  This one
seems like it's hardly worth looping -- only 5 iterations with no delay at
all?  Maybe you could even look at the similar loops in other drivers
and copy the style of the loop.

> +static int nwl_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct nwl_pcie *pcie;
> +	struct pci_bus *bus;
> +	int err;
> +
> +	resource_size_t iobase = 0;
> +	LIST_HEAD(res);
> +
> +	/* Allocate private nwl_pcie struct */

More comments that really aren't necessary.  "Allocate," "set ecam
value," "parse device tree," these are all just what the function
names already tell us.

> +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	/* Set ecam value */
> +	pcie->ecam_value = NWL_ECAM_VALUE_DEFAULT;
> +
> +	pcie->dev = &pdev->dev;
> +
> +	/* Parse the device tree */
> +	err = nwl_pcie_parse_dt(pcie, pdev);
> +	if (err) {
> +		dev_err(pcie->dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +	/* Bridge initialization */
> +	err = nwl_pcie_bridge_init(pcie);
> +	if (err) {
> +		dev_err(pcie->dev, "HW Initalization failed\n");
> +		return err;
> +	}
> +
> +	err = of_pci_get_host_bridge_resources(node, 0, 0xff, &res, &iobase);
> +	if (err) {
> +		pr_err("Getting bridge resources failed\n");
> +		return err;
> +	}
> +	bus = pci_create_root_bus(&pdev->dev, 0,
> +				  &nwl_pcie_ops, pcie, &res);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	/* Enable MSI */
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		err = nwl_pcie_enable_msi(pcie, bus);
> +		if (err < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to enable MSI support: %d\n",
> +				err);
> +			return err;
> +		}
> +	}
> +	pci_scan_child_bus(bus);
> +	pci_assign_unassigned_bus_resources(bus);
> +	pci_bus_add_devices(bus);
> +	platform_set_drvdata(pdev, pcie);
> +
> +	return 0;
> +}

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists