Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[David Daney]

On 11/29/2017 08:07 AM, Souptick Joarder wrote:
> On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder <jrdr.linux@xxxxxxxxx> wrote:
> > On Wed, Nov 29, 2017 at 6:25 AM, David Daney <david.daney@xxxxxxxxxx> wrote:
> > > From: Carlos Munoz <cmunoz@xxxxxxxxxx>
> > >
> > > The Cavium OCTEON cn78xx and cn73xx SoCs have network packet I/O
> > > hardware that is significantly different from previous generations of
> > > the family.
>
> > > 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>
> > > +
>
> > > +static void bgx_port_sgmii_set_link_down(struct bgx_port_priv *priv)
> > > +{
> > > +       u64     data;
>
> > > +       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.

> > > +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;
> > > +
>
> > > +
> > > +       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.


> > > +static int bgx_port_gser_27882(struct bgx_port_priv *priv)
> > > +{
> > > +       u64     data;
> > > +       u64     addr;
> >
> > > +       int     timeout = 200;
> > > +
> > > +   //    timeout = 200;
> Better to initialize the timeout value

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


> > > +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.

> > > +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
> > > +{
>
> > > +
> > > +               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.

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