[v2,06/20] irqchip/mmp: do not use of_address_to_resource() to get mux regs
diff mbox series

Message ID 20190822092643.593488-7-lkundrak@v3.sk
State New
Headers show
Series
  • Initial support for Marvell MMP3 SoC
Related show

Commit Message

Lubomir Rintel Aug. 22, 2019, 9:26 a.m. UTC
The "regs" property of the "mrvl,mmp2-mux-intc" devices are silly. They
are offsets from intc's base, not addresses on the parent bus. At this
point it probably can't be fixed.

On an OLPC XO-1.75 machine, the muxes are children of the intc, not the
axi bus, and thus of_address_to_resource() won't work. We should treat
the values as mere integers as opposed to bus addresses.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>

---
Changes since v4 of "MMP platform fixes" set:
- Add a comment, as suggested by Pavel

Changes since v1:
- Fix up typoes in the comment
- Do not allow the regs property be larger than the bindings document.

 drivers/irqchip/irq-mmp.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Rob Herring March 9, 2020, 4:25 p.m. UTC | #1
On Thu, Aug 22, 2019 at 4:34 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The "regs" property of the "mrvl,mmp2-mux-intc" devices are silly. They
> are offsets from intc's base, not addresses on the parent bus. At this
> point it probably can't be fixed.

Another option is for platform code to fixup the live DT and just add
'ranges' to make this work.

> On an OLPC XO-1.75 machine, the muxes are children of the intc, not the
> axi bus, and thus of_address_to_resource() won't work. We should treat
> the values as mere integers as opposed to bus addresses.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
> ---
> Changes since v4 of "MMP platform fixes" set:
> - Add a comment, as suggested by Pavel
>
> Changes since v1:
> - Fix up typoes in the comment
> - Do not allow the regs property be larger than the bindings document.
>
>  drivers/irqchip/irq-mmp.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> index 14618dc0bd396..e41e47ab71d3b 100644
> --- a/drivers/irqchip/irq-mmp.c
> +++ b/drivers/irqchip/irq-mmp.c
> @@ -424,9 +424,9 @@ IRQCHIP_DECLARE(mmp2_intc, "mrvl,mmp2-intc", mmp2_of_init);
>  static int __init mmp2_mux_of_init(struct device_node *node,
>                                    struct device_node *parent)
>  {
> -       struct resource res;
>         int i, ret, irq, j = 0;
>         u32 nr_irqs, mfp_irq;
> +       u32 reg[4];
>
>         if (!parent)
>                 return -ENODEV;
> @@ -438,18 +438,22 @@ static int __init mmp2_mux_of_init(struct device_node *node,
>                 pr_err("Not found mrvl,intc-nr-irqs property\n");
>                 return -EINVAL;
>         }
> -       ret = of_address_to_resource(node, 0, &res);
> +
> +       /*
> +        * For historical reasons, the "regs" property of the
> +        * mrvl,mmp2-mux-intc is not a regular "regs" property containing
> +        * addresses on the parent bus, but offsets from the intc's base.
> +        * That is why we can't use of_address_to_resource() here.
> +        */
> +       ret = of_property_read_variable_u32_array(node, "reg", reg,
> +                                                 ARRAY_SIZE(reg),
> +                                                 ARRAY_SIZE(reg));
>         if (ret < 0) {
>                 pr_err("Not found reg property\n");
>                 return -EINVAL;
>         }
> -       icu_data[i].reg_status = mmp_icu_base + res.start;

Seems like it was treated as an offset already?

> -       ret = of_address_to_resource(node, 1, &res);
> -       if (ret < 0) {
> -               pr_err("Not found reg property\n");
> -               return -EINVAL;
> -       }
> -       icu_data[i].reg_mask = mmp_icu_base + res.start;
> +       icu_data[i].reg_status = mmp_icu_base + reg[0];
> +       icu_data[i].reg_mask = mmp_icu_base + reg[2];

This is a bit fragile as it's hard coding the cell sizes. Are they the
same for all the platforms? I'd be more comfortable doing that in
platform specific fixup code than here.

Rob
Rob Herring March 9, 2020, 4:28 p.m. UTC | #2
On Mon, Mar 9, 2020 at 11:25 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Aug 22, 2019 at 4:34 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
> >
> > The "regs" property of the "mrvl,mmp2-mux-intc" devices are silly. They
> > are offsets from intc's base, not addresses on the parent bus. At this
> > point it probably can't be fixed.
>
> Another option is for platform code to fixup the live DT and just add
> 'ranges' to make this work.

Nevermind my reply on this old thread. It caught my attention when
looking for the other thread and I missed the date.

Rob

Patch
diff mbox series

diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
index 14618dc0bd396..e41e47ab71d3b 100644
--- a/drivers/irqchip/irq-mmp.c
+++ b/drivers/irqchip/irq-mmp.c
@@ -424,9 +424,9 @@  IRQCHIP_DECLARE(mmp2_intc, "mrvl,mmp2-intc", mmp2_of_init);
 static int __init mmp2_mux_of_init(struct device_node *node,
 				   struct device_node *parent)
 {
-	struct resource res;
 	int i, ret, irq, j = 0;
 	u32 nr_irqs, mfp_irq;
+	u32 reg[4];
 
 	if (!parent)
 		return -ENODEV;
@@ -438,18 +438,22 @@  static int __init mmp2_mux_of_init(struct device_node *node,
 		pr_err("Not found mrvl,intc-nr-irqs property\n");
 		return -EINVAL;
 	}
-	ret = of_address_to_resource(node, 0, &res);
+
+	/*
+	 * For historical reasons, the "regs" property of the
+	 * mrvl,mmp2-mux-intc is not a regular "regs" property containing
+	 * addresses on the parent bus, but offsets from the intc's base.
+	 * That is why we can't use of_address_to_resource() here.
+	 */
+	ret = of_property_read_variable_u32_array(node, "reg", reg,
+						  ARRAY_SIZE(reg),
+						  ARRAY_SIZE(reg));
 	if (ret < 0) {
 		pr_err("Not found reg property\n");
 		return -EINVAL;
 	}
-	icu_data[i].reg_status = mmp_icu_base + res.start;
-	ret = of_address_to_resource(node, 1, &res);
-	if (ret < 0) {
-		pr_err("Not found reg property\n");
-		return -EINVAL;
-	}
-	icu_data[i].reg_mask = mmp_icu_base + res.start;
+	icu_data[i].reg_status = mmp_icu_base + reg[0];
+	icu_data[i].reg_mask = mmp_icu_base + reg[2];
 	icu_data[i].cascade_irq = irq_of_parse_and_map(node, 0);
 	if (!icu_data[i].cascade_irq)
 		return -EINVAL;