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

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

On 2/26/24 16:13, Albecki, Mateusz wrote:
> 1. Using SerialDxe instead of PciSioSerialDxe - from our perspective
> the main idea is to avoid maintaining our own implementation of
> functions that actually communicate with UART controller. To use
> SerialDxe we would have to still maintain our own SerialPortLib that
> actually goes and sends data over UART. Also a note on LPSS UART - in
> terms of registers it is just a standard UART controller, the only
> quirk here is how to access it hence the custom PCI_IO_PROTOCOL
> implementation.

Is it possible to factor out a common base library class (with just one
instance) for accessing an LPSS UART? I guess the PCI B/D/F numbers
would have to be passed to the individual functions.

Then that library could be used in PciSioSerialDxe, and also in a (thin
wrapper) SerialPortLib instance.

>
> 2. Using ConnectController before BDS - I noticed that the section I
> quoted says that BDS will use ConnectController however it doesn't say
> that it can't be used outside of that context. I did search the UEFI
> spec to see whether it provides additional restrictions and the only
> section that elaborates on this is the following:
> /"Under the UEFI Driver Model , the act of connecting and
> disconnecting drivers from controllers in a platform is under the
> platform firmware’s control. This will typically be implemented as
> part of the UEFI Boot Manager, but other implementations are possible.
> " - from  Section 2.5.6 Platform components/ It seems to be rather
> permissive when it comes to who and when can call it.
>
> 3. How to make sure dispatch is early enough and not too late - this
> will depend on the overall platform implementation. For our part - we
> simply put it into flash as early as we can get away with. Even
> apriori section isn't strictly necessary if the platform is
> comfortable with relying on the fact that DXE core in MdeModulePkg
> dispatches modules in order of their placement in flash(that's not
> architectural as far as I know). Other platforms might choose to
> introduce explicit depex on gEfiSerialIoProtocolGuid. To reiterate
> this is the implementation that works for us in a sense that we get
> logs from all modules that change frequently from generation to
> generation, I understand that the same might not be true for platforms
> other than Intel however I think majority of platforms could still
> make at least some use of early UART debugging.
>
> 4. When exactly do we connect LPSS UART and start logging - we try to
> be as early as possible for this interface. We miss all of DXE_CORE
> logs(obviously), Pcd.inf and a couple of modules that implement some
> of the architectural protocols(from PciSio perspective metronome is
> the only actually required as far as we can tell since stall has to
> work).
>
> I also want to note that I get why this is a controversial change. I
> didn't realize it earlier but it would be the first DXE_DRIVER in EDK2
> tree that implements driver binding and in general it is strange to
> have PCI device driver that could potentially dispatch before PCI bus
> driver(however it is worth noting here that PciSioSerialDxe is not
> just a PCI driver, it is a combo driver capable of supporting PCI and
> SIO). That said I still think EDK2 in general needs a way to support
> early device drivers and using DXE_DRIVER seems like the least
> invasive idea. We need early drivers not just for debug(although that
> is one of the most important use cases I would say) other important
> use case is platform management through SMBUS/I2C/other serial
> interface which might be required to even be able to enumerate full
> PCI hierarchy(for instance some of the PCI slots might be powered down
> and you need to power them on sending commands over I2C) or maybe
> flash access to EFI variable storage(nothing says that you can't have
> it connected to PCI SPI controller) or maybe GPIO control to do any
> number of platform specific things.

To my eyes, this series saves a bit of refactoring now and the
maintenance of a thin wrapper library instance later, in exchange for a
very unconventional solution in MdeModulePkg that is not easily reusable
by other platforms. I don't like that trade-off; it makes me
uncomfortable.

If Liming and Mike accept this solution, I won't try to block it. In
that case however, please document the design (all we've discussed thus
far) in the "PciSioSerialDxeEarly.inf" file, in a (long) series of
comments.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115971): https://edk2.groups.io/g/devel/message/115971
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]
-=-=-=-=-=-=-=-=-=-=-=-