Hi Marc:
在 2015/11/19 17:41, Marc Zyngier 写道:
> On Fri, 6 Nov 2015 16:28:42 +0800
> MaJun <majun...@huawei.com> wrote:
>
>> From: Ma Jun <majun...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
