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