Re: [Linux-stm32] [PATCH V3 1/3] mmc: mmci: fix read status for busy detect

[Ludovic BARRE]

hi Ulf

On 7/26/19 11:41 AM, Ludovic BARRE wrote:
> hi Ulf
>
> Thanks to your "Clarify comments ..." commit, like is closes
> I resumed upstream of this series.
>
> On 7/15/19 6:31 PM, Ulf Hansson wrote:
> > On Mon, 3 Jun 2019 at 17:55, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> > > From: Ludovic Barre <ludovic.barre@xxxxxx>
> > >
> > > "busy_detect_flag" is used to read & clear busy value of mmci status.
> > > "busy_detect_mask" is used to manage busy irq of mmci mask.
> > > So to read mmci status the busy_detect_flag must be take account.
> > > if the variant does not support busy detect feature the flag is null
> > > and there is no impact.
> >
> > By reading the changelog, it doesn't tell me the purpose of this
> > change. When going forward, please work harder on your changelogs.
> >
> > Make sure to answer the questions, *why* is this change needed,
> > *what/how* does the change do.
>
> Ok, I will explain the differences with the legacy and the needs of 
> sdmmc variant about busy detection.
>
> > > Not need to re-read the status register in mmci_cmd_irq, the
> > > status parameter can be used.
> > >
> > > Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
> > > ---
> > > Â drivers/mmc/host/mmci.c | 5 +++--
> > > Â 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > > index 356833a..5b5cc45 100644
> > > --- a/drivers/mmc/host/mmci.c
> > > +++ b/drivers/mmc/host/mmci.c
> > > @@ -1240,7 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct 
> > > mmc_command *cmd,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ */
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!host->busy_status &&
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (readl(base + MMCISTATUS) & 
> > > host->variant->busy_detect_flag)) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (status & host->variant->busy_detect_flag)) {
> >
> > I suggested you to do this change through some of my earlier comments,
> > however I think it should be made as a stand alone change.
> >
> > Anyway, when looking at the details in your series, I decided to try
> > to help out a bit, so I have prepared a couple of related patches for
> > cleaning up and clarifying the busy detection code/comments in mmci. I
> > have incorporated the above change, so let me post them asap.
> >
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* Clear the busy start IRQ */
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ writel(host->variant->busy_detect_mask,
> > > @@ -1517,7 +1517,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * to make sure that both start and end interrupts 
> > > are always
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * cleared one after the other.
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ */
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ status &= readl(host->base + MMCIMASK0);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ status &= readl(host->base + MMCIMASK0) |
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->variant->busy_detect_flag;
> >
> > As I told earlier in the review, this looks wrong to me.
> >
> > It means that you will add the bit for the ->busy_detect_flag to the
> > status field we have just read from the MMCISTATUS register. That
> > means the busy status may be set when it shouldn't.
> >
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (host->variant->busy_detect)
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ writel(status & 
> > > ~host->variant->busy_detect_mask,
> > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->base + MMCICLEAR);
> > > --
> > > 2.7.4
> >
> > By looking at the other changes in the series, I assume @subject patch
> > is intended to prepare for the other changes on top. But it's not
> > really clear.
> >
> > Anyway, in that regards, the below is my observations of what seems to
> > be important part, when supporting busy detection for the stm32 sdmmc
> > variant (except the timeout things in patch2, which I intend to
> > comment separately on).
> >
> > I figured, these are the involved register bits/masks:
> >
> > MMCISTATUS:
> > MCI_STM32_BUSYD0 BIT(20)
> > MCI_STM32_BUSYD0END BIT(21)
> >
> > MMCIMASK0:
> > MCI_STM32_BUSYD0ENDMASK BIT(21)
>
> it's exact:
> MCI_STM32_BUSYD0 BIT(20): This is a hardware status flag only (inverted 
> value of d0 line), it does not generate an interrupt, and so no mask
> bit.
>
> MCI_STM32_BUSYD0ENDMASK BIT(21): This indicates only end of busy
> following a CMD response. On busy to Not busy changes, an interrupt
> is generated (if unmask) and BUSYD0END status flag is set.
> status flag is cleared by writing corresponding interrupt clear bit in 
> MMCICLEAR.
>
> > For the legacy ST variant, there is only one register bit in
> > MMCISTATUS that is used for indicating busy (MCI_ST_CARDBUSY BIT(24)).
> > There is no dedicated busy-end bit for the busy-end IRQ, which I
> > believe is the reason to why the current code also is bit messy.
>
> yes
>
> > It seems like the stm32 sdmmc variant have a separate status bit for
> > the busy-end IRQ, correct?
>
> yes
>
> > If I understand correctly by looking at patch3, you don't use the
> > dedicated busy-end status bit (MCI_STM32_BUSYD0END), right? Then why
> > not?
>
> like your are clarify in previous series, the busy detection is done
> in 3 steps.
>
> if I use:
> .busy_detect_flagÂÂÂ = MCI_STM32_BUSYD0ENDMASK,
> .busy_detect_maskÂÂÂ = MCI_STM32_BUSYD0ENDMASK,
>
> the sdmmc request will be not correctly completed, because the third 
> step can't be happen.
>
> chronologies:
> step1: when busyd0end change to 1
>  Â=> busyd0end interrupt is unmasked
>  Â=> busy_status = cmd_sent | respend
>  Â=> return to mmci_irq
> step2: busyd0end is yet to 1
>  Â=> clear the busyd0end interrupt
>  ÂÂÂÂ=> the hardware clear busyd0end status flag on interrupt clear
>  Â=> return to mmci_irq
>
> like MCI_STM32_BUSYD0END interrupt is generated only on change
> busy to not busy, when the interrupt is cleared (status is 0)
> the step 3 can't happen (no irq pending to re-enter in mmci_cmd_irq).
> sdmmc can't complete the request.
>
> If I use:
> .busy_detect_flagÂÂÂ = MCI_STM32_BUSYD0,
> .busy_detect_maskÂÂÂ = MCI_STM32_BUSYD0ENDMASK,
>
> Like there is no MCI_STM32_BUSYD0 irq mask, the status read in mmci_irq
> "status &= readl(host->base + MMCIMASK0)" can't take account the 
> busy_detect_flag (for sdmmc). So the step 2 can't be passed.
> However we could share re-read between step 1 and step 2.
>
> proposal:
>
> +
> +ÂÂÂÂÂÂÂ u32 busy_val = readl(base + MMCISTATUS) &
> +ÂÂÂÂÂÂÂÂÂÂÂ host->variant->busy_detect_flag;
> +
>  ÂÂÂÂÂÂÂÂ if (!host->busy_status &&
> -ÂÂÂÂÂÂÂÂÂÂÂ !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> -ÂÂÂÂÂÂÂÂÂÂÂ (readl(base + MMCISTATUS) & 
> host->variant->busy_detect_flag)) {
> +ÂÂÂÂÂÂÂÂÂÂÂ !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) && busy_val) {
>
>  ÂÂÂÂÂÂÂÂÂÂÂÂ writel(readl(base + MMCIMASK0) |
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->variant->busy_detect_mask,
> @@ -1262,8 +1265,7 @@ mmci_cmd_irq(struct mmci_host *host, struct 
> mmc_command *cmd,
>  ÂÂÂÂÂÂÂÂÂ * both the start and the end interrupts needs to be cleared,
>  ÂÂÂÂÂÂÂÂÂ * one after the other. So, clear the busy start IRQ here.
>  ÂÂÂÂÂÂÂÂÂ */
> -ÂÂÂÂÂÂÂ if (host->busy_status &&
> -ÂÂÂÂÂÂÂÂÂÂÂ (status & host->variant->busy_detect_flag)) {
> +ÂÂÂÂÂÂÂ if (host->busy_status && busy_val) {
>
>
> what do you think about it ?

I give up this proposal for a new version based on mmci_host_ops
callback to done the busy completion.

> > Thoughts?
> >
> > Kind regards
> > Uffe
>
> Regards
> Ludo
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32