Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1272972 > unrolled thread

Re: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation functions

Started byMarc Zyngier <marc.zyngier@arm.com>
First post2015-11-19 10:50 +0100
Last post2015-11-19 12:00 +0100
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation  functions Marc Zyngier <marc.zyngier@arm.com> - 2015-11-19 10:50 +0100
    Re: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation  functions "majun (F)" <majun258@huawei.com> - 2015-11-19 12:00 +0100

#1272972 — Re: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation functions

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-11-19 10:50 +0100
SubjectRe: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation functions
Message-ID<qwu0P-7xT-17@gated-at.bofh.it>
On Fri, 6 Nov 2015 16:28:42 +0800
MaJun <majun258@huawei.com> wrote:

> From: Ma Jun <majun258@huawei.com>
> 
> Add the interrupt controller chip operation functions of mbigen chip.
> 
> Signed-off-by: Ma Jun <majun258@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c |  102 +++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 97 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 71291cb..155c210 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -46,6 +46,19 @@
>  /* offset of vector register in mbigen node */
>  #define REG_MBIGEN_VEC_OFFSET		0x200
>  
> +/**
> + * offset of clear register in mbigen node
> + * This register is used to clear the status
> + * of interrupt
> + */
> +#define REG_MBIGEN_CLEAR_OFFSET		0xa00
> +
> +/**
> + * offset of interrupt type register
> + * This register is used to configure interrupt
> + * trigger type
> + */
> +#define REG_MBIGEN_TYPE_OFFSET		0x0
>  
>  /**
>   * struct mbigen_device - holds the information of mbigen device.
> @@ -64,11 +77,19 @@ struct mbigen_device {
>   * struct mbigen_irq_data - private data of each irq
>   *
>   * @base:		mapped address of mbigen chip
> + * @pin_offset:		local pin offset of interrupt.
>   * @reg_vec:		addr offset of interrupt vector register.
> + * @reg_type:		addr offset of interrupt trigger type register.
> + * @reg_clear:		addr offset of interrupt clear register.
> + * @type:		interrupt trigger type.
>   */
>  struct mbigen_irq_data {
>  	void __iomem		*base;
> +	unsigned int		pin_offset;
>  	unsigned int		reg_vec;
> +	unsigned int		reg_type;
> +	unsigned int		reg_clear;
> +	unsigned int		type;
>  };

I have the same comments here as for patch #3. You're storing
information that are just a pure function of hwirq, and essentially
free to compute at runtime. Please fix it in a similar way.

>  
>  static inline int get_mbigen_vec_reg(u32 nid, u32 offset)
> @@ -77,8 +98,68 @@ static inline int get_mbigen_vec_reg(u32 nid, u32 offset)
>  			+ REG_MBIGEN_VEC_OFFSET;
>  }
>  
> +static int get_mbigen_type_reg(u32 nid, u32 offset)
> +{
> +	int ofst;
> +
> +	ofst = offset / 32 * 4;
> +	return ofst + nid * MBIGEN_NODE_OFFSET
> +		+ REG_MBIGEN_TYPE_OFFSET;
> +}
> +
> +static int get_mbigen_clear_reg(u32 nid, u32 offset)
> +{
> +	int ofst;
> +
> +	ofst = offset / 32 * 4;
> +	return ofst + nid * MBIGEN_NODE_OFFSET
> +		+ REG_MBIGEN_CLEAR_OFFSET;
> +}
> +
> +static void mbigen_eoi_irq(struct irq_data *data)
> +{
> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
> +	u32 mask;
> +
> +	/* only level triggered interrupt need to clear status */
> +	if (mgn_irq_data->type == IRQ_TYPE_LEVEL_HIGH) {
> +		mask = 1 << (mgn_irq_data->pin_offset % 32);
> +		writel_relaxed(mask, mgn_irq_data->reg_clear + mgn_irq_data->base);
> +	}

Does this have the effect of regenerating an edge if the level is still
active?

> +
> +	irq_chip_eoi_parent(data);
> +}
> +
> +static int mbigen_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(d);
> +	u32 mask;
> +	int val;
> +
> +	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> +		return -EINVAL;

If these are the only two interrupt triggers you support, then you
should update the documentation (DT binding) to reflect this, as all it
says is "The 2nd cell is the interrupt trigger type" which is pretty
vague.

> +
> +	mask = 1 << (mgn_irq_data->pin_offset % 32);
> +
> +	val = readl_relaxed(mgn_irq_data->reg_type + mgn_irq_data->base);
> +
> +	if (type == IRQ_TYPE_LEVEL_HIGH)
> +		val |= mask;
> +	else if (type == IRQ_TYPE_EDGE_RISING)
> +		val &= ~mask;

Given that you've excluded anything but LEVEL_HIGH and EDGE_RISING
already, the second if() is superfluous.

> +
> +	writel_relaxed(val, mgn_irq_data->reg_type + mgn_irq_data->base);
> +
> +	return 0;
> +}
> +
>  static struct irq_chip mbigen_irq_chip = {
>  	.name =			"mbigen-v2",
> +	.irq_mask =		irq_chip_mask_parent,
> +	.irq_unmask =		irq_chip_unmask_parent,
> +	.irq_eoi =		mbigen_eoi_irq,
> +	.irq_set_type =		mbigen_set_type,
> +	.irq_set_affinity =	irq_chip_set_affinity_parent,
>  };
>  
>  static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> @@ -94,10 +175,11 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>  	writel_relaxed(val, mgn_irq_data->reg_vec + mgn_irq_data->base);
>  }
>  
> -static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq)
> +static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq,
> +						unsigned int type)
>  {
>  	struct mbigen_irq_data *datap;
> -	unsigned int nid, pin_offset;
> +	unsigned int nid;
>  
>  	datap = kzalloc(sizeof(*datap), GFP_KERNEL);
>  	if (!datap)
> @@ -106,10 +188,20 @@ static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq)
>  	/* get the mbigen node number */
>  	nid = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP) / IRQS_PER_MBIGEN_NODE + 1;
>  
> -	pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP)
> +	datap->pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP)
>  					% IRQS_PER_MBIGEN_NODE;
>  
> -	datap->reg_vec = get_mbigen_vec_reg(nid, pin_offset);
> +	datap->reg_vec = get_mbigen_vec_reg(nid, datap->pin_offset);
> +	datap->reg_type = get_mbigen_type_reg(nid, datap->pin_offset);
> +
> +	/* no clear register for edge triggered interrupt */
> +	if (type == IRQ_TYPE_EDGE_RISING)
> +		datap->reg_clear = 0;
> +	else
> +		datap->reg_clear = get_mbigen_clear_reg(nid,
> +					datap->pin_offset);
> +
> +	datap->type = type;
>  	return datap;
>  }

That function can entirely go.

>  
> @@ -151,7 +243,7 @@ static int mbigen_irq_domain_alloc(struct irq_domain *domain,
>  		return err;
>  
>  	/* set related information of this irq */
> -	mgn_irq_data = set_mbigen_irq_data(hwirq);
> +	mgn_irq_data = set_mbigen_irq_data(hwirq, type);
>  	if (!mgn_irq_data)
>  		return err;
>  

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1273029

From"majun (F)" <majun258@huawei.com>
Date2015-11-19 12:00 +0100
Message-ID<qwv6y-8c4-23@gated-at.bofh.it>
In reply to#1272972
Hi Marc:

在 2015/11/19 17:41, Marc Zyngier 写道:
> On Fri, 6 Nov 2015 16:28:42 +0800
> MaJun <majun258@huawei.com> wrote:
> 
>> From: Ma Jun <majun258@huawei.com>
>>
[...]
>>  struct mbigen_irq_data {
>>  	void __iomem		*base;
>> +	unsigned int		pin_offset;
>>  	unsigned int		reg_vec;
>> +	unsigned int		reg_type;
>> +	unsigned int		reg_clear;
>> +	unsigned int		type;
>>  };
> 
> I have the same comments here as for patch #3. You're storing
> information that are just a pure function of hwirq, and essentially
> free to compute at runtime. Please fix it in a similar way.
> 

ok, I'll fix this.

>>  
>>  static inline int get_mbigen_vec_reg(u32 nid, u32 offset)
>> @@ -77,8 +98,68 @@ static inline int get_mbigen_vec_reg(u32 nid, u32 offset)
>>  			+ REG_MBIGEN_VEC_OFFSET;
>>  }
>>  
>> +static int get_mbigen_type_reg(u32 nid, u32 offset)
>> +{
>> +	int ofst;
>> +
>> +	ofst = offset / 32 * 4;
>> +	return ofst + nid * MBIGEN_NODE_OFFSET
>> +		+ REG_MBIGEN_TYPE_OFFSET;
>> +}
>> +
>> +static int get_mbigen_clear_reg(u32 nid, u32 offset)
>> +{
>> +	int ofst;
>> +
>> +	ofst = offset / 32 * 4;
>> +	return ofst + nid * MBIGEN_NODE_OFFSET
>> +		+ REG_MBIGEN_CLEAR_OFFSET;
>> +}
>> +
>> +static void mbigen_eoi_irq(struct irq_data *data)
>> +{
>> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
>> +	u32 mask;
>> +
>> +	/* only level triggered interrupt need to clear status */
>> +	if (mgn_irq_data->type == IRQ_TYPE_LEVEL_HIGH) {
>> +		mask = 1 << (mgn_irq_data->pin_offset % 32);
>> +		writel_relaxed(mask, mgn_irq_data->reg_clear + mgn_irq_data->base);
>> +	}
> 
> Does this have the effect of regenerating an edge if the level is still
> active?
> 

yes, it does.

>> +
>> +	irq_chip_eoi_parent(data);
>> +}
>> +
>> +static int mbigen_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +	struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(d);
>> +	u32 mask;
>> +	int val;
>> +
>> +	if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>> +		return -EINVAL;
> 
> If these are the only two interrupt triggers you support, then you
> should update the documentation (DT binding) to reflect this, as all it
> says is "The 2nd cell is the interrupt trigger type" which is pretty
> vague.
> 

I'll specify this point in DT binding.

Thanks
Ma Jun

>> +
>> +	mask = 1 << (mgn_irq_data->pin_offset % 32);
>> +
>> +	val = readl_relaxed(mgn_irq_data->reg_type + mgn_irq_data->base);
>> +
>> +	if (type == IRQ_TYPE_LEVEL_HIGH)
>> +		val |= mask;
>> +	else if (type == IRQ_TYPE_EDGE_RISING)
>> +		val &= ~mask;
> 
> Given that you've excluded anything but LEVEL_HIGH and EDGE_RISING
> already, the second if() is superfluous.
> 
>> +
>> +	writel_relaxed(val, mgn_irq_data->reg_type + mgn_irq_data->base);
>> +
>> +	return 0;
>> +}
>> +
>>  static struct irq_chip mbigen_irq_chip = {
>>  	.name =			"mbigen-v2",
>> +	.irq_mask =		irq_chip_mask_parent,
>> +	.irq_unmask =		irq_chip_unmask_parent,
>> +	.irq_eoi =		mbigen_eoi_irq,
>> +	.irq_set_type =		mbigen_set_type,
>> +	.irq_set_affinity =	irq_chip_set_affinity_parent,
>>  };
>>  
>>  static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>> @@ -94,10 +175,11 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
>>  	writel_relaxed(val, mgn_irq_data->reg_vec + mgn_irq_data->base);
>>  }
>>  
>> -static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq)
>> +static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq,
>> +						unsigned int type)
>>  {
>>  	struct mbigen_irq_data *datap;
>> -	unsigned int nid, pin_offset;
>> +	unsigned int nid;
>>  
>>  	datap = kzalloc(sizeof(*datap), GFP_KERNEL);
>>  	if (!datap)
>> @@ -106,10 +188,20 @@ static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq)
>>  	/* get the mbigen node number */
>>  	nid = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP) / IRQS_PER_MBIGEN_NODE + 1;
>>  
>> -	pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP)
>> +	datap->pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP)
>>  					% IRQS_PER_MBIGEN_NODE;
>>  
>> -	datap->reg_vec = get_mbigen_vec_reg(nid, pin_offset);
>> +	datap->reg_vec = get_mbigen_vec_reg(nid, datap->pin_offset);
>> +	datap->reg_type = get_mbigen_type_reg(nid, datap->pin_offset);
>> +
>> +	/* no clear register for edge triggered interrupt */
>> +	if (type == IRQ_TYPE_EDGE_RISING)
>> +		datap->reg_clear = 0;
>> +	else
>> +		datap->reg_clear = get_mbigen_clear_reg(nid,
>> +					datap->pin_offset);
>> +
>> +	datap->type = type;
>>  	return datap;
>>  }
> 
> That function can entirely go.
> 
>>  
>> @@ -151,7 +243,7 @@ static int mbigen_irq_domain_alloc(struct irq_domain *domain,
>>  		return err;
>>  
>>  	/* set related information of this irq */
>> -	mgn_irq_data = set_mbigen_irq_data(hwirq);
>> +	mgn_irq_data = set_mbigen_irq_data(hwirq, type);
>>  	if (!mgn_irq_data)
>>  		return err;
>>  
> 
> Thanks,
> 
> 	M.
> 

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web