On 11/29/2017 08:07 AM, Souptick Joarder wrote:

On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder [off-list ref] wrote:

quoted

On Wed, Nov 29, 2017 at 6:25 AM, David Daney [off-list ref] wrote:

quoted

From: Carlos Munoz <cmunoz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

The Cavium OCTEON cn78xx and cn73xx SoCs have network packet I/O
hardware that is significantly different from previous generations of
the family.

quoted

quoted

diff --git a/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
new file mode 100644
index 000000000000..4dad35fa4270
--- /dev/null
+++ b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c

@@ -0,0 +1,2033 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2017 Cavium, Inc.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+#include <linux/platform_device.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+

quoted

quoted

+static void bgx_port_sgmii_set_link_down(struct bgx_port_priv *priv)
+{
+       u64     data;

quoted

quoted

+       data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, priv->index));
+       data |= BIT(11);
+       oct_csr_write(data, BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, priv->index));
+       data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, priv->index));

Any particular reason to read immediately after write ?

Yes, to ensure the write is committed to hardware before the next step.

quoted

quoted

+static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, struct port_status status)
+{
+       u64     data;
+       u64     prtx;
+       u64     miscx;
+       int     timeout;
+

quoted

quoted

+
+       switch (status.speed) {
+       case 10:

In my opinion, instead of hard coding the value, is it fine to use ENUM ?

    Similar comments applicable in other places where hard coded values are used.

There is nothing to be gained by interposing an extra layer of 
abstraction in this case.  The code is more clear with the raw numbers 
in this particular case.

quoted

quoted

+static int bgx_port_gser_27882(struct bgx_port_priv *priv)
+{
+       u64     data;
+       u64     addr;

quoted

+       int     timeout = 200;
+
+   //    timeout = 200;

Better to initialize the timeout value

What are you talking about?  It is properly initialized using valid C code.

quoted

quoted

+static int bgx_port_qlm_rx_equalization(struct bgx_port_priv *priv, int qlm, int lane)
+{
+       lmode = oct_csr_read(GSER_LANE_MODE(priv->node, qlm));
+       lmode &= 0xf;
+       addr = GSER_LANE_P_MODE_1(priv->node, qlm, lmode);
+       data = oct_csr_read(addr);
+       /* Don't complete rx equalization if in VMA manual mode */
+       if (data & BIT(14))
+               return 0;
+
+       /* Apply rx equalization for speed > 6250 */
+       if (bgx_port_get_qlm_speed(priv, qlm) < 6250)
+               return 0;
+
+       /* Wait until rx data is valid (CDRLOCK) */
+       timeout = 500;

500 us is the min required value or it can be further reduced ?

500 uS works well and is shorter than the 2000 uS from the hardware manual.

If you would like to verify shorter timeout values, we could consider 
merging such a patch.  But really, this doesn't matter as it is a very 
short one-off action when the link is brought up.

quoted

quoted

+static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
+{

quoted

quoted

+
+               if (use_ber) {
+                       timeout = 10000;
+                       do {
+                               data =
+                               oct_csr_read(BGX_SPU_BR_STATUS1(priv->node, priv->bgx, priv->index));
+                               if (data & BIT(0))
+                                       break;
+                               timeout--;
+                               udelay(1);
+                       } while (timeout);

In my opinion, it's better to implement similar kind of loops inside macros.

Ok, duly noted.  I think we are in disagreement with respect to this point.

quoted

quoted

+                       if (!timeout) {
+                               pr_debug("BGX%d:%d:%d: BLK_LOCK timeout\n",
+                                        priv->bgx, priv->index, priv->node);
+                               return -1;
+                       }
+               } else {
+                       timeout = 10000;
+                       do {
+                               data =
+                               oct_csr_read(BGX_SPU_BX_STATUS(priv->node, priv->bgx, priv->index));
+                               if (data & BIT(12))
+                                       break;
+                               timeout--;
+                               udelay(1);
+                       } while (timeout);

same here

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html