| Message ID | 20241116-ep-msi-v8-4-6f1f68ffd1bb@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: EP: Add RC-to-EP doorbell with platform MSI controller | expand |
On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a I don't see 'doorbell_done' defined anywhere. > doorbell address space. > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > callback handler by writing doorbell_data to the mapped doorbell_bar's > address space. > > Set doorbell_done in the doorbell callback to indicate completion. > Same here. > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL 'avoid breaking compatibility between host and endpoint,...' > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > to map one bar's inbound address to MSI space. the command > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > Host side new driver Host side old driver > > EP: new driver S F > EP: old driver F F So the last case of old EP and host drivers will fail? > > S: If EP side support MSI, 'pcitest -B' return success. > If EP side doesn't support MSI, the same to 'F'. > > F: 'pcitest -B' return failure, other case as usual. > > Tested-by: Niklas Cassel <cassel@kernel.org> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v7 to v8 > - rename to pci_epf_align_inbound_addr_lo_hi() > > Change from v6 to v7 > - use help function pci_epf_align_addr_lo_hi() > > Change from v5 to v6 > - rename doorbell_addr to doorbell_offset > > Chagne from v4 to v5 > - Add doorbell free at unbind function. > - Move msi irq handler to here to more complex user case, such as differece > doorbell can use difference handler function. > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > tag or co-developer tag, please let me know. > > change from v3 to v4 > - remove revid requirement > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > - call pci_epc_set_bar() to map inbound address to MSI space only at > COMMAND_ENABLE_DOORBELL. > --- > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > 1 file changed, 117 insertions(+) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index ef6677f34116e..410b2f4bb7ce7 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -11,12 +11,14 @@ > #include <linux/dmaengine.h> > #include <linux/io.h> > #include <linux/module.h> > +#include <linux/msi.h> > #include <linux/slab.h> > #include <linux/pci_ids.h> > #include <linux/random.h> > > #include <linux/pci-epc.h> > #include <linux/pci-epf.h> > +#include <linux/pci-ep-msi.h> > #include <linux/pci_regs.h> > > #define IRQ_TYPE_INTX 0 > @@ -29,6 +31,8 @@ > #define COMMAND_READ BIT(3) > #define COMMAND_WRITE BIT(4) > #define COMMAND_COPY BIT(5) > +#define COMMAND_ENABLE_DOORBELL BIT(6) > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > #define STATUS_READ_SUCCESS BIT(0) > #define STATUS_READ_FAIL BIT(1) > @@ -39,6 +43,11 @@ > #define STATUS_IRQ_RAISED BIT(6) > #define STATUS_SRC_ADDR_INVALID BIT(7) > #define STATUS_DST_ADDR_INVALID BIT(8) > +#define STATUS_DOORBELL_SUCCESS BIT(9) > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > #define FLAG_USE_DMA BIT(0) > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > u32 irq_type; > u32 irq_number; > u32 flags; > + u32 doorbell_bar; > + u32 doorbell_offset; > + u32 doorbell_data; > } __packed; > > static struct pci_epf_header test_header = { > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > } > } > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > +{ > + enum pci_barno bar = reg->doorbell_bar; > + struct pci_epf *epf = epf_test->epf; > + struct pci_epc *epc = epf->epc; > + struct pci_epf_bar db_bar; db_bar = {}; > + struct msi_msg *msg; > + size_t offset; > + int ret; > + > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've allocated proper BAR already. > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > + return; > + } > + > + msg = &epf->db_msg[0].msg; > + ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi, > + &db_bar.phys_addr, &offset); > + > + if (ret) { > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > + return; > + } > + > + reg->doorbell_offset = offset; > + > + db_bar.barno = bar; > + db_bar.size = epf->bar[bar].size; > + db_bar.flags = epf->bar[bar].flags; > + db_bar.addr = NULL; Not needed if you initialize above. > + > + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar); > + if (!ret) > + reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS; > + else > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > +} > + [...] > static const struct pci_epc_event_ops pci_epf_test_event_ops = { > .epc_init = pci_epf_test_epc_init, > .epc_deinit = pci_epf_test_epc_deinit, > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf) > if (ret) > return ret; > > + ret = pci_epf_alloc_doorbell(epf, 1); > + if (!ret) { > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > + struct msi_msg *msg = &epf->db_msg[0].msg; > + enum pci_barno bar; > + > + bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); NO_BAR check? - Mani
On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > I don't see 'doorbell_done' defined anywhere. > > > doorbell address space. > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > address space. > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > Same here. > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > 'avoid breaking compatibility between host and endpoint,...' > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > to map one bar's inbound address to MSI space. the command > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > Host side new driver Host side old driver > > > > EP: new driver S F > > EP: old driver F F > > So the last case of old EP and host drivers will fail? doorbell test will fail if old EP. > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > If EP side doesn't support MSI, the same to 'F'. > > > > F: 'pcitest -B' return failure, other case as usual. > > > > Tested-by: Niklas Cassel <cassel@kernel.org> > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Change from v7 to v8 > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > Change from v6 to v7 > > - use help function pci_epf_align_addr_lo_hi() > > > > Change from v5 to v6 > > - rename doorbell_addr to doorbell_offset > > > > Chagne from v4 to v5 > > - Add doorbell free at unbind function. > > - Move msi irq handler to here to more complex user case, such as differece > > doorbell can use difference handler function. > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > tag or co-developer tag, please let me know. > > > > change from v3 to v4 > > - remove revid requirement > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > COMMAND_ENABLE_DOORBELL. > > --- > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > 1 file changed, 117 insertions(+) > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > index ef6677f34116e..410b2f4bb7ce7 100644 > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > @@ -11,12 +11,14 @@ > > #include <linux/dmaengine.h> > > #include <linux/io.h> > > #include <linux/module.h> > > +#include <linux/msi.h> > > #include <linux/slab.h> > > #include <linux/pci_ids.h> > > #include <linux/random.h> > > > > #include <linux/pci-epc.h> > > #include <linux/pci-epf.h> > > +#include <linux/pci-ep-msi.h> > > #include <linux/pci_regs.h> > > > > #define IRQ_TYPE_INTX 0 > > @@ -29,6 +31,8 @@ > > #define COMMAND_READ BIT(3) > > #define COMMAND_WRITE BIT(4) > > #define COMMAND_COPY BIT(5) > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > #define STATUS_READ_SUCCESS BIT(0) > > #define STATUS_READ_FAIL BIT(1) > > @@ -39,6 +43,11 @@ > > #define STATUS_IRQ_RAISED BIT(6) > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > #define STATUS_DST_ADDR_INVALID BIT(8) > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > #define FLAG_USE_DMA BIT(0) > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > u32 irq_type; > > u32 irq_number; > > u32 flags; > > + u32 doorbell_bar; > > + u32 doorbell_offset; > > + u32 doorbell_data; > > } __packed; > > > > static struct pci_epf_header test_header = { > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > } > > } > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > +{ > > + enum pci_barno bar = reg->doorbell_bar; > > + struct pci_epf *epf = epf_test->epf; > > + struct pci_epc *epc = epf->epc; > > + struct pci_epf_bar db_bar; > > db_bar = {}; > > > + struct msi_msg *msg; > > + size_t offset; > > + int ret; > > + > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > allocated proper BAR already. Not check it at call pci_epf_alloc_doorbell() because it optional feature. It return failure when it actually use it. > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > + return; > > + } > > + > > + msg = &epf->db_msg[0].msg; > > + ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi, > > + &db_bar.phys_addr, &offset); > > + > > + if (ret) { > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > + return; > > + } > > + > > + reg->doorbell_offset = offset; > > + > > + db_bar.barno = bar; > > + db_bar.size = epf->bar[bar].size; > > + db_bar.flags = epf->bar[bar].flags; > > + db_bar.addr = NULL; > > Not needed if you initialize above. > > > + > > + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar); > > + if (!ret) > > + reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS; > > + else > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > +} > > + > > [...] > > > static const struct pci_epc_event_ops pci_epf_test_event_ops = { > > .epc_init = pci_epf_test_epc_init, > > .epc_deinit = pci_epf_test_epc_deinit, > > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > if (ret) > > return ret; > > > > + ret = pci_epf_alloc_doorbell(epf, 1); > > + if (!ret) { > > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > > + struct msi_msg *msg = &epf->db_msg[0].msg; > > + enum pci_barno bar; > > + > > + bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); > > NO_BAR check? This is optional feature, It should check when use it. Frank > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > > > I don't see 'doorbell_done' defined anywhere. > > > > > doorbell address space. > > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > > address space. > > > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > > > > Same here. > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > > > 'avoid breaking compatibility between host and endpoint,...' > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > > to map one bar's inbound address to MSI space. the command > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > > > Host side new driver Host side old driver > > > > > > EP: new driver S F > > > EP: old driver F F > > > > So the last case of old EP and host drivers will fail? > > doorbell test will fail if old EP. > How come there would be doorbell test if it is an old host driver? > > > > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > > If EP side doesn't support MSI, the same to 'F'. > > > > > > F: 'pcitest -B' return failure, other case as usual. > > > > > > Tested-by: Niklas Cassel <cassel@kernel.org> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > Change from v7 to v8 > > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > > > Change from v6 to v7 > > > - use help function pci_epf_align_addr_lo_hi() > > > > > > Change from v5 to v6 > > > - rename doorbell_addr to doorbell_offset > > > > > > Chagne from v4 to v5 > > > - Add doorbell free at unbind function. > > > - Move msi irq handler to here to more complex user case, such as differece > > > doorbell can use difference handler function. > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > > tag or co-developer tag, please let me know. > > > > > > change from v3 to v4 > > > - remove revid requirement > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > > COMMAND_ENABLE_DOORBELL. > > > --- > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > > 1 file changed, 117 insertions(+) > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > index ef6677f34116e..410b2f4bb7ce7 100644 > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > @@ -11,12 +11,14 @@ > > > #include <linux/dmaengine.h> > > > #include <linux/io.h> > > > #include <linux/module.h> > > > +#include <linux/msi.h> > > > #include <linux/slab.h> > > > #include <linux/pci_ids.h> > > > #include <linux/random.h> > > > > > > #include <linux/pci-epc.h> > > > #include <linux/pci-epf.h> > > > +#include <linux/pci-ep-msi.h> > > > #include <linux/pci_regs.h> > > > > > > #define IRQ_TYPE_INTX 0 > > > @@ -29,6 +31,8 @@ > > > #define COMMAND_READ BIT(3) > > > #define COMMAND_WRITE BIT(4) > > > #define COMMAND_COPY BIT(5) > > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > > > #define STATUS_READ_SUCCESS BIT(0) > > > #define STATUS_READ_FAIL BIT(1) > > > @@ -39,6 +43,11 @@ > > > #define STATUS_IRQ_RAISED BIT(6) > > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > > #define STATUS_DST_ADDR_INVALID BIT(8) > > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > > > #define FLAG_USE_DMA BIT(0) > > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > > u32 irq_type; > > > u32 irq_number; > > > u32 flags; > > > + u32 doorbell_bar; > > > + u32 doorbell_offset; > > > + u32 doorbell_data; > > > } __packed; > > > > > > static struct pci_epf_header test_header = { > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > > } > > > } > > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > > +{ > > > + enum pci_barno bar = reg->doorbell_bar; > > > + struct pci_epf *epf = epf_test->epf; > > > + struct pci_epc *epc = epf->epc; > > > + struct pci_epf_bar db_bar; > > > > db_bar = {}; > > > > > + struct msi_msg *msg; > > > + size_t offset; > > > + int ret; > > > + > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > > allocated proper BAR already. > > Not check it at call pci_epf_alloc_doorbell() because it optional feature. What is 'optional feature' here? allocating doorbell? > It return failure when it actually use it. > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()? > > > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > > + return; > > > + } > > > + > > > + msg = &epf->db_msg[0].msg; > > > + ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi, > > > + &db_bar.phys_addr, &offset); > > > + > > > + if (ret) { > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > > + return; > > > + } > > > + > > > + reg->doorbell_offset = offset; > > > + > > > + db_bar.barno = bar; > > > + db_bar.size = epf->bar[bar].size; > > > + db_bar.flags = epf->bar[bar].flags; > > > + db_bar.addr = NULL; > > > > Not needed if you initialize above. > > > > > + > > > + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar); > > > + if (!ret) > > > + reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS; > > > + else > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > > +} > > > + > > > > [...] > > > > > static const struct pci_epc_event_ops pci_epf_test_event_ops = { > > > .epc_init = pci_epf_test_epc_init, > > > .epc_deinit = pci_epf_test_epc_deinit, > > > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > > if (ret) > > > return ret; > > > > > > + ret = pci_epf_alloc_doorbell(epf, 1); > > > + if (!ret) { > > > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > > > + struct msi_msg *msg = &epf->db_msg[0].msg; > > > + enum pci_barno bar; > > > + > > > + bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); > > > > NO_BAR check? > > This is optional feature, It should check when use it. > NO. Why would you call request_irq() if the doorbell BAR is not available? It doesn't make sense. - Mani
On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote: > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > > > > > I don't see 'doorbell_done' defined anywhere. > > > > > > > doorbell address space. > > > > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > > > address space. > > > > > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > > > > > > > Same here. > > > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > > > > > 'avoid breaking compatibility between host and endpoint,...' > > > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > > > to map one bar's inbound address to MSI space. the command > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > > > > > Host side new driver Host side old driver > > > > > > > > EP: new driver S F > > > > EP: old driver F F > > > > > > So the last case of old EP and host drivers will fail? > > > > doorbell test will fail if old EP. > > > > How come there would be doorbell test if it is an old host driver? I also don't understand this. The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE can only be sent if there is a new host driver. Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously return "Invalid command" if the EP driver is old. If EP driver is new, DOORBELL_ENABLE will only return success if the SoC has support for GIC ITS and has configured DTS with msi-parent (i.e. if the pci_epf_alloc_doorbell() call was successful). > > > > > > > > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > > > If EP side doesn't support MSI, the same to 'F'. > > > > > > > > F: 'pcitest -B' return failure, other case as usual. > > > > > > > > Tested-by: Niklas Cassel <cassel@kernel.org> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > > --- > > > > Change from v7 to v8 > > > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > > > > > Change from v6 to v7 > > > > - use help function pci_epf_align_addr_lo_hi() > > > > > > > > Change from v5 to v6 > > > > - rename doorbell_addr to doorbell_offset > > > > > > > > Chagne from v4 to v5 > > > > - Add doorbell free at unbind function. > > > > - Move msi irq handler to here to more complex user case, such as differece > > > > doorbell can use difference handler function. > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > > > tag or co-developer tag, please let me know. > > > > > > > > change from v3 to v4 > > > > - remove revid requirement > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > > > COMMAND_ENABLE_DOORBELL. > > > > --- > > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > > > 1 file changed, 117 insertions(+) > > > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > index ef6677f34116e..410b2f4bb7ce7 100644 > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > @@ -11,12 +11,14 @@ > > > > #include <linux/dmaengine.h> > > > > #include <linux/io.h> > > > > #include <linux/module.h> > > > > +#include <linux/msi.h> > > > > #include <linux/slab.h> > > > > #include <linux/pci_ids.h> > > > > #include <linux/random.h> > > > > > > > > #include <linux/pci-epc.h> > > > > #include <linux/pci-epf.h> > > > > +#include <linux/pci-ep-msi.h> > > > > #include <linux/pci_regs.h> > > > > > > > > #define IRQ_TYPE_INTX 0 > > > > @@ -29,6 +31,8 @@ > > > > #define COMMAND_READ BIT(3) > > > > #define COMMAND_WRITE BIT(4) > > > > #define COMMAND_COPY BIT(5) > > > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > > > > > #define STATUS_READ_SUCCESS BIT(0) > > > > #define STATUS_READ_FAIL BIT(1) > > > > @@ -39,6 +43,11 @@ > > > > #define STATUS_IRQ_RAISED BIT(6) > > > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > > > #define STATUS_DST_ADDR_INVALID BIT(8) > > > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > > > > > #define FLAG_USE_DMA BIT(0) > > > > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > > > u32 irq_type; > > > > u32 irq_number; > > > > u32 flags; > > > > + u32 doorbell_bar; > > > > + u32 doorbell_offset; > > > > + u32 doorbell_data; > > > > } __packed; > > > > > > > > static struct pci_epf_header test_header = { > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > > > } > > > > } > > > > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > > > +{ > > > > + enum pci_barno bar = reg->doorbell_bar; > > > > + struct pci_epf *epf = epf_test->epf; > > > > + struct pci_epc *epc = epf->epc; > > > > + struct pci_epf_bar db_bar; > > > > > > db_bar = {}; > > > > > > > + struct msi_msg *msg; > > > > + size_t offset; > > > > + int ret; > > > > + > > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > > > allocated proper BAR already. > > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature. > > What is 'optional feature' here? allocating doorbell? > > > It return failure when it actually use it. > > > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()? This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at .bind() time. DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver could theoretically send these even if pci_epf_alloc_doorbell() failed. pci_epf_test_cmd_handler() additions looks like this: + case COMMAND_ENABLE_DOORBELL: + pci_epf_enable_doorbell(epf_test, reg); + pci_epf_test_raise_irq(epf_test, reg); + break; + case COMMAND_DISABLE_DOORBELL: + pci_epf_disable_doorbell(epf_test, reg); + pci_epf_test_raise_irq(epf_test, reg); + break; so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell() unconditionally, without any check to see if the doorbell was allocated. We could move the was doorbell allocated check (if (!epf->db_msg)) to pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler() more messy, so personally I think it is fine to keep the doorbell allocated check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell(). I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call to pci_epf_enable_doorbell(): https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/ His reply is here:: https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/ "it may be too frequent to allocate and free msi resources when call pci_epf_enable_doorbell()/pci_epf_disable_doorbell()." I don't think that is a good argument, as presumably (in the normal case) an EPF driver will enable doorbell in the beginning, and then keep it enabled. However, one point could be that pci-epf-test currently does all allocations (the allocations for the backing memory) in .bind(), so in one way it makes sense to also allocate the doorbell in .bind(). To play devil's advocate, I guess you could argue that doorbell feature is optional, while allocating backing memory for BARs is not, so it makes sense that they are not allocated at the same time. I guess it is up to the EPF maintainer to decide what he thinks is best :) Kind regards, Niklas
On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote: > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote: > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > > > > > > > I don't see 'doorbell_done' defined anywhere. > > > > > > > > > doorbell address space. > > > > > > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > > > > address space. > > > > > > > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > > > > > > > > > > Same here. > > > > > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > > > > > > > 'avoid breaking compatibility between host and endpoint,...' > > > > > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > > > > to map one bar's inbound address to MSI space. the command > > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > > > > > > > Host side new driver Host side old driver > > > > > > > > > > EP: new driver S F > > > > > EP: old driver F F > > > > > > > > So the last case of old EP and host drivers will fail? > > > > > > doorbell test will fail if old EP. > > > > > > > How come there would be doorbell test if it is an old host driver? > > I also don't understand this. > > The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE > can only be sent if there is a new host driver. > > Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously > return "Invalid command" if the EP driver is old. > > If EP driver is new, DOORBELL_ENABLE will only return success if the SoC > has support for GIC ITS and has configured DTS with msi-parent > (i.e. if the pci_epf_alloc_doorbell() call was successful). > > > > > > > > > > > > > > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > > > > If EP side doesn't support MSI, the same to 'F'. > > > > > > > > > > F: 'pcitest -B' return failure, other case as usual. > > > > > > > > > > Tested-by: Niklas Cassel <cassel@kernel.org> > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > > > --- > > > > > Change from v7 to v8 > > > > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > > > > > > > Change from v6 to v7 > > > > > - use help function pci_epf_align_addr_lo_hi() > > > > > > > > > > Change from v5 to v6 > > > > > - rename doorbell_addr to doorbell_offset > > > > > > > > > > Chagne from v4 to v5 > > > > > - Add doorbell free at unbind function. > > > > > - Move msi irq handler to here to more complex user case, such as differece > > > > > doorbell can use difference handler function. > > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > > > > tag or co-developer tag, please let me know. > > > > > > > > > > change from v3 to v4 > > > > > - remove revid requirement > > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > > > > COMMAND_ENABLE_DOORBELL. > > > > > --- > > > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > > > > 1 file changed, 117 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > index ef6677f34116e..410b2f4bb7ce7 100644 > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > @@ -11,12 +11,14 @@ > > > > > #include <linux/dmaengine.h> > > > > > #include <linux/io.h> > > > > > #include <linux/module.h> > > > > > +#include <linux/msi.h> > > > > > #include <linux/slab.h> > > > > > #include <linux/pci_ids.h> > > > > > #include <linux/random.h> > > > > > > > > > > #include <linux/pci-epc.h> > > > > > #include <linux/pci-epf.h> > > > > > +#include <linux/pci-ep-msi.h> > > > > > #include <linux/pci_regs.h> > > > > > > > > > > #define IRQ_TYPE_INTX 0 > > > > > @@ -29,6 +31,8 @@ > > > > > #define COMMAND_READ BIT(3) > > > > > #define COMMAND_WRITE BIT(4) > > > > > #define COMMAND_COPY BIT(5) > > > > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > > > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > > > > > > > #define STATUS_READ_SUCCESS BIT(0) > > > > > #define STATUS_READ_FAIL BIT(1) > > > > > @@ -39,6 +43,11 @@ > > > > > #define STATUS_IRQ_RAISED BIT(6) > > > > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > > > > #define STATUS_DST_ADDR_INVALID BIT(8) > > > > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > > > > > > > #define FLAG_USE_DMA BIT(0) > > > > > > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > > > > u32 irq_type; > > > > > u32 irq_number; > > > > > u32 flags; > > > > > + u32 doorbell_bar; > > > > > + u32 doorbell_offset; > > > > > + u32 doorbell_data; > > > > > } __packed; > > > > > > > > > > static struct pci_epf_header test_header = { > > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > > > > } > > > > > } > > > > > > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > > > > +{ > > > > > + enum pci_barno bar = reg->doorbell_bar; > > > > > + struct pci_epf *epf = epf_test->epf; > > > > > + struct pci_epc *epc = epf->epc; > > > > > + struct pci_epf_bar db_bar; > > > > > > > > db_bar = {}; > > > > > > > > > + struct msi_msg *msg; > > > > > + size_t offset; > > > > > + int ret; > > > > > + > > > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > > > > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > > > > allocated proper BAR already. > > > > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature. > > > > What is 'optional feature' here? allocating doorbell? > > > > > It return failure when it actually use it. > > > > > > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()? > > This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at > .bind() time. > > DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver > could theoretically send these even if pci_epf_alloc_doorbell() failed. > > > pci_epf_test_cmd_handler() additions looks like this: > > + case COMMAND_ENABLE_DOORBELL: > + pci_epf_enable_doorbell(epf_test, reg); > + pci_epf_test_raise_irq(epf_test, reg); > + break; > + case COMMAND_DISABLE_DOORBELL: > + pci_epf_disable_doorbell(epf_test, reg); > + pci_epf_test_raise_irq(epf_test, reg); > + break; > > so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell() > unconditionally, without any check to see if the doorbell was allocated. > > We could move the was doorbell allocated check (if (!epf->db_msg)) to > pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler() > more messy, so personally I think it is fine to keep the doorbell allocated > check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell(). > > > I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call > to pci_epf_enable_doorbell(): > https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/ > > His reply is here:: > https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/ > > "it may be too frequent to allocate and free msi resources when call > pci_epf_enable_doorbell()/pci_epf_disable_doorbell()." > > I don't think that is a good argument, as presumably (in the normal case) an > EPF driver will enable doorbell in the beginning, and then keep it enabled. > > However, one point could be that pci-epf-test currently does all allocations > (the allocations for the backing memory) in .bind(), so in one way it makes > sense to also allocate the doorbell in .bind(). > > To play devil's advocate, I guess you could argue that doorbell feature is > optional, while allocating backing memory for BARs is not, so it makes sense > that they are not allocated at the same time. > I like the idea of calling pci_epf_alloc_doorbell() in pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense to call these APIs too frequently. - Mani
On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote: > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > > > > > I don't see 'doorbell_done' defined anywhere. > > > > > > > doorbell address space. > > > > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > > > address space. > > > > > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > > > > > > > Same here. > > > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > > > > > 'avoid breaking compatibility between host and endpoint,...' > > > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > > > to map one bar's inbound address to MSI space. the command > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > > > > > Host side new driver Host side old driver > > > > > > > > EP: new driver S F > > > > EP: old driver F F > > > > > > So the last case of old EP and host drivers will fail? > > > > doorbell test will fail if old EP. > > > > How come there would be doorbell test if it is an old host driver? fail by unsupport ioctrl(). It should "implicit default behavior". Frank > > > > > > > > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > > > If EP side doesn't support MSI, the same to 'F'. > > > > > > > > F: 'pcitest -B' return failure, other case as usual. > > > > > > > > Tested-by: Niklas Cassel <cassel@kernel.org> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > > --- > > > > Change from v7 to v8 > > > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > > > > > Change from v6 to v7 > > > > - use help function pci_epf_align_addr_lo_hi() > > > > > > > > Change from v5 to v6 > > > > - rename doorbell_addr to doorbell_offset > > > > > > > > Chagne from v4 to v5 > > > > - Add doorbell free at unbind function. > > > > - Move msi irq handler to here to more complex user case, such as differece > > > > doorbell can use difference handler function. > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > > > tag or co-developer tag, please let me know. > > > > > > > > change from v3 to v4 > > > > - remove revid requirement > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > > > COMMAND_ENABLE_DOORBELL. > > > > --- > > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > > > 1 file changed, 117 insertions(+) > > > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > index ef6677f34116e..410b2f4bb7ce7 100644 > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > @@ -11,12 +11,14 @@ > > > > #include <linux/dmaengine.h> > > > > #include <linux/io.h> > > > > #include <linux/module.h> > > > > +#include <linux/msi.h> > > > > #include <linux/slab.h> > > > > #include <linux/pci_ids.h> > > > > #include <linux/random.h> > > > > > > > > #include <linux/pci-epc.h> > > > > #include <linux/pci-epf.h> > > > > +#include <linux/pci-ep-msi.h> > > > > #include <linux/pci_regs.h> > > > > > > > > #define IRQ_TYPE_INTX 0 > > > > @@ -29,6 +31,8 @@ > > > > #define COMMAND_READ BIT(3) > > > > #define COMMAND_WRITE BIT(4) > > > > #define COMMAND_COPY BIT(5) > > > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > > > > > #define STATUS_READ_SUCCESS BIT(0) > > > > #define STATUS_READ_FAIL BIT(1) > > > > @@ -39,6 +43,11 @@ > > > > #define STATUS_IRQ_RAISED BIT(6) > > > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > > > #define STATUS_DST_ADDR_INVALID BIT(8) > > > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > > > > > #define FLAG_USE_DMA BIT(0) > > > > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > > > u32 irq_type; > > > > u32 irq_number; > > > > u32 flags; > > > > + u32 doorbell_bar; > > > > + u32 doorbell_offset; > > > > + u32 doorbell_data; > > > > } __packed; > > > > > > > > static struct pci_epf_header test_header = { > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > > > } > > > > } > > > > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > > > +{ > > > > + enum pci_barno bar = reg->doorbell_bar; > > > > + struct pci_epf *epf = epf_test->epf; > > > > + struct pci_epc *epc = epf->epc; > > > > + struct pci_epf_bar db_bar; > > > > > > db_bar = {}; > > > > > > > + struct msi_msg *msg; > > > > + size_t offset; > > > > + int ret; > > > > + > > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > > > allocated proper BAR already. > > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature. > > What is 'optional feature' here? allocating doorbell? Not all platform support ITS. If hardware have not ITS support, pci_epf_alloc_doorbell() will return fail. > > > It return failure when it actually use it. > > > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()? Suppose yes because host don't know if EP support doorbell. > > > > > > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > > > + return; > > > > + } > > > > + > > > > + msg = &epf->db_msg[0].msg; > > > > + ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi, > > > > + &db_bar.phys_addr, &offset); > > > > + > > > > + if (ret) { > > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > > > + return; > > > > + } > > > > + > > > > + reg->doorbell_offset = offset; > > > > + > > > > + db_bar.barno = bar; > > > > + db_bar.size = epf->bar[bar].size; > > > > + db_bar.flags = epf->bar[bar].flags; > > > > + db_bar.addr = NULL; > > > > > > Not needed if you initialize above. > > > > > > > + > > > > + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar); > > > > + if (!ret) > > > > + reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS; > > > > + else > > > > + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; > > > > +} > > > > + > > > > > > [...] > > > > > > > static const struct pci_epc_event_ops pci_epf_test_event_ops = { > > > > .epc_init = pci_epf_test_epc_init, > > > > .epc_deinit = pci_epf_test_epc_deinit, > > > > @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf) > > > > if (ret) > > > > return ret; > > > > > > > > + ret = pci_epf_alloc_doorbell(epf, 1); > > > > + if (!ret) { > > > > + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; > > > > + struct msi_msg *msg = &epf->db_msg[0].msg; > > > > + enum pci_barno bar; > > > > + > > > > + bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); > > > > > > NO_BAR check? > > > > This is optional feature, It should check when use it. > > > > NO. Why would you call request_irq() if the doorbell BAR is not available? It > doesn't make sense. Maybe reasonable now. But it will be not true if we support map two seperate memory regions to a bar in future. Anyway I can add check here. Frank > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Nov 26, 2024 at 06:11:12PM +0530, Manivannan Sadhasivam wrote: > On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote: > > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote: > > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > > > > > > > > > I don't see 'doorbell_done' defined anywhere. > > > > > > > > > > > doorbell address space. > > > > > > > > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > > > > > address space. > > > > > > > > > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > > > > > > > > > > > > > Same here. > > > > > > > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > > > > > > > > > 'avoid breaking compatibility between host and endpoint,...' > > > > > > > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > > > > > to map one bar's inbound address to MSI space. the command > > > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > > > > > > > > > Host side new driver Host side old driver > > > > > > > > > > > > EP: new driver S F > > > > > > EP: old driver F F > > > > > > > > > > So the last case of old EP and host drivers will fail? > > > > > > > > doorbell test will fail if old EP. > > > > > > > > > > How come there would be doorbell test if it is an old host driver? > > > > I also don't understand this. > > > > The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE > > can only be sent if there is a new host driver. > > > > Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously > > return "Invalid command" if the EP driver is old. > > > > If EP driver is new, DOORBELL_ENABLE will only return success if the SoC > > has support for GIC ITS and has configured DTS with msi-parent > > (i.e. if the pci_epf_alloc_doorbell() call was successful). > > > > > > > > > > > > > > > > > > > > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > > > > > If EP side doesn't support MSI, the same to 'F'. > > > > > > > > > > > > F: 'pcitest -B' return failure, other case as usual. > > > > > > > > > > > > Tested-by: Niklas Cassel <cassel@kernel.org> > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > > > > --- > > > > > > Change from v7 to v8 > > > > > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > > > > > > > > > Change from v6 to v7 > > > > > > - use help function pci_epf_align_addr_lo_hi() > > > > > > > > > > > > Change from v5 to v6 > > > > > > - rename doorbell_addr to doorbell_offset > > > > > > > > > > > > Chagne from v4 to v5 > > > > > > - Add doorbell free at unbind function. > > > > > > - Move msi irq handler to here to more complex user case, such as differece > > > > > > doorbell can use difference handler function. > > > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > > > > > tag or co-developer tag, please let me know. > > > > > > > > > > > > change from v3 to v4 > > > > > > - remove revid requirement > > > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > > > > > COMMAND_ENABLE_DOORBELL. > > > > > > --- > > > > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > > > > > 1 file changed, 117 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > > index ef6677f34116e..410b2f4bb7ce7 100644 > > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > > @@ -11,12 +11,14 @@ > > > > > > #include <linux/dmaengine.h> > > > > > > #include <linux/io.h> > > > > > > #include <linux/module.h> > > > > > > +#include <linux/msi.h> > > > > > > #include <linux/slab.h> > > > > > > #include <linux/pci_ids.h> > > > > > > #include <linux/random.h> > > > > > > > > > > > > #include <linux/pci-epc.h> > > > > > > #include <linux/pci-epf.h> > > > > > > +#include <linux/pci-ep-msi.h> > > > > > > #include <linux/pci_regs.h> > > > > > > > > > > > > #define IRQ_TYPE_INTX 0 > > > > > > @@ -29,6 +31,8 @@ > > > > > > #define COMMAND_READ BIT(3) > > > > > > #define COMMAND_WRITE BIT(4) > > > > > > #define COMMAND_COPY BIT(5) > > > > > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > > > > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > > > > > > > > > #define STATUS_READ_SUCCESS BIT(0) > > > > > > #define STATUS_READ_FAIL BIT(1) > > > > > > @@ -39,6 +43,11 @@ > > > > > > #define STATUS_IRQ_RAISED BIT(6) > > > > > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > > > > > #define STATUS_DST_ADDR_INVALID BIT(8) > > > > > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > > > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > > > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > > > > > > > > > #define FLAG_USE_DMA BIT(0) > > > > > > > > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > > > > > u32 irq_type; > > > > > > u32 irq_number; > > > > > > u32 flags; > > > > > > + u32 doorbell_bar; > > > > > > + u32 doorbell_offset; > > > > > > + u32 doorbell_data; > > > > > > } __packed; > > > > > > > > > > > > static struct pci_epf_header test_header = { > > > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > > > > > } > > > > > > } > > > > > > > > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > > > > > +{ > > > > > > + enum pci_barno bar = reg->doorbell_bar; > > > > > > + struct pci_epf *epf = epf_test->epf; > > > > > > + struct pci_epc *epc = epf->epc; > > > > > > + struct pci_epf_bar db_bar; > > > > > > > > > > db_bar = {}; > > > > > > > > > > > + struct msi_msg *msg; > > > > > > + size_t offset; > > > > > > + int ret; > > > > > > + > > > > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > > > > > > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > > > > > allocated proper BAR already. > > > > > > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature. > > > > > > What is 'optional feature' here? allocating doorbell? > > > > > > > It return failure when it actually use it. > > > > > > > > > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()? > > > > This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at > > .bind() time. > > > > DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver > > could theoretically send these even if pci_epf_alloc_doorbell() failed. > > > > > > pci_epf_test_cmd_handler() additions looks like this: > > > > + case COMMAND_ENABLE_DOORBELL: > > + pci_epf_enable_doorbell(epf_test, reg); > > + pci_epf_test_raise_irq(epf_test, reg); > > + break; > > + case COMMAND_DISABLE_DOORBELL: > > + pci_epf_disable_doorbell(epf_test, reg); > > + pci_epf_test_raise_irq(epf_test, reg); > > + break; > > > > so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell() > > unconditionally, without any check to see if the doorbell was allocated. > > > > We could move the was doorbell allocated check (if (!epf->db_msg)) to > > pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler() > > more messy, so personally I think it is fine to keep the doorbell allocated > > check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell(). > > > > > > I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call > > to pci_epf_enable_doorbell(): > > https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/ > > > > His reply is here:: > > https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/ > > > > "it may be too frequent to allocate and free msi resources when call > > pci_epf_enable_doorbell()/pci_epf_disable_doorbell()." > > > > I don't think that is a good argument, as presumably (in the normal case) an > > EPF driver will enable doorbell in the beginning, and then keep it enabled. > > > > However, one point could be that pci-epf-test currently does all allocations > > (the allocations for the backing memory) in .bind(), so in one way it makes > > sense to also allocate the doorbell in .bind(). > > > > To play devil's advocate, I guess you could argue that doorbell feature is > > optional, while allocating backing memory for BARs is not, so it makes sense > > that they are not allocated at the same time. > > > > I like the idea of calling pci_epf_alloc_doorbell() in > pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense > to call these APIs too frequently. I not sure what's you means and direction for next version. Ideally, host driver should use doorbell for every command because it will avoid EP's side polling reg change. It still is problem to waste a bar to do doorbell. Ideally, we can append doorbell ITS register after test bar, which require map two segmemt memory region to bar0. This patch just go first step. If we can append to ITS to bar0 in future, pci_epf_alloc_doorbell() will become more reasonable at bind() function. Frank > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Nov 26, 2024 at 11:55:13AM -0500, Frank Li wrote: > On Tue, Nov 26, 2024 at 06:11:12PM +0530, Manivannan Sadhasivam wrote: > > On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote: > > > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote: > > > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > > > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > > I like the idea of calling pci_epf_alloc_doorbell() in > > pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense > > to call these APIs too frequently. > > I not sure what's you means and direction for next version. Move pci_epf_alloc_doorbell() from .bind() to pci_epf_enable_doorbell(). Move pci_epf_free_doorbell() from .unbind() to pci_epf_disable_doorbell(). If the pci_epf_alloc_doorbell() call within pci_epf_enable_doorbell() fails, let pci_epf_enable_doorbell() set STATUS_DOORBELL_ENABLE_FAIL. > This patch just go first step. If we can append to ITS to bar0 in future, > pci_epf_alloc_doorbell() will become more reasonable at bind() function. To be fair, we are probably quite far away from supporting a BAR with two backing memory areas. It would require a lot of changes in the PCI endpoint framework, and a lot of changes in the DWC driver. And even if we do add all the support for it, why can't we keep the doorbell allocation in pci_epf_enable_doorbell() ? Kind regards, Niklas
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c index ef6677f34116e..410b2f4bb7ce7 100644 --- a/drivers/pci/endpoint/functions/pci-epf-test.c +++ b/drivers/pci/endpoint/functions/pci-epf-test.c @@ -11,12 +11,14 @@ #include <linux/dmaengine.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/msi.h> #include <linux/slab.h> #include <linux/pci_ids.h> #include <linux/random.h> #include <linux/pci-epc.h> #include <linux/pci-epf.h> +#include <linux/pci-ep-msi.h> #include <linux/pci_regs.h> #define IRQ_TYPE_INTX 0 @@ -29,6 +31,8 @@ #define COMMAND_READ BIT(3) #define COMMAND_WRITE BIT(4) #define COMMAND_COPY BIT(5) +#define COMMAND_ENABLE_DOORBELL BIT(6) +#define COMMAND_DISABLE_DOORBELL BIT(7) #define STATUS_READ_SUCCESS BIT(0) #define STATUS_READ_FAIL BIT(1) @@ -39,6 +43,11 @@ #define STATUS_IRQ_RAISED BIT(6) #define STATUS_SRC_ADDR_INVALID BIT(7) #define STATUS_DST_ADDR_INVALID BIT(8) +#define STATUS_DOORBELL_SUCCESS BIT(9) +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) #define FLAG_USE_DMA BIT(0) @@ -74,6 +83,9 @@ struct pci_epf_test_reg { u32 irq_type; u32 irq_number; u32 flags; + u32 doorbell_bar; + u32 doorbell_offset; + u32 doorbell_data; } __packed; static struct pci_epf_header test_header = { @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, } } +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) +{ + enum pci_barno bar = reg->doorbell_bar; + struct pci_epf *epf = epf_test->epf; + struct pci_epc *epc = epf->epc; + struct pci_epf_bar db_bar; + struct msi_msg *msg; + size_t offset; + int ret; + + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; + return; + } + + msg = &epf->db_msg[0].msg; + ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi, + &db_bar.phys_addr, &offset); + + if (ret) { + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; + return; + } + + reg->doorbell_offset = offset; + + db_bar.barno = bar; + db_bar.size = epf->bar[bar].size; + db_bar.flags = epf->bar[bar].flags; + db_bar.addr = NULL; + + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar); + if (!ret) + reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS; + else + reg->status |= STATUS_DOORBELL_ENABLE_FAIL; +} + +static void pci_epf_disable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) +{ + enum pci_barno bar = reg->doorbell_bar; + struct pci_epf *epf = epf_test->epf; + struct pci_epc *epc = epf->epc; + int ret; + + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { + reg->status |= STATUS_DOORBELL_DISABLE_FAIL; + return; + } + + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &epf->bar[bar]); + if (ret) + reg->status |= STATUS_DOORBELL_DISABLE_FAIL; + else + reg->status |= STATUS_DOORBELL_DISABLE_SUCCESS; +} + static void pci_epf_test_cmd_handler(struct work_struct *work) { u32 command; @@ -688,6 +757,14 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) pci_epf_test_copy(epf_test, reg); pci_epf_test_raise_irq(epf_test, reg); break; + case COMMAND_ENABLE_DOORBELL: + pci_epf_enable_doorbell(epf_test, reg); + pci_epf_test_raise_irq(epf_test, reg); + break; + case COMMAND_DISABLE_DOORBELL: + pci_epf_disable_doorbell(epf_test, reg); + pci_epf_test_raise_irq(epf_test, reg); + break; default: dev_err(dev, "Invalid command 0x%x\n", command); break; @@ -822,6 +899,18 @@ static int pci_epf_test_link_down(struct pci_epf *epf) return 0; } +static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data) +{ + struct pci_epf_test *epf_test = data; + enum pci_barno test_reg_bar = epf_test->test_reg_bar; + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; + + reg->status |= STATUS_DOORBELL_SUCCESS; + pci_epf_test_raise_irq(epf_test, reg); + + return IRQ_HANDLED; +} + static const struct pci_epc_event_ops pci_epf_test_event_ops = { .epc_init = pci_epf_test_epc_init, .epc_deinit = pci_epf_test_epc_deinit, @@ -921,12 +1010,34 @@ static int pci_epf_test_bind(struct pci_epf *epf) if (ret) return ret; + ret = pci_epf_alloc_doorbell(epf, 1); + if (!ret) { + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar]; + struct msi_msg *msg = &epf->db_msg[0].msg; + enum pci_barno bar; + + bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1); + + ret = request_irq(epf->db_msg[0].virq, pci_epf_test_doorbell_handler, 0, + "pci-test-doorbell", epf_test); + if (ret) { + dev_err(&epf->dev, + "Failed to request irq %d, doorbell feature is not supported\n", + epf->db_msg[0].virq); + return 0; + } + + reg->doorbell_data = msg->data; + reg->doorbell_bar = bar; + } + return 0; } static void pci_epf_test_unbind(struct pci_epf *epf) { struct pci_epf_test *epf_test = epf_get_drvdata(epf); + struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar]; struct pci_epc *epc = epf->epc; cancel_delayed_work_sync(&epf_test->cmd_handler); @@ -934,6 +1045,12 @@ static void pci_epf_test_unbind(struct pci_epf *epf) pci_epf_test_clean_dma_chan(epf_test); pci_epf_test_clear_bar(epf); } + + if (reg->doorbell_bar > 0) { + free_irq(epf->db_msg[0].virq, epf_test); + pci_epf_free_doorbell(epf); + } + pci_epf_test_free_space(epf); }