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

[Bjorn Helgaas]

Capitalize subject line similarly.

s/suport/support/

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.

> +#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.