Re: [PATCH v2 3/5] PCI: endpoint: pci-epf-test: add doorbell test

[Manivannan Sadhasivam <mani@kernel.org>]

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@nxp.com>

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
> 

-- 
மணிவண்ணன் சதாசிவம்