base_fw: update mem_state_info TLV by dnikodem · Pull Request #8871 · thesofproject/sof

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

 #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

 #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

 #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.

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).

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

 	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 dbaluta, kv2019i, lbetlej, mmaka1 and plbossart 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

Comment thread src/audio/base_fw_intel.c Outdated

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

Conversation

dnikodem commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnikodem commented Mar 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnikodem Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

dnikodem commented Apr 16, 2024

Uh oh!

kv2019i commented May 7, 2024

Uh oh!

lgirdwood commented May 8, 2024

Uh oh!

dnikodem commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnikodem commented Feb 19, 2024 •

edited

Loading

dnikodem Mar 18, 2024 •

edited

Loading

dnikodem commented May 8, 2024 •

edited

Loading