Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755240Ab3HBKY3 (ORCPT ); Fri, 2 Aug 2013 06:24:29 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:41767 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754886Ab3HBKYZ (ORCPT ); Fri, 2 Aug 2013 06:24:25 -0400 MIME-Version: 1.0 In-Reply-To: <20130802100406.GC2884@e106331-lin.cambridge.arm.com> References: <1375280401-5564-1-git-send-email-jonas.jensen@gmail.com> <1375349968-16059-1-git-send-email-jonas.jensen@gmail.com> <20130802100406.GC2884@e106331-lin.cambridge.arm.com> From: Florian Fainelli Date: Fri, 2 Aug 2013 11:23:45 +0100 Message-ID: Subject: Re: [PATCH v4] net: Add MOXA ART SoCs ethernet driver To: Mark Rutland Cc: Jonas Jensen , "netdev@vger.kernel.org" , "davem@davemloft.net" , "linux-doc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "arm@kernel.org" , "joe@perches.com" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 21925 Lines: 538 2013/8/2 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 >> --- > > [...] > >> 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 >> + * >> + * Based on code from >> + * Moxa Technology Co., Ltd. >> + * >> + * 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/