Re: [External] Re: [PATCH v2 1/3] riscv: obtain ACPI RSDP from FFI.

[运辉崔]

Hi Conor,

On Mon, Jul 3, 2023 at 8:20 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 03, 2023 at 06:16:07PM +0800, 运辉崔 wrote:
> > Hi Conor,
>
> > This needs to be confirmed with you:
>
> > Continue to follow the current code structure, patch 1/3 is placed in
> > arch/riscv/, and 2/3 is placed under driver/firmware?
>
> You do want the SMBIOS stuff to be cross architecture, right?
> If so, keeping the code as-is seems to make the most sense to me.

Okay, other arches may use FFI in the future. Keep the code as-is seems.

> > How about changing the commit log to the following?
> >
> > riscv: obtain ACPI RSDP from devicetree.
> >
> > On RISC-V, when using Coreboot to start, since Coreboot only supports
> > DTS but not EFI, and
> > RISC-V does not have a reserved address segment.
> > When the system enables ACPI, ACPI RSDP needs to be passed through DTS
>
> I would probably write something like:
>         On RISC-V, Coreboot does not support booting using EFI, only devicetree
>         nor does RISC-V have a reserved address segment.
>         To allow using Coreboot on platforms that require ACPI, the ACPI RSDP
>         needs to be passed to supervisor mode software using devicetree.
>         Add support for parsing the "configtbls" devicetree node to find the
>         ACPI entry point and use wire up acpi_arch_get_root_pointer().
>         This feature is known as FDT Firmware Interface (FFI).

Great,  I have to learn from it.

> > > > > > +extern u64 acpi_rsdp;
> > > > >
> > > > > /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long')
> > > > >
> > > > > Fails to build when Kexec is enabled.
> > > >
> > > > Rename my acpi_rsdp to arch_acpi_rsdp? WDYT?
> > >
> > > You could do s/arch/riscv/ either, that'd match what we prefix a lot of
> > > stuff with.
> >
> >  Sorry, I don't quite understand what you mean. Could you tell me in detail?
>
> What I meant is that variables & functions in /arch/riscv are often
> prefixed with riscv_. I was saying that you could change "arch_acpi_rsdp"
> to "riscv_acpi_rsdp".

Oh, that's what it means, okay, I'll update it on v3.

>
> Thanks,
> Conor.

Thanks,
Yunhui