[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230906171211.GA230112@bhelgaas>
Date: Wed, 6 Sep 2023 12:12:11 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: sharath.kumar.d.m@...el.com
Cc: lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
bhelgaas@...gle.com, linux-pci@...r.kernel.org,
dinguyen@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] PCI: altera: add suport for Agilex Family FPGA
Capitalize subject line similarly.
s/suport/support/
On Wed, Sep 06, 2023 at 04:39:18PM +0530, sharath.kumar.d.m@...el.com wrote:
> From: D M Sharath Kumar <sharath.kumar.d.m@...el.com>
Needs a commit log. It's ok to repeat the subject line.
> +#define AGLX_BDF_REG 0x00002004
> +#define AGLX_ROOT_PORT_IRQ_STATUS 0x14c
> +#define AGLX_ROOT_PORT_IRQ_ENABLE 0x150
> +#define CFG_AER (1<<4)
This seems to be AGLX-specific so maybe should have a prefix?
> +static u32 port_conf_off;
port_conf_off looks like something that should be per-controller.
> +static int aglx_rp_read_cfg(struct altera_pcie *pcie, u8 busno, u32 devfn,
> + int where, int size, u32 *value)
> +{
> + void __iomem *addr = AGLX_RP_CFG_ADDR(pcie, where);
> +
> + switch (size) {
> + case 1:
> + *value = readb(addr);
> + break;
> + case 2:
> + *value = readw(addr);
> + break;
> + default:
> + *value = readl(addr);
> + break;
> + }
> +
> + /* interrupt pin not programmed in hardware
> + */
Use single-line comment style:
/* interrupt pin not programmed in hardware */
> + if (where == 0x3d)
> + *value = 0x01;
> + if (where == 0x3c)
> + *value |= 0x0100;
Use PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN.
> + return PCIBIOS_SUCCESSFUL;
> +}
> +static void aglx_isr(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct altera_pcie *pcie;
> + struct device *dev;
> + u32 status;
> + int ret;
> +
> + chained_irq_enter(chip, desc);
> + pcie = irq_desc_get_handler_data(desc);
> + dev = &pcie->pdev->dev;
> +
> + status = readl((pcie->hip_base + port_conf_off
> + + AGLX_ROOT_PORT_IRQ_STATUS));
> + if (status & CFG_AER) {
> + ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> + if (ret)
> + dev_err_ratelimited(dev, "unexpected IRQ,\n");
Remove the comma at end (or maybe you meant to add something else?)
Looks like the place it was copied from had "unexpected IRQ, INT%d".
> + if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> + pcie->cs_base =
> + devm_platform_ioremap_resource_byname(pdev, "Cs");
> + if (IS_ERR(pcie->cs_base))
> + return PTR_ERR(pcie->cs_base);
> + of_property_read_u32(pcie->pdev->dev.of_node, "port_conf_stat",
> + &port_conf_off);
> + dev_info(&pcie->pdev->dev, "port_conf_stat_off =%x\n", port_conf_off);
Is this a debug message? Doesn't look like something we need all the
time. If you want it all the time, use %#x so it's clear that it's
hex.
> +static const struct altera_pcie_data altera_pcie_3_0_data = {
> + .ops = &altera_pcie_ops_3_0,
> + .version = ALTERA_PCIE_V3,
> + .cap_offset = 0x70,
> + .cfgrd0 = 0,
> + .cfgrd1 = 0,
> + .cfgwr0 = 0,
> + .cfgwr1 = 0,
cfgrd0, ..., cfgwr1 aren't used here, so no need to initialize them.
Powered by blists - more mailing lists