Move sensor read to common code by Yi-Shum · Pull Request #144 · facebook/OpenBIC

Yi-Shum · 2022-01-26T09:19:49Z

Summary:

Move code about sensor reading from platform code to common code.
Modify sensor reading value to 4 bytes, 2 bytes for integer, 2 bytes for fraction.
Provide README file describing how to add new device.

Notice:

BMC needs to support parsing 4-byte accuracy sensor reading. Otherwise, sensor-util command would print wrong value.

Test Plan:

Build code: Pass
Test OEM sensor reading command: Pass

Log:

root@bmc-oob:~# bic-util slot1 0xe0 0x23 0x9c 0x9c 0x00 0x01 0x00
9C 9C 00 26 00 00 00 C0

Summary: - Move code about sensor reading from platform code to common code. - Provide README file describing how to add new sensors. Test Plan: - Build code: Pass - Command test: Pass Log: ``` root@bmc-oob:~# bic-util slot1 0xe0 0x23 0x9c 0x9c 0x00 0x01 0x00 9C 9C 00 26 00 00 00 C0 ```

facebook-github-bot · 2022-01-26T18:49:23Z

@GoldenBug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

DelphineChiu · 2022-01-27T02:17:20Z

@GoldenBug @garnermic

This PR need to be phased in with BMC code change, otherwise it will cause sensor reading/reporting issues for all the BIC sensors. BMC portion is planned to be PRed by today taiwan time and not yet pull requested.

For this moment, (before one day of release), I would suggest that we don't merge them (both BMC/BIC port) today.

Wiwynn is discussing with ODMB to get more detail test status to get more confidence for the pull request.

GoldenBug · 2022-01-27T02:48:14Z

+  sequence.resolution  = ADC_RESOLUTION;
+  sequence.calibrate   = ADC_CALIBRATE;
+
+  for (uint8_t i = 0; i < ADC_NUM; i++) {


In this for loop, if ADC0 is not ready, it skips ADC1 totally. Is this the expected behavior or should it also check ADC1?

In this case, ADC0 will be skipped and continue to initialize ADC1.

GoldenBug · 2022-01-27T02:48:47Z

+#include <string.h>
+#include "intel_peci.h"
+
+#define READ_RTY 0x03


I am assuming RTY is retry, but it is not clear. In this #define and in the function names below, retry should be spelled out if that is what it is intended to do.

We will rename rty to retry in next commit.

GoldenBug · 2022-01-27T02:48:56Z

+#define RDPKG_IDX_DIMM_TEMP 0x0E
+#define RDPKG_IDX_TJMAX_TEMP 0x10
+
+static bool peci_rty_read(uint8_t cmd, uint8_t addr, uint8_t idx, uint16_t param, uint8_t rlen, uint8_t *rbuf)


rty should be retry

We will rename rty to retry in next commit.

GoldenBug · 2022-01-27T02:49:18Z

+  uint8_t rbuf[rlen];
+  memset(rbuf, 0, sizeof(rbuf));
+
+  if (peci_rty_read(PECI_CMD_RD_PKG_CFG0, addr, RDPKG_IDX_TJMAX_TEMP, param, rlen, rbuf) == false)


Same as above with rty

We will rename rty to retry in next commit.

GoldenBug · 2022-01-27T02:49:29Z

+  if (!reading)
+    return false;
+
+  const uint16_t param = (type == PECI_TEMP_DIMM_A) ? 0x00 :


This makes assumptions on DIMM_X always being mapped to DIMM_A through DIMM_H on platforms. With the recent proposal by the RTP team to renumber DIMM slots to A0 through A7, hard-coding these values might lead to future code changes to accommodate both numberings.

Thanks for the suggestion, I think we can follow the intel PECI command format to define which value should be used in the PECI generic code.
In the PECI specification, the DDR DIMM temperature command with
request parameters - channel index, and
response data - DIMM# temperature.
There are 8 channels in this CPU SKU with two DIMMs per channel.
Therefore, the DIMM temperature of the enum can be defined as below.

PECI_TEMP_CHANNEL0_DIMM0, PECI_TEMP_CHANNEL0_DIMM1, PECI_TEMP_CHANNEL1_DIMM0, PECI_TEMP_CHANNEL1_DIMM1, PECI_TEMP_CHANNEL2_DIMM0, PECI_TEMP_CHANNEL2_DIMM1, PECI_TEMP_CHANNEL3_DIMM0, PECI_TEMP_CHANNEL3_DIMM1, PECI_TEMP_CHANNEL4_DIMM0, PECI_TEMP_CHANNEL4_DIMM1, PECI_TEMP_CHANNEL5_DIMM0, PECI_TEMP_CHANNEL5_DIMM1, PECI_TEMP_CHANNEL6_DIMM0, PECI_TEMP_CHANNEL6_DIMM1, PECI_TEMP_CHANNEL7_DIMM0, PECI_TEMP_CHANNEL7_DIMM1,

In the platform code, it can map the DIMM temperature sensor to the physical DIMM channel with any naming by the above enum define.
I think it is more clear in the common code and separate the platform naming rule.

I am looking forward to your reply.
Thanks

GoldenBug · 2022-01-27T02:49:57Z

+    peci_init();
+    is_init = true;
+  }
+  return true;


Not sure about the return code for this function. It never returns false even if the sensor is not initialized. Thus, I think this line should be return is_init and not return true.

Will check if the return value of peci_init() is except.
If not, return false.

GoldenBug · 2022-01-27T02:50:23Z

+
+uint8_t isl28022_read(uint8_t sensor_num, int* reading) {
+  if((reading == NULL) ||
+     (sensor_config[SnrNum_SnrCfg_map[sensor_num]].init_args == NULL)) {


Need consistency in naming of 'sensor' or 'Snr'. probably just use sensor

Will discuss with ODMA.

Summary: - Modify macro and function name from "rty" to "retry". - Add enumerate for return value of device return function. - Modify DIMM name in intel_peci. - Check if peci is successfully initialized in intel_peci. Test Plan: - Build Code: Pass

facebook-github-bot · 2022-01-28T07:02:53Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-07T01:12:29Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

Eddie043 · 2022-02-15T03:26:37Z

Hi @GoldenBug,

Thanks for your suggestion and we modified the DIMM temperature and the return code of the sensor initialization function.
The sensor naming isn't impacted the firmware behavior, so I will create another issue to track.
Except for the sensor naming, do you have other concerns about the code logic of this PR?

ODMA had been run the AC/DC cycle testing over 1000 without failed and the ODMB also passed the same test with 250 cycles.
We have enough confidence for the PR by the test result.

GoldenBug · 2022-02-23T06:50:02Z

Could you please rebase this then I can get it reviewed again?
Thanks!

facebook-github-bot · 2022-02-23T06:54:26Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-23T07:24:04Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

Yi-Shum · 2022-02-23T07:31:18Z

We've forced updated the PR to rebase the main branch.
Please help to review again, thank you.

GoldenBug

The 2 main comments I have are:

Please extend all sensor names to follow the same pattern.
sen -> sensor and snr -> sensor
Some functions return 1 on success and 0 on failure. Usually, C standard is to return 0 on success.

GoldenBug · 2022-02-23T17:48:26Z

+            Take TMP75 sensor as an example.
+            1. common/sensor/sensor.c
+                '''
+                SEN_DRIVE_INIT_DECLARE(tmp75);


Can we spell out sensor to make it more obvious what we are referring to.

GoldenBug · 2022-02-23T17:48:50Z

+                sen_drive_tbl[] = {
+                    SEN_DRIVE_TYPE_INIT_MAP(tmp75),
+                    .....


Spell out SENSOR and sensor_

GoldenBug · 2022-02-23T17:50:20Z

+
+	default:
+		printf("Invalid sensor 0x%x\n", sensor_num);
+		return false;


Can this return a uint8_t, either an actual value or a enum for failure?

GoldenBug · 2022-02-23T17:51:10Z

+		return false;
+	}
+
+	sen_val *sval = (sen_val *)reading;


sen -> sensor

GoldenBug · 2022-02-23T17:59:45Z

+
+enum adc_device_idx { adc0, adc1, ADC_NUM };
+
+#define ADC_CHAN_NUM 8


Lets extend it to ADC_CHANNEL_NUM so it is written out like the other ADC constant names

GoldenBug · 2022-02-23T18:15:38Z

+				    uint8_t data_len)
+{
+	if (!set_full_addr(bus, addr, oft))
+		return 0;


return non-0 on failure

GoldenBug · 2022-02-23T18:17:06Z

+exit:
+	return rc;


This goto seems unnecessary. Can we just return failure when it happens?

GoldenBug · 2022-02-23T18:18:01Z

+exit:
+	return rc;


Same here, goto seems unnecessary because there isn't any cleanup happening as far as I can tell.
Just return error when it happens.

GoldenBug · 2022-02-23T18:18:22Z

+exit:
+	return rc;


Unnecessary goto

GoldenBug · 2022-02-23T18:21:23Z

+	PMBUS_POUT_OP_FAULT_LIMIT = 0x68,
+	PMBUS_POUT_OP_WARN_LIMIT = 0x6A,
+	PMBUS_PIN_OP_WARN_LIMIT = 0x6B,


was 0x69 skipped on purpose?

Summary: - Extend sensor names to follow the same pattern. "sen" -> "sensor" and "snr" -> "sensor" - Modify function return value, return 0 if success, non-0 if fail. - Remove unnecessary goto. - Add 0x69 in pmbus.h Test plan: - Build Code: Pass

facebook-github-bot · 2022-02-24T05:18:13Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

Yi-Shum · 2022-02-24T05:28:20Z

Hi @GoldenBug,
We have submitted a commit to revise the comment you provided, including:

Extend sensor names to follow the same pattern.
"sen" -> "sensor" and "snr" -> "sensor"
Modify function return value, return 0 if success, non-0 if fail.
Remove unnecessary goto.
Add 0x69 in pmbus.h

Please help to review again, thank you.

facebook-github-bot · 2022-02-24T17:58:31Z

@GoldenBug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

GoldenBug · 2022-02-25T20:22:51Z

I still see a lot of SNR uses. Please look at extending all the use cases of SNR.
We may need to change SNR in other files this commit didn't touch and that's ok.

Yi-Shum · 2022-03-01T00:44:11Z

OK, we will change all SNR to sensor.

Summary: - Extend the names of variables, functions and macros from "snr" or "sen" to "sensor" Test Plan: - Build Code: PASS

facebook-github-bot · 2022-03-01T03:06:58Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

Yi-Shum · 2022-03-01T03:57:45Z

Hi @GoldenBug,
We have submitted two commits with changes including:

merge to main branch.
Extend the name of variables, functions and macros from "snr" or "sen" to "sensor".

Please help to review again, thank you.

facebook-github-bot · 2022-03-03T00:07:04Z

@GoldenBug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-03-08T06:10:56Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

Summary: - Add path of common sensor code to yv35_bb CMakeList.txt - Apply Clang format to all change files Test plan: - Build Code: PASS

facebook-github-bot · 2022-03-08T06:35:27Z

@Yi-Shum has updated the pull request. You must reimport the pull request before landing.

Yi-Shum · 2022-03-08T06:43:00Z

Hi @GoldenBug,
We have submitted two commits with changes including:

Merge to main branch.
Add path of common sensor code into CMakeList.txt of yv35_bb.
Apply Clang format to all change files.

Please help to review again, thank you.

Summary: Recreating this PR because #144 was improperly merged from a middle commit instead of the HEAD of that PR branch. Pull Request resolved: #192 Reviewed By: garnermic Differential Revision: D34743248 Pulled By: GoldenBug fbshipit-source-id: f98af23

Summary: Adding Missing Common Code Commits for #144 Pull Request resolved: #193 Test Plan: GitHub Actions Test for Now Reviewed By: doranand Differential Revision: D34749382 Pulled By: GoldenBug fbshipit-source-id: bcd341e

Yi-Shum requested a review from GoldenBug January 26, 2022 09:19

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 Jan 26, 2022

GoldenBug suggested changes Jan 27, 2022

View reviewed changes

Modify for comment of Meta

abe9d19

Summary: - Modify macro and function name from "rty" to "retry". - Add enumerate for return value of device return function. - Modify DIMM name in intel_peci. - Check if peci is successfully initialized in intel_peci. Test Plan: - Build Code: Pass

Merge branch 'main' into Modify_sensor_read_to_common_code

b2560e2

Eddie043 requested review from GoldenBug and garnermic February 21, 2022 01:57

Merge branch main(commit 7b084a9) into Modify_sensor_read_to_common_code

147cd96

Yi-Shum force-pushed the Modify_sensor_read_to_common_code branch from 6130dd6 to 147cd96 Compare February 23, 2022 07:24

GoldenBug suggested changes Feb 23, 2022

View reviewed changes

Yi-Shum added 2 commits March 1, 2022 08:52

Merge branch 'main' into Modify_sensor_read_to_common_code

ef29c12

Extend names "snr" or "sen" to "sensor"

b923e32

Summary: - Extend the names of variables, functions and macros from "snr" or "sen" to "sensor" Test Plan: - Build Code: PASS

Merge branch 'main'(b89cd07) into Modify_sensor_read_to_common_code

49fd865

Add sensor code to yv35_bb CMakeList and Clang format

38ac074

Summary: - Add path of common sensor code to yv35_bb CMakeList.txt - Apply Clang format to all change files Test plan: - Build Code: PASS

Yi-Shum force-pushed the Modify_sensor_read_to_common_code branch from eb423a0 to 38ac074 Compare March 8, 2022 06:35

Yi-Shum requested a review from GoldenBug March 8, 2022 06:43

facebook-github-bot closed this in 8abd135 Mar 9, 2022

GoldenBug reopened this Mar 9, 2022

GoldenBug mentioned this pull request Mar 9, 2022

Modify sensor read to common code #192

Closed

GoldenBug mentioned this pull request Mar 9, 2022

Fixup: Add missing commits #193

Closed

GoldenBug closed this Mar 9, 2022


		enum adc_device_idx { adc0, adc1, ADC_NUM };

		#define ADC_CHAN_NUM 8

Uh oh!

Conversation

Yi-Shum commented Jan 26, 2022

facebook-github-bot commented Jan 26, 2022

DelphineChiu commented Jan 27, 2022

Uh oh!

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

Uh oh!

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 Jan 28, 2022

facebook-github-bot commented Feb 7, 2022

Eddie043 commented Feb 15, 2022

GoldenBug commented Feb 23, 2022

facebook-github-bot commented Feb 23, 2022

facebook-github-bot commented Feb 23, 2022

Yi-Shum commented Feb 23, 2022

GoldenBug left a 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 Feb 24, 2022

Yi-Shum commented Feb 24, 2022

facebook-github-bot commented Feb 24, 2022

GoldenBug commented Feb 25, 2022

Yi-Shum commented Mar 1, 2022

facebook-github-bot commented Mar 1, 2022

Yi-Shum commented Mar 1, 2022

facebook-github-bot commented Mar 3, 2022

facebook-github-bot commented Mar 8, 2022

facebook-github-bot commented Mar 8, 2022

Yi-Shum commented Mar 8, 2022

Labels

6 participants