Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
Mike,
Please check the updated PciSioSerialDxe driver and verify whether it can
replace the Quark version UART driver. Several changes were made:
1. Change the PCD structure layout
2. Change the SerialPortReadRegister function name to avoid linking error when
using DebugLibSerialPort
3. Support up to 921600 baud rate, and use the actual baud rate (same algorithm
as Quark version UART driver)
4. Flush Tx FIFO when reset or change the configuration of UART
Regards,
Ray
-Original Message-
From: Kinney, Michael D
Sent: Friday, December 4, 2015 9:00 AM
To: Tian, Feng <feng.t...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Kinney,
Michael D <michael.d.kin...@intel.com>
Cc: edk2-devel@lists.01.org
Subject: RE: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
Ray,
I tried to build this module into a platform, and I am getting function name
collisions with BaseSerialPortLib16550 for the symbols:
SerialPortReadRegister
SerialPortWriteRegister
I recommend you change the names of these 2 functions.
Mike
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, December 3, 2015 4:00 PM
> To: Tian, Feng <feng.t...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
>
> Hi Ray,
>
> I agree a UINT64 Offset would be good for the Offset field. It would also be
> good to order the fields and pad the structure so they are
> naturally aligned. Maybe like the following with some comment changes:
>
> typedef struct {
> UINT16 VendorId; ///< Vendor ID to match the PCI device. The value
> 0x terminates the list of entries.
> UINT16 DeviceId; ///< Device ID to match the PCI device
> UINT32 ClockRate; ///< UART clock rate. Set to 0 for default clock
> rate of 1843200 Hz
> UINT64 Offset; ///< The byte offset into to the BAR
> UINT8 BarIndex; ///< Which BAR to get the UART base address
> UINT8 RegisterStride; ///< UART register stride in bytes. Set to 0 for
> default register stride of 1 byte.
> UINT8 Reserved[6];
> } PCI_SERIAL_PARAMETER;
>
> You need to add this module to the MdeModulePkg DSC file.
>
> The code is currently limited to 115200 baud. There are many UARTs that
> support higher baud rates, so this restriction needs to be
> removed.
>
> If a non-standard ClockRate is used, then the actual baud rate may not
> exactly match the expected baud rate, but as long as we are
> within a few percent, the UART is stable.
>
> I would like to use this driver as a replacement for the UART driver for
> Quark that I have posted on GitHub at:
>
>
> https://github.com/mdkinney/edk2/tree/Quark/QuarkSocPkg/QuarkSouthCluster/Uart/Dxe
>
> It has support for baud rate up to 921600 and closest baud rate match with a
> few percent and support for larger Tx FIFOs and extra
> flushes of the Tx FIFO when doing a Reset or a UART config change. Please
> let me know if you want me to help merge in some of
> those features to your patch.
>
> Thanks,
>
> Mike
>
> > -Original Message-
> > From: Tian, Feng
> > Sent: Wednesday, December 2, 2015 9:13 PM
> > To: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>
> > Cc: edk2-devel@lists.01.org; Tian, Feng <feng.t...@intel.com>
> > Subject: RE: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
> >
> > Suggest to use UINT64 for Offset field to be consistent with PciIo protocol
> > definition.
> >
> > Others look good to me
> >
> > Reviewed-by: Feng Tian <feng.t...@intel.com>
> >
> > Thanks
> > Feng
> >
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni,
> > Ruiyu
> > Sent: Monday, November 30, 2015 10:12
> > To: Kinney, Michael D
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
> >
> > Mike,
> > Could you please review whether the PCD structure defined as below is good?
> >
> > +#pragma pack(1)
> > +///
> > +/// PcdPciSerialParameters contains zero or more instances of the below
> > structure.
> > +/// If a PCI device contains multiple UARTs, PcdPciSerialParameters needs
> > to contain
> > +/// two instances of the below structure, with the VendorId and DeviceId
> > equals to the
> > +/// device ID and vendor ID of the device. If the PCI device uses the
> > first two BARs
> > +/// to support m
Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
Hi Ray,
I agree a UINT64 Offset would be good for the Offset field. It would also be
good to order the fields and pad the structure so they are naturally aligned.
Maybe like the following with some comment changes:
typedef struct {
UINT16 VendorId; ///< Vendor ID to match the PCI device. The value
0x terminates the list of entries.
UINT16 DeviceId; ///< Device ID to match the PCI device
UINT32 ClockRate; ///< UART clock rate. Set to 0 for default clock
rate of 1843200 Hz
UINT64 Offset; ///< The byte offset into to the BAR
UINT8 BarIndex; ///< Which BAR to get the UART base address
UINT8 RegisterStride; ///< UART register stride in bytes. Set to 0 for
default register stride of 1 byte.
UINT8 Reserved[6];
} PCI_SERIAL_PARAMETER;
You need to add this module to the MdeModulePkg DSC file.
The code is currently limited to 115200 baud. There are many UARTs that
support higher baud rates, so this restriction needs to be removed.
If a non-standard ClockRate is used, then the actual baud rate may not exactly
match the expected baud rate, but as long as we are within a few percent, the
UART is stable.
I would like to use this driver as a replacement for the UART driver for Quark
that I have posted on GitHub at:
https://github.com/mdkinney/edk2/tree/Quark/QuarkSocPkg/QuarkSouthCluster/Uart/Dxe
It has support for baud rate up to 921600 and closest baud rate match with a
few percent and support for larger Tx FIFOs and extra flushes of the Tx FIFO
when doing a Reset or a UART config change. Please let me know if you want me
to help merge in some of those features to your patch.
Thanks,
Mike
> -Original Message-
> From: Tian, Feng
> Sent: Wednesday, December 2, 2015 9:13 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: edk2-devel@lists.01.org; Tian, Feng <feng.t...@intel.com>
> Subject: RE: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
>
> Suggest to use UINT64 for Offset field to be consistent with PciIo protocol
> definition.
>
> Others look good to me
>
> Reviewed-by: Feng Tian <feng.t...@intel.com>
>
> Thanks
> Feng
>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni,
> Ruiyu
> Sent: Monday, November 30, 2015 10:12
> To: Kinney, Michael D
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
>
> Mike,
> Could you please review whether the PCD structure defined as below is good?
>
> +#pragma pack(1)
> +///
> +/// PcdPciSerialParameters contains zero or more instances of the below
> structure.
> +/// If a PCI device contains multiple UARTs, PcdPciSerialParameters needs to
> contain
> +/// two instances of the below structure, with the VendorId and DeviceId
> equals to the
> +/// device ID and vendor ID of the device. If the PCI device uses the first
> two BARs
> +/// to support multiple UARTs, BarIndex of first instance equals to 0 and
> BarIndex of
> +/// second one equals to 1; if the PCI device uses the first BAR to support
> multiple
> +/// UARTs, BarIndex of both instance equals to 0 and Offset of first
> instance equals
> +/// to 0 while Offset of second one equals to some value bigger or equal to
> 8.
> +/// For certain UART whose register needs to be accessed in DWORD aligned
> address,
> +/// RegisterStride equals to 4.
> +///
> +typedef struct {
> + UINT16 VendorId; ///< Vendor ID to match the PCI device, 0x
> terminates the entries
> + UINT16 DeviceId; ///< Device ID to match the PCI device
> + UINT8 BarIndex; ///< Which BAR to get the UART base address
> + UINT16 Offset; ///< The offset to the BAR
> + UINT8 RegisterStride; ///< UART register stride, 0 to use 1 as register
> stride
> + UINT32 ClockRate; ///< UART clock rate, 0 to use default clock rate
> 1843200
> +} PCI_SERIAL_PARAMETER;> +#pragma pack()
>
> Regards,
> Ray
>
> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, November 25, 2015 10:30 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Tian, Feng <feng.t...@intel.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>
> Subject: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
>
> PciSioSerialDxe driver can manages UARTs on a SIO chip or a PCI/PCIE
> card.
> It manages the SIO instance whose last device path node is a ACPI
> device path and the HID in the ACPI device path node equals to
> EISA_PNP_ID (0x501).
> It also manages the PCI IO instance whose class code is 7/0/2 (16550
> UART). But when prope
Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
Ray,
I tried to build this module into a platform, and I am getting function name
collisions with BaseSerialPortLib16550 for the symbols:
SerialPortReadRegister
SerialPortWriteRegister
I recommend you change the names of these 2 functions.
Mike
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, December 3, 2015 4:00 PM
> To: Tian, Feng <feng.t...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
>
> Hi Ray,
>
> I agree a UINT64 Offset would be good for the Offset field. It would also be
> good to order the fields and pad the structure so they are
> naturally aligned. Maybe like the following with some comment changes:
>
> typedef struct {
> UINT16 VendorId; ///< Vendor ID to match the PCI device. The value
> 0x terminates the list of entries.
> UINT16 DeviceId; ///< Device ID to match the PCI device
> UINT32 ClockRate; ///< UART clock rate. Set to 0 for default clock
> rate of 1843200 Hz
> UINT64 Offset; ///< The byte offset into to the BAR
> UINT8 BarIndex; ///< Which BAR to get the UART base address
> UINT8 RegisterStride; ///< UART register stride in bytes. Set to 0 for
> default register stride of 1 byte.
> UINT8 Reserved[6];
> } PCI_SERIAL_PARAMETER;
>
> You need to add this module to the MdeModulePkg DSC file.
>
> The code is currently limited to 115200 baud. There are many UARTs that
> support higher baud rates, so this restriction needs to be
> removed.
>
> If a non-standard ClockRate is used, then the actual baud rate may not
> exactly match the expected baud rate, but as long as we are
> within a few percent, the UART is stable.
>
> I would like to use this driver as a replacement for the UART driver for
> Quark that I have posted on GitHub at:
>
>
> https://github.com/mdkinney/edk2/tree/Quark/QuarkSocPkg/QuarkSouthCluster/Uart/Dxe
>
> It has support for baud rate up to 921600 and closest baud rate match with a
> few percent and support for larger Tx FIFOs and extra
> flushes of the Tx FIFO when doing a Reset or a UART config change. Please
> let me know if you want me to help merge in some of
> those features to your patch.
>
> Thanks,
>
> Mike
>
> > -Original Message-
> > From: Tian, Feng
> > Sent: Wednesday, December 2, 2015 9:13 PM
> > To: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>
> > Cc: edk2-devel@lists.01.org; Tian, Feng <feng.t...@intel.com>
> > Subject: RE: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
> >
> > Suggest to use UINT64 for Offset field to be consistent with PciIo protocol
> > definition.
> >
> > Others look good to me
> >
> > Reviewed-by: Feng Tian <feng.t...@intel.com>
> >
> > Thanks
> > Feng
> >
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni,
> > Ruiyu
> > Sent: Monday, November 30, 2015 10:12
> > To: Kinney, Michael D
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
> >
> > Mike,
> > Could you please review whether the PCD structure defined as below is good?
> >
> > +#pragma pack(1)
> > +///
> > +/// PcdPciSerialParameters contains zero or more instances of the below
> > structure.
> > +/// If a PCI device contains multiple UARTs, PcdPciSerialParameters needs
> > to contain
> > +/// two instances of the below structure, with the VendorId and DeviceId
> > equals to the
> > +/// device ID and vendor ID of the device. If the PCI device uses the
> > first two BARs
> > +/// to support multiple UARTs, BarIndex of first instance equals to 0 and
> > BarIndex of
> > +/// second one equals to 1; if the PCI device uses the first BAR to
> > support multiple
> > +/// UARTs, BarIndex of both instance equals to 0 and Offset of first
> > instance equals
> > +/// to 0 while Offset of second one equals to some value bigger or equal
> > to 8.
> > +/// For certain UART whose register needs to be accessed in DWORD aligned
> > address,
> > +/// RegisterStride equals to 4.
> > +///
> > +typedef struct {
> > + UINT16 VendorId; ///< Vendor ID to match the PCI device, 0x
> > terminates the entries
> > + UINT16 DeviceId; ///< Device ID to match the PCI device
> >
Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
Suggest to use UINT64 for Offset field to be consistent with PciIo protocol
definition.
Others look good to me
Reviewed-by: Feng Tian <feng.t...@intel.com>
Thanks
Feng
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni, Ruiyu
Sent: Monday, November 30, 2015 10:12
To: Kinney, Michael D
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
Mike,
Could you please review whether the PCD structure defined as below is good?
+#pragma pack(1)
+///
+/// PcdPciSerialParameters contains zero or more instances of the below
structure.
+/// If a PCI device contains multiple UARTs, PcdPciSerialParameters needs to
contain
+/// two instances of the below structure, with the VendorId and DeviceId
equals to the
+/// device ID and vendor ID of the device. If the PCI device uses the first
two BARs
+/// to support multiple UARTs, BarIndex of first instance equals to 0 and
BarIndex of
+/// second one equals to 1; if the PCI device uses the first BAR to support
multiple
+/// UARTs, BarIndex of both instance equals to 0 and Offset of first instance
equals
+/// to 0 while Offset of second one equals to some value bigger or equal to 8.
+/// For certain UART whose register needs to be accessed in DWORD aligned
address,
+/// RegisterStride equals to 4.
+///
+typedef struct {
+ UINT16 VendorId; ///< Vendor ID to match the PCI device, 0x
terminates the entries
+ UINT16 DeviceId; ///< Device ID to match the PCI device
+ UINT8 BarIndex; ///< Which BAR to get the UART base address
+ UINT16 Offset; ///< The offset to the BAR
+ UINT8 RegisterStride; ///< UART register stride, 0 to use 1 as register
stride
+ UINT32 ClockRate; ///< UART clock rate, 0 to use default clock rate
1843200
+} PCI_SERIAL_PARAMETER;
+#pragma pack()
Regards,
Ray
-Original Message-
From: Ni, Ruiyu
Sent: Wednesday, November 25, 2015 10:30 PM
To: edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Tian, Feng <feng.t...@intel.com>; Kinney,
Michael D <michael.d.kin...@intel.com>
Subject: [Patch 1/2] MdeModulePkg: Add PciSioSerialDxe driver
PciSioSerialDxe driver can manages UARTs on a SIO chip or a PCI/PCIE
card.
It manages the SIO instance whose last device path node is a ACPI
device path and the HID in the ACPI device path node equals to
EISA_PNP_ID (0x501).
It also manages the PCI IO instance whose class code is 7/0/2 (16550
UART). But when proper value is set to PcdPciSerialParameters, the
driver can also manage non-standard PCI serial cards by matching
the Vendor ID and Device ID specified in PcdPciSerialParameters.
The PCI BAR index, IO/MMIO offset, register stride, clock rate can
also be specified through the same PCD.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
Cc: Feng Tian <feng.t...@intel.com>
Cc: Michael Kinney <michael.d.kin...@intel.com>
---
.../Bus/Pci/PciSioSerialDxe/ComponentName.c| 288 +
.../Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf| 81 ++
.../Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.uni| Bin 0 -> 1940 bytes
.../Pci/PciSioSerialDxe/PciSioSerialDxeExtra.uni | Bin 0 -> 1380 bytes
MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c | 1206 +++
MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.h | 787 +
MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c| 1243
MdeModulePkg/MdeModulePkg.dec | 34 +
8 files changed, 3639 insertions(+)
create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/ComponentName.c
create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf
create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.uni
create mode 100644
MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeExtra.uni
create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c
create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.h
create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c
diff --git a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/ComponentName.c
b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/ComponentName.c
new file mode 100644
index 000..994dc84
--- /dev/null
+++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/ComponentName.c
@@ -0,0 +1,288 @@
+/** @file
+ UEFI Component Name and Name2 protocol for Isa serial driver.
+
+Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+This program and the accompanying materials
+are licensed and made available under the terms and conditions of the BSD
License
+which accompanies this distribution. The full text of the license may be
found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
