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.

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

[Edgecore][as4500-52p][as4581-52p] Add new platform #285

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

[Edgecore][as4500-52p][as4581-52p] Add new platform #285

wants to merge 2 commits into from

Conversation

brandonchuang
Copy link
Contributor

@brandonchuang brandonchuang commented Oct 6, 2023 •
edited
Loading

CPU:
[as4500-52p] Marvell 98DX3530 with integrated CPU
[as4581-52p] COMe CPU Module

MAC: Marvell 98DX3530
PHY: Marvell 88E1780 x 4 (1G port 16~32)
Marvell 88E2780 x 2 (Migi-G port 33-48)
DRAM: 8GB(MAC) DDR4 SDRAM
AirFlow: Front To Back
Function port: 1 x USB port
1 x RJ45 Mgmt port
1 x RJ45 Console port
Ethernet Port: 48 x 1G
Uplink port: 4xSFP+
PoE: Microsemi PD69208M x 12 + PD69210 x 2

DTS:
The DTS for as4500-52p/as4581-52p
dentproject/linux#12
dentproject/linux#11

Signed-off-by: Brandon Chuang [email protected]

KanjiMonster
KanjiMonster requested changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@KanjiMonster KanjiMonster left a comment

Choose a reason for hiding this comment

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

Please make use of managed functions, and properly model the devices in the devicetree (it exists for a reason), if you haven't.

packages/base/any/kernels/5.15-lts/configs/arm64-all/arm64-all.config
# CONFIG_USB_LAN78XX is not set
CONFIG_USB_LAN78XX=y
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this? Does this device use an USB-LAN adapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as4560 use this USB-LAN adapter

packages/base/any/kernels/5.15-lts/configs/arm64-all/arm64-all.config Outdated
Comment on lines 7387 to 7392
# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to disable them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

packages/base/any/kernels/5.15-lts/kconfig.mk
K_GIT_BRANCH := dent-linux-5.15.y
K_GIT_BRANCH := dent-linux-5.15.105
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be part of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May I know if there is any plan to change default branch to "dent-linux-5.15.105"?

packages/platforms/accton/arm64/as456x-52p/src/modules/optoe.c Outdated
Comment on lines 1 to 3
/*
* optoe.c - A driver to read and write the EEPROM on optical transceivers
* (SFP, QSFP and similar I2C based devices)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have the changes needed added to the main/default optoe.c instead of a copy with unknown changes, which will then never receive any fixes or updates.

packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-sfp.c Outdated
Comment on lines 340 to 344
data->pdev = platform_device_register_simple(DRVNAME, -1, NULL, 0);
if (IS_ERR(data->pdev)) {
ret = PTR_ERR(data->pdev);
goto dev_reg_err;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this driver/device relies on the CPLD present, it would make more sense for the CPLD driver to register the platform device on probe instead of this module.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, to show what I want I did this for the AS4610 drivers (which use a similar pattern) with opencomputeproject/OpenNetworkLinux#963 - this should even simplify the drivers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The driver has been modified to follow the design of the AS4610.

packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-mux.c Outdated
Comment on lines 151 to 159
case as456x_cpldm_mux:
ret = as456x_mux_write(muxc->parent, client,
CPLD_CHANNEL_SELECT_REG, CPLD_CHANNEL_DESELECT_VAL);
break;
case pca9641_mux:
ret = as456x_mux_write(muxc->parent, client, 0x1, 0);
break;
default:
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise. Or add select_reg and deselect_val to struct as456x_mux_data, then you don't need a switch statement here at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified.

packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-mux.c Outdated
CPLD_CHANNEL_SELECT_REG, CPLD_CHANNEL_DESELECT_VAL);
break;
case pca9641_mux:
ret = as456x_mux_write(muxc->parent, client, 0x1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ret = as456x_mux_write(muxc->parent, client, 0x1, 0);
ret = as456x_mux_write(muxc->parent, client,
PCA9641_DESELECT_CHANNEL_REG, PCA9641_DESELECT_CHANNEL_VAL);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified.

packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-mux.c Outdated
switch (data->type) {
case as456x_cpldm_mux:
ret = as456x_mux_write(muxc->parent, client,
CPLD_CHANNEL_SELECT_REG, 1 << chan);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CPLD_CHANNEL_SELECT_REG, 1 << chan);
CPLD_CHANNEL_SELECT_REG, BIT(chan));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified.

packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-mux.c Outdated
ret = -EIO;

/* Try to close the mux channel */
as456x_mux_write(muxc->parent, client, 0x1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
as456x_mux_write(muxc->parent, client, 0x1, 0);
as456x_mux_write(muxc->parent, client, PCA9641_DESELECT_CHANNEL_REG, PCA9641_DESELECT_CHANNEL_VAL);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified.

packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-mux.c Outdated
Comment on lines 201 to 208
if (ret)
goto exit_mux;
}

return 0;

exit_mux:
as456x_mux_cleanup(muxc);
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid a goto here:

Suggested change
if (ret)
goto exit_mux;
}
return 0;
exit_mux:
as456x_mux_cleanup(muxc);
if (ret)
break;
}
if (ret)
as456x_mux_cleanup(muxc);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified.

@KanjiMonster
Copy link
Contributor

Since this PR depends on the devicetree file being added to the kernel, you should mention it in your description and link the appropriate PR.

KanjiMonster
KanjiMonster reviewed Oct 13, 2023
View reviewed changes
packages/platforms/accton/arm64/as456x-52p/src/modules/arm64-accton-as456x-52p-mux.c Outdated
MODULE_DEVICE_TABLE(i2c, as456x_mux_id);

static const struct of_device_id as456x_mux_of_match[] = {
{ .compatible = "edgecore,pca9641_mux", .data = &chips[pca9641_mux] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything special about this PCA9641, or could this be replaced by importing e.g. https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/#2184718 into our kernel tree (with a bit of clean-up/forward porting)?

If the latter, I would heavily suggest doing so instead, and maybe even pick up the work of submitting the support again (not necessarily by you).

Copy link

Choose a reason for hiding this comment

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

If it matters, the series that you linked seems to time out the i2c controller. I think it might have something to do with the way this mux is being used (there are two controllers attached but only one is actually powered at a time) - I didn't dive too deeply but it looks like the chip doesn't arbitrate correctly if there aren't two controllers on it. Directly programming the mux like this driver doesn't have the same effect. More investigation is probably needed to be sure but there seems to be reasonable technical reason not to use the other driver.

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 for testing this. Could you please also relay this test result to the LKML?

@brandonchuang brandonchuang changed the title [Edgecore][as4560-52p] Add new platform [Edgecore][as4560-52p][as4561-52p] Add new platform Feb 21, 2024
brandonchuang added a commit to brandonchuang/dentOS-linux that referenced this pull request Mar 20, 2024
@brandonchuang
[Edgecore] Add as4500-52p DTS
fee8f91 
CPU: Marvell 98DX3530 with integrated CPU
MAC: Marvell 98DX3530
PHY: Marvell 88E1780 x 4 (1G port 16~32)
Marvell 88E2780 x 2 (Migi-G port 33-48)
DRAM: 8GB(MAC) DDR4 SDRAM
AirFlow: Front To Back
Function port: 1 x USB port
1 x RJ45 Mgmt port
1 x RJ45 Console port
Ethernet Port: 48 x 1G
Uplink port: 4xSFP+
PoE: Microsemi PD69208M x 12 + PD69210 x 2

The DTS is for the PR:
dentproject/dentOS#285

Signed-off-by: Brandon Chuang <[email protected]>
brandonchuang added a commit to brandonchuang/dentOS-linux that referenced this pull request Mar 20, 2024
@brandonchuang
[Edgecore] Add as4581-52p DTS
1ab5319 
CPU: COMe CPU Module
MAC: Marvell 98DX3530
PHY: Marvell 88E1780 x 4 (1G port 16~32)
Marvell 88E2780 x 2 (Migi-G port 33-48)
DRAM: 8GB(MAC) DDR4 SDRAM
AirFlow: Front To Back
Function port: 1 x USB port
1 x RJ45 Mgmt port
1 x RJ45 Console port
Ethernet Port: 48 x 1G
Uplink port: 4xSFP+
PoE: Microsemi PD69208M x 12 + PD69210 x 2

The DTS is for the PR:
dentproject/dentOS#285

Signed-off-by: Brandon Chuang <[email protected]>
@brandonchuang brandonchuang mentioned this pull request Mar 20, 2024
Open
@brandonchuang brandonchuang changed the title [Edgecore][as4560-52p][as4561-52p] Add new platform [Edgecore][as4500-52p][as4581-52p] Add new platform Mar 20, 2024
@brandonchuang
Copy link
Contributor Author

Updated the PR due to the platform name changed:
as4560 -> as4500
as4561 -> as4581

brandonchuang added a commit to brandonchuang/dentOS-linux that referenced this pull request Jun 17, 2024
@brandonchuang
[Edgecore] Add as4581-52p DTS
f6ffb86 
CPU: COMe CPU Module
MAC: Marvell 98DX3530
PHY: Marvell 88E1780 x 4 (1G port 16~32)
Marvell 88E2780 x 2 (Migi-G port 33-48)
DRAM: 8GB(MAC) DDR4 SDRAM
AirFlow: Front To Back
Function port: 1 x USB port
1 x RJ45 Mgmt port
1 x RJ45 Console port
Ethernet Port: 48 x 1G
Uplink port: 4xSFP+
PoE: Microsemi PD69208M x 12 + PD69210 x 2

The DTS is for the PR:
dentproject/dentOS#285

Signed-off-by: Brandon Chuang <[email protected]>
Signed-off-by: Brandon Chuang <[email protected]>
@brandonchuang
[Edgecore][as4500-52p][as4581-52p] Add new platform
103ba8d 
CPU:
[as4500-52p] Marvell 98DX3530 with integrated CPU
[as4581-52p] COMe CPU Module

MAC: Marvell 98DX3530
PHY: Marvell 88E1780 x 4 (1G port 16~32)
     Marvell 88E2780 x 2 (Migi-G port 33-48)
DRAM: 8GB(MAC) DDR4 SDRAM
AirFlow: Front To Back
Function port: 1 x USB port
	   1 x RJ45 Mgmt port
	   1 x RJ45 Console port
Ethernet Port: 48 x 1G
Uplink port: 4xSFP+
PoE: Microsemi PD69208M x 12 + PD69210 x 2

DTS:
The DTS for as4500-52p/as4581-52p
dentproject/linux#12
dentproject/linux#11

Signed-off-by: Brandon Chuang <[email protected]>
@brandonchuang
[as4500-52p][as4581-52p] Add thermal policy
62ce694 
as4581-52p(COMe + LTE SKU):
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) + ASC10(ASC_TMON1) < 197 = 10% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) + ASC10(ASC_TMON1) > 200 = 15% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) + ASC10(ASC_TMON1) < 232 = 15% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) + ASC10(ASC_TMON1) > 235 = 25% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) + ASC10(ASC_TMON1) < 262 = 25% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) + ASC10(ASC_TMON1) > 265 = 35% FAN speed
   COMe ASC10 location is U38 (ASC_TMON1)
   One FAN failure = 50% FAN speed
   Two FAN failure = 80% FAN speed

as4500-52p(Without COMe):
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) < 196.5 = 10% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) > 199 = 20% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) < 214 = 20% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) > 217 = 30% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) < 227.5 = 30% FAN speed
   U48(0x48)+U19(0x49)+U106(0x4B)+U135(0x4C) > 230 = 35% FAN speed
   One FAN failure = 35% FAN speed
   Two FAN failure = 50% FAN speed

   Note1: 0x48, 0x49, 0x4B, 0x4C are i2c address
   Note2: Critical components with shutdown rules when it is over temperature.
   1. COMe CPU: NXP LX2080SE71826B > 105C Tj, it will be shutdown
   2. PSU G1482-0920WNA > 55C , it will be shutdwon

Signed-off-by: Brandon Chuang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulmenzel paulmenzel paulmenzel left review comments

@CosmicPenguin CosmicPenguin CosmicPenguin left review comments

@KanjiMonster KanjiMonster KanjiMonster requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brandonchuang @KanjiMonster @CosmicPenguin @paulmenzel