[U-Boot] [PATCH v3 4/7] ARM: switch to non-secure state during bootm execution

[Christoffer Dall <christoffer.dall@linaro.org>]

On Tue, Jul 30, 2013 at 01:32:14PM +0200, Andre Przywara wrote:
> On 07/30/2013 12:02 AM, Christoffer Dall wrote:
> >On Wed, Jul 10, 2013 at 01:54:16AM +0200, Andre Przywara wrote:
> >
> >[...]
> >
> >>diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> >>index 1b6e0ac..7b0619e 100644
> >>--- a/arch/arm/lib/bootm.c
> >>+++ b/arch/arm/lib/bootm.c
> >>@@ -34,6 +34,10 @@
> >>  #include <asm/bootm.h>
> >>  #include <linux/compiler.h>
> >>
> >>+#ifdef CONFIG_ARMV7_NONSEC
> >>+#include <asm/armv7.h>
> >>+#endif
> >>+
> >>  DECLARE_GLOBAL_DATA_PTR;
> >>
> >>  static struct tag *params;
> >>@@ -186,6 +190,29 @@ static void setup_end_tag(bd_t *bd)
> >>
> >>  __weak void setup_board_tags(struct tag **in_params) {}
> >>
> >>+static void do_nonsec_virt_switch(void)
> >>+{
> >>+#ifdef CONFIG_ARMV7_NONSEC
> >>+	int ret;
> >>+
> >>+	ret = armv7_switch_nonsec();
> >>+	switch (ret) {
> >>+	case NONSEC_VIRT_SUCCESS:
> >>+		debug("entered non-secure state\n");
> >>+		break;
> >>+	case NONSEC_ERR_NO_SEC_EXT:
> >>+		printf("nonsec: Security extensions not implemented.\n");
> >>+		break;
> >>+	case NONSEC_ERR_NO_GIC_ADDRESS:
> >>+		printf("nonsec: could not determine GIC address.\n");
> >>+		break;
> >>+	case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
> >>+		printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
> >>+		break;
> >>+	}
> >>+#endif
> >>+}
> >
> >I still don't get why you just don't make armv7_switch_nonsec a void and
> >print the error when they occur... ???
> 
> My apologies for not elaborating on these comments I didn't incorporate:
> 
> So, I don't like the idea of marrying a low-level routine with high
> level output. I don't want to constraint the usage of the routine by
> requiring an output channel. Also some parts may not be fatal for
> all users - someone could just try to switch and then behave
> differently if that failed - without bothering the user.
> May seem a bit over-engineered, but I like it better this way ;-)
> 
> If that is a show-stopper for you, I can change it, of course.
> 
I won't hold back my ack for the patch series based on this, but I do
think it's over-engineered.  I think at least just returning -1 for
error and 0 for success (or even make it a bool) and just printing a
generic error message is cleaner - the level of details as to why the
switch to hyp/nonsec didn't work could then be debug statements that a
board developer could enable with a "#define DEBUG 1" in the
corresponding file.

But ok, we've had the conversation, if you still feel this is better and
necessary, then I'll let it be.

-Christoffer