| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53161281.8040203@mm-sol.com>
Date: Tue, 04 Mar 2014 19:50:57 +0200
From: Georgi Djakov <gdjakov@...sol.com>
To: Bjorn Andersson <bjorn@...o.se>
CC: linux-mmc@...r.kernel.org, Chris Ball <cjb@...top.org>,
ulf.hansson@...aro.org,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Stephen Warren <swarren@...dotorg.org>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Rob Landley <rob@...dley.net>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v9 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets
On 03/04/2014 05:15 AM, Bjorn Andersson wrote:
> On Fri, Feb 28, 2014 at 3:24 AM, Georgi Djakov <gdjakov@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists