[PATCH 1/1] mmc: debugfs: Set frequency to maximum possible on higher request

[PATCH 1/1] mmc: debugfs: Set frequency to maximum possible on higher request

[Vinit Shenoy]

Currently if we enter a frequency greater than maximum supported,
-EINVAL is returned. Due to this clock is not switched to maximum
supported frequency.

For example consider the max supported frequency to be 48MHz

echo 20000000 > /sys/kernel/debug/mmc0/clock Here clock is switched to 20Mhz.

echo 80000000 > /sys/kernel/debug/mmc0/clock
Ideally clock should be set back to 48MHz, but it is still set to 20MHz.

This patch sets value to f_max when the requested frequency is greater
than f_max

Signed-off-by: Vinit Shenoy <vinit.shenoy@st.com>
---
 drivers/mmc/core/debugfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 9ab5b17..c71d3eb 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -174,7 +174,7 @@ static int mmc_clock_opt_set(void *data, u64 val)
 
 	/* We need this check due to input value is u64 */
 	if (val > host->f_max)
-		return -EINVAL;
+		val = host->f_max;
 
 	mmc_claim_host(host);
 	mmc_set_clock(host, (unsigned int) val);
-- 
1.7.3.4

Re: [PATCH 1/1] mmc: debugfs: Set frequency to maximum possible on higher request

[Andy Shevchenko]

On Mon, Sep 3, 2012 at 2:08 PM, Vinit Shenoy <vinit.shenoy@st.com> wrote:
> Currently if we enter a frequency greater than maximum supported,
> -EINVAL is returned. Due to this clock is not switched to maximum
> supported frequency.
>
> For example consider the max supported frequency to be 48MHz
>
> echo 20000000 > /sys/kernel/debug/mmc0/clock Here clock is switched to 20Mhz.
>
> echo 80000000 > /sys/kernel/debug/mmc0/clock
> Ideally clock should be set back to 48MHz, but it is still set to 20MHz.
>
> This patch sets value to f_max when the requested frequency is greater
> than f_max
>
> Signed-off-by: Vinit Shenoy <vinit.shenoy@st.com>
> ---
>  drivers/mmc/core/debugfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 9ab5b17..c71d3eb 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -174,7 +174,7 @@ static int mmc_clock_opt_set(void *data, u64 val)
>
>         /* We need this check due to input value is u64 */
>         if (val > host->f_max)
> -               return -EINVAL;
> +               val = host->f_max;
It's a matter of taste. However, I think it's time to start
Documentation/mmc/debugfs.txt
and describe the file there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] mmc: debugfs: Set frequency to maximum possible on higher request

[Subhash Jadavani]

On 9/3/2012 4:38 PM, Vinit Shenoy wrote:
> Currently if we enter a frequency greater than maximum supported,
> -EINVAL is returned. Due to this clock is not switched to maximum
> supported frequency.
>
> For example consider the max supported frequency to be 48MHz
>
> echo 20000000 > /sys/kernel/debug/mmc0/clock Here clock is switched to 20Mhz.
>
> echo 80000000 > /sys/kernel/debug/mmc0/clock
> Ideally clock should be set back to 48MHz, but it is still set to 20MHz.
>
> This patch sets value to f_max when the requested frequency is greater
> than f_max
>
> Signed-off-by: Vinit Shenoy <vinit.shenoy@st.com>
> ---
>   drivers/mmc/core/debugfs.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 9ab5b17..c71d3eb 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -174,7 +174,7 @@ static int mmc_clock_opt_set(void *data, u64 val)
>   
>   	/* We need this check due to input value is u64 */
>   	if (val > host->f_max)
> -		return -EINVAL;
> +		val = host->f_max;

As such this is ok if you want this behaviour. But i see other issue 
which is not related to your patch.

Let's say if the host supports 400KHz, 25 MHz,  50MHz, 100MHz and 
currently card is operating in HS (High Speed) mode@50MHz. and now if 
someone tries to change the frequency to 100MHz, then we should just 
don't let the clock change to 100MHz before we put the card in 
appropriate bus speed mode (in SD cards case, it would be SDR50 and eMMC 
case it would be HS200). Although this is anyway not an issue created by 
your patch but this is just to let you know that frequency can't be 
changed indepently without informing the card beforehand.

As far as this patch is concerned, it looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

>   
>   	mmc_claim_host(host);
>   	mmc_set_clock(host, (unsigned int) val);

Re: [PATCH 1/1] mmc: debugfs: Set frequency to maximum possible on higher request

[Andy Shevchenko]

On Mon, Sep 3, 2012 at 2:50 PM, Subhash Jadavani
<subhashj@codeaurora.org> wrote:
> As such this is ok if you want this behaviour. But i see other issue which
> is not related to your patch.

> Let's say if the host supports 400KHz, 25 MHz,  50MHz, 100MHz and currently
> card is operating in HS (High Speed) mode@50MHz. and now if someone tries to
> change the frequency to 100MHz, then we should just don't let the clock
> change to 100MHz before we put the card in appropriate bus speed mode (in SD
> cards case, it would be SDR50 and eMMC case it would be HS200). Although
> this is anyway not an issue created by your patch but this is just to let
> you know that frequency can't be changed indepently without informing the
> card beforehand.

That's why the file is located under debugfs. There is an assumption
that developer understands
what he or she is doing by changing this parameter.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] mmc: debugfs: Set frequency to maximum possible on higher request

[Subhash Jadavani]

On 9/3/2012 6:00 PM, Andy Shevchenko wrote:
> On Mon, Sep 3, 2012 at 2:50 PM, Subhash Jadavani
> <subhashj@codeaurora.org> wrote:
>> As such this is ok if you want this behaviour. But i see other issue which
>> is not related to your patch.
>> Let's say if the host supports 400KHz, 25 MHz,  50MHz, 100MHz and currently
>> card is operating in HS (High Speed) mode@50MHz. and now if someone tries to
>> change the frequency to 100MHz, then we should just don't let the clock
>> change to 100MHz before we put the card in appropriate bus speed mode (in SD
>> cards case, it would be SDR50 and eMMC case it would be HS200). Although
>> this is anyway not an issue created by your patch but this is just to let
>> you know that frequency can't be changed indepently without informing the
>> card beforehand.
> That's why the file is located under debugfs. There is an assumption
> that developer understands
> what he or she is doing by changing this parameter.
Agreed. Thanks.
>
>
>