diff mbox series

[RFC,01/10] PCI: dwc: Add MSI-X callbacks handler

Message ID 77b7b2687e9618d3f7d1f11c3fc6ecec9a9442ef.1523379766.git.gustavo.pimentel@synopsys.com
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series Adds pcitest tool support for MSI-X | expand

Commit Message

Gustavo Pimentel April 10, 2018, 5:14 p.m. UTC
Changes the pcie_raise_irq function signature, namely the interrupt_num
variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
of 2048.

Implements a PCIe config space capability iterator function to search and
save the MSI and MSI-X pointers. With this method the code becomes more
generic and flexible.

Implements MSI-X set/get functions for sysfs interface in order to change
the EP entries number.

Implements EP MSI-X interface for triggering interruptions.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/dwc/pci-dra7xx.c           |   2 +-
 drivers/pci/dwc/pcie-artpec6.c         |   2 +-
 drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
 drivers/pci/dwc/pcie-designware-plat.c |   6 +-
 drivers/pci/dwc/pcie-designware.h      |  23 +++++-
 5 files changed, 173 insertions(+), 5 deletions(-)

Comments

Kishon Vijay Abraham I April 16, 2018, 9:29 a.m. UTC | #1
Hi Gustavo,

On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
> Changes the pcie_raise_irq function signature, namely the interrupt_num
> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
> of 2048.
> 
> Implements a PCIe config space capability iterator function to search and
> save the MSI and MSI-X pointers. With this method the code becomes more
> generic and flexible.
> 
> Implements MSI-X set/get functions for sysfs interface in order to change
> the EP entries number.
> 
> Implements EP MSI-X interface for triggering interruptions.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>  drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>  5 files changed, 173 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index ed8558d..5265725 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>  }
>  
>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> -				 enum pci_epc_irq_type type, u8 interrupt_num)
> +				 enum pci_epc_irq_type type, u16 interrupt_num)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
> index e66cede..96dc259 100644
> --- a/drivers/pci/dwc/pcie-artpec6.c
> +++ b/drivers/pci/dwc/pcie-artpec6.c
> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>  }
>  
>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> -				  enum pci_epc_irq_type type, u8 interrupt_num)
> +				  enum pci_epc_irq_type type, u16 interrupt_num)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index 15b22a6..874d4c2 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>  }
>  
> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
> +{

This should be implemented in a generic way similar to pci_find_capability().
It'll be useful when we try to implement other capabilities as well.

Thanks
Kishon
Gustavo Pimentel April 23, 2018, 9:36 a.m. UTC | #2
Hi Kishon,

On 16/04/2018 10:29, Kishon Vijay Abraham I wrote:
> Hi Gustavo,
> 
> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>> of 2048.
>>
>> Implements a PCIe config space capability iterator function to search and
>> save the MSI and MSI-X pointers. With this method the code becomes more
>> generic and flexible.
>>
>> Implements MSI-X set/get functions for sysfs interface in order to change
>> the EP entries number.
>>
>> Implements EP MSI-X interface for triggering interruptions.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>  drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>> index ed8558d..5265725 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>>  }
>>  
>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> -				 enum pci_epc_irq_type type, u8 interrupt_num)
>> +				 enum pci_epc_irq_type type, u16 interrupt_num)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
>> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
>> index e66cede..96dc259 100644
>> --- a/drivers/pci/dwc/pcie-artpec6.c
>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>>  }
>>  
>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> -				  enum pci_epc_irq_type type, u8 interrupt_num)
>> +				  enum pci_epc_irq_type type, u16 interrupt_num)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>  
>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
>> index 15b22a6..874d4c2 100644
>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>  }
>>  
>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
>> +{
> 
> This should be implemented in a generic way similar to pci_find_capability().
> It'll be useful when we try to implement other capabilities as well.

Hum, what you suggest? Something implemented on the pci-epf-core?

> 
> Thanks
> Kishon
> 

Regards,
Gustavo
Kishon Vijay Abraham I April 24, 2018, 7:07 a.m. UTC | #3
Hi,

On Monday 23 April 2018 03:06 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 16/04/2018 10:29, Kishon Vijay Abraham I wrote:
>> Hi Gustavo,
>>
>> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>>> of 2048.
>>>
>>> Implements a PCIe config space capability iterator function to search and
>>> save the MSI and MSI-X pointers. With this method the code becomes more
>>> generic and flexible.
>>>
>>> Implements MSI-X set/get functions for sysfs interface in order to change
>>> the EP entries number.
>>>
>>> Implements EP MSI-X interface for triggering interruptions.
>>>
>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>> ---
>>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>>  drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
>>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>>> index ed8558d..5265725 100644
>>> --- a/drivers/pci/dwc/pci-dra7xx.c
>>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>>>  }
>>>  
>>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>> -				 enum pci_epc_irq_type type, u8 interrupt_num)
>>> +				 enum pci_epc_irq_type type, u16 interrupt_num)
>>>  {
>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
>>> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
>>> index e66cede..96dc259 100644
>>> --- a/drivers/pci/dwc/pcie-artpec6.c
>>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>>>  }
>>>  
>>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>> -				  enum pci_epc_irq_type type, u8 interrupt_num)
>>> +				  enum pci_epc_irq_type type, u16 interrupt_num)
>>>  {
>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>  
>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
>>> index 15b22a6..874d4c2 100644
>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>>  }
>>>  
>>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
>>> +{
>>
>> This should be implemented in a generic way similar to pci_find_capability().
>> It'll be useful when we try to implement other capabilities as well.
> 
> Hum, what you suggest? Something implemented on the pci-epf-core?

yeah, Initially thought it could be implemented as a helper function in
pci-epc-core so that both designware and cadence can use it.

But do we really have to find the address like this? since all designware IP's
will have a particular capability at a fixed address offset, why not follow use
existing mechanism in dw_pcie_ep_get_msi?

Or is it possible for a particular capability to have address offsets for
different vendors? How is it for cadence?

Thanks
Kishon
Alan Douglas April 24, 2018, 9:15 a.m. UTC | #4
Hi,

On 10 April 2018 18:15 Gustavo Pimentel wrote:
> Changes the pcie_raise_irq function signature, namely the interrupt_num
> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
> of 2048.
> 
> Implements a PCIe config space capability iterator function to search and save
> the MSI and MSI-X pointers. With this method the code becomes more
> generic and flexible.
> 
> Implements MSI-X set/get functions for sysfs interface in order to change the
> EP entries number.
> 
> Implements EP MSI-X interface for triggering interruptions.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>  drivers/pci/dwc/pcie-designware-ep.c   | 145
> ++++++++++++++++++++++++++++++++-
>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>  5 files changed, 173 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c index
> ed8558d..5265725 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct
> dra7xx_pcie *dra7xx,  }
> 
>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> -				 enum pci_epc_irq_type type, u8
> interrupt_num)
> +				 enum pci_epc_irq_type type, u16
> interrupt_num)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); diff --git
> a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c index
> e66cede..96dc259 100644
> --- a/drivers/pci/dwc/pcie-artpec6.c
> +++ b/drivers/pci/dwc/pcie-artpec6.c
> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep
> *ep)  }
> 
>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> -				  enum pci_epc_irq_type type, u8
> interrupt_num)
> +				  enum pci_epc_irq_type type, u16
> interrupt_num)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> 
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-
> designware-ep.c
> index 15b22a6..874d4c2 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci,
> enum pci_barno bar)
>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>  }
> 
> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep) {
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	u8 next_ptr, curr_ptr, cap_id;
> +	u16 reg;
> +
> +	memset(&ep->cap_addr, 0, sizeof(ep->cap_addr));
> +
> +	reg = dw_pcie_readw_dbi(pci, PCI_STATUS);
> +	if (!(reg & PCI_STATUS_CAP_LIST))
> +		return;
> +
> +	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> +	next_ptr = (reg & 0x00ff);
> +	if (!next_ptr)
> +		return;
> +
> +	reg = dw_pcie_readw_dbi(pci, next_ptr);
> +	curr_ptr = next_ptr;
> +	next_ptr = (reg & 0xff00) >> 8;
> +	cap_id = (reg & 0x00ff);
> +
> +	while (next_ptr && (cap_id <= PCI_CAP_ID_MAX)) {
> +		switch (cap_id) {
> +		case PCI_CAP_ID_MSI:
> +			ep->cap_addr.msi_addr = curr_ptr;
> +			break;
> +		case PCI_CAP_ID_MSIX:
> +			ep->cap_addr.msix_addr = curr_ptr;
> +			break;
> +		}
> +		reg = dw_pcie_readw_dbi(pci, next_ptr);
> +		curr_ptr = next_ptr;
> +		next_ptr = (reg & 0xff00) >> 8;
> +		cap_id = (reg & 0x00ff);
> +	}
> +}
> +
>  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>  				   struct pci_epf_header *hdr)
>  {
> @@ -241,8 +279,47 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc,
> u8 func_no, u8 encode_int)
>  	return 0;
>  }
> 
> +static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no) {
> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	u32 val, reg;
> +
> +	if (ep->cap_addr.msix_addr == 0)
> +		return 0;
> +
> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
> +	val = dw_pcie_readw_dbi(pci, reg);
> +	if (!(val & PCI_MSIX_FLAGS_ENABLE))
> +		return -EINVAL;
> +
> +	val &= PCI_MSIX_FLAGS_QSIZE;
> +
> +	return val;
> +}
> +
> +static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u16
> +interrupts) {
> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	u32 val, reg;
> +
> +	if (ep->cap_addr.msix_addr == 0)
> +		return 0;
> +
> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
> +	val = dw_pcie_readw_dbi(pci, reg);
> +	val &= ~PCI_MSIX_FLAGS_QSIZE;
> +	val |= interrupts;
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writew_dbi(pci, reg, val);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
> +}
> +
>  static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
> -				enum pci_epc_irq_type type, u8
> interrupt_num)
> +				enum pci_epc_irq_type type, u16
> interrupt_num)
>  {
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> 
> @@ -282,6 +359,8 @@ static const struct pci_epc_ops epc_ops = {
>  	.unmap_addr		= dw_pcie_ep_unmap_addr,
>  	.set_msi		= dw_pcie_ep_set_msi,
>  	.get_msi		= dw_pcie_ep_get_msi,
> +	.set_msix		= dw_pcie_ep_set_msix,
> +	.get_msix		= dw_pcie_ep_get_msix,
>  	.raise_irq		= dw_pcie_ep_raise_irq,
>  	.start			= dw_pcie_ep_start,
>  	.stop			= dw_pcie_ep_stop,
> @@ -322,6 +401,60 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep
> *ep, u8 func_no,
>  	return 0;
>  }
> 
> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> +			     u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct pci_epc *epc = ep->epc;
> +	u16 tbl_offset, bir;
> +	u32 bar_addr_upper, bar_addr_lower;
> +	u32 msg_addr_upper, msg_addr_lower;
> +	u32 reg, msg_data;
> +	u64 tbl_addr, msg_addr, reg_u64;
> +	void __iomem *msix_tbl;
> +	int ret;
> +
> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_TABLE;
> +	tbl_offset = dw_pcie_readl_dbi(pci, reg);
> +	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> +	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
> +	tbl_offset >>= 3;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bir);
> +	bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
> +	reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
> +	if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
> +		bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);
> +	else
> +		bar_addr_upper = 0;
> +
> +	tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;
> +	tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
> +	tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;
> +
> +	msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr, ep->addr_size);
> +	if (!msix_tbl)
> +		return -EINVAL;
> +
I think you need to check the mask bit in  vector control for the requested IRQ.
You could set the pending bit if masked, but would then need some output 
signal to inform when the mask bit is cleared (or poll it) so the message can be sent
later.

Also, do you need to check PCI_MSIX_FLAGS_ENABLE here as well, or is it checked earlier?

Regards,
Alan
Gustavo Pimentel April 24, 2018, 9:36 a.m. UTC | #5
Hi Kishon,

On 24/04/2018 08:07, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Monday 23 April 2018 03:06 PM, Gustavo Pimentel wrote:
>> Hi Kishon,
>>
>> On 16/04/2018 10:29, Kishon Vijay Abraham I wrote:
>>> Hi Gustavo,
>>>
>>> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>>>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>>>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>>>> of 2048.
>>>>
>>>> Implements a PCIe config space capability iterator function to search and
>>>> save the MSI and MSI-X pointers. With this method the code becomes more
>>>> generic and flexible.
>>>>
>>>> Implements MSI-X set/get functions for sysfs interface in order to change
>>>> the EP entries number.
>>>>
>>>> Implements EP MSI-X interface for triggering interruptions.
>>>>
>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> ---
>>>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>>>  drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
>>>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>>>> index ed8558d..5265725 100644
>>>> --- a/drivers/pci/dwc/pci-dra7xx.c
>>>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>>>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>>>>  }
>>>>  
>>>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>> -				 enum pci_epc_irq_type type, u8 interrupt_num)
>>>> +				 enum pci_epc_irq_type type, u16 interrupt_num)
>>>>  {
>>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
>>>> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
>>>> index e66cede..96dc259 100644
>>>> --- a/drivers/pci/dwc/pcie-artpec6.c
>>>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>>>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>>>>  }
>>>>  
>>>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>> -				  enum pci_epc_irq_type type, u8 interrupt_num)
>>>> +				  enum pci_epc_irq_type type, u16 interrupt_num)
>>>>  {
>>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>  
>>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
>>>> index 15b22a6..874d4c2 100644
>>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>>>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>>>  }
>>>>  
>>>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
>>>> +{
>>>
>>> This should be implemented in a generic way similar to pci_find_capability().
>>> It'll be useful when we try to implement other capabilities as well.
>>
>> Hum, what you suggest? Something implemented on the pci-epf-core?
> 
> yeah, Initially thought it could be implemented as a helper function in
> pci-epc-core so that both designware and cadence can use it.

That would be nice, however I couldn't find out how to access the config space,
through the pci_epf or pci_epc structs.

So, I reworked the functions like this:

(on pcie-designware-ep.c)

u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
                              u8 cap)
{
        u8 cap_id, next_cap_ptr;
        u16 reg;

        reg = dw_pcie_readw_dbi(pci, cap_ptr);
        next_cap_ptr = (reg & 0xff00) >> 8;
        cap_id = (reg & 0x00ff);

        if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
                return 0;

        if (cap_id == cap)
                return cap_ptr;

        return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
}

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

        if (!next_cap_ptr)
                return 0;

        return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
}

int dw_pcie_ep_init(struct dw_pcie_ep *ep)
{
[...]
        ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
        ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
[...]
}

> 
> But do we really have to find the address like this? since all designware IP's
> will have a particular capability at a fixed address offset, why not follow use
> existing mechanism in dw_pcie_ep_get_msi?

The capabilities are not fixed to a specific address offset by default they
assume those values, but they can be easily change at design stage.

> 
> Or is it possible for a particular capability to have address offsets for
> different vendors? How is it for cadence?

Yes, it's possible to have different address offset for different vendors.

> 
> Thanks
> Kishon
> 

Thanks,
Gustavo
Kishon Vijay Abraham I April 24, 2018, 11:24 a.m. UTC | #6
Hi,

On Tuesday 24 April 2018 03:06 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 24/04/2018 08:07, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Monday 23 April 2018 03:06 PM, Gustavo Pimentel wrote:
>>> Hi Kishon,
>>>
>>> On 16/04/2018 10:29, Kishon Vijay Abraham I wrote:
>>>> Hi Gustavo,
>>>>
>>>> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>>>>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>>>>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>>>>> of 2048.
>>>>>
>>>>> Implements a PCIe config space capability iterator function to search and
>>>>> save the MSI and MSI-X pointers. With this method the code becomes more
>>>>> generic and flexible.
>>>>>
>>>>> Implements MSI-X set/get functions for sysfs interface in order to change
>>>>> the EP entries number.
>>>>>
>>>>> Implements EP MSI-X interface for triggering interruptions.
>>>>>
>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>> ---
>>>>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>>>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>>>>  drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
>>>>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>>>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>>>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>>>>> index ed8558d..5265725 100644
>>>>> --- a/drivers/pci/dwc/pci-dra7xx.c
>>>>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>>>>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>>>>>  }
>>>>>  
>>>>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>>> -				 enum pci_epc_irq_type type, u8 interrupt_num)
>>>>> +				 enum pci_epc_irq_type type, u16 interrupt_num)
>>>>>  {
>>>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
>>>>> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
>>>>> index e66cede..96dc259 100644
>>>>> --- a/drivers/pci/dwc/pcie-artpec6.c
>>>>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>>>>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>>>>>  }
>>>>>  
>>>>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>>> -				  enum pci_epc_irq_type type, u8 interrupt_num)
>>>>> +				  enum pci_epc_irq_type type, u16 interrupt_num)
>>>>>  {
>>>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>>  
>>>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
>>>>> index 15b22a6..874d4c2 100644
>>>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>>>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>>>>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>>>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>>>>  }
>>>>>  
>>>>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
>>>>> +{
>>>>
>>>> This should be implemented in a generic way similar to pci_find_capability().
>>>> It'll be useful when we try to implement other capabilities as well.
>>>
>>> Hum, what you suggest? Something implemented on the pci-epf-core?
>>
>> yeah, Initially thought it could be implemented as a helper function in
>> pci-epc-core so that both designware and cadence can use it.
> 
> That would be nice, however I couldn't find out how to access the config space,
> through the pci_epf or pci_epc structs.

It's just a helper function so it can directly take the base address of the
configuration space as argument (in our case, it should be dbi_base).

Thanks
Kishon

> 
> So, I reworked the functions like this:
> 
> (on pcie-designware-ep.c)
> 
> u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>                               u8 cap)
> {
>         u8 cap_id, next_cap_ptr;
>         u16 reg;
> 
>         reg = dw_pcie_readw_dbi(pci, cap_ptr);
>         next_cap_ptr = (reg & 0xff00) >> 8;
>         cap_id = (reg & 0x00ff);
> 
>         if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
>                 return 0;
> 
>         if (cap_id == cap)
>                 return cap_ptr;
> 
>         return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> }
> 
> 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);
> 
>         if (!next_cap_ptr)
>                 return 0;
> 
>         return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> }
> 
> int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> {
> [...]
>         ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
>         ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
> [...]
> }
> 
>>
>> But do we really have to find the address like this? since all designware IP's
>> will have a particular capability at a fixed address offset, why not follow use
>> existing mechanism in dw_pcie_ep_get_msi?
> 
> The capabilities are not fixed to a specific address offset by default they
> assume those values, but they can be easily change at design stage.
> 
>>
>> Or is it possible for a particular capability to have address offsets for
>> different vendors? How is it for cadence?
> 
> Yes, it's possible to have different address offset for different vendors.
> 
>>
>> Thanks
>> Kishon
>>
> 
> Thanks,
> Gustavo
>
Gustavo Pimentel April 24, 2018, 11:43 a.m. UTC | #7
On 24/04/2018 10:15, Alan Douglas wrote:
> Hi,
> 
> On 10 April 2018 18:15 Gustavo Pimentel wrote:
>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>> of 2048.
>>
>> Implements a PCIe config space capability iterator function to search and save
>> the MSI and MSI-X pointers. With this method the code becomes more
>> generic and flexible.
>>
>> Implements MSI-X set/get functions for sysfs interface in order to change the
>> EP entries number.
>>
>> Implements EP MSI-X interface for triggering interruptions.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>  drivers/pci/dwc/pcie-designware-ep.c   | 145
>> ++++++++++++++++++++++++++++++++-
>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c index
>> ed8558d..5265725 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct
>> dra7xx_pcie *dra7xx,  }
>>
>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> -				 enum pci_epc_irq_type type, u8
>> interrupt_num)
>> +				 enum pci_epc_irq_type type, u16
>> interrupt_num)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); diff --git
>> a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c index
>> e66cede..96dc259 100644
>> --- a/drivers/pci/dwc/pcie-artpec6.c
>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep
>> *ep)  }
>>
>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> -				  enum pci_epc_irq_type type, u8
>> interrupt_num)
>> +				  enum pci_epc_irq_type type, u16
>> interrupt_num)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>
>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-
>> designware-ep.c
>> index 15b22a6..874d4c2 100644
>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci,
>> enum pci_barno bar)
>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>  }
>>
>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep) {
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	u8 next_ptr, curr_ptr, cap_id;
>> +	u16 reg;
>> +
>> +	memset(&ep->cap_addr, 0, sizeof(ep->cap_addr));
>> +
>> +	reg = dw_pcie_readw_dbi(pci, PCI_STATUS);
>> +	if (!(reg & PCI_STATUS_CAP_LIST))
>> +		return;
>> +
>> +	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>> +	next_ptr = (reg & 0x00ff);
>> +	if (!next_ptr)
>> +		return;
>> +
>> +	reg = dw_pcie_readw_dbi(pci, next_ptr);
>> +	curr_ptr = next_ptr;
>> +	next_ptr = (reg & 0xff00) >> 8;
>> +	cap_id = (reg & 0x00ff);
>> +
>> +	while (next_ptr && (cap_id <= PCI_CAP_ID_MAX)) {
>> +		switch (cap_id) {
>> +		case PCI_CAP_ID_MSI:
>> +			ep->cap_addr.msi_addr = curr_ptr;
>> +			break;
>> +		case PCI_CAP_ID_MSIX:
>> +			ep->cap_addr.msix_addr = curr_ptr;
>> +			break;
>> +		}
>> +		reg = dw_pcie_readw_dbi(pci, next_ptr);
>> +		curr_ptr = next_ptr;
>> +		next_ptr = (reg & 0xff00) >> 8;
>> +		cap_id = (reg & 0x00ff);
>> +	}
>> +}
>> +
>>  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>>  				   struct pci_epf_header *hdr)
>>  {
>> @@ -241,8 +279,47 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc,
>> u8 func_no, u8 encode_int)
>>  	return 0;
>>  }
>>
>> +static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no) {
>> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	u32 val, reg;
>> +
>> +	if (ep->cap_addr.msix_addr == 0)
>> +		return 0;
>> +
>> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
>> +	val = dw_pcie_readw_dbi(pci, reg);
>> +	if (!(val & PCI_MSIX_FLAGS_ENABLE))
>> +		return -EINVAL;
>> +
>> +	val &= PCI_MSIX_FLAGS_QSIZE;
>> +
>> +	return val;
>> +}
>> +
>> +static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u16
>> +interrupts) {
>> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	u32 val, reg;
>> +
>> +	if (ep->cap_addr.msix_addr == 0)
>> +		return 0;
>> +
>> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
>> +	val = dw_pcie_readw_dbi(pci, reg);
>> +	val &= ~PCI_MSIX_FLAGS_QSIZE;
>> +	val |= interrupts;
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writew_dbi(pci, reg, val);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>> +
>> +	return 0;
>> +}
>> +
>>  static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>> -				enum pci_epc_irq_type type, u8
>> interrupt_num)
>> +				enum pci_epc_irq_type type, u16
>> interrupt_num)
>>  {
>>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>
>> @@ -282,6 +359,8 @@ static const struct pci_epc_ops epc_ops = {
>>  	.unmap_addr		= dw_pcie_ep_unmap_addr,
>>  	.set_msi		= dw_pcie_ep_set_msi,
>>  	.get_msi		= dw_pcie_ep_get_msi,
>> +	.set_msix		= dw_pcie_ep_set_msix,
>> +	.get_msix		= dw_pcie_ep_get_msix,
>>  	.raise_irq		= dw_pcie_ep_raise_irq,
>>  	.start			= dw_pcie_ep_start,
>>  	.stop			= dw_pcie_ep_stop,
>> @@ -322,6 +401,60 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep
>> *ep, u8 func_no,
>>  	return 0;
>>  }
>>
>> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>> +			     u16 interrupt_num)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct pci_epc *epc = ep->epc;
>> +	u16 tbl_offset, bir;
>> +	u32 bar_addr_upper, bar_addr_lower;
>> +	u32 msg_addr_upper, msg_addr_lower;
>> +	u32 reg, msg_data;
>> +	u64 tbl_addr, msg_addr, reg_u64;
>> +	void __iomem *msix_tbl;
>> +	int ret;
>> +
>> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_TABLE;
>> +	tbl_offset = dw_pcie_readl_dbi(pci, reg);
>> +	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
>> +	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
>> +	tbl_offset >>= 3;
>> +
>> +	reg = PCI_BASE_ADDRESS_0 + (4 * bir);
>> +	bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
>> +	reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
>> +	if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
>> +		bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);
>> +	else
>> +		bar_addr_upper = 0;
>> +
>> +	tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;
>> +	tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
>> +	tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;
>> +
>> +	msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr, ep->addr_size);
>> +	if (!msix_tbl)
>> +		return -EINVAL;
>> +
> I think you need to check the mask bit in  vector control for the requested IRQ.
> You could set the pending bit if masked, but would then need some output 
> signal to inform when the mask bit is cleared (or poll it) so the message can be sent
> later.

I have to analyze this careful and with detail to provide you a valid and good
feedback as soon as possible.

> 
> Also, do you need to check PCI_MSIX_FLAGS_ENABLE here as well, or is it checked earlier?

I applied the same logic for MSI-X already implemented before for the MSI.

Before calling the pci_epc_raise_irq(), the number of interrupts is checked by
calling the pci_epc_get_msi() first, which leads to one of
dw_pcie_ep_get_msi()/cdns_pcie_ep_get_msi().

From what I could see the PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE flags are
check on the dw_pcie_ep_get_msi()/dw_pcie_ep_get_msix() funcitons, the same
logic is applied on cdns_pcie_ep_get_msi().

> 
> Regards,
> Alan
>
Alan Douglas April 24, 2018, 2:05 p.m. UTC | #8
Hi Kishon,

On 24 April 2018 10:36 Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 24/04/2018 08:07, Kishon Vijay Abraham I wrote:
> > Hi,
> >
> > On Monday 23 April 2018 03:06 PM, Gustavo Pimentel wrote:
> >> Hi Kishon,
> >>
> >> On 16/04/2018 10:29, Kishon Vijay Abraham I wrote:
> >>> Hi Gustavo,
> >>>
> >>> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
> >>>> Changes the pcie_raise_irq function signature, namely the
> >>>> interrupt_num variable type from u8 to u16 to accommodate the MSI-X
> >>>> maximum interrupts of 2048.
> >>>>
> >>>> Implements a PCIe config space capability iterator function to
> >>>> search and save the MSI and MSI-X pointers. With this method the
> >>>> code becomes more generic and flexible.
> >>>>
> >>>> Implements MSI-X set/get functions for sysfs interface in order to
> >>>> change the EP entries number.
> >>>>
> >>>> Implements EP MSI-X interface for triggering interruptions.
> >>>>
> >>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> >>>> ---
> >>>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
> >>>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
> >>>>  drivers/pci/dwc/pcie-designware-ep.c   | 145
> ++++++++++++++++++++++++++++++++-
> >>>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
> >>>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
> >>>>  5 files changed, 173 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c
> >>>> b/drivers/pci/dwc/pci-dra7xx.c index ed8558d..5265725 100644
> >>>> --- a/drivers/pci/dwc/pci-dra7xx.c
> >>>> +++ b/drivers/pci/dwc/pci-dra7xx.c
> >>>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct
> >>>> dra7xx_pcie *dra7xx,  }
> >>>>
> >>>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >>>> -				 enum pci_epc_irq_type type, u8
> interrupt_num)
> >>>> +				 enum pci_epc_irq_type type, u16
> interrupt_num)
> >>>>  {
> >>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >>>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); diff --git
> >>>> a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
> >>>> index e66cede..96dc259 100644
> >>>> --- a/drivers/pci/dwc/pcie-artpec6.c
> >>>> +++ b/drivers/pci/dwc/pcie-artpec6.c
> >>>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct
> >>>> dw_pcie_ep *ep)  }
> >>>>
> >>>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >>>> -				  enum pci_epc_irq_type type, u8
> interrupt_num)
> >>>> +				  enum pci_epc_irq_type type, u16
> interrupt_num)
> >>>>  {
> >>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >>>>
> >>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c
> >>>> b/drivers/pci/dwc/pcie-designware-ep.c
> >>>> index 15b22a6..874d4c2 100644
> >>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
> >>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> >>>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie
> *pci, enum pci_barno bar)
> >>>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);  }
> >>>>
> >>>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep) {
> >>>
> >>> This should be implemented in a generic way similar to
> pci_find_capability().
> >>> It'll be useful when we try to implement other capabilities as well.
> >>
> >> Hum, what you suggest? Something implemented on the pci-epf-core?
> >
> > yeah, Initially thought it could be implemented as a helper function
> > in pci-epc-core so that both designware and cadence can use it.
> 
> That would be nice, however I couldn't find out how to access the config
> space, through the pci_epf or pci_epc structs.
> 
> So, I reworked the functions like this:
> 
> (on pcie-designware-ep.c)
> 
> u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>                               u8 cap)
> {
>         u8 cap_id, next_cap_ptr;
>         u16 reg;
> 
>         reg = dw_pcie_readw_dbi(pci, cap_ptr);
>         next_cap_ptr = (reg & 0xff00) >> 8;
>         cap_id = (reg & 0x00ff);
> 
>         if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
>                 return 0;
> 
>         if (cap_id == cap)
>                 return cap_ptr;
> 
>         return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap); }
> 
> 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);
> 
>         if (!next_cap_ptr)
>                 return 0;
> 
>         return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap); }
> 
> int dw_pcie_ep_init(struct dw_pcie_ep *ep) { [...]
>         ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
>         ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX); [...]
> }
> 
> >
> > But do we really have to find the address like this? since all
> > designware IP's will have a particular capability at a fixed address
> > offset, why not follow use existing mechanism in dw_pcie_ep_get_msi?
> 
> The capabilities are not fixed to a specific address offset by default they
> assume those values, but they can be easily change at design stage.
> 
> >
> > Or is it possible for a particular capability to have address offsets
> > for different vendors? How is it for cadence?
> 
> Yes, it's possible to have different address offset for different vendors.
> 
For cadence the offsets are fixed for each specific hw (if a capability is 
disabled it will just be removed from the linked list) but it would be 
better to allow for the possibility for them to vary between different hw.

> >
> > Thanks
> > Kishon
> >
> 
> Thanks,
> Gustavo
Regards,
Alan
Gustavo Pimentel April 26, 2018, 3:30 p.m. UTC | #9
Hi Kishon,

On 24/04/2018 12:24, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Tuesday 24 April 2018 03:06 PM, Gustavo Pimentel wrote:
>> Hi Kishon,
>>
>> On 24/04/2018 08:07, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Monday 23 April 2018 03:06 PM, Gustavo Pimentel wrote:
>>>> Hi Kishon,
>>>>
>>>> On 16/04/2018 10:29, Kishon Vijay Abraham I wrote:
>>>>> Hi Gustavo,
>>>>>
>>>>> On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote:
>>>>>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>>>>>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>>>>>> of 2048.
>>>>>>
>>>>>> Implements a PCIe config space capability iterator function to search and
>>>>>> save the MSI and MSI-X pointers. With this method the code becomes more
>>>>>> generic and flexible.
>>>>>>
>>>>>> Implements MSI-X set/get functions for sysfs interface in order to change
>>>>>> the EP entries number.
>>>>>>
>>>>>> Implements EP MSI-X interface for triggering interruptions.
>>>>>>
>>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>>> ---
>>>>>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>>>>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>>>>>  drivers/pci/dwc/pcie-designware-ep.c   | 145 ++++++++++++++++++++++++++++++++-
>>>>>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>>>>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>>>>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>>>>>> index ed8558d..5265725 100644
>>>>>> --- a/drivers/pci/dwc/pci-dra7xx.c
>>>>>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>>>>>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
>>>>>>  }
>>>>>>  
>>>>>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>>>> -				 enum pci_epc_irq_type type, u8 interrupt_num)
>>>>>> +				 enum pci_epc_irq_type type, u16 interrupt_num)
>>>>>>  {
>>>>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
>>>>>> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
>>>>>> index e66cede..96dc259 100644
>>>>>> --- a/drivers/pci/dwc/pcie-artpec6.c
>>>>>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>>>>>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>>>>>>  }
>>>>>>  
>>>>>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>>>> -				  enum pci_epc_irq_type type, u8 interrupt_num)
>>>>>> +				  enum pci_epc_irq_type type, u16 interrupt_num)
>>>>>>  {
>>>>>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>>>>  
>>>>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
>>>>>> index 15b22a6..874d4c2 100644
>>>>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>>>>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>>>>>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>>>>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>>>>>  }
>>>>>>  
>>>>>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
>>>>>> +{
>>>>>
>>>>> This should be implemented in a generic way similar to pci_find_capability().
>>>>> It'll be useful when we try to implement other capabilities as well.
>>>>
>>>> Hum, what you suggest? Something implemented on the pci-epf-core?
>>>
>>> yeah, Initially thought it could be implemented as a helper function in
>>> pci-epc-core so that both designware and cadence can use it.
>>
>> That would be nice, however I couldn't find out how to access the config space,
>> through the pci_epf or pci_epc structs.
> 
> It's just a helper function so it can directly take the base address of the
> configuration space as argument (in our case, it should be dbi_base).

I don't think it will bring much benefit to this particular scope at this time
being. In any case this could be improved later.

Regards,
Gustavo

> 
> Thanks
> Kishon
> 
>>
>> So, I reworked the functions like this:
>>
>> (on pcie-designware-ep.c)
>>
>> u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>>                               u8 cap)
>> {
>>         u8 cap_id, next_cap_ptr;
>>         u16 reg;
>>
>>         reg = dw_pcie_readw_dbi(pci, cap_ptr);
>>         next_cap_ptr = (reg & 0xff00) >> 8;
>>         cap_id = (reg & 0x00ff);
>>
>>         if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
>>                 return 0;
>>
>>         if (cap_id == cap)
>>                 return cap_ptr;
>>
>>         return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>> }
>>
>> 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);
>>
>>         if (!next_cap_ptr)
>>                 return 0;
>>
>>         return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>> }
>>
>> int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> {
>> [...]
>>         ep->msi_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSI);
>>         ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
>> [...]
>> }
>>
>>>
>>> But do we really have to find the address like this? since all designware IP's
>>> will have a particular capability at a fixed address offset, why not follow use
>>> existing mechanism in dw_pcie_ep_get_msi?
>>
>> The capabilities are not fixed to a specific address offset by default they
>> assume those values, but they can be easily change at design stage.
>>
>>>
>>> Or is it possible for a particular capability to have address offsets for
>>> different vendors? How is it for cadence?
>>
>> Yes, it's possible to have different address offset for different vendors.
>>
>>>
>>> Thanks
>>> Kishon
>>>
>>
>> Thanks,
>> Gustavo
>>
Gustavo Pimentel May 10, 2018, 10:40 a.m. UTC | #10
Hi Alan,

Sorry for the delay on the response, I only have time to proper analyze this now.

On 24/04/2018 10:15, Alan Douglas wrote:
> Hi,
> 
> On 10 April 2018 18:15 Gustavo Pimentel wrote:
>> Changes the pcie_raise_irq function signature, namely the interrupt_num
>> variable type from u8 to u16 to accommodate the MSI-X maximum interrupts
>> of 2048.
>>
>> Implements a PCIe config space capability iterator function to search and save
>> the MSI and MSI-X pointers. With this method the code becomes more
>> generic and flexible.
>>
>> Implements MSI-X set/get functions for sysfs interface in order to change the
>> EP entries number.
>>
>> Implements EP MSI-X interface for triggering interruptions.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  drivers/pci/dwc/pci-dra7xx.c           |   2 +-
>>  drivers/pci/dwc/pcie-artpec6.c         |   2 +-
>>  drivers/pci/dwc/pcie-designware-ep.c   | 145
>> ++++++++++++++++++++++++++++++++-
>>  drivers/pci/dwc/pcie-designware-plat.c |   6 +-
>>  drivers/pci/dwc/pcie-designware.h      |  23 +++++-
>>  5 files changed, 173 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c index
>> ed8558d..5265725 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -369,7 +369,7 @@ static void dra7xx_pcie_raise_msi_irq(struct
>> dra7xx_pcie *dra7xx,  }
>>
>>  static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> -				 enum pci_epc_irq_type type, u8
>> interrupt_num)
>> +				 enum pci_epc_irq_type type, u16
>> interrupt_num)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>  	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); diff --git
>> a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c index
>> e66cede..96dc259 100644
>> --- a/drivers/pci/dwc/pcie-artpec6.c
>> +++ b/drivers/pci/dwc/pcie-artpec6.c
>> @@ -428,7 +428,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep
>> *ep)  }
>>
>>  static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> -				  enum pci_epc_irq_type type, u8
>> interrupt_num)
>> +				  enum pci_epc_irq_type type, u16
>> interrupt_num)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>
>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-
>> designware-ep.c
>> index 15b22a6..874d4c2 100644
>> --- a/drivers/pci/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
>> @@ -40,6 +40,44 @@ void dw_pcie_ep_reset_bar(struct dw_pcie *pci,
>> enum pci_barno bar)
>>  	__dw_pcie_ep_reset_bar(pci, bar, 0);
>>  }
>>
>> +void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep) {
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	u8 next_ptr, curr_ptr, cap_id;
>> +	u16 reg;
>> +
>> +	memset(&ep->cap_addr, 0, sizeof(ep->cap_addr));
>> +
>> +	reg = dw_pcie_readw_dbi(pci, PCI_STATUS);
>> +	if (!(reg & PCI_STATUS_CAP_LIST))
>> +		return;
>> +
>> +	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>> +	next_ptr = (reg & 0x00ff);
>> +	if (!next_ptr)
>> +		return;
>> +
>> +	reg = dw_pcie_readw_dbi(pci, next_ptr);
>> +	curr_ptr = next_ptr;
>> +	next_ptr = (reg & 0xff00) >> 8;
>> +	cap_id = (reg & 0x00ff);
>> +
>> +	while (next_ptr && (cap_id <= PCI_CAP_ID_MAX)) {
>> +		switch (cap_id) {
>> +		case PCI_CAP_ID_MSI:
>> +			ep->cap_addr.msi_addr = curr_ptr;
>> +			break;
>> +		case PCI_CAP_ID_MSIX:
>> +			ep->cap_addr.msix_addr = curr_ptr;
>> +			break;
>> +		}
>> +		reg = dw_pcie_readw_dbi(pci, next_ptr);
>> +		curr_ptr = next_ptr;
>> +		next_ptr = (reg & 0xff00) >> 8;
>> +		cap_id = (reg & 0x00ff);
>> +	}
>> +}
>> +
>>  static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
>>  				   struct pci_epf_header *hdr)
>>  {
>> @@ -241,8 +279,47 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc,
>> u8 func_no, u8 encode_int)
>>  	return 0;
>>  }
>>
>> +static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no) {
>> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	u32 val, reg;
>> +
>> +	if (ep->cap_addr.msix_addr == 0)
>> +		return 0;
>> +
>> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
>> +	val = dw_pcie_readw_dbi(pci, reg);
>> +	if (!(val & PCI_MSIX_FLAGS_ENABLE))
>> +		return -EINVAL;
>> +
>> +	val &= PCI_MSIX_FLAGS_QSIZE;
>> +
>> +	return val;
>> +}
>> +
>> +static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u16
>> +interrupts) {
>> +	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	u32 val, reg;
>> +
>> +	if (ep->cap_addr.msix_addr == 0)
>> +		return 0;
>> +
>> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
>> +	val = dw_pcie_readw_dbi(pci, reg);
>> +	val &= ~PCI_MSIX_FLAGS_QSIZE;
>> +	val |= interrupts;
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writew_dbi(pci, reg, val);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>> +
>> +	return 0;
>> +}
>> +
>>  static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
>> -				enum pci_epc_irq_type type, u8
>> interrupt_num)
>> +				enum pci_epc_irq_type type, u16
>> interrupt_num)
>>  {
>>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>>
>> @@ -282,6 +359,8 @@ static const struct pci_epc_ops epc_ops = {
>>  	.unmap_addr		= dw_pcie_ep_unmap_addr,
>>  	.set_msi		= dw_pcie_ep_set_msi,
>>  	.get_msi		= dw_pcie_ep_get_msi,
>> +	.set_msix		= dw_pcie_ep_set_msix,
>> +	.get_msix		= dw_pcie_ep_get_msix,
>>  	.raise_irq		= dw_pcie_ep_raise_irq,
>>  	.start			= dw_pcie_ep_start,
>>  	.stop			= dw_pcie_ep_stop,
>> @@ -322,6 +401,60 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep
>> *ep, u8 func_no,
>>  	return 0;
>>  }
>>
>> +int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>> +			     u16 interrupt_num)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct pci_epc *epc = ep->epc;
>> +	u16 tbl_offset, bir;
>> +	u32 bar_addr_upper, bar_addr_lower;
>> +	u32 msg_addr_upper, msg_addr_lower;
>> +	u32 reg, msg_data;
>> +	u64 tbl_addr, msg_addr, reg_u64;
>> +	void __iomem *msix_tbl;
>> +	int ret;
>> +
>> +	reg = ep->cap_addr.msix_addr + PCI_MSIX_TABLE;
>> +	tbl_offset = dw_pcie_readl_dbi(pci, reg);
>> +	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
>> +	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
>> +	tbl_offset >>= 3;
>> +
>> +	reg = PCI_BASE_ADDRESS_0 + (4 * bir);
>> +	bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
>> +	reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
>> +	if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
>> +		bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);
>> +	else
>> +		bar_addr_upper = 0;
>> +
>> +	tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;
>> +	tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
>> +	tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;
>> +
>> +	msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr, ep->addr_size);
>> +	if (!msix_tbl)
>> +		return -EINVAL;
>> +
> I think you need to check the mask bit in  vector control for the requested IRQ.

Yes, you are right. I'll fix it.

Regards,
Gustavo

> You could set the pending bit if masked, but would then need some output 
> signal to inform when the mask bit is cleared (or poll it) so the message can be sent
> later.
> 
> Also, do you need to check PCI_MSIX_FLAGS_ENABLE here as well, or is it checked earlier?
> 
> Regards,
> Alan
>
diff mbox series

Patch

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index ed8558d..5265725 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -369,7 +369,7 @@  static void dra7xx_pcie_raise_msi_irq(struct dra7xx_pcie *dra7xx,
 }
 
 static int dra7xx_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
-				 enum pci_epc_irq_type type, u8 interrupt_num)
+				 enum pci_epc_irq_type type, u16 interrupt_num)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index e66cede..96dc259 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -428,7 +428,7 @@  static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
 }
 
 static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
-				  enum pci_epc_irq_type type, u8 interrupt_num)
+				  enum pci_epc_irq_type type, u16 interrupt_num)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 15b22a6..874d4c2 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -40,6 +40,44 @@  void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 	__dw_pcie_ep_reset_bar(pci, bar, 0);
 }
 
+void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	u8 next_ptr, curr_ptr, cap_id;
+	u16 reg;
+
+	memset(&ep->cap_addr, 0, sizeof(ep->cap_addr));
+
+	reg = dw_pcie_readw_dbi(pci, PCI_STATUS);
+	if (!(reg & PCI_STATUS_CAP_LIST))
+		return;
+
+	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
+	next_ptr = (reg & 0x00ff);
+	if (!next_ptr)
+		return;
+
+	reg = dw_pcie_readw_dbi(pci, next_ptr);
+	curr_ptr = next_ptr;
+	next_ptr = (reg & 0xff00) >> 8;
+	cap_id = (reg & 0x00ff);
+
+	while (next_ptr && (cap_id <= PCI_CAP_ID_MAX)) {
+		switch (cap_id) {
+		case PCI_CAP_ID_MSI:
+			ep->cap_addr.msi_addr = curr_ptr;
+			break;
+		case PCI_CAP_ID_MSIX:
+			ep->cap_addr.msix_addr = curr_ptr;
+			break;
+		}
+		reg = dw_pcie_readw_dbi(pci, next_ptr);
+		curr_ptr = next_ptr;
+		next_ptr = (reg & 0xff00) >> 8;
+		cap_id = (reg & 0x00ff);
+	}
+}
+
 static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no,
 				   struct pci_epf_header *hdr)
 {
@@ -241,8 +279,47 @@  static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 encode_int)
 	return 0;
 }
 
+static int dw_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no)
+{
+	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	u32 val, reg;
+
+	if (ep->cap_addr.msix_addr == 0)
+		return 0;
+
+	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
+	val = dw_pcie_readw_dbi(pci, reg);
+	if (!(val & PCI_MSIX_FLAGS_ENABLE))
+		return -EINVAL;
+
+	val &= PCI_MSIX_FLAGS_QSIZE;
+
+	return val;
+}
+
+static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u16 interrupts)
+{
+	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	u32 val, reg;
+
+	if (ep->cap_addr.msix_addr == 0)
+		return 0;
+
+	reg = ep->cap_addr.msix_addr + PCI_MSIX_FLAGS;
+	val = dw_pcie_readw_dbi(pci, reg);
+	val &= ~PCI_MSIX_FLAGS_QSIZE;
+	val |= interrupts;
+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writew_dbi(pci, reg, val);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	return 0;
+}
+
 static int dw_pcie_ep_raise_irq(struct pci_epc *epc, u8 func_no,
-				enum pci_epc_irq_type type, u8 interrupt_num)
+				enum pci_epc_irq_type type, u16 interrupt_num)
 {
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 
@@ -282,6 +359,8 @@  static const struct pci_epc_ops epc_ops = {
 	.unmap_addr		= dw_pcie_ep_unmap_addr,
 	.set_msi		= dw_pcie_ep_set_msi,
 	.get_msi		= dw_pcie_ep_get_msi,
+	.set_msix		= dw_pcie_ep_set_msix,
+	.get_msix		= dw_pcie_ep_get_msix,
 	.raise_irq		= dw_pcie_ep_raise_irq,
 	.start			= dw_pcie_ep_start,
 	.stop			= dw_pcie_ep_stop,
@@ -322,6 +401,60 @@  int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 	return 0;
 }
 
+int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
+			     u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct pci_epc *epc = ep->epc;
+	u16 tbl_offset, bir;
+	u32 bar_addr_upper, bar_addr_lower;
+	u32 msg_addr_upper, msg_addr_lower;
+	u32 reg, msg_data;
+	u64 tbl_addr, msg_addr, reg_u64;
+	void __iomem *msix_tbl;
+	int ret;
+
+	reg = ep->cap_addr.msix_addr + PCI_MSIX_TABLE;
+	tbl_offset = dw_pcie_readl_dbi(pci, reg);
+	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
+	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
+	tbl_offset >>= 3;
+
+	reg = PCI_BASE_ADDRESS_0 + (4 * bir);
+	bar_addr_lower = dw_pcie_readl_dbi(pci, reg);
+	reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
+	if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
+		bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);
+	else
+		bar_addr_upper = 0;
+
+	tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;
+	tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
+	tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;
+
+	msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr, ep->addr_size);
+	if (!msix_tbl)
+		return -EINVAL;
+
+	msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR);
+	msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR);
+	msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
+	msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA);
+
+	iounmap(msix_tbl);
+
+	ret = dw_pcie_ep_map_addr(epc, func_no, ep->msix_mem_phys, msg_addr,
+				  epc->mem->page_size);
+	if (ret)
+		return ret;
+
+	writel(msg_data, ep->msix_mem);
+
+	dw_pcie_ep_unmap_addr(epc, func_no, ep->msix_mem_phys);
+
+	return 0;
+}
+
 void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 {
 	struct pci_epc *epc = ep->epc;
@@ -329,6 +462,9 @@  void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
 			      epc->mem->page_size);
 
+	pci_epc_mem_free_addr(epc, ep->msix_mem_phys, ep->msix_mem,
+			      epc->mem->page_size);
+
 	pci_epc_mem_exit(epc);
 }
 
@@ -411,6 +547,13 @@  int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -ENOMEM;
 	}
 
+	ep->msix_mem = pci_epc_mem_alloc_addr(epc, &ep->msix_mem_phys,
+					     epc->mem->page_size);
+	if (!ep->msix_mem) {
+		dev_err(dev, "Failed to reserve memory for MSI-\n");
+		return -ENOMEM;
+	}
+
 	ep->epc = epc;
 	epc_set_drvdata(epc, ep);
 	dw_pcie_setup(pci);
diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
index 2bad68d..c3a4707 100644
--- a/drivers/pci/dwc/pcie-designware-plat.c
+++ b/drivers/pci/dwc/pcie-designware-plat.c
@@ -74,11 +74,13 @@  static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	for (bar = BAR_0; bar <= BAR_5; bar++)
 		dw_pcie_ep_reset_bar(pci, bar);
+
+	dw_pcie_ep_find_cap_addr(ep);
 }
 
 static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 				     enum pci_epc_irq_type type,
-				     u8 interrupt_num)
+				     u16 interrupt_num)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
@@ -88,6 +90,8 @@  static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 		return -EINVAL;
 	case PCI_EPC_IRQ_MSI:
 		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+	case PCI_EPC_IRQ_MSIX:
+		return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
 	default:
 		dev_err(pci->dev, "UNKNOWN IRQ type\n");
 	}
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index bee4e25..456fd94 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -191,7 +191,12 @@  enum dw_pcie_as_type {
 struct dw_pcie_ep_ops {
 	void	(*ep_init)(struct dw_pcie_ep *ep);
 	int	(*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
-			     enum pci_epc_irq_type type, u8 interrupt_num);
+			     enum pci_epc_irq_type type, u16 interrupt_num);
+};
+
+struct dw_pcie_cap_addr {
+	u8	msi_addr;
+	u8	msix_addr;
 };
 
 struct dw_pcie_ep {
@@ -208,6 +213,9 @@  struct dw_pcie_ep {
 	u32			num_ob_windows;
 	void __iomem		*msi_mem;
 	phys_addr_t		msi_mem_phys;
+	void __iomem		*msix_mem;
+	phys_addr_t		msix_mem_phys;
+	struct dw_pcie_cap_addr	cap_addr;
 };
 
 struct dw_pcie_ops {
@@ -359,7 +367,10 @@  int dw_pcie_ep_init(struct dw_pcie_ep *ep);
 void dw_pcie_ep_exit(struct dw_pcie_ep *ep);
 int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 			     u8 interrupt_num);
+int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
+			     u16 interrupt_num);
 void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar);
+void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep);
 #else
 static inline void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 {
@@ -380,8 +391,18 @@  static inline int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 	return 0;
 }
 
+static inline int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
+					   u16 interrupt_num)
+{
+	return 0;
+}
+
 static inline void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 {
 }
+
+static inline void dw_pcie_ep_find_cap_addr(struct dw_pcie_ep *ep)
+{
+}
 #endif
 #endif /* _PCIE_DESIGNWARE_H */