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 89575740035 for ; Mon, 13 Nov 2023 10:45:31 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Rgvs+I5ZnaImMMLNaGbXUaggTArsNYz2CMCFpHkW3pg=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version: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-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699872330; v=1; b=aYyM654ZZebY8d0lPuGLINZPx0qfuzB649FD/1KAGtk2ihmxYs2/JUP8eYo3MyiQzvejaUFE KWaaAEjve0YXgYvTi01lfKYv6ajesW63IurSZJfUQd4l3qso+JSaWx1aueqFktGFdVT99xhnKvw 682iJ1hmYtWo+jlC4EKjItZA= X-Received: by 127.0.0.2 with SMTP id RNbpYY7687511xWZjHbKqLxG; Mon, 13 Nov 2023 02:45:30 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.34227.1699872329477077215 for ; Mon, 13 Nov 2023 02:45:29 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-342-JHKACqIyO-OExgzG2L9nzA-1; Mon, 13 Nov 2023 05:45:22 -0500 X-MC-Unique: JHKACqIyO-OExgzG2L9nzA-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C7DF2881E21; Mon, 13 Nov 2023 10:45:21 +0000 (UTC) X-Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 84822370; Mon, 13 Nov 2023 10:45:18 +0000 (UTC) Message-ID: <4eb38d63-c5e6-0dd3-1649-17ae9c0cc379@redhat.com> Date: Mon, 13 Nov 2023 11:45:17 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 20/30] OvmfPkg/LoongArchVirt: Add serial port library To: Chao Li , 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> <177ad5cc-f256-49f3-96ea-c3bf2ab60389@loongson.cn> From: "Laszlo Ersek" In-Reply-To: <177ad5cc-f256-49f3-96ea-c3bf2ab60389@loongson.cn> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: cbOSW2i1CVaRUNZI21ifDoWMx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=aYyM654Z; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=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 On 11/10/23 05:51, Chao Li wrote: > 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? Yes, I think that's consistent with earlier code movements. Moving code from ArmVirtPkg to OvmfPkg for improving (or enabling) code reuse is valid, because ArmVirtPkg can continue consuming the same code from OvmfPkg. > > 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. Sure, that sometimes happens when a new feature takes long (either to implement, or to review). Laszlo > > > > 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 (#111136): https://edk2.groups.io/g/devel/message/111136 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] -=-=-=-=-=-=-=-=-=-=-=-