[PATCH net-next 1/2 v6] net: ethernet: Add DT bindings for the Gemini ethernet

[PATCH net-next 1/2 v6] net: ethernet: Add DT bindings for the Gemini ethernet

[Linus Walleij]

This adds the device tree bindings for the Gemini ethernet
controller. It is pretty straight-forward, using standard
bindings and modelling the two child ports as child devices
under the parent ethernet controller device.

Cc: devicetree at vger.kernel.org
Cc: Tobias Waldvogel <tobias.waldvogel@gmail.com>
Cc: Micha? Miros?aw <mirq-linux@rere.qmqm.pl>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/net/cortina,gemini-ethernet.txt       | 92 ++++++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt

diff --git a/Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt b/Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt
new file mode 100644
index 000000000000..35fa3abd1c73
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt
@@ -0,0 +1,92 @@
+Cortina Systems Gemini Ethernet Controller
+==========================================
+
+This ethernet controller is found in the Gemini SoC family:
+StorLink SL3512 and SL3516, also known as Cortina Systems
+CS3512 and CS3516.
+
+Required properties:
+- compatible: must be "cortina,gemini-ethernet"
+- reg: must contain the global registers and the V-bit and A-bit
+  memory areas, in total three register sets.
+- syscon: a phandle to the system controller
+- #address-cells: must be specified, must be <1>
+- #size-cells: must be specified, must be <1>
+- ranges: should be state like this giving a 1:1 address translation
+  for the subnodes
+
+The subnodes represents the two ethernet ports in this device.
+They are not independent of each other since they share resources
+in the parent node, and are thus children.
+
+Required subnodes:
+- port0: contains the resources for ethernet port 0
+- port1: contains the resources for ethernet port 1
+
+Required subnode properties:
+- compatible: must be "cortina,gemini-ethernet-port"
+- reg: must contain two register areas: the DMA/TOE memory and
+  the GMAC memory area of the port
+- interrupts: should contain the interrupt line of the port.
+  this is nominally a level interrupt active high.
+- resets: this must provide an SoC-integrated reset line for
+  the port.
+- clocks: this should contain a handle to the PCLK clock for
+  clocking the silicon in this port
+- clock-names: must be "PCLK"
+
+Optional subnode properties:
+- phy-mode: see ethernet.txt
+- phy-handle: see ethernet.txt
+
+Example:
+
+mdio-bus {
+	(...)
+	phy0: ethernet-phy at 1 {
+		reg = <1>;
+		device_type = "ethernet-phy";
+	};
+	phy1: ethernet-phy at 3 {
+		reg = <3>;
+		device_type = "ethernet-phy";
+	};
+};
+
+
+ethernet at 60000000 {
+	compatible = "cortina,gemini-ethernet";
+	reg = <0x60000000 0x4000>, /* Global registers, queue */
+	      <0x60004000 0x2000>, /* V-bit */
+	      <0x60006000 0x2000>; /* A-bit */
+	syscon = <&syscon>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	gmac0: port0 {
+		compatible = "cortina,gemini-ethernet-port";
+		reg = <0x60008000 0x2000>, /* Port 0 DMA/TOE */
+		      <0x6000a000 0x2000>; /* Port 0 GMAC */
+		interrupt-parent = <&intcon>;
+		interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
+		resets = <&syscon GEMINI_RESET_GMAC0>;
+		clocks = <&syscon GEMINI_CLK_GATE_GMAC0>;
+		clock-names = "PCLK";
+		phy-mode = "rgmii";
+		phy-handle = <&phy0>;
+	};
+
+	gmac1: port1 {
+		compatible = "cortina,gemini-ethernet-port";
+		reg = <0x6000c000 0x2000>, /* Port 1 DMA/TOE */
+		      <0x6000e000 0x2000>; /* Port 1 GMAC */
+		interrupt-parent = <&intcon>;
+		interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
+		resets = <&syscon GEMINI_RESET_GMAC1>;
+		clocks = <&syscon GEMINI_CLK_GATE_GMAC1>;
+		clock-names = "PCLK";
+		phy-mode = "rgmii";
+		phy-handle = <&phy1>;
+	};
+};
-- 
2.14.3

[PATCH net-next 2/2 v6] net: ethernet: Add a driver for Gemini gigabit ethernet

[David Miller]

From: Linus Walleij <linus.walleij@linaro.org>
Date: Sat,  2 Dec 2017 12:06:40 +0100

> +struct gmac_txq {
> +	GMAC_TXDESC_T	*ring;

Please don't create struct based typedef's, express this using
a straight "struct gmac_rxdesc_t", and also make it lowercase.

Uppercase names are reserved for CPP macros, and this is how
visually one can determine if something is a CPP macro in the
kernel sources.

Please fix this for your entire submission.

> +struct gemini_ethernet_port {
> +	unsigned int id; /* 0 or 1 */

A value taking on only 0 or 1 can be stored in a smaller type
such as 'u8'

> +		for (i = 0; i < RX_STATS_NUM; ++i)

Please always express this in the canonical way which is
to increment the index using "i++" post-postdecrement.

Please fix this for your entire submission.

> +static irqreturn_t gemini_port_irq_thread(int irq, void *data)
> +{
> +	struct gemini_ethernet_port *port = data;
> +	struct gemini_ethernet *geth = port->geth;
> +	unsigned long irqmask = SWFQ_EMPTY_INT_BIT;
> +	unsigned long flags;

Always order local variables in reverse-christmas-tree format,
which is longest to shortest line.

Again, please fix this for your entire submission.

Thank you.

[PATCH net-next 1/2 v6] net: ethernet: Add DT bindings for the Gemini ethernet

[Hans Ulli Kroll]

Hi Linus

On Sat, 2 Dec 2017, Linus Walleij wrote:

> This adds the device tree bindings for the Gemini ethernet
> controller. It is pretty straight-forward, using standard
> bindings and modelling the two child ports as child devices
> under the parent ethernet controller device.
> 
> Cc: devicetree at vger.kernel.org
> Cc: Tobias Waldvogel <tobias.waldvogel@gmail.com>
> Cc: Micha? Miros?aw <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/net/cortina,gemini-ethernet.txt       | 92 ++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt
>

Acked-by: Hans Ulli Kroll <ulli.kroll@googlemail.com>

[PATCH net-next 1/2 v6] net: ethernet: Add DT bindings for the Gemini ethernet

[Rob Herring]

On Sat, Dec 02, 2017 at 12:06:39PM +0100, Linus Walleij wrote:
> This adds the device tree bindings for the Gemini ethernet
> controller. It is pretty straight-forward, using standard
> bindings and modelling the two child ports as child devices
> under the parent ethernet controller device.
> 
> Cc: devicetree at vger.kernel.org
> Cc: Tobias Waldvogel <tobias.waldvogel@gmail.com>
> Cc: Micha? Miros?aw <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/net/cortina,gemini-ethernet.txt       | 92 ++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt b/Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt
> new file mode 100644
> index 000000000000..35fa3abd1c73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/cortina,gemini-ethernet.txt
> @@ -0,0 +1,92 @@
> +Cortina Systems Gemini Ethernet Controller
> +==========================================
> +
> +This ethernet controller is found in the Gemini SoC family:
> +StorLink SL3512 and SL3516, also known as Cortina Systems
> +CS3512 and CS3516.
> +
> +Required properties:
> +- compatible: must be "cortina,gemini-ethernet"
> +- reg: must contain the global registers and the V-bit and A-bit
> +  memory areas, in total three register sets.
> +- syscon: a phandle to the system controller
> +- #address-cells: must be specified, must be <1>
> +- #size-cells: must be specified, must be <1>
> +- ranges: should be state like this giving a 1:1 address translation
> +  for the subnodes
> +
> +The subnodes represents the two ethernet ports in this device.
> +They are not independent of each other since they share resources
> +in the parent node, and are thus children.
> +
> +Required subnodes:
> +- port0: contains the resources for ethernet port 0
> +- port1: contains the resources for ethernet port 1
> +
> +Required subnode properties:
> +- compatible: must be "cortina,gemini-ethernet-port"
> +- reg: must contain two register areas: the DMA/TOE memory and
> +  the GMAC memory area of the port
> +- interrupts: should contain the interrupt line of the port.
> +  this is nominally a level interrupt active high.
> +- resets: this must provide an SoC-integrated reset line for
> +  the port.
> +- clocks: this should contain a handle to the PCLK clock for
> +  clocking the silicon in this port
> +- clock-names: must be "PCLK"
> +
> +Optional subnode properties:
> +- phy-mode: see ethernet.txt
> +- phy-handle: see ethernet.txt
> +
> +Example:
> +
> +mdio-bus {
> +	(...)
> +	phy0: ethernet-phy at 1 {
> +		reg = <1>;
> +		device_type = "ethernet-phy";
> +	};
> +	phy1: ethernet-phy at 3 {
> +		reg = <3>;
> +		device_type = "ethernet-phy";
> +	};
> +};
> +
> +
> +ethernet at 60000000 {
> +	compatible = "cortina,gemini-ethernet";
> +	reg = <0x60000000 0x4000>, /* Global registers, queue */
> +	      <0x60004000 0x2000>, /* V-bit */
> +	      <0x60006000 0x2000>; /* A-bit */
> +	syscon = <&syscon>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges;

Would be better to define the actual range used by child nodes.

> +
> +	gmac0: port0 {

Needs a unit-address. Building with W=1 (or W=2) will tell you this.

As port is used by the OF graph binding, use ethernet-port at ... instead.

> +		compatible = "cortina,gemini-ethernet-port";
> +		reg = <0x60008000 0x2000>, /* Port 0 DMA/TOE */
> +		      <0x6000a000 0x2000>; /* Port 0 GMAC */
> +		interrupt-parent = <&intcon>;
> +		interrupts = <1 IRQ_TYPE_LEVEL_HIGH>;
> +		resets = <&syscon GEMINI_RESET_GMAC0>;
> +		clocks = <&syscon GEMINI_CLK_GATE_GMAC0>;
> +		clock-names = "PCLK";
> +		phy-mode = "rgmii";
> +		phy-handle = <&phy0>;
> +	};
> +
> +	gmac1: port1 {
> +		compatible = "cortina,gemini-ethernet-port";
> +		reg = <0x6000c000 0x2000>, /* Port 1 DMA/TOE */
> +		      <0x6000e000 0x2000>; /* Port 1 GMAC */
> +		interrupt-parent = <&intcon>;
> +		interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> +		resets = <&syscon GEMINI_RESET_GMAC1>;
> +		clocks = <&syscon GEMINI_CLK_GATE_GMAC1>;
> +		clock-names = "PCLK";
> +		phy-mode = "rgmii";
> +		phy-handle = <&phy1>;
> +	};
> +};
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 2/2 v6] net: ethernet: Add a driver for Gemini gigabit ethernet

[Michał Mirosław]

On Sat, Dec 02, 2017 at 12:06:40PM +0100, Linus Walleij wrote:
[...]
> The latest v6 incarnation of this driver was written by Micha?
> Miros?aw and submitted for inclusion in 2011. This was the
> last post:
> https://lwn.net/Articles/437889/
> 
> DaveM ACKed it at the time:
> https://marc.info/?l=linux-netdev&m=130255434310315&w=2
> 
> The controversial pieces under ARM (board files) and other
> subsystems are now gone and replaced by DeviceTree.
> 
> Micha?: I hope you don't mind me picking it up and hope
> you can still test it on your ICYbox, I have device tree
> patches in my tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/log/?h=gemini-ethernet

I'm happy to see that my work didn't go to /dev/null after all.
I haven't finished it at the time because the box I had broke down
beyond repair.

I skimmed through the patch - please find my comments below.

Best Regards,
Micha? Miros?aw


[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -0,0 +1,2461 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ethernet device driver for Cortina Systems Gemini SoC
> + * Also known as the StorLink SL3512 and SL3516 (SL351x) GMAC
> + *
> + * Authors:
> + * Linus Walleij <linus.walleij@linaro.org>
> + * Tobias Waldvogel <tobias.waldvogel@gmail.com> (OpenWRT)
> + * Micha?? Miros??aw <mirq-linux@rere.qmqm.pl>

Doubly UTF-8 encoded?

[...]
> +static int gmac_setup_txqs(struct net_device *netdev)
> +{
[...]
> +	desc_ring = dma_alloc_coherent(geth->dev, len * sizeof(*desc_ring),
> +		&port->txq_dma_base, GFP_KERNEL);
> +
> +	if (!desc_ring) {

Should check with dma_mapping_error().

> +		kfree(skb_tab);
> +		return -ENOMEM;
> +	}
> +
> +	BUG_ON(port->txq_dma_base & ~DMA_Q_BASE_MASK);

BUG is too hard here. return -EINVAL or other error? Shouldn't happen
if dma_alloc_coherent() guarantees 16B alignment.

[...]
> +static int gmac_setup_rxq(struct net_device *netdev)
> +{
[...]
> +	BUG_ON(port->rxq_dma_base & ~NONTOE_QHDR0_BASE_MASK);

Like above.

[...]
> +static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
> +					      int pn)
> +{
> +	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
> +	unsigned int frag_len = 1 << geth->freeq_frag_order;
> +	GMAC_RXDESC_T *freeq_entry;
> +	dma_addr_t mapping;
> +	struct page *page;
> +	int i;
> +
> +	page = alloc_page(GFP_ATOMIC);
> +	if (!page)
> +		return NULL;
> +
> +	mapping = dma_map_single(geth->dev, page_address(page),
> +				PAGE_SIZE, DMA_FROM_DEVICE);
> +
> +	if (unlikely(dma_mapping_error(geth->dev, mapping) || !mapping)) {

This should test only dma_mapping_error() since mapping == 0 is valid,
but unlikely.

> +static int geth_setup_freeq(struct gemini_ethernet *geth)
> +{
> +	void __iomem *dma_reg = geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG;
> +	QUEUE_THRESHOLD_T qt;
> +	DMA_SKB_SIZE_T skbsz;
> +	unsigned int filled;
> +	unsigned int frag_len = 1 << geth->freeq_frag_order;
> +	unsigned int len = 1 << geth->freeq_order;
> +	unsigned int fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
> +	unsigned int pages = len >> fpp_order;
> +	dma_addr_t mapping;
> +	unsigned int pn;
> +
> +	geth->freeq_ring = dma_alloc_coherent(geth->dev,
> +		sizeof(*geth->freeq_ring) << geth->freeq_order,
> +		&geth->freeq_dma_base, GFP_KERNEL);
> +	if (!geth->freeq_ring)
> +		return -ENOMEM;
> +
> +	BUG_ON(geth->freeq_dma_base & ~DMA_Q_BASE_MASK);

Another BUG_ON:

if (WARN_ON(...)) goto err_freeq;

[...]
> +static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
> +			    struct gmac_txq *txq, unsigned short *desc)
> +{
[...]
> +	if (word1 > mtu) {
> +		word1 |= TSS_MTU_ENABLE_BIT;
> +		word3 += mtu;

word3 |= mtu; would be more natural.

[...]
> +		mapping = dma_map_single(geth->dev, buffer, buflen,
> +					DMA_TO_DEVICE);
> +		if (dma_mapping_error(geth->dev, mapping) ||
> +			!(mapping & PAGE_MASK))
> +			goto map_error;

Is page at phys 0 special to the HW?

[...]
> +map_error:
> +	while (w != *desc) {
> +		w--;
> +		w &= m;
> +
> +		dma_unmap_page(geth->dev, txq->ring[w].word2.buf_adr,
> +			txq->ring[w].word0.bits.buffer_size, DMA_TO_DEVICE);
> +	}
> +	return ENOMEM;

return -ENOMEM; for consistency, though not used in the caller.

[...]
> +static int gmac_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
[...]
> +	if (unlikely(gmac_map_tx_bufs(netdev, skb, txq, &w))) {
> +		if (skb_linearize(skb))
> +			goto out_drop;
> +
> +		if (unlikely(gmac_map_tx_bufs(netdev, skb, txq, &w)))
> +			goto out_drop_free;
> +
> +		u64_stats_update_begin(&port->tx_stats_syncp);
> +		port->tx_frags_linearized++;
> +		u64_stats_update_end(&port->tx_stats_syncp);

This misses stats update when mapping after skb_linearize() fails.

[...]
> +static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
> +	GMAC_RXDESC_0_T word0, unsigned frame_len)
> +{
> +	struct sk_buff *skb = NULL;
> +	unsigned rx_status = word0.bits.status;
> +	unsigned rx_csum = word0.bits.chksum_status;

> +	port->rx_stats[rx_status]++;
> +	port->rx_csum_stats[rx_csum]++;
> +
> +	if (word0.bits.derr || word0.bits.perr ||
> +	    rx_status || frame_len < ETH_ZLEN ||
> +	    rx_csum >= RX_CHKSUM_IP_ERR_UNKNOWN) {
> +		port->stats.rx_errors++;
> +
> +		if (frame_len < ETH_ZLEN || RX_ERROR_LENGTH(rx_status))
> +			port->stats.rx_length_errors++;
> +		if (RX_ERROR_OVER(rx_status))
> +			port->stats.rx_over_errors++;
> +		if (RX_ERROR_CRC(rx_status))
> +			port->stats.rx_crc_errors++;
> +		if (RX_ERROR_FRAME(rx_status))
> +			port->stats.rx_frame_errors++;
> +
> +		return NULL;

Could support RXALL feature here.

> +	skb = napi_get_frags(&port->napi);
> +	if (!skb)
> +		return NULL;

This case could get a stats update, too.

> +static unsigned int gmac_rx(struct net_device *netdev, unsigned budget)
> +{
[...]
> +		if (unlikely(!mapping)) {
> +			netdev_err(netdev,
> +				   "rxq[%u]: HW BUG: zero DMA desc\n", r);
> +			goto err_drop;
> +		}

I wonder if this was a bug in the driver or in HW. Does it trigger on
your boxes?

[...]
> +static void gmac_set_rx_mode(struct net_device *netdev)
> +{
> +	struct gemini_ethernet_port *port = netdev_priv(netdev);
> +	struct netdev_hw_addr *ha;
> +	__u32 mc_filter[2];
> +	unsigned bit_nr;
> +	GMAC_RX_FLTR_T filter = { .bits = {
> +		.broadcast = 1,
> +		.multicast = 1,
> +		.unicast = 1,
> +	} };
> +
> +	mc_filter[1] = mc_filter[0] = 0;

Looks like this should be = ~0u (IFF_ALLMULTI case).

> +	if (netdev->flags & IFF_PROMISC) {
> +		filter.bits.error = 1;
> +		filter.bits.promiscuous = 1;
> +	} else if (!(netdev->flags & IFF_ALLMULTI)) {
> +		mc_filter[1] = mc_filter[0] = 0;
> +		netdev_for_each_mc_addr(ha, netdev) {
> +			bit_nr = ~crc32_le(~0, ha->addr, ETH_ALEN) & 0x3f;
> +			mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 0x1f);
> +		}
> +	}
[...]

[PATCH net-next 2/2 v6] net: ethernet: Add a driver for Gemini gigabit ethernet

[Linus Walleij]

On Mon, Dec 4, 2017 at 12:21 AM, David Miller <davem@davemloft.net> wrote:

>> +static irqreturn_t gemini_port_irq_thread(int irq, void *data)
>> +{
>> +     struct gemini_ethernet_port *port = data;
>> +     struct gemini_ethernet *geth = port->geth;
>> +     unsigned long irqmask = SWFQ_EMPTY_INT_BIT;
>> +     unsigned long flags;
>
> Always order local variables in reverse-christmas-tree format,
> which is longest to shortest line.

It's hard to do in cases like this where I use the variable
when defining another variable:

struct gemini_ethernet_port *port = netdev_priv(netdev);
void __iomem *status_reg = port->gmac_base + GMAC_STATUS;

But I understand the aesthetic, so I fix it as well as I can.

If you want me to even rewrite the above using more lines of code
to keep the aestetic (moving assignment out of the variable
definitions), tell me and I can fix that too, whatever you like.

Yours,
Linus Walleij

[PATCH net-next 2/2 v6] net: ethernet: Add a driver for Gemini gigabit ethernet

[David Miller]

From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 6 Dec 2017 14:02:49 +0100

> On Mon, Dec 4, 2017 at 12:21 AM, David Miller <davem@davemloft.net> wrote:
> 
>>> +static irqreturn_t gemini_port_irq_thread(int irq, void *data)
>>> +{
>>> +     struct gemini_ethernet_port *port = data;
>>> +     struct gemini_ethernet *geth = port->geth;
>>> +     unsigned long irqmask = SWFQ_EMPTY_INT_BIT;
>>> +     unsigned long flags;
>>
>> Always order local variables in reverse-christmas-tree format,
>> which is longest to shortest line.
> 
> It's hard to do in cases like this where I use the variable
> when defining another variable:
> 
> struct gemini_ethernet_port *port = netdev_priv(netdev);
> void __iomem *status_reg = port->gmac_base + GMAC_STATUS;
> 
> But I understand the aesthetic, so I fix it as well as I can.
> 
> If you want me to even rewrite the above using more lines of code
> to keep the aestetic (moving assignment out of the variable
> definitions), tell me and I can fix that too, whatever you like.

Yes, that is the way generally we recommend doing this.  Move
the troublesome assignment down into the actual function body.

[PATCH net-next 2/2 v6] net: ethernet: Add a driver for Gemini gigabit ethernet

[Linus Walleij]

On Tue, Dec 5, 2017 at 8:03 PM, Micha? Miros?aw <mirq-linux@rere.qmqm.pl> wrote:

> I'm happy to see that my work didn't go to /dev/null after all.
> I haven't finished it at the time because the box I had broke down
> beyond repair.

Ooops that explains why the submission was just haning in mid-air.
Sorry man :(

> I skimmed through the patch - please find my comments below.

Thanks a lot! :)

I fixed most of them for v8, just some comments:

> [...]
>> +static struct sk_buff *gmac_skb_if_good_frame(struct gemini_ethernet_port *port,
>> +     GMAC_RXDESC_0_T word0, unsigned frame_len)
>> +{
>> +     struct sk_buff *skb = NULL;
>> +     unsigned rx_status = word0.bits.status;
>> +     unsigned rx_csum = word0.bits.chksum_status;
>
>> +     port->rx_stats[rx_status]++;
>> +     port->rx_csum_stats[rx_csum]++;
>> +
>> +     if (word0.bits.derr || word0.bits.perr ||
>> +         rx_status || frame_len < ETH_ZLEN ||
>> +         rx_csum >= RX_CHKSUM_IP_ERR_UNKNOWN) {
>> +             port->stats.rx_errors++;
>> +
>> +             if (frame_len < ETH_ZLEN || RX_ERROR_LENGTH(rx_status))
>> +                     port->stats.rx_length_errors++;
>> +             if (RX_ERROR_OVER(rx_status))
>> +                     port->stats.rx_over_errors++;
>> +             if (RX_ERROR_CRC(rx_status))
>> +                     port->stats.rx_crc_errors++;
>> +             if (RX_ERROR_FRAME(rx_status))
>> +                     port->stats.rx_frame_errors++;
>> +
>> +             return NULL;
>
> Could support RXALL feature here.

Probably! I might experiment with it in a separate (follow up) patch
since I have to figure out how much the hardware supports ignoring
errors and exploit that properly.

>> +static unsigned int gmac_rx(struct net_device *netdev, unsigned budget)
>> +{
> [...]
>> +             if (unlikely(!mapping)) {
>> +                     netdev_err(netdev,
>> +                                "rxq[%u]: HW BUG: zero DMA desc\n", r);
>> +                     goto err_drop;
>> +             }
>
> I wonder if this was a bug in the driver or in HW. Does it trigger on
> your boxes?

No I haven't seen it. I think it may be a HW bug on elder chips
(before SL3512 and SL3516) that is just not manifesting on newer
hardware.

Better keep the code though.

>> +static void gmac_set_rx_mode(struct net_device *netdev)
>> +{
>> +     struct gemini_ethernet_port *port = netdev_priv(netdev);
>> +     struct netdev_hw_addr *ha;
>> +     __u32 mc_filter[2];
>> +     unsigned bit_nr;
>> +     GMAC_RX_FLTR_T filter = { .bits = {
>> +             .broadcast = 1,
>> +             .multicast = 1,
>> +             .unicast = 1,
>> +     } };
>> +
>> +     mc_filter[1] = mc_filter[0] = 0;
>
> Looks like this should be = ~0u (IFF_ALLMULTI case).

Yeah it's some error compared to the (horrible) vendor code
here. The vendor tree explicitly checks for both promiscuous
and multicast and sets the masks to ~0 in both cases
explicitly, else leave it as default 0 with the funky
algorithm to set up the mask bit by bit.

I rewrote this piece of code to do the same.

Yours,
Linus Walleij