* [PATCH v9 0/3] mmc: sdhci-msm: Add support for Qualcomm chipsets
@ 2014-02-28 11:24 Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation Georgi Djakov
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Georgi Djakov @ 2014-02-28 11:24 UTC (permalink / raw)
To: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob
Cc: linux-doc, linux-kernel, linux-arm-msm, Georgi Djakov
This patchset adds basic support of the Secure Digital Host Controller
Interface compliant controller found in Qualcomm SoCs.
Tested with eMMC and various micro SD cards on APQ8074 Dragonboard.
Applies to linux-next.
Changes from v8:
- Added controller version suffix to the DT compatible string.
- Switched Kconfig dependency from ARCH_MSM to the new ARCH_QCOM multiplatform.
- Addressed comments from Stephen Boyd on the 2nd patch (execute tunning).
- Added signed-off-by lines of the initial driver authors.
- Picked up tested-by. https://lkml.org/lkml/2013/11/14/85
- Minor changes on comments, prints and formatting.
Changes from v7:
- Added call to sdhci_get_of_property().
- Refactored sdhci_msm_dt_parse_vreg_info().
- Fixed possible ERR_PTR() dereferencing.
- Updated DT binding documentation.
- Removed lpm and currents from DT.
- Removed bus-speed-mode from DT.
- Updated and moved the sanity checks.
- Various typo and coding style fixes.
- Added platform_execute_tunning implementation.
Changes from v6:
- Fixed wrong pointer in sdhci_msm_pwr_irq().
- Added platform_execute_tuning() callback as the MSM SDHC does not
support tuning as in SDHC 3.0 spec and will need custom implementation
in order to support SDR104, HS200 and HS400.
- Removed the always-on devicetree property - if the regulator is
configured as always-on, it will not be disabled anyway.
- Removed devm_pinctrl_get_select_default() - the default pins are
already set from the device core.
- Removed wrapper function sdhci_msm_set_vdd_io_vol() and enum
vdd_io_level and now calling regulator_set_voltage() directly.
- Converted #defines to use BIT() macro.
- Added IS_ERR(vreg->reg) check at the beginning of sdhci_msm_vreg
functions.
- Do not print errors when regulators init return -EPROBE_DEFER as the
deffered init is not an actual error.
- Handle each power irq status bit separately in sdhci_msm_pwr_irq().
- Ensure that any pending power irq is acknowledged before enabling it,
otherwise the irq handler will be fired prematurely.
- Minor changes.
Changes from v5:
- Driver is split into multiple patches
- Do not initialize variables that are assigned later in code
- Remove some useless comments
- Use shorter variable names
- Change pr_err() to dev_err()
- Optimized sdhci_msm_setup_vreg()
- Some code alignment fixes
- Improved DT values sanity check
- Added dev_dbg print for sdhci controller version in probe()
- Added usleep_range() after SW reset - it can take some time
- Added SDHCI_QUIRK_SINGLE_POWER_WRITE - power handled by PMIC
- Renamed DT property vdd-io to vddio
Changes from v4:
- Simplified sdhci_msm_vreg_disable() and sdhci_msm_set_vdd_io_vol()
- Use devm_ioremap_resource() instead of devm_ioremap()
- Converted IS_ERR_OR_NULL to IS_ERR
- Disable regulators in sdhci_msm_remove()
- Check for DT node at the beginning in sdhci_msm_probe()
- Removed more redundant code
- Changes in some error messages
- Minor fixes
Changes from v3:
- Allocate memory for all required structs at once
- Added termination entry in sdhci_msm_dt_match[]
- Fixed a missing sdhci_pltfm_free() in probe()
- Removed redundant of_match_ptr
- Removed the unneeded function sdhci_msm_vreg_reset()
Changes from v2:
- Added DT bindings for clocks
- Moved voltage regulators data to platform data
- Removed unneeded includes
- Removed obsolete and wrapper functions
- Removed error checking where unnecessary
- Removed redundant _clk suffix from clock names
- Just return instead of goto where possible
- Minor fixes
Georgi Djakov (3):
mmc: sdhci-msm: Qualcomm SDHCI binding documentation
mmc: sdhci-msm: Initial support for Qualcomm chipsets
mmc: sdhci-msm: Add platform_execute_tunning implementation
.../devicetree/bindings/mmc/sdhci-msm.txt | 80 ++
drivers/mmc/host/Kconfig | 13 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-msm.c | 940 ++++++++++++++++++++
4 files changed, 1034 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
create mode 100644 drivers/mmc/host/sdhci-msm.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation
2014-02-28 11:24 [PATCH v9 0/3] mmc: sdhci-msm: Add support for Qualcomm chipsets Georgi Djakov
@ 2014-02-28 11:24 ` Georgi Djakov
2014-03-03 2:03 ` Bjorn Andersson
2014-02-28 11:24 ` [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation Georgi Djakov
2 siblings, 1 reply; 13+ messages in thread
From: Georgi Djakov @ 2014-02-28 11:24 UTC (permalink / raw)
To: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob
Cc: linux-doc, linux-kernel, linux-arm-msm, Georgi Djakov
This patch adds the device-tree binding documentation for
Qualcomm SDHCI driver. It contains the differences between
the core properties in mmc.txt and the properties used by
the sdhci-msm driver.
Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
---
.../devicetree/bindings/mmc/sdhci-msm.txt | 80 ++++++++++++++++++++
1 file changed, 80 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-msm.txt
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
new file mode 100644
index 0000000..d136cb7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -0,0 +1,80 @@
+* Qualcomm SDHCI controller (sdhci-msm)
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci-msm driver.
+
+Required properties:
+- compatible: Should contain "qcom,sdhci-msm-v4".
+- reg: Base address and length of the register set listed in reg-names.
+- reg-names: Should contain the following:
+ "hc_mem" - Host controller register map
+ "core_mem" - SD Core register map
+- interrupts: Should contain an interrupt-specifiers for the interrupts listed in interrupt-names.
+- interrupt-names: Should contain the following:
+ "hc_irq" - Host controller interrupt
+ "pwr_irq" - PMIC interrupt
+- vdd-supply: Phandle to the regulator for the vdd (core voltage) supply.
+- vdd-io-supply: Phandle to the regulator for the vdd-io (i/o voltage) supply.
+- pinctrl-names: Should contain only one value - "default".
+- pinctrl-0: Should specify pin control groups used for this controller.
+- clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock-names.
+- clock-names: Should contain the following:
+ "iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
+ "core" - SDC MMC clock (MCLK) (required)
+ "bus" - SDCC bus voter clock (optional)
+
+Optional properties:
+
+- qcom,vdd-voltage-min - Specifies the minimum core voltage supported by the device in microvolts.
+- qcom,vdd-voltage-max - Specifies the maximum core voltage supported by the device in microvolts.
+- qcom,vdd-io-voltage-min - Specifies the minimum i/o voltage supported by the device in microvolts.
+- qcom,vdd-io-voltage-max - Specifies the maximum i/o voltage supported by the device in microvolts.
+
+Example:
+
+ sdhc_1: sdhci@f9824900 {
+ compatible = "qcom,sdhci-msm-v4";
+ reg = <0xf9824900 0x11c>, <0xf9824000 0x800>;
+ reg-names = "hc_mem", "core_mem";
+ interrupts = <0 123 0>, <0 138 0>;
+ interrupt-names = "hc_irq", "pwr_irq";
+ bus-width = <8>;
+ non-removable;
+
+ vdd-supply = <&pm8941_l20>;
+ vdd-io-supply = <&pm8941_s3>;
+
+ qcom,vdd-voltage-min = <2950000>;
+ qcom,vdd-voltage-max = <2950000>;
+ qcom,vdd-io-voltage-min = <1800000>;
+ qcom,vdd-io-voltage-max = <1800000>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
+
+ clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
+ clock-names = "core", "iface";
+ };
+
+ sdhc_2: sdhci@f98a4900 {
+ compatible = "qcom,sdhci-msm-v4";
+ reg = <0xf98a4900 0x11c>, <0xf98a4000 0x800>;
+ reg-names = "hc_mem", "core_mem";
+ interrupts = <0 125 0>, <0 221 0>;
+ interrupt-names = "hc_irq", "pwr_irq";
+ bus-width = <4>;
+
+ vdd-supply = <&pm8941_l21>;
+ vdd-io-supply = <&pm8941_l13>;
+
+ qcom,vdd-voltage-min = <2950000>;
+ qcom,vdd-voltage-max = <2950000>;
+ qcom,vdd-io-voltage-min = <1800000>;
+ qcom,vdd-io-voltage-max = <2950000>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&sdc2_clk &sdc2_cmd &sdc2_data>;
+
+ clocks = <&gcc GCC_SDCC2_APPS_CLK>, <&gcc GCC_SDCC2_AHB_CLK>;
+ clock-names = "core", "iface";
+ };
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets
2014-02-28 11:24 [PATCH v9 0/3] mmc: sdhci-msm: Add support for Qualcomm chipsets Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation Georgi Djakov
@ 2014-02-28 11:24 ` Georgi Djakov
2014-03-04 3:15 ` Bjorn Andersson
2014-02-28 11:24 ` [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation Georgi Djakov
2 siblings, 1 reply; 13+ messages in thread
From: Georgi Djakov @ 2014-02-28 11:24 UTC (permalink / raw)
To: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob
Cc: linux-doc, linux-kernel, linux-arm-msm, Georgi Djakov
This platform driver adds the initial support of Secure
Digital Host Controller Interface compliant controller
found in Qualcomm chipsets.
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Tested-by: Ivan T. Ivanov <iivanov@mm-sol.com>
Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
---
drivers/mmc/host/Kconfig | 13 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-msm.c | 528 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 542 insertions(+)
create mode 100644 drivers/mmc/host/sdhci-msm.c
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 82cc34d..66ef8b9 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -334,6 +334,19 @@ config MMC_ATMELMCI
If unsure, say N.
+config MMC_SDHCI_MSM
+ tristate "Qualcomm SDHCI Controller Support"
+ depends on ARCH_QCOM
+ depends on MMC_SDHCI_PLTFM
+ help
+ This selects the Secure Digital Host Controller Interface (SDHCI)
+ support present in Qualcomm 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.
+
config MMC_MSM
tristate "Qualcomm SDCC Controller Support"
depends on MMC && (ARCH_MSM7X00A || ARCH_MSM7X30 || ARCH_QSD8X50)
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index f162f87a0..0c8aa5e 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o
obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o
obj-$(CONFIG_MMC_SDHCI_BCM_KONA) += sdhci-bcm-kona.o
obj-$(CONFIG_MMC_SDHCI_BCM2835) += sdhci-bcm2835.o
+obj-$(CONFIG_MMC_SDHCI_MSM) += sdhci-msm.o
ifeq ($(CONFIG_CB710_DEBUG),y)
CFLAGS-cb710-mmc += -DDEBUG
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
new file mode 100644
index 0000000..b4490a2
--- /dev/null
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -0,0 +1,528 @@
+/*
+ * drivers/mmc/host/sdhci-msm.c - Qualcomm SDHCI Platform driver
+ *
+ * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+
+#include "sdhci-pltfm.h"
+
+#define CORE_HC_MODE 0x78
+#define HC_MODE_EN 0x1
+
+#define CORE_POWER 0x0
+#define CORE_SW_RST BIT(7)
+
+#define CORE_PWRCTL_STATUS 0xdc
+#define CORE_PWRCTL_MASK 0xe0
+#define CORE_PWRCTL_CLEAR 0xe4
+#define CORE_PWRCTL_CTL 0xe8
+
+#define CORE_PWRCTL_BUS_OFF BIT(0)
+#define CORE_PWRCTL_BUS_ON BIT(1)
+#define CORE_PWRCTL_IO_LOW BIT(2)
+#define CORE_PWRCTL_IO_HIGH BIT(3)
+
+#define CORE_PWRCTL_BUS_SUCCESS BIT(0)
+#define CORE_PWRCTL_BUS_FAIL BIT(1)
+#define CORE_PWRCTL_IO_SUCCESS BIT(2)
+#define CORE_PWRCTL_IO_FAIL BIT(3)
+
+#define INT_MASK 0xf
+
+
+/* This structure keeps information per regulator */
+struct sdhci_msm_reg_data {
+ struct regulator *reg;
+ const char *name;
+ /* Voltage level values */
+ u32 low_vol_level;
+ u32 high_vol_level;
+};
+
+struct sdhci_msm_pltfm_data {
+ u32 caps; /* Supported UHS-I Modes */
+ u32 caps2; /* More capabilities */
+ struct sdhci_msm_reg_data vdd; /* VDD/VCC regulator info */
+ struct sdhci_msm_reg_data vdd_io; /* VDD IO regulator info */
+};
+
+struct sdhci_msm_host {
+ struct platform_device *pdev;
+ void __iomem *core_mem; /* MSM SDCC mapped address */
+ int pwr_irq; /* power irq */
+ struct clk *clk; /* main SD/MMC bus clock */
+ struct clk *pclk; /* SDHC peripheral bus clock */
+ struct clk *bus_clk; /* SDHC bus voter clock */
+ struct sdhci_msm_pltfm_data pdata;
+ struct mmc_host *mmc;
+ struct sdhci_pltfm_data sdhci_msm_pdata;
+};
+
+/* MSM platform specific tuning */
+int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
+{
+ /*
+ * Tuning is required for SDR104, HS200 and HS400 cards and if the clock
+ * frequency greater than 100MHz in those modes. The standard tuning
+ * procedure should not be executed, but a custom implementation will be
+ * added here instead.
+ */
+ return 0;
+}
+
+#define MAX_PROP_SIZE 32
+static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
+ struct sdhci_msm_reg_data *vreg, const char *vreg_name)
+{
+ char prop_name[MAX_PROP_SIZE];
+ struct device_node *np = dev->of_node;
+
+ vreg->name = vreg_name;
+
+ snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-voltage-min", vreg_name);
+ of_property_read_u32(np, prop_name, &vreg->low_vol_level);
+ snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-voltage-max", vreg_name);
+ of_property_read_u32(np, prop_name, &vreg->high_vol_level);
+
+ /* Sanity check */
+ if (vreg->low_vol_level > vreg->high_vol_level) {
+ dev_err(dev, "%s Invalid constraints specified\n", vreg->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* Parse devicetree data */
+static int sdhci_msm_populate_pdata(struct device *dev,
+ struct sdhci_msm_pltfm_data *pdata)
+{
+ if (sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd, "vdd")) {
+ dev_err(dev, "Failed parsing vdd data\n");
+ return -EINVAL;
+ }
+
+ if (sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_io, "vdd-io")) {
+ dev_err(dev, "Failed parsing vdd-io data\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int sdhci_msm_vreg_enable(struct device *dev,
+ struct sdhci_msm_reg_data *vreg)
+{
+ int ret = 0;
+
+ if (!regulator_is_enabled(vreg->reg)) {
+ /* Set voltage level */
+ ret = regulator_set_voltage(vreg->reg, vreg->high_vol_level,
+ vreg->high_vol_level);
+ if (ret)
+ return ret;
+ }
+
+ ret = regulator_enable(vreg->reg);
+ if (ret) {
+ dev_err(dev, "Failed to enable regulator %s (%d)\n",
+ vreg->name, ret);
+ }
+
+ return ret;
+}
+
+static int sdhci_msm_vreg_disable(struct device *dev,
+ struct sdhci_msm_reg_data *vreg)
+{
+ int ret = 0;
+
+ if (!regulator_is_enabled(vreg->reg))
+ return ret;
+
+ /* Set min. voltage to 0 */
+ ret = regulator_set_voltage(vreg->reg, 0, vreg->high_vol_level);
+ if (ret)
+ return ret;
+
+ ret = regulator_disable(vreg->reg);
+ if (ret) {
+ dev_err(dev, "Failed to disable regulator %s (%d)\n",
+ vreg->name, ret);
+ }
+
+ return ret;
+}
+
+static int sdhci_msm_setup_vreg(struct sdhci_msm_host *msm_host, bool enable)
+{
+ int ret, i;
+ struct sdhci_msm_reg_data *vreg_table[2];
+
+ vreg_table[0] = &msm_host->pdata.vdd;
+ vreg_table[1] = &msm_host->pdata.vdd_io;
+
+ for (i = 0; i < ARRAY_SIZE(vreg_table); i++) {
+ if (enable)
+ ret = sdhci_msm_vreg_enable(&msm_host->pdev->dev,
+ vreg_table[i]);
+ else
+ ret = sdhci_msm_vreg_disable(&msm_host->pdev->dev,
+ vreg_table[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int sdhci_msm_vreg_init(struct device *dev,
+ struct sdhci_msm_pltfm_data *pdata)
+{
+ struct sdhci_msm_reg_data *vdd_reg = &pdata->vdd;
+ struct sdhci_msm_reg_data *vdd_io_reg = &pdata->vdd_io;
+
+ vdd_reg->reg = devm_regulator_get(dev, vdd_reg->name);
+ if (IS_ERR(vdd_reg->reg))
+ return PTR_ERR(vdd_reg->reg);
+
+ vdd_io_reg->reg = devm_regulator_get(dev, vdd_io_reg->name);
+ if (IS_ERR(vdd_io_reg->reg))
+ return PTR_ERR(vdd_io_reg->reg);
+
+ return 0;
+}
+
+static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
+{
+ struct sdhci_host *host = (struct sdhci_host *)data;
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = pltfm_host->priv;
+ u8 irq_status;
+ u8 irq_ack = 0;
+ int ret = 0;
+
+ irq_status = readb_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
+ dev_dbg(mmc_dev(msm_host->mmc), "%s: Received IRQ(%d), status=0x%x\n",
+ mmc_hostname(msm_host->mmc), irq, irq_status);
+
+ /* Clear the interrupt */
+ writeb_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
+ /*
+ * SDHC has core_mem and hc_mem device memory and these memory
+ * addresses do not fall within 1KB region. Hence, any update to
+ * core_mem address space would require an mb() to ensure this gets
+ * completed before its next update to registers within hc_mem.
+ */
+ mb();
+
+ /* Handle BUS ON/OFF */
+ if (irq_status & CORE_PWRCTL_BUS_ON) {
+ ret = sdhci_msm_setup_vreg(msm_host, true);
+ if (!ret)
+ ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+ msm_host->pdata.
+ vdd_io.high_vol_level,
+ msm_host->pdata.
+ vdd_io.high_vol_level);
+ if (ret)
+ irq_ack |= CORE_PWRCTL_BUS_FAIL;
+ else
+ irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+ }
+
+ if (irq_status & CORE_PWRCTL_BUS_OFF) {
+ ret = sdhci_msm_setup_vreg(msm_host, false);
+ if (!ret)
+ ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+ msm_host->pdata.
+ vdd_io.low_vol_level,
+ msm_host->pdata.
+ vdd_io.low_vol_level);
+ if (ret)
+ irq_ack |= CORE_PWRCTL_BUS_FAIL;
+ else
+ irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+ }
+
+ /* Handle IO LOW/HIGH */
+ if (irq_status & CORE_PWRCTL_IO_LOW) {
+ ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+ msm_host->pdata.
+ vdd_io.low_vol_level,
+ msm_host->pdata.
+ vdd_io.low_vol_level);
+ if (ret)
+ irq_ack |= CORE_PWRCTL_IO_FAIL;
+ else
+ irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+ }
+
+ if (irq_status & CORE_PWRCTL_IO_HIGH) {
+ ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
+ msm_host->pdata.
+ vdd_io.high_vol_level,
+ msm_host->pdata.
+ vdd_io.high_vol_level);
+ if (ret)
+ irq_ack |= CORE_PWRCTL_IO_FAIL;
+ else
+ irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+ }
+
+ /* ACK status to the core */
+ writeb_relaxed(irq_ack, (msm_host->core_mem + CORE_PWRCTL_CTL));
+ /*
+ * SDHC has core_mem and hc_mem device memory and these memory
+ * addresses do not fall within 1KB region. Hence, any update to
+ * core_mem address space would require an mb() to ensure this gets
+ * completed before its next update to registers within hc_mem.
+ */
+ mb();
+
+ dev_dbg(mmc_dev(msm_host->mmc), "%s: Handled IRQ(%d), ret=%d, ack=0x%x\n",
+ mmc_hostname(msm_host->mmc), irq, ret, irq_ack);
+ return IRQ_HANDLED;
+}
+
+static const struct of_device_id sdhci_msm_dt_match[] = {
+ { .compatible = "qcom,sdhci-msm-v4" },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
+
+static struct sdhci_ops sdhci_msm_ops = {
+ .platform_execute_tuning = sdhci_msm_execute_tuning,
+};
+
+static int sdhci_msm_probe(struct platform_device *pdev)
+{
+ struct sdhci_host *host;
+ struct sdhci_pltfm_host *pltfm_host;
+ struct sdhci_msm_host *msm_host;
+ struct resource *core_memres = NULL;
+ int ret, dead;
+ u16 host_version;
+
+ if (!pdev->dev.of_node) {
+ dev_err(&pdev->dev, "No device tree data\n");
+ return -ENOENT;
+ }
+
+ msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
+ if (!msm_host)
+ return -ENOMEM;
+
+ msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
+ host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
+ if (IS_ERR(host))
+ return PTR_ERR(host);
+
+ pltfm_host = sdhci_priv(host);
+ pltfm_host->priv = msm_host;
+ msm_host->mmc = host->mmc;
+ msm_host->pdev = pdev;
+
+ ret = mmc_of_parse(host->mmc);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed parsing mmc device tree\n");
+ goto pltfm_free;
+ }
+
+ sdhci_get_of_property(pdev);
+
+ ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
+ if (ret) {
+ dev_err(&pdev->dev, "DT parsing error\n");
+ goto pltfm_free;
+ }
+
+ /* Setup SDCC bus voter clock. */
+ msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ if (!IS_ERR(msm_host->bus_clk)) {
+ /* Vote for max. clk rate for max. performance */
+ ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
+ if (ret)
+ goto pltfm_free;
+ ret = clk_prepare_enable(msm_host->bus_clk);
+ if (ret)
+ goto pltfm_free;
+ }
+
+ /* Setup main peripheral bus clock */
+ msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
+ if (!IS_ERR(msm_host->pclk)) {
+ ret = clk_prepare_enable(msm_host->pclk);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Peripheral clock setup failed (%d)\n", ret);
+ goto bus_clk_disable;
+ }
+ }
+
+ /* Setup SDC MMC clock */
+ msm_host->clk = devm_clk_get(&pdev->dev, "core");
+ if (IS_ERR(msm_host->clk)) {
+ ret = PTR_ERR(msm_host->clk);
+ dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
+ goto pclk_disable;
+ }
+
+ ret = clk_prepare_enable(msm_host->clk);
+ if (ret)
+ goto pclk_disable;
+
+ /* Setup regulators */
+ ret = sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev,
+ "Regulator setup failed (%d)\n", ret);
+ goto clk_disable;
+ }
+
+ core_memres = platform_get_resource_byname(pdev,
+ IORESOURCE_MEM, "core_mem");
+ msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
+
+ if (IS_ERR(msm_host->core_mem)) {
+ dev_err(&pdev->dev, "Failed to remap registers\n");
+ ret = PTR_ERR(msm_host->core_mem);
+ goto vreg_disable;
+ }
+
+ /* Reset the core and Enable SDHC mode */
+ writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
+ CORE_SW_RST, msm_host->core_mem + CORE_POWER);
+
+ /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
+ usleep_range(1000, 5000);
+ if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
+ dev_err(&pdev->dev, "Stuck in reset\n");
+ ret = -ETIMEDOUT;
+ goto vreg_disable;
+ }
+
+ /* Set HC_MODE_EN bit in HC_MODE register */
+ writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
+
+ /*
+ * Following are the deviations from SDHC spec v3.0 -
+ * 1. Card detection is handled using separate GPIO.
+ * 2. Bus power control is handled by interacting with PMIC.
+ */
+ host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
+ host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;
+
+ host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
+ dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
+ host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
+ SDHCI_VENDOR_VER_SHIFT));
+
+ /* Setup PWRCTL irq */
+ msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
+ if (msm_host->pwr_irq < 0) {
+ dev_err(&pdev->dev, "Failed to get pwr_irq by name (%d)\n",
+ msm_host->pwr_irq);
+ goto vreg_disable;
+ }
+ ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
+ sdhci_msm_pwr_irq, IRQF_ONESHOT,
+ dev_name(&pdev->dev), host);
+ if (ret) {
+ dev_err(&pdev->dev, "Request threaded irq(%d) failed (%d)\n",
+ msm_host->pwr_irq, ret);
+ goto vreg_disable;
+ }
+
+ /* Enable pwr irq interrupts */
+ writel_relaxed(INT_MASK, (msm_host->core_mem + CORE_PWRCTL_MASK));
+
+ msm_host->mmc->caps |= msm_host->pdata.caps;
+ msm_host->mmc->caps2 |= msm_host->pdata.caps2;
+
+ ret = sdhci_add_host(host);
+ if (ret) {
+ dev_err(&pdev->dev, "Add host failed (%d)\n", ret);
+ goto vreg_disable;
+ }
+
+ ret = clk_set_rate(msm_host->clk, host->max_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "MClk rate set failed (%d)\n", ret);
+ goto remove_host;
+ }
+
+ return 0;
+
+remove_host:
+ dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
+ sdhci_remove_host(host, dead);
+vreg_disable:
+ if (!IS_ERR(msm_host->pdata.vdd.reg))
+ sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
+ if (!IS_ERR(msm_host->pdata.vdd_io.reg))
+ sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
+clk_disable:
+ if (!IS_ERR(msm_host->clk))
+ clk_disable_unprepare(msm_host->clk);
+pclk_disable:
+ if (!IS_ERR(msm_host->pclk))
+ clk_disable_unprepare(msm_host->pclk);
+bus_clk_disable:
+ if (!IS_ERR(msm_host->bus_clk))
+ clk_disable_unprepare(msm_host->bus_clk);
+pltfm_free:
+ sdhci_pltfm_free(pdev);
+ return ret;
+}
+
+static int sdhci_msm_remove(struct platform_device *pdev)
+{
+ struct sdhci_host *host = platform_get_drvdata(pdev);
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = pltfm_host->priv;
+ int dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) ==
+ 0xffffffff);
+
+ sdhci_remove_host(host, dead);
+ sdhci_pltfm_free(pdev);
+ sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
+ sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
+ clk_disable_unprepare(msm_host->clk);
+ clk_disable_unprepare(msm_host->pclk);
+ if (!IS_ERR(msm_host->bus_clk))
+ clk_disable_unprepare(msm_host->bus_clk);
+ return 0;
+}
+
+static struct platform_driver sdhci_msm_driver = {
+ .probe = sdhci_msm_probe,
+ .remove = sdhci_msm_remove,
+ .driver = {
+ .name = "sdhci_msm",
+ .owner = THIS_MODULE,
+ .of_match_table = sdhci_msm_dt_match,
+ },
+};
+
+module_platform_driver(sdhci_msm_driver);
+
+MODULE_DESCRIPTION("Qualcomm Secure Digital Host Controller Interface driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
2014-02-28 11:24 [PATCH v9 0/3] mmc: sdhci-msm: Add support for Qualcomm chipsets Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets Georgi Djakov
@ 2014-02-28 11:24 ` Georgi Djakov
2014-02-28 16:51 ` Kumar Gala
` (2 more replies)
2 siblings, 3 replies; 13+ messages in thread
From: Georgi Djakov @ 2014-02-28 11:24 UTC (permalink / raw)
To: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob
Cc: linux-doc, linux-kernel, linux-arm-msm, Georgi Djakov
This patch adds implementation for platform specific tuning in order to support
HS200 bus speed mode on Qualcomm SDHCI controller.
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
---
drivers/mmc/host/sdhci-msm.c | 424 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 418 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index b4490a2..69f6887 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -18,6 +18,8 @@
#include <linux/of_device.h>
#include <linux/regulator/consumer.h>
#include <linux/delay.h>
+#include <linux/mmc/mmc.h>
+#include <linux/slab.h>
#include "sdhci-pltfm.h"
@@ -43,7 +45,45 @@
#define CORE_PWRCTL_IO_FAIL BIT(3)
#define INT_MASK 0xf
+#define MAX_PHASES 16
+
+#define CORE_DLL_LOCK BIT(7)
+#define CORE_DLL_EN BIT(16)
+#define CORE_CDR_EN BIT(17)
+#define CORE_CK_OUT_EN BIT(18)
+#define CORE_CDR_EXT_EN BIT(19)
+#define CORE_DLL_PDN BIT(29)
+#define CORE_DLL_RST BIT(30)
+#define CORE_DLL_CONFIG 0x100
+#define CORE_DLL_TEST_CTL 0x104
+#define CORE_DLL_STATUS 0x108
+
+#define CORE_VENDOR_SPEC 0x10c
+#define CORE_CLK_PWRSAVE BIT(1)
+#define CORE_IO_PAD_PWR_SWITCH BIT(16)
+
+#define CDR_SELEXT_SHIFT 20
+#define CDR_SELEXT_MASK (0xf << CDR_SELEXT_SHIFT)
+#define CMUX_SHIFT_PHASE_SHIFT 24
+#define CMUX_SHIFT_PHASE_MASK (7 << CMUX_SHIFT_PHASE_SHIFT)
+
+static const u32 tuning_block_64[] = {
+ 0x00ff0fff, 0xccc3ccff, 0xffcc3cc3, 0xeffefffe,
+ 0xddffdfff, 0xfbfffbff, 0xff7fffbf, 0xefbdf777,
+ 0xf0fff0ff, 0x3cccfc0f, 0xcfcc33cc, 0xeeffefff,
+ 0xfdfffdff, 0xffbfffdf, 0xfff7ffbb, 0xde7b7ff7
+};
+static const u32 tuning_block_128[] = {
+ 0xff00ffff, 0x0000ffff, 0xccccffff, 0xcccc33cc,
+ 0xcc3333cc, 0xffffcccc, 0xffffeeff, 0xffeeeeff,
+ 0xffddffff, 0xddddffff, 0xbbffffff, 0xbbffffff,
+ 0xffffffbb, 0xffffff77, 0x77ff7777, 0xffeeddbb,
+ 0x00ffffff, 0x00ffffff, 0xccffff00, 0xcc33cccc,
+ 0x3333cccc, 0xffcccccc, 0xffeeffff, 0xeeeeffff,
+ 0xddffffff, 0xddffffff, 0xffffffdd, 0xffffffbb,
+ 0xffffbbbb, 0xffff77ff, 0xff7777ff, 0xeeddbb77
+};
/* This structure keeps information per regulator */
struct sdhci_msm_reg_data {
@@ -73,18 +113,390 @@ struct sdhci_msm_host {
struct sdhci_pltfm_data sdhci_msm_pdata;
};
-/* MSM platform specific tuning */
-int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
+/* Platform specific tuning */
+static inline int msm_dll_poll_ck_out_en(struct sdhci_host *host, u8 poll)
+{
+ u32 wait_cnt = 50;
+ u8 ck_out_en;
+ struct mmc_host *mmc = host->mmc;
+
+ /* Poll for CK_OUT_EN bit. max. poll time = 50us */
+ ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
+ CORE_CK_OUT_EN);
+
+ while (ck_out_en != poll) {
+ if (--wait_cnt == 0) {
+ dev_err(mmc_dev(mmc), "%s: CK_OUT_EN bit is not %d\n",
+ mmc_hostname(mmc), poll);
+ return -ETIMEDOUT;
+ }
+ udelay(1);
+
+ ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
+ CORE_CK_OUT_EN);
+ }
+
+ return 0;
+}
+
+static int msm_config_cm_dll_phase(struct sdhci_host *host, u8 phase)
+{
+ int rc;
+ static const u8 grey_coded_phase_table[] = {
+ 0x0, 0x1, 0x3, 0x2, 0x6, 0x7, 0x5, 0x4,
+ 0xc, 0xd, 0xf, 0xe, 0xa, 0xb, 0x9, 0x8
+ };
+ unsigned long flags;
+ u32 config;
+ struct mmc_host *mmc = host->mmc;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+ config &= ~(CORE_CDR_EN | CORE_CK_OUT_EN);
+ config |= (CORE_CDR_EXT_EN | CORE_DLL_EN);
+ writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '0' */
+ rc = msm_dll_poll_ck_out_en(host, 0);
+ if (rc)
+ goto err_out;
+
+ /*
+ * Write the selected DLL clock output phase (0 ... 15)
+ * to CDR_SELEXT bit field of DLL_CONFIG register.
+ */
+ config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+ config &= ~CDR_SELEXT_MASK;
+ config |= grey_coded_phase_table[phase] << CDR_SELEXT_SHIFT;
+ writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Set CK_OUT_EN bit of DLL_CONFIG register to 1. */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ | CORE_CK_OUT_EN), host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '1' */
+ rc = msm_dll_poll_ck_out_en(host, 1);
+ if (rc)
+ goto err_out;
+
+ config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+ config |= CORE_CDR_EN;
+ config &= ~CORE_CDR_EXT_EN;
+ writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+ goto out;
+
+err_out:
+ dev_err(mmc_dev(mmc), "%s: Failed to set DLL phase: %d\n",
+ mmc_hostname(mmc), phase);
+out:
+ spin_unlock_irqrestore(&host->lock, flags);
+ return rc;
+}
+
+/*
+ * Find out the greatest range of consecuitive selected
+ * DLL clock output phases that can be used as sampling
+ * setting for SD3.0 UHS-I card read operation (in SDR104
+ * timing mode) or for eMMC4.5 card read operation (in HS200
+ * timing mode).
+ * Select the 3/4 of the range and configure the DLL with the
+ * selected DLL clock output phase.
+ */
+
+static int msm_find_most_appropriate_phase(struct sdhci_host *host,
+ u8 *phase_table, u8 total_phases)
+{
+ int ret;
+ u8 ranges[MAX_PHASES][MAX_PHASES] = { {0}, {0} };
+ u8 phases_per_row[MAX_PHASES] = { 0 };
+ int row_index = 0, col_index = 0, selected_row_index = 0, curr_max = 0;
+ int i, cnt, phase_0_raw_index = 0, phase_15_raw_index = 0;
+ bool phase_0_found = false, phase_15_found = false;
+ struct mmc_host *mmc = host->mmc;
+
+ if (!total_phases || (total_phases > MAX_PHASES)) {
+ dev_err(mmc_dev(mmc), "%s: Invalid argument: total_phases=%d\n",
+ mmc_hostname(mmc), total_phases);
+ return -EINVAL;
+ }
+
+ for (cnt = 0; cnt < total_phases; cnt++) {
+ ranges[row_index][col_index] = phase_table[cnt];
+ phases_per_row[row_index] += 1;
+ col_index++;
+
+ if ((cnt + 1) == total_phases) {
+ continue;
+ /* check if next phase in phase_table is consecutive or not */
+ } else if ((phase_table[cnt] + 1) != phase_table[cnt + 1]) {
+ row_index++;
+ col_index = 0;
+ }
+ }
+
+ if (row_index >= MAX_PHASES)
+ return -EINVAL;
+
+ /* Check if phase-0 is present in first valid window? */
+ if (!ranges[0][0]) {
+ phase_0_found = true;
+ phase_0_raw_index = 0;
+ /* Check if cycle exist between 2 valid windows */
+ for (cnt = 1; cnt <= row_index; cnt++) {
+ if (phases_per_row[cnt]) {
+ for (i = 0; i < phases_per_row[cnt]; i++) {
+ if (ranges[cnt][i] == 15) {
+ phase_15_found = true;
+ phase_15_raw_index = cnt;
+ break;
+ }
+ }
+ }
+ }
+ }
+
+ /* If 2 valid windows form cycle then merge them as single window */
+ if (phase_0_found && phase_15_found) {
+ /* number of phases in raw where phase 0 is present */
+ u8 phases_0 = phases_per_row[phase_0_raw_index];
+ /* number of phases in raw where phase 15 is present */
+ u8 phases_15 = phases_per_row[phase_15_raw_index];
+
+ if (phases_0 + phases_15 >= MAX_PHASES)
+ /*
+ * If there are more than 1 phase windows then total
+ * number of phases in both the windows should not be
+ * more than or equal to MAX_PHASES.
+ */
+ return -EINVAL;
+
+ /* Merge 2 cyclic windows */
+ i = phases_15;
+ for (cnt = 0; cnt < phases_0; cnt++) {
+ ranges[phase_15_raw_index][i] =
+ ranges[phase_0_raw_index][cnt];
+ if (++i >= MAX_PHASES)
+ break;
+ }
+
+ phases_per_row[phase_0_raw_index] = 0;
+ phases_per_row[phase_15_raw_index] = phases_15 + phases_0;
+ }
+
+ for (cnt = 0; cnt <= row_index; cnt++) {
+ if (phases_per_row[cnt] > curr_max) {
+ curr_max = phases_per_row[cnt];
+ selected_row_index = cnt;
+ }
+ }
+
+ i = (curr_max * 3) / 4;
+ if (i)
+ i--;
+
+ ret = ranges[selected_row_index][i];
+
+ if (ret >= MAX_PHASES) {
+ ret = -EINVAL;
+ dev_err(mmc_dev(mmc), "%s: Invalid phase selected=%d\n",
+ mmc_hostname(mmc), ret);
+ }
+
+ return ret;
+}
+
+static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
+{
+ u32 mclk_freq = 0, config;
+
+ /* Program the MCLK value to MCLK_FREQ bit field */
+ if (host->clock <= 112000000)
+ mclk_freq = 0;
+ else if (host->clock <= 125000000)
+ mclk_freq = 1;
+ else if (host->clock <= 137000000)
+ mclk_freq = 2;
+ else if (host->clock <= 150000000)
+ mclk_freq = 3;
+ else if (host->clock <= 162000000)
+ mclk_freq = 4;
+ else if (host->clock <= 175000000)
+ mclk_freq = 5;
+ else if (host->clock <= 187000000)
+ mclk_freq = 6;
+ else if (host->clock <= 200000000)
+ mclk_freq = 7;
+
+ config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+ config &= ~CMUX_SHIFT_PHASE_MASK;
+ config |= mclk_freq << CMUX_SHIFT_PHASE_SHIFT;
+ writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+}
+
+/* Initialize the DLL (Programmable Delay Line) */
+static int msm_init_cm_dll(struct sdhci_host *host)
{
+ struct mmc_host *mmc = host->mmc;
+ int wait_cnt = 50;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
/*
- * Tuning is required for SDR104, HS200 and HS400 cards and if the clock
- * frequency greater than 100MHz in those modes. The standard tuning
- * procedure should not be executed, but a custom implementation will be
- * added here instead.
+ * Make sure that clock is always enabled when DLL
+ * tuning is in progress. Keeping PWRSAVE ON may
+ * turn off the clock.
*/
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC)
+ & ~CORE_CLK_PWRSAVE), host->ioaddr + CORE_VENDOR_SPEC);
+
+ /* Write 1 to DLL_RST bit of DLL_CONFIG register */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ | CORE_DLL_RST), host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Write 1 to DLL_PDN bit of DLL_CONFIG register */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ | CORE_DLL_PDN), host->ioaddr + CORE_DLL_CONFIG);
+ msm_cm_dll_set_freq(host);
+
+ /* Write 0 to DLL_RST bit of DLL_CONFIG register */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ & ~CORE_DLL_RST), host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Write 0 to DLL_PDN bit of DLL_CONFIG register */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ & ~CORE_DLL_PDN), host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Set DLL_EN bit to 1. */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ | CORE_DLL_EN), host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Set CK_OUT_EN bit to 1. */
+ writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
+ | CORE_CK_OUT_EN), host->ioaddr + CORE_DLL_CONFIG);
+
+ /* Wait until DLL_LOCK bit of DLL_STATUS register becomes '1' */
+ while (!(readl_relaxed(host->ioaddr + CORE_DLL_STATUS) &
+ CORE_DLL_LOCK)) {
+ /* max. wait for 50us sec for LOCK bit to be set */
+ if (--wait_cnt == 0) {
+ dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
+ mmc_hostname(mmc));
+ spin_unlock_irqrestore(&host->lock, flags);
+ return -ETIMEDOUT;
+ }
+ udelay(1);
+ }
+
+ spin_unlock_irqrestore(&host->lock, flags);
return 0;
}
+int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
+{
+ int tuning_seq_cnt = 3;
+ u8 phase, *data_buf, tuned_phases[16], tuned_phase_cnt = 0;
+ const u32 *tuning_block_pattern = tuning_block_64;
+ int size = sizeof(tuning_block_64); /* Pattern size in bytes */
+ int rc;
+ struct mmc_host *mmc = host->mmc;
+ struct mmc_ios ios = host->mmc->ios;
+
+ /*
+ * Tuning is required for SDR104, HS200 and HS400 cards and
+ * if clock frequency is greater than 100MHz in these modes.
+ */
+ if (host->clock <= 100 * 1000 * 1000 ||
+ !((ios.timing == MMC_TIMING_MMC_HS200) ||
+ (ios.timing == MMC_TIMING_UHS_SDR104)))
+ return 0;
+
+ if ((opcode == MMC_SEND_TUNING_BLOCK_HS200) &&
+ (mmc->ios.bus_width == MMC_BUS_WIDTH_8)) {
+ tuning_block_pattern = tuning_block_128;
+ size = sizeof(tuning_block_128);
+ }
+
+ data_buf = kmalloc(size, GFP_KERNEL);
+ if (!data_buf)
+ return -ENOMEM;
+
+retry:
+ /* First of all reset the tuning block */
+ rc = msm_init_cm_dll(host);
+ if (rc)
+ goto out;
+
+ phase = 0;
+ do {
+ struct mmc_command cmd = { 0 };
+ struct mmc_data data = { 0 };
+ struct mmc_request mrq = {
+ .cmd = &cmd,
+ .data = &data
+ };
+ struct scatterlist sg;
+
+ /* Set the phase in delay line hw block */
+ rc = msm_config_cm_dll_phase(host, phase);
+ if (rc)
+ goto out;
+
+ cmd.opcode = opcode;
+ cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+
+ data.blksz = size;
+ data.blocks = 1;
+ data.flags = MMC_DATA_READ;
+ data.timeout_ns = NSEC_PER_SEC; /* 1 second */
+
+ data.sg = &sg;
+ data.sg_len = 1;
+ sg_init_one(&sg, data_buf, sizeof(data_buf));
+ memset(data_buf, 0, sizeof(data_buf));
+ mmc_wait_for_req(mmc, &mrq);
+
+ if (!cmd.error && !data.error &&
+ !memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
+ /* Tuning is successful at this tuning point */
+ tuned_phases[tuned_phase_cnt++] = phase;
+ dev_dbg(mmc_dev(mmc), "%s: Found good phase = %d\n",
+ mmc_hostname(mmc), phase);
+ }
+ } while (++phase < ARRAY_SIZE(tuned_phases));
+
+ if (tuned_phase_cnt) {
+ rc = msm_find_most_appropriate_phase(host, tuned_phases,
+ tuned_phase_cnt);
+ if (rc < 0)
+ goto out;
+ else
+ phase = rc;
+
+ /*
+ * Finally set the selected phase in delay
+ * line hw block.
+ */
+ rc = msm_config_cm_dll_phase(host, phase);
+ if (rc)
+ goto out;
+ dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
+ mmc_hostname(mmc), phase);
+ } else {
+ if (--tuning_seq_cnt)
+ goto retry;
+ /* Tuning failed */
+ dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
+ mmc_hostname(mmc));
+ rc = -EIO;
+ }
+
+out:
+ kfree(data_buf);
+ return rc;
+}
+
#define MAX_PROP_SIZE 32
static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
struct sdhci_msm_reg_data *vreg, const char *vreg_name)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
2014-02-28 11:24 ` [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation Georgi Djakov
@ 2014-02-28 16:51 ` Kumar Gala
2014-02-28 22:10 ` Georgi Djakov
2014-02-28 20:45 ` Josh Cartwright
[not found] ` <1393586675-14628-4-git-send-email-gdjakov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
2 siblings, 1 reply; 13+ messages in thread
From: Kumar Gala @ 2014-02-28 16:51 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, rob,
linux-doc, linux-kernel, linux-arm-msm
On Feb 28, 2014, at 5:24 AM, Georgi Djakov <gdjakov@mm-sol.com> wrote:
> This patch adds implementation for platform specific tuning in order to support
> HS200 bus speed mode on Qualcomm SDHCI controller.
>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
> ---
> drivers/mmc/host/sdhci-msm.c | 424 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 418 insertions(+), 6 deletions(-)
minor nit, in subject (platform_execute_tuning, with one ’n’).
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
2014-02-28 11:24 ` [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation Georgi Djakov
2014-02-28 16:51 ` Kumar Gala
@ 2014-02-28 20:45 ` Josh Cartwright
[not found] ` <1393586675-14628-4-git-send-email-gdjakov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
2 siblings, 0 replies; 13+ messages in thread
From: Josh Cartwright @ 2014-02-28 20:45 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob,
linux-doc, linux-kernel, linux-arm-msm
Nit below.
On Fri, Feb 28, 2014 at 01:24:35PM +0200, Georgi Djakov wrote:
> This patch adds implementation for platform specific tuning in order to support
> HS200 bus speed mode on Qualcomm SDHCI controller.
>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
> ---
> drivers/mmc/host/sdhci-msm.c | 424 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 418 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b4490a2..69f6887 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
[..]
>
> +int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
This wants to be static.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
2014-02-28 11:24 ` [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation Georgi Djakov
@ 2014-02-28 20:51 ` Josh Cartwright
2014-02-28 20:45 ` Josh Cartwright
[not found] ` <1393586675-14628-4-git-send-email-gdjakov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
2 siblings, 0 replies; 13+ messages in thread
From: Josh Cartwright @ 2014-02-28 20:51 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA, cjb-2X9k7bc8m7Mdnm+yROfE0A,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, swarren-3lzwWm7+Weoh9ZMKESR00Q,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, rob-VoJi6FS/r0vR7s880joybQ,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
On Fri, Feb 28, 2014 at 01:24:35PM +0200, Georgi Djakov wrote:
> This patch adds implementation for platform specific tuning in order to support
> HS200 bus speed mode on Qualcomm SDHCI controller.
>
> Signed-off-by: Asutosh Das <asutoshd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Georgi Djakov <gdjakov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/mmc/host/sdhci-msm.c | 424 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 418 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b4490a2..69f6887 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
[..]
> +int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> + int tuning_seq_cnt = 3;
> + u8 phase, *data_buf, tuned_phases[16], tuned_phase_cnt = 0;
> + const u32 *tuning_block_pattern = tuning_block_64;
> + int size = sizeof(tuning_block_64); /* Pattern size in bytes */
> + int rc;
> + struct mmc_host *mmc = host->mmc;
> + struct mmc_ios ios = host->mmc->ios;
> +
> + /*
> + * Tuning is required for SDR104, HS200 and HS400 cards and
> + * if clock frequency is greater than 100MHz in these modes.
> + */
> + if (host->clock <= 100 * 1000 * 1000 ||
> + !((ios.timing == MMC_TIMING_MMC_HS200) ||
> + (ios.timing == MMC_TIMING_UHS_SDR104)))
> + return 0;
> +
> + if ((opcode == MMC_SEND_TUNING_BLOCK_HS200) &&
> + (mmc->ios.bus_width == MMC_BUS_WIDTH_8)) {
> + tuning_block_pattern = tuning_block_128;
> + size = sizeof(tuning_block_128);
> + }
> +
> + data_buf = kmalloc(size, GFP_KERNEL);
> + if (!data_buf)
> + return -ENOMEM;
> +
> +retry:
> + /* First of all reset the tuning block */
> + rc = msm_init_cm_dll(host);
> + if (rc)
> + goto out;
> +
> + phase = 0;
> + do {
> + struct mmc_command cmd = { 0 };
> + struct mmc_data data = { 0 };
> + struct mmc_request mrq = {
> + .cmd = &cmd,
> + .data = &data
> + };
> + struct scatterlist sg;
> +
> + /* Set the phase in delay line hw block */
> + rc = msm_config_cm_dll_phase(host, phase);
> + if (rc)
> + goto out;
> +
> + cmd.opcode = opcode;
> + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> + data.blksz = size;
> + data.blocks = 1;
> + data.flags = MMC_DATA_READ;
> + data.timeout_ns = NSEC_PER_SEC; /* 1 second */
> +
> + data.sg = &sg;
> + data.sg_len = 1;
> + sg_init_one(&sg, data_buf, sizeof(data_buf));
> + memset(data_buf, 0, sizeof(data_buf));
> + mmc_wait_for_req(mmc, &mrq);
> +
> + if (!cmd.error && !data.error &&
> + !memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
This memcmp is broken, sizeof(data_buf) is likely not what you want,
maybe you want 'size'? Same thing for sg_init_one()/memset() above.
From sparse:
drivers/mmc/host/sdhci-msm.c: In function ‘sdhci_msm_execute_tuning’:
drivers/mmc/host/sdhci-msm.c:461:53: warning: argument to ‘sizeof’ in ‘memcmp’ call is the same expression as the first source; did you mean to provide an explicit length? [-Wsizeof-pointer-memaccess]
!memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
@ 2014-02-28 20:51 ` Josh Cartwright
0 siblings, 0 replies; 13+ messages in thread
From: Josh Cartwright @ 2014-02-28 20:51 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob,
linux-doc, linux-kernel, linux-arm-msm
On Fri, Feb 28, 2014 at 01:24:35PM +0200, Georgi Djakov wrote:
> This patch adds implementation for platform specific tuning in order to support
> HS200 bus speed mode on Qualcomm SDHCI controller.
>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
> ---
> drivers/mmc/host/sdhci-msm.c | 424 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 418 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b4490a2..69f6887 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
[..]
> +int sdhci_msm_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> + int tuning_seq_cnt = 3;
> + u8 phase, *data_buf, tuned_phases[16], tuned_phase_cnt = 0;
> + const u32 *tuning_block_pattern = tuning_block_64;
> + int size = sizeof(tuning_block_64); /* Pattern size in bytes */
> + int rc;
> + struct mmc_host *mmc = host->mmc;
> + struct mmc_ios ios = host->mmc->ios;
> +
> + /*
> + * Tuning is required for SDR104, HS200 and HS400 cards and
> + * if clock frequency is greater than 100MHz in these modes.
> + */
> + if (host->clock <= 100 * 1000 * 1000 ||
> + !((ios.timing == MMC_TIMING_MMC_HS200) ||
> + (ios.timing == MMC_TIMING_UHS_SDR104)))
> + return 0;
> +
> + if ((opcode == MMC_SEND_TUNING_BLOCK_HS200) &&
> + (mmc->ios.bus_width == MMC_BUS_WIDTH_8)) {
> + tuning_block_pattern = tuning_block_128;
> + size = sizeof(tuning_block_128);
> + }
> +
> + data_buf = kmalloc(size, GFP_KERNEL);
> + if (!data_buf)
> + return -ENOMEM;
> +
> +retry:
> + /* First of all reset the tuning block */
> + rc = msm_init_cm_dll(host);
> + if (rc)
> + goto out;
> +
> + phase = 0;
> + do {
> + struct mmc_command cmd = { 0 };
> + struct mmc_data data = { 0 };
> + struct mmc_request mrq = {
> + .cmd = &cmd,
> + .data = &data
> + };
> + struct scatterlist sg;
> +
> + /* Set the phase in delay line hw block */
> + rc = msm_config_cm_dll_phase(host, phase);
> + if (rc)
> + goto out;
> +
> + cmd.opcode = opcode;
> + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> + data.blksz = size;
> + data.blocks = 1;
> + data.flags = MMC_DATA_READ;
> + data.timeout_ns = NSEC_PER_SEC; /* 1 second */
> +
> + data.sg = &sg;
> + data.sg_len = 1;
> + sg_init_one(&sg, data_buf, sizeof(data_buf));
> + memset(data_buf, 0, sizeof(data_buf));
> + mmc_wait_for_req(mmc, &mrq);
> +
> + if (!cmd.error && !data.error &&
> + !memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
This memcmp is broken, sizeof(data_buf) is likely not what you want,
maybe you want 'size'? Same thing for sg_init_one()/memset() above.
>From sparse:
drivers/mmc/host/sdhci-msm.c: In function ‘sdhci_msm_execute_tuning’:
drivers/mmc/host/sdhci-msm.c:461:53: warning: argument to ‘sizeof’ in ‘memcmp’ call is the same expression as the first source; did you mean to provide an explicit length? [-Wsizeof-pointer-memaccess]
!memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
2014-02-28 16:51 ` Kumar Gala
@ 2014-02-28 22:10 ` Georgi Djakov
0 siblings, 0 replies; 13+ messages in thread
From: Georgi Djakov @ 2014-02-28 22:10 UTC (permalink / raw)
To: Kumar Gala
Cc: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, rob,
linux-doc, linux-kernel, linux-arm-msm
On 28.02.14, 18:51, Kumar Gala wrote:
>
> On Feb 28, 2014, at 5:24 AM, Georgi Djakov <gdjakov@mm-sol.com> wrote:
>
>> This patch adds implementation for platform specific tuning in order to support
>> HS200 bus speed mode on Qualcomm SDHCI controller.
>>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
>> ---
>> drivers/mmc/host/sdhci-msm.c | 424 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 418 insertions(+), 6 deletions(-)
>
> minor nit, in subject (platform_execute_tuning, with one ’n’).
>
> - k
Oh, of course! Thanks!
BR,
Georgi
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation
2014-02-28 20:51 ` Josh Cartwright
(?)
@ 2014-02-28 22:29 ` Georgi Djakov
-1 siblings, 0 replies; 13+ messages in thread
From: Georgi Djakov @ 2014-02-28 22:29 UTC (permalink / raw)
To: Josh Cartwright
Cc: linux-mmc, cjb, ulf.hansson, devicetree, grant.likely, robh+dt,
pawel.moll, mark.rutland, swarren, ijc+devicetree, galak, rob,
linux-doc, linux-kernel, linux-arm-msm
On 28.02.14, 22:51, Josh Cartwright wrote:
[..]
>> + sg_init_one(&sg, data_buf, sizeof(data_buf));
>> + memset(data_buf, 0, sizeof(data_buf));
>> + mmc_wait_for_req(mmc, &mrq);
>> +
>> + if (!cmd.error && !data.error &&
>> + !memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
>
> This memcmp is broken, sizeof(data_buf) is likely not what you want,
> maybe you want 'size'? Same thing for sg_init_one()/memset() above.
>
> From sparse:
>
> drivers/mmc/host/sdhci-msm.c: In function ‘sdhci_msm_execute_tuning’:
> drivers/mmc/host/sdhci-msm.c:461:53: warning: argument to ‘sizeof’ in ‘memcmp’ call is the same expression as the first source; did you mean to provide an explicit length? [-Wsizeof-pointer-memaccess]
> !memcmp(data_buf, tuning_block_pattern, sizeof(data_buf))) {
>
Nice catch, Josh! Thanks for reviewing!
BR,
Georgi
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation
2014-02-28 11:24 ` [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation Georgi Djakov
@ 2014-03-03 2:03 ` Bjorn Andersson
0 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2014-03-03 2:03 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-mmc, Chris Ball, ulf.hansson, devicetree, Grant Likely,
Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren,
Ian Campbell, Kumar Gala, Rob Landley, linux-doc, linux-kernel,
linux-arm-msm
On Fri, Feb 28, 2014 at 3:24 AM, Georgi Djakov <gdjakov@mm-sol.com> wrote:
> This platform driver adds the initial support of Secure
> Digital Host Controller Interface compliant controller
> found in Qualcomm chipsets.
>
Hi Georgi,
When testing this I was confused by the warnings from sdhci not finding vmmc
and vqmmc. Is the power irq something Qualcomm specific or is there any other
reason why the sdhci provided regulator functionality can't be used?
Regarding the usage of the regulator api here, I think you should call
regulator_set_voltage() with your default voltage when you acquire the
regulator handles; then your power enable/disable functions will be simpler and
you should be able to clean up the power irq function further.
>
[...]
> +/* This structure keeps information per regulator */
> +struct sdhci_msm_reg_data {
> + struct regulator *reg;
> + const char *name;
> + /* Voltage level values */
> + u32 low_vol_level;
> + u32 high_vol_level;
Is there a reason why these should be different? In your example and the other
cases I've seen they are always 2.95V and 1.8V.
>
[...]
> +
> +static int sdhci_msm_vreg_enable(struct device *dev,
> + struct sdhci_msm_reg_data *vreg)
> +{
> + int ret = 0;
> +
> + if (!regulator_is_enabled(vreg->reg)) {
> + /* Set voltage level */
> + ret = regulator_set_voltage(vreg->reg, vreg->high_vol_level,
> + vreg->high_vol_level);
> + if (ret)
> + return ret;
So when you enable voltage in the irq handler or in probe, you will go to "high
voltage", then you might lower this directly again.
> + }
> +
> + ret = regulator_enable(vreg->reg);
> + if (ret) {
> + dev_err(dev, "Failed to enable regulator %s (%d)\n",
> + vreg->name, ret);
> + }
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_vreg_disable(struct device *dev,
> + struct sdhci_msm_reg_data *vreg)
> +{
> + int ret = 0;
> +
> + if (!regulator_is_enabled(vreg->reg))
> + return ret;
> +
> + /* Set min. voltage to 0 */
> + ret = regulator_set_voltage(vreg->reg, 0, vreg->high_vol_level);
> + if (ret)
> + return ret;
Why do you set the voltage to 0 here?
> +
> + ret = regulator_disable(vreg->reg);
> + if (ret) {
> + dev_err(dev, "Failed to disable regulator %s (%d)\n",
> + vreg->name, ret);
> + }
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_setup_vreg(struct sdhci_msm_host *msm_host, bool enable)
> +{
Instead of having a function with one big if statement of which path you came
from you should have two functions for this.
> + int ret, i;
> + struct sdhci_msm_reg_data *vreg_table[2];
> +
> + vreg_table[0] = &msm_host->pdata.vdd;
> + vreg_table[1] = &msm_host->pdata.vdd_io;
> +
> + for (i = 0; i < ARRAY_SIZE(vreg_table); i++) {
> + if (enable)
> + ret = sdhci_msm_vreg_enable(&msm_host->pdev->dev,
> + vreg_table[i]);
> + else
> + ret = sdhci_msm_vreg_disable(&msm_host->pdev->dev,
> + vreg_table[i]);
> + if (ret)
> + return ret;
> + }
This seems to a complicated way of saying:
if (enable) {
sdhci_msm_vreg_enable(vdd)
sdhci_msm_vreg_enable(vdd_io)
} else {
sdhci_msm_vreg_disable(vdd)
sdhci_msm_vreg_disable(vdd_io)
}
Do you plan to add more regulators here?
> +
> + return 0;
> +}
> +
> +static int sdhci_msm_vreg_init(struct device *dev,
> + struct sdhci_msm_pltfm_data *pdata)
> +{
> + struct sdhci_msm_reg_data *vdd_reg = &pdata->vdd;
> + struct sdhci_msm_reg_data *vdd_io_reg = &pdata->vdd_io;
> +
> + vdd_reg->reg = devm_regulator_get(dev, vdd_reg->name);
> + if (IS_ERR(vdd_reg->reg))
> + return PTR_ERR(vdd_reg->reg);
> +
> + vdd_io_reg->reg = devm_regulator_get(dev, vdd_io_reg->name);
> + if (IS_ERR(vdd_io_reg->reg))
> + return PTR_ERR(vdd_io_reg->reg);
> +
> + return 0;
> +}
> +
> +static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
> +{
> + struct sdhci_host *host = (struct sdhci_host *)data;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = pltfm_host->priv;
> + u8 irq_status;
> + u8 irq_ack = 0;
> + int ret = 0;
> +
> + irq_status = readb_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> + dev_dbg(mmc_dev(msm_host->mmc), "%s: Received IRQ(%d), status=0x%x\n",
> + mmc_hostname(msm_host->mmc), irq, irq_status);
> +
> + /* Clear the interrupt */
> + writeb_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
> + /*
> + * SDHC has core_mem and hc_mem device memory and these memory
> + * addresses do not fall within 1KB region. Hence, any update to
> + * core_mem address space would require an mb() to ensure this gets
> + * completed before its next update to registers within hc_mem.
> + */
This is the standard Qualcomm disclaimer regarding memory barriers, but what
part of the system does actually touch hc_mem? As far as I can see this driver
does not go outside that 1K and if the core sdhci core does, it seems to all be
using the non-relaxed write*; so there would be an implicit sync there.
Is it so that we make sure to clear the interrupt here and now?
> + mb();
> +
> + /* Handle BUS ON/OFF */
> + if (irq_status & CORE_PWRCTL_BUS_ON) {
> + ret = sdhci_msm_setup_vreg(msm_host, true);
> + if (!ret)
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.high_vol_level,
> + msm_host->pdata.
> + vdd_io.high_vol_level);
If sdhci_msm_setup_vreg succeeds, you've already set a voltage to vdd_io and
enabled it, why do this one more time?
> + if (ret)
> + irq_ack |= CORE_PWRCTL_BUS_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> + }
> +
> + if (irq_status & CORE_PWRCTL_BUS_OFF) {
> + ret = sdhci_msm_setup_vreg(msm_host, false);
> + if (!ret)
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.low_vol_level,
> + msm_host->pdata.
> + vdd_io.low_vol_level);
Same here.
> + if (ret)
> + irq_ack |= CORE_PWRCTL_BUS_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> + }
> +
> + /* Handle IO LOW/HIGH */
> + if (irq_status & CORE_PWRCTL_IO_LOW) {
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.low_vol_level,
> + msm_host->pdata.
> + vdd_io.low_vol_level);
I assume that LOW is xor HIGH here, or you sould set it low then high.
May I suggest that you restructure this to first figuring out what new voltage
(if any) you're aiming for and then call regulator_set_voltage(vdd_io) once and
based on that update the IO_{SUCCESS,FAIL} bits of irq_ack.
> + if (ret)
> + irq_ack |= CORE_PWRCTL_IO_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> + }
> +
> + if (irq_status & CORE_PWRCTL_IO_HIGH) {
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.high_vol_level,
> + msm_host->pdata.
> + vdd_io.high_vol_level);
> + if (ret)
> + irq_ack |= CORE_PWRCTL_IO_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> + }
> +
> + /* ACK status to the core */
> + writeb_relaxed(irq_ack, (msm_host->core_mem + CORE_PWRCTL_CTL));
> + /*
> + * SDHC has core_mem and hc_mem device memory and these memory
> + * addresses do not fall within 1KB region. Hence, any update to
> + * core_mem address space would require an mb() to ensure this gets
> + * completed before its next update to registers within hc_mem.
> + */
Like above, is this mb() to guard for re-ordering or to commit the write?
> + mb();
> +
> + dev_dbg(mmc_dev(msm_host->mmc), "%s: Handled IRQ(%d), ret=%d, ack=0x%x\n",
> + mmc_hostname(msm_host->mmc), irq, ret, irq_ack);
> + return IRQ_HANDLED;
> +}
> +
> +static const struct of_device_id sdhci_msm_dt_match[] = {
> + { .compatible = "qcom,sdhci-msm-v4" },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
> +
> +static struct sdhci_ops sdhci_msm_ops = {
> + .platform_execute_tuning = sdhci_msm_execute_tuning,
> +};
> +
> +static int sdhci_msm_probe(struct platform_device *pdev)
> +{
> + struct sdhci_host *host;
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_msm_host *msm_host;
> + struct resource *core_memres = NULL;
No need to initialize, as first reference is an assignment.
> + int ret, dead;
> + u16 host_version;
> +
> + if (!pdev->dev.of_node) {
> + dev_err(&pdev->dev, "No device tree data\n");
> + return -ENOENT;
> + }
> +
> + msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
> + if (!msm_host)
> + return -ENOMEM;
> +
> + msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
> + host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);
> + pltfm_host->priv = msm_host;
> + msm_host->mmc = host->mmc;
> + msm_host->pdev = pdev;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed parsing mmc device tree\n");
> + goto pltfm_free;
> + }
> +
> + sdhci_get_of_property(pdev);
> +
> + ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
> + if (ret) {
> + dev_err(&pdev->dev, "DT parsing error\n");
> + goto pltfm_free;
> + }
> +
> + /* Setup SDCC bus voter clock. */
> + msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + if (!IS_ERR(msm_host->bus_clk)) {
> + /* Vote for max. clk rate for max. performance */
> + ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
> + if (ret)
> + goto pltfm_free;
> + ret = clk_prepare_enable(msm_host->bus_clk);
> + if (ret)
> + goto pltfm_free;
> + }
> +
> + /* Setup main peripheral bus clock */
> + msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
> + if (!IS_ERR(msm_host->pclk)) {
iface clock is marked required in the binding documentation, so you probably
don't want to fall through here on error.
> + ret = clk_prepare_enable(msm_host->pclk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Peripheral clock setup failed (%d)\n", ret);
> + goto bus_clk_disable;
> + }
> + }
> +
> + /* Setup SDC MMC clock */
> + msm_host->clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(msm_host->clk)) {
> + ret = PTR_ERR(msm_host->clk);
> + dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
> + goto pclk_disable;
> + }
> +
> + ret = clk_prepare_enable(msm_host->clk);
> + if (ret)
> + goto pclk_disable;
> +
> + /* Setup regulators */
> + ret = sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev,
> + "Regulator setup failed (%d)\n", ret);
> + goto clk_disable;
> + }
> +
> + core_memres = platform_get_resource_byname(pdev,
> + IORESOURCE_MEM, "core_mem");
> + msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
> +
> + if (IS_ERR(msm_host->core_mem)) {
> + dev_err(&pdev->dev, "Failed to remap registers\n");
> + ret = PTR_ERR(msm_host->core_mem);
> + goto vreg_disable;
> + }
> +
> + /* Reset the core and Enable SDHC mode */
> + writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
> + CORE_SW_RST, msm_host->core_mem + CORE_POWER);
> +
> + /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
> + usleep_range(1000, 5000);
> + if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
> + dev_err(&pdev->dev, "Stuck in reset\n");
> + ret = -ETIMEDOUT;
> + goto vreg_disable;
At this point you have only acquired a handle to the vregs, you have not
enabled them. So you don't need to disable them.
> + }
> +
> + /* Set HC_MODE_EN bit in HC_MODE register */
> + writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
> +
> + /*
> + * Following are the deviations from SDHC spec v3.0 -
> + * 1. Card detection is handled using separate GPIO.
> + * 2. Bus power control is handled by interacting with PMIC.
> + */
> + host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> + host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;
> +
> + host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> + dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
> + host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> + SDHCI_VENDOR_VER_SHIFT));
> +
> + /* Setup PWRCTL irq */
> + msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
> + if (msm_host->pwr_irq < 0) {
> + dev_err(&pdev->dev, "Failed to get pwr_irq by name (%d)\n",
> + msm_host->pwr_irq);
> + goto vreg_disable;
At this point you have only acquired a handle to the vregs, you have not
enabled them. So you don't need to disable them.
> + }
> + ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
> + sdhci_msm_pwr_irq, IRQF_ONESHOT,
> + dev_name(&pdev->dev), host);
> + if (ret) {
> + dev_err(&pdev->dev, "Request threaded irq(%d) failed (%d)\n",
> + msm_host->pwr_irq, ret);
> + goto vreg_disable;
If this fails you haven't enabled the regulators, so no need to disable them
again.
> + }
> +
> + /* Enable pwr irq interrupts */
> + writel_relaxed(INT_MASK, (msm_host->core_mem + CORE_PWRCTL_MASK));
> +
> + msm_host->mmc->caps |= msm_host->pdata.caps;
> + msm_host->mmc->caps2 |= msm_host->pdata.caps2;
> +
> + ret = sdhci_add_host(host);
> + if (ret) {
> + dev_err(&pdev->dev, "Add host failed (%d)\n", ret);
> + goto vreg_disable;
> + }
> +
> + ret = clk_set_rate(msm_host->clk, host->max_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "MClk rate set failed (%d)\n", ret);
> + goto remove_host;
> + }
Why do you enable the clk further up in this function but wait with setting the
rate until the last thing in this function?
> +
> + return 0;
> +
> +remove_host:
> + dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
> + sdhci_remove_host(host, dead);
> +vreg_disable:
> + if (!IS_ERR(msm_host->pdata.vdd.reg))
If IS_ERR(vdd) or IS_ERR(vdd_io) then you would end up in clk_disable.
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
> + if (!IS_ERR(msm_host->pdata.vdd_io.reg))
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
> +clk_disable:
> + if (!IS_ERR(msm_host->clk))
If IS_ERR(clk) then you would end up in pclk_disable.
> + clk_disable_unprepare(msm_host->clk);
> +pclk_disable:
> + if (!IS_ERR(msm_host->pclk))
Based on the assumption that the check for errors on pclk above is incorrect,
then you would end up in bus_clk_disable if IS_ERR(pclk).
> + clk_disable_unprepare(msm_host->pclk);
> +bus_clk_disable:
> + if (!IS_ERR(msm_host->bus_clk))
bus_clk might be IS_ERR() as it's optional, so this makes sense.
> + clk_disable_unprepare(msm_host->bus_clk);
> +pltfm_free:
> + sdhci_pltfm_free(pdev);
> + return ret;
> +}
> +
> +static int sdhci_msm_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = pltfm_host->priv;
> + int dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) ==
> + 0xffffffff);
> +
You should probably start with disabling the pwr_irq here, to make sure that it
doesn't kick in after you starting to free resources.
> + sdhci_remove_host(host, dead);
> + sdhci_pltfm_free(pdev);
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
> + clk_disable_unprepare(msm_host->clk);
> + clk_disable_unprepare(msm_host->pclk);
> + if (!IS_ERR(msm_host->bus_clk))
> + clk_disable_unprepare(msm_host->bus_clk);
> + return 0;
> +}
> +
Regards,
Bjorn
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets
2014-02-28 11:24 ` [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets Georgi Djakov
@ 2014-03-04 3:15 ` Bjorn Andersson
2014-03-04 17:50 ` Georgi Djakov
0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2014-03-04 3:15 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-mmc, Chris Ball, ulf.hansson, devicetree, Grant Likely,
Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren,
Ian Campbell, Kumar Gala, Rob Landley, linux-doc, linux-kernel,
linux-arm-msm
On Fri, Feb 28, 2014 at 3:24 AM, Georgi Djakov <gdjakov@mm-sol.com> wrote:
> This platform driver adds the initial support of Secure
> Digital Host Controller Interface compliant controller
> found in Qualcomm chipsets.
>
Hi Georgi,
Sorry for reposting this, I have no idea how I managed to send this as an answer
to patch 1/3...
When testing this I was confused by the warnings from sdhci not finding vmmc
and vqmmc. Is the power irq something Qualcomm specific or is there any other
reason why the sdhci provided regulator functionality can't be used?
Regarding the usage of the regulator api here, I think you should call
regulator_set_voltage() with your default voltage when you acquire the
regulator handles; then your power enable/disable functions will be simpler and
you should be able to clean up the power irq function further.
>
[...]
> +/* This structure keeps information per regulator */
> +struct sdhci_msm_reg_data {
> + struct regulator *reg;
> + const char *name;
> + /* Voltage level values */
> + u32 low_vol_level;
> + u32 high_vol_level;
Is there a reason why these should be different? In your example and the other
cases I've seen they are always 2.95V and 1.8V.
>
[...]
> +
> +static int sdhci_msm_vreg_enable(struct device *dev,
> + struct sdhci_msm_reg_data *vreg)
> +{
> + int ret = 0;
> +
> + if (!regulator_is_enabled(vreg->reg)) {
> + /* Set voltage level */
> + ret = regulator_set_voltage(vreg->reg, vreg->high_vol_level,
> + vreg->high_vol_level);
> + if (ret)
> + return ret;
So when you enable voltage in the irq handler or in probe, you will go to "high
voltage", then you might lower this directly again.
> + }
> +
> + ret = regulator_enable(vreg->reg);
> + if (ret) {
> + dev_err(dev, "Failed to enable regulator %s (%d)\n",
> + vreg->name, ret);
> + }
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_vreg_disable(struct device *dev,
> + struct sdhci_msm_reg_data *vreg)
> +{
> + int ret = 0;
> +
> + if (!regulator_is_enabled(vreg->reg))
> + return ret;
> +
> + /* Set min. voltage to 0 */
> + ret = regulator_set_voltage(vreg->reg, 0, vreg->high_vol_level);
> + if (ret)
> + return ret;
Why do you set the voltage to 0 here?
> +
> + ret = regulator_disable(vreg->reg);
> + if (ret) {
> + dev_err(dev, "Failed to disable regulator %s (%d)\n",
> + vreg->name, ret);
> + }
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_setup_vreg(struct sdhci_msm_host *msm_host, bool enable)
> +{
Instead of having a function with one big if statement of which path you came
from you should have two functions for this.
> + int ret, i;
> + struct sdhci_msm_reg_data *vreg_table[2];
> +
> + vreg_table[0] = &msm_host->pdata.vdd;
> + vreg_table[1] = &msm_host->pdata.vdd_io;
> +
> + for (i = 0; i < ARRAY_SIZE(vreg_table); i++) {
> + if (enable)
> + ret = sdhci_msm_vreg_enable(&msm_host->pdev->dev,
> + vreg_table[i]);
> + else
> + ret = sdhci_msm_vreg_disable(&msm_host->pdev->dev,
> + vreg_table[i]);
> + if (ret)
> + return ret;
> + }
This seems to a complicated way of saying:
if (enable) {
sdhci_msm_vreg_enable(vdd)
sdhci_msm_vreg_enable(vdd_io)
} else {
sdhci_msm_vreg_disable(vdd)
sdhci_msm_vreg_disable(vdd_io)
}
Do you plan to add more regulators here?
> +
> + return 0;
> +}
> +
> +static int sdhci_msm_vreg_init(struct device *dev,
> + struct sdhci_msm_pltfm_data *pdata)
> +{
> + struct sdhci_msm_reg_data *vdd_reg = &pdata->vdd;
> + struct sdhci_msm_reg_data *vdd_io_reg = &pdata->vdd_io;
> +
> + vdd_reg->reg = devm_regulator_get(dev, vdd_reg->name);
> + if (IS_ERR(vdd_reg->reg))
> + return PTR_ERR(vdd_reg->reg);
> +
> + vdd_io_reg->reg = devm_regulator_get(dev, vdd_io_reg->name);
> + if (IS_ERR(vdd_io_reg->reg))
> + return PTR_ERR(vdd_io_reg->reg);
> +
> + return 0;
> +}
> +
> +static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
> +{
> + struct sdhci_host *host = (struct sdhci_host *)data;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = pltfm_host->priv;
> + u8 irq_status;
> + u8 irq_ack = 0;
> + int ret = 0;
> +
> + irq_status = readb_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> + dev_dbg(mmc_dev(msm_host->mmc), "%s: Received IRQ(%d), status=0x%x\n",
> + mmc_hostname(msm_host->mmc), irq, irq_status);
> +
> + /* Clear the interrupt */
> + writeb_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
> + /*
> + * SDHC has core_mem and hc_mem device memory and these memory
> + * addresses do not fall within 1KB region. Hence, any update to
> + * core_mem address space would require an mb() to ensure this gets
> + * completed before its next update to registers within hc_mem.
> + */
This is the standard Qualcomm disclaimer regarding memory barriers, but what
part of the system does actually touch hc_mem? As far as I can see this driver
does not go outside that 1K and if the core sdhci core does, it seems to all be
using the non-relaxed write*; so there would be an implicit sync there.
Is it so that we make sure to clear the interrupt here and now?
> + mb();
> +
> + /* Handle BUS ON/OFF */
> + if (irq_status & CORE_PWRCTL_BUS_ON) {
> + ret = sdhci_msm_setup_vreg(msm_host, true);
> + if (!ret)
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.high_vol_level,
> + msm_host->pdata.
> + vdd_io.high_vol_level);
If sdhci_msm_setup_vreg succeeds, you've already set a voltage to vdd_io and
enabled it, why do this one more time?
> + if (ret)
> + irq_ack |= CORE_PWRCTL_BUS_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> + }
> +
> + if (irq_status & CORE_PWRCTL_BUS_OFF) {
> + ret = sdhci_msm_setup_vreg(msm_host, false);
> + if (!ret)
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.low_vol_level,
> + msm_host->pdata.
> + vdd_io.low_vol_level);
Same here.
> + if (ret)
> + irq_ack |= CORE_PWRCTL_BUS_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> + }
> +
> + /* Handle IO LOW/HIGH */
> + if (irq_status & CORE_PWRCTL_IO_LOW) {
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.low_vol_level,
> + msm_host->pdata.
> + vdd_io.low_vol_level);
I assume that LOW is xor HIGH here, or you sould set it low then high.
May I suggest that you restructure this to first figuring out what new voltage
(if any) you're aiming for and then call regulator_set_voltage(vdd_io) once and
based on that update the IO_{SUCCESS,FAIL} bits of irq_ack.
> + if (ret)
> + irq_ack |= CORE_PWRCTL_IO_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> + }
> +
> + if (irq_status & CORE_PWRCTL_IO_HIGH) {
> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
> + msm_host->pdata.
> + vdd_io.high_vol_level,
> + msm_host->pdata.
> + vdd_io.high_vol_level);
> + if (ret)
> + irq_ack |= CORE_PWRCTL_IO_FAIL;
> + else
> + irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> + }
> +
> + /* ACK status to the core */
> + writeb_relaxed(irq_ack, (msm_host->core_mem + CORE_PWRCTL_CTL));
> + /*
> + * SDHC has core_mem and hc_mem device memory and these memory
> + * addresses do not fall within 1KB region. Hence, any update to
> + * core_mem address space would require an mb() to ensure this gets
> + * completed before its next update to registers within hc_mem.
> + */
Like above, is this mb() to guard for re-ordering or to commit the write?
> + mb();
> +
> + dev_dbg(mmc_dev(msm_host->mmc), "%s: Handled IRQ(%d), ret=%d, ack=0x%x\n",
> + mmc_hostname(msm_host->mmc), irq, ret, irq_ack);
> + return IRQ_HANDLED;
> +}
> +
> +static const struct of_device_id sdhci_msm_dt_match[] = {
> + { .compatible = "qcom,sdhci-msm-v4" },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
> +
> +static struct sdhci_ops sdhci_msm_ops = {
> + .platform_execute_tuning = sdhci_msm_execute_tuning,
> +};
> +
> +static int sdhci_msm_probe(struct platform_device *pdev)
> +{
> + struct sdhci_host *host;
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_msm_host *msm_host;
> + struct resource *core_memres = NULL;
No need to initialize, as first reference is an assignment.
> + int ret, dead;
> + u16 host_version;
> +
> + if (!pdev->dev.of_node) {
> + dev_err(&pdev->dev, "No device tree data\n");
> + return -ENOENT;
> + }
> +
> + msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
> + if (!msm_host)
> + return -ENOMEM;
> +
> + msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
> + host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);
> + pltfm_host->priv = msm_host;
> + msm_host->mmc = host->mmc;
> + msm_host->pdev = pdev;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed parsing mmc device tree\n");
> + goto pltfm_free;
> + }
> +
> + sdhci_get_of_property(pdev);
> +
> + ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
> + if (ret) {
> + dev_err(&pdev->dev, "DT parsing error\n");
> + goto pltfm_free;
> + }
> +
> + /* Setup SDCC bus voter clock. */
> + msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + if (!IS_ERR(msm_host->bus_clk)) {
> + /* Vote for max. clk rate for max. performance */
> + ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
> + if (ret)
> + goto pltfm_free;
> + ret = clk_prepare_enable(msm_host->bus_clk);
> + if (ret)
> + goto pltfm_free;
> + }
> +
> + /* Setup main peripheral bus clock */
> + msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
> + if (!IS_ERR(msm_host->pclk)) {
iface clock is marked required in the binding documentation, so you probably
don't want to fall through here on error.
> + ret = clk_prepare_enable(msm_host->pclk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Peripheral clock setup failed (%d)\n", ret);
> + goto bus_clk_disable;
> + }
> + }
> +
> + /* Setup SDC MMC clock */
> + msm_host->clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(msm_host->clk)) {
> + ret = PTR_ERR(msm_host->clk);
> + dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
> + goto pclk_disable;
> + }
> +
> + ret = clk_prepare_enable(msm_host->clk);
> + if (ret)
> + goto pclk_disable;
> +
> + /* Setup regulators */
> + ret = sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev,
> + "Regulator setup failed (%d)\n", ret);
> + goto clk_disable;
> + }
> +
> + core_memres = platform_get_resource_byname(pdev,
> + IORESOURCE_MEM, "core_mem");
> + msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
> +
> + if (IS_ERR(msm_host->core_mem)) {
> + dev_err(&pdev->dev, "Failed to remap registers\n");
> + ret = PTR_ERR(msm_host->core_mem);
> + goto vreg_disable;
> + }
> +
> + /* Reset the core and Enable SDHC mode */
> + writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
> + CORE_SW_RST, msm_host->core_mem + CORE_POWER);
> +
> + /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
> + usleep_range(1000, 5000);
> + if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
> + dev_err(&pdev->dev, "Stuck in reset\n");
> + ret = -ETIMEDOUT;
> + goto vreg_disable;
At this point you have only acquired a handle to the vregs, you have not
enabled them. So you don't need to disable them.
> + }
> +
> + /* Set HC_MODE_EN bit in HC_MODE register */
> + writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
> +
> + /*
> + * Following are the deviations from SDHC spec v3.0 -
> + * 1. Card detection is handled using separate GPIO.
> + * 2. Bus power control is handled by interacting with PMIC.
> + */
> + host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> + host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;
> +
> + host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> + dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
> + host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> + SDHCI_VENDOR_VER_SHIFT));
> +
> + /* Setup PWRCTL irq */
> + msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
> + if (msm_host->pwr_irq < 0) {
> + dev_err(&pdev->dev, "Failed to get pwr_irq by name (%d)\n",
> + msm_host->pwr_irq);
> + goto vreg_disable;
At this point you have only acquired a handle to the vregs, you have not
enabled them. So you don't need to disable them.
> + }
> + ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
> + sdhci_msm_pwr_irq, IRQF_ONESHOT,
> + dev_name(&pdev->dev), host);
> + if (ret) {
> + dev_err(&pdev->dev, "Request threaded irq(%d) failed (%d)\n",
> + msm_host->pwr_irq, ret);
> + goto vreg_disable;
If this fails you haven't enabled the regulators, so no need to disable them
again.
> + }
> +
> + /* Enable pwr irq interrupts */
> + writel_relaxed(INT_MASK, (msm_host->core_mem + CORE_PWRCTL_MASK));
> +
> + msm_host->mmc->caps |= msm_host->pdata.caps;
> + msm_host->mmc->caps2 |= msm_host->pdata.caps2;
> +
> + ret = sdhci_add_host(host);
> + if (ret) {
> + dev_err(&pdev->dev, "Add host failed (%d)\n", ret);
> + goto vreg_disable;
> + }
> +
> + ret = clk_set_rate(msm_host->clk, host->max_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "MClk rate set failed (%d)\n", ret);
> + goto remove_host;
> + }
Why do you enable the clk further up in this function but wait with setting the
rate until the last thing in this function?
> +
> + return 0;
> +
> +remove_host:
> + dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
> + sdhci_remove_host(host, dead);
> +vreg_disable:
> + if (!IS_ERR(msm_host->pdata.vdd.reg))
If IS_ERR(vdd) or IS_ERR(vdd_io) then you would end up in clk_disable.
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
> + if (!IS_ERR(msm_host->pdata.vdd_io.reg))
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
> +clk_disable:
> + if (!IS_ERR(msm_host->clk))
If IS_ERR(clk) then you would end up in pclk_disable.
> + clk_disable_unprepare(msm_host->clk);
> +pclk_disable:
> + if (!IS_ERR(msm_host->pclk))
Based on the assumption that the check for errors on pclk above is incorrect,
then you would end up in bus_clk_disable if IS_ERR(pclk).
> + clk_disable_unprepare(msm_host->pclk);
> +bus_clk_disable:
> + if (!IS_ERR(msm_host->bus_clk))
bus_clk might be IS_ERR() as it's optional, so this makes sense.
> + clk_disable_unprepare(msm_host->bus_clk);
> +pltfm_free:
> + sdhci_pltfm_free(pdev);
> + return ret;
> +}
> +
> +static int sdhci_msm_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = pltfm_host->priv;
> + int dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) ==
> + 0xffffffff);
> +
You should probably start with disabling the pwr_irq here, to make sure that it
doesn't kick in after you starting to free resources.
> + sdhci_remove_host(host, dead);
> + sdhci_pltfm_free(pdev);
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
> + clk_disable_unprepare(msm_host->clk);
> + clk_disable_unprepare(msm_host->pclk);
> + if (!IS_ERR(msm_host->bus_clk))
> + clk_disable_unprepare(msm_host->bus_clk);
> + return 0;
> +}
> +
Regards,
Bjorn
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets
2014-03-04 3:15 ` Bjorn Andersson
@ 2014-03-04 17:50 ` Georgi Djakov
0 siblings, 0 replies; 13+ messages in thread
From: Georgi Djakov @ 2014-03-04 17:50 UTC (permalink / raw)
To: Bjorn Andersson
Cc: linux-mmc, Chris Ball, ulf.hansson, devicetree, Grant Likely,
Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren,
Ian Campbell, Kumar Gala, Rob Landley, linux-doc, linux-kernel,
linux-arm-msm
On 03/04/2014 05:15 AM, Bjorn Andersson wrote:
> On Fri, Feb 28, 2014 at 3:24 AM, Georgi Djakov <gdjakov@mm-sol.com> wrote:
>> This platform driver adds the initial support of Secure
>> Digital Host Controller Interface compliant controller
>> found in Qualcomm chipsets.
>>
>
> Hi Georgi,
>
> Sorry for reposting this, I have no idea how I managed to send this as an answer
> to patch 1/3...
>
>
> When testing this I was confused by the warnings from sdhci not finding vmmc
> and vqmmc. Is the power irq something Qualcomm specific or is there any other
> reason why the sdhci provided regulator functionality can't be used?
>
> Regarding the usage of the regulator api here, I think you should call
> regulator_set_voltage() with your default voltage when you acquire the
> regulator handles; then your power enable/disable functions will be simpler and
> you should be able to clean up the power irq function further.
>
Hi Bjorn,
Yes it is Qualcomm specific - voltage control is done not via the
standard SDHCI control registers. Writing to the registers will trigger
a separate IRQ and the handler configures the PMIC voltages.
>>
> [...]
>> +/* This structure keeps information per regulator */
>> +struct sdhci_msm_reg_data {
>> + struct regulator *reg;
>> + const char *name;
>> + /* Voltage level values */
>> + u32 low_vol_level;
>> + u32 high_vol_level;
>
> Is there a reason why these should be different? In your example and the other
> cases I've seen they are always 2.95V and 1.8V.
>
The host can support also 1.2V for DDR modes. Now I'll do it with
2.95/1.8V as you suggest and later i can expand it with optional
properties like mmc-hs200-1_2v or mmc-highspeed-ddr-1_2v.
>>
> [...]
>> +
>> +static int sdhci_msm_vreg_enable(struct device *dev,
>> + struct sdhci_msm_reg_data *vreg)
>> +{
>> + int ret = 0;
>> +
>> + if (!regulator_is_enabled(vreg->reg)) {
>> + /* Set voltage level */
>> + ret = regulator_set_voltage(vreg->reg, vreg->high_vol_level,
>> + vreg->high_vol_level);
>> + if (ret)
>> + return ret;
>
> So when you enable voltage in the irq handler or in probe, you will go to "high
> voltage", then you might lower this directly again.
>
Yes, but I will clean-up the irq handler.
>> + }
>> +
>> + ret = regulator_enable(vreg->reg);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable regulator %s (%d)\n",
>> + vreg->name, ret);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_vreg_disable(struct device *dev,
>> + struct sdhci_msm_reg_data *vreg)
>> +{
>> + int ret = 0;
>> +
>> + if (!regulator_is_enabled(vreg->reg))
>> + return ret;
>> +
>> + /* Set min. voltage to 0 */
>> + ret = regulator_set_voltage(vreg->reg, 0, vreg->high_vol_level);
>> + if (ret)
>> + return ret;
>
> Why do you set the voltage to 0 here?
>
The regulators can be shared with other peripherals, so when we are not
using them, we vote for 0 as minimum acceptable voltage.
>> +
>> + ret = regulator_disable(vreg->reg);
>> + if (ret) {
>> + dev_err(dev, "Failed to disable regulator %s (%d)\n",
>> + vreg->name, ret);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_setup_vreg(struct sdhci_msm_host *msm_host, bool enable)
>> +{
>
> Instead of having a function with one big if statement of which path you came
> from you should have two functions for this.
>
Oh sure! Will fix! Thanks!
>> + int ret, i;
>> + struct sdhci_msm_reg_data *vreg_table[2];
>> +
>> + vreg_table[0] = &msm_host->pdata.vdd;
>> + vreg_table[1] = &msm_host->pdata.vdd_io;
>> +
>> + for (i = 0; i < ARRAY_SIZE(vreg_table); i++) {
>> + if (enable)
>> + ret = sdhci_msm_vreg_enable(&msm_host->pdev->dev,
>> + vreg_table[i]);
>> + else
>> + ret = sdhci_msm_vreg_disable(&msm_host->pdev->dev,
>> + vreg_table[i]);
>> + if (ret)
>> + return ret;
>> + }
>
> This seems to a complicated way of saying:
>
> if (enable) {
> sdhci_msm_vreg_enable(vdd)
> sdhci_msm_vreg_enable(vdd_io)
> } else {
> sdhci_msm_vreg_disable(vdd)
> sdhci_msm_vreg_disable(vdd_io)
> }
>
> Do you plan to add more regulators here?
>
No.
>> +
>> + return 0;
>> +}
>> +
>> +static int sdhci_msm_vreg_init(struct device *dev,
>> + struct sdhci_msm_pltfm_data *pdata)
>> +{
>> + struct sdhci_msm_reg_data *vdd_reg = &pdata->vdd;
>> + struct sdhci_msm_reg_data *vdd_io_reg = &pdata->vdd_io;
>> +
>> + vdd_reg->reg = devm_regulator_get(dev, vdd_reg->name);
>> + if (IS_ERR(vdd_reg->reg))
>> + return PTR_ERR(vdd_reg->reg);
>> +
>> + vdd_io_reg->reg = devm_regulator_get(dev, vdd_io_reg->name);
>> + if (IS_ERR(vdd_io_reg->reg))
>> + return PTR_ERR(vdd_io_reg->reg);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>> +{
>> + struct sdhci_host *host = (struct sdhci_host *)data;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = pltfm_host->priv;
>> + u8 irq_status;
>> + u8 irq_ack = 0;
>> + int ret = 0;
>> +
>> + irq_status = readb_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
>> + dev_dbg(mmc_dev(msm_host->mmc), "%s: Received IRQ(%d), status=0x%x\n",
>> + mmc_hostname(msm_host->mmc), irq, irq_status);
>> +
>> + /* Clear the interrupt */
>> + writeb_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
>> + /*
>> + * SDHC has core_mem and hc_mem device memory and these memory
>> + * addresses do not fall within 1KB region. Hence, any update to
>> + * core_mem address space would require an mb() to ensure this gets
>> + * completed before its next update to registers within hc_mem.
>> + */
>
> This is the standard Qualcomm disclaimer regarding memory barriers, but what
> part of the system does actually touch hc_mem? As far as I can see this driver
> does not go outside that 1K and if the core sdhci core does, it seems to all be
> using the non-relaxed write*; so there would be an implicit sync there.
>
> Is it so that we make sure to clear the interrupt here and now?
>
Perhaps the comment is not entirely correct. We must make sure that the
interrupt is cleared first and code is not reordered.
>> + mb();
>> +
>> + /* Handle BUS ON/OFF */
>> + if (irq_status & CORE_PWRCTL_BUS_ON) {
>> + ret = sdhci_msm_setup_vreg(msm_host, true);
>> + if (!ret)
>> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
>> + msm_host->pdata.
>> + vdd_io.high_vol_level,
>> + msm_host->pdata.
>> + vdd_io.high_vol_level);
>
> If sdhci_msm_setup_vreg succeeds, you've already set a voltage to vdd_io and
> enabled it, why do this one more time?
>
Oops! Thanks!
>> + if (ret)
>> + irq_ack |= CORE_PWRCTL_BUS_FAIL;
>> + else
>> + irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> + }
>> +
>> + if (irq_status & CORE_PWRCTL_BUS_OFF) {
>> + ret = sdhci_msm_setup_vreg(msm_host, false);
>> + if (!ret)
>> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
>> + msm_host->pdata.
>> + vdd_io.low_vol_level,
>> + msm_host->pdata.
>> + vdd_io.low_vol_level);
>
> Same here.
>
Thanks!
>> + if (ret)
>> + irq_ack |= CORE_PWRCTL_BUS_FAIL;
>> + else
>> + irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> + }
>> +
>> + /* Handle IO LOW/HIGH */
>> + if (irq_status & CORE_PWRCTL_IO_LOW) {
>> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
>> + msm_host->pdata.
>> + vdd_io.low_vol_level,
>> + msm_host->pdata.
>> + vdd_io.low_vol_level);
>
> I assume that LOW is xor HIGH here, or you sould set it low then high.
>
> May I suggest that you restructure this to first figuring out what new voltage
> (if any) you're aiming for and then call regulator_set_voltage(vdd_io) once and
> based on that update the IO_{SUCCESS,FAIL} bits of irq_ack.
>
Ok. I'll restructure it. Thanks!
>> + if (ret)
>> + irq_ack |= CORE_PWRCTL_IO_FAIL;
>> + else
>> + irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>> + }
>> +
>> + if (irq_status & CORE_PWRCTL_IO_HIGH) {
>> + ret = regulator_set_voltage(msm_host->pdata.vdd_io.reg,
>> + msm_host->pdata.
>> + vdd_io.high_vol_level,
>> + msm_host->pdata.
>> + vdd_io.high_vol_level);
>> + if (ret)
>> + irq_ack |= CORE_PWRCTL_IO_FAIL;
>> + else
>> + irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>> + }
>> +
>> + /* ACK status to the core */
>> + writeb_relaxed(irq_ack, (msm_host->core_mem + CORE_PWRCTL_CTL));
>> + /*
>> + * SDHC has core_mem and hc_mem device memory and these memory
>> + * addresses do not fall within 1KB region. Hence, any update to
>> + * core_mem address space would require an mb() to ensure this gets
>> + * completed before its next update to registers within hc_mem.
>> + */
>
> Like above, is this mb() to guard for re-ordering or to commit the write?
>
Here we want to commit the write.
>> + mb();
>> +
>> + dev_dbg(mmc_dev(msm_host->mmc), "%s: Handled IRQ(%d), ret=%d, ack=0x%x\n",
>> + mmc_hostname(msm_host->mmc), irq, ret, irq_ack);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct of_device_id sdhci_msm_dt_match[] = {
>> + { .compatible = "qcom,sdhci-msm-v4" },
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>> +
>> +static struct sdhci_ops sdhci_msm_ops = {
>> + .platform_execute_tuning = sdhci_msm_execute_tuning,
>> +};
>> +
>> +static int sdhci_msm_probe(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host;
>> + struct sdhci_pltfm_host *pltfm_host;
>> + struct sdhci_msm_host *msm_host;
>> + struct resource *core_memres = NULL;
>
> No need to initialize, as first reference is an assignment.
>
Agree! Thank you!
>> + int ret, dead;
>> + u16 host_version;
>> +
>> + if (!pdev->dev.of_node) {
>> + dev_err(&pdev->dev, "No device tree data\n");
>> + return -ENOENT;
>> + }
>> +
>> + msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
>> + if (!msm_host)
>> + return -ENOMEM;
>> +
>> + msm_host->sdhci_msm_pdata.ops = &sdhci_msm_ops;
>> + host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
>> + if (IS_ERR(host))
>> + return PTR_ERR(host);
>> +
>> + pltfm_host = sdhci_priv(host);
>> + pltfm_host->priv = msm_host;
>> + msm_host->mmc = host->mmc;
>> + msm_host->pdev = pdev;
>> +
>> + ret = mmc_of_parse(host->mmc);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed parsing mmc device tree\n");
>> + goto pltfm_free;
>> + }
>> +
>> + sdhci_get_of_property(pdev);
>> +
>> + ret = sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
>> + if (ret) {
>> + dev_err(&pdev->dev, "DT parsing error\n");
>> + goto pltfm_free;
>> + }
>> +
>> + /* Setup SDCC bus voter clock. */
>> + msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>> + if (!IS_ERR(msm_host->bus_clk)) {
>> + /* Vote for max. clk rate for max. performance */
>> + ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
>> + if (ret)
>> + goto pltfm_free;
>> + ret = clk_prepare_enable(msm_host->bus_clk);
>> + if (ret)
>> + goto pltfm_free;
>> + }
>> +
>> + /* Setup main peripheral bus clock */
>> + msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>> + if (!IS_ERR(msm_host->pclk)) {
>
> iface clock is marked required in the binding documentation, so you probably
> don't want to fall through here on error.
>
I'll fix it. This code is from a few months ago when there was no GCC
support and i was using some power-on default clocks for testing. Thanks!
>> + ret = clk_prepare_enable(msm_host->pclk);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "Peripheral clock setup failed (%d)\n", ret);
>> + goto bus_clk_disable;
>> + }
>> + }
>> +
>> + /* Setup SDC MMC clock */
>> + msm_host->clk = devm_clk_get(&pdev->dev, "core");
>> + if (IS_ERR(msm_host->clk)) {
>> + ret = PTR_ERR(msm_host->clk);
>> + dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
>> + goto pclk_disable;
>> + }
>> +
>> + ret = clk_prepare_enable(msm_host->clk);
>> + if (ret)
>> + goto pclk_disable;
>> +
>> + /* Setup regulators */
>> + ret = sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata);
>> + if (ret) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(&pdev->dev,
>> + "Regulator setup failed (%d)\n", ret);
>> + goto clk_disable;
>> + }
>> +
>> + core_memres = platform_get_resource_byname(pdev,
>> + IORESOURCE_MEM, "core_mem");
>> + msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
>> +
>> + if (IS_ERR(msm_host->core_mem)) {
>> + dev_err(&pdev->dev, "Failed to remap registers\n");
>> + ret = PTR_ERR(msm_host->core_mem);
>> + goto vreg_disable;
>> + }
>> +
>> + /* Reset the core and Enable SDHC mode */
>> + writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
>> + CORE_SW_RST, msm_host->core_mem + CORE_POWER);
>> +
>> + /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
>> + usleep_range(1000, 5000);
>> + if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>> + dev_err(&pdev->dev, "Stuck in reset\n");
>> + ret = -ETIMEDOUT;
>> + goto vreg_disable;
>
> At this point you have only acquired a handle to the vregs, you have not
> enabled them. So you don't need to disable them.
Agree! Thanks!
>
>> + }
>> +
>> + /* Set HC_MODE_EN bit in HC_MODE register */
>> + writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
>> +
>> + /*
>> + * Following are the deviations from SDHC spec v3.0 -
>> + * 1. Card detection is handled using separate GPIO.
>> + * 2. Bus power control is handled by interacting with PMIC.
>> + */
>> + host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
>> + host->quirks |= SDHCI_QUIRK_SINGLE_POWER_WRITE;
>> +
>> + host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
>> + dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
>> + host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
>> + SDHCI_VENDOR_VER_SHIFT));
>> +
>> + /* Setup PWRCTL irq */
>> + msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>> + if (msm_host->pwr_irq < 0) {
>> + dev_err(&pdev->dev, "Failed to get pwr_irq by name (%d)\n",
>> + msm_host->pwr_irq);
>> + goto vreg_disable;
>
> At this point you have only acquired a handle to the vregs, you have not
> enabled them. So you don't need to disable them.
Thanks!
>
>> + }
>> + ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
>> + sdhci_msm_pwr_irq, IRQF_ONESHOT,
>> + dev_name(&pdev->dev), host);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Request threaded irq(%d) failed (%d)\n",
>> + msm_host->pwr_irq, ret);
>> + goto vreg_disable;
>
> If this fails you haven't enabled the regulators, so no need to disable them
> again.
>
Thanks!
>> + }
>> +
>> + /* Enable pwr irq interrupts */
>> + writel_relaxed(INT_MASK, (msm_host->core_mem + CORE_PWRCTL_MASK));
>> +
>> + msm_host->mmc->caps |= msm_host->pdata.caps;
>> + msm_host->mmc->caps2 |= msm_host->pdata.caps2;
>> +
>> + ret = sdhci_add_host(host);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Add host failed (%d)\n", ret);
>> + goto vreg_disable;
>> + }
>> +
>> + ret = clk_set_rate(msm_host->clk, host->max_clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "MClk rate set failed (%d)\n", ret);
>> + goto remove_host;
>> + }
>
> Why do you enable the clk further up in this function but wait with setting the
> rate until the last thing in this function?
>
This serves mostly as a workaround as the hardware requires a custom
set_clock() implementation and the clocks will fail in sdhci_add_host().
I prefer to keep it this way until i submit a patch that implements the
clock control - clock scaling and clock gating.
>> +
>> + return 0;
>> +
>> +remove_host:
>> + dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
>> + sdhci_remove_host(host, dead);
>> +vreg_disable:
>> + if (!IS_ERR(msm_host->pdata.vdd.reg))
>
> If IS_ERR(vdd) or IS_ERR(vdd_io) then you would end up in clk_disable.
>
>> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd);
>> + if (!IS_ERR(msm_host->pdata.vdd_io.reg))
>> + sdhci_msm_vreg_disable(&pdev->dev, &msm_host->pdata.vdd_io);
>> +clk_disable:
>> + if (!IS_ERR(msm_host->clk))
>
> If IS_ERR(clk) then you would end up in pclk_disable.
>
>> + clk_disable_unprepare(msm_host->clk);
>> +pclk_disable:
>> + if (!IS_ERR(msm_host->pclk))
>
> Based on the assumption that the check for errors on pclk above is incorrect,
> then you would end up in bus_clk_disable if IS_ERR(pclk).
>
Yes, de-init in reverse order and free resources. It seems that i can
drop the IS_ERR.
>> + clk_disable_unprepare(msm_host->pclk);
>> +bus_clk_disable:
>> + if (!IS_ERR(msm_host->bus_clk))
>
> bus_clk might be IS_ERR() as it's optional, so this makes sense.
>
>> + clk_disable_unprepare(msm_host->bus_clk);
>> +pltfm_free:
>> + sdhci_pltfm_free(pdev);
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_remove(struct platform_device *pdev)
>> +{
>> + struct sdhci_host *host = platform_get_drvdata(pdev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = pltfm_host->priv;
>> + int dead = (readl_relaxed(host->ioaddr + SDHCI_INT_STATUS) ==
>> + 0xffffffff);
>> +
>
> You should probably start with disabling the pwr_irq here, to make sure that it
> doesn't kick in after you starting to free resources.
>
Oops, i will fix it.
Thanks for the detailed review and all the comments!
BR,
Georgi
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-03-04 17:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-28 11:24 [PATCH v9 0/3] mmc: sdhci-msm: Add support for Qualcomm chipsets Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 1/3] mmc: sdhci-msm: Qualcomm SDHCI binding documentation Georgi Djakov
2014-03-03 2:03 ` Bjorn Andersson
2014-02-28 11:24 ` [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets Georgi Djakov
2014-03-04 3:15 ` Bjorn Andersson
2014-03-04 17:50 ` Georgi Djakov
2014-02-28 11:24 ` [PATCH v9 3/3] mmc: sdhci-msm: Add platform_execute_tunning implementation Georgi Djakov
2014-02-28 16:51 ` Kumar Gala
2014-02-28 22:10 ` Georgi Djakov
2014-02-28 20:45 ` Josh Cartwright
[not found] ` <1393586675-14628-4-git-send-email-gdjakov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
2014-02-28 20:51 ` Josh Cartwright
2014-02-28 20:51 ` Josh Cartwright
2014-02-28 22:29 ` Georgi Djakov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.