diff mbox

[02/02] ARM: shmobile: marzen-reference: Remove IRLM workaround

Message ID 20141203121813.5936.17433.sendpatchset@w520 (mailing list archive)
State Deferred
Headers show

Commit Message

Magnus Damm Dec. 3, 2014, 12:18 p.m. UTC 
From: Magnus Damm <damm+renesas@opensource.se>

Adjust the r8a7779 SoC DTS and the Marzen Reference
C board code to use DTS only for INTC-IRQPIN IRLM setup.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Written on top of renesas-devel-20141202-v3.18-rc7 and
 [PATCH] ARM: shmobile: r8a7779 CCF DTS update

 Has a runtime dependency on:
 [PATCH 01/02] irqchip: renesas-intc-irqpin: r8a7779 IRLM setup support

 arch/arm/boot/dts/r8a7779.dtsi                  |    5 +++--
 arch/arm/mach-shmobile/board-marzen-reference.c |    7 -------
 2 files changed, 3 insertions(+), 9 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Simon Horman Dec. 4, 2014, 7:21 a.m. UTC | #1 
Hi Magnus,

On Wed, Dec 03, 2014 at 09:18:13PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Adjust the r8a7779 SoC DTS and the Marzen Reference
> C board code to use DTS only for INTC-IRQPIN IRLM setup.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Written on top of renesas-devel-20141202-v3.18-rc7 and
>  [PATCH] ARM: shmobile: r8a7779 CCF DTS update
> 
>  Has a runtime dependency on:
>  [PATCH 01/02] irqchip: renesas-intc-irqpin: r8a7779 IRLM setup support
> 
>  arch/arm/boot/dts/r8a7779.dtsi                  |    5 +++--
>  arch/arm/mach-shmobile/board-marzen-reference.c |    7 -------
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
> +++ work/arch/arm/boot/dts/r8a7779.dtsi	2014-12-03 20:27:49.000000000 +0900
> @@ -139,7 +139,7 @@
>  		interrupt-controller;
>  	};
>  
> -	irqpin0: irqpin@fe780010 {
> +	irqpin0: irqpin@fe780000 {
>  		compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
>  		#interrupt-cells = <2>;
>  		status = "disabled";
> @@ -148,7 +148,8 @@
>  			<0xfe780010 4>,
>  			<0xfe780024 4>,
>  			<0xfe780044 4>,
> -			<0xfe780064 4>;
> +			<0xfe780064 4>,
> +			<0xfe780000 4>;

Is there any order implied by the above list?
Naïvely I would expect it to be sorted numerically.

>  		interrupts = <0 27 IRQ_TYPE_LEVEL_HIGH
>  			      0 28 IRQ_TYPE_LEVEL_HIGH
>  			      0 29 IRQ_TYPE_LEVEL_HIGH
> --- 0001/arch/arm/mach-shmobile/board-marzen-reference.c
> +++ work/arch/arm/mach-shmobile/board-marzen-reference.c	2014-12-03 20:28:37.000000000 +0900
> @@ -32,12 +32,6 @@ static void __init marzen_init_timer(voi
>  	clocksource_of_init();
>  }
>  
> -static void __init marzen_init(void)
> -{
> -	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> -	r8a7779_init_irq_extpin_dt(1); /* IRQ1 as individual interrupt */
> -}
> -
>  static const char *marzen_boards_compat_dt[] __initdata = {
>  	"renesas,marzen",
>  	"renesas,marzen-reference",
> @@ -50,7 +44,6 @@ DT_MACHINE_START(MARZEN, "marzen")
>  	.init_early	= shmobile_init_delay,
>  	.init_time	= marzen_init_timer,
>  	.init_irq	= r8a7779_init_irq_dt,
> -	.init_machine	= marzen_init,
>  	.init_late	= shmobile_init_late,
>  	.dt_compat	= marzen_boards_compat_dt,
>  MACHINE_END
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Magnus Damm Dec. 4, 2014, 7:33 a.m. UTC | #2 
Hi Simon,

On Thu, Dec 4, 2014 at 4:21 PM, Simon Horman <horms@verge.net.au> wrote:
> Hi Magnus,
>
> On Wed, Dec 03, 2014 at 09:18:13PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Adjust the r8a7779 SoC DTS and the Marzen Reference
>> C board code to use DTS only for INTC-IRQPIN IRLM setup.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Written on top of renesas-devel-20141202-v3.18-rc7 and
>>  [PATCH] ARM: shmobile: r8a7779 CCF DTS update
>>
>>  Has a runtime dependency on:
>>  [PATCH 01/02] irqchip: renesas-intc-irqpin: r8a7779 IRLM setup support
>>
>>  arch/arm/boot/dts/r8a7779.dtsi                  |    5 +++--
>>  arch/arm/mach-shmobile/board-marzen-reference.c |    7 -------
>>  2 files changed, 3 insertions(+), 9 deletions(-)
>>
>> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
>> +++ work/arch/arm/boot/dts/r8a7779.dtsi       2014-12-03 20:27:49.000000000 +0900
>> @@ -139,7 +139,7 @@
>>               interrupt-controller;
>>       };
>>
>> -     irqpin0: irqpin@fe780010 {
>> +     irqpin0: irqpin@fe780000 {
>>               compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
>>               #interrupt-cells = <2>;
>>               status = "disabled";
>> @@ -148,7 +148,8 @@
>>                       <0xfe780010 4>,
>>                       <0xfe780024 4>,
>>                       <0xfe780044 4>,
>> -                     <0xfe780064 4>;
>> +                     <0xfe780064 4>,
>> +                     <0xfe780000 4>;
>
> Is there any order implied by the above list?
> Naïvely I would expect it to be sorted numerically.

Yes, the driver assumes the register banks to be passed in a certain
order. In the case of r8a7779 we add one more register bank at the end
for IRLM setup. Register detail (base address, access size, order and
bitfield width) varies with SoC version. So the IRLM register will be
at different addresses depending on SoC, but the driver wants it at
the end of the list.

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman Dec. 4, 2014, 7:52 a.m. UTC | #3 
On Thu, Dec 04, 2014 at 04:33:25PM +0900, Magnus Damm wrote:
> Hi Simon,
> 
> On Thu, Dec 4, 2014 at 4:21 PM, Simon Horman <horms@verge.net.au> wrote:
> > Hi Magnus,
> >
> > On Wed, Dec 03, 2014 at 09:18:13PM +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm+renesas@opensource.se>
> >>
> >> Adjust the r8a7779 SoC DTS and the Marzen Reference
> >> C board code to use DTS only for INTC-IRQPIN IRLM setup.
> >>
> >> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >> ---
> >>
> >>  Written on top of renesas-devel-20141202-v3.18-rc7 and
> >>  [PATCH] ARM: shmobile: r8a7779 CCF DTS update
> >>
> >>  Has a runtime dependency on:
> >>  [PATCH 01/02] irqchip: renesas-intc-irqpin: r8a7779 IRLM setup support
> >>
> >>  arch/arm/boot/dts/r8a7779.dtsi                  |    5 +++--
> >>  arch/arm/mach-shmobile/board-marzen-reference.c |    7 -------
> >>  2 files changed, 3 insertions(+), 9 deletions(-)
> >>
> >> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
> >> +++ work/arch/arm/boot/dts/r8a7779.dtsi       2014-12-03 20:27:49.000000000 +0900
> >> @@ -139,7 +139,7 @@
> >>               interrupt-controller;
> >>       };
> >>
> >> -     irqpin0: irqpin@fe780010 {
> >> +     irqpin0: irqpin@fe780000 {
> >>               compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
> >>               #interrupt-cells = <2>;
> >>               status = "disabled";
> >> @@ -148,7 +148,8 @@
> >>                       <0xfe780010 4>,
> >>                       <0xfe780024 4>,
> >>                       <0xfe780044 4>,
> >> -                     <0xfe780064 4>;
> >> +                     <0xfe780064 4>,
> >> +                     <0xfe780000 4>;
> >
> > Is there any order implied by the above list?
> > Naïvely I would expect it to be sorted numerically.
> 
> Yes, the driver assumes the register banks to be passed in a certain
> order. In the case of r8a7779 we add one more register bank at the end
> for IRLM setup. Register detail (base address, access size, order and
> bitfield width) varies with SoC version. So the IRLM register will be
> at different addresses depending on SoC, but the driver wants it at
> the end of the list.

Thanks, if it is intentional then that is fine by me.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Dec. 4, 2014, 9:19 a.m. UTC | #4 
Hi Magnus,

On Thu, Dec 4, 2014 at 8:33 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
>>> +++ work/arch/arm/boot/dts/r8a7779.dtsi       2014-12-03 20:27:49.000000000 +0900
>>> @@ -139,7 +139,7 @@
>>>               interrupt-controller;
>>>       };
>>>
>>> -     irqpin0: irqpin@fe780010 {
>>> +     irqpin0: irqpin@fe780000 {
>>>               compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
>>>               #interrupt-cells = <2>;
>>>               status = "disabled";
>>> @@ -148,7 +148,8 @@
>>>                       <0xfe780010 4>,
>>>                       <0xfe780024 4>,
>>>                       <0xfe780044 4>,
>>> -                     <0xfe780064 4>;
>>> +                     <0xfe780064 4>,
>>> +                     <0xfe780000 4>;
>>
>> Is there any order implied by the above list?
>> Naïvely I would expect it to be sorted numerically.
>
> Yes, the driver assumes the register banks to be passed in a certain
> order. In the case of r8a7779 we add one more register bank at the end
> for IRLM setup. Register detail (base address, access size, order and
> bitfield width) varies with SoC version. So the IRLM register will be
> at different addresses depending on SoC, but the driver wants it at
> the end of the list.

As these are all individual registers, and there are that many, I think
it's worth adding a reg-names property to identify the registers.
Of course the driver still has to support the old anonymous order
for backwards compatibility.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Magnus Damm Dec. 4, 2014, 9:24 a.m. UTC | #5 
Hi Geert,

On Thu, Dec 4, 2014 at 6:19 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Thu, Dec 4, 2014 at 8:33 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
>>>> +++ work/arch/arm/boot/dts/r8a7779.dtsi       2014-12-03 20:27:49.000000000 +0900
>>>> @@ -139,7 +139,7 @@
>>>>               interrupt-controller;
>>>>       };
>>>>
>>>> -     irqpin0: irqpin@fe780010 {
>>>> +     irqpin0: irqpin@fe780000 {
>>>>               compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
>>>>               #interrupt-cells = <2>;
>>>>               status = "disabled";
>>>> @@ -148,7 +148,8 @@
>>>>                       <0xfe780010 4>,
>>>>                       <0xfe780024 4>,
>>>>                       <0xfe780044 4>,
>>>> -                     <0xfe780064 4>;
>>>> +                     <0xfe780064 4>,
>>>> +                     <0xfe780000 4>;
>>>
>>> Is there any order implied by the above list?
>>> Naïvely I would expect it to be sorted numerically.
>>
>> Yes, the driver assumes the register banks to be passed in a certain
>> order. In the case of r8a7779 we add one more register bank at the end
>> for IRLM setup. Register detail (base address, access size, order and
>> bitfield width) varies with SoC version. So the IRLM register will be
>> at different addresses depending on SoC, but the driver wants it at
>> the end of the list.
>
> As these are all individual registers, and there are that many, I think
> it's worth adding a reg-names property to identify the registers.
> Of course the driver still has to support the old anonymous order
> for backwards compatibility.

If we should rework things, then I propose going the other way around.
=) Basically only passing a single base address with a certain SoC
specific compat string, and based on that letting the driver
internally figure out which register is at what offset and the access
size and bitfield size. Either way we have a limited number of SoCs
and they are all old.

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Dec. 4, 2014, 9:31 a.m. UTC | #6 
Hi Magnus,

On Thu, Dec 4, 2014 at 10:24 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Thu, Dec 4, 2014 at 6:19 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Dec 4, 2014 at 8:33 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>>> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
>>>>> +++ work/arch/arm/boot/dts/r8a7779.dtsi       2014-12-03 20:27:49.000000000 +0900
>>>>> @@ -139,7 +139,7 @@
>>>>>               interrupt-controller;
>>>>>       };
>>>>>
>>>>> -     irqpin0: irqpin@fe780010 {
>>>>> +     irqpin0: irqpin@fe780000 {
>>>>>               compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
>>>>>               #interrupt-cells = <2>;
>>>>>               status = "disabled";
>>>>> @@ -148,7 +148,8 @@
>>>>>                       <0xfe780010 4>,
>>>>>                       <0xfe780024 4>,
>>>>>                       <0xfe780044 4>,
>>>>> -                     <0xfe780064 4>;
>>>>> +                     <0xfe780064 4>,
>>>>> +                     <0xfe780000 4>;
>>>>
>>>> Is there any order implied by the above list?
>>>> Naïvely I would expect it to be sorted numerically.
>>>
>>> Yes, the driver assumes the register banks to be passed in a certain
>>> order. In the case of r8a7779 we add one more register bank at the end
>>> for IRLM setup. Register detail (base address, access size, order and
>>> bitfield width) varies with SoC version. So the IRLM register will be
>>> at different addresses depending on SoC, but the driver wants it at
>>> the end of the list.
>>
>> As these are all individual registers, and there are that many, I think
>> it's worth adding a reg-names property to identify the registers.
>> Of course the driver still has to support the old anonymous order
>> for backwards compatibility.
>
> If we should rework things, then I propose going the other way around.
> =) Basically only passing a single base address with a certain SoC
> specific compat string, and based on that letting the driver
> internally figure out which register is at what offset and the access
> size and bitfield size.

That's gonna mean a complete new compatible value.
Seems like we shouldn't have added "renesas,intc-irqpin-r8a7779",
as the SoC-type was encoded in the reg properties...

> Either way we have a limited number of SoCs and they are all old.

So your current patch looks like the best option for now
(can you promise future R-Car SoCs won't have an intc-irqpin hardware
 block ;-)?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Magnus Damm Dec. 4, 2014, 10:20 a.m. UTC | #7 
Hi Geert,

On Thu, Dec 4, 2014 at 6:31 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Thu, Dec 4, 2014 at 10:24 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> On Thu, Dec 4, 2014 at 6:19 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Dec 4, 2014 at 8:33 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>>>> --- 0002/arch/arm/boot/dts/r8a7779.dtsi
>>>>>> +++ work/arch/arm/boot/dts/r8a7779.dtsi       2014-12-03 20:27:49.000000000 +0900
>>>>>> @@ -139,7 +139,7 @@
>>>>>>               interrupt-controller;
>>>>>>       };
>>>>>>
>>>>>> -     irqpin0: irqpin@fe780010 {
>>>>>> +     irqpin0: irqpin@fe780000 {
>>>>>>               compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
>>>>>>               #interrupt-cells = <2>;
>>>>>>               status = "disabled";
>>>>>> @@ -148,7 +148,8 @@
>>>>>>                       <0xfe780010 4>,
>>>>>>                       <0xfe780024 4>,
>>>>>>                       <0xfe780044 4>,
>>>>>> -                     <0xfe780064 4>;
>>>>>> +                     <0xfe780064 4>,
>>>>>> +                     <0xfe780000 4>;
>>>>>
>>>>> Is there any order implied by the above list?
>>>>> Naïvely I would expect it to be sorted numerically.
>>>>
>>>> Yes, the driver assumes the register banks to be passed in a certain
>>>> order. In the case of r8a7779 we add one more register bank at the end
>>>> for IRLM setup. Register detail (base address, access size, order and
>>>> bitfield width) varies with SoC version. So the IRLM register will be
>>>> at different addresses depending on SoC, but the driver wants it at
>>>> the end of the list.
>>>
>>> As these are all individual registers, and there are that many, I think
>>> it's worth adding a reg-names property to identify the registers.
>>> Of course the driver still has to support the old anonymous order
>>> for backwards compatibility.
>>
>> If we should rework things, then I propose going the other way around.
>> =) Basically only passing a single base address with a certain SoC
>> specific compat string, and based on that letting the driver
>> internally figure out which register is at what offset and the access
>> size and bitfield size.
>
> That's gonna mean a complete new compatible value.
> Seems like we shouldn't have added "renesas,intc-irqpin-r8a7779",
> as the SoC-type was encoded in the reg properties...

Yeah, having that SoC specific compat string in the DTS does not
exactly help us that this point. We could however use a slightly
different SoC compat string or something else that is unused if we
wanted to change things around.

>> Either way we have a limited number of SoCs and they are all old.
>
> So your current patch looks like the best option for now
> (can you promise future R-Car SoCs won't have an intc-irqpin hardware
>  block ;-)?

Hehe, almost. =)

I think I can promise that R-Car hardware won't use that hardware -
the IRQC hardware block replaced INTC-IRQPIN in R-Car Gen2 and it will
most likely not make a comeback. However, other SoC product lines may
show up with old interrupt controllers. But if so we can rework things
at that point and make use of a fresh compatible string for a clean
start.

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- 0002/arch/arm/boot/dts/r8a7779.dtsi
+++ work/arch/arm/boot/dts/r8a7779.dtsi	2014-12-03 20:27:49.000000000 +0900
@@ -139,7 +139,7 @@ 
 		interrupt-controller;
 	};
 
-	irqpin0: irqpin@fe780010 {
+	irqpin0: irqpin@fe780000 {
 		compatible = "renesas,intc-irqpin-r8a7779", "renesas,intc-irqpin";
 		#interrupt-cells = <2>;
 		status = "disabled";
@@ -148,7 +148,8 @@ 
 			<0xfe780010 4>,
 			<0xfe780024 4>,
 			<0xfe780044 4>,
-			<0xfe780064 4>;
+			<0xfe780064 4>,
+			<0xfe780000 4>;
 		interrupts = <0 27 IRQ_TYPE_LEVEL_HIGH
 			      0 28 IRQ_TYPE_LEVEL_HIGH
 			      0 29 IRQ_TYPE_LEVEL_HIGH
--- 0001/arch/arm/mach-shmobile/board-marzen-reference.c
+++ work/arch/arm/mach-shmobile/board-marzen-reference.c	2014-12-03 20:28:37.000000000 +0900
@@ -32,12 +32,6 @@  static void __init marzen_init_timer(voi
 	clocksource_of_init();
 }
 
-static void __init marzen_init(void)
-{
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-	r8a7779_init_irq_extpin_dt(1); /* IRQ1 as individual interrupt */
-}
-
 static const char *marzen_boards_compat_dt[] __initdata = {
 	"renesas,marzen",
 	"renesas,marzen-reference",
@@ -50,7 +44,6 @@  DT_MACHINE_START(MARZEN, "marzen")
 	.init_early	= shmobile_init_delay,
 	.init_time	= marzen_init_timer,
 	.init_irq	= r8a7779_init_irq_dt,
-	.init_machine	= marzen_init,
 	.init_late	= shmobile_init_late,
 	.dt_compat	= marzen_boards_compat_dt,
 MACHINE_END