Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cd1c0266-a1be-f6c3-fbf3-0b75ecf4e3df@quicinc.com>
Date:   Thu, 18 Aug 2022 19:57:11 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.0.3
Subject: Re: [RFC PATCH v2 1/2] scsi: ufs: Add Multi-Circular Queue support
To:     Manivannan Sadhasivam <mani@kernel.org>,
        Can Guo <quic_cang@quicinc.com>
CC:     <quic_nguyenb@quicinc.com>, <quic_xiaosenh@quicinc.com>,
        <stanley.chu@mediatek.com>, <adrian.hunter@intel.com>,
        <bvanassche@acm.org>, <beanhuo@micron.com>, <avri.altman@wdc.com>,
        <linux-scsi@vger.kernel.org>, <kernel-team@android.com>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Jinyoung Choi <j-young.choi@samsung.com>,
        jongmin jeong <jjmin.jeong@samsung.com>,
        Kiwoong Kim <kwmad.kim@samsung.com>,
        open list <linux-kernel@vger.kernel.org>
References: <1660213984-37793-1-git-send-email-quic_cang@quicinc.com>
 <1660213984-37793-2-git-send-email-quic_cang@quicinc.com>
 <20220812091012.GB4956@thinkpad>
From:   "Asutosh Das (asd)" <quic_asutoshd@quicinc.com>
In-Reply-To: <20220812091012.GB4956@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.
>>
>