| Message ID | 20240909231139.2367576-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Improve the UFS driver link hibernation code | expand |
On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Make the role of the structure members related to UIC command > processing > more clear. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > include/ufs/ufshcd.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index a43b14276bc3..85933775c9f3 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -868,9 +868,10 @@ enum ufshcd_mcq_opr { > * @tmf_tag_set: TMF tag set. > * @tmf_queue: Used to allocate TMF tags. > * @tmf_rqs: array with pointers to TMF requests while these are in > progress. > - * @active_uic_cmd: handle of active UIC command > - * @uic_cmd_mutex: mutex for UIC command > - * @uic_async_done: completion used during UIC processing > + * @active_uic_cmd: pointer to active UIC command. > + * @uic_cmd_mutex: mutex used for serializing UIC command > processing. > + * @uic_async_done: completion used to wait for power mode or > hibernation state > + * changes. > * @ufshcd_state: UFSHCD state > * @eh_flags: Error handling flags > * @intr_mask: Interrupt Mask Bits Reviewed-by: Peter Wang <peter.wang@mediatek.com>
On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Simplify __ufshcd_send_uic_cmd() by always initializing the > uic_cmd::done completion. This is fine since the time required to > initialize a completion is small compared to the time required to > process an UIC command. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Hi Bart, The time to compare should not be with the UIC command process time. It should be compared with the time for the "if (completion)" conditional expression. We cannot justify increasing the latency of sending a UIC command just because the UIC command process time is longer. Thanks. Peter
On 9/10/24 5:34 AM, Peter Wang (王信友) wrote: > On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: >> Simplify __ufshcd_send_uic_cmd() by always initializing the >> uic_cmd::done completion. This is fine since the time required to >> initialize a completion is small compared to the time required to >> process an UIC command. > > The time to compare should not be with the UIC command process time. > It should be compared with the time for the "if (completion)" > conditional > expression. We cannot justify increasing the latency of sending a UIC > command just because the UIC command process time is longer. Although I appreciate your concern about performance, I don't think that the above feedback is appropriate. On my test setup UIC commands take between 20 and 1500 microseconds to complete. A single init_completion() call takes about 0.01 microseconds. So I don't think that the time spent in init_completion() matters for UIC commands. Additionally, this patch only introduces an init_completion() call for power management commands and not any other type of UIC command. Thanks, Bart.
On Tue, 2024-09-10 at 09:52 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 9/10/24 5:34 AM, Peter Wang (王信友) wrote: > > On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: > >> Simplify __ufshcd_send_uic_cmd() by always initializing the > >> uic_cmd::done completion. This is fine since the time required to > >> initialize a completion is small compared to the time required to > >> process an UIC command. > > > > The time to compare should not be with the UIC command process > time. > > It should be compared with the time for the "if (completion)" > > conditional > > expression. We cannot justify increasing the latency of sending a > UIC > > command just because the UIC command process time is longer. > > Although I appreciate your concern about performance, I don't think > that > the above feedback is appropriate. On my test setup UIC commands take > between 20 and 1500 microseconds to complete. A single > init_completion() > call takes about 0.01 microseconds. So I don't think that the time > spent > in init_completion() matters for UIC commands. Additionally, this > patch > only introduces an init_completion() call for power management > commands > and not any other type of UIC command. > > Thanks, > > Bart. > Hi Bart, I have concerns about adding latency just for power management commands. This is because when DVFS needs performance and wants to scale up, it will take extra time to complete the scaling up process. However, I agree that such a 0.01ms delay may not have a significant impact on the overall scale-up and read/write process. It might be helpful to add this information to the patch description. Thank you. Peter Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Hi Martin, UFS controllers can behave as follows with regard to link hibernation: (a) The UIC completion interrupt is not generated after having submitted a power management command. (b) The UIC completion interrupt is generated and reenabling the UIC completion interrupt does not affect the link hibernation state. (c) The UIC completion interrupt is generated and reenabling the UIC completion interrupt causes the link to exit the link hibernation state. Support these cases as follows: * Support (a) by setting UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS in the host controller driver and by disabling UIC completion interrupts before submitting a power management command. * Support (b) and (c) by leaving UIC completion interrupts enabled while submitting a power management command. Please consider this patch series for the next merge window. Thanks, Bart. Changes compared to v1 of this patch series: - A patch that improves the struct ufs_hba documentation has been added. - Patch 2/2 has been split into two patches. - Instead of leaving the UIC completion interrupt enabled, disable it if UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS has been set. Bart Van Assche (4): scsi: ufs: core: Improve the struct ufs_hba documentation scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read scsi: ufs: core: Always initialize the UIC done completion scsi: ufs: core: Change the approach for power change UIC commands drivers/ufs/core/ufshcd.c | 47 ++++++++++++++++++++------------- drivers/ufs/host/ufs-mediatek.c | 1 + drivers/ufs/host/ufs-qcom.c | 2 ++ include/ufs/ufshcd.h | 13 ++++++--- 4 files changed, 41 insertions(+), 22 deletions(-)