| Message ID | 20230214140858.1133292-8-rick.wertenbroek@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: rockchip: Fix RK3399 PCIe endpoint controller driver | expand |
On 2/14/23 23:08, Rick Wertenbroek wrote: > Fix legacy IRQ generation for RK3399 PCIe endpoint core according to > the technical reference manual (TRM). Assert and deassert legacy > interrupt (INTx) through the legacy interrupt control register > ("PCIE_CLIENT_LEGACY_INT_CTRL") instead of manually generating a PCIe > message. The generation of the legacy interrupt was tested and validated > with the PCIe endpoint test driver. > > Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") > Cc: stable@vger.kernel.org > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> Some nits below. But otherwise works fine for me. > --- > drivers/pci/controller/pcie-rockchip-ep.c | 38 +++++------------------ > drivers/pci/controller/pcie-rockchip.h | 6 ++++ > 2 files changed, 14 insertions(+), 30 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > index cbc281a6a..ca5b363ba 100644 > --- a/drivers/pci/controller/pcie-rockchip-ep.c > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > @@ -328,45 +328,23 @@ static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn, > u8 intx, bool is_asserted) > { > struct rockchip_pcie *rockchip = &ep->rockchip; > - u32 r = ep->max_regions - 1; > - u32 offset; > - u32 status; > - u8 msg_code; > - > - if (unlikely(ep->irq_pci_addr != ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR || > - ep->irq_pci_fn != fn)) { > - rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r, > - AXI_WRAPPER_NOR_MSG, > - ep->irq_phys_addr, 0, 0); > - ep->irq_pci_addr = ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR; > - ep->irq_pci_fn = fn; > - } > > intx &= 3; > if (is_asserted) { > ep->irq_pending |= BIT(intx); > - msg_code = ROCKCHIP_PCIE_MSG_CODE_ASSERT_INTA + intx; > } else { > ep->irq_pending &= ~BIT(intx); > - msg_code = ROCKCHIP_PCIE_MSG_CODE_DEASSERT_INTA + intx; > } > > - status = rockchip_pcie_read(rockchip, > - ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > - ROCKCHIP_PCIE_EP_CMD_STATUS); > - status &= ROCKCHIP_PCIE_EP_CMD_STATUS_IS; > - > - if ((status != 0) ^ (ep->irq_pending != 0)) { > - status ^= ROCKCHIP_PCIE_EP_CMD_STATUS_IS; > - rockchip_pcie_write(rockchip, status, > - ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + > - ROCKCHIP_PCIE_EP_CMD_STATUS); > + if (is_asserted) { > + rockchip_pcie_write(rockchip, > + PCIE_CLIENT_INT_IN_ASSERT | PCIE_CLIENT_INT_PEND_ST_PEND, > + PCIE_CLIENT_LEGACY_INT_CTRL); > + } else { > + rockchip_pcie_write(rockchip, > + PCIE_CLIENT_INT_IN_DEASSERT | PCIE_CLIENT_INT_PEND_ST_NORMAL, > + PCIE_CLIENT_LEGACY_INT_CTRL); > } With this change, you have now twice "if (is_asserted) {", which is not necessary. You can simplify the code a bit: static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn, u8 intx, bool do_assert) { u8 msg_code; intx &= 3; if (do_assert) { ep->irq_pending |= BIT(intx); msg_code = ROCKCHIP_PCIE_MSG_CODE_ASSERT_INTA + intx; rockchip_pcie_write(&ep->rockchip, PCIE_CLIENT_INT_IN_ASSERT | PCIE_CLIENT_INT_PEND_ST_PEND, PCIE_CLIENT_LEGACY_INT_CTRL); return; } ep->irq_pending &= ~BIT(intx); msg_code = ROCKCHIP_PCIE_MSG_CODE_DEASSERT_INTA + intx; rockchip_pcie_write(&ep->rockchip, PCIE_CLIENT_INT_IN_DEASSERT | PCIE_CLIENT_INT_PEND_ST_NORMAL, PCIE_CLIENT_LEGACY_INT_CTRL); } Note also the renaming of the argument "is_asserted" to "do_assert". The name is_asserted is badly misleading considering the english meaning given that it is true when we *must* do the assert and false when we must deassert. So do_assert as a name better match the use of that argument I think. > - > - offset = > - ROCKCHIP_PCIE_MSG_ROUTING(ROCKCHIP_PCIE_MSG_ROUTING_LOCAL_INTX) | > - ROCKCHIP_PCIE_MSG_CODE(msg_code) | ROCKCHIP_PCIE_MSG_NO_DATA; > - writel(0, ep->irq_cpu_addr + offset); > } > > static int rockchip_pcie_ep_send_legacy_irq(struct rockchip_pcie_ep *ep, u8 fn, > diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h > index 72e427a0f..e90c2a2b8 100644 > --- a/drivers/pci/controller/pcie-rockchip.h > +++ b/drivers/pci/controller/pcie-rockchip.h > @@ -39,6 +39,12 @@ > #define PCIE_CLIENT_GEN_SEL_1 HIWORD_UPDATE(0x0080, 0) > #define PCIE_CLIENT_GEN_SEL_2 HIWORD_UPDATE_BIT(0x0080) > #define PCIE_CLIENT_SIDE_BAND_STATUS (PCIE_CLIENT_BASE + 0x20) > +#define PCIE_CLIENT_LEGACY_INT_CTRL (PCIE_CLIENT_BASE + 0x0c) > +#define PCIE_CLIENT_INT_IN_ASSERT HIWORD_UPDATE_BIT(0x0002) > +#define PCIE_CLIENT_INT_IN_DEASSERT HIWORD_UPDATE(0x0002, 0) > +#define PCIE_CLIENT_INT_PEND_ST_PEND HIWORD_UPDATE_BIT(0x0001) > +#define PCIE_CLIENT_INT_PEND_ST_NORMAL HIWORD_UPDATE(0x0001, 0) > +#define PCIE_CLIENT_SIDE_BAND_STATUS (PCIE_CLIENT_BASE + 0x20) > #define PCIE_CLIENT_PHY_ST BIT(12) > #define PCIE_CLIENT_DEBUG_OUT_0 (PCIE_CLIENT_BASE + 0x3c) > #define PCIE_CLIENT_DEBUG_LTSSM_MASK GENMASK(5, 0)
On 2/14/23 23:08, Rick Wertenbroek wrote: > Fix legacy IRQ generation for RK3399 PCIe endpoint core according to > the technical reference manual (TRM). Assert and deassert legacy > interrupt (INTx) through the legacy interrupt control register > ("PCIE_CLIENT_LEGACY_INT_CTRL") instead of manually generating a PCIe > message. The generation of the legacy interrupt was tested and validated > with the PCIe endpoint test driver. > > Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") > Cc: stable@vger.kernel.org > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> > --- > drivers/pci/controller/pcie-rockchip-ep.c | 38 +++++------------------ > drivers/pci/controller/pcie-rockchip.h | 6 ++++ > 2 files changed, 14 insertions(+), 30 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > index cbc281a6a..ca5b363ba 100644 > --- a/drivers/pci/controller/pcie-rockchip-ep.c > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > @@ -328,45 +328,23 @@ static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn, > u8 intx, bool is_asserted) > { > struct rockchip_pcie *rockchip = &ep->rockchip; > - u32 r = ep->max_regions - 1; > - u32 offset; > - u32 status; > - u8 msg_code; > - > - if (unlikely(ep->irq_pci_addr != ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR || > - ep->irq_pci_fn != fn)) { > - rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r, > - AXI_WRAPPER_NOR_MSG, > - ep->irq_phys_addr, 0, 0); > - ep->irq_pci_addr = ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR; By the way, ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR is now unused. Remove it too please.
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c index cbc281a6a..ca5b363ba 100644 --- a/drivers/pci/controller/pcie-rockchip-ep.c +++ b/drivers/pci/controller/pcie-rockchip-ep.c @@ -328,45 +328,23 @@ static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn, u8 intx, bool is_asserted) { struct rockchip_pcie *rockchip = &ep->rockchip; - u32 r = ep->max_regions - 1; - u32 offset; - u32 status; - u8 msg_code; - - if (unlikely(ep->irq_pci_addr != ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR || - ep->irq_pci_fn != fn)) { - rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r, - AXI_WRAPPER_NOR_MSG, - ep->irq_phys_addr, 0, 0); - ep->irq_pci_addr = ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR; - ep->irq_pci_fn = fn; - } intx &= 3; if (is_asserted) { ep->irq_pending |= BIT(intx); - msg_code = ROCKCHIP_PCIE_MSG_CODE_ASSERT_INTA + intx; } else { ep->irq_pending &= ~BIT(intx); - msg_code = ROCKCHIP_PCIE_MSG_CODE_DEASSERT_INTA + intx; } - status = rockchip_pcie_read(rockchip, - ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + - ROCKCHIP_PCIE_EP_CMD_STATUS); - status &= ROCKCHIP_PCIE_EP_CMD_STATUS_IS; - - if ((status != 0) ^ (ep->irq_pending != 0)) { - status ^= ROCKCHIP_PCIE_EP_CMD_STATUS_IS; - rockchip_pcie_write(rockchip, status, - ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + - ROCKCHIP_PCIE_EP_CMD_STATUS); + if (is_asserted) { + rockchip_pcie_write(rockchip, + PCIE_CLIENT_INT_IN_ASSERT | PCIE_CLIENT_INT_PEND_ST_PEND, + PCIE_CLIENT_LEGACY_INT_CTRL); + } else { + rockchip_pcie_write(rockchip, + PCIE_CLIENT_INT_IN_DEASSERT | PCIE_CLIENT_INT_PEND_ST_NORMAL, + PCIE_CLIENT_LEGACY_INT_CTRL); } - - offset = - ROCKCHIP_PCIE_MSG_ROUTING(ROCKCHIP_PCIE_MSG_ROUTING_LOCAL_INTX) | - ROCKCHIP_PCIE_MSG_CODE(msg_code) | ROCKCHIP_PCIE_MSG_NO_DATA; - writel(0, ep->irq_cpu_addr + offset); } static int rockchip_pcie_ep_send_legacy_irq(struct rockchip_pcie_ep *ep, u8 fn, diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h index 72e427a0f..e90c2a2b8 100644 --- a/drivers/pci/controller/pcie-rockchip.h +++ b/drivers/pci/controller/pcie-rockchip.h @@ -39,6 +39,12 @@ #define PCIE_CLIENT_GEN_SEL_1 HIWORD_UPDATE(0x0080, 0) #define PCIE_CLIENT_GEN_SEL_2 HIWORD_UPDATE_BIT(0x0080) #define PCIE_CLIENT_SIDE_BAND_STATUS (PCIE_CLIENT_BASE + 0x20) +#define PCIE_CLIENT_LEGACY_INT_CTRL (PCIE_CLIENT_BASE + 0x0c) +#define PCIE_CLIENT_INT_IN_ASSERT HIWORD_UPDATE_BIT(0x0002) +#define PCIE_CLIENT_INT_IN_DEASSERT HIWORD_UPDATE(0x0002, 0) +#define PCIE_CLIENT_INT_PEND_ST_PEND HIWORD_UPDATE_BIT(0x0001) +#define PCIE_CLIENT_INT_PEND_ST_NORMAL HIWORD_UPDATE(0x0001, 0) +#define PCIE_CLIENT_SIDE_BAND_STATUS (PCIE_CLIENT_BASE + 0x20) #define PCIE_CLIENT_PHY_ST BIT(12) #define PCIE_CLIENT_DEBUG_OUT_0 (PCIE_CLIENT_BASE + 0x3c) #define PCIE_CLIENT_DEBUG_LTSSM_MASK GENMASK(5, 0)
Fix legacy IRQ generation for RK3399 PCIe endpoint core according to the technical reference manual (TRM). Assert and deassert legacy interrupt (INTx) through the legacy interrupt control register ("PCIE_CLIENT_LEGACY_INT_CTRL") instead of manually generating a PCIe message. The generation of the legacy interrupt was tested and validated with the PCIe endpoint test driver. Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") Cc: stable@vger.kernel.org Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> --- drivers/pci/controller/pcie-rockchip-ep.c | 38 +++++------------------ drivers/pci/controller/pcie-rockchip.h | 6 ++++ 2 files changed, 14 insertions(+), 30 deletions(-)