linux-kernel - Re: [PATCH v2] mmc: mtk-sd: reduce CIT for better performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <98a13576b874fb5dca6c9c2cdc298cb71fd3c2c4.camel@mediatek.com>
Date:   Wed, 31 May 2023 07:36:33 +0000
From:   Wenbin Mei (梅文彬) 
        <Wenbin.Mei@...iatek.com>
To:     "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Chaotian Jing (井朝天) 
        <Chaotian.Jing@...iatek.com>
Subject: Re: [PATCH v2] mmc: mtk-sd: reduce CIT for better performance

On Wed, 2023-05-31 at 07:32 +0000, Wenbin Mei (梅文彬) wrote:
> 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 
update: ITCFMUL is 0x2(0x1MHz) -> ITCFMUL is 0x2(0.1MHz)
> 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.
> 
> 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.
> 
> Thanks
> Wenbin
> > 
> > > 0x40 that corresponds to 3us, which can improve the performance
> > > of
> > > some
> > > eMMC devices.
> > > 
> > > Signed-off-by: Wenbin Mei <wenbin.mei@...iatek.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)
> > 
> >