linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v4 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY
@ 2023-01-27 14:26 Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 1/4] net: phylink: Set host_interfaces for a non-sfp PHY Yoshihiro Shimoda
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-27 14:26 UTC (permalink / raw)
  To: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda

So, I would like to change the MACTYPE as SGMII by software for the platform.

The patch [1/4] sets phydev->host_interfaces by phylink for Marvell PHY
driver (marvell10g) to initialize the MACTYPE.

The patch [2/4] siplifies the rswitch driver, the patch [3/4] enables
the ovr_host_interfaces flag, and the patch [4/4] phy_power_on() calling
to initialize the Ethernet SERDES PHY driver (r8a779f0-eth-serdes)
for each channel.

Changes from v3:
https://lore.kernel.org/all/20230127014812.1656340-1-yoshihiro.shimoda.uh@renesas.com/
 - Keep a pointer of "port" and more simplify the code.

Changes from v2:
 - Add some blank lines for readability.

Changes from v1:
 - Add a new flag (ovr_host_interfaces) into phylink_config in the patch [1/4].
 - Add a new patch [3/4] for the new flag.
 - Add a error message to the patch [4/4/] for MLO_AN_INBAND mode.


Yoshihiro Shimoda (4):
  net: phylink: Set host_interfaces for a non-sfp PHY
  net: ethernet: renesas: rswitch: Simplify struct phy * handling
  net: ethernet: renesas: rswitch: Enable ovr_host_interfaces
  net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling

 drivers/net/ethernet/renesas/rswitch.c | 116 ++++++++-----------------
 drivers/net/ethernet/renesas/rswitch.h |   2 +
 drivers/net/phy/phylink.c              |  11 +++
 include/linux/phylink.h                |   3 +
 4 files changed, 54 insertions(+), 78 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH net-next v4 1/4] net: phylink: Set host_interfaces for a non-sfp PHY
  2023-01-27 14:26 [PATCH net-next v4 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY Yoshihiro Shimoda
@ 2023-01-27 14:26 ` Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling Yoshihiro Shimoda
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-27 14:26 UTC (permalink / raw)
  To: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda

If a new flag (ovr_host_interfaces) in the phylink_config is set,
overwrite the host_interfaces in the phy_device by link_interface.

Note that an ethernet PHY driver like marvell10g will check
PHY_INTERFACE_MODE_SGMII in the host_interfaces whther the host
controller supports a rate matching interface mode or not. So, set
PHY_INTERFACE_MODE_SGMII to the host_interfaces if it is set in
the supported_interfaces.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/phy/phylink.c | 11 +++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 319790221d7f..dee64b4a1175 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1814,6 +1814,17 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	if (pl->config->ovr_host_interfaces) {
+		__set_bit(pl->link_interface, phy_dev->host_interfaces);
+
+		/* An ethernet PHY driver will check PHY_INTERFACE_MODE_SGMII
+		 * in the host_interfaces whether the host controller supports
+		 * a rate matching interface mode or not.
+		 */
+		if (test_bit(PHY_INTERFACE_MODE_SGMII, pl->config->supported_interfaces))
+			__set_bit(PHY_INTERFACE_MODE_SGMII, phy_dev->host_interfaces);
+	}
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	if (ret) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..c8dd53b1e857 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -124,6 +124,8 @@ enum phylink_op_type {
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
+ * @ovr_host_interfaces: if true, override host_interfaces of phy_device from
+ *			 link_interface.
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
@@ -137,6 +139,7 @@ struct phylink_config {
 	bool poll_fixed_state;
 	bool mac_managed_pm;
 	bool ovr_an_inband;
+	bool ovr_host_interfaces;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net-next v4 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling
  2023-01-27 14:26 [PATCH net-next v4 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 1/4] net: phylink: Set host_interfaces for a non-sfp PHY Yoshihiro Shimoda
@ 2023-01-27 14:26 ` Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 3/4] net: ethernet: renesas: rswitch: Enable ovr_host_interfaces Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling Yoshihiro Shimoda
  3 siblings, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-27 14:26 UTC (permalink / raw)
  To: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda

Simplify struct phy *serdes handling by keeping the valiable in
the struct rswitch_device.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 103 ++++++-------------------
 drivers/net/ethernet/renesas/rswitch.h |   2 +
 2 files changed, 27 insertions(+), 78 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 14fc0af304ce..1c154018d795 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1071,32 +1071,14 @@ static struct device_node *rswitch_get_port_node(struct rswitch_device *rdev)
 	return port;
 }
 
-/* Call of_node_put(mdio) after done */
-static struct device_node *rswitch_get_mdio_node(struct rswitch_device *rdev)
-{
-	struct device_node *port, *mdio;
-
-	port = rswitch_get_port_node(rdev);
-	if (!port)
-		return NULL;
-
-	mdio = of_get_child_by_name(port, "mdio");
-	of_node_put(port);
-
-	return mdio;
-}
-
 static int rswitch_etha_get_params(struct rswitch_device *rdev)
 {
-	struct device_node *port;
 	int err;
 
-	port = rswitch_get_port_node(rdev);
-	if (!port)
+	if (!rdev->np_port)
 		return 0;	/* ignored */
 
-	err = of_get_phy_mode(port, &rdev->etha->phy_interface);
-	of_node_put(port);
+	err = of_get_phy_mode(rdev->np_port, &rdev->etha->phy_interface);
 
 	switch (rdev->etha->phy_interface) {
 	case PHY_INTERFACE_MODE_MII:
@@ -1133,7 +1115,7 @@ static int rswitch_mii_register(struct rswitch_device *rdev)
 	mii_bus->write_c45 = rswitch_etha_mii_write_c45;
 	mii_bus->parent = &rdev->priv->pdev->dev;
 
-	mdio_np = rswitch_get_mdio_node(rdev);
+	mdio_np = of_get_child_by_name(rdev->np_port, "mdio");
 	err = of_mdiobus_register(mii_bus, mdio_np);
 	if (err < 0) {
 		mdiobus_free(mii_bus);
@@ -1185,12 +1167,9 @@ static const struct phylink_mac_ops rswitch_phylink_ops = {
 
 static int rswitch_phylink_init(struct rswitch_device *rdev)
 {
-	struct device_node *port;
 	struct phylink *phylink;
-	int err;
 
-	port = rswitch_get_port_node(rdev);
-	if (!port)
+	if (!rdev->np_port)
 		return -ENODEV;
 
 	rdev->phylink_config.dev = &rdev->ndev->dev;
@@ -1199,19 +1178,14 @@ static int rswitch_phylink_init(struct rswitch_device *rdev)
 	__set_bit(PHY_INTERFACE_MODE_USXGMII, rdev->phylink_config.supported_interfaces);
 	rdev->phylink_config.mac_capabilities = MAC_100FD | MAC_1000FD | MAC_2500FD;
 
-	phylink = phylink_create(&rdev->phylink_config, &port->fwnode,
+	phylink = phylink_create(&rdev->phylink_config, &rdev->np_port->fwnode,
 				 rdev->etha->phy_interface, &rswitch_phylink_ops);
-	if (IS_ERR(phylink)) {
-		err = PTR_ERR(phylink);
-		goto out;
-	}
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
 
 	rdev->phylink = phylink;
-	err = phylink_of_phy_connect(rdev->phylink, port, rdev->etha->phy_interface);
-out:
-	of_node_put(port);
 
-	return err;
+	return phylink_of_phy_connect(rdev->phylink, rdev->np_port, rdev->etha->phy_interface);
 }
 
 static void rswitch_phylink_deinit(struct rswitch_device *rdev)
@@ -1224,47 +1198,14 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev)
 
 static int rswitch_serdes_set_params(struct rswitch_device *rdev)
 {
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
 	int err;
 
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
+	err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET,
 			       rdev->etha->phy_interface);
 	if (err < 0)
 		return err;
 
-	return phy_set_speed(serdes, rdev->etha->speed);
-}
-
-static int rswitch_serdes_init(struct rswitch_device *rdev)
-{
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
-
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	return phy_init(serdes);
-}
-
-static int rswitch_serdes_deinit(struct rswitch_device *rdev)
-{
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
-
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	return phy_exit(serdes);
+	return phy_set_speed(rdev->serdes, rdev->etha->speed);
 }
 
 static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
@@ -1286,6 +1227,12 @@ static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
 	if (err < 0)
 		goto err_phylink_init;
 
+	rdev->serdes = devm_of_phy_get(&rdev->priv->pdev->dev, rdev->np_port, NULL);
+	if (IS_ERR(rdev->serdes)) {
+		err = PTR_ERR(rdev->serdes);
+		goto err_serdes_phy_get;
+	}
+
 	err = rswitch_serdes_set_params(rdev);
 	if (err < 0)
 		goto err_serdes_set_params;
@@ -1293,6 +1240,7 @@ static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
 	return 0;
 
 err_serdes_set_params:
+err_serdes_phy_get:
 	rswitch_phylink_deinit(rdev);
 
 err_phylink_init:
@@ -1318,7 +1266,7 @@ static int rswitch_ether_port_init_all(struct rswitch_private *priv)
 	}
 
 	rswitch_for_each_enabled_port(priv, i) {
-		err = rswitch_serdes_init(priv->rdev[i]);
+		err = phy_init(priv->rdev[i]->serdes);
 		if (err)
 			goto err_serdes;
 	}
@@ -1327,7 +1275,7 @@ static int rswitch_ether_port_init_all(struct rswitch_private *priv)
 
 err_serdes:
 	rswitch_for_each_enabled_port_continue_reverse(priv, i)
-		rswitch_serdes_deinit(priv->rdev[i]);
+		phy_exit(priv->rdev[i]->serdes);
 	i = RSWITCH_NUM_PORTS;
 
 err_init_one:
@@ -1342,7 +1290,7 @@ static void rswitch_ether_port_deinit_all(struct rswitch_private *priv)
 	int i;
 
 	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
-		rswitch_serdes_deinit(priv->rdev[i]);
+		phy_exit(priv->rdev[i]->serdes);
 		rswitch_ether_port_deinit_one(priv->rdev[i]);
 	}
 }
@@ -1565,7 +1513,6 @@ static int rswitch_device_alloc(struct rswitch_private *priv, int index)
 {
 	struct platform_device *pdev = priv->pdev;
 	struct rswitch_device *rdev;
-	struct device_node *port;
 	struct net_device *ndev;
 	int err;
 
@@ -1594,10 +1541,10 @@ static int rswitch_device_alloc(struct rswitch_private *priv, int index)
 
 	netif_napi_add(ndev, &rdev->napi, rswitch_poll);
 
-	port = rswitch_get_port_node(rdev);
-	rdev->disabled = !port;
-	err = of_get_ethdev_address(port, ndev);
-	of_node_put(port);
+	rdev->np_port = rswitch_get_port_node(rdev);
+	rdev->disabled = !rdev->np_port;
+	err = of_get_ethdev_address(rdev->np_port, ndev);
+	of_node_put(rdev->np_port);
 	if (err) {
 		if (is_valid_ether_addr(rdev->etha->mac_addr))
 			eth_hw_addr_set(ndev, rdev->etha->mac_addr);
@@ -1798,7 +1745,7 @@ static void rswitch_deinit(struct rswitch_private *priv)
 	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
 		struct rswitch_device *rdev = priv->rdev[i];
 
-		rswitch_serdes_deinit(rdev);
+		phy_exit(priv->rdev[i]->serdes);
 		rswitch_ether_port_deinit_one(rdev);
 		unregister_netdev(rdev->ndev);
 		rswitch_device_free(priv, i);
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 49efb0f31c77..6ae79395006e 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -953,6 +953,8 @@ struct rswitch_device {
 
 	int port;
 	struct rswitch_etha *etha;
+	struct device_node *np_port;
+	struct phy *serdes;
 };
 
 struct rswitch_mfwd_mac_table_entry {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net-next v4 3/4] net: ethernet: renesas: rswitch: Enable ovr_host_interfaces
  2023-01-27 14:26 [PATCH net-next v4 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 1/4] net: phylink: Set host_interfaces for a non-sfp PHY Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling Yoshihiro Shimoda
@ 2023-01-27 14:26 ` Yoshihiro Shimoda
  2023-01-27 14:26 ` [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling Yoshihiro Shimoda
  3 siblings, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-27 14:26 UTC (permalink / raw)
  To: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda

Enable ovr_host_interfaces to set the host_interfaces of phy_dev
for a non-sfp PHY.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 1c154018d795..b9addbc29ef9 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1174,6 +1174,7 @@ static int rswitch_phylink_init(struct rswitch_device *rdev)
 
 	rdev->phylink_config.dev = &rdev->ndev->dev;
 	rdev->phylink_config.type = PHYLINK_NETDEV;
+	rdev->phylink_config.ovr_host_interfaces = true;
 	__set_bit(PHY_INTERFACE_MODE_SGMII, rdev->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_USXGMII, rdev->phylink_config.supported_interfaces);
 	rdev->phylink_config.mac_capabilities = MAC_100FD | MAC_1000FD | MAC_2500FD;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-27 14:26 [PATCH net-next v4 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY Yoshihiro Shimoda
                   ` (2 preceding siblings ...)
  2023-01-27 14:26 ` [PATCH net-next v4 3/4] net: ethernet: renesas: rswitch: Enable ovr_host_interfaces Yoshihiro Shimoda
@ 2023-01-27 14:26 ` Yoshihiro Shimoda
  2023-01-27 15:17   ` Russell King (Oracle)
  3 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-27 14:26 UTC (permalink / raw)
  To: linux, andrew, hkallweit1, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda

Some Ethernet PHYs (like marvell10g) will decide the host interface
mode by the media-side speed. So, the rswitch driver needs to
initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
.init() for initializing all ports and .power_on() for initializing
each port. So, add phy_power_{on,off} calling for it.

Notes that in-band mode will not work because the initialization
is not completed. So, output error message if in-band mode.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index b9addbc29ef9..7bdfcb5270c0 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1143,12 +1143,20 @@ static void rswitch_mac_config(struct phylink_config *config,
 			       unsigned int mode,
 			       const struct phylink_link_state *state)
 {
+	struct net_device *ndev = to_net_dev(config->dev);
+
+	if (mode == MLO_AN_INBAND)
+		netdev_err(ndev, "Link up/down will not work because in-band mode\n");
 }
 
 static void rswitch_mac_link_down(struct phylink_config *config,
 				  unsigned int mode,
 				  phy_interface_t interface)
 {
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct rswitch_device *rdev = netdev_priv(ndev);
+
+	phy_power_off(rdev->serdes);
 }
 
 static void rswitch_mac_link_up(struct phylink_config *config,
@@ -1156,7 +1164,11 @@ static void rswitch_mac_link_up(struct phylink_config *config,
 				phy_interface_t interface, int speed,
 				int duplex, bool tx_pause, bool rx_pause)
 {
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct rswitch_device *rdev = netdev_priv(ndev);
+
 	/* Current hardware cannot change speed at runtime */
+	phy_power_on(rdev->serdes);
 }
 
 static const struct phylink_mac_ops rswitch_phylink_ops = {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-27 14:26 ` [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling Yoshihiro Shimoda
@ 2023-01-27 15:17   ` Russell King (Oracle)
  2023-01-30  5:52     ` Yoshihiro Shimoda
  0 siblings, 1 reply; 13+ messages in thread
From: Russell King (Oracle) @ 2023-01-27 15:17 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: andrew, hkallweit1, davem, edumazet, kuba, pabeni, netdev,
	linux-renesas-soc

On Fri, Jan 27, 2023 at 11:26:21PM +0900, Yoshihiro Shimoda wrote:
> Some Ethernet PHYs (like marvell10g) will decide the host interface
> mode by the media-side speed. So, the rswitch driver needs to
> initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
> after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
> .init() for initializing all ports and .power_on() for initializing
> each port. So, add phy_power_{on,off} calling for it.

So how does this work?

88x3310 can change it's MAC facing interface according to the speed
negotiated on the media side, or it can use rate adaption mode, but
if it's not a MACSEC device, the MAC must pace its transmission
rate to that of the media side link.

The former requires one to reconfigure the interface mode in
mac_config(), which I don't see happening in this patch set.

The latter requires some kind of configuration in mac_link_up()
which I also don't see happening in this patch set.

So, I doubt this works properly.

Also, I can't see any sign of any working DT configuration for this
switch to even be able to review a use case - all there is in net-next
is the basic description of the rswitch in a .dtsi and no users. It
may be helpful if there was some visibility of its use, and why
phylink is being used in this driver - because right now with phylink's
MAC methods stubbed out in the way they are, and even after this patch
set, I see little point to this driver using phylink.

Moreover, looking at the binding document, you don't even support SFPs
or fixed link, which are really the two reasons to use phylink over
phylib.

Also, phylink only really makes sense if the methods in its _ops
structures actually do something useful, because without that there
can be no dynamic configuration of the system to suit what is
connected.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-27 15:17   ` Russell King (Oracle)
@ 2023-01-30  5:52     ` Yoshihiro Shimoda
  2023-01-30 12:15       ` Russell King (Oracle)
  0 siblings, 1 reply; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-30  5:52 UTC (permalink / raw)
  To: Russell King
  Cc: andrew, hkallweit1, davem, edumazet, kuba, pabeni, netdev,
	linux-renesas-soc

Hi Russell,

> From: Russell King, Sent: Saturday, January 28, 2023 12:18 AM
> 
> On Fri, Jan 27, 2023 at 11:26:21PM +0900, Yoshihiro Shimoda wrote:
> > Some Ethernet PHYs (like marvell10g) will decide the host interface
> > mode by the media-side speed. So, the rswitch driver needs to
> > initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
> > after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
> > .init() for initializing all ports and .power_on() for initializing
> > each port. So, add phy_power_{on,off} calling for it.
> 
> So how does this work?

This hardware has MDIO interfaces, and the MDIO can communicate the Ethernet
PHY without the Ethernet SERDES initialization. And, the Ethernet PHY can be
initialized, and media-side of the PHY works. So, this works.

> 88x3310 can change it's MAC facing interface according to the speed
> negotiated on the media side, or it can use rate adaption mode, but
> if it's not a MACSEC device, the MAC must pace its transmission
> rate to that of the media side link.

My platform has 88x2110 so that it's not a MACSEC device.

> The former requires one to reconfigure the interface mode in
> mac_config(), which I don't see happening in this patch set.

You're correct. This patch set doesn't have such reconfiguration
because this driver doesn't support such a feature (for now).

However, this driver has configured the interface mode when
driver is probing.

> The latter requires some kind of configuration in mac_link_up()
> which I also don't see happening in this patch set.

You're correct. This patch set doesn't have such configuration
in mac_link_up() because this hardware cannot change speed at
runtime (for now).

However, this driver has configured the speed when driver is
probing.

> So, I doubt this works properly.
> 
> Also, I can't see any sign of any working DT configuration for this
> switch to even be able to review a use case - all there is in net-next
> is the basic description of the rswitch in a .dtsi and no users. It
> may be helpful if there was some visibility of its use, and why
> phylink is being used in this driver - because right now with phylink's
> MAC methods stubbed out in the way they are, and even after this patch
> set, I see little point to this driver using phylink.

In the latest net-next, r8a779f0-spider.dts is a user.

In r8a779f0-spider-ether.dtsi:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider-ethernet.dtsi#n41

In r8a779f0-spider.dts:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider.dts#n10

> Moreover, looking at the binding document, you don't even support SFPs
> or fixed link, which are really the two reasons to use phylink over
> phylib.

You're correct. This hardware doesn't support SFPs or fixed link.

I sent a patch at the first, I had used phylib and had added a new function
for setting the phy_dev->host_interfaces [1]. And then, Marek suggested
that I should use phylink instead of phylib. That's why this driver
is using phylink even if this doesn't support SFPs and fixed link.

[1]
https://lore.kernel.org/netdev/20221019124100.41c9bbaf@dellmb/

> Also, phylink only really makes sense if the methods in its _ops
> structures actually do something useful, because without that there
> can be no dynamic configuration of the system to suit what is
> connected.

I think so. This rswitch doesn't need dynamic configuration,
but Marvell 88x2110 on my platform needs dynamic configuration.
That's why this driver uses phylink.

Best regards,
Yoshihiro Shimoda


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-30  5:52     ` Yoshihiro Shimoda
@ 2023-01-30 12:15       ` Russell King (Oracle)
  2023-01-30 16:30         ` Marek Behún
  2023-01-31  4:42         ` Yoshihiro Shimoda
  0 siblings, 2 replies; 13+ messages in thread
From: Russell King (Oracle) @ 2023-01-30 12:15 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Marek Behún, andrew, hkallweit1, davem, edumazet, kuba,
	pabeni, netdev, linux-renesas-soc

On Mon, Jan 30, 2023 at 05:52:15AM +0000, Yoshihiro Shimoda wrote:
> Hi Russell,
> 
> > From: Russell King, Sent: Saturday, January 28, 2023 12:18 AM
> > 
> > On Fri, Jan 27, 2023 at 11:26:21PM +0900, Yoshihiro Shimoda wrote:
> > > Some Ethernet PHYs (like marvell10g) will decide the host interface
> > > mode by the media-side speed. So, the rswitch driver needs to
> > > initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
> > > after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
> > > .init() for initializing all ports and .power_on() for initializing
> > > each port. So, add phy_power_{on,off} calling for it.
> > 
> > So how does this work?
> 
> This hardware has MDIO interfaces, and the MDIO can communicate the Ethernet
> PHY without the Ethernet SERDES initialization. And, the Ethernet PHY can be
> initialized, and media-side of the PHY works. So, this works.

Ethernet PHYs can generally be communicated with irrespective of the
serdes state, so that isn't the concern.

What I'm trying to grasp is your decision making behind putting the
serdes phy power control in the link_up/link_down functions, when
doing so is fundamentally problematical if in-band mode is ever
supported - and in-band mode has to be supported for things like
fibre connections to work.

> > 88x3310 can change it's MAC facing interface according to the speed
> > negotiated on the media side, or it can use rate adaption mode, but
> > if it's not a MACSEC device, the MAC must pace its transmission
> > rate to that of the media side link.
> 
> My platform has 88x2110 so that it's not a MACSEC device.

... which supports USXGMII, 10GBaseR, 5GBaseR, 2500BaseX and SGMII,
possibly with rate adaption, and if it's not a MACSEC device, that
rate adaption will likely require the MAC side to pace its
transmission to the media speed.

> > The former requires one to reconfigure the interface mode in
> > mac_config(), which I don't see happening in this patch set.
> 
> You're correct. This patch set doesn't have such reconfiguration
> because this driver doesn't support such a feature (for now).

Is this planned? When are we likely to see this code?

> > The latter requires some kind of configuration in mac_link_up()
> > which I also don't see happening in this patch set.
> 
> You're correct. This patch set doesn't have such configuration
> in mac_link_up() because this hardware cannot change speed at
> runtime (for now).

the hardware can't even change between the various SGMII speeds? What
kind of utterly crippled hardware implementation is this? You make it
sound like the hardware designers don't have a clue what they're doing.

> > So, I doubt this works properly.
> > 
> > Also, I can't see any sign of any working DT configuration for this
> > switch to even be able to review a use case - all there is in net-next
> > is the basic description of the rswitch in a .dtsi and no users. It
> > may be helpful if there was some visibility of its use, and why
> > phylink is being used in this driver - because right now with phylink's
> > MAC methods stubbed out in the way they are, and even after this patch
> > set, I see little point to this driver using phylink.
> 
> In the latest net-next, r8a779f0-spider.dts is a user.
> 
> In r8a779f0-spider-ether.dtsi:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider-ethernet.dtsi#n41
> 
> In r8a779f0-spider.dts:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider.dts#n10

So these configure the ports with PHYs on to use SGMII mode. No mention
of any speed, yet you say that's configured at probe time? Do you just
set them to 1G, and hope that the media side link negotiates to 1G
speeds?

That doesn't sound like a good idea to me.

> > Moreover, looking at the binding document, you don't even support SFPs
> > or fixed link, which are really the two reasons to use phylink over
> > phylib.
> 
> You're correct. This hardware doesn't support SFPs or fixed link.
> 
> I sent a patch at the first, I had used phylib and had added a new function
> for setting the phy_dev->host_interfaces [1]. And then, Marek suggested
> that I should use phylink instead of phylib. That's why this driver
> is using phylink even if this doesn't support SFPs and fixed link.
> 
`> [1]
> https://lore.kernel.org/netdev/20221019124100.41c9bbaf@dellmb/

[Adding Marek to the Cc]

I'm afraid I don't agree with Marek given the state of this driver.
His assertion is "there's an API for doing this" which is demonstrably
false. If his assertion were true, then you wouldn't need to add the
code to phylink to set phydev->host_interfaces for on-board PHYs.

I'm not particularly happy about adding that to phylink, and now that
I read your current rather poor implementation of phylink, I'm even
less happy about it.

> > Also, phylink only really makes sense if the methods in its _ops
> > structures actually do something useful, because without that there
> > can be no dynamic configuration of the system to suit what is
> > connected.
> 
> I think so. This rswitch doesn't need dynamic configuration,
> but Marvell 88x2110 on my platform needs dynamic configuration.
> That's why this driver uses phylink.

Given that you use the 88x2110, and you've set the phy-mode to
SGMII, it should support 10M, 100M and 1G speeds on the media
side. Please test - and if not, I think the code which supports
that should at the very least be part of this patch set - so we
begin to see a proper implementation in the mac_* ops.

The reason for this is I utterly detest shoddy users of phylink, and
I will ask people not to use phylink if they aren't prepared to
implement it properly - because shoddy phylink users add greatly to
my maintenance workload.

Thanks.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-30 12:15       ` Russell King (Oracle)
@ 2023-01-30 16:30         ` Marek Behún
  2023-01-30 16:48           ` Russell King (Oracle)
  2023-01-31  4:42         ` Yoshihiro Shimoda
  1 sibling, 1 reply; 13+ messages in thread
From: Marek Behún @ 2023-01-30 16:30 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Yoshihiro Shimoda, andrew, hkallweit1, davem, edumazet, kuba,
	pabeni, netdev, linux-renesas-soc

On Mon, 30 Jan 2023 12:15:33 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Jan 30, 2023 at 05:52:15AM +0000, Yoshihiro Shimoda wrote:
> > Hi Russell,
> >   
> > > From: Russell King, Sent: Saturday, January 28, 2023 12:18 AM
> > > 
> > > On Fri, Jan 27, 2023 at 11:26:21PM +0900, Yoshihiro Shimoda wrote:  
> > > > Some Ethernet PHYs (like marvell10g) will decide the host interface
> > > > mode by the media-side speed. So, the rswitch driver needs to
> > > > initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
> > > > after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
> > > > .init() for initializing all ports and .power_on() for initializing
> > > > each port. So, add phy_power_{on,off} calling for it.  
> > > 
> > > So how does this work?  
> > 
> > This hardware has MDIO interfaces, and the MDIO can communicate the Ethernet
> > PHY without the Ethernet SERDES initialization. And, the Ethernet PHY can be
> > initialized, and media-side of the PHY works. So, this works.  
> 
> Ethernet PHYs can generally be communicated with irrespective of the
> serdes state, so that isn't the concern.
> 
> What I'm trying to grasp is your decision making behind putting the
> serdes phy power control in the link_up/link_down functions, when
> doing so is fundamentally problematical if in-band mode is ever
> supported - and in-band mode has to be supported for things like
> fibre connections to work.
> 
> > > 88x3310 can change it's MAC facing interface according to the speed
> > > negotiated on the media side, or it can use rate adaption mode, but
> > > if it's not a MACSEC device, the MAC must pace its transmission
> > > rate to that of the media side link.  
> > 
> > My platform has 88x2110 so that it's not a MACSEC device.  
> 
> ... which supports USXGMII, 10GBaseR, 5GBaseR, 2500BaseX and SGMII,
> possibly with rate adaption, and if it's not a MACSEC device, that
> rate adaption will likely require the MAC side to pace its
> transmission to the media speed.
> 
> > > The former requires one to reconfigure the interface mode in
> > > mac_config(), which I don't see happening in this patch set.  
> > 
> > You're correct. This patch set doesn't have such reconfiguration
> > because this driver doesn't support such a feature (for now).  
> 
> Is this planned? When are we likely to see this code?
> 
> > > The latter requires some kind of configuration in mac_link_up()
> > > which I also don't see happening in this patch set.  
> > 
> > You're correct. This patch set doesn't have such configuration
> > in mac_link_up() because this hardware cannot change speed at
> > runtime (for now).  
> 
> the hardware can't even change between the various SGMII speeds? What
> kind of utterly crippled hardware implementation is this? You make it
> sound like the hardware designers don't have a clue what they're doing.
> 
> > > So, I doubt this works properly.
> > > 
> > > Also, I can't see any sign of any working DT configuration for this
> > > switch to even be able to review a use case - all there is in net-next
> > > is the basic description of the rswitch in a .dtsi and no users. It
> > > may be helpful if there was some visibility of its use, and why
> > > phylink is being used in this driver - because right now with phylink's
> > > MAC methods stubbed out in the way they are, and even after this patch
> > > set, I see little point to this driver using phylink.  
> > 
> > In the latest net-next, r8a779f0-spider.dts is a user.
> > 
> > In r8a779f0-spider-ether.dtsi:
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider-ethernet.dtsi#n41
> > 
> > In r8a779f0-spider.dts:
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider.dts#n10  
> 
> So these configure the ports with PHYs on to use SGMII mode. No mention
> of any speed, yet you say that's configured at probe time? Do you just
> set them to 1G, and hope that the media side link negotiates to 1G
> speeds?
> 
> That doesn't sound like a good idea to me.
> 
> > > Moreover, looking at the binding document, you don't even support SFPs
> > > or fixed link, which are really the two reasons to use phylink over
> > > phylib.  
> > 
> > You're correct. This hardware doesn't support SFPs or fixed link.
> > 
> > I sent a patch at the first, I had used phylib and had added a new function
> > for setting the phy_dev->host_interfaces [1]. And then, Marek suggested
> > that I should use phylink instead of phylib. That's why this driver
> > is using phylink even if this doesn't support SFPs and fixed link.
> > 
> `> [1]
> > https://lore.kernel.org/netdev/20221019124100.41c9bbaf@dellmb/  
> 
> [Adding Marek to the Cc]
> 
> I'm afraid I don't agree with Marek given the state of this driver.
> His assertion is "there's an API for doing this" which is demonstrably
> false. If his assertion were true, then you wouldn't need to add the
> code to phylink to set phydev->host_interfaces for on-board PHYs.
> 
> I'm not particularly happy about adding that to phylink, and now that
> I read your current rather poor implementation of phylink, I'm even
> less happy about it.

OK, it seems that I had a invalid expectation that the author wants to
implement the driver fully, for future possible devices that use
rswitch with SFP cage. If this is not the case, I guess we just have to
end up with another phylib initialization function... Blegh.

But rswitch already uses phylink, so should Yoshihiro convert it whole
back to phylib? (I am not sure how much phylink API is used, maybe it
can stay that way and the new phylib function as proposed in Yoshihiro's
previous proposal can just be added.)

Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-30 16:30         ` Marek Behún
@ 2023-01-30 16:48           ` Russell King (Oracle)
  2023-01-30 16:59             ` Marek Behún
  0 siblings, 1 reply; 13+ messages in thread
From: Russell King (Oracle) @ 2023-01-30 16:48 UTC (permalink / raw)
  To: Marek Behún
  Cc: Yoshihiro Shimoda, andrew, hkallweit1, davem, edumazet, kuba,
	pabeni, netdev, linux-renesas-soc

On Mon, Jan 30, 2023 at 05:30:48PM +0100, Marek Behún wrote:
> But rswitch already uses phylink, so should Yoshihiro convert it whole
> back to phylib? (I am not sure how much phylink API is used, maybe it
> can stay that way and the new phylib function as proposed in Yoshihiro's
> previous proposal can just be added.)

In terms of "how much phylink API is used"... well, all the phylink
ops functions are currently entirely empty. So, phylink in this case
is just being nothing more than a shim between the driver and the
corresponding phylib functions.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-30 16:48           ` Russell King (Oracle)
@ 2023-01-30 16:59             ` Marek Behún
  2023-01-31  4:42               ` Yoshihiro Shimoda
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Behún @ 2023-01-30 16:59 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Russell King (Oracle),
	andrew, hkallweit1, davem, edumazet, kuba, pabeni, netdev,
	linux-renesas-soc

On Mon, 30 Jan 2023 16:48:02 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Jan 30, 2023 at 05:30:48PM +0100, Marek Behún wrote:
> > But rswitch already uses phylink, so should Yoshihiro convert it whole
> > back to phylib? (I am not sure how much phylink API is used, maybe it
> > can stay that way and the new phylib function as proposed in Yoshihiro's
> > previous proposal can just be added.)  
> 
> In terms of "how much phylink API is used"... well, all the phylink
> ops functions are currently entirely empty. So, phylink in this case
> is just being nothing more than a shim between the driver and the
> corresponding phylib functions.
> 

Yoshihiro, sorry for this. If not for my complaints, your proposal could
already be merged (maybe). Anyway, I think the best solution would be
to implement phylink properly, even for cases that are not relevant for
your board*, but this would take a non-trivial amount of time, so
I will understand if you want to stick with phylib.

* Altough you don't use fixed-link or SFP on your board, I think it
  should be possible to test it somehow if you implemented it...
  For example, I have tested fixed-link between SOC and switch SerDes
  by configuring it in device-tree on both sides.

Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-30 12:15       ` Russell King (Oracle)
  2023-01-30 16:30         ` Marek Behún
@ 2023-01-31  4:42         ` Yoshihiro Shimoda
  1 sibling, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-31  4:42 UTC (permalink / raw)
  To: Russell King
  Cc: Marek Behún, andrew, hkallweit1, davem, edumazet, kuba,
	pabeni, netdev, linux-renesas-soc

Hi Russell,

> From: Russell King, Sent: Monday, January 30, 2023 9:16 PM
> 
> On Mon, Jan 30, 2023 at 05:52:15AM +0000, Yoshihiro Shimoda wrote:
> > Hi Russell,
> >
> > > From: Russell King, Sent: Saturday, January 28, 2023 12:18 AM
> > >
> > > On Fri, Jan 27, 2023 at 11:26:21PM +0900, Yoshihiro Shimoda wrote:
> > > > Some Ethernet PHYs (like marvell10g) will decide the host interface
> > > > mode by the media-side speed. So, the rswitch driver needs to
> > > > initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
> > > > after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
> > > > .init() for initializing all ports and .power_on() for initializing
> > > > each port. So, add phy_power_{on,off} calling for it.
> > >
> > > So how does this work?
> >
> > This hardware has MDIO interfaces, and the MDIO can communicate the Ethernet
> > PHY without the Ethernet SERDES initialization. And, the Ethernet PHY can be
> > initialized, and media-side of the PHY works. So, this works.
> 
> Ethernet PHYs can generally be communicated with irrespective of the
> serdes state, so that isn't the concern.
> 
> What I'm trying to grasp is your decision making behind putting the
> serdes phy power control in the link_up/link_down functions, when
> doing so is fundamentally problematical if in-band mode is ever
> supported - and in-band mode has to be supported for things like
> fibre connections to work.

I understood it.

> > > 88x3310 can change it's MAC facing interface according to the speed
> > > negotiated on the media side, or it can use rate adaption mode, but
> > > if it's not a MACSEC device, the MAC must pace its transmission
> > > rate to that of the media side link.
> >
> > My platform has 88x2110 so that it's not a MACSEC device.
> 
> ... which supports USXGMII, 10GBaseR, 5GBaseR, 2500BaseX and SGMII,
> possibly with rate adaption, and if it's not a MACSEC device, that
> rate adaption will likely require the MAC side to pace its
> transmission to the media speed.

I understood it.

> > > The former requires one to reconfigure the interface mode in
> > > mac_config(), which I don't see happening in this patch set.
> >
> > You're correct. This patch set doesn't have such reconfiguration
> > because this driver doesn't support such a feature (for now).
> 
> Is this planned? When are we likely to see this code?

I'm afraid, but this is not planned.

> > > The latter requires some kind of configuration in mac_link_up()
> > > which I also don't see happening in this patch set.
> >
> > You're correct. This patch set doesn't have such configuration
> > in mac_link_up() because this hardware cannot change speed at
> > runtime (for now).
> 
> the hardware can't even change between the various SGMII speeds?

Unfortunately, it's true. But, I'm sorry for my unclear explanation.
This is related to a hardware restriction which cannot changed
an internal mode if it enters "operation" mode once...
But, I heard that this restriction will be fixed on a new SoC revision.

> What
> kind of utterly crippled hardware implementation is this? You make it
> sound like the hardware designers don't have a clue what they're doing.
> 
> > > So, I doubt this works properly.
> > >
> > > Also, I can't see any sign of any working DT configuration for this
> > > switch to even be able to review a use case - all there is in net-next
> > > is the basic description of the rswitch in a .dtsi and no users. It
> > > may be helpful if there was some visibility of its use, and why
> > > phylink is being used in this driver - because right now with phylink's
> > > MAC methods stubbed out in the way they are, and even after this patch
> > > set, I see little point to this driver using phylink.
> >
> > In the latest net-next, r8a779f0-spider.dts is a user.
> >
> > In r8a779f0-spider-ether.dtsi:
> >
> <snip the URL>
> >
> > In r8a779f0-spider.dts:
> >
<snip the URL>
> 
> So these configure the ports with PHYs on to use SGMII mode. No mention
> of any speed, yet you say that's configured at probe time? Do you just
> set them to 1G, and hope that the media side link negotiates to 1G
> speeds?

You're correct.

> That doesn't sound like a good idea to me.

I agreed. So, I will fix it somehow...

> > > Moreover, looking at the binding document, you don't even support SFPs
> > > or fixed link, which are really the two reasons to use phylink over
> > > phylib.
> >
> > You're correct. This hardware doesn't support SFPs or fixed link.
> >
> > I sent a patch at the first, I had used phylib and had added a new function
> > for setting the phy_dev->host_interfaces [1]. And then, Marek suggested
> > that I should use phylink instead of phylib. That's why this driver
> > is using phylink even if this doesn't support SFPs and fixed link.
> >
> `> [1]
> >
<snip the URL>
> 
> [Adding Marek to the Cc]
> 
> I'm afraid I don't agree with Marek given the state of this driver.
> His assertion is "there's an API for doing this" which is demonstrably
> false. If his assertion were true, then you wouldn't need to add the
> code to phylink to set phydev->host_interfaces for on-board PHYs.
> 
> I'm not particularly happy about adding that to phylink, and now that
> I read your current rather poor implementation of phylink, I'm even
> less happy about it.

I understood it.

> > > Also, phylink only really makes sense if the methods in its _ops
> > > structures actually do something useful, because without that there
> > > can be no dynamic configuration of the system to suit what is
> > > connected.
> >
> > I think so. This rswitch doesn't need dynamic configuration,
> > but Marvell 88x2110 on my platform needs dynamic configuration.
> > That's why this driver uses phylink.
> 
> Given that you use the 88x2110, and you've set the phy-mode to
> SGMII, it should support 10M, 100M and 1G speeds on the media
> side. Please test - and if not, I think the code which supports
> that should at the very least be part of this patch set - so we
> begin to see a proper implementation in the mac_* ops.

I got it.

> The reason for this is I utterly detest shoddy users of phylink, and
> I will ask people not to use phylink if they aren't prepared to
> implement it properly - because shoddy phylink users add greatly to
> my maintenance workload.

I understood it. I don't want to add your maintenance workload by my patch.

Best regards,
Yoshihiro Shimoda


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling
  2023-01-30 16:59             ` Marek Behún
@ 2023-01-31  4:42               ` Yoshihiro Shimoda
  0 siblings, 0 replies; 13+ messages in thread
From: Yoshihiro Shimoda @ 2023-01-31  4:42 UTC (permalink / raw)
  To: Marek Behún
  Cc: Russell King (Oracle),
	andrew, hkallweit1, davem, edumazet, kuba, pabeni, netdev,
	linux-renesas-soc

Hi Marek,

> From: Marek Behún, Sent: Tuesday, January 31, 2023 1:59 AM
> 
> On Mon, 30 Jan 2023 16:48:02 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Mon, Jan 30, 2023 at 05:30:48PM +0100, Marek Behún wrote:
> > > But rswitch already uses phylink, so should Yoshihiro convert it whole
> > > back to phylib? (I am not sure how much phylink API is used, maybe it
> > > can stay that way and the new phylib function as proposed in Yoshihiro's
> > > previous proposal can just be added.)
> >
> > In terms of "how much phylink API is used"... well, all the phylink
> > ops functions are currently entirely empty. So, phylink in this case
> > is just being nothing more than a shim between the driver and the
> > corresponding phylib functions.
> >
> 
> Yoshihiro, sorry for this.

No warries!

> If not for my complaints, your proposal could
> already be merged (maybe). Anyway, I think the best solution would be
> to implement phylink properly, even for cases that are not relevant for
> your board*, but this would take a non-trivial amount of time, so
> I will understand if you want to stick with phylib.
> 
> * Altough you don't use fixed-link or SFP on your board, I think it
>   should be possible to test it somehow if you implemented it...
>   For example, I have tested fixed-link between SOC and switch SerDes
>   by configuring it in device-tree on both sides.

Thank you very much for your comments!
For now I'm intending to use phylib instead, because I'm thinking
that I cannot implement the in-band mode of phylink on my board.
# As you mentioned, fixed-link can be implemented, I guess.

Best regards,
Yoshihiro Shimoda

> Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-01-31  4:43 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-27 14:26 [PATCH net-next v4 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY Yoshihiro Shimoda
2023-01-27 14:26 ` [PATCH net-next v4 1/4] net: phylink: Set host_interfaces for a non-sfp PHY Yoshihiro Shimoda
2023-01-27 14:26 ` [PATCH net-next v4 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling Yoshihiro Shimoda
2023-01-27 14:26 ` [PATCH net-next v4 3/4] net: ethernet: renesas: rswitch: Enable ovr_host_interfaces Yoshihiro Shimoda
2023-01-27 14:26 ` [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling Yoshihiro Shimoda
2023-01-27 15:17   ` Russell King (Oracle)
2023-01-30  5:52     ` Yoshihiro Shimoda
2023-01-30 12:15       ` Russell King (Oracle)
2023-01-30 16:30         ` Marek Behún
2023-01-30 16:48           ` Russell King (Oracle)
2023-01-30 16:59             ` Marek Behún
2023-01-31  4:42               ` Yoshihiro Shimoda
2023-01-31  4:42         ` Yoshihiro Shimoda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).