Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
Hi David, Dan,
On Thu, Nov 30, 2017 at 12:50 AM, David Daney wrote:
> On 11/29/2017 08:07 AM, Souptick Joarder wrote:
>>
>> On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder
>> wrote:
>>>
>>> On Wed, Nov 29, 2017 at 6:25 AM, David Daney
>>> wrote:
From: Carlos Munoz
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
>>
>>
+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.
As mentioned by Andrew, macros defined in uapi/linux/ethtool.h may
be useful here.
Otherwise it's fine to me :)
>
>
>>
>>
+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.
I mean, instead of writing
int timeout;
timeout = 200;
write,
int timeout = 200;
Anyway both are correct and there is nothing wrong in your code.
Please ignore my comment here.
>
>
>>
>>
+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.
Ok.
>
>>
+static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
+{
>>
>>
+
+ if (use_ber) {
+ timeout = 1;
+ 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 po
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On 11/29/2017 02:56 PM, Andrew Lunn wrote:
On Tue, Nov 28, 2017 at 04:55:39PM -0800, David Daney wrote:
+static int bgx_probe(struct platform_device *pdev)
+{
+ struct mac_platform_data platform_data;
+ const __be32 *reg;
+ u32 port;
+ u64 addr;
+ struct device_node *child;
+ struct platform_device *new_dev;
+ struct platform_device *pki_dev;
+ int numa_node, interface;
+ int i;
+ int r = 0;
+ char id[64];
+ u64 data;
+
+ reg = of_get_property(pdev->dev.of_node, "reg", NULL);
+ addr = of_translate_address(pdev->dev.of_node, reg);
+ interface = (addr >> 24) & 0xf;
+ numa_node = (addr >> 36) & 0x7;
Hi David
You have these two a few times in the code. Maybe add a helper to do
it? The NUMA one i assume could go somewhere in the SoC code?
Thanks for looking at it, I will try with helpers.
The rest of your comments below raise valid points, I will fix those too.
+static int bgx_mix_init_from_fdt(void)
+{
+ struct device_node *node;
+ struct device_node *parent = NULL;
+ int mix = 0;
+ /* Get the lmac index */
+ reg = of_get_property(lmac_fdt_node, "reg", NULL);
+ if (!reg)
+ goto err;
+
+ mix_port_lmacs[mix].lmac = *reg;
I don't think of_get_property() deals with endianness. Is there any
danger of this driver being used on hardware with the other endianness
to what you have tested?
+/**
+ * bgx_pki_init_from_param - Initialize the list of lmacs that connect to the
+ * pki from information in the "pki_port" parameter.
+ *
+ * The pki_port parameter format is as follows:
+ * pki_port=nbl
+ * where:
+ * n = node
+ * b = bgx
+ * l = lmac
+ *
+ * Commas must be used to separate multiple lmacs:
+ * pki_port=000,100,110
+ *
+ * Asterisks (*) specify all possible characters in
+ * the subset:
+ * pki_port=00* (all lmacs of node0 bgx0).
+ *
+ * Missing lmacs identifiers default to all
+ * possible characters in the subset:
+ * pki_port=00 (all lmacs on node0 bgx0)
+ *
+ * Brackets ('[' and ']') specify the valid
+ * characters in the subset:
+ * pki_port=00[01] (lmac0 and lmac1 of node0 bgx0).
+ *
+ * Returns 0 if successful.
+ * Returns <0 for error codes.
I've not used kerneldoc much, but i suspect this is wrongly formated:
https://www.kernel.org/doc/html/v4.9/kernel-documentation.html#function-documentation
+int bgx_port_ethtool_set_settings(struct net_device*netdev,
+ struct ethtool_cmd*cmd)
+{
+ struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
Not required. The enforces this. See dev_ethtool()
+
+ if (p->phydev)
+ return phy_ethtool_sset(p->phydev, cmd);
+
+ return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(bgx_port_ethtool_set_settings);
+
+int bgx_port_ethtool_nway_reset(struct net_device *netdev)
+{
+ struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
Also not needed.
+static void bgx_port_adjust_link(struct net_device *netdev)
+{
+ struct bgx_port_priv*priv = bgx_port_netdev2priv(netdev);
+ boollink_changed = false;
+ unsigned intlink;
+ unsigned intspeed;
+ unsigned intduplex;
+
+ mutex_lock(&priv->lock);
+
+ if (!priv->phydev->link && priv->last_status.link)
+ link_changed = true;
+
+ if (priv->phydev->link &&
+ (priv->last_status.link != priv->phydev->link ||
+priv->last_status.duplex != priv->phydev->duplex ||
+priv->last_status.speed != priv->phydev->speed))
+ link_changed = true;
+
+ link = priv->phydev->link;
+ priv->last_status.link = priv->phydev->link;
+
+ speed = priv->phydev->speed;
+ priv->last_status.speed = priv->phydev->speed;
+
+ duplex = priv->phydev->duplex;
+ priv->last_status.duplex = priv->phydev->duplex;
+
+ mutex_unlock(&priv->lock);
+
+ if (link_changed) {
+ struct port_status status;
+
+ phy_print_status(priv->phydev);
+
+ status.link = link ? 1 : 0;
+ status.duplex = duplex;
+ status.speed = speed;
+ if (!link) {
+ netif_carrier_off(netdev);
+
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On Tue, Nov 28, 2017 at 04:55:39PM -0800, David Daney wrote:
> +static int bgx_probe(struct platform_device *pdev)
> +{
> + struct mac_platform_data platform_data;
> + const __be32 *reg;
> + u32 port;
> + u64 addr;
> + struct device_node *child;
> + struct platform_device *new_dev;
> + struct platform_device *pki_dev;
> + int numa_node, interface;
> + int i;
> + int r = 0;
> + char id[64];
> + u64 data;
> +
> + reg = of_get_property(pdev->dev.of_node, "reg", NULL);
> + addr = of_translate_address(pdev->dev.of_node, reg);
> + interface = (addr >> 24) & 0xf;
> + numa_node = (addr >> 36) & 0x7;
Hi David
You have these two a few times in the code. Maybe add a helper to do
it? The NUMA one i assume could go somewhere in the SoC code?
> +static int bgx_mix_init_from_fdt(void)
> +{
> + struct device_node *node;
> + struct device_node *parent = NULL;
> + int mix = 0;
> + /* Get the lmac index */
> + reg = of_get_property(lmac_fdt_node, "reg", NULL);
> + if (!reg)
> + goto err;
> +
> + mix_port_lmacs[mix].lmac = *reg;
I don't think of_get_property() deals with endianness. Is there any
danger of this driver being used on hardware with the other endianness
to what you have tested?
> +/**
> + * bgx_pki_init_from_param - Initialize the list of lmacs that connect to the
> + *pki from information in the "pki_port" parameter.
> + *
> + *The pki_port parameter format is as follows:
> + *pki_port=nbl
> + *where:
> + * n = node
> + * b = bgx
> + * l = lmac
> + *
> + *Commas must be used to separate multiple lmacs:
> + *pki_port=000,100,110
> + *
> + *Asterisks (*) specify all possible characters in
> + *the subset:
> + *pki_port=00* (all lmacs of node0 bgx0).
> + *
> + *Missing lmacs identifiers default to all
> + *possible characters in the subset:
> + *pki_port=00 (all lmacs on node0 bgx0)
> + *
> + *Brackets ('[' and ']') specify the valid
> + *characters in the subset:
> + *pki_port=00[01] (lmac0 and lmac1 of node0 bgx0).
> + *
> + * Returns 0 if successful.
> + * Returns <0 for error codes.
I've not used kerneldoc much, but i suspect this is wrongly formated:
https://www.kernel.org/doc/html/v4.9/kernel-documentation.html#function-documentation
> +int bgx_port_ethtool_set_settings(struct net_device *netdev,
> + struct ethtool_cmd*cmd)
> +{
> + struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
> +
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;
Not required. The enforces this. See dev_ethtool()
> +
> + if (p->phydev)
> + return phy_ethtool_sset(p->phydev, cmd);
> +
> + return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(bgx_port_ethtool_set_settings);
> +
> +int bgx_port_ethtool_nway_reset(struct net_device *netdev)
> +{
> + struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
> +
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;
Also not needed.
> +static void bgx_port_adjust_link(struct net_device *netdev)
> +{
> + struct bgx_port_priv*priv = bgx_port_netdev2priv(netdev);
> + boollink_changed = false;
> + unsigned intlink;
> + unsigned intspeed;
> + unsigned intduplex;
> +
> + mutex_lock(&priv->lock);
> +
> + if (!priv->phydev->link && priv->last_status.link)
> + link_changed = true;
> +
> + if (priv->phydev->link &&
> + (priv->last_status.link != priv->phydev->link ||
> + priv->last_status.duplex != priv->phydev->duplex ||
> + priv->last_status.speed != priv->phydev->speed))
> + link_changed = true;
> +
> + link = priv->phydev->link;
> + priv->last_status.link = priv->phydev->link;
> +
> + speed = priv->phydev->speed;
> + priv->last_status.speed = priv->phydev->speed;
> +
> + duplex = priv->phydev->duplex;
> + priv->last_status.duplex = priv->phydev->duplex;
> +
> + mutex_unlock(&priv->lock);
> +
> + if (link_changed) {
> + struct port_status status;
> +
> + phy_print_status(priv->phydev);
> +
> + status.link = link ? 1 : 0;
> + status.duplex = duplex;
> + status.speed = speed;
> + if (!link) {
> + netif_carrier_off(netdev);
> + /* Let TX drain. FIXME check that it is drained. */
> + mdelay(50);
> +
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On Wed, Nov 29, 2017 at 10:11:38PM +0300, Dan Carpenter wrote:
> On Wed, Nov 29, 2017 at 09:37:15PM +0530, Souptick Joarder wrote:
> > >> +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.
> >
>
> 10 means 10M right? That's not really a magic number. It's fine.
There are also :
uapi/linux/ethtool.h:#define SPEED_10 10
uapi/linux/ethtool.h:#define SPEED_100 100
uapi/linux/ethtool.h:#define SPEED_1000 1000
uapi/linux/ethtool.h:#define SPEED_11
uapi/linux/ethtool.h:#define SPEED_10 10
Andrew
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On 11/29/2017 08:07 AM, Souptick Joarder wrote:
On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder wrote:
On Wed, Nov 29, 2017 at 6:25 AM, David Daney wrote:
From: Carlos Munoz
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+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 = 1;
+ 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 = 1;
+ 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
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On Wed, Nov 29, 2017 at 09:37:15PM +0530, Souptick Joarder wrote:
> >> +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.
>
10 means 10M right? That's not really a magic number. It's fine.
> >> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
> >> +{
>
> >> +
> >> + if (use_ber) {
> >> + timeout = 1;
> >> + 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.
I don't understand what you mean here. For what it's worth this code
seems clear enough to me (except for the bad indenting of oct_csr_read().
It should be something like:
data =
oct_csr_read(BGX_SPU_BR_STATUS1(priv->node,
priv->bgx, priv->index));
That's over the 80 char limit but so is the original code.
regards,
dan carpenter
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder wrote:
> On Wed, Nov 29, 2017 at 6:25 AM, David Daney wrote:
>> From: Carlos Munoz
>>
>> 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 ..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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +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 ?
>> +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.
>> +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
>> +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 ?
>> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
>> +{
>> +
>> + if (use_ber) {
>> + timeout = 1;
>> + 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.
>
>> + if (!timeout) {
>> + pr_debug("BGX%d:%d:%d: BLK_LOCK timeout\n",
>> +priv->bgx, priv->index, priv->node);
>> + return -1;
>> + }
>> + } else {
>> + timeout = 1;
>> + 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
Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
On Wed, Nov 29, 2017 at 04:00:01PM +0530, Souptick Joarder wrote: Hi Souptick Please trim the code when giving reviews. We don't want to have to page through 8K lines of code it find a few comments mixed in. Just keep the beginning of the function you are commented on to make the context clear. Cut the rest. Thanks Andrew
