[PATCH 1/3] phy: marvell: phy-mvebu-cp110-comphy: Rename HS-SGMMI to 2500Base-X

Comphy phy mode 0x3 is incorrectly named. It is not SGMII but rather
2500Base-X mode which runs at 3.125 Gbps speed.

Rename macro names and comments to 2500Base-X.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: eb6a1fcb53e2 ("phy: mvebu-cp110-comphy: Add SMC call support")
Fixes: c2afb2fef595 ("phy: mvebu-cp110-comphy: Rename the macro handling only Ethernet modes")
---
 drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
index 53ad127b100f..bbd6f2ad6f24 100644
--- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c
+++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c

@@ -167,7 +167,7 @@
 
 #define COMPHY_FW_MODE_SATA		0x1
 #define COMPHY_FW_MODE_SGMII		0x2 /* SGMII 1G */
-#define COMPHY_FW_MODE_HS_SGMII		0x3 /* SGMII 2.5G */
+#define COMPHY_FW_MODE_2500BASEX	0x3 /* 2500BASE-X */
 #define COMPHY_FW_MODE_USB3H		0x4
 #define COMPHY_FW_MODE_USB3D		0x5
 #define COMPHY_FW_MODE_PCIE		0x6

@@ -207,7 +207,7 @@ static const struct mvebu_comphy_conf mvebu_comphy_cp110_modes[] = {
 	/* lane 0 */
 	GEN_CONF(0, 0, PHY_MODE_PCIE, COMPHY_FW_MODE_PCIE),
 	ETH_CONF(0, 1, PHY_INTERFACE_MODE_SGMII, 0x1, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(0, 1, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(0, 1, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_2500BASEX),
 	GEN_CONF(0, 1, PHY_MODE_SATA, COMPHY_FW_MODE_SATA),
 	/* lane 1 */
 	GEN_CONF(1, 0, PHY_MODE_USB_HOST_SS, COMPHY_FW_MODE_USB3H),

@@ -215,10 +215,10 @@ static const struct mvebu_comphy_conf mvebu_comphy_cp110_modes[] = {
 	GEN_CONF(1, 0, PHY_MODE_SATA, COMPHY_FW_MODE_SATA),
 	GEN_CONF(1, 0, PHY_MODE_PCIE, COMPHY_FW_MODE_PCIE),
 	ETH_CONF(1, 2, PHY_INTERFACE_MODE_SGMII, 0x1, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(1, 2, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(1, 2, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_2500BASEX),
 	/* lane 2 */
 	ETH_CONF(2, 0, PHY_INTERFACE_MODE_SGMII, 0x1, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(2, 0, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(2, 0, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_2500BASEX),
 	ETH_CONF(2, 0, PHY_INTERFACE_MODE_RXAUI, 0x1, COMPHY_FW_MODE_RXAUI),
 	ETH_CONF(2, 0, PHY_INTERFACE_MODE_10GBASER, 0x1, COMPHY_FW_MODE_XFI),
 	GEN_CONF(2, 0, PHY_MODE_USB_HOST_SS, COMPHY_FW_MODE_USB3H),

@@ -227,26 +227,26 @@ static const struct mvebu_comphy_conf mvebu_comphy_cp110_modes[] = {
 	/* lane 3 */
 	GEN_CONF(3, 0, PHY_MODE_PCIE, COMPHY_FW_MODE_PCIE),
 	ETH_CONF(3, 1, PHY_INTERFACE_MODE_SGMII, 0x2, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(3, 1, PHY_INTERFACE_MODE_2500BASEX, 0x2, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(3, 1, PHY_INTERFACE_MODE_2500BASEX, 0x2, COMPHY_FW_MODE_2500BASEX),
 	ETH_CONF(3, 1, PHY_INTERFACE_MODE_RXAUI, 0x1, COMPHY_FW_MODE_RXAUI),
 	GEN_CONF(3, 1, PHY_MODE_USB_HOST_SS, COMPHY_FW_MODE_USB3H),
 	GEN_CONF(3, 1, PHY_MODE_SATA, COMPHY_FW_MODE_SATA),
 	/* lane 4 */
 	ETH_CONF(4, 0, PHY_INTERFACE_MODE_SGMII, 0x2, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(4, 0, PHY_INTERFACE_MODE_2500BASEX, 0x2, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(4, 0, PHY_INTERFACE_MODE_2500BASEX, 0x2, COMPHY_FW_MODE_2500BASEX),
 	ETH_CONF(4, 0, PHY_INTERFACE_MODE_10GBASER, 0x2, COMPHY_FW_MODE_XFI),
 	ETH_CONF(4, 0, PHY_INTERFACE_MODE_RXAUI, 0x2, COMPHY_FW_MODE_RXAUI),
 	GEN_CONF(4, 0, PHY_MODE_USB_DEVICE_SS, COMPHY_FW_MODE_USB3D),
 	GEN_CONF(4, 1, PHY_MODE_USB_HOST_SS, COMPHY_FW_MODE_USB3H),
 	GEN_CONF(4, 1, PHY_MODE_PCIE, COMPHY_FW_MODE_PCIE),
 	ETH_CONF(4, 1, PHY_INTERFACE_MODE_SGMII, 0x1, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(4, 1, PHY_INTERFACE_MODE_2500BASEX, -1, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(4, 1, PHY_INTERFACE_MODE_2500BASEX, -1, COMPHY_FW_MODE_2500BASEX),
 	ETH_CONF(4, 1, PHY_INTERFACE_MODE_10GBASER, -1, COMPHY_FW_MODE_XFI),
 	/* lane 5 */
 	ETH_CONF(5, 1, PHY_INTERFACE_MODE_RXAUI, 0x2, COMPHY_FW_MODE_RXAUI),
 	GEN_CONF(5, 1, PHY_MODE_SATA, COMPHY_FW_MODE_SATA),
 	ETH_CONF(5, 2, PHY_INTERFACE_MODE_SGMII, 0x1, COMPHY_FW_MODE_SGMII),
-	ETH_CONF(5, 2, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_HS_SGMII),
+	ETH_CONF(5, 2, PHY_INTERFACE_MODE_2500BASEX, 0x1, COMPHY_FW_MODE_2500BASEX),
 	GEN_CONF(5, 2, PHY_MODE_PCIE, COMPHY_FW_MODE_PCIE),
 };
 

-- 
2.20.1

Comphy phy mode 0x3 is incorrectly named. It is not SGMII but rather
2500Base-X mode which runs at 3.125 Gbps speed.

Rename macro names and comments to 2500Base-X.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support")
---
 drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
index 810f25a47632..cc534a5c4b3b 100644
--- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
+++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c

@@ -29,7 +29,7 @@
 
 #define COMPHY_FW_MODE_SATA			0x1
 #define COMPHY_FW_MODE_SGMII			0x2
-#define COMPHY_FW_MODE_HS_SGMII			0x3
+#define COMPHY_FW_MODE_2500BASEX		0x3
 #define COMPHY_FW_MODE_USB3H			0x4
 #define COMPHY_FW_MODE_USB3D			0x5
 #define COMPHY_FW_MODE_PCIE			0x6

@@ -40,7 +40,7 @@
 
 #define COMPHY_FW_SPEED_1_25G			0 /* SGMII 1G */
 #define COMPHY_FW_SPEED_2_5G			1
-#define COMPHY_FW_SPEED_3_125G			2 /* SGMII 2.5G */
+#define COMPHY_FW_SPEED_3_125G			2 /* 2500BASE-X */
 #define COMPHY_FW_SPEED_5G			3
 #define COMPHY_FW_SPEED_5_15625G		4 /* XFI 5G */
 #define COMPHY_FW_SPEED_6G			5

@@ -84,14 +84,14 @@ static const struct mvebu_a3700_comphy_conf mvebu_a3700_comphy_modes[] = {
 	MVEBU_A3700_COMPHY_CONF_ETH(0, PHY_INTERFACE_MODE_SGMII, 1,
 				    COMPHY_FW_MODE_SGMII),
 	MVEBU_A3700_COMPHY_CONF_ETH(0, PHY_INTERFACE_MODE_2500BASEX, 1,
-				    COMPHY_FW_MODE_HS_SGMII),
+				    COMPHY_FW_MODE_2500BASEX),
 	/* lane 1 */
 	MVEBU_A3700_COMPHY_CONF_GEN(1, PHY_MODE_PCIE, 0,
 				    COMPHY_FW_MODE_PCIE),
 	MVEBU_A3700_COMPHY_CONF_ETH(1, PHY_INTERFACE_MODE_SGMII, 0,
 				    COMPHY_FW_MODE_SGMII),
 	MVEBU_A3700_COMPHY_CONF_ETH(1, PHY_INTERFACE_MODE_2500BASEX, 0,
-				    COMPHY_FW_MODE_HS_SGMII),
+				    COMPHY_FW_MODE_2500BASEX),
 	/* lane 2 */
 	MVEBU_A3700_COMPHY_CONF_GEN(2, PHY_MODE_SATA, 0,
 				    COMPHY_FW_MODE_SATA),

@@ -205,7 +205,7 @@ static int mvebu_a3700_comphy_power_on(struct phy *phy)
 						 COMPHY_FW_SPEED_1_25G);
 			break;
 		case PHY_INTERFACE_MODE_2500BASEX:
-			dev_dbg(lane->dev, "set lane %d to HS SGMII mode\n",
+			dev_dbg(lane->dev, "set lane %d to 2500BASEX mode\n",
 				lane->id);
 			fw_param = COMPHY_FW_NET(fw_mode, lane->port,
 						 COMPHY_FW_SPEED_3_125G);

-- 
2.20.1

Armada 3700 does not support RXAUI, XFI and neither SFI. Remove unused
macros for these unsupported modes.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support")
---
 drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
index cc534a5c4b3b..6781488cfc58 100644
--- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c
+++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c

@@ -33,18 +33,12 @@
 #define COMPHY_FW_MODE_USB3H			0x4
 #define COMPHY_FW_MODE_USB3D			0x5
 #define COMPHY_FW_MODE_PCIE			0x6
-#define COMPHY_FW_MODE_RXAUI			0x7
-#define COMPHY_FW_MODE_XFI			0x8
-#define COMPHY_FW_MODE_SFI			0x9
 #define COMPHY_FW_MODE_USB3			0xa
 
 #define COMPHY_FW_SPEED_1_25G			0 /* SGMII 1G */
 #define COMPHY_FW_SPEED_2_5G			1
 #define COMPHY_FW_SPEED_3_125G			2 /* 2500BASE-X */
 #define COMPHY_FW_SPEED_5G			3
-#define COMPHY_FW_SPEED_5_15625G		4 /* XFI 5G */
-#define COMPHY_FW_SPEED_6G			5
-#define COMPHY_FW_SPEED_10_3125G		6 /* XFI 10G */
 #define COMPHY_FW_SPEED_MAX			0x3F
 
 #define COMPHY_FW_MODE(mode)			((mode) << 12)

-- 
2.20.1

On Fri, 27 Aug 2021 11:27:53 +0200
Pali Rohár [off-list ref] wrote:

Armada 3700 does not support RXAUI, XFI and neither SFI. Remove unused
macros for these unsupported modes.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support")

BTW I was thinking about merging some parts of these 2 drivers into
common code. Not entirely sure if I should follow, though.

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Fri, 27 Aug 2021 11:27:51 +0200 you wrote:

Comphy phy mode 0x3 is incorrectly named. It is not SGMII but rather
2500Base-X mode which runs at 3.125 Gbps speed.

Rename macro names and comments to 2500Base-X.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: eb6a1fcb53e2 ("phy: mvebu-cp110-comphy: Add SMC call support")
Fixes: c2afb2fef595 ("phy: mvebu-cp110-comphy: Rename the macro handling only Ethernet modes")

[...]

Here is the summary with links:
  - [1/3] phy: marvell: phy-mvebu-cp110-comphy: Rename HS-SGMMI to 2500Base-X
    https://git.kernel.org/netdev/net-next/c/3f141ad61745
  - [2/3] phy: marvell: phy-mvebu-a3700-comphy: Rename HS-SGMMI to 2500Base-X
    https://git.kernel.org/netdev/net-next/c/b756bbec9cdd
  - [3/3] phy: marvell: phy-mvebu-a3700-comphy: Remove unsupported modes
    https://git.kernel.org/netdev/net-next/c/0c1f5f2a5581

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Friday 27 August 2021 13:27:13 Marek Behún wrote:

On Fri, 27 Aug 2021 11:27:53 +0200
Pali Rohár [off-list ref] wrote:

quoted

Armada 3700 does not support RXAUI, XFI and neither SFI. Remove unused
macros for these unsupported modes.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 9695375a3f4a ("phy: add A3700 COMPHY support")

BTW I was thinking about merging some parts of these 2 drivers into
common code. Not entirely sure if I should follow, though.

cp110-comphy and a3700-comphy are just RPC drivers which calls SMC and
relay on firmware support which implements real work. And both uses same
RPC / SMC API. So merging drivers into one is possible.

But I do not think that it is a good idea that Linux kernel depends on
some external firmware which implements RPC API for configuring HW.

Rather kernel should implements its native drivers without dependency on
external firmware.

We know from history that kernel tried to avoid using x86 BIOS/firmware
due to bugs and all functionality (re)-implemented itself without using
firmware RPC functionality.

Kernel has already "hacks" in other drivers which are using these comphy
drivers, just because older versions of firmware do not support all
necessary functionality and upgrading this firmware is not easy step
(and sometimes even not possible; e.g. when is cryptographically
signed).

On Fri, Aug 27, 2021 at 08:25:02PM +0200, Pali Rohár wrote:

cp110-comphy and a3700-comphy are just RPC drivers which calls SMC and
relay on firmware support which implements real work. And both uses same
RPC / SMC API. So merging drivers into one is possible.

But I do not think that it is a good idea that Linux kernel depends on
some external firmware which implements RPC API for configuring HW.

Rather kernel should implements its native drivers without dependency on
external firmware.

We know from history that kernel tried to avoid using x86 BIOS/firmware
due to bugs and all functionality (re)-implemented itself without using
firmware RPC functionality.

Not really an argument in this case. We're not talking about closed
source firmware.

Kernel has already "hacks" in other drivers which are using these comphy
drivers, just because older versions of firmware do not support all
necessary functionality and upgrading this firmware is not easy step
(and sometimes even not possible; e.g. when is cryptographically
signed).

The kernel used to (and probably still does) contain code to configure
the comphys. Having worked on trying to get the 10G lanes stable on
Macchiatobin, I much prefer the existing solution where it's in the
ATF firmware. I've rebuilt the firmware several times during the course
of that.

The advantage is that fixing the setup of the COMPHY is done in one
place, and it fixes it not only for the kernel, but also for u-boot
and UEFI too. So rather than having to maintain three different
places for a particular board, we can maintain the parameters in one
place - in the ATF firmware.

The problem with the past has been that stuff gets accepted into the
kernel without the "full system" view and without regard for "should
this actually be done in the firmware". Then, when it's decided that
it really should be done in the firmware, we end up needing to keep
the old stuff in the kernel for compatibility with older firmware,
which incidentally may not be up to date.

If we were to drop the comphy setup from firmware, then we will need
a lot of additional properties in the kernel's DT and u-boot DT for
the comphy to configure it appropriately. And ACPI. I don't think
that scales very well, and is a recipe for things getting out of
step.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Friday 27 August 2021 19:33:55 Russell King (Oracle) wrote:

On Fri, Aug 27, 2021 at 08:25:02PM +0200, Pali Rohár wrote:

quoted

cp110-comphy and a3700-comphy are just RPC drivers which calls SMC and
relay on firmware support which implements real work. And both uses same
RPC / SMC API. So merging drivers into one is possible.

But I do not think that it is a good idea that Linux kernel depends on
some external firmware which implements RPC API for configuring HW.

Rather kernel should implements its native drivers without dependency on
external firmware.

We know from history that kernel tried to avoid using x86 BIOS/firmware
due to bugs and all functionality (re)-implemented itself without using
firmware RPC functionality.

Not really an argument in this case. We're not talking about closed
source firmware.

quoted

Kernel has already "hacks" in other drivers which are using these comphy
drivers, just because older versions of firmware do not support all
necessary functionality and upgrading this firmware is not easy step
(and sometimes even not possible; e.g. when is cryptographically
signed).

The kernel used to (and probably still does) contain code to configure
the comphys.

Kernel does not have code for A3700. Hence reason why there are hacks in
other drivers, like libata/ahci or usb/xhci.

Having worked on trying to get the 10G lanes stable on
Macchiatobin, I much prefer the existing solution where it's in the
ATF firmware. I've rebuilt the firmware several times during the course
of that.

In some cases rebuilt of firmware does not have to be possible (e.g.
when it it signed).

The advantage is that fixing the setup of the COMPHY is done in one
place, and it fixes it not only for the kernel, but also for u-boot

U-Boot for A3720 has its own implementation and does not use firmware
implementation (yet). So currently the only consumer of firmware
implementation is just Linux kernel.

and UEFI too. So rather than having to maintain three different
places for a particular board, we can maintain the parameters in one
place - in the ATF firmware.

Same argument can be used for any other driver which is implemented in
both bootloader and kernel... But I understand also your argument.

The problem with the past has been that stuff gets accepted into the
kernel without the "full system" view and without regard for "should
this actually be done in the firmware". Then, when it's decided that
it really should be done in the firmware, we end up needing to keep
the old stuff in the kernel for compatibility with older firmware,
which incidentally may not be up to date.

If we were to drop the comphy setup from firmware, then we will need
a lot of additional properties in the kernel's DT and u-boot DT for
the comphy to configure it appropriately. And ACPI. I don't think
that scales very well, and is a recipe for things getting out of
step.

I think that whatever is used (firmware code, kernel code, ...), DT
should always contains full HW description with all nodes, and not only
some "subset". DT should be independent of current driver / firmware
implementation.

On Fri, Aug 27, 2021 at 09:02:34PM +0200, Pali Rohár wrote:

I think that whatever is used (firmware code, kernel code, ...), DT
should always contains full HW description with all nodes, and not only
some "subset". DT should be independent of current driver / firmware
implementation.

A "full" description of the hardware settings for the comphy on Armada
8040 would be very big if we included every single setting. There are
about a thousand settings - and that is likely an under-estimate. I
know, I've a shell script that decodes around a thousand settings from
the registers for a _single_ lane, and that's incomplete.

With many of the settings not very well documented in the manual, we
would struggle to describe it sufficiently well to get it past the DT
maintainers.

So, "full hardware description" is impractical. Sorry.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!