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

[D M, Sharath Kumar]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, September 6, 2023 10:38 PM
> To: D M, Sharath Kumar <sharath.kumar.d.m@xxxxxxxxx>
> Cc: lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; dinguyen@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
> platform
> 
> 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.
> 
ok
> > @@ -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.
> 
Ok will change to "nonrp_read_cfg" ?
> > +	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.
> 
Will not take up as part of this commit
> > +
> > +	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;
> > +}