On Mon, 2023-03-13 at 14:36 -0700, Bart Van Assche wrote:
> 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?
>
True, I will fix the flow to get irq after `struct ufs_hba` is
allocated so that these infomation can be stored as per host instance.
> > - 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?
MCQ configuration will be reset through HCE cycle on our host
controller, hence we need to reconfigure those registers.
>
> > + /*
> > + * 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?
We did not implement ESI at hardware level but per queue hw interrupt.
Without disabling MCQ_CQ_EVENT_STATUS, there will be two interrupts,
CQES (traditional interrupt) and CQ Tail Entry Push Interrupt (per
queue hw interrupt), raised on a signle command arrival.
I wouldn't consider it as a bug, but different interrupt design.
>
> Thanks,
>
> Bart.
>
Thanks for your review
Powen
