Re: [RFC PATCH 4/6] drivers: misc: k3_bist: Add K3 BIST driver

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@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@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

[..]