2024-05-13 15:12:46

by Andrew Halaney

[permalink] [raw]
Subject: Re: [net-next PATCH v6 2/2] net: stmmac: move the EST structure to struct stmmac_priv

On Mon, May 13, 2024 at 09:43:46AM GMT, Xiaolei Wang wrote:
> Move the EST structure to struct stmmac_priv, because the
> EST configs don't look like platform config, but EST is
> enabled in runtime with the settings retrieved for the TC
> TAPRIO feature also in runtime. So it's better to have the
> EST-data preserved in the driver private data instead of
> the platform data storage.
>
> Signed-off-by: Xiaolei Wang <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> Reviewed-by: Serge Semin <[email protected]>

Reviewed-by: Andrew Halaney <[email protected]>

> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 15 +++++++
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 ++++-----
> .../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 22 +++++-----
> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 40 +++++++++----------
> include/linux/stmmac.h | 15 -------
> 5 files changed, 54 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 64b21c83e2b8..011683abf97f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -221,6 +221,20 @@ struct stmmac_dma_conf {
> unsigned int dma_tx_size;
> };
>
> +#define EST_GCL 1024
> +struct stmmac_est {
> + int enable;
> + u32 btr_reserve[2];
> + u32 btr_offset[2];
> + u32 btr[2];
> + u32 ctr[2];
> + u32 ter;
> + u32 gcl_unaligned[EST_GCL];
> + u32 gcl[EST_GCL];
> + u32 gcl_size;
> + u32 max_sdu[MTL_MAX_TX_QUEUES];
> +};
> +
> struct stmmac_priv {
> /* Frequently used values are kept adjacent for cache effect */
> u32 tx_coal_frames[MTL_MAX_TX_QUEUES];
> @@ -263,6 +277,7 @@ struct stmmac_priv {
> struct plat_stmmacenet_data *plat;
> /* Protect est parameters */
> struct mutex est_lock;
> + struct stmmac_est *est;
> struct dma_features dma_cap;
> struct stmmac_counters mmc;
> int hw_cap_support;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 7c6fb14b5555..0eafd609bf53 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2491,9 +2491,9 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> if (!xsk_tx_peek_desc(pool, &xdp_desc))
> break;
>
> - if (priv->plat->est && priv->plat->est->enable &&
> - priv->plat->est->max_sdu[queue] &&
> - xdp_desc.len > priv->plat->est->max_sdu[queue]) {
> + if (priv->est && priv->est->enable &&
> + priv->est->max_sdu[queue] &&
> + xdp_desc.len > priv->est->max_sdu[queue]) {
> priv->xstats.max_sdu_txq_drop[queue]++;
> continue;
> }
> @@ -4528,9 +4528,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> return stmmac_tso_xmit(skb, dev);
> }
>
> - if (priv->plat->est && priv->plat->est->enable &&
> - priv->plat->est->max_sdu[queue] &&
> - skb->len > priv->plat->est->max_sdu[queue]){
> + if (priv->est && priv->est->enable &&
> + priv->est->max_sdu[queue] &&
> + skb->len > priv->est->max_sdu[queue]){
> priv->xstats.max_sdu_txq_drop[queue]++;
> goto max_sdu_err;
> }
> @@ -4909,9 +4909,9 @@ static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
> if (stmmac_tx_avail(priv, queue) < STMMAC_TX_THRESH(priv))
> return STMMAC_XDP_CONSUMED;
>
> - if (priv->plat->est && priv->plat->est->enable &&
> - priv->plat->est->max_sdu[queue] &&
> - xdpf->len > priv->plat->est->max_sdu[queue]) {
> + if (priv->est && priv->est->enable &&
> + priv->est->max_sdu[queue] &&
> + xdpf->len > priv->est->max_sdu[queue]) {
> priv->xstats.max_sdu_txq_drop[queue]++;
> return STMMAC_XDP_CONSUMED;
> }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 0c5aab6dd7a7..a6b1de9a251d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -68,11 +68,11 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> nsec = reminder;
>
> /* If EST is enabled, disabled it before adjust ptp time. */
> - if (priv->plat->est && priv->plat->est->enable) {
> + if (priv->est && priv->est->enable) {
> est_rst = true;
> mutex_lock(&priv->est_lock);
> - priv->plat->est->enable = false;
> - stmmac_est_configure(priv, priv, priv->plat->est,
> + priv->est->enable = false;
> + stmmac_est_configure(priv, priv, priv->est,
> priv->plat->clk_ptp_rate);
> mutex_unlock(&priv->est_lock);
> }
> @@ -90,19 +90,19 @@ static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> mutex_lock(&priv->est_lock);
> priv->ptp_clock_ops.gettime64(&priv->ptp_clock_ops, &current_time);
> current_time_ns = timespec64_to_ktime(current_time);
> - time.tv_nsec = priv->plat->est->btr_reserve[0];
> - time.tv_sec = priv->plat->est->btr_reserve[1];
> + time.tv_nsec = priv->est->btr_reserve[0];
> + time.tv_sec = priv->est->btr_reserve[1];
> basetime = timespec64_to_ktime(time);
> - cycle_time = (u64)priv->plat->est->ctr[1] * NSEC_PER_SEC +
> - priv->plat->est->ctr[0];
> + cycle_time = (u64)priv->est->ctr[1] * NSEC_PER_SEC +
> + priv->est->ctr[0];
> time = stmmac_calc_tas_basetime(basetime,
> current_time_ns,
> cycle_time);
>
> - priv->plat->est->btr[0] = (u32)time.tv_nsec;
> - priv->plat->est->btr[1] = (u32)time.tv_sec;
> - priv->plat->est->enable = true;
> - ret = stmmac_est_configure(priv, priv, priv->plat->est,
> + priv->est->btr[0] = (u32)time.tv_nsec;
> + priv->est->btr[1] = (u32)time.tv_sec;
> + priv->est->enable = true;
> + ret = stmmac_est_configure(priv, priv, priv->est,
> priv->plat->clk_ptp_rate);
> mutex_unlock(&priv->est_lock);
> if (ret)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 620c16e9be3a..222540b55480 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -918,7 +918,6 @@ struct timespec64 stmmac_calc_tas_basetime(ktime_t old_base_time,
> static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
> struct tc_taprio_qopt_offload *qopt)
> {
> - struct plat_stmmacenet_data *plat = priv->plat;
> u32 num_tc = qopt->mqprio.qopt.num_tc;
> u32 offset, count, i, j;
>
> @@ -933,7 +932,7 @@ static void tc_taprio_map_maxsdu_txq(struct stmmac_priv *priv,
> count = qopt->mqprio.qopt.count[i];
>
> for (j = offset; j < offset + count; j++)
> - plat->est->max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;
> + priv->est->max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;
> }
> }
>
> @@ -941,7 +940,6 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> struct tc_taprio_qopt_offload *qopt)
> {
> u32 size, wid = priv->dma_cap.estwid, dep = priv->dma_cap.estdep;
> - struct plat_stmmacenet_data *plat = priv->plat;
> struct timespec64 time, current_time, qopt_time;
> ktime_t current_time_ns;
> bool fpe = false;
> @@ -998,24 +996,24 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> if (qopt->cycle_time_extension >= BIT(wid + 7))
> return -ERANGE;
>
> - if (!plat->est) {
> - plat->est = devm_kzalloc(priv->device, sizeof(*plat->est),
> + if (!priv->est) {
> + priv->est = devm_kzalloc(priv->device, sizeof(*priv->est),
> GFP_KERNEL);
> - if (!plat->est)
> + if (!priv->est)
> return -ENOMEM;
>
> mutex_init(&priv->est_lock);
> } else {
> mutex_lock(&priv->est_lock);
> - memset(plat->est, 0, sizeof(*plat->est));
> + memset(priv->est, 0, sizeof(*priv->est));
> mutex_unlock(&priv->est_lock);
> }
>
> size = qopt->num_entries;
>
> mutex_lock(&priv->est_lock);
> - priv->plat->est->gcl_size = size;
> - priv->plat->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
> + priv->est->gcl_size = size;
> + priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
> mutex_unlock(&priv->est_lock);
>
> for (i = 0; i < size; i++) {
> @@ -1044,7 +1042,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> return -EOPNOTSUPP;
> }
>
> - priv->plat->est->gcl[i] = delta_ns | (gates << wid);
> + priv->est->gcl[i] = delta_ns | (gates << wid);
> }
>
> mutex_lock(&priv->est_lock);
> @@ -1054,18 +1052,18 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> time = stmmac_calc_tas_basetime(qopt->base_time, current_time_ns,
> qopt->cycle_time);
>
> - priv->plat->est->btr[0] = (u32)time.tv_nsec;
> - priv->plat->est->btr[1] = (u32)time.tv_sec;
> + priv->est->btr[0] = (u32)time.tv_nsec;
> + priv->est->btr[1] = (u32)time.tv_sec;
>
> qopt_time = ktime_to_timespec64(qopt->base_time);
> - priv->plat->est->btr_reserve[0] = (u32)qopt_time.tv_nsec;
> - priv->plat->est->btr_reserve[1] = (u32)qopt_time.tv_sec;
> + priv->est->btr_reserve[0] = (u32)qopt_time.tv_nsec;
> + priv->est->btr_reserve[1] = (u32)qopt_time.tv_sec;
>
> ctr = qopt->cycle_time;
> - priv->plat->est->ctr[0] = do_div(ctr, NSEC_PER_SEC);
> - priv->plat->est->ctr[1] = (u32)ctr;
> + priv->est->ctr[0] = do_div(ctr, NSEC_PER_SEC);
> + priv->est->ctr[1] = (u32)ctr;
>
> - priv->plat->est->ter = qopt->cycle_time_extension;
> + priv->est->ter = qopt->cycle_time_extension;
>
> tc_taprio_map_maxsdu_txq(priv, qopt);
>
> @@ -1079,7 +1077,7 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> */
> priv->plat->fpe_cfg->enable = fpe;
>
> - ret = stmmac_est_configure(priv, priv, priv->plat->est,
> + ret = stmmac_est_configure(priv, priv, priv->est,
> priv->plat->clk_ptp_rate);
> mutex_unlock(&priv->est_lock);
> if (ret) {
> @@ -1097,10 +1095,10 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
> return 0;
>
> disable:
> - if (priv->plat->est) {
> + if (priv->est) {
> mutex_lock(&priv->est_lock);
> - priv->plat->est->enable = false;
> - stmmac_est_configure(priv, priv, priv->plat->est,
> + priv->est->enable = false;
> + stmmac_est_configure(priv, priv, priv->est,
> priv->plat->clk_ptp_rate);
> /* Reset taprio status */
> for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index c0d74f97fd18..5da45d025601 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -115,20 +115,6 @@ struct stmmac_axi {
> bool axi_rb;
> };
>
> -#define EST_GCL 1024
> -struct stmmac_est {
> - int enable;
> - u32 btr_reserve[2];
> - u32 btr_offset[2];
> - u32 btr[2];
> - u32 ctr[2];
> - u32 ter;
> - u32 gcl_unaligned[EST_GCL];
> - u32 gcl[EST_GCL];
> - u32 gcl_size;
> - u32 max_sdu[MTL_MAX_TX_QUEUES];
> -};
> -
> struct stmmac_rxq_cfg {
> u8 mode_to_use;
> u32 chan;
> @@ -245,7 +231,6 @@ struct plat_stmmacenet_data {
> struct fwnode_handle *port_node;
> struct device_node *mdio_node;
> struct stmmac_dma_cfg *dma_cfg;
> - struct stmmac_est *est;
> struct stmmac_fpe_cfg *fpe_cfg;
> struct stmmac_safety_feature_cfg *safety_feat_cfg;
> int clk_csr;
> --
> 2.25.1
>