Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230630055723.7673-1-jyanchou@realtek.com>
In-Reply-To: <20230630055723.7673-1-jyanchou@realtek.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri, 30 Jun 2023 13:39:09 +0200
Message-ID: <CAPDyKFp96N=nUS_8aLQcJyd0DZ+yce42xpHtzyPMn0y7hStjbg@mail.gmail.com>
Subject: Re: [PATCH] mmc: Add Synopsys DesignWare mmc cmdq host driver
To:     Jyan Chou <jyanchou@realtek.com>
Cc:     adrian.hunter@intel.com, jh80.chung@samsung.com,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        james.tai@realtek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 30 Jun 2023 at 07:57, Jyan Chou <jyanchou@realtek.com> wrote:
>
> The difference between dw_mmc.c and dw_mmc_cqe.c is that
> we implemrnted cmdq feature, and we found out the difference
> between distint version of synopsys' data book and user guide.
>
> We add the mmc driver for the Synopsys DesignWare mmc cmdq host
> controller that can improve performance. Also, we add our
> Realtek mmc driver that make good use of it.
>
> Signed-off-by: Jyan Chou <jyanchou@realtek.com>
> ---
>  drivers/mmc/host/cqhci-core.c     |    5 +
>  drivers/mmc/host/cqhci.h          |    2 +
>  drivers/mmc/host/dw_mmc_cqe-rtk.c | 1343 +++++++++++++++++++++
>  drivers/mmc/host/dw_mmc_cqe-rtk.h |  164 +++
>  drivers/mmc/host/dw_mmc_cqe.c     | 1821 +++++++++++++++++++++++++++++
>  drivers/mmc/host/dw_mmc_cqe.h     |  441 +++++++
>  6 files changed, 3776 insertions(+)
>  create mode 100644 drivers/mmc/host/dw_mmc_cqe-rtk.c
>  create mode 100644 drivers/mmc/host/dw_mmc_cqe-rtk.h
>  create mode 100644 drivers/mmc/host/dw_mmc_cqe.c
>  create mode 100644 drivers/mmc/host/dw_mmc_cqe.h
>
> diff --git a/drivers/mmc/host/cqhci-core.c b/drivers/mmc/host/cqhci-core.c
> index b3d7d6d8d654..f4ddad62632a 100644
> --- a/drivers/mmc/host/cqhci-core.c
> +++ b/drivers/mmc/host/cqhci-core.c
> @@ -514,6 +514,11 @@ static int cqhci_prep_tran_desc(struct mmc_request *mrq,
>                 return sg_count;
>         }
>
> +       if (cq_host->ops->setup_tran_desc) {
> +               cq_host->ops->setup_tran_desc(data, cq_host, desc, sg_count);
> +               return 0;
> +       }
> +
>         desc = get_trans_desc(cq_host, tag);
>
>         for_each_sg(data->sg, sg, sg_count, i) {
> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h
> index ba9387ed90eb..a3e56da6189d 100644
> --- a/drivers/mmc/host/cqhci.h
> +++ b/drivers/mmc/host/cqhci.h
> @@ -286,6 +286,8 @@ struct cqhci_host_ops {
>                                  u64 *data);
>         void (*pre_enable)(struct mmc_host *mmc);
>         void (*post_disable)(struct mmc_host *mmc);
> +       void (*setup_tran_desc)(struct mmc_data *data,
> +               struct cqhci_host *cq_host, u8 *desc, int sg_count);
>  #ifdef CONFIG_MMC_CRYPTO
>         int (*program_key)(struct cqhci_host *cq_host,
>                            const union cqhci_crypto_cfg_entry *cfg, int slot);
> diff --git a/drivers/mmc/host/dw_mmc_cqe-rtk.c b/drivers/mmc/host/dw_mmc_cqe-rtk.c
> new file mode 100644
> index 000000000000..577cb1f1ed70
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc_cqe-rtk.c
> @@ -0,0 +1,1343 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Realtek Semiconductor Corp.
> + *
> + */
> +
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_address.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include "dw_mmc_cqe.h"
> +#include "dw_mmc_cqe-rtk.h"
> +
> +#define LOWER_BIT_MASK         0x00ffffff
> +#define HIGH_BIT_MASK          0xff000000
> +
> +static void dw_mci_rtk_hs400_complete(struct mmc_host *mmc);
> +static int dw_mci_rtk_execute_tuning(struct dw_mci_slot *slot, u32 opcode);
> +
> +int dw_mci_rtk_write_protect_cmd(struct mmc_host *mmc, u32 args, bool is_wrtie_protect)
> +{
> +       struct mmc_request mrq = {};
> +       struct mmc_command cmd = {};
> +       int err = 0;
> +
> +       if (is_wrtie_protect)
> +               cmd.opcode = MMC_SET_WRITE_PROT;
> +       else
> +               cmd.opcode = MMC_CLR_WRITE_PROT;
> +
> +       cmd.arg = args;
> +       cmd.flags = MMC_CMD_AC|MMC_RSP_SPI_R1B | MMC_RSP_R1B;
> +       mrq.cmd = &cmd;
> +       mrq.data = NULL;
> +
> +       mmc_wait_for_req(mmc, &mrq);

This is not how the mmc subsystem works. Commands and protocol
specific things are managed by the core layer.

Please don't abuse the interfaces/APIs from the core. Just looking at
a few lines below, I noticed that there are more cases like this in
$subject patch, therefore I am stopping the review already at this
point. I am simply afraid that I will waste my time on this, sorry.

Next time, if you honestly want me to review this, please do your
homework and take care of the above before posting a new version.

[...]

Kind regards
Uffe