| Message ID | 1393586675-14628-2-git-send-email-gdjakov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"; + };
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