> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, September 6, 2023 10:42 PM
> To: D M, Sharath Kumar <sharath.kumar.d.m@xxxxxxxxx>
> Cc: lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; dinguyen@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] PCI: altera: add suport for Agilex Family FPGA
>
> Capitalize subject line similarly.
ok
>
> s/suport/support/
ok
>
> On Wed, Sep 06, 2023 at 04:39:18PM +0530, sharath.kumar.d.m@xxxxxxxxx
> wrote:
> > From: D M Sharath Kumar <sharath.kumar.d.m@xxxxxxxxx>
>
> Needs a commit log. It's ok to repeat the subject line.
ok
>
> > +#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?
Will change to AGLX_CFG_AER
>
> > +static u32 port_conf_off;
>
> port_conf_off looks like something that should be per-controller.
Specific to agilex, will rename to "aglx_port_conf_off"
>
> > +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:
ok
>
> /* interrupt pin not programmed in hardware */
>
> > + if (where == 0x3d)
> > + *value = 0x01;
> > + if (where == 0x3c)
> > + *value |= 0x0100;
>
> Use PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN.
ok
>
> > + 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".
ok
>
> > + 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.
ok
>
> > +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.
ok
