Add support to get number of IRQs from the respective DT node for sam9x60
and sam9x7 devices. Since only this factor differs between the two SoCs,
this patch adds support for the same. Adapt the sam9x60 dtsi
accordingly.
Signed-off-by: Varshini Rajendran <[email protected]>
---
Changes in v4:
- Changed the implementation to fetch the NIRQs from DT as per the
comment to avoid introducing a new compatible when this is the only
difference between the SoCs related to this IP.
---
arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 +
drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++---
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
index 73d570a17269..e405f68c9f54 100644
--- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
+++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
@@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 {
interrupt-controller;
reg = <0xfffff100 0x100>;
atmel,external-irqs = <31>;
+ microchip,nr-irqs = <50>;
};
dbgu: serial@fffff200 {
diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
index 145535bd7560..5d96ad8860d3 100644
--- a/drivers/irqchip/irq-atmel-aic5.c
+++ b/drivers/irqchip/irq-atmel-aic5.c
@@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node,
}
IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init);
-#define NR_SAM9X60_IRQS 50
-
static int __init sam9x60_aic5_of_init(struct device_node *node,
struct device_node *parent)
{
- return aic5_of_init(node, parent, NR_SAM9X60_IRQS);
+ int ret, nr_irqs;
+
+ ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs);
+ if (ret) {
+ pr_err("Not found microchip,nr-irqs property\n");
+ return ret;
+ }
+ return aic5_of_init(node, parent, nr_irqs);
}
IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
--
2.25.1
On 23.02.2024 19:29, Varshini Rajendran wrote:
> Add support to get number of IRQs from the respective DT node for sam9x60
> and sam9x7 devices. Since only this factor differs between the two SoCs,
> this patch adds support for the same. Adapt the sam9x60 dtsi
> accordingly.
>
> Signed-off-by: Varshini Rajendran <[email protected]>
> ---
> Changes in v4:
> - Changed the implementation to fetch the NIRQs from DT as per the
> comment to avoid introducing a new compatible when this is the only
> difference between the SoCs related to this IP.
> ---
> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 +
> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++---
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> index 73d570a17269..e405f68c9f54 100644
> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 {
> interrupt-controller;
> reg = <0xfffff100 0x100>;
> atmel,external-irqs = <31>;
> + microchip,nr-irqs = <50>;
> };
>
> dbgu: serial@fffff200 {
> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
> index 145535bd7560..5d96ad8860d3 100644
> --- a/drivers/irqchip/irq-atmel-aic5.c
> +++ b/drivers/irqchip/irq-atmel-aic5.c
> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node,
> }
> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init);
>
> -#define NR_SAM9X60_IRQS 50
> -
> static int __init sam9x60_aic5_of_init(struct device_node *node,
> struct device_node *parent)
> {
> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS);
> + int ret, nr_irqs;
> +
> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs);
> + if (ret) {
> + pr_err("Not found microchip,nr-irqs property\n");
This breaks the ABI. You should ensure old device trees are still working
with this patch.
> + return ret;
> + }
> + return aic5_of_init(node, parent, nr_irqs);
> }
> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
On 03/03/24 5:51 pm, claudiu beznea wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 23.02.2024 19:29, Varshini Rajendran wrote:
>> Add support to get number of IRQs from the respective DT node for sam9x60
>> and sam9x7 devices. Since only this factor differs between the two SoCs,
>> this patch adds support for the same. Adapt the sam9x60 dtsi
>> accordingly.
>>
>> Signed-off-by: Varshini Rajendran <[email protected]>
>> ---
>> Changes in v4:
>> - Changed the implementation to fetch the NIRQs from DT as per the
>> comment to avoid introducing a new compatible when this is the only
>> difference between the SoCs related to this IP.
>> ---
>> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 +
>> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++---
>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
>> index 73d570a17269..e405f68c9f54 100644
>> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
>> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
>> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 {
>> interrupt-controller;
>> reg = <0xfffff100 0x100>;
>> atmel,external-irqs = <31>;
>> + microchip,nr-irqs = <50>;
>> };
>>
>> dbgu: serial@fffff200 {
>> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
>> index 145535bd7560..5d96ad8860d3 100644
>> --- a/drivers/irqchip/irq-atmel-aic5.c
>> +++ b/drivers/irqchip/irq-atmel-aic5.c
>> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node,
>> }
>> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init);
>>
>> -#define NR_SAM9X60_IRQS 50
>> -
>> static int __init sam9x60_aic5_of_init(struct device_node *node,
>> struct device_node *parent)
>> {
>> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS);
>> + int ret, nr_irqs;
>> +
>> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs);
>> + if (ret) {
>> + pr_err("Not found microchip,nr-irqs property\n");
>
> This breaks the ABI. You should ensure old device trees are still working
> with this patch.
The only older device that uses this API is sam9x60 and the newly added
sam9x7. This change has been tested to be working fine in both the
devices. If you still want me to use the macros as a fallback in the
failure case I can do it. But this change was proposed to avoid adding
macros in the first place. I can remove the error check just like they
do while getting other device tree properties. Or if this is just a
concern of the old devices working with the new change, then sam9x60
works. Please let me know how to proceed.
>
>> + return ret;
>> + }
>> + return aic5_of_init(node, parent, nr_irqs);
>> }
>> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
--
Thanks and Regards,
Varshini Rajendran.
On Fri, Mar 08, 2024 at 08:50:43AM +0000, [email protected] wrote:
> On 03/03/24 5:51 pm, claudiu beznea wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 23.02.2024 19:29, Varshini Rajendran wrote:
> >> Add support to get number of IRQs from the respective DT node for sam9x60
> >> and sam9x7 devices. Since only this factor differs between the two SoCs,
> >> this patch adds support for the same. Adapt the sam9x60 dtsi
> >> accordingly.
> >>
> >> Signed-off-by: Varshini Rajendran <[email protected]>
> >> ---
> >> Changes in v4:
> >> - Changed the implementation to fetch the NIRQs from DT as per the
> >> comment to avoid introducing a new compatible when this is the only
> >> difference between the SoCs related to this IP.
> >> ---
> >> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 +
> >> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++---
Driver and binding changes should be in different patches. Having them
in the same patch is usually a red flag for ABI breakage.
> >> 2 files changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> >> index 73d570a17269..e405f68c9f54 100644
> >> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
> >> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> >> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 {
> >> interrupt-controller;
> >> reg = <0xfffff100 0x100>;
> >> atmel,external-irqs = <31>;
> >> + microchip,nr-irqs = <50>;
> >> };
> >>
> >> dbgu: serial@fffff200 {
> >> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
> >> index 145535bd7560..5d96ad8860d3 100644
> >> --- a/drivers/irqchip/irq-atmel-aic5.c
> >> +++ b/drivers/irqchip/irq-atmel-aic5.c
> >> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node,
> >> }
> >> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init);
> >>
> >> -#define NR_SAM9X60_IRQS 50
> >> -
> >> static int __init sam9x60_aic5_of_init(struct device_node *node,
> >> struct device_node *parent)
> >> {
> >> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS);
> >> + int ret, nr_irqs;
> >> +
> >> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs);
> >> + if (ret) {
> >> + pr_err("Not found microchip,nr-irqs property\n");
> >
> > This breaks the ABI. You should ensure old device trees are still working
> > with this patch.
>
> The only older device that uses this API is sam9x60 and the newly added
> sam9x7. This change has been tested to be working fine in both the
> devices.
Does it still work for a sam9x60 that does not have "microchip,nr-irqs"?
I can't see how it would, because you remove the define and return an
error. That's a pretty clear ABI breakage to me and I don't think it is
justified.
> If you still want me to use the macros as a fallback in the
> failure case I can do it. But this change was proposed to avoid adding
> macros in the first place. I can remove the error check just like they
> do while getting other device tree properties. Or if this is just a
> concern of the old devices working with the new change, then sam9x60
> works. Please let me know how to proceed.
I just noticed that this property is not documented in a binding. The
first thing you would will be asked when trying to add that is "why can
this not be determined based on the compatible", which means back to
having a define in the driver.
That said, having specific $soc_aic5_of_init() functions for each SoC
seems silly when usually only the number of interrupts changes. The
number of IRQs could be in the match data and you could use
aic5_of_init in your IRQCHIP_DECLARE directly.
> >> + return ret;
> >> + }
> >> + return aic5_of_init(node, parent, nr_irqs);
> >> }
> >> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
>
> --
> Thanks and Regards,
> Varshini Rajendran.
>
On 08.03.2024 10:50, [email protected] wrote:
> On 03/03/24 5:51 pm, claudiu beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 23.02.2024 19:29, Varshini Rajendran wrote:
>>> Add support to get number of IRQs from the respective DT node for sam9x60
>>> and sam9x7 devices. Since only this factor differs between the two SoCs,
>>> this patch adds support for the same. Adapt the sam9x60 dtsi
>>> accordingly.
>>>
>>> Signed-off-by: Varshini Rajendran <[email protected]>
>>> ---
>>> Changes in v4:
>>> - Changed the implementation to fetch the NIRQs from DT as per the
>>> comment to avoid introducing a new compatible when this is the only
>>> difference between the SoCs related to this IP.
>>> ---
>>> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 +
>>> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++---
>>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
>>> index 73d570a17269..e405f68c9f54 100644
>>> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
>>> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
>>> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 {
>>> interrupt-controller;
>>> reg = <0xfffff100 0x100>;
>>> atmel,external-irqs = <31>;
>>> + microchip,nr-irqs = <50>;
>>> };
>>>
>>> dbgu: serial@fffff200 {
>>> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
>>> index 145535bd7560..5d96ad8860d3 100644
>>> --- a/drivers/irqchip/irq-atmel-aic5.c
>>> +++ b/drivers/irqchip/irq-atmel-aic5.c
>>> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node,
>>> }
>>> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init);
>>>
>>> -#define NR_SAM9X60_IRQS 50
>>> -
>>> static int __init sam9x60_aic5_of_init(struct device_node *node,
>>> struct device_node *parent)
>>> {
>>> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS);
>>> + int ret, nr_irqs;
>>> +
>>> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs);
>>> + if (ret) {
>>> + pr_err("Not found microchip,nr-irqs property\n");
>>
>> This breaks the ABI. You should ensure old device trees are still working
>> with this patch.
>
> The only older device that uses this API is sam9x60 and the newly added
> sam9x7. This change has been tested to be working fine in both the
> devices.
As Conor explained, the code in this patch should work with device trees
from previous kernel releases (thus not having microchip,nr-irqs DT binding).
> If you still want me to use the macros as a fallback in the
> failure case I can do it. But this change was proposed to avoid adding
> macros in the first place. I can remove the error check just like they
> do while getting other device tree properties. Or if this is just a
> concern of the old devices working with the new change, then sam9x60
> works. Please let me know how to proceed.
>>
>>> + return ret;
>>> + }
>>> + return aic5_of_init(node, parent, nr_irqs);
>>> }
>>> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
>