[PATCH v5 0/3] net: Add Keystone NetCP ethernet driver support

[PATCH v5 0/3] net: Add Keystone NetCP ethernet driver support

[Santosh Shilimkar]

v4->v5
Sorry to spin v5 quickly but I missed few check-patch warnings which
were pointed by Joe Perches(thanks). I folded his changes [5] along with
few more check-patch warning fixes. I would like get this in for v3.18
merge window if David is happy with this version.

v3->v4
Couple of fixes in in error path as pointed [4] out by David. Rest of
the patches are unchanged from v3. 

v2->v3
Update v3 after incorporating Jamal and David Miller's comment/suggestion
from earlier versions [1] [2].  After per the discussion here [3], the
controversial custom exports have been dropped now. And for future
future offload support additions, we will plug into generic frameworks
as an when they are available.

The network coprocessor (NetCP) is a hardware accelerator that processes
Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsystem with a ethernet
switch sub-module to send and receive packets. NetCP also includes a packet
accelerator (PA) module to perform packet classification operations such as
header matching, and packet modification operations such as checksum
generation. NetCP can also optionally include a Security Accelerator(SA)
capable of performing IPSec operations on ingress/egress packets.
    
Keystone SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
includes a 3-port Ethernet switch sub-module capable of 10Gb/s and
1Gb/s rates per Ethernet port.
    
Both GBE and XGBE network processors supported using common driver. It
is also designed to handle future variants of NetCP.

Cc: David Miller <davem@davemloft.net>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Sandeep Nair <sandeep_n@ti.com>

Sandeep Nair (3):
  Documentation: dt: net: Add binding doc for Keystone NetCP ethernet
    driver
  net: Add Keystone NetCP ethernet driver
  MAINTAINER: net: Add TI NETCP Ethernet driver entry

 .../devicetree/bindings/net/keystone-netcp.txt     |  197 ++
 MAINTAINERS                                        |    7 +
 drivers/net/ethernet/ti/Kconfig                    |   12 +-
 drivers/net/ethernet/ti/Makefile                   |    4 +
 drivers/net/ethernet/ti/netcp.h                    |  227 ++
 drivers/net/ethernet/ti/netcp_core.c               | 2262 ++++++++++++++++++++
 drivers/net/ethernet/ti/netcp_ethss.c              | 2173 +++++++++++++++++++
 drivers/net/ethernet/ti/netcp_sgmii.c              |  130 ++
 drivers/net/ethernet/ti/netcp_xgbepcsr.c           |  502 +++++
 9 files changed, 5513 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/keystone-netcp.txt
 create mode 100644 drivers/net/ethernet/ti/netcp.h
 create mode 100644 drivers/net/ethernet/ti/netcp_core.c
 create mode 100644 drivers/net/ethernet/ti/netcp_ethss.c
 create mode 100644 drivers/net/ethernet/ti/netcp_sgmii.c
 create mode 100644 drivers/net/ethernet/ti/netcp_xgbepcsr.c

Regards,
Santosh
[1] https://lkml.org/lkml/2014/4/22/805
[2] https://lkml.org/lkml/2014/8/15/218
[3] https://lkml.org/lkml/2014/9/11/691
[4] https://lkml.org/lkml/2014/9/15/403
[5] https://lkml.org/lkml/2014/9/25/332
-- 
1.7.9.5

[PATCH v5 1/3] Documentation: dt: net: Add binding doc for Keystone NetCP ethernet driver

[Santosh Shilimkar]

From: Sandeep Nair <sandeep_n@ti.com>

The network coprocessor (NetCP) is a hardware accelerator that processes
Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsystem with a ethernet
switch sub-module to send and receive packets. NetCP also includes a packet
accelerator (PA) module to perform packet classification operations such as
header matching, and packet modification operations such as checksum
generation. NetCP can also optionally include a Security Accelerator(SA)
capable of performing IPSec operations on ingress/egress packets.

Keystone SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
includes a 3-port Ethernet switch sub-module capable of 10Gb/s and
1Gb/s rates per Ethernet port.

NetCP Subsystem device tree layout looks something like below:

-----------------------------
  NetCP subsystem(10G or 1G)
-----------------------------
	|
	|-> NetCP Devices ->	|
	|			|-> GBE/XGBE Switch
	|			|
	|			|-> Packet Accelerator
	|			|
	|			|-> Security Accelerator
	|
	|
	|
	|-> NetCP Interfaces ->	|
				|-> Ethernet Port 0
				|
				|-> Ethernet Port 1
				|
				|-> Ethernet Port 2
				|
				|-> Ethernet Port 3

Common driver supports GBE as well XGBE network processors.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: David Miller <davem@davemloft.net>

Signed-off-by: Sandeep Nair <sandeep_n@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 .../devicetree/bindings/net/keystone-netcp.txt     |  197 ++++++++++++++++++++
 1 file changed, 197 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/keystone-netcp.txt

diff --git a/Documentation/devicetree/bindings/net/keystone-netcp.txt b/Documentation/devicetree/bindings/net/keystone-netcp.txt
new file mode 100644
index 0000000..a7d061b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/keystone-netcp.txt
@@ -0,0 +1,197 @@
+This document describes the device tree bindings associated with the
+keystone network coprocessor(NetCP) driver support.
+
+The network coprocessor (NetCP) is a hardware accelerator that processes
+Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsytem with a ethernet
+switch sub-module to send and receive packets. NetCP also includes a packet
+accelerator (PA) module to perform packet classification operations such as
+header matching, and packet modification operations such as checksum
+generation. NetCP can also optionally include a Security Accelerator (SA)
+capable of performing IPSec operations on ingress/egress packets.
+
+Keystone II SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
+includes a 3-port Ethernet switch sub-module capable of 10Gb/s and 1Gb/s rates
+per Ethernet port.
+
+Keystone NetCP driver has a plug-in module architecture where each of the NetCP
+sub-modules exist as a loadable kernel module which plug in to the netcp core.
+These sub-modules are represented as "netcp-devices" in the dts bindings. It is
+mandatory to have the ethernet switch sub-module for the ethernet interface to
+be operational. Any other sub-module like the PA is optional.
+
+NetCP Ethernet SubSystem Layout:
+
+-----------------------------
+  NetCP subsystem(10G or 1G)
+-----------------------------
+	|
+	|-> NetCP Devices ->	|
+	|			|-> GBE/XGBE Switch
+	|			|
+	|			|-> Packet Accelerator
+	|			|
+	|			|-> Security Accelerator
+	|
+	|
+	|
+	|-> NetCP Interfaces ->	|
+				|-> Ethernet Port 0
+				|
+				|-> Ethernet Port 1
+				|
+				|-> Ethernet Port 2
+				|
+				|-> Ethernet Port 3
+
+
+NetCP subsystem properties:
+Required properties:
+- compatible:	Should be "ti,netcp-1.0"
+- clocks:	phandle to the reference clocks for the subsystem.
+- dma-id:	Navigator packet dma instance id.
+
+Optional properties:
+- reg:		register location and the size for the following register
+		regions in the specified order.
+		- Efuse MAC address register
+- dma-coherent:	Present if dma operations are coherent
+- big-endian:	Keystone devices can be operated in a mode where the DSP is in
+		the big endian mode. In such cases enable this option. This
+		option should also be enabled if the ARM is operated in
+		big endian mode with the DSP in little endian.
+
+NetCP device properties: Device specification for NetCP sub-modules.
+1Gb/10Gb (gbe/xgbe) ethernet switch sub-module specifications.
+Required properties:
+- label:	Must be "netcp-gbe" for 1Gb & "netcp-xgbe" for 10Gb.
+- reg:		register location and the size for the following register
+		regions in the specified order.
+		- subsystem registers
+		- serdes registers
+- tx-channel:	the navigator packet dma channel number for tx.
+- tx-queue:	the navigator queue number associated with the tx dma channel.
+- interfaces:	specification for each of the switch port to be registered as a
+		network interface in the stack.
+-- slave-port:	Switch port number, 0 based numbering.
+-- link-interface:	type of link interface, supported options are
+			- mac<->mac auto negotiate mode: 0
+			- mac<->phy mode: 1
+			- mac<->mac forced mode: 2
+			- mac<->fiber mode: 3
+			- mac<->phy mode with no mdio: 4
+			- 10Gb mac<->phy mode : 10
+			- 10Gb mac<->mac forced mode : 11
+----phy-handle:	phandle to PHY device
+
+Optional properties:
+- enable-ale:	NetCP driver keeps the address learning feature in the ethernet
+		switch module disabled. This attribute is to enable the address
+		learning.
+- secondary-slave-ports:	specification for each of the switch port not be
+				registered as a network interface. NetCP driver
+				will only initialize these ports and attach PHY
+				driver to them if needed.
+
+NetCP interface properties: Interface specification for NetCP sub-modules.
+Required properties:
+- rx-channel:	the navigator packet dma channel number for rx.
+- rx-queue:	the navigator queue number associated with rx dma channel.
+- rx-pool:	specifies the number of descriptors to be used & the region-id
+		for creating the rx descriptor pool.
+- tx-pool:	specifies the number of descriptors to be used & the region-id
+		for creating the tx descriptor pool.
+- rx-queue-depth:	number of descriptors in each of the free descriptor
+			queue (FDQ) for the pktdma Rx flow. There can be at
+			present a maximum of 4 queues per Rx flow.
+- rx-buffer-size:	the buffer size for each of the Rx flow FDQ.
+- tx-completion-queue:	the navigator queue number where the descriptors are
+			recycled after Tx DMA completion.
+
+Optional properties:
+- efuse-mac:	If this is 1, then the MAC address for the interface is
+		obtained from the device efuse mac address register
+- local-mac-address:	the driver is designed to use the of_get_mac_address api
+			only if efuse-mac is 0. When efuse-mac is 0, the MAC
+			address is obtained from local-mac-address. If this
+			attribute is not present, then the driver will use a
+			random MAC address.
+- "netcp-device label":	phandle to the device specification for each of NetCP
+			sub-module attached to this interface.
+
+Example binding:
+
+netcp: netcp at 2090000 {
+	reg = <0x2620110 0x8>;
+	reg-names = "efuse";
+	compatible = "ti,netcp-1.0";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
+	dma-coherent;
+	/* big-endian; */
+	dma-id = <0>;
+
+	netcp-devices {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		gbe at 0x2090000 {
+			label = "netcp-gbe";
+			reg = <0x2090000 0xf00>;
+			/* enable-ale; */
+			tx-queue = <648>;
+			tx-channel = <8>;
+
+			interfaces {
+				gbe0: interface-0 {
+					slave-port = <0>;
+					link-interface	= <4>;
+				};
+				gbe1: interface-1 {
+					slave-port = <1>;
+					link-interface	= <4>;
+				};
+			};
+
+			secondary-slave-ports {
+				port-2 {
+					slave-port = <2>;
+					link-interface	= <2>;
+				};
+				port-3 {
+					slave-port = <3>;
+					link-interface	= <2>;
+				};
+			};
+		};
+	};
+
+	netcp-interfaces {
+		interface-0 {
+			rx-channel = <22>;
+			rx-pool = <1024 12>;
+			tx-pool = <1024 12>;
+			rx-queue-depth = <128 128 0 0>;
+			rx-buffer-size = <1518 4096 0 0>;
+			rx-queue = <8704>;
+			tx-completion-queue = <8706>;
+			efuse-mac = <1>;
+			netcp-gbe = <&gbe0>;
+
+		};
+		interface-1 {
+			rx-channel = <23>;
+			rx-pool = <1024 12>;
+			tx-pool = <1024 12>;
+			rx-queue-depth = <128 128 0 0>;
+			rx-buffer-size = <1518 4096 0 0>;
+			rx-queue = <8705>;
+			tx-completion-queue = <8707>;
+			efuse-mac = <0>;
+			local-mac-address = [02 18 31 7e 3e 6f];
+			netcp-gbe = <&gbe1>;
+		};
+	};
+};
-- 
1.7.9.5

[PATCH v5 3/3] MAINTAINER: net: Add TI NETCP Ethernet driver entry

[Santosh Shilimkar]

From: Sandeep Nair <sandeep_n@ti.com>

Signed-off-by: Sandeep Nair <sandeep_n@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 MAINTAINERS |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aefa948..4821dcf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9117,6 +9117,13 @@ F:	drivers/power/lp8788-charger.c
 F:	drivers/regulator/lp8788-*.c
 F:	include/linux/mfd/lp8788*.h
 
+TI NETCP ETHERNET DRIVER
+M:	Sandeep Nair <sandeep_n@ti.com>
+M:	Santosh Shilimkar <santosh.shilimkar@ti.com>
+L:	netdev at vger.kernel.org
+S:	Maintained
+F:	drivers/net/ethernet/ti/netcp*
+
 TI TWL4030 SERIES SOC CODEC DRIVER
 M:	Peter Ujfalusi <peter.ujfalusi@ti.com>
 L:	alsa-devel at alsa-project.org (moderated for non-subscribers)
-- 
1.7.9.5

[PATCH v5 2/3] net: Add Keystone NetCP ethernet driver

[David Miller]

From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Thu, 25 Sep 2014 13:48:36 -0400

> +static inline int gbe_phy_link_status(struct gbe_slave *slave)
> +{
> +	if (!slave->phy)
> +		return 1;
> +
> +	if (slave->phy->link)
> +		return 1;
> +
> +	return 0;
> +}

Please use 'bool' as the return type and return 'true' or 'false'.

Do not use 'inline' in foo.c files, let the compiler decide.  Please
audit this entire submission for this problem.

> +static int gbe_port_reset(struct gbe_slave *slave)
> +{
> +	u32 i, v;
> +
> +	/* Set the soft reset bit */
> +	writel_relaxed(SOFT_RESET, GBE_REG_ADDR(slave, emac_regs, soft_reset));

This driver seems to use relaxed readl and writel for almost everything.

That absolutely cannot be right.  For example, here, you depend upon the
ordering of this writel_relaxed() to reset the chip relative to the
real_relaxed() you subsequently do to check ths bits.

I seriously think that *_relaxed() should only be done in very special
circumstances where 1) the performance matters and 2) the validity of
the usage has been put under a microscope and fully documented with huge
comments above the *_relaxed() calls.

If you cannot reduce and properly document the really necessary *_relaxed()
uses, just convert them all to non-_relaxed() for now.

I'm also warning you ahead of time that since nobody else seems to feel
like reviewing this enormous submission, you are going to have to get used
to me pushing back on these changes over and over for small things like
coding style and structural/API issues until some reviews it on a higher
level.

I really don't want to apply this series until someone thinks seriously
about the driver's design and the long term ramifications of having a
driver like this in the tree with so many random TX etc. hooks.

[PATCH v5 2/3] net: Add Keystone NetCP ethernet driver

[Santosh Shilimkar]

On Monday 29 September 2014 03:52 PM, David Miller wrote:
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Thu, 25 Sep 2014 13:48:36 -0400
> 
>> +static inline int gbe_phy_link_status(struct gbe_slave *slave)
>> +{
>> +	if (!slave->phy)
>> +		return 1;
>> +
>> +	if (slave->phy->link)
>> +		return 1;
>> +
>> +	return 0;
>> +}
> 
> Please use 'bool' as the return type and return 'true' or 'false'.
> 
ok

> Do not use 'inline' in foo.c files, let the compiler decide.  Please
> audit this entire submission for this problem.
>
ok.
 
>> +static int gbe_port_reset(struct gbe_slave *slave)
>> +{
>> +	u32 i, v;
>> +
>> +	/* Set the soft reset bit */
>> +	writel_relaxed(SOFT_RESET, GBE_REG_ADDR(slave, emac_regs, soft_reset));
> 
> This driver seems to use relaxed readl and writel for almost everything.
> 
> That absolutely cannot be right.  For example, here, you depend upon the
> ordering of this writel_relaxed() to reset the chip relative to the
> real_relaxed() you subsequently do to check ths bits.
> 
> I seriously think that *_relaxed() should only be done in very special
> circumstances where 1) the performance matters and 2) the validity of
> the usage has been put under a microscope and fully documented with huge
> comments above the *_relaxed() calls.
> 
> If you cannot reduce and properly document the really necessary *_relaxed()
> uses, just convert them all to non-_relaxed() for now.
> 
We can stick to non-*relaxed() versions. No problems here.

> I'm also warning you ahead of time that since nobody else seems to feel
> like reviewing this enormous submission, you are going to have to get used
> to me pushing back on these changes over and over for small things like
> coding style and structural/API issues until some reviews it on a higher
> level.
>
> I really don't want to apply this series until someone thinks seriously
> about the driver's design and the long term ramifications of having a
> driver like this in the tree with so many random TX etc. hooks.
> 
The driver has been on the list. Jamal and you have given your comments,
suggestion and we have incorporated that. What else we can do ?

We are badly missing mainline network driver support for the Keystone
and hence I request you to help here.

regards,
Santosh

[PATCH v5 2/3] net: Add Keystone NetCP ethernet driver

[David Miller]

From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Mon, 29 Sep 2014 16:02:24 -0400

> We are badly missing mainline network driver support for the Keystone
> and hence I request you to help here.

It is absolutely not reasonable for you depend specifically upon me
for top-level review of a given change, sorry.

[PATCH v5 2/3] net: Add Keystone NetCP ethernet driver

[Santosh Shilimkar]

On Monday 29 September 2014 04:12 PM, David Miller wrote:
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Mon, 29 Sep 2014 16:02:24 -0400
> 
>> We are badly missing mainline network driver support for the Keystone
>> and hence I request you to help here.
> 
> It is absolutely not reasonable for you depend specifically upon me
> for top-level review of a given change, sorry.
> 
Sorry I didn't mean you specifically to review everything. But a guidance
on how we can move forward. If you can suggest someone, we can try to
request them to have a look. The design of driver is just now standard
NIC card. We removed the controversial plug in framework after your
and Jamal's suggestion. Jamal already commented on the offload design of
the driver and we addressed them in the last version.

We can follow it up on any comments, suggestions quickly so do let us
know.

Regards,
Santosh

[PATCH v5 2/3] net: Add Keystone NetCP ethernet driver

[David Laight]

From: David Miller
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Thu, 25 Sep 2014 13:48:36 -0400
> 
> > +static inline int gbe_phy_link_status(struct gbe_slave *slave)
> > +{
> > +	if (!slave->phy)
> > +		return 1;
> > +
> > +	if (slave->phy->link)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> 
> Please use 'bool' as the return type and return 'true' or 'false'.

That function body could also be just:
	return !slave->phy && slave->phy->link;
which might be more readable if directly coded.

I also wonder if slave>phy can actually be NULL?
If it can be under unusual circumstances it might be worth
assigning the address of a dummy 'phy' structure so that the
tests can all be removed.

	David

[PATCH v5 2/3] net: Add Keystone NetCP ethernet driver

[Geert Uytterhoeven]

On Tue, Sep 30, 2014 at 3:09 PM, David Laight <David.Laight@aculab.com> wrote:
>> > +static inline int gbe_phy_link_status(struct gbe_slave *slave)
>> > +{
>> > +   if (!slave->phy)
>> > +           return 1;
>> > +
>> > +   if (slave->phy->link)
>> > +           return 1;
>> > +
>> > +   return 0;
>> > +}
>>
>> Please use 'bool' as the return type and return 'true' or 'false'.
>
> That function body could also be just:
>         return !slave->phy && slave->phy->link;
> which might be more readable if directly coded.

return !slave->phy || slave->phy->link;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds