fby4: sd: Initial commit for yv4-sd pdr implementation by BonnieLo · Pull Request #1304 · facebook/OpenBIC

BonnieLo · 2023-08-23T07:22:26Z

Description

Support pldm over mctp over i2c on yv4-sd.
Add platform code for pdr implementation.

Motivation

For implementing sensor monitoring, the BMC should get sensor information from the BIC.

Test Plan:

Build code: Pass
Add some PDR samples in BIC and BMC get pdr from BIC: Pass

Log:

Get PDR info:
pldmtool raw -m 8 -d 0x80 0x02 0x50
pldmtool: Tx: 80 02 50
pldmtool: Rx: 00 02 50 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00 44 01 00 00 6c 00 00 00 00

Get PDR:
pldmtool platform GetPDR -m 8 -a -v
[
pldmtool: Tx: 80 02 51 00 00 00 00 00 00 00 00 01 ff ff 00 00 Success in creating the socket : RC = 4
Write to socket successful : RC = 16
Total length:123
pldmtool: Rx: 00 02 51 00 01 00 00 00 00 00 00 00 05 6c 00 00 00 00 00 01 02 00 00 60 00 00 00 00 10 20 00 01 88 00 10 00 00 00 00 05 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0d 00 90 08 0a 00 00 20 0a 00 a0 0d 0a 00 b0 1b 0e 00 4d 01 0a 00 5b 00
{
"nextRecordHandle": 1,
"responseCount": 108,
"recordHandle": 0,
"PDRHeaderVersion": 1,
"PDRType": "Numeric Sensor PDR",
"recordChangeNumber": 0,
"dataLength": 96
}
,pldmtool: Tx: 81 02 51 01 00 00 00 00 00 00 00 01 ff ff 00 00 Success in creating the socket : RC = 4
Write to socket successful : RC = 16
Total length:123
pldmtool: Rx: 01 02 51 00 02 00 00 00 00 00 00 00 05 6c 00 01 00 00 00 01 02 00 00 60 00 00 00 00 10 2d 00 01 88 00 10 00 00 00 00 05 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6e 03 01 00 ca 02 01 00 80 03 01 00 89 02 01 00 02 00 02 00 04 00 00 00
{
"nextRecordHandle": 2,
"responseCount": 108,
"recordHandle": 1,
"PDRHeaderVersion": 1,
"PDRType": "Numeric Sensor PDR",
"recordChangeNumber": 0,
"dataLength": 96
}
,pldmtool: Tx: 82 02 51 02 00 00 00 00 00 00 00 01 ff ff 00 00 Success in creating the socket : RC = 4
Write to socket successful : RC = 16
Total length:123
pldmtool: Rx: 02 02 51 00 00 00 00 00 00 00 00 00 05 6c 00 02 00 00 00 01 02 00 00 60 00 00 00 00 10 01 00 01 88 00 10 00 00 00 00 02 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 29 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 34 00 00 00 96 00 00 00 00 00
{
"nextRecordHandle": 0,
"responseCount": 108,
"recordHandle": 2,
"PDRHeaderVersion": 1,
"PDRType": "Numeric Sensor PDR",
"recordChangeNumber": 0,
"dataLength": 96
}
]

facebook-github-bot · 2023-08-23T07:22:36Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jagpalgill · 2023-08-25T23:49:39Z

@@ -0,0 +1,326 @@
+/*


The mctp code seems to be generic and better to keep it in common layer, so other platforms can also use it.

It is a lot more complicate to put the code in common layer, since every platform is using the same common code. We need to make sure that ongoing projects won't be affected. Therefore, we plan to put this on platform like other project in this stage.

@PeterHo-wiwynn can you move it to common and change meta-facebook/gt-cc/src/platform/plat_mctp.c as well. I can help with testing GTT/GTI which are the only other platforms which use this.

Preferably I would like to see plat_*.c contain just configuration data structures not functions which do actions.

Sure, I will move it to common. However, there are still some functions which implemented differently between platforms. I will try to move the functions doing same behavior, and list the others for disscusion.

jagpalgill · 2023-08-25T23:51:19Z

+}
+
+static void set_endpoint_resp_handler(void *args, uint8_t *buf, uint16_t len)
+{


The API is not setting any resp_handler as suggested by name or are you going to populate this in next commit? If so, better to leave a TODO for indications.

It doesn't set device endpoint in this commit. I leaved a TODO for indication.

jagpalgill · 2023-08-25T23:51:46Z

+
+static void set_endpoint_resp_timeout(void *args)
+{
+	CHECK_NULL_ARG(args);


The API is not setting any timeout or are you going to populate it in next commit? If so, better to leave a TODO for indications.

It doesn't set device endpoint in this commit. I leaved a TODO for indication.

jagpalgill · 2023-08-25T23:52:11Z

+			msg.timeout_cb_fn = set_endpoint_resp_timeout;
+			msg.timeout_cb_fn_args = p;
+
+			mctp_ctrl_send_msg(find_mctp_by_smbus(p->bus), &msg);


Is mctp_ctrl_send_msg NULL safe for in_arg?

It will check the input args.
https://github.com/facebook/OpenBIC/blob/main/common/service/mctp/mctp_ctrl.c#L276

jagpalgill · 2023-08-25T23:52:35Z

+
+	switch (msg_type) {
+	case MCTP_MSG_TYPE_CTRL:
+		LOG_ERR("type: mctp_ctrl");


LOG_INFO or DEBUG rather than LOG_ERR?

Sorry, it was supposed to be delete.

jagpalgill · 2023-08-25T23:54:05Z

+		medium_type = MCTP_MEDIUM_TYPE_SMBUS;
+		break;
+		default:
+		medium_type = -1;


Already -1, no need to set again.

jagpalgill · 2023-08-25T23:54:10Z

+		target = I2C_BUS_BMC;
+		break;
+		default:
+		target = -1;


Already -1, no need to set again.

jagpalgill · 2023-08-25T23:54:34Z

+
+		uint8_t rc = mctp_set_medium_configure(p->mctp_inst, MCTP_MEDIUM_TYPE_SMBUS, p->conf);
+		if (rc != MCTP_SUCCESS) {
+			LOG_INF("mctp set medium configure failed");


Should this be LOG_ERR?

jagpalgill · 2023-08-25T23:55:01Z

+
+		mctp_reg_msg_rx_func(p->mctp_inst, mctp_msg_recv);
+
+		ret = mctp_start(p->mctp_inst);


Better to print some error if start fails?

The mctp_start will print error itself.
https://github.com/facebook/OpenBIC/blob/main/common/service/mctp/mctp.c#L481

jagpalgill · 2023-08-25T23:55:45Z

+#define MAX_SENSOR_SIZE 60
+
+uint8_t plat_get_pdr_size();
+void load_pdr_table(void);


Better to use consistent plat_ prefix (if possible, i.e. doesn't involve much change in other places).

facebook-github-bot · 2023-08-28T09:02:45Z

@BonnieLo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-08-28T09:02:53Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-08-28T09:07:45Z

@BonnieLo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-08-28T09:07:52Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-08-29T05:52:53Z

@BonnieLo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-08-29T05:53:00Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-09-08T08:43:49Z

@BonnieLo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-09-08T08:43:57Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Description - Support pldm over mctp over i2c on yv4-sd. - Add platform code for pdr implementation. Motivation For implementing sensor monitoring, the BMC should get sensor information from the BIC. Test Plan: - Build code: Pass - Add some PDR samples in BIC and BMC get pdr from BIC: Pass Log: Get PDR info: pldmtool raw -m 8 -d 0x80 0x02 0x50 pldmtool: Tx: 80 02 50 pldmtool: Rx: 00 02 50 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00 44 01 00 00 6c 00 00 00 00 Get PDR: pldmtool platform GetPDR -m 8 -a -v [ pldmtool: Tx: 80 02 51 00 00 00 00 00 00 00 00 01 ff ff 00 00 Success in creating the socket : RC = 4 Write to socket successful : RC = 16 Total length:123 pldmtool: Rx: 00 02 51 00 01 00 00 00 00 00 00 00 05 6c 00 00 00 00 00 01 02 00 00 60 00 00 00 00 10 20 00 01 88 00 10 00 00 00 00 05 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 3f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0d 00 90 08 0a 00 00 20 0a 00 a0 0d 0a 00 b0 1b 0e 00 4d 01 0a 00 5b 00 { "nextRecordHandle": 1, "responseCount": 108, "recordHandle": 0, "PDRHeaderVersion": 1, "PDRType": "Numeric Sensor PDR", "recordChangeNumber": 0, "dataLength": 96 } ,pldmtool: Tx: 81 02 51 01 00 00 00 00 00 00 00 01 ff ff 00 00 Success in creating the socket : RC = 4 Write to socket successful : RC = 16 Total length:123 pldmtool: Rx: 01 02 51 00 02 00 00 00 00 00 00 00 05 6c 00 01 00 00 00 01 02 00 00 60 00 00 00 00 10 2d 00 01 88 00 10 00 00 00 00 05 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 6e 03 01 00 ca 02 01 00 80 03 01 00 89 02 01 00 02 00 02 00 04 00 00 00 { "nextRecordHandle": 2, "responseCount": 108, "recordHandle": 1, "PDRHeaderVersion": 1, "PDRType": "Numeric Sensor PDR", "recordChangeNumber": 0, "dataLength": 96 } ,pldmtool: Tx: 82 02 51 02 00 00 00 00 00 00 00 01 ff ff 00 00 Success in creating the socket : RC = 4 Write to socket successful : RC = 16 Total length:123 pldmtool: Rx: 02 02 51 00 00 00 00 00 00 00 00 00 05 6c 00 02 00 00 00 01 02 00 00 60 00 00 00 00 10 01 00 01 88 00 10 00 00 00 00 02 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 29 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 34 00 00 00 96 00 00 00 00 00 { "nextRecordHandle": 0, "responseCount": 108, "recordHandle": 2, "PDRHeaderVersion": 1, "PDRType": "Numeric Sensor PDR", "recordChangeNumber": 0, "dataLength": 96 } ]

facebook-github-bot · 2023-09-08T08:53:33Z

@BonnieLo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-09-08T08:53:38Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-09-08T19:28:43Z

This pull request has been merged in 77ab5fd.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2023

jagpalgill reviewed Aug 25, 2023

View reviewed changes

PeterHo-wiwynn force-pushed the Peter/yv4-sd-pldm-pdr branch from f6e9b46 to 4a5c066 Compare August 28, 2023 09:02

PeterHo-wiwynn force-pushed the Peter/yv4-sd-pldm-pdr branch from 4a5c066 to c2660f6 Compare August 28, 2023 09:07

PeterHo-wiwynn force-pushed the Peter/yv4-sd-pldm-pdr branch from c2660f6 to 6b59787 Compare August 29, 2023 05:52

EliHuang-wiwynn mentioned this pull request Sep 1, 2023

fby4 - Support get/set gpio through PLDM #1307

Closed

PeterHo-wiwynn force-pushed the Peter/yv4-sd-pldm-pdr branch from 6b59787 to 4f3d950 Compare September 8, 2023 08:43

PeterHo-wiwynn force-pushed the Peter/yv4-sd-pldm-pdr branch from 4f3d950 to cb45878 Compare September 8, 2023 08:53

facebook-github-bot closed this in 77ab5fd Sep 8, 2023

facebook-github-bot added the Merged label Sep 8, 2023


		mctp_reg_msg_rx_func(p->mctp_inst, mctp_msg_recv);

		ret = mctp_start(p->mctp_inst);

Uh oh!

Conversation

BonnieLo commented Aug 23, 2023

Description

Motivation

Test Plan:

Log:

facebook-github-bot commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 28, 2023

facebook-github-bot commented Aug 28, 2023

facebook-github-bot commented Aug 28, 2023

facebook-github-bot commented Aug 28, 2023

facebook-github-bot commented Aug 29, 2023

facebook-github-bot commented Aug 29, 2023

facebook-github-bot commented Sep 8, 2023

facebook-github-bot commented Sep 8, 2023

facebook-github-bot commented Sep 8, 2023

facebook-github-bot commented Sep 8, 2023

facebook-github-bot commented Sep 8, 2023

Labels

5 participants