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

[Mark Rutland]

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...

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