lkml.org 
[lkml]   [2018]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V2 2/2] mmc: mmci: add variant property to send stop cmd if a command fail
    From
    Date
    hi Ulf

    due to some ST internal works, I will busy the next days.
    But I will sent the change as soon as possible.


    On 11/21/18 6:56 PM, Ulf Hansson wrote:
    > On 7 November 2018 at 10:30, Ludovic Barre <ludovic.Barre@st.com> wrote:
    >> From: Ludovic Barre <ludovic.barre@st.com>
    >>
    >> The mmc framework follows the requirement of SD_Specification:
    >> the STOP_TRANSMISSION is sent on multiple write/read commands
    >> and the stop command (alone), not needed on other ADTC commands.
    >>
    >> But, if an error happens on command or data transmission, some
    >> variants require a stop command "STOP_TRANSMISION" to clear the DPSM
    >> "Data Path State Machine". If it's not done the next data
    >> command freezes hardware block.
    >> Needed to support the STM32 sdmmc variant.
    >
    > May I suggest some re-wording of this changelog, as to make it more
    > clear of why this is needed. Something along the lines of:
    >
    > "The current approach with sending a CMD12 (STOP_TRANSMISSION) to
    > complete a data transfer request, either because of using the open
    > ended transmission type or because of receiving an error during a data
    > transfer, isn't sufficient for the STM32 sdmmc variant.
    >
    > More precisely, for STM32 sdmmc the DPSM ("Data Path State Machine" )
    > needs to be cleared by sending a CMD12, also for the so called ADTC
    > commands. For this reason, add a new mmci variant property and let the
    > driver send a CMD12 to complete ADTC commands, in case it's set."

    I will change, it's more accurate and concise, thanks for re-wording

    >
    >>
    >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
    >> ---
    >> drivers/mmc/host/mmci.c | 33 +++++++++++++++++++++++++++++++++
    >> drivers/mmc/host/mmci.h | 4 ++++
    >> 2 files changed, 37 insertions(+)
    >>
    >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
    >> index 13fa640..47b865d 100644
    >> --- a/drivers/mmc/host/mmci.c
    >> +++ b/drivers/mmc/host/mmci.c
    >> @@ -21,6 +21,7 @@
    >> #include <linux/err.h>
    >> #include <linux/highmem.h>
    >> #include <linux/log2.h>
    >> +#include <linux/mmc/mmc.h>
    >> #include <linux/mmc/pm.h>
    >> #include <linux/mmc/host.h>
    >> #include <linux/mmc/card.h>
    >> @@ -57,6 +58,8 @@ void sdmmc_variant_init(struct mmci_host *host);
    >> #else
    >> static inline void sdmmc_variant_init(struct mmci_host *host) {}
    >> #endif
    >> +static void
    >> +mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c);
    >>
    >> static unsigned int fmax = 515633;
    >>
    >> @@ -274,6 +277,7 @@ static struct variant_data variant_stm32_sdmmc = {
    >> .cmdreg_lrsp_crc = MCI_CPSM_STM32_LRSP_CRC,
    >> .cmdreg_srsp_crc = MCI_CPSM_STM32_SRSP_CRC,
    >> .cmdreg_srsp = MCI_CPSM_STM32_SRSP,
    >> + .cmdreg_stop = MCI_CPSM_STM32_CMDSTOP,
    >> .data_cmd_enable = MCI_CPSM_STM32_CMDTRANS,
    >> .irq_pio_mask = MCI_IRQ_PIO_STM32_MASK,
    >> .datactrl_first = true,
    >> @@ -573,6 +577,24 @@ void mmci_dma_error(struct mmci_host *host)
    >> static void
    >> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
    >> {
    >> + /*
    >> + * If an error happens on command or data transmission, some variants
    >> + * require a stop command to reinit the DPSM.
    >> + * If it's not done the next data command freeze hardware block.
    >> + */
    >> + if (host->variant->cmdreg_stop) {
    >> + u32 dpsm;
    >> +
    >> + dpsm = readl_relaxed(host->base + MMCISTATUS);
    >> + dpsm &= MCI_STM32_DPSMACTIVE;
    >> +
    >> + if (dpsm && ((mrq->cmd && mrq->cmd->error) ||
    >> + (mrq->data && mrq->data->error))) {
    >> + mmci_start_command(host, &host->stop_abort, 0);
    >> + return;
    >> + }
    >
    > I would rather move this code to a separate function.

    OK

    Also, I think
    > you need something else (or additional) than polling the MMCISTATUS
    > register, as in principle you could end up sending CMD12 several times
    > for the same request, which isn't correct.

    In mmci_request_end, if the DPSM is still enabled, there was no previous
    "cmd12" sent. So Normally, the regular and special cmd12 can't be sent
    for the same request.

    >
    > To me the best solution would probably be to make use of the
    > host->data pointer, as it becomes set when DPSM has been enabled.
    > However, host->data is also cleared in mmci_stop_data() which is being
    > called prior mmci_request_end(). In other words, we need to figure
    > under what conditions the new code above should be triggered/called
    > and then also change the conditions for when mmci_request_end() shall
    > be called.
    >
    > In principle look at callers of mmci_request_end() and
    > mmci_stop_data() and update those paths.
    >
    >> + }
    >> +
    >> writel(0, host->base + MMCICOMMAND);
    >>
    >> BUG_ON(host->data);
    >> @@ -1100,6 +1122,10 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
    >> mmci_reg_delay(host);
    >> }
    >>
    >> + if (host->variant->cmdreg_stop &&
    >> + cmd->opcode == MMC_STOP_TRANSMISSION)
    >> + c |= host->variant->cmdreg_stop;
    >> +
    >
    > Hmm.
    >
    > It looks like the above changes, together with the introduction of the
    > variant property, belongs in a separate patch, preceding $subject
    > patch in the series.
    >
    > The reason is, that to me, it makes sense to add the special treatment
    > of the ADTC commands in a separate patch. Can you please split it up
    > and then of course update the change log accordingly!?

    Ok, I will split this part in separate patch.

    >
    >> c |= cmd->opcode | host->variant->cmdreg_cpsm_enable;
    >> if (cmd->flags & MMC_RSP_PRESENT) {
    >> if (cmd->flags & MMC_RSP_136)
    >> @@ -1950,6 +1976,13 @@ static int mmci_probe(struct amba_device *dev,
    >> mmc->max_busy_timeout = 0;
    >> }
    >>
    >> + /* prepare the stop command, used to abort and reinitialized the DPSM */
    >> + if (variant->cmdreg_stop) {
    >> + host->stop_abort.opcode = MMC_STOP_TRANSMISSION;
    >> + host->stop_abort.arg = 0;
    >> + host->stop_abort.flags = MMC_RSP_R1B | MMC_CMD_AC;
    >> + }
    >> +
    >> mmc->ops = &mmci_ops;
    >>
    >> /* We support these PM capabilities. */
    >> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
    >> index 550dd39..35372cd 100644
    >> --- a/drivers/mmc/host/mmci.h
    >> +++ b/drivers/mmc/host/mmci.h
    >> @@ -161,6 +161,7 @@
    >> #define MCI_ST_CEATAEND (1 << 23)
    >> #define MCI_ST_CARDBUSY (1 << 24)
    >> /* Extended status bits for the STM32 variants */
    >> +#define MCI_STM32_DPSMACTIVE BIT(12)
    >> #define MCI_STM32_BUSYD0 BIT(20)
    >>
    >> #define MMCICLEAR 0x038
    >> @@ -264,6 +265,7 @@ struct mmci_host;
    >> * @cmdreg_lrsp_crc: enable value for long response with crc
    >> * @cmdreg_srsp_crc: enable value for short response with crc
    >> * @cmdreg_srsp: enable value for short response without crc
    >> + * @cmdreg_stop: enable value for stop and abort transmission
    >> * @datalength_bits: number of bits in the MMCIDATALENGTH register
    >> * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
    >> * is asserted (likewise for RX)
    >> @@ -316,6 +318,7 @@ struct variant_data {
    >> unsigned int cmdreg_lrsp_crc;
    >> unsigned int cmdreg_srsp_crc;
    >> unsigned int cmdreg_srsp;
    >> + unsigned int cmdreg_stop;
    >> unsigned int datalength_bits;
    >> unsigned int fifosize;
    >> unsigned int fifohalfsize;
    >> @@ -375,6 +378,7 @@ struct mmci_host {
    >> void __iomem *base;
    >> struct mmc_request *mrq;
    >> struct mmc_command *cmd;
    >> + struct mmc_command stop_abort;
    >> struct mmc_data *data;
    >> struct mmc_host *mmc;
    >> struct clk *clk;
    >> --
    >> 2.7.4
    >>
    >
    > Kind regards
    > Uffe
    >

    \
     
     \ /
      Last update: 2018-11-22 11:16    [W:3.148 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site