| Message ID | 20230418111459.811553-5-vladimir.oltean@nxp.com |
|---|---|
| State | Accepted |
| Commit | 16a2c7634442b48ff24177d04b7ce429840b102e |
| Headers | show |
| Series | ethtool mm API consolidation | expand |
On Tue, Apr 18, 2023 at 02:14:54PM +0300, Vladimir Oltean wrote: > These have been useful in debugging various problems related to frame > preemption, so make them available through ethtool --register-dump for > later too. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > v1->v2: patch is new > > .../ethernet/freescale/enetc/enetc_ethtool.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > index 838a92131963..e993ed04ab57 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > @@ -32,6 +32,12 @@ static const u32 enetc_port_regs[] = { > ENETC_PM0_CMD_CFG, ENETC_PM0_MAXFRM, ENETC_PM0_IF_MODE > }; > > +static const u32 enetc_port_mm_regs[] = { > + ENETC_MMCSR, ENETC_PFPMR, ENETC_PTCFPR(0), ENETC_PTCFPR(1), > + ENETC_PTCFPR(2), ENETC_PTCFPR(3), ENETC_PTCFPR(4), ENETC_PTCFPR(5), > + ENETC_PTCFPR(6), ENETC_PTCFPR(7), > +}; > + > static int enetc_get_reglen(struct net_device *ndev) > { > struct enetc_ndev_priv *priv = netdev_priv(ndev); > @@ -45,6 +51,9 @@ static int enetc_get_reglen(struct net_device *ndev) > if (hw->port) > len += ARRAY_SIZE(enetc_port_regs); > > + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU)) nit: I think you could make the condition. if (hw->port && priv->si->hw_features & ENETC_SI_F_QBU) which would be consistent with the condition in the next hunk. > + len += ARRAY_SIZE(enetc_port_mm_regs); > + > len *= sizeof(u32) * 2; /* store 2 entries per reg: addr and value */ > > return len; > @@ -90,6 +99,14 @@ static void enetc_get_regs(struct net_device *ndev, struct ethtool_regs *regs, > *buf++ = addr; > *buf++ = enetc_rd(hw, addr); > } > + > + if (priv->si->hw_features & ENETC_SI_F_QBU) { > + for (i = 0; i < ARRAY_SIZE(enetc_port_mm_regs); i++) { > + addr = ENETC_PORT_BASE + enetc_port_mm_regs[i]; > + *buf++ = addr; > + *buf++ = enetc_rd(hw, addr); > + } > + } > } > > static const struct { > -- > 2.34.1 >
On Thu, Apr 20, 2023 at 04:38:00PM +0200, Simon Horman wrote: > > + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU)) > > nit: I think you could make the condition. > > if (hw->port && priv->si->hw_features & ENETC_SI_F_QBU) > > which would be consistent with the condition in the next hunk. > > > + if (priv->si->hw_features & ENETC_SI_F_QBU) { Maybe, but it generates the exact same object code (tested with "make drivers/net/ethernet/freescale/enetc/enetc_ethtool.lst"). When I'm debugging, I'm a bit of a conspiracy theorist when it comes to operator precedence (& vs &&), and so, "A && B & C" doesn't read particularly well to me, and would be one of my first suspects at hiding a bug. I do know it would have worked in this case though, and that modern gcc/clang usually complains about suspicious/ unintuitive precedence.
On Thu, Apr 20, 2023 at 07:58:52PM +0300, Vladimir Oltean wrote: > On Thu, Apr 20, 2023 at 04:38:00PM +0200, Simon Horman wrote: > > > + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU)) > > > > nit: I think you could make the condition. > > > > if (hw->port && priv->si->hw_features & ENETC_SI_F_QBU) > > > > which would be consistent with the condition in the next hunk. > > > > > + if (priv->si->hw_features & ENETC_SI_F_QBU) { > > Maybe, but it generates the exact same object code (tested with > "make drivers/net/ethernet/freescale/enetc/enetc_ethtool.lst"). > > When I'm debugging, I'm a bit of a conspiracy theorist when it comes > to operator precedence (& vs &&), and so, "A && B & C" doesn't read > particularly well to me, and would be one of my first suspects at > hiding a bug. I do know it would have worked in this case though, > and that modern gcc/clang usually complains about suspicious/ > unintuitive precedence. Thanks, I guess it's subjective. And I do understand your point regarding & vs &&. No need to resend because of this (or update the code at all if that is your choice).
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c index 838a92131963..e993ed04ab57 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c @@ -32,6 +32,12 @@ static const u32 enetc_port_regs[] = { ENETC_PM0_CMD_CFG, ENETC_PM0_MAXFRM, ENETC_PM0_IF_MODE }; +static const u32 enetc_port_mm_regs[] = { + ENETC_MMCSR, ENETC_PFPMR, ENETC_PTCFPR(0), ENETC_PTCFPR(1), + ENETC_PTCFPR(2), ENETC_PTCFPR(3), ENETC_PTCFPR(4), ENETC_PTCFPR(5), + ENETC_PTCFPR(6), ENETC_PTCFPR(7), +}; + static int enetc_get_reglen(struct net_device *ndev) { struct enetc_ndev_priv *priv = netdev_priv(ndev); @@ -45,6 +51,9 @@ static int enetc_get_reglen(struct net_device *ndev) if (hw->port) len += ARRAY_SIZE(enetc_port_regs); + if (hw->port && !!(priv->si->hw_features & ENETC_SI_F_QBU)) + len += ARRAY_SIZE(enetc_port_mm_regs); + len *= sizeof(u32) * 2; /* store 2 entries per reg: addr and value */ return len; @@ -90,6 +99,14 @@ static void enetc_get_regs(struct net_device *ndev, struct ethtool_regs *regs, *buf++ = addr; *buf++ = enetc_rd(hw, addr); } + + if (priv->si->hw_features & ENETC_SI_F_QBU) { + for (i = 0; i < ARRAY_SIZE(enetc_port_mm_regs); i++) { + addr = ENETC_PORT_BASE + enetc_port_mm_regs[i]; + *buf++ = addr; + *buf++ = enetc_rd(hw, addr); + } + } } static const struct {
These have been useful in debugging various problems related to frame preemption, so make them available through ethtool --register-dump for later too. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: patch is new .../ethernet/freescale/enetc/enetc_ethtool.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)