Messages in this thread | | | Date | Thu, 18 Aug 2022 19:57:11 -0700 | Subject | Re: [RFC PATCH v2 1/2] scsi: ufs: Add Multi-Circular Queue support | From | "Asutosh Das (asd)" <> |
| |
Hello Manivannan,
Thanks for taking a look.
Sorry for the late reponse, was a bit caught up.
On 8/12/2022 2:10 AM, Manivannan Sadhasivam wrote: > On Thu, Aug 11, 2022 at 03:33:03AM -0700, Can Guo wrote: >> From: Asutosh Das <quic_asutoshd@quicinc.com> >> >> Adds MCQ support to UFS. >> >> The design uses shared tags across all the hw queues. >> The queue-depth is chosen within range supported by controller & >> device. It also takes EXT_IID into account while choosing the queue >> depth. >> >> It supports default, read, poll and a dev cmd queue. >> It enables MCQ after determining the queue-depth that the ufs >> device supports. >> >> Co-developed-by: Can Guo <quic_cang@quicinc.com> > > As per the Documentation, "Co-developed-by" should be followed by > "Signed-off-by" of the author. So you can just move this "Co-developed-by" > below Asutosh's S-o-b and that should be fine. > Ok. >> Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> --- >> drivers/ufs/core/Makefile | 2 +- >> drivers/ufs/core/ufs-mcq.c | 524 +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/ufs/core/ufshcd.c | 394 ++++++++++++++++++++++++++-------- >> include/ufs/ufs.h | 5 + >> include/ufs/ufshcd.h | 223 ++++++++++++++++++- >> include/ufs/ufshci.h | 77 +++++++ >> 6 files changed, 1135 insertions(+), 90 deletions(-) > > This patch is too big to review. Could you please split it into multiple > patches? > The current changes make up a single functional base MCQ driver. Usually splitting into multiple changes are per feature based. Since this is a single feature, it makes sense to be separated out into core and host only. Refer 7a3e97b0d - the base ufs driver.
> > > s/qd/depth > Here queue depth is abbreviated to qd which I think is inline with what the function does. So I prefer to keep it as qd.
>> + >> + val = ufshcd_readl(hba, REG_UFS_MCQ_CFG); > > >> +struct ufs_hw_queue { >> + void *sqe_base_addr; > > s/sqe_base_addr/sqe_base > >> + dma_addr_t sqe_dma_addr; > > s/sqe_dma_addr/sqe_dma > >> + struct cq_entry *cqe_base_addr; > > s/cqe_base_addr/cqe_base > >> + dma_addr_t cqe_dma_addr; > > s/cqe_dma_addr/cqe_dma > Existing ufs driver dma addresses have the suffix addr, Refer struct ufshcd_lrb. So it is in line with the current naming convention.
>> + u32 max_entries; >> + u32 id; >> + >> + void __iomem *mcq_sq_hp; >> + void __iomem *mcq_sq_tp; >> + void __iomem *mcq_cq_hp; >> + void __iomem *mcq_cq_tp; [...]
>> + MCQ_CFG_n(REG_SQATTR, i)); > If you are writing to the same memory region, like in this case "mcq_base", > then you should be able to use _relaxed variants as the writes to the same > device memory are guaranteed to be in-order. > Not sure if I understand this correctly. Let me check this internally and get back.
> This also removes the overhead associated with __iowmb included in writel. > > Please audit this change throught the driver and use _relaxed variants where > applicable. >
-asd
>> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. >> >
| |