From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752779AbeDQRkF (ORCPT ); Tue, 17 Apr 2018 13:40:05 -0400 Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:46216 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751927AbeDQRkE (ORCPT ); Tue, 17 Apr 2018 13:40:04 -0400 Subject: Re: [RFC 06/10] misc: pci_endpoint_test: Add MSI-X support To: Kishon Vijay Abraham I , "bhelgaas@google.com" , "lorenzo.pieralisi@arm.com" , "Joao.Pinto@synopsys.com" , "jingoohan1@gmail.com" , "adouglas@cadence.com" , "niklas.cassel@axis.com" , "jesper.nilsson@axis.com" Cc: "linux-pci@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <8b88f8c2b766f36c71659deb0fce635154b2b39f.1523379766.git.gustavo.pimentel@synopsys.com> From: Gustavo Pimentel Message-ID: Date: Tue, 17 Apr 2018 18:38:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kishon, On 17/04/2018 11:33, Kishon Vijay Abraham I wrote: > Hi, > > On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote: >> Adds the MSI-X support and updates driver documentation accordingly. >> >> Changes the driver parameter in order to allow the interruption type >> selection. >> >> Signed-off-by: Gustavo Pimentel >> --- >> Documentation/misc-devices/pci-endpoint-test.txt | 3 + >> drivers/misc/pci_endpoint_test.c | 102 +++++++++++++++++------ >> 2 files changed, 79 insertions(+), 26 deletions(-) >> >> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt >> index 4ebc359..fdfa0f6 100644 >> --- a/Documentation/misc-devices/pci-endpoint-test.txt >> +++ b/Documentation/misc-devices/pci-endpoint-test.txt >> @@ -10,6 +10,7 @@ The PCI driver for the test device performs the following tests >> *) verifying addresses programmed in BAR >> *) raise legacy IRQ >> *) raise MSI IRQ >> + *) raise MSI-X IRQ >> *) read data >> *) write data >> *) copy data >> @@ -25,6 +26,8 @@ ioctl >> PCITEST_LEGACY_IRQ: Tests legacy IRQ >> PCITEST_MSI: Tests message signalled interrupts. The MSI number >> to be tested should be passed as argument. >> + PCITEST_MSIX: Tests message signalled interrupts. The MSI-X number >> + to be tested should be passed as argument. >> PCITEST_WRITE: Perform write tests. The size of the buffer should be passed >> as argument. >> PCITEST_READ: Perform read tests. The size of the buffer should be passed >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >> index 37db0fc..a7d9354 100644 >> --- a/drivers/misc/pci_endpoint_test.c >> +++ b/drivers/misc/pci_endpoint_test.c >> @@ -42,11 +42,16 @@ >> #define PCI_ENDPOINT_TEST_COMMAND 0x4 >> #define COMMAND_RAISE_LEGACY_IRQ BIT(0) >> #define COMMAND_RAISE_MSI_IRQ BIT(1) >> -#define MSI_NUMBER_SHIFT 2 >> -/* 6 bits for MSI number */ >> -#define COMMAND_READ BIT(8) >> -#define COMMAND_WRITE BIT(9) >> -#define COMMAND_COPY BIT(10) >> +#define COMMAND_RAISE_MSIX_IRQ BIT(2) >> +#define IRQ_TYPE_SHIFT 3 >> +#define IRQ_TYPE_LEGACY 0 >> +#define IRQ_TYPE_MSI 1 >> +#define IRQ_TYPE_MSIX 2 >> +#define MSI_NUMBER_SHIFT 5 > > Now that you are anyways fixing this, add a new register entry for MSI numbers. > Let's not keep COMMAND and MSI's together. What you suggest? >> +/* 12 bits for MSI number */ >> +#define COMMAND_READ BIT(17) >> +#define COMMAND_WRITE BIT(18) >> +#define COMMAND_COPY BIT(19) > > This change should be done along with the pci-epf-test in a single patch. To be clear, you're saying is this patch should be just be squashed into the patch number 8 [1], because there is a lot of dependencies namely the defines, that is used on the alter functions. [1] -> https://patchwork.ozlabs.org/patch/896841/ >> >> #define PCI_ENDPOINT_TEST_STATUS 0x8 >> #define STATUS_READ_SUCCESS BIT(0) >> @@ -73,9 +78,9 @@ static DEFINE_IDA(pci_endpoint_test_ida); >> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ >> miscdev) >> >> -static bool no_msi; >> -module_param(no_msi, bool, 0444); >> -MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); > > Let's not remove this just to make sure existing users doesn't get affected. Hum, by making an internal conversion? Like this no_msi = false <=> irq_type = 1 no_msi = true <=> irq_type = 0 >> +static int irq_type = IRQ_TYPE_MSIX; >> +module_param(irq_type, int, 0444); >> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)"); >> >> enum pci_barno { >> BAR_0, >> @@ -103,7 +108,7 @@ struct pci_endpoint_test { >> struct pci_endpoint_test_data { >> enum pci_barno test_reg_bar; >> size_t alignment; >> - bool no_msi; >> + int irq_type; >> }; >> >> static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test, >> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >> >> static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) >> { >> - u32 val; >> + u32 val = COMMAND_RAISE_LEGACY_IRQ; >> >> - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, >> - COMMAND_RAISE_LEGACY_IRQ); >> + val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT); >> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val); >> val = wait_for_completion_timeout(&test->irq_raised, >> msecs_to_jiffies(1000)); >> if (!val) >> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) >> static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, >> u8 msi_num) >> { >> - u32 val; >> + u32 val = COMMAND_RAISE_MSI_IRQ; >> struct pci_dev *pdev = test->pdev; >> >> - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, >> - msi_num << MSI_NUMBER_SHIFT | >> - COMMAND_RAISE_MSI_IRQ); >> + val |= (msi_num << MSI_NUMBER_SHIFT); >> + val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT); >> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val); >> val = wait_for_completion_timeout(&test->irq_raised, >> msecs_to_jiffies(1000)); >> if (!val) >> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, >> return false; >> } >> >> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test, >> + u16 msix_num) >> +{ >> + u32 val = COMMAND_RAISE_MSIX_IRQ; >> + struct pci_dev *pdev = test->pdev; >> + >> + val |= (msix_num << MSI_NUMBER_SHIFT); >> + val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT); >> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val); >> + val = wait_for_completion_timeout(&test->irq_raised, >> + msecs_to_jiffies(1000)); >> + if (!val) >> + return false; >> + >> + if (test->last_irq - pdev->irq == msix_num - 1) >> + return true; >> + >> + return false; > > I think you can have a single function for msix_irq and msi_irq. Ok. > > Thanks > Kishon > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on archive.lwn.net X-Spam-Level: X-Spam-Status: No, score=-5.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by archive.lwn.net (Postfix) with ESMTP id AC2557DE74 for ; Tue, 17 Apr 2018 17:40:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752609AbeDQRkF (ORCPT ); Tue, 17 Apr 2018 13:40:05 -0400 Received: from us01smtprelay-2.synopsys.com ([198.182.60.111]:46216 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751927AbeDQRkE (ORCPT ); Tue, 17 Apr 2018 13:40:04 -0400 Received: from mailhost.synopsys.com (mailhost1.synopsys.com [10.12.238.239]) by smtprelay.synopsys.com (Postfix) with ESMTP id 6C76B10C0790; Tue, 17 Apr 2018 10:40:03 -0700 (PDT) Received: from pt02.synopsys.com (pt02.synopsys.com [10.107.23.240]) by mailhost.synopsys.com (Postfix) with ESMTP id C8BFA6E20; Tue, 17 Apr 2018 10:40:02 -0700 (PDT) Received: from [127.0.0.1] (gustavo-e7480.internal.synopsys.com [10.107.25.102]) by pt02.synopsys.com (Postfix) with ESMTP id 27C683D6B0; Tue, 17 Apr 2018 18:40:02 +0100 (WEST) Subject: Re: [RFC 06/10] misc: pci_endpoint_test: Add MSI-X support To: Kishon Vijay Abraham I , "bhelgaas@google.com" , "lorenzo.pieralisi@arm.com" , "Joao.Pinto@synopsys.com" , "jingoohan1@gmail.com" , "adouglas@cadence.com" , "niklas.cassel@axis.com" , "jesper.nilsson@axis.com" Cc: "linux-pci@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <8b88f8c2b766f36c71659deb0fce635154b2b39f.1523379766.git.gustavo.pimentel@synopsys.com> From: Gustavo Pimentel Message-ID: Date: Tue, 17 Apr 2018 18:38:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org Hi Kishon, On 17/04/2018 11:33, Kishon Vijay Abraham I wrote: > Hi, > > On Tuesday 10 April 2018 10:44 PM, Gustavo Pimentel wrote: >> Adds the MSI-X support and updates driver documentation accordingly. >> >> Changes the driver parameter in order to allow the interruption type >> selection. >> >> Signed-off-by: Gustavo Pimentel >> --- >> Documentation/misc-devices/pci-endpoint-test.txt | 3 + >> drivers/misc/pci_endpoint_test.c | 102 +++++++++++++++++------ >> 2 files changed, 79 insertions(+), 26 deletions(-) >> >> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt >> index 4ebc359..fdfa0f6 100644 >> --- a/Documentation/misc-devices/pci-endpoint-test.txt >> +++ b/Documentation/misc-devices/pci-endpoint-test.txt >> @@ -10,6 +10,7 @@ The PCI driver for the test device performs the following tests >> *) verifying addresses programmed in BAR >> *) raise legacy IRQ >> *) raise MSI IRQ >> + *) raise MSI-X IRQ >> *) read data >> *) write data >> *) copy data >> @@ -25,6 +26,8 @@ ioctl >> PCITEST_LEGACY_IRQ: Tests legacy IRQ >> PCITEST_MSI: Tests message signalled interrupts. The MSI number >> to be tested should be passed as argument. >> + PCITEST_MSIX: Tests message signalled interrupts. The MSI-X number >> + to be tested should be passed as argument. >> PCITEST_WRITE: Perform write tests. The size of the buffer should be passed >> as argument. >> PCITEST_READ: Perform read tests. The size of the buffer should be passed >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >> index 37db0fc..a7d9354 100644 >> --- a/drivers/misc/pci_endpoint_test.c >> +++ b/drivers/misc/pci_endpoint_test.c >> @@ -42,11 +42,16 @@ >> #define PCI_ENDPOINT_TEST_COMMAND 0x4 >> #define COMMAND_RAISE_LEGACY_IRQ BIT(0) >> #define COMMAND_RAISE_MSI_IRQ BIT(1) >> -#define MSI_NUMBER_SHIFT 2 >> -/* 6 bits for MSI number */ >> -#define COMMAND_READ BIT(8) >> -#define COMMAND_WRITE BIT(9) >> -#define COMMAND_COPY BIT(10) >> +#define COMMAND_RAISE_MSIX_IRQ BIT(2) >> +#define IRQ_TYPE_SHIFT 3 >> +#define IRQ_TYPE_LEGACY 0 >> +#define IRQ_TYPE_MSI 1 >> +#define IRQ_TYPE_MSIX 2 >> +#define MSI_NUMBER_SHIFT 5 > > Now that you are anyways fixing this, add a new register entry for MSI numbers. > Let's not keep COMMAND and MSI's together. What you suggest? >> +/* 12 bits for MSI number */ >> +#define COMMAND_READ BIT(17) >> +#define COMMAND_WRITE BIT(18) >> +#define COMMAND_COPY BIT(19) > > This change should be done along with the pci-epf-test in a single patch. To be clear, you're saying is this patch should be just be squashed into the patch number 8 [1], because there is a lot of dependencies namely the defines, that is used on the alter functions. [1] -> https://patchwork.ozlabs.org/patch/896841/ >> >> #define PCI_ENDPOINT_TEST_STATUS 0x8 >> #define STATUS_READ_SUCCESS BIT(0) >> @@ -73,9 +78,9 @@ static DEFINE_IDA(pci_endpoint_test_ida); >> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ >> miscdev) >> >> -static bool no_msi; >> -module_param(no_msi, bool, 0444); >> -MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test"); > > Let's not remove this just to make sure existing users doesn't get affected. Hum, by making an internal conversion? Like this no_msi = false <=> irq_type = 1 no_msi = true <=> irq_type = 0 >> +static int irq_type = IRQ_TYPE_MSIX; >> +module_param(irq_type, int, 0444); >> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)"); >> >> enum pci_barno { >> BAR_0, >> @@ -103,7 +108,7 @@ struct pci_endpoint_test { >> struct pci_endpoint_test_data { >> enum pci_barno test_reg_bar; >> size_t alignment; >> - bool no_msi; >> + int irq_type; >> }; >> >> static inline u32 pci_endpoint_test_readl(struct pci_endpoint_test *test, >> @@ -177,10 +182,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test, >> >> static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) >> { >> - u32 val; >> + u32 val = COMMAND_RAISE_LEGACY_IRQ; >> >> - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, >> - COMMAND_RAISE_LEGACY_IRQ); >> + val |= (IRQ_TYPE_LEGACY << IRQ_TYPE_SHIFT); >> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val); >> val = wait_for_completion_timeout(&test->irq_raised, >> msecs_to_jiffies(1000)); >> if (!val) >> @@ -192,12 +197,12 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test) >> static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, >> u8 msi_num) >> { >> - u32 val; >> + u32 val = COMMAND_RAISE_MSI_IRQ; >> struct pci_dev *pdev = test->pdev; >> >> - pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, >> - msi_num << MSI_NUMBER_SHIFT | >> - COMMAND_RAISE_MSI_IRQ); >> + val |= (msi_num << MSI_NUMBER_SHIFT); >> + val |= (IRQ_TYPE_MSI << IRQ_TYPE_SHIFT); >> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val); >> val = wait_for_completion_timeout(&test->irq_raised, >> msecs_to_jiffies(1000)); >> if (!val) >> @@ -209,6 +214,26 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test, >> return false; >> } >> >> +static bool pci_endpoint_test_msix_irq(struct pci_endpoint_test *test, >> + u16 msix_num) >> +{ >> + u32 val = COMMAND_RAISE_MSIX_IRQ; >> + struct pci_dev *pdev = test->pdev; >> + >> + val |= (msix_num << MSI_NUMBER_SHIFT); >> + val |= (IRQ_TYPE_MSIX << IRQ_TYPE_SHIFT); >> + pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND, val); >> + val = wait_for_completion_timeout(&test->irq_raised, >> + msecs_to_jiffies(1000)); >> + if (!val) >> + return false; >> + >> + if (test->last_irq - pdev->irq == msix_num - 1) >> + return true; >> + >> + return false; > > I think you can have a single function for msix_irq and msi_irq. Ok. > > Thanks > Kishon > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html