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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux