| Message ID | 1556264798-18540-4-git-send-email-ludovic.Barre@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: add busy detect for stm32 sdmmc variant | expand |
On Fri, 26 Apr 2019 at 09:46, Ludovic Barre <ludovic.Barre@st.com> wrote: > > From: Ludovic Barre <ludovic.barre@st.com> > > The "busy_detect_flag" is used to read/clear busy value of > mmci status. The "busy_detect_mask" is used to manage busy irq of > mmci mask. > For sdmmc variant, the 2 properties have not the same offset. > To clear the busyd0 status bit, we must add busy detect flag, > the mmci mask is not enough. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> Ludovic, again, apologies for the delay. > --- > drivers/mmc/host/mmci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index a040f54..3cd52e8 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -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; I think this is not entirely correct, because it would mean we check for busy even if we haven't unmasked the busy IRQ via the variant->busy_detect_mask. I suggest to store a new bool in the host (call it "busy_detect_unmasked" or whatever makes sense to you), to track whether we have unmasked the busy IRQ or not. Then take this flag into account, before ORing the value of host->variant->busy_detect_flag, according to above. > if (host->variant->busy_detect) > writel(status & ~host->variant->busy_detect_mask, > host->base + MMCICLEAR); > -- > 2.7.4 > Kind regards Uffe
hi Ulf On 5/27/19 8:17 PM, Ulf Hansson wrote: > On Fri, 26 Apr 2019 at 09:46, Ludovic Barre <ludovic.Barre@st.com> wrote: >> >> From: Ludovic Barre <ludovic.barre@st.com> >> >> The "busy_detect_flag" is used to read/clear busy value of >> mmci status. The "busy_detect_mask" is used to manage busy irq of >> mmci mask. >> For sdmmc variant, the 2 properties have not the same offset. >> To clear the busyd0 status bit, we must add busy detect flag, >> the mmci mask is not enough. >> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > > Ludovic, again, apologies for the delay. > >> --- >> drivers/mmc/host/mmci.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index a040f54..3cd52e8 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -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; > > I think this is not entirely correct, because it would mean we check > for busy even if we haven't unmasked the busy IRQ via the > variant->busy_detect_mask. if the variant is busy_detect false: => no problem because the busy_detect_flag or busy_detect_mask is not defined. if variant is busy_detect true: the busy handle is split in 3 steps (see mmci_cmd_irq): step 1: detection of busy line => unmasked the busy irq end step 2: in busy wait => ignore cmd irq while current busy flag is enabled. step 3: end of busy => clear and mask busy irq To detect the first step (see mmci_cmd_irq: which unmasks the busy irq) we need to know the current busy state. Actually, the status register is re-read in mmci_cmd_irq, why not used the status read in mmci_irq and in parameter ? Regards, Ludo > > I suggest to store a new bool in the host (call it > "busy_detect_unmasked" or whatever makes sense to you), to track > whether we have unmasked the busy IRQ or not. Then take this flag into > account, before ORing the value of host->variant->busy_detect_flag, > according to above. > >> if (host->variant->busy_detect) >> writel(status & ~host->variant->busy_detect_mask, >> host->base + MMCICLEAR); >> -- >> 2.7.4 >> > > Kind regards > Uffe >
On Wed, 29 May 2019 at 11:20, Ludovic BARRE <ludovic.barre@st.com> wrote: > > hi Ulf > > On 5/27/19 8:17 PM, Ulf Hansson wrote: > > On Fri, 26 Apr 2019 at 09:46, Ludovic Barre <ludovic.Barre@st.com> wrote: > >> > >> From: Ludovic Barre <ludovic.barre@st.com> > >> > >> The "busy_detect_flag" is used to read/clear busy value of > >> mmci status. The "busy_detect_mask" is used to manage busy irq of > >> mmci mask. > >> For sdmmc variant, the 2 properties have not the same offset. > >> To clear the busyd0 status bit, we must add busy detect flag, > >> the mmci mask is not enough. > >> > >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > > > > Ludovic, again, apologies for the delay. > > > >> --- > >> drivers/mmc/host/mmci.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > >> index a040f54..3cd52e8 100644 > >> --- a/drivers/mmc/host/mmci.c > >> +++ b/drivers/mmc/host/mmci.c > >> @@ -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; > > > > I think this is not entirely correct, because it would mean we check > > for busy even if we haven't unmasked the busy IRQ via the > > variant->busy_detect_mask. > > if the variant is busy_detect false: > => no problem because the busy_detect_flag or busy_detect_mask is not > defined. Right. > > if variant is busy_detect true: > the busy handle is split in 3 steps (see mmci_cmd_irq): > step 1: detection of busy line => unmasked the busy irq end > step 2: in busy wait => ignore cmd irq while current busy flag is > enabled. > step 3: end of busy => clear and mask busy irq > > To detect the first step (see mmci_cmd_irq: which unmasks the busy irq) > we need to know the current busy state. Actually, the status register is > re-read in mmci_cmd_irq, why not used the status read in mmci_irq and in > parameter ? Right, I see your point. On the other hand, that re-read of the status registers should really not be needed. Maybe it's a leftover from my initial version of the code, but in any case we should remove that. > > Regards, > Ludo > > > > > I suggest to store a new bool in the host (call it > > "busy_detect_unmasked" or whatever makes sense to you), to track > > whether we have unmasked the busy IRQ or not. Then take this flag into > > account, before ORing the value of host->variant->busy_detect_flag, > > according to above. > > > >> if (host->variant->busy_detect) > >> writel(status & ~host->variant->busy_detect_mask, > >> host->base + MMCICLEAR); > >> -- > >> 2.7.4 Kind regards Uffe
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index a040f54..3cd52e8 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -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; if (host->variant->busy_detect) writel(status & ~host->variant->busy_detect_mask, host->base + MMCICLEAR);