On Wed, Dec 11, 2013 at 01:55:36PM +0000, Mark Rutland wrote:
On Wed, Dec 11, 2013 at 01:13:23PM +0000, Mark Brown wrote:
+/dts-v1/;
+/memreserve/ 0x80000000 0x00010000;
While we are admittedly missing them elsewhere, it would be nice to have a comment stating what the memreserve is for. With a proper PSCI implementation, I don't see why this should be necessary.
Could you tell me what it's there for? Like you say everyone else has it with no comments and the source doesn't either...
+/ {
- model = "FVP Base";
FVP Base (is as the name implies) a base upon which particular model instances are built. This name should be clarified (e.g. "FVP Base A57x4 A53x4").
That also applies to the filename.
I can update these, though they do seem to come from what you guys are releasing - you might want to follow up on this internally (this applies to almost all of your review comments, sorry). It's probably going to be a bit confusing for users to have the filename change but ho hum :/
As the FVP is not a vexpress (though it is similar), we should probably have "arm,fvp-base" as the fallback and drop the "arm,vexpress" entry.
Is it not intended to be a software emulation of a vexpress and hence supposed to be broadly compatible with one?
- psci {
compatible = "arm,psci";method = "smc";cpu_suspend = <0xc4000001>;cpu_off = <0x84000002>;cpu_on = <0xc4000003>;- };
Are these IDs right? One of these IDs is a different width than the others.
I have no idea, I have no documentation for any of this stuff other than the DT you guys release on github - it's just copied verbatim from there.
Which firmware/bootloader does this correspond to?
I'm testing using the Linaro 13.11 release.
big0: cpu@0 {device_type = "cpu";compatible = "arm,cortex-a57", "arm,armv8";reg = <0x0 0x0>;enable-method = "psci";clock-frequency = <1000000>;
Is clock-frequency used anywhere? Is it a useful thing to have (regardless of whether ePAPR defines it)?
The topology code for ARMv7 (which I'm following for ARMv8) uses this in conjunction with the compatible to provide information to the scheduler about the relative performance of the cores.
- timer {
compatible = "arm,armv8-timer";interrupts = <1 13 0xff01>,<1 14 0xff01>,<1 11 0xff01>,<1 10 0xff01>;clock-frequency = <100000000>;
A clock-frequency property in a timer is an indicator that the bootloader/firmware is broken and should be fixed. Is this actually necessary, or does the firmware/loader program CNTFRQ correctly on all CPUs?
I can try to see what happens if I remove it - I doubt anyone has tested without it...
- pmu {
compatible = "arm,armv8-pmuv3";interrupts = <0 60 4>,<0 61 4>,<0 62 4>,<0 63 4>;- };
I assume this is just the A57 cores? That should probably be noted for now. In future we'll need to define the relationship between interrupts and CPUs, and describe the A53 cores.
Again I don't know, this is just copied verbatim from what you guys release - I have no additional documentation.