2023-10-23 15:48:02

by Parthiban Veerasooran

[permalink] [raw]
Subject: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

Implement register read/write interface according to the control
communication specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
Interface document. Control transactions consist of one or more control
commands. Control commands are used by the SPI host to read and write
registers within the MAC-PHY. Each control commands are composed of a
32-bit control command header followed by register data.

Control write commands can write either a single register or multiple
consecutive registers. When multiple consecutive registers are written,
the address is automatically post-incremented by the MAC-PHY. The write
command and data is also echoed from the MAC-PHY back to the SPI host to
enable the SPI host to identify which register write failed in the case
of any bus errors.

Control read commands can read either a single register or multiple
consecutive registers. When multiple consecutive registers are read, the
address is automatically post-incremented by the MAC-PHY.

The register data being read or written can be protected against simple
bit errors. When enabled by setting the Protection Enable (PROTE) bit in
the CONFIG0 register, protection is accomplished by duplication of each
32-bit word containing register data with its ones’ complement. Errors
are detected at the receiver by performing a simple exclusive-OR (XOR) of
each received 32-bit word containing register data with its received
complement and detecting if there are any zeros in the result.

Signed-off-by: Parthiban Veerasooran <[email protected]>
---
Documentation/networking/oa-tc6-framework.rst | 233 ++++++++++++++
MAINTAINERS | 8 +
drivers/net/ethernet/Kconfig | 12 +
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/oa_tc6.c | 288 ++++++++++++++++++
include/linux/oa_tc6.h | 28 ++
6 files changed, 570 insertions(+)
create mode 100644 Documentation/networking/oa-tc6-framework.rst
create mode 100644 drivers/net/ethernet/oa_tc6.c
create mode 100644 include/linux/oa_tc6.h

diff --git a/Documentation/networking/oa-tc6-framework.rst b/Documentation/networking/oa-tc6-framework.rst
new file mode 100644
index 000000000000..5406f6f4b4f1
--- /dev/null
+++ b/Documentation/networking/oa-tc6-framework.rst
@@ -0,0 +1,233 @@
+.. SPDX-License-Identifier: GPL-2.0+
+.. include:: <isonum.txt>
+
+=========================================================================
+OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface (TC6) Framework Support
+=========================================================================
+
+:Copyright: |copy| 2023 MICROCHIP
+
+Introduction
+------------
+
+The IEEE 802.3cg project defines two 10 Mbit/s PHYs operating over a
+single pair of conductors. The 10BASE-T1L (Clause 146) is a long reach
+PHY supporting full duplex point-to-point operation over 1 km of single
+balanced pair of conductors. The 10BASE-T1S (Clause 147) is a short reach
+PHY supporting full / half duplex point-to-point operation over 15 m of
+single balanced pair of conductors, or half duplex multidrop bus
+operation over 25 m of single balanced pair of conductors.
+
+Furthermore, the IEEE 802.3cg project defines the new Physical Layer
+Collision Avoidance (PLCA) Reconciliation Sublayer (Clause 148) meant to
+provide improved determinism to the CSMA/CD media access method. PLCA
+works in conjunction with the 10BASE-T1S PHY operating in multidrop mode.
+
+The aforementioned PHYs are intended to cover the low-speed / low-cost
+applications in industrial and automotive environment. The large number
+of pins (16) required by the MII interface, which is specified by the
+IEEE 802.3 in Clause 22, is one of the major cost factors that need to be
+addressed to fulfil this objective.
+
+The MAC-PHY solution integrates an IEEE Clause 4 MAC and a 10BASE-T1x PHY
+exposing a low pin count Serial Peripheral Interface (SPI) to the host
+microcontroller. This also enables the addition of Ethernet functionality
+to existing low-end microcontrollers which do not integrate a MAC
+controller.
+
+Overview
+--------
+
+The MAC-PHY is specified to carry both data (Ethernet frames) and control
+(register access) transactions over a single full-duplex serial
+peripheral interface.
+
+Protocol Overview
+-----------------
+
+Two types of transactions are defined in the protocol: data transactions
+for Ethernet frame transfers and control transactions for register
+read/write transfers. A chunk is the basic element of data transactions
+and is composed of 4 bytes of overhead plus the configured payload size
+for each chunk. Ethernet frames are transferred over one or more data
+chunks. Control transactions consist of one or more register read/write
+control commands.
+
+SPI transactions are initiated by the SPI host with the assertion of CSn
+low to the MAC-PHY and ends with the deassertion of CSn high. In between
+each SPI transaction, the SPI host may need time for additional
+processing and to setup the next SPI data or control transaction.
+
+SPI data transactions consist of an equal number of transmit (TX) and
+receive (RX) chunks. Chunks in both transmit and receive directions may
+or may not contain valid frame data independent from each other, allowing
+for the simultaneous transmission and reception of different length
+frames.
+
+Each transmit data chunk begins with a 32-bit data header followed by a
+data chunk payload on MOSI. The data header indicates whether transmit
+frame data is present and provides the information to determine which
+bytes of the payload contain valid frame data.
+
+In parallel, receive data chunks are received on MISO. Each receive data
+chunk consists of a data chunk payload ending with a 32-bit data footer.
+The data footer indicates if there is receive frame data present within
+the payload or not and provides the information to determine which bytes
+of the payload contain valid frame data.
+
+Reference
+---------
+
+10BASE-T1x MAC-PHY Serial Interface Specification,
+
+Link: https://www.opensig.org/about/specifications/
+
+Hardware Architecture
+---------------------
+
+.. code-block:: none
+
+ +-------------------------------------+
+ | MAC-PHY |
+ +----------+ | +-----------+ +-------+ +-------+ |
+ | SPI Host |<---->| | SPI Slave | | MAC | | PHY | |
+ +----------+ | +-----------+ +-------+ +-------+ |
+ +-------------------------------------+
+
+Software Architecture
+---------------------
+
+.. code-block:: none
+
+ +----------------------------------------------------------+
+ | Networking Subsystem |
+ +----------------------------------------------------------+
+ | |
+ | |
+ +----------------------+ +-----------------------------+
+ | MAC Driver |<--->| OPEN Alliance TC6 Framework |
+ +----------------------+ +-----------------------------+
+ | |
+ | |
+ +----------------------+ +-----------------------------+
+ | PHYLIB | | SPI Subsystem |
+ +----------------------+ +-----------------------------+
+ |
+ |
+ +----------------------------------------------------------+
+ | 10BASE-T1x MAC-PHY Device |
+ +----------------------------------------------------------+
+
+Implementation
+--------------
+
+MAC Driver
+~~~~~~~~~~
+- Initializes and configures the OA TC6 framework for the MAC-PHY.
+
+- Initializes PHYLIB interface.
+
+- Registers and configures the network device.
+
+- Sends the tx ethernet frame from n/w subsystem to OA TC6 framework.
+
+OPEN Alliance TC6 Framework
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+- Registers macphy interrupt which is used to indicate receive data
+ available, any communication errors and tx credit count availability in
+ case it was 0 already.
+
+- Prepares SPI chunks from the tx ethernet frame enqueued by the MAC
+ driver and sends to MAC-PHY.
+
+- Receives SPI chunks from MAC-PHY and prepares ethernet frame and sends
+ to n/w subsystem.
+
+- Prepares and performs control read/write commands.
+
+Data Transaction
+~~~~~~~~~~~~~~~~
+
+Tx ethernet frame from the n/w layer is divided into multiple chunks in
+the oa_tc6_prepare_tx_chunks() function. Each tx chunk will have 4 bytes
+header and 64/32 bytes chunk payload.
+
+.. code-block:: none
+
+ +---------------------------------------------------+
+ | Tx Chunk |
+ | +---------------------------+ +----------------+ | MOSI
+ | | 64/32 bytes chunk payload | | 4 bytes header | |------------>
+ | +---------------------------+ +----------------+ |
+ +---------------------------------------------------+
+
+The number of buffers available in the MAC-PHY to store the incoming tx
+chunk payloads is represented as tx credit count (txc). This txc can be
+read either from the Buffer Status Register or footer (Refer below in the
+rx case for the footer info) received from the MAC-PHY. The number of txc
+is needed to transport the ethernet frame is calculated from the size of
+the ethernet frame. The header in the each chunk is updated with the
+chunk payload details like data not control, data valid, start valid,
+start word offset, end byte offset, end valid and parity bit.
+
+Once the tx chunks are ready, oa_tc6_handler() task is triggered to
+perform SPI transfer. This task checks for the txc availability in the
+MAC-PHY and sends the number of chunks equal to the txc. If there is no
+txc is available then the task waits for the interrupt to indicate the
+txc availability. If the available txc is less than the needed txc then
+the SPI transfer is performed for the available txc and the rx chunks
+footer is processed for the txc availability again. For every SPI
+transfer the received rx chunks will be processed for the rx ethernet
+frame (if any), txc and rca.
+
+Rx ethernet frame from the MAC-PHY is framed from the rx chunks received
+from the MAC-PHY and will be transferred to the n/w layer. Each rx chunk
+will have 64/32 bytes chunk payload and 4 bytes footer.
+
+.. code-block:: none
+
+ +---------------------------------------------------+
+ | Rx Chunk |
+ | +----------------+ +---------------------------+ | MISO
+ | | 4 bytes footer | | 64/32 bytes chunk payload | |------------>
+ | +----------------+ +---------------------------+ |
+ +---------------------------------------------------+
+
+In case of interrupt, the oa_tc6_handler() task performs an empty chunk
+SPI transfer to get the reason for the interrupt in the received chunk
+footer. The reason can be receive chunk available (rca) or extended block
+status (exst) or txc availability. Based on this the corresponding
+operation is performed. If it is for rca then the SPI transfer is
+performed with the empty chunks equal to the rca to get the rx chunks.
+Rx ethernet frame is framed from the rx chunks received and transferred
+to n/w layer. If it is for exst then the STATUS0 register will be read
+for the error detail.
+
+In the beginning the interrupt occurs for indicating the reset complete
+from the MAC-PHY and is ready for the configuration. oa_tc6_handler() task
+handles this interrupt to get and clear the reset complete status.
+
+Control Transaction
+~~~~~~~~~~~~~~~~~~~
+
+Control transactions are performed to read/write the registers in the
+MAC-PHY. Each control command headers are 4 bytes length with the
+necessary details to read/write the registers.
+
+In case of a register write command, the write command header has the
+information like data not control, write not read, address increment
+disable, memory map selector, address, length and parity followed by the
+data to be written. If the protected mode is enabled in the CONFIG0
+register then each data to be written will be sent first followed by its
+ones' complement value to ensure the error free transfer. For every write
+command, both the write command header and the data will be echoed back in
+the rx path to confirm the error free transaction.
+
+In case of a register read command, the read command is preferred with the
+above mentioned details and the echoed rx data will be processed for the
+register data. In case of protected mode enabled the echoed rx data
+contains the ones' complemented data also for verifying the data error.
+
+oa_tc6_perform_ctrl() function prepares control commands based on the
+read/write request, performs SPI transfer, checks for the error and
+returns read register data in case of control read command.
diff --git a/MAINTAINERS b/MAINTAINERS
index 8985a1b0b5ee..1c165026bbd4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15971,6 +15971,14 @@ L: [email protected]
S: Supported
F: drivers/infiniband/ulp/opa_vnic

+OPEN ALLIANCE 10BASE-T1S MACPHY SERIAL INTERFACE FRAMEWORK
+M: Parthiban Veerasooran <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/networking/oa-tc6-framework.rst
+F: drivers/include/linux/oa_tc6.h
+F: drivers/net/ethernet/oa_tc6.c
+
OPEN FIRMWARE AND FLATTENED DEVICE TREE
M: Rob Herring <[email protected]>
M: Frank Rowand <[email protected]>
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 5a274b99f299..c58c46591baf 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -160,6 +160,18 @@ config ETHOC
help
Say Y here if you want to use the OpenCores 10/100 Mbps Ethernet MAC.

+config OA_TC6
+ tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support"
+ depends on SPI
+ select PHYLIB
+ help
+ This library implements OPEN Alliance TC6 10BASE-T1x MAC-PHY
+ Serial Interface protocol for supporting 10BASE-T1x MAC-PHYs.
+
+ This option is provided for the case where no in-kernel-tree modules
+ require OA_TC6 functions, but a module built outside the kernel tree
+ does. Such modules that use library OA_TC6 functions require M here.
+
source "drivers/net/ethernet/packetengines/Kconfig"
source "drivers/net/ethernet/pasemi/Kconfig"
source "drivers/net/ethernet/pensando/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 0d872d4efcd1..cf5487fc0761 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -104,3 +104,4 @@ obj-$(CONFIG_NET_VENDOR_XILINX) += xilinx/
obj-$(CONFIG_NET_VENDOR_XIRCOM) += xircom/
obj-$(CONFIG_NET_VENDOR_SYNOPSYS) += synopsys/
obj-$(CONFIG_NET_VENDOR_PENSANDO) += pensando/
+obj-$(CONFIG_OA_TC6) += oa_tc6.o
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
new file mode 100644
index 000000000000..acedc327b05e
--- /dev/null
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface framework
+ *
+ * Author: Parthiban Veerasooran <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/oa_tc6.h>
+
+/* Opaque structure for MACPHY drivers */
+struct oa_tc6 {
+ struct spi_device *spi;
+ u8 *ctrl_tx_buf;
+ u8 *ctrl_rx_buf;
+ bool prote;
+};
+
+static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, u16 len)
+{
+ struct spi_transfer xfer = {
+ .tx_buf = ptx,
+ .rx_buf = prx,
+ .len = len,
+ };
+ struct spi_message msg;
+
+ spi_message_init(&msg);
+ spi_message_add_tail(&xfer, &msg);
+
+ return spi_sync(spi, &msg);
+}
+
+static int oa_tc6_get_parity(u32 p)
+{
+ /* Public domain code snippet, lifted from
+ * http://www-graphics.stanford.edu/~seander/bithacks.html
+ */
+ p ^= p >> 1;
+ p ^= p >> 2;
+ p = (p & 0x11111111U) * 0x11111111U;
+
+ /* Odd parity is used here */
+ return !((p >> 28) & 1);
+}
+
+static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
+ u8 len, bool wnr, u8 *buf, bool prote)
+{
+ u32 hdr;
+
+ /* Prepare the control header with the required details */
+ hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
+ FIELD_PREP(CTRL_HDR_WNR, wnr) |
+ FIELD_PREP(CTRL_HDR_AID, 0) |
+ FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
+ FIELD_PREP(CTRL_HDR_ADDR, addr) |
+ FIELD_PREP(CTRL_HDR_LEN, len - 1);
+ hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
+ *(__be32 *)buf = cpu_to_be32(hdr);
+
+ if (wnr) {
+ for (u8 i = 0; i < len; i++) {
+ u16 pos;
+
+ if (!prote) {
+ /* Send the value to be written followed by the
+ * header.
+ */
+ pos = (i + 1) * TC6_HDR_SIZE;
+ *(__be32 *)&buf[pos] = cpu_to_be32(val[i]);
+ } else {
+ /* If protected then send complemented value
+ * also followed by actual value.
+ */
+ pos = TC6_HDR_SIZE + (i * (TC6_HDR_SIZE * 2));
+ *(__be32 *)&buf[pos] = cpu_to_be32(val[i]);
+ pos = (i + 1) * (TC6_HDR_SIZE * 2);
+ *(__be32 *)&buf[pos] = cpu_to_be32(~val[i]);
+ }
+ }
+ }
+}
+
+static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
+ bool wnr, bool prote)
+{
+ /* 1st 4 bytes of rx chunk data can be discarded */
+ u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
+ u32 tx_hdr = *(u32 *)ptx;
+ u32 rx_data_complement;
+ u32 tx_data;
+ u32 rx_data;
+ u16 pos1;
+ u16 pos2;
+
+ /* If tx hdr and echoed hdr are not equal then there might be an issue
+ * with the connection between SPI host and MAC-PHY. Here this case is
+ * considered as MAC-PHY is not connected.
+ */
+ if (tx_hdr != rx_hdr)
+ return -ENODEV;
+
+ if (wnr) {
+ if (!prote) {
+ /* In case of ctrl write, both tx data & echoed
+ * data are compared for the error.
+ */
+ pos1 = TC6_HDR_SIZE;
+ pos2 = TC6_HDR_SIZE * 2;
+ for (u8 i = 0; i < len; i++) {
+ tx_data = *(u32 *)&ptx[pos1 + (i * TC6_HDR_SIZE)];
+ rx_data = *(u32 *)&prx[pos2 + (i * TC6_HDR_SIZE)];
+ if (tx_data != rx_data)
+ return -ENODEV;
+ }
+ return 0;
+ }
+ } else {
+ if (!prote)
+ return 0;
+ }
+
+ /* In case of ctrl read or ctrl write in protected mode, the rx data and
+ * the complement of rx data are compared for the error.
+ */
+ pos1 = TC6_HDR_SIZE * 2;
+ pos2 = TC6_HDR_SIZE * 3;
+ for (u8 i = 0; i < len; i++) {
+ rx_data = *(u32 *)&prx[pos1 + (i * TC6_HDR_SIZE * 2)];
+ rx_data_complement = *(u32 *)&prx[pos2 + (i * TC6_HDR_SIZE * 2)];
+ if (rx_data != ~rx_data_complement)
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+/**
+ * oa_tc6_perform_ctrl - function to perform control transaction.
+ * @tc6: oa_tc6 struct.
+ * @addr: register address of the MACPHY to be written/read.
+ * @val: value to be written/read in the @addr register address of the MACPHY.
+ * @len: number of consecutive registers to be written/read from @addr.
+ * @wnr: operation to be performed for the register @addr (write not read).
+ * @prote: control data (register) read/write protection enable/disable.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
+ bool wnr, bool prote)
+{
+ u8 *tx_buf = tc6->ctrl_tx_buf;
+ u8 *rx_buf = tc6->ctrl_rx_buf;
+ u16 size;
+ u16 pos;
+ int ret;
+
+ if (prote)
+ size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
+ else
+ size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);
+
+ /* Prepare control command */
+ oa_tc6_prepare_ctrl_buf(tc6, addr, val, len, wnr, tx_buf, prote);
+
+ /* Perform SPI transfer */
+ ret = oa_tc6_spi_transfer(tc6->spi, tx_buf, rx_buf, size);
+ if (ret)
+ return ret;
+
+ /* Check echoed/received control reply for errors */
+ ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, prote);
+ if (ret)
+ return ret;
+
+ if (!wnr) {
+ /* Copy read data from the rx data in case of ctrl read */
+ for (u8 i = 0; i < len; i++) {
+ if (!prote) {
+ pos = (TC6_HDR_SIZE * 2) + (i * TC6_HDR_SIZE);
+ val[i] = be32_to_cpu(*(__be32 *)&rx_buf[pos]);
+ } else {
+ pos = (TC6_HDR_SIZE * 2) +
+ (i * (TC6_HDR_SIZE * 2));
+ val[i] = be32_to_cpu(*(__be32 *)&rx_buf[pos]);
+ }
+ }
+ }
+
+ return ret;
+}
+
+/**
+ * oa_tc6_write_register - function for writing a MACPHY register.
+ * @tc6: oa_tc6 struct.
+ * @addr: register address of the MACPHY to be written.
+ * @val: value to be written in the @addr register address of the MACPHY.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val)
+{
+ return oa_tc6_perform_ctrl(tc6, addr, &val, 1, true, tc6->prote);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_write_register);
+
+/**
+ * oa_tc6_read_register - function for reading a MACPHY register.
+ * @tc6: oa_tc6 struct.
+ * @addr: register address of the MACPHY to be read.
+ * @val: value read from the @addr register address of the MACPHY.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 *val)
+{
+ return oa_tc6_perform_ctrl(tc6, addr, val, 1, false, tc6->prote);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_read_register);
+
+/**
+ * oa_tc6_write_registers - function for writing multiple consecutive registers.
+ * @tc6: oa_tc6 struct.
+ * @addr: address of the first register to be written in the MACPHY.
+ * @val: values to be written from the starting register address @addr.
+ * @len: number of consecutive registers to be written from @addr.
+ *
+ * Maximum of 128 consecutive registers can be written starting at @addr.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len)
+{
+ return oa_tc6_perform_ctrl(tc6, addr, val, len, true, tc6->prote);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_write_registers);
+
+/**
+ * oa_tc6_read_registers - function for reading multiple consecutive registers.
+ * @tc6: oa_tc6 struct.
+ * @addr: address of the first register to be read in the MACPHY.
+ * @val: values to be read from the starting register address @addr.
+ *
+ * Maximum of 128 consecutive registers can be read starting at @addr.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len)
+{
+ return oa_tc6_perform_ctrl(tc6, addr, val, len, false, tc6->prote);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
+
+/**
+ * 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.
+ *
+ * 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 *tc6;
+
+ tc6 = devm_kzalloc(&spi->dev, sizeof(*tc6), GFP_KERNEL);
+ if (!tc6)
+ return NULL;
+
+ tc6->ctrl_tx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
+ if (!tc6->ctrl_tx_buf)
+ return NULL;
+
+ 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->prote = prote;
+
+ return tc6;
+}
+EXPORT_SYMBOL_GPL(oa_tc6_init);
+
+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
new file mode 100644
index 000000000000..6284828bda42
--- /dev/null
+++ b/include/linux/oa_tc6.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface framework
+ *
+ * Author: Parthiban Veerasooran <[email protected]>
+ */
+
+#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 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 */
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote);
+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);
--
2.34.1


2023-10-23 21:29:22

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
> + u8 len, bool wnr, u8 *buf, bool prote)

One of the comments i made last time was that wnr is not obvious. I
assume it means write-not-read. So why not just write? Since it a
boolean, i assume war is never needed, so !wnr cal always be
considered rnw.

And prote could well be protect, the two extra characters make it a
lot more obvious. Or better still.

> +{
> + u32 hdr;
> +
> + /* Prepare the control header with the required details */
> + hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
> + FIELD_PREP(CTRL_HDR_WNR, wnr) |
> + FIELD_PREP(CTRL_HDR_AID, 0) |
> + FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
> + FIELD_PREP(CTRL_HDR_ADDR, addr) |
> + FIELD_PREP(CTRL_HDR_LEN, len - 1);
> + hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
> + *(__be32 *)buf = cpu_to_be32(hdr);
> +
> + if (wnr) {
> + for (u8 i = 0; i < len; i++) {
> + u16 pos;
> +
> + if (!prote) {

nitpick, but please use positive logic. Do the protected case first.

Andrew

2023-10-23 23:10:47

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

Hi Parthiban,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url: https://github.com/intel-lab-lkp/linux/commits/Parthiban-Veerasooran/net-ethernet-implement-OPEN-Alliance-control-transaction-interface/20231023-235310
base: net-next/main
patch link: https://lore.kernel.org/r/20231023154649.45931-2-Parthiban.Veerasooran%40microchip.com
patch subject: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20231024/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231024/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/oa_tc6.c:250: warning: Function parameter or member 'len' not described in 'oa_tc6_read_registers'


vim +250 drivers/net/ethernet/oa_tc6.c

238
239 /**
240 * oa_tc6_read_registers - function for reading multiple consecutive registers.
241 * @tc6: oa_tc6 struct.
242 * @addr: address of the first register to be read in the MACPHY.
243 * @val: values to be read from the starting register address @addr.
244 *
245 * Maximum of 128 consecutive registers can be read starting at @addr.
246 *
247 * Returns 0 on success otherwise failed.
248 */
249 int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len)
> 250 {
251 return oa_tc6_perform_ctrl(tc6, addr, val, len, false, tc6->prote);
252 }
253 EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
254

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-10-25 11:17:12

by Parthiban Veerasooran

[permalink] [raw]
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

Hi Andrew,

Thanks for reviewing my patches.

On 24/10/23 2:58 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>> + u8 len, bool wnr, u8 *buf, bool prote)
>
> One of the comments i made last time was that wnr is not obvious. I
> assume it means write-not-read. So why not just write? Since it a
> boolean, i assume war is never needed, so !wnr cal always be
> considered rnw.
>
> And prote could well be protect, the two extra characters make it a
> lot more obvious. Or better still.
Just to be aligned with the naming in the OPEN Alliance specification
document, used the same name as it is. wnr and prote are taken from the
specification document. Please refer the screenshots attached here from
the specification document.

Still if you feel like using "write" instead of "wnr" and "protect"
instead of "prote", I will change them in the next revision.
>
>> +{
>> + u32 hdr;
>> +
>> + /* Prepare the control header with the required details */
>> + hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>> + FIELD_PREP(CTRL_HDR_WNR, wnr) |
>> + FIELD_PREP(CTRL_HDR_AID, 0) |
>> + FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>> + FIELD_PREP(CTRL_HDR_ADDR, addr) |
>> + FIELD_PREP(CTRL_HDR_LEN, len - 1);
>> + hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>> + *(__be32 *)buf = cpu_to_be32(hdr);
>> +
>> + if (wnr) {
>> + for (u8 i = 0; i < len; i++) {
>> + u16 pos;
>> +
>> + if (!prote) {
>
> nitpick, but please use positive logic. Do the protected case first.
Sure, will do it in the next revision.

Best Regards,
Parthiban V
>
> Andrew
>


Attachments:
wnr.png (106.38 kB)
wnr.png
prote.png (159.71 kB)
prote.png
Download all attachments

2023-10-25 19:10:13

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

Hi Parthiban,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url: https://github.com/intel-lab-lkp/linux/commits/Parthiban-Veerasooran/net-ethernet-implement-OPEN-Alliance-control-transaction-interface/20231023-235310
base: net-next/main
patch link: https://lore.kernel.org/r/20231023154649.45931-2-Parthiban.Veerasooran%40microchip.com
patch subject: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface
reproduce: (https://download.01.org/0day-ci/archive/20231026/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> Documentation/networking/oa-tc6-framework.rst: WARNING: document isn't included in any toctree

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-10-26 19:47:02

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

> Still if you feel like using "write" instead of "wnr" and "protect"
> instead of "prote", I will change them in the next revision.

There is some value in using names from the standard, if they are
actually good names. But i guess most developers don't have a copy of
the standard by there side.

You actually wrote in the patch:

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

The comments suggest you also don't think the names are particularly
good, otherwise you would not of added comments.

But if you instead had:

/* Control header */
#define CTRL_HDR_DATA_NOT_CTRL BIT(31)
#define CTRL_HDR_HDR_RX_BAD BIT(30)
#define CTRL_HDR_WRITE BIT(29)
#define CTRL_HDR_ADDR_INC_DISABLE BIT(28)
#define CTRL_HDR_MEM_MAP_SELECTOR GENMASK(27, 24)

the names are probably sufficient that comments are not needed. And
is should be easy for somebody to map these back to the names used in
the standard.

This also to some extent comes into the comment about coding style, a
function does one thing, is short, etc. Short functions tend to have
less indentation, meaning you can use longer names. And longer names
are more readable, making the function easier to understand, so it
does that one thing well.

Andrew

2023-10-27 07:16:19

by Parthiban Veerasooran

[permalink] [raw]
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance control transaction interface

Hi Andrew,

On 27/10/23 1:16 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> Still if you feel like using "write" instead of "wnr" and "protect"
>> instead of "prote", I will change them in the next revision.
>
> There is some value in using names from the standard, if they are
> actually good names. But i guess most developers don't have a copy of
> the standard by there side.
>
> You actually wrote in the patch:
>
> +/* 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 */
>
> The comments suggest you also don't think the names are particularly
> good, otherwise you would not of added comments.
>
> But if you instead had:
>
> /* Control header */
> #define CTRL_HDR_DATA_NOT_CTRL BIT(31)
> #define CTRL_HDR_HDR_RX_BAD BIT(30)
> #define CTRL_HDR_WRITE BIT(29)
> #define CTRL_HDR_ADDR_INC_DISABLE BIT(28)
> #define CTRL_HDR_MEM_MAP_SELECTOR GENMASK(27, 24)
>
> the names are probably sufficient that comments are not needed. And
> is should be easy for somebody to map these back to the names used in
> the standard.
>
> This also to some extent comes into the comment about coding style, a
> function does one thing, is short, etc. Short functions tend to have
> less indentation, meaning you can use longer names. And longer names
> are more readable, making the function easier to understand, so it
> does that one thing well.
Ok, thanks for the explanation. I will do it in the next revision.

Best Regards,
Parthiban V
>
> Andrew
>