bootloader: enable LPSRAM memory for cannonlake by mrajwa · Pull Request #1552 · thesofproject/sof

mrajwa · 2019-06-12T09:45:43Z

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

tlauda

Could we create something like CONFIG_LP_SRAM?
Adding additional platforms is not a good idea.

mrajwa · 2019-06-12T10:49:12Z

Yes this is good idea I think.

lgirdwood

Commit message.

jajanusz · 2019-06-12T21:47:40Z

config LP_SRAM

Why its not "depends on CAVS" if LP_MEMORY_BANKS depends on CAVS?

Anyway the best option:
LP_SRAM should depends on CAVS and LP_MEMORY_BANKS depends on LP_SRAM

tlauda

You are not selecting LP_SRAM for any platform.

tlauda · 2019-06-12T22:45:46Z

Better to use #if CONFIG_LP_SRAM.

The question is should we enable LPSRAM in defconfig? And if so for what platforms?

we can select LP_SRAM under platform config, e.g. select LP_SRAM under CONFIG_APOLLOLAKE and CONFIG_CANNOLAKE.

@mrajwa I would expect to have it enabled by default for APL and CNL.

jajanusz · 2019-06-13T09:35:46Z

It got a bit messy, we have LSPGCTL once used as shim (here) and another time as just memory mng register to turn it off at all (in asm_memory_management). Please clean it up.

abonislawski · 2019-06-13T11:30:48Z

Why its not "depends on CAVS" if LP_MEMORY_BANKS depends on CAVS?

Anyway the best option:
LP_SRAM should depends on CAVS and LP_MEMORY_BANKS depends on LP_SRAM

abonislawski · 2019-06-13T11:38:06Z

With this change you will have whole LP_SRAM for heap buffer but also in the same place there will be heaps like HEAP_LP_SYSTEM_BASE or HEAP_LP_RUNTIME_BASE, change them if not needed

@abonislawski, and this is the intention we need whole LPSRAM for heap buffer.

@mrajwa I mean in this case not needed defines like HEAP_LP_RUNTIME_BASE should be deleted to make this clear because right now two defines will point to the same memory and we need to check which one is actually used

jajanusz · 2019-06-13T23:35:49Z

Ending newline pls

To be able to use register read interfaces like shim_read() we need to specify offset. This patch adds such offset for LSPGCTL register. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-07-25T13:52:17Z

CI fails because FW touches LP related defines even though they have been disabled. I will commit further in regards to this.

mrajwa · 2019-07-26T09:11:14Z

@jajanusz @tlauda does it look OK now?

tlauda

Small style change and we're good to go.

tlauda · 2019-07-26T09:15:02Z

Go with the rest of the source code: #if.

tlauda · 2019-07-26T09:15:24Z

tlauda · 2019-07-26T09:16:28Z

tlauda · 2019-07-26T09:16:39Z

mrajwa · 2019-07-26T10:09:38Z

@tlauda, updated.

tlauda

@jajanusz Do you know what is the meaning of checkpatch complain?

mrajwa · 2019-07-26T10:41:57Z

@tlauda I found it complains -> "WARNING: please write a paragraph that describes the config symbol fully... FILE: Kconfig:28"

mrajwa · 2019-07-26T10:55:59Z

@tlauda @jajanusz, I have fixed that checkpatch warning also.

jajanusz · 2019-07-26T16:04:32Z

@jajanusz Do you know what is the meaning of checkpatch complain?

@tlauda I found it complains -> "WARNING: please write a paragraph that describes the config symbol fully... FILE: Kconfig:28"

Yea, for checkpatch fully means 4 lines ;) No need to always satisfy this check as sometimes there are not so much to write about config

jajanusz · 2019-07-26T16:06:35Z

no need for depends here (as above)

jajanusz · 2019-07-26T16:10:57Z

I think you may remove and all related defines will return zero, it's better not to write implemenation details, especially when this statement is not true, because it will result in different (0/false/undefined) values depending in what enviroment config is used (fe cmake/make vs C preprocessor vs C compiler).

Stale review, feel free to rereview tho.

Choose to power ON LPSRAM banks in Kconfig menu. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch changes the rules of LPSRAM enabling. Now it is no longer platform dependent but rather rely on config selection. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Enable whole LPSRAM bank to be allocated by heap buffer. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch enables LPSRAM memory by default on all APL platforms. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

jajanusz

@mrajwa please squash UT commit with breaking commit

This patch sets all LPSRAM related memory defines to zero if CONFIG_LP_MEMORY_BANKS is not defined. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

marc-hb · 2020-06-02T17:43:45Z

 #define PLATFORM_LPSRAM_EBB_COUNT	CONFIG_LP_MEMORY_BANKS
+#else
+#define PLATFORM_LPSRAM_EBB_COUNT	0
+#endif


@mrajwa , the preprocessor replaces undefined macros with 0 so I don't see when or how the newer and more convoluted code behaves differently from the previous code, can you explain? Besides moving any -Wundef warning to the previous line I couldn't find any difference.

mrajwa requested a review from tlauda as a code owner June 12, 2019 09:45

tlauda suggested changes Jun 12, 2019

View reviewed changes

mrajwa force-pushed the ghw_dev branch from 5694d7b to f69a10b Compare June 12, 2019 11:42

lgirdwood previously requested changes Jun 12, 2019

View reviewed changes

mrajwa force-pushed the ghw_dev branch from f69a10b to 09803da Compare June 12, 2019 21:44

mrajwa requested review from jajanusz, lbetlej, mmaka1 and plbossart as code owners June 12, 2019 21:44

jajanusz reviewed Jun 12, 2019

View reviewed changes

tlauda suggested changes Jun 12, 2019

View reviewed changes

jajanusz reviewed Jun 13, 2019

View reviewed changes

abonislawski previously requested changes Jun 13, 2019

View reviewed changes

mrajwa force-pushed the ghw_dev branch 2 times, most recently from 3f351c8 to 3a0cfd6 Compare June 13, 2019 23:33

jajanusz reviewed Jun 13, 2019

View reviewed changes

Comment thread src/arch/xtensa/configs/apollolake_defconfig Outdated

jajanusz Jun 13, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ending newline pls

mrajwa force-pushed the ghw_dev branch from 3a0cfd6 to 9d93de3 Compare June 27, 2019 22:39

shim: add LSPGCTL register relative offset

cd92c18

To be able to use register read interfaces like shim_read() we need to specify offset. This patch adds such offset for LSPGCTL register. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_dev branch from 9d93de3 to 44ec4b9 Compare July 25, 2019 12:00

mrajwa added the P1 Blocker bugs or important features label Jul 25, 2019

tlauda suggested changes Jul 26, 2019

View reviewed changes

mrajwa force-pushed the ghw_dev branch from 57f3698 to a9477ae Compare July 26, 2019 10:09

tlauda approved these changes Jul 26, 2019

View reviewed changes

mrajwa force-pushed the ghw_dev branch from a9477ae to 74a1c27 Compare July 26, 2019 10:55

jajanusz suggested changes Jul 26, 2019

View reviewed changes

jajanusz requested a review from abonislawski July 26, 2019 16:13

jajanusz requested a review from lgirdwood July 26, 2019 16:16

mrajwa added 3 commits July 29, 2019 10:41

Kconfig: add option to enable CONFIG_LP_SRAM

7cedbb3

Choose to power ON LPSRAM banks in Kconfig menu. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

bootloader: enable LPSRAM depending on CONFIG

5096db4

This patch changes the rules of LPSRAM enabling. Now it is no longer platform dependent but rather rely on config selection. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

memory: fix LPSRAM defines on CANNONLAKE

ea10d48

Enable whole LPSRAM bank to be allocated by heap buffer. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_dev branch from 74a1c27 to 7a248d5 Compare July 29, 2019 08:49

mrajwa added 2 commits July 29, 2019 11:04

kconfig: enable LPSRAM by default for APL

c814628

This patch enables LPSRAM memory by default on all APL platforms. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kconfig: enable LPSRAM by default for CNL

5e40872

This patch enables LPSRAM memory by default on all APL platforms. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_dev branch from 7a248d5 to 03382d5 Compare July 29, 2019 09:24

jajanusz approved these changes Jul 29, 2019

View reviewed changes

jajanusz suggested changes Jul 30, 2019

View reviewed changes

memory: set LPSRAM related defines to zero if LP not configured

09957d2

This patch sets all LPSRAM related memory defines to zero if CONFIG_LP_MEMORY_BANKS is not defined. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_dev branch from 3a73b10 to 09957d2 Compare July 30, 2019 16:09

jajanusz approved these changes Jul 30, 2019

View reviewed changes

jajanusz merged commit d180cd3 into thesofproject:master Jul 30, 2019

marc-hb reviewed Jun 2, 2020

View reviewed changes

Conversation

mrajwa commented Jun 12, 2019

Uh oh!

tlauda left a comment

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Jun 12, 2019

Uh oh!

lgirdwood left a comment

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!

tlauda left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

abonislawski Jun 13, 2019 • 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!

mrajwa commented Jul 25, 2019

Uh oh!

mrajwa commented Jul 26, 2019

Uh oh!

tlauda left a comment

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!

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Jul 26, 2019

Uh oh!

tlauda left a comment

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented Jul 26, 2019

Uh oh!

jajanusz commented Jul 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jajanusz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

abonislawski Jun 13, 2019 •

edited

Loading

mrajwa commented Jul 26, 2019 •

edited

Loading