diff mbox

[v3,09/17] irqchip/irq-mvebu-icu: support ICU subnodes

Message ID 20180622151432.1566-10-miquel.raynal@bootlin.com
State New, archived
Headers show

Commit Message

Miquel Raynal June 22, 2018, 3:14 p.m. UTC
The ICU can handle several type of interrupt, each of them being handled
differently on AP side. On CP side, the ICU should be able to make the
distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU
node as the parent of multiple ICU sub-nodes, each of them being an
interrupt type with a different interrupt parent. ICU interrupt 'clients'
now directly point to the right sub-node, avoiding the need for the extra
ICU_GRP_* parameter.

ICU subnodes are probed automatically with devm_platform_populate(). If
the node as no child, the probe function for NSRs will still be called
'manually' in order to preserve backward compatibility with DT using the
old binding.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

Comments

Marc Zyngier June 28, 2018, 12:45 p.m. UTC | #1
On 22/06/18 16:14, Miquel Raynal wrote:
> The ICU can handle several type of interrupt, each of them being handled
> differently on AP side. On CP side, the ICU should be able to make the
> distinction between each interrupt group by pointing to the right parent.
> 
> This is done through the introduction of new bindings, presenting the ICU
> node as the parent of multiple ICU sub-nodes, each of them being an
> interrupt type with a different interrupt parent. ICU interrupt 'clients'
> now directly point to the right sub-node, avoiding the need for the extra
> ICU_GRP_* parameter.
> 
> ICU subnodes are probed automatically with devm_platform_populate(). If
> the node as no child, the probe function for NSRs will still be called
> 'manually' in order to preserve backward compatibility with DT using the
> old binding.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> index 24d45186eb6b..f7c2ede9c222 100644
> --- a/drivers/irqchip/irq-mvebu-icu.c
> +++ b/drivers/irqchip/irq-mvebu-icu.c
> @@ -43,6 +43,7 @@ struct mvebu_icu {
>  	struct regmap *regmap;
>  	struct device *dev;
>  	atomic_t initialized;
> +	bool legacy_bindings;
>  };
>  
>  struct mvebu_icu_irq_data {
> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
>  	unsigned int type;
>  };
>  
> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
> +{
> +	struct mvebu_icu *icu;
> +
> +	/*
> +	 * Device data being populated means we should be using legacy bindings.
> +	 * Using the _parent_ device data means we should be using new bindings.
> +	 */
> +	icu = dev_get_drvdata(&pdev->dev);
> +	if (icu) {
> +		if (!icu->legacy_bindings)
> +			return ERR_PTR(-EINVAL);
> +	} else {
> +		icu = dev_get_drvdata(pdev->dev.parent);
> +		if (!icu)
> +			return ERR_PTR(-ENODEV);
> +
> +		if (icu->legacy_bindings)
> +			return ERR_PTR(-EINVAL);
> +	}

Doesn't this make legacy_bindings completely redundant? Either the
pointer is !NULL in the device, and this is using a legacy binging, or
it is stored in the parent, and this is a new binding. You could even
have a helper for that:

static bool is_legacy(struct platform_device *pdev)
{
	return !dev_get_drvdata(&pdev->dev);
}

The driver really doesn't need to be defending against itself, if
anything, and it would save you quite a bit of error handling in the
callers of this function.

> +
> +	return icu;
> +}
> +
>  static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
>  {
>  	if (atomic_cmpxchg(&icu->initialized, false, true))
> @@ -107,24 +132,28 @@ 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);
> -	unsigned int icu_group;
> +	unsigned int param_count = icu->legacy_bindings ? 3 : 2;
>  
>  	/* Check the count of the parameters in dt */
> -	if (WARN_ON(fwspec->param_count < 3)) {
> +	if (WARN_ON(fwspec->param_count != param_count)) {
>  		dev_err(icu->dev, "wrong ICU parameter count %d\n",
>  			fwspec->param_count);
>  		return -EINVAL;
>  	}
>  
> -	/* Only ICU group type is handled */
> -	icu_group = fwspec->param[0];
> -	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
> -	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
> -		dev_err(icu->dev, "wrong ICU group type %x\n", icu_group);
> -		return -EINVAL;
> +	if (icu->legacy_bindings) {
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2];
> +		if (fwspec->param[0] != ICU_GRP_NSR) {
> +			dev_err(icu->dev, "wrong ICU group type %x\n",
> +				fwspec->param[0]);
> +			return -EINVAL;
> +		}
> +	} else {
> +		*hwirq = fwspec->param[0];
> +		*type = fwspec->param[1];
>  	}
>  
> -	*hwirq = fwspec->param[1];
>  	if (*hwirq >= ICU_MAX_IRQS) {
>  		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
>  		return -EINVAL;
> @@ -157,7 +186,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  		goto free_irqd;
>  	}
>  
> -	icu_irqd->icu_group = fwspec->param[0];
> +	if (icu->legacy_bindings)
> +		icu_irqd->icu_group = fwspec->param[0];
> +	else
> +		icu_irqd->icu_group = ICU_GRP_NSR;
>  	icu_irqd->icu = icu;
>  
>  	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
> @@ -211,9 +243,9 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
>  	struct irq_domain *irq_domain;
>  	struct mvebu_icu *icu;
>  
> -	icu = dev_get_drvdata(&pdev->dev);
> -	if (!icu)
> -		return -ENODEV;
> +	icu = mvebu_icu_dev_get_drvdata(pdev);
> +	if (IS_ERR(icu))
> +		return PTR_ERR(icu);
>  
>  	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, pdev->dev.of_node,
>  						 DOMAIN_BUS_PLATFORM_MSI);
> @@ -236,6 +268,22 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct of_device_id mvebu_icu_subset_of_match[] = {
> +	{
> +		.compatible = "marvell,cp110-icu-nsr",
> +	},
> +	{},
> +};
> +
> +static struct platform_driver mvebu_icu_subset_driver = {
> +	.probe  = mvebu_icu_subset_probe,
> +	.driver = {
> +		.name = "mvebu-icu-subset",
> +		.of_match_table = mvebu_icu_subset_of_match,
> +	},
> +};
> +builtin_platform_driver(mvebu_icu_subset_driver);
> +
>  static struct regmap_config mvebu_icu_regmap_config = {
>  	.reg_bits	= 32,
>  	.val_bits	= 32,
> @@ -275,6 +323,15 @@ static int mvebu_icu_probe(struct platform_device *pdev)
>  	if (!icu->irq_chip.name)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Legacy bindings: ICU is one node with one MSI parent: force manually
> +	 *                  the probe of the NSR interrupts side.
> +	 * New bindings: ICU node has children, one per interrupt controller
> +	 *               having its own MSI parent: call platform_populate().
> +	 */
> +	if (!of_get_child_count(pdev->dev.of_node))
> +		icu->legacy_bindings = true;
> +
>  	icu->irq_chip.irq_mask = irq_chip_mask_parent;
>  	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
>  	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;
> @@ -299,7 +356,10 @@ static int mvebu_icu_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, icu);
>  
> -	return mvebu_icu_subset_probe(pdev);
> +	if (icu->legacy_bindings)
> +		return mvebu_icu_subset_probe(pdev);
> +	else
> +		return devm_of_platform_populate(&pdev->dev);
>  }
>  
>  static const struct of_device_id mvebu_icu_of_match[] = {
> 

Thanks,

	M.
Miquel Raynal June 29, 2018, 12:34 p.m. UTC | #2
Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:45:09
+0100:

> On 22/06/18 16:14, Miquel Raynal wrote:
> > The ICU can handle several type of interrupt, each of them being handled
> > differently on AP side. On CP side, the ICU should be able to make the
> > distinction between each interrupt group by pointing to the right parent.
> > 
> > This is done through the introduction of new bindings, presenting the ICU
> > node as the parent of multiple ICU sub-nodes, each of them being an
> > interrupt type with a different interrupt parent. ICU interrupt 'clients'
> > now directly point to the right sub-node, avoiding the need for the extra
> > ICU_GRP_* parameter.
> > 
> > ICU subnodes are probed automatically with devm_platform_populate(). If
> > the node as no child, the probe function for NSRs will still be called
> > 'manually' in order to preserve backward compatibility with DT using the
> > old binding.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 74 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> > index 24d45186eb6b..f7c2ede9c222 100644
> > --- a/drivers/irqchip/irq-mvebu-icu.c
> > +++ b/drivers/irqchip/irq-mvebu-icu.c
> > @@ -43,6 +43,7 @@ struct mvebu_icu {
> >  	struct regmap *regmap;
> >  	struct device *dev;
> >  	atomic_t initialized;
> > +	bool legacy_bindings;
> >  };
> >  
> >  struct mvebu_icu_irq_data {
> > @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
> >  	unsigned int type;
> >  };
> >  
> > +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
> > +{
> > +	struct mvebu_icu *icu;
> > +
> > +	/*
> > +	 * Device data being populated means we should be using legacy bindings.
> > +	 * Using the _parent_ device data means we should be using new bindings.
> > +	 */
> > +	icu = dev_get_drvdata(&pdev->dev);
> > +	if (icu) {
> > +		if (!icu->legacy_bindings)
> > +			return ERR_PTR(-EINVAL);
> > +	} else {
> > +		icu = dev_get_drvdata(pdev->dev.parent);
> > +		if (!icu)
> > +			return ERR_PTR(-ENODEV);
> > +
> > +		if (icu->legacy_bindings)
> > +			return ERR_PTR(-EINVAL);
> > +	}  
> 
> Doesn't this make legacy_bindings completely redundant? Either the
> pointer is !NULL in the device, and this is using a legacy binging, or
> it is stored in the parent, and this is a new binding. You could even
> have a helper for that:
> 
> static bool is_legacy(struct platform_device *pdev)
> {
> 	return !dev_get_drvdata(&pdev->dev);
> }
> 
> The driver really doesn't need to be defending against itself, if
> anything, and it would save you quite a bit of error handling in the
> callers of this function.

You're right, I will simplify this.

Thanks,
Miquèl
Miquel Raynal July 4, 2018, 9:09 a.m. UTC | #3
Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:45:09
+0100:

> On 22/06/18 16:14, Miquel Raynal wrote:
> > The ICU can handle several type of interrupt, each of them being handled
> > differently on AP side. On CP side, the ICU should be able to make the
> > distinction between each interrupt group by pointing to the right parent.
> > 
> > This is done through the introduction of new bindings, presenting the ICU
> > node as the parent of multiple ICU sub-nodes, each of them being an
> > interrupt type with a different interrupt parent. ICU interrupt 'clients'
> > now directly point to the right sub-node, avoiding the need for the extra
> > ICU_GRP_* parameter.
> > 
> > ICU subnodes are probed automatically with devm_platform_populate(). If
> > the node as no child, the probe function for NSRs will still be called
> > 'manually' in order to preserve backward compatibility with DT using the
> > old binding.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 74 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> > index 24d45186eb6b..f7c2ede9c222 100644
> > --- a/drivers/irqchip/irq-mvebu-icu.c
> > +++ b/drivers/irqchip/irq-mvebu-icu.c
> > @@ -43,6 +43,7 @@ struct mvebu_icu {
> >  	struct regmap *regmap;
> >  	struct device *dev;
> >  	atomic_t initialized;
> > +	bool legacy_bindings;
> >  };
> >  
> >  struct mvebu_icu_irq_data {
> > @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
> >  	unsigned int type;
> >  };
> >  
> > +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
> > +{
> > +	struct mvebu_icu *icu;
> > +
> > +	/*
> > +	 * Device data being populated means we should be using legacy bindings.
> > +	 * Using the _parent_ device data means we should be using new bindings.
> > +	 */
> > +	icu = dev_get_drvdata(&pdev->dev);
> > +	if (icu) {
> > +		if (!icu->legacy_bindings)
> > +			return ERR_PTR(-EINVAL);
> > +	} else {
> > +		icu = dev_get_drvdata(pdev->dev.parent);
> > +		if (!icu)
> > +			return ERR_PTR(-ENODEV);
> > +
> > +		if (icu->legacy_bindings)
> > +			return ERR_PTR(-EINVAL);
> > +	}  
> 
> Doesn't this make legacy_bindings completely redundant? Either the
> pointer is !NULL in the device, and this is using a legacy binging, or
> it is stored in the parent, and this is a new binding. You could even
> have a helper for that:
> 
> static bool is_legacy(struct platform_device *pdev)
> {
> 	return !dev_get_drvdata(&pdev->dev);
> }
> 
> The driver really doesn't need to be defending against itself, if
> anything, and it would save you quite a bit of error handling in the
> callers of this function.

I simplified the sanity checks but I had to keep an icu->is_legacy
boolean because the above function would not have worked, for instance,
in the *_translate() hook. As this hook does not receive a struct
device * (or platform_device) as parameter, I tried to use icu->dev
instead. This cannot work as it always points to the device having the
driver data attached.

Please tell me if the next version fits what you expect.

Thanks,
Miquèl
Marc Zyngier July 4, 2018, 12:43 p.m. UTC | #4
On 04/07/18 10:09, Miquel Raynal wrote:
> Hi Marc,
> 
> Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:45:09
> +0100:
> 
>> On 22/06/18 16:14, Miquel Raynal wrote:
>>> The ICU can handle several type of interrupt, each of them being handled
>>> differently on AP side. On CP side, the ICU should be able to make the
>>> distinction between each interrupt group by pointing to the right parent.
>>>
>>> This is done through the introduction of new bindings, presenting the ICU
>>> node as the parent of multiple ICU sub-nodes, each of them being an
>>> interrupt type with a different interrupt parent. ICU interrupt 'clients'
>>> now directly point to the right sub-node, avoiding the need for the extra
>>> ICU_GRP_* parameter.
>>>
>>> ICU subnodes are probed automatically with devm_platform_populate(). If
>>> the node as no child, the probe function for NSRs will still be called
>>> 'manually' in order to preserve backward compatibility with DT using the
>>> old binding.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>>  drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 74 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
>>> index 24d45186eb6b..f7c2ede9c222 100644
>>> --- a/drivers/irqchip/irq-mvebu-icu.c
>>> +++ b/drivers/irqchip/irq-mvebu-icu.c
>>> @@ -43,6 +43,7 @@ struct mvebu_icu {
>>>  	struct regmap *regmap;
>>>  	struct device *dev;
>>>  	atomic_t initialized;
>>> +	bool legacy_bindings;
>>>  };
>>>  
>>>  struct mvebu_icu_irq_data {
>>> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
>>>  	unsigned int type;
>>>  };
>>>  
>>> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
>>> +{
>>> +	struct mvebu_icu *icu;
>>> +
>>> +	/*
>>> +	 * Device data being populated means we should be using legacy bindings.
>>> +	 * Using the _parent_ device data means we should be using new bindings.
>>> +	 */
>>> +	icu = dev_get_drvdata(&pdev->dev);
>>> +	if (icu) {
>>> +		if (!icu->legacy_bindings)
>>> +			return ERR_PTR(-EINVAL);
>>> +	} else {
>>> +		icu = dev_get_drvdata(pdev->dev.parent);
>>> +		if (!icu)
>>> +			return ERR_PTR(-ENODEV);
>>> +
>>> +		if (icu->legacy_bindings)
>>> +			return ERR_PTR(-EINVAL);
>>> +	}  
>>
>> Doesn't this make legacy_bindings completely redundant? Either the
>> pointer is !NULL in the device, and this is using a legacy binging, or
>> it is stored in the parent, and this is a new binding. You could even
>> have a helper for that:
>>
>> static bool is_legacy(struct platform_device *pdev)
>> {
>> 	return !dev_get_drvdata(&pdev->dev);
>> }
>>
>> The driver really doesn't need to be defending against itself, if
>> anything, and it would save you quite a bit of error handling in the
>> callers of this function.
> 
> I simplified the sanity checks but I had to keep an icu->is_legacy
> boolean because the above function would not have worked, for instance,
> in the *_translate() hook. As this hook does not receive a struct
> device * (or platform_device) as parameter, I tried to use icu->dev
> instead. This cannot work as it always points to the device having the
> driver data attached.

You could still have the pdev as part of the domain host_data, right?
Isn't that just a matter of having a pointer to the pdev as part of the
icu data structure?

Thanks,

	M.
Miquel Raynal July 4, 2018, 3:16 p.m. UTC | #5
Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Wed, 4 Jul 2018 13:43:58
+0100:

> On 04/07/18 10:09, Miquel Raynal wrote:
> > Hi Marc,
> > 
> > Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:45:09
> > +0100:
> >   
> >> On 22/06/18 16:14, Miquel Raynal wrote:  
> >>> The ICU can handle several type of interrupt, each of them being handled
> >>> differently on AP side. On CP side, the ICU should be able to make the
> >>> distinction between each interrupt group by pointing to the right parent.
> >>>
> >>> This is done through the introduction of new bindings, presenting the ICU
> >>> node as the parent of multiple ICU sub-nodes, each of them being an
> >>> interrupt type with a different interrupt parent. ICU interrupt 'clients'
> >>> now directly point to the right sub-node, avoiding the need for the extra
> >>> ICU_GRP_* parameter.
> >>>
> >>> ICU subnodes are probed automatically with devm_platform_populate(). If
> >>> the node as no child, the probe function for NSRs will still be called
> >>> 'manually' in order to preserve backward compatibility with DT using the
> >>> old binding.
> >>>
> >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>> ---
> >>>  drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
> >>>  1 file changed, 74 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> >>> index 24d45186eb6b..f7c2ede9c222 100644
> >>> --- a/drivers/irqchip/irq-mvebu-icu.c
> >>> +++ b/drivers/irqchip/irq-mvebu-icu.c
> >>> @@ -43,6 +43,7 @@ struct mvebu_icu {
> >>>  	struct regmap *regmap;
> >>>  	struct device *dev;
> >>>  	atomic_t initialized;
> >>> +	bool legacy_bindings;
> >>>  };
> >>>  
> >>>  struct mvebu_icu_irq_data {
> >>> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
> >>>  	unsigned int type;
> >>>  };
> >>>  
> >>> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
> >>> +{
> >>> +	struct mvebu_icu *icu;
> >>> +
> >>> +	/*
> >>> +	 * Device data being populated means we should be using legacy bindings.
> >>> +	 * Using the _parent_ device data means we should be using new bindings.
> >>> +	 */
> >>> +	icu = dev_get_drvdata(&pdev->dev);
> >>> +	if (icu) {
> >>> +		if (!icu->legacy_bindings)
> >>> +			return ERR_PTR(-EINVAL);
> >>> +	} else {
> >>> +		icu = dev_get_drvdata(pdev->dev.parent);
> >>> +		if (!icu)
> >>> +			return ERR_PTR(-ENODEV);
> >>> +
> >>> +		if (icu->legacy_bindings)
> >>> +			return ERR_PTR(-EINVAL);
> >>> +	}    
> >>
> >> Doesn't this make legacy_bindings completely redundant? Either the
> >> pointer is !NULL in the device, and this is using a legacy binging, or
> >> it is stored in the parent, and this is a new binding. You could even
> >> have a helper for that:
> >>
> >> static bool is_legacy(struct platform_device *pdev)
> >> {
> >> 	return !dev_get_drvdata(&pdev->dev);
> >> }
> >>
> >> The driver really doesn't need to be defending against itself, if
> >> anything, and it would save you quite a bit of error handling in the
> >> callers of this function.  
> > 
> > I simplified the sanity checks but I had to keep an icu->is_legacy
> > boolean because the above function would not have worked, for instance,
> > in the *_translate() hook. As this hook does not receive a struct
> > device * (or platform_device) as parameter, I tried to use icu->dev
> > instead. This cannot work as it always points to the device having the
> > driver data attached.  
> 
> You could still have the pdev as part of the domain host_data, right?
> Isn't that just a matter of having a pointer to the pdev as part of the
> icu data structure?

There is already a 'struct device *dev' pointer in the ICU data
structure. This pointer is set to "&pdev->dev" in the main probe
function.

Legacy: dev is the unique ICU device, the driver data is attached to it.
Now: dev is the 'host' ICU device that is the parent of two other
devices, ICU NSR controller and SEI NSR controller. The driver data is
also attached to this device.

So in both situations checking dev_get_drvdata(icu->dev) would return a
valid pointer.

Does this answer your question?

Kind regards,
Miquèl
Marc Zyngier July 5, 2018, 8:19 a.m. UTC | #6
On 04/07/18 16:16, Miquel Raynal wrote:
> Hi Marc,
> 
> Marc Zyngier <marc.zyngier@arm.com> wrote on Wed, 4 Jul 2018 13:43:58
> +0100:
> 
>> On 04/07/18 10:09, Miquel Raynal wrote:
>>> Hi Marc,
>>>
>>> Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 28 Jun 2018 13:45:09
>>> +0100:
>>>   
>>>> On 22/06/18 16:14, Miquel Raynal wrote:  
>>>>> The ICU can handle several type of interrupt, each of them being handled
>>>>> differently on AP side. On CP side, the ICU should be able to make the
>>>>> distinction between each interrupt group by pointing to the right parent.
>>>>>
>>>>> This is done through the introduction of new bindings, presenting the ICU
>>>>> node as the parent of multiple ICU sub-nodes, each of them being an
>>>>> interrupt type with a different interrupt parent. ICU interrupt 'clients'
>>>>> now directly point to the right sub-node, avoiding the need for the extra
>>>>> ICU_GRP_* parameter.
>>>>>
>>>>> ICU subnodes are probed automatically with devm_platform_populate(). If
>>>>> the node as no child, the probe function for NSRs will still be called
>>>>> 'manually' in order to preserve backward compatibility with DT using the
>>>>> old binding.
>>>>>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 74 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
>>>>> index 24d45186eb6b..f7c2ede9c222 100644
>>>>> --- a/drivers/irqchip/irq-mvebu-icu.c
>>>>> +++ b/drivers/irqchip/irq-mvebu-icu.c
>>>>> @@ -43,6 +43,7 @@ struct mvebu_icu {
>>>>>  	struct regmap *regmap;
>>>>>  	struct device *dev;
>>>>>  	atomic_t initialized;
>>>>> +	bool legacy_bindings;
>>>>>  };
>>>>>  
>>>>>  struct mvebu_icu_irq_data {
>>>>> @@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
>>>>>  	unsigned int type;
>>>>>  };
>>>>>  
>>>>> +static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct mvebu_icu *icu;
>>>>> +
>>>>> +	/*
>>>>> +	 * Device data being populated means we should be using legacy bindings.
>>>>> +	 * Using the _parent_ device data means we should be using new bindings.
>>>>> +	 */
>>>>> +	icu = dev_get_drvdata(&pdev->dev);
>>>>> +	if (icu) {
>>>>> +		if (!icu->legacy_bindings)
>>>>> +			return ERR_PTR(-EINVAL);
>>>>> +	} else {
>>>>> +		icu = dev_get_drvdata(pdev->dev.parent);
>>>>> +		if (!icu)
>>>>> +			return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +		if (icu->legacy_bindings)
>>>>> +			return ERR_PTR(-EINVAL);
>>>>> +	}    
>>>>
>>>> Doesn't this make legacy_bindings completely redundant? Either the
>>>> pointer is !NULL in the device, and this is using a legacy binging, or
>>>> it is stored in the parent, and this is a new binding. You could even
>>>> have a helper for that:
>>>>
>>>> static bool is_legacy(struct platform_device *pdev)
>>>> {
>>>> 	return !dev_get_drvdata(&pdev->dev);
>>>> }
>>>>
>>>> The driver really doesn't need to be defending against itself, if
>>>> anything, and it would save you quite a bit of error handling in the
>>>> callers of this function.  
>>>
>>> I simplified the sanity checks but I had to keep an icu->is_legacy
>>> boolean because the above function would not have worked, for instance,
>>> in the *_translate() hook. As this hook does not receive a struct
>>> device * (or platform_device) as parameter, I tried to use icu->dev
>>> instead. This cannot work as it always points to the device having the
>>> driver data attached.  
>>
>> You could still have the pdev as part of the domain host_data, right?
>> Isn't that just a matter of having a pointer to the pdev as part of the
>> icu data structure?
> 
> There is already a 'struct device *dev' pointer in the ICU data
> structure. This pointer is set to "&pdev->dev" in the main probe
> function.
> 
> Legacy: dev is the unique ICU device, the driver data is attached to it.
> Now: dev is the 'host' ICU device that is the parent of two other
> devices, ICU NSR controller and SEI NSR controller. The driver data is
> also attached to this device.
> 
> So in both situations checking dev_get_drvdata(icu->dev) would return a
> valid pointer.
> 
> Does this answer your question?

I think I need to see the full picture again. Post the updated series,
and we'll see what we can do.

Thanks,

	M.
diff mbox

Patch

diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
index 24d45186eb6b..f7c2ede9c222 100644
--- a/drivers/irqchip/irq-mvebu-icu.c
+++ b/drivers/irqchip/irq-mvebu-icu.c
@@ -43,6 +43,7 @@  struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {
@@ -51,6 +52,30 @@  struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {
+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}
+
+	return icu;
+}
+
 static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 {
 	if (atomic_cmpxchg(&icu->initialized, false, true))
@@ -107,24 +132,28 @@  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);
-	unsigned int icu_group;
+	unsigned int param_count = icu->legacy_bindings ? 3 : 2;
 
 	/* Check the count of the parameters in dt */
-	if (WARN_ON(fwspec->param_count < 3)) {
+	if (WARN_ON(fwspec->param_count != param_count)) {
 		dev_err(icu->dev, "wrong ICU parameter count %d\n",
 			fwspec->param_count);
 		return -EINVAL;
 	}
 
-	/* Only ICU group type is handled */
-	icu_group = fwspec->param[0];
-	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
-	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
-		dev_err(icu->dev, "wrong ICU group type %x\n", icu_group);
-		return -EINVAL;
+	if (icu->legacy_bindings) {
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2];
+		if (fwspec->param[0] != ICU_GRP_NSR) {
+			dev_err(icu->dev, "wrong ICU group type %x\n",
+				fwspec->param[0]);
+			return -EINVAL;
+		}
+	} else {
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1];
 	}
 
-	*hwirq = fwspec->param[1];
 	if (*hwirq >= ICU_MAX_IRQS) {
 		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
 		return -EINVAL;
@@ -157,7 +186,10 @@  mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 		goto free_irqd;
 	}
 
-	icu_irqd->icu_group = fwspec->param[0];
+	if (icu->legacy_bindings)
+		icu_irqd->icu_group = fwspec->param[0];
+	else
+		icu_irqd->icu_group = ICU_GRP_NSR;
 	icu_irqd->icu = icu;
 
 	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
@@ -211,9 +243,9 @@  static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	struct irq_domain *irq_domain;
 	struct mvebu_icu *icu;
 
-	icu = dev_get_drvdata(&pdev->dev);
-	if (!icu)
-		return -ENODEV;
+	icu = mvebu_icu_dev_get_drvdata(pdev);
+	if (IS_ERR(icu))
+		return PTR_ERR(icu);
 
 	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, pdev->dev.of_node,
 						 DOMAIN_BUS_PLATFORM_MSI);
@@ -236,6 +268,22 @@  static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id mvebu_icu_subset_of_match[] = {
+	{
+		.compatible = "marvell,cp110-icu-nsr",
+	},
+	{},
+};
+
+static struct platform_driver mvebu_icu_subset_driver = {
+	.probe  = mvebu_icu_subset_probe,
+	.driver = {
+		.name = "mvebu-icu-subset",
+		.of_match_table = mvebu_icu_subset_of_match,
+	},
+};
+builtin_platform_driver(mvebu_icu_subset_driver);
+
 static struct regmap_config mvebu_icu_regmap_config = {
 	.reg_bits	= 32,
 	.val_bits	= 32,
@@ -275,6 +323,15 @@  static int mvebu_icu_probe(struct platform_device *pdev)
 	if (!icu->irq_chip.name)
 		return -ENOMEM;
 
+	/*
+	 * Legacy bindings: ICU is one node with one MSI parent: force manually
+	 *                  the probe of the NSR interrupts side.
+	 * New bindings: ICU node has children, one per interrupt controller
+	 *               having its own MSI parent: call platform_populate().
+	 */
+	if (!of_get_child_count(pdev->dev.of_node))
+		icu->legacy_bindings = true;
+
 	icu->irq_chip.irq_mask = irq_chip_mask_parent;
 	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
 	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;
@@ -299,7 +356,10 @@  static int mvebu_icu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, icu);
 
-	return mvebu_icu_subset_probe(pdev);
+	if (icu->legacy_bindings)
+		return mvebu_icu_subset_probe(pdev);
+	else
+		return devm_of_platform_populate(&pdev->dev);
 }
 
 static const struct of_device_id mvebu_icu_of_match[] = {