Re: [PATCH] mmc: rtsx: Add SD Express mode support for RTS5261

From: Greg KH
Date: Tue May 05 2020 - 10:28:33 EST

Next message: Wolfram Sang: "Re: [PATCH] i2c: use my kernel.org address from now on"
Previous message: Wolfram Sang: "Re: [PATCH] i2c: fix missing pm_runtime_put_sync in i2c_device_probe"
Next in thread: Ulf Hansson: "Re: [PATCH] mmc: rtsx: Add SD Express mode support for RTS5261"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Apr 26, 2020 at 09:25:46AM +0800, rui_feng@xxxxxxxxxxxxxx wrote:
> From: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
>
> RTS5261 support legacy SD mode and SD Express mode.
> In SD7.x, SD association introduce SD Express as a new mode.
> SD Express mode is distinguished by CMD8.
> Therefore, CMD8 has new bit for SD Express.
> SD Express is based on PCIe/NVMe.
> RTS5261 uses CMD8 to switch to SD Express mode.
>
> Signed-off-by: Rui Feng <rui_feng@xxxxxxxxxxxxxx>
> ---
> drivers/misc/cardreader/Kconfig | 8 ++++++
> drivers/misc/cardreader/Makefile | 3 +++
> drivers/misc/cardreader/rts5261.c | 7 +++++
> drivers/misc/cardreader/rtsx_pcr.c | 7 +++++
> drivers/mmc/core/sd_ops.c | 9 ++++++-
> drivers/mmc/host/rtsx_pci_sdmmc.c | 43 ++++++++++++++++++++++++++++++
> include/linux/mmc/host.h | 1 +
> include/linux/rtsx_pci.h | 15 +++++++++++
> 8 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/cardreader/Kconfig b/drivers/misc/cardreader/Kconfig
> index 022322dfb36e..1ce6bf1d121c 100644
> --- a/drivers/misc/cardreader/Kconfig
> +++ b/drivers/misc/cardreader/Kconfig
> @@ -21,6 +21,14 @@ config MISC_RTSX_PCI
> such as Memory Stick, Memory Stick Pro, Secure Digital and
> MultiMediaCard.
>
> +config MISC_RTSX_PCI_SD_EXPRESS
> + bool "Realtek PCI-E card reader support sd express card"
> + depends on MISC_RTSX_PCI
> + default y

"default y" is only if the machine will no longer boot if you do not
accept this option. That's not the case for a driver like this, sorry.

> + help
> + RTS5261 support SD Express card.
> + Say Y here if you want to enable PCIe/NVMe mode.

No hint as to what the module name will be?

> +
> config MISC_RTSX_USB
> tristate "Realtek USB card reader"
> depends on USB
> diff --git a/drivers/misc/cardreader/Makefile b/drivers/misc/cardreader/Makefile
> index 1f56267ed2f4..589011999f78 100644
> --- a/drivers/misc/cardreader/Makefile
> +++ b/drivers/misc/cardreader/Makefile
> @@ -2,4 +2,7 @@
> obj-$(CONFIG_MISC_ALCOR_PCI) += alcor_pci.o
> obj-$(CONFIG_MISC_RTSX_PCI) += rtsx_pci.o
> rtsx_pci-objs := rtsx_pcr.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o rts5260.o rts5261.o
> +
> +ccflags-$(CONFIG_MISC_RTSX_PCI_SD_EXPRESS) += -DMISC_RTSX_PCI_SD_EXPRESS

Ick, really? Why? Why can't you just look at the config option when
you build instead?

> +
> obj-$(CONFIG_MISC_RTSX_USB) += rtsx_usb.o
> diff --git a/drivers/misc/cardreader/rts5261.c b/drivers/misc/cardreader/rts5261.c
> index 547db5ffd3f6..68cb0562c278 100644
> --- a/drivers/misc/cardreader/rts5261.c
> +++ b/drivers/misc/cardreader/rts5261.c
> @@ -759,8 +759,14 @@ void rts5261_init_params(struct rtsx_pcr *pcr)
> {
> struct rtsx_cr_option *option = &pcr->option;
> struct rtsx_hw_param *hw_param = &pcr->hw_param;
> + u8 val;
>
> pcr->extra_caps = EXTRA_CAPS_SD_SDR50 | EXTRA_CAPS_SD_SDR104;
> +#ifdef MISC_RTSX_PCI_SD_EXPRESS
> + rtsx_pci_read_register(pcr, RTS5261_FW_STATUS, &val);
> + if (!(val & RTS5261_EXPRESS_LINK_FAIL_MASK))
> + pcr->extra_caps |= EXTRA_CAPS_SD_EXPRESS;
> +#endif

No #ifdef in .c files please, that is not the Linux kernel way.

> pcr->num_slots = 1;
> pcr->ops = &rts5261_pcr_ops;
>
> @@ -791,6 +797,7 @@ void rts5261_init_params(struct rtsx_pcr *pcr)
> option->ltr_l1off_snooze_sspwrgate = 0x78;
> option->dev_aspm_mode = DEV_ASPM_DYNAMIC;
>
> + hw_param->interrupt_en |= DELINK_INT_EN;
> option->ocp_en = 1;
> hw_param->interrupt_en |= SD_OC_INT_EN;
> hw_param->ocp_glitch = SD_OCP_GLITCH_800U;
> diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c
> index 06038b325b02..2127c7b6b2e1 100644
> --- a/drivers/misc/cardreader/rtsx_pcr.c
> +++ b/drivers/misc/cardreader/rtsx_pcr.c
> @@ -1026,6 +1026,13 @@ static irqreturn_t rtsx_pci_isr(int irq, void *dev_id)
> } else {
> pcr->card_removed |= SD_EXIST;
> pcr->card_inserted &= ~SD_EXIST;
> +#ifdef MISC_RTSX_PCI_SD_EXPRESS
> + if (PCI_PID(pcr) == PID_5261) {
> + rtsx_pci_write_register(pcr, RTS5261_FW_STATUS,
> + RTS5261_EXPRESS_LINK_FAIL_MASK, 0);
> + pcr->extra_caps |= EXTRA_CAPS_SD_EXPRESS;
> + }
> +#endif

Same here, not allowed.

Why do you need the config option at all anyway, shouldn't it be
dynamically detected?

> }
> pcr->dma_error_count = 0;
> }
> diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> index 22bf528294b9..7c70d267644b 100644
> --- a/drivers/mmc/core/sd_ops.c
> +++ b/drivers/mmc/core/sd_ops.c
> @@ -171,7 +171,14 @@ int mmc_send_if_cond(struct mmc_host *host, u32 ocr)
> * SD 1.0 cards.
> */
> cmd.opcode = SD_SEND_IF_COND;
> - cmd.arg = ((ocr & 0xFF8000) != 0) << 8 | test_pattern;
> + /*
> + * Host asks card's PCIe availability
> + * if PCIe interface is supported by host.
> + */
> + if ((ocr & 0xFF8000) && (host->caps2 & MMC_CAP2_SD_EXPRESS))
> + cmd.arg = 0x31 << 8 | test_pattern;
> + else
> + cmd.arg = ((ocr & 0xFF8000) != 0) << 8 | test_pattern;

See you detect this dynamically here, so the above config option should
not be needed.

> cmd.flags = MMC_RSP_SPI_R7 | MMC_RSP_R7 | MMC_CMD_BCR;
>
> err = mmc_wait_for_cmd(host, &cmd, 0);
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index bd50935dc37d..87ad75b253eb 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -219,6 +219,7 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc *host,
> int rsp_type;
> int stat_idx;
> bool clock_toggled = false;
> + u32 relink_time, val;
>
> dev_dbg(sdmmc_dev(host), "%s: SD/MMC CMD %d, arg = 0x%08x\n",
> __func__, cmd_idx, arg);
> @@ -322,6 +323,44 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc *host,
> if (err && clock_toggled)
> rtsx_pci_write_register(pcr, SD_BUS_STAT,
> SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0);
> +
> + /*
> + * If card has PCIe availability and WP if off,
> + * reader switch to PCIe mode.
> + */
> + val = rtsx_pci_readl(pcr, RTSX_BIPR);
> + if (cmd->opcode == 8 && ((cmd->resp[0] >> 8) & 0x10)
> + && !(val & SD_WRITE_PROTECT)) {
> + /* 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);
> + cmd->error = -EPERM;
> + }
> }
>
> static int sd_read_data(struct realtek_pci_sdmmc *host, struct mmc_command *cmd,
> @@ -1123,6 +1162,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc)
> dev_dbg(sdmmc_dev(host), "%s: RTSX_BIPR = 0x%08x\n", __func__, val);
> if (val & SD_EXIST)
> cd = 1;
> + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS)
> + mmc->caps2 |= MMC_CAP2_SD_EXPRESS;
>
> mutex_unlock(&pcr->pcr_mutex);
>
> @@ -1333,6 +1374,8 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host)
> mmc->caps |= MMC_CAP_1_8V_DDR;
> if (pcr->extra_caps & EXTRA_CAPS_MMC_8BIT)
> mmc->caps |= MMC_CAP_8_BIT_DATA;
> + if (pcr->extra_caps & EXTRA_CAPS_SD_EXPRESS)
> + mmc->caps2 |= MMC_CAP2_SD_EXPRESS;
> }
>
> static void realtek_init_host(struct realtek_pci_sdmmc *host)
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index ba703384bea0..ac1e3da4ad4f 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -369,6 +369,7 @@ struct mmc_host {
> #define MMC_CAP2_CQE_DCMD (1 << 24) /* CQE can issue a direct command */
> #define MMC_CAP2_AVOID_3_3V (1 << 25) /* Host must negotiate down from 3.3V */
> #define MMC_CAP2_MERGE_CAPABLE (1 << 26) /* Host can merge a segment over the segment size */
> +#define MMC_CAP2_SD_EXPRESS (1 << 27) /* Host support sd express card */

BIT(27)?

thanks,

greg k-h

Next message: Wolfram Sang: "Re: [PATCH] i2c: use my kernel.org address from now on"
Previous message: Wolfram Sang: "Re: [PATCH] i2c: fix missing pm_runtime_put_sync in i2c_device_probe"
Next in thread: Ulf Hansson: "Re: [PATCH] mmc: rtsx: Add SD Express mode support for RTS5261"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]