This patch series contain the below updates,
- Adds support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface in the
net/ethernet/oa_tc6.c.
Link to the spec:
-----------------
https://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf
- Adds driver support for Microchip LAN8650/1 Rev.B1 10BASE-T1S MACPHY
Ethernet driver in the net/ethernet/microchip/lan865x/lan865x.c.
Link to the product:
--------------------
https://www.microchip.com/en-us/product/lan8650
Testing Details:
----------------
The driver performance was tested using iperf3 in the below two setups
separately.
Setup 1:
--------
Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY
Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
Setup 2:
--------
Node 0 - SAMA7G54-EK with LAN8650 MAC-PHY
Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
Achieved maximum of 9.4 Mbps.
Some systems like Raspberry Pi 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
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.
v3:
- Completely redesigned all the patches.
- Control and data interface patches are divided into multiple small
patches.
- Device driver APIs added in the oa-tc6-framework.rst file.
- Code readability improved in all the patches.
- Defined macros wherever is possible.
- Changed RESETC to STATUS0_RESETC for improving the readability.
- Removed OA specific DT bindings.
- Used default configurations defined in the OA spec.
- All variables are named properly as per OA spec for more redability.
- Bigger functions are split into multiple smaller functions.
- DT binding check is done.
- Phy mask is removed in phy scanning.
- Used NET_RX_DROP to compare the rx packet submission status.
- Indentation in the Kconfig file corrected.
- Removed CONFIG_OF and CONFIG_ACPI ifdefs.
- Removed MODULE_ALIAS().
Parthiban Veerasooran (12):
Documentation: networking: add OPEN Alliance 10BASE-T1x MAC-PHY serial
interface
net: ethernet: oa_tc6: implement register write operation
net: ethernet: oa_tc6: implement register read operation
net: ethernet: oa_tc6: implement software reset
net: ethernet: oa_tc6: implement error interrupts unmasking
net: ethernet: oa_tc6: implement internal PHY initialization
net: ethernet: oa_tc6: enable open alliance tc6 data communication
net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet
frames
net: ethernet: oa_tc6: implement receive path to receive rx ethernet
frames
net: ethernet: oa_tc6: implement mac-phy interrupt
microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY
dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY
.../bindings/net/microchip,lan865x.yaml | 80 ++
Documentation/networking/oa-tc6-framework.rst | 491 +++++++
MAINTAINERS | 15 +
drivers/net/ethernet/Kconfig | 15 +
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/microchip/Kconfig | 1 +
drivers/net/ethernet/microchip/Makefile | 1 +
.../net/ethernet/microchip/lan865x/Kconfig | 19 +
.../net/ethernet/microchip/lan865x/Makefile | 6 +
.../net/ethernet/microchip/lan865x/lan865x.c | 350 +++++
drivers/net/ethernet/oa_tc6.c | 1235 +++++++++++++++++
include/linux/oa_tc6.h | 23 +
12 files changed, 2237 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml
create mode 100644 Documentation/networking/oa-tc6-framework.rst
create mode 100644 drivers/net/ethernet/microchip/lan865x/Kconfig
create mode 100644 drivers/net/ethernet/microchip/lan865x/Makefile
create mode 100644 drivers/net/ethernet/microchip/lan865x/lan865x.c
create mode 100644 drivers/net/ethernet/oa_tc6.c
create mode 100644 include/linux/oa_tc6.h
--
2.34.1
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.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
Documentation/networking/oa-tc6-framework.rst | 491 ++++++++++++++++++
MAINTAINERS | 6 +
2 files changed, 497 insertions(+)
create mode 100644 Documentation/networking/oa-tc6-framework.rst
diff --git a/Documentation/networking/oa-tc6-framework.rst b/Documentation/networking/oa-tc6-framework.rst
new file mode 100644
index 000000000000..d8150a26e499
--- /dev/null
+++ b/Documentation/networking/oa-tc6-framework.rst
@@ -0,0 +1,491 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+=========================================================================
+OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface (TC6) Framework Support
+=========================================================================
+
+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 64 bytes of 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://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf
+
+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 |
+ +----------------------+ +-----------------------------+
+ / \ / \
+ | |
+ | |
+ | \ /
+ +----------------------------------------------------------+
+ | SPI Subsystem |
+ +----------------------------------------------------------+
+ / \
+ |
+ |
+ \ /
+ +----------------------------------------------------------+
+ | 10BASE-T1x MAC-PHY Device |
+ +----------------------------------------------------------+
+
+Implementation
+--------------
+
+MAC Driver
+~~~~~~~~~~
+
+- Probed by SPI subsystem.
+
+- Initializes OA TC6 framework for the MAC-PHY.
+
+- Registers and configures the network device.
+
+- Sends the tx ethernet frames from n/w subsystem to OA TC6 framework.
+
+OPEN Alliance TC6 Framework
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- Initializes PHYLIB interface.
+
+- Registers mac-phy interrupt.
+
+- Performs mac-phy register read/write operation using the control
+ transaction protocol specified in the OPEN Alliance 10BASE-T1x MAC-PHY
+ Serial Interface specification.
+
+- Performs Ethernet frames transaction using the data transaction protocol
+ for Ethernet frames specified in the OPEN Alliance 10BASE-T1x MAC-PHY
+ Serial Interface specification.
+
+- Forwards the received Ethernet frame from 10Base-T1x MAC-PHY to n/w
+ subsystem.
+
+Data Transaction
+~~~~~~~~~~~~~~~~
+
+The Ethernet frames that are typically transferred from the SPI host to
+the MAC-PHY will be converted into multiple transmit data chunks. Each
+transmit data chunk will have a 4 bytes header which contains the
+information needed to determine the validity and the location of the
+transmit frame data within the 64 bytes data chunk payload.
+
+.. code-block:: none
+
+ +---------------------------------------------------+
+ | Tx Chunk |
+ | +---------------------------+ +----------------+ | MOSI
+ | | 64 bytes chunk payload | | 4 bytes header | |------------>
+ | +---------------------------+ +----------------+ |
+ +---------------------------------------------------+
+
+4 bytes header contains the below fields,
+
+DNC (Bit 31) - Data-Not-Control flag. This flag specifies the type of SPI
+ transaction. For TX data chunks, this bit shall be ’1’.
+ 0 - Control command
+ 1 - Data chunk
+
+SEQ (Bit 30) - Data Chunk Sequence. This bit is used to indicate an
+ even/odd transmit data chunk sequence to the MAC-PHY.
+
+NORX (Bit 29) - No Receive flag. The SPI host may set this bit to prevent
+ the MAC-PHY from conveying RX data on the MISO for the
+ current chunk (DV = 0 in the footer), indicating that the
+ host would not process it. Typically, the SPI host should
+ set NORX = 0 indicating that it will accept and process
+ any receive frame data within the current chunk.
+
+RSVD (Bit 28..24) - Reserved: All reserved bits shall be ‘0’.
+
+VS (Bit 23..22) - Vendor Specific. These bits are implementation specific.
+ If the MAC-PHY does not implement these bits, the host
+ shall set them to ‘0’.
+
+DV (Bit 21) - Data Valid flag. The SPI host uses this bit to indicate
+ whether the current chunk contains valid transmit frame data
+ (DV = 1) or not (DV = 0). When ‘0’, the MAC-PHY ignores the
+ chunk payload. Note that the receive path is unaffected by
+ the setting of the DV bit in the data header.
+
+SV (Bit 20) - Start Valid flag. The SPI host shall set this bit when the
+ beginning of an Ethernet frame is present in the current
+ transmit data chunk payload. Otherwise, this bit shall be
+ zero. This bit is not to be confused with the Start-of-Frame
+ Delimiter (SFD) byte described in IEEE 802.3 [2].
+
+SWO (Bit 19..16) - Start Word Offset. When SV = 1, this field shall
+ contain the 32-bit word offset into the transmit data
+ chunk payload that points to the start of a new
+ Ethernet frame to be transmitted. The host shall write
+ this field as zero when SV = 0.
+
+RSVD (Bit 15) - Reserved: All reserved bits shall be ‘0’.
+
+EV (Bit 14) - End Valid flag. The SPI host shall set this bit when the end
+ of an Ethernet frame is present in the current transmit data
+ chunk payload. Otherwise, this bit shall be zero.
+
+EBO (Bit 13..8) - End Byte Offset. When EV = 1, this field shall contain
+ the byte offset into the transmit data chunk payload
+ that points to the last byte of the Ethernet frame to
+ transmit. This field shall be zero when EV = 0.
+
+TSC (Bit 7..6) - Timestamp Capture. Request a timestamp capture when the
+ frame is transmitted onto the network.
+ 00 - Do not capture a timestamp
+ 01 - Capture timestamp into timestamp capture register A
+ 10 - Capture timestamp into timestamp capture register B
+ 11 - Capture timestamp into timestamp capture register C
+
+RSVD (Bit 5..1) - Reserved: All reserved bits shall be ‘0’.
+
+P (Bit 0) - Parity. Parity bit calculated over the transmit data header.
+ Method used is odd parity.
+
+The number of buffers available in the MAC-PHY to store the incoming
+transmit data chunk payloads is represented as transmit credits. The
+available transmit credits in the MAC-PHY can be read either from the
+Buffer Status Register or footer (Refer below for the footer info)
+received from the MAC-PHY. The SPI host should not write more data chunks
+than the available transmit credits as this will lead to transmit buffer
+overflow error.
+
+In case the previous data footer had no transmit credits available and
+once the transmit credits become available for transmitting transmit data
+chunks, the MAC-PHY interrupt is asserted to SPI host. On reception of the
+first data header this interrupt will be deasserted and the received
+footer for the first data chunk will have the transmit credits available
+information.
+
+The Ethernet frames that are typically transferred from MAC-PHY to SPI
+host will be sent as multiple receive data chunks. Each receive data
+chunk will have 64 bytes of data chunk payload followed by 4 bytes footer
+which contains the information needed to determine the validity and the
+location of the receive frame data within the 64 bytes data chunk payload.
+
+.. code-block:: none
+
+ +---------------------------------------------------+
+ | Rx Chunk |
+ | +----------------+ +---------------------------+ | MISO
+ | | 4 bytes footer | | 64 bytes chunk payload | |------------>
+ | +----------------+ +---------------------------+ |
+ +---------------------------------------------------+
+
+4 bytes footer contains the below fields,
+
+EXST (Bit 31) - Extended Status. This bit is set when any bit in the
+ STATUS0 or STATUS1 registers are set and not masked.
+
+HDRB (Bit 30) - Received Header Bad. When set, indicates that the MAC-PHY
+ received a control or data header with a parity error.
+
+SYNC (Bit 29) - Configuration Synchronized flag. This bit reflects the
+ state of the SYNC bit in the CONFIG0 configuration
+ register (see Table 12). A zero indicates that the MAC-PHY
+ configuration may not be as expected by the SPI host.
+ Following configuration, the SPI host sets the
+ corresponding bitin the configuration register which is
+ reflected in this field.
+
+RCA (Bit 28..24) - Receive Chunks Available. The RCA field indicates to
+ the SPI host the minimum number of additional receive
+ data chunks of frame data that are available for
+ reading beyond the current receive data chunk. This
+ field is zero when there is no receive frame data
+ pending in the MAC-PHY’s buffer for reading.
+
+VS (Bit 23..22) - Vendor Specific. These bits are implementation specific.
+ If not implemented, the MAC-PHY shall set these bits to
+ ‘0’.
+
+DV (Bit 21) - Data Valid flag. The MAC-PHY uses this bit to indicate
+ whether the current receive data chunk contains valid
+ receive frame data (DV = 1) or not (DV = 0). When ‘0’, the
+ SPI host shall ignore the chunk payload.
+
+SV (Bit 20) - Start Valid flag. The MAC-PHY sets this bit when the current
+ chunk payload contains the start of an Ethernet frame.
+ Otherwise, this bit is zero. The SV bit is not to be
+ confused with the Start-of-Frame Delimiter (SFD) byte
+ described in IEEE 802.3 [2].
+
+SWO (Bit 19..16) - Start Word Offset. When SV = 1, this field contains the
+ 32-bit word offset into the receive data chunk payload
+ containing the first byte of a new received Ethernet
+ frame. When a receive timestamp has been added to the
+ beginning of the received Ethernet frame (RTSA = 1)
+ then SWO points to the most significant byte of the
+ timestamp. This field will be zero when SV = 0.
+
+FD (Bit 15) - Frame Drop. When set, this bit indicates that the MAC has
+ detected a condition for which the SPI host should drop the
+ received Ethernet frame. This bit is only valid at the end
+ of a received Ethernet frame (EV = 1) and shall be zero at
+ all other times.
+
+EV (Bit 14) - End Valid flag. The MAC-PHY sets this bit when the end of a
+ received Ethernet frame is present in this receive data
+ chunk payload.
+
+EBO (Bit 13..8) - End Byte Offset: When EV = 1, this field contains the
+ byte offset into the receive data chunk payload that
+ locates the last byte of the received Ethernet frame.
+ This field is zero when EV = 0.
+
+RTSA (Bit 7) - Receive Timestamp Added. This bit is set when a 32-bit or
+ 64-bit timestamp has been added to the beginning of the
+ received Ethernet frame. The MAC-PHY shall set this bit to
+ zero when SV = 0.
+
+RTSP (Bit 6) - Receive Timestamp Parity. Parity bit calculated over the
+ 32-bit/64-bit timestamp added to the beginning of the
+ received Ethernet frame. Method used is odd parity. The
+ MAC-PHY shall set this bit to zero when RTSA = 0.
+
+TXC (Bit 5..1) - Transmit Credits. This field contains the minimum number
+ of transmit data chunks of frame data that the SPI host
+ can write in a single transaction without incurring a
+ transmit buffer overflow error.
+
+P (Bit 0) - Parity. Parity bit calculated over the receive data footer.
+ Method used is odd parity.
+
+SPI host will initiate the data receive transaction based on the receive
+chunks available in the MAC-PHY which is provided in the receive chunk
+footer (RCA - Receive Chunks Available). SPI host will create data invalid
+transmit data chunks (empty chunks) or data valid transmit data chunks in
+case there are valid Ethernet frames to transmit to the MAC-PHY. The
+receive chunks available in MAC-PHY can be read either from the Buffer
+Status Register or footer.
+
+In case the previous data footer had no receive data chunks available and
+once the receive data chunks become available again for reading, the
+MAC-PHY interrupt is asserted to SPI host. On reception of the first data
+header this interrupt will be deasserted and the received footer for the
+first data chunk will have the receive chunks available information.
+
+MAC-PHY Interrupt
+~~~~~~~~~~~~~~~~~
+
+The MAC-PHY interrupt is asserted when the following conditions are met.
+
+Receive chunks available - This interrupt is asserted when the previous
+data footer had no receive data chunks available and once the receive
+data chunks become available for reading. On reception of the first data
+header this interrupt will be deasserted.
+
+Transmit chunk credits available - This interrupt is asserted when the
+previous data footer indicated no transmit credits available and once the
+transmit credits become available for transmitting transmit data chunks.
+On reception of the first data header this interrupt will be deasserted.
+
+Extended status event - This interrupt is asserted when the previous data
+footer indicated no extended status and once the extended event become
+available. In this case the host should read status #0 register to know
+the corresponding error/event. On reception of the first data header this
+interrupt will be deasserted.
+
+Control Transaction
+~~~~~~~~~~~~~~~~~~~
+
+4 bytes control header contains the below fields,
+
+DNC (Bit 31) - Data-Not-Control flag. This flag specifies the type of SPI
+ transaction. For control commands, this bit shall be ‘0’.
+ 0 - Control command
+ 1 - Data chunk
+
+HDRB (Bit 30) - Received Header Bad. When set by the MAC-PHY, indicates
+ that a header was received with a parity error. The SPI
+ host should always clear this bit. The MAC-PHY ignores the
+ HDRB value sent by the SPI host on MOSI.
+
+WNR (Bit 29) - Write-Not-Read. This bit indicates if data is to be written
+ to registers (when set) or read from registers
+ (when clear).
+
+AID (Bit 28) - Address Increment Disable. When clear, the address will be
+ automatically post-incremented by one following each
+ register read or write. When set, address auto increment is
+ disabled allowing successive reads and writes to occur at
+ the same register address.
+
+MMS (Bit 27..24) - Memory Map Selector. This field selects the specific
+ register memory map to access.
+
+ADDR (Bit 23..8) - Address. Address of the first register within the
+ selected memory map to access.
+
+LEN (Bit 7..1) - Length. Specifies the number of registers to read/write.
+ This field is interpreted as the number of registers
+ minus 1 allowing for up to 128 consecutive registers read
+ or written starting at the address specified in ADDR. A
+ length of zero shall read or write a single register.
+
+P (Bit 0) - Parity. Parity bit calculated over the control command header.
+ Method used is odd parity.
+
+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 4 bytes control command
+header followed by register write data in case of control write command.
+
+The MAC-PHY ignores the final 4 bytes of data from the SPI host at the end
+of the control write command. The control write command is also echoed
+from the MAC-PHY back to the SPI host to identify which register write
+failed in case of any bus errors. The echoed Control write command will
+have the first 4 bytes unused value to be ignored by the SPI host
+followed by 4 bytes echoed control header followed by echoed register
+write 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.
+Writing to any unimplemented or undefined registers shall be ignored and
+yield no effect.
+
+The MAC-PHY ignores all data from the SPI host following the control
+header for the remainder of the control read command. The control read
+command is also echoed from the MAC-PHY back to the SPI host to identify
+which register read is failed in case of any bus errors. The echoed
+Control read command will have the first 4 bytes of unused value to be
+ignored by the SPI host followed by 4 bytes echoed control header followed
+by register read data. 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. Reading any unimplemented or undefined registers shall return
+zero.
+
+Device drivers API
+==================
+
+The include/linux/oa_tc6.h defines the following functions:
+
+.. c:function:: struct oa_tc6 *oa_tc6_init(struct spi_device *spi,
+ struct net_device *netdev)
+
+Initialize OA TC6 lib.
+
+.. c:function:: void oa_tc6_exit(struct oa_tc6 *tc6)
+
+Free allocated OA TC6 lib.
+
+.. c:function:: int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address,
+ u32 value)
+
+Write a single register in the MAC-PHY.
+
+.. c:function:: int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address,
+ u32 value[], u8 length)
+
+Writing multiple consecutive registers starting from @address in the MAC-PHY.
+Maximum of 128 consecutive registers can be written starting at @address.
+
+.. c:function:: int oa_tc6_read_register(struct oa_tc6 *tc6, u32 address,
+ u32 *value)
+
+Read a single register in the MAC-PHY.
+
+.. c:function:: int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address,
+ u32 value[], u8 length)
+
+Reading multiple consecutive registers starting from @address in the MAC-PHY.
+Maximum of 128 consecutive registers can be read starting at @address.
+
+.. c:function:: netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6,
+ struct sk_buff *skb);
+
+The transmit Ethernet frame in the skb is or going to be transmitted through
+the MAC-PHY.
diff --git a/MAINTAINERS b/MAINTAINERS
index 04e5f7c20e30..79fa7abb4ec9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16400,6 +16400,12 @@ 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
+
OPEN FIRMWARE AND FLATTENED DEVICE TREE
M: Rob Herring <[email protected]>
M: Frank Rowand <[email protected]>
--
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 defined in
OPEN Alliance specification. Currently the direct register access is only
supported.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 160 +++++++++++++++++++++++++++++++++-
include/linux/oa_tc6.h | 4 +-
2 files changed, 162 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index f8593b793291..82b4de13438f 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -7,9 +7,15 @@
#include <linux/bitfield.h>
#include <linux/iopoll.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
#include <linux/oa_tc6.h>
/* OPEN Alliance TC6 registers */
+/* Standard Capabilities Register */
+#define OA_TC6_REG_STDCAP 0x0002
+#define STDCAP_DIRECT_PHY_REG_ACCESS BIT(8)
+
/* Reset Control and Status Register */
#define OA_TC6_REG_RESET 0x0003
#define RESET_SWRESET BIT(0) /* Software Reset */
@@ -25,6 +31,10 @@
#define INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK BIT(3)
#define INT_MASK0_TX_PROTOCOL_ERR_MASK BIT(0)
+/* PHY Clause 22 and 29 registers base address and mask */
+#define OA_TC6_PHY_STD_REG_ADDR_BASE 0xFF00
+#define OA_TC6_PHY_STD_REG_ADDR_MASK 0x3F
+
/* Control command header */
#define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
#define OA_TC6_CTRL_HEADER_WRITE BIT(29)
@@ -46,6 +56,10 @@
/* Internal structure for MAC-PHY drivers */
struct oa_tc6 {
+ struct device *dev;
+ struct net_device *netdev;
+ struct phy_device *phydev;
+ struct mii_bus *mdiobus;
struct spi_device *spi;
struct mutex spi_ctrl_lock; /* Protects spi control transfer */
void *spi_ctrl_tx_buf;
@@ -298,6 +312,130 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
}
EXPORT_SYMBOL_GPL(oa_tc6_write_register);
+static int oa_tc6_check_phy_reg_direct_access_capability(struct oa_tc6 *tc6)
+{
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_STDCAP, ®val);
+ if (ret)
+ return ret;
+
+ if (!(regval & STDCAP_DIRECT_PHY_REG_ACCESS))
+ return -ENODEV;
+
+ return 0;
+}
+
+static void oa_tc6_handle_link_change(struct net_device *netdev)
+{
+ phy_print_status(netdev->phydev);
+}
+
+static int oa_tc6_mdiobus_direct_read(struct mii_bus *bus, int addr, int regnum)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+ u32 regval;
+ bool ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
+ (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
+ ®val);
+ if (ret)
+ return -ENODEV;
+
+ return regval;
+}
+
+static int oa_tc6_mdiobus_direct_write(struct mii_bus *bus, int addr, int regnum,
+ u16 val)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+
+ return oa_tc6_write_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
+ (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
+ val);
+}
+
+static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
+{
+ int ret;
+
+ tc6->mdiobus = mdiobus_alloc();
+ if (!tc6->mdiobus) {
+ netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
+ return -ENODEV;
+ }
+
+ tc6->mdiobus->priv = tc6;
+ tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
+ tc6->mdiobus->write = oa_tc6_mdiobus_direct_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;
+ }
+
+ return 0;
+}
+
+static void oa_tc6_mdiobus_unregister(struct oa_tc6 *tc6)
+{
+ mdiobus_unregister(tc6->mdiobus);
+ mdiobus_free(tc6->mdiobus);
+}
+
+static int oa_tc6_phy_init(struct oa_tc6 *tc6)
+{
+ int ret;
+
+ ret = oa_tc6_check_phy_reg_direct_access_capability(tc6);
+ if (ret) {
+ netdev_err(tc6->netdev,
+ "Direct PHY register access is not supported by the MAC-PHY\n");
+ return ret;
+ }
+
+ ret = oa_tc6_mdiobus_register(tc6);
+ if (ret)
+ return ret;
+
+ tc6->phydev = phy_find_first(tc6->mdiobus);
+ if (!tc6->phydev) {
+ netdev_err(tc6->netdev, "No PHY found\n");
+ oa_tc6_mdiobus_unregister(tc6);
+ 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);
+ oa_tc6_mdiobus_unregister(tc6);
+ return ret;
+ }
+
+ phy_attached_info(tc6->netdev->phydev);
+
+ return 0;
+}
+
+static void oa_tc6_phy_exit(struct oa_tc6 *tc6)
+{
+ phy_disconnect(tc6->phydev);
+ oa_tc6_mdiobus_unregister(tc6);
+}
+
static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
{
u32 regval;
@@ -351,11 +489,12 @@ static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
/**
* oa_tc6_init - allocates and initializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
+ * @netdev: network device interface structure.
*
* Returns pointer reference to the oa_tc6 structure if the MAC-PHY
* initialization is successful 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;
int ret;
@@ -365,6 +504,8 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
return NULL;
tc6->spi = spi;
+ tc6->netdev = netdev;
+ SET_NETDEV_DEV(netdev, &spi->dev);
mutex_init(&tc6->spi_ctrl_lock);
/* Set the SPI controller to pump at realtime priority */
@@ -395,10 +536,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
return NULL;
}
+ ret = oa_tc6_phy_init(tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev,
+ "MAC internal PHY initialization failed: %d\n", ret);
+ 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 85aeecf87306..606ba9f1e663 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -7,11 +7,13 @@
* Author: Parthiban Veerasooran <[email protected]>
*/
+#include <linux/etherdevice.h>
#include <linux/spi/spi.h>
struct oa_tc6;
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
+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 address, u32 value);
int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
u8 length);
--
2.34.1
The transmit ethernet frame will be converted into multiple transmit data
chunks. Each transmit data chunk consists of a 4 bytes header followed by
a 64 bytes transmit data chunk payload. The 4 bytes data header occurs at
the beginning of each transmit data chunk on MOSI. The data header
contains the information needed to determine the validity and location of
the transmit frame data within the data chunk payload. The number of
transmit data chunks transmitted to mac-phy is limited to the number
transmit credits available in the mac-phy. Initially the transmit credits
will be updated from the buffer status register and then it will be
updated from the footer received on each spi data transfer. The received
footer will be examined for the transmit errors if any.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 386 +++++++++++++++++++++++++++++++++-
include/linux/oa_tc6.h | 1 +
2 files changed, 385 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 371687670409..44e8c86a5305 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -27,6 +27,13 @@
/* Status Register #0 */
#define OA_TC6_REG_STATUS0 0x0008
#define STATUS0_RESETC BIT(6) /* Reset Complete */
+#define STATUS0_HEADER_ERROR BIT(5)
+#define STATUS0_LOSS_OF_FRAME_ERROR BIT(4)
+#define STATUS0_TX_PROTOCOL_ERROR BIT(0)
+
+/* Buffer Status Register */
+#define OA_TC6_REG_BUFFER_STATUS 0x000B
+#define BUFFER_STATUS_TX_CREDITS_AVAILABLE GENMASK(15, 8)
/* Interrupt Mask Register #0 */
#define OA_TC6_REG_INT_MASK0 0x000C
@@ -47,6 +54,21 @@
#define OA_TC6_CTRL_HEADER_LENGTH GENMASK(7, 1)
#define OA_TC6_CTRL_HEADER_PARITY BIT(0)
+/* Data header */
+#define OA_TC6_DATA_HEADER_DATA_NOT_CTRL BIT(31)
+#define OA_TC6_DATA_HEADER_DATA_VALID BIT(21)
+#define OA_TC6_DATA_HEADER_START_VALID BIT(20)
+#define OA_TC6_DATA_HEADER_START_WORD_OFFSET GENMASK(19, 16)
+#define OA_TC6_DATA_HEADER_END_VALID BIT(14)
+#define OA_TC6_DATA_HEADER_END_BYTE_OFFSET GENMASK(13, 8)
+#define OA_TC6_DATA_HEADER_PARITY BIT(0)
+
+/* Data footer */
+#define OA_TC6_DATA_FOOTER_EXTENDED_STS BIT(31)
+#define OA_TC6_DATA_FOOTER_RXD_HEADER_BAD BIT(30)
+#define OA_TC6_DATA_FOOTER_CONFIG_SYNC BIT(29)
+#define OA_TC6_DATA_FOOTER_TX_CREDITS GENMASK(5, 1)
+
#define OA_TC6_CTRL_HEADER_SIZE 4
#define OA_TC6_CTRL_REG_VALUE_SIZE 4
#define OA_TC6_CTRL_IGNORED_SIZE 4
@@ -55,6 +77,14 @@
(OA_TC6_CTRL_MAX_REGISTERS *\
OA_TC6_CTRL_REG_VALUE_SIZE) +\
OA_TC6_CTRL_IGNORED_SIZE)
+#define OA_TC6_CHUNK_PAYLOAD_SIZE 64
+#define OA_TC6_DATA_HEADER_SIZE 4
+#define OA_TC6_CHUNK_SIZE (OA_TC6_DATA_HEADER_SIZE +\
+ OA_TC6_CHUNK_PAYLOAD_SIZE)
+#define OA_TC6_TX_SKB_QUEUE_SIZE 100
+#define OA_TC6_MAX_TX_CHUNKS 48
+#define OA_TC6_SPI_DATA_BUF_SIZE (OA_TC6_MAX_TX_CHUNKS *\
+ OA_TC6_CHUNK_SIZE)
#define STATUS0_RESETC_POLL_DELAY 5
#define STATUS0_RESETC_POLL_TIMEOUT 100
@@ -68,10 +98,20 @@ struct oa_tc6 {
struct mutex spi_ctrl_lock; /* Protects spi control transfer */
void *spi_ctrl_tx_buf;
void *spi_ctrl_rx_buf;
+ void *spi_data_tx_buf;
+ void *spi_data_rx_buf;
+ struct sk_buff_head tx_skb_q;
+ struct sk_buff *tx_skb;
+ struct task_struct *spi_thread;
+ wait_queue_head_t spi_wq;
+ u16 tx_skb_offset;
+ u16 spi_data_tx_buf_offset;
+ u16 tx_credits;
};
enum oa_tc6_header_type {
OA_TC6_CTRL_HEADER,
+ OA_TC6_DATA_HEADER,
};
enum oa_tc6_register_op {
@@ -79,14 +119,34 @@ enum oa_tc6_register_op {
OA_TC6_CTRL_REG_WRITE = 1,
};
+enum oa_tc6_data_valid_info {
+ OA_TC6_DATA_INVALID,
+ OA_TC6_DATA_VALID,
+};
+
+enum oa_tc6_data_start_valid_info {
+ OA_TC6_DATA_START_INVALID,
+ OA_TC6_DATA_START_VALID,
+};
+
+enum oa_tc6_data_end_valid_info {
+ OA_TC6_DATA_END_INVALID,
+ OA_TC6_DATA_END_VALID,
+};
+
static int oa_tc6_spi_transfer(struct oa_tc6 *tc6,
enum oa_tc6_header_type header_type, u16 length)
{
struct spi_transfer xfer = { 0 };
struct spi_message msg;
- xfer.tx_buf = tc6->spi_ctrl_tx_buf;
- xfer.rx_buf = tc6->spi_ctrl_rx_buf;
+ if (header_type == OA_TC6_DATA_HEADER) {
+ xfer.tx_buf = tc6->spi_data_tx_buf;
+ xfer.rx_buf = tc6->spi_data_rx_buf;
+ } else {
+ xfer.tx_buf = tc6->spi_ctrl_tx_buf;
+ xfer.rx_buf = tc6->spi_ctrl_rx_buf;
+ }
xfer.len = length;
spi_message_init(&msg);
@@ -505,6 +565,296 @@ static int oa_tc6_enable_data_transfer(struct oa_tc6 *tc6)
return oa_tc6_write_register(tc6, OA_TC6_REG_CONFIG0, value);
}
+static void oa_tc6_cleanup_ongoing_tx_skb(struct oa_tc6 *tc6)
+{
+ if (tc6->tx_skb) {
+ tc6->netdev->stats.tx_dropped++;
+ kfree_skb(tc6->tx_skb);
+ tc6->tx_skb = NULL;
+ }
+}
+
+static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
+{
+ u32 value;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, &value);
+ if (ret) {
+ netdev_err(tc6->netdev, "STATUS0 register read failed: %d\n",
+ ret);
+ return -ENODEV;
+ }
+
+ /* Clear the error interrupts status */
+ ret = oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, value);
+ if (ret) {
+ netdev_err(tc6->netdev, "STATUS0 register write failed: %d\n",
+ ret);
+ return -ENODEV;
+ }
+
+ if (FIELD_GET(STATUS0_TX_PROTOCOL_ERROR, value)) {
+ netdev_err(tc6->netdev, "Transmit protocol error\n");
+ return -ENODEV;
+ }
+ /* TODO: Currently loss of frame and header errors are treated as
+ * non-recoverable errors. They will be handled in the next version.
+ */
+ if (FIELD_GET(STATUS0_LOSS_OF_FRAME_ERROR, value)) {
+ netdev_err(tc6->netdev, "Loss of frame error\n");
+ return -ENODEV;
+ }
+ if (FIELD_GET(STATUS0_HEADER_ERROR, value)) {
+ netdev_err(tc6->netdev, "Header error\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
+{
+ /* Process rx chunk footer for the following,
+ * 1. tx credits
+ * 2. errors if any from MAC-PHY
+ */
+ tc6->tx_credits = FIELD_GET(OA_TC6_DATA_FOOTER_TX_CREDITS, footer);
+
+ if (FIELD_GET(OA_TC6_DATA_FOOTER_EXTENDED_STS, footer)) {
+ int ret = oa_tc6_process_extended_status(tc6);
+
+ if (ret)
+ return ret;
+ }
+
+ /* TODO: Currently received header bad and configuration unsync errors
+ * are treated as non-recoverable errors. They will be handled in the
+ * next version.
+ */
+ if (FIELD_GET(OA_TC6_DATA_FOOTER_RXD_HEADER_BAD, footer)) {
+ netdev_err(tc6->netdev, "Rxd header bad error\n");
+ return -ENODEV;
+ }
+
+ if (!FIELD_GET(OA_TC6_DATA_FOOTER_CONFIG_SYNC, footer)) {
+ netdev_err(tc6->netdev, "Config unsync error\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
+{
+ u8 *rx_buf = tc6->spi_data_rx_buf;
+ __be32 footer;
+
+ footer = *((__be32 *)&rx_buf[footer_offset]);
+
+ return be32_to_cpu(footer);
+}
+
+static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
+{
+ u16 no_of_rx_chunks = length / OA_TC6_CHUNK_SIZE;
+ u32 footer;
+ int ret;
+
+ /* All the rx chunks in the receive SPI data buffer are examined here */
+ for (int i = 0; i < no_of_rx_chunks; i++) {
+ /* Last 4 bytes in each received chunk consist footer info */
+ footer = oa_tc6_get_rx_chunk_footer(tc6, i * OA_TC6_CHUNK_SIZE +
+ OA_TC6_CHUNK_PAYLOAD_SIZE);
+
+ ret = oa_tc6_process_rx_chunk_footer(tc6, footer);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static __be32 oa_tc6_prepare_data_header(bool data_valid, bool start_valid,
+ bool end_valid, u8 end_byte_offset)
+{
+ u32 header = FIELD_PREP(OA_TC6_DATA_HEADER_DATA_NOT_CTRL,
+ OA_TC6_DATA_HEADER) |
+ FIELD_PREP(OA_TC6_DATA_HEADER_DATA_VALID, data_valid) |
+ FIELD_PREP(OA_TC6_DATA_HEADER_START_VALID, start_valid) |
+ FIELD_PREP(OA_TC6_DATA_HEADER_END_VALID, end_valid) |
+ FIELD_PREP(OA_TC6_DATA_HEADER_END_BYTE_OFFSET,
+ end_byte_offset);
+
+ header |= FIELD_PREP(OA_TC6_DATA_HEADER_PARITY,
+ oa_tc6_get_parity(header));
+
+ return cpu_to_be32(header);
+}
+
+static void oa_tc6_add_tx_skb_to_spi_buf(struct oa_tc6 *tc6)
+{
+ enum oa_tc6_data_start_valid_info start_valid = OA_TC6_DATA_START_INVALID;
+ enum oa_tc6_data_end_valid_info end_valid = OA_TC6_DATA_END_INVALID;
+ __be32 *tx_buf = tc6->spi_data_tx_buf + tc6->spi_data_tx_buf_offset;
+ u16 remaining_length = tc6->tx_skb->len - tc6->tx_skb_offset;
+ u8 *tx_skb_data = tc6->tx_skb->data + tc6->tx_skb_offset;
+ u8 end_byte_offset = 0;
+ u16 length_to_copy;
+
+ /* Set start valid if the current tx chunk contains the start of the tx
+ * ethernet frame.
+ */
+ if (!tc6->tx_skb_offset)
+ start_valid = OA_TC6_DATA_START_VALID;
+
+ /* If the remaining tx skb length is more than the chunk payload size of
+ * 64 bytes then copy only 64 bytes and leave the ongoing tx skb for
+ * next tx chunk.
+ */
+ length_to_copy = min_t(u16, remaining_length, OA_TC6_CHUNK_PAYLOAD_SIZE);
+
+ /* Copy the tx skb data to the tx chunk payload buffer */
+ memcpy(tx_buf + 1, tx_skb_data, length_to_copy);
+ tc6->tx_skb_offset += length_to_copy;
+
+ /* Set end valid if the current tx chunk contains the end of the tx
+ * ethernet frame.
+ */
+ if (tc6->tx_skb->len == tc6->tx_skb_offset) {
+ end_valid = OA_TC6_DATA_END_VALID;
+ end_byte_offset = length_to_copy - 1;
+ tc6->tx_skb_offset = 0;
+ tc6->netdev->stats.tx_bytes += tc6->tx_skb->len;
+ tc6->netdev->stats.tx_packets++;
+ kfree_skb(tc6->tx_skb);
+ tc6->tx_skb = NULL;
+ }
+
+ *tx_buf = oa_tc6_prepare_data_header(OA_TC6_DATA_VALID, start_valid,
+ end_valid, end_byte_offset);
+ tc6->spi_data_tx_buf_offset += OA_TC6_CHUNK_SIZE;
+}
+
+static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
+{
+ u16 used_tx_credits;
+
+ /* Get tx skbs and convert them into tx chunks based on the tx credits
+ * available.
+ */
+ for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
+ used_tx_credits++) {
+ if (!tc6->tx_skb)
+ tc6->tx_skb = skb_dequeue(&tc6->tx_skb_q);
+ if (!tc6->tx_skb)
+ break;
+ oa_tc6_add_tx_skb_to_spi_buf(tc6);
+ }
+
+ return used_tx_credits * OA_TC6_CHUNK_SIZE;
+}
+
+static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
+{
+ int ret;
+
+ while (true) {
+ u16 spi_length = 0;
+
+ tc6->spi_data_tx_buf_offset = 0;
+
+ if (tc6->tx_skb || !skb_queue_empty(&tc6->tx_skb_q))
+ spi_length = oa_tc6_prepare_spi_tx_buf_for_tx_skbs(tc6);
+
+ if (spi_length == 0)
+ break;
+
+ ret = oa_tc6_spi_transfer(tc6, OA_TC6_DATA_HEADER, spi_length);
+ if (ret) {
+ netdev_err(tc6->netdev,
+ "SPI data transfer failed. Restart the system: %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = oa_tc6_process_spi_data_rx_buf(tc6, spi_length);
+ if (ret) {
+ oa_tc6_cleanup_ongoing_tx_skb(tc6);
+ netdev_err(tc6->netdev,
+ "Device error. Restart the system\n");
+ return ret;
+ }
+
+ if (skb_queue_len(&tc6->tx_skb_q) < OA_TC6_TX_SKB_QUEUE_SIZE &&
+ netif_queue_stopped(tc6->netdev))
+ netif_start_queue(tc6->netdev);
+ }
+
+ return 0;
+}
+
+static int oa_tc6_spi_thread_handler(void *data)
+{
+ struct oa_tc6 *tc6 = data;
+ int ret;
+
+ while (likely(!kthread_should_stop())) {
+ /* This kthread will be waken up if there is a tx skb */
+ wait_event_interruptible(tc6->spi_wq,
+ !skb_queue_empty(&tc6->tx_skb_q) ||
+ kthread_should_stop());
+ ret = oa_tc6_try_spi_transfer(tc6);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int oa_tc6_update_buffer_status_from_register(struct oa_tc6 *tc6)
+{
+ u32 value;
+ int ret;
+
+ /* Initially tx credits to be updated from the register as there is no
+ * data transfer performed yet. Later it will be updated from the rx
+ * footer.
+ */
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_BUFFER_STATUS, &value);
+ if (ret)
+ return ret;
+
+ tc6->tx_credits = FIELD_GET(BUFFER_STATUS_TX_CREDITS_AVAILABLE, value);
+
+ return 0;
+}
+
+/**
+ * oa_tc6_start_xmit - function for sending the tx skb which consists ethernet
+ * frame.
+ * @tc6: oa_tc6 struct.
+ * @skb: socket buffer in which the ethernet frame is stored.
+ *
+ * Returns NETDEV_TX_OK if the transmit ethernet frame skb added in the tx_skb_q
+ * otherwise returns NETDEV_TX_BUSY.
+ */
+netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb)
+{
+ if (skb_queue_len(&tc6->tx_skb_q) > OA_TC6_TX_SKB_QUEUE_SIZE) {
+ netif_stop_queue(tc6->netdev);
+ return NETDEV_TX_BUSY;
+ }
+
+ skb_queue_tail(&tc6->tx_skb_q, skb);
+
+ /* Wake spi kthread to perform spi transfer */
+ wake_up_interruptible(&tc6->spi_wq);
+
+ return NETDEV_TX_OK;
+}
+EXPORT_SYMBOL_GPL(oa_tc6_start_xmit);
+
/**
* oa_tc6_init - allocates and initializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
@@ -541,6 +891,16 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
if (!tc6->spi_ctrl_rx_buf)
return NULL;
+ tc6->spi_data_tx_buf = devm_kzalloc(&tc6->spi->dev,
+ OA_TC6_SPI_DATA_BUF_SIZE, GFP_KERNEL);
+ if (!tc6->spi_data_tx_buf)
+ return NULL;
+
+ tc6->spi_data_rx_buf = devm_kzalloc(&tc6->spi->dev,
+ OA_TC6_SPI_DATA_BUF_SIZE, GFP_KERNEL);
+ if (!tc6->spi_data_rx_buf)
+ return NULL;
+
ret = oa_tc6_sw_reset_macphy(tc6);
if (ret) {
dev_err(&tc6->spi->dev,
@@ -569,6 +929,25 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
goto phy_exit;
}
+ ret = oa_tc6_update_buffer_status_from_register(tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev,
+ "Failed to update buffer status: %d\n", ret);
+ goto phy_exit;
+ }
+
+ skb_queue_head_init(&tc6->tx_skb_q);
+ init_waitqueue_head(&tc6->spi_wq);
+
+ tc6->spi_thread = kthread_run(oa_tc6_spi_thread_handler, tc6,
+ "oa-tc6-spi-thread");
+ if (IS_ERR(tc6->spi_thread)) {
+ dev_err(&tc6->spi->dev, "Failed to create SPI thread\n");
+ goto phy_exit;
+ }
+
+ sched_set_fifo(tc6->spi_thread);
+
return tc6;
phy_exit:
@@ -584,6 +963,9 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);
void oa_tc6_exit(struct oa_tc6 *tc6)
{
oa_tc6_phy_exit(tc6);
+ kthread_stop(tc6->spi_thread);
+ dev_kfree_skb_any(tc6->tx_skb);
+ skb_queue_purge(&tc6->tx_skb_q);
}
EXPORT_SYMBOL_GPL(oa_tc6_exit);
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 606ba9f1e663..5c7811ac9cbe 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -20,3 +20,4 @@ int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
int oa_tc6_read_register(struct oa_tc6 *tc6, u32 address, u32 *value);
int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
u8 length);
+netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb);
--
2.34.1
SPI rx data buffer can contain one or more receive data chunks. A receive
data chunk consists a 64 bytes receive data chunk payload followed a
4 bytes data footer at the end. The data footer contains the information
needed to determine the validity and location of the receive frame data
within the receive data chunk payload and the host can use these
information to generate ethernet frame. Initially the receive chunks
available will be updated from the buffer status register and then it
will be updated from the footer received on each spi data transfer. Tx
data valid or empty chunks equal to the number receive chunks available
will be transmitted in the MOSI to receive all the rx chunks.
Additionally the receive data footer contains the below information as
well. The received footer will be examined for the receive errors if any.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 227 +++++++++++++++++++++++++++++++++-
1 file changed, 224 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 44e8c86a5305..93b9afafdf9c 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -29,11 +29,13 @@
#define STATUS0_RESETC BIT(6) /* Reset Complete */
#define STATUS0_HEADER_ERROR BIT(5)
#define STATUS0_LOSS_OF_FRAME_ERROR BIT(4)
+#define STATUS0_RX_BUFFER_OVERFLOW_ERROR BIT(3)
#define STATUS0_TX_PROTOCOL_ERROR BIT(0)
/* Buffer Status Register */
#define OA_TC6_REG_BUFFER_STATUS 0x000B
#define BUFFER_STATUS_TX_CREDITS_AVAILABLE GENMASK(15, 8)
+#define BUFFER_STATUS_RX_CHUNKS_AVAILABLE GENMASK(7, 0)
/* Interrupt Mask Register #0 */
#define OA_TC6_REG_INT_MASK0 0x000C
@@ -67,6 +69,12 @@
#define OA_TC6_DATA_FOOTER_EXTENDED_STS BIT(31)
#define OA_TC6_DATA_FOOTER_RXD_HEADER_BAD BIT(30)
#define OA_TC6_DATA_FOOTER_CONFIG_SYNC BIT(29)
+#define OA_TC6_DATA_FOOTER_RX_CHUNKS_AVAILABLE GENMASK(28, 24)
+#define OA_TC6_DATA_FOOTER_DATA_VALID BIT(21)
+#define OA_TC6_DATA_FOOTER_START_VALID BIT(20)
+#define OA_TC6_DATA_FOOTER_START_WORD_OFFSET GENMASK(19, 16)
+#define OA_TC6_DATA_FOOTER_END_VALID BIT(14)
+#define OA_TC6_DATA_FOOTER_END_BYTE_OFFSET GENMASK(13, 8)
#define OA_TC6_DATA_FOOTER_TX_CREDITS GENMASK(5, 1)
#define OA_TC6_CTRL_HEADER_SIZE 4
@@ -102,11 +110,14 @@ struct oa_tc6 {
void *spi_data_rx_buf;
struct sk_buff_head tx_skb_q;
struct sk_buff *tx_skb;
+ struct sk_buff *rx_skb;
struct task_struct *spi_thread;
wait_queue_head_t spi_wq;
u16 tx_skb_offset;
u16 spi_data_tx_buf_offset;
u16 tx_credits;
+ u8 rx_chunks_available;
+ bool rx_buf_overflow;
};
enum oa_tc6_header_type {
@@ -565,6 +576,15 @@ static int oa_tc6_enable_data_transfer(struct oa_tc6 *tc6)
return oa_tc6_write_register(tc6, OA_TC6_REG_CONFIG0, value);
}
+static void oa_tc6_cleanup_ongoing_rx_skb(struct oa_tc6 *tc6)
+{
+ if (tc6->rx_skb) {
+ tc6->netdev->stats.rx_dropped++;
+ kfree_skb(tc6->rx_skb);
+ tc6->rx_skb = NULL;
+ }
+}
+
static void oa_tc6_cleanup_ongoing_tx_skb(struct oa_tc6 *tc6)
{
if (tc6->tx_skb) {
@@ -594,6 +614,13 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
return -ENODEV;
}
+ if (FIELD_GET(STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
+ tc6->rx_buf_overflow = true;
+ oa_tc6_cleanup_ongoing_rx_skb(tc6);
+ net_err_ratelimited("%s: Receive buffer overflow error\n",
+ tc6->netdev->name);
+ return -EAGAIN;
+ }
if (FIELD_GET(STATUS0_TX_PROTOCOL_ERROR, value)) {
netdev_err(tc6->netdev, "Transmit protocol error\n");
return -ENODEV;
@@ -618,8 +645,11 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
/* Process rx chunk footer for the following,
* 1. tx credits
* 2. errors if any from MAC-PHY
+ * 3. receive chunks available
*/
tc6->tx_credits = FIELD_GET(OA_TC6_DATA_FOOTER_TX_CREDITS, footer);
+ tc6->rx_chunks_available = FIELD_GET(OA_TC6_DATA_FOOTER_RX_CHUNKS_AVAILABLE,
+ footer);
if (FIELD_GET(OA_TC6_DATA_FOOTER_EXTENDED_STS, footer)) {
int ret = oa_tc6_process_extended_status(tc6);
@@ -645,6 +675,139 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
return 0;
}
+static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
+{
+ tc6->rx_skb->protocol = eth_type_trans(tc6->rx_skb, tc6->netdev);
+ tc6->netdev->stats.rx_packets++;
+ tc6->netdev->stats.rx_bytes += tc6->rx_skb->len;
+
+ if (netif_rx(tc6->rx_skb) == NET_RX_DROP)
+ tc6->netdev->stats.rx_dropped++;
+
+ tc6->rx_skb = NULL;
+}
+
+static void oa_tc6_update_rx_skb(struct oa_tc6 *tc6, u8 *payload, u8 length)
+{
+ memcpy(skb_put(tc6->rx_skb, length), payload, length);
+}
+
+static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
+{
+ tc6->rx_skb = netdev_alloc_skb(tc6->netdev, tc6->netdev->mtu + ETH_HLEN +
+ ETH_FCS_LEN + NET_IP_ALIGN);
+ if (!tc6->rx_skb) {
+ tc6->netdev->stats.rx_dropped++;
+ netdev_err(tc6->netdev, "Out of memory for rx'd frame");
+ return -ENOMEM;
+ }
+ skb_reserve(tc6->rx_skb, NET_IP_ALIGN);
+
+ return 0;
+}
+
+static int oa_tc6_prcs_complete_rx_frame(struct oa_tc6 *tc6, u8 *payload,
+ u16 size)
+{
+ int ret;
+
+ ret = oa_tc6_allocate_rx_skb(tc6);
+ if (ret)
+ return ret;
+
+ oa_tc6_update_rx_skb(tc6, payload, size);
+
+ oa_tc6_submit_rx_skb(tc6);
+
+ return 0;
+}
+
+static int oa_tc6_prcs_rx_frame_start(struct oa_tc6 *tc6, u8 *payload, u16 size)
+{
+ int ret;
+
+ ret = oa_tc6_allocate_rx_skb(tc6);
+ if (ret)
+ return ret;
+
+ oa_tc6_update_rx_skb(tc6, payload, size);
+
+ return 0;
+}
+
+static void oa_tc6_prcs_rx_frame_end(struct oa_tc6 *tc6, u8 *payload, u16 size)
+{
+ oa_tc6_update_rx_skb(tc6, payload, size);
+
+ oa_tc6_submit_rx_skb(tc6);
+}
+
+static void oa_tc6_prcs_ongoing_rx_frame(struct oa_tc6 *tc6, u8 *payload,
+ u32 footer)
+{
+ oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);
+}
+
+static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *payload,
+ u32 footer)
+{
+ u8 start_byte_offset = FIELD_GET(OA_TC6_DATA_FOOTER_START_WORD_OFFSET,
+ footer) * sizeof(u32);
+ u8 end_byte_offset = FIELD_GET(OA_TC6_DATA_FOOTER_END_BYTE_OFFSET,
+ footer);
+ bool start_valid = FIELD_GET(OA_TC6_DATA_FOOTER_START_VALID, footer);
+ bool end_valid = FIELD_GET(OA_TC6_DATA_FOOTER_END_VALID, footer);
+ u16 size;
+
+ /* Restart the new rx frame after receiving rx buffer overflow error */
+ if (start_valid && tc6->rx_buf_overflow)
+ tc6->rx_buf_overflow = false;
+
+ if (tc6->rx_buf_overflow)
+ return 0;
+
+ /* Process the chunk with complete rx frame */
+ if (start_valid && end_valid && start_byte_offset < end_byte_offset) {
+ size = end_byte_offset + 1 - start_byte_offset;
+ return oa_tc6_prcs_complete_rx_frame(tc6, &payload[start_byte_offset],
+ size);
+ }
+
+ /* Process the chunk with only rx frame start */
+ if (start_valid && !end_valid) {
+ size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
+ return oa_tc6_prcs_rx_frame_start(tc6, &payload[start_byte_offset],
+ size);
+ }
+
+ /* Process the chunk with only rx frame end */
+ if (end_valid && !start_valid) {
+ size = end_byte_offset + 1;
+ oa_tc6_prcs_rx_frame_end(tc6, payload, size);
+ return 0;
+ }
+
+ /* Process the chunk with previous rx frame end and next rx frame start */
+ if (start_valid && end_valid && start_byte_offset > end_byte_offset) {
+ /* After rx buffer overflow error received, there might be a
+ * possibility of getting an end valid of a previously
+ * incomplete rx frame along with the new rx frame start valid.
+ */
+ if (tc6->rx_skb) {
+ size = end_byte_offset + 1;
+ oa_tc6_prcs_rx_frame_end(tc6, payload, size);
+ }
+ size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
+ return oa_tc6_prcs_rx_frame_start(tc6, &payload[start_byte_offset],
+ size);
+ }
+
+ /* Process the chunk with ongoing rx frame data */
+ oa_tc6_prcs_ongoing_rx_frame(tc6, payload, footer);
+
+ return 0;
+}
+
static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
{
u8 *rx_buf = tc6->spi_data_rx_buf;
@@ -670,6 +833,18 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
ret = oa_tc6_process_rx_chunk_footer(tc6, footer);
if (ret)
return ret;
+
+ /* If there is a data valid chunks then process it for the
+ * information needed to determine the validity and the location
+ * of the receive frame data.
+ */
+ if (FIELD_GET(OA_TC6_DATA_FOOTER_DATA_VALID, footer)) {
+ u8 *payload = tc6->spi_data_rx_buf + i * OA_TC6_CHUNK_SIZE;
+
+ ret = oa_tc6_prcs_rx_chunk_payload(tc6, payload, footer);
+ if (ret)
+ return ret;
+ }
}
return 0;
@@ -755,6 +930,42 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
return used_tx_credits * OA_TC6_CHUNK_SIZE;
}
+static void oa_tc6_add_empty_chunks_to_spi_buf(struct oa_tc6 *tc6,
+ u16 needed_empty_chunks)
+{
+ __be32 header;
+
+ header = oa_tc6_prepare_data_header(OA_TC6_DATA_INVALID,
+ OA_TC6_DATA_START_INVALID,
+ OA_TC6_DATA_END_INVALID, 0);
+
+ while (needed_empty_chunks--) {
+ __be32 *tx_buf = tc6->spi_data_tx_buf + tc6->spi_data_tx_buf_offset;
+
+ *tx_buf = header;
+ tc6->spi_data_tx_buf_offset += OA_TC6_CHUNK_SIZE;
+ }
+}
+
+static u16 oa_tc6_prepare_spi_tx_buf_for_rx_chunks(struct oa_tc6 *tc6, u16 len)
+{
+ u16 tx_chunks = len / OA_TC6_CHUNK_SIZE;
+ u16 needed_empty_chunks;
+
+ /* If there are more chunks to receive than to transmit, we need to add
+ * enough empty tx chunks to allow the reception of the excess rx
+ * chunks.
+ */
+ if (tx_chunks >= tc6->rx_chunks_available)
+ return len;
+
+ needed_empty_chunks = tc6->rx_chunks_available - tx_chunks;
+
+ oa_tc6_add_empty_chunks_to_spi_buf(tc6, needed_empty_chunks);
+
+ return needed_empty_chunks * OA_TC6_CHUNK_SIZE + len;
+}
+
static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
{
int ret;
@@ -767,6 +978,9 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
if (tc6->tx_skb || !skb_queue_empty(&tc6->tx_skb_q))
spi_length = oa_tc6_prepare_spi_tx_buf_for_tx_skbs(tc6);
+ if (tc6->rx_chunks_available)
+ spi_length = oa_tc6_prepare_spi_tx_buf_for_rx_chunks(tc6, spi_length);
+
if (spi_length == 0)
break;
@@ -780,7 +994,11 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
ret = oa_tc6_process_spi_data_rx_buf(tc6, spi_length);
if (ret) {
+ if (ret == -EAGAIN)
+ continue;
+
oa_tc6_cleanup_ongoing_tx_skb(tc6);
+ oa_tc6_cleanup_ongoing_rx_skb(tc6);
netdev_err(tc6->netdev,
"Device error. Restart the system\n");
return ret;
@@ -817,15 +1035,17 @@ static int oa_tc6_update_buffer_status_from_register(struct oa_tc6 *tc6)
u32 value;
int ret;
- /* Initially tx credits to be updated from the register as there is no
- * data transfer performed yet. Later it will be updated from the rx
- * footer.
+ /* Initially tx credits and rx chunks available to be updated from the
+ * register as there is no data transfer performed yet. Later they will
+ * be updated from the rx footer.
*/
ret = oa_tc6_read_register(tc6, OA_TC6_REG_BUFFER_STATUS, &value);
if (ret)
return ret;
tc6->tx_credits = FIELD_GET(BUFFER_STATUS_TX_CREDITS_AVAILABLE, value);
+ tc6->rx_chunks_available = FIELD_GET(BUFFER_STATUS_RX_CHUNKS_AVAILABLE,
+ value);
return 0;
}
@@ -965,6 +1185,7 @@ void oa_tc6_exit(struct oa_tc6 *tc6)
oa_tc6_phy_exit(tc6);
kthread_stop(tc6->spi_thread);
dev_kfree_skb_any(tc6->tx_skb);
+ dev_kfree_skb_any(tc6->rx_skb);
skb_queue_purge(&tc6->tx_skb_q);
}
EXPORT_SYMBOL_GPL(oa_tc6_exit);
--
2.34.1
The MAC-PHY interrupt is asserted when the following conditions are met.
Receive chunks available - This interrupt is asserted when the previous
data footer had no receive data chunks available and once the receive
data chunks become available for reading. On reception of the first data
header this interrupt will be deasserted.
Transmit chunk credits available - This interrupt is asserted when the
previous data footer indicated no transmit credits available and once the
transmit credits become available for transmitting transmit data chunks.
On reception of the first data header this interrupt will be deasserted.
Extended status event - This interrupt is asserted when the previous data
footer indicated no extended status and once the extended event become
available. In this case the host should read status #0 register to know
the corresponding error/event. On reception of the first data header this
interrupt will be deasserted.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 44 +++++++++++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 93b9afafdf9c..e8c1bd7ba3a5 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -118,6 +118,7 @@ struct oa_tc6 {
u16 tx_credits;
u8 rx_chunks_available;
bool rx_buf_overflow;
+ bool int_flag;
};
enum oa_tc6_header_type {
@@ -981,6 +982,14 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
if (tc6->rx_chunks_available)
spi_length = oa_tc6_prepare_spi_tx_buf_for_rx_chunks(tc6, spi_length);
+ if (tc6->int_flag) {
+ tc6->int_flag = false;
+ if (spi_length == 0) {
+ oa_tc6_add_empty_chunks_to_spi_buf(tc6, 1);
+ spi_length = OA_TC6_CHUNK_SIZE;
+ }
+ }
+
if (spi_length == 0)
break;
@@ -1018,8 +1027,10 @@ static int oa_tc6_spi_thread_handler(void *data)
int ret;
while (likely(!kthread_should_stop())) {
- /* This kthread will be waken up if there is a tx skb */
- wait_event_interruptible(tc6->spi_wq,
+ /* This kthread will be waken up if there is a tx skb or mac-phy
+ * interrupt to perform spi transfer with tx chunks.
+ */
+ wait_event_interruptible(tc6->spi_wq, tc6->int_flag ||
!skb_queue_empty(&tc6->tx_skb_q) ||
kthread_should_stop());
ret = oa_tc6_try_spi_transfer(tc6);
@@ -1050,6 +1061,24 @@ static int oa_tc6_update_buffer_status_from_register(struct oa_tc6 *tc6)
return 0;
}
+static irqreturn_t oa_tc6_macphy_isr(int irq, void *data)
+{
+ struct oa_tc6 *tc6 = data;
+
+ /* MAC-PHY interrupt can occur for the following reasons.
+ * - availability of tx credits if it was 0 before and not reported in
+ * the previous rx footer.
+ * - availability of rx chunks if it was 0 before and not reported in
+ * the previous rx footer.
+ * - extended status event not reported in the previous rx footer.
+ */
+ tc6->int_flag = true;
+ /* Wake spi kthread to perform spi transfer */
+ wake_up_interruptible(&tc6->spi_wq);
+
+ return IRQ_HANDLED;
+}
+
/**
* oa_tc6_start_xmit - function for sending the tx skb which consists ethernet
* frame.
@@ -1168,8 +1197,19 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
sched_set_fifo(tc6->spi_thread);
+ ret = devm_request_irq(&tc6->spi->dev, tc6->spi->irq, oa_tc6_macphy_isr,
+ IRQF_TRIGGER_FALLING, dev_name(&tc6->spi->dev),
+ tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev, "Failed to request macphy isr %d\n",
+ ret);
+ goto kthread_stop;
+ }
+
return tc6;
+kthread_stop:
+ kthread_stop(tc6->spi_thread);
phy_exit:
oa_tc6_phy_exit(tc6);
return NULL;
--
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. The Ethernet
Media Access Controller (MAC) module implements a 10 Mbps half duplex
Ethernet MAC, compatible with the IEEE 802.3 standard. 10BASE-T1S
physical layer transceiver integrated is into the LAN8650/1. The PHY and
MAC are connected via an internal Media Independent Interface (MII).
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
MAINTAINERS | 6 +
drivers/net/ethernet/microchip/Kconfig | 1 +
drivers/net/ethernet/microchip/Makefile | 1 +
.../net/ethernet/microchip/lan865x/Kconfig | 19 +
.../net/ethernet/microchip/lan865x/Makefile | 6 +
.../net/ethernet/microchip/lan865x/lan865x.c | 350 ++++++++++++++++++
6 files changed, 383 insertions(+)
create mode 100644 drivers/net/ethernet/microchip/lan865x/Kconfig
create mode 100644 drivers/net/ethernet/microchip/lan865x/Makefile
create mode 100644 drivers/net/ethernet/microchip/lan865x/lan865x.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 603528948f61..f41b7f2257d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14374,6 +14374,12 @@ L: [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/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 43ba71e82260..06ca79669053 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -56,6 +56,7 @@ config LAN743X
To compile this driver as a module, choose M here. The module will be
called lan743x.
+source "drivers/net/ethernet/microchip/lan865x/Kconfig"
source "drivers/net/ethernet/microchip/lan966x/Kconfig"
source "drivers/net/ethernet/microchip/sparx5/Kconfig"
source "drivers/net/ethernet/microchip/vcap/Kconfig"
diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
index bbd349264e6f..15dfbb321057 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_LAN743X) += lan743x.o
lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
+obj-$(CONFIG_LAN865X) += lan865x/
obj-$(CONFIG_LAN966X_SWITCH) += lan966x/
obj-$(CONFIG_SPARX5_SWITCH) += sparx5/
obj-$(CONFIG_VCAP) += vcap/
diff --git a/drivers/net/ethernet/microchip/lan865x/Kconfig b/drivers/net/ethernet/microchip/lan865x/Kconfig
new file mode 100644
index 000000000000..f3d60d14e202
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Microchip LAN865x Driver Support
+#
+
+if NET_VENDOR_MICROCHIP
+
+config LAN865X
+ tristate "LAN865x support"
+ depends on SPI
+ depends on OA_TC6
+ help
+ Support for the Microchip LAN8650/1 Rev.B1 MACPHY Ethernet chip. It
+ uses OPEN Alliance 10BASE-T1x Serial Interface specification.
+
+ To compile this driver as a module, choose M here. The module will be
+ called lan865x.
+
+endif # NET_VENDOR_MICROCHIP
diff --git a/drivers/net/ethernet/microchip/lan865x/Makefile b/drivers/net/ethernet/microchip/lan865x/Makefile
new file mode 100644
index 000000000000..9f5dd89c1eb8
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for the Microchip LAN865x Driver
+#
+
+obj-$(CONFIG_LAN865X) += lan865x.o
diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
new file mode 100644
index 000000000000..8f265227f75a
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
@@ -0,0 +1,350 @@
+// 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/phy.h>
+#include <linux/oa_tc6.h>
+
+#define DRV_NAME "lan865x"
+
+/* MAC Network Control Register */
+#define LAN865X_REG_MAC_NET_CTL 0x00010000
+#define MAC_NET_CTL_TXEN BIT(3) /* Transmit Enable */
+#define MAC_NET_CTL_RXEN BIT(2) /* Receive Enable */
+
+#define LAN865X_REG_MAC_NET_CFG 0x00010001 /* MAC Network Configuration Reg */
+#define MAC_NET_CFG_PROMISCUOUS_MODE BIT(4)
+#define MAC_NET_CFG_MULTICAST_MODE BIT(6)
+#define MAC_NET_CFG_UNICAST_MODE BIT(7)
+
+#define LAN865X_REG_MAC_L_HASH 0x00010020 /* MAC Hash Register Bottom */
+#define LAN865X_REG_MAC_H_HASH 0x00010021 /* MAC Hash Register Top */
+#define LAN865X_REG_MAC_L_SADDR1 0x00010022 /* MAC Specific Addr 1 Bottom Reg */
+#define LAN865X_REG_MAC_H_SADDR1 0x00010023 /* MAC Specific Addr 1 Top Reg */
+
+struct lan865x_priv {
+ struct work_struct multicast_work;
+ struct net_device *netdev;
+ struct spi_device *spi;
+ struct oa_tc6 *tc6;
+};
+
+static int lan865x_set_hw_macaddr_low_bytes(struct oa_tc6 *tc6, const u8 *mac)
+{
+ u32 regval;
+
+ regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) | mac[0];
+
+ return oa_tc6_write_register(tc6, LAN865X_REG_MAC_L_SADDR1, regval);
+}
+
+static int lan865x_set_hw_macaddr(struct lan865x_priv *priv, const u8 *mac)
+{
+ int restore_ret;
+ u32 regval;
+ int ret;
+
+ /* Configure MAC address low bytes */
+ ret = lan865x_set_hw_macaddr_low_bytes(priv->tc6, mac);
+ if (ret)
+ return ret;
+
+ /* Prepare and configure MAC address high bytes */
+ regval = (mac[5] << 8) | mac[4];
+ ret = oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_SADDR1, regval);
+ if (!ret)
+ return 0;
+
+ /* Restore the old MAC address low bytes from netdev if the new MAC
+ * address high bytes setting failed.
+ */
+ restore_ret = lan865x_set_hw_macaddr_low_bytes(priv->tc6,
+ priv->netdev->dev_addr);
+ if (restore_ret)
+ return restore_ret;
+
+ return ret;
+}
+
+static void
+lan865x_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *info)
+{
+ strscpy(info->driver, DRV_NAME, sizeof(info->driver));
+ strscpy(info->bus_info, dev_name(netdev->dev.parent),
+ sizeof(info->bus_info));
+}
+
+static const struct ethtool_ops lan865x_ethtool_ops = {
+ .get_drvinfo = lan865x_get_drvinfo,
+ .get_link_ksettings = phy_ethtool_get_link_ksettings,
+ .set_link_ksettings = phy_ethtool_set_link_ksettings,
+};
+
+static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ struct sockaddr *address = addr;
+ int ret;
+
+ ret = eth_prepare_mac_addr_change(netdev, addr);
+ if (ret < 0)
+ return ret;
+
+ if (ether_addr_equal(address->sa_data, netdev->dev_addr))
+ return 0;
+
+ ret = lan865x_set_hw_macaddr(priv, address->sa_data);
+ if (ret)
+ return ret;
+
+ eth_hw_addr_set(netdev, address->sa_data);
+
+ return 0;
+}
+
+static u32 lan865x_hash(u8 addr[ETH_ALEN])
+{
+ return (ether_crc(ETH_ALEN, addr) >> 26) & GENMASK(5, 0);
+}
+
+static void lan865x_set_specific_multicast_addr(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ struct netdev_hw_addr *ha;
+ u32 hash_lo = 0;
+ u32 hash_hi = 0;
+
+ netdev_for_each_mc_addr(ha, netdev) {
+ u32 bit_num = lan865x_hash(ha->addr);
+ u32 mask = BIT(bit_num);
+
+ /* 5th bit of the 6 bits hash value is used to determine which
+ * bit to set in either a high or low hash register.
+ */
+ if (bit_num & BIT(5))
+ hash_hi |= mask;
+ else
+ hash_lo |= mask;
+ }
+
+ /* Enabling specific multicast addresses */
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH, hash_hi)) {
+ netdev_err(netdev, "Failed to write reg_hashh");
+ return;
+ }
+
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_L_HASH, hash_lo))
+ netdev_err(netdev, "Failed to write reg_hashl");
+}
+
+static void lan865x_multicast_work_handler(struct work_struct *work)
+{
+ struct lan865x_priv *priv = container_of(work, struct lan865x_priv,
+ multicast_work);
+ u32 regval = 0;
+
+ if (priv->netdev->flags & IFF_PROMISC) {
+ /* Enabling promiscuous mode */
+ regval |= MAC_NET_CFG_PROMISCUOUS_MODE;
+ regval &= (~MAC_NET_CFG_MULTICAST_MODE);
+ regval &= (~MAC_NET_CFG_UNICAST_MODE);
+ } else if (priv->netdev->flags & IFF_ALLMULTI) {
+ /* Enabling all multicast mode */
+ regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
+ regval |= MAC_NET_CFG_MULTICAST_MODE;
+ regval &= (~MAC_NET_CFG_UNICAST_MODE);
+ } else if (!netdev_mc_empty(priv->netdev)) {
+ lan865x_set_specific_multicast_addr(priv->netdev);
+ regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
+ regval &= (~MAC_NET_CFG_MULTICAST_MODE);
+ regval |= MAC_NET_CFG_UNICAST_MODE;
+ } else {
+ /* enabling local mac address only */
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH,
+ regval)) {
+ netdev_err(priv->netdev, "Failed to write reg_hashh");
+ return;
+ }
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_L_HASH,
+ regval)) {
+ netdev_err(priv->netdev, "Failed to write reg_hashl");
+ return;
+ }
+ }
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CFG, regval))
+ netdev_err(priv->netdev,
+ "Failed to enable promiscuous/multicast/normal mode");
+}
+
+static void lan865x_set_multicast_list(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+
+ schedule_work(&priv->multicast_work);
+}
+
+static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
+ struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+
+ return oa_tc6_start_xmit(priv->tc6, skb);
+}
+
+static int lan865x_hw_disable(struct lan865x_priv *priv)
+{
+ u32 regval;
+
+ if (oa_tc6_read_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, ®val))
+ return -ENODEV;
+
+ regval &= ~(MAC_NET_CTL_TXEN | MAC_NET_CTL_RXEN);
+
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, regval))
+ return -ENODEV;
+
+ return 0;
+}
+
+static int lan865x_net_close(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ int ret;
+
+ netif_stop_queue(netdev);
+ phy_stop(netdev->phydev);
+ ret = lan865x_hw_disable(priv);
+ if (ret) {
+ netdev_err(netdev, "Failed to disable the hardware: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int lan865x_hw_enable(struct lan865x_priv *priv)
+{
+ u32 regval;
+
+ if (oa_tc6_read_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, ®val))
+ return -ENODEV;
+
+ regval |= MAC_NET_CTL_TXEN | MAC_NET_CTL_RXEN;
+
+ if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CTL, regval))
+ return -ENODEV;
+
+ return 0;
+}
+
+static int lan865x_net_open(struct net_device *netdev)
+{
+ struct lan865x_priv *priv = netdev_priv(netdev);
+ int ret;
+
+ ret = lan865x_hw_enable(priv);
+ if (ret) {
+ netdev_err(netdev, "Failed to enable hardware: %d\n", ret);
+ return ret;
+ }
+
+ phy_start(netdev->phydev);
+
+ return 0;
+}
+
+static const struct net_device_ops lan865x_netdev_ops = {
+ .ndo_open = lan865x_net_open,
+ .ndo_stop = lan865x_net_close,
+ .ndo_start_xmit = lan865x_send_packet,
+ .ndo_set_rx_mode = lan865x_set_multicast_list,
+ .ndo_set_mac_address = lan865x_set_mac_address,
+};
+
+static int lan865x_probe(struct spi_device *spi)
+{
+ struct net_device *netdev;
+ struct lan865x_priv *priv;
+ int ret;
+
+ netdev = alloc_etherdev(sizeof(struct lan865x_priv));
+ if (!netdev)
+ return -ENOMEM;
+
+ priv = netdev_priv(netdev);
+ priv->netdev = netdev;
+ priv->spi = spi;
+ spi_set_drvdata(spi, priv);
+ INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
+
+ priv->tc6 = oa_tc6_init(spi, netdev);
+ if (!priv->tc6) {
+ ret = -ENODEV;
+ goto free_netdev;
+ }
+
+ /* Get the MAC address from the SPI device tree node */
+ if (device_get_ethdev_address(&spi->dev, netdev))
+ eth_hw_addr_random(netdev);
+
+ ret = lan865x_set_hw_macaddr(priv, netdev->dev_addr);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to configure MAC");
+ goto oa_tc6_exit;
+ }
+
+ netdev->if_port = IF_PORT_10BASET;
+ netdev->irq = spi->irq;
+ netdev->netdev_ops = &lan865x_netdev_ops;
+ netdev->ethtool_ops = &lan865x_ethtool_ops;
+
+ ret = register_netdev(netdev);
+ if (ret) {
+ dev_err(&spi->dev, "Register netdev failed (ret = %d)", ret);
+ goto oa_tc6_exit;
+ }
+
+ return 0;
+
+oa_tc6_exit:
+ oa_tc6_exit(priv->tc6);
+free_netdev:
+ free_netdev(priv->netdev);
+ return ret;
+}
+
+static void lan865x_remove(struct spi_device *spi)
+{
+ struct lan865x_priv *priv = spi_get_drvdata(spi);
+
+ cancel_work_sync(&priv->multicast_work);
+ unregister_netdev(priv->netdev);
+ oa_tc6_exit(priv->tc6);
+ free_netdev(priv->netdev);
+}
+
+static const struct of_device_id lan865x_dt_ids[] = {
+ { .compatible = "microchip,lan8650" },
+ { .compatible = "microchip,lan8651" },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
+
+static struct spi_driver lan865x_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .of_match_table = lan865x_dt_ids,
+ },
+ .probe = lan865x_probe,
+ .remove = lan865x_remove,
+};
+module_spi_driver(lan865x_driver);
+
+MODULE_DESCRIPTION(DRV_NAME " 10Base-T1S MACPHY Ethernet Driver");
+MODULE_AUTHOR("Parthiban Veerasooran <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.34.1
This will unmask the following error interrupts from the MAC-PHY.
tx protocol error
rx buffer overflow error
loss of frame error
header error
The MAC-PHY will signal an error by setting the EXST bit in the receive
data footer which will then allow the host to read the STATUS0 register
find the source of the error.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index e9ddc4ff7d0d..f8593b793291 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -18,6 +18,13 @@
#define OA_TC6_REG_STATUS0 0x0008
#define STATUS0_RESETC BIT(6) /* Reset Complete */
+/* Interrupt Mask Register #0 */
+#define OA_TC6_REG_INT_MASK0 0x000C
+#define INT_MASK0_HEADER_ERR_MASK BIT(5)
+#define INT_MASK0_LOSS_OF_FRAME_ERR_MASK BIT(4)
+#define INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK BIT(3)
+#define INT_MASK0_TX_PROTOCOL_ERR_MASK BIT(0)
+
/* Control command header */
#define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
#define OA_TC6_CTRL_HEADER_WRITE BIT(29)
@@ -324,6 +331,23 @@ static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
}
+static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
+{
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_INT_MASK0, ®val);
+ if (ret)
+ return ret;
+
+ regval &= ~(INT_MASK0_TX_PROTOCOL_ERR_MASK |
+ INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK |
+ INT_MASK0_LOSS_OF_FRAME_ERR_MASK |
+ INT_MASK0_HEADER_ERR_MASK);
+
+ return oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
+}
+
/**
* oa_tc6_init - allocates and initializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
@@ -364,6 +388,13 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
return NULL;
}
+ ret = oa_tc6_unmask_macphy_error_interrupts(tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev,
+ "MAC-PHY error interrupts unmask failed: %d\n", ret);
+ return NULL;
+ }
+
return tc6;
}
EXPORT_SYMBOL_GPL(oa_tc6_init);
--
2.34.1
Reset complete bit is set when the MAC-PHY reset completes and ready for
configuration. Additionally reset complete bit in the STS0 register has
to be written by one upon reset complete to clear the interrupt.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 53 +++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 35e377577ba4..e9ddc4ff7d0d 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -6,8 +6,18 @@
*/
#include <linux/bitfield.h>
+#include <linux/iopoll.h>
#include <linux/oa_tc6.h>
+/* OPEN Alliance TC6 registers */
+/* Reset Control and Status Register */
+#define OA_TC6_REG_RESET 0x0003
+#define RESET_SWRESET BIT(0) /* Software Reset */
+
+/* Status Register #0 */
+#define OA_TC6_REG_STATUS0 0x0008
+#define STATUS0_RESETC BIT(6) /* Reset Complete */
+
/* Control command header */
#define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
#define OA_TC6_CTRL_HEADER_WRITE BIT(29)
@@ -24,6 +34,8 @@
(OA_TC6_CTRL_MAX_REGISTERS *\
OA_TC6_CTRL_REG_VALUE_SIZE) +\
OA_TC6_CTRL_IGNORED_SIZE)
+#define STATUS0_RESETC_POLL_DELAY 5
+#define STATUS0_RESETC_POLL_TIMEOUT 100
/* Internal structure for MAC-PHY drivers */
struct oa_tc6 {
@@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
}
EXPORT_SYMBOL_GPL(oa_tc6_write_register);
+static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
+{
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val);
+ if (ret)
+ return 0;
+
+ return regval;
+}
+
+static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
+{
+ u32 regval = RESET_SWRESET;
+ int ret;
+
+ ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval);
+ if (ret)
+ return ret;
+
+ /* Poll for soft reset complete for every 5us until 100us timeout */
+ ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval,
+ regval & STATUS0_RESETC,
+ STATUS0_RESETC_POLL_DELAY,
+ STATUS0_RESETC_POLL_TIMEOUT);
+ if (ret)
+ return -ENODEV;
+
+ /* Clear the reset complete status */
+ return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
+}
+
/**
* oa_tc6_init - allocates and initializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
@@ -289,6 +334,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_write_register);
struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
{
struct oa_tc6 *tc6;
+ int ret;
tc6 = devm_kzalloc(&spi->dev, sizeof(*tc6), GFP_KERNEL);
if (!tc6)
@@ -311,6 +357,13 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
if (!tc6->spi_ctrl_rx_buf)
return NULL;
+ ret = oa_tc6_sw_reset_macphy(tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev,
+ "MAC-PHY software reset failed: %d\n", ret);
+ return NULL;
+ }
+
return tc6;
}
EXPORT_SYMBOL_GPL(oa_tc6_init);
--
2.34.1
Implement register write operation according to the control communication
specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface
document. Control write commands are used by the SPI host to write
registers within the MAC-PHY. Each control write commands are composed of
a 32 bits control command header followed by register write data.
The MAC-PHY ignores the final 32 bits of data from the SPI host at the
end of the control write command. 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 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. Writing to
any unimplemented or undefined registers shall be ignored and yield no
effect.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
MAINTAINERS | 2 +
drivers/net/ethernet/Kconfig | 15 +++
drivers/net/ethernet/Makefile | 1 +
drivers/net/ethernet/oa_tc6.c | 240 ++++++++++++++++++++++++++++++++++
include/linux/oa_tc6.h | 17 +++
5 files changed, 275 insertions(+)
create mode 100644 drivers/net/ethernet/oa_tc6.c
create mode 100644 include/linux/oa_tc6.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 79fa7abb4ec9..603528948f61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16405,6 +16405,8 @@ 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]>
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 6a19b5393ed1..b3acf21233e7 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -157,6 +157,21 @@ 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.
+
+ To know the implementation details, refer documentation in
+ <file:Documentation/networking/oa-tc6-framework.rst>.
+
+ 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..82131f865fe7
--- /dev/null
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -0,0 +1,240 @@
+// 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>
+
+/* Control command header */
+#define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
+#define OA_TC6_CTRL_HEADER_WRITE BIT(29)
+#define OA_TC6_CTRL_HEADER_MEM_MAP_SELECTOR GENMASK(27, 24)
+#define OA_TC6_CTRL_HEADER_ADDR GENMASK(23, 8)
+#define OA_TC6_CTRL_HEADER_LENGTH GENMASK(7, 1)
+#define OA_TC6_CTRL_HEADER_PARITY BIT(0)
+
+#define OA_TC6_CTRL_HEADER_SIZE 4
+#define OA_TC6_CTRL_REG_VALUE_SIZE 4
+#define OA_TC6_CTRL_IGNORED_SIZE 4
+#define OA_TC6_CTRL_MAX_REGISTERS 128
+#define OA_TC6_CTRL_SPI_BUF_SIZE (OA_TC6_CTRL_HEADER_SIZE +\
+ (OA_TC6_CTRL_MAX_REGISTERS *\
+ OA_TC6_CTRL_REG_VALUE_SIZE) +\
+ OA_TC6_CTRL_IGNORED_SIZE)
+
+/* Internal structure for MAC-PHY drivers */
+struct oa_tc6 {
+ struct spi_device *spi;
+ struct mutex spi_ctrl_lock; /* Protects spi control transfer */
+ void *spi_ctrl_tx_buf;
+ void *spi_ctrl_rx_buf;
+};
+
+enum oa_tc6_header_type {
+ OA_TC6_CTRL_HEADER,
+};
+
+enum oa_tc6_register_op {
+ OA_TC6_CTRL_REG_WRITE = 1,
+};
+
+static int oa_tc6_spi_transfer(struct oa_tc6 *tc6,
+ enum oa_tc6_header_type header_type, u16 length)
+{
+ struct spi_transfer xfer = { 0 };
+ struct spi_message msg;
+
+ xfer.tx_buf = tc6->spi_ctrl_tx_buf;
+ xfer.rx_buf = tc6->spi_ctrl_rx_buf;
+ xfer.len = length;
+
+ spi_message_init(&msg);
+ spi_message_add_tail(&xfer, &msg);
+
+ return spi_sync(tc6->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 __be32 oa_tc6_prepare_ctrl_header(u32 address, u8 length,
+ enum oa_tc6_register_op reg_op)
+{
+ u32 header;
+
+ header = FIELD_PREP(OA_TC6_CTRL_HEADER_DATA_NOT_CTRL,
+ OA_TC6_CTRL_HEADER) |
+ FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op) |
+ FIELD_PREP(OA_TC6_CTRL_HEADER_MEM_MAP_SELECTOR, address >> 16) |
+ FIELD_PREP(OA_TC6_CTRL_HEADER_ADDR, address) |
+ FIELD_PREP(OA_TC6_CTRL_HEADER_LENGTH, length - 1);
+ header |= FIELD_PREP(OA_TC6_CTRL_HEADER_PARITY,
+ oa_tc6_get_parity(header));
+
+ return cpu_to_be32(header);
+}
+
+static void oa_tc6_update_ctrl_write_data(struct oa_tc6 *tc6, u32 value[],
+ u8 length)
+{
+ __be32 *tx_buf = tc6->spi_ctrl_tx_buf + OA_TC6_CTRL_HEADER_SIZE;
+
+ for (int i = 0; i < length; i++)
+ *tx_buf++ = cpu_to_be32(value[i]);
+}
+
+static u16 oa_tc6_calculate_ctrl_buf_size(u8 length)
+{
+ /* Control command consists 4 bytes header + 4 bytes register value for
+ * each register + 4 bytes ignored value.
+ */
+ return OA_TC6_CTRL_HEADER_SIZE + OA_TC6_CTRL_REG_VALUE_SIZE * length +
+ OA_TC6_CTRL_IGNORED_SIZE;
+}
+
+static void oa_tc6_prepare_ctrl_spi_buf(struct oa_tc6 *tc6, u32 address,
+ u32 value[], u8 length,
+ enum oa_tc6_register_op reg_op)
+{
+ __be32 *tx_buf = tc6->spi_ctrl_tx_buf;
+
+ *tx_buf = oa_tc6_prepare_ctrl_header(address, length, reg_op);
+
+ oa_tc6_update_ctrl_write_data(tc6, value, length);
+}
+
+static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size)
+{
+ u8 *tx_buf = tc6->spi_ctrl_tx_buf;
+ u8 *rx_buf = tc6->spi_ctrl_rx_buf;
+
+ rx_buf += OA_TC6_CTRL_IGNORED_SIZE;
+
+ /* The echoed control write must match with the one that was
+ * transmitted.
+ */
+ if (memcmp(tx_buf, rx_buf, size - OA_TC6_CTRL_IGNORED_SIZE))
+ return -ENODEV;
+
+ return 0;
+}
+
+static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[],
+ u8 length, enum oa_tc6_register_op reg_op)
+{
+ u16 size;
+ int ret;
+
+ /* Prepare control command and copy to SPI control buffer */
+ oa_tc6_prepare_ctrl_spi_buf(tc6, address, value, length, reg_op);
+
+ size = oa_tc6_calculate_ctrl_buf_size(length);
+
+ /* Perform SPI transfer */
+ ret = oa_tc6_spi_transfer(tc6, OA_TC6_CTRL_HEADER, size);
+ if (ret) {
+ dev_err(&tc6->spi->dev, "SPI transfer failed for control: %d\n",
+ ret);
+ return ret;
+ }
+
+ /* Check echoed/received control write command reply for errors */
+ return oa_tc6_check_ctrl_write_reply(tc6, size);
+}
+
+/**
+ * oa_tc6_write_registers - function for writing multiple consecutive registers.
+ * @tc6: oa_tc6 struct.
+ * @address: address of the first register to be written in the MAC-PHY.
+ * @value: values to be written from the starting register address @address.
+ * @length: number of consecutive registers to be written from @address.
+ *
+ * Maximum of 128 consecutive registers can be written starting at @address.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
+ u8 length)
+{
+ int ret;
+
+ if (!length || length > OA_TC6_CTRL_MAX_REGISTERS) {
+ dev_err(&tc6->spi->dev, "Invalid register length parameter\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&tc6->spi_ctrl_lock);
+ ret = oa_tc6_perform_ctrl(tc6, address, value, length,
+ OA_TC6_CTRL_REG_WRITE);
+ mutex_unlock(&tc6->spi_ctrl_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(oa_tc6_write_registers);
+
+/**
+ * oa_tc6_write_register - function for writing a MAC-PHY register.
+ * @tc6: oa_tc6 struct.
+ * @address: register address of the MAC-PHY to be written.
+ * @value: value to be written in the @address register address of the MAC-PHY.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
+{
+ return oa_tc6_write_registers(tc6, address, &value, 1);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_write_register);
+
+/**
+ * oa_tc6_init - allocates and initializes oa_tc6 structure.
+ * @spi: device with which data will be exchanged.
+ *
+ * Returns pointer reference to the oa_tc6 structure if the MAC-PHY
+ * initialization is successful otherwise NULL.
+ */
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
+{
+ struct oa_tc6 *tc6;
+
+ tc6 = devm_kzalloc(&spi->dev, sizeof(*tc6), GFP_KERNEL);
+ if (!tc6)
+ return NULL;
+
+ tc6->spi = spi;
+ mutex_init(&tc6->spi_ctrl_lock);
+
+ /* Set the SPI controller to pump at realtime priority */
+ tc6->spi->rt = true;
+ spi_setup(tc6->spi);
+
+ tc6->spi_ctrl_tx_buf = devm_kzalloc(&tc6->spi->dev,
+ OA_TC6_CTRL_SPI_BUF_SIZE, GFP_KERNEL);
+ if (!tc6->spi_ctrl_tx_buf)
+ return NULL;
+
+ tc6->spi_ctrl_rx_buf = devm_kzalloc(&tc6->spi->dev,
+ OA_TC6_CTRL_SPI_BUF_SIZE, GFP_KERNEL);
+ if (!tc6->spi_ctrl_rx_buf)
+ return NULL;
+
+ 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..99c490f1c8a8
--- /dev/null
+++ b/include/linux/oa_tc6.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface framework
+ *
+ * Link: https://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf
+ *
+ * Author: Parthiban Veerasooran <[email protected]>
+ */
+
+#include <linux/spi/spi.h>
+
+struct oa_tc6;
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
+int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value);
+int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
+ u8 length);
--
2.34.1
Enabling Configuration Synchronization bit (SYNC) in the Configuration
Register #0 enables data communication in the MAC-PHY. The state of this
bit is reflected in the data footer SYNC bit.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 82b4de13438f..371687670409 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -20,6 +20,10 @@
#define OA_TC6_REG_RESET 0x0003
#define RESET_SWRESET BIT(0) /* Software Reset */
+/* Configuration Register #0 */
+#define OA_TC6_REG_CONFIG0 0x0004
+#define CONFIG0_SYNC BIT(15)
+
/* Status Register #0 */
#define OA_TC6_REG_STATUS0 0x0008
#define STATUS0_RESETC BIT(6) /* Reset Complete */
@@ -486,6 +490,21 @@ static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
return oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
}
+static int oa_tc6_enable_data_transfer(struct oa_tc6 *tc6)
+{
+ u32 value;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_CONFIG0, &value);
+ if (ret)
+ return ret;
+
+ /* Enable configuration synchronization for data transfer */
+ value |= CONFIG0_SYNC;
+
+ return oa_tc6_write_register(tc6, OA_TC6_REG_CONFIG0, value);
+}
+
/**
* oa_tc6_init - allocates and initializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
@@ -543,7 +562,18 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
return NULL;
}
+ ret = oa_tc6_enable_data_transfer(tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev, "Failed to enable data transfer: %d\n",
+ ret);
+ goto phy_exit;
+ }
+
return tc6;
+
+phy_exit:
+ oa_tc6_phy_exit(tc6);
+ return NULL;
}
EXPORT_SYMBOL_GPL(oa_tc6_init);
--
2.34.1
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 | 80 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 81 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..ee52f9d8e93c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
@@ -0,0 +1,80 @@
+# 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).
+
+allOf:
+ - $ref: ethernet-controller.yaml#
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - const: microchip,lan8650
+ - const: microchip,lan8651
+ - enum:
+ - microchip,lan8650
+
+ 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
+
+ spi-max-frequency:
+ minimum: 15000000
+ maximum: 25000000
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - spi-max-frequency
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethernet@0 {
+ compatible = "microchip,lan8650";
+ 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>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index f41b7f2257d2..2172431a1935 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14378,6 +14378,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/lan865x.c
MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
--
2.34.1
> +NORX (Bit 29) - No Receive flag. The SPI host may set this bit to prevent
> + the MAC-PHY from conveying RX data on the MISO for the
> + current chunk (DV = 0 in the footer), indicating that the
> + host would not process it. Typically, the SPI host should
> + set NORX = 0 indicating that it will accept and process
> + any receive frame data within the current chunk.
> +
> +RSVD (Bit 28..24) - Reserved: All reserved bits shall be ‘0’.
> +
> +VS (Bit 23..22) - Vendor Specific. These bits are implementation specific.
> + If the MAC-PHY does not implement these bits, the host
> + shall set them to ‘0’.
> +
> +DV (Bit 21) - Data Valid flag. The SPI host uses this bit to indicate
> + whether the current chunk contains valid transmit frame data
> + (DV = 1) or not (DV = 0). When ‘0’, the MAC-PHY ignores the
> + chunk payload. Note that the receive path is unaffected by
> + the setting of the DV bit in the data header.
There is some odd indentation going on here. Maybe tab vs spaces?
Otherwise, this is nice documentation. Thanks.
Andrew
> +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.
> +
> + To know the implementation details, refer documentation in
> + <file:Documentation/networking/oa-tc6-framework.rst>.
> +
> + 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.
We generally don't refer to out of tree modules. We know they exist,
but we don't take any steps to support them, the internal APIs are not
fixed etc. So i would drop this last paragraph.
> +static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size)
> +{
> + u8 *tx_buf = tc6->spi_ctrl_tx_buf;
> + u8 *rx_buf = tc6->spi_ctrl_rx_buf;
> +
> + rx_buf += OA_TC6_CTRL_IGNORED_SIZE;
> +
> + /* The echoed control write must match with the one that was
> + * transmitted.
> + */
> + if (memcmp(tx_buf, rx_buf, size - OA_TC6_CTRL_IGNORED_SIZE))
> + return -ENODEV;
> +
I think EPROTO or EIO would be better. The device might have crashed,
burned and is gone, but isn't a bit flip on the SPI bus more likely?
Andrew
On Wed, Mar 06, 2024 at 02:20:17PM +0530, Parthiban Veerasooran wrote:
> 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 | 80 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 81 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..ee52f9d8e93c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
> @@ -0,0 +1,80 @@
> +# 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).
> +
> +allOf:
> + - $ref: ethernet-controller.yaml#
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - const: microchip,lan8650
> + - const: microchip,lan8651
The order here is wrong, lan8561 needs to come before the fallback of
lan8650.
> + - enum:
> + - microchip,lan8650
> +
> + 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
> +
> + spi-max-frequency:
> + minimum: 15000000
> + maximum: 25000000
You're missing a reference to spi-peripheral-props where this property
is defined.
Thanks,
Conor.
> > +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).
> > +
> > +allOf:
> > + - $ref: ethernet-controller.yaml#
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - items:
> > + - const: microchip,lan8650
> > + - const: microchip,lan8651
>
> The order here is wrong, lan8561 needs to come before the fallback of
> lan8650.
I don't think it is a fallback. There are two devices, and hence two
different compatibles. So i suspect the -items: is wrong here?
Andrew
On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
> > > +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).
> > > +
> > > +allOf:
> > > + - $ref: ethernet-controller.yaml#
> > > +
> > > +properties:
> > > + compatible:
> > > + oneOf:
> > > + - items:
> > > + - const: microchip,lan8650
> > > + - const: microchip,lan8651
> >
> > The order here is wrong, lan8561 needs to come before the fallback of
> > lan8650.
>
> I don't think it is a fallback. There are two devices, and hence two
> different compatibles. So i suspect the -items: is wrong here?
It'd just be a two entry enum then, but I did take a quick look at the
driver earlier and saw:
+static const struct of_device_id lan865x_dt_ids[] = {
+ { .compatible = "microchip,lan8650" },
+ { .compatible = "microchip,lan8651" },
+ { /* Sentinel */ }
+};
That, along with no other of_device_is_compatible() type operations
made me think that having a fallback actually was suitable.
You cropped it out, but the patch had:
> + compatible:
> + oneOf:
> + - items:
> + - const: microchip,lan8650
> + - const: microchip,lan8651
> + - enum:
> + - microchip,lan8650
So it doesn't appear to be an accidental items in place of an enum,
since the other compatible is in another enum.
I just noticed also that that enum should actually be const, so I'd
expect this to be:
compatible:
oneOf:
- items:
- const: microchip,lan8651
- const: microchip,lan8650
- const: microchip,lan8650
Hi Parthiban,
..
> +static irqreturn_t oa_tc6_macphy_isr(int irq, void *data)
> +{
> + struct oa_tc6 *tc6 = data;
> +
> + /* MAC-PHY interrupt can occur for the following reasons.
> + * - availability of tx credits if it was 0 before and not reported
> in
> + * the previous rx footer.
Per description above, it may be typo of "the previous rx footer"
> + * - availability of rx chunks if it was 0 before and not reported
> in
> + * the previous rx footer.
> + * - extended status event not reported in the previous rx footer.
> + */
> + tc6->int_flag = true;
> + /* Wake spi kthread to perform spi transfer */
> + wake_up_interruptible(&tc6->spi_wq);
> +
> + return IRQ_HANDLED;
> +}
> +
Hi Parthiban,
> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> b/drivers/net/ethernet/microchip/lan865x/lan865x.c
..
> +static void lan865x_multicast_work_handler(struct work_struct *work)
> +{
> + struct lan865x_priv *priv = container_of(work, struct lan865x_priv,
> + multicast_work);
> + u32 regval = 0;
> +
> + if (priv->netdev->flags & IFF_PROMISC) {
> + /* Enabling promiscuous mode */
> + regval |= MAC_NET_CFG_PROMISCUOUS_MODE;
> + regval &= (~MAC_NET_CFG_MULTICAST_MODE);
> + regval &= (~MAC_NET_CFG_UNICAST_MODE);
> + } else if (priv->netdev->flags & IFF_ALLMULTI) {
> + /* Enabling all multicast mode */
> + regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
> + regval |= MAC_NET_CFG_MULTICAST_MODE;
> + regval &= (~MAC_NET_CFG_UNICAST_MODE);
> + } else if (!netdev_mc_empty(priv->netdev)) {
> + lan865x_set_specific_multicast_addr(priv->netdev);
> + regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
> + regval &= (~MAC_NET_CFG_MULTICAST_MODE);
> + regval |= MAC_NET_CFG_UNICAST_MODE;
> + } else {
> + /* enabling local mac address only */
> + if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH,
> + regval)) {
Your intention to write 0 into LAN865X_REG_MAC_H_HASH?
If then, using 0 than regval makes more clear.
> + netdev_err(priv->netdev, "Failed to write reg_hashh");
> + return;
> + }
> + if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_L_HASH,
> + regval)) {
Same here.
> + netdev_err(priv->netdev, "Failed to write reg_hashl");
> + return;
> + }
> + }
> + if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CFG,
> regval))
> + netdev_err(priv->netdev,
> + "Failed to enable promiscuous/multicast/normal mode");
> +}
> +
> +/* Status Register #0 */
> +#define OA_TC6_REG_STATUS0 0x0008
> +#define STATUS0_RESETC BIT(6) /* Reset Complete */
> +
> /* Control command header */
> #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
> #define OA_TC6_CTRL_HEADER_WRITE BIT(29)
> @@ -24,6 +34,8 @@
> (OA_TC6_CTRL_MAX_REGISTERS *\
> OA_TC6_CTRL_REG_VALUE_SIZE) +\
> OA_TC6_CTRL_IGNORED_SIZE)
> +#define STATUS0_RESETC_POLL_DELAY 5
> +#define STATUS0_RESETC_POLL_TIMEOUT 100
>
> /* Internal structure for MAC-PHY drivers */
> struct oa_tc6 {
> @@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
> }
> EXPORT_SYMBOL_GPL(oa_tc6_write_register);
>
> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
> +{
> + u32 regval;
> + int ret;
> +
> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val);
> + if (ret)
> + return 0;
> +
> + return regval;
The function name does not really fit what the function does. The
function returns OA_TC6_REG_STATUS0. I assume it has more bits in it
than just STATUS0_RESETC. So either this function should be called
oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so
that it does actually return the sw reset status.
> +static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
> +{
> + u32 regval = RESET_SWRESET;
> + int ret;
> +
> + ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval);
> + if (ret)
> + return ret;
> +
> + /* Poll for soft reset complete for every 5us until 100us timeout */
Is this 100us defined in the standard? It is pretty short. If it is
not part of the standard, maybe set it to something much bigger?
Also, polling every 5us is pretty quick. I doubt most systems can even
sleep that short a time. So maybe 1ms between polls, and 1 second
before -ETIMEDOUT?
> + ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval,
> + regval & STATUS0_RESETC,
> + STATUS0_RESETC_POLL_DELAY,
> + STATUS0_RESETC_POLL_TIMEOUT);
> + if (ret)
> + return -ENODEV;
> +
> + /* Clear the reset complete status */
> + return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
Andrew
On Wed, Mar 06, 2024 at 02:20:10PM +0530, Parthiban Veerasooran wrote:
> This will unmask the following error interrupts from the MAC-PHY.
> tx protocol error
> rx buffer overflow error
> loss of frame error
The standard seems to call it "Loss of framing". To me as a network
person, a frame is an L2 Ethernet packet. However, framing is
something completely different, being able to synchronise a bitstream
with some sort of markers, e.g. E1 to know where the 32 slots are.
In this context, we are not talking about packets, but the SPI bit
stream. So it would be good to use framing, not frame.
Andrew
> +/* PHY Clause 22 and 29 registers base address and mask */
> +#define OA_TC6_PHY_STD_REG_ADDR_BASE 0xFF00
> +#define OA_TC6_PHY_STD_REG_ADDR_MASK 0x3F
[Goes and looks a 802.3]
Clause 29 is "System considerations for multisegment 100BASE-T networks"
I don't see any mention of registers in there.
TC6 says:
"Clause 22 standard registers and Clause 22 extended registers (Clause
29) are directly mapped into MMS 0 as shown in Table 7."
Going back to 802.3, we have 22.2.4:
The MII basic register set consists of two registers referred to as
the Control register (Register 0) and the Status register (Register
1). All PHYs that provide an MII Management Interface shall
incorporate the basic register set. All PHYs that provide a GMII shall
incorporate an extended basic register set consisting of the Control
register (Register 0), Status register (Register 1), and Extended
Status register (Register 15). The status and control functions
defined here are considered basic and fundamental to 100 Mb/s and 1000
Mb/s PHYs. Registers 2 through 14 are part of the extended register
set. The format of Registers 4 through 10 are defined for the specific
Auto-Negotiation protocol used (Clause 28 or Clause 37). The format of
these registers is selected by the bit settings of Registers 1 and 15.
So clause 29 is not making much sense here. Can anybody explain it?
> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
> +{
> + int ret;
> +
> + tc6->mdiobus = mdiobus_alloc();
> + if (!tc6->mdiobus) {
> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> + return -ENODEV;
> + }
> +
> + tc6->mdiobus->priv = tc6;
> + tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
> + tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
This might get answered in later patches. PLCA registers are in C45
address space, VEND1 if i remember correctly. You don't provide any
C45 access methods here. Does TC6 specify that C45 over C22 must be
implemented?
The standard does say:
Vendor specific registers may be mapped into MMS 10 though MMS
15. When directly mapped, PHY vendor specific registers in MMD 30 or
MMD 31 would be mapped into the vendor specific MMS 10 through MMS 15.
So i'm thinking you might need to provide C45 access, at least MMD 30,
via MMS 10-15?
Andrew
Hi Andrew,
Thanks for reviewing the patches.
On 06/03/24 6:53 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +NORX (Bit 29) - No Receive flag. The SPI host may set this bit to prevent
>> + the MAC-PHY from conveying RX data on the MISO for the
>> + current chunk (DV = 0 in the footer), indicating that the
>> + host would not process it. Typically, the SPI host should
>> + set NORX = 0 indicating that it will accept and process
>> + any receive frame data within the current chunk.
>> +
>> +RSVD (Bit 28..24) - Reserved: All reserved bits shall be ‘0’.
>> +
>> +VS (Bit 23..22) - Vendor Specific. These bits are implementation specific.
>> + If the MAC-PHY does not implement these bits, the host
>> + shall set them to ‘0’.
>> +
>> +DV (Bit 21) - Data Valid flag. The SPI host uses this bit to indicate
>> + whether the current chunk contains valid transmit frame data
>> + (DV = 1) or not (DV = 0). When ‘0’, the MAC-PHY ignores the
>> + chunk payload. Note that the receive path is unaffected by
>> + the setting of the DV bit in the data header.
>
> There is some odd indentation going on here. Maybe tab vs spaces?
Yes, there is a mixture of tab and spaces. I will correct this
indentation in the next version.
>
> Otherwise, this is nice documentation. Thanks.
Thanks.
Best regards,
Parthiban V
>
> Andrew
Hi Andrew,
On 06/03/24 7:10 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +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.
>> +
>> + To know the implementation details, refer documentation in
>> + <file:Documentation/networking/oa-tc6-framework.rst>.
>> +
>> + 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.
>
> We generally don't refer to out of tree modules. We know they exist,
> but we don't take any steps to support them, the internal APIs are not
> fixed etc. So i would drop this last paragraph.
Ah ok, sure I will drop this last three lines paragraph in the next version.
>
>> +static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size)
>> +{
>> + u8 *tx_buf = tc6->spi_ctrl_tx_buf;
>> + u8 *rx_buf = tc6->spi_ctrl_rx_buf;
>> +
>> + rx_buf += OA_TC6_CTRL_IGNORED_SIZE;
>> +
>> + /* The echoed control write must match with the one that was
>> + * transmitted.
>> + */
>> + if (memcmp(tx_buf, rx_buf, size - OA_TC6_CTRL_IGNORED_SIZE))
>> + return -ENODEV;
>> +
>
> I think EPROTO or EIO would be better. The device might have crashed,
> burned and is gone, but isn't a bit flip on the SPI bus more likely?
Yes, it results bit flip in the SPI. So I think it leads to "Protocol
error". EPROTO would be a better option here. I will correct it in the
next version.
Best regards,
Parthiban V
>
> Andrew
>
Hi Andrew,
On 07/03/24 6:05 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +/* Status Register #0 */
>> +#define OA_TC6_REG_STATUS0 0x0008
>> +#define STATUS0_RESETC BIT(6) /* Reset Complete */
>> +
>> /* Control command header */
>> #define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
>> #define OA_TC6_CTRL_HEADER_WRITE BIT(29)
>> @@ -24,6 +34,8 @@
>> (OA_TC6_CTRL_MAX_REGISTERS *\
>> OA_TC6_CTRL_REG_VALUE_SIZE) +\
>> OA_TC6_CTRL_IGNORED_SIZE)
>> +#define STATUS0_RESETC_POLL_DELAY 5
>> +#define STATUS0_RESETC_POLL_TIMEOUT 100
>>
>> /* Internal structure for MAC-PHY drivers */
>> struct oa_tc6 {
>> @@ -279,6 +291,39 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
>> }
>> EXPORT_SYMBOL_GPL(oa_tc6_write_register);
>>
>> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
>> +{
>> + u32 regval;
>> + int ret;
>> +
>> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val);
>> + if (ret)
>> + return 0;
>> +
>> + return regval;
>
> The function name does not really fit what the function does. The
> function returns OA_TC6_REG_STATUS0. I assume it has more bits in it
> than just STATUS0_RESETC. So either this function should be called
> oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so
> that it does actually return the sw reset status.
Ok, as we do the masking in the calling function readx_poll_timeout, as
you suggested I will change the function name as oa_tc6_read_status0 in
the next version.
>
>> +static int oa_tc6_sw_reset_macphy(struct oa_tc6 *tc6)
>> +{
>> + u32 regval = RESET_SWRESET;
>> + int ret;
>> +
>> + ret = oa_tc6_write_register(tc6, OA_TC6_REG_RESET, regval);
>> + if (ret)
>> + return ret;
>> +
>> + /* Poll for soft reset complete for every 5us until 100us timeout */
>
> Is this 100us defined in the standard? It is pretty short. If it is
> not part of the standard, maybe set it to something much bigger?
No, the standard doesn't define this and it is not part of the standard.
>
> Also, polling every 5us is pretty quick. I doubt most systems can even
> sleep that short a time. So maybe 1ms between polls, and 1 second
> before -ETIMEDOUT?
Ok, I agree with your proposal and will change the poll delay as 1ms
with the poll timeout of 1 second.
Best regards,
Parthiban V
>
>> + ret = readx_poll_timeout(oa_tc6_read_sw_reset_status, tc6, regval,
>> + regval & STATUS0_RESETC,
>> + STATUS0_RESETC_POLL_DELAY,
>> + STATUS0_RESETC_POLL_TIMEOUT);
>> + if (ret)
>> + return -ENODEV;
>> +
>> + /* Clear the reset complete status */
>> + return oa_tc6_write_register(tc6, OA_TC6_REG_STATUS0, regval);
>
> Andrew
>
Hi Andrew,
On 07/03/24 6:13 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, Mar 06, 2024 at 02:20:10PM +0530, Parthiban Veerasooran wrote:
>> This will unmask the following error interrupts from the MAC-PHY.
>> tx protocol error
>> rx buffer overflow error
>> loss of frame error
>
> The standard seems to call it "Loss of framing". To me as a network
> person, a frame is an L2 Ethernet packet. However, framing is
> something completely different, being able to synchronise a bitstream
> with some sort of markers, e.g. E1 to know where the 32 slots are.
>
> In this context, we are not talking about packets, but the SPI bit
> stream. So it would be good to use framing, not frame.
Yes you are right. The specification also says the same thing as you
said "Loss of Framing Error". I will correct it in the next version.
Best regards,
Parthiban V
>
> Andrew
>
Hi Woojung,
On 07/03/24 5:14 am, Woojung Huh - C21699 wrote:
> Hi Parthiban,
>
>> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c
>> b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> ...
>> +static void lan865x_multicast_work_handler(struct work_struct *work)
>> +{
>> + struct lan865x_priv *priv = container_of(work, struct lan865x_priv,
>> + multicast_work);
>> + u32 regval = 0;
>> +
>> + if (priv->netdev->flags & IFF_PROMISC) {
>> + /* Enabling promiscuous mode */
>> + regval |= MAC_NET_CFG_PROMISCUOUS_MODE;
>> + regval &= (~MAC_NET_CFG_MULTICAST_MODE);
>> + regval &= (~MAC_NET_CFG_UNICAST_MODE);
>> + } else if (priv->netdev->flags & IFF_ALLMULTI) {
>> + /* Enabling all multicast mode */
>> + regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
>> + regval |= MAC_NET_CFG_MULTICAST_MODE;
>> + regval &= (~MAC_NET_CFG_UNICAST_MODE);
>> + } else if (!netdev_mc_empty(priv->netdev)) {
>> + lan865x_set_specific_multicast_addr(priv->netdev);
>> + regval &= (~MAC_NET_CFG_PROMISCUOUS_MODE);
>> + regval &= (~MAC_NET_CFG_MULTICAST_MODE);
>> + regval |= MAC_NET_CFG_UNICAST_MODE;
>> + } else {
>> + /* enabling local mac address only */
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH,
>> + regval)) {
>
> Your intention to write 0 into LAN865X_REG_MAC_H_HASH?
> If then, using 0 than regval makes more clear.
Yes sure, will correct it in the next version.
>
>> + netdev_err(priv->netdev, "Failed to write reg_hashh");
>> + return;
>> + }
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_L_HASH,
>> + regval)) {
>
> Same here.
Yes sure, will correct it in the next version.
Best regards,
Parthiban V
>
>> + netdev_err(priv->netdev, "Failed to write reg_hashl");
>> + return;
>> + }
>> + }
>> + if (oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_NET_CFG,
>> regval))
>> + netdev_err(priv->netdev,
>> + "Failed to enable promiscuous/multicast/normal mode");
>> +}
>> +
>
Hi Woojung,
On 07/03/24 5:12 am, Woojung Huh - C21699 wrote:
> Hi Parthiban,
>
> ...
>
>> +static irqreturn_t oa_tc6_macphy_isr(int irq, void *data)
>> +{
>> + struct oa_tc6 *tc6 = data;
>> +
>> + /* MAC-PHY interrupt can occur for the following reasons.
>> + * - availability of tx credits if it was 0 before and not reported
>> in
>> + * the previous rx footer.
>
> Per description above, it may be typo of "the previous rx footer"
Sorry, I don't get your comment. Could you please elaborate?
Best regards,
Parthiban V
>
>> + * - availability of rx chunks if it was 0 before and not reported
>> in
>> + * the previous rx footer.
>> + * - extended status event not reported in the previous rx footer.
>> + */
>> + tc6->int_flag = true;
>> + /* Wake spi kthread to perform spi transfer */
>> + wake_up_interruptible(&tc6->spi_wq);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>
> >> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
> >> +{
> >> + u32 regval;
> >> + int ret;
> >> +
> >> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val);
> >> + if (ret)
> >> + return 0;
> >> +
> >> + return regval;
> >
> > The function name does not really fit what the function does. The
> > function returns OA_TC6_REG_STATUS0. I assume it has more bits in it
> > than just STATUS0_RESETC. So either this function should be called
> > oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so
> > that it does actually return the sw reset status.
> Ok, as we do the masking in the calling function readx_poll_timeout, as
> you suggested I will change the function name as oa_tc6_read_status0 in
> the next version.
O.K. And i think some of the later patches can them make use of this
generic function.
Andrew
Hi Andrew,
On 07/03/24 6:43 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +/* PHY Clause 22 and 29 registers base address and mask */
>> +#define OA_TC6_PHY_STD_REG_ADDR_BASE 0xFF00
>> +#define OA_TC6_PHY_STD_REG_ADDR_MASK 0x3F
>
> [Goes and looks a 802.3]
>
> Clause 29 is "System considerations for multisegment 100BASE-T networks"
>
> I don't see any mention of registers in there.
>
> TC6 says:
>
> "Clause 22 standard registers and Clause 22 extended registers (Clause
> 29) are directly mapped into MMS 0 as shown in Table 7."
>
> Going back to 802.3, we have 22.2.4:
>
> The MII basic register set consists of two registers referred to as
> the Control register (Register 0) and the Status register (Register
> 1). All PHYs that provide an MII Management Interface shall
> incorporate the basic register set. All PHYs that provide a GMII shall
> incorporate an extended basic register set consisting of the Control
> register (Register 0), Status register (Register 1), and Extended
> Status register (Register 15). The status and control functions
> defined here are considered basic and fundamental to 100 Mb/s and 1000
> Mb/s PHYs. Registers 2 through 14 are part of the extended register
> set. The format of Registers 4 through 10 are defined for the specific
> Auto-Negotiation protocol used (Clause 28 or Clause 37). The format of
> these registers is selected by the bit settings of Registers 1 and 15.
>
> So clause 29 is not making much sense here. Can anybody explain it?
>
>> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
>> +{
>> + int ret;
>> +
>> + tc6->mdiobus = mdiobus_alloc();
>> + if (!tc6->mdiobus) {
>> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>> + return -ENODEV;
>> + }
>> +
>> + tc6->mdiobus->priv = tc6;
>> + tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
>> + tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
>
> This might get answered in later patches. PLCA registers are in C45
> address space, VEND1 if i remember correctly. You don't provide any
> C45 access methods here. Does TC6 specify that C45 over C22 must be
> implemented?
No the spec doesn't say anything like this. But, as C22 registers are
mapped in the MMS 0, registers 0xD and 0xE can be used to access C45
registers indirectly. That's why the driver implemented the above
functions. I agree that indirect access is slower and requires more
control commands than direct access. So implementing the direct access
of C45 registers will overcome this issue.
>
> The standard does say:
>
> Vendor specific registers may be mapped into MMS 10 though MMS
> 15. When directly mapped, PHY vendor specific registers in MMD 30 or
> MMD 31 would be mapped into the vendor specific MMS 10 through MMS 15.
>
> So i'm thinking you might need to provide C45 access, at least MMD 30,
> via MMS 10-15?
Thanks for this detailed comment. If understand you correctly by
consolidating all your above explanations, the driver should provide C45
access to the PHY vendor specific and PLCA registers (MMD 31). As per
the specification, Table 6 describes the Register Memory Map Selector
(MMS) Assignment. In this, MMS 4 maps the PHY vendor specific and PLCA
registers. They are in the MMD 31 address space as per spec. They can be
directly accessed using read_c45 and write_c45 functions in the mdio bus.
In Microchip's MAC-PHY (LAN8650), PHY – Vendor Specific and PLCA
Registers (MMD 31) mapped in the MMS 4 as per the table 6 in the spec.
There is no other PHY vendor specific registers are mapped in the MMS 10
through 15. No idea whether any other vendor's MAC-PHY uses MMS 10
through 15 to map PHY – Vendor Specific and PLCA Registers (MMD 31).
I have given the code below for the C45 access methods. Kindly check is
this something you expected?
--- Code starts ---
/* PHY – Vendor Specific and PLCA Registers (MMD 31) */
#define OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE 0x40000
,,,
static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
devnum, int regnum)
{
struct oa_tc6 *tc6 = bus->priv;
u32 regval;
bool ret;
ret = oa_tc6_read_register(tc6,
OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE | regnum, ®val);
if (ret)
return -ENODEV;
return regval;
}
static int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int
devnum, int regnum, u16 val)
{
struct oa_tc6 *tc6 = bus->priv;
return oa_tc6_write_register(tc6,
OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE | regnum, val);
}
,,,
tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45;
tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45;
--- Code ends ---
Best regrads,
Parthiban V
>
> Andrew
>
> >> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
> >> +{
> >> + int ret;
> >> +
> >> + tc6->mdiobus = mdiobus_alloc();
> >> + if (!tc6->mdiobus) {
> >> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + tc6->mdiobus->priv = tc6;
> >> + tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
> >> + tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
> >
> > This might get answered in later patches. PLCA registers are in C45
> > address space, VEND1 if i remember correctly. You don't provide any
> > C45 access methods here. Does TC6 specify that C45 over C22 must be
> > implemented?
> No the spec doesn't say anything like this. But, as C22 registers are
> mapped in the MMS 0, registers 0xD and 0xE can be used to access C45
> registers indirectly. That's why the driver implemented the above
> functions. I agree that indirect access is slower and requires more
> control commands than direct access. So implementing the direct access
> of C45 registers will overcome this issue.
It is not just about performance. It is about compliance to the
standard. The standard does not say anything about C45 over C22. So
there is no reason to expect a PHY device to implement it. It might,
but its optional.
> > The standard does say:
> >
> > Vendor specific registers may be mapped into MMS 10 though MMS
> > 15. When directly mapped, PHY vendor specific registers in MMD 30 or
> > MMD 31 would be mapped into the vendor specific MMS 10 through MMS 15.
> >
> > So i'm thinking you might need to provide C45 access, at least MMD 30,
> > via MMS 10-15?
> Thanks for this detailed comment. If understand you correctly by
> consolidating all your above explanations, the driver should provide C45
> access to the PHY vendor specific and PLCA registers (MMD 31). As per
> the specification, Table 6 describes the Register Memory Map Selector
> (MMS) Assignment. In this, MMS 4 maps the PHY vendor specific and PLCA
> registers. They are in the MMD 31 address space as per spec. They can be
> directly accessed using read_c45 and write_c45 functions in the mdio bus.
Yes. I think this is required to conform to the standard.
> In Microchip's MAC-PHY (LAN8650), PHY – Vendor Specific and PLCA
> Registers (MMD 31) mapped in the MMS 4 as per the table 6 in the spec.
> There is no other PHY vendor specific registers are mapped in the MMS 10
> through 15. No idea whether any other vendor's MAC-PHY uses MMS 10
> through 15 to map PHY – Vendor Specific and PLCA Registers (MMD 31).
>
> I have given the code below for the C45 access methods. Kindly check is
> this something you expected?
The code got mangled by your mail client :-(
> --- Code starts ---
>
> /* PHY – Vendor Specific and PLCA Registers (MMD 31) */
>
> #define OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE 0x40000
> ,,,
>
> static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
> devnum, int regnum)
> {
>
> struct oa_tc6 *tc6 = bus->priv;
>
> u32 regval;
>
> bool ret;
>
>
>
> ret = oa_tc6_read_register(tc6,
> OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE | regnum, ®val);
You appear to ignore devnum. I don't think you can do that. The core
phylib code might try to access other MMDs, e.g. it might try to see
if EEE is supported, by reading MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE.
Andrew
> @@ -55,6 +77,14 @@
> (OA_TC6_CTRL_MAX_REGISTERS *\
> OA_TC6_CTRL_REG_VALUE_SIZE) +\
> OA_TC6_CTRL_IGNORED_SIZE)
> +#define OA_TC6_CHUNK_PAYLOAD_SIZE 64
> +#define OA_TC6_DATA_HEADER_SIZE 4
> +#define OA_TC6_CHUNK_SIZE (OA_TC6_DATA_HEADER_SIZE +\
> + OA_TC6_CHUNK_PAYLOAD_SIZE)
> +#define OA_TC6_TX_SKB_QUEUE_SIZE 100
So you keep up to 100 packets in a queue. If use assume typical MTU
size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
traffic. That is quite a lot of latency when a high priority packet is
added to the tail of the queue and needs to wait for all the other
packets to be sent first.
Chunks are 64 bytes. So in practice, you only ever need two
packets. You need to be able to fill a chunk with the final part of
one packet, and the beginning of the next. So i would try using a much
smaller queue size. That will allow Linux queue disciplines to give
you the high priority packets first which you send with low latency.
> +static void oa_tc6_add_tx_skb_to_spi_buf(struct oa_tc6 *tc6)
> +{
> + enum oa_tc6_data_start_valid_info start_valid = OA_TC6_DATA_START_INVALID;
> + enum oa_tc6_data_end_valid_info end_valid = OA_TC6_DATA_END_INVALID;
> + __be32 *tx_buf = tc6->spi_data_tx_buf + tc6->spi_data_tx_buf_offset;
> + u16 remaining_length = tc6->tx_skb->len - tc6->tx_skb_offset;
> + u8 *tx_skb_data = tc6->tx_skb->data + tc6->tx_skb_offset;
> + u8 end_byte_offset = 0;
> + u16 length_to_copy;
> +
> + /* Set start valid if the current tx chunk contains the start of the tx
> + * ethernet frame.
> + */
> + if (!tc6->tx_skb_offset)
> + start_valid = OA_TC6_DATA_START_VALID;
> +
> + /* If the remaining tx skb length is more than the chunk payload size of
> + * 64 bytes then copy only 64 bytes and leave the ongoing tx skb for
> + * next tx chunk.
> + */
> + length_to_copy = min_t(u16, remaining_length, OA_TC6_CHUNK_PAYLOAD_SIZE);
> +
> + /* Copy the tx skb data to the tx chunk payload buffer */
> + memcpy(tx_buf + 1, tx_skb_data, length_to_copy);
> + tc6->tx_skb_offset += length_to_copy;
You probably need a call to skb_linearize() somewhere. You assume the
packet data is contiguous. It can in fact be split into multiple
segments. skb_linearize() will convert it to a single buffer.
> +static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
> +{
> + int ret;
> +
> + while (true) {
> + u16 spi_length = 0;
> +
> + tc6->spi_data_tx_buf_offset = 0;
> +
> + if (tc6->tx_skb || !skb_queue_empty(&tc6->tx_skb_q))
> + spi_length = oa_tc6_prepare_spi_tx_buf_for_tx_skbs(tc6);
> +
> + if (spi_length == 0)
> + break;
> +
> + ret = oa_tc6_spi_transfer(tc6, OA_TC6_DATA_HEADER, spi_length);
> + if (ret) {
> + netdev_err(tc6->netdev,
> + "SPI data transfer failed. Restart the system: %d\n",
> + ret);
What does Restart the system mean?
> +static int oa_tc6_spi_thread_handler(void *data)
> +{
> + struct oa_tc6 *tc6 = data;
> + int ret;
> +
> + while (likely(!kthread_should_stop())) {
> + /* This kthread will be waken up if there is a tx skb */
> + wait_event_interruptible(tc6->spi_wq,
> + !skb_queue_empty(&tc6->tx_skb_q) ||
> + kthread_should_stop());
> + ret = oa_tc6_try_spi_transfer(tc6);
Shouldn't you check why you have been woken up? It seems more logical
to test here for kthread_should_stop() rather than have
oa_tc6_try_spi_transfer() handle there is not actually a packet to be
sent.
Andrew
> +static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
> +{
> + tc6->rx_skb = netdev_alloc_skb(tc6->netdev, tc6->netdev->mtu + ETH_HLEN +
> + ETH_FCS_LEN + NET_IP_ALIGN);
> + if (!tc6->rx_skb) {
> + tc6->netdev->stats.rx_dropped++;
> + netdev_err(tc6->netdev, "Out of memory for rx'd frame");
If that happens, it is not something which will fix itself quickly. So
you are likely to spam the logs. The counter on its own is probably
enough.
Andrew
Hi Andrew,
On 07/03/24 6:54 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> +static int oa_tc6_read_sw_reset_status(struct oa_tc6 *tc6)
>>>> +{
>>>> + u32 regval;
>>>> + int ret;
>>>> +
>>>> + ret = oa_tc6_read_register(tc6, OA_TC6_REG_STATUS0, ®val);
>>>> + if (ret)
>>>> + return 0;
>>>> +
>>>> + return regval;
>>>
>>> The function name does not really fit what the function does. The
>>> function returns OA_TC6_REG_STATUS0. I assume it has more bits in it
>>> than just STATUS0_RESETC. So either this function should be called
>>> oa_tc6_read_status0, or you should mask regval with STATUS0_RESETC, so
>>> that it does actually return the sw reset status.
>> Ok, as we do the masking in the calling function readx_poll_timeout, as
>> you suggested I will change the function name as oa_tc6_read_status0 in
>> the next version.
>
> O.K. And i think some of the later patches can them make use of this
> generic function.
Yes, might be helpful.
Best regards,
Parthiban V
>
> Andrew
Hi Andrew,
On 07/03/24 10:06 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + tc6->mdiobus = mdiobus_alloc();
>>>> + if (!tc6->mdiobus) {
>>>> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + tc6->mdiobus->priv = tc6;
>>>> + tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
>>>> + tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
>>>
>>> This might get answered in later patches. PLCA registers are in C45
>>> address space, VEND1 if i remember correctly. You don't provide any
>>> C45 access methods here. Does TC6 specify that C45 over C22 must be
>>> implemented?
>> No the spec doesn't say anything like this. But, as C22 registers are
>> mapped in the MMS 0, registers 0xD and 0xE can be used to access C45
>> registers indirectly. That's why the driver implemented the above
>> functions. I agree that indirect access is slower and requires more
>> control commands than direct access. So implementing the direct access
>> of C45 registers will overcome this issue.
>
> It is not just about performance. It is about compliance to the
> standard. The standard does not say anything about C45 over C22. So
> there is no reason to expect a PHY device to implement it. It might,
> but its optional.
Yes, understood.
>
>>> The standard does say:
>>>
>>> Vendor specific registers may be mapped into MMS 10 though MMS
>>> 15. When directly mapped, PHY vendor specific registers in MMD 30 or
>>> MMD 31 would be mapped into the vendor specific MMS 10 through MMS 15.
>>>
>>> So i'm thinking you might need to provide C45 access, at least MMD 30,
>>> via MMS 10-15?
>> Thanks for this detailed comment. If understand you correctly by
>> consolidating all your above explanations, the driver should provide C45
>> access to the PHY vendor specific and PLCA registers (MMD 31). As per
>> the specification, Table 6 describes the Register Memory Map Selector
>> (MMS) Assignment. In this, MMS 4 maps the PHY vendor specific and PLCA
>> registers. They are in the MMD 31 address space as per spec. They can be
>> directly accessed using read_c45 and write_c45 functions in the mdio bus.
>
> Yes. I think this is required to conform to the standard.
Ok then let's implement like below.
>
>> In Microchip's MAC-PHY (LAN8650), PHY – Vendor Specific and PLCA
>> Registers (MMD 31) mapped in the MMS 4 as per the table 6 in the spec.
>> There is no other PHY vendor specific registers are mapped in the MMS 10
>> through 15. No idea whether any other vendor's MAC-PHY uses MMS 10
>> through 15 to map PHY – Vendor Specific and PLCA Registers (MMD 31).
>>
>> I have given the code below for the C45 access methods. Kindly check is
>> this something you expected?
>
> The code got mangled by your mail client :-(
Oh sorry.
>
>> --- Code starts ---
>>
>> /* PHY – Vendor Specific and PLCA Registers (MMD 31) */
>>
>> #define OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE 0x40000
>> ,,,
>>
>> static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
>> devnum, int regnum)
>> {
>>
>> struct oa_tc6 *tc6 = bus->priv;
>>
>> u32 regval;
>>
>> bool ret;
>>
>>
>>
>> ret = oa_tc6_read_register(tc6,
>> OA_TC6_PHY_VS_PLCA_REG_ADDR_BASE | regnum, ®val);
>
> You appear to ignore devnum. I don't think you can do that. The core
> phylib code might try to access other MMDs, e.g. it might try to see
> if EEE is supported, by reading MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE.
Ok, as per the table 6 in the spec, PHY C45 registers are mapped in the
MMS like below,
PHY – PCS Registers (MMD 3) ---> MMS 2
PHY – PMA/PMD Registers (MMD 1) ---> MMS 3
PHY – Vendor Specific and PLCA Registers (MMD 31) ---> MMS 4
PHY – Auto-Negotiation Registers (MMD 7) ---> MMS 5
PHY – Power Unit (MMD 13) ---> MMS 6
MMD 13 for PHY - Power Unit is not defined in the mdio.h. So in the
below code I have defined it locally (MDIO_MMD_POWER_UNIT). May be
needed to do this in the mdio.h file when coming to this patch.
https://elixir.bootlin.com/linux/v6.8-rc7/source/include/uapi/linux/mdio.h
Hope you are expecting like below? I believe this time the code will not
get mangled. If happens then sorry for that.
--- Code starts here ---
/* PHY – Clause 45 registers memory map selector (MMS) as per table 6 in
the OPEN Alliance specification.
*/
#define OA_TC6_PHY_PCS_MMS2 2 /* MMD 3 */
#define OA_TC6_PHY_PMA_PMD_MMS3 3 /* MMD 1 */
#define OA_TC6_PHY_VS_PLCA_MMS4 4 /* MMD 31 */
#define OA_TC6_PHY_AUTO_NEG_MMS5 5 /* MMD 7 */
#define OA_TC6_PHY_POWER_UNIT_MMS6 6 /* MMD 13 */
/* MDIO Manageable Device (MMD) for PHY Power Unit */
#define MDIO_MMD_POWER_UNIT 13 /* PHY Power Unit */
static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
devnum, int regnum)
{
struct oa_tc6 *tc6 = bus->priv;
u32 regval;
bool ret;
u32 mms;
if (devnum == MDIO_MMD_PCS)
mms = OA_TC6_PHY_PCS_MMS2;
else if (devnum == MDIO_MMD_PMAPMD)
mms = OA_TC6_PHY_PMA_PMD_MMS3;
else if (devnum == MDIO_MMD_VEND2)
mms = OA_TC6_PHY_VS_PLCA_MMS4;
else if (devnum == MDIO_MMD_AN)
mms = OA_TC6_PHY_AUTO_NEG_MMS5;
else if (devnum == MDIO_MMD_POWER_UNIT)
mms = OA_TC6_PHY_POWER_UNIT_MMS6;
else
return -ENOTSUPP;
ret = oa_tc6_read_register(tc6, (mms << 16) | regnum, ®val);
if (ret)
return -ENODEV;
return regval;
}
static int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int
devnum, int regnum, u16 val)
{
struct oa_tc6 *tc6 = bus->priv;
u32 mms;
if (devnum == MDIO_MMD_PCS)
mms = OA_TC6_PHY_PCS_MMS2;
else if (devnum == MDIO_MMD_PMAPMD)
mms = OA_TC6_PHY_PMA_PMD_MMS3;
else if (devnum == MDIO_MMD_VEND2)
mms = OA_TC6_PHY_VS_PLCA_MMS4;
else if (devnum == MDIO_MMD_AN)
mms = OA_TC6_PHY_AUTO_NEG_MMS5;
else if (devnum == MDIO_MMD_POWER_UNIT)
mms = OA_TC6_PHY_POWER_UNIT_MMS6;
else
return -ENOTSUPP;
return oa_tc6_write_register(tc6, (mms << 16) | regnum, val);
}
--- Code ends here ---
Best regards,
Parthiban V
>
> Andrew
>
> Ok, as per the table 6 in the spec, PHY C45 registers are mapped in the
> MMS like below,
>
> PHY – PCS Registers (MMD 3) ---> MMS 2
> PHY – PMA/PMD Registers (MMD 1) ---> MMS 3
> PHY – Vendor Specific and PLCA Registers (MMD 31) ---> MMS 4
> PHY – Auto-Negotiation Registers (MMD 7) ---> MMS 5
> PHY – Power Unit (MMD 13) ---> MMS 6
>
> MMD 13 for PHY - Power Unit is not defined in the mdio.h. So in the
> below code I have defined it locally (MDIO_MMD_POWER_UNIT). May be
> needed to do this in the mdio.h file when coming to this patch.
Yes, please add it to mdio.h
> /* PHY – Clause 45 registers memory map selector (MMS) as per table 6 in
> the OPEN Alliance specification.
> */
> #define OA_TC6_PHY_PCS_MMS2 2 /* MMD 3 */
> #define OA_TC6_PHY_PMA_PMD_MMS3 3 /* MMD 1 */
> #define OA_TC6_PHY_VS_PLCA_MMS4 4 /* MMD 31 */
> #define OA_TC6_PHY_AUTO_NEG_MMS5 5 /* MMD 7 */
> #define OA_TC6_PHY_POWER_UNIT_MMS6 6 /* MMD 13 */
>
> /* MDIO Manageable Device (MMD) for PHY Power Unit */
> #define MDIO_MMD_POWER_UNIT 13 /* PHY Power Unit */
>
> static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
> devnum, int regnum)
> {
>
> struct oa_tc6 *tc6 = bus->priv;
>
> u32 regval;
>
> bool ret;
>
> u32 mms;
>
>
>
> if (devnum == MDIO_MMD_PCS)
>
> mms = OA_TC6_PHY_PCS_MMS2;
>
> else if (devnum == MDIO_MMD_PMAPMD)
>
> mms = OA_TC6_PHY_PMA_PMD_MMS3;
>
> else if (devnum == MDIO_MMD_VEND2)
>
> mms = OA_TC6_PHY_VS_PLCA_MMS4;
>
> else if (devnum == MDIO_MMD_AN)
>
> mms = OA_TC6_PHY_AUTO_NEG_MMS5;
>
> else if (devnum == MDIO_MMD_POWER_UNIT)
>
> mms = OA_TC6_PHY_POWER_UNIT_MMS6;
I would probably use a switch statement.
>
> else
>
> return -ENOTSUPP;
802.3 says:
If a device supports the MDIO interface it shall respond to all
possible register addresses for the device and return a value of
zero for undefined and unsupported registers. Writes to undefined
registers and read-only registers shall have no effect. The
operation of an MMD shall not be affected by writes to reserved and
unsupported register bits, and such register bits shall return a
value of zero when read.
So maybe return 0. ENOTSUPP is wrong, that is an NFS only error
code. The generic one is EOPNOTSUPP. I would say -EOPNOTSUPP is also
O.K.
> ret = oa_tc6_read_register(tc6, (mms << 16) | regnum, ®val);
>
> if (ret)
>
> return -ENODEV;
oa_tc6_read_register() should return an error code, so return whatever
is returns. Don't overwrite error codes. It makes it harder to track
errors through the call stack.
Andrew
Hi Andrew,
On 08/03/24 7:03 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> Ok, as per the table 6 in the spec, PHY C45 registers are mapped in the
>> MMS like below,
>>
>> PHY – PCS Registers (MMD 3) ---> MMS 2
>> PHY – PMA/PMD Registers (MMD 1) ---> MMS 3
>> PHY – Vendor Specific and PLCA Registers (MMD 31) ---> MMS 4
>> PHY – Auto-Negotiation Registers (MMD 7) ---> MMS 5
>> PHY – Power Unit (MMD 13) ---> MMS 6
>>
>> MMD 13 for PHY - Power Unit is not defined in the mdio.h. So in the
>> below code I have defined it locally (MDIO_MMD_POWER_UNIT). May be
>> needed to do this in the mdio.h file when coming to this patch.
>
> Yes, please add it to mdio.h
Sure will add it in the mdio.h file.
>
>> /* PHY – Clause 45 registers memory map selector (MMS) as per table 6 in
>> the OPEN Alliance specification.
>> */
>> #define OA_TC6_PHY_PCS_MMS2 2 /* MMD 3 */
>> #define OA_TC6_PHY_PMA_PMD_MMS3 3 /* MMD 1 */
>> #define OA_TC6_PHY_VS_PLCA_MMS4 4 /* MMD 31 */
>> #define OA_TC6_PHY_AUTO_NEG_MMS5 5 /* MMD 7 */
>> #define OA_TC6_PHY_POWER_UNIT_MMS6 6 /* MMD 13 */
>>
>> /* MDIO Manageable Device (MMD) for PHY Power Unit */
>> #define MDIO_MMD_POWER_UNIT 13 /* PHY Power Unit */
>>
>> static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
>> devnum, int regnum)
>> {
>>
>> struct oa_tc6 *tc6 = bus->priv;
>>
>> u32 regval;
>>
>> bool ret;
>>
>> u32 mms;
>>
>>
>>
>> if (devnum == MDIO_MMD_PCS)
>>
>> mms = OA_TC6_PHY_PCS_MMS2;
>>
>> else if (devnum == MDIO_MMD_PMAPMD)
>>
>> mms = OA_TC6_PHY_PMA_PMD_MMS3;
>>
>> else if (devnum == MDIO_MMD_VEND2)
>>
>> mms = OA_TC6_PHY_VS_PLCA_MMS4;
>>
>> else if (devnum == MDIO_MMD_AN)
>>
>> mms = OA_TC6_PHY_AUTO_NEG_MMS5;
>>
>> else if (devnum == MDIO_MMD_POWER_UNIT)
>>
>> mms = OA_TC6_PHY_POWER_UNIT_MMS6;
>
> I would probably use a switch statement.
Ah ok, I will use switch here.
>
>>
>> else
>>
>> return -ENOTSUPP;
>
> 802.3 says:
>
> If a device supports the MDIO interface it shall respond to all
> possible register addresses for the device and return a value of
> zero for undefined and unsupported registers. Writes to undefined
> registers and read-only registers shall have no effect. The
> operation of an MMD shall not be affected by writes to reserved and
> unsupported register bits, and such register bits shall return a
> value of zero when read.
>
> So maybe return 0. ENOTSUPP is wrong, that is an NFS only error
> code. The generic one is EOPNOTSUPP. I would say -EOPNOTSUPP is also
> O.K.
Sure, I will use -EOPNOTSUPP in the next version instead of -ENOTSUPP.
>
>> ret = oa_tc6_read_register(tc6, (mms << 16) | regnum, ®val);
>>
>> if (ret)
>>
>> return -ENODEV;
>
> oa_tc6_read_register() should return an error code, so return whatever
> is returns. Don't overwrite error codes. It makes it harder to track
> errors through the call stack.
Ah ok, will return the error code from oa_tc6_read_register() in the
next version.
Best regards,
Parthiban V
>
> Andrew
>
Hi Andrew,
On 07/03/24 10:38 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> @@ -55,6 +77,14 @@
>> (OA_TC6_CTRL_MAX_REGISTERS *\
>> OA_TC6_CTRL_REG_VALUE_SIZE) +\
>> OA_TC6_CTRL_IGNORED_SIZE)
>> +#define OA_TC6_CHUNK_PAYLOAD_SIZE 64
>> +#define OA_TC6_DATA_HEADER_SIZE 4
>> +#define OA_TC6_CHUNK_SIZE (OA_TC6_DATA_HEADER_SIZE +\
>> + OA_TC6_CHUNK_PAYLOAD_SIZE)
>> +#define OA_TC6_TX_SKB_QUEUE_SIZE 100
>
> So you keep up to 100 packets in a queue. If use assume typical MTU
> size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
> traffic. That is quite a lot of latency when a high priority packet is
> added to the tail of the queue and needs to wait for all the other
> packets to be sent first.
>
> Chunks are 64 bytes. So in practice, you only ever need two
> packets. You need to be able to fill a chunk with the final part of
> one packet, and the beginning of the next. So i would try using a much
> smaller queue size. That will allow Linux queue disciplines to give
> you the high priority packets first which you send with low latency.
Thanks for the detailed explanation. If I understand you correctly,
1. The tx skb queue size (OA_TC6_TX_SKB_QUEUE_SIZE) should be 2 to avoid
the latency when a high priority packet added.
2. Need to implement the handling part of the below case,
In case if one packet ends in a chunk and that chunk still having some
space left to accommodate some bytes from the next packet if available
from network layer.
Will implement in the next version.
>
>> +static void oa_tc6_add_tx_skb_to_spi_buf(struct oa_tc6 *tc6)
>> +{
>> + enum oa_tc6_data_start_valid_info start_valid = OA_TC6_DATA_START_INVALID;
>> + enum oa_tc6_data_end_valid_info end_valid = OA_TC6_DATA_END_INVALID;
>> + __be32 *tx_buf = tc6->spi_data_tx_buf + tc6->spi_data_tx_buf_offset;
>> + u16 remaining_length = tc6->tx_skb->len - tc6->tx_skb_offset;
>> + u8 *tx_skb_data = tc6->tx_skb->data + tc6->tx_skb_offset;
>> + u8 end_byte_offset = 0;
>> + u16 length_to_copy;
>> +
>> + /* Set start valid if the current tx chunk contains the start of the tx
>> + * ethernet frame.
>> + */
>> + if (!tc6->tx_skb_offset)
>> + start_valid = OA_TC6_DATA_START_VALID;
>> +
>> + /* If the remaining tx skb length is more than the chunk payload size of
>> + * 64 bytes then copy only 64 bytes and leave the ongoing tx skb for
>> + * next tx chunk.
>> + */
>> + length_to_copy = min_t(u16, remaining_length, OA_TC6_CHUNK_PAYLOAD_SIZE);
>> +
>> + /* Copy the tx skb data to the tx chunk payload buffer */
>> + memcpy(tx_buf + 1, tx_skb_data, length_to_copy);
>> + tc6->tx_skb_offset += length_to_copy;
>
> You probably need a call to skb_linearize() somewhere. You assume the
> packet data is contiguous. It can in fact be split into multiple
> segments. skb_linearize() will convert it to a single buffer.
Ah ok. Then probably I have to add the below code in the
oa_tc6_start_xmit() function before adding the skb into the transmit queue.
if (skb_linearize(skb)) {
dev_kfree_skb_any(skb);
tc6->netdev->stats.tx_dropped++;
return NETDEV_TX_OK;
}
>
>> +static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
>> +{
>> + int ret;
>> +
>> + while (true) {
>> + u16 spi_length = 0;
>> +
>> + tc6->spi_data_tx_buf_offset = 0;
>> +
>> + if (tc6->tx_skb || !skb_queue_empty(&tc6->tx_skb_q))
>> + spi_length = oa_tc6_prepare_spi_tx_buf_for_tx_skbs(tc6);
>> +
>> + if (spi_length == 0)
>> + break;
>> +
>> + ret = oa_tc6_spi_transfer(tc6, OA_TC6_DATA_HEADER, spi_length);
>> + if (ret) {
>> + netdev_err(tc6->netdev,
>> + "SPI data transfer failed. Restart the system: %d\n",
>> + ret);
>
> What does Restart the system mean?
Hmm, actually if SPI transfer failed then it can be hardware failure or
poor SPI connection. Now I realize that just restarting the system will
not help. I will remove "Restart the system:" as it is not the correct info.
>
>> +static int oa_tc6_spi_thread_handler(void *data)
>> +{
>> + struct oa_tc6 *tc6 = data;
>> + int ret;
>> +
>> + while (likely(!kthread_should_stop())) {
>> + /* This kthread will be waken up if there is a tx skb */
>> + wait_event_interruptible(tc6->spi_wq,
>> + !skb_queue_empty(&tc6->tx_skb_q) ||
>> + kthread_should_stop());
>> + ret = oa_tc6_try_spi_transfer(tc6);
>
> Shouldn't you check why you have been woken up? It seems more logical
> to test here for kthread_should_stop() rather than have
> oa_tc6_try_spi_transfer() handle there is not actually a packet to be
> sent.
Ok, then I will add the below code before the oa_tc6_try_spi_transfer().
if (kthread_should_stop())
break;
Best regards,
Parthiban V
>
> Andrew
>
Hi Andrew,
On 08/03/24 5:44 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
>> +{
>> + tc6->rx_skb = netdev_alloc_skb(tc6->netdev, tc6->netdev->mtu + ETH_HLEN +
>> + ETH_FCS_LEN + NET_IP_ALIGN);
>> + if (!tc6->rx_skb) {
>> + tc6->netdev->stats.rx_dropped++;
>> + netdev_err(tc6->netdev, "Out of memory for rx'd frame");
>
> If that happens, it is not something which will fix itself quickly. So
> you are likely to spam the logs. The counter on its own is probably
> enough.
Ok, then don't we need to convey this info in the dmesg to the user. For
that shall we use net_err_ratelimited() instead of netdev_err()? Or we
don't need any print at all?
Best regards,
Parthiban V
>
> Andrew
>
On Tue, Mar 19, 2024 at 12:54:30PM +0000, [email protected] wrote:
> Hi Andrew,
>
> On 07/03/24 10:38 pm, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> >> @@ -55,6 +77,14 @@
> >> (OA_TC6_CTRL_MAX_REGISTERS *\
> >> OA_TC6_CTRL_REG_VALUE_SIZE) +\
> >> OA_TC6_CTRL_IGNORED_SIZE)
> >> +#define OA_TC6_CHUNK_PAYLOAD_SIZE 64
> >> +#define OA_TC6_DATA_HEADER_SIZE 4
> >> +#define OA_TC6_CHUNK_SIZE (OA_TC6_DATA_HEADER_SIZE +\
> >> + OA_TC6_CHUNK_PAYLOAD_SIZE)
> >> +#define OA_TC6_TX_SKB_QUEUE_SIZE 100
> >
> > So you keep up to 100 packets in a queue. If use assume typical MTU
> > size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
> > traffic. That is quite a lot of latency when a high priority packet is
> > added to the tail of the queue and needs to wait for all the other
> > packets to be sent first.
> >
> > Chunks are 64 bytes. So in practice, you only ever need two
> > packets. You need to be able to fill a chunk with the final part of
> > one packet, and the beginning of the next. So i would try using a much
> > smaller queue size. That will allow Linux queue disciplines to give
> > you the high priority packets first which you send with low latency.
> Thanks for the detailed explanation. If I understand you correctly,
>
> 1. The tx skb queue size (OA_TC6_TX_SKB_QUEUE_SIZE) should be 2 to avoid
> the latency when a high priority packet added.
>
> 2. Need to implement the handling part of the below case,
> In case if one packet ends in a chunk and that chunk still having some
> space left to accommodate some bytes from the next packet if available
> from network layer.
This second part is clearly an optimisation. If you have lots of full
MTU packets, 1514 bytes, they take around 24 chunks. Having the last
chunk only 1/2 full does not waste too much bandwidth. But if you are
carrying lots of small packets, say voice, 130 bytes, the wasted
bandwidth starts to add up. But is there a use case for 10Mbps of
small packets? I doubt it.
So if you don't have the ability to combine two packets into one
chunk, i would do that later. Lets get the basics merged first, it can
be optimised later.
Andrew
On Tue, Mar 19, 2024 at 12:54:34PM +0000, [email protected] wrote:
> Hi Andrew,
>
> On 08/03/24 5:44 am, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> >> +static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
> >> +{
> >> + tc6->rx_skb = netdev_alloc_skb(tc6->netdev, tc6->netdev->mtu + ETH_HLEN +
> >> + ETH_FCS_LEN + NET_IP_ALIGN);
> >> + if (!tc6->rx_skb) {
> >> + tc6->netdev->stats.rx_dropped++;
> >> + netdev_err(tc6->netdev, "Out of memory for rx'd frame");
> >
> > If that happens, it is not something which will fix itself quickly. So
> > you are likely to spam the logs. The counter on its own is probably
> > enough.
> Ok, then don't we need to convey this info in the dmesg to the user. For
> that shall we use net_err_ratelimited() instead of netdev_err()? Or we
> don't need any print at all?
I would not print anything at all.
Andrew
Hi Andrew,
On 19/03/24 6:50 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, Mar 19, 2024 at 12:54:34PM +0000, [email protected] wrote:
>> Hi Andrew,
>>
>> On 08/03/24 5:44 am, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>>> +static int oa_tc6_allocate_rx_skb(struct oa_tc6 *tc6)
>>>> +{
>>>> + tc6->rx_skb = netdev_alloc_skb(tc6->netdev, tc6->netdev->mtu + ETH_HLEN +
>>>> + ETH_FCS_LEN + NET_IP_ALIGN);
>>>> + if (!tc6->rx_skb) {
>>>> + tc6->netdev->stats.rx_dropped++;
>>>> + netdev_err(tc6->netdev, "Out of memory for rx'd frame");
>>>
>>> If that happens, it is not something which will fix itself quickly. So
>>> you are likely to spam the logs. The counter on its own is probably
>>> enough.
>> Ok, then don't we need to convey this info in the dmesg to the user. For
>> that shall we use net_err_ratelimited() instead of netdev_err()? Or we
>> don't need any print at all?
>
> I would not print anything at all.
OK, I will remove the print in the next version.
Best regards,
Parthiban V
>
> Andrew
>
Hi Conor,
On 06/03/24 11:46 pm, Conor Dooley wrote:
> On Wed, Mar 06, 2024 at 02:20:17PM +0530, Parthiban Veerasooran wrote:
>> 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 | 80 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 81 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..ee52f9d8e93c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml
>> @@ -0,0 +1,80 @@
>> +# 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).
>> +
>> +allOf:
>> + - $ref: ethernet-controller.yaml#
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - items:
>> + - const: microchip,lan8650
>> + - const: microchip,lan8651
> The order here is wrong, lan8561 needs to come before the fallback of
> lan8650.
Reply to this comment is in another email.
>
>> + - enum:
>> + - microchip,lan8650
>> +
>> + 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
>> +
>> + spi-max-frequency:
>> + minimum: 15000000
>> + maximum: 25000000
> You're missing a reference to spi-peripheral-props where this property
> is defined.
OK, I will add the below line in the above "allOf" section.
- $ref: /schemas/spi/spi-peripheral-props.yaml#
Best regards,
Parthiban V
>
> Thanks,
> Conor.
Hi Conor & Andrew,
Please find my reply below by consolidating other two emails comments
related to this.
On 07/03/24 12:31 am, Conor Dooley wrote:
> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>> +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).
>>>> +
>>>> +allOf:
>>>> + - $ref: ethernet-controller.yaml#
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + oneOf:
>>>> + - items:
>>>> + - const: microchip,lan8650
>>>> + - const: microchip,lan8651
>>> The order here is wrong, lan8561 needs to come before the fallback of
>>> lan8650.
>> I don't think it is a fallback. There are two devices, and hence two
>> different compatibles. So i suspect the -items: is wrong here?
> It'd just be a two entry enum then, but I did take a quick look at the
> driver earlier and saw:
> +static const struct of_device_id lan865x_dt_ids[] = {
> + { .compatible = "microchip,lan8650" },
> + { .compatible = "microchip,lan8651" },
> + { /* Sentinel */ }
> +};
>
> That, along with no other of_device_is_compatible() type operations
> made me think that having a fallback actually was suitable.
>
> You cropped it out, but the patch had:
>> + compatible:
>> + oneOf:
>> + - items:
>> + - const: microchip,lan8650
>> + - const: microchip,lan8651
>> + - enum:
>> + - microchip,lan8650
> So it doesn't appear to be an accidental items in place of an enum,
> since the other compatible is in another enum.
As per Andrew's comment in another email, both LAN8650 and LAN8651 are
two different variants but they both share almost all characteristics
except one thing that is LAN8651 has "Single 3.3V supply with integrated
1.8V regulator" which doesn't have anything to do with driver. That's
why I have kept them as fallback as Conor said in this email. Hope you
all OK with this.
>
> I just noticed also that that enum should actually be const, so I'd
> expect this to be:
> compatible:
> oneOf:
> - items:
> - const: microchip,lan8651
> - const: microchip,lan8650
>
> - const: microchip,lan8650
OK, I will update this in the next version.
Best regards,
Parthiban V
On 20/03/2024 09:40, [email protected] wrote:
> Hi Conor & Andrew,
>
> Please find my reply below by consolidating other two emails comments
> related to this.
>
> On 07/03/24 12:31 am, Conor Dooley wrote:
>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>>> +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).
>>>>> +
>>>>> +allOf:
>>>>> + - $ref: ethernet-controller.yaml#
>>>>> +
>>>>> +properties:
>>>>> + compatible:
>>>>> + oneOf:
>>>>> + - items:
>>>>> + - const: microchip,lan8650
>>>>> + - const: microchip,lan8651
>>>> The order here is wrong, lan8561 needs to come before the fallback of
>>>> lan8650.
>>> I don't think it is a fallback. There are two devices, and hence two
>>> different compatibles. So i suspect the -items: is wrong here?
>> It'd just be a two entry enum then, but I did take a quick look at the
>> driver earlier and saw:
>> +static const struct of_device_id lan865x_dt_ids[] = {
>> + { .compatible = "microchip,lan8650" },
>> + { .compatible = "microchip,lan8651" },
>> + { /* Sentinel */ }
>> +};
>>
>> That, along with no other of_device_is_compatible() type operations
>> made me think that having a fallback actually was suitable.
>>
>> You cropped it out, but the patch had:
>>> + compatible:
>>> + oneOf:
>>> + - items:
>>> + - const: microchip,lan8650
>>> + - const: microchip,lan8651
>>> + - enum:
>>> + - microchip,lan8650
>> So it doesn't appear to be an accidental items in place of an enum,
>> since the other compatible is in another enum.
> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
> two different variants but they both share almost all characteristics
> except one thing that is LAN8651 has "Single 3.3V supply with integrated
> 1.8V regulator" which doesn't have anything to do with driver. That's
So why this is not reflected in your driver? Why didn't you address that
part, but ignored?
> why I have kept them as fallback as Conor said in this email. Hope you
> all OK with this.
Did you read the feedback? Your response is not solving here anything.
How 8650 can be used twice? Please point me to DTS showing both usages.
Best regards,
Krzysztof
Hi Andrew,
On 19/03/24 6:49 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, Mar 19, 2024 at 12:54:30PM +0000, [email protected] wrote:
>> Hi Andrew,
>>
>> On 07/03/24 10:38 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>>> @@ -55,6 +77,14 @@
>>>> (OA_TC6_CTRL_MAX_REGISTERS *\
>>>> OA_TC6_CTRL_REG_VALUE_SIZE) +\
>>>> OA_TC6_CTRL_IGNORED_SIZE)
>>>> +#define OA_TC6_CHUNK_PAYLOAD_SIZE 64
>>>> +#define OA_TC6_DATA_HEADER_SIZE 4
>>>> +#define OA_TC6_CHUNK_SIZE (OA_TC6_DATA_HEADER_SIZE +\
>>>> + OA_TC6_CHUNK_PAYLOAD_SIZE)
>>>> +#define OA_TC6_TX_SKB_QUEUE_SIZE 100
>>>
>>> So you keep up to 100 packets in a queue. If use assume typical MTU
>>> size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
>>> traffic. That is quite a lot of latency when a high priority packet is
>>> added to the tail of the queue and needs to wait for all the other
>>> packets to be sent first.
>>>
>>> Chunks are 64 bytes. So in practice, you only ever need two
>>> packets. You need to be able to fill a chunk with the final part of
>>> one packet, and the beginning of the next. So i would try using a much
>>> smaller queue size. That will allow Linux queue disciplines to give
>>> you the high priority packets first which you send with low latency.
>> Thanks for the detailed explanation. If I understand you correctly,
>>
>> 1. The tx skb queue size (OA_TC6_TX_SKB_QUEUE_SIZE) should be 2 to avoid
>> the latency when a high priority packet added.
>>
>> 2. Need to implement the handling part of the below case,
>> In case if one packet ends in a chunk and that chunk still having some
>> space left to accommodate some bytes from the next packet if available
>> from network layer.
>
> This second part is clearly an optimisation. If you have lots of full
> MTU packets, 1514 bytes, they take around 24 chunks. Having the last
> chunk only 1/2 full does not waste too much bandwidth. But if you are
> carrying lots of small packets, say voice, 130 bytes, the wasted
> bandwidth starts to add up. But is there a use case for 10Mbps of
> small packets? I doubt it.
Yes, for sure there is a possibility to get into this scenario and the
protocol also supports that. But as proposed by you below, let's
implement it as part of optimization later.
>
> So if you don't have the ability to combine two packets into one
> chunk, i would do that later. Lets get the basics merged first, it can
> be optimised later.
Yes, I agree with this proposal to get the basic version merged first.
Best regards,
Parthiban V
>
> Andrew
Hi Krzysztof,
On 20/03/24 3:23 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 20/03/2024 09:40, [email protected] wrote:
>> Hi Conor & Andrew,
>>
>> Please find my reply below by consolidating other two emails comments
>> related to this.
>>
>> On 07/03/24 12:31 am, Conor Dooley wrote:
>>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>>>> +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).
>>>>>> +
>>>>>> +allOf:
>>>>>> + - $ref: ethernet-controller.yaml#
>>>>>> +
>>>>>> +properties:
>>>>>> + compatible:
>>>>>> + oneOf:
>>>>>> + - items:
>>>>>> + - const: microchip,lan8650
>>>>>> + - const: microchip,lan8651
>>>>> The order here is wrong, lan8561 needs to come before the fallback of
>>>>> lan8650.
>>>> I don't think it is a fallback. There are two devices, and hence two
>>>> different compatibles. So i suspect the -items: is wrong here?
>>> It'd just be a two entry enum then, but I did take a quick look at the
>>> driver earlier and saw:
>>> +static const struct of_device_id lan865x_dt_ids[] = {
>>> + { .compatible = "microchip,lan8650" },
>>> + { .compatible = "microchip,lan8651" },
>>> + { /* Sentinel */ }
>>> +};
>>>
>>> That, along with no other of_device_is_compatible() type operations
>>> made me think that having a fallback actually was suitable.
>>>
>>> You cropped it out, but the patch had:
>>>> + compatible:
>>>> + oneOf:
>>>> + - items:
>>>> + - const: microchip,lan8650
>>>> + - const: microchip,lan8651
>>>> + - enum:
>>>> + - microchip,lan8650
>>> So it doesn't appear to be an accidental items in place of an enum,
>>> since the other compatible is in another enum.
>> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
>> two different variants but they both share almost all characteristics
>> except one thing that is LAN8651 has "Single 3.3V supply with integrated
>> 1.8V regulator" which doesn't have anything to do with driver. That's
>
> So why this is not reflected in your driver? Why didn't you address that
> part, but ignored?
No, it is not ignored. This difference is specific to hardware and there
is no configuration/setting to be done from driver.
>
>> why I have kept them as fallback as Conor said in this email. Hope you
>> all OK with this.
>
> Did you read the feedback? Your response is not solving here anything.
> How 8650 can be used twice? Please point me to DTS showing both usages.
May be I have a misunderstanding here. Let's clarify it.
LAN8650 and LAN8651 both are two different variants but both implements
same functionality. The only difference is LAN8651 has "Single 3.3V
supply with integrated" where LAN8650 doesn't have this. This is
hardware specific difference and there is no configuration/setting to be
done in the driver specific to this difference in the LAN8651. So
basically the driver can support for both variants without any
additional settings.
LAN8650: https://www.microchip.com/en-us/product/lan8650
LAN8651: https://www.microchip.com/en-us/product/lan8651
The below link shows the difference between them,
https://www.microchip.com/en-us/product-comparison.lan8650.lan8651
With the above details, I would change the microchip,lan865x.yaml with
the below details.
compatible:
enum:
- microchip,lan8650
- microchip,lan8651
And in the lan865x.c, I would remove the below line because
.compatible = "microchip,lan8650" already supports for LAN8651 as well.
.compatible = "microchip,lan8651"
Let me know your opinion on this proposal? or do you have any
misunderstanding here?
Best regards,
Parthiban V
>
> Best regards,
> Krzysztof
>
>
On 21/03/2024 09:38, [email protected] wrote:
> Hi Krzysztof,
>
> On 20/03/24 3:23 pm, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 20/03/2024 09:40, [email protected] wrote:
>>> Hi Conor & Andrew,
>>>
>>> Please find my reply below by consolidating other two emails comments
>>> related to this.
>>>
>>> On 07/03/24 12:31 am, Conor Dooley wrote:
>>>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>>>>> +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).
>>>>>>> +
>>>>>>> +allOf:
>>>>>>> + - $ref: ethernet-controller.yaml#
>>>>>>> +
>>>>>>> +properties:
>>>>>>> + compatible:
>>>>>>> + oneOf:
>>>>>>> + - items:
>>>>>>> + - const: microchip,lan8650
>>>>>>> + - const: microchip,lan8651
>>>>>> The order here is wrong, lan8561 needs to come before the fallback of
>>>>>> lan8650.
>>>>> I don't think it is a fallback. There are two devices, and hence two
>>>>> different compatibles. So i suspect the -items: is wrong here?
>>>> It'd just be a two entry enum then, but I did take a quick look at the
>>>> driver earlier and saw:
>>>> +static const struct of_device_id lan865x_dt_ids[] = {
>>>> + { .compatible = "microchip,lan8650" },
>>>> + { .compatible = "microchip,lan8651" },
>>>> + { /* Sentinel */ }
>>>> +};
>>>>
>>>> That, along with no other of_device_is_compatible() type operations
>>>> made me think that having a fallback actually was suitable.
>>>>
>>>> You cropped it out, but the patch had:
>>>>> + compatible:
>>>>> + oneOf:
>>>>> + - items:
>>>>> + - const: microchip,lan8650
>>>>> + - const: microchip,lan8651
>>>>> + - enum:
>>>>> + - microchip,lan8650
>>>> So it doesn't appear to be an accidental items in place of an enum,
>>>> since the other compatible is in another enum.
>>> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
>>> two different variants but they both share almost all characteristics
>>> except one thing that is LAN8651 has "Single 3.3V supply with integrated
>>> 1.8V regulator" which doesn't have anything to do with driver. That's
>>
>> So why this is not reflected in your driver? Why didn't you address that
>> part, but ignored?
> No, it is not ignored. This difference is specific to hardware and there
> is no configuration/setting to be done from driver.
>>
>>> why I have kept them as fallback as Conor said in this email. Hope you
>>> all OK with this.
>>
>> Did you read the feedback? Your response is not solving here anything.
>> How 8650 can be used twice? Please point me to DTS showing both usages.
> May be I have a misunderstanding here. Let's clarify it.
>
> LAN8650 and LAN8651 both are two different variants but both implements
> same functionality. The only difference is LAN8651 has "Single 3.3V
> supply with integrated" where LAN8650 doesn't have this. This is
> hardware specific difference and there is no configuration/setting to be
> done in the driver specific to this difference in the LAN8651. So
> basically the driver can support for both variants without any
> additional settings.
>
> LAN8650: https://www.microchip.com/en-us/product/lan8650
> LAN8651: https://www.microchip.com/en-us/product/lan8651
>
> The below link shows the difference between them,
> https://www.microchip.com/en-us/product-comparison.lan8650.lan8651
>
> With the above details, I would change the microchip,lan865x.yaml with
> the below details.
>
> compatible:
> enum:
> - microchip,lan8650
> - microchip,lan8651
>
> And in the lan865x.c, I would remove the below line because
> .compatible = "microchip,lan8650" already supports for LAN8651 as well.
>
> .compatible = "microchip,lan8651"
>
> Let me know your opinion on this proposal? or do you have any
> misunderstanding here?
It's still wrong. Upstream your DTS and then test it. You will
immediately see that it does not work. So first make it working, then
send code to review.
Best regards,
Krzysztof
Hi Krzysztof,
On 21/03/24 2:10 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 21/03/2024 09:38, [email protected] wrote:
>> Hi Krzysztof,
>>
>> On 20/03/24 3:23 pm, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 20/03/2024 09:40, [email protected] wrote:
>>>> Hi Conor & Andrew,
>>>>
>>>> Please find my reply below by consolidating other two emails comments
>>>> related to this.
>>>>
>>>> On 07/03/24 12:31 am, Conor Dooley wrote:
>>>>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>>>>>> +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).
>>>>>>>> +
>>>>>>>> +allOf:
>>>>>>>> + - $ref: ethernet-controller.yaml#
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> + compatible:
>>>>>>>> + oneOf:
>>>>>>>> + - items:
>>>>>>>> + - const: microchip,lan8650
>>>>>>>> + - const: microchip,lan8651
>>>>>>> The order here is wrong, lan8561 needs to come before the fallback of
>>>>>>> lan8650.
>>>>>> I don't think it is a fallback. There are two devices, and hence two
>>>>>> different compatibles. So i suspect the -items: is wrong here?
>>>>> It'd just be a two entry enum then, but I did take a quick look at the
>>>>> driver earlier and saw:
>>>>> +static const struct of_device_id lan865x_dt_ids[] = {
>>>>> + { .compatible = "microchip,lan8650" },
>>>>> + { .compatible = "microchip,lan8651" },
>>>>> + { /* Sentinel */ }
>>>>> +};
>>>>>
>>>>> That, along with no other of_device_is_compatible() type operations
>>>>> made me think that having a fallback actually was suitable.
>>>>>
>>>>> You cropped it out, but the patch had:
>>>>>> + compatible:
>>>>>> + oneOf:
>>>>>> + - items:
>>>>>> + - const: microchip,lan8650
>>>>>> + - const: microchip,lan8651
>>>>>> + - enum:
>>>>>> + - microchip,lan8650
>>>>> So it doesn't appear to be an accidental items in place of an enum,
>>>>> since the other compatible is in another enum.
>>>> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
>>>> two different variants but they both share almost all characteristics
>>>> except one thing that is LAN8651 has "Single 3.3V supply with integrated
>>>> 1.8V regulator" which doesn't have anything to do with driver. That's
>>>
>>> So why this is not reflected in your driver? Why didn't you address that
>>> part, but ignored?
>> No, it is not ignored. This difference is specific to hardware and there
>> is no configuration/setting to be done from driver.
>>>
>>>> why I have kept them as fallback as Conor said in this email. Hope you
>>>> all OK with this.
>>>
>>> Did you read the feedback? Your response is not solving here anything.
>>> How 8650 can be used twice? Please point me to DTS showing both usages.
>> May be I have a misunderstanding here. Let's clarify it.
>>
>> LAN8650 and LAN8651 both are two different variants but both implements
>> same functionality. The only difference is LAN8651 has "Single 3.3V
>> supply with integrated" where LAN8650 doesn't have this. This is
>> hardware specific difference and there is no configuration/setting to be
>> done in the driver specific to this difference in the LAN8651. So
>> basically the driver can support for both variants without any
>> additional settings.
>>
>> LAN8650: https://www.microchip.com/en-us/product/lan8650
>> LAN8651: https://www.microchip.com/en-us/product/lan8651
>>
>> The below link shows the difference between them,
>> https://www.microchip.com/en-us/product-comparison.lan8650.lan8651
>>
>> With the above details, I would change the microchip,lan865x.yaml with
>> the below details.
>>
>> compatible:
>> enum:
>> - microchip,lan8650
>> - microchip,lan8651
>>
>> And in the lan865x.c, I would remove the below line because
>> .compatible = "microchip,lan8650" already supports for LAN8651 as well.
>>
>> .compatible = "microchip,lan8651"
>>
>> Let me know your opinion on this proposal? or do you have any
>> misunderstanding here?
>
> It's still wrong. Upstream your DTS and then test it. You will
> immediately see that it does not work. So first make it working, then
> send code to review.
Sorry for the inconvenience. I did the below changes in my
microchip,lan865x.yaml file and executed dt_binding_check. It
successfully created the microchip,lan865x.example.dts without any
errors. Herewith I have attached the updated microchip,lan865x.yaml file
and the generated microchip,lan865x.example.dts file for your reference.
properties:
compatible:
oneOf:
- items:
- const: microchip,lan8651
- const: microchip,lan8650
....
ethernet@0 {
compatible = "microchip,lan8651", "microchip,lan8650";
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>;
};
LAN8650 is the fallback here as the driver uses the compatible string as
"microchip,lan8650". I am using LAN8651 in my RPI4 test setup. So I
added the below compatible string
compatible = "microchip,lan8651", "microchip,lan8650"
in my RPI 4 DT overlay file and tested in my RPI 4 test setup and it
worked well.
Herewith I have attached my RPI 4 dt overlay file lan8651-overlay.dts
for your reference.
Hope this is OK now?
Best regards,
Parthiban V
>
> Best regards,
> Krzysztof
>
>
On Thu, Mar 21, 2024 at 12:00:56PM +0000, [email protected] wrote:
> Hi Krzysztof,
>
> On 21/03/24 2:10 pm, Krzysztof Kozlowski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 21/03/2024 09:38, [email protected] wrote:
> >> Hi Krzysztof,
> >>
> >> On 20/03/24 3:23 pm, Krzysztof Kozlowski wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On 20/03/2024 09:40, [email protected] wrote:
> >>>> Hi Conor & Andrew,
> >>>>
> >>>> Please find my reply below by consolidating other two emails comments
> >>>> related to this.
> >>>>
> >>>> On 07/03/24 12:31 am, Conor Dooley wrote:
> >>>>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
> >>>>>>>> +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).
> >>>>>>>> +
> >>>>>>>> +allOf:
> >>>>>>>> + - $ref: ethernet-controller.yaml#
> >>>>>>>> +
> >>>>>>>> +properties:
> >>>>>>>> + compatible:
> >>>>>>>> + oneOf:
> >>>>>>>> + - items:
> >>>>>>>> + - const: microchip,lan8650
> >>>>>>>> + - const: microchip,lan8651
> >>>>>>> The order here is wrong, lan8561 needs to come before the fallback of
> >>>>>>> lan8650.
> >>>>>> I don't think it is a fallback. There are two devices, and hence two
> >>>>>> different compatibles. So i suspect the -items: is wrong here?
> >>>>> It'd just be a two entry enum then, but I did take a quick look at the
> >>>>> driver earlier and saw:
> >>>>> +static const struct of_device_id lan865x_dt_ids[] = {
> >>>>> + { .compatible = "microchip,lan8650" },
> >>>>> + { .compatible = "microchip,lan8651" },
> >>>>> + { /* Sentinel */ }
> >>>>> +};
> >>>>>
> >>>>> That, along with no other of_device_is_compatible() type operations
> >>>>> made me think that having a fallback actually was suitable.
> >>>>>
> >>>>> You cropped it out, but the patch had:
> >>>>>> + compatible:
> >>>>>> + oneOf:
> >>>>>> + - items:
> >>>>>> + - const: microchip,lan8650
> >>>>>> + - const: microchip,lan8651
> >>>>>> + - enum:
> >>>>>> + - microchip,lan8650
> >>>>> So it doesn't appear to be an accidental items in place of an enum,
> >>>>> since the other compatible is in another enum.
> >>>> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
> >>>> two different variants but they both share almost all characteristics
> >>>> except one thing that is LAN8651 has "Single 3.3V supply with integrated
> >>>> 1.8V regulator" which doesn't have anything to do with driver. That's
> >>>
> >>> So why this is not reflected in your driver? Why didn't you address that
> >>> part, but ignored?
> >> No, it is not ignored. This difference is specific to hardware and there
> >> is no configuration/setting to be done from driver.
> >>>
> >>>> why I have kept them as fallback as Conor said in this email. Hope you
> >>>> all OK with this.
> >>>
> >>> Did you read the feedback? Your response is not solving here anything.
> >>> How 8650 can be used twice? Please point me to DTS showing both usages.
> >> May be I have a misunderstanding here. Let's clarify it.
> >>
> >> LAN8650 and LAN8651 both are two different variants but both implements
> >> same functionality. The only difference is LAN8651 has "Single 3.3V
> >> supply with integrated" where LAN8650 doesn't have this. This is
> >> hardware specific difference and there is no configuration/setting to be
> >> done in the driver specific to this difference in the LAN8651. So
> >> basically the driver can support for both variants without any
> >> additional settings.
> >>
> >> LAN8650: https://www.microchip.com/en-us/product/lan8650
> >> LAN8651: https://www.microchip.com/en-us/product/lan8651
> >>
> >> The below link shows the difference between them,
> >> https://www.microchip.com/en-us/product-comparison.lan8650.lan8651
> >>
> >> With the above details, I would change the microchip,lan865x.yaml with
> >> the below details.
> >>
> >> compatible:
> >> enum:
> >> - microchip,lan8650
> >> - microchip,lan8651
> >>
> >> And in the lan865x.c, I would remove the below line because
> >> .compatible = "microchip,lan8650" already supports for LAN8651 as well.
> >>
> >> .compatible = "microchip,lan8651"
> >>
> >> Let me know your opinion on this proposal? or do you have any
> >> misunderstanding here?
> >
> > It's still wrong. Upstream your DTS and then test it. You will
> > immediately see that it does not work. So first make it working, then
> > send code to review.
> Sorry for the inconvenience. I did the below changes in my
> microchip,lan865x.yaml file and executed dt_binding_check. It
> successfully created the microchip,lan865x.example.dts without any
> errors. Herewith I have attached the updated microchip,lan865x.yaml file
> and the generated microchip,lan865x.example.dts file for your reference.
>
> properties:
> compatible:
> oneOf:
> - items:
> - const: microchip,lan8651
> - const: microchip,lan8650
No, this is not right either. You need to also allow the lan8650 on its
own. All you had to do with the original items list was flip the order
of the lan8650 and lan8651.
> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6) {
> + int ret;
> +
> + tc6->mdiobus = mdiobus_alloc();
> + if (!tc6->mdiobus) {
> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
> + return -ENODEV;
> + }
Shouldn't it be appropriate to return -ENOMEM here?
> +
> + tc6->mdiobus->priv = tc6;
> + tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
> + tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
> + tc6->mdiobus->name = "oa-tc6-mdiobus";
> + tc6->mdiobus->parent = tc6->dev;
> +
> > > This second part is clearly an optimisation. If you have lots of full
> > > MTU packets, 1514 bytes, they take around 24 chunks. Having the last
> > > chunk only 1/2 full does not waste too much bandwidth. But if you are
> > > carrying lots of small packets, say voice, 130 bytes, the wasted
> > > bandwidth starts to add up. But is there a use case for 10Mbps of
> > > small packets? I doubt it.
> > Yes, for sure there is a possibility to get into this scenario and the protocol also
> > supports that. But as proposed by you below, let's implement it as part of
> > optimization later.
> > >
> > > So if you don't have the ability to combine two packets into one
> > > chunk, i would do that later. Lets get the basics merged first, it can
> > > be optimised later.
> > Yes, I agree with this proposal to get the basic version merged first.
>
> While latency is important, so is using the available bandwidth efficiently. Here is a suggestion. We know that the tx credit available basically tells us,
> how many chunks could be transmitted without overflow. Instead of stopping the netif queue based on number of skbs queued, why not stop the queue based on
> number of bytes accumulated? Basically, at any given point of time, we enqueue the tx_skb_q until we are have enough bytes to cross the threshold of (tc6->tc_credit * OA_TC6_CHUNK_PAYLOAD_SIZE).
> This way, during the next transmit, we could utilize the whole available credits. Bandwidth utilization between bigger frames and smaller frames would be not be vastly different.
Please configure your email client to wrap emails at around 70
characters.
tc_credit is 5 bits. So it is a maximum of 32.
A 1514 frame takes around 24 chunks. So you only need two full size
frames to consume all your possible credit.
If you happen to have smaller voice packets, say 130 bytes, you need
three chunks to send it. So you might want to have 10 such packets on
hand in order to make use of all your credit. But if you have 10 voice
packets to send in a burst, your voice quality is going to be bad,
they should be 10ms to 20ms apart, not in a burst...
I don't like the original idea of having lots of packets in a transmit
queue. But having 1/2 dozen should not be an issue.
In general, we prefer things to be simple. We can then optimise later,
and use benchmarks to show the optimisations really do bring a benefit
to justify the added complexity.
Andrew
> -----Original Message-----
> From: [email protected]
> <[email protected]>
> Sent: Wednesday, March 20, 2024 3:43 AM
> To: [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> Piergiorgio Beruto <[email protected]>; Selvamani Rajagopal
> <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next v3 08/12] net: ethernet: oa_tc6: implement
> transmit path to transfer tx ethernet frames
>
> [External Email]: This email arrived from an external source - Please exercise
> caution when opening any attachments or clicking on links.
>
> Hi Andrew,
>
> On 19/03/24 6:49 pm, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > On Tue, Mar 19, 2024 at 12:54:30PM +0000,
> [email protected] wrote:
> >> Hi Andrew,
> >>
> >> On 07/03/24 10:38 pm, Andrew Lunn wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>> know the content is safe
> >>>
> >>>> @@ -55,6 +77,14 @@
> >>>> (OA_TC6_CTRL_MAX_REGISTERS *\
> >>>> OA_TC6_CTRL_REG_VALUE_SIZE) +\
> >>>>
> >>>> OA_TC6_CTRL_IGNORED_SIZE)
> >>>> +#define OA_TC6_CHUNK_PAYLOAD_SIZE 64
> >>>> +#define OA_TC6_DATA_HEADER_SIZE 4
> >>>> +#define OA_TC6_CHUNK_SIZE (OA_TC6_DATA_HEADER_SIZE
> +\
> >>>> + OA_TC6_CHUNK_PAYLOAD_SIZE)
> >>>> +#define OA_TC6_TX_SKB_QUEUE_SIZE 100
> >>>
> >>> So you keep up to 100 packets in a queue. If use assume typical MTU
> >>> size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
> >>> traffic. That is quite a lot of latency when a high priority packet
> >>> is added to the tail of the queue and needs to wait for all the
> >>> other packets to be sent first.
> >>>
> >>> Chunks are 64 bytes. So in practice, you only ever need two packets.
> >>> You need to be able to fill a chunk with the final part of one
> >>> packet, and the beginning of the next. So i would try using a much
> >>> smaller queue size. That will allow Linux queue disciplines to give
> >>> you the high priority packets first which you send with low latency.
> >> Thanks for the detailed explanation. If I understand you correctly,
> >>
> >> 1. The tx skb queue size (OA_TC6_TX_SKB_QUEUE_SIZE) should be 2 to
> >> avoid the latency when a high priority packet added.
> >>
> >> 2. Need to implement the handling part of the below case, In case if
> >> one packet ends in a chunk and that chunk still having some space
> >> left to accommodate some bytes from the next packet if available from
> >> network layer.
> >
> > This second part is clearly an optimisation. If you have lots of full
> > MTU packets, 1514 bytes, they take around 24 chunks. Having the last
> > chunk only 1/2 full does not waste too much bandwidth. But if you are
> > carrying lots of small packets, say voice, 130 bytes, the wasted
> > bandwidth starts to add up. But is there a use case for 10Mbps of
> > small packets? I doubt it.
> Yes, for sure there is a possibility to get into this scenario and the protocol also
> supports that. But as proposed by you below, let's implement it as part of
> optimization later.
> >
> > So if you don't have the ability to combine two packets into one
> > chunk, i would do that later. Lets get the basics merged first, it can
> > be optimised later.
> Yes, I agree with this proposal to get the basic version merged first.
While latency is important, so is using the available bandwidth efficiently. Here is a suggestion. We know that the tx credit available basically tells us,
how many chunks could be transmitted without overflow. Instead of stopping the netif queue based on number of skbs queued, why not stop the queue based on
number of bytes accumulated? Basically, at any given point of time, we enqueue the tx_skb_q until we are have enough bytes to cross the threshold of (tc6->tc_credit * OA_TC6_CHUNK_PAYLOAD_SIZE).
This way, during the next transmit, we could utilize the whole available credits. Bandwidth utilization between bigger frames and smaller frames would be not be vastly different.
>
> Best regards,
> Parthiban V
> >
> > Andrew
On 22/03/24 12:19 am, Selvamani Rajagopal 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
>
>> +static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6) {
>> + int ret;
>> +
>> + tc6->mdiobus = mdiobus_alloc();
>> + if (!tc6->mdiobus) {
>> + netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>> + return -ENODEV;
>> + }
>
> Shouldn't it be appropriate to return -ENOMEM here?
Yes, will change it in the next version.
Best regards,
Parthiban V
>
>> +
>> + tc6->mdiobus->priv = tc6;
>> + tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
>> + tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
>> + tc6->mdiobus->name = "oa-tc6-mdiobus";
>> + tc6->mdiobus->parent = tc6->dev;
>> +
>
Hi Conor,
On 21/03/24 9:04 pm, Conor Dooley wrote:
> On Thu, Mar 21, 2024 at 12:00:56PM +0000,[email protected] wrote:
>> Hi Krzysztof,
>>
>> On 21/03/24 2:10 pm, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 21/03/2024 09:38,[email protected] wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 20/03/24 3:23 pm, Krzysztof Kozlowski wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 20/03/2024 09:40,[email protected] wrote:
>>>>>> Hi Conor & Andrew,
>>>>>>
>>>>>> Please find my reply below by consolidating other two emails comments
>>>>>> related to this.
>>>>>>
>>>>>> On 07/03/24 12:31 am, Conor Dooley wrote:
>>>>>>> On Wed, Mar 06, 2024 at 07:48:57PM +0100, Andrew Lunn wrote:
>>>>>>>>>> +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).
>>>>>>>>>> +
>>>>>>>>>> +allOf:
>>>>>>>>>> + - $ref: ethernet-controller.yaml#
>>>>>>>>>> +
>>>>>>>>>> +properties:
>>>>>>>>>> + compatible:
>>>>>>>>>> + oneOf:
>>>>>>>>>> + - items:
>>>>>>>>>> + - const: microchip,lan8650
>>>>>>>>>> + - const: microchip,lan8651
>>>>>>>>> The order here is wrong, lan8561 needs to come before the fallback of
>>>>>>>>> lan8650.
>>>>>>>> I don't think it is a fallback. There are two devices, and hence two
>>>>>>>> different compatibles. So i suspect the -items: is wrong here?
>>>>>>> It'd just be a two entry enum then, but I did take a quick look at the
>>>>>>> driver earlier and saw:
>>>>>>> +static const struct of_device_id lan865x_dt_ids[] = {
>>>>>>> + { .compatible = "microchip,lan8650" },
>>>>>>> + { .compatible = "microchip,lan8651" },
>>>>>>> + { /* Sentinel */ }
>>>>>>> +};
>>>>>>>
>>>>>>> That, along with no other of_device_is_compatible() type operations
>>>>>>> made me think that having a fallback actually was suitable.
>>>>>>>
>>>>>>> You cropped it out, but the patch had:
>>>>>>>> + compatible:
>>>>>>>> + oneOf:
>>>>>>>> + - items:
>>>>>>>> + - const: microchip,lan8650
>>>>>>>> + - const: microchip,lan8651
>>>>>>>> + - enum:
>>>>>>>> + - microchip,lan8650
>>>>>>> So it doesn't appear to be an accidental items in place of an enum,
>>>>>>> since the other compatible is in another enum.
>>>>>> As per Andrew's comment in another email, both LAN8650 and LAN8651 are
>>>>>> two different variants but they both share almost all characteristics
>>>>>> except one thing that is LAN8651 has "Single 3.3V supply with integrated
>>>>>> 1.8V regulator" which doesn't have anything to do with driver. That's
>>>>> So why this is not reflected in your driver? Why didn't you address that
>>>>> part, but ignored?
>>>> No, it is not ignored. This difference is specific to hardware and there
>>>> is no configuration/setting to be done from driver.
>>>>>> why I have kept them as fallback as Conor said in this email. Hope you
>>>>>> all OK with this.
>>>>> Did you read the feedback? Your response is not solving here anything.
>>>>> How 8650 can be used twice? Please point me to DTS showing both usages.
>>>> May be I have a misunderstanding here. Let's clarify it.
>>>>
>>>> LAN8650 and LAN8651 both are two different variants but both implements
>>>> same functionality. The only difference is LAN8651 has "Single 3.3V
>>>> supply with integrated" where LAN8650 doesn't have this. This is
>>>> hardware specific difference and there is no configuration/setting to be
>>>> done in the driver specific to this difference in the LAN8651. So
>>>> basically the driver can support for both variants without any
>>>> additional settings.
>>>>
>>>> LAN8650:https://www.microchip.com/en-us/product/lan8650
>>>> LAN8651:https://www.microchip.com/en-us/product/lan8651
>>>>
>>>> The below link shows the difference between them,
>>>> https://www.microchip.com/en-us/product-comparison.lan8650.lan8651
>>>>
>>>> With the above details, I would change the microchip,lan865x.yaml with
>>>> the below details.
>>>>
>>>> compatible:
>>>> enum:
>>>> - microchip,lan8650
>>>> - microchip,lan8651
>>>>
>>>> And in the lan865x.c, I would remove the below line because
>>>> .compatible = "microchip,lan8650" already supports for LAN8651 as well.
>>>>
>>>> .compatible = "microchip,lan8651"
>>>>
>>>> Let me know your opinion on this proposal? or do you have any
>>>> misunderstanding here?
>>> It's still wrong. Upstream your DTS and then test it. You will
>>> immediately see that it does not work. So first make it working, then
>>> send code to review.
>> Sorry for the inconvenience. I did the below changes in my
>> microchip,lan865x.yaml file and executed dt_binding_check. It
>> successfully created the microchip,lan865x.example.dts without any
>> errors. Herewith I have attached the updated microchip,lan865x.yaml file
>> and the generated microchip,lan865x.example.dts file for your reference.
>>
>> properties:
>> compatible:
>> oneOf:
>> - items:
>> - const: microchip,lan8651
>> - const: microchip,lan8650
> No, this is not right either. You need to also allow the lan8650 on its
> own. All you had to do with the original items list was flip the order
> of the lan8650 and lan8651.
Ah ok, now I understand this. Then it is supposed to be like below,
properties:
compatible:
oneOf:
- const: microchip,lan8650
- items:
- const: microchip,lan8651
- const: microchip,lan8650
Executed dt_binding_check with the above update and it was successful.
Hope this is OK?
Best regards,
Parthiban V
On 22/03/2024 07:25, [email protected] wrote:
>>> properties:
>>> compatible:
>>> oneOf:
>>> - items:
>>> - const: microchip,lan8651
>>> - const: microchip,lan8650
>> No, this is not right either. You need to also allow the lan8650 on its
>> own. All you had to do with the original items list was flip the order
>> of the lan8650 and lan8651.
> Ah ok, now I understand this. Then it is supposed to be like below,
>
> properties:
>
> compatible:
>
> oneOf:
>
> - const: microchip,lan8650
>
> - items:
>
> - const: microchip,lan8651
>
> - const: microchip,lan8650
>
> Executed dt_binding_check with the above update and it was successful.
> Hope this is OK?
This is the third time you ask us. None of the previous cases were
actually tested. Maybe this one was, maybe not. I assume the latter.
First, test your code.
Best regards,
Krzysztof
Hi Krzysztof,
On 22/03/24 12:33 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 22/03/2024 07:25, [email protected] wrote:
>>>> properties:
>>>> compatible:
>>>> oneOf:
>>>> - items:
>>>> - const: microchip,lan8651
>>>> - const: microchip,lan8650
>>> No, this is not right either. You need to also allow the lan8650 on its
>>> own. All you had to do with the original items list was flip the order
>>> of the lan8650 and lan8651.
>> Ah ok, now I understand this. Then it is supposed to be like below,
>>
>> properties:
>>
>> compatible:
>>
>> oneOf:
>>
>> - const: microchip,lan8650
>>
>> - items:
>>
>> - const: microchip,lan8651
>>
>> - const: microchip,lan8650
>>
>> Executed dt_binding_check with the above update and it was successful.
>> Hope this is OK?
>
> This is the third time you ask us. None of the previous cases were
> actually tested. Maybe this one was, maybe not. I assume the latter.
>
> First, test your code.
As I mentioned in the previous email itself, I tested this case and the
previous case both in my RPI 4 setup before replying to the comment. The
question I ask is to check whether the person who comments is OK with
the changes or not. Didn't you see the attachments in my previous email
which I used in my RPI 4 dts setup for my testing? if not I can forward
it again.
Best regards,
Parthiban V
>
> Best regards,
> Krzysztof
>
On Fri, Mar 22, 2024 at 06:25:02AM +0000, [email protected] wrote:
> Ah ok, now I understand this. Then it is supposed to be like below,
>
> properties:
>
> compatible:
>
> oneOf:
>
> - const: microchip,lan8650
>
> - items:
>
> - const: microchip,lan8651
>
> - const: microchip,lan8650
>
> Executed dt_binding_check with the above update and it was successful.
> Hope this is OK?
That looks about what I would expect to see, yes.
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, March 21, 2024 12:42 PM
> To: Selvamani Rajagopal <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Piergiorgio Beruto
> <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH net-next v3 08/12] net: ethernet: oa_tc6:
> implement transmit path to transfer tx ethernet frames
>
> [External Email]: This email arrived from an external source - Please
> exercise caution when opening any attachments or clicking on links.
>
> > > > This second part is clearly an optimisation. If you have lots of
> > > > full MTU packets, 1514 bytes, they take around 24 chunks. Having
> > > > the last chunk only 1/2 full does not waste too much bandwidth.
> > > > But if you are carrying lots of small packets, say voice, 130
> > > > bytes, the wasted bandwidth starts to add up. But is there a use
> > > > case for 10Mbps of small packets? I doubt it.
> > > Yes, for sure there is a possibility to get into this scenario and
> > > the protocol also supports that. But as proposed by you below, let's
> > > implement it as part of optimization later.
> > > >
> > > > So if you don't have the ability to combine two packets into one
> > > > chunk, i would do that later. Lets get the basics merged first, it
> > > > can be optimised later.
> > > Yes, I agree with this proposal to get the basic version merged first.
> >
> > While latency is important, so is using the available bandwidth
> > efficiently. Here is a suggestion. We know that the tx credit
> > available basically tells us, how many chunks could be transmitted
> without overflow. Instead of stopping the netif queue based on number
> of skbs queued, why not stop the queue based on number of bytes
> accumulated? Basically, at any given point of time, we enqueue the
> tx_skb_q until we are have enough bytes to cross the threshold of (tc6-
> >tc_credit * OA_TC6_CHUNK_PAYLOAD_SIZE).
> > This way, during the next transmit, we could utilize the whole available
> credits. Bandwidth utilization between bigger frames and smaller
> frames would be not be vastly different.
>
> Please configure your email client to wrap emails at around 70
> characters.
>
> tc_credit is 5 bits. So it is a maximum of 32.
>
> A 1514 frame takes around 24 chunks. So you only need two full size
> frames to consume all your possible credit.
>
> If you happen to have smaller voice packets, say 130 bytes, you need
> three chunks to send it. So you might want to have 10 such packets on
> hand in order to make use of all your credit. But if you have 10 voice
> packets to send in a burst, your voice quality is going to be bad, they
> should be 10ms to 20ms apart, not in a burst...
>
> I don't like the original idea of having lots of packets in a transmit queue.
> But having 1/2 dozen should not be an issue.
>
> In general, we prefer things to be simple. We can then optimise later,
> and use benchmarks to show the optimisations really do bring a benefit
> to justify the added complexity.
True. I should get some performance numbers to see where we are
with the current code. That would be time to look at the improvement.
>
> Andrew
On 22/03/2024 09:28, [email protected] wrote:
>>>
>>> - const: microchip,lan8650
>>>
>>> - items:
>>>
>>> - const: microchip,lan8651
>>>
>>> - const: microchip,lan8650
>>>
>>> Executed dt_binding_check with the above update and it was successful.
>>> Hope this is OK?
>>
>> This is the third time you ask us. None of the previous cases were
>> actually tested. Maybe this one was, maybe not. I assume the latter.
>>
>> First, test your code.
> As I mentioned in the previous email itself, I tested this case and the
> previous case both in my RPI 4 setup before replying to the comment. The
I don't understand how one can test bindings and DTS on RPI 4. Testing
is with dt_bindings_check and dtbs_check.
Best regards,
Krzysztof
Hi Krzysztof,
On 23/03/24 3:54 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 22/03/2024 09:28, [email protected] wrote:
>>>>
>>>> - const: microchip,lan8650
>>>>
>>>> - items:
>>>>
>>>> - const: microchip,lan8651
>>>>
>>>> - const: microchip,lan8650
>>>>
>>>> Executed dt_binding_check with the above update and it was successful.
>>>> Hope this is OK?
>>>
>>> This is the third time you ask us. None of the previous cases were
>>> actually tested. Maybe this one was, maybe not. I assume the latter.
>>>
>>> First, test your code.
>> As I mentioned in the previous email itself, I tested this case and the
>> previous case both in my RPI 4 setup before replying to the comment. The
>
> I don't understand how one can test bindings and DTS on RPI 4. Testing
> is with dt_bindings_check and dtbs_check.
Ok, may be I had a misunderstanding here. Every time I used to test the
driver with the generated dts in RPI 4. I thought you are asking about
it. I used the below commands to check the dt bindings and dtbs.
$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/microchip,lan865x.yaml
dt_binding_check
$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/microchip,lan865x.yaml
dtbs_check
They didn't report any error.
Best regards,
Parthiban V
>
>
>
> Best regards,
> Krzysztof
>
>
Hi Conor,
On 22/03/24 11:38 pm, Conor Dooley wrote:
> On Fri, Mar 22, 2024 at 06:25:02AM +0000,[email protected] wrote:
>> Ah ok, now I understand this. Then it is supposed to be like below,
>>
>> properties:
>>
>> compatible:
>>
>> oneOf:
>>
>> - const: microchip,lan8650
>>
>> - items:
>>
>> - const: microchip,lan8651
>>
>> - const: microchip,lan8650
>>
>> Executed dt_binding_check with the above update and it was successful.
>> Hope this is OK?
> That looks about what I would expect to see, yes.
OK, thanks for the confirmation. I will update this change in the next
version.
Best regards,
Parthiban V
>
Hi Krzysztof,
On 23/03/24 3:54 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 22/03/2024 09:28, [email protected] wrote:
>>>>
>>>> - const: microchip,lan8650
>>>>
>>>> - items:
>>>>
>>>> - const: microchip,lan8651
>>>>
>>>> - const: microchip,lan8650
>>>>
>>>> Executed dt_binding_check with the above update and it was successful.
>>>> Hope this is OK?
>>>
>>> This is the third time you ask us. None of the previous cases were
>>> actually tested. Maybe this one was, maybe not. I assume the latter.
>>>
>>> First, test your code.
>> As I mentioned in the previous email itself, I tested this case and the
>> previous case both in my RPI 4 setup before replying to the comment. The
>
> I don't understand how one can test bindings and DTS on RPI 4. Testing
> is with dt_bindings_check and dtbs_check.
Ok, may be I had a misunderstanding here. Every time I used to test the
driver with the generated dts in RPI 4. I thought you are asking about
it. I used the below commands to check the dt bindings and dtbs.
$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/microchip,lan865x.yaml
dt_binding_check
$ make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf-
DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/microchip,lan865x.yaml
dtbs_check
They didn't report any error.
Best regards,
Parthiban V
>
>
>
> Best regards,
> Krzysztof
>
>
Hi Andrew,
On 18/03/24 4:31 pm, [email protected] wrote:
> Hi Andrew,
>
> On 08/03/24 7:03 pm, Andrew Lunn wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>>> Ok, as per the table 6 in the spec, PHY C45 registers are mapped in the
>>> MMS like below,
>>>
>>> PHY – PCS Registers (MMD 3) ---> MMS 2
>>> PHY – PMA/PMD Registers (MMD 1) ---> MMS 3
>>> PHY – Vendor Specific and PLCA Registers (MMD 31) ---> MMS 4
>>> PHY – Auto-Negotiation Registers (MMD 7) ---> MMS 5
>>> PHY – Power Unit (MMD 13) ---> MMS 6
>>>
>>> MMD 13 for PHY - Power Unit is not defined in the mdio.h. So in the
>>> below code I have defined it locally (MDIO_MMD_POWER_UNIT). May be
>>> needed to do this in the mdio.h file when coming to this patch.
>>
>> Yes, please add it to mdio.h
> Sure will add it in the mdio.h file.
>>
>>> /* PHY – Clause 45 registers memory map selector (MMS) as per table 6 in
>>> the OPEN Alliance specification.
>>> */
>>> #define OA_TC6_PHY_PCS_MMS2 2 /* MMD 3 */
>>> #define OA_TC6_PHY_PMA_PMD_MMS3 3 /* MMD 1 */
>>> #define OA_TC6_PHY_VS_PLCA_MMS4 4 /* MMD 31 */
>>> #define OA_TC6_PHY_AUTO_NEG_MMS5 5 /* MMD 7 */
>>> #define OA_TC6_PHY_POWER_UNIT_MMS6 6 /* MMD 13 */
>>>
>>> /* MDIO Manageable Device (MMD) for PHY Power Unit */
>>> #define MDIO_MMD_POWER_UNIT 13 /* PHY Power Unit */
>>>
>>> static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int
>>> devnum, int regnum)
>>> {
>>>
>>> struct oa_tc6 *tc6 = bus->priv;
>>>
>>> u32 regval;
>>>
>>> bool ret;
>>>
>>> u32 mms;
>>>
>>>
>>>
>>> if (devnum == MDIO_MMD_PCS)
>>>
>>> mms = OA_TC6_PHY_PCS_MMS2;
>>>
>>> else if (devnum == MDIO_MMD_PMAPMD)
>>>
>>> mms = OA_TC6_PHY_PMA_PMD_MMS3;
>>>
>>> else if (devnum == MDIO_MMD_VEND2)
>>>
>>> mms = OA_TC6_PHY_VS_PLCA_MMS4;
>>>
>>> else if (devnum == MDIO_MMD_AN)
>>>
>>> mms = OA_TC6_PHY_AUTO_NEG_MMS5;
>>>
>>> else if (devnum == MDIO_MMD_POWER_UNIT)
>>>
>>> mms = OA_TC6_PHY_POWER_UNIT_MMS6;
>>
>> I would probably use a switch statement.
> Ah ok, I will use switch here.
>>
>>>
>>> else
>>>
>>> return -ENOTSUPP;
>>
>> 802.3 says:
>>
>> If a device supports the MDIO interface it shall respond to all
>> possible register addresses for the device and return a value of
>> zero for undefined and unsupported registers. Writes to undefined
>> registers and read-only registers shall have no effect. The
>> operation of an MMD shall not be affected by writes to reserved and
>> unsupported register bits, and such register bits shall return a
>> value of zero when read.
>>
>> So maybe return 0. ENOTSUPP is wrong, that is an NFS only error
>> code. The generic one is EOPNOTSUPP. I would say -EOPNOTSUPP is also
>> O.K.
> Sure, I will use -EOPNOTSUPP in the next version instead of -ENOTSUPP.
>>
>>> ret = oa_tc6_read_register(tc6, (mms << 16) | regnum, ®val);
>>>
>>> if (ret)
>>>
>>> return -ENODEV;
>>
>> oa_tc6_read_register() should return an error code, so return whatever
>> is returns. Don't overwrite error codes. It makes it harder to track
>> errors through the call stack.
> Ah ok, will return the error code from oa_tc6_read_register() in the
> next version.
After implementing the new APIs oa_tc6_mdiobus_read_c45() and
oa_tc6_mdiobus_write_c45(), I tried testing. But, for some reason these
APIs are never get called by phy_read_mmd() or phy_write_mmd() as those
APIs are checking for phydev->is_c45 flag for calling this new c45 APIs
which is not set in this case.
If the phydev is found via c22, phylib does not set phydev->is_c45, and
everything ends up going indirect.
To verify the APIs, I just called them locally withing the oa_tc6_init()
function, there I got the expected results but accessing them through
phylib is not working because of the above reason.
Do you have any idea or suggestion to test this APIs or do I miss
anything here?
Best regards,
Parthiban V
>
> Best regards,
> Parthiban V
>>
>> Andrew
>>
>
On Fri, Apr 12, 2024 at 10:43:15AM +0000, [email protected] wrote:
> Hi Andrew,
>
> After implementing the new APIs oa_tc6_mdiobus_read_c45() and
> oa_tc6_mdiobus_write_c45(), I tried testing. But, for some reason these
> APIs are never get called by phy_read_mmd() or phy_write_mmd() as those
> APIs are checking for phydev->is_c45 flag for calling this new c45 APIs
> which is not set in this case.
>
> If the phydev is found via c22, phylib does not set phydev->is_c45, and
> everything ends up going indirect.
We don't have a clean separation between C22/C45 register space and
C22/C45 MDIO bus protocols. As you said, if the PHY is discovered via
C22 bus protocol it assumes it uses C22 protocol.
Those PHYs which do support C45, and in particular, features which
need C45, all call genphy_c45_pma_read_abilities() in there
get_features function to add in C45 features. However, it will
continue using C45 over C22 because genphy_c45_pma_read_abilities()
only really says the device has C45 registers, not C45 protocol.
I can see two different things you can try.
1) set .read_mmd and .write_mmd in the PHY driver to phy_read_mmd()
and phy_write_mmd(). That is not great however, since each vendor
is likely to have their own PHY driver.
2) Add a helper to set is_c45. This however has side effects you might
not want. e.g. auto-neg will be performed via C45, not C22. This
might not matter for a T1 PHY where is think auto-neg is not
actually used. But i don't see anything in TC6 which limits it to
T1, i could well imagine a T2 or T4 PHY being used with full
auto-neg.
We really do need to separate C45 registers from C45 protocol in the
phylib core to cleanly solve this.
Andrew
Hi Andrew,
On 15/04/24 6:45 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Apr 12, 2024 at 10:43:15AM +0000, [email protected] wrote:
>> Hi Andrew,
>>
>> After implementing the new APIs oa_tc6_mdiobus_read_c45() and
>> oa_tc6_mdiobus_write_c45(), I tried testing. But, for some reason these
>> APIs are never get called by phy_read_mmd() or phy_write_mmd() as those
>> APIs are checking for phydev->is_c45 flag for calling this new c45 APIs
>> which is not set in this case.
>>
>> If the phydev is found via c22, phylib does not set phydev->is_c45, and
>> everything ends up going indirect.
>
> We don't have a clean separation between C22/C45 register space and
> C22/C45 MDIO bus protocols. As you said, if the PHY is discovered via
> C22 bus protocol it assumes it uses C22 protocol.
>
Thanks for confirming my understanding.
> Those PHYs which do support C45, and in particular, features which
> need C45, all call genphy_c45_pma_read_abilities() in there
> .get_features function to add in C45 features. However, it will
> continue using C45 over C22 because genphy_c45_pma_read_abilities()
> only really says the device has C45 registers, not C45 protocol.
>
OK.
> I can see two different things you can try.
>
> 1) set .read_mmd and .write_mmd in the PHY driver to phy_read_mmd()
> and phy_write_mmd(). That is not great however, since each vendor
> is likely to have their own PHY driver.
>
I tried this approach and it works as expected. Means whenever there is
a c45 register access, it directly uses the
oa_tc6_read_c45()/oa_tc6_write_c45() functions. Herewith I have attached
the patch
(v4-0006-net-ethernet-oa_tc6-implement-internal-PHY-initia.patch) which
has this new implementation for your reference. Is this you expected?
Can you comment on this?
> 2) Add a helper to set is_c45. This however has side effects you might
> not want. e.g. auto-neg will be performed via C45, not C22. This
> might not matter for a T1 PHY where is think auto-neg is not
> actually used. But i don't see anything in TC6 which limits it to
> T1, i could well imagine a T2 or T4 PHY being used with full
> auto-neg.
>
I tried this approach by setting up is_c45 flag when I use
phy_read_mmd() function but ended up with the kernel call trace
(c45_kernel_call_trace.png) attached here for your reference.
> We really do need to separate C45 registers from C45 protocol in the
> phylib core to cleanly solve this.
OK. In my opinion, until having the above implementation in the phylib
core, shall we go with the first approach [1] I tried out as it is
fulfilling our requirement? What do you think?
Best regards,
Parthiban V
>
> Andrew
>
> I tried this approach and it works as expected. Means whenever there is
> a c45 register access, it directly uses the
> oa_tc6_read_c45()/oa_tc6_write_c45() functions. Herewith I have attached
> the patch
> (v4-0006-net-ethernet-oa_tc6-implement-internal-PHY-initia.patch) which
> has this new implementation for your reference. Is this you expected?
> Can you comment on this?
Please just post a new patch series. I will then review it just like
other patches. Its O.K. to send patch series frequently, not just more
than one per day.
> I tried this approach by setting up is_c45 flag when I use
> phy_read_mmd() function but ended up with the kernel call trace
> (c45_kernel_call_trace.png) attached here for your reference.
Please post plain ASCII. I assume you have a serial port, so you
should be able to capture it. I'm not too surprised though, no other
driver plays with is_c45.
Andrew
Hi Andrew,
On 16/04/24 11:48 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> I tried this approach and it works as expected. Means whenever there is
>> a c45 register access, it directly uses the
>> oa_tc6_read_c45()/oa_tc6_write_c45() functions. Herewith I have attached
>> the patch
>> (v4-0006-net-ethernet-oa_tc6-implement-internal-PHY-initia.patch) which
>> has this new implementation for your reference. Is this you expected?
>> Can you comment on this?
>
> Please just post a new patch series. I will then review it just like
> other patches. Its O.K. to send patch series frequently, not just more
> than one per day.
>
Sure, then I will send v4 patch series with this changes as well soon
for the review.
>> I tried this approach by setting up is_c45 flag when I use
>> phy_read_mmd() function but ended up with the kernel call trace
>> (c45_kernel_call_trace.png) attached here for your reference.
>
> Please post plain ASCII. I assume you have a serial port, so you
> should be able to capture it. I'm not too surprised though, no other
> driver plays with is_c45.
O.K. Please find the below kernel trace,
[15890.127525] ------------[ cut here ]------------
[15890.127540] phy_start_aneg+0x0/0x58: returned: -22
[15890.127592] WARNING: CPU: 0 PID: 3937 at drivers/net/phy/phy.c:1233
phy_state_machine+0xac/0x2f0
[15890.127602] Modules linked in: lan865x_t1s(O) microchip_t1s(O) rfcomm
snd_seq_dummy snd_hrtimer snd_seq snd_seq_device cmac algif_hash
aes_arm64 aes_generic algif_skcipher af_alg bnep brcmfmac_wcc hci_uart
btbcm bluetooth brcmfmac binfmt_misc bcm2835_v4l2(C) rpivid_hevc(C)
bcm2835_codec(C) bcm2835_isp(C) v4l2_mem2mem brcmutil
bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig cfg80211
ecdh_generic ecc videobuf2_memops videobuf2_v4l2 rfkill videodev libaes
raspberrypi_hwmon videobuf2_common snd_bcm2835(C) raspberrypi_gpiomem mc
vc_sm_cma(C) nvmem_rmem gpio_fan uio_pdrv_genirq uio i2c_dev fuse dm_mod
ip_tables x_tables ipv6 spidev vc4 snd_soc_hdmi_codec drm_display_helper
cec drm_dma_helper v3d gpu_sched drm_kms_helper drm_shmem_helper drm
drm_panel_orientation_quirks i2c_brcmstb snd_soc_core spi_bcm2835
snd_compress snd_pcm_dmaengine snd_pcm snd_timer snd backlight [last
unloaded: microchip_t1s(O)]
[15890.127756] CPU: 0 PID: 3937 Comm: kworker/0:1 Tainted: G WC O
6.6.20+rpt-rpi-v8 #1 Debian 1:6.6.20-1+rpt1
[15890.127763] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[15890.127767] Workqueue: events_power_efficient phy_state_machine
[15890.127773] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[15890.127778] pc : phy_state_machine+0xac/0x2f0
[15890.127783] lr : phy_state_machine+0xac/0x2f0
[15890.127787] sp : ffffffc086c2bd60
[15890.127790] x29: ffffffc086c2bd60 x28: 0000000000000000 x27:
0000000000000000
[15890.127798] x26: ffffff81fef781a8 x25: ffffff8101906640 x24:
ffffff810010ca05
[15890.127806] x23: 00000000ffffffea x22: ffffff8103e41c98 x21:
0000000000000004
[15890.127815] x20: ffffff8103e41cf0 x19: ffffff8103e41800 x18:
00000000fffffffe
[15890.127822] x17: 0000000000000000 x16: ffffffe7346b5b60 x15:
ffffffc086c2b960
[15890.127830] x14: 0000000000000000 x13: 32322d203a64656e x12:
7275746572203a38
[15890.127837] x11: 3578302f3078302b x10: ffffffe735ca3708 x9 :
ffffffe73470946c
[15890.127845] x8 : 00000000ffffefff x7 : ffffffe735ca3708 x6 :
80000000fffff000
[15890.127852] x5 : ffffff81fef67d48 x4 : 0000000000000000 x3 :
0000000000000027
[15890.127860] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
ffffff8105f1bd80
[15890.127868] Call trace:
[15890.127872] phy_state_machine+0xac/0x2f0
[15890.127878] process_one_work+0x148/0x3b8
[15890.127887] worker_thread+0x32c/0x450
[15890.127893] kthread+0x11c/0x128
[15890.127902] ret_from_fork+0x10/0x20
[15890.127908] ---[ end trace 0000000000000000 ]---
Best regards,
Parthiban V
>
> Andrew
>