| 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: <970c3531-5d7d-fe94-55c1-13603735b43a@intel.com>
Date: Fri, 7 Dec 2018 15:32:00 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Faiz Abbas <faiz_abbas@...com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-mmc@...r.kernel.org
Cc: ulf.hansson@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
michal.simek@...inx.com, kishon@...com
Subject: Re: [PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI
driver
On 5/12/18 5:07 PM, Faiz Abbas wrote:
> Hi Adrian,
>
> On 05/12/18 7:12 PM, Adrian Hunter wrote:
>> On 29/11/18 6:15 PM, Faiz Abbas wrote:
>>> The host controllers on TI's AM654 SOCs are not compatible with
>>> the phy and consumer model of the sdhci-of-arasan driver. It turns out
>>> that for optimal operation at higher speeds, a special tuning procedure
>>> needs to be implemented which involves configuration of platform
>>> specific phy registers.
>>>
>>> Therefore, branch out to a new sdhci_am654 driver and add the phy
>>> register space with all phy configurations to it. Populate AM654
>>> specific callbacks to sdhci_ops and add SDHCI_QUIRKS wherever
>>> applicable.
>>>
>>> Only add support for upto High Speed for SD card and upto DDR52 speed
>>> mode for eMMC. Higher speeds will be added in subsequent patches.
>>>
> ...
>>> + sdhci_am654->dll_on = true;
>>> + }
>>> +}
>>> +
>>> +
>>
>> Double blank line
>
> Will fix.
>
>>
>>> +static void sdhci_am654_set_power(struct sdhci_host *host, unsigned char mode,
>>> + unsigned short vdd)
>>> +{
>>> + if (!IS_ERR(host->mmc->supply.vmmc)) {
>>> + struct mmc_host *mmc = host->mmc;
>>> +
>>> + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
>>> + }
>>> + sdhci_set_power_noreg(host, mode, vdd);
>>> +}
>>> +
>>> +struct sdhci_ops sdhci_am654_ops = {
>>> + .get_max_clock = sdhci_pltfm_clk_get_max_clock,
>>> + .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>>> + .set_uhs_signaling = sdhci_set_uhs_signaling,
>>> + .set_bus_width = sdhci_set_bus_width,
>>> + .set_power = sdhci_am654_set_power,
>>> + .set_clock = sdhci_am654_set_clock,
>>> + .reset = sdhci_reset,
>>> +};
>>> +
>>> +static const struct sdhci_pltfm_data sdhci_am654_pdata = {
>>> + .ops = &sdhci_am654_ops,
>>> + .quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
>>> + SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>> +};
>>> +
>>> +static int sdhci_am654_init(struct sdhci_host *host)
>>> +{
>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> + struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>> + u32 ctl_cfg_2 = 0;
>>> + u32 val;
>>> + int ret;
>>> +
>>> + regmap_read(sdhci_am654->base, PHY_STAT1, &val);
>>> + if (~val & CALDONE_MASK) {
>>> + /* Calibrate IO lines */
>>> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>>> + PDB_MASK, PDB_MASK);
>>> + ret = regmap_read_poll_timeout(sdhci_am654->base, PHY_STAT1,
>>> + val, val & CALDONE_MASK, 1, 20);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + /* Enable pins by setting IO mux to 0 */
>>> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>>> + IOMUX_ENABLE_MASK, 0);
>>> +
>>> + /* Set slot type based on SD or eMMC */
>>> + if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
>>> + ctl_cfg_2 = SLOTTYPE_EMBEDDED;
>>> +
>>> + regmap_update_bits(sdhci_am654->base, CTL_CFG_2,
>>> + ctl_cfg_2, SLOTTYPE_MASK);
>>> +
>>> + return sdhci_add_host(host);
>>> +}
>>> +
>>> +static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>> + struct sdhci_am654_data *sdhci_am654)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + int drv_strength;
>>> + int ret;
>>> +
>>> + ret = device_property_read_u32(dev, "ti,trm-icp",
>>> + &sdhci_am654->trm_icp);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = device_property_read_u32(dev, "ti,otap-del-sel",
>>> + &sdhci_am654->otap_del_sel);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = device_property_read_u32(dev, "ti,driver-strength-ohm",
>>> + &drv_strength);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + switch (drv_strength) {
>>> + case 50:
>>> + sdhci_am654->drv_strength = DRIVER_STRENGTH_50_OHM;
>>> + break;
>>> + case 33:
>>> + sdhci_am654->drv_strength = DRIVER_STRENGTH_33_OHM;
>>> + break;
>>> + case 66:
>>> + sdhci_am654->drv_strength = DRIVER_STRENGTH_66_OHM;
>>> + break;
>>> + case 100:
>>> + sdhci_am654->drv_strength = DRIVER_STRENGTH_100_OHM;
>>> + break;
>>> + case 40:
>>> + sdhci_am654->drv_strength = DRIVER_STRENGTH_40_OHM;
>>> + break;
>>> + default:
>>> + dev_err(dev, "Invalid driver strength\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + sdhci_get_of_property(pdev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sdhci_am654_probe(struct platform_device *pdev)
>>> +{
>>> + struct sdhci_pltfm_host *pltfm_host;
>>> + struct sdhci_am654_data *sdhci_am654;
>>> + struct sdhci_host *host;
>>> + struct resource *res;
>>> + struct clk *clk_xin;
>>> + struct device *dev = &pdev->dev;
>>> + void __iomem *base;
>>> + int ret;
>>> +
>>> + host = sdhci_pltfm_init(pdev, &sdhci_am654_pdata, sizeof(*sdhci_am654));
>>> + if (IS_ERR(host))
>>> + return PTR_ERR(host);
>>> +
>>> + pltfm_host = sdhci_priv(host);
>>> + sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> + clk_xin = devm_clk_get(dev, "clk_xin");
>>> + if (IS_ERR(clk_xin)) {
>>> + dev_err(dev, "clk_xin clock not found.\n");
>>> + ret = PTR_ERR(clk_xin);
>>> + goto err_pltfm_free;
>>> + }
>>> +
>>> + sdhci_am654->clk_ahb = devm_clk_get(dev, "clk_ahb");
>>> + if (IS_ERR(sdhci_am654->clk_ahb)) {
>>> + dev_err(dev, "clk_ahb clock not found.\n");
>>> + ret = PTR_ERR(sdhci_am654->clk_ahb);
>>> + goto err_pltfm_free;
>>> + }
>>
>> Did you intend not to enable clks?
>
> Yes. Clocks get enabled as a part of pm_runtime calls.
Ok, but that could use an explanatory comment. Also why get a reference to
clk_ahb if that reference is never used?
>>
>>> +
>>> + pltfm_host->clk = clk_xin;
>>> +
>>> + pm_runtime_enable(dev);
>>> + ret = pm_runtime_get_sync(dev);
>>> + if (ret > 0) {
>>
>> Did you intend 'ret > 0'?
>
> Sorry. That was intended to be < 0.
>
> Thanks,
> Faiz
>
Powered by blists - more mailing lists