* Re: [PATCH 3/3] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800
From: Krzysztof Kozlowski @ 2019-07-10 9:02 UTC (permalink / raw)
To: k.konieczny
Cc: Marek Szyprowski, Bartlomiej Zolnierkiewicz, Chanwoo Choi,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc@vger.kernel.org
In-Reply-To: <20190708141140.24379-4-k.konieczny@partner.samsung.com>
On Mon, 8 Jul 2019 at 16:12, <k.konieczny@partner.samsung.com> wrote:
>
> From: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
> be in 300mV range.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
> arch/arm/boot/dts/exynos5420.dtsi | 34 +++++++++----------
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +++
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +++
> arch/arm/boot/dts/exynos5800.dtsi | 32 ++++++++---------
> 4 files changed, 41 insertions(+), 33 deletions(-)
Looks good, I assume bisectability is not affected, because of
dependency on the driver changes I will take it for next next release
(v5.5). Assuming that driver change goes into v5.4.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 0/3] add coupled regulators for Exynos5422/5800
From: Krzysztof Kozlowski @ 2019-07-10 9:00 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc@vger.kernel.org
In-Reply-To: <20190708141140.24379-1-k.konieczny@partner.samsung.com>
On Mon, 8 Jul 2019 at 16:12, <k.konieczny@partner.samsung.com> wrote:
>
> From: Kamil Konieczny <k.konieczny@partner.samsung.com>
>
> Hi,
>
> The main purpose of this patch series is to add coupled regulators for
> Exynos5422/5800 to keep constrain on voltage difference between vdd_arm
> and vdd_int to be at most 300mV. In exynos-bus instead of using
> regulator_set_voltage_tol() with default voltage tolerance it should be
> used regulator_set_voltage_triplet() with volatege range, and this is
> already present in opp/core.c code, so it can be reused. While at this,
> move setting regulators into opp/core.
>
> This patchset was tested on Odroid XU3.
>
> The last patch depends on two previous.
So you break the ABI... I assume that patchset maintains
bisectability. However there is no explanation why ABI break is needed
so this does not look good...
Best regards,
Krzysztof
>
> Regards,
> Kamil
>
> Kamil Konieczny (2):
> opp: core: add regulators enable and disable
> devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
>
> Marek Szyprowski (1):
> ARM: dts: exynos: add initial data for coupled regulators for
> Exynos5422/5800
>
> arch/arm/boot/dts/exynos5420.dtsi | 34 ++--
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +
> arch/arm/boot/dts/exynos5800.dtsi | 32 ++--
> drivers/devfreq/exynos-bus.c | 172 +++++++-----------
> drivers/opp/core.c | 13 ++
> 6 files changed, 120 insertions(+), 139 deletions(-)
>
> --
> 2.22.0
>
^ permalink raw reply
* Re: [PATCH] power: supply: bq25890_charger: Add the BQ25895 part
From: Krzysztof Kozlowski @ 2019-07-10 8:49 UTC (permalink / raw)
To: Angus Ainslie (Purism)
Cc: angus.ainslie, Sebastian Reichel, linux-pm, linux-kernel
In-Reply-To: <20190705113751.18116-1-angus@akkea.ca>
On Fri, 5 Jul 2019 at 13:38, Angus Ainslie (Purism) <angus@akkea.ca> wrote:
>
> The BQ25895 is almost identical to the BQ25890.
>
> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
> drivers/power/supply/bq25890_charger.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH RFC 2/9] OPP: Export a number of helpers to prevent code duplication
From: Sibi Sankar @ 2019-07-10 8:01 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Rob Herring, andy.gross, MyungJoo Ham, Kyungmin Park,
Rafael J. Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd,
georgi.djakov, bjorn.andersson, david.brown, Mark Rutland, lkml,
linux-arm-msm-owner, devicetree, rnayak, Chanwoo Choi, linux-pm,
evgreen, daidavid1, dianders
In-Reply-To: <CAJMQK-gcBC=ZyscuHzOe4t6xQzviTYo9W9_DSsppoaTZuiEOcw@mail.gmail.com>
Hi Hsin-Yi,
I'll get this addressed in the next re-spin which I plan to post by
end of this week.
On 7/8/19 8:58 AM, Hsin-Yi Wang wrote:
> On Thu, Mar 28, 2019 at 3:28 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
>> +
>> +/* The caller must call dev_pm_opp_put() after the OPP is used */
>> +struct dev_pm_opp *dev_pm_opp_find_opp_of_np(struct opp_table *opp_table,
>> + struct device_node *opp_np)
>> +{
>> + return _find_opp_of_np(opp_table, opp_np);
>> +}
> Hi Sibi,
>
> Though this is not the latest version, we've seen following issue:
>
> We would get lockdep warnings on this:
> [ 79.068957] Call trace:
> [ 79.071396] _find_opp_of_np+0xa0/0xa8
> [ 79.075136] dev_pm_opp_find_opp_of_np+0x24/0x30
> [ 79.079744] devfreq_passive_event_handler+0x304/0x51c
> [ 79.084872] devfreq_add_device+0x368/0x434
> [ 79.089046] devm_devfreq_add_device+0x68/0xb0
> [ 79.093480] mtk_cci_devfreq_probe+0x108/0x158
> [ 79.097915] platform_drv_probe+0x80/0xb0
> [ 79.101915] really_probe+0x1b4/0x28c
> [ 79.105568] driver_probe_device+0x64/0xfc
> [ 79.109655] __driver_attach+0x94/0xcc
> [ 79.113395] bus_for_each_dev+0x84/0xcc
> [ 79.117221] driver_attach+0x2c/0x38
> [ 79.120788] bus_add_driver+0x120/0x1f4
> [ 79.124614] driver_register+0x64/0xf8
> [ 79.128355] __platform_driver_register+0x4c/0x58
> [ 79.133049] mtk_cci_devfreq_init+0x1c/0x24
> [ 79.137224] do_one_initcall+0x1c0/0x3e0
> [ 79.141138] do_initcall_level+0x1f4/0x224
> [ 79.145225] do_basic_setup+0x34/0x4c
> [ 79.148878] kernel_init_freeable+0x10c/0x194
> [ 79.153225] kernel_init+0x14/0x100
> [ 79.156705] ret_from_fork+0x10/0x18
> [ 79.160270] irq event stamp: 238006
> [ 79.163750] hardirqs last enabled at (238005):
> [<ffffffa71fdea0a4>] _raw_spin_unlock_irqrestore+0x40/0x84
> [ 79.173391] hardirqs last disabled at (238006):
> [<ffffffa71f480e78>] do_debug_exception+0x70/0x198
> [ 79.182337] softirqs last enabled at (237998):
> [<ffffffa71f48165c>] __do_softirq+0x45c/0x4a4
> [ 79.190850] softirqs last disabled at (237987):
> [<ffffffa71f4bc0d4>] irq_exit+0xd8/0xf8
> [ 79.198842] ---[ end trace 0e66a55077a0abab ]---
>
> In _find_opp_of_np()[1], there's
> lockdep_assert_held(&opp_table_lock);
>
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/opp/of.c#L75
>
> But in governor passive.c#cpufreq_passive_register(), it call
> dev_pm_opp_find_opp_of_np() directly, so it wouldn't access
> opp_table_lock lock.
>
> Another similar place is in dev_pm_opp_of_add_table(), most devfreq
> would call this to get opp table.
> dev_pm_opp_of_add_table
> --> _opp_add_static_v2
> --> _of_opp_alloc_required_opps // would goes here if opp
> table contains "required-opps" property.
> --> _find_opp_of_np
> cpufreq-map governor needs devfreq to have "required-opps" property.
> So it would also trigger above lockdep warning.
>
>
> The question is: Is lockdep_assert_held(&opp_table_lock); needed in
> above use cases? Since they don't need to modify device and opp lists.
>
> Thanks
>
>
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 04/13] cpufreq: qcom: Refactor the driver to make it easier to extend
From: Viresh Kumar @ 2019-07-10 6:30 UTC (permalink / raw)
To: Niklas Cassel
Cc: Andy Gross, Ilia Lin, Rafael J. Wysocki, linux-arm-msm,
jorge.ramirez-ortiz, sboyd, vireshk, bjorn.andersson, ulf.hansson,
linux-pm, linux-kernel
In-Reply-To: <20190705095726.21433-5-niklas.cassel@linaro.org>
On 05-07-19, 11:57, Niklas Cassel wrote:
> + drv->opp_tables = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tables),
> + GFP_KERNEL);
> + if (!drv->opp_tables) {
> + ret = -ENOMEM;
> + goto free_drv;
> + }
>
> for_each_possible_cpu(cpu) {
> cpu_dev = get_cpu_device(cpu);
> @@ -166,19 +195,23 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> goto free_opp;
> }
>
> - opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> - &versions, 1);
> - if (IS_ERR(opp_tables[cpu])) {
> - ret = PTR_ERR(opp_tables[cpu]);
> - dev_err(cpu_dev, "Failed to set supported hardware\n");
> - goto free_opp;
> + if (drv->data->get_version) {
Why depend on get_version here ? The OPP table is already allocated
unconditionally.
> + drv->opp_tables[cpu] =
> + dev_pm_opp_set_supported_hw(cpu_dev,
> + &drv->versions, 1);
> + if (IS_ERR(drv->opp_tables[cpu])) {
> + ret = PTR_ERR(drv->opp_tables[cpu]);
> + dev_err(cpu_dev,
> + "Failed to set supported hardware\n");
> + goto free_opp;
> + }
> }
> }
--
viresh
^ permalink raw reply
* Re: [PATCH 02/13] cpufreq: qcom: Re-organise kryo cpufreq to use it for other nvmem based qcom socs
From: Viresh Kumar @ 2019-07-10 6:18 UTC (permalink / raw)
To: Niklas Cassel
Cc: Rafael J. Wysocki, Andy Gross, Ilia Lin, linux-arm-msm,
jorge.ramirez-ortiz, sboyd, vireshk, bjorn.andersson, ulf.hansson,
Sricharan R, linux-kernel, linux-pm
In-Reply-To: <20190705095726.21433-3-niklas.cassel@linaro.org>
On 05-07-19, 11:57, Niklas Cassel wrote:
> -static struct platform_driver qcom_cpufreq_kryo_driver = {
> - .probe = qcom_cpufreq_kryo_probe,
> - .remove = qcom_cpufreq_kryo_remove,
> +static struct platform_driver qcom_cpufreq_driver = {
> + .probe = qcom_cpufreq_probe,
> + .remove = qcom_cpufreq_remove,
> .driver = {
> - .name = "qcom-cpufreq-kryo",
> + .name = "qcom-cpufreq",
Should we still name it "qcom-cpufreq-nvmem" here ? Only the string
here.
> },
> };
--
viresh
^ permalink raw reply
* Re: [PATCH v4 2/2] dt-bindings: mfd: max8998: Add charger subnode binding
From: Rob Herring @ 2019-07-09 21:58 UTC (permalink / raw)
To: Paweł Chmiel
Cc: sre, lee.jones, mark.rutland, linux-kernel, linux-pm, devicetree,
linux-samsung-soc
In-Reply-To: <20190621115602.17559-3-pawel.mikolaj.chmiel@gmail.com>
On Fri, Jun 21, 2019 at 01:56:02PM +0200, Paweł Chmiel wrote:
> This patch adds devicetree bindings documentation for
> battery charging controller as the subnode of MAX8998 PMIC.
>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
> Changes from v3:
> - Property prefix should be maxim, not max8998
> - Describe what End of Charge in percent means
>
> Changes from v2:
> - Make charge-restart-level-microvolt optional.
> - Make charge-timeout-hours optional.
>
> Changes from v1:
> - Removed unneeded Fixes tag
> - Correct description of all charger values
> - Added missing property unit
> ---
> .../devicetree/bindings/mfd/max8998.txt | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> index 5f2f07c09c90..368f787d6079 100644
> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> @@ -48,6 +48,25 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> for buck2 regulator that can be selected using dvs gpio.
>
> +Charger: Configuration for battery charging controller should be added
> +inside a child node named 'charger'.
> + Required properties:
> + - maxim,end-of-charge-percentage: End of Charge in percent.
> + When the charge current in constant-voltage phase drops below
> + end-of-charge-percentage of it's start value, charging is terminated.
> + If value equals 0, leave it unchanged. Otherwise it should be value
> + from 10 to 45 by 5 step.
> +
> + Optional properties:
> + - maxim,charge-restart-threshold: Charge restart threshold in millivolts.
> + If property is not present, this will be disabled.
> + Valid values are: 0, 100, 150, 200. If the value equals 0, leave it
> + unchanged.
Needs a unit suffix as defined in property-units.txt.
> +
> + - maxim,charge-timeout: Charge timeout in hours. If property is not
> + present, this will be disabled. Valid values are: 0, 5, 6, 7.
> + If the value equals 0, leave it unchanged.
Needs a unit suffix as defined in property-units.txt.
> +
> Regulators: All the regulators of MAX8998 to be instantiated shall be
> listed in a child node named 'regulators'. Each regulator is represented
> by a child node of the 'regulators' node.
> @@ -97,6 +116,13 @@ Example:
> max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
> max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
>
> + /* Charger configuration */
> + charger {
> + maxim,end-of-charge-percentage = <20>;
> + maxim,charge-restart-threshold = <100>;
> + maxim,charge-timeout = <7>;
> + };
> +
> /* Regulators to instantiate */
> regulators {
> ldo2_reg: LDO2 {
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: soc: add mtk svs dt-bindings
From: Rob Herring @ 2019-07-09 21:47 UTC (permalink / raw)
To: Roger Lu
Cc: Kevin Hilman, Nicolas Boichat, Stephen Boyd, Fan Chen,
HenryC Chen, Mark Rutland, Matthias Brugger, Nishanth Menon,
devicetree, linux-arm-kernel, linux-mediatek, linux-kernel,
linux-pm
In-Reply-To: <20190621084348.16834-2-roger.lu@mediatek.com>
On Fri, Jun 21, 2019 at 04:43:47PM +0800, Roger Lu wrote:
> Document the binding for enabling mtk svs on MediaTek SoC.
>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> ---
> .../devicetree/bindings/power/mtk-svs.txt | 88 +++++++++++++++++++
> 1 file changed, 88 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/mtk-svs.txt
>
> diff --git a/Documentation/devicetree/bindings/power/mtk-svs.txt b/Documentation/devicetree/bindings/power/mtk-svs.txt
> new file mode 100644
> index 000000000000..6a71992ef162
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/mtk-svs.txt
> @@ -0,0 +1,88 @@
> +* Mediatek Smart Voltage Scaling (MTK SVS)
> +
> +This describes the device tree binding for the MTK SVS controller (bank)
> +which helps provide the optimized CPU/GPU/CCI voltages. This device also
> +needs thermal data to calculate thermal slope for accurately compensate
> +the voltages when temperature change.
> +
> +Required properties:
> +- compatible:
> + - "mediatek,mt8183-svs" : For MT8183 family of SoCs
> +- reg: Address range of the MTK SVS controller.
> +- interrupts: IRQ for the MTK SVS controller.
> +- clocks, clock-names: Clocks needed for the svs controller. required
> + clocks are:
> + "main_clk": Main clock needed for register access
'_clk' is redundant and can be dropped.
> +- nvmem-cells: Phandle to the calibration data provided by a nvmem device.
> +- nvmem-cell-names: Should be "svs-calibration-data" and "calibration-data"
> +
> +Subnodes:
> +- svs_cpu_little: SVS bank device node of little CPU
> + compatible: "mediatek,mt8183-svs-cpu-little"
> + operating-points-v2: OPP table hooked by SVS little CPU bank.
> + SVS will optimze this OPP table voltage part.
> + vcpu-little-supply: PMIC buck of little CPU
> +- svs_cpu_big: SVS bank device node of big CPU
> + compatible: "mediatek,mt8183-svs-cpu-big"
> + operating-points-v2: OPP table hooked by SVS big CPU bank.
> + SVS will optimze this OPP table voltage part.
> + vcpu-big-supply: PMIC buck of big CPU
> +- svs_cci: SVS bank device node of CCI
> + compatible: "mediatek,mt8183-svs-cci"
> + operating-points-v2: OPP table hooked by SVS CCI bank.
> + SVS will optimze this OPP table voltage part.
> + vcci-supply: PMIC buck of CCI
> +- svs_gpu: SVS bank device node of GPU
> + compatible: "mediatek,mt8183-svs-gpu"
> + operating-points-v2: OPP table hooked by SVS GPU bank.
> + SVS will optimze this OPP table voltage part.
> + vgpu-spply: PMIC buck of GPU
typo
> +
> +Example:
> +
> + svs: svs@1100b000 {
> + compatible = "mediatek,mt8183-svs";
> + reg = <0 0x1100b000 0 0x1000>;
> + interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW 0>;
> + clocks = <&infracfg CLK_INFRA_THERM>;
> + clock-names = "main_clk";
> + nvmem-cells = <&svs_calibration>, <&thermal_calibration>;
> + nvmem-cell-names = "svs-calibration-data", "calibration-data";
> +
> + svs_cpu_little: svs_cpu_little {
> + compatible = "mediatek,mt8183-svs-cpu-little";
> + operating-points-v2 = <&cluster0_opp>;
> + };
> +
> + svs_cpu_big: svs_cpu_big {
> + compatible = "mediatek,mt8183-svs-cpu-big";
> + operating-points-v2 = <&cluster1_opp>;
> + };
> +
> + svs_cci: svs_cci {
> + compatible = "mediatek,mt8183-svs-cci";
> + operating-points-v2 = <&cci_opp>;
> + };
> +
> + svs_gpu: svs_gpu {
> + compatible = "mediatek,mt8183-svs-gpu";
> + power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
> + operating-points-v2 = <&gpu_opp_table>;
> + };
This all looks like redundant data which can be found in the cpu, gpu,
etc. nodes. Can't you parse those nodes to get the information?
> + };
> +
> + &svs_cpu_little {
> + vcpu-little-supply = <&mt6358_vproc12_reg>;
Don't split examples like this. Just should one flat example.
> + };
> +
> + &svs_cpu_big {
> + vcpu-big-supply = <&mt6358_vproc11_reg>;
> + };
> +
> + &svs_cci {
> + vcci-supply = <&mt6358_vproc12_reg>;
> + };
> +
> + &svs_gpu {
> + vgpu-spply = <&mt6358_vgpu_reg>;
> + };
> --
> 2.18.0
>
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Saravana Kannan @ 2019-07-09 19:02 UTC (permalink / raw)
To: Vincent Guittot
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Sweeney, Sean,
daidavid1, Rajendra Nayak, sibis, Bjorn Andersson, Evan Green,
Android Kernel Team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <CAKfTPtBngOT__TfmHXmmim-b9YhExOOvwVRaxYM9g9M6ffr_zQ@mail.gmail.com>
On Tue, Jul 9, 2019 at 12:25 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Sun, 7 Jul 2019 at 23:48, Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Jul 4, 2019 at 12:12 AM Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Wed, 3 Jul 2019 at 23:33, Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Tue, Jul 2, 2019 at 11:45 PM Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > On Wed, 3 Jul 2019 at 03:10, Saravana Kannan <saravanak@google.com> wrote:
> > > > > >
> > > > > > Interconnect paths can have different performance points. Now that OPP
> > > > > > framework supports bandwidth OPP tables, add OPP table support for
> > > > > > interconnects.
> > > > > >
> > > > > > Devices can use the interconnect-opp-table DT property to specify OPP
> > > > > > tables for interconnect paths. And the driver can obtain the OPP table for
> > > > > > an interconnect path by calling icc_get_opp_table().
> > > > >
> > > > > The opp table of a path must come from the aggregation of OPP tables
> > > > > of the interconnect providers.
> > > >
> > > > The aggregation of OPP tables of the providers is certainly the
> > > > superset of what a path can achieve, but to say that OPPs for
> > > > interconnect path should match that superset is an oversimplification
> > > > of the reality in hardware.
> > > >
> > > > There are lots of reasons an interconnect path might not want to use
> > > > all the available bandwidth options across all the interconnects in
> > > > the route.
> > > >
> > > > 1. That particular path might not have been validated or verified
> > > > during the HW design process for some of the frequencies/bandwidth
> > > > combinations of the providers.
> > >
> > > All these constraint are provider's constraints and not consumer's one
> > >
> > > The consumer asks for a bandwidth according to its needs and then the
> > > providers select the optimal bandwidth of each interconnect after
> > > aggregating all the request and according to what OPP have been
> > > validated
> >
> > Not really. The screening can be a consumer specific issue. The
> > consumer IP itself might have some issue with using too low of a
> > bandwidth or bandwidth that's not within some range. It should not be
>
> How can an IP ask for not enough bandwidth ?
> It asks the needed bandwidth based on its requirements
The "enough bandwidth" is not always obvious. It's only for very
simple cases that you can calculate the required bandwidth. Even for
cases that you think might be "obvious/easy" aren't always easy.
For example, you'd think a display IP would have a fixed bandwidth
requirement for a fixed resolution screen. But that's far from the
truth. It can also change as the number of layers change per frame.
For video decoder/encoder, it depends on how well the frames compress
with a specific compression scheme.
So the "required" bandwidth is often a heuristic based on the IP
frequency or traffic measurement.
But that's not even the point I was making in this specific "bullet".
A hardware IP might be screen/verified with only certain bandwidth
levels. Or it might have hardware bugs that prevent it from using
lower bandwidths even though it's technically sufficient. We need a
way to capture that per path. This is not even a fictional case. This
has been true multiple times over widely used IPs.
> > the provider's job to take into account all the IP that might be
> > connected to the interconnects. If the interconnect HW itself didn't
>
> That's not what I'm saying. The provider knows which bandwidth the
> interconnect can provide as it is the ones which configures it. So if
> the interconnect has a finite number of bandwidth point based probably
> on the possible clock frequency and others config of the interconnect,
> it selects the best final config after aggregating the request of the
> consumer.
I completely agree with this. What you are stating above is how it
should work and that's the whole point of the interconnect framework.
But this is orthogonal to the point I'm making.
> > change, the provider driver shouldn't need to change. By your
> > definition, a provider driver will have to account for all the
> > possible bus masters that might be connected to it across all SoCs.
>
> you didn't catch my point
Same. I think we are talking over each other. Let me try again.
You are trying to describe how and interconnect provider and framework
should work. There's no disagreement there.
My point is that consumers might not want to or can not always use all
the available bandwidth levels offered by the providers. There can be
many reasons for that (which is what I listed in my earlier emails)
and we need a good and generic way to capture that so that everyone
isn't trying to invent their own property.
> > That's not good design nor is it scalable.
> >
> > > >
> > > > 2. Similarly during parts screening in the factory, some of the
> > > > combinations might not have been screened and can't be guaranteed
> > > > to work.
> > >
> > > As above, it's the provider's job to select the final bandwidth
> > > according to its constraint
> >
> > Same reply as above.
> >
> > > >
> > > > 3. Only a certain set of bandwidth levels might make sense to use from
> > > > a power/performance balance given the device using it. For example:
> > > > - The big CPU might not want to use some of the lower bandwidths
> > > > but the little CPU might want to.
> > > > - The big CPU might not want to use some intermediate bandwidth
> > > > points if they don't save a lot of power compared to a higher
> > > > bandwidth levels, but the little CPU might want to.
> > > > - The little CPU might never want to use the higher set of
> > > > bandwidth levels since they won't be power efficient for the use
> > > > cases that might run on it.
> > >
> > > These example are quite vague about the reasons why little might never
> > > want to use higher bandwidth.
> >
> > How is it vague? I just said because of power/performance balance.
> >
> > > But then, if little doesn't ask high bandwidth it will not use them.
> >
> > If you are running a heuristics based algorithm to pick bandwidth,
> > this is how it'll know NOT to use some of the bandwidth levels.
>
> so you want to set a bandwidth according to the cpu frequency which is
> what has been proposed in other thread
Nope, that's just one heuristic. Often times it's based on hardware
monitors measuring interconnect activity. If you go look at the SDM845
in a Pixel 3, almost nothing is directly tied to the CPU frequency.
Even if you are scaling bandwidth based on other hardware
measurements, you might want to avoid some bandwidth level provided by
the interconnect providers because it's suboptimal.
For example, when making bandwidth votes to accommodate the big CPUs,
you might never want to use some of the lower bandwidth levels because
they are not power efficient for any CPU frequency or any bandwidth
level. Because at those levels the memory/interconnect is so slow that
it has a non-trivial utilization increase (because the CPU is
stalling) of the big CPUs.
Again, this is completely different from what the providers/icc
framework does. Which is, once the request is made, they aggregate and
set the actual interconnect frequencies correctly.
> >
> > > >
> > > > 4. It might not make sense from a system level power perspective.
> > > > Let's take an example of a path S (source) -> A -> B -> C -> D
> > > > (destination).
> > > > - A supports only 2, 5, 7 and 10 GB/s. B supports 1, 2 ... 10 GB/s.
> > > > C supports 5 and 10 GB/s
> > > > - If you combine and list the superset of bandwidth levels
> > > > supported in that path, that'd be 1, 2, 3, ... 10 GB/s.
> > > > - Which set of bandwidth levels make sense will depend on the
> > > > hardware characteristics of the interconnects.
> > > > - If B is the biggest power sink, then you might want to use all 10
> > > > levels.
> > > > - If A is the biggest power sink, then you might want to use all 2,
> > > > 5 and 10 GB/s of the levels.
> > > > - If C is the biggest power sink then you might only want to use 5
> > > > and 10 GB/s
> > > > - The more hops and paths you get the more convoluted this gets.
> > > >
> > > > 5. The design of the interconnects themselves might have an impact on
> > > > which bandwidth levels are used.
> > > > - For example, the FIFO depth between two specific interconnects
> > > > might affect the valid bandwidth levels for a specific path.
> > > > - Say S1 -> A -> B -> D1, S2 -> C -> B -> D1 and S2 -> C -> D2 are
> > > > three paths.
> > > > - If C <-> B FIFO depth is small, then there might be a requirement
> > > > that C and B be closely performance matched to avoid system level
> > > > congestion due to back pressure.
> > > > - So S2 -> D1 path can't use all the bandwidth levels supported by
> > > > C-B combination.
> > > > - But S2 -> D2 can use all the bandwidth levels supported by C.
> > > > - And S1 -> D1 can use all the levels supported by A-B combination.
> > > >
> > >
> > > All the examples above makes sense but have to be handle by the
> > > provider not the consumer. The consumer asks for a bandwidth according
> > > to its constraints. Then the provider which is the driver that manages
> > > the interconnect IP, should manage all this hardware and platform
> > > specific stuff related to the interconnect IP in order to set the
> > > optimal bandwidth that fit both consumer constraint and platform
> > > specific configuration.
> >
> > Sure, but the provider itself can have interconnect properties to
> > indicate which other interconnects it's tied to. And the provider will
> > still need the interconnect-opp-table to denote which bandwidth levels
> > are sensible to use with each of its connections.
You seem to have missed this comment.
Thanks,
Saravana
> > So in some instances the interconnect-opp-table covers the needs of
> > purely consumers and in some instances purely providers. But in either
> > case, it's still needed to describe the hardware properly.
> >
> > -Saravana
> >
> > > > These are just some of the reasons I could recollect in a few minutes.
> > > > These are all real world cases I had to deal with in the past several
> > > > years of dealing with scaling interconnects. I'm sure vendors and SoCs
> > > > I'm not familiar with have other good reasons I'm not aware of.
> > > >
> > > > Trying to figure this all out by aggregating OPP tables of
> > > > interconnect providers just isn't feasible nor is it efficient. The
> > > > OPP tables for an interconnect path is describing the valid BW levels
> > > > supported by that path and verified in hardware and makes a lot of
> > > > sense to capture it clearly in DT.
> > > >
> > > > > So such kind of OPP table should be at
> > > > > provider level but not at path level.
> > > >
> > > > They can also use it if they want to, but they'll probably want to use
> > > > a frequency OPP table.
> > > >
> > > >
> > > > -Saravana
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > ---
> > > > > > drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
> > > > > > include/linux/interconnect.h | 7 +++++++
> > > > > > 2 files changed, 33 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > > > > > index 871eb4bc4efc..881bac80bc1e 100644
> > > > > > --- a/drivers/interconnect/core.c
> > > > > > +++ b/drivers/interconnect/core.c
> > > > > > @@ -47,6 +47,7 @@ struct icc_req {
> > > > > > */
> > > > > > struct icc_path {
> > > > > > size_t num_nodes;
> > > > > > + struct opp_table *opp_table;
> > > > > > struct icc_req reqs[];
> > > > > > };
> > > > > >
> > > > > > @@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > {
> > > > > > struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> > > > > > struct icc_node *src_node, *dst_node;
> > > > > > - struct device_node *np = NULL;
> > > > > > + struct device_node *np = NULL, *opp_node;
> > > > > > struct of_phandle_args src_args, dst_args;
> > > > > > int idx = 0;
> > > > > > int ret;
> > > > > > @@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> > > > > > mutex_unlock(&icc_lock);
> > > > > >
> > > > > > + opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
> > > > > > + if (opp_node) {
> > > > > > + path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
> > > > > > + of_node_put(opp_node);
> > > > > > + }
> > > > > > +
> > > > > > +
> > > > > > return path;
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(of_icc_get);
> > > > > >
> > > > > > +/**
> > > > > > + * icc_get_opp_table() - Get the OPP table that corresponds to a path
> > > > > > + * @path: reference to the path returned by icc_get()
> > > > > > + *
> > > > > > + * This function will return the OPP table that corresponds to a path handle.
> > > > > > + * If the interconnect API is disabled, NULL is returned and the consumer
> > > > > > + * drivers will still build. Drivers are free to handle this specifically, but
> > > > > > + * they don't have to.
> > > > > > + *
> > > > > > + * Return: opp_table pointer on success. NULL is returned when the API is
> > > > > > + * disabled or the OPP table is missing.
> > > > > > + */
> > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > +{
> > > > > > + return path->opp_table;
> > > > > > +}
> > > > > > +
> > > > > > /**
> > > > > > * icc_set_bw() - set bandwidth constraints on an interconnect path
> > > > > > * @path: reference to the path returned by icc_get()
> > > > > > diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> > > > > > index dc25864755ba..0c0bc55f0e89 100644
> > > > > > --- a/include/linux/interconnect.h
> > > > > > +++ b/include/linux/interconnect.h
> > > > > > @@ -9,6 +9,7 @@
> > > > > >
> > > > > > #include <linux/mutex.h>
> > > > > > #include <linux/types.h>
> > > > > > +#include <linux/pm_opp.h>
> > > > > >
> > > > > > /* macros for converting to icc units */
> > > > > > #define Bps_to_icc(x) ((x) / 1000)
> > > > > > @@ -28,6 +29,7 @@ struct device;
> > > > > > struct icc_path *icc_get(struct device *dev, const int src_id,
> > > > > > const int dst_id);
> > > > > > struct icc_path *of_icc_get(struct device *dev, const char *name);
> > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path);
> > > > > > void icc_put(struct icc_path *path);
> > > > > > int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> > > > > >
> > > > > > @@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
> > > > > > {
> > > > > > }
> > > > > >
> > > > > > +static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > +{
> > > > > > + return NULL;
> > > > > > +}
> > > > > > +
> > > > > > static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > > > > > {
> > > > > > return 0;
> > > > > > --
> > > > > > 2.22.0.410.gd8fdbe21b5-goog
> > > > > >
> > >
> > > --
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > >
^ permalink raw reply
* Re: [GIT PULL] ACPI updates for v5.3-rc1
From: pr-tracker-bot @ 2019-07-09 18:05 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linus Torvalds, ACPI Devel Maling List, Linux Kernel Mailing List,
Linux PM
In-Reply-To: <CAJZ5v0gSbQy-GFz2Bo4bGGj7WemDvS21TW6=VHVvivDZCvEKWg@mail.gmail.com>
The pull request you sent on Mon, 8 Jul 2019 23:37:58 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-5.3-rc1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/4b4704520d97b74e045154fc3b844b73ae4e7ebd
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [GIT PULL] Power management updates for v5.3-rc1
From: pr-tracker-bot @ 2019-07-09 18:05 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linus Torvalds, Linux PM, ACPI Devel Maling List, Linux PCI,
Linux Kernel Mailing List
In-Reply-To: <CAJZ5v0jfQX=QmX9NFRu7M98=WjeVhSW4X0nTW93-MeB3FR1uWw@mail.gmail.com>
The pull request you sent on Mon, 8 Jul 2019 23:36:26 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-5.3-rc1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/cf2d213e49fdf47e4c10dc629a3659e0026a54b8
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v2] tools/power/x86/intel-speed-select: Add .gitignore file
From: Andy Shevchenko @ 2019-07-09 16:39 UTC (permalink / raw)
To: Prarit Bhargava; +Cc: Linux PM, Srinivas Pandruvada, David Arcari
In-Reply-To: <20190708231725.11353-1-prarit@redhat.com>
On Tue, Jul 9, 2019 at 2:17 AM Prarit Bhargava <prarit@redhat.com> wrote:
>
> Add a .gitignore file for build include/ and final binary.
>
It has improper Cc list (No PDx86, no its maintainers, no LKML, which
is also requirement to send patches to PDx86).
Please, gather the tags you got and send v2 with properly formed Cc list.
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: David Arcari <darcari@redhat.com>
> ---
> tools/power/x86/intel-speed-select/.gitignore | 2 ++
> 1 file changed, 2 insertions(+)
> create mode 100644 tools/power/x86/intel-speed-select/.gitignore
>
> diff --git a/tools/power/x86/intel-speed-select/.gitignore b/tools/power/x86/intel-speed-select/.gitignore
> new file mode 100644
> index 000000000000..f61145925ce9
> --- /dev/null
> +++ b/tools/power/x86/intel-speed-select/.gitignore
> @@ -0,0 +1,2 @@
> +include/
> +intel-speed-select
> --
> 2.21.0
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] tools/power/x86/intel-speed-select: Add .gitignore file
From: Srinivas Pandruvada @ 2019-07-09 16:27 UTC (permalink / raw)
To: Rafael J. Wysocki, Prarit Bhargava
Cc: Linux PM, Andy Shevchenko, David Arcari, platform-driver-x86
In-Reply-To: <699e770258725ae8e7c9597e0e4a4cb17e08de5e.camel@linux.intel.com>
+platform-driver-x86@vger.kernel.org
On Tue, 2019-07-09 at 09:18 -0700, Srinivas Pandruvada wrote:
> On Tue, 2019-07-09 at 10:17 +0200, Rafael J. Wysocki wrote:
> > On Tue, Jul 9, 2019 at 1:17 AM Prarit Bhargava <prarit@redhat.com>
> > wrote:
> > >
> > > Add a .gitignore file for build include/ and final binary.
> > >
> > > Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Cc: David Arcari <darcari@redhat.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
> >
> > > ---
> > > tools/power/x86/intel-speed-select/.gitignore | 2 ++
> > > 1 file changed, 2 insertions(+)
> > > create mode 100644 tools/power/x86/intel-speed-select/.gitignore
> > >
> > > diff --git a/tools/power/x86/intel-speed-select/.gitignore
> > > b/tools/power/x86/intel-speed-select/.gitignore
> > > new file mode 100644
> > > index 000000000000..f61145925ce9
> > > --- /dev/null
> > > +++ b/tools/power/x86/intel-speed-select/.gitignore
> > > @@ -0,0 +1,2 @@
> > > +include/
> > > +intel-speed-select
> > > --
> > > 2.21.0
> > >
>
>
^ permalink raw reply
* Re: [PATCH v2] tools/power/x86/intel-speed-select: Add .gitignore file
From: Srinivas Pandruvada @ 2019-07-09 16:18 UTC (permalink / raw)
To: Rafael J. Wysocki, Prarit Bhargava
Cc: Linux PM, Andy Shevchenko, David Arcari
In-Reply-To: <CAJZ5v0j4fsqK=AwqtytDM-T-HwcsCzmyGamQ3ZOMNZngh=2oeQ@mail.gmail.com>
On Tue, 2019-07-09 at 10:17 +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 9, 2019 at 1:17 AM Prarit Bhargava <prarit@redhat.com>
> wrote:
> >
> > Add a .gitignore file for build include/ and final binary.
> >
> > Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: David Arcari <darcari@redhat.com>
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
> > ---
> > tools/power/x86/intel-speed-select/.gitignore | 2 ++
> > 1 file changed, 2 insertions(+)
> > create mode 100644 tools/power/x86/intel-speed-select/.gitignore
> >
> > diff --git a/tools/power/x86/intel-speed-select/.gitignore
> > b/tools/power/x86/intel-speed-select/.gitignore
> > new file mode 100644
> > index 000000000000..f61145925ce9
> > --- /dev/null
> > +++ b/tools/power/x86/intel-speed-select/.gitignore
> > @@ -0,0 +1,2 @@
> > +include/
> > +intel-speed-select
> > --
> > 2.21.0
> >
^ permalink raw reply
* Re: [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter
From: Lorenzo Pieralisi @ 2019-07-09 15:31 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sudeep Holla, Mark Rutland, linux-arm-kernel, Rafael J . Wysocki,
Daniel Lezcano, Raju P . L . S . S . S . N, Amit Kucheria,
Bjorn Andersson, Stephen Boyd, Niklas Cassel, Tony Lindgren,
Kevin Hilman, Lina Iyer, Viresh Kumar, Vincent Guittot,
Geert Uytterhoeven, Souvik Chakravarty, linux-pm, linux-arm-msm,
linux-kernel
In-Reply-To: <20190513192300.653-11-ulf.hansson@linaro.org>
On Mon, May 13, 2019 at 09:22:52PM +0200, Ulf Hansson wrote:
> If the hierarchical CPU topology is used, but the OS initiated mode isn't
> supported, we need to rely solely on the regular cpuidle framework to
> manage the idle state selection, rather than using genpd and its governor.
>
> For this reason, introduce a new PSCI DT helper function,
> psci_dt_pm_domains_parse_states(), which parses and converts the
> hierarchically described domain idle states from DT, into regular flattened
> cpuidle states. The converted states are added to the existing cpuidle
> driver's array of idle states, which make them available for cpuidle.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes:
> - Some simplification of the code.
>
> ---
> drivers/firmware/psci/psci.h | 5 ++
> drivers/firmware/psci/psci_pm_domain.c | 118 +++++++++++++++++++++++++
> 2 files changed, 123 insertions(+)
>
> diff --git a/drivers/firmware/psci/psci.h b/drivers/firmware/psci/psci.h
> index 00d2e3dcef49..c36e0e6649e9 100644
> --- a/drivers/firmware/psci/psci.h
> +++ b/drivers/firmware/psci/psci.h
> @@ -3,6 +3,7 @@
> #ifndef __PSCI_H
> #define __PSCI_H
>
> +struct cpuidle_driver;
> struct device_node;
>
> int psci_set_osi_mode(void);
> @@ -13,8 +14,12 @@ void psci_set_domain_state(u32 state);
> int psci_dt_parse_state_node(struct device_node *np, u32 *state);
> #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> int psci_dt_init_pm_domains(struct device_node *np);
> +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> + struct device_node *cpu_node, u32 *psci_states);
> #else
> static inline int psci_dt_init_pm_domains(struct device_node *np) { return 0; }
> +static inline int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> + struct device_node *cpu_node, u32 *psci_states) { return 0; }
> #endif
> #endif
>
> diff --git a/drivers/firmware/psci/psci_pm_domain.c b/drivers/firmware/psci/psci_pm_domain.c
> index 3c6ca846caf4..3aa645dba81b 100644
> --- a/drivers/firmware/psci/psci_pm_domain.c
> +++ b/drivers/firmware/psci/psci_pm_domain.c
> @@ -14,6 +14,10 @@
> #include <linux/pm_domain.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> +#include <linux/cpuidle.h>
> +#include <linux/cpu_pm.h>
> +
> +#include <asm/cpuidle.h>
>
> #include "psci.h"
>
> @@ -104,6 +108,53 @@ static void psci_pd_free_states(struct genpd_power_state *states,
> kfree(states);
> }
>
> +static int psci_pd_enter_pc(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int idx)
> +{
> + return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, idx);
> +}
> +
> +static void psci_pd_enter_s2idle_pc(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int idx)
> +{
> + psci_pd_enter_pc(dev, drv, idx);
> +}
> +
> +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> + u32 *psci_state, struct genpd_power_state *state)
> +{
> + u32 *state_data = state->data;
> + u64 target_residency_us = state->residency_ns;
> + u64 exit_latency_us = state->power_on_latency_ns +
> + state->power_off_latency_ns;
> +
> + *psci_state = *state_data;
> + do_div(target_residency_us, 1000);
> + idle_state->target_residency = target_residency_us;
> + do_div(exit_latency_us, 1000);
> + idle_state->exit_latency = exit_latency_us;
> + idle_state->enter = &psci_pd_enter_pc;
> + idle_state->enter_s2idle = &psci_pd_enter_s2idle_pc;
> + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
This is arbitrary and not necessarily true.
I think that this patch is useful to represent my reservations about the
current approach. As a matter of fact, idle state entry will always be a
CPUidle decision.
You only need PM domain information to understand when all CPUs
in a power domain are actually idle but that's all genPD can do
in this respect.
I think this patchset would be much simpler if both CPUidle and
genPD governor would work on *one* set of idle states, globally
indexed (and that would be true for PSCI suspend parameters too).
To work with a unified set of idle states between CPUidle and genPD
(tossing some ideas around):
- We can implement a genPD CPUidle governor that in its select method
takes into account genPD information (for instance by avoiding
selection of idle states that require multiple cpus to be in idle
to be effectively active)
- We can use genPD to enable/disable CPUidle states through runtime
PM information
There may be other ways. My point is that current code, with two (or
more if the hierarchy grows) sets of idle states across two subsystems
(CPUidle and genPD) is not very well defined and honestly very hard to
grasp and prone to errors.
> +
> + strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> + CPUIDLE_NAME_LEN - 1);
> + strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> + CPUIDLE_NAME_LEN - 1);
> +}
> +
> +static bool psci_pd_is_provider(struct device_node *np)
> +{
> + struct psci_pd_provider *pd_prov, *it;
> +
> + list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> + if (pd_prov->node == np)
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int psci_pd_init(struct device_node *np)
> {
> struct generic_pm_domain *pd;
> @@ -265,4 +316,71 @@ int psci_dt_init_pm_domains(struct device_node *np)
> pr_err("failed to create CPU PM domains ret=%d\n", ret);
> return ret;
> }
> +
> +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> + struct device_node *cpu_node, u32 *psci_states)
> +{
> + struct genpd_power_state *pd_states;
> + struct of_phandle_args args;
> + int ret, pd_state_count, i, state_idx, psci_idx;
> + u32 cpu_psci_state = psci_states[drv->state_count - 2];
This (-2) is very dodgy and I doubt it would work on hierarchies going
above "cluster" level.
As I say above, I think we should work towards a single array of
idle states to be selected by a CPUidle governor using genPD
runtime information to bias the results according to the number
of CPUs in a genPD that entered/exit idle.
To be more precise, all idles states should be "domain-idle-state"
compatible, even the CPU ones, the distinction between what CPUidle
and genPD manage is a bit stretched IMO in this patchset.
We will have a chance to talk about this but I thought I would
comment publically if anyone else is willing to chime in, this
is not a PSCI problem at all, it is a CPUidle/genPD coexistence
design problem which is much broader.
Lorenzo
> + struct device_node *np = of_node_get(cpu_node);
> +
> +
> + /* Walk the CPU topology to find compatible domain idle states. */
> + while (np) {
> + ret = of_parse_phandle_with_args(np, "power-domains",
> + "#power-domain-cells", 0, &args);
> + of_node_put(np);
> + if (ret)
> + return 0;
> +
> + np = args.np;
> +
> + /* Verify that the node represents a psci pd provider. */
> + if (!psci_pd_is_provider(np)) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + /* Parse for compatible domain idle states. */
> + ret = psci_pd_parse_states(np, &pd_states, &pd_state_count);
> + if (ret) {
> + of_node_put(np);
> + return ret;
> + }
> +
> + i = 0;
> + while (i < pd_state_count) {
> +
> + state_idx = drv->state_count;
> + if (state_idx >= CPUIDLE_STATE_MAX) {
> + pr_warn("exceeding max cpuidle states\n");
> + of_node_put(np);
> + return 0;
> + }
> +
> + /* WFI state is not part of psci_states. */
> + psci_idx = state_idx - 1 + i;
> + psci_pd_convert_states(&drv->states[state_idx + i],
> + &psci_states[psci_idx], &pd_states[i]);
> +
> + /*
> + * In the hierarchical CPU topology the master PM domain
> + * idle state's DT property, "arm,psci-suspend-param",
> + * don't contain the bits for the idle state of the CPU,
> + * let's add those here.
> + */
> + psci_states[psci_idx] |= cpu_psci_state;
> + pr_debug("psci-power-state %#x index %d\n",
> + psci_states[psci_idx], psci_idx);
> +
> + drv->state_count++;
> + i++;
> + }
> + psci_pd_free_states(pd_states, pd_state_count);
> + }
> +
> + return 0;
> +}
> #endif
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RESEND, PATCH v4 2/2] dt-bindings: cpufreq: Document allwinner,sun50i-h6-operating-points
From: Rob Herring @ 2019-07-09 14:31 UTC (permalink / raw)
To: Yangtao Li
Cc: vireshk, nm, sboyd, robh+dt, mark.rutland, maxime.ripard, wens,
rjw, davem, mchehab+samsung, gregkh, linus.walleij, nicolas.ferre,
paulmck, linux-pm, devicetree, linux-arm-kernel, linux-kernel,
Yangtao Li
In-Reply-To: <20190612162816.31713-3-tiny.windzz@gmail.com>
On Wed, 12 Jun 2019 12:28:16 -0400, Yangtao Li wrote:
> Allwinner Process Voltage Scaling Tables defines the voltage and
> frequency value based on the speedbin blown in the efuse combination.
> The sunxi-cpufreq-nvmem driver reads the efuse value from the SoC to
> provide the OPP framework with required information.
> This is used to determine the voltage and frequency value for each
> OPP of operating-points-v2 table when it is parsed by the OPP framework.
>
> The "allwinner,sun50i-h6-operating-points" DT extends the
> "operating-points-v2"
> with following parameters:
> - nvmem-cells (NVMEM area containig the speedbin information)
> - opp-microvolt-<name>: voltage in micro Volts.
> At runtime, the platform can pick a <name> and matching
> opp-microvolt-<name> property.
> HW: <name>:
> sun50i-h6 speed0 speed1 speed2
>
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> .../bindings/opp/sun50i-nvmem-cpufreq.txt | 167 ++++++++++++++++++
> 1 file changed, 167 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/opp/sun50i-nvmem-cpufreq.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH V2 00/13] intel_rapl: RAPL abstraction and MMIO RAPL support
From: Rafael J. Wysocki @ 2019-07-09 13:59 UTC (permalink / raw)
To: Zhang Rui
Cc: Rafael J. Wysocki, Pandruvada, Srinivas, linux-pm@vger.kernel.org,
rjw@rjwysocki.net
In-Reply-To: <1562679696.2440.9.camel@intel.com>
On Tue, Jul 9, 2019 at 3:41 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On 一, 2019-07-08 at 10:58 +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 8, 2019 at 8:53 AM Zhang Rui <rui.zhang@intel.com> wrote:
> > >
> > >
> > > On 六, 2019-07-06 at 10:19 +0200, Rafael J. Wysocki wrote:
> > > >
> > > > On Fri, Jul 5, 2019 at 4:59 PM Pandruvada, Srinivas
> > > > <srinivas.pandruvada@intel.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On Fri, 2019-07-05 at 10:57 +0200, Rafael J. Wysocki wrote:
> > > > > >
> > > > > >
> > > > > > On Thu, Jul 4, 2019 at 6:34 PM Zhang Rui <rui.zhang@intel.com
> > > > > > >
> > > > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Besideis MSR interface, RAPL can also be controlled via the
> > > > > > > MMIO
> > > > > > > interface,
> > > > > > > by accessing the MCHBar registers exposed by the processor
> > > > > > > thermal
> > > > > > > device.
> > > > > > >
> > > > > > > Currently, we only have RAPL MSR interface in Linux kernel,
> > > > > > > this
> > > > > > > brings
> > > > > > > problems on some platforms that BIOS performs a low power
> > > > > > > limits
> > > > > > > via the
> > > > > > > MMIO interface by default. This results in poor system
> > > > > > > performance,
> > > > > > > and there is no way for us to change the MMIO MSR setting
> > > > > > > in
> > > > > > > Linux.
> > > > > > >
> > > > > > > To fix this, RAPL MMIO interface support is introduced in
> > > > > > > this
> > > > > > > patch set.
> > > > > > >
> > > > > > > Patch 1/13 to patch 11/13 abstract the RAPL code, and move
> > > > > > > all
> > > > > > > the
> > > > > > > shared
> > > > > > > code into a separate file, intel_rapl_common.c, so that it
> > > > > > > can
> > > > > > > be
> > > > > > > used
> > > > > > > by both MSR and MMIO interfaces.
> > > > > > > Patch 12/13 introduced RAPL support via MMIO registers,
> > > > > > > exposed
> > > > > > > by
> > > > > > > the
> > > > > > > processor thermal devices.
> > > > > > > Patch 13/13 fixes a module autoloading issue found later.
> > > > > > >
> > > > > > > The patch series has been tested on Dell XPS 9360, a SKL
> > > > > > > platform.
> > > > > > >
> > > > > > > Note that this patch series are based on the -tip tree,
> > > > > > > which
> > > > > > > contains the
> > > > > > > latest RAPL changes for multi-die support.
> > > > > > >
> > > > > > > Changes in V2:
> > > > > > > - add kerneldoc for struct rapl_if_priv.
> > > > > > > - use intel_rapl_msr.c for RAPL MSR I/F driver, instead
> > > > > > > of
> > > > > > > intel_rapl.c.
> > > > > > > - changelog and coding style update.
> > > > > > What tree is the series against?
> > > > > >
> > > > > > It doesn't apply either on top of my powercap branch or on
> > > > > > top of
> > > > > > 5.2-rc7 for me.
> > > > > This needs linux tip tree. There are some package/die changes
> > > > > in
> > > > > tip
> > > > > tree, which this patch depends on.
> > > > OK, so the changes in -tip need to go in first.
> > > >
> > > exactly.
> > > BTW, this patch set also conflicts with the RAPL support patches
> > > for
> > > icelake platforms.
> > Do you mean commits
> >
> > cc3ae777098b..88679b2587a0
> >
> > in linux-next?
> >
> yes.
> > >
> > > Thus IMO, having a separate rapl branch, and apply
> > > the icl rapl patches on top of this patch set will be much easier.
> > OK, I'll do that if that's preferred.
>
> thanks. I think you can apply the ICL RAPL patches by replacing
> intel_rapl.c with intel_rapl_common.c in the patch files. Or I can
> resend the ICL RAPL patches rebased on the this patch series.
It would help if you did that, thanks!
^ permalink raw reply
* Re: [PATCH V2 00/13] intel_rapl: RAPL abstraction and MMIO RAPL support
From: Zhang Rui @ 2019-07-09 13:41 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Pandruvada, Srinivas, linux-pm@vger.kernel.org, rjw@rjwysocki.net
In-Reply-To: <CAJZ5v0gC8iBjXFtawiRDJ0oCpQDrjXm9OjP-b6ZLnoG=_bwd8g@mail.gmail.com>
On 一, 2019-07-08 at 10:58 +0200, Rafael J. Wysocki wrote:
> On Mon, Jul 8, 2019 at 8:53 AM Zhang Rui <rui.zhang@intel.com> wrote:
> >
> >
> > On 六, 2019-07-06 at 10:19 +0200, Rafael J. Wysocki wrote:
> > >
> > > On Fri, Jul 5, 2019 at 4:59 PM Pandruvada, Srinivas
> > > <srinivas.pandruvada@intel.com> wrote:
> > > >
> > > >
> > > >
> > > > On Fri, 2019-07-05 at 10:57 +0200, Rafael J. Wysocki wrote:
> > > > >
> > > > >
> > > > > On Thu, Jul 4, 2019 at 6:34 PM Zhang Rui <rui.zhang@intel.com
> > > > > >
> > > > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > Besideis MSR interface, RAPL can also be controlled via the
> > > > > > MMIO
> > > > > > interface,
> > > > > > by accessing the MCHBar registers exposed by the processor
> > > > > > thermal
> > > > > > device.
> > > > > >
> > > > > > Currently, we only have RAPL MSR interface in Linux kernel,
> > > > > > this
> > > > > > brings
> > > > > > problems on some platforms that BIOS performs a low power
> > > > > > limits
> > > > > > via the
> > > > > > MMIO interface by default. This results in poor system
> > > > > > performance,
> > > > > > and there is no way for us to change the MMIO MSR setting
> > > > > > in
> > > > > > Linux.
> > > > > >
> > > > > > To fix this, RAPL MMIO interface support is introduced in
> > > > > > this
> > > > > > patch set.
> > > > > >
> > > > > > Patch 1/13 to patch 11/13 abstract the RAPL code, and move
> > > > > > all
> > > > > > the
> > > > > > shared
> > > > > > code into a separate file, intel_rapl_common.c, so that it
> > > > > > can
> > > > > > be
> > > > > > used
> > > > > > by both MSR and MMIO interfaces.
> > > > > > Patch 12/13 introduced RAPL support via MMIO registers,
> > > > > > exposed
> > > > > > by
> > > > > > the
> > > > > > processor thermal devices.
> > > > > > Patch 13/13 fixes a module autoloading issue found later.
> > > > > >
> > > > > > The patch series has been tested on Dell XPS 9360, a SKL
> > > > > > platform.
> > > > > >
> > > > > > Note that this patch series are based on the -tip tree,
> > > > > > which
> > > > > > contains the
> > > > > > latest RAPL changes for multi-die support.
> > > > > >
> > > > > > Changes in V2:
> > > > > > - add kerneldoc for struct rapl_if_priv.
> > > > > > - use intel_rapl_msr.c for RAPL MSR I/F driver, instead
> > > > > > of
> > > > > > intel_rapl.c.
> > > > > > - changelog and coding style update.
> > > > > What tree is the series against?
> > > > >
> > > > > It doesn't apply either on top of my powercap branch or on
> > > > > top of
> > > > > 5.2-rc7 for me.
> > > > This needs linux tip tree. There are some package/die changes
> > > > in
> > > > tip
> > > > tree, which this patch depends on.
> > > OK, so the changes in -tip need to go in first.
> > >
> > exactly.
> > BTW, this patch set also conflicts with the RAPL support patches
> > for
> > icelake platforms.
> Do you mean commits
>
> cc3ae777098b..88679b2587a0
>
> in linux-next?
>
yes.
> >
> > Thus IMO, having a separate rapl branch, and apply
> > the icl rapl patches on top of this patch set will be much easier.
> OK, I'll do that if that's preferred.
thanks. I think you can apply the ICL RAPL patches by replacing
intel_rapl.c with intel_rapl_common.c in the patch files. Or I can
resend the ICL RAPL patches rebased on the this patch series.
thanks,
rui
^ permalink raw reply
* Re: [RFC PATCH v2 0/5] sched/cpufreq: Make schedutil energy aware
From: Patrick Bellasi @ 2019-07-09 10:37 UTC (permalink / raw)
To: Douglas Raillard
Cc: Peter Zijlstra, linux-kernel, linux-pm, mingo, rjw, viresh.kumar,
quentin.perret, dietmar.eggemann
In-Reply-To: <b35c2281-4d91-2164-65f9-9ef3a28c35d0@arm.com>
On 08-Jul 14:46, Douglas Raillard wrote:
> Hi Patrick,
>
> On 7/8/19 12:09 PM, Patrick Bellasi wrote:
> > On 03-Jul 17:36, Douglas Raillard wrote:
> > > On 7/2/19 4:51 PM, Peter Zijlstra wrote:
> > > > On Thu, Jun 27, 2019 at 06:15:58PM +0100, Douglas RAILLARD wrote:
[...]
> > You are also correct in pointing out that in the steady state
> > ramp_boost will not be triggered in that steady state.
> >
> > IMU, that's for two main reasons:
> > a) it's very likely that enqueued <= util_avg
> > b) even in case enqueued should turn out to be _slightly_ bigger then
> > util_avg, the corresponding (proportional) ramp_boost would be so
> > tiny to not have any noticeable effect on OPP selection.
> >
> > Am I correct on point b) above?
>
> Assuming you meant "util_avg slightly bigger than enqueued" (which is when boosting triggers),
> then yes since ramp_boost effect is proportional to "task_ue.enqueue - task_u". It makes it robust
> against that.
Right :)
> > Could you maybe come up with some experimental numbers related to that
> > case specifically?
>
> With:
> * an rt-app task ramping up from 5% to 75% util in one big step. The
> whole cycle is 0.6s long (0.3s at 5% followed by 0.3s at 75%). This
> cycle is repeated 20 times and the average of boosting is taken.
>
> * a hikey 960 (this impact the frequency at which the test runs at
> the beginning of 75% phase, which impacts the number of missed
> activations before the util ramped up).
>
> * assuming an OPP exists for each util value (i.e. 1024 OPPs, so the
> effect of boost on consumption is not impacted by OPP capacities
> granularity)
>
> Then the boosting feature would increase the average power
> consumption by 3.1%, out of which 0.12% can be considered "spurious
> boosting" due to the util taking some time to really converge to its
> steady state value.
>
> In practice, the impact of small boosts will be even lower since
> they will less likely trigger the selection of a high OPP due to OPP
> capacity granularity > 1 util unit.
That's ok for the energy side: you estimate a ~3% worst case more
energy on that specific target.
By boosting I expect the negative boost to improve.
Do you have also numbers/stats related to the negative slack?
Can you share a percentage figure for that improvement?
Best,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2] tools/power/x86/intel-speed-select: Add .gitignore file
From: Rafael J. Wysocki @ 2019-07-09 8:17 UTC (permalink / raw)
To: Prarit Bhargava
Cc: Linux PM, Srinivas Pandruvada, Andy Shevchenko, David Arcari
In-Reply-To: <20190708231725.11353-1-prarit@redhat.com>
On Tue, Jul 9, 2019 at 1:17 AM Prarit Bhargava <prarit@redhat.com> wrote:
>
> Add a .gitignore file for build include/ and final binary.
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: David Arcari <darcari@redhat.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> tools/power/x86/intel-speed-select/.gitignore | 2 ++
> 1 file changed, 2 insertions(+)
> create mode 100644 tools/power/x86/intel-speed-select/.gitignore
>
> diff --git a/tools/power/x86/intel-speed-select/.gitignore b/tools/power/x86/intel-speed-select/.gitignore
> new file mode 100644
> index 000000000000..f61145925ce9
> --- /dev/null
> +++ b/tools/power/x86/intel-speed-select/.gitignore
> @@ -0,0 +1,2 @@
> +include/
> +intel-speed-select
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH V2 1/4] dt-bindings: opp: Support multiple opp-suspend properties
From: Viresh Kumar @ 2019-07-09 8:14 UTC (permalink / raw)
To: Anson.Huang
Cc: vireshk, nm, sboyd, robh+dt, mark.rutland, shawnguo, s.hauer,
kernel, festevam, leonard.crestez, p.zabel, ping.bai,
daniel.baluta, l.stach, abel.vesa, angus, andrew.smirnov, ccaione,
agx, linux-pm, devicetree, linux-kernel, linux-arm-kernel,
Linux-imx
In-Reply-To: <20190709080015.43442-1-Anson.Huang@nxp.com>
On 09-07-19, 16:00, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
>
> Update opp-suspend property's description to support multiple
> opp-suspend properties defined in DT, the OPP with highest opp-hz
> and with opp-suspend property present will be used as suspend opp.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> New patch.
> ---
> Documentation/devicetree/bindings/opp/opp.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 76b6c79..6859227 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -140,8 +140,8 @@ Optional properties:
> frequency for a short duration of time limited by the device's power, current
> and thermal limits.
>
> -- opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in
> - the table should have this.
> +- opp-suspend: Marks the OPP to be used during device suspend. If multiple OPPs
> + in the table have this, the OPP with highest opp-hz will be used.
>
> - opp-supported-hw: This enables us to select only a subset of OPPs from the
> larger OPP table, based on what version of the hardware we are running on. We
LGTM. Once Rob Acks it, I will apply the first two patches to the OPP
tree.
--
viresh
^ permalink raw reply
* Re: [PATCH v5] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()
From: Viresh Kumar @ 2019-07-09 8:12 UTC (permalink / raw)
To: Wen Yang
Cc: linux-kernel, xue.zhihong, wang.yi59, cheng.shengyu,
Rafael J. Wysocki, Michael Ellerman, linuxppc-dev, linux-pm
In-Reply-To: <1562659447-39989-1-git-send-email-wen.yang99@zte.com.cn>
On 09-07-19, 16:04, Wen Yang wrote:
> The cpu variable is still being used in the of_get_property() call
> after the of_node_put() call, which may result in use-after-free.
>
> Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak")
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> v5: put together the code to get, use, and release cpu device_node.
> v4: restore the blank line.
> v3: fix a leaked reference.
> v2: clean up the code according to the advice of viresh.
>
> drivers/cpufreq/pasemi-cpufreq.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
> index 6b1e4ab..1f0beb7 100644
> --- a/drivers/cpufreq/pasemi-cpufreq.c
> +++ b/drivers/cpufreq/pasemi-cpufreq.c
> @@ -131,10 +131,17 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
> int err = -ENODEV;
>
> cpu = of_get_cpu_node(policy->cpu, NULL);
> -
> - of_node_put(cpu);
> if (!cpu)
> goto out;
I would have loved a blank line here :)
> + max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> + of_node_put(cpu);
> + if (!max_freqp) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + /* we need the freq in kHz */
> + max_freq = *max_freqp / 1000;
>
> dn = of_find_compatible_node(NULL, NULL, "1682m-sdc");
> if (!dn)
> @@ -171,16 +178,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
> }
>
> pr_debug("init cpufreq on CPU %d\n", policy->cpu);
> -
> - max_freqp = of_get_property(cpu, "clock-frequency", NULL);
> - if (!max_freqp) {
> - err = -EINVAL;
> - goto out_unmap_sdcpwr;
> - }
> -
> - /* we need the freq in kHz */
> - max_freq = *max_freqp / 1000;
> -
> pr_debug("max clock-frequency is at %u kHz\n", max_freq);
> pr_debug("initializing frequency table\n");
Though, enough versions have happened now.
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* [PATCH V2 2/4] opp: of: Support multiple suspend OPPs defined in DT
From: Anson.Huang @ 2019-07-09 8:00 UTC (permalink / raw)
To: vireshk, nm, sboyd, robh+dt, mark.rutland, shawnguo, s.hauer,
kernel, festevam, leonard.crestez, p.zabel, ping.bai,
daniel.baluta, l.stach, abel.vesa, angus, andrew.smirnov, ccaione,
agx, linux-pm, devicetree, linux-kernel, linux-arm-kernel
Cc: Linux-imx
In-Reply-To: <20190709080015.43442-1-Anson.Huang@nxp.com>
From: Anson Huang <Anson.Huang@nxp.com>
With property "opp-supported-hw" introduced, the OPP table
in DT could be a large OPP table and ONLY a subset of OPPs
are available, based on the version of the hardware running
on. That introduces restriction of using "opp-suspend"
property to define the suspend OPP, as we are NOT sure if the
OPP containing "opp-suspend" property is available for the
hardware running on, and the of opp core does NOT allow multiple
suspend OPPs defined in DT OPP table.
To eliminate this restrition, make of opp core allow multiple
suspend OPPs defined in DT, and pick the OPP with highest rate
and with "opp-suspend" property present to be suspend OPP, it
can speed up the suspend/resume process.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
drivers/opp/of.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index b313aca..7e8ec6c 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -617,9 +617,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
/* OPP to select on device suspend */
if (of_property_read_bool(np, "opp-suspend")) {
if (opp_table->suspend_opp) {
- dev_warn(dev, "%s: Multiple suspend OPPs found (%lu %lu)\n",
- __func__, opp_table->suspend_opp->rate,
- new_opp->rate);
+ /* Pick the OPP with higher rate as suspend OPP */
+ if (new_opp->rate > opp_table->suspend_opp->rate) {
+ opp_table->suspend_opp->suspend = false;
+ new_opp->suspend = true;
+ opp_table->suspend_opp = new_opp;
+ }
} else {
new_opp->suspend = true;
opp_table->suspend_opp = new_opp;
--
2.7.4
^ permalink raw reply related
* [PATCH V2 3/4] arm64: dts: imx8mq: Add opp-suspend property to OPP table
From: Anson.Huang @ 2019-07-09 8:00 UTC (permalink / raw)
To: vireshk, nm, sboyd, robh+dt, mark.rutland, shawnguo, s.hauer,
kernel, festevam, leonard.crestez, p.zabel, ping.bai,
daniel.baluta, l.stach, abel.vesa, angus, andrew.smirnov, ccaione,
agx, linux-pm, devicetree, linux-kernel, linux-arm-kernel
Cc: Linux-imx
In-Reply-To: <20190709080015.43442-1-Anson.Huang@nxp.com>
From: Anson Huang <Anson.Huang@nxp.com>
Add opp-suspend property to each OPP, the of opp core will
select the OPP HW supported and with highest rate to be
suspend opp, it will speed up the suspend/resume process.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 58f66cb..4ba6a25f 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -156,6 +156,7 @@
/* Industrial only */
opp-supported-hw = <0xf>, <0x4>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
opp-1000000000 {
@@ -164,6 +165,7 @@
/* Consumer only */
opp-supported-hw = <0xe>, <0x3>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
opp-1300000000 {
@@ -171,6 +173,7 @@
opp-microvolt = <1000000>;
opp-supported-hw = <0xc>, <0x4>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
opp-1500000000 {
@@ -178,6 +181,7 @@
opp-microvolt = <1000000>;
opp-supported-hw = <0x8>, <0x3>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
};
--
2.7.4
^ permalink raw reply related
* [PATCH V2 4/4] arm64: dts: imx8mm: Add opp-suspend property to OPP table
From: Anson.Huang @ 2019-07-09 8:00 UTC (permalink / raw)
To: vireshk, nm, sboyd, robh+dt, mark.rutland, shawnguo, s.hauer,
kernel, festevam, leonard.crestez, p.zabel, ping.bai,
daniel.baluta, l.stach, abel.vesa, angus, andrew.smirnov, ccaione,
agx, linux-pm, devicetree, linux-kernel, linux-arm-kernel
Cc: Linux-imx
In-Reply-To: <20190709080015.43442-1-Anson.Huang@nxp.com>
From: Anson Huang <Anson.Huang@nxp.com>
Add opp-suspend property to each OPP, the of opp core will
select the OPP HW supported and with highest rate to be
suspend opp, it will speed up the suspend/resume process.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 398318b..973f457 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -108,6 +108,7 @@
opp-microvolt = <850000>;
opp-supported-hw = <0xe>, <0x7>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
opp-1600000000 {
@@ -115,6 +116,7 @@
opp-microvolt = <900000>;
opp-supported-hw = <0xc>, <0x7>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
opp-1800000000 {
@@ -122,6 +124,7 @@
opp-microvolt = <1000000>;
opp-supported-hw = <0x8>, <0x3>;
clock-latency-ns = <150000>;
+ opp-suspend;
};
};
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox