| Message ID | 1346670519-13097-1-git-send-email-vinit.shenoy@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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); -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)