If the Renesas External IRQ Pin driver cannot find a functional clock,
it prints a warning, .e.g.
renesas_intc_irqpin fe78001c.interrupt-controller: unable to get clock
and continues, as the clock is optional, depending on the SoC type.
This warning may confuse users.
To fix this, add a flag to indicate that the clock is mandatory or
optional, and add a few more compatible entries:
- If the clock is mandatory (on R-Mobile A1 or SH-Mobile AG5), a
missing clock is now treated as a fatal error,
- If the clock is optional (on R-Car Gen1, or using the generic
"renesas,intc-irqpin" compatible value), the warning is no longer
printed.
This requires making struct intc_irqpin_irlm_config more generic by
renaming it to intc_irqpin_config, and adding a flag to indicate if IRLM
is needed.
The new clock flag is merged with the existing shared_irqs boolean into
a bitfield to save space.
Suggested-by: Magnus Damm <[email protected]>
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Tested on
- r8a7779/marzen (no functional clock),
- r8a7740/armadillo (with and without mandatory functional clock),
- sh73a0/kzm9g (with and without mandatory functional clock).
drivers/irqchip/irq-renesas-intc-irqpin.c | 45 ++++++++++++++++++++++---------
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
index 7f6cf19aa6ac00d7..713177d97c7aa0b6 100644
--- a/drivers/irqchip/irq-renesas-intc-irqpin.c
+++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
@@ -79,12 +79,15 @@ struct intc_irqpin_priv {
struct irq_chip irq_chip;
struct irq_domain *irq_domain;
struct clk *clk;
- bool shared_irqs;
+ unsigned shared_irqs:1;
+ unsigned needs_clk:1;
u8 shared_irq_mask;
};
-struct intc_irqpin_irlm_config {
+struct intc_irqpin_config {
unsigned int irlm_bit;
+ unsigned needs_irlm:1;
+ unsigned needs_clk:1;
};
static unsigned long intc_irqpin_read32(void __iomem *iomem)
@@ -359,8 +362,15 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
.xlate = irq_domain_xlate_twocell,
};
-static const struct intc_irqpin_irlm_config intc_irqpin_irlm_r8a777x = {
+static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
.irlm_bit = 23, /* ICR0.IRLM0 */
+ .needs_irlm = 1,
+ .needs_clk = 0,
+};
+
+static const struct intc_irqpin_config intc_irqpin_rmobile = {
+ .needs_irlm = 0,
+ .needs_clk = 1,
};
static const struct of_device_id intc_irqpin_dt_ids[] = {
@@ -369,12 +379,17 @@ static const struct of_device_id intc_irqpin_dt_ids[] = {
.data = &intc_irqpin_irlm_r8a777x },
{ .compatible = "renesas,intc-irqpin-r8a7779",
.data = &intc_irqpin_irlm_r8a777x },
+ { .compatible = "renesas,intc-irqpin-r8a7740",
+ .data = &intc_irqpin_rmobile },
+ { .compatible = "renesas,intc-irqpin-sh73a0",
+ .data = &intc_irqpin_rmobile },
{},
};
MODULE_DEVICE_TABLE(of, intc_irqpin_dt_ids);
static int intc_irqpin_probe(struct platform_device *pdev)
{
+ const struct intc_irqpin_config *config = NULL;
struct device *dev = &pdev->dev;
const struct of_device_id *of_id;
struct intc_irqpin_priv *p;
@@ -407,9 +422,19 @@ static int intc_irqpin_probe(struct platform_device *pdev)
p->pdev = pdev;
platform_set_drvdata(pdev, p);
+ of_id = of_match_device(intc_irqpin_dt_ids, dev);
+ if (of_id && of_id->data) {
+ config = of_id->data;
+ p->needs_clk = config->needs_clk;
+ }
+
p->clk = devm_clk_get(dev, NULL);
if (IS_ERR(p->clk)) {
- dev_warn(dev, "unable to get clock\n");
+ if (p->needs_clk) {
+ dev_err(dev, "unable to get clock\n");
+ ret = PTR_ERR(p->clk);
+ goto err0;
+ }
p->clk = NULL;
}
@@ -479,14 +504,10 @@ static int intc_irqpin_probe(struct platform_device *pdev)
}
/* configure "individual IRQ mode" where needed */
- of_id = of_match_device(intc_irqpin_dt_ids, dev);
- if (of_id && of_id->data) {
- const struct intc_irqpin_irlm_config *irlm_config = of_id->data;
-
+ if (config && config->needs_irlm) {
if (io[INTC_IRQPIN_REG_IRLM])
intc_irqpin_read_modify_write(p, INTC_IRQPIN_REG_IRLM,
- irlm_config->irlm_bit,
- 1, 1);
+ config->irlm_bit, 1, 1);
else
dev_warn(dev, "unable to select IRLM mode\n");
}
@@ -500,10 +521,10 @@ static int intc_irqpin_probe(struct platform_device *pdev)
/* scan for shared interrupt lines */
ref_irq = p->irq[0].requested_irq;
- p->shared_irqs = true;
+ p->shared_irqs = 1;
for (k = 1; k < nirqs; k++) {
if (ref_irq != p->irq[k].requested_irq) {
- p->shared_irqs = false;
+ p->shared_irqs = 0;
break;
}
}
--
1.9.1
Hey Geert,
On Tue, Nov 24, 2015 at 04:08:13PM +0100, Geert Uytterhoeven wrote:
> If the Renesas External IRQ Pin driver cannot find a functional clock,
> it prints a warning, .e.g.
>
> renesas_intc_irqpin fe78001c.interrupt-controller: unable to get clock
>
> and continues, as the clock is optional, depending on the SoC type.
> This warning may confuse users.
>
> To fix this, add a flag to indicate that the clock is mandatory or
> optional, and add a few more compatible entries:
> - If the clock is mandatory (on R-Mobile A1 or SH-Mobile AG5), a
> missing clock is now treated as a fatal error,
> - If the clock is optional (on R-Car Gen1, or using the generic
> "renesas,intc-irqpin" compatible value), the warning is no longer
> printed.
>
> This requires making struct intc_irqpin_irlm_config more generic by
> renaming it to intc_irqpin_config, and adding a flag to indicate if IRLM
> is needed.
> The new clock flag is merged with the existing shared_irqs boolean into
> a bitfield to save space.
>
> Suggested-by: Magnus Damm <[email protected]>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Tested on
> - r8a7779/marzen (no functional clock),
> - r8a7740/armadillo (with and without mandatory functional clock),
> - sh73a0/kzm9g (with and without mandatory functional clock).
>
> drivers/irqchip/irq-renesas-intc-irqpin.c | 45 ++++++++++++++++++++++---------
> 1 file changed, 33 insertions(+), 12 deletions(-)
I'll pick up this a the previous series you just sent (fyi: tglx).
> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
> index 7f6cf19aa6ac00d7..713177d97c7aa0b6 100644
> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
> @@ -79,12 +79,15 @@ struct intc_irqpin_priv {
> struct irq_chip irq_chip;
> struct irq_domain *irq_domain;
> struct clk *clk;
> - bool shared_irqs;
> + unsigned shared_irqs:1;
> + unsigned needs_clk:1;
> u8 shared_irq_mask;
> };
>
> -struct intc_irqpin_irlm_config {
> +struct intc_irqpin_config {
> unsigned int irlm_bit;
> + unsigned needs_irlm:1;
> + unsigned needs_clk:1;
> };
Why not just stick with bool? Changing shared_irqs -> has_shared_irqs?
> static unsigned long intc_irqpin_read32(void __iomem *iomem)
> @@ -359,8 +362,15 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
> .xlate = irq_domain_xlate_twocell,
> };
>
> -static const struct intc_irqpin_irlm_config intc_irqpin_irlm_r8a777x = {
> +static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
> .irlm_bit = 23, /* ICR0.IRLM0 */
> + .needs_irlm = 1,
> + .needs_clk = 0,
> +};
> +
> +static const struct intc_irqpin_config intc_irqpin_rmobile = {
> + .needs_irlm = 0,
> + .needs_clk = 1,
> };
>
> static const struct of_device_id intc_irqpin_dt_ids[] = {
> @@ -369,12 +379,17 @@ static const struct of_device_id intc_irqpin_dt_ids[] = {
> .data = &intc_irqpin_irlm_r8a777x },
> { .compatible = "renesas,intc-irqpin-r8a7779",
> .data = &intc_irqpin_irlm_r8a777x },
> + { .compatible = "renesas,intc-irqpin-r8a7740",
> + .data = &intc_irqpin_rmobile },
> + { .compatible = "renesas,intc-irqpin-sh73a0",
> + .data = &intc_irqpin_rmobile },
> {},
> };
> MODULE_DEVICE_TABLE(of, intc_irqpin_dt_ids);
Add these compatibles and the clock requirement info to the devicetree
binding?
> @@ -500,10 +521,10 @@ static int intc_irqpin_probe(struct platform_device *pdev)
>
> /* scan for shared interrupt lines */
> ref_irq = p->irq[0].requested_irq;
> - p->shared_irqs = true;
> + p->shared_irqs = 1;
> for (k = 1; k < nirqs; k++) {
> if (ref_irq != p->irq[k].requested_irq) {
> - p->shared_irqs = false;
> + p->shared_irqs = 0;
> break;
> }
> }
The bool suggestion, above, would make this easier to read imo.
thx,
Jason.
Hi Jason,
On Tue, Nov 24, 2015 at 4:19 PM, Jason Cooper <[email protected]> wrote:
> On Tue, Nov 24, 2015 at 04:08:13PM +0100, Geert Uytterhoeven wrote:
>> If the Renesas External IRQ Pin driver cannot find a functional clock,
>> it prints a warning, .e.g.
>>
>> renesas_intc_irqpin fe78001c.interrupt-controller: unable to get clock
>>
>> and continues, as the clock is optional, depending on the SoC type.
>> This warning may confuse users.
>>
>> To fix this, add a flag to indicate that the clock is mandatory or
>> optional, and add a few more compatible entries:
>> - If the clock is mandatory (on R-Mobile A1 or SH-Mobile AG5), a
>> missing clock is now treated as a fatal error,
>> - If the clock is optional (on R-Car Gen1, or using the generic
>> "renesas,intc-irqpin" compatible value), the warning is no longer
>> printed.
>>
>> This requires making struct intc_irqpin_irlm_config more generic by
>> renaming it to intc_irqpin_config, and adding a flag to indicate if IRLM
>> is needed.
>> The new clock flag is merged with the existing shared_irqs boolean into
>> a bitfield to save space.
>>
>> Suggested-by: Magnus Damm <[email protected]>
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> ---
>> Tested on
>> - r8a7779/marzen (no functional clock),
>> - r8a7740/armadillo (with and without mandatory functional clock),
>> - sh73a0/kzm9g (with and without mandatory functional clock).
>>
>> drivers/irqchip/irq-renesas-intc-irqpin.c | 45 ++++++++++++++++++++++---------
>> 1 file changed, 33 insertions(+), 12 deletions(-)
>
> I'll pick up this a the previous series you just sent (fyi: tglx).
Thanks!
>> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
>> index 7f6cf19aa6ac00d7..713177d97c7aa0b6 100644
>> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
>> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
>> @@ -79,12 +79,15 @@ struct intc_irqpin_priv {
>> struct irq_chip irq_chip;
>> struct irq_domain *irq_domain;
>> struct clk *clk;
>> - bool shared_irqs;
>> + unsigned shared_irqs:1;
>> + unsigned needs_clk:1;
>> u8 shared_irq_mask;
>> };
>>
>> -struct intc_irqpin_irlm_config {
>> +struct intc_irqpin_config {
>> unsigned int irlm_bit;
>> + unsigned needs_irlm:1;
>> + unsigned needs_clk:1;
>> };
>
> Why not just stick with bool? Changing shared_irqs -> has_shared_irqs?
That's another option. It depends on how many flags we will add in the
future...
>> static unsigned long intc_irqpin_read32(void __iomem *iomem)
>> @@ -359,8 +362,15 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
>> .xlate = irq_domain_xlate_twocell,
>> };
>>
>> -static const struct intc_irqpin_irlm_config intc_irqpin_irlm_r8a777x = {
>> +static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
>> .irlm_bit = 23, /* ICR0.IRLM0 */
>> + .needs_irlm = 1,
>> + .needs_clk = 0,
>> +};
>> +
>> +static const struct intc_irqpin_config intc_irqpin_rmobile = {
>> + .needs_irlm = 0,
>> + .needs_clk = 1,
>> };
>>
>> static const struct of_device_id intc_irqpin_dt_ids[] = {
>> @@ -369,12 +379,17 @@ static const struct of_device_id intc_irqpin_dt_ids[] = {
>> .data = &intc_irqpin_irlm_r8a777x },
>> { .compatible = "renesas,intc-irqpin-r8a7779",
>> .data = &intc_irqpin_irlm_r8a777x },
>> + { .compatible = "renesas,intc-irqpin-r8a7740",
>> + .data = &intc_irqpin_rmobile },
>> + { .compatible = "renesas,intc-irqpin-sh73a0",
>> + .data = &intc_irqpin_rmobile },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, intc_irqpin_dt_ids);
>
> Add these compatibles and the clock requirement info to the devicetree
> binding?
Already there, cfr.
Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
The clock being mandatory just wasn't enforced.
>> @@ -500,10 +521,10 @@ static int intc_irqpin_probe(struct platform_device *pdev)
>>
>> /* scan for shared interrupt lines */
>> ref_irq = p->irq[0].requested_irq;
>> - p->shared_irqs = true;
>> + p->shared_irqs = 1;
>> for (k = 1; k < nirqs; k++) {
>> if (ref_irq != p->irq[k].requested_irq) {
>> - p->shared_irqs = false;
>> + p->shared_irqs = 0;
>> break;
>> }
>> }
>
> The bool suggestion, above, would make this easier to read imo.
I can respin the patch if you prefer bool.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Tue, Nov 24, 2015 at 04:39:26PM +0100, Geert Uytterhoeven wrote:
> On Tue, Nov 24, 2015 at 4:19 PM, Jason Cooper <[email protected]> wrote:
> > On Tue, Nov 24, 2015 at 04:08:13PM +0100, Geert Uytterhoeven wrote:
> >> If the Renesas External IRQ Pin driver cannot find a functional clock,
> >> it prints a warning, .e.g.
> >>
> >> renesas_intc_irqpin fe78001c.interrupt-controller: unable to get clock
> >>
> >> and continues, as the clock is optional, depending on the SoC type.
> >> This warning may confuse users.
> >>
> >> To fix this, add a flag to indicate that the clock is mandatory or
> >> optional, and add a few more compatible entries:
> >> - If the clock is mandatory (on R-Mobile A1 or SH-Mobile AG5), a
> >> missing clock is now treated as a fatal error,
> >> - If the clock is optional (on R-Car Gen1, or using the generic
> >> "renesas,intc-irqpin" compatible value), the warning is no longer
> >> printed.
> >>
> >> This requires making struct intc_irqpin_irlm_config more generic by
> >> renaming it to intc_irqpin_config, and adding a flag to indicate if IRLM
> >> is needed.
> >> The new clock flag is merged with the existing shared_irqs boolean into
> >> a bitfield to save space.
> >>
> >> Suggested-by: Magnus Damm <[email protected]>
> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
> >> ---
> >> Tested on
> >> - r8a7779/marzen (no functional clock),
> >> - r8a7740/armadillo (with and without mandatory functional clock),
> >> - sh73a0/kzm9g (with and without mandatory functional clock).
> >>
> >> drivers/irqchip/irq-renesas-intc-irqpin.c | 45 ++++++++++++++++++++++---------
> >> 1 file changed, 33 insertions(+), 12 deletions(-)
> >
> > I'll pick up this a the previous series you just sent (fyi: tglx).
>
> Thanks!
>
> >> diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c b/drivers/irqchip/irq-renesas-intc-irqpin.c
> >> index 7f6cf19aa6ac00d7..713177d97c7aa0b6 100644
> >> --- a/drivers/irqchip/irq-renesas-intc-irqpin.c
> >> +++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
> >> @@ -79,12 +79,15 @@ struct intc_irqpin_priv {
> >> struct irq_chip irq_chip;
> >> struct irq_domain *irq_domain;
> >> struct clk *clk;
> >> - bool shared_irqs;
> >> + unsigned shared_irqs:1;
> >> + unsigned needs_clk:1;
> >> u8 shared_irq_mask;
> >> };
> >>
> >> -struct intc_irqpin_irlm_config {
> >> +struct intc_irqpin_config {
> >> unsigned int irlm_bit;
> >> + unsigned needs_irlm:1;
> >> + unsigned needs_clk:1;
> >> };
> >
> > Why not just stick with bool? Changing shared_irqs -> has_shared_irqs?
>
> That's another option. It depends on how many flags we will add in the
> future...
Nope, you're right.
> >> static unsigned long intc_irqpin_read32(void __iomem *iomem)
> >> @@ -359,8 +362,15 @@ static const struct irq_domain_ops intc_irqpin_irq_domain_ops = {
> >> .xlate = irq_domain_xlate_twocell,
> >> };
> >>
> >> -static const struct intc_irqpin_irlm_config intc_irqpin_irlm_r8a777x = {
> >> +static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
> >> .irlm_bit = 23, /* ICR0.IRLM0 */
> >> + .needs_irlm = 1,
> >> + .needs_clk = 0,
> >> +};
> >> +
> >> +static const struct intc_irqpin_config intc_irqpin_rmobile = {
> >> + .needs_irlm = 0,
> >> + .needs_clk = 1,
> >> };
> >>
> >> static const struct of_device_id intc_irqpin_dt_ids[] = {
> >> @@ -369,12 +379,17 @@ static const struct of_device_id intc_irqpin_dt_ids[] = {
> >> .data = &intc_irqpin_irlm_r8a777x },
> >> { .compatible = "renesas,intc-irqpin-r8a7779",
> >> .data = &intc_irqpin_irlm_r8a777x },
> >> + { .compatible = "renesas,intc-irqpin-r8a7740",
> >> + .data = &intc_irqpin_rmobile },
> >> + { .compatible = "renesas,intc-irqpin-sh73a0",
> >> + .data = &intc_irqpin_rmobile },
> >> {},
> >> };
> >> MODULE_DEVICE_TABLE(of, intc_irqpin_dt_ids);
> >
> > Add these compatibles and the clock requirement info to the devicetree
> > binding?
>
> Already there, cfr.
> Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
> The clock being mandatory just wasn't enforced.
Perfect.
thx,
Jason.
Hi Geert,
On Tue, Nov 24, 2015 at 04:08:13PM +0100, Geert Uytterhoeven wrote:
> If the Renesas External IRQ Pin driver cannot find a functional clock,
> it prints a warning, .e.g.
>
> renesas_intc_irqpin fe78001c.interrupt-controller: unable to get clock
>
> and continues, as the clock is optional, depending on the SoC type.
> This warning may confuse users.
>
> To fix this, add a flag to indicate that the clock is mandatory or
> optional, and add a few more compatible entries:
> - If the clock is mandatory (on R-Mobile A1 or SH-Mobile AG5), a
> missing clock is now treated as a fatal error,
> - If the clock is optional (on R-Car Gen1, or using the generic
> "renesas,intc-irqpin" compatible value), the warning is no longer
> printed.
>
> This requires making struct intc_irqpin_irlm_config more generic by
> renaming it to intc_irqpin_config, and adding a flag to indicate if IRLM
> is needed.
> The new clock flag is merged with the existing shared_irqs boolean into
> a bitfield to save space.
>
> Suggested-by: Magnus Damm <[email protected]>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Tested on
> - r8a7779/marzen (no functional clock),
> - r8a7740/armadillo (with and without mandatory functional clock),
> - sh73a0/kzm9g (with and without mandatory functional clock).
>
> drivers/irqchip/irq-renesas-intc-irqpin.c | 45 ++++++++++++++++++++++---------
> 1 file changed, 33 insertions(+), 12 deletions(-)
Applied to irqchip/core.
thx,
Jason.