On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote:
> Initialize the ACPI core for RISC-V during boot.
>
> ACPI tables and interpreter are initialized based on
> the information passed from the firmware and the value of
> the kernel parameter 'acpi'.
>
> With ACPI support added for RISC-V, the kernel parameter 'acpi'
> is also supported on RISC-V. Hence, update the documentation.
>
> Signed-off-by: Sunil V L <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Andrew Jones <[email protected]>
> Acked-by: Conor Dooley <[email protected]>
> + /* Parse the ACPI tables for possible boot-time configuration */
> + acpi_boot_table_init();
> + if (acpi_disabled) {
> + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) {
> + unflatten_and_copy_device_tree();
> + } else {
> + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> + unflatten_device_tree();
> + else
> + pr_err("No DTB found in kernel mappings\n");
> + }
> + } else {
> + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)));
I'm probably forgetting something, but this seems very non-obvious to
me:
Why are you running early_init_dt_verify() when ACPI is enabled?
I think that one deserves a comment so that next time someone looks at
this (that doesn't live in ACPI land) they've know exactly why this is
like it is.
Doubly so since this is likely to change with some of Alex's bits moving
the dtb back into the fixmap.
Cheers,
Conor.
On Tue, Apr 04, 2023 at 10:38:56PM +0100, Conor Dooley wrote:
> On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote:
> > Initialize the ACPI core for RISC-V during boot.
> >
> > ACPI tables and interpreter are initialized based on
> > the information passed from the firmware and the value of
> > the kernel parameter 'acpi'.
> >
> > With ACPI support added for RISC-V, the kernel parameter 'acpi'
> > is also supported on RISC-V. Hence, update the documentation.
> >
> > Signed-off-by: Sunil V L <[email protected]>
> > Acked-by: Rafael J. Wysocki <[email protected]>
> > Reviewed-by: Andrew Jones <[email protected]>
> > Acked-by: Conor Dooley <[email protected]>
>
> > + /* Parse the ACPI tables for possible boot-time configuration */
> > + acpi_boot_table_init();
> > + if (acpi_disabled) {
> > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) {
> > + unflatten_and_copy_device_tree();
> > + } else {
> > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> > + unflatten_device_tree();
> > + else
> > + pr_err("No DTB found in kernel mappings\n");
> > + }
> > + } else {
> > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)));
>
> I'm probably forgetting something, but this seems very non-obvious to
> me:
> Why are you running early_init_dt_verify() when ACPI is enabled?
> I think that one deserves a comment so that next time someone looks at
> this (that doesn't live in ACPI land) they've know exactly why this is
> like it is.
>
> Doubly so since this is likely to change with some of Alex's bits moving
> the dtb back into the fixmap.
>
Good question. The kernel creates a tiny DTB even when the FW didn't
pass the FDT (ACPI systems). Please see update_fdt(). So, parse_dtb()
would have set initial_boot_params to early VA and if we don't call
early_init_dt_verify() again with __va, it panics since
initial_boot_params can not be translated.
Thanks,
Sunil
On Wed, Apr 05, 2023 at 08:41:54PM +0530, Sunil V L wrote:
> On Tue, Apr 04, 2023 at 10:38:56PM +0100, Conor Dooley wrote:
> > On Tue, Apr 04, 2023 at 11:50:33PM +0530, Sunil V L wrote:
> > > Initialize the ACPI core for RISC-V during boot.
> > >
> > > ACPI tables and interpreter are initialized based on
> > > the information passed from the firmware and the value of
> > > the kernel parameter 'acpi'.
> > >
> > > With ACPI support added for RISC-V, the kernel parameter 'acpi'
> > > is also supported on RISC-V. Hence, update the documentation.
> > >
> > > Signed-off-by: Sunil V L <[email protected]>
> > > Acked-by: Rafael J. Wysocki <[email protected]>
> > > Reviewed-by: Andrew Jones <[email protected]>
> > > Acked-by: Conor Dooley <[email protected]>
> >
> > > + /* Parse the ACPI tables for possible boot-time configuration */
> > > + acpi_boot_table_init();
> > > + if (acpi_disabled) {
> > > + if (IS_ENABLED(CONFIG_BUILTIN_DTB)) {
> > > + unflatten_and_copy_device_tree();
> > > + } else {
> > > + if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> > > + unflatten_device_tree();
> > > + else
> > > + pr_err("No DTB found in kernel mappings\n");
> > > + }
> > > + } else {
> > > + early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)));
> >
> > I'm probably forgetting something, but this seems very non-obvious to
> > me:
> > Why are you running early_init_dt_verify() when ACPI is enabled?
> > I think that one deserves a comment so that next time someone looks at
> > this (that doesn't live in ACPI land) they've know exactly why this is
> > like it is.
> >
> > Doubly so since this is likely to change with some of Alex's bits moving
> > the dtb back into the fixmap.
> >
> Good question. The kernel creates a tiny DTB even when the FW didn't
> pass the FDT (ACPI systems). Please see update_fdt().
Can you add a comment about this either on-location or in the commit
message please?
I think this counts as non-obvious behaviour. At least to me it does!
Cheers,
Conor.