Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1600999061-13669-1-git-send-email-rui_feng@realsil.com.cn>
In-Reply-To: <1600999061-13669-1-git-send-email-rui_feng@realsil.com.cn>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 21 Oct 2020 15:59:53 +0200
Message-ID: <CAPDyKFrnkF3mU5PJsy0VtEjPSToktSsRRtyMvQF97vymc+rY5A@mail.gmail.com>
Subject: Re: [PATCH 3/3] mmc: rtsx: Add SD Express mode support for RTS5261
To:     Rui Feng <rui_feng@realsil.com.cn>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 25 Sep 2020 at 03:57, <rui_feng@realsil.com.cn> wrote:
>
> From: Rui Feng <rui_feng@realsil.com.cn>
>
> RTS5261 support legacy SD mode and SD Express mode.
> In SD7.x, SD association introduce SD Express as a new mode.
> This patch makes RTS5261 support SD Express mode.

As per patch 2, can you please add some more information about what
changes are needed to support SD Express? This just states that the
support is implemented, but please elaborate how.

>
> Signed-off-by: Rui Feng <rui_feng@realsil.com.cn>
> ---
>  drivers/mmc/host/rtsx_pci_sdmmc.c | 59 +++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index 2763a376b054..efde374a4a5e 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -895,7 +895,9 @@ static int sd_set_bus_width(struct realtek_pci_sdmmc *host,
>  static int sd_power_on(struct realtek_pci_sdmmc *host)
>  {
>         struct rtsx_pcr *pcr = host->pcr;
> +       struct mmc_host *mmc = host->mmc;
>         int err;
> +       u32 val;
>
>         if (host->power_state == SDMMC_POWER_ON)
>                 return 0;
> @@ -922,6 +924,14 @@ static int sd_power_on(struct realtek_pci_sdmmc *host)
>         if (err < 0)
>                 return err;
>
> +       if (PCI_PID(pcr) == PID_5261) {
> +               val = rtsx_pci_readl(pcr, RTSX_BIPR);
> +               if (val & SD_WRITE_PROTECT) {
> +                       pcr->extra_caps &= ~EXTRA_CAPS_SD_EXPRESS;
> +                       mmc->caps2 &= ~(MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V);

This looks a bit weird to me. For a write protected card you want to
disable the SD_EXPRESS support, right?

Is there no mechanism to support read-only PCIe/NVMe based storage
devices? If that is the case, maybe it's simply better to not support
the readonly option at all for SD express cards?

> +               }
> +       }
> +
>         host->power_state = SDMMC_POWER_ON;
>         return 0;
>  }
> @@ -1127,6 +1137,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc)
>         if (val & SD_EXIST)
>                 cd = 1;
>
> +       if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS)
> +               mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V;

This looks wrong. You shouldn't be using the ->get_cd() callback to
re-enable mmc caps.

Normally set the mmc caps while host probes (from the ->probe()
callback), but I guess this is kind of a special case, as the
read-only switch state isn't known until we have powered on the card,
right?

If that is the case, I suggest to re-enable the mmc caps from the
->set_ios() callback instead, when ios->power_mode == MMC_POWER_OFF.

>         mutex_unlock(&pcr->pcr_mutex);
>
>         return cd;
> @@ -1308,6 +1320,50 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
>         return err;
>  }
>
> +static int sdmmc_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +       u32 relink_time, val;
> +       struct realtek_pci_sdmmc *host = mmc_priv(mmc);
> +       struct rtsx_pcr *pcr = host->pcr;
> +
> +       /*
> +        * If card has PCIe availability and WP if off,
> +        * reader switch to PCIe mode.
> +        */
> +       val = rtsx_pci_readl(pcr, RTSX_BIPR);
> +       if (!(val & SD_WRITE_PROTECT)) {

This should not be needed, as you have already checked the write
protect bit before enabling the mmc caps for SD EXPRESS, right?

> +               /* Set relink_time for changing to PCIe card */
> +               relink_time = 0x8FFF;
> +
> +               rtsx_pci_write_register(pcr, 0xFF01, 0xFF, relink_time);
> +               rtsx_pci_write_register(pcr, 0xFF02, 0xFF, relink_time >> 8);
> +               rtsx_pci_write_register(pcr, 0xFF03, 0x01, relink_time >> 16);
> +
> +               rtsx_pci_write_register(pcr, PETXCFG, 0x80, 0x80);
> +               rtsx_pci_write_register(pcr, LDO_VCC_CFG0,
> +                       RTS5261_LDO1_OCP_THD_MASK,
> +                       pcr->option.sd_800mA_ocp_thd);
> +
> +               if (pcr->ops->disable_auto_blink)
> +                       pcr->ops->disable_auto_blink(pcr);
> +
> +               /* For PCIe/NVMe mode can't enter delink issue */
> +               pcr->hw_param.interrupt_en &= ~(SD_INT_EN);
> +               rtsx_pci_writel(pcr, RTSX_BIER, pcr->hw_param.interrupt_en);
> +
> +               rtsx_pci_write_register(pcr, RTS5260_AUTOLOAD_CFG4,
> +                       RTS5261_AUX_CLK_16M_EN, RTS5261_AUX_CLK_16M_EN);
> +               rtsx_pci_write_register(pcr, RTS5261_FW_CFG0,
> +                       RTS5261_FW_ENTER_EXPRESS, RTS5261_FW_ENTER_EXPRESS);
> +               rtsx_pci_write_register(pcr, RTS5261_FW_CFG1,
> +                       RTS5261_MCU_BUS_SEL_MASK | RTS5261_MCU_CLOCK_SEL_MASK
> +                       | RTS5261_MCU_CLOCK_GATING | RTS5261_DRIVER_ENABLE_FW,
> +                       RTS5261_MCU_CLOCK_SEL_16M | RTS5261_MCU_CLOCK_GATING
> +                       | RTS5261_DRIVER_ENABLE_FW);
> +       }
> +       return 0;
> +}
> +
>  static const struct mmc_host_ops realtek_pci_sdmmc_ops = {
>         .pre_req = sdmmc_pre_req,
>         .post_req = sdmmc_post_req,
> @@ -1317,6 +1373,7 @@ static const struct mmc_host_ops realtek_pci_sdmmc_ops = {
>         .get_cd = sdmmc_get_cd,
>         .start_signal_voltage_switch = sdmmc_switch_voltage,
>         .execute_tuning = sdmmc_execute_tuning,
> +       .init_sd_express = sdmmc_init_sd_express,
>  };
>
>  static void init_extra_caps(struct realtek_pci_sdmmc *host)
> @@ -1338,6 +1395,8 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host)
>                 mmc->caps |= MMC_CAP_8_BIT_DATA;
>         if (pcr->extra_caps & EXTRA_CAPS_NO_MMC)
>                 mmc->caps2 |= MMC_CAP2_NO_MMC;
> +       if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS)
> +               mmc->caps2 |= MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V;
>  }
>
>  static void realtek_init_host(struct realtek_pci_sdmmc *host)
> --
> 2.17.1
>

A follow up question:

Based upon your feedback from our earlier discussions, I believe you
stated that the card reader driver (rtsx_pci_driver) will unregister
the corresponding mfd/platform device that corresponds to the
rtsx_pci_sdmmc_driver - when it gets configured to manage a PCIe/NVMe
based storage device. Correct?

Perhaps I didn't get that part correctly, but if this is the case, it
means that the ->remove() callback (rtsx_pci_sdmmc_drv_remove()) will
be invoked. Furthermore, this will cause the ->set_ios() callback to
be invoked when the core calls mmc_power_off() in that path. Isn't
that a problem that you need to address?

Kind regards
Uffe