[U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de --- common/autoboot.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/common/autoboot.c b/common/autoboot.c index c27cc2c..4635551 100644 --- a/common/autoboot.c +++ b/common/autoboot.c @@ -12,6 +12,7 @@ #include <fdtdec.h> #include <menu.h> #include <post.h> +#include <u-boot/sha256.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -35,6 +36,11 @@ static int abortboot_keyed(int bootdelay) { int abort = 0; uint64_t etime = endtick(bootdelay); +#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) + const char *sha_env_str = getenv("bootstopkeysha256"); + u8 sha_env[SHA256_SUM_LEN]; + u8 sha[SHA256_SUM_LEN]; +#else struct { char *str; u_int len; @@ -46,10 +52,11 @@ static int abortboot_keyed(int bootdelay) { .str = getenv("bootstopkey"), .retry = 0 }, { .str = getenv("bootstopkey2"), .retry = 0 }, }; + u_int presskey_max = 0; +#endif
char presskey[MAX_DELAY_STOP_STR]; u_int presskey_len = 0; - u_int presskey_max = 0; u_int i;
#ifndef CONFIG_ZERO_BOOTDELAY_CHECK @@ -61,6 +68,41 @@ static int abortboot_keyed(int bootdelay) printf(CONFIG_AUTOBOOT_PROMPT); # endif
+#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) + if (sha_env_str == NULL) + sha_env_str = CONFIG_AUTOBOOT_STOP_STR_SHA256; + + /* + * Generate the binary value from the environment hash value + * so that we can compare this value with the computed hash + * from the user input + */ + for (i = 0; i < SHA256_SUM_LEN; i++) { + char chr[3]; + + strncpy(chr, &sha_env_str[i * 2], 2); + sha_env[i] = simple_strtoul(chr, NULL, 16); + } + + /* + * We don't know how long the stop-string is, so we need to + * generate the sha256 hash upon each input character and + * compare the value with the one saved in the environment + */ + do { + if (tstc()) { + presskey[presskey_len++] = getc(); + + /* Calculate sha256 upon each new char */ + sha256_csum_wd((unsigned char *)presskey, presskey_len, + sha, CHUNKSZ_SHA256); + + /* And check if sha matches saved value in env */ + if (memcmp(sha, sha_env, SHA256_SUM_LEN) == 0) + abort = 1; + } + } while (!abort && get_ticks() <= etime); +#else # ifdef CONFIG_AUTOBOOT_DELAY_STR if (delaykey[0].str == NULL) delaykey[0].str = CONFIG_AUTOBOOT_DELAY_STR; @@ -124,6 +166,7 @@ static int abortboot_keyed(int bootdelay) } } } while (!abort && get_ticks() <= etime); +#endif
if (!abort) debug_bootkeys("key timeout\n");
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de --- drivers/bootcount/bootcount.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/bootcount/bootcount.c b/drivers/bootcount/bootcount.c index e0343f7..b3e79de 100644 --- a/drivers/bootcount/bootcount.c +++ b/drivers/bootcount/bootcount.c @@ -57,9 +57,11 @@ __weak void bootcount_store(ulong a)
#if defined(CONFIG_SYS_BOOTCOUNT_SINGLEWORD) raw_bootcount_store(reg, (BOOTCOUNT_MAGIC & 0xffff0000) | a); + flush_dcache_range((u32)reg, (u32)reg + 4); #else raw_bootcount_store(reg, a); raw_bootcount_store(reg + 4, BOOTCOUNT_MAGIC); + flush_dcache_range((u32)reg, (u32)reg + 8); #endif }
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de
Reviewed-by: Tom Rini trini@konsulko.com
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dca che_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
Frankly, after looking at most of arch/powerpc/cpu/*/cache.c I suspect 8xx and 83xx just need the dummy files copied over.
Thanks!
... Add a few more PowerPC people.
On Fri, Mar 13, 2015 at 10:34:03AM -0400, Tom Rini wrote:
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
Frankly, after looking at most of arch/powerpc/cpu/*/cache.c I suspect 8xx and 83xx just need the dummy files copied over.
OK, I went and poked at going one direction on this and then dug into the higher level problem more. PowerPC _needs_ the current kernel's arch/powerpc/kernel/misc_32.S relevant cache functions ported over for everyone and we should kill the dummy functions we have today. To try and encourage some folks to do this I'm going to drop this bootcount patch for now.
Tom,
On 03/15/2015 11:30 AM, Tom Rini wrote:
... Add a few more PowerPC people.
On Fri, Mar 13, 2015 at 10:34:03AM -0400, Tom Rini wrote:
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
Frankly, after looking at most of arch/powerpc/cpu/*/cache.c I suspect 8xx and 83xx just need the dummy files copied over.
OK, I went and poked at going one direction on this and then dug into the higher level problem more. PowerPC _needs_ the current kernel's arch/powerpc/kernel/misc_32.S relevant cache functions ported over for everyone and we should kill the dummy functions we have today. To try and encourage some folks to do this I'm going to drop this bootcount patch for now.
FWIW, powerpc mpc83xx, mpc85xx, mpc86xx all have flush_dcache_range() function defined.
York
On Mon, Mar 16, 2015 at 08:57:02AM -0700, York Sun wrote:
Tom,
On 03/15/2015 11:30 AM, Tom Rini wrote:
... Add a few more PowerPC people.
On Fri, Mar 13, 2015 at 10:34:03AM -0400, Tom Rini wrote:
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
Frankly, after looking at most of arch/powerpc/cpu/*/cache.c I suspect 8xx and 83xx just need the dummy files copied over.
OK, I went and poked at going one direction on this and then dug into the higher level problem more. PowerPC _needs_ the current kernel's arch/powerpc/kernel/misc_32.S relevant cache functions ported over for everyone and we should kill the dummy functions we have today. To try and encourage some folks to do this I'm going to drop this bootcount patch for now.
FWIW, powerpc mpc83xx, mpc85xx, mpc86xx all have flush_dcache_range() function defined.
Yes and no: $ git grep -l flush_dcache_range arch/powerpc/ arch/powerpc/cpu/mpc512x/cache.c arch/powerpc/cpu/mpc5xxx/cache.c arch/powerpc/cpu/mpc83xx/cache.c arch/powerpc/cpu/mpc85xx/cache.c arch/powerpc/cpu/mpc86xx/cache.S arch/powerpc/cpu/ppc4xx/cache.S
Of these only ppc4xx and mpc86xx are real functions, borrowed from the kernel long long ago. The rest are dummy functions. And we should instead make everyone have the same real functions the kernel does :)
On 03/16/2015 10:05 AM, Tom Rini wrote:
8xx and 83xx just need the dummy files copied over.
OK, I went and poked at going one direction on this and then dug into the higher level problem more. PowerPC _needs_ the current kernel's arch/powerpc/kernel/misc_32.S relevant cache functions ported over for everyone and we should kill the dummy functions we have today. To try and encourage some folks to do this I'm going to drop this bootcount patch for now.
FWIW, powerpc mpc83xx, mpc85xx, mpc86xx all have flush_dcache_range() function defined.
Yes and no: $ git grep -l flush_dcache_range arch/powerpc/ arch/powerpc/cpu/mpc512x/cache.c arch/powerpc/cpu/mpc5xxx/cache.c arch/powerpc/cpu/mpc83xx/cache.c arch/powerpc/cpu/mpc85xx/cache.c arch/powerpc/cpu/mpc86xx/cache.S arch/powerpc/cpu/ppc4xx/cache.S
Of these only ppc4xx and mpc86xx are real functions, borrowed from the kernel long long ago. The rest are dummy functions. And we should instead make everyone have the same real functions the kernel does :)
Ah! I was under the impression we use it. But we actually used flush_cache() function.
York
On Mon, Mar 16, 2015 at 10:11:17AM -0700, York Sun wrote:
On 03/16/2015 10:05 AM, Tom Rini wrote:
8xx and 83xx just need the dummy files copied over.
OK, I went and poked at going one direction on this and then dug into the higher level problem more. PowerPC _needs_ the current kernel's arch/powerpc/kernel/misc_32.S relevant cache functions ported over for everyone and we should kill the dummy functions we have today. To try and encourage some folks to do this I'm going to drop this bootcount patch for now.
FWIW, powerpc mpc83xx, mpc85xx, mpc86xx all have flush_dcache_range() function defined.
Yes and no: $ git grep -l flush_dcache_range arch/powerpc/ arch/powerpc/cpu/mpc512x/cache.c arch/powerpc/cpu/mpc5xxx/cache.c arch/powerpc/cpu/mpc83xx/cache.c arch/powerpc/cpu/mpc85xx/cache.c arch/powerpc/cpu/mpc86xx/cache.S arch/powerpc/cpu/ppc4xx/cache.S
Of these only ppc4xx and mpc86xx are real functions, borrowed from the kernel long long ago. The rest are dummy functions. And we should instead make everyone have the same real functions the kernel does :)
Ah! I was under the impression we use it. But we actually used flush_cache() function.
Yeah, there's some related cleanup that should be done too :)
Hi Tom,
On 03/13/2015 03:34 PM, Tom Rini wrote:
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
we still would like to keep mgcoge and mgcoge3ne support. These boards are still in maintenance. Unfortunately this week we are very busy. Next week Valentin or myself have planned to find some time to look at this.
Regards Holger
Hi!
(added a few more people to the Cc list)
On 17.03.2015 10:00, Holger Brunck wrote:
Hi Tom,
On 03/13/2015 03:34 PM, Tom Rini wrote:
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
we still would like to keep mgcoge and mgcoge3ne support. These boards are still in maintenance. Unfortunately this week we are very busy. Next week Valentin or myself have planned to find some time to look at this.
I just yesterday noticed this code for mx6:
------------------------- arch/arm/cpu/armv7/mx6/soc.c ------------------------- void enable_caches(void) { ... /* Enable caching on OCRAM and ROM */ mmu_set_region_dcache_behaviour(ROMCP_ARB_BASE_ADDR, ROMCP_ARB_END_ADDR, option); mmu_set_region_dcache_behaviour(IRAM_BASE_ADDR, IRAM_SIZE, option);
So we definitely have the dcache enabled on mx6 in the OCRAM. And this of course explains, why I need the cache flush operations in the bootcounter code.
I'm not really sure if we want this area to be cached though. This got introduced with this patch:
Author: Nitin Garg nitin.garg@freescale.com 2014-09-16 20:33:25 Committer: Stefano Babic sbabic@denx.de 2014-09-22 16:21:04
imx: Support i.MX6 High Assurance Boot authentication
When CONFIG_SECURE_BOOT is enabled, the signed images like kernel and dtb can be authenticated using iMX6 CAAM. The added command hab_auth_img can be used for HAB authentication of images. The command takes the image DDR location, IVT (Image Vector Table) offset inside image as parameters. Detailed info about signing images can be found in Freescale AppNote AN4581.
Signed-off-by: Nitin Garg nitin.garg@freescale.com
The cache stuff is not mentioned here in this commit log. Nitin, why did you enable the cache here? Performance reason only?
If the OCRAM was uncached (again), we could really drop my patch (this mail thread) with those flush calls. So how should we proceed? Make this OCRAM area uncached again?
Comments welcome...
Thanks, Stefan
BTW: I'm on vacation now until 8th April, so please don't expect any replies in this time.
Hi Stefan, On 03/27/2015 08:18 AM, Stefan Roese wrote:
Hi!
(added a few more people to the Cc list)
On 17.03.2015 10:00, Holger Brunck wrote:
Hi Tom,
On 03/13/2015 03:34 PM, Tom Rini wrote:
On Fri, Mar 13, 2015 at 09:48:56AM -0400, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:38AM +0100, Stefan Roese wrote:
Without this dcache_flush the updated bootcounter may not be saved to its location.
This was detected on an iMX.6 platform using the OCRAM (internal SRAM) as bootcounter storage area. And issuing "reset" from within U-Boot cause the bootcounter to stay on its initial value.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
OK, this breaks some platforms: powerpc: + TQM850L +(TQM850L) drivers/built-in.o: In function `bootcount_store': +(TQM850L) build/../drivers/bootcount/bootcount.c:64: undefined reference to `flush_dcache_range' +(TQM850L) make[1]: *** [u-boot] Error 1 +(TQM850L) make: *** [sub-make] Error 2
We'll see how many others have the same problem soon and then I'll decide on nuking the old platforms of holding off on this change.
Aside from the TQM 8xx family that Wolfgang owns we have mgcoge and mgcoge3ne also breaking from this (http://patchwork.ozlabs.org/patch/448849/) change. Wolfgang, Holger, how do you want to proceed? We either need cache operations or dropping bootcount from the platforms or dropping the platforms.
we still would like to keep mgcoge and mgcoge3ne support. These boards are still in maintenance. Unfortunately this week we are very busy. Next week Valentin or myself have planned to find some time to look at this.
I just yesterday noticed this code for mx6:
------------------------- arch/arm/cpu/armv7/mx6/soc.c ------------------------- void enable_caches(void) { ... /* Enable caching on OCRAM and ROM */ mmu_set_region_dcache_behaviour(ROMCP_ARB_BASE_ADDR, ROMCP_ARB_END_ADDR, option); mmu_set_region_dcache_behaviour(IRAM_BASE_ADDR, IRAM_SIZE, option);
So we definitely have the dcache enabled on mx6 in the OCRAM. And this of course explains, why I need the cache flush operations in the bootcounter code.
I'm not really sure if we want this area to be cached though. This got introduced with this patch:
Author: Nitin Garg nitin.garg@freescale.com 2014-09-16 20:33:25 Committer: Stefano Babic sbabic@denx.de 2014-09-22 16:21:04
imx: Support i.MX6 High Assurance Boot authentication When CONFIG_SECURE_BOOT is enabled, the signed images like kernel and dtb can be authenticated using iMX6 CAAM. The added command hab_auth_img can be used for HAB authentication of images. The command takes the image DDR location, IVT (Image Vector Table) offset inside image as parameters. Detailed info about signing images can be found in Freescale AppNote AN4581. Signed-off-by: Nitin Garg <nitin.garg@freescale.com>The cache stuff is not mentioned here in this commit log. Nitin, why did you enable the cache here? Performance reason only?
Seems like I missed mentioning this in the commit log, sorry. We enabled the OCRAM d-cache since it helped HAB to execute faster when authenticating OS image. The time taken to complete HAB improved by 3 times with d-cache enabled for ROM and OCRAM. This is because the HAB uses various regions of the OCRAM for data.
If the OCRAM was uncached (again), we could really drop my patch (this mail thread) with those flush calls. So how should we proceed? Make this OCRAM area uncached again?
Comments welcome...
Thanks, Stefan
BTW: I'm on vacation now until 8th April, so please don't expect any replies in this time.
Regards, Nitin
This patch defines the 2 flush_dcache_range and invalidate_dcache_range functions for all the powerpc architecture. Their implementation is borrowed from the kernel's misc_32.S file and replace the ones from mpc86xx and ppc4xx since they were equivalent.
This is a fix for the problem introduced by this patch: http://patchwork.ozlabs.org/patch/448849/
Signed-off-by: Valentin Longchamp valentin.longchamp@keymile.com --- arch/powerpc/cpu/mpc512x/Makefile | 3 --- arch/powerpc/cpu/mpc512x/cache.c | 17 -------------- arch/powerpc/cpu/mpc5xxx/Makefile | 1 - arch/powerpc/cpu/mpc5xxx/cache.c | 15 ------------ arch/powerpc/cpu/mpc83xx/Makefile | 3 --- arch/powerpc/cpu/mpc83xx/cache.c | 17 -------------- arch/powerpc/cpu/mpc85xx/Makefile | 3 --- arch/powerpc/cpu/mpc85xx/cache.c | 17 -------------- arch/powerpc/cpu/mpc86xx/cache.S | 45 ------------------------------------ arch/powerpc/cpu/ppc4xx/cache.S | 43 ----------------------------------- arch/powerpc/lib/ppccache.S | 48 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 48 insertions(+), 164 deletions(-) delete mode 100644 arch/powerpc/cpu/mpc512x/cache.c delete mode 100644 arch/powerpc/cpu/mpc5xxx/cache.c delete mode 100644 arch/powerpc/cpu/mpc83xx/cache.c delete mode 100644 arch/powerpc/cpu/mpc85xx/cache.c
diff --git a/arch/powerpc/cpu/mpc512x/Makefile b/arch/powerpc/cpu/mpc512x/Makefile index a4934ef..98991c6 100644 --- a/arch/powerpc/cpu/mpc512x/Makefile +++ b/arch/powerpc/cpu/mpc512x/Makefile @@ -17,6 +17,3 @@ obj-y += speed.o obj-$(CONFIG_FSL_DIU_FB) += diu.o obj-$(CONFIG_CMD_IDE) += ide.o obj-$(CONFIG_PCI) += pci.o - -# Stub implementations of cache management functions for USB -obj-$(CONFIG_USB_EHCI) += cache.o diff --git a/arch/powerpc/cpu/mpc512x/cache.c b/arch/powerpc/cpu/mpc512x/cache.c deleted file mode 100644 index 66384f9..0000000 --- a/arch/powerpc/cpu/mpc512x/cache.c +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright (C) 2012 Marek Vasut marex@denx.de - * - * This file contains stub implementation of - * invalidate_dcache_range() - * flush_dcache_range() - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -void invalidate_dcache_range(unsigned long start, unsigned long stop) -{ -} - -void flush_dcache_range(unsigned long start, unsigned long stop) -{ -} diff --git a/arch/powerpc/cpu/mpc5xxx/Makefile b/arch/powerpc/cpu/mpc5xxx/Makefile index d122b29..5c67e1d 100644 --- a/arch/powerpc/cpu/mpc5xxx/Makefile +++ b/arch/powerpc/cpu/mpc5xxx/Makefile @@ -7,7 +7,6 @@
extra-y = start.o extra-y += traps.o -obj-y += cache.o obj-y += io.o obj-y += firmware_sc_task_bestcomm.impl.o obj-y += i2c.o diff --git a/arch/powerpc/cpu/mpc5xxx/cache.c b/arch/powerpc/cpu/mpc5xxx/cache.c deleted file mode 100644 index 5d674bc..0000000 --- a/arch/powerpc/cpu/mpc5xxx/cache.c +++ /dev/null @@ -1,15 +0,0 @@ -/* - * This file contains stub implementation of - * invalidate_dcache_range() - * flush_dcache_range() - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -void invalidate_dcache_range(unsigned long start, unsigned long stop) -{ -} - -void flush_dcache_range(unsigned long start, unsigned long stop) -{ -} diff --git a/arch/powerpc/cpu/mpc83xx/Makefile b/arch/powerpc/cpu/mpc83xx/Makefile index cf91162..a93cf13 100644 --- a/arch/powerpc/cpu/mpc83xx/Makefile +++ b/arch/powerpc/cpu/mpc83xx/Makefile @@ -35,9 +35,6 @@ obj-$(CONFIG_PCI) += pci.o obj-$(CONFIG_PCIE) += pcie.o obj-$(CONFIG_OF_LIBFDT) += fdt.o
-# Stub implementations of cache management functions for USB -obj-y += cache.o - ifndef CONFIG_SYS_FSL_DDRC_GEN2 obj-y += spd_sdram.o endif diff --git a/arch/powerpc/cpu/mpc83xx/cache.c b/arch/powerpc/cpu/mpc83xx/cache.c deleted file mode 100644 index 66384f9..0000000 --- a/arch/powerpc/cpu/mpc83xx/cache.c +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright (C) 2012 Marek Vasut marex@denx.de - * - * This file contains stub implementation of - * invalidate_dcache_range() - * flush_dcache_range() - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -void invalidate_dcache_range(unsigned long start, unsigned long stop) -{ -} - -void flush_dcache_range(unsigned long start, unsigned long stop) -{ -} diff --git a/arch/powerpc/cpu/mpc85xx/Makefile b/arch/powerpc/cpu/mpc85xx/Makefile index b93158b..65c26c0 100644 --- a/arch/powerpc/cpu/mpc85xx/Makefile +++ b/arch/powerpc/cpu/mpc85xx/Makefile @@ -114,7 +114,4 @@ endif obj-y += tlb.o obj-y += traps.o
-# Stub implementations of cache management functions for USB -obj-y += cache.o - endif # not minimal diff --git a/arch/powerpc/cpu/mpc85xx/cache.c b/arch/powerpc/cpu/mpc85xx/cache.c deleted file mode 100644 index 66384f9..0000000 --- a/arch/powerpc/cpu/mpc85xx/cache.c +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright (C) 2012 Marek Vasut marex@denx.de - * - * This file contains stub implementation of - * invalidate_dcache_range() - * flush_dcache_range() - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -void invalidate_dcache_range(unsigned long start, unsigned long stop) -{ -} - -void flush_dcache_range(unsigned long start, unsigned long stop) -{ -} diff --git a/arch/powerpc/cpu/mpc86xx/cache.S b/arch/powerpc/cpu/mpc86xx/cache.S index 536d9b9..34968c6 100644 --- a/arch/powerpc/cpu/mpc86xx/cache.S +++ b/arch/powerpc/cpu/mpc86xx/cache.S @@ -115,51 +115,6 @@ _GLOBAL(clean_dcache_range) blr
/* - * Write any modified data cache blocks out to memory - * and invalidate the corresponding instruction cache blocks. - * - * flush_dcache_range(unsigned long start, unsigned long stop) - */ -_GLOBAL(flush_dcache_range) - li r5,CACHE_LINE_SIZE-1 - andc r3,r3,r5 - subf r4,r3,r4 - add r4,r4,r5 - srwi. r4,r4,LG_CACHE_LINE_SIZE - beqlr - mtctr r4 - - sync -1: dcbf 0,r3 - addi r3,r3,CACHE_LINE_SIZE - bdnz 1b - sync /* wait for dcbf's to get to ram */ - blr - -/* - * Like above, but invalidate the D-cache. This is used by the 8xx - * to invalidate the cache so the PPC core doesn't get stale data - * from the CPM (no cache snooping here :-). - * - * invalidate_dcache_range(unsigned long start, unsigned long stop) - */ -_GLOBAL(invalidate_dcache_range) - li r5,CACHE_LINE_SIZE-1 - andc r3,r3,r5 - subf r4,r3,r4 - add r4,r4,r5 - srwi. r4,r4,LG_CACHE_LINE_SIZE - beqlr - mtctr r4 - - sync -1: dcbi 0,r3 - addi r3,r3,CACHE_LINE_SIZE - bdnz 1b - sync /* wait for dcbi's to get to ram */ - blr - -/* * Flush a particular page from the data cache to RAM. * Note: this is necessary because the instruction cache does *not* * snoop from the data cache. diff --git a/arch/powerpc/cpu/ppc4xx/cache.S b/arch/powerpc/cpu/ppc4xx/cache.S index 2714c2f..93e8366 100644 --- a/arch/powerpc/cpu/ppc4xx/cache.S +++ b/arch/powerpc/cpu/ppc4xx/cache.S @@ -74,49 +74,6 @@ _GLOBAL(clean_dcache_range) blr
/* - * Write any modified data cache blocks out to memory and invalidate them. - * Does not invalidate the corresponding instruction cache blocks. - * - * flush_dcache_range(unsigned long start, unsigned long stop) - */ -_GLOBAL(flush_dcache_range) - li r5,L1_CACHE_BYTES-1 - andc r3,r3,r5 - subf r4,r3,r4 - add r4,r4,r5 - srwi. r4,r4,L1_CACHE_SHIFT - beqlr - mtctr r4 - -1: dcbf 0,r3 - addi r3,r3,L1_CACHE_BYTES - bdnz 1b - sync /* wait for dcbst's to get to ram */ - blr - -/* - * Like above, but invalidate the D-cache. This is used by the 8xx - * to invalidate the cache so the PPC core doesn't get stale data - * from the CPM (no cache snooping here :-). - * - * invalidate_dcache_range(unsigned long start, unsigned long stop) - */ -_GLOBAL(invalidate_dcache_range) - li r5,L1_CACHE_BYTES-1 - andc r3,r3,r5 - subf r4,r3,r4 - add r4,r4,r5 - srwi. r4,r4,L1_CACHE_SHIFT - beqlr - mtctr r4 - -1: dcbi 0,r3 - addi r3,r3,L1_CACHE_BYTES - bdnz 1b - sync /* wait for dcbi's to get to ram */ - blr - -/* * 40x cores have 8K or 16K dcache and 32 byte line size. * 44x has a 32K dcache and 32 byte line size. * 8xx has 1, 2, 4, 8K variants. diff --git a/arch/powerpc/lib/ppccache.S b/arch/powerpc/lib/ppccache.S index 349a1c1..b96dbc6 100644 --- a/arch/powerpc/lib/ppccache.S +++ b/arch/powerpc/lib/ppccache.S @@ -9,6 +9,9 @@
#include <config.h> #include <ppc_asm.tmpl> +#include <ppc_defs.h> + +#include <asm/cache.h>
/*------------------------------------------------------------------------------- */ /* Function: ppcDcbf */ @@ -54,3 +57,48 @@ ppcDcbz: ppcSync: sync blr + +/* + * Write any modified data cache blocks out to memory and invalidate them. + * Does not invalidate the corresponding instruction cache blocks. + * + * flush_dcache_range(unsigned long start, unsigned long stop) + */ +_GLOBAL(flush_dcache_range) + li r5,L1_CACHE_BYTES-1 + andc r3,r3,r5 + subf r4,r3,r4 + add r4,r4,r5 + srwi. r4,r4,L1_CACHE_SHIFT + beqlr + mtctr r4 + +1: dcbf 0,r3 + addi r3,r3,L1_CACHE_BYTES + bdnz 1b + sync /* wait for dcbst's to get to ram */ + blr + +/* + * Like above, but invalidate the D-cache. This is used by the 8xx + * to invalidate the cache so the PPC core doesn't get stale data + * from the CPM (no cache snooping here :-). + * + * invalidate_dcache_range(unsigned long start, unsigned long stop) + */ +_GLOBAL(invalidate_dcache_range) + li r5,L1_CACHE_BYTES-1 + andc r3,r3,r5 + subf r4,r3,r4 + add r4,r4,r5 + srwi. r4,r4,L1_CACHE_SHIFT + beqlr + mtctr r4 + + sync +1: dcbi 0,r3 + addi r3,r3,L1_CACHE_BYTES + bdnz 1b + sync /* wait for dcbi's to get to ram */ + blr +
On Fri, Mar 27, 2015 at 04:07:32PM +0100, Valentin Longchamp wrote:
This patch defines the 2 flush_dcache_range and invalidate_dcache_range functions for all the powerpc architecture. Their implementation is borrowed from the kernel's misc_32.S file and replace the ones from mpc86xx and ppc4xx since they were equivalent.
This is a fix for the problem introduced by this patch: http://patchwork.ozlabs.org/patch/448849/
Signed-off-by: Valentin Longchamp valentin.longchamp@keymile.com
Reviewed-by: Tom Rini trini@konsulko.com
On 03/27/2015 08:07 AM, Valentin Longchamp wrote:
This patch defines the 2 flush_dcache_range and invalidate_dcache_range functions for all the powerpc architecture. Their implementation is borrowed from the kernel's misc_32.S file and replace the ones from mpc86xx and ppc4xx since they were equivalent.
This is a fix for the problem introduced by this patch: http://patchwork.ozlabs.org/patch/448849/
Signed-off-by: Valentin Longchamp valentin.longchamp@keymile.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot-mpc85xx master. Awaiting upstream.
York
This patch extends the U-Boot "led" command to support automatic blinking by setting a blink frequency in milliseconds. Additionally the number of supported LEDs is increased to 6 (0...5).
This will be used by the PCA9551 LED driver.
Signed-off-by: Stefan Roese sr@denx.de --- common/cmd_led.c | 48 +++++++++++++++++++++++++++++++++++++---------- drivers/misc/status_led.c | 14 ++++++++++++++ include/status_led.h | 1 + 3 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/common/cmd_led.c b/common/cmd_led.c index 172bc30..b0f1a61 100644 --- a/common/cmd_led.c +++ b/common/cmd_led.c @@ -39,6 +39,12 @@ static const led_tbl_t led_commands[] = { #ifdef STATUS_LED_BIT3 { "3", STATUS_LED_BIT3, NULL, NULL, NULL }, #endif +#ifdef STATUS_LED_BIT4 + { "4", STATUS_LED_BIT4, NULL, NULL, NULL }, +#endif +#ifdef STATUS_LED_BIT5 + { "5", STATUS_LED_BIT5, NULL, NULL, NULL }, +#endif #endif #ifdef STATUS_LED_GREEN { "green", STATUS_LED_GREEN, green_led_off, green_led_on, NULL }, @@ -55,30 +61,39 @@ static const led_tbl_t led_commands[] = { { NULL, 0, NULL, NULL, NULL } };
-enum led_cmd { LED_ON, LED_OFF, LED_TOGGLE }; +enum led_cmd { LED_ON, LED_OFF, LED_TOGGLE, LED_BLINK };
enum led_cmd get_led_cmd(char *var) { - if (strcmp(var, "off") == 0) { + if (strcmp(var, "off") == 0) return LED_OFF; - } - if (strcmp(var, "on") == 0) { + if (strcmp(var, "on") == 0) return LED_ON; - } if (strcmp(var, "toggle") == 0) return LED_TOGGLE; + if (strcmp(var, "blink") == 0) + return LED_BLINK; + return -1; }
+/* + * LED drivers providing a blinking LED functionality, like the + * PCA9551, can override this empty weak function + */ +void __weak __led_blink(led_id_t mask, int freq) +{ +} + int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { int i, match = 0; enum led_cmd cmd; + int freq;
/* Validate arguments */ - if ((argc != 3)) { + if ((argc < 3) || (argc > 4)) return CMD_RET_USAGE; - }
cmd = get_led_cmd(argv[2]); if (cmd < 0) { @@ -109,6 +124,13 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) led_commands[i].toggle(); else __led_toggle(led_commands[i].mask); + break; + case LED_BLINK: + if (argc != 4) + return CMD_RET_USAGE; + + freq = simple_strtoul(argv[3], NULL, 10); + __led_blink(led_commands[i].mask, freq); } /* Need to set only 1 led if led_name wasn't 'all' */ if (strcmp("all", argv[1]) != 0) @@ -125,7 +147,7 @@ int do_led (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
U_BOOT_CMD( - led, 3, 1, do_led, + led, 4, 1, do_led, "[" #ifdef CONFIG_BOARD_SPECIFIC_LED #ifdef STATUS_LED_BIT @@ -140,6 +162,12 @@ U_BOOT_CMD( #ifdef STATUS_LED_BIT3 "3|" #endif +#ifdef STATUS_LED_BIT4 + "4|" +#endif +#ifdef STATUS_LED_BIT5 + "5|" +#endif #endif #ifdef STATUS_LED_GREEN "green|" @@ -153,6 +181,6 @@ U_BOOT_CMD( #ifdef STATUS_LED_BLUE "blue|" #endif - "all] [on|off|toggle]", - "[led_name] [on|off|toggle] sets or clears led(s)" + "all] [on|off|toggle|blink] [blink-freq in ms]", + "[led_name] [on|off|toggle|blink] sets or clears led(s)" ); diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c index ed9adb2..9869d98 100644 --- a/drivers/misc/status_led.c +++ b/drivers/misc/status_led.c @@ -53,6 +53,20 @@ led_dev_t led_dev[] = { 0, }, #endif +#if defined(STATUS_LED_BIT4) + { STATUS_LED_BIT4, + STATUS_LED_STATE4, + STATUS_LED_PERIOD4, + 0, + }, +#endif +#if defined(STATUS_LED_BIT5) + { STATUS_LED_BIT5, + STATUS_LED_STATE5, + STATUS_LED_PERIOD5, + 0, + }, +#endif };
#define MAX_LED_DEV (sizeof(led_dev)/sizeof(led_dev_t)) diff --git a/include/status_led.h b/include/status_led.h index 27f4bdf..a5e35df 100644 --- a/include/status_led.h +++ b/include/status_led.h @@ -105,6 +105,7 @@ typedef unsigned long led_id_t; extern void __led_toggle (led_id_t mask); extern void __led_init (led_id_t mask, int state); extern void __led_set (led_id_t mask, int state); +void __led_blink(led_id_t mask, int freq); #else # error Status LED configuration missing #endif
On Wed, Mar 11, 2015 at 09:51:39AM +0100, Stefan Roese wrote:
This patch extends the U-Boot "led" command to support automatic blinking by setting a blink frequency in milliseconds. Additionally the number of supported LEDs is increased to 6 (0...5).
This will be used by the PCA9551 LED driver.
Signed-off-by: Stefan Roese sr@denx.de
Reviewed-by: Tom Rini trini@konsulko.com
On Wed, Mar 11, 2015 at 09:51:39AM +0100, Stefan Roese wrote:
This patch extends the U-Boot "led" command to support automatic blinking by setting a blink frequency in milliseconds. Additionally the number of supported LEDs is increased to 6 (0...5).
This will be used by the PCA9551 LED driver.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
This patch adds a driver for the PCA9551 LED controller.
Originated-by: Timo Herbrecher t.herbrecher@gateware.de Signed-off-by: Stefan Roese sr@denx.de --- drivers/misc/Kconfig | 14 +++++ drivers/misc/Makefile | 1 + drivers/misc/pca9551_led.c | 147 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 162 insertions(+) create mode 100644 drivers/misc/pca9551_led.c
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 36a8f0d..b8884ec 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -61,3 +61,17 @@ config CONFIG_FSL_SEC_MON system states. Security Monitor can be transitioned on any security failures, like software violations or hardware security violations. + +config PCA9551_LED + bool "Enable PCA9551 LED driver" + depends on TARGET_TQMA6 + help + Enable driver for PCA9551 LED controller. This controller + is connected via I2C. So I2C needs to be enabled. + +config PCA9551_I2C_ADDR + hex "I2C address of PCA9551 LED controller" + depends on PCA9551_LED + default 0x60 + help + The I2C address of the PCA9551 LED controller. diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 6028cd4..149e686 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -29,3 +29,4 @@ obj-$(CONFIG_STATUS_LED) += status_led.o obj-$(CONFIG_TWL4030_LED) += twl4030_led.o obj-$(CONFIG_FSL_IFC) += fsl_ifc.o obj-$(CONFIG_FSL_SEC_MON) += fsl_sec_mon.o +obj-$(CONFIG_PCA9551_LED) += pca9551_led.o diff --git a/drivers/misc/pca9551_led.c b/drivers/misc/pca9551_led.c new file mode 100644 index 0000000..273cc53 --- /dev/null +++ b/drivers/misc/pca9551_led.c @@ -0,0 +1,147 @@ +/* + * Copyright (C) 2015 Stefan Roese sr@denx.de + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <i2c.h> + +#ifndef CONFIG_PCA9551_I2C_ADDR +#error "CONFIG_PCA9551_I2C_ADDR not defined!" +#endif + +#define PCA9551_REG_INPUT 0x00 /* Input register (read only) */ +#define PCA9551_REG_PSC0 0x01 /* Frequency prescaler 0 */ +#define PCA9551_REG_PWM0 0x02 /* PWM0 */ +#define PCA9551_REG_PSC1 0x03 /* Frequency prescaler 1 */ +#define PCA9551_REG_PWM1 0x04 /* PWM1 */ +#define PCA9551_REG_LS0 0x05 /* LED0 to LED3 selector */ +#define PCA9551_REG_LS1 0x06 /* LED4 to LED7 selector */ + +#define PCA9551_CTRL_AI (1 << 4) /* Auto-increment flag */ + +#define PCA9551_LED_STATE_ON 0x00 +#define PCA9551_LED_STATE_OFF 0x01 +#define PCA9551_LED_STATE_BLINK0 0x02 +#define PCA9551_LED_STATE_BLINK1 0x03 + +struct pca9551_blink_rate { + u8 psc; /* Frequency preescaler, see PCA9551_7.pdf p. 6 */ + u8 pwm; /* Pulse width modulation, see PCA9551_7.pdf p. 6 */ +} __packed; + +static int freq0, freq1; + +static int pca9551_led_get_state(int led, int *state) +{ + unsigned int reg; + u8 shift, buf; + + if (led < 0 || led > 7) { + return -1; + } else if (led < 4) { + reg = PCA9551_REG_LS0; + shift = led << 1; + } else { + reg = PCA9551_REG_LS1; + shift = (led - 4) << 1; + } + + if (i2c_read(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1)) + return -1; + + *state = (buf >> shift) & 0x03; + return 0; +} + +static int pca9551_led_set_state(int led, int state) +{ + unsigned int reg; + u8 shift, buf, mask; + + if (led < 0 || led > 7) { + return -1; + } else if (led < 4) { + reg = PCA9551_REG_LS0; + shift = led << 1; + } else { + reg = PCA9551_REG_LS1; + shift = (led - 4) << 1; + } + mask = 0x03 << shift; + + if (i2c_read(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1)) + return -1; + + buf = (buf & ~mask) | ((state & 0x03) << shift); + + if (i2c_write(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1)) + return -1; + + return 0; +} + +static int pca9551_led_set_blink_rate(int idx, struct pca9551_blink_rate rate) +{ + unsigned int reg; + + switch (idx) { + case 0: + reg = PCA9551_REG_PSC0; + break; + case 1: + reg = PCA9551_REG_PSC1; + break; + default: + return -1; + } + reg |= PCA9551_CTRL_AI; + + if (i2c_write(CONFIG_PCA9551_I2C_ADDR, reg, 1, (u8 *)&rate, 2)) + return -1; + + return 0; +} + +/* + * Functions referenced by cmd_led.c + */ +void __led_set(led_id_t mask, int state) +{ + if (state == STATUS_LED_OFF) + pca9551_led_set_state(mask, PCA9551_LED_STATE_OFF); + else + pca9551_led_set_state(mask, PCA9551_LED_STATE_ON); +} + +void __led_toggle(led_id_t mask) +{ + int state = 0; + + pca9551_led_get_state(mask, &state); + pca9551_led_set_state(mask, !state); +} + +void __led_blink(led_id_t mask, int freq) +{ + struct pca9551_blink_rate rate; + int mode; + int blink; + + if ((freq0 == 0) || (freq == freq0)) { + blink = 0; + mode = PCA9551_LED_STATE_BLINK0; + freq0 = freq; + } else { + blink = 1; + mode = PCA9551_LED_STATE_BLINK1; + freq1 = freq; + } + + rate.psc = ((freq * 38) / 1000) - 1; + rate.pwm = 128; /* 50% duty cycle */ + + pca9551_led_set_blink_rate(blink, rate); + pca9551_led_set_state(mask, mode); +}
On Wed, Mar 11, 2015 at 09:51:40AM +0100, Stefan Roese wrote:
This patch adds a driver for the PCA9551 LED controller.
Originated-by: Timo Herbrecher t.herbrecher@gateware.de Signed-off-by: Stefan Roese sr@denx.de
Reviewed-by: Tom Rini trini@konsulko.com
But:
+struct pca9551_blink_rate {
- u8 psc; /* Frequency preescaler, see PCA9551_7.pdf p. 6 */
- u8 pwm; /* Pulse width modulation, see PCA9551_7.pdf p. 6 */
+} __packed;
Yes, really, this must be packed because otherwise we get unwanted padding in between these u8s?
Hi Stefan,
On Wed, Mar 11, 2015 at 5:51 AM, Stefan Roese sr@denx.de wrote:
+config PCA9551_LED
bool "Enable PCA9551 LED driver"depends on TARGET_TQMA6
Is this 'depends' correct? We should be able to use this driver with other boards.
+static int pca9551_led_get_state(int led, int *state) +{
unsigned int reg;u8 shift, buf;if (led < 0 || led > 7) {return -1;} else if (led < 4) {reg = PCA9551_REG_LS0;shift = led << 1;} else {reg = PCA9551_REG_LS1;shift = (led - 4) << 1;}if (i2c_read(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1))return -1;
what about:
ret = i2c_read(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1); if (ret) return ret;
*state = (buf >> shift) & 0x03;return 0;+}
+static int pca9551_led_set_state(int led, int state) +{
unsigned int reg;u8 shift, buf, mask;if (led < 0 || led > 7) {return -1;
return -EINVAL;
} else if (led < 4) {reg = PCA9551_REG_LS0;shift = led << 1;} else {reg = PCA9551_REG_LS1;shift = (led - 4) << 1;}mask = 0x03 << shift;if (i2c_read(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1))return -1;
Same here.
buf = (buf & ~mask) | ((state & 0x03) << shift);if (i2c_write(CONFIG_PCA9551_I2C_ADDR, reg, 1, &buf, 1))return -1;
Same here.
return 0;+}
+static int pca9551_led_set_blink_rate(int idx, struct pca9551_blink_rate rate) +{
unsigned int reg;switch (idx) {case 0:reg = PCA9551_REG_PSC0;break;case 1:reg = PCA9551_REG_PSC1;break;default:return -1;
return -EINVAL;
On Wed, Mar 11, 2015 at 09:51:37AM +0100, Stefan Roese wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Hi Tom,
On 11.03.2015 15:36, Tom Rini wrote:
On Wed, Mar 11, 2015 at 09:51:37AM +0100, Stefan Roese wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead?
Yes, I'll try to rework this patch a bit to make this feature integration less ugly.
Thanks, Stefan
Hi,
On 11 March 2015 at 08:36, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 11, 2015 at 09:51:37AM +0100, Stefan Roese wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Also if these CONFIG options are in Kconfig (as they should be) then we can use
if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256))
instead of #ifdef which may improve the code.
Regards, Simon
Hi Simon,
On 13.03.2015 03:48, Simon Glass wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Also if these CONFIG options are in Kconfig (as they should be) then we can use
if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256))
instead of #ifdef which may improve the code.
Right. I also thought about this. But the resulting code has all the functionality extracted into 2 functions:
#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) static int passwd_abort(uint64_t etime) { const char *sha_env_str = getenv("bootstopkeysha256"); ... } #else static int passwd_abort(uint64_t etime) { int abort = 0; ... } #endif
And this function is now called unconditionally:
... abort = passwd_abort(etime);
So there is nothing here that could be simplified by using IS_ENABLED().
I could of course just add this new config option to Kconfig. But with all the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some point all those config options should be moved to Kconfig. Unfortunately I don't have the time for this right now. But I'll add it to my list to do this at a later time.
Thanks, Stefan
Hi Stefan,
On 13 March 2015 at 01:15, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 13.03.2015 03:48, Simon Glass wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Also if these CONFIG options are in Kconfig (as they should be) then we can use
if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256))
instead of #ifdef which may improve the code.
Right. I also thought about this. But the resulting code has all the functionality extracted into 2 functions:
#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) static int passwd_abort(uint64_t etime) { const char *sha_env_str = getenv("bootstopkeysha256"); ... } #else static int passwd_abort(uint64_t etime) { int abort = 0; ... } #endif
And this function is now called unconditionally:
... abort = passwd_abort(etime);So there is nothing here that could be simplified by using IS_ENABLED().
I could of course just add this new config option to Kconfig. But with all the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some point all those config options should be moved to Kconfig. Unfortunately I don't have the time for this right now. But I'll add it to my list to do this at a later time.
Well rather than adding more options, perhaps we should wait until we get this moved to Kconfig? It's not going to get any easier :-)
Regards, Simon
Hi Simon,
On 23.03.2015 21:28, Simon Glass wrote:
Hi Stefan,
On 13 March 2015 at 01:15, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 13.03.2015 03:48, Simon Glass wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Also if these CONFIG options are in Kconfig (as they should be) then we can use
if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256))
instead of #ifdef which may improve the code.
Right. I also thought about this. But the resulting code has all the functionality extracted into 2 functions:
#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) static int passwd_abort(uint64_t etime) { const char *sha_env_str = getenv("bootstopkeysha256"); ... } #else static int passwd_abort(uint64_t etime) { int abort = 0; ... } #endif
And this function is now called unconditionally:
... abort = passwd_abort(etime);So there is nothing here that could be simplified by using IS_ENABLED().
I could of course just add this new config option to Kconfig. But with all the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some point all those config options should be moved to Kconfig. Unfortunately I don't have the time for this right now. But I'll add it to my list to do this at a later time.
Well rather than adding more options, perhaps we should wait until we get this moved to Kconfig? It's not going to get any easier :-)
Right. And now I'm finally back at this task. To get this encrypted password support into mainline. With Kconfig support of course this time. ;)
Unfortunately I'm hitting a problem while moving some of the "old" macros to Kconfig. Especially some strings like CONFIG_AUTOBOOT_PROMPT. Here how this looks in some config headers:
#define CONFIG_AUTOBOOT_PROMPT "autoboot in %d seconds\n", bootdelay
This does not work, as Kconfig truncates the string after the 2nd '"'. Escaping this '"' using '' also doesn't seem to work. Do you or Masahiro have some experience with this kind of Kconfig macro transition?
Thanks, Stefan
Hi Stefan,
On 5 May 2015 at 09:06, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 23.03.2015 21:28, Simon Glass wrote:
Hi Stefan,
On 13 March 2015 at 01:15, Stefan Roese sr@denx.de wrote:
Hi Simon,
On 13.03.2015 03:48, Simon Glass wrote:
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment.
This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256
Signed-off-by: Stefan Roese sr@denx.de
This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Also if these CONFIG options are in Kconfig (as they should be) then we can use
if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256))
instead of #ifdef which may improve the code.
Right. I also thought about this. But the resulting code has all the functionality extracted into 2 functions:
#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) static int passwd_abort(uint64_t etime) { const char *sha_env_str = getenv("bootstopkeysha256"); ... } #else static int passwd_abort(uint64_t etime) { int abort = 0; ... } #endif
And this function is now called unconditionally:
... abort = passwd_abort(etime);So there is nothing here that could be simplified by using IS_ENABLED().
I could of course just add this new config option to Kconfig. But with all the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some point all those config options should be moved to Kconfig. Unfortunately I don't have the time for this right now. But I'll add it to my list to do this at a later time.
Well rather than adding more options, perhaps we should wait until we get this moved to Kconfig? It's not going to get any easier :-)
Right. And now I'm finally back at this task. To get this encrypted password support into mainline. With Kconfig support of course this time. ;)
Unfortunately I'm hitting a problem while moving some of the "old" macros to Kconfig. Especially some strings like CONFIG_AUTOBOOT_PROMPT. Here how this looks in some config headers:
#define CONFIG_AUTOBOOT_PROMPT "autoboot in %d seconds\n", bootdelay
This does not work, as Kconfig truncates the string after the 2nd '"'. Escaping this '"' using '' also doesn't seem to work. Do you or Masahiro have some experience with this kind of Kconfig macro transition?
Not me. I noticed this when refactoring the code. IMO it is broken - we should not be doing things like that.
From what I can see we only ever pass bootdelay as a parameter. So
perhaps you can drop the ", bootdelay" part and adjust the code in from common/autoboot.c from:
printf(CONFIG_AUTOBOOT_PROMPT);
to:
printf(CONFIG_AUTOBOOT_PROMPT, bootdelay);
Of course that will create printf() warnings for a few boards but it should be possible to turn them off at that call site.
Regards, Simon
participants (8)
-
Fabio Estevam -
Holger Brunck -
Nitin Garg -
Simon Glass -
Stefan Roese -
Tom Rini -
Valentin Longchamp -
York Sun