From: Simon Horman <simon.horman@corigine.com>
To: Mengyuan Lou <mengyuanlou@net-swift.com>
Cc: netdev@vger.kernel.org, jiawenwu@trustnetic.com,
Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH net-next v8] net: ngbe: Add ngbe mdio bus driver.
Date: Fri, 13 Jan 2023 12:07:13 +0100 [thread overview]
Message-ID: <Y8E7YQmkRFBEwsjx@corigine.com> (raw)
In-Reply-To: <20230111111718.40745-1-mengyuanlou@net-swift.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.
...
next prev parent reply other threads:[~2023-01-13 11:33 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-11 11:17 [PATCH net-next v8] net: ngbe: Add ngbe mdio bus driver Mengyuan Lou
2023-01-13 11:07 ` Simon Horman [this message]
2023-01-13 19:37 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y8E7YQmkRFBEwsjx@corigine.com \
--to=simon.horman@corigine.com \
--cc=andrew@lunn.ch \
--cc=jiawenwu@trustnetic.com \
--cc=mengyuanlou@net-swift.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).