Re: [PATCH v2 1/2] mmc: mtk-sd: Add support for MT8196

From: Andy-ld Lu (卢东)
Date: Wed Oct 09 2024 - 02:15:15 EST

Next message: Mohammad Rafi Shaik: "Re: [RESEND v1] arm64: defconfig: enable WCD937x driver as module"
Previous message: kernel test robot: "[tip:core/merge] BUILD SUCCESS fe730cbdb53f5d8cfe2c443484de21e0f59a403c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 2024-09-30 at 11:13 +0200, AngeloGioacchino Del Regno wrote:
> 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@xxxxxxxxxxxx>
> > ---
> > 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.

Thanks for your review, and sorry for the late reply.

New tx and rx could be separately supported in our next platforms, so
we use two bool members to indicate.
>
> > };
> >
> > 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;

I will follow your comment in next change.
>
> > + 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;

I will follow your comment in next change.
>
> >
> > 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;

I will follow your comment in next change.
>
> > +
> > 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.

Thanks for your wonderful suggestion, I will follow your comment in
next change.
>
> > + 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.

I would move it to another commit, and add the value of 'stop_dly_sel'
to all the SoCs whose 'stop_clk_fix' is true.
>
> > + 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

Next message: Mohammad Rafi Shaik: "Re: [RESEND v1] arm64: defconfig: enable WCD937x driver as module"
Previous message: kernel test robot: "[tip:core/merge] BUILD SUCCESS fe730cbdb53f5d8cfe2c443484de21e0f59a403c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]