| Message ID | 1448248513-39760-4-git-send-email-majun258@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/11/15 03:15, MaJun wrote: > From: Ma Jun <majun258@huawei.com> > > For peripheral devices which connect to mbigen,mbigen is a interrupt > controller. So, we create irq domain for each mbigen device and add > mbigen irq domain into irq hierarchy structure. > > Signed-off-by: Ma Jun <majun258@huawei.com> > --- > drivers/irqchip/irq-mbigen.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 119 insertions(+), 0 deletions(-) > > diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c > index 9f036c2..81ae61f 100644 > --- a/drivers/irqchip/irq-mbigen.c > +++ b/drivers/irqchip/irq-mbigen.c > @@ -16,13 +16,36 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/interrupt.h> > +#include <linux/irqchip.h> > #include <linux/module.h> > +#include <linux/msi.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > > +/* Interrupt numbers per mbigen node supported */ > +#define IRQS_PER_MBIGEN_NODE 128 > + > +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ > +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 > + > +/** > + * In mbigen vector register > + * bit[21:12]: event id value > + * bit[11:0]: device id > + */ > +#define IRQ_EVENT_ID_SHIFT 12 > +#define IRQ_EVENT_ID_MASK 0x3ff > + > +/* register range of each mbigen node */ > +#define MBIGEN_NODE_OFFSET 0x1000 > + > +/* offset of vector register in mbigen node */ > +#define REG_MBIGEN_VEC_OFFSET 0x200 > + > /** > * struct mbigen_device - holds the information of mbigen device. > * > @@ -34,10 +57,94 @@ struct mbigen_device { > void __iomem *base; > }; > > +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) > +{ > + unsigned int nid, pin; > + > + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; > + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; > + pin = hwirq % IRQS_PER_MBIGEN_NODE; > + > + return pin * 4 + nid * MBIGEN_NODE_OFFSET > + + REG_MBIGEN_VEC_OFFSET; > +} > + > +static struct irq_chip mbigen_irq_chip = { > + .name = "mbigen-v2", > +}; > + > +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) > +{ > + struct irq_data *d = irq_get_irq_data(desc->irq); > + void __iomem *base = d->chip_data; > + u32 val; > + > + base += get_mbigen_vec_reg(d->hwirq); > + val = readl_relaxed(base); > + > + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); > + val |= (msg->data << IRQ_EVENT_ID_SHIFT); > + > + writel_relaxed(val, base); nit: It would be good to have a comment explaining why you do not need to program the address of the doorbell... > +} > + > +static int mbigen_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + if (is_of_node(fwspec->fwnode)) { > + if (fwspec->param_count != 2) > + return -EINVAL; > + > + *hwirq = fwspec->param[0]; > + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; > + > + return 0; > + } > + return -EINVAL; > +} > + > +static int mbigen_irq_domain_alloc(struct irq_domain *domain, > + unsigned int virq, > + unsigned int nr_irqs, > + void *args) > +{ > + struct irq_fwspec *fwspec = args; > + irq_hw_number_t hwirq; > + unsigned int type; > + struct mbigen_device *mgn_chip; > + int i, err; > + > + err = mbigen_domain_translate(domain, fwspec, &hwirq, &type); > + if (err) > + return err; > + > + err = platform_msi_domain_alloc(domain, virq, nr_irqs); > + if (err) > + return err; > + > + mgn_chip = platform_msi_get_host_data(domain); > + > + for (i = 0; i < nr_irqs; i++) > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > + &mbigen_irq_chip, mgn_chip->base); > + > + return 0; > +} > + > +static struct irq_domain_ops mbigen_domain_ops = { > + .translate = mbigen_domain_translate, > + .alloc = mbigen_irq_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > static int mbigen_device_probe(struct platform_device *pdev) > { > struct mbigen_device *mgn_chip; > struct resource *res; > + struct irq_domain *domain; > + u32 num_msis; > > mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL); > if (!mgn_chip) > @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev) > if (IS_ERR(mgn_chip->base)) > return PTR_ERR(mgn_chip->base); > > + /* If there is no "num-msis" property, assume 64... */ > + if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) > + num_msis = 64; nit: Is that always true? This has been lifted from my dummy example, so I wonder if that's what you actually want to do. > + > + domain = platform_msi_create_device_domain(&pdev->dev, num_msis, > + mbigen_write_msg, > + &mbigen_domain_ops, > + mgn_chip); > + > + if (!domain) > + return -ENOMEM; > + > platform_set_drvdata(pdev, mgn_chip); > > return 0; > Apart for the two above point, Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
Hi Marc: On 2015/12/3 11:25, Marc Zyngier wrote: > On 23/11/15 03:15, MaJun wrote: >> From: Ma Jun <majun258@huawei.com> >> >> For peripheral devices which connect to mbigen,mbigen is a interrupt >> controller. So, we create irq domain for each mbigen device and add >> mbigen irq domain into irq hierarchy structure. >> >> Signed-off-by: Ma Jun <majun258@huawei.com> >> --- >> drivers/irqchip/irq-mbigen.c | 119 ++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 119 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c >> index 9f036c2..81ae61f 100644 >> --- a/drivers/irqchip/irq-mbigen.c >> +++ b/drivers/irqchip/irq-mbigen.c >> @@ -16,13 +16,36 @@ >> * along with this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include <linux/interrupt.h> >> +#include <linux/irqchip.h> >> #include <linux/module.h> >> +#include <linux/msi.h> >> #include <linux/of_address.h> >> #include <linux/of_irq.h> >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> #include <linux/slab.h> >> >> +/* Interrupt numbers per mbigen node supported */ >> +#define IRQS_PER_MBIGEN_NODE 128 >> + >> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ >> +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 >> + >> +/** >> + * In mbigen vector register >> + * bit[21:12]: event id value >> + * bit[11:0]: device id >> + */ >> +#define IRQ_EVENT_ID_SHIFT 12 >> +#define IRQ_EVENT_ID_MASK 0x3ff >> + >> +/* register range of each mbigen node */ >> +#define MBIGEN_NODE_OFFSET 0x1000 >> + >> +/* offset of vector register in mbigen node */ >> +#define REG_MBIGEN_VEC_OFFSET 0x200 >> + >> /** >> * struct mbigen_device - holds the information of mbigen device. >> * >> @@ -34,10 +57,94 @@ struct mbigen_device { >> void __iomem *base; >> }; >> >> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) >> +{ >> + unsigned int nid, pin; >> + >> + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; >> + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; >> + pin = hwirq % IRQS_PER_MBIGEN_NODE; >> + >> + return pin * 4 + nid * MBIGEN_NODE_OFFSET >> + + REG_MBIGEN_VEC_OFFSET; >> +} >> + >> +static struct irq_chip mbigen_irq_chip = { >> + .name = "mbigen-v2", >> +}; >> + >> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) >> +{ >> + struct irq_data *d = irq_get_irq_data(desc->irq); >> + void __iomem *base = d->chip_data; >> + u32 val; >> + >> + base += get_mbigen_vec_reg(d->hwirq); >> + val = readl_relaxed(base); >> + >> + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); >> + val |= (msg->data << IRQ_EVENT_ID_SHIFT); >> + >> + writel_relaxed(val, base); > > nit: It would be good to have a comment explaining why you do not need > to program the address of the doorbell... The address of doorbell is encoded in mbigen register by default, So, we don't need to program the doorbell address in mbigen driver. I'll add this comment in next version. > >> +} >> + >> +static int mbigen_domain_translate(struct irq_domain *d, >> + struct irq_fwspec *fwspec, >> + unsigned long *hwirq, >> + unsigned int *type) >> +{ >> + if (is_of_node(fwspec->fwnode)) { >> + if (fwspec->param_count != 2) >> + return -EINVAL; >> + >> + *hwirq = fwspec->param[0]; >> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; >> + >> + return 0; >> + } >> + return -EINVAL; >> +} >> + >> +static int mbigen_irq_domain_alloc(struct irq_domain *domain, >> + unsigned int virq, >> + unsigned int nr_irqs, >> + void *args) >> +{ >> + struct irq_fwspec *fwspec = args; >> + irq_hw_number_t hwirq; >> + unsigned int type; >> + struct mbigen_device *mgn_chip; >> + int i, err; >> + >> + err = mbigen_domain_translate(domain, fwspec, &hwirq, &type); >> + if (err) >> + return err; >> + >> + err = platform_msi_domain_alloc(domain, virq, nr_irqs); >> + if (err) >> + return err; >> + >> + mgn_chip = platform_msi_get_host_data(domain); >> + >> + for (i = 0; i < nr_irqs; i++) >> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, >> + &mbigen_irq_chip, mgn_chip->base); >> + >> + return 0; >> +} >> + >> +static struct irq_domain_ops mbigen_domain_ops = { >> + .translate = mbigen_domain_translate, >> + .alloc = mbigen_irq_domain_alloc, >> + .free = irq_domain_free_irqs_common, >> +}; >> + >> static int mbigen_device_probe(struct platform_device *pdev) >> { >> struct mbigen_device *mgn_chip; >> struct resource *res; >> + struct irq_domain *domain; >> + u32 num_msis; >> >> mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL); >> if (!mgn_chip) >> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev) >> if (IS_ERR(mgn_chip->base)) >> return PTR_ERR(mgn_chip->base); >> >> + /* If there is no "num-msis" property, assume 64... */ >> + if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) >> + num_msis = 64; > > nit: Is that always true? This has been lifted from my dummy example, do you mean patch v2? I just checked your patch, this part still exits. so > I wonder if that's what you actually want to do. I think the default num_msis value should be maximum msis(256) the current msi core supported. How about your opinion, or I need to remove this part ? Thanks! Ma Jun > >> + >> + domain = platform_msi_create_device_domain(&pdev->dev, num_msis, >> + mbigen_write_msg, >> + &mbigen_domain_ops, >> + mgn_chip); >> + >> + if (!domain) >> + return -ENOMEM; >> + >> platform_set_drvdata(pdev, mgn_chip); >> >> return 0; >> > > Apart for the two above point, > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > > M. >
On Sun, 6 Dec 2015 15:53:20 -0500 majun <majun258@huawei.com> wrote: > Hi Marc: > > On 2015/12/3 11:25, Marc Zyngier wrote: > > On 23/11/15 03:15, MaJun wrote: > >> From: Ma Jun <majun258@huawei.com> > >> > >> For peripheral devices which connect to mbigen,mbigen is a interrupt > >> controller. So, we create irq domain for each mbigen device and add > >> mbigen irq domain into irq hierarchy structure. > >> > >> Signed-off-by: Ma Jun <majun258@huawei.com> > >> --- > >> drivers/irqchip/irq-mbigen.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > >> 1 files changed, 119 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c > >> index 9f036c2..81ae61f 100644 > >> --- a/drivers/irqchip/irq-mbigen.c > >> +++ b/drivers/irqchip/irq-mbigen.c > >> @@ -16,13 +16,36 @@ > >> * along with this program. If not, see <http://www.gnu.org/licenses/>. > >> */ > >> > >> +#include <linux/interrupt.h> > >> +#include <linux/irqchip.h> > >> #include <linux/module.h> > >> +#include <linux/msi.h> > >> #include <linux/of_address.h> > >> #include <linux/of_irq.h> > >> #include <linux/of_platform.h> > >> #include <linux/platform_device.h> > >> #include <linux/slab.h> > >> > >> +/* Interrupt numbers per mbigen node supported */ > >> +#define IRQS_PER_MBIGEN_NODE 128 > >> + > >> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ > >> +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 > >> + > >> +/** > >> + * In mbigen vector register > >> + * bit[21:12]: event id value > >> + * bit[11:0]: device id > >> + */ > >> +#define IRQ_EVENT_ID_SHIFT 12 > >> +#define IRQ_EVENT_ID_MASK 0x3ff > >> + > >> +/* register range of each mbigen node */ > >> +#define MBIGEN_NODE_OFFSET 0x1000 > >> + > >> +/* offset of vector register in mbigen node */ > >> +#define REG_MBIGEN_VEC_OFFSET 0x200 > >> + > >> /** > >> * struct mbigen_device - holds the information of mbigen device. > >> * > >> @@ -34,10 +57,94 @@ struct mbigen_device { > >> void __iomem *base; > >> }; > >> > >> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) > >> +{ > >> + unsigned int nid, pin; > >> + > >> + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; > >> + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; > >> + pin = hwirq % IRQS_PER_MBIGEN_NODE; > >> + > >> + return pin * 4 + nid * MBIGEN_NODE_OFFSET > >> + + REG_MBIGEN_VEC_OFFSET; > >> +} > >> + > >> +static struct irq_chip mbigen_irq_chip = { > >> + .name = "mbigen-v2", > >> +}; > >> + > >> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) > >> +{ > >> + struct irq_data *d = irq_get_irq_data(desc->irq); > >> + void __iomem *base = d->chip_data; > >> + u32 val; > >> + > >> + base += get_mbigen_vec_reg(d->hwirq); > >> + val = readl_relaxed(base); > >> + > >> + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); > >> + val |= (msg->data << IRQ_EVENT_ID_SHIFT); > >> + > >> + writel_relaxed(val, base); > > > > nit: It would be good to have a comment explaining why you do not need > > to program the address of the doorbell... > > The address of doorbell is encoded in mbigen register by default, > So, we don't need to program the doorbell address in mbigen driver. > > I'll add this comment in next version. > > > > >> +} > >> + > >> +static int mbigen_domain_translate(struct irq_domain *d, > >> + struct irq_fwspec *fwspec, > >> + unsigned long *hwirq, > >> + unsigned int *type) > >> +{ > >> + if (is_of_node(fwspec->fwnode)) { > >> + if (fwspec->param_count != 2) > >> + return -EINVAL; > >> + > >> + *hwirq = fwspec->param[0]; > >> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; > >> + > >> + return 0; > >> + } > >> + return -EINVAL; > >> +} > >> + > >> +static int mbigen_irq_domain_alloc(struct irq_domain *domain, > >> + unsigned int virq, > >> + unsigned int nr_irqs, > >> + void *args) > >> +{ > >> + struct irq_fwspec *fwspec = args; > >> + irq_hw_number_t hwirq; > >> + unsigned int type; > >> + struct mbigen_device *mgn_chip; > >> + int i, err; > >> + > >> + err = mbigen_domain_translate(domain, fwspec, &hwirq, &type); > >> + if (err) > >> + return err; > >> + > >> + err = platform_msi_domain_alloc(domain, virq, nr_irqs); > >> + if (err) > >> + return err; > >> + > >> + mgn_chip = platform_msi_get_host_data(domain); > >> + > >> + for (i = 0; i < nr_irqs; i++) > >> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > >> + &mbigen_irq_chip, mgn_chip->base); > >> + > >> + return 0; > >> +} > >> + > >> +static struct irq_domain_ops mbigen_domain_ops = { > >> + .translate = mbigen_domain_translate, > >> + .alloc = mbigen_irq_domain_alloc, > >> + .free = irq_domain_free_irqs_common, > >> +}; > >> + > >> static int mbigen_device_probe(struct platform_device *pdev) > >> { > >> struct mbigen_device *mgn_chip; > >> struct resource *res; > >> + struct irq_domain *domain; > >> + u32 num_msis; > >> > >> mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL); > >> if (!mgn_chip) > >> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev) > >> if (IS_ERR(mgn_chip->base)) > >> return PTR_ERR(mgn_chip->base); > >> > >> + /* If there is no "num-msis" property, assume 64... */ > >> + if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) > >> + num_msis = 64; > > > > nit: Is that always true? This has been lifted from my dummy example, > > do you mean patch v2? I just checked your patch, this part still exits. It does exist because this example is just a toy, and I wanted to make it easy for people to play with it. > so > > I wonder if that's what you actually want to do. > > I think the default num_msis value should be maximum msis(256) the current > msi core supported. I don't think so. If you have a fallback mechanism, it should reflect the default value on your *own* HW. If there is no common value that is generally used, then you should not have a default. > How about your opinion, or I need to remove this part ? If you don't know what this value should really be, just drop that part, and generate an error when the num-msis property is not present. Thanks, M.
On Mon, Nov 23, 2015 at 11:15:12AM +0800, MaJun wrote: > From: Ma Jun <majun258@huawei.com> > > For peripheral devices which connect to mbigen,mbigen is a interrupt > controller. So, we create irq domain for each mbigen device and add > mbigen irq domain into irq hierarchy structure. > > Signed-off-by: Ma Jun <majun258@huawei.com> > --- > drivers/irqchip/irq-mbigen.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 119 insertions(+), 0 deletions(-) > > diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c > index 9f036c2..81ae61f 100644 > --- a/drivers/irqchip/irq-mbigen.c > +++ b/drivers/irqchip/irq-mbigen.c > @@ -16,13 +16,36 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/interrupt.h> > +#include <linux/irqchip.h> > #include <linux/module.h> > +#include <linux/msi.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > > +/* Interrupt numbers per mbigen node supported */ > +#define IRQS_PER_MBIGEN_NODE 128 > + > +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ > +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 > + > +/** > + * In mbigen vector register > + * bit[21:12]: event id value > + * bit[11:0]: device id > + */ > +#define IRQ_EVENT_ID_SHIFT 12 > +#define IRQ_EVENT_ID_MASK 0x3ff > + > +/* register range of each mbigen node */ > +#define MBIGEN_NODE_OFFSET 0x1000 > + > +/* offset of vector register in mbigen node */ > +#define REG_MBIGEN_VEC_OFFSET 0x200 > + > /** > * struct mbigen_device - holds the information of mbigen device. > * > @@ -34,10 +57,94 @@ struct mbigen_device { > void __iomem *base; > }; > > +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) > +{ > + unsigned int nid, pin; > + > + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; > + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; > + pin = hwirq % IRQS_PER_MBIGEN_NODE; > + > + return pin * 4 + nid * MBIGEN_NODE_OFFSET > + + REG_MBIGEN_VEC_OFFSET; > +} Ok. So your "global" pin id is "global" per mbigen chip. I think it may make more sense to have separate nid and pin fields in your interrupt-specifier, e.g. interrupt = <1 3 x> for nid 1, pin 3. That's easier for someone to check against a datasheet that describes the nid and pin rather than the global number space you've come up with, and also makes it impossible to describe the reserved IRQs. > + > +static struct irq_chip mbigen_irq_chip = { > + .name = "mbigen-v2", > +}; > + > +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) > +{ > + struct irq_data *d = irq_get_irq_data(desc->irq); > + void __iomem *base = d->chip_data; > + u32 val; > + > + base += get_mbigen_vec_reg(d->hwirq); > + val = readl_relaxed(base); > + > + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); > + val |= (msg->data << IRQ_EVENT_ID_SHIFT); > + > + writel_relaxed(val, base); > +} > + > +static int mbigen_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + if (is_of_node(fwspec->fwnode)) { > + if (fwspec->param_count != 2) > + return -EINVAL; > + > + *hwirq = fwspec->param[0]; You should validate the hwirq here. For instance, we never expect a hwirq < RESERVED_IRQ_PER_MBIGEN_CHIP here. > + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; Don't mask out bits you don't expect to be set. Validate that they aren't set and complain if they are. > @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev) > if (IS_ERR(mgn_chip->base)) > return PTR_ERR(mgn_chip->base); > > + /* If there is no "num-msis" property, assume 64... */ > + if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) > + num_msis = 64; The "num-msis" property was mandatory in the binding. We shouldn't need the fallback. It it is missing, print an error, and abort probing here. Thanks, Mark.
Hi Marc and Mark: On 2015/12/11 10:42, Mark Rutland wrote: > On Mon, Nov 23, 2015 at 11:15:12AM +0800, MaJun wrote: >> From: Ma Jun <majun258@huawei.com> >> >> For peripheral devices which connect to mbigen,mbigen is a interrupt >> controller. So, we create irq domain for each mbigen device and add >> mbigen irq domain into irq hierarchy structure. >> >> Signed-off-by: Ma Jun <majun258@huawei.com> >> --- >> drivers/irqchip/irq-mbigen.c | 119 ++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 119 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c >> index 9f036c2..81ae61f 100644 >> --- a/drivers/irqchip/irq-mbigen.c >> +++ b/drivers/irqchip/irq-mbigen.c >> @@ -16,13 +16,36 @@ >> * along with this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include <linux/interrupt.h> >> +#include <linux/irqchip.h> >> #include <linux/module.h> >> +#include <linux/msi.h> >> #include <linux/of_address.h> >> #include <linux/of_irq.h> >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> #include <linux/slab.h> >> >> +/* Interrupt numbers per mbigen node supported */ >> +#define IRQS_PER_MBIGEN_NODE 128 >> + >> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ >> +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 >> + >> +/** >> + * In mbigen vector register >> + * bit[21:12]: event id value >> + * bit[11:0]: device id >> + */ >> +#define IRQ_EVENT_ID_SHIFT 12 >> +#define IRQ_EVENT_ID_MASK 0x3ff >> + >> +/* register range of each mbigen node */ >> +#define MBIGEN_NODE_OFFSET 0x1000 >> + >> +/* offset of vector register in mbigen node */ >> +#define REG_MBIGEN_VEC_OFFSET 0x200 >> + >> /** >> * struct mbigen_device - holds the information of mbigen device. >> * >> @@ -34,10 +57,94 @@ struct mbigen_device { >> void __iomem *base; >> }; >> >> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) >> +{ >> + unsigned int nid, pin; >> + >> + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; >> + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; >> + pin = hwirq % IRQS_PER_MBIGEN_NODE; >> + >> + return pin * 4 + nid * MBIGEN_NODE_OFFSET >> + + REG_MBIGEN_VEC_OFFSET; >> +} > > Ok. So your "global" pin id is "global" per mbigen chip. right. > > I think it may make more sense to have separate nid and pin fields in > your interrupt-specifier, e.g. interrupt = <1 3 x> for nid 1, pin 3. > > That's easier for someone to check against a datasheet that describes > the nid and pin rather than the global number space you've come up with, > and also makes it impossible to describe the reserved IRQs. There are no nid and pin fields in our new datasheet now. All we can see is hardware pin number. So adding nid and pin fields makes the people more confused about using mbigen. Further more, "pin" is not a good variable name. I should name it as "pin_offset" or just"offset" to present the interrupt pin offset to mbigen node. > >> + >> +static struct irq_chip mbigen_irq_chip = { >> + .name = "mbigen-v2", >> +}; >> + >> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) >> +{ >> + struct irq_data *d = irq_get_irq_data(desc->irq); >> + void __iomem *base = d->chip_data; >> + u32 val; >> + >> + base += get_mbigen_vec_reg(d->hwirq); >> + val = readl_relaxed(base); >> + >> + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); >> + val |= (msg->data << IRQ_EVENT_ID_SHIFT); >> + >> + writel_relaxed(val, base); >> +} >> + >> +static int mbigen_domain_translate(struct irq_domain *d, >> + struct irq_fwspec *fwspec, >> + unsigned long *hwirq, >> + unsigned int *type) >> +{ >> + if (is_of_node(fwspec->fwnode)) { >> + if (fwspec->param_count != 2) >> + return -EINVAL; >> + >> + *hwirq = fwspec->param[0]; > > You should validate the hwirq here. For instance, we never expect a > hwirq < RESERVED_IRQ_PER_MBIGEN_CHIP here. Yes, I also think I need to check the hwirq input value. The hwirq should be: hwirq > RESERVED_IRQ_PER_MBIGEN_CHIP && hwirq < MAXIMUM_INTERRUPT_NUMBER > >> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; > > Don't mask out bits you don't expect to be set. Validate that they > aren't set and complain if they are. > I referred Marc's dummy driver when coding this function. Marc, do you have any different comment about these two parts. >> @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev) >> if (IS_ERR(mgn_chip->base)) >> return PTR_ERR(mgn_chip->base); >> >> + /* If there is no "num-msis" property, assume 64... */ >> + if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) >> + num_msis = 64; > > The "num-msis" property was mandatory in the binding. We shouldn't need > the fallback. > > It it is missing, print an error, and abort probing here. > Yes you are right. Thanks MaJun > Thanks, > Mark. > > . >
On 16/12/15 14:57, majun wrote: > Hi Marc and Mark: > > On 2015/12/11 10:42, Mark Rutland wrote: >> On Mon, Nov 23, 2015 at 11:15:12AM +0800, MaJun wrote: >>> From: Ma Jun <majun258@huawei.com> >>> >>> For peripheral devices which connect to mbigen,mbigen is a interrupt >>> controller. So, we create irq domain for each mbigen device and add >>> mbigen irq domain into irq hierarchy structure. >>> >>> Signed-off-by: Ma Jun <majun258@huawei.com> >>> --- >>> drivers/irqchip/irq-mbigen.c | 119 ++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 119 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c >>> index 9f036c2..81ae61f 100644 >>> --- a/drivers/irqchip/irq-mbigen.c >>> +++ b/drivers/irqchip/irq-mbigen.c >>> @@ -16,13 +16,36 @@ >>> * along with this program. If not, see <http://www.gnu.org/licenses/>. >>> */ >>> >>> +#include <linux/interrupt.h> >>> +#include <linux/irqchip.h> >>> #include <linux/module.h> >>> +#include <linux/msi.h> >>> #include <linux/of_address.h> >>> #include <linux/of_irq.h> >>> #include <linux/of_platform.h> >>> #include <linux/platform_device.h> >>> #include <linux/slab.h> >>> >>> +/* Interrupt numbers per mbigen node supported */ >>> +#define IRQS_PER_MBIGEN_NODE 128 >>> + >>> +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ >>> +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 >>> + >>> +/** >>> + * In mbigen vector register >>> + * bit[21:12]: event id value >>> + * bit[11:0]: device id >>> + */ >>> +#define IRQ_EVENT_ID_SHIFT 12 >>> +#define IRQ_EVENT_ID_MASK 0x3ff >>> + >>> +/* register range of each mbigen node */ >>> +#define MBIGEN_NODE_OFFSET 0x1000 >>> + >>> +/* offset of vector register in mbigen node */ >>> +#define REG_MBIGEN_VEC_OFFSET 0x200 >>> + >>> /** >>> * struct mbigen_device - holds the information of mbigen device. >>> * >>> @@ -34,10 +57,94 @@ struct mbigen_device { >>> void __iomem *base; >>> }; >>> >>> +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) >>> +{ >>> + unsigned int nid, pin; >>> + >>> + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; >>> + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; >>> + pin = hwirq % IRQS_PER_MBIGEN_NODE; >>> + >>> + return pin * 4 + nid * MBIGEN_NODE_OFFSET >>> + + REG_MBIGEN_VEC_OFFSET; >>> +} >> >> Ok. So your "global" pin id is "global" per mbigen chip. > > right. > >> >> I think it may make more sense to have separate nid and pin fields in >> your interrupt-specifier, e.g. interrupt = <1 3 x> for nid 1, pin 3. >> >> That's easier for someone to check against a datasheet that describes >> the nid and pin rather than the global number space you've come up with, >> and also makes it impossible to describe the reserved IRQs. > > There are no nid and pin fields in our new datasheet now. > All we can see is hardware pin number. > So adding nid and pin fields makes the people more confused about using > mbigen. > > Further more, "pin" is not a good variable name. I should name it as > "pin_offset" or just"offset" to present the interrupt pin offset to mbigen node. > >> >>> + >>> +static struct irq_chip mbigen_irq_chip = { >>> + .name = "mbigen-v2", >>> +}; >>> + >>> +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) >>> +{ >>> + struct irq_data *d = irq_get_irq_data(desc->irq); >>> + void __iomem *base = d->chip_data; >>> + u32 val; >>> + >>> + base += get_mbigen_vec_reg(d->hwirq); >>> + val = readl_relaxed(base); >>> + >>> + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); >>> + val |= (msg->data << IRQ_EVENT_ID_SHIFT); >>> + >>> + writel_relaxed(val, base); >>> +} >>> + >>> +static int mbigen_domain_translate(struct irq_domain *d, >>> + struct irq_fwspec *fwspec, >>> + unsigned long *hwirq, >>> + unsigned int *type) >>> +{ >>> + if (is_of_node(fwspec->fwnode)) { >>> + if (fwspec->param_count != 2) >>> + return -EINVAL; >>> + >>> + *hwirq = fwspec->param[0]; >> >> You should validate the hwirq here. For instance, we never expect a >> hwirq < RESERVED_IRQ_PER_MBIGEN_CHIP here. > > Yes, I also think I need to check the hwirq input value. > The hwirq should be: > hwirq > RESERVED_IRQ_PER_MBIGEN_CHIP && hwirq < MAXIMUM_INTERRUPT_NUMBER > >> >>> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; >> >> Don't mask out bits you don't expect to be set. Validate that they >> aren't set and complain if they are. >> > > I referred Marc's dummy driver when coding this function. > > Marc, do you have any different comment about these two parts. My dummy driver was exactly that: a dummy. It was not meant to be followed to the letter, but just an example showing how to use a new API. And given that it didn't handle any interrupt, it really didn't matter what it did in the translate function. Here, I think Mark is right, and you should follow his recommendation. Thanks, M.
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c index 9f036c2..81ae61f 100644 --- a/drivers/irqchip/irq-mbigen.c +++ b/drivers/irqchip/irq-mbigen.c @@ -16,13 +16,36 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <linux/interrupt.h> +#include <linux/irqchip.h> #include <linux/module.h> +#include <linux/msi.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/slab.h> +/* Interrupt numbers per mbigen node supported */ +#define IRQS_PER_MBIGEN_NODE 128 + +/* 64 irqs (Pin0-pin63) are reserved for each mbigen chip */ +#define RESERVED_IRQ_PER_MBIGEN_CHIP 64 + +/** + * In mbigen vector register + * bit[21:12]: event id value + * bit[11:0]: device id + */ +#define IRQ_EVENT_ID_SHIFT 12 +#define IRQ_EVENT_ID_MASK 0x3ff + +/* register range of each mbigen node */ +#define MBIGEN_NODE_OFFSET 0x1000 + +/* offset of vector register in mbigen node */ +#define REG_MBIGEN_VEC_OFFSET 0x200 + /** * struct mbigen_device - holds the information of mbigen device. * @@ -34,10 +57,94 @@ struct mbigen_device { void __iomem *base; }; +static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq) +{ + unsigned int nid, pin; + + hwirq -= RESERVED_IRQ_PER_MBIGEN_CHIP; + nid = hwirq / IRQS_PER_MBIGEN_NODE + 1; + pin = hwirq % IRQS_PER_MBIGEN_NODE; + + return pin * 4 + nid * MBIGEN_NODE_OFFSET + + REG_MBIGEN_VEC_OFFSET; +} + +static struct irq_chip mbigen_irq_chip = { + .name = "mbigen-v2", +}; + +static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg) +{ + struct irq_data *d = irq_get_irq_data(desc->irq); + void __iomem *base = d->chip_data; + u32 val; + + base += get_mbigen_vec_reg(d->hwirq); + val = readl_relaxed(base); + + val &= ~(IRQ_EVENT_ID_MASK << IRQ_EVENT_ID_SHIFT); + val |= (msg->data << IRQ_EVENT_ID_SHIFT); + + writel_relaxed(val, base); +} + +static int mbigen_domain_translate(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + if (is_of_node(fwspec->fwnode)) { + if (fwspec->param_count != 2) + return -EINVAL; + + *hwirq = fwspec->param[0]; + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; + + return 0; + } + return -EINVAL; +} + +static int mbigen_irq_domain_alloc(struct irq_domain *domain, + unsigned int virq, + unsigned int nr_irqs, + void *args) +{ + struct irq_fwspec *fwspec = args; + irq_hw_number_t hwirq; + unsigned int type; + struct mbigen_device *mgn_chip; + int i, err; + + err = mbigen_domain_translate(domain, fwspec, &hwirq, &type); + if (err) + return err; + + err = platform_msi_domain_alloc(domain, virq, nr_irqs); + if (err) + return err; + + mgn_chip = platform_msi_get_host_data(domain); + + for (i = 0; i < nr_irqs; i++) + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, + &mbigen_irq_chip, mgn_chip->base); + + return 0; +} + +static struct irq_domain_ops mbigen_domain_ops = { + .translate = mbigen_domain_translate, + .alloc = mbigen_irq_domain_alloc, + .free = irq_domain_free_irqs_common, +}; + static int mbigen_device_probe(struct platform_device *pdev) { struct mbigen_device *mgn_chip; struct resource *res; + struct irq_domain *domain; + u32 num_msis; mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL); if (!mgn_chip) @@ -50,6 +157,18 @@ static int mbigen_device_probe(struct platform_device *pdev) if (IS_ERR(mgn_chip->base)) return PTR_ERR(mgn_chip->base); + /* If there is no "num-msis" property, assume 64... */ + if (of_property_read_u32(pdev->dev.of_node, "num-msis", &num_msis) < 0) + num_msis = 64; + + domain = platform_msi_create_device_domain(&pdev->dev, num_msis, + mbigen_write_msg, + &mbigen_domain_ops, + mgn_chip); + + if (!domain) + return -ENOMEM; + platform_set_drvdata(pdev, mgn_chip); return 0;