lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOZdJXXQKMrOJpm3XJmX5ZULEoOZqS12u5FWKdcxfUm+LnGbww@mail.gmail.com>
Date:	Wed, 9 Dec 2015 14:09:27 -0600
From:	Timur Tabi <timur@...eaurora.org>
To:	Gilad Avidov <gavidov@...eaurora.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	netdev@...r.kernel.org, sdharia@...eaurora.org,
	linux-arm-msm@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
	vikrams@...eaurora.org,
	Shanker Donthineni <shankerd@...eaurora.org>
Subject: Re: [PATCH] net: emac: emac gigabit ethernet controller driver

So first of all, thanks for posting this.  I know it's missing a bunch
of stuff that's necessary for Qualcomm's Server chip, but it's a
start.

Unfortunately, 6,000 lines is a lot to review at once.  Any chance you
can break up the next version into smaller patches?

On Mon, Dec 7, 2015 at 4:58 PM, Gilad Avidov <gavidov@...eaurora.org> wrote:

> +               qcom,emac-gpio-mdc = <&msmgpio 123 0>;
> +               qcom,emac-gpio-mdio = <&msmgpio 124 0>;

Is there any chance you can remove all references to "MSM" throughout
the entire driver, since the EMAC exists on non-MSM parts?

> +               qcom,emac-tstamp-en;
> +               qcom,emac-ptp-frac-ns-adj = <125000000 1>;
> +               phy-addr = <0>;
> +       };
> diff --git a/drivers/net/ethernet/qualcomm/Kconfig b/drivers/net/ethernet/qualcomm/Kconfig
> index a76e380..ae9442d 100644
> --- a/drivers/net/ethernet/qualcomm/Kconfig
> +++ b/drivers/net/ethernet/qualcomm/Kconfig
> @@ -24,4 +24,11 @@ config QCA7000
>           To compile this driver as a module, choose M here. The module
>           will be called qcaspi.
>
> +config QCOM_EMAC
> +       tristate "MSM EMAC Gigabit Ethernet support"
> +       default n

"default n" is redundant

> +       select CRC32
> +       ---help---
> +         This driver supports the Qualcomm EMAC Gigabit Ethernet controller.

I think should be longer, perhaps by adding some more info about the
controller itself?

> +
>  endif # NET_VENDOR_QUALCOMM
> diff --git a/drivers/net/ethernet/qualcomm/Makefile b/drivers/net/ethernet/qualcomm/Makefile
> index 9da2d75..b14686e 100644
> --- a/drivers/net/ethernet/qualcomm/Makefile
> +++ b/drivers/net/ethernet/qualcomm/Makefile
> @@ -4,3 +4,5 @@
>
>  obj-$(CONFIG_QCA7000) += qcaspi.o
>  qcaspi-objs := qca_spi.o qca_framing.o qca_7k.o qca_debug.o
> +
> +obj-$(CONFIG_QCOM_EMAC) += emac/
> \ No newline at end of file

Please fix

> +/* RSS */
> +static void emac_mac_rss_config(struct emac_adapter *adpt)
> +{
> +       int key_len_by_u32 = sizeof(adpt->rss_key) / sizeof(u32);
> +       int idt_len_by_u32 = sizeof(adpt->rss_idt) / sizeof(u32);

Can you use ARRAY_SIZE here?

> +       u32 rxq0;
> +       int i;
> +
> +       /* Fill out hash function keys */
> +       for (i = 0; i < key_len_by_u32; i++) {
> +               u32 key, idx_base;
> +
> +               idx_base = (key_len_by_u32 - i) * 4;
> +               key = ((adpt->rss_key[idx_base - 1])       |
> +                      (adpt->rss_key[idx_base - 2] << 8)  |
> +                      (adpt->rss_key[idx_base - 3] << 16) |
> +                      (adpt->rss_key[idx_base - 4] << 24));
> +               writel_relaxed(key, adpt->base + EMAC_RSS_KEY(i, u32));
> +       }
> +
> +       /* Fill out redirection table */
> +       for (i = 0; i < idt_len_by_u32; i++)
> +               writel_relaxed(adpt->rss_idt[i],
> +                              adpt->base + EMAC_RSS_TBL(i, u32));
> +
> +       writel_relaxed(adpt->rss_base_cpu, adpt->base + EMAC_BASE_CPU_NUMBER);
> +
> +       rxq0 = readl_relaxed(adpt->base + EMAC_RXQ_CTRL_0);
> +       if (adpt->rss_hstype & EMAC_RSS_HSTYP_IPV4_EN)
> +               rxq0 |= RXQ0_RSS_HSTYP_IPV4_EN;
> +       else
> +               rxq0 &= ~RXQ0_RSS_HSTYP_IPV4_EN;
> +
> +       if (adpt->rss_hstype & EMAC_RSS_HSTYP_TCP4_EN)
> +               rxq0 |= RXQ0_RSS_HSTYP_IPV4_TCP_EN;
> +       else
> +               rxq0 &= ~RXQ0_RSS_HSTYP_IPV4_TCP_EN;
> +
> +       if (adpt->rss_hstype & EMAC_RSS_HSTYP_IPV6_EN)
> +               rxq0 |= RXQ0_RSS_HSTYP_IPV6_EN;
> +       else
> +               rxq0 &= ~RXQ0_RSS_HSTYP_IPV6_EN;
> +
> +       if (adpt->rss_hstype & EMAC_RSS_HSTYP_TCP6_EN)
> +               rxq0 |= RXQ0_RSS_HSTYP_IPV6_TCP_EN;
> +       else
> +               rxq0 &= ~RXQ0_RSS_HSTYP_IPV6_TCP_EN;
> +
> +       rxq0 |= ((adpt->rss_idt_size << IDT_TABLE_SIZE_SHFT) &
> +               IDT_TABLE_SIZE_BMSK);
> +       rxq0 |= RSS_HASH_EN;
> +
> +       wmb(); /* ensure all parameters are written before enabling RSS */
> +
> +       writel_relaxed(rxq0, adpt->base + EMAC_RXQ_CTRL_0);

Why not just use writel(), which already includes a wmb()

> +/* Power Management */
> +void emac_mac_pm(struct emac_adapter *adpt, u32 speed, bool wol_en, bool rx_en)
> +{
> +       u32 dma_mas, mac;
> +
> +       dma_mas = readl_relaxed(adpt->base + EMAC_DMA_MAS_CTRL);
> +       dma_mas &= ~LPW_CLK_SEL;
> +       dma_mas |= LPW_STATE;
> +
> +       mac = readl_relaxed(adpt->base + EMAC_MAC_CTRL);
> +       mac &= ~(FULLD | RXEN | TXEN);
> +       mac = (mac & ~SPEED_BMSK) |
> +         (((u32)emac_mac_speed_10_100 << SPEED_SHFT) & SPEED_BMSK);
> +
> +       if (wol_en) {
> +               if (rx_en)
> +                       mac |= (RXEN | BROAD_EN);

You don't need the parentheses.

> +/* Config descriptor rings */
> +static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
> +{
> +       if (adpt->timestamp_en)
> +               emac_reg_update32(adpt->csr + EMAC_EMAC_WRAPPER_CSR1,
> +                                 0, ENABLE_RRD_TIMESTAMP);
> +
> +       /* TPD */

What does TPD stand for?

> +       writel_relaxed(EMAC_DMA_ADDR_HI(adpt->tx_q[0].tpd.p_addr),
> +                      adpt->base + EMAC_DESC_CTRL_1);
> +       switch (adpt->tx_q_cnt) {
> +       case 4:
> +               writel_relaxed(EMAC_DMA_ADDR_LO(adpt->tx_q[3].tpd.p_addr),
> +                              adpt->base + EMAC_H3TPD_BASE_ADDR_LO);
> +               /* fall through */
> +       case 3:
> +               writel_relaxed(EMAC_DMA_ADDR_LO(adpt->tx_q[2].tpd.p_addr),
> +                              adpt->base + EMAC_H2TPD_BASE_ADDR_LO);
> +               /* fall through */
> +       case 2:
> +               writel_relaxed(EMAC_DMA_ADDR_LO(adpt->tx_q[1].tpd.p_addr),
> +                              adpt->base + EMAC_H1TPD_BASE_ADDR_LO);
> +               /* fall through */
> +       case 1:
> +               writel_relaxed(EMAC_DMA_ADDR_LO(adpt->tx_q[0].tpd.p_addr),
> +                              adpt->base + EMAC_DESC_CTRL_8);
> +               break;
> +       default:
> +               netdev_err(adpt->netdev,
> +                          "error: invalid number of TX queues (%d)\n",
> +                          adpt->tx_q_cnt);
> +               return;
> +       }

This is not time-critical code.  Why not just create a for-loop?

> +/* Config transmit parameters */
> +static void emac_mac_tx_config(struct emac_adapter *adpt)
> +{
> +       u16 tx_offload_thresh = EMAC_MAX_TX_OFFLOAD_THRESH;
> +       u32 val;
> +
> +       writel_relaxed((tx_offload_thresh >> 3) &

Why is tx_offload_thresh a u16 if you're going to use writel anyway?
Make it a u32.

> +void emac_mac_reset(struct emac_adapter *adpt)
> +{
> +       writel_relaxed(0, adpt->base + EMAC_INT_MASK);
> +       writel_relaxed(DIS_INT, adpt->base + EMAC_INT_STATUS);
> +
> +       emac_mac_stop(adpt);
> +
> +       emac_reg_update32(adpt->base + EMAC_DMA_MAS_CTRL, 0, SOFT_RST);
> +       wmb(); /* ensure mac is fully reset */
> +       usleep_range(100, 150);

Please add a comment explaiing why this delay is necessary.

> +void emac_mac_stop(struct emac_adapter *adpt)
> +{
> +       emac_reg_update32(adpt->base + EMAC_RXQ_CTRL_0, RXQ_EN, 0);
> +       emac_reg_update32(adpt->base + EMAC_TXQ_CTRL_0, TXQ_EN, 0);
> +       emac_reg_update32(adpt->base + EMAC_MAC_CTRL, (TXEN | RXEN), 0);
> +       wmb(); /* ensure mac is stopped before we proceed */
> +       usleep_range(1000, 1050);

Same here.

> +/* set MAC address */
> +void emac_mac_addr_clear(struct emac_adapter *adpt, u8 *addr)
> +{
> +       u32 sta;
> +
> +       /* for example: 00-A0-C6-11-22-33
> +        * 0<-->C6112233, 1<-->00A0.
> +        */

/*
 * Multi-line comments
 * look like this.
 */

> +/* Free all descriptors of given transmit queue */
> +static void emac_tx_q_descs_free(struct emac_adapter *adpt,
> +                                struct emac_tx_queue *tx_q)
> +{
> +       unsigned long size;
> +       u32 i;

Since 'i' is used as an index, it should be an unsized integer.  And
'size' should be a 'size_t'

> +static void emac_tx_q_descs_free_all(struct emac_adapter *adpt)
> +{
> +       u8 i;

'int'.  Personally, I'd prefer "unsigned int", but 'int' is what you
use elsewhere.

> +/* Free all descriptors of given receive queue */
> +static void emac_rx_q_free_descs(struct emac_adapter *adpt,
> +                                struct emac_rx_queue *rx_q)
> +{
> +       struct device *dev = adpt->netdev->dev.parent;
> +       unsigned long size;
> +       u32 i;

size_t size;
int i;

> +/* Allocate TX descriptor ring for the given transmit queue */
> +static int emac_tx_q_desc_alloc(struct emac_adapter *adpt,
> +                               struct emac_tx_queue *tx_q)
> +{
> +       struct emac_ring_header *ring_header = &adpt->ring_header;
> +       unsigned long size;

size_t

> +
> +       size = sizeof(struct emac_buffer) * tx_q->tpd.count;
> +       tx_q->tpd.tpbuff = kzalloc(size, GFP_KERNEL);
> +       if (!tx_q->tpd.tpbuff)
> +               return -ENOMEM;
> +
> +       tx_q->tpd.size = tx_q->tpd.count * (adpt->tpd_size * 4);
> +       tx_q->tpd.p_addr = ring_header->p_addr + ring_header->used;
> +       tx_q->tpd.v_addr = ring_header->v_addr + ring_header->used;
> +       ring_header->used += ALIGN(tx_q->tpd.size, 8);
> +       tx_q->tpd.produce_idx = 0;
> +       tx_q->tpd.consume_idx = 0;
> +       return 0;

blank line above "return".

> +}
> +
> +static int emac_tx_q_desc_alloc_all(struct emac_adapter *adpt)
> +{
> +       int retval = 0;
> +       u8 i;

int i;

> +static void emac_rx_q_free_bufs_all(struct emac_adapter *adpt)
> +{
> +       u8 i;

int i;

> +/* Allocate RX descriptor rings for the given receive queue */
> +static int emac_rx_descs_alloc(struct emac_adapter *adpt,
> +                              struct emac_rx_queue *rx_q)
> +{
> +       struct emac_ring_header *ring_header = &adpt->ring_header;
> +       unsigned long size;

size_t

> +static int emac_rx_descs_allocs_all(struct emac_adapter *adpt)
> +{
> +       int retval = 0;
> +       u8 i;

int i;

> +
> +       for (i = 0; i < adpt->rx_q_cnt; i++) {
> +               retval = emac_rx_descs_alloc(adpt, &adpt->rx_q[i]);
> +               if (retval)
> +                       break;
> +       }
> +
> +       if (retval) {
> +               netdev_err(adpt->netdev, "error: Rx Queue %u alloc failed\n",

%d

> +/* Produce new receive free descriptor */
> +static bool emac_mac_rx_rfd_create(struct emac_adapter *adpt,
> +                                  struct emac_rx_queue *rx_q,
> +                                  union emac_rfd *rfd)
> +{
> +       u32 *hw_rfd = EMAC_RFD(rx_q, adpt->rfd_size,
> +                              rx_q->rfd.produce_idx);
> +
> +       *(hw_rfd++) = rfd->word[0];
> +       *hw_rfd = rfd->word[1];
> +
> +       if (++rx_q->rfd.produce_idx == rx_q->rfd.count)
> +               rx_q->rfd.produce_idx = 0;
> +
> +       return true;

You never check the return value, so just make this a void function.

> +}
> +
> +/* Fill up receive queue's RFD with preallocated receive buffers */
> +static int emac_mac_rx_descs_refill(struct emac_adapter *adpt,
> +                                   struct emac_rx_queue *rx_q)
> +{
> +       struct emac_buffer *curr_rxbuf;
> +       struct emac_buffer *next_rxbuf;
> +       union emac_rfd rfd;
> +       struct sk_buff *skb;
> +       void *skb_data = NULL;
> +       u32 count = 0;

int count = 0;

The type should match the return value of the function.

> +       u32 next_produce_idx;
> +
> +       next_produce_idx = rx_q->rfd.produce_idx;
> +       if (++next_produce_idx == rx_q->rfd.count)
> +               next_produce_idx = 0;
> +       curr_rxbuf = GET_RFD_BUFFER(rx_q, rx_q->rfd.produce_idx);
> +       next_rxbuf = GET_RFD_BUFFER(rx_q, next_produce_idx);
> +
> +       /* this always has a blank rx_buffer*/
> +       while (!next_rxbuf->dma) {
> +               skb = dev_alloc_skb(adpt->rxbuf_size + NET_IP_ALIGN);
> +               if (unlikely(!skb))
> +                       break;

I don't think this is time-critical code, so don't use unlikely().

> +/* Consume next received packet descriptor */
> +static bool emac_rx_process_rrd(struct emac_adapter *adpt,
> +                               struct emac_rx_queue *rx_q,
> +                               union emac_rrd *rrd)
> +{
> +       u32 *hw_rrd = EMAC_RRD(rx_q, adpt->rrd_size,
> +                              rx_q->rrd.consume_idx);
> +
> +       /* If time stamping is enabled, it will be added in the beginning of
> +        * the hw rrd (hw_rrd). In sw rrd (rrd), 32bit words 4 & 5 are reserved
> +        * for the time stamp; hence the conversion.
> +        * Also, read the rrd word with update flag first; read rest of rrd
> +        * only if update flag is set.
> +        */
> +       if (adpt->timestamp_en)
> +               rrd->word[3] = *(hw_rrd + 5);
> +       else
> +               rrd->word[3] = *(hw_rrd + 3);
> +       rmb(); /* ensure hw receive returned descriptor timestamp is read */
> +
> +       if (!rrd->genr.update)
> +               return false;
> +
> +       if (adpt->timestamp_en) {
> +               rrd->word[4] = *(hw_rrd++);
> +               rrd->word[5] = *(hw_rrd++);
> +       } else {
> +               rrd->word[4] = 0;
> +               rrd->word[5] = 0;
> +       }
> +
> +       rrd->word[0] = *(hw_rrd++);
> +       rrd->word[1] = *(hw_rrd++);
> +       rrd->word[2] = *(hw_rrd++);
> +       mb(); /* ensure descriptor is read */

Why do you use mb() here, but rmb() above?  The comment is the same.

> +static void emac_rx_rfd_clean(struct emac_rx_queue *rx_q,
> +                             union emac_rrd *rrd)
> +{
> +       struct emac_buffer *rfbuf = rx_q->rfd.rfbuff;
> +       u32 consume_idx = rrd->genr.si;
> +       u16 i;

int i;

> +static inline bool emac_skb_cb_expired(struct sk_buff *skb)
> +{
> +       if (time_is_after_jiffies(EMAC_SKB_CB(skb)->jiffies +
> +                                 msecs_to_jiffies(100)))
> +               return false;
> +       return true;

return time_is_before_jiffies(EMAC_SKB_CB(skb)->jiffies +
msecs_to_jiffies(100));

> +/* Process receive event */
> +void emac_mac_rx_process(struct emac_adapter *adpt, struct emac_rx_queue *rx_q,
> +                        int *num_pkts, int max_pkts)
> +{
> +       struct net_device *netdev  = adpt->netdev;
> +
> +       union emac_rrd rrd;
> +       struct emac_buffer *rfbuf;
> +       struct sk_buff *skb;
> +
> +       u32 hw_consume_idx, num_consume_pkts;
> +       u32 count = 0;

unsigned int count;

> +       u32 proc_idx;
> +       u32 reg = readl_relaxed(adpt->base + rx_q->consume_reg);
> +
> +       hw_consume_idx = (reg & rx_q->consume_mask) >> rx_q->consume_shft;
> +       num_consume_pkts = (hw_consume_idx >= rx_q->rrd.consume_idx) ?
> +               (hw_consume_idx -  rx_q->rrd.consume_idx) :
> +               (hw_consume_idx + rx_q->rrd.count - rx_q->rrd.consume_idx);
> +
> +       while (1) {
> +               if (!num_consume_pkts)
> +                       break;
> +
> +               if (!emac_rx_process_rrd(adpt, rx_q, &rrd))
> +                       break;
> +
> +               if (likely(rrd.genr.nor == 1)) {
> +                       /* good receive */
> +                       rfbuf = GET_RFD_BUFFER(rx_q, rrd.genr.si);
> +                       dma_unmap_single(adpt->netdev->dev.parent, rfbuf->dma,
> +                                        rfbuf->length, DMA_FROM_DEVICE);
> +                       rfbuf->dma = 0;
> +                       skb = rfbuf->skb;
> +               } else {
> +                       netdev_err(adpt->netdev,
> +                                  "error: multi-RFD not support yet!\n");
> +                       break;
> +               }
> +               emac_rx_rfd_clean(rx_q, &rrd);
> +               num_consume_pkts--;
> +               count++;
> +
> +               /* Due to a HW issue in L4 check sum detection (UDP/TCP frags
> +                * with DF set are marked as error), drop packets based on the
> +                * error mask rather than the summary bit (ignoring L4F errors)
> +                */
> +               if (rrd.word[EMAC_RRD_STATS_DW_IDX] & EMAC_RRD_ERROR) {
> +                       netif_dbg(adpt, rx_status, adpt->netdev,
> +                                 "Drop error packet[RRD: 0x%x:0x%x:0x%x:0x%x]\n",
> +                                 rrd.word[0], rrd.word[1],
> +                                 rrd.word[2], rrd.word[3]);
> +
> +                       dev_kfree_skb(skb);
> +                       continue;
> +               }
> +
> +               skb_put(skb, rrd.genr.pkt_len - ETH_FCS_LEN);
> +               skb->dev = netdev;
> +               skb->protocol = eth_type_trans(skb, skb->dev);
> +               if (netdev->features & NETIF_F_RXCSUM)
> +                       skb->ip_summed = ((rrd.genr.l4f) ?
> +                                         CHECKSUM_NONE : CHECKSUM_UNNECESSARY);
> +               else
> +                       skb_checksum_none_assert(skb);
> +
> +               if (test_bit(EMAC_STATUS_TS_RX_EN, &adpt->status)) {
> +                       struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
> +
> +                       hwts->hwtstamp = ktime_set(rrd.genr.ts_high,
> +                                                  rrd.genr.ts_low);
> +               }
> +
> +               emac_receive_skb(rx_q, skb, (u16)rrd.genr.cvlan_tag,
> +                                (bool)rrd.genr.cvlan_flag);
> +
> +               netdev->last_rx = jiffies;
> +               (*num_pkts)++;
> +               if (*num_pkts >= max_pkts)
> +                       break;
> +       }

How about

do {
...
} while (*num_pkts < max_pkts);

> +/* Check if enough transmit descriptors are available */
> +static bool emac_tx_has_enough_descs(struct emac_tx_queue *tx_q,
> +                                    const struct sk_buff *skb)
> +{
> +       u32 num_required = 1;
> +       u16 i;

int i;

Anyway, you got the idea.  I think sized integers should be used
sparingly, and general counting and index variable should be unsized
integers, preferably also unsigned.

> +/* Fill up transmit descriptors with TSO and Checksum offload information */
> +static int emac_tso_csum(struct emac_adapter *adpt,
> +                        struct emac_tx_queue *tx_q,
> +                        struct sk_buff *skb,
> +                        union emac_tpd *tpd)
> +{
> +       u8  hdr_len;
> +       int retval;
> +
> +       if (skb_is_gso(skb)) {
> +               if (skb_header_cloned(skb)) {
> +                       retval = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> +                       if (unlikely(retval))
> +                               return retval;
> +               }
> +
> +               if (skb->protocol == htons(ETH_P_IP)) {
> +                       u32 pkt_len =
> +                               ((unsigned char *)ip_hdr(skb) - skb->data) +

Use void* for pointer math, instead of "unsigned char *".

> +/* Transmit the packet using specified transmit queue */
> +int emac_mac_tx_buf_send(struct emac_adapter *adpt, struct emac_tx_queue *tx_q,
> +                        struct sk_buff *skb)
> +{
> +       union emac_tpd tpd;
> +       u32 prod_idx;
> +
> +       if (test_bit(EMAC_STATUS_DOWN, &adpt->status)) {
> +               dev_kfree_skb_any(skb);
> +               return NETDEV_TX_OK;
> +       }
> +
> +       if (!emac_tx_has_enough_descs(tx_q, skb)) {
> +               /* not enough descriptors, just stop queue */
> +               netif_stop_queue(adpt->netdev);
> +               return NETDEV_TX_BUSY;
> +       }
> +
> +       memset(&tpd, 0, sizeof(tpd));
> +
> +       if (emac_tso_csum(adpt, tx_q, skb, &tpd) != 0) {
> +               dev_kfree_skb_any(skb);
> +               return NETDEV_TX_OK;
> +       }
> +
> +       if (skb_vlan_tag_present(skb)) {
> +               u16 vlan = skb_vlan_tag_get(skb);
> +               u16 tag;
> +
> +               EMAC_VLAN_TO_TAG(vlan, tag);
> +               tpd.genr.cvlan_tag = tag;

Can't you just do EMAC_VLAN_TO_TAG(vlan, tpd.genr.cvlan_tag);





> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.h b/drivers/net/ethernet/qualcomm/emac/emac-mac.h
> new file mode 100644
> index 0000000..a6761af
> --- /dev/null
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.h
> @@ -0,0 +1,341 @@
> +/* Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +/* EMAC DMA HW engine uses three rings:
> + * Tx:
> + *   TPD: Transmit Packet Descriptor ring.
> + * Rx:
> + *   RFD: Receive Free Descriptor ring.
> + *     Ring of descriptors with empty buffers to be filled by Rx HW.
> + *   RRD: Receive Return Descriptor ring.
> + *     Ring of descriptors with buffers filled with received data.
> + */
> +
> +#ifndef _EMAC_HW_H_
> +#define _EMAC_HW_H_
> +
> +/* EMAC_CSR register offsets */
> +#define EMAC_EMAC_WRAPPER_CSR1                                0x000000
> +#define EMAC_EMAC_WRAPPER_CSR2                                0x000004
> +#define EMAC_EMAC_WRAPPER_CSR3                                0x000008
> +#define EMAC_EMAC_WRAPPER_CSR5                                0x000010
> +#define EMAC_EMAC_WRAPPER_TX_TS_LO                            0x000104
> +#define EMAC_EMAC_WRAPPER_TX_TS_HI                            0x000108
> +#define EMAC_EMAC_WRAPPER_TX_TS_INX                           0x00010c

Can you move some of the macros into the .c files?  For example, I'm
pretty sure that the EMAC_EMAC_WRAPPER_CSRx macros are used only in
emac-sgmii.c.

Anyway, I'm stopping for now.  I'll post more later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ