From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Ricardo Martinez <ricardo.martinez@linux.intel.com>
Cc: Netdev <netdev@vger.kernel.org>,
linux-wireless@vger.kernel.org, kuba@kernel.org,
davem@davemloft.net, johannes@sipsolutions.net,
ryazanov.s.a@gmail.com, loic.poulain@linaro.org,
m.chetan.kumar@intel.com, chandrashekar.devegowda@intel.com,
linuxwwan@intel.com, chiranjeevi.rapolu@linux.intel.com,
haijun.liu@mediatek.com, amir.hanania@intel.com,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
dinesh.sharma@intel.com, eliot.lee@intel.com,
moises.veleta@intel.com, pierre-louis.bossart@intel.com,
muralidharan.sethuraman@intel.com,
Soumya.Prakash.Mishra@intel.com, sreehari.kancharla@intel.com
Subject: Re: [PATCH net-next v4 07/13] net: wwan: t7xx: Data path HW layer
Date: Tue, 1 Feb 2022 11:08:39 +0200 (EET) [thread overview]
Message-ID: <8593f871-6737-7f85-5035-b1b2d5d312e@linux.intel.com> (raw)
In-Reply-To: <20220114010627.21104-8-ricardo.martinez@linux.intel.com>
On Thu, 13 Jan 2022, Ricardo Martinez wrote:
> From: Haijun Liu <haijun.liu@mediatek.com>
>
> Data Path Modem AP Interface (DPMAIF) HW layer provides HW abstraction
> for the upper layer (DPMAIF HIF). It implements functions to do the HW
> configuration, TX/RX control and interrupt handling.
>
> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com>
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> ---
> +static int t7xx_dpmaif_init_intr(struct dpmaif_hw_info *hw_info)
> +{
> + struct dpmaif_isr_en_mask *isr_en_msk = &hw_info->isr_en_mask;
dpmaif_isr_en_mask is not defined until patch 08.
> +/* The para->intr_cnt counter is set to zero before this function is called.
> + * It does not check for overflow as there is no risk of overflowing intr_types or intr_queues.
> + */
> +static void t7xx_dpmaif_hw_check_rx_intr(struct dpmaif_ctrl *dpmaif_ctrl,
> + unsigned int *pl2_rxisar0,
> + struct dpmaif_hw_intr_st_para *para, int qno)
> +{
> + struct dpmaif_hw_info *hw_info = &dpmaif_ctrl->hif_hw_info;
> + unsigned int l2_rxisar0 = *pl2_rxisar0;
> + unsigned int value;
> +
> + if (qno == DPF_RX_QNO_DFT) {
> + value = l2_rxisar0 & DP_DL_INT_SKB_LEN_ERR;
> + if (value)
In this function, value variable doesn't seem that useful compared with
checking the condition directly on if line. And the value is never used
after that.
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_SKB_LEN_ERR, DPF_RX_QNO_DFT);
> +
> + value = l2_rxisar0 & DP_DL_INT_BATCNT_LEN_ERR;
> + if (value) {
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_BATCNT_LEN_ERR, DPF_RX_QNO_DFT);
> + hw_info->isr_en_mask.ap_dl_l2intr_en_msk &= ~DP_DL_INT_BATCNT_LEN_ERR;
> + iowrite32(DP_DL_INT_BATCNT_LEN_ERR,
> + hw_info->pcie_base + DPMAIF_AO_UL_APDL_L2TIMSR0);
> + }
> +
> + value = l2_rxisar0 & DP_DL_INT_PITCNT_LEN_ERR;
> + if (value) {
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_PITCNT_LEN_ERR, DPF_RX_QNO_DFT);
> + hw_info->isr_en_mask.ap_dl_l2intr_en_msk &= ~DP_DL_INT_PITCNT_LEN_ERR;
> + iowrite32(DP_DL_INT_PITCNT_LEN_ERR,
> + hw_info->pcie_base + DPMAIF_AO_UL_APDL_L2TIMSR0);
> + }
> +
> + value = l2_rxisar0 & DP_DL_INT_PKT_EMPTY_MSK;
> + if (value)
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_PKT_EMPTY_SET, DPF_RX_QNO_DFT);
> +
> + value = l2_rxisar0 & DP_DL_INT_FRG_EMPTY_MSK;
> + if (value)
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_FRG_EMPTY_SET, DPF_RX_QNO_DFT);
> +
> + value = l2_rxisar0 & DP_DL_INT_MTU_ERR_MSK;
> + if (value)
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_MTU_ERR, DPF_RX_QNO_DFT);
> +
> + value = l2_rxisar0 & DP_DL_INT_FRG_LENERR_MSK;
> + if (value)
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_FRGCNT_LEN_ERR, DPF_RX_QNO_DFT);
> +
> + value = l2_rxisar0 & DP_DL_INT_Q0_PITCNT_LEN_ERR;
> + if (value) {
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_Q0_PITCNT_LEN_ERR, BIT(qno));
> + t7xx_dpmaif_dlq_mask_rx_pitcnt_len_err_intr(hw_info, qno);
> + }
> +
> + value = l2_rxisar0 & DP_DL_INT_HPC_ENT_TYPE_ERR;
> + if (value)
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_HPC_ENT_TYPE_ERR,
> + DPF_RX_QNO_DFT);
> +
> + value = l2_rxisar0 & DP_DL_INT_Q0_DONE;
> + if (value) {
> + /* Mask RX done interrupt immediately after it occurs */
> + if (!t7xx_mask_dlq_intr(dpmaif_ctrl, qno)) {
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_Q0_DONE, BIT(qno));
> + } else {
> + /* Unable to clear the interrupt, try again on the next one
> + * device entered low power mode or suffer exception
> + */
It's not obvious what "on the next one" means. I assume you're
also missing period from end of the first line.
> + *pl2_rxisar0 = l2_rxisar0 & ~DP_DL_INT_Q0_DONE;
> + }
> + }
> + } else {
> + value = l2_rxisar0 & DP_DL_INT_Q1_PITCNT_LEN_ERR;
> + if (value) {
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_Q1_PITCNT_LEN_ERR, BIT(qno));
> + t7xx_dpmaif_dlq_mask_rx_pitcnt_len_err_intr(hw_info, qno);
> + }
> +
> + value = l2_rxisar0 & DP_DL_INT_Q1_DONE;
> + if (value) {
> + if (!t7xx_mask_dlq_intr(dpmaif_ctrl, qno))
> + t7xx_dpmaif_set_intr_para(para, DPF_INTR_DL_Q1_DONE, BIT(qno));
> + else
> + *pl2_rxisar0 = l2_rxisar0 & ~DP_DL_INT_Q1_DONE;
> + }
> + }
> +}
> +static void t7xx_dpmaif_dl_dlq_pit_en(struct dpmaif_hw_info *hw_info, unsigned char q_num,
> + bool enable)
> +{
> + unsigned int value;
> +
> + value = ioread32(hw_info->pcie_base + DPMAIF_DL_DLQPIT_INIT_CON3);
> +
> + if (enable)
> + value |= DPMAIF_DLQPIT_EN_MSK;
> + else
> + value &= ~DPMAIF_DLQPIT_EN_MSK;
> +
> + iowrite32(value, hw_info->pcie_base + DPMAIF_DL_DLQPIT_INIT_CON3);
> +}
Only called with enabled = true
> +static int t7xx_dpmaif_config_dlq_hw(struct dpmaif_ctrl *dpmaif_ctrl)
> +{
> + struct dpmaif_hw_info *hw_info = &dpmaif_ctrl->hif_hw_info;
> + struct dpmaif_dl_hwq *dl_hw;
Only defined in 08. I might have not noticed all missing defs
so please compile test yourself to find the rest if any.
In general, it would be useful to use, e.g., a shell for loop to compile
test every change incrementally in the patchset before sending them out.
Another thing is that the values inside struct dpmaif_dl_hwq are
just set from constants and never changed anywhere. Why not use
the constants directly?
> +static void t7xx_dpmaif_ul_all_q_en(struct dpmaif_hw_info *hw_info, bool enable)
> +{
> + u32 ul_arb_en = ioread32(hw_info->pcie_base + DPMAIF_AO_UL_CHNL_ARB0);
> +
> + if (enable)
> + ul_arb_en |= DPMAIF_UL_ALL_QUE_ARB_EN;
> + else
> + ul_arb_en &= ~DPMAIF_UL_ALL_QUE_ARB_EN;
> +
> + iowrite32(ul_arb_en, hw_info->pcie_base + DPMAIF_AO_UL_CHNL_ARB0);
> +}
> +
> +static bool t7xx_dpmaif_ul_idle_check(struct dpmaif_hw_info *hw_info)
> +{
> + u32 dpmaif_ul_is_busy = ioread32(hw_info->pcie_base + DPMAIF_UL_CHK_BUSY);
> +
> + return !(dpmaif_ul_is_busy & DPMAIF_UL_IDLE_STS);
> +}
> +
> + /* DPMAIF UL Part HW setting */
While not extremely useful to begin with, isn't this comment too late as
it is after two UL related functions?
> +unsigned int t7xx_dpmaif_dl_get_frg_rd_idx(struct dpmaif_hw_info *hw_info, unsigned char q_num)
> +{
> + u32 value;
> +
> + value = ioread32(hw_info->pcie_base + DPMAIF_AO_DL_FRGBAT_WRIDX);
> + return value & DPMAIF_DL_FRG_WRIDX_MSK;
> +}
Function name has rd_idx but defines used are WRIDX. Is it intentional?
> +enum dpmaif_hw_intr_type {
> + DPF_INTR_INVALID_MIN,
> + DPF_INTR_UL_DONE,
> + DPF_INTR_UL_DRB_EMPTY,
> + DPF_INTR_UL_MD_NOTREADY,
> + DPF_INTR_UL_MD_PWR_NOTREADY,
> + DPF_INTR_UL_LEN_ERR,
> + DPF_INTR_DL_DONE,
> + DPF_INTR_DL_SKB_LEN_ERR,
> + DPF_INTR_DL_BATCNT_LEN_ERR,
> + DPF_INTR_DL_PITCNT_LEN_ERR,
> + DPF_INTR_DL_PKT_EMPTY_SET,
> + DPF_INTR_DL_FRG_EMPTY_SET,
> + DPF_INTR_DL_MTU_ERR,
> + DPF_INTR_DL_FRGCNT_LEN_ERR,
> + DPF_INTR_DL_Q0_PITCNT_LEN_ERR,
> + DPF_INTR_DL_Q1_PITCNT_LEN_ERR,
> + DPF_INTR_DL_HPC_ENT_TYPE_ERR,
> + DPF_INTR_DL_Q0_DONE,
> + DPF_INTR_DL_Q1_DONE,
> + DPF_INTR_INVALID_MAX
> +};
> +
> +#define DPF_RX_QNO0 0
> +#define DPF_RX_QNO1 1
> +#define DPF_RX_QNO_DFT DPF_RX_QNO0
> +
> +struct dpmaif_hw_intr_st_para {
> + unsigned int intr_cnt;
> + enum dpmaif_hw_intr_type intr_types[DPF_INTR_INVALID_MAX - 1];
> + unsigned int intr_queues[DPF_INTR_INVALID_MAX - 1];
Off-by-one errors?
In addition, I think there's some other problem related to these as
there are 20 values in enum (of which two are named "INVALID") but
t7xx_dpmaif_set_intr_para seems to be called only with 17 of them
(DPF_INTR_DL_DONE not among the calls). This implies intr_cnt will
likely be too small to cover the last entry when it is being used
in 08/13 for a for loop termination condition.
--
i.
next prev parent reply other threads:[~2022-02-01 9:08 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-14 1:06 [PATCH net-next v4 00/13] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem Ricardo Martinez
2022-01-14 1:06 ` [PATCH net-next v4 01/13] list: Add list_next_entry_circular() and list_prev_entry_circular() Ricardo Martinez
2022-01-14 13:42 ` Andy Shevchenko
2022-01-14 1:06 ` [PATCH net-next v4 02/13] net: wwan: t7xx: Add control DMA interface Ricardo Martinez
2022-01-14 14:13 ` Andy Shevchenko
2022-01-18 14:13 ` Ilpo Järvinen
2022-01-18 22:22 ` Martinez, Ricardo
2022-01-19 9:52 ` Ilpo Järvinen
2022-01-19 19:04 ` Martinez, Ricardo
2022-02-11 0:25 ` Sergey Ryazanov
2022-02-16 2:24 ` Martinez, Ricardo
2022-02-10 13:50 ` Ilpo Järvinen
2022-01-14 1:06 ` [PATCH net-next v4 03/13] net: wwan: t7xx: Add core components Ricardo Martinez
2022-01-16 15:37 ` kernel test robot
2022-01-24 14:51 ` Ilpo Järvinen
2022-01-25 19:13 ` Martinez, Ricardo
2022-01-26 10:45 ` Ilpo Järvinen
2022-01-27 17:36 ` Ilpo Järvinen
2022-01-14 1:06 ` [PATCH net-next v4 04/13] net: wwan: t7xx: Add port proxy infrastructure Ricardo Martinez
2022-01-25 13:38 ` Ilpo Järvinen
2022-02-10 13:34 ` Ilpo Järvinen
2022-01-14 1:06 ` [PATCH net-next v4 05/13] net: wwan: t7xx: Add control port Ricardo Martinez
2022-01-27 10:40 ` Ilpo Järvinen
2022-01-27 14:53 ` Andy Shevchenko
2022-01-14 1:06 ` [PATCH net-next v4 06/13] net: wwan: t7xx: Add AT and MBIM WWAN ports Ricardo Martinez
2022-01-27 11:56 ` Ilpo Järvinen
2022-01-14 1:06 ` [PATCH net-next v4 07/13] net: wwan: t7xx: Data path HW layer Ricardo Martinez
2022-02-01 9:08 ` Ilpo Järvinen [this message]
2022-02-01 10:13 ` Ilpo Järvinen
2022-02-03 2:30 ` Martinez, Ricardo
2022-01-14 1:06 ` [PATCH net-next v4 08/13] net: wwan: t7xx: Add data path interface Ricardo Martinez
2022-02-03 14:23 ` Ilpo Järvinen
2022-02-08 8:19 ` Ilpo Järvinen
2022-02-16 2:17 ` Martinez, Ricardo
2022-02-16 14:36 ` Ilpo Järvinen
2022-02-22 18:40 ` Martinez, Ricardo
2022-01-14 1:06 ` [PATCH net-next v4 09/13] net: wwan: t7xx: Add WWAN network interface Ricardo Martinez
2022-02-10 10:45 ` Ilpo Järvinen
2022-01-14 1:06 ` [PATCH net-next v4 10/13] net: wwan: t7xx: Introduce power management support Ricardo Martinez
2022-02-10 10:58 ` Ilpo Järvinen
2022-01-14 1:06 ` [PATCH net-next v4 11/13] net: wwan: t7xx: Runtime PM Ricardo Martinez
2022-01-14 1:06 ` [PATCH net-next v4 12/13] net: wwan: t7xx: Device deep sleep lock/unlock Ricardo Martinez
2022-01-14 1:06 ` [PATCH net-next v4 13/13] net: wwan: t7xx: Add maintainers and documentation Ricardo Martinez
2022-01-15 14:53 ` [PATCH net-next v4 00/13] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem Loic Poulain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8593f871-6737-7f85-5035-b1b2d5d312e@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=Soumya.Prakash.Mishra@intel.com \
--cc=amir.hanania@intel.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=chandrashekar.devegowda@intel.com \
--cc=chiranjeevi.rapolu@linux.intel.com \
--cc=davem@davemloft.net \
--cc=dinesh.sharma@intel.com \
--cc=eliot.lee@intel.com \
--cc=haijun.liu@mediatek.com \
--cc=johannes@sipsolutions.net \
--cc=kuba@kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linuxwwan@intel.com \
--cc=loic.poulain@linaro.org \
--cc=m.chetan.kumar@intel.com \
--cc=moises.veleta@intel.com \
--cc=muralidharan.sethuraman@intel.com \
--cc=netdev@vger.kernel.org \
--cc=pierre-louis.bossart@intel.com \
--cc=ricardo.martinez@linux.intel.com \
--cc=ryazanov.s.a@gmail.com \
--cc=sreehari.kancharla@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).