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.
>> + 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));
>
