On 28/06/17 13:53, Shah, Nehal-bakulchandra wrote:
> From: Shah Nehal-Bakulchandra <Nehal-bakulchandra.Shah@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> ---
> 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);
>
> /*
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
