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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 15 Oct 2017 16:26:18 +0800
From:   CK Hu <ck.hu@...iatek.com>
To:     Chaotian Jing <chaotian.jing@...iatek.com>
CC:     Ulf Hansson <ulf.hansson@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        <devicetree@...r.kernel.org>, <srv_heupstream@...iatek.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Will Deacon <will.deacon@....com>,
        <linux-kernel@...r.kernel.org>, yong mao <yong.mao@...iatek.com>,
        Phong LE <ple@...libre.com>, Rob Herring <robh+dt@...nel.org>,
        <linux-mediatek@...ts.infradead.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        <linux-mmc@...r.kernel.org>, Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH v5 02/12] mmc: mediatek: add support of mt2701/mt2712

Hi, Chaotian:

On Wed, 2017-10-11 at 10:41 +0800, Chaotian Jing wrote:
> mt2701/mt2712 has 12bit clock div, which is not compatible with
> mt8135/mt8173. and, some additional features will be added in
> mt2701/mt2712, so that need distinguish it by comatibale name.
> 
> Signed-off-by: Chaotian Jing <chaotian.jing@...iatek.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 82 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 267f7ab..643c795 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -95,6 +95,9 @@
>  #define MSDC_CFG_CKDIV          (0xff << 8)	/* RW */
>  #define MSDC_CFG_CKMOD          (0x3 << 16)	/* RW */
>  #define MSDC_CFG_HS400_CK_MODE  (0x1 << 18)	/* RW */
> +#define MSDC_CFG_HS400_CK_MODE_EXTRA  (0x1 << 22)	/* RW */
> +#define MSDC_CFG_CKDIV_EXTRA    (0xfff << 8)	/* RW */
> +#define MSDC_CFG_CKMOD_EXTRA    (0x3 << 20)	/* RW */
>  
>  /* MSDC_IOCON mask */
>  #define MSDC_IOCON_SDR104CKS    (0x1 << 0)	/* RW */
> @@ -295,6 +298,10 @@ struct msdc_save_para {
>  	u32 emmc50_cfg0;
>  };
>  
> +struct mtk_mmc_compatible {
> +	u8 clk_div_bits;
> +};
> +
>  struct msdc_tune_para {
>  	u32 iocon;
>  	u32 pad_tune;
> @@ -309,6 +316,7 @@ struct msdc_delay_phase {
>  
>  struct msdc_host {
>  	struct device *dev;
> +	const struct mtk_mmc_compatible *dev_comp;
>  	struct mmc_host *mmc;	/* mmc structure */
>  	int cmd_rsp;
>  
> @@ -350,6 +358,31 @@ struct msdc_host {
>  	struct msdc_tune_para saved_tune_para; /* tune result of CMD21/CMD19 */
>  };
>  
> +static const struct mtk_mmc_compatible mt8135_compat = {
> +	.clk_div_bits = 8,
> +};
> +
> +static const struct mtk_mmc_compatible mt8173_compat = {
> +	.clk_div_bits = 8,
> +};
> +
> +static const struct mtk_mmc_compatible mt2701_compat = {
> +	.clk_div_bits = 12,
> +};
> +
> +static const struct mtk_mmc_compatible mt2712_compat = {
> +	.clk_div_bits = 12,
> +};
> +
> +static const struct of_device_id msdc_of_ids[] = {
> +	{ .compatible = "mediatek,mt8135-mmc", .data = &mt8135_compat},
> +	{ .compatible = "mediatek,mt8173-mmc", .data = &mt8173_compat},

Because mt8135_compat is equal to mt8173_compat, so I think the
compatible of "mediatek,mt8173-mmc" is redundant. You could just keep
compatible of "mediatek,mt8135-mmc" in driver, and use
"mediatek,mt8135-mmc" for both mt8135.dtsi and mt8173.dtsi.

> +	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
> +	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},

Ditto.

Regards,
CK

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, msdc_of_ids);
> +
>  static void sdr_set_bits(void __iomem *reg, u32 bs)
>  {
>  	u32 val = readl(reg);
> @@ -509,7 +542,12 @@ static void msdc_set_timeout(struct msdc_host *host, u32 ns, u32 clks)
>  		timeout = (ns + clk_ns - 1) / clk_ns + clks;
>  		/* in 1048576 sclk cycle unit */
>  		timeout = (timeout + (0x1 << 20) - 1) >> 20;
> -		sdr_get_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD, &mode);
> +		if (host->dev_comp->clk_div_bits == 8)
> +			sdr_get_field(host->base + MSDC_CFG,
> +				      MSDC_CFG_CKMOD, &mode);
> +		else
> +			sdr_get_field(host->base + MSDC_CFG,
> +				      MSDC_CFG_CKMOD_EXTRA, &mode);
>  		/*DDR mode will double the clk cycles for data timeout */
>  		timeout = mode >= 2 ? timeout * 2 : timeout;
>  		timeout = timeout > 1 ? timeout - 1 : 0;
> @@ -548,7 +586,11 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>  
>  	flags = readl(host->base + MSDC_INTEN);
>  	sdr_clr_bits(host->base + MSDC_INTEN, flags);
> -	sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_HS400_CK_MODE);
> +	if (host->dev_comp->clk_div_bits == 8)
> +		sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_HS400_CK_MODE);
> +	else
> +		sdr_clr_bits(host->base + MSDC_CFG,
> +			     MSDC_CFG_HS400_CK_MODE_EXTRA);
>  	if (timing == MMC_TIMING_UHS_DDR50 ||
>  	    timing == MMC_TIMING_MMC_DDR52 ||
>  	    timing == MMC_TIMING_MMC_HS400) {
> @@ -568,8 +610,12 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>  
>  		if (timing == MMC_TIMING_MMC_HS400 &&
>  		    hz >= (host->src_clk_freq >> 1)) {
> -			sdr_set_bits(host->base + MSDC_CFG,
> -				     MSDC_CFG_HS400_CK_MODE);
> +			if (host->dev_comp->clk_div_bits == 8)
> +				sdr_set_bits(host->base + MSDC_CFG,
> +					     MSDC_CFG_HS400_CK_MODE);
> +			else
> +				sdr_set_bits(host->base + MSDC_CFG,
> +					     MSDC_CFG_HS400_CK_MODE_EXTRA);
>  			sclk = host->src_clk_freq >> 1;
>  			div = 0; /* div is ignore when bit18 is set */
>  		}
> @@ -587,8 +633,15 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>  			sclk = (host->src_clk_freq >> 2) / div;
>  		}
>  	}
> -	sdr_set_field(host->base + MSDC_CFG, MSDC_CFG_CKMOD | MSDC_CFG_CKDIV,
> -		      (mode << 8) | div);
> +	if (host->dev_comp->clk_div_bits == 8)
> +		sdr_set_field(host->base + MSDC_CFG,
> +			      MSDC_CFG_CKMOD | MSDC_CFG_CKDIV,
> +			      (mode << 8) | div);
> +	else
> +		sdr_set_field(host->base + MSDC_CFG,
> +			      MSDC_CFG_CKMOD_EXTRA | MSDC_CFG_CKDIV_EXTRA,
> +			      (mode << 12) | div);
> +
>  	sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_CKPDN);
>  	while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
>  		cpu_relax();
> @@ -1617,12 +1670,17 @@ static int msdc_drv_probe(struct platform_device *pdev)
>  	struct mmc_host *mmc;
>  	struct msdc_host *host;
>  	struct resource *res;
> +	const struct of_device_id *of_id;
>  	int ret;
>  
>  	if (!pdev->dev.of_node) {
>  		dev_err(&pdev->dev, "No DT found\n");
>  		return -EINVAL;
>  	}
> +
> +	of_id = of_match_node(msdc_of_ids, pdev->dev.of_node);
> +	if (!of_id)
> +		return -EINVAL;
>  	/* Allocate MMC host for this device */
>  	mmc = mmc_alloc_host(sizeof(struct msdc_host), &pdev->dev);
>  	if (!mmc)
> @@ -1686,11 +1744,15 @@ static int msdc_drv_probe(struct platform_device *pdev)
>  	msdc_of_property_parse(pdev, host);
>  
>  	host->dev = &pdev->dev;
> +	host->dev_comp = of_id->data;
>  	host->mmc = mmc;
>  	host->src_clk_freq = clk_get_rate(host->src_clk);
>  	/* Set host parameters to mmc */
>  	mmc->ops = &mt_msdc_ops;
> -	mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 255);
> +	if (host->dev_comp->clk_div_bits == 8)
> +		mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 255);
> +	else
> +		mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 4095);
>  
>  	mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
>  	/* MMC core transfer sizes tunable parameters */
> @@ -1839,12 +1901,6 @@ static int msdc_runtime_resume(struct device *dev)
>  	SET_RUNTIME_PM_OPS(msdc_runtime_suspend, msdc_runtime_resume, NULL)
>  };
>  
> -static const struct of_device_id msdc_of_ids[] = {
> -	{   .compatible = "mediatek,mt8135-mmc", },
> -	{}
> -};
> -MODULE_DEVICE_TABLE(of, msdc_of_ids);
> -
>  static struct platform_driver mt_msdc_driver = {
>  	.probe = msdc_drv_probe,
>  	.remove = msdc_drv_remove,


Powered by blists - more mailing lists