Re: [RFC] ARCv2: MCIP: Deprecate setting of affinity in Device Tree

[Vineet Gupta]

+CC MarcZ

Hi Marc,

I have a question below

On 11/04/2016 05:17 AM, Yuriy Kolerov wrote:
> Ignore value of interrupt distribution mode for common interrupts in
> IDU since setting an affinity using value from Device Tree is deprecated
> in ARC. Originially it is done in idu_irq_xlate function and it is
> semantically wrong and does not guaranty that an affinity value will be
> set properly.
>
> However it is necessary to set a default affinity value manually for all
> common interrupts since initially all of them are disabled by IDU (a
> CPU mask for common interrupts is set to 0 after CPU reset) and in
> some cases the kernel cannot do it itself after initialization of
> endpoint devices (e.g. when IRQ chip below of IDU does not support
> setting of affinity and it cannot propagate an affinity value to IDU).
>
> P.S.
>
> Despite the fact that the patch works fine I would like to discuss
> few topics which are one way or another related to the original problem.
>
> Despite the fact that this patch works fine it does not work well when
> the intc below IDU does not support setting and propagating of affinity
> (e.g. GPIO intc in AXS103 configuration which does not implement a function
> for setting an affinity). That is why it is necessary to set an affinity
> manually for IDU - you cannot be sure that an affinity will be propagated
> to IDU properly even with support of hierarchical IRQ domains. But an
> affinity *must* be set because IDU common interrupts are disabled by
> default (registers which stores cpu masks are zeroes). In this patch
> I use this soulution in idu_irq_map() function:
>
>     irq_set_affinity(virq, cpu_online_mask);
>
> And there is a second problem. You cannot be sure that all cpus are
> online at the moment of calling idu_irq_map() that is why cpu_online_mask
> mask is used for setting of the initial affinity. It means that affinity
> of all common interrupts is unpredictable after kernel boot (of course it
> would be cool to distribute all common interrupts to all available cores
> by default).
>
> And the third topic is setting of affinity for AXS103 devices. Devices
> in AXS103 are connected to CPU through a chain of GPIO controllers which
> funnel all signals to 2 IDU common interrupts. Thus you cannot simply set
> an affinity for devices using something like:
>
>     echo 2 > /proc/irq/3/...
>
> because 1) GPIO contorrels in AXS103 does not support setting an affinity
> and 2) there is a logical explanation for it: if you want to set an affinity
> for a device connected to GPIO you produce a side affect by touchin an
> affinity for nearby devices.
>
> So if in case of AXS103 you must set an affinity not for devices but for
> GPIO controllers which are connected to IDU. Also you must to know a virq
> of the common interrupt line in IDU to set an affinity for it. Problem is
> that you cannot retrieve this value from /proc/interrupts for AXS103 even
> if support of hierarchical domains is implemented: /proc/interrupts does
> not show chained virqs and virqs without actions (e.g. all virqs of IDU).
>
> P.S.S.
>
> The question is how to solve all those problems properly.
>
> Signed-off-by: Yuriy Kolerov <yuriy.kolerov@xxxxxxxxxxxx>
> ---
>  .../interrupt-controller/snps,archs-idu-intc.txt   |  2 ++
>  arch/arc/kernel/mcip.c                             | 35 +++++-----------------
>  2 files changed, 9 insertions(+), 28 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt
> index 0dcb7c7..e967e7f 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/snps,archs-idu-intc.txt
> @@ -15,6 +15,8 @@ Properties:
>    Second cell specifies the irq distribution mode to cores
>       0=Round Robin; 1=cpu0, 2=cpu1, 4=cpu2, 8=cpu3
>  
> +  The second cell in interrupts property is deprecated and ignored.
> +
>    intc accessed via the special ARC AUX register interface, hence "reg" property
>    is not specified.
>  
> diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
> index 90f9934..2f4b0dc 100644
> --- a/arch/arc/kernel/mcip.c
> +++ b/arch/arc/kernel/mcip.c
> @@ -242,6 +242,7 @@ static int idu_irq_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t
>  {
>  	irq_set_chip_and_handler(virq, &idu_irq_chip, handle_level_irq);
>  	irq_set_status_flags(virq, IRQ_MOVE_PCNTXT);
> +	irq_set_affinity(virq, cpu_online_mask);

So as discussed in a prior thread, we no longer support setting the default
affinity via DT (so removed from xlate function). However we are now using the
domain map function to set "seed" affinity based on cpu_online_mask(). Will that
be the wrong place again to do this too.
Also as Yuriy notes above, it is not guaranteed that all cpus will be online when
the map function is called. So then better to set to boot cpu only : wondering
what approach you guys take for ARM ?

Thx,
-Vineet

>  
>  	return 0;
>  }
> @@ -250,36 +251,14 @@ static int idu_irq_xlate(struct irq_domain *d, struct device_node *n,
>  			 const u32 *intspec, unsigned int intsize,
>  			 irq_hw_number_t *out_hwirq, unsigned int *out_type)
>  {
> -	irq_hw_number_t hwirq = *out_hwirq = intspec[0];
> -	int distri = intspec[1];
> -	unsigned long flags;
> -
> +	/*
> +	 * Ignore value of interrupt distribution mode for common interrupts in
> +	 * IDU which resides in intspec[1] since setting an affinity using value
> +	 * from Device Tree is deprecated in ARC.
> +	 */
> +	*out_hwirq = intspec[0];
>  	*out_type = IRQ_TYPE_NONE;
>  
> -	/* XXX: validate distribution scheme again online cpu mask */
> -	if (distri == 0) {
> -		/* 0 - Round Robin to all cpus, otherwise 1 bit per core */
> -		raw_spin_lock_irqsave(&mcip_lock, flags);
> -		idu_set_dest(hwirq, BIT(num_online_cpus()) - 1);
> -		idu_set_mode(hwirq, IDU_M_TRIG_LEVEL, IDU_M_DISTRI_RR);
> -		raw_spin_unlock_irqrestore(&mcip_lock, flags);
> -	} else {
> -		/*
> -		 * DEST based distribution for Level Triggered intr can only
> -		 * have 1 CPU, so generalize it to always contain 1 cpu
> -		 */
> -		int cpu = ffs(distri);
> -
> -		if (cpu != fls(distri))
> -			pr_warn("IDU irq %lx distri mode set to cpu %x\n",
> -				hwirq, cpu);
> -
> -		raw_spin_lock_irqsave(&mcip_lock, flags);
> -		idu_set_dest(hwirq, cpu);
> -		idu_set_mode(hwirq, IDU_M_TRIG_LEVEL, IDU_M_DISTRI_DEST);
> -		raw_spin_unlock_irqrestore(&mcip_lock, flags);
> -	}
> -
>  	return 0;
>  }
>