Re: [PATCH v3 3/4] mmc: sh_mobile_sdhi: Add tuning support

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 4034 bytes --]

On Tue, May 24, 2016 at 11:43:16AM +0900, Simon Horman wrote:
> From: Ai Kyuse <ai.kyuse.uw@renesas.com>

I wonder if you shouldn't take over ownership of this and the previous
patch? You changed quite a lot.

> +static inline u32 sd_scc_read32(struct tmio_mmc_host *host, int addr)
> +{
> +	return readl(host_to_priv(host)->scc_ctl + (addr << host->bus_shift));
> +}

What about passing 'priv' to these functions? Then we can save the
host_to_priv for each access.

> +
> +static inline void sd_scc_write32(struct tmio_mmc_host *host, int addr, u32 val)
> +{
> +	writel(val, host_to_priv(host)->scc_ctl + (addr << host->bus_shift));
> +}

Ditto.

> +
> +static unsigned int sh_mobile_sdhi_init_tuning(struct tmio_mmc_host *host)
> +{
> +	if (!(host->mmc->caps & MMC_CAP_UHS_SDR104))
> +		return 0;

Will the core call us if MMC_CAP_UHS_SDR104 was not set?


> +
> +	/* set sampling clock selection range */
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_DTCNTL,
> +			0x8 << SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT);
> +
> +	/* Initialize SCC */
> +	sd_ctrl_write32_as_16_and_16(host, CTL_STATUS, 0x00000000);

..., CTL_STATUS, 0);

?

> +
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_DTCNTL,
> +		SH_MOBILE_SDHI_SCC_DTCNTL_TAPEN |
> +		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_DTCNTL));
> +
> +	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
> +		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));

'CLK_CTL_SCLKEN' instead of 0x100?

> +
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_CKSEL,
> +		SH_MOBILE_SDHI_SCC_CKSEL_DTSEL |
> +		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_CKSEL));
> +
> +	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
> +		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
> +
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSCNTL,
> +		~SH_MOBILE_SDHI_SCC_RVSCNTL_RVSEN &
> +		sd_scc_read32(host, SH_MOBILE_SDHI_SCC_RVSCNTL));
> +
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_DT2FF, host->scc_tappos);
> +
> +	/* Read TAPNUM */
> +	return (sd_scc_read32(host, SH_MOBILE_SDHI_SCC_DTCNTL) >>
> +		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
> +		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
> +}
> +
> +static void sh_mobile_sdhi_prepare_tuning(struct tmio_mmc_host *host,
> +					 unsigned long tap)
> +{
> +	/* Set sampling clock position */
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_TAPSET, tap);
> +}
> +
> +#define SH_MOBILE_SDHI_MAX_TAP	3

unused

> +
> +static int sh_mobile_sdhi_select_tuning(struct tmio_mmc_host *host,
> +					bool *tap, int tap_size)
> +{
> +	unsigned long tap_num, i;
> +	int ok_count;
> +
> +	/* Clear SCC_RVSREQ */
> +	sd_scc_write32(host, SH_MOBILE_SDHI_SCC_RVSREQ, 0);
> +
> +	/* Select SCC */
> +	tap_num = (sd_scc_read32(host, SH_MOBILE_SDHI_SCC_DTCNTL) >>
> +		   SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_SHIFT) &
> +		SH_MOBILE_SDHI_SCC_DTCNTL_TAPNUM_MASK;
> +
> +	if (tap_num * 2 != tap_size)
> +		return -EINVAL;
> +
> +	/*
> +	 * Select clock where three consecutive bock reads succeeded.
> +	 *
> +	 * There may be multiple occurrences of three successive reads
> +	 * and selecting any of them is correct. Here the first one is
> +	 * selected.
> +	 */
> +	ok_count = 0;
> +	for (i = 0; i < tap_size; i++) {
> +		if (tap[i])
> +			ok_count++;
> +		else
> +			ok_count = 0;

ok_count = tap[i] ? ok_count + 1 : 0;

? Yes, I do like the ternary operator :D

...

> +	if (host->mmc->caps & MMC_CAP_UHS_SDR104) {
> +		/* Reset SCC */
> +		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~0x0100 &
> +			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));

'CLK_CTL_SCLKEN' instead of 0x100?

> +
> +		sd_scc_write32(host, SH_MOBILE_SDHI_SCC_CKSEL,
> +			~SH_MOBILE_SDHI_SCC_CKSEL_DTSEL &
> +			sd_scc_read32(host, SH_MOBILE_SDHI_SCC_CKSEL));
> +
> +		sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, 0x0100 |
> +			sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));

Ditto.

...

> +		if (!hit)
> +			dev_warn(&host->pdev->dev, "Unknown clock rate for SDR104 and HS200\n");

HS200 will come later, I think (although the path should be easy now).

Thanks, I think we are quite close. Maybe Ulf does have some high level
comments?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]