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

[Hau <hau@realtek.com>]

> 
> On 15.09.2022 16:48, 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.
> >
> > And set mdiobb_ops owner to NULL to avoid increase module's refcount
> > to prevent rmmod cannot be done.
> > https://patchwork.kernel.org/project/linux-renesas-soc/patch/202007301
> > 00151.7490-1-ashiduka@fujitsu.com/
> >
> > Signed-off-by: Chunhao Lin <hau@realtek.com>
> > ---
> >  drivers/net/ethernet/realtek/Kconfig      |   1 +
> >  drivers/net/ethernet/realtek/r8169_main.c | 286
> > +++++++++++++++++++++-
> >  2 files changed, 286 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 f6f63ba6593a..395eae62050a 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>
> >
> > @@ -333,6 +334,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)
> > @@ -573,6 +583,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;
> > +	const u16 mdio_oe;
> > +	const u16 mdio;
> > +	const u16 mdc;
> > +	const u16 phy_addr;
> > +	const u16 rb_pos;
> > +};
> > +
> >  enum rtl_flag {
> >  	RTL_FLAG_TASK_ENABLED = 0,
> >  	RTL_FLAG_TASK_RESET_PENDING,
> > @@ -585,6 +613,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;
> > @@ -624,6 +658,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 */
> 
> This is at least misleading because mii_bus is unused for the integrated
> copper PHY's. Maybe you can store the mii_bus in struct bb_info and store
> the bb_info instance in struct rtl8169_private.
> 
I will do this.

> >  };
> >
> >  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp); @@
> > -1199,6 +1237,242 @@ 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 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,
> > +};
> > +
> > +#define MDIO_READ 2
> > +#define MDIO_WRITE 1
> 
> This duplicates the defines in drivers/net/mdio/mdio-bitbang.c
> 
I will check this.

> > +/* MDIO bus init function */
> > +static int rtl_mdio_bitbang_init(struct rtl8169_private *tp) {
> > +	struct device *d = tp_to_dev(tp);
> > +	struct bb_info *bitbang;
> > +	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;
> 
> MDIO_READ/WRITE are assigned by alloc_mdio_bitbang(), you don't need
> this here.
> 
I will check this.

> > +
> > +	/* MII controller setting */
> > +	new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
> 
> It seems the allocated MDIO bus is never registered.
> This should be added.
> 
> > +	if (!new_bus)
> > +		return -ENOMEM;
> > +
> > +	tp->mii_bus = new_bus;
> > +
> > +	return 0;
> > +}
> > +
> > +static void rtl_sfp_bitbang_init(struct rtl8169_private *tp,
> > +				  struct rtl_sfp_if_mask *mask)
> > +{
> > +	struct bb_info *bitbang =
> > +		container_of(tp->mii_bus->priv, struct bb_info, ctrl);
> > +
> > +	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_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->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->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
> > +
> 
> As stated earlier, there should be no dependency on a specific external PHY
> in a MAC driver. It's not clear why the bitbanged MDIO bus can't be used with
> other PHY's.
> 
I will try to remove this restriction.

> > +static enum rtl_sfp_if_type rtl8168h_check_sfp(struct rtl8169_private
> > +*tp) {
> > +	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};
> 
> You hard-code PHY addresses 0/1 here. Why not auto-probing?
> And can't there be designs using other PHY addresses?
> 
Not this application only for rtl8211fs.

> > +	int const checkcnt = 4;
> > +	int i;
> > +
> > +	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;
> 
> See earlier comment on dependency on a specific PHY model.
> 
> > +	}
> > +
> > +	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_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) {
> 
> This PHY model specific initialization belongs to the PHY driver.
> Presumably RTL8211FS support should be added to drivers/net/phy/realtek.c
> 
I am not familiar with phy driver. May I know how to do this?

> > +	/* 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));
> > +
> > +	/* enable pass_linkctl_en */
> > +	rtl_sfp_mdio_write(tp, 0x1f, 0x0a4b);
> > +	rtl_sfp_mdio_modify(tp, 0x11, 0, BIT(4));
> > +
> > +	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) {
> > @@ -2168,6 +2442,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);
> >
> > @@ -2224,7 +2501,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);
> >  	}
> >  }
> > @@ -4866,6 +5144,10 @@ static void rtl_remove_one(struct pci_dev
> *pdev)
> >  	if (tp->dash_type != RTL_DASH_NONE)
> >  		rtl8168_driver_stop(tp);
> >
> > +	/* free bitbang info */
> > +	if (tp->mii_bus)
> > +		free_mdio_bitbang(tp->mii_bus);
> > +
> >  	rtl_release_firmware(tp);
> >
> >  	/* restore original MAC address */
> > @@ -5210,6 +5492,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
> > &&
> 
> 
> ------Please consider the environment before printing this e-mail.