[PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Chunhao Lin]

rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
rtl8168h is connected to rtl8211fs utp interface. And fiber is connected
to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo pins to control
rtl8211fs mdio bus.

In this patch, driver will register a phy device which uses bitbanged MDIO
framework to access rtl8211fs.

Becuse driver only needs to set rtl8211fs phy parameters, so after setting
rtl8211fs phy parameters, it will release  phy device.

Fiber does not support speed down, so driver will not speed down phy when
system suspend or shutdown.

Driver will set mdiobb_ops owner to NULL when call alloc_mdio_bitbang().
That avoid increase module's refcount to prevent rmmod cannot be done.
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20200730100151.7490-1-ashiduka@fujitsu.com/

Signed-off-by: Chunhao Lin <hau@realtek.com>
---
v4 -> v5: register a phy device for rtl8211fs

 drivers/net/ethernet/realtek/Kconfig      |   1 +
 drivers/net/ethernet/realtek/r8169_main.c | 247 +++++++++++++++++++++-
 2 files changed, 247 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 93d9df55b361..20367114ac72 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -100,6 +100,7 @@ config R8169
 	depends on PCI
 	select FW_LOADER
 	select CRC32
+	select MDIO_BITBANG
 	select PHYLIB
 	select REALTEK_PHY
 	help
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5bc1181f829b..cd6cd64a197f 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -28,6 +28,7 @@
 #include <linux/bitfield.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
+#include <linux/mdio-bitbang.h>
 #include <asm/unaligned.h>
 #include <net/ip6_checksum.h>
 
@@ -325,6 +326,15 @@ enum rtl8168_registers {
 #define EARLY_TALLY_EN			(1 << 16)
 };
 
+enum rtl_bb_registers {
+	MDIO_IN			= 0xdc04,
+	PINOE			= 0xdc06,
+	PIN_I_SEL_1		= 0xdc08,
+	PIN_I_SEL_2		= 0xdc0A,
+	PINPU			= 0xdc18,
+	GPOUTPIN_SEL	= 0xdc20,
+};
+
 enum rtl8125_registers {
 	IntrMask_8125		= 0x38,
 	IntrStatus_8125		= 0x3c,
@@ -573,6 +583,14 @@ struct rtl8169_tc_offsets {
 	__le16	rx_missed;
 };
 
+struct rtl_bb_mask {
+	const u16 pin;
+	const u16 mdio_oe;
+	const u16 mdio;
+	const u16 mdc;
+	const u16 rb_pos;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED = 0,
 	RTL_FLAG_TASK_RESET_PENDING,
@@ -624,6 +642,12 @@ struct rtl8169_private {
 	struct rtl_fw *rtl_fw;
 
 	u32 ocp_base;
+
+	struct {
+		struct mii_bus *mii_bus;
+		struct phy_device *phydev;
+		struct bb_info *ctrl;
+	} bb;
 };
 
 typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
@@ -1199,6 +1223,217 @@ static enum rtl_dash_type rtl_check_dash(struct rtl8169_private *tp)
 	}
 }
 
+struct bb_info {
+	struct rtl8169_private *tp;
+	struct mdiobb_ctrl ctrl;
+	struct rtl_bb_mask mask;
+	u16 pinoe;
+	u16 pin_i_sel_1;
+	u16 pin_i_sel_2;
+};
+
+/* Data I/O pin control */
+static void rtl_bb_mdio_dir(struct mdiobb_ctrl *ctrl, int output)
+{
+	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
+	struct rtl8169_private *tp = bitbang->tp;
+	const u16 mask = bitbang->mask.mdio_oe;
+	const u16 reg = PINOE;
+	u16 value;
+
+	value = bitbang->pinoe;
+	if (output)
+		value |= mask;
+	else
+		value &= ~mask;
+	r8168_mac_ocp_write(tp, reg, value);
+}
+
+/* Set bit data*/
+static void rtl_bb_set_mdio(struct mdiobb_ctrl *ctrl, int set)
+{
+	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
+	struct rtl8169_private *tp = bitbang->tp;
+	const u16 mask = bitbang->mask.mdio;
+	const u16 reg = PIN_I_SEL_2;
+	u16 value;
+
+	value = bitbang->pin_i_sel_2;
+	if (set)
+		value |= mask;
+	else
+		value &= ~mask;
+	r8168_mac_ocp_write(tp, reg, value);
+}
+
+/* Get bit data*/
+static int rtl_bb_get_mdio(struct mdiobb_ctrl *ctrl)
+{
+	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
+	struct rtl8169_private *tp = bitbang->tp;
+	const u16 reg = MDIO_IN;
+
+	return (r8168_mac_ocp_read(tp, reg) & BIT(bitbang->mask.rb_pos)) != 0;
+}
+
+/* MDC pin control */
+static void rtl_bb_mdc_ctrl(struct mdiobb_ctrl *ctrl, int set)
+{
+	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
+	struct rtl8169_private *tp = bitbang->tp;
+	const u16 mask = bitbang->mask.mdc;
+	const u16 mdc_reg = PIN_I_SEL_1;
+	u16 value;
+
+	value = bitbang->pin_i_sel_1;
+	if (set)
+		value |= mask;
+	else
+		value &= ~mask;
+	r8168_mac_ocp_write(tp, mdc_reg, value);
+}
+
+/* mdio bus control struct */
+static const struct mdiobb_ops bb_ops = {
+	.owner = NULL, /* set to NULL for not increase refcount */
+	.set_mdc = rtl_bb_mdc_ctrl,
+	.set_mdio_dir = rtl_bb_mdio_dir,
+	.set_mdio_data = rtl_bb_set_mdio,
+	.get_mdio_data = rtl_bb_get_mdio,
+};
+
+static void rtl_bb_init(struct bb_info *bitbang, struct rtl_bb_mask *mask)
+{
+	struct rtl8169_private *tp = bitbang->tp;
+
+	r8168_mac_ocp_modify(tp, PINPU, mask->pin, 0);
+	r8168_mac_ocp_modify(tp, PINOE, 0, mask->pin);
+	bitbang->pinoe = r8168_mac_ocp_read(tp, PINOE);
+	bitbang->pin_i_sel_1 = r8168_mac_ocp_read(tp, PIN_I_SEL_1);
+	bitbang->pin_i_sel_2 = r8168_mac_ocp_read(tp, PIN_I_SEL_2);
+	memcpy(&bitbang->mask, mask, sizeof(struct rtl_bb_mask));
+}
+
+/* Bitbang MDIO bus init function */
+static int rtl_bb_mdio_bus_init(struct rtl8169_private *tp,
+				  struct rtl_bb_mask *mask)
+{
+	struct pci_dev *pdev = tp->pci_dev;
+	struct bb_info *bitbang;
+	struct mii_bus *new_bus;
+	int rc = 0;
+
+	/* create bit control struct for PHY */
+	bitbang = kzalloc(sizeof(struct bb_info), GFP_KERNEL);
+	if (!bitbang) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	/* bitbang init */
+	bitbang->tp = tp;
+	bitbang->ctrl.ops = &bb_ops;
+
+	/* Bitbang MII controller setting */
+	new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
+	if (!new_bus) {
+		rc = -ENOMEM;
+		goto err_bb_init_0;
+	}
+
+	new_bus->name = "r8169_bb_mii_bus";
+	new_bus->parent = &pdev->dev;
+	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-bb-%x-%x",
+		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
+
+	rtl_bb_init(bitbang, mask);
+
+	rc = mdiobus_register(new_bus);
+	if (rc)
+		goto err_bb_init_1;
+
+	tp->bb.phydev = mdiobus_get_phy(new_bus, 0);
+	if (!tp->bb.phydev) {
+		rc = -ENODEV;
+		goto err_bb_init_2;
+	} else if (!tp->bb.phydev->drv) {
+		/* Most chip versions fail with the genphy driver.
+		 * Therefore ensure that the dedicated PHY driver is loaded.
+		 */
+		rc =  -EUNATCH;
+		goto err_bb_init_2;
+	}
+
+	tp->bb.mii_bus = new_bus;
+	tp->bb.ctrl = bitbang;
+out:
+	return rc;
+
+err_bb_init_2:
+	mdiobus_unregister(new_bus);
+err_bb_init_1:
+	free_mdio_bitbang(new_bus);
+err_bb_init_0:
+	kfree(bitbang);
+	goto out;
+}
+
+static void rtl_bb_mdio_bus_release(struct rtl8169_private *tp)
+{
+	/* Unregister mdio bus */
+	mdiobus_unregister(tp->bb.mii_bus);
+
+	/* free bitbang info */
+	free_mdio_bitbang(tp->bb.mii_bus);
+
+	/* free bit control struct */
+	kfree(tp->bb.ctrl);
+}
+
+static int __rtl_check_bb_mdio_bus(struct rtl8169_private *tp)
+{
+	static struct rtl_bb_mask pin_mask_eeprom = {
+		0x0050, 0x0040, 0x000f, 0x0f00, 6};
+	static struct rtl_bb_mask pin_mask_gpo = {
+		0x0210, 0x0200, 0xf000, 0x0f00, 9};
+
+	if (!rtl_bb_mdio_bus_init(tp, &pin_mask_eeprom) ||
+		!rtl_bb_mdio_bus_init(tp, &pin_mask_gpo))
+		return 0;
+
+	return -1;
+}
+
+static bool rtl_supports_bb_mdio_bus(struct rtl8169_private *tp)
+{
+	return tp->mac_version == RTL_GIGA_MAC_VER_46 &&
+		   tp->pci_dev->revision == 0x2a;
+}
+
+static int rtl_check_bb_mdio_bus(struct rtl8169_private *tp)
+{
+	if (rtl_supports_bb_mdio_bus(tp))
+		return __rtl_check_bb_mdio_bus(tp);
+	return -1;
+}
+
+static void rtl8169_init_bb_phy(struct rtl8169_private *tp)
+{
+	struct phy_device *phydev = tp->bb.phydev;
+
+	/* disable ctap */
+	phy_modify_paged(phydev, 0x0a43, 0x19, BIT(6), 0);
+
+	/* change Rx threshold */
+	phy_modify_paged(phydev, 0x0dcc, 0x14, 0, BIT(2) | BIT(3) | BIT(4));
+
+	/* switch pin34 to PMEB pin */
+	phy_modify_paged(phydev, 0x0d40, 0x16, 0, BIT(5));
+
+	/* enable pass_linkctl_en */
+	phy_modify_paged(phydev, 0x0a4b, 0x11, 0, BIT(4));
+}
+
 static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool enable)
 {
 	switch (tp->mac_version) {
@@ -2171,6 +2406,14 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
 	    tp->pci_dev->subsystem_device == 0xe000)
 		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
 
+	if (!rtl_check_bb_mdio_bus(tp)) {
+		rtl8169_init_bb_phy(tp);
+		/* disable ctap */
+		phy_modify_paged(tp->phydev, 0x0a43, 0x11, BIT(6), 0);
+
+		rtl_bb_mdio_bus_release(tp);
+	}
+
 	/* We may have called phy_speed_down before */
 	phy_speed_up(tp->phydev);
 
@@ -2227,7 +2470,9 @@ static void rtl_prepare_power_down(struct rtl8169_private *tp)
 		rtl_ephy_write(tp, 0x19, 0xff64);
 
 	if (device_may_wakeup(tp_to_dev(tp))) {
-		phy_speed_down(tp->phydev, false);
+		/* Fiber not support speed down */
+		if (!tp->bb.mii_bus)
+			phy_speed_down(tp->phydev, false);
 		rtl_wol_enable_rx(tp);
 	}
 }
-- 
2.25.1

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Heiner Kallweit]

On 01.12.2022 15:39, Chunhao Lin wrote:
> rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
> rtl8168h is connected to rtl8211fs utp interface. And fiber is connected
> to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo pins to control
> rtl8211fs mdio bus.
> 
You mention a SFP interface. Does this mean that combination RTL8168H/RTL8211FS
needs a SFP module for fiber connectivity?
If yes, which software component is controlling the SFP I2C interface?

> In this patch, driver will register a phy device which uses bitbanged MDIO
> framework to access rtl8211fs.
> 
> Becuse driver only needs to set rtl8211fs phy parameters, so after setting
> rtl8211fs phy parameters, it will release  phy device.
> 
That's not clear to me. Starting with the most obvious questions:
How are link up/down events propagated to the MAC driver?

And when bringing down the network interface or suspending the system
I think you want to stop the PHY.

> Fiber does not support speed down, so driver will not speed down phy when
> system suspend or shutdown.
> 
See comment below at the related code change.

> Driver will set mdiobb_ops owner to NULL when call alloc_mdio_bitbang().
> That avoid increase module's refcount to prevent rmmod cannot be done.
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20200730100151.7490-1-ashiduka@fujitsu.com/
> 
> Signed-off-by: Chunhao Lin <hau@realtek.com>
> ---
> v4 -> v5: register a phy device for rtl8211fs
> 
>  drivers/net/ethernet/realtek/Kconfig      |   1 +
>  drivers/net/ethernet/realtek/r8169_main.c | 247 +++++++++++++++++++++-
>  2 files changed, 247 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
> index 93d9df55b361..20367114ac72 100644
> --- a/drivers/net/ethernet/realtek/Kconfig
> +++ b/drivers/net/ethernet/realtek/Kconfig
> @@ -100,6 +100,7 @@ config R8169
>  	depends on PCI
>  	select FW_LOADER
>  	select CRC32
> +	select MDIO_BITBANG
>  	select PHYLIB
>  	select REALTEK_PHY
>  	help
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 5bc1181f829b..cd6cd64a197f 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -28,6 +28,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/prefetch.h>
>  #include <linux/ipv6.h>
> +#include <linux/mdio-bitbang.h>
>  #include <asm/unaligned.h>
>  #include <net/ip6_checksum.h>
>  
> @@ -325,6 +326,15 @@ enum rtl8168_registers {
>  #define EARLY_TALLY_EN			(1 << 16)
>  };
>  
> +enum rtl_bb_registers {
> +	MDIO_IN			= 0xdc04,
> +	PINOE			= 0xdc06,
> +	PIN_I_SEL_1		= 0xdc08,
> +	PIN_I_SEL_2		= 0xdc0A,
> +	PINPU			= 0xdc18,
> +	GPOUTPIN_SEL	= 0xdc20,
> +};
> +
>  enum rtl8125_registers {
>  	IntrMask_8125		= 0x38,
>  	IntrStatus_8125		= 0x3c,
> @@ -573,6 +583,14 @@ struct rtl8169_tc_offsets {
>  	__le16	rx_missed;
>  };
>  
> +struct rtl_bb_mask {
> +	const u16 pin;
> +	const u16 mdio_oe;
> +	const u16 mdio;
> +	const u16 mdc;
> +	const u16 rb_pos;
> +};

It shouldn't be needed to mark every member as const.
If you don't want that a struct instance can be changed,
pass a reference as "const struct rtl_bb_mask *".

> +
>  enum rtl_flag {
>  	RTL_FLAG_TASK_ENABLED = 0,
>  	RTL_FLAG_TASK_RESET_PENDING,
> @@ -624,6 +642,12 @@ struct rtl8169_private {
>  	struct rtl_fw *rtl_fw;
>  
>  	u32 ocp_base;
> +
> +	struct {
> +		struct mii_bus *mii_bus;
> +		struct phy_device *phydev;

We have a phydev member in in struct rtl8169_private already,
why can't you use it and need a separate one instead?

> +		struct bb_info *ctrl;
> +	} bb;
>  };
>  
>  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
> @@ -1199,6 +1223,217 @@ static enum rtl_dash_type rtl_check_dash(struct rtl8169_private *tp)
>  	}
>  }
>  
> +struct bb_info {
> +	struct rtl8169_private *tp;
> +	struct mdiobb_ctrl ctrl;
> +	struct rtl_bb_mask mask;

Why not just storing a pointer to one of the two possible mask structs?

> +	u16 pinoe;
> +	u16 pin_i_sel_1;
> +	u16 pin_i_sel_2;
> +};
> +
> +/* Data I/O pin control */
> +static void rtl_bb_mdio_dir(struct mdiobb_ctrl *ctrl, int output)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 mask = bitbang->mask.mdio_oe;
> +	const u16 reg = PINOE;
> +	u16 value;
> +
> +	value = bitbang->pinoe;
> +	if (output)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, reg, value);
> +}
> +
> +/* Set bit data*/
> +static void rtl_bb_set_mdio(struct mdiobb_ctrl *ctrl, int set)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 mask = bitbang->mask.mdio;
> +	const u16 reg = PIN_I_SEL_2;
> +	u16 value;
> +
> +	value = bitbang->pin_i_sel_2;
> +	if (set)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, reg, value);
> +}
> +
> +/* Get bit data*/
> +static int rtl_bb_get_mdio(struct mdiobb_ctrl *ctrl)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 reg = MDIO_IN;
> +
> +	return (r8168_mac_ocp_read(tp, reg) & BIT(bitbang->mask.rb_pos)) != 0;
> +}
> +
> +/* MDC pin control */
> +static void rtl_bb_mdc_ctrl(struct mdiobb_ctrl *ctrl, int set)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 mask = bitbang->mask.mdc;
> +	const u16 mdc_reg = PIN_I_SEL_1;
> +	u16 value;
> +
> +	value = bitbang->pin_i_sel_1;
> +	if (set)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, mdc_reg, value);
> +}
> +
> +/* mdio bus control struct */
> +static const struct mdiobb_ops bb_ops = {
> +	.owner = NULL, /* set to NULL for not increase refcount */
> +	.set_mdc = rtl_bb_mdc_ctrl,
> +	.set_mdio_dir = rtl_bb_mdio_dir,
> +	.set_mdio_data = rtl_bb_set_mdio,
> +	.get_mdio_data = rtl_bb_get_mdio,
> +};
> +
> +static void rtl_bb_init(struct bb_info *bitbang, struct rtl_bb_mask *mask)
> +{
> +	struct rtl8169_private *tp = bitbang->tp;
> +
> +	r8168_mac_ocp_modify(tp, PINPU, mask->pin, 0);
> +	r8168_mac_ocp_modify(tp, PINOE, 0, mask->pin);

A comment would be nice explaining what these calls do.

> +	bitbang->pinoe = r8168_mac_ocp_read(tp, PINOE);
> +	bitbang->pin_i_sel_1 = r8168_mac_ocp_read(tp, PIN_I_SEL_1);
> +	bitbang->pin_i_sel_2 = r8168_mac_ocp_read(tp, PIN_I_SEL_2);
> +	memcpy(&bitbang->mask, mask, sizeof(struct rtl_bb_mask));
> +}
> +
> +/* Bitbang MDIO bus init function */
> +static int rtl_bb_mdio_bus_init(struct rtl8169_private *tp,
> +				  struct rtl_bb_mask *mask)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	struct bb_info *bitbang;
> +	struct mii_bus *new_bus;
> +	int rc = 0;
> +
> +	/* create bit control struct for PHY */
> +	bitbang = kzalloc(sizeof(struct bb_info), GFP_KERNEL);
> +	if (!bitbang) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* bitbang init */
> +	bitbang->tp = tp;
> +	bitbang->ctrl.ops = &bb_ops;
> +
> +	/* Bitbang MII controller setting */
> +	new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
> +	if (!new_bus) {
> +		rc = -ENOMEM;
> +		goto err_bb_init_0;
> +	}
> +
> +	new_bus->name = "r8169_bb_mii_bus";
> +	new_bus->parent = &pdev->dev;
> +	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-bb-%x-%x",
> +		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
> +
> +	rtl_bb_init(bitbang, mask);
> +
> +	rc = mdiobus_register(new_bus);
> +	if (rc)
> +		goto err_bb_init_1;
> +
> +	tp->bb.phydev = mdiobus_get_phy(new_bus, 0);

You check PHY address 0 only. But a PHY could be at any PHY address.

> +	if (!tp->bb.phydev) {
> +		rc = -ENODEV;
> +		goto err_bb_init_2;
> +	} else if (!tp->bb.phydev->drv) {
> +		/* Most chip versions fail with the genphy driver.
> +		 * Therefore ensure that the dedicated PHY driver is loaded.
> +		 */
> +		rc =  -EUNATCH;
> +		goto err_bb_init_2;
> +	}
> +
> +	tp->bb.mii_bus = new_bus;
> +	tp->bb.ctrl = bitbang;
> +out:
> +	return rc;
> +
> +err_bb_init_2:
> +	mdiobus_unregister(new_bus);
> +err_bb_init_1:
> +	free_mdio_bitbang(new_bus);
> +err_bb_init_0:
> +	kfree(bitbang);
> +	goto out;
> +}
> +
> +static void rtl_bb_mdio_bus_release(struct rtl8169_private *tp)
> +{
> +	/* Unregister mdio bus */
> +	mdiobus_unregister(tp->bb.mii_bus);
> +
> +	/* free bitbang info */
> +	free_mdio_bitbang(tp->bb.mii_bus);
> +
> +	/* free bit control struct */
> +	kfree(tp->bb.ctrl);
> +}
> +
> +static int __rtl_check_bb_mdio_bus(struct rtl8169_private *tp)
> +{
> +	static struct rtl_bb_mask pin_mask_eeprom = {
> +		0x0050, 0x0040, 0x000f, 0x0f00, 6};
> +	static struct rtl_bb_mask pin_mask_gpo = {
> +		0x0210, 0x0200, 0xf000, 0x0f00, 9};
> +

Would be better to use the folloing here:

static const struct rtl_bb_mask pin_mask_eeprom = {
	.pin = xxx;
	...
};

> +	if (!rtl_bb_mdio_bus_init(tp, &pin_mask_eeprom) ||
> +		!rtl_bb_mdio_bus_init(tp, &pin_mask_gpo))
> +		return 0;
> +
> +	return -1;

The caller doesn't use the errno, so you could change the
return value type to bool.

> +}
> +
> +static bool rtl_supports_bb_mdio_bus(struct rtl8169_private *tp)
> +{
> +	return tp->mac_version == RTL_GIGA_MAC_VER_46 &&
> +		   tp->pci_dev->revision == 0x2a;
> +}
> +
> +static int rtl_check_bb_mdio_bus(struct rtl8169_private *tp)
> +{
> +	if (rtl_supports_bb_mdio_bus(tp))
> +		return __rtl_check_bb_mdio_bus(tp);
> +	return -1;

Also here the return value type better would be bool.

> +}
> +
> +static void rtl8169_init_bb_phy(struct rtl8169_private *tp)
> +{
> +	struct phy_device *phydev = tp->bb.phydev;
> +
> +	/* disable ctap */
> +	phy_modify_paged(phydev, 0x0a43, 0x19, BIT(6), 0);
> +
> +	/* change Rx threshold */
> +	phy_modify_paged(phydev, 0x0dcc, 0x14, 0, BIT(2) | BIT(3) | BIT(4));
> +
> +	/* switch pin34 to PMEB pin */
> +	phy_modify_paged(phydev, 0x0d40, 0x16, 0, BIT(5));
> +
> +	/* enable pass_linkctl_en */
> +	phy_modify_paged(phydev, 0x0a4b, 0x11, 0, BIT(4));
> +}

As stated earlier, PHY initialization should be done in the PHY driver,
not in the MAC driver.

> +
>  static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool enable)
>  {
>  	switch (tp->mac_version) {
> @@ -2171,6 +2406,14 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
>  	    tp->pci_dev->subsystem_device == 0xe000)
>  		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
>  
> +	if (!rtl_check_bb_mdio_bus(tp)) {
> +		rtl8169_init_bb_phy(tp);
> +		/* disable ctap */
> +		phy_modify_paged(tp->phydev, 0x0a43, 0x11, BIT(6), 0);

See previous comment, this belongs into the PHY driver.

> +
> +		rtl_bb_mdio_bus_release(tp);
> +	}
> +
>  	/* We may have called phy_speed_down before */
>  	phy_speed_up(tp->phydev);
>  
> @@ -2227,7 +2470,9 @@ static void rtl_prepare_power_down(struct rtl8169_private *tp)
>  		rtl_ephy_write(tp, 0x19, 0xff64);
>  
>  	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> +		/* Fiber not support speed down */

phy_speed_down() will switch to the slowest mode supported by both
link partners. If the local PHY doesn't support a speed below 1Gbps
then the call to phy_speed_down() is a no-op, it won't hurt.
Therefore I see no reason to add complexity for skipping this call.

Another question may be whether switching to a slower speed is
benefitial to power consumption in general on fiber.
That's something I can't answer. But if true, we should add
to phy_speed_down() to do nothing if port is PORT_FIBRE.

> +		if (!tp->bb.mii_bus)
> +			phy_speed_down(tp->phydev, false);
>  		rtl_wol_enable_rx(tp);
>  	}
>  }

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Andrew Lunn]

> >  enum rtl_flag {
> >  	RTL_FLAG_TASK_ENABLED = 0,
> >  	RTL_FLAG_TASK_RESET_PENDING,
> > @@ -624,6 +642,12 @@ struct rtl8169_private {
> >  	struct rtl_fw *rtl_fw;
> >  
> >  	u32 ocp_base;
> > +
> > +	struct {
> > +		struct mii_bus *mii_bus;
> > +		struct phy_device *phydev;
> 
> We have a phydev member in in struct rtl8169_private already,
> why can't you use it and need a separate one instead?

There is also one in the net_device structure. That is the best one to
use, since a lot of the ethtool core code will use that.

> >  	if (device_may_wakeup(tp_to_dev(tp))) {
> > -		phy_speed_down(tp->phydev, false);
> > +		/* Fiber not support speed down */
> 
> phy_speed_down() will switch to the slowest mode supported by both
> link partners. If the local PHY doesn't support a speed below 1Gbps
> then the call to phy_speed_down() is a no-op, it won't hurt.
> Therefore I see no reason to add complexity for skipping this call.
> 
> Another question may be whether switching to a slower speed is
> benefitial to power consumption in general on fiber.
> That's something I can't answer. But if true, we should add
> to phy_speed_down() to do nothing if port is PORT_FIBRE.

If the SFP is actually a copper module, going down to 10/Half should
save power, just as it does with a normal copper PHY.

The problem with a fibre SFP is that there is no negotiation. So in
theory you might be able to save some power by changing from 2500BaseX
to 1000BaseX, but there is no way to tell the link partner you want to
drop the speed.

For this MAC driver, i think it is a moot point. Since it appears to
be using phylib, not phylink. To correctly support SFPs this MAC
driver probably needs to change to phylink.

     Andrew

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Heiner Kallweit]

On 01.12.2022 15:39, Chunhao Lin wrote:
> rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
> rtl8168h is connected to rtl8211fs utp interface. And fiber is connected
> to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo pins to control
> rtl8211fs mdio bus.
> 

I found a datasheet for RTL8211FS and it doesn't mention SFP support.
For the fiber use case it mentions RGMII for MAC/PHY connection and
SerDes for connecting the PHY to the fiber module. Is this the mode
you'd like to support?
"utp to fiber" sounds like the media converter application, and I think
that's not what we want here. So it's misleading.

> In this patch, driver will register a phy device which uses bitbanged MDIO
> framework to access rtl8211fs.
> 
> Becuse driver only needs to set rtl8211fs phy parameters, so after setting
> rtl8211fs phy parameters, it will release  phy device.
> 
> Fiber does not support speed down, so driver will not speed down phy when
> system suspend or shutdown.
> 
> Driver will set mdiobb_ops owner to NULL when call alloc_mdio_bitbang().
> That avoid increase module's refcount to prevent rmmod cannot be done.
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20200730100151.7490-1-ashiduka@fujitsu.com/
> 
> Signed-off-by: Chunhao Lin <hau@realtek.com>
> ---
> v4 -> v5: register a phy device for rtl8211fs
> 
>  drivers/net/ethernet/realtek/Kconfig      |   1 +
>  drivers/net/ethernet/realtek/r8169_main.c | 247 +++++++++++++++++++++-
>  2 files changed, 247 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
> index 93d9df55b361..20367114ac72 100644
> --- a/drivers/net/ethernet/realtek/Kconfig
> +++ b/drivers/net/ethernet/realtek/Kconfig
> @@ -100,6 +100,7 @@ config R8169
>  	depends on PCI
>  	select FW_LOADER
>  	select CRC32
> +	select MDIO_BITBANG
>  	select PHYLIB
>  	select REALTEK_PHY
>  	help
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 5bc1181f829b..cd6cd64a197f 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -28,6 +28,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/prefetch.h>
>  #include <linux/ipv6.h>
> +#include <linux/mdio-bitbang.h>
>  #include <asm/unaligned.h>
>  #include <net/ip6_checksum.h>
>  
> @@ -325,6 +326,15 @@ enum rtl8168_registers {
>  #define EARLY_TALLY_EN			(1 << 16)
>  };
>  
> +enum rtl_bb_registers {
> +	MDIO_IN			= 0xdc04,
> +	PINOE			= 0xdc06,
> +	PIN_I_SEL_1		= 0xdc08,
> +	PIN_I_SEL_2		= 0xdc0A,
> +	PINPU			= 0xdc18,
> +	GPOUTPIN_SEL	= 0xdc20,
> +};
> +
>  enum rtl8125_registers {
>  	IntrMask_8125		= 0x38,
>  	IntrStatus_8125		= 0x3c,
> @@ -573,6 +583,14 @@ struct rtl8169_tc_offsets {
>  	__le16	rx_missed;
>  };
>  
> +struct rtl_bb_mask {
> +	const u16 pin;
> +	const u16 mdio_oe;
> +	const u16 mdio;
> +	const u16 mdc;
> +	const u16 rb_pos;
> +};
> +
>  enum rtl_flag {
>  	RTL_FLAG_TASK_ENABLED = 0,
>  	RTL_FLAG_TASK_RESET_PENDING,
> @@ -624,6 +642,12 @@ struct rtl8169_private {
>  	struct rtl_fw *rtl_fw;
>  
>  	u32 ocp_base;
> +
> +	struct {
> +		struct mii_bus *mii_bus;
> +		struct phy_device *phydev;
> +		struct bb_info *ctrl;
> +	} bb;
>  };
>  
>  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
> @@ -1199,6 +1223,217 @@ static enum rtl_dash_type rtl_check_dash(struct rtl8169_private *tp)
>  	}
>  }
>  
> +struct bb_info {
> +	struct rtl8169_private *tp;
> +	struct mdiobb_ctrl ctrl;
> +	struct rtl_bb_mask mask;
> +	u16 pinoe;
> +	u16 pin_i_sel_1;
> +	u16 pin_i_sel_2;
> +};
> +
> +/* Data I/O pin control */
> +static void rtl_bb_mdio_dir(struct mdiobb_ctrl *ctrl, int output)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 mask = bitbang->mask.mdio_oe;
> +	const u16 reg = PINOE;
> +	u16 value;
> +
> +	value = bitbang->pinoe;
> +	if (output)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, reg, value);
> +}
> +
> +/* Set bit data*/
> +static void rtl_bb_set_mdio(struct mdiobb_ctrl *ctrl, int set)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 mask = bitbang->mask.mdio;
> +	const u16 reg = PIN_I_SEL_2;
> +	u16 value;
> +
> +	value = bitbang->pin_i_sel_2;
> +	if (set)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, reg, value);
> +}
> +
> +/* Get bit data*/
> +static int rtl_bb_get_mdio(struct mdiobb_ctrl *ctrl)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 reg = MDIO_IN;
> +
> +	return (r8168_mac_ocp_read(tp, reg) & BIT(bitbang->mask.rb_pos)) != 0;
> +}
> +
> +/* MDC pin control */
> +static void rtl_bb_mdc_ctrl(struct mdiobb_ctrl *ctrl, int set)
> +{
> +	struct bb_info *bitbang = container_of(ctrl, struct bb_info, ctrl);
> +	struct rtl8169_private *tp = bitbang->tp;
> +	const u16 mask = bitbang->mask.mdc;
> +	const u16 mdc_reg = PIN_I_SEL_1;
> +	u16 value;
> +
> +	value = bitbang->pin_i_sel_1;
> +	if (set)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, mdc_reg, value);
> +}
> +
> +/* mdio bus control struct */
> +static const struct mdiobb_ops bb_ops = {
> +	.owner = NULL, /* set to NULL for not increase refcount */
> +	.set_mdc = rtl_bb_mdc_ctrl,
> +	.set_mdio_dir = rtl_bb_mdio_dir,
> +	.set_mdio_data = rtl_bb_set_mdio,
> +	.get_mdio_data = rtl_bb_get_mdio,
> +};
> +
> +static void rtl_bb_init(struct bb_info *bitbang, struct rtl_bb_mask *mask)
> +{
> +	struct rtl8169_private *tp = bitbang->tp;
> +
> +	r8168_mac_ocp_modify(tp, PINPU, mask->pin, 0);
> +	r8168_mac_ocp_modify(tp, PINOE, 0, mask->pin);
> +	bitbang->pinoe = r8168_mac_ocp_read(tp, PINOE);
> +	bitbang->pin_i_sel_1 = r8168_mac_ocp_read(tp, PIN_I_SEL_1);
> +	bitbang->pin_i_sel_2 = r8168_mac_ocp_read(tp, PIN_I_SEL_2);
> +	memcpy(&bitbang->mask, mask, sizeof(struct rtl_bb_mask));
> +}
> +
> +/* Bitbang MDIO bus init function */
> +static int rtl_bb_mdio_bus_init(struct rtl8169_private *tp,
> +				  struct rtl_bb_mask *mask)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	struct bb_info *bitbang;
> +	struct mii_bus *new_bus;
> +	int rc = 0;
> +
> +	/* create bit control struct for PHY */
> +	bitbang = kzalloc(sizeof(struct bb_info), GFP_KERNEL);
> +	if (!bitbang) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* bitbang init */
> +	bitbang->tp = tp;
> +	bitbang->ctrl.ops = &bb_ops;
> +
> +	/* Bitbang MII controller setting */
> +	new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
> +	if (!new_bus) {
> +		rc = -ENOMEM;
> +		goto err_bb_init_0;
> +	}
> +
> +	new_bus->name = "r8169_bb_mii_bus";
> +	new_bus->parent = &pdev->dev;
> +	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-bb-%x-%x",
> +		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
> +
> +	rtl_bb_init(bitbang, mask);
> +
> +	rc = mdiobus_register(new_bus);
> +	if (rc)
> +		goto err_bb_init_1;
> +
> +	tp->bb.phydev = mdiobus_get_phy(new_bus, 0);
> +	if (!tp->bb.phydev) {
> +		rc = -ENODEV;
> +		goto err_bb_init_2;
> +	} else if (!tp->bb.phydev->drv) {
> +		/* Most chip versions fail with the genphy driver.
> +		 * Therefore ensure that the dedicated PHY driver is loaded.
> +		 */
> +		rc =  -EUNATCH;
> +		goto err_bb_init_2;
> +	}
> +
> +	tp->bb.mii_bus = new_bus;
> +	tp->bb.ctrl = bitbang;
> +out:
> +	return rc;
> +
> +err_bb_init_2:
> +	mdiobus_unregister(new_bus);
> +err_bb_init_1:
> +	free_mdio_bitbang(new_bus);
> +err_bb_init_0:
> +	kfree(bitbang);
> +	goto out;
> +}
> +
> +static void rtl_bb_mdio_bus_release(struct rtl8169_private *tp)
> +{
> +	/* Unregister mdio bus */
> +	mdiobus_unregister(tp->bb.mii_bus);
> +
> +	/* free bitbang info */
> +	free_mdio_bitbang(tp->bb.mii_bus);
> +
> +	/* free bit control struct */
> +	kfree(tp->bb.ctrl);
> +}
> +
> +static int __rtl_check_bb_mdio_bus(struct rtl8169_private *tp)
> +{
> +	static struct rtl_bb_mask pin_mask_eeprom = {
> +		0x0050, 0x0040, 0x000f, 0x0f00, 6};
> +	static struct rtl_bb_mask pin_mask_gpo = {
> +		0x0210, 0x0200, 0xf000, 0x0f00, 9};
> +
> +	if (!rtl_bb_mdio_bus_init(tp, &pin_mask_eeprom) ||
> +		!rtl_bb_mdio_bus_init(tp, &pin_mask_gpo))
> +		return 0;
> +
> +	return -1;
> +}
> +
> +static bool rtl_supports_bb_mdio_bus(struct rtl8169_private *tp)
> +{
> +	return tp->mac_version == RTL_GIGA_MAC_VER_46 &&
> +		   tp->pci_dev->revision == 0x2a;
> +}
> +
> +static int rtl_check_bb_mdio_bus(struct rtl8169_private *tp)
> +{
> +	if (rtl_supports_bb_mdio_bus(tp))
> +		return __rtl_check_bb_mdio_bus(tp);
> +	return -1;
> +}
> +
> +static void rtl8169_init_bb_phy(struct rtl8169_private *tp)
> +{
> +	struct phy_device *phydev = tp->bb.phydev;
> +
> +	/* disable ctap */
> +	phy_modify_paged(phydev, 0x0a43, 0x19, BIT(6), 0);
> +
> +	/* change Rx threshold */
> +	phy_modify_paged(phydev, 0x0dcc, 0x14, 0, BIT(2) | BIT(3) | BIT(4));
> +
> +	/* switch pin34 to PMEB pin */
> +	phy_modify_paged(phydev, 0x0d40, 0x16, 0, BIT(5));
> +
> +	/* enable pass_linkctl_en */
> +	phy_modify_paged(phydev, 0x0a4b, 0x11, 0, BIT(4));
> +}
> +
>  static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool enable)
>  {
>  	switch (tp->mac_version) {
> @@ -2171,6 +2406,14 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
>  	    tp->pci_dev->subsystem_device == 0xe000)
>  		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
>  
> +	if (!rtl_check_bb_mdio_bus(tp)) {
> +		rtl8169_init_bb_phy(tp);
> +		/* disable ctap */
> +		phy_modify_paged(tp->phydev, 0x0a43, 0x11, BIT(6), 0);
> +
> +		rtl_bb_mdio_bus_release(tp);
> +	}
> +
>  	/* We may have called phy_speed_down before */
>  	phy_speed_up(tp->phydev);
>  
> @@ -2227,7 +2470,9 @@ static void rtl_prepare_power_down(struct rtl8169_private *tp)
>  		rtl_ephy_write(tp, 0x19, 0xff64);
>  
>  	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> +		/* Fiber not support speed down */
> +		if (!tp->bb.mii_bus)
> +			phy_speed_down(tp->phydev, false);
>  		rtl_wol_enable_rx(tp);
>  	}
>  }

RE: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Hau]

> 
> On 01.12.2022 15:39, Chunhao Lin wrote:
> > rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
> > rtl8168h is connected to rtl8211fs utp interface. And fiber is
> > connected to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo
> > pins to control rtl8211fs mdio bus.
> >
> 
> I found a datasheet for RTL8211FS and it doesn't mention SFP support.
> For the fiber use case it mentions RGMII for MAC/PHY connection and
> SerDes for connecting the PHY to the fiber module. Is this the mode you'd
> like to support?
> "utp to fiber" sounds like the media converter application, and I think that's
> not what we want here. So it's misleading.
This application is not listed in datasheet. But it is similar to utp to fiber application. Fiber connects to rtl8211fs through SerDes interface.
rtl8168h connects to rtl8211fs through mdi interface. rtl8168h also connects to rtl8211fs mdc/mdio interface through its eeprom or gpo pins
 for controlling rtl8211fs. The link between rtl8211fs and fiber, and the link between rtl8211fs and rtl8168h should be the same.
 Driver just needs to set the link capability of rtl8168h to auto negation and rtl8211fs will propagate the link status between fiber and itself to rtl8168h. 
But rtl8168h will not know the link capability of fiber. So when system suspend, if wol is enabled, driver cannot speed down rtl8168h's phy.
Or rtl8168h cannot be waken up.

I will submit a new patch according your advice. But we are considering not to use driver(r8169) to setup rtl8211fs. So next patch maybe simpler.

------Please consider the environment before printing this e-mail.

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Heiner Kallweit]

On 07.12.2022 18:43, Hau wrote:
>>
>> On 01.12.2022 15:39, Chunhao Lin wrote:
>>> rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
>>> rtl8168h is connected to rtl8211fs utp interface. And fiber is
>>> connected to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo
>>> pins to control rtl8211fs mdio bus.
>>>
>>
>> I found a datasheet for RTL8211FS and it doesn't mention SFP support.
>> For the fiber use case it mentions RGMII for MAC/PHY connection and
>> SerDes for connecting the PHY to the fiber module. Is this the mode you'd
>> like to support?
>> "utp to fiber" sounds like the media converter application, and I think that's
>> not what we want here. So it's misleading.
> This application is not listed in datasheet. But it is similar to utp to fiber application. Fiber connects to rtl8211fs through SerDes interface.
> rtl8168h connects to rtl8211fs through mdi interface. rtl8168h also connects to rtl8211fs mdc/mdio interface through its eeprom or gpo pins
>  for controlling rtl8211fs. The link between rtl8211fs and fiber, and the link between rtl8211fs and rtl8168h should be the same.
>  Driver just needs to set the link capability of rtl8168h to auto negation and rtl8211fs will propagate the link status between fiber and itself to rtl8168h. 
> But rtl8168h will not know the link capability of fiber. So when system suspend, if wol is enabled, driver cannot speed down rtl8168h's phy.
> Or rtl8168h cannot be waken up.
> 
> I will submit a new patch according your advice. But we are considering not to use driver(r8169) to setup rtl8211fs. So next patch maybe simpler.
> 

Sounds strange that RTL8168H connects to RTL8211FS via MDI. Typically you would use RGMII here.
Is it because RTL8168H has no pins for RGMII to external PHY's?

Then my understanding would be that you do it like this:
RTL8168H MAC -> <internal RGMII> -> RTL8168H PHY -> <MDI> -> RTL8211FS -> <SerDes> -> Fiber module
   |                                                             |
    -------------------bit-banged MDIO---------------------------

Sou you would need to control both PHY's, right? Because setup wouldn't work if e.g. RTL8168H-internal PHY is powered down.
Is the RTL8211FS interrupt pin connected to RTL8168H? Or has polling to be used to get the status from RTL8211FS?

RE: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Hau]

> On 07.12.2022 18:43, Hau wrote:
> >>
> >> On 01.12.2022 15:39, Chunhao Lin wrote:
> >>> rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
> >>> rtl8168h is connected to rtl8211fs utp interface. And fiber is
> >>> connected to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo
> >>> pins to control rtl8211fs mdio bus.
> >>>
> >>
> >> I found a datasheet for RTL8211FS and it doesn't mention SFP support.
> >> For the fiber use case it mentions RGMII for MAC/PHY connection and
> >> SerDes for connecting the PHY to the fiber module. Is this the mode
> >> you'd like to support?
> >> "utp to fiber" sounds like the media converter application, and I
> >> think that's not what we want here. So it's misleading.
> > This application is not listed in datasheet. But it is similar to utp to fiber
> application. Fiber connects to rtl8211fs through SerDes interface.
> > rtl8168h connects to rtl8211fs through mdi interface. rtl8168h also
> > connects to rtl8211fs mdc/mdio interface through its eeprom or gpo pins
> for controlling rtl8211fs. The link between rtl8211fs and fiber, and the link
> between rtl8211fs and rtl8168h should be the same.
> >  Driver just needs to set the link capability of rtl8168h to auto negation and
> rtl8211fs will propagate the link status between fiber and itself to rtl8168h.
> > But rtl8168h will not know the link capability of fiber. So when system
> suspend, if wol is enabled, driver cannot speed down rtl8168h's phy.
> > Or rtl8168h cannot be waken up.
> >
> > I will submit a new patch according your advice. But we are considering not
> to use driver(r8169) to setup rtl8211fs. So next patch maybe simpler.
> >
> 
> Sounds strange that RTL8168H connects to RTL8211FS via MDI. Typically you
> would use RGMII here.
> Is it because RTL8168H has no pins for RGMII to external PHY's?
> 
> Then my understanding would be that you do it like this:
> RTL8168H MAC -> <internal RGMII> -> RTL8168H PHY -> <MDI> -> RTL8211FS -
> > <SerDes> -> Fiber module
>    |                                                             |
>     -------------------bit-banged MDIO---------------------------
> 
> Sou you would need to control both PHY's, right? Because setup wouldn't
> work if e.g. RTL8168H-internal PHY is powered down.
> Is the RTL8211FS interrupt pin connected to RTL8168H? Or has polling to be
> used to get the status from RTL8211FS?
> 
rtl8168H is an integrated Ethernet controller, it contains MAC and PHY. It has no RGMII interface to connect to external PHY.
In this application, driver r8169 controls two PHY. One is rtl8168h's PHY, another PHY is rtl8211fs.
What r8169 have to do is to enable all link capability. rtl8211fs firmware will propagate fiber's link status to rtl8168h. 
rtl8168h will know the fiber's link status from its MAC register 0x6c. This the same as before. So rtl8211fs's interrupt pin 
will not connect to rtl8168h. And rtl8168h does not have to polling the link status of rtl8211fs.

RTL8168H MAC -> <internal RGMII> -> RTL8168H PHY -> <MDI> -> RTL8211FS -> <SerDes> -> Fiber module
   |                                                                                                                                    |
    -------------------bit-banged MDIO(use eeprom or gpo pin)--------------------

Because rtl8211fs's firmware will set link capability to 100M and GIGA when fiber link is from off to on..
So when system suspend, if wol is enabled, rtl8168h will speed down to 100M(because rtl8211fs advertise 100M and giga to rtl8168h).
The link speed between rtl8168h and fiber will mismatch. That will cause wol fail.

And in the application, we also need to setup rtl8211fs. Or it may always link down.

 ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Heiner Kallweit]

On 08.12.2022 16:59, Hau wrote:
>> On 07.12.2022 18:43, Hau wrote:
>>>>
>>>> On 01.12.2022 15:39, Chunhao Lin wrote:
>>>>> rtl8168h(revid 0x2a) + rtl8211fs is for utp to fiber application.
>>>>> rtl8168h is connected to rtl8211fs utp interface. And fiber is
>>>>> connected to rtl8211fs sfp interface. rtl8168h use its eeprm or gpo
>>>>> pins to control rtl8211fs mdio bus.
>>>>>
>>>>
>>>> I found a datasheet for RTL8211FS and it doesn't mention SFP support.
>>>> For the fiber use case it mentions RGMII for MAC/PHY connection and
>>>> SerDes for connecting the PHY to the fiber module. Is this the mode
>>>> you'd like to support?
>>>> "utp to fiber" sounds like the media converter application, and I
>>>> think that's not what we want here. So it's misleading.
>>> This application is not listed in datasheet. But it is similar to utp to fiber
>> application. Fiber connects to rtl8211fs through SerDes interface.
>>> rtl8168h connects to rtl8211fs through mdi interface. rtl8168h also
>>> connects to rtl8211fs mdc/mdio interface through its eeprom or gpo pins
>> for controlling rtl8211fs. The link between rtl8211fs and fiber, and the link
>> between rtl8211fs and rtl8168h should be the same.
>>>  Driver just needs to set the link capability of rtl8168h to auto negation and
>> rtl8211fs will propagate the link status between fiber and itself to rtl8168h.
>>> But rtl8168h will not know the link capability of fiber. So when system
>> suspend, if wol is enabled, driver cannot speed down rtl8168h's phy.
>>> Or rtl8168h cannot be waken up.
>>>
>>> I will submit a new patch according your advice. But we are considering not
>> to use driver(r8169) to setup rtl8211fs. So next patch maybe simpler.
>>>
>>
>> Sounds strange that RTL8168H connects to RTL8211FS via MDI. Typically you
>> would use RGMII here.
>> Is it because RTL8168H has no pins for RGMII to external PHY's?
>>
>> Then my understanding would be that you do it like this:
>> RTL8168H MAC -> <internal RGMII> -> RTL8168H PHY -> <MDI> -> RTL8211FS -
>>> <SerDes> -> Fiber module
>>    |                                                             |
>>     -------------------bit-banged MDIO---------------------------
>>
>> Sou you would need to control both PHY's, right? Because setup wouldn't
>> work if e.g. RTL8168H-internal PHY is powered down.
>> Is the RTL8211FS interrupt pin connected to RTL8168H? Or has polling to be
>> used to get the status from RTL8211FS?
>>
> rtl8168H is an integrated Ethernet controller, it contains MAC and PHY. It has no RGMII interface to connect to external PHY.
> In this application, driver r8169 controls two PHY. One is rtl8168h's PHY, another PHY is rtl8211fs.
> What r8169 have to do is to enable all link capability. rtl8211fs firmware will propagate fiber's link status to rtl8168h. 
> rtl8168h will know the fiber's link status from its MAC register 0x6c. This the same as before. So rtl8211fs's interrupt pin 
> will not connect to rtl8168h. And rtl8168h does not have to polling the link status of rtl8211fs.
> 
> RTL8168H MAC -> <internal RGMII> -> RTL8168H PHY -> <MDI> -> RTL8211FS -> <SerDes> -> Fiber module
>    |                                                                                                                                    |
>     -------------------bit-banged MDIO(use eeprom or gpo pin)--------------------
> 
> Because rtl8211fs's firmware will set link capability to 100M and GIGA when fiber link is from off to on..
> So when system suspend, if wol is enabled, rtl8168h will speed down to 100M(because rtl8211fs advertise 100M and giga to rtl8168h).

Can't you disable 100M advertising in RTL8211FS using the standard PHY advertisement register?
This would be more straight-forward and the hack to disable speed-down isn't needed in r8169.
The described firmware behavior to enable 100M advertisement even with 1Gbps speed on fiber side
doesn't seem to make sense.

> The link speed between rtl8168h and fiber will mismatch. That will cause wol fail.
> 
> And in the application, we also need to setup rtl8211fs. Or it may always link down.
> 
>  ------Please consider the environment before printing this e-mail.

OK, I think I get a better idea of your setup.
So it seems RTL8211FS indeed acts as media converter. Link status on MDI side of RTL8211FS reflects link status on fiber/serdes side.
RTL8168H PHY has no idea whether it's connected to RJ45 magnetics or to the MDI side of a RTL8211FS.

I think for configuring RTL8211FS you have two options:
1. Extend the Realtek PHY driver to support RTL8211FS fiber mode
2. Configure RTL8211FS from userspace (phytool, mii-tool, ..). However to be able to do this you may need to add a dummy netdevice
   that RTL8211FS is attached to. When going with this option it may be better to avoid phylib taking control of RTL8211FS.
   This can be done by setting the phy_mask of the bit-banged mii_bus.

RE: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Hau]

> 
> OK, I think I get a better idea of your setup.
> So it seems RTL8211FS indeed acts as media converter. Link status on MDI
> side of RTL8211FS reflects link status on fiber/serdes side.
> RTL8168H PHY has no idea whether it's connected to RJ45 magnetics or to the
> MDI side of a RTL8211FS.
> 
> I think for configuring RTL8211FS you have two options:
> 1. Extend the Realtek PHY driver to support RTL8211FS fiber mode 2.
> Configure RTL8211FS from userspace (phytool, mii-tool, ..). However to be
> able to do this you may need to add a dummy netdevice
>    that RTL8211FS is attached to. When going with this option it may be better
> to avoid phylib taking control of RTL8211FS.
>    This can be done by setting the phy_mask of the bit-banged mii_bus.

Thanks for your advaice.
Is that possible for us to register a PHY fixup function(phy_register_fixup()) to setup rtl8211fs instead of setup it in PHY driver?

 ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Heiner Kallweit]

On 09.12.2022 16:29, Hau wrote:
>>
>> OK, I think I get a better idea of your setup.
>> So it seems RTL8211FS indeed acts as media converter. Link status on MDI
>> side of RTL8211FS reflects link status on fiber/serdes side.
>> RTL8168H PHY has no idea whether it's connected to RJ45 magnetics or to the
>> MDI side of a RTL8211FS.
>>
>> I think for configuring RTL8211FS you have two options:
>> 1. Extend the Realtek PHY driver to support RTL8211FS fiber mode 2.
>> Configure RTL8211FS from userspace (phytool, mii-tool, ..). However to be
>> able to do this you may need to add a dummy netdevice
>>    that RTL8211FS is attached to. When going with this option it may be better
>> to avoid phylib taking control of RTL8211FS.
>>    This can be done by setting the phy_mask of the bit-banged mii_bus.
> 
> Thanks for your advaice.
> Is that possible for us to register a PHY fixup function(phy_register_fixup()) to setup rtl8211fs instead of setup it in PHY driver?
> 
From where would you like to register the PHY fixup? r8169 would be the wrong place here.
There are very few drivers using a PHY fixup and AFAICS typically PHY drivers apply
fixups from the config_init callback.
Having said that, if possible I'd recommend to avoid using a PHY fixup.

RE: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Hau]

> On 09.12.2022 16:29, Hau wrote:
> >>
> >> OK, I think I get a better idea of your setup.
> >> So it seems RTL8211FS indeed acts as media converter. Link status on
> >> MDI side of RTL8211FS reflects link status on fiber/serdes side.
> >> RTL8168H PHY has no idea whether it's connected to RJ45 magnetics or
> >> to the MDI side of a RTL8211FS.
> >>
> >> I think for configuring RTL8211FS you have two options:
> >> 1. Extend the Realtek PHY driver to support RTL8211FS fiber mode 2.
> >> Configure RTL8211FS from userspace (phytool, mii-tool, ..). However
> >> to be able to do this you may need to add a dummy netdevice
> >>    that RTL8211FS is attached to. When going with this option it may
> >> be better to avoid phylib taking control of RTL8211FS.
> >>    This can be done by setting the phy_mask of the bit-banged mii_bus.
> >
> > Thanks for your advaice.
> > Is that possible for us to register a PHY fixup function(phy_register_fixup())
> to setup rtl8211fs instead of setup it in PHY driver?
> >
> From where would you like to register the PHY fixup? r8169 would be the
> wrong place here.
> There are very few drivers using a PHY fixup and AFAICS typically PHY drivers
> apply fixups from the config_init callback.
> Having said that, if possible I'd recommend to avoid using a PHY fixup.
> 
Thanks for your prompt reply. I think in next patch I will remove the rtl8211fs phy parameter setting.
And only keep non speed down patch.

 ------Please consider the environment before printing this e-mail.

Re: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Heiner Kallweit]

On 12.12.2022 15:11, Hau wrote:
>> On 09.12.2022 16:29, Hau wrote:
>>>>
>>>> OK, I think I get a better idea of your setup.
>>>> So it seems RTL8211FS indeed acts as media converter. Link status on
>>>> MDI side of RTL8211FS reflects link status on fiber/serdes side.
>>>> RTL8168H PHY has no idea whether it's connected to RJ45 magnetics or
>>>> to the MDI side of a RTL8211FS.
>>>>
>>>> I think for configuring RTL8211FS you have two options:
>>>> 1. Extend the Realtek PHY driver to support RTL8211FS fiber mode 2.
>>>> Configure RTL8211FS from userspace (phytool, mii-tool, ..). However
>>>> to be able to do this you may need to add a dummy netdevice
>>>>    that RTL8211FS is attached to. When going with this option it may
>>>> be better to avoid phylib taking control of RTL8211FS.
>>>>    This can be done by setting the phy_mask of the bit-banged mii_bus.
>>>
>>> Thanks for your advaice.
>>> Is that possible for us to register a PHY fixup function(phy_register_fixup())
>> to setup rtl8211fs instead of setup it in PHY driver?
>>>
>> From where would you like to register the PHY fixup? r8169 would be the
>> wrong place here.
>> There are very few drivers using a PHY fixup and AFAICS typically PHY drivers
>> apply fixups from the config_init callback.
>> Having said that, if possible I'd recommend to avoid using a PHY fixup.
>>
> Thanks for your prompt reply. I think in next patch I will remove the rtl8211fs phy parameter setting.
> And only keep non speed down patch.
> If there's any possibility I'd like to avoid the non speed down patch.
You would have to think also about the case that a user uses ethtool
to restrict advertisement to 100Mbps, what would break the connection.
r8169 isn't the right place for a workaround for a broken media converter.
The media converter should be fully transparent to r8169.

RTL8211FS should align the advertisement on MDI side with the link
speed on fiber side, based on SGMII in-band information.
If the fiber link is 1Gbps it must not advertise 100Mbps on MDI side.

RE: [PATCH net-next v5] r8169: add support for rtl8168h(revid 0x2a) + rtl8211fs fiber application

[Hau]

> 
> On 12.12.2022 15:11, Hau wrote:
> >> On 09.12.2022 16:29, Hau wrote:
> >>>>
> >>>> OK, I think I get a better idea of your setup.
> >>>> So it seems RTL8211FS indeed acts as media converter. Link status
> >>>> on MDI side of RTL8211FS reflects link status on fiber/serdes side.
> >>>> RTL8168H PHY has no idea whether it's connected to RJ45 magnetics
> >>>> or to the MDI side of a RTL8211FS.
> >>>>
> >>>> I think for configuring RTL8211FS you have two options:
> >>>> 1. Extend the Realtek PHY driver to support RTL8211FS fiber mode 2.
> >>>> Configure RTL8211FS from userspace (phytool, mii-tool, ..). However
> >>>> to be able to do this you may need to add a dummy netdevice
> >>>>    that RTL8211FS is attached to. When going with this option it
> >>>> may be better to avoid phylib taking control of RTL8211FS.
> >>>>    This can be done by setting the phy_mask of the bit-banged mii_bus.
> >>>
> >>> Thanks for your advaice.
> >>> Is that possible for us to register a PHY fixup
> >>> function(phy_register_fixup())
> >> to setup rtl8211fs instead of setup it in PHY driver?
> >>>
> >> From where would you like to register the PHY fixup? r8169 would be
> >> the wrong place here.
> >> There are very few drivers using a PHY fixup and AFAICS typically PHY
> >> drivers apply fixups from the config_init callback.
> >> Having said that, if possible I'd recommend to avoid using a PHY fixup.
> >>
> > Thanks for your prompt reply. I think in next patch I will remove the
> rtl8211fs phy parameter setting.
> > And only keep non speed down patch.
> > If there's any possibility I'd like to avoid the non speed down patch.
> You would have to think also about the case that a user uses ethtool to
> restrict advertisement to 100Mbps, what would break the connection.
> r8169 isn't the right place for a workaround for a broken media converter.
> The media converter should be fully transparent to r8169.
> 
> RTL8211FS should align the advertisement on MDI side with the link speed on
> fiber side, based on SGMII in-band information.
> If the fiber link is 1Gbps it must not advertise 100Mbps on MDI side.
> 
The fw in rtl8211fs will default advertise 100Mbps and 1000Mbps  on MDI side. 
We are trying to find a way to fix this issue. If we cannot fix this issue by fw than we
will submit a patch for this issue.
  
------Please consider the environment before printing this e-mail.