[RFC PATCH 0/4] Tegra124: EMC scaling

[RFC PATCH 0/4] Tegra124: EMC scaling

[Tomeu Vizoso]

Hi,

here is an initial implementation of EMC scaling for Tegra124, I'm most
interested in feedback on the approach.

The present incarnation is very much specific to Tegra, but I'm sure that we
could find an API that can be shared across SoC families.

I have looked at using existing frameworks (common clock, pm_qos and devfreq)
to cover this use case, but it always felt like trying to fit a square peg in a
round hole. These are our requirements:

    1. Ceiling frequencies (for thermal and other power management components)

    2. Floor frequencies (determined from load statistics that are often
maintained by some firmware, to avoid frequent interrupts)

    3. Let misc. device drivers such as display controllers or USB set their
bandwidth requirements, which would be aggregated to calculate the final
effective frequency.

    4. The EMC on the Tegra124 has per-consumer latency allowance registers
that influence how memory access requests are arbitrated, and these depend on
the clock frequency.

1 and 2 could be implemented as additions to the common clock framework, but I
feel that 3 should live at a higher level (as not all clocks are used to drive
memory buses), and 4 doesn't seem related to clocks at all.

For past discussions on this see: https://lkml.org/lkml/2014/5/13/469

Tomeu Vizoso (4):
  memory: tegra124-emc: Add EMC driver
  ARM: tegra: Add Tegra124 EMC support
  drm/tegra: Request memory bandwidth for the display controller
  cpufreq: tegra: Register a minimum EMC frequency based on the CPU
    clock

 .../bindings/arm/tegra/nvidia,tegra124-emc.txt     |  26 ++++
 arch/arm/boot/dts/tegra124.dtsi                    |   9 ++
 drivers/cpufreq/tegra-cpufreq.c                    |  20 +--
 drivers/gpu/drm/tegra/dc.c                         |   9 ++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/tegra124-emc.c                      | 173 +++++++++++++++++++++
 include/linux/platform_data/tegra_emc.h            |  23 +++
 8 files changed, 254 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
 create mode 100644 drivers/memory/tegra124-emc.c

-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Tomeu Vizoso]

Adds functionality for registering memory bandwidth needs and setting
the EMC clock rate based on that.

Also adds API for setting floor and ceiling frequency rates.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 .../bindings/arm/tegra/nvidia,tegra124-emc.txt     |  26 ++++
 drivers/memory/Kconfig                             |   8 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/tegra124-emc.c                      | 173 +++++++++++++++++++++
 include/linux/platform_data/tegra_emc.h            |  23 +++
 5 files changed, 231 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
 create mode 100644 drivers/memory/tegra124-emc.c

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
new file mode 100644
index 0000000..88e6a55
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
@@ -0,0 +1,26 @@
+Tegra124 External Memory Controller
+
+Properties:
+- compatible : Should contain "nvidia,tegra124-emc".
+- reg : Should contain the register range of the device
+- #address-cells : Should be 1
+- #size-cells : Should be 0
+- nvidia,mc : phandle to the mc bus connected to EMC.
+- clocks : phandle to EMC, EMC shared bus override, and all parent clocks.
+- clock-names : name of each clock.
+- nvidia,pmc : phandle to the PMC syscon node.
+- max-clock-frequency : optional, specifies the maximum EMC rate in kHz.
+
+Child device nodes describe the memory settings for different configurations and
+clock rates.
+
+Example:
+
+	memory-controller@7001b000 {
+		compatible = "nvidia,tegra124-emc";
+		reg = <0x7001b000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car TEGRA124_CLK_EMC>;
+		clock-names = "emc";
+	};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index c59e9c9..48fa0dd 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -61,6 +61,14 @@ config TEGRA30_MC
 	  analysis, especially for IOMMU/SMMU(System Memory Management
 	  Unit) module.
 
+config TEGRA124_EMC
+	tristate "Tegra124 External Memory Controller (EMC) driver"
+	default y
+	depends on ARCH_TEGRA_124_SOC
+	help
+	  This driver is for the External Memory Controller (EMC) module
+	  available in Tegra124 SoCs.
+
 config FSL_IFC
 	bool
 	depends on FSL_SOC
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 71160a2..0b7290b 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_FSL_IFC)		+= fsl_ifc.o
 obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_TEGRA30_MC)	+= tegra30-mc.o
+obj-$(CONFIG_TEGRA124_EMC)	+= tegra124-emc.o
diff --git a/drivers/memory/tegra124-emc.c b/drivers/memory/tegra124-emc.c
new file mode 100644
index 0000000..b7a54a5
--- /dev/null
+++ b/drivers/memory/tegra124-emc.c
@@ -0,0 +1,173 @@
+/*
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/of_platform.h>
+#include <linux/platform_data/tegra_emc.h>
+
+#define DRV_NAME "tegra124-emc"
+#define EMC_FREQ_CUTOFF_USE_130_PERCENT	100000000
+#define EMC_FREQ_CUTOFF_USE_140_PERCENT	50000000
+#define BYTES_PER_EMC_CLOCK 16
+
+struct tegra124_emc {
+	struct clk *clk;
+	unsigned long bandwidth_requests[TEGRA_EMC_CONSUMER_LAST];
+	unsigned long floor_freq;
+	unsigned long ceiling_freq;
+	/*
+	 * Cannot use a mutex here because the ACTMON driver would set a floor
+	 * frequency from an IRQ handler.
+	 */
+	spinlock_t spinlock;
+};
+
+static struct platform_device *emc_pdev;
+
+static unsigned long tegra124_emc_bw_to_freq_req(unsigned long bw)
+{
+	return (bw + BYTES_PER_EMC_CLOCK - 1) / BYTES_PER_EMC_CLOCK;
+}
+
+static void tegra124_emc_update_rate(struct tegra124_emc *emc)
+{
+	int i;
+	struct clk *emc_master;
+	unsigned long total_bandwidth = 0;
+	unsigned long freq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&emc->spinlock, flags);
+
+	for (i = 0; i < TEGRA_EMC_CONSUMER_LAST; i++)
+		total_bandwidth += emc->bandwidth_requests[i];
+
+	emc_master = clk_get_parent(emc->clk);
+	freq = tegra124_emc_bw_to_freq_req(total_bandwidth) * 1000;
+	freq = clk_round_rate(emc_master, freq);
+
+	/* XXX: Add safety margins for DVFS */
+
+	if (freq < EMC_FREQ_CUTOFF_USE_140_PERCENT)
+		total_bandwidth += 4 * total_bandwidth / 10;
+	else if (freq < EMC_FREQ_CUTOFF_USE_130_PERCENT)
+		total_bandwidth += 3 * total_bandwidth / 10;
+	else
+		total_bandwidth += total_bandwidth / 10;
+
+	freq = tegra124_emc_bw_to_freq_req(total_bandwidth) * 1000;
+	freq = max(freq, emc->floor_freq);
+	freq = min(freq, emc->ceiling_freq);
+
+	spin_unlock_irqrestore(&emc->spinlock, flags);
+
+
+	clk_set_rate(emc->clk, freq);
+}
+
+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
+{
+	struct tegra124_emc *emc = platform_get_drvdata(emc_pdev);
+	unsigned long flags;
+
+	if (consumer >= TEGRA_EMC_CONSUMER_LAST) {
+		dev_err(&emc_pdev->dev, "Invalid EMC consumer ID (%u)\n", consumer);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&emc->spinlock, flags);
+	emc->bandwidth_requests[consumer] = rate;
+	spin_unlock_irqrestore(&emc->spinlock, flags);
+
+	tegra124_emc_update_rate(emc);
+
+	return 0;
+}
+
+void tegra124_emc_set_floor(unsigned long freq)
+{
+	struct tegra124_emc *emc = platform_get_drvdata(emc_pdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&emc->spinlock, flags);
+	emc->floor_freq = freq;
+	spin_unlock_irqrestore(&emc->spinlock, flags);
+
+	tegra124_emc_update_rate(emc);
+}
+
+void tegra124_emc_set_ceiling(unsigned long freq)
+{
+	struct tegra124_emc *emc = platform_get_drvdata(emc_pdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&emc->spinlock, flags);
+	emc->ceiling_freq = freq;
+	spin_unlock_irqrestore(&emc->spinlock, flags);
+
+	tegra124_emc_update_rate(emc);
+}
+
+static int tegra124_emc_probe(struct platform_device *pdev)
+{
+	struct tegra124_emc *emc;
+
+	emc_pdev = pdev;
+
+	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
+	if (emc == NULL) {
+		dev_err(&pdev->dev, "Failed to allocate private memory\n");
+		return -ENOMEM;
+	}
+
+	emc->ceiling_freq = ULONG_MAX;
+
+	emc->clk = devm_clk_get(&pdev->dev, "emc");
+	if (IS_ERR(emc->clk)) {
+		devm_kfree(&pdev->dev, emc);
+		dev_err(&pdev->dev, "Can not find EMC clock\n");
+		return -EINVAL;
+	}
+
+	spin_lock_init(&emc->spinlock);
+
+	platform_set_drvdata(emc_pdev, emc);
+
+	return 0;
+}
+
+static struct of_device_id tegra124_emc_of_match[] = {
+	{ .compatible = "nvidia,tegra124-emc", },
+	{ },
+};
+
+static struct platform_driver tegra124_emc_driver = {
+	.driver         = {
+		.name   = DRV_NAME,
+		.owner  = THIS_MODULE,
+		.of_match_table = tegra124_emc_of_match,
+	},
+	.probe          = tegra124_emc_probe,
+};
+
+module_platform_driver(tegra124_emc_driver);
+
+MODULE_AUTHOR("Tomeu Vizoso <tomeu.vizoso@collabora.com>");
+MODULE_DESCRIPTION("Tegra124 EMC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/tegra_emc.h b/include/linux/platform_data/tegra_emc.h
index df67505..2967964 100644
--- a/include/linux/platform_data/tegra_emc.h
+++ b/include/linux/platform_data/tegra_emc.h
@@ -31,4 +31,27 @@ struct tegra_emc_pdata {
 	struct tegra_emc_table *tables;
 };
 
+enum {
+	TEGRA_EMC_CONSUMER_DISP1 = 0,
+	TEGRA_EMC_CONSUMER_DISP2,
+	TEGRA_EMC_CONSUMER_MSENC,
+	TEGRA_EMC_CONSUMER_CAMERA,
+	TEGRA_EMC_CONSUMER_AVP,
+	TEGRA_EMC_CONSUMER_ISO,
+	TEGRA_EMC_CONSUMER_LAST
+};
+
+#ifdef CONFIG_TEGRA124_EMC
+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate);
+void tegra124_emc_set_floor(unsigned long freq);
+void tegra124_emc_set_ceiling(unsigned long freq);
+#else
+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
+{ return -ENODEV; }
+void tegra124_emc_set_floor(unsigned long freq)
+{ return; }
+void tegra124_emc_set_ceiling(unsigned long freq)
+{ return; }
+#endif
+
 #endif
-- 
1.9.3

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Mikko Perttunen]

It should be mentioned that calling clk_set_rate on the EMC clock 
currently does absolutely nothing (except probably returning an error). 
The rate switching sequence is not implemented (nor is the clock tree 
entirely correct. For example, the kernel thinks that PLL_M is disabled).

Another note: I am currently implementing an actmon driver. I'm not 
entirely enthusiastic about the downstream style of managing EMC rate 
policy in the actmon driver, but haven't yet given much thought to it.

Yet another note: I think the exported API should not be SoC-specific, 
so s/tegra124_/tegra_/.

Thanks,
- Mikko

On 06/16/2014 04:35 PM, Tomeu Vizoso wrote:
> Adds functionality for registering memory bandwidth needs and setting
> the EMC clock rate based on that.
>
> Also adds API for setting floor and ceiling frequency rates.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>   .../bindings/arm/tegra/nvidia,tegra124-emc.txt     |  26 ++++
>   drivers/memory/Kconfig                             |   8 +
>   drivers/memory/Makefile                            |   1 +
>   drivers/memory/tegra124-emc.c                      | 173 +++++++++++++++++++++
>   include/linux/platform_data/tegra_emc.h            |  23 +++
>   5 files changed, 231 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
>   create mode 100644 drivers/memory/tegra124-emc.c
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> new file mode 100644
> index 0000000..88e6a55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> @@ -0,0 +1,26 @@
> +Tegra124 External Memory Controller
> +
> +Properties:
> +- compatible : Should contain "nvidia,tegra124-emc".
> +- reg : Should contain the register range of the device
> +- #address-cells : Should be 1
> +- #size-cells : Should be 0
> +- nvidia,mc : phandle to the mc bus connected to EMC.
> +- clocks : phandle to EMC, EMC shared bus override, and all parent clocks.
> +- clock-names : name of each clock.
> +- nvidia,pmc : phandle to the PMC syscon node.
> +- max-clock-frequency : optional, specifies the maximum EMC rate in kHz.
> +
> +Child device nodes describe the memory settings for different configurations and
> +clock rates.
> +
> +Example:
> +
> +	memory-controller@7001b000 {
> +		compatible = "nvidia,tegra124-emc";
> +		reg = <0x7001b000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&tegra_car TEGRA124_CLK_EMC>;
> +		clock-names = "emc";
> +	};
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c59e9c9..48fa0dd 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -61,6 +61,14 @@ config TEGRA30_MC
>   	  analysis, especially for IOMMU/SMMU(System Memory Management
>   	  Unit) module.
>
> +config TEGRA124_EMC
> +	tristate "Tegra124 External Memory Controller (EMC) driver"
> +	default y
> +	depends on ARCH_TEGRA_124_SOC
> +	help
> +	  This driver is for the External Memory Controller (EMC) module
> +	  available in Tegra124 SoCs.
> +
>   config FSL_IFC
>   	bool
>   	depends on FSL_SOC
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 71160a2..0b7290b 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_FSL_IFC)		+= fsl_ifc.o
>   obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
>   obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
>   obj-$(CONFIG_TEGRA30_MC)	+= tegra30-mc.o
> +obj-$(CONFIG_TEGRA124_EMC)	+= tegra124-emc.o
> diff --git a/drivers/memory/tegra124-emc.c b/drivers/memory/tegra124-emc.c
> new file mode 100644
> index 0000000..b7a54a5
> --- /dev/null
> +++ b/drivers/memory/tegra124-emc.c
> @@ -0,0 +1,173 @@
> +/*
> + * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_data/tegra_emc.h>
> +
> +#define DRV_NAME "tegra124-emc"
> +#define EMC_FREQ_CUTOFF_USE_130_PERCENT	100000000
> +#define EMC_FREQ_CUTOFF_USE_140_PERCENT	50000000
> +#define BYTES_PER_EMC_CLOCK 16
> +
> +struct tegra124_emc {
> +	struct clk *clk;
> +	unsigned long bandwidth_requests[TEGRA_EMC_CONSUMER_LAST];
> +	unsigned long floor_freq;
> +	unsigned long ceiling_freq;
> +	/*
> +	 * Cannot use a mutex here because the ACTMON driver would set a floor
> +	 * frequency from an IRQ handler.
> +	 */
> +	spinlock_t spinlock;
> +};
> +
> +static struct platform_device *emc_pdev;
> +
> +static unsigned long tegra124_emc_bw_to_freq_req(unsigned long bw)
> +{
> +	return (bw + BYTES_PER_EMC_CLOCK - 1) / BYTES_PER_EMC_CLOCK;
> +}
> +
> +static void tegra124_emc_update_rate(struct tegra124_emc *emc)
> +{
> +	int i;
> +	struct clk *emc_master;
> +	unsigned long total_bandwidth = 0;
> +	unsigned long freq;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&emc->spinlock, flags);
> +
> +	for (i = 0; i < TEGRA_EMC_CONSUMER_LAST; i++)
> +		total_bandwidth += emc->bandwidth_requests[i];
> +
> +	emc_master = clk_get_parent(emc->clk);
> +	freq = tegra124_emc_bw_to_freq_req(total_bandwidth) * 1000;
> +	freq = clk_round_rate(emc_master, freq);
> +
> +	/* XXX: Add safety margins for DVFS */
> +
> +	if (freq < EMC_FREQ_CUTOFF_USE_140_PERCENT)
> +		total_bandwidth += 4 * total_bandwidth / 10;
> +	else if (freq < EMC_FREQ_CUTOFF_USE_130_PERCENT)
> +		total_bandwidth += 3 * total_bandwidth / 10;
> +	else
> +		total_bandwidth += total_bandwidth / 10;
> +
> +	freq = tegra124_emc_bw_to_freq_req(total_bandwidth) * 1000;
> +	freq = max(freq, emc->floor_freq);
> +	freq = min(freq, emc->ceiling_freq);
> +
> +	spin_unlock_irqrestore(&emc->spinlock, flags);
> +
> +
> +	clk_set_rate(emc->clk, freq);
> +}
> +
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
> +{
> +	struct tegra124_emc *emc = platform_get_drvdata(emc_pdev);
> +	unsigned long flags;
> +
> +	if (consumer >= TEGRA_EMC_CONSUMER_LAST) {
> +		dev_err(&emc_pdev->dev, "Invalid EMC consumer ID (%u)\n", consumer);
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&emc->spinlock, flags);
> +	emc->bandwidth_requests[consumer] = rate;
> +	spin_unlock_irqrestore(&emc->spinlock, flags);
> +
> +	tegra124_emc_update_rate(emc);
> +
> +	return 0;
> +}
> +
> +void tegra124_emc_set_floor(unsigned long freq)
> +{
> +	struct tegra124_emc *emc = platform_get_drvdata(emc_pdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&emc->spinlock, flags);
> +	emc->floor_freq = freq;
> +	spin_unlock_irqrestore(&emc->spinlock, flags);
> +
> +	tegra124_emc_update_rate(emc);
> +}
> +
> +void tegra124_emc_set_ceiling(unsigned long freq)
> +{
> +	struct tegra124_emc *emc = platform_get_drvdata(emc_pdev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&emc->spinlock, flags);
> +	emc->ceiling_freq = freq;
> +	spin_unlock_irqrestore(&emc->spinlock, flags);
> +
> +	tegra124_emc_update_rate(emc);
> +}
> +
> +static int tegra124_emc_probe(struct platform_device *pdev)
> +{
> +	struct tegra124_emc *emc;
> +
> +	emc_pdev = pdev;
> +
> +	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
> +	if (emc == NULL) {
> +		dev_err(&pdev->dev, "Failed to allocate private memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	emc->ceiling_freq = ULONG_MAX;
> +
> +	emc->clk = devm_clk_get(&pdev->dev, "emc");
> +	if (IS_ERR(emc->clk)) {
> +		devm_kfree(&pdev->dev, emc);
> +		dev_err(&pdev->dev, "Can not find EMC clock\n");
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_init(&emc->spinlock);
> +
> +	platform_set_drvdata(emc_pdev, emc);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id tegra124_emc_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-emc", },
> +	{ },
> +};
> +
> +static struct platform_driver tegra124_emc_driver = {
> +	.driver         = {
> +		.name   = DRV_NAME,
> +		.owner  = THIS_MODULE,
> +		.of_match_table = tegra124_emc_of_match,
> +	},
> +	.probe          = tegra124_emc_probe,
> +};
> +
> +module_platform_driver(tegra124_emc_driver);
> +
> +MODULE_AUTHOR("Tomeu Vizoso <tomeu.vizoso@collabora.com>");
> +MODULE_DESCRIPTION("Tegra124 EMC driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/platform_data/tegra_emc.h b/include/linux/platform_data/tegra_emc.h
> index df67505..2967964 100644
> --- a/include/linux/platform_data/tegra_emc.h
> +++ b/include/linux/platform_data/tegra_emc.h
> @@ -31,4 +31,27 @@ struct tegra_emc_pdata {
>   	struct tegra_emc_table *tables;
>   };
>
> +enum {
> +	TEGRA_EMC_CONSUMER_DISP1 = 0,
> +	TEGRA_EMC_CONSUMER_DISP2,
> +	TEGRA_EMC_CONSUMER_MSENC,
> +	TEGRA_EMC_CONSUMER_CAMERA,
> +	TEGRA_EMC_CONSUMER_AVP,
> +	TEGRA_EMC_CONSUMER_ISO,
> +	TEGRA_EMC_CONSUMER_LAST
> +};
> +
> +#ifdef CONFIG_TEGRA124_EMC
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate);
> +void tegra124_emc_set_floor(unsigned long freq);
> +void tegra124_emc_set_ceiling(unsigned long freq);
> +#else
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
> +{ return -ENODEV; }
> +void tegra124_emc_set_floor(unsigned long freq)
> +{ return; }
> +void tegra124_emc_set_ceiling(unsigned long freq)
> +{ return; }
> +#endif
> +
>   #endif
>

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> Adds functionality for registering memory bandwidth needs and setting
> the EMC clock rate based on that.
> 
> Also adds API for setting floor and ceiling frequency rates.

> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> new file mode 100644
> index 0000000..88e6a55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> @@ -0,0 +1,26 @@
> +Tegra124 External Memory Controller
> +
> +Properties:
> +- compatible : Should contain "nvidia,tegra124-emc".
> +- reg : Should contain the register range of the device
> +- #address-cells : Should be 1
> +- #size-cells : Should be 0
> +- nvidia,mc : phandle to the mc bus connected to EMC.
> +- clocks : phandle to EMC, EMC shared bus override, and all parent clocks.
> +- clock-names : name of each clock.
> +- nvidia,pmc : phandle to the PMC syscon node.
> +- max-clock-frequency : optional, specifies the maximum EMC rate in kHz.
> +
> +Child device nodes describe the memory settings for different configurations and
> +clock rates.

How do the child nodes do that? The binding needs to specify the format
of the child node. This binding looks quite anaemic vs.
Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
would expect that this binding needs all the EMC register data from the
tegra20-emc binding too. Can the two bindings be identical?

Can you explain what the nvidia,mc and nvidia,pmc references are needed
for? Hopefully, this driver isn't going to reach into those devices and
touch their registers directly.

> diff --git a/include/linux/platform_data/tegra_emc.h b/include/linux/platform_data/tegra_emc.h

A header file that defines platform data format isn't the correct place
to put the definitions of public APIs. I'd expect something more like
<linux/tegra-soc.h>.

> +#ifdef CONFIG_TEGRA124_EMC
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate);
> +void tegra124_emc_set_floor(unsigned long freq);
> +void tegra124_emc_set_ceiling(unsigned long freq);
> +#else
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
> +{ return -ENODEV; }
> +void tegra124_emc_set_floor(unsigned long freq)
> +{ return; }
> +void tegra124_emc_set_ceiling(unsigned long freq)
> +{ return; }
> +#endif

I'll repeat what I said off-list so that we can have the whole
conversation on the list:

That looks like a custom Tegra-specific API. I think it'd be much better
to integrate this into the common clock framework as a standard clock
constraints API. There are other use-cases for clock constraints besides
EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
SoCs too).

See https://lkml.org/lkml/2014/5/16/569 for some previous discussion on
this topic.

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Tomeu Vizoso]

On 06/16/2014 10:02 PM, Stephen Warren wrote:
> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>> +
>> +Child device nodes describe the memory settings for different configurations and
>> +clock rates.
>
> How do the child nodes do that? The binding needs to specify the format
> of the child node.

Sorry, that file was sent before I had finished removing the bits from 
downstream that aren't needed yet. There's no current need for any child 
nodes.

> This binding looks quite anaemic vs.
> Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
> would expect that this binding needs all the EMC register data from the
> tegra20-emc binding too. Can the two bindings be identical?

There's even less stuff needed right now, as all what ultimately the EMC 
driver does is call clk_set_rate on the EMC clock. As the T124 EMC 
driver gains more features, they should get more similar.

> Can you explain what the nvidia,mc and nvidia,pmc references are needed
> for? Hopefully, this driver isn't going to reach into those devices and
> touch their registers directly.

Not really needed, see above.

>> diff --git a/include/linux/platform_data/tegra_emc.h b/include/linux/platform_data/tegra_emc.h
>
> A header file that defines platform data format isn't the correct place
> to put the definitions of public APIs. I'd expect something more like
> <linux/tegra-soc.h>.

Sounds better indeed, thanks.

>> +#ifdef CONFIG_TEGRA124_EMC
>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate);
>> +void tegra124_emc_set_floor(unsigned long freq);
>> +void tegra124_emc_set_ceiling(unsigned long freq);
>> +#else
>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
>> +{ return -ENODEV; }
>> +void tegra124_emc_set_floor(unsigned long freq)
>> +{ return; }
>> +void tegra124_emc_set_ceiling(unsigned long freq)
>> +{ return; }
>> +#endif
>
> I'll repeat what I said off-list so that we can have the whole
> conversation on the list:
>
> That looks like a custom Tegra-specific API. I think it'd be much better
> to integrate this into the common clock framework as a standard clock
> constraints API. There are other use-cases for clock constraints besides
> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
> SoCs too).

Yes, I wrote a bit in the cover letter about our requirements and how 
they map to the CCF. Could you please comment on that?

Thanks,

Tomeu

> See https://lkml.org/lkml/2014/5/16/569 for some previous discussion on
> this topic.

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:

>> This binding looks quite anaemic vs.
>> Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
>> would expect that this binding needs all the EMC register data from the
>> tegra20-emc binding too. Can the two bindings be identical?
> 
> There's even less stuff needed right now, as all what ultimately the EMC
> driver does is call clk_set_rate on the EMC clock. As the T124 EMC
> driver gains more features, they should get more similar.

IIRC, even changing the EMC clock rate requires modifying the memory
controller's programming (e.g. delays/taps/tuning etc.). That's exactly
what the more complex stuff in the nvidia,tegra20-emc.txt is all about.
I not convinced that a driver that just modifies the clock rate without
adjusting the EMC programming will work reliably.

>>> +#ifdef CONFIG_TEGRA124_EMC
>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>> long rate);
>>> +void tegra124_emc_set_floor(unsigned long freq);
>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>> +#else
>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>> long rate)
>>> +{ return -ENODEV; }
>>> +void tegra124_emc_set_floor(unsigned long freq)
>>> +{ return; }
>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>> +{ return; }
>>> +#endif
>>
>> I'll repeat what I said off-list so that we can have the whole
>> conversation on the list:
>>
>> That looks like a custom Tegra-specific API. I think it'd be much better
>> to integrate this into the common clock framework as a standard clock
>> constraints API. There are other use-cases for clock constraints besides
>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>> SoCs too).
> 
> Yes, I wrote a bit in the cover letter about our requirements and how
> they map to the CCF. Could you please comment on that?

My comments remain the same. I believe this is something that belongs in
the clock driver, or at the least, some API that takes a struct clock as
its parameter, so that drivers can use the existing DT clock lookup
mechanism.

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Mikko Perttunen]

On 06/17/2014 07:15 PM, Stephen Warren wrote:
> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>
>>> This binding looks quite anaemic vs.
>>> Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
>>> would expect that this binding needs all the EMC register data from the
>>> tegra20-emc binding too. Can the two bindings be identical?
>>
>> There's even less stuff needed right now, as all what ultimately the EMC
>> driver does is call clk_set_rate on the EMC clock. As the T124 EMC
>> driver gains more features, they should get more similar.
>
> IIRC, even changing the EMC clock rate requires modifying the memory
> controller's programming (e.g. delays/taps/tuning etc.). That's exactly
> what the more complex stuff in the nvidia,tegra20-emc.txt is all about.
> I not convinced that a driver that just modifies the clock rate without
> adjusting the EMC programming will work reliably.

Indeed, changing the EMC clock rate is a somewhat complicated sequence 
of ~10 steps. The kernel even won't let one the rate be change directly, 
as the change would be propagated to PLL_M which cannot have its rate 
changed while it is enabled. I suppose the sequence should be hidden in 
the EMC clk's set_rate implementation, which I guess would leave just 
the rate policy to this driver.

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Tomeu Vizoso]

On 06/17/2014 06:15 PM, Stephen Warren wrote:
> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>> long rate);
>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>> +#else
>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>> long rate)
>>>> +{ return -ENODEV; }
>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>> +{ return; }
>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>> +{ return; }
>>>> +#endif
>>>
>>> I'll repeat what I said off-list so that we can have the whole
>>> conversation on the list:
>>>
>>> That looks like a custom Tegra-specific API. I think it'd be much better
>>> to integrate this into the common clock framework as a standard clock
>>> constraints API. There are other use-cases for clock constraints besides
>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>> SoCs too).
>>
>> Yes, I wrote a bit in the cover letter about our requirements and how
>> they map to the CCF. Could you please comment on that?
>
> My comments remain the same. I believe this is something that belongs in
> the clock driver, or at the least, some API that takes a struct clock as
> its parameter, so that drivers can use the existing DT clock lookup
> mechanism.

Ok, let me put this strawman here to see if I have gotten close to what 
you have in mind:

* add per-client accounting (Rabin's patches referenced before)

* add clk_set_floor, to be used by cpufreq, load stats, etc.

* add clk_set_ceiling, to be used by battery drivers, thermal, etc.

* an EMC driver would collect bandwidth and latency requests from 
consumers and call clk_set_floor on the EMC clock.

* the EMC driver would also register for rate change notifications in 
the EMC clock and would update the latency allowance registers at that 
point.

How does it sound?

Regards,

Tomeu

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>> long rate);
>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>> +#else
>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>> long rate)
>>>>> +{ return -ENODEV; }
>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>> +{ return; }
>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>> +{ return; }
>>>>> +#endif
>>>>
>>>> I'll repeat what I said off-list so that we can have the whole
>>>> conversation on the list:
>>>>
>>>> That looks like a custom Tegra-specific API. I think it'd be much
>>>> better
>>>> to integrate this into the common clock framework as a standard clock
>>>> constraints API. There are other use-cases for clock constraints
>>>> besides
>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>> SoCs too).
>>>
>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>> they map to the CCF. Could you please comment on that?
>>
>> My comments remain the same. I believe this is something that belongs in
>> the clock driver, or at the least, some API that takes a struct clock as
>> its parameter, so that drivers can use the existing DT clock lookup
>> mechanism.
> 
> Ok, let me put this strawman here to see if I have gotten close to what
> you have in mind:
> 
> * add per-client accounting (Rabin's patches referenced before)
> 
> * add clk_set_floor, to be used by cpufreq, load stats, etc.
> 
> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.

Yes. I'd expect those to be maintained per-client, and so the clock core
(or whatever higher level code implements clk_set_floor/ceiling)
performs the logic that "blends" together all the different requests
from different clients.

As an aside, for audio usage, I would expect clk_set_rate to be a
per-client (rather than per HW clock) operation too, and to error out if
one client says it wants to set pll_a to the rate needed for
44.1KHz-based audio and a different client wants the rate for
48KHz-based audio.

> * an EMC driver would collect bandwidth and latency requests from
> consumers and call clk_set_floor on the EMC clock.
> 
> * the EMC driver would also register for rate change notifications in
> the EMC clock and would update the latency allowance registers at that
> point.
> 
> How does it sound?

At a high level, yes this sounds about right to me.

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3073 bytes --]

On Wed, Jun 18, 2014 at 11:46:49AM -0600, Stephen Warren wrote:
> On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
> > On 06/17/2014 06:15 PM, Stephen Warren wrote:
> >> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> >>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
> >>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> >>>>> +#ifdef CONFIG_TEGRA124_EMC
> >>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>> long rate);
> >>>>> +void tegra124_emc_set_floor(unsigned long freq);
> >>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
> >>>>> +#else
> >>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>> long rate)
> >>>>> +{ return -ENODEV; }
> >>>>> +void tegra124_emc_set_floor(unsigned long freq)
> >>>>> +{ return; }
> >>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
> >>>>> +{ return; }
> >>>>> +#endif
> >>>>
> >>>> I'll repeat what I said off-list so that we can have the whole
> >>>> conversation on the list:
> >>>>
> >>>> That looks like a custom Tegra-specific API. I think it'd be much
> >>>> better
> >>>> to integrate this into the common clock framework as a standard clock
> >>>> constraints API. There are other use-cases for clock constraints
> >>>> besides
> >>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
> >>>> SoCs too).
> >>>
> >>> Yes, I wrote a bit in the cover letter about our requirements and how
> >>> they map to the CCF. Could you please comment on that?
> >>
> >> My comments remain the same. I believe this is something that belongs in
> >> the clock driver, or at the least, some API that takes a struct clock as
> >> its parameter, so that drivers can use the existing DT clock lookup
> >> mechanism.
> > 
> > Ok, let me put this strawman here to see if I have gotten close to what
> > you have in mind:
> > 
> > * add per-client accounting (Rabin's patches referenced before)
> > 
> > * add clk_set_floor, to be used by cpufreq, load stats, etc.
> > 
> > * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
> 
> Yes. I'd expect those to be maintained per-client, and so the clock core
> (or whatever higher level code implements clk_set_floor/ceiling)
> performs the logic that "blends" together all the different requests
> from different clients.
> 
> As an aside, for audio usage, I would expect clk_set_rate to be a
> per-client (rather than per HW clock) operation too, and to error out if
> one client says it wants to set pll_a to the rate needed for
> 44.1KHz-based audio and a different client wants the rate for
> 48KHz-based audio.

From what I remember, Mike was fairly strongly opposing the idea of
virtual clocks, but what you're proposing here sounds like it would
assume the existence of virtual clocks. clk_set_rate() per client
doesn't work with the current API as I understand it.

Or perhaps what you're proposing isn't about the individual clocks at
all but rather about a mechanism to express constraints for a set of
clocks?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

[-- Attachment #1: Type: text/plain, Size: 3828 bytes --]

On 06/18/2014 04:03 PM, Thierry Reding wrote:
> On Wed, Jun 18, 2014 at 11:46:49AM -0600, Stephen Warren wrote:
>> On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
>>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate);
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>>>> +#else
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate)
>>>>>>> +{ return -ENODEV; }
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +#endif
>>>>>>
>>>>>> I'll repeat what I said off-list so that we can have the whole
>>>>>> conversation on the list:
>>>>>>
>>>>>> That looks like a custom Tegra-specific API. I think it'd be much
>>>>>> better
>>>>>> to integrate this into the common clock framework as a standard clock
>>>>>> constraints API. There are other use-cases for clock constraints
>>>>>> besides
>>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>>>> SoCs too).
>>>>>
>>>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>>>> they map to the CCF. Could you please comment on that?
>>>>
>>>> My comments remain the same. I believe this is something that belongs in
>>>> the clock driver, or at the least, some API that takes a struct clock as
>>>> its parameter, so that drivers can use the existing DT clock lookup
>>>> mechanism.
>>>
>>> Ok, let me put this strawman here to see if I have gotten close to what
>>> you have in mind:
>>>
>>> * add per-client accounting (Rabin's patches referenced before)
>>>
>>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>>
>>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>
>> Yes. I'd expect those to be maintained per-client, and so the clock core
>> (or whatever higher level code implements clk_set_floor/ceiling)
>> performs the logic that "blends" together all the different requests
>> from different clients.
>>
>> As an aside, for audio usage, I would expect clk_set_rate to be a
>> per-client (rather than per HW clock) operation too, and to error out if
>> one client says it wants to set pll_a to the rate needed for
>> 44.1KHz-based audio and a different client wants the rate for
>> 48KHz-based audio.
> 
> From what I remember, Mike was fairly strongly opposing the idea of
> virtual clocks, but what you're proposing here sounds like it would
> assume the existence of virtual clocks. clk_set_rate() per client
> doesn't work with the current API as I understand it.
> 
> Or perhaps what you're proposing isn't about the individual clocks at
> all but rather about a mechanism to express constraints for a set of
> clocks?

This doesn't have anything to do with virtual clocks. As you mention,
it's just about constraints.

One user of clock "cpu" wants min rate 216MHz. Another wants max rate
1GHz. cpufreq will request some rate between the 2, or be capped to
those limits. These set of imposed constraints would need to be stored
per client of the clock, not per HW clock, since many clients could set
different max rates (e.g. thermal throttle 1.5GHz due to temperature,
CPU policy 1GHz due to the user selecting low CPU power, etc.)

Similarly for audio, of there are N clients of 1 clock/PLL, and they
each want the PLL to run at a different rate, something needs to detect
that and deny it.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4276 bytes --]

On Wed, Jun 18, 2014 at 04:09:06PM -0600, Stephen Warren wrote:
> On 06/18/2014 04:03 PM, Thierry Reding wrote:
> > On Wed, Jun 18, 2014 at 11:46:49AM -0600, Stephen Warren wrote:
> >> On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
> >>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
> >>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> >>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
> >>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> >>>>>>> +#ifdef CONFIG_TEGRA124_EMC
> >>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>>>> long rate);
> >>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
> >>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
> >>>>>>> +#else
> >>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>>>> long rate)
> >>>>>>> +{ return -ENODEV; }
> >>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
> >>>>>>> +{ return; }
> >>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
> >>>>>>> +{ return; }
> >>>>>>> +#endif
> >>>>>>
> >>>>>> I'll repeat what I said off-list so that we can have the whole
> >>>>>> conversation on the list:
> >>>>>>
> >>>>>> That looks like a custom Tegra-specific API. I think it'd be much
> >>>>>> better
> >>>>>> to integrate this into the common clock framework as a standard clock
> >>>>>> constraints API. There are other use-cases for clock constraints
> >>>>>> besides
> >>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
> >>>>>> SoCs too).
> >>>>>
> >>>>> Yes, I wrote a bit in the cover letter about our requirements and how
> >>>>> they map to the CCF. Could you please comment on that?
> >>>>
> >>>> My comments remain the same. I believe this is something that belongs in
> >>>> the clock driver, or at the least, some API that takes a struct clock as
> >>>> its parameter, so that drivers can use the existing DT clock lookup
> >>>> mechanism.
> >>>
> >>> Ok, let me put this strawman here to see if I have gotten close to what
> >>> you have in mind:
> >>>
> >>> * add per-client accounting (Rabin's patches referenced before)
> >>>
> >>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
> >>>
> >>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
> >>
> >> Yes. I'd expect those to be maintained per-client, and so the clock core
> >> (or whatever higher level code implements clk_set_floor/ceiling)
> >> performs the logic that "blends" together all the different requests
> >> from different clients.
> >>
> >> As an aside, for audio usage, I would expect clk_set_rate to be a
> >> per-client (rather than per HW clock) operation too, and to error out if
> >> one client says it wants to set pll_a to the rate needed for
> >> 44.1KHz-based audio and a different client wants the rate for
> >> 48KHz-based audio.
> > 
> > From what I remember, Mike was fairly strongly opposing the idea of
> > virtual clocks, but what you're proposing here sounds like it would
> > assume the existence of virtual clocks. clk_set_rate() per client
> > doesn't work with the current API as I understand it.
> > 
> > Or perhaps what you're proposing isn't about the individual clocks at
> > all but rather about a mechanism to express constraints for a set of
> > clocks?
> 
> This doesn't have anything to do with virtual clocks. As you mention,
> it's just about constraints.
> 
> One user of clock "cpu" wants min rate 216MHz. Another wants max rate
> 1GHz. cpufreq will request some rate between the 2, or be capped to
> those limits. These set of imposed constraints would need to be stored
> per client of the clock, not per HW clock, since many clients could set
> different max rates (e.g. thermal throttle 1.5GHz due to temperature,
> CPU policy 1GHz due to the user selecting low CPU power, etc.)
> 
> Similarly for audio, of there are N clients of 1 clock/PLL, and they
> each want the PLL to run at a different rate, something needs to detect
> that and deny it.

I'm wondering how this should work with the current API. Could the clock
core be modified to return a per-client struct clk * that references the
hardware clock internally? Or do we need to add a new API?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

[-- Attachment #1.1: Type: text/plain, Size: 1720 bytes --]

On 06/18/2014 05:14 PM, Thierry Reding wrote:
> On Wed, Jun 18, 2014 at 04:09:06PM -0600, Stephen Warren wrote:
>> On 06/18/2014 04:03 PM, Thierry Reding wrote:
...
>>> From what I remember, Mike was fairly strongly opposing the idea of
>>> virtual clocks, but what you're proposing here sounds like it would
>>> assume the existence of virtual clocks. clk_set_rate() per client
>>> doesn't work with the current API as I understand it.
>>>
>>> Or perhaps what you're proposing isn't about the individual clocks at
>>> all but rather about a mechanism to express constraints for a set of
>>> clocks?
>>
>> This doesn't have anything to do with virtual clocks. As you mention,
>> it's just about constraints.
>>
>> One user of clock "cpu" wants min rate 216MHz. Another wants max rate
>> 1GHz. cpufreq will request some rate between the 2, or be capped to
>> those limits. These set of imposed constraints would need to be stored
>> per client of the clock, not per HW clock, since many clients could set
>> different max rates (e.g. thermal throttle 1.5GHz due to temperature,
>> CPU policy 1GHz due to the user selecting low CPU power, etc.)
>>
>> Similarly for audio, of there are N clients of 1 clock/PLL, and they
>> each want the PLL to run at a different rate, something needs to detect
>> that and deny it.
> 
> I'm wondering how this should work with the current API. Could the clock
> core be modified to return a per-client struct clk * that references the
> hardware clock internally? Or do we need to add a new API?

I would assume the we can just change struct clk and hide the details
from any driver. Hopefully only clock-core and clock-drivers would need
any changes.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2566 bytes --]

On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
> On 06/17/2014 06:15 PM, Stephen Warren wrote:
> >On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> >>On 06/16/2014 10:02 PM, Stephen Warren wrote:
> >>>On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> >>>>+#ifdef CONFIG_TEGRA124_EMC
> >>>>+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>long rate);
> >>>>+void tegra124_emc_set_floor(unsigned long freq);
> >>>>+void tegra124_emc_set_ceiling(unsigned long freq);
> >>>>+#else
> >>>>+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>long rate)
> >>>>+{ return -ENODEV; }
> >>>>+void tegra124_emc_set_floor(unsigned long freq)
> >>>>+{ return; }
> >>>>+void tegra124_emc_set_ceiling(unsigned long freq)
> >>>>+{ return; }
> >>>>+#endif
> >>>
> >>>I'll repeat what I said off-list so that we can have the whole
> >>>conversation on the list:
> >>>
> >>>That looks like a custom Tegra-specific API. I think it'd be much better
> >>>to integrate this into the common clock framework as a standard clock
> >>>constraints API. There are other use-cases for clock constraints besides
> >>>EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
> >>>SoCs too).
> >>
> >>Yes, I wrote a bit in the cover letter about our requirements and how
> >>they map to the CCF. Could you please comment on that?
> >
> >My comments remain the same. I believe this is something that belongs in
> >the clock driver, or at the least, some API that takes a struct clock as
> >its parameter, so that drivers can use the existing DT clock lookup
> >mechanism.
> 
> Ok, let me put this strawman here to see if I have gotten close to what you
> have in mind:
> 
> * add per-client accounting (Rabin's patches referenced before)
> 
> * add clk_set_floor, to be used by cpufreq, load stats, etc.
> 
> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
> 
> * an EMC driver would collect bandwidth and latency requests from consumers
> and call clk_set_floor on the EMC clock.
> 
> * the EMC driver would also register for rate change notifications in the
> EMC clock and would update the latency allowance registers at that point.

Latency allowance registers are part of the MC rather than the EMC. So I
think we have two options: a) have a unified driver for MC and EMC or b)
provide two parts of the API in two drivers.

Or perhaps c), create a generic framework that both MC and EMC can
register with (bandwidth for EMC, latency for MC).

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stéphane Marchesin]

On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>> >On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>> >>On 06/16/2014 10:02 PM, Stephen Warren wrote:
>> >>>On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>> >>>>+#ifdef CONFIG_TEGRA124_EMC
>> >>>>+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>> >>>>long rate);
>> >>>>+void tegra124_emc_set_floor(unsigned long freq);
>> >>>>+void tegra124_emc_set_ceiling(unsigned long freq);
>> >>>>+#else
>> >>>>+int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>> >>>>long rate)
>> >>>>+{ return -ENODEV; }
>> >>>>+void tegra124_emc_set_floor(unsigned long freq)
>> >>>>+{ return; }
>> >>>>+void tegra124_emc_set_ceiling(unsigned long freq)
>> >>>>+{ return; }
>> >>>>+#endif
>> >>>
>> >>>I'll repeat what I said off-list so that we can have the whole
>> >>>conversation on the list:
>> >>>
>> >>>That looks like a custom Tegra-specific API. I think it'd be much better
>> >>>to integrate this into the common clock framework as a standard clock
>> >>>constraints API. There are other use-cases for clock constraints besides
>> >>>EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>> >>>SoCs too).
>> >>
>> >>Yes, I wrote a bit in the cover letter about our requirements and how
>> >>they map to the CCF. Could you please comment on that?
>> >
>> >My comments remain the same. I believe this is something that belongs in
>> >the clock driver, or at the least, some API that takes a struct clock as
>> >its parameter, so that drivers can use the existing DT clock lookup
>> >mechanism.
>>
>> Ok, let me put this strawman here to see if I have gotten close to what you
>> have in mind:
>>
>> * add per-client accounting (Rabin's patches referenced before)
>>
>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>
>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>
>> * an EMC driver would collect bandwidth and latency requests from consumers
>> and call clk_set_floor on the EMC clock.
>>
>> * the EMC driver would also register for rate change notifications in the
>> EMC clock and would update the latency allowance registers at that point.
>
> Latency allowance registers are part of the MC rather than the EMC. So I
> think we have two options: a) have a unified driver for MC and EMC or b)
> provide two parts of the API in two drivers.
>
> Or perhaps c), create a generic framework that both MC and EMC can
> register with (bandwidth for EMC, latency for MC).

Is there any motivation for keeping MC and EMC separate? In my mind,
the solution was always to handle those together.

Stéphane
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Stephen Warren]

On 06/18/2014 04:19 PM, Stéphane Marchesin wrote:
> On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
>>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate);
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>>>> +#else
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate)
>>>>>>> +{ return -ENODEV; }
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +#endif
>>>>>>
>>>>>> I'll repeat what I said off-list so that we can have the whole
>>>>>> conversation on the list:
>>>>>>
>>>>>> That looks like a custom Tegra-specific API. I think it'd be much better
>>>>>> to integrate this into the common clock framework as a standard clock
>>>>>> constraints API. There are other use-cases for clock constraints besides
>>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>>>> SoCs too).
>>>>>
>>>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>>>> they map to the CCF. Could you please comment on that?
>>>>
>>>> My comments remain the same. I believe this is something that belongs in
>>>> the clock driver, or at the least, some API that takes a struct clock as
>>>> its parameter, so that drivers can use the existing DT clock lookup
>>>> mechanism.
>>>
>>> Ok, let me put this strawman here to see if I have gotten close to what you
>>> have in mind:
>>>
>>> * add per-client accounting (Rabin's patches referenced before)
>>>
>>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>>
>>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>>
>>> * an EMC driver would collect bandwidth and latency requests from consumers
>>> and call clk_set_floor on the EMC clock.
>>>
>>> * the EMC driver would also register for rate change notifications in the
>>> EMC clock and would update the latency allowance registers at that point.
>>
>> Latency allowance registers are part of the MC rather than the EMC. So I
>> think we have two options: a) have a unified driver for MC and EMC or b)
>> provide two parts of the API in two drivers.
>>
>> Or perhaps c), create a generic framework that both MC and EMC can
>> register with (bandwidth for EMC, latency for MC).
> 
> Is there any motivation for keeping MC and EMC separate? In my mind,
> the solution was always to handle those together.

Well, they are documented as being separate HW modules in the TRM.

I know there's an interlock in HW so that when the EMC clock is changed,
the EMC registers can flip atomically to a new configuration.

I'm not aware of any similar HW interlock between MC and EMC registers.
That would imply that very tight co-ordination shouldn't be required.

Do the MC latency allowance registers /really/ need to be *very tightly*
managed whenever the EMC clock is changed, or is it just a matter of it
being a good idea to update EMC clock and MC latency allowance registers
at roughly the same time? Even if there's some co-ordination required,
maybe it can be handled rather like cpufreq notifications; use clock
pre-rate change notifications to set MC up in a way that'll work at both
old/new EMC clocks, and then clock post-rate notifications to the final
MC configuration?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4802 bytes --]

On Wed, Jun 18, 2014 at 04:33:39PM -0600, Stephen Warren wrote:
> On 06/18/2014 04:19 PM, Stéphane Marchesin wrote:
> > On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> >> On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
> >>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
> >>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> >>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
> >>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> >>>>>>> +#ifdef CONFIG_TEGRA124_EMC
> >>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>>>> long rate);
> >>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
> >>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
> >>>>>>> +#else
> >>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
> >>>>>>> long rate)
> >>>>>>> +{ return -ENODEV; }
> >>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
> >>>>>>> +{ return; }
> >>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
> >>>>>>> +{ return; }
> >>>>>>> +#endif
> >>>>>>
> >>>>>> I'll repeat what I said off-list so that we can have the whole
> >>>>>> conversation on the list:
> >>>>>>
> >>>>>> That looks like a custom Tegra-specific API. I think it'd be much better
> >>>>>> to integrate this into the common clock framework as a standard clock
> >>>>>> constraints API. There are other use-cases for clock constraints besides
> >>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
> >>>>>> SoCs too).
> >>>>>
> >>>>> Yes, I wrote a bit in the cover letter about our requirements and how
> >>>>> they map to the CCF. Could you please comment on that?
> >>>>
> >>>> My comments remain the same. I believe this is something that belongs in
> >>>> the clock driver, or at the least, some API that takes a struct clock as
> >>>> its parameter, so that drivers can use the existing DT clock lookup
> >>>> mechanism.
> >>>
> >>> Ok, let me put this strawman here to see if I have gotten close to what you
> >>> have in mind:
> >>>
> >>> * add per-client accounting (Rabin's patches referenced before)
> >>>
> >>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
> >>>
> >>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
> >>>
> >>> * an EMC driver would collect bandwidth and latency requests from consumers
> >>> and call clk_set_floor on the EMC clock.
> >>>
> >>> * the EMC driver would also register for rate change notifications in the
> >>> EMC clock and would update the latency allowance registers at that point.
> >>
> >> Latency allowance registers are part of the MC rather than the EMC. So I
> >> think we have two options: a) have a unified driver for MC and EMC or b)
> >> provide two parts of the API in two drivers.
> >>
> >> Or perhaps c), create a generic framework that both MC and EMC can
> >> register with (bandwidth for EMC, latency for MC).
> > 
> > Is there any motivation for keeping MC and EMC separate? In my mind,
> > the solution was always to handle those together.
> 
> Well, they are documented as being separate HW modules in the TRM.
> 
> I know there's an interlock in HW so that when the EMC clock is changed,
> the EMC registers can flip atomically to a new configuration.
> 
> I'm not aware of any similar HW interlock between MC and EMC registers.
> That would imply that very tight co-ordination shouldn't be required.
> 
> Do the MC latency allowance registers /really/ need to be *very tightly*
> managed whenever the EMC clock is changed, or is it just a matter of it
> being a good idea to update EMC clock and MC latency allowance registers
> at roughly the same time?

I guess depending on the timing you could get FIFO underflows if the
registers aren't updated promptly enough. Once the programming takes
effect things should be able to catch up again, but it's possible that
there could be glitches.

> Even if there's some co-ordination required,
> maybe it can be handled rather like cpufreq notifications; use clock
> pre-rate change notifications to set MC up in a way that'll work at both
> old/new EMC clocks, and then clock post-rate notifications to the final
> MC configuration?

Yes, I think something like that should work. As I understand it, the
latency allowance is dependent on the EMC frequency, so in case where
the EMC frequency is increased, adjusting in a post-rate notifier should
be fine. When the EMC frequency is decreased, then programming the
latency allowance registers in a pre-rate notifier should allow glitch-
free transition.

Note that this is all purely theoretical knowledge, so I have no idea if
it'll actually work like that in practice.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1836 bytes --]

On Tue, Jun 17, 2014 at 02:16:06PM +0200, Tomeu Vizoso wrote:
> On 06/16/2014 10:02 PM, Stephen Warren wrote:
> >On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> >>+
> >>+Child device nodes describe the memory settings for different configurations and
> >>+clock rates.
> >
> >How do the child nodes do that? The binding needs to specify the format
> >of the child node.
> 
> Sorry, that file was sent before I had finished removing the bits from
> downstream that aren't needed yet. There's no current need for any child
> nodes.
> 
> >This binding looks quite anaemic vs.
> >Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
> >would expect that this binding needs all the EMC register data from the
> >tegra20-emc binding too. Can the two bindings be identical?
> 
> There's even less stuff needed right now, as all what ultimately the EMC
> driver does is call clk_set_rate on the EMC clock. As the T124 EMC driver
> gains more features, they should get more similar.
> 
> >Can you explain what the nvidia,mc and nvidia,pmc references are needed
> >for? Hopefully, this driver isn't going to reach into those devices and
> >touch their registers directly.
> 
> Not really needed, see above.

I've been working on a prototype driver for the memory controller. Part
of what I've added is programming of the latency allowance registers (it
doesn't yet expose an API to do so yet, though). I think that needs to
eventually take into account the EMC frequency (and needs to be notified
of changes to the same).

Without having thought this through very thoroughly, I suspect that
rather than referencing the MC from the EMC it might be better to have
the MC register with the EMC for notifications.

But perhaps there are other services from MC that EMC needs to work?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver

[Peter De Schrijver]

On Wed, Jun 18, 2014 at 12:35:27AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jun 17, 2014 at 02:16:06PM +0200, Tomeu Vizoso wrote:
> > On 06/16/2014 10:02 PM, Stephen Warren wrote:
> > >On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> > >>+
> > >>+Child device nodes describe the memory settings for different configurations and
> > >>+clock rates.
> > >
> > >How do the child nodes do that? The binding needs to specify the format
> > >of the child node.
> > 
> > Sorry, that file was sent before I had finished removing the bits from
> > downstream that aren't needed yet. There's no current need for any child
> > nodes.
> > 
> > >This binding looks quite anaemic vs.
> > >Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
> > >would expect that this binding needs all the EMC register data from the
> > >tegra20-emc binding too. Can the two bindings be identical?
> > 
> > There's even less stuff needed right now, as all what ultimately the EMC
> > driver does is call clk_set_rate on the EMC clock. As the T124 EMC driver
> > gains more features, they should get more similar.
> > 
> > >Can you explain what the nvidia,mc and nvidia,pmc references are needed
> > >for? Hopefully, this driver isn't going to reach into those devices and
> > >touch their registers directly.
> > 
> > Not really needed, see above.
> 
> I've been working on a prototype driver for the memory controller. Part
> of what I've added is programming of the latency allowance registers (it
> doesn't yet expose an API to do so yet, though). I think that needs to
> eventually take into account the EMC frequency (and needs to be notified
> of changes to the same).
> 
> Without having thought this through very thoroughly, I suspect that
> rather than referencing the MC from the EMC it might be better to have
> the MC register with the EMC for notifications.
> 
> But perhaps there are other services from MC that EMC needs to work?
> 

If the emc frequency change is modelled as a clock, you could use CCF
notifications for this?

Cheers,

Peter.

[RFC PATCH 2/4] ARM: tegra: Add Tegra124 EMC support

[Tomeu Vizoso]

Add a device node for the EMC found on Tegra124.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 6e6bc4e..5aa42ff 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -449,6 +449,15 @@
 		clock-names = "pclk", "clk32k_in";
 	};
 
+	memory-controller@7001b000 {
+		compatible = "nvidia,tegra124-emc";
+		reg = <0x00 0x7001b000 0x00 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&tegra_car 57>;
+		clock-names = "emc";
+	};
+
 	sdhci@0,700b0000 {
 		compatible = "nvidia,tegra124-sdhci";
 		reg = <0x0 0x700b0000 0x0 0x200>;
-- 
1.9.3

Re: [RFC PATCH 2/4] ARM: tegra: Add Tegra124 EMC support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 334 bytes --]

On Mon, Jun 16, 2014 at 03:35:11PM +0200, Tomeu Vizoso wrote:
[...]
> +	memory-controller@7001b000 {
> +		compatible = "nvidia,tegra124-emc";
> +		reg = <0x00 0x7001b000 0x00 0x1000>;

For consistency, perhaps s/0x00/0x0/?

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&tegra_car 57>;

TEGRA124_CLK_EMC?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 3/4] drm/tegra: Request memory bandwidth for the display controller

[Tomeu Vizoso]

Request it based solely on the current mode's refresh rate. More
accurate requirements can be requested in future patches.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/gpu/drm/tegra/dc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index ef40381..6739d69 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/debugfs.h>
 #include <linux/reset.h>
+#include <linux/platform_data/tegra_emc.h>
 
 #include "dc.h"
 #include "drm.h"
@@ -683,6 +684,8 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	drm_vblank_off(drm, dc->pipe);
+
+	tegra124_emc_reserve_bandwidth(TEGRA_EMC_CONSUMER_DISP1, 0);
 }
 
 static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -769,6 +772,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 	struct tegra_dc *dc = to_tegra_dc(crtc);
 	struct tegra_dc_window window;
 	u32 value;
+	unsigned long bandwidth;
 	int err;
 
 	drm_vblank_pre_modeset(crtc->dev, dc->pipe);
@@ -809,6 +813,11 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc,
 	if (err < 0)
 		dev_err(dc->dev, "failed to enable root plane\n");
 
+	bandwidth = mode->clock * window.bits_per_pixel / 8;
+	err = tegra124_emc_reserve_bandwidth(TEGRA_EMC_CONSUMER_DISP1, bandwidth);
+	if (err)
+		dev_err(dc->dev, "failed to reserve EMC bandwidth: %d\n", err);
+
 	return 0;
 }
 
-- 
1.9.3

Re: [RFC PATCH 3/4] drm/tegra: Request memory bandwidth for the display controller

[Stephen Warren]

On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> Request it based solely on the current mode's refresh rate. More
> accurate requirements can be requested in future patches.

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c

> +	bandwidth = mode->clock * window.bits_per_pixel / 8;
> +	err = tegra124_emc_reserve_bandwidth(TEGRA_EMC_CONSUMER_DISP1, bandwidth);

DISP1 shouldn't be hard-coded here; the code should use DISP1 or DISP2
based on head or DC identity. We certainly have some boards capable of
dual-head operation.

Re: [RFC PATCH 3/4] drm/tegra: Request memory bandwidth for the display controller

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 991 bytes --]

On Mon, Jun 16, 2014 at 02:06:43PM -0600, Stephen Warren wrote:
> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> > Request it based solely on the current mode's refresh rate. More
> > accurate requirements can be requested in future patches.
> 
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> 
> > +	bandwidth = mode->clock * window.bits_per_pixel / 8;
> > +	err = tegra124_emc_reserve_bandwidth(TEGRA_EMC_CONSUMER_DISP1, bandwidth);
> 
> DISP1 shouldn't be hard-coded here; the code should use DISP1 or DISP2
> based on head or DC identity. We certainly have some boards capable of
> dual-head operation.

On a general note, I think perhaps a better way to represent this in an
API, and perhaps this would help with making the API more generic, too,
would be to make drivers request some sort of handle in .probe() and use
that handle subsequently when making requests. That's somewhat analogous
to the PM QoS' struct pm_qos_request.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 4/4] cpufreq: tegra: Register a minimum EMC frequency based on the CPU clock

[Tomeu Vizoso]

Instead of setting a direct correlation to the CPU frequency. This allows
for other devices to influence the final effective EMC frequency.

In the future, this should be done instead by an ACTMON driver,
which would also take load stats into account when calculating the
floor EMC frequency.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/cpufreq/tegra-cpufreq.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 8084c7f..64935f8 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -26,6 +26,7 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/platform_data/tegra_emc.h>
 
 static struct cpufreq_frequency_table freq_table[] = {
 	{ .frequency = 216000 },
@@ -44,7 +45,6 @@ static struct cpufreq_frequency_table freq_table[] = {
 static struct clk *cpu_clk;
 static struct clk *pll_x_clk;
 static struct clk *pll_p_clk;
-static struct clk *emc_clk;
 static bool pll_x_prepared;
 
 static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
@@ -96,15 +96,15 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 	int ret = 0;
 
 	/*
-	 * Vote on memory bus frequency based on cpu frequency
+	 * Set minimum memory bus frequency based on cpu frequency
 	 * This sets the minimum frequency, display or avp may request higher
 	 */
 	if (rate >= 816000)
-		clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
+		tegra124_emc_set_floor(600000000); /* cpu 816 MHz, emc max */
 	else if (rate >= 456000)
-		clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
+		tegra124_emc_set_floor(300000000); /* cpu 456 MHz, emc 150Mhz */
 	else
-		clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
+		tegra124_emc_set_floor(100000000);  /* emc 50Mhz */
 
 	/*
 	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
@@ -141,14 +141,12 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
 	if (policy->cpu >= NUM_CPUS)
 		return -EINVAL;
 
-	clk_prepare_enable(emc_clk);
 	clk_prepare_enable(cpu_clk);
 
 	/* FIXME: what's the actual transition time? */
 	ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
 	if (ret) {
 		clk_disable_unprepare(cpu_clk);
-		clk_disable_unprepare(emc_clk);
 		return ret;
 	}
 
@@ -160,7 +158,6 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
 static int tegra_cpu_exit(struct cpufreq_policy *policy)
 {
 	clk_disable_unprepare(cpu_clk);
-	clk_disable_unprepare(emc_clk);
 	return 0;
 }
 
@@ -194,19 +191,12 @@ static int __init tegra_cpufreq_init(void)
 	if (IS_ERR(pll_p_clk))
 		return PTR_ERR(pll_p_clk);
 
-	emc_clk = clk_get_sys("cpu", "emc");
-	if (IS_ERR(emc_clk)) {
-		clk_put(cpu_clk);
-		return PTR_ERR(emc_clk);
-	}
-
 	return cpufreq_register_driver(&tegra_cpufreq_driver);
 }
 
 static void __exit tegra_cpufreq_exit(void)
 {
         cpufreq_unregister_driver(&tegra_cpufreq_driver);
-	clk_put(emc_clk);
 	clk_put(cpu_clk);
 }
 
-- 
1.9.3

Re: [RFC PATCH 4/4] cpufreq: tegra: Register a minimum EMC frequency based on the CPU clock

[Mikko Perttunen]

The tegra-cpufreq driver is only for Tegra20, an upcoming driver for 
Tegra124 will be separate, so this is not needed.

Thanks,
- Mikko

On 06/16/2014 04:35 PM, Tomeu Vizoso wrote:
> Instead of setting a direct correlation to the CPU frequency. This allows
> for other devices to influence the final effective EMC frequency.
>
> In the future, this should be done instead by an ACTMON driver,
> which would also take load stats into account when calculating the
> floor EMC frequency.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>   drivers/cpufreq/tegra-cpufreq.c | 20 +++++---------------
>   1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
> index 8084c7f..64935f8 100644
> --- a/drivers/cpufreq/tegra-cpufreq.c
> +++ b/drivers/cpufreq/tegra-cpufreq.c
> @@ -26,6 +26,7 @@
>   #include <linux/err.h>
>   #include <linux/clk.h>
>   #include <linux/io.h>
> +#include <linux/platform_data/tegra_emc.h>
>
>   static struct cpufreq_frequency_table freq_table[] = {
>   	{ .frequency = 216000 },
> @@ -44,7 +45,6 @@ static struct cpufreq_frequency_table freq_table[] = {
>   static struct clk *cpu_clk;
>   static struct clk *pll_x_clk;
>   static struct clk *pll_p_clk;
> -static struct clk *emc_clk;
>   static bool pll_x_prepared;
>
>   static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
> @@ -96,15 +96,15 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>   	int ret = 0;
>
>   	/*
> -	 * Vote on memory bus frequency based on cpu frequency
> +	 * Set minimum memory bus frequency based on cpu frequency
>   	 * This sets the minimum frequency, display or avp may request higher
>   	 */
>   	if (rate >= 816000)
> -		clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
> +		tegra124_emc_set_floor(600000000); /* cpu 816 MHz, emc max */
>   	else if (rate >= 456000)
> -		clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
> +		tegra124_emc_set_floor(300000000); /* cpu 456 MHz, emc 150Mhz */
>   	else
> -		clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
> +		tegra124_emc_set_floor(100000000);  /* emc 50Mhz */
>
>   	/*
>   	 * target freq == pll_p, don't need to take extra reference to pll_x_clk
> @@ -141,14 +141,12 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
>   	if (policy->cpu >= NUM_CPUS)
>   		return -EINVAL;
>
> -	clk_prepare_enable(emc_clk);
>   	clk_prepare_enable(cpu_clk);
>
>   	/* FIXME: what's the actual transition time? */
>   	ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
>   	if (ret) {
>   		clk_disable_unprepare(cpu_clk);
> -		clk_disable_unprepare(emc_clk);
>   		return ret;
>   	}
>
> @@ -160,7 +158,6 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
>   static int tegra_cpu_exit(struct cpufreq_policy *policy)
>   {
>   	clk_disable_unprepare(cpu_clk);
> -	clk_disable_unprepare(emc_clk);
>   	return 0;
>   }
>
> @@ -194,19 +191,12 @@ static int __init tegra_cpufreq_init(void)
>   	if (IS_ERR(pll_p_clk))
>   		return PTR_ERR(pll_p_clk);
>
> -	emc_clk = clk_get_sys("cpu", "emc");
> -	if (IS_ERR(emc_clk)) {
> -		clk_put(cpu_clk);
> -		return PTR_ERR(emc_clk);
> -	}
> -
>   	return cpufreq_register_driver(&tegra_cpufreq_driver);
>   }
>
>   static void __exit tegra_cpufreq_exit(void)
>   {
>           cpufreq_unregister_driver(&tegra_cpufreq_driver);
> -	clk_put(emc_clk);
>   	clk_put(cpu_clk);
>   }
>
>