[PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Shah, Nehal-bakulchandra]

From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>

This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
HS400 and HS200 mode requires hardware work around also. This patch
adds the quirks for the same.

Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/mmc/core/mmc.c        | 36 +++++++++++++++++++-----------------
 drivers/mmc/host/sdhci-acpi.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c      | 22 ++++++++++++++++++++++
 drivers/mmc/host/sdhci.h      |  4 +++-
 include/linux/mmc/host.h      |  1 +
 5 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 4ffea14..29c46d7 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1161,14 +1161,14 @@ static int mmc_select_hs400(struct mmc_card *card)
 			mmc_hostname(host), err);
 		return err;
 	}
-
-	/* Set host controller to HS timing */
-	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-
-	/* Reduce frequency to HS frequency */
-	max_dtr = card->ext_csd.hs_max_dtr;
-	mmc_set_clock(host, max_dtr);
-
+	/*In AMD Platform due to hardware ip issue this fails*/
+	if (!host->ops->set_hs400_dll) {
+		/* Set host controller to HS timing */
+		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
+		/* Reduce frequency to HS frequency */
+		max_dtr = card->ext_csd.hs_max_dtr;
+		mmc_set_clock(host, max_dtr);
+	}
 	err = mmc_switch_status(card);
 	if (err)
 		goto out_err;
@@ -1204,7 +1204,8 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = mmc_switch_status(card);
 	if (err)
 		goto out_err;
-
+	if (host->ops->set_hs400_dll)
+		host->ops->set_hs400_dll(host);
 	return 0;
 
 out_err:
@@ -1227,8 +1228,8 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 
 	/* Reduce frequency to HS */
 	max_dtr = card->ext_csd.hs_max_dtr;
-	mmc_set_clock(host, max_dtr);
-
+	if (!host->ops->set_hs400_dll)
+		mmc_set_clock(host, max_dtr);
 	/* Switch HS400 to HS DDR */
 	val = EXT_CSD_TIMING_HS;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
@@ -1236,12 +1237,13 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 			   true, false, true);
 	if (err)
 		goto out_err;
-
-	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
-
-	err = mmc_switch_status(card);
-	if (err)
-		goto out_err;
+       /*In AMD Platform due to hardware ip issue this fails*/
+	if (!host->ops->set_hs400_dll) {
+		mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
+		err = mmc_switch_status(card);
+		if (err)
+			goto out_err;
+	}
 
 	/* Switch HS DDR to HS */
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index cf66a3d..7702459 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -103,6 +103,16 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
 	usleep_range(300, 1000);
 }
 
+static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
+{
+	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+	if (host->mmc->caps2 == MMC_CAP2_HS400_1_8V) {
+		sdhci_writel(host, 0x40003210, 0x908);
+		udelay(10);
+		sdhci_writel(host, 0x40033210, 0x908);
+	}
+}
+
 static const struct sdhci_ops sdhci_acpi_ops_dflt = {
 	.set_clock = sdhci_set_clock,
 	.set_bus_width = sdhci_set_bus_width,
@@ -122,6 +132,17 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
 	.ops = &sdhci_acpi_ops_int,
 };
 
+static const struct sdhci_ops sdhci_acpi_ops_amd = {
+	.set_clock = sdhci_set_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.set_hs400_dll = sdhci_acpi_amd_hs400_dll,
+};
+
+static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
+	.ops = &sdhci_acpi_ops_amd,
+};
 #ifdef CONFIG_X86
 
 static bool sdhci_acpi_byt(void)
@@ -282,6 +303,18 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
 	.probe_slot	= sdhci_acpi_emmc_probe_slot,
 };
 
+static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
+	.chip    = &sdhci_acpi_chip_amd,
+	.caps    = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |
+		   MMC_CAP_HW_RESET,
+	.quirks  = SDHCI_QUIRK_32BIT_DMA_ADDR |
+		   SDHCI_QUIRK_32BIT_DMA_SIZE |
+		   SDHCI_QUIRK_32BIT_ADMA_SIZE,
+	.quirks2 = SDHCI_QUIRK2_BROKEN_TUNING_WA,
+	.probe_slot     = sdhci_acpi_emmc_probe_slot,
+
+};
+
 static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
 	.quirks  = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		   SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
@@ -335,6 +368,8 @@ struct sdhci_acpi_uid_slot {
 	{ "INT344D"  , NULL, &sdhci_acpi_slot_int_sdio },
 	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
 	{ "PNP0D40"  },
+	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc  },
+	{ "AMDI0040"},
 	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
 	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
 	{ },
@@ -353,6 +388,7 @@ struct sdhci_acpi_uid_slot {
 	{ "PNP0D40"  },
 	{ "QCOM8051" },
 	{ "QCOM8052" },
+	{ "AMDI0040" },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d43..6e0e73d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1170,6 +1170,13 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		flags |= SDHCI_CMD_DATA;
 
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
+
+	if (cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200  &&
+	    (host->quirks2 & SDHCI_QUIRK2_BROKEN_TUNING_WA)) {
+		mdelay(10);
+		sdhci_writel(host, 0x8803040a, 0x8b8);
+		mdelay(10);
+	}
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);
 
@@ -1818,6 +1825,14 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
 		host->ops->hw_reset(host);
 }
 
+static void sdhci_set_hs400_dll(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+
+	if (host->ops && host->ops->set_hs400_dll)
+		host->ops->set_hs400_dll(host);
+}
+
 static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
 {
 	if (!(host->flags & SDHCI_DEVICE_DEAD)) {
@@ -2295,6 +2310,7 @@ static void sdhci_card_event(struct mmc_host *mmc)
 	.get_cd		= sdhci_get_cd,
 	.get_ro		= sdhci_get_ro,
 	.hw_reset	= sdhci_hw_reset,
+	.set_hs400_dll = sdhci_set_hs400_dll,
 	.enable_sdio_irq = sdhci_enable_sdio_irq,
 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
 	.prepare_hs400_tuning		= sdhci_prepare_hs400_tuning,
@@ -3202,6 +3218,12 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
 		host->caps1 &= ~upper_32_bits(dt_caps_mask);
 		host->caps1 |= upper_32_bits(dt_caps);
 	}
+
+	if ((host->caps1 & SDHCI_SUPPORT_SDR104) &&
+	    (host->caps1 & SDHCI_SUPPORT_DDR50) &&
+	    (host->quirks2 & SDHCI_QUIRK2_BROKEN_TUNING_WA)) {
+		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
+	}
 }
 EXPORT_SYMBOL_GPL(__sdhci_read_caps);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0469fa1..d3ec57c 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -435,7 +435,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
 /* Broken Clock divider zero in controller */
 #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
-
+/* Tuning work around */
+#define SDHCI_QUIRK2_BROKEN_TUNING_WA		        (1<<16)
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
 
@@ -576,6 +577,7 @@ struct sdhci_ops {
 	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
 	void	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
 	void	(*hw_reset)(struct sdhci_host *host);
+	void    (*set_hs400_dll)(struct sdhci_host *host);
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void    (*card_event)(struct sdhci_host *host);
 	void	(*voltage_switch)(struct sdhci_host *host);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ebd1ceb..1b00c28 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -152,6 +152,7 @@ struct mmc_host_ops {
 					 unsigned int max_dtr, int host_drv,
 					 int card_drv, int *drv_type);
 	void	(*hw_reset)(struct mmc_host *host);
+	void	(*set_hs400_dll)(struct mmc_host *host);
 	void	(*card_event)(struct mmc_host *host);
 
 	/*
-- 
1.9.1

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Adrian Hunter]

On 28/06/17 13:53, Shah, Nehal-bakulchandra wrote:
> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> 
> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
> HS400 and HS200 mode requires hardware work around also. This patch
> adds the quirks for the same.

I have some comments and we need Ulf's feedback for the core changes.

> 
> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/mmc/core/mmc.c        | 36 +++++++++++++++++++-----------------
>  drivers/mmc/host/sdhci-acpi.c | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c      | 22 ++++++++++++++++++++++
>  drivers/mmc/host/sdhci.h      |  4 +++-
>  include/linux/mmc/host.h      |  1 +

Please split separate the core changes (mmc.c and host.h) from the driver
changes by moving them to a separate patch.

>  5 files changed, 81 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 4ffea14..29c46d7 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1161,14 +1161,14 @@ static int mmc_select_hs400(struct mmc_card *card)
>  			mmc_hostname(host), err);
>  		return err;
>  	}
> -
> -	/* Set host controller to HS timing */
> -	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
> -
> -	/* Reduce frequency to HS frequency */
> -	max_dtr = card->ext_csd.hs_max_dtr;
> -	mmc_set_clock(host, max_dtr);
> -
> +	/*In AMD Platform due to hardware ip issue this fails*/
> +	if (!host->ops->set_hs400_dll) {
> +		/* Set host controller to HS timing */
> +		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
> +		/* Reduce frequency to HS frequency */
> +		max_dtr = card->ext_csd.hs_max_dtr;
> +		mmc_set_clock(host, max_dtr);
> +	}

Really need to re-consider the host API here.  One possibility is to add
another member to struct mmc_ios that indicates the transition being made.
e.g. HS200_TO_HS400_TO_HS in this case i.e. going to HS while on the way
from HS200 to HS400.  Then the driver can presumably do the right thing (in
this case nothing) in its ->set_ios() callback.  In any case it is up to Ulf
to comment on this.

>  	err = mmc_switch_status(card);
>  	if (err)
>  		goto out_err;
> @@ -1204,7 +1204,8 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	err = mmc_switch_status(card);
>  	if (err)
>  		goto out_err;
> -
> +	if (host->ops->set_hs400_dll)
> +		host->ops->set_hs400_dll(host);

Why is this after checking switch status instead of before?

>  	return 0;
>  
>  out_err:
> @@ -1227,8 +1228,8 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>  
>  	/* Reduce frequency to HS */
>  	max_dtr = card->ext_csd.hs_max_dtr;
> -	mmc_set_clock(host, max_dtr);
> -
> +	if (!host->ops->set_hs400_dll)
> +		mmc_set_clock(host, max_dtr);
>  	/* Switch HS400 to HS DDR */
>  	val = EXT_CSD_TIMING_HS;
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
> @@ -1236,12 +1237,13 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>  			   true, false, true);
>  	if (err)
>  		goto out_err;
> -
> -	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
> -
> -	err = mmc_switch_status(card);
> -	if (err)
> -		goto out_err;
> +       /*In AMD Platform due to hardware ip issue this fails*/
> +	if (!host->ops->set_hs400_dll) {
> +		mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
> +		err = mmc_switch_status(card);
> +		if (err)
> +			goto out_err;
> +	}
>  
>  	/* Switch HS DDR to HS */
>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index cf66a3d..7702459 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -103,6 +103,16 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>  	usleep_range(300, 1000);
>  }
>  
> +static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
> +{
> +	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);

Why are you re-reading caps1 here?

> +	if (host->mmc->caps2 == MMC_CAP2_HS400_1_8V) {

It would be better to avoid setting the callback function when it isn't needed.

> +		sdhci_writel(host, 0x40003210, 0x908);
> +		udelay(10);
> +		sdhci_writel(host, 0x40033210, 0x908);
> +	}
> +}
> +
>  static const struct sdhci_ops sdhci_acpi_ops_dflt = {
>  	.set_clock = sdhci_set_clock,
>  	.set_bus_width = sdhci_set_bus_width,
> @@ -122,6 +132,17 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>  	.ops = &sdhci_acpi_ops_int,
>  };
>  
> +static const struct sdhci_ops sdhci_acpi_ops_amd = {
> +	.set_clock = sdhci_set_clock,
> +	.set_bus_width = sdhci_set_bus_width,
> +	.reset = sdhci_reset,
> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
> +	.set_hs400_dll = sdhci_acpi_amd_hs400_dll,
> +};
> +
> +static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
> +	.ops = &sdhci_acpi_ops_amd,
> +};
>  #ifdef CONFIG_X86
>  
>  static bool sdhci_acpi_byt(void)
> @@ -282,6 +303,18 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
>  	.probe_slot	= sdhci_acpi_emmc_probe_slot,
>  };
>  
> +static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
> +	.chip    = &sdhci_acpi_chip_amd,
> +	.caps    = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |
> +		   MMC_CAP_HW_RESET,
> +	.quirks  = SDHCI_QUIRK_32BIT_DMA_ADDR |
> +		   SDHCI_QUIRK_32BIT_DMA_SIZE |
> +		   SDHCI_QUIRK_32BIT_ADMA_SIZE,
> +	.quirks2 = SDHCI_QUIRK2_BROKEN_TUNING_WA,
> +	.probe_slot     = sdhci_acpi_emmc_probe_slot,

You should probably implement your own ->probe_slot() function instead of
sdhci_acpi_emmc_probe_slot() and then put your caps setting there.

> +

Unnecessary blank line.

> +};
> +
>  static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>  	.quirks  = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
>  		   SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> @@ -335,6 +368,8 @@ struct sdhci_acpi_uid_slot {
>  	{ "INT344D"  , NULL, &sdhci_acpi_slot_int_sdio },
>  	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
>  	{ "PNP0D40"  },
> +	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc  },
> +	{ "AMDI0040"},

The second line here will never match anything, please remove it.

>  	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
>  	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
>  	{ },
> @@ -353,6 +388,7 @@ struct sdhci_acpi_uid_slot {
>  	{ "PNP0D40"  },
>  	{ "QCOM8051" },
>  	{ "QCOM8052" },
> +	{ "AMDI0040" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d43..6e0e73d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1170,6 +1170,13 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  		flags |= SDHCI_CMD_DATA;
>  
>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
> +
> +	if (cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200  &&
> +	    (host->quirks2 & SDHCI_QUIRK2_BROKEN_TUNING_WA)) {
> +		mdelay(10);
> +		sdhci_writel(host, 0x8803040a, 0x8b8);
> +		mdelay(10);
> +	}

That could be better done by making use of CONFIG_MMC_SDHCI_IO_ACCESSORS and
providing an implementation for host->ops->write_w() that does that check
for the SDHCI_COMMAND register.

>  }
>  EXPORT_SYMBOL_GPL(sdhci_send_command);
>  
> @@ -1818,6 +1825,14 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
>  		host->ops->hw_reset(host);
>  }
>  
> +static void sdhci_set_hs400_dll(struct mmc_host *mmc)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +
> +	if (host->ops && host->ops->set_hs400_dll)
> +		host->ops->set_hs400_dll(host);
> +}

This isn't necessary.  The driver can override mmc host operations directly.
i.e.
	host->mmc_host_ops.set_hs400_dll = sdhci_acpi_amd_hs400_dll;

Although if my suggestion about struct mmc_ios is taken up, then you will
need to override ->set_ios as well or instead.

> +
>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>  {
>  	if (!(host->flags & SDHCI_DEVICE_DEAD)) {
> @@ -2295,6 +2310,7 @@ static void sdhci_card_event(struct mmc_host *mmc)
>  	.get_cd		= sdhci_get_cd,
>  	.get_ro		= sdhci_get_ro,
>  	.hw_reset	= sdhci_hw_reset,
> +	.set_hs400_dll = sdhci_set_hs400_dll,

As above, this isn't necessary.

>  	.enable_sdio_irq = sdhci_enable_sdio_irq,
>  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
>  	.prepare_hs400_tuning		= sdhci_prepare_hs400_tuning,
> @@ -3202,6 +3218,12 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
>  		host->caps1 &= ~upper_32_bits(dt_caps_mask);
>  		host->caps1 |= upper_32_bits(dt_caps);
>  	}
> +
> +	if ((host->caps1 & SDHCI_SUPPORT_SDR104) &&
> +	    (host->caps1 & SDHCI_SUPPORT_DDR50) &&
> +	    (host->quirks2 & SDHCI_QUIRK2_BROKEN_TUNING_WA)) {
> +		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
> +	}

This doesn't belong here.  See further above about ->probe_slot().

>  }
>  EXPORT_SYMBOL_GPL(__sdhci_read_caps);
>  
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0469fa1..d3ec57c 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -435,7 +435,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
>  /* Broken Clock divider zero in controller */
>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
> -
> +/* Tuning work around */
> +#define SDHCI_QUIRK2_BROKEN_TUNING_WA		        (1<<16)
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
>  
> @@ -576,6 +577,7 @@ struct sdhci_ops {
>  	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>  	void	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
>  	void	(*hw_reset)(struct sdhci_host *host);
> +	void    (*set_hs400_dll)(struct sdhci_host *host);

As further above, this isn't necessary.

>  	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
>  	void    (*card_event)(struct sdhci_host *host);
>  	void	(*voltage_switch)(struct sdhci_host *host);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index ebd1ceb..1b00c28 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -152,6 +152,7 @@ struct mmc_host_ops {
>  					 unsigned int max_dtr, int host_drv,
>  					 int card_drv, int *drv_type);
>  	void	(*hw_reset)(struct mmc_host *host);
> +	void	(*set_hs400_dll)(struct mmc_host *host);
>  	void	(*card_event)(struct mmc_host *host);
>  
>  	/*
> 

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Shah, Nehal-bakulchandra]

On 7/25/2017 3:28 PM, Adrian Hunter wrote:
> On 28/06/17 13:53, Shah, Nehal-bakulchandra wrote:
>> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>
>> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
>> HS400 and HS200 mode requires hardware work around also. This patch
>> adds the quirks for the same.
> 
> I have some comments and we need Ulf's feedback for the core changes.
> 
>>
>> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/mmc/core/mmc.c        | 36 +++++++++++++++++++-----------------
>>  drivers/mmc/host/sdhci-acpi.c | 36 ++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/host/sdhci.c      | 22 ++++++++++++++++++++++
>>  drivers/mmc/host/sdhci.h      |  4 +++-
>>  include/linux/mmc/host.h      |  1 +
> 
> Please split separate the core changes (mmc.c and host.h) from the driver
> changes by moving them to a separate patch.
> 
>>  5 files changed, 81 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 4ffea14..29c46d7 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1161,14 +1161,14 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  			mmc_hostname(host), err);
>>  		return err;
>>  	}
>> -
>> -	/* Set host controller to HS timing */
>> -	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> -
>> -	/* Reduce frequency to HS frequency */
>> -	max_dtr = card->ext_csd.hs_max_dtr;
>> -	mmc_set_clock(host, max_dtr);
>> -
>> +	/*In AMD Platform due to hardware ip issue this fails*/
>> +	if (!host->ops->set_hs400_dll) {
>> +		/* Set host controller to HS timing */
>> +		mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> +		/* Reduce frequency to HS frequency */
>> +		max_dtr = card->ext_csd.hs_max_dtr;
>> +		mmc_set_clock(host, max_dtr);
>> +	}
> 
> Really need to re-consider the host API here.  One possibility is to add
> another member to struct mmc_ios that indicates the transition being made.
> e.g. HS200_TO_HS400_TO_HS in this case i.e. going to HS while on the way
> from HS200 to HS400.  Then the driver can presumably do the right thing (in
> this case nothing) in its ->set_ios() callback.  In any case it is up to Ulf
> to comment on this.
> 
>>  	err = mmc_switch_status(card);
>>  	if (err)
>>  		goto out_err;
>> @@ -1204,7 +1204,8 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	err = mmc_switch_status(card);
>>  	if (err)
>>  		goto out_err;
>> -
>> +	if (host->ops->set_hs400_dll)
>> +		host->ops->set_hs400_dll(host);
> 
> Why is this after checking switch status instead of before?
> 
>>  	return 0;
>>  
>>  out_err:
>> @@ -1227,8 +1228,8 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>>  
>>  	/* Reduce frequency to HS */
>>  	max_dtr = card->ext_csd.hs_max_dtr;
>> -	mmc_set_clock(host, max_dtr);
>> -
>> +	if (!host->ops->set_hs400_dll)
>> +		mmc_set_clock(host, max_dtr);
>>  	/* Switch HS400 to HS DDR */
>>  	val = EXT_CSD_TIMING_HS;
>>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
>> @@ -1236,12 +1237,13 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
>>  			   true, false, true);
>>  	if (err)
>>  		goto out_err;
>> -
>> -	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
>> -
>> -	err = mmc_switch_status(card);
>> -	if (err)
>> -		goto out_err;
>> +       /*In AMD Platform due to hardware ip issue this fails*/
>> +	if (!host->ops->set_hs400_dll) {
>> +		mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
>> +		err = mmc_switch_status(card);
>> +		if (err)
>> +			goto out_err;
>> +	}
>>  
>>  	/* Switch HS DDR to HS */
>>  	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index cf66a3d..7702459 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -103,6 +103,16 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>>  	usleep_range(300, 1000);
>>  }
>>  
>> +static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
>> +{
>> +	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
> 
> Why are you re-reading caps1 here?
> 
>> +	if (host->mmc->caps2 == MMC_CAP2_HS400_1_8V) {
> 
> It would be better to avoid setting the callback function when it isn't needed.
> 
>> +		sdhci_writel(host, 0x40003210, 0x908);
>> +		udelay(10);
>> +		sdhci_writel(host, 0x40033210, 0x908);
>> +	}
>> +}
>> +
>>  static const struct sdhci_ops sdhci_acpi_ops_dflt = {
>>  	.set_clock = sdhci_set_clock,
>>  	.set_bus_width = sdhci_set_bus_width,
>> @@ -122,6 +132,17 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>>  	.ops = &sdhci_acpi_ops_int,
>>  };
>>  
>> +static const struct sdhci_ops sdhci_acpi_ops_amd = {
>> +	.set_clock = sdhci_set_clock,
>> +	.set_bus_width = sdhci_set_bus_width,
>> +	.reset = sdhci_reset,
>> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
>> +	.set_hs400_dll = sdhci_acpi_amd_hs400_dll,
>> +};
>> +
>> +static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
>> +	.ops = &sdhci_acpi_ops_amd,
>> +};
>>  #ifdef CONFIG_X86
>>  
>>  static bool sdhci_acpi_byt(void)
>> @@ -282,6 +303,18 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
>>  	.probe_slot	= sdhci_acpi_emmc_probe_slot,
>>  };
>>  
>> +static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
>> +	.chip    = &sdhci_acpi_chip_amd,
>> +	.caps    = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |
>> +		   MMC_CAP_HW_RESET,
>> +	.quirks  = SDHCI_QUIRK_32BIT_DMA_ADDR |
>> +		   SDHCI_QUIRK_32BIT_DMA_SIZE |
>> +		   SDHCI_QUIRK_32BIT_ADMA_SIZE,
>> +	.quirks2 = SDHCI_QUIRK2_BROKEN_TUNING_WA,
>> +	.probe_slot     = sdhci_acpi_emmc_probe_slot,
> 
> You should probably implement your own ->probe_slot() function instead of
> sdhci_acpi_emmc_probe_slot() and then put your caps setting there.
> 
>> +
> 
> Unnecessary blank line.
> 
>> +};
>> +
>>  static const struct sdhci_acpi_slot sdhci_acpi_slot_int_sdio = {
>>  	.quirks  = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
>>  		   SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
>> @@ -335,6 +368,8 @@ struct sdhci_acpi_uid_slot {
>>  	{ "INT344D"  , NULL, &sdhci_acpi_slot_int_sdio },
>>  	{ "PNP0FFF"  , "3" , &sdhci_acpi_slot_int_sd   },
>>  	{ "PNP0D40"  },
>> +	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc  },
>> +	{ "AMDI0040"},
> 
> The second line here will never match anything, please remove it.
> 
>>  	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
>>  	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
>>  	{ },
>> @@ -353,6 +388,7 @@ struct sdhci_acpi_uid_slot {
>>  	{ "PNP0D40"  },
>>  	{ "QCOM8051" },
>>  	{ "QCOM8052" },
>> +	{ "AMDI0040" },
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ecd0d43..6e0e73d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1170,6 +1170,13 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>  		flags |= SDHCI_CMD_DATA;
>>  
>>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>> +
>> +	if (cmd->opcode == MMC_SEND_TUNING_BLOCK_HS200  &&
>> +	    (host->quirks2 & SDHCI_QUIRK2_BROKEN_TUNING_WA)) {
>> +		mdelay(10);
>> +		sdhci_writel(host, 0x8803040a, 0x8b8);
>> +		mdelay(10);
>> +	}
> 
> That could be better done by making use of CONFIG_MMC_SDHCI_IO_ACCESSORS and
> providing an implementation for host->ops->write_w() that does that check
> for the SDHCI_COMMAND register.
> 
>>  }
>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
>>  
>> @@ -1818,6 +1825,14 @@ static void sdhci_hw_reset(struct mmc_host *mmc)
>>  		host->ops->hw_reset(host);
>>  }
>>  
>> +static void sdhci_set_hs400_dll(struct mmc_host *mmc)
>> +{
>> +	struct sdhci_host *host = mmc_priv(mmc);
>> +
>> +	if (host->ops && host->ops->set_hs400_dll)
>> +		host->ops->set_hs400_dll(host);
>> +}
> 
> This isn't necessary.  The driver can override mmc host operations directly.
> i.e.
> 	host->mmc_host_ops.set_hs400_dll = sdhci_acpi_amd_hs400_dll;
> 
> Although if my suggestion about struct mmc_ios is taken up, then you will
> need to override ->set_ios as well or instead.
> 
>> +
>>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
>>  {
>>  	if (!(host->flags & SDHCI_DEVICE_DEAD)) {
>> @@ -2295,6 +2310,7 @@ static void sdhci_card_event(struct mmc_host *mmc)
>>  	.get_cd		= sdhci_get_cd,
>>  	.get_ro		= sdhci_get_ro,
>>  	.hw_reset	= sdhci_hw_reset,
>> +	.set_hs400_dll = sdhci_set_hs400_dll,
> 
> As above, this isn't necessary.
> 
>>  	.enable_sdio_irq = sdhci_enable_sdio_irq,
>>  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
>>  	.prepare_hs400_tuning		= sdhci_prepare_hs400_tuning,
>> @@ -3202,6 +3218,12 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
>>  		host->caps1 &= ~upper_32_bits(dt_caps_mask);
>>  		host->caps1 |= upper_32_bits(dt_caps);
>>  	}
>> +
>> +	if ((host->caps1 & SDHCI_SUPPORT_SDR104) &&
>> +	    (host->caps1 & SDHCI_SUPPORT_DDR50) &&
>> +	    (host->quirks2 & SDHCI_QUIRK2_BROKEN_TUNING_WA)) {
>> +		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
>> +	}
> 
> This doesn't belong here.  See further above about ->probe_slot().
> 
>>  }
>>  EXPORT_SYMBOL_GPL(__sdhci_read_caps);
>>  
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 0469fa1..d3ec57c 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -435,7 +435,8 @@ struct sdhci_host {
>>  #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
>>  /* Broken Clock divider zero in controller */
>>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
>> -
>> +/* Tuning work around */
>> +#define SDHCI_QUIRK2_BROKEN_TUNING_WA		        (1<<16)
>>  	int irq;		/* Device IRQ */
>>  	void __iomem *ioaddr;	/* Mapped address */
>>  
>> @@ -576,6 +577,7 @@ struct sdhci_ops {
>>  	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>>  	void	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
>>  	void	(*hw_reset)(struct sdhci_host *host);
>> +	void    (*set_hs400_dll)(struct sdhci_host *host);
> 
> As further above, this isn't necessary.
> 
>>  	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
>>  	void    (*card_event)(struct sdhci_host *host);
>>  	void	(*voltage_switch)(struct sdhci_host *host);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index ebd1ceb..1b00c28 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -152,6 +152,7 @@ struct mmc_host_ops {
>>  					 unsigned int max_dtr, int host_drv,
>>  					 int card_drv, int *drv_type);
>>  	void	(*hw_reset)(struct mmc_host *host);
>> +	void	(*set_hs400_dll)(struct mmc_host *host);
>>  	void	(*card_event)(struct mmc_host *host);
>>  
>>  	/*
>>
> 
Hi Adrian

Thanks for the valuable feedback and i will get back with new patch trying incorporating reviw comments. I will also try to minimize the dependency on core changes.

Regards
Nehal Shah

[PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Shah, Nehal-bakulchandra]

From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>

This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
HS400 and HS200 mode requires hardware work around also. This patch
adds the quirks for the same.

Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/mmc/core/mmc.c        |  6 +--
 drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c      |  3 +-
 drivers/mmc/host/sdhci.h      |  5 +++
 include/linux/mmc/host.h      |  6 +++
 5 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 4ffea14..7bf3736 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 
 	/* Set host controller to HS timing */
 	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-
+	host->ios.transition = HS200_TO_HS_TO_HS400;
 	/* Reduce frequency to HS frequency */
 	max_dtr = card->ext_csd.hs_max_dtr;
 	mmc_set_clock(host, max_dtr);
@@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 			 mmc_hostname(host), err);
 		return err;
 	}
-
+	host->ios.transition = SWITCHING_TO_HS400;
 	/* Set host controller to HS400 timing and frequency */
 	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
 	mmc_set_bus_speed(card);
@@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = mmc_switch_status(card);
 	if (err)
 		goto out_err;
-
+	host->ios.transition = SWITCHED_TO_HS400;
 	return 0;
 
 out_err:
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index ac678e9..a3456be 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -89,6 +89,47 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
 	return c->slot && (c->slot->flags & flag);
 }
 
+ /*AMD Driver Strength function*/
+
+static int amd_select_drive_strength(struct mmc_card *card,
+				     unsigned int max_dtr, int host_drv,
+				     int card_drv, int *drv_type)
+{
+	return MMC_SET_DRIVER_TYPE_A;
+}
+
+static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
+{
+	/*AMD Platform requires dll setting*/
+	sdhci_writel(host, 0x40003210, SDHCI_AMD_REST_DLL_REGISTER);
+	usleep_range(10, 20);
+	sdhci_writel(host, 0x40033210, SDHCI_AMD_REST_DLL_REGISTER);
+}
+
+/*
+ * For AMD Platform it is required to disable the tuning
+ * bit first controller to bring to HS Mode from HS200
+ * mode, later enable to tune to HS400 mode.
+ */
+
+static void sdhci_amd_set_hs400_transition(struct sdhci_host *host)
+{
+	switch (host->mmc->ios.transition) {
+	case HS200_TO_HS_TO_HS400:
+		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
+		break;
+
+	case SWITCHING_TO_HS400:
+		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
+		sdhci_acpi_amd_hs400_dll(host);
+		break;
+
+	case SWITCHED_TO_HS400:
+	default:
+		break;
+	}
+}
+
 static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
 {
 	u8 reg;
@@ -123,6 +164,18 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
 	.ops = &sdhci_acpi_ops_int,
 };
 
+static const struct sdhci_ops sdhci_acpi_ops_amd = {
+	.set_clock = sdhci_set_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.reset = sdhci_reset,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+	.set_platform_hs400_transition = sdhci_amd_set_hs400_transition,
+};
+
+static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
+	.ops = &sdhci_acpi_ops_amd,
+};
+
 #ifdef CONFIG_X86
 
 static bool sdhci_acpi_byt(void)
@@ -269,6 +322,32 @@ static int bxt_get_cd(struct mmc_host *mmc)
 	return ret;
 }
 
+static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
+					  const char *hid, const char *uid)
+{
+	struct sdhci_acpi_host *c = platform_get_drvdata(pdev);
+	struct sdhci_host *host;
+	unsigned int caps1, caps;
+
+	if (!c || !c->host)
+		return 0;
+
+	host = c->host;
+
+	caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+	caps  = sdhci_readl(host, SDHCI_CAPABILITIES);
+
+	if (caps1 & SDHCI_SUPPORT_DDR50)
+		host->mmc->caps = MMC_CAP_1_8V_DDR;
+
+	if ((caps1 & SDHCI_SUPPORT_SDR104) &&
+	    (host->mmc->caps & MMC_CAP_1_8V_DDR))
+		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
+
+	host->mmc_host_ops.select_drive_strength = amd_select_drive_strength;
+	return 0;
+}
+
 static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
 				      const char *hid, const char *uid)
 {
@@ -370,6 +449,14 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
 	.caps    = MMC_CAP_NONREMOVABLE,
 };
 
+static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
+	.chip   = &sdhci_acpi_chip_amd,
+	.caps   = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |  MMC_CAP_HW_RESET,
+	.quirks = SDHCI_QUIRK_32BIT_DMA_ADDR | SDHCI_QUIRK_32BIT_DMA_SIZE |
+			SDHCI_QUIRK_32BIT_ADMA_SIZE,
+	.probe_slot     = sdhci_acpi_emmc_amd_probe_slot,
+};
+
 struct sdhci_acpi_uid_slot {
 	const char *hid;
 	const char *uid;
@@ -393,6 +480,7 @@ struct sdhci_acpi_uid_slot {
 	{ "PNP0D40"  },
 	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
 	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
+	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc},
 	{ },
 };
 
@@ -409,6 +497,7 @@ struct sdhci_acpi_uid_slot {
 	{ "PNP0D40"  },
 	{ "QCOM8051" },
 	{ "QCOM8052" },
+	{ "AMDI0040" },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d43..ac74390 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1633,7 +1633,8 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		host->ops->set_power(host, ios->power_mode, ios->vdd);
 	else
 		sdhci_set_power(host, ios->power_mode, ios->vdd);
-
+	if (host->ops->set_platform_hs400_transition)
+		host->ops->set_platform_hs400_transition(host);
 	if (host->ops->platform_send_init_74_clocks)
 		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0469fa1..42dd043 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -271,6 +271,9 @@
 #define   SDHCI_SPEC_200	1
 #define   SDHCI_SPEC_300	2
 
+/* AMD sdhci reset dll register.*/
+#define SDHCI_AMD_REST_DLL_REGISTER	0x908
+
 /*
  * End of controller registers.
  */
@@ -571,6 +574,8 @@ struct sdhci_ops {
 	void		(*set_bus_width)(struct sdhci_host *host, int width);
 	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
 					     u8 power_mode);
+	/* ios for transiton phase for going to hs400 */
+	void (*set_platform_hs400_transition)(struct sdhci_host *host);
 	unsigned int    (*get_ro)(struct sdhci_host *host);
 	void		(*reset)(struct sdhci_host *host, u8 mask);
 	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ebd1ceb..0d0d5d3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -77,6 +77,12 @@ struct mmc_ios {
 #define MMC_SET_DRIVER_TYPE_D	3
 
 	bool enhanced_strobe;			/* hs400es selection */
+
+	unsigned int transition;      /* track transition modes (hs200 hs400) */
+
+#define HS200_TO_HS_TO_HS400	1
+#define SWITCHING_TO_HS400	2
+#define SWITCHED_TO_HS400	3
 };
 
 struct mmc_host;
-- 
1.9.1

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Ulf Hansson]

On 12 October 2017 at 12:32, Shah, Nehal-bakulchandra
<Nehal-Bakulchandra.shah@amd.com> wrote:
> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>
> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
> HS400 and HS200 mode requires hardware work around also. This patch
> adds the quirks for the same.

By looking at the changes in the patch, obviously you make changes to
the core, but I think the reasons to why explicitly needs to be stated
also in the change log.

>
> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> ---
>  drivers/mmc/core/mmc.c        |  6 +--
>  drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c      |  3 +-
>  drivers/mmc/host/sdhci.h      |  5 +++
>  include/linux/mmc/host.h      |  6 +++
>  5 files changed, 105 insertions(+), 4 deletions(-)

Please split the core changes into a separate patch.

>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 4ffea14..7bf3736 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>
>         /* Set host controller to HS timing */
>         mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
> -
> +       host->ios.transition = HS200_TO_HS_TO_HS400;
>         /* Reduce frequency to HS frequency */
>         max_dtr = card->ext_csd.hs_max_dtr;
>         mmc_set_clock(host, max_dtr);
> @@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>                          mmc_hostname(host), err);
>                 return err;
>         }
> -
> +       host->ios.transition = SWITCHING_TO_HS400;
>         /* Set host controller to HS400 timing and frequency */
>         mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>         mmc_set_bus_speed(card);
> @@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>         err = mmc_switch_status(card);
>         if (err)
>                 goto out_err;
> -
> +       host->ios.transition = SWITCHED_TO_HS400;
>         return 0;

We have some other host drivers that also needs to follow a specific
sequence while switching to HS400. Those drivers makes use of the
->prepare_hs400_tuning() callback. Have you explored that option, and
if so, why exactly doesn't it work for you?

[...]

>  /*
>   * End of controller registers.
>   */
> @@ -571,6 +574,8 @@ struct sdhci_ops {
>         void            (*set_bus_width)(struct sdhci_host *host, int width);
>         void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>                                              u8 power_mode);
> +       /* ios for transiton phase for going to hs400 */
> +       void (*set_platform_hs400_transition)(struct sdhci_host *host);

In general we avoid adding new sdhci callbacks, but instead we try to
use the sdhci core as a set of library functions.

However, I leave this to managed by Adrian, as we decide this on case
by case basis.

>         unsigned int    (*get_ro)(struct sdhci_host *host);
>         void            (*reset)(struct sdhci_host *host, u8 mask);
>         int     (*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index ebd1ceb..0d0d5d3 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -77,6 +77,12 @@ struct mmc_ios {
>  #define MMC_SET_DRIVER_TYPE_D  3
>
>         bool enhanced_strobe;                   /* hs400es selection */
> +
> +       unsigned int transition;      /* track transition modes (hs200 hs400) */
> +
> +#define HS200_TO_HS_TO_HS400   1
> +#define SWITCHING_TO_HS400     2
> +#define SWITCHED_TO_HS400      3
>  };
>
>  struct mmc_host;
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Shah, Nehal-bakulchandra]

Hi Ulf

Thanks for your review comments.
On 10/16/2017 1:56 PM, Ulf Hansson wrote:
> On 12 October 2017 at 12:32, Shah, Nehal-bakulchandra
> <Nehal-Bakulchandra.shah@amd.com> wrote:
>> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>
>> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
>> HS400 and HS200 mode requires hardware work around also. This patch
>> adds the quirks for the same.
> 
> By looking at the changes in the patch, obviously you make changes to
> the core, but I think the reasons to why explicitly needs to be stated
> also in the change log.
I will update the change log with detailed description.
>>
>> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>> ---
>>  drivers/mmc/core/mmc.c        |  6 +--
>>  drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/host/sdhci.c      |  3 +-
>>  drivers/mmc/host/sdhci.h      |  5 +++
>>  include/linux/mmc/host.h      |  6 +++
>>  5 files changed, 105 insertions(+), 4 deletions(-)
> 
> Please split the core changes into a separate patch.

Sure i will split the core changes into a separate patch.

>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 4ffea14..7bf3736 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>
>>         /* Set host controller to HS timing */
>>         mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> -
>> +       host->ios.transition = HS200_TO_HS_TO_HS400;
>>         /* Reduce frequency to HS frequency */
>>         max_dtr = card->ext_csd.hs_max_dtr;
>>         mmc_set_clock(host, max_dtr);
>> @@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>                          mmc_hostname(host), err);
>>                 return err;
>>         }
>> -
>> +       host->ios.transition = SWITCHING_TO_HS400;
>>         /* Set host controller to HS400 timing and frequency */
>>         mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>         mmc_set_bus_speed(card);
>> @@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>         err = mmc_switch_status(card);
>>         if (err)
>>                 goto out_err;
>> -
>> +       host->ios.transition = SWITCHED_TO_HS400;
>>         return 0;
> 
> We have some other host drivers that also needs to follow a specific
> sequence while switching to HS400. Those drivers makes use of the
> ->prepare_hs400_tuning() callback. Have you explored that option, and
> if so, why exactly doesn't it work for you?
> 
> [...]
> 
I am aware about prepare_hs400_tuning(), however in AMD Platform case, some settings are required after it switches to HS400. HS400 switching is also
required intermediate transition, so in past i discussed with Adrian ( very initial patch ) and he suggested to use ios with transition.
>>  /*
>>   * End of controller registers.
>>   */
>> @@ -571,6 +574,8 @@ struct sdhci_ops {
>>         void            (*set_bus_width)(struct sdhci_host *host, int width);
>>         void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>>                                              u8 power_mode);
>> +       /* ios for transiton phase for going to hs400 */
>> +       void (*set_platform_hs400_transition)(struct sdhci_host *host);
> 
> In general we avoid adding new sdhci callbacks, but instead we try to
> use the sdhci core as a set of library functions.
> 
> However, I leave this to managed by Adrian, as we decide this on case
> by case basis.
> 
I will wait for Adrian's comments also. Based on that will submit the new patch.
>>         unsigned int    (*get_ro)(struct sdhci_host *host);
>>         void            (*reset)(struct sdhci_host *host, u8 mask);
>>         int     (*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index ebd1ceb..0d0d5d3 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -77,6 +77,12 @@ struct mmc_ios {
>>  #define MMC_SET_DRIVER_TYPE_D  3
>>
>>         bool enhanced_strobe;                   /* hs400es selection */
>> +
>> +       unsigned int transition;      /* track transition modes (hs200 hs400) */
>> +
>> +#define HS200_TO_HS_TO_HS400   1
>> +#define SWITCHING_TO_HS400     2
>> +#define SWITCHED_TO_HS400      3
>>  };
>>
>>  struct mmc_host;
>> --
>> 1.9.1
>>
> 
> Kind regards
> Uffe
> 

Regards
Nehal Shah

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Adrian Hunter]

On 12/10/17 13:32, Shah, Nehal-bakulchandra wrote:
> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> 
> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
> HS400 and HS200 mode requires hardware work around also. This patch
> adds the quirks for the same.
> 
> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>

This is V2 of this patch.  In future, please put the version number in the
subject and describe the changes since the previous version in a cover email
or after the "---" below.

> ---
>  drivers/mmc/core/mmc.c        |  6 +--
>  drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c      |  3 +-
>  drivers/mmc/host/sdhci.h      |  5 +++
>  include/linux/mmc/host.h      |  6 +++
>  5 files changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 4ffea14..7bf3736 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>  
>  	/* Set host controller to HS timing */
>  	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
> -
> +	host->ios.transition = HS200_TO_HS_TO_HS400;
>  	/* Reduce frequency to HS frequency */
>  	max_dtr = card->ext_csd.hs_max_dtr;
>  	mmc_set_clock(host, max_dtr);
> @@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>  			 mmc_hostname(host), err);
>  		return err;
>  	}
> -
> +	host->ios.transition = SWITCHING_TO_HS400;
>  	/* Set host controller to HS400 timing and frequency */
>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>  	mmc_set_bus_speed(card);
> @@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>  	err = mmc_switch_status(card);
>  	if (err)
>  		goto out_err;
> -
> +	host->ios.transition = SWITCHED_TO_HS400;
>  	return 0;
>  
>  out_err:
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index ac678e9..a3456be 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -89,6 +89,47 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>  	return c->slot && (c->slot->flags & flag);
>  }
>  
> + /*AMD Driver Strength function*/

This comment is redundant - please omit it.

> +
> +static int amd_select_drive_strength(struct mmc_card *card,
> +				     unsigned int max_dtr, int host_drv,
> +				     int card_drv, int *drv_type)
> +{
> +	return MMC_SET_DRIVER_TYPE_A;
> +}
> +
> +static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
> +{
> +	/*AMD Platform requires dll setting*/
> +	sdhci_writel(host, 0x40003210, SDHCI_AMD_REST_DLL_REGISTER);
> +	usleep_range(10, 20);
> +	sdhci_writel(host, 0x40033210, SDHCI_AMD_REST_DLL_REGISTER);
> +}
> +
> +/*
> + * For AMD Platform it is required to disable the tuning
> + * bit first controller to bring to HS Mode from HS200
> + * mode, later enable to tune to HS400 mode.
> + */
> +

Why can't you hook ->set_ios() e.g.

	host->mmc_host_ops.set_ios = amd_set_ios;

static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
{
	sdhci_set_ios(mmc, ios);

	if (ios->timing == MMC_TIMING_MMC_HS400) {
		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
		sdhci_acpi_amd_hs400_dll(host);
	}
}


> +static void sdhci_amd_set_hs400_transition(struct sdhci_host *host)
> +{
> +	switch (host->mmc->ios.transition) {
> +	case HS200_TO_HS_TO_HS400:
> +		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);

Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
and leave the other bits alone.

We know when we do tuning whether it is for HS400, so maybe you could hook
->execute_tuning() for this e.g.

	host->mmc_host_ops.execute_tuning = amd_execute_tuning;

static int amd_execute_tuning(struct mmc_host *mmc, u32 opcode)
{
	bool hs400_tuning = host->flags & SDHCI_HS400_TUNING;
	int err;

	err = sdhci_execute_tuning(mmc, opcode);
	if (err)
		return err;

	/* Disable tuning during switch to HS400 */
	if (hs400_tuning)
		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
}

> +		break;
> +
> +	case SWITCHING_TO_HS400:

This case is always ios->timing == MMC_TIMING_MMC_HS400 so you don't need
the ios.transition for this

> +		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);

Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
and leave the other bits alone.

> +		sdhci_acpi_amd_hs400_dll(host);
> +		break;
> +
> +	case SWITCHED_TO_HS400:
> +	default:
> +		break;
> +	}
> +}
> +
>  static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>  {
>  	u8 reg;
> @@ -123,6 +164,18 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>  	.ops = &sdhci_acpi_ops_int,
>  };
>  
> +static const struct sdhci_ops sdhci_acpi_ops_amd = {
> +	.set_clock = sdhci_set_clock,
> +	.set_bus_width = sdhci_set_bus_width,
> +	.reset = sdhci_reset,
> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
> +	.set_platform_hs400_transition = sdhci_amd_set_hs400_transition,
> +};
> +
> +static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
> +	.ops = &sdhci_acpi_ops_amd,
> +};
> +
>  #ifdef CONFIG_X86
>  
>  static bool sdhci_acpi_byt(void)
> @@ -269,6 +322,32 @@ static int bxt_get_cd(struct mmc_host *mmc)
>  	return ret;
>  }
>  
> +static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
> +					  const char *hid, const char *uid)
> +{
> +	struct sdhci_acpi_host *c = platform_get_drvdata(pdev);
> +	struct sdhci_host *host;
> +	unsigned int caps1, caps;
> +
> +	if (!c || !c->host)
> +		return 0;

c and c->host cannot be NULL;

> +
> +	host = c->host;
> +
> +	caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
> +	caps  = sdhci_readl(host, SDHCI_CAPABILITIES);

Use sdhci_read_caps(host) and then host->caps and host->caps1

> +
> +	if (caps1 & SDHCI_SUPPORT_DDR50)
> +		host->mmc->caps = MMC_CAP_1_8V_DDR;
> +
> +	if ((caps1 & SDHCI_SUPPORT_SDR104) &&
> +	    (host->mmc->caps & MMC_CAP_1_8V_DDR))
> +		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
> +
> +	host->mmc_host_ops.select_drive_strength = amd_select_drive_strength;
> +	return 0;
> +}
> +
>  static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
>  				      const char *hid, const char *uid)
>  {
> @@ -370,6 +449,14 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
>  	.caps    = MMC_CAP_NONREMOVABLE,
>  };
>  
> +static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
> +	.chip   = &sdhci_acpi_chip_amd,
> +	.caps   = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |  MMC_CAP_HW_RESET,
> +	.quirks = SDHCI_QUIRK_32BIT_DMA_ADDR | SDHCI_QUIRK_32BIT_DMA_SIZE |
> +			SDHCI_QUIRK_32BIT_ADMA_SIZE,
> +	.probe_slot     = sdhci_acpi_emmc_amd_probe_slot,
> +};
> +
>  struct sdhci_acpi_uid_slot {
>  	const char *hid;
>  	const char *uid;
> @@ -393,6 +480,7 @@ struct sdhci_acpi_uid_slot {
>  	{ "PNP0D40"  },
>  	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
>  	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
> +	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc},
>  	{ },
>  };
>  
> @@ -409,6 +497,7 @@ struct sdhci_acpi_uid_slot {
>  	{ "PNP0D40"  },
>  	{ "QCOM8051" },
>  	{ "QCOM8052" },
> +	{ "AMDI0040" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d43..ac74390 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1633,7 +1633,8 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>  		host->ops->set_power(host, ios->power_mode, ios->vdd);
>  	else
>  		sdhci_set_power(host, ios->power_mode, ios->vdd);
> -
> +	if (host->ops->set_platform_hs400_transition)
> +		host->ops->set_platform_hs400_transition(host);
>  	if (host->ops->platform_send_init_74_clocks)
>  		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>  
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0469fa1..42dd043 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -271,6 +271,9 @@
>  #define   SDHCI_SPEC_200	1
>  #define   SDHCI_SPEC_300	2
>  
> +/* AMD sdhci reset dll register.*/
> +#define SDHCI_AMD_REST_DLL_REGISTER	0x908

Put this definition in sdhci-acpi.c

> +
>  /*
>   * End of controller registers.
>   */
> @@ -571,6 +574,8 @@ struct sdhci_ops {
>  	void		(*set_bus_width)(struct sdhci_host *host, int width);
>  	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>  					     u8 power_mode);
> +	/* ios for transiton phase for going to hs400 */
> +	void (*set_platform_hs400_transition)(struct sdhci_host *host);
>  	unsigned int    (*get_ro)(struct sdhci_host *host);
>  	void		(*reset)(struct sdhci_host *host, u8 mask);
>  	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index ebd1ceb..0d0d5d3 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -77,6 +77,12 @@ struct mmc_ios {
>  #define MMC_SET_DRIVER_TYPE_D	3
>  
>  	bool enhanced_strobe;			/* hs400es selection */
> +
> +	unsigned int transition;      /* track transition modes (hs200 hs400) */

I am not sure you need this since, as described above, the 2 cases you are
using are:
- after tuning but you can hook ->execute tuning()
- when switching to HS400 but that is ios->timing == MMC_TIMING_MMC_HS400

> +
> +#define HS200_TO_HS_TO_HS400	1
> +#define SWITCHING_TO_HS400	2
> +#define SWITCHED_TO_HS400	3
>  };
>  
>  struct mmc_host;
> 

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Shah, Nehal-bakulchandra]

Hi Adrian 

On 10/19/2017 3:32 PM, Adrian Hunter wrote:
> On 12/10/17 13:32, Shah, Nehal-bakulchandra wrote:
>> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>
>> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
>> HS400 and HS200 mode requires hardware work around also. This patch
>> adds the quirks for the same.
>>
>> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> 
> This is V2 of this patch.  In future, please put the version number in the
> subject and describe the changes since the previous version in a cover email
> or after the "---" below.
> 
>> ---
>>  drivers/mmc/core/mmc.c        |  6 +--
>>  drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/host/sdhci.c      |  3 +-
>>  drivers/mmc/host/sdhci.h      |  5 +++
>>  include/linux/mmc/host.h      |  6 +++
>>  5 files changed, 105 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 4ffea14..7bf3736 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  
>>  	/* Set host controller to HS timing */
>>  	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>> -
>> +	host->ios.transition = HS200_TO_HS_TO_HS400;
>>  	/* Reduce frequency to HS frequency */
>>  	max_dtr = card->ext_csd.hs_max_dtr;
>>  	mmc_set_clock(host, max_dtr);
>> @@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  			 mmc_hostname(host), err);
>>  		return err;
>>  	}
>> -
>> +	host->ios.transition = SWITCHING_TO_HS400;
>>  	/* Set host controller to HS400 timing and frequency */
>>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>  	mmc_set_bus_speed(card);
>> @@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	err = mmc_switch_status(card);
>>  	if (err)
>>  		goto out_err;
>> -
>> +	host->ios.transition = SWITCHED_TO_HS400;
>>  	return 0;
>>  
>>  out_err:
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index ac678e9..a3456be 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -89,6 +89,47 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>  	return c->slot && (c->slot->flags & flag);
>>  }
>>  
>> + /*AMD Driver Strength function*/
> 
> This comment is redundant - please omit it.
> 
>> +
>> +static int amd_select_drive_strength(struct mmc_card *card,
>> +				     unsigned int max_dtr, int host_drv,
>> +				     int card_drv, int *drv_type)
>> +{
>> +	return MMC_SET_DRIVER_TYPE_A;
>> +}
>> +
>> +static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
>> +{
>> +	/*AMD Platform requires dll setting*/
>> +	sdhci_writel(host, 0x40003210, SDHCI_AMD_REST_DLL_REGISTER);
>> +	usleep_range(10, 20);
>> +	sdhci_writel(host, 0x40033210, SDHCI_AMD_REST_DLL_REGISTER);
>> +}
>> +
>> +/*
>> + * For AMD Platform it is required to disable the tuning
>> + * bit first controller to bring to HS Mode from HS200
>> + * mode, later enable to tune to HS400 mode.
>> + */
>> +
> 
> Why can't you hook ->set_ios() e.g.
> 
> 	host->mmc_host_ops.set_ios = amd_set_ios;
> 
> static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> {
> 	sdhci_set_ios(mmc, ios);
> 
> 	if (ios->timing == MMC_TIMING_MMC_HS400) {
> 		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
> 		sdhci_acpi_amd_hs400_dll(host);
> 	}
> }
> 
> 
>> +static void sdhci_amd_set_hs400_transition(struct sdhci_host *host)
>> +{
>> +	switch (host->mmc->ios.transition) {
>> +	case HS200_TO_HS_TO_HS400:
>> +		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
> 
> Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
> and leave the other bits alone.
> 
> We know when we do tuning whether it is for HS400, so maybe you could hook
> ->execute_tuning() for this e.g.
> 
> 	host->mmc_host_ops.execute_tuning = amd_execute_tuning;
> 
> static int amd_execute_tuning(struct mmc_host *mmc, u32 opcode)
> {
> 	bool hs400_tuning = host->flags & SDHCI_HS400_TUNING;
> 	int err;
> 
> 	err = sdhci_execute_tuning(mmc, opcode);
> 	if (err)
> 		return err;
> 
> 	/* Disable tuning during switch to HS400 */
> 	if (hs400_tuning)
> 		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
> }
> 
>> +		break;
>> +
>> +	case SWITCHING_TO_HS400:
> 
> This case is always ios->timing == MMC_TIMING_MMC_HS400 so you don't need
> the ios.transition for this
> 
>> +		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
> 
> Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
> and leave the other bits alone.
> 
>> +		sdhci_acpi_amd_hs400_dll(host);
>> +		break;
>> +
>> +	case SWITCHED_TO_HS400:
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
Liked your suggestions and tried but doesn't work.Because The changes are required in transition phase i.e in case of switching to HS400 just after "mmc_set_timing(card->host, MMC_TIMING_MMC_HS)" call so these changes are intermediate as we have issue with the IP. Now Hooking with execute_tuning makes the functionality non working as execute_tuning gets called before. So is there any better way to handle intermediate transition? Can you please suggest. So instead of using execute_tuning hook up can be done in set_ios however still it requires the transition cases. Something like this

static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
{
        struct sdhci_host *host = mmc_priv(mmc);
        sdhci_set_ios(mmc, ios);


       switch (host->mmc->ios.transition) {
       case HS200_TO_HS_TO_HS400:
       {
           printk(KERN_ERR "Sandeep transition HS200 to HS to HS400 \n");
           sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
       }
       break;

       case SWITCHING_TO_HS400:
       {

        printk(KERN_ERR "Sandeep transition Switching to HS400 \n");
               sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);

               sdhci_acpi_amd_hs400_dll(host);
       }
       break;

       case SWITCHED_TO_HS400:
       default:
       break;
       }

}




>>  static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>>  {
>>  	u8 reg;
>> @@ -123,6 +164,18 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>>  	.ops = &sdhci_acpi_ops_int,
>>  };
>>  
>> +static const struct sdhci_ops sdhci_acpi_ops_amd = {
>> +	.set_clock = sdhci_set_clock,
>> +	.set_bus_width = sdhci_set_bus_width,
>> +	.reset = sdhci_reset,
>> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
>> +	.set_platform_hs400_transition = sdhci_amd_set_hs400_transition,
>> +};
>> +
>> +static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
>> +	.ops = &sdhci_acpi_ops_amd,
>> +};
>> +
>>  #ifdef CONFIG_X86
>>  
>>  static bool sdhci_acpi_byt(void)
>> @@ -269,6 +322,32 @@ static int bxt_get_cd(struct mmc_host *mmc)
>>  	return ret;
>>  }
>>  
>> +static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
>> +					  const char *hid, const char *uid)
>> +{
>> +	struct sdhci_acpi_host *c = platform_get_drvdata(pdev);
>> +	struct sdhci_host *host;
>> +	unsigned int caps1, caps;
>> +
>> +	if (!c || !c->host)
>> +		return 0;
> 
> c and c->host cannot be NULL;
> 
>> +
>> +	host = c->host;
>> +
>> +	caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>> +	caps  = sdhci_readl(host, SDHCI_CAPABILITIES);
> 
> Use sdhci_read_caps(host) and then host->caps and host->caps1
> 
>> +
>> +	if (caps1 & SDHCI_SUPPORT_DDR50)
>> +		host->mmc->caps = MMC_CAP_1_8V_DDR;
>> +
>> +	if ((caps1 & SDHCI_SUPPORT_SDR104) &&
>> +	    (host->mmc->caps & MMC_CAP_1_8V_DDR))
>> +		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
>> +
>> +	host->mmc_host_ops.select_drive_strength = amd_select_drive_strength;
>> +	return 0;
>> +}
>> +
>>  static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
>>  				      const char *hid, const char *uid)
>>  {
>> @@ -370,6 +449,14 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
>>  	.caps    = MMC_CAP_NONREMOVABLE,
>>  };
>>  
>> +static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
>> +	.chip   = &sdhci_acpi_chip_amd,
>> +	.caps   = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |  MMC_CAP_HW_RESET,
>> +	.quirks = SDHCI_QUIRK_32BIT_DMA_ADDR | SDHCI_QUIRK_32BIT_DMA_SIZE |
>> +			SDHCI_QUIRK_32BIT_ADMA_SIZE,
>> +	.probe_slot     = sdhci_acpi_emmc_amd_probe_slot,
>> +};
>> +
>>  struct sdhci_acpi_uid_slot {
>>  	const char *hid;
>>  	const char *uid;
>> @@ -393,6 +480,7 @@ struct sdhci_acpi_uid_slot {
>>  	{ "PNP0D40"  },
>>  	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
>>  	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
>> +	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc},
>>  	{ },
>>  };
>>  
>> @@ -409,6 +497,7 @@ struct sdhci_acpi_uid_slot {
>>  	{ "PNP0D40"  },
>>  	{ "QCOM8051" },
>>  	{ "QCOM8052" },
>> +	{ "AMDI0040" },
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ecd0d43..ac74390 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1633,7 +1633,8 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>  		host->ops->set_power(host, ios->power_mode, ios->vdd);
>>  	else
>>  		sdhci_set_power(host, ios->power_mode, ios->vdd);
>> -
>> +	if (host->ops->set_platform_hs400_transition)
>> +		host->ops->set_platform_hs400_transition(host);
>>  	if (host->ops->platform_send_init_74_clocks)
>>  		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>>  
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 0469fa1..42dd043 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -271,6 +271,9 @@
>>  #define   SDHCI_SPEC_200	1
>>  #define   SDHCI_SPEC_300	2
>>  
>> +/* AMD sdhci reset dll register.*/
>> +#define SDHCI_AMD_REST_DLL_REGISTER	0x908
> 
> Put this definition in sdhci-acpi.c
> 
>> +
>>  /*
>>   * End of controller registers.
>>   */
>> @@ -571,6 +574,8 @@ struct sdhci_ops {
>>  	void		(*set_bus_width)(struct sdhci_host *host, int width);
>>  	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>>  					     u8 power_mode);
>> +	/* ios for transiton phase for going to hs400 */
>> +	void (*set_platform_hs400_transition)(struct sdhci_host *host);
>>  	unsigned int    (*get_ro)(struct sdhci_host *host);
>>  	void		(*reset)(struct sdhci_host *host, u8 mask);
>>  	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index ebd1ceb..0d0d5d3 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -77,6 +77,12 @@ struct mmc_ios {
>>  #define MMC_SET_DRIVER_TYPE_D	3
>>  
>>  	bool enhanced_strobe;			/* hs400es selection */
>> +
>> +	unsigned int transition;      /* track transition modes (hs200 hs400) */
> 
> I am not sure you need this since, as described above, the 2 cases you are
> using are:
> - after tuning but you can hook ->execute tuning()
> - when switching to HS400 but that is ios->timing == MMC_TIMING_MMC_HS400
> 
>> +
>> +#define HS200_TO_HS_TO_HS400	1
>> +#define SWITCHING_TO_HS400	2
>> +#define SWITCHED_TO_HS400	3
>>  };
>>  
>>  struct mmc_host;
>>
> 

Regards
Nehal Shah

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Shah, Nehal-bakulchandra]

Hi Adrian

On 11/3/2017 2:53 PM, Shah, Nehal-bakulchandra wrote:
> Hi Adrian 
> 
> On 10/19/2017 3:32 PM, Adrian Hunter wrote:
>> On 12/10/17 13:32, Shah, Nehal-bakulchandra wrote:
>>> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>>
>>> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
>>> HS400 and HS200 mode requires hardware work around also. This patch
>>> adds the quirks for the same.
>>>
>>> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>
>> This is V2 of this patch.  In future, please put the version number in the
>> subject and describe the changes since the previous version in a cover email
>> or after the "---" below.
>>
>>> ---
>>>  drivers/mmc/core/mmc.c        |  6 +--
>>>  drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/mmc/host/sdhci.c      |  3 +-
>>>  drivers/mmc/host/sdhci.h      |  5 +++
>>>  include/linux/mmc/host.h      |  6 +++
>>>  5 files changed, 105 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 4ffea14..7bf3736 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>>  
>>>  	/* Set host controller to HS timing */
>>>  	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>>> -
>>> +	host->ios.transition = HS200_TO_HS_TO_HS400;
>>>  	/* Reduce frequency to HS frequency */
>>>  	max_dtr = card->ext_csd.hs_max_dtr;
>>>  	mmc_set_clock(host, max_dtr);
>>> @@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>>  			 mmc_hostname(host), err);
>>>  		return err;
>>>  	}
>>> -
>>> +	host->ios.transition = SWITCHING_TO_HS400;
>>>  	/* Set host controller to HS400 timing and frequency */
>>>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>>  	mmc_set_bus_speed(card);
>>> @@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>>  	err = mmc_switch_status(card);
>>>  	if (err)
>>>  		goto out_err;
>>> -
>>> +	host->ios.transition = SWITCHED_TO_HS400;
>>>  	return 0;
>>>  
>>>  out_err:
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index ac678e9..a3456be 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -89,6 +89,47 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>>  	return c->slot && (c->slot->flags & flag);
>>>  }
>>>  
>>> + /*AMD Driver Strength function*/
>>
>> This comment is redundant - please omit it.
>>
>>> +
>>> +static int amd_select_drive_strength(struct mmc_card *card,
>>> +				     unsigned int max_dtr, int host_drv,
>>> +				     int card_drv, int *drv_type)
>>> +{
>>> +	return MMC_SET_DRIVER_TYPE_A;
>>> +}
>>> +
>>> +static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
>>> +{
>>> +	/*AMD Platform requires dll setting*/
>>> +	sdhci_writel(host, 0x40003210, SDHCI_AMD_REST_DLL_REGISTER);
>>> +	usleep_range(10, 20);
>>> +	sdhci_writel(host, 0x40033210, SDHCI_AMD_REST_DLL_REGISTER);
>>> +}
>>> +
>>> +/*
>>> + * For AMD Platform it is required to disable the tuning
>>> + * bit first controller to bring to HS Mode from HS200
>>> + * mode, later enable to tune to HS400 mode.
>>> + */
>>> +
>>
>> Why can't you hook ->set_ios() e.g.
>>
>> 	host->mmc_host_ops.set_ios = amd_set_ios;
>>
>> static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> {
>> 	sdhci_set_ios(mmc, ios);
>>
>> 	if (ios->timing == MMC_TIMING_MMC_HS400) {
>> 		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
>> 		sdhci_acpi_amd_hs400_dll(host);
>> 	}
>> }
>>
>>
>>> +static void sdhci_amd_set_hs400_transition(struct sdhci_host *host)
>>> +{
>>> +	switch (host->mmc->ios.transition) {
>>> +	case HS200_TO_HS_TO_HS400:
>>> +		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
>>
>> Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
>> and leave the other bits alone.
>>
>> We know when we do tuning whether it is for HS400, so maybe you could hook
>> ->execute_tuning() for this e.g.
>>
>> 	host->mmc_host_ops.execute_tuning = amd_execute_tuning;
>>
>> static int amd_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> {
>> 	bool hs400_tuning = host->flags & SDHCI_HS400_TUNING;
>> 	int err;
>>
>> 	err = sdhci_execute_tuning(mmc, opcode);
>> 	if (err)
>> 		return err;
>>
>> 	/* Disable tuning during switch to HS400 */
>> 	if (hs400_tuning)
>> 		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
>> }
>>
>>> +		break;
>>> +
>>> +	case SWITCHING_TO_HS400:
>>
>> This case is always ios->timing == MMC_TIMING_MMC_HS400 so you don't need
>> the ios.transition for this
>>
>>> +		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
>>
>> Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
>> and leave the other bits alone.
>>
>>> +		sdhci_acpi_amd_hs400_dll(host);
>>> +		break;
>>> +
>>> +	case SWITCHED_TO_HS400:
>>> +	default:
>>> +		break;
>>> +	}
>>> +}
>>> +
> Liked your suggestions and tried but doesn't work.Because The changes are required in transition phase i.e in case of switching to HS400 just after "mmc_set_timing(card->host, MMC_TIMING_MMC_HS)" call so these changes are intermediate as we have issue with the IP. Now Hooking with execute_tuning makes the functionality non working as execute_tuning gets called before. So is there any better way to handle intermediate transition? Can you please suggest. So instead of using execute_tuning hook up can be done in set_ios however still it requires the transition cases. Something like this
> 
> static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> {
>         struct sdhci_host *host = mmc_priv(mmc);
>         sdhci_set_ios(mmc, ios);
> 
> 
>        switch (host->mmc->ios.transition) {
>        case HS200_TO_HS_TO_HS400:
>        {
>            printk(KERN_ERR "Sandeep transition HS200 to HS to HS400 \n");
>            sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
>        }
>        break;
> 
ignore printk message mistakenly put debug message in sample example
>        case SWITCHING_TO_HS400:
>        {
> 
>         printk(KERN_ERR "Sandeep transition Switching to HS400 \n");
>                sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
> 
>                sdhci_acpi_amd_hs400_dll(host);
>        }
>        break;

ignore printk message mistakenly put debug message in sample example
>        case SWITCHED_TO_HS400:
>        default:
>        break;
>        }
> 
> }
> 
> 
> 
> 
>>>  static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>>>  {
>>>  	u8 reg;
>>> @@ -123,6 +164,18 @@ static void sdhci_acpi_int_hw_reset(struct sdhci_host *host)
>>>  	.ops = &sdhci_acpi_ops_int,
>>>  };
>>>  
>>> +static const struct sdhci_ops sdhci_acpi_ops_amd = {
>>> +	.set_clock = sdhci_set_clock,
>>> +	.set_bus_width = sdhci_set_bus_width,
>>> +	.reset = sdhci_reset,
>>> +	.set_uhs_signaling = sdhci_set_uhs_signaling,
>>> +	.set_platform_hs400_transition = sdhci_amd_set_hs400_transition,
>>> +};
>>> +
>>> +static const struct sdhci_acpi_chip sdhci_acpi_chip_amd = {
>>> +	.ops = &sdhci_acpi_ops_amd,
>>> +};
>>> +
>>>  #ifdef CONFIG_X86
>>>  
>>>  static bool sdhci_acpi_byt(void)
>>> @@ -269,6 +322,32 @@ static int bxt_get_cd(struct mmc_host *mmc)
>>>  	return ret;
>>>  }
>>>  
>>> +static int sdhci_acpi_emmc_amd_probe_slot(struct platform_device *pdev,
>>> +					  const char *hid, const char *uid)
>>> +{
>>> +	struct sdhci_acpi_host *c = platform_get_drvdata(pdev);
>>> +	struct sdhci_host *host;
>>> +	unsigned int caps1, caps;
>>> +
>>> +	if (!c || !c->host)
>>> +		return 0;
>>
>> c and c->host cannot be NULL;
>>
>>> +
>>> +	host = c->host;
>>> +
>>> +	caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>>> +	caps  = sdhci_readl(host, SDHCI_CAPABILITIES);
>>
>> Use sdhci_read_caps(host) and then host->caps and host->caps1
>>
>>> +
>>> +	if (caps1 & SDHCI_SUPPORT_DDR50)
>>> +		host->mmc->caps = MMC_CAP_1_8V_DDR;
>>> +
>>> +	if ((caps1 & SDHCI_SUPPORT_SDR104) &&
>>> +	    (host->mmc->caps & MMC_CAP_1_8V_DDR))
>>> +		host->mmc->caps2 = MMC_CAP2_HS400_1_8V;
>>> +
>>> +	host->mmc_host_ops.select_drive_strength = amd_select_drive_strength;
>>> +	return 0;
>>> +}
>>> +
>>>  static int sdhci_acpi_emmc_probe_slot(struct platform_device *pdev,
>>>  				      const char *hid, const char *uid)
>>>  {
>>> @@ -370,6 +449,14 @@ static int sdhci_acpi_sd_probe_slot(struct platform_device *pdev,
>>>  	.caps    = MMC_CAP_NONREMOVABLE,
>>>  };
>>>  
>>> +static const struct sdhci_acpi_slot sdhci_acpi_slot_amd_emmc = {
>>> +	.chip   = &sdhci_acpi_chip_amd,
>>> +	.caps   = MMC_CAP_8_BIT_DATA | MMC_CAP_NONREMOVABLE |  MMC_CAP_HW_RESET,
>>> +	.quirks = SDHCI_QUIRK_32BIT_DMA_ADDR | SDHCI_QUIRK_32BIT_DMA_SIZE |
>>> +			SDHCI_QUIRK_32BIT_ADMA_SIZE,
>>> +	.probe_slot     = sdhci_acpi_emmc_amd_probe_slot,
>>> +};
>>> +
>>>  struct sdhci_acpi_uid_slot {
>>>  	const char *hid;
>>>  	const char *uid;
>>> @@ -393,6 +480,7 @@ struct sdhci_acpi_uid_slot {
>>>  	{ "PNP0D40"  },
>>>  	{ "QCOM8051", NULL, &sdhci_acpi_slot_qcom_sd_3v },
>>>  	{ "QCOM8052", NULL, &sdhci_acpi_slot_qcom_sd },
>>> +	{ "AMDI0040", NULL, &sdhci_acpi_slot_amd_emmc},
>>>  	{ },
>>>  };
>>>  
>>> @@ -409,6 +497,7 @@ struct sdhci_acpi_uid_slot {
>>>  	{ "PNP0D40"  },
>>>  	{ "QCOM8051" },
>>>  	{ "QCOM8052" },
>>> +	{ "AMDI0040" },
>>>  	{ },
>>>  };
>>>  MODULE_DEVICE_TABLE(acpi, sdhci_acpi_ids);
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index ecd0d43..ac74390 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1633,7 +1633,8 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>>  		host->ops->set_power(host, ios->power_mode, ios->vdd);
>>>  	else
>>>  		sdhci_set_power(host, ios->power_mode, ios->vdd);
>>> -
>>> +	if (host->ops->set_platform_hs400_transition)
>>> +		host->ops->set_platform_hs400_transition(host);
>>>  	if (host->ops->platform_send_init_74_clocks)
>>>  		host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>>>  
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 0469fa1..42dd043 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -271,6 +271,9 @@
>>>  #define   SDHCI_SPEC_200	1
>>>  #define   SDHCI_SPEC_300	2
>>>  
>>> +/* AMD sdhci reset dll register.*/
>>> +#define SDHCI_AMD_REST_DLL_REGISTER	0x908
>>
>> Put this definition in sdhci-acpi.c
>>
>>> +
>>>  /*
>>>   * End of controller registers.
>>>   */
>>> @@ -571,6 +574,8 @@ struct sdhci_ops {
>>>  	void		(*set_bus_width)(struct sdhci_host *host, int width);
>>>  	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>>>  					     u8 power_mode);
>>> +	/* ios for transiton phase for going to hs400 */
>>> +	void (*set_platform_hs400_transition)(struct sdhci_host *host);
>>>  	unsigned int    (*get_ro)(struct sdhci_host *host);
>>>  	void		(*reset)(struct sdhci_host *host, u8 mask);
>>>  	int	(*platform_execute_tuning)(struct sdhci_host *host, u32 opcode);
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index ebd1ceb..0d0d5d3 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -77,6 +77,12 @@ struct mmc_ios {
>>>  #define MMC_SET_DRIVER_TYPE_D	3
>>>  
>>>  	bool enhanced_strobe;			/* hs400es selection */
>>> +
>>> +	unsigned int transition;      /* track transition modes (hs200 hs400) */
>>
>> I am not sure you need this since, as described above, the 2 cases you are
>> using are:
>> - after tuning but you can hook ->execute tuning()
>> - when switching to HS400 but that is ios->timing == MMC_TIMING_MMC_HS400
>>
>>> +
>>> +#define HS200_TO_HS_TO_HS400	1
>>> +#define SWITCHING_TO_HS400	2
>>> +#define SWITCHED_TO_HS400	3
>>>  };
>>>  
>>>  struct mmc_host;
>>>
>>
> 
> Regards
> Nehal Shah
> 

Regards
Nehal Shah

Re: [PATCH] mmc: sdhci-acpi: Add support for ACPI HID of AMD Controller with HS400

[Adrian Hunter]

On 03/11/17 11:23, Shah, Nehal-bakulchandra wrote:
> Hi Adrian 
> 
> On 10/19/2017 3:32 PM, Adrian Hunter wrote:
>> On 12/10/17 13:32, Shah, Nehal-bakulchandra wrote:
>>> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>>
>>> This patch supports HS400 for AMD upcoming emmc 5.0 controller.The
>>> HS400 and HS200 mode requires hardware work around also. This patch
>>> adds the quirks for the same.
>>>
>>> Signed-off-by: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>
>> This is V2 of this patch.  In future, please put the version number in the
>> subject and describe the changes since the previous version in a cover email
>> or after the "---" below.
>>
>>> ---
>>>  drivers/mmc/core/mmc.c        |  6 +--
>>>  drivers/mmc/host/sdhci-acpi.c | 89 +++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/mmc/host/sdhci.c      |  3 +-
>>>  drivers/mmc/host/sdhci.h      |  5 +++
>>>  include/linux/mmc/host.h      |  6 +++
>>>  5 files changed, 105 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index 4ffea14..7bf3736 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -1164,7 +1164,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>>  
>>>  	/* Set host controller to HS timing */
>>>  	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
>>> -
>>> +	host->ios.transition = HS200_TO_HS_TO_HS400;
>>>  	/* Reduce frequency to HS frequency */
>>>  	max_dtr = card->ext_csd.hs_max_dtr;
>>>  	mmc_set_clock(host, max_dtr);
>>> @@ -1196,7 +1196,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>>  			 mmc_hostname(host), err);
>>>  		return err;
>>>  	}
>>> -
>>> +	host->ios.transition = SWITCHING_TO_HS400;
>>>  	/* Set host controller to HS400 timing and frequency */
>>>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>>  	mmc_set_bus_speed(card);
>>> @@ -1204,7 +1204,7 @@ static int mmc_select_hs400(struct mmc_card *card)
>>>  	err = mmc_switch_status(card);
>>>  	if (err)
>>>  		goto out_err;
>>> -
>>> +	host->ios.transition = SWITCHED_TO_HS400;
>>>  	return 0;
>>>  
>>>  out_err:
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index ac678e9..a3456be 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -89,6 +89,47 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)
>>>  	return c->slot && (c->slot->flags & flag);
>>>  }
>>>  
>>> + /*AMD Driver Strength function*/
>>
>> This comment is redundant - please omit it.
>>
>>> +
>>> +static int amd_select_drive_strength(struct mmc_card *card,
>>> +				     unsigned int max_dtr, int host_drv,
>>> +				     int card_drv, int *drv_type)
>>> +{
>>> +	return MMC_SET_DRIVER_TYPE_A;
>>> +}
>>> +
>>> +static void sdhci_acpi_amd_hs400_dll(struct sdhci_host *host)
>>> +{
>>> +	/*AMD Platform requires dll setting*/
>>> +	sdhci_writel(host, 0x40003210, SDHCI_AMD_REST_DLL_REGISTER);
>>> +	usleep_range(10, 20);
>>> +	sdhci_writel(host, 0x40033210, SDHCI_AMD_REST_DLL_REGISTER);
>>> +}
>>> +
>>> +/*
>>> + * For AMD Platform it is required to disable the tuning
>>> + * bit first controller to bring to HS Mode from HS200
>>> + * mode, later enable to tune to HS400 mode.
>>> + */
>>> +
>>
>> Why can't you hook ->set_ios() e.g.
>>
>> 	host->mmc_host_ops.set_ios = amd_set_ios;
>>
>> static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> {
>> 	sdhci_set_ios(mmc, ios);
>>
>> 	if (ios->timing == MMC_TIMING_MMC_HS400) {
>> 		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
>> 		sdhci_acpi_amd_hs400_dll(host);
>> 	}
>> }
>>
>>
>>> +static void sdhci_amd_set_hs400_transition(struct sdhci_host *host)
>>> +{
>>> +	switch (host->mmc->ios.transition) {
>>> +	case HS200_TO_HS_TO_HS400:
>>> +		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
>>
>> Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
>> and leave the other bits alone.
>>
>> We know when we do tuning whether it is for HS400, so maybe you could hook
>> ->execute_tuning() for this e.g.
>>
>> 	host->mmc_host_ops.execute_tuning = amd_execute_tuning;
>>
>> static int amd_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> {
>> 	bool hs400_tuning = host->flags & SDHCI_HS400_TUNING;
>> 	int err;
>>
>> 	err = sdhci_execute_tuning(mmc, opcode);
>> 	if (err)
>> 		return err;
>>
>> 	/* Disable tuning during switch to HS400 */
>> 	if (hs400_tuning)
>> 		sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
>> }
>>
>>> +		break;
>>> +
>>> +	case SWITCHING_TO_HS400:
>>
>> This case is always ios->timing == MMC_TIMING_MMC_HS400 so you don't need
>> the ios.transition for this
>>
>>> +		sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
>>
>> Maybe better to just change the bits in SDHCI_HOST_CONTROL2 that you need
>> and leave the other bits alone.
>>
>>> +		sdhci_acpi_amd_hs400_dll(host);
>>> +		break;
>>> +
>>> +	case SWITCHED_TO_HS400:
>>> +	default:
>>> +		break;
>>> +	}
>>> +}
>>> +
> Liked your suggestions and tried but doesn't work.Because The changes are required in transition phase i.e in case of switching to HS400 just after "mmc_set_timing(card->host, MMC_TIMING_MMC_HS)" call so these changes are intermediate as we have issue with the IP. Now Hooking with execute_tuning makes the functionality non working as execute_tuning gets called before. So is there any better way to handle intermediate transition? Can you please suggest. So instead of using execute_tuning hook up can be done in set_ios however still it requires the transition cases. Something like this

But what about below:

> 
> static void amd_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> {
>         struct sdhci_host *host = mmc_priv(mmc);

	unsigned old_timing = host->timing;

>         sdhci_set_ios(mmc, ios);
> 
> 
>        switch (host->mmc->ios.transition) {
>        case HS200_TO_HS_TO_HS400:

	if (old_timing == MMC_TIMING_MMC_HS200 &&
	    ios->timing == MMC_TIMING_SD_HS)

If you want to know if this is the HS400 case, you could record whether the
SDHCI_HS400_TUNING flag was set during tuning.

>        {
>            printk(KERN_ERR "Sandeep transition HS200 to HS to HS400 \n");
>            sdhci_writew(host, 0x9, SDHCI_HOST_CONTROL2);
>        }
>        break;
> 
>        case SWITCHING_TO_HS400:

	if (old_timing != MMC_TIMING_MMC_HS400 &&
	    ios->timing == MMC_TIMING_MMC_HS400)

>        {
> 
>         printk(KERN_ERR "Sandeep transition Switching to HS400 \n");
>                sdhci_writew(host, 0x80, SDHCI_HOST_CONTROL2);
> 
>                sdhci_acpi_amd_hs400_dll(host);
>        }
>        break;
> 
>        case SWITCHED_TO_HS400:
>        default:
>        break;
>        }
> 
> }