Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

[Arnd Bergmann]

On Tuesday 23 June 2015 23:19:41 Paul Osmialowski wrote:
> Some SoCs need additional actions to be performed after arch idle,
> e.g. Kinetis requires invalidation of the I/D bus cache.
> 
> Such handler could be held in provided <mach/idle.h> header file.
> 
> Signed-off-by: Paul Osmialowski <pawelo@king.net.pl>
> ---
>  arch/arm/Kconfig          | 7 +++++++
>  arch/arm/kernel/process.c | 7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 8e3a833..8ef8f8f 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -98,6 +98,13 @@ config ARM_HAS_SG_CHAIN
>  config NEED_SG_DMA_LENGTH
>  	bool
>  
> +config NEED_MACH_IDLE_H
> +	bool
> +
> +config ARM_CPU_IDLE_QUIRKS
> +	bool
> +	select NEED_MACH_IDLE_H
> +

We're not adding header files like this, please come up
with another solution. How about a cpuidle driver, or
possibly just overriding arm_pm_idle()?


	Arnd

Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

[Nicolas Pitre]

On Tue, 23 Jun 2015, Arnd Bergmann wrote:

> On Tuesday 23 June 2015 23:19:41 Paul Osmialowski wrote:
> > Some SoCs need additional actions to be performed after arch idle,
> > e.g. Kinetis requires invalidation of the I/D bus cache.
> > 
> > Such handler could be held in provided <mach/idle.h> header file.
> > 
> > Signed-off-by: Paul Osmialowski <pawelo@king.net.pl>
> > ---
> >  arch/arm/Kconfig          | 7 +++++++
> >  arch/arm/kernel/process.c | 7 +++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 8e3a833..8ef8f8f 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -98,6 +98,13 @@ config ARM_HAS_SG_CHAIN
> >  config NEED_SG_DMA_LENGTH
> >  	bool
> >  
> > +config NEED_MACH_IDLE_H
> > +	bool
> > +
> > +config ARM_CPU_IDLE_QUIRKS
> > +	bool
> > +	select NEED_MACH_IDLE_H
> > +
> 
> We're not adding header files like this, please come up
> with another solution. How about a cpuidle driver, or
> possibly just overriding arm_pm_idle()?

If the WFI instruction always requires I and D flushing, then it might 
be a better idea to provide a replacement for the corresponding 
cpu_*_do_idle function.  Plenty of examples exist for other cpu_* 
functions.


Nicolas

Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

[Paul Osmialowski]

Hi Nicolas,

Thanks for your proposal, however, I have some trouble with it.

That would look lovely:

#include <linux/linkage.h>
#include <asm/assembler.h>

/*
  *      cpu_kinetis_do_idle()
  *
  *      Idle the processor (wait for interrupt).
  *
  *      IRQs are already disabled.
  */
ENTRY(cpu_kinetis_do_idle)
         wfi
         movw    r3, #:lower16:0xe0082000
         movt    r3, #:upper16:0xe0082000
         ldr     r0, [r3, #0]
         orr     r2, r0, #0x85000000
         str     r2, [r3, #0]
         ret     lr
ENDPROC(cpu_kinetis_do_idle)

But... what about the rest of this hypothetical proc_kinetis.S file?

It would be a lot of code duplication with proc-v7m.S I think.

I could define CPU_KINETIS as such:

config CPU_KINETIS
 	bool
 	select CPU_V7M

But there's no such thing like customize_processor_functions that would be 
called after proc-v7m.S define_processor_functions specified things like 
idle routine.

Finally, while I was searching for suitable arm_pm_idle-based solution, I 
found idle.c in mach-gemini that encouraged me to write my own idle.c for 
Kinetis:

/*
  * arch/arm/mach-kinetis/idle.c
  */

#include <linux/init.h>
#include <linux/io.h>
#include <asm/system_misc.h>
#include <asm/proc-fns.h>

static void kinetis_idle(void)
{
         asm volatile ("wfi");

         /*
          * This is a dirty hack that invalidates the I/D bus cache
          * on Kinetis K70. This must be done after idle.
          */
         writel(readl(IOMEM(0xe0082000)) | 0x85000000, IOMEM(0xe0082000));
}

static int __init kinetis_idle_init(void)
{
         arm_pm_idle = kinetis_idle;
         return 0;
}

arch_initcall(kinetis_idle_init);

Et voila:

(gdb) c
Continuing.
^C
Program received signal SIGTRAP, Trace/breakpoint trap.
kinetis_idle () at ../arch/arm/mach-kinetis/idle.c:18
18		writel(readl(IOMEM(0xe0082000)) | 0x85000000, 
IOMEM(0xe0082000));
(gdb) bt
#0  kinetis_idle () at ../arch/arm/mach-kinetis/idle.c:18
#1  0x0800a91e in arch_cpu_idle () at ../arch/arm/kernel/process.c:70
#2  0x08025402 in cpuidle_idle_call (state=<value optimized out>)
     at ../kernel/sched/idle.c:157
#3  cpu_idle_loop (state=<value optimized out>) at 
../kernel/sched/idle.c:253
#4  cpu_startup_entry (state=<value optimized out>) at 
../kernel/sched/idle.c:301
#5  0x081bf816 in start_kernel () at ../init/main.c:683
#6  0x08008024 in stext () at ../arch/arm/kernel/head-nommu.S:85
(gdb)

On Thu, 25 Jun 2015, Nicolas Pitre wrote:

> On Tue, 23 Jun 2015, Arnd Bergmann wrote:
>
>> On Tuesday 23 June 2015 23:19:41 Paul Osmialowski wrote:
>>> Some SoCs need additional actions to be performed after arch idle,
>>> e.g. Kinetis requires invalidation of the I/D bus cache.
>>>
>>> Such handler could be held in provided <mach/idle.h> header file.
>>>
>>> Signed-off-by: Paul Osmialowski <pawelo-rhuoMcPwk82rDJvtcaxF/A@public.gmane.org>
>>> ---
>>>  arch/arm/Kconfig          | 7 +++++++
>>>  arch/arm/kernel/process.c | 7 +++++++
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>> index 8e3a833..8ef8f8f 100644
>>> --- a/arch/arm/Kconfig
>>> +++ b/arch/arm/Kconfig
>>> @@ -98,6 +98,13 @@ config ARM_HAS_SG_CHAIN
>>>  config NEED_SG_DMA_LENGTH
>>>  	bool
>>>
>>> +config NEED_MACH_IDLE_H
>>> +	bool
>>> +
>>> +config ARM_CPU_IDLE_QUIRKS
>>> +	bool
>>> +	select NEED_MACH_IDLE_H
>>> +
>>
>> We're not adding header files like this, please come up
>> with another solution. How about a cpuidle driver, or
>> possibly just overriding arm_pm_idle()?
>
> If the WFI instruction always requires I and D flushing, then it might
> be a better idea to provide a replacement for the corresponding
> cpu_*_do_idle function.  Plenty of examples exist for other cpu_*
> functions.
>
>
> Nicolas
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

[Arnd Bergmann]

On Friday 26 June 2015 07:30:49 Paul Osmialowski wrote:
> ENDPROC(cpu_kinetis_do_idle)
> 
> But... what about the rest of this hypothetical proc_kinetis.S file?
> 
> It would be a lot of code duplication with proc-v7m.S I think.
> 

I doubt that the Kinetis has a custom version of the CPU core. Are you
sure that not all Cortex-M4 need this?

As long as the idle implementation depends only on the CPU core type,
it's easy to just define multiple .proc.info.init entries in the
same file, like we do in proc-v7.S.

	Arnd

Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

[Paul Osmialowski]

On Fri, 26 Jun 2015, Arnd Bergmann wrote:

> I doubt that the Kinetis has a custom version of the CPU core. Are you
> sure that not all Cortex-M4 need this?

I'm not sure and I don't have any other Cortex-M SoC to verify that...
Without this hack, I'm experiencing following exception:

init started: BusyBox v1.17.0 (2015-03-27 08:16:34 CET)
~ # [    1.910000]
[    1.910000] Unhandled exception: IPSR = 00000006 LR = fffffff1
[    1.910000] CPU: 0 PID: 0 Comm: swapper Not tainted 
4.1.0-next-20150623-00007-gf22ea1d-dirty #1
[    1.910000] Hardware name: Freescale Kinetis
[    1.910000] task: 082666f8 ti: 08264000 task.ti: 08264000
[    1.910000] PC is at arch_cpu_idle+0x14/0x1c
[    1.910000] LR is at arch_cpu_idle+0x13/0x1c
[    1.910000] pc : [<0800a928>]    lr : [<0800a927>]    psr: 6100000b
[    1.910000] sp : 08265f98  ip : 08264020  fp : 00000001
[    1.910000] r10: 0827433d  r9 : 00000000  r8 : 08266070
[    1.910000] r7 : 08272a88  r6 : 08266068  r5 : 00000000  r4 : 08266068
[    1.910000] r3 : 00000000  r2 : 08265fa0  r1 : 08264020  r0 : 08264000
[    1.910000] xPSR: 6100000b
[    1.910000] CPU: 0 PID: 0 Comm: swapper Not tainted 
4.1.0-next-20150623-00007-gf22ea1d-dirty #1
[    1.910000] Hardware name: Freescale Kinetis
[    1.910000] [<0800c5cd>] (unwind_backtrace) from [<0800b8b3>] 
(show_stack+0xb/0xc)
[    1.910000] [<0800b8b3>] (show_stack) from [<0800bd8b>] 
(__invalid_entry+0x4b/0x4c)
[    1.910000] [<0800bd8b>] (__invalid_entry) from [<ffffffff>] 
(0xffffffff)

Funny, it does not occur when JTAG is connected.

>
> As long as the idle implementation depends only on the CPU core type,
> it's easy to just define multiple .proc.info.init entries in the
> same file, like we do in proc-v7.S.
>

I tried it and it works. This approach does not cause code duplication, 
but (comparing to arm_pm_idle-based approach) its long. Would something 
like this be accepted ever?

Note that contrary to proc-v7, there's no __v7m_proc macro, I had to 
define it in order to avoid another code duplication:

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 8ccffee..da35b0b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -967,6 +967,7 @@ config ARCH_EFM32
  config ARCH_KINETIS
  	bool "Freescale Kinetis MCU"
  	depends on ARM_SINGLE_ARMV7M
+	select CPU_KINETIS
  	select ARMV7M_SYSTICK
  	select CLKSRC_KINETIS
  	select PINCTRL
diff --git a/arch/arm/include/asm/cputype.h 
b/arch/arm/include/asm/cputype.h
index 85e374f..79792e9 100644
--- a/arch/arm/include/asm/cputype.h
+++ b/arch/arm/include/asm/cputype.h
@@ -234,6 +234,12 @@ static inline int cpu_is_xsc3(void)
  #define	cpu_is_xscale()	1
  #endif

+#if !defined(CONFIG_CPU_KINETIS)
+#define	cpu_is_kinetis()	0
+#else
+#define	cpu_is_kinetis()	1
+#endif
+
  /*
   * Marvell's PJ4 and PJ4B cores are based on V7 version,
   * but require a specical sequence for enabling coprocessors.
diff --git a/arch/arm/include/asm/glue-proc.h 
b/arch/arm/include/asm/glue-proc.h
index 74be7c2..08583bb 100644
--- a/arch/arm/include/asm/glue-proc.h
+++ b/arch/arm/include/asm/glue-proc.h
@@ -230,6 +230,15 @@
  # endif
  #endif

+#ifdef CONFIG_CPU_KINETIS
+# ifdef CPU_NAME
+#  undef  MULTI_CPU
+#  define MULTI_CPU
+# else
+#  define CPU_NAME cpu_kinetis
+# endif
+#endif
+
  #ifdef CONFIG_CPU_PJ4B
  # ifdef CPU_NAME
  #  undef  MULTI_CPU
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 121b580..ef59407 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -356,6 +356,11 @@ config CPU_PJ4B
  	bool
  	select CPU_V7

+# Freescale Kinetis
+config CPU_KINETIS
+	bool
+	select CPU_V7M
+
  # ARMv6
  config CPU_V6
  	bool "Support ARM V6 processor" if (!ARCH_MULTIPLATFORM || 
ARCH_MULTI_V6) && (ARCH_INTEGRATOR || MACH_REALVIEW_EB || 
MACH_REALVIEW_PBX)
diff --git a/arch/arm/mm/proc-v7m.S b/arch/arm/mm/proc-v7m.S
index ded9504..671374f 100644
--- a/arch/arm/mm/proc-v7m.S
+++ b/arch/arm/mm/proc-v7m.S
@@ -73,6 +73,30 @@ ENTRY(cpu_v7m_do_resume)
  ENDPROC(cpu_v7m_do_resume)
  #endif

+#ifdef CONFIG_CPU_KINETIS
+	globl_equ	cpu_kinetis_switch_mm,	cpu_v7m_switch_mm
+	globl_equ	cpu_kinetis_proc_init,	cpu_v7m_proc_init
+	globl_equ	cpu_kinetis_proc_fin, 	cpu_v7m_proc_fin
+	globl_equ	cpu_kinetis_reset,	cpu_v7m_reset
+ENTRY(cpu_kinetis_do_idle)
+	dsb
+	wfi
+	movw	r3, #:lower16:0xe0082000
+	movt	r3, #:upper16:0xe0082000
+	ldr	r0, [r3, #0]
+	orr	r2, r0, #0x85000000
+	str	r2, [r3, #0]
+	ret	lr
+ENDPROC(cpu_kinetis_do_idle)
+	globl_equ	cpu_kinetis_dcache_clean_area, 
cpu_v7m_dcache_clean_area
+#ifdef CONFIG_ARM_CPU_SUSPEND
+	globl_equ	cpu_kinetis_do_suspend,	cpu_v7m_do_suspend
+	globl_equ	cpu_kinedis_do_resume,	cpu_v7m_do_resume
+#endif
+.globl	cpu_kinetis_suspend_size
+.equ	cpu_kinetis_suspend_size, cpu_v7m_suspend_size
+#endif
+
  	.section ".text.init", #alloc, #execinstr

  /*
@@ -140,6 +164,9 @@ __v7m_setup_stack:
  __v7m_setup_stack_top:

  	define_processor_functions v7m, dabort=nommu_early_abort, 
pabort=legacy_pabort, nommu=1
+#ifdef CONFIG_CPU_KINETIS
+	define_processor_functions kinetis, dabort=nommu_early_abort, 
pabort=legacy_pabort, nommu=1
+#endif

  	.section ".rodata"
  	string cpu_arch_name, "armv7m"
@@ -148,23 +175,35 @@ __v7m_setup_stack_top:

  	.section ".proc.info.init", #alloc

-	/*
-	 * Match any ARMv7-M processor core.
-	 */
-	.type	__v7m_proc_info, #object
-__v7m_proc_info:
-	.long	0x000f0000		@ Required ID value
-	.long	0x000f0000		@ Mask for ID
+.macro __v7m_proc name, initfunc, proc_fns = v7m_processor_functions
  	.long   0			@ 
proc_info_list.__cpu_mm_mmu_flags
  	.long   0			@ 
proc_info_list.__cpu_io_mmu_flags
-	initfn	__v7m_setup, __v7m_proc_info	@ 
proc_info_list.__cpu_flush
+	initfn	\initfunc, \name	@ proc_info_list.__cpu_flush
  	.long	cpu_arch_name
  	.long	cpu_elf_name
  	.long	HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT
  	.long	cpu_v7m_name
-	.long	v7m_processor_functions	@ proc_info_list.proc
+	.long	\proc_fns		@ proc_info_list.proc
  	.long	0			@ proc_info_list.tlb
  	.long	0			@ proc_info_list.user
  	.long	nop_cache_fns		@ proc_info_list.cache
-	.size	__v7m_proc_info, . - __v7m_proc_info
+.endm
+
+#ifdef CONFIG_CPU_KINETIS
+	.type   __v7m_kinetis_proc_info, #object
+__v7m_kinetis_proc_info:
+	.long	0x410fc240
+	.long	0xff0ffff0
+	__v7m_proc __v7m_kinetis_proc_info, __v7m_setup, proc_fns = 
kinetis_processor_functions
+	.size	__v7m_kinetis_proc_info, . - __v7m_kinetis_proc_info
+#endif

+	/*
+	 * Match any ARMv7-M processor core.
+	 */
+	.type	__v7m_proc_info, #object
+__v7m_proc_info:
+	.long	0x000f0000		@ Required ID value
+	.long	0x000f0000		@ Mask for ID
+	__v7m_proc __v7m_proc_info, __v7m_setup
+	.size	__v7m_proc_info, . - __v7m_proc_info


Works with and without JTAG connected.

(gdb) c
Continuing.
^C
Program received signal SIGTRAP, Trace/breakpoint trap.
cpu_kinetis_do_idle () at ../arch/arm/mm/proc-v7m.S:84
84		movw	r3, #:lower16:0xe0082000
(gdb) bt
#0  cpu_kinetis_do_idle () at ../arch/arm/mm/proc-v7m.S:84
#1  0x0800a4b0 in arch_cpu_idle () at ../arch/arm/kernel/process.c:72
#2  0x0802542a in cpuidle_idle_call (state=<value optimized out>)
     at ../kernel/sched/idle.c:157
#3  cpu_idle_loop (state=<value optimized out>) at 
../kernel/sched/idle.c:253
#4  cpu_startup_entry (state=<value optimized out>) at 
../kernel/sched/idle.c:301
#5  0x081bf816 in start_kernel () at ../init/main.c:683
#6  0x08008024 in stext () at ../arch/arm/kernel/head-nommu.S:85
(gdb)

Note dsb before wfi - I'm experiencing another (different) exception 
without it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/9] arm: add call to CPU idle quirks handler

[Russell King - ARM Linux]

On Fri, Jun 26, 2015 at 11:52:23PM +0200, Paul Osmialowski wrote:
> I tried it and it works. This approach does not cause code duplication, but
> (comparing to arm_pm_idle-based approach) its long. Would something like
> this be accepted ever?
> 
> Note that contrary to proc-v7, there's no __v7m_proc macro, I had to define
> it in order to avoid another code duplication:

And I don't like it - until we have word from Uwe, I'd like to keep this
simple and restricted just to the affected SoC.  Until we have it proven
that this is a generic problem, we shouldn't go treating it as such.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/9] arm: twr-k70f120m: basic support for Kinetis TWR-K70F120M

[Arnd Bergmann]

On Tuesday 23 June 2015 23:19:43 Paul Osmialowski wrote:
> @@ -1740,7 +1752,7 @@ source "mm/Kconfig"
>  config FORCE_MAX_ZONEORDER
>  	int "Maximum zone order" if ARCH_SHMOBILE_LEGACY
>  	range 11 64 if ARCH_SHMOBILE_LEGACY
> -	default "12" if SOC_AM33XX
> +	default "12" if SOC_AM33XX || ARCH_KINETIS
>  	default "9" if SA1111 || ARCH_EFM32
>  	default "11"
>  	help

Put it in the defconfig?

> +#
> +
> +obj-$(CONFIG_MACH_KINETIS)	+= kinetis_platform.o

just use

obj-y	+= kinetis.o

> diff --git a/arch/arm/mach-kinetis/include/mach/idle.h b/arch/arm/mach-kinetis/include/mach/idle.h
> new file mode 100644
> index 0000000..0aafefd
> --- /dev/null
> +++ b/arch/arm/mach-kinetis/include/mach/idle.h

No mach/*.h files please.

> +/*
> + * This Kinetis port assumes that the CPU works in little-endian mode.
> + * Switching to big-endian will require different bit offsets in peripheral
> + * devices' registers. Also, some bit groups may lay on byte edges, so issue
> + * with big-endian cannot be fixed only by defining bit offsets differently
> + * for the big-endian mode.
> + */
> +#ifndef __LITTLE_ENDIAN
> +#error This Kinetis port assumes that the CPU works in little-endian mode
> +#endif

Fix the drivers instead?

> +/*
> + * Peripheral memory map
> + */
> +#define KINETIS_AIPS0PERIPH_BASE	0x40000000
> +#define KINETIS_AIPS1PERIPH_BASE	0x40080000

Move it into DT

> +/*
> + * System Integration Module (SIM) register map
> + *
> + * This map actually covers two hardware modules:
> + *     1. SIM low-power logic, at 0x40047000
> + *     2. System integration module (SIM), at 0x40048000
> + */
> +struct kinetis_sim_regs {
> +	u32 sopt1;	/* System Options Register 1 */
> +	u32 rsv0[1024];
> +	u32 sopt2;	/* System Options Register 2 */
> +	u32 rsv1;
> +	u32 sopt4;	/* System Options Register 4 */
> +	u32 sopt5;	/* System Options Register 5 */
> +	u32 sopt6;	/* System Options Register 6 */
> +	u32 sopt7;	/* System Options Register 7 */
> +	u32 rsv2[2];
> +	u32 sdid;	/* System Device Identification Register */
> +	u32 scgc[KINETIS_SIM_CG_NUMREGS];	/* Clock Gating Regs 1...7 */
> +	u32 clkdiv1;	/* System Clock Divider Register 1 */
> +	u32 clkdiv2;	/* System Clock Divider Register 2 */
> +	u32 fcfg1;	/* Flash Configuration Register 1 */
> +	u32 fcfg2;	/* Flash Configuration Register 2 */
> +	u32 uidh;	/* Unique Identification Register High */
> +	u32 uidmh;	/* Unique Identification Register Mid-High */
> +	u32 uidml;	/* Unique Identification Register Mid Low */
> +	u32 uidl;	/* Unique Identification Register Low */
> +	u32 clkdiv3;	/* System Clock Divider Register 3 */
> +	u32 clkdiv4;	/* System Clock Divider Register 4 */
> +	u32 mcr;	/* Misc Control Register */
> +};

Move it into the driver that uses these.

> +/*
> + * SIM registers base
> + */
> +#define KINETIS_SIM_BASE		(KINETIS_AIPS0PERIPH_BASE + 0x00047000)
> +#define KINETIS_SIM_PTR(reg) \
> +	(&(((struct kinetis_sim_regs *)(KINETIS_SIM_BASE))->reg))
> +#define KINETIS_SIM_RD(reg) readl_relaxed(KINETIS_SIM_PTR(reg))
> +#define KINETIS_SIM_WR(reg, val) writel_relaxed((val), KINETIS_SIM_PTR(reg))
> +#define KINETIS_SIM_SET(reg, mask) \
> +	KINETIS_SIM_WR(reg, (KINETIS_SIM_RD(reg)) | (mask))
> +#define KINETIS_SIM_RESET(reg, mask) \
> +	KINETIS_SIM_WR(reg, (KINETIS_SIM_RD(reg)) & (~(mask)))
> +#define KINETIS_SIM_ISSET(reg, mask) \
> +	(KINETIS_SIM_RD(reg) & (mask))
> +
> +/*
> + * SIM registers
> + */
> +/*
> + * System Options Register 2
> + */
> +/* USB HS clock source select */
> +#define KINETIS_SIM_SOPT2_USBHSRC_BITS	2
> +#define KINETIS_SIM_SOPT2_USBHSRC_MSK	(3 << KINETIS_SIM_SOPT2_USBHSRC_BITS)
> +#define KINETIS_SIM_SOPT2_USBHSRC_PLL0	(1 << KINETIS_SIM_SOPT2_USBHSRC_BITS)
> +#define KINETIS_SIM_SOPT2_USBHSRC_PLL1	(2 << KINETIS_SIM_SOPT2_USBHSRC_BITS)
> +
> +/* USB FS clock source select */
> +#define KINETIS_SIM_SOPT2_USBFSRC_BITS	22
> +#define KINETIS_SIM_SOPT2_USBFSRC_MSK	(3 << KINETIS_SIM_SOPT2_USBFSRC_BITS)
> +#define KINETIS_SIM_SOPT2_USBFSRC_PLL0	(1 << KINETIS_SIM_SOPT2_USBFSRC_BITS)
> +#define KINETIS_SIM_SOPT2_USBFSRC_PLL1	(2 << KINETIS_SIM_SOPT2_USBFSRC_BITS)
> +#define KINETIS_SIM_SOPT2_USBF_CLKSEL	(1 << 18)
> +

remove all these here.

> +/*
> + * Map required regions.
> + * This being the no-MMU Linux, I am not mapping anything
> + * since all I/O registers are available at their physical addresses.
> + */
> +static void __init kinetis_map_io(void)
> +{
> +}

Not needed, remove.

> +/*
> + * Freescale Kinetis platform initialization
> + */
> +static void __init kinetis_init(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +}

same here.

> diff --git a/arch/arm/tools/mach-types b/arch/arm/tools/mach-types
> index 2ed1b8a..1d05516 100644
> --- a/arch/arm/tools/mach-types
> +++ b/arch/arm/tools/mach-types
> @@ -554,6 +554,7 @@ smdk4412		MACH_SMDK4412		SMDK4412		3765
>  marzen			MACH_MARZEN		MARZEN			3790
>  krome			MACH_KROME		KROME			3797
>  armadillo800eva		MACH_ARMADILLO800EVA	ARMADILLO800EVA		3863
> +kinetis			MACH_KINETIS		KINETIS			3896
>  mx53_umobo		MACH_MX53_UMOBO		MX53_UMOBO		3927
>  mt4			MACH_MT4		MT4			3981
>  u8520			MACH_U8520		U8520			3990

Unused, please remove.

	Arnd

Re: [PATCH 5/9] arm: twr-k70f120m: basic support for Kinetis TWR-K70F120M

[Russell King - ARM Linux]

On Wed, Jun 24, 2015 at 12:05:40AM +0200, Arnd Bergmann wrote:
> On Tuesday 23 June 2015 23:19:43 Paul Osmialowski wrote:
> > @@ -1740,7 +1752,7 @@ source "mm/Kconfig"
> >  config FORCE_MAX_ZONEORDER
> >  	int "Maximum zone order" if ARCH_SHMOBILE_LEGACY
> >  	range 11 64 if ARCH_SHMOBILE_LEGACY
> > -	default "12" if SOC_AM33XX
> > +	default "12" if SOC_AM33XX || ARCH_KINETIS
> >  	default "9" if SA1111 || ARCH_EFM32
> >  	default "11"
> >  	help
> 
> Put it in the defconfig?

That doesn't work - defconfigs only provide the answers to _visible_
options.  This option won't be visible.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 5/9] arm: twr-k70f120m: basic support for Kinetis TWR-K70F120M

[Paul Osmialowski]

Hi Arnd,

Thanks for all of your input. Your comments will all be considered during 
my works on the second iteration of this patchset.

On Wed, 24 Jun 2015, Arnd Bergmann wrote:

> On Tuesday 23 June 2015 23:19:43 Paul Osmialowski wrote:
>> @@ -1740,7 +1752,7 @@ source "mm/Kconfig"
>>  config FORCE_MAX_ZONEORDER
>>  	int "Maximum zone order" if ARCH_SHMOBILE_LEGACY
>>  	range 11 64 if ARCH_SHMOBILE_LEGACY
>> -	default "12" if SOC_AM33XX
>> +	default "12" if SOC_AM33XX || ARCH_KINETIS
>>  	default "9" if SA1111 || ARCH_EFM32
>>  	default "11"
>>  	help
>
> Put it in the defconfig?
>
>> +#
>> +
>> +obj-$(CONFIG_MACH_KINETIS)	+= kinetis_platform.o
>
> just use
>
> obj-y	+= kinetis.o
>
>> diff --git a/arch/arm/mach-kinetis/include/mach/idle.h b/arch/arm/mach-kinetis/include/mach/idle.h
>> new file mode 100644
>> index 0000000..0aafefd
>> --- /dev/null
>> +++ b/arch/arm/mach-kinetis/include/mach/idle.h
>
> No mach/*.h files please.
>
>> +/*
>> + * This Kinetis port assumes that the CPU works in little-endian mode.
>> + * Switching to big-endian will require different bit offsets in peripheral
>> + * devices' registers. Also, some bit groups may lay on byte edges, so issue
>> + * with big-endian cannot be fixed only by defining bit offsets differently
>> + * for the big-endian mode.
>> + */
>> +#ifndef __LITTLE_ENDIAN
>> +#error This Kinetis port assumes that the CPU works in little-endian mode
>> +#endif
>
> Fix the drivers instead?
>
>> +/*
>> + * Peripheral memory map
>> + */
>> +#define KINETIS_AIPS0PERIPH_BASE	0x40000000
>> +#define KINETIS_AIPS1PERIPH_BASE	0x40080000
>
> Move it into DT
>
>> +/*
>> + * System Integration Module (SIM) register map
>> + *
>> + * This map actually covers two hardware modules:
>> + *     1. SIM low-power logic, at 0x40047000
>> + *     2. System integration module (SIM), at 0x40048000
>> + */
>> +struct kinetis_sim_regs {
>> +	u32 sopt1;	/* System Options Register 1 */
>> +	u32 rsv0[1024];
>> +	u32 sopt2;	/* System Options Register 2 */
>> +	u32 rsv1;
>> +	u32 sopt4;	/* System Options Register 4 */
>> +	u32 sopt5;	/* System Options Register 5 */
>> +	u32 sopt6;	/* System Options Register 6 */
>> +	u32 sopt7;	/* System Options Register 7 */
>> +	u32 rsv2[2];
>> +	u32 sdid;	/* System Device Identification Register */
>> +	u32 scgc[KINETIS_SIM_CG_NUMREGS];	/* Clock Gating Regs 1...7 */
>> +	u32 clkdiv1;	/* System Clock Divider Register 1 */
>> +	u32 clkdiv2;	/* System Clock Divider Register 2 */
>> +	u32 fcfg1;	/* Flash Configuration Register 1 */
>> +	u32 fcfg2;	/* Flash Configuration Register 2 */
>> +	u32 uidh;	/* Unique Identification Register High */
>> +	u32 uidmh;	/* Unique Identification Register Mid-High */
>> +	u32 uidml;	/* Unique Identification Register Mid Low */
>> +	u32 uidl;	/* Unique Identification Register Low */
>> +	u32 clkdiv3;	/* System Clock Divider Register 3 */
>> +	u32 clkdiv4;	/* System Clock Divider Register 4 */
>> +	u32 mcr;	/* Misc Control Register */
>> +};
>
> Move it into the driver that uses these.
>
>> +/*
>> + * SIM registers base
>> + */
>> +#define KINETIS_SIM_BASE		(KINETIS_AIPS0PERIPH_BASE + 0x00047000)
>> +#define KINETIS_SIM_PTR(reg) \
>> +	(&(((struct kinetis_sim_regs *)(KINETIS_SIM_BASE))->reg))
>> +#define KINETIS_SIM_RD(reg) readl_relaxed(KINETIS_SIM_PTR(reg))
>> +#define KINETIS_SIM_WR(reg, val) writel_relaxed((val), KINETIS_SIM_PTR(reg))
>> +#define KINETIS_SIM_SET(reg, mask) \
>> +	KINETIS_SIM_WR(reg, (KINETIS_SIM_RD(reg)) | (mask))
>> +#define KINETIS_SIM_RESET(reg, mask) \
>> +	KINETIS_SIM_WR(reg, (KINETIS_SIM_RD(reg)) & (~(mask)))
>> +#define KINETIS_SIM_ISSET(reg, mask) \
>> +	(KINETIS_SIM_RD(reg) & (mask))
>> +
>> +/*
>> + * SIM registers
>> + */
>> +/*
>> + * System Options Register 2
>> + */
>> +/* USB HS clock source select */
>> +#define KINETIS_SIM_SOPT2_USBHSRC_BITS	2
>> +#define KINETIS_SIM_SOPT2_USBHSRC_MSK	(3 << KINETIS_SIM_SOPT2_USBHSRC_BITS)
>> +#define KINETIS_SIM_SOPT2_USBHSRC_PLL0	(1 << KINETIS_SIM_SOPT2_USBHSRC_BITS)
>> +#define KINETIS_SIM_SOPT2_USBHSRC_PLL1	(2 << KINETIS_SIM_SOPT2_USBHSRC_BITS)
>> +
>> +/* USB FS clock source select */
>> +#define KINETIS_SIM_SOPT2_USBFSRC_BITS	22
>> +#define KINETIS_SIM_SOPT2_USBFSRC_MSK	(3 << KINETIS_SIM_SOPT2_USBFSRC_BITS)
>> +#define KINETIS_SIM_SOPT2_USBFSRC_PLL0	(1 << KINETIS_SIM_SOPT2_USBFSRC_BITS)
>> +#define KINETIS_SIM_SOPT2_USBFSRC_PLL1	(2 << KINETIS_SIM_SOPT2_USBFSRC_BITS)
>> +#define KINETIS_SIM_SOPT2_USBF_CLKSEL	(1 << 18)
>> +
>
> remove all these here.
>
>> +/*
>> + * Map required regions.
>> + * This being the no-MMU Linux, I am not mapping anything
>> + * since all I/O registers are available at their physical addresses.
>> + */
>> +static void __init kinetis_map_io(void)
>> +{
>> +}
>
> Not needed, remove.
>
>> +/*
>> + * Freescale Kinetis platform initialization
>> + */
>> +static void __init kinetis_init(void)
>> +{
>> +	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>> +}
>
> same here.
>
>> diff --git a/arch/arm/tools/mach-types b/arch/arm/tools/mach-types
>> index 2ed1b8a..1d05516 100644
>> --- a/arch/arm/tools/mach-types
>> +++ b/arch/arm/tools/mach-types
>> @@ -554,6 +554,7 @@ smdk4412		MACH_SMDK4412		SMDK4412		3765
>>  marzen			MACH_MARZEN		MARZEN			3790
>>  krome			MACH_KROME		KROME			3797
>>  armadillo800eva		MACH_ARMADILLO800EVA	ARMADILLO800EVA		3863
>> +kinetis			MACH_KINETIS		KINETIS			3896
>>  mx53_umobo		MACH_MX53_UMOBO		MX53_UMOBO		3927
>>  mt4			MACH_MT4		MT4			3981
>>  u8520			MACH_U8520		U8520			3990
>
> Unused, please remove.
>
> 	Arnd
>

Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis SoC

[Arnd Bergmann]

On Tuesday 23 June 2015 23:19:44 Paul Osmialowski wrote:
> +#define KINETIS_CG_UART0       KINETIS_MKCG(3, 10)     /* SIM_SCGC4[10] */
> +#define KINETIS_CG_UART1       KINETIS_MKCG(3, 11)     /* SIM_SCGC4[11] */
> +#define KINETIS_CG_UART2       KINETIS_MKCG(3, 12)     /* SIM_SCGC4[12] */
> +#define KINETIS_CG_UART3       KINETIS_MKCG(3, 13)     /* SIM_SCGC4[13] */
> +#define KINETIS_CG_UART4       KINETIS_MKCG(0, 10)     /* SIM_SCGC1[10] */
> +#define KINETIS_CG_UART5       KINETIS_MKCG(0, 11)     /* SIM_SCGC1[11] */
> +/* Ports */
> +#define KINETIS_CG_PORTA       KINETIS_MKCG(4, 9)      /* SIM_SCGC5[9] */
> +#define KINETIS_CG_PORTB       KINETIS_MKCG(4, 10)     /* SIM_SCGC5[10] */
> +#define KINETIS_CG_PORTC       KINETIS_MKCG(4, 11)     /* SIM_SCGC5[11] */
> +#define KINETIS_CG_PORTD       KINETIS_MKCG(4, 12)     /* SIM_SCGC5[12] */
> +#define KINETIS_CG_PORTE       KINETIS_MKCG(4, 13)     /* SIM_SCGC5[13] */
> +#define KINETIS_CG_PORTF       KINETIS_MKCG(4, 14)     /* SIM_SCGC5[14] */
> 

Instead of using a triple indirection here, just put the tuples
in the DT directly using #clock-cells=<2>, and get rid of both this
header file and the dt-bindings/clock/kinetis-mcg.h file.

	Arnd

Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis SoC

[Paul Osmialowski]

Hi Stephen,

Thanks for the valuable input - all of those points are now on my 
checklist for the work on the second iteration of this patchset.

On Tue, 23 Jun 2015, Stephen Boyd wrote:

> On 06/23/2015 02:19 PM, Paul Osmialowski wrote:
>>
>> diff --git a/drivers/clk/clk-kinetis.c b/drivers/clk/clk-kinetis.c
>> new file mode 100644
>> index 0000000..dea1054
>> --- /dev/null
>> +++ b/drivers/clk/clk-kinetis.c
>> @@ -0,0 +1,226 @@
>> +/*
>> + * clk-kinetis.c - Clock driver for Kinetis K70 MCG
>> + *
>> + * Based on legacy pre-OF code by Alexander Potashev <aspotashev@emcraft.com>
>> + *
>> + * Copyright (C) 2015 Paul Osmialowski <pawelo@king.net.pl>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it under
>> + * the terms of the GNU General Public License version 2 as published by the
>> + * Free Software Foundation.
>> + */
>> +
>> +#include <linux/clk.h>
>
> Is this using the consumer API? Please remove this include.
>
>> +#include <linux/io.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/err.h>
>> +#include <mach/kinetis.h>
>> +#include <mach/power.h>
>
> It would be nice if we didn't need these mach includes so that this
> driver can be easily build tested.
>
>> +
>> +#include <dt-bindings/clock/kinetis-mcg.h>
> [..]
>> +}
>> +
>> +CLK_OF_DECLARE(kinetis_mcg, "fsl,kinetis-cmu", kinetis_mcg_init);
>
> A clocksource isn't the same as a clk provider. Please split this patch
> into two, one for the clk provider (drivers/clk) and one for the
> clocksource driver (drivers/clocksource).
>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 0f1c77e..1d2ecde 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -106,6 +106,11 @@ config CLKSRC_EFM32
>>  	  Support to use the timers of EFM32 SoCs as clock source and clock
>>  	  event device.
>>
>> +config CLKSRC_KINETIS
>> +	bool "Clocksource for Kinetis SoCs"
>> +	depends on OF && ARM && ARCH_KINETIS
>
> Doesn't ARCH_KINETIS imply ARM? Seems that we can drop the ARM
> dependency here.
>
>> +	select CLKSRC_OF
>>
>>
>> diff --git a/drivers/clocksource/timer-kinetis.c b/drivers/clocksource/timer-kinetis.c
>> new file mode 100644
>> index 0000000..634f365
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-kinetis.c
> [..]
>> +
>> +/*
>> + * Clock event device set mode function
>> + */
>> +static void kinetis_clockevent_tmr_set_mode(
>> +	enum clock_event_mode mode, struct clock_event_device *clk)
>
> s/clk/evt/ ?
>
>> +{
>> +	struct kinetis_clock_event_ddata *pit =
>> +		container_of(clk, struct kinetis_clock_event_ddata, evtdev);
>> +
>> +	switch (mode) {
>> +	case CLOCK_EVT_MODE_PERIODIC:
>> +		kinetis_pit_enable(pit->base, 1);
>> +		break;
>> +	case CLOCK_EVT_MODE_ONESHOT:
>> +	case CLOCK_EVT_MODE_UNUSED:
>> +	case CLOCK_EVT_MODE_SHUTDOWN:
>> +	default:
>> +		kinetis_pit_enable(pit->base, 0);
>> +	}
>> +}
>> +
>> +/*
>> + * Configure the timer to generate an interrupt in the specified amount of ticks
>> + */
>> +static int kinetis_clockevent_tmr_set_next_event(
>> +	unsigned long delta, struct clock_event_device *c)
>> +{
>> +	struct kinetis_clock_event_ddata *pit =
>> +		container_of(c, struct kinetis_clock_event_ddata, evtdev);
>> +	unsigned long flags;
>> +
>> +	raw_local_irq_save(flags);
>
> What is this protecting against?
>
>> +	kinetis_pit_init(pit->base, delta);
>> +	kinetis_pit_enable(pit->base, 1);
>> +	raw_local_irq_restore(flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct kinetis_clock_event_ddata
>> +		kinetis_clockevent_tmrs[KINETIS_PIT_CHANNELS] = {
>> +	{
>> +		.evtdev = {
>> +			.name		= "fsl,kinetis-pit-timer0",
>> +			.rating		= 200,
>> +			.features	=
>> +			    CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>> +			.set_mode	= kinetis_clockevent_tmr_set_mode,
>> +			.set_next_event	= kinetis_clockevent_tmr_set_next_event,
>> +		},
>> +	},
>> +	{
>> +		.evtdev = {
>> +			.name		= "fsl,kinetis-pit-timer1",
>> +		},
>> +	},
>> +	{
>> +		.evtdev = {
>> +			.name		= "fsl,kinetis-pit-timer2",
>> +		},
>> +	},
>> +	{
>> +		.evtdev = {
>> +			.name		= "fsl,kinetis-pit-timer3",
>> +		},
>> +	},
>> +};
>> +
>> +/*
>> + * Timer IRQ handler
>> + */
>> +static irqreturn_t kinetis_clockevent_tmr_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct kinetis_clock_event_ddata *tmr = dev_id;
>> +
>> +	KINETIS_PIT_WR(tmr->base, tflg, KINETIS_PIT_TFLG_TIF_MSK);
>> +
>> +	tmr->evtdev.event_handler(&(tmr->evtdev));
>
> Unnecessary parentheses, please remove them.
>
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/*
>> + * System timer IRQ action
>> + */
>> +static struct irqaction kinetis_clockevent_irqaction[KINETIS_PIT_CHANNELS] = {
>> +	{
>> +		.name = "Kinetis Kernel Time Tick (pit0)",
>> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
>> +		.dev_id = &kinetis_clockevent_tmrs[0],
>> +		.handler = kinetis_clockevent_tmr_irq_handler,
>> +	}, {
>> +		.name = "Kinetis Kernel Time Tick (pit1)",
>> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
>> +		.dev_id = &kinetis_clockevent_tmrs[1],
>> +		.handler = kinetis_clockevent_tmr_irq_handler,
>> +	}, {
>> +		.name = "Kinetis Kernel Time Tick (pit2)",
>> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
>> +		.dev_id = &kinetis_clockevent_tmrs[2],
>> +		.handler = kinetis_clockevent_tmr_irq_handler,
>> +	}, {
>> +		.name = "Kinetis Kernel Time Tick (pit3)",
>> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
>> +		.dev_id = &kinetis_clockevent_tmrs[3],
>> +		.handler = kinetis_clockevent_tmr_irq_handler,
>> +	},
>> +};
>
> Any reason we can't just use request_irq() instead of having a set of
> static irq actions?
>
>> +
>> +static void __init kinetis_clockevent_init(struct device_node *np)
>> +{
> [..]
>> irq;
>> +	}
>> +
>> +	chan = of_alias_get_id(np, "pit");
>> +	if ((chan < 0) || (chan >= KINETIS_PIT_CHANNELS)) {
>
> Unnecessary parentheses, please remove them.
>
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis SoC

[Thomas Gleixner]

On Tue, 23 Jun 2015, Paul Osmialowski wrote:
> +/*
> + * Clock event device set mode function
> + */
> +static void kinetis_clockevent_tmr_set_mode(
> +	enum clock_event_mode mode, struct clock_event_device *clk)
> +{
> +	struct kinetis_clock_event_ddata *pit =
> +		container_of(clk, struct kinetis_clock_event_ddata, evtdev);
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		kinetis_pit_enable(pit->base, 1);
> +		break;
> +	case CLOCK_EVT_MODE_ONESHOT:
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +	default:
> +		kinetis_pit_enable(pit->base, 0);
> +	}
> +}

Please move to the new set_state_* interfaces. set_mode() is deprecated.

> +static int kinetis_clockevent_tmr_set_next_event(
> +	unsigned long delta, struct clock_event_device *c)
> +{
> +	struct kinetis_clock_event_ddata *pit =
> +		container_of(c, struct kinetis_clock_event_ddata, evtdev);
> +	unsigned long flags;
> +
> +	raw_local_irq_save(flags);

Pointless exercise. This is called with interrupts disabled.

> +	kinetis_pit_init(pit->base, delta);
> +	kinetis_pit_enable(pit->base, 1);
> +	raw_local_irq_restore(flags);


> +static struct irqaction kinetis_clockevent_irqaction[KINETIS_PIT_CHANNELS] = {
> +	{
> +		.name = "Kinetis Kernel Time Tick (pit0)",

Please use oneword descriptive names not half sentences.

> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +		.dev_id = &kinetis_clockevent_tmrs[0],
> +		.handler = kinetis_clockevent_tmr_irq_handler,
> +	}, {
> +		.name = "Kinetis Kernel Time Tick (pit1)",
> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +		.dev_id = &kinetis_clockevent_tmrs[1],
> +		.handler = kinetis_clockevent_tmr_irq_handler,
> +	}, {
> +		.name = "Kinetis Kernel Time Tick (pit2)",
> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +		.dev_id = &kinetis_clockevent_tmrs[2],
> +		.handler = kinetis_clockevent_tmr_irq_handler,
> +	}, {
> +		.name = "Kinetis Kernel Time Tick (pit3)",
> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +		.dev_id = &kinetis_clockevent_tmrs[3],
> +		.handler = kinetis_clockevent_tmr_irq_handler,
> +	},

Aside of that. Please use standard request_irq() there is no reason to
use setup_irq here.

> +
> +	setup_irq(irq, &(kinetis_clockevent_irqaction[chan]));

  request_irq(irq, handler, flags, "name", &kinetis_clockevent_tmrs[chan]);

....

Thanks,

	tglx

Re: [PATCH 8/9] arm: twr-k70f120m: extend Freescale eDMA driver with ability to support Kinetis SoC

[Vinod Koul]

On Tue, Jun 23, 2015 at 11:19:46PM +0200, Paul Osmialowski wrote:
> Surprisingly small amount of work was required in order to extend already
> existing eDMA driver with the support for Kinetis SoC architecture.
> 
> Note that <mach/memory.h> is needed (which is denoted by
> CONFIG_NEED_MACH_MEMORY_H) as it provides macros required for proper
> operation of DMA allocation functions.
> 
> Signed-off-by: Paul Osmialowski <pawelo-rhuoMcPwk82rDJvtcaxF/A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/dma/fsl-edma.txt | 38 +++++++++-
>  arch/arm/Kconfig                                   |  4 ++
>  arch/arm/boot/dts/kinetis.dtsi                     | 34 +++++++++
>  arch/arm/mach-kinetis/include/mach/memory.h        | 61 ++++++++++++++++
>  drivers/clk/clk-kinetis.c                          | 15 ++++
>  drivers/dma/fsl-edma.c                             | 81 +++++++++++++++++++++-
>  include/dt-bindings/clock/kinetis-mcg.h            |  5 +-
having so many change into one patch is not a great idea, please breka them
up. I am looking for single/multiple patches which only touch dmaengine
files


> +#ifdef CONFIG_ARCH_KINETIS
> +static const char * const txirq_names[] = {
> +	"edma-tx-0,16",
> +	"edma-tx-1,17",
> +	"edma-tx-2,18",
> +	"edma-tx-3,19",
> +	"edma-tx-4,20",
> +	"edma-tx-5,21",
> +	"edma-tx-6,22",
> +	"edma-tx-7,23",
> +	"edma-tx-8,24",
> +	"edma-tx-9,25",
> +	"edma-tx-10,26",
> +	"edma-tx-11,27",
> +	"edma-tx-12,28",
> +	"edma-tx-13,29",
> +	"edma-tx-14,30",
> +	"edma-tx-15,31",
> +};
why do we need this array, these seem to come from DT, right?
> +#endif
> +
>  struct fsl_edma_engine {
>  	struct dma_device	dma_dev;
>  	void __iomem		*membase;
> +#ifdef CONFIG_ARCH_KINETIS
> +	struct clk		*clk;
> +#endif
>  	void __iomem		*muxbase[DMAMUX_NR];
>  	struct clk		*muxclk[DMAMUX_NR];
>  	struct mutex		fsl_edma_mutex;
>  	u32			n_chans;
> +#ifdef CONFIG_ARCH_KINETIS
> +	int			txirq[ARRAY_SIZE(txirq_names)];
> +#else
>  	int			txirq;
> +#endif
>  	int			errirq;
>  	bool			big_endian;
>  	struct fsl_edma_chan	chans[];
we can define these bits and only be used on kinetis machines?

> @@ -709,6 +737,7 @@ static irqreturn_t fsl_edma_err_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +#ifndef CONFIG_ARCH_KINETIS
>  static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
>  {
>  	if (fsl_edma_tx_handler(irq, dev_id) == IRQ_HANDLED)
> @@ -716,6 +745,7 @@ static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
>  
>  	return fsl_edma_err_handler(irq, dev_id);
>  }
> +#endif
>  
>  static void fsl_edma_issue_pending(struct dma_chan *chan)
>  {
> @@ -788,15 +818,29 @@ static void fsl_edma_free_chan_resources(struct dma_chan *chan)
>  }
>  
>  static int
> -fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma)
> +fsl_edma_irq_init(struct platform_device *pdev,
> +		  struct fsl_edma_engine *fsl_edma)
>  {
>  	int ret;
> +#ifdef CONFIG_ARCH_KINETIS
> +	int i;
>  
> +	for (i = 0; i < ARRAY_SIZE(txirq_names); i++) {
> +		fsl_edma->txirq[i] = platform_get_irq_byname(pdev,
> +					txirq_names[i]);
> +		if (fsl_edma->txirq[i] < 0) {
> +			dev_err(&pdev->dev, "Can't get %s irq.\n",
> +						txirq_names[i]);
> +			return fsl_edma->txirq[i];
> +		}
> +	}
> +#else
>  	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
>  	if (fsl_edma->txirq < 0) {
>  		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
>  		return fsl_edma->txirq;
>  	}
> +#endif
can you have two routines and with one of them onvoked based on machine type
which should be configured based on DT data rather

>  
>  	fsl_edma->errirq = platform_get_irq_byname(pdev, "edma-err");
>  	if (fsl_edma->errirq < 0) {
> @@ -804,6 +848,16 @@ fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma
>  		return fsl_edma->errirq;
>  	}
>  
> +#ifdef CONFIG_ARCH_KINETIS
> +	for (i = 0; i < ARRAY_SIZE(txirq_names); i++) {
> +		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq[i],
> +			fsl_edma_tx_handler, 0, txirq_names[i], fsl_edma);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
> +			return  ret;
> +		}
> +	}
> +#else
>  	if (fsl_edma->txirq == fsl_edma->errirq) {
>  		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
>  				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
> @@ -818,6 +872,7 @@ fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma
>  			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
>  			return  ret;
>  		}
> +#endif
only one of them will be populated so if-else should work too

please get rid of these ifdef stuff and make it based on DT data based flags

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] arm: twr-k70f120m: extend Freescale eDMA driver with ability to support Kinetis SoC

[Paul Osmialowski]

Hi Vinod,

Tanks for your comments. Actually, fsl-lpuart driver is done the way you 
propose to rework this one. I'll consider this during my work on the 
second iteration.

On Wed, 24 Jun 2015, Vinod Koul wrote:

> On Tue, Jun 23, 2015 at 11:19:46PM +0200, Paul Osmialowski wrote:
>> Surprisingly small amount of work was required in order to extend already
>> existing eDMA driver with the support for Kinetis SoC architecture.
>>
>> Note that <mach/memory.h> is needed (which is denoted by
>> CONFIG_NEED_MACH_MEMORY_H) as it provides macros required for proper
>> operation of DMA allocation functions.
>>
>> Signed-off-by: Paul Osmialowski <pawelo@king.net.pl>
>> ---
>>  Documentation/devicetree/bindings/dma/fsl-edma.txt | 38 +++++++++-
>>  arch/arm/Kconfig                                   |  4 ++
>>  arch/arm/boot/dts/kinetis.dtsi                     | 34 +++++++++
>>  arch/arm/mach-kinetis/include/mach/memory.h        | 61 ++++++++++++++++
>>  drivers/clk/clk-kinetis.c                          | 15 ++++
>>  drivers/dma/fsl-edma.c                             | 81 +++++++++++++++++++++-
>>  include/dt-bindings/clock/kinetis-mcg.h            |  5 +-
> having so many change into one patch is not a great idea, please breka them
> up. I am looking for single/multiple patches which only touch dmaengine
> files
>
>
>> +#ifdef CONFIG_ARCH_KINETIS
>> +static const char * const txirq_names[] = {
>> +	"edma-tx-0,16",
>> +	"edma-tx-1,17",
>> +	"edma-tx-2,18",
>> +	"edma-tx-3,19",
>> +	"edma-tx-4,20",
>> +	"edma-tx-5,21",
>> +	"edma-tx-6,22",
>> +	"edma-tx-7,23",
>> +	"edma-tx-8,24",
>> +	"edma-tx-9,25",
>> +	"edma-tx-10,26",
>> +	"edma-tx-11,27",
>> +	"edma-tx-12,28",
>> +	"edma-tx-13,29",
>> +	"edma-tx-14,30",
>> +	"edma-tx-15,31",
>> +};
> why do we need this array, these seem to come from DT, right?
>> +#endif
>> +
>>  struct fsl_edma_engine {
>>  	struct dma_device	dma_dev;
>>  	void __iomem		*membase;
>> +#ifdef CONFIG_ARCH_KINETIS
>> +	struct clk		*clk;
>> +#endif
>>  	void __iomem		*muxbase[DMAMUX_NR];
>>  	struct clk		*muxclk[DMAMUX_NR];
>>  	struct mutex		fsl_edma_mutex;
>>  	u32			n_chans;
>> +#ifdef CONFIG_ARCH_KINETIS
>> +	int			txirq[ARRAY_SIZE(txirq_names)];
>> +#else
>>  	int			txirq;
>> +#endif
>>  	int			errirq;
>>  	bool			big_endian;
>>  	struct fsl_edma_chan	chans[];
> we can define these bits and only be used on kinetis machines?
>
>> @@ -709,6 +737,7 @@ static irqreturn_t fsl_edma_err_handler(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>
>> +#ifndef CONFIG_ARCH_KINETIS
>>  static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
>>  {
>>  	if (fsl_edma_tx_handler(irq, dev_id) == IRQ_HANDLED)
>> @@ -716,6 +745,7 @@ static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
>>
>>  	return fsl_edma_err_handler(irq, dev_id);
>>  }
>> +#endif
>>
>>  static void fsl_edma_issue_pending(struct dma_chan *chan)
>>  {
>> @@ -788,15 +818,29 @@ static void fsl_edma_free_chan_resources(struct dma_chan *chan)
>>  }
>>
>>  static int
>> -fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma)
>> +fsl_edma_irq_init(struct platform_device *pdev,
>> +		  struct fsl_edma_engine *fsl_edma)
>>  {
>>  	int ret;
>> +#ifdef CONFIG_ARCH_KINETIS
>> +	int i;
>>
>> +	for (i = 0; i < ARRAY_SIZE(txirq_names); i++) {
>> +		fsl_edma->txirq[i] = platform_get_irq_byname(pdev,
>> +					txirq_names[i]);
>> +		if (fsl_edma->txirq[i] < 0) {
>> +			dev_err(&pdev->dev, "Can't get %s irq.\n",
>> +						txirq_names[i]);
>> +			return fsl_edma->txirq[i];
>> +		}
>> +	}
>> +#else
>>  	fsl_edma->txirq = platform_get_irq_byname(pdev, "edma-tx");
>>  	if (fsl_edma->txirq < 0) {
>>  		dev_err(&pdev->dev, "Can't get edma-tx irq.\n");
>>  		return fsl_edma->txirq;
>>  	}
>> +#endif
> can you have two routines and with one of them onvoked based on machine type
> which should be configured based on DT data rather
>
>>
>>  	fsl_edma->errirq = platform_get_irq_byname(pdev, "edma-err");
>>  	if (fsl_edma->errirq < 0) {
>> @@ -804,6 +848,16 @@ fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma
>>  		return fsl_edma->errirq;
>>  	}
>>
>> +#ifdef CONFIG_ARCH_KINETIS
>> +	for (i = 0; i < ARRAY_SIZE(txirq_names); i++) {
>> +		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq[i],
>> +			fsl_edma_tx_handler, 0, txirq_names[i], fsl_edma);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
>> +			return  ret;
>> +		}
>> +	}
>> +#else
>>  	if (fsl_edma->txirq == fsl_edma->errirq) {
>>  		ret = devm_request_irq(&pdev->dev, fsl_edma->txirq,
>>  				fsl_edma_irq_handler, 0, "eDMA", fsl_edma);
>> @@ -818,6 +872,7 @@ fsl_edma_irq_init(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma
>>  			dev_err(&pdev->dev, "Can't register eDMA tx IRQ.\n");
>>  			return  ret;
>>  		}
>> +#endif
> only one of them will be populated so if-else should work too
>
> please get rid of these ifdef stuff and make it based on DT data based flags
>
> -- 
> ~Vinod
>

Re: [PATCH 7/9] arm: twr-k70f120m: IOMUX driver for Kinetis SoC

[Paul Osmialowski]

Hi Paul,

Thanks for your comments. I'll make it tristate.

On Wed, 24 Jun 2015, Paul Bolle wrote:

> On Tue, 2015-06-23 at 23:19 +0200, Paul Osmialowski wrote:
>> --- a/drivers/pinctrl/freescale/Kconfig
>> +++ b/drivers/pinctrl/freescale/Kconfig
>
>> +config PINCTRL_KINETIS
>> +	bool "Kinetis pinctrl driver"
>> +	depends on OF
>> +	depends on SOC_K70
>> +	select PINMUX
>> +	help
>> +	  Say Y here to enable the Kinetis pinctrl driver
>
>> --- a/drivers/pinctrl/freescale/Makefile
>> +++ b/drivers/pinctrl/freescale/Makefile
>
>> +obj-$(CONFIG_PINCTRL_KINETIS)	+= pinctrl-kinetis.o
>
>> --- /dev/null
>> +++ b/drivers/pinctrl/freescale/pinctrl-kinetis.c
>
>> +#include <linux/module.h>
>
>> +static struct pinctrl_desc kinetis_pinctrl_desc = {
>> +	[...]
>> +	.owner = THIS_MODULE,
>> +};
>
>> +static void __exit kinetis_pinctrl_exit(void)
>> +{
>> +	platform_driver_unregister(&kinetis_pinctrl_driver);
>> +}
>> +module_exit(kinetis_pinctrl_exit);
>> +
>> +MODULE_DESCRIPTION("Freescale Kinetis pinctrl driver");
>> +MODULE_LICENSE("GPL v2");
>
> pinctrl-kinetis.o can only be built-in, right? But the code uses a few
> module specific constructs. Did you mean to make PINCTRL_KINETIS
> tristate instead?
>
> Thanks,
>
>
> Paul Bolle
>