| Message ID | 20190913191542.9908-6-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | irqchip/irq-bcm7038-l1 updates | expand |
On Fri, 13 Sep 2019 12:15:42 -0700 Florian Fainelli <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> > --- > 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? > + 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? M.
On 9/22/2019 5:38 AM, Marc Zyngier wrote: > On Fri, 13 Sep 2019 12:15:42 -0700 > Florian Fainelli <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> >> --- >> 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. > >> + 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. 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. In hindsight, it may be safer to make sure no one in Linux can actually map that interrupt, so we would need something like this in addition to what we already have in this patch: diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c index fc75c61233aa..558e74be0d60 100644 --- a/drivers/irqchip/irq-bcm7038-l1.c +++ b/drivers/irqchip/irq-bcm7038-l1.c @@ -300,6 +300,14 @@ static struct irq_chip bcm7038_l1_irq_chip = { static int bcm7038_l1_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t hw_irq) { + struct bcm7038_l1_chip *intc = d->host_data; + int i; + + for (i = 0; i < intc->n_words; i++) { + if (intc->irq_fwd_mask[i] & BIT(hw_irq)) + return -EINVAL; + } + irq_set_chip_and_handler(virq, &bcm7038_l1_irq_chip, handle_level_irq); irq_set_chip_data(virq, d->host_data); irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq)));
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 <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> >>> --- >>> 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 <stdio.h> 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. M.
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 <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> >>>> --- >>>> 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 <stdio.h> > > 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?
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 <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> >>>>> --- >>>>> 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 <stdio.h> >> >> 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.
On 9/23/2019 7:57 AM, Marc Zyngier wrote: > 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 <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> >>>>>> --- >>>>>> 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 <stdio.h> >>> >>> 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). I understand quickly, you just need to repeat many times, thanks for bearing with me, this is indeed simpler and clearer.
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", + 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]; } parent_irq = irq_of_parse_and_map(dn, idx);
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 <f.fainelli@gmail.com> --- drivers/irqchip/irq-bcm7038-l1.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)