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@xxxxxxxxxxx>
> ---
> 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.
--
Jazz is not dead. It just smells funny...
--
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
