Re: [PATCH 3/5] ARM: Exynos: add device tree support for MCT controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester,

Thanks for your comments.

On 3 November 2012 22:20, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi Thomas,
>
>
> On 11/03/2012 03:45 PM, Thomas Abraham wrote:
>>
>> Allow the MCT controller base address and interrupts to be obtained from
>> device tree and remove unused static definitions of these. The non-dt
>> support
>> for Exynos5250 is removed but retained for Exynos4210 based platforms.
>>
>> Cc: Changhwan Youn<chaos.youn@xxxxxxxxxxx>
>> Signed-off-by: Thomas Abraham<thomas.abraham@xxxxxxxxxx>
>> ---
>>   .../bindings/timer/samsung,exynos4210-mct.txt      |   70
>> ++++++++++++++++++++
>>   arch/arm/mach-exynos/include/mach/irqs.h           |    6 --
>>   arch/arm/mach-exynos/mct.c                         |   42 ++++++++----
>>   3 files changed, 99 insertions(+), 19 deletions(-)
>>   create mode 100644
>> Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
>> b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
>> new file mode 100644
>> index 0000000..c53fd93
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
>> @@ -0,0 +1,70 @@
>> +Samsung's Multi Core Timer (MCT)
>> +
>> +The Samsung's Multi Core Timer (MCT) module includes two main blocks, the
>> +global timer and CPU local timers. The global timer is a 64-bit free
>> running
>> +up-counter and can generate 4 interrupts when the counter reaches one of
>> the
>> +four preset counter values. The CPU local timers are 32-bit free running
>> +down-counters and generates an interrupt when the counter expires. There
>> is
>
>
> s/generates/generate ?

Ok.

>
>
>> +one CPU local timer instantiated in MCT for every CPU in the system.
>> +
>> +Required properties:
>> +
>> +- compatible: should be "samsung,exynos4210-mct".
>> +- reg: base address of the mct controller and length of the address space
>> +  it occupies.
>> +- interrupts: the list of interrupts generated by the controller. The
>> following
>> +  should be the order of the interrupts specified. The local timer
>> interrupts
>> +  should be specified after the four global timer interrupts have been
>> +  specified.
>> +
>> +       0: Global Timer Interrupt 0
>> +       1: Global Timer Interrupt 1
>> +       2: Global Timer Interrupt 2
>> +       3: Global Timer Interrupt 3
>> +       4: Local Timer Interrupt 0
>> +       5: Local Timer Interrupt 1
>> +       6: ..
>> +       7: ..
>> +       i: Local Timer Interrupt n
>> +
>> +- samsung,mct-nr-local-irqs: The number of local timer interrupts
>> supported
>> +  by the MCT controller.
>> +
>> +Example 1: In this example, the system uses only the first global timer
>> +          interrupt generated by MCT and the remaining three global timer
>> +          interrupts are unused. Two local timer interrupts have been
>> +          specified.
>> +
>> +       mct@10050000 {
>> +               compatible = "samsung,exynos4210-mct";
>> +               reg =<0x10050000 0x800>;
>> +               interrupts =<0 57 0>,<0 0 0>,<0 0 0>,<0 0 0>,
>> +                       <0 42 0>,<0 48 0>;
>> +               samsung,mct-nr-local-irqs =<4>;
>
>
> Then this means the MCT supports 4 local interrupts but only 2 are
> specified here ? Doesn't the code below expect
>
>         samsung,mct-nr-local-irqs = <2>;
>
> in this case ? Or should interrupts really be
>
>
>         interrupts =<0 57 0>, <0 0 0>, <0 0 0>, <0 0 0>,
>                 <0 42 0>, <0 48 0>, <0 0 0>, <0 0 0>;
>
> ?

No, that was a typo. It should be samsung,mct-nr-local-irqs = <2>.
Thanks for spotting this.

>>
>> +       };
>> +
>> +Example 2: In this example, the MCT global and local timer interrupts are
>> +          connected to two seperate interrupt controllers. Hence, an
>> +          interrupt-map is created to map the interrupts to the
>> respective
>> +          interrupt controllers.
>> +
>> +       mct@101C0000 {
>> +               compatible = "samsung,exynos4210-mct";
>> +               reg =<0x101C0000 0x800>;
>> +               interrupt-controller;
>> +               #interrups-cells =<2>;
>> +               interrupt-parent =<&mct_map>;
>> +               interrupts =<0 0>,<1 0>,<2 0>,<3 0>,
>> +                       <4 0>,<5 0>;
>> +               samsung,mct-nr-local-irqs =<2>;
>
>
> Here the samsung,mct-nr-local-irqs' value matches what's specified in the
> interrupts property.
>
>
>> +
>> +               mct_map: mct-map {
>> +                       compatible = "samsung,mct-map";
>
>
> Do we need a compatible property in sub-nodes like this one ?
> Wouldn't it be sufficient to reference this node, for example by name ?

Yes, it is not really required. I will remove it.

>
>> +                       #interrupt-cells =<2>;
>> +                       #address-cells =<0>;
>> +                       #size-cells =<0>;
>> +                       interrupt-map =<0x0 0&combiner 23 3>,
>> +                                       <0x4 0&gic 0 120 0>,
>> +                                       <0x5 0&gic 0 121 0>;
>>
>> +               };
>> +       };
>> diff --git a/arch/arm/mach-exynos/include/mach/irqs.h
>> b/arch/arm/mach-exynos/include/mach/irqs.h
>> index 6da3115..03c9f04 100644
>> --- a/arch/arm/mach-exynos/include/mach/irqs.h
>> +++ b/arch/arm/mach-exynos/include/mach/irqs.h
>> @@ -30,8 +30,6 @@
>>
>>   /* For EXYNOS4 and EXYNOS5 */
>>
>> -#define EXYNOS_IRQ_MCT_LOCALTIMER      IRQ_PPI(12)
>> -
>>   #define EXYNOS_IRQ_EINT16_31          IRQ_SPI(32)
>>
>>   /* For EXYNOS4 SoCs */
>> @@ -320,8 +318,6 @@
>>   #define EXYNOS5_IRQ_CEC                       IRQ_SPI(114)
>>   #define EXYNOS5_IRQ_SATA              IRQ_SPI(115)
>>
>> -#define EXYNOS5_IRQ_MCT_L0             IRQ_SPI(120)
>> -#define EXYNOS5_IRQ_MCT_L1             IRQ_SPI(121)
>>   #define EXYNOS5_IRQ_MMC44             IRQ_SPI(123)
>>   #define EXYNOS5_IRQ_MDMA1             IRQ_SPI(124)
>>   #define EXYNOS5_IRQ_FIMC_LITE0                IRQ_SPI(125)
>> @@ -411,8 +407,6 @@
>>   #define EXYNOS5_IRQ_PMU_CPU1          COMBINER_IRQ(22, 4)
>>
>>   #define EXYNOS5_IRQ_EINT0             COMBINER_IRQ(23, 0)
>> -#define EXYNOS5_IRQ_MCT_G0             COMBINER_IRQ(23, 3)
>> -#define EXYNOS5_IRQ_MCT_G1             COMBINER_IRQ(23, 4)
>>
>>   #define EXYNOS5_IRQ_EINT1             COMBINER_IRQ(24, 0)
>>   #define EXYNOS5_IRQ_SYSMMU_LITE1_0    COMBINER_IRQ(24, 1)
>> diff --git a/arch/arm/mach-exynos/mct.c b/arch/arm/mach-exynos/mct.c
>> index d65d0c7..f7792b8 100644
>> --- a/arch/arm/mach-exynos/mct.c
>> +++ b/arch/arm/mach-exynos/mct.c
>> @@ -19,6 +19,9 @@
>>   #include<linux/platform_device.h>
>>   #include<linux/delay.h>
>>   #include<linux/percpu.h>
>> +#include<linux/of.h>
>> +#include<linux/of_irq.h>
>> +#include<linux/of_address.h>
>>
>>   #include<asm/hardware/gic.h>
>>   #include<asm/localtimer.h>
>> @@ -483,14 +486,16 @@ static struct local_timer_ops exynos4_mct_tick_ops
>> __cpuinitdata = {
>>   };
>>   #endif /* CONFIG_LOCAL_TIMERS */
>>
>> -static void __init exynos4_timer_resources(void)
>> +static void __init exynos4_timer_resources(struct device_node *np)
>>   {
>>         struct clk *mct_clk;
>>         mct_clk = clk_get(NULL, "xtal");
>>
>>         clk_rate = clk_get_rate(mct_clk);
>>
>> -       reg_base = S5P_VA_SYSTIMER;
>> +       reg_base = (np) ? of_iomap(np, 0) : S5P_VA_SYSTIMER;
>
>
> nit: Parentheses around np look redundant.

Ok.

>
>
>> +       if (!reg_base)
>> +               panic("%s: unable to ioremap mct address space\n",
>> __func__);
>
>
> How about adding a line like:
>
> #define pr_fmt(fmt) "%s: " fmt, __func__
>
> on top of this file and dropping "%s: " and __func__ in those panic() calls
> ? It would make the logs more consistent across whole file.

Ok.

>
>
>>   #ifdef CONFIG_LOCAL_TIMERS
>>         if (mct_int_type == MCT_INT_PPI) {
>> @@ -509,23 +514,34 @@ static void __init exynos4_timer_resources(void)
>>
>>   static void __init exynos4_timer_init(void)
>>   {
>> -       if (soc_is_exynos4210()) {
>> +       struct device_node *np;
>> +       u32 nr_irqs, i;
>> +
>> +       np = of_find_compatible_node(NULL, NULL,
>> "samsung,exynos4210-mct");
>> +       if (np) {
>> +               if (of_machine_is_compatible("samsung,exynos4210") ||
>> +                       of_machine_is_compatible("samsung,exynos5250"))
>> +                       mct_int_type = MCT_INT_SPI;
>> +               else
>> +                       mct_int_type = MCT_INT_PPI;
>
>
> Does it make sense to specify this mct_int_type as a property of
> the mct node ?

The MCT bindings are independent of the system integration details.
There could be a system that uses MCT controller but not GIC
controller.

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


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux