Messages in this thread | | | From | Wei Fang <> | Subject | RE: [PATCH net-next] net: fec: add CBS offload support | Date | Fri, 10 Feb 2023 07:24:03 +0000 |
| |
> -----Original Message----- > From: Simon Horman <simon.horman@corigine.com> > Sent: 2023年2月10日 0:14 > To: Wei Fang <wei.fang@nxp.com> > Cc: Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang > <xiaoning.wang@nxp.com>; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next] net: fec: add CBS offload support > > On Thu, Feb 09, 2023 at 05:24:22PM +0800, wei.fang@nxp.com wrote: > > From: Wei Fang <wei.fang@nxp.com> > > > > + > > + if (idle_slope >= 128) { > > + /* For values equal to or greater than 128, idle_slope = 128 * m, > > + * where m = 1, 2, 3, ...12. Here we use the rounding method. > > + */ > > Perhaps the following would be clearer? > > For values greater than or equal to 128, > idle_slope is rounded to the nearest multiple of 128. > > > + quotient = idle_slope / 128; > > + if (idle_slope >= quotient * 128 + 64) > > + idle_slope = 128 * (quotient + 1); > > + else > > + idle_slope = 128 * quotient; > > Maybe there is a helper that does this, but if > not, perhaps: > > idle_slope = DIV_ROUND_CLOSEST(idle_slope, 128U) * 128U; > > > > + > > + goto end; > > Maybe return here > > > + } > > Or an else here is nicer? > > > + > > + /* For values less than 128, idle_slope = 2 ^ n, where > > Perhaps the following would be clearer? > > For values less than 128, idle_slope is rounded > to the nearest power of 2. > > > + * n = 0, 1, 2, ...6. Here we use the rounding method. > > n is 7 for input idle_slope around 128 (2^7) > > > + * So the minimum of idle_slope is 1. > > + */ > > + msb = fls(idle_slope); > > + > > + if (msb == 0 || msb == 1) { > > + idle_slope = 1; > > + goto end; > > + } > > nit: maybe this is nicer > > if (msb <= 1) > return 1; > > > + > > + msb -= 1; > > + if (idle_slope >= (1 << msb) + (1 << (msb - 1))) > > + idle_slope = 1 << (msb + 1); > > + else > > + idle_slope = 1 << msb; > > In the same vein as the suggestion for the >= 128 case, perhaps: > > u32 d; > > d = BIT(fls(idle_slope)); > idle_slope = DIV_ROUND_CLOSEST(idle_slope, d) * d; > > > + > > +end: > > + return idle_slope; > > +} > > + > > +static int fec_enet_setup_tc_cbs(struct net_device *ndev, void > > +*type_data) { > > + struct fec_enet_private *fep = netdev_priv(ndev); > > + struct tc_cbs_qopt_offload *cbs = type_data; > > + int queue = cbs->queue; > > nit: extra space after '=' > > > + u32 speed = fep->speed; > > + u32 val, idle_slope; > > + u8 bw; > > + > > + if (!(fep->quirks & FEC_QUIRK_HAS_AVB)) > > + return -EOPNOTSUPP; > > + > > + /* Queue 1 for Class A, Queue 2 for Class B, so the ENET must has > > nit: s/has/have/ > > > + * three queues. > > + */ > > + if (fep->num_tx_queues != FEC_ENET_MAX_TX_QS) > > + return -EOPNOTSUPP; > > + > > + /* Queue 0 is not AVB capable */ > > + if (queue <= 0 || queue >= fep->num_tx_queues) > > + return -EINVAL; > > + > > + val = readl(fep->hwp + FEC_QOS_SCHEME); > > + val &= ~FEC_QOS_TX_SHEME_MASK; > > + if (!cbs->enable) { > > + val |= ROUND_ROBIN_SCHEME; > > + writel(val, fep->hwp + FEC_QOS_SCHEME); > > + > > + return 0; > > + } > > + > > + val |= CREDIT_BASED_SCHEME; > > + writel(val, fep->hwp + FEC_QOS_SCHEME); > > + > > + /* cbs->idleslope is in kilobits per second. speed is the port rate > > + * in megabits per second. So bandwidth ratio bw = (idleslope / > > + * (speed * 1000)) * 100, the unit is percentage. > > + */ > > suggestion: > > /* cbs->idleslope is in kilobits per second. > * Speed is the port rate in megabits per second. > * So bandwidth the ratio, bw, is (idleslope / (speed * 1000)) * 100. > * The unit of bw is a percentage. > */ > > > + bw = cbs->idleslope / (speed * 10UL); > > + /* bw% can not >= 100% */ > > + if (bw >= 100) > > + return -EINVAL; > > nit: maybe the above calculation and check fits better inside > fec_enet_get_idle_slope() >
Your suggestions are very helpful, I'll amend the patch in v2. Thanks a lot.
| |