| Message ID | 1562761759-17255-1-git-send-email-ego@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | init: Perform pstates-init before dt blob creation | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (4db38a36b31045f0a116d388ddeac850b38c8680) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2019-07-10 17:59:19]: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > On FSP based systems (particularly POWER8), we perform > occ_pstates_init() late in the boot to allow OCC to be loaded. Hence > this was being performed in platform.exit(). occ_pstates_init() would > add pstate information into the device-tree. > > A recent commit 9fc0c1287ada ("Move FSP specific op-panel calls to > platform.exit()") moved the invocation of platform.exit() after the > creation of device-tree blob. As a result, on FSP based systems, we > don't have the pstate information in the device-tree, and thus the > Kernel is unable to perform frequency scaling. > > Fix this by moving occ_pstates_init() out of ibm_fsp_exit() and call > it before the creation of the device-tree blob. > > Fixes: commit 9fc0c1287ada ("Move FSP specific op-panel calls to > platform.exit()") > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com> > --- > core/init.c | 6 ++++++ > platforms/ibm-fsp/common.c | 7 ------- > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/core/init.c b/core/init.c > index d0f28f2..8eb0729 100644 > --- a/core/init.c > +++ b/core/init.c > @@ -557,6 +557,12 @@ void __noreturn load_and_boot_kernel(bool is_reboot) > if (!occ_sensors_init()) > dts_sensor_create_nodes(sensor_node); > > + /* > + * OCC takes few secs to boot on FSP systems. Call > + * this as late as as possible to avoid delay. > + */ > + if (!platform.bmc) > + occ_pstates_init(); > } else { > /* fdt will be rebuilt */ > free(fdt); Thanks Gautham for the fix. We need to add the DT binding before it is wrapped up and passed to host OS kernel. --Vaidy
On 07/10/2019 05:59 PM, Gautham R. Shenoy wrote: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > On FSP based systems (particularly POWER8), we perform > occ_pstates_init() late in the boot to allow OCC to be loaded. Hence > this was being performed in platform.exit(). occ_pstates_init() would > add pstate information into the device-tree. > > A recent commit 9fc0c1287ada ("Move FSP specific op-panel calls to > platform.exit()") moved the invocation of platform.exit() after the > creation of device-tree blob. As a result, on FSP based systems, we > don't have the pstate information in the device-tree, and thus the > Kernel is unable to perform frequency scaling. > > Fix this by moving occ_pstates_init() out of ibm_fsp_exit() and call > it before the creation of the device-tree blob. > > Fixes: commit 9fc0c1287ada ("Move FSP specific op-panel calls to > platform.exit()") > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> Thanks for the fix. > --- > core/init.c | 6 ++++++ > platforms/ibm-fsp/common.c | 7 ------- > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/core/init.c b/core/init.c > index d0f28f2..8eb0729 100644 > --- a/core/init.c > +++ b/core/init.c > @@ -557,6 +557,12 @@ void __noreturn load_and_boot_kernel(bool is_reboot) > if (!occ_sensors_init()) > dts_sensor_create_nodes(sensor_node); > > + /* > + * OCC takes few secs to boot on FSP systems. Call > + * this as late as as possible to avoid delay. > + */ > + if (!platform.bmc) > + occ_pstates_init(); So you will end up calling occ_pstates_init() on simulators as well. Do you really want that? Today its occ_pstates, tomorrow we may have something else. IMO we should continue to have all those things inside platform.exit() function and move platform.exit function just before creating dtb. > } else { > /* fdt will be rebuilt */ > free(fdt); > diff --git a/platforms/ibm-fsp/common.c b/platforms/ibm-fsp/common.c > index 7f7a1f2..6da25d1 100644 > --- a/platforms/ibm-fsp/common.c > +++ b/platforms/ibm-fsp/common.c > @@ -186,13 +186,6 @@ void ibm_fsp_exit(void) > /* Wait for FW VPD data read to complete */ > fsp_code_update_wait_vpd(true); > > - /* > - * OCC takes few secs to boot. Call this as late as > - * as possible to avoid delay. > - */ > - if (fsp_present()) > - occ_pstates_init(); Given that its inside fsp exit() function, fsp_present() check is redundant. We should cleanup that. -Vasant
Hi Vasant, On Thu, Jul 11, 2019 at 10:40:58AM +0530, Vasant Hegde wrote: > On 07/10/2019 05:59 PM, Gautham R. Shenoy wrote: > >From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > >On FSP based systems (particularly POWER8), we perform > >occ_pstates_init() late in the boot to allow OCC to be loaded. Hence > >this was being performed in platform.exit(). occ_pstates_init() would > >add pstate information into the device-tree. > > > >A recent commit 9fc0c1287ada ("Move FSP specific op-panel calls to > >platform.exit()") moved the invocation of platform.exit() after the > >creation of device-tree blob. As a result, on FSP based systems, we > >don't have the pstate information in the device-tree, and thus the > >Kernel is unable to perform frequency scaling. > > > >Fix this by moving occ_pstates_init() out of ibm_fsp_exit() and call > >it before the creation of the device-tree blob. > > > >Fixes: commit 9fc0c1287ada ("Move FSP specific op-panel calls to > >platform.exit()") > > > >Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > > Thanks for the fix. > > >--- > > core/init.c | 6 ++++++ > > platforms/ibm-fsp/common.c | 7 ------- > > 2 files changed, 6 insertions(+), 7 deletions(-) > > > >diff --git a/core/init.c b/core/init.c > >index d0f28f2..8eb0729 100644 > >--- a/core/init.c > >+++ b/core/init.c > >@@ -557,6 +557,12 @@ void __noreturn load_and_boot_kernel(bool is_reboot) > > if (!occ_sensors_init()) > > dts_sensor_create_nodes(sensor_node); > > > >+ /* > >+ * OCC takes few secs to boot on FSP systems. Call > >+ * this as late as as possible to avoid delay. > >+ */ > >+ if (!platform.bmc) > >+ occ_pstates_init(); > > So you will end up calling occ_pstates_init() on simulators as well. Do you > really want that? Perhaps not. You are right, I hadn't factored in the simulators. We should do this only for FSP based systems. > Today its occ_pstates, tomorrow we may have something else. > IMO we should continue to have all those things inside platform.exit() > function and move > platform.exit function just before creating dtb. Fine by me. However, in commit 9fc0c1287ada the platform.exit() was moved from the point right before creation of the dtb to a later stage to incorporate the op-panel calls into the platform.exit(). Will it cause any problems if we move the platform.exit() up in the code ? > > > > } else { > > /* fdt will be rebuilt */ > > free(fdt); > >diff --git a/platforms/ibm-fsp/common.c b/platforms/ibm-fsp/common.c > >index 7f7a1f2..6da25d1 100644 > >--- a/platforms/ibm-fsp/common.c > >+++ b/platforms/ibm-fsp/common.c > >@@ -186,13 +186,6 @@ void ibm_fsp_exit(void) > > /* Wait for FW VPD data read to complete */ > > fsp_code_update_wait_vpd(true); > > > >- /* > >- * OCC takes few secs to boot. Call this as late as > >- * as possible to avoid delay. > >- */ > >- if (fsp_present()) > >- occ_pstates_init(); > > Given that its inside fsp exit() function, fsp_present() check is redundant. > We should cleanup that. Yeah, will clean that up. > > -Vasant
On 07/11/2019 10:40 AM, Vasant Hegde wrote: > On 07/10/2019 05:59 PM, Gautham R. Shenoy wrote: >> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> >> >> On FSP based systems (particularly POWER8), we perform >> occ_pstates_init() late in the boot to allow OCC to be loaded. Hence >> this was being performed in platform.exit(). occ_pstates_init() would >> add pstate information into the device-tree. >> >> A recent commit 9fc0c1287ada ("Move FSP specific op-panel calls to >> platform.exit()") moved the invocation of platform.exit() after the >> creation of device-tree blob. As a result, on FSP based systems, we >> don't have the pstate information in the device-tree, and thus the >> Kernel is unable to perform frequency scaling. >> >> Fix this by moving occ_pstates_init() out of ibm_fsp_exit() and call >> it before the creation of the device-tree blob. >> >> Fixes: commit 9fc0c1287ada ("Move FSP specific op-panel calls to >> platform.exit()") >> >> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > > Thanks for the fix. > >> --- >> core/init.c | 6 ++++++ >> platforms/ibm-fsp/common.c | 7 ------- >> 2 files changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/core/init.c b/core/init.c >> index d0f28f2..8eb0729 100644 >> --- a/core/init.c >> +++ b/core/init.c >> @@ -557,6 +557,12 @@ void __noreturn load_and_boot_kernel(bool is_reboot) >> if (!occ_sensors_init()) >> dts_sensor_create_nodes(sensor_node); >> >> + /* >> + * OCC takes few secs to boot on FSP systems. Call >> + * this as late as as possible to avoid delay. >> + */ >> + if (!platform.bmc) >> + occ_pstates_init(); > > So you will end up calling occ_pstates_init() on simulators as well. Do you > really want that? > Today its occ_pstates, tomorrow we may have something else. > IMO we should continue to have all those things inside platform.exit() function > and move > platform.exit function just before creating dtb. Does this work? And we should put fat comment explaining why we should have platform.exit before create_dtb(). diff --git a/core/init.c b/core/init.c index fb55d4a9d..b8c2c05b9 100644 --- a/core/init.c +++ b/core/init.c @@ -580,6 +580,9 @@ void __noreturn load_and_boot_kernel(bool is_reboot) add_fast_reboot_dt_entries(); + if (platform.exit) + platform.exit(); + /* Create the device tree blob to boot OS. */ fdt = create_dtb(dt_root, false); if (!fdt) { @@ -603,9 +606,6 @@ void __noreturn load_and_boot_kernel(bool is_reboot) assert(0); } - if (platform.exit) - platform.exit(); - /* Take processors out of nap */ cpu_set_sreset_enable(false); cpu_set_ipi_enable(false); -Vasant
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2019-07-11 10:54:58]: > Hi Vasant, > > > On Thu, Jul 11, 2019 at 10:40:58AM +0530, Vasant Hegde wrote: > > On 07/10/2019 05:59 PM, Gautham R. Shenoy wrote: > > >From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > > > > >On FSP based systems (particularly POWER8), we perform > > >occ_pstates_init() late in the boot to allow OCC to be loaded. Hence > > >this was being performed in platform.exit(). occ_pstates_init() would > > >add pstate information into the device-tree. > > > > > >A recent commit 9fc0c1287ada ("Move FSP specific op-panel calls to > > >platform.exit()") moved the invocation of platform.exit() after the > > >creation of device-tree blob. As a result, on FSP based systems, we > > >don't have the pstate information in the device-tree, and thus the > > >Kernel is unable to perform frequency scaling. > > > > > >Fix this by moving occ_pstates_init() out of ibm_fsp_exit() and call > > >it before the creation of the device-tree blob. > > > > > >Fixes: commit 9fc0c1287ada ("Move FSP specific op-panel calls to > > >platform.exit()") > > > > > >Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > > > > Thanks for the fix. > > > > >--- > > > core/init.c | 6 ++++++ > > > platforms/ibm-fsp/common.c | 7 ------- > > > 2 files changed, 6 insertions(+), 7 deletions(-) > > > > > >diff --git a/core/init.c b/core/init.c > > >index d0f28f2..8eb0729 100644 > > >--- a/core/init.c > > >+++ b/core/init.c > > >@@ -557,6 +557,12 @@ void __noreturn load_and_boot_kernel(bool is_reboot) > > > if (!occ_sensors_init()) > > > dts_sensor_create_nodes(sensor_node); > > > > > >+ /* > > >+ * OCC takes few secs to boot on FSP systems. Call > > >+ * this as late as as possible to avoid delay. > > >+ */ > > >+ if (!platform.bmc) > > >+ occ_pstates_init(); > > > > So you will end up calling occ_pstates_init() on simulators as well. Do you > > really want that? > > Perhaps not. You are right, I hadn't factored in the simulators. We > should do this only for FSP based systems. Doing this call only on FSP based system will be the right check. However, we will return if chip->homer is not present, so functionally we can call occ_pstates_init() on a simulator and gracefully error out. But it certainly adds unnecessary cycles. Maybe we need platform callback before platform.exit where these can fit in before DTB is rolled-up and passed to kernel. Like a platform dtb_exit() as a last call to pass any info to host kernel through DT. While platform.exit means we are done with OPAL and passing control to kernel now. --Vaidy
diff --git a/core/init.c b/core/init.c index d0f28f2..8eb0729 100644 --- a/core/init.c +++ b/core/init.c @@ -557,6 +557,12 @@ void __noreturn load_and_boot_kernel(bool is_reboot) if (!occ_sensors_init()) dts_sensor_create_nodes(sensor_node); + /* + * OCC takes few secs to boot on FSP systems. Call + * this as late as as possible to avoid delay. + */ + if (!platform.bmc) + occ_pstates_init(); } else { /* fdt will be rebuilt */ free(fdt); diff --git a/platforms/ibm-fsp/common.c b/platforms/ibm-fsp/common.c index 7f7a1f2..6da25d1 100644 --- a/platforms/ibm-fsp/common.c +++ b/platforms/ibm-fsp/common.c @@ -186,13 +186,6 @@ void ibm_fsp_exit(void) /* Wait for FW VPD data read to complete */ fsp_code_update_wait_vpd(true); - /* - * OCC takes few secs to boot. Call this as late as - * as possible to avoid delay. - */ - if (fsp_present()) - occ_pstates_init(); - fsp_console_select_stdout(); op_panel_disable_src_echo();