[v1,net-next,1/3] net: dsa: felix: qos classified based on pcp

Set the default QoS Classification based on PCP and DEI of vlan tag,
after that, frames can be Classified to different Qos based on PCP tag.
If there is no vlan tag or vlan ignored, use port default Qos.

Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         |  6 ++++++
 drivers/net/dsa/ocelot/felix.h         |  1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c | 23 +++++++++++++++++++++++
 3 files changed, 30 insertions(+)

Hi Jiri, Jakub, Ido,

On Mon, 11 May 2020 at 08:50, Xiaoliang Yang <xiaoliang.yang_1@nxp.com> wrote:
>
> Set the default QoS Classification based on PCP and DEI of vlan tag,
> after that, frames can be Classified to different Qos based on PCP tag.
> If there is no vlan tag or vlan ignored, use port default Qos.
>
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> ---

The new skbedit priority offload action looks interesting to me.
But it also raises the question of what to do in the default case
where such rules are not installed. I think it is ok to support a
1-to-1 VLAN PCP to TC mapping by default? This should also be needed
for features such as Priority Flow Control.

>  drivers/net/dsa/ocelot/felix.c         |  6 ++++++
>  drivers/net/dsa/ocelot/felix.h         |  1 +
>  drivers/net/dsa/ocelot/felix_vsc9959.c | 23 +++++++++++++++++++++++
>  3 files changed, 30 insertions(+)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index a2dfd73f8a1a..0afdc6fc3f57 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -547,6 +547,12 @@ static int felix_setup(struct dsa_switch *ds)
>                         ocelot_configure_cpu(ocelot, port,
>                                              OCELOT_TAG_PREFIX_NONE,
>                                              OCELOT_TAG_PREFIX_LONG);
> +
> +               /* Set the default QoS Classification based on PCP and DEI
> +                * bits of vlan tag.
> +                */
> +               if (felix->info->port_qos_map_init)
> +                       felix->info->port_qos_map_init(ocelot, port);

Xiaoliang, just a small comment in case you need to resend.
The felix->info structure is intended to hold SoC-specific data that
is likely to differ between chips (like for example if vsc7511 support
ever appears in felix). But I see ANA:PORT:QOS_CFG and
ANA:PORT:QOS_PCP_DEI_MAP_CFG are common registers, so are there any
specific reasons why you put this in felix_vsc9959 and not in the
common ocelot code?

>         }
>
>         /* Include the CPU port module in the forwarding mask for unknown
> diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
> index b94386fa8d63..0d4ec34309c7 100644
> --- a/drivers/net/dsa/ocelot/felix.h
> +++ b/drivers/net/dsa/ocelot/felix.h
> @@ -35,6 +35,7 @@ struct felix_info {
>                                   struct phylink_link_state *state);
>         int     (*prevalidate_phy_mode)(struct ocelot *ocelot, int port,
>                                         phy_interface_t phy_mode);
> +       void    (*port_qos_map_init)(struct ocelot *ocelot, int port);
>  };
>
>  extern struct felix_info               felix_info_vsc9959;
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 1c56568d5aca..5c931fb3e4cd 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -4,6 +4,7 @@
>   */
>  #include <linux/fsl/enetc_mdio.h>
>  #include <soc/mscc/ocelot_vcap.h>
> +#include <soc/mscc/ocelot_ana.h>
>  #include <soc/mscc/ocelot_sys.h>
>  #include <soc/mscc/ocelot.h>
>  #include <linux/iopoll.h>
> @@ -1209,6 +1210,27 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
>         mdiobus_unregister(felix->imdio);
>  }
>
> +static void vsc9959_port_qos_map_init(struct ocelot *ocelot, int port)
> +{
> +       int i;
> +
> +       ocelot_rmw_gix(ocelot,
> +                      ANA_PORT_QOS_CFG_QOS_PCP_ENA,
> +                      ANA_PORT_QOS_CFG_QOS_PCP_ENA,
> +                      ANA_PORT_QOS_CFG,
> +                      port);
> +
> +       for (i = 0; i < FELIX_NUM_TC * 2; i++) {
> +               ocelot_rmw_ix(ocelot,
> +                             (ANA_PORT_PCP_DEI_MAP_DP_PCP_DEI_VAL & i) |

ANA_PORT_PCP_DEI_MAP_DP_PCP_DEI_VAL is 1 bit. Are you sure this should
be % i and not % 2?

> +                             ANA_PORT_PCP_DEI_MAP_QOS_PCP_DEI_VAL(i),
> +                             ANA_PORT_PCP_DEI_MAP_DP_PCP_DEI_VAL |
> +                             ANA_PORT_PCP_DEI_MAP_QOS_PCP_DEI_VAL_M,
> +                             ANA_PORT_PCP_DEI_MAP,
> +                             port, i);
> +       }
> +}
> +
>  struct felix_info felix_info_vsc9959 = {
>         .target_io_res          = vsc9959_target_io_res,
>         .port_io_res            = vsc9959_port_io_res,
> @@ -1232,4 +1254,5 @@ struct felix_info felix_info_vsc9959 = {
>         .pcs_an_restart         = vsc9959_pcs_an_restart,
>         .pcs_link_state         = vsc9959_pcs_link_state,
>         .prevalidate_phy_mode   = vsc9959_prevalidate_phy_mode,
> +       .port_qos_map_init      = vsc9959_port_qos_map_init,
>  };
> --
> 2.17.1
>

Thanks,
-Vladimir

Hi Vladimir,

On Mon, 11 May 2020 at 08:19, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> The new skbedit priority offload action looks interesting to me.
> But it also raises the question of what to do in the default case where such rules are not installed. I think it is ok to support a
> 1-to-1 VLAN PCP to TC mapping by default? This should also be needed for features such as Priority Flow Control.

skbedit priority offload seems only support port based prority set now, I haven't found how to set a priority for each port and QoS. So I set a 1-to-1 VLAN PCP to TC mapping by default.

> Xiaoliang, just a small comment in case you need to resend.
> The felix->info structure is intended to hold SoC-specific data that 
> is likely to differ between chips (like for example if vsc7511 support 
> ever appears in felix). But I see ANA:PORT:QOS_CFG and 
> ANA:PORT:QOS_PCP_DEI_MAP_CFG are common registers, so are there any 
> specific reasons why you put this in felix_vsc9959 and not in the 
> common ocelot code?

All right, I have checked they are common registers, I will move port_qos_map_init() function to felix.c.

> > +       for (i = 0; i < FELIX_NUM_TC * 2; i++) {
> > +               ocelot_rmw_ix(ocelot,
> > +                             (ANA_PORT_PCP_DEI_MAP_DP_PCP_DEI_VAL & 
> > + i) |
> > +								 ANA_PORT_PCP_DEI_MAP_QOS_PCP_DEI_VAL(i),
> > +                             ANA_PORT_PCP_DEI_MAP_DP_PCP_DEI_VAL |
> > +                             ANA_PORT_PCP_DEI_MAP_QOS_PCP_DEI_VAL_M,
> > +                             ANA_PORT_PCP_DEI_MAP,
> > +                             port, i);
> 
> ANA_PORT_PCP_DEI_MAP_DP_PCP_DEI_VAL is 1 bit. Are you sure this should be % i and not % 2?

Because in QOS_PCP_DEI_MAP_CFG register, BIT(3) is DP value, BIT(2, 0) is QOS value. QoS class=QOS_PCP_DEI_MAP_CFG[i].QOS_PC
P_DEI_VAL, i=8*DEI + PCP, so DP value need to be set BIT(3)&i.

Regards,
Xiaoliang Yang