diff mbox series

[net-next,v2,8/9] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY

Message ID 20231023154649.45931-9-Parthiban.Veerasooran@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1363 this patch: 1363
netdev/cc_maintainers warning 1 maintainers not CCed: parthiban.veerasooran@microchip.com
netdev/build_clang success Errors and warnings before: 1388 this patch: 1388
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1388 this patch: 1388
netdev/checkpatch warning WARNING: DT compatible string "microchip,lan865x" appears un-documented -- check ./Documentation/devicetree/bindings/ WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Parthiban Veerasooran Oct. 23, 2023, 3:46 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 <Parthiban.Veerasooran@microchip.com>
---
 MAINTAINERS                              |   6 +
 drivers/net/ethernet/microchip/Kconfig   |  11 +
 drivers/net/ethernet/microchip/Makefile  |   2 +
 drivers/net/ethernet/microchip/lan865x.c | 415 +++++++++++++++++++++++
 4 files changed, 434 insertions(+)
 create mode 100644 drivers/net/ethernet/microchip/lan865x.c

Comments

Andrew Lunn Oct. 24, 2023, 2:33 a.m. UTC | #1
> +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, LAN865X_MAC_NCR, &regval);
> +	if (ret)
> +		goto error_mac;
> +	if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) {

Would testing netif_running(netdev) be enough? That is a more common
test you see. In fact, you already have that in
lan865x_set_mac_address(). And in lan865x_probe() why would the
hardware be enabled?


> +		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, LAN865X_MAC_SAB1, regval);
> +	if (ret)
> +		goto error_mac;
> +
> +	regval = (mac[5] << 8) | mac[4];
> +	ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval);
> +	if (ret)
> +		goto error_mac;
> +
> +	return 0;
> +
> +error_mac:
> +	return -ENODEV;

oa_tc6_write_register() should return an error code, so return it.

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

You should ideally only update the netdev MAC address, if you managed
to change the value in the hardware.

> +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, LAN865X_MAC_HRT, hash_hi)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashh");
> +			return;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) {
> +			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, LAN865X_MAC_HRT, regval)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashh");
> +			return;
> +		}
> +		if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) {
> +			if (netif_msg_timer(priv))
> +				netdev_err(netdev, "Failed to write reg_hashl");
> +			return;
> +		}
> +	}
> +	if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) {
> +		if (netif_msg_timer(priv))
> +			netdev_err(netdev, "Failed to enable promiscuous mode");
> +	}
> +}

Another of those big functions which could be lots of simple functions
each doing one thing.

> +/* Configures the number of bytes allocated to each buffer in the
> + * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64
> + * is the default value. So it is enough to configure the queue buffer size only
> + * for 32 bytes. Generally cps can't be changed during run time and also it is
> + * configured in the device tree. The values for the Tx/Rx queue buffer size are
> + * taken from the LAN865x datasheet.
> + */

Its unclear why this needs to be a callback. Why not just set it after
oa_tc6_init() returns?

> +static void lan865x_remove(struct spi_device *spi)
> +{
> +	struct lan865x_priv *priv = spi_get_drvdata(spi);
> +
> +	oa_tc6_exit(priv->tc6);
> +	unregister_netdev(priv->netdev);

Is that the correct order? Seems like you should unregister the netdev
first.

> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id lan865x_acpi_ids[] = {
> +	{ .id = "LAN865X",
> +	},
> +	{},

Does this work? You don't have access to the device tree properties.

     Andrew
Krzysztof Kozlowski Oct. 24, 2023, 11:57 a.m. UTC | #2
On 23/10/2023 17:46, 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 <Parthiban.Veerasooran@microchip.com>
> ---
>  MAINTAINERS                              |   6 +
>  drivers/net/ethernet/microchip/Kconfig   |  11 +
>  drivers/net/ethernet/microchip/Makefile  |   2 +
>  drivers/net/ethernet/microchip/lan865x.c | 415 +++++++++++++++++++++++
>  4 files changed, 434 insertions(+)
>  create mode 100644 drivers/net/ethernet/microchip/lan865x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9580be91f5e9..1b1bd3218a2d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14001,6 +14001,12 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/microchip/lan743x_*
>  
> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
> +M:	Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	drivers/net/ethernet/microchip/lan865x.c
> +
>  MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
>  M:	Arun Ramadoss <arun.ramadoss@microchip.com>
>  R:	UNGLinuxDriver@microchip.com
> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> index 329e374b9539..596caf59dea6 100644
> --- a/drivers/net/ethernet/microchip/Kconfig
> +++ b/drivers/net/ethernet/microchip/Kconfig
> @@ -59,4 +59,15 @@ 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
> +	depends on OA_TC6
> +	help
> +      	  Support for the Microchip LAN8650/1 Rev.B0 MACPHY 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.

That's odd indentation. Kconfig help goes with tab and two spaces.

> +
>  endif # NET_VENDOR_MICROCHIP
> diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
> index bbd349264e6f..1fa4e15a067d 100644
> --- a/drivers/net/ethernet/microchip/Makefile
> +++ b/drivers/net/ethernet/microchip/Makefile
> @@ -12,3 +12,5 @@ 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/

...

> +static void lan865x_remove(struct spi_device *spi)
> +{
> +	struct lan865x_priv *priv = spi_get_drvdata(spi);
> +
> +	oa_tc6_exit(priv->tc6);
> +	unregister_netdev(priv->netdev);
> +	free_netdev(priv->netdev);
> +}
> +
> +#ifdef CONFIG_OF

Drop ifdef

> +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

Drop ifdef

> +		.of_match_table = lan865x_dt_ids,
> +#endif
> +#ifdef CONFIG_ACPI

Why do you need this ifdef?

> +		   .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 <parthiban.veerasooran@microchip.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:" DRV_NAME);

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong.


Best regards,
Krzysztof
Parthiban Veerasooran Oct. 31, 2023, 10:25 a.m. UTC | #3
Hi Krzysztof,

On 24/10/23 5:27 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 23/10/2023 17:46, 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 <Parthiban.Veerasooran@microchip.com>
>> ---
>>   MAINTAINERS                              |   6 +
>>   drivers/net/ethernet/microchip/Kconfig   |  11 +
>>   drivers/net/ethernet/microchip/Makefile  |   2 +
>>   drivers/net/ethernet/microchip/lan865x.c | 415 +++++++++++++++++++++++
>>   4 files changed, 434 insertions(+)
>>   create mode 100644 drivers/net/ethernet/microchip/lan865x.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9580be91f5e9..1b1bd3218a2d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14001,6 +14001,12 @@ L:   netdev@vger.kernel.org
>>   S:   Maintained
>>   F:   drivers/net/ethernet/microchip/lan743x_*
>>
>> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
>> +M:   Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
>> +L:   netdev@vger.kernel.org
>> +S:   Maintained
>> +F:   drivers/net/ethernet/microchip/lan865x.c
>> +
>>   MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
>>   M:   Arun Ramadoss <arun.ramadoss@microchip.com>
>>   R:   UNGLinuxDriver@microchip.com
>> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
>> index 329e374b9539..596caf59dea6 100644
>> --- a/drivers/net/ethernet/microchip/Kconfig
>> +++ b/drivers/net/ethernet/microchip/Kconfig
>> @@ -59,4 +59,15 @@ 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
>> +     depends on OA_TC6
>> +     help
>> +               Support for the Microchip LAN8650/1 Rev.B0 MACPHY 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.
> 
> That's odd indentation. Kconfig help goes with tab and two spaces.
Ah yes, will correct it.
> 
>> +
>>   endif # NET_VENDOR_MICROCHIP
>> diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
>> index bbd349264e6f..1fa4e15a067d 100644
>> --- a/drivers/net/ethernet/microchip/Makefile
>> +++ b/drivers/net/ethernet/microchip/Makefile
>> @@ -12,3 +12,5 @@ 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/
> 
> ...
> 
>> +static void lan865x_remove(struct spi_device *spi)
>> +{
>> +     struct lan865x_priv *priv = spi_get_drvdata(spi);
>> +
>> +     oa_tc6_exit(priv->tc6);
>> +     unregister_netdev(priv->netdev);
>> +     free_netdev(priv->netdev);
>> +}
>> +
>> +#ifdef CONFIG_OF
> 
> Drop ifdef
Yes ok.
> 
>> +static const struct of_device_id lan865x_dt_ids[] = {
>> +     { .compatible = "microchip,lan865x" },
>> +     { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI
I think I need to remove this ifdef as well?
>> +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
> 
> Drop ifdef
Yes ok.
> 
>> +             .of_match_table = lan865x_dt_ids,
>> +#endif
>> +#ifdef CONFIG_ACPI
> 
> Why do you need this ifdef?
Ya it is not needed. Will remove it.
> 
>> +                .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 <parthiban.veerasooran@microchip.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("spi:" DRV_NAME);
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong.
Ok, will remove it.

Best Regards,
Parthiban V
> 
> 
> Best regards,
> Krzysztof
>
Parthiban Veerasooran Oct. 31, 2023, 11:04 a.m. UTC | #4
Hi Andrew,

On 24/10/23 8:03 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +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, LAN865X_MAC_NCR, &regval);
>> +     if (ret)
>> +             goto error_mac;
>> +     if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) {
> 
> Would testing netif_running(netdev) be enough? That is a more common
> test you see. In fact, you already have that in
> lan865x_set_mac_address(). And in lan865x_probe() why would the
> hardware be enabled?
Ah yes, this check is not needed at all. Will correct it.
> 
> 
>> +             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, LAN865X_MAC_SAB1, regval);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     regval = (mac[5] << 8) | mac[4];
>> +     ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     return 0;
>> +
>> +error_mac:
>> +     return -ENODEV;
> 
> oa_tc6_write_register() should return an error code, so return it.
Yes, noted. Will do it.
> 
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> +     struct sockaddr *address = addr;
>> +
>> +     if (netif_running(netdev))
>> +             return -EBUSY;
>> +
>> +     eth_hw_addr_set(netdev, address->sa_data);
>> +
>> +     return lan865x_set_hw_macaddr(netdev);
> 
> You should ideally only update the netdev MAC address, if you managed
> to change the value in the hardware.
Ah ok, then it is supposed to be like below, isn't it?

static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
{
	struct sockaddr *address = addr;
	int ret;

	if (netif_running(netdev))
		return -EBUSY;

	ret = lan865x_set_hw_macaddr(netdev);
	if (ret)
		return ret;

	eth_hw_addr_set(netdev, address->sa_data);

	return 0;
}
> 
>> +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, LAN865X_MAC_HRT, hash_hi)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) {
>> +                     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, LAN865X_MAC_HRT, regval)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +     }
>> +     if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) {
>> +             if (netif_msg_timer(priv))
>> +                     netdev_err(netdev, "Failed to enable promiscuous mode");
>> +     }
>> +}
> 
> Another of those big functions which could be lots of simple functions
> each doing one thing.
Sure, will split into multiple functions.
> 
>> +/* Configures the number of bytes allocated to each buffer in the
>> + * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64
>> + * is the default value. So it is enough to configure the queue buffer size only
>> + * for 32 bytes. Generally cps can't be changed during run time and also it is
>> + * configured in the device tree. The values for the Tx/Rx queue buffer size are
>> + * taken from the LAN865x datasheet.
>> + */
> 
> Its unclear why this needs to be a callback. Why not just set it after
> oa_tc6_init() returns?
oa_tc6_init() function internally calls oa_tc6_configure() function to 
configure different settings. At the end it writes SYNC bit to CONFIG0 
register which enables the MAC-PHY to start the data transfer. So the 
buffer configuration should be done prior to that. Since this is part of 
the configuration this callback is implemented.
> 
>> +static void lan865x_remove(struct spi_device *spi)
>> +{
>> +     struct lan865x_priv *priv = spi_get_drvdata(spi);
>> +
>> +     oa_tc6_exit(priv->tc6);
>> +     unregister_netdev(priv->netdev);
> 
> Is that the correct order? Seems like you should unregister the netdev
> first.
Ah yes, will correct it.
> 
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lan865x_acpi_ids[] = {
>> +     { .id = "LAN865X",
>> +     },
>> +     {},
> 
> Does this work? You don't have access to the device tree properties.
Going to remove all the OA specific configurations in the next revisions.

Best Regards,
Parthiban V
> 
>       Andrew
>
Andrew Lunn Oct. 31, 2023, 12:53 p.m. UTC | #5
> Ah ok, then it is supposed to be like below, isn't it?
> 
> static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
> {
> 	struct sockaddr *address = addr;
> 	int ret;
> 
> 	if (netif_running(netdev))
> 		return -EBUSY;
> 
> 	ret = lan865x_set_hw_macaddr(netdev);
> 	if (ret)
> 		return ret;
> 
> 	eth_hw_addr_set(netdev, address->sa_data);
> 
> 	return 0;
> }

Yes, that is better. In practice, its probably not an issue, setting
the MAC address will never fail, but it is good to get right, just in
case.

	Andrew
Parthiban Veerasooran Nov. 1, 2023, 4:51 a.m. UTC | #6
Hi Andrew,

On 31/10/23 6:23 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Ah ok, then it is supposed to be like below, isn't it?
>>
>> static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> {
>>        struct sockaddr *address = addr;
>>        int ret;
>>
>>        if (netif_running(netdev))
>>                return -EBUSY;
>>
>>        ret = lan865x_set_hw_macaddr(netdev);
>>        if (ret)
>>                return ret;
>>
>>        eth_hw_addr_set(netdev, address->sa_data);
>>
>>        return 0;
>> }
> 
> Yes, that is better. In practice, its probably not an issue, setting
> the MAC address will never fail, but it is good to get right, just in
> case.
Ok, thanks.

Best Regards,
Parthiban V
> 
>          Andrew
Ramón Nordin Rodriguez Nov. 2, 2023, 12:20 p.m. UTC | #7
On Mon, Oct 23, 2023 at 09:16:48PM +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 <Parthiban.Veerasooran@microchip.com>

Hi Parthiban

I've been testing the v2 patches out a bit, at Ferroamp we're planning
on using a dual LAN8650 setup in a product.

First let me say that we'd be happy to assist with testing and
development.

I got some observations that I think this patch is the resonable place
to discuss it, since they seem to be MAC/PHY related.

In order to get a reliable link I'm using the dts snippet below (for an
imx8 cpu)

&ecspi1 {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_ecspi1>;
	cs-gpios = <0> , <&gpio5 9 GPIO_ACTIVE_LOW>;
	status = "okay";

	spe1: ethernet@1{
		compatible = "microchip,lan865x";
		reg = <1>;
		interrupt-parent = <&gpio5>;
		interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
		spi-max-frequency = <50000000>;
		oa-tc6{
			#address-cells = <1>;
			#size-cells = <0>;
			oa-cps = <32>;
			oa-prote;
			oa-dprac;
		};
	};
};

With this setup I'm getting a maximum throughput of about 90kB/s.
If I increase the chunk size / oa-cps to 64 I get a might higher
throughput ~900kB/s, but after 0-2s I get dump below (or similar).

[  363.444460] eth0: Transmit protocol error
[  363.448527] eth0: Transmit buffer underflow
[  363.452740] eth0: Receive buffer overflow
[  363.456780] eth0: Header error
[  363.459869] eth0: Footer frame drop
[  363.463379] eth0: SPI transfer failed
[  363.470590] eth0: Receive buffer overflow
[  363.474631] eth0: Header error
[  363.477776] eth0: SPI transfer failed
[  363.482596] eth0: Footer frame drop
[  369.884680] ------------[ cut here ]------------
[  369.889336] NETDEV WATCHDOG: eth0 (lan865x): transmit queue 0 timed out 6448 ms
[  369.896726] WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:525 dev_watchdog+0x22c/0x234
[  369.905023] Modules linked in:
[  369.908091] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.4.16-gc5e8aa9586d6 #3
[  369.915241] Hardware name: <Ferroamp dev kit>
[  369.921169] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  369.928146] pc : dev_watchdog+0x22c/0x234
[  369.932168] lr : dev_watchdog+0x22c/0x234
[  369.936190] sp : ffff80000800be20
[  369.939510] x29: ffff80000800be20 x28: 0000000000000101 x27: ffff80000800bf00
[  369.946665] x26: ffff8000092469c0 x25: 0000000000001930 x24: ffff800009246000
[  369.953817] x23: 0000000000000000 x22: ffff000000e883dc x21: ffff000000e88000
[  369.960971] x20: ffff0000010dc000 x19: ffff000000e88488 x18: ffffffffffffffff
[  369.968124] x17: 383434362074756f x16: 2064656d69742030 x15: 0720072007200720
[  369.975276] x14: 0720072007200720 x13: ffff80000925fe88 x12: 0000000000000444
[  369.982431] x11: 000000000000016c x10: ffff8000092b7e88 x9 : ffff80000925fe88
[  369.989584] x8 : 00000000ffffefff x7 : ffff8000092b7e88 x6 : 80000000fffff000
[  369.996738] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
[  370.003890] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000000dd400
[  370.011044] Call trace:
[  370.013496]  dev_watchdog+0x22c/0x234
[  370.017173]  call_timer_fn.constprop.0+0x24/0x80
[  370.021802]  __run_timers.part.0+0x1f8/0x244
[  370.026080]  run_timer_softirq+0x3c/0x74
[  370.030012]  __do_softirq+0x10c/0x280
[  370.033683]  ____do_softirq+0x10/0x1c
[  370.037357]  call_on_irq_stack+0x24/0x4c
[  370.041292]  do_softirq_own_stack+0x1c/0x28
[  370.045484]  __irq_exit_rcu+0xe4/0x100
[  370.049244]  irq_exit_rcu+0x10/0x1c
[  370.052744]  el1_interrupt+0x38/0x68
[  370.056331]  el1h_64_irq_handler+0x18/0x24
[  370.060439]  el1h_64_irq+0x64/0x68
[  370.063851]  cpuidle_enter_state+0x134/0x2e0
[  370.068133]  cpuidle_enter+0x38/0x50
[  370.071719]  do_idle+0x1f4/0x264
[  370.074960]  cpu_startup_entry+0x24/0x2c
[  370.078895]  secondary_start_kernel+0x130/0x150
[  370.083440]  __secondary_switched+0xb8/0xbc
[  370.087634] ---[ end trace 0000000000000000 ]---


Additionally when hotplugging cables, which might not be a realworld
scenario I'm also seeing intermittent watchdog timeouts.

In both scenarios the driver does not recover.
Andrew Lunn Nov. 2, 2023, 12:39 p.m. UTC | #8
> 	spe1: ethernet@1{
> 		compatible = "microchip,lan865x";
> 		reg = <1>;
> 		interrupt-parent = <&gpio5>;
> 		interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
> 		spi-max-frequency = <50000000>;

That is a pretty high frequency. It is actually running at that speed?

Have you tried lower speed?

> 		oa-tc6{
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			oa-cps = <32>;
> 			oa-prote;
> 			oa-dprac;
> 		};
> 	};
> };
> 
> With this setup I'm getting a maximum throughput of about 90kB/s.
> If I increase the chunk size / oa-cps to 64 I get a might higher
> throughput ~900kB/s, but after 0-2s I get dump below (or similar).

Is this tcp traffic? Lost packets will have a big impact. You might
want to look at the link peer with tcpdump and look for retries. Also,
look if there are frames with bad checksums.

     Andrew
Ramón Nordin Rodriguez Nov. 2, 2023, 1:40 p.m. UTC | #9
Hi Andrew

> > 		spi-max-frequency = <50000000>;
> 
> That is a pretty high frequency. It is actually running at that speed?

I have not verified that we're actually hitting 50M.

> 
> Have you tried lower speed?

Sorry for being too brief about the things I've tested. But yes, I've
tested running at 15MHz with the same or similar enough results that
I've not been able to discern any difference.
Meaningful to mention here as well would be that I've tested with and
without DMA as well.

> > With this setup I'm getting a maximum throughput of about 90kB/s.
> > If I increase the chunk size / oa-cps to 64 I get a might higher
> > throughput ~900kB/s, but after 0-2s I get dump below (or similar).
> 
> Is this tcp traffic? Lost packets will have a big impact. You might
> want to look at the link peer with tcpdump and look for retries. Also,
> look if there are frames with bad checksums.
> 

As you assume, this was with TCP. I'll do a run with tcpdump and see if
I can compile any intelligent statistics from that.
Parthiban Veerasooran Nov. 3, 2023, 2:59 p.m. UTC | #10
Hi Ramon,

On 02/11/23 5:50 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Oct 23, 2023 at 09:16:48PM +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 <Parthiban.Veerasooran@microchip.com>
> 
> Hi Parthiban
> 
> I've been testing the v2 patches out a bit, at Ferroamp we're planning
> on using a dual LAN8650 setup in a product.
I understand that you are using two LAN8650, isn't it? if so are they 
both running simultaneously? or you are doing the testing with one alone?
> 
> First let me say that we'd be happy to assist with testing and
> development.
Thank you for your support on this.
> 
> I got some observations that I think this patch is the resonable place
> to discuss it, since they seem to be MAC/PHY related.
> 
> In order to get a reliable link I'm using the dts snippet below (for an
> imx8 cpu)
> 
> &ecspi1 {
>          pinctrl-names = "default";
>          pinctrl-0 = <&pinctrl_ecspi1>;
>          cs-gpios = <0> , <&gpio5 9 GPIO_ACTIVE_LOW>;
>          status = "okay";
> 
>          spe1: ethernet@1{
>                  compatible = "microchip,lan865x";
>                  reg = <1>;
>                  interrupt-parent = <&gpio5>;
>                  interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
>                  spi-max-frequency = <50000000>;
>                  oa-tc6{
>                          #address-cells = <1>;
>                          #size-cells = <0>;
>                          oa-cps = <32>;
>                          oa-prote;
>                          oa-dprac;
>                  };
>          };
> };
> 
> With this setup I'm getting a maximum throughput of about 90kB/s.
> If I increase the chunk size / oa-cps to 64 I get a might higher
> throughput ~900kB/s, but after 0-2s I get dump below (or similar).
Did you or possible to try a test case with below configuration?

- Single LAN8650 enabled
- UDP
- oa_cps = 64
- spi-max-frequency = 15000000,

Did you run iperf3 test? or any other tool?
If iperf3 then can you share the configuration you used?

I don't know whether these may audiences are needed in the CC for this 
thread. Let's see what's Andrew Lunn thinks about it?

Best Regards,
Parthiban V
> 
> [  363.444460] eth0: Transmit protocol error
> [  363.448527] eth0: Transmit buffer underflow
> [  363.452740] eth0: Receive buffer overflow
> [  363.456780] eth0: Header error
> [  363.459869] eth0: Footer frame drop
> [  363.463379] eth0: SPI transfer failed
> [  363.470590] eth0: Receive buffer overflow
> [  363.474631] eth0: Header error
> [  363.477776] eth0: SPI transfer failed
> [  363.482596] eth0: Footer frame drop
> [  369.884680] ------------[ cut here ]------------
> [  369.889336] NETDEV WATCHDOG: eth0 (lan865x): transmit queue 0 timed out 6448 ms
> [  369.896726] WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:525 dev_watchdog+0x22c/0x234
> [  369.905023] Modules linked in:
> [  369.908091] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.4.16-gc5e8aa9586d6 #3
> [  369.915241] Hardware name: <Ferroamp dev kit>
> [  369.921169] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  369.928146] pc : dev_watchdog+0x22c/0x234
> [  369.932168] lr : dev_watchdog+0x22c/0x234
> [  369.936190] sp : ffff80000800be20
> [  369.939510] x29: ffff80000800be20 x28: 0000000000000101 x27: ffff80000800bf00
> [  369.946665] x26: ffff8000092469c0 x25: 0000000000001930 x24: ffff800009246000
> [  369.953817] x23: 0000000000000000 x22: ffff000000e883dc x21: ffff000000e88000
> [  369.960971] x20: ffff0000010dc000 x19: ffff000000e88488 x18: ffffffffffffffff
> [  369.968124] x17: 383434362074756f x16: 2064656d69742030 x15: 0720072007200720
> [  369.975276] x14: 0720072007200720 x13: ffff80000925fe88 x12: 0000000000000444
> [  369.982431] x11: 000000000000016c x10: ffff8000092b7e88 x9 : ffff80000925fe88
> [  369.989584] x8 : 00000000ffffefff x7 : ffff8000092b7e88 x6 : 80000000fffff000
> [  369.996738] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> [  370.003890] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000000dd400
> [  370.011044] Call trace:
> [  370.013496]  dev_watchdog+0x22c/0x234
> [  370.017173]  call_timer_fn.constprop.0+0x24/0x80
> [  370.021802]  __run_timers.part.0+0x1f8/0x244
> [  370.026080]  run_timer_softirq+0x3c/0x74
> [  370.030012]  __do_softirq+0x10c/0x280
> [  370.033683]  ____do_softirq+0x10/0x1c
> [  370.037357]  call_on_irq_stack+0x24/0x4c
> [  370.041292]  do_softirq_own_stack+0x1c/0x28
> [  370.045484]  __irq_exit_rcu+0xe4/0x100
> [  370.049244]  irq_exit_rcu+0x10/0x1c
> [  370.052744]  el1_interrupt+0x38/0x68
> [  370.056331]  el1h_64_irq_handler+0x18/0x24
> [  370.060439]  el1h_64_irq+0x64/0x68
> [  370.063851]  cpuidle_enter_state+0x134/0x2e0
> [  370.068133]  cpuidle_enter+0x38/0x50
> [  370.071719]  do_idle+0x1f4/0x264
> [  370.074960]  cpu_startup_entry+0x24/0x2c
> [  370.078895]  secondary_start_kernel+0x130/0x150
> [  370.083440]  __secondary_switched+0xb8/0xbc
> [  370.087634] ---[ end trace 0000000000000000 ]---
> 
> 
> Additionally when hotplugging cables, which might not be a realworld
> scenario I'm also seeing intermittent watchdog timeouts.
> 
> In both scenarios the driver does not recover.
>
Ramón Nordin Rodriguez Nov. 6, 2023, 8:59 a.m. UTC | #11
> I understand that you are using two LAN8650, isn't it? if so are they 
> both running simultaneously? or you are doing the testing with one alone?

Currently we've only got one running, hopefully we've wrapped up the
board bring up in the next few days and will be able to run the second
mac/phy as well.

> > With this setup I'm getting a maximum throughput of about 90kB/s.
> > If I increase the chunk size / oa-cps to 64 I get a might higher
> > throughput ~900kB/s, but after 0-2s I get dump below (or similar).
> Did you or possible to try a test case with below configuration?
> 
> - Single LAN8650 enabled
> - UDP
> - oa_cps = 64
> - spi-max-frequency = 15000000,

I'll set that up and will get back to you, all testing I have performed
has been with tcp.

> 
> Did you run iperf3 test? or any other tool?
> If iperf3 then can you share the configuration you used?

I just tried copying a file over the network with rsync, but I'll run some
udp tests with iperf.
I'll do my best to get back to you with results today.

> 
> I don't know whether these may audiences are needed in the CC for this 
> thread. Let's see what's Andrew Lunn thinks about it?

No opinions on my end at least.

BR
Ramón
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 9580be91f5e9..1b1bd3218a2d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14001,6 +14001,12 @@  L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/microchip/lan743x_*
 
+MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
+M:	Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/ethernet/microchip/lan865x.c
+
 MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
 M:	Arun Ramadoss <arun.ramadoss@microchip.com>
 R:	UNGLinuxDriver@microchip.com
diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 329e374b9539..596caf59dea6 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -59,4 +59,15 @@  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
+	depends on OA_TC6
+	help
+      	  Support for the Microchip LAN8650/1 Rev.B0 MACPHY 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..1fa4e15a067d 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -12,3 +12,5 @@  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.o
diff --git a/drivers/net/ethernet/microchip/lan865x.c b/drivers/net/ethernet/microchip/lan865x.c
new file mode 100644
index 000000000000..3ac6a0a31b37
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x.c
@@ -0,0 +1,415 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Microchip's LAN865x 10BASE-T1S MAC-PHY driver
+ *
+ * Author: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
+ */
+
+#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"
+
+/* MAC Network Control Register */
+#define LAN865X_MAC_NCR         0x00010000
+#define LAN865X_TXEN		BIT(3) /* Transmit Enable */
+#define LAN865X_RXEN		BIT(2) /* Receive Enable */
+#define LAN865X_MAC_NCFGR	0x00010001 /* MAC Network Configuration Register */
+#define LAN865X_MAC_HRB		0x00010020 /* MAC Hash Register Bottom */
+#define LAN865X_MAC_HRT		0x00010021 /* MAC Hash Register Top */
+#define LAN865X_MAC_SAB1	0x00010022 /* MAC Specific Address 1 Bottom Register */
+#define LAN865X_MAC_SAT1	0x00010023 /* MAC Specific Address 1 Top Register */
+/* Queue Transmit Configuration */
+#define LAN865X_QTXCFG		0x000A0081
+/* Queue Receive Configuration */
+#define LAN865X_QRXCFG		0x000A0082
+#define LAN865X_BUFSZ		GENMASK(22, 20) /* Buffer Size */
+
+#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;
+	u32 msg_enable;
+	bool protected;
+	bool txcte;
+	bool rxcte;
+	u32 cps;
+};
+
+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, LAN865X_MAC_NCR, &regval);
+	if (ret)
+		goto error_mac;
+	if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) {
+		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, LAN865X_MAC_SAB1, regval);
+	if (ret)
+		goto error_mac;
+
+	regval = (mac[5] << 8) | mac[4];
+	ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval);
+	if (ret)
+		goto error_mac;
+
+	return 0;
+
+error_mac:
+	return -ENODEV;
+}
+
+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->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 = phy_ethtool_get_link_ksettings,
+	.set_link_ksettings = phy_ethtool_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;
+
+	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, LAN865X_MAC_HRT, hash_hi)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashh");
+			return;
+		}
+		if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) {
+			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, LAN865X_MAC_HRT, regval)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashh");
+			return;
+		}
+		if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) {
+			if (netif_msg_timer(priv))
+				netdev_err(netdev, "Failed to write reg_hashl");
+			return;
+		}
+	}
+	if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) {
+		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;
+
+	if (oa_tc6_read_register(priv->tc6, LAN865X_MAC_NCR, &regval))
+		return -ENODEV;
+
+	regval &= ~(LAN865X_TXEN | LAN865X_RXEN);
+
+	if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCR, regval))
+		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);
+	if (netdev->phydev)
+		phy_stop(netdev->phydev);
+	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;
+
+	if (oa_tc6_read_register(priv->tc6, LAN865X_MAC_NCR, &regval))
+		return -ENODEV;
+
+	regval |= LAN865X_TXEN | LAN865X_RXEN;
+
+	if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCR, regval))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int lan865x_net_open(struct net_device *netdev)
+{
+	struct lan865x_priv *priv = netdev_priv(netdev);
+
+	if (lan865x_hw_enable(priv) != 0) {
+		if (netif_msg_ifup(priv))
+			netdev_err(netdev, "Failed to enable hardware\n");
+		return -ENODEV;
+	}
+	phy_start(netdev->phydev);
+	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,
+};
+
+/* Configures the number of bytes allocated to each buffer in the
+ * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64
+ * is the default value. So it is enough to configure the queue buffer size only
+ * for 32 bytes. Generally cps can't be changed during run time and also it is
+ * configured in the device tree. The values for the Tx/Rx queue buffer size are
+ * taken from the LAN865x datasheet.
+ */
+static int lan865x_config_cps_buf(void *tc6, u32 cps)
+{
+	u32 regval;
+	int ret;
+
+	if (cps == 32) {
+		ret = oa_tc6_read_register(tc6, LAN865X_QTXCFG, &regval);
+		if (ret)
+			return ret;
+
+		regval &= ~LAN865X_BUFSZ;
+		regval |= FIELD_PREP(LAN865X_BUFSZ, 0x0);
+
+		ret = oa_tc6_write_register(tc6, LAN865X_QTXCFG, regval);
+		if (ret)
+			return ret;
+
+		ret = oa_tc6_read_register(tc6, LAN865X_QRXCFG, &regval);
+		if (ret)
+			return ret;
+
+		regval &= ~LAN865X_BUFSZ;
+		regval |= FIELD_PREP(LAN865X_BUFSZ, 0x0);
+
+		ret = oa_tc6_write_register(tc6, LAN865X_QRXCFG, regval);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int lan865x_probe(struct spi_device *spi)
+{
+	struct net_device *netdev;
+	struct lan865x_priv *priv;
+	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);
+
+	priv->tc6 = oa_tc6_init(spi, netdev, lan865x_config_cps_buf);
+	if (!priv->tc6) {
+		ret = -ENODEV;
+		goto err_oa_tc6_init;
+	}
+	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 err_config;
+	}
+
+	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 err_config;
+	}
+
+	return 0;
+
+err_config:
+	oa_tc6_exit(priv->tc6);
+err_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);
+
+	oa_tc6_exit(priv->tc6);
+	unregister_netdev(priv->netdev);
+	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 <parthiban.veerasooran@microchip.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:" DRV_NAME);