Hi Marc,
Thank you for the review.
On Wed, 23 May 2018 15:23:48 +0100, Marc Zyngier <marc.zyngier@xxxxxxx>
wrote:
> On 22/05/18 10:40, Miquel Raynal wrote:
> > 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).
> >
> > For clarity we do not use tree IRQ domains for now but linear ones
> > instead, allocating the 207 ICU lines for each interrupt group.
>
> What's the rational for not using trees? Because that's effectively a
> 100% overhead...
There is none.
I had a look at how to do it.
In the ICU driver I would like to just drop the nvec parameter (number
of interrupts in the domain) when calling
platform_msi_create_device_domain().
The above function would call irq_domain_create_hierarchy() which would
create a tree domain instead of a linear one because of nvec being 0.
However, there is a check in platform_msi_alloc_priv_data() (also
called by platform_msi_create_device_domain()) that will error out if
nvec is null.
I'm not 100% sure this is safe but I don't see the point of
prohibiting nvec to be null here. So would you accept this
change?
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -203,7 +203,7 @@ platform_msi_alloc_priv_data(struct device *dev,
unsigned int nvec,
* accordingly (which would impact the max number of MSI
* capable devices).
*/
- if (!dev->msi_domain || !write_msi_msg || !nvec || nvec > MAX_DEV_MSIS)
+ if (!dev->msi_domain || !write_msi_msg || nvec > MAX_DEV_MSIS)
return ERR_PTR(-EINVAL);
if (dev->msi_domain->bus_token != DOMAIN_BUS_PLATFORM_MSI) {
>
> > Reallocating an ICU slot is prevented by the use of an ICU-wide bitmap.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > ---
[...]
> > @@ -131,7 +160,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->legacy_bindings ? 3 : 2;
> >
> > /* Check the count of the parameters in dt */
> > @@ -172,7 +202,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);
> > @@ -186,16 +218,22 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > goto free_irqd;
> > }
> >
> > + spin_lock(&icu->msi_lock);
> > + err = bitmap_allocate_region(icu->msi_bitmap, hwirq, 0);
> > + spin_unlock(&icu->msi_lock);
>
> This (and the freeing counterpart) could deserve a couple of helpers.
Sure.
>
> > + if (err < 0)
> > + goto free_irqd;
> > +
> > if (icu->legacy_bindings)
> > 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 */
[...]
> > @@ -268,9 +332,30 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +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,
> > + .offset_clr_ah = ICU_CLR_SEI_AH,
> > + .offset_clr_al = ICU_CLR_SEI_AL,
>
> I thought SEI was edge only, given what you do in mvebu_icu_init.
> Confused...
AFAIK, the ICU can produce both level and edge MSI. Currently,
when it comes to SEI, we don't use the .offset_clr_a[hl] entries
because the SEI block expects edge-MSIs, but I thought useful to fill
them anyway. I will remove them both to avoid the confusion.
Thanks,
Miquèl
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
