Hi Adrian, On Tuesday 15 August 2017 01:52 PM, Adrian Hunter wrote: > On 07/08/17 19:01, Kishon Vijay Abraham I wrote: >> Create a new sdhci-omap driver to configure the eMMC/SD/SDIO controller >> in TI's OMAP SoCs making use of the SDHCI core library. For OMAP specific >> configurations, populate sdhci_ops with OMAP specific callbacks and use >> SDHCI quirks. >> Enable only high speed mode for both SD and eMMC here and add other >> UHS mode support later. > > I only have a couple of comments below. > >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx> >> --- >> drivers/mmc/host/Kconfig | 12 + >> drivers/mmc/host/Makefile | 1 + >> drivers/mmc/host/sdhci-omap.c | 593 ++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 606 insertions(+) >> create mode 100644 drivers/mmc/host/sdhci-omap.c >> >> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig >> index 5755b69f2f72..9aa7e5ce43ba 100644 >> --- a/drivers/mmc/host/Kconfig >> +++ b/drivers/mmc/host/Kconfig >> @@ -871,3 +871,15 @@ config MMC_SDHCI_XENON >> This selects Marvell Xenon eMMC/SD/SDIO SDHCI. >> If you have a controller with this interface, say Y or M here. >> If unsure, say N. >> + >> +config MMC_SDHCI_OMAP >> + tristate "TI SDHCI Controller Support" >> + depends on MMC_SDHCI_PLTFM >> + help >> + This selects the Secure Digital Host Controller Interface (SDHCI) >> + support present in TI's DRA7 SOCs. The controller supports >> + SD/MMC/SDIO devices. >> + >> + If you have a controller with this interface, say Y or M here. >> + >> + If unsure, say N. >> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile >> index 4d4547116311..30fb8b0fd0e1 100644 >> --- a/drivers/mmc/host/Makefile >> +++ b/drivers/mmc/host/Makefile >> @@ -82,6 +82,7 @@ obj-$(CONFIG_MMC_SDHCI_MSM) += sdhci-msm.o >> obj-$(CONFIG_MMC_SDHCI_ST) += sdhci-st.o >> obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32) += sdhci-pic32.o >> obj-$(CONFIG_MMC_SDHCI_BRCMSTB) += sdhci-brcmstb.o >> +obj-$(CONFIG_MMC_SDHCI_OMAP) += sdhci-omap.o >> >> ifeq ($(CONFIG_CB710_DEBUG),y) >> CFLAGS-cb710-mmc += -DDEBUG >> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c >> new file mode 100644 >> index 000000000000..c067c428a0cd >> --- /dev/null >> +++ b/drivers/mmc/host/sdhci-omap.c >> @@ -0,0 +1,593 @@ >> +/* >> + * SDHCI Controller driver for TI's OMAP SoCs >> + * >> + * Copyright (C) 2016-2017 Texas Instruments Incorporated - http://www.ti.com >> + * >> + * Authors: Kishon Vijay Abraham I <kishon@xxxxxx> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/delay.h> >> +#include <linux/mmc/slot-gpio.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/regulator/consumer.h> >> + >> +#include "sdhci-pltfm.h" >> + >> +#define SDHCI_OMAP_CON 0x12c >> +#define CON_DW8 BIT(5) >> +#define CON_DMA_MASTER BIT(20) >> +#define CON_INIT BIT(1) >> +#define CON_OD BIT(0) >> + >> +#define SDHCI_OMAP_CMD 0x20c >> + >> +#define SDHCI_OMAP_HCTL 0x228 >> +#define HCTL_SDBP BIT(8) >> +#define HCTL_SDVS_SHIFT 9 >> +#define HCTL_SDVS_MASK (0x7 << HCTL_SDVS_SHIFT) >> +#define HCTL_SDVS_33 (0x7 << HCTL_SDVS_SHIFT) >> +#define HCTL_SDVS_30 (0x6 << HCTL_SDVS_SHIFT) >> +#define HCTL_SDVS_18 (0x5 << HCTL_SDVS_SHIFT) >> + >> +#define SDHCI_OMAP_SYSCTL 0x22c >> +#define SYSCTL_CEN BIT(2) >> +#define SYSCTL_CLKD_SHIFT 6 >> +#define SYSCTL_CLKD_MASK 0x3ff >> + >> +#define SDHCI_OMAP_STAT 0x230 >> + >> +#define SDHCI_OMAP_IE 0x234 >> +#define INT_CC_EN BIT(0) >> + >> +#define SDHCI_OMAP_AC12 0x23c >> +#define AC12_V1V8_SIGEN BIT(19) >> + >> +#define SDHCI_OMAP_CAPA 0x240 >> +#define CAPA_VS33 BIT(24) >> +#define CAPA_VS30 BIT(25) >> +#define CAPA_VS18 BIT(26) >> + >> +#define MMC_TIMEOUT 1 /* 1 msec */ > > This name "MMC_TIMEOUT" is too generic. SDHCI_OMAP_TIMEOUT would be better. okay. > >> + >> +#define SYSCTL_CLKD_MAX 0x3FF >> + >> +#define IOV_1V8 1800000 /* 180000 uV */ >> +#define IOV_3V0 3000000 /* 300000 uV */ >> +#define IOV_3V3 3300000 /* 330000 uV */ >> + >> +struct sdhci_omap_data { >> + u32 offset; >> +}; >> + >> +struct sdhci_omap_host { >> + void __iomem *base; >> + struct device *dev; >> + struct regulator *pbias; >> + bool pbias_enabled; >> + struct sdhci_host *host; >> + unsigned int flags; >> + >> +#define SDHCI_OMAP_SUPPORTS_DUAL_VOLT BIT(0) >> +#define SDHCI_OMAP_NO_1_8_V BIT(1) >> + >> + u8 power_mode; >> + bool io_3_3v_support; >> +}; >> + . . <<snip>> . . >> +static int sdhci_omap_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + u32 offset; >> + struct device *dev = &pdev->dev; >> + struct sdhci_host *host; >> + struct sdhci_pltfm_host *pltfm_host; >> + struct sdhci_omap_host *omap_host; >> + struct mmc_host *mmc; >> + const struct of_device_id *match; >> + struct sdhci_omap_data *data; >> + >> + match = of_match_device(omap_sdhci_match, dev); >> + if (!match) >> + return -EINVAL; >> + >> + data = (struct sdhci_omap_data *)match->data; >> + if (!data) { >> + dev_err(dev, "no sdhci omap data\n"); >> + return -EINVAL; >> + } >> + >> + offset = data->offset; >> + >> + host = sdhci_pltfm_init(pdev, &sdhci_omap_pdata, >> + sizeof(*omap_host)); >> + if (IS_ERR(host)) { >> + dev_err(dev, "Failed sdhci_pltfm_init\n"); >> + return PTR_ERR(host); >> + } >> + >> + pltfm_host = sdhci_priv(host); >> + omap_host = sdhci_pltfm_priv(pltfm_host); >> + omap_host->host = host; >> + omap_host->base = host->ioaddr; >> + omap_host->dev = dev; >> + sdhci_omap_get_of_property(omap_host); >> + >> + host->ioaddr += offset; >> + >> + mmc = host->mmc; >> + ret = mmc_of_parse(mmc); >> + if (ret) >> + goto err_of_parse; >> + >> + pltfm_host->clk = devm_clk_get(dev, "fck"); >> + if (IS_ERR(pltfm_host->clk)) { >> + ret = PTR_ERR(pltfm_host->clk); >> + goto err_of_parse; >> + } >> + >> + ret = clk_set_rate(pltfm_host->clk, mmc->f_max); >> + if (ret) { >> + dev_err(dev, "failed to set clock to %d\n", mmc->f_max); >> + goto err_of_parse; >> + } >> + >> + omap_host->pbias = devm_regulator_get_optional(dev, "pbias"); >> + if (IS_ERR(omap_host->pbias)) { >> + ret = PTR_ERR(omap_host->pbias); >> + if (ret != -ENODEV) { >> + dev_err(dev, "CONFIG_REGULATOR_PBIAS not enabled??\n"); >> + return ret; >> + } >> + dev_dbg(dev, "unable to get pbias regulator %ld\n", >> + PTR_ERR(omap_host->pbias)); >> + } else { >> + if (regulator_is_supported_voltage(omap_host->pbias, IOV_3V3, >> + IOV_3V3)) >> + omap_host->io_3_3v_support = true; >> + dev_vdbg(dev, "Max PBIAS support Voltage: %dmv\n", >> + omap_host->io_3_3v_support ? IOV_3V3 : IOV_3V0); >> + } >> + omap_host->pbias_enabled = false; >> + >> + pm_runtime_enable(dev); >> + ret = pm_runtime_get_sync(dev); > > What are you trying to do with runtime pm? There don't seem to be any pm > callbacks, so does this do anything? yeah, pm_runtime_get_sync enables the functional clock and also configures the SYSCONFIG regiters present in the controller. It gets the details for configuring those from arch/arm/mach-omap2/omap_hwmod_* Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
