Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier in DXE

["Laszlo Ersek" <lersek@redhat.com>]

On 2/27/24 18:15, Albecki, Mateusz wrote:
> Is the idea to refactor PciSioSerialDxe to extract functions that access
> the HW and wrap it in the SerialPortLib instance? That solution would
> still save us some maintenance cost. However exploring the idea further
> I see following problems:
> 
> 1. Relying on driver binding produces a fairly nice flow: platform
> driver initializes enough platform HW for UART to work -> platform
> driver calls ConnectController -> driver initializes UART. With
> SerialDxe a depex would have to be injected through our instance of
> SerialPortLib to stop the SerialDxe dispatch until platform driver made
> the platform ready.
> 2. I am wondering how it would work in case we want to allow dynamic
> configuration of debug port(basically selecting which UART controller
> would be used). With current solution we can select which one(or which
> ones) will be used by simply installing and connecting corresponding
> handles. With library solution I guess library would have to locate some
> protocols(possibly the same PCI_IO and DEVICE_PATH) that were installed
> by platform driver. It would also need to install notify on those
> protocols installation in case platform wants to add more uarts later on
> in the boot flow.

I think these requirements are *way* out of scope, and too clever, for
producing debug output. I'm now tempted to think that you are actually
best served by your existent separate driver.

SerialDxe and SerialPortLib are ill-suited if your system has multiple
serial ports, not to mention if you want different configurations from
boot to boot.

I think I'm permitted to disagree with a proposed patch without having
to counter-propose an alternative myself (i.e., the burden is on you, to
find an alternative). But, I'll brainstorm one more:

a. I'll assume that you store the debug port config in some non-volatile
UEFI variable.

b. In the PEI phase, have a PEIM turn that variable into a GUID HOB.
(This is doable because PEI can have read-only variable access.) Or
produce that HOB in some other way.

c. In the early platform DXE driver (= debug driver), read the HOB, and
publish a single instance of a new gEdkiiLpssUartDebugProtocolGuid.

d. The protocol should have a single member function,
WriteToAllConfiguredDebugPorts(). The debug driver should replicate the
message that is passed to this API to all ports enabled in the HOB.

e. Introduce a new DebugLib instance that, for DXE_DRIVER modules, has a
DEPEX on gEdkiiLpssUartDebugProtocolGuid:

[Depex.common.DXE_DRIVER]
  gEdkiiLpssUartDebugProtocolGuid

f. The debug ports should never appear in the system as EFI handles
(i.e., neither SerialIo nor PciIo nor DevicePath protocols -- no handles
at all).

g. The debug driver should use PciLib for configuring and accessing the
ports. Configuration includes any necessary "platform configuration" too.

h. If there is any LPSS UART access logic that's worth sharing -- for
maintenance reasons -- between PciSioSerialDxe and the debug driver,
then that should be extracted to new library class, and *two* library
instances (same directory, though). Both instances would nearly be
identical, but they'd have to *internally* abstract away PCI access. The
DXE instance (underlying the debug driver) would use PciLib, the UEFI
instance (underlying PciSioSerialDxe) would use PciIo. The code that
interacted with the LPSS UARTs would be common though.

i. The debug driver could register protocol notifies for the HII
protocols (which would become available much later), and once those
appeared, the debug driver could install HII forms, for managing the
non-volatile UEFI variable that configures the debug ports. (This could
perhaps be used for truly dynamic configuration, i.e., without having to
reboot; although personally I'd steer very clear of that avenue.)


As lower-hanging fruit, you could probably just implement step (h) in
isolation, and rebase both your existent driver, and PciSioSerialDxe,
onto that new library class (and its two library instances).

Other opinions are very welcome, of course! Personally, I'm out of
cycles on this topic.

Laszlo

> 3. We still end up with 2 UART drivers in flash since PciSioSerialDxe is
> needed for PCI UARTs.
> 
> I also think this solution is comparable in effort to refactoring the
> PciSioSerialDxe so that it doesn't use driver binding when used as a DXE
> driver. In this solution driver would have one .c file for code with
> driver binding and one .c file for code when everything is done in entry
> point, it would still use DEVICE_PATH and PCI_IO/SIO. While I still
> think using the driver as is in DXE is best for us, in case that
> solution gets blocked I would like to understand if everyone would be ok
> with such refactoring.
> 
> Thanks,
> Mateusz
> 



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