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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Sep 08, 2023 at 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > ...

> > > +	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.
> > 
> Ok will change to "nonrp_read_cfg" ?

I think the important point is not whether it's a Root Port or not,
but whether it's on the root *bus* or not.  In other words, I think
the driver has to determine whether to generate a Type 0 (targeting
something on the root bus) or a Type 1 (targeting something below a
bridge) config transaction (see PCI-to-PCI Bridge spec r1.2, sec
3.1.2.1).

There can be non-Root Ports on the root bus, so "nonrp" doesn't seem
quite right.  "Other" would be OK, since that's already used by other
drivers.  Maybe "type0" and "type1" would be better and would fit well
with the root_bus_nr check you use to distinguish them?

> > > +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);
> > > +
> > > +	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;
> > > +}