[v1,net-next,3/3] net: dsa: felix: add support Credit Based Shaper(CBS) for hardware offload

VSC9959 hardware support the Credit Based Shaper(CBS) which part
of the IEEE-802.1Qav. This patch support sch_cbs set for VSC9959.

Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c | 52 +++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

On Mon, 11 May 2020 13:43:32 +0800 Xiaoliang Yang wrote:
> +int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
> +			     struct tc_cbs_qopt_offload *cbs_qopt)

static

Xiaoliang Yang <xiaoliang.yang_1@nxp.com> writes:

> VSC9959 hardware support the Credit Based Shaper(CBS) which part
> of the IEEE-802.1Qav. This patch support sch_cbs set for VSC9959.
>
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> ---
>  drivers/net/dsa/ocelot/felix_vsc9959.c | 52 +++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index ccbd875c7a47..d8d1657ee8ba 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -208,7 +208,7 @@ static const u32 vsc9959_qsys_regmap[] = {
>  	REG(QSYS_QMAXSDU_CFG_6,			0x00f62c),
>  	REG(QSYS_QMAXSDU_CFG_7,			0x00f648),
>  	REG(QSYS_PREEMPTION_CFG,		0x00f664),
> -	REG_RESERVED(QSYS_CIR_CFG),
> +	REG(QSYS_CIR_CFG,			0x000000),
>  	REG(QSYS_EIR_CFG,			0x000004),
>  	REG(QSYS_SE_CFG,			0x000008),
>  	REG(QSYS_SE_DWRR_CFG,			0x00000c),
> @@ -1354,6 +1354,54 @@ static int vsc9959_qos_port_tas_set(struct ocelot *ocelot, int port,
>  	return ret;
>  }
>  
> +int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
> +			     struct tc_cbs_qopt_offload *cbs_qopt)
> +{
> +	struct ocelot *ocelot = ds->priv;
> +	int port_ix = port * 8 + cbs_qopt->queue;
> +	u32 cbs = 0;
> +	u32 cir = 0;
> +
> +	if (cbs_qopt->queue >= ds->num_tx_queues)
> +		return -EINVAL;
> +
> +	if (!cbs_qopt->enable) {
> +		ocelot_write_gix(ocelot, QSYS_CIR_CFG_CIR_RATE(0) |
> +				 QSYS_CIR_CFG_CIR_BURST(0),
> +				 QSYS_CIR_CFG, port_ix);
> +
> +		ocelot_rmw_gix(ocelot, 0, QSYS_SE_CFG_SE_AVB_ENA,
> +			       QSYS_SE_CFG, port_ix);
> +
> +		return 0;
> +	}
> +
> +	/* Rate unit is 100 kbps */
> +	cir = DIV_ROUND_UP(cbs_qopt->idleslope, 100);
> +	cir = (cir ? cir : 1);
> +	cir = min_t(u32, GENMASK(14, 0), cir);

Please rename 'cir' to "rate" or "idleslope".

Also consider using clamp_t here and below (I just found out about it).

> +	/* Burst unit is 4kB */
> +	cbs = DIV_ROUND_UP(cbs_qopt->hicredit, 4096);
> +	/* Avoid using zero burst size */
> +	cbs = (cbs ? cbs : 1);
> +	cbs = min_t(u32, GENMASK(5, 0), cbs);

And please(!) rename 'cbs' to "burst" or "hicredit". Re-using the name
"cbs" with a completely different meaning here is confusing.

> +	ocelot_write_gix(ocelot,
> +			 QSYS_CIR_CFG_CIR_RATE(cir) |
> +			 QSYS_CIR_CFG_CIR_BURST(cbs),
> +			 QSYS_CIR_CFG,
> +			 port_ix);
> +
> +	ocelot_rmw_gix(ocelot,
> +		       QSYS_SE_CFG_SE_FRM_MODE(0) |
> +		       QSYS_SE_CFG_SE_AVB_ENA,
> +		       QSYS_SE_CFG_SE_AVB_ENA |
> +		       QSYS_SE_CFG_SE_FRM_MODE_M,
> +		       QSYS_SE_CFG,
> +		       port_ix);
> +
> +	return 0;
> +}
> +
>  static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port,
>  				 enum tc_setup_type type,
>  				 void *type_data)
> @@ -1363,6 +1411,8 @@ static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port,
>  	switch (type) {
>  	case TC_SETUP_QDISC_TAPRIO:
>  		return vsc9959_qos_port_tas_set(ocelot, port, type_data);
> +	case TC_SETUP_QDISC_CBS:
> +		return vsc9959_qos_port_cbs_set(ds, port, type_data);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> -- 
> 2.17.1
>

Hi Jakub,

On Mon, 11 May 2020 22:34:32 Jakub Kicinski <kuba@kernel.org> wrote:
> > +int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
> > +                          struct tc_cbs_qopt_offload *cbs_qopt)
>
> static

I will update this in v2, thanks.

Regards,
Xiaoliang Yang

Hi Vinicius,


On Tue, 12 May 2020 9:42:23 Vinicius Costa Gomes wrote:
> > +
> > +     /* Rate unit is 100 kbps */
> > +     cir = DIV_ROUND_UP(cbs_qopt->idleslope, 100);
> > +     cir = (cir ? cir : 1);
> > +     cir = min_t(u32, GENMASK(14, 0), cir);
>
> Please rename 'cir' to "rate" or "idleslope".
>
> Also consider using clamp_t here and below (I just found out about it).
>
> > +     /* Burst unit is 4kB */
> > +     cbs = DIV_ROUND_UP(cbs_qopt->hicredit, 4096);
> > +     /* Avoid using zero burst size */
> > +     cbs = (cbs ? cbs : 1);
> > +     cbs = min_t(u32, GENMASK(5, 0), cbs);
> 
> And please(!) rename 'cbs' to "burst" or "hicredit". Re-using the name "cbs" with a completely different meaning here is confusing.
> 
I will update this, using clamp_t seems more concise in the codes.

Regards,
Xiaoliang