| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Aug 2017 13:29:18 +0530
From: Kishon Vijay Abraham I <kishon@...com>
To: Adrian Hunter <adrian.hunter@...el.com>,
Ulf Hansson <ulf.hansson@...aro.org>,
Tony Lindgren <tony@...mide.com>,
Rob Herring <robh+dt@...nel.org>
CC: <nsekhar@...com>, <linux-omap@...r.kernel.org>,
<linux-mmc@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 6/7] mmc: sdhci-omap: Add OMAP SDHCI driver
Hi Adrian,
On Thursday 17 August 2017 12:13 PM, Adrian Hunter wrote:
> 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@...com>
>>>> ---
>>>> 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@...com>
>>>> + *
>>>> + * 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.
No. Generally in callbacks, 'optional' functional clocks are enabled. But main
functional clock and interface clocks are enabled in pm_runtime_get_sync.
Without pm_runtime_get_sync, we won't be able to access controller registers.
Thanks
Kishon
Powered by blists - more mailing lists