From: "Kumar, M Chetan" <m.chetan.kumar@linux•intel.com>
To: Jesse Brandeburg <jesse.brandeburg@intel•com>, netdev@vger•kernel.org
Cc: kuba@kernel•org, davem@davemloft•net, johannes@sipsolutions•net,
ryazanov.s.a@gmail•com, loic.poulain@linaro•org,
ilpo.jarvinen@linux•intel.com, ricardo.martinez@linux•intel.com,
chiranjeevi.rapolu@linux•intel.com, haijun.liu@mediatek•com,
edumazet@google•com, pabeni@redhat•com, linuxwwan@intel•com,
linuxwwan_5g@intel•com, chandrashekar.devegowda@intel•com,
matthias.bgg@gmail•com, linux-arm-kernel@lists•infradead.org,
linux-mediatek@lists•infradead.org,
Madhusmita Sahu <madhusmita.sahu@intel•com>
Subject: Re: [PATCH v3 net-next 2/5] net: wwan: t7xx: Infrastructure for early port configuration
Date: Sun, 8 Jan 2023 19:37:35 +0530 [thread overview]
Message-ID: <e392d2df-48b3-fb7b-86d3-2ea22aa11467@linux.intel.com> (raw)
In-Reply-To: <8b1bd24c-f14c-0244-d2fb-69d4f02b46d5@intel.com>
Hi Jesse,
Thank you for the feedback.
On 1/7/2023 12:01 AM, Jesse Brandeburg wrote:
> On 1/6/2023 8:26 AM, m.chetan.kumar@linux•intel.com wrote:
>> From: M Chetan Kumar <m.chetan.kumar@linux•intel.com>
>>
>> To support cases such as FW update or Core dump, the t7xx
>> device is capable of signaling the host that a special port
>> needs to be created before the handshake phase.
>>
>> This patch adds the infrastructure required to create the
>> early ports which also requires a different configuration of
>> CLDMA queues.
>
> nit: use imperative voice in your commit messages: no "this patch".
> instead:
> "Add the infrastructure..."
Sure. Will correct it.
>
>>
>> Signed-off-by: Haijun Liu <haijun.liu@mediatek•com>
>> Co-developed-by: Madhusmita Sahu <madhusmita.sahu@intel•com>
>> Signed-off-by: Madhusmita Sahu <madhusmita.sahu@intel•com>
>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux•intel.com>
>> Signed-off-by: Devegowda Chandrashekar
>> <chandrashekar.devegowda@intel•com>
>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux•intel.com>
>> --
>> v3:
>> * No Change.
>> v2:
>> * Move recv_skb handler to cldma_queue.
>> * Drop cldma_queue_type.
>> * Restore prototype of t7xx_port_send_raw_skb().
>> * Remove PORT_CFG_ID_INVALID check in t7xx_port_proxy_set_cfg().
>> * Add space before */.
>> * Drop unnecessary logs.
>> * Use WARN_ON on early port.
>> * Use new MISC_DEV_STATUS_INVALID instead of MISC_DEV_STATUS.
>> * Use macros instead of const identifiers.
>> * Change ports member type from pointer to array type.
>> * Prefix LK_EVENT_XX with MISC prefix.
>> * Use t7xx prefix for device_stage enums.
>> * Correct log messages.
>> * Don’t override pkt_size for non-download port under dedicated Queue.
>> * Drop cldma_txq_rxq_ids.
>> * Use macro for txq/rxq index.
>> * Use warn_on for rxq_idx comparison.
>> * Drop t7xx_port_proxy_get_port_by_name().
>> * Replace fsm poll with read_poll_timeout().
>> * Use "\n" consistently across log message.
>> * Remove local var _dev prefixes in fsm_routine_start().
>> * Use max_t.
>
> ...
>
>
>> diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h
>> b/drivers/net/wwan/t7xx/t7xx_reg.h
>> index c41d7d094c08..44352cd02460 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_reg.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_reg.h
>> @@ -102,10 +102,28 @@ enum t7xx_pm_resume_state {
>> };
>> #define T7XX_PCIE_MISC_DEV_STATUS 0x0d1c
>> -#define MISC_STAGE_MASK GENMASK(2, 0)
>> -#define MISC_RESET_TYPE_PLDR BIT(26)
>> #define MISC_RESET_TYPE_FLDR BIT(27)
>> -#define LINUX_STAGE 4
>> +#define MISC_RESET_TYPE_PLDR BIT(26)
>> +#define MISC_DEV_STATUS_MASK GENMASK(15, 0)
>> +#define MISC_DEV_STATUS_INVALID GENMASK(15, 0)
>
> I don't see any uses of this, even though it's mentioned in the commit
> message. The only reason I looked was because it was weird to have
> DEV_STATUS_MASK and STATUS_INVALID be the same values, is that correct?
Both are unused. Will drop it.
>
>
>> +#define MISC_LK_EVENT_MASK GENMASK(11, 8)
>> +
>> +enum lk_event_id {
>> + LK_EVENT_NORMAL = 0,
>> + LK_EVENT_CREATE_PD_PORT = 1,
>> + LK_EVENT_CREATE_POST_DL_PORT = 2,
>> + LK_EVENT_RESET = 7,
>> +};
>> +
>> +#define MISC_STAGE_MASK GENMASK(2, 0)
>> +
>> +enum t7xx_device_stage {
>> + T7XX_DEV_STAGE_INIT = 0,
>> + T7XX_DEV_STAGE_BROM_PRE = 1,
>> + T7XX_DEV_STAGE_BROM_POST = 2,
>> + T7XX_DEV_STAGE_LK = 3,
>> + T7XX_DEV_STAGE_LINUX = 4,
>> +};
>> #define T7XX_PCIE_RESOURCE_STATUS 0x0d28
>> #define T7XX_PCIE_RESOURCE_STS_MSK GENMASK(4, 0)
>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>> b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>> index 80edb8e75a6a..76fb5d57d4d7 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>> @@ -206,6 +206,34 @@ static void fsm_routine_exception(struct
>> t7xx_fsm_ctl *ctl, struct t7xx_fsm_comm
>> fsm_finish_command(ctl, cmd, 0);
>> }
>> +static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl,
>> unsigned int status)
>> +{
>> + struct t7xx_modem *md = ctl->md;
>> + struct cldma_ctrl *md_ctrl;
>> + enum lk_event_id lk_event;
>> + struct device *dev;
>> +
>> + dev = &md->t7xx_dev->pdev->dev;
>> + lk_event = FIELD_GET(MISC_LK_EVENT_MASK, status);
>> + switch (lk_event) {
>> + case LK_EVENT_NORMAL:
>> + case LK_EVENT_RESET:
>> + break;
>> +
>> + case LK_EVENT_CREATE_PD_PORT:
>> + md_ctrl = md->md_ctrl[CLDMA_ID_AP];
>> + t7xx_cldma_hif_hw_init(md_ctrl);
>> + t7xx_cldma_stop(md_ctrl);
>> + t7xx_cldma_switch_cfg(md_ctrl, CLDMA_DEDICATED_Q_CFG);
>> + t7xx_cldma_start(md_ctrl);
>> + break;
>> +
>> + default:
>> + dev_err(dev, "Invalid LK event %d\n", lk_event);
>> + break;
>> + }
>> +}
>> +
>> static int fsm_stopped_handler(struct t7xx_fsm_ctl *ctl)
>> {
>> ctl->curr_state = FSM_STATE_STOPPED;
>> @@ -317,8 +345,9 @@ static int fsm_routine_starting(struct
>> t7xx_fsm_ctl *ctl)
>> static void fsm_routine_start(struct t7xx_fsm_ctl *ctl, struct
>> t7xx_fsm_command *cmd)
>> {
>> struct t7xx_modem *md = ctl->md;
>> - u32 dev_status;
>> - int ret;
>> + struct device *dev;
>> + u32 status, stage;
>> + int ret = 0;
>> if (!md)
>> return;
>> @@ -329,23 +358,55 @@ static void fsm_routine_start(struct
>> t7xx_fsm_ctl *ctl, struct t7xx_fsm_command
>> return;
>> }
>> + dev = &md->t7xx_dev->pdev->dev;
>> ctl->curr_state = FSM_STATE_PRE_START;
>> t7xx_md_event_notify(md, FSM_PRE_START);
>> - ret = read_poll_timeout(ioread32, dev_status,
>> - (dev_status & MISC_STAGE_MASK) == LINUX_STAGE, 20000,
>> 2000000,
>> - false, IREG_BASE(md->t7xx_dev) +
>> T7XX_PCIE_MISC_DEV_STATUS);
>> + ret = read_poll_timeout(ioread32, status,
>> + ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LINUX) ||
>> + ((status & MISC_STAGE_MASK) == T7XX_DEV_STAGE_LK),
>> 100000,
>> + 20000000, false, IREG_BASE(md->t7xx_dev) +
>> + T7XX_PCIE_MISC_DEV_STATUS);
>> +
>> if (ret) {
>> - struct device *dev = &md->t7xx_dev->pdev->dev;
>> + ret = -ETIMEDOUT;
>> + dev_err(dev, "read poll %d\n", ret);
>> + goto finish_command;
>> + }
>> - fsm_finish_command(ctl, cmd, -ETIMEDOUT);
>> - dev_err(dev, "Invalid device status 0x%lx\n", dev_status &
>> MISC_STAGE_MASK);
>> - return;
>> + if (status != ctl->prev_status) {
>> + stage = FIELD_GET(MISC_STAGE_MASK, status);
>
> if stage is only used down here you can declare it locally. cppcheck has
> a check that will find these for you.
Ok. Will consider it.
>
>> + switch (stage) {
>> + case T7XX_DEV_STAGE_INIT:
>> + case T7XX_DEV_STAGE_BROM_PRE:
>> + case T7XX_DEV_STAGE_BROM_POST:
>> + dev_info(dev, "BROM_STAGE Entered\n");
>> + ret = t7xx_fsm_append_cmd(ctl, FSM_CMD_START, 0);
>> + break;
>> +
>> + case T7XX_DEV_STAGE_LK:
>> + dev_info(dev, "LK_STAGE Entered\n");
>> + t7xx_lk_stage_event_handling(ctl, status);
>> + break;
>> +
>> + case T7XX_DEV_STAGE_LINUX:
>> + dev_info(dev, "LINUX_STAGE Entered\n");
>> + t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
>> + t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
>> + t7xx_mhccif_mask_clr(md->t7xx_dev, D2H_INT_PORT_ENUM |
>> + D2H_INT_ASYNC_MD_HK | D2H_INT_ASYNC_AP_HK);
>> + t7xx_port_proxy_set_cfg(md, PORT_CFG_ID_NORMAL);
>> + ret = fsm_routine_starting(ctl);
>> + break;
>> +
>> + default:
>> + break;
>> + }
>> + ctl->prev_status = status;
>> }
>> - t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_AP]);
>> - t7xx_cldma_hif_hw_init(md->md_ctrl[CLDMA_ID_MD]);
>> - fsm_finish_command(ctl, cmd, fsm_routine_starting(ctl));
>> +finish_command:
>> + fsm_finish_command(ctl, cmd, ret);
>> }
>> static int fsm_main_thread(void *data)
>> @@ -516,6 +577,7 @@ void t7xx_fsm_reset(struct t7xx_modem *md)
>> fsm_flush_event_cmd_qs(ctl);
>> ctl->curr_state = FSM_STATE_STOPPED;
>> ctl->exp_flg = false;
>> + ctl->prev_status = 0;
>> }
>> int t7xx_fsm_init(struct t7xx_modem *md)
>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> b/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> index b6e76f3903c8..5e8012567ba1 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.h
>> @@ -96,6 +96,7 @@ struct t7xx_fsm_ctl {
>> bool exp_flg;
>> spinlock_t notifier_lock; /* Protects notifier
>> list */
>> struct list_head notifier_list;
>> + u32 prev_status;
>> };
>> struct t7xx_fsm_event {
>
--
Chetan
next prev parent reply other threads:[~2023-01-08 14:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-06 16:26 [PATCH v3 net-next 0/5] net: wwan: t7xx: fw flashing & coredump support m.chetan.kumar
2023-01-06 16:26 ` [PATCH v3 net-next 1/5] net: wwan: t7xx: Add AP CLDMA m.chetan.kumar
2023-01-06 17:56 ` Jesse Brandeburg
2023-01-06 16:26 ` [PATCH v3 net-next 2/5] net: wwan: t7xx: Infrastructure for early port configuration m.chetan.kumar
2023-01-06 18:31 ` Jesse Brandeburg
2023-01-08 14:07 ` Kumar, M Chetan [this message]
2023-01-06 16:27 ` [PATCH v3 net-next 3/5] net: wwan: t7xx: PCIe reset rescan m.chetan.kumar
2023-01-06 18:42 ` Jesse Brandeburg
2023-01-08 14:14 ` Kumar, M Chetan
2023-01-06 16:27 ` [PATCH v3 net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection m.chetan.kumar
2023-01-06 18:50 ` Jesse Brandeburg
2023-01-06 16:28 ` [PATCH v3 net-next 5/5] net: wwan: t7xx: Devlink documentation m.chetan.kumar
2023-01-06 18:37 ` Jesse Brandeburg
2023-01-08 14:20 ` Kumar, M Chetan
2023-01-09 3:48 ` Bagas Sanjaya
2023-01-09 16:07 ` Kumar, M Chetan
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=e392d2df-48b3-fb7b-86d3-2ea22aa11467@linux.intel.com \
--to=m.chetan.kumar@linux$(echo .)intel.com \
--cc=chandrashekar.devegowda@intel$(echo .)com \
--cc=chiranjeevi.rapolu@linux$(echo .)intel.com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=haijun.liu@mediatek$(echo .)com \
--cc=ilpo.jarvinen@linux$(echo .)intel.com \
--cc=jesse.brandeburg@intel$(echo .)com \
--cc=johannes@sipsolutions$(echo .)net \
--cc=kuba@kernel$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-mediatek@lists$(echo .)infradead.org \
--cc=linuxwwan@intel$(echo .)com \
--cc=linuxwwan_5g@intel$(echo .)com \
--cc=loic.poulain@linaro$(echo .)org \
--cc=madhusmita.sahu@intel$(echo .)com \
--cc=matthias.bgg@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=ricardo.martinez@linux$(echo .)intel.com \
--cc=ryazanov.s.a@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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