On 17/08/17 08:57, Kishon Vijay Abraham I wrote: > 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_* You mean it will do those things when you add the callbacks? I would prefer you leave out runtime pm until you can add the callbacks as well. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
