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 >