On Thu, 2023-06-01 at 12:00 +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 01/06/23 05:16, Wenbin Mei (梅文彬) ha scritto:
> > On Wed, 2023-05-31 at 10:18 +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 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 for your proposal.
> >
> >
> > I have discussed with our designer, and this fields of CQCAP's
> ITCFVAL and ITCFMUL will not change.
> > If we add more code for it, these codes will also affect the
> execution efficiency, even if it has a very
> > small effect.
> > I think if it's just for reading convenience, we can add mode
> comments to make it easier to read the code.
> > Do you think it's okay to add more comments?
> >
>
> This isn't a performance path, but anyway, if you think that it will
> be at some
> point, you can read the two registers at probe time as part of the
> MMC_CAP2_CQE
> if branch, and then cache the invariable values to `struct
> msdc_host`: this
> will make you able to never perform register reads for ITCFVAL/FMUL
> in
> msdc_cqe_enable(), resolving the efficiency issue.
>
> Even better, instead of caching ITCFVAL/FMUL to two variables, since
> the idle
> timer value likely won't ever change during runtime, you can directly
> perform
> the calculation for SSC1 at probe time and cache that value instead,
> so that
> in msdc_cqe_enable() you will have something like...
>
> /* Set the send status command idle timer */
> cqhci_writel(cq_host, host->cq_ssc1_time, CQHCI_SSC1);
>
> where cq_ssc1_time is
> struct msdc_host {
> .......
> u32 cq_ssc1_time;
> ....
> }
>
> and where your probe function is
>
> static int msdc_drv_probe(struct platform_device *pdev)
> {
> ......
>
> if (mmc->caps2 & MMC_CAP2_CQE) {
> host->cq_host = ......
> ........
> read itcfval;
> read itcfmul;
> host->cq_ssc1_time = calculated-value;
> ........
> }
>
> .......
> }
>
Yes, I think it's okay for me.
Another problem, ITCFVAL reports 182 for MediaTek SoCs, but we can not
use it to calculate, as i said earlier, since our CQE uses
msdc_hclk(273MHz), CMD13' interval calculation drivers should use
273MHz to get the actual time, not 182MHz.
If we use ITCFVAL, we will get a wrong value.
So I think it's meaningless.
Begards,
Wenbin
> Regards,
> Angelo
>
>
> > Begards,
> > Wenbin
> >
> >> Thanks
> >
> >> Wenbin
> >
> >>>
> >
> >>>> 0x40 that corresponds to 3us, which can improve the performance
> of
> >
> >>>> some
> >
> >>>> eMMC devices.
> >
> >>>>
> >
> >>>> Signed-off-by: Wenbin Mei <wenbin.mei@xxxxxxxxxxxx>
> >
> >>>> ---
> >
> >>>> 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)
> >
> >>>
> >
> >>>
> >
> >
> >
>
