* Re: [PATCH 3/8] Add all memory via sysfs probe interface at once
From: Michael Ellerman @ 2013-08-05 3:13 UTC (permalink / raw)
To: Nathan Fontenot
Cc: linux-mm, isimatu.yasuaki, linuxppc-dev, LKML, Greg Kroah-Hartman
In-Reply-To: <51FC04C2.70100@linux.vnet.ibm.com>
On Fri, Aug 02, 2013 at 02:13:06PM -0500, Nathan Fontenot wrote:
> On 08/01/2013 09:32 PM, Michael Ellerman wrote:
> > On Wed, Jul 24, 2013 at 01:37:47PM -0500, Nathan Fontenot wrote:
> >> When doing memory hot add via the 'probe' interface in sysfs we do not
> >> need to loop through and add memory one section at a time. I think this
> >> was originally done for powerpc, but is not needed. This patch removes
> >> the loop and just calls add_memory for all of the memory to be added.
> >
> > Looks like memory hot add is supported on ia64, x86, sh, powerpc and
> > s390. Have you tested on any?
>
> I have tested on powerpc. I would love to say I tested on the other
> platforms... but I haven't. I should be able to get a x86 box to test
> on but the other architectures may not be possible.
Is the rest of your series dependent on this patch? Or is it sort of
incidental?
If possible it might be worth pulling this one out and sticking it in
linux-next for a cycle to give people a chance to test it. Unless
someone who knows the code well is comfortable with it.
cheers
^ permalink raw reply
* Re: [PATCH 2/8] Mark powerpc memory resources as busy
From: Michael Ellerman @ 2013-08-05 3:11 UTC (permalink / raw)
To: Nathan Fontenot
Cc: linux-mm, isimatu.yasuaki, linuxppc-dev, LKML, Greg Kroah-Hartman
In-Reply-To: <51FC0315.1010601@linux.vnet.ibm.com>
On Fri, Aug 02, 2013 at 02:05:57PM -0500, Nathan Fontenot wrote:
> On 08/01/2013 09:28 PM, Michael Ellerman wrote:
> > On Wed, Jul 24, 2013 at 01:36:34PM -0500, Nathan Fontenot wrote:
> >> Memory I/O resources need to be marked as busy or else we cannot remove
> >> them when doing memory hot remove.
> >
> > I would have thought it was the opposite?
>
> Me too.
>
> As it turns out the code in kernel/resource.c checks to make sure the
> IORESOURCE_BUSY flag is set when trying to release a resource.
OK, I guess there's probably some sane reason, but it does seem
backward.
cheers
^ permalink raw reply
* Re: [PATCH] drivers/crypto/nx: saves chaining value from co-processor
From: Michael Ellerman @ 2013-08-05 2:21 UTC (permalink / raw)
To: Fionnuala Gunter; +Cc: linuxppc-dev
In-Reply-To: <1375481816.32477.22.camel@what-is-leg>
On Fri, Aug 02, 2013 at 05:16:56PM -0500, Fionnuala Gunter wrote:
> On Sat, 2013-08-03 at 07:59 +1000, Benjamin Herrenschmidt wrote:
> > On Fri, 2013-08-02 at 16:55 -0500, Fionnuala Gunter wrote:
> > > Sorry, I thought linuxppc-dev was the right tree since the patch is
> > > for
> > > a ppc driver and you've carried previous nx driver submissions
> > > upstream.
> > >
> > > I want to submit to the proper place, so what is your preference?
> >
> > Well, that's the right place if you want me to merge it, which seems to
> > be what Marcelo is also expecting...
>
> Please merge this patch, it fixes a hot bug.
You don't actually mention that anywhere in the changelog.
The description implies that it's probably a bug, but doesn't describe
the symptoms in any way - does it make it completely non-functional or
just break in certain corner cases.
And you make no mention of when this bug appeared, has it always been
there, or was it introduced by a particular commit? That's important
because it helps determine whether the patch should go into any of the
stable trees.
cheers
^ permalink raw reply
* Re: 3.11-rc3+git: __divdi3 undefined on powerpc (from radeon)
From: Guenter Roeck @ 2013-08-04 15:49 UTC (permalink / raw)
To: Meelis Roos; +Cc: linuxppc-dev, Linux Kernel list, dri-devel
In-Reply-To: <alpine.SOC.1.00.1308031534390.1352@math.ut.ee>
On Sat, Aug 03, 2013 at 03:44:26PM +0300, Meelis Roos wrote:
> While trying to compile v3.11-rc3-288-gabe0308 on powerpc 32-bit,
> it failed with the following linking error:
>
> ERROR: "__divdi3" [drivers/gpu/drm/radeon/radeon.ko] undefined!
>
> Some new 64-bit division in radeon that is not implemented on 32-bit
> powerpc?
>
> This is new - 3.11-rc3 worked fine.
>
Not only powerpc - i386 builds fail with the same error.
Guenter
^ permalink raw reply
* Re: windfarm_fcu_controls: cpu-pump-0 <HW FAULT>
From: Benjamin Herrenschmidt @ 2013-08-04 2:21 UTC (permalink / raw)
To: Andreas Schwab; +Cc: linuxppc-dev
In-Reply-To: <87mwoyhmj9.fsf@igel.home>
On Sat, 2013-08-03 at 16:33 +0200, Andreas Schwab wrote:
> But then, MacOS is happy and the hardware test didn't find anything
> either...
Ok, I'll take another peek at the MacOS code see if there's anything
meaningful there (though we don't have the complete stuff, I think the
fault mask we decoded from the OFW methods).
Cheers,
Ben.
^ permalink raw reply
* Re: windfarm_fcu_controls: cpu-pump-0 <HW FAULT>
From: Benjamin Herrenschmidt @ 2013-08-04 2:20 UTC (permalink / raw)
To: Andreas Schwab; +Cc: linuxppc-dev
In-Reply-To: <87r4eahoka.fsf@igel.home>
On Sat, 2013-08-03 at 15:49 +0200, Andreas Schwab wrote:
> Actually I see the same failure mask with the therm_pm72 driver, I
> didn't notice since the driver ignores the return value from set_rpm_fan
> and there is no sysfs file to monitor the cpu pumps. So it looks like
> some real hardware fault?
Either that, or the fault indication might be something to ignore on
those machines, I don't know at this point. It might be worth trying to
find another of these machines to compare.
Maybe the water leaked out ? :-)
Cheers,
Ben.
>
> Andreas.
>
^ permalink raw reply
* Re: [PATCH v2 7/8] powerpc/fsl_booke: make sure PAGE_OFFSET map to memstart_addr for relocatable kernel
From: Kevin Hao @ 2013-08-04 0:50 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1374884277.30721.39@snotra>
[-- Attachment #1: Type: text/plain, Size: 6088 bytes --]
On Fri, Jul 26, 2013 at 07:17:57PM -0500, Scott Wood wrote:
> On 07/04/2013 07:54:13 AM, Kevin Hao wrote:
> >@@ -1222,6 +1266,9 @@ _GLOBAL(switch_to_as1)
> > /*
> > * Restore to the address space 0 and also invalidate the tlb
> >entry created
> > * by switch_to_as1.
> >+ * r3 - the tlb entry which should be invalidated
> >+ * r4 - __pa(PAGE_OFFSET in AS0) - pa(PAGE_OFFSET in AS1)
> >+ * r5 - device tree virtual address
> > */
> > _GLOBAL(restore_to_as0)
> > mflr r0
> >@@ -1230,7 +1277,15 @@ _GLOBAL(restore_to_as0)
> > 0: mflr r9
> > addi r9,r9,1f - 0b
> >
> >- mfmsr r7
> >+ /*
> >+ * We may map the PAGE_OFFSET in AS0 to a different physical
> >address,
> >+ * so we need calculate the right jump and device tree address
> >based
> >+ * on the offset passed by r4.
> >+ */
>
> Whitespace
Fixed.
>
> >+ subf r9,r4,r9
> >+ subf r5,r4,r5
> >+
> >+2: mfmsr r7
> > li r8,(MSR_IS | MSR_DS)
> > andc r7,r7,r8
> >
> >@@ -1249,9 +1304,19 @@ _GLOBAL(restore_to_as0)
> > mtspr SPRN_MAS1,r9
> > tlbwe
> > isync
> >+
> >+ cmpwi r4,0
> >+ bne 3f
> > mtlr r0
> > blr
> >
> >+ /*
> >+ * The PAGE_OFFSET will map to a different physical address,
> >+ * jump to _start to do another relocation again.
> >+ */
> >+3: mr r3,r5
> >+ bl _start
> >+
> > /*
> > * We put a few things here that have to be page-aligned. This stuff
> > * goes at the beginning of the data segment, which is page-aligned.
> >diff --git a/arch/powerpc/mm/fsl_booke_mmu.c
> >b/arch/powerpc/mm/fsl_booke_mmu.c
> >index 8f60ef8..dd283fd 100644
> >--- a/arch/powerpc/mm/fsl_booke_mmu.c
> >+++ b/arch/powerpc/mm/fsl_booke_mmu.c
> >@@ -224,7 +224,7 @@ void __init adjust_total_lowmem(void)
> >
> > i = switch_to_as1();
> > __max_low_memory = map_mem_in_cams(ram, CONFIG_LOWMEM_CAM_NUM);
> >- restore_to_as0(i);
> >+ restore_to_as0(i, 0, 0);
>
> The device tree virtual address is zero?
No. But if the __pa(PAGE_OFFSET in AS0) is equal to __pa(PAGE_OFFSET in AS1),
that mean we don't need to do another relocation and the device tree virtual
address is useless in this case.
>
> > pr_info("Memory CAM mapping: ");
> > for (i = 0; i < tlbcam_index - 1; i++)
> >@@ -245,30 +245,56 @@ void setup_initial_memory_limit(phys_addr_t
> >first_memblock_base,
> > }
> >
> > #ifdef CONFIG_RELOCATABLE
> >-notrace void __init relocate_init(phys_addr_t start)
> >+int __initdata is_second_reloc;
> >+notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
> > {
> > unsigned long base = KERNELBASE;
> >
> >- /*
> >- * Relocatable kernel support based on processing of dynamic
> >- * relocation entries.
> >- * Compute the virt_phys_offset :
> >- * virt_phys_offset = stext.run - kernstart_addr
> >- *
> >- * stext.run = (KERNELBASE & ~0xfffffff) + (kernstart_addr &
> >0xfffffff)
> >- * When we relocate, we have :
> >- *
> >- * (kernstart_addr & 0xfffffff) = (stext.run & 0xfffffff)
> >- *
> >- * hence:
> >- * virt_phys_offset = (KERNELBASE & ~0xfffffff) -
> >- * (kernstart_addr & ~0xfffffff)
> >- *
> >- */
> > kernstart_addr = start;
> >- start &= ~0xfffffff;
> >- base &= ~0xfffffff;
> >- virt_phys_offset = base - start;
> >+ if (!is_second_reloc) {
>
> Since it's at the end of a function and one side is much shorter
> than the
> other, please do:
>
> if (is_second_reloc) {
> virt_phys_offset = PAGE_OFFSET - memstart_addr;
> return;
> }
>
> /* the rest of the code goes here without having to indent
> everything */
>
Yes, this looks much better. Changed.
> Otherwise, please use positive logic for if/else constructs.
>
> >+ phys_addr_t size;
> >+
> >+ /*
> >+ * Relocatable kernel support based on processing of dynamic
> >+ * relocation entries. Before we get the real memstart_addr,
> >+ * We will compute the virt_phys_offset like this:
> >+ * virt_phys_offset = stext.run - kernstart_addr
> >+ *
> >+ * stext.run = (KERNELBASE & ~0xfffffff) +
> >+ * (kernstart_addr & 0xfffffff)
> >+ * When we relocate, we have :
> >+ *
> >+ * (kernstart_addr & 0xfffffff) = (stext.run & 0xfffffff)
> >+ *
> >+ * hence:
> >+ * virt_phys_offset = (KERNELBASE & ~0xfffffff) -
> >+ * (kernstart_addr & ~0xfffffff)
> >+ *
> >+ */
> >+ start &= ~0xfffffff;
> >+ base &= ~0xfffffff;
> >+ virt_phys_offset = base - start;
> >+ early_get_first_memblock_info(__va(dt_ptr), &size);
> >+ /*
> >+ * We now get the memstart_addr, then we should check if this
> >+ * address is the same as what the PAGE_OFFSET map to now. If
> >+ * not we have to change the map of PAGE_OFFSET to
> >memstart_addr
> >+ * and do a second relocation.
> >+ */
> >+ if (start != memstart_addr) {
> >+ unsigned long ram;
> >+ int n, offset = memstart_addr - start;
> >+
> >+ is_second_reloc = 1;
> >+ ram = size;
> >+ n = switch_to_as1();
> >+ map_mem_in_cams(ram, CONFIG_LOWMEM_CAM_NUM);
>
> Do we really need this much RAM mapped at this point?
Not really.
> Why can't we
> continue
> with the same size TLB entry that we've been using, until the second
> relocation?
OK, I will change it to use 64M.
>
> >+ restore_to_as0(n, offset, __va(dt_ptr));
> >+ /* We should never reach here */
> >+ panic("Relocation error");
>
> Where is execution supposed to resume? It looks like you're
> expecting it
> to resume from _start,
Yes.
> but why?
For second relocation, we need to:
* do the real relocate
* set the interrupt vector
* zero the BSS
So starting from the _start will avoid to duplicate these codes.
> And where is this effect of
> restore_to_as0() documented?
There is a comment about this in restore_to_as0.
/*
* The PAGE_OFFSET will map to a different physical address,
* jump to _start to do another relocation again.
*/
3: mr r3,r5
bl _start
Thanks,
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH v2 6/8] powerpc: introduce early_get_first_memblock_info
From: Kevin Hao @ 2013-08-04 0:45 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1374884281.30721.40@snotra>
[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]
On Fri, Jul 26, 2013 at 07:18:01PM -0500, Scott Wood wrote:
> >+ * This function run before early_init_devtree, so we have to init
> >+ * initial_boot_params. Since early_init_dt_scan_memory_ppc will be
> >+ * executed again in early_init_devtree, we have to reinitialize the
> >+ * memblock data before return.
> >+ */
> >+void __init early_get_first_memblock_info(void *params,
> >phys_addr_t *size)
> >+{
> >+ /* Setup flat device-tree pointer */
> >+ initial_boot_params = params;
> >+
> >+ /* Scan memory nodes and rebuild MEMBLOCKs */
> >+ of_scan_flat_dt(early_init_dt_scan_root, NULL);
> >+ of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> >+
> >+ if (size)
> >+ *size = first_memblock_size;
> >+
> >+ /* Undo what early_init_dt_scan_memory_ppc does to memblock */
> >+ memblock_reinit();
> >+}
> >+#endif
>
> Wouldn't it be simpler to set a flag so that
> early_init_dt_add_memory_arch() doesn't mess with memblocks on the
> first pass?
Do you mean something like this?
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 9a69d2d..e861394 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -523,6 +523,10 @@ static int __init early_init_dt_scan_memory_ppc(unsigned long node,
void __init early_init_dt_add_memory_arch(u64 base, u64 size)
{
+#if defined(CONFIG_RELOCATABLE) && defined(CONFIG_FSL_BOOKE)
+ static int first_time = 1;
+#endif
+
#ifdef CONFIG_PPC64
if (iommu_is_off) {
if (base >= 0x80000000ul)
@@ -541,6 +545,13 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
}
/* Add the chunk to the MEMBLOCK list */
+
+#if defined(CONFIG_RELOCATABLE) && defined(CONFIG_FSL_BOOKE)
+ if (first_time) {
+ first_time = 0;
+ return;
+ }
+#endif
memblock_add(base, size);
}
Thanks,
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* Re: [PATCH v2 4/8] powerpc/fsl_booke: set the tlb entry for the kernel address in AS1
From: Kevin Hao @ 2013-08-04 0:42 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1374881830.30721.38@snotra>
[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]
On Fri, Jul 26, 2013 at 06:37:10PM -0500, Scott Wood wrote:
> On 07/04/2013 07:54:10 AM, Kevin Hao wrote:
> >--- a/arch/powerpc/kernel/head_fsl_booke.S
<snip>
> >+++ b/arch/powerpc/kernel/head_fsl_booke.S
> >@@ -1172,6 +1172,87 @@ __secondary_hold_acknowledge:
> > #endif
> >
> > /*
> >+ * Create a tbl entry
>
> s/tbl/tlb/
Fixed.
>
> >diff --git a/arch/powerpc/mm/fsl_booke_mmu.c
> >b/arch/powerpc/mm/fsl_booke_mmu.c
> >index 5fe271c..8f60ef8 100644
> >--- a/arch/powerpc/mm/fsl_booke_mmu.c
> >+++ b/arch/powerpc/mm/fsl_booke_mmu.c
> >@@ -222,7 +222,9 @@ void __init adjust_total_lowmem(void)
> > /* adjust lowmem size to __max_low_memory */
> > ram = min((phys_addr_t)__max_low_memory,
> >(phys_addr_t)total_lowmem);
> >
> >+ i = switch_to_as1();
> > __max_low_memory = map_mem_in_cams(ram, CONFIG_LOWMEM_CAM_NUM);
> >+ restore_to_as0(i);
>
> Wouldn't it be simpler to just run out of AS1 from the end of
> fsl_booke_entry_mapping.S, similar to what U-Boot does? With ESEL
> being changed to something non-conflicting, of course.
This pair of functions will be used by the codes in the following patch.
Thanks,
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/8] powerpc: enable the relocatable support for the fsl booke 32bit kernel
From: Kevin Hao @ 2013-08-04 0:38 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1374881326.30721.37@snotra>
[-- Attachment #1: Type: text/plain, Size: 1916 bytes --]
On Fri, Jul 26, 2013 at 06:28:46PM -0500, Scott Wood wrote:
> On 07/04/2013 07:54:09 AM, Kevin Hao wrote:
> >This is based on the codes in the head_44x.S. Since we always align to
> >256M before mapping the PAGE_OFFSET for a relocatable kernel, we also
> >change the init tlb map to 256M size.
> >
> >Signed-off-by: Kevin Hao <haokexin@gmail.com>
> >---
> >v2: Move the code to set kernstart_addr and virt_phys_offset to a
> >c function.
> > So we can expand it easily later.
> >
> >Hi Scott,
> >
> >I still use the 256M align for the init tlb as in v1 for the
> >following reasons:
> > * This should be the most possible case in reality.
>
> There is no "most possible case". It's either possible (and
> supported) or not. And having less than 256M is definitely
> possible. The 8540 reference board has 64M.
>
> AMP scenarios that start on a 64M-aligned but not 256M-aligned
> address are also something I've done.
>
> > * This is just for very early booting code and should not be a
> >big issue
> > if the first tlb entry shrink to a less size later.
>
> "We can probably get away with it most of the time" is not a very
> good justification. What's wrong with the suggestion I made last
> time, of basing the size on the alignment of the address?
OK, I will use the 64M align.
>
> >+ /*
> >+ * We have the runtime (virutal) address of our base.
> >+ * We calculate our shift of offset from a 256M page.
> >+ * We could map the 256M page we belong to at PAGE_OFFSET and
> >+ * get going from there.
> >+ */
> >+ lis r4,KERNELBASE@h
> >+ ori r4,r4,KERNELBASE@l
> >+ rlwinm r6,r25,0,0xfffffff /* r6 = PHYS_START % 256M */
> >+ rlwinm r5,r4,0,0xfffffff /* r5 = KERNELBASE % 256M */
> >+ subf r3,r5,r6 /* r3 = r6 - r5 */
> >+ add r3,r4,r3 /* Required Virutal Address */
>
> s/Virutal/Virtual/
Fixed.
Thanks,
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/8] powerpc/fsl_booke: protect the access to MAS7 with MMU_FTR_BIG_PHYS
From: Kevin Hao @ 2013-08-04 0:30 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc
In-Reply-To: <1374880440.30721.36@snotra>
[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]
On Fri, Jul 26, 2013 at 06:14:00PM -0500, Scott Wood wrote:
> On 07/04/2013 07:54:07 AM, Kevin Hao wrote:
> >diff --git a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> >b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> >index a92c79b..2201f84 100644
> >--- a/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> >+++ b/arch/powerpc/kernel/fsl_booke_entry_mapping.S
> >@@ -88,9 +88,11 @@ skpinv: addi r6,r6,1 /* Increment */
> > 1: mflr r7
> >
> > mfspr r8,SPRN_MAS3
> >-#ifdef CONFIG_PHYS_64BIT
> >+BEGIN_MMU_FTR_SECTION
> > mfspr r23,SPRN_MAS7
> >-#endif
> >+MMU_FTR_SECTION_ELSE
> >+ li r23,0
> >+ALT_MMU_FTR_SECTION_END_IFSET(MMU_FTR_BIG_PHYS)
> > and r8,r6,r8
> > subfic r9,r6,-4096
> > and r9,r9,r7
> >diff --git a/arch/powerpc/kernel/head_fsl_booke.S
> >b/arch/powerpc/kernel/head_fsl_booke.S
> >index d10a7ca..a04a48d 100644
> >--- a/arch/powerpc/kernel/head_fsl_booke.S
> >+++ b/arch/powerpc/kernel/head_fsl_booke.S
> >@@ -82,7 +82,11 @@ _ENTRY(_start);
> > and r19,r3,r18 /* r19 = page offset */
> > andc r31,r20,r18 /* r31 = page base */
> > or r31,r31,r19 /* r31 = devtree phys addr */
> >+BEGIN_MMU_FTR_SECTION
> > mfspr r30,SPRN_MAS7
> >+MMU_FTR_SECTION_ELSE
> >+ li r30,0
> >+ALT_MMU_FTR_SECTION_END_IFSET(MMU_FTR_BIG_PHYS)
>
> Code patching hasn't been done yet at this point.
Indeed. I overlooked this. I will change it to #ifdef CONFIG_PHYS_64BIT.
Thanks,
Kevin
>
> -Scott
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
From: Gerhard Sittig @ 2013-08-03 17:07 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Mike Turquette, Detlev Zundel, Greg Kroah-Hartman, Mark Brown,
linux-arm-kernel, Anatolij Gustschin, linuxppc-dev,
Wolfgang Grandegger
In-Reply-To: <51EE77FC.8090809@pengutronix.de>
[ trimming the CC: list a bit, as this is CAN and clock specific,
keeping Mark Brown and Greg KH for the UART and SPI part ]
On Tue, Jul 23, 2013 at 14:33 +0200, Marc Kleine-Budde wrote:
>
> On 07/23/2013 01:53 PM, Gerhard Sittig wrote:
> > On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote:
> >>
> >> On 07/22/2013 02:14 PM, Gerhard Sittig wrote:
> >>> the .get_clock() callback is run from probe() and might allocate
> >>> resources, introduce a .put_clock() callback that is run from remove()
> >>> to undo any allocation activities
> >>
> >> looks good
> >>
> >>> use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put
> >>> upon driver unload
> >>
> >> fine
> >>
> >>> assume that resources get prepared but not necessarily enabled in the
> >>> setup phase, make the open() and close() callbacks of the CAN network
> >>> device enable and disable a previously acquired and prepared clock
> >>
> >> I think you should call prepare_enable and disable_unprepare in the
> >> open/close functions.
> >
> > After more local research, which totally eliminated the need to
> > pre-enable the CAN related clocks, but might need more discussion
> > as it touches the common gate support, I've learned something
> > more:
> >
> > The CAN clock needs to get enabled during probe() already, since
> > registers get accessed between probe() for the driver and open()
> > for the network device -- while access to peripheral registers
> > crashes the kernel when clocks still are disabled (other hardware
> > may just hang or provide fake data, neither of this is OK).
>
> Then call prepare_enable(); before and disable_unprepare(); after
> accessing the registers. Have a look at the flexcan driver.
OK, your feedback made me notice that I mentally have mixed
peripheral access clocks ('ipg') and bitrate clocks ('per') in
the past versions of the driver.
Fixing this, telling 'ipg' and 'per' apart, "in bypassing"
eliminates the need for "shared clock gates". Since the MCLK
subtree of the clock tree apply to both the CAN controller and
the PSC controller, I will have to adjust all of the following:
- the platform's clock driver, telling the gate for the registers
and the mux/div for the bitrate apart
- the CAN driver, acquiring both the 'ipg' clock item for
register access and the "can" clock for the bitrate, the latter
may get derived from either 'ips' or 'mclk', while 'mclk' may
be derived from either 'sys' or 'ref' (or 'ips' in this
hardware while the mscan(4) driver doesn't use this feature)
- the UART and SPI drivers, acquiring both the 'ipg' clock item
for register access and the 'mclk' item for the bitrate
This obsoletes the request for "shared gates" and eliminates
another pre-enable workaround in the clock driver backend.
It also is an improvement for the MPC512x platform, and remains
neutral to the MPC52xx platform. It's clearly desirable and
useful, and doesn't break anything. So I will do it.
[ the above applied to CAN, SPI, and UART; the remainder is
specific to CAN only ]
But I won't try to even further widen the scope of the series, I
won't try to address each and every potential for improvement
which drivers may have had for several years and which happened
to not have been addressed yet. This needs to stop at some
reasonable point. I'm not refusing to improve, but I'm asking to
check what is reasonable and what needs to get avoided.
I already introduced a bug in a recent version of the series
which went unnoticed during review (the unbalanced error path in
the network device open routine). I'd rather not mess with power
management aspects "in bypassing" in a driver that I'm unable to
test thoroughly. Not when I'm trying to work on something
totally different (introducing proper common clock support) and
try to minimize risk and avoid damage.
> > But I see the point in your suggestion to prepare _and_ enable
> > the clock during open() as well -- to have open() cope with
> > whatever probe() did, after all the driver is shared among
> > platforms, which may differ in what they do during probe().
>
> If you enable a clock to access the registers before open() (and disable
> it afterwards), it should not harm any architecture that doesn't need
> this clock enabled.
You suggest to turn on the clock during initialization, and turn
it off until the network device actually gets used? I had a look
at the flexcan driver, saw that it used two clock items, as
outlined above for register access and for wired communication.
This is good. But I somehow doubt that the flexcan driver will
work if the ipg clock gets disabled (I assume it's a shared clock
that happens to remain enabled since others use it as well).
I'd rather not open that can of worms, too. My gut is telling me
that either the peripheral does weird things or will lose data
when its (register access) clock gets disabled. I won't try to
address power management and save/restore issues in that driver
now, and I won't try to hunt down and instrument any register
access in the shared code paths of a driver for multiple
platforms which is full of callbacks.
That's just out of the scope of the series. It may be desirable
to address this issue as well, but it shall be done in a separate
action, not now "in bypassing". Thank you for understanding. :)
And I do appreciate your feedback and desire for even better
drivers, just disagree on what to do now in this very moment.
What we already have is:
- probe() and remove() for the driver, calling into clock setup
and allocation and deallocation routines
- no allocation for MPC52xx and thus no deallocation, keeping the
status of how things used to be
- allocation of a "can" clock for MPC512x and the respective
deallocation
- open() and close() for the network device, which prepare/enable
and disable/unprepare the allocated clocks
What I will add is:
- allocation and release of both the 'ipg' and a "can" clock for
the MPC512x case
- handling of all allocated clocks in open() and close() (such
that no assumption is made what occurs at probe() and remove()
time)
- permanently enabled 'ipg' clock when allocated, such that the
driver may happily access the controller's registers and may
assume things remain there
- usually disabled 'can' clock, but enable/disable between open()
and close()
This shall result in:
- no change in behaviour for MPC52xx
- disabled clocks and hardware for MPC512x when CAN isn't probed
(not listed in the device tree, or disabled)
- enabled internal peripherals but wire disabled for MPC512x when
CAN is probed and attached but not in use
- only enabled wire related clock when the network device is open
and in actual use
The above goal of the next update in the series won't break any
operation of peripherals, will be a clear improvement in that the
driver finally does properly use the clock API, and will result
in an appropriate use of hardware. There may be potential to
conserve even more power, but it's not essential given the
previous status of the driver, and it's out of the scope for the
very series we are talking about.
> > So I will:
> > - make open() of the network device prepare _and_ enable the
> > clock for the peripheral (if acquired during probe())
>
> good
>
> > - adjust open() because ATM it leaves the clock enabled when the
> > network device operation fails (the error path is incomplete in
> > v3)
>
> yes, clock should be disabled if open() fails.
>
> > - make the MPC512x specific probe() time .get_clock() routine not
> > just prepare but enable the clock as well
>
> If needed enable the clock, but disable after probe() has finished.
>
> > - and of course address all the shutdown counter parts of the
> > above setup paths
>
> > This results in:
> > - specific chip drivers only need to balance their private get
> > and put clock routines which are called from probe and remove,
> > common paths DTRT for all of them
>
> Yes, but clock should not stay enabled between probe() and open().
For this one I offered the compromise of only enabling the "can"
clock during network device operation, but keeping the 'ipg'
clock active over the driver's complete attachment period. This
shall be acceptable. Anything else can be done later and
independently.
> [...]
>
> > Removing unnecessary devm_put_clk() calls is orthogonal to that.
> > Putting these in isn't totally wrong (they won't harm, and they
> > do signal "visual balance" more clearly such that the next person
> > won't stop and wonder), but it's true that they are redundant.
> > "Trained persons" will wonder as much about their presence as
> > untrained persons wonder about their absence. :) Apparently I'm
> > not well trained yet.
>
> The whole point about devm_* is to get rid of auto manually tear down
> functions. So please remove all devm_put_clk() calls, as it will be
> called automatically if a driver instance is removed.
Ah, yes, the devm_*() feedback was processed as well. Release
won't occur explicitly, as it's done implicitly by common code.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH RFC v3 1/5] dma: mpc512x: reorder mpc8308 specific instructions
From: Gerhard Sittig @ 2013-08-03 16:06 UTC (permalink / raw)
To: Alexander Popov
Cc: Lars-Peter Clausen, Arnd Bergmann, Vinod Koul, linux-kernel,
Dan Williams, Anatolij Gustschin, linuxppc-dev
In-Reply-To: <1375255254-10955-1-git-send-email-a13xp0p0v88@gmail.com>
On Wed, Jul 31, 2013 at 11:20 +0400, Alexander Popov wrote:
>
> From: Gerhard Sittig <gsi@denx.de>
>
> Concentrate the test and the specific code for MPC8308
> in the 'if' branch and handle MPC512x in the 'else' branch.
>
> This modification only reorders instructions but doesn't change behaviour.
>
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> ---
> drivers/dma/mpc512x_dma.c | 42 +++++++++++++++++++++++++-----------------
> 1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 2d95673..b8881de 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -50,9 +50,17 @@
> #define MPC_DMA_DESCRIPTORS 64
>
> /* Macro definitions */
> -#define MPC_DMA_CHANNELS 64
> #define MPC_DMA_TCD_OFFSET 0x1000
>
> +/*
> + * Maximum channel counts for individual hardware variants
> + * and the maximum channel count over all supported controllers,
> + * used for data structure size
> + */
> +#define MPC8308_DMACHAN_MAX 16
> +#define MPC512x_DMACHAN_MAX 64
> +#define MPC_DMA_CHANNELS 64
> +
> /* Arbitration mode of group and channel */
> #define MPC_DMA_DMACR_EDCG (1 << 31)
> #define MPC_DMA_DMACR_ERGA (1 << 3)
nit: That's not what I wrote. Please make sure to either cite
properly or to properly mark changes as such. Don't spread false
information, please. You are free to change what I submitted,
but you should not pretend that I wrote what has become of the
code after you have modified it. Please fix the attribution.
Just to clarify: The defines here appear to be more appropriate
than the initial enums, after it turned out that we need not
handle indiviudal channels in special ways, and really only need
these three numbers (one of them being the maximum of the
others). But regardless of what you have changed, you should
clearly state the fact.
> @@ -716,10 +724,10 @@ static int mpc_dma_probe(struct platform_device *op)
>
> dma = &mdma->dma;
> dma->dev = dev;
> - if (!mdma->is_mpc8308)
> - dma->chancnt = MPC_DMA_CHANNELS;
> + if (mdma->is_mpc8308)
> + dma->chancnt = MPC8308_DMACHAN_MAX;
> else
> - dma->chancnt = 16; /* MPC8308 DMA has only 16 channels */
> + dma->chancnt = MPC512x_DMACHAN_MAX;
> dma->device_alloc_chan_resources = mpc_dma_alloc_chan_resources;
> dma->device_free_chan_resources = mpc_dma_free_chan_resources;
> dma->device_issue_pending = mpc_dma_issue_pending;
> @@ -753,7 +761,19 @@ static int mpc_dma_probe(struct platform_device *op)
> * - Round-robin group arbitration,
> * - Round-robin channel arbitration.
> */
> - if (!mdma->is_mpc8308) {
> + if (mdma->is_mpc8308) {
> + /* MPC8308 has 16 channels and lacks some registers */
> + out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_ERCA);
> +
> + /* enable snooping */
> + out_be32(&mdma->regs->dmagpor, MPC_DMA_DMAGPOR_SNOOP_ENABLE);
> + /* Disable error interrupts */
> + out_be32(&mdma->regs->dmaeeil, 0);
> +
> + /* Clear interrupts status */
> + out_be32(&mdma->regs->dmaintl, 0xFFFF);
> + out_be32(&mdma->regs->dmaerrl, 0xFFFF);
> + } else {
> out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_EDCG |
> MPC_DMA_DMACR_ERGA | MPC_DMA_DMACR_ERCA);
>
> @@ -774,18 +794,6 @@ static int mpc_dma_probe(struct platform_device *op)
> /* Route interrupts to IPIC */
> out_be32(&mdma->regs->dmaihsa, 0);
> out_be32(&mdma->regs->dmailsa, 0);
> - } else {
> - /* MPC8308 has 16 channels and lacks some registers */
> - out_be32(&mdma->regs->dmacr, MPC_DMA_DMACR_ERCA);
> -
> - /* enable snooping */
> - out_be32(&mdma->regs->dmagpor, MPC_DMA_DMAGPOR_SNOOP_ENABLE);
> - /* Disable error interrupts */
> - out_be32(&mdma->regs->dmaeeil, 0);
> -
> - /* Clear interrupts status */
> - out_be32(&mdma->regs->dmaintl, 0xFFFF);
> - out_be32(&mdma->regs->dmaerrl, 0xFFFF);
> }
>
> /* Register DMA engine */
> --
> 1.7.11.3
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH RFC v3 2/5] dma: mpc512x: add support for peripheral transfers
From: Gerhard Sittig @ 2013-08-03 15:53 UTC (permalink / raw)
To: Alexander Popov
Cc: Lars-Peter Clausen, Arnd Bergmann, Vinod Koul, linux-kernel,
Dan Williams, Anatolij Gustschin, linuxppc-dev
In-Reply-To: <1375255292-11288-1-git-send-email-a13xp0p0v88@gmail.com>
On Wed, Jul 31, 2013 at 11:21 +0400, Alexander Popov wrote:
>
> Introduce support for slave s/g transfer preparation and the associated
> device control callback in the MPC512x DMA controller driver, which adds
> support for data transfers between memory and peripheral I/O to the
> previously supported mem-to-mem transfers.
>
> Refuse to prepare chunked transfers (transfers with more than one part)
> as long as proper support for scatter/gather is lacking.
>
> Keep MPC8308 operational by always starting transfers from software,
> this SoC appears to not have request lines for flow control when
> peripherals are involved in transfers.
>
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> ---
> drivers/dma/mpc512x_dma.c | 183 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 176 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
You don't provide a lot of information to those you want to
receive feedback from. You should keep a history and list the
changes between versions. And you may want to somehow link this
v3 to its predecessor -- especially when you only send part of
the series and assume that reviewers may know where to find the
remainder.
Please help those persons you want to get help from.
> index b8881de..d96d107 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -2,6 +2,7 @@
> * Copyright (C) Freescale Semicondutor, Inc. 2007, 2008.
> * Copyright (C) Semihalf 2009
> * Copyright (C) Ilya Yanok, Emcraft Systems 2010
> + * Copyright (C) Alexander Popov, Promcontroller 2013
> *
> * Written by Piotr Ziecik <kosmo@semihalf.com>. Hardware description
> * (defines, structures and comments) was taken from MPC5121 DMA driver
> @@ -28,11 +29,6 @@
> * file called COPYING.
> */
>
> -/*
> - * This is initial version of MPC5121 DMA driver. Only memory to memory
> - * transfers are supported (tested using dmatest module).
> - */
> -
> #include <linux/module.h>
> #include <linux/dmaengine.h>
> #include <linux/dma-mapping.h>
> @@ -199,6 +195,11 @@ struct mpc_dma_chan {
> struct mpc_dma_tcd *tcd;
> dma_addr_t tcd_paddr;
>
> + /* Settings for access to peripheral FIFO */
> + int will_access_peripheral;
> + dma_addr_t per_paddr; /* FIFO address */
> + u32 tcd_nunits;
> +
> /* Lock for this structure */
> spinlock_t lock;
> };
> @@ -264,7 +265,10 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
>
> prev->tcd->dlast_sga = mdesc->tcd_paddr;
> prev->tcd->e_sg = 1;
> - mdesc->tcd->start = 1;
> +
> + /* software start for mem-to-mem transfers */
> + if (mdma->is_mpc8308 || !mchan->will_access_peripheral)
> + mdesc->tcd->start = 1;
here (channel -> will access peripheral)
>
> prev = mdesc;
> }
> @@ -276,7 +280,17 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
>
> if (first != prev)
> mdma->tcd[cid].e_sg = 1;
> - out_8(&mdma->regs->dmassrt, cid);
> +
> + if (mdma->is_mpc8308) {
> + /* MPC8308, no request lines, software initiated start */
> + out_8(&mdma->regs->dmassrt, cid);
> + } else if (mchan->will_access_peripheral) {
> + /* peripherals involved, use external request line */
> + out_8(&mdma->regs->dmaserq, cid);
> + } else {
> + /* memory to memory transfer, software initiated start */
> + out_8(&mdma->regs->dmassrt, cid);
> + }
and here (channel -> will access peripheral)
> }
>
> /* Handle interrupt on one half of DMA controller (32 channels) */
> @@ -649,6 +663,158 @@ mpc_dma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
> return &mdesc->desc;
> }
>
> +static struct dma_async_tx_descriptor *mpc_dma_prep_slave_sg(
> + struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sg_len, enum dma_transfer_direction direction,
> + unsigned long flags, void *context)
> +{
> + struct mpc_dma *mdma = dma_chan_to_mpc_dma(chan);
> + struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
> + struct mpc_dma_desc *mdesc = NULL;
> + dma_addr_t per_paddr;
> + u32 tcd_nunits = 0;
> + struct mpc_dma_tcd *tcd;
> + unsigned long iflags;
> + struct scatterlist *sg;
> + size_t len;
> + int iter, i;
> +
> + if (!list_empty(&mchan->active))
> + return NULL;
> +
> + /* currently there is no proper support for scatter/gather */
> + if (sg_len > 1)
> + return NULL;
> +
> + for_each_sg(sgl, sg, sg_len, i) {
> + spin_lock_irqsave(&mchan->lock, iflags);
> +
> + mdesc = list_first_entry(&mchan->free, struct mpc_dma_desc,
> + node);
> + if (!mdesc) {
> + spin_unlock_irqrestore(&mchan->lock, iflags);
> + /* try to free completed descriptors */
> + mpc_dma_process_completed(mdma);
> + return NULL;
> + }
> +
> + list_del(&mdesc->node);
> +
> + per_paddr = mchan->per_paddr;
> + tcd_nunits = mchan->tcd_nunits;
> +
> + spin_unlock_irqrestore(&mchan->lock, iflags);
> +
> + mdesc->error = 0;
> + tcd = mdesc->tcd;
> +
> + /* Prepare Transfer Control Descriptor for this transaction */
> + memset(tcd, 0, sizeof(struct mpc_dma_tcd));
> +
> + if (!IS_ALIGNED(sg_dma_address(sg), 4))
> + return NULL;
> +
> + if (direction == DMA_DEV_TO_MEM) {
> + tcd->saddr = per_paddr;
> + tcd->daddr = sg_dma_address(sg);
> + tcd->soff = 0;
> + tcd->doff = 4;
> + } else if (direction == DMA_MEM_TO_DEV) {
> + tcd->saddr = sg_dma_address(sg);
> + tcd->daddr = per_paddr;
> + tcd->soff = 4;
> + tcd->doff = 0;
> + } else {
> + return NULL;
> + }
> + tcd->ssize = MPC_DMA_TSIZE_4;
> + tcd->dsize = MPC_DMA_TSIZE_4;
> +
> + len = sg_dma_len(sg);
> +
> + if (tcd_nunits)
> + tcd->nbytes = tcd_nunits * 4;
> + else
> + return NULL;
> +
> + if (!IS_ALIGNED(len, tcd->nbytes))
> + return NULL;
> +
> + iter = len / tcd->nbytes;
> + if (iter > ((1 << 15) - 1)) { /* maximum biter */
> + return NULL; /* len is too big */
> + } else {
> + /* citer_linkch contains the high bits of iter */
> + tcd->biter = iter & 0x1ff;
> + tcd->biter_linkch = iter >> 9;
> + tcd->citer = tcd->biter;
> + tcd->citer_linkch = tcd->biter_linkch;
> + }
> +
> + tcd->e_sg = 0;
> + tcd->d_req = 1;
> +
> + /* Place descriptor in prepared list */
> + spin_lock_irqsave(&mchan->lock, iflags);
> + list_add_tail(&mdesc->node, &mchan->prepared);
> + spin_unlock_irqrestore(&mchan->lock, iflags);
> + }
> +
> + return &mdesc->desc;
> +}
> +
> +static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> + unsigned long arg)
> +{
> + struct mpc_dma_chan *mchan;
> + struct mpc_dma *mdma;
> + struct dma_slave_config *cfg;
> + unsigned long flags;
> +
> + mchan = dma_chan_to_mpc_dma_chan(chan);
> + switch (cmd) {
> + case DMA_TERMINATE_ALL:
> + /* disable channel requests */
> + mdma = dma_chan_to_mpc_dma(chan);
> +
> + spin_lock_irqsave(&mchan->lock, flags);
> +
> + out_8(&mdma->regs->dmacerq, chan->chan_id);
> + list_splice_tail_init(&mchan->prepared, &mchan->free);
> + list_splice_tail_init(&mchan->queued, &mchan->free);
> + list_splice_tail_init(&mchan->active, &mchan->free);
> +
> + spin_unlock_irqrestore(&mchan->lock, flags);
> +
> + return 0;
> + case DMA_SLAVE_CONFIG:
> + cfg = (void *)arg;
> + if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES &&
> + cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + return -EINVAL;
> +
> + spin_lock_irqsave(&mchan->lock, flags);
> +
> + mchan->will_access_peripheral = 1;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {
> + mchan->per_paddr = cfg->src_addr;
> + mchan->tcd_nunits = cfg->src_maxburst;
> + } else {
> + mchan->per_paddr = cfg->dst_addr;
> + mchan->tcd_nunits = cfg->dst_maxburst;
> + }
> +
> + spin_unlock_irqrestore(&mchan->lock, flags);
> +
> + return 0;
> + default:
> + return -ENOSYS;
> + }
> +
> + return -EINVAL;
> +}
> +
and here
I think it's unfortunate to attribute the "will access
peripheral" to the channel instead of the transfer job, and to
set the flag from within the device control callback, and to
nevery clear the flag (what will happen if a channel gets freed
and reallocated by some other client?).
I think that the peripheral access is an attribute of the
transfer job, and should be setup in the prep routines (both set
and cleared, depending on what gets setup). This would be more
robust and more readable (read: maintainable) in my eyes.
> static int mpc_dma_probe(struct platform_device *op)
> {
> struct device_node *dn = op->dev.of_node;
> @@ -733,9 +899,12 @@ static int mpc_dma_probe(struct platform_device *op)
> dma->device_issue_pending = mpc_dma_issue_pending;
> dma->device_tx_status = mpc_dma_tx_status;
> dma->device_prep_dma_memcpy = mpc_dma_prep_memcpy;
> + dma->device_prep_slave_sg = mpc_dma_prep_slave_sg;
> + dma->device_control = mpc_dma_device_control;
>
> INIT_LIST_HEAD(&dma->channels);
> dma_cap_set(DMA_MEMCPY, dma->cap_mask);
> + dma_cap_set(DMA_SLAVE, dma->cap_mask);
>
> for (i = 0; i < dma->chancnt; i++) {
> mchan = &mdma->channels[i];
> --
> 1.7.11.3
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 18/31] dts: mpc512x: add clock specs for client lookups
From: Gerhard Sittig @ 2013-08-03 15:36 UTC (permalink / raw)
To: Mike Turquette
Cc: devicetree, Anatolij Gustschin, linuxppc-dev, Rob Herring,
linux-arm-kernel
In-Reply-To: <20130802234120.6450.57074@quantum>
[ trimming the CC: list to device tree and ARM(clock) and PPC ]
On Fri, Aug 02, 2013 at 16:41 -0700, Mike Turquette wrote:
>
> Quoting Gerhard Sittig (2013-07-22 05:14:45)
> > this addresses the client side of device tree based clock lookups
> >
> > add clock specifiers to the mbx, nfc, mscan, sdhc, i2c, axe, diu, viu,
> > mdio, fec, usb, pata, psc, psc fifo, and pci nodes in the shared
> > mpc5121.dtsi include
> >
> > these specs map 'clock-names' encoded in drivers to their respective
> > 'struct clk' items in the platform's clock driver
> >
> > Signed-off-by: Gerhard Sittig <gsi@denx.de>
>
> Reviewed-by: Mike Turquette <mturquette@linaro.org>
Thank you for the review, Mike!
Please note that there will be another update for the series.
These clock specs for client side lookups will have additional
'ipg' clock items next to 'mclk' for the PSC (UART, SPI) and
MSCAN (CAN) nodes. But this change is straight forward.
> > ---
> > arch/powerpc/boot/dts/mpc5121.dtsi | 79 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi
> > index 8f4cba0..3657ae6 100644
> > --- a/arch/powerpc/boot/dts/mpc5121.dtsi
> > +++ b/arch/powerpc/boot/dts/mpc5121.dtsi
> > @@ -51,6 +51,10 @@
> > compatible = "fsl,mpc5121-mbx";
> > reg = <0x20000000 0x4000>;
> > interrupts = <66 0x8>;
> > + clocks = <&clks MPC512x_CLK_MBX_BUS>,
> > + <&clks MPC512x_CLK_MBX_3D>,
> > + <&clks MPC512x_CLK_MBX>;
> > + clock-names = "mbx-bus", "mbx-3d", "mbx";
> > };
> >
> > sram@30000000 {
> > @@ -64,6 +68,8 @@
> > interrupts = <6 8>;
> > #address-cells = <1>;
> > #size-cells = <1>;
> > + clocks = <&clks MPC512x_CLK_NFC>;
> > + clock-names = "per";
> > };
> >
> > localbus@80000020 {
> > @@ -153,12 +159,22 @@
> > compatible = "fsl,mpc5121-mscan";
> > reg = <0x1300 0x80>;
> > interrupts = <12 0x8>;
> > + clocks = <&clks MPC512x_CLK_IPS>,
> > + <&clks MPC512x_CLK_SYS>,
> > + <&clks MPC512x_CLK_REF>,
> > + <&clks MPC512x_CLK_MSCAN0_MCLK>;
> > + clock-names = "ips", "sys", "ref", "mclk";
> > };
> >
> > can@1380 {
> > compatible = "fsl,mpc5121-mscan";
> > reg = <0x1380 0x80>;
> > interrupts = <13 0x8>;
> > + clocks = <&clks MPC512x_CLK_IPS>,
> > + <&clks MPC512x_CLK_SYS>,
> > + <&clks MPC512x_CLK_REF>,
> > + <&clks MPC512x_CLK_MSCAN1_MCLK>;
> > + clock-names = "ips", "sys", "ref", "mclk";
> > };
> >
> > sdhc@1500 {
> > @@ -167,6 +183,9 @@
> > interrupts = <8 0x8>;
> > dmas = <&dma0 30>;
> > dma-names = "rx-tx";
> > + clocks = <&clks MPC512x_CLK_IPS>,
> > + <&clks MPC512x_CLK_SDHC>;
> > + clock-names = "ipg", "per";
> > };
> >
> > i2c@1700 {
> > @@ -175,6 +194,8 @@
> > compatible = "fsl,mpc5121-i2c", "fsl-i2c";
> > reg = <0x1700 0x20>;
> > interrupts = <9 0x8>;
> > + clocks = <&clks MPC512x_CLK_I2C>;
> > + clock-names = "per";
> > };
> >
> > i2c@1720 {
> > @@ -183,6 +204,8 @@
> > compatible = "fsl,mpc5121-i2c", "fsl-i2c";
> > reg = <0x1720 0x20>;
> > interrupts = <10 0x8>;
> > + clocks = <&clks MPC512x_CLK_I2C>;
> > + clock-names = "per";
> > };
> >
> > i2c@1740 {
> > @@ -191,6 +214,8 @@
> > compatible = "fsl,mpc5121-i2c", "fsl-i2c";
> > reg = <0x1740 0x20>;
> > interrupts = <11 0x8>;
> > + clocks = <&clks MPC512x_CLK_I2C>;
> > + clock-names = "per";
> > };
> >
> > i2ccontrol@1760 {
> > @@ -202,30 +227,46 @@
> > compatible = "fsl,mpc5121-axe";
> > reg = <0x2000 0x100>;
> > interrupts = <42 0x8>;
> > + clocks = <&clks MPC512x_CLK_AXE>;
> > + clock-names = "per";
> > };
> >
> > display@2100 {
> > compatible = "fsl,mpc5121-diu";
> > reg = <0x2100 0x100>;
> > interrupts = <64 0x8>;
> > + clocks = <&clks MPC512x_CLK_DIU>;
> > + clock-names = "per";
> > };
> >
> > can@2300 {
> > compatible = "fsl,mpc5121-mscan";
> > reg = <0x2300 0x80>;
> > interrupts = <90 0x8>;
> > + clocks = <&clks MPC512x_CLK_IPS>,
> > + <&clks MPC512x_CLK_SYS>,
> > + <&clks MPC512x_CLK_REF>,
> > + <&clks MPC512x_CLK_MSCAN2_MCLK>;
> > + clock-names = "ips", "sys", "ref", "mclk";
> > };
> >
> > can@2380 {
> > compatible = "fsl,mpc5121-mscan";
> > reg = <0x2380 0x80>;
> > interrupts = <91 0x8>;
> > + clocks = <&clks MPC512x_CLK_IPS>,
> > + <&clks MPC512x_CLK_SYS>,
> > + <&clks MPC512x_CLK_REF>,
> > + <&clks MPC512x_CLK_MSCAN3_MCLK>;
> > + clock-names = "ips", "sys", "ref", "mclk";
> > };
> >
> > viu@2400 {
> > compatible = "fsl,mpc5121-viu";
> > reg = <0x2400 0x400>;
> > interrupts = <67 0x8>;
> > + clocks = <&clks MPC512x_CLK_VIU>;
> > + clock-names = "per";
> > };
> >
> > mdio@2800 {
> > @@ -233,6 +274,8 @@
> > reg = <0x2800 0x800>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> > + clocks = <&clks MPC512x_CLK_FEC>;
> > + clock-names = "per";
> > };
> >
> > eth0: ethernet@2800 {
> > @@ -241,6 +284,8 @@
> > reg = <0x2800 0x800>;
> > local-mac-address = [ 00 00 00 00 00 00 ];
> > interrupts = <4 0x8>;
> > + clocks = <&clks MPC512x_CLK_FEC>;
> > + clock-names = "per";
> > };
> >
> > /* USB1 using external ULPI PHY */
> > @@ -252,6 +297,8 @@
> > interrupts = <43 0x8>;
> > dr_mode = "otg";
> > phy_type = "ulpi";
> > + clocks = <&clks MPC512x_CLK_USB1>;
> > + clock-names = "per";
> > };
> >
> > /* USB0 using internal UTMI PHY */
> > @@ -263,6 +310,8 @@
> > interrupts = <44 0x8>;
> > dr_mode = "otg";
> > phy_type = "utmi_wide";
> > + clocks = <&clks MPC512x_CLK_USB2>;
> > + clock-names = "per";
> > };
> >
> > /* IO control */
> > @@ -281,6 +330,8 @@
> > compatible = "fsl,mpc5121-pata";
> > reg = <0x10200 0x100>;
> > interrupts = <5 0x8>;
> > + clocks = <&clks MPC512x_CLK_PATA>;
> > + clock-names = "per";
> > };
> >
> > /* 512x PSCs are not 52xx PSC compatible */
> > @@ -292,6 +343,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC0_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC1 */
> > @@ -301,6 +354,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC1_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC2 */
> > @@ -310,6 +365,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC2_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC3 */
> > @@ -319,6 +376,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC3_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC4 */
> > @@ -328,6 +387,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC4_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC5 */
> > @@ -337,6 +398,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC5_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC6 */
> > @@ -346,6 +409,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC6_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC7 */
> > @@ -355,6 +420,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC7_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC8 */
> > @@ -364,6 +431,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC8_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC9 */
> > @@ -373,6 +442,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC9_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC10 */
> > @@ -382,6 +453,8 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC10_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > /* PSC11 */
> > @@ -391,12 +464,16 @@
> > interrupts = <40 0x8>;
> > fsl,rx-fifo-size = <16>;
> > fsl,tx-fifo-size = <16>;
> > + clocks = <&clks MPC512x_CLK_PSC11_MCLK>;
> > + clock-names = "mclk";
> > };
> >
> > pscfifo@11f00 {
> > compatible = "fsl,mpc5121-psc-fifo";
> > reg = <0x11f00 0x100>;
> > interrupts = <40 0x8>;
> > + clocks = <&clks MPC512x_CLK_PSC_FIFO>;
> > + clock-names = "per";
> > };
> >
> > dma0: dma@14000 {
> > @@ -414,6 +491,8 @@
> > #address-cells = <3>;
> > #size-cells = <2>;
> > #interrupt-cells = <1>;
> > + clocks = <&clks MPC512x_CLK_PCI>;
> > + clock-names = "per";
> >
> > reg = <0x80008500 0x100 /* internal registers */
> > 0x80008300 0x8>; /* config space access registers */
> > --
> > 1.7.10.4
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 17/31] clk: mpc512x: introduce COMMON_CLK for MPC512x
From: Gerhard Sittig @ 2013-08-03 15:03 UTC (permalink / raw)
To: Mike Turquette; +Cc: Anatolij Gustschin, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20130802234143.6450.39877@quantum>
[ this is about the MPC512x platform's clock driver, the core of
the series; trim the CC: list and only keep ARM (for clock) and
PPC lists and people ]
On Fri, Aug 02, 2013 at 16:41 -0700, Mike Turquette wrote:
>
> Quoting Gerhard Sittig (2013-07-22 05:14:44)
> > this change implements a clock driver for the MPC512x PowerPC platform
> > which follows the COMMON_CLK approach and uses common clock drivers
> > shared with other platforms
> >
> > this driver implements the publicly announced set of clocks (which can
> > get referenced by means of symbolic identifiers from the dt-bindings
> > header file), as well as generates additional 'struct clk' items where
> > the SoC hardware cannot easily get mapped to the common primitives of
> > the clock API, or requires "intermediate" clock nodes to represent
> > clocks that have both gates and dividers
> >
> > the previous PPC_CLOCK implementation is kept in place and remains in
> > parallel to the common clock implementation for test and comparison
> > during migration, a compile time option picks one of the two
> > alternatives (Kconfig switch, common clock used by default)
> >
> > some of the clock items get pre-enabled in the clock driver to not have
> > them automatically disabled by the underlying clock subsystem because of
> > their being unused -- this approach is desirable because
> > - some of the clocks are useful to have for diagnostics and information
> > despite their not getting claimed by any drivers (CPU, internal and
> > external RAM, internal busses, boot media)
> > - some of the clocks aren't claimed by their peripheral drivers yet,
> > either because of missing driver support or because device tree specs
> > aren't available yet (but the workarounds will get removed as the
> > drivers get adjusted and the device tree provides the clock specs)
> > - some help introduce support for and migrate to the common
> > infrastructure, while more appropriate support for specific hardware
> > constraints isn't available yet (remaining changes are strictly
> > internal to the clock driver and won't affect peripheral drivers)
> >
> > clkdev registration provides "alias names" for few clock items
> > - to not break those peripheral drivers which encode their component
> > index into the name that is used for clock lookup (UART, SPI, USB)
> > - to not break those drivers which use names for the clock lookup which
> > were encoded in the previous PPC_CLOCK implementation (NFC, VIU, CAN)
> > this workaround will get removed as these drivers get adjusted after
> > device tree based clock lookup has become available
> >
> > Signed-off-by: Gerhard Sittig <gsi@denx.de>
>
> Hi Gerhard,
>
> This looks OK to me. Do you want me to take it or will you keep the
> series together? Note that I took "clk: wrap I/O access for improved
> portability" into the clk tree already.
>
> Regards,
> Mike
Thank you for the feedback and for the interest! It's nice to
hear that you like the central part of the series. :)
There will be another version of the series (v4), addressing the
remaining feedback, further reducing pre-enable workarounds,
eliminating the need for "shared gates" by adding more 'ipg'
clocks for serial communication (UART, SPI, CAN). It's true that
the series has widened its scope after initial submission, but it
as well has much improved from the feedback.
This specific patch introduces an alternative clock driver which
is enabled by default, so it depends on the device tree data
being available before the code becomes operational. And the
common clock platform driver might break those peripheral drivers
which haven't received their clock API use cleanup yet (earlier
parts of the series).
So I'd like to keep the series together until it has passed
review. I will make sure that the series remains bisectable and
always keeps working when applied in sequence, and would prefer
to only start splitting it or staging parts of it later if
needed.
> > ---
> > arch/powerpc/platforms/512x/Kconfig | 14 +-
> > arch/powerpc/platforms/512x/Makefile | 4 +-
> > arch/powerpc/platforms/512x/clock-commonclk.c | 786 +++++++++++++++++++++++++
> > include/linux/clk-provider.h | 16 +
> > 4 files changed, 818 insertions(+), 2 deletions(-)
> > create mode 100644 arch/powerpc/platforms/512x/clock-commonclk.c
> >
> > diff --git a/arch/powerpc/platforms/512x/Kconfig b/arch/powerpc/platforms/512x/Kconfig
> > index fc9c1cb..c5fcdd0 100644
> > --- a/arch/powerpc/platforms/512x/Kconfig
> > +++ b/arch/powerpc/platforms/512x/Kconfig
> > @@ -1,9 +1,21 @@
> > +config MPC512x_COMMON_CLK
> > + bool "MPC512x platform uses COMMON_CLK"
> > + default y
> > + depends on PPC_MPC512x
> > + help
> > + This option is only here to support tests and comparison
> > + during development and migration. This option will get
> > + removed after the COMMON_CLK support for MPC512x has become
> > + fully operational and all drivers were adjusted to explicitly
> > + acquire their required clocks.
> > +
> > config PPC_MPC512x
> > bool "512x-based boards"
> > depends on 6xx
> > select FSL_SOC
> > select IPIC
> > - select PPC_CLOCK
> > + select PPC_CLOCK if !MPC512x_COMMON_CLK
> > + select COMMON_CLK if MPC512x_COMMON_CLK
> > select PPC_PCI_CHOICE
> > select FSL_PCI if PCI
> > select ARCH_WANT_OPTIONAL_GPIOLIB
> > diff --git a/arch/powerpc/platforms/512x/Makefile b/arch/powerpc/platforms/512x/Makefile
> > index 72fb934..1e05f9d 100644
> > --- a/arch/powerpc/platforms/512x/Makefile
> > +++ b/arch/powerpc/platforms/512x/Makefile
> > @@ -1,7 +1,9 @@
> > #
> > # Makefile for the Freescale PowerPC 512x linux kernel.
> > #
> > -obj-y += clock.o mpc512x_shared.o
> > +obj-$(CONFIG_PPC_CLOCK) += clock.o
> > +obj-$(CONFIG_COMMON_CLK) += clock-commonclk.o
> > +obj-y += mpc512x_shared.o
> > obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o mpc5121_ads_cpld.o
> > obj-$(CONFIG_MPC512x_GENERIC) += mpc512x_generic.o
> > obj-$(CONFIG_PDM360NG) += pdm360ng.o
> > diff --git a/arch/powerpc/platforms/512x/clock-commonclk.c b/arch/powerpc/platforms/512x/clock-commonclk.c
> > new file mode 100644
> > index 0000000..762ee85
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/512x/clock-commonclk.c
> > @@ -0,0 +1,786 @@
> > +/*
> > + * Copyright (C) 2013 DENX Software Engineering
> > + *
> > + * Gerhard Sittig, <gsi@denx.de>
> > + *
> > + * common clock driver support for the MPC512x platform
> > + *
> > + * This is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +
> > +#include <asm/mpc5121.h>
> > +#include <dt-bindings/clock/mpc512x-clock.h>
> > +
> > +#include "mpc512x.h" /* our public mpc5121_clk_init() API */
> > +
> > +/* helpers to keep the MCLK intermediates "somewhere" in our table */
> > +enum {
> > + MCLK_IDX_MUX0,
> > + MCLK_IDX_EN0,
> > + MCLK_IDX_DIV0,
> > + MCLK_IDX_MUX1,
> > + MCLK_MAX_IDX,
> > +};
> > +
> > +#define NR_PSCS 12
> > +#define NR_MSCANS 4
> > +#define NR_SPDIFS 1
> > +#define NR_MCLKS (NR_PSCS + NR_MSCANS + NR_SPDIFS)
> > +
> > +/* extend the public set of clocks by adding internal slots for management */
> > +enum {
> > + /* arrange for adjacent numbers after the public set */
> > + MPC512x_CLK_START_PRIVATE = MPC512x_CLK_LAST_PUBLIC,
> > + /* clocks which aren't announced to the public */
> > + MPC512x_CLK_DDR,
> > + MPC512x_CLK_MEM,
> > + MPC512x_CLK_IIM,
> > + MPC512x_CLK_SDHC_2,
> > + /* intermediates in div+gate combos or fractional dividers */
> > + MPC512x_CLK_DDR_UG,
> > + MPC512x_CLK_SDHC_x4,
> > + MPC512x_CLK_SDHC_UG,
> > + MPC512x_CLK_DIU_x4,
> > + MPC512x_CLK_DIU_UG,
> > + MPC512x_CLK_MBX_BUS_UG,
> > + MPC512x_CLK_MBX_UG,
> > + MPC512x_CLK_MBX_3D_UG,
> > + MPC512x_CLK_PCI_UG,
> > + MPC512x_CLK_NFC_UG,
> > + MPC512x_CLK_LPC_UG,
> > + MPC512x_CLK_SPDIF_TX_IN,
> > + /* intermediates for the mux+gate+div+mux MCLK generation */
> > + MPC512x_CLK_MCLKS_FIRST,
> > + MPC512x_CLK_MCLKS_LAST = MPC512x_CLK_MCLKS_FIRST
> > + + NR_MCLKS * MCLK_MAX_IDX,
> > + /* internal, symbolic spec for the number of slots */
> > + MPC512x_CLK_LAST_PRIVATE,
> > +};
> > +
> > +/* data required for the OF clock provider registration */
> > +static struct clk *clks[MPC512x_CLK_LAST_PRIVATE];
> > +static struct clk_onecell_data clk_data;
> > +
> > +/* CCM register access */
> > +static struct mpc512x_ccm __iomem *clkregs;
> > +static DEFINE_SPINLOCK(clklock);
> > +
> > +/* convenience wrappers around the common clk API */
> > +static inline struct clk *mpc512x_clk_fixed(const char *name, int rate)
> > +{
> > + return clk_register_fixed_rate(NULL, name, NULL, CLK_IS_ROOT, rate);
> > +}
> > +
> > +static inline struct clk *mpc512x_clk_factor(
> > + const char *name, const char *parent_name,
> > + int mul, int div)
> > +{
> > + int clkflags;
> > +
> > + clkflags = CLK_SET_RATE_PARENT;
> > + return clk_register_fixed_factor(NULL, name, parent_name, clkflags,
> > + mul, div);
> > +}
> > +
> > +static inline struct clk *mpc512x_clk_divider(
> > + const char *name, const char *parent_name, u8 clkflags,
> > + u32 __iomem *reg, u8 pos, u8 len, int divflags)
> > +{
> > + return clk_register_divider(NULL, name, parent_name, clkflags,
> > + reg, pos, len, divflags, &clklock);
> > +}
> > +
> > +static inline struct clk *mpc512x_clk_divtable(
> > + const char *name, const char *parent_name,
> > + u32 __iomem *reg, u8 pos, u8 len,
> > + const struct clk_div_table *divtab)
> > +{
> > + u8 divflags;
> > +
> > + divflags = 0;
> > + return clk_register_divider_table(NULL, name, parent_name, 0,
> > + reg, pos, len, divflags,
> > + divtab, &clklock);
> > +}
> > +
> > +static inline struct clk *mpc512x_clk_gated(
> > + const char *name, const char *parent_name,
> > + u32 __iomem *reg, u8 pos)
> > +{
> > + int clkflags;
> > +
> > + clkflags = CLK_SET_RATE_PARENT;
> > + return clk_register_gate(NULL, name, parent_name, clkflags,
> > + reg, pos, 0, &clklock);
> > +}
> > +
> > +static inline struct clk *mpc512x_clk_muxed(const char *name,
> > + const char **parent_names, int parent_count,
> > + u32 __iomem *reg, u8 pos, u8 len)
> > +{
> > + int clkflags;
> > + u8 muxflags;
> > +
> > + clkflags = CLK_SET_RATE_PARENT;
> > + muxflags = 0;
> > + return clk_register_mux(NULL, name,
> > + parent_names, parent_count, clkflags,
> > + reg, pos, len, muxflags, &clklock);
> > +}
> > +
> > +/* helper to isolate a bit field from a register */
> > +static inline int get_bit_field(uint32_t __iomem *reg, uint8_t pos, uint8_t len)
> > +{
> > + uint32_t val;
> > +
> > + val = in_be32(reg);
> > + val >>= pos;
> > + val &= (1 << len) - 1;
> > + return val;
> > +}
> > +
> > +/* get the SPMF and translate it into the "sys pll" multiplier */
> > +static int get_spmf_mult(void)
> > +{
> > + static int spmf_to_mult[] = {
> > + 68, 1, 12, 16, 20, 24, 28, 32,
> > + 36, 40, 44, 48, 52, 56, 60, 64,
> > + };
> > + int spmf;
> > +
> > + spmf = get_bit_field(&clkregs->spmr, 24, 4);
> > + return spmf_to_mult[spmf];
> > +}
> > +
> > +/*
> > + * get the SYS_DIV value and translate it into a divide factor
> > + *
> > + * values returned from here are a multiple of the real factor since the
> > + * divide ratio is fractional
> > + */
> > +static int get_sys_div_x2(void)
> > +{
> > + static int sysdiv_code_to_x2[] = {
> > + 4, 5, 6, 7, 8, 9, 10, 14,
> > + 12, 16, 18, 22, 20, 24, 26, 30,
> > + 28, 32, 34, 38, 36, 40, 42, 46,
> > + 44, 48, 50, 54, 52, 56, 58, 62,
> > + 60, 64, 66,
> > + };
> > + int divcode;
> > +
> > + divcode = get_bit_field(&clkregs->scfr2, 26, 6);
> > + return sysdiv_code_to_x2[divcode];
> > +}
> > +
> > +/*
> > + * get the CPMF value and translate it into a multiplier factor
> > + *
> > + * values returned from here are a multiple of the real factor since the
> > + * multiplier ratio is fractional
> > + */
> > +static int get_cpmf_mult_x2(void)
> > +{
> > + static int cpmf_to_mult[] = {
> > + 72, 2, 2, 3, 4, 5, 6, 7,
> > + };
> > + int cpmf;
> > +
> > + cpmf = get_bit_field(&clkregs->spmr, 16, 4);
> > + return cpmf_to_mult[cpmf];
> > +}
> > +
> > +/*
> > + * some of the clock dividers do scale in a linear way, yet not all of
> > + * their bit combinations are legal; use a divider table to get a
> > + * resulting set of applicable divider values
> > + */
> > +
> > +/* applies to the IPS_DIV, and PCI_DIV values */
> > +static struct clk_div_table divtab_2346[] = {
> > + { .val = 2, .div = 2, },
> > + { .val = 3, .div = 3, },
> > + { .val = 4, .div = 4, },
> > + { .val = 6, .div = 6, },
> > + { .div = 0, },
> > +};
> > +
> > +/* applies to the MBX_DIV, LPC_DIV, and NFC_DIV values */
> > +static struct clk_div_table divtab_1234[] = {
> > + { .val = 1, .div = 1, },
> > + { .val = 2, .div = 2, },
> > + { .val = 3, .div = 3, },
> > + { .val = 4, .div = 4, },
> > + { .div = 0, },
> > +};
> > +
> > +static int get_freq_from_dt(char *propname)
> > +{
> > + struct device_node *np;
> > + const unsigned int *prop;
> > + int val;
> > +
> > + val = 0;
> > + np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-immr");
> > + if (np) {
> > + prop = of_get_property(np, propname, NULL);
> > + if (prop)
> > + val = *prop;
> > + of_node_put(np);
> > + }
> > + return val;
> > +}
> > +
> > +static void mpc512x_clk_preset_data(void)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(clks); i++)
> > + clks[i] = ERR_PTR(-ENODEV);
> > +}
> > +
> > +/*
> > + * - receives the "bus frequency" from the caller (that's the IPS clock
> > + * rate, the historical source of clock information)
> > + * - fetches the system PLL multiplier and divider values as well as the
> > + * IPS divider value from hardware
> > + * - determines the REF clock rate either from the XTAL/OSC spec (if
> > + * there is a device tree node describing the oscillator) or from the
> > + * IPS bus clock (supported for backwards compatibility, such that
> > + * setups without XTAL/OSC specs keep working)
> > + * - creates the "ref" clock item in the clock tree, such that
> > + * subsequent code can create the remainder of the hierarchy (REF ->
> > + * SYS -> CSB -> IPS) from the REF clock rate and the returned mul/div
> > + * values
> > + */
> > +static void mpc512x_clk_setup_ref_clock(int bus_freq,
> > + int *sys_mul, int *sys_div, int *ips_div)
> > +{
> > + struct clk *osc_clk;
> > + int calc_freq;
> > +
> > + /* fetch mul/div factors from the hardware */
> > + *sys_mul = get_spmf_mult();
> > + *sys_mul *= 2; /* compensate for the fractional divider */
> > + *sys_div = get_sys_div_x2();
> > + *ips_div = get_bit_field(&clkregs->scfr1, 23, 3);
> > +
> > + /* lookup the oscillator node */
> > + osc_clk = clk_get(NULL, "osc");
> > + if (osc_clk) {
> > + /* descend REF directly from OSC, verify the IPS rate */
> > + clks[MPC512x_CLK_REF] = mpc512x_clk_factor("ref", "osc", 1, 1);
> > + calc_freq = clk_get_rate(clks[MPC512x_CLK_REF]);
> > + calc_freq *= *sys_mul;
> > + calc_freq /= *sys_div;
> > + calc_freq /= 2;
> > + calc_freq /= *ips_div;
> > + if (bus_freq && calc_freq != bus_freq)
> > + pr_warn("calc rate %d != OF spec %d\n",
> > + calc_freq, bus_freq);
> > + } else {
> > + /* calculate OSC rate and create REF from the freq value */
> > + calc_freq = bus_freq; /* start with IPS */
> > + calc_freq *= *ips_div; /* IPS -> CSB */
> > + calc_freq *= 2; /* CSB -> SYS */
> > + calc_freq *= *sys_div; /* SYS -> PLL out */
> > + calc_freq /= *sys_mul; /* PLL out -> REF == OSC */
> > + clks[MPC512x_CLK_REF] = mpc512x_clk_fixed("ref", calc_freq);
> > + }
> > +}
> > +
> > +/*
> > + * helper code for the MCLK subtree setup
> > + *
> > + * the overview in section 5.2.4 of the MPC5121e Reference Manual rev4
> > + * suggests that all instances of the "PSC clock generation" are equal,
> > + * and that one might re-use the PSC setup for MSCAN clock generation
> > + * (section 5.2.5) as well, at least the logic if not the data for
> > + * description
> > + *
> > + * the details (starting at page 5-20) show differences in the specific
> > + * inputs of the first mux stage ("can clk in", "spdif tx"), and the
> > + * factual non-availability of the second mux stage (it's present yet
> > + * only one input is valid)
> > + *
> > + * the MSCAN clock related registers (starting at page 5-35) all
> > + * reference "spdif clk" at the first mux stage and don't mention any
> > + * "can clk" at all, which somehow is unexpected
> > + *
> > + * TODO re-check the document, and clarify whether the RM is correct in
> > + * the overview or in the details, and whether the difference is a
> > + * clipboard induced error or results from chip revisions
> > + *
> > + * it turns out that the RM rev4 as of 2012-06 talks about "can" for the
> > + * PSCs while RM rev3 as of 2008-10 talks about "spdif", so I guess that
> > + * first a doc update is required which better reflects reality in the
> > + * SoC before the implementation should follow while no questions remain
> > + */
> > +
> > +/*
> > + * note that this declaration raises a checkpatch warning, but
> > + * it's the very data type which <linux/clk-provider.h> expects,
> > + * making this declaration pass checkpatch will break compilation
> > + */
> > +static const char *parent_names_mux0[] = {
> > + "sys", "ref", "psc-mclk-in", "spdif-tx",
> > +};
> > +
> > +enum mclk_type {
> > + MCLK_TYPE_PSC,
> > + MCLK_TYPE_MSCAN,
> > + MCLK_TYPE_SPDIF,
> > +};
> > +
> > +struct mclk_setup_data {
> > + enum mclk_type type;
> > + int comp_idx;
> > + bool has_mclk1;
> > + int bit_sccr1, bit_sccr2;
> > + const char *name_mux0;
> > + const char *name_en0;
> > + const char *name_div0;
> > + const char *parent_names_mux1[2];
> > + const char *name_mux1;
> > + const char *name_mclk;
> > +};
> > +
> > +#define MCLK_SETUP_DATA_PSC(id) { \
> > + MCLK_TYPE_PSC, id, \
> > + 0, 27 - id, -1, \
> > + "psc" #id "-mux0", \
> > + "psc" #id "-en0", \
> > + "psc" #id "_mclk_div", \
> > + { "psc" #id "_mclk_div", "dummy", }, \
> > + "psc" #id "_mclk_out", \
> > + "psc" #id "_mclk", \
> > +}
> > +
> > +#define MCLK_SETUP_DATA_MSCAN(id) { \
> > + MCLK_TYPE_MSCAN, id, \
> > + 0, -1, 25, \
> > + "mscan" #id "-mux0", \
> > + "mscan" #id "-en0", \
> > + "mscan" #id "_mclk_div", \
> > + { "mscan" #id "_mclk_div", "dummy", }, \
> > + "mscan" #id "_mclk_out", \
> > + "mscan" #id "_mclk", \
> > +}
> > +
> > +#define MCLK_SETUP_DATA_SPDIF { \
> > + MCLK_TYPE_SPDIF, 0, \
> > + 1, -1, 23, \
> > + "spdif-mux0", \
> > + "spdif-en0", \
> > + "spdif_mclk_div", \
> > + { "spdif_mclk_div", "spdif-rx", }, \
> > + "spdif_mclk_out", \
> > + "spdif_mclk", \
> > +}
> > +
> > +static struct mclk_setup_data mclk_psc_data[] = {
> > + MCLK_SETUP_DATA_PSC(0),
> > + MCLK_SETUP_DATA_PSC(1),
> > + MCLK_SETUP_DATA_PSC(2),
> > + MCLK_SETUP_DATA_PSC(3),
> > + MCLK_SETUP_DATA_PSC(4),
> > + MCLK_SETUP_DATA_PSC(5),
> > + MCLK_SETUP_DATA_PSC(6),
> > + MCLK_SETUP_DATA_PSC(7),
> > + MCLK_SETUP_DATA_PSC(8),
> > + MCLK_SETUP_DATA_PSC(9),
> > + MCLK_SETUP_DATA_PSC(10),
> > + MCLK_SETUP_DATA_PSC(11),
> > +};
> > +
> > +static struct mclk_setup_data mclk_mscan_data[] = {
> > + MCLK_SETUP_DATA_MSCAN(0),
> > + MCLK_SETUP_DATA_MSCAN(1),
> > + MCLK_SETUP_DATA_MSCAN(2),
> > + MCLK_SETUP_DATA_MSCAN(3),
> > +};
> > +
> > +static struct mclk_setup_data mclk_spdif_data[] = {
> > + MCLK_SETUP_DATA_SPDIF,
> > +};
> > +
> > +/* setup the MCLK clock subtree of an individual PSC/MSCAN/SPDIF */
> > +static void mpc512x_clk_setup_mclk(struct mclk_setup_data *entry)
> > +{
> > + size_t clks_idx_pub, clks_idx_int;
> > + u32 __iomem *mccr_reg; /* MCLK control register (mux, en, div) */
> > + u32 __iomem *sccr_reg; /* system clock control register (enable) */
> > + int sccr_bit;
> > + int div;
> > +
> > + /* derive a few parameters from the component type and index */
> > + switch (entry->type) {
> > + case MCLK_TYPE_PSC:
> > + clks_idx_pub = MPC512x_CLK_PSC0_MCLK + entry->comp_idx;
> > + clks_idx_int = MPC512x_CLK_MCLKS_FIRST
> > + + (entry->comp_idx) * MCLK_MAX_IDX;
> > + mccr_reg = &clkregs->psc_ccr[entry->comp_idx];
> > + break;
> > + case MCLK_TYPE_MSCAN:
> > + clks_idx_pub = MPC512x_CLK_MSCAN0_MCLK + entry->comp_idx;
> > + clks_idx_int = MPC512x_CLK_MCLKS_FIRST
> > + + (NR_PSCS + entry->comp_idx) * MCLK_MAX_IDX;
> > + mccr_reg = &clkregs->mscan_ccr[entry->comp_idx];
> > + break;
> > + case MCLK_TYPE_SPDIF:
> > + clks_idx_pub = MPC512x_CLK_SPDIF_MCLK;
> > + clks_idx_int = MPC512x_CLK_MCLKS_FIRST
> > + + (NR_PSCS + NR_MSCANS) * MCLK_MAX_IDX;
> > + mccr_reg = &clkregs->spccr;
> > + break;
> > + default:
> > + return;
> > + }
> > + if (entry->bit_sccr1 >= 0) {
> > + sccr_reg = &clkregs->sccr1;
> > + sccr_bit = entry->bit_sccr1;
> > + } else if (entry->bit_sccr2 >= 0) {
> > + sccr_reg = &clkregs->sccr2;
> > + sccr_bit = entry->bit_sccr2;
> > + } else {
> > + sccr_reg = NULL;
> > + }
> > +
> > + /*
> > + * this was grabbed from the PPC_CLOCK implementation, which
> > + * enforced a specific MCLK divider while the clock was gated
> > + * during setup (that's a documented hardware requirement)
> > + *
> > + * the PPC_CLOCK implementation might even have violated the
> > + * "MCLK <= IPS" constraint, the fixed divider value of 1
> > + * results in a divider of 2 and thus MCLK = SYS/2 which equals
> > + * CSB which is greater than IPS; the serial port setup may have
> > + * adjusted the divider which the clock setup might have left in
> > + * an undesirable state
> > + *
> > + * initial setup is:
> > + * - MCLK 0 from SYS
> > + * - MCLK DIV such to not exceed the IPS clock
> > + * - MCLK 0 enabled
> > + * - MCLK 1 from MCLK DIV
> > + */
> > + div = clk_get_rate(clks[MPC512x_CLK_SYS]);
> > + div /= clk_get_rate(clks[MPC512x_CLK_IPS]);
> > + out_be32(mccr_reg, (0 << 16));
> > + out_be32(mccr_reg, (0 << 16) | ((div - 1) << 17));
> > + out_be32(mccr_reg, (1 << 16) | ((div - 1) << 17));
> > +
> > + /*
> > + * create the 'struct clk' items of the MCLK's clock subtree
> > + *
> > + * note that by design we always create all nodes and won't take
> > + * shortcuts here, because
> > + * - the "internal" MCLK_DIV and MCLK_OUT signal in turn are
> > + * selectable inputs to the CFM while those who "actually use"
> > + * the PSC/MSCAN/SPDIF (serial drivers et al) need the MCLK
> > + * for their bitrate
> > + * - in the absence of "aliases" for clocks we need to create
> > + * individial 'struct clk' items for whatever might get
> > + * referenced or looked up, even if several of those items are
> > + * identical from the logical POV (their rate value)
> > + * - for easier future maintenance and for better reflection of
> > + * the SoC's documentation, it appears appropriate to generate
> > + * clock items even for those muxers which actually are NOPs
> > + * (those with two inputs of which one is reserved)
> > + */
> > + clks[clks_idx_int + MCLK_IDX_MUX0] = mpc512x_clk_muxed(
> > + entry->name_mux0,
> > + &parent_names_mux0[0], ARRAY_SIZE(parent_names_mux0),
> > + mccr_reg, 14, 2);
> > + clks[clks_idx_int + MCLK_IDX_EN0] = mpc512x_clk_gated(
> > + entry->name_en0, entry->name_mux0,
> > + mccr_reg, 16);
> > + clks[clks_idx_int + MCLK_IDX_DIV0] = mpc512x_clk_divider(
> > + entry->name_div0,
> > + entry->name_en0, CLK_SET_RATE_GATE,
> > + mccr_reg, 17, 15, 0);
> > + if (entry->has_mclk1) {
> > + clks[clks_idx_int + MCLK_IDX_MUX1] = mpc512x_clk_muxed(
> > + entry->name_mux1,
> > + &entry->parent_names_mux1[0],
> > + ARRAY_SIZE(entry->parent_names_mux1),
> > + mccr_reg, 7, 1);
> > + } else {
> > + clks[clks_idx_int + MCLK_IDX_MUX1] = mpc512x_clk_factor(
> > + entry->name_mux1, entry->parent_names_mux1[0],
> > + 1, 1);
> > + }
> > + if (sccr_reg) {
> > + clks[clks_idx_pub] = mpc512x_clk_gated(
> > + entry->name_mclk,
> > + entry->name_mux1, sccr_reg, sccr_bit);
> > + } else {
> > + clks[clks_idx_pub] = mpc512x_clk_factor(
> > + entry->name_mclk,
> > + entry->name_mux1, 1, 1);
> > + }
> > +
> > + /*
> > + * without this "clock device" registration, "simple" lookups in
> > + * the SPI master initialization and serial port setup will fail
> > + *
> > + * those drivers need to get adjusted to lookup their required
> > + * clocks from device tree specs, and device tree nodes need to
> > + * provide the clock specs, before this clkdev registration
> > + * becomes obsolete
> > + */
> > + clk_register_clkdev(clks[clks_idx_pub], entry->name_mclk, NULL);
> > +}
> > +
> > +static void mpc512x_clk_setup_mclks(struct mclk_setup_data *table, size_t count)
> > +{
> > + while (count-- > 0)
> > + mpc512x_clk_setup_mclk(table++);
> > +}
> > +
> > +static void mpc512x_clk_setup_clock_tree(int busfreq)
> > +{
> > + int sys_mul, sys_div, ips_div;
> > + int mul, div;
> > + int freq;
> > +
> > + /*
> > + * TODO
> > + * - consider whether to handle clocks which have both gates and
> > + * dividers via intermediates or by means of composites
> > + * - fractional dividers appear to not map well to composites
> > + * since they can be seen as a fixed multiplier and an
> > + * adjustable divider, while composites can only combine at
> > + * most one of a mux, div, and gate each into one 'struct clk'
> > + * item
> > + * - PSC/MSCAN/SPDIF clock generation OTOH already is very
> > + * specific and cannot get mapped to componsites (at least not
> > + * a single one, maybe two of them, but see the comment about
> > + * "intermediates are referenced from elsewhere, too")
> > + * - trim the list of auto-enabled clocks after drivers acquire
> > + * them correctly as needed
> > + */
> > +
> > + /* regardless of whether XTAL/OSC exists, have REF created */
> > + mpc512x_clk_setup_ref_clock(busfreq, &sys_mul, &sys_div, &ips_div);
> > +
> > + /* now setup the REF -> SYS -> CSB -> IPS hierarchy */
> > + clks[MPC512x_CLK_SYS] = mpc512x_clk_factor("sys", "ref",
> > + sys_mul, sys_div);
> > + clks[MPC512x_CLK_CSB] = mpc512x_clk_factor("csb", "sys", 1, 2);
> > + clks[MPC512x_CLK_IPS] = mpc512x_clk_divtable("ips", "csb",
> > + &clkregs->scfr1, 23, 3,
> > + divtab_2346);
> > +
> > + /* now setup anything below SYS and CSB and IPS */
> > + clks[MPC512x_CLK_DDR_UG] = mpc512x_clk_factor("ddr-ug", "sys", 1, 2);
> > + clks[MPC512x_CLK_SDHC_x4] = mpc512x_clk_factor("sdhc-x4", "csb", 4, 1);
> > + clks[MPC512x_CLK_SDHC_UG] = mpc512x_clk_divider("sdhc-ug", "sdhc-x4", 0,
> > + &clkregs->scfr2, 0, 8,
> > + CLK_DIVIDER_ONE_BASED);
> > + clks[MPC512x_CLK_DIU_x4] = mpc512x_clk_factor("diu-x4", "csb", 4, 1);
> > + clks[MPC512x_CLK_DIU_UG] = mpc512x_clk_divider("diu-ug", "diu-x4", 0,
> > + &clkregs->scfr1, 0, 8,
> > + CLK_DIVIDER_ONE_BASED);
> > +
> > + /*
> > + * the "power architecture PLL" was setup from data which was
> > + * sampled from the reset config word, at this point in time the
> > + * configuration can be considered fixed and read only (i.e. no
> > + * longer adjustable, or no longer in need of adjustment), which
> > + * is why we don't register a PLL here but assume fixed factors
> > + */
> > + mul = get_cpmf_mult_x2();
> > + div = 2; /* compensate for the fractional factor */
> > + clks[MPC512x_CLK_E300] = mpc512x_clk_factor("e300", "csb", mul, div);
> > +
> > + clks[MPC512x_CLK_MBX_BUS_UG] = mpc512x_clk_factor("mbx-bus-ug", "csb",
> > + 1, 2);
> > + clks[MPC512x_CLK_MBX_UG] = mpc512x_clk_divtable("mbx-ug", "mbx-bus-ug",
> > + &clkregs->scfr1, 14, 3,
> > + divtab_1234);
> > + clks[MPC512x_CLK_MBX_3D_UG] = mpc512x_clk_factor("mbx-3d-ug", "mbx-ug",
> > + 1, 1);
> > + clks[MPC512x_CLK_PCI_UG] = mpc512x_clk_divtable("pci-ug", "csb",
> > + &clkregs->scfr1, 20, 3,
> > + divtab_2346);
> > + clks[MPC512x_CLK_NFC_UG] = mpc512x_clk_divtable("nfc-ug", "ips",
> > + &clkregs->scfr1, 8, 3,
> > + divtab_1234);
> > + clks[MPC512x_CLK_LPC_UG] = mpc512x_clk_divtable("lpc-ug", "ips",
> > + &clkregs->scfr1, 11, 3,
> > + divtab_1234);
> > +
> > + clks[MPC512x_CLK_LPC] = mpc512x_clk_gated("lpc", "lpc-ug",
> > + &clkregs->sccr1, 30);
> > + clks[MPC512x_CLK_NFC] = mpc512x_clk_gated("nfc", "nfc-ug",
> > + &clkregs->sccr1, 29);
> > + clks[MPC512x_CLK_PATA] = mpc512x_clk_gated("pata", "ips",
> > + &clkregs->sccr1, 28);
> > + mpc512x_clk_setup_mclks(mclk_psc_data, ARRAY_SIZE(mclk_psc_data));
> > + clks[MPC512x_CLK_PSC_FIFO] = mpc512x_clk_gated("psc-fifo", "ips",
> > + &clkregs->sccr1, 15);
> > + clks[MPC512x_CLK_SATA] = mpc512x_clk_gated("sata", "ips",
> > + &clkregs->sccr1, 14);
> > + clks[MPC512x_CLK_FEC] = mpc512x_clk_gated("fec", "ips",
> > + &clkregs->sccr1, 13);
> > + clks[MPC512x_CLK_PCI] = mpc512x_clk_gated("pci", "pci-ug",
> > + &clkregs->sccr1, 11);
> > + clks[MPC512x_CLK_DDR] = mpc512x_clk_gated("ddr", "ddr-ug",
> > + &clkregs->sccr1, 10);
> > +
> > + clks[MPC512x_CLK_DIU] = mpc512x_clk_gated("diu", "diu-ug",
> > + &clkregs->sccr2, 31);
> > + clks[MPC512x_CLK_AXE] = mpc512x_clk_gated("axe", "csb",
> > + &clkregs->sccr2, 30);
> > + clks[MPC512x_CLK_MEM] = mpc512x_clk_gated("mem", "ips",
> > + &clkregs->sccr2, 29);
> > + clks[MPC512x_CLK_USB1] = mpc512x_clk_gated("usb1", "csb",
> > + &clkregs->sccr2, 28);
> > + clks[MPC512x_CLK_USB2] = mpc512x_clk_gated("usb2", "csb",
> > + &clkregs->sccr2, 27);
> > + clks[MPC512x_CLK_I2C] = mpc512x_clk_gated("i2c", "ips",
> > + &clkregs->sccr2, 26);
> > + mpc512x_clk_setup_mclks(mclk_mscan_data, ARRAY_SIZE(mclk_mscan_data));
> > + clks[MPC512x_CLK_SDHC] = mpc512x_clk_gated("sdhc", "sdhc-ug",
> > + &clkregs->sccr2, 24);
> > + mpc512x_clk_setup_mclks(mclk_spdif_data, ARRAY_SIZE(mclk_spdif_data));
> > + clks[MPC512x_CLK_MBX_BUS] = mpc512x_clk_gated("mbx-bus", "mbx-bus-ug",
> > + &clkregs->sccr2, 22);
> > + clks[MPC512x_CLK_MBX] = mpc512x_clk_gated("mbx", "mbx-ug",
> > + &clkregs->sccr2, 21);
> > + clks[MPC512x_CLK_MBX_3D] = mpc512x_clk_gated("mbx-3d", "mbx-3d-ug",
> > + &clkregs->sccr2, 20);
> > + clks[MPC512x_CLK_IIM] = mpc512x_clk_gated("iim", "csb",
> > + &clkregs->sccr2, 19);
> > + clks[MPC512x_CLK_VIU] = mpc512x_clk_gated("viu", "csb",
> > + &clkregs->sccr2, 18);
> > + clks[MPC512x_CLK_SDHC_2] = mpc512x_clk_gated("sdhc-2", "sdhc-ug",
> > + &clkregs->sccr2, 17);
> > +
> > + /*
> > + * externally provided clocks (when implemented in hardware,
> > + * device tree may specify values which otherwise were unknown)
> > + */
> > + freq = get_freq_from_dt("psc_mclk_in");
> > + if (!freq)
> > + freq = 25000000;
> > + clks[MPC512x_CLK_PSC_MCLK_IN] = mpc512x_clk_fixed("psc_mclk_in", freq);
> > + freq = get_freq_from_dt("spdif_tx_in");
> > + clks[MPC512x_CLK_SPDIF_TX_IN] = mpc512x_clk_fixed("spdif_tx_in", freq);
> > + freq = get_freq_from_dt("spdif_rx_in");
> > + clks[MPC512x_CLK_SPDIF_TX_IN] = mpc512x_clk_fixed("spdif_rx_in", freq);
> > +
> > + /* fixed frequency for AC97, always 24.567MHz */
> > + clks[MPC512x_CLK_AC97] = mpc512x_clk_fixed("ac97", 24567000);
> > +
> > + /* clkdev registration for compatibility reasons */
> > + clk_register_clkdev(clks[MPC512x_CLK_REF], "ref_clk", NULL);
> > + clk_register_clkdev(clks[MPC512x_CLK_SYS], "sys_clk", NULL);
> > + clk_register_clkdev(clks[MPC512x_CLK_VIU], "viu_clk", NULL);
> > + clk_register_clkdev(clks[MPC512x_CLK_NFC], "nfc_clk", NULL);
> > + clk_register_clkdev(clks[MPC512x_CLK_USB1], "usb1_clk", NULL);
> > + clk_register_clkdev(clks[MPC512x_CLK_USB2], "usb2_clk", NULL);
> > +
> > + pr_debug("clock tree setup complete\n");
> > + freq = clk_get_rate(clks[MPC512x_CLK_E300]);
> > + pr_debug("derived PPC freq [%d]\n", freq);
> > + freq = clk_get_rate(clks[MPC512x_CLK_IPS]);
> > + pr_debug("derived IPS freq [%d]\n", freq);
> > + freq = clk_get_rate(clks[MPC512x_CLK_LPC]);
> > + pr_debug("derived LPC freq [%d]\n", freq);
> > +
> > + /* enable some of the clocks here unconditionally because ... */
> > + pr_debug("automatically enabling some clocks\n");
> > + /* some are essential yet never get claimed by any driver */
> > + clk_prepare_enable(clks[MPC512x_CLK_DUMMY]);
> > + clk_prepare_enable(clks[MPC512x_CLK_E300]); /* PowerPC CPU */
> > + clk_prepare_enable(clks[MPC512x_CLK_DDR]); /* DRAM */
> > + clk_prepare_enable(clks[MPC512x_CLK_MEM]); /* SRAM */
> > + clk_prepare_enable(clks[MPC512x_CLK_IPS]); /* SoC periph */
> > + clk_prepare_enable(clks[MPC512x_CLK_LPC]); /* boot media */
> > + /* some are required yet no dependencies were declared */
> > + clk_prepare_enable(clks[MPC512x_CLK_PSC_FIFO]);
> > + /* some are not yet acquired by their respective drivers */
> > + clk_prepare_enable(clks[MPC512x_CLK_PSC3_MCLK]);/* serial console */
> > + clk_prepare_enable(clks[MPC512x_CLK_FEC]); /* network, NFS */
> > + clk_prepare_enable(clks[MPC512x_CLK_DIU]); /* display */
> > + clk_prepare_enable(clks[MPC512x_CLK_I2C]);
> > + /*
> > + * some have their individual clock subtree with separate clock
> > + * items and their individual enable counters, yet share a
> > + * common gate (refer to the same register location) while the
> > + * common clock driver code is not aware of the fact and the
> > + * platform's code doesn't provide specific support either
> > + *
> > + * what might happen is that e.g. enabling two MSCAN clock items
> > + * and disabling one of them will disable the common gate and
> > + * thus break the other MSCAN clock as well
> > + */
> > + clk_prepare_enable(clks[MPC512x_CLK_MSCAN0_MCLK]);
> > + clk_prepare_enable(clks[MPC512x_CLK_MSCAN1_MCLK]);
> > + clk_prepare_enable(clks[MPC512x_CLK_MSCAN2_MCLK]);
> > + clk_prepare_enable(clks[MPC512x_CLK_MSCAN3_MCLK]);
> > +}
> > +
> > +/*
> > + * registers the set of public clocks (those listed in the dt-bindings/
> > + * header file) for OF lookups, keeps the intermediates private to us
> > + */
> > +static void mpc5121_clk_register_of_provider(struct device_node *np)
> > +{
> > + clk_data.clks = clks;
> > + clk_data.clk_num = MPC512x_CLK_LAST_PUBLIC + 1; /* _not_ ARRAY_SIZE() */
> > + of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > +}
> > +
> > +int __init mpc5121_clk_init(void)
> > +{
> > + struct device_node *clk_np;
> > + int busfreq;
> > +
> > + /* map the clock control registers */
> > + clk_np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-clock");
> > + if (!clk_np)
> > + return -ENODEV;
> > + clkregs = of_iomap(clk_np, 0);
> > + WARN_ON(!clkregs);
> > +
> > + /* invalidate all not yet registered clock slots */
> > + mpc512x_clk_preset_data();
> > +
> > + /*
> > + * have the device tree scanned for "fixed-clock" nodes (which
> > + * includes the oscillator node if the board's DT provides one)
> > + */
> > + of_clk_init(NULL);
> > +
> > + /*
> > + * add a dummy clock for those situations where a clock spec is
> > + * required yet no real clock is involved
> > + */
> > + clks[MPC512x_CLK_DUMMY] = mpc512x_clk_fixed("dummy", 0);
> > +
> > + /*
> > + * have all the real nodes in the clock tree populated from REF
> > + * down to all leaves, either starting from the OSC node or from
> > + * a REF root that was created from the IPS bus clock input
> > + */
> > + busfreq = get_freq_from_dt("bus-frequency");
> > + mpc512x_clk_setup_clock_tree(busfreq);
> > +
> > + /* register as an OF clock provider */
> > + mpc5121_clk_register_of_provider(clk_np);
> > +
> > + return 0;
> > +}
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index c4f7799..7f8fc64 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -497,6 +497,20 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
> > * for improved portability across platforms
> > */
> >
> > +#if IS_ENABLED(CONFIG_PPC)
> > +
> > +static inline u32 clk_readl(u32 __iomem *reg)
> > +{
> > + return ioread32be(reg);
> > +}
> > +
> > +static inline void clk_writel(u32 val, u32 __iomem *reg)
> > +{
> > + iowrite32be(val, reg);
> > +}
> > +
> > +#else /* platform dependent I/O accessors */
> > +
> > static inline u32 clk_readl(u32 __iomem *reg)
> > {
> > return readl(reg);
> > @@ -507,5 +521,7 @@ static inline void clk_writel(u32 val, u32 __iomem *reg)
> > writel(val, reg);
> > }
> >
> > +#endif /* platform dependent I/O accessors */
> > +
> > #endif /* CONFIG_COMMON_CLK */
> > #endif /* CLK_PROVIDER_H */
> > --
> > 1.7.10.4
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 17/31] clk: mpc512x: introduce COMMON_CLK for MPC512x
From: Gerhard Sittig @ 2013-08-03 14:39 UTC (permalink / raw)
To: Mike Turquette; +Cc: Anatolij Gustschin, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20130802233009.6450.21915@quantum>
[ we are strictly talking about clocks and source code again,
I have trimmed the CC: list to not spam the device tree ML or
subsystem maintainers ]
On Fri, Aug 02, 2013 at 16:30 -0700, Mike Turquette wrote:
>
> Quoting Gerhard Sittig (2013-07-23 06:14:06)
> > [ summary: "shared gate" support desirable? approach acceptable? ]
> >
> > On Mon, Jul 22, 2013 at 14:14 +0200, Gerhard Sittig wrote:
> > >
> > > this change implements a clock driver for the MPC512x PowerPC platform
> > > which follows the COMMON_CLK approach and uses common clock drivers
> > > shared with other platforms
> > >
> > > [ ... ]
> > >
> > > some of the clock items get pre-enabled in the clock driver to not have
> > > them automatically disabled by the underlying clock subsystem because of
> > > their being unused -- this approach is desirable because
> > > [ ... ]
> > > - some help introduce support for and migrate to the common
> > > infrastructure, while more appropriate support for specific hardware
> > > constraints isn't available yet (remaining changes are strictly
> > > internal to the clock driver and won't affect peripheral drivers)
> >
> > This remark was related to the CAN clocks of the MPC512x SoC.
>
> Gerhard,
>
> Thanks for the patch (way far down below here). I'll check into it to
> see if that implementation looks OK. It would be helpful if another
> platform with shared gates could weigh in on whether the implementation
> works for them.
>
> Still, a shared gate solution is not a prerequisite for this series,
> correct?
Well, the recent CAN driver related discussion suggested that I
had a mental misconception there. The need for "shared gates"
was felt because of mixing up unrelated paths in the clock tree.
But the MCLK subtree is for bitrate generation, while the BDLC
gate is for register access into the peripheral controller.
Currently I'm investigating how I can cleanly tell those
individual aspects apart. Telling the gate for register access
(in ARM speak often referred to as 'ipg') from the bitrate
generation (the 'per' clock, or 'mclk' here) seems so much more
appropriate.
After clean separation, and more testing to make sure nothing
gets broken throughout the series, there will be v4.
So "shared gate" support might have become obsolete for the
MPC512x platform. But if others need it, the outlined approach
(patch below) may be viable. The change to the common code is
minimal. The use in the platform's clock driver was kind of
overengineered for the case of exactly one such gate, but this
immediately makes it a working approach for several gates, if
others need it.
I'll trim the motivation and just leave the suggested approach
for "shared gates" here. Feel free to drop it or to only
resurrect it as the need may re-arise later. So far nobody
appears to have felt the need up to now ...
> > [ ... ]
> >
> > The question now is how to correctly support the situation where
> > a gate is shared between subtrees yet isn't really part of any
> > path within the subtrees. I really cannot find a single spot
> > where to introduce the gate such that it's not duplicated.
> >
> > The appropriate solution would not be to pre-enable those clocks,
> > but to either introduce another gate clock type which supports a
> > shared reference, or to add support for the shared reference to
> > the existing gate code.
> >
> >
> > I'd rather not duplicate most or all of the code of clk-gate.c,
> > instead I looked into how to add "shared gate" support to the
> > existing driver.
> >
> > My question is whether the approach is acceptable. It adds
> > minimal overhead and shall be OK for the enable/disable path from
> > a technical POV. And it doesn't feel like too much of a stretch.
> > But there may be non-technical reasons to reject the approach.
> > I'd like to learn whether to follow that path before preparing
> > another version of the patch series.
> >
> > The diffs were taken with the '-w -b' options to demonstrate
> > their essence and not drown it in whitespace changes. The
> > implementation assumes that the caller which registers the gate
> > (the platform's clock driver) provides both the counter cell and
> > the lock. And that all gates with a "shared use counter" use the
> > same lock (which is satisfied as they all get registered from the
> > same spot in the platform's clock driver).
> >
> > The CLK_IGNORE_UNUSED flag addresses a different problem. The
> > SoC has four MSCAN components, while two of them are enabled in
> > the device tree (the other two are present but disabled). So
> > during probe two of the clocks get enabled. After probe all
> > unused clocks automatically get disabled (that's another two).
> > So the "shared use counter" drops to zero although components are
> > in use, because "disable, it's unused" isn't told from "disable
> > after enable, regular use". The flag would become obsolete if
> > the common gate logic would implement a separate disable_unused()
> > routine, but I guess this isn't necessary and the use of the flag
> > is appropriate.
> >
> > That the example use creates a field for just one counter is to
> > better demonstrate the use and potential extension as need
> > arises. Reducing this to a mere integer variable would be a
> > micro optimization.
> >
> >
> > The extension of the existing clk_gate implementation:
> >
> > --- a/drivers/clk/clk-gate.c
> > +++ b/drivers/clk/clk-gate.c
> > @@ -46,6 +46,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
> > struct clk_gate *gate = to_clk_gate(hw);
> > int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
> > unsigned long flags = 0;
> > + int need_reg_access;
> > u32 reg;
> >
> > set ^= enable;
> > @@ -53,6 +54,20 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
> > if (gate->lock)
> > spin_lock_irqsave(gate->lock, flags);
> >
> > + /*
> > + * if a "shared use counter" was specified, keep track of enable
> > + * and disable calls and only access hardware registers upon the
> > + * very first enable or very last disable call
> > + */
> > + if (!gate->share_count) {
> > + need_reg_access = 1;
> > + } else if (enable) {
> > + need_reg_access = (*gate->share_count)++ == 0;
> > + } else {
> > + need_reg_access = --(*gate->share_count) == 0;
> > + }
> > +
> > + if (need_reg_access) {
> > if (gate->flags & CLK_GATE_HIWORD_MASK) {
> > reg = BIT(gate->bit_idx + 16);
> > if (set)
> > @@ -67,6 +82,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
> > }
> >
> > clk_writel(reg, gate->reg);
> > + }
> >
> > if (gate->lock)
> > spin_unlock_irqrestore(gate->lock, flags);
> > @@ -118,10 +134,11 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
> > * @clk_gate_flags: gate-specific flags for this clock
> > * @lock: shared register lock for this clock
> > */
> > -struct clk *clk_register_gate(struct device *dev, const char *name,
> > +struct clk *clk_register_gate_shared(struct device *dev, const char *name,
> > const char *parent_name, unsigned long flags,
> > void __iomem *reg, u8 bit_idx,
> > - u8 clk_gate_flags, spinlock_t *lock)
> > + u8 clk_gate_flags, spinlock_t *lock,
> > + int *share_count)
> > {
> > struct clk_gate *gate;
> > struct clk *clk;
> > @@ -152,6 +169,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
> > gate->bit_idx = bit_idx;
> > gate->flags = clk_gate_flags;
> > gate->lock = lock;
> > + gate->share_count = share_count;
> > gate->hw.init = &init;
> >
> > clk = clk_register(dev, &gate->hw);
> > @@ -161,3 +179,14 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
> >
> > return clk;
> > }
> > +
> > +struct clk *clk_register_gate(struct device *dev, const char *name,
> > + const char *parent_name, unsigned long flags,
> > + void __iomem *reg, u8 bit_idx,
> > + u8 clk_gate_flags, spinlock_t *lock)
> > +{
> > +
> > + return clk_register_gate_shared(dev, name, parent_name, flags,
> > + reg, bit_idx, clk_gate_flags,
> > + lock, NULL);
> > +}
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -222,6 +222,7 @@ struct clk_gate {
> > u8 bit_idx;
> > u8 flags;
> > spinlock_t *lock;
> > + int *share_count;
> > };
> >
> > #define CLK_GATE_SET_TO_DISABLE BIT(0)
> > @@ -232,6 +233,11 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
> > const char *parent_name, unsigned long flags,
> > void __iomem *reg, u8 bit_idx,
> > u8 clk_gate_flags, spinlock_t *lock);
> > +struct clk *clk_register_gate_shared(struct device *dev, const char *name,
> > + const char *parent_name, unsigned long flags,
> > + void __iomem *reg, u8 bit_idx,
> > + u8 clk_gate_flags, spinlock_t *lock,
> > + int *share_count);
> >
> > struct clk_div_table {
> > unsigned int val;
> >
> >
> > How to use these shared gates:
> >
> > --- a/arch/powerpc/platforms/512x/clock-commonclk.c
> > +++ b/arch/powerpc/platforms/512x/clock-commonclk.c
> > @@ -123,6 +123,39 @@ static inline struct clk *mpc512x_clk_gated(
> > reg, pos, 0, &clklock);
> > }
> >
> > +enum mpc512x_clk_shared_gate_id_t {
> > + MPC512x_CLK_SHARED_GATE_MSCAN,
> > + MPC512x_CLK_SHARED_GATE_MAX,
> > +};
> > +
> > +static int mpc512x_clk_gate_counters[MPC512x_CLK_SHARED_GATE_MAX];
> > +
> > +/*
> > + * implementor's note: since clk_gate items don't implement a separate
> > + * .disable_unused() callback, their .disable() routine gets called and
> > + * "disable the clock as we can't see it's in use" cannot be told from
> > + * "regular disable, count these events please"
> > + *
> > + * passing the CLK_IGNORE_UNUSED flag upon clock creation will suppress
> > + * the "disable, unused" call, so use counts won't get unbalanced, the
> > + * clock either never got enabled and thus need not get disabled, or
> > + * part of the hardware got enabled while disabling the other part isn't
> > + * wanted
> > + */
> > +static inline struct clk *mpc512x_clk_gated_shared(
> > + const char *name, const char *parent_name,
> > + u32 __iomem *reg, u8 pos,
> > + enum mpc512x_clk_shared_gate_id_t share_id)
> > +{
> > + int clkflags;
> > +
> > + clkflags = CLK_SET_RATE_PARENT;
> > + clkflags |= CLK_IGNORE_UNUSED;
> > + return clk_register_gate_shared(NULL, name, parent_name, clkflags,
> > + reg, pos, 0, &clklock,
> > + &mpc512x_clk_gate_counters[share_id]);
> > +}
> > +
> > static inline struct clk *mpc512x_clk_muxed(const char *name,
> > const char **parent_names, int parent_count,
> > u32 __iomem *reg, u8 pos, u8 len)
> > @@ -520,9 +553,16 @@ static void mpc512x_clk_setup_mclk(struct mclk_setup_data *entry)
> > 1, 1);
> > }
> > if (sccr_reg) {
> > + if (entry->type == MCLK_TYPE_MSCAN) {
> > + clks[clks_idx_pub] = mpc512x_clk_gated_shared(
> > + entry->name_mclk,
> > + entry->name_mux1, sccr_reg, sccr_bit,
> > + MPC512x_CLK_SHARED_GATE_MSCAN);
> > + } else {
> > clks[clks_idx_pub] = mpc512x_clk_gated(
> > entry->name_mclk,
> > entry->name_mux1, sccr_reg, sccr_bit);
> > + }
> > } else {
> > clks[clks_idx_pub] = mpc512x_clk_factor(
> > entry->name_mclk,
> >
> > Local tests have shown that the extension solves the problem of
> > how to satisfy the SoC's constraints on the MPC512x platform.
> > The MSCAN clocks no longer need to get pre-enabled, instead they
> > get setup and enabled only as the mscan(4) driver probes devices
> > according to how it was instructed (device tree nodes).
> >
> > What do you think? Is the "shared gate" support in the common
> > logic appropriate? I'd rather not duplicate all of this code
> > just to introduce the specific gate I need, while most of the
> > logic is identical to the existing gate implementation. The
> > desire isn't to override the gate's operations, but to wrap them
> > and to consult a counter in addition, while the register access
> > still applies.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: windfarm_fcu_controls: cpu-pump-0 <HW FAULT>
From: Andreas Schwab @ 2013-08-03 14:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <87r4eahoka.fsf__2165.9838072539$1375537818$gmane$org@igel.home>
But then, MacOS is happy and the hardware test didn't find anything
either...
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH v3 15/31] dts: mpc512x: introduce dt-bindings/clock/ header
From: Gerhard Sittig @ 2013-08-03 14:19 UTC (permalink / raw)
To: Mike Turquette
Cc: devicetree, Detlev Zundel, Wolfram Sang, David Woodhouse,
Greg Kroah-Hartman, Rob Herring, Mark Brown, Marc Kleine-Budde,
Wolfgang Grandegger, Anatolij Gustschin, linuxppc-dev,
linux-arm-kernel, Mauro Carvalho Chehab
In-Reply-To: <20130802224308.6450.55171@quantum>
On Fri, Aug 02, 2013 at 15:43 -0700, Mike Turquette wrote:
>
> Quoting Gerhard Sittig (2013-07-22 05:14:42)
> > introduce a dt-bindings/ header file for MPC512x clocks,
> > providing symbolic identifiers for those SoC clocks which
> > clients will reference from their device tree nodes
> >
> > Signed-off-by: Gerhard Sittig <gsi@denx.de>
> > ---
> > include/dt-bindings/clock/mpc512x-clock.h | 59 +++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> > create mode 100644 include/dt-bindings/clock/mpc512x-clock.h
> >
> > diff --git a/include/dt-bindings/clock/mpc512x-clock.h b/include/dt-bindings/clock/mpc512x-clock.h
> > new file mode 100644
> > index 0000000..46c560e
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/mpc512x-clock.h
> > @@ -0,0 +1,59 @@
> > +/*
> > + * This header provides constants for MPC512x clock specs in DT bindings.
> > + *
> > + * Unfortunately the clock number declaration cannot be an enum but
> > + * needs to be a list of #define directives since when referenced from
> > + * within DTS files they need to get resolved "at compile time".
>
> Above comment is not really necessary. Otherwise,
>
> Reviewed-by: Mike Turquette <mturquette@linaro.org>
OK, the next update of the series will remove this comment.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_CLOCK_MPC512x_CLOCK_H
> > +#define _DT_BINDINGS_CLOCK_MPC512x_CLOCK_H
> > +
> > +#define MPC512x_CLK_DUMMY 0
> > +#define MPC512x_CLK_REF 1
> > +#define MPC512x_CLK_SYS 2
> > +#define MPC512x_CLK_DIU 3
> > +#define MPC512x_CLK_VIU 4
> > +#define MPC512x_CLK_CSB 5
> > +#define MPC512x_CLK_E300 6
> > +#define MPC512x_CLK_IPS 7
> > +#define MPC512x_CLK_FEC 8
> > +#define MPC512x_CLK_SATA 9
> > +#define MPC512x_CLK_PATA 10
> > +#define MPC512x_CLK_NFC 11
> > +#define MPC512x_CLK_LPC 12
> > +#define MPC512x_CLK_MBX_BUS 13
> > +#define MPC512x_CLK_MBX 14
> > +#define MPC512x_CLK_MBX_3D 15
> > +#define MPC512x_CLK_AXE 16
> > +#define MPC512x_CLK_USB1 17
> > +#define MPC512x_CLK_USB2 18
> > +#define MPC512x_CLK_I2C 19
> > +#define MPC512x_CLK_MSCAN0_MCLK 20
> > +#define MPC512x_CLK_MSCAN1_MCLK 21
> > +#define MPC512x_CLK_MSCAN2_MCLK 22
> > +#define MPC512x_CLK_MSCAN3_MCLK 23
> > +#define MPC512x_CLK_SDHC 24
> > +#define MPC512x_CLK_PCI 25
> > +#define MPC512x_CLK_PSC_MCLK_IN 26
> > +#define MPC512x_CLK_SPDIF_TX 27
> > +#define MPC512x_CLK_SPDIF_RX 28
> > +#define MPC512x_CLK_SPDIF_MCLK 29
> > +#define MPC512x_CLK_AC97 30
> > +#define MPC512x_CLK_PSC0_MCLK 31
> > +#define MPC512x_CLK_PSC1_MCLK 32
> > +#define MPC512x_CLK_PSC2_MCLK 33
> > +#define MPC512x_CLK_PSC3_MCLK 34
> > +#define MPC512x_CLK_PSC4_MCLK 35
> > +#define MPC512x_CLK_PSC5_MCLK 36
> > +#define MPC512x_CLK_PSC6_MCLK 37
> > +#define MPC512x_CLK_PSC7_MCLK 38
> > +#define MPC512x_CLK_PSC8_MCLK 39
> > +#define MPC512x_CLK_PSC9_MCLK 40
> > +#define MPC512x_CLK_PSC10_MCLK 41
> > +#define MPC512x_CLK_PSC11_MCLK 42
> > +#define MPC512x_CLK_PSC_FIFO 43
> > +
> > +#define MPC512x_CLK_LAST_PUBLIC 43
> > +
> > +#endif
> > --
> > 1.7.10.4
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 13/31] clk: wrap I/O access for improved portability
From: Gerhard Sittig @ 2013-08-03 14:08 UTC (permalink / raw)
To: Mike Turquette; +Cc: Anatolij Gustschin, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20130802223000.6450.97817@quantum>
[ trimming the CC: list for this strictly clock related and
source code adjusting change, to not spam the device tree ML or
other subsystem maintainers, just keeping ARM (for clock) and
PPC lists and people in the loop ]
On Fri, Aug 02, 2013 at 15:30 -0700, Mike Turquette wrote:
>
> Quoting Gerhard Sittig (2013-07-22 05:14:40)
> > the common clock drivers were motivated/initiated by ARM development
> > and apparently assume little endian peripherals
> >
> > wrap register/peripherals access in the common code (div, gate, mux)
> > in preparation of adding COMMON_CLK support for other platforms
> >
> > Signed-off-by: Gerhard Sittig <gsi@denx.de>
>
> I've taken this into clk-next for testing. regmap deserves investigation
> but I don't think your series should be blocked on that. We can always
> overhaul the basic clock primitives with regmap support later on if that
> makes sense.
>
> Regards,
> Mike
That's fine. Though I will re-post this change when updating the
series, but this should not harm (won't conflict) as this
specific patch is stable and won't change any longer. Keeping
this one patch in the series keeps the series applicable on top
of v3.11-rcN as well as clk-next.
Note that this patch only changes those parts of the code under
drivers/clk/ which get shared among platforms (div, gate, mux).
It doesn't touch non-shared and platform specific drivers. I
felt this was the most appropriate thing to do.
> > ---
> > drivers/clk/clk-divider.c | 6 +++---
> > drivers/clk/clk-gate.c | 6 +++---
> > drivers/clk/clk-mux.c | 6 +++---
> > include/linux/clk-provider.h | 17 +++++++++++++++++
> > 4 files changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index 6d55eb2..2c07061 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
> > struct clk_divider *divider = to_clk_divider(hw);
> > unsigned int div, val;
> >
> > - val = readl(divider->reg) >> divider->shift;
> > + val = clk_readl(divider->reg) >> divider->shift;
> > val &= div_mask(divider);
> >
> > div = _get_div(divider, val);
> > @@ -230,11 +230,11 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> > if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
> > val = div_mask(divider) << (divider->shift + 16);
> > } else {
> > - val = readl(divider->reg);
> > + val = clk_readl(divider->reg);
> > val &= ~(div_mask(divider) << divider->shift);
> > }
> > val |= value << divider->shift;
> > - writel(val, divider->reg);
> > + clk_writel(val, divider->reg);
> >
> > if (divider->lock)
> > spin_unlock_irqrestore(divider->lock, flags);
> > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> > index 790306e..b7fbd96 100644
> > --- a/drivers/clk/clk-gate.c
> > +++ b/drivers/clk/clk-gate.c
> > @@ -58,7 +58,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
> > if (set)
> > reg |= BIT(gate->bit_idx);
> > } else {
> > - reg = readl(gate->reg);
> > + reg = clk_readl(gate->reg);
> >
> > if (set)
> > reg |= BIT(gate->bit_idx);
> > @@ -66,7 +66,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
> > reg &= ~BIT(gate->bit_idx);
> > }
> >
> > - writel(reg, gate->reg);
> > + clk_writel(reg, gate->reg);
> >
> > if (gate->lock)
> > spin_unlock_irqrestore(gate->lock, flags);
> > @@ -89,7 +89,7 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
> > u32 reg;
> > struct clk_gate *gate = to_clk_gate(hw);
> >
> > - reg = readl(gate->reg);
> > + reg = clk_readl(gate->reg);
> >
> > /* if a set bit disables this clk, flip it before masking */
> > if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> > index 614444c..02ef506 100644
> > --- a/drivers/clk/clk-mux.c
> > +++ b/drivers/clk/clk-mux.c
> > @@ -42,7 +42,7 @@ static u8 clk_mux_get_parent(struct clk_hw *hw)
> > * OTOH, pmd_trace_clk_mux_ck uses a separate bit for each clock, so
> > * val = 0x4 really means "bit 2, index starts at bit 0"
> > */
> > - val = readl(mux->reg) >> mux->shift;
> > + val = clk_readl(mux->reg) >> mux->shift;
> > val &= mux->mask;
> >
> > if (mux->table) {
> > @@ -89,11 +89,11 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
> > if (mux->flags & CLK_MUX_HIWORD_MASK) {
> > val = mux->mask << (mux->shift + 16);
> > } else {
> > - val = readl(mux->reg);
> > + val = clk_readl(mux->reg);
> > val &= ~(mux->mask << mux->shift);
> > }
> > val |= index << mux->shift;
> > - writel(val, mux->reg);
> > + clk_writel(val, mux->reg);
> >
> > if (mux->lock)
> > spin_unlock_irqrestore(mux->lock, flags);
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 1ec14a7..c4f7799 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -12,6 +12,7 @@
> > #define __LINUX_CLK_PROVIDER_H
> >
> > #include <linux/clk.h>
> > +#include <linux/io.h>
> >
> > #ifdef CONFIG_COMMON_CLK
> >
> > @@ -490,5 +491,21 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
> > #define of_clk_init(matches) \
> > { while (0); }
> > #endif /* CONFIG_OF */
> > +
> > +/*
> > + * wrap access to peripherals in accessor routines
> > + * for improved portability across platforms
> > + */
> > +
> > +static inline u32 clk_readl(u32 __iomem *reg)
> > +{
> > + return readl(reg);
> > +}
> > +
> > +static inline void clk_writel(u32 val, u32 __iomem *reg)
> > +{
> > + writel(val, reg);
> > +}
> > +
> > #endif /* CONFIG_COMMON_CLK */
> > #endif /* CLK_PROVIDER_H */
> > --
> > 1.7.10.4
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: 3.11-rc3+git: __divdi3 undefined on powerpc (from radeon)
From: Alex Deucher @ 2013-08-03 13:52 UTC (permalink / raw)
To: Meelis Roos; +Cc: linuxppc-dev, Linux Kernel list, dri-devel
In-Reply-To: <alpine.SOC.1.00.1308031534390.1352@math.ut.ee>
On Sat, Aug 3, 2013 at 8:44 AM, Meelis Roos <mroos@linux.ee> wrote:
> While trying to compile v3.11-rc3-288-gabe0308 on powerpc 32-bit,
> it failed with the following linking error:
>
> ERROR: "__divdi3" [drivers/gpu/drm/radeon/radeon.ko] undefined!
>
> Some new 64-bit division in radeon that is not implemented on 32-bit
> powerpc?
>
> This is new - 3.11-rc3 worked fine.
Fix is already queued:
http://lists.freedesktop.org/archives/dri-devel/2013-August/042668.html
Alex
>
> --
> Meelis Roos (mroos@linux.ee)
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: windfarm_fcu_controls: cpu-pump-0 <HW FAULT>
From: Andreas Schwab @ 2013-08-03 13:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1375519493.15999.83.camel__49452.0221990117$1375519543$gmane$org@pasglop>
Actually I see the same failure mask with the therm_pm72 driver, I
didn't notice since the driver ignores the return value from set_rpm_fan
and there is no sysfs file to monitor the cpu pumps. So it looks like
some real hardware fault?
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* 3.11-rc3+git: __divdi3 undefined on powerpc (from radeon)
From: Meelis Roos @ 2013-08-03 12:44 UTC (permalink / raw)
To: Linux Kernel list, dri-devel, linuxppc-dev
While trying to compile v3.11-rc3-288-gabe0308 on powerpc 32-bit,
it failed with the following linking error:
ERROR: "__divdi3" [drivers/gpu/drm/radeon/radeon.ko] undefined!
Some new 64-bit division in radeon that is not implemented on 32-bit
powerpc?
This is new - 3.11-rc3 worked fine.
--
Meelis Roos (mroos@linux.ee)
^ permalink raw reply
* Re: windfarm_fcu_controls: cpu-pump-0 <HW FAULT>
From: Benjamin Herrenschmidt @ 2013-08-03 8:44 UTC (permalink / raw)
To: Andreas Schwab; +Cc: linuxppc-dev
In-Reply-To: <m2vc3nnp01.fsf@linux-m68k.org>
On Sat, 2013-08-03 at 10:43 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>
> > Can you add some more printk's in there to check what's going on inside
> > wf_fcu_get_pump_minmax() ?
>
> All values from mpu->processor_part_num are 0xffff.
>
> > Also is it getting faults for both pumps ?
>
> Yes.
>
> > Does it work with the older driver ? (both the minmax and the reading of
> > the pump).
>
> The minmax situation is the same, but otherwise appears to work (no fan
> reading errors logged). Here is an example debug output:
Odd. Can you try to trace if there is any significant difference in the
i2c messages used ? Some typo I might have done somewhere ?
Something I might do at init time that puts them into a faulty state ?
I don't have one of these anymore (mine died) so I can't really test.
Cheers,
Ben.
> ** CPU 1 RPM: 300 Ex, 300, Pump: 1250, In, overtemp: 0
> cpu 0, exhaust RPM: 300
> cpu 0, temp raw: 023c, m_diode: 9982, b_diode: fffff799
> temp: 52.139
> cpu 0, current: 8.789, voltage: 1.286, power: 11.308 W
> cpu 1, exhaust RPM: 300
> cpu 1, temp raw: 021c, m_diode: a047, b_diode: fffff777
> temp: 50.380
> cpu 1, current: 8.666, voltage: 1.281, power: 11.108 W
> power target: 55.000, error: 43.691
> integral: 00f42f8d
> integ_p: 10
> adj_in_target: 65.011, ttarget: 74
> deriv_p: -15
> prop_p: -103
> sum: -118
>
> > What is the "failures" bitmask value ?
>
> 3 for both.
>
> Andreas.
>
^ permalink raw reply
* Re: windfarm_fcu_controls: cpu-pump-0 <HW FAULT>
From: Andreas Schwab @ 2013-08-03 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Andreas Schwab
In-Reply-To: <1375481448.15999.47.camel__48019.1930350431$1375481501$gmane$org@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> Can you add some more printk's in there to check what's going on inside
> wf_fcu_get_pump_minmax() ?
All values from mpu->processor_part_num are 0xffff.
> Also is it getting faults for both pumps ?
Yes.
> Does it work with the older driver ? (both the minmax and the reading of
> the pump).
The minmax situation is the same, but otherwise appears to work (no fan
reading errors logged). Here is an example debug output:
** CPU 1 RPM: 300 Ex, 300, Pump: 1250, In, overtemp: 0
cpu 0, exhaust RPM: 300
cpu 0, temp raw: 023c, m_diode: 9982, b_diode: fffff799
temp: 52.139
cpu 0, current: 8.789, voltage: 1.286, power: 11.308 W
cpu 1, exhaust RPM: 300
cpu 1, temp raw: 021c, m_diode: a047, b_diode: fffff777
temp: 50.380
cpu 1, current: 8.666, voltage: 1.281, power: 11.108 W
power target: 55.000, error: 43.691
integral: 00f42f8d
integ_p: 10
adj_in_target: 65.011, ttarget: 74
deriv_p: -15
prop_p: -103
sum: -118
> What is the "failures" bitmask value ?
3 for both.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox