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

["majun (F)" <majun258@huawei.com>]

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.
>