| Message ID | 1402925713-25426-2-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 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.
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 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 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 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: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 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 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.
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
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