[RFC PATCH 4/6] drivers: misc: k3_bist: Add K3 BIST driver
Kumar, Udit
u-kumar1 at ti.com
Wed Sep 4 07:05:13 CEST 2024
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-francis at 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
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6009d55f400..8e28a93d74c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -664,6 +664,14 @@ config ESM_K3
> help
> Support ESM (Error Signaling Module) on TI K3 SoCs.
>
> +config K3_BIST
> + bool "Enable K3 BIST driver"
> + depends on ARCH_K3
> + help
> + Support BIST (Built-In Self Test) module on TI K3 SoCs. This driver
> + supports running both PBIST (Memory BIST) and LBIST (Logic BIST) on
> + a region or IP in the SoC.
> +
> config MICROCHIP_FLEXCOM
> bool "Enable Microchip Flexcom driver"
> depends on MISC
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e53d52c47b3..15c5c4810dd 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -89,6 +89,7 @@ obj-$(CONFIG_JZ4780_EFUSE) += jz4780_efuse.o
> obj-$(CONFIG_MICROCHIP_FLEXCOM) += microchip_flexcom.o
> obj-$(CONFIG_K3_AVS0) += k3_avs.o
> obj-$(CONFIG_ESM_K3) += k3_esm.o
> +obj-$(CONFIG_K3_BIST) += k3_bist.o
> obj-$(CONFIG_ESM_PMIC) += esm_pmic.o
> obj-$(CONFIG_SL28CPLD) += sl28cpld.o
> obj-$(CONFIG_SPL_SOCFPGA_DT_REG) += socfpga_dtreg.o
> diff --git a/drivers/misc/k3_bist.c b/drivers/misc/k3_bist.c
> new file mode 100644
> index 00000000000..a4728376b73
> --- /dev/null
> +++ b/drivers/misc/k3_bist.c
> @@ -0,0 +1,507 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Texas Instruments' BIST (Built-In Self-Test) driver
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + * Neha Malcom Francis <n-francis at ti.com>
> + *
> + */
> +
> +#include <dm.h>
> +#include <errno.h>
> +#include <clk.h>
> +#include <asm/io.h>
> +#include <dm/device_compat.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <asm/arch/hardware.h>
> +#include <linux/soc/ti/ti_sci_protocol.h>
> +#include <remoteproc.h>
> +#include <power-domain.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.
> +#include "k3_bist_static_data.h"
> +
> +/* PBIST Timeout Value */
> +#define PBIST_MAX_TIMEOUT_VALUE 100000000
> +
> +/**
> + * struct k3_bist_privdata - K3 BIST structure
> + * @dev: device pointer
> + * @base: base of register set
> + * @instance: PBIST instance number
> + * @intr_num: corresponding interrupt ID of the PBIST instance
> + */
> +struct k3_bist_privdata {
> + struct udevice *dev;
> + void *base;
> + u32 instance;
> + u32 intr_num;
> +};
> +
> +static struct k3_bist_privdata *k3_bist_priv;
> +
> +/**
> + * pbist_run_post_pbist_check() - Check POST results
> + *
> + * Function to check whether HW Power-On Self Test, i.e. POST has run
> + * successfully on the MCU domain.
> + *
> + * Return: 0 if all went fine, else corresponding error.
> + */
> +int pbist_run_post_pbist_check(void)
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
> +{
> + 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
> + } 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
> + }
> + } else {
> + /* HW POST: PBIST was completed on this device, check the result */
> + mask = WKUP_CTRL_MMR_CFG0_WKUP_POST_STAT_POST_MCU_PBIST_FAIL_MASK;
> +
> + if ((post_reg_val & mask) != 0) {
> + debug("%s: PBIST was completed, but the test failed\n", __func__);
> + return -EINVAL;
> + } else {
> + debug("%s: HW POST PBIST completed, test passed\n", __func__);
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * core_get_by_index() - Configure processor to correct state
Two operation here. please rename if possible
> + *
> + * Function to configure processor under test to correct state for SW-initiated
> + * PBIST
> + * @dev: BIST device
> + * @index: corresponding index of the core in the cores-under-test list
> + * @turnoff: true if core is needed to be turned off
> + *
> + * Return: 0 if all went fine, else corresponding error.
> + */
> +int core_get_by_index(struct udevice *dev, int index, bool turnoff)
> +{
> + struct ofnode_phandle_args args;
> + int ret;
> + struct udevice *dev_core;
> +
> + ret = dev_read_phandle_with_args(dev, "cores-under-test", NULL, 0, index, &args);
> + if (ret) {
> + debug("%s: dev_read_phandle_with_args failed: %d\n", __func__,
> + ret);
> + return ret;
> + }
> + ret = uclass_get_device_by_ofnode(UCLASS_REMOTEPROC, args.node, &dev_core);
> + if (ret) {
> + debug("%s: uclass_get_device_by_of_offset failed: %d\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + if (turnoff) {
> + struct power_domain pwrdmn;
> + struct clk fclk;
> +
isn't some tisci call are ok to turn off the CPUs.
> + ret = power_domain_get_by_index(dev_core, &pwrdmn, 0);
> + if (ret) {
> + dev_err(dev, "failed to get power domain for the core %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_get_by_index(dev_core, 0, &fclk);
> + if (ret) {
> + dev_err(dev, "failed to get clock for the core %d\n", ret);
> + return ret;
> + }
> +
> + ret = power_domain_off(&pwrdmn);
> + if (ret) {
> + dev_err(dev, "failed to power OFF the core %d\n", ret);
> + return ret;
> + }
> +
> + ret = power_domain_free(&pwrdmn);
> + if (ret) {
> + dev_err(dev, "failed to free the core %d\n", ret);
> + return ret;
> + }
> + ret = clk_disable(&fclk);
> + if (ret) {
> + dev_err(dev, "failed to disable clock of the core %d\n", ret);
> + return ret;
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + * pbist_self_test() - Run PBIST_TEST on specified cores
> + * @config: pbist_config structure for PBIST test
> + *
> + * Function to run PBIST_TEST
> + *
> + * Return: 0 if all went fine, else corresponding error.
> + */
> +int pbist_self_test(struct pbist_config *config)
> +{
> + struct udevice *dev = k3_bist_priv->dev;
> + void *base = k3_bist_priv->base;
> + u32 intr_num = k3_bist_priv->intr_num;
> + bool test_result = true;
> +
> + /* Turns on PBIST clock in PBIST ACTivate register */
> + writel(PBIST_PACT_PACT_MASK, base + PBIST_PACT);
> +
> + /* Set Margin mode register for Test mode */
> + writel(PBIST_TEST_MODE, base + PBIST_MARGIN_MODE);
> +
> + /* Zero out Loop counter 0 */
> + writel(0x0, base + PBIST_L0);
> +
> + /* Set algorithm bitmap */
> + writel(config->algorithms_bit_map, base + PBIST_ALGO);
> +
> + /* Set Memory group bitmap */
> + writel(config->memory_groups_bit_map, base + PBIST_RINFO);
> +
> + /* Zero out override register */
> + writel(config->override, base + PBIST_OVER);
> +
> + /* Set Scramble value - 64 bit*/
> + writel(config->scramble_value_lo, base + PBIST_SCR_LO);
> + writel(config->scramble_value_hi, base + PBIST_SCR_HI);
> +
> + /* Set DLR register for ROM based testing and Config Access */
> + writel(PBIST_DLR_DLR0_ROM_MASK
> + | PBIST_DLR_DLR0_CAM_MASK, base + PBIST_DLR);
> +
> + 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
> +
> + 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
> +}
> +
> +/**
> + * pbist_neg_self_test() - Run PBIST_negTEST on specified cores
> + * @config: pbist_config_neg structure for PBIST negative test
> + *
> + * Function to run PBIST failure insertion test
> + *
> + * Return: 0 if all went fine, else corresponding error.
> + */
> +int pbist_neg_self_test(struct pbist_config_neg *config)
> +{
> + struct udevice *dev = k3_bist_priv->dev;
> + void *base = k3_bist_priv->base;
> + u32 intr_num = k3_bist_priv->intr_num;
> + bool test_result = true;
> +
> + /* Turns on PBIST clock in PBIST ACTivate register */
> + writel(PBIST_PACT_PACT_MASK, base + PBIST_PACT);
> +
> + /* Set Margin mode register for Test mode */
> + writel(PBIST_FAILURE_INSERTION_TEST_MODE, base + PBIST_MARGIN_MODE);
> +
> + /* Zero out Loop counter 0 */
> + writel(0x0, base + PBIST_L0);
> +
> + /* Set DLR register */
> + writel(0x10, base + PBIST_DLR);
> +
> + /* Set Registers*/
> + writel(0x00000001, base + PBIST_RF0L);
> + writel(0x00003123, base + PBIST_RF0U);
> + writel(0x0513FC02, base + PBIST_RF1L);
> + writel(0x00000002, base + PBIST_RF1U);
> + writel(0x00000003, base + PBIST_RF2L);
> + writel(0x00000000, base + PBIST_RF2U);
> + writel(0x00000004, base + PBIST_RF3L);
> + writel(0x00000028, base + PBIST_RF3U);
> + writel(0x64000044, base + PBIST_RF4L);
> + writel(0x00000000, base + PBIST_RF4U);
> + writel(0x0006A006, base + PBIST_RF5L);
> + writel(0x00000000, base + PBIST_RF5U);
> + writel(0x00000007, base + PBIST_RF6L);
> + writel(0x0000A0A0, base + PBIST_RF6U);
> + writel(0x00000008, base + PBIST_RF7L);
> + writel(0x00000064, base + PBIST_RF7U);
> + writel(0x00000009, base + PBIST_RF8L);
> + writel(0x0000A5A5, base + PBIST_RF8U);
> + writel(0x0000000A, base + PBIST_RF9L);
> + writel(0x00000079, base + PBIST_RF9U);
> + writel(0x00000000, base + PBIST_RF10L);
> + writel(0x00000001, base + PBIST_RF10U);
> + writel(0xAAAAAAAA, base + PBIST_D);
> + writel(0xAAAAAAAA, base + PBIST_E);
too much direct values
> +
> + writel(config->CA2, base + PBIST_CA2);
> + writel(config->CL0, base + PBIST_CL0);
> + writel(config->CA3, base + PBIST_CA3);
> + writel(config->I0, base + PBIST_I0);
> + writel(config->CL1, base + PBIST_CL1);
> + writel(config->I3, base + PBIST_I3);
> + writel(config->I2, base + PBIST_I2);
> + writel(config->CL2, base + PBIST_CL2);
> + writel(config->CA1, base + PBIST_CA1);
> + writel(config->CA0, base + PBIST_CA0);
> + writel(config->CL3, base + PBIST_CL3);
> + writel(config->I1, base + PBIST_I1);
> + writel(config->RAMT, base + PBIST_RAMT);
> + writel(config->CSR, base + PBIST_CSR);
> + writel(config->CMS, base + PBIST_CMS);
> +
> + writel(0x00000009, base + PBIST_STR);
> +
> + /* Start PBIST */
> + writel(0x00000001, base + PBIST_STR);
> +
> + udelay(1000);
> +
> + u32 timeout_count = 0;
> +
> + while ((!(readl(VIM_RAW(intr_num)) & VIM_RAW_MASK(intr_num))) &&
> + (timeout_count++ < PBIST_MAX_TIMEOUT_VALUE))
> + ;
> +
> + if (!(readl(VIM_RAW(intr_num)) & VIM_RAW_MASK(intr_num))) {
> + test_result = false;
> + debug("%s(dev=%p): test failed\n", __func__, dev);
> + } else {
> + debug("%s(dev=%p): test passed\n", __func__, dev);
> + }
> +
> + writel(0xffffffff, VIM_STS(intr_num));
> +
> + return 0;
Same as in above function for error
> +}
> +
> +/**
> + * pbist_rom_self_test() - Run PBIST_ROM_TEST on specified cores
> + * @config: pbist_config_rom structure for PBIST negative test
> + *
> + * Function to run PBIST test of ROM
> + *
> + * Return: 0 if all went fine, else corresponding error.
> + */
> +int pbist_rom_self_test(struct pbist_config_rom *config)
> +{
> + struct udevice *dev = k3_bist_priv->dev;
> + void *base = k3_bist_priv->base;
> + u32 intr_num = k3_bist_priv->intr_num;
> + bool test_result = true;
> +
> + /* Turns on PBIST clock in PBIST ACTivate register */
> + writel(0x1, base + PBIST_PACT);
> +
> + /* Set Margin mode register for Test mode */
> + writel(0xf, base + PBIST_MARGIN_MODE);
> +
> + /* Zero out Loop counter 0 */
> + writel(0x0, base + PBIST_L0);
> +
> + /* Set DLR register */
> + writel(0x310, base + PBIST_DLR);
> +
> + /* Set Registers*/
> + writel(0x00000001, base + PBIST_RF0L);
> + writel(0x00003123, base + PBIST_RF0U);
> + writel(0x7A400183, base + PBIST_RF1L);
> + writel(0x00000060, base + PBIST_RF1U);
> + writel(0x00000184, base + PBIST_RF2L);
> + writel(0x00000000, base + PBIST_RF2U);
> + writel(0x7B600181, base + PBIST_RF3L);
> + writel(0x00000061, base + PBIST_RF3U);
> + writel(0x00000000, base + PBIST_RF4L);
> + writel(0x00000000, base + PBIST_RF4U);
> +
> + writel(config->D, base + PBIST_D);
> + writel(config->E, base + PBIST_E);
> + writel(config->CA2, base + PBIST_CA2);
> + writel(config->CL0, base + PBIST_CL0);
> + writel(config->CA3, base + PBIST_CA3);
> + writel(config->I0, base + PBIST_I0);
> + writel(config->CL1, base + PBIST_CL1);
> + writel(config->I3, base + PBIST_I3);
> + writel(config->I2, base + PBIST_I2);
> + writel(config->CL2, base + PBIST_CL2);
> + writel(config->CA1, base + PBIST_CA1);
> + writel(config->CA0, base + PBIST_CA0);
> + writel(config->CL3, base + PBIST_CL3);
> + writel(config->I1, base + PBIST_I1);
> + writel(config->RAMT, base + PBIST_RAMT);
> + writel(config->CSR, base + PBIST_CSR);
> + writel(config->CMS, base + PBIST_CMS);
> +
> + writel(0x00000009, base + PBIST_STR);
> +
> + /* Start PBIST */
> + writel(0x00000001, base + PBIST_STR);
> +
> + udelay(1000);
> +
Why delay is needed. please add comment for that
> + u32 timeout_count = 0;
same as above
> +
> + while ((!(readl(VIM_RAW(intr_num)) & VIM_RAW_MASK(intr_num))) &&
> + (timeout_count++ < PBIST_MAX_TIMEOUT_VALUE))
> + ;
> +
> + if (!(readl(VIM_RAW(intr_num)) & VIM_RAW_MASK(intr_num))) {
> + test_result = false;
> + debug("%s(dev=%p): test failed\n", __func__, dev);
> + } else {
> + debug("%s(dev=%p): test passed\n", __func__, dev);
> + }
> +
> + writel(0xffffffff, VIM_STS(intr_num));
> +
> + return 0;
> +}
> +
> +/**
> + * k3_bist_probe() - Basic probe
> + * @dev: corresponding BIST device
> + *
> + * Parses BIST info from device tree, and configures the module accordingly.
> + * Return: 0 if all goes good, else appropriate error message.
> + */
> +static int k3_bist_probe(struct udevice *dev)
> +{
> + int ret = 0, num_runs, i, j;
> + struct k3_bist_privdata *priv = dev_get_priv(dev);
> + struct pbist_inst_info *info;
> +
> + debug("%s(dev=%p)\n", __func__, dev);
> +
> + priv = dev_get_priv(dev);
NULL error check for priv
> + priv->dev = dev;
> +
> + k3_bist_priv = priv;
> +
> + priv->base = dev_remap_addr_index(dev, 0);
> + if (!priv->base)
> + return -ENODEV;
> +
> + ret = dev_read_u32(dev, "ti,bist-instance", &priv->instance);
> + if (!priv->instance)
> + return -ENODEV;
> +
> + switch (priv->instance) {
> + case PBIST14_INSTANCE:
> + info = &pbist14_inst_info;
> + priv->intr_num = info->intr_num;
> + break;
> + default:
> + dev_err(dev, "%s: PBIST instance %d not supported\n", __func__, priv->instance);
> + return -ENODEV;
> + };
> +
> + /* Probe the cores under test */
> + for (i = 0; ; i++) {
> + ret = core_get_by_index(dev, i, false);
> + if (ret)
> + break;
> + }
> +
> + if (!i) {
> + dev_err(dev, "%s: Acquiring the core failed. ret = %d\n", __func__, ret);
> + return ret;
> + }
Please add a check, what you expect in device tree at start of probe .
I assume, you can hit this only case of incorrect DT
> +
> + /* Check whether HW POST successfully completely PBIST on the MCU domain */
> + ret = pbist_run_post_pbist_check();
> + if (ret) {
> + dev_err(dev, "HW POST failed to run successfully %d\n", ret);
> + return ret;
> + }
> +
you might need to do this check first. before probing other cores
> + /* Run PBIST test */
> + num_runs = info->num_pbist_runs;
if you want num_runs configurable, prefer to use DT
> +
> + for (j = 0; j < num_runs; j++) {
> + ret = pbist_self_test(&info->pbist_config_run[j]);
> + if (ret) {
> + dev_err(dev, "failed to run PBIST test %d\n", ret);
> + return ret;
> + }
> + }
Dummy question, will above run the BIST on all selected cores ?
> +
> + /* Run PBIST failure insertion test */
> + ret = pbist_neg_self_test(&info->pbist_neg_config_run);
> + if (ret) {
> + dev_err(dev, "failed to run PBIST negative test %d\n", ret);
> + return ret;
> + }
> +
> + /* Run PBIST test on ROM */
> + num_runs = info->num_pbist_rom_test_runs;
> +
> + for (j = 0; j < num_runs; j++) {
> + ret = pbist_rom_self_test(&info->pbist_rom_test_config_run[j]);
> + if (ret) {
> + dev_err(dev, "failed to run ROM PBIST test %d\n", ret);
> + return ret;
> + }
> + }
> +
> + /* Power off cores under test */
> + while (i--) {
> + ret = core_get_by_index(dev, i, true);
> + if (ret)
> + break;
> + }
can we get rid from this 'i' , May you can do all core ON and OFF in one
function,
Largely its from device tree
> +
> + if (i) {
> + dev_err(dev, "%s: Stopping the core failed. ret = %d\n", __func__, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct udevice_id k3_bist_ids[] = {
> + { .compatible = "ti,j784s4-bist" },
> + {}
> +};
> +
> +U_BOOT_DRIVER(k3_bist) = {
> + .name = "k3_bist",
> + .of_match = k3_bist_ids,
> + .id = UCLASS_MISC,
> + .probe = k3_bist_probe,
> + .priv_auto = sizeof(struct k3_bist_privdata),
> +};
> diff --git a/drivers/misc/k3_bist_static_data.h b/drivers/misc/k3_bist_static_data.h
> new file mode 100644
> index 00000000000..f30fb7935b6
> --- /dev/null
> +++ b/drivers/misc/k3_bist_static_data.h
> @@ -0,0 +1,551 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Static Data for Texas Instruments' BIST (Built-In Self-Test) driver
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + *
> + */
> +
> +#ifndef __K3_BIST_STATIC_DATA_H
> +#define __K3_BIST_STATIC_DATA_H
> +
> +#define PBIST_MAX_NUM_RUNS 2
> +#define NUM_MAX_PBIST_TEST_ROM_RUNS 13
> +#define PBIST14_DFT_PBIST_CPU_0_INTR_NUM 311
> +
> +/* VIM Registers */
> +#define VIM_STS_BASE 0x40f80404
> +#define VIM_RAW_BASE 0x40f80400
> +
> +#define VIM_STS(i) (VIM_STS_BASE + (i) / 32 * 0x20)
> +#define VIM_RAW(i) (VIM_RAW_BASE + (i) / 32 * 0x20)
> +#define VIM_RAW_MASK(i) (BIT((i) % 32))
Above this SOC specific data , Please have some SOC specific data in
another header file and include here.
So that adding next SOC will be easy
> [..]
> +#if IS_ENABLED(CONFIG_SOC_K3_J784S4)
Please put this data in SOC specific header file
> [..]
More information about the U-Boot
mailing list