Re: [PATCH net-next v8] net: ngbe: Add ngbe mdio bus driver.

[Simon Horman <simon.horman@corigine.com>]

On Wed, Jan 11, 2023 at 07:17:18PM +0800, Mengyuan Lou wrote:
> Add mdio bus register for ngbe.
> The internal phy and external phy need to be handled separately.
> Add phy changed event detection.
> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks Mengyuan Lou,

some minor feedback from my side.

...

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index a52908d01c9c..165f61698177 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -133,11 +133,14 @@
>  /************************************* ETH MAC *****************************/
>  #define WX_MAC_TX_CFG                0x11000
>  #define WX_MAC_TX_CFG_TE             BIT(0)
> +#define WX_MAC_TX_CFG_SPEED_MASK     GENMASK(30, 29)
> +#define WX_MAC_TX_CFG_SPEED_1G       (0x3 << 29)

nit: can GENMASK() be used to define WX_MAC_TX_CFG_SPEED_1G too?

>  #define WX_MAC_RX_CFG                0x11004
>  #define WX_MAC_RX_CFG_RE             BIT(0)
>  #define WX_MAC_RX_CFG_JE             BIT(8)
>  #define WX_MAC_PKT_FLT               0x11008
>  #define WX_MAC_PKT_FLT_PR            BIT(0) /* promiscuous mode */
> +#define WX_MAC_WDG_TIMEOUT           0x1100C
>  #define WX_MAC_RX_FLOW_CTRL          0x11090
>  #define WX_MAC_RX_FLOW_CTRL_RFE      BIT(0) /* receive fc enable */
>  #define WX_MMC_CONTROL               0x11800

...

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c
> index 588de24b5e18..b9534d608d35 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_hw.c
> @@ -39,16 +39,24 @@ int ngbe_eeprom_chksum_hostif(struct wx *wx)

...

> +void ngbe_sfp_modules_txrx_powerctl(struct wx *wx, bool swi)
> +{
> +	if (swi)
> +		/* gpio0 is used to power on control*/
> +		wr32(wx, NGBE_GPIO_DR, 0);
> +	else
> +		/* gpio0 is used to power off control*/
> +		wr32(wx, NGBE_GPIO_DR, NGBE_GPIO_DR_0);

nit: maybe this is nicer?

	wr32(wx, NGBE_GPIO_DR, swi ? 0 : NGBE_GPIO_DR_0);

...

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index f66513ddf6d9..ed52f80b5475 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c

...

> @@ -385,6 +411,11 @@ static int ngbe_probe(struct pci_dev *pdev,
>  	eth_hw_addr_set(netdev, wx->mac.perm_addr);
>  	wx_mac_set_default_filter(wx, wx->mac.perm_addr);
>  
> +	/* phy Interface Configuration */
> +	err = ngbe_mdio_init(wx);
> +	if (err)
> +		goto err_free_mac_table;

Should this branch to err_register, as is the case a few lines below?

> +
>  	err = register_netdev(netdev);
>  	if (err)
>  		goto err_register;

...

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_type.h b/drivers/net/ethernet/wangxun/ngbe/ngbe_type.h
> index 369d181930bc..612b9da2db8f 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_type.h
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_type.h
> @@ -60,6 +60,26 @@
>  #define NGBE_EEPROM_VERSION_L			0x1D
>  #define NGBE_EEPROM_VERSION_H			0x1E
>  
> +/* mdio access */
> +#define NGBE_MSCA				0x11200
> +#define NGBE_MSCA_RA(v)				((0xFFFF & (v)))
> +#define NGBE_MSCA_PA(v)				((0x1F & (v)) << 16)
> +#define NGBE_MSCA_DA(v)				((0x1F & (v)) << 21)
> +#define NGBE_MSCC				0x11204
> +#define NGBE_MSCC_DATA(v)			((0xFFFF & (v)))
> +#define NGBE_MSCC_CMD(v)			((0x3 & (v)) << 16)

nit: perhaps the above sould be cleaner when expressed
     using FILED_PREP and U16_MAX.

> +
> +enum NGBE_MSCA_CMD_value {
> +	NGBE_MSCA_CMD_RSV = 0,
> +	NGBE_MSCA_CMD_WRITE,
> +	NGBE_MSCA_CMD_POST_READ,
> +	NGBE_MSCA_CMD_READ,
> +};
> +
> +#define NGBE_MSCC_SADDR				BIT(18)
> +#define NGBE_MSCC_BUSY				BIT(22)
> +#define NGBE_MDIO_CLK(v)			((0x7 & (v)) << 19)

Ditto.

...