| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e969918b-3af4-4880-9dc6-fba868a9937a@collabora.com>
Date: Mon, 30 Sep 2024 11:13:50 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Andy-ld Lu <andy-ld.lu@...iatek.com>, ulf.hansson@...aro.org,
robh@...nel.org, krzk+dt@...nel.org, matthias.bgg@...il.com,
wenbin.mei@...iatek.com
Cc: linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2 1/2] mmc: mtk-sd: Add support for MT8196
Il 29/09/24 09:44, Andy-ld Lu ha scritto:
> Mediatek SoC MT8196 features a new design for tx/rx path. The new tx
> path incorporates register settings that are closely associated with
> bus timing. And the difference between new rx path and older versions
> is the usage of distinct register bits when setting the data sampling
> edge as part of the tuning process.
>
> Besides, there are modified register settings for STOP_DLY_SEL and
> POP_EN_CNT, with two new fields added to the compatibility structure
> to reflect the modifications.
>
> For the changes mentioned in relation to the MT8196, the new compatible
> string 'mediatek,mt8196-mmc' is introduced. This is to accommodate
> different settings and workflows specific to the MT8196.
>
> Signed-off-by: Andy-ld Lu <andy-ld.lu@...iatek.com>
> ---
> drivers/mmc/host/mtk-sd.c | 162 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 141 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 89018b6c97b9..4254b3012aeb 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -65,6 +65,7 @@
> #define SDC_RESP3 0x4c
> #define SDC_BLK_NUM 0x50
> #define SDC_ADV_CFG0 0x64
> +#define MSDC_NEW_RX_CFG 0x68
> #define EMMC_IOCON 0x7c
> #define SDC_ACMD_RESP 0x80
> #define DMA_SA_H4BIT 0x8c
> @@ -91,6 +92,7 @@
> #define EMMC_TOP_CONTROL 0x00
> #define EMMC_TOP_CMD 0x04
> #define EMMC50_PAD_DS_TUNE 0x0c
> +#define LOOP_TEST_CONTROL 0x30
>
> /*--------------------------------------------------------------------------*/
> /* Register Mask */
> @@ -202,9 +204,13 @@
> #define SDC_STS_CMDBUSY BIT(1) /* RW */
> #define SDC_STS_SWR_COMPL BIT(31) /* RW */
>
> -#define SDC_DAT1_IRQ_TRIGGER BIT(19) /* RW */
> /* SDC_ADV_CFG0 mask */
> +#define SDC_DAT1_IRQ_TRIGGER BIT(19) /* RW */
> #define SDC_RX_ENHANCE_EN BIT(20) /* RW */
> +#define SDC_NEW_TX_EN BIT(31) /* RW */
> +
> +/* MSDC_NEW_RX_CFG mask */
> +#define MSDC_NEW_RX_PATH_SEL BIT(0) /* RW */
>
> /* DMA_SA_H4BIT mask */
> #define DMA_ADDR_HIGH_4BIT GENMASK(3, 0) /* RW */
> @@ -226,6 +232,7 @@
>
> /* MSDC_PATCH_BIT mask */
> #define MSDC_PATCH_BIT_ODDSUPP BIT(1) /* RW */
> +#define MSDC_PATCH_BIT_RD_DAT_SEL BIT(3) /* RW */
> #define MSDC_INT_DAT_LATCH_CK_SEL GENMASK(9, 7)
> #define MSDC_CKGEN_MSDC_DLY_SEL GENMASK(14, 10)
> #define MSDC_PATCH_BIT_IODSSEL BIT(16) /* RW */
> @@ -247,6 +254,8 @@
> #define MSDC_PB2_SUPPORT_64G BIT(1) /* RW */
> #define MSDC_PB2_RESPWAIT GENMASK(3, 2) /* RW */
> #define MSDC_PB2_RESPSTSENSEL GENMASK(18, 16) /* RW */
> +#define MSDC_PB2_POP_EN_CNT GENMASK(23, 20) /* RW */
> +#define MSDC_PB2_CFGCRCSTSEDGE BIT(25) /* RW */
> #define MSDC_PB2_CRCSTSENSEL GENMASK(31, 29) /* RW */
>
> #define MSDC_PAD_TUNE_DATWRDLY GENMASK(4, 0) /* RW */
> @@ -311,6 +320,12 @@
> #define PAD_DS_DLY1 GENMASK(14, 10) /* RW */
> #define PAD_DS_DLY3 GENMASK(4, 0) /* RW */
>
> +/* LOOP_TEST_CONTROL mask */
> +#define TEST_LOOP_DSCLK_MUX_SEL BIT(0) /* RW */
> +#define TEST_LOOP_LATCH_MUX_SEL BIT(1) /* RW */
> +#define LOOP_EN_SEL_CLK BIT(20) /* RW */
> +#define TEST_HS400_CMD_LOOP_MUX_SEL BIT(31) /* RW */
> +
> #define REQ_CMD_EIO BIT(0)
> #define REQ_CMD_TMO BIT(1)
> #define REQ_DAT_ERR BIT(2)
> @@ -391,6 +406,7 @@ struct msdc_save_para {
> u32 emmc_top_control;
> u32 emmc_top_cmd;
> u32 emmc50_pad_ds_tune;
> + u32 loop_test_control;
> };
>
> struct mtk_mmc_compatible {
> @@ -402,9 +418,13 @@ struct mtk_mmc_compatible {
> bool data_tune;
> bool busy_check;
> bool stop_clk_fix;
> + u8 stop_dly_sel;
> + u8 pop_en_cnt;
> bool enhance_rx;
> bool support_64g;
> bool use_internal_cd;
> + bool support_new_tx;
> + bool support_new_rx;
Is there really any platform that supports new_tx but *not* new_rx, or vice-versa?
If not, you can add just one `bool support_new_rx_tx` member to this structure.
> };
>
> struct msdc_tune_para {
> @@ -621,6 +641,23 @@ static const struct mtk_mmc_compatible mt8516_compat = {
> .stop_clk_fix = true,
> };
>
> +static const struct mtk_mmc_compatible mt8196_compat = {
> + .clk_div_bits = 12,
> + .recheck_sdio_irq = false,
> + .hs400_tune = false,
> + .pad_tune_reg = MSDC_PAD_TUNE0,
> + .async_fifo = true,
> + .data_tune = true,
> + .busy_check = true,
> + .stop_clk_fix = true,
> + .stop_dly_sel = 1,
> + .pop_en_cnt = 2,
> + .enhance_rx = true,
> + .support_64g = true,
> + .support_new_tx = true,
> + .support_new_rx = true,
> +};
> +
> static const struct of_device_id msdc_of_ids[] = {
> { .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
> { .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
> @@ -632,6 +669,7 @@ static const struct of_device_id msdc_of_ids[] = {
> { .compatible = "mediatek,mt8135-mmc", .data = &mt8135_compat},
> { .compatible = "mediatek,mt8173-mmc", .data = &mt8173_compat},
> { .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
> + { .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
> { .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
>
> {}
> @@ -872,6 +910,42 @@ static int msdc_ungate_clock(struct msdc_host *host)
> (val & MSDC_CFG_CKSTB), 1, 20000);
> }
>
> +static void msdc_new_tx_setting(struct msdc_host *host)
> +{
That's simpler:
if (!host->top_base)
return;
> + if (host->top_base) {
> + sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> + TEST_LOOP_DSCLK_MUX_SEL);
> + sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> + TEST_LOOP_LATCH_MUX_SEL);
> + sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
> + TEST_HS400_CMD_LOOP_MUX_SEL);
> + }
> +
> + switch (host->timing) {
> + case MMC_TIMING_LEGACY:
> + case MMC_TIMING_MMC_HS:
> + case MMC_TIMING_SD_HS:
> + case MMC_TIMING_UHS_SDR12:
> + case MMC_TIMING_UHS_SDR25:
> + case MMC_TIMING_UHS_DDR50:
> + case MMC_TIMING_MMC_DDR52:
> + if (host->top_base)
> + sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
> + LOOP_EN_SEL_CLK);
> + break;
> + case MMC_TIMING_UHS_SDR50:
> + case MMC_TIMING_UHS_SDR104:
> + case MMC_TIMING_MMC_HS200:
> + case MMC_TIMING_MMC_HS400:
> + if (host->top_base)
> + sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> + LOOP_EN_SEL_CLK);
> + break;
> + default:
> + break;
> + }
> +}
> +
> static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
> {
> struct mmc_host *mmc = mmc_from_priv(host);
> @@ -881,6 +955,7 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
> u32 sclk;
> u32 tune_reg = host->dev_comp->pad_tune_reg;
> u32 val;
> + bool timing_changed = false;
bool timing_changed;
>
> if (!hz) {
> dev_dbg(host->dev, "set mclk to 0\n");
> @@ -890,6 +965,9 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
> return;
> }
>
> + if (host->timing != timing)
> + timing_changed = true;
else
timing_changed = false;
> +
> flags = readl(host->base + MSDC_INTEN);
> sdr_clr_bits(host->base + MSDC_INTEN, flags);
> if (host->dev_comp->clk_div_bits == 8)
> @@ -996,6 +1074,9 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
> sdr_set_field(host->base + tune_reg,
> MSDC_PAD_TUNE_CMDRRDLY,
> host->hs400_cmd_int_delay);
> + if (timing_changed && host->dev_comp->support_new_tx)
I would invert this to (host->dev_comp->support_new_tx && timing_changed)
as that, at least to me, reads as "if this is new_tx - and the timing changed"
maning that the primary reason why we're checking is "if this is new_tx".
It's a personal preference though, so the final choice is yours.
> + msdc_new_tx_setting(host);
> +
> dev_dbg(host->dev, "sclk: %d, timing: %d\n", mmc->actual_clock,
> timing);
> }
> @@ -1704,6 +1785,17 @@ static void msdc_init_hw(struct msdc_host *host)
> reset_control_deassert(host->reset);
> }
>
> + /* New tx/rx enable bit need to be 0->1 for hardware check */
> + if (host->dev_comp->support_new_tx) {
> + sdr_clr_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
> + sdr_set_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
> + msdc_new_tx_setting(host);
> + }
> + if (host->dev_comp->support_new_rx) {
> + sdr_clr_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
> + sdr_set_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
> + }
> +
> /* Configure to MMC/SD mode, clock free running */
> sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_MODE | MSDC_CFG_CKPDN);
>
> @@ -1742,8 +1834,19 @@ static void msdc_init_hw(struct msdc_host *host)
> sdr_set_bits(host->base + EMMC50_CFG0, EMMC50_CFG_CFCSTS_SEL);
>
> if (host->dev_comp->stop_clk_fix) {
> - sdr_set_field(host->base + MSDC_PATCH_BIT1,
> - MSDC_PATCH_BIT1_STOP_DLY, 3);
> + if (host->dev_comp->stop_dly_sel)
> + sdr_set_field(host->base + MSDC_PATCH_BIT1,
> + MSDC_PATCH_BIT1_STOP_DLY,
> + host->dev_comp->stop_dly_sel & 0xf);
This doesn't look like being something to support new_tx and new_rx, but rather
a way to specify a different STOP_DLY depending on the SoC and its platdata.
So this one goes to a different commit, and you won't need either the 0xf masking
nor the `else`, as you will have to add the value to all SoCs' platdata instead.
> + else
> + sdr_set_field(host->base + MSDC_PATCH_BIT1,
> + MSDC_PATCH_BIT1_STOP_DLY, 3);
> +
> + if (host->dev_comp->pop_en_cnt)
> + sdr_set_field(host->base + MSDC_PATCH_BIT2,
> + MSDC_PB2_POP_EN_CNT,
> + host->dev_comp->pop_en_cnt & 0xf);
> +
> sdr_clr_bits(host->base + SDC_FIFO_CFG,
> SDC_FIFO_CFG_WRVALIDSEL);
> sdr_clr_bits(host->base + SDC_FIFO_CFG,
> @@ -2055,6 +2158,19 @@ static inline void msdc_set_data_delay(struct msdc_host *host, u32 value)
> }
> }
>
Regards,
Angelo
Powered by blists - more mailing lists