| Message ID | 20240815-ufs-bug-fix-v2-0-b373afae888f@linaro.org |
|---|---|
| Headers | show |
| Series | ufs: qcom: Fix probe failure on SM8550 SoC due to broken LSDBS field | expand |
On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote: > /* > * The UFSHCI 3.0 specification does not define MCQ_SUPPORT and > - * LSDB_SUPPORT, but [31:29] as reserved bits with reset value 0s, which > + * LSDBS_SUPPORT, but [31:29] as reserved bits with reset value 0s, which > * means we can simply read values regardless of version. > */ Hmm ... neither MCQ_SUPPORT nor LSDBS_SUPPORT occurs in the UFSHCI 4.0 specification. I found the acronyms "MCQS" and "LSDBS" in that specification. I propose either not to modify the above comment or to use the acronyms used in the UFSHCI 4.0 standard. > hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities); > @@ -2426,7 +2426,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) > * 0h: legacy single doorbell support is available > * 1h: indicate that legacy single doorbell support has been removed > */ > - hba->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities); > + hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities); > if (!hba->mcq_sup) > return 0; The final "s" in "lsdbs" stands for "support" so there are now two references to the word "support" in the "lsdbs_sup" member name. Isn't the original structure member name ("lsdb_sup") better because it doesn't have that redundancy? > MASK_CRYPTO_SUPPORT = 0x10000000, > - MASK_LSDB_SUPPORT = 0x20000000, > + MASK_LSDBS_SUPPORT = 0x20000000, > MASK_MCQ_SUPPORT = 0x40000000, Same comment here: in the constant name "MASK_LSDBS_SUPPORT" there are two references to the word "support". Isn't the original name better? Additionally, this change introduces an inconsistency between the constant names "MASK_LSDBS_SUPPORT" and "MASK_MCQ_SUPPORT". The former name includes the acronym from the spec (LSDBS) but the latter name not (MCQS). Wouldn't it be better to leave this change out? Thanks, Bart.
On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote: > + /* > + * This quirk needs to be enabled if the host controller has the broken > + * Legacy Queue & Single Doorbell Support (LSDBS) field in Controller > + * Capabilities register. > + */ > + UFSHCD_QUIRK_BROKEN_LSDBS_CAP = 1 << 25, The above comment is misleading because it suggests that the definition of this bit in the UFSHCI specification is broken, which is not the case. How about this comment? /* * This quirk indicates that the controller reports the value 1 * (not supported) in the Legacy Single DoorBell Support (LSDBS) * bit although it supports the legacy single doorbell mode. */ Thanks, Bart.
On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote: > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 0b1787074215..8c9ff8696bcd 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2426,7 +2426,11 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) > * 0h: legacy single doorbell support is available > * 1h: indicate that legacy single doorbell support has been removed > */ > - hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities); > + if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_LSDBS_CAP)) > + hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities); > + else > + hba->lsdbs_sup = true; > + > if (!hba->mcq_sup) > return 0; An additional question: since the next patch only sets UFSHCD_QUIRK_BROKEN_LSDBS_CAP for a board with a UFSHCI 3.0 controller, do we really need the new quirk or can we replace the "!(hba->quirks & UFSHCD_QUIRK_BROKEN_LSDBS_CAP)" test with a test that verifies that the UFSHCI controller implements version 4.0 or later of the specification? Thanks, Bart.
On Thu, Aug 15, 2024 at 11:09:06AM -0700, Bart Van Assche wrote: > On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote: > > /* > > * The UFSHCI 3.0 specification does not define MCQ_SUPPORT and > > - * LSDB_SUPPORT, but [31:29] as reserved bits with reset value 0s, which > > + * LSDBS_SUPPORT, but [31:29] as reserved bits with reset value 0s, which > > * means we can simply read values regardless of version. > > */ > > Hmm ... neither MCQ_SUPPORT nor LSDBS_SUPPORT occurs in the UFSHCI 4.0 > specification. I found the acronyms "MCQS" and "LSDBS" in that > specification. I propose either not to modify the above comment or to use > the acronyms used in the UFSHCI 4.0 standard. > > > hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities); > > @@ -2426,7 +2426,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) > > * 0h: legacy single doorbell support is available > > * 1h: indicate that legacy single doorbell support has been removed > > */ > > - hba->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities); > > + hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities); > > if (!hba->mcq_sup) > > return 0; > > The final "s" in "lsdbs" stands for "support" so there are now two > references to the word "support" in the "lsdbs_sup" member name. Isn't > the original structure member name ("lsdb_sup") better because it doesn't > have that redundancy? > > > MASK_CRYPTO_SUPPORT = 0x10000000, > > - MASK_LSDB_SUPPORT = 0x20000000, > > + MASK_LSDBS_SUPPORT = 0x20000000, > > MASK_MCQ_SUPPORT = 0x40000000, > > Same comment here: in the constant name "MASK_LSDBS_SUPPORT" there are > two references to the word "support". Isn't the original name better? > Additionally, this change introduces an inconsistency between the > constant names "MASK_LSDBS_SUPPORT" and "MASK_MCQ_SUPPORT". The former > name includes the acronym from the spec (LSDBS) but the latter name not > (MCQS). Wouldn't it be better to leave this change out? > Hmm, agree. My intention was to align with the spec, but then the _SUPPORT suffix is screwing it up :/ I'll drop the patch then. - Mani
On Thu, Aug 15, 2024 at 11:25:38AM -0700, Bart Van Assche wrote: > On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote: > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index 0b1787074215..8c9ff8696bcd 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -2426,7 +2426,11 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) > > * 0h: legacy single doorbell support is available > > * 1h: indicate that legacy single doorbell support has been removed > > */ > > - hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities); > > + if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_LSDBS_CAP)) > > + hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities); > > + else > > + hba->lsdbs_sup = true; > > + > > if (!hba->mcq_sup) > > return 0; > > An additional question: since the next patch only sets > UFSHCD_QUIRK_BROKEN_LSDBS_CAP for a board with a UFSHCI 3.0 controller, > do we really need the new quirk or can we replace the "!(hba->quirks & > UFSHCD_QUIRK_BROKEN_LSDBS_CAP)" test with a test that verifies that the > UFSHCI controller implements version 4.0 or later of the specification? > Ok. First I made a mistake by believing that SM8550 is a 3.0 based controller. But by looking into the internal documentation, I learned that it is a 4.0 controller without MCQ support. So version check is not possible (and I need to fix the description as well). Also, while looking into the version info I found that the Qcom driver sets UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION quirk and the callback function get_ufs_hci_version() just hardcodes the version to 2.0. But the recent SoCs do reveal the UFSHCD version info correctly in REG_UFS_VERSION register. So the quirk might only be applicable for 2.0 controllers (not sure if those are supported now). Will check that and remove that quirk altogether. - Mani
On Thu, Aug 15, 2024 at 11:12:57AM -0700, Bart Van Assche wrote: > On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote: > > + /* > > + * This quirk needs to be enabled if the host controller has the broken > > + * Legacy Queue & Single Doorbell Support (LSDBS) field in Controller > > + * Capabilities register. > > + */ > > + UFSHCD_QUIRK_BROKEN_LSDBS_CAP = 1 << 25, > > The above comment is misleading because it suggests that the definition > of this bit in the UFSHCI specification is broken, which is not the > case. Really? I don't see where the comment implies that the bit in the specification is broken. It clearly says that the 'host controller has the broken bit'. >How about this comment? > > /* > * This quirk indicates that the controller reports the value 1 > * (not supported) in the Legacy Single DoorBell Support (LSDBS) > * bit although it supports the legacy single doorbell mode. But this comment is more elaborative. So I'll use it, thanks! - Mani
Hi, This series fixes the probe failure on the Qcom SM8550 SoC due to the broken LSDBS field in the host controller capabilities register. Please consider this series for v6.11 as it fixes a regression. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- Changes in v2: - Changed SDBS to LSDBS as per the final version of UFSHCI 4.0 spec - Moved the quirk check to assignment - Used correct fixes tag in patch 3/3 - Added tested-by tags - Link to v1: https://lore.kernel.org/r/20240814-ufs-bug-fix-v1-0-5eb49d5f7571@linaro.org --- Manivannan Sadhasivam (3): ufs: core: Rename LSDB to LSDBS to reflect the UFSHCI 4.0 spec ufs: core: Add a quirk for handling broken LSDBS field in controller capabilities register ufs: qcom: Add UFSHCD_QUIRK_BROKEN_LSDBS_CAP for SM8550 SoC drivers/ufs/core/ufshcd.c | 10 +++++++--- drivers/ufs/host/ufs-qcom.c | 6 +++++- include/ufs/ufshcd.h | 9 ++++++++- include/ufs/ufshci.h | 2 +- 4 files changed, 21 insertions(+), 6 deletions(-) --- base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba change-id: 20240814-ufs-bug-fix-4427fb01b860 Best regards,