| Message ID | 20241113111409.935032-1-quic_ziqichen@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] scsi: ufs: core: Add ufshcd_send_bsg_uic_cmd() for UFS BSG | expand |
On Wed, 2024-11-13 at 19:14 +0800, Ziqi Chen wrote: > User layer applications can send UIC GET/SET commands via the BSG > framework, and if the user layer application sends a UIC SET command > to the PA_PWRMODE attribute, a power mode change shall be initiated > in UniPro and two interrupts shall be triggered if the power mode is > successfully changed, i.e., UIC Command Completion interrupt and UIC > Power Mode interrupt. > > The current UFS BSG code calls ufshcd_send_uic_cmd() directly, with > which the second interrupt, i.e., UIC Power Mode interrupt, shall be > treated as unhandled interrupt. In addition, after the UIC command is > completed, user layer application has to poll UniPro and/or M-PHY > state > machine to confirm the power mode change is finished. > > Add a new wrapper function ufshcd_send_bsg_uic_cmd() and call it from > ufs_bsg_request() so that if a UIC SET command is targeting the > PA_PWRMODE > attribute it can be redirected to ufshcd_uic_pwr_ctrl(). > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> Mabye this should be a bug fix. Reviewed-by: Bean Huo <beanhuo@micron.com>
> User layer applications can send UIC GET/SET commands via the BSG > framework, and if the user layer application sends a UIC SET command to the > PA_PWRMODE attribute, a power mode change shall be initiated in UniPro > and two interrupts shall be triggered if the power mode is successfully > changed, i.e., UIC Command Completion interrupt and UIC Power Mode > interrupt. > > The current UFS BSG code calls ufshcd_send_uic_cmd() directly, with which > the second interrupt, i.e., UIC Power Mode interrupt, shall be treated as > unhandled interrupt. In addition, after the UIC command is completed, user > layer application has to poll UniPro and/or M-PHY state machine to confirm > the power mode change is finished. > > Add a new wrapper function ufshcd_send_bsg_uic_cmd() and call it from > ufs_bsg_request() so that if a UIC SET command is targeting the > PA_PWRMODE attribute it can be redirected to ufshcd_uic_pwr_ctrl(). > Fixes tag? > Signed-off-by: Can Guo <quic_cang@quicinc.com> > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> With that fixed: Reviewed-by: Avri Altman <avri.altman@wdc.com> > --- > V1 -> V2: Rebased on Linux 6.13 > --- > drivers/ufs/core/ufs_bsg.c | 2 +- > drivers/ufs/core/ufshcd-priv.h | 1 + > drivers/ufs/core/ufshcd.c | 36 ++++++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c index > 433d0480391e..6c09d97ae006 100644 > --- a/drivers/ufs/core/ufs_bsg.c > +++ b/drivers/ufs/core/ufs_bsg.c > @@ -170,7 +170,7 @@ static int ufs_bsg_request(struct bsg_job *job) > break; > case UPIU_TRANSACTION_UIC_CMD: > memcpy(&uc, &bsg_request->upiu_req.uc, UIC_CMD_SIZE); > - ret = ufshcd_send_uic_cmd(hba, &uc); > + ret = ufshcd_send_bsg_uic_cmd(hba, &uc); > if (ret) > dev_err(hba->dev, "send uic cmd: error code %d\n", ret); > > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index 7aea8fbaeee8..9ffd94ddf8c7 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -84,6 +84,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 > desc_index, > u8 **buf, bool ascii); > > int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command > *uic_cmd); > +int ufshcd_send_bsg_uic_cmd(struct ufs_hba *hba, struct uic_command > +*uic_cmd); > > int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, > struct utp_upiu_req *req_upiu, diff --git > a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > e338867bc96c..c01f4b0c1b4f 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4319,6 +4319,42 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba > *hba, struct uic_command *cmd) > return ret; > } > > +/** > + * ufshcd_send_bsg_uic_cmd - Send UIC commands requested via BSG layer > +and retrieve the result > + * @hba: per adapter instance > + * @uic_cmd: UIC command > + * > + * Return: 0 only if success. > + */ > +int ufshcd_send_bsg_uic_cmd(struct ufs_hba *hba, struct uic_command > +*uic_cmd) { > + int ret; > + > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD) > + return 0; > + > + ufshcd_hold(hba); > + > + if (uic_cmd->argument1 == UIC_ARG_MIB(PA_PWRMODE) && > + uic_cmd->command == UIC_CMD_DME_SET) { > + ret = ufshcd_uic_pwr_ctrl(hba, uic_cmd); > + goto out; > + } > + > + mutex_lock(&hba->uic_cmd_mutex); > + ufshcd_add_delay_before_dme_cmd(hba); > + > + ret = __ufshcd_send_uic_cmd(hba, uic_cmd); > + if (!ret) > + ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); > + > + mutex_unlock(&hba->uic_cmd_mutex); > + > +out: > + ufshcd_release(hba); > + return ret; > +} > + > /** > * ufshcd_uic_change_pwr_mode - Perform the UIC power mode chage > * using DME_SET primitives. > -- > 2.34.1
On 11/19/2024 2:02 PM, Avri Altman wrote: >> User layer applications can send UIC GET/SET commands via the BSG >> framework, and if the user layer application sends a UIC SET command to the >> PA_PWRMODE attribute, a power mode change shall be initiated in UniPro >> and two interrupts shall be triggered if the power mode is successfully >> changed, i.e., UIC Command Completion interrupt and UIC Power Mode >> interrupt. >> >> The current UFS BSG code calls ufshcd_send_uic_cmd() directly, with which >> the second interrupt, i.e., UIC Power Mode interrupt, shall be treated as >> unhandled interrupt. In addition, after the UIC command is completed, user >> layer application has to poll UniPro and/or M-PHY state machine to confirm >> the power mode change is finished. >> >> Add a new wrapper function ufshcd_send_bsg_uic_cmd() and call it from >> ufs_bsg_request() so that if a UIC SET command is targeting the >> PA_PWRMODE attribute it can be redirected to ufshcd_uic_pwr_ctrl(). >> > > Fixes tag? Thank Avri， will update new version to add Fixes tag. >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> > With that fixed: > Reviewed-by: Avri Altman <avri.altman@wdc.com> > >> --- >> V1 -> V2: Rebased on Linux 6.13
On Wed, 2024-11-13 at 19:14 +0800, Ziqi Chen wrote: > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index e338867bc96c..c01f4b0c1b4f 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4319,6 +4319,42 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba > *hba, struct uic_command *cmd) > return ret; > } > > +/** > + * ufshcd_send_bsg_uic_cmd - Send UIC commands requested via BSG > layer and retrieve the result > + * @hba: per adapter instance > + * @uic_cmd: UIC command > + * > + * Return: 0 only if success. > + */ > +int ufshcd_send_bsg_uic_cmd(struct ufs_hba *hba, struct uic_command > *uic_cmd) > +{ > + int ret; > + > + if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD) > + return 0; > + > + ufshcd_hold(hba); > + > + if (uic_cmd->argument1 == UIC_ARG_MIB(PA_PWRMODE) && > + uic_cmd->command == UIC_CMD_DME_SET) { > Hi Ziqi, Should we also check if uic_cmd->command == UIC_CMD_DME_HIBER_ENTER or UIC_CMD_DME_HIBER_EXIT? Thanks Peter > + ret = ufshcd_uic_pwr_ctrl(hba, uic_cmd); > + goto out; > + } > + > + mutex_lock(&hba->uic_cmd_mutex); > + ufshcd_add_delay_before_dme_cmd(hba); > + > + ret = __ufshcd_send_uic_cmd(hba, uic_cmd); > + if (!ret) > + ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); > + > + mutex_unlock(&hba->uic_cmd_mutex); > + > +out: > + ufshcd_release(hba); > + return ret; > +} > + > /** > * ufshcd_uic_change_pwr_mode - Perform the UIC power mode chage > * using DME_SET primitives. > -- > 2.34.1 >
Hi Peter, Thank you for your comment. We also recognize that the H8 Enter/Exit has the same issue. In fact, we have tried to add the H8 command before. Unfortunately, the situation with Hibern8 is complex, there are a lot of status checks, and some vendors have their own sequence that is implemented in their vendor driver by Vops. Simply including the H8 command here would cause other issues since the vendor sequence for H8 enter/exit is required. If we add the full sequence here, the code will be bloated. We haven’t come up with a good solution for the Hibern8 situation yet. However, whether we include the H8 command or not, the above issue is present as well (directly call ufshcd_send_uic_cmd() via BSG framework). Right now, we have tested the PA_PWRMODE case and confirmed that it works, so we want to quickly resolve the PA_PWRMODE issue to unblock our customers’ urgent cases first. As for the Hibern8 situation, we have plan to fix it but we want to fix it afterward, in a separated change. If you have any good suggestions, we can discuss separately. BRs, Ziqi On 11/19/2024 2:40 PM, Peter Wang (王信友) wrote: > On Wed, 2024-11-13 at 19:14 +0800, Ziqi Chen wrote: > >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index e338867bc96c..c01f4b0c1b4f 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -4319,6 +4319,42 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba >> *hba, struct uic_command *cmd) >> return ret; >> } >> >> +/** >> + * ufshcd_send_bsg_uic_cmd - Send UIC commands requested via BSG >> layer and retrieve the result >> + * @hba: per adapter instance >> + * @uic_cmd: UIC command >> + * >> + * Return: 0 only if success. >> + */ >> +int ufshcd_send_bsg_uic_cmd(struct ufs_hba *hba, struct uic_command >> *uic_cmd) >> +{ >> + int ret; >> + >> + if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD) >> + return 0; >> + >> + ufshcd_hold(hba); >> + >> + if (uic_cmd->argument1 == UIC_ARG_MIB(PA_PWRMODE) && >> + uic_cmd->command == UIC_CMD_DME_SET) { >> > > Hi Ziqi, > > Should we also check if uic_cmd->command == UIC_CMD_DME_HIBER_ENTER > or UIC_CMD_DME_HIBER_EXIT? > > Thanks > Peter > > > >> + ret = ufshcd_uic_pwr_ctrl(hba, uic_cmd); >> + goto out; >> + } >> + >> + mutex_lock(&hba->uic_cmd_mutex); >> + ufshcd_add_delay_before_dme_cmd(hba); >> + >> + ret = __ufshcd_send_uic_cmd(hba, uic_cmd); >> + if (!ret) >> + ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); >> + >> + mutex_unlock(&hba->uic_cmd_mutex); >> + >> +out: >> + ufshcd_release(hba); >> + return ret; >> +} >> + >> /** >> * ufshcd_uic_change_pwr_mode - Perform the UIC power mode chage >> * using DME_SET primitives. >> -- >> 2.34.1 >>
On Tue, 2024-11-19 at 16:52 +0800, Ziqi Chen wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Hi Peter, > > Thank you for your comment. We also recognize that the H8 Enter/Exit > has > the same issue. In fact, we have tried to add the H8 command before. > Unfortunately, the situation with Hibern8 is complex, there are a lot > of > status checks, and some vendors have their own sequence that is > implemented in their vendor driver by Vops. Simply including the H8 > command here would cause other issues since the vendor sequence for > H8 > enter/exit is required. If we add the full sequence here, the code > will > be bloated. We haven’t come up with a good solution for the Hibern8 > situation yet. However, whether we include the H8 command or not, the > above issue is present as well (directly call ufshcd_send_uic_cmd() > via > BSG framework). > Right now, we have tested the PA_PWRMODE case and confirmed that it > works, so we want to quickly resolve the PA_PWRMODE issue to unblock > our > customers’ urgent cases first. As for the Hibern8 situation, we have > plan to fix it but we want to fix it afterward, in a separated > change. > If you have any good suggestions, we can discuss separately. > > BRs, > Ziqi > Hi Ziqi, I can understand. Thanks. Peter Reviewed-by: Peter Wang <peter.wang@mediatek.com>
diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c index 433d0480391e..6c09d97ae006 100644 --- a/drivers/ufs/core/ufs_bsg.c +++ b/drivers/ufs/core/ufs_bsg.c @@ -170,7 +170,7 @@ static int ufs_bsg_request(struct bsg_job *job) break; case UPIU_TRANSACTION_UIC_CMD: memcpy(&uc, &bsg_request->upiu_req.uc, UIC_CMD_SIZE); - ret = ufshcd_send_uic_cmd(hba, &uc); + ret = ufshcd_send_bsg_uic_cmd(hba, &uc); if (ret) dev_err(hba->dev, "send uic cmd: error code %d\n", ret); diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index 7aea8fbaeee8..9ffd94ddf8c7 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -84,6 +84,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, bool ascii); int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd); +int ufshcd_send_bsg_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd); int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, struct utp_upiu_req *req_upiu, diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index e338867bc96c..c01f4b0c1b4f 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4319,6 +4319,42 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) return ret; } +/** + * ufshcd_send_bsg_uic_cmd - Send UIC commands requested via BSG layer and retrieve the result + * @hba: per adapter instance + * @uic_cmd: UIC command + * + * Return: 0 only if success. + */ +int ufshcd_send_bsg_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) +{ + int ret; + + if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD) + return 0; + + ufshcd_hold(hba); + + if (uic_cmd->argument1 == UIC_ARG_MIB(PA_PWRMODE) && + uic_cmd->command == UIC_CMD_DME_SET) { + ret = ufshcd_uic_pwr_ctrl(hba, uic_cmd); + goto out; + } + + mutex_lock(&hba->uic_cmd_mutex); + ufshcd_add_delay_before_dme_cmd(hba); + + ret = __ufshcd_send_uic_cmd(hba, uic_cmd); + if (!ret) + ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); + + mutex_unlock(&hba->uic_cmd_mutex); + +out: + ufshcd_release(hba); + return ret; +} + /** * ufshcd_uic_change_pwr_mode - Perform the UIC power mode chage * using DME_SET primitives.