Re: [edk2-devel] [edk2-platforms] [PATCH 0/5] Enable Ps2 keyboard

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

Hi Prince,

There's some high-level changes I suggest to the patch series:
1. This patch series mixes changes across packages in single patches
   which should be avoided.

2. Some of the patches include unrelated changes that would make git
   revert difficult.
   2.a. Example: In patch #1, one change is to add
        gBoardModulePkgTokenSpaceGuid. This is a pre-requisite for
        adding the PCDs to BoardModulePkg.dec but still an isolated
        change. If someone were to add more commits in the future
        that rely upon gBoardModulePkgTokenSpaceGuid and then wanted
        to revert the PS/2 keyboard change it would be difficult due to
        the coupling.
   2.b. Example: In patch #3, a change is made that affects all
        driver consumers in BoardModulePkg/LegacySioDxe but the commit
        includes changes for enabling the driver in SimicsOpenBoardPkg.
        The change to enable PS/2 keyboard/mouse in SimicsOpenBoardPkg
        could not easily be reverted without affecting the overall driver.

3. The order of changes leaves some earlier commits incomplete.
   3.a. For example, patch #1 exposes a PCD interface in BoardModulePkg
        that is effectively not used until later patches so using the
        PCDs at that point in the commit log would be misleading.

As far as I can tell, there's a high-level order to do the following:
1. Define gBoardModulePkgTokenSpaceGuid in BoardModulePkg.dec
2. Remove the LegacySioDxe driver from SimicsOpenBoardPkg
3. Add the LegacySioDxe driver to BoardModulePkg
4. Add new PCDs to BoardModulePkg for usage with LegacySioDxe
5. Update relevant boards in KabylakeOpenBoardPkg to use the LegacySioDxe
   driver and PCDs from BoardModulePkg, remove the now redundant PCD
   gKabylakeOpenBoardPkgTokenSpaceGuid.PcdPs2KbMsEnable in
   KabylakeOpenBoardPkg/OpenBoardPkg.dec, configure the relevant PCDs
   in BoardModulePkg in the board DSC files to properly use the
   LegacySioDxe driver.
6. Add the Ps2KbcLib changes in KabylakeOpenBoardPkg as currently done
   in V1 patches #4 and #5.
7. Do the same as #5 for SimicsOpenBoardPkg.
8. Explicitly set gBoardModulePkgTokenSpaceGuid.PcdPs2KbMsEnable
   in WhiskeylakeOpenBoardPkg/WhiskeylakeURvp.

Thanks,
Michael
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Agyeman, Prince
> Sent: Friday, November 1, 2019 12:51 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [edk2-platforms] [PATCH 0/5] Enable Ps2 keyboard
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2228
> 
> This patch series enables BIOS Ps2 keyboard in GalagoPro3
> 
> 
> What was done:
> Patch 0001 adds PCDs to BoardModulePkg that will enable/disable, describe
> Super I/O , Ps2 keyboard/mouse, uart1 and uart2 com ports
> 
> Patch 0002 moves the generic Super I/O driver from SimicsOpenBoardPkg to
> BoardModulePkg in order for it to be shared. This driver publishes the
> gEfiSioProtocolGuid consumed by edk2's
> MdeModulePkg/Bus/Isa/Ps2KeyboardDxe
> driver to enable Ps2 keyboard functions in BIOS
> 
> Patch 0003 adds PCDs defined in patch 0001 to enable/disable devices in the
> Super I/O driver added in patch 0002
> 
> Patch 0004 adds a Null Ps2 Library that adds Ps2 keyboard device path to
> ConIn and ConInDev
> 
> Patch 0005 enables Ps2 keyboard in BIOS by setting Ps2 keyboard related
> PCDs Prince Agyeman (5):
>   Platform/Intel: Add gBoardModulePkgTokenSpaceGuid
>   Platform/Intel: Move Sio Dxe Driver
>   BoardModulePkg: Added Pcds Sio Driver
>   KabylakeOpenBoardPkg: Add Ps2 keyboard Null Library
>   KabylakeOpenBoardPkg: Add Ps2 Keyboard Support
> 
>  .../Intel/BoardModulePkg/BoardModulePkg.dec   |  25 +++
>  .../Intel/BoardModulePkg/BoardModulePkg.dsc   |   1 +
>  .../LegacySioDxe/ComponentName.c              |   0
>  .../LegacySioDxe/ComponentName.h              |   0
>  .../LegacySioDxe/LegacySioDxe.inf             |  18 +-
>  .../LegacySioDxe/Register.h                   |   0
>  .../LegacySioDxe/SioChip.c                    |  71 +++++-
>  .../LegacySioDxe/SioChip.h                    |  18 +-
>  .../LegacySioDxe/SioDriver.c                  |  42 +++-
>  .../LegacySioDxe/SioDriver.h                  |   1 -
>  .../LegacySioDxe/SioService.c                 |   0
>  .../LegacySioDxe/SioService.h                 |   0
>  .../BoardAcpiLib/DxeBoardAcpiTableLib.inf     |   3 +-
>  .../DxeMultiBoardAcpiSupportLib.inf           |   3 +-
>  .../GalagoPro3/Library/Ps2KbcLib/Ps2KbcLib.c  | 202 ++++++++++++++++++
> .../GalagoPro3/Library/Ps2KbcLib/Ps2KbcLib.h  |  65 ++++++
>  .../Library/Ps2KbcLib/Ps2KbcLib.inf           |  39 ++++
>  .../GalagoPro3/OpenBoardPkg.dsc               |   7 +
>  .../GalagoPro3/OpenBoardPkg.fdf               |   2 +
>  .../GalagoPro3/OpenBoardPkgPcd.dsc            |   6 +
>  .../BoardAcpiLib/DxeBoardAcpiTableLib.inf     |   3 +-
>  .../DxeMultiBoardAcpiSupportLib.inf           |   3 +-
>  .../KabylakeRvp3/OpenBoardPkgPcd.dsc          |   5 +
>  .../KabylakeOpenBoardPkg/OpenBoardPkg.dec     |   2 -
>  .../BoardX58Ich10/OpenBoardPkg.dsc            |   2 +-
>  .../BoardX58Ich10/OpenBoardPkg.fdf            |   2 +-
>  .../BoardX58Ich10/OpenBoardPkgPcd.dsc         |   6 +
>  .../WhiskeylakeOpenBoardPkg/OpenBoardPkg.dec  |   1 -
>  .../WhiskeylakeURvp/OpenBoardPkgPcd.dsc       |   5 +
>  29 files changed, 499 insertions(+), 33 deletions(-)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/ComponentName.c (100%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/ComponentName.h (100%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/LegacySioDxe.inf (63%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/Register.h (100%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/SioChip.c (75%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/SioChip.h (90%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/SioDriver.c (88%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/SioDriver.h (95%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/SioService.c (100%)  rename
> Platform/Intel/{SimicsOpenBoardPkg =>
> BoardModulePkg}/LegacySioDxe/SioService.h (100%)  create mode 100644
> Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/Ps2KbcLib/Ps2K
> bcLib.c
>  create mode 100644
> Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/Ps2KbcLib/Ps2K
> bcLib.h
>  create mode 100644
> Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/Ps2KbcLib/Ps2K
> bcLib.inf
> 
> --
> 2.19.1.windows.1
> 
> 
>