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