Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754531AbdFWGVT (ORCPT ); Fri, 23 Jun 2017 02:21:19 -0400 Received: from mail-lf0-f67.google.com ([209.85.215.67]:35950 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754312AbdFWGVP (ORCPT ); Fri, 23 Jun 2017 02:21:15 -0400 MIME-Version: 1.0 In-Reply-To: <20170622182226.GH15336@arm.com> References: <1498133138-20244-1-git-send-email-gakula@caviumnetworks.com> <1498133138-20244-4-git-send-email-gakula@caviumnetworks.com> <20170622182226.GH15336@arm.com> From: Geetha Akula Date: Fri, 23 Jun 2017 11:51:12 +0530 Message-ID: Subject: Re: [PATCH v9 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 To: Will Deacon Cc: Geetha sowjanya , Robin Murphy , Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , Linux IOMMU , Robert Moore , Lv Zheng , "Rafael J. Wysocki" , jcm@redhat.com, linux-kernel@vger.kernel.org, Robert Richter , Catalin Marinas , Sunil Goutham , linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, devel@acpica.org, Linu Cherian , Charles Garcia-Tobin , Rob Herring , Geetha Sowjanya Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7488 Lines: 176 On Thu, Jun 22, 2017 at 11:52 PM, Will Deacon wrote: > Hi Geetha, > > On Thu, Jun 22, 2017 at 05:35:38PM +0530, Geetha sowjanya wrote: >> From: Geetha Sowjanya >> >> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq >> lines for gerror, eventq and cmdq-sync. >> >> New named irq "combined" is set as a errata workaround, which allows to >> share the irq line by register single irq handler for all the interrupts. >> >> Signed-off-by: Geetha sowjanya >> --- >> Documentation/arm64/silicon-errata.txt | 1 + >> .../devicetree/bindings/iommu/arm,smmu-v3.txt | 1 + >> drivers/acpi/arm64/iort.c | 54 +++++++---- >> drivers/iommu/arm-smmu-v3.c | 105 +++++++++++++++----- >> 4 files changed, 116 insertions(+), 45 deletions(-) > > Thanks, this looks much better. Two things to change below, and I'd like to > see Lorenzo ack the iort changes. Thanks Will. I have resend the patch with suggested changes. Geetha. > >> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt >> index 4693a32..42422f6 100644 >> --- a/Documentation/arm64/silicon-errata.txt >> +++ b/Documentation/arm64/silicon-errata.txt >> @@ -63,6 +63,7 @@ stable kernels. >> | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | >> | Cavium | ThunderX SMMUv2 | #27704 | N/A | >> | Cavium | ThunderX2 SMMUv3| #74 | N/A | >> +| Cavium | ThunderX2 SMMUv3| #126 | N/A | >> | | | | | >> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 | >> | | | | | >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt >> index 6ecc48c..a5a1ca4 100644 >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt >> @@ -26,6 +26,7 @@ the PCIe specification. >> * "priq" - PRI Queue not empty >> * "cmdq-sync" - CMD_SYNC complete >> * "gerror" - Global Error activated >> + * "combined" - Handles above all 4 interrupts. > > Please make it clear that: > > * The combined interrupt is optional, and should only be provided if > the hardware supports just a single, combined interrupt line. > > * If provided, then the combined interrupt will be used in preference > to any others. > >> - #iommu-cells : See the generic IOMMU binding described in >> devicetree/bindings/pci/pci-iommu.txt >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >> index c166f3e..43e1f13 100644 >> --- a/drivers/acpi/arm64/iort.c >> +++ b/drivers/acpi/arm64/iort.c >> @@ -828,6 +828,18 @@ static int __init arm_smmu_v3_count_resources(struct acpi_iort_node *node) >> return num_res; >> } >> >> +static bool arm_smmu_v3_is_combined_irq(struct acpi_iort_smmu_v3 *smmu) >> +{ >> + /* >> + * Cavium ThunderX2 implementation doesn't not support unique >> + * irq line. Use single irq line for all the SMMUv3 interrupts. >> + */ >> + if (smmu->model == ACPI_IORT_SMMU_V3_CAVIUM_CN99XX) >> + return true; >> + >> + return false; >> +} >> + >> static unsigned long arm_smmu_v3_resource_size(struct acpi_iort_smmu_v3 *smmu) >> { >> /* >> @@ -855,26 +867,32 @@ static void __init arm_smmu_v3_init_resources(struct resource *res, >> res[num_res].flags = IORESOURCE_MEM; >> >> num_res++; >> - >> - if (smmu->event_gsiv) >> - acpi_iort_register_irq(smmu->event_gsiv, "eventq", >> - ACPI_EDGE_SENSITIVE, >> - &res[num_res++]); >> - >> - if (smmu->pri_gsiv) >> - acpi_iort_register_irq(smmu->pri_gsiv, "priq", >> - ACPI_EDGE_SENSITIVE, >> - &res[num_res++]); >> - >> - if (smmu->gerr_gsiv) >> - acpi_iort_register_irq(smmu->gerr_gsiv, "gerror", >> - ACPI_EDGE_SENSITIVE, >> - &res[num_res++]); >> - >> - if (smmu->sync_gsiv) >> - acpi_iort_register_irq(smmu->sync_gsiv, "cmdq-sync", >> + if (arm_smmu_v3_is_combined_irq(smmu)) >> + acpi_iort_register_irq(smmu->event_gsiv, "combined", >> ACPI_EDGE_SENSITIVE, >> &res[num_res++]); >> + else { >> + >> + if (smmu->event_gsiv) >> + acpi_iort_register_irq(smmu->event_gsiv, "eventq", >> + ACPI_EDGE_SENSITIVE, >> + &res[num_res++]); >> + >> + if (smmu->pri_gsiv) >> + acpi_iort_register_irq(smmu->pri_gsiv, "priq", >> + ACPI_EDGE_SENSITIVE, >> + &res[num_res++]); >> + >> + if (smmu->gerr_gsiv) >> + acpi_iort_register_irq(smmu->gerr_gsiv, "gerror", >> + ACPI_EDGE_SENSITIVE, >> + &res[num_res++]); >> + >> + if (smmu->sync_gsiv) >> + acpi_iort_register_irq(smmu->sync_gsiv, "cmdq-sync", >> + ACPI_EDGE_SENSITIVE, >> + &res[num_res++]); >> + } >> } >> >> static bool __init arm_smmu_v3_is_coherent(struct acpi_iort_node *node) >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 2dea4a9..0f83f7d 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -605,6 +605,7 @@ struct arm_smmu_device { >> struct arm_smmu_priq priq; >> >> int gerr_irq; >> + int combined_irq; >> >> unsigned long ias; /* IPA */ >> unsigned long oas; /* PA */ >> @@ -1314,6 +1315,29 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev) >> return IRQ_HANDLED; >> } >> >> +static irqreturn_t arm_smmu_combined_irq_thread(int irq, void *dev) >> +{ >> + struct arm_smmu_device *smmu = dev; >> + >> + arm_smmu_evtq_thread(irq, dev); >> + if (smmu->features & ARM_SMMU_FEAT_PRI) >> + arm_smmu_priq_thread(irq, dev); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev) >> +{ >> + irqreturn_t ret; >> + >> + ret = arm_smmu_gerror_handler(irq, dev); >> + if (ret == IRQ_NONE) { > > I don't think you can play that trick if the irq is an edge-triggered > interrupt, since you could lose an interrupt that fired whilst we were > in the handler. > > The easiest thing is to always run the gerror and cmdq_sync handlers, and > then always return IRQ_WAKE_THREAD. > > Will