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
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
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) moduleavailable 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
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.
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.
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.
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.
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
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.
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
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.
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
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.
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
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
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?
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
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
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.
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>;
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
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; }
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 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
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); }
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*/ if (rate >= 816000)
- This sets the minimum frequency, display or avp may request higher
clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
tegra124_emc_set_floor(600000000); /* cpu 816 MHz, emc max */
clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
tegra124_emc_set_floor(300000000); /* cpu 456 MHz, emc 150Mhz */
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); }
dri-devel@lists.freedesktop.org
-
Mikko Perttunen -
Peter De Schrijver -
Stephen Warren -
Stéphane Marchesin -
Thierry Reding -
Tomeu Vizoso