diff mbox series

[V7,04/15] PCI: dwc: Move config space capability search API

Message ID 20190517123846.3708-5-vidyas@nvidia.com (mailing list archive)
State Superseded, archived
Headers show
Series Add Tegra194 PCIe support | expand

Commit Message

Vidya Sagar May 17, 2019, 12:38 p.m. UTC
Move PCIe config space capability search API to common DesignWare file
as this can be used by both host and ep mode codes.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
Changes since [v6]:
* Exported dw_pcie_find_capability() API

Changes since [v5]:
* None

Changes since [v4]:
* Removed redundant APIs in pcie-designware-ep.c file after moving them
  to pcie-designware.c file based on Bjorn's comments.

Changes since [v3]:
* Rebased to linux-next top of the tree

Changes since [v2]:
* None

Changes since [v1]:
* Removed dw_pcie_find_next_ext_capability() API from here and made a
  separate patch for that

 .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
 drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +
 3 files changed, 44 insertions(+), 35 deletions(-)

Comments

Thierry Reding May 21, 2019, 10:29 a.m. UTC | #1
On Fri, May 17, 2019 at 06:08:35PM +0530, Vidya Sagar wrote:
> Move PCIe config space capability search API to common DesignWare file
> as this can be used by both host and ep mode codes.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> Changes since [v6]:
> * Exported dw_pcie_find_capability() API
> 
> Changes since [v5]:
> * None
> 
> Changes since [v4]:
> * Removed redundant APIs in pcie-designware-ep.c file after moving them
>   to pcie-designware.c file based on Bjorn's comments.
> 
> Changes since [v3]:
> * Rebased to linux-next top of the tree
> 
> Changes since [v2]:
> * None
> 
> Changes since [v1]:
> * Removed dw_pcie_find_next_ext_capability() API from here and made a
>   separate patch for that
> 
>  .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
>  drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
>  3 files changed, 44 insertions(+), 35 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
Bjorn Helgaas May 21, 2019, 9:17 p.m. UTC | #2
On Fri, May 17, 2019 at 06:08:35PM +0530, Vidya Sagar wrote:
> Move PCIe config space capability search API to common DesignWare file
> as this can be used by both host and ep mode codes.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
> Changes since [v6]:
> * Exported dw_pcie_find_capability() API
> 
> Changes since [v5]:
> * None
> 
> Changes since [v4]:
> * Removed redundant APIs in pcie-designware-ep.c file after moving them
>   to pcie-designware.c file based on Bjorn's comments.
> 
> Changes since [v3]:
> * Rebased to linux-next top of the tree
> 
> Changes since [v2]:
> * None
> 
> Changes since [v1]:
> * Removed dw_pcie_find_next_ext_capability() API from here and made a
>   separate patch for that
> 
>  .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
>  drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
>  3 files changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 2bf5a35c0570..65f479250087 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -40,39 +40,6 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>  }
>  
> -static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> -			      u8 cap)
> -{
> -	u8 cap_id, next_cap_ptr;
> -	u16 reg;
> -
> -	if (!cap_ptr)
> -		return 0;
> -
> -	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> -	cap_id = (reg & 0x00ff);
> -
> -	if (cap_id > PCI_CAP_ID_MAX)
> -		return 0;
> -
> -	if (cap_id == cap)
> -		return cap_ptr;
> -
> -	next_cap_ptr = (reg & 0xff00) >> 8;
> -	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> -}
> -
> -static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
> -{
> -	u8 next_cap_ptr;
> -	u16 reg;
> -
> -	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> -	next_cap_ptr = (reg & 0x00ff);
> -
> -	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> -}
> -
>  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>  				   struct pci_epf_header *hdr)
>  {
> @@ -612,9 +579,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>  		return -ENOMEM;
>  	}
> -	ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
> +	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>  
> -	ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
> +	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>  
>  	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>  	if (offset) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 83cdd2ce2486..8f53ce63d17e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -14,6 +14,46 @@
>  
>  #include "pcie-designware.h"
>  
> +/*
> + * These APIs are different from standard pci_find_*capability() APIs in the
> + * sense that former can only be used post device enumeration as they require
> + * 'struct pci_dev *' pointer whereas these APIs require 'struct dw_pcie *'
> + * pointer and can be used before link up also.

I think this comment is slightly misleading because it suggests the
reason we need these DW interfaces is because we're doing something
before a pci_dev pointer is available.

But these DW interfaces are used on devices that will *never* have a
pci_dev pointer because they are not PCI devices.  They're used on
host controller devices, which have a PCIe link on the downstream
side, but the host controller driver operates them using their
upstream, non-PCI interfaces.  Logically, I think they would be
considered parts of Root Complexes, not Root Ports.

There's actually no reason why that upstream interface should look
anything like PCI; it doesn't need to organize registers into
capability lists at all.  It might be convenient for the hardware to
do that and share things with a Root Port device, which *is* a PCI
device, but it's not required.

It also really has nothing to do with whether the link is up.  This
code operates on hardware that is upstream from the link, so we can
reach it regardless of the link.

> + */
> +static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> +				  u8 cap)
> +{
> +	u8 cap_id, next_cap_ptr;
> +	u16 reg;
> +
> +	if (!cap_ptr)
> +		return 0;
> +
> +	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> +	cap_id = (reg & 0x00ff);
> +
> +	if (cap_id > PCI_CAP_ID_MAX)
> +		return 0;
> +
> +	if (cap_id == cap)
> +		return cap_ptr;
> +
> +	next_cap_ptr = (reg & 0xff00) >> 8;
> +	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
> +}
> +
> +u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> +{
> +	u8 next_cap_ptr;
> +	u16 reg;
> +
> +	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> +	next_cap_ptr = (reg & 0x00ff);
> +
> +	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
> +
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>  {
>  	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 14762e262758..6cb978132469 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -251,6 +251,8 @@ struct dw_pcie {
>  #define to_dw_pcie_from_ep(endpoint)   \
>  		container_of((endpoint), struct dw_pcie, ep)
>  
> +u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> +
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>  int dw_pcie_write(void __iomem *addr, int size, u32 val);
>  
> -- 
> 2.17.1
>
Vidya Sagar May 22, 2019, 8:56 a.m. UTC | #3
On 5/22/2019 2:47 AM, Bjorn Helgaas wrote:
> On Fri, May 17, 2019 at 06:08:35PM +0530, Vidya Sagar wrote:
>> Move PCIe config space capability search API to common DesignWare file
>> as this can be used by both host and ep mode codes.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>> Changes since [v6]:
>> * Exported dw_pcie_find_capability() API
>>
>> Changes since [v5]:
>> * None
>>
>> Changes since [v4]:
>> * Removed redundant APIs in pcie-designware-ep.c file after moving them
>>    to pcie-designware.c file based on Bjorn's comments.
>>
>> Changes since [v3]:
>> * Rebased to linux-next top of the tree
>>
>> Changes since [v2]:
>> * None
>>
>> Changes since [v1]:
>> * Removed dw_pcie_find_next_ext_capability() API from here and made a
>>    separate patch for that
>>
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
>>   drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
>>   drivers/pci/controller/dwc/pcie-designware.h  |  2 +
>>   3 files changed, 44 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> index 2bf5a35c0570..65f479250087 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> @@ -40,39 +40,6 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>   	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>   }
>>   
>> -static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>> -			      u8 cap)
>> -{
>> -	u8 cap_id, next_cap_ptr;
>> -	u16 reg;
>> -
>> -	if (!cap_ptr)
>> -		return 0;
>> -
>> -	reg = dw_pcie_readw_dbi(pci, cap_ptr);
>> -	cap_id = (reg & 0x00ff);
>> -
>> -	if (cap_id > PCI_CAP_ID_MAX)
>> -		return 0;
>> -
>> -	if (cap_id == cap)
>> -		return cap_ptr;
>> -
>> -	next_cap_ptr = (reg & 0xff00) >> 8;
>> -	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>> -}
>> -
>> -static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
>> -{
>> -	u8 next_cap_ptr;
>> -	u16 reg;
>> -
>> -	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>> -	next_cap_ptr = (reg & 0x00ff);
>> -
>> -	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>> -}
>> -
>>   static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>>   				   struct pci_epf_header *hdr)
>>   {
>> @@ -612,9 +579,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>   		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>>   		return -ENOMEM;
>>   	}
>> -	ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
>> +	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>>   
>> -	ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
>> +	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>>   
>>   	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>>   	if (offset) {
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index 83cdd2ce2486..8f53ce63d17e 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -14,6 +14,46 @@
>>   
>>   #include "pcie-designware.h"
>>   
>> +/*
>> + * These APIs are different from standard pci_find_*capability() APIs in the
>> + * sense that former can only be used post device enumeration as they require
>> + * 'struct pci_dev *' pointer whereas these APIs require 'struct dw_pcie *'
>> + * pointer and can be used before link up also.
> 
> I think this comment is slightly misleading because it suggests the
> reason we need these DW interfaces is because we're doing something
> before a pci_dev pointer is available.
> 
> But these DW interfaces are used on devices that will *never* have a
> pci_dev pointer because they are not PCI devices.  They're used on
> host controller devices, which have a PCIe link on the downstream
> side, but the host controller driver operates them using their
> upstream, non-PCI interfaces.  Logically, I think they would be
> considered parts of Root Complexes, not Root Ports.
> 
> There's actually no reason why that upstream interface should look
> anything like PCI; it doesn't need to organize registers into
> capability lists at all.  It might be convenient for the hardware to
> do that and share things with a Root Port device, which *is* a PCI
> device, but it's not required.
> 
> It also really has nothing to do with whether the link is up.  This
> code operates on hardware that is upstream from the link, so we can
> reach it regardless of the link.
I added this comment after receiving a review comment to justify why standard
pci_find_*capability() APIs can't be used here. Hence added this.
I understand your comment that DW interface need not have to be a PCI device,
but what is present in the hardware is effectively a root port implementation
and post enumeration, we get a 'struct pci_dev' created for it, hence I thought
it is fine to bring 'struct pci_dev' into picture.
Also, I agree that mention of 'link up' is unwarranted and could be reworded in
a better way.
Do you suggest to remove this comment altogether or reword it
s/and can be used before link up also/and can be used before 'struct pci_dev' is
available/ ?

> 
>> + */
>> +static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>> +				  u8 cap)
>> +{
>> +	u8 cap_id, next_cap_ptr;
>> +	u16 reg;
>> +
>> +	if (!cap_ptr)
>> +		return 0;
>> +
>> +	reg = dw_pcie_readw_dbi(pci, cap_ptr);
>> +	cap_id = (reg & 0x00ff);
>> +
>> +	if (cap_id > PCI_CAP_ID_MAX)
>> +		return 0;
>> +
>> +	if (cap_id == cap)
>> +		return cap_ptr;
>> +
>> +	next_cap_ptr = (reg & 0xff00) >> 8;
>> +	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
>> +}
>> +
>> +u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
>> +{
>> +	u8 next_cap_ptr;
>> +	u16 reg;
>> +
>> +	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>> +	next_cap_ptr = (reg & 0x00ff);
>> +
>> +	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
>> +}
>> +EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
>> +
>>   int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>>   {
>>   	if (!IS_ALIGNED((uintptr_t)addr, size)) {
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 14762e262758..6cb978132469 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -251,6 +251,8 @@ struct dw_pcie {
>>   #define to_dw_pcie_from_ep(endpoint)   \
>>   		container_of((endpoint), struct dw_pcie, ep)
>>   
>> +u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
>> +
>>   int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>>   int dw_pcie_write(void __iomem *addr, int size, u32 val);
>>   
>> -- 
>> 2.17.1
>>
Bjorn Helgaas May 22, 2019, 2:02 p.m. UTC | #4
On Wed, May 22, 2019 at 02:26:08PM +0530, Vidya Sagar wrote:
> On 5/22/2019 2:47 AM, Bjorn Helgaas wrote:
> > On Fri, May 17, 2019 at 06:08:35PM +0530, Vidya Sagar wrote:
> > > Move PCIe config space capability search API to common DesignWare file
> > > as this can be used by both host and ep mode codes.

> > >   .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
> > >   drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
> > >   drivers/pci/controller/dwc/pcie-designware.h  |  2 +
> > >   3 files changed, 44 insertions(+), 35 deletions(-)

> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -14,6 +14,46 @@
> > >   #include "pcie-designware.h"
> > > +/*
> > > + * These APIs are different from standard pci_find_*capability() APIs in the
> > > + * sense that former can only be used post device enumeration as they require
> > > + * 'struct pci_dev *' pointer whereas these APIs require 'struct dw_pcie *'
> > > + * pointer and can be used before link up also.
> > 
> > I think this comment is slightly misleading because it suggests the
> > reason we need these DW interfaces is because we're doing something
> > before a pci_dev pointer is available.
> > 
> > But these DW interfaces are used on devices that will *never* have a
> > pci_dev pointer because they are not PCI devices.  They're used on
> > host controller devices, which have a PCIe link on the downstream
> > side, but the host controller driver operates them using their
> > upstream, non-PCI interfaces.  Logically, I think they would be
> > considered parts of Root Complexes, not Root Ports.
> > 
> > There's actually no reason why that upstream interface should look
> > anything like PCI; it doesn't need to organize registers into
> > capability lists at all.  It might be convenient for the hardware to
> > do that and share things with a Root Port device, which *is* a PCI
> > device, but it's not required.
> > 
> > It also really has nothing to do with whether the link is up.  This
> > code operates on hardware that is upstream from the link, so we can
> > reach it regardless of the link.
>
> I added this comment after receiving a review comment to justify why
> standard pci_find_*capability() APIs can't be used here. Hence added
> this.  I understand your comment that DW interface need not have to
> be a PCI device, but what is present in the hardware is effectively
> a root port implementation and post enumeration, we get a 'struct
> pci_dev' created for it, hence I thought it is fine to bring 'struct
> pci_dev' into picture.

This code operates on the host controller.  It configures the bridge
that leads *to* PCI devices.  Since that bridge is not a PCI device,
the PCI specs don't say anything about how to program it.

The fact that the host controller programming interface happens to
resemble the PCI programming interface is purely coincidental.

> Also, I agree that mention of 'link up' is unwarranted and could be
> reworded in a better way.
>
> Do you suggest to remove this comment altogether or reword it s/and
> can be used before link up also/and can be used before 'struct
> pci_dev' is available/ ?

Maybe something like this?

    These interfaces resemble the pci_find_*capability() interfaces,
    but these are for configuring host controllers, which are bridges
    *to* PCI devices but are not PCI devices themselves.
Vidya Sagar May 24, 2019, 2:46 p.m. UTC | #5
On 5/22/2019 7:32 PM, Bjorn Helgaas wrote:
> On Wed, May 22, 2019 at 02:26:08PM +0530, Vidya Sagar wrote:
>> On 5/22/2019 2:47 AM, Bjorn Helgaas wrote:
>>> On Fri, May 17, 2019 at 06:08:35PM +0530, Vidya Sagar wrote:
>>>> Move PCIe config space capability search API to common DesignWare file
>>>> as this can be used by both host and ep mode codes.
> 
>>>>    .../pci/controller/dwc/pcie-designware-ep.c   | 37 +----------------
>>>>    drivers/pci/controller/dwc/pcie-designware.c  | 40 +++++++++++++++++++
>>>>    drivers/pci/controller/dwc/pcie-designware.h  |  2 +
>>>>    3 files changed, 44 insertions(+), 35 deletions(-)
> 
>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>>>> @@ -14,6 +14,46 @@
>>>>    #include "pcie-designware.h"
>>>> +/*
>>>> + * These APIs are different from standard pci_find_*capability() APIs in the
>>>> + * sense that former can only be used post device enumeration as they require
>>>> + * 'struct pci_dev *' pointer whereas these APIs require 'struct dw_pcie *'
>>>> + * pointer and can be used before link up also.
>>>
>>> I think this comment is slightly misleading because it suggests the
>>> reason we need these DW interfaces is because we're doing something
>>> before a pci_dev pointer is available.
>>>
>>> But these DW interfaces are used on devices that will *never* have a
>>> pci_dev pointer because they are not PCI devices.  They're used on
>>> host controller devices, which have a PCIe link on the downstream
>>> side, but the host controller driver operates them using their
>>> upstream, non-PCI interfaces.  Logically, I think they would be
>>> considered parts of Root Complexes, not Root Ports.
>>>
>>> There's actually no reason why that upstream interface should look
>>> anything like PCI; it doesn't need to organize registers into
>>> capability lists at all.  It might be convenient for the hardware to
>>> do that and share things with a Root Port device, which *is* a PCI
>>> device, but it's not required.
>>>
>>> It also really has nothing to do with whether the link is up.  This
>>> code operates on hardware that is upstream from the link, so we can
>>> reach it regardless of the link.
>>
>> I added this comment after receiving a review comment to justify why
>> standard pci_find_*capability() APIs can't be used here. Hence added
>> this.  I understand your comment that DW interface need not have to
>> be a PCI device, but what is present in the hardware is effectively
>> a root port implementation and post enumeration, we get a 'struct
>> pci_dev' created for it, hence I thought it is fine to bring 'struct
>> pci_dev' into picture.
> 
> This code operates on the host controller.  It configures the bridge
> that leads *to* PCI devices.  Since that bridge is not a PCI device,
> the PCI specs don't say anything about how to program it.
> 
> The fact that the host controller programming interface happens to
> resemble the PCI programming interface is purely coincidental.
> 
>> Also, I agree that mention of 'link up' is unwarranted and could be
>> reworded in a better way.
>>
>> Do you suggest to remove this comment altogether or reword it s/and
>> can be used before link up also/and can be used before 'struct
>> pci_dev' is available/ ?
> 
> Maybe something like this?
> 
>      These interfaces resemble the pci_find_*capability() interfaces,
>      but these are for configuring host controllers, which are bridges
>      *to* PCI devices but are not PCI devices themselves.
Ok. Done.

>
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 2bf5a35c0570..65f479250087 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -40,39 +40,6 @@  void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 	__dw_pcie_ep_reset_bar(pci, bar, 0);
 }
 
-static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
-			      u8 cap)
-{
-	u8 cap_id, next_cap_ptr;
-	u16 reg;
-
-	if (!cap_ptr)
-		return 0;
-
-	reg = dw_pcie_readw_dbi(pci, cap_ptr);
-	cap_id = (reg & 0x00ff);
-
-	if (cap_id > PCI_CAP_ID_MAX)
-		return 0;
-
-	if (cap_id == cap)
-		return cap_ptr;
-
-	next_cap_ptr = (reg & 0xff00) >> 8;
-	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
-}
-
-static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
-{
-	u8 next_cap_ptr;
-	u16 reg;
-
-	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
-	next_cap_ptr = (reg & 0x00ff);
-
-	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
-}
-
 static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
 				   struct pci_epf_header *hdr)
 {
@@ -612,9 +579,9 @@  int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
 		return -ENOMEM;
 	}
-	ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
+	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
 
-	ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
+	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
 
 	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
 	if (offset) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 83cdd2ce2486..8f53ce63d17e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -14,6 +14,46 @@ 
 
 #include "pcie-designware.h"
 
+/*
+ * These APIs are different from standard pci_find_*capability() APIs in the
+ * sense that former can only be used post device enumeration as they require
+ * 'struct pci_dev *' pointer whereas these APIs require 'struct dw_pcie *'
+ * pointer and can be used before link up also.
+ */
+static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
+				  u8 cap)
+{
+	u8 cap_id, next_cap_ptr;
+	u16 reg;
+
+	if (!cap_ptr)
+		return 0;
+
+	reg = dw_pcie_readw_dbi(pci, cap_ptr);
+	cap_id = (reg & 0x00ff);
+
+	if (cap_id > PCI_CAP_ID_MAX)
+		return 0;
+
+	if (cap_id == cap)
+		return cap_ptr;
+
+	next_cap_ptr = (reg & 0xff00) >> 8;
+	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+}
+
+u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
+{
+	u8 next_cap_ptr;
+	u16 reg;
+
+	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
+	next_cap_ptr = (reg & 0x00ff);
+
+	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+}
+EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
+
 int dw_pcie_read(void __iomem *addr, int size, u32 *val)
 {
 	if (!IS_ALIGNED((uintptr_t)addr, size)) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 14762e262758..6cb978132469 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -251,6 +251,8 @@  struct dw_pcie {
 #define to_dw_pcie_from_ep(endpoint)   \
 		container_of((endpoint), struct dw_pcie, ep)
 
+u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
+
 int dw_pcie_read(void __iomem *addr, int size, u32 *val);
 int dw_pcie_write(void __iomem *addr, int size, u32 val);