Re: [PATCH 1/2] MdePkg/IndustryStandard: Adds definitions for IPMI SSIF

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]


Hi Tinh,
Below is my comments,

> -----Original Message-----
> From: Tinh Nguyen <tinhnguyen@os.amperecomputing.com>
> Sent: Friday, April 28, 2023 12:00 PM
> To: devel@edk2.groups.io
> Cc: patches@amperecomputing.com; michael.d.kinney@intel.com;
> gaoliming@byosoft.com.cn; zhiguang.liu@intel.com; Chang, Abner
> <Abner.Chang@amd.com>; Tinh Nguyen
> <tinhnguyen@os.amperecomputing.com>
> Subject: [PATCH 1/2] MdePkg/IndustryStandard: Adds definitions for IPMI
> SSIF
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> Specification reference:
> https://www.intel.com/content/www/us/en/products/docs/servers/ipmi/i
> pmi-second-gen-interface-spec-v2-rev1-1.html
> 
> Signed-off-by: Tinh Nguyen <tinhnguyen@os.amperecomputing.com>
> ---
>  MdePkg/MdePkg.dec                          | 26 ++++++
>  MdePkg/Include/IndustryStandard/IpmiSsif.h | 98
> ++++++++++++++++++++
>  2 files changed, 124 insertions(+)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index
> 7488ccda7a00..518e4200e9af 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -10,6 +10,7 @@
>  # Copyright (c) 2022, Loongson Technology Corporation Limited. All rights
> reserved.<BR>  # Copyright (c) 2021 - 2022, Arm Limited. All rights
> reserved.<BR>  # Copyright (C) 2023 Advanced Micro Devices, Inc. All rights
> reserved.<BR>
> +# Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -2353,6 +2354,31 @@
> [PcdsFixedAtBuild,PcdsPatchableInModule]
>    # @Prompt IPMI KCS Interface I/O Base Address
> 
> gEfiMdePkgTokenSpaceGuid.PcdIpmiKcsIoBaseAddress|0xca2|UINT16|0x00
> 000031
> 
> +  ## This is SMBus slave address for the SSIF to the BMC.
> +  #  The recommended value defined by IPMI specification is 0x20 (section
> 12.12).
> +  # @Prompt IPMI SSIF SMBus slave address
> +
> gEfiMdePkgTokenSpaceGuid.PcdIpmiSmbusSlaveAddr|0x20|UINT8|0x00000
> 032
> +
> +  ## This is the maximum number of IPMI SSIF request retries.
> +  #  The IPMI specification specified min value is 5 (section 12.17).
> +  # @Prompt Number of IPMI SSIF request retries.
> +
> +
> gEfiMdePkgTokenSpaceGuid.PcdIpmiSsifRequestRetryCount|0x05|UINT8|0
> x000
> + 00033
> +
> +  ## This is the required interval for each IPMI request retry.
> +  #  The IPMI specification specified a time range of 60ms to 250ms (section
> 12.17).
> +  #  The default setting is min.
> +  # @Prompt Time between IPMI SSIF request retries.
> +
> +
> gEfiMdePkgTokenSpaceGuid.PcdIpmiSsifRequestRetryInterval|60000|UINT3
> 2|
> + 0x00000034
Please give the unit to PCD name, for example PcdIpmiSsifRequestRetryIntervalMicrosecond that looks more clear to readers.


> +
> +  ## This value is the maximum retries of an IPMI SSIF response  #
> + @Prompt Number of IPMI SSIF response retries.
> +
> +
> gEfiMdePkgTokenSpaceGuid.PcdIpmiSsifResponseRetryCount|250|UINT8|0
> x000
> + 00035
Is 250 times of retry defined in spec? Seems to me it is too many?


> +
> +  ## This is the required interval for each IPMI response retry.
> +  #  The IPMI specification specified min value is 60ms (section 12.17).
> +  # @Prompt Time-out for a response, internal
> +
> +
> gEfiMdePkgTokenSpaceGuid.PcdIpmiSsifResponseRetryInterval|60000|UINT
> 32
> + |0x00000036
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## This value is used to set the base address of PCI express hierarchy.
>    # @Prompt PCI Express Base Address.
> diff --git a/MdePkg/Include/IndustryStandard/IpmiSsif.h
> b/MdePkg/Include/IndustryStandard/IpmiSsif.h
> new file mode 100644
> index 000000000000..4a97438109a9
> --- /dev/null
> +++ b/MdePkg/Include/IndustryStandard/IpmiSsif.h
> @@ -0,0 +1,98 @@
> +/** @file
> +  IPMI SSIF Definitions
> +
> +  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Revision Reference:
> +    - IPMI Specification
> +      Version 2.0, Rev. 1.1
> +
> +
> +https://www.intel.com/content/www/us/en/products/docs/servers/ipmi/
> ipmi
> +-second-gen-interface-spec-v2-rev1-1.html
> +**/
> +
> +#ifndef IPMI_SSIF_H_
> +#define  IPMI_SSIF_H_
An additional whitespace between "#define"  and "IPMI_SSIF_H_".


> +
> +///
> +/// Definitions for SMBUS Commands for SSIF /// Table 12 - Summary of
> +SMBUS Commands for SSIF ///
> +
> +// Write block
Please have a consist comment format "///".


> +#define IPMI_SSIF_SMBUS_CMD_SINGLE_PART_WRITE        0x02
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_WRITE_START   0x06
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_WRITE_MIDDLE  0x07
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_WRITE_END     0x08
> +
> +// Read block
Please have a consist comment format "///".


Thanks
Abner

> +#define IPMI_SSIF_SMBUS_CMD_SINGLE_PART_READ        0x03
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_READ_START   0x03
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_READ_MIDDLE  0x09
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_READ_END     0x09
> +#define IPMI_SSIF_SMBUS_CMD_MULTI_PART_READ_RETRY   0x0A
> +
> +///
> +/// Definitions for Multi-Part Read Transactions /// Section 12.5 ///
> +#define IPMI_SSIF_MULTI_PART_READ_START_SIZE      0x1E
> +#define IPMI_SSIF_MULTI_PART_READ_START_PATTERN1  0x00 #define
> +IPMI_SSIF_MULTI_PART_READ_START_PATTERN2  0x01
> +#define IPMI_SSIF_MULTI_PART_READ_END_PATTERN     0xFF
> +
> +///
> +/// IPMI SSIF maximum message size
> +///
> +#define IPMI_SSIF_INPUT_MESSAGE_SIZE_MAX   0xFF
> +#define IPMI_SSIF_OUTPUT_MESSAGE_SIZE_MAX  0xFF
> +
> +///
> +/// IPMI SMBus system interface maximum packet size in byte /// #define
> +IPMI_SSIF_MAXIMUM_PACKET_SIZE_IN_BYTES  0x20
> +
> +typedef enum {
> +  IpmiSsifPacketStart = 0,
> +  IpmiSsifPacketMiddle,
> +  IpmiSsifPacketEnd,
> +  IpmiSsifPacketSingle,
> +  IpmiSsifPacketMax
> +} IPMI_SSIF_PACKET_ATTRIBUTE;
> +
> +#pragma pack (1)
> +///
> +/// IPMI SSIF Interface Request Format
> +/// Section 12.2 and 12.3
> +///
> +typedef struct {
> +  UINT8    NetFunc;
> +  UINT8    Command;
> +} IPMI_SSIF_REQUEST_HEADER;
> +
> +///
> +/// IPMI SSIF Interface Response Format /// Section 12.4 and 12.5 ///
> +typedef struct {
> +  UINT8    StartPattern[2];
> +  UINT8    NetFunc;
> +  UINT8    Command;
> +} IPMI_SSIF_RESPONSE_PACKET_START;
> +
> +typedef struct {
> +  UINT8    BlockNumber;
> +} IPMI_SSIF_RESPONSE_PACKET_MIDDLE;
> +
> +typedef struct {
> +  UINT8    EndPattern;
> +} IPMI_SSIF_RESPONSE_PACKET_END;
> +
> +typedef struct {
> +  UINT8    NetFunc;
> +  UINT8    Command;
> +} IPMI_SSIF_RESPONSE_SINGLE_PACKET;
> +
> +#pragma pack ()
> +
> +#endif /* IPMI_SSIF_H_ */
> --
> 2.40.0