Re: [PATCH V6 2/3] mmc: mmci: add busy_complete callback

[Ulf Hansson]

On Thu, 5 Sep 2019 at 14:22, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
>
> From: Ludovic Barre <ludovic.barre@xxxxxx>
>
> This patch adds busy_completion callback at mmci_host_ops
> to allow to define a specific busy completion by variant.
>
> The legacy code corresponding to busy completion used
> by ux500 variants is moved to ux500_busy_complete function.
>
> The busy_detect boolean property is replaced by
> busy_complete callback definition.

At this point I prefer to keep th busy_detect boolean property. It
makes the code a bit more consistent.

Although, I think in case busy_detect is set for the variant, the
variant also needs to assign the new ->busy_completion() callback. In
other words, we don't need to check for a valid callback in code if
busy_detect is set.

Otherwise, this looks good to me!

Kind regards
Uffe

>
> Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
> ---
>  drivers/mmc/host/mmci.c | 142 +++++++++++++++++++++-------------------
>  drivers/mmc/host/mmci.h |   3 +-
>  2 files changed, 76 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index c30319255dc2..e20164f4354d 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -44,6 +44,7 @@
>  #define DRIVER_NAME "mmci-pl18x"
>
>  static void mmci_variant_init(struct mmci_host *host);
> +static void ux500_variant_init(struct mmci_host *host);
>  static void ux500v2_variant_init(struct mmci_host *host);
>
>  static unsigned int fmax = 515633;
> @@ -175,7 +176,6 @@ static struct variant_data variant_ux500 = {
>         .f_max                  = 100000000,
>         .signal_direction       = true,
>         .pwrreg_clkgate         = true,
> -       .busy_detect            = true,
>         .busy_dpsm_flag         = MCI_DPSM_ST_BUSYMODE,
>         .busy_detect_flag       = MCI_ST_CARDBUSY,
>         .busy_detect_mask       = MCI_ST_BUSYENDMASK,
> @@ -184,7 +184,7 @@ static struct variant_data variant_ux500 = {
>         .irq_pio_mask           = MCI_IRQ_PIO_MASK,
>         .start_err              = MCI_STARTBITERR,
>         .opendrain              = MCI_OD,
> -       .init                   = mmci_variant_init,
> +       .init                   = ux500_variant_init,
>  };
>
>  static struct variant_data variant_ux500v2 = {
> @@ -208,7 +208,6 @@ static struct variant_data variant_ux500v2 = {
>         .f_max                  = 100000000,
>         .signal_direction       = true,
>         .pwrreg_clkgate         = true,
> -       .busy_detect            = true,
>         .busy_dpsm_flag         = MCI_DPSM_ST_BUSYMODE,
>         .busy_detect_flag       = MCI_ST_CARDBUSY,
>         .busy_detect_mask       = MCI_ST_BUSYENDMASK,
> @@ -610,6 +609,67 @@ static u32 ux500v2_get_dctrl_cfg(struct mmci_host *host)
>         return MCI_DPSM_ENABLE | (host->data->blksz << 16);
>  }
>
> +static bool ux500_busy_complete(struct mmci_host *host, u32 status, u32 err_msk)
> +{
> +       void __iomem *base = host->base;
> +
> +       /*
> +        * Before unmasking for the busy end IRQ, confirm that the
> +        * command was sent successfully. To keep track of having a
> +        * command in-progress, waiting for busy signaling to end,
> +        * store the status in host->busy_status.
> +        *
> +        * Note that, the card may need a couple of clock cycles before
> +        * it starts signaling busy on DAT0, hence re-read the
> +        * MMCISTATUS register here, to allow the busy bit to be set.
> +        * Potentially we may even need to poll the register for a
> +        * while, to allow it to be set, but tests indicates that it
> +        * isn't needed.
> +        */
> +       if (!host->busy_status && !(status & err_msk) &&
> +           (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> +               writel(readl(base + MMCIMASK0) |
> +                      host->variant->busy_detect_mask,
> +                      base + MMCIMASK0);
> +
> +               host->busy_status = status & (MCI_CMDSENT | MCI_CMDRESPEND);
> +               return false;
> +       }
> +
> +       /*
> +        * If there is a command in-progress that has been successfully
> +        * sent, then bail out if busy status is set and wait for the
> +        * busy end IRQ.
> +        *
> +        * Note that, the HW triggers an IRQ on both edges while
> +        * monitoring DAT0 for busy completion, but there is only one
> +        * status bit in MMCISTATUS for the busy state. Therefore
> +        * 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)) {
> +               writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> +               return false;
> +       }
> +
> +       /*
> +        * If there is a command in-progress that has been successfully
> +        * sent and the busy bit isn't set, it means we have received
> +        * the busy end IRQ. Clear and mask the IRQ, then continue to
> +        * process the command.
> +        */
> +       if (host->busy_status) {
> +               writel(host->variant->busy_detect_mask, base + MMCICLEAR);
> +
> +               writel(readl(base + MMCIMASK0) &
> +                      ~host->variant->busy_detect_mask, base + MMCIMASK0);
> +               host->busy_status = 0;
> +       }
> +
> +       return true;
> +}
> +
>  /*
>   * All the DMA operation mode stuff goes inside this ifdef.
>   * This assumes that you have a generic DMA device interface,
> @@ -953,9 +1013,16 @@ void mmci_variant_init(struct mmci_host *host)
>         host->ops = &mmci_variant_ops;
>  }
>
> +void ux500_variant_init(struct mmci_host *host)
> +{
> +       host->ops = &mmci_variant_ops;
> +       host->ops->busy_complete = ux500_busy_complete;
> +}
> +
>  void ux500v2_variant_init(struct mmci_host *host)
>  {
>         host->ops = &mmci_variant_ops;
> +       host->ops->busy_complete = ux500_busy_complete;
>         host->ops->get_datactrl_cfg = ux500v2_get_dctrl_cfg;
>  }
>
> @@ -1235,68 +1302,9 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>                 return;
>
>         /* Handle busy detection on DAT0 if the variant supports it. */
> -       if (busy_resp && host->variant->busy_detect) {
> -
> -               /*
> -                * Before unmasking for the busy end IRQ, confirm that the
> -                * command was sent successfully. To keep track of having a
> -                * command in-progress, waiting for busy signaling to end,
> -                * store the status in host->busy_status.
> -                *
> -                * Note that, the card may need a couple of clock cycles before
> -                * it starts signaling busy on DAT0, hence re-read the
> -                * MMCISTATUS register here, to allow the busy bit to be set.
> -                * Potentially we may even need to poll the register for a
> -                * while, to allow it to be set, but tests indicates that it
> -                * isn't needed.
> -                */
> -               if (!host->busy_status && !(status & err_msk) &&
> -                   (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> -
> -                       writel(readl(base + MMCIMASK0) |
> -                              host->variant->busy_detect_mask,
> -                              base + MMCIMASK0);
> -
> -                       host->busy_status =
> -                               status & (MCI_CMDSENT|MCI_CMDRESPEND);
> -                       return;
> -               }
> -
> -               /*
> -                * If there is a command in-progress that has been successfully
> -                * sent, then bail out if busy status is set and wait for the
> -                * busy end IRQ.
> -                *
> -                * Note that, the HW triggers an IRQ on both edges while
> -                * monitoring DAT0 for busy completion, but there is only one
> -                * status bit in MMCISTATUS for the busy state. Therefore
> -                * 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)) {
> -                       writel(host->variant->busy_detect_mask,
> -                              host->base + MMCICLEAR);
> +       if (busy_resp && host->ops->busy_complete)
> +               if (!host->ops->busy_complete(host, status, err_msk))
>                         return;
> -               }
> -
> -               /*
> -                * If there is a command in-progress that has been successfully
> -                * sent and the busy bit isn't set, it means we have received
> -                * the busy end IRQ. Clear and mask the IRQ, then continue to
> -                * process the command.
> -                */
> -               if (host->busy_status) {
> -
> -                       writel(host->variant->busy_detect_mask,
> -                              host->base + MMCICLEAR);
> -
> -                       writel(readl(base + MMCIMASK0) &
> -                              ~host->variant->busy_detect_mask,
> -                              base + MMCIMASK0);
> -                       host->busy_status = 0;
> -               }
> -       }
>
>         host->cmd = NULL;
>
> @@ -1537,7 +1545,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>                  * clear the corresponding IRQ.
>                  */
>                 status &= readl(host->base + MMCIMASK0);
> -               if (host->variant->busy_detect)
> +               if (host->ops->busy_complete)
>                         writel(status & ~host->variant->busy_detect_mask,
>                                host->base + MMCICLEAR);
>                 else
> @@ -1605,7 +1613,7 @@ static void mmci_set_max_busy_timeout(struct mmc_host *mmc)
>         struct mmci_host *host = mmc_priv(mmc);
>         u32 max_busy_timeout = 0;
>
> -       if (!host->variant->busy_detect)
> +       if (!host->ops->busy_complete)
>                 return;
>
>         if (host->variant->busy_timeout && mmc->actual_clock)
> @@ -1980,7 +1988,7 @@ static int mmci_probe(struct amba_device *dev,
>         /*
>          * Enable busy detection.
>          */
> -       if (variant->busy_detect) {
> +       if (host->ops->busy_complete) {
>                 mmci_ops.card_busy = mmci_card_busy;
>                 /*
>                  * Not all variants have a flag to enable busy detection
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index d8b7f6774e8f..733f9a035b06 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -286,7 +286,6 @@ struct mmci_host;
>   * @f_max: maximum clk frequency supported by the controller.
>   * @signal_direction: input/out direction of bus signals can be indicated
>   * @pwrreg_clkgate: MMCIPOWER register must be used to gate the clock
> - * @busy_detect: true if the variant supports busy detection on DAT0.
>   * @busy_timeout: true if the variant starts data timer when the DPSM
>   *               enter in Wait_R or Busy state.
>   * @busy_dpsm_flag: bitmask enabling busy detection in the DPSM
> @@ -334,7 +333,6 @@ struct variant_data {
>         u32                     f_max;
>         u8                      signal_direction:1;
>         u8                      pwrreg_clkgate:1;
> -       u8                      busy_detect:1;
>         u8                      busy_timeout:1;
>         u32                     busy_dpsm_flag;
>         u32                     busy_detect_flag;
> @@ -369,6 +367,7 @@ struct mmci_host_ops {
>         void (*dma_error)(struct mmci_host *host);
>         void (*set_clkreg)(struct mmci_host *host, unsigned int desired);
>         void (*set_pwrreg)(struct mmci_host *host, unsigned int pwr);
> +       bool (*busy_complete)(struct mmci_host *host, u32 status, u32 err_msk);
>  };
>
>  struct mmci_host {
> --
> 2.17.1
>