On 9/5/2024 3:11 PM, Neha Malcom Francis wrote:
On 04/09/24 10:35, Kumar, Udit wrote:
On 9/3/2024 5:14 PM, Neha Malcom Francis wrote:
Add a driver for the BIST module which currently includes support for
BIST IPs that trigger PBIST (Memory BIST).
Signed-off-by: Neha Malcom Francis <n-fran...@ti.com>
---
drivers/misc/Kconfig | 8 +
drivers/misc/Makefile | 1 +
drivers/misc/k3_bist.c | 507 ++++++++++++++++++++++++++
drivers/misc/k3_bist_static_data.h | 551
+++++++++++++++++++++++++++++
4 files changed, 1067 insertions(+)
create mode 100644 drivers/misc/k3_bist.c
create mode 100644 drivers/misc/k3_bist_static_data.h
[...]
+
In general, few macro's name are too long
many places hard-coded values are used, please consider to move to macro
driver looks to be j784s4 specific including header files ,
please see, if we can make this generic driver.
I've put SoC specific (J784S4 right now) data protected with SoC
specific configs in k3_bist_static_data.h; the hardcoded values are a
sequence for triggering a specific test, whatever is generic and known
I have put as a macro, however I'll try to better understand the
sequence if I can put them as macros.
+#include "k3_bist_static_data.h"
+
[...]
Please consider to rename function name as per description above
something like
check_pbist_results_of_mcu_domain,, if you agree
Also give more context, I believe, ROM runs BIST on MCU domain,
Please consider to mention, if you want
Yes will do!
+{
+ bool is_done, timed_out;
+ u32 mask;
+ u32 post_reg_val, shift;
+
+ /* Read HW POST status register */
+ post_reg_val = readl(WKUP_CTRL_MMR0_BASE +
WKUP_CTRL_MMR_CFG0_WKUP_POST_STAT);
+
+ /* Check if HW POST PBIST was performed */
+ shift =
WKUP_CTRL_MMR_CFG0_WKUP_POST_STAT_POST_MCU_PBIST_DONE_SHIFT;
+ is_done = (((post_reg_val >> shift) & 0x1u) == 0x1u) ?
(bool)true : (bool)false;
+
+ if (!is_done) {
+ /* HW POST: PBIST not completed, check if it timed out */
+ shift =
WKUP_CTRL_MMR_CFG0_WKUP_POST_STAT_POST_MCU_PBIST_TIMEOUT_SHIFT;
Too long macro name
+ timed_out = (((post_reg_val >> shift) & 0x1u) == 0x1u) ?
(bool)true : (bool)false;
+
+ if (!timed_out) {
+ debug("%s: PBIST was not performed at all on this
device for this core\n",
+ __func__);
+ return -EINVAL;
This is error no ? , move to dev_err instead of debug
The return in k3_bist_probe throws a dev_err saying HW POST failed to
run successfully. So these were added as debugs in case the end user
wants to know exact cause of failure, I can move it as a dev_err as well.
ok, thanks
+ } else {
+ debug("%s: PBIST was attempted but timed out for this
section\n", __func__);
+ return -ETIMEDOUT;
This is error no ? , move to dev_err instead of debug .
What next, reboot SOC or just continue booting in case of error
This is also something I wanted this RFC to address, I prefer
rebooting SoC if HW POST fails. HW POST is performed by ROM based on
certain switch settings, which implies that an end-user wants this
check done if selected. And if it fails on the MCU domain itself, I do
not think we should continue.
Ok, if you are saying BIST in MCU domain in done based upon some switch
settings, then please put that switch logic here .
Reading code above looks, BIST run always
if (!is_done) {
+ /* HW POST: PBIST not completed, check if it timed out */
+ } else {
+ /* HW POST: PBIST was completed on this device, check the
result */
So now have three conditions,
1) BIST not attempted
2) BIST ran and passed
3) BIST ran and failed
For 1) condition, please see if you can read some register or switch
settings or so.
For 3) , as default we should hang.. In case users wants to continue on
failure they can modify u-boot source to do so :)
[..]
isn't some tisci call are ok to turn off the CPUs.
DM (Device Manager) firmware, responsible for power management is not
up at this point in the boot flow (R5 SPL). Thus TISCI calls that turn
on/turn off clocks and power domains are not available and we rely on
the primitive clk-k3.c and ti-power-domain.c drivers to do this for
us. As seen below, using the uclass functions which internally calls
these primitive drivers would be the way to go.
But DM is kind implemented by dev-data and dev-clk logic ?
For few calls DM just talk to TIFS, which are available at this point.
I request to check once, if possible
I could have used the remoteproc framework to do this but currently
the rproc driver uses TISCI firmware calls from DM and as mentioned
above that's not possible.
We should probably target modifying our remoteproc driver to use these
generic uclass APIs instead of direct TISCI calls.
Ok,
[...]
+ udelay(1000);
+
+ u32 timeout_count = 0;
Please move timeout_count at start of function
+
+ while ((!(readl(VIM_RAW(intr_num)) & VIM_RAW_MASK(intr_num))) &&
+ (timeout_count++ < PBIST_MAX_TIMEOUT_VALUE))
+ ;
Do you want to add some delay instead of just reading in a loop
That is possible... is there any benefit of delay over polling?
Eithercase the possible time would be at max be PBIST_MAX_TIMEOUT_VALUE.
You could be definitive in waiting for this register,
Say if hardware specs says, VIM register will be set in max time of 100
ms. then
keep PBIST_MAX_TIMEOUT_VALUE as 100 and read wait for 1 ms and then read.
+
+ if (!(readl(VIM_RAW(intr_num)) & VIM_RAW_MASK(intr_num))) {
+ test_result = false;
+ debug("%s(dev=%p): test failed\n", __func__, dev);
Fail is error , no ?
+ } else {
+ debug("%s(dev=%p): test passed\n", __func__, dev);
+ }
+
+ writel(0xffffffff, VIM_STS(intr_num));
+
+ return 0;
Caller always will see success
Right, I wasn't sure about what action to take based on the result.
Failure is failure, as default please print and call hang
+}
+
+/**
[..]
