Skip to content

e6000sw: Add support for MV88E6190X switch variant #1408

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.

Sign up for GitHub

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

Jump to bottom
Closed
salekseev wants to merge 2 commits into from
Closed

e6000sw: Add support for MV88E6190X switch variant #1408

salekseev wants to merge 2 commits into from

Conversation

@salekseev
Copy link

@salekseev salekseev commented Sep 3, 2024 •
edited
Loading

This commit adds support for MV88E6190X switch variant that is similar to MV88E6190 in its configuration.

It also adds is6190x hint support for enabling on amd64 platform without device tree support.

PR: 281211

@salekseev salekseev force-pushed the e6000sw_support_6190x branch from c82471c to 4e7ba71 Compare September 3, 2024 20:53
rilysh
rilysh reviewed Sep 4, 2024
View reviewed changes
@salekseev salekseev force-pushed the e6000sw_support_6190x branch 6 times, most recently from 5e19f53 to 7ea66cb Compare September 4, 2024 14:20
@salekseev salekseev requested a review from rilysh September 5, 2024 01:58
rilysh
rilysh reviewed Sep 5, 2024
View reviewed changes
@salekseev salekseev force-pushed the e6000sw_support_6190x branch from 7ea66cb to 8aece39 Compare September 5, 2024 12:37
salekseev
salekseev commented Sep 5, 2024
View reviewed changes
Copy link
Author

@salekseev salekseev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@rilysh do you think this is the best approach to check for multiple device hints being set and to initialize variables if set?

@salekseev salekseev requested a review from rilysh September 5, 2024 12:41
@salekseev salekseev force-pushed the e6000sw_support_6190x branch 2 times, most recently from d359a43 to 812efad Compare September 5, 2024 12:53
rilysh
rilysh reviewed Sep 6, 2024
View reviewed changes
@salekseev salekseev force-pushed the e6000sw_support_6190x branch 2 times, most recently from 0ef3d9c to 475d528 Compare September 7, 2024 23:42
@salekseev salekseev requested a review from rilysh September 10, 2024 11:43
@salekseev salekseev force-pushed the e6000sw_support_6190x branch from 8a68b7c to ae6652e Compare September 13, 2024 18:30
salekseev
salekseev commented Sep 13, 2024
View reviewed changes
rilysh
rilysh reviewed Sep 18, 2024
View reviewed changes
sys/dev/etherswitch/e6000sw/e6000sw.c Outdated
device_get_unit(sc->dev), "is8190", &is_6190);
resource_int_value(device_get_name(sc->dev),
device_get_unit(sc->dev), "is6190x", &is_6190x);
if (resource_int_value(device_get_name(sc->dev),
Copy link
Contributor

@rilysh rilysh Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If "is6190" is available, but "is6190x" isn't, it will just stop the execution (according to resource_int_value() function, source: https://github.com/freebsd/freebsd-src/blob/abed32f91d01d91faa709622e3279e9baec37272/sys/kern/subr_hints.c#L338). I'm not exactly sure if MV88E6190 and MV88E6190X are related to each other (without one either one can't work).

Also, return isn't returning any meaningful values, and the function expects an integer.

@bsdimp bsdimp self-assigned this Oct 4, 2024
@bsdimp bsdimp added the changes-required Cannot land as is, change requested of submitter label Oct 4, 2024
@bsdimp
Copy link
Member

bsdimp commented Oct 4, 2024

rylish's comments need to be addressed.

@salekseev
e6000sw: Add support for MV88E6190X switch variant
3b8568f 
This commit adds support for MV88E6190X switch variant that is similar
to MV88E6190 in its configuration.

It also adds is6190x hint support for enabling on amd64 platform without
device tree support.

PR: 281211

Signed-off-by: Stas Alekseev <[email protected]>
@salekseev
fixup! e6000sw: Add support for MV88E6190X switch variant
20b9633
@salekseev salekseev force-pushed the e6000sw_support_6190x branch from b2b9d57 to 20b9633 Compare October 14, 2024 19:22
erikarn pushed a commit to erikarn/freebsd-src that referenced this pull request Apr 26, 2025
e6000sw: add support for 88E6190X
07fa75e 
This adds the minimum support required to probe/attach the 88E6190X.

I've tested this against an AT&T ATT-150 OCP device (Silicom i3000)
with local changes to export MDIO via ixge(4).

Hints are required to probe/attach/configure the switch on amd64,
but with the mentioned diffs, it does work.

Thanks to Stas Alekseev <[email protected]> for the pull request
and Stas / Jason Hensler <[email protected]> for chasing
down information about the chipset, linux stuff and AT&T OCP
hardware information.

PR:		kern/281211
Pull Request:	freebsd#1408
Differential Revision:	https://reviews.freebsd.org/D50044
erikarn pushed a commit to erikarn/freebsd-src that referenced this pull request Apr 27, 2025
e6000sw: add support for 88E6190X
9b2a503 
This adds the minimum support required to probe/attach the 88E6190X.

I've tested this against an AT&T ATT-150 OCP device (Silicom i3000)
with local changes to export MDIO via ixge(4).

Hints are required to probe/attach/configure the switch on amd64,
but with the mentioned diffs, it does work.

Thanks to Stas Alekseev <[email protected]> for the pull request
and Stas / Jason Hensler <[email protected]> for chasing
down information about the chipset, linux stuff and AT&T OCP
hardware information.

PR:		kern/281211
Pull Request:	freebsd#1408
Differential Revision:	https://reviews.freebsd.org/D50044
Reviewed by:	imp
erikarn pushed a commit to erikarn/freebsd-src that referenced this pull request Apr 27, 2025
e6000sw: add support for 88E6190X
36cc2f0 
This adds the minimum support required to probe/attach the 88E6190X.

I've tested this against an AT&T ATT-150 OCP device (Silicom i3000)
with local changes to export MDIO via ixge(4).

Hints are required to probe/attach/configure the switch on amd64,
but with the mentioned diffs, it does work.

Thanks to Stas Alekseev <[email protected]> for the pull request
and Stas / Jason Hensler <[email protected]> for chasing
down information about the chipset, linux stuff and AT&T OCP
hardware information.

PR:		kern/281211
Pull Request:	freebsd#1408
Differential Revision:	https://reviews.freebsd.org/D50044
@erikarn
Copy link
Member

erikarn commented May 4, 2025

hi! This has been landed in -HEAD now, thanks!

@erikarn erikarn closed this May 4, 2025
jrtc27
jrtc27 reviewed May 4, 2025
View reviewed changes
sys/dev/etherswitch/e6000sw/e6000sw.c
if (is_6190 !=0 && is_6190x != 0)
if (bootverbose)
device_printf(dev, "Cannot configure conflicting variants\n");
return (ENXIO);
Copy link
Contributor

@jrtc27 jrtc27 May 4, 2025

Choose a reason for hiding this comment

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

Given the lack of curly braces amounts to an unconditional return (ENXIO) I don't see how this could possibly have been tested.

Copy link
Member

@emaste emaste May 4, 2025

Choose a reason for hiding this comment

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

The committed change has braces though

@jrtc27
Copy link
Contributor

jrtc27 commented May 4, 2025

There are clear style issues in this patch (lacking space around != and what look to be overly long lines). I don't know why GitHub actions didn't run checkstyle9.pl but please run it yourself locally (tools/build/checkstyle9.pl) and fix the issues that arise.

jrtc27
jrtc27 reviewed May 4, 2025
View reviewed changes
sys/dev/etherswitch/e6000sw/e6000sw.c
phandle_t switch_node;
#else
int is_6190;
int is_6190x;
Copy link
Contributor

@jrtc27 jrtc27 May 4, 2025

Choose a reason for hiding this comment

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

So, for is_6190 the code assumes one of the two properties is present (otherwise this is left uninitialised). That's a bit dodgy, but for is_6190x you definitely can't assume that otherwise you will break existing hints that predate the existence of is_6190x.

@bsdimp bsdimp added merged Closed commit that's been merged and removed changes-required Cannot land as is, change requested of submitter labels May 4, 2025
@bsdimp
Copy link
Member

bsdimp commented May 4, 2025

hi! This has been landed in -HEAD now, thanks!

Thanks!

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

Reviewers

3 more reviewers

@jrtc27 jrtc27 jrtc27 left review comments

@emaste emaste emaste left review comments

@rilysh rilysh rilysh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@bsdimp bsdimp

Labels

merged Closed commit that's been merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@salekseev @bsdimp @erikarn @jrtc27 @emaste @rilysh