| Message ID | 20250404174539.28707-3-quic_mapa@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: Implement Quirks for Samsung UFS Devices | expand |
On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: > Some UFS devices need additional time in hibern8 mode before exiting, > beyond the negotiated handshaking phase between the host and device. > Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs > to ensure proper hibernation process. > This commit message didn't mention the UFS device for which this quirk is being applied. > Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > --- > drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ > include/ufs/ufs_quirks.h | 6 ++++++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 464f13da259a..2b8203fe7b8c 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { > .model = UFS_ANY_MODEL, > .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | > UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | > + UFS_DEVICE_QUIRK_PA_HIBER8TIME | > UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, > { .wmanufacturerid = UFS_VENDOR_SKHYNIX, > .model = UFS_ANY_MODEL, > @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) > return ret; > } > > +/** > + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. > + * @hba: per-adapter instance > + * > + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter > + * to ensure proper hibernation timing. This function retrieves the current > + * PA_HIBERN8TIME value and increments it by 100us. > + */ > +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) > +{ > + u32 pa_h8time = 0; Why do you need to initialize it? > + int ret; > + > + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), > + &pa_h8time); > + if (ret) { > + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); > + return; > + } > + > + /* Increment by 1 to increase hibernation time by 100 µs */ From where the value of 100us adjustment is coming from? - Mani
On 4/7/2025 12:05 AM, Manivannan Sadhasivam wrote: > On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: >> Some UFS devices need additional time in hibern8 mode before exiting, >> beyond the negotiated handshaking phase between the host and device. >> Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs >> to ensure proper hibernation process. >> > > This commit message didn't mention the UFS device for which this quirk is being > applied. > Since it's a quirk and may be applicable to other vendors also in future, so i thought to keep it general. Will update in next patch set if required. >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> >> --- >> drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ >> include/ufs/ufs_quirks.h | 6 ++++++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 464f13da259a..2b8203fe7b8c 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { >> .model = UFS_ANY_MODEL, >> .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | >> UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | >> + UFS_DEVICE_QUIRK_PA_HIBER8TIME | >> UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, >> { .wmanufacturerid = UFS_VENDOR_SKHYNIX, >> .model = UFS_ANY_MODEL, >> @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) >> return ret; >> } >> >> +/** >> + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. >> + * @hba: per-adapter instance >> + * >> + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter >> + * to ensure proper hibernation timing. This function retrieves the current >> + * PA_HIBERN8TIME value and increments it by 100us. >> + */ >> +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) >> +{ >> + u32 pa_h8time = 0; > > Why do you need to initialize it? > Agree.. Not needed, will update.>> + int ret; >> + >> + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), >> + &pa_h8time); >> + if (ret) { >> + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); >> + return; >> + } >> + >> + /* Increment by 1 to increase hibernation time by 100 µs */ > > From where the value of 100us adjustment is coming from? > > - Mani > These values are derived from experiments on Qualcomm SoCs. However this is also matching with ufs-exynos.c fsd_ufs_post_link() { ufshcd_dme_get(hba,UIC_ARG_MIB(PA_HIBERN8TIME), &max_rx_hibern8_time_cap); ....... ufshcd_dme_set(hba, UIC_ARG_MIB(PA_HIBERN8TIME), max_rx_hibern8_time_cap + 1); ... } Thanks Manish
On Tue, Apr 08, 2025 at 11:07:58AM +0530, MANISH PANDEY wrote: > > > On 4/7/2025 12:05 AM, Manivannan Sadhasivam wrote: > > On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: > > > Some UFS devices need additional time in hibern8 mode before exiting, > > > beyond the negotiated handshaking phase between the host and device. > > > Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs > > > to ensure proper hibernation process. > > > > > > > This commit message didn't mention the UFS device for which this quirk is being > > applied. > > > Since it's a quirk and may be applicable to other vendors also in future, so > i thought to keep it general. > You cannot make commit message generic. It should precisely describe the change. > Will update in next patch set if required. > >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > > > --- > > > drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ > > > include/ufs/ufs_quirks.h | 6 ++++++ > > > 2 files changed, 37 insertions(+) > > > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > > index 464f13da259a..2b8203fe7b8c 100644 > > > --- a/drivers/ufs/core/ufshcd.c > > > +++ b/drivers/ufs/core/ufshcd.c > > > @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { > > > .model = UFS_ANY_MODEL, > > > .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | > > > UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | > > > + UFS_DEVICE_QUIRK_PA_HIBER8TIME | > > > UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, > > > { .wmanufacturerid = UFS_VENDOR_SKHYNIX, > > > .model = UFS_ANY_MODEL, > > > @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) > > > return ret; > > > } > > > +/** > > > + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. > > > + * @hba: per-adapter instance > > > + * > > > + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter > > > + * to ensure proper hibernation timing. This function retrieves the current > > > + * PA_HIBERN8TIME value and increments it by 100us. > > > + */ > > > +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) > > > +{ > > > + u32 pa_h8time = 0; > > > > Why do you need to initialize it? > > > Agree.. Not needed, will update.>> + int ret; > > > + > > > + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), > > > + &pa_h8time); > > > + if (ret) { > > > + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); > > > + return; > > > + } > > > + > > > + /* Increment by 1 to increase hibernation time by 100 µs */ > > > > From where the value of 100us adjustment is coming from? > > > > - Mani > > > These values are derived from experiments on Qualcomm SoCs. > However this is also matching with ufs-exynos.c > Okay. In that case, you should mention that the 100us value is derived from experiments on Qcom and Samsung SoCs. Otherwise, it gives an assumption that this value is universal. - Mani
On 4/8/2025 12:53 PM, Manivannan Sadhasivam wrote: > On Tue, Apr 08, 2025 at 11:07:58AM +0530, MANISH PANDEY wrote: >> >> >> On 4/7/2025 12:05 AM, Manivannan Sadhasivam wrote: >>> On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: >>>> Some UFS devices need additional time in hibern8 mode before exiting, >>>> beyond the negotiated handshaking phase between the host and device. >>>> Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs >>>> to ensure proper hibernation process. >>>> >>> >>> This commit message didn't mention the UFS device for which this quirk is being >>> applied. >>> >> Since it's a quirk and may be applicable to other vendors also in future, so >> i thought to keep it general. >> > > You cannot make commit message generic. It should precisely describe the change. > >> Will update in next patch set if required. >> >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> >>>> --- >>>> drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ >>>> include/ufs/ufs_quirks.h | 6 ++++++ >>>> 2 files changed, 37 insertions(+) >>>> >>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >>>> index 464f13da259a..2b8203fe7b8c 100644 >>>> --- a/drivers/ufs/core/ufshcd.c >>>> +++ b/drivers/ufs/core/ufshcd.c >>>> @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { >>>> .model = UFS_ANY_MODEL, >>>> .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | >>>> UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | >>>> + UFS_DEVICE_QUIRK_PA_HIBER8TIME | >>>> UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, >>>> { .wmanufacturerid = UFS_VENDOR_SKHYNIX, >>>> .model = UFS_ANY_MODEL, >>>> @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) >>>> return ret; >>>> } >>>> +/** >>>> + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. >>>> + * @hba: per-adapter instance >>>> + * >>>> + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter >>>> + * to ensure proper hibernation timing. This function retrieves the current >>>> + * PA_HIBERN8TIME value and increments it by 100us. >>>> + */ >>>> +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) >>>> +{ >>>> + u32 pa_h8time = 0; >>> >>> Why do you need to initialize it? >>> >> Agree.. Not needed, will update.>> + int ret; >>>> + >>>> + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), >>>> + &pa_h8time); >>>> + if (ret) { >>>> + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); >>>> + return; >>>> + } >>>> + >>>> + /* Increment by 1 to increase hibernation time by 100 µs */ >>> >>> From where the value of 100us adjustment is coming from? >>> >>> - Mani >>> >> These values are derived from experiments on Qualcomm SoCs. >> However this is also matching with ufs-exynos.c >> > > Okay. In that case, you should mention that the 100us value is derived from > experiments on Qcom and Samsung SoCs. Otherwise, it gives an assumption that > this value is universal. > > - Mani > << Otherwise, it gives an assumption that this value is universal. >> So with this, should i add this quirk for Qcom only, or should add in ufshcd.c and make it common for all SoC vendors? Regards Manish
On Tue, Apr 08, 2025 at 01:14:50PM +0530, MANISH PANDEY wrote: > > > On 4/8/2025 12:53 PM, Manivannan Sadhasivam wrote: > > On Tue, Apr 08, 2025 at 11:07:58AM +0530, MANISH PANDEY wrote: > > > > > > > > > On 4/7/2025 12:05 AM, Manivannan Sadhasivam wrote: > > > > On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: > > > > > Some UFS devices need additional time in hibern8 mode before exiting, > > > > > beyond the negotiated handshaking phase between the host and device. > > > > > Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs > > > > > to ensure proper hibernation process. > > > > > > > > > > > > > This commit message didn't mention the UFS device for which this quirk is being > > > > applied. > > > > > > > Since it's a quirk and may be applicable to other vendors also in future, so > > > i thought to keep it general. > > > > > > > You cannot make commit message generic. It should precisely describe the change. > > > > > Will update in next patch set if required. > > > >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > > > > > --- > > > > > drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ > > > > > include/ufs/ufs_quirks.h | 6 ++++++ > > > > > 2 files changed, 37 insertions(+) > > > > > > > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > > > > index 464f13da259a..2b8203fe7b8c 100644 > > > > > --- a/drivers/ufs/core/ufshcd.c > > > > > +++ b/drivers/ufs/core/ufshcd.c > > > > > @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { > > > > > .model = UFS_ANY_MODEL, > > > > > .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | > > > > > UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | > > > > > + UFS_DEVICE_QUIRK_PA_HIBER8TIME | > > > > > UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, > > > > > { .wmanufacturerid = UFS_VENDOR_SKHYNIX, > > > > > .model = UFS_ANY_MODEL, > > > > > @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) > > > > > return ret; > > > > > } > > > > > +/** > > > > > + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. > > > > > + * @hba: per-adapter instance > > > > > + * > > > > > + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter > > > > > + * to ensure proper hibernation timing. This function retrieves the current > > > > > + * PA_HIBERN8TIME value and increments it by 100us. > > > > > + */ > > > > > +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) > > > > > +{ > > > > > + u32 pa_h8time = 0; > > > > > > > > Why do you need to initialize it? > > > > > > > Agree.. Not needed, will update.>> + int ret; > > > > > + > > > > > + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), > > > > > + &pa_h8time); > > > > > + if (ret) { > > > > > + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); > > > > > + return; > > > > > + } > > > > > + > > > > > + /* Increment by 1 to increase hibernation time by 100 µs */ > > > > > > > > From where the value of 100us adjustment is coming from? > > > > > > > > - Mani > > > > > > > These values are derived from experiments on Qualcomm SoCs. > > > However this is also matching with ufs-exynos.c > > > > > > > Okay. In that case, you should mention that the 100us value is derived from > > experiments on Qcom and Samsung SoCs. Otherwise, it gives an assumption that > > this value is universal. > > > > - Mani > > > << Otherwise, it gives an assumption that this value is universal. >> > So with this, should i add this quirk for Qcom only, or should add in > ufshcd.c and make it common for all SoC vendors? > You can add the quirk for both Qcom and Samsung. My comment was about clarifying it in the kernel doc comments. - Mani
On 4/8/2025 10:01 PM, Manivannan Sadhasivam wrote: > On Tue, Apr 08, 2025 at 01:14:50PM +0530, MANISH PANDEY wrote: >> >> >> On 4/8/2025 12:53 PM, Manivannan Sadhasivam wrote: >>> On Tue, Apr 08, 2025 at 11:07:58AM +0530, MANISH PANDEY wrote: >>>> >>>> >>>> On 4/7/2025 12:05 AM, Manivannan Sadhasivam wrote: >>>>> On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: >>>>>> Some UFS devices need additional time in hibern8 mode before exiting, >>>>>> beyond the negotiated handshaking phase between the host and device. >>>>>> Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs >>>>>> to ensure proper hibernation process. >>>>>> >>>>> >>>>> This commit message didn't mention the UFS device for which this quirk is being >>>>> applied. >>>>> >>>> Since it's a quirk and may be applicable to other vendors also in future, so >>>> i thought to keep it general. >>>> >>> >>> You cannot make commit message generic. It should precisely describe the change. >>> >>>> Will update in next patch set if required. >>>> >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> >>>>>> --- >>>>>> drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ >>>>>> include/ufs/ufs_quirks.h | 6 ++++++ >>>>>> 2 files changed, 37 insertions(+) >>>>>> >>>>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >>>>>> index 464f13da259a..2b8203fe7b8c 100644 >>>>>> --- a/drivers/ufs/core/ufshcd.c >>>>>> +++ b/drivers/ufs/core/ufshcd.c >>>>>> @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { >>>>>> .model = UFS_ANY_MODEL, >>>>>> .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | >>>>>> UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | >>>>>> + UFS_DEVICE_QUIRK_PA_HIBER8TIME | >>>>>> UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, >>>>>> { .wmanufacturerid = UFS_VENDOR_SKHYNIX, >>>>>> .model = UFS_ANY_MODEL, >>>>>> @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) >>>>>> return ret; >>>>>> } >>>>>> +/** >>>>>> + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. >>>>>> + * @hba: per-adapter instance >>>>>> + * >>>>>> + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter >>>>>> + * to ensure proper hibernation timing. This function retrieves the current >>>>>> + * PA_HIBERN8TIME value and increments it by 100us. >>>>>> + */ >>>>>> +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) >>>>>> +{ >>>>>> + u32 pa_h8time = 0; >>>>> >>>>> Why do you need to initialize it? >>>>> >>>> Agree.. Not needed, will update.>> + int ret; >>>>>> + >>>>>> + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), >>>>>> + &pa_h8time); >>>>>> + if (ret) { >>>>>> + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + /* Increment by 1 to increase hibernation time by 100 µs */ >>>>> >>>>> From where the value of 100us adjustment is coming from? >>>>> >>>>> - Mani >>>>> >>>> These values are derived from experiments on Qualcomm SoCs. >>>> However this is also matching with ufs-exynos.c >>>> >>> >>> Okay. In that case, you should mention that the 100us value is derived from >>> experiments on Qcom and Samsung SoCs. Otherwise, it gives an assumption that >>> this value is universal. >>> >>> - Mani >>> >> << Otherwise, it gives an assumption that this value is universal. >> >> So with this, should i add this quirk for Qcom only, or should add in >> ufshcd.c and make it common for all SoC vendors? >> > > You can add the quirk for both Qcom and Samsung. My comment was about clarifying > it in the kernel doc comments. > > - Mani > Just for conclusion, why i moved this quirk from ufs-qcom to ufshcd.c is as per Bart's suggestion in patchset https://lore.kernel.org/lkml/c0691392-1523-4863-a722-d4f4640e4e28@acm.org/ << Which of these quirks are required for all host controllers and which of these quirks are only required for Qualcomm host controllers? > Should we consider moving the QUIRK_PA_HIBER8TIME quirk to the ufshcd > driver? Please advise. That would be appreciated. >> Just to brief, QUIRK_PA_HIBER8TIME is required for Samsung UFS devices and may be applicable to all SoC vendors with Samsung ufs device. If you suggest to move it to ufs-qcom.c, i don't have any concern. BTW Samsung UFS driver already has this implemented in their driver, So i need not have to add this quirk to Samsung driver (ufs-exynos.c). Regards Manish
On Tue, Apr 08, 2025 at 10:58:10PM +0530, MANISH PANDEY wrote: > > > On 4/8/2025 10:01 PM, Manivannan Sadhasivam wrote: > > On Tue, Apr 08, 2025 at 01:14:50PM +0530, MANISH PANDEY wrote: > > > > > > > > > On 4/8/2025 12:53 PM, Manivannan Sadhasivam wrote: > > > > On Tue, Apr 08, 2025 at 11:07:58AM +0530, MANISH PANDEY wrote: > > > > > > > > > > > > > > > On 4/7/2025 12:05 AM, Manivannan Sadhasivam wrote: > > > > > > On Fri, Apr 04, 2025 at 11:15:39PM +0530, Manish Pandey wrote: > > > > > > > Some UFS devices need additional time in hibern8 mode before exiting, > > > > > > > beyond the negotiated handshaking phase between the host and device. > > > > > > > Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs > > > > > > > to ensure proper hibernation process. > > > > > > > > > > > > > > > > > > > This commit message didn't mention the UFS device for which this quirk is being > > > > > > applied. > > > > > > > > > > > Since it's a quirk and may be applicable to other vendors also in future, so > > > > > i thought to keep it general. > > > > > > > > > > > > > You cannot make commit message generic. It should precisely describe the change. > > > > > > > > > Will update in next patch set if required. > > > > > >> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> > > > > > > > --- > > > > > > > drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ > > > > > > > include/ufs/ufs_quirks.h | 6 ++++++ > > > > > > > 2 files changed, 37 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > > > > > > index 464f13da259a..2b8203fe7b8c 100644 > > > > > > > --- a/drivers/ufs/core/ufshcd.c > > > > > > > +++ b/drivers/ufs/core/ufshcd.c > > > > > > > @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { > > > > > > > .model = UFS_ANY_MODEL, > > > > > > > .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | > > > > > > > UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | > > > > > > > + UFS_DEVICE_QUIRK_PA_HIBER8TIME | > > > > > > > UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, > > > > > > > { .wmanufacturerid = UFS_VENDOR_SKHYNIX, > > > > > > > .model = UFS_ANY_MODEL, > > > > > > > @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) > > > > > > > return ret; > > > > > > > } > > > > > > > +/** > > > > > > > + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. > > > > > > > + * @hba: per-adapter instance > > > > > > > + * > > > > > > > + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter > > > > > > > + * to ensure proper hibernation timing. This function retrieves the current > > > > > > > + * PA_HIBERN8TIME value and increments it by 100us. > > > > > > > + */ > > > > > > > +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) > > > > > > > +{ > > > > > > > + u32 pa_h8time = 0; > > > > > > > > > > > > Why do you need to initialize it? > > > > > > > > > > > Agree.. Not needed, will update.>> + int ret; > > > > > > > + > > > > > > > + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), > > > > > > > + &pa_h8time); > > > > > > > + if (ret) { > > > > > > > + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); > > > > > > > + return; > > > > > > > + } > > > > > > > + > > > > > > > + /* Increment by 1 to increase hibernation time by 100 µs */ > > > > > > > > > > > > From where the value of 100us adjustment is coming from? > > > > > > > > > > > > - Mani > > > > > > > > > > > These values are derived from experiments on Qualcomm SoCs. > > > > > However this is also matching with ufs-exynos.c > > > > > > > > > > > > > Okay. In that case, you should mention that the 100us value is derived from > > > > experiments on Qcom and Samsung SoCs. Otherwise, it gives an assumption that > > > > this value is universal. > > > > > > > > - Mani > > > > > > > << Otherwise, it gives an assumption that this value is universal. >> > > > So with this, should i add this quirk for Qcom only, or should add in > > > ufshcd.c and make it common for all SoC vendors? > > > > > > > You can add the quirk for both Qcom and Samsung. My comment was about clarifying > > it in the kernel doc comments. > > > > - Mani > > > Just for conclusion, why i moved this quirk from ufs-qcom to ufshcd.c is as > per Bart's suggestion in patchset > https://lore.kernel.org/lkml/c0691392-1523-4863-a722-d4f4640e4e28@acm.org/ > > << Which of these quirks are required for all host controllers and which of > these quirks are only required for Qualcomm host controllers? > > > Should we consider moving the QUIRK_PA_HIBER8TIME quirk to the ufshcd > > driver? Please advise. > > That would be appreciated. >> > > Just to brief, QUIRK_PA_HIBER8TIME is required for Samsung UFS devices and > may be applicable to all SoC vendors with Samsung ufs device. > > If you suggest to move it to ufs-qcom.c, i don't have any concern. > BTW Samsung UFS driver already has this implemented in their driver, > So i need not have to add this quirk to Samsung driver (ufs-exynos.c). > No. I think there was a miscommunication. You can add the quirk in ufshcd.c, but I just want you to mention that the quirk is currently applicable to Samsung UFS devices and the value of 100us is derived from experiments. - Mani
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 464f13da259a..2b8203fe7b8c 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -278,6 +278,7 @@ static const struct ufs_dev_quirk ufs_fixups[] = { .model = UFS_ANY_MODEL, .quirk = UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM | UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE | + UFS_DEVICE_QUIRK_PA_HIBER8TIME | UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS }, { .wmanufacturerid = UFS_VENDOR_SKHYNIX, .model = UFS_ANY_MODEL, @@ -8384,6 +8385,33 @@ static int ufshcd_quirk_tune_host_pa_tactivate(struct ufs_hba *hba) return ret; } +/** + * ufshcd_quirk_override_pa_h8time - Ensures proper adjustment of PA_HIBERN8TIME. + * @hba: per-adapter instance + * + * Some UFS devices require specific adjustments to the PA_HIBERN8TIME parameter + * to ensure proper hibernation timing. This function retrieves the current + * PA_HIBERN8TIME value and increments it by 100us. + */ +static void ufshcd_quirk_override_pa_h8time(struct ufs_hba *hba) +{ + u32 pa_h8time = 0; + int ret; + + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_HIBERN8TIME), + &pa_h8time); + if (ret) { + dev_err(hba->dev, "Failed to get PA_HIBERN8TIME: %d\n", ret); + return; + } + + /* Increment by 1 to increase hibernation time by 100 µs */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(PA_HIBERN8TIME), + pa_h8time + 1); + if (ret) + dev_err(hba->dev, "Failed updating PA_HIBERN8TIME: %d\n", ret); +} + static void ufshcd_tune_unipro_params(struct ufs_hba *hba) { ufshcd_vops_apply_dev_quirks(hba); @@ -8394,6 +8422,9 @@ static void ufshcd_tune_unipro_params(struct ufs_hba *hba) if (hba->dev_quirks & UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE) ufshcd_quirk_tune_host_pa_tactivate(hba); + + if (hba->dev_quirks & UFS_DEVICE_QUIRK_PA_HIBER8TIME) + ufshcd_quirk_override_pa_h8time(hba); } static void ufshcd_clear_dbg_ufs_stats(struct ufs_hba *hba) diff --git a/include/ufs/ufs_quirks.h b/include/ufs/ufs_quirks.h index 41ff44dfa1db..f52de5ed1b3b 100644 --- a/include/ufs/ufs_quirks.h +++ b/include/ufs/ufs_quirks.h @@ -107,4 +107,10 @@ struct ufs_dev_quirk { */ #define UFS_DEVICE_QUIRK_DELAY_AFTER_LPM (1 << 11) +/* + * Some ufs devices may need more time to be in hibern8 before exiting. + * Enable this quirk to give it an additional 100us. + */ +#define UFS_DEVICE_QUIRK_PA_HIBER8TIME (1 << 12) + #endif /* UFS_QUIRKS_H_ */
Some UFS devices need additional time in hibern8 mode before exiting, beyond the negotiated handshaking phase between the host and device. Introduce a quirk to increase the PA_HIBERN8TIME parameter by 100 µs to ensure proper hibernation process. Signed-off-by: Manish Pandey <quic_mapa@quicinc.com> --- drivers/ufs/core/ufshcd.c | 31 +++++++++++++++++++++++++++++++ include/ufs/ufs_quirks.h | 6 ++++++ 2 files changed, 37 insertions(+)