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: <7f5e14e5-a814-efd7-6082-c4eff697c536@collabora.com>
Date:   Wed, 31 May 2023 10:18:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v2] mmc: mtk-sd: reduce CIT for better performance
Content-Language: en-US
To:     =?UTF-8?B?V2VuYmluIE1laSAo5qKF5paH5b2sKQ==?= 
        <Wenbin.Mei@mediatek.com>,
        "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>
Cc:     "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        =?UTF-8?B?Q2hhb3RpYW4gSmluZyAo5LqV5pyd5aSpKQ==?= 
        <Chaotian.Jing@mediatek.com>
References: <20230510015851.11830-1-wenbin.mei@mediatek.com>
 <0df3968e-da34-b36c-4cb4-92d66508a46a@collabora.com>
 <e682b8b66261a71601707a4e74af2829f86800dc.camel@mediatek.com>
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <e682b8b66261a71601707a4e74af2829f86800dc.camel@mediatek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Il 31/05/23 09:32, Wenbin Mei (梅文彬) ha scritto:
> On Thu, 2023-05-18 at 11:13 +0200, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 10/05/23 03:58, Wenbin Mei ha scritto:
>>> CQHCI_SSC1 indicates to CQE the polling period to use when using
>>> periodic
>>> SEND_QUEUE_STATUS(CMD13) polling.
>>> The default value 0x1000 that corresponds to 150us, let's decrease
>>> it to
>>
>> The default value 0x1000 (4096) corresponds to 4096 * 52.08uS =
>> 231.33uS
>> ...so the default is not 150uS.
>>
>> If I'm wrong, this means that the CQCAP field is not 0, which would
>> mean
>> that the expected 3uS would be wrong.
>>
>> Also, since the calculation can be done dynamically, this is what we
>> should
>> actually do in the driver, as this gives information to the next
>> engineer
>> checking this piece of code.
>>
>> Apart from this, by just writing 0x40 to the CQHCI_SSC1 register, you
>> are
>> assuming that the CQCAP value requirement is fullfilled, but you
>> cannot
>> assume that the bootloader has set the CQCAP's ITCFVAL and ITCFMUL
>> fields
>> as you expect on all platforms: this means that implementing this
>> takes
>> a little more effort.
>>
>> You have two ways to implement this:
>>    *** First ***
>>    1. Read ITCFMUL and ITCFVAL, then:
>>       tclk_mul = itcfmul_to_mhz(ITCFMUL); /* pseudo function
>> interprets reg value*/
>>       tclk = ITCFVAL * tclk_mul;
>>
>>    2. Set SSC1 so that we get 3nS:
>>       #define CQHCI_SSC1_CIT GENMASK(15, 0)
>>       poll_time = cit_time_ns_to_regval(3);
>>       sscit = FIELD_PREP(CQHCI_SSC1_CIT, poll_time)
>>       cqhci_writel( ... )
>>
>>    *** Second **
>>
>>    1. Pre-set ITCFMUL and ITCFVAL to
>>       ITCFVAL = 192 (decimal)
>>       ITCFMUL = 2 (where 2 == 0.1MHz)
>>
>>    2. Set SSC1 so that we get 3nS:
>>       #define CQHCI_SSC1_CIT GENMASK(15, 0)
>>       poll_time = cit_time_ns_to_regval(3);
>>       sscit = FIELD_PREP(CQHCI_SSC1_CIT, poll_time)
>>       cqhci_writel( ... )
>>
>> I would implement the first way, as it paves the way to extend this
>> to different
>> tclk values if needed in the future.
>>
>> Regards,
>> Angelo
> Hi Angelo,
> 
> Sorry for lately reply.
> 
> For Mediatek mmc host IP, ITCFMUL is 0x2(0x1MHz), ITVFVAL reports 182,
> and these fields are the same and are readonly for all IC, but since
> Mediatek CQE uses msdc_hclk(273MHz), CMD13'interval calculation driver
> should use 273MHz to get the actual time, so the actual clock is
> 27.3MHz.
> 

You're right, I've misread the datasheet, just rechecked and it reports RO.

> If CIT is 0x1000 by default, CMD idle time: 0x1000 * 1 / 27.3MHz =
> around 150us.
> 
> In addition the bootloader will not set the CQCAP's ITCFVAL and ITCFMUL
> fields, because these fields of CQCAP register is RO(readonly), so we
> can ignore the change for the CQCAP's ITCFVAL and ITCFMUL fields.
> 

Yes, that's right, again - this means that you should go for the first
proposed implementation, as future MediaTek SoCs may (or may not) change
that: if you implement as proposed, this is going to be a one-time thing
and future SoCs won't need specific changes.

That implementation also documents the flow about how we're getting to
the actual value, which is important for community people reading this
driver in the future for debugging purposes.

Regards,
Angelo

> Thanks
> Wenbin
>>
>>> 0x40 that corresponds to 3us, which can improve the performance of
>>> some
>>> eMMC devices.
>>>
>>> Signed-off-by: Wenbin Mei <wenbin.mei@mediatek.com>
>>> ---
>>>    drivers/mmc/host/mtk-sd.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
>>> index edade0e54a0c..ffeccddcd028 100644
>>> --- a/drivers/mmc/host/mtk-sd.c
>>> +++ b/drivers/mmc/host/mtk-sd.c
>>> @@ -2453,6 +2453,7 @@ static void msdc_hs400_enhanced_strobe(struct
>>> mmc_host *mmc,
>>>    static void msdc_cqe_enable(struct mmc_host *mmc)
>>>    {
>>>        struct msdc_host *host = mmc_priv(mmc);
>>> +     struct cqhci_host *cq_host = mmc->cqe_private;
>>>
>>>        /* enable cmdq irq */
>>>        writel(MSDC_INT_CMDQ, host->base + MSDC_INTEN);
>>> @@ -2462,6 +2463,9 @@ static void msdc_cqe_enable(struct mmc_host
>>> *mmc)
>>>        msdc_set_busy_timeout(host, 20 * 1000000000ULL, 0);
>>>        /* default read data timeout 1s */
>>>        msdc_set_timeout(host, 1000000000ULL, 0);
>>> +
>>> +     /* decrease the send status command idle timer to 3us */
>>> +     cqhci_writel(cq_host, 0x40, CQHCI_SSC1);
>>>    }
>>>
>>>    static void msdc_cqe_disable(struct mmc_host *mmc, bool recovery)
>>
>>