23.12.2019 18:20, Dmitry Osipenko пишет:
> 23.12.2019 18:06, Dmitry Osipenko пишет:
>>
>> Hello Thierry,
>>
>> 22.12.2019 14:40, Thierry Reding пишет:
>>> From: Thierry Reding <treding@xxxxxxxxxx>
>>>
>>> A common debugfs interface is already available on Tegra124, Tegra186
>>> and Tegra194. Implement the same interface on Tegra20 to enable testing
>>> of the EMC frequency scaling code using a unified interface.
>>>
>>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>>> ---
>>> drivers/memory/tegra/tegra20-emc.c | 175 +++++++++++++++++++++++++++++
>>> 1 file changed, 175 insertions(+)
>>>
>>> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
>>> index 1b23b1c34476..8ae474d9bfb9 100644
>>> --- a/drivers/memory/tegra/tegra20-emc.c
>>> +++ b/drivers/memory/tegra/tegra20-emc.c
>>> @@ -8,6 +8,7 @@
>>> #include <linux/clk.h>
>>> #include <linux/clk/tegra.h>
>>> #include <linux/completion.h>
>>> +#include <linux/debugfs.h>
>>> #include <linux/err.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/io.h>
>>> @@ -150,6 +151,12 @@ struct tegra_emc {
>>>
>>> struct emc_timing *timings;
>>> unsigned int num_timings;
>>> +
>>> + struct {
>>> + struct dentry *root;
>>> + unsigned long min_rate;
>>> + unsigned long max_rate;
>>> + } debugfs;
>>> };
>>>
>>> static irqreturn_t tegra_emc_isr(int irq, void *data)
>>> @@ -478,6 +485,171 @@ static long emc_round_rate(unsigned long rate,
>>> return timing->rate;
>>> }
>>>
>>> +/*
>>> + * debugfs interface
>>> + *
>>> + * The memory controller driver exposes some files in debugfs that can be used
>>> + * to control the EMC frequency. The top-level directory can be found here:
>>> + *
>>> + * /sys/kernel/debug/emc
>>> + *
>>> + * It contains the following files:
>>> + *
>>> + * - available_rates: This file contains a list of valid, space-separated
>>> + * EMC frequencies.
>>> + *
>>> + * - min_rate: Writing a value to this file sets the given frequency as the
>>> + * floor of the permitted range. If this is higher than the currently
>>> + * configured EMC frequency, this will cause the frequency to be
>>> + * increased so that it stays within the valid range.
>>> + *
>>> + * - max_rate: Similarily to the min_rate file, writing a value to this file
>>> + * sets the given frequency as the ceiling of the permitted range. If
>>> + * the value is lower than the currently configured EMC frequency, this
>>> + * will cause the frequency to be decreased so that it stays within the
>>> + * valid range.
>>> + */
>>> +
>>> +static bool tegra_emc_validate_rate(struct tegra_emc *emc, unsigned long rate)
>>> +{
>>> + unsigned int i;
>>> +
>>> + for (i = 0; i < emc->num_timings; i++)
>>> + if (rate == emc->timings[i].rate)
>>> + return true;
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static int tegra_emc_debug_available_rates_show(struct seq_file *s, void *data)
>>> +{
>>> + struct tegra_emc *emc = s->private;
>>> + const char *prefix = "";
>>> + unsigned int i;
>>> +
>>> + for (i = 0; i < emc->num_timings; i++) {
>>> + seq_printf(s, "%s%lu", prefix, emc->timings[i].rate);
>>> + prefix = " ";
>>> + }
>>> +
>>> + seq_puts(s, "\n");
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_emc_debug_available_rates_open(struct inode *inode,
>>> + struct file *file)
>>> +{
>>> + return single_open(file, tegra_emc_debug_available_rates_show,
>>> + inode->i_private);
>>> +}
>>> +
>>> +static const struct file_operations tegra_emc_debug_available_rates_fops = {
>>> + .open = tegra_emc_debug_available_rates_open,
>>> + .read = seq_read,
>>> + .llseek = seq_lseek,
>>> + .release = single_release,
>>> +};
>>> +
>>> +static int tegra_emc_debug_min_rate_get(void *data, u64 *rate)
>>> +{
>>> + struct tegra_emc *emc = data;
>>> +
>>> + *rate = emc->debugfs.min_rate;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_emc_debug_min_rate_set(void *data, u64 rate)
>>> +{
>>> + struct tegra_emc *emc = data;
>>> + int err;
>>> +
>>> + if (!tegra_emc_validate_rate(emc, rate))
>>> + return -EINVAL;
>>> +
>>> + err = clk_set_min_rate(emc->clk, rate);
>>> + if (err < 0)
>>> + return err;
>
> Changing of min-rate may not result in the actual change of the rate,
> this is needed if you're expecting the change-rate to happen:
>
> clk_set_rate(emc->clk, 0);
>
> Perhaps you'd also want to provide some kind of integration with the
> devfreq in order to stop it for the time of testing, otherwise it may
> intervene here.
Please note that simply pausing devfreq won't work because that won't
remove its min-rate request.
Although, I guess you could solve that by simply unloading devfreq
module for the time of testing.
For now I'm not sure what could be done about interconnect once we'll
support it.
>>> + emc->debugfs.min_rate = rate;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(tegra_emc_debug_min_rate_fops,
>>> + tegra_emc_debug_min_rate_get,
>>> + tegra_emc_debug_min_rate_set, "%llu\n");
>>> +
>>> +static int tegra_emc_debug_max_rate_get(void *data, u64 *rate)
>>> +{
>>> + struct tegra_emc *emc = data;
>>> +
>>> + *rate = emc->debugfs.max_rate;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int tegra_emc_debug_max_rate_set(void *data, u64 rate)
>>> +{
>>> + struct tegra_emc *emc = data;
>>> + int err;
>>> +
>>> + if (!tegra_emc_validate_rate(emc, rate))
>>> + return -EINVAL;
>>> +
>>> + err = clk_set_max_rate(emc->clk, rate);
>>> + if (err < 0)
>>> + return err;
>
> Similar to min-rate.
>
>>> + emc->debugfs.max_rate = rate;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(tegra_emc_debug_max_rate_fops,
>>> + tegra_emc_debug_max_rate_get,
>>> + tegra_emc_debug_max_rate_set, "%llu\n");
>>> +
>>> +static void tegra_emc_debugfs_init(struct tegra_emc *emc)
>>> +{
>>> + struct device *dev = emc->dev;
>>> + unsigned int i;
>>> + int err;
>>> +
>>> + emc->debugfs.min_rate = ULONG_MAX;
>>> + emc->debugfs.max_rate = 0;
>>> +
>>> + for (i = 0; i < emc->num_timings; i++) {
>>> + if (emc->timings[i].rate < emc->debugfs.min_rate)
>>> + emc->debugfs.min_rate = emc->timings[i].rate;
>>> +
>>> + if (emc->timings[i].rate > emc->debugfs.max_rate)
>>> + emc->debugfs.max_rate = emc->timings[i].rate;
>>> + }
>>> +
>>> + err = clk_set_rate_range(emc->clk, emc->debugfs.min_rate,
>>> + emc->debugfs.max_rate);
>>> + if (err < 0) {
>>> + dev_err(dev, "failed to set rate range [%lu-%lu] for %pC\n",
>>> + emc->debugfs.min_rate, emc->debugfs.max_rate,
>>> + emc->clk);
>>> + }
>>> +
>>> + emc->debugfs.root = debugfs_create_dir("emc", NULL);
>>
>> What about "tegra-emc"?
>>
>>> + if (!emc->debugfs.root) {
>>> + dev_err(emc->dev, "failed to create debugfs directory\n");
>>> + return;
>>> + }
>>> +
>>> + debugfs_create_file("available_rates", S_IRUGO, emc->debugfs.root,
>>> + emc, &tegra_emc_debug_available_rates_fops);
>>> + debugfs_create_file("min_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
>>> + emc, &tegra_emc_debug_min_rate_fops);
>>> + debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
>>> + emc, &tegra_emc_debug_max_rate_fops);
>>> +}
>>> +
>>> static int tegra_emc_probe(struct platform_device *pdev)
>>> {
>>> struct device_node *np;
>>> @@ -550,6 +722,9 @@ static int tegra_emc_probe(struct platform_device *pdev)
>>> goto unset_cb;
>>> }
>>>
>>> + platform_set_drvdata(pdev, emc);
>>> + tegra_emc_debugfs_init(emc);
>>> +
>>> return 0;
>>>
>>> unset_cb:
>>>
>>
>> In general this looks good to me, but maybe there is a bit too much of
>> the code duplication here for 3 drivers?
>>
>> Perhaps it all could be factored out into a common debug.c, something like:
>>
>> debug.c:
>> --------
>>
>> static struct tegra_emc_debug {
>> struct clk *clk;
>> struct dentry *root;
>> unsigned int num_rates;
>> unsigned long rates[];
>> } *emc_dbg;
>>
>> int tegra_emc_register_debugfs(struct device *dev,
>> struct clk *clk,
>> unsigned int num_rates,
>> unsigned long *rates,
>> size_t emc_timing_size)
>> {
>> ...
>> emc_dbg_size = struct_size(emc_dbg, rates, num_rates);
>> emc_dbg = devm_kcalloc(dev, emc_dbg_size);
>> ...
>>
>> for (i = 0; i < num_rates; i++) {
>> emc_dbg->rates[i] = rates[i];
>> rates = (void*)rates + emc_timing_size;
>> }
>>
>> emc_dbg->num_rates = i;
>>
>> ...
>> }
>>
>> emc-driver.c:
>> -------------
>>
>> tegra_emc_register_debugfs(emc->dev, emc->clk, emc->num_timings,
>> &emc->timings[0].rate, sizeof(*emc->timings));
>>
>
