Re: [PATCH v4] net: Add MOXA ART SoCs ethernet driver

From: Florian Fainelli
Date: Fri Aug 02 2013 - 06:24:33 EST

Next message: Greg Kroah-Hartman: "[ 18/23] USB: serial: ftdi_sio: add more RT Systems ftdi devices"
Previous message: Greg Kroah-Hartman: "[ 09/23] USB: misc: Add Manhattan Hi-Speed USB DVI Converter to sisusbvga"
In reply to: Mark Rutland: "Re: [PATCH v4] net: Add MOXA ART SoCs ethernet driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

2013/8/2 Mark Rutland <mark.rutland@xxxxxxx>:
> On Thu, Aug 01, 2013 at 10:39:28AM +0100, Jonas Jensen wrote:
>> The MOXA UC-711X hardware(s) has an ethernet controller that seem to be
>> developed internally. The IC used is "RTL8201CP".
>>
>> Since there is no public documentation, this driver is mostly the one
>> published by MOXA that has been heavily cleaned up / ported from linux 2.6.9.
>>
>> Signed-off-by: Jonas Jensen <jonas.jensen@xxxxxxxxx>
>> ---
>
> [...]
>
>> diff --git a/Documentation/devicetree/bindings/net/moxa,moxart-mac.txt b/Documentation/devicetree/bindings/net/moxa,moxart-mac.txt
>> new file mode 100644
>> index 0000000..1fc44ff
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/moxa,moxart-mac.txt
>> @@ -0,0 +1,25 @@
>> +MOXA ART Ethernet Controller
>> +
>> +Required properties:
>> +
>> +- compatible : Must be "moxa,moxart-mac"
>> +- reg : Should contain registers location and length
>> + index 0 : main register
>
> I assume there's more than one register in the main register bank?
>
> Are there any other register banks not used by this driver?
>
>> + index 1 : mac address (stored on flash)
>
> Is that flash a part of the MAC unit?
>
> Is it only 6 bytes long (judging by the examples), or is the th only
> portion used by the driver?
>
> If it's bigger, I'd prefer to describe the whole of flash. This driver
> will know the offset within it, and can choose to map only the protion
> it wants to use. Another driver may choose to do more interesting
> things. We need to describe the hardware, not how we expect it to be
> used...
>
> If it's not associated with the MAC, I'm not sure this is the best way
> of describing the linkage...

I also do not quite like it, this is very unusual. Here is how you
could potentially solve this:

- generate a random ethernet MAC adddress and later on let user-space
fetch the MAC address from the flash and set it correctly
- have some early code in arch/arm/mach-moxart which fetches the MAC
address from the flash and then update the ethernet node
local-mac-address property accordingly

Ideally, fetching the MAC address from whatever backend storage is
something that should really be left to user-space...

>
>> +- interrupts : Should contain the mac interrupt number
>
> Is there no need to link this to a phy, or is it a fixed-link device?
>
>> +
>> +Example:
>> +
>> + mac0: mac@90900000 {
>> + compatible = "moxa,moxart-mac";
>> + reg = <0x90900000 0x100>,
>> + <0x80000050 0x6>;
>> + interrupts = <25 0>;
>> + };
>> +
>> + mac1: mac@92000000 {
>> + compatible = "moxa,moxart-mac";
>> + reg = <0x92000000 0x100>,
>> + <0x80000056 0x6>;
>> + interrupts = <27 0>;
>> + };
>
> [...]
>
>> diff --git a/drivers/net/ethernet/moxa/moxart_ether.h b/drivers/net/ethernet/moxa/moxart_ether.h
>> new file mode 100644
>> index 0000000..790b6a9
>> --- /dev/null
>> +++ b/drivers/net/ethernet/moxa/moxart_ether.h
>> @@ -0,0 +1,513 @@
>> +/* MOXA ART Ethernet (RTL8201CP) driver.
>> + *
>> + * Copyright (C) 2013 Jonas Jensen
>> + *
>> + * Jonas Jensen <jonas.jensen@xxxxxxxxx>
>> + *
>> + * Based on code from
>> + * Moxa Technology Co., Ltd. <www.moxa.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#ifndef _MOXART_ETHERNET_H
>> +#define _MOXART_ETHERNET_H
>> +
>> +#define TX_DESC_NUM 64
>> +#define TX_DESC_NUM_MASK (TX_DESC_NUM-1)
>> +#define TX_NEXT(N) (((N) + 1) & (TX_DESC_NUM_MASK))
>> +#define TX_BUF_SIZE 1600
>> +
>> +#define RX_DESC_NUM 64
>> +#define RX_DESC_NUM_MASK (RX_DESC_NUM-1)
>> +#define RX_NEXT(N) (((N) + 1) & (RX_DESC_NUM_MASK))
>> +#define RX_BUF_SIZE 1600
>> +
>> +struct tx_desc_t {
>> + union {
>> +#define TXDMA_OWN BIT(31)
>> +#define TXPKT_EXSCOL BIT(1)
>> +#define TXPKT_LATECOL BIT(0)
>> + unsigned int ui;
>> +
>> + struct {
>> + /* is aborted due to late collision */
>> + unsigned int tx_pkt_late_col:1;
>> +
>> + /* is aborted after 16 collisions */
>> + unsigned int rx_pkt_exs_col:1;
>> +
>> + unsigned int reserved1:29;
>> +
>> + /* is owned by the MAC controller */
>> + unsigned int tx_dma_own:1;
>> + } ubit;
>> + } txdes0;
>
> Unless I've misunderstood the later code, this is the format of data
> shared with the device? Bitfields aren't portable, and you can't rely on
> the padding here being what you expect...
>
> Similarly, the other structures below used in this way below seem scary
> to me.
>
>> +
>> + union {
>> +#define EDOTR BIT(31)
>> +#define TXIC BIT(30)
>> +#define TX2FIC BIT(29)
>> +#define FTS BIT(28)
>> +#define LTS BIT(27)
>> +#define TXBUF_SIZE_MASK 0x7ff
>> +#define TXBUF_SIZE_MAX (TXBUF_SIZE_MASK+1)
>> + unsigned int ui;
>> +
>> + struct {
>> + /* transmit buffer size in byte */
>> + unsigned int tx_buf_size:11;
>> +
>> + unsigned int reserved2:16;
>> +
>> + /* is the last descriptor of a Tx packet */
>> + unsigned int lts:1;
>> +
>> + /* is the first descriptor of a Tx packet */
>> + unsigned int fts:1;
>> +
>> + /* transmit to FIFO interrupt on completion */
>> + unsigned int tx2_fic:1;
>> +
>> + /* transmit interrupt on completion */
>> + unsigned int tx_ic:1;
>> +
>> + /* end descriptor of transmit ring */
>> + unsigned int edotr:1;
>> + } ubit;
>> + } txdes1;
>> +
>> + struct {
>> + /* transmit buffer physical base address */
>> + unsigned int addr_phys;
>> +
>> + /* transmit buffer virtual base address */
>> + unsigned char *addr_virt;
>> + } txdes2;
>> +};
>> +
>> +struct rx_desc_t {
>> + union {
>> +#define RXDMA_OWN BIT(31)
>> +#define FRS BIT(29)
>> +#define LRS BIT(28)
>> +#define RX_ODD_NB BIT(22)
>> +#define RUNT BIT(21)
>> +#define FTL BIT(20)
>> +#define CRC_ERR BIT(19)
>> +#define RX_ERR BIT(18)
>> +#define BROADCAST_RXDES0 BIT(17)
>> +#define MULTICAST_RXDES0 BIT(16)
>> +#define RFL_MASK 0x7ff
>> +#define RFL_MAX (RFL_MASK+1)
>> + unsigned int ui;
>> +
>> + struct {
>> + /* receive frame length */
>> + unsigned int recv_frame_len:11;
>> + unsigned int reserved1:5;
>> +
>> + /* multicast frame */
>> + unsigned int multicast:1;
>> +
>> + /* broadcast frame */
>> + unsigned int broadcast:1;
>> + unsigned int rx_err:1; /* receive error */
>> + unsigned int crc_err:1; /* CRC error */
>> + unsigned int ftl:1; /* frame too long */
>> +
>> + /* runt packet, less than 64 bytes */
>> + unsigned int runt:1;
>> +
>> + /* receive odd nibbles */
>> + unsigned int rx_odd_nb:1;
>> + unsigned int reserved2:5;
>> +
>> + /* last receive segment descriptor */
>> + unsigned int lrs:1;
>> +
>> + /* first receive segment descriptor */
>> + unsigned int frs:1;
>> +
>> + unsigned int reserved3:1;
>> + unsigned int rx_dma_own:1; /* RXDMA onwership */
>> + } ubit;
>> + } rxdes0;
>> +
>> + union {
>> +#define EDORR BIT(31)
>> +#define RXBUF_SIZE_MASK 0x7ff
>> +#define RXBUF_SIZE_MAX (RXBUF_SIZE_MASK+1)
>> + unsigned int ui;
>> +
>> + struct {
>> + /* receive buffer size */
>> + unsigned int rx_buf_size:11;
>> +
>> + unsigned int reserved4:20;
>> +
>> + /* end descriptor of receive ring */
>> + unsigned int edorr:1;
>> + } ubit;
>> + } rxdes1;
>> +
>> + struct {
>> + /* receive buffer physical base address */
>> + unsigned int addr_phys;
>> +
>> + /* receive buffer virtual base address */
>> + unsigned char *addr_virt;
>> + } rxdes2;
>> +};
>> +
>> +struct mac_control_reg_t {
>> +
>> +/* RXDMA has received packets into RX buffer successfully */
>> +#define RPKT_FINISH BIT(0)
>> +/* receive buffer unavailable */
>> +#define NORXBUF BIT(1)
>> +/* TXDMA has moved data into the TX FIFO */
>> +#define XPKT_FINISH BIT(2)
>> +/* transmit buffer unavailable */
>> +#define NOTXBUF BIT(3)
>> +/* packets transmitted to ethernet successfully */
>> +#define XPKT_OK_INT_STS BIT(4)
>> +/* packets transmitted to ethernet lost due to late
>> + * collision or excessive collision
>> + */
>> +#define XPKT_LOST_INT_STS BIT(5)
>> +/* packets received into RX FIFO successfully */
>> +#define RPKT_SAV BIT(6)
>> +/* received packet lost due to RX FIFO full */
>> +#define RPKT_LOST_INT_STS BIT(7)
>> +#define AHB_ERR BIT(8) /* AHB error */
>> +#define PHYSTS_CHG BIT(9) /* PHY link status change */
>> + unsigned int isr; /* interrupt status, 0x0 */
>> +
>> +#define RPKT_FINISH_M BIT(0) /* interrupt mask of ISR[0] */
>> +#define NORXBUF_M BIT(1) /* interrupt mask of ISR[1] */
>> +#define XPKT_FINISH_M BIT(2) /* interrupt mask of ISR[2] */
>> +#define NOTXBUF_M BIT(3) /* interrupt mask of ISR[3] */
>> +#define XPKT_OK_M BIT(4) /* interrupt mask of ISR[4] */
>> +#define XPKT_LOST_M BIT(5) /* interrupt mask of ISR[5] */
>> +#define RPKT_SAV_M BIT(6) /* interrupt mask of ISR[6] */
>> +#define RPKT_LOST_M BIT(7) /* interrupt mask of ISR[7] */
>> +#define AHB_ERR_M BIT(8) /* interrupt mask of ISR[8] */
>> +#define PHYSTS_CHG_M BIT(9) /* interrupt mask of ISR[9] */
>> + unsigned int imr; /* interrupt mask, 0x4 */
>> +
>> +/* the most significant 2 bytes of MAC address */
>> +#define MAC_MADR_MASK 0xffff
>> + /* MAC most significant address, 0x8 */
>> + unsigned int mac_madr;
>> +
>> + /* MAC least significant address, 0xc */
>> + unsigned int mac_ldar;
>> +
>> + /* multicast address hash table 0, 0x10 */
>> + unsigned int matht0;
>> +
>> + /* multicast address hash table 1, 0x14 */
>> + unsigned int matht1;
>> +
>> + /* transmit poll demand, 0x18 */
>> + unsigned int txpd;
>> +
>> + /* receive poll demand, 0x1c */
>> + unsigned int rxpd;
>> +
>> + /* transmit ring base address, 0x20 */
>> + unsigned int txr_badr;
>> +
>> + /* receive ring base address, 0x24 */
>> + unsigned int rxr_badr;
>> +
>> +/* defines the period of TX cycle time */
>> +#define TXINT_TIME_SEL BIT(15)
>> +#define TXINT_THR_MASK 0x7000
>> +#define TXINT_CNT_MASK 0xf00
>> +/* defines the period of RX cycle time */
>> +#define RXINT_TIME_SEL BIT(7)
>> +#define RXINT_THR_MASK 0x70
>> +#define RXINT_CNT_MASK 0xF
>> + /* interrupt timer control, 0x28 */
>> + unsigned int itc;
>> +
>> +/* defines the period of TX poll time */
>> +#define TXPOLL_TIME_SEL BIT(12)
>> +#define TXPOLL_CNT_MASK 0xf00
>> +#define TXPOLL_CNT_SHIFT_BIT 8
>> +/* defines the period of RX poll time */
>> +#define RXPOLL_TIME_SEL BIT(4)
>> +#define RXPOLL_CNT_MASK 0xF
>> +#define RXPOLL_CNT_SHIFT_BIT 0
>> + /* automatic polling timer control, 0x2c */
>> + unsigned int aptc;
>> +
>> +/* enable RX FIFO threshold arbitration */
>> +#define RX_THR_EN BIT(9)
>> +#define RXFIFO_HTHR_MASK 0x1c0
>> +#define RXFIFO_LTHR_MASK 0x38
>> +/* use INCR16 burst command in AHB bus */
>> +#define INCR16_EN BIT(2)
>> +/* use INCR8 burst command in AHB bus */
>> +#define INCR8_EN BIT(1)
>> +/* use INCR4 burst command in AHB bus */
>> +#define INCR4_EN BIT(0)
>> + /* DMA burst length and arbitration control, 0x30 */
>> + unsigned int dblac;
>> +
>> + unsigned int reserved1[21]; /* 0x34 - 0x84 */
>> +
>> +#define RX_BROADPKT BIT(17) /* receive boradcast packet */
>> +/* receive all multicast packet */
>> +#define RX_MULTIPKT BIT(16)
>> +#define FULLDUP BIT(15) /* full duplex */
>> +/* append CRC to transmitted packet */
>> +#define CRC_APD BIT(14)
>> +/* do not check incoming packet's destination address */
>> +#define RCV_ALL BIT(12)
>> +/* store incoming packet even if its length is great than 1518 bytes */
>> +#define RX_FTL BIT(11)
>> +/* store incoming packet even if its length is less than 64 bytes */
>> +#define RX_RUNT BIT(10)
>> +/* enable storing incoming packet if the packet passes hash table
>> + * address filtering and is a multicast packet
>> + */
>> +#define HT_MULTI_EN BIT(9)
>> +#define RCV_EN BIT(8) /* receiver enable */
>> +/* enable packet reception when transmitting packet in half duplex mode */
>> +#define ENRX_IN_HALFTX BIT(6)
>> +#define XMT_EN BIT(5) /* transmitter enable */
>> +/* disable CRC check when receiving packets */
>> +#define CRC_DIS BIT(4)
>> +#define LOOP_EN BIT(3) /* internal loop-back */
>> +/* software reset, last 64 AHB bus clocks */
>> +#define SW_RST BIT(2)
>> +#define RDMA_EN BIT(1) /* enable receive DMA chan */
>> +#define XDMA_EN BIT(0) /* enable transmit DMA chan */
>> + unsigned int maccr; /* MAC control, 0x88 */
>> +
>> +#define COL_EXCEED BIT(11) /* collisions exceeds 16 */
>> +/* transmitter detects late collision */
>> +#define LATE_COL BIT(10)
>> +/* packet transmitted to ethernet lost due to late collision
>> + * or excessive collision
>> + */
>> +#define XPKT_LOST BIT(9)
>> +/* packets transmitted to ethernet successfully */
>> +#define XPKT_OK BIT(8)
>> +/* receiver detects a runt packet */
>> +#define RUNT_MAC_STS BIT(7)
>> +/* receiver detects a frame that is too long */
>> +#define FTL_MAC_STS BIT(6)
>> +#define CRC_ERR_MAC_STS BIT(5)
>> +/* received packets list due to RX FIFO full */
>> +#define RPKT_LOST BIT(4)
>> +/* packets received into RX FIFO successfully */
>> +#define RPKT_SAVE BIT(3)
>> +/* incoming packet dropped due to collision */
>> +#define COL BIT(2)
>> +#define MCPU_BROADCAST BIT(1)
>> +#define MCPU_MULTICAST BIT(0)
>> + unsigned int macsr; /* MAC status, 0x8c */
>> +
>> +/* initialize a write sequence to PHY by setting this bit to 1
>> + * this bit would be auto cleared after the write operation is finished.
>> + */
>> +#define MIIWR BIT(27)
>> +#define MIIRD BIT(26)
>> +#define REGAD_MASK 0x3e00000
>> +#define PHYAD_MASK 0x1f0000
>> +#define MIIRDATA_MASK 0xffff
>> + unsigned int phycr; /* PHY control, 0x90 */
>> +
>> +#define MIIWDATA_MASK 0xffff
>> + unsigned int phywdata; /* PHY write data, 0x94 */
>> +
>> +#define PAUSE_TIME_MASK 0xffff0000
>> +#define FC_HIGH_MASK 0xf000
>> +#define FC_LOW_MASK 0xf00
>> +#define RX_PAUSE BIT(4) /* receive pause frame */
>> +/* packet transmission is paused due to receive */
>> +#define TXPAUSED BIT(3)
>> + /* pause frame */
>> +/* enable flow control threshold mode. */
>> +#define FCTHR_EN BIT(2)
>> +#define TX_PAUSE BIT(1) /* transmit pause frame */
>> +#define FC_EN BIT(0) /* flow control mode enable */
>> + unsigned int fcr; /* flow control, 0x98 */
>> +
>> +#define BK_LOW_MASK 0xf00
>> +#define BKJAM_LEN_MASK 0xf0
>> +#define BK_MODE BIT(1) /* back pressure address mode */
>> +#define BK_EN BIT(0) /* back pressure mode enable */
>> + unsigned int bpr; /* back pressure, 0x9c */
>> +
>> + unsigned int reserved2[9]; /* 0xa0 - 0xc0 */
>> +
>> +#define TEST_SEED_MASK 0x3fff
>> + unsigned int ts; /* test seed, 0xc4 */
>> +
>> +#define TXD_REQ BIT(31) /* TXDMA request */
>> +#define RXD_REQ BIT(30) /* RXDMA request */
>> +#define DARB_TXGNT BIT(29) /* TXDMA grant */
>> +#define DARB_RXGNT BIT(28) /* RXDMA grant */
>> +#define TXFIFO_EMPTY BIT(27) /* TX FIFO is empty */
>> +#define RXFIFO_EMPTY BIT(26) /* RX FIFO is empty */
>> +#define TXDMA2_SM_MASK 0x7000
>> +#define TXDMA1_SM_MASK 0xf00
>> +#define RXDMA2_SM_MASK 0x70
>> +#define RXDMA1_SM_MASK 0xF
>> + unsigned int dmafifos; /* DMA/FIFO state, 0xc8 */
>> +
>> +#define SINGLE_PKT BIT(26) /* single packet mode */
>> +/* automatic polling timer test mode */
>> +#define PTIMER_TEST BIT(25)
>> +#define ITIMER_TEST BIT(24) /* interrupt timer test mode */
>> +#define TEST_SEED_SEL BIT(22) /* test seed select */
>> +#define SEED_SEL BIT(21) /* seed select */
>> +#define TEST_MODE BIT(20) /* transmission test mode */
>> +#define TEST_TIME_MASK 0xffc00
>> +#define TEST_EXCEL_MASK 0x3e0
>> + unsigned int tm; /* test mode, 0xcc */
>> +
>> + unsigned int reserved3; /* 0xd0 */
>> +
>> +#define TX_MCOL_MASK 0xffff0000
>> +#define TX_MCOL_SHIFT_BIT 16
>> +#define TX_SCOL_MASK 0xffff
>> +#define TX_SCOL_SHIFT_BIT 0
>> + /* TX_MCOL and TX_SCOL counter, 0xd4 */
>> + unsigned int txmcol_xscol;
>> +
>> +#define RPF_MASK 0xffff0000
>> +#define RPF_SHIFT_BIT 16
>> +#define AEP_MASK 0xffff
>> +#define AEP_SHIFT_BIT 0
>> + unsigned int rpf_aep; /* RPF and AEP counter, 0xd8 */
>> +
>> +#define XM_MASK 0xffff0000
>> +#define XM_SHIFT_BIT 16
>> +#define PG_MASK 0xffff
>> +#define PG_SHIFT_BIT 0
>> + unsigned int xm_pg; /* XM and PG counter, 0xdc */
>> +
>> +#define RUNT_CNT_MASK 0xffff0000
>> +#define RUNT_CNT_SHIFT_BIT 16
>> +#define TLCC_MASK 0xffff
>> +#define TLCC_SHIFT_BIT 0
>> + /* RUNT_CNT and TLCC counter, 0xe0 */
>> + unsigned int runtcnt_tlcc;
>> +
>> +#define CRCER_CNT_MASK 0xffff0000
>> +#define CRCER_CNT_SHIFT_BIT 16
>> +#define FTL_CNT_MASK 0xffff
>> +#define FTL_CNT_SHIFT_BIT 0
>> + /* CRCER_CNT and FTL_CNT counter, 0xe4 */
>> + unsigned int crcercnt_ftlcnt;
>> +
>> +#define RLC_MASK 0xffff0000
>> +#define RLC_SHIFT_BIT 16
>> +#define RCC_MASK 0xffff
>> +#define RCC_SHIFT_BIT 0
>> + unsigned int rlc_rcc; /* RLC and RCC counter, 0xe8 */
>> +
>> + unsigned int broc; /* BROC counter, 0xec */
>> + unsigned int mulca; /* MULCA counter, 0xf0 */
>> + unsigned int rp; /* RP counter, 0xf4 */
>> + unsigned int xp; /* XP counter, 0xf8 */
>> +};
>
> I don't think you can rely on the fields to be the size you expect (int
> is not necessarily 32 bits), and nor can you expect them to have the
> padding you expect (a 64 bit arch could pad ints to 64 bits). I think it
> would be better to just #define the offsets explicitly, which will be
> safe and easy to verify.
>
> Thanks,
> Mark.

--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Greg Kroah-Hartman: "[ 18/23] USB: serial: ftdi_sio: add more RT Systems ftdi devices"
Previous message: Greg Kroah-Hartman: "[ 09/23] USB: misc: Add Manhattan Hi-Speed USB DVI Converter to sisusbvga"
In reply to: Mark Rutland: "Re: [PATCH v4] net: Add MOXA ART SoCs ethernet driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]