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

[tn48m] Add new platform TN48M2-SWDEV #217

Merged
merged 5 commits into from Jul 27, 2023

Conversation

CaryChen-Deltaww
Copy link
Contributor

@CaryChen-Deltaww CaryChen-Deltaww commented May 16, 2023

  • Add new platform support for TN48M2-SWDEV to Dent-3.0. This Platform is modified from TN48M2 with the SPI flash changed to 64MB to accommodate switchdev supported ONIE.
  • Show DC power input status in ONLP for TN48M2 and TN48M2-SWDEV platform

Additional pull request also created in Dentproject Linux repo for driver and device tree support
https://github.com/dentproject/linux/pull/9

Copy link
Contributor

@paulmenzel paulmenzel left a comment

Choose a reason for hiding this comment

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

Nit: It’s spelled switchdev.

@@ -40,6 +40,7 @@ typedef enum plat_id {
PID_TN4810M_PVT,
PID_TN48M2,
PID_TN4810M_NONPVT,
PID_TN48M2_SWDEV,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort it (below PID_TN48M2)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the IDs are fixed by HW, its not suitable to disarray them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. It’d be nice, if you mentioned the (new) ID (number) in the commit message.

[PLAT_PSU_ID_3] = {
.name = "DCIN",
.type = PLAT_PSU_TYPE_DC12,
.power_status_path = "/sys/bus/i2c/devices/0-0041/dcin_powergood",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it dcin and not psu3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is representing a DC power connector, there is no physical DC PSU in the box.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. Maybe mention that hardware detail in the commit message.

@@ -0,0 +1 @@
include $(ONL)/make/pkg.mk
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the endings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -108,7 +109,23 @@ plat_info_t gPlat_info[] = {

.thermal_count = 11,
.fan_count = 3,
.psu_count = 2,
.psu_count = 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please factor out changing an existing platform into a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, but it’d be great if you rephrased the commit message. Maybe something like:

With the new device model …, a third PSU has to be added, so increase and add the third PSU type.

@taraschornyiplv
Copy link
Contributor

reverify

 - Add new platform support for TN48M2-SWDEV to Dent-3.0.
   This Platform is modified from TN48M2 with the SPI flash changed to 64MB to accommodate switchdev supported ONIE.

Signed-off-by: Cary Chen <cary.sc.chen@deltaww.com>
 - Added support to show DC power input status in ONLP for TN48M2 platform

Signed-off-by: Cary Chen <cary.sc.chen@deltaww.com>
@CaryChen-Deltaww
Copy link
Contributor Author

Added fix and changes as @paulmenzel commented

@taraschornyiplv taraschornyiplv requested a review from a team May 17, 2023 10:21
@@ -63,6 +63,7 @@ arm64-nxp-ls1046ardb-r0
arm64-delta-tx4810-r0
arm64-delta-tn4810m-r0
arm64-delta-tn48m2-r0
arm64-delta-tn48m2-swdev-r0
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the only difference is the SPI flash size, please reconsider renaming the suffix from "swdev" to something like "spi64m". The fact that the board has extra SPI can be used for other things in the HW level, and it is not specific to switch-dev, which is in the software layer only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this platform name isn't quite distinctive, however the name was decided by our customer.

@CaryChen-Deltaww
Copy link
Contributor Author

Hi there, do we have any schedule for this PR getting merged into main?

@paulmenzel
Copy link
Contributor

  • Added support to show DC power input status in ONLP for TN48M2 and TN48M2-SWDEV platform

Please use imperative mood.

Additional pull request also created in Dentproject Linux repo for driver and device tree support

Please make it a link (add the URL).

Copy link
Contributor

@paulmenzel paulmenzel left a comment

Choose a reason for hiding this comment

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

I still find it hard to review in GitHub. Maybe a commit in the beginning just copying the files, and then adding separate commits on top would make review easier.

@taraschornyiplv
Copy link
Contributor

reverify

@taraschornyiplv taraschornyiplv merged commit 407966a into dentproject:main Jul 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants