| Message ID | 20180705124011.7661-10-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/07/18 13:40, Miquel Raynal wrote: > An SEI driver provides an MSI domain through which it is possible to > raise SEIs. Is that really relevant to this patch? It is about the ICU driver, and I would expect you to explain how the ICU so far only handled NSR interrupts through GICP, but now can also generate SEIs by routing MSIs through the SEI block. > > Handle the NSR probe function in a more generic way to support other > type of interrupts (ie. the SEIs). > > Each interrupt domain is a tree domain because of maximum 207 entries. > Reallocating an ICU slot is prevented by the use of an ICU-wide bitmap. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/irqchip/irq-mvebu-icu.c | 148 +++++++++++++++++++++++++++++++++------- > 1 file changed, 123 insertions(+), 25 deletions(-) > > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c > index bb4f06833d17..0cf741dd0f51 100644 > --- a/drivers/irqchip/irq-mvebu-icu.c > +++ b/drivers/irqchip/irq-mvebu-icu.c > @@ -26,6 +26,10 @@ > #define ICU_SETSPI_NSR_AH 0x14 > #define ICU_CLRSPI_NSR_AL 0x18 > #define ICU_CLRSPI_NSR_AH 0x1c > +#define ICU_SET_SEI_AL 0x50 > +#define ICU_SET_SEI_AH 0x54 > +#define ICU_CLR_SEI_AL 0x58 > +#define ICU_CLR_SEI_AH 0x5C > #define ICU_INT_CFG(x) (0x100 + 4 * (x)) > #define ICU_INT_ENABLE BIT(24) > #define ICU_IS_EDGE BIT(28) > @@ -36,12 +40,28 @@ > #define ICU_SATA0_ICU_ID 109 > #define ICU_SATA1_ICU_ID 107 > > +struct mvebu_icu_subset_data { > + unsigned int icu_group; > + unsigned int offset_set_ah; > + unsigned int offset_set_al; > + unsigned int offset_clr_ah; > + unsigned int offset_clr_al; > +}; > + > struct mvebu_icu { > struct irq_chip irq_chip; > void __iomem *base; > struct device *dev; > bool is_legacy; > + /* Lock on interrupt allocations/releases */ > + struct mutex msi_lock; > + DECLARE_BITMAP(msi_bitmap, ICU_MAX_IRQS); > +}; > + > +struct mvebu_icu_msi_data { > + struct mvebu_icu *icu; > atomic_t initialized; > + const struct mvebu_icu_subset_data *subset_data; > }; > > struct mvebu_icu_irq_data { > @@ -50,28 +70,38 @@ struct mvebu_icu_irq_data { > unsigned int type; > }; > > -static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) > +static void mvebu_icu_init(struct mvebu_icu *icu, > + struct mvebu_icu_msi_data *msi_data, > + struct msi_msg *msg) > { > - if (atomic_cmpxchg(&icu->initialized, false, true)) > + const struct mvebu_icu_subset_data *subset = msi_data->subset_data; > + > + if (atomic_cmpxchg(&msi_data->initialized, false, true)) > + return; > + > + /* Set 'SET' ICU SPI message address in AP */ > + writel_relaxed(msg[0].address_hi, icu->base + subset->offset_set_ah); > + writel_relaxed(msg[0].address_lo, icu->base + subset->offset_set_al); > + > + if (subset->icu_group != ICU_GRP_NSR) > return; > > - /* Set Clear/Set ICU SPI message address in AP */ > - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); > - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); > - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); > - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); > + /* Set 'CLEAR' ICU SPI message address in AP (level-MSI only) */ > + writel_relaxed(msg[1].address_hi, icu->base + subset->offset_clr_ah); > + writel_relaxed(msg[1].address_lo, icu->base + subset->offset_clr_al); > } > > static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) > { > struct irq_data *d = irq_get_irq_data(desc->irq); > + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d->domain); > struct mvebu_icu_irq_data *icu_irqd = d->chip_data; > struct mvebu_icu *icu = icu_irqd->icu; > unsigned int icu_int; > > if (msg->address_lo || msg->address_hi) { > - /* One off initialization */ > - mvebu_icu_init(icu, msg); > + /* One off initialization per domain */ > + mvebu_icu_init(icu, msi_data, msg); > /* Configure the ICU with irq number & type */ > icu_int = msg->data | ICU_INT_ENABLE; > if (icu_irqd->type & IRQ_TYPE_EDGE_RISING) > @@ -105,7 +135,8 @@ static int > mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > unsigned long *hwirq, unsigned int *type) > { > - struct mvebu_icu *icu = platform_msi_get_host_data(d); > + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d); > + struct mvebu_icu *icu = msi_data->icu; > unsigned int param_count = icu->is_legacy ? 3 : 2; > > /* Check the count of the parameters in dt */ > @@ -117,7 +148,7 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > > if (icu->is_legacy) { > *hwirq = fwspec->param[1]; > - *type = fwspec->param[2]; > + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > if (fwspec->param[0] != ICU_GRP_NSR) { > dev_err(icu->dev, "wrong ICU group type %x\n", > fwspec->param[0]); > @@ -125,7 +156,7 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > } > } else { > *hwirq = fwspec->param[0]; > - *type = fwspec->param[1]; > + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; Shouldn't this change (and the one just above) be moved to another patch (such as patch #6)? They feel oddly out of place here. > } > > if (*hwirq >= ICU_MAX_IRQS) { > @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > return -EINVAL; > } > > - /* Mask the type to prevent wrong DT configuration */ > - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > + /* > + * The ICU receives level-interrupts. MSI SEI are > + * edge-interrupts while MSI NSR are level-interrupts. Update the type > + * accordingly for the parent irqchip. > + */ > + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) > + *type = IRQ_TYPE_EDGE_RISING; That's interesting. How is the resampling done here? > > return 0; > } > > +static int mvebu_icu_msi_bitmap_region_alloc(struct mvebu_icu *icu, int hwirq) > +{ > + int ret; > + > + mutex_lock(&icu->msi_lock); > + ret = bitmap_allocate_region(icu->msi_bitmap, hwirq, 0); See my earlier comment about the use of find_first_free/set_bit. > + mutex_unlock(&icu->msi_lock); > + > + return ret; > +} > + > +static void mvebu_icu_msi_bitmap_region_release(struct mvebu_icu *icu, > + int hwirq) > +{ > + mutex_lock(&icu->msi_lock); > + bitmap_release_region(icu->msi_bitmap, hwirq, 0); > + mutex_unlock(&icu->msi_lock); > +} > + > static int > mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > unsigned int nr_irqs, void *args) > @@ -146,7 +201,9 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > int err; > unsigned long hwirq; > struct irq_fwspec *fwspec = args; > - struct mvebu_icu *icu = platform_msi_get_host_data(domain); > + struct mvebu_icu_msi_data *msi_data = > + platform_msi_get_host_data(domain); On a single line, please. > + struct mvebu_icu *icu = msi_data->icu; > struct mvebu_icu_irq_data *icu_irqd; > > icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL); > @@ -160,16 +217,20 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > goto free_irqd; > } > > + err = mvebu_icu_msi_bitmap_region_alloc(icu, hwirq); > + if (err) > + goto free_irqd; > + > if (icu->is_legacy) > icu_irqd->icu_group = fwspec->param[0]; > else > - icu_irqd->icu_group = ICU_GRP_NSR; > + icu_irqd->icu_group = msi_data->subset_data->icu_group; > icu_irqd->icu = icu; > > err = platform_msi_domain_alloc(domain, virq, nr_irqs); > if (err) { > dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n"); > - goto free_irqd; > + goto free_bitmap; > } > > /* Make sure there is no interrupt left pending by the firmware */ > @@ -188,6 +249,8 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > free_msi: > platform_msi_domain_free(domain, virq, nr_irqs); > +free_bitmap: > + mvebu_icu_msi_bitmap_region_release(icu, hwirq); > free_irqd: > kfree(icu_irqd); > return err; > @@ -197,12 +260,17 @@ static void > mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq, > unsigned int nr_irqs) > { > + struct mvebu_icu_msi_data *msi_data = > + platform_msi_get_host_data(domain); Single line. > + struct mvebu_icu *icu = msi_data->icu; > struct irq_data *d = irq_get_irq_data(virq); > struct mvebu_icu_irq_data *icu_irqd = d->chip_data; > > kfree(icu_irqd); > > platform_msi_domain_free(domain, virq, nr_irqs); > + > + mvebu_icu_msi_bitmap_region_release(icu, d->hwirq); > } > > static const struct irq_domain_ops mvebu_icu_domain_ops = { > @@ -211,28 +279,54 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = { > .free = mvebu_icu_irq_domain_free, > }; > > +static const struct mvebu_icu_subset_data mvebu_icu_nsr_subset_data = { > + .icu_group = ICU_GRP_NSR, > + .offset_set_ah = ICU_SETSPI_NSR_AH, > + .offset_set_al = ICU_SETSPI_NSR_AL, > + .offset_clr_ah = ICU_CLRSPI_NSR_AH, > + .offset_clr_al = ICU_CLRSPI_NSR_AL, > +}; > + > +static const struct mvebu_icu_subset_data mvebu_icu_sei_subset_data = { > + .icu_group = ICU_GRP_SEI, > + .offset_set_ah = ICU_SET_SEI_AH, > + .offset_set_al = ICU_SET_SEI_AL, > +}; > + > static const struct of_device_id mvebu_icu_subset_of_match[] = { > { > .compatible = "marvell,cp110-icu-nsr", > + .data = &mvebu_icu_nsr_subset_data, > + }, > + { > + .compatible = "marvell,cp110-icu-sei", > + .data = &mvebu_icu_sei_subset_data, > }, > {}, > }; > > static int mvebu_icu_subset_probe(struct platform_device *pdev) > { > + struct mvebu_icu_msi_data *msi_data; > struct device_node *msi_parent_dn; > struct device *dev = &pdev->dev; > struct irq_domain *irq_domain; > - struct mvebu_icu *icu; > + > + msi_data = devm_kzalloc(dev, sizeof(*msi_data), GFP_KERNEL); > + if (!msi_data) > + return -ENOMEM; > > /* > * Device data being populated means we are using the legacy bindings. > * Using the parent device data means we are using the new bindings. > */ > - if (dev_get_drvdata(dev)) > - icu = dev_get_drvdata(dev); > - else > - icu = dev_get_drvdata(dev->parent); > + if (dev_get_drvdata(dev)) { > + msi_data->icu = dev_get_drvdata(dev); > + msi_data->subset_data = &mvebu_icu_nsr_subset_data; > + } else { > + msi_data->icu = dev_get_drvdata(dev->parent); > + msi_data->subset_data = of_device_get_match_data(dev); > + } > > dev->msi_domain = of_msi_get_domain(dev, dev->of_node, > DOMAIN_BUS_PLATFORM_MSI); > @@ -246,7 +340,7 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev) > irq_domain = platform_msi_create_device_tree_domain(dev, ICU_MAX_IRQS, > mvebu_icu_write_msg, > &mvebu_icu_domain_ops, > - icu); > + msi_data); > if (!irq_domain) { > dev_err(dev, "Failed to create ICU MSI domain\n"); > return -ENOMEM; > @@ -284,6 +378,8 @@ static int mvebu_icu_probe(struct platform_device *pdev) > return PTR_ERR(icu->base); > } > > + mutex_init(&icu->msi_lock); > + > icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > "ICU.%x", > (unsigned int)res->start); > @@ -308,7 +404,7 @@ static int mvebu_icu_probe(struct platform_device *pdev) > #endif > > /* > - * Clean all ICU interrupts with type SPI_NSR, required to > + * Clean all ICU interrupts of type NSR and SEI, required to > * avoid unpredictable SPI assignments done by firmware. > */ > for (i = 0 ; i < ICU_MAX_IRQS ; i++) { > @@ -331,7 +427,9 @@ static int mvebu_icu_probe(struct platform_device *pdev) > } > > static const struct of_device_id mvebu_icu_of_match[] = { > - { .compatible = "marvell,cp110-icu", }, > + { > + .compatible = "marvell,cp110-icu", > + }, Pointless change? > {}, > }; > > Thanks, M.
Hi Marc, I'm fine with the rest of the comments, please find just one last question below. [...] > > @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > > return -EINVAL; > > } > > > > - /* Mask the type to prevent wrong DT configuration */ > > - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > > + /* > > + * The ICU receives level-interrupts. MSI SEI are > > + * edge-interrupts while MSI NSR are level-interrupts. Update the type > > + * accordingly for the parent irqchip. > > + */ > > + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) > > + *type = IRQ_TYPE_EDGE_RISING; > > That's interesting. How is the resampling done here? I'm not sure to understand the question. What does 'resampling' means in such context? MSI SEIs are of type "edge" and use the traditional MSI signalling infrastructure. I'm asking to be sure not to ignore something wrong in my code. > > > > > return 0; > > } Thanks, Miquèl
On 21/08/18 10:08, Miquel Raynal wrote: > Hi Marc, > > I'm fine with the rest of the comments, please find just one last > question below. > > [...] > >>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, >>> return -EINVAL; >>> } >>> >>> - /* Mask the type to prevent wrong DT configuration */ >>> - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; >>> + /* >>> + * The ICU receives level-interrupts. MSI SEI are >>> + * edge-interrupts while MSI NSR are level-interrupts. Update the type >>> + * accordingly for the parent irqchip. >>> + */ >>> + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) >>> + *type = IRQ_TYPE_EDGE_RISING; >> >> That's interesting. How is the resampling done here? > > I'm not sure to understand the question. What does 'resampling' means > in such context? MSI SEIs are of type "edge" and use the traditional > MSI signalling infrastructure. I'm asking to be sure not to ignore > something wrong in my code. You seems to be turning a level interrupt into an edge. You can do that, but only if you resample the level on EOI (and regenerate the corresponding edge if the level is still high). Or am I reading it the wrong way? M.
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 10:19:04 +0100: > On 21/08/18 10:08, Miquel Raynal wrote: > > Hi Marc, > > > > I'm fine with the rest of the comments, please find just one last > > question below. > > > > [...] > > > >>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > >>> return -EINVAL; > >>> } > >>> > >>> - /* Mask the type to prevent wrong DT configuration */ > >>> - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > >>> + /* > >>> + * The ICU receives level-interrupts. MSI SEI are > >>> + * edge-interrupts while MSI NSR are level-interrupts. Update the type > >>> + * accordingly for the parent irqchip. > >>> + */ > >>> + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) > >>> + *type = IRQ_TYPE_EDGE_RISING; > >> > >> That's interesting. How is the resampling done here? > > > > I'm not sure to understand the question. What does 'resampling' means > > in such context? MSI SEIs are of type "edge" and use the traditional > > MSI signalling infrastructure. I'm asking to be sure not to ignore > > something wrong in my code. > > You seems to be turning a level interrupt into an edge. If is an SEI interrupt, it cannot be a level interrupt and the type will be IRQ_TYPE_EDGE_RISING. > You can do that, > but only if you resample the level on EOI (and regenerate the > corresponding edge if the level is still high). What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. I thought more clear to enforce it but if this unclear and useless, let's drop it? > > Or am I reading it the wrong way? No, that's probably me not understanding correctly the purpose of the above function. Thanks, Miquèl
On 21/08/18 11:28, Miquel Raynal wrote: > Hi Marc, > > Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 10:19:04 > +0100: > >> On 21/08/18 10:08, Miquel Raynal wrote: >>> Hi Marc, >>> >>> I'm fine with the rest of the comments, please find just one last >>> question below. >>> >>> [...] >>> >>>>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, >>>>> return -EINVAL; >>>>> } >>>>> >>>>> - /* Mask the type to prevent wrong DT configuration */ >>>>> - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; >>>>> + /* >>>>> + * The ICU receives level-interrupts. MSI SEI are >>>>> + * edge-interrupts while MSI NSR are level-interrupts. Update the type >>>>> + * accordingly for the parent irqchip. >>>>> + */ >>>>> + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) >>>>> + *type = IRQ_TYPE_EDGE_RISING; >>>> >>>> That's interesting. How is the resampling done here? >>> >>> I'm not sure to understand the question. What does 'resampling' means >>> in such context? MSI SEIs are of type "edge" and use the traditional >>> MSI signalling infrastructure. I'm asking to be sure not to ignore >>> something wrong in my code. >> >> You seems to be turning a level interrupt into an edge. > > If is an SEI interrupt, it cannot be a level interrupt and the type > will be IRQ_TYPE_EDGE_RISING. > >> You can do that, >> but only if you resample the level on EOI (and regenerate the >> corresponding edge if the level is still high). > > What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, > *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, > as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. > I thought more clear to enforce it but if this unclear and useless, > let's drop it? I think overriding the trigger that way is very confusing. Either it comes from DT, or it comes from the MSI framwork. In both cases, it has to be correct, and overriding it is just papering over other bugs. I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger. Thanks, M.
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 11:37:47 +0100: > On 21/08/18 11:28, Miquel Raynal wrote: > > Hi Marc, > > > > Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 10:19:04 > > +0100: > > > >> On 21/08/18 10:08, Miquel Raynal wrote: > >>> Hi Marc, > >>> > >>> I'm fine with the rest of the comments, please find just one last > >>> question below. > >>> > >>> [...] > >>> > >>>>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > >>>>> return -EINVAL; > >>>>> } > >>>>> > >>>>> - /* Mask the type to prevent wrong DT configuration */ > >>>>> - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > >>>>> + /* > >>>>> + * The ICU receives level-interrupts. MSI SEI are > >>>>> + * edge-interrupts while MSI NSR are level-interrupts. Update the type > >>>>> + * accordingly for the parent irqchip. > >>>>> + */ > >>>>> + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) > >>>>> + *type = IRQ_TYPE_EDGE_RISING; > >>>> > >>>> That's interesting. How is the resampling done here? > >>> > >>> I'm not sure to understand the question. What does 'resampling' means > >>> in such context? MSI SEIs are of type "edge" and use the traditional > >>> MSI signalling infrastructure. I'm asking to be sure not to ignore > >>> something wrong in my code. > >> > >> You seems to be turning a level interrupt into an edge. > > > > If is an SEI interrupt, it cannot be a level interrupt and the type > > will be IRQ_TYPE_EDGE_RISING. > > > >> You can do that, > >> but only if you resample the level on EOI (and regenerate the > >> corresponding edge if the level is still high). > > > > What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, > > *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, > > as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. > > I thought more clear to enforce it but if this unclear and useless, > > let's drop it? > > I think overriding the trigger that way is very confusing. Either it > comes from DT, or it comes from the MSI framwork. In both cases, it has > to be correct, and overriding it is just papering over other bugs. > > I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger. Fine by me. Thanks for all the guidance, I'll send a new iteration ASAP (with the bindings updated). Thanks, Miquèl
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 11:37:47 +0100: > On 21/08/18 11:28, Miquel Raynal wrote: > > Hi Marc, > > > > Marc Zyngier <marc.zyngier@arm.com> wrote on Tue, 21 Aug 2018 10:19:04 > > +0100: > > > >> On 21/08/18 10:08, Miquel Raynal wrote: > >>> Hi Marc, > >>> > >>> I'm fine with the rest of the comments, please find just one last > >>> question below. > >>> > >>> [...] > >>> > >>>>> @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, > >>>>> return -EINVAL; > >>>>> } > >>>>> > >>>>> - /* Mask the type to prevent wrong DT configuration */ > >>>>> - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > >>>>> + /* > >>>>> + * The ICU receives level-interrupts. MSI SEI are > >>>>> + * edge-interrupts while MSI NSR are level-interrupts. Update the type > >>>>> + * accordingly for the parent irqchip. > >>>>> + */ > >>>>> + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) > >>>>> + *type = IRQ_TYPE_EDGE_RISING; > >>>> > >>>> That's interesting. How is the resampling done here? > >>> > >>> I'm not sure to understand the question. What does 'resampling' means > >>> in such context? MSI SEIs are of type "edge" and use the traditional > >>> MSI signalling infrastructure. I'm asking to be sure not to ignore > >>> something wrong in my code. > >> > >> You seems to be turning a level interrupt into an edge. > > > > If is an SEI interrupt, it cannot be a level interrupt and the type > > will be IRQ_TYPE_EDGE_RISING. > > > >> You can do that, > >> but only if you resample the level on EOI (and regenerate the > >> corresponding edge if the level is still high). > > > > What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, > > *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, > > as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. > > I thought more clear to enforce it but if this unclear and useless, > > let's drop it? > > I think overriding the trigger that way is very confusing. Either it > comes from DT, or it comes from the MSI framwork. In both cases, it has > to be correct, and overriding it is just papering over other bugs. > > I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger. Actually I do remember now why I enforced the type: Let's say my thermal IP in a CP110 triggers an interrupt. This interrupt is of type LEVEL_HIGH, and it is declared in the DT as: thermal: thermal-sensor@... { [...] interrupts-extended = <&icu_sei 116 IRQ_TYPE_LEVEL_HIGH>; }; The ICU callback ->translate() will be called with fwspec being the C view of the above "interrupts-extended" property, so the interrupt type will be LEVEL_HIGH. However, the "icu_sei" parent is the SEI IP in the AP806 and this IP only receives edge interrupts. What happens is that the SEI's ->set_type() callback is called with LEVEL_HIGH type (while it only supports EDGE_RISING interrupts) and I want the driver to throw an error in this case. My way of handling this was to force *type to be EDGE_RISING in the ICU ->translate() callback whenever an SEI was triggered. As this seems not to be correct, how would you handle such situation? Thanks, Miquèl
diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c index bb4f06833d17..0cf741dd0f51 100644 --- a/drivers/irqchip/irq-mvebu-icu.c +++ b/drivers/irqchip/irq-mvebu-icu.c @@ -26,6 +26,10 @@ #define ICU_SETSPI_NSR_AH 0x14 #define ICU_CLRSPI_NSR_AL 0x18 #define ICU_CLRSPI_NSR_AH 0x1c +#define ICU_SET_SEI_AL 0x50 +#define ICU_SET_SEI_AH 0x54 +#define ICU_CLR_SEI_AL 0x58 +#define ICU_CLR_SEI_AH 0x5C #define ICU_INT_CFG(x) (0x100 + 4 * (x)) #define ICU_INT_ENABLE BIT(24) #define ICU_IS_EDGE BIT(28) @@ -36,12 +40,28 @@ #define ICU_SATA0_ICU_ID 109 #define ICU_SATA1_ICU_ID 107 +struct mvebu_icu_subset_data { + unsigned int icu_group; + unsigned int offset_set_ah; + unsigned int offset_set_al; + unsigned int offset_clr_ah; + unsigned int offset_clr_al; +}; + struct mvebu_icu { struct irq_chip irq_chip; void __iomem *base; struct device *dev; bool is_legacy; + /* Lock on interrupt allocations/releases */ + struct mutex msi_lock; + DECLARE_BITMAP(msi_bitmap, ICU_MAX_IRQS); +}; + +struct mvebu_icu_msi_data { + struct mvebu_icu *icu; atomic_t initialized; + const struct mvebu_icu_subset_data *subset_data; }; struct mvebu_icu_irq_data { @@ -50,28 +70,38 @@ struct mvebu_icu_irq_data { unsigned int type; }; -static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) +static void mvebu_icu_init(struct mvebu_icu *icu, + struct mvebu_icu_msi_data *msi_data, + struct msi_msg *msg) { - if (atomic_cmpxchg(&icu->initialized, false, true)) + const struct mvebu_icu_subset_data *subset = msi_data->subset_data; + + if (atomic_cmpxchg(&msi_data->initialized, false, true)) + return; + + /* Set 'SET' ICU SPI message address in AP */ + writel_relaxed(msg[0].address_hi, icu->base + subset->offset_set_ah); + writel_relaxed(msg[0].address_lo, icu->base + subset->offset_set_al); + + if (subset->icu_group != ICU_GRP_NSR) return; - /* Set Clear/Set ICU SPI message address in AP */ - writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH); - writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL); - writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH); - writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL); + /* Set 'CLEAR' ICU SPI message address in AP (level-MSI only) */ + writel_relaxed(msg[1].address_hi, icu->base + subset->offset_clr_ah); + writel_relaxed(msg[1].address_lo, icu->base + subset->offset_clr_al); } static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg) { struct irq_data *d = irq_get_irq_data(desc->irq); + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d->domain); struct mvebu_icu_irq_data *icu_irqd = d->chip_data; struct mvebu_icu *icu = icu_irqd->icu; unsigned int icu_int; if (msg->address_lo || msg->address_hi) { - /* One off initialization */ - mvebu_icu_init(icu, msg); + /* One off initialization per domain */ + mvebu_icu_init(icu, msi_data, msg); /* Configure the ICU with irq number & type */ icu_int = msg->data | ICU_INT_ENABLE; if (icu_irqd->type & IRQ_TYPE_EDGE_RISING) @@ -105,7 +135,8 @@ static int mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { - struct mvebu_icu *icu = platform_msi_get_host_data(d); + struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d); + struct mvebu_icu *icu = msi_data->icu; unsigned int param_count = icu->is_legacy ? 3 : 2; /* Check the count of the parameters in dt */ @@ -117,7 +148,7 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, if (icu->is_legacy) { *hwirq = fwspec->param[1]; - *type = fwspec->param[2]; + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; if (fwspec->param[0] != ICU_GRP_NSR) { dev_err(icu->dev, "wrong ICU group type %x\n", fwspec->param[0]); @@ -125,7 +156,7 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, } } else { *hwirq = fwspec->param[0]; - *type = fwspec->param[1]; + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; } if (*hwirq >= ICU_MAX_IRQS) { @@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, return -EINVAL; } - /* Mask the type to prevent wrong DT configuration */ - *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; + /* + * The ICU receives level-interrupts. MSI SEI are + * edge-interrupts while MSI NSR are level-interrupts. Update the type + * accordingly for the parent irqchip. + */ + if (msi_data->subset_data->icu_group == ICU_GRP_SEI) + *type = IRQ_TYPE_EDGE_RISING; return 0; } +static int mvebu_icu_msi_bitmap_region_alloc(struct mvebu_icu *icu, int hwirq) +{ + int ret; + + mutex_lock(&icu->msi_lock); + ret = bitmap_allocate_region(icu->msi_bitmap, hwirq, 0); + mutex_unlock(&icu->msi_lock); + + return ret; +} + +static void mvebu_icu_msi_bitmap_region_release(struct mvebu_icu *icu, + int hwirq) +{ + mutex_lock(&icu->msi_lock); + bitmap_release_region(icu->msi_bitmap, hwirq, 0); + mutex_unlock(&icu->msi_lock); +} + static int mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs, void *args) @@ -146,7 +201,9 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, int err; unsigned long hwirq; struct irq_fwspec *fwspec = args; - struct mvebu_icu *icu = platform_msi_get_host_data(domain); + struct mvebu_icu_msi_data *msi_data = + platform_msi_get_host_data(domain); + struct mvebu_icu *icu = msi_data->icu; struct mvebu_icu_irq_data *icu_irqd; icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL); @@ -160,16 +217,20 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, goto free_irqd; } + err = mvebu_icu_msi_bitmap_region_alloc(icu, hwirq); + if (err) + goto free_irqd; + if (icu->is_legacy) icu_irqd->icu_group = fwspec->param[0]; else - icu_irqd->icu_group = ICU_GRP_NSR; + icu_irqd->icu_group = msi_data->subset_data->icu_group; icu_irqd->icu = icu; err = platform_msi_domain_alloc(domain, virq, nr_irqs); if (err) { dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n"); - goto free_irqd; + goto free_bitmap; } /* Make sure there is no interrupt left pending by the firmware */ @@ -188,6 +249,8 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, free_msi: platform_msi_domain_free(domain, virq, nr_irqs); +free_bitmap: + mvebu_icu_msi_bitmap_region_release(icu, hwirq); free_irqd: kfree(icu_irqd); return err; @@ -197,12 +260,17 @@ static void mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs) { + struct mvebu_icu_msi_data *msi_data = + platform_msi_get_host_data(domain); + struct mvebu_icu *icu = msi_data->icu; struct irq_data *d = irq_get_irq_data(virq); struct mvebu_icu_irq_data *icu_irqd = d->chip_data; kfree(icu_irqd); platform_msi_domain_free(domain, virq, nr_irqs); + + mvebu_icu_msi_bitmap_region_release(icu, d->hwirq); } static const struct irq_domain_ops mvebu_icu_domain_ops = { @@ -211,28 +279,54 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = { .free = mvebu_icu_irq_domain_free, }; +static const struct mvebu_icu_subset_data mvebu_icu_nsr_subset_data = { + .icu_group = ICU_GRP_NSR, + .offset_set_ah = ICU_SETSPI_NSR_AH, + .offset_set_al = ICU_SETSPI_NSR_AL, + .offset_clr_ah = ICU_CLRSPI_NSR_AH, + .offset_clr_al = ICU_CLRSPI_NSR_AL, +}; + +static const struct mvebu_icu_subset_data mvebu_icu_sei_subset_data = { + .icu_group = ICU_GRP_SEI, + .offset_set_ah = ICU_SET_SEI_AH, + .offset_set_al = ICU_SET_SEI_AL, +}; + static const struct of_device_id mvebu_icu_subset_of_match[] = { { .compatible = "marvell,cp110-icu-nsr", + .data = &mvebu_icu_nsr_subset_data, + }, + { + .compatible = "marvell,cp110-icu-sei", + .data = &mvebu_icu_sei_subset_data, }, {}, }; static int mvebu_icu_subset_probe(struct platform_device *pdev) { + struct mvebu_icu_msi_data *msi_data; struct device_node *msi_parent_dn; struct device *dev = &pdev->dev; struct irq_domain *irq_domain; - struct mvebu_icu *icu; + + msi_data = devm_kzalloc(dev, sizeof(*msi_data), GFP_KERNEL); + if (!msi_data) + return -ENOMEM; /* * Device data being populated means we are using the legacy bindings. * Using the parent device data means we are using the new bindings. */ - if (dev_get_drvdata(dev)) - icu = dev_get_drvdata(dev); - else - icu = dev_get_drvdata(dev->parent); + if (dev_get_drvdata(dev)) { + msi_data->icu = dev_get_drvdata(dev); + msi_data->subset_data = &mvebu_icu_nsr_subset_data; + } else { + msi_data->icu = dev_get_drvdata(dev->parent); + msi_data->subset_data = of_device_get_match_data(dev); + } dev->msi_domain = of_msi_get_domain(dev, dev->of_node, DOMAIN_BUS_PLATFORM_MSI); @@ -246,7 +340,7 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev) irq_domain = platform_msi_create_device_tree_domain(dev, ICU_MAX_IRQS, mvebu_icu_write_msg, &mvebu_icu_domain_ops, - icu); + msi_data); if (!irq_domain) { dev_err(dev, "Failed to create ICU MSI domain\n"); return -ENOMEM; @@ -284,6 +378,8 @@ static int mvebu_icu_probe(struct platform_device *pdev) return PTR_ERR(icu->base); } + mutex_init(&icu->msi_lock); + icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ICU.%x", (unsigned int)res->start); @@ -308,7 +404,7 @@ static int mvebu_icu_probe(struct platform_device *pdev) #endif /* - * Clean all ICU interrupts with type SPI_NSR, required to + * Clean all ICU interrupts of type NSR and SEI, required to * avoid unpredictable SPI assignments done by firmware. */ for (i = 0 ; i < ICU_MAX_IRQS ; i++) { @@ -331,7 +427,9 @@ static int mvebu_icu_probe(struct platform_device *pdev) } static const struct of_device_id mvebu_icu_of_match[] = { - { .compatible = "marvell,cp110-icu", }, + { + .compatible = "marvell,cp110-icu", + }, {}, };
An SEI driver provides an MSI domain through which it is possible to raise SEIs. Handle the NSR probe function in a more generic way to support other type of interrupts (ie. the SEIs). Each interrupt domain is a tree domain because of maximum 207 entries. Reallocating an ICU slot is prevented by the use of an ICU-wide bitmap. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/irqchip/irq-mvebu-icu.c | 148 +++++++++++++++++++++++++++++++++------- 1 file changed, 123 insertions(+), 25 deletions(-)