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

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

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.

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

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@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@1 {
> +		reg = <1>;
> +		device_type = "ethernet-phy";
> +	};
> +	phy1: ethernet-phy@3 {
> +		reg = <3>;
> +		device_type = "ethernet-phy";
> +	};
> +};
> +
> +
> +ethernet@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@... 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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);
> +		}
> +	}
[...]

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

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.

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