2016-11-01 15:25 GMT+08:00 Jaehoon Chung <jh80.chung@xxxxxxxxxxx>:
> On 11/01/2016 10:16 AM, Jun Nie wrote:
>> 2016-10-31 17:40 GMT+08:00 Jaehoon Chung <jh80.chung@xxxxxxxxxxx>:
>>> On 10/31/2016 05:47 PM, Jun Nie wrote:
>>>> 2016-10-28 13:16 GMT+08:00 Jaehoon Chung <jh80.chung@xxxxxxxxxxx>:
>>>>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>>>>> This platform driver adds initial support for the DW host controller
>>>>>> found on ZTE SoCs.
>>>>>>
>>>>>> It has been tested on ZX296718 EVB board currently. More support on
>>>>>> timing tuning will be added when hardware is available.
>>>>>>
>>>>>> Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/mmc/host/Kconfig | 9 ++
>>>>>> drivers/mmc/host/Makefile | 1 +
>>>>>> drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>>>>>> drivers/mmc/host/dw_mmc-zx.h | 23 +++++
>>>>>> 4 files changed, 263 insertions(+)
>>>>>> create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>>>>>> create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>>>> index 5274f50..2b3202c 100644
>>>>>> --- a/drivers/mmc/host/Kconfig
>>>>>> +++ b/drivers/mmc/host/Kconfig
>>>>>> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>>>>>> Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>>> for platforms based on RK3066, RK3188 and RK3288 SoC's.
>>>>>>
>>>>>> +config MMC_DW_ZX
>>>>>> + tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
>>>>>> + depends on MMC_DW
>>>>>
>>>>> I guess MMC_DW_ZX depends on your SoC config, doesn't?
>>>>
>>>> Right, will add dependency.
>>>>
>>>>>
>>>>>> + select MMC_DW_PLTFM
>>>>>> + help
>>>>>> + This selects support for ZTE SoC specific extensions to the
>>>>>> + Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>>> + for platforms based on ZX296718 SoC's.
>>>>>> +
>>>>>> config MMC_SH_MMCIF
>>>>>> tristate "SuperH Internal MMCIF support"
>>>>>> depends on HAS_DMA
>>>>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>>>>> index e2bdaaf..9766143 100644
>>>>>> --- a/drivers/mmc/host/Makefile
>>>>>> +++ b/drivers/mmc/host/Makefile
>>>>>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
>>>>>> obj-$(CONFIG_MMC_DW_K3) += dw_mmc-k3.o
>>>>>> obj-$(CONFIG_MMC_DW_PCI) += dw_mmc-pci.o
>>>>>> obj-$(CONFIG_MMC_DW_ROCKCHIP) += dw_mmc-rockchip.o
>>>>>> +obj-$(CONFIG_MMC_DW_ZX) += dw_mmc-zx.o
>>>>>> obj-$(CONFIG_MMC_SH_MMCIF) += sh_mmcif.o
>>>>>> obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o
>>>>>> obj-$(CONFIG_MMC_VUB300) += vub300.o
>>>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
>>>>>> new file mode 100644
>>>>>> index 0000000..0404f8e
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mmc/host/dw_mmc-zx.c
>>>>>> @@ -0,0 +1,230 @@
>>>>>> +/*
>>>>>> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
>>>>>> + *
>>>>>> + * Copyright (C) 2016, Linaro Ltd.
>>>>>> + * Copyright (C) 2016, ZTE Corp.
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>>> + * (at your option) any later version.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/clk.h>
>>>>>> +#include <linux/mfd/syscon.h>
>>>>>> +#include <linux/mmc/dw_mmc.h>
>>>>>> +#include <linux/mmc/host.h>
>>>>>> +#include <linux/mmc/mmc.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/of.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/pm_runtime.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +
>>>>>> +#include "dw_mmc.h"
>>>>>> +#include "dw_mmc-pltfm.h"
>>>>>> +#include "dw_mmc-zx.h"
>>>>>> +
>>>>>> +#define ZX_DLL_LOCKED BIT(2)
>>>>>
>>>>> Some DLL rigsters and bits are defined in dw_mmc-zx.h.
>>>>> why defined ZX_DLL_LOCKED at here.
>>>>>
>>>>> You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h
>>>>>
>>>> Will move together.
>>>>
>>>>>> +
>>>>>> +struct dw_mci_zx_priv_data {
>>>>>> + struct regmap *sysc_base;
>>>>>> +};
>>>>>> +
>>>>>> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
>>>>>> + unsigned int clk_flag)
>>>>>
>>>>> Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
>>>>> And 0 and 1 are what means? On/Off?
>>>>>
>>>> Yes, enumeration shall looks better here. It is a flag for different
>>>> delay type. Will change to enumeration.
>>>>
>>>>>> +{
>>>>>> + struct dw_mci_zx_priv_data *priv = host->priv;
>>>>>> + struct regmap *sysc_base = priv->sysc_base;
>>>>>> + unsigned int clksel;
>>>>>> + unsigned int loop = 1000;
>>>>>> + int ret;
>>>>>> +
>>>>>
>>>>> priv->sysc_base doesn't never NULL?
>>>>
>>>> For this SoC, it is never NULL if dts is correct. Adding a check is
>>>> better anyway.
>>>>>
>>>>>> + ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>>>> + PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
>>>>>
>>>>> Could you add the comment for controlling this regs?
>>>>> I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?
>>>>>
>>>>> It doesn't affect to other bit?
>>>>> I think you can use the regmap_update_bits instead of regmap_write.
>>>>
>>>> It does not affect other bit as all bits are write as desired. But
>>>> your suggestion make it clearer.
>>>>
>>>>>
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + if (clk_flag) {
>>>>>> + clksel &= ~(CLK_SAMP_DELAY(0x7F));
>>>>>
>>>>> It's meaningless..CLK_SAMP_DELAY used only at here.
>>>>> CLK_SAMP_DELAY ((x) & 0x7F << 0)
>>>>>
>>>>> It's just CLK_SAMP_DELAY_MASK.?
>>>>>
>>>>> #define CLK_SAMP_DELAY_MASK (0x7F << 0)
>>>>> clksel &= ~CLK_SAMP_DELAY_MASK;
>>>>>
>>>> Right, clearer.
>>>>
>>>>>
>>>>>> + clksel |= (delay << 8);
>>>>>
>>>>> Use the CLK_SAMP_DELAY_SHIFT instead of 8.
>>>>>
>>>>>> + } else {
>>>>>> + clksel &= ~(READ_DQS_DELAY(0x7F));
>>>>>
>>>>> Ditto.
>>>>> And i think it also can be changed to regmap_update_bits.
>>>>>
>>>>>> + clksel |= delay;
>>>>>> + }
>>>>>> +
>>>>>> + regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
>>>>>> + regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>>>> + PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
>>>>>> + DLL_REG_SET);
>>>>>
>>>>> regmap_update_bits?
>>>>
>>>> Will do.
>>>>
>>>>>
>>>>>> +
>>>>>> + do {
>>>>>> + ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + } while (--loop && !(clksel & ZX_DLL_LOCKED));
>>>>>> +
>>>>>> + if (!loop) {
>>>>>> + dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
>>>>>> + return -EIO;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>>>> +{
>>>>>> + struct dw_mci *host = slot->host;
>>>>>> + struct mmc_host *mmc = slot->mmc;
>>>>>> + int len, start = 0, end = 0, delay, best = 0;
>>>>>> + int ret = 0;
>>>>>> +
>>>>>> + for (delay = 1 ; delay < 128; delay++) {
>>>>>> + ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>
>>>>> When it's failed, just returned.
>>>>> Doesn't need to try with next delay value?
>>>>
>>>> Yes, more retry make robust.
>>>>
>>>>>
>>>>>> +
>>>>>> + if (mmc_send_tuning(mmc, opcode, NULL)) {
>>>>>> + if (start >= 0) {
>>>>>> + end = delay - 1;
>>>>>> + /* check and update longest good range */
>>>>>> + if ((end - start) > len) {
>>>>>> + best = (start + end) >> 1;
>>>>>> + len = end - start;
>>>>>> + }
>>>>>> + }
>>>>>> + start = -1;
>>>>>> + end = 0;
>>>>>> + continue;
>>>>>> + }
>>>>>> + if (start < 0)
>>>>>> + start = delay;
>>>>>> + }
>>>>>> +
>>>>>> + if (start >= 0) {
>>>>>> + end = delay - 1;
>>>>>> + if ((end - start) > len) {
>>>>>> + best = (start + end) >> 1;
>>>>>> + len = end - start;
>>>>>> + }
>>>>>> + }
>>>>>> + if (best < 0)
>>>>>> + return -EIO;
>>>>>> +
>>>>>> + dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
>>>>>> + start, end);
>>>>>> + dw_mci_zx_emmc_set_delay(host, best, 1);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
>>>>>> + struct mmc_ios *ios)
>>>>>> +{
>>>>>> + int ret;
>>>>>> +
>>>>>> + /* config phase shift 90 */
>>>>>> + ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
>>>>>
>>>>> It's always fixed to 32? What means 32?
>>>>
>>>> It means 90 degree shift for value 32. This configuration comes from
>>>> ZTE engineer as I do not have hardware for this tuning. Let's just
>>>> keep it with adding more comments till tuning is needed.
>>>>
>>>>>
>>>>>> + if (ret < 0)
>>>>>> + return -EIO;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>>>> +{
>>>>>> + struct dw_mci *host = slot->host;
>>>>>> +
>>>>>> + if (host->verid == 0x290a) /* emmc */
>>>>>> + return dw_mci_zx_emmc_execute_tuning(slot, opcode);
>>>>>
>>>>> I didn't know why you check host->verid is 2.90a..
>>>>> Is there any reason?
>>>> There are two version DW MMC IP on this SoC and different
>>>> configuration is needed for them. I do not have hardware for tuning
>>>> version 210A timing and will be added later.
>>>
>>> you means there are two IP version with same SoC?
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>
>> Right, two version IP exist on the same SoC.
>
> It's strange. :) The latest and older versions are existed on same SoC.
> Then i recommend that add the comment for this status.
> Why you put this condition, and which case is used..
> Not just "/* emmc */" :)
>
> One more thing..dw_mci_zx_emmc_set_delay() and dw_mci_zx_emmc_set_delay() are for only emmc?
> other cards don't use this function?
>
> just can remove "_emmc_"?
>
> Best Regards,
> Jaehoon Chung
Yes, 290A version for eMMC and 210A version for SD/SDIO. Only 290A
version timing tuning use this function and tuning registers is SoC
specific. So *_emmc_* name is dedicated for 290A version here. We need
a name anyway, *_290A_* can be a candidate, but not as direct as
*_emmc_* for this SoCs.
>
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
>>>>>> +{
>>>>>> + struct device_node *np = host->dev->of_node;
>>>>>> + struct device_node *node;
>>>>>> + struct dw_mci_zx_priv_data *priv;
>>>>>> + struct regmap *sysc_base;
>>>>>> + int ret;
>>>>>> +
>>>>>> + node = of_parse_phandle(np, "zte,aon-syscon", 0);
>>>>>> + if (node) {
>>>>>> + sysc_base = syscon_node_to_regmap(node);
>>>>>> + of_node_put(node);
>>>>>
>>>>> Use the syscon_regmap_lookup_by_phandle(). It's same behavior.
>>>> Will do.
>>>>>
>>>>>> +
>>>>>> + if (IS_ERR(sysc_base)) {
>>>>>> + ret = PTR_ERR(sysc_base);
>>>>>> + if (ret != -EPROBE_DEFER)
>>>>>> + dev_err(host->dev, "Can't get syscon: %d\n",
>>>>>> + ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>>>>>> + if (!priv)
>>>>>> + return -ENOMEM;
>>>>>> + priv->sysc_base = sysc_base;
>>>>>
>>>>> Is there no case that sysc_base is NULL?
>>>>
>>>> sysc_base is needed only for eMMC. So it is NULL for SD/MMC cases, and
>>>> we can save memory for SD/MMC cases here :)
>>>>
>>>>>
>>>>>> + host->priv = priv;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static unsigned long zx_dwmmc_caps[3] = {
>>>>>> + MMC_CAP_CMD23,
>>>>>> + MMC_CAP_CMD23,
>>>>>> + MMC_CAP_CMD23,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct dw_mci_drv_data zx_drv_data = {
>>>>>> + .caps = zx_dwmmc_caps,
>>>>>> + .execute_tuning = dw_mci_zx_execute_tuning,
>>>>>> + .prepare_hs400_tuning = dw_mci_zx_prepare_hs400_tuning,
>>>>>> + .parse_dt = dw_mci_zx_parse_dt,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct of_device_id dw_mci_zx_match[] = {
>>>>>> + { .compatible = "zte,dw-mshc", .data = &zx_drv_data},
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
>>>>>> +
>>>>>> +static int dw_mci_zx_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> + const struct dw_mci_drv_data *drv_data;
>>>>>> + const struct of_device_id *match;
>>>>>> +
>>>>>> + match = of_match_node(dw_mci_zx_match, pdev->dev.of_node);
>>>>>> + drv_data = match->data;
>>>>>> +
>>>>>> + return dw_mci_pltfm_register(pdev, drv_data);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
>>>>>> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>>>> + pm_runtime_force_resume)
>>>>>> + SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
>>>>>> + dw_mci_runtime_resume,
>>>>>> + NULL)
>>>>>> +};
>>>>>> +
>>>>>> +static struct platform_driver dw_mci_zx_pltfm_driver = {
>>>>>> + .probe = dw_mci_zx_probe,
>>>>>> + .remove = dw_mci_pltfm_remove,
>>>>>> + .driver = {
>>>>>> + .name = "dwmmc_zx",
>>>>>> + .of_match_table = dw_mci_zx_match,
>>>>>> + .pm = &dw_mci_zx_dev_pm_ops,
>>>>>> + },
>>>>>> +};
>>>>>> +
>>>>>> +module_platform_driver(dw_mci_zx_pltfm_driver);
>>>>>> +
>>>>>> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
>>>>>> new file mode 100644
>>>>>> index 0000000..b1aac52
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mmc/host/dw_mmc-zx.h
>>>>>> @@ -0,0 +1,23 @@
>>>>>> +#ifndef _DW_MMC_ZX_H_
>>>>>> +#define _DW_MMC_ZX_H_
>>>>>> +
>>>>>> +/* dll reg offset*/
>>>>>> +#define LB_AON_EMMC_CFG_REG0 0x1B0
>>>>>> +#define LB_AON_EMMC_CFG_REG1 0x1B4
>>>>>> +#define LB_AON_EMMC_CFG_REG2 0x1B8
>>>>>> +
>>>>>> +/* LB_AON_EMMC_CFG_REG0 register defines */
>>>>>> +#define PARA_DLL_START_POINT(x) (((x) & 0xFF) << 0)
>>>>>> +#define DLL_REG_SET BIT(8)
>>>>>> +#define PARA_DLL_LOCK_NUM(x) (((x) & 7) << 16)
>>>>>> +#define PARA_PHASE_DET_SEL(x) (((x) & 7) << 20)
>>>>>> +#define PARA_DLL_BYPASS_MODE BIT(23)
>>>>>> +#define PARA_HALF_CLK_MODE BIT(24)
>>>>>
>>>>> PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.
>>>>>
>>>>>> +
>>>>>> +/* LB_AON_EMMC_CFG_REG1 register defines */
>>>>>> +#define READ_DQS_DELAY(x) (((x) & 0x7F) << 0)
>>>>>> +#define READ_DQS_BYPASS_MODE BIT(7)
>>>>>> +#define CLK_SAMP_DELAY(x) (((x) & 0x7F) << 8)
>>>>>> +#define CLK_SAMP_BYPASS_MODE BIT(15)
>>>>>
>>>>> Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.
>>>>>
>>>>>
>>>>> Hmm. These are not dwmmc host controller's register.
>>>>> So If you needs to add these defines..I think you needs to add dessriptions in more detail.
>>>>>
>>>>> At least..Which board's DLL reg offset.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>>> +
>>>>>> +#endif /* _DW_MMC_ZX_H_ */
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
