[RFC,net-next,5/6] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY

Commit Message

Parthiban Veerasooran Sept. 8, 2023, 2:29 p.m. UTC

The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
MAC integration provides the low pin count standard SPI interface to any
microcontroller therefore providing Ethernet functionality without
requiring MAC integration within the microcontroller. The LAN8650/1
operates as an SPI client supporting SCLK clock rates up to a maximum of
25 MHz. This SPI interface supports the transfer of both data (Ethernet
frames) and control (register access).

By default, the chunk data payload is 64 bytes in size. A smaller payload
data size of 32 bytes is also supported and may be configured in the
Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
register. Changing the chunk payload size requires the LAN8650/1 be reset
and shall not be done during normal operation.

The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
PHY and MAC are connected via an internal Media Independent Interface
(MII).

Signed-off-by: Parthiban Veerasooran <[e-mail]>
---
 MAINTAINERS                              |   6 +
 drivers/net/ethernet/microchip/Kconfig   |  10 +
 drivers/net/ethernet/microchip/Makefile  |   3 +
 drivers/net/ethernet/microchip/lan865x.c | 589 +++++++++++++++++++++++
 4 files changed, 608 insertions(+)
 create mode 100644 drivers/net/ethernet/microchip/lan865x.c

Comments

Simon Horman Sept. 10, 2023, 5:44 p.m. UTC | #1

On Fri, Sep 08, 2023 at 07:59:18PM +0530, Parthiban Veerasooran wrote:
> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
> MAC integration provides the low pin count standard SPI interface to any
> microcontroller therefore providing Ethernet functionality without
> requiring MAC integration within the microcontroller. The LAN8650/1
> operates as an SPI client supporting SCLK clock rates up to a maximum of
> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
> frames) and control (register access).
> 
> By default, the chunk data payload is 64 bytes in size. A smaller payload
> data size of 32 bytes is also supported and may be configured in the
> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
> register. Changing the chunk payload size requires the LAN8650/1 be reset
> and shall not be done during normal operation.
> 
> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
> PHY and MAC are connected via an internal Media Independent Interface
> (MII).
> 
> Signed-off-by: Parthiban Veerasooran <[e-mail]>

Hi Parthiban,

thanks for your patches.
Some minor feedback on this one follows.

...

> diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c

...

> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
> +{
> +	u32 regval;
> +	bool ret;
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	const u8 *mac = netdev->dev_addr;

Please arrange local variables in Networking code in reverse xmas tree
order - longest line to shortest.

This tool can be of assistance here:
https://github.com/ecree-solarflare/xmastree

...

> +static int lan865x_probe(struct spi_device *spi)
> +{
> +	struct net_device *netdev;
> +	struct lan865x_priv *priv;
> +	u32 regval;
> +	int ret;
> +
> +	netdev = alloc_etherdev(sizeof(struct lan865x_priv));
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(netdev);
> +	priv->netdev = netdev;
> +	priv->spi = spi;
> +	priv->msg_enable = 0;
> +	spi_set_drvdata(spi, priv);
> +	SET_NETDEV_DEV(netdev, &spi->dev);
> +
> +	ret = lan865x_get_dt_data(priv);
> +	if (ret)
> +		return ret;
> +
> +	spi->rt = true;
> +	spi_setup(spi);
> +
> +	priv->tc6 = oa_tc6_init(spi, netdev);
> +	if (!priv->tc6) {
> +		ret = -ENOMEM;
> +		goto error_oa_tc6_init;
> +	}
> +
> +	if (priv->cps == 32) {
> +		regval = CCS_Q0_TX_CFG_32;
> +		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
> +		if (ret)
> +			return ret;
> +
> +		regval = CCS_Q0_RX_CFG_32;
> +		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
> +			     priv->rxcte))
> +		goto err_macphy_config;

Jumping to err_macphy_config will result in this function returning ret.
However, ret will be 0 at this point. Perhaps it should be set to an
error value.

Flagged by Smatch.

> +
> +	ret = lan865x_phy_init(priv);
> +	if (ret)
> +		goto error_phy;
> +
> +	if (device_get_ethdev_address(&spi->dev, netdev))
> +		eth_hw_addr_random(netdev);
> +
> +	ret = lan865x_set_hw_macaddr(netdev);
> +	if (ret) {
> +		if (netif_msg_probe(priv))
> +			dev_err(&spi->dev, "Failed to configure MAC");
> +		goto error_set_mac;
> +	}
> +
> +	netdev->if_port = IF_PORT_10BASET;
> +	netdev->irq = spi->irq;
> +	netdev->netdev_ops = &lan865x_netdev_ops;
> +	netdev->watchdog_timeo = TX_TIMEOUT;
> +	netdev->ethtool_ops = &lan865x_ethtool_ops;
> +	ret = register_netdev(netdev);
> +	if (ret) {
> +		if (netif_msg_probe(priv))
> +			dev_err(&spi->dev, "Register netdev failed (ret = %d)",
> +				ret);
> +		goto error_netdev_register;
> +	}
> +
> +	phy_start(priv->phydev);
> +	return 0;
> +
> +error_netdev_register:
> +error_set_mac:
> +	phy_disconnect(priv->phydev);
> +	mdiobus_unregister(priv->mdiobus);
> +	mdiobus_free(priv->mdiobus);
> +error_phy:
> +err_macphy_config:
> +	oa_tc6_deinit(priv->tc6);
> +error_oa_tc6_init:
> +	free_netdev(priv->netdev);
> +	return ret;
> +}


...

Ziyang Xuan (William) Sept. 11, 2023, 1:17 p.m. UTC | #2

> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
> MAC integration provides the low pin count standard SPI interface to any
> microcontroller therefore providing Ethernet functionality without
> requiring MAC integration within the microcontroller. The LAN8650/1
> operates as an SPI client supporting SCLK clock rates up to a maximum of
> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
> frames) and control (register access).
> 
> By default, the chunk data payload is 64 bytes in size. A smaller payload
> data size of 32 bytes is also supported and may be configured in the
> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
> register. Changing the chunk payload size requires the LAN8650/1 be reset
> and shall not be done during normal operation.
> 
> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
> PHY and MAC are connected via an internal Media Independent Interface
> (MII).
> 
> Signed-off-by: Parthiban Veerasooran <[e-mail]>
> ---
>  MAINTAINERS                              |   6 +
>  drivers/net/ethernet/microchip/Kconfig   |  10 +
>  drivers/net/ethernet/microchip/Makefile  |   3 +
>  drivers/net/ethernet/microchip/lan865x.c | 589 +++++++++++++++++++++++
>  4 files changed, 608 insertions(+)
>  create mode 100644 drivers/net/ethernet/microchip/lan865x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c54454c7e7a1..666c042a15b2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13879,6 +13879,12 @@ L:	[e-mail]
>  S:	Maintained
>  F:	drivers/net/ethernet/microchip/lan743x_*
>  
> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
> +M:	Parthiban Veerasooran <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +F:	drivers/net/ethernet/microchip/lan865x.c
> +
>  MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
>  M:	Arun Ramadoss <[e-mail]>
>  R:	[e-mail]
> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> index 329e374b9539..d99be51b99f1 100644
> --- a/drivers/net/ethernet/microchip/Kconfig
> +++ b/drivers/net/ethernet/microchip/Kconfig
> @@ -59,4 +59,14 @@ source "drivers/net/ethernet/microchip/lan966x/Kconfig"
>  source "drivers/net/ethernet/microchip/sparx5/Kconfig"
>  source "drivers/net/ethernet/microchip/vcap/Kconfig"
>  
> +config LAN865X
> +	tristate "LAN865x support"
> +	depends on SPI
> +	help
> +      	  Support for the Microchip LAN8650/1 Rev.B0 Ethernet chip. It uses OPEN
> +	  Alliance 10BASE-T1x Serial Interface specification.
> +
> +      	  To compile this driver as a module, choose M here. The module will be
> +          called lan865x.
> +
>  endif # NET_VENDOR_MICROCHIP
> diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
> index bbd349264e6f..315e850b2b26 100644
> --- a/drivers/net/ethernet/microchip/Makefile
> +++ b/drivers/net/ethernet/microchip/Makefile
> @@ -12,3 +12,6 @@ lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
>  obj-$(CONFIG_LAN966X_SWITCH) += lan966x/
>  obj-$(CONFIG_SPARX5_SWITCH) += sparx5/
>  obj-$(CONFIG_VCAP) += vcap/
> +
> +obj-$(CONFIG_LAN865X) += lan865x_t1s.o
> +lan865x_t1s-objs := lan865x.o ../oa_tc6.o
> diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c
> new file mode 100644
> index 000000000000..3c8ebf4c258f
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan865x.c
> @@ -0,0 +1,589 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Microchip's LAN865x 10BASE-T1S MAC-PHY driver
> + *
> + * Author: Parthiban Veerasooran <[e-mail]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/etherdevice.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
> +#include <linux/oa_tc6.h>
> +
> +#define DRV_NAME		"lan865x"
> +#define DRV_VERSION		"0.1"
> +
> +#define REG_STDR_RESET		0x00000003
> +#define REG_MAC_ADDR_BO		0x00010022
> +#define REG_MAC_ADDR_L		0x00010024
> +#define REG_MAC_ADDR_H		0x00010025
> +#define REG_MAC_NW_CTRL         0x00010000
> +#define REG_MAC_NW_CONFIG	0x00010001
> +#define REG_MAC_HASHL		0x00010020
> +#define REG_MAC_HASHH		0x00010021
> +#define REG_MAC_ADDR_BO		0x00010022
> +#define REG_MAC_ADDR_L		0x00010024
> +#define REG_MAC_ADDR_H		0x00010025
> +
> +#define CCS_Q0_TX_CFG		0x000A0081
> +#define CCS_Q0_RX_CFG		0x000A0082
> +
> +/* Buffer configuration for 32-bytes chunk payload */
> +#define CCS_Q0_TX_CFG_32	0x70000000
> +#define CCS_Q0_RX_CFG_32	0x30000C00
> +
> +#define NW_RX_STATUS		BIT(2)
> +#define NW_TX_STATUS		BIT(3)
> +#define NW_DISABLE		0x0
> +
> +#define MAC_PROMISCUOUS_MODE	BIT(4)
> +#define MAC_MULTICAST_MODE	BIT(6)
> +#define MAC_UNICAST_MODE	BIT(7)
> +
> +#define TX_TIMEOUT		(4 * HZ)
> +#define LAN865X_MSG_DEFAULT	\
> +	(NETIF_MSG_PROBE | NETIF_MSG_IFUP | NETIF_MSG_IFDOWN | NETIF_MSG_LINK)
> +
> +struct lan865x_priv {
> +	struct net_device *netdev;
> +	struct spi_device *spi;
> +	struct oa_tc6 *tc6;
> +	struct mii_bus *mdiobus;
> +	struct phy_device *phydev;
> +	struct device *dev;
> +	u32 msg_enable;
> +	bool txcte;
> +	bool rxcte;
> +	u32 cps;
> +	bool protected;
> +};
> +
> +static void lan865x_handle_link_change(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +
> +	phy_print_status(priv->phydev);
> +}
> +
> +static int lan865x_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
> +{
> +	struct lan865x_priv *priv = bus->priv;
> +	u32 regval;
> +	bool ret;
> +
> +	ret = oa_tc6_read_register(priv->tc6, 0xFF00 | (idx & 0xFF), &regval, 1);
> +	if (ret)
> +		return -ENODEV;
> +
> +	return regval;
> +}
> +
> +static int lan865x_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
> +				 u16 regval)
> +{
> +	struct lan865x_priv *priv = bus->priv;
> +	u32 value = regval;
> +	bool ret;
> +
> +	ret = oa_tc6_write_register(priv->tc6, 0xFF00 | (idx & 0xFF), &value, 1);
> +	if (ret)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int lan865x_phy_init(struct lan865x_priv *priv)
> +{
> +	int ret;
> +
> +	priv->mdiobus = mdiobus_alloc();
> +	if (!priv->mdiobus) {
> +		netdev_err(priv->netdev, "MDIO bus alloc failed\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->mdiobus->phy_mask = ~(u32)BIT(1);
> +	priv->mdiobus->priv = priv;
> +	priv->mdiobus->read = lan865x_mdiobus_read;
> +	priv->mdiobus->write = lan865x_mdiobus_write;
> +	priv->mdiobus->name = "lan865x-mdiobus";
> +	priv->mdiobus->parent = priv->dev;
> +
> +	snprintf(priv->mdiobus->id, ARRAY_SIZE(priv->mdiobus->id),
> +		 "%s", dev_name(&priv->spi->dev));
> +
> +	ret = mdiobus_register(priv->mdiobus);
> +	if (ret) {
> +		netdev_err(priv->netdev, "Could not register MDIO bus\n");
> +		mdiobus_free(priv->mdiobus);
> +		return ret;
> +	}
> +	priv->phydev = phy_find_first(priv->mdiobus);
> +	if (!priv->phydev) {
> +		netdev_err(priv->netdev, "No PHY found\n");
> +		mdiobus_unregister(priv->mdiobus);
> +		mdiobus_free(priv->mdiobus);
> +		return -ENODEV;
> +	}
> +	priv->phydev->is_internal = true;
> +	ret = phy_connect_direct(priv->netdev, priv->phydev,
> +				 &lan865x_handle_link_change,
> +				 PHY_INTERFACE_MODE_INTERNAL);
> +	if (ret) {
> +		netdev_err(priv->netdev, "Can't attach PHY to %s\n", priv->mdiobus->id);
> +		return ret;
Here return directly without above resources recycle. Please check if it is correct.
If it is needed, it is recommended to use error labels to avoid duplicate code.

> +	}
> +	phy_attached_info(priv->phydev);
> +	return ret;
> +}
> +
> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
> +{
> +	u32 regval;
> +	bool ret;
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	const u8 *mac = netdev->dev_addr;
> +
> +	ret = oa_tc6_read_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1);
> +	if (ret)
> +		goto error_mac;
> +	if ((regval & NW_TX_STATUS) | (regval & NW_RX_STATUS)) {
> +		if (netif_msg_drv(priv))
> +			netdev_warn(netdev, "Hardware must be disabled for MAC setting\n");
> +		return -EBUSY;
> +	}
> +	/* MAC address setting */
> +	regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) |
> +		mac[0];
> +	ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_L, &regval, 1);
> +	if (ret)
> +		goto error_mac;
> +
> +	regval = (mac[5] << 8) | mac[4];
> +	ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_H, &regval, 1);
> +	if (ret)
> +		goto error_mac;
> +
> +	regval = (mac[5] << 24) | (mac[4] << 16) |
> +		(mac[3] << 8) | mac[2];
> +	ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_BO, &regval, 1);
> +	if (ret)
> +		goto error_mac;
> +
> +	return 0;
> +
> +error_mac:
> +	return -ENODEV;

No resource recycle, goto and error label are not needed. What's more,
the same label corresponds to different errors.

> +}
> +
> +static int
> +lan865x_set_link_ksettings(struct net_device *netdev,
> +			   const struct ethtool_link_ksettings *cmd)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	int ret = 0;
> +
> +	if (cmd->base.autoneg != AUTONEG_DISABLE ||
> +	    cmd->base.speed != SPEED_10 || cmd->base.duplex != DUPLEX_HALF) {
> +		if (netif_msg_link(priv))
> +			netdev_warn(netdev, "Unsupported link setting");
> +		ret = -EOPNOTSUPP;
> +	} else {
> +		if (netif_msg_link(priv))
> +			netdev_warn(netdev, "Hardware must be disabled to set link mode");
> +		ret = -EBUSY;
> +	}
> +	return ret;
> +}
> +
> +static int
> +lan865x_get_link_ksettings(struct net_device *netdev,
> +			   struct ethtool_link_ksettings *cmd)
> +{
> +	ethtool_link_ksettings_zero_link_mode(cmd, supported);
> +	ethtool_link_ksettings_add_link_mode(cmd, supported, 10baseT_Half);
> +	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
> +
> +	cmd->base.speed = SPEED_10;
> +	cmd->base.duplex = DUPLEX_HALF;
> +	cmd->base.port	= PORT_TP;
> +	cmd->base.autoneg = AUTONEG_DISABLE;
> +
> +	return 0;
> +}
> +
> +static void lan865x_set_msglevel(struct net_device *netdev, u32 val)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +
> +	priv->msg_enable = val;
> +}
> +
> +static u32 lan865x_get_msglevel(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +
> +	return priv->msg_enable;
> +}
> +
> +static void
> +lan865x_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *info)
> +{
> +	strscpy(info->driver, DRV_NAME, sizeof(info->driver));
> +	strscpy(info->version, DRV_VERSION, sizeof(info->version));
> +	strscpy(info->bus_info,
> +		dev_name(netdev->dev.parent), sizeof(info->bus_info));
> +}
> +
> +static const struct ethtool_ops lan865x_ethtool_ops = {
> +	.get_drvinfo	= lan865x_get_drvinfo,
> +	.get_msglevel	= lan865x_get_msglevel,
> +	.set_msglevel	= lan865x_set_msglevel,
> +	.get_link_ksettings = lan865x_get_link_ksettings,
> +	.set_link_ksettings = lan865x_set_link_ksettings,
> +};
> +
> +static void lan865x_tx_timeout(struct net_device *netdev, unsigned int txqueue)
> +{
> +	netdev->stats.tx_errors++;
> +}
> +
> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
> +{
> +	struct sockaddr *address = addr;
> +
> +	if (netif_running(netdev))
> +		return -EBUSY;
> +	if (!is_valid_ether_addr(address->sa_data))
> +		return -EADDRNOTAVAIL;
> +
> +	eth_hw_addr_set(netdev, address->sa_data);
> +	return lan865x_set_hw_macaddr(netdev);
> +}
> +
> +static u32 lan865x_hash(u8 addr[ETH_ALEN])
> +{
> +	return (ether_crc(ETH_ALEN, addr) >> 26) & 0x3f;
> +}
> +
> +static void lan865x_set_multicast_list(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	u32 regval = 0;
> +
> +	if (netdev->flags & IFF_PROMISC) {
> +		/* Enabling promiscuous mode */
> +		regval |= MAC_PROMISCUOUS_MODE;
> +		regval &= (~MAC_MULTICAST_MODE);
> +		regval &= (~MAC_UNICAST_MODE);
> +	} else if (netdev->flags & IFF_ALLMULTI) {
> +		/* Enabling all multicast mode */
> +		regval &= (~MAC_PROMISCUOUS_MODE);
> +		regval |= MAC_MULTICAST_MODE;
> +		regval &= (~MAC_UNICAST_MODE);
> +	} else if (!netdev_mc_empty(netdev)) {
> +		/* Enabling specific multicast addresses */
> +		struct netdev_hw_addr *ha;
> +		u32 hash_lo = 0;
> +		u32 hash_hi = 0;
> +
> +		netdev_for_each_mc_addr(ha, netdev) {
> +			u32 bit_num = lan865x_hash(ha->addr);
> +			u32 mask = 1 << (bit_num & 0x1f);
> +
> +			if (bit_num & 0x20)
> +				hash_hi |= mask;
> +			else
> +				hash_lo |= mask;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &hash_hi, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashh");
> +			return;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &hash_lo, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashl");
> +			return;
> +		}
> +		regval &= (~MAC_PROMISCUOUS_MODE);
> +		regval &= (~MAC_MULTICAST_MODE);
> +		regval |= MAC_UNICAST_MODE;
> +	} else {
> +		/* enabling local mac address only */
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &regval, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashh");
> +			return;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &regval, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashl");
> +			return;
> +		}
> +	}
> +	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CONFIG, &regval, 1)) {
> +		if (netif_msg_timer(priv))
> +			netdev_err(netdev, "Failed to enable promiscuous mode");
> +	}
> +}
> +
> +static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
> +				       struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +
> +	return oa_tc6_send_eth_pkt(priv->tc6, skb);
> +}
> +
> +static int lan865x_hw_disable(struct lan865x_priv *priv)
> +{
> +	u32 regval = NW_DISABLE;
> +
> +	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int lan865x_net_close(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	int ret;
> +
> +	netif_stop_queue(netdev);
> +	ret = lan865x_hw_disable(priv);
> +	if (ret) {
> +		if (netif_msg_ifup(priv))
> +			netdev_err(netdev, "Failed to disable the hardware\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan865x_hw_enable(struct lan865x_priv *priv)
> +{
> +	u32 regval = NW_TX_STATUS | NW_RX_STATUS;
> +
> +	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int lan865x_net_open(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	int ret;
> +
> +	if (!is_valid_ether_addr(netdev->dev_addr)) {
> +		if (netif_msg_ifup(priv))
> +			netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
> +		return -EADDRNOTAVAIL;
> +	}
> +	if (lan865x_hw_disable(priv)) {
> +		if (netif_msg_ifup(priv))
> +			netdev_err(netdev, "Failed to disable the hardware\n");
> +		return -ENODEV;
> +	}
> +	ret = lan865x_set_hw_macaddr(netdev);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (lan865x_hw_enable(priv) != 0) {
> +		if (netif_msg_ifup(priv))
> +			netdev_err(netdev, "Failed to enable hardware\n");
> +		return -ENODEV;
> +	}
> +	netif_start_queue(netdev);
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops lan865x_netdev_ops = {
> +	.ndo_open		= lan865x_net_open,
> +	.ndo_stop		= lan865x_net_close,
> +	.ndo_start_xmit		= lan865x_send_packet,
> +	.ndo_set_rx_mode	= lan865x_set_multicast_list,
> +	.ndo_set_mac_address	= lan865x_set_mac_address,
> +	.ndo_tx_timeout		= lan865x_tx_timeout,
> +	.ndo_validate_addr	= eth_validate_addr,
> +};
> +
> +static int lan865x_get_dt_data(struct lan865x_priv *priv)
> +{
> +	struct spi_device *spi = priv->spi;
> +	int ret;
> +
> +	if (of_property_present(spi->dev.of_node, "oa-chunk-size")) {
> +		ret = of_property_read_u32(spi->dev.of_node, "oa-chunk-size",
> +					   &priv->cps);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		priv->cps = 64;
> +		dev_info(&spi->dev, "Property oa-chunk-size is not found in dt and proceeding with the size 64\n");
> +	}
> +
> +	if (of_property_present(spi->dev.of_node, "oa-tx-cut-through"))
> +		priv->txcte = true;
> +	else
> +		dev_info(&spi->dev, "Property oa-tx-cut-through is not found in dt and proceeding with tx store and forward mode\n");
> +
> +	if (of_property_present(spi->dev.of_node, "oa-rx-cut-through"))
> +		priv->rxcte = true;
> +	else
> +		dev_info(&spi->dev, "Property oa-rx-cut-through is not found in dt and proceeding with rx store and forward mode\n");
> +
> +	if (of_property_present(spi->dev.of_node, "oa-protected"))
> +		priv->protected = true;
> +	else
> +		dev_info(&spi->dev, "Property oa-protected is not found in dt and proceeding with protection enabled\n");
> +
> +	return 0;
> +}
> +
> +static int lan865x_probe(struct spi_device *spi)
> +{
> +	struct net_device *netdev;
> +	struct lan865x_priv *priv;
> +	u32 regval;
> +	int ret;
> +
> +	netdev = alloc_etherdev(sizeof(struct lan865x_priv));
> +	if (!netdev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(netdev);
> +	priv->netdev = netdev;
> +	priv->spi = spi;
> +	priv->msg_enable = 0;
> +	spi_set_drvdata(spi, priv);
> +	SET_NETDEV_DEV(netdev, &spi->dev);
> +
> +	ret = lan865x_get_dt_data(priv);
> +	if (ret)
> +		return ret;

Has not free allocated netdev.

> +
> +	spi->rt = true;
> +	spi_setup(spi);
> +
> +	priv->tc6 = oa_tc6_init(spi, netdev);
> +	if (!priv->tc6) {
> +		ret = -ENOMEM;
> +		goto error_oa_tc6_init;
> +	}
> +
> +	if (priv->cps == 32) {
> +		regval = CCS_Q0_TX_CFG_32;
> +		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
> +		if (ret)
> +			return ret;

No resources recycle. Please check.

> +
> +		regval = CCS_Q0_RX_CFG_32;
> +		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
> +		if (ret)
> +			return ret;

No resources recycle. Please check.

> +	}
> +
> +	if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
> +			     priv->rxcte))
> +		goto err_macphy_config;
> +
> +	ret = lan865x_phy_init(priv);
> +	if (ret)
> +		goto error_phy;
> +
> +	if (device_get_ethdev_address(&spi->dev, netdev))
> +		eth_hw_addr_random(netdev);
> +
> +	ret = lan865x_set_hw_macaddr(netdev);
> +	if (ret) {
> +		if (netif_msg_probe(priv))
> +			dev_err(&spi->dev, "Failed to configure MAC");
> +		goto error_set_mac;
> +	}
> +
> +	netdev->if_port = IF_PORT_10BASET;
> +	netdev->irq = spi->irq;
> +	netdev->netdev_ops = &lan865x_netdev_ops;
> +	netdev->watchdog_timeo = TX_TIMEOUT;
> +	netdev->ethtool_ops = &lan865x_ethtool_ops;
> +	ret = register_netdev(netdev);
> +	if (ret) {
> +		if (netif_msg_probe(priv))
> +			dev_err(&spi->dev, "Register netdev failed (ret = %d)",
> +				ret);
> +		goto error_netdev_register;
> +	}
> +
> +	phy_start(priv->phydev);
> +	return 0;
> +
> +error_netdev_register:
> +error_set_mac:
> +	phy_disconnect(priv->phydev);
> +	mdiobus_unregister(priv->mdiobus);
> +	mdiobus_free(priv->mdiobus);
> +error_phy:
> +err_macphy_config:
> +	oa_tc6_deinit(priv->tc6);
> +error_oa_tc6_init:
> +	free_netdev(priv->netdev);
> +	return ret;
> +}
> +
> +static void lan865x_remove(struct spi_device *spi)
> +{
> +	struct lan865x_priv *priv = spi_get_drvdata(spi);
> +
> +	phy_stop(priv->phydev);
> +	phy_disconnect(priv->phydev);
> +	mdiobus_unregister(priv->mdiobus);
> +	mdiobus_free(priv->mdiobus);
> +	unregister_netdev(priv->netdev);
> +	if (oa_tc6_deinit(priv->tc6))
> +		dev_err(&spi->dev, "Failed to deinitialize oa tc6\n");
> +	free_netdev(priv->netdev);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lan865x_dt_ids[] = {
> +	{ .compatible = "microchip,lan865x" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id lan865x_acpi_ids[] = {
> +	{ .id = "LAN865X",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, lan865x_acpi_ids);
> +#endif
> +
> +static struct spi_driver lan865x_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +#ifdef CONFIG_OF
> +		.of_match_table = lan865x_dt_ids,
> +#endif
> +#ifdef CONFIG_ACPI
> +		   .acpi_match_table = ACPI_PTR(lan865x_acpi_ids),
> +#endif
> +	 },
> +	.probe = lan865x_probe,
> +	.remove = lan865x_remove,
> +};
> +module_spi_driver(lan865x_driver);
> +
> +MODULE_DESCRIPTION(DRV_NAME " 10Base-T1S MACPHY Ethernet Driver");
> +MODULE_AUTHOR("Parthiban Veerasooran <[e-mail]>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:" DRV_NAME);
>

Parthiban Veerasooran Sept. 12, 2023, 10:53 a.m. UTC | #3

Hi Simon,

Thank you for reviewing the patch.

On 10/09/23 11:14 pm, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Sep 08, 2023 at 07:59:18PM +0530, Parthiban Veerasooran wrote:
>> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
>> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
>> MAC integration provides the low pin count standard SPI interface to any
>> microcontroller therefore providing Ethernet functionality without
>> requiring MAC integration within the microcontroller. The LAN8650/1
>> operates as an SPI client supporting SCLK clock rates up to a maximum of
>> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
>> frames) and control (register access).
>>
>> By default, the chunk data payload is 64 bytes in size. A smaller payload
>> data size of 32 bytes is also supported and may be configured in the
>> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
>> register. Changing the chunk payload size requires the LAN8650/1 be reset
>> and shall not be done during normal operation.
>>
>> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
>> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
>> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
>> PHY and MAC are connected via an internal Media Independent Interface
>> (MII).
>>
>> Signed-off-by: Parthiban Veerasooran <[e-mail]>
> 
> Hi Parthiban,
> 
> thanks for your patches.
> Some minor feedback on this one follows.
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c
> 
> ...
> 
>> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
>> +{
>> +     u32 regval;
>> +     bool ret;
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     const u8 *mac = netdev->dev_addr;
> 
> Please arrange local variables in Networking code in reverse xmas tree
> order - longest line to shortest.
> 
> This tool can be of assistance here:
> https://github.com/ecree-solarflare/xmastree
Sure will do it. Somehow this area got escaped from my eyes.
> 
> ...
> 
>> +static int lan865x_probe(struct spi_device *spi)
>> +{
>> +     struct net_device *netdev;
>> +     struct lan865x_priv *priv;
>> +     u32 regval;
>> +     int ret;
>> +
>> +     netdev = alloc_etherdev(sizeof(struct lan865x_priv));
>> +     if (!netdev)
>> +             return -ENOMEM;
>> +
>> +     priv = netdev_priv(netdev);
>> +     priv->netdev = netdev;
>> +     priv->spi = spi;
>> +     priv->msg_enable = 0;
>> +     spi_set_drvdata(spi, priv);
>> +     SET_NETDEV_DEV(netdev, &spi->dev);
>> +
>> +     ret = lan865x_get_dt_data(priv);
>> +     if (ret)
>> +             return ret;
>> +
>> +     spi->rt = true;
>> +     spi_setup(spi);
>> +
>> +     priv->tc6 = oa_tc6_init(spi, netdev);
>> +     if (!priv->tc6) {
>> +             ret = -ENOMEM;
>> +             goto error_oa_tc6_init;
>> +     }
>> +
>> +     if (priv->cps == 32) {
>> +             regval = CCS_Q0_TX_CFG_32;
>> +             ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             regval = CCS_Q0_RX_CFG_32;
>> +             ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
>> +                          priv->rxcte))
>> +             goto err_macphy_config;
> 
> Jumping to err_macphy_config will result in this function returning ret.
> However, ret will be 0 at this point. Perhaps it should be set to an
> error value.
Ah yes, thanks for pointing it out. Will correct in the next version.

Best Regards,
Parthiban V
> 
> Flagged by Smatch.
> 
>> +
>> +     ret = lan865x_phy_init(priv);
>> +     if (ret)
>> +             goto error_phy;
>> +
>> +     if (device_get_ethdev_address(&spi->dev, netdev))
>> +             eth_hw_addr_random(netdev);
>> +
>> +     ret = lan865x_set_hw_macaddr(netdev);
>> +     if (ret) {
>> +             if (netif_msg_probe(priv))
>> +                     dev_err(&spi->dev, "Failed to configure MAC");
>> +             goto error_set_mac;
>> +     }
>> +
>> +     netdev->if_port = IF_PORT_10BASET;
>> +     netdev->irq = spi->irq;
>> +     netdev->netdev_ops = &lan865x_netdev_ops;
>> +     netdev->watchdog_timeo = TX_TIMEOUT;
>> +     netdev->ethtool_ops = &lan865x_ethtool_ops;
>> +     ret = register_netdev(netdev);
>> +     if (ret) {
>> +             if (netif_msg_probe(priv))
>> +                     dev_err(&spi->dev, "Register netdev failed (ret = %d)",
>> +                             ret);
>> +             goto error_netdev_register;
>> +     }
>> +
>> +     phy_start(priv->phydev);
>> +     return 0;
>> +
>> +error_netdev_register:
>> +error_set_mac:
>> +     phy_disconnect(priv->phydev);
>> +     mdiobus_unregister(priv->mdiobus);
>> +     mdiobus_free(priv->mdiobus);
>> +error_phy:
>> +err_macphy_config:
>> +     oa_tc6_deinit(priv->tc6);
>> +error_oa_tc6_init:
>> +     free_netdev(priv->netdev);
>> +     return ret;
>> +}
> 
> 
> ...

Parthiban Veerasooran Sept. 12, 2023, 11:41 a.m. UTC | #4

Hi William,

On 11/09/23 6:47 pm, Ziyang Xuan (William) wrote:
> [Some people who received this message don't often get email from [e-mail]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
>> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
>> MAC integration provides the low pin count standard SPI interface to any
>> microcontroller therefore providing Ethernet functionality without
>> requiring MAC integration within the microcontroller. The LAN8650/1
>> operates as an SPI client supporting SCLK clock rates up to a maximum of
>> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
>> frames) and control (register access).
>>
>> By default, the chunk data payload is 64 bytes in size. A smaller payload
>> data size of 32 bytes is also supported and may be configured in the
>> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
>> register. Changing the chunk payload size requires the LAN8650/1 be reset
>> and shall not be done during normal operation.
>>
>> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
>> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
>> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
>> PHY and MAC are connected via an internal Media Independent Interface
>> (MII).
>>
>> Signed-off-by: Parthiban Veerasooran <[e-mail]>
>> ---
>>   MAINTAINERS                              |   6 +
>>   drivers/net/ethernet/microchip/Kconfig   |  10 +
>>   drivers/net/ethernet/microchip/Makefile  |   3 +
>>   drivers/net/ethernet/microchip/lan865x.c | 589 +++++++++++++++++++++++
>>   4 files changed, 608 insertions(+)
>>   create mode 100644 drivers/net/ethernet/microchip/lan865x.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c54454c7e7a1..666c042a15b2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13879,6 +13879,12 @@ L:   [e-mail]
>>   S:   Maintained
>>   F:   drivers/net/ethernet/microchip/lan743x_*
>>
>> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
>> +M:   Parthiban Veerasooran <[e-mail]>
>> +L:   [e-mail]
>> +S:   Maintained
>> +F:   drivers/net/ethernet/microchip/lan865x.c
>> +
>>   MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
>>   M:   Arun Ramadoss <[e-mail]>
>>   R:   [e-mail]
>> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
>> index 329e374b9539..d99be51b99f1 100644
>> --- a/drivers/net/ethernet/microchip/Kconfig
>> +++ b/drivers/net/ethernet/microchip/Kconfig
>> @@ -59,4 +59,14 @@ source "drivers/net/ethernet/microchip/lan966x/Kconfig"
>>   source "drivers/net/ethernet/microchip/sparx5/Kconfig"
>>   source "drivers/net/ethernet/microchip/vcap/Kconfig"
>>
>> +config LAN865X
>> +     tristate "LAN865x support"
>> +     depends on SPI
>> +     help
>> +               Support for the Microchip LAN8650/1 Rev.B0 Ethernet chip. It uses OPEN
>> +       Alliance 10BASE-T1x Serial Interface specification.
>> +
>> +               To compile this driver as a module, choose M here. The module will be
>> +          called lan865x.
>> +
>>   endif # NET_VENDOR_MICROCHIP
>> diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
>> index bbd349264e6f..315e850b2b26 100644
>> --- a/drivers/net/ethernet/microchip/Makefile
>> +++ b/drivers/net/ethernet/microchip/Makefile
>> @@ -12,3 +12,6 @@ lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
>>   obj-$(CONFIG_LAN966X_SWITCH) += lan966x/
>>   obj-$(CONFIG_SPARX5_SWITCH) += sparx5/
>>   obj-$(CONFIG_VCAP) += vcap/
>> +
>> +obj-$(CONFIG_LAN865X) += lan865x_t1s.o
>> +lan865x_t1s-objs := lan865x.o ../oa_tc6.o
>> diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c
>> new file mode 100644
>> index 000000000000..3c8ebf4c258f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/microchip/lan865x.c
>> @@ -0,0 +1,589 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Microchip's LAN865x 10BASE-T1S MAC-PHY driver
>> + *
>> + * Author: Parthiban Veerasooran <[e-mail]>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/etherdevice.h>
>> +#include <linux/mdio.h>
>> +#include <linux/phy.h>
>> +#include <linux/of.h>
>> +#include <linux/oa_tc6.h>
>> +
>> +#define DRV_NAME             "lan865x"
>> +#define DRV_VERSION          "0.1"
>> +
>> +#define REG_STDR_RESET               0x00000003
>> +#define REG_MAC_ADDR_BO              0x00010022
>> +#define REG_MAC_ADDR_L               0x00010024
>> +#define REG_MAC_ADDR_H               0x00010025
>> +#define REG_MAC_NW_CTRL         0x00010000
>> +#define REG_MAC_NW_CONFIG    0x00010001
>> +#define REG_MAC_HASHL                0x00010020
>> +#define REG_MAC_HASHH                0x00010021
>> +#define REG_MAC_ADDR_BO              0x00010022
>> +#define REG_MAC_ADDR_L               0x00010024
>> +#define REG_MAC_ADDR_H               0x00010025
>> +
>> +#define CCS_Q0_TX_CFG                0x000A0081
>> +#define CCS_Q0_RX_CFG                0x000A0082
>> +
>> +/* Buffer configuration for 32-bytes chunk payload */
>> +#define CCS_Q0_TX_CFG_32     0x70000000
>> +#define CCS_Q0_RX_CFG_32     0x30000C00
>> +
>> +#define NW_RX_STATUS         BIT(2)
>> +#define NW_TX_STATUS         BIT(3)
>> +#define NW_DISABLE           0x0
>> +
>> +#define MAC_PROMISCUOUS_MODE BIT(4)
>> +#define MAC_MULTICAST_MODE   BIT(6)
>> +#define MAC_UNICAST_MODE     BIT(7)
>> +
>> +#define TX_TIMEOUT           (4 * HZ)
>> +#define LAN865X_MSG_DEFAULT  \
>> +     (NETIF_MSG_PROBE | NETIF_MSG_IFUP | NETIF_MSG_IFDOWN | NETIF_MSG_LINK)
>> +
>> +struct lan865x_priv {
>> +     struct net_device *netdev;
>> +     struct spi_device *spi;
>> +     struct oa_tc6 *tc6;
>> +     struct mii_bus *mdiobus;
>> +     struct phy_device *phydev;
>> +     struct device *dev;
>> +     u32 msg_enable;
>> +     bool txcte;
>> +     bool rxcte;
>> +     u32 cps;
>> +     bool protected;
>> +};
>> +
>> +static void lan865x_handle_link_change(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     phy_print_status(priv->phydev);
>> +}
>> +
>> +static int lan865x_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
>> +{
>> +     struct lan865x_priv *priv = bus->priv;
>> +     u32 regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_read_register(priv->tc6, 0xFF00 | (idx & 0xFF), &regval, 1);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return regval;
>> +}
>> +
>> +static int lan865x_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
>> +                              u16 regval)
>> +{
>> +     struct lan865x_priv *priv = bus->priv;
>> +     u32 value = regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_write_register(priv->tc6, 0xFF00 | (idx & 0xFF), &value, 1);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_phy_init(struct lan865x_priv *priv)
>> +{
>> +     int ret;
>> +
>> +     priv->mdiobus = mdiobus_alloc();
>> +     if (!priv->mdiobus) {
>> +             netdev_err(priv->netdev, "MDIO bus alloc failed\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     priv->mdiobus->phy_mask = ~(u32)BIT(1);
>> +     priv->mdiobus->priv = priv;
>> +     priv->mdiobus->read = lan865x_mdiobus_read;
>> +     priv->mdiobus->write = lan865x_mdiobus_write;
>> +     priv->mdiobus->name = "lan865x-mdiobus";
>> +     priv->mdiobus->parent = priv->dev;
>> +
>> +     snprintf(priv->mdiobus->id, ARRAY_SIZE(priv->mdiobus->id),
>> +              "%s", dev_name(&priv->spi->dev));
>> +
>> +     ret = mdiobus_register(priv->mdiobus);
>> +     if (ret) {
>> +             netdev_err(priv->netdev, "Could not register MDIO bus\n");
>> +             mdiobus_free(priv->mdiobus);
>> +             return ret;
>> +     }
>> +     priv->phydev = phy_find_first(priv->mdiobus);
>> +     if (!priv->phydev) {
>> +             netdev_err(priv->netdev, "No PHY found\n");
>> +             mdiobus_unregister(priv->mdiobus);
>> +             mdiobus_free(priv->mdiobus);
>> +             return -ENODEV;
>> +     }
>> +     priv->phydev->is_internal = true;
>> +     ret = phy_connect_direct(priv->netdev, priv->phydev,
>> +                              &lan865x_handle_link_change,
>> +                              PHY_INTERFACE_MODE_INTERNAL);
>> +     if (ret) {
>> +             netdev_err(priv->netdev, "Can't attach PHY to %s\n", priv->mdiobus->id);
>> +             return ret;
> Here return directly without above resources recycle. Please check if it is correct.
> If it is needed, it is recommended to use error labels to avoid duplicate code.
Ok noted. Will correct it in the next revision.
> 
>> +     }
>> +     phy_attached_info(priv->phydev);
>> +     return ret;
>> +}
>> +
>> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
>> +{
>> +     u32 regval;
>> +     bool ret;
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     const u8 *mac = netdev->dev_addr;
>> +
>> +     ret = oa_tc6_read_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1);
>> +     if (ret)
>> +             goto error_mac;
>> +     if ((regval & NW_TX_STATUS) | (regval & NW_RX_STATUS)) {
>> +             if (netif_msg_drv(priv))
>> +                     netdev_warn(netdev, "Hardware must be disabled for MAC setting\n");
>> +             return -EBUSY;
>> +     }
>> +     /* MAC address setting */
>> +     regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) |
>> +             mac[0];
>> +     ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_L, &regval, 1);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     regval = (mac[5] << 8) | mac[4];
>> +     ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_H, &regval, 1);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     regval = (mac[5] << 24) | (mac[4] << 16) |
>> +             (mac[3] << 8) | mac[2];
>> +     ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_BO, &regval, 1);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     return 0;
>> +
>> +error_mac:
>> +     return -ENODEV;
> 
> No resource recycle, goto and error label are not needed. What's more,
> the same label corresponds to different errors.
Ah yes, will correct it in the next revision.
> 
>> +}
>> +
>> +static int
>> +lan865x_set_link_ksettings(struct net_device *netdev,
>> +                        const struct ethtool_link_ksettings *cmd)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret = 0;
>> +
>> +     if (cmd->base.autoneg != AUTONEG_DISABLE ||
>> +         cmd->base.speed != SPEED_10 || cmd->base.duplex != DUPLEX_HALF) {
>> +             if (netif_msg_link(priv))
>> +                     netdev_warn(netdev, "Unsupported link setting");
>> +             ret = -EOPNOTSUPP;
>> +     } else {
>> +             if (netif_msg_link(priv))
>> +                     netdev_warn(netdev, "Hardware must be disabled to set link mode");
>> +             ret = -EBUSY;
>> +     }
>> +     return ret;
>> +}
>> +
>> +static int
>> +lan865x_get_link_ksettings(struct net_device *netdev,
>> +                        struct ethtool_link_ksettings *cmd)
>> +{
>> +     ethtool_link_ksettings_zero_link_mode(cmd, supported);
>> +     ethtool_link_ksettings_add_link_mode(cmd, supported, 10baseT_Half);
>> +     ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
>> +
>> +     cmd->base.speed = SPEED_10;
>> +     cmd->base.duplex = DUPLEX_HALF;
>> +     cmd->base.port  = PORT_TP;
>> +     cmd->base.autoneg = AUTONEG_DISABLE;
>> +
>> +     return 0;
>> +}
>> +
>> +static void lan865x_set_msglevel(struct net_device *netdev, u32 val)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     priv->msg_enable = val;
>> +}
>> +
>> +static u32 lan865x_get_msglevel(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     return priv->msg_enable;
>> +}
>> +
>> +static void
>> +lan865x_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *info)
>> +{
>> +     strscpy(info->driver, DRV_NAME, sizeof(info->driver));
>> +     strscpy(info->version, DRV_VERSION, sizeof(info->version));
>> +     strscpy(info->bus_info,
>> +             dev_name(netdev->dev.parent), sizeof(info->bus_info));
>> +}
>> +
>> +static const struct ethtool_ops lan865x_ethtool_ops = {
>> +     .get_drvinfo    = lan865x_get_drvinfo,
>> +     .get_msglevel   = lan865x_get_msglevel,
>> +     .set_msglevel   = lan865x_set_msglevel,
>> +     .get_link_ksettings = lan865x_get_link_ksettings,
>> +     .set_link_ksettings = lan865x_set_link_ksettings,
>> +};
>> +
>> +static void lan865x_tx_timeout(struct net_device *netdev, unsigned int txqueue)
>> +{
>> +     netdev->stats.tx_errors++;
>> +}
>> +
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> +     struct sockaddr *address = addr;
>> +
>> +     if (netif_running(netdev))
>> +             return -EBUSY;
>> +     if (!is_valid_ether_addr(address->sa_data))
>> +             return -EADDRNOTAVAIL;
>> +
>> +     eth_hw_addr_set(netdev, address->sa_data);
>> +     return lan865x_set_hw_macaddr(netdev);
>> +}
>> +
>> +static u32 lan865x_hash(u8 addr[ETH_ALEN])
>> +{
>> +     return (ether_crc(ETH_ALEN, addr) >> 26) & 0x3f;
>> +}
>> +
>> +static void lan865x_set_multicast_list(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     u32 regval = 0;
>> +
>> +     if (netdev->flags & IFF_PROMISC) {
>> +             /* Enabling promiscuous mode */
>> +             regval |= MAC_PROMISCUOUS_MODE;
>> +             regval &= (~MAC_MULTICAST_MODE);
>> +             regval &= (~MAC_UNICAST_MODE);
>> +     } else if (netdev->flags & IFF_ALLMULTI) {
>> +             /* Enabling all multicast mode */
>> +             regval &= (~MAC_PROMISCUOUS_MODE);
>> +             regval |= MAC_MULTICAST_MODE;
>> +             regval &= (~MAC_UNICAST_MODE);
>> +     } else if (!netdev_mc_empty(netdev)) {
>> +             /* Enabling specific multicast addresses */
>> +             struct netdev_hw_addr *ha;
>> +             u32 hash_lo = 0;
>> +             u32 hash_hi = 0;
>> +
>> +             netdev_for_each_mc_addr(ha, netdev) {
>> +                     u32 bit_num = lan865x_hash(ha->addr);
>> +                     u32 mask = 1 << (bit_num & 0x1f);
>> +
>> +                     if (bit_num & 0x20)
>> +                             hash_hi |= mask;
>> +                     else
>> +                             hash_lo |= mask;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &hash_hi, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &hash_lo, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +             regval &= (~MAC_PROMISCUOUS_MODE);
>> +             regval &= (~MAC_MULTICAST_MODE);
>> +             regval |= MAC_UNICAST_MODE;
>> +     } else {
>> +             /* enabling local mac address only */
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &regval, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &regval, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +     }
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CONFIG, &regval, 1)) {
>> +             if (netif_msg_timer(priv))
>> +                     netdev_err(netdev, "Failed to enable promiscuous mode");
>> +     }
>> +}
>> +
>> +static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
>> +                                    struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     return oa_tc6_send_eth_pkt(priv->tc6, skb);
>> +}
>> +
>> +static int lan865x_hw_disable(struct lan865x_priv *priv)
>> +{
>> +     u32 regval = NW_DISABLE;
>> +
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_net_close(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret;
>> +
>> +     netif_stop_queue(netdev);
>> +     ret = lan865x_hw_disable(priv);
>> +     if (ret) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Failed to disable the hardware\n");
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_hw_enable(struct lan865x_priv *priv)
>> +{
>> +     u32 regval = NW_TX_STATUS | NW_RX_STATUS;
>> +
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_net_open(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret;
>> +
>> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
>> +             return -EADDRNOTAVAIL;
>> +     }
>> +     if (lan865x_hw_disable(priv)) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Failed to disable the hardware\n");
>> +             return -ENODEV;
>> +     }
>> +     ret = lan865x_set_hw_macaddr(netdev);
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     if (lan865x_hw_enable(priv) != 0) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Failed to enable hardware\n");
>> +             return -ENODEV;
>> +     }
>> +     netif_start_queue(netdev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct net_device_ops lan865x_netdev_ops = {
>> +     .ndo_open               = lan865x_net_open,
>> +     .ndo_stop               = lan865x_net_close,
>> +     .ndo_start_xmit         = lan865x_send_packet,
>> +     .ndo_set_rx_mode        = lan865x_set_multicast_list,
>> +     .ndo_set_mac_address    = lan865x_set_mac_address,
>> +     .ndo_tx_timeout         = lan865x_tx_timeout,
>> +     .ndo_validate_addr      = eth_validate_addr,
>> +};
>> +
>> +static int lan865x_get_dt_data(struct lan865x_priv *priv)
>> +{
>> +     struct spi_device *spi = priv->spi;
>> +     int ret;
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-chunk-size")) {
>> +             ret = of_property_read_u32(spi->dev.of_node, "oa-chunk-size",
>> +                                        &priv->cps);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else {
>> +             priv->cps = 64;
>> +             dev_info(&spi->dev, "Property oa-chunk-size is not found in dt and proceeding with the size 64\n");
>> +     }
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-tx-cut-through"))
>> +             priv->txcte = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-tx-cut-through is not found in dt and proceeding with tx store and forward mode\n");
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-rx-cut-through"))
>> +             priv->rxcte = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-rx-cut-through is not found in dt and proceeding with rx store and forward mode\n");
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-protected"))
>> +             priv->protected = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-protected is not found in dt and proceeding with protection enabled\n");
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_probe(struct spi_device *spi)
>> +{
>> +     struct net_device *netdev;
>> +     struct lan865x_priv *priv;
>> +     u32 regval;
>> +     int ret;
>> +
>> +     netdev = alloc_etherdev(sizeof(struct lan865x_priv));
>> +     if (!netdev)
>> +             return -ENOMEM;
>> +
>> +     priv = netdev_priv(netdev);
>> +     priv->netdev = netdev;
>> +     priv->spi = spi;
>> +     priv->msg_enable = 0;
>> +     spi_set_drvdata(spi, priv);
>> +     SET_NETDEV_DEV(netdev, &spi->dev);
>> +
>> +     ret = lan865x_get_dt_data(priv);
>> +     if (ret)
>> +             return ret;
> 
> Has not free allocated netdev.
Ok noted, will correct it in the next revision.
> 
>> +
>> +     spi->rt = true;
>> +     spi_setup(spi);
>> +
>> +     priv->tc6 = oa_tc6_init(spi, netdev);
>> +     if (!priv->tc6) {
>> +             ret = -ENOMEM;
>> +             goto error_oa_tc6_init;
>> +     }
>> +
>> +     if (priv->cps == 32) {
>> +             regval = CCS_Q0_TX_CFG_32;
>> +             ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
>> +             if (ret)
>> +                     return ret;
> 
> No resources recycle. Please check.
Ok noted, will correct it in the next revision.
> 
>> +
>> +             regval = CCS_Q0_RX_CFG_32;
>> +             ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
>> +             if (ret)
>> +                     return ret;
> 
> No resources recycle. Please check.
Ok noted, will correct it in the next revision.

Best Regards,
Parthiban V
> 
>> +     }
>> +
>> +     if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
>> +                          priv->rxcte))
>> +             goto err_macphy_config;
>> +
>> +     ret = lan865x_phy_init(priv);
>> +     if (ret)
>> +             goto error_phy;
>> +
>> +     if (device_get_ethdev_address(&spi->dev, netdev))
>> +             eth_hw_addr_random(netdev);
>> +
>> +     ret = lan865x_set_hw_macaddr(netdev);
>> +     if (ret) {
>> +             if (netif_msg_probe(priv))
>> +                     dev_err(&spi->dev, "Failed to configure MAC");
>> +             goto error_set_mac;
>> +     }
>> +
>> +     netdev->if_port = IF_PORT_10BASET;
>> +     netdev->irq = spi->irq;
>> +     netdev->netdev_ops = &lan865x_netdev_ops;
>> +     netdev->watchdog_timeo = TX_TIMEOUT;
>> +     netdev->ethtool_ops = &lan865x_ethtool_ops;
>> +     ret = register_netdev(netdev);
>> +     if (ret) {
>> +             if (netif_msg_probe(priv))
>> +                     dev_err(&spi->dev, "Register netdev failed (ret = %d)",
>> +                             ret);
>> +             goto error_netdev_register;
>> +     }
>> +
>> +     phy_start(priv->phydev);
>> +     return 0;
>> +
>> +error_netdev_register:
>> +error_set_mac:
>> +     phy_disconnect(priv->phydev);
>> +     mdiobus_unregister(priv->mdiobus);
>> +     mdiobus_free(priv->mdiobus);
>> +error_phy:
>> +err_macphy_config:
>> +     oa_tc6_deinit(priv->tc6);
>> +error_oa_tc6_init:
>> +     free_netdev(priv->netdev);
>> +     return ret;
>> +}
>> +
>> +static void lan865x_remove(struct spi_device *spi)
>> +{
>> +     struct lan865x_priv *priv = spi_get_drvdata(spi);
>> +
>> +     phy_stop(priv->phydev);
>> +     phy_disconnect(priv->phydev);
>> +     mdiobus_unregister(priv->mdiobus);
>> +     mdiobus_free(priv->mdiobus);
>> +     unregister_netdev(priv->netdev);
>> +     if (oa_tc6_deinit(priv->tc6))
>> +             dev_err(&spi->dev, "Failed to deinitialize oa tc6\n");
>> +     free_netdev(priv->netdev);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id lan865x_dt_ids[] = {
>> +     { .compatible = "microchip,lan865x" },
>> +     { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lan865x_acpi_ids[] = {
>> +     { .id = "LAN865X",
>> +     },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(acpi, lan865x_acpi_ids);
>> +#endif
>> +
>> +static struct spi_driver lan865x_driver = {
>> +     .driver = {
>> +             .name = DRV_NAME,
>> +#ifdef CONFIG_OF
>> +             .of_match_table = lan865x_dt_ids,
>> +#endif
>> +#ifdef CONFIG_ACPI
>> +                .acpi_match_table = ACPI_PTR(lan865x_acpi_ids),
>> +#endif
>> +      },
>> +     .probe = lan865x_probe,
>> +     .remove = lan865x_remove,
>> +};
>> +module_spi_driver(lan865x_driver);
>> +
>> +MODULE_DESCRIPTION(DRV_NAME " 10Base-T1S MACPHY Ethernet Driver");
>> +MODULE_AUTHOR("Parthiban Veerasooran <[e-mail]>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("spi:" DRV_NAME);
>>

Andrew Lunn Sept. 14, 2023, 1:51 a.m. UTC | #5

> +#define DRV_VERSION		"0.1"

This is pointless. The ethtool code will fill in the git hash which is
a much more useful value to have.

> +static void lan865x_handle_link_change(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +
> +	phy_print_status(priv->phydev);
> +}
> +
> +static int lan865x_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
> +{
> +	struct lan865x_priv *priv = bus->priv;
> +	u32 regval;
> +	bool ret;
> +
> +	ret = oa_tc6_read_register(priv->tc6, 0xFF00 | (idx & 0xFF), &regval, 1);
> +	if (ret)
> +		return -ENODEV;
> +
> +	return regval;
> +}
> +
> +static int lan865x_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
> +				 u16 regval)
> +{
> +	struct lan865x_priv *priv = bus->priv;
> +	u32 value = regval;
> +	bool ret;
> +
> +	ret = oa_tc6_write_register(priv->tc6, 0xFF00 | (idx & 0xFF), &value, 1);
> +	if (ret)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int lan865x_phy_init(struct lan865x_priv *priv)
> +{
> +	int ret;
> +
> +	priv->mdiobus = mdiobus_alloc();
> +	if (!priv->mdiobus) {
> +		netdev_err(priv->netdev, "MDIO bus alloc failed\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->mdiobus->phy_mask = ~(u32)BIT(1);
> +	priv->mdiobus->priv = priv;
> +	priv->mdiobus->read = lan865x_mdiobus_read;
> +	priv->mdiobus->write = lan865x_mdiobus_write;

The MDIO bus is part of the standard. So i would expect this to be in
the library. From what i remember, there are two different ways to
implement MDIO, either via the PHY registers being directly mapped
into the register space, or indirect like this. And i think there is a
status bit somewhere which tells you which is implemented? So please
move this code into the library, but check the status bit and return
ENODEV if the silicon does not actually implement this access method.

> +static int
> +lan865x_set_link_ksettings(struct net_device *netdev,
> +			   const struct ethtool_link_ksettings *cmd)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	int ret = 0;
> +
> +	if (cmd->base.autoneg != AUTONEG_DISABLE ||
> +	    cmd->base.speed != SPEED_10 || cmd->base.duplex != DUPLEX_HALF) {
> +		if (netif_msg_link(priv))
> +			netdev_warn(netdev, "Unsupported link setting");
> +		ret = -EOPNOTSUPP;
> +	} else {
> +		if (netif_msg_link(priv))
> +			netdev_warn(netdev, "Hardware must be disabled to set link mode");
> +		ret = -EBUSY;
> +	}
> +	return ret;

I would expect to see a call to phy_ethtool_ksettings_set()
here. phylib should be able to do some of the validation.

> +}
> +
> +static int
> +lan865x_get_link_ksettings(struct net_device *netdev,
> +			   struct ethtool_link_ksettings *cmd)
> +{
> +	ethtool_link_ksettings_zero_link_mode(cmd, supported);
> +	ethtool_link_ksettings_add_link_mode(cmd, supported, 10baseT_Half);
> +	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
> +
> +	cmd->base.speed = SPEED_10;
> +	cmd->base.duplex = DUPLEX_HALF;
> +	cmd->base.port	= PORT_TP;
> +	cmd->base.autoneg = AUTONEG_DISABLE;
> +
> +	return 0;

phy_ethtool_ksettings_get().

I also think this can be moved along with the MDIO bus and PHY
handling into the library.

> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
> +{
> +	struct sockaddr *address = addr;
> +
> +	if (netif_running(netdev))
> +		return -EBUSY;
> +	if (!is_valid_ether_addr(address->sa_data))
> +		return -EADDRNOTAVAIL;

Does the core allow an invalid MAC address be passed to the driver?

> +
> +	eth_hw_addr_set(netdev, address->sa_data);
> +	return lan865x_set_hw_macaddr(netdev);
> +}
> +
> +static u32 lan865x_hash(u8 addr[ETH_ALEN])
> +{
> +	return (ether_crc(ETH_ALEN, addr) >> 26) & 0x3f;
> +}
> +
> +static void lan865x_set_multicast_list(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	u32 regval = 0;
> +
> +	if (netdev->flags & IFF_PROMISC) {
> +		/* Enabling promiscuous mode */
> +		regval |= MAC_PROMISCUOUS_MODE;
> +		regval &= (~MAC_MULTICAST_MODE);
> +		regval &= (~MAC_UNICAST_MODE);
> +	} else if (netdev->flags & IFF_ALLMULTI) {
> +		/* Enabling all multicast mode */
> +		regval &= (~MAC_PROMISCUOUS_MODE);
> +		regval |= MAC_MULTICAST_MODE;
> +		regval &= (~MAC_UNICAST_MODE);
> +	} else if (!netdev_mc_empty(netdev)) {
> +		/* Enabling specific multicast addresses */
> +		struct netdev_hw_addr *ha;
> +		u32 hash_lo = 0;
> +		u32 hash_hi = 0;
> +
> +		netdev_for_each_mc_addr(ha, netdev) {
> +			u32 bit_num = lan865x_hash(ha->addr);
> +			u32 mask = 1 << (bit_num & 0x1f);
> +
> +			if (bit_num & 0x20)
> +				hash_hi |= mask;
> +			else
> +				hash_lo |= mask;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &hash_hi, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashh");
> +			return;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &hash_lo, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashl");
> +			return;
> +		}
> +		regval &= (~MAC_PROMISCUOUS_MODE);
> +		regval &= (~MAC_MULTICAST_MODE);
> +		regval |= MAC_UNICAST_MODE;
> +	} else {
> +		/* enabling local mac address only */
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &regval, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashh");
> +			return;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &regval, 1)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashl");
> +			return;
> +		}
> +	}
> +	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CONFIG, &regval, 1)) {
> +		if (netif_msg_timer(priv))
> +			netdev_err(netdev, "Failed to enable promiscuous mode");
> +	}
> +}
> +
> +static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
> +				       struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +
> +	return oa_tc6_send_eth_pkt(priv->tc6, skb);
> +}
> +
> +static int lan865x_hw_disable(struct lan865x_priv *priv)
> +{
> +	u32 regval = NW_DISABLE;
> +
> +	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int lan865x_net_close(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	int ret;
> +
> +	netif_stop_queue(netdev);
> +	ret = lan865x_hw_disable(priv);
> +	if (ret) {
> +		if (netif_msg_ifup(priv))
> +			netdev_err(netdev, "Failed to disable the hardware\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan865x_hw_enable(struct lan865x_priv *priv)
> +{
> +	u32 regval = NW_TX_STATUS | NW_RX_STATUS;
> +
> +	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int lan865x_net_open(struct net_device *netdev)
> +{
> +	struct lan865x_priv *priv = netdev_priv(netdev);
> +	int ret;
> +
> +	if (!is_valid_ether_addr(netdev->dev_addr)) {
> +		if (netif_msg_ifup(priv))
> +			netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
> +		return -EADDRNOTAVAIL;

Using a random MAC address is the normal workaround for not having a
valid MAC address via OTP flash etc.


> +static int lan865x_get_dt_data(struct lan865x_priv *priv)
> +{
> +	struct spi_device *spi = priv->spi;
> +	int ret;
> +
> +	if (of_property_present(spi->dev.of_node, "oa-chunk-size")) {
> +		ret = of_property_read_u32(spi->dev.of_node, "oa-chunk-size",
> +					   &priv->cps);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		priv->cps = 64;
> +		dev_info(&spi->dev, "Property oa-chunk-size is not found in dt and proceeding with the size 64\n");
> +	}
> +
> +	if (of_property_present(spi->dev.of_node, "oa-tx-cut-through"))
> +		priv->txcte = true;
> +	else
> +		dev_info(&spi->dev, "Property oa-tx-cut-through is not found in dt and proceeding with tx store and forward mode\n");

Please remove all these dev_info() prints. The device tree binding
should make it clear what the defaults are when not specified in DT.

> +
> +	if (of_property_present(spi->dev.of_node, "oa-rx-cut-through"))
> +		priv->rxcte = true;
> +	else
> +		dev_info(&spi->dev, "Property oa-rx-cut-through is not found in dt and proceeding with rx store and forward mode\n");
> +
> +	if (of_property_present(spi->dev.of_node, "oa-protected"))
> +		priv->protected = true;
> +	else
> +		dev_info(&spi->dev, "Property oa-protected is not found in dt and proceeding with protection enabled\n");

Which of these are proprietary properties, and which are part of the
standard? Please move parsing all the standard properties into the
library.

> +static int lan865x_probe(struct spi_device *spi)
> +{

...

> +
> +	phy_start(priv->phydev);
> +	return 0;

phy_start() is normally done in open, not probe.

	    Andrew

Andrew Lunn Sept. 14, 2023, 1:55 a.m. UTC | #6

> +#define REG_STDR_RESET		0x00000003

This appears to be a standard register, so you should not need to
define it here.

> +#define REG_MAC_ADDR_BO		0x00010022
> +#define REG_MAC_ADDR_L		0x00010024
> +#define REG_MAC_ADDR_H		0x00010025
> +#define REG_MAC_NW_CTRL         0x00010000
> +#define REG_MAC_NW_CONFIG	0x00010001
> +#define REG_MAC_HASHL		0x00010020
> +#define REG_MAC_HASHH		0x00010021
> +#define REG_MAC_ADDR_BO		0x00010022
> +#define REG_MAC_ADDR_L		0x00010024
> +#define REG_MAC_ADDR_H		0x00010025
> +
> +#define CCS_Q0_TX_CFG		0x000A0081
> +#define CCS_Q0_RX_CFG		0x000A0082

These are proprietary vendor registers, so please add a prefix to make
this clear.

     Andrew

David Wretman Sept. 15, 2023, 1:01 p.m. UTC | #7

---
On Fri, Sep 08, 2023 at 07:59:18PM +0530, Parthiban Veerasooran wrote:
> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
> MAC integration provides the low pin count standard SPI interface to any
> microcontroller therefore providing Ethernet functionality without
> requiring MAC integration within the microcontroller. The LAN8650/1
> operates as an SPI client supporting SCLK clock rates up to a maximum of
> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
> frames) and control (register access).
> 
> By default, the chunk data payload is 64 bytes in size. A smaller payload
> data size of 32 bytes is also supported and may be configured in the
> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
> register. Changing the chunk payload size requires the LAN8650/1 be reset
> and shall not be done during normal operation.
> 
> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
> PHY and MAC are connected via an internal Media Independent Interface
> (MII).
> 
> Signed-off-by: Parthiban Veerasooran <[e-mail]>

Hi Parthiban,

Thanks for these patches.

One thing I am missing is settings for PLCA parameters. I feel that the
driver is a bit lacking as long as this is missing.

Adding support for the ethtool plca options would make this much more
complete.

Regards,
David

Parthiban Veerasooran Sept. 18, 2023, 11:22 a.m. UTC | #8

Hi David,

Thanks for your comments. Please see my reply below,

On 15/09/23 6:31 pm, David Wretman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> ---
> On Fri, Sep 08, 2023 at 07:59:18PM +0530, Parthiban Veerasooran wrote:
>> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
>> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
>> MAC integration provides the low pin count standard SPI interface to any
>> microcontroller therefore providing Ethernet functionality without
>> requiring MAC integration within the microcontroller. The LAN8650/1
>> operates as an SPI client supporting SCLK clock rates up to a maximum of
>> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
>> frames) and control (register access).
>>
>> By default, the chunk data payload is 64 bytes in size. A smaller payload
>> data size of 32 bytes is also supported and may be configured in the
>> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
>> register. Changing the chunk payload size requires the LAN8650/1 be reset
>> and shall not be done during normal operation.
>>
>> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
>> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
>> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
>> PHY and MAC are connected via an internal Media Independent Interface
>> (MII).
>>
>> Signed-off-by: Parthiban Veerasooran <[e-mail]>
> 
> Hi Parthiban,
> 
> Thanks for these patches.
> 
> One thing I am missing is settings for PLCA parameters. I feel that the
> driver is a bit lacking as long as this is missing.
No it is not missed. The driver still supports for setting the PLCA 
parameters using ethtool. This is just a MAC driver and its internal 
PHY's driver is already mainlined which has PLCA setting support. Please 
have a look in the below link which has the implementation.

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/microchip_t1s.c#L283

You can use the below ethtool command to set your PLCA settings.

$ ethtool --set-plca-cfg eth1 enable on node-id 0 node-cnt 8 to-tmr 0x20 
burst-cnt 0x0 burst-tmr 0x80
> 
> Adding support for the ethtool plca options would make this much more
> complete.
Hope the above explanation helps. Please let me know if I misunderstand 
your comment.

Best Regards,
Parthiban V
> 
> Regards,
> David
>

Parthiban Veerasooran Sept. 18, 2023, 11:23 a.m. UTC | #9

Hi Andrew,

On 14/09/23 7:25 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +#define REG_STDR_RESET               0x00000003
> 
> This appears to be a standard register, so you should not need to
> define it here.
Ah ok, will do it.
> 
>> +#define REG_MAC_ADDR_BO              0x00010022
>> +#define REG_MAC_ADDR_L               0x00010024
>> +#define REG_MAC_ADDR_H               0x00010025
>> +#define REG_MAC_NW_CTRL         0x00010000
>> +#define REG_MAC_NW_CONFIG    0x00010001
>> +#define REG_MAC_HASHL                0x00010020
>> +#define REG_MAC_HASHH                0x00010021
>> +#define REG_MAC_ADDR_BO              0x00010022
>> +#define REG_MAC_ADDR_L               0x00010024
>> +#define REG_MAC_ADDR_H               0x00010025
>> +
>> +#define CCS_Q0_TX_CFG                0x000A0081
>> +#define CCS_Q0_RX_CFG                0x000A0082
> 
> These are proprietary vendor registers, so please add a prefix to make
> this clear.
Sure, will add it in the next version.

Best Regards,
Parthiban V
> 
>       Andrew

Parthiban Veerasooran Sept. 19, 2023, 9:18 a.m. UTC | #10

Hi Andrew,

On 14/09/23 7:21 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +#define DRV_VERSION          "0.1"
> 
> This is pointless. The ethtool code will fill in the git hash which is
> a much more useful value to have.
Ah ok, so I can also remove the below code updating this info in the 
lan865x_get_drvinfo() function.

strscpy(info->version, DRV_VERSION, sizeof(info->version));
> 
>> +static void lan865x_handle_link_change(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     phy_print_status(priv->phydev);
>> +}
>> +
>> +static int lan865x_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
>> +{
>> +     struct lan865x_priv *priv = bus->priv;
>> +     u32 regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_read_register(priv->tc6, 0xFF00 | (idx & 0xFF), &regval, 1);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return regval;
>> +}
>> +
>> +static int lan865x_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
>> +                              u16 regval)
>> +{
>> +     struct lan865x_priv *priv = bus->priv;
>> +     u32 value = regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_write_register(priv->tc6, 0xFF00 | (idx & 0xFF), &value, 1);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_phy_init(struct lan865x_priv *priv)
>> +{
>> +     int ret;
>> +
>> +     priv->mdiobus = mdiobus_alloc();
>> +     if (!priv->mdiobus) {
>> +             netdev_err(priv->netdev, "MDIO bus alloc failed\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     priv->mdiobus->phy_mask = ~(u32)BIT(1);
>> +     priv->mdiobus->priv = priv;
>> +     priv->mdiobus->read = lan865x_mdiobus_read;
>> +     priv->mdiobus->write = lan865x_mdiobus_write;
> 
> The MDIO bus is part of the standard. So i would expect this to be in
> the library. From what i remember, there are two different ways to
> implement MDIO, either via the PHY registers being directly mapped
> into the register space, or indirect like this. And i think there is a
> status bit somewhere which tells you which is implemented? So please
> move this code into the library, but check the status bit and return
> ENODEV if the silicon does not actually implement this access method.
Sure, I can move this part to oa_tc6 lib. If I understand you correctly 
you are talking about the Standard Capabilities Register (0x0002) 
defined in the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface spec 
right? If so, the 9th bit of this register tells about Indirect PHY 
Register access Capability. Did you mean this bit? If so, this bit 
describes the below,

IPRAC - Indirect PHY Register Access Capability. Indicates if PHY 
registers are indirectly accessible through the MDIO/MDC registers MDIOACCn.

The MDIOACCn – MDIO Access Register 0-7 is defined under Register Memory 
Map 0 - Standard Control and Status Registers of the OA spec. As you 
said, some vendors may implement this and some may not be. But still we 
have PHY clause 45 registers indirect access through clause 22 registers 
namely 0xd and 0xe. Using them we will be able to create a MDIO virtual 
bus to access PHY clause 45 registers. We already have all the required 
API's provided by Linux for this approach.

https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/net/phy/phy-core.c#L529

Microchip's LAN865x doesn't support OA provided indirect access of PHY 
registers. But it supports PHY registers indirect access through clause 
22 registers namely 0xd and 0xe. Also it supports direct PHY register 
access within the SPI register memory space.

As the PLCA registers are already standardized and mainlined it will be 
more straight forward if the driver implements the PHY register indirect 
access using clause 22. Otherwise there will be an extra effort for 
mapping those PLCA registers into SPI memory space to access them 
directly. Also every vendor will have their own SPI memory space to map.

https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/net/phy/mdio-open-alliance.h

So my proposal would be, I stick with this standard implementation of 
indirect PHY register access through clause 22 registers 0xd and 0xe. I 
believe almost all the vendors will have this access. Later we will add 
this feature tested (Since Microchip's LAN865x doesn't support this, I 
can't test) if there is a vendor supports this. What do you think?
> 
>> +static int
>> +lan865x_set_link_ksettings(struct net_device *netdev,
>> +                        const struct ethtool_link_ksettings *cmd)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret = 0;
>> +
>> +     if (cmd->base.autoneg != AUTONEG_DISABLE ||
>> +         cmd->base.speed != SPEED_10 || cmd->base.duplex != DUPLEX_HALF) {
>> +             if (netif_msg_link(priv))
>> +                     netdev_warn(netdev, "Unsupported link setting");
>> +             ret = -EOPNOTSUPP;
>> +     } else {
>> +             if (netif_msg_link(priv))
>> +                     netdev_warn(netdev, "Hardware must be disabled to set link mode");
>> +             ret = -EBUSY;
>> +     }
>> +     return ret;
> 
> I would expect to see a call to phy_ethtool_ksettings_set()
> here. phylib should be able to do some of the validation.
Ah ok, doing the below will make the life easier.
.set_link_ksettings	= phy_ethtool_set_link_ksettings,
> 
>> +}
>> +
>> +static int
>> +lan865x_get_link_ksettings(struct net_device *netdev,
>> +                        struct ethtool_link_ksettings *cmd)
>> +{
>> +     ethtool_link_ksettings_zero_link_mode(cmd, supported);
>> +     ethtool_link_ksettings_add_link_mode(cmd, supported, 10baseT_Half);
>> +     ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
>> +
>> +     cmd->base.speed = SPEED_10;
>> +     cmd->base.duplex = DUPLEX_HALF;
>> +     cmd->base.port  = PORT_TP;
>> +     cmd->base.autoneg = AUTONEG_DISABLE;
>> +
>> +     return 0;
> 
> phy_ethtool_ksettings_get().
Yes,
.get_link_ksettings	= phy_ethtool_get_link_ksettings,
> 
> I also think this can be moved along with the MDIO bus and PHY
> handling into the library.
Ok will do that.
> 
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> +     struct sockaddr *address = addr;
>> +
>> +     if (netif_running(netdev))
>> +             return -EBUSY;
>> +     if (!is_valid_ether_addr(address->sa_data))
>> +             return -EADDRNOTAVAIL;
> 
> Does the core allow an invalid MAC address be passed to the driver?
Ah yes, it is not needed here.
> 
>> +
>> +     eth_hw_addr_set(netdev, address->sa_data);
>> +     return lan865x_set_hw_macaddr(netdev);
>> +}
>> +
>> +static u32 lan865x_hash(u8 addr[ETH_ALEN])
>> +{
>> +     return (ether_crc(ETH_ALEN, addr) >> 26) & 0x3f;
>> +}
>> +
>> +static void lan865x_set_multicast_list(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     u32 regval = 0;
>> +
>> +     if (netdev->flags & IFF_PROMISC) {
>> +             /* Enabling promiscuous mode */
>> +             regval |= MAC_PROMISCUOUS_MODE;
>> +             regval &= (~MAC_MULTICAST_MODE);
>> +             regval &= (~MAC_UNICAST_MODE);
>> +     } else if (netdev->flags & IFF_ALLMULTI) {
>> +             /* Enabling all multicast mode */
>> +             regval &= (~MAC_PROMISCUOUS_MODE);
>> +             regval |= MAC_MULTICAST_MODE;
>> +             regval &= (~MAC_UNICAST_MODE);
>> +     } else if (!netdev_mc_empty(netdev)) {
>> +             /* Enabling specific multicast addresses */
>> +             struct netdev_hw_addr *ha;
>> +             u32 hash_lo = 0;
>> +             u32 hash_hi = 0;
>> +
>> +             netdev_for_each_mc_addr(ha, netdev) {
>> +                     u32 bit_num = lan865x_hash(ha->addr);
>> +                     u32 mask = 1 << (bit_num & 0x1f);
>> +
>> +                     if (bit_num & 0x20)
>> +                             hash_hi |= mask;
>> +                     else
>> +                             hash_lo |= mask;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &hash_hi, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &hash_lo, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +             regval &= (~MAC_PROMISCUOUS_MODE);
>> +             regval &= (~MAC_MULTICAST_MODE);
>> +             regval |= MAC_UNICAST_MODE;
>> +     } else {
>> +             /* enabling local mac address only */
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &regval, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &regval, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +     }
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CONFIG, &regval, 1)) {
>> +             if (netif_msg_timer(priv))
>> +                     netdev_err(netdev, "Failed to enable promiscuous mode");
>> +     }
>> +}
>> +
>> +static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
>> +                                    struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     return oa_tc6_send_eth_pkt(priv->tc6, skb);
>> +}
>> +
>> +static int lan865x_hw_disable(struct lan865x_priv *priv)
>> +{
>> +     u32 regval = NW_DISABLE;
>> +
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_net_close(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret;
>> +
>> +     netif_stop_queue(netdev);
>> +     ret = lan865x_hw_disable(priv);
>> +     if (ret) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Failed to disable the hardware\n");
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_hw_enable(struct lan865x_priv *priv)
>> +{
>> +     u32 regval = NW_TX_STATUS | NW_RX_STATUS;
>> +
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_net_open(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret;
>> +
>> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
>> +             return -EADDRNOTAVAIL;
> 
> Using a random MAC address is the normal workaround for not having a
> valid MAC address via OTP flash etc.
Ah ok, you mean to use eth_hw_addr_random(netdev) instead of returning 
error.
> 
> 
>> +static int lan865x_get_dt_data(struct lan865x_priv *priv)
>> +{
>> +     struct spi_device *spi = priv->spi;
>> +     int ret;
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-chunk-size")) {
>> +             ret = of_property_read_u32(spi->dev.of_node, "oa-chunk-size",
>> +                                        &priv->cps);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else {
>> +             priv->cps = 64;
>> +             dev_info(&spi->dev, "Property oa-chunk-size is not found in dt and proceeding with the size 64\n");
>> +     }
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-tx-cut-through"))
>> +             priv->txcte = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-tx-cut-through is not found in dt and proceeding with tx store and forward mode\n");
> 
> Please remove all these dev_info() prints. The device tree binding
> should make it clear what the defaults are when not specified in DT.
Ah ok, will remove them and update device tree binding.
> 
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-rx-cut-through"))
>> +             priv->rxcte = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-rx-cut-through is not found in dt and proceeding with rx store and forward mode\n");
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-protected"))
>> +             priv->protected = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-protected is not found in dt and proceeding with protection enabled\n");
> 
> Which of these are proprietary properties, and which are part of the
> standard? Please move parsing all the standard properties into the
> library.
Ah ok good idea. Will do that.
> 
>> +static int lan865x_probe(struct spi_device *spi)
>> +{
> 
> ...
> 
>> +
>> +     phy_start(priv->phydev);
>> +     return 0;
> 
> phy_start() is normally done in open, not probe.
Ok will move it.

Best Regards,
Parthiban V
> 
>              Andrew

Andrew Lunn Sept. 19, 2023, 12:50 p.m. UTC | #11

> Sure, I can move this part to oa_tc6 lib. If I understand you correctly 
> you are talking about the Standard Capabilities Register (0x0002) 
> defined in the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface spec 
> right? If so, the 9th bit of this register tells about Indirect PHY 
> Register access Capability. Did you mean this bit? If so, this bit 
> describes the below,
> 
> IPRAC - Indirect PHY Register Access Capability. Indicates if PHY 
> registers are indirectly accessible through the MDIO/MDC registers MDIOACCn.

Yes. If the core relies on any functionality which is optional in the
standard, it should check if the capability bit is set, and do a
dev_erro() and return -ENODEV if a device does not actually have
it. That makes it clear the core needs extending to support a device.

If you are only using mandatory parts of the spec, then no test is
needed.

> > I would expect to see a call to phy_ethtool_ksettings_set()
> > here. phylib should be able to do some of the validation.
> Ah ok, doing the below will make the life easier.
> .set_link_ksettings	= phy_ethtool_set_link_ksettings,

Please do some testing and check that phy_ethtool_set_link_ksettings
doe actually reject all invalid setting. I cannot guarantee it does,
and if it does not, it might actually be a PHY driver bug.

> >> +static int lan865x_net_open(struct net_device *netdev)
> >> +{
> >> +     struct lan865x_priv *priv = netdev_priv(netdev);
> >> +     int ret;
> >> +
> >> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
> >> +             if (netif_msg_ifup(priv))
> >> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
> >> +             return -EADDRNOTAVAIL;
> > 
> > Using a random MAC address is the normal workaround for not having a
> > valid MAC address via OTP flash etc.
> Ah ok, you mean to use eth_hw_addr_random(netdev) instead of returning 
> error.

Yes. And this is generally done earlier than open, as part of
probe. You want to avoid surprising userspace when the MAC address
suddenly changes at open time.

	 Andrew

Parthiban Veerasooran Sept. 20, 2023, 12:53 p.m. UTC | #12

Hi Andrew,

On 19/09/23 6:20 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Sure, I can move this part to oa_tc6 lib. If I understand you correctly
>> you are talking about the Standard Capabilities Register (0x0002)
>> defined in the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface spec
>> right? If so, the 9th bit of this register tells about Indirect PHY
>> Register access Capability. Did you mean this bit? If so, this bit
>> describes the below,
>>
>> IPRAC - Indirect PHY Register Access Capability. Indicates if PHY
>> registers are indirectly accessible through the MDIO/MDC registers MDIOACCn.
> 
> Yes. If the core relies on any functionality which is optional in the
> standard, it should check if the capability bit is set, and do a
> dev_erro() and return -ENODEV if a device does not actually have
> it. That makes it clear the core needs extending to support a device.
Ok, will do that.
> 
> If you are only using mandatory parts of the spec, then no test is
> needed.
Ok.
> 
>>> I would expect to see a call to phy_ethtool_ksettings_set()
>>> here. phylib should be able to do some of the validation.
>> Ah ok, doing the below will make the life easier.
>> .set_link_ksettings   = phy_ethtool_set_link_ksettings,
> 
> Please do some testing and check that phy_ethtool_set_link_ksettings
> doe actually reject all invalid setting. I cannot guarantee it does,
> and if it does not, it might actually be a PHY driver bug.
Yes sure.
> 
>>>> +static int lan865x_net_open(struct net_device *netdev)
>>>> +{
>>>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>>>> +     int ret;
>>>> +
>>>> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
>>>> +             if (netif_msg_ifup(priv))
>>>> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
>>>> +             return -EADDRNOTAVAIL;
>>>
>>> Using a random MAC address is the normal workaround for not having a
>>> valid MAC address via OTP flash etc.
>> Ah ok, you mean to use eth_hw_addr_random(netdev) instead of returning
>> error.
> 
> Yes. And this is generally done earlier than open, as part of
> probe. You want to avoid surprising userspace when the MAC address
> suddenly changes at open time.
Ah ok ok, will move that to probe.

Best Regards,
Parthiban V
> 
>           Andrew

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index c54454c7e7a1..666c042a15b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13879,6 +13879,12 @@  L:	[e-mail]
 S:	Maintained
 F:	drivers/net/ethernet/microchip/lan743x_*
 
+MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
+M:	Parthiban Veerasooran <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	drivers/net/ethernet/microchip/lan865x.c
+
 MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
 M:	Arun Ramadoss <[e-mail]>
 R:	[e-mail]
diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 329e374b9539..d99be51b99f1 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -59,4 +59,14 @@  source "drivers/net/ethernet/microchip/lan966x/Kconfig"
 source "drivers/net/ethernet/microchip/sparx5/Kconfig"
 source "drivers/net/ethernet/microchip/vcap/Kconfig"
 
+config LAN865X
+	tristate "LAN865x support"
+	depends on SPI
+	help
+      	  Support for the Microchip LAN8650/1 Rev.B0 Ethernet chip. It uses OPEN
+	  Alliance 10BASE-T1x Serial Interface specification.
+
+      	  To compile this driver as a module, choose M here. The module will be
+          called lan865x.
+
 endif # NET_VENDOR_MICROCHIP
diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
index bbd349264e6f..315e850b2b26 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -12,3 +12,6 @@  lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
 obj-$(CONFIG_LAN966X_SWITCH) += lan966x/
 obj-$(CONFIG_SPARX5_SWITCH) += sparx5/
 obj-$(CONFIG_VCAP) += vcap/
+
+obj-$(CONFIG_LAN865X) += lan865x_t1s.o
+lan865x_t1s-objs := lan865x.o ../oa_tc6.o
diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c
new file mode 100644
index 000000000000..3c8ebf4c258f
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x.c
@@ -0,0 +1,589 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Microchip's LAN865x 10BASE-T1S MAC-PHY driver
+ *
+ * Author: Parthiban Veerasooran <[e-mail]>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/etherdevice.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+#include <linux/oa_tc6.h>
+
+#define DRV_NAME		"lan865x"
+#define DRV_VERSION		"0.1"
+
+#define REG_STDR_RESET		0x00000003
+#define REG_MAC_ADDR_BO		0x00010022
+#define REG_MAC_ADDR_L		0x00010024
+#define REG_MAC_ADDR_H		0x00010025
+#define REG_MAC_NW_CTRL         0x00010000
+#define REG_MAC_NW_CONFIG	0x00010001
+#define REG_MAC_HASHL		0x00010020
+#define REG_MAC_HASHH		0x00010021
+#define REG_MAC_ADDR_BO		0x00010022
+#define REG_MAC_ADDR_L		0x00010024
+#define REG_MAC_ADDR_H		0x00010025
+
+#define CCS_Q0_TX_CFG		0x000A0081
+#define CCS_Q0_RX_CFG		0x000A0082
+
+/* Buffer configuration for 32-bytes chunk payload */
+#define CCS_Q0_TX_CFG_32	0x70000000
+#define CCS_Q0_RX_CFG_32	0x30000C00
+
+#define NW_RX_STATUS		BIT(2)
+#define NW_TX_STATUS		BIT(3)
+#define NW_DISABLE		0x0
+
+#define MAC_PROMISCUOUS_MODE	BIT(4)
+#define MAC_MULTICAST_MODE	BIT(6)
+#define MAC_UNICAST_MODE	BIT(7)
+
+#define TX_TIMEOUT		(4 * HZ)
+#define LAN865X_MSG_DEFAULT	\
+	(NETIF_MSG_PROBE | NETIF_MSG_IFUP | NETIF_MSG_IFDOWN | NETIF_MSG_LINK)
+
+struct lan865x_priv {
+	struct net_device *netdev;
+	struct spi_device *spi;
+	struct oa_tc6 *tc6;
+	struct mii_bus *mdiobus;
+	struct phy_device *phydev;
+	struct device *dev;
+	u32 msg_enable;
+	bool txcte;
+	bool rxcte;
+	u32 cps;
+	bool protected;
+};
+
+static void lan865x_handle_link_change(struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+
+	phy_print_status(priv->phydev);
+}
+
+static int lan865x_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
+{
+	struct lan865x_priv *priv = bus->priv;
+	u32 regval;
+	bool ret;
+
+	ret = oa_tc6_read_register(priv->tc6, 0xFF00 | (idx & 0xFF), &regval, 1);
+	if (ret)
+		return -ENODEV;
+
+	return regval;
+}
+
+static int lan865x_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
+				 u16 regval)
+{
+	struct lan865x_priv *priv = bus->priv;
+	u32 value = regval;
+	bool ret;
+
+	ret = oa_tc6_write_register(priv->tc6, 0xFF00 | (idx & 0xFF), &value, 1);
+	if (ret)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int lan865x_phy_init(struct lan865x_priv *priv)
+{
+	int ret;
+
+	priv->mdiobus = mdiobus_alloc();
+	if (!priv->mdiobus) {
+		netdev_err(priv->netdev, "MDIO bus alloc failed\n");
+		return -ENODEV;
+	}
+
+	priv->mdiobus->phy_mask = ~(u32)BIT(1);
+	priv->mdiobus->priv = priv;
+	priv->mdiobus->read = lan865x_mdiobus_read;
+	priv->mdiobus->write = lan865x_mdiobus_write;
+	priv->mdiobus->name = "lan865x-mdiobus";
+	priv->mdiobus->parent = priv->dev;
+
+	snprintf(priv->mdiobus->id, ARRAY_SIZE(priv->mdiobus->id),
+		 "%s", dev_name(&priv->spi->dev));
+
+	ret = mdiobus_register(priv->mdiobus);
+	if (ret) {
+		netdev_err(priv->netdev, "Could not register MDIO bus\n");
+		mdiobus_free(priv->mdiobus);
+		return ret;
+	}
+	priv->phydev = phy_find_first(priv->mdiobus);
+	if (!priv->phydev) {
+		netdev_err(priv->netdev, "No PHY found\n");
+		mdiobus_unregister(priv->mdiobus);
+		mdiobus_free(priv->mdiobus);
+		return -ENODEV;
+	}
+	priv->phydev->is_internal = true;
+	ret = phy_connect_direct(priv->netdev, priv->phydev,
+				 &lan865x_handle_link_change,
+				 PHY_INTERFACE_MODE_INTERNAL);
+	if (ret) {
+		netdev_err(priv->netdev, "Can't attach PHY to %s\n", priv->mdiobus->id);
+		return ret;
+	}
+	phy_attached_info(priv->phydev);
+	return ret;
+}
+
+static int lan865x_set_hw_macaddr(struct net_device *netdev)
+{
+	u32 regval;
+	bool ret;
+	struct lan865x_priv *priv = netdev_priv(netdev);
+	const u8 *mac = netdev->dev_addr;
+
+	ret = oa_tc6_read_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1);
+	if (ret)
+		goto error_mac;
+	if ((regval & NW_TX_STATUS) | (regval & NW_RX_STATUS)) {
+		if (netif_msg_drv(priv))
+			netdev_warn(netdev, "Hardware must be disabled for MAC setting\n");
+		return -EBUSY;
+	}
+	/* MAC address setting */
+	regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) |
+		mac[0];
+	ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_L, &regval, 1);
+	if (ret)
+		goto error_mac;
+
+	regval = (mac[5] << 8) | mac[4];
+	ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_H, &regval, 1);
+	if (ret)
+		goto error_mac;
+
+	regval = (mac[5] << 24) | (mac[4] << 16) |
+		(mac[3] << 8) | mac[2];
+	ret = oa_tc6_write_register(priv->tc6, REG_MAC_ADDR_BO, &regval, 1);
+	if (ret)
+		goto error_mac;
+
+	return 0;
+
+error_mac:
+	return -ENODEV;
+}
+
+static int
+lan865x_set_link_ksettings(struct net_device *netdev,
+			   const struct ethtool_link_ksettings *cmd)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+	int ret = 0;
+
+	if (cmd->base.autoneg != AUTONEG_DISABLE ||
+	    cmd->base.speed != SPEED_10 || cmd->base.duplex != DUPLEX_HALF) {
+		if (netif_msg_link(priv))
+			netdev_warn(netdev, "Unsupported link setting");
+		ret = -EOPNOTSUPP;
+	} else {
+		if (netif_msg_link(priv))
+			netdev_warn(netdev, "Hardware must be disabled to set link mode");
+		ret = -EBUSY;
+	}
+	return ret;
+}
+
+static int
+lan865x_get_link_ksettings(struct net_device *netdev,
+			   struct ethtool_link_ksettings *cmd)
+{
+	ethtool_link_ksettings_zero_link_mode(cmd, supported);
+	ethtool_link_ksettings_add_link_mode(cmd, supported, 10baseT_Half);
+	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
+
+	cmd->base.speed = SPEED_10;
+	cmd->base.duplex = DUPLEX_HALF;
+	cmd->base.port	= PORT_TP;
+	cmd->base.autoneg = AUTONEG_DISABLE;
+
+	return 0;
+}
+
+static void lan865x_set_msglevel(struct net_device *netdev, u32 val)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+
+	priv->msg_enable = val;
+}
+
+static u32 lan865x_get_msglevel(struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+
+	return priv->msg_enable;
+}
+
+static void
+lan865x_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *info)
+{
+	strscpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strscpy(info->version, DRV_VERSION, sizeof(info->version));
+	strscpy(info->bus_info,
+		dev_name(netdev->dev.parent), sizeof(info->bus_info));
+}
+
+static const struct ethtool_ops lan865x_ethtool_ops = {
+	.get_drvinfo	= lan865x_get_drvinfo,
+	.get_msglevel	= lan865x_get_msglevel,
+	.set_msglevel	= lan865x_set_msglevel,
+	.get_link_ksettings = lan865x_get_link_ksettings,
+	.set_link_ksettings = lan865x_set_link_ksettings,
+};
+
+static void lan865x_tx_timeout(struct net_device *netdev, unsigned int txqueue)
+{
+	netdev->stats.tx_errors++;
+}
+
+static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
+{
+	struct sockaddr *address = addr;
+
+	if (netif_running(netdev))
+		return -EBUSY;
+	if (!is_valid_ether_addr(address->sa_data))
+		return -EADDRNOTAVAIL;
+
+	eth_hw_addr_set(netdev, address->sa_data);
+	return lan865x_set_hw_macaddr(netdev);
+}
+
+static u32 lan865x_hash(u8 addr[ETH_ALEN])
+{
+	return (ether_crc(ETH_ALEN, addr) >> 26) & 0x3f;
+}
+
+static void lan865x_set_multicast_list(struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+	u32 regval = 0;
+
+	if (netdev->flags & IFF_PROMISC) {
+		/* Enabling promiscuous mode */
+		regval |= MAC_PROMISCUOUS_MODE;
+		regval &= (~MAC_MULTICAST_MODE);
+		regval &= (~MAC_UNICAST_MODE);
+	} else if (netdev->flags & IFF_ALLMULTI) {
+		/* Enabling all multicast mode */
+		regval &= (~MAC_PROMISCUOUS_MODE);
+		regval |= MAC_MULTICAST_MODE;
+		regval &= (~MAC_UNICAST_MODE);
+	} else if (!netdev_mc_empty(netdev)) {
+		/* Enabling specific multicast addresses */
+		struct netdev_hw_addr *ha;
+		u32 hash_lo = 0;
+		u32 hash_hi = 0;
+
+		netdev_for_each_mc_addr(ha, netdev) {
+			u32 bit_num = lan865x_hash(ha->addr);
+			u32 mask = 1 << (bit_num & 0x1f);
+
+			if (bit_num & 0x20)
+				hash_hi |= mask;
+			else
+				hash_lo |= mask;
+		}
+		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &hash_hi, 1)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashh");
+			return;
+		}
+		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &hash_lo, 1)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashl");
+			return;
+		}
+		regval &= (~MAC_PROMISCUOUS_MODE);
+		regval &= (~MAC_MULTICAST_MODE);
+		regval |= MAC_UNICAST_MODE;
+	} else {
+		/* enabling local mac address only */
+		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &regval, 1)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashh");
+			return;
+		}
+		if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &regval, 1)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashl");
+			return;
+		}
+	}
+	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CONFIG, &regval, 1)) {
+		if (netif_msg_timer(priv))
+			netdev_err(netdev, "Failed to enable promiscuous mode");
+	}
+}
+
+static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
+				       struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+
+	return oa_tc6_send_eth_pkt(priv->tc6, skb);
+}
+
+static int lan865x_hw_disable(struct lan865x_priv *priv)
+{
+	u32 regval = NW_DISABLE;
+
+	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int lan865x_net_close(struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+	int ret;
+
+	netif_stop_queue(netdev);
+	ret = lan865x_hw_disable(priv);
+	if (ret) {
+		if (netif_msg_ifup(priv))
+			netdev_err(netdev, "Failed to disable the hardware\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int lan865x_hw_enable(struct lan865x_priv *priv)
+{
+	u32 regval = NW_TX_STATUS | NW_RX_STATUS;
+
+	if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int lan865x_net_open(struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+	int ret;
+
+	if (!is_valid_ether_addr(netdev->dev_addr)) {
+		if (netif_msg_ifup(priv))
+			netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
+		return -EADDRNOTAVAIL;
+	}
+	if (lan865x_hw_disable(priv)) {
+		if (netif_msg_ifup(priv))
+			netdev_err(netdev, "Failed to disable the hardware\n");
+		return -ENODEV;
+	}
+	ret = lan865x_set_hw_macaddr(netdev);
+	if (ret != 0)
+		return ret;
+
+	if (lan865x_hw_enable(priv) != 0) {
+		if (netif_msg_ifup(priv))
+			netdev_err(netdev, "Failed to enable hardware\n");
+		return -ENODEV;
+	}
+	netif_start_queue(netdev);
+
+	return 0;
+}
+
+static const struct net_device_ops lan865x_netdev_ops = {
+	.ndo_open		= lan865x_net_open,
+	.ndo_stop		= lan865x_net_close,
+	.ndo_start_xmit		= lan865x_send_packet,
+	.ndo_set_rx_mode	= lan865x_set_multicast_list,
+	.ndo_set_mac_address	= lan865x_set_mac_address,
+	.ndo_tx_timeout		= lan865x_tx_timeout,
+	.ndo_validate_addr	= eth_validate_addr,
+};
+
+static int lan865x_get_dt_data(struct lan865x_priv *priv)
+{
+	struct spi_device *spi = priv->spi;
+	int ret;
+
+	if (of_property_present(spi->dev.of_node, "oa-chunk-size")) {
+		ret = of_property_read_u32(spi->dev.of_node, "oa-chunk-size",
+					   &priv->cps);
+		if (ret < 0)
+			return ret;
+	} else {
+		priv->cps = 64;
+		dev_info(&spi->dev, "Property oa-chunk-size is not found in dt and proceeding with the size 64\n");
+	}
+
+	if (of_property_present(spi->dev.of_node, "oa-tx-cut-through"))
+		priv->txcte = true;
+	else
+		dev_info(&spi->dev, "Property oa-tx-cut-through is not found in dt and proceeding with tx store and forward mode\n");
+
+	if (of_property_present(spi->dev.of_node, "oa-rx-cut-through"))
+		priv->rxcte = true;
+	else
+		dev_info(&spi->dev, "Property oa-rx-cut-through is not found in dt and proceeding with rx store and forward mode\n");
+
+	if (of_property_present(spi->dev.of_node, "oa-protected"))
+		priv->protected = true;
+	else
+		dev_info(&spi->dev, "Property oa-protected is not found in dt and proceeding with protection enabled\n");
+
+	return 0;
+}
+
+static int lan865x_probe(struct spi_device *spi)
+{
+	struct net_device *netdev;
+	struct lan865x_priv *priv;
+	u32 regval;
+	int ret;
+
+	netdev = alloc_etherdev(sizeof(struct lan865x_priv));
+	if (!netdev)
+		return -ENOMEM;
+
+	priv = netdev_priv(netdev);
+	priv->netdev = netdev;
+	priv->spi = spi;
+	priv->msg_enable = 0;
+	spi_set_drvdata(spi, priv);
+	SET_NETDEV_DEV(netdev, &spi->dev);
+
+	ret = lan865x_get_dt_data(priv);
+	if (ret)
+		return ret;
+
+	spi->rt = true;
+	spi_setup(spi);
+
+	priv->tc6 = oa_tc6_init(spi, netdev);
+	if (!priv->tc6) {
+		ret = -ENOMEM;
+		goto error_oa_tc6_init;
+	}
+
+	if (priv->cps == 32) {
+		regval = CCS_Q0_TX_CFG_32;
+		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_TX_CFG, &regval, 1);
+		if (ret)
+			return ret;
+
+		regval = CCS_Q0_RX_CFG_32;
+		ret = oa_tc6_write_register(priv->tc6, CCS_Q0_RX_CFG, &regval, 1);
+		if (ret)
+			return ret;
+	}
+
+	if (oa_tc6_configure(priv->tc6, priv->cps, priv->protected, priv->txcte,
+			     priv->rxcte))
+		goto err_macphy_config;
+
+	ret = lan865x_phy_init(priv);
+	if (ret)
+		goto error_phy;
+
+	if (device_get_ethdev_address(&spi->dev, netdev))
+		eth_hw_addr_random(netdev);
+
+	ret = lan865x_set_hw_macaddr(netdev);
+	if (ret) {
+		if (netif_msg_probe(priv))
+			dev_err(&spi->dev, "Failed to configure MAC");
+		goto error_set_mac;
+	}
+
+	netdev->if_port = IF_PORT_10BASET;
+	netdev->irq = spi->irq;
+	netdev->netdev_ops = &lan865x_netdev_ops;
+	netdev->watchdog_timeo = TX_TIMEOUT;
+	netdev->ethtool_ops = &lan865x_ethtool_ops;
+	ret = register_netdev(netdev);
+	if (ret) {
+		if (netif_msg_probe(priv))
+			dev_err(&spi->dev, "Register netdev failed (ret = %d)",
+				ret);
+		goto error_netdev_register;
+	}
+
+	phy_start(priv->phydev);
+	return 0;
+
+error_netdev_register:
+error_set_mac:
+	phy_disconnect(priv->phydev);
+	mdiobus_unregister(priv->mdiobus);
+	mdiobus_free(priv->mdiobus);
+error_phy:
+err_macphy_config:
+	oa_tc6_deinit(priv->tc6);
+error_oa_tc6_init:
+	free_netdev(priv->netdev);
+	return ret;
+}
+
+static void lan865x_remove(struct spi_device *spi)
+{
+	struct lan865x_priv *priv = spi_get_drvdata(spi);
+
+	phy_stop(priv->phydev);
+	phy_disconnect(priv->phydev);
+	mdiobus_unregister(priv->mdiobus);
+	mdiobus_free(priv->mdiobus);
+	unregister_netdev(priv->netdev);
+	if (oa_tc6_deinit(priv->tc6))
+		dev_err(&spi->dev, "Failed to deinitialize oa tc6\n");
+	free_netdev(priv->netdev);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id lan865x_dt_ids[] = {
+	{ .compatible = "microchip,lan865x" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id lan865x_acpi_ids[] = {
+	{ .id = "LAN865X",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, lan865x_acpi_ids);
+#endif
+
+static struct spi_driver lan865x_driver = {
+	.driver = {
+		.name = DRV_NAME,
+#ifdef CONFIG_OF
+		.of_match_table = lan865x_dt_ids,
+#endif
+#ifdef CONFIG_ACPI
+		   .acpi_match_table = ACPI_PTR(lan865x_acpi_ids),
+#endif
+	 },
+	.probe = lan865x_probe,
+	.remove = lan865x_remove,
+};
+module_spi_driver(lan865x_driver);
+
+MODULE_DESCRIPTION(DRV_NAME " 10Base-T1S MACPHY Ethernet Driver");
+MODULE_AUTHOR("Parthiban Veerasooran <[e-mail]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:" DRV_NAME);