On Thu, Aug 13, 2020 at 06:51:13AM +0530, Alim Akhtar wrote: > Hi Eric, > > > -----Original Message----- > > From: Eric Biggers <[email protected]> > > Sent: 12 August 2020 05:59 > > To: Alim Akhtar <[email protected]> > > Cc: [email protected]; [email protected]; > [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; linux-arm- > > [email protected]; [email protected]; [email protected] > > Subject: Re: [RESEND PATCH v10 04/10] scsi: ufs: introduce > > UFSHCD_QUIRK_PRDT_BYTE_GRAN quirk > > > > Hi Alim, > > > > On Sat, Jun 13, 2020 at 08:17:00AM +0530, Alim Akhtar wrote: > > > Some UFS host controllers like Exynos uses granularities of PRDT > > > length and offset as bytes, whereas others uses actual segment count. > > > > > > Reviewed-by: Avri Altman <[email protected]> > > > Signed-off-by: Kiwoong Kim <[email protected]> > > > Signed-off-by: Alim Akhtar <[email protected]> > > > --- > > > drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++++++++++------- > > > drivers/scsi/ufs/ufshcd.h | 6 ++++++ > > > 2 files changed, 29 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > > index ee30ed6cc805..ba093d0d0942 100644 > > > --- a/drivers/scsi/ufs/ufshcd.c > > > +++ b/drivers/scsi/ufs/ufshcd.c > > > @@ -2151,8 +2151,14 @@ static int ufshcd_map_sg(struct ufs_hba *hba, > > struct ufshcd_lrb *lrbp) > > > return sg_segments; > > > > > > if (sg_segments) { > > > - lrbp->utr_descriptor_ptr->prd_table_length = > > > - cpu_to_le16((u16)sg_segments); > > > + > > > + if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) > > > + lrbp->utr_descriptor_ptr->prd_table_length = > > > + cpu_to_le16((sg_segments * > > > + sizeof(struct ufshcd_sg_entry))); > > > + else > > > + lrbp->utr_descriptor_ptr->prd_table_length = > > > + cpu_to_le16((u16) (sg_segments)); > > > > > > prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr; > > > > > > @@ -3500,11 +3506,21 @@ static void > > ufshcd_host_memory_configure(struct ufs_hba *hba) > > > > > cpu_to_le32(upper_32_bits(cmd_desc_element_addr)); > > > > > > /* Response upiu and prdt offset should be in double words > */ > > > - utrdlp[i].response_upiu_offset = > > > - cpu_to_le16(response_offset >> 2); > > > - utrdlp[i].prd_table_offset = cpu_to_le16(prdt_offset >> 2); > > > - utrdlp[i].response_upiu_length = > > > - cpu_to_le16(ALIGNED_UPIU_SIZE >> 2); > > > + if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) { > > > + utrdlp[i].response_upiu_offset = > > > + cpu_to_le16(response_offset); > > > + utrdlp[i].prd_table_offset = > > > + cpu_to_le16(prdt_offset); > > > + utrdlp[i].response_upiu_length = > > > + cpu_to_le16(ALIGNED_UPIU_SIZE); > > > + } else { > > > + utrdlp[i].response_upiu_offset = > > > + cpu_to_le16(response_offset >> 2); > > > + utrdlp[i].prd_table_offset = > > > + cpu_to_le16(prdt_offset >> 2); > > > + utrdlp[i].response_upiu_length = > > > + cpu_to_le16(ALIGNED_UPIU_SIZE >> 2); > > > + } > > > > > > ufshcd_init_lrb(hba, &hba->lrb[i], i); > > > } > > > > Isn't this patch missing an update to ufshcd_print_trs()? It uses > > ->prd_table_length as the number of segments, not the number of bytes. > > > prd_table_length will be populated before it reaches ufshcd_print_trs() > based on UFSHCD_QUIRK_PRDT_BYTE_GRAN. >
Yes, which is why it seems ufshcd_print_trs() needs to be updated to take UFSHCD_QUIRK_PRDT_BYTE_GRAN into account. - Eric
