Hi Bharat,
I'm resending this since you sent a ping three days after I responded,
so I don't know whether you got this the first time around.
DMARC got turned on for mail coming from @google.com, which apparently
caused some email providers to drop it. I switched to sending from
helgaas@xxxxxxxxxx to avoid that problem.
Bjorn
On Fri, Sep 18, 2015 at 03:37:36PM -0500, Bjorn Helgaas wrote:
> 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(®, 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-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
