base_fw: update mem_state_info TLV #8871

dnikodem · 2024-02-19T09:31:35Z

This change improves the TLV assembly algorithm in accordance with how the part for hpsram is assembled. In the case when ebb_state_dword_count is equal to 0, we skip adding the LSPGCTL register value for lpsram.

The following PR must be merged into the Zephyr repository before we can merge this PR:

intel_adsp: adsp_memory: add lpsram and hpsram struct definition zephyrproject-rtos/zephyr#70221

lgirdwood · 2024-02-19T13:05:35Z

src/platform/lunarlake/include/platform/lib/shim.h

@@ -47,7 +47,8 @@
 #define L2HSBPM(x)			(0x17A800 + 0x0008 * (x))
 #define SHIM_HSPGCTL(x)			(L2HSBPM(x) + 0x0000)

-#define LSPGCTL				0x71D80
+#define LSPGCTL0		0x71D80


@dnikodem can we read this from Zephyr headers today ?

@dnikodem sorry I mean we should use the Zephyr header for these HW specific macros (and not use any duplicated values from the SOF directory which is deprecated).

ok, I will update this PR, thanks.

dnikodem · 2024-03-17T18:25:58Z

Added zephyrproject-rtos/zephyr#70221 - this is the first step to clean up SOF headers.

lyakh · 2024-03-18T08:14:24Z

src/audio/base_fw.c

 #endif
+	}
 	tuple_data[index++] = info.page_alloc_struct.page_alloc_count;
 	ptr = (uint16_t *)(tuple_data + index);


this of course changes the behaviour significantly. Before this commit index was incremented twice in lines 226 and 228, now it isn't any more. Is this intended? Was that a bug?

The tuple_data should contain the correct number of LPSRAM PGCTL register values (in the same way as it is done for HPSRAM). In the previous solution, if the number of ebb_state_dword_count equaled 0, a dword was added in tuple_data with the PGCTL status value for a non-existing/not used LPSRAM bank.

Therefore, the parser that is using data from tuple_data should not expect an "extra" LSPGCTL status value in the tuple_data, and then it could misinterpret it as some other type of data or status.

@dnikodem right, but if ebb_state_dword_count != 0? Then you'd add several register values into tuple_data without incrementing index, so after the loop you'd overwrite the first of those values... I think you actually wanted to do

for (i = 0; i < info.ebb_state_dword_count; i++) { #ifdef INTEL_ADSP tuple_data[index++] = LPSRAM_REGS(i)->USxPGCTL; #else tuple_data[index++] = 0; #endif } if (info.ebb_state_dword_count) tuple_data[index++] = info.page_alloc_struct.page_alloc_count;

@lyakh Fixed, thanks

lyakh · 2024-04-12T07:50:33Z

src/audio/base_fw.c

 #endif
+	}
 	tuple_data[index++] = info.page_alloc_struct.page_alloc_count;
 	ptr = (uint16_t *)(tuple_data + index);


@dnikodem right, but if ebb_state_dword_count != 0? Then you'd add several register values into tuple_data without incrementing index, so after the loop you'd overwrite the first of those values... I think you actually wanted to do

for (i = 0; i < info.ebb_state_dword_count; i++) { #ifdef INTEL_ADSP tuple_data[index++] = LPSRAM_REGS(i)->USxPGCTL; #else tuple_data[index++] = 0; #endif } if (info.ebb_state_dword_count) tuple_data[index++] = info.page_alloc_struct.page_alloc_count;

lgirdwood

@dnikodem I assume you will make non draft after the Zephyr part is merged and we can rebase/merge.

dnikodem · 2024-04-16T12:14:16Z

@dnikodem I assume you will make non draft after the Zephyr part is merged and we can rebase/merge.

correct, we need firstly zephyr part merged.

bug fixed

kv2019i · 2024-05-07T11:32:09Z

Oops, @dnikodem I didn't realize this PR was still open as well. I was intending to get he pending base_fw changes out of the wait, but missed this one. This will now conflict with #9103

Let's see which gets the reviews together first, and then rebase the other one.

lgirdwood · 2024-05-08T10:20:13Z

Oops, @dnikodem I didn't realize this PR was still open as well. I was intending to get he pending base_fw changes out of the wait, but missed this one. This will now conflict with #9103

Let's see which gets the reviews together first, and then rebase the other one.

oh, yeah - this one was blocked on Zephyr PR merging (now merged). @dnikodem do we need a west update ? I think @kv2019i did a west update a few days ago so we may be good just to rebase this ?

dnikodem · 2024-05-08T10:23:31Z

Yes, the west was updated by Kai and points on correct commit. Here is needed to rebase / resolve conflict with base_fw_platform.c file. I will do this today.

lgirdwood

Just one question on ifdefs, but otherwise LGTM.

src/audio/base_fw_intel.c

kv2019i

Thanks @dnikodem , this is a welcome update. Code looks good, but I think you can go a bit further and drop the ifdefs as this code is now only compiled for Intel targets that have enabled the IPC4 basefw Intel extensions (so should cover all platforms now covered by INTEL_ADSP ifdef guard).

src/audio/base_fw_intel.c

kv2019i · 2024-05-09T08:20:19Z

src/audio/base_fw_intel.c

@@ -154,7 +156,13 @@ static int basefw_mem_state_info(uint32_t *data_offset, char *data)
 	index = 0;
 	tuple_data[index++] = info.free_phys_mem_pages;
 	tuple_data[index++] = info.ebb_state_dword_count;
-	tuple_data[index++] = io_reg_read(LSPGCTL);
+	for (i = 0; i < info.ebb_state_dword_count; i++) {
+#ifdef INTEL_ADSP


Likewise here.

Fixed, thanks :)

This commit refactors the memory power management register access to use the HPSRAM_REGS and LPSRAM_REGS macros instead of direct io_reg_read calls. Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>

kv2019i

Thanks, looks good now!

lgirdwood

Nice to remove all the old macros :)

lgirdwood reviewed Feb 19, 2024

View reviewed changes

lyakh reviewed Mar 18, 2024

View reviewed changes

dnikodem force-pushed the update_tlv_mem_state_info branch 2 times, most recently from a2f6588 to 749e8a4 Compare April 11, 2024 16:30

lyakh previously requested changes Apr 12, 2024

View reviewed changes

dnikodem force-pushed the update_tlv_mem_state_info branch from 749e8a4 to 72de4c0 Compare April 16, 2024 08:56

lgirdwood reviewed Apr 16, 2024

View reviewed changes

dnikodem force-pushed the update_tlv_mem_state_info branch from 72de4c0 to 2a9a70c Compare May 7, 2024 10:34

dnikodem marked this pull request as ready for review May 7, 2024 11:09

dnikodem requested review from plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners May 7, 2024 11:09

dnikodem force-pushed the update_tlv_mem_state_info branch from 2a9a70c to e1c8e7d Compare May 8, 2024 11:02

lgirdwood reviewed May 8, 2024

View reviewed changes

src/audio/base_fw_intel.c Outdated Show resolved Hide resolved

kv2019i reviewed May 9, 2024

View reviewed changes

audio: base_fw_intel: refactor memory power management register access

7d2af0d

This commit refactors the memory power management register access to use the HPSRAM_REGS and LPSRAM_REGS macros instead of direct io_reg_read calls. Signed-off-by: Damian Nikodem <damian.nikodem@intel.com>

dnikodem force-pushed the update_tlv_mem_state_info branch from e1c8e7d to 7d2af0d Compare May 10, 2024 10:18

kv2019i approved these changes May 10, 2024

View reviewed changes

lgirdwood approved these changes May 13, 2024

View reviewed changes

lgirdwood merged commit 529a4c6 into thesofproject:main May 13, 2024
43 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

base_fw: update mem_state_info TLV #8871

base_fw: update mem_state_info TLV #8871

dnikodem commented Feb 19, 2024 •

edited

Loading

lgirdwood Feb 19, 2024

lgirdwood Feb 19, 2024

dnikodem Feb 19, 2024

dnikodem commented Mar 17, 2024

lyakh Mar 18, 2024

dnikodem Mar 18, 2024 •

edited

Loading

lyakh Apr 12, 2024

dnikodem Apr 23, 2024

lyakh Apr 12, 2024

lgirdwood left a comment

dnikodem commented Apr 16, 2024

kv2019i commented May 7, 2024

lgirdwood commented May 8, 2024

dnikodem commented May 8, 2024 •

edited

Loading

lgirdwood left a comment

kv2019i left a comment

kv2019i May 9, 2024

dnikodem May 10, 2024

kv2019i left a comment

lgirdwood left a comment

base_fw: update mem_state_info TLV #8871

base_fw: update mem_state_info TLV #8871

Conversation

dnikodem commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnikodem commented Mar 17, 2024

Choose a reason for hiding this comment

dnikodem Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

dnikodem commented Apr 16, 2024

kv2019i commented May 7, 2024

lgirdwood commented May 8, 2024

dnikodem commented May 8, 2024 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

dnikodem commented Feb 19, 2024 •

edited

Loading

dnikodem Mar 18, 2024 •

edited

Loading

dnikodem commented May 8, 2024 •

edited

Loading