On Mon, Sep 11, 2023 at 06:09:18PM -0400, Frank Li wrote:
Subject could be,
PCI: endpoint: pci-epf-test: Add doorbell support
> Add three register: doorbell_bar, doorbell_addr, doorbell_data,
> doorbell_done. Call pci_epf_alloc_doorbell() all a doorbell address space.
>
> Root complex(RC) side driver can trigger pci-epc-test's doorbell callback
> handler by write doorbell_data to mapped doorbell_bar's address space.
>
> pci-epc-test will set doorbell_done in doorbell callback.
>
How about,
Add doorbell support to the EPF test driver by introducing 3 new registers:
doorbell_bar
doorbell_addr
doorbell_data
The PCI RC driver can trigger the doorbell on the EP side by writing the
content of "doorbell_data" to the address specified by the "doorbell_addr"
register in the "doorbell_bar" BAR region.
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
You should also update Documentation/PCI/endpoint/pci-test-* files in a separate
commit with doorbell support.
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 59 ++++++++++++++++++-
> 1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 1f0d2b84296a3..566549919b87b 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -11,6 +11,7 @@
> #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>
> @@ -39,17 +40,21 @@
> #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 FLAG_USE_DMA BIT(0)
>
> #define TIMER_RESOLUTION 1
>
> +#define MAGIC_VERSION_MASK GENMASK(7, 0)
> +
> static struct workqueue_struct *kpcitest_workqueue;
>
> struct pci_epf_test {
> void *reg[PCI_STD_NUM_BARS];
> struct pci_epf *epf;
> enum pci_barno test_reg_bar;
> + enum pci_barno doorbell_bar;
> size_t msix_table_offset;
> struct delayed_work cmd_handler;
> struct dma_chan *dma_chan_tx;
> @@ -74,6 +79,9 @@ struct pci_epf_test_reg {
> u32 irq_type;
> u32 irq_number;
> u32 flags;
> + u32 doorbell_bar;
> + u32 doorbell_addr;
> + u32 doorbell_data;
> } __packed;
>
> static struct pci_epf_header test_header = {
> @@ -693,6 +701,8 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> struct pci_epf_bar *epf_bar;
> int bar;
>
> + pci_epf_free_doorbell(epf);
> +
> cancel_delayed_work(&epf_test->cmd_handler);
> pci_epf_test_clean_dma_chan(epf_test);
> for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> @@ -808,9 +818,22 @@ static int pci_epf_test_link_up(struct pci_epf *epf)
> return 0;
> }
>
> +static int pci_epf_test_doorbell(struct pci_epf *epf, int index)
> +{
> + struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> + 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 0;
> +}
> +
> static const struct pci_epc_event_ops pci_epf_test_event_ops = {
> .core_init = pci_epf_test_core_init,
> .link_up = pci_epf_test_link_up,
> + .doorbell = pci_epf_test_doorbell,
I would like to pass this callback directly to the pci_epf_alloc_doorbell() API.
Would that be feasible?
> };
>
> static int pci_epf_test_alloc_space(struct pci_epf *epf)
> @@ -859,7 +882,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> epf_bar = &epf->bar[bar];
> add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
>
> - if (bar == test_reg_bar)
> + if (bar == test_reg_bar || bar == epf_test->doorbell_bar)
> continue;
>
> if (!!(epc_features->reserved_bar & (1 << bar)))
> @@ -900,9 +923,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> const struct pci_epc_features *epc_features;
> enum pci_barno test_reg_bar = BAR_0;
> + enum pci_barno doorbell_bar = NO_BAR;
> struct pci_epc *epc = epf->epc;
> bool linkup_notifier = false;
> bool core_init_notifier = false;
> + struct pci_epf_test_reg *reg;
> + struct msi_msg *msg;
> + u64 doorbell_addr;
> + u32 align;
>
> if (WARN_ON_ONCE(!epc))
> return -EINVAL;
> @@ -923,10 +951,39 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> epf_test->test_reg_bar = test_reg_bar;
> epf_test->epc_features = epc_features;
>
> + align = epc_features->align;
> + align = align ? align : 128;
> +
> + ret = pci_epf_alloc_doorbell(epf, 1);
This should be renamed as pci_epc_alloc_doorbell() as per comment on patch 1/3.
Also, the "msi_msg" pointer should be part of the EPC struct.
> + if (!ret) {
> + msg = epf->msg;
> + doorbell_bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
> +
> + if (doorbell_bar > 0) {
> + epf_test->doorbell_bar = doorbell_bar;
> + doorbell_addr = msg->address_hi;
> + doorbell_addr <<= 32;
> + doorbell_addr |= msg->address_lo;
> + epf->bar[doorbell_bar].phys_addr = round_down(doorbell_addr, align);
> + epf->bar[doorbell_bar].barno = doorbell_bar;
> + epf->bar[doorbell_bar].size = align;
> + } else {
> + pci_epf_free_doorbell(epf);
This one too should be renamed.
> + }
> + }
> +
> ret = pci_epf_test_alloc_space(epf);
This one too.
> if (ret)
> return ret;
>
> + reg = epf_test->reg[test_reg_bar];
> + reg->magic |= FIELD_PREP(MAGIC_VERSION_MASK, 0x1);
Why are you writing this register? This register serves for the purpose of
testing BAR0.
- Mani
> + if (doorbell_bar > 0) {
> + reg->doorbell_addr = doorbell_addr & (align - 1);
> + reg->doorbell_data = msg->data;
> + reg->doorbell_bar = doorbell_bar;
> + }
> +
> if (!core_init_notifier) {
> ret = pci_epf_test_core_init(epf);
> if (ret)
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
