Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new platform

[Bjorn Helgaas]

Capitalize subject line to match history:

  PCI: altera: Refactor driver for supporting new platform

On Wed, Sep 06, 2023 at 04:39:17PM +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.  Also helpful
if you can hint about why it needs to be refactored.  In this case, it
looks like you'll need to have a different ISR for Agilex, and also
something different about root bus vs downstream config accesses.

> @@ -100,10 +101,15 @@ struct altera_pcie_ops {
>  	void (*tlp_write_pkt)(struct altera_pcie *pcie, u32 *headers,
>  			      u32 data, bool align);
>  	bool (*get_link_status)(struct altera_pcie *pcie);
> -	int (*rp_read_cfg)(struct altera_pcie *pcie, int where,
> -			   int size, u32 *value);
> +	int (*rp_read_cfg)(struct altera_pcie *pcie, u8 busno,
> +			unsigned int devfn, int where, int size, u32 *value);
>  	int (*rp_write_cfg)(struct altera_pcie *pcie, u8 busno,
> -			    int where, int size, u32 value);
> +			unsigned int devfn, int where, int size, u32 value);
> +	int (*ep_read_cfg)(struct altera_pcie *pcie, u8 busno,
> +			unsigned int devfn, int where, int size, u32 *value);
> +	int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
> +			unsigned int devfn, int where, int size, u32 value);

"ep_read_cfg" isn't the ideal name because it suggests "endpoint", but
it may be either an endpoint or a switch upstream port.  The rockchip
driver uses "other", which isn't super descriptive either but might be
better.

> +	void (*rp_isr)(struct irq_desc *desc);
>  };

> +static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
> +				 unsigned int devfn, int where, int size,
> +				 u32 *value)
> +{
> +	if (busno == pcie->root_bus_nr && pcie->pcie_data->ops->rp_read_cfg)
> +		return pcie->pcie_data->ops->rp_read_cfg(pcie, busno, devfn,
> +							where, size, value);

Several other drivers use pci_is_root_bus() instead of keeping track
of root_bus_nr and watching for changes to it.  Maybe simpler and more
reliable?  That would be best as a separate patch all by itself if you
go that direction.

> +
> +	if (pcie->pcie_data->ops->ep_read_cfg)
> +		return pcie->pcie_data->ops->ep_read_cfg(pcie, busno, devfn,
> +							where, size, value);
> +	return PCIBIOS_FUNC_NOT_SUPPORTED;
> +}