@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;++/{+model="Calxeda Highbank";+compatible="calxeda,highbank";+#address-cells=<1>;+#size-cells=<1>;++cpus{+#address-cells=<1>;+#size-cells=<0>;++cpuat0{+compatible="arm,cortex-a9";+reg=<0>;+next-level-cache=<&L2>;+};++cpuat1{+compatible="arm,cortex-a9";+reg=<1>;+next-level-cache=<&L2>;+};++cpuat2{+compatible="arm,cortex-a9";+reg=<2>;+next-level-cache=<&L2>;+};++cpuat3{+compatible="arm,cortex-a9";+reg=<3>;+next-level-cache=<&L2>;+};+};++memory{+name="memory";+device_type="memory";+reg=<0x000000000xff900000>;+};++chosen{+bootargs="console=ttyAMA0";+};++intc:interrupt-controlleratfff11000{+compatible="arm,cortex-a9-gic";+#interrupt-cells=<1>;+#size-cells=<0>;+#address-cells=<1>;+interrupt-controller;+reg=<0xfff110000x1000>,+<0xfff101000x100>;++gicppi0:gic-ppiat0{+compatible="arm,cortex-a9-gic-ppi";+#interrupt-cells=<1>;+interrupt-controller;+reg=<0>;+};+gicppi1:gic-ppiat1{+compatible="arm,cortex-a9-gic-ppi";+#interrupt-cells=<1>;+interrupt-controller;+reg=<1>;+};+};++soc{+#address-cells=<1>;+#size-cells=<1>;+compatible="simple-bus";+interrupt-parent=<&intc>;+ranges;++timeratfff10600{+compatible="arm,smp-twd";+reg=<0xfff106000x20>;+interrupt-parent=<&gicppi0>;+interrupts=<29>;+};++watchdogatfff10620{+compatible="arm,cortex-a9-wdt";+reg=<0xfff106200x20>;+interrupt-parent=<&gicppi0>;+interrupts=<30>;+};++L2:l2-cache{+compatible="arm,pl310-cache";+reg=<0xfff120000x1000>;+interrupts=<102>;+cache-unified;+cache-level=<2>;+};++pmu{+compatible="arm,cortex-a9-pmu";+interrupts=<108107106105>;+};++sataatffe08000{+compatible="calxeda,hb-ahci";+reg=<0xffe080000x10000>;+interrupts=<115>;+};++sdhciatffe0e000{+compatible="calxeda,hb-sdhci";+reg=<0xffe0e0000x1000>;+interrupts=<122>;+};++ipcatfff20000{+compatible="arm,pl320","arm,primecell";+reg=<0xfff200000x1000>;+interrupts=<39>;+};++gpioe:gpioatfff30000{+#gpio-cells=<2>;+compatible="arm,pl061","arm,primecell";+gpio-controller;+reg=<0xfff300000x1000>;+interrupts=<46>;+};++gpiof:gpioatfff31000{+#gpio-cells=<2>;+compatible="arm,pl061","arm,primecell";+gpio-controller;+reg=<0xfff310000x1000>;+interrupts=<47>;+};++gpiog:gpioatfff32000{+#gpio-cells=<2>;+compatible="arm,pl061","arm,primecell";+gpio-controller;+reg=<0xfff320000x1000>;+interrupts=<48>;+};++gpioh:gpioatfff33000{+#gpio-cells=<2>;+compatible="arm,pl061","arm,primecell";+gpio-controller;+reg=<0xfff330000x1000>;+interrupts=<49>;+};++timer{+compatible="arm,sp804","arm,primecell";+reg=<0xfff340000x1000>;+interrupts=<50>;+};++rtcatfff35000{+compatible="arm,pl031","arm,primecell";+reg=<0xfff350000x1000>;+interrupts=<51>;+};++serialatfff36000{+compatible="arm,pl011","arm,primecell";+reg=<0xfff360000x1000>;+interrupts=<52>;+};++smicatfff3a000{+compatible="ipmi-smic";+device_type="ipmi";+reg=<0xfff3a0000x1000>;+interrupts=<56>;+reg-size=<4>;+reg-spacing=<4>;+};++sregsatfff3c000{+compatible="calxeda,hb-sregs";+reg=<0xfff3c0000x1000>;+};++dmaatfff3d000{+compatible="arm,pl330","arm,primecell";+reg=<0xfff3d0000x1000>;+interrupts=<124>;+};+};+};
From: Rob Herring <hidden> Date: 2011-08-16 20:34:57
From: Martin Bogomolni <redacted>
This adds cpu hotplug for highbank. On highbank, a core is always reset and
boots up the same path as a cold boot.
Signed-off-by: Martin Bogomolni <redacted>
Signed-off-by: Rob Herring <redacted>
---
arch/arm/mach-highbank/Makefile | 1 +
arch/arm/mach-highbank/hotplug.c | 56 ++++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-highbank/hotplug.c
@@ -0,0 +1,56 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/+#include<linux/kernel.h>+#include<linux/errno.h>+#include<linux/smp.h>++#include<asm/smp_scu.h>+#include<asm/cacheflush.h>++#include"core.h"++externvoidsecondary_startup(void);++intplatform_cpu_kill(unsignedintcpu)+{+return1;+}++/*+*platform-specificcodetoshutdownaCPU+*+*/+voidplatform_cpu_die(unsignedintcpu)+{+flush_cache_all();++highbank_set_cpu_jump(cpu,secondary_startup);+scu_power_mode(a9_base_addr,SCU_PM_POWEROFF);++cpu_do_idle();++/* We should never return from idle */+panic("highbank: cpu %d unexpectedly exit from shutdown\n",cpu);+}++intplatform_cpu_disable(unsignedintcpu)+{+/*+*CPU0shouldnotbeshutdownviahotplug.cpu_idlecanWFI+*orapropershutdownorhibernateshouldbeused.+*/+returncpu==0?-EPERM:0;+}
Should the return values be checked here? I know that all valid device trees
should have these nodes and valid a reg property, but I don't know if the
error handling needs to be a bit more explicit. For my platform I have put
these checks and panics() if they fail, but I'm not sure if that's the right
thing!
Jamie
@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;+
I failed to find the "pen" handling in the whole patch series. Am I
missing anything?
--
Regards,
Shawn
-----Original Message-----
From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm-
kernel-bounces at lists.infradead.org] On Behalf Of Rob Herring
Sent: 16 August 2011 21:35
To: linux-arm-kernel at lists.infradead.org
Cc: Rob Herring
Subject: [PATCH 1/6] ARM: highbank: add devicetree source
From: Rob Herring <redacted>
This adds the devicetree source and documentation for the Calxeda
highbank
platform.
Signed-off-by: Rob Herring <redacted>
---
Documentation/devicetree/bindings/arm/calxeda.txt | 8 +
arch/arm/boot/dts/highbank.dts | 212
+++++++++++++++++++++
2 files changed, 220 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/calxeda.txt
create mode 100644 arch/arm/boot/dts/highbank.dts
@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;+
I failed to find the "pen" handling in the whole patch series. Am I
missing anything?
Only if something is wrong and the mapping did not get setup. The system
should still boot with 1 core without accessing the the SCU in this case.
This BTW is the only reason we need a static mapping of the SCU. So I
could just hard code it or use device tree to get the number of cores
and eliminate the static mapping.
Rob
@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;+
I failed to find the "pen" handling in the whole patch series. Am I
missing anything?
Ah, ok, the 'pen' you meant is the entry address of secondary cores.
I thought of something like 'pen_release' in plat-versatile/platsmp.c.
So you do not need 'boot_lock' and 'pen_release' stuff to sync secondary
cores with the primary one (like all other smp platforms do)?
--
Regards,
Shawn
Only if something is wrong and the mapping did not get setup. The system
should still boot with 1 core without accessing the the SCU in this case.
However, in a previous patch, a9_base_addr is statically initialized
and never written to, so that doesn't work. Not only that but its
also used for poweroff and cpu suspend...
This BTW is the only reason we need a static mapping of the SCU. So I
could just hard code it or use device tree to get the number of cores
and eliminate the static mapping.
this statement isn't accurate.
I can see no way for scu_base to ever be NULL here.
I really liked the way that Barry moved the io_desc out to the
drivers using them, e.g arch/arm/mach-prima2/lluart.c.
Can you do the same thing with your lluart and with the a9_base_addr?
I guess it can live locally in platsmp.c.
Hmm, I missed that during the review of the patch that adds l2x0_of_init,
but I think the #ifdef should really be in the header file, not in the
user, so that calling l2x0_of_init when CONFIG_CACHE_L2X0 is not set
automatically turns into an empty stub.
In 3.2, we shouldn't need this any more. We'll have to come up with a
way to remember removing the new definitions that come in in parallel
to the patch that removes the old ones.
Do these really need to be global?
I think it's better to put the base address and register definitions into a
single file and export functions to be used from elsewhere.
Arnd
In 3.2, we shouldn't need this any more. We'll have to come up with a
way to remember removing the new definitions that come in in parallel
to the patch that removes the old ones.
Has anyone really properly evaluated the CLOCK_TICK_RATE issues on things
like NTP etc? I have problems with kernels on OMAP4 constantly jumping
forwards/back by .5sec when NTP is running which suggests that there's
something not quite right _somewhere_.
Given that OMAP uses an untrue value for this, and the platforms I have
which _do_ behave properly when running NTP have correct values, I _still_
remain entirely unconvinced about the claims surrounding CLOCK_TICK_RATE
not mattering.
Has anyone managed to run NTP on OMAP4 and had it sync successfully over
a few days?
It's a great demo that we can get this base address from CA9 itself.
However I'm wondering if we have to do so, since this address should
be known for given SoC. Can we put this known base into device tree
and map it when needed? In that case, we can save another static
mapping.
--
Regards,
Shawn
It's a great demo that we can get this base address from CA9 itself.
However I'm wondering if we have to do so, since this address should
be known for given SoC. Can we put this known base into device tree
and map it when needed? In that case, we can save another static
mapping.
Hmm, if CA9 can figure this address out by itself, why do not we create
a init hook in arch/arm/kernel/smp_scu.c to have scu code to get and
map the address? Then the parameter "scu_base" in those scu helper
functions can be saved.
--
Regards,
Shawn
It's a great demo that we can get this base address from CA9 itself.
However I'm wondering if we have to do so, since this address should
be known for given SoC. Can we put this known base into device tree
and map it when needed? In that case, we can save another static
mapping.
Since device tree is mainly about providing data to describe system
components that are not discoverable, maybe it is better after all
not to put this address in the device tree.
Discovery feels like the best approach, where possible.
---Dave
It's a great demo that we can get this base address from CA9 itself.
However I'm wondering if we have to do so, since this address should
be known for given SoC. Can we put this known base into device tree
and map it when needed? In that case, we can save another static
mapping.
Since device tree is mainly about providing data to describe system
components that are not discoverable, maybe it is better after all
not to put this address in the device tree.
Discovery feels like the best approach, where possible.
It's a bit silly to leave it out of the device tree when you otherwise
describe all major components.
However, I agree that if you can probe the value from hardware,
there is no need to read it from there.
Arnd
How about moving the sp804 initialization from device tree into the
arch/arm/common/timer-sp.c file?
Why do you initialize sregs_base from timer_init?
That'd create special cases - ARM platforms need registers twiddled to
change the clock rate for the timers from 32kHz to a more sensible 1MHz.
Is that a bad thing? Platforms that don't need the special case can
simply call sp804_clocksource_init_dt() which scans the device tree,
while other platforms do whatever is necessary to the registers
and then call the existing sp804_clockevents_init.
In 3.2, we shouldn't need this any more. We'll have to come up with a
way to remember removing the new definitions that come in in parallel
to the patch that removes the old ones.
Has anyone really properly evaluated the CLOCK_TICK_RATE issues on things
like NTP etc? I have problems with kernels on OMAP4 constantly jumping
forwards/back by .5sec when NTP is running which suggests that there's
something not quite right _somewhere_.
Given that OMAP uses an untrue value for this, and the platforms I have
which do behave properly when running NTP have correct values, I still
remain entirely unconvinced about the claims surrounding CLOCK_TICK_RATE
not mattering.
(Taking John, Deepak an Thomas on Cc, they have all worked on this
in the past)
The argument why it is assumed to be safe is that almost all machines
today use a totally bogus CLOCK_TICK_RATE. This includes most x86
machines (which don't use PIT for periodic ticks any more), all sparc,
powerpc, s390, parisc and mips machines that have never used the PIT
time base but define CLOCK_TICK_RATE to 1193180 or 1193182 anyway.
The only explanation I have for these working correctly is that
the effect of the ACTHZ macro is not what it was meant to be
and that it should better be removed.
Has anyone managed to run NTP on OMAP4 and had it sync successfully over
a few days?
Omap is weird in many ways here. They define CLOCK_TICK_RATE to be
equal to HZ, which in turn is a power-of-two value, typically 128.
I have verified that the strange CLOCK_TICK_RATE won't cause problems
in the kernel (in theory), but I could well imagine that the problems
of OMAP are stemming from rounding problems when converting between
kernel ticks (128 Hz) to user ticks (100 Hz) in kernel/time.c, or perhaps
from the omap read_persistent_clock() function not being SMP safe.
Arnd
@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;++/{+model="Calxeda Highbank";+compatible="calxeda,highbank";+#address-cells=<1>;+#size-cells=<1>;++cpus{+#address-cells=<1>;+#size-cells=<0>;++cpuat0{+compatible="arm,cortex-a9";+reg=<0>;+next-level-cache=<&L2>;+};++cpuat1{+compatible="arm,cortex-a9";+reg=<1>;+next-level-cache=<&L2>;+};++cpuat2{+compatible="arm,cortex-a9";+reg=<2>;+next-level-cache=<&L2>;+};++cpuat3{+compatible="arm,cortex-a9";+reg=<3>;+next-level-cache=<&L2>;+};+};+
I'm not sure if this whole "cpus" stuff is needed, I do not see any
code playing with it.
--
Regards,
Shawn
160, magic number? I guess it's the GIC IRQ number has been
discovered in gic_dist_init(). I'm not sure if we should simply
define a macro for it or manage to retrieve it from GIC.
It's a great demo that we can get this base address from CA9 itself.
However I'm wondering if we have to do so, since this address should
be known for given SoC. Can we put this known base into device tree
and map it when needed? In that case, we can save another static
mapping.
Hmm, if CA9 can figure this address out by itself, why do not we create
a init hook in arch/arm/kernel/smp_scu.c to have scu code to get and
map the address? Then the parameter "scu_base" in those scu helper
functions can be saved.
smp_scu.c is not CA9 only and this is a CA9 specific feature. Also, you
have to statically map the scu to get the core count. ioremap is not up
at that point in time. Don't be fooled by omap code either. They do an
ioremap, but it handles their static mappings as well.
Rob
@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;++/{+model="Calxeda Highbank";+compatible="calxeda,highbank";+#address-cells=<1>;+#size-cells=<1>;++cpus{+#address-cells=<1>;+#size-cells=<0>;++cpuat0{+compatible="arm,cortex-a9";+reg=<0>;+next-level-cache=<&L2>;+};++cpuat1{+compatible="arm,cortex-a9";+reg=<1>;+next-level-cache=<&L2>;+};++cpuat2{+compatible="arm,cortex-a9";+reg=<2>;+next-level-cache=<&L2>;+};++cpuat3{+compatible="arm,cortex-a9";+reg=<3>;+next-level-cache=<&L2>;+};+};+
I'm not sure if this whole "cpus" stuff is needed, I do not see any
code playing with it.
Prior reviews of dts's by Grant and others suggested putting this in.
Rob
160, magic number? I guess it's the GIC IRQ number has been
discovered in gic_dist_init(). I'm not sure if we should simply
define a macro for it or manage to retrieve it from GIC.
It's the number of GIC irqs. This is temporary until we have dynamic
assignment of linux irq numbers.
Rob
@@ -0,0 +1,212 @@+/*+*Copyright2011Calxeda,Inc.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/++/dts-v1/;++/* First 4KB has pen for secondary cores. */+/memreserve/0x00000000 0x0001000;+
I failed to find the "pen" handling in the whole patch series. Am I
missing anything?
Ah, ok, the 'pen' you meant is the entry address of secondary cores.
I thought of something like 'pen_release' in plat-versatile/platsmp.c.
So you do not need 'boot_lock' and 'pen_release' stuff to sync secondary
cores with the primary one (like all other smp platforms do)?
The kernel pen code is only needed for cores that don't reset on hot
unplug and just go to wfi. All other smp platforms just cut and paste
the same code.
Rob
I really liked the way that Barry moved the io_desc out to the
drivers using them, e.g arch/arm/mach-prima2/lluart.c.
Can you do the same thing with your lluart and with the a9_base_addr?
I guess it can live locally in platsmp.c.
Where does the "160 + (8 * n)" come from? Is that something that should
be in a property of the gic binding?
All this should go away once we have dynamic linux irq number
assignment. Actually, I should just delete this for now as the pl061
driver doesn't support interrupts yet with DT binding (without platform
data).
Hmm, I missed that during the review of the patch that adds l2x0_of_init,
but I think the #ifdef should really be in the header file, not in the
user, so that calling l2x0_of_init when CONFIG_CACHE_L2X0 is not set
automatically turns into an empty stub.
It's also a problem with l2x0_init. I'll add a patch to do that.
How about moving the sp804 initialization from device tree into the
arch/arm/common/timer-sp.c file?
Why do you initialize sregs_base from timer_init?
It will be needed before the clocks are initialized. The clock code is
not using it at the moment as I just did a minimal fixed clock
implementation until the clock api and DT clock bindings gets sorted
out. As there are multiple users, I didn't put it in highbank_clocks_init.
In 3.2, we shouldn't need this any more. We'll have to come up with a
way to remember removing the new definitions that come in in parallel
to the patch that removes the old ones.
I'm tracking the various clean-ups and we can coordinate the order
things go in. I've already made gpio.h empty for example, so gpio will
fail to compile if enabled in this series.
Or I can just submit a patch deleting this file later. It will just be
dead code and won't conflict.
Do these really need to be global?
I think it's better to put the base address and register definitions into a
single file and export functions to be used from elsewhere.
Yes, sregs are a random collection of functions, so it's going to be a
mixture of various users. Just HB_PWR_* alone are in 2 or 3 different
places.
Rob
How about moving the sp804 initialization from device tree into the
arch/arm/common/timer-sp.c file?
Why do you initialize sregs_base from timer_init?
That'd create special cases - ARM platforms need registers twiddled to
change the clock rate for the timers from 32kHz to a more sensible 1MHz.
Is that a bad thing? Platforms that don't need the special case can
simply call sp804_clocksource_init_dt() which scans the device tree,
while other platforms do whatever is necessary to the registers
and then call the existing sp804_clockevents_init.
This was something I considered as I've worked on doing that in other
cases like l2x0 and gic.
There's also the issue that the clocksource and clockevent timers may or
may not be at the same address range (base and base + 0x20), so you may
need 1 or 2 mappings. You could figure all that out, but it would be a
lot of work for little gain. Also, which timer is used for which
function is also platform dependent. For example, we don't have the 2nd
timer's interrupt hooked up so it has to be the .
On top of that this really needs to wait until the DT clock binding is
in place as clock setup is a major part of the init.
Rob
From: Russell King - ARM Linux <hidden> Date: 2011-08-20 23:05:33
On Fri, Aug 19, 2011 at 04:11:35PM +0200, Arnd Bergmann wrote:
Is that a bad thing? Platforms that don't need the special case can
simply call sp804_clocksource_init_dt() which scans the device tree,
while other platforms do whatever is necessary to the registers
and then call the existing sp804_clockevents_init.
It means we're not solving the problem. We end up with some platforms
which do the sp804 init from DT, others which are DT but ignore the
DT stuff for sp804. That's not very consistent, and in the long run
is rather confusing.
I failed to find the implementation of gic_of_ppi_init() in your patch
series below.
[RFC PATCH 0/3] Yet another GIC OF binding series
Also one nit: the braces seem redundant.
--
Regards,
Shawn
It seems that the second file already includes the first one.
It's preferred to use explicit includes. From
Documentation/SubmitChecklist:
1: If you use a facility then #include the file that
defines/declares that facility. Don't depend on other header
files pulling in ones that you use.
Jamie
It seems that the second file already includes the first one.
It's preferred to use explicit includes. From
Documentation/SubmitChecklist:
1: If you use a facility then #include the file that
defines/declares that facility. Don't depend on other header
files pulling in ones that you use.
We should not include it even with above point, because we do not have
anything in entry-macro.S requiring gic.h.
--
Regards,
Shawn
Should the return values be checked here? I know that all valid device trees
should have these nodes and valid a reg property, but I don't know if the
error handling needs to be a bit more explicit. For my platform I have put
these checks and panics() if they fail, but I'm not sure if that's the right
thing!
A panic will stop the boot at a point the console is not up unless
DEBUG_LL is enabled. If you continue, you may be able to continue long
enough to get a console and then fail when something that depends on
this is used. For this case, it would be when the clocks are not setup
correctly (once real clock setup is implemented). As it is now, these
registers aren't accessed until you do suspend, hotplug, or poweroff.
So probably just a WARN_ON would be better here.
Rob
I really liked the way that Barry moved the io_desc out to the
drivers using them, e.g arch/arm/mach-prima2/lluart.c.
Can you do the same thing with your lluart and with the a9_base_addr?
I guess it can live locally in platsmp.c.
Okay.
Looking at this some more, it doesn't work too well. platsmp.c depends
on CONFIG_SMP, but the SCU mapping is always needed even for !SMP
because the SCU has a power mode register for each core used by the
power controller. So putting it in platsmp.c would add ifdefs.
So I'll move out the lluart mapping, but keep SCU mapping in highbank.c.
Rob
Where does the "160 + (8 * n)" come from? Is that something that should
be in a property of the gic binding?
All this should go away once we have dynamic linux irq number
assignment. Actually, I should just delete this for now as the pl061
driver doesn't support interrupts yet with DT binding (without platform
data).
Hmm, I missed that during the review of the patch that adds l2x0_of_init,
but I think the #ifdef should really be in the header file, not in the
user, so that calling l2x0_of_init when CONFIG_CACHE_L2X0 is not set
automatically turns into an empty stub.
It's also a problem with l2x0_init. I'll add a patch to do that.
How about moving the sp804 initialization from device tree into the
arch/arm/common/timer-sp.c file?
Why do you initialize sregs_base from timer_init?
It will be needed before the clocks are initialized. The clock code is
not using it at the moment as I just did a minimal fixed clock
implementation until the clock api and DT clock bindings gets sorted
out. As there are multiple users, I didn't put it in highbank_clocks_init.
In 3.2, we shouldn't need this any more. We'll have to come up with a
way to remember removing the new definitions that come in in parallel
to the patch that removes the old ones.
I'm tracking the various clean-ups and we can coordinate the order
things go in. I've already made gpio.h empty for example, so gpio will
fail to compile if enabled in this series.
Or I can just submit a patch deleting this file later. It will just be
dead code and won't conflict.
Do these really need to be global?
I think it's better to put the base address and register definitions into a
single file and export functions to be used from elsewhere.
Yes, sregs are a random collection of functions, so it's going to be a
mixture of various users. Just HB_PWR_* alone are in 2 or 3 different
places.
Rob
I really liked the way that Barry moved the io_desc out to the
drivers using them, e.g arch/arm/mach-prima2/lluart.c.
Can you do the same thing with your lluart and with the a9_base_addr?
I guess it can live locally in platsmp.c.
Okay.
Looking at this some more, it doesn't work too well. platsmp.c depends
on CONFIG_SMP, but the SCU mapping is always needed even for !SMP
because the SCU has a power mode register for each core used by the
power controller. So putting it in platsmp.c would add ifdefs.
It sounds you have a well designed power controller which will shut
down different CA-9 power domains per SCU power mode settings, e.g.
it will shut off different set of CA-9 power domains for Dormant and
Shutdown modes. Is that the case?
i.MX6Q is different from that case, so I can actually follow Arnd's
suggestion to keep scu_base locally in platsmp.c.
--
Regards,
Shawn
Looking at this some more, it doesn't work too well. platsmp.c depends
on CONFIG_SMP, but the SCU mapping is always needed even for !SMP
because the SCU has a power mode register for each core used by the
power controller. So putting it in platsmp.c would add ifdefs.
So I'll move out the lluart mapping, but keep SCU mapping in highbank.c.
Ok, makes sense. Another alternative would be to have multiple mappings
for this: one local to the platsmp file and another one local to the
power management code. However, that is ugly for other reasons.
Arnd
Do these really need to be global?
I think it's better to put the base address and register definitions into a
single file and export functions to be used from elsewhere.
Yes, sregs are a random collection of functions, so it's going to be a
mixture of various users. Just HB_PWR_* alone are in 2 or 3 different
places.
Sorry, I'm not following.
Do you mean 'yes, they need to be global' or 'yes, it's better to export
the functions'?
Arnd
Do these really need to be global?
I think it's better to put the base address and register definitions into a
single file and export functions to be used from elsewhere.
Yes, sregs are a random collection of functions, so it's going to be a
mixture of various users. Just HB_PWR_* alone are in 2 or 3 different
places.
Sorry, I'm not following.
Do you mean 'yes, they need to be global' or 'yes, it's better to export
the functions'?
I meant the former and functions as in h/w functionality, not C functions.
This will mainly be clock control plus a few other things. Is it really
desired to add another layer here when these are all just single
register writes?
Rob
On Thursday 25 August 2011 13:03:24 Rob Herring wrote:
quoted
Do you mean 'yes, they need to be global' or 'yes, it's better to export
the functions'?
I meant the former and functions as in h/w functionality, not C functions.
This will mainly be clock control plus a few other things. Is it really
desired to add another layer here when these are all just single
register writes?
It really depends on what the registers do, which I haven't seen. My feeling
is still that you should have an abstract interface for drivers to use,
but if the drivers need to do very little, exporting the symbol for the
base address and using it in inline functions from the header could be
appropriate.
Arnd