From patchwork Fri May 1 14:55:12 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244746 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 1 May 2020 16:55:12 +0200 Subject: [PATCH 1/5] sysreset: mpc83xx: use scnprintf instead of snprintf In-Reply-To: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> References: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> Message-ID: <20200501145516.18168-2-rasmus.villemoes@prevas.dk> Neither snprintf or scnprintf ever return a negative value, so the error checking is pointless. The correct idiom for printing piecemeal to a buffer of a given size is to use scnprintf(), since that will ensure buf will never point past the actual given buffer, and the remaining size will never become negative (since scnprintf(), when given a non-zero size, has the property that the return value is strictly less than the given size). Signed-off-by: Rasmus Villemoes --- drivers/sysreset/sysreset_mpc83xx.c | 54 ++++++++++------------------- 1 file changed, 19 insertions(+), 35 deletions(-) diff --git a/drivers/sysreset/sysreset_mpc83xx.c b/drivers/sysreset/sysreset_mpc83xx.c index 9092764e0b..7148464d8b 100644 --- a/drivers/sysreset/sysreset_mpc83xx.c +++ b/drivers/sysreset/sysreset_mpc83xx.c @@ -104,23 +104,23 @@ static int print_83xx_arb_event(bool force, char *buf, int size) return 0; if (CONFIG_IS_ENABLED(CONFIG_DISPLAY_AER_FULL)) { - res = snprintf(buf, size, - "Arbiter Event Status:\n" - " %s: 0x%08lX\n" - " %s: 0x%1x = %s\n" - " %s: 0x%02x = %s\n" - " %s: 0x%1x = %d bytes\n" - " %s: 0x%02x = %s\n", - "Event Address", gd->arch.arbiter_event_address, - "Event Type", etype, event[etype], - "Master ID", mstr_id, master[mstr_id], - "Transfer Size", tsize_val, tsize_bytes, - "Transfer Type", ttype, transfer[ttype]); + res = scnprintf(buf, size, + "Arbiter Event Status:\n" + " %s: 0x%08lX\n" + " %s: 0x%1x = %s\n" + " %s: 0x%02x = %s\n" + " %s: 0x%1x = %d bytes\n" + " %s: 0x%02x = %s\n", + "Event Address", gd->arch.arbiter_event_address, + "Event Type", etype, event[etype], + "Master ID", mstr_id, master[mstr_id], + "Transfer Size", tsize_val, tsize_bytes, + "Transfer Type", ttype, transfer[ttype]); } else if (CONFIG_IS_ENABLED(CONFIG_DISPLAY_AER_BRIEF)) { - res = snprintf(buf, size, - "Arbiter Event Status: AEATR=0x%08lX, AEADR=0x%08lX\n", - gd->arch.arbiter_event_attributes, - gd->arch.arbiter_event_address); + res = scnprintf(buf, size, + "Arbiter Event Status: AEATR=0x%08lX, AEADR=0x%08lX\n", + gd->arch.arbiter_event_attributes, + gd->arch.arbiter_event_address); } return res; @@ -150,13 +150,7 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) int i; char *sep; - res = snprintf(buf, size, "Reset Status:"); - if (res < 0) { - debug("%s: Could not write reset status message (err = %d)\n", - dev->name, res); - return -EIO; - } - + res = scnprintf(buf, size, "Reset Status:"); buf += res; size -= res; @@ -164,13 +158,8 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) for (i = 0; i < ARRAY_SIZE(bits); i++) /* Print description of set bits */ if (rsr & bits[i].mask) { - res = snprintf(buf, size, "%s%s%s", sep, bits[i].desc, + res = scnprintf(buf, size, "%s%s%s", sep, bits[i].desc, (i == ARRAY_SIZE(bits) - 1) ? "\n" : ""); - if (res < 0) { - debug("%s: Could not write reset status message (err = %d)\n", - dev->name, res); - return -EIO; - } buf += res; size -= res; sep = ", "; @@ -187,15 +176,10 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) * event to be printed */ res = print_83xx_arb_event(rsr & RSR_BMRS, buf, size); - if (res < 0) { - debug("%s: Could not write arbiter event message (err = %d)\n", - dev->name, res); - return -EIO; - } buf += res; size -= res; } - snprintf(buf, size, "\n"); + scnprintf(buf, size, "\n"); return 0; } From patchwork Fri May 1 14:55:13 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244747 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 1 May 2020 16:55:13 +0200 Subject: [PATCH 2/5] sysreset: mpc83xx: shuffle newline logic in mpc83xx_sysreset_get_status In-Reply-To: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> References: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> Message-ID: <20200501145516.18168-3-rasmus.villemoes@prevas.dk> mpc83xx_sysreset_get_status() adds a newline to the buffer at the end of the function. There's no point adding a newline in case the reset status word happens to contain the RSR_HRS bit. On the other hand, if one of the CONFIG_DISPLAY_AER_* options is enabled, and the RSR_HSR bit wasn't set, we'd miss a newline before the print_83xx_arb_event() output. That latter always includes a newline in its output, so just pull up the scnprintf("\n") a bit. Signed-off-by: Rasmus Villemoes --- drivers/sysreset/sysreset_mpc83xx.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/sysreset/sysreset_mpc83xx.c b/drivers/sysreset/sysreset_mpc83xx.c index 7148464d8b..3447b8e75c 100644 --- a/drivers/sysreset/sysreset_mpc83xx.c +++ b/drivers/sysreset/sysreset_mpc83xx.c @@ -158,13 +158,16 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) for (i = 0; i < ARRAY_SIZE(bits); i++) /* Print description of set bits */ if (rsr & bits[i].mask) { - res = scnprintf(buf, size, "%s%s%s", sep, bits[i].desc, - (i == ARRAY_SIZE(bits) - 1) ? "\n" : ""); + res = scnprintf(buf, size, "%s%s", sep, bits[i].desc); buf += res; size -= res; sep = ", "; } + res = scnprintf(buf, size, "\n"); + buf += res; + size -= res; + /* * TODO(mario.six at gdsys.cc): Move this into a dedicated * arbiter driver @@ -179,7 +182,6 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) buf += res; size -= res; } - scnprintf(buf, size, "\n"); return 0; } From patchwork Fri May 1 14:55:14 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244748 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 1 May 2020 16:55:14 +0200 Subject: [PATCH 3/5] sysreset: mpc83xx: fix CONFIG_IS_ENABLED logic In-Reply-To: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> References: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> Message-ID: <20200501145516.18168-4-rasmus.villemoes@prevas.dk> Unlike IS_ENABLED, one should not include the CONFIG_ part in the argument to CONFIG_IS_ENABLED - currently, the first of these tests for whether CONFIG_CONFIG_AER_FULL or CONFIG_SPL_CONFIG_AER_FULL are defined (depending on SPL or not), similarly for the other one. Signed-off-by: Rasmus Villemoes --- drivers/sysreset/sysreset_mpc83xx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/sysreset/sysreset_mpc83xx.c b/drivers/sysreset/sysreset_mpc83xx.c index 3447b8e75c..631ae6a5dc 100644 --- a/drivers/sysreset/sysreset_mpc83xx.c +++ b/drivers/sysreset/sysreset_mpc83xx.c @@ -103,7 +103,7 @@ static int print_83xx_arb_event(bool force, char *buf, int size) if (!force && !gd->arch.arbiter_event_address) return 0; - if (CONFIG_IS_ENABLED(CONFIG_DISPLAY_AER_FULL)) { + if (CONFIG_IS_ENABLED(DISPLAY_AER_FULL)) { res = scnprintf(buf, size, "Arbiter Event Status:\n" " %s: 0x%08lX\n" @@ -116,7 +116,7 @@ static int print_83xx_arb_event(bool force, char *buf, int size) "Master ID", mstr_id, master[mstr_id], "Transfer Size", tsize_val, tsize_bytes, "Transfer Type", ttype, transfer[ttype]); - } else if (CONFIG_IS_ENABLED(CONFIG_DISPLAY_AER_BRIEF)) { + } else if (CONFIG_IS_ENABLED(DISPLAY_AER_BRIEF)) { res = scnprintf(buf, size, "Arbiter Event Status: AEATR=0x%08lX, AEADR=0x%08lX\n", gd->arch.arbiter_event_attributes, @@ -172,8 +172,8 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) * TODO(mario.six at gdsys.cc): Move this into a dedicated * arbiter driver */ - if (CONFIG_IS_ENABLED(CONFIG_DISPLAY_AER_FULL) || - CONFIG_IS_ENABLED(CONFIG_DISPLAY_AER_BRIEF)) { + if (CONFIG_IS_ENABLED(DISPLAY_AER_FULL) || + CONFIG_IS_ENABLED(DISPLAY_AER_BRIEF)) { /* * If there was a bus monitor reset event, we force the arbiter * event to be printed From patchwork Fri May 1 14:55:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244749 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 1 May 2020 16:55:15 +0200 Subject: [PATCH 4/5] sysreset: mpc83xx: add output in case of cold boot In-Reply-To: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> References: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> Message-ID: <20200501145516.18168-5-rasmus.villemoes@prevas.dk> For a powercycle/cold boot, none of the RSR_* bits in the reset status register are set, so one gets an empty Reset Status: line. Print an indication that this was likely a cold boot. Signed-off-by: Rasmus Villemoes --- drivers/sysreset/sysreset_mpc83xx.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/drivers/sysreset/sysreset_mpc83xx.c b/drivers/sysreset/sysreset_mpc83xx.c index 631ae6a5dc..6457d73418 100644 --- a/drivers/sysreset/sysreset_mpc83xx.c +++ b/drivers/sysreset/sysreset_mpc83xx.c @@ -149,20 +149,28 @@ static int mpc83xx_sysreset_get_status(struct udevice *dev, char *buf, int size) ulong rsr = gd->arch.reset_status; int i; char *sep; + ulong known_bits = RSR_SWSR | RSR_SWHR | RSR_JSRS | RSR_CSHR | + RSR_SWRS | RSR_BMRS | RSR_SRS | RSR_HRS; res = scnprintf(buf, size, "Reset Status:"); buf += res; size -= res; - sep = " "; - for (i = 0; i < ARRAY_SIZE(bits); i++) - /* Print description of set bits */ - if (rsr & bits[i].mask) { - res = scnprintf(buf, size, "%s%s", sep, bits[i].desc); - buf += res; - size -= res; - sep = ", "; - } + if (rsr & known_bits) { + sep = " "; + for (i = 0; i < ARRAY_SIZE(bits); i++) + /* Print description of set bits */ + if (rsr & bits[i].mask) { + res = scnprintf(buf, size, "%s%s", sep, bits[i].desc); + buf += res; + size -= res; + sep = ", "; + } + } else { + res = scnprintf(buf, size, " Unknown/Cold boot"); + buf += res; + size -= res; + } res = scnprintf(buf, size, "\n"); buf += res; From patchwork Fri May 1 14:55:16 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244750 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 1 May 2020 16:55:16 +0200 Subject: [PATCH 5/5] sysreset: move print_resetinfo() to sysreset-uclass.c In-Reply-To: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> References: <20200501145516.18168-1-rasmus.villemoes@prevas.dk> Message-ID: <20200501145516.18168-6-rasmus.villemoes@prevas.dk> Moving this out of board_f.c allows board-specific SPL code to call this rather than duplicating its implementation. Signed-off-by: Rasmus Villemoes Reviewed-by: Simon Glass --- common/board_f.c | 24 ------------------------ drivers/sysreset/sysreset-uclass.c | 22 ++++++++++++++++++++++ include/sysreset.h | 5 +++++ 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/common/board_f.c b/common/board_f.c index 82a164752a..252ad1c520 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -152,30 +152,6 @@ static int display_text_info(void) return 0; } -#ifdef CONFIG_SYSRESET -static int print_resetinfo(void) -{ - struct udevice *dev; - char status[256]; - int ret; - - ret = uclass_first_device_err(UCLASS_SYSRESET, &dev); - if (ret) { - debug("%s: No sysreset device found (error: %d)\n", - __func__, ret); - /* Not all boards have sysreset drivers available during early - * boot, so don't fail if one can't be found. - */ - return 0; - } - - if (!sysreset_get_status(dev, status, sizeof(status))) - printf("%s", status); - - return 0; -} -#endif - #if defined(CONFIG_DISPLAY_CPUINFO) && CONFIG_IS_ENABLED(CPU) static int print_cpuinfo(void) { diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c index 51fdb1055e..739e4526ff 100644 --- a/drivers/sysreset/sysreset-uclass.c +++ b/drivers/sysreset/sysreset-uclass.c @@ -18,6 +18,28 @@ #include #include +int print_resetinfo(void) +{ + struct udevice *dev; + char status[256]; + int ret; + + ret = uclass_first_device_err(UCLASS_SYSRESET, &dev); + if (ret) { + debug("%s: No sysreset device found (error: %d)\n", + __func__, ret); + /* Not all boards have sysreset drivers available during early + * boot, so don't fail if one can't be found. + */ + return 0; + } + + if (!sysreset_get_status(dev, status, sizeof(status))) + printf("%s", status); + + return 0; +} + int sysreset_request(struct udevice *dev, enum sysreset_t type) { struct sysreset_ops *ops = sysreset_get_ops(dev); diff --git a/include/sysreset.h b/include/sysreset.h index 61295e3fcb..5f402b9d04 100644 --- a/include/sysreset.h +++ b/include/sysreset.h @@ -116,4 +116,9 @@ void sysreset_walk_halt(enum sysreset_t type); */ void reset_cpu(ulong addr); +/** + * print_resetinfo() - print reset information to console + */ +int print_resetinfo(void); + #endif