Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1287809 > unrolled thread
| Started by | Timur Tabi <timur@codeaurora.org> |
|---|---|
| First post | 2015-12-09 21:10 +0100 |
| Last post | 2015-12-10 05:10 +0100 |
| Articles | 5 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2015-12-09 21:10 +0100
Re: [PATCH] net: emac: emac gigabit ethernet controller driver Fabio Estevam <festevam@gmail.com> - 2015-12-09 21:40 +0100
Re: [PATCH] net: emac: emac gigabit ethernet controller driver David Miller <davem@davemloft.net> - 2015-12-09 22:00 +0100
Re: [PATCH] net: emac: emac gigabit ethernet controller driver Gilad Avidov <gavidov@codeaurora.org> - 2015-12-10 01:30 +0100
Re: [PATCH] net: emac: emac gigabit ethernet controller driver Timur Tabi <timur@codeaurora.org> - 2015-12-10 05:10 +0100
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-12-09 21:10 +0100 |
| Subject | Re: [PATCH] net: emac: emac gigabit ethernet controller driver |
| Message-ID | <qDTdN-4yA-31@gated-at.bofh.it> |
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@codeaurora.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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2015-12-09 21:40 +0100 |
| Message-ID | <qDTGN-4Jx-21@gated-at.bofh.it> |
| In reply to | #1287809 |
On Wed, Dec 9, 2015 at 6:09 PM, Timur Tabi <timur@codeaurora.org> wrote:
>> +/* 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.
> */
Except in drivers/net. The convention in drivers/net is to use
multi-line format as posted in this patch.
--
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/
[toc] | [prev] | [next] | [standalone]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2015-12-09 22:00 +0100 |
| Message-ID | <qDU0a-4T6-7@gated-at.bofh.it> |
| In reply to | #1287835 |
From: Fabio Estevam <festevam@gmail.com>
Date: Wed, 9 Dec 2015 18:37:35 -0200
> On Wed, Dec 9, 2015 at 6:09 PM, Timur Tabi <timur@codeaurora.org> wrote:
>
>>> +/* 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.
>> */
>
> Except in drivers/net. The convention in drivers/net is to use
> multi-line format as posted in this patch.
Correct.
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Gilad Avidov <gavidov@codeaurora.org> |
|---|---|
| Date | 2015-12-10 01:30 +0100 |
| Message-ID | <qDXho-76y-17@gated-at.bofh.it> |
| In reply to | #1287809 |
Thank you Timur for the good review.
On Wed, 9 Dec 2015 14:09:27 -0600
Timur Tabi <timur@codeaurora.org> wrote:
> 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?
Agree its a lot to review, however:
1) This driver is the what left after I removed all unnecessary
features, thus it is already minimal.
I have removed features such as support for: server, ACPI, ethtool,
ptp/1588, etc.
2) This size seems comparable to first patches of existing Ethernet
drivers.
>
> On Mon, Dec 7, 2015 at 4:58 PM, Gilad Avidov <gavidov@codeaurora.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?
msm appears only in this DT binding example. None in the code.
I will look into removing this instance too.
>
> > + 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?
ok.
>
> > +
> > 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
ok.
>
> > +/* 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?
Agree.
>
> > + 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()
>
ok.
> > +/* 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.
ok.
>
> > +/* 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?
TPD: Transmit Packet Descriptor ring.
See definition of TPD, RFD and RRD at the top of emac-mac.h
>
> > + 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?
>
the offsets are all different:
EMAC_H3TPD_BASE_ADDR_LO, EMAC_H2TPD_BASE_ADDR_LO,
EMAC_H1TPD_BASE_ADDR_LO, EMAC_DESC_CTRL_8
of course I can put the offset in an array, but I am not sure that it
will look better.
> > +/* 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.
agree.
>
> > +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.
ok.
>
> > +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.
ok.
>
> > +/* 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'
ok.
>
> > +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.
>
Ill go with unsigned int.
> > +/* 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;
will do.
>
> > +/* 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
ok.
>
> > +
> > + 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".
ok.
>
> > +}
> > +
> > +static int emac_tx_q_desc_alloc_all(struct emac_adapter *adpt)
> > +{
> > + int retval = 0;
> > + u8 i;
>
> int i;
ok.
>
> > +static void emac_rx_q_free_bufs_all(struct emac_adapter *adpt)
> > +{
> > + u8 i;
>
> int i;
ok.
>
> > +/* 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
ok.
>
> > +static int emac_rx_descs_allocs_all(struct emac_adapter *adpt)
> > +{
> > + int retval = 0;
> > + u8 i;
>
> int i;
ok.
>
> > +
> > + 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
ok.
>
> > +/* 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.
>
Agree.
> > +}
> > +
> > +/* 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.
Agree.
>
> > + 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().
ok.
>
> > +/* 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.
I will change both to rmb()
>
> > +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;
ok.
>
> > +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));
Agree.
>
> > +/* 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;
ok.
>
> > + 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);
Agree.
>
> > +/* 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.
>
ok. I will change it throughout the driver.
> > +/* 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 *".
pointer math on void* ?
what is the size of void ?
>
> > +/* 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);
>
>
Should have been the way you suggested.
However, per Felix's comment I will change the bit fields to bitwise
macros. This will force code slow similar to the original.
>
>
>
> > 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.
I have moved all that made sense to .c files already. If I find
anything else that can be moved I will move it too.
For the case of EMAC_EMAC_WRAPPER_CSRx:
EMAC_EMAC_WRAPPER_CSR1 used in emac-mac.c
EMAC_EMAC_WRAPPER_CSR1 used in emac-mac.c and emac-sgmii.c
and
EMAC_EMAC_WRAPPER_TX_xxx used in emac-mac.c
Since they are all part of EMAC_CSR register space I think that it is
better to keep them togther.
>
> Anyway, I'm stopping for now. I'll post more later.
Thank you again. Looking forward for the rest of it.
Gilad
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-arm-msm" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Timur Tabi <timur@codeaurora.org> |
|---|---|
| Date | 2015-12-10 05:10 +0100 |
| Message-ID | <qE0Ih-11h-7@gated-at.bofh.it> |
| In reply to | #1288080 |
Gilad Avidov wrote: > pointer math on void* ? > what is the size of void ? I'm talking about adding and subtracting pointer values, so u32 pkt_len =((void *)ip_hdr(skb) - skb->data) -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation. -- 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/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web