| Message ID | 20210803074932.19820-1-kishon@ti.com |
|---|---|
| Headers | show |
| Series | PCI: Add support for J7200 and AM64 | expand |
On Tue, Aug 03, 2021 at 01:19:26PM +0530, Kishon Vijay Abraham I wrote: > This series adds the compatible specific to J7200 and AM64 and > applies the erratas and configuration specific to them. > > This series also includes Nadeem's patch that adds a quirk in > Cadence driver which is used by J7200 [1]. > > The DT binding for both J7200 and AM64 is already merged. > > v1 of the patch series can be found at [2] > > Changes from v1: > 1) As suggested by Bjorn, used unsigned int :1, instead of bool for > structure members > 2) Removed using unnecessary local variables and also fixed some > code alignment > > [1] -> https://lore.kernel.org/r/20210528155626.21793-1-nadeem@cadence.com > [2] -> https://lore.kernel.org/r/20210706105035.9915-1-kishon@ti.com > > Kishon Vijay Abraham I (5): > PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool > PCI: j721e: Add PCIe support for J7200 > PCI: j721e: Add PCIe support for AM64 > misc: pci_endpoint_test: Do not request or allocate IRQs in probe > misc: pci_endpoint_test: Add deviceID for AM64 and J7200 > > Nadeem Athani (1): > PCI: cadence: Add quirk flag to set minimum delay in LTSSM > Detect.Quiet state > > drivers/misc/pci_endpoint_test.c | 27 ++++++-- > drivers/pci/controller/cadence/pci-j721e.c | 61 +++++++++++++++++-- > .../pci/controller/cadence/pcie-cadence-ep.c | 4 ++ > .../controller/cadence/pcie-cadence-host.c | 3 + > drivers/pci/controller/cadence/pcie-cadence.c | 17 ++++++ > drivers/pci/controller/cadence/pcie-cadence.h | 17 +++++- > 6 files changed, 117 insertions(+), 12 deletions(-) I am not convinced about patch (5) the rest of the series can be merged (even though I assume patch (6) depends on (5)). Please let me know, Lorenzo
Hi Lorenzo, On 03/08/21 3:28 pm, Lorenzo Pieralisi wrote: > On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote: >> Allocation of IRQ vectors and requesting IRQ is done as part of >> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for >> AM654 and J721E so that the user space test script has better control >> of the devices for which the IRQs are configured. Since certain user >> space scripts could rely on allocation of IRQ vectors during probe, >> remove allocation of IRQs only for TI's K3 platform. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) > > I don't claim to understand the inner details of the endpoint test > device but it looks like this approach should be redesigned. > > I don't believe using devices quirks is the best approach to > expose/remove a feature to userspace, this can soon become > unmaintenable. > > Maybe you can elaborate a bit more on what the real issue is please ? The actual reason for introducing this patch (affects only AM654 and J721E) is due to Errata ID #i2101 GIC: ITS Misbehavior (https://www.ti.com/lit/er/sprz455a/sprz455a.pdf). So if more than 5 devices use GIC ITS simultaneously, GIC fails to raise interrupts. Though this patch is not an actual workaround for the issue (the workaround is in GIC ITS driver provided in the errata document), it helps to keep testing PCIe RC/EP using pci-endpoint-test even when multiple pci-epf-test endpoint devices are connected (Normal test-setup having J721E-J721E back to back connection can support 21 pci-epf-test devices). So this patch lets user to individually enable interrupts for each of the devices and could disable after the interrupt test. Since pci_endpoint_test is used only for testing PCIE RC/EP communication and pci-endpoint-test has already implemented PCITEST_SET_IRQTYPE for the userspace to enable interrupt, tried to not enable the interrupts of all the devices by default in the probe (for AM654 and J721E where this errata applies). Thanks, Kishon > > Thanks, > Lorenzo > >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >> index c7ee34013485..9740f2a0e7cd 100644 >> --- a/drivers/misc/pci_endpoint_test.c >> +++ b/drivers/misc/pci_endpoint_test.c >> @@ -79,6 +79,9 @@ >> #define PCI_DEVICE_ID_RENESAS_R8A774C0 0x002d >> #define PCI_DEVICE_ID_RENESAS_R8A774E1 0x0025 >> >> +#define is_j721e_pci_dev(pdev) \ >> + ((pdev)->device == PCI_DEVICE_ID_TI_J721E) >> + >> static DEFINE_IDA(pci_endpoint_test_ida); >> >> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ >> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, >> >> pci_set_master(pdev); >> >> - if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { >> - err = -EINVAL; >> - goto err_disable_irq; >> + if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) { >> + if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { >> + err = -EINVAL; >> + goto err_disable_irq; >> + } >> } >> >> for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { >> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, >> goto err_ida_remove; >> } >> >> - if (!pci_endpoint_test_request_irq(test)) { >> - err = -EINVAL; >> - goto err_kfree_test_name; >> + if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) { >> + if (!pci_endpoint_test_request_irq(test)) { >> + err = -EINVAL; >> + goto err_kfree_test_name; >> + } >> } >> >> misc_device = &test->miscdev; >> -- >> 2.17.1 >>
On Wed, Aug 04, 2021 at 07:32:44PM +0530, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > On 03/08/21 3:28 pm, Lorenzo Pieralisi wrote: > > On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote: > >> Allocation of IRQ vectors and requesting IRQ is done as part of > >> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for > >> AM654 and J721E so that the user space test script has better control > >> of the devices for which the IRQs are configured. Since certain user > >> space scripts could rely on allocation of IRQ vectors during probe, > >> remove allocation of IRQs only for TI's K3 platform. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------ > >> 1 file changed, 13 insertions(+), 6 deletions(-) > > > > I don't claim to understand the inner details of the endpoint test > > device but it looks like this approach should be redesigned. > > > > I don't believe using devices quirks is the best approach to > > expose/remove a feature to userspace, this can soon become > > unmaintenable. > > > > Maybe you can elaborate a bit more on what the real issue is please ? > > The actual reason for introducing this patch (affects only AM654 and > J721E) is due to Errata ID #i2101 GIC: ITS Misbehavior > (https://www.ti.com/lit/er/sprz455a/sprz455a.pdf). So if more than 5 > devices use GIC ITS simultaneously, GIC fails to raise interrupts. > > Though this patch is not an actual workaround for the issue (the > workaround is in GIC ITS driver provided in the errata document), it > helps to keep testing PCIe RC/EP using pci-endpoint-test even when > multiple pci-epf-test endpoint devices are connected (Normal test-setup > having J721E-J721E back to back connection can support 21 pci-epf-test > devices). So this patch lets user to individually enable interrupts for > each of the devices and could disable after the interrupt test. > > Since pci_endpoint_test is used only for testing PCIE RC/EP > communication and pci-endpoint-test has already implemented > PCITEST_SET_IRQTYPE for the userspace to enable interrupt, tried to not > enable the interrupts of all the devices by default in the probe (for > AM654 and J721E where this errata applies). I understand - what I am asking is: is it possible, instead of applying this patch, to make pci_endpoint_test_alloc_irq_vectors() and pci_endpoint_test_request_irq() fail in the target platforms instead of preventing to call them ? My worry is that you may end up with more corner cases in the future and peppering code with is_() calls to work around them which does not look right. Thanks, Lorenzo > Thanks, > Kishon > > > > > Thanks, > > Lorenzo > > > >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > >> index c7ee34013485..9740f2a0e7cd 100644 > >> --- a/drivers/misc/pci_endpoint_test.c > >> +++ b/drivers/misc/pci_endpoint_test.c > >> @@ -79,6 +79,9 @@ > >> #define PCI_DEVICE_ID_RENESAS_R8A774C0 0x002d > >> #define PCI_DEVICE_ID_RENESAS_R8A774E1 0x0025 > >> > >> +#define is_j721e_pci_dev(pdev) \ > >> + ((pdev)->device == PCI_DEVICE_ID_TI_J721E) > >> + > >> static DEFINE_IDA(pci_endpoint_test_ida); > >> > >> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ > >> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > >> > >> pci_set_master(pdev); > >> > >> - if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { > >> - err = -EINVAL; > >> - goto err_disable_irq; > >> + if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) { > >> + if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { > >> + err = -EINVAL; > >> + goto err_disable_irq; > >> + } > >> } > >> > >> for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { > >> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > >> goto err_ida_remove; > >> } > >> > >> - if (!pci_endpoint_test_request_irq(test)) { > >> - err = -EINVAL; > >> - goto err_kfree_test_name; > >> + if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) { > >> + if (!pci_endpoint_test_request_irq(test)) { > >> + err = -EINVAL; > >> + goto err_kfree_test_name; > >> + } > >> } > >> > >> misc_device = &test->miscdev; > >> -- > >> 2.17.1 > >>
Hi Lorenzo, On 05/08/21 4:56 pm, Lorenzo Pieralisi wrote: > On Wed, Aug 04, 2021 at 07:32:44PM +0530, Kishon Vijay Abraham I wrote: >> Hi Lorenzo, >> >> On 03/08/21 3:28 pm, Lorenzo Pieralisi wrote: >>> On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote: >>>> Allocation of IRQ vectors and requesting IRQ is done as part of >>>> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for >>>> AM654 and J721E so that the user space test script has better control >>>> of the devices for which the IRQs are configured. Since certain user >>>> space scripts could rely on allocation of IRQ vectors during probe, >>>> remove allocation of IRQs only for TI's K3 platform. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------ >>>> 1 file changed, 13 insertions(+), 6 deletions(-) >>> >>> I don't claim to understand the inner details of the endpoint test >>> device but it looks like this approach should be redesigned. >>> >>> I don't believe using devices quirks is the best approach to >>> expose/remove a feature to userspace, this can soon become >>> unmaintenable. >>> >>> Maybe you can elaborate a bit more on what the real issue is please ? >> >> The actual reason for introducing this patch (affects only AM654 and >> J721E) is due to Errata ID #i2101 GIC: ITS Misbehavior >> (https://www.ti.com/lit/er/sprz455a/sprz455a.pdf). So if more than 5 >> devices use GIC ITS simultaneously, GIC fails to raise interrupts. >> >> Though this patch is not an actual workaround for the issue (the >> workaround is in GIC ITS driver provided in the errata document), it >> helps to keep testing PCIe RC/EP using pci-endpoint-test even when >> multiple pci-epf-test endpoint devices are connected (Normal test-setup >> having J721E-J721E back to back connection can support 21 pci-epf-test >> devices). So this patch lets user to individually enable interrupts for >> each of the devices and could disable after the interrupt test. >> >> Since pci_endpoint_test is used only for testing PCIE RC/EP >> communication and pci-endpoint-test has already implemented >> PCITEST_SET_IRQTYPE for the userspace to enable interrupt, tried to not >> enable the interrupts of all the devices by default in the probe (for >> AM654 and J721E where this errata applies). > > I understand - what I am asking is: > > is it possible, instead of applying this patch, to make > > pci_endpoint_test_alloc_irq_vectors() and pci_endpoint_test_request_irq() > > fail in the target platforms instead of preventing to call them ? hmm.. since both the platforms use GIC ITS it would need modifications in the irq-gic-v3-its.c driver. I could add the workaround mentioned in the errata document and send the patch for review to see what others think. Thanks, Kishon > > My worry is that you may end up with more corner cases in the future > and peppering code with is_() calls to work around them which does > not look right. > > Thanks, > Lorenzo > >> Thanks, >> Kishon >> >>> >>> Thanks, >>> Lorenzo >>> >>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c >>>> index c7ee34013485..9740f2a0e7cd 100644 >>>> --- a/drivers/misc/pci_endpoint_test.c >>>> +++ b/drivers/misc/pci_endpoint_test.c >>>> @@ -79,6 +79,9 @@ >>>> #define PCI_DEVICE_ID_RENESAS_R8A774C0 0x002d >>>> #define PCI_DEVICE_ID_RENESAS_R8A774E1 0x0025 >>>> >>>> +#define is_j721e_pci_dev(pdev) \ >>>> + ((pdev)->device == PCI_DEVICE_ID_TI_J721E) >>>> + >>>> static DEFINE_IDA(pci_endpoint_test_ida); >>>> >>>> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ >>>> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, >>>> >>>> pci_set_master(pdev); >>>> >>>> - if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { >>>> - err = -EINVAL; >>>> - goto err_disable_irq; >>>> + if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) { >>>> + if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) { >>>> + err = -EINVAL; >>>> + goto err_disable_irq; >>>> + } >>>> } >>>> >>>> for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) { >>>> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, >>>> goto err_ida_remove; >>>> } >>>> >>>> - if (!pci_endpoint_test_request_irq(test)) { >>>> - err = -EINVAL; >>>> - goto err_kfree_test_name; >>>> + if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) { >>>> + if (!pci_endpoint_test_request_irq(test)) { >>>> + err = -EINVAL; >>>> + goto err_kfree_test_name; >>>> + } >>>> } >>>> >>>> misc_device = &test->miscdev; >>>> -- >>>> 2.17.1 >>>>
Hi Lorenzo, On 03/08/21 4:22 pm, Lorenzo Pieralisi wrote: > On Tue, Aug 03, 2021 at 01:19:26PM +0530, Kishon Vijay Abraham I wrote: >> This series adds the compatible specific to J7200 and AM64 and >> applies the erratas and configuration specific to them. >> >> This series also includes Nadeem's patch that adds a quirk in >> Cadence driver which is used by J7200 [1]. >> >> The DT binding for both J7200 and AM64 is already merged. >> >> v1 of the patch series can be found at [2] >> >> Changes from v1: >> 1) As suggested by Bjorn, used unsigned int :1, instead of bool for >> structure members >> 2) Removed using unnecessary local variables and also fixed some >> code alignment >> >> [1] -> https://lore.kernel.org/r/20210528155626.21793-1-nadeem@cadence.com >> [2] -> https://lore.kernel.org/r/20210706105035.9915-1-kishon@ti.com >> >> Kishon Vijay Abraham I (5): >> PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool >> PCI: j721e: Add PCIe support for J7200 >> PCI: j721e: Add PCIe support for AM64 >> misc: pci_endpoint_test: Do not request or allocate IRQs in probe >> misc: pci_endpoint_test: Add deviceID for AM64 and J7200 >> >> Nadeem Athani (1): >> PCI: cadence: Add quirk flag to set minimum delay in LTSSM >> Detect.Quiet state >> >> drivers/misc/pci_endpoint_test.c | 27 ++++++-- >> drivers/pci/controller/cadence/pci-j721e.c | 61 +++++++++++++++++-- >> .../pci/controller/cadence/pcie-cadence-ep.c | 4 ++ >> .../controller/cadence/pcie-cadence-host.c | 3 + >> drivers/pci/controller/cadence/pcie-cadence.c | 17 ++++++ >> drivers/pci/controller/cadence/pcie-cadence.h | 17 +++++- >> 6 files changed, 117 insertions(+), 12 deletions(-) > > I am not convinced about patch (5) the rest of the series can be > merged (even though I assume patch (6) depends on (5)). I'll drop patch 5, fix your other comments and send a new revision. Thanks, Kishon
This series adds the compatible specific to J7200 and AM64 and applies the erratas and configuration specific to them. This series also includes Nadeem's patch that adds a quirk in Cadence driver which is used by J7200 [1]. The DT binding for both J7200 and AM64 is already merged. v1 of the patch series can be found at [2] Changes from v1: 1) As suggested by Bjorn, used unsigned int :1, instead of bool for structure members 2) Removed using unnecessary local variables and also fixed some code alignment [1] -> https://lore.kernel.org/r/20210528155626.21793-1-nadeem@cadence.com [2] -> https://lore.kernel.org/r/20210706105035.9915-1-kishon@ti.com Kishon Vijay Abraham I (5): PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool PCI: j721e: Add PCIe support for J7200 PCI: j721e: Add PCIe support for AM64 misc: pci_endpoint_test: Do not request or allocate IRQs in probe misc: pci_endpoint_test: Add deviceID for AM64 and J7200 Nadeem Athani (1): PCI: cadence: Add quirk flag to set minimum delay in LTSSM Detect.Quiet state drivers/misc/pci_endpoint_test.c | 27 ++++++-- drivers/pci/controller/cadence/pci-j721e.c | 61 +++++++++++++++++-- .../pci/controller/cadence/pcie-cadence-ep.c | 4 ++ .../controller/cadence/pcie-cadence-host.c | 3 + drivers/pci/controller/cadence/pcie-cadence.c | 17 ++++++ drivers/pci/controller/cadence/pcie-cadence.h | 17 +++++- 6 files changed, 117 insertions(+), 12 deletions(-)