Re: [PATCH 3/3] mmc: sdhci_am654: Add Initial Support for AM654 SDHCI driver

[Faiz Abbas <faiz_abbas@ti.com>]

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.
> 
>> +
>> +	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