| Message ID | 20241116-ep-msi-v8-5-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:45AM -0500, Frank Li wrote: > Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR, > and PCIE_ENDPOINT_TEST_DB_DATA. > > Trigger the doorbell by writing data from PCI_ENDPOINT_TEST_DB_DATA to the > address provided by PCI_ENDPOINT_TEST_DB_OFFSET and wait for endpoint > feedback. > > Add two command to COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL > to enable EP side's doorbell support and avoid compatible problem. Can you explain the 'compatible problem' and how this patch avoids it? Just for the sake of completeness. > > Host side new driver Host side old driver > EP: new driver S F > EP: old driver F F > > 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 form v6 to v8 > - none > > Change from v5 to v6 > - %s/PCI_ENDPOINT_TEST_DB_ADDR/PCI_ENDPOINT_TEST_DB_OFFSET/g > > Change from v4 to v5 > - remove unused varible > - add irq_type at pci_endpoint_test_doorbell(); > > change from v3 to v4 > - Add COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > - Remove new DID requirement. > --- > drivers/misc/pci_endpoint_test.c | 71 ++++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/pcitest.h | 1 + > 2 files changed, 72 insertions(+) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index 3aaaf47fa4ee2..dc766055aa594 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -42,6 +42,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 PCI_ENDPOINT_TEST_STATUS 0x8 > #define STATUS_READ_SUCCESS BIT(0) > @@ -53,6 +55,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 PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c > #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10 > @@ -67,6 +74,10 @@ > #define PCI_ENDPOINT_TEST_IRQ_NUMBER 0x28 > > #define PCI_ENDPOINT_TEST_FLAGS 0x2c > +#define PCI_ENDPOINT_TEST_DB_BAR 0x30 > +#define PCI_ENDPOINT_TEST_DB_OFFSET 0x34 > +#define PCI_ENDPOINT_TEST_DB_DATA 0x38 > + > #define FLAG_USE_DMA BIT(0) > > #define PCI_DEVICE_ID_TI_AM654 0xb00c > @@ -108,6 +119,7 @@ enum pci_barno { > BAR_3, > BAR_4, > BAR_5, > + NO_BAR = -1, I really hate duplicating this enum definition both in EPF driver and here. Maybe we should move this to pci.h? > }; > > struct pci_endpoint_test { > @@ -746,6 +758,62 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test, > return false; > } > > +static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test) > +{ > + struct pci_dev *pdev = test->pdev; > + struct device *dev = &pdev->dev; > + int irq_type = test->irq_type; > + enum pci_barno bar; > + u32 data, status; > + u32 addr; > + > + if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) { > + dev_err(dev, "Invalid IRQ type option\n"); > + return false; > + } Is this check necessary? > + > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type); > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1); > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, > + COMMAND_ENABLE_DOORBELL); > + > + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); > + > + status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); > + if (status & STATUS_DOORBELL_ENABLE_FAIL) > + return false; I think we should add a error print here and below. > + > + data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA); > + addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET); > + bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR); > + > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type); > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1); > + > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0); > + > + bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR); > + > + writel(data, test->bar[bar] + addr); > + > + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); > + > + status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); > + > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, > + COMMAND_DISABLE_DOORBELL); > + > + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); > + > + status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); > + > + if ((status & STATUS_DOORBELL_SUCCESS) && > + (status & STATUS_DOORBELL_DISABLE_SUCCESS)) > + return true; Usual convention is to check for error and return true at the end. > + > + return false; > +} > + > static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > @@ -793,6 +861,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, > case PCITEST_CLEAR_IRQ: > ret = pci_endpoint_test_clear_irq(test); > break; > + case PCITEST_DOORBELL: > + ret = pci_endpoint_test_doorbell(test); > + break; > } > > ret: > diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h > index 94b46b043b536..06d9f548b510e 100644 > --- a/include/uapi/linux/pcitest.h > +++ b/include/uapi/linux/pcitest.h > @@ -21,6 +21,7 @@ > #define PCITEST_SET_IRQTYPE _IOW('P', 0x8, int) > #define PCITEST_GET_IRQTYPE _IO('P', 0x9) > #define PCITEST_CLEAR_IRQ _IO('P', 0x10) > +#define PCITEST_DOORBELL _IO('P', 0x11) I think defining PCITEST_CLEAR_IRQ as 0x10 was a mistake. It should've been 0xa. But since it is a uapi, we cannot change it. Atleast add new ones starting from 0xa. Niklas's consecutive BAR patch adds a new ioctl for 0xa, but we can fix the conflict later depending on which patch gets merged first. - Mani
On Sun, Nov 24, 2024 at 07:21:28PM +0530, Manivannan Sadhasivam wrote: > On Sat, Nov 16, 2024 at 09:40:45AM -0500, Frank Li wrote: > > Add three registers: PCIE_ENDPOINT_TEST_DB_BAR, PCIE_ENDPOINT_TEST_DB_ADDR, > > and PCIE_ENDPOINT_TEST_DB_DATA. > > > > Trigger the doorbell by writing data from PCI_ENDPOINT_TEST_DB_DATA to the > > address provided by PCI_ENDPOINT_TEST_DB_OFFSET and wait for endpoint > > feedback. > > > > Add two command to COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL > > to enable EP side's doorbell support and avoid compatible problem. > > Can you explain the 'compatible problem' and how this patch avoids it? Just for > the sake of completeness. > > > > > Host side new driver Host side old driver > > EP: new driver S F > > EP: old driver F F > > > > 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 form v6 to v8 > > - none > > > > Change from v5 to v6 > > - %s/PCI_ENDPOINT_TEST_DB_ADDR/PCI_ENDPOINT_TEST_DB_OFFSET/g > > > > Change from v4 to v5 > > - remove unused varible > > - add irq_type at pci_endpoint_test_doorbell(); > > > > change from v3 to v4 > > - Add COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > - Remove new DID requirement. > > --- > > drivers/misc/pci_endpoint_test.c | 71 ++++++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/pcitest.h | 1 + > > 2 files changed, 72 insertions(+) > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > index 3aaaf47fa4ee2..dc766055aa594 100644 > > --- a/drivers/misc/pci_endpoint_test.c > > +++ b/drivers/misc/pci_endpoint_test.c > > @@ -42,6 +42,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 PCI_ENDPOINT_TEST_STATUS 0x8 > > #define STATUS_READ_SUCCESS BIT(0) > > @@ -53,6 +55,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 PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c > > #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10 > > @@ -67,6 +74,10 @@ > > #define PCI_ENDPOINT_TEST_IRQ_NUMBER 0x28 > > > > #define PCI_ENDPOINT_TEST_FLAGS 0x2c > > +#define PCI_ENDPOINT_TEST_DB_BAR 0x30 > > +#define PCI_ENDPOINT_TEST_DB_OFFSET 0x34 > > +#define PCI_ENDPOINT_TEST_DB_DATA 0x38 > > + > > #define FLAG_USE_DMA BIT(0) > > > > #define PCI_DEVICE_ID_TI_AM654 0xb00c > > @@ -108,6 +119,7 @@ enum pci_barno { > > BAR_3, > > BAR_4, > > BAR_5, > > + NO_BAR = -1, > > I really hate duplicating this enum definition both in EPF driver and here. > Maybe we should move this to pci.h? Yes, It is not related this patch. It should belong clean up patch. We can do it later. > > > }; > > > > struct pci_endpoint_test { > > @@ -746,6 +758,62 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test, > > return false; > > } > > > > +static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test) > > +{ > > + struct pci_dev *pdev = test->pdev; > > + struct device *dev = &pdev->dev; > > + int irq_type = test->irq_type; > > + enum pci_barno bar; > > + u32 data, status; > > + u32 addr; > > + > > + if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) { > > + dev_err(dev, "Invalid IRQ type option\n"); > > + return false; > > + } > > Is this check necessary? Suppose yes, it pass down from RC driver side, we should not suppose it alwasy do correct things. > > > + > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type); > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1); > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, > > + COMMAND_ENABLE_DOORBELL); > > + > > + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); > > + > > + status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); > > + if (status & STATUS_DOORBELL_ENABLE_FAIL) > > + return false; > > I think we should add a error print here and below. > > > + > > + data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA); > > + addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET); > > + bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR); > > + > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type); > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1); > > + > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0); > > + > > + bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR); > > + > > + writel(data, test->bar[bar] + addr); > > + > > + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); > > + > > + status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); > > + > > + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, > > + COMMAND_DISABLE_DOORBELL); > > + > > + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); > > + > > + status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); > > + > > + if ((status & STATUS_DOORBELL_SUCCESS) && > > + (status & STATUS_DOORBELL_DISABLE_SUCCESS)) > > + return true; > > Usual convention is to check for error and return true at the end. > > > + > > + return false; > > +} > > + > > static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, > > unsigned long arg) > > { > > @@ -793,6 +861,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, > > case PCITEST_CLEAR_IRQ: > > ret = pci_endpoint_test_clear_irq(test); > > break; > > + case PCITEST_DOORBELL: > > + ret = pci_endpoint_test_doorbell(test); > > + break; > > } > > > > ret: > > diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h > > index 94b46b043b536..06d9f548b510e 100644 > > --- a/include/uapi/linux/pcitest.h > > +++ b/include/uapi/linux/pcitest.h > > @@ -21,6 +21,7 @@ > > #define PCITEST_SET_IRQTYPE _IOW('P', 0x8, int) > > #define PCITEST_GET_IRQTYPE _IO('P', 0x9) > > #define PCITEST_CLEAR_IRQ _IO('P', 0x10) > > +#define PCITEST_DOORBELL _IO('P', 0x11) > > I think defining PCITEST_CLEAR_IRQ as 0x10 was a mistake. It should've been 0xa. > But since it is a uapi, we cannot change it. Atleast add new ones starting from > 0xa. > Yes, thanks. > Niklas's consecutive BAR patch adds a new ioctl for 0xa, but we can fix the > conflict later depending on which patch gets merged first. > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 3aaaf47fa4ee2..dc766055aa594 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -42,6 +42,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 PCI_ENDPOINT_TEST_STATUS 0x8 #define STATUS_READ_SUCCESS BIT(0) @@ -53,6 +55,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 PCI_ENDPOINT_TEST_LOWER_SRC_ADDR 0x0c #define PCI_ENDPOINT_TEST_UPPER_SRC_ADDR 0x10 @@ -67,6 +74,10 @@ #define PCI_ENDPOINT_TEST_IRQ_NUMBER 0x28 #define PCI_ENDPOINT_TEST_FLAGS 0x2c +#define PCI_ENDPOINT_TEST_DB_BAR 0x30 +#define PCI_ENDPOINT_TEST_DB_OFFSET 0x34 +#define PCI_ENDPOINT_TEST_DB_DATA 0x38 + #define FLAG_USE_DMA BIT(0) #define PCI_DEVICE_ID_TI_AM654 0xb00c @@ -108,6 +119,7 @@ enum pci_barno { BAR_3, BAR_4, BAR_5, + NO_BAR = -1, }; struct pci_endpoint_test { @@ -746,6 +758,62 @@ static bool pci_endpoint_test_set_irq(struct pci_endpoint_test *test, return false; } +static bool pci_endpoint_test_doorbell(struct pci_endpoint_test *test) +{ + struct pci_dev *pdev = test->pdev; + struct device *dev = &pdev->dev; + int irq_type = test->irq_type; + enum pci_barno bar; + u32 data, status; + u32 addr; + + if (irq_type < IRQ_TYPE_INTX || irq_type > IRQ_TYPE_MSIX) { + dev_err(dev, "Invalid IRQ type option\n"); + return false; + } + + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type); + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1); + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, + COMMAND_ENABLE_DOORBELL); + + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); + + status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); + if (status & STATUS_DOORBELL_ENABLE_FAIL) + return false; + + data = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_DATA); + addr = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_OFFSET); + bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR); + + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE, irq_type); + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, 1); + + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0); + + bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR); + + writel(data, test->bar[bar] + addr); + + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); + + status = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); + + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, + COMMAND_DISABLE_DOORBELL); + + wait_for_completion_timeout(&test->irq_raised, msecs_to_jiffies(1000)); + + status |= pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_STATUS); + + if ((status & STATUS_DOORBELL_SUCCESS) && + (status & STATUS_DOORBELL_DISABLE_SUCCESS)) + return true; + + return false; +} + static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { @@ -793,6 +861,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd, case PCITEST_CLEAR_IRQ: ret = pci_endpoint_test_clear_irq(test); break; + case PCITEST_DOORBELL: + ret = pci_endpoint_test_doorbell(test); + break; } ret: diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h index 94b46b043b536..06d9f548b510e 100644 --- a/include/uapi/linux/pcitest.h +++ b/include/uapi/linux/pcitest.h @@ -21,6 +21,7 @@ #define PCITEST_SET_IRQTYPE _IOW('P', 0x8, int) #define PCITEST_GET_IRQTYPE _IO('P', 0x9) #define PCITEST_CLEAR_IRQ _IO('P', 0x10) +#define PCITEST_DOORBELL _IO('P', 0x11) #define PCITEST_FLAGS_USE_DMA 0x00000001