Re: [PATCH 1/1] pci: agilex_pci: add pcie rootport support for agilex

[Bjorn Helgaas <helgaas@kernel.org>]

Thanks for your patch.  If/when you revise it, run "git log --oneline
drivers/pci/controller/pcie-altera.c" and match the style of subject
lines, e.g.,

  PCI: altera: Add Intel Agilex support

On Fri, Jun 16, 2023 at 12:03:13PM +0530, sharath.kumar.d.m@intel.com wrote:
> From: D M Sharath Kumar <sharath.kumar.d.m@intel.com>

Also, please include a commit log.

Probably should also update the Kconfig help text to mention Agilex in
addition to Altera.

> +#include <linux/bitops.h>

I don't think this is needed.

> +static int aglx_rp_read_cfg(struct altera_pcie *pcie, 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;
> +	}
> +
> +	if (where == 0x3d)
> +		*value = 0x01;
> +	if (where == 0x3c)
> +		*value |= 0x0100;

This magic needs a comment.  Apparently it works around some hardware
defect?  What happens if this is a single byte read?  Looks like it
may set more than one byte of *value.

> +static int aglx_ep_write_cfg(struct altera_pcie *pcie, u8 busno,
> +		unsigned int devfn, int where, int size, u32 value)
> +{
> +	cs_writel(pcie, ((busno << 8) | devfn), AGLX_BDF_REG);
> +	if (busno > AGLX_RP_SECONDARY(pcie)) {
> +		/* type 1 */
> +		switch (size) {
> +		case 1:
> +			cs_writeb(pcie, value, ((1 << 12) | where));
> +			break;
> +		case 2:
> +			cs_writew(pcie, value, ((1 << 12) | where));
> +			break;
> +		default:
> +			cs_writel(pcie, value, ((1 << 12) | where));
> +			break;
> +		}
> +	} else {
> +		/* type 0 */
> +		switch (size) {
> +		case 1:
> +			cs_writeb(pcie, value, where);
> +			break;
> +		case 2:
> +			cs_writew(pcie, value, where);
> +			break;
> +		default:
> +			cs_writel(pcie, value, where);
> +				break;
> +		}
> +	}

These switches could be combined, e.g.,

  if (busno > AGLX_RP_SECONDARY(pcie))
    where |= 1 << 12;

> +	if (status & CFG_AER) {
> +		ret = generic_handle_domain_irq(pcie->irq_domain, 0);
> +		if (ret)
> +			dev_err_ratelimited(dev, "unexpected IRQ,\n");

Spurious "," at end of line.

> @@ -692,11 +904,27 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
>  {
>  	struct platform_device *pdev = pcie->pdev;
>  
> -	pcie->cra_base = devm_platform_ioremap_resource_byname(pdev, "Cra");
> -	if (IS_ERR(pcie->cra_base))
> -		return PTR_ERR(pcie->cra_base);
> +	if ((pcie->pcie_data->version == ALTERA_PCIE_V1) ||
> +		(pcie->pcie_data->version == ALTERA_PCIE_V2)) {
> +		pcie->cra_base =
> +			devm_platform_ioremap_resource_byname(pdev, "Cra");
> +		if (IS_ERR(pcie->cra_base))
> +			return PTR_ERR(pcie->cra_base);
> +	}

Should be a separate introductory patch since it's not directly
related to Agilex.

> -	if (pcie->pcie_data->version == ALTERA_PCIE_V2) {
> +	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);
> +	}
> +
> +	if ((pcie->pcie_data->version == ALTERA_PCIE_V2) ||
> +		(pcie->pcie_data->version == ALTERA_PCIE_V3)) {

Ditto.

>  		pcie->hip_base =
>  			devm_platform_ioremap_resource_byname(pdev, "Hip");
>  		if (IS_ERR(pcie->hip_base))
> @@ -708,7 +936,8 @@ static int altera_pcie_parse_dt(struct altera_pcie *pcie)
>  	if (pcie->irq < 0)
>  		return pcie->irq;
>  
> -	irq_set_chained_handler_and_data(pcie->irq, altera_pcie_isr, pcie);
> +	irq_set_chained_handler_and_data(pcie->irq,
> +		pcie->pcie_data->ops->rp_isr, pcie);

Ditto (including the new .rp_isr initializations below).

> @@ -721,6 +950,7 @@ static const struct altera_pcie_ops altera_pcie_ops_1_0 = {
>  	.tlp_read_pkt = tlp_read_packet,
>  	.tlp_write_pkt = tlp_write_packet,
>  	.get_link_status = altera_pcie_link_up,
> +	.rp_isr = altera_pcie_isr,
>  };

> @@ -793,11 +1045,17 @@ static int altera_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	/* clear all interrupts */
> -	cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
> -	/* enable all interrupts */
> -	cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
> -	altera_pcie_host_init(pcie);
> +	if ((pcie->pcie_data->version == ALTERA_PCIE_V1) ||
> +		(pcie->pcie_data->version == ALTERA_PCIE_V2)) {
> +		/* clear all interrupts */
> +		cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
> +		/* enable all interrupts */
> +		cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
> +		altera_pcie_host_init(pcie);

Ditto.

> +	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> +		writel(CFG_AER, (pcie->hip_base + port_conf_off
> +			+ AGLX_ROOT_PORT_IRQ_ENABLE));
> +	}
>  
>  	bridge->sysdata = pcie;
>  	bridge->busnr = pcie->root_bus_nr;
> -- 
> 2.34.1
>