| Message ID | 1446798522-28000-5-git-send-email-majun258@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. >
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; }; 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); + } + + 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; + + 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; + + 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; } @@ -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;