Re: [PATCH 5/7 v4] mmc: sh_mmcif: calculate best clock with parent clock

Re: [PATCH 5/7 v4] mmc: sh_mmcif: calculate best clock with parent clock

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

I'm CC'ing the device-tree mailing list as well as Mike Turquette, please see 
below for the discussion.

On Thursday 23 April 2015 08:17:23 Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> MMCIF IP on R-Car series has parent clock which can be set
> several rate, and it was not implemented on old SH-Mobile series
> (= SH-Mobile series parent clock was fixed rate)
> R-Car series MMCIF can use more high speed access if it setup
> parent clock. This patch adds parent clock setup method,
> and r8a7790/r8a7791 can use it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Tested-by: Keita Kobayashi <keita.kobayashi.ym@renesas.com>
> ---
> v3 -> v4
> 
>  - add new clk-range on DT
>  - use clk_round_rate() for parent_clk
>  - struct sh_mmcif_ver instead of sh_mmcif_parent_clk
> 
>  .../devicetree/bindings/mmc/renesas,mmcif.txt      |  3 +
>  drivers/mmc/host/sh_mmcif.c                        | 89 ++++++++++++++++++-
>  2 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt index
> 299081f..eb50f4e 100644
> --- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> +++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> @@ -14,6 +14,8 @@ Required properties:
> 
>  - clocks: reference to the functional clock
> 
> +- clk-range: parent clock range
> +

I think the idea of a DT property to specify the admissible range for clock 
frequencies is good. However, I have a couple of small comments regarding the 
implementation.

First of all, this doesn't seem Renesas-specific to me (feel free to disagree, 
but in that case the property should probably be called renesas,clk-range). 
Wouldn't it make sense to specify the property in 
Documentation/devicetree/bindings/clock/clock-bindings.txt instead ?

Then, I think the documentation should be clarified a bit, as "parent clock 
range" is probably a bit vague for people who haven't followed the development 
of this patch series. How about something like "range of admissible 
frequencies for the input clock" ?

There's already a clock-ranges property with a totally different meaning. 
That's quite confusing, it would thus be good to rename clk-range. How about 
clock-freq-range or clock-frequency-range ?

Finally, if a DT node references several clocks in its clocks property, we 
need a way to specify the range for each of them.

>  - dmas: reference to the DMA channels, one per channel name listed in the
>    dma-names property.
>  - dma-names: must contain "tx" for the transmit DMA channel and "rx" for
> the @@ -29,4 +31,5 @@ Example: R8A7790 (R-Car H2) MMCIF0
>  		clocks = <&mstp3_clks R8A7790_CLK_MMCIF0>;
>  		dmas = <&dmac0 0xd1>, <&dmac0 0xd2>;
>  		dma-names = "tx", "rx";
> +		clk-range = <12187500 97500000>;
>  	};
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index d2f1158..437aa3c 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -57,6 +57,7 @@
>  #include <linux/mmc/slot-gpio.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/mutex.h>
> +#include <linux/of_device.h>
>  #include <linux/pagemap.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_qos.h>
> @@ -224,6 +225,13 @@ enum mmcif_wait_for {
>  	MMCIF_WAIT_FOR_STOP,
>  };
> 
> +/*
> + * difference for each SoC
> + */
> +struct sh_mmcif_ver {
> +	u32 clkdiv_map;	 /* see CE_CLK_CTRL::CLKDIV */
> +};
> +
>  struct sh_mmcif_host {
>  	struct mmc_host *mmc;
>  	struct mmc_request *mrq;
> @@ -248,6 +256,7 @@ struct sh_mmcif_host {
>  	bool ccs_enable;		/* Command Completion Signal support */
>  	bool clk_ctrl2_enable;
>  	struct mutex thread_lock;
> +	const struct sh_mmcif_ver *ver;
> 
>  	/* DMA support */
>  	struct dma_chan		*chan_rx;
> @@ -256,8 +265,14 @@ struct sh_mmcif_host {
>  	bool			dma_active;
>  };
> 
> +static const struct sh_mmcif_ver sh_mmcif_gen2 = {
> +	.clkdiv_map = 0x3ff,
> +};
> +
>  static const struct of_device_id mmcif_of_match[] = {
>  	{ .compatible = "renesas,sh-mmcif" },
> +	{ .compatible = "renesas,mmcif-r8a7790", .data = &sh_mmcif_gen2},
> +	{ .compatible = "renesas,mmcif-r8a7791", .data = &sh_mmcif_gen2},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, mmcif_of_match);
> @@ -490,12 +505,50 @@ static void sh_mmcif_clock_control(struct
> sh_mmcif_host *host, unsigned int clk)
> 
>  	if (!clk)
>  		return;
> -	if (sup_pclk && clk == current_clk)
> +
> +	if (host->ver) {
> +		const struct sh_mmcif_ver *ver = host->ver;
> +		unsigned int freq, best_freq, myclk, clkdiv, div, diff_min, diff;
> +		int i;
> +
> +		clkdiv = 0;
> +		diff_min = ~0;
> +		best_freq = 0;
> +		for (i = fls(ver->clkdiv_map); i >= 0; i--) {
> +			if (!((1 << i) & ver->clkdiv_map))
> +				continue;
> +
> +			/*
> +			 * clk = parent_freq / div
> +			 * -> parent_freq = clk x div
> +			 */
> +
> +			div = 1 << (i + 1);
> +			freq = clk_round_rate(host->clk, clk * div);
> +			myclk = freq / div;
> +			diff = (myclk > clk) ? myclk - clk : clk - myclk;
> +
> +			if (diff <= diff_min) {
> +				best_freq = freq;
> +				clkdiv = i;
> +				diff_min = diff;
> +			}
> +		}
> +
> +		dev_dbg(&host->pd->dev, "clk %u/%u (%u, 0x%x)\n",
> +			(best_freq / (1 << (clkdiv + 1))), clk,
> +			best_freq, clkdiv);
> +
> +		clk_set_rate(host->clk, best_freq);
> +		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL,
> +				CLK_CLEAR & (clkdiv << 16));
> +	} else if (sup_pclk && clk == current_clk) {
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
> -	else
> +	} else {
>  		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
>  				((fls(DIV_ROUND_UP(current_clk,
>  						   clk) - 1) - 1) << 16));
> +	}
> 
>  	sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_ENABLE);
>  }
> @@ -980,10 +1033,42 @@ static void sh_mmcif_request(struct mmc_host *mmc,
> struct mmc_request *mrq)
> 
>  static void sh_mmcif_clk_setup(struct sh_mmcif_host *host)
>  {
> +	struct device *dev = &host->pd->dev;
> +	const struct of_device_id *of_id = of_match_device(mmcif_of_match, dev);
>  	unsigned int clk = clk_get_rate(host->clk);
> 
> +	if (of_id) {
> +		struct device_node *np = dev->of_node;
> +		const struct sh_mmcif_ver *ver = of_id->data;
> +		u32 range[2]; /* min/max */
> +
> +		if (!ver)
> +			goto sh_mmcif_clk_setup_default;
> +
> +		if (0 != of_property_read_u32_array(np, "clk-range",
> +						    range, ARRAY_SIZE(range)))
> +			goto sh_mmcif_clk_setup_default;
> +
> +		if (range[0] > range[1])
> +			goto sh_mmcif_clk_setup_default;
> +
> +		host->mmc->f_min = range[0] / (1 << fls(ver->clkdiv_map));
> +		host->mmc->f_max = range[1] / (1 << ffs(ver->clkdiv_map));
> +
> +		dev_dbg(dev, "parent clk <%d - %d> (%d/%d)\n",
> +			range[0], range[1],
> +			host->mmc->f_max, host->mmc->f_min);
> +
> +		host->ver = ver;
> +		return;
> +	}
> +
> +sh_mmcif_clk_setup_default:
>  	host->mmc->f_max = clk / 2;
>  	host->mmc->f_min = clk / 512;
> +
> +	dev_dbg(dev, "clk max/min = %d/%d\n",
> +		host->mmc->f_max, host->mmc->f_min);
>  }
> 
>  static void sh_mmcif_set_power(struct sh_mmcif_host *host, struct mmc_ios
> *ios)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7 v4] mmc: sh_mmcif: calculate best clock with parent clock

[Kuninori Morimoto]

Hi Laurent

Thank you for your feedback

> > diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt index
> > 299081f..eb50f4e 100644
> > --- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > +++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > @@ -14,6 +14,8 @@ Required properties:
> > 
> >  - clocks: reference to the functional clock
> > 
> > +- clk-range: parent clock range
> > +
> 
> I think the idea of a DT property to specify the admissible range for clock 
> frequencies is good. However, I have a couple of small comments regarding the 
> implementation.
> 
> First of all, this doesn't seem Renesas-specific to me (feel free to disagree, 
> but in that case the property should probably be called renesas,clk-range). 
> Wouldn't it make sense to specify the property in 
> Documentation/devicetree/bindings/clock/clock-bindings.txt instead ?
> 
> Then, I think the documentation should be clarified a bit, as "parent clock 
> range" is probably a bit vague for people who haven't followed the development 
> of this patch series. How about something like "range of admissible 
> frequencies for the input clock" ?
> 
> There's already a clock-ranges property with a totally different meaning. 
> That's quite confusing, it would thus be good to rename clk-range. How about 
> clock-freq-range or clock-frequency-range ?
> 
> Finally, if a DT node references several clocks in its clocks property, we 
> need a way to specify the range for each of them.

I didn't tell you about this, but actually I created v5 patch
(it is under test stage now, I will send it to ML soon)
which doesn't add new DT property.

This v4 added new property for clock frequencies, and it was Geert's idea.
This is just my opinion, and I don't know this is good match for DT,
but, this clock frequencies range depends on each SoC, and we are already
using SoC based "compatible" on DT. This means we can (should ?) get each SoC
specific feature (which is not related to each platform/board) from "compatible".
Thus, v5 patch gets clock frequency range from SoC based "compatible".

Thank you for your help, please check my next v5 patch.

Best regards
---
Kuninori Morimoto