From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A18D9C04EBF for ; Mon, 23 Sep 2019 14:57:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7057620578 for ; Mon, 23 Sep 2019 14:57:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1569250655; bh=E6p3ykbgxjB4Vzwb2QdIxLekApqGhHkKm84XDG657Lc=; h=Subject:To:Cc:References:From:Date:In-Reply-To:List-ID:From; b=RcckVEQXatIL0R+DXNkXkJFtp9zl332P/DeUbhqE1fAuokuoMN416jMWcrLMt4lkL uFySNubYpZBnvTP5oWS0jzIiLLoL8rA3PiGdLeD6EAiHCmwX4swA+h5BX4jgLl5Nf4 /NWdiufVou6DG9rU3HwiWyfgpdTQ36nQfkKTaORE= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726903AbfIWO5f (ORCPT ); Mon, 23 Sep 2019 10:57:35 -0400 Received: from foss.arm.com ([217.140.110.172]:43722 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726902AbfIWO5f (ORCPT ); Mon, 23 Sep 2019 10:57:35 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7CCAA1000; Mon, 23 Sep 2019 07:57:34 -0700 (PDT) Received: from [10.1.197.61] (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 05FCC3F59C; Mon, 23 Sep 2019 07:57:32 -0700 (PDT) Subject: Re: [PATCH v2 5/5] irqchip/irq-bcm7038-l1: Support brcm,int-fwd-mask To: Florian Fainelli Cc: linux-kernel@vger.kernel.org, Thomas Gleixner , Jason Cooper , Rob Herring , Mark Rutland , Kevin Cernekee , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "open list:BROADCOM BMIPS MIPS ARCHITECTURE" , "open list:BROADCOM BMIPS MIPS ARCHITECTURE" References: <20190913191542.9908-1-f.fainelli@gmail.com> <20190913191542.9908-6-f.fainelli@gmail.com> <20190922133805.2cdf2d99@why> <260e61b8-a083-743e-43fc-70b9ea644e0e@gmail.com> <8b0f18ef-7b72-e6fd-9930-0f698ced270b@gmail.com> From: Marc Zyngier Organization: Approximate Message-ID: <7610ea17-de8a-d475-7a9e-8f314f5f34a3@kernel.org> Date: Mon, 23 Sep 2019 15:57:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <8b0f18ef-7b72-e6fd-9930-0f698ced270b@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-mips-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mips@vger.kernel.org On 23/09/2019 15:39, Florian Fainelli wrote: > > > On 9/23/2019 1:52 AM, Marc Zyngier wrote: >> On 22/09/2019 20:08, Florian Fainelli wrote: >>> >>> >>> On 9/22/2019 5:38 AM, Marc Zyngier wrote: >>>> On Fri, 13 Sep 2019 12:15:42 -0700 >>>> Florian Fainelli wrote: >>>> >>>>> On some specific chips like 7211 we need to leave some interrupts >>>>> untouched/forwarded to the VPU which is another agent in the system >>>>> making use of that interrupt controller hardware (goes to both ARM GIC >>>>> and VPU L1 interrupt controller). Make that possible by using the >>>>> existing brcm,int-fwd-mask property. >>>>> >>>>> Signed-off-by: Florian Fainelli >>>>> --- >>>>> drivers/irqchip/irq-bcm7038-l1.c | 15 +++++++++++++-- >>>>> 1 file changed, 13 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c >>>>> index 0673a44bbdc2..811a34201dd4 100644 >>>>> --- a/drivers/irqchip/irq-bcm7038-l1.c >>>>> +++ b/drivers/irqchip/irq-bcm7038-l1.c >>>>> @@ -44,6 +44,7 @@ struct bcm7038_l1_chip { >>>>> struct list_head list; >>>>> u32 wake_mask[MAX_WORDS]; >>>>> #endif >>>>> + u32 irq_fwd_mask[MAX_WORDS]; >>>>> u8 affinity[MAX_WORDS * IRQS_PER_WORD]; >>>>> }; >>>>> >>>>> @@ -265,6 +266,7 @@ static int __init bcm7038_l1_init_one(struct device_node *dn, >>>>> resource_size_t sz; >>>>> struct bcm7038_l1_cpu *cpu; >>>>> unsigned int i, n_words, parent_irq; >>>>> + int ret; >>>>> >>>>> if (of_address_to_resource(dn, idx, &res)) >>>>> return -EINVAL; >>>>> @@ -278,6 +280,14 @@ static int __init bcm7038_l1_init_one(struct device_node *dn, >>>>> else if (intc->n_words != n_words) >>>>> return -EINVAL; >>>>> >>>>> + ret = of_property_read_u32_array(dn , "brcm,int-fwd-mask", >>>> >>>> What is the exact meaning of "fwd"? Forward? FirmWare Dementia? >>> >>> Here it is meant to be "forward", we have defined this property name >>> before for irq-bcm7120-l2.c and felt like reusing the same name to avoid >>> multiplying properties would be appropriate, see patch #4. If you prefer >>> something named brcm,firmware-configured-mask, let me know. >> >> It's just a name, but I found it a bit confusing. Bah, never mind. >> >>>> >>>>> + intc->irq_fwd_mask, n_words); >>>>> + if (ret != 0 && ret != -EINVAL) { >>>>> + /* property exists but has the wrong number of words */ >>>>> + pr_err("invalid brcm,int-fwd-mask property\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> cpu = intc->cpus[idx] = kzalloc(sizeof(*cpu) + n_words * sizeof(u32), >>>>> GFP_KERNEL); >>>>> if (!cpu) >>>>> @@ -288,8 +298,9 @@ static int __init bcm7038_l1_init_one(struct device_node *dn, >>>>> return -ENOMEM; >>>>> >>>>> for (i = 0; i < n_words; i++) { >>>>> - l1_writel(0xffffffff, cpu->map_base + reg_mask_set(intc, i)); >>>>> - cpu->mask_cache[i] = 0xffffffff; >>>>> + l1_writel(0xffffffff & ~intc->irq_fwd_mask[i], >>>>> + cpu->map_base + reg_mask_set(intc, i)); >>>>> + cpu->mask_cache[i] = 0xffffffff & ~intc->irq_fwd_mask[i]; >>>> >>>> I seem to remember that (0xffffffff & whatever) == whatever, as long as >>>> 'whatever' is a 32bit quantity. So what it this for? >>> >>> It is 0xffff_ffff & ~whatever here. >> >> Which doesn't change anything. >> >>> In the absence of this property >>> being specified, the data is all zeroed out, so we would have >>> 0xffff_ffff & 0xffff_ffff which is 0xffff_ffff. If this property is >>> specified, we would have one more or bits set, and it would be e.g.: >>> 0x100 so we would have 0xffff_ffff & ~(0x100) = 0xffff_feff which is >>> what we would want here to preserve whatever the firmware has already >>> configured. >> >> OK, I must be stupid: >> >> #include >> >> int main(int argc, char *argv[]) >> { >> unsigned int v = 0x100; >> printf ("%x\n", ~v); >> } >> maz@filthy-habit$ ./x >> fffffeff >> >> You might as well OR it with zeroes, if you want. > > Not sure I understand your point here. > > We used to write 0xffff_ffff to both the hardware and the mask cache to > have all interrupts masked by default. Now we want to have some bits > optionally set to 0 (unmasked), based on the brcm,int-fwd-mask property, > which is what this patch achieves (or tries to). If we write, say > 0xffff_feff to the hardware, which has a Write Only register behavior, > then we also want to have the mask cache be set to the same value for > consistency if nothing else. Am I failing at doing what I just described > and also failing at see it? You write this: > for (i = 0; i < n_words; i++) { > - l1_writel(0xffffffff, cpu->map_base + reg_mask_set(intc, i)); > - cpu->mask_cache[i] = 0xffffffff; > + l1_writel(0xffffffff & ~intc->irq_fwd_mask[i], > + cpu->map_base + reg_mask_set(intc, i)); > + cpu->mask_cache[i] = 0xffffffff & ~intc->irq_fwd_mask[i]; > } And I'm saying that this is strictly equivalent to: for (i = 0; i < n_words; i++) { l1_writel(~intc->irq_fwd_mask[i], cpu->map_base + reg_mask_set(intc, i)); cpu->mask_cache[i] = ~intc->irq_fwd_mask[i]; } without this 0xffffffff that does exactly nothing (I'm pretty sure the compiler drops it anyway). M. -- Jazz is not dead, it just smells funny...