Skip to content

cavs: memory bank powerup flow adjustment - #7065

Merged
lgirdwood merged 1 commit into
mainfrom
mbukowsk/memory-powerup
Feb 13, 2023
Merged

cavs: memory bank powerup flow adjustment#7065
lgirdwood merged 1 commit into
mainfrom
mbukowsk/memory-powerup

Conversation

@mbukowsk

@mbukowskmbukowsk commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

Memory banks enablement flow is adjusted to reccommended approach. Power status should be read twice to ensure ebb readiness

Signed-off-by: Michal Bukowski michal.bukowski@intel.com

@mbukowsk
mbukowsk marked this pull request as draft February 9, 2023 12:12
@mbukowskmbukowsk mentioned this pull request Feb 9, 2023
Closed
pblaszko
pblaszko reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/platform/intel/cavs/include/cavs/lib/pm_memory.h Outdated
@mbukowsk
mbukowskforce-pushed the mbukowsk/memory-powerup branch from 02f6d3b to 90d5316CompareFebruary 9, 2023 12:53
@lgirdwoodlgirdwood added the P1 Blocker bugs or important features label Feb 9, 2023
lgirdwood
lgirdwood approved these changes Feb 9, 2023
View reviewed changes

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mwasko@cujomalainey LGTM.

@mbukowsk

mbukowsk commented Feb 9, 2023

Copy link
Copy Markdown
ContributorAuthor

@cujomalainey Could you please verify this fix on your setup?

abonislawski
abonislawski approved these changes Feb 9, 2023
View reviewed changes

@abonislawskiabonislawski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@wszypelt can you check CI issue?

@lgirdwood

lgirdwood commented Feb 9, 2023

Copy link
Copy Markdown
Member

@mbukowsk still draft ?

@plbossart

plbossart commented Feb 9, 2023

Copy link
Copy Markdown
Member

@lgirdwood this PR looks like a prime candidate for a stable-2.2 update?

@mbukowsk

mbukowsk commented Feb 9, 2023

Copy link
Copy Markdown
ContributorAuthor

@mbukowsk still draft ?

Still fix is not verified and CI not working

@mbukowsk
mbukowskforce-pushed the mbukowsk/memory-powerup branch from 90d5316 to 43a4f19CompareFebruary 9, 2023 16:55
@mbukowsk

mbukowsk commented Feb 9, 2023

Copy link
Copy Markdown
ContributorAuthor

I've done cosmetic change. for loop is added instead of code duplication. Primarily code was duplicated due to ifdef for cavs 2_5. Fix is applicable for all cavs versions, define is removed so same code is executed in simple loop.

@cujomalainey

cujomalainey commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

Got it cycling, I'll check it tonight, probably will hit about 2k cycles by then

cujomalainey
cujomalainey reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/platform/intel/cavs/include/cavs/lib/pm_memory.h Outdated
@mbukowsk
mbukowskforce-pushed the mbukowsk/memory-powerup branch from 43a4f19 to 4c77545CompareFebruary 9, 2023 21:44
@cujomalainey

cujomalainey commented Feb 9, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Just shy of 400 cycles right now. Probably will hit 1k tonight. If its still going by then I will give my +1 so we can submit, but will still let it run through the night

cujomalainey
cujomalainey approved these changes Feb 10, 2023
View reviewed changes

@cujomalaineycujomalainey left a comment

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.

935 cycles passed so far

@mwasko

mwasko commented Feb 10, 2023

Copy link
Copy Markdown
Contributor

SOFCI TEST

@mbukowsk
mbukowsk marked this pull request as ready for review February 10, 2023 09:33
@kv2019i

kv2019i commented Feb 10, 2023

Copy link
Copy Markdown
Collaborator

#6864 hit once in https://sof-ci.01.org/sofpr/PR7065/build3847/devicetest/index.html

The qemu-failures are more complicated, the changed code affects CNL and ICL and the qemu check in CI now fails, so this seems to be related to the code.

OTOH, we have #7000 review which will remove CNL and ICL from mainline, making the failures moot.

@lgirdwood@mwasko guidance how to proceed? I can't immediately see how the change of this PR can break the qemu test, but apparently it does.

lyakh
lyakh reviewed Feb 10, 2023
View reviewed changes
Comment threadsrc/platform/intel/cavs/include/cavs/lib/pm_memory.h Outdated
Comment threadsrc/platform/intel/cavs/include/cavs/lib/pm_memory.h Outdated

@lyakhlyakhFeb 10, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not sure I understand. Does this mean, that once we successfully identified the register value to match what we expect, it can change and we have to run the loop again to catch a second such change? Or do we just have to

idelay();
while (io_reg_read() != expected)
idelay();
idelay();
io_reg_read();

? If this is the case, I wouldn't use a loop there but just add that additional register read (possibly with a delay) after the loop.

@cujomalaineycujomalaineyFeb 10, 2023

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.

regarding your recommendation, is that not what the outer loop enforces?

@mbukowsk
cavs: memory bank powerup flow adjustment
cbf5199
Memory banks enablement flow is adjusted to recommended approach.
Power status should be read twice to ensure ebb readiness
Signed-off-by: Michal Bukowski <michal.bukowski@intel.com>
@mbukowsk
mbukowskforce-pushed the mbukowsk/memory-powerup branch from 4c77545 to cbf5199CompareFebruary 10, 2023 12:56
cujomalainey
cujomalainey reviewed Feb 10, 2023
View reviewed changes

@cujomalaineycujomalainey left a comment

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.

1930 cycles passed

@cujomalainey

cujomalainey commented Feb 12, 2023

Copy link
Copy Markdown
Contributor

stopped my test at 4600 reboots, i think we are good

@lgirdwood
lgirdwood merged commit aaac08a into mainFeb 13, 2023
@lgirdwood
lgirdwood deleted the mbukowsk/memory-powerup branch February 13, 2023 11:01
@cujomalainey

cujomalainey commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

This needs to be cherry picked to Rpl-001 asap

@sathyap-chrome

sathyap-chrome commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

created #7086 for picking to RPL-001

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lyakhlyakhlyakh left review comments
@lgirdwoodlgirdwoodlgirdwood approved these changes
@abonislawskiabonislawskiabonislawski approved these changes
@dnikodemdnikodemdnikodem approved these changes
@pblaszkopblaszkopblaszko approved these changes
@plbossartplbossartAwaiting requested review from plbossartplbossart is a code owner
@mmaka1mmaka1Awaiting requested review from mmaka1mmaka1 is a code owner
@lbetlejlbetlejAwaiting requested review from lbetlejlbetlej is a code owner
@dbalutadbalutaAwaiting requested review from dbalutadbaluta is a code owner
+1 more reviewer
@cujomalaineycujomalaineycujomalainey approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P1Blocker bugs or important features

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@mbukowsk@lgirdwood@plbossart@cujomalainey@mwasko@kv2019i@sathyap-chrome@lyakh@abonislawski@dnikodem@pblaszko