Message-ID: <5d317f56-b1ba-e35a-a558-e462a70a4717@acm.org>
Date:   Mon, 13 Mar 2023 14:36:27 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v4 5/5] scsi: ufs: ufs-mediatek: Add MCQ support for MTK
 platform
Content-Language: en-US
To:     Po-Wen Kao <powen.kao@mediatek.com>, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        Stanley Chu <stanley.chu@mediatek.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Matthias Brugger <matthias.bgg@gmail.com>
Cc:     wsd_upstream@mediatek.com, peter.wang@mediatek.com,
        alice.chao@mediatek.com, naomi.chu@mediatek.com,
        chun-hung.wu@mediatek.com, cc.chou@mediatek.com,
        eddie.huang@mediatek.com, mason.zhang@mediatek.com,
        chaotian.jing@mediatek.com, jiajie.hao@mediatek.com
References: <20230307065448.15279-1-powen.kao@mediatek.com>
 <20230307065448.15279-6-powen.kao@mediatek.com>
From:   Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <20230307065448.15279-6-powen.kao@mediatek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/6/23 22:54, Po-Wen Kao wrote:
> +static unsigned int mtk_mcq_irq[UFSHCD_MAX_Q_NR];

Shouldn't there be one instance of this array per controller such that 
this driver can support multiple host controllers instead of only one?

> -	err = ufshcd_make_hba_operational(hba);
> +	if (!hba->mcq_enabled) {
> +		err = ufshcd_make_hba_operational(hba);
> +	} else {
> +		ufs_mtk_config_mcq(hba, false);
> +		ufshcd_mcq_make_queues_operational(hba);
> +		ufshcd_mcq_config_mac(hba, hba->nutrs);
> +		ufshcd_writel(hba, ufshcd_readl(hba, REG_UFS_MEM_CFG) | 0x1,
> +			      REG_UFS_MEM_CFG);
> +	}

ufshcd_config_mcq() in the UFSHCD core already calls 
ufshcd_mcq_config_mac(). Why is there another call to 
ufshcd_mcq_config_mac() in the MediaTek driver?

> +	/*
> +	 * Disable MCQ_CQ_EVENT interrupt.
> +	 * Use CQ Tail Entry Push Status instead.
> +	 */
> +	ufshcd_disable_intr(hba, MCQ_CQ_EVENT_STATUS);

UFS host controller drivers should not call ufshcd_disable_intr().

 From the UFSHCI 4.0 specification: "MCQ CQ Event Status (CQES): This 
bit is transparent and becomes ‘1’ when all of the following conditions 
are met:
• Controller is operating in MCQ mode (Config.QT=1)
• ESI is not enabled (Config.ESIE=0)
• CQES set only for Events in Queues that do not have interrupt 
aggregation enabled or the Events that do not belong to MCQIACRy.IACTH 
counter operation criteria.
• At least one bit in CQISy is set and associated bit in CQIEy is set. 
y=0..31"

Is there perhaps a bug in the MediaTek controller that causes the MCQ CQ 
Event Status to be set in ESI mode? If not, can the above 
ufshcd_disable_intr() call be left out?

Thanks,

Bart.