[PATCH 0/3] ASoC: SOF: Intel: hda: Correct Firmware State Register use

[PATCH 0/3] ASoC: SOF: Intel: hda: Correct Firmware State Register use

[Peter Ujfalusi]

Hi,

The FSR (Firmware State Register) holds the ROM state information, it does not
contain error information.
The FSR itself is a bit more complicated as well as the state depends on the
module currently in use.

The error code from ROM or the status code from the firmware is located at the
next register.

Fix the handling of the FSR in order to provide usable and human readable (in
most cases) report on the status and error.

Regards,
Peter
---
Peter Ujfalusi (3):
  ASoC: SOF: Intel: hda: Correct the ROM/FW state reporting code
  ASoC: SOF: Intel: hda-loader: Use the FSR state definitions during
    bootup
  ASoC: SOF: Intel: hda: Drop no longer used ROM state definitions

 sound/soc/sof/intel/hda-loader.c |  10 +--
 sound/soc/sof/intel/hda.c        | 147 ++++++++++++++++++++++++++-----
 sound/soc/sof/intel/hda.h        |  69 +++++++++++++--
 3 files changed, 194 insertions(+), 32 deletions(-)

-- 
2.37.0

[PATCH 1/3] ASoC: SOF: Intel: hda: Correct the ROM/FW state reporting code

[Peter Ujfalusi]

The FSR (Firmware State Register) can be found at offset 0 in the SRAM and
it is holding information about the state of the ROM/FW.
In case of a boot failure it can be used to get the state where the boot
process got stuck, it does not itself contains error codes as such.

The error code (or the firmware state information) is stored in the next
soft register at offset 0x4.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda.c | 147 ++++++++++++++++++++++++++++++++------
 sound/soc/sof/intel/hda.h |  63 ++++++++++++++++
 2 files changed, 190 insertions(+), 20 deletions(-)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index bc07df1fc39f..d519b9802b3b 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -353,7 +353,7 @@ static inline bool hda_sdw_check_wakeen_irq(struct snd_sof_dev *sdev)
 
 struct hda_dsp_msg_code {
 	u32 code;
-	const char *msg;
+	const char *text;
 };
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG)
@@ -382,10 +382,7 @@ module_param_named(use_common_hdmi, hda_codec_use_common_hdmi, bool, 0444);
 MODULE_PARM_DESC(use_common_hdmi, "SOF HDA use common HDMI codec driver");
 #endif
 
-static const struct hda_dsp_msg_code hda_dsp_rom_msg[] = {
-	{HDA_DSP_ROM_FW_MANIFEST_LOADED, "status: manifest loaded"},
-	{HDA_DSP_ROM_FW_FW_LOADED, "status: fw loaded"},
-	{HDA_DSP_ROM_FW_ENTERED, "status: fw entered"},
+static const struct hda_dsp_msg_code hda_dsp_rom_fw_error_texts[] = {
 	{HDA_DSP_ROM_CSE_ERROR, "error: cse error"},
 	{HDA_DSP_ROM_CSE_WRONG_RESPONSE, "error: cse wrong response"},
 	{HDA_DSP_ROM_IMR_TO_SMALL, "error: IMR too small"},
@@ -404,26 +401,136 @@ static const struct hda_dsp_msg_code hda_dsp_rom_msg[] = {
 	{HDA_DSP_ROM_NULL_FW_ENTRY,	"error: null FW entry point"},
 };
 
-static void hda_dsp_get_status(struct snd_sof_dev *sdev, const char *level)
+#define FSR_ROM_STATE_ENTRY(state)	{FSR_STATE_ROM_##state, #state}
+static const struct hda_dsp_msg_code fsr_rom_state_names[] = {
+	FSR_ROM_STATE_ENTRY(INIT),
+	FSR_ROM_STATE_ENTRY(INIT_DONE),
+	FSR_ROM_STATE_ENTRY(CSE_MANIFEST_LOADED),
+	FSR_ROM_STATE_ENTRY(FW_MANIFEST_LOADED),
+	FSR_ROM_STATE_ENTRY(FW_FW_LOADED),
+	FSR_ROM_STATE_ENTRY(FW_ENTERED),
+	FSR_ROM_STATE_ENTRY(VERIFY_FEATURE_MASK),
+	FSR_ROM_STATE_ENTRY(GET_LOAD_OFFSET),
+	FSR_ROM_STATE_ENTRY(FETCH_ROM_EXT),
+	FSR_ROM_STATE_ENTRY(FETCH_ROM_EXT_DONE),
+	/* CSE states */
+	FSR_ROM_STATE_ENTRY(CSE_IMR_REQUEST),
+	FSR_ROM_STATE_ENTRY(CSE_IMR_GRANTED),
+	FSR_ROM_STATE_ENTRY(CSE_VALIDATE_IMAGE_REQUEST),
+	FSR_ROM_STATE_ENTRY(CSE_IMAGE_VALIDATED),
+	FSR_ROM_STATE_ENTRY(CSE_IPC_IFACE_INIT),
+	FSR_ROM_STATE_ENTRY(CSE_IPC_RESET_PHASE_1),
+	FSR_ROM_STATE_ENTRY(CSE_IPC_OPERATIONAL_ENTRY),
+	FSR_ROM_STATE_ENTRY(CSE_IPC_OPERATIONAL),
+	FSR_ROM_STATE_ENTRY(CSE_IPC_DOWN),
+};
+
+#define FSR_BRINGUP_STATE_ENTRY(state)	{FSR_STATE_BRINGUP_##state, #state}
+static const struct hda_dsp_msg_code fsr_bringup_state_names[] = {
+	FSR_BRINGUP_STATE_ENTRY(INIT),
+	FSR_BRINGUP_STATE_ENTRY(INIT_DONE),
+	FSR_BRINGUP_STATE_ENTRY(HPSRAM_LOAD),
+	FSR_BRINGUP_STATE_ENTRY(UNPACK_START),
+	FSR_BRINGUP_STATE_ENTRY(IMR_RESTORE),
+	FSR_BRINGUP_STATE_ENTRY(FW_ENTERED),
+};
+
+#define FSR_WAIT_STATE_ENTRY(state)	{FSR_WAIT_FOR_##state, #state}
+static const struct hda_dsp_msg_code fsr_wait_state_names[] = {
+	FSR_WAIT_STATE_ENTRY(IPC_BUSY),
+	FSR_WAIT_STATE_ENTRY(IPC_DONE),
+	FSR_WAIT_STATE_ENTRY(CACHE_INVALIDATION),
+	FSR_WAIT_STATE_ENTRY(LP_SRAM_OFF),
+	FSR_WAIT_STATE_ENTRY(DMA_BUFFER_FULL),
+	FSR_WAIT_STATE_ENTRY(CSE_CSR),
+};
+
+#define FSR_MODULE_NAME_ENTRY(mod)	[FSR_MOD_##mod] = #mod
+static const char * const fsr_module_names[] = {
+	FSR_MODULE_NAME_ENTRY(ROM),
+	FSR_MODULE_NAME_ENTRY(ROM_BYP),
+	FSR_MODULE_NAME_ENTRY(BASE_FW),
+	FSR_MODULE_NAME_ENTRY(LP_BOOT),
+	FSR_MODULE_NAME_ENTRY(BRNGUP),
+	FSR_MODULE_NAME_ENTRY(ROM_EXT),
+};
+
+static const char *
+hda_dsp_get_state_text(u32 code, const struct hda_dsp_msg_code *msg_code,
+		       size_t array_size)
 {
-	const struct sof_intel_dsp_desc *chip;
-	u32 status;
 	int i;
 
-	chip = get_chip_info(sdev->pdata);
-	status = snd_sof_dsp_read(sdev, HDA_DSP_BAR,
-				  chip->rom_status_reg);
-
-	for (i = 0; i < ARRAY_SIZE(hda_dsp_rom_msg); i++) {
-		if (status == hda_dsp_rom_msg[i].code) {
-			dev_printk(level, sdev->dev, "%s - code %8.8x\n",
-				   hda_dsp_rom_msg[i].msg, status);
-			return;
-		}
+	for (i = 0; i < array_size; i++) {
+		if (code == msg_code[i].code)
+			return msg_code[i].text;
 	}
 
+	return NULL;
+}
+
+static void hda_dsp_get_state(struct snd_sof_dev *sdev, const char *level)
+{
+	const struct sof_intel_dsp_desc *chip = get_chip_info(sdev->pdata);
+	const char *state_text, *error_text, *module_text;
+	u32 fsr, state, wait_state, module, error_code;
+
+	fsr = snd_sof_dsp_read(sdev, HDA_DSP_BAR, chip->rom_status_reg);
+	state = FSR_TO_STATE_CODE(fsr);
+	wait_state = FSR_TO_WAIT_STATE_CODE(fsr);
+	module = FSR_TO_MODULE_CODE(fsr);
+
+	if (module > FSR_MOD_ROM_EXT)
+		module_text = "unknown";
+	else
+		module_text = fsr_module_names[module];
+
+	if (module == FSR_MOD_BRNGUP)
+		state_text = hda_dsp_get_state_text(state, fsr_bringup_state_names,
+						    ARRAY_SIZE(fsr_bringup_state_names));
+	else
+		state_text = hda_dsp_get_state_text(state, fsr_rom_state_names,
+						    ARRAY_SIZE(fsr_rom_state_names));
+
 	/* not for us, must be generic sof message */
-	dev_dbg(sdev->dev, "unknown ROM status value %8.8x\n", status);
+	if (!state_text) {
+		dev_printk(level, sdev->dev, "%#010x: unknown ROM status value\n", fsr);
+		return;
+	}
+
+	if (wait_state) {
+		const char *wait_state_text;
+
+		wait_state_text = hda_dsp_get_state_text(wait_state, fsr_wait_state_names,
+							 ARRAY_SIZE(fsr_wait_state_names));
+		if (!wait_state_text)
+			wait_state_text = "unknown";
+
+		dev_printk(level, sdev->dev,
+			   "%#010x: module: %s, state: %s, waiting for: %s, %s\n",
+			   fsr, module_text, state_text, wait_state_text,
+			   fsr & FSR_HALTED ? "not running" : "running");
+	} else {
+		dev_printk(level, sdev->dev, "%#010x: module: %s, state: %s, %s\n",
+			   fsr, module_text, state_text,
+			   fsr & FSR_HALTED ? "not running" : "running");
+	}
+
+	error_code = snd_sof_dsp_read(sdev, HDA_DSP_BAR, chip->rom_status_reg + 4);
+	if (!error_code)
+		return;
+
+	error_text = hda_dsp_get_state_text(error_code, hda_dsp_rom_fw_error_texts,
+					    ARRAY_SIZE(hda_dsp_rom_fw_error_texts));
+	if (!error_text)
+		error_text = "unknown";
+
+	if (state == FSR_STATE_FW_ENTERED)
+		dev_printk(level, sdev->dev, "status code: %#x (%s)\n", error_code,
+			   error_text);
+	else
+		dev_printk(level, sdev->dev, "error code: %#x (%s)\n", error_code,
+			   error_text);
 }
 
 static void hda_dsp_get_registers(struct snd_sof_dev *sdev,
@@ -482,7 +589,7 @@ void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags)
 	u32 stack[HDA_DSP_STACK_DUMP_SIZE];
 
 	/* print ROM/FW status */
-	hda_dsp_get_status(sdev, level);
+	hda_dsp_get_state(sdev, level);
 
 	if (flags & SOF_DBG_DUMP_REGS) {
 		u32 status = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_SRAM_REG_FW_STATUS);
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index dc713c20ba1d..77c74e3c09b1 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -187,6 +187,69 @@
 
 #define HDA_DSP_STACK_DUMP_SIZE			32
 
+/* ROM/FW status register */
+#define FSR_STATE_MASK				GENMASK(23, 0)
+#define FSR_WAIT_STATE_MASK			GENMASK(27, 24)
+#define FSR_MODULE_MASK				GENMASK(30, 28)
+#define FSR_HALTED				BIT(31)
+#define FSR_TO_STATE_CODE(x)			((x) & FSR_STATE_MASK)
+#define FSR_TO_WAIT_STATE_CODE(x)		(((x) & FSR_WAIT_STATE_MASK) >> 24)
+#define FSR_TO_MODULE_CODE(x)			(((x) & FSR_MODULE_MASK) >> 28)
+
+/* Wait states */
+#define FSR_WAIT_FOR_IPC_BUSY			0x1
+#define FSR_WAIT_FOR_IPC_DONE			0x2
+#define FSR_WAIT_FOR_CACHE_INVALIDATION		0x3
+#define FSR_WAIT_FOR_LP_SRAM_OFF		0x4
+#define FSR_WAIT_FOR_DMA_BUFFER_FULL		0x5
+#define FSR_WAIT_FOR_CSE_CSR			0x6
+
+/* Module codes */
+#define FSR_MOD_ROM				0x0
+#define FSR_MOD_ROM_BYP				0x1
+#define FSR_MOD_BASE_FW				0x2
+#define FSR_MOD_LP_BOOT				0x3
+#define FSR_MOD_BRNGUP				0x4
+#define FSR_MOD_ROM_EXT				0x5
+
+/* State codes (module dependent) */
+/* Module independent states */
+#define FSR_STATE_INIT				0x0
+#define FSR_STATE_INIT_DONE			0x1
+#define FSR_STATE_FW_ENTERED			0x5
+
+/* ROM states */
+#define FSR_STATE_ROM_INIT			FSR_STATE_INIT
+#define FSR_STATE_ROM_INIT_DONE			FSR_STATE_INIT_DONE
+#define FSR_STATE_ROM_CSE_MANIFEST_LOADED	0x2
+#define FSR_STATE_ROM_FW_MANIFEST_LOADED	0x3
+#define FSR_STATE_ROM_FW_FW_LOADED		0x4
+#define FSR_STATE_ROM_FW_ENTERED		FSR_STATE_FW_ENTERED
+#define FSR_STATE_ROM_VERIFY_FEATURE_MASK	0x6
+#define FSR_STATE_ROM_GET_LOAD_OFFSET		0x7
+#define FSR_STATE_ROM_FETCH_ROM_EXT		0x8
+#define FSR_STATE_ROM_FETCH_ROM_EXT_DONE	0x9
+
+/* (ROM) CSE states */
+#define FSR_STATE_ROM_CSE_IMR_REQUEST			0x10
+#define FSR_STATE_ROM_CSE_IMR_GRANTED			0x11
+#define FSR_STATE_ROM_CSE_VALIDATE_IMAGE_REQUEST	0x12
+#define FSR_STATE_ROM_CSE_IMAGE_VALIDATED		0x13
+
+#define FSR_STATE_ROM_CSE_IPC_IFACE_INIT	0x20
+#define FSR_STATE_ROM_CSE_IPC_RESET_PHASE_1	0x21
+#define FSR_STATE_ROM_CSE_IPC_OPERATIONAL_ENTRY	0x22
+#define FSR_STATE_ROM_CSE_IPC_OPERATIONAL	0x23
+#define FSR_STATE_ROM_CSE_IPC_DOWN		0x24
+
+/* BRINGUP (or BRNGUP) states */
+#define FSR_STATE_BRINGUP_INIT			FSR_STATE_INIT
+#define FSR_STATE_BRINGUP_INIT_DONE		FSR_STATE_INIT_DONE
+#define FSR_STATE_BRINGUP_HPSRAM_LOAD		0x2
+#define FSR_STATE_BRINGUP_UNPACK_START		0X3
+#define FSR_STATE_BRINGUP_IMR_RESTORE		0x4
+#define FSR_STATE_BRINGUP_FW_ENTERED		FSR_STATE_FW_ENTERED
+
 /* ROM  status/error values */
 #define HDA_DSP_ROM_STS_MASK			GENMASK(23, 0)
 #define HDA_DSP_ROM_INIT			0x1
-- 
2.37.0

[PATCH 2/3] ASoC: SOF: Intel: hda-loader: Use the FSR state definitions during bootup

[Peter Ujfalusi]

Switch to use the newly added FSR (Firmware State Register) definitions
for DSP state handling and targeting.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index 819b3b08c655..304fa3923f1a 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -177,14 +177,13 @@ int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag, bool imr_boot)
 	 * - IMR boot: wait for ROM firmware entered (firmware booted up from IMR)
 	 */
 	if (imr_boot)
-		target_status = HDA_DSP_ROM_FW_ENTERED;
+		target_status = FSR_STATE_FW_ENTERED;
 	else
-		target_status = HDA_DSP_ROM_INIT;
+		target_status = FSR_STATE_INIT_DONE;
 
 	ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_BAR,
 					chip->rom_status_reg, status,
-					((status & HDA_DSP_ROM_STS_MASK)
-						== target_status),
+					(FSR_TO_STATE_CODE(status) == target_status),
 					HDA_DSP_REG_POLL_INTERVAL_US,
 					chip->rom_init_timeout *
 					USEC_PER_MSEC);
@@ -292,8 +291,7 @@ int hda_cl_copy_fw(struct snd_sof_dev *sdev, struct hdac_ext_stream *hext_stream
 
 	status = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_BAR,
 					chip->rom_status_reg, reg,
-					((reg & HDA_DSP_ROM_STS_MASK)
-						== HDA_DSP_ROM_FW_ENTERED),
+					(FSR_TO_STATE_CODE(reg) == FSR_STATE_FW_ENTERED),
 					HDA_DSP_REG_POLL_INTERVAL_US,
 					HDA_DSP_BASEFW_TIMEOUT_US);
 
-- 
2.37.0

[PATCH 3/3] ASoC: SOF: Intel: hda: Drop no longer used ROM state definitions

[Peter Ujfalusi]

All code have been switched to use the new FSR defines and macros for
ROM/FW state tracking.
The old definitions can be dropped now.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 77c74e3c09b1..effa1b0f556d 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -251,12 +251,6 @@
 #define FSR_STATE_BRINGUP_FW_ENTERED		FSR_STATE_FW_ENTERED
 
 /* ROM  status/error values */
-#define HDA_DSP_ROM_STS_MASK			GENMASK(23, 0)
-#define HDA_DSP_ROM_INIT			0x1
-#define HDA_DSP_ROM_FW_MANIFEST_LOADED		0x3
-#define HDA_DSP_ROM_FW_FW_LOADED		0x4
-#define HDA_DSP_ROM_FW_ENTERED			0x5
-#define HDA_DSP_ROM_RFW_START			0xf
 #define HDA_DSP_ROM_CSE_ERROR			40
 #define HDA_DSP_ROM_CSE_WRONG_RESPONSE		41
 #define HDA_DSP_ROM_IMR_TO_SMALL		42
-- 
2.37.0

Re: [PATCH 0/3] ASoC: SOF: Intel: hda: Correct Firmware State Register use

[Mark Brown]

On Tue, 12 Jul 2022 15:57:31 +0300, Peter Ujfalusi wrote:
> The FSR (Firmware State Register) holds the ROM state information, it does not
> contain error information.
> The FSR itself is a bit more complicated as well as the state depends on the
> module currently in use.
> 
> The error code from ROM or the status code from the firmware is located at the
> next register.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: SOF: Intel: hda: Correct the ROM/FW state reporting code
      commit: 15d8370cf6d5b3316ad58954086433301363be67

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 0/3] ASoC: SOF: Intel: hda: Correct Firmware State Register use

[Mark Brown]

On Tue, 12 Jul 2022 15:57:31 +0300, Peter Ujfalusi wrote:
> The FSR (Firmware State Register) holds the ROM state information, it does not
> contain error information.
> The FSR itself is a bit more complicated as well as the state depends on the
> module currently in use.
> 
> The error code from ROM or the status code from the firmware is located at the
> next register.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: SOF: Intel: hda: Correct the ROM/FW state reporting code
      (no commit info)
[2/3] ASoC: SOF: Intel: hda-loader: Use the FSR state definitions during bootup
      commit: 43a03d247091e1fcd3065dae3407b959e8921c16
[3/3] ASoC: SOF: Intel: hda: Drop no longer used ROM state definitions
      commit: 8613753a681e7a5c63313dea9b04bf103d601368

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark