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

[matthew . gerlach]

On Wed, 31 Jul 2024, Bjorn Helgaas wrote:

> In subject:
>
>  PCI: altera: Add Agilex support
>
> to match style of history.

I will change the subject to match the 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.

Yes, it is unrelated to Agilex and should be in a separate patch.

> > +#define AGLX_RP_CFG_ADDR(pcie, reg)	\
> > +	(((pcie)->hip_base) + (reg))
>
> Fits on one line.

One line would be better.

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

This is an unrelated change to Agilex and possibly wrong. I will remove 
both instances of the change from this patch.

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

It is a spurious "," that will be removed.

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

I will ensure this patch has the absolute minimum required to enable 
Agilex.

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

The cap_offset structure member is the offset in the "Hip" memory space 
where the PCIe Capability starts. The offset is explicitly stated in the 
relevent documentation for Statix10 and Agilex. One could follow the 
capability list, but adding a function like __pci_find_next_cap_ttl() 
seems like a bigger change than having the constants.

Thanks for the review,
Matthew

> Bjorn