| Message ID | 1347524018-19301-5-git-send-email-t.figa@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: > Some Exynos-based boards contain secure firmware and must use firmware > operations to set up some hardware. > > This patch adds firmware operations for Exynos secure firmware and a way > for board code and device tree to specify that they must be used. > > Example of use: > > In board code: > > ...MACHINE_START(...) > /* ... */ > .init_early = exynos_firmware_init, > /* ... */ > MACHINE_END > > In device tree: > > / { > /* ... */ > > firmware { > compatible = "samsung,secure-firmware"; > }; > > /* ... */ > }; > > This is a follow-up on the patch by Kyungmin Park: > [PATCH v5 2/2] ARM: EXYNOS: SMC instruction (aka firmware) support > http://thread.gmane.org/gmane.linux.ports.arm.kernel/183608/focus=184109 > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > --- > .../devicetree/bindings/arm/samsung-boards.txt | 8 ++++ > arch/arm/mach-exynos/Makefile | 1 + > arch/arm/mach-exynos/common.h | 2 + > arch/arm/mach-exynos/firmware.c | 52 ++++++++++++++++++++++ > arch/arm/mach-exynos/mach-exynos4-dt.c | 1 + > 5 files changed, 64 insertions(+) > create mode 100644 arch/arm/mach-exynos/firmware.c > > diff --git a/Documentation/devicetree/bindings/arm/samsung-boards.txt b/Documentation/devicetree/bindings/arm/samsung-boards.txt > index 0bf68be..f447059 100644 > --- a/Documentation/devicetree/bindings/arm/samsung-boards.txt > +++ b/Documentation/devicetree/bindings/arm/samsung-boards.txt > @@ -6,3 +6,11 @@ Required root node properties: > - compatible = should be one or more of the following. > (a) "samsung,smdkv310" - for Samsung's SMDKV310 eval board. > (b) "samsung,exynos4210" - for boards based on Exynos4210 SoC. > + > +Optional: > + - firmware node, specifying presence and type of secure firmware, currently > + supported value of compatible property is "samsung,secure-firmware": > + > + firmware { > + compatible = "samsung,secure-firmware"; > + }; > diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile > index 997864b..9451637 100644 > --- a/arch/arm/mach-exynos/Makefile > +++ b/arch/arm/mach-exynos/Makefile > @@ -31,6 +31,7 @@ obj-$(CONFIG_EXYNOS4_MCT) += mct.o > obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o > > obj-$(CONFIG_ARCH_EXYNOS) += exynos-smc.o > +obj-$(CONFIG_ARCH_EXYNOS) += firmware.o > > plus_sec := $(call as-instr,.arch_extension sec,+sec) > AFLAGS_exynos-smc.o :=-Wa,-march=armv7-a$(plus_sec) > diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h > index d7f28ca..358f6a5 100644 > --- a/arch/arm/mach-exynos/common.h > +++ b/arch/arm/mach-exynos/common.h > @@ -21,6 +21,8 @@ void exynos4_restart(char mode, const char *cmd); > void exynos5_restart(char mode, const char *cmd); > void exynos_init_late(void); > > +void exynos_firmware_init(void); > + > #ifdef CONFIG_PM_GENERIC_DOMAINS > int exynos_pm_late_initcall(void); > #else > diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c > new file mode 100644 > index 0000000..3f3641d > --- /dev/null > +++ b/arch/arm/mach-exynos/firmware.c > @@ -0,0 +1,52 @@ > +/* > + * Copyright (C) 2012 Samsung Electronics. > + * Kyungmin Park <kyungmin.park@samsung.com> > + * Tomasz Figa <t.figa@samsung.com> > + * > + * This program is free software,you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/of.h> > + > +#include <asm/firmware.h> > + > +#include <mach/map.h> > + > +#include "smc.h" > + > +static int exynos_do_idle(void) > +{ > + exynos_smc(SMC_CMD_SLEEP, 0, 0, 0); > + return 0; > +} > + > +static void exynos_cpu_boot(int cpu) > +{ > + exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); > +} > + > +static void __iomem *exynos_cpu_boot_reg(int cpu) > +{ > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; > +} This communication area in sysram should probably be seen as a part of the firmware interface. It should thus be defined as part of the binding instead, i.e. through a reg property or similar there. That also would make it easy to convert to using ioremap() instead of iodesc tables, which always a nice thing. -Olof
Hi Olof, On Saturday 15 of September 2012 17:44:55 Olof Johansson wrote: > On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: > > +static void __iomem *exynos_cpu_boot_reg(int cpu) > > +{ > > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; > > +} > > This communication area in sysram should probably be seen as a part of > the firmware interface. It should thus be defined as part of the binding > instead, i.e. through a reg property or similar there. That also would > make it easy to convert to using ioremap() instead of iodesc tables, > which always a nice thing. The problem with SYSRAM_NS is that it might be also used in other code, not related to firmware only. I don't know exactly all the use cases for it. Is it really a big problem or we could let it be for now, merge the patches for firmware and then convert SYSRAM_NS to dynamic mapping when its situation clarifies? Best regards,
On Wed, Sep 19, 2012 at 3:10 AM, Tomasz Figa <t.figa@samsung.com> wrote: > Hi Olof, > > On Saturday 15 of September 2012 17:44:55 Olof Johansson wrote: >> On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: >> > +static void __iomem *exynos_cpu_boot_reg(int cpu) >> > +{ >> > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; >> > +} >> >> This communication area in sysram should probably be seen as a part of >> the firmware interface. It should thus be defined as part of the binding >> instead, i.e. through a reg property or similar there. That also would >> make it easy to convert to using ioremap() instead of iodesc tables, >> which always a nice thing. > > The problem with SYSRAM_NS is that it might be also used in other code, not > related to firmware only. I don't know exactly all the use cases for it. If you don't know the use cases, and the use cases are not in the kernel tree that we care about here (upstream), then there's really nothing to worry about. It's after all just a define that's moved to an ioremap, if there's some out of tree code that needs the old legacy define then it can be added in whatever out-of-tree code that uses it. Right? > Is it really a big problem or we could let it be for now, merge the patches > for firmware and then convert SYSRAM_NS to dynamic mapping when its > situation clarifies? What do you expect is required to clarify the situation, and when do you expect that to happen? -Olof
On 9/22/12, Olof Johansson <olof@lixom.net> wrote: > On Wed, Sep 19, 2012 at 3:10 AM, Tomasz Figa <t.figa@samsung.com> wrote: >> Hi Olof, >> >> On Saturday 15 of September 2012 17:44:55 Olof Johansson wrote: >>> On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: >>> > +static void __iomem *exynos_cpu_boot_reg(int cpu) >>> > +{ >>> > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; >>> > +} >>> >>> This communication area in sysram should probably be seen as a part of >>> the firmware interface. It should thus be defined as part of the binding >>> instead, i.e. through a reg property or similar there. That also would >>> make it easy to convert to using ioremap() instead of iodesc tables, >>> which always a nice thing. >> >> The problem with SYSRAM_NS is that it might be also used in other code, >> not >> related to firmware only. I don't know exactly all the use cases for it. > > If you don't know the use cases, and the use cases are not in the > kernel tree that we care about here (upstream), then there's really > nothing to worry about. It's after all just a define that's moved to > an ioremap, if there's some out of tree code that needs the old legacy > define then it can be added in whatever out-of-tree code that uses it. > Right? Now this address is used at cpu boot, cpuidle, inform at this time. As it touched at several places, it's defined at iodesc. if it changed with ioremap, it has to export remaped address and each codes should use it. As I wrote at cover letter, if you want to use ioremap, it can be modified. however can you merge firmware codes itself? since ioremap is not related with trustzone or firmware issues and it's exynos specific implementation issues. Right? Thank you, Kyungmin Park > >> Is it really a big problem or we could let it be for now, merge the >> patches >> for firmware and then convert SYSRAM_NS to dynamic mapping when its >> situation clarifies? > > What do you expect is required to clarify the situation, and when do > you expect that to happen? > > > -Olof > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Fri, Sep 21, 2012 at 10:57 PM, Kyungmin Park <kyungmin.park@samsung.com> wrote: > On 9/22/12, Olof Johansson <olof@lixom.net> wrote: >> On Wed, Sep 19, 2012 at 3:10 AM, Tomasz Figa <t.figa@samsung.com> wrote: >>> Hi Olof, >>> >>> On Saturday 15 of September 2012 17:44:55 Olof Johansson wrote: >>>> On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: >>>> > +static void __iomem *exynos_cpu_boot_reg(int cpu) >>>> > +{ >>>> > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; >>>> > +} >>>> >>>> This communication area in sysram should probably be seen as a part of >>>> the firmware interface. It should thus be defined as part of the binding >>>> instead, i.e. through a reg property or similar there. That also would >>>> make it easy to convert to using ioremap() instead of iodesc tables, >>>> which always a nice thing. >>> >>> The problem with SYSRAM_NS is that it might be also used in other code, >>> not >>> related to firmware only. I don't know exactly all the use cases for it. >> >> If you don't know the use cases, and the use cases are not in the >> kernel tree that we care about here (upstream), then there's really >> nothing to worry about. It's after all just a define that's moved to >> an ioremap, if there's some out of tree code that needs the old legacy >> define then it can be added in whatever out-of-tree code that uses it. >> Right? > Now this address is used at cpu boot, cpuidle, inform at this time. > As it touched at several places, it's defined at iodesc. if it changed > with ioremap, it has to export remaped address and each codes should > use it. Won't you have to push down all the references to VA_SYSRAM vs VA_SYSRAM_NS into the firmware interface anyway, since you will need to use different addresses for whether you have firmware enabled or not? I.e. you'll have a "firmware call" at the appropriate level for the non-trustzone cases that uses the equivalent of VA_SYSRAM, and for the trustzone firmware op you'll use VA_SYSRAM_NS? > As I wrote at cover letter, if you want to use ioremap, it can be > modified. however can you merge firmware codes itself? since ioremap > is not related with trustzone or firmware issues and it's exynos > specific implementation issues. Right? I'm not quite sure which part you are asking me to merge, if it's the infrastructure pieces or the exynos-specific pieces. Either way, one isn't really usable without the other, and it doesn't make sense to merge code that can't be used. Infrastructure is best merged together with the first user of it. -Olof
On 9/22/12, Olof Johansson <olof@lixom.net> wrote: > On Fri, Sep 21, 2012 at 10:57 PM, Kyungmin Park > <kyungmin.park@samsung.com> wrote: >> On 9/22/12, Olof Johansson <olof@lixom.net> wrote: >>> On Wed, Sep 19, 2012 at 3:10 AM, Tomasz Figa <t.figa@samsung.com> wrote: >>>> Hi Olof, >>>> >>>> On Saturday 15 of September 2012 17:44:55 Olof Johansson wrote: >>>>> On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: >>>>> > +static void __iomem *exynos_cpu_boot_reg(int cpu) >>>>> > +{ >>>>> > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; >>>>> > +} >>>>> >>>>> This communication area in sysram should probably be seen as a part of >>>>> the firmware interface. It should thus be defined as part of the >>>>> binding >>>>> instead, i.e. through a reg property or similar there. That also >>>>> would >>>>> make it easy to convert to using ioremap() instead of iodesc tables, >>>>> which always a nice thing. >>>> >>>> The problem with SYSRAM_NS is that it might be also used in other code, >>>> not >>>> related to firmware only. I don't know exactly all the use cases for >>>> it. >>> >>> If you don't know the use cases, and the use cases are not in the >>> kernel tree that we care about here (upstream), then there's really >>> nothing to worry about. It's after all just a define that's moved to >>> an ioremap, if there's some out of tree code that needs the old legacy >>> define then it can be added in whatever out-of-tree code that uses it. >>> Right? >> Now this address is used at cpu boot, cpuidle, inform at this time. >> As it touched at several places, it's defined at iodesc. if it changed >> with ioremap, it has to export remaped address and each codes should >> use it. > > Won't you have to push down all the references to VA_SYSRAM vs > VA_SYSRAM_NS into the firmware interface anyway, since you will need > to use different addresses for whether you have firmware enabled or > not? I.e. you'll have a "firmware call" at the appropriate level for > the non-trustzone cases that uses the equivalent of VA_SYSRAM, and for > the trustzone firmware op you'll use VA_SYSRAM_NS? Right, in case of no firmware, it uses VA_SYSRAM, but VA_SYSRAM_NS is used at firmware case. > > >> As I wrote at cover letter, if you want to use ioremap, it can be >> modified. however can you merge firmware codes itself? since ioremap >> is not related with trustzone or firmware issues and it's exynos >> specific implementation issues. Right? > > I'm not quite sure which part you are asking me to merge, if it's the > infrastructure pieces or the exynos-specific pieces. > > Either way, one isn't really usable without the other, and it doesn't > make sense to merge code that can't be used. Infrastructure is best > merged together with the first user of it. Okay, we will fix it. Thank you, Kyungmin Park
Hi Olof, On Saturday 15 of September 2012 17:44:55 Olof Johansson wrote: > On Thu, Sep 13, 2012 at 10:13:37AM +0200, Tomasz Figa wrote: > > +static void __iomem *exynos_cpu_boot_reg(int cpu) > > +{ > > + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; > > +} > > This communication area in sysram should probably be seen as a part of > the firmware interface. It should thus be defined as part of the binding > instead, i.e. through a reg property or similar there. That also would > make it easy to convert to using ioremap() instead of iodesc tables, > which always a nice thing. I have tried to get around the need of statical mapping for SYSRAM, but the firmware has to be initialized very early, before low level L2x0 cache initialization (which is an early initcall, so it has to be in init_early machine callback) and at that time ioremap is not available yet. I think we should just allow this additional static mapping, I don't see any sane way of mapping it dynamically, at least at the moment. Best regards,
diff --git a/Documentation/devicetree/bindings/arm/samsung-boards.txt b/Documentation/devicetree/bindings/arm/samsung-boards.txt index 0bf68be..f447059 100644 --- a/Documentation/devicetree/bindings/arm/samsung-boards.txt +++ b/Documentation/devicetree/bindings/arm/samsung-boards.txt @@ -6,3 +6,11 @@ Required root node properties: - compatible = should be one or more of the following. (a) "samsung,smdkv310" - for Samsung's SMDKV310 eval board. (b) "samsung,exynos4210" - for boards based on Exynos4210 SoC. + +Optional: + - firmware node, specifying presence and type of secure firmware, currently + supported value of compatible property is "samsung,secure-firmware": + + firmware { + compatible = "samsung,secure-firmware"; + }; diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile index 997864b..9451637 100644 --- a/arch/arm/mach-exynos/Makefile +++ b/arch/arm/mach-exynos/Makefile @@ -31,6 +31,7 @@ obj-$(CONFIG_EXYNOS4_MCT) += mct.o obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o obj-$(CONFIG_ARCH_EXYNOS) += exynos-smc.o +obj-$(CONFIG_ARCH_EXYNOS) += firmware.o plus_sec := $(call as-instr,.arch_extension sec,+sec) AFLAGS_exynos-smc.o :=-Wa,-march=armv7-a$(plus_sec) diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h index d7f28ca..358f6a5 100644 --- a/arch/arm/mach-exynos/common.h +++ b/arch/arm/mach-exynos/common.h @@ -21,6 +21,8 @@ void exynos4_restart(char mode, const char *cmd); void exynos5_restart(char mode, const char *cmd); void exynos_init_late(void); +void exynos_firmware_init(void); + #ifdef CONFIG_PM_GENERIC_DOMAINS int exynos_pm_late_initcall(void); #else diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c new file mode 100644 index 0000000..3f3641d --- /dev/null +++ b/arch/arm/mach-exynos/firmware.c @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2012 Samsung Electronics. + * Kyungmin Park <kyungmin.park@samsung.com> + * Tomasz Figa <t.figa@samsung.com> + * + * This program is free software,you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/of.h> + +#include <asm/firmware.h> + +#include <mach/map.h> + +#include "smc.h" + +static int exynos_do_idle(void) +{ + exynos_smc(SMC_CMD_SLEEP, 0, 0, 0); + return 0; +} + +static void exynos_cpu_boot(int cpu) +{ + exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); +} + +static void __iomem *exynos_cpu_boot_reg(int cpu) +{ + return S5P_VA_SYSRAM_NS + 0x1c + 4*cpu; +} + +static const struct firmware_ops exynos_firmware_ops __initdata = { + .do_idle = exynos_do_idle, + .cpu_boot = exynos_cpu_boot, + .cpu_boot_reg = exynos_cpu_boot_reg, +}; + +void __init exynos_firmware_init(void) +{ + if (of_have_populated_dt() && + !of_find_compatible_node(NULL, NULL, "samsung,secure-firmware")) + return; + + pr_info("Running under secure firmware.\n"); + + register_firmware_ops(&exynos_firmware_ops); +} diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c index 5c48c82..e0827b3 100644 --- a/arch/arm/mach-exynos/mach-exynos4-dt.c +++ b/arch/arm/mach-exynos/mach-exynos4-dt.c @@ -116,6 +116,7 @@ DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)") .init_irq = exynos4_init_irq, .map_io = exynos4_dt_map_io, .handle_irq = gic_handle_irq, + .init_early = exynos_firmware_init, .init_machine = exynos4_dt_machine_init, .init_late = exynos_init_late, .timer = &exynos4_timer,
Some Exynos-based boards contain secure firmware and must use firmware operations to set up some hardware. This patch adds firmware operations for Exynos secure firmware and a way for board code and device tree to specify that they must be used. Example of use: In board code: ...MACHINE_START(...) /* ... */ .init_early = exynos_firmware_init, /* ... */ MACHINE_END In device tree: / { /* ... */ firmware { compatible = "samsung,secure-firmware"; }; /* ... */ }; This is a follow-up on the patch by Kyungmin Park: [PATCH v5 2/2] ARM: EXYNOS: SMC instruction (aka firmware) support http://thread.gmane.org/gmane.linux.ports.arm.kernel/183608/focus=184109 Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Signed-off-by: Tomasz Figa <t.figa@samsung.com> --- .../devicetree/bindings/arm/samsung-boards.txt | 8 ++++ arch/arm/mach-exynos/Makefile | 1 + arch/arm/mach-exynos/common.h | 2 + arch/arm/mach-exynos/firmware.c | 52 ++++++++++++++++++++++ arch/arm/mach-exynos/mach-exynos4-dt.c | 1 + 5 files changed, 64 insertions(+) create mode 100644 arch/arm/mach-exynos/firmware.c