From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geetha Akula Subject: Re: [PATCH v3 6/7] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 Date: Mon, 8 May 2017 17:32:34 +0530 Message-ID: References: <1493986091-30521-1-git-send-email-gakula@caviumnetworks.com> <1493986091-30521-7-git-send-email-gakula@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Robin Murphy Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, catalin.marinas-5wv7dgnIgG8@public.gmane.org, Linu Cherian , Geetha Sowjanya , jcm-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Geetha sowjanya , Will Deacon , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, sudeep.holla-5wv7dgnIgG8@public.gmane.org, Sunil Goutham , Robert Richter List-Id: linux-acpi@vger.kernel.org On Mon, May 8, 2017 at 4:51 PM, Robin Murphy wrote: > On 05/05/17 13:08, 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. >> >> This patch addresses the issue by checking if any interrupt sources are >> using same irq number, then they are registered as shared irqs. >> >> Signed-off-by: Geetha Sowjanya >> --- >> drivers/iommu/arm-smmu-v3.c | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 016b702..46428e7 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -2236,10 +2236,30 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu) >> devm_add_action(dev, arm_smmu_free_msis, dev); >> } >> >> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq) >> +{ >> + int match_count = 0; >> + >> + if (irq == smmu->evtq.q.irq) >> + match_count++; >> + if (irq == smmu->cmdq.q.irq) >> + match_count++; >> + if (irq == smmu->gerr_irq) >> + match_count++; >> + if (irq == smmu->priq.q.irq) >> + match_count++; >> + >> + if (match_count > 1) >> + return IRQF_SHARED | IRQF_ONESHOT; >> + >> + return 0; > > I'd say just have this return IRQF_ONESHOT in the non-shared case... > >> +} >> + >> static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> { >> int ret, irq; >> u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN; >> + u32 irqflags = 0; >> >> /* Disable IRQs first */ >> ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, >> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> /* Request interrupt lines */ >> irq = smmu->evtq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_threaded_irq(smmu->dev, irq, NULL, >> arm_smmu_evtq_thread, >> - IRQF_ONESHOT, >> + IRQF_ONESHOT | irqflags, > > ...and pass get_irq_flags(smmu, irq) directly as the argument here. > > The local variable and intermediate logic only seem to add unnecessary > complexity, given that the two cases we actually end up with are: > > IRQF_ONESHOT | 0 > > vs. > > IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT > > neither of which looks particularly sensible. > > Robin. > I will resubmit the patch with suggested changes. Thank you, Geetha. >> "arm-smmu-v3-evtq", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable evtq irq\n"); >> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> >> irq = smmu->cmdq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_irq(smmu->dev, irq, >> - arm_smmu_cmdq_sync_handler, 0, >> + arm_smmu_cmdq_sync_handler, irqflags, >> "arm-smmu-v3-cmdq-sync", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n"); >> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> >> irq = smmu->gerr_irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler, >> - 0, "arm-smmu-v3-gerror", smmu); >> + irqflags, "arm-smmu-v3-gerror", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable gerror irq\n"); >> } >> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> if (smmu->features & ARM_SMMU_FEAT_PRI) { >> irq = smmu->priq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_threaded_irq(smmu->dev, irq, NULL, >> arm_smmu_priq_thread, >> - IRQF_ONESHOT, >> + IRQF_ONESHOT | irqflags, >> "arm-smmu-v3-priq", >> smmu); >> if (ret < 0) >> > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754402AbdEHMCj (ORCPT ); Mon, 8 May 2017 08:02:39 -0400 Received: from mail-wr0-f193.google.com ([209.85.128.193]:35983 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754037AbdEHMCh (ORCPT ); Mon, 8 May 2017 08:02:37 -0400 MIME-Version: 1.0 In-Reply-To: References: <1493986091-30521-1-git-send-email-gakula@caviumnetworks.com> <1493986091-30521-7-git-send-email-gakula@caviumnetworks.com> From: Geetha Akula Date: Mon, 8 May 2017 17:32:34 +0530 Message-ID: Subject: Re: [PATCH v3 6/7] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 To: Robin Murphy Cc: Geetha sowjanya , Will Deacon , lorenzo.pieralisi@arm.com, Hanjun Guo , sudeep.holla@arm.com, iommu@lists.linux-foundation.org, jcm@redhat.com, linux-kernel@vger.kernel.org, Robert Richter , catalin.marinas@arm.com, Sunil Goutham , linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, Linu Cherian , 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 On Mon, May 8, 2017 at 4:51 PM, Robin Murphy wrote: > On 05/05/17 13:08, 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. >> >> This patch addresses the issue by checking if any interrupt sources are >> using same irq number, then they are registered as shared irqs. >> >> Signed-off-by: Geetha Sowjanya >> --- >> drivers/iommu/arm-smmu-v3.c | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 016b702..46428e7 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -2236,10 +2236,30 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu) >> devm_add_action(dev, arm_smmu_free_msis, dev); >> } >> >> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq) >> +{ >> + int match_count = 0; >> + >> + if (irq == smmu->evtq.q.irq) >> + match_count++; >> + if (irq == smmu->cmdq.q.irq) >> + match_count++; >> + if (irq == smmu->gerr_irq) >> + match_count++; >> + if (irq == smmu->priq.q.irq) >> + match_count++; >> + >> + if (match_count > 1) >> + return IRQF_SHARED | IRQF_ONESHOT; >> + >> + return 0; > > I'd say just have this return IRQF_ONESHOT in the non-shared case... > >> +} >> + >> static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> { >> int ret, irq; >> u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN; >> + u32 irqflags = 0; >> >> /* Disable IRQs first */ >> ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, >> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> /* Request interrupt lines */ >> irq = smmu->evtq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_threaded_irq(smmu->dev, irq, NULL, >> arm_smmu_evtq_thread, >> - IRQF_ONESHOT, >> + IRQF_ONESHOT | irqflags, > > ...and pass get_irq_flags(smmu, irq) directly as the argument here. > > The local variable and intermediate logic only seem to add unnecessary > complexity, given that the two cases we actually end up with are: > > IRQF_ONESHOT | 0 > > vs. > > IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT > > neither of which looks particularly sensible. > > Robin. > I will resubmit the patch with suggested changes. Thank you, Geetha. >> "arm-smmu-v3-evtq", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable evtq irq\n"); >> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> >> irq = smmu->cmdq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_irq(smmu->dev, irq, >> - arm_smmu_cmdq_sync_handler, 0, >> + arm_smmu_cmdq_sync_handler, irqflags, >> "arm-smmu-v3-cmdq-sync", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n"); >> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> >> irq = smmu->gerr_irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler, >> - 0, "arm-smmu-v3-gerror", smmu); >> + irqflags, "arm-smmu-v3-gerror", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable gerror irq\n"); >> } >> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> if (smmu->features & ARM_SMMU_FEAT_PRI) { >> irq = smmu->priq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_threaded_irq(smmu->dev, irq, NULL, >> arm_smmu_priq_thread, >> - IRQF_ONESHOT, >> + IRQF_ONESHOT | irqflags, >> "arm-smmu-v3-priq", >> smmu); >> if (ret < 0) >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: geethasowjanya.akula@gmail.com (Geetha Akula) Date: Mon, 8 May 2017 17:32:34 +0530 Subject: [PATCH v3 6/7] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 In-Reply-To: References: <1493986091-30521-1-git-send-email-gakula@caviumnetworks.com> <1493986091-30521-7-git-send-email-gakula@caviumnetworks.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, May 8, 2017 at 4:51 PM, Robin Murphy wrote: > On 05/05/17 13:08, 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. >> >> This patch addresses the issue by checking if any interrupt sources are >> using same irq number, then they are registered as shared irqs. >> >> Signed-off-by: Geetha Sowjanya >> --- >> drivers/iommu/arm-smmu-v3.c | 32 ++++++++++++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c >> index 016b702..46428e7 100644 >> --- a/drivers/iommu/arm-smmu-v3.c >> +++ b/drivers/iommu/arm-smmu-v3.c >> @@ -2236,10 +2236,30 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu) >> devm_add_action(dev, arm_smmu_free_msis, dev); >> } >> >> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq) >> +{ >> + int match_count = 0; >> + >> + if (irq == smmu->evtq.q.irq) >> + match_count++; >> + if (irq == smmu->cmdq.q.irq) >> + match_count++; >> + if (irq == smmu->gerr_irq) >> + match_count++; >> + if (irq == smmu->priq.q.irq) >> + match_count++; >> + >> + if (match_count > 1) >> + return IRQF_SHARED | IRQF_ONESHOT; >> + >> + return 0; > > I'd say just have this return IRQF_ONESHOT in the non-shared case... > >> +} >> + >> static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> { >> int ret, irq; >> u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN; >> + u32 irqflags = 0; >> >> /* Disable IRQs first */ >> ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, >> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> /* Request interrupt lines */ >> irq = smmu->evtq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_threaded_irq(smmu->dev, irq, NULL, >> arm_smmu_evtq_thread, >> - IRQF_ONESHOT, >> + IRQF_ONESHOT | irqflags, > > ...and pass get_irq_flags(smmu, irq) directly as the argument here. > > The local variable and intermediate logic only seem to add unnecessary > complexity, given that the two cases we actually end up with are: > > IRQF_ONESHOT | 0 > > vs. > > IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT > > neither of which looks particularly sensible. > > Robin. > I will resubmit the patch with suggested changes. Thank you, Geetha. >> "arm-smmu-v3-evtq", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable evtq irq\n"); >> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> >> irq = smmu->cmdq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_irq(smmu->dev, irq, >> - arm_smmu_cmdq_sync_handler, 0, >> + arm_smmu_cmdq_sync_handler, irqflags, >> "arm-smmu-v3-cmdq-sync", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n"); >> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> >> irq = smmu->gerr_irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler, >> - 0, "arm-smmu-v3-gerror", smmu); >> + irqflags, "arm-smmu-v3-gerror", smmu); >> if (ret < 0) >> dev_warn(smmu->dev, "failed to enable gerror irq\n"); >> } >> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) >> if (smmu->features & ARM_SMMU_FEAT_PRI) { >> irq = smmu->priq.q.irq; >> if (irq) { >> + irqflags = get_irq_flags(smmu, irq); >> ret = devm_request_threaded_irq(smmu->dev, irq, NULL, >> arm_smmu_priq_thread, >> - IRQF_ONESHOT, >> + IRQF_ONESHOT | irqflags, >> "arm-smmu-v3-priq", >> smmu); >> if (ret < 0) >> >