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

[Simon Horman]

On Fri, May 27, 2016 at 01:59:34PM +0200, Wolfram Sang wrote:
> On Tue, May 24, 2016 at 11:43:16AM +0900, Simon Horman wrote:
> > From: Ai Kyuse 
> 
> I wonder if you shouldn't take over ownership of this and the previous
> patch? You changed quite a lot.

Sure, will do.

[snip]

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

Empirically, yes, that appears to be the case.


For all your other suggestions: thanks, I will update things as you suggest.

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

[Simon Horman]

On Tue, May 31, 2016 at 12:39:58PM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Tue, May 24, 2016 at 4:43 AM, Simon Horman
>  wrote:
> > --- a/drivers/mmc/host/sh_mobile_sdhi.c
> > +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> 
> > @@ -403,6 +580,30 @@ static int sh_mobile_sdhi_probe(struct platform_device 
> > *pdev)
> > if (ret < 0)
> > goto efree;
> >
> > +   if (host->mmc->caps & MMC_CAP_UHS_SDR104)
> > +   host->mmc->caps |= MMC_CAP_HW_RESET;
> > +
> > +   if (of_id && of_id->data) {
> > +   const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> > +   const struct sh_mobile_sdhi_scc *taps = of_data->taps;
> > +   bool hit = false;
> > +
> > +   for (i = 0; i < of_data->taps_num; i++) {
> > +   if (taps[i].clk_rate == 0 ||
> > +   taps[i].clk_rate == host->mmc->f_max) {
> > +   host->scc_tappos = taps->tap;
> > +   hit = true;
> > +   break;
> > +   }
> > +   }
> > +
> > +   if (!hit)
> > +   dev_warn(>pdev->dev, "Unknown clock rate for 
> > SDR104 and HS200\n");
> 
> This warning triggers on sh7a0/kzm9g, r8a73a4/ape6evm, and
> r8a7740/armadillo.
> 
> Perhaps the tap code should check if MMC_CAP_UHS_SDR104 is
> enabled?

Thanks. Yes, I think that would be sensible.

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

[Geert Uytterhoeven]

Hi Simon,

On Tue, May 24, 2016 at 4:43 AM, Simon Horman
 wrote:
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c

> @@ -403,6 +580,30 @@ static int sh_mobile_sdhi_probe(struct platform_device 
> *pdev)
> if (ret < 0)
> goto efree;
>
> +   if (host->mmc->caps & MMC_CAP_UHS_SDR104)
> +   host->mmc->caps |= MMC_CAP_HW_RESET;
> +
> +   if (of_id && of_id->data) {
> +   const struct sh_mobile_sdhi_of_data *of_data = of_id->data;
> +   const struct sh_mobile_sdhi_scc *taps = of_data->taps;
> +   bool hit = false;
> +
> +   for (i = 0; i < of_data->taps_num; i++) {
> +   if (taps[i].clk_rate == 0 ||
> +   taps[i].clk_rate == host->mmc->f_max) {
> +   host->scc_tappos = taps->tap;
> +   hit = true;
> +   break;
> +   }
> +   }
> +
> +   if (!hit)
> +   dev_warn(>pdev->dev, "Unknown clock rate for 
> SDR104 and HS200\n");

This warning triggers on sh7a0/kzm9g, r8a73a4/ape6evm, and
r8a7740/armadillo.

Perhaps the tap code should check if MMC_CAP_UHS_SDR104 is
enabled?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

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

[Wolfram Sang]

On Tue, May 24, 2016 at 11:43:16AM +0900, Simon Horman wrote:
> From: Ai Kyuse 

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, 0x);

..., 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(>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?



signature.asc
Description: PGP signature

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

[Simon Horman]

From: Ai Kyuse 

Add tuning support for use with SDR104 mode
This includes adding support for the sampling clock controller (SCC).

Signed-off-by: Ai Kyuse 
Signed-off-by: Simon Horman 
---
v3 [Simon Horman]
* As suggested by Kuninori Morimoto:
  - Do not add unused retuning callback to struct tmio_mmc_host
  - Change return type of prepare_tuning callback to void
  - Add tap_size parameter to select_tuning callback

v2 [Simon Horman]
* As suggested by Kuninori Morimoto
  - Use host->mmc->caps & MMC_CAP_UHS_SDR104 instead of
pdata->flags & TMIO_MMC_HAS_UHS_SCC to avoid needing the
MMC_CAP_UHS_SDR104 flag at all.
N.B: Morimoto-san suggested using but this flag is not actually
set there in by current probe come.
  - Simplify logic in sh_mobile_sdhi_inquiry_tuning
* As suggested by Wolfram Sang
  - Use clk_rate instead of clk for field in struct sh_mobile_sdhi_scc
  - Remove inquiry_tuning callback which is now unnecessary as calling
of tuning is handled by the core
  - Removed unused sh_mobile_sdhi_set_clk_div callback
  - Save sci_base address rather than calculating it on each read and write
* Update selection logic in sh_mobile_sdhi_select_tuning to match spec
* Use bool instead of long for taps parameter of
  sh_mobile_sdhi_select_tuning()
* Return 0 from sh_mobile_sdhi_init_tuning() if the
  SDR104 capability is not set and thus tuning should not
  be performed because it is not supported by the hardware

v1 [Simon Horman]
* Rebase
* Always use value of 0x8 for TAPNUM field of DTCNTL register
  rather than reading value from DT property. There does not
  seem to be a need to expose this in DT at this point.
* Do not include tmio_mmc_start_signal_voltage_switch changes which
  are already in mainline in a different form
* Do not add renesas,clk-rate property as the max-frequency property, which
  is now present in mainline, seems to provide the needed rate
* Omit Gen3 specific changes
* Do not provide renesas,mmc-scc-tappos DT property.
  Instead, always use taps provided in driver.
* Do not parse sd-uhs-sdr50 and sd-uhs-sdr104 properties.
  This is handled by the core.

v0 [Ai Kyuse]
---
 drivers/mmc/host/sh_mobile_sdhi.c | 203 +-
 1 file changed, 202 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c 
b/drivers/mmc/host/sh_mobile_sdhi.c
index 5309c73be1f0..3ad52de45b89 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -41,6 +41,11 @@
 
 #define host_to_priv(host) container_of((host)->pdata, struct sh_mobile_sdhi, 
mmc_data)
 
+struct sh_mobile_sdhi_scc {
+   unsigned long clk_rate; /* clock rate for SDR104 */
+   u32 tap;/* sampling clock position for SDR104 */
+};
+
 struct sh_mobile_sdhi_of_data {
unsigned long tmio_flags;
unsigned long capabilities;
@@ -48,6 +53,9 @@ struct sh_mobile_sdhi_of_data {
enum dma_slave_buswidth dma_buswidth;
dma_addr_t dma_rx_offset;
unsigned bus_shift;
+   int scc_offset;
+   struct sh_mobile_sdhi_scc *taps;
+   int taps_num;
 };
 
 static const struct sh_mobile_sdhi_of_data of_default_cfg = {
@@ -60,12 +68,27 @@ static const struct sh_mobile_sdhi_of_data 
of_rcar_gen1_compatible = {
.capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
 };
 
+/* Definitions for sampling clocks */
+static struct sh_mobile_sdhi_scc rcar_gen2_scc_taps[] = {
+   {
+   .clk_rate = 15600,
+   .tap = 0x0703,
+   },
+   {
+   .clk_rate = 0,
+   .tap = 0x0300,
+   },
+};
+
 static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
.tmio_flags = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE |
  TMIO_MMC_CLK_ACTUAL | TMIO_MMC_MIN_RCAR2,
.capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
.dma_buswidth   = DMA_SLAVE_BUSWIDTH_4_BYTES,
.dma_rx_offset  = 0x2000,
+   .scc_offset = 0x0300,
+   .taps   = rcar_gen2_scc_taps,
+   .taps_num   = ARRAY_SIZE(rcar_gen2_scc_taps),
 };
 
 static const struct sh_mobile_sdhi_of_data of_rcar_gen3_compatible = {
@@ -98,6 +121,7 @@ struct sh_mobile_sdhi {
struct tmio_mmc_dma dma_priv;
struct pinctrl *pinctrl;
struct pinctrl_state *pins_default, *pins_uhs;
+   void __iomem *scc_ctl;
 };
 
 static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
@@ -241,6 +265,155 @@ static int 
sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
return pinctrl_select_state(priv->pinctrl, pin_state);
 }
 
+/* SCC registers */
+#define SH_MOBILE_SDHI_SCC_DTCNTL  0x000
+#define SH_MOBILE_SDHI_SCC_TAPSET  0x002
+#define SH_MOBILE_SDHI_SCC_DT2FF   0x004
+#define SH_MOBILE_SDHI_SCC_CKSEL   0x006
+#define SH_MOBILE_SDHI_SCC_RVSCNTL