Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tahan: config: adjusted sensor config to support 2nd source #281

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zhongedward
Copy link
Contributor

Description
This PR is for tahan 2nd source sensor service.

Motivation
1.Than 2nd source sensor list is as following:
https://docs.google.com/spreadsheets/d/1r0Vbv1cx3KelYLEM4vl0VNyIeEpFwurG5JJZ6BckMCI/edit?gid=1266933266#gid=1266933266
image

image

2.Currently,all the SMB number combinations and COME number combinations are as follows,in the future if factories have new numbers, we should add them.
image

3.In platform config, adjust SMBFRU_SLOT to SMB_FRU_SLOT,adjust BCB_SLOT.

4.Test Plan
1.The correctness of the format has been verified on this website https://jsonlint.com/
2.Used jq cmd to pretty the format.
3.Test log as follows:
Tested both in janga dvt main source & janga dvt 2nd source machine.

tahan 2nd source tested log:
image

tahan main source tested log:

tahan_dvt

tahan_dvt_2nd_source_platform_test_log.txt tahan_dvt_2nd_source_sensor_test_log.txt tahan_dvt_platform_test_log.txt tahan_dvt_sensor_test_log.txt

FYI: the sensor_hw_test_log:
tahan_dvt_2nd_source_sensor_hw_test_log.txt
tahan_dvt_sensor_hw_test_log.txt

@facebook-github-bot
Copy link
Contributor

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

@@ -12,6 +12,15 @@
},
"pmUnitName": "TAHAN"
},
"SMB_FRU_SLOT": {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since "SMB_SLOT" is renamed as "TAHAN_SLOT", can we rename "SMB_FRU_SLOT" to "SMB_SLOT" please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.Hi , You have a point.Named to SMB_FRU_SLOT is because in this slot we only put the versioned sensors not all SMB sensors, SMB_SLOT is a big scope, we just avoid the misunderstanding and confusion.That’s why it’s named that.
image

2.If you insist on modifying it, I can do so and wait for your reply, thank you!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Okay, can we rename "SMB" PmUnit to "SMB_FRU" then to reflect that it's versioned PmUnit in the name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's a good point.According to your suggestion i have modified it and tested pass. Committed it again.Please review, thanks.

image
image
image

{
"sensors": [
{
"name": "TH5_VDD_CORE_IOUT",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sensors like "TH5_VDD_CORE_IOUT" and "TH5_VDD_CORE_VIN" have identical definitions across all "versionedSensors" field.

Please identify all the other sensors like them and move them to "sensors" field in line 1142.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi kimdo

1.Thanks for your reply, i know your point, in fact "TH5_VDD_CORE_IOUT" and "TH5_VDD_CORE_VIN" are versioned sensors so i put it in SMB_FRU_SLOT. The SMB common sensor we put in the TAHAN slot.

2.All the number combinations(productionstate, productversion, product subversion) are as follow(in the future PVT will add new)
image

3.So i think every number combination will have these sensors according to current platform config structure. If i am wrong please correct me, than you!

@facebook-github-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants