Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[on hold] Istep 8 6 #32

Open
wants to merge 12 commits into
base: talos_2_support
Choose a base branch
from
Open

[on hold] Istep 8 6 #32

wants to merge 12 commits into from

Conversation

miczyg1
Copy link
Member

@miczyg1 miczyg1 commented Feb 23, 2021

No description provided.

Signed-off-by: Michał Żygowski <michal.zygowski@3mdeb.com>
@miczyg1
Copy link
Member Author

miczyg1 commented Feb 23, 2021

Not build-tested. It seems it doesn't touch the hardware but calculates coefficients for later use.

src/soc/ibm/power9/isteps/istep8.c Outdated Show resolved Hide resolved
src/soc/ibm/power9/isteps/istep8.c Outdated Show resolved Hide resolved
src/soc/ibm/power9/isteps/istep8.c Outdated Show resolved Hide resolved
@krystian-hebel
Copy link
Member

@miczyg1 which values are written to ATTR_PROC_EPS_READ_CYCLES_T{0,1,2}?

*/
if ((eps_r[0] > eps_r[1]) || (eps_r[1] > eps_r[2]) || (eps_w[0] > eps_w[1]))
printk(BIOS_WARNING, "Invalid relationship between base epsilon values\n");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@krystian-hebel the attributes ATTR_PROC_EPS_READ_CYCLES_T{0,1,2,} are written right here. Their values are as follows:
ATTR_PROC_EPS_READ_CYCLES_T0 = eps_r[0];
ATTR_PROC_EPS_READ_CYCLES_T1 = eps_r[1];
ATTR_PROC_EPS_READ_CYCLES_T2 = eps_r[2];

ATTR_PROC_EPS_WRITE_CYCLES_T1 = eps_r[0];
ATTR_PROC_EPS_WRITE_CYCLES_T2 = eps_r[1];

@macpijan
Copy link
Member

@miczyg1 @krystian-hebel What is the status here?

@macpijan
Copy link
Member

macpijan commented Mar 17, 2021

@miczyg1 @krystian-hebel Could you update what is the status if this MR?

I am also wondering on the code structure. In this MR we have istep8.h/istep8.c, while in the others MRs we have a separate file for each minor istep (like here: #33)

Moreover, we already have some istep 8.x implementation here: https://github.com/3mdeb/coreboot/pull/39/files

The directory structure is also different, e.g. src/soc/ibm/power9/isteps/istep8.c vs src/soc/ibm/power9/istep_13_2.c

mmkow and others added 10 commits March 19, 2021 10:26
…) function name

Co-authored-by: krystian-hebel <40995177+krystian-hebel@users.noreply.github.com>
…eps_table_type variable

Co-authored-by: krystian-hebel <40995177+krystian-hebel@users.noreply.github.com>
Co-authored-by: krystian-hebel <40995177+krystian-hebel@users.noreply.github.com>
variables

Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
Removing ./isteps directory and istep8.c file

Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
src/soc/ibm/power9

Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
previously static variables

Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
global

Signed-off-by: Mateusz Kowalski <mateusz.kowalski@3mdeb.com>
@macpijan
Copy link
Member

@miczyg1 @krystian-hebel Please take a look at that.

@mmkow Please confirm if you believe it is finished

@macpijan
Copy link
Member

@mmkow Is it ready for review or not?

@mmkow mmkow changed the title Istep 8 6 WIP: Istep 8 6 Mar 26, 2021
@mmkow
Copy link

mmkow commented Mar 26, 2021

No, it still needs some work. I've changed its status to wip.

@macpijan
Copy link
Member

@mmkow Ok. Make sure to ping me and @krystian-hebel once ready.

@mmkow mmkow changed the title WIP: Istep 8 6 Istep 8 6 Jun 7, 2021
@mmkow
Copy link

mmkow commented Jun 7, 2021

@macpijan @krystian-hebel

* CHIP_IS_NODE = MODE1 = default
* CHIP_IS_GROUP = MODE2
*/
uint8_t pump_mode = CHIP_IS_GROUP; /* ATTR_PROC_FABRIC_PUMP_MODE MODE2 from talos.xml */
Copy link
Member

Choose a reason for hiding this comment

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

This should be read from here: https://github.com/open-power/hostboot/blob/master/src/include/usr/initservice/mboxRegs.H#L163
Value from talos.xml is written by SBE, but it may change for different platforms or even different versions of SBE.

Comment on lines +270 to +271
if (!p9_calculate_frequencies(&core_floor_ratio, &core_ceiling_ratio,
&freq_fbc, &freq_core_ceiling)) {
Copy link
Member

Choose a reason for hiding this comment

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

freq_fbc is uninitialized here, it is later compared in p9_calculate_frequencies() and never written to.

Comment on lines +51 to +56
uint32_t freq_core_floor = 4800;
/* According to src/usr/targeting/common/xmltohb/attribute_types.xml
ATTR_FREQ_CORE_NOMINAL_MHZ maps to NOMINAL_FREQ_MHZ present in talos.xml */
uint32_t freq_core_nom = 4800;
uint8_t async_safe_mode = FABRIC_ASYNC_PERFORMANCE_MODE;
*freq_ceiling = 4800;
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/3mdeb/openpower-coreboot-docs/blob/main/logs/scom_dumps/istep_6_12_host_voltage_config.log#L40 and surrounding lines. Obviously the default values are not the same as actual.

@macpijan macpijan changed the title Istep 8 6 [on hold] Istep 8 6 Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants