diff mbox series

[v20,04/19] PCI: designware-ep: Add INTx IRQs support

Message ID 20230825093219.2685912-5-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand

Commit Message

Yoshihiro Shimoda Aug. 25, 2023, 9:32 a.m. UTC
Add support for triggering INTx IRQs by using outbound iATU.
Outbound iATU is utilized to send assert and de-assert INTA TLPs
as simulated edge IRQ for INTA. (Other INT[BCD] are not asserted.)
This INTx support is optional (if there is no memory for INTx,
probe will not fail).

The message is generated based on the payloadless Msg TLP with type
0x14, where 0x4 is the routing code implying the Terminate at
Receiver message. The message code is specified as b1000xx for
the INTx assertion and b1001xx for the INTx de-assertion.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 70 +++++++++++++++++--
 drivers/pci/controller/dwc/pcie-designware.h  |  2 +
 2 files changed, 68 insertions(+), 4 deletions(-)

Comments

Bjorn Helgaas Sept. 13, 2023, 11:31 p.m. UTC | #1
On Fri, Aug 25, 2023 at 06:32:04PM +0900, Yoshihiro Shimoda wrote:
> Add support for triggering INTx IRQs by using outbound iATU.
> Outbound iATU is utilized to send assert and de-assert INTA TLPs
> as simulated edge IRQ for INTA. (Other INT[BCD] are not asserted.)
> This INTx support is optional (if there is no memory for INTx,
> probe will not fail).
> 
> The message is generated based on the payloadless Msg TLP with type
> 0x14, where 0x4 is the routing code implying the Terminate at
> Receiver message. The message code is specified as b1000xx for
> the INTx assertion and b1001xx for the INTx de-assertion.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 70 +++++++++++++++++--
>  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
>  2 files changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 747d5bc07222..91e3c4335031 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -6,9 +6,11 @@
>   * Author: Kishon Vijay Abraham I <kishon@ti.com>
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  
> +#include "../../pci.h"
>  #include "pcie-designware.h"
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> @@ -484,14 +486,61 @@ static const struct pci_epc_ops epc_ops = {
>  	.get_features		= dw_pcie_ep_get_features,
>  };
>  
> +static int dw_pcie_ep_send_msg(struct dw_pcie_ep *ep, u8 func_no, u8 code,
> +			       u8 routing)
> +{
> +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> +	struct pci_epc *epc = ep->epc;
> +	int ret;
> +
> +	atu.func_no = func_no;
> +	atu.code = code;
> +	atu.routing = routing;
> +	atu.type = PCIE_ATU_TYPE_MSG;
> +	atu.cpu_addr = ep->intx_mem_phys;
> +	atu.size = epc->mem->window.page_size;
> +
> +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> +	if (ret)
> +		return ret;
> +
> +	/* A dummy-write ep->intx_mem is converted to a Msg TLP */
> +	writel(0, ep->intx_mem);
> +
> +	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->intx_mem_phys);
> +
> +	return 0;
> +}
> +
>  int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct device *dev = pci->dev;
> +	int ret;
>  
> -	dev_err(dev, "EP cannot trigger legacy IRQs\n");
> +	if (!ep->intx_mem) {
> +		dev_err(dev, "legacy IRQs not supported\n");
> +		return -EOPNOTSUPP;
> +	}
>  
> -	return -EINVAL;
> +	/*
> +	 * Even though the PCI bus specification implies the level-triggered
> +	 * INTx interrupts the kernel PCIe endpoint framework has a single
> +	 * PCI_EPC_IRQ_INTx flag defined for the legacy IRQs simulation. Thus
> +	 * this function sends the Deassert_INTx PCIe TLP after the Assert_INTx
> +	 * message with the 50 usec duration basically implementing the
> +	 * rising-edge triggering IRQ. Hopefully the interrupt controller will
> +	 * still be able to register the incoming IRQ event...

I'm not really convinced about this "assert INTA, wait 50us, deassert
INTA" thing.  All the INTx language in the spec is like this:

  ... the virtual INTx wire must be asserted whenever and *as long as*
  the following conditions are satisfied:

    - The Interrupt Disable bit in the Command register is set to 0b.

    - The <feature> Interrupt Enable bit in the <feature> Control
      Register is set to 1b.

    - The <feature> Status bit in the <feature> Status register is
      set.

E.g., sec PCIe r6.0, sec 5.5.6 (Link Activation), 6.1.6 (Native PME),
6.2.4.1.2 (AER Interrupt Generation), 6.2.11.1 (DPC Interrupts),
6.7.3.4 (Software Notification of Hot-Plug Events).

So it seems to me like the endpoint needs an "interrupt status" bit,
and the Deassert_INTx message would be sent when the host interrupt
handler clears that bit.

> +	 */
> +	ret = dw_pcie_ep_send_msg(ep, func_no, PCI_MSG_CODE_ASSERT_INTA,
> +				  PCI_MSG_TYPE_R_LOCAL);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(50, 100);
> +
> +	return dw_pcie_ep_send_msg(ep, func_no, PCI_MSG_CODE_DEASSERT_INTA,
> +				   PCI_MSG_TYPE_R_LOCAL);
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_ep_raise_legacy_irq);
>  
> @@ -622,6 +671,10 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>  
>  	dw_pcie_edma_remove(pci);
>  
> +	if (ep->intx_mem)
> +		pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep->intx_mem,
> +				      epc->mem->window.page_size);
> +
>  	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
>  			      epc->mem->window.page_size);
>  
> @@ -793,9 +846,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		goto err_exit_epc_mem;
>  	}
>  
> +	ep->intx_mem = pci_epc_mem_alloc_addr(epc, &ep->intx_mem_phys,
> +					      epc->mem->window.page_size);
> +	if (!ep->intx_mem)
> +		dev_warn(dev, "Failed to reserve memory for INTx\n");
> +
>  	ret = dw_pcie_edma_detect(pci);
>  	if (ret)
> -		goto err_free_epc_mem;
> +		goto err_free_epc_mem_intx;
>  
>  	if (ep->ops->get_features) {
>  		epc_features = ep->ops->get_features(ep);
> @@ -812,7 +870,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  err_remove_edma:
>  	dw_pcie_edma_remove(pci);
>  
> -err_free_epc_mem:
> +err_free_epc_mem_intx:
> +	if (ep->intx_mem)
> +		pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep->intx_mem,
> +				      epc->mem->window.page_size);
> +
>  	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
>  			      epc->mem->window.page_size);
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 8f22a7bc0523..e02d4986bc2b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -376,6 +376,8 @@ struct dw_pcie_ep {
>  	unsigned long		*ob_window_map;
>  	void __iomem		*msi_mem;
>  	phys_addr_t		msi_mem_phys;
> +	void __iomem		*intx_mem;
> +	phys_addr_t		intx_mem_phys;
>  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
>  };
>  
> -- 
> 2.25.1
>
Yoshihiro Shimoda Sept. 14, 2023, 7:56 a.m. UTC | #2
Hi Bjorn,

> From: Bjorn Helgaas, Sent: Thursday, September 14, 2023 8:31 AM
> 
> On Fri, Aug 25, 2023 at 06:32:04PM +0900, Yoshihiro Shimoda wrote:
> > Add support for triggering INTx IRQs by using outbound iATU.
> > Outbound iATU is utilized to send assert and de-assert INTA TLPs
> > as simulated edge IRQ for INTA. (Other INT[BCD] are not asserted.)
> > This INTx support is optional (if there is no memory for INTx,
> > probe will not fail).
> >
> > The message is generated based on the payloadless Msg TLP with type
> > 0x14, where 0x4 is the routing code implying the Terminate at
> > Receiver message. The message code is specified as b1000xx for
> > the INTx assertion and b1001xx for the INTx de-assertion.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 70 +++++++++++++++++--
> >  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
> >  2 files changed, 68 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 747d5bc07222..91e3c4335031 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -6,9 +6,11 @@
> >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> >   */
> >
> > +#include <linux/delay.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >
> > +#include "../../pci.h"
> >  #include "pcie-designware.h"
> >  #include <linux/pci-epc.h>
> >  #include <linux/pci-epf.h>
> > @@ -484,14 +486,61 @@ static const struct pci_epc_ops epc_ops = {
> >  	.get_features		= dw_pcie_ep_get_features,
> >  };
> >
> > +static int dw_pcie_ep_send_msg(struct dw_pcie_ep *ep, u8 func_no, u8 code,
> > +			       u8 routing)
> > +{
> > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > +	struct pci_epc *epc = ep->epc;
> > +	int ret;
> > +
> > +	atu.func_no = func_no;
> > +	atu.code = code;
> > +	atu.routing = routing;
> > +	atu.type = PCIE_ATU_TYPE_MSG;
> > +	atu.cpu_addr = ep->intx_mem_phys;
> > +	atu.size = epc->mem->window.page_size;
> > +
> > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* A dummy-write ep->intx_mem is converted to a Msg TLP */
> > +	writel(0, ep->intx_mem);
> > +
> > +	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->intx_mem_phys);
> > +
> > +	return 0;
> > +}
> > +
> >  int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >  	struct device *dev = pci->dev;
> > +	int ret;
> >
> > -	dev_err(dev, "EP cannot trigger legacy IRQs\n");
> > +	if (!ep->intx_mem) {
> > +		dev_err(dev, "legacy IRQs not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> >
> > -	return -EINVAL;
> > +	/*
> > +	 * Even though the PCI bus specification implies the level-triggered
> > +	 * INTx interrupts the kernel PCIe endpoint framework has a single
> > +	 * PCI_EPC_IRQ_INTx flag defined for the legacy IRQs simulation. Thus
> > +	 * this function sends the Deassert_INTx PCIe TLP after the Assert_INTx
> > +	 * message with the 50 usec duration basically implementing the
> > +	 * rising-edge triggering IRQ. Hopefully the interrupt controller will
> > +	 * still be able to register the incoming IRQ event...
> 
> I'm not really convinced about this "assert INTA, wait 50us, deassert
> INTA" thing.  All the INTx language in the spec is like this:
> 
>   ... the virtual INTx wire must be asserted whenever and *as long as*
>   the following conditions are satisfied:
> 
>     - The Interrupt Disable bit in the Command register is set to 0b.
> 
>     - The <feature> Interrupt Enable bit in the <feature> Control
>       Register is set to 1b.
> 
>     - The <feature> Status bit in the <feature> Status register is
>       set.
> 
> E.g., sec PCIe r6.0, sec 5.5.6 (Link Activation), 6.1.6 (Native PME),
> 6.2.4.1.2 (AER Interrupt Generation), 6.2.11.1 (DPC Interrupts),
> 6.7.3.4 (Software Notification of Hot-Plug Events).
> 
> So it seems to me like the endpoint needs an "interrupt status" bit,
> and the Deassert_INTx message would be sent when the host interrupt
> handler clears that bit.

Thank you very much for your comments! About this topic, 
Frank also has a similar opinion before [1]. So, I asked Kishon
about this, but I didn't get any comment from Kishon at that time.
Anyway, to handle INTx on PCIe endpoint framework properly,
we need to modify the PCIe Endpoint framework, IIUC.

Should I modify the PCIe Endpoint framework at first?
Or, can this patch be applied as-is? 
I guess that such modification of the PCIe Endpoint framework
need much time. So, if I should modify the framework at first,
I would like to drop adding INTx support [2] from my patch series
because supporting INTx on my PCIe controller is not mandatory.

[1]
https://lore.kernel.org/linux-pci/TYBPR01MB5341EFAC471AEBB9100D6051D8719@TYBPR01MB5341.jpnprd01.prod.outlook.com/

[2]
The following patches are not needed if INTx support should be dropped:

eb185e1e628a PCI: designware-ep: Add INTx IRQs support
5d0e51f85b23 PCI: dwc: Add outbound MSG TLPs support
4758bef61cc2 PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()
44938b13046b PCI: Add INTx Mechanism Messages macros

Best regards,
Yoshihiro Shimoda

> > +	 */
> > +	ret = dw_pcie_ep_send_msg(ep, func_no, PCI_MSG_CODE_ASSERT_INTA,
> > +				  PCI_MSG_TYPE_R_LOCAL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	usleep_range(50, 100);
> > +
> > +	return dw_pcie_ep_send_msg(ep, func_no, PCI_MSG_CODE_DEASSERT_INTA,
> > +				   PCI_MSG_TYPE_R_LOCAL);
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_ep_raise_legacy_irq);
> >
> > @@ -622,6 +671,10 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
> >
> >  	dw_pcie_edma_remove(pci);
> >
> > +	if (ep->intx_mem)
> > +		pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep->intx_mem,
> > +				      epc->mem->window.page_size);
> > +
> >  	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
> >  			      epc->mem->window.page_size);
> >
> > @@ -793,9 +846,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		goto err_exit_epc_mem;
> >  	}
> >
> > +	ep->intx_mem = pci_epc_mem_alloc_addr(epc, &ep->intx_mem_phys,
> > +					      epc->mem->window.page_size);
> > +	if (!ep->intx_mem)
> > +		dev_warn(dev, "Failed to reserve memory for INTx\n");
> > +
> >  	ret = dw_pcie_edma_detect(pci);
> >  	if (ret)
> > -		goto err_free_epc_mem;
> > +		goto err_free_epc_mem_intx;
> >
> >  	if (ep->ops->get_features) {
> >  		epc_features = ep->ops->get_features(ep);
> > @@ -812,7 +870,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  err_remove_edma:
> >  	dw_pcie_edma_remove(pci);
> >
> > -err_free_epc_mem:
> > +err_free_epc_mem_intx:
> > +	if (ep->intx_mem)
> > +		pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep->intx_mem,
> > +				      epc->mem->window.page_size);
> > +
> >  	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
> >  			      epc->mem->window.page_size);
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 8f22a7bc0523..e02d4986bc2b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -376,6 +376,8 @@ struct dw_pcie_ep {
> >  	unsigned long		*ob_window_map;
> >  	void __iomem		*msi_mem;
> >  	phys_addr_t		msi_mem_phys;
> > +	void __iomem		*intx_mem;
> > +	phys_addr_t		intx_mem_phys;
> >  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> >  };
> >
> > --
> > 2.25.1
> >
Bjorn Helgaas Sept. 15, 2023, 9:23 p.m. UTC | #3
On Thu, Sep 14, 2023 at 07:56:21AM +0000, Yoshihiro Shimoda wrote:
> > From: Bjorn Helgaas, Sent: Thursday, September 14, 2023 8:31 AM
> > On Fri, Aug 25, 2023 at 06:32:04PM +0900, Yoshihiro Shimoda wrote:
> > > Add support for triggering INTx IRQs by using outbound iATU.
> > > Outbound iATU is utilized to send assert and de-assert INTA TLPs
> > > as simulated edge IRQ for INTA. (Other INT[BCD] are not asserted.)
> > > This INTx support is optional (if there is no memory for INTx,
> > > probe will not fail).
> > >
> > > The message is generated based on the payloadless Msg TLP with type
> > > 0x14, where 0x4 is the routing code implying the Terminate at
> > > Receiver message. The message code is specified as b1000xx for
> > > the INTx assertion and b1001xx for the INTx de-assertion.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > ---
> > >  .../pci/controller/dwc/pcie-designware-ep.c   | 70 +++++++++++++++++--
> > >  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
> > >  2 files changed, 68 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 747d5bc07222..91e3c4335031 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -6,9 +6,11 @@
> > >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> > >   */
> > >
> > > +#include <linux/delay.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >
> > > +#include "../../pci.h"
> > >  #include "pcie-designware.h"
> > >  #include <linux/pci-epc.h>
> > >  #include <linux/pci-epf.h>
> > > @@ -484,14 +486,61 @@ static const struct pci_epc_ops epc_ops = {
> > >  	.get_features		= dw_pcie_ep_get_features,
> > >  };
> > >
> > > +static int dw_pcie_ep_send_msg(struct dw_pcie_ep *ep, u8 func_no, u8 code,
> > > +			       u8 routing)
> > > +{
> > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > +	struct pci_epc *epc = ep->epc;
> > > +	int ret;
> > > +
> > > +	atu.func_no = func_no;
> > > +	atu.code = code;
> > > +	atu.routing = routing;
> > > +	atu.type = PCIE_ATU_TYPE_MSG;
> > > +	atu.cpu_addr = ep->intx_mem_phys;
> > > +	atu.size = epc->mem->window.page_size;
> > > +
> > > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* A dummy-write ep->intx_mem is converted to a Msg TLP */
> > > +	writel(0, ep->intx_mem);
> > > +
> > > +	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->intx_mem_phys);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no)
> > >  {
> > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >  	struct device *dev = pci->dev;
> > > +	int ret;
> > >
> > > -	dev_err(dev, "EP cannot trigger legacy IRQs\n");
> > > +	if (!ep->intx_mem) {
> > > +		dev_err(dev, "legacy IRQs not supported\n");
> > > +		return -EOPNOTSUPP;
> > > +	}
> > >
> > > -	return -EINVAL;
> > > +	/*
> > > +	 * Even though the PCI bus specification implies the level-triggered
> > > +	 * INTx interrupts the kernel PCIe endpoint framework has a single
> > > +	 * PCI_EPC_IRQ_INTx flag defined for the legacy IRQs simulation. Thus
> > > +	 * this function sends the Deassert_INTx PCIe TLP after the Assert_INTx
> > > +	 * message with the 50 usec duration basically implementing the
> > > +	 * rising-edge triggering IRQ. Hopefully the interrupt controller will
> > > +	 * still be able to register the incoming IRQ event...
> > 
> > I'm not really convinced about this "assert INTA, wait 50us, deassert
> > INTA" thing.  All the INTx language in the spec is like this:
> > 
> >   ... the virtual INTx wire must be asserted whenever and *as long as*
> >   the following conditions are satisfied:
> > 
> >     - The Interrupt Disable bit in the Command register is set to 0b.
> > 
> >     - The <feature> Interrupt Enable bit in the <feature> Control
> >       Register is set to 1b.
> > 
> >     - The <feature> Status bit in the <feature> Status register is
> >       set.
> > 
> > E.g., sec PCIe r6.0, sec 5.5.6 (Link Activation), 6.1.6 (Native PME),
> > 6.2.4.1.2 (AER Interrupt Generation), 6.2.11.1 (DPC Interrupts),
> > 6.7.3.4 (Software Notification of Hot-Plug Events).
> > 
> > So it seems to me like the endpoint needs an "interrupt status" bit,
> > and the Deassert_INTx message would be sent when the host interrupt
> > handler clears that bit.
> 
> Thank you very much for your comments! About this topic, 
> Frank also has a similar opinion before [1]. So, I asked Kishon
> about this, but I didn't get any comment from Kishon at that time.
> Anyway, to handle INTx on PCIe endpoint framework properly,
> we need to modify the PCIe Endpoint framework, IIUC.
> 
> Should I modify the PCIe Endpoint framework at first?
> Or, can this patch be applied as-is? 
> I guess that such modification of the PCIe Endpoint framework
> need much time. So, if I should modify the framework at first,
> I would like to drop adding INTx support [2] from my patch series
> because supporting INTx on my PCIe controller is not mandatory.
> 
> [1]
> https://lore.kernel.org/linux-pci/TYBPR01MB5341EFAC471AEBB9100D6051D8719@TYBPR01MB5341.jpnprd01.prod.outlook.com/
> 
> [2]
> The following patches are not needed if INTx support should be dropped:
> 
> eb185e1e628a PCI: designware-ep: Add INTx IRQs support
> 5d0e51f85b23 PCI: dwc: Add outbound MSG TLPs support
> 4758bef61cc2 PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()
> 44938b13046b PCI: Add INTx Mechanism Messages macros

Since INTx support isn't mandatory at this time, I think we should
drop these patches for now.  If/when somebody definitely needs INTx
support, we can resurrect them, and then we'll have more clarity on
how it should work and we'll be better able to test it.

Bjorn
Yoshihiro Shimoda Sept. 19, 2023, 7:22 a.m. UTC | #4
Hello Bjorn,

> From: Bjorn Helgaas, Sent: Saturday, September 16, 2023 6:24 AM
> 
> On Thu, Sep 14, 2023 at 07:56:21AM +0000, Yoshihiro Shimoda wrote:
> > > From: Bjorn Helgaas, Sent: Thursday, September 14, 2023 8:31 AM
> > > On Fri, Aug 25, 2023 at 06:32:04PM +0900, Yoshihiro Shimoda wrote:
> > > > Add support for triggering INTx IRQs by using outbound iATU.
> > > > Outbound iATU is utilized to send assert and de-assert INTA TLPs
> > > > as simulated edge IRQ for INTA. (Other INT[BCD] are not asserted.)
> > > > This INTx support is optional (if there is no memory for INTx,
> > > > probe will not fail).
> > > >
> > > > The message is generated based on the payloadless Msg TLP with type
> > > > 0x14, where 0x4 is the routing code implying the Terminate at
> > > > Receiver message. The message code is specified as b1000xx for
> > > > the INTx assertion and b1001xx for the INTx de-assertion.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > > ---
> > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 70 +++++++++++++++++--
> > > >  drivers/pci/controller/dwc/pcie-designware.h  |  2 +
> > > >  2 files changed, 68 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 747d5bc07222..91e3c4335031 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -6,9 +6,11 @@
> > > >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> > > >   */
> > > >
> > > > +#include <linux/delay.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > >
> > > > +#include "../../pci.h"
> > > >  #include "pcie-designware.h"
> > > >  #include <linux/pci-epc.h>
> > > >  #include <linux/pci-epf.h>
> > > > @@ -484,14 +486,61 @@ static const struct pci_epc_ops epc_ops = {
> > > >  	.get_features		= dw_pcie_ep_get_features,
> > > >  };
> > > >
> > > > +static int dw_pcie_ep_send_msg(struct dw_pcie_ep *ep, u8 func_no, u8 code,
> > > > +			       u8 routing)
> > > > +{
> > > > +	struct dw_pcie_ob_atu_cfg atu = { 0 };
> > > > +	struct pci_epc *epc = ep->epc;
> > > > +	int ret;
> > > > +
> > > > +	atu.func_no = func_no;
> > > > +	atu.code = code;
> > > > +	atu.routing = routing;
> > > > +	atu.type = PCIE_ATU_TYPE_MSG;
> > > > +	atu.cpu_addr = ep->intx_mem_phys;
> > > > +	atu.size = epc->mem->window.page_size;
> > > > +
> > > > +	ret = dw_pcie_ep_outbound_atu(ep, &atu);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* A dummy-write ep->intx_mem is converted to a Msg TLP */
> > > > +	writel(0, ep->intx_mem);
> > > > +
> > > > +	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->intx_mem_phys);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no)
> > > >  {
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > >  	struct device *dev = pci->dev;
> > > > +	int ret;
> > > >
> > > > -	dev_err(dev, "EP cannot trigger legacy IRQs\n");
> > > > +	if (!ep->intx_mem) {
> > > > +		dev_err(dev, "legacy IRQs not supported\n");
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > >
> > > > -	return -EINVAL;
> > > > +	/*
> > > > +	 * Even though the PCI bus specification implies the level-triggered
> > > > +	 * INTx interrupts the kernel PCIe endpoint framework has a single
> > > > +	 * PCI_EPC_IRQ_INTx flag defined for the legacy IRQs simulation. Thus
> > > > +	 * this function sends the Deassert_INTx PCIe TLP after the Assert_INTx
> > > > +	 * message with the 50 usec duration basically implementing the
> > > > +	 * rising-edge triggering IRQ. Hopefully the interrupt controller will
> > > > +	 * still be able to register the incoming IRQ event...
> > >
> > > I'm not really convinced about this "assert INTA, wait 50us, deassert
> > > INTA" thing.  All the INTx language in the spec is like this:
> > >
> > >   ... the virtual INTx wire must be asserted whenever and *as long as*
> > >   the following conditions are satisfied:
> > >
> > >     - The Interrupt Disable bit in the Command register is set to 0b.
> > >
> > >     - The <feature> Interrupt Enable bit in the <feature> Control
> > >       Register is set to 1b.
> > >
> > >     - The <feature> Status bit in the <feature> Status register is
> > >       set.
> > >
> > > E.g., sec PCIe r6.0, sec 5.5.6 (Link Activation), 6.1.6 (Native PME),
> > > 6.2.4.1.2 (AER Interrupt Generation), 6.2.11.1 (DPC Interrupts),
> > > 6.7.3.4 (Software Notification of Hot-Plug Events).
> > >
> > > So it seems to me like the endpoint needs an "interrupt status" bit,
> > > and the Deassert_INTx message would be sent when the host interrupt
> > > handler clears that bit.
> >
> > Thank you very much for your comments! About this topic,
> > Frank also has a similar opinion before [1]. So, I asked Kishon
> > about this, but I didn't get any comment from Kishon at that time.
> > Anyway, to handle INTx on PCIe endpoint framework properly,
> > we need to modify the PCIe Endpoint framework, IIUC.
> >
> > Should I modify the PCIe Endpoint framework at first?
> > Or, can this patch be applied as-is?
> > I guess that such modification of the PCIe Endpoint framework
> > need much time. So, if I should modify the framework at first,
> > I would like to drop adding INTx support [2] from my patch series
> > because supporting INTx on my PCIe controller is not mandatory.
> >
> > [1]
> > https://lore.kernel.org/linux-pci/TYBPR01MB5341EFAC471AEBB9100D6051D8719@TYBPR01MB5341.jpnprd01.prod.outlook.com/
> >
> > [2]
> > The following patches are not needed if INTx support should be dropped:
> >
> > eb185e1e628a PCI: designware-ep: Add INTx IRQs support
> > 5d0e51f85b23 PCI: dwc: Add outbound MSG TLPs support
> > 4758bef61cc2 PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()
> > 44938b13046b PCI: Add INTx Mechanism Messages macros
> 
> Since INTx support isn't mandatory at this time, I think we should
> drop these patches for now.  If/when somebody definitely needs INTx
> support, we can resurrect them, and then we'll have more clarity on
> how it should work and we'll be better able to test it.

I got it.

In this case, should I submit the patch series as v21? Or, like the previous
time [1], should I submit some patches for squashing the controller/rcar branch?

[1] https://lore.kernel.org/linux-pci/20230901131711.2861283-1-yoshihiro.shimoda.uh@renesas.com/

Best regards,
Yoshihiro Shimoda

> Bjorn
Bjorn Helgaas Sept. 19, 2023, 10:39 a.m. UTC | #5
On Tue, Sep 19, 2023 at 07:22:48AM +0000, Yoshihiro Shimoda wrote:
> > From: Bjorn Helgaas, Sent: Saturday, September 16, 2023 6:24 AM
> > On Thu, Sep 14, 2023 at 07:56:21AM +0000, Yoshihiro Shimoda wrote:
> > ...

> > > The following patches are not needed if INTx support should be dropped:
> > >
> > > eb185e1e628a PCI: designware-ep: Add INTx IRQs support
> > > 5d0e51f85b23 PCI: dwc: Add outbound MSG TLPs support
> > > 4758bef61cc2 PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()
> > > 44938b13046b PCI: Add INTx Mechanism Messages macros
> > 
> > Since INTx support isn't mandatory at this time, I think we should
> > drop these patches for now.  If/when somebody definitely needs INTx
> > support, we can resurrect them, and then we'll have more clarity on
> > how it should work and we'll be better able to test it.
> 
> I got it.
> 
> In this case, should I submit the patch series as v21?

I think it will be simpler if you post a v21 that includes squashing
the R-Car drivers together as well as dropping these INTx patches.

Bjorn
Yoshihiro Shimoda Sept. 19, 2023, 11:55 a.m. UTC | #6
Hello Bjorn,

> From: Bjorn Helgaas, Sent: Tuesday, September 19, 2023 7:40 PM
> 
> On Tue, Sep 19, 2023 at 07:22:48AM +0000, Yoshihiro Shimoda wrote:
> > > From: Bjorn Helgaas, Sent: Saturday, September 16, 2023 6:24 AM
> > > On Thu, Sep 14, 2023 at 07:56:21AM +0000, Yoshihiro Shimoda wrote:
> > > ...
> 
> > > > The following patches are not needed if INTx support should be dropped:
> > > >
> > > > eb185e1e628a PCI: designware-ep: Add INTx IRQs support
> > > > 5d0e51f85b23 PCI: dwc: Add outbound MSG TLPs support
> > > > 4758bef61cc2 PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()
> > > > 44938b13046b PCI: Add INTx Mechanism Messages macros
> > >
> > > Since INTx support isn't mandatory at this time, I think we should
> > > drop these patches for now.  If/when somebody definitely needs INTx
> > > support, we can resurrect them, and then we'll have more clarity on
> > > how it should work and we'll be better able to test it.
> >
> > I got it.
> >
> > In this case, should I submit the patch series as v21?
> 
> I think it will be simpler if you post a v21 that includes squashing
> the R-Car drivers together as well as dropping these INTx patches.

Thank you for your reply! I'll make such a patch series.

Best regards,
Yoshihiro Shimoda

> Bjorn
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 747d5bc07222..91e3c4335031 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -6,9 +6,11 @@ 
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
+#include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 
+#include "../../pci.h"
 #include "pcie-designware.h"
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
@@ -484,14 +486,61 @@  static const struct pci_epc_ops epc_ops = {
 	.get_features		= dw_pcie_ep_get_features,
 };
 
+static int dw_pcie_ep_send_msg(struct dw_pcie_ep *ep, u8 func_no, u8 code,
+			       u8 routing)
+{
+	struct dw_pcie_ob_atu_cfg atu = { 0 };
+	struct pci_epc *epc = ep->epc;
+	int ret;
+
+	atu.func_no = func_no;
+	atu.code = code;
+	atu.routing = routing;
+	atu.type = PCIE_ATU_TYPE_MSG;
+	atu.cpu_addr = ep->intx_mem_phys;
+	atu.size = epc->mem->window.page_size;
+
+	ret = dw_pcie_ep_outbound_atu(ep, &atu);
+	if (ret)
+		return ret;
+
+	/* A dummy-write ep->intx_mem is converted to a Msg TLP */
+	writel(0, ep->intx_mem);
+
+	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->intx_mem_phys);
+
+	return 0;
+}
+
 int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct device *dev = pci->dev;
+	int ret;
 
-	dev_err(dev, "EP cannot trigger legacy IRQs\n");
+	if (!ep->intx_mem) {
+		dev_err(dev, "legacy IRQs not supported\n");
+		return -EOPNOTSUPP;
+	}
 
-	return -EINVAL;
+	/*
+	 * Even though the PCI bus specification implies the level-triggered
+	 * INTx interrupts the kernel PCIe endpoint framework has a single
+	 * PCI_EPC_IRQ_INTx flag defined for the legacy IRQs simulation. Thus
+	 * this function sends the Deassert_INTx PCIe TLP after the Assert_INTx
+	 * message with the 50 usec duration basically implementing the
+	 * rising-edge triggering IRQ. Hopefully the interrupt controller will
+	 * still be able to register the incoming IRQ event...
+	 */
+	ret = dw_pcie_ep_send_msg(ep, func_no, PCI_MSG_CODE_ASSERT_INTA,
+				  PCI_MSG_TYPE_R_LOCAL);
+	if (ret)
+		return ret;
+
+	usleep_range(50, 100);
+
+	return dw_pcie_ep_send_msg(ep, func_no, PCI_MSG_CODE_DEASSERT_INTA,
+				   PCI_MSG_TYPE_R_LOCAL);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_raise_legacy_irq);
 
@@ -622,6 +671,10 @@  void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 
 	dw_pcie_edma_remove(pci);
 
+	if (ep->intx_mem)
+		pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep->intx_mem,
+				      epc->mem->window.page_size);
+
 	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
 			      epc->mem->window.page_size);
 
@@ -793,9 +846,14 @@  int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		goto err_exit_epc_mem;
 	}
 
+	ep->intx_mem = pci_epc_mem_alloc_addr(epc, &ep->intx_mem_phys,
+					      epc->mem->window.page_size);
+	if (!ep->intx_mem)
+		dev_warn(dev, "Failed to reserve memory for INTx\n");
+
 	ret = dw_pcie_edma_detect(pci);
 	if (ret)
-		goto err_free_epc_mem;
+		goto err_free_epc_mem_intx;
 
 	if (ep->ops->get_features) {
 		epc_features = ep->ops->get_features(ep);
@@ -812,7 +870,11 @@  int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 err_remove_edma:
 	dw_pcie_edma_remove(pci);
 
-err_free_epc_mem:
+err_free_epc_mem_intx:
+	if (ep->intx_mem)
+		pci_epc_mem_free_addr(epc, ep->intx_mem_phys, ep->intx_mem,
+				      epc->mem->window.page_size);
+
 	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
 			      epc->mem->window.page_size);
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 8f22a7bc0523..e02d4986bc2b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -376,6 +376,8 @@  struct dw_pcie_ep {
 	unsigned long		*ob_window_map;
 	void __iomem		*msi_mem;
 	phys_addr_t		msi_mem_phys;
+	void __iomem		*intx_mem;
+	phys_addr_t		intx_mem_phys;
 	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
 };