| Message ID | 20240405095216.353829-9-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Enhanced DCB and DSCP Support for KSZ Switches | expand |
Hi Oleksij, > +static int ksz9477_set_default_prio_queue_mapping(struct ksz_device > *dev, > + int port) > +{ > + u32 queue_map = 0; > + int ipv; Just a suggestion for readability: if you can use common variable for counter variable like cnt or n similar to return variable ret, it will be easier to follow instead of ipv here, queue in other function. > + > + for (ipv = 0; ipv < dev->info->num_ipvs; ipv++) { > + int queue; > + > + /* Traffic Type (TT) is corresponding to the Internal > Priority > + * Value (IPV) in the switch. Traffic Class (TC) is > + * corresponding to the queue in the switch. > + */ > + queue = ieee8021q_tt_to_tc(ipv, dev->info- > >num_tx_queues); > + if (queue < 0) > + return queue; what happens if the num_tx_queues value passed is other than 1 to 8. For ex: if it is 9, then ieee8021q_tt_to_tc( ) generates pr_warn as invalid number of queue and return queue as 0. if will not execute, so queue_map will be updated. Do we need check for valid range of values instead of just queue < 0. Not sure this scenario can occur. > + > + queue_map |= queue << (ipv * KSZ9477_PORT_TC_MAP_S); > + } > + > + return ksz_pwrite32(dev, port, KSZ9477_PORT_MRI_TC_MAP__4, > queue_map); > +} > + >
Hi Arun, On Mon, Apr 08, 2024 at 03:41:46AM +0000, Arun.Ramadoss@microchip.com wrote: > Hi Oleksij, > > > +static int ksz9477_set_default_prio_queue_mapping(struct ksz_device > > *dev, > > + int port) > > +{ > > + u32 queue_map = 0; > > + int ipv; > > Just a suggestion for readability: if you can use common variable for > counter variable like cnt or n similar to return variable ret, it will > be easier to follow instead of ipv here, queue in other function. Sorry, I disagree here. Variable name should describe the purpose of it, especially if things got complicated. If other function uses "queue" as variable name but actually ipv is the meaning of it, then it should be renamed. > > + > > + for (ipv = 0; ipv < dev->info->num_ipvs; ipv++) { > > + int queue; > > + > > + /* Traffic Type (TT) is corresponding to the Internal > > Priority > > + * Value (IPV) in the switch. Traffic Class (TC) is > > + * corresponding to the queue in the switch. > > + */ > > + queue = ieee8021q_tt_to_tc(ipv, dev->info- > > >num_tx_queues); > > + if (queue < 0) > > + return queue; > > what happens if the num_tx_queues value passed is other than 1 to 8. > For ex: if it is 9, then ieee8021q_tt_to_tc( ) generates pr_warn as > invalid number of queue and return queue as 0. if will not execute, so > queue_map will be updated. Do we need check for valid range of values > instead of just queue < 0. > > Not sure this scenario can occur. Good point, I should fix ieee8021q_tt_to_tc(), it should return -EINVAL instead.
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index dc96931e62da8..3f7977a423407 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -24,6 +24,7 @@ #include <linux/of_net.h> #include <linux/micrel_phy.h> #include <net/dsa.h> +#include <net/ieee8021q.h> #include <net/pkt_cls.h> #include <net/switchdev.h> @@ -2685,9 +2686,33 @@ static int ksz_port_mdb_del(struct dsa_switch *ds, int port, return dev->dev_ops->mdb_del(dev, port, mdb, db); } +static int ksz9477_set_default_prio_queue_mapping(struct ksz_device *dev, + int port) +{ + u32 queue_map = 0; + int ipv; + + for (ipv = 0; ipv < dev->info->num_ipvs; ipv++) { + int queue; + + /* Traffic Type (TT) is corresponding to the Internal Priority + * Value (IPV) in the switch. Traffic Class (TC) is + * corresponding to the queue in the switch. + */ + queue = ieee8021q_tt_to_tc(ipv, dev->info->num_tx_queues); + if (queue < 0) + return queue; + + queue_map |= queue << (ipv * KSZ9477_PORT_TC_MAP_S); + } + + return ksz_pwrite32(dev, port, KSZ9477_PORT_MRI_TC_MAP__4, queue_map); +} + static int ksz_port_setup(struct dsa_switch *ds, int port) { struct ksz_device *dev = ds->priv; + int ret; if (!dsa_is_user_port(ds, port)) return 0; @@ -2695,6 +2720,12 @@ static int ksz_port_setup(struct dsa_switch *ds, int port) /* setup user port */ dev->dev_ops->port_setup(dev, port, false); + if (!is_ksz8(dev)) { + ret = ksz9477_set_default_prio_queue_mapping(dev, port); + if (ret) + return ret; + } + /* port_stp_state_set() will be called after to enable the port so * there is no need to do anything. */ @@ -3559,8 +3590,7 @@ static int ksz_tc_ets_add(struct ksz_device *dev, int port, static int ksz_tc_ets_del(struct ksz_device *dev, int port) { - int ret, queue, tc_prio, s; - u32 queue_map = 0; + int ret, queue; /* To restore the default chip configuration, set all queues to use the * WRR scheduler with a weight of 1. @@ -3572,31 +3602,10 @@ static int ksz_tc_ets_del(struct ksz_device *dev, int port) return ret; } - switch (dev->info->num_tx_queues) { - case 2: - s = 2; - break; - case 4: - s = 1; - break; - case 8: - s = 0; - break; - default: - return -EINVAL; - } - /* Revert the queue mapping for TC-priority to its default setting on * the chip. */ - for (tc_prio = 0; tc_prio < dev->info->num_ipvs; tc_prio++) { - int queue; - - queue = tc_prio >> s; - queue_map |= queue << (tc_prio * KSZ9477_PORT_TC_MAP_S); - } - - return ksz_pwrite32(dev, port, KSZ9477_PORT_MRI_TC_MAP__4, queue_map); + return ksz9477_set_default_prio_queue_mapping(dev, port); } static int ksz_tc_ets_validate(struct ksz_device *dev, int port,
Init priority to queue mapping in the way as it shown in IEEE 802.1Q mapping example. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- changes v2: - s/ksz_set_default_prio_queue_mapping/ksz9477_set_default_prio_queue_mapping - remove error on queue < 0. --- drivers/net/dsa/microchip/ksz_common.c | 57 +++++++++++++++----------- 1 file changed, 33 insertions(+), 24 deletions(-)