public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, lichao@loongson.cn, kraxel@redhat.com
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Jiewen Yao <jiewen.yao@intel.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Xianglai Li <lixianglai@loongson.cn>
Subject: Re: [edk2-devel] [PATCH v2 20/30] OvmfPkg/LoongArchVirt: Add serial port library
Date: Wed, 8 Nov 2023 23:21:21 +0100	[thread overview]
Message-ID: <95147d56-ee9b-ebf7-6e69-7e41402f4cf0@redhat.com> (raw)
In-Reply-To: <0d008dd7-2622-4d8b-9ad6-d0381d797e47@loongson.cn>

On 11/7/23 11:12, Chao Li wrote:
> Hi Gerd,
> 
> These two libraries is not only copy code, the way of obtain the serial
> base address is different from ARM, and the early serial port output
> also different from ARM, so these two libraries are LoongArch specific.

I think we're going to have to go through the design with "baby steps".

The commit message of the patch is empty.

Please don't expect reviewers to "reverse engineer" the design from the
code. That's hugely taxing. It's hard enough to review code when we
precisey know in advance, from the commit message, what the code will
try to achieve.

You are saying that the way to obtain serial base address is different
from ARM, yet the GetSerialConsolePortAddress() function looks very
similar to the function that I *replaced* in recent commits eb83b5330961
("ArmVirtPkg: introduce FdtSerialPortAddressLib", 2023-10-26) and
f078a6fdd4d7 ("ArmVirtPkg/Fdt16550SerialPortHookLib: rebase to
FdtSerialPortAddressLib", 2023-10-26).

So there are two issues with your GetSerialConsolePortAddress() function
immediately, I would say:

- you say that it's different from ARM, but it seems to parse the DTB
identically (or mostly identically -- I'm speaking from memory),

- I factored out the subject DTB parsing logic in the above-noted
commits recently, so even if your GetSerialConsolePortAddress() function
is *supposed* to parse the DTB identically to ARM, then the way to
employ that logic is different; it should be reused, not duplicated.

Now that you have a running prototype (basically evidence that the port
to this new architecture can be done), can we go through the design of
each component (library, driver etc) one by one? Dumping the whole thing
on reviewers all at once, with little documentation, is not helpful. We
basically need to start with the specification of each of your modules.
See where the existing components in edk2 are not good enough or not
flexible enough, etc.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110931): https://edk2.groups.io/g/devel/message/110931
Mute This Topic: https://groups.io/mt/102413885/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2023-11-08 22:21 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-06  3:25 [edk2-devel] [PATCH v2 00/30] Enable LoongArch virtual machine in edk2 Chao Li
2023-11-06  3:26 ` [edk2-devel] [PATCH v2 01/30] MdePkg: Add the header file named Csr.h for LoongArch64 Chao Li
2023-11-07  3:30   ` Michael D Kinney
2023-11-06  3:26 ` [edk2-devel] [PATCH v2 02/30] MdePkg: Add LoongArch64 FPU function set into BaseCpuLib Chao Li
2023-11-07  3:31   ` Michael D Kinney
2023-11-06  3:26 ` [edk2-devel] [PATCH v2 03/30] MdePkg: Add LoongArch64 exception function set into BaseLib Chao Li
2023-11-07  3:31   ` Michael D Kinney
2023-11-06  3:26 ` [edk2-devel] [PATCH v2 04/30] MdePkg: Add LoongArch64 local interrupt " Chao Li
2023-11-07  3:31   ` Michael D Kinney
2023-11-06  3:27 ` [edk2-devel] [PATCH v2 05/30] MdePkg: Add LoongArch Cpucfg function Chao Li
2023-11-07  3:31   ` Michael D Kinney
2023-11-06  3:27 ` [edk2-devel] [PATCH v2 06/30] MdePkg: Add read stable counter operation for LoongArch Chao Li
2023-11-07  3:31   ` Michael D Kinney
2023-11-06  3:27 ` [edk2-devel] [PATCH v2 07/30] MdePkg: Add CSR " Chao Li
2023-11-07  3:31   ` Michael D Kinney
2023-11-06  3:27 ` [edk2-devel] [PATCH v2 08/30] MdePkg: Add IOCSR " Chao Li
2023-11-07  3:32   ` Michael D Kinney
2023-11-06  3:28 ` [edk2-devel] [PATCH v2 09/30] UefiCpuPkg: Add LoongArch64 CPU Timer library Chao Li
2023-11-06  3:28 ` [edk2-devel] [PATCH v2 10/30] UefiCpuPkg: Add CPU exception library for LoongArch Chao Li
2023-11-06  3:28 ` [edk2-devel] [PATCH v2 11/30] UefiCpuPkg: Add CpuMmuLib.h to UefiCpuPkg Chao Li
2023-11-09  1:17   ` Andrei Warkentin
2023-11-14  2:00     ` Chao Li
2023-11-06  3:28 ` [edk2-devel] [PATCH v2 12/30] UefiCpuPkg: Add LoongArch64CpuMmuLib " Chao Li
2023-11-06  3:28 ` [edk2-devel] [PATCH v2 13/30] UefiCpuPkg: Add multiprocessor library for LoongArch64 Chao Li
2023-11-06  3:28 ` [edk2-devel] [PATCH v2 14/30] UefiCpuPkg: Add CpuDxe driver " Chao Li
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64 Chao Li
2023-11-15 18:55   ` Leif Lindholm
2023-11-16  8:15     ` Pedro Falcato
2023-11-16  9:17       ` Chao Li
2023-11-16 11:21       ` Leif Lindholm
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 16/30] ArmVirtPkg: Move PCD of FDT base address and FDT padding to OvmfPkg Chao Li
2023-11-07  9:37   ` Gerd Hoffmann
2023-11-13 11:46   ` Sami Mujawar
2023-11-14  1:41     ` Chao Li
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 17/30] OvmfPkg/LoongArchVirt: Add PciCpuIo2Dxe module Chao Li
2023-11-07  9:42   ` Gerd Hoffmann
2023-11-07 10:03     ` Chao Li
2023-11-08 23:11       ` Sunil V L
2023-11-10  3:31         ` Chao Li
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 18/30] OvmfPkg/LoongArchVirt: Add stable timer driver Chao Li
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 19/30] OvmfPkg/LoongArchVirt: Add a NULL library named CollectApResouceLibNull Chao Li
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 20/30] OvmfPkg/LoongArchVirt: Add serial port library Chao Li
2023-11-07  9:47   ` Gerd Hoffmann
2023-11-07 10:12     ` Chao Li
2023-11-08 22:21       ` Laszlo Ersek [this message]
2023-11-10  4:51         ` Chao Li
2023-11-13 10:45           ` Laszlo Ersek
2023-11-06  3:29 ` [edk2-devel] [PATCH v2 21/30] OvmfPkg/LoongArchVirt: Add real time clock library Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 22/30] OvmfPkg/LoongArchVirt: Add NorFlashQemuLib Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 23/30] OvmfPkg/LoongArchVirt: Add PeiServiceTablePointerLib Chao Li
2023-11-08 22:22   ` Laszlo Ersek
2023-11-10  6:44     ` Chao Li
2023-11-13 11:02       ` Laszlo Ersek
2023-11-14  2:08         ` Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 24/30] OvmfPkg/LoongArchVirt: Add platform boot manager library Chao Li
2023-11-08 22:24   ` Laszlo Ersek
2023-11-10  7:09     ` Chao Li
2023-11-10  9:46       ` Gerd Hoffmann
2023-11-13 11:08         ` Laszlo Ersek
2023-11-15  3:21           ` Chao Li
2023-11-15 12:52             ` Laszlo Ersek
2023-11-16  7:09               ` Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 25/30] OvmfPkg/LoongArchVirt: Add FdtQemuFwCfgLib Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 26/30] OvmfPkg/LoongArchVirt: Add reset system library Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 27/30] OvmfPkg/LoongArchVirt: Support SEC phase Chao Li
2023-11-06  3:30 ` [edk2-devel] [PATCH v2 28/30] OvmfPkg/LoongArchVirt: Support PEI phase Chao Li
2023-11-06  3:31 ` [edk2-devel] [PATCH v2 29/30] OvmfPkg/LoongArchVirt: Add build file Chao Li
2023-11-06  3:31 ` [edk2-devel] [PATCH v2 30/30] OvmfPkg/LoongArchVirt: Add self introduction file Chao Li
     [not found] ` <1794EAF40590BB23.26468@groups.io>
2023-11-07  6:41   ` [edk2-devel] [PATCH v2 09/30] UefiCpuPkg: Add LoongArch64 CPU Timer library Chao Li
     [not found] ` <1794EB01154A1CFD.12603@groups.io>
2023-11-07  8:40   ` [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64 Chao Li
     [not found] ` <1794EB0445E96DCB.26468@groups.io>
2023-11-07  8:41   ` [edk2-devel] [PATCH v2 16/30] ArmVirtPkg: Move PCD of FDT base address and FDT padding to OvmfPkg Chao Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=95147d56-ee9b-ebf7-6e69-7e41402f4cf0@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox