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

Commit Message

D M, Sharath Kumar Sept. 6, 2023, 11:09 a.m. UTC

From: D M Sharath Kumar <[e-mail]>

Signed-off-by: D M Sharath Kumar <[e-mail]>
---
 drivers/pci/controller/pcie-altera.c | 100 +++++++++++++++++++--------
 1 file changed, 70 insertions(+), 30 deletions(-)

Comments

Bjorn Helgaas Sept. 6, 2023, 5:08 p.m. UTC | #1

Capitalize subject line to match history:

  PCI: altera: Refactor driver for supporting new platform

On Wed, Sep 06, 2023 at 04:39:17PM +0530, [e-mail] wrote:
> From: D M Sharath Kumar <[e-mail]>

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

D M, Sharath Kumar Sept. 8, 2023, 9:09 a.m. UTC | #2

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: Wednesday, September 6, 2023 10:38 PM
> To: D M, Sharath Kumar <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> 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, [e-mail]
> wrote:
> > From: D M Sharath Kumar <[e-mail]>
> 
> 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;
> > +}

Bjorn Helgaas Sept. 8, 2023, 12:44 p.m. UTC | #3

On Fri, Sep 08, 2023 at 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > ...

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

D M, Sharath Kumar Sept. 8, 2023, 1:40 p.m. UTC | #4

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: Friday, September 8, 2023 6:14 PM
> To: D M, Sharath Kumar <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
> platform
> 
> On Fri, Sep 08, 2023 at 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]> ...
> 
> > > > +	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?
> 
Situation is
Root port configuration space  - memory mapped
Non root port configuration space - indirect access/proprietary access
    Type 0 for devices directly connected to root port
    Type 1 for others
> > > > +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; }

Bjorn Helgaas Sept. 8, 2023, 7:52 p.m. UTC | #5

On Fri, Sep 08, 2023 at 01:40:13PM +0000, D M, Sharath Kumar wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > On Fri, Sep 08, 2023 at 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <[e-mail]> ...
> > 
> > > > > +	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?
> > 
> Situation is
> Root port configuration space  - memory mapped
> Non root port configuration space - indirect access/proprietary access
>     Type 0 for devices directly connected to root port
>     Type 1 for others

"mm", "ind"?

D M, Sharath Kumar Sept. 11, 2023, 12:24 p.m. UTC | #6

From: D M Sharath Kumar <[e-mail]>

added new callback for
1) read,write to root port configuration registers
2) read,write to endpoint configuration registers
3) root port interrupt handler

agilex and newer platforms need to implemant the callback and generic root 
port driver should work ( without much changes ) , legacy platforms (arria
 and startix) implement configuration read,write directly in wrapper 
api _altera_pcie_cfg_read/_altera_pcie_cfg_write

changelog v2:
saperated into two patches
1.refactored the driver for easily portability to future Altera FPGA
platforms
2.added support for "Agilex" FPGA

this driver supports PCI RP IP on Agilex FPGA, as these are FPGA its up
to the user to add PCI RP or not ( as per his needs). we are not adding
the device tree as part of this commit. we are expecting the add device
tree changes only if he is adding PCI RP IP in his design

changelog v3:
incorporate review comments from Bjorn Helgaas


D M Sharath Kumar (2):
  PCI: altera: refactor driver for supporting new platforms
  PCI: altera: add support for agilex family fpga

 drivers/pci/controller/pcie-altera.c | 305 ++++++++++++++++++++++++---
 1 file changed, 275 insertions(+), 30 deletions(-)

D M, Sharath Kumar Sept. 11, 2023, 1:35 p.m. UTC | #7

> -----Original Message-----
> From: Bjorn Helgaas <[e-mail]>
> Sent: Saturday, September 9, 2023 1:23 AM
> To: D M, Sharath Kumar <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
> platform
> 
> On Fri, Sep 08, 2023 at 01:40:13PM +0000, D M, Sharath Kumar wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]> On Fri, Sep 08, 2023 at
> > > 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <[e-mail]> ...
> > >
> > > > > > +	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?
> > >
> > Situation is
> > Root port configuration space  - memory mapped Non root port
> > configuration space - indirect access/proprietary access
> >     Type 0 for devices directly connected to root port
> >     Type 1 for others
> 
> "mm", "ind"?
Memory mapped -  configuration registers available at pre defined address

D M, Sharath Kumar Sept. 11, 2023, 1:53 p.m. UTC | #8

From: D M Sharath Kumar <[e-mail]>

added new callback for
1) read,write to root port configuration registers
2) read,write to endpoint configuration registers
3) root port interrupt handler

agilex and newer platforms need to implemant the callback and generic root 
port driver should work ( without much changes ) , legacy platforms (arria
 and startix) implement configuration read,write directly in wrapper 
api _altera_pcie_cfg_read/_altera_pcie_cfg_write

changelog v2:
saperated into two patches
1.refactored the driver for easily portability to future Altera FPGA
platforms
2.added support for "Agilex" FPGA

this driver supports PCI RP IP on Agilex FPGA, as these are FPGA its up
to the user to add PCI RP or not ( as per his needs). we are not adding
the device tree as part of this commit. we are expecting the add device
tree changes only if he is adding PCI RP IP in his design

changelog v3:
incorporate review comments from Bjorn Helgaas


D M Sharath Kumar (2):
  PCI: altera: refactor driver for supporting new platforms
  PCI: altera: add support for agilex family fpga

 drivers/pci/controller/pcie-altera.c | 305 ++++++++++++++++++++++++---
 1 file changed, 275 insertions(+), 30 deletions(-)

Bjorn Helgaas Sept. 11, 2023, 8:08 p.m. UTC | #9

On Fri, Sep 08, 2023 at 01:40:13PM +0000, D M, Sharath Kumar wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <[e-mail]>
> > Sent: Friday, September 8, 2023 6:14 PM
> > To: D M, Sharath Kumar <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
> > platform
> > 
> > On Fri, Sep 08, 2023 at 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <[e-mail]> ...
> > 
> > > > > +	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?
>
> Situation is
> Root port configuration space  - memory mapped

I don't quite believe the idea that the access method is based on
whether it's a root port.  For one thing, you decide whether to use
the memory-mapped accessor or the indirect accessor based on whether
the read targets the *root bus*, not whether it targets a root port.
And obviously you don't *know* whether the device at a B/D/F address
is a root port until after you read the PCIe type.

I think using names similar to other drivers will be helpful.

These all work on the root bus:

  exynos_pcie_rd_own_conf
  meson_pcie_rd_own_conf
  rockchip_pcie_rd_own_conf

These work on non-root buses:

  dw_pcie_rd_other_conf
  rockchip_pcie_rd_other_conf

> Non root port configuration space - indirect access/proprietary access
>     Type 0 for devices directly connected to root port
>     Type 1 for others

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

Bjorn Helgaas Sept. 13, 2023, 12:59 p.m. UTC | #10

On Mon, Sep 11, 2023 at 03:08:08PM -0500, Bjorn Helgaas wrote:
> On Fri, Sep 08, 2023 at 01:40:13PM +0000, D M, Sharath Kumar wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <[e-mail]>
> > > Sent: Friday, September 8, 2023 6:14 PM
> > > To: D M, Sharath Kumar <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]
> > > Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
> > > platform
> > > 
> > > On Fri, Sep 08, 2023 at 09:09:34AM +0000, D M, Sharath Kumar wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <[e-mail]> ...
> > > 
> > > > > > +	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?
> >
> > Situation is
> > Root port configuration space  - memory mapped
> 
> I don't quite believe the idea that the access method is based on
> whether it's a root port.  For one thing, you decide whether to use
> the memory-mapped accessor or the indirect accessor based on whether
> the read targets the *root bus*, not whether it targets a root port.
> And obviously you don't *know* whether the device at a B/D/F address
> is a root port until after you read the PCIe type.

I see eight copies of "[PATCH v3 0/2]" on the list:
https://lore.kernel.org/linux-pci/[e-mail]/T/#t
The duplication just causes confusion and slows things down.

I do think the naming as root port config accessors and endpoint
config accessors is fundamentally broken and needs to be fixed.  It's
not a *functional* issue, but it is important to avoid misleading
names.

> I think using names similar to other drivers will be helpful.
> 
> These all work on the root bus:
> 
>   exynos_pcie_rd_own_conf
>   meson_pcie_rd_own_conf
>   rockchip_pcie_rd_own_conf
> 
> These work on non-root buses:
> 
>   dw_pcie_rd_other_conf
>   rockchip_pcie_rd_other_conf
> 
> > Non root port configuration space - indirect access/proprietary access
> >     Type 0 for devices directly connected to root port
> >     Type 1 for others
> 
> > > > > > +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; }

Dinh Nguyen Sept. 15, 2023, 10:51 a.m. UTC | #11

On 9/11/23 08:53, [e-mail] wrote:
> From: D M Sharath Kumar <[e-mail]>
> 
> added new callback for
> 1) read,write to root port configuration registers
> 2) read,write to endpoint configuration registers
> 3) root port interrupt handler
> 
> agilex and newer platforms need to implemant the callback and generic root
> port driver should work ( without much changes ) , legacy platforms (arria
>   and startix) implement configuration read,write directly in wrapper
> api _altera_pcie_cfg_read/_altera_pcie_cfg_write
> 
> changelog v2:
> saperated into two patches
> 1.refactored the driver for easily portability to future Altera FPGA
> platforms
> 2.added support for "Agilex" FPGA
> 
> this driver supports PCI RP IP on Agilex FPGA, as these are FPGA its up
> to the user to add PCI RP or not ( as per his needs). we are not adding
> the device tree as part of this commit. we are expecting the add device
> tree changes only if he is adding PCI RP IP in his design
> 
> changelog v3:
> incorporate review comments from Bjorn Helgaas
> 
> 

You've sent 6 versions of this patchset in a 3-hour time span, what is 
going on?

Dinh

Patch

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index 18b2361d6462..4e543ec6dfb6 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -3,6 +3,7 @@ 
  * Copyright Altera Corporation (C) 2013-2015. All rights reserved
  *
  * Author: Ley Foon Tan <[e-mail]>
+ * Author: sharath <[e-mail]>
  * Description: Altera PCIe host controller driver
  */
 
@@ -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);
+	void (*rp_isr)(struct irq_desc *desc);
 };
 
 struct altera_pcie_data {
@@ -380,8 +386,8 @@  static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static int s10_rp_read_cfg(struct altera_pcie *pcie, int where,
-			   int size, u32 *value)
+static int s10_rp_read_cfg(struct altera_pcie *pcie, u8 busno, u32 devfn,
+		int where, int size, u32 *value)
 {
 	void __iomem *addr = S10_RP_CFG_ADDR(pcie, where);
 
@@ -400,7 +406,7 @@  static int s10_rp_read_cfg(struct altera_pcie *pcie, int where,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static int s10_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
+static int s10_rp_write_cfg(struct altera_pcie *pcie, u8 busno, u32 devfn,
 			    int where, int size, u32 value)
 {
 	void __iomem *addr = S10_RP_CFG_ADDR(pcie, where);
@@ -427,18 +433,13 @@  static int s10_rp_write_cfg(struct altera_pcie *pcie, u8 busno,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
-				 unsigned int devfn, int where, int size,
-				 u32 *value)
+static int arr_read_cfg(struct altera_pcie *pcie, u8 busno, u32 devfn,
+		int where, int size, u32 *value)
 {
 	int ret;
 	u32 data;
 	u8 byte_en;
 
-	if (busno == pcie->root_bus_nr && pcie->pcie_data->ops->rp_read_cfg)
-		return pcie->pcie_data->ops->rp_read_cfg(pcie, where,
-							 size, value);
-
 	switch (size) {
 	case 1:
 		byte_en = 1 << (where & 3);
@@ -471,18 +472,13 @@  static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static int _altera_pcie_cfg_write(struct altera_pcie *pcie, u8 busno,
-				  unsigned int devfn, int where, int size,
-				  u32 value)
+static int arr_write_cfg(struct altera_pcie *pcie, u8 busno, u32 devfn,
+			    int where, int size, u32 value)
 {
 	u32 data32;
 	u32 shift = 8 * (where & 3);
 	u8 byte_en;
 
-	if (busno == pcie->root_bus_nr && pcie->pcie_data->ops->rp_write_cfg)
-		return pcie->pcie_data->ops->rp_write_cfg(pcie, busno,
-						     where, size, value);
-
 	switch (size) {
 	case 1:
 		data32 = (value & 0xff) << shift;
@@ -500,6 +496,35 @@  static int _altera_pcie_cfg_write(struct altera_pcie *pcie, u8 busno,
 
 	return tlp_cfg_dword_write(pcie, busno, devfn, (where & ~DWORD_MASK),
 				   byte_en, data32);
+
+}
+
+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;
+}
+
+static int _altera_pcie_cfg_write(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_write_cfg)
+		return pcie->pcie_data->ops->rp_write_cfg(pcie, busno, devfn,
+						     where, size, value);
+
+	if (pcie->pcie_data->ops->ep_write_cfg)
+		return pcie->pcie_data->ops->ep_write_cfg(pcie, busno, devfn,
+						     where, size, value);
+	return PCIBIOS_FUNC_NOT_SUPPORTED;
 }
 
 static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
@@ -661,7 +686,6 @@  static void altera_pcie_isr(struct irq_desc *desc)
 				dev_err_ratelimited(dev, "unexpected IRQ, INT%d\n", bit);
 		}
 	}
-
 	chained_irq_exit(chip, desc);
 }
 
@@ -692,9 +716,13 @@  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);
+	}
 
 	if (pcie->pcie_data->version == ALTERA_PCIE_V2) {
 		pcie->hip_base =
@@ -708,7 +736,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);
 	return 0;
 }
 
@@ -721,6 +750,11 @@  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_read_cfg = arr_read_cfg,
+	.rp_write_cfg = arr_write_cfg,
+	.ep_read_cfg = arr_read_cfg,
+	.ep_write_cfg = arr_write_cfg,
+	.rp_isr = altera_pcie_isr,
 };
 
 static const struct altera_pcie_ops altera_pcie_ops_2_0 = {
@@ -729,6 +763,9 @@  static const struct altera_pcie_ops altera_pcie_ops_2_0 = {
 	.get_link_status = s10_altera_pcie_link_up,
 	.rp_read_cfg = s10_rp_read_cfg,
 	.rp_write_cfg = s10_rp_write_cfg,
+	.ep_read_cfg = arr_read_cfg,
+	.ep_write_cfg = arr_write_cfg,
+	.rp_isr = altera_pcie_isr,
 };
 
 static const struct altera_pcie_data altera_pcie_1_0_data = {
@@ -793,11 +830,14 @@  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);
+	}
 
 	bridge->sysdata = pcie;
 	bridge->busnr = pcie->root_bus_nr;