Re: [PATCH 7/7] pci: controller: pcie-altera: Add support for Agilex

[Bjorn Helgaas]

In subject:

  PCI: altera: Add Agilex support

to match style of history.

>  #define TLP_CFG_DW1(pcie, tag, be)	\
> -	(((TLP_REQ_ID(pcie->root_bus_nr,  RP_DEVFN)) << 16) | (tag << 8) | (be))
> +	(((TLP_REQ_ID((pcie)->root_bus_nr,  RP_DEVFN)) << 16) | ((tag) << 8) | (be))

Seems OK, but unrelated to adding Agilex support, so it should be a
separate patch.

> +#define AGLX_RP_CFG_ADDR(pcie, reg)	\
> +	(((pcie)->hip_base) + (reg))

Fits on one line.

> +#define AGLX_BDF_REG 0x00002004
> +#define AGLX_ROOT_PORT_IRQ_STATUS 0x14c
> +#define AGLX_ROOT_PORT_IRQ_ENABLE 0x150
> +#define CFG_AER                   BIT(4)

Indent values to match #defines above.

>  static bool altera_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int  devfn,
>  				    int offset)
>  {
> -	if (pci_is_root_bus(bus) && (devfn == 0) &&
> -	    (offset == PCI_BASE_ADDRESS_0))
> +	if (pci_is_root_bus(bus) && devfn == 0 && offset == PCI_BASE_ADDRESS_0)
>  		return true;

OK, but again unrelated to Agilex.

> @@ -373,7 +422,7 @@ static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
>  	 * Monitor changes to PCI_PRIMARY_BUS register on root port
>  	 * and update local copy of root bus number accordingly.
>  	 */
> -	if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
> +	if (bus == pcie->root_bus_nr && where == PCI_PRIMARY_BUS)

Ditto.

> @@ -577,7 +731,7 @@ static void altera_wait_link_retrain(struct altera_pcie *pcie)
>  			dev_err(dev, "link retrain timeout\n");
>  			break;
>  		}
> -		udelay(100);
> +		usleep_range(50, 150);

Where do these values come from?  Needs a comment, ideally with a spec
citation.

How do we know a 50us delay is enough when we previously waited at
least 100us?

> @@ -590,7 +744,7 @@ static void altera_wait_link_retrain(struct altera_pcie *pcie)
>  			dev_err(dev, "link up timeout\n");
>  			break;
>  		}
> -		udelay(100);
> +		usleep_range(50, 150);

Ditto.

> +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 + pcie->pcie_data->port_conf_offset +
> +		       pcie->pcie_data->port_irq_status_offset);
> +	if (status & CFG_AER) {
> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> +		if (ret)
> +			dev_err_ratelimited(dev, "unexpected IRQ,\n");

Was there supposed to be more data here, e.g., an IRQ %d or something?
Or is it just a spurious "," at the end of the line?

>  	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
> -					&intx_domain_ops, pcie);
> +						 &intx_domain_ops, pcie);

Cleanup that should be in a separate patch.  *This* patch should have
the absolute minimum required to enable Agilex to make it easier to
review/backport/revert/etc.

> +static const struct altera_pcie_data altera_pcie_3_0_f_tile_data = {
> +	.ops = &altera_pcie_ops_3_0,
> +	.version = ALTERA_PCIE_V3,
> +	.cap_offset = 0x70,

It looks like this is where the PCIe Capability is?  There's no way to
discover this offset, e.g., by following the capability list like
pci_find_capability() does?

Bjorn