[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BY5PR11MB43066214534FA7D1604E0F78FDEDA@BY5PR11MB4306.namprd11.prod.outlook.com>
Date: Fri, 8 Sep 2023 09:09:34 +0000
From: "D M, Sharath Kumar" <sharath.kumar.d.m@...el.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
"kw@...ux.com" <kw@...ux.com>, "robh@...nel.org" <robh@...nel.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"dinguyen@...nel.org" <dinguyen@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
platform
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Wednesday, September 6, 2023 10:38 PM
> To: D M, Sharath Kumar <sharath.kumar.d.m@...el.com>
> Cc: lpieralisi@...nel.org; kw@...ux.com; robh@...nel.org;
> bhelgaas@...gle.com; linux-pci@...r.kernel.org; dinguyen@...nel.org;
> linux-kernel@...r.kernel.org
> 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@...el.com
> wrote:
> > From: D M Sharath Kumar <sharath.kumar.d.m@...el.com>
>
> 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;
> > +}
Powered by blists - more mailing lists