| Message ID | 1612950500-9682-13-git-send-email-stefanc@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: mvpp2: Add TX Flow Control support | expand |
From: <stefanc@marvell.com> Date: Wed, 10 Feb 2021 11:48:17 +0200 > > +static int bm_underrun_protect = 1; > + > +module_param(bm_underrun_protect, int, 0444); > +MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect feature (0-1), def=1"); No new module parameters, please.
> > ---------------------------------------------------------------------- > From: <stefanc@marvell.com> > Date: Wed, 10 Feb 2021 11:48:17 +0200 > > > > > +static int bm_underrun_protect = 1; > > + > > +module_param(bm_underrun_protect, int, 0444); > > +MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect > > +feature (0-1), def=1"); > > No new module parameters, please. Ok, I would remove new module parameters. By the way why new module parameters forbitten? Thanks, Stefan
On Thu, Feb 11, 2021 at 08:22:19AM +0000, Stefan Chulski wrote: > > > > > ---------------------------------------------------------------------- > > From: <stefanc@marvell.com> > > Date: Wed, 10 Feb 2021 11:48:17 +0200 > > > > > > > > +static int bm_underrun_protect = 1; > > > + > > > +module_param(bm_underrun_protect, int, 0444); > > > +MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect > > > +feature (0-1), def=1"); > > > > No new module parameters, please. > > Ok, I would remove new module parameters. > By the way why new module parameters forbitten? Historically, module parameters are a bad interface for configuration. Vendors have stuffed all sorts of random junk into module parameters. There is little documentation. Different drivers can have similar looking module parameters which do different things. Or different module parameters, which actually do the same thing. But maybe with slightly different parameters. We get a much better overall result if you stop and think for a while. How can this be made a generic configuration knob which multiple vendors could use? And then add it to ethtool. Extend the ethtool -h text and the man page. Maybe even hack some other vendors driver to make use of it. Or we have also found out, that pushing back on parameters like this, the developers goes back and looks at the code, and sometimes figures out a way to automatically do the right thing, removing the configuration knob, and just making it all simpler for the user to use. Andrew
Hi, czw., 11 lut 2021 o 15:19 Andrew Lunn <andrew@lunn.ch> napisał(a): > > On Thu, Feb 11, 2021 at 08:22:19AM +0000, Stefan Chulski wrote: > > > > > > > > ---------------------------------------------------------------------- > > > From: <stefanc@marvell.com> > > > Date: Wed, 10 Feb 2021 11:48:17 +0200 > > > > > > > > > > > +static int bm_underrun_protect = 1; > > > > + > > > > +module_param(bm_underrun_protect, int, 0444); > > > > +MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect > > > > +feature (0-1), def=1"); > > > > > > No new module parameters, please. > > > > Ok, I would remove new module parameters. > > By the way why new module parameters forbitten? > > Historically, module parameters are a bad interface for > configuration. Vendors have stuffed all sorts of random junk into > module parameters. There is little documentation. Different drivers > can have similar looking module parameters which do different > things. Or different module parameters, which actually do the same > thing. But maybe with slightly different parameters. > > We get a much better overall result if you stop and think for a > while. How can this be made a generic configuration knob which > multiple vendors could use? And then add it to ethtool. Extend the > ethtool -h text and the man page. Maybe even hack some other vendors > driver to make use of it. > > Or we have also found out, that pushing back on parameters like this, > the developers goes back and looks at the code, and sometimes figures > out a way to automatically do the right thing, removing the > configuration knob, and just making it all simpler for the user to > use. I think of 2 alternatives: * `ethtool --set-priv-flags` - in such case there is a question if switching this particular feature in runtime is a good idea. * New DT/ACPI property - it is a hardware feature after all, so maybe let the user decide whether to enable it on the platform description level. What do you think? Best regards, Marcin
> > Or we have also found out, that pushing back on parameters like this, > > the developers goes back and looks at the code, and sometimes figures > > out a way to automatically do the right thing, removing the > > configuration knob, and just making it all simpler for the user to > > use. > > I think of 2 alternatives: > * `ethtool --set-priv-flags` - in such case there is a question if > switching this particular feature in runtime is a good idea. > * New DT/ACPI property - it is a hardware feature after all, so maybe > let the user decide whether to enable it on the platform description > level. Does this even need to be configurable? What is the cost of turning it on? How does having less pools affect the system? Does average latency go up? When would i consider an underrun actually a good thing? Maybe it should just be hard coded on? Or we should try to detect when underruns are happening a lot, and dynamically turn it on for a while? Andrew
> > > Or we have also found out, that pushing back on parameters like > > > this, the developers goes back and looks at the code, and sometimes > > > figures out a way to automatically do the right thing, removing the > > > configuration knob, and just making it all simpler for the user to > > > use. > > > > I think of 2 alternatives: > > * `ethtool --set-priv-flags` - in such case there is a question if > > switching this particular feature in runtime is a good idea. > > * New DT/ACPI property - it is a hardware feature after all, so maybe > > let the user decide whether to enable it on the platform description > > level. > > Does this even need to be configurable? What is the cost of turning it on? > How does having less pools affect the system? Does average latency go up? > When would i consider an underrun actually a good thing? > > Maybe it should just be hard coded on? Or we should try to detect when > underruns are happening a lot, and dynamically turn it on for a while? > > Andrew The cost of this change is that the number of pools reduced from 16 to 8. The current driver uses only 4pools, but some future features like QoS can use over 4 pools. Regards, Stefan.
> > Does this even need to be configurable? What is the cost of turning it on? > > How does having less pools affect the system? Does average latency go up? > > When would i consider an underrun actually a good thing? > > > > Maybe it should just be hard coded on? Or we should try to detect when > > underruns are happening a lot, and dynamically turn it on for a while? > > > The cost of this change is that the number of pools reduced from 16 to 8. > The current driver uses only 4pools, but some future features like QoS can use over 4 pools. So you are saying, there is currently no cost for turning it on. So it seems like you should just turn it on, and forget the module parameter. When QoS features are added which require more than 8 pools you can then address the issue of if this should be configurable. Andrew
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h index 0731dc7..9b525b60 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h @@ -324,6 +324,10 @@ #define MVPP2_BM_HIGH_THRESH_MASK 0x7f0000 #define MVPP2_BM_HIGH_THRESH_VALUE(val) ((val) << \ MVPP2_BM_HIGH_THRESH_OFFS) +#define MVPP2_BM_BPPI_HIGH_THRESH 0x1E +#define MVPP2_BM_BPPI_LOW_THRESH 0x1C +#define MVPP23_BM_BPPI_HIGH_THRESH 0x34 +#define MVPP23_BM_BPPI_LOW_THRESH 0x28 #define MVPP2_BM_INTR_CAUSE_REG(pool) (0x6240 + ((pool) * 4)) #define MVPP2_BM_RELEASED_DELAY_MASK BIT(0) #define MVPP2_BM_ALLOC_FAILED_MASK BIT(1) @@ -352,6 +356,10 @@ #define MVPP2_OVERRUN_ETH_DROP 0x7000 #define MVPP2_CLS_ETH_DROP 0x7020 +#define MVPP22_BM_POOL_BASE_ADDR_HIGH_REG 0x6310 +#define MVPP22_BM_POOL_BASE_ADDR_HIGH_MASK 0xff +#define MVPP23_BM_8POOL_MODE BIT(8) + /* Hit counters registers */ #define MVPP2_CTRS_IDX 0x7040 #define MVPP22_CTRS_TX_CTR(port, txq) ((txq) | ((port) << 3) | BIT(7)) diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 90c9265..3faad04 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -69,6 +69,11 @@ enum mvpp2_bm_pool_log_num { module_param(queue_mode, int, 0444); MODULE_PARM_DESC(queue_mode, "Set queue_mode (single=0, multi=1)"); +static int bm_underrun_protect = 1; + +module_param(bm_underrun_protect, int, 0444); +MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect feature (0-1), def=1"); + /* Utility/helper methods */ void mvpp2_write(struct mvpp2 *priv, u32 offset, u32 data) @@ -423,6 +428,21 @@ static int mvpp2_bm_pool_create(struct device *dev, struct mvpp2 *priv, val = mvpp2_read(priv, MVPP2_BM_POOL_CTRL_REG(bm_pool->id)); val |= MVPP2_BM_START_MASK; + + val &= ~MVPP2_BM_LOW_THRESH_MASK; + val &= ~MVPP2_BM_HIGH_THRESH_MASK; + + /* Set 8 Pools BPPI threshold if BM underrun protection feature + * was enabled + */ + if (priv->hw_version == MVPP23 && bm_underrun_protect) { + val |= MVPP2_BM_LOW_THRESH_VALUE(MVPP23_BM_BPPI_LOW_THRESH); + val |= MVPP2_BM_HIGH_THRESH_VALUE(MVPP23_BM_BPPI_HIGH_THRESH); + } else { + val |= MVPP2_BM_LOW_THRESH_VALUE(MVPP2_BM_BPPI_LOW_THRESH); + val |= MVPP2_BM_HIGH_THRESH_VALUE(MVPP2_BM_BPPI_HIGH_THRESH); + } + mvpp2_write(priv, MVPP2_BM_POOL_CTRL_REG(bm_pool->id), val); bm_pool->size = size; @@ -591,6 +611,16 @@ static int mvpp2_bm_pools_init(struct device *dev, struct mvpp2 *priv) return err; } +/* Routine enable PPv23 8 pool mode */ +static void mvpp23_bm_set_8pool_mode(struct mvpp2 *priv) +{ + int val; + + val = mvpp2_read(priv, MVPP22_BM_POOL_BASE_ADDR_HIGH_REG); + val |= MVPP23_BM_8POOL_MODE; + mvpp2_write(priv, MVPP22_BM_POOL_BASE_ADDR_HIGH_REG, val); +} + static int mvpp2_bm_init(struct device *dev, struct mvpp2 *priv) { enum dma_data_direction dma_dir = DMA_FROM_DEVICE; @@ -644,6 +674,9 @@ static int mvpp2_bm_init(struct device *dev, struct mvpp2 *priv) if (!priv->bm_pools) return -ENOMEM; + if (priv->hw_version == MVPP23 && bm_underrun_protect) + mvpp23_bm_set_8pool_mode(priv); + err = mvpp2_bm_pools_init(dev, priv); if (err < 0) return err; @@ -6490,7 +6523,7 @@ static void mvpp2_mac_link_up(struct phylink_config *config, val); } - if (port->priv->global_tx_fc) { + if (port->priv->global_tx_fc && bm_underrun_protect) { port->tx_fc = tx_pause; if (tx_pause) mvpp2_rxq_enable_fc(port);