RE: [PATCH v2 2/2] PCI: altera: add suport for Agilex Family FPGA

[D M, Sharath Kumar]

> -----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