RE: [PATCH net-next] net: fec: add CBS offload support

Wei Fang
2023-02-10T07:24:11Z

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