This patch series contain the below updates,
- Adds support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface in the
net/ethernet/oa_tc6.c.
- Adds driver support for Microchip LAN8650/1 Rev.B0 10BASE-T1S MACPHY
Ethernet driver in the net/ethernet/microchip/lan865x.c.
Changes:
v2:
- Removed RFC tag.
- OA TC6 framework configured in the Kconfig and Makefile to compile as a
module.
- Kerneldoc headers added for all the API methods exposed to MAC driver.
- Odd parity calculation logic updated from the below link,
https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348
- Control buffer memory allocation moved to the initial function.
- struct oa_tc6 implemented as an obaque structure.
- Removed kthread for handling mac-phy interrupt instead threaded irq is
used.
- Removed interrupt implementation for soft reset handling instead of
that polling has been implemented.
- Registers name in the defines changed according to the specification
document.
- Registers defines are arranged in the order of offset and followed by
register fields.
- oa_tc6_write_register() implemented for writing a single register and
oa_tc6_write_registers() implemented for writing multiple registers.
- oa_tc6_read_register() implemented for reading a single register and
oa_tc6_read_registers() implemented for reading multiple registers.
- Removed DRV_VERSION macro as git hash provided by ethtool.
- Moved MDIO bus registration and PHY initialization to the OA TC6 lib.
- Replaced lan865x_set/get_link_ksettings() functions with
phy_ethtool_ksettings_set/get() functions.
- MAC-PHY's standard capability register values checked against the
user configured values.
- Removed unnecessary parameters validity check in various places.
- Removed MAC address configuration in the lan865x_net_open() function as
it is done in the lan865x_probe() function already.
- Moved standard registers and proprietary vendor registers to the
respective files.
- Added proper subject prefixes for the DT bindings.
- Moved OA specific properties to a separate DT bindings and corrected the
types & mistakes in the DT bindings.
- Inherited OA specific DT bindings to the LAN865x specific DT bindings.
- Removed sparse warnings in all the places.
- Used net_err_ratelimited() for printing the error messages.
- oa_tc6_process_rx_chunks() function and the content of oa_tc6_handler()
function are split into small functions.
- Used proper macros provided by network layer for calculating the
MAX_ETH_LEN.
- Return value of netif_rx() function handled properly.
- Removed unnecessary NULL initialization of skb in the
oa_tc6_rx_eth_ready() function removed.
- Local variables declaration ordered in reverse xmas tree notation.
Parthiban Veerasooran (9):
net: ethernet: implement OPEN Alliance control transaction interface
net: ethernet: oa_tc6: implement mac-phy software reset
net: ethernet: oa_tc6: implement OA TC6 configuration function
dt-bindings: net: add OPEN Alliance 10BASE-T1x MAC-PHY Serial
Interface
net: ethernet: oa_tc6: implement internal PHY initialization
dt-bindings: net: oa-tc6: add PHY register access capability
net: ethernet: oa_tc6: implement data transaction interface
microchip: lan865x: add driver support for Microchip's LAN865X MACPHY
dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY
.../bindings/net/microchip,lan865x.yaml | 101 ++
.../devicetree/bindings/net/oa-tc6.yaml | 86 ++
Documentation/networking/oa-tc6-framework.rst | 233 ++++
MAINTAINERS | 16 +
drivers/net/ethernet/Kconfig | 12 +
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/microchip/Kconfig | 11 +
drivers/net/ethernet/microchip/Makefile | 2 +
drivers/net/ethernet/microchip/lan865x.c | 415 ++++++
drivers/net/ethernet/oa_tc6.c | 1117 +++++++++++++++++
include/linux/oa_tc6.h | 109 ++
11 files changed, 2103 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
create mode 100644 Documentation/networking/oa-tc6-framework.rst
create mode 100644 drivers/net/ethernet/microchip/lan865x.c
create mode 100644 drivers/net/ethernet/oa_tc6.c
create mode 100644 include/linux/oa_tc6.h
--
2.34.1
Add DT bindings OPEN Alliance 10BASE-T1x MACPHY Serial Interface
parameters. These are generic properties that can apply to any 10BASE-T1x
MAC-PHY which uses OPEN Alliance TC6 specification.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
.../devicetree/bindings/net/oa-tc6.yaml | 72 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 73 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
diff --git a/Documentation/devicetree/bindings/net/oa-tc6.yaml b/Documentation/devicetree/bindings/net/oa-tc6.yaml
new file mode 100644
index 000000000000..9f442fa6cace
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/oa-tc6.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/oa-tc6.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OPEN Alliance 10BASE-T1x MAC-PHY Specification Common Properties
+
+maintainers:
+ - Parthiban Veerasooran <[email protected]>
+
+description:
+ These are generic properties that can apply to any 10BASE-T1x MAC-PHY
+ which uses OPEN Alliance TC6 specification.
+
+ 10BASE-T1x MAC-PHY Serial Interface Specification can be found at:
+ https://opensig.org/about/specifications/
+
+properties:
+ $nodename:
+ pattern: "^oa-tc6(@.*)?"
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ oa-cps:
+ maxItems: 1
+ description:
+ Chunk Payload Size. Configures the data chunk payload size to 2^N,
+ where N is the value of this bitfield. The minimum possible data
+ chunk payload size is 8 bytes or N = 3. The default data chunk
+ payload size is 64 bytes, or N = 6. The minimum supported data chunk
+ payload size for this MAC-PHY device is indicated in the CPSMIN
+ field of the CAPABILITY register. Valid values for this parameter
+ are 8, 16, 32 and 64. All other values are reserved.
+
+ oa-txcte:
+ maxItems: 1
+ description:
+ Transmit Cut-Through Enable. When supported by this MAC-PHY device,
+ this bit enables the cut-through mode of frame transfer through the
+ MAC-PHY device from the SPI host to the network.
+
+ oa-rxcte:
+ maxItems: 1
+ description:
+ Receive Cut-Through Enable. When supported by this MAC-PHY device,
+ this bit enables the cut-through mode of frame transfer through the
+ MAC-PHY device from the network to the SPI host.
+
+ oa-prote:
+ maxItems: 1
+ description:
+ Control data read/write Protection Enable. When set, all control
+ data written to and read from the MAC-PHY will be transferred with
+ its complement for detection of bit errors.
+
+additionalProperties: true
+
+examples:
+ - |
+ oa-tc6 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ oa-cps = <64>;
+ oa-txcte;
+ oa-rxcte;
+ oa-prote;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1c165026bbd4..9580be91f5e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15975,6 +15975,7 @@ OPEN ALLIANCE 10BASE-T1S MACPHY SERIAL INTERFACE FRAMEWORK
M: Parthiban Veerasooran <[email protected]>
L: [email protected]
S: Maintained
+F: Documentation/devicetree/bindings/net/oa-tc6.yaml
F: Documentation/networking/oa-tc6-framework.rst
F: drivers/include/linux/oa_tc6.h
F: drivers/net/ethernet/oa_tc6.c
--
2.34.1
Read STDCAP register for the MAC-PHY capability and check against the
configuration parameters such as chunk payload, tx cut through and rx cut
through to configure the MAC-PHY. It also configures the control command
protected/unprotected mode.
In cut through mode configuration the MAC-PHY doesn't buffer the incoming
data. In tx case, it passes the data to the network if the configured cps
of data available. In rx case, it passes the data to the SPI host if the
configured cps of data available from the network. Also disables all the
errors mask in the IMASK0 register to check for the errors.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 86 ++++++++++++++++++++++++++++++++++-
include/linux/oa_tc6.h | 24 +++++++++-
2 files changed, 107 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index e4457569135f..9a98d59f286d 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -8,6 +8,7 @@
#include <linux/bitfield.h>
#include <linux/interrupt.h>
#include <linux/delay.h>
+#include <linux/of.h>
#include <linux/oa_tc6.h>
/* Opaque structure for MACPHY drivers */
@@ -16,6 +17,7 @@ struct oa_tc6 {
u8 *ctrl_tx_buf;
u8 *ctrl_rx_buf;
bool prote;
+ u32 cps;
};
static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, u16 len)
@@ -198,6 +200,81 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
return ret;
}
+static int oa_tc6_configure(struct oa_tc6 *tc6)
+{
+ struct spi_device *spi = tc6->spi;
+ struct device_node *oa_node;
+ u32 regval;
+ u8 mincps;
+ bool ctc;
+ int ret;
+
+ /* Read and configure the IMASK0 register for unmasking the interrupts */
+ ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, false, false);
+ if (ret)
+ return ret;
+
+ regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
+
+ ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, true, false);
+ if (ret)
+ return ret;
+
+ /* Read STDCAP register to get the MAC-PHY standard capabilities */
+ ret = oa_tc6_perform_ctrl(tc6, STDCAP, ®val, 1, false, false);
+ if (ret)
+ return ret;
+
+ mincps = FIELD_GET(MINCPS, regval);
+ ctc = (regval & CTC) ? true : false;
+
+ regval = 0;
+ oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
+ if (oa_node) {
+ /* Read OA parameters from DT */
+ if (of_property_present(oa_node, "oa-cps")) {
+ ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);
+ if (ret < 0)
+ return ret;
+ /* Return error if the configured cps is less than the
+ * minimum cps supported by the MAC-PHY.
+ */
+ if (tc6->cps < mincps)
+ return -ENODEV;
+ } else {
+ tc6->cps = 64;
+ }
+ if (of_property_present(oa_node, "oa-txcte")) {
+ /* Return error if the tx cut through mode is configured
+ * but it is not supported by MAC-PHY.
+ */
+ if (ctc)
+ regval |= TXCTE;
+ else
+ return -ENODEV;
+ }
+ if (of_property_present(oa_node, "oa-rxcte")) {
+ /* Return error if the rx cut through mode is configured
+ * but it is not supported by MAC-PHY.
+ */
+ if (ctc)
+ regval |= RXCTE;
+ else
+ return -ENODEV;
+ }
+ if (of_property_present(oa_node, "oa-prote")) {
+ regval |= PROTE;
+ tc6->prote = true;
+ }
+ } else {
+ tc6->cps = 64;
+ }
+
+ regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
+
+ return oa_tc6_perform_ctrl(tc6, CONFIG0, ®val, 1, true, false);
+}
+
static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
{
u32 regval;
@@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
* Returns pointer reference to the oa_tc6 structure if all the memory
* allocation success otherwise NULL.
*/
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
{
struct oa_tc6 *tc6;
@@ -327,7 +404,6 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
return NULL;
tc6->spi = spi;
- tc6->prote = prote;
/* Perform MAC-PHY software reset */
if (oa_tc6_sw_reset(tc6)) {
@@ -335,6 +411,12 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
return NULL;
}
+ /* Perform OA parameters and MAC-PHY configuration */
+ if (oa_tc6_configure(tc6)) {
+ dev_err(&spi->dev, "OA and MAC-PHY configuration failed\n");
+ return NULL;
+ }
+
return tc6;
}
EXPORT_SYMBOL_GPL(oa_tc6_init);
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 8a838499da97..378636fd9ca8 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -22,15 +22,37 @@
#define TC6_CTRL_BUF_SIZE 1032 /* Max ctrl buffer size for 128 regs */
/* Open Alliance TC6 Standard Control and Status Registers */
+/* Standard Capabilities Register */
+#define STDCAP 0x0002
+#define CTC BIT(7) /* Cut-Through Capability */
+#define MINCPS GENMASK(2, 0) /* Minimum supported cps */
+
/* Reset Control and Status Register */
#define RESET 0x0003
#define SWRESET BIT(0) /* Software Reset */
+/* Configuration Register #0 */
+#define CONFIG0 0x0004
+#define SYNC BIT(15) /* Configuration Synchronization */
+#define TXCTE BIT(9) /* Tx cut-through enable */
+#define RXCTE BIT(8) /* Rx cut-through enable */
+#define PROTE BIT(5) /* Ctrl read/write Protection Enable */
+#define CPS GENMASK(2, 0) /* Chunk Payload Size */
+
/* Status Register #0 */
#define STATUS0 0x0008
#define RESETC BIT(6) /* Reset Complete */
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote);
+/* Interrupt Mask Register #0 */
+#define IMASK0 0x000C
+#define HDREM BIT(5) /* Header Error Mask */
+#define LOFEM BIT(4) /* Loss of Framing Error Mask */
+#define RXBOEM BIT(3) /* Rx Buffer Overflow Error Mask */
+#define TXBUEM BIT(2) /* Tx Buffer Underflow Error Mask */
+#define TXBOEM BIT(1) /* Tx Buffer Overflow Error Mask */
+#define TXPEM BIT(0) /* Tx Protocol Error Mask */
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val);
int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 *val);
int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len);
--
2.34.1
Internal PHY is initialized as per the PHY register capability supported
by the MAC-PHY. Direct PHY Register Access Capability indicates if PHY
registers are directly accessible within the SPI register memory space.
Indirect PHY Register Access Capability indicates if PHY registers are
indirectly accessible through the MDIO/MDC registers MDIOACCn.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 157 ++++++++++++++++++++++++++++++++--
include/linux/oa_tc6.h | 65 +++++++-------
2 files changed, 187 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 9a98d59f286d..a4532c83e909 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -6,16 +6,23 @@
*/
#include <linux/bitfield.h>
-#include <linux/interrupt.h>
#include <linux/delay.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
#include <linux/of.h>
#include <linux/oa_tc6.h>
/* Opaque structure for MACPHY drivers */
struct oa_tc6 {
+ struct net_device *netdev;
+ struct phy_device *phydev;
+ struct mii_bus *mdiobus;
struct spi_device *spi;
+ struct device *dev;
u8 *ctrl_tx_buf;
u8 *ctrl_rx_buf;
+ bool dprac;
+ bool iprac;
bool prote;
u32 cps;
};
@@ -204,6 +211,8 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
{
struct spi_device *spi = tc6->spi;
struct device_node *oa_node;
+ bool dprac;
+ bool iprac;
u32 regval;
u8 mincps;
bool ctc;
@@ -225,8 +234,14 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
if (ret)
return ret;
+ /* Minimum supported Chunk Payload Size */
mincps = FIELD_GET(MINCPS, regval);
+ /* Cut-Through Capability */
ctc = (regval & CTC) ? true : false;
+ /* Direct PHY Register Access Capability */
+ dprac = (regval & DPRAC) ? true : false;
+ /* Indirect PHY Register access Capability */
+ iprac = (regval & IPRAC) ? true : false;
regval = 0;
oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
@@ -242,7 +257,7 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
if (tc6->cps < mincps)
return -ENODEV;
} else {
- tc6->cps = 64;
+ tc6->cps = OA_TC6_MAX_CPS;
}
if (of_property_present(oa_node, "oa-txcte")) {
/* Return error if the tx cut through mode is configured
@@ -266,8 +281,26 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
regval |= PROTE;
tc6->prote = true;
}
+ if (of_property_present(oa_node, "oa-dprac")) {
+ /* Return error if the direct phy register access mode
+ * is configured but it is not supported by MAC-PHY.
+ */
+ if (dprac)
+ tc6->dprac = true;
+ else
+ return -ENODEV;
+ }
+ if (of_property_present(oa_node, "oa-iprac")) {
+ /* Return error if the indirect phy register access mode
+ * is configured but it is not supported by MAC-PHY.
+ */
+ if (iprac)
+ tc6->iprac = true;
+ else
+ return -ENODEV;
+ }
} else {
- tc6->cps = 64;
+ tc6->cps = OA_TC6_MAX_CPS;
}
regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
@@ -379,15 +412,108 @@ int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len)
}
EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
+static void oa_tc6_handle_link_change(struct net_device *netdev)
+{
+ phy_print_status(netdev->phydev);
+}
+
+static int oa_tc6_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+ u32 regval;
+ bool ret;
+
+ ret = oa_tc6_read_register(tc6, 0xFF00 | (idx & 0xFF), ®val);
+ if (ret)
+ return -ENODEV;
+
+ return regval;
+}
+
+static int oa_tc6_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
+ u16 val)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+
+ return oa_tc6_write_register(tc6, 0xFF00 | (idx & 0xFF), val);
+}
+
+static int oa_tc6_phy_init(struct oa_tc6 *tc6)
+{
+ int ret;
+
+ if (tc6->dprac) {
+ tc6->mdiobus = mdiobus_alloc();
+ if (!tc6->mdiobus) {
+ netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
+ return -ENODEV;
+ }
+
+ tc6->mdiobus->phy_mask = ~(u32)BIT(1);
+ tc6->mdiobus->priv = tc6;
+ tc6->mdiobus->read = oa_tc6_mdiobus_read;
+ tc6->mdiobus->write = oa_tc6_mdiobus_write;
+ tc6->mdiobus->name = "oa-tc6-mdiobus";
+ tc6->mdiobus->parent = tc6->dev;
+
+ snprintf(tc6->mdiobus->id, ARRAY_SIZE(tc6->mdiobus->id), "%s",
+ dev_name(&tc6->spi->dev));
+
+ ret = mdiobus_register(tc6->mdiobus);
+ if (ret) {
+ netdev_err(tc6->netdev, "Could not register MDIO bus\n");
+ mdiobus_free(tc6->mdiobus);
+ return ret;
+ }
+
+ tc6->phydev = phy_find_first(tc6->mdiobus);
+ if (!tc6->phydev) {
+ netdev_err(tc6->netdev, "No PHY found\n");
+ mdiobus_unregister(tc6->mdiobus);
+ mdiobus_free(tc6->mdiobus);
+ return -ENODEV;
+ }
+
+ tc6->phydev->is_internal = true;
+ ret = phy_connect_direct(tc6->netdev, tc6->phydev,
+ &oa_tc6_handle_link_change,
+ PHY_INTERFACE_MODE_INTERNAL);
+ if (ret) {
+ netdev_err(tc6->netdev, "Can't attach PHY to %s\n",
+ tc6->mdiobus->id);
+ mdiobus_unregister(tc6->mdiobus);
+ mdiobus_free(tc6->mdiobus);
+ return ret;
+ }
+
+ phy_attached_info(tc6->netdev->phydev);
+
+ return ret;
+ } else if (tc6->iprac) {
+ // To be implemented. Currently returns -ENODEV.
+ return -ENODEV;
+ } else {
+ return -ENODEV;
+ }
+ return 0;
+}
+
+static void oa_tc6_phy_exit(struct oa_tc6 *tc6)
+{
+ phy_disconnect(tc6->phydev);
+ mdiobus_unregister(tc6->mdiobus);
+ mdiobus_free(tc6->mdiobus);
+}
+
/**
* oa_tc6_init - allocates and intializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
- * @prote: control data (register) read/write protection enable/disable.
+ * @netdev: network device to use.
*
* Returns pointer reference to the oa_tc6 structure if all the memory
* allocation success otherwise NULL.
*/
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
{
struct oa_tc6 *tc6;
@@ -395,15 +521,19 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
if (!tc6)
return NULL;
+ /* Allocate memory for the control tx buffer used for SPI transfer. */
tc6->ctrl_tx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
if (!tc6->ctrl_tx_buf)
return NULL;
+ /* Allocate memory for the control rx buffer used for SPI transfer. */
tc6->ctrl_rx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
if (!tc6->ctrl_rx_buf)
return NULL;
tc6->spi = spi;
+ tc6->netdev = netdev;
+ SET_NETDEV_DEV(netdev, &spi->dev);
/* Perform MAC-PHY software reset */
if (oa_tc6_sw_reset(tc6)) {
@@ -417,10 +547,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
return NULL;
}
+ /* Initialize PHY */
+ if (oa_tc6_phy_init(tc6)) {
+ dev_err(&spi->dev, "PHY initialization failed\n");
+ return NULL;
+ }
+
return tc6;
}
EXPORT_SYMBOL_GPL(oa_tc6_init);
+/**
+ * oa_tc6_exit - exit function.
+ * @tc6: oa_tc6 struct.
+ *
+ */
+void oa_tc6_exit(struct oa_tc6 *tc6)
+{
+ oa_tc6_phy_exit(tc6);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_exit);
+
MODULE_DESCRIPTION("OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface Lib");
MODULE_AUTHOR("Parthiban Veerasooran <[email protected]>");
MODULE_LICENSE("GPL");
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 378636fd9ca8..36b729c384ac 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -5,54 +5,59 @@
* Author: Parthiban Veerasooran <[email protected]>
*/
+#include <linux/etherdevice.h>
#include <linux/spi/spi.h>
/* Control header */
-#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
-#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
-#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
-#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
-#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
-#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
-#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
-#define CTRL_HDR_P BIT(0) /* Parity Bit */
+#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
+#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
+#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
+#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
+#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
+#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
+#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
+#define CTRL_HDR_P BIT(0) /* Parity Bit */
#define TC6_HDR_SIZE 4 /* Ctrl command header size as per OA */
#define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */
#define TC6_CTRL_BUF_SIZE 1032 /* Max ctrl buffer size for 128 regs */
+#define OA_TC6_MAX_CPS 64
/* Open Alliance TC6 Standard Control and Status Registers */
/* Standard Capabilities Register */
-#define STDCAP 0x0002
-#define CTC BIT(7) /* Cut-Through Capability */
-#define MINCPS GENMASK(2, 0) /* Minimum supported cps */
+#define STDCAP 0x0002
+#define IPRAC BIT(9) /* Indirect PHY Reg access Capability */
+#define DPRAC BIT(8) /* Direct PHY Reg Access Capability */
+#define CTC BIT(7) /* Cut-Through Capability */
+#define MINCPS GENMASK(2, 0) /* Minimum supported cps */
/* Reset Control and Status Register */
-#define RESET 0x0003
-#define SWRESET BIT(0) /* Software Reset */
+#define RESET 0x0003
+#define SWRESET BIT(0) /* Software Reset */
/* Configuration Register #0 */
-#define CONFIG0 0x0004
-#define SYNC BIT(15) /* Configuration Synchronization */
-#define TXCTE BIT(9) /* Tx cut-through enable */
-#define RXCTE BIT(8) /* Rx cut-through enable */
-#define PROTE BIT(5) /* Ctrl read/write Protection Enable */
-#define CPS GENMASK(2, 0) /* Chunk Payload Size */
+#define CONFIG0 0x0004
+#define SYNC BIT(15) /* Configuration Synchronization */
+#define TXCTE BIT(9) /* Tx cut-through enable */
+#define RXCTE BIT(8) /* Rx cut-through enable */
+#define PROTE BIT(5) /* Ctrl read/write Protection Enable */
+#define CPS GENMASK(2, 0) /* Chunk Payload Size */
/* Status Register #0 */
-#define STATUS0 0x0008
-#define RESETC BIT(6) /* Reset Complete */
+#define STATUS0 0x0008
+#define RESETC BIT(6) /* Reset Complete */
/* Interrupt Mask Register #0 */
-#define IMASK0 0x000C
-#define HDREM BIT(5) /* Header Error Mask */
-#define LOFEM BIT(4) /* Loss of Framing Error Mask */
-#define RXBOEM BIT(3) /* Rx Buffer Overflow Error Mask */
-#define TXBUEM BIT(2) /* Tx Buffer Underflow Error Mask */
-#define TXBOEM BIT(1) /* Tx Buffer Overflow Error Mask */
-#define TXPEM BIT(0) /* Tx Protocol Error Mask */
-
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
+#define IMASK0 0x000C
+#define HDREM BIT(5) /* Header Error Mask */
+#define LOFEM BIT(4) /* Loss of Framing Error Mask */
+#define RXBOEM BIT(3) /* Rx Buffer Overflow Error Mask */
+#define TXBUEM BIT(2) /* Tx Buffer Underflow Error Mask */
+#define TXBOEM BIT(1) /* Tx Buffer Overflow Error Mask */
+#define TXPEM BIT(0) /* Tx Protocol Error Mask */
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev);
+void oa_tc6_exit(struct oa_tc6 *tc6);
int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val);
int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 *val);
int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len);
--
2.34.1
Direct PHY Register Access Capability indicates if PHY registers are
directly accessible within the SPI register memory space. Indirect PHY
Register Access Capability indicates if PHY registers are indirectly
accessible through the MDIO/MDC registers MDIOACCn.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
Documentation/devicetree/bindings/net/oa-tc6.yaml | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/oa-tc6.yaml b/Documentation/devicetree/bindings/net/oa-tc6.yaml
index 9f442fa6cace..09f1c11c68b9 100644
--- a/Documentation/devicetree/bindings/net/oa-tc6.yaml
+++ b/Documentation/devicetree/bindings/net/oa-tc6.yaml
@@ -58,6 +58,18 @@ properties:
data written to and read from the MAC-PHY will be transferred with
its complement for detection of bit errors.
+ oa-dprac:
+ maxItems: 1
+ description:
+ Direct PHY Register Access Capability. Indicates if PHY registers
+ are directly accessible within the SPI register memory space.
+
+ oa-dprac:
+ maxItems: 1
+ description:
+ Indirect PHY Register Access Capability. Indicates if PHY registers
+ are indirectly accessible through the MDIO/MDC registers MDIOACCn.
+
additionalProperties: true
examples:
@@ -69,4 +81,6 @@ examples:
oa-txcte;
oa-rxcte;
oa-prote;
+ oa-dprac;
+ oa-iprac;
};
--
2.34.1
The ethernet frame to be sent to MAC-PHY is converted into multiple
transmit data chunks. A transmit data chunk consists of a 4-byte data
header followed by the transmit data chunk payload.
The received ethernet frame from the network is converted into multiple
receive data chunks by the MAC-PHY and a receive data chunk consists of
the receive data chunk payload followed by a 4-byte data footer at the
end.
The MAC-PHY shall support a default data chunk payload size of 64 bytes.
Data chunk payload sizes of 32, 16, or 8 bytes may also be supported. The
data chunk payload is always a multiple of 4 bytes.
The 4-byte data header occurs at the beginning of each transmit data
chunk on MOSI and the 4-byte data footer occurs at the end of each
receive data chunk on MISO. The data header and footer contain the
information needed to determine the validity and location of the transmit
and receive frame data within the data chunk payload. Ethernet frames
shall be aligned to a 32-bit boundary within the data chunk payload.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 546 +++++++++++++++++++++++++++++++++-
include/linux/oa_tc6.h | 47 ++-
2 files changed, 591 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index a4532c83e909..1306ca0b0884 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -14,17 +14,36 @@
/* Opaque structure for MACPHY drivers */
struct oa_tc6 {
+ int (*config_cps_buf)(void *tc6, u32 cps);
+ struct work_struct tx_work;
struct net_device *netdev;
struct phy_device *phydev;
struct mii_bus *mdiobus;
struct spi_device *spi;
+ struct sk_buff *tx_skb;
+ bool rx_eth_started;
struct device *dev;
+ /* Protects oa_tc6_perform_spi_xfer function elements between MAC-PHY
+ * interrupt handler and the tx work handler.
+ */
+ struct mutex lock;
u8 *ctrl_tx_buf;
u8 *ctrl_rx_buf;
+ u8 *spi_tx_buf;
+ u8 *spi_rx_buf;
+ u8 *eth_tx_buf;
+ u8 *eth_rx_buf;
+ u16 rxd_bytes;
+ u8 txc_needed;
+ bool int_flag;
+ bool tx_flag;
bool dprac;
bool iprac;
bool prote;
+ u16 tx_pos;
u32 cps;
+ u8 txc;
+ u8 rca;
};
static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, u16 len)
@@ -55,6 +74,24 @@ static int oa_tc6_get_parity(u32 p)
return !((p >> 28) & 1);
}
+static u16 oa_tc6_prepare_empty_chunk(struct oa_tc6 *tc6, u8 *buf, u8 cp_count)
+{
+ u32 hdr;
+
+ /* Prepare empty chunks used for getting interrupt information or if
+ * receive data available.
+ */
+ for (u8 i = 0; i < cp_count; i++) {
+ hdr = FIELD_PREP(DATA_HDR_DNC, 1);
+ hdr |= FIELD_PREP(DATA_HDR_P, oa_tc6_get_parity(hdr));
+ *(__be32 *)&buf[i * (tc6->cps + TC6_HDR_SIZE)] = cpu_to_be32(hdr);
+ memset(&buf[TC6_HDR_SIZE + (i * (tc6->cps + TC6_HDR_SIZE))], 0,
+ tc6->cps);
+ }
+
+ return cp_count * (tc6->cps + TC6_HDR_SIZE);
+}
+
static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
u8 len, bool wnr, u8 *buf, bool prote)
{
@@ -218,6 +255,14 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
bool ctc;
int ret;
+ /* Read BUFSTS register to get the current txc and rca. */
+ ret = oa_tc6_read_register(tc6, OA_TC6_BUFSTS, ®val);
+ if (ret)
+ return ret;
+
+ tc6->txc = FIELD_GET(TXC, regval);
+ tc6->rca = FIELD_GET(RCA, regval);
+
/* Read and configure the IMASK0 register for unmasking the interrupts */
ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, false, false);
if (ret)
@@ -259,6 +304,12 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
} else {
tc6->cps = OA_TC6_MAX_CPS;
}
+ /* Call queue buffer size config function if defined by MAC */
+ if (tc6->config_cps_buf) {
+ ret = tc6->config_cps_buf(tc6, tc6->cps);
+ if (ret)
+ return ret;
+ }
if (of_property_present(oa_node, "oa-txcte")) {
/* Return error if the tx cut through mode is configured
* but it is not supported by MAC-PHY.
@@ -498,6 +549,240 @@ static int oa_tc6_phy_init(struct oa_tc6 *tc6)
return 0;
}
+static int oa_tc6_process_exst(struct oa_tc6 *tc6)
+{
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, STATUS0, ®val);
+ if (ret)
+ return ret;
+
+ if (regval & TXPE)
+ net_err_ratelimited("%s: Transmit protocol error\n",
+ tc6->netdev->name);
+
+ if (regval & TXBOE)
+ net_err_ratelimited("%s: Transmit buffer overflow\n",
+ tc6->netdev->name);
+
+ if (regval & TXBUE)
+ net_err_ratelimited("%s: Transmit buffer underflow\n",
+ tc6->netdev->name);
+
+ if (regval & RXBOE)
+ net_err_ratelimited("%s: Receive buffer overflow\n",
+ tc6->netdev->name);
+
+ if (regval & LOFE)
+ net_err_ratelimited("%s: Loss of frame\n", tc6->netdev->name);
+
+ if (regval & HDRE)
+ net_err_ratelimited("%s: Header error\n", tc6->netdev->name);
+
+ if (regval & TXFCSE)
+ net_err_ratelimited("%s: Tx Frame Check Seq Error\n",
+ tc6->netdev->name);
+
+ return oa_tc6_write_register(tc6, STATUS0, regval);
+}
+
+static void oa_tc6_rx_eth_ready(struct oa_tc6 *tc6)
+{
+ struct sk_buff *skb;
+
+ /* Send the received ethernet packet to network layer */
+ skb = netdev_alloc_skb(tc6->netdev, tc6->rxd_bytes + NET_IP_ALIGN);
+ if (!skb) {
+ tc6->netdev->stats.rx_dropped++;
+ netdev_dbg(tc6->netdev, "Out of memory for rx'd frame");
+ } else {
+ skb_reserve(skb, NET_IP_ALIGN);
+ memcpy(skb_put(skb, tc6->rxd_bytes), &tc6->eth_rx_buf[0],
+ tc6->rxd_bytes);
+ skb->protocol = eth_type_trans(skb, tc6->netdev);
+ tc6->netdev->stats.rx_packets++;
+ tc6->netdev->stats.rx_bytes += tc6->rxd_bytes;
+ /* 0 for NET_RX_SUCCESS and 1 for NET_RX_DROP */
+ if (netif_rx(skb))
+ tc6->netdev->stats.rx_dropped++;
+ }
+}
+
+static void oa_tc6_rx_eth_complete2(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
+{
+ u16 ebo;
+
+ if (FIELD_GET(DATA_FTR_EV, ftr))
+ ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
+ else
+ ebo = tc6->cps;
+
+ memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
+ tc6->rxd_bytes += ebo;
+ if (FIELD_GET(DATA_FTR_EV, ftr)) {
+ /* If EV set then send the received ethernet frame to n/w */
+ oa_tc6_rx_eth_ready(tc6);
+ tc6->rxd_bytes = 0;
+ tc6->rx_eth_started = false;
+ }
+}
+
+static void oa_tc6_rx_eth_complete1(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
+{
+ u16 ebo;
+ u16 sbo;
+
+ sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
+ ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
+
+ if (ebo <= sbo) {
+ memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
+ tc6->rxd_bytes += ebo;
+ oa_tc6_rx_eth_ready(tc6);
+ tc6->rxd_bytes = 0;
+ memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
+ tc6->cps - sbo);
+ tc6->rxd_bytes += (tc6->cps - sbo);
+ } else {
+ memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
+ ebo - sbo);
+ tc6->rxd_bytes += (ebo - sbo);
+ oa_tc6_rx_eth_ready(tc6);
+ tc6->rxd_bytes = 0;
+ }
+}
+
+static void oa_tc6_start_rx_eth(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
+{
+ u16 sbo;
+
+ tc6->rxd_bytes = 0;
+ tc6->rx_eth_started = true;
+ sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
+ memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo], tc6->cps - sbo);
+ tc6->rxd_bytes += (tc6->cps - sbo);
+}
+
+static u32 oa_tc6_get_footer(struct oa_tc6 *tc6, u8 *buf, u8 cp_num)
+{
+ __be32 ftr;
+
+ ftr = *(__be32 *)&buf[tc6->cps + (cp_num * (tc6->cps + TC6_FTR_SIZE))];
+
+ return be32_to_cpu(ftr);
+}
+
+static void oa_tc6_update_txc_rca(struct oa_tc6 *tc6, u32 ftr)
+{
+ tc6->txc = FIELD_GET(DATA_FTR_TXC, ftr);
+ tc6->rca = FIELD_GET(DATA_FTR_RCA, ftr);
+}
+
+static int oa_tc6_check_ftr_errors(struct oa_tc6 *tc6, u32 ftr)
+{
+ /* Check for footer parity error */
+ if (oa_tc6_get_parity(ftr)) {
+ net_err_ratelimited("%s: Footer parity error\n",
+ tc6->netdev->name);
+ return FTR_ERR;
+ }
+ /* If EXST set in the footer then read STS0 register to get the
+ * status information.
+ */
+ if (FIELD_GET(DATA_FTR_EXST, ftr)) {
+ if (oa_tc6_process_exst(tc6))
+ net_err_ratelimited("%s: Failed to process EXST\n",
+ tc6->netdev->name);
+ return FTR_ERR;
+ }
+ if (FIELD_GET(DATA_FTR_HDRB, ftr)) {
+ net_err_ratelimited("%s: Footer eeceived header bad\n",
+ tc6->netdev->name);
+ return FTR_ERR;
+ }
+ if (!FIELD_GET(DATA_FTR_SYNC, ftr)) {
+ net_err_ratelimited("%s: Footer configuration unsync\n",
+ tc6->netdev->name);
+ return FTR_ERR;
+ }
+ return FTR_OK;
+}
+
+static void oa_tc6_drop_rx_eth(struct oa_tc6 *tc6)
+{
+ tc6->rxd_bytes = 0;
+ tc6->rx_eth_started = false;
+ tc6->netdev->stats.rx_dropped++;
+ net_err_ratelimited("%s: Footer frame drop\n",
+ tc6->netdev->name);
+}
+
+static int oa_tc6_process_rx_chunks(struct oa_tc6 *tc6, u8 *buf, u16 len)
+{
+ u8 cp_count;
+ u8 *payload;
+ u32 ftr;
+ int ret;
+
+ /* Calculate the number of chunks received */
+ cp_count = len / (tc6->cps + TC6_FTR_SIZE);
+
+ for (u8 i = 0; i < cp_count; i++) {
+ /* Get the footer and payload */
+ ftr = oa_tc6_get_footer(tc6, buf, i);
+ payload = &buf[(i * (tc6->cps + TC6_FTR_SIZE))];
+ /* Check for footer errors */
+ ret = oa_tc6_check_ftr_errors(tc6, ftr);
+ if (ret) {
+ if (tc6->rx_eth_started)
+ oa_tc6_drop_rx_eth(tc6);
+ return ret;
+ }
+ /* If Frame Drop is set, indicates that the MAC has detected a
+ * condition for which the SPI host should drop the received
+ * ethernet frame.
+ */
+ if (FIELD_GET(DATA_FTR_FD, ftr) && FIELD_GET(DATA_FTR_EV, ftr)) {
+ if (tc6->rx_eth_started)
+ oa_tc6_drop_rx_eth(tc6);
+
+ if (FIELD_GET(DATA_FTR_SV, ftr)) {
+ oa_tc6_start_rx_eth(tc6, payload, ftr);
+ oa_tc6_update_txc_rca(tc6, ftr);
+ }
+ continue;
+ }
+ /* Check for data valid */
+ if (FIELD_GET(DATA_FTR_DV, ftr)) {
+ /* Check whether both start valid and end valid are in a
+ * single chunk payload means a single chunk payload may
+ * contain an entire ethernet frame.
+ */
+ if (FIELD_GET(DATA_FTR_SV, ftr) &&
+ FIELD_GET(DATA_FTR_EV, ftr)) {
+ oa_tc6_rx_eth_complete1(tc6, payload, ftr);
+ oa_tc6_update_txc_rca(tc6, ftr);
+ continue;
+ }
+ /* Check for start valid to start capturing the incoming
+ * ethernet frame.
+ */
+ if (FIELD_GET(DATA_FTR_SV, ftr) && !tc6->rx_eth_started) {
+ oa_tc6_start_rx_eth(tc6, payload, ftr);
+ oa_tc6_update_txc_rca(tc6, ftr);
+ continue;
+ }
+
+ /* Check for end valid and calculate the copy length */
+ if (tc6->rx_eth_started)
+ oa_tc6_rx_eth_complete2(tc6, payload, ftr);
+ }
+ oa_tc6_update_txc_rca(tc6, ftr);
+ }
+ return FTR_OK;
+}
+
static void oa_tc6_phy_exit(struct oa_tc6 *tc6)
{
phy_disconnect(tc6->phydev);
@@ -505,17 +790,237 @@ static void oa_tc6_phy_exit(struct oa_tc6 *tc6)
mdiobus_free(tc6->mdiobus);
}
+static void oa_tc6_prepare_tx_chunks(struct oa_tc6 *tc6, u8 *buf,
+ struct sk_buff *skb)
+{
+ bool frame_started = false;
+ u16 copied_bytes = 0;
+ u16 copy_len;
+ u32 hdr;
+
+ /* Calculate the number tx credit counts needed to transport the tx
+ * ethernet frame.
+ */
+ tc6->txc_needed = (skb->len / tc6->cps) + ((skb->len % tc6->cps) ? 1 : 0);
+
+ for (u8 i = 0; i < tc6->txc_needed; i++) {
+ /* Prepare the header for each chunks to be transmitted */
+ hdr = FIELD_PREP(DATA_HDR_DNC, 1) |
+ FIELD_PREP(DATA_HDR_DV, 1);
+ if (!frame_started) {
+ hdr |= FIELD_PREP(DATA_HDR_SV, 1) |
+ FIELD_PREP(DATA_HDR_SWO, 0);
+ frame_started = true;
+ }
+ if ((tc6->cps + copied_bytes) >= skb->len) {
+ copy_len = skb->len - copied_bytes;
+ hdr |= FIELD_PREP(DATA_HDR_EBO, copy_len - 1) |
+ FIELD_PREP(DATA_HDR_EV, 1);
+ } else {
+ copy_len = tc6->cps;
+ }
+ copied_bytes += copy_len;
+ hdr |= FIELD_PREP(DATA_HDR_P, oa_tc6_get_parity(hdr));
+ *(__be32 *)&buf[i * (tc6->cps + TC6_HDR_SIZE)] = cpu_to_be32(hdr);
+ /* Copy the ethernet frame in the chunk payload section */
+ memcpy(&buf[TC6_HDR_SIZE + (i * (tc6->cps + TC6_HDR_SIZE))],
+ &skb->data[copied_bytes - copy_len], copy_len);
+ }
+}
+
+static u16 oa_tc6_calculate_tx_len(struct oa_tc6 *tc6)
+{
+ /* If the available txc is greater than the txc needed (calculated from
+ * the tx ethernet frame then the tx length can be txc needed. Else the
+ * tx length can be available txc and the remaining needed txc will be
+ * updated either in the footer of the current transfer or through the
+ * interrupt.
+ */
+ if (tc6->txc >= tc6->txc_needed)
+ return tc6->txc_needed * (tc6->cps + TC6_HDR_SIZE);
+ else
+ return tc6->txc * (tc6->cps + TC6_HDR_SIZE);
+}
+
+static u16 oa_tc6_calculate_rca_len(struct oa_tc6 *tc6, u16 tx_len)
+{
+ u16 rca_needed = 0;
+ u16 tx_txc;
+
+ /* If tx eth frame and rca are available at the same time then check
+ * whether the rca is less than the needed txc for the tx eth frame. If
+ * not then add additional empty chunks along with the tx chunks to get
+ * all the rca.
+ */
+ if (tc6->tx_flag && tc6->txc) {
+ tx_txc = tc6->txc_needed - (tx_len / (tc6->cps + TC6_HDR_SIZE));
+ if (tx_txc < tc6->rca)
+ rca_needed = tc6->rca - tx_txc;
+ } else {
+ /* Add only empty chunks for rca if there is no tx chunks
+ * available to transmit.
+ */
+ rca_needed = tc6->rca;
+ }
+ return oa_tc6_prepare_empty_chunk(tc6, &tc6->spi_tx_buf[tx_len],
+ rca_needed);
+}
+
+static void oa_tc6_tx_eth_complete(struct oa_tc6 *tc6)
+{
+ tc6->netdev->stats.tx_packets++;
+ tc6->netdev->stats.tx_bytes += tc6->tx_skb->len;
+ dev_kfree_skb(tc6->tx_skb);
+ tc6->tx_pos = 0;
+ tc6->tx_skb = NULL;
+ tc6->tx_flag = false;
+ if (netif_queue_stopped(tc6->netdev))
+ netif_wake_queue(tc6->netdev);
+}
+
+static int oa_tc6_perform_spi_xfer(struct oa_tc6 *tc6)
+{
+ bool do_tx_again;
+ u16 total_len;
+ u16 rca_len;
+ u16 tx_len;
+ int ret;
+
+ do {
+ do_tx_again = false;
+ rca_len = 0;
+ tx_len = 0;
+
+ /* In case of an interrupt, perform an empty chunk transfer to
+ * know the purpose of the interrupt. Interrupt may occur in
+ * case of RCA (Receive Chunk Available) and TXC (Transmit
+ * Credit Count). Both will occur if they are not indicated
+ * through the previous footer.
+ */
+ if (tc6->int_flag) {
+ tc6->int_flag = false;
+ total_len = oa_tc6_prepare_empty_chunk(tc6,
+ tc6->spi_tx_buf,
+ 1);
+ } else {
+ /* Calculate the transfer length */
+ if (tc6->tx_flag && tc6->txc) {
+ tx_len = oa_tc6_calculate_tx_len(tc6);
+ memcpy(&tc6->spi_tx_buf[0],
+ &tc6->eth_tx_buf[tc6->tx_pos], tx_len);
+ }
+
+ if (tc6->rca)
+ rca_len = oa_tc6_calculate_rca_len(tc6, tx_len);
+
+ total_len = tx_len + rca_len;
+ }
+ ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf,
+ tc6->spi_rx_buf, total_len);
+ if (ret)
+ return ret;
+ /* Process the rxd chunks to get the ethernet frame or status */
+ ret = oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, total_len);
+ if (ret)
+ return ret;
+ if (tc6->tx_flag) {
+ tc6->tx_pos += tx_len;
+ tc6->txc_needed = tc6->txc_needed -
+ (tx_len / (tc6->cps + TC6_HDR_SIZE));
+ /* If the complete ethernet frame is transmitted then
+ * return the skb and update the details to n/w layer.
+ */
+ if (!tc6->txc_needed)
+ oa_tc6_tx_eth_complete(tc6);
+ else if (tc6->txc)
+ /* If txc is available again and updated from
+ * the previous footer then perform tx again.
+ */
+ do_tx_again = true;
+ }
+
+ /* If rca is updated from the previous footer then perform empty
+ * tx to receive ethernet frame.
+ */
+ if (tc6->rca)
+ do_tx_again = true;
+ } while (do_tx_again);
+
+ return 0;
+}
+
+/* MAC-PHY interrupt handler */
+static irqreturn_t macphy_irq_handler(int irq, void *dev_id)
+{
+ struct oa_tc6 *tc6 = dev_id;
+
+ tc6->int_flag = true;
+ mutex_lock(&tc6->lock);
+ if (oa_tc6_perform_spi_xfer(tc6))
+ net_err_ratelimited("%s: SPI transfer failed\n",
+ tc6->netdev->name);
+ mutex_unlock(&tc6->lock);
+
+ return IRQ_HANDLED;
+}
+
+/* Workqueue to perform SPI transfer */
+static void tc6_tx_work_handler(struct work_struct *work)
+{
+ struct oa_tc6 *tc6 = container_of(work, struct oa_tc6, tx_work);
+
+ mutex_lock(&tc6->lock);
+ if (oa_tc6_perform_spi_xfer(tc6))
+ net_err_ratelimited("%s: SPI transfer failed\n",
+ tc6->netdev->name);
+ mutex_unlock(&tc6->lock);
+}
+
+/**
+ * oa_tc6_send_eth_pkt - function for sending the tx ethernet frame.
+ * @tc6: oa_tc6 struct.
+ * @skb: socket buffer in which the ethernet frame is stored.
+ *
+ * As this is called from atomic context, work queue is used here because the
+ * spi_sync will block for the transfer completion.
+ *
+ * Returns NETDEV_TX_OK if the tx work is scheduled or NETDEV_TX_BUSY if the
+ * previous enqueued tx skb is in progress.
+ */
+netdev_tx_t oa_tc6_send_eth_pkt(struct oa_tc6 *tc6, struct sk_buff *skb)
+{
+ if (tc6->tx_flag) {
+ netif_stop_queue(tc6->netdev);
+ return NETDEV_TX_BUSY;
+ }
+
+ tc6->tx_skb = skb;
+ /* Prepare tx chunks using the tx ethernet frame */
+ oa_tc6_prepare_tx_chunks(tc6, tc6->eth_tx_buf, skb);
+
+ tc6->tx_flag = true;
+ schedule_work(&tc6->tx_work);
+
+ return NETDEV_TX_OK;
+}
+EXPORT_SYMBOL_GPL(oa_tc6_send_eth_pkt);
+
/**
* oa_tc6_init - allocates and intializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
* @netdev: network device to use.
+ * @config_cps_buf: function pointer passed by MAC driver to be called for
+ * configuring cps buffer size. Queue buffer size in the MAC has to be configured
+ * according to the cps.
*
* Returns pointer reference to the oa_tc6 structure if all the memory
* allocation success otherwise NULL.
*/
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
+ int (*config_cps_buf)(void *tc6, u32 cps))
{
struct oa_tc6 *tc6;
+ int ret;
tc6 = devm_kzalloc(&spi->dev, sizeof(*tc6), GFP_KERNEL);
if (!tc6)
@@ -531,9 +1036,37 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
if (!tc6->ctrl_rx_buf)
return NULL;
+ /* Allocate memory for the tx buffer used for SPI transfer. */
+ tc6->spi_tx_buf = devm_kzalloc(&spi->dev, ETH_LEN + (OA_TC6_MAX_CPS * TC6_HDR_SIZE),
+ GFP_KERNEL);
+ if (!tc6->spi_tx_buf)
+ return NULL;
+
+ /* Allocate memory for the rx buffer used for SPI transfer. */
+ tc6->spi_rx_buf = devm_kzalloc(&spi->dev, ETH_LEN + (OA_TC6_MAX_CPS * TC6_FTR_SIZE),
+ GFP_KERNEL);
+ if (!tc6->spi_rx_buf)
+ return NULL;
+
+ /* Allocate memory for the tx ethernet chunks to transfer on SPI. */
+ tc6->eth_tx_buf = devm_kzalloc(&spi->dev, ETH_LEN + (OA_TC6_MAX_CPS * TC6_HDR_SIZE),
+ GFP_KERNEL);
+ if (!tc6->eth_tx_buf)
+ return NULL;
+
+ /* Allocate memory for the rx ethernet packet. */
+ tc6->eth_rx_buf = devm_kzalloc(&spi->dev, ETH_LEN + (OA_TC6_MAX_CPS * TC6_FTR_SIZE),
+ GFP_KERNEL);
+ if (!tc6->eth_rx_buf)
+ return NULL;
+
tc6->spi = spi;
tc6->netdev = netdev;
SET_NETDEV_DEV(netdev, &spi->dev);
+ tc6->config_cps_buf = config_cps_buf;
+ /* Set the SPI controller to pump at realtime priority */
+ spi->rt = true;
+ spi_setup(spi);
/* Perform MAC-PHY software reset */
if (oa_tc6_sw_reset(tc6)) {
@@ -552,6 +1085,16 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
dev_err(&spi->dev, "PHY initialization failed\n");
return NULL;
}
+ mutex_init(&tc6->lock);
+ INIT_WORK(&tc6->tx_work, tc6_tx_work_handler);
+ /* Register MAC-PHY interrupt service routine */
+ ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
+ macphy_irq_handler, IRQF_ONESHOT,
+ "macphy int", tc6);
+ if (ret) {
+ dev_err(&spi->dev, "Error attaching macphy irq %d\n", ret);
+ return NULL;
+ }
return tc6;
}
@@ -564,6 +1107,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);
*/
void oa_tc6_exit(struct oa_tc6 *tc6)
{
+ devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6);
oa_tc6_phy_exit(tc6);
}
EXPORT_SYMBOL_GPL(oa_tc6_exit);
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 36b729c384ac..58d1143f15ba 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -18,9 +18,39 @@
#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
#define CTRL_HDR_P BIT(0) /* Parity Bit */
+/* Data header */
+#define DATA_HDR_DNC BIT(31) /* Data-Not-Control */
+#define DATA_HDR_SEQ BIT(30) /* Data Chunk Sequence */
+#define DATA_HDR_NORX BIT(29) /* No Receive */
+#define DATA_HDR_DV BIT(21) /* Data Valid */
+#define DATA_HDR_SV BIT(20) /* Start Valid */
+#define DATA_HDR_SWO GENMASK(19, 16) /* Start Word Offset */
+#define DATA_HDR_EV BIT(14) /* End Valid */
+#define DATA_HDR_EBO GENMASK(13, 8) /* End Byte Offset */
+#define DATA_HDR_P BIT(0) /* Header Parity Bit */
+
+/* Data footer */
+#define DATA_FTR_EXST BIT(31) /* Extended Status */
+#define DATA_FTR_HDRB BIT(30) /* Received Header Bad */
+#define DATA_FTR_SYNC BIT(29) /* Configuration Synchronized */
+#define DATA_FTR_RCA GENMASK(28, 24) /* Receive Chunks Available */
+#define DATA_FTR_DV BIT(21) /* Data Valid */
+#define DATA_FTR_SV BIT(20) /* Start Valid */
+#define DATA_FTR_SWO GENMASK(19, 16) /* Start Word Offset */
+#define DATA_FTR_FD BIT(15) /* Frame Drop */
+#define DATA_FTR_EV BIT(14) /* End Valid */
+#define DATA_FTR_EBO GENMASK(13, 8) /* End Byte Offset */
+#define DATA_FTR_TXC GENMASK(5, 1) /* Transmit Credits */
+#define DATA_FTR_P BIT(0) /* Footer Parity Bit */
+
#define TC6_HDR_SIZE 4 /* Ctrl command header size as per OA */
#define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */
#define TC6_CTRL_BUF_SIZE 1032 /* Max ctrl buffer size for 128 regs */
+
+#define FTR_OK 0
+#define FTR_ERR 1
+
+#define ETH_LEN (ETH_DATA_LEN + ETH_HLEN + ETH_FCS_LEN)
#define OA_TC6_MAX_CPS 64
/* Open Alliance TC6 Standard Control and Status Registers */
@@ -45,7 +75,20 @@
/* Status Register #0 */
#define STATUS0 0x0008
+#define CDPE BIT(12) /* Control Data Protection Error */
+#define TXFCSE BIT(11) /* Transmit Frame Check Sequence Error */
#define RESETC BIT(6) /* Reset Complete */
+#define HDRE BIT(5) /* Header Error */
+#define LOFE BIT(4) /* Loss of Framing Error */
+#define RXBOE BIT(3) /* Receive Buffer Overflow Error */
+#define TXBUE BIT(2) /* Transmit Buffer Underflow Error */
+#define TXBOE BIT(1) /* Transmit Buffer Overflow Error */
+#define TXPE BIT(0) /* Transmit Protocol Error */
+
+/* Buffer Status Register */
+#define OA_TC6_BUFSTS 0x000B
+#define TXC GENMASK(15, 8) /* Transmit Credits Available */
+#define RCA GENMASK(7, 0) /* Receive Chunks Available */
/* Interrupt Mask Register #0 */
#define IMASK0 0x000C
@@ -56,9 +99,11 @@
#define TXBOEM BIT(1) /* Tx Buffer Overflow Error Mask */
#define TXPEM BIT(0) /* Tx Protocol Error Mask */
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev);
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
+ int (*config_cps_buf)(void *tc6, u32 cps));
void oa_tc6_exit(struct oa_tc6 *tc6);
int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val);
int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 *val);
int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len);
int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len);
+netdev_tx_t oa_tc6_send_eth_pkt(struct oa_tc6 *tc6, struct sk_buff *skb);
--
2.34.1
Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
combines a Media Access Controller (MAC) and an Ethernet PHY to enable
10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
802.3 standard and a 10BASE-T1S physical layer transceiver integrated
into the LAN8650/1. The communication between the Host and the MAC-PHY is
specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
.../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 102 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
new file mode 100644
index 000000000000..974622dd6846
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
+
+maintainers:
+ - Parthiban Veerasooran <[email protected]>
+
+description:
+ The LAN8650/1 combines a Media Access Controller (MAC) and an Ethernet
+ PHY to enable 10BASE‑T1S networks. The Ethernet Media Access Controller
+ (MAC) module implements a 10 Mbps half duplex Ethernet MAC, compatible
+ with the IEEE 802.3 standard and a 10BASE-T1S physical layer transceiver
+ integrated into the LAN8650/1. The communication between the Host and
+ the MAC-PHY is specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
+ Interface (TC6).
+
+ Specifications about the LAN8650/1 can be found at:
+ https://www.microchip.com/en-us/product/lan8650
+
+allOf:
+ - $ref: ethernet-controller.yaml#
+
+properties:
+ compatible:
+ const: microchip,lan865x
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ Interrupt from MAC-PHY asserted in the event of Receive Chunks
+ Available, Transmit Chunk Credits Available and Extended Status
+ Event.
+ maxItems: 1
+
+ local-mac-address:
+ description:
+ Specifies the MAC address assigned to the network device.
+ $ref: /schemas/types.yaml#/definitions/uint8-array
+ minItems: 6
+ maxItems: 6
+
+ spi-max-frequency:
+ minimum: 15000000
+ maximum: 25000000
+
+ oa-tc6:
+ $ref: oa-tc6.yaml#
+ unevaluatedProperties: true
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - pinctrl-names
+ - pinctrl-0
+ - interrupts
+ - interrupt-parent
+ - local-mac-address
+ - spi-max-frequency
+ - oa-tc6
+
+additionalProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethernet@0 {
+ compatible = "microchip,lan865x";
+ reg = <0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <ð0_pins>;
+ interrupt-parent = <&gpio>;
+ interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
+ local-mac-address = [04 05 06 01 02 03];
+ spi-max-frequency = <15000000>;
+ status = "okay";
+ oa-tc6 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ oa-cps = <64>;
+ oa-txcte;
+ oa_rxcte;
+ oa-prote;
+ oa-dprac;
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1b1bd3218a2d..d2b3c0e8d97e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14005,6 +14005,7 @@ MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
M: Parthiban Veerasooran <[email protected]>
L: [email protected]
S: Maintained
+F: Documentation/devicetree/bindings/net/microchip,lan865x.yaml
F: drivers/net/ethernet/microchip/lan865x.c
MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
--
2.34.1
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 <[email protected]>
---
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: [email protected]
S: Maintained
F: drivers/net/ethernet/microchip/lan743x_*
+MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
+M: Parthiban Veerasooran <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/net/ethernet/microchip/lan865x.c
+
MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
M: Arun Ramadoss <[email protected]>
R: [email protected]
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 <[email protected]>
+ */
+
+#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, ®val);
+ 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, ®val))
+ 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, ®val))
+ 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, ®val);
+ 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, ®val);
+ 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 <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:" DRV_NAME);
--
2.34.1
On Mon, 23 Oct 2023 21:16:44 +0530, Parthiban Veerasooran wrote:
> Add DT bindings OPEN Alliance 10BASE-T1x MACPHY Serial Interface
> parameters. These are generic properties that can apply to any 10BASE-T1x
> MAC-PHY which uses OPEN Alliance TC6 specification.
>
> Signed-off-by: Parthiban Veerasooran <[email protected]>
> ---
> .../devicetree/bindings/net/oa-tc6.yaml | 72 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 73 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/net/oa-tc6.yaml:16:68: [error] syntax error: mapping values are not allowed here (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/oa-tc6.example.dts'
Documentation/devicetree/bindings/net/oa-tc6.yaml:16:68: mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/oa-tc6.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/net/oa-tc6.yaml:16:68: mapping values are not allowed in this context
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/oa-tc6.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Mon, 23 Oct 2023 21:16:49 +0530, Parthiban Veerasooran wrote:
> Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
> combines a Media Access Controller (MAC) and an Ethernet PHY to enable
> 10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
> implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
> 802.3 standard and a 10BASE-T1S physical layer transceiver integrated
> into the LAN8650/1. The communication between the Host and the MAC-PHY is
> specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
>
> Signed-off-by: Parthiban Veerasooran <[email protected]>
> ---
> .../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/net/microchip,lan865x.yaml:21:53: [error] syntax error: mapping values are not allowed here (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/microchip,lan865x.example.dts'
Documentation/devicetree/bindings/net/microchip,lan865x.yaml:21:53: mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/microchip,lan865x.example.dts] Error 1
./Documentation/devicetree/bindings/net/microchip,lan865x.yaml:21:53: mapping values are not allowed in this context
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/microchip,lan865x.yaml: ignoring, error parsing file
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
> + /* Read and configure the IMASK0 register for unmasking the interrupts */
> + ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, false, false);
> + if (ret)
> + return ret;
Can you use oa_tc6_read_register() here? I guess the question is, what
does tc6->protect default to until it is set later in this function?
So long as it defaults to false, i guess you can use the register
read/write functions, which are a lot more readable than this generic
oa_tc6_perform_ctrl().
> +
> + regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
> +
> + ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, true, false);
> + if (ret)
> + return ret;
> +
> + /* Read STDCAP register to get the MAC-PHY standard capabilities */
> + ret = oa_tc6_perform_ctrl(tc6, STDCAP, ®val, 1, false, false);
> + if (ret)
> + return ret;
> +
> + mincps = FIELD_GET(MINCPS, regval);
> + ctc = (regval & CTC) ? true : false;
> +
> + regval = 0;
> + oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
> + if (oa_node) {
> + /* Read OA parameters from DT */
> + if (of_property_present(oa_node, "oa-cps")) {
> + ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);
If of_property_read_u32() does not find the property, it is documented
to not touch tc6->cps. So you can set tc6->cps to the default 64,
before the big if, and skip the of_property_present(). You can then
probably remove the else at the end as well.
> + if (ret < 0)
> + return ret;
> + /* Return error if the configured cps is less than the
> + * minimum cps supported by the MAC-PHY.
> + */
> + if (tc6->cps < mincps)
> + return -ENODEV;
A dev_err() would be nice here to indicate why.
> + } else {
> + tc6->cps = 64;
> + }
> + if (of_property_present(oa_node, "oa-txcte")) {
> + /* Return error if the tx cut through mode is configured
> + * but it is not supported by MAC-PHY.
> + */
> + if (ctc)
> + regval |= TXCTE;
> + else
> + return -ENODEV;
and a dev_err() here as well.
> + }
> + if (of_property_present(oa_node, "oa-rxcte")) {
> + /* Return error if the rx cut through mode is configured
> + * but it is not supported by MAC-PHY.
> + */
> + if (ctc)
> + regval |= RXCTE;
> + else
> + return -ENODEV;
> + }
and another dev_err(). Without these prints, you probably need to
modify the code to figure out why the probe failed.
> + if (of_property_present(oa_node, "oa-prote")) {
> + regval |= PROTE;
> + tc6->prote = true;
> + }
> + } else {
> + tc6->cps = 64;
> + }
> +
> + regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
> +
> + return oa_tc6_perform_ctrl(tc6, CONFIG0, ®val, 1, true, false);
> +}
> +
> static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
> {
> u32 regval;
> @@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
> * Returns pointer reference to the oa_tc6 structure if all the memory
> * allocation success otherwise NULL.
> */
> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
Was there a reason to have prote initially, and then remove it here?
Andrew
> + oa-cps:
> + maxItems: 1
> + description:
> + Chunk Payload Size. Configures the data chunk payload size to 2^N,
> + where N is the value of this bitfield. The minimum possible data
> + chunk payload size is 8 bytes or N = 3. The default data chunk
> + payload size is 64 bytes, or N = 6. The minimum supported data chunk
> + payload size for this MAC-PHY device is indicated in the CPSMIN
> + field of the CAPABILITY register. Valid values for this parameter
> + are 8, 16, 32 and 64. All other values are reserved.
> +
> + oa-txcte:
> + maxItems: 1
> + description:
> + Transmit Cut-Through Enable. When supported by this MAC-PHY device,
> + this bit enables the cut-through mode of frame transfer through the
> + MAC-PHY device from the SPI host to the network.
> +
> + oa-rxcte:
> + maxItems: 1
> + description:
> + Receive Cut-Through Enable. When supported by this MAC-PHY device,
> + this bit enables the cut-through mode of frame transfer through the
> + MAC-PHY device from the network to the SPI host.
> +
> + oa-prote:
> + maxItems: 1
> + description:
> + Control data read/write Protection Enable. When set, all control
> + data written to and read from the MAC-PHY will be transferred with
> + its complement for detection of bit errors.
Device tree described hardware. Its not supposed to be used to
describe configuration. So it is not clear to me if any of these are
valid in DT.
It seems to me, the amount of control transfers should be very small
compared to data transfers. So why not just set protection enable to
be true?
What is the effect of chunk payload size ? Is there a reason to use a
lower value than the default 64? I assume smaller sizes make data
transfer more expensive, since you need more DMA setup and completion
handing etc.
An Ethernet driver is allowed to have driver specific private
flags. See ethtool(1) --show-priv-flags and --set-priv-flags You could
maybe use these to configure cut through?
Andrew
> + /* Minimum supported Chunk Payload Size */
> mincps = FIELD_GET(MINCPS, regval);
> + /* Cut-Through Capability */
> ctc = (regval & CTC) ? true : false;
These comment should be in the patch which added these, not here.
> + /* Direct PHY Register Access Capability */
> + dprac = (regval & DPRAC) ? true : false;
> + /* Indirect PHY Register access Capability */
> + iprac = (regval & IPRAC) ? true : false;
>
> regval = 0;
> oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
> @@ -242,7 +257,7 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
> if (tc6->cps < mincps)
> return -ENODEV;
> } else {
> - tc6->cps = 64;
> + tc6->cps = OA_TC6_MAX_CPS;
This also should of been in an earlier patch.
> }
> if (of_property_present(oa_node, "oa-txcte")) {
> /* Return error if the tx cut through mode is configured
> @@ -266,8 +281,26 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
> regval |= PROTE;
> tc6->prote = true;
> }
> + if (of_property_present(oa_node, "oa-dprac")) {
> + /* Return error if the direct phy register access mode
> + * is configured but it is not supported by MAC-PHY.
> + */
> + if (dprac)
> + tc6->dprac = true;
> + else
> + return -ENODEV;
> + }
This is not in the binding. Why do we even need to be able to
configure it. Direct is faster, so use it is available. If not, use
indirect. And if both dprac and iproc are false, dev_err() and
-ENODEV.
> +static int oa_tc6_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
It would be good to put direct in the name. If somebody implements
indirect, it will make the naming easier.
> +{
> + struct oa_tc6 *tc6 = bus->priv;
> + u32 regval;
> + bool ret;
> +
> + ret = oa_tc6_read_register(tc6, 0xFF00 | (idx & 0xFF), ®val);
> + if (ret)
> + return -ENODEV;
> +
> + return regval;
> +}
> +
> +static int oa_tc6_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
> + u16 val)
> +{
> + struct oa_tc6 *tc6 = bus->priv;
> +
> + return oa_tc6_write_register(tc6, 0xFF00 | (idx & 0xFF), val);
> +}
> +
> +static int oa_tc6_phy_init(struct oa_tc6 *tc6)
> +{
> + int ret;
> +
> + if (tc6->dprac) {
You can avoid the indentation by first checking indirect is the only
choice, and doing a dev_err() followed by return -ENODEV.
> + tc6->mdiobus = mdiobus_alloc();
> + if (!tc6->mdiobus) {
> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> + return -ENODEV;
> + }
> +
> + tc6->mdiobus->phy_mask = ~(u32)BIT(1);
Does the standard define this ? BIT(1), not BIT(0)?
> /**
> * oa_tc6_init - allocates and intializes oa_tc6 structure.
> * @spi: device with which data will be exchanged.
> - * @prote: control data (register) read/write protection enable/disable.
Something else which should of been in the previous patch. Please look
through this patch and find all the other instances.
> + * @netdev: network device to use.
> *
> * Returns pointer reference to the oa_tc6 structure if all the memory
> * allocation success otherwise NULL.
> */
> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
> {
> struct oa_tc6 *tc6;
>
> @@ -395,15 +521,19 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> if (!tc6)
> return NULL;
>
> + /* Allocate memory for the control tx buffer used for SPI transfer. */
> tc6->ctrl_tx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
> if (!tc6->ctrl_tx_buf)
> return NULL;
>
> + /* Allocate memory for the control rx buffer used for SPI transfer. */
> tc6->ctrl_rx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
> if (!tc6->ctrl_rx_buf)
> return NULL;
>
> tc6->spi = spi;
> + tc6->netdev = netdev;
> + SET_NETDEV_DEV(netdev, &spi->dev);
>
> /* Perform MAC-PHY software reset */
> if (oa_tc6_sw_reset(tc6)) {
> @@ -417,10 +547,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> return NULL;
> }
>
> + /* Initialize PHY */
> + if (oa_tc6_phy_init(tc6)) {
> + dev_err(&spi->dev, "PHY initialization failed\n");
> + return NULL;
> + }
> +
> return tc6;
> }
> EXPORT_SYMBOL_GPL(oa_tc6_init);
>
> +/**
> + * oa_tc6_exit - exit function.
> + * @tc6: oa_tc6 struct.
> + *
> + */
> +void oa_tc6_exit(struct oa_tc6 *tc6)
> +{
> + oa_tc6_phy_exit(tc6);
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_exit);
> +
> MODULE_DESCRIPTION("OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface Lib");
> MODULE_AUTHOR("Parthiban Veerasooran <[email protected]>");
> MODULE_LICENSE("GPL");
> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
> index 378636fd9ca8..36b729c384ac 100644
> --- a/include/linux/oa_tc6.h
> +++ b/include/linux/oa_tc6.h
> @@ -5,54 +5,59 @@
> * Author: Parthiban Veerasooran <[email protected]>
> */
>
> +#include <linux/etherdevice.h>
> #include <linux/spi/spi.h>
>
> /* Control header */
> -#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
> -#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
> -#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
> -#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
> -#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
> -#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
> -#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
> -#define CTRL_HDR_P BIT(0) /* Parity Bit */
> +#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
> +#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
> +#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
> +#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
> +#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
> +#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
> +#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
> +#define CTRL_HDR_P BIT(0) /* Parity Bit */
Please don't change the whitespace like this.
Andrew
On Mon, Oct 23, 2023 at 09:16:46PM +0530, Parthiban Veerasooran wrote:
> Direct PHY Register Access Capability indicates if PHY registers are
> directly accessible within the SPI register memory space. Indirect PHY
> Register Access Capability indicates if PHY registers are indirectly
> accessible through the MDIO/MDC registers MDIOACCn.
>
> Signed-off-by: Parthiban Veerasooran <[email protected]>
It is more normal to put all the bindings into one patch.
Again, this seems like configuration, not a description of the
hardware. Its also not clear to my why you would want to configure it.
Andrew
> +static u16 oa_tc6_prepare_empty_chunk(struct oa_tc6 *tc6, u8 *buf, u8 cp_count)
> +{
> + u32 hdr;
> +
> + /* Prepare empty chunks used for getting interrupt information or if
> + * receive data available.
> + */
> + for (u8 i = 0; i < cp_count; i++) {
> + hdr = FIELD_PREP(DATA_HDR_DNC, 1);
> + hdr |= FIELD_PREP(DATA_HDR_P, oa_tc6_get_parity(hdr));
> + *(__be32 *)&buf[i * (tc6->cps + TC6_HDR_SIZE)] = cpu_to_be32(hdr);
> + memset(&buf[TC6_HDR_SIZE + (i * (tc6->cps + TC6_HDR_SIZE))], 0,
> + tc6->cps);
> + }
This is not simple, and its the sort of code which makes me wounder if
its gone off the end of the buffer. It would be good to find somebody
internally within Microchip to review this code.
> +static void oa_tc6_rx_eth_ready(struct oa_tc6 *tc6)
> +{
> + struct sk_buff *skb;
> +
> + /* Send the received ethernet packet to network layer */
> + skb = netdev_alloc_skb(tc6->netdev, tc6->rxd_bytes + NET_IP_ALIGN);
> + if (!skb) {
> + tc6->netdev->stats.rx_dropped++;
> + netdev_dbg(tc6->netdev, "Out of memory for rx'd frame");
You can just return here, and skip the else. Less indentation is
better, it generally makes the code more readable.
> + } else {
> + skb_reserve(skb, NET_IP_ALIGN);
> + memcpy(skb_put(skb, tc6->rxd_bytes), &tc6->eth_rx_buf[0],
> + tc6->rxd_bytes);
> + skb->protocol = eth_type_trans(skb, tc6->netdev);
> + tc6->netdev->stats.rx_packets++;
> + tc6->netdev->stats.rx_bytes += tc6->rxd_bytes;
> + /* 0 for NET_RX_SUCCESS and 1 for NET_RX_DROP */
> + if (netif_rx(skb))
> + tc6->netdev->stats.rx_dropped++;
Rather than have a comment do:
if (netif_rx(skb) == NET_RX_DROP)
tc6->netdev->stats.rx_dropped++;
> +static void oa_tc6_rx_eth_complete2(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
> +{
> + u16 ebo;
What does ftr and ebo mean? Its really hard to read this code because
the names are not really meaningful.
> +
> + if (FIELD_GET(DATA_FTR_EV, ftr))
> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
> + else
> + ebo = tc6->cps;
> +
> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
> + tc6->rxd_bytes += ebo;
> + if (FIELD_GET(DATA_FTR_EV, ftr)) {
> + /* If EV set then send the received ethernet frame to n/w */
> + oa_tc6_rx_eth_ready(tc6);
> + tc6->rxd_bytes = 0;
> + tc6->rx_eth_started = false;
> + }
> +}
> +
> +static void oa_tc6_rx_eth_complete1(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
> +{
> + u16 ebo;
> + u16 sbo;
> +
> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
> +
> + if (ebo <= sbo) {
> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
> + tc6->rxd_bytes += ebo;
> + oa_tc6_rx_eth_ready(tc6);
> + tc6->rxd_bytes = 0;
> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
> + tc6->cps - sbo);
> + tc6->rxd_bytes += (tc6->cps - sbo);
> + } else {
> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
> + ebo - sbo);
> + tc6->rxd_bytes += (ebo - sbo);
> + oa_tc6_rx_eth_ready(tc6);
> + tc6->rxd_bytes = 0;
> + }
> +}
> +
> +static void oa_tc6_start_rx_eth(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
> +{
> + u16 sbo;
> +
> + tc6->rxd_bytes = 0;
> + tc6->rx_eth_started = true;
> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo], tc6->cps - sbo);
> + tc6->rxd_bytes += (tc6->cps - sbo);
> +}
> +
> +static u32 oa_tc6_get_footer(struct oa_tc6 *tc6, u8 *buf, u8 cp_num)
> +{
> + __be32 ftr;
> +
> + ftr = *(__be32 *)&buf[tc6->cps + (cp_num * (tc6->cps + TC6_FTR_SIZE))];
> +
> + return be32_to_cpu(ftr);
> +}
> +
> +static void oa_tc6_update_txc_rca(struct oa_tc6 *tc6, u32 ftr)
> +{
> + tc6->txc = FIELD_GET(DATA_FTR_TXC, ftr);
> + tc6->rca = FIELD_GET(DATA_FTR_RCA, ftr);
> +}
> +
> +static int oa_tc6_check_ftr_errors(struct oa_tc6 *tc6, u32 ftr)
> +{
> + /* Check for footer parity error */
> + if (oa_tc6_get_parity(ftr)) {
> + net_err_ratelimited("%s: Footer parity error\n",
> + tc6->netdev->name);
> + return FTR_ERR;
> + }
> + /* If EXST set in the footer then read STS0 register to get the
> + * status information.
> + */
> + if (FIELD_GET(DATA_FTR_EXST, ftr)) {
> + if (oa_tc6_process_exst(tc6))
> + net_err_ratelimited("%s: Failed to process EXST\n",
> + tc6->netdev->name);
> + return FTR_ERR;
> + }
> + if (FIELD_GET(DATA_FTR_HDRB, ftr)) {
> + net_err_ratelimited("%s: Footer eeceived header bad\n",
> + tc6->netdev->name);
> + return FTR_ERR;
> + }
> + if (!FIELD_GET(DATA_FTR_SYNC, ftr)) {
> + net_err_ratelimited("%s: Footer configuration unsync\n",
> + tc6->netdev->name);
> + return FTR_ERR;
> + }
> + return FTR_OK;
> +}
> +
> +static void oa_tc6_drop_rx_eth(struct oa_tc6 *tc6)
> +{
> + tc6->rxd_bytes = 0;
> + tc6->rx_eth_started = false;
> + tc6->netdev->stats.rx_dropped++;
> + net_err_ratelimited("%s: Footer frame drop\n",
> + tc6->netdev->name);
> +}
> +
> +static int oa_tc6_process_rx_chunks(struct oa_tc6 *tc6, u8 *buf, u16 len)
> +{
> + u8 cp_count;
> + u8 *payload;
> + u32 ftr;
> + int ret;
> +
> + /* Calculate the number of chunks received */
> + cp_count = len / (tc6->cps + TC6_FTR_SIZE);
> +
> + for (u8 i = 0; i < cp_count; i++) {
> + /* Get the footer and payload */
> + ftr = oa_tc6_get_footer(tc6, buf, i);
> + payload = &buf[(i * (tc6->cps + TC6_FTR_SIZE))];
This would be more readable:
/* Calculate the number of chunks received */
chunks = len / (tc6->cps + TC6_FTR_SIZE);
for (u8 chunk = 0; chunk < chunks; chunk++) {
/* Get the footer and payload */
ftr = oa_tc6_get_footer(tc6, buf, chunk);
payload = &buf[(chunk * (tc6->cps + TC6_FTR_SIZE))];
etc.
And maybe move most of this code into a function
oa_tc6_process_rx_chunk(). With lots of small functions with good
names, you need less comments.
> + /* Check for data valid */
> + if (FIELD_GET(DATA_FTR_DV, ftr)) {
> + /* Check whether both start valid and end valid are in a
> + * single chunk payload means a single chunk payload may
> + * contain an entire ethernet frame.
> + */
> + if (FIELD_GET(DATA_FTR_SV, ftr) &&
> + FIELD_GET(DATA_FTR_EV, ftr)) {
if (FIELD_GET(DATA_FOOTER_START_VALID, footer) &&
FIELD_GET(DATA_FOOTER_END_VALID, footer)) {
Don't you think that is more readable?
> +static void oa_tc6_prepare_tx_chunks(struct oa_tc6 *tc6, u8 *buf,
> + struct sk_buff *skb)
> +{
> + bool frame_started = false;
> + u16 copied_bytes = 0;
> + u16 copy_len;
> + u32 hdr;
> +
> + /* Calculate the number tx credit counts needed to transport the tx
> + * ethernet frame.
> + */
> + tc6->txc_needed = (skb->len / tc6->cps) + ((skb->len % tc6->cps) ? 1 : 0);
Why call it a credit here, but a chunk when receiving?
> +static int oa_tc6_perform_spi_xfer(struct oa_tc6 *tc6)
> +{
> + bool do_tx_again;
> + u16 total_len;
> + u16 rca_len;
> + u16 tx_len;
> + int ret;
> +
> + do {
> + do_tx_again = false;
> + rca_len = 0;
> + tx_len = 0;
> +
> + /* In case of an interrupt, perform an empty chunk transfer to
> + * know the purpose of the interrupt. Interrupt may occur in
> + * case of RCA (Receive Chunk Available) and TXC (Transmit
> + * Credit Count). Both will occur if they are not indicated
> + * through the previous footer.
> + */
> + if (tc6->int_flag) {
> + tc6->int_flag = false;
> + total_len = oa_tc6_prepare_empty_chunk(tc6,
> + tc6->spi_tx_buf,
> + 1);
> + } else {
> + /* Calculate the transfer length */
> + if (tc6->tx_flag && tc6->txc) {
> + tx_len = oa_tc6_calculate_tx_len(tc6);
> + memcpy(&tc6->spi_tx_buf[0],
> + &tc6->eth_tx_buf[tc6->tx_pos], tx_len);
> + }
> +
> + if (tc6->rca)
> + rca_len = oa_tc6_calculate_rca_len(tc6, tx_len);
> +
> + total_len = tx_len + rca_len;
> + }
> + ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf,
> + tc6->spi_rx_buf, total_len);
> + if (ret)
> + return ret;
> + /* Process the rxd chunks to get the ethernet frame or status */
> + ret = oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, total_len);
> + if (ret)
> + return ret;
> + if (tc6->tx_flag) {
> + tc6->tx_pos += tx_len;
> + tc6->txc_needed = tc6->txc_needed -
> + (tx_len / (tc6->cps + TC6_HDR_SIZE));
> + /* If the complete ethernet frame is transmitted then
> + * return the skb and update the details to n/w layer.
> + */
> + if (!tc6->txc_needed)
> + oa_tc6_tx_eth_complete(tc6);
> + else if (tc6->txc)
> + /* If txc is available again and updated from
> + * the previous footer then perform tx again.
> + */
> + do_tx_again = true;
> + }
> +
> + /* If rca is updated from the previous footer then perform empty
> + * tx to receive ethernet frame.
> + */
> + if (tc6->rca)
> + do_tx_again = true;
> + } while (do_tx_again);
The coding standard say:
Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen size
is 80x24, as we all know), and do one thing and do that well.
This is too long, and does too many things.
Andrew
> +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, ®val);
> + 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
On 23/10/2023 17:46, Parthiban Veerasooran wrote:
> Add DT bindings OPEN Alliance 10BASE-T1x MACPHY Serial Interface
> parameters. These are generic properties that can apply to any 10BASE-T1x
> MAC-PHY which uses OPEN Alliance TC6 specification.
Except that it was not tested at all few more issues.
>
> Signed-off-by: Parthiban Veerasooran <[email protected]>
> ---
> .../devicetree/bindings/net/oa-tc6.yaml | 72 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 73 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/oa-tc6.yaml b/Documentation/devicetree/bindings/net/oa-tc6.yaml
> new file mode 100644
> index 000000000000..9f442fa6cace
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/oa-tc6.yaml
Filename based on compatible.
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/oa-tc6.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OPEN Alliance 10BASE-T1x MAC-PHY Specification Common Properties
> +
> +maintainers:
> + - Parthiban Veerasooran <[email protected]>
> +
> +description:
> + These are generic properties that can apply to any 10BASE-T1x MAC-PHY
> + which uses OPEN Alliance TC6 specification.
> +
> + 10BASE-T1x MAC-PHY Serial Interface Specification can be found at:
> + https://opensig.org/about/specifications/
> +
> +properties:
> + $nodename:
> + pattern: "^oa-tc6(@.*)?"
Drop
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
Why?
> +
> + oa-cps:
> + maxItems: 1
> + description:
> + Chunk Payload Size. Configures the data chunk payload size to 2^N,
> + where N is the value of this bitfield. The minimum possible data
> + chunk payload size is 8 bytes or N = 3. The default data chunk
> + payload size is 64 bytes, or N = 6. The minimum supported data chunk
> + payload size for this MAC-PHY device is indicated in the CPSMIN
> + field of the CAPABILITY register. Valid values for this parameter
> + are 8, 16, 32 and 64. All other values are reserved.
> +
> + oa-txcte:
> + maxItems: 1
> + description:
> + Transmit Cut-Through Enable. When supported by this MAC-PHY device,
> + this bit enables the cut-through mode of frame transfer through the
> + MAC-PHY device from the SPI host to the network.
> +
> + oa-rxcte:
> + maxItems: 1
> + description:
> + Receive Cut-Through Enable. When supported by this MAC-PHY device,
> + this bit enables the cut-through mode of frame transfer through the
> + MAC-PHY device from the network to the SPI host.
> +
> + oa-prote:
> + maxItems: 1
> + description:
> + Control data read/write Protection Enable. When set, all control
> + data written to and read from the MAC-PHY will be transferred with
> + its complement for detection of bit errors.
> +
> +additionalProperties: true
> +
> +examples:
> + - |
> + oa-tc6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
That's some total mess in indentation.
> + oa-cps = <64>;
> + oa-txcte;
> + oa-rxcte;
> + oa-prote;
> + };
Best regards,
Krzysztof
On 23/10/2023 17:46, Parthiban Veerasooran wrote:
> Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
> combines a Media Access Controller (MAC) and an Ethernet PHY to enable
> 10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
> implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
> 802.3 standard and a 10BASE-T1S physical layer transceiver integrated
> into the LAN8650/1. The communication between the Host and the MAC-PHY is
> specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.
>
> Signed-off-by: Parthiban Veerasooran <[email protected]>
> ---
> .../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
> new file mode 100644
> index 000000000000..974622dd6846
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
> +
> +maintainers:
> + - Parthiban Veerasooran <[email protected]>
> +
> +description:
> + The LAN8650/1 combines a Media Access Controller (MAC) and an Ethernet
> + PHY to enable 10BASE‑T1S networks. The Ethernet Media Access Controller
> + (MAC) module implements a 10 Mbps half duplex Ethernet MAC, compatible
> + with the IEEE 802.3 standard and a 10BASE-T1S physical layer transceiver
> + integrated into the LAN8650/1. The communication between the Host and
> + the MAC-PHY is specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
> + Interface (TC6).
> +
> + Specifications about the LAN8650/1 can be found at:
> + https://www.microchip.com/en-us/product/lan8650
> +
> +allOf:
> + - $ref: ethernet-controller.yaml#
> +
> +properties:
> + compatible:
> + const: microchip,lan865x
No wildcards in compatibles.
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description:
> + Interrupt from MAC-PHY asserted in the event of Receive Chunks
> + Available, Transmit Chunk Credits Available and Extended Status
> + Event.
> + maxItems: 1
> +
> + local-mac-address:
Drop property, not needed.
> + description:
> + Specifies the MAC address assigned to the network device.
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> + minItems: 6
> + maxItems: 6
> +
> + spi-max-frequency:
> + minimum: 15000000
> + maximum: 25000000
> +
> + oa-tc6:
> + $ref: oa-tc6.yaml#
> + unevaluatedProperties: true
This must be false.
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> + - pinctrl-names
> + - pinctrl-0
> + - interrupts
> + - interrupt-parent
> + - local-mac-address
> + - spi-max-frequency
> + - oa-tc6
> +
> +additionalProperties: false
Instead:
unevaluatedProperties: false
> +
> +examples:
> + - |
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethernet@0 {
> + compatible = "microchip,lan865x";
> + reg = <0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <ð0_pins>;
> + interrupt-parent = <&gpio>;
> + interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
> + local-mac-address = [04 05 06 01 02 03];
> + spi-max-frequency = <15000000>;
> + status = "okay";
Drop status.
> + oa-tc6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + oa-cps = <64>;
> + oa-txcte;
> + oa_rxcte;
> + oa-prote;
> + oa-dprac;
Again totally mixed up indentation.
> + };
> + };
> + };
Best regards,
Krzysztof
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 <[email protected]>
> ---
> 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: [email protected]
> S: Maintained
> F: drivers/net/ethernet/microchip/lan743x_*
>
> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
> +M: Parthiban Veerasooran <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/net/ethernet/microchip/lan865x.c
> +
> MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
> M: Arun Ramadoss <[email protected]>
> R: [email protected]
> 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 <[email protected]>");
> +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
Hi Andrew,
On 24/10/23 4:28 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> + /* Read and configure the IMASK0 register for unmasking the interrupts */
>> + ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, false, false);
>> + if (ret)
>> + return ret;
>
> Can you use oa_tc6_read_register() here? I guess the question is, what
> does tc6->protect default to until it is set later in this function?
> So long as it defaults to false, i guess you can use the register
> read/write functions, which are a lot more readable than this generic
> oa_tc6_perform_ctrl().
Yes, I will do that. Also for next two calls as well.
>
>> +
>> + regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
>> +
>> + ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, true, false);
>> + if (ret)
>> + return ret;
>> +
>> + /* Read STDCAP register to get the MAC-PHY standard capabilities */
>> + ret = oa_tc6_perform_ctrl(tc6, STDCAP, ®val, 1, false, false);
>> + if (ret)
>> + return ret;
>> +
>> + mincps = FIELD_GET(MINCPS, regval);
>> + ctc = (regval & CTC) ? true : false;
>> +
>> + regval = 0;
>> + oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
>> + if (oa_node) {
>> + /* Read OA parameters from DT */
>> + if (of_property_present(oa_node, "oa-cps")) {
>> + ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);
>
> If of_property_read_u32() does not find the property, it is documented
> to not touch tc6->cps. So you can set tc6->cps to the default 64,
> before the big if, and skip the of_property_present(). You can then
> probably remove the else at the end as well.
Ah ok, will do that.
>
>> + if (ret < 0)
>> + return ret;
>> + /* Return error if the configured cps is less than the
>> + * minimum cps supported by the MAC-PHY.
>> + */
>> + if (tc6->cps < mincps)
>> + return -ENODEV;
>
> A dev_err() would be nice here to indicate why.
Ok sure.
>
>> + } else {
>> + tc6->cps = 64;
>> + }
>> + if (of_property_present(oa_node, "oa-txcte")) {
>> + /* Return error if the tx cut through mode is configured
>> + * but it is not supported by MAC-PHY.
>> + */
>> + if (ctc)
>> + regval |= TXCTE;
>> + else
>> + return -ENODEV;
>
> and a dev_err() here as well.
Ok sure.
>
>> + }
>> + if (of_property_present(oa_node, "oa-rxcte")) {
>> + /* Return error if the rx cut through mode is configured
>> + * but it is not supported by MAC-PHY.
>> + */
>> + if (ctc)
>> + regval |= RXCTE;
>> + else
>> + return -ENODEV;
>> + }
>
> and another dev_err(). Without these prints, you probably need to
> modify the code to figure out why the probe failed.
Yes I understand. Will do that in the next revision.
>
>> + if (of_property_present(oa_node, "oa-prote")) {
>> + regval |= PROTE;
>> + tc6->prote = true;
>> + }
>> + } else {
>> + tc6->cps = 64;
>> + }
>> +
>> + regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
>> +
>> + return oa_tc6_perform_ctrl(tc6, CONFIG0, ®val, 1, true, false);
>> +}
>> +
>> static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
>> {
>> u32 regval;
>> @@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
>> * Returns pointer reference to the oa_tc6 structure if all the memory
>> * allocation success otherwise NULL.
>> */
>> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>
> Was there a reason to have prote initially, and then remove it here?
The reason is, control communication uses "protect". But in the first
patch there was no dt used. Later in this patch, dt used for all the
configuration parameters and this also part of that. That's why removed
and moved this to dt configuration.
What's your opinion? shall I keep as it is like this? or remove the
protect in the first two patches and introduce in this patch?
Best Regards,
Parthiban V
>
> Andrew
On 23/10/23 11:10 pm, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, 23 Oct 2023 21:16:44 +0530, Parthiban Veerasooran wrote:
>> Add DT bindings OPEN Alliance 10BASE-T1x MACPHY Serial Interface
>> parameters. These are generic properties that can apply to any 10BASE-T1x
>> MAC-PHY which uses OPEN Alliance TC6 specification.
>>
>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>> ---
>> .../devicetree/bindings/net/oa-tc6.yaml | 72 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 73 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
>>
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/net/oa-tc6.yaml:16:68: [error] syntax error: mapping values are not allowed here (syntax)
>
> dtschema/dtc warnings/errors:
> make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/oa-tc6.example.dts'
> Documentation/devicetree/bindings/net/oa-tc6.yaml:16:68: mapping values are not allowed in this context
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/oa-tc6.example.dts] Error 1
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/net/oa-tc6.yaml:16:68: mapping values are not allowed in this context
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/oa-tc6.yaml: ignoring, error parsing file
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
Sorry, somehow I missed doing this check. Will fix this in the next
revision.
Best Regards,
Parthiban V
> >> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
> >> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> >
> > Was there a reason to have prote initially, and then remove it here?
> The reason is, control communication uses "protect". But in the first
> patch there was no dt used. Later in this patch, dt used for all the
> configuration parameters and this also part of that. That's why removed
> and moved this to dt configuration.
>
> What's your opinion? shall I keep as it is like this? or remove the
> protect in the first two patches and introduce in this patch?
It will actually depend on what goes into the DT binding. If using
protections costs very little, i would just hard code it on. Maybe you
can run some iperf tests and see if it makes a measurable difference.
How fast an SPI bus are you using on your development board? If you
have a 50Mbps SPI bus, it does not even matter, since the media
bandwidth is just 10Mbps.
Andrew
Hi Andrew,
On 27/10/23 1:36 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
>>>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>>>
>>> Was there a reason to have prote initially, and then remove it here?
>> The reason is, control communication uses "protect". But in the first
>> patch there was no dt used. Later in this patch, dt used for all the
>> configuration parameters and this also part of that. That's why removed
>> and moved this to dt configuration.
>>
>> What's your opinion? shall I keep as it is like this? or remove the
>> protect in the first two patches and introduce in this patch?
>
> It will actually depend on what goes into the DT binding. If using
> protections costs very little, i would just hard code it on. Maybe you
> can run some iperf tests and see if it makes a measurable difference.
>
> How fast an SPI bus are you using on your development board? If you
> have a 50Mbps SPI bus, it does not even matter, since the media
> bandwidth is just 10Mbps.
Actually protection is only used for control communication to read/write
registers. It is not used in data communication where ethernet frame
transfer performed. So it doesn't hurt data traffic. But of course in
between data communication I may perform some control transfer for
register read/write but they are not big and will not affect the speed.
In my development board I use 15MHz speed SPI bus.
As this is given as a configurable parameter in the OPEN Alliance
specification, I have implemented it in the DT binding for user
input/choice.
Best Regards,
Parthiban V
>
> Andrew
Hi Andrew,
On 24/10/23 6:07 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> + oa-cps:
>> + maxItems: 1
>> + description:
>> + Chunk Payload Size. Configures the data chunk payload size to 2^N,
>> + where N is the value of this bitfield. The minimum possible data
>> + chunk payload size is 8 bytes or N = 3. The default data chunk
>> + payload size is 64 bytes, or N = 6. The minimum supported data chunk
>> + payload size for this MAC-PHY device is indicated in the CPSMIN
>> + field of the CAPABILITY register. Valid values for this parameter
>> + are 8, 16, 32 and 64. All other values are reserved.
>> +
>> + oa-txcte:
>> + maxItems: 1
>> + description:
>> + Transmit Cut-Through Enable. When supported by this MAC-PHY device,
>> + this bit enables the cut-through mode of frame transfer through the
>> + MAC-PHY device from the SPI host to the network.
>> +
>> + oa-rxcte:
>> + maxItems: 1
>> + description:
>> + Receive Cut-Through Enable. When supported by this MAC-PHY device,
>> + this bit enables the cut-through mode of frame transfer through the
>> + MAC-PHY device from the network to the SPI host.
>> +
>> + oa-prote:
>> + maxItems: 1
>> + description:
>> + Control data read/write Protection Enable. When set, all control
>> + data written to and read from the MAC-PHY will be transferred with
>> + its complement for detection of bit errors.
>
> Device tree described hardware. Its not supposed to be used to
> describe configuration. So it is not clear to me if any of these are
> valid in DT.
>
> It seems to me, the amount of control transfers should be very small
> compared to data transfers. So why not just set protection enable to
> be true?
Yes having protection enabled for control transfer doesn't hurt
anything. The only intention for keeping this as configurable is, it is
defined in the OPEN Alliance specification to enable/disable.
>
> What is the effect of chunk payload size ? Is there a reason to use a
> lower value than the default 64? I assume smaller sizes make data
> transfer more expensive, since you need more DMA setup and completion
> handing etc.
Again the intention for keeping this as configurable is, it is defined
in the OPEN Alliance specification as user configurable. They can be 8,
16, 32 and 64. And the default is 64. Also Microchip's LAN8650 supports
for 32 and 64.
>
> An Ethernet driver is allowed to have driver specific private
> flags. See ethtool(1) --show-priv-flags and --set-priv-flags You could
> maybe use these to configure cut through?
So you mean, we have to implement the support in the ethtool interface
to enable/disable tx/rx cut through feature, isn't it?
If you feel like the above configurations are not needed, so by keeping
protection true always, chunk payload size (cps) 64 always and moving
tx/rx cut through to ethtool, we can get rid of this DT bindings?
Best Regards,
Parthiban V
>
> Andrew
>
>
>
> > Device tree described hardware. Its not supposed to be used to
> > describe configuration. So it is not clear to me if any of these are
> > valid in DT.
> >
> > It seems to me, the amount of control transfers should be very small
> > compared to data transfers. So why not just set protection enable to
> > be true?
> Yes having protection enabled for control transfer doesn't hurt
> anything. The only intention for keeping this as configurable is, it is
> defined in the OPEN Alliance specification to enable/disable.
Standards often have options which nobody ever use, or are only useful
in particular niches. Its often best to keep it simple, get the basic
feature working, and then add these optional features if anybody
actually needs them.
> > What is the effect of chunk payload size ? Is there a reason to use a
> > lower value than the default 64? I assume smaller sizes make data
> > transfer more expensive, since you need more DMA setup and completion
> > handing etc.
> Again the intention for keeping this as configurable is, it is defined
> in the OPEN Alliance specification as user configurable. They can be 8,
> 16, 32 and 64. And the default is 64. Also Microchip's LAN8650 supports
> for 32 and 64.
Do you have any idea why the standard has different sizes? Why would
you want to use 32? If you can answer this, it helps decide how
important it is to support multiple sizes, or just hard code it to 64.
There are plenty of old research on Ethernet frame sizes, but they are
for LAN/Internet usage. You typically see two peeks, one around 64-80
bytes, and other around the full frame size. The small packets are TCP
ACKS, and the rest is TCP data. However, this is a T1S device for
automotive. I personally have no idea if the same traffic distribution
is seen in that application?
Are there protocols running which use a lot of frames smaller than 64
bytes? If so, 64 byte chunk size i assume could be wasteful, if there
are lots of 32 byte frames.
The other potential issue is latency and the way the SPI bus
works. Its a synchronised bi-directional bus. You can receive and
transmit at the same time, but you have to setup the transfer to do
that. If you are busy doing a receive only, and there is a new packet
to send, you have to wait for the chunk transfer to complete before
you can start a bi-directional chunk transfer. So a 32 byte chunk
might make your link more efficient if you have heavy but bursty
traffic. However, you have to consider the overheads of setting up the
transfer and running the completion handler afterwards. This can be
costly.
Do you have real use cases for using different chunks sizes? If not, i
probably would just hard code it to 64, until somebody comes along
needing something else.
> > An Ethernet driver is allowed to have driver specific private
> > flags. See ethtool(1) --show-priv-flags and --set-priv-flags You could
> > maybe use these to configure cut through?
> So you mean, we have to implement the support in the ethtool interface
> to enable/disable tx/rx cut through feature, isn't it?
>
> If you feel like the above configurations are not needed, so by keeping
> protection true always, chunk payload size (cps) 64 always and moving
> tx/rx cut through to ethtool, we can get rid of this DT bindings?
Again, do you have a real use case for cut through? Or maybe flip it
around, Why would you not use cut through?
Andrew
Hi Andrew,
On 27/10/23 6:02 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>> Device tree described hardware. Its not supposed to be used to
>>> describe configuration. So it is not clear to me if any of these are
>>> valid in DT.
>>>
>>> It seems to me, the amount of control transfers should be very small
>>> compared to data transfers. So why not just set protection enable to
>>> be true?
>> Yes having protection enabled for control transfer doesn't hurt
>> anything. The only intention for keeping this as configurable is, it is
>> defined in the OPEN Alliance specification to enable/disable.
>
> Standards often have options which nobody ever use, or are only useful
> in particular niches. Its often best to keep it simple, get the basic
> feature working, and then add these optional features if anybody
> actually needs them.
>
>>> What is the effect of chunk payload size ? Is there a reason to use a
>>> lower value than the default 64? I assume smaller sizes make data
>>> transfer more expensive, since you need more DMA setup and completion
>>> handing etc.
>> Again the intention for keeping this as configurable is, it is defined
>> in the OPEN Alliance specification as user configurable. They can be 8,
>> 16, 32 and 64. And the default is 64. Also Microchip's LAN8650 supports
>> for 32 and 64.
>
> Do you have any idea why the standard has different sizes? Why would
> you want to use 32? If you can answer this, it helps decide how
> important it is to support multiple sizes, or just hard code it to 64.
>
> There are plenty of old research on Ethernet frame sizes, but they are
> for LAN/Internet usage. You typically see two peeks, one around 64-80
> bytes, and other around the full frame size. The small packets are TCP
> ACKS, and the rest is TCP data. However, this is a T1S device for
> automotive. I personally have no idea if the same traffic distribution
> is seen in that application?
>
> Are there protocols running which use a lot of frames smaller than 64
> bytes? If so, 64 byte chunk size i assume could be wasteful, if there
> are lots of 32 byte frames.
>
> The other potential issue is latency and the way the SPI bus
> works. Its a synchronised bi-directional bus. You can receive and
> transmit at the same time, but you have to setup the transfer to do
> that. If you are busy doing a receive only, and there is a new packet
> to send, you have to wait for the chunk transfer to complete before
> you can start a bi-directional chunk transfer. So a 32 byte chunk
> might make your link more efficient if you have heavy but bursty
> traffic. However, you have to consider the overheads of setting up the
> transfer and running the completion handler afterwards. This can be
> costly.
>
> Do you have real use cases for using different chunks sizes? If not, i
> probably would just hard code it to 64, until somebody comes along
> needing something else.
>
>>> An Ethernet driver is allowed to have driver specific private
>>> flags. See ethtool(1) --show-priv-flags and --set-priv-flags You could
>>> maybe use these to configure cut through?
>> So you mean, we have to implement the support in the ethtool interface
>> to enable/disable tx/rx cut through feature, isn't it?
>>
>> If you feel like the above configurations are not needed, so by keeping
>> protection true always, chunk payload size (cps) 64 always and moving
>> tx/rx cut through to ethtool, we can get rid of this DT bindings?
>
> Again, do you have a real use case for cut through? Or maybe flip it
> around, Why would you not use cut through?
Thanks a lot for your detailed explanation. From this what I understand is,
1. Will make protection enabled always for control transfer.
2. Keep the default chunk payload size (64) as it is.
3. Default tx/rx cut through modes are disabled. So will not touch them.
So that we don't need DT bindings.
Best Regards,
Parthiban V
>
> Andrew
Hi,
On 23/10/23 11:10 pm, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, 23 Oct 2023 21:16:49 +0530, Parthiban Veerasooran wrote:
>> Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
>> combines a Media Access Controller (MAC) and an Ethernet PHY to enable
>> 10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
>> implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
>> 802.3 standard and a 10BASE-T1S physical layer transceiver integrated
>> into the LAN8650/1. The communication between the Host and the MAC-PHY is
>> specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
>>
>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>> ---
>> .../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 102 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/net/microchip,lan865x.yaml:21:53: [error] syntax error: mapping values are not allowed here (syntax)
>
> dtschema/dtc warnings/errors:
> make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/microchip,lan865x.example.dts'
> Documentation/devicetree/bindings/net/microchip,lan865x.yaml:21:53: mapping values are not allowed in this context
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/microchip,lan865x.example.dts] Error 1
> ./Documentation/devicetree/bindings/net/microchip,lan865x.yaml:21:53: mapping values are not allowed in this context
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/microchip,lan865x.yaml: ignoring, error parsing file
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
Sorry, somehow I missed doing this check. Will fix this in the next
revision.
Best Regards,
Parthiban V
>
>
Hi Krzysztof,
On 24/10/23 1:33 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:
>> Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
>> combines a Media Access Controller (MAC) and an Ethernet PHY to enable
>> 10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
>> implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
>> 802.3 standard and a 10BASE-T1S physical layer transceiver integrated
>> into the LAN8650/1. The communication between the Host and the MAC-PHY is
>> specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
Sorry, somehow I missed doing this check. Will fix this in the next
revision.
>
>>
>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>> ---
>> .../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 102 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>> new file mode 100644
>> index 000000000000..974622dd6846
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>> @@ -0,0 +1,101 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>> +
>> +maintainers:
>> + - Parthiban Veerasooran <[email protected]>
>> +
>> +description:
>> + The LAN8650/1 combines a Media Access Controller (MAC) and an Ethernet
>> + PHY to enable 10BASE‑T1S networks. The Ethernet Media Access Controller
>> + (MAC) module implements a 10 Mbps half duplex Ethernet MAC, compatible
>> + with the IEEE 802.3 standard and a 10BASE-T1S physical layer transceiver
>> + integrated into the LAN8650/1. The communication between the Host and
>> + the MAC-PHY is specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
>> + Interface (TC6).
>> +
>> + Specifications about the LAN8650/1 can be found at:
>> + https://www.microchip.com/en-us/product/lan8650
>> +
>> +allOf:
>> + - $ref: ethernet-controller.yaml#
>> +
>> +properties:
>> + compatible:
>> + const: microchip,lan865x
>
> No wildcards in compatibles.
Ah ok missed it. So it will become like below isn't it?
properties:
compatible:
enum:
- microchip,lan8650
- microchip,lan8651
>
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + description:
>> + Interrupt from MAC-PHY asserted in the event of Receive Chunks
>> + Available, Transmit Chunk Credits Available and Extended Status
>> + Event.
>> + maxItems: 1
>> +
>> + local-mac-address:
>
> Drop property, not needed.
Ok noted.
>
>> + description:
>> + Specifies the MAC address assigned to the network device.
>> + $ref: /schemas/types.yaml#/definitions/uint8-array
>> + minItems: 6
>> + maxItems: 6
>> +
>> + spi-max-frequency:
>> + minimum: 15000000
>> + maximum: 25000000
>> +
>> + oa-tc6:
>> + $ref: oa-tc6.yaml#
>> + unevaluatedProperties: true
>
> This must be false.
Ok noted.
>
>> +
>> + "#address-cells":
>> + const: 1
>> +
>> + "#size-cells":
>> + const: 0
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - pinctrl-names
>> + - pinctrl-0
>> + - interrupts
>> + - interrupt-parent
>> + - local-mac-address
>> + - spi-max-frequency
>> + - oa-tc6
>> +
>> +additionalProperties: false
>
> Instead:
> unevaluatedProperties: false
Ok noted.
>
>> +
>> +examples:
>> + - |
>> + spi {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + ethernet@0 {
>> + compatible = "microchip,lan865x";
>> + reg = <0>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <ð0_pins>;
>> + interrupt-parent = <&gpio>;
>> + interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
>> + local-mac-address = [04 05 06 01 02 03];
>> + spi-max-frequency = <15000000>;
>> + status = "okay";
>
> Drop status.
Ok noted.
>
>> + oa-tc6 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + oa-cps = <64>;
>> + oa-txcte;
>> + oa_rxcte;
>> + oa-prote;
>> + oa-dprac;
>
> Again totally mixed up indentation.
Sorry, probably in the next revision they will not be anymore.
Best Regards,
Parthiban V
>
>> + };
>> + };
>> + };
>
> Best regards,
> Krzysztof
>
>
On 30/10/2023 14:16, [email protected] wrote:
> Hi Krzysztof,
>
> On 24/10/23 1:33 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:
>>> Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
>>> combines a Media Access Controller (MAC) and an Ethernet PHY to enable
>>> 10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
>>> implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
>>> 802.3 standard and a 10BASE-T1S physical layer transceiver integrated
>>> into the LAN8650/1. The communication between the Host and the MAC-PHY is
>>> specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
>>
>> It does not look like you tested the bindings, at least after quick
>> look. Please run `make dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>> Maybe you need to update your dtschema and yamllint.
> Sorry, somehow I missed doing this check. Will fix this in the next
> revision.
Please also build your driver.
>>
>>>
>>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>>> ---
>>> .../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 2 files changed, 102 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>> new file mode 100644
>>> index 000000000000..974622dd6846
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>> @@ -0,0 +1,101 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>>> +
>>> +maintainers:
>>> + - Parthiban Veerasooran <[email protected]>
>>> +
>>> +description:
>>> + The LAN8650/1 combines a Media Access Controller (MAC) and an Ethernet
>>> + PHY to enable 10BASE‑T1S networks. The Ethernet Media Access Controller
>>> + (MAC) module implements a 10 Mbps half duplex Ethernet MAC, compatible
>>> + with the IEEE 802.3 standard and a 10BASE-T1S physical layer transceiver
>>> + integrated into the LAN8650/1. The communication between the Host and
>>> + the MAC-PHY is specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
>>> + Interface (TC6).
>>> +
>>> + Specifications about the LAN8650/1 can be found at:
>>> + https://www.microchip.com/en-us/product/lan8650
>>> +
>>> +allOf:
>>> + - $ref: ethernet-controller.yaml#
>>> +
>>> +properties:
>>> + compatible:
>>> + const: microchip,lan865x
>>
>> No wildcards in compatibles.
> Ah ok missed it. So it will become like below isn't it?
>
> properties:
> compatible:
> enum:
> - microchip,lan8650
> - microchip,lan8651
Rather enum for lan8650 and items for lan8651 with fallback. Aren't they
compatible, since you wanted to use wildcard?
Best regards,
Krzysztof
Hi Andrew,
On 24/10/23 6:26 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> + /* Minimum supported Chunk Payload Size */
>> mincps = FIELD_GET(MINCPS, regval);
>> + /* Cut-Through Capability */
>> ctc = (regval & CTC) ? true : false;
>
> These comment should be in the patch which added these, not here.
Ah yes. Will correct it.
>
>> + /* Direct PHY Register Access Capability */
>> + dprac = (regval & DPRAC) ? true : false;
>> + /* Indirect PHY Register access Capability */
>> + iprac = (regval & IPRAC) ? true : false;
>>
>> regval = 0;
>> oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
>> @@ -242,7 +257,7 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
>> if (tc6->cps < mincps)
>> return -ENODEV;
>> } else {
>> - tc6->cps = 64;
>> + tc6->cps = OA_TC6_MAX_CPS;
>
> This also should of been in an earlier patch.
Yes, will correct it.
>
>> }
>> if (of_property_present(oa_node, "oa-txcte")) {
>> /* Return error if the tx cut through mode is configured
>> @@ -266,8 +281,26 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
>> regval |= PROTE;
>> tc6->prote = true;
>> }
>> + if (of_property_present(oa_node, "oa-dprac")) {
>> + /* Return error if the direct phy register access mode
>> + * is configured but it is not supported by MAC-PHY.
>> + */
>> + if (dprac)
>> + tc6->dprac = true;
>> + else
>> + return -ENODEV;
>> + }
>
> This is not in the binding. Why do we even need to be able to
> configure it. Direct is faster, so use it is available. If not, use
> indirect. And if both dprac and iproc are false, dev_err() and
> -ENODEV.
Ok, I will remove this option even in the next patch and will go with
the option read from the capability register (STDCAP).
>
>> +static int oa_tc6_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
>
> It would be good to put direct in the name. If somebody implements
> indirect, it will make the naming easier.
Yes sure.
>
>> +{
>> + struct oa_tc6 *tc6 = bus->priv;
>> + u32 regval;
>> + bool ret;
>> +
>> + ret = oa_tc6_read_register(tc6, 0xFF00 | (idx & 0xFF), ®val);
>> + if (ret)
>> + return -ENODEV;
>> +
>> + return regval;
>> +}
>> +
>> +static int oa_tc6_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
>> + u16 val)
>> +{
>> + struct oa_tc6 *tc6 = bus->priv;
>> +
>> + return oa_tc6_write_register(tc6, 0xFF00 | (idx & 0xFF), val);
>> +}
>> +
>> +static int oa_tc6_phy_init(struct oa_tc6 *tc6)
>> +{
>> + int ret;
>> +
>> + if (tc6->dprac) {
>
> You can avoid the indentation by first checking indirect is the only
> choice, and doing a dev_err() followed by return -ENODEV.
Ah ok, will do it.
>
>> + tc6->mdiobus = mdiobus_alloc();
>> + if (!tc6->mdiobus) {
>> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>> + return -ENODEV;
>> + }
>> +
>> + tc6->mdiobus->phy_mask = ~(u32)BIT(1);
>
> Does the standard define this ? BIT(1), not BIT(0)?
Ok, I think here is a typo. Will correct it.
>
>> /**
>> * oa_tc6_init - allocates and intializes oa_tc6 structure.
>> * @spi: device with which data will be exchanged.
>> - * @prote: control data (register) read/write protection enable/disable.
>
> Something else which should of been in the previous patch. Please look
> through this patch and find all the other instances.
Yes sure. Will correct it.
>
>> + * @netdev: network device to use.
>> *
>> * Returns pointer reference to the oa_tc6 structure if all the memory
>> * allocation success otherwise NULL.
>> */
>> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
>> {
>> struct oa_tc6 *tc6;
>>
>> @@ -395,15 +521,19 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>> if (!tc6)
>> return NULL;
>>
>> + /* Allocate memory for the control tx buffer used for SPI transfer. */
>> tc6->ctrl_tx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
>> if (!tc6->ctrl_tx_buf)
>> return NULL;
>>
>> + /* Allocate memory for the control rx buffer used for SPI transfer. */
>> tc6->ctrl_rx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
>> if (!tc6->ctrl_rx_buf)
>> return NULL;
>>
>> tc6->spi = spi;
>> + tc6->netdev = netdev;
>> + SET_NETDEV_DEV(netdev, &spi->dev);
>>
>> /* Perform MAC-PHY software reset */
>> if (oa_tc6_sw_reset(tc6)) {
>> @@ -417,10 +547,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>> return NULL;
>> }
>>
>> + /* Initialize PHY */
>> + if (oa_tc6_phy_init(tc6)) {
>> + dev_err(&spi->dev, "PHY initialization failed\n");
>> + return NULL;
>> + }
>> +
>> return tc6;
>> }
>> EXPORT_SYMBOL_GPL(oa_tc6_init);
>>
>> +/**
>> + * oa_tc6_exit - exit function.
>> + * @tc6: oa_tc6 struct.
>> + *
>> + */
>> +void oa_tc6_exit(struct oa_tc6 *tc6)
>> +{
>> + oa_tc6_phy_exit(tc6);
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_exit);
>> +
>> MODULE_DESCRIPTION("OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface Lib");
>> MODULE_AUTHOR("Parthiban Veerasooran <[email protected]>");
>> MODULE_LICENSE("GPL");
>> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
>> index 378636fd9ca8..36b729c384ac 100644
>> --- a/include/linux/oa_tc6.h
>> +++ b/include/linux/oa_tc6.h
>> @@ -5,54 +5,59 @@
>> * Author: Parthiban Veerasooran <[email protected]>
>> */
>>
>> +#include <linux/etherdevice.h>
>> #include <linux/spi/spi.h>
>>
>> /* Control header */
>> -#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
>> -#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
>> -#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
>> -#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
>> -#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
>> -#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
>> -#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
>> -#define CTRL_HDR_P BIT(0) /* Parity Bit */
>> +#define CTRL_HDR_DNC BIT(31) /* Data-Not-Control */
>> +#define CTRL_HDR_HDRB BIT(30) /* Received Header Bad */
>> +#define CTRL_HDR_WNR BIT(29) /* Write-Not-Read */
>> +#define CTRL_HDR_AID BIT(28) /* Address Increment Disable */
>> +#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
>> +#define CTRL_HDR_ADDR GENMASK(23, 8) /* Address */
>> +#define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
>> +#define CTRL_HDR_P BIT(0) /* Parity Bit */
>
> Please don't change the whitespace like this.
Ah yes, will correct it.
Best Regards,
Parthiban V
>
> Andrew
Hi Andrew,
On 24/10/23 6:51 am, Andrew Lunn 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:46PM +0530, Parthiban Veerasooran wrote:
>> Direct PHY Register Access Capability indicates if PHY registers are
>> directly accessible within the SPI register memory space. Indirect PHY
>> Register Access Capability indicates if PHY registers are indirectly
>> accessible through the MDIO/MDC registers MDIOACCn.
>>
>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>
> It is more normal to put all the bindings into one patch.
>
> Again, this seems like configuration, not a description of the
> hardware. Its also not clear to my why you would want to configure it.
Yes, will remove this option from DT binding and will read the
capability register (STDCAP) for the support.
Best Regards,
Parthiban V
>
> Andrew
Hi Andrew,
On 24/10/23 7:37 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static u16 oa_tc6_prepare_empty_chunk(struct oa_tc6 *tc6, u8 *buf, u8 cp_count)
>> +{
>> + u32 hdr;
>> +
>> + /* Prepare empty chunks used for getting interrupt information or if
>> + * receive data available.
>> + */
>> + for (u8 i = 0; i < cp_count; i++) {
>> + hdr = FIELD_PREP(DATA_HDR_DNC, 1);
>> + hdr |= FIELD_PREP(DATA_HDR_P, oa_tc6_get_parity(hdr));
>> + *(__be32 *)&buf[i * (tc6->cps + TC6_HDR_SIZE)] = cpu_to_be32(hdr);
>> + memset(&buf[TC6_HDR_SIZE + (i * (tc6->cps + TC6_HDR_SIZE))], 0,
>> + tc6->cps);
>> + }
>
> This is not simple, and its the sort of code which makes me wounder if
> its gone off the end of the buffer. It would be good to find somebody
> internally within Microchip to review this code.
I think, I don't need to do memset here as the header itself doesn't
describe any valid information about the payload except data not control
as it is an empty chunk and no matter what the payload contains. Apart
from that I don't get what's wrong here, anyway I will ask our internal
reviewers to review the code.
>
>> +static void oa_tc6_rx_eth_ready(struct oa_tc6 *tc6)
>> +{
>> + struct sk_buff *skb;
>> +
>> + /* Send the received ethernet packet to network layer */
>> + skb = netdev_alloc_skb(tc6->netdev, tc6->rxd_bytes + NET_IP_ALIGN);
>> + if (!skb) {
>> + tc6->netdev->stats.rx_dropped++;
>> + netdev_dbg(tc6->netdev, "Out of memory for rx'd frame");
>
> You can just return here, and skip the else. Less indentation is
> better, it generally makes the code more readable.
Ah yes. Noted.
>
>> + } else {
>> + skb_reserve(skb, NET_IP_ALIGN);
>> + memcpy(skb_put(skb, tc6->rxd_bytes), &tc6->eth_rx_buf[0],
>> + tc6->rxd_bytes);
>> + skb->protocol = eth_type_trans(skb, tc6->netdev);
>> + tc6->netdev->stats.rx_packets++;
>> + tc6->netdev->stats.rx_bytes += tc6->rxd_bytes;
>> + /* 0 for NET_RX_SUCCESS and 1 for NET_RX_DROP */
>> + if (netif_rx(skb))
>> + tc6->netdev->stats.rx_dropped++;
>
> Rather than have a comment do:
Ah ok, will do it.
>
> if (netif_rx(skb) == NET_RX_DROP)
> tc6->netdev->stats.rx_dropped++;
>
>
>> +static void oa_tc6_rx_eth_complete2(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 ebo;
>
> What does ftr and ebo mean? Its really hard to read this code because
> the names are not really meaningful.
Ok, ftr -> footer and ebo -> end_byte_offset. Will update in the next
revision.
>
>> +
>> + if (FIELD_GET(DATA_FTR_EV, ftr))
>> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
>> + else
>> + ebo = tc6->cps;
>> +
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
>> + tc6->rxd_bytes += ebo;
>> + if (FIELD_GET(DATA_FTR_EV, ftr)) {
>> + /* If EV set then send the received ethernet frame to n/w */
>> + oa_tc6_rx_eth_ready(tc6);
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = false;
>> + }
>> +}
>> +
>> +static void oa_tc6_rx_eth_complete1(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 ebo;
>> + u16 sbo;
>> +
>> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
>> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
>> +
>> + if (ebo <= sbo) {
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
>> + tc6->rxd_bytes += ebo;
>> + oa_tc6_rx_eth_ready(tc6);
>> + tc6->rxd_bytes = 0;
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
>> + tc6->cps - sbo);
>> + tc6->rxd_bytes += (tc6->cps - sbo);
>> + } else {
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
>> + ebo - sbo);
>> + tc6->rxd_bytes += (ebo - sbo);
>> + oa_tc6_rx_eth_ready(tc6);
>> + tc6->rxd_bytes = 0;
>> + }
>> +}
>> +
>> +static void oa_tc6_start_rx_eth(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 sbo;
>> +
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = true;
>> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo], tc6->cps - sbo);
>> + tc6->rxd_bytes += (tc6->cps - sbo);
>> +}
>> +
>> +static u32 oa_tc6_get_footer(struct oa_tc6 *tc6, u8 *buf, u8 cp_num)
>> +{
>> + __be32 ftr;
>> +
>> + ftr = *(__be32 *)&buf[tc6->cps + (cp_num * (tc6->cps + TC6_FTR_SIZE))];
>> +
>> + return be32_to_cpu(ftr);
>> +}
>> +
>> +static void oa_tc6_update_txc_rca(struct oa_tc6 *tc6, u32 ftr)
>> +{
>> + tc6->txc = FIELD_GET(DATA_FTR_TXC, ftr);
>> + tc6->rca = FIELD_GET(DATA_FTR_RCA, ftr);
>> +}
>> +
>> +static int oa_tc6_check_ftr_errors(struct oa_tc6 *tc6, u32 ftr)
>> +{
>> + /* Check for footer parity error */
>> + if (oa_tc6_get_parity(ftr)) {
>> + net_err_ratelimited("%s: Footer parity error\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + /* If EXST set in the footer then read STS0 register to get the
>> + * status information.
>> + */
>> + if (FIELD_GET(DATA_FTR_EXST, ftr)) {
>> + if (oa_tc6_process_exst(tc6))
>> + net_err_ratelimited("%s: Failed to process EXST\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + if (FIELD_GET(DATA_FTR_HDRB, ftr)) {
>> + net_err_ratelimited("%s: Footer eeceived header bad\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + if (!FIELD_GET(DATA_FTR_SYNC, ftr)) {
>> + net_err_ratelimited("%s: Footer configuration unsync\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + return FTR_OK;
>> +}
>> +
>> +static void oa_tc6_drop_rx_eth(struct oa_tc6 *tc6)
>> +{
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = false;
>> + tc6->netdev->stats.rx_dropped++;
>> + net_err_ratelimited("%s: Footer frame drop\n",
>> + tc6->netdev->name);
>> +}
>> +
>> +static int oa_tc6_process_rx_chunks(struct oa_tc6 *tc6, u8 *buf, u16 len)
>> +{
>> + u8 cp_count;
>> + u8 *payload;
>> + u32 ftr;
>> + int ret;
>> +
>> + /* Calculate the number of chunks received */
>> + cp_count = len / (tc6->cps + TC6_FTR_SIZE);
>> +
>> + for (u8 i = 0; i < cp_count; i++) {
>> + /* Get the footer and payload */
>> + ftr = oa_tc6_get_footer(tc6, buf, i);
>> + payload = &buf[(i * (tc6->cps + TC6_FTR_SIZE))];
>
> This would be more readable:
>
> /* Calculate the number of chunks received */
> chunks = len / (tc6->cps + TC6_FTR_SIZE);
>
> for (u8 chunk = 0; chunk < chunks; chunk++) {
> /* Get the footer and payload */
> ftr = oa_tc6_get_footer(tc6, buf, chunk);
> payload = &buf[(chunk * (tc6->cps + TC6_FTR_SIZE))];
>
> etc.
Ok thanks for the input. Will do it.
>
> And maybe move most of this code into a function
> oa_tc6_process_rx_chunk(). With lots of small functions with good
> names, you need less comments.
Yes sure. Will do it.
>
>
>> + /* Check for data valid */
>> + if (FIELD_GET(DATA_FTR_DV, ftr)) {
>> + /* Check whether both start valid and end valid are in a
>> + * single chunk payload means a single chunk payload may
>> + * contain an entire ethernet frame.
>> + */
>> + if (FIELD_GET(DATA_FTR_SV, ftr) &&
>> + FIELD_GET(DATA_FTR_EV, ftr)) {
>
>
> if (FIELD_GET(DATA_FOOTER_START_VALID, footer) &&
> FIELD_GET(DATA_FOOTER_END_VALID, footer)) {
>
> Don't you think that is more readable?
Yes it is more readable now. Will update in the next revision.
>
>> +static void oa_tc6_prepare_tx_chunks(struct oa_tc6 *tc6, u8 *buf,
>> + struct sk_buff *skb)
>> +{
>> + bool frame_started = false;
>> + u16 copied_bytes = 0;
>> + u16 copy_len;
>> + u32 hdr;
>> +
>> + /* Calculate the number tx credit counts needed to transport the tx
>> + * ethernet frame.
>> + */
>> + tc6->txc_needed = (skb->len / tc6->cps) + ((skb->len % tc6->cps) ? 1 : 0);
>
> Why call it a credit here, but a chunk when receiving?
I named as the tx path always gives the number of tx credits counts can
be enqueued for transfer. So used txc needed name to represent. Ok I
will change it as chunks_needed.
>
>> +static int oa_tc6_perform_spi_xfer(struct oa_tc6 *tc6)
>> +{
>> + bool do_tx_again;
>> + u16 total_len;
>> + u16 rca_len;
>> + u16 tx_len;
>> + int ret;
>> +
>> + do {
>> + do_tx_again = false;
>> + rca_len = 0;
>> + tx_len = 0;
>> +
>> + /* In case of an interrupt, perform an empty chunk transfer to
>> + * know the purpose of the interrupt. Interrupt may occur in
>> + * case of RCA (Receive Chunk Available) and TXC (Transmit
>> + * Credit Count). Both will occur if they are not indicated
>> + * through the previous footer.
>> + */
>> + if (tc6->int_flag) {
>> + tc6->int_flag = false;
>> + total_len = oa_tc6_prepare_empty_chunk(tc6,
>> + tc6->spi_tx_buf,
>> + 1);
>> + } else {
>> + /* Calculate the transfer length */
>> + if (tc6->tx_flag && tc6->txc) {
>> + tx_len = oa_tc6_calculate_tx_len(tc6);
>> + memcpy(&tc6->spi_tx_buf[0],
>> + &tc6->eth_tx_buf[tc6->tx_pos], tx_len);
>> + }
>> +
>> + if (tc6->rca)
>> + rca_len = oa_tc6_calculate_rca_len(tc6, tx_len);
>> +
>> + total_len = tx_len + rca_len;
>> + }
>> + ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf,
>> + tc6->spi_rx_buf, total_len);
>> + if (ret)
>> + return ret;
>> + /* Process the rxd chunks to get the ethernet frame or status */
>> + ret = oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, total_len);
>> + if (ret)
>> + return ret;
>> + if (tc6->tx_flag) {
>> + tc6->tx_pos += tx_len;
>> + tc6->txc_needed = tc6->txc_needed -
>> + (tx_len / (tc6->cps + TC6_HDR_SIZE));
>> + /* If the complete ethernet frame is transmitted then
>> + * return the skb and update the details to n/w layer.
>> + */
>> + if (!tc6->txc_needed)
>> + oa_tc6_tx_eth_complete(tc6);
>> + else if (tc6->txc)
>> + /* If txc is available again and updated from
>> + * the previous footer then perform tx again.
>> + */
>> + do_tx_again = true;
>> + }
>> +
>> + /* If rca is updated from the previous footer then perform empty
>> + * tx to receive ethernet frame.
>> + */
>> + if (tc6->rca)
>> + do_tx_again = true;
>> + } while (do_tx_again);
>
> The coding standard say:
>
> Functions should be short and sweet, and do just one thing. They
> should fit on one or two screenfuls of text (the ISO/ANSI screen size
> is 80x24, as we all know), and do one thing and do that well.
>
> This is too long, and does too many things.
Sure, will split into multiple small functions.
Best Regards,
Parthiban V
>
> Andrew
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 <[email protected]>
>> ---
>> 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: [email protected]
>> S: Maintained
>> F: drivers/net/ethernet/microchip/lan743x_*
>>
>> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
>> +M: Parthiban Veerasooran <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: drivers/net/ethernet/microchip/lan865x.c
>> +
>> MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
>> M: Arun Ramadoss <[email protected]>
>> R: [email protected]
>> 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 <[email protected]>");
>> +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
>
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, ®val);
>> + 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
>
> >> + tc6->mdiobus = mdiobus_alloc();
> >> + if (!tc6->mdiobus) {
> >> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + tc6->mdiobus->phy_mask = ~(u32)BIT(1);
> >
> > Does the standard define this ? BIT(1), not BIT(0)?
> Ok, I think here is a typo. Will correct it.
There is still the open question, does the standard define this? If
not, a vendor might decide to use some other address, not 0. So it
might be better to not set a mask and scan the whole bus.
phy_find_first() should then work, no matter what address it is using.
Andrew
> 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
Hi Krzysztof
As per the comments in the other email of this patch, this DT bindings
are going to be removed.
Best Regards,
Parthiban V
On 24/10/23 1:14 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:
>> Add DT bindings OPEN Alliance 10BASE-T1x MACPHY Serial Interface
>> parameters. These are generic properties that can apply to any 10BASE-T1x
>> MAC-PHY which uses OPEN Alliance TC6 specification.
>
> Except that it was not tested at all few more issues.
>
>>
>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>> ---
>> .../devicetree/bindings/net/oa-tc6.yaml | 72 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 73 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/oa-tc6.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/oa-tc6.yaml b/Documentation/devicetree/bindings/net/oa-tc6.yaml
>> new file mode 100644
>> index 000000000000..9f442fa6cace
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/oa-tc6.yaml
>
> Filename based on compatible.
>
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/oa-tc6.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: OPEN Alliance 10BASE-T1x MAC-PHY Specification Common Properties
>> +
>> +maintainers:
>> + - Parthiban Veerasooran <[email protected]>
>> +
>> +description:
>> + These are generic properties that can apply to any 10BASE-T1x MAC-PHY
>> + which uses OPEN Alliance TC6 specification.
>> +
>> + 10BASE-T1x MAC-PHY Serial Interface Specification can be found at:
>> + https://opensig.org/about/specifications/
>> +
>> +properties:
>> + $nodename:
>> + pattern: "^oa-tc6(@.*)?"
>
> Drop
>
>> +
>> + "#address-cells":
>> + const: 1
>> +
>> + "#size-cells":
>> + const: 0
>
> Why?
>
>> +
>> + oa-cps:
>> + maxItems: 1
>> + description:
>> + Chunk Payload Size. Configures the data chunk payload size to 2^N,
>> + where N is the value of this bitfield. The minimum possible data
>> + chunk payload size is 8 bytes or N = 3. The default data chunk
>> + payload size is 64 bytes, or N = 6. The minimum supported data chunk
>> + payload size for this MAC-PHY device is indicated in the CPSMIN
>> + field of the CAPABILITY register. Valid values for this parameter
>> + are 8, 16, 32 and 64. All other values are reserved.
>> +
>> + oa-txcte:
>> + maxItems: 1
>> + description:
>> + Transmit Cut-Through Enable. When supported by this MAC-PHY device,
>> + this bit enables the cut-through mode of frame transfer through the
>> + MAC-PHY device from the SPI host to the network.
>> +
>> + oa-rxcte:
>> + maxItems: 1
>> + description:
>> + Receive Cut-Through Enable. When supported by this MAC-PHY device,
>> + this bit enables the cut-through mode of frame transfer through the
>> + MAC-PHY device from the network to the SPI host.
>> +
>> + oa-prote:
>> + maxItems: 1
>> + description:
>> + Control data read/write Protection Enable. When set, all control
>> + data written to and read from the MAC-PHY will be transferred with
>> + its complement for detection of bit errors.
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> + - |
>> + oa-tc6 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>
> That's some total mess in indentation.
>
>> + oa-cps = <64>;
>> + oa-txcte;
>> + oa-rxcte;
>> + oa-prote;
>> + };
> Best regards,
> Krzysztof
>
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
Hi Andrew,
On 31/10/23 6:18 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> + tc6->mdiobus = mdiobus_alloc();
>>>> + if (!tc6->mdiobus) {
>>>> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + tc6->mdiobus->phy_mask = ~(u32)BIT(1);
>>>
>>> Does the standard define this ? BIT(1), not BIT(0)?
>> Ok, I think here is a typo. Will correct it.
>
> There is still the open question, does the standard define this? If
> not, a vendor might decide to use some other address, not 0. So it
> might be better to not set a mask and scan the whole bus.
> phy_find_first() should then work, no matter what address it is using.
The standard doesn't define anything about this. Ok I agree with this,
and remove the phy_mask and leave the phy_find_first() to find the phy.
Best Regards,
Parthiban V
>
> Andrew
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 <[email protected]>
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.
> 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
Hi Krzysztof
On 30/10/23 8:15 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 30/10/2023 14:16, [email protected] wrote:
>> Hi Krzysztof,
>>
>> On 24/10/23 1:33 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:
>>>> Add DT bindings for Microchip's LAN865X 10BASE-T1S MACPHY. The LAN8650/1
>>>> combines a Media Access Controller (MAC) and an Ethernet PHY to enable
>>>> 10BASE‑T1S networks. The Ethernet Media Access Controller (MAC) module
>>>> implements a 10 Mbps half duplex Ethernet MAC, compatible with the IEEE
>>>> 802.3 standard and a 10BASE-T1S physical layer transceiver integrated
>>>> into the LAN8650/1. The communication between the Host and the MAC-PHY is
>>>> specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6).
>>>
>>> It does not look like you tested the bindings, at least after quick
>>> look. Please run `make dt_binding_check` (see
>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>> Maybe you need to update your dtschema and yamllint.
>> Sorry, somehow I missed doing this check. Will fix this in the next
>> revision.
>
> Please also build your driver.
Didn't see any error in the driver compilation.
>
>>>
>>>>
>>>> Signed-off-by: Parthiban Veerasooran <[email protected]>
>>>> ---
>>>> .../bindings/net/microchip,lan865x.yaml | 101 ++++++++++++++++++
>>>> MAINTAINERS | 1 +
>>>> 2 files changed, 102 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>> new file mode 100644
>>>> index 000000000000..974622dd6846
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>>>> @@ -0,0 +1,101 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/net/microchip,lan865x.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Microchip LAN8650/1 10BASE-T1S MACPHY Ethernet Controllers
>>>> +
>>>> +maintainers:
>>>> + - Parthiban Veerasooran <[email protected]>
>>>> +
>>>> +description:
>>>> + The LAN8650/1 combines a Media Access Controller (MAC) and an Ethernet
>>>> + PHY to enable 10BASE‑T1S networks. The Ethernet Media Access Controller
>>>> + (MAC) module implements a 10 Mbps half duplex Ethernet MAC, compatible
>>>> + with the IEEE 802.3 standard and a 10BASE-T1S physical layer transceiver
>>>> + integrated into the LAN8650/1. The communication between the Host and
>>>> + the MAC-PHY is specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
>>>> + Interface (TC6).
>>>> +
>>>> + Specifications about the LAN8650/1 can be found at:
>>>> + https://www.microchip.com/en-us/product/lan8650
>>>> +
>>>> +allOf:
>>>> + - $ref: ethernet-controller.yaml#
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + const: microchip,lan865x
>>>
>>> No wildcards in compatibles.
>> Ah ok missed it. So it will become like below isn't it?
>>
>> properties:
>> compatible:
>> enum:
>> - microchip,lan8650
>> - microchip,lan8651
>
> Rather enum for lan8650 and items for lan8651 with fallback. Aren't they
> compatible, since you wanted to use wildcard?
So do you mean like below?
properties:
compatible:
oneOf:
- items:
- const: microchip,lan8650
- const: microchip,lan8651
- enum:
- microchip,lan8650
Best Regards,
Parthiban V
>
>
> Best regards,
> Krzysztof
>
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.
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 <[email protected]>
>
> 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.
>
> 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
Hi Parthiban
I hope I send this in the right context as it is not related to just one patch or
some specific code.
I conducted UDP load testing using three i.MX8MM boards in conjunction with the
LAN8651. The setup involved one board functioning as a server, which is just
echoing back received data, while the remaining two boards acted as clients,
sending UDP packets of different sizes in various bursts to the server.
Due to hardware constraints, the SPI bus speed was limited to 15 MHz, which might
have influenced the results.
During the tests I experienced some issues:
- The boards just start receiving after first sending something (ping another board).
Some measurements showed that the irq stays asserted after init. This makes sense
as far as I understand the chapter 7.7 of the specification, the irq is deasserted
on reception of the first data header following CSn being asserted. As a workaround
I trigger the thread at the end of oa_tc6_init.
- If there is a lot of traffic, the receive buffer overflow error spams the log.
- If there is a lot of traffic, I got various kernel panics in oa_tc6_update_rx_skb.
Mostly because more data to rx_skb is added than allocated and sometimes because
rx_skb is null in oa_tc6_update_rx_skb or oa_tc6_prcs_rx_frame_end. Some debugging
with a logic analyzer showed that the chip is not behave correctly. There is more
bytes between start_valid and end_valid than there should be. Also there
seems to be 2 end_valid without a start_valid between. What is common is that the incorrect
frame starts in a chunk where end_valid and start_valid is set.
In my opinion its a problem in the chip (maybe related to the errata in the next point)
but the driver should be resilent and just drop the packet and not cause a kernel panic.
- Sometimes the chip stops working. It always asserts the irq but there is no data (rca=0)
and also exst is not active. I found out that there is an errata (DS80001075) point s3
that explains this. I set the ZARFE bit in CONFIG0. This also fixes the point above.
The driver now works since about 2.5 weeks with various load with just one loss of frame
error where I had to reboot the system after about 4 days.
Is there a reason why you removed the netdev watchdog which was active in v2?
Thanks,
Benjamin Bigler
Hi Benjamin Bigler,
Thank you for your testing and feedback. It would be really helpful to
bring the driver to a good shape. We really appreciate your efforts on this.
On 24/03/24 5:25 pm, Benjamin Bigler wrote:
> [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Parthiban
>
> I hope I send this in the right context as it is not related to just one patch or
> some specific code.
>
> I conducted UDP load testing using three i.MX8MM boards in conjunction with the
> LAN8651. The setup involved one board functioning as a server, which is just
> echoing back received data, while the remaining two boards acted as clients,
> sending UDP packets of different sizes in various bursts to the server.
> Due to hardware constraints, the SPI bus speed was limited to 15 MHz, which might
> have influenced the results.
>
> During the tests I experienced some issues:
>
> - The boards just start receiving after first sending something (ping another board).
> Some measurements showed that the irq stays asserted after init. This makes sense
> as far as I understand the chapter 7.7 of the specification, the irq is deasserted
> on reception of the first data header following CSn being asserted. As a workaround
> I trigger the thread at the end of oa_tc6_init.
It looks like the IRQ is asserted on RESET completion and expects a data
chunk from host to deassert the IRQ. I used to test the driver in RPI 4
using iperf3. For some reason I never faced this issue, may be when the
network device is being registered there might be some packet
transmission which leads to deliver a data chunk so that the IRQ is
deasserted. Thanks for the workaround. I think that would be the
solution to solve this issue. Adding the below lines in the end of the
function oa_tc6_init() will trigger the oa_tc6_spi_thread_handler() to
perform an empty data chunk transfer which will deassert the IRQ before
starting the actual data transfer.
/* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
* complete status. IRQ is also asserted on reset completion and it is
* remain asserted until MAC-PHY receives a data chunk. So performing an
* empty data chunk transmission will deassert the IRQ. Refer section
* 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
*/
tc6->int_flag = true;
wake_up_interruptible(&tc6->spi_wq);
>
> - If there is a lot of traffic, the receive buffer overflow error spams the log.
>
> - If there is a lot of traffic, I got various kernel panics in oa_tc6_update_rx_skb.
> Mostly because more data to rx_skb is added than allocated and sometimes because
> rx_skb is null in oa_tc6_update_rx_skb or oa_tc6_prcs_rx_frame_end. Some debugging
> with a logic analyzer showed that the chip is not behave correctly. There is more
> bytes between start_valid and end_valid than there should be. Also there
> seems to be 2 end_valid without a start_valid between. What is common is that the incorrect
> frame starts in a chunk where end_valid and start_valid is set.
> In my opinion its a problem in the chip (maybe related to the errata in the next point)
> but the driver should be resilent and just drop the packet and not cause a kernel panic.
Usually I run into this issue "receive buffer overflow" when I run RPI 4
with default cpu governor setting which is "ondemand". In this case,
even though if I set SPI clock speed as 15 MHz the RPI 4 core clock is
clocking down when it is idle which leads delivering half of the
configured SPI clock speed around 5.9 MHz. So the systems like RPI 4
need performance mode enabled to get the proper clock speed for SPI.
Refer below link for more details.
https://github.com/raspberrypi/linux/issues/3381#issuecomment-1144723750
I used to enable performance mode using the below command.
echo performance | sudo tee
/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor > /dev/null
So please ensure the SPI clock speed using a logic analyzer to get the
maximum throughput without receive buffer overflow.
Of course, I agree that the driver should not crash in case of receive
buffer overflow. By referring your investigations, I understand that the
buffers in the MAC-PHY is being continuously overwritten again and again
as the host is very slow to read the data from the MAC-PHY buffers
through SPI which alters the descriptors. There might be two reasons why
we run into this situation.
1. The host is busy doing something else and delays to initiate SPI even
though SPI clock speed is 15 MHz.
2. The SPI clock speed is less than 15 MHz.
I use the below iperf3 setup for my testing and never faced the driver
crash issue even though faced "receive buffer overflow" error when I run
RPI 4 with "ondemand" default mode.
Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY
$ iperf3 -s
Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
$ iperf3 -c 192.168.5.100 -u -b 10M -i 1 -t 0
and vice versa.
I never faced "receive buffer overflow" error when I run RPI 4 with
"performance" mode enabled and even though all the cores are stressed
using the below command,
$ yes >/dev/null & yes >/dev/null & yes >/dev/null & yes >/dev/null &
Can you share more details about your testing setup and applications you
use, so that I will try to reproduce the issue in my setup to debug the
driver?
>
> - Sometimes the chip stops working. It always asserts the irq but there is no data (rca=0)
> and also exst is not active. I found out that there is an errata (DS80001075) point s3
> that explains this. I set the ZARFE bit in CONFIG0. This also fixes the point above.
> The driver now works since about 2.5 weeks with various load with just one loss of frame
> error where I had to reboot the system after about 4 days.
It is good to hear that the driver works fine with the above changes. As
mentioned in the errata, this continuous interrupt issue is a known
issue with LAN8651 Rev.B0. Switching to LAN8651 Rev.B1 will solve this
issue and no need of any workaround. Setting ZARFE bit in the CONFIG0
will solve the continuous interrupt issue but don't know how the above
"receive buffer overflow" issue also solved. I think it is a good idea
to test with LAN8651 Rev.B1 without setting ZARFE bit once. It would be
interesting to see the result. I am always using LAN8651 Rev.B1 for my
testing.
I should be able to reproduce the "receive buffer overflow" issue and
consequently kernel crash in my setup with LAN8651 Rev.B1 so that I can
investigate the issue further. As I am not able to reproduce in my RPI
4, I need your support for the tests and applications you used in your
setup.
>
> Is there a reason why you removed the netdev watchdog which was active in v2?
When the timeout occurs, there is no further action except increasing
tx_errors. Not seeing this except USB-to-Ethernet which can be removed
unexpectedly. But this is SPI interface which will not be removed
unexpectedly as it is a platform device. That's why we removed this.
Best regards,
Parthiban V
>
> Thanks,
> Benjamin Bigler
>
> It looks like the IRQ is asserted on RESET completion and expects a data
> chunk from host to deassert the IRQ. I used to test the driver in RPI 4
> using iperf3. For some reason I never faced this issue, may be when the
> network device is being registered there might be some packet
> transmission which leads to deliver a data chunk so that the IRQ is
> deasserted.
If you have IPv6 enabled, the network stack will try to add a link
local IPv6 address to the interface, which means performing a
Duplicate Address Detection. That means sending a few packets.
Try disabling IPv6 if you want to reproduce the problem.
Andrew
Hi Andrew,
On 25/03/24 7:31 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> It looks like the IRQ is asserted on RESET completion and expects a data
>> chunk from host to deassert the IRQ. I used to test the driver in RPI 4
>> using iperf3. For some reason I never faced this issue, may be when the
>> network device is being registered there might be some packet
>> transmission which leads to deliver a data chunk so that the IRQ is
>> deasserted.
>
> If you have IPv6 enabled, the network stack will try to add a link
> local IPv6 address to the interface, which means performing a
> Duplicate Address Detection. That means sending a few packets.
>
> Try disabling IPv6 if you want to reproduce the problem.
Yes, I saw that IPv6 is enabled by default in my RPI 4. So as you
mentioned, I tried disabling it but still approximately in 1 second
there is a packet coming from the n/w stack which makes the IRQ
deasserted (Refer the attached screenshot). So just for testing, I
disabled waking up the SPI thread from the oa_tc6_start_xmit() function
and noted that the IRQ is not deasserted throughout the period in Logic
analyzer.
As I replied in the previous email, using the below code makes the IRQ
deasserted when the oa_tc6_init() finishes without expecting any data
chunk from oa_tc6_start_xmit(). So in my opinion let's stick with this
solution.
/* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
* complete status. IRQ is also asserted on reset completion and it is
* remain asserted until MAC-PHY receives a data chunk. So performing an
* empty data chunk transmission will deassert the IRQ. Refer section
* 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
*/
tc6->int_flag = true;
wake_up_interruptible(&tc6->spi_wq);
Best regards,
Parthiban V
>
> Andrew
Hi Parthiban,
Sorry for the late answer, I was quite busy the last few days.
On Mon, 2024-03-25 at 13:24 +0000, [email protected] wrote:
> Hi Benjamin Bigler,
>
> Thank you for your testing and feedback. It would be really helpful to
> bring the driver to a good shape. We really appreciate your efforts on this.
>
> On 24/03/24 5:25 pm, Benjamin Bigler wrote:
> > [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Hi Parthiban
> >
> > I hope I send this in the right context as it is not related to just one patch or
> > some specific code.
> >
> > I conducted UDP load testing using three i.MX8MM boards in conjunction with the
> > LAN8651. The setup involved one board functioning as a server, which is just
> > echoing back received data, while the remaining two boards acted as clients,
> > sending UDP packets of different sizes in various bursts to the server.
> > Due to hardware constraints, the SPI bus speed was limited to 15 MHz, which might
> > have influenced the results.
> >
> > During the tests I experienced some issues:
> >
> > - The boards just start receiving after first sending something (ping another board).
> > Some measurements showed that the irq stays asserted after init. This makes sense
> > as far as I understand the chapter 7.7 of the specification, the irq is deasserted
> > on reception of the first data header following CSn being asserted. As a workaround
> > I trigger the thread at the end of oa_tc6_init.
> It looks like the IRQ is asserted on RESET completion and expects a data
> chunk from host to deassert the IRQ. I used to test the driver in RPI 4
> using iperf3. For some reason I never faced this issue, may be when the
> network device is being registered there might be some packet
> transmission which leads to deliver a data chunk so that the IRQ is
> deasserted. Thanks for the workaround. I think that would be the
> solution to solve this issue. Adding the below lines in the end of the
> function oa_tc6_init() will trigger the oa_tc6_spi_thread_handler() to
> perform an empty data chunk transfer which will deassert the IRQ before
> starting the actual data transfer.
I have ipv6 disabled and use static ipv4 addresses. That could be the reason why on
my side no packet is sent.
>
> /* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
> * complete status. IRQ is also asserted on reset completion and it is
> * remain asserted until MAC-PHY receives a data chunk. So performing an
> * empty data chunk transmission will deassert the IRQ. Refer section
> * 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
> */
> tc6->int_flag = true;
> wake_up_interruptible(&tc6->spi_wq);
Perfect, thats the same I added and also works on my side.
> >
> > - If there is a lot of traffic, the receive buffer overflow error spams the log.
> >
> > - If there is a lot of traffic, I got various kernel panics in oa_tc6_update_rx_skb.
> > Mostly because more data to rx_skb is added than allocated and sometimes because
> > rx_skb is null in oa_tc6_update_rx_skb or oa_tc6_prcs_rx_frame_end. Some debugging
> > with a logic analyzer showed that the chip is not behave correctly. There is more
> > bytes between start_valid and end_valid than there should be. Also there
> > seems to be 2 end_valid without a start_valid between. What is common is that the incorrect
> > frame starts in a chunk where end_valid and start_valid is set.
> > In my opinion its a problem in the chip (maybe related to the errata in the next point)
> > but the driver should be resilent and just drop the packet and not cause a kernel panic.
> Usually I run into this issue "receive buffer overflow" when I run RPI 4
> with default cpu governor setting which is "ondemand". In this case,
> even though if I set SPI clock speed as 15 MHz the RPI 4 core clock is
> clocking down when it is idle which leads delivering half of the
> configured SPI clock speed around 5.9 MHz. So the systems like RPI 4
> need performance mode enabled to get the proper clock speed for SPI.
> Refer below link for more details.
>
> https://github.com/raspberrypi/linux/issues/3381#issuecomment-1144723750
>
> I used to enable performance mode using the below command.
>
> echo performance | sudo tee
> /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor > /dev/null
>
> So please ensure the SPI clock speed using a logic analyzer to get the
> maximum throughput without receive buffer overflow.
>
> Of course, I agree that the driver should not crash in case of receive
> buffer overflow. By referring your investigations, I understand that the
> buffers in the MAC-PHY is being continuously overwritten again and again
> as the host is very slow to read the data from the MAC-PHY buffers
> through SPI which alters the descriptors. There might be two reasons why
> we run into this situation.
> 1. The host is busy doing something else and delays to initiate SPI even
> though SPI clock speed is 15 MHz.
> 2. The SPI clock speed is less than 15 MHz.
Sorry there is a missunderstanding between us. The receive buffer overflow is not
causing any harm except filling the log. In my setup I get in one day about 35000
entries. I am not sure if its appropriate to log these errors.
The SPI Frequency is at 14.8 MHz. If I just have 2 boards connected, I am not able
to reproduce this. Only with 3 boards when 2 boards sends multiple big ethernet
frames (1512 byte per Frame) to one, I get these log entries.
The latency seems to be quite low, from IRQ to start reading first frame it takes
always less than 500us. Also the boards are just running the udp test.
>
> I use the below iperf3 setup for my testing and never faced the driver
> crash issue even though faced "receive buffer overflow" error when I run
> RPI 4 with "ondemand" default mode.
>
> Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY
> $ iperf3 -s
> Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
> $ iperf3 -c 192.168.5.100 -u -b 10M -i 1 -t 0
>
> and vice versa.
>
> I never faced "receive buffer overflow" error when I run RPI 4 with
> "performance" mode enabled and even though all the cores are stressed
> using the below command,
>
> $ yes >/dev/null & yes >/dev/null & yes >/dev/null & yes >/dev/null &
>
> Can you share more details about your testing setup and applications you
> use, so that I will try to reproduce the issue in my setup to debug the
> driver?
I use a internal tool which does some stress tests using udp. Unfortunately,
I am not allowed to publish it, but a colleague works on a rust implementation,
which we can publish, but its not fully ready yet.
On one board the tool is running in server mode. It just echoes back the received
data. On the 2 other boards the tool is running in client mode. It sends various
sized udp-packets in different bursts and then checks if it receives the same
data in the same order.
The crashes only happens when ZARFE is not set (with Rev B0). When the crash
happens, I see on the logic analyzer that there are more bytes than mtu + headers
between the frame where start_valid is set and the frame where end_valid is set.
Then this happens:
[ 437.155673] skbuff: skb_over_panic: text:ffff80007a8c2bd8 len:1600 put:64 head:ffff00000de28080
data:ffff00000de280c0 tail:0x680 end:0x640 dev:eth1
[ 437.168987] kernel BUG at net/core/skbuff.c:192!
[ 437.173612] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
[ 437.180407] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
tpm_tis_spi tpm_tis_core spi_imx imx_sdma
[ 437.196016] CPU: 1 PID: 455 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
gce336e2c2bc3-dirty #1
[ 437.205853] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
[ 437.212820] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 437.219790] pc : skb_panic+0x58/0x5c
[ 437.223376] lr : skb_panic+0x58/0x5c
[ 437.226959] sp : ffff80008362bd90
[ 437.230278] x29: ffff80008362bda0 x28: 0000000000000000 x27: ffff000001066878
[ 437.237426] x26: 000000000000001e x25: 00000000000007f8 x24: ffff0000010cea80
[ 437.244571] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
[ 437.251720] x20: ffff0000010ceaa8 x19: 000000003f20003f x18: ffffffffffffffff
[ 437.258867] x17: ffff7ffffded9000 x16: ffff800080008000 x15: 073a0764076e0765
[ 437.266015] x14: 0720073007380736 x13: ffff8000823d1f58 x12: 0000000000000534
[ 437.273162] x11: 00000000000001bc x10: ffff800082429f58 x9 : ffff8000823d1f58
[ 437.280310] x8 : 00000000ffffefff x7 : ffff800082429f58 x6 : 0000000000000000
[ 437.287455] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000
[ 437.294606] x2 : 0000000000000000 x1 : ffff000001223b00 x0 : 0000000000000087
[ 437.301753] Call trace:
[ 437.304203] skb_panic+0x58/0x5c
[ 437.307436] skb_find_text+0x0/0xf0
[ 437.310933] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
[ 437.316523] kthread+0x118/0x11c
[ 437.319758] ret_from_fork+0x10/0x20
[ 437.323343] Code: f90007e9 b940b908 f90003e8 97ca3c34 (d4210000)
[ 437.329446] ---[ end trace 0000000000000000 ]---
Sometimes there are 2 end_valid after eachother without a start_valid between.
Then this happens:
[ 469.737297] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074
[ 469.746137] Mem abort info:
[ 469.748950] ESR = 0x0000000096000004
[ 469.752709] EC = 0x25: DABT (current EL), IL = 32 bits
[ 469.758036] SET = 0, FnV = 0
[ 469.761098] EA = 0, S1PTW = 0
[ 469.764252] FSC = 0x04: level 0 translation fault
[ 469.769144] Data abort info:
[ 469.772033] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 469.777529] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 469.782594] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 469.787921] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000043c32000
[ 469.794377] [0000000000000074] pgd=0000000000000000, p4d=0000000000000000
[ 469.801184] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 469.807459] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
tpm_tis_spi tpm_tis_core spi_imx imx_sdma
[ 469.823064] CPU: 2 PID: 456 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
g350ed394a6ca-dirty #1
[ 469.832903] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
[ 469.839871] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 469.846841] pc : skb_put+0xc/0x6c
[ 469.850169] lr : oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
[ 469.856106] sp : ffff80008376bdb0
[ 469.859424] x29: ffff80008376bdb0 x28: 0000000000000000 x27: ffff00000194c080
[ 469.866573] x26: 0000000000000000 x25: 0000000000000000 x24: ffff000001095c80
[ 469.873720] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
[ 469.880870] x20: ffff000001095ca8 x19: 000000003f20003f x18: 0000000000000000
[ 469.888023] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[ 469.895174] x14: 0000031acf8b86d8 x13: 0000000000000000 x12: 0000000000000000
[ 469.902321] x11: 0000000000000002 x10: 0000000000000a60 x9 : ffff80008376b970
[ 469.909467] x8 : ffff00007fb6e580 x7 : 000000000194b080 x6 : 0000000000000000
[ 469.916616] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 000000000000fc80
[ 469.923765] x2 : 0000000000000001 x1 : 0000000000000040 x0 : 0000000000000000
[ 469.930915] Call trace:
[ 469.933365] skb_put+0xc/0x6c
[ 469.936342] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
[ 469.941929] kthread+0x118/0x11c
[ 469.945166] ret_from_fork+0x10/0x20
[ 469.948752] Code: d65f03c0 d503233f a9bf7bfd 910003fd (b9407406)
[ 469.954854] ---[ end trace 0000000000000000 ]---
If interested I can try to get a recording with the logic analyzer and send it to you.
By the way in the other answer you attached a screenshot of the logic analyzer and you
have a very nice HLA for oa_tc6. Are they open-source or are there any plans to publish them?
> >
> > - Sometimes the chip stops working. It always asserts the irq but there is no data (rca=0)
> > and also exst is not active. I found out that there is an errata (DS80001075) point s3
> > that explains this. I set the ZARFE bit in CONFIG0. This also fixes the point above.
> > The driver now works since about 2.5 weeks with various load with just one loss of frame
> > error where I had to reboot the system after about 4 days.
> It is good to hear that the driver works fine with the above changes. As
> mentioned in the errata, this continuous interrupt issue is a known
> issue with LAN8651 Rev.B0. Switching to LAN8651 Rev.B1 will solve this
> issue and no need of any workaround. Setting ZARFE bit in the CONFIG0
> will solve the continuous interrupt issue but don't know how the above
> "receive buffer overflow" issue also solved. I think it is a good idea
> to test with LAN8651 Rev.B1 without setting ZARFE bit once. It would be
> interesting to see the result. I am always using LAN8651 Rev.B1 for my
> testing.
Unfortunately I just have LAN8651 Rev. B0 Chips. Are you sure that the Rev B1 has the
issue fixed? The errata here says that B1 is affected too:
https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/Errata/LAN8650-1-Errata-80001075.pdf
>
> I should be able to reproduce the "receive buffer overflow" issue and
> consequently kernel crash in my setup with LAN8651 Rev.B1 so that I can
> investigate the issue further. As I am not able to reproduce in my RPI
> 4, I need your support for the tests and applications you used in your
> setup.
> >
> > Is there a reason why you removed the netdev watchdog which was active in v2?
> When the timeout occurs, there is no further action except increasing
> tx_errors. Not seeing this except USB-to-Ethernet which can be removed
> unexpectedly. But this is SPI interface which will not be removed
> unexpectedly as it is a platform device. That's why we removed this.
>
> Best regards,
> Parthiban V
> >
> > Thanks,
> > Benjamin Bigler
> >
>
Thanks,
Benjamin Bigler
Hi Benjamin,
On 04/04/24 3:10 am, Benjamin Bigler wrote:
> [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi Parthiban,
>
> Sorry for the late answer, I was quite busy the last few days.
No problem.
>
> On Mon, 2024-03-25 at 13:24 +0000, [email protected] wrote:
>> Hi Benjamin Bigler,
>>
>> Thank you for your testing and feedback. It would be really helpful to
>> bring the driver to a good shape. We really appreciate your efforts on this.
>>
>> On 24/03/24 5:25 pm, Benjamin Bigler wrote:
>>> [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi Parthiban
>>>
>>> I hope I send this in the right context as it is not related to just one patch or
>>> some specific code.
>>>
>>> I conducted UDP load testing using three i.MX8MM boards in conjunction with the
>>> LAN8651. The setup involved one board functioning as a server, which is just
>>> echoing back received data, while the remaining two boards acted as clients,
>>> sending UDP packets of different sizes in various bursts to the server.
>>> Due to hardware constraints, the SPI bus speed was limited to 15 MHz, which might
>>> have influenced the results.
>>>
>>> During the tests I experienced some issues:
>>>
>>> - The boards just start receiving after first sending something (ping another board).
>>> Some measurements showed that the irq stays asserted after init. This makes sense
>>> as far as I understand the chapter 7.7 of the specification, the irq is deasserted
>>> on reception of the first data header following CSn being asserted. As a workaround
>>> I trigger the thread at the end of oa_tc6_init.
>> It looks like the IRQ is asserted on RESET completion and expects a data
>> chunk from host to deassert the IRQ. I used to test the driver in RPI 4
>> using iperf3. For some reason I never faced this issue, may be when the
>> network device is being registered there might be some packet
>> transmission which leads to deliver a data chunk so that the IRQ is
>> deasserted. Thanks for the workaround. I think that would be the
>> solution to solve this issue. Adding the below lines in the end of the
>> function oa_tc6_init() will trigger the oa_tc6_spi_thread_handler() to
>> perform an empty data chunk transfer which will deassert the IRQ before
>> starting the actual data transfer.
>
> I have ipv6 disabled and use static ipv4 addresses. That could be the reason why on
> my side no packet is sent.
>
>>
>> /* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
>> * complete status. IRQ is also asserted on reset completion and it is
>> * remain asserted until MAC-PHY receives a data chunk. So performing an
>> * empty data chunk transmission will deassert the IRQ. Refer section
>> * 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
>> */
>> tc6->int_flag = true;
>> wake_up_interruptible(&tc6->spi_wq);
>
> Perfect, thats the same I added and also works on my side.
>
>>>
>>> - If there is a lot of traffic, the receive buffer overflow error spams the log.
>>>
>>> - If there is a lot of traffic, I got various kernel panics in oa_tc6_update_rx_skb.
>>> Mostly because more data to rx_skb is added than allocated and sometimes because
>>> rx_skb is null in oa_tc6_update_rx_skb or oa_tc6_prcs_rx_frame_end. Some debugging
>>> with a logic analyzer showed that the chip is not behave correctly. There is more
>>> bytes between start_valid and end_valid than there should be. Also there
>>> seems to be 2 end_valid without a start_valid between. What is common is that the incorrect
>>> frame starts in a chunk where end_valid and start_valid is set.
>>> In my opinion its a problem in the chip (maybe related to the errata in the next point)
>>> but the driver should be resilent and just drop the packet and not cause a kernel panic.
>> Usually I run into this issue "receive buffer overflow" when I run RPI 4
>> with default cpu governor setting which is "ondemand". In this case,
>> even though if I set SPI clock speed as 15 MHz the RPI 4 core clock is
>> clocking down when it is idle which leads delivering half of the
>> configured SPI clock speed around 5.9 MHz. So the systems like RPI 4
>> need performance mode enabled to get the proper clock speed for SPI.
>> Refer below link for more details.
>>
>> https://github.com/raspberrypi/linux/issues/3381#issuecomment-1144723750
>>
>> I used to enable performance mode using the below command.
>>
>> echo performance | sudo tee
>> /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor > /dev/null
>>
>> So please ensure the SPI clock speed using a logic analyzer to get the
>> maximum throughput without receive buffer overflow.
>>
>> Of course, I agree that the driver should not crash in case of receive
>> buffer overflow. By referring your investigations, I understand that the
>> buffers in the MAC-PHY is being continuously overwritten again and again
>> as the host is very slow to read the data from the MAC-PHY buffers
>> through SPI which alters the descriptors. There might be two reasons why
>> we run into this situation.
>> 1. The host is busy doing something else and delays to initiate SPI even
>> though SPI clock speed is 15 MHz.
>> 2. The SPI clock speed is less than 15 MHz.
>
> Sorry there is a missunderstanding between us. The receive buffer overflow is not
> causing any harm except filling the log. In my setup I get in one day about 35000
> entries. I am not sure if its appropriate to log these errors.
>
> The SPI Frequency is at 14.8 MHz. If I just have 2 boards connected, I am not able
> to reproduce this. Only with 3 boards when 2 boards sends multiple big ethernet
> frames (1512 byte per Frame) to one, I get these log entries.
> The latency seems to be quite low, from IRQ to start reading first frame it takes
> always less than 500us. Also the boards are just running the udp test.
>
>>
>> I use the below iperf3 setup for my testing and never faced the driver
>> crash issue even though faced "receive buffer overflow" error when I run
>> RPI 4 with "ondemand" default mode.
>>
>> Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY
>> $ iperf3 -s
>> Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
>> $ iperf3 -c 192.168.5.100 -u -b 10M -i 1 -t 0
>>
>> and vice versa.
>>
>> I never faced "receive buffer overflow" error when I run RPI 4 with
>> "performance" mode enabled and even though all the cores are stressed
>> using the below command,
>>
>> $ yes >/dev/null & yes >/dev/null & yes >/dev/null & yes >/dev/null &
>>
>> Can you share more details about your testing setup and applications you
>> use, so that I will try to reproduce the issue in my setup to debug the
>> driver?
>
> I use a internal tool which does some stress tests using udp. Unfortunately,
> I am not allowed to publish it, but a colleague works on a rust implementation,
> which we can publish, but its not fully ready yet.
> On one board the tool is running in server mode. It just echoes back the received
> data. On the 2 other boards the tool is running in client mode. It sends various
> sized udp-packets in different bursts and then checks if it receives the same
> data in the same order.
>
>
> The crashes only happens when ZARFE is not set (with Rev B0). When the crash
> happens, I see on the logic analyzer that there are more bytes than mtu + headers
> between the frame where start_valid is set and the frame where end_valid is set.
> Then this happens:
Thanks for all the above details. I will include this ZARFE fix in the
next version v4 which I am going to post soon.
>
> [ 437.155673] skbuff: skb_over_panic: text:ffff80007a8c2bd8 len:1600 put:64 head:ffff00000de28080
> data:ffff00000de280c0 tail:0x680 end:0x640 dev:eth1
> [ 437.168987] kernel BUG at net/core/skbuff.c:192!
> [ 437.173612] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> [ 437.180407] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
> tpm_tis_spi tpm_tis_core spi_imx imx_sdma
> [ 437.196016] CPU: 1 PID: 455 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
> gce336e2c2bc3-dirty #1
> [ 437.205853] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
> [ 437.212820] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 437.219790] pc : skb_panic+0x58/0x5c
> [ 437.223376] lr : skb_panic+0x58/0x5c
> [ 437.226959] sp : ffff80008362bd90
> [ 437.230278] x29: ffff80008362bda0 x28: 0000000000000000 x27: ffff000001066878
> [ 437.237426] x26: 000000000000001e x25: 00000000000007f8 x24: ffff0000010cea80
> [ 437.244571] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
> [ 437.251720] x20: ffff0000010ceaa8 x19: 000000003f20003f x18: ffffffffffffffff
> [ 437.258867] x17: ffff7ffffded9000 x16: ffff800080008000 x15: 073a0764076e0765
> [ 437.266015] x14: 0720073007380736 x13: ffff8000823d1f58 x12: 0000000000000534
> [ 437.273162] x11: 00000000000001bc x10: ffff800082429f58 x9 : ffff8000823d1f58
> [ 437.280310] x8 : 00000000ffffefff x7 : ffff800082429f58 x6 : 0000000000000000
> [ 437.287455] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000
> [ 437.294606] x2 : 0000000000000000 x1 : ffff000001223b00 x0 : 0000000000000087
> [ 437.301753] Call trace:
> [ 437.304203] skb_panic+0x58/0x5c
> [ 437.307436] skb_find_text+0x0/0xf0
> [ 437.310933] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
> [ 437.316523] kthread+0x118/0x11c
> [ 437.319758] ret_from_fork+0x10/0x20
> [ 437.323343] Code: f90007e9 b940b908 f90003e8 97ca3c34 (d4210000)
> [ 437.329446] ---[ end trace 0000000000000000 ]---
>
>
> Sometimes there are 2 end_valid after eachother without a start_valid between.
> Then this happens:
>
> [ 469.737297] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074
> [ 469.746137] Mem abort info:
> [ 469.748950] ESR = 0x0000000096000004
> [ 469.752709] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 469.758036] SET = 0, FnV = 0
> [ 469.761098] EA = 0, S1PTW = 0
> [ 469.764252] FSC = 0x04: level 0 translation fault
> [ 469.769144] Data abort info:
> [ 469.772033] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [ 469.777529] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 469.782594] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 469.787921] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000043c32000
> [ 469.794377] [0000000000000074] pgd=0000000000000000, p4d=0000000000000000
> [ 469.801184] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [ 469.807459] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
> tpm_tis_spi tpm_tis_core spi_imx imx_sdma
> [ 469.823064] CPU: 2 PID: 456 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
> g350ed394a6ca-dirty #1
> [ 469.832903] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
> [ 469.839871] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 469.846841] pc : skb_put+0xc/0x6c
> [ 469.850169] lr : oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
> [ 469.856106] sp : ffff80008376bdb0
> [ 469.859424] x29: ffff80008376bdb0 x28: 0000000000000000 x27: ffff00000194c080
> [ 469.866573] x26: 0000000000000000 x25: 0000000000000000 x24: ffff000001095c80
> [ 469.873720] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
> [ 469.880870] x20: ffff000001095ca8 x19: 000000003f20003f x18: 0000000000000000
> [ 469.888023] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> [ 469.895174] x14: 0000031acf8b86d8 x13: 0000000000000000 x12: 0000000000000000
> [ 469.902321] x11: 0000000000000002 x10: 0000000000000a60 x9 : ffff80008376b970
> [ 469.909467] x8 : ffff00007fb6e580 x7 : 000000000194b080 x6 : 0000000000000000
> [ 469.916616] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 000000000000fc80
> [ 469.923765] x2 : 0000000000000001 x1 : 0000000000000040 x0 : 0000000000000000
> [ 469.930915] Call trace:
> [ 469.933365] skb_put+0xc/0x6c
> [ 469.936342] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
> [ 469.941929] kthread+0x118/0x11c
> [ 469.945166] ret_from_fork+0x10/0x20
> [ 469.948752] Code: d65f03c0 d503233f a9bf7bfd 910003fd (b9407406)
> [ 469.954854] ---[ end trace 0000000000000000 ]---
>
>
> If interested I can try to get a recording with the logic analyzer and send it to you.
I don't think it is needed.
>
> By the way in the other answer you attached a screenshot of the logic analyzer and you
> have a very nice HLA for oa_tc6. Are they open-source or are there any plans to publish them?
It is already available in the Microchip's github page for public.
Checkout the below link for the same.
https://github.com/MicrochipTech/oa-tc6-saleae-extension
>
>>>
>>> - Sometimes the chip stops working. It always asserts the irq but there is no data (rca=0)
>>> and also exst is not active. I found out that there is an errata (DS80001075) point s3
>>> that explains this. I set the ZARFE bit in CONFIG0. This also fixes the point above.
>>> The driver now works since about 2.5 weeks with various load with just one loss of frame
>>> error where I had to reboot the system after about 4 days.
>> It is good to hear that the driver works fine with the above changes. As
>> mentioned in the errata, this continuous interrupt issue is a known
>> issue with LAN8651 Rev.B0. Switching to LAN8651 Rev.B1 will solve this
>> issue and no need of any workaround. Setting ZARFE bit in the CONFIG0
>> will solve the continuous interrupt issue but don't know how the above
>> "receive buffer overflow" issue also solved. I think it is a good idea
>> to test with LAN8651 Rev.B1 without setting ZARFE bit once. It would be
>> interesting to see the result. I am always using LAN8651 Rev.B1 for my
>> testing.
>
> Unfortunately I just have LAN8651 Rev. B0 Chips. Are you sure that the Rev B1 has the
> issue fixed? The errata here says that B1 is affected too:
> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/Errata/LAN8650-1-Errata-80001075.pdf
As per my knowledge it is fixed in the Rev.B1 but as you said errata
says the issue persists in both revisions. Let me check internally and
get back to you on this. But it is always recommended to use Rev.B1
rather Rev.B0. If possible, I would suggest to use the latest one.
Best regards,
Parthiban V
>
>>
>> I should be able to reproduce the "receive buffer overflow" issue and
>> consequently kernel crash in my setup with LAN8651 Rev.B1 so that I can
>> investigate the issue further. As I am not able to reproduce in my RPI
>> 4, I need your support for the tests and applications you used in your
>> setup.
>>>
>>> Is there a reason why you removed the netdev watchdog which was active in v2?
>> When the timeout occurs, there is no further action except increasing
>> tx_errors. Not seeing this except USB-to-Ethernet which can be removed
>> unexpectedly. But this is SPI interface which will not be removed
>> unexpectedly as it is a platform device. That's why we removed this.
>>
>> Best regards,
>> Parthiban V
>>>
>>> Thanks,
>>> Benjamin Bigler
>>>
>>
>
> Thanks,
> Benjamin Bigler
>