Re: [PATCH] mmc: sdhci-cadence: Add support for Cadence sdmmc v6

[Jaehoon Chung <jh80.chung@gmail.com>]

On 11/15/23 12:36, KuanLim.Lee wrote:
> Hi Jaehoon,
> If there are no more feedback, I will send out version2 patch soon.
> 
> On 11/7/23 14:24, Kuan Lim Lee wrote:
>>
>> Hi Jaehoon,
>>
>> On 11/1/23 08:20, Jaehoon Chung wrote:
>>> From: Jaehoon Chung <jh80.chung@samsung.com> Hi
>>>
>>> On 10/3/23 16:22, Kuan Lim Lee wrote:
>>>> From: Kuan Lim Lee <kuanlim.lee@linux.starfivetech.com>
>>>>
>>>> Cadence SDMMC v6 controller has a lot of changes on initialize
>>>> compared to v4 controller. PHY is needed by v6 controller.
>>>>
>>>> Signed-off-by: Kuan Lim Lee <kuanlim.lee@starfivetech.com>
>>>> Reviewed-by: Alex Soo <yuklin.soo@starfivetech.com>
>>>> Reviewed-by: Wei Liang Lim <weiliang.lim@starfivetech.com>
>>>
>>> I didn't see their Reviewed-by tag in mailing list.
>> They are my colleagues who collaborated develop code with me.
>> I think I should them in Signed-off-by, and put your name in Reviewed-by
>>
>>>
>>>> ---
>>>>  drivers/mmc/Kconfig              |  13 ++
>>>>  drivers/mmc/Makefile             |   1 +
>>>>  drivers/mmc/sdhci-cadence.c      |  63 ++-----
>>>>  drivers/mmc/sdhci-cadence.h      |  68 +++++++
>>>>  drivers/mmc/sdhci-cadence6-phy.c | 302
>>>> +++++++++++++++++++++++++++++++
>>>>  5 files changed, 396 insertions(+), 51 deletions(-)  create mode
>>>> 100644 drivers/mmc/sdhci-cadence.h  create mode 100644
>>>> drivers/mmc/sdhci-cadence6-phy.c
>>>>
>>>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig index
>>>> de01b9687b..cec881d862 100644
>>>> --- a/drivers/mmc/Kconfig
>>>> +++ b/drivers/mmc/Kconfig
>>>> @@ -573,6 +573,19 @@ config MMC_SDHCI_CADENCE
>>>>
>>>>  	  If unsure, say N.
>>>>
>>>> +config MMC_SDHCI_CADENCE_V6
>>>> +	bool "SDHCI support for the Cadence SD/SDIO/eMMC controller &
>>> driver version 6"
>>>> +	depends on BLK && DM_MMC
>>>> +	depends on MMC_SDHCI
>>>> +	depends on OF_CONTROL
>>>> +	select MMC_SDHCI_CADENCE
>>>> +	help
>>>> +	  This selects the Cadence SD/SDIO/eMMC driver version 6.
>>>> +
>>>> +	  If you have a controller with this interface, say Y here.
>>>> +
>>>> +	  If unsure, say N.
>>>> +
>>>>  config MMC_SDHCI_AM654
>>>>  	bool "SDHCI Controller on TI's Am654 devices"
>>>>  	depends on ARCH_K3
>>>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index
>>>> 2c65c4765a..cdcce55b8b 100644
>>>> --- a/drivers/mmc/Makefile
>>>> +++ b/drivers/mmc/Makefile
>>>> @@ -61,6 +61,7 @@ obj-$(CONFIG_MMC_SDHCI_ATMEL)		+=
>>> atmel_sdhci.o
>>>>  obj-$(CONFIG_MMC_SDHCI_BCM2835)		+= bcm2835_sdhci.o
>>>>  obj-$(CONFIG_MMC_SDHCI_BCMSTB)		+= bcmstb_sdhci.o
>>>>  obj-$(CONFIG_MMC_SDHCI_CADENCE)		+= sdhci-cadence.o
>>>> +obj-$(CONFIG_MMC_SDHCI_CADENCE_V6)	+= sdhci-cadence6-phy.o
>>>>  obj-$(CONFIG_MMC_SDHCI_AM654)		+= am654_sdhci.o
>>>>  obj-$(CONFIG_MMC_SDHCI_IPROC)		+= iproc_sdhci.o
>>>>  obj-$(CONFIG_MMC_SDHCI_KONA)		+= kona_sdhci.o
>>>> diff --git a/drivers/mmc/sdhci-cadence.c
>>>> b/drivers/mmc/sdhci-cadence.c index 327a05ad11..d7a270e74c 100644
>>>> --- a/drivers/mmc/sdhci-cadence.c
>>>> +++ b/drivers/mmc/sdhci-cadence.c
>>>> @@ -17,56 +17,7 @@
>>>>  #include <linux/libfdt.h>
>>>>  #include <mmc.h>
>>>>  #include <sdhci.h>
>>>> -
>>>> -/* HRS - Host Register Set (specific to Cadence) */
>>>> -#define SDHCI_CDNS_HRS04		0x10		/* PHY access
>> port */
>>>> -#define   SDHCI_CDNS_HRS04_ACK			BIT(26)
>>>> -#define   SDHCI_CDNS_HRS04_RD			BIT(25)
>>>> -#define   SDHCI_CDNS_HRS04_WR			BIT(24)
>>>> -#define   SDHCI_CDNS_HRS04_RDATA		GENMASK(23, 16)
>>>> -#define   SDHCI_CDNS_HRS04_WDATA		GENMASK(15, 8)
>>>> -#define   SDHCI_CDNS_HRS04_ADDR			GENMASK(5,
>> 0)
>>>> -
>>>> -#define SDHCI_CDNS_HRS06		0x18		/* eMMC
>> control */
>>>> -#define   SDHCI_CDNS_HRS06_TUNE_UP		BIT(15)
>>>> -#define   SDHCI_CDNS_HRS06_TUNE			GENMASK(13,
>> 8)
>>>> -#define   SDHCI_CDNS_HRS06_MODE			GENMASK(2,
>> 0)
>>>> -#define   SDHCI_CDNS_HRS06_MODE_SD		0x0
>>>> -#define   SDHCI_CDNS_HRS06_MODE_MMC_SDR		0x2
>>>> -#define   SDHCI_CDNS_HRS06_MODE_MMC_DDR		0x3
>>>> -#define   SDHCI_CDNS_HRS06_MODE_MMC_HS200	0x4
>>>> -#define   SDHCI_CDNS_HRS06_MODE_MMC_HS400	0x5
>>>> -#define   SDHCI_CDNS_HRS06_MODE_MMC_HS400ES	0x6
>>>> -
>>>> -/* SRS - Slot Register Set (SDHCI-compatible) */
>>>> -#define SDHCI_CDNS_SRS_BASE		0x200
>>>> -
>>>> -/* PHY */
>>>> -#define SDHCI_CDNS_PHY_DLY_SD_HS	0x00
>>>> -#define SDHCI_CDNS_PHY_DLY_SD_DEFAULT	0x01
>>>> -#define SDHCI_CDNS_PHY_DLY_UHS_SDR12	0x02
>>>> -#define SDHCI_CDNS_PHY_DLY_UHS_SDR25	0x03
>>>> -#define SDHCI_CDNS_PHY_DLY_UHS_SDR50	0x04
>>>> -#define SDHCI_CDNS_PHY_DLY_UHS_DDR50	0x05
>>>> -#define SDHCI_CDNS_PHY_DLY_EMMC_LEGACY	0x06
>>>> -#define SDHCI_CDNS_PHY_DLY_EMMC_SDR	0x07
>>>> -#define SDHCI_CDNS_PHY_DLY_EMMC_DDR	0x08
>>>> -#define SDHCI_CDNS_PHY_DLY_SDCLK	0x0b
>>>> -#define SDHCI_CDNS_PHY_DLY_HSMMC	0x0c
>>>> -#define SDHCI_CDNS_PHY_DLY_STROBE	0x0d
>>>> -
>>>> -/*
>>>> - * The tuned val register is 6 bit-wide, but not the whole of the
>>>> range is
>>>> - * available.  The range 0-42 seems to be available (then 43 wraps
>>>> around to 0)
>>>> - * but I am not quite sure if it is official.  Use only 0 to 39 for safety.
>>>> - */
>>>> -#define SDHCI_CDNS_MAX_TUNING_LOOP	40
>>>> -
>>>> -struct sdhci_cdns_plat {
>>>> -	struct mmc_config cfg;
>>>> -	struct mmc mmc;
>>>> -	void __iomem *hrs_addr;
>>>> -};
>>>> +#include "sdhci-cadence.h"
>>>>
>>>>  struct sdhci_cdns_phy_cfg {
>>>>  	const char *property;
>>>> @@ -112,8 +63,11 @@ static int sdhci_cdns_write_phy_reg(struct
>>>> sdhci_cdns_plat *plat,  }
>>>>
>>>>  static int sdhci_cdns_phy_init(struct sdhci_cdns_plat *plat,
>>>> -				const void *fdt, int nodeoffset)
>>>> +			       const void *fdt, int nodeoffset)
>>>
>>> Is there any reason to touch this?
>> I thought the space key is misaligned. I think I better not to touch it.
>>>
>>>
>>>>  {
>>>> +	if (IS_ENABLED(CONFIG_MMC_SDHCI_CADENCE_V6))
>>>> +		return sdhci_cdns6_phy_init(plat,
>>> SDHCI_CDNS_HRS06_MODE_SD);
>>>> +
>>>>  	const fdt32_t *prop;
>>>>  	int ret, i;
>>>>
>>>> @@ -163,6 +117,9 @@ static void sdhci_cdns_set_control_reg(struct
>>> sdhci_host *host)
>>>>  	tmp &= ~SDHCI_CDNS_HRS06_MODE;
>>>>  	tmp |= FIELD_PREP(SDHCI_CDNS_HRS06_MODE, mode);
>>>>  	writel(tmp, plat->hrs_addr + SDHCI_CDNS_HRS06);
>>>> +
>>>> +	if (IS_ENABLED(CONFIG_MMC_SDHCI_CADENCE_V6))
>>>> +		sdhci_cdns6_phy_init(plat, mode);
>>>>  }
>>>>
>>>>  static const struct sdhci_ops sdhci_cdns_ops = { @@ -172,6 +129,9
>>>> @@ static const struct sdhci_ops sdhci_cdns_ops = {  static int
>>>> sdhci_cdns_set_tune_val(struct sdhci_cdns_plat *plat,
>>>>  				   unsigned int val)
>>>>  {
>>>> +	if (IS_ENABLED(CONFIG_MMC_SDHCI_CADENCE_V6))
>>>> +		return sdhci_cdns6_set_tune_val(plat, val);
>>>> +
>>>>  	void __iomem *reg = plat->hrs_addr + SDHCI_CDNS_HRS06;
>>>>  	u32 tmp;
>>>>  	int i, ret;
>>>> @@ -301,6 +261,7 @@ static int sdhci_cdns_probe(struct udevice *dev)
>>>> static const struct udevice_id sdhci_cdns_match[] = {
>>>>  	{ .compatible = "socionext,uniphier-sd4hc" },
>>>>  	{ .compatible = "cdns,sd4hc" },
>>>> +	{ .compatible = "cdns,sd6hc" },
>>>>  	{ /* sentinel */ }
>>>>  };
>>>>
>>>> diff --git a/drivers/mmc/sdhci-cadence.h
>>>> b/drivers/mmc/sdhci-cadence.h new file mode 100644 index
>>>> 0000000000..2c42cff2f3
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/sdhci-cadence.h
>>>> @@ -0,0 +1,68 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Copyright (C) 2022 Starfive.
>>>> + *   Author: Kuan Lim Lee <kuanlim.lee@starfivetech.com>
>>>
>>>
>>> Does it right Copyright and author? the below codes are taken from
>>> sdhci- cadence.c.
>> Some parts of #define are taken from sdchi-cadence.c, some parts of #define
>> are written by me. I think I still better put back the original Copyright and
>> author.
>>
>>>
>>>> + */
>>>> +
>>>> +#ifndef SDHCI_CADENCE_H_
>>>> +#define SDHCI_CADENCE_H_
>>>> +
>>>> +/* HRS - Host Register Set (specific to Cadence) */
>>>> +/* PHY access port */
>>>> +#define SDHCI_CDNS_HRS04		0x10
>>>> +/* Cadence V4 HRS04 Description*/
>>>> +#define   SDHCI_CDNS_HRS04_ACK			BIT(26)
>>>> +#define   SDHCI_CDNS_HRS04_RD			BIT(25)
>>>> +#define   SDHCI_CDNS_HRS04_WR			BIT(24)
>>>> +#define   SDHCI_CDNS_HRS04_RDATA		GENMASK(23, 16)
>>>> +#define   SDHCI_CDNS_HRS04_WDATA		GENMASK(15, 8)
>>>> +#define   SDHCI_CDNS_HRS04_ADDR			GENMASK(5,
>> 0)
>>>> +
>>>> +#define SDHCI_CDNS_HRS05		0x14
>>>> +
>>>> +/* eMMC control */
>>>> +#define SDHCI_CDNS_HRS06		0x18
>>>> +#define   SDHCI_CDNS_HRS06_TUNE_UP		BIT(15)
>>>> +#define   SDHCI_CDNS_HRS06_TUNE			GENMASK(13,
>> 8)
>>>> +#define   SDHCI_CDNS_HRS06_MODE			GENMASK(2,
>>> 0)
>>>> +#define   SDHCI_CDNS_HRS06_MODE_SD		0x0
>>>> +#define   SDHCI_CDNS_HRS06_MODE_MMC_SDR		0x2
>>>> +#define   SDHCI_CDNS_HRS06_MODE_MMC_DDR		0x3
>>>> +#define   SDHCI_CDNS_HRS06_MODE_MMC_HS200	0x4
>>>> +#define   SDHCI_CDNS_HRS06_MODE_MMC_HS400	0x5
>>>> +#define   SDHCI_CDNS_HRS06_MODE_MMC_HS400ES	0x6
>>>> +
>>>> +/* SRS - Slot Register Set (SDHCI-compatible) */
>>>> +#define SDHCI_CDNS_SRS_BASE		0x200
>>>> +
>>>> +/* Cadence V4 PHY Setting*/
>>>> +#define SDHCI_CDNS_PHY_DLY_SD_HS	0x00
>>>> +#define SDHCI_CDNS_PHY_DLY_SD_DEFAULT	0x01
>>>> +#define SDHCI_CDNS_PHY_DLY_UHS_SDR12	0x02
>>>> +#define SDHCI_CDNS_PHY_DLY_UHS_SDR25	0x03
>>>> +#define SDHCI_CDNS_PHY_DLY_UHS_SDR50	0x04
>>>> +#define SDHCI_CDNS_PHY_DLY_UHS_DDR50	0x05
>>>> +#define SDHCI_CDNS_PHY_DLY_EMMC_LEGACY	0x06
>>>> +#define SDHCI_CDNS_PHY_DLY_EMMC_SDR	0x07
>>>> +#define SDHCI_CDNS_PHY_DLY_EMMC_DDR	0x08
>>>> +#define SDHCI_CDNS_PHY_DLY_SDCLK	0x0b
>>>> +#define SDHCI_CDNS_PHY_DLY_HSMMC	0x0c
>>>> +#define SDHCI_CDNS_PHY_DLY_STROBE	0x0d
>>>> +
>>>> +/*
>>>> + * The tuned val register is 6 bit-wide, but not the whole of the
>>>> +range is
>>>> + * available.  The range 0-42 seems to be available (then 43 wraps
>>>> +around to 0)
>>>> + * but I am not quite sure if it is official.  Use only 0 to 39 for safety.
>>>> + */
>>>> +#define SDHCI_CDNS_MAX_TUNING_LOOP	40
>>>> +
>>>> +struct sdhci_cdns_plat {
>>>> +	struct mmc_config cfg;
>>>> +	struct mmc mmc;
>>>> +	void __iomem *hrs_addr;
>>>> +};
>>>> +
>>>> +int sdhci_cdns6_phy_init(struct sdhci_cdns_plat *plat, u32 mode);
>>>> +int sdhci_cdns6_set_tune_val(struct sdhci_cdns_plat *plat, unsigned
>>>> +int val);
>>>> +
>>>> +#endif
>>>> diff --git a/drivers/mmc/sdhci-cadence6-phy.c
>>>> b/drivers/mmc/sdhci-cadence6-phy.c
>>>> new file mode 100644
>>>> index 0000000000..dd3df27dc8
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/sdhci-cadence6-phy.c
>>>> @@ -0,0 +1,302 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-platform_driver
>>>> +/*
>>>> + * Copyright (C) 2022 Starfive.
>>>
>>> s/2022/2023?
>> I will put 2023.
>>
>>>
>>>> + *   Author: Kuan Lim Lee <kuanlim.lee@starfivetech.com>
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <dm.h>
>>>> +#include <asm/global_data.h>
>>>> +#include <dm/device_compat.h>
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/bug.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/sizes.h>
>>>> +#include <linux/libfdt.h>
>>>> +#include <mmc.h>
>>>> +#include <sdhci.h>
>>>> +#include "sdhci-cadence.h"
>>>> +
>>>> +/* IO Delay Information */
>>>> +#define SDHCI_CDNS_HRS07		0X1C
>>>> +#define   SDHCI_CDNS_HRS07_RW_COMPENSATE		GENMASK(20,
>>> 16)
>>>> +#define   SDHCI_CDNS_HRS07_IDELAY_VAL			GENMASK(4,
>>> 0)
>>>> +
>>>> +#define SDHCI_CDNS_HRS09		0x24		/* PHY Control
>> and
>>> Status */
>>>> +#define   SDHCI_CDNS_HRS09_RDDATA_EN		BIT(16)
>>>> +#define   SDHCI_CDNS_HRS09_RDCMD_EN		BIT(15)
>>>> +#define   SDHCI_CDNS_HRS09_EXTENDED_WR_MODE	BIT(3)
>>>> +#define   SDHCI_CDNS_HRS09_EXTENDED_RD_MODE	BIT(2)
>>>> +#define   SDHCI_CDNS_HRS09_PHY_INIT_COMPLETE	BIT(1)
>>>> +#define   SDHCI_CDNS_HRS09_PHY_SW_RESET		BIT(0)
>>>> +
>>>> +#define SDHCI_CDNS_HRS10		0x28		/* SDCLK
>> adjustment
>>> */
>>>> +#define   SDHCI_CDNS_HRS10_HCSDCLKADJ		GENMASK(19,
>> 16)
>>>> +
>>>> +#define SDHCI_CDNS_HRS16		0x40		/* CMD/DAT
>> output
>>> delay */
>>>> +
>>>> +/* PHY Special Function Registers */
>>>> +//#define DLL_PHY_REG_BASE		0x2000
>>>> +
>>>> +/* register to control the DQ related timing */
>>>> +#define PHY_DQ_TIMING_REG_ADDR		0x2000
>>>> +
>>>> +/* register to control the DQS related timing */
>>>> +#define PHY_DQS_TIMING_REG_ADDR		0x2004
>>>> +
>>>> +/* register to control the gate and loopback control related timing */
>>>> +#define PHY_GATE_LPBK_CTRL_REG_ADDR	0x2008
>>>> +
>>>> +/* register to control the Master DLL logic */
>>>> +#define PHY_DLL_MASTER_CTRL_REG_ADDR	0x200C
>>>> +
>>>> +/* register to control the Slave DLL logic */
>>>> +#define PHY_DLL_SLAVE_CTRL_REG_ADDR	0x2010
>>>> +#define PHY_DLL_SLAVE_CTRL_REG_READ_DQS_CMD_DELAY
>>> 	GENMASK(31, 24)
>>>> +#define PHY_DLL_SLAVE_CTRL_REG_READ_DQS_DELAY
>>> 	GENMASK(7, 0)
>>>> +
>>>> +/* register to control the global settings for PHY */
>>>> +#define PHY_CTRL_REG_ADDR		0x2080
>>>> +
>>>> +struct phy_reg {
>>>> +	u32 phy_dqs_timing;
>>>> +	u32 phy_gate_lpbk_ctrl;
>>>> +	//u32 phy_dll_master_ctrl;
>>>
>>> Remove unused code.
>> Noted.
>>>
>>>> +	u32 phy_dll_slave_ctrl;
>>>> +	//u32 phy_ctrl;
>>>
>>> ditto
>> Noted.
>>>
>>>> +	u32 phy_dq_timing;
>>>> +	//cp_sw_half_cycle_shift; ASIC
>>>
>>> Ditto
>> Noted.
>>>
>>>> +};
>>>> +
>>>> +struct controller_reg {
>>>> +	u32 hrs07;
>>>> +	u32 hrs09;
>>>> +	u32 hrs10;
>>>> +	u32 hrs16;
>>>> +};
>>>
>>>
>>> As some reg values can be re-used. I don't know what its value means.
>>> e.g) 0x00380004 is sued sd_ds/emmc_sdr/emmc_ddr/emmc_hs200.
>> This value is generated by script provided by Cadence.
>> e.g.) phy_dqs_timing_reg (0x2004) is a 32 bits register, inside the register has
>> few component: use_ext_lpbk_dqs, use_lpbk_dqs, use_phony_dqs,
>> use_phony_dqs_cmd, phony_dqs_sel, dqs_select_tsel_start and
>> dqs_select_tsel_end.
>>
>> Each components value is generated by script and I combined the value in a 32
>> bits value, 0x00380004.
>>
>>>
>>> I don't want to use a magic code. If you can add some macros, it's
>>> more readable.
>> Should I change the struct in way below, so that it is more reable?
>> static struct sdhci_cdns_phy_reg_pairs sd_ds_mode_setting[] = {
>> 		{0x00380000, PHY_DQS_TIMING_REG_ADDR},
>> 		{0x01A00040, PHY_GATE_LPBK_CTRL_REG_ADDR},
>> 		{0x00000000, PHY_DLL_SLAVE_CTRL_REG_ADDR},
>> 		{0x00000180, PHY_CTRL_REG_ADDR},
>> 		{0x00000001, PHY_DQ_TIMING_REG_ADDR},
>> };
>>
>> Or I have to put each value of components and put a convertion function in the
>> code to change this to 32 bits value and program inti register,
>> phy_dqs_timing_reg (0x2004)?
>>
>>>
>>>> +
>>>> +static struct phy_reg sd_ds_phy_cfg = {
>>>> +	0x00380004,
>>>> +	0x01A00040,
>>>> +	0x00000000,
>>>> +	0x00000001,
>>>> +};
>>>> +
>>>> +static struct phy_reg emmc_sdr_phy_cfg = {
>>>> +	0x00380004,
>>>> +	0x01A00040,
>>>> +	0x00000000,
>>>> +	0x00000001,
>>>> +};
>>>> +
>>>> +static struct phy_reg emmc_ddr_phy_cfg = {
>>>> +	0x00380004,
>>>> +	0x01A00040,
>>>> +	0x00000000,
>>>> +	0x10000001,
>>>> +};
>>>> +
>>>> +static struct phy_reg emmc_hs200_phy_cfg = {
>>>> +	0x00380004,
>>>> +	0x01A00040,
>>>> +	0x00DADA00,
>>>> +	0x00000001,
>>>> +};
>>>> +
>>>> +static struct phy_reg emmc_hs400_phy_cfg = {
>>>> +	0x00280004,
>>>> +	0x01A00040,
>>>> +	0x00DAD800,
>>>> +	0x00000001,
>>>> +};
>>>> +
>>>> +static struct controller_reg sd_ds_ctrl_cfg = {
>>>> +	0x00080000,
>>>> +	0x0001800C,
>>>> +	0x00020000,
>>>> +	0x00000000,
>>>> +};
>>>> +
>>>> +static struct controller_reg emmc_sdr_ctrl_cfg = {
>>>> +	0x00080000,
>>>> +	0x0001800C,
>>>> +	0x00030000,
>>>> +	0x00000000,
>>>> +};
>>>> +
>>>> +static struct controller_reg emmc_ddr_ctrl_cfg = {
>>>> +	0x00090001,
>>>> +	0x0001800C,
>>>> +	0x00020000,
>>>> +	0x11000001,
>>>> +};
>>>> +
>>>> +static struct controller_reg emmc_hs200_ctrl_cfg = {
>>>> +	0x00090000,
>>>> +	0x00018000,
>>>> +	0x00080000,
>>>> +	0x00000000,
>>>> +};
>>>> +
>>>> +static struct controller_reg emmc_hs400_ctrl_cfg = {
>>>> +	0x00080000,
>>>> +	0x00018000,
>>>> +	0x00080000,
>>>> +	0x11000000,
>>>> +};
>>>> +
>>>> +static unsigned int sdhci_cdns6_read_phy_reg(struct sdhci_cdns_plat *plat,
>>>> +					     u32 addr)
>>>> +{
>>>> +	u32 data = 0;
>>>> +
>>>> +	writel(addr, plat->hrs_addr + SDHCI_CDNS_HRS04);
>>>> +	data = readl(plat->hrs_addr + SDHCI_CDNS_HRS05);
>>>> +	return data;
>>>> +}
>>>> +
>>>> +static void sdhci_cdns6_write_phy_reg(struct sdhci_cdns_plat *plat,
>>>> +				      u32 addr, u32 data)
>>>> +{
>>>> +	u32 readdat = 0;
>>>> +
>>>> +	writel(addr, plat->hrs_addr + SDHCI_CDNS_HRS04);
>>>> +	writel(data, plat->hrs_addr + SDHCI_CDNS_HRS05);
>>>> +	readdat = readl(plat->hrs_addr + SDHCI_CDNS_HRS05);
>>>> +
>>>> +	if (readdat != data) {
>>>> +		pr_err("Error, %s: Writing failed!: address: 0x%x, value : 0x%x,
>>>> +		       readval: 0x%x\n", __func__, addr, data, readdat);
>>>
>>>
>>> Doesn't need to return error value?
>> Yes, I think it will be changed  to return error value.
>>>
>>>
>>>> +	}
>>>> +}
>>>> +
>>>> +static int sdhci_cdns6_reset_phy_dll(struct sdhci_cdns_plat *plat,
>>>> +				     unsigned int reset)
>>>> +{
>>>> +	void __iomem *reg = plat->hrs_addr + SDHCI_CDNS_HRS09;
>>>> +	u32 tmp;
>>>> +	int ret;
>>>> +
>>>> +	tmp = readl(reg);
>>>> +	tmp &= ~SDHCI_CDNS_HRS09_PHY_SW_RESET;
>>>> +
>>>> +	if (reset)	/* Switch On DLL Reset */
>>>
>>>
>>> 	/* Swithc On DLL Reset */
>> Noted, thanks for telling.
>>
>>> 	if (reset)
>>> 		...
>>> 	else
>>> 		...
>>>
>>>
>>>> +		tmp |= FIELD_PREP(SDHCI_CDNS_HRS09_PHY_SW_RESET, 0);
>>>> +	else		/* Switch Off DLL Reset */
>>>> +		tmp |= FIELD_PREP(SDHCI_CDNS_HRS09_PHY_SW_RESET, 1);
>>>> +
>>>> +	writel(tmp, reg);
>>>> +
>>>> +	if (!reset) {
>>>> +		ret = readl_poll_timeout(reg, tmp,
>>>> +					 (tmp &
>>> SDHCI_CDNS_HRS09_PHY_INIT_COMPLETE),
>>>> +					 3000);
>>>
>>> Add a comment why it needs to poll during 3000.
>> Actually 3000 us is a safe range. User manual doesn’t specify, I put myself.
>> Comment will be like below:
>> /* After reset, wait for PHY_INIT_COMPLETE in HRS09 register until it is set to 1
>> within 3000us*/
>>
>>>
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +int sdhci_cdns6_phy_init(struct sdhci_cdns_plat *plat, u32 mode) {
>>>> +	struct phy_reg *phy_cfgs;
>>>> +	struct controller_reg *ctrl_cfgs;
>>>> +	void __iomem *reg = NULL;
>>>> +	u32 tmp;
>>>> +	int ret;
>>>> +
>>>> +	switch (mode) {
>>>> +	case SDHCI_CDNS_HRS06_MODE_SD:
>>>> +		phy_cfgs = &sd_ds_phy_cfg;
>>>> +		ctrl_cfgs = &sd_ds_ctrl_cfg;
>>>> +		break;
>>>> +
>>>> +	case SDHCI_CDNS_HRS06_MODE_MMC_SDR:
>>>> +		phy_cfgs = &emmc_sdr_phy_cfg;
>>>> +		ctrl_cfgs = &emmc_sdr_ctrl_cfg;
>>>> +		break;
>>>> +
>>>> +	case SDHCI_CDNS_HRS06_MODE_MMC_DDR:
>>>> +		phy_cfgs = &emmc_ddr_phy_cfg;
>>>> +		ctrl_cfgs = &emmc_ddr_ctrl_cfg;
>>>> +		break;
>>>> +
>>>> +	case SDHCI_CDNS_HRS06_MODE_MMC_HS200:
>>>> +		phy_cfgs = &emmc_hs200_phy_cfg;
>>>> +		ctrl_cfgs = &emmc_hs200_ctrl_cfg;
>>>> +		break;
>>>> +
>>>> +	case SDHCI_CDNS_HRS06_MODE_MMC_HS400:
>>>> +		phy_cfgs = &emmc_hs400_phy_cfg;
>>>> +		ctrl_cfgs = &emmc_hs400_ctrl_cfg;
>>>> +		break;
>>>
>>>
>>> If there is no matched mod, phy_cfgs and ctrl_cfgs should be NULL.
>> Noted.
>>
>>>
>>>> +	}
>>>> +
>>>> +	/* Switch On the DLL Reset */
>>>> +	sdhci_cdns6_reset_phy_dll(plat, 1);
>>>> +
>>>> +	sdhci_cdns6_write_phy_reg(plat, PHY_DQS_TIMING_REG_ADDR,
>>>> +				  phy_cfgs->phy_dqs_timing);
>>>> +	sdhci_cdns6_write_phy_reg(plat, PHY_GATE_LPBK_CTRL_REG_ADDR,
>>>> +				  phy_cfgs->phy_gate_lpbk_ctrl);
>>>> +	sdhci_cdns6_write_phy_reg(plat, PHY_DLL_SLAVE_CTRL_REG_ADDR,
>>>> +				  phy_cfgs->phy_dll_slave_ctrl);
>>>> +
>>>> +	/* Switch Off the DLL Reset */
>>>> +	ret = sdhci_cdns6_reset_phy_dll(plat, 0);
>>>> +	if (ret) {
>>>> +		printf("sdhci_cdns6_reset_phy is not completed\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	/* Set PHY DQ TIMING control register */
>>>> +	sdhci_cdns6_write_phy_reg(plat, PHY_DQ_TIMING_REG_ADDR,
>>>> +				  phy_cfgs->phy_dq_timing);
>>>> +
>>>> +	/* Set HRS09 register */
>>>> +	reg = plat->hrs_addr + SDHCI_CDNS_HRS09;
>>>> +	tmp = readl(reg);
>>>
>>>
>>> I'm not suer why plat-hrs_addr assigned to reg.
>>> How about using the below?
>>>
>>> hrs_base_addr = plat->hrs_addr;
>>>
>>> readl(hrs_base_addr + SDHCI_CDNS_HRS10);
>>>
>>> Then you can change the below code like
>>>
>>> readl(hrs_base_addr + SDHCI_CDNS_HRS16); readl(hrs_base_addr +
>>> SDHCI_CDNS_HRS07);
>>>
>>> Otherwise, readl(plat->hrs_addr + SDHCI_xxx);
>> Noted, this looked simpler.
>>
>>>
>>>
>>>> +	tmp &= ~(SDHCI_CDNS_HRS09_EXTENDED_WR_MODE |
>>>> +		 SDHCI_CDNS_HRS09_EXTENDED_RD_MODE |
>>>> +		 SDHCI_CDNS_HRS09_RDDATA_EN |
>>>> +		 SDHCI_CDNS_HRS09_RDCMD_EN);
>>>> +	tmp |= ctrl_cfgs->hrs09;
>>>> +	writel(tmp, reg);
>>>> +
>>>> +	/* Set HRS10 register */
>>>> +	reg = plat->hrs_addr + SDHCI_CDNS_HRS10;
>>>> +	tmp = readl(reg);
>>>> +	tmp &= ~SDHCI_CDNS_HRS10_HCSDCLKADJ;
>>>> +	tmp |= ctrl_cfgs->hrs10;
>>>> +	writel(tmp, reg);
>>>> +
>>>> +	/* Set HRS16 register */
>>>> +	reg = plat->hrs_addr + SDHCI_CDNS_HRS16;
>>>> +	tmp = ctrl_cfgs->hrs16;
>>>> +	writel(tmp, reg);
>>>> +
>>>> +	/* Set HRS07 register */
>>>> +	reg = plat->hrs_addr + SDHCI_CDNS_HRS07;
>>>> +	tmp = ctrl_cfgs->hrs07;
>>>> +	writel(tmp, reg);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int sdhci_cdns6_set_tune_val(struct sdhci_cdns_plat *plat,
>>>> +			     unsigned int val)
>>>> +{
>>>> +	u32 tmp, tuneval;
>>>
>>> Ditto.
>> Any problem with this? I think current way looked simple.

Ah, My opinion is the using a val or other meaningful name instead of tmp.

Best Regards,
Jaehoon Chung

>>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>> +
>>>> +	tuneval = (val * 256) / SDHCI_CDNS_MAX_TUNING_LOOP;
>>>> +
>>>> +	tmp = sdhci_cdns6_read_phy_reg(plat,
>>> PHY_DLL_SLAVE_CTRL_REG_ADDR);
>>>> +	tmp &= ~(PHY_DLL_SLAVE_CTRL_REG_READ_DQS_CMD_DELAY |
>>>> +		 PHY_DLL_SLAVE_CTRL_REG_READ_DQS_DELAY);
>>>> +	tmp |=
>>> FIELD_PREP(PHY_DLL_SLAVE_CTRL_REG_READ_DQS_CMD_DELAY, tuneval) |
>>>> +		FIELD_PREP(PHY_DLL_SLAVE_CTRL_REG_READ_DQS_DELAY,
>>> tuneval);
>>>> +	sdhci_cdns6_write_phy_reg(plat, PHY_DLL_SLAVE_CTRL_REG_ADDR,
>>> tmp);
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>