From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 085759410B5 for ; Fri, 10 Nov 2023 04:51:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=9CFZtX2D22iej83PzNaibtgYMgskYa9MEXux0kNWlfo=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1699591889; v=1; b=C1w/wI8XX0Js85vSIcjkZ5A5ReYyLLKTjv1Ww4dIh8GO2ADNt4XAE/ks4kVBeYwyrqpta4eb s0GCGy8YpVzrOKpiY13s8B9qTeuFEMnmhZGDOmMnWWbjv7b90uqyphgJX8zPuIMetKE7Yypo78r 5i2MhGh8IOOqAIUA+GYOpgUI= X-Received: by 127.0.0.2 with SMTP id fwKIYY7687511xiEz6TEUxf4; Thu, 09 Nov 2023 20:51:29 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.21782.1699591887886481144 for ; Thu, 09 Nov 2023 20:51:28 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8DxqOrNtk1l76E4AA--.10085S3; Fri, 10 Nov 2023 12:51:25 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8Axzy_Mtk1l_cA9AA--.4163S3; Fri, 10 Nov 2023 12:51:24 +0800 (CST) Message-ID: <177ad5cc-f256-49f3-96ea-c3bf2ab60389@loongson.cn> Date: Fri, 10 Nov 2023 12:51:24 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v2 20/30] OvmfPkg/LoongArchVirt: Add serial port library To: Laszlo Ersek , devel@edk2.groups.io, kraxel@redhat.com Cc: Ard Biesheuvel , Jiewen Yao , Jordan Justen , Xianglai Li References: <20231106032521.2251143-1-lichao@loongson.cn> <20231106032945.2285855-1-lichao@loongson.cn> <0d008dd7-2622-4d8b-9ad6-d0381d797e47@loongson.cn> <95147d56-ee9b-ebf7-6e69-7e41402f4cf0@redhat.com> From: "Chao Li" In-Reply-To: <95147d56-ee9b-ebf7-6e69-7e41402f4cf0@redhat.com> X-CM-TRANSID: AQAAf8Axzy_Mtk1l_cA9AA--.4163S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQALCGVNkykCEQABsC X-Coremail-Antispam: 1Uk129KBj93XoWxGFyUCr1fCFWrZF13Zw4UWrX_yoW5urWDpF W5Kr47Ar4DJr1Iyws2qw4rXFWYkrZavF45tryrCr1S9wn8Wr1I9rWakrs0yw4UA3ykJw10 qF4qya4kAFyjyagCm3ZEXasCq-sJn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnRJUUUyCb4IE77IF4wAF F20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r 106r15M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAF wI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1l84ACjcxK6I8E87Iv67 AKxVW8Jr0_Cr1UM28EF7xvwVC2z280aVCY1x0267AKxVW8Jr0_Cr1UM2AIxVAIcxkEcVAq 07x20xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx1lYx0E2Ix0cI8IcVAFwI0_Jr0_Jr 4lYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCj r7xvwVCIw2I0I7xG6c02F41l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr 1lx2IqxVAqx4xG67AKxVWUGVWUWwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE 14v26r126r1DMIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7 IYx2IY6xkF7I0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E 87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0x ZFpf9x07UEFAJUUUUU= Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 21N9tORkMVKOsIu3dq2iGJq9x7686176AA= Content-Type: multipart/alternative; boundary="------------cnNX89yBWcgGs7gXM3mzCMHd" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="C1w/wI8X"; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------cnNX89yBWcgGs7gXM3mzCMHd Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Laszlo, Sorry, it is my fault, I should separate this this patch for two standalone patches, and with some detailed commit message. I will fix this in V3. Currently, there are two libraries are not independent, so I guess you are talking about Fdt16550SerialPortHookLib. I would say that the different is not in GetSerialConsolePortAddress, the logic about this function is same to ARM, the different is in PlatformHookSerialPortInitialize. Since some platform do not require running code on memory during the PEI phase, so the ARM version of PlatformHookSerialPortInitialize will use PcdSet to set a PCD value, if the memory is not ready, this funciton can not be used. So this library is override from ArmVirtPkg to LongArchVirt. BTW, the new library FdtSerialProtAddressLib which you committed a few days ago, I will look into it and try it. Roughly speaking, it is look like the code for obtaining the serial port address is sparated from the HookLIb. I have a question, can I move this library to OvmfPkg so that other ARCH can use it easily? Regarding FdtSerialPortAddressLib, I would like to say that it was not committed when I porting LoongArchVirt,  my code base is based on stable202308, so I don't know you committed a new library, sorry. Thanks, Chao 在 2023/11/9 06:21, Laszlo Ersek 写道: > 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 (#111010): https://edk2.groups.io/g/devel/message/111010 Mute This Topic: https://groups.io/mt/102413885/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- --------------cnNX89yBWcgGs7gXM3mzCMHd Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi Laszlo,

Sorry, it is my fault, I should separate this this patch for two standalone patches, and with some detailed commit message. I will fix this in V3.

Currently, there are two libraries are not independent, so I guess you are talking about Fdt16550SerialPortHookLib. I would say that the different is not in GetSerialConsolePortAddress, the logic about this function is same to ARM, the different is in PlatformHookSerialPortInitialize. Since some platform do not require running code on memory during the PEI phase, so the ARM version of PlatformHookSerialPortInitialize will use PcdSet to set a PCD value, if the memory is not ready, this funciton can not be used. So this library is override from ArmVirtPkg to LongArchVirt. 

BTW, the new library FdtSerialProtAddressLib which you committed a few days ago, I will look into it and try it. Roughly speaking, it is look like the code for obtaining the serial port address is sparated from the HookLIb. I have a question, can I move this library to OvmfPkg so that other ARCH can use it easily?

Regarding FdtSerialPortAddressLib, I would like to say that it was not committed when I porting LoongArchVirt,  my code base is based on stable202308, so I don't know you committed a new library, sorry.



Thanks,
Chao
在 2023/11/9 06:21, Laszlo Ersek 写道:
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 (#111010) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------cnNX89yBWcgGs7gXM3mzCMHd--