Mailing List Archive

[PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY
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. 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 is 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 | 1 +
drivers/net/ethernet/microchip/Makefile | 1 +
.../net/ethernet/microchip/lan865x/Kconfig | 19 +
.../net/ethernet/microchip/lan865x/Makefile | 6 +
.../net/ethernet/microchip/lan865x/lan865x.c | 384 ++++++++++++++++++
6 files changed, 417 insertions(+)
create mode 100644 drivers/net/ethernet/microchip/lan865x/Kconfig
create mode 100644 drivers/net/ethernet/microchip/lan865x/Makefile
create mode 100644 drivers/net/ethernet/microchip/lan865x/lan865x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 603528948f61..f41b7f2257d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14374,6 +14374,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/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 43ba71e82260..06ca79669053 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -56,6 +56,7 @@ config LAN743X
To compile this driver as a module, choose M here. The module will be
called lan743x.

+source "drivers/net/ethernet/microchip/lan865x/Kconfig"
source "drivers/net/ethernet/microchip/lan966x/Kconfig"
source "drivers/net/ethernet/microchip/sparx5/Kconfig"
source "drivers/net/ethernet/microchip/vcap/Kconfig"
diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
index bbd349264e6f..15dfbb321057 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_LAN743X) += lan743x.o

lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o

+obj-$(CONFIG_LAN865X) += lan865x/
obj-$(CONFIG_LAN966X_SWITCH) += lan966x/
obj-$(CONFIG_SPARX5_SWITCH) += sparx5/
obj-$(CONFIG_VCAP) += vcap/
diff --git a/drivers/net/ethernet/microchip/lan865x/Kconfig b/drivers/net/ethernet/microchip/lan865x/Kconfig
new file mode 100644
index 000000000000..f3d60d14e202
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Microchip LAN865x Driver Support
+#
+
+if NET_VENDOR_MICROCHIP
+
+config LAN865X
+ tristate "LAN865x support"
+ depends on SPI
+ depends on OA_TC6
+ help
+ Support for the Microchip LAN8650/1 Rev.B1 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/lan865x/Makefile b/drivers/net/ethernet/microchip/lan865x/Makefile
new file mode 100644
index 000000000000..9f5dd89c1eb8
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the Microchip LAN865x Driver
+#
+
+obj-$(CONFIG_LAN865X) += lan865x.o
diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
new file mode 100644
index 000000000000..9abefa8b9d9f
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
@@ -0,0 +1,384 @@
+// 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/phy.h>
+#include <linux/oa_tc6.h>
+
+#define DRV_NAME "lan865x"
+
+/* MAC Network Control Register */
+#define LAN865X_REG_MAC_NET_CTL 0x00010000
+#define MAC_NET_CTL_TXEN BIT(3) /* Transmit Enable */
+#define MAC_NET_CTL_RXEN BIT(2) /* Receive Enable */
+
+#define LAN865X_REG_MAC_NET_CFG 0x00010001 /* MAC Network Configuration Reg */
+#define MAC_NET_CFG_PROMISCUOUS_MODE BIT(4)
+#define MAC_NET_CFG_MULTICAST_MODE BIT(6)
+#define MAC_NET_CFG_UNICAST_MODE BIT(7)
+
+#define LAN865X_REG_MAC_L_HASH 0x00010020 /* MAC Hash Register Bottom */
+#define LAN865X_REG_MAC_H_HASH 0x00010021 /* MAC Hash Register Top */
+#define LAN865X_REG_MAC_L_SADDR1 0x00010022 /* MAC Specific Addr 1 Bottom Reg */
+#define LAN865X_REG_MAC_H_SADDR1 0x00010023 /* MAC Specific Addr 1 Top Reg */
+
+/* OPEN Alliance Configuration Register #0 */
+#define OA_TC6_REG_CONFIG0 0x0004
+#define CONFIG0_ZARFE_ENABLE BIT(12)
+
+struct lan865x_priv {
+ struct work_struct multicast_work;
+ struct net_device *netdev;
+ struct spi_device *spi;
+ struct oa_tc6 *tc6;
+};
+
+static int lan865x_set_hw_macaddr_low_bytes(struct oa_tc6 *tc6, const u8 *mac)
+{
+ u32 regval;
+
+ regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) | mac[0];
+
+ return oa_tc6_write_register(tc6, LAN865X_REG_MAC_L_SADDR1, regval);
+}
+
+static int lan865x_set_hw_macaddr(struct lan865x_priv *priv, const u8 *mac)
+{
+ int restore_ret;
+ u32 regval;
+ int ret;
+
+ /* Configure MAC address low bytes */
+ ret = lan865x_set_hw_macaddr_low_bytes(priv->tc6, mac);
+ if (ret)
+ return ret;
+
+ /* Prepare and configure MAC address high bytes */
+ regval = (mac[5] << 8) | mac[4];
+ ret = oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_SADDR1, regval);
+ if (!ret)
+ return 0;
+
+ /* Restore the old MAC address low bytes from netdev if the new MAC
+ * address high bytes setting failed.
+ */
+ restore_ret = lan865x_set_hw_macaddr_low_bytes(priv->tc6,
+ priv->netdev->dev_addr);
+ if (restore_ret)
+ return restore_ret;
+
+ return ret;
+}
+
+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_link_ksettings = phy_ethtool_get_link_ksettings,
+ .set_link_ksettings = phy_ethtool_set_link_ksettings,
+};
+
+static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ struct sockaddr *address = addr;
+ int ret;
+
+ ret = eth_prepare_mac_addr_change(netdev, addr);
+ if (ret < 0)
+ return ret;
+
+ if (ether_addr_equal(address->sa_data, netdev->dev_addr))
+ return 0;
+
+ ret = lan865x_set_hw_macaddr(priv, address->sa_data);
+ if (ret)
+ return ret;
+
+ eth_hw_addr_set(netdev, address->sa_data);
+
+ return 0;
+}
+
+static u32 lan865x_hash(u8 addr[ETH_ALEN])
+{
+ return (ether_crc(ETH_ALEN, addr) >> 26) & GENMASK(5, 0);
+}
+
+static void lan865x_set_specific_multicast_addr(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ 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 = BIT(bit_num);
+
+ /* 5th bit of the 6 bits hash value is used to determine which
+ * bit to set in either a high or low hash register.
+ */
+ if (bit_num & BIT(5))
+ hash_hi |= mask;
+ else
+ hash_lo |= mask;
+ }
+
+ /* Enabling specific multicast addresses */
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH, hash_hi)) {
+ netdev_err(netdev, "Failed to write reg_hashh");
+ return;
+ }
+
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_L_HASH, hash_lo))
+ netdev_err(netdev, "Failed to write reg_hashl");
+}
+
+static void lan865x_multicast_work_handler(struct work_struct *work)
+{
+ struct lan865x_priv *priv = container_of(work, struct lan865x_priv,
+ multicast_work);
+ u32 regval = 0;
+
+ if (priv->netdev->flags & IFF_PROMISC) {
+ /* Enabling promiscuous mode */
+ regval |= MAC_NET_CFG_PROMISCUOUS_MODE;
+ regval &= (~MAC_NET_CFG_MULTICAST_MODE);
+ regval &= (~MAC_NET_CFG_UNICAST_MODE);
+ } else if (priv->netdev->flags & IFF_ALLMULTI) {
+ /* Enabling all multicast mode */
+ regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
+ regval |= MAC_NET_CFG_MULTICAST_MODE;
+ regval &= (~MAC_NET_CFG_UNICAST_MODE);
+ } else if (!netdev_mc_empty(priv->netdev)) {
+ lan865x_set_specific_multicast_addr(priv->netdev);
+ regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
+ regval &= (~MAC_NET_CFG_MULTICAST_MODE);
+ regval |= MAC_NET_CFG_UNICAST_MODE;
+ } else {
+ /* enabling local mac address only */
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH, 0)) {
+ netdev_err(priv->netdev, "Failed to write reg_hashh");
+ return;
+ }
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_L_HASH, 0)) {
+ netdev_err(priv->netdev, "Failed to write reg_hashl");
+ return;
+ }
+ }
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CFG, regval))
+ netdev_err(priv->netdev,
+ "Failed to enable promiscuous/multicast/normal mode");
+}
+
+static void lan865x_set_multicast_list(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+
+ schedule_work(&priv->multicast_work);
+}
+
+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_start_xmit(priv->tc6, skb);
+}
+
+static int lan865x_hw_disable(struct lan865x_priv *priv)
+{
+ u32 regval;
+
+ if (oa_tc6_read_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, &regval))
+ return -ENODEV;
+
+ regval &= ~(MAC_NET_CTL_TXEN | MAC_NET_CTL_RXEN);
+
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, 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);
+ phy_stop(netdev->phydev);
+ ret = lan865x_hw_disable(priv);
+ if (ret) {
+ netdev_err(netdev, "Failed to disable the hardware: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int lan865x_hw_enable(struct lan865x_priv *priv)
+{
+ u32 regval;
+
+ if (oa_tc6_read_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, &regval))
+ return -ENODEV;
+
+ regval |= MAC_NET_CTL_TXEN | MAC_NET_CTL_RXEN;
+
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, regval))
+ return -ENODEV;
+
+ return 0;
+}
+
+static int lan865x_net_open(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ int ret;
+
+ ret = lan865x_hw_enable(priv);
+ if (ret) {
+ netdev_err(netdev, "Failed to enable hardware: %d\n", ret);
+ return ret;
+ }
+
+ phy_start(netdev->phydev);
+
+ 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,
+};
+
+static int lan865x_set_zarfe(struct lan865x_priv *priv)
+{
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(priv->tc6, OA_TC6_REG_CONFIG0, &regval);
+ if (ret)
+ return ret;
+
+ /* Set Zero-Align Receive Frame Enable */
+ regval |= CONFIG0_ZARFE_ENABLE;
+
+ return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
+}
+
+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;
+ spi_set_drvdata(spi, priv);
+ INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
+
+ priv->tc6 = oa_tc6_init(spi, netdev);
+ if (!priv->tc6) {
+ ret = -ENODEV;
+ goto free_netdev;
+ }
+
+ /* As per the point s3 in the below errata, SPI receive Ethernet frame
+ * transfer may halt when starting the next frame in the same data block
+ * (chunk) as the end of a previous frame. The RFA field should be
+ * configured to 01b or 10b for proper operation. In these modes, only
+ * one receive Ethernet frame will be placed in a single data block.
+ * When the RFA field is written to 01b, received frames will be forced
+ * to only start in the first word of the data block payload (SWO=0). As
+ * recommended, ZARFE bit in the OPEN Alliance CONFIG0 register is set
+ * to 1 for proper operation.
+ *
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/Errata/LAN8650-1-Errata-80001075.pdf
+ */
+ ret = lan865x_set_zarfe(priv);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to set ZARFE: %d\n", ret);
+ goto oa_tc6_exit;
+ }
+
+ /* Get the MAC address from the SPI device tree node */
+ if (device_get_ethdev_address(&spi->dev, netdev))
+ eth_hw_addr_random(netdev);
+
+ ret = lan865x_set_hw_macaddr(priv, netdev->dev_addr);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to configure MAC: %d\n", ret);
+ goto oa_tc6_exit;
+ }
+
+ netdev->if_port = IF_PORT_10BASET;
+ netdev->irq = spi->irq;
+ netdev->netdev_ops = &lan865x_netdev_ops;
+ netdev->ethtool_ops = &lan865x_ethtool_ops;
+
+ ret = register_netdev(netdev);
+ if (ret) {
+ dev_err(&spi->dev, "Register netdev failed (ret = %d)", ret);
+ goto oa_tc6_exit;
+ }
+
+ return 0;
+
+oa_tc6_exit:
+ oa_tc6_exit(priv->tc6);
+free_netdev:
+ free_netdev(priv->netdev);
+ return ret;
+}
+
+static void lan865x_remove(struct spi_device *spi)
+{
+ struct lan865x_priv *priv = spi_get_drvdata(spi);
+
+ cancel_work_sync(&priv->multicast_work);
+ unregister_netdev(priv->netdev);
+ oa_tc6_exit(priv->tc6);
+ free_netdev(priv->netdev);
+}
+
+static const struct of_device_id lan865x_dt_ids[] = {
+ { .compatible = "microchip,lan8651", "microchip,lan8650" },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
+
+static struct spi_driver lan865x_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .of_match_table = lan865x_dt_ids,
+ },
+ .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");
--
2.34.1
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
> +/* OPEN Alliance Configuration Register #0 */
> +#define OA_TC6_REG_CONFIG0 0x0004
> +#define CONFIG0_ZARFE_ENABLE BIT(12)

If this is a standard register, you should put these defined where
other drivers can use them.

> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
> +{
> + struct lan865x_priv *priv = netdev_priv(netdev);
> + struct sockaddr *address = addr;
> + int ret;
> +
> + ret = eth_prepare_mac_addr_change(netdev, addr);
> + if (ret < 0)
> + return ret;
> +
> + if (ether_addr_equal(address->sa_data, netdev->dev_addr))
> + return 0;
> +
> + ret = lan865x_set_hw_macaddr(priv, address->sa_data);
> + if (ret)
> + return ret;
> +
> + eth_hw_addr_set(netdev, address->sa_data);

It seems more normal to call eth_commit_mac_addr_change(), which
better pairs with eth_prepare_mac_addr_change().

> +static int lan865x_set_zarfe(struct lan865x_priv *priv)
> +{
> + u32 regval;
> + int ret;
> +
> + ret = oa_tc6_read_register(priv->tc6, OA_TC6_REG_CONFIG0, &regval);
> + if (ret)
> + return ret;
> +
> + /* Set Zero-Align Receive Frame Enable */
> + regval |= CONFIG0_ZARFE_ENABLE;
> +
> + return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
> +}

There does not appear to be anything specific to your device here. So
please make this a helper in the shared code, so any driver can use
it.

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Hi Andrew,

On 24/04/24 5:57 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +/* OPEN Alliance Configuration Register #0 */
>> +#define OA_TC6_REG_CONFIG0 0x0004
>> +#define CONFIG0_ZARFE_ENABLE BIT(12)
>
> If this is a standard register, you should put these defined where
> other drivers can use them.
OK. Will move it to oa_tc6.c in the next version as you have also
suggested to use this as a helper function in the below comment.
>
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> + struct lan865x_priv *priv = netdev_priv(netdev);
>> + struct sockaddr *address = addr;
>> + int ret;
>> +
>> + ret = eth_prepare_mac_addr_change(netdev, addr);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (ether_addr_equal(address->sa_data, netdev->dev_addr))
>> + return 0;
>> +
>> + ret = lan865x_set_hw_macaddr(priv, address->sa_data);
>> + if (ret)
>> + return ret;
>> +
>> + eth_hw_addr_set(netdev, address->sa_data);
>
> It seems more normal to call eth_commit_mac_addr_change(), which
> better pairs with eth_prepare_mac_addr_change().
OK, so I will replace eth_hw_addr_set() with
eth_prepare_mac_addr_change() in the next version.
>
>> +static int lan865x_set_zarfe(struct lan865x_priv *priv)
>> +{
>> + u32 regval;
>> + int ret;
>> +
>> + ret = oa_tc6_read_register(priv->tc6, OA_TC6_REG_CONFIG0, &regval);
>> + if (ret)
>> + return ret;
>> +
>> + /* Set Zero-Align Receive Frame Enable */
>> + regval |= CONFIG0_ZARFE_ENABLE;
>> +
>> + return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
>> +}
>
> There does not appear to be anything specific to your device here. So
> please make this a helper in the shared code, so any driver can use
> it.
OK, I will implement this helper function as oa_tc6_enable_zarfe() in
the oa_tc6.c. Also do you want me to move this helper function
implementation to a new patch?

Best regards,
Parthiban V
>
> Andrew
>
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
> OK, I will implement this helper function as oa_tc6_enable_zarfe() in
> the oa_tc6.c. Also do you want me to move this helper function
> implementation to a new patch?

Yes please.

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Hi,

For me the mac driver fails to probe with the following log
[ 0.123325] SPI driver lan865x has no spi_device_id for microchip,lan8651

With this change the driver probes

diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
index 9abefa8b9d9f..72a663f14f50 100644
--- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
+++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
@@ -364,7 +364,7 @@ static void lan865x_remove(struct spi_device *spi)
}

static const struct of_device_id lan865x_dt_ids[] = {
- { .compatible = "microchip,lan8651", "microchip,lan8650" },
+ { .compatible = "microchip,lan865x", "microchip,lan8650" },
{ /* Sentinel */ }
};
MODULE_DEVICE_TABLE(of, lan865x_dt_ids);

Along with compatible = "microchip,lan865x" in the dts
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
I'm running a dual lan8650 setup, neither IC passed the sw reset in the
oa_tc.c module, I need to pull the reset pin low to reset the pin before
the rest of the init stuff happens.

The datasheet recommends not doing a sw reset, excerpt from section
4.1.1.3 Software Reset
"Note: The SW_RESET bit of the Clause 22 Basic Control register will reset only the internal PHY, not
the entire device. This PHY only reset is not recommended for use. If such a reset is detected, by
reading the RESETC bit of the STS2 register, reset the entire device."

Doing a hw reset followed by a sw reset seems to work fine though. I
added the folloing patch to get things moving.

diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
index 72a663f14f50..993c4f9dec7e 100644
--- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
+++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
@@ -9,6 +9,7 @@
#include <linux/kernel.h>
#include <linux/phy.h>
#include <linux/oa_tc6.h>
+#include <linux/gpio/driver.h>

#define DRV_NAME "lan865x"

@@ -36,6 +37,7 @@ struct lan865x_priv {
struct net_device *netdev;
struct spi_device *spi;
struct oa_tc6 *tc6;
+ struct gpio_desc *reset_gpio;
};

static int lan865x_set_hw_macaddr_low_bytes(struct oa_tc6 *tc6, const u8 *mac)
@@ -283,6 +285,29 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv)
return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
}

+static int lan865x_probe_reset_gpio(struct lan865x_priv *priv)
+{
+ priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev,
+ "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->reset_gpio)) {
+ dev_err(&priv->spi->dev, "failed to parse reset gpio from dt");
+ return PTR_ERR(priv->reset_gpio);
+ }
+
+ return 0;
+}
+
+static void lan865x_hw_reset(struct lan865x_priv *priv)
+{
+ dev_info(&priv->spi->dev, "resetting device");
+ gpiod_set_value(priv->reset_gpio, 1);
+ // the datasheet specifies a minimum 5?s hold time
+ usleep_range(5,10);
+ gpiod_set_value(priv->reset_gpio, 0);
+ dev_info(&priv->spi->dev, "reset completed");
+}
+
static int lan865x_probe(struct spi_device *spi)
{
struct net_device *netdev;
@@ -297,6 +322,9 @@ static int lan865x_probe(struct spi_device *spi)
priv->netdev = netdev;
priv->spi = spi;
spi_set_drvdata(spi, priv);
+ lan865x_probe_reset_gpio(priv);
+ if(priv->reset_gpio)
+ lan865x_hw_reset(priv);
INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);

priv->tc6 = oa_tc6_init(spi, netdev);

Since the chip does have a HW reset pin I think it would be nice to
at least expose this as an optional dt binding.
Maybe ignore the prints I forgot to remove :)
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sat, Apr 27, 2024 at 09:19:34PM +0200, Ram?n Nordin Rodriguez wrote:
> Hi,
>
> For me the mac driver fails to probe with the following log
> [ 0.123325] SPI driver lan865x has no spi_device_id for microchip,lan8651
>
> With this change the driver probes
>
> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 9abefa8b9d9f..72a663f14f50 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -364,7 +364,7 @@ static void lan865x_remove(struct spi_device *spi)
> }
>
> static const struct of_device_id lan865x_dt_ids[] = {
> - { .compatible = "microchip,lan8651", "microchip,lan8650" },

Huh, that's very strange. I don't see a single instance in the tree of a
of_device_id struct like this with two compatibles like this (at least
with a search of `rg "\.compatible.*\", \"" drivers/`.

Given the fallbacks in the binding, only "microchip,lan8650" actually
needs to be here.

> + { .compatible = "microchip,lan865x", "microchip,lan8650" },
> { /* Sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>
> Along with compatible = "microchip,lan865x" in the dts

Just to be clear, the compatible w/ an x is unacceptable due to the
wildcard and the binding should stay as-is. Whatever probing bugs
the code has need to be resolved instead :)

Thanks,
Conor.
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sat, Apr 27, 2024 at 08:57:43PM +0100, Conor Dooley wrote:
> > static const struct of_device_id lan865x_dt_ids[] = {
> > - { .compatible = "microchip,lan8651", "microchip,lan8650" },
>
> Huh, that's very strange. I don't see a single instance in the tree of a
> of_device_id struct like this with two compatibles like this (at least
> with a search of `rg "\.compatible.*\", \"" drivers/`.
>
> Given the fallbacks in the binding, only "microchip,lan8650" actually
> needs to be here.
>
> > + { .compatible = "microchip,lan865x", "microchip,lan8650" },
> > { /* Sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
> >
> > Along with compatible = "microchip,lan865x" in the dts
>
> Just to be clear, the compatible w/ an x is unacceptable due to the
> wildcard and the binding should stay as-is. Whatever probing bugs
> the code has need to be resolved instead :)
>

All right, so when I change to

@@ -364,7 +364,7 @@ static void lan865x_remove(struct spi_device *spi)
}

static const struct of_device_id lan865x_dt_ids[] = {
- { .compatible = "microchip,lan8651", "microchip,lan8650" },
+ { .compatible = "microchip,lan8650" },
{ /* Sentinel */ }
};
MODULE_DEVICE_TABLE(of, lan865x_dt_ids);

I still get the output
[ 0.124266] SPI driver lan865x has no spi_device_id for microchip,lan8650
But the driver does probe and I get a network interface.

If no one beats me to it I'll single step the probe tomorrow.
R
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sat, Apr 27, 2024 at 10:13:33PM +0200, Ram?n Nordin Rodriguez wrote:
> On Sat, Apr 27, 2024 at 08:57:43PM +0100, Conor Dooley wrote:
> > > static const struct of_device_id lan865x_dt_ids[] = {
> > > - { .compatible = "microchip,lan8651", "microchip,lan8650" },
> >
> > Huh, that's very strange. I don't see a single instance in the tree of a
> > of_device_id struct like this with two compatibles like this (at least
> > with a search of `rg "\.compatible.*\", \"" drivers/`.
> >
> > Given the fallbacks in the binding, only "microchip,lan8650" actually
> > needs to be here.
> >
> > > + { .compatible = "microchip,lan865x", "microchip,lan8650" },
> > > { /* Sentinel */ }
> > > };
> > > MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
> > >
> > > Along with compatible = "microchip,lan865x" in the dts
> >
> > Just to be clear, the compatible w/ an x is unacceptable due to the
> > wildcard and the binding should stay as-is. Whatever probing bugs
> > the code has need to be resolved instead :)
> >
>
> All right, so when I change to
>
> @@ -364,7 +364,7 @@ static void lan865x_remove(struct spi_device *spi)
> }
>
> static const struct of_device_id lan865x_dt_ids[] = {
> - { .compatible = "microchip,lan8651", "microchip,lan8650" },
> + { .compatible = "microchip,lan8650" },
> { /* Sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>
> I still get the output
> [ 0.124266] SPI driver lan865x has no spi_device_id for microchip,lan8650
> But the driver does probe and I get a network interface.
>
> If no one beats me to it I'll single step the probe tomorrow.

I think the error pretty much is what it says it is, the driver doesn't
appear to have a spi_device_id table containing lan8650. The name of
the driver is lan685x which is used in the fallback clause in
__spi_register_driver(), so it complains as it does not find lan8650 in
either. If my understanding is correct, either a spi_device_id table is
required or the driver needs a rename with s/x/0/.

Cheers,
Conor.
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sat, Apr 27, 2024 at 09:19:34PM +0200, Ram?n Nordin Rodriguez wrote:
> Hi,
>
> For me the mac driver fails to probe with the following log
> [ 0.123325] SPI driver lan865x has no spi_device_id for microchip,lan8651
>
> With this change the driver probes
>
> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 9abefa8b9d9f..72a663f14f50 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -364,7 +364,7 @@ static void lan865x_remove(struct spi_device *spi)
> }
>
> static const struct of_device_id lan865x_dt_ids[] = {
> - { .compatible = "microchip,lan8651", "microchip,lan8650" },
> + { .compatible = "microchip,lan865x", "microchip,lan8650" },
> { /* Sentinel */ }
> };

The device tree binding says:

+ compatible:
+ oneOf:
+ - const: microchip,lan8650
+ - items:
+ - const: microchip,lan8651
+ - const: microchip,lan8650

So your DT node should either be:

compatible = "microchip,lan8651", "microchip,lan8650";

or

compatible = "microchip,lan8650"

There is no mention of lan865x in the binding, so this patch is
clearly wrong.

What do you have in your DT node?

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sat, Apr 27, 2024 at 09:35:06PM +0200, Ram?n Nordin Rodriguez wrote:
> I'm running a dual lan8650 setup, neither IC passed the sw reset in the
> oa_tc.c module, I need to pull the reset pin low to reset the pin before
> the rest of the init stuff happens.
>
> The datasheet recommends not doing a sw reset, excerpt from section
> 4.1.1.3 Software Reset
> "Note: The SW_RESET bit of the Clause 22 Basic Control register will reset only the internal PHY, not
> the entire device. This PHY only reset is not recommended for use. If such a reset is detected, by
> reading the RESETC bit of the STS2 register, reset the entire device."

That is not so good. The PHY driver does not know the PHY is embedded
within another device. It has no idea of RESETC bit in STS2. Looking
at the phy driver, i don't actually seeing it using
genphy_soft_reset(). Do you see a code path where this could actually
be an issue?

Supporting a hardware reset does however make sense. It would be best
if you submitted a proper clean patch. It can be added to the end of
this series, keeping you as author.

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Ok this tripped me up.

> The device tree binding says:
>
> + compatible:
> + oneOf:
> + - const: microchip,lan8650
> + - items:
> + - const: microchip,lan8651
> + - const: microchip,lan8650
>
> So your DT node should either be:
>
> compatible = "microchip,lan8651", "microchip,lan8650";
>
> or
>
> compatible = "microchip,lan8650"
>
> There is no mention of lan865x in the binding, so this patch is
> clearly wrong.
>
> What do you have in your DT node?

Initially I set compatible = "microchip,lan8650", and did not get the
driver to probe, so I got carried away with adding things that were not
necessary.

I dropped my patch and tested again.
What does work is setting:

compatible = "microchip,lan8651"

- or -

compatible = "microchip,lan8651", "microchip,lan8650"

but just compatible = "lan8650" does not work.

Also I'm getting the output
[ 0.125056] SPI driver lan8650 has no spi_device_id for microchip,lan8651

As Conor pointed out setting the define DRV_NAME to "lan8651" fixes
that.
Setting the define to "lan8650" yet gets the spi module to log the 'no spi_device id..'.

I don't really have an opinion here, but I think there is a risk that
more than one dev might stumble on the same thing as me and expect that
either or should work.

BR
R
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
> I think the error pretty much is what it says it is, the driver doesn't
> appear to have a spi_device_id table containing lan8650. The name of
> the driver is lan685x which is used in the fallback clause in
> __spi_register_driver(), so it complains as it does not find lan8650 in
> either. If my understanding is correct, either a spi_device_id table is
> required or the driver needs a rename with s/x/0/.
>

Right you are, no gdb necessary. With the caveat that I only get it
working when setting DRV_NAME to "lan8651", setting it to "lan8650"
still produces the log

R
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sat, Apr 27, 2024 at 10:58:07PM +0200, Andrew Lunn wrote:
> On Sat, Apr 27, 2024 at 09:35:06PM +0200, Ram?n Nordin Rodriguez wrote:
> > I'm running a dual lan8650 setup, neither IC passed the sw reset in the
> > oa_tc.c module, I need to pull the reset pin low to reset the pin before
> > the rest of the init stuff happens.
> >
> > The datasheet recommends not doing a sw reset, excerpt from section
> > 4.1.1.3 Software Reset
> > "Note: The SW_RESET bit of the Clause 22 Basic Control register will reset only the internal PHY, not
> > the entire device. This PHY only reset is not recommended for use. If such a reset is detected, by
> > reading the RESETC bit of the STS2 register, reset the entire device."
>
> That is not so good. The PHY driver does not know the PHY is embedded
> within another device. It has no idea of RESETC bit in STS2. Looking
> at the phy driver, i don't actually seeing it using
> genphy_soft_reset(). Do you see a code path where this could actually
> be an issue?
>

I agree with your assesment, the phy won't reset itself, but maybe we
could add some comment doc about not adding it for the lan8670,
so no one trips over that in the future.
Though the phy does not have to be baked with the lan8650 mac, so that
might be tricky to cover all future bases there. But for now I don't
think it matters.

Then regarding doing the soft reset in the oa_tc6 module, I'm not sure
that it matters, since it seems to work fine for me for as long as I do
the hw reset first.
But I can submit a suggestion for how to deal with reset-quirks if we
want to have the soft reset optional.

I'll run a test with and without the soft reset and see if I can spot
any change in behaviour.

Let me know if I missed any nuance in your question.

> Supporting a hardware reset does however make sense. It would be best
> if you submitted a proper clean patch. It can be added to the end of
> this series, keeping you as author.
>

I'd be happy to. Getting late in scandinavia so I'll clean it up and
submit tomorrow.

R
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
> but just compatible = "lan8650" does not work.

The device tree binding says that is valid, so this needs fixing.

Maybe copy max1111.c in hwmon:

static const struct spi_device_id max1111_ids[] = {
{ "max1110", max1110 },
{ "max1111", max1111 },
{ "max1112", max1112 },
{ "max1113", max1113 },
{ },
};
MODULE_DEVICE_TABLE(spi, max1111_ids);

static struct spi_driver max1111_driver = {
.driver = {
.name = "max1111",
},
.id_table = max1111_ids,
.probe = max1111_probe,
.remove = max1111_remove,
};

Interestingly, there is no compatible table. So the device tree
binding probably should change, not require two compatibles for
lan8651.

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
> I agree with your assesment, the phy won't reset itself, but maybe we
> could add some comment doc about not adding it for the lan8670,
> so no one trips over that in the future.

In the PHY driver, you can provide your own .soft_reset handler. It
could return -EOPNOTSUPP, or -EINVAL. Maybe check the data sheets for
the standalone devices supported by the driver. Can you limit this to
just the TC6 PHY?

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Sun, Apr 28, 2024 at 04:25:28PM +0200, Andrew Lunn wrote:
> > I agree with your assesment, the phy won't reset itself, but maybe we
> > could add some comment doc about not adding it for the lan8670,
> > so no one trips over that in the future.
>
> In the PHY driver, you can provide your own .soft_reset handler. It
> could return -EOPNOTSUPP, or -EINVAL. Maybe check the data sheets for
> the standalone devices supported by the driver. Can you limit this to
> just the TC6 PHY?
>

Gotcha, I think that should be pretty easy to handle then. The
microchip_t1s.c module handles two phy families
* lan865x - baked in
* lan867x - standalone

I need to do some thinking and get a bit more oriented. But pretty sure
there is a simple path for this.

R
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Hi Andrew,

On 26/04/24 11:44 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> OK, I will implement this helper function as oa_tc6_enable_zarfe() in
>> the oa_tc6.c. Also do you want me to move this helper function
>> implementation to a new patch?
>
> Yes please.
OK. Thanks.

Best regards,
Parthiban V
>
> Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Hi All,

On 28/04/24 1:27 am, Conor Dooley wrote:
> On Sat, Apr 27, 2024 at 09:19:34PM +0200, Ramón Nordin Rodriguez wrote:
>> Hi,
>>
>> For me the mac driver fails to probe with the following log
>> [ 0.123325] SPI driver lan865x has no spi_device_id for microchip,lan8651
>>
>> With this change the driver probes
>>
>> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
>> index 9abefa8b9d9f..72a663f14f50 100644
>> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
>> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
>> @@ -364,7 +364,7 @@ static void lan865x_remove(struct spi_device *spi)
>> }
>>
>> static const struct of_device_id lan865x_dt_ids[] = {
>> - { .compatible = "microchip,lan8651", "microchip,lan8650" },
> Huh, that's very strange. I don't see a single instance in the tree of a
> of_device_id struct like this with two compatibles like this (at least
> with a search of `rg "\.compatible.*\", \"" drivers/`.
>
> Given the fallbacks in the binding, only "microchip,lan8650" actually
> needs to be here.
>
>> + { .compatible = "microchip,lan865x", "microchip,lan8650" },
>> { /* Sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>>
>> Along with compatible = "microchip,lan865x" in the dts
> Just to be clear, the compatible w/ an x is unacceptable due to the
> wildcard and the binding should stay as-is. Whatever probing bugs
> the code has need to be resolved instead ????
>
Looks like, the below changes needed to work correctly,

lan865x.c:
- compatible string to be changed like below as it is a fallback for all
variants,
.compatible = "microchip,lan8650"
- DRV_NAME to be changed like below,
#define DRV_NAME "lan8650"

microchip,lan865x.example.dts for lan8650:
- compatible string to be changed like below,
.compatible = "microchip,lan8650";
OR
microchip,lan865x.example.dts for lan8651:
- compatible string to be changed like below,
compatible = "microchip,lan8651", "microchip,lan8650";

I tested with the above changes and there was no issues observed. Any
comments on this? Otherwise we can stick with these changes for the next
version.

Best regards,
Parthiban V

> Thanks,
> Conor.
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
> Looks like, the below changes needed to work correctly,
>
> lan865x.c:
> - compatible string to be changed like below as it is a fallback for all
> variants,
> .compatible = "microchip,lan8650"
> - DRV_NAME to be changed like below,
> #define DRV_NAME "lan8650"
>
> microchip,lan865x.example.dts for lan8650:
> - compatible string to be changed like below,
> .compatible = "microchip,lan8650";
> OR
> microchip,lan865x.example.dts for lan8651:
> - compatible string to be changed like below,
> compatible = "microchip,lan8651", "microchip,lan8650";
>
> I tested with the above changes and there was no issues observed. Any
> comments on this? Otherwise we can stick with these changes for the next
> version.

As Conor said, this is probably relying on the fallback
mechanism. Please look at other SPI devices, e.g. hwmon, and see how
they probe for multiple different devices.

Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Hi Andrew,

On 29/04/24 5:39 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> Looks like, the below changes needed to work correctly,
>>
>> lan865x.c:
>> - compatible string to be changed like below as it is a fallback for all
>> variants,
>> .compatible = "microchip,lan8650"
>> - DRV_NAME to be changed like below,
>> #define DRV_NAME "lan8650"
>>
>> microchip,lan865x.example.dts for lan8650:
>> - compatible string to be changed like below,
>> .compatible = "microchip,lan8650";
>> OR
>> microchip,lan865x.example.dts for lan8651:
>> - compatible string to be changed like below,
>> compatible = "microchip,lan8651", "microchip,lan8650";
>>
>> I tested with the above changes and there was no issues observed. Any
>> comments on this? Otherwise we can stick with these changes for the next
>> version.
>
> As Conor said, this is probably relying on the fallback
> mechanism. Please look at other SPI devices, e.g. hwmon, and see how
> they probe for multiple different devices.
I just referred the below drivers for the spi devices handling along
with the compatible,

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/davicom/dm9051.c#L1239

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/adi/adin1110.c#L1644

lan8650 - MAC-PHY chip
lan8651 - ETH Click-Mikroe with MAC-PHY chip

So, they are different in interface but not in functionality. There is
no difference in the configuration. So let's consider lan8650 is the
fallback option for lan8651.

By referring the above links, I have restructured the code like below to
support with lan8651 fallback. Still there is no change in the existing
device tree binding. This is the only change in lan865x.c.

static const struct spi_device_id spidev_spi_ids[] = {
{ .name = "lan8650" },
{},
};

static const struct of_device_id lan865x_dt_ids[] = {
{ .compatible = "microchip,lan8650" },
{ /* Sentinel */ }
};
MODULE_DEVICE_TABLE(of, lan865x_dt_ids);

static struct spi_driver lan865x_driver = {
.driver = {
.name = DRV_NAME,
.of_match_table = lan865x_dt_ids,
},
.probe = lan865x_probe,
.remove = lan865x_remove,
.id_table = spidev_spi_ids,
};

I also referred the below two links for the device tree binding and
driver in case of fallback.

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/smsc,lan9115.yaml#L18

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/smsc,lan9115.yaml#L102

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/smsc/smsc911x.c#L2652

Best regards,
Parthiban V
>
> Andrew
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
On Tue, Apr 30, 2024 at 01:30:22PM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Andrew,
>
> On 29/04/24 5:39 pm, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> >> Looks like, the below changes needed to work correctly,
> >>
> >> lan865x.c:
> >> - compatible string to be changed like below as it is a fallback for all
> >> variants,
> >> .compatible = "microchip,lan8650"
> >> - DRV_NAME to be changed like below,
> >> #define DRV_NAME "lan8650"
> >>
> >> microchip,lan865x.example.dts for lan8650:
> >> - compatible string to be changed like below,
> >> .compatible = "microchip,lan8650";
> >> OR
> >> microchip,lan865x.example.dts for lan8651:
> >> - compatible string to be changed like below,
> >> compatible = "microchip,lan8651", "microchip,lan8650";
> >>
> >> I tested with the above changes and there was no issues observed. Any
> >> comments on this? Otherwise we can stick with these changes for the next
> >> version.
> >
> > As Conor said, this is probably relying on the fallback
> > mechanism. Please look at other SPI devices, e.g. hwmon, and see how
> > they probe for multiple different devices.
> I just referred the below drivers for the spi devices handling along
> with the compatible,
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/davicom/dm9051.c#L1239
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/adi/adin1110.c#L1644
>
> lan8650 - MAC-PHY chip
> lan8651 - ETH Click-Mikroe with MAC-PHY chip
>
> So, they are different in interface but not in functionality. There is
> no difference in the configuration. So let's consider lan8650 is the
> fallback option for lan8651.
>
> By referring the above links, I have restructured the code like below to
> support with lan8651 fallback. Still there is no change in the existing
> device tree binding. This is the only change in lan865x.c.
>
> static const struct spi_device_id spidev_spi_ids[] = {
> { .name = "lan8650" },
> {},
> };
>
> static const struct of_device_id lan865x_dt_ids[] = {
> { .compatible = "microchip,lan8650" },
> { /* Sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>
> static struct spi_driver lan865x_driver = {
> .driver = {
> .name = DRV_NAME,
> .of_match_table = lan865x_dt_ids,
> },
> .probe = lan865x_probe,
> .remove = lan865x_remove,
> .id_table = spidev_spi_ids,
> };
>
> I also referred the below two links for the device tree binding and
> driver in case of fallback.

Did you also verify that the warning from the spi core is no longer
generated when using compatible = "microchip,lan8651", "microchip,lan8650"?
Re: [PATCH net-next v4 11/12] microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY [ In reply to ]
Hi Conor,

On 30/04/24 10:25 pm, Conor Dooley wrote:
> On Tue, Apr 30, 2024 at 01:30:22PM +0000,Parthiban.Veerasooran@microchip.com wrote:
>> Hi Andrew,
>>
>> On 29/04/24 5:39 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>>> Looks like, the below changes needed to work correctly,
>>>>
>>>> lan865x.c:
>>>> - compatible string to be changed like below as it is a fallback for all
>>>> variants,
>>>> .compatible = "microchip,lan8650"
>>>> - DRV_NAME to be changed like below,
>>>> #define DRV_NAME "lan8650"
>>>>
>>>> microchip,lan865x.example.dts for lan8650:
>>>> - compatible string to be changed like below,
>>>> .compatible = "microchip,lan8650";
>>>> OR
>>>> microchip,lan865x.example.dts for lan8651:
>>>> - compatible string to be changed like below,
>>>> compatible = "microchip,lan8651", "microchip,lan8650";
>>>>
>>>> I tested with the above changes and there was no issues observed. Any
>>>> comments on this? Otherwise we can stick with these changes for the next
>>>> version.
>>> As Conor said, this is probably relying on the fallback
>>> mechanism. Please look at other SPI devices, e.g. hwmon, and see how
>>> they probe for multiple different devices.
>> I just referred the below drivers for the spi devices handling along
>> with the compatible,
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/davicom/dm9051.c#L1239
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/adi/adin1110.c#L1644
>>
>> lan8650 - MAC-PHY chip
>> lan8651 - ETH Click-Mikroe with MAC-PHY chip
>>
>> So, they are different in interface but not in functionality. There is
>> no difference in the configuration. So let's consider lan8650 is the
>> fallback option for lan8651.
>>
>> By referring the above links, I have restructured the code like below to
>> support with lan8651 fallback. Still there is no change in the existing
>> device tree binding. This is the only change in lan865x.c.
>>
>> static const struct spi_device_id spidev_spi_ids[] = {
>> { .name = "lan8650" },
>> {},
>> };
>>
>> static const struct of_device_id lan865x_dt_ids[] = {
>> { .compatible = "microchip,lan8650" },
>> { /* Sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>>
>> static struct spi_driver lan865x_driver = {
>> .driver = {
>> .name = DRV_NAME,
>> .of_match_table = lan865x_dt_ids,
>> },
>> .probe = lan865x_probe,
>> .remove = lan865x_remove,
>> .id_table = spidev_spi_ids,
>> };
>>
>> I also referred the below two links for the device tree binding and
>> driver in case of fallback.
> Did you also verify that the warning from the spi core is no longer
> generated when using compatible = "microchip,lan8651", "microchip,lan8650"?
Do you mean changing in the lan865x.c file? if yes then I got the
warning "SPI driver lan865x has no spi_device_id for microchip,lan8651"

But after updating the lan865x.c like below, the warning disappeared.

static const struct spi_device_id spidev_spi_ids[] = {
{ .name = "lan8650" },
{ .name = "lan8651" },
{},
};

Note: In both the above two cases compatible in the dts is
compatible = "microchip,lan8651", "microchip,lan8650";

Best regards,
Parthiban V