| Message ID | 1386767606-6391-3-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 11, 2013 at 01:13:23PM +0000, Mark Brown wrote: > From: Mark Brown <broonie@linaro.org> > > Add a dts file for ARMv8 fixed virtual platform which models a > big.LITTLE configuration for the architecture. This is based on the > DT shipped by ARM but has been modified for mainline, removing features > not present in mainline and updating the CPU bindings for mainline. > > Signed-off-by: Mark Brown <broonie@linaro.org> > --- > arch/arm64/boot/dts/Makefile | 4 +- > arch/arm64/boot/dts/fvp-base-gicv2-psci.dts | 233 ++++++++++++++++++++++++++++ > 2 files changed, 236 insertions(+), 1 deletion(-) > create mode 100644 arch/arm64/boot/dts/fvp-base-gicv2-psci.dts > > diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile > index c52bdb051f66..2d16cdbe5d47 100644 > --- a/arch/arm64/boot/dts/Makefile > +++ b/arch/arm64/boot/dts/Makefile > @@ -1,4 +1,6 @@ > -dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb > +dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb \ > + foundation-v8.dtb \ > + fvp-base-gicv2-psci.dtb > dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb > > targets += dtbs > diff --git a/arch/arm64/boot/dts/fvp-base-gicv2-psci.dts b/arch/arm64/boot/dts/fvp-base-gicv2-psci.dts > new file mode 100644 > index 000000000000..736f546123bb > --- /dev/null > +++ b/arch/arm64/boot/dts/fvp-base-gicv2-psci.dts > @@ -0,0 +1,233 @@ > +/* > + * Copyright (c) 2013, ARM Limited. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are met: > + * > + * Redistributions of source code must retain the above copyright notice, this > + * list of conditions and the following disclaimer. > + * > + * Redistributions in binary form must reproduce the above copyright notice, > + * this list of conditions and the following disclaimer in the documentation > + * and/or other materials provided with the distribution. > + * > + * Neither the name of ARM nor the names of its contributors may be used > + * to endorse or promote products derived from this software without specific > + * prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > + * POSSIBILITY OF SUCH DAMAGE. > + */ > + > +/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. > + > +/ { > +}; > + > +/ { > + 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. > + compatible = "arm,vfp-base", "arm,vexpress"; s/vfp/fvp/ Likewise, it would be nice to have a more specific compatible string here. 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. > + interrupt-parent = <&gic>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + chosen { }; > + > + aliases { > + serial0 = &v2m_serial0; > + serial1 = &v2m_serial1; > + serial2 = &v2m_serial2; > + serial3 = &v2m_serial3; > + }; > + > + 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. Which firmware/bootloader does this correspond to? > + > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + 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)? [...] > + 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? > + }; > + > + timer@2a810000 { > + compatible = "arm,armv7-timer-mem"; > + reg = <0x0 0x2a810000 0x0 0x10000>; > + clock-frequency = <100000000>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + frame@2a820000 { > + frame-number = <0>; > + interrupts = <0 25 4>; > + reg = <0x0 0x2a820000 0x0 0x10000>; > + }; > + }; > + > + 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. Thanks, Mark.
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.
On Wed, Dec 11, 2013 at 02:11:48PM +0000, Mark Brown wrote: > 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... When using the bootwrapper, the bootwrapper itself takes up a portion of memory (along with the spin-table) which would otherwise be available to the kernel. This is also true with my bootwrapper PSCI implementation. The memreserve tells the kernel not to stomp over this memory (though it will map it in). With a proper firmware, I'd expect the PSCI implementation to be outside of the memory exposed to non-secure software, and thus there shouldn't be anything to reserve. It's possible that there is a reason for the reservation, but I'd rather we understood it than we propagated and codified a misunderstanding. > > > > +/ { > > > + 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 :/ I'll try to chase up the issues, thanks for making me aware. I don't see the name issue as a big problem. This DT has never been part of the kernel tree, so there's no filename compatibility issue to deal with. Existing users of the DT will already have to be modified to get the DTs from a new source. There should be nothing hanging off the compatible string for the platform yet -- we have no board files or platform blobs in the arm64 port. If the model name is being used as anything other than a handy indicator to users, then that's broken anyway. > > > 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? As I understand it, "Base" is a base platform that's somewhat like vexpress, but is not intended to be an emulation of vexpress. It may be broadly equivalent, but to limit confusion I'd rather treat them separately. Regardless, top-level compatible strings aren't used for anything at the moment (and hopefully never will be), so nothing should be relying on "arm,vexpress" being present and we can remove it. > > > > + 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. So this goes with the trusted firmware? > > > Which firmware/bootloader does this correspond to? > > I'm testing using the Linaro 13.11 release. Release of what? I'm not familiar with the entire stack Linaro manage. I guess the trusted firmware is being used, as mentioned above? > > > > + 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. Ok. While I have concerns regarding the topology code, I'm not averse to describing the clock-frequency here. However I worry about how configurable clocks are going to be handled here. On the 32-bit side I see some platforms with clock-frequency and others with clocks. We should figure out what the expected way to handle configurable clocks before we add the code for handling a fixed rate clock here, or we're going to back ourselves into a corner where this support can only work on a subset of systems. > > > > + 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... I would very much hope it's unnecessary. If it's needed, then bugs should be filed and the firmware fixed. The system initialisation code _must_ set CNTFRQ on all CPUs or it's fundamentally broken. > > > > + 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. Given that the DTs on the github page don't mention specific CPUs, these interrupts might not be correct for specific implementations. I'd rather that the elements we are unsure about were dropped from the DT for the timebeing rather than being present but incorrect. There's less room for something to blow up that way, and it makes it clearer where the deficiencies are. Thanks, Mark.
On Wed, Dec 11, 2013 at 03:04:33PM +0000, Mark Rutland wrote: > On Wed, Dec 11, 2013 at 02:11:48PM +0000, Mark Brown wrote: > > 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: > > > > +/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... > When using the bootwrapper, the bootwrapper itself takes up a portion of > memory (along with the spin-table) which would otherwise be available to > the kernel. This is also true with my bootwrapper PSCI implementation. > The memreserve tells the kernel not to stomp over this memory (though it > will map it in). OK, I'll try to distill that down to a comment (and add the same comment to the other DTs). However see below... > With a proper firmware, I'd expect the PSCI implementation to be outside > of the memory exposed to non-secure software, and thus there shouldn't > be anything to reserve. > It's possible that there is a reason for the reservation, but I'd rather > we understood it than we propagated and codified a misunderstanding. It'd certainly be nice if it were clearer. > I don't see the name issue as a big problem. This DT has never been part > of the kernel tree, so there's no filename compatibility issue to deal > with. Existing users of the DT will already have to be modified to get > the DTs from a new source. I'm more worried about existing users not noticing that this is a DT for the same thing. > There should be nothing hanging off the compatible string for the > platform yet -- we have no board files or platform blobs in the arm64 > port. If the model name is being used as anything other than a handy > indicator to users, then that's broken anyway. Yeah, that doesn't seem at all risky. > > > > + 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. > So this goes with the trusted firmware? This is on the ARM github site so I'm not 100% sure how tied that is. > > > Which firmware/bootloader does this correspond to? > > I'm testing using the Linaro 13.11 release. > Release of what? I'm not familiar with the entire stack Linaro manage. > I guess the trusted firmware is being used, as mentioned above? The only release Linaro have for ARMv8 is an OE one AFAICT which I got from here: http://releases.linaro.org/13.11/openembedded/aarch64 According to the notes there it includes the trusted firmware. The above is all the information I have on it. > Ok. While I have concerns regarding the topology code, I'm not averse to > describing the clock-frequency here. > However I worry about how configurable clocks are going to be handled > here. On the 32-bit side I see some platforms with clock-frequency and > others with clocks. We should figure out what the expected way to handle > configurable clocks before we add the code for handling a fixed rate > clock here, or we're going to back ourselves into a corner where this > support can only work on a subset of systems. Well, we can always extend later. I do share your concerns about doing this using clock-frequency though, that's why I specified it as being the maximum when I copied it into the kernel bindings. That's clearly the intention of the existing ARMv7 implementation. One trick here is that this is all happening rather early in boot so going through the whole clock tree might be entertaining, though we can probably go round and do another pass when cpufreq comes up. > I'd rather that the elements we are unsure about were dropped from the > DT for the timebeing rather than being present but incorrect. There's > less room for something to blow up that way, and it makes it clearer > where the deficiencies are. OK. To be honest I'm leaning towards letting you guys upstream this given that all the information I have on it is the DT so from my point of view I'm unsure about essentially all of it.
On Wed, 2013-12-11 at 15:04 +0000, Mark Rutland wrote: > On Wed, Dec 11, 2013 at 02:11:48PM +0000, Mark Brown wrote: > > 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: [...] > > > > > > +/ { > > > > + 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 :/ > > I'll try to chase up the issues, thanks for making me aware. > > I don't see the name issue as a big problem. This DT has never been part > of the kernel tree, so there's no filename compatibility issue to deal > with. Existing users of the DT will already have to be modified to get > the DTs from a new source. > > There should be nothing hanging off the compatible string for the > platform yet -- we have no board files or platform blobs in the arm64 > port. If the model name is being used as anything other than a handy > indicator to users, then that's broken anyway. I believe Android uses model names to determine the filenames of init scripts to run. That's not a kernel problem, but thought I would point out one 'broken' use that I have first hand experience of, having been tripped up before by ARM's twice yearly lets-rename-everything-again exercise ;-)
On 11 December 2013 16:08, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > On Wed, 2013-12-11 at 15:04 +0000, Mark Rutland wrote: >> On Wed, Dec 11, 2013 at 02:11:48PM +0000, Mark Brown wrote: >> > 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: > [...] >> > >> > > > +/ { >> > > > + 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. This same file is used to boot the AEMv8 architectural model as well as the Cortex A57-A73 model, so I think someone would need to find another filename that makes sense in both contexts. I guess that using the same file for two models could in itself be a problem solved via includes and simpler wrappers. But as Mark Brown says, ARM have originated this file and personally I'd rather it was changed in the ARM Trusted Firmware repo first and propagated here. To answer another question from earlier: there is no direct correlation between the ARM Trusted Firmware and the device tree files other than the same repo hosts both files. Trusted firmware does not build or embed the DTBs. UEFI is currently what loads the DTB and passes it to the kernel. And that isn't part of the trusted firmware repo, of course. >> > >> > 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 :/ >> >> I'll try to chase up the issues, thanks for making me aware. >> >> I don't see the name issue as a big problem. This DT has never been part >> of the kernel tree, so there's no filename compatibility issue to deal >> with. Existing users of the DT will already have to be modified to get >> the DTs from a new source. >> >> There should be nothing hanging off the compatible string for the >> platform yet -- we have no board files or platform blobs in the arm64 >> port. If the model name is being used as anything other than a handy >> indicator to users, then that's broken anyway. > > I believe Android uses model names to determine the filenames of init > scripts to run. That's not a kernel problem, but thought I would point > out one 'broken' use that I have first hand experience of, having been > tripped up before by ARM's twice yearly lets-rename-everything-again > exercise ;-) > > -- > Tixy > > > > _______________________________________________ > linaro-kernel mailing list > linaro-kernel@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-kernel
On Wed, Dec 11, 2013 at 04:41:32PM +0000, Ryan Harkin wrote: > On 11 December 2013 16:08, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > > On Wed, 2013-12-11 at 15:04 +0000, Mark Rutland wrote: > >> On Wed, Dec 11, 2013 at 02:11:48PM +0000, Mark Brown wrote: > >> > 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: > > [...] > >> > > >> > > > +/ { > >> > > > + 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. > > This same file is used to boot the AEMv8 architectural model as well > as the Cortex A57-A73 model, so I think someone would need to find > another filename that makes sense in both contexts. > > I guess that using the same file for two models could in itself be a > problem solved via includes and simpler wrappers. We should have a base platform dtsi that describes the standard devices and memory map. Individual variants can include the dtsi and describe the model name and compatible string, CPUs, variant-specific devices, and firmware details. While the same DT will work regardless of cpu type currently, it's relatively easy to factor that out anyway. > > But as Mark Brown says, ARM have originated this file and personally > I'd rather it was changed in the ARM Trusted Firmware repo first and > propagated here. I completely agree that the DTs in the Trusted Firmware repo should be corrected. I will try to get them fixed. > > To answer another question from earlier: there is no direct > correlation between the ARM Trusted Firmware and the device tree files > other than the same repo hosts both files. Trusted firmware does not > build or embed the DTBs. UEFI is currently what loads the DTB and > passes it to the kernel. And that isn't part of the trusted firmware > repo, of course. There _is_ a direct correlation between the trusted firmware and the DT; the psci node describes the Trusted Firmware PSCI implementation. If you don't use the Trusted Firmware then you need a different DT. However, this would only be a platform variant built atop of the more common FVP Base, so most of the DT can be shared. Thanks, Mark.
On Wed, Dec 11, 2013 at 05:09:03PM +0000, Mark Rutland wrote: > On Wed, Dec 11, 2013 at 04:41:32PM +0000, Ryan Harkin wrote: > > But as Mark Brown says, ARM have originated this file and personally > > I'd rather it was changed in the ARM Trusted Firmware repo first and > > propagated here. > I completely agree that the DTs in the Trusted Firmware repo should be > corrected. I will try to get them fixed. And mainlined?
diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile index c52bdb051f66..2d16cdbe5d47 100644 --- a/arch/arm64/boot/dts/Makefile +++ b/arch/arm64/boot/dts/Makefile @@ -1,4 +1,6 @@ -dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb +dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb \ + foundation-v8.dtb \ + fvp-base-gicv2-psci.dtb dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb targets += dtbs diff --git a/arch/arm64/boot/dts/fvp-base-gicv2-psci.dts b/arch/arm64/boot/dts/fvp-base-gicv2-psci.dts new file mode 100644 index 000000000000..736f546123bb --- /dev/null +++ b/arch/arm64/boot/dts/fvp-base-gicv2-psci.dts @@ -0,0 +1,233 @@ +/* + * Copyright (c) 2013, ARM Limited. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * Neither the name of ARM nor the names of its contributors may be used + * to endorse or promote products derived from this software without specific + * prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +/dts-v1/; + +/memreserve/ 0x80000000 0x00010000; + +/ { +}; + +/ { + model = "FVP Base"; + compatible = "arm,vfp-base", "arm,vexpress"; + interrupt-parent = <&gic>; + #address-cells = <2>; + #size-cells = <2>; + + chosen { }; + + aliases { + serial0 = &v2m_serial0; + serial1 = &v2m_serial1; + serial2 = &v2m_serial2; + serial3 = &v2m_serial3; + }; + + psci { + compatible = "arm,psci"; + method = "smc"; + cpu_suspend = <0xc4000001>; + cpu_off = <0x84000002>; + cpu_on = <0xc4000003>; + }; + + cpus { + #address-cells = <2>; + #size-cells = <0>; + + big0: cpu@0 { + device_type = "cpu"; + compatible = "arm,cortex-a57", "arm,armv8"; + reg = <0x0 0x0>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + big1: cpu@1 { + device_type = "cpu"; + compatible = "arm,cortex-a57", "arm,armv8"; + reg = <0x0 0x1>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + big2: cpu@2 { + device_type = "cpu"; + compatible = "arm,cortex-a57", "arm,armv8"; + reg = <0x0 0x2>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + big3: cpu@3 { + device_type = "cpu"; + compatible = "arm,cortex-a57", "arm,armv8"; + reg = <0x0 0x3>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + little0: cpu@100 { + device_type = "cpu"; + compatible = "arm,cortex-a53", "arm,armv8"; + reg = <0x0 0x100>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + little1: cpu@101 { + device_type = "cpu"; + compatible = "arm,cortex-a53", "arm,armv8"; + reg = <0x0 0x101>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + little2: cpu@102 { + device_type = "cpu"; + compatible = "arm,cortex-a53", "arm,armv8"; + reg = <0x0 0x102>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + little3: cpu@103 { + device_type = "cpu"; + compatible = "arm,cortex-a53", "arm,armv8"; + reg = <0x0 0x103>; + enable-method = "psci"; + clock-frequency = <1000000>; + }; + }; + + memory@80000000 { + device_type = "memory"; + reg = <0x00000000 0x80000000 0 0x80000000>, + <0x00000008 0x80000000 0 0x80000000>; + }; + + gic: interrupt-controller@2f000000 { + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; + #interrupt-cells = <3>; + #address-cells = <0>; + interrupt-controller; + reg = <0x0 0x2f000000 0 0x10000>, + <0x0 0x2c000000 0 0x2000>, + <0x0 0x2c010000 0 0x2000>, + <0x0 0x2c02F000 0 0x2000>; + interrupts = <1 9 0xf04>; + }; + + timer { + compatible = "arm,armv8-timer"; + interrupts = <1 13 0xff01>, + <1 14 0xff01>, + <1 11 0xff01>, + <1 10 0xff01>; + clock-frequency = <100000000>; + }; + + timer@2a810000 { + compatible = "arm,armv7-timer-mem"; + reg = <0x0 0x2a810000 0x0 0x10000>; + clock-frequency = <100000000>; + #address-cells = <2>; + #size-cells = <2>; + ranges; + frame@2a820000 { + frame-number = <0>; + interrupts = <0 25 4>; + reg = <0x0 0x2a820000 0x0 0x10000>; + }; + }; + + pmu { + compatible = "arm,armv8-pmuv3"; + interrupts = <0 60 4>, + <0 61 4>, + <0 62 4>, + <0 63 4>; + }; + + smb { + compatible = "simple-bus"; + + #address-cells = <2>; + #size-cells = <1>; + ranges = <0 0 0 0x08000000 0x04000000>, + <1 0 0 0x14000000 0x04000000>, + <2 0 0 0x18000000 0x04000000>, + <3 0 0 0x1c000000 0x04000000>, + <4 0 0 0x0c000000 0x04000000>, + <5 0 0 0x10000000 0x04000000>; + + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 63>; + interrupt-map = <0 0 0 &gic 0 0 4>, + <0 0 1 &gic 0 1 4>, + <0 0 2 &gic 0 2 4>, + <0 0 3 &gic 0 3 4>, + <0 0 4 &gic 0 4 4>, + <0 0 5 &gic 0 5 4>, + <0 0 6 &gic 0 6 4>, + <0 0 7 &gic 0 7 4>, + <0 0 8 &gic 0 8 4>, + <0 0 9 &gic 0 9 4>, + <0 0 10 &gic 0 10 4>, + <0 0 11 &gic 0 11 4>, + <0 0 12 &gic 0 12 4>, + <0 0 13 &gic 0 13 4>, + <0 0 14 &gic 0 14 4>, + <0 0 15 &gic 0 15 4>, + <0 0 16 &gic 0 16 4>, + <0 0 17 &gic 0 17 4>, + <0 0 18 &gic 0 18 4>, + <0 0 19 &gic 0 19 4>, + <0 0 20 &gic 0 20 4>, + <0 0 21 &gic 0 21 4>, + <0 0 22 &gic 0 22 4>, + <0 0 23 &gic 0 23 4>, + <0 0 24 &gic 0 24 4>, + <0 0 25 &gic 0 25 4>, + <0 0 26 &gic 0 26 4>, + <0 0 27 &gic 0 27 4>, + <0 0 28 &gic 0 28 4>, + <0 0 29 &gic 0 29 4>, + <0 0 30 &gic 0 30 4>, + <0 0 31 &gic 0 31 4>, + <0 0 32 &gic 0 32 4>, + <0 0 33 &gic 0 33 4>, + <0 0 34 &gic 0 34 4>, + <0 0 35 &gic 0 35 4>, + <0 0 36 &gic 0 36 4>, + <0 0 37 &gic 0 37 4>, + <0 0 38 &gic 0 38 4>, + <0 0 39 &gic 0 39 4>, + <0 0 40 &gic 0 40 4>, + <0 0 41 &gic 0 41 4>, + <0 0 42 &gic 0 42 4>; + + /include/ "rtsm_ve-motherboard.dtsi" + }; +};