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

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

[Chunhao Lin]

rtl8168h(revid 0x2a) + rtl8211fs is for fiber related application.
rtl8168h is connected to rtl8211fs mdio bus via its eeprom or gpio pins.
In this patch, use bitbanged MDIO framework to access rtl8211fs via
rtl8168h's eeprom or gpio pins.

Signed-off-by: Chunhao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/Kconfig      |   1 +
 drivers/net/ethernet/realtek/r8169_main.c | 289 +++++++++++++++++++++-
 2 files changed, 288 insertions(+), 2 deletions(-)

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 1b7fdb4f056b..8051cdf46f85 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>
 
@@ -344,6 +345,15 @@ enum rtl8125_registers {
 	EEE_TXIDLE_TIMER_8125	= 0x6048,
 };
 
+enum rtl8168_sfp_registers {
+	MDIO_IN			= 0xdc04,
+	PINOE			= 0xdc06,
+	PIN_I_SEL_1		= 0xdc08,
+	PIN_I_SEL_2		= 0xdc0A,
+	PINPU			= 0xdc18,
+	GPOUTPIN_SEL	= 0xdc20,
+};
+
 #define RX_VLAN_INNER_8125	BIT(22)
 #define RX_VLAN_OUTER_8125	BIT(23)
 #define RX_VLAN_8125		(RX_VLAN_INNER_8125 | RX_VLAN_OUTER_8125)
@@ -584,6 +594,24 @@ struct rtl8169_tc_offsets {
 	__le16	rx_missed;
 };
 
+struct rtl_sfp_if_info {
+	u16 mdio_oe_i;
+	u16 mdio_oe_o;
+	u16 mdio_pu;
+	u16 mdio_pd;
+	u16 mdc_pu;
+	u16 mdc_pd;
+};
+
+struct rtl_sfp_if_mask {
+	const u16 pin_mask;
+	const u16 mdio_oe_mask;
+	const u16 mdio_mask;
+	const u16 mdc_mask;
+	const u16 phy_addr;
+	const u16 rb_pos;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED = 0,
 	RTL_FLAG_TASK_RESET_PENDING,
@@ -596,6 +624,12 @@ enum rtl_dash_type {
 	RTL_DASH_EP,
 };
 
+enum rtl_sfp_if_type {
+	RTL_SFP_IF_NONE,
+	RTL_SFP_IF_EEPROM,
+	RTL_SFP_IF_GPIO,
+};
+
 struct rtl8169_private {
 	void __iomem *mmio_addr;	/* memory map physical address */
 	struct pci_dev *pci_dev;
@@ -635,6 +669,10 @@ struct rtl8169_private {
 	struct rtl_fw *rtl_fw;
 
 	u32 ocp_base;
+
+	enum rtl_sfp_if_type sfp_if_type;
+
+	struct mii_bus *mii_bus;	/* MDIO bus control */
 };
 
 typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
@@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
 	if (tp->ocp_base != OCP_STD_PHY_BASE)
 		reg -= 0x10;
 
-	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
+	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
+		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value & BMCR_PDOWN)
+			return;
+
 		rtl8168g_phy_suspend_quirk(tp, value);
+	}
 
 	r8168_phy_ocp_write(tp, tp->ocp_base + reg * 2, value);
 }
@@ -1214,6 +1256,243 @@ 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_sfp_if_mask sfp_mask;
+	u16 pinoe_value;
+	u16 pin_i_sel_1_value;
+	u16 pin_i_sel_2_value;
+};
+
+/* Data I/O pin control */
+static void rtl_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 reg = PINOE;
+	const u16 mask = bitbang->sfp_mask.mdio_oe_mask;
+	u16 value;
+
+	value = bitbang->pinoe_value;
+	if (output)
+		value |= mask;
+	else
+		value &= ~mask;
+	r8168_mac_ocp_write(tp, reg, value);
+}
+
+/* Set bit data*/
+static void rtl_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 reg = PIN_I_SEL_2;
+	const u16 mask = bitbang->sfp_mask.mdio_mask;
+	u16 value;
+
+	value = bitbang->pin_i_sel_2_value;
+	if (set)
+		value |= mask;
+	else
+		value &= ~mask;
+	r8168_mac_ocp_write(tp, reg, value);
+}
+
+/* Get bit data*/
+static int rtl_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->sfp_mask.rb_pos)) != 0;
+}
+
+/* MDC pin control */
+static void rtl_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 mdc_reg = PIN_I_SEL_1;
+	const u16 mask = bitbang->sfp_mask.mdc_mask;
+	u16 value;
+
+	value = bitbang->pin_i_sel_1_value;
+	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 = THIS_MODULE,
+	.set_mdc = rtl_mdc_ctrl,
+	.set_mdio_dir = rtl_mdio_dir,
+	.set_mdio_data = rtl_set_mdio,
+	.get_mdio_data = rtl_get_mdio,
+};
+
+#define MDIO_READ 2
+#define MDIO_WRITE 1
+/* MDIO bus init function */
+static int rtl_mdio_bitbang_init(struct rtl8169_private *tp)
+{
+	struct bb_info *bitbang;
+	struct device *d = tp_to_dev(tp);
+	struct mii_bus *new_bus;
+
+	/* create bit control struct for PHY */
+	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
+	if (!bitbang)
+		return -ENOMEM;
+
+	/* bitbang init */
+	bitbang->tp = tp;
+	bitbang->ctrl.ops = &bb_ops;
+	bitbang->ctrl.op_c22_read = MDIO_READ;
+	bitbang->ctrl.op_c22_write = MDIO_WRITE;
+
+	/* MII controller setting */
+	new_bus = devm_mdiobus_alloc(d);
+	if (!new_bus)
+		return -ENOMEM;
+
+	new_bus->read = mdiobb_read;
+	new_bus->write = mdiobb_write;
+	new_bus->priv = &bitbang->ctrl;
+
+	tp->mii_bus = new_bus;
+
+	return 0;
+}
+
+static void rtl_sfp_bitbang_init(struct rtl8169_private *tp,
+				  struct rtl_sfp_if_mask *sfp_mask)
+{
+	struct mii_bus *bus = tp->mii_bus;
+	struct bb_info *bitbang = container_of(bus->priv, struct bb_info, ctrl);
+
+	r8168_mac_ocp_modify(tp, PINPU, sfp_mask->pin_mask, 0);
+	r8168_mac_ocp_modify(tp, PINOE, 0, sfp_mask->pin_mask);
+	bitbang->pinoe_value = r8168_mac_ocp_read(tp, PINOE);
+	bitbang->pin_i_sel_1_value = r8168_mac_ocp_read(tp, PIN_I_SEL_1);
+	bitbang->pin_i_sel_2_value = r8168_mac_ocp_read(tp, PIN_I_SEL_2);
+	memcpy(&bitbang->sfp_mask, sfp_mask, sizeof(struct rtl_sfp_if_mask));
+}
+
+static void rtl_sfp_mdio_write(struct rtl8169_private *tp,
+				  u8 reg,
+				  u16 val)
+{
+	struct mii_bus *bus = tp->mii_bus;
+	struct bb_info *bitbang;
+
+	if (!bus)
+		return;
+
+	bitbang = container_of(bus->priv, struct bb_info, ctrl);
+	bus->write(bus, bitbang->sfp_mask.phy_addr, reg, val);
+}
+
+static u16 rtl_sfp_mdio_read(struct rtl8169_private *tp,
+				  u8 reg)
+{
+	struct mii_bus *bus = tp->mii_bus;
+	struct bb_info *bitbang;
+
+	if (!bus)
+		return ~0;
+
+	bitbang = container_of(bus->priv, struct bb_info, ctrl);
+
+	return bus->read(bus, bitbang->sfp_mask.phy_addr, reg);
+}
+
+static void rtl_sfp_mdio_modify(struct rtl8169_private *tp, u32 reg, u16 mask,
+				 u16 set)
+{
+	u16 data = rtl_sfp_mdio_read(tp, reg);
+
+	rtl_sfp_mdio_write(tp, reg, (data & ~mask) | set);
+}
+
+#define RTL8211FS_PHY_ID_1 0x001c
+#define RTL8211FS_PHY_ID_2 0xc916
+
+static enum rtl_sfp_if_type rtl8168h_check_sfp(struct rtl8169_private *tp)
+{
+	int i;
+	int const checkcnt = 4;
+	static struct rtl_sfp_if_mask rtl_sfp_if_eeprom_mask = {
+		0x0050, 0x0040, 0x000f, 0x0f00, 0, 6};
+	static struct rtl_sfp_if_mask rtl_sfp_if_gpo_mask = {
+		0x0210, 0x0200, 0xf000, 0x0f00, 1, 9};
+
+	if (rtl_mdio_bitbang_init(tp))
+		return RTL_SFP_IF_NONE;
+
+	rtl_sfp_bitbang_init(tp, &rtl_sfp_if_eeprom_mask);
+	rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
+	for (i = 0; i < checkcnt; i++) {
+		if (rtl_sfp_mdio_read(tp, MII_PHYSID1) != RTL8211FS_PHY_ID_1 ||
+			rtl_sfp_mdio_read(tp, MII_PHYSID2) != RTL8211FS_PHY_ID_2)
+			break;
+	}
+
+	if (i == checkcnt)
+		return RTL_SFP_IF_EEPROM;
+
+	rtl_sfp_bitbang_init(tp, &rtl_sfp_if_gpo_mask);
+	rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
+	for (i = 0; i < checkcnt; i++) {
+		if (rtl_sfp_mdio_read(tp, MII_PHYSID1) != RTL8211FS_PHY_ID_1 ||
+			rtl_sfp_mdio_read(tp, MII_PHYSID2) != RTL8211FS_PHY_ID_2)
+			break;
+	}
+
+	if (i == checkcnt)
+		return RTL_SFP_IF_GPIO;
+
+	return RTL_SFP_IF_NONE;
+}
+
+static enum rtl_sfp_if_type rtl_check_sfp(struct rtl8169_private *tp)
+{
+	switch (tp->mac_version) {
+	case RTL_GIGA_MAC_VER_45:
+	case RTL_GIGA_MAC_VER_46:
+		if (tp->pci_dev->revision == 0x2a)
+			return rtl8168h_check_sfp(tp);
+		else
+			return RTL_SFP_IF_NONE;
+	default:
+		return RTL_SFP_IF_NONE;
+	}
+}
+
+static void rtl_hw_sfp_phy_config(struct rtl8169_private *tp)
+{
+	/* disable ctap */
+	rtl_sfp_mdio_write(tp, 0x1f, 0x0a43);
+	rtl_sfp_mdio_modify(tp, 0x19, BIT(6), 0);
+
+	/* change Rx threshold */
+	rtl_sfp_mdio_write(tp, 0x1f, 0x0dcc);
+	rtl_sfp_mdio_modify(tp, 0x14, 0, BIT(2) | BIT(3) | BIT(4));
+
+	/* switch pin34 to PMEB pin */
+	rtl_sfp_mdio_write(tp, 0x1f, 0x0d40);
+	rtl_sfp_mdio_modify(tp, 0x16, 0, BIT(5));
+
+	rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
+
+	/* disable ctap */
+	phy_modify_paged(tp->phydev, 0x0a43, 0x11, BIT(6), 0);
+}
+
 static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool enable)
 {
 	switch (tp->mac_version) {
@@ -2195,6 +2474,9 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
 	    tp->pci_dev->subsystem_device == 0xe000)
 		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
 
+	if (tp->sfp_if_type != RTL_SFP_IF_NONE)
+		rtl_hw_sfp_phy_config(tp);
+
 	/* We may have called phy_speed_down before */
 	phy_speed_up(tp->phydev);
 
@@ -2251,7 +2533,8 @@ 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);
+		if (tp->sfp_if_type == RTL_SFP_IF_NONE)
+			phy_speed_down(tp->phydev, false);
 		rtl_wol_enable_rx(tp);
 	}
 }
@@ -5386,6 +5669,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	tp->dash_type = rtl_check_dash(tp);
 
+	tp->sfp_if_type = rtl_check_sfp(tp);
+
 	tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
 
 	if (sizeof(dma_addr_t) > 4 && tp->mac_version >= RTL_GIGA_MAC_VER_18 &&
-- 
2.25.1

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

[Andrew Lunn]

> @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
>  	if (tp->ocp_base != OCP_STD_PHY_BASE)
>  		reg -= 0x10;
>  
> -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value & BMCR_PDOWN)
> +			return;
> +

Please could you explain this change.

> +/* Data I/O pin control */
> +static void rtl_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 reg = PINOE;
> +	const u16 mask = bitbang->sfp_mask.mdio_oe_mask;
> +	u16 value;

Reverse christmas tree please. Please sort this, longest first.

> +/* MDIO bus init function */
> +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp)
> +{
> +	struct bb_info *bitbang;
> +	struct device *d = tp_to_dev(tp);
> +	struct mii_bus *new_bus;
> +
> +	/* create bit control struct for PHY */
> +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> +	if (!bitbang)
> +		return -ENOMEM;
> +
> +	/* bitbang init */
> +	bitbang->tp = tp;
> +	bitbang->ctrl.ops = &bb_ops;
> +	bitbang->ctrl.op_c22_read = MDIO_READ;
> +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> +
> +	/* MII controller setting */
> +	new_bus = devm_mdiobus_alloc(d);
> +	if (!new_bus)
> +		return -ENOMEM;
> +
> +	new_bus->read = mdiobb_read;
> +	new_bus->write = mdiobb_write;
> +	new_bus->priv = &bitbang->ctrl;

Please use alloc_mdio_bitbang().

> +static u16 rtl_sfp_mdio_read(struct rtl8169_private *tp,
> +				  u8 reg)
> +{
> +	struct mii_bus *bus = tp->mii_bus;
> +	struct bb_info *bitbang;
> +
> +	if (!bus)
> +		return ~0;
> +
> +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> +
> +	return bus->read(bus, bitbang->sfp_mask.phy_addr, reg);

By doing this, you are bypassing all the locking. You don't normally
need operations like this. When you register the MDIO bus to the core,
it will go find any PHYs on the bus. You can then use phylib to access
the PHY. A MAC accessing the PHY is generally wrong.

    Andrew

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

[Heiner Kallweit]

On 22.08.2022 18:07, Chunhao Lin wrote:
> rtl8168h(revid 0x2a) + rtl8211fs is for fiber related application.
> rtl8168h is connected to rtl8211fs mdio bus via its eeprom or gpio pins.
> In this patch, use bitbanged MDIO framework to access rtl8211fs via
> rtl8168h's eeprom or gpio pins.
> 
> Signed-off-by: Chunhao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/Kconfig      |   1 +
>  drivers/net/ethernet/realtek/r8169_main.c | 289 +++++++++++++++++++++-
>  2 files changed, 288 insertions(+), 2 deletions(-)
> 
> 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 1b7fdb4f056b..8051cdf46f85 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>
>  
> @@ -344,6 +345,15 @@ enum rtl8125_registers {
>  	EEE_TXIDLE_TIMER_8125	= 0x6048,
>  };
>  
> +enum rtl8168_sfp_registers {
> +	MDIO_IN			= 0xdc04,
> +	PINOE			= 0xdc06,
> +	PIN_I_SEL_1		= 0xdc08,
> +	PIN_I_SEL_2		= 0xdc0A,
> +	PINPU			= 0xdc18,
> +	GPOUTPIN_SEL	= 0xdc20,
> +};
> +
>  #define RX_VLAN_INNER_8125	BIT(22)
>  #define RX_VLAN_OUTER_8125	BIT(23)
>  #define RX_VLAN_8125		(RX_VLAN_INNER_8125 | RX_VLAN_OUTER_8125)
> @@ -584,6 +594,24 @@ struct rtl8169_tc_offsets {
>  	__le16	rx_missed;
>  };
>  
> +struct rtl_sfp_if_info {
> +	u16 mdio_oe_i;
> +	u16 mdio_oe_o;
> +	u16 mdio_pu;
> +	u16 mdio_pd;
> +	u16 mdc_pu;
> +	u16 mdc_pd;
> +};
> +
> +struct rtl_sfp_if_mask {
> +	const u16 pin_mask;
> +	const u16 mdio_oe_mask;
> +	const u16 mdio_mask;
> +	const u16 mdc_mask;
> +	const u16 phy_addr;
> +	const u16 rb_pos;
> +};
> +
>  enum rtl_flag {
>  	RTL_FLAG_TASK_ENABLED = 0,
>  	RTL_FLAG_TASK_RESET_PENDING,
> @@ -596,6 +624,12 @@ enum rtl_dash_type {
>  	RTL_DASH_EP,
>  };
>  
> +enum rtl_sfp_if_type {
> +	RTL_SFP_IF_NONE,
> +	RTL_SFP_IF_EEPROM,
> +	RTL_SFP_IF_GPIO,
> +};
> +
>  struct rtl8169_private {
>  	void __iomem *mmio_addr;	/* memory map physical address */
>  	struct pci_dev *pci_dev;
> @@ -635,6 +669,10 @@ struct rtl8169_private {
>  	struct rtl_fw *rtl_fw;
>  
>  	u32 ocp_base;
> +
> +	enum rtl_sfp_if_type sfp_if_type;
> +
> +	struct mii_bus *mii_bus;	/* MDIO bus control */
>  };
>  
>  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
> @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
>  	if (tp->ocp_base != OCP_STD_PHY_BASE)
>  		reg -= 0x10;
>  
> -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value & BMCR_PDOWN)
> +			return;
> +
>  		rtl8168g_phy_suspend_quirk(tp, value);
> +	}
>  
>  	r8168_phy_ocp_write(tp, tp->ocp_base + reg * 2, value);
>  }
> @@ -1214,6 +1256,243 @@ 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_sfp_if_mask sfp_mask;
> +	u16 pinoe_value;
> +	u16 pin_i_sel_1_value;
> +	u16 pin_i_sel_2_value;
> +};
> +
> +/* Data I/O pin control */
> +static void rtl_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 reg = PINOE;
> +	const u16 mask = bitbang->sfp_mask.mdio_oe_mask;
> +	u16 value;
> +
> +	value = bitbang->pinoe_value;
> +	if (output)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, reg, value);
> +}
> +
> +/* Set bit data*/
> +static void rtl_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 reg = PIN_I_SEL_2;
> +	const u16 mask = bitbang->sfp_mask.mdio_mask;
> +	u16 value;
> +
> +	value = bitbang->pin_i_sel_2_value;
> +	if (set)
> +		value |= mask;
> +	else
> +		value &= ~mask;
> +	r8168_mac_ocp_write(tp, reg, value);
> +}
> +
> +/* Get bit data*/
> +static int rtl_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->sfp_mask.rb_pos)) != 0;
> +}
> +
> +/* MDC pin control */
> +static void rtl_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 mdc_reg = PIN_I_SEL_1;
> +	const u16 mask = bitbang->sfp_mask.mdc_mask;
> +	u16 value;
> +
> +	value = bitbang->pin_i_sel_1_value;
> +	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 = THIS_MODULE,
> +	.set_mdc = rtl_mdc_ctrl,
> +	.set_mdio_dir = rtl_mdio_dir,
> +	.set_mdio_data = rtl_set_mdio,
> +	.get_mdio_data = rtl_get_mdio,
> +};
> +
> +#define MDIO_READ 2
> +#define MDIO_WRITE 1
> +/* MDIO bus init function */
> +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp)
> +{
> +	struct bb_info *bitbang;
> +	struct device *d = tp_to_dev(tp);
> +	struct mii_bus *new_bus;
> +
> +	/* create bit control struct for PHY */
> +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> +	if (!bitbang)
> +		return -ENOMEM;
> +
> +	/* bitbang init */
> +	bitbang->tp = tp;
> +	bitbang->ctrl.ops = &bb_ops;
> +	bitbang->ctrl.op_c22_read = MDIO_READ;
> +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> +
> +	/* MII controller setting */
> +	new_bus = devm_mdiobus_alloc(d);
> +	if (!new_bus)
> +		return -ENOMEM;
> +
> +	new_bus->read = mdiobb_read;
> +	new_bus->write = mdiobb_write;
> +	new_bus->priv = &bitbang->ctrl;
> +

This looks like an open-coded version of alloc_mdio_bitbang().

> +	tp->mii_bus = new_bus;
> +
> +	return 0;
> +}
> +
> +static void rtl_sfp_bitbang_init(struct rtl8169_private *tp,
> +				  struct rtl_sfp_if_mask *sfp_mask)
> +{
> +	struct mii_bus *bus = tp->mii_bus;
> +	struct bb_info *bitbang = container_of(bus->priv, struct bb_info, ctrl);
> +
> +	r8168_mac_ocp_modify(tp, PINPU, sfp_mask->pin_mask, 0);
> +	r8168_mac_ocp_modify(tp, PINOE, 0, sfp_mask->pin_mask);
> +	bitbang->pinoe_value = r8168_mac_ocp_read(tp, PINOE);
> +	bitbang->pin_i_sel_1_value = r8168_mac_ocp_read(tp, PIN_I_SEL_1);
> +	bitbang->pin_i_sel_2_value = r8168_mac_ocp_read(tp, PIN_I_SEL_2);
> +	memcpy(&bitbang->sfp_mask, sfp_mask, sizeof(struct rtl_sfp_if_mask));
> +}
> +
> +static void rtl_sfp_mdio_write(struct rtl8169_private *tp,
> +				  u8 reg,
> +				  u16 val)
> +{
> +	struct mii_bus *bus = tp->mii_bus;
> +	struct bb_info *bitbang;
> +
> +	if (!bus)
> +		return;
> +
> +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> +	bus->write(bus, bitbang->sfp_mask.phy_addr, reg, val);
> +}
> +
> +static u16 rtl_sfp_mdio_read(struct rtl8169_private *tp,
> +				  u8 reg)
> +{
> +	struct mii_bus *bus = tp->mii_bus;
> +	struct bb_info *bitbang;
> +
> +	if (!bus)
> +		return ~0;
> +
> +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> +
> +	return bus->read(bus, bitbang->sfp_mask.phy_addr, reg);
> +}
> +
> +static void rtl_sfp_mdio_modify(struct rtl8169_private *tp, u32 reg, u16 mask,
> +				 u16 set)
> +{
> +	u16 data = rtl_sfp_mdio_read(tp, reg);
> +
> +	rtl_sfp_mdio_write(tp, reg, (data & ~mask) | set);
> +}
> +
> +#define RTL8211FS_PHY_ID_1 0x001c
> +#define RTL8211FS_PHY_ID_2 0xc916
> +

There shouldn't be a dependency on a specific PHY type. It may not reflect your
use cases, but it should be perfectly possible to combine this MAC with other
PHY types, also from other vendors, supporting fiber.

> +static enum rtl_sfp_if_type rtl8168h_check_sfp(struct rtl8169_private *tp)
> +{
> +	int i;
> +	int const checkcnt = 4;
> +	static struct rtl_sfp_if_mask rtl_sfp_if_eeprom_mask = {
> +		0x0050, 0x0040, 0x000f, 0x0f00, 0, 6};
> +	static struct rtl_sfp_if_mask rtl_sfp_if_gpo_mask = {
> +		0x0210, 0x0200, 0xf000, 0x0f00, 1, 9};
> +
> +	if (rtl_mdio_bitbang_init(tp))
> +		return RTL_SFP_IF_NONE;
> +
> +	rtl_sfp_bitbang_init(tp, &rtl_sfp_if_eeprom_mask);
> +	rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
> +	for (i = 0; i < checkcnt; i++) {
> +		if (rtl_sfp_mdio_read(tp, MII_PHYSID1) != RTL8211FS_PHY_ID_1 ||
> +			rtl_sfp_mdio_read(tp, MII_PHYSID2) != RTL8211FS_PHY_ID_2)
> +			break;
> +	}
> +
> +	if (i == checkcnt)
> +		return RTL_SFP_IF_EEPROM;
> +
> +	rtl_sfp_bitbang_init(tp, &rtl_sfp_if_gpo_mask);
> +	rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
> +	for (i = 0; i < checkcnt; i++) {
> +		if (rtl_sfp_mdio_read(tp, MII_PHYSID1) != RTL8211FS_PHY_ID_1 ||
> +			rtl_sfp_mdio_read(tp, MII_PHYSID2) != RTL8211FS_PHY_ID_2)
> +			break;
> +	}
> +
> +	if (i == checkcnt)
> +		return RTL_SFP_IF_GPIO;
> +
> +	return RTL_SFP_IF_NONE;
> +}
> +
> +static enum rtl_sfp_if_type rtl_check_sfp(struct rtl8169_private *tp)
> +{
> +	switch (tp->mac_version) {
> +	case RTL_GIGA_MAC_VER_45:
> +	case RTL_GIGA_MAC_VER_46:
> +		if (tp->pci_dev->revision == 0x2a)
> +			return rtl8168h_check_sfp(tp);
> +		else
> +			return RTL_SFP_IF_NONE;
> +	default:
> +		return RTL_SFP_IF_NONE;
> +	}
> +}
> +
> +static void rtl_hw_sfp_phy_config(struct rtl8169_private *tp)
> +{
> +	/* disable ctap */
> +	rtl_sfp_mdio_write(tp, 0x1f, 0x0a43);
> +	rtl_sfp_mdio_modify(tp, 0x19, BIT(6), 0);
> +
> +	/* change Rx threshold */
> +	rtl_sfp_mdio_write(tp, 0x1f, 0x0dcc);
> +	rtl_sfp_mdio_modify(tp, 0x14, 0, BIT(2) | BIT(3) | BIT(4));
> +
> +	/* switch pin34 to PMEB pin */
> +	rtl_sfp_mdio_write(tp, 0x1f, 0x0d40);
> +	rtl_sfp_mdio_modify(tp, 0x16, 0, BIT(5));
> +
> +	rtl_sfp_mdio_write(tp, 0x1f, 0x0000);
> +
> +	/* disable ctap */
> +	phy_modify_paged(tp->phydev, 0x0a43, 0x11, BIT(6), 0);
> +}
> +
>  static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool enable)
>  {
>  	switch (tp->mac_version) {
> @@ -2195,6 +2474,9 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
>  	    tp->pci_dev->subsystem_device == 0xe000)
>  		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
>  
> +	if (tp->sfp_if_type != RTL_SFP_IF_NONE)
> +		rtl_hw_sfp_phy_config(tp);
> +
>  	/* We may have called phy_speed_down before */
>  	phy_speed_up(tp->phydev);
>  
> @@ -2251,7 +2533,8 @@ 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);
> +		if (tp->sfp_if_type == RTL_SFP_IF_NONE)
> +			phy_speed_down(tp->phydev, false);
>  		rtl_wol_enable_rx(tp);
>  	}
>  }
> @@ -5386,6 +5669,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	tp->dash_type = rtl_check_dash(tp);
>  
> +	tp->sfp_if_type = rtl_check_sfp(tp);
> +
>  	tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
>  
>  	if (sizeof(dma_addr_t) > 4 && tp->mac_version >= RTL_GIGA_MAC_VER_18 &&

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

[Hau]

> > @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct
> rtl8169_private *tp, int reg, int value)
> >  	if (tp->ocp_base != OCP_STD_PHY_BASE)
> >  		reg -= 0x10;
> >
> > -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> > +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> > +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value &
> BMCR_PDOWN)
> > +			return;
> > +
> 
> Please could you explain this change.
H/W has issue. If we power down phy, then H/W will become abnormal.

> > +/* Data I/O pin control */
> > +static void rtl_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 reg = PINOE;
> > +	const u16 mask = bitbang->sfp_mask.mdio_oe_mask;
> > +	u16 value;
> 
> Reverse christmas tree please. Please sort this, longest first.
I will correct this.
 
> > +/* MDIO bus init function */
> > +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp) {
> > +	struct bb_info *bitbang;
> > +	struct device *d = tp_to_dev(tp);
> > +	struct mii_bus *new_bus;
> > +
> > +	/* create bit control struct for PHY */
> > +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> > +	if (!bitbang)
> > +		return -ENOMEM;
> > +
> > +	/* bitbang init */
> > +	bitbang->tp = tp;
> > +	bitbang->ctrl.ops = &bb_ops;
> > +	bitbang->ctrl.op_c22_read = MDIO_READ;
> > +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> > +
> > +	/* MII controller setting */
> > +	new_bus = devm_mdiobus_alloc(d);
> > +	if (!new_bus)
> > +		return -ENOMEM;
> > +
> > +	new_bus->read = mdiobb_read;
> > +	new_bus->write = mdiobb_write;
> > +	new_bus->priv = &bitbang->ctrl;
> 
> Please use alloc_mdio_bitbang().
>
I have tried to use alloc_mdio_bitbang(). But I will get error message " rmmod: ERROR: Module r8169 is in use " when I try to unload r8169.
After debug, I found it is cause by "__module_get(ctrl->ops->owner); " in alloc_mdio_bitbang().

> > +static u16 rtl_sfp_mdio_read(struct rtl8169_private *tp,
> > +				  u8 reg)
> > +{
> > +	struct mii_bus *bus = tp->mii_bus;
> > +	struct bb_info *bitbang;
> > +
> > +	if (!bus)
> > +		return ~0;
> > +
> > +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> > +
> > +	return bus->read(bus, bitbang->sfp_mask.phy_addr, reg);
> 
> By doing this, you are bypassing all the locking. You don't normally need
> operations like this. When you register the MDIO bus to the core, it will go
> find any PHYs on the bus. You can then use phylib to access the PHY. A MAC
> accessing the PHY is generally wrong.
> 
Because there is a issue when use alloc_mdio_bitbang() and I just want to use read/write function from bitbanged MDIO framework.
So I did not register MDIO bus.

Thanks,
Hau

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

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

[Andrew Lunn]

On Tue, Aug 23, 2022 at 03:17:16PM +0000, Hau wrote:
> > > @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct
> > rtl8169_private *tp, int reg, int value)
> > >  	if (tp->ocp_base != OCP_STD_PHY_BASE)
> > >  		reg -= 0x10;
> > >
> > > -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> > > +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> > > +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value &
> > BMCR_PDOWN)
> > > +			return;
> > > +
> > 
> > Please could you explain this change.
> H/W has issue. If we power down phy, then H/W will become abnormal.

Does the MAC break, or the PHY? If the PHY is the problem, the work
around should be in the PHY driver, not the MAC driver.

> > > +/* MDIO bus init function */
> > > +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp) {
> > > +	struct bb_info *bitbang;
> > > +	struct device *d = tp_to_dev(tp);
> > > +	struct mii_bus *new_bus;
> > > +
> > > +	/* create bit control struct for PHY */
> > > +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> > > +	if (!bitbang)
> > > +		return -ENOMEM;
> > > +
> > > +	/* bitbang init */
> > > +	bitbang->tp = tp;
> > > +	bitbang->ctrl.ops = &bb_ops;
> > > +	bitbang->ctrl.op_c22_read = MDIO_READ;
> > > +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> > > +
> > > +	/* MII controller setting */
> > > +	new_bus = devm_mdiobus_alloc(d);
> > > +	if (!new_bus)
> > > +		return -ENOMEM;
> > > +
> > > +	new_bus->read = mdiobb_read;
> > > +	new_bus->write = mdiobb_write;
> > > +	new_bus->priv = &bitbang->ctrl;
> > 
> > Please use alloc_mdio_bitbang().
> >
> I have tried to use alloc_mdio_bitbang(). But I will get error message " rmmod: ERROR: Module r8169 is in use " when I try to unload r8169.
> After debug, I found it is cause by "__module_get(ctrl->ops->owner); " in alloc_mdio_bitbang().

void free_mdio_bitbang(struct mii_bus *bus)
{
        struct mdiobb_ctrl *ctrl = bus->priv;

        module_put(ctrl->ops->owner);
        mdiobus_free(bus);
}

Make sure your cleanup is symmetrical to your setup.

     Andrew

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

[Hau]

> > +#define MDIO_READ 2
> > +#define MDIO_WRITE 1
> > +/* MDIO bus init function */
> > +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp) {
> > +	struct bb_info *bitbang;
> > +	struct device *d = tp_to_dev(tp);
> > +	struct mii_bus *new_bus;
> > +
> > +	/* create bit control struct for PHY */
> > +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> > +	if (!bitbang)
> > +		return -ENOMEM;
> > +
> > +	/* bitbang init */
> > +	bitbang->tp = tp;
> > +	bitbang->ctrl.ops = &bb_ops;
> > +	bitbang->ctrl.op_c22_read = MDIO_READ;
> > +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> > +
> > +	/* MII controller setting */
> > +	new_bus = devm_mdiobus_alloc(d);
> > +	if (!new_bus)
> > +		return -ENOMEM;
> > +
> > +	new_bus->read = mdiobb_read;
> > +	new_bus->write = mdiobb_write;
> > +	new_bus->priv = &bitbang->ctrl;
> > +
> 
> This looks like an open-coded version of alloc_mdio_bitbang().
> 
Yes, it is part of alloc_mdio_bitbang().

> > +	tp->mii_bus = new_bus;
> > +
> > +	return 0;
> > +}
> > +
> > +static void rtl_sfp_bitbang_init(struct rtl8169_private *tp,
> > +				  struct rtl_sfp_if_mask *sfp_mask) {
> > +	struct mii_bus *bus = tp->mii_bus;
> > +	struct bb_info *bitbang = container_of(bus->priv, struct bb_info,
> > +ctrl);
> > +
> > +	r8168_mac_ocp_modify(tp, PINPU, sfp_mask->pin_mask, 0);
> > +	r8168_mac_ocp_modify(tp, PINOE, 0, sfp_mask->pin_mask);
> > +	bitbang->pinoe_value = r8168_mac_ocp_read(tp, PINOE);
> > +	bitbang->pin_i_sel_1_value = r8168_mac_ocp_read(tp,
> PIN_I_SEL_1);
> > +	bitbang->pin_i_sel_2_value = r8168_mac_ocp_read(tp,
> PIN_I_SEL_2);
> > +	memcpy(&bitbang->sfp_mask, sfp_mask, sizeof(struct
> > +rtl_sfp_if_mask)); }
> > +
> > +static void rtl_sfp_mdio_write(struct rtl8169_private *tp,
> > +				  u8 reg,
> > +				  u16 val)
> > +{
> > +	struct mii_bus *bus = tp->mii_bus;
> > +	struct bb_info *bitbang;
> > +
> > +	if (!bus)
> > +		return;
> > +
> > +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> > +	bus->write(bus, bitbang->sfp_mask.phy_addr, reg, val); }
> > +
> > +static u16 rtl_sfp_mdio_read(struct rtl8169_private *tp,
> > +				  u8 reg)
> > +{
> > +	struct mii_bus *bus = tp->mii_bus;
> > +	struct bb_info *bitbang;
> > +
> > +	if (!bus)
> > +		return ~0;
> > +
> > +	bitbang = container_of(bus->priv, struct bb_info, ctrl);
> > +
> > +	return bus->read(bus, bitbang->sfp_mask.phy_addr, reg); }
> > +
> > +static void rtl_sfp_mdio_modify(struct rtl8169_private *tp, u32 reg, u16
> mask,
> > +				 u16 set)
> > +{
> > +	u16 data = rtl_sfp_mdio_read(tp, reg);
> > +
> > +	rtl_sfp_mdio_write(tp, reg, (data & ~mask) | set); }
> > +
> > +#define RTL8211FS_PHY_ID_1 0x001c
> > +#define RTL8211FS_PHY_ID_2 0xc916
> > +
> 
> There shouldn't be a dependency on a specific PHY type. It may not reflect
> your use cases, but it should be perfectly possible to combine this MAC with
> other PHY types, also from other vendors, supporting fiber.
> 
This is a special use case. rtl8168h is not designed as MAC to connect to other PHY.

Thanks,
Hau
 ------Please consider the environment before printing this e-mail.

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

[Hau]

> > > > @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct
> > > rtl8169_private *tp, int reg, int value)
> > > >  	if (tp->ocp_base != OCP_STD_PHY_BASE)
> > > >  		reg -= 0x10;
> > > >
> > > > -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> > > > +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> > > > +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value &
> > > BMCR_PDOWN)
> > > > +			return;
> > > > +
> > >
> > > Please could you explain this change.
> > H/W has issue. If we power down phy, then H/W will become abnormal.
> 
> Does the MAC break, or the PHY? If the PHY is the problem, the work around
> should be in the PHY driver, not the MAC driver.
>
It is a workaround. We did not get the root cause yet.

> > > > +/* MDIO bus init function */
> > > > +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp) {
> > > > +	struct bb_info *bitbang;
> > > > +	struct device *d = tp_to_dev(tp);
> > > > +	struct mii_bus *new_bus;
> > > > +
> > > > +	/* create bit control struct for PHY */
> > > > +	bitbang = devm_kzalloc(d, sizeof(struct bb_info), GFP_KERNEL);
> > > > +	if (!bitbang)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	/* bitbang init */
> > > > +	bitbang->tp = tp;
> > > > +	bitbang->ctrl.ops = &bb_ops;
> > > > +	bitbang->ctrl.op_c22_read = MDIO_READ;
> > > > +	bitbang->ctrl.op_c22_write = MDIO_WRITE;
> > > > +
> > > > +	/* MII controller setting */
> > > > +	new_bus = devm_mdiobus_alloc(d);
> > > > +	if (!new_bus)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	new_bus->read = mdiobb_read;
> > > > +	new_bus->write = mdiobb_write;
> > > > +	new_bus->priv = &bitbang->ctrl;
> > >
> > > Please use alloc_mdio_bitbang().
> > >
> > I have tried to use alloc_mdio_bitbang(). But I will get error message "
> rmmod: ERROR: Module r8169 is in use " when I try to unload r8169.
> > After debug, I found it is cause by "__module_get(ctrl->ops->owner); " in
> alloc_mdio_bitbang().
> 
> void free_mdio_bitbang(struct mii_bus *bus) {
>         struct mdiobb_ctrl *ctrl = bus->priv;
> 
>         module_put(ctrl->ops->owner);
>         mdiobus_free(bus);
> }
> 
> Make sure your cleanup is symmetrical to your setup.
> 
I have tried to call free_mdio_bitbang() in rtl_remove_one (). But when I try to unload r8169, rtl_remove_one() did not get called.
So there is no chance to call free_mdio_bitbang().

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

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

[Andrew Lunn]

On Tue, Aug 23, 2022 at 03:48:13PM +0000, Hau wrote:
> > > > > @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct
> > > > rtl8169_private *tp, int reg, int value)
> > > > >  	if (tp->ocp_base != OCP_STD_PHY_BASE)
> > > > >  		reg -= 0x10;
> > > > >
> > > > > -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
> > > > > +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR) {
> > > > > +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value &
> > > > BMCR_PDOWN)
> > > > > +			return;
> > > > > +
> > > >
> > > > Please could you explain this change.
> > > H/W has issue. If we power down phy, then H/W will become abnormal.
> > 
> > Does the MAC break, or the PHY? If the PHY is the problem, the work around
> > should be in the PHY driver, not the MAC driver.
> >
> It is a workaround. We did not get the root cause yet.

O.K, since you are modifying a PHY register, lets assume it is a PHY
issue. Please put the workaround in the PHY driver.

> > > I have tried to use alloc_mdio_bitbang(). But I will get error message "
> > rmmod: ERROR: Module r8169 is in use " when I try to unload r8169.
> > > After debug, I found it is cause by "__module_get(ctrl->ops->owner); " in
> > alloc_mdio_bitbang().
> > 
> > void free_mdio_bitbang(struct mii_bus *bus) {
> >         struct mdiobb_ctrl *ctrl = bus->priv;
> > 
> >         module_put(ctrl->ops->owner);
> >         mdiobus_free(bus);
> > }
> > 
> > Make sure your cleanup is symmetrical to your setup.
> > 
> I have tried to call free_mdio_bitbang() in rtl_remove_one (). But when I try to unload r8169, rtl_remove_one() did not get called.

So you probably need to dig into driver/base/dd.c and maybe the
drivers/pci, to understand why.

	Andrew

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

[Hau]

> On Tue, Aug 23, 2022 at 03:48:13PM +0000, Hau wrote:
> > > > > > @@ -914,8 +952,12 @@ static void r8168g_mdio_write(struct
> > > > > rtl8169_private *tp, int reg, int value)
> > > > > >  	if (tp->ocp_base != OCP_STD_PHY_BASE)
> > > > > >  		reg -= 0x10;
> > > > > >
> > > > > > -	if (tp->ocp_base == OCP_STD_PHY_BASE && reg ==
> MII_BMCR)
> > > > > > +	if (tp->ocp_base == OCP_STD_PHY_BASE && reg ==
> MII_BMCR) {
> > > > > > +		if (tp->sfp_if_type != RTL_SFP_IF_NONE && value &
> > > > > BMCR_PDOWN)
> > > > > > +			return;
> > > > > > +
> > > > >
> > > > > Please could you explain this change.
> > > > H/W has issue. If we power down phy, then H/W will become abnormal.
> > >
> > > Does the MAC break, or the PHY? If the PHY is the problem, the work
> > > around should be in the PHY driver, not the MAC driver.
> > >
> > It is a workaround. We did not get the root cause yet.
> 
> O.K, since you are modifying a PHY register, lets assume it is a PHY issue.
> Please put the workaround in the PHY driver.
I will try to do this.

> > > > I have tried to use alloc_mdio_bitbang(). But I will get error message "
> > > rmmod: ERROR: Module r8169 is in use " when I try to unload r8169.
> > > > After debug, I found it is cause by
> > > > "__module_get(ctrl->ops->owner); " in
> > > alloc_mdio_bitbang().
> > >
> > > void free_mdio_bitbang(struct mii_bus *bus) {
> > >         struct mdiobb_ctrl *ctrl = bus->priv;
> > >
> > >         module_put(ctrl->ops->owner);
> > >         mdiobus_free(bus);
> > > }
> > >
> > > Make sure your cleanup is symmetrical to your setup.
> > >
> > I have tried to call free_mdio_bitbang() in rtl_remove_one (). But when I
> try to unload r8169, rtl_remove_one() did not get called.
> 
> So you probably need to dig into driver/base/dd.c and maybe the drivers/pci,
> to understand why.
In command "rmmod", if module refcnt not equal to 0, It will show error message " Module r8169 is in use ". And "__module_get()" will increase module refcnt.
If module refcnt is not equal to 0, rmmod will not delete module.

Is that possible for us to call alloc_mdio_bitbang() but set owner to null? I have try this setting, command "rmmod" can be executed normally.

Thanks,
Hau

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