Re: [PATCH 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support

[Thomas Gleixner]

On Thu, 17 Sep 2015, Oleksij Rempel wrote:
>  static struct icoll_priv icoll_priv;
>  static struct irq_domain *icoll_domain;
> +static DEFINE_RAW_SPINLOCK(icoll_lock);
> +
> +/* calculate bit offset depending on number of intterupt per register */
> +static u32 icoll_intr_bitshift(struct irq_data *d, u32 bit)
> +{
> +	/*
> +	 * We expect intr_per_reg to be 4 or 1, it means
> +	 * "n" will be 3 or 0.
> +	 */
> +	int n = icoll_priv.intr_per_reg - 1;
> +
> +	/*
> +	 * If n = 0, "bit" is never shifted.
> +	 * If n = 3, mask lower part of hwirq to convert it
> +	 * in 0, 1, 2 or 3 and then multiply it by 8 (or shift by 3)
> +	 */
> +	return bit << ((d->hwirq & n) << n);
> +}
> +
> +/* calculate mem offset depending on number of intterupt per register */
> +static void __iomem *icoll_intr_reg(struct irq_data *d)
> +{
> +	int n = icoll_priv.intr_per_reg >> 1;
> +
> +	/* offset = hwirq / intr_per_reg * 0x10 */
> +	return icoll_priv.intr + ((d->hwirq >> n) * 0x10);
> +}
>  
>  static void icoll_ack_irq(struct irq_data *d)
>  {
> @@ -76,14 +107,21 @@ static void icoll_ack_irq(struct irq_data *d)
>  
>  static void icoll_mask_irq(struct irq_data *d)
>  {
> -	__raw_writel(BM_ICOLL_INTR_ENABLE,
> -			icoll_priv.intr + CLR_REG + HW_ICOLL_INTERRUPTn(d->hwirq));
> +	__raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE),
> +			icoll_intr_reg(d) + CLR_REG);
>  }
>  
>  static void icoll_unmask_irq(struct irq_data *d)
>  {
> -	__raw_writel(BM_ICOLL_INTR_ENABLE,
> -			icoll_priv.intr + SET_REG + HW_ICOLL_INTERRUPTn(d->hwirq));
> +	raw_spin_lock(&icoll_lock);
> +	if (icoll_priv.clear)
> +		__raw_writel(ASM9260_BM_CLEAR_BIT(d->hwirq),
> +				icoll_priv.clear +
> +				ASM9260_HW_ICOLL_CLEARn(d->hwirq));
> +
> +	__raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE),
> +			icoll_intr_reg(d) + SET_REG);
> +	raw_spin_unlock(&icoll_lock);
>  }

I don't think this is a brilliant idea. You burden the existing users
with that bit shift mask machinery, an extra conditional and the
spinlock in the unmask path.

You can simply implement a seperate pair of callbacks and assign them
at setup time.

Btw, why is that spinlock only in unmask()? 
  
>  static struct irq_chip mxs_icoll_chip = {
> @@ -115,12 +153,34 @@ static const struct irq_domain_ops icoll_irq_domain_ops = {
>  	.xlate = irq_domain_xlate_onecell,
>  };
>  
> +static void __init icoll_add_domain(struct device_node *np,
> +			  int num)
> +{
> +	icoll_domain = irq_domain_add_linear(np, num,
> +					     &icoll_irq_domain_ops, NULL);
> +
> +	if (!icoll_domain)
> +		panic("%s: unable add irq domain", np->full_name);
> +	irq_set_default_host(icoll_domain);
> +	set_handle_irq(icoll_handle_irq);
> +}
> +
> +static void __iomem * __init icoll_init_iobase(struct device_node *np)
> +{
> +	void __iomem *icoll_base;
> +
> +	icoll_base = of_io_request_and_map(np, 0, np->name);
> +	if (!icoll_base)
> +		panic("%s: unable to map resource", np->full_name);
> +	return icoll_base;
> +}
> +
>  static int __init icoll_of_init(struct device_node *np,
>  			  struct device_node *interrupt_parent)
>  {
> -	void __iomem *icoll_base = of_iomap(np, 0);
> -	WARN_ON(!icoll_base);
> +	void __iomem *icoll_base;
>  
> +	icoll_base		= icoll_init_iobase(np);
>  	icoll_priv.vector	= icoll_base + HW_ICOLL_VECTOR;
>  	icoll_priv.levelack	= icoll_base + HW_ICOLL_LEVELACK;
>  	icoll_priv.ctrl		= icoll_base + HW_ICOLL_CTRL;
> @@ -135,8 +195,38 @@ static int __init icoll_of_init(struct device_node *np,
>  	 */
>  	stmp_reset_block(icoll_priv.ctrl);
>  
> -	icoll_domain = irq_domain_add_linear(np, ICOLL_NUM_IRQS,
> -					     &icoll_irq_domain_ops, NULL);
> +	icoll_add_domain(np, ICOLL_NUM_IRQS);
> +
>  	return icoll_domain ? 0 : -ENODEV;
>  }

That part of the patch should move into the preparatory patch. It has
nothing to do with asm9260.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/