* [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One @ 2016-06-25 3:44 megous at megous.com 2016-06-25 3:44 ` [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock megous at megous.com ` (13 more replies) 0 siblings, 14 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:44 UTC (permalink / raw) To: linux-arm-kernel This patch series implements thermal regulation on Orange Pi PC and Orange Pi One. The patch series contains: - Thermal sensor driver for Allwinner H3 SoC - Regulator driver for sy8106a I2C connected regulator - DTS setup for gpio regulator used on Orange Pi One - Operating points and thermal zone setup for both SBCs This is the second version of the patch series with incorporated changes from the review of the first series. Changes in v2: ths: - removed incorrect use of SID driver in sun8i_ths - read calibration data directly from iomem - better explanation for the thermal sensor driver - dt documentation fixes - dropped unncecessary macros and init code reorganization - moved resource aquisition from init to probe function - deassert reset after clock rate is set, not before - enable irq after all other registers are configured sy8106a: - added dt-bindings for the regulator - changed to use of_device_id for probing - added initialization failure checks dts: - add missing pinctrl-names for gpio-regulator - move clocks/clock-latency to sun8i-h3.dtsi regards, Ond?ej Jirman ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com @ 2016-06-25 3:44 ` megous at megous.com 2016-06-25 7:13 ` Maxime Ripard 2016-06-25 3:44 ` [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 megous at megous.com ` (12 subsequent siblings) 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:44 UTC (permalink / raw) To: linux-arm-kernel From: Josef Gajdusek <atx@atx.name> This patch adds a driver for the THS clock which is present on the Allwinner H3. Signed-off-by: Josef Gajdusek <atx@atx.name> --- Documentation/devicetree/bindings/clock/sunxi.txt | 1 + drivers/clk/sunxi/Makefile | 1 + drivers/clk/sunxi/clk-h3-ths.c | 98 +++++++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 drivers/clk/sunxi/clk-h3-ths.c diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt index 8f7619d..5faae05 100644 --- a/Documentation/devicetree/bindings/clock/sunxi.txt +++ b/Documentation/devicetree/bindings/clock/sunxi.txt @@ -87,6 +87,7 @@ Required properties: "allwinner,sun9i-a80-usb-phy-clk" - for usb phy gates + resets on A80 "allwinner,sun4i-a10-ve-clk" - for the Video Engine clock "allwinner,sun6i-a31-display-clk" - for the display clocks + "allwinner,sun8i-h3-ths-clk" - for THS on H3 Required properties for all clocks: - reg : shall be the control register address for the clock. diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile index 39d2044..8e245e3 100644 --- a/drivers/clk/sunxi/Makefile +++ b/drivers/clk/sunxi/Makefile @@ -9,6 +9,7 @@ obj-y += clk-a10-mod1.o obj-y += clk-a10-pll2.o obj-y += clk-a10-ve.o obj-y += clk-a20-gmac.o +obj-y += clk-h3-ths.o obj-y += clk-mod0.o obj-y += clk-simple-gates.o obj-y += clk-sun4i-display.o diff --git a/drivers/clk/sunxi/clk-h3-ths.c b/drivers/clk/sunxi/clk-h3-ths.c new file mode 100644 index 0000000..c1d6d32 --- /dev/null +++ b/drivers/clk/sunxi/clk-h3-ths.c @@ -0,0 +1,98 @@ +/* + * sun8i THS clock driver + * + * Copyright (C) 2015 Josef Gajdusek + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that 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. + * + */ + +#include <linux/clk-provider.h> +#include <linux/of_address.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +#define SUN8I_H3_THS_CLK_ENABLE 31 +#define SUN8I_H3_THS_CLK_DIVIDER_SHIFT 0 +#define SUN8I_H3_THS_CLK_DIVIDER_WIDTH 2 + +static DEFINE_SPINLOCK(sun8i_h3_ths_clk_lock); + +static const struct clk_div_table sun8i_h3_ths_clk_table[] __initconst = { + { .val = 0, .div = 1 }, + { .val = 1, .div = 2 }, + { .val = 2, .div = 4 }, + { .val = 3, .div = 6 }, + { } /* sentinel */ +}; + +static void __init sun8i_h3_ths_clk_setup(struct device_node *node) +{ + struct clk *clk; + struct clk_gate *gate; + struct clk_divider *div; + const char *parent; + const char *clk_name = node->name; + void __iomem *reg; + int err; + + reg = of_io_request_and_map(node, 0, of_node_full_name(node)); + + if (IS_ERR(reg)) + return; + + gate = kzalloc(sizeof(*gate), GFP_KERNEL); + if (!gate) + goto err_unmap; + + div = kzalloc(sizeof(*gate), GFP_KERNEL); + if (!div) + goto err_gate_free; + + of_property_read_string(node, "clock-output-names", &clk_name); + parent = of_clk_get_parent_name(node, 0); + + gate->reg = reg; + gate->bit_idx = SUN8I_H3_THS_CLK_ENABLE; + gate->lock = &sun8i_h3_ths_clk_lock; + + div->reg = reg; + div->shift = SUN8I_H3_THS_CLK_DIVIDER_SHIFT; + div->width = SUN8I_H3_THS_CLK_DIVIDER_WIDTH; + div->table = sun8i_h3_ths_clk_table; + div->lock = &sun8i_h3_ths_clk_lock; + + clk = clk_register_composite(NULL, clk_name, &parent, 1, + NULL, NULL, + &div->hw, &clk_divider_ops, + &gate->hw, &clk_gate_ops, + CLK_SET_RATE_PARENT); + + if (IS_ERR(clk)) + goto err_div_free; + + err = of_clk_add_provider(node, of_clk_src_simple_get, clk); + if (err) + goto err_unregister_clk; + + return; + +err_unregister_clk: + clk_unregister(clk); +err_gate_free: + kfree(gate); +err_div_free: + kfree(div); +err_unmap: + iounmap(reg); +} + +CLK_OF_DECLARE(sun8i_h3_ths_clk, "allwinner,sun8i-h3-ths-clk", + sun8i_h3_ths_clk_setup); -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock 2016-06-25 3:44 ` [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock megous at megous.com @ 2016-06-25 7:13 ` Maxime Ripard 2016-06-25 15:23 ` Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-06-25 7:13 UTC (permalink / raw) To: linux-arm-kernel Hi, On Sat, Jun 25, 2016 at 05:44:58AM +0200, megous at megous.com wrote: > From: Josef Gajdusek <atx@atx.name> > > This patch adds a driver for the THS clock which is present on the > Allwinner H3. > > Signed-off-by: Josef Gajdusek <atx@atx.name> You might not have noticed, but we are currently rewriting the whole clock support for the Allwinner SoCs, targetting the H3 as the first SoC to use it. There's already some support for the H3 THS clock, so please use that instead. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160625/2b01cef9/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock 2016-06-25 7:13 ` Maxime Ripard @ 2016-06-25 15:23 ` Ondřej Jirman 2016-06-28 11:52 ` Maxime Ripard 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-06-25 15:23 UTC (permalink / raw) To: linux-arm-kernel Hi Maxime, I try to base everything on the torvalds's kernel. I did notice the patches. Is there some main git tree/branch where this work is tracked in? I'd gladly use it. Also there's a PLL1 rate application patch, that would need to be ported to the new CCU code, in the case I would use it as a base for this work. Given that the new CCU code is your work and it's fresh in your mind, do you have suggestion how to approach it? It is [PATCH v2 06/14] in this series. regards, o. On 25.6.2016 09:13, Maxime Ripard wrote: > Hi, > > On Sat, Jun 25, 2016 at 05:44:58AM +0200, megous at megous.com wrote: >> From: Josef Gajdusek <atx@atx.name> >> >> This patch adds a driver for the THS clock which is present on the >> Allwinner H3. >> >> Signed-off-by: Josef Gajdusek <atx@atx.name> > > You might not have noticed, but we are currently rewriting the whole > clock support for the Allwinner SoCs, targetting the H3 as the first > SoC to use it. > > There's already some support for the H3 THS clock, so please use that > instead. > > Thanks! > Maxime > ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock 2016-06-25 15:23 ` Ondřej Jirman @ 2016-06-28 11:52 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-06-28 11:52 UTC (permalink / raw) To: linux-arm-kernel Hi, On Sat, Jun 25, 2016 at 05:23:12PM +0200, Ond?ej Jirman wrote: > Hi Maxime, > > I try to base everything on the torvalds's kernel. > > I did notice the patches. Is there some main git tree/branch where this > work is tracked in? I'd gladly use it. I just pushed it, branch sunxi/pen/clk-rework, on my github repo: https://github.com/mripard/linux > Also there's a PLL1 rate application patch, that would need to be ported > to the new CCU code, in the case I would use it as a base for this work. > Given that the new CCU code is your work and it's fresh in your mind, do > you have suggestion how to approach it? > > It is [PATCH v2 06/14] in this series. Yes, it's still in my queue of things to review, let's discuss this on this patch thread. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160628/46441f3b/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com 2016-06-25 3:44 ` [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock megous at megous.com @ 2016-06-25 3:44 ` megous at megous.com 2016-06-25 7:10 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver megous at megous.com ` (11 subsequent siblings) 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:44 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> This patch adds support for the sun8i thermal sensor on Allwinner H3 SoC. Signed-off-by: Ond?ej Jirman <megous@megous.com> --- v2: - removed incorrect use of SID driver in sun8i_ths - read calibration data directly from iomem - better explanation for the thermal sensor driver - dt documentation fixes - dropped unncecessary macros and init code reorganization - moved resource aquisition from init to probe function - deassert reset after clock rate is set, not before - enable irq after all other registers are configured --- drivers/thermal/Kconfig | 7 ++ drivers/thermal/Makefile | 1 + drivers/thermal/sun8i_ths.c | 260 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 268 insertions(+) create mode 100644 drivers/thermal/sun8i_ths.c diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 2d702ca..d3209d9 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -351,6 +351,13 @@ config MTK_THERMAL Enable this option if you want to have support for thermal management controller present in Mediatek SoCs +config SUN8I_THS + tristate "Thermal sensor driver for Allwinner H3" + depends on MACH_SUN8I + depends on OF + help + Enable this to support thermal reporting on some newer Allwinner SoCs. + menu "Texas Instruments thermal drivers" depends on ARCH_HAS_BANDGAP || COMPILE_TEST depends on HAS_IOMEM diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index 10b07c1..7261ee8 100644 --- a/drivers/thermal/Makefile +++ b/drivers/thermal/Makefile @@ -51,3 +51,4 @@ obj-$(CONFIG_TEGRA_SOCTHERM) += tegra/ obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o +obj-$(CONFIG_SUN8I_THS) += sun8i_ths.o diff --git a/drivers/thermal/sun8i_ths.c b/drivers/thermal/sun8i_ths.c new file mode 100644 index 0000000..9ba0f96 --- /dev/null +++ b/drivers/thermal/sun8i_ths.c @@ -0,0 +1,260 @@ +/* + * Thermal sensor driver for Allwinner H3 SoC + * + * Copyright (C) 2016 Ond?ej Jirman + * Based on the work of Josef Gajdusek <atx@atx.name> + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that 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. + * + */ + +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> +#include <linux/thermal.h> +#include <linux/printk.h> + +#define THS_H3_CTRL0 0x00 +#define THS_H3_CTRL2 0x40 +#define THS_H3_INT_CTRL 0x44 +#define THS_H3_STAT 0x48 +#define THS_H3_FILTER 0x70 +#define THS_H3_CDATA 0x74 +#define THS_H3_DATA 0x80 + +#define THS_H3_CTRL0_SENSOR_ACQ0(x) (x) +#define THS_H3_CTRL2_SENSE_EN BIT(0) +#define THS_H3_CTRL2_SENSOR_ACQ1(x) ((x) << 16) +#define THS_H3_INT_CTRL_DATA_IRQ_EN BIT(8) +#define THS_H3_INT_CTRL_THERMAL_PER(x) ((x) << 12) +#define THS_H3_STAT_DATA_IRQ_STS BIT(8) +#define THS_H3_FILTER_TYPE(x) ((x) << 0) +#define THS_H3_FILTER_EN BIT(2) + +#define THS_H3_CLK_IN 40000000 /* Hz */ +#define THS_H3_DATA_PERIOD 330 /* ms */ + +#define THS_H3_FILTER_TYPE_VALUE 2 /* average over 2^(n+1) samples */ +#define THS_H3_FILTER_DIV (1 << (THS_H3_FILTER_TYPE_VALUE + 1)) +#define THS_H3_INT_CTRL_THERMAL_PER_VALUE \ + (THS_H3_DATA_PERIOD * (THS_H3_CLK_IN / 1000) / THS_H3_FILTER_DIV / 4096 - 1) +#define THS_H3_CTRL0_SENSOR_ACQ0_VALUE 0x3f /* 16us */ +#define THS_H3_CTRL2_SENSOR_ACQ1_VALUE 0x3f + +struct sun8i_ths_data { + struct reset_control *reset; + struct clk *clk; + struct clk *busclk; + void __iomem *regs; + void __iomem *calreg; + struct thermal_zone_device *tzd; + u32 temp; +}; + +static int sun8i_ths_get_temp(void *_data, int *out) +{ + struct sun8i_ths_data *data = _data; + + if (data->temp == 0) + return -EINVAL; + + /* Formula and parameters from the Allwinner 3.4 kernel */ + *out = 217000 - (int)((data->temp * 1000000) / 8253); + return 0; +} + +static irqreturn_t sun8i_ths_irq_thread(int irq, void *_data) +{ + struct sun8i_ths_data *data = _data; + + writel(THS_H3_STAT_DATA_IRQ_STS, data->regs + THS_H3_STAT); + + data->temp = readl(data->regs + THS_H3_DATA); + if (data->temp) + thermal_zone_device_update(data->tzd); + + return IRQ_HANDLED; +} + +static void sun8i_ths_h3_init(struct sun8i_ths_data *data) +{ + u32 caldata; + + caldata = readl(data->calreg) & 0xfff; + if (caldata != 0) + writel(caldata, data->regs + THS_H3_CDATA); + + writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE), + data->regs + THS_H3_CTRL0); + writel(THS_H3_FILTER_EN | THS_H3_FILTER_TYPE(THS_H3_FILTER_TYPE_VALUE), + data->regs + THS_H3_FILTER); + writel(THS_H3_CTRL2_SENSOR_ACQ1(THS_H3_CTRL2_SENSOR_ACQ1_VALUE) | + THS_H3_CTRL2_SENSE_EN, + data->regs + THS_H3_CTRL2); + writel(THS_H3_INT_CTRL_THERMAL_PER(THS_H3_INT_CTRL_THERMAL_PER_VALUE) | + THS_H3_INT_CTRL_DATA_IRQ_EN, + data->regs + THS_H3_INT_CTRL); +} + +static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = { + .get_temp = sun8i_ths_get_temp, +}; + +static int sun8i_ths_probe(struct platform_device *pdev) +{ + struct sun8i_ths_data *data; + struct resource *res; + int ret; + int irq; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "no memory resources defined\n"); + return -EINVAL; + } + + data->regs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(data->regs)) { + ret = PTR_ERR(data->regs); + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); + return ret; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + if (!res) { + dev_err(&pdev->dev, "no calibration data memory resource defined\n"); + return -EINVAL; + } + + data->calreg = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(data->calreg)) { + ret = PTR_ERR(data->calreg); + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); + return ret; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq); + return irq; + } + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + sun8i_ths_irq_thread, IRQF_ONESHOT, + dev_name(&pdev->dev), data); + if (ret) + return ret; + + data->busclk = devm_clk_get(&pdev->dev, "ahb"); + if (IS_ERR(data->busclk)) { + ret = PTR_ERR(data->busclk); + dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret); + return ret; + } + + data->clk = devm_clk_get(&pdev->dev, "ths"); + if (IS_ERR(data->clk)) { + ret = PTR_ERR(data->clk); + dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret); + return ret; + } + + data->reset = devm_reset_control_get(&pdev->dev, "ahb"); + if (IS_ERR(data->reset)) { + ret = PTR_ERR(data->reset); + dev_err(&pdev->dev, "failed to get reset: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(data->busclk); + if (ret) { + dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(data->clk); + if (ret) { + dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret); + goto err_disable_bus; + } + + ret = clk_set_rate(data->clk, THS_H3_CLK_IN); + if (ret) + goto err_disable_ths; + + ret = reset_control_deassert(data->reset); + if (ret) { + dev_err(&pdev->dev, "reset deassert failed: %d\n", ret); + goto err_disable_ths; + } + + sun8i_ths_h3_init(data); + + data->tzd = thermal_zone_of_sensor_register(&pdev->dev, 0, data, + &sun8i_ths_thermal_ops); + if (IS_ERR(data->tzd)) { + ret = PTR_ERR(data->tzd); + dev_err(&pdev->dev, "failed to register thermal zone: %d\n", + ret); + goto err_assert_reset; + } + + platform_set_drvdata(pdev, data); + return 0; + +err_assert_reset: + reset_control_assert(data->reset); +err_disable_ths: + clk_disable_unprepare(data->clk); +err_disable_bus: + clk_disable_unprepare(data->busclk); + return ret; +} + +static int sun8i_ths_remove(struct platform_device *pdev) +{ + struct sun8i_ths_data *data = platform_get_drvdata(pdev); + + thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd); + reset_control_assert(data->reset); + clk_disable_unprepare(data->clk); + clk_disable_unprepare(data->busclk); + return 0; +} + +static const struct of_device_id sun8i_ths_id_table[] = { + { .compatible = "allwinner,sun8i-h3-ths", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, sun8i_ths_id_table); + +static struct platform_driver sun8i_ths_driver = { + .probe = sun8i_ths_probe, + .remove = sun8i_ths_remove, + .driver = { + .name = "sun8i_ths", + .of_match_table = sun8i_ths_id_table, + }, +}; + +module_platform_driver(sun8i_ths_driver); + +MODULE_AUTHOR("Ond?ej Jirman <megous@megous.com>"); +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner H3 SoC"); +MODULE_LICENSE("GPL v2"); -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 2016-06-25 3:44 ` [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 megous at megous.com @ 2016-06-25 7:10 ` Maxime Ripard 2016-06-25 15:12 ` Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-06-25 7:10 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:44:59AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > This patch adds support for the sun8i thermal sensor on > Allwinner H3 SoC. > > Signed-off-by: Ond?ej Jirman <megous@megous.com> > --- > v2: > - removed incorrect use of SID driver in sun8i_ths > - read calibration data directly from iomem > - better explanation for the thermal sensor driver > - dt documentation fixes > - dropped unncecessary macros and init code reorganization > - moved resource aquisition from init to probe function > - deassert reset after clock rate is set, not before > - enable irq after all other registers are configured > --- > drivers/thermal/Kconfig | 7 ++ > drivers/thermal/Makefile | 1 + > drivers/thermal/sun8i_ths.c | 260 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 268 insertions(+) > create mode 100644 drivers/thermal/sun8i_ths.c > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index 2d702ca..d3209d9 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -351,6 +351,13 @@ config MTK_THERMAL > Enable this option if you want to have support for thermal management > controller present in Mediatek SoCs > > +config SUN8I_THS > + tristate "Thermal sensor driver for Allwinner H3" > + depends on MACH_SUN8I > + depends on OF > + help > + Enable this to support thermal reporting on some newer Allwinner SoCs. > + > menu "Texas Instruments thermal drivers" > depends on ARCH_HAS_BANDGAP || COMPILE_TEST > depends on HAS_IOMEM > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile > index 10b07c1..7261ee8 100644 > --- a/drivers/thermal/Makefile > +++ b/drivers/thermal/Makefile > @@ -51,3 +51,4 @@ obj-$(CONFIG_TEGRA_SOCTHERM) += tegra/ > obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o > obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o > obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o > +obj-$(CONFIG_SUN8I_THS) += sun8i_ths.o > diff --git a/drivers/thermal/sun8i_ths.c b/drivers/thermal/sun8i_ths.c > new file mode 100644 > index 0000000..9ba0f96 > --- /dev/null > +++ b/drivers/thermal/sun8i_ths.c > @@ -0,0 +1,260 @@ > +/* > + * Thermal sensor driver for Allwinner H3 SoC > + * > + * Copyright (C) 2016 Ond?ej Jirman > + * Based on the work of Josef Gajdusek <atx@atx.name> > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that 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. > + * > + */ > + > +#include <linux/clk.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/slab.h> > +#include <linux/thermal.h> > +#include <linux/printk.h> > + > +#define THS_H3_CTRL0 0x00 > +#define THS_H3_CTRL2 0x40 > +#define THS_H3_INT_CTRL 0x44 > +#define THS_H3_STAT 0x48 > +#define THS_H3_FILTER 0x70 > +#define THS_H3_CDATA 0x74 > +#define THS_H3_DATA 0x80 > + > +#define THS_H3_CTRL0_SENSOR_ACQ0(x) (x) > +#define THS_H3_CTRL2_SENSE_EN BIT(0) > +#define THS_H3_CTRL2_SENSOR_ACQ1(x) ((x) << 16) > +#define THS_H3_INT_CTRL_DATA_IRQ_EN BIT(8) > +#define THS_H3_INT_CTRL_THERMAL_PER(x) ((x) << 12) > +#define THS_H3_STAT_DATA_IRQ_STS BIT(8) > +#define THS_H3_FILTER_TYPE(x) ((x) << 0) > +#define THS_H3_FILTER_EN BIT(2) > + > +#define THS_H3_CLK_IN 40000000 /* Hz */ > +#define THS_H3_DATA_PERIOD 330 /* ms */ > + > +#define THS_H3_FILTER_TYPE_VALUE 2 /* average over 2^(n+1) samples */ > +#define THS_H3_FILTER_DIV (1 << (THS_H3_FILTER_TYPE_VALUE + 1)) > +#define THS_H3_INT_CTRL_THERMAL_PER_VALUE \ > + (THS_H3_DATA_PERIOD * (THS_H3_CLK_IN / 1000) / THS_H3_FILTER_DIV / 4096 - 1) > +#define THS_H3_CTRL0_SENSOR_ACQ0_VALUE 0x3f /* 16us */ > +#define THS_H3_CTRL2_SENSOR_ACQ1_VALUE 0x3f > + > +struct sun8i_ths_data { > + struct reset_control *reset; > + struct clk *clk; > + struct clk *busclk; > + void __iomem *regs; > + void __iomem *calreg; > + struct thermal_zone_device *tzd; > + u32 temp; > +}; > + > +static int sun8i_ths_get_temp(void *_data, int *out) > +{ > + struct sun8i_ths_data *data = _data; > + > + if (data->temp == 0) > + return -EINVAL; > + > + /* Formula and parameters from the Allwinner 3.4 kernel */ > + *out = 217000 - (int)((data->temp * 1000000) / 8253); > + return 0; > +} > + > +static irqreturn_t sun8i_ths_irq_thread(int irq, void *_data) > +{ > + struct sun8i_ths_data *data = _data; > + > + writel(THS_H3_STAT_DATA_IRQ_STS, data->regs + THS_H3_STAT); > + > + data->temp = readl(data->regs + THS_H3_DATA); > + if (data->temp) > + thermal_zone_device_update(data->tzd); > + > + return IRQ_HANDLED; > +} > + > +static void sun8i_ths_h3_init(struct sun8i_ths_data *data) > +{ > + u32 caldata; > + > + caldata = readl(data->calreg) & 0xfff; > + if (caldata != 0) > + writel(caldata, data->regs + THS_H3_CDATA); > + > + writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE), > + data->regs + THS_H3_CTRL0); > + writel(THS_H3_FILTER_EN | THS_H3_FILTER_TYPE(THS_H3_FILTER_TYPE_VALUE), > + data->regs + THS_H3_FILTER); > + writel(THS_H3_CTRL2_SENSOR_ACQ1(THS_H3_CTRL2_SENSOR_ACQ1_VALUE) | > + THS_H3_CTRL2_SENSE_EN, > + data->regs + THS_H3_CTRL2); > + writel(THS_H3_INT_CTRL_THERMAL_PER(THS_H3_INT_CTRL_THERMAL_PER_VALUE) | > + THS_H3_INT_CTRL_DATA_IRQ_EN, > + data->regs + THS_H3_INT_CTRL); > +} > + > +static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = { > + .get_temp = sun8i_ths_get_temp, > +}; > + > +static int sun8i_ths_probe(struct platform_device *pdev) > +{ > + struct sun8i_ths_data *data; > + struct resource *res; > + int ret; > + int irq; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(&pdev->dev, "no memory resources defined\n"); > + return -EINVAL; > + } > + > + data->regs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->regs)) { > + ret = PTR_ERR(data->regs); > + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); > + return ret; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(&pdev->dev, "no calibration data memory resource defined\n"); > + return -EINVAL; > + } > + > + data->calreg = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->calreg)) { > + ret = PTR_ERR(data->calreg); > + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); > + return ret; > + } Why did you remove the SID use through the nvmem framework ?! > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq); > + return irq; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > + sun8i_ths_irq_thread, IRQF_ONESHOT, > + dev_name(&pdev->dev), data); > + if (ret) > + return ret; > + > + data->busclk = devm_clk_get(&pdev->dev, "ahb"); > + if (IS_ERR(data->busclk)) { > + ret = PTR_ERR(data->busclk); > + dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret); > + return ret; > + } > + > + data->clk = devm_clk_get(&pdev->dev, "ths"); > + if (IS_ERR(data->clk)) { > + ret = PTR_ERR(data->clk); > + dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret); > + return ret; > + } > + > + data->reset = devm_reset_control_get(&pdev->dev, "ahb"); > + if (IS_ERR(data->reset)) { > + ret = PTR_ERR(data->reset); > + dev_err(&pdev->dev, "failed to get reset: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(data->busclk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(data->clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret); > + goto err_disable_bus; > + } > + > + ret = clk_set_rate(data->clk, THS_H3_CLK_IN); > + if (ret) > + goto err_disable_ths; > + > + ret = reset_control_deassert(data->reset); > + if (ret) { > + dev_err(&pdev->dev, "reset deassert failed: %d\n", ret); > + goto err_disable_ths; > + } Having runtime_pm support would be great. > + sun8i_ths_h3_init(data); > + > + data->tzd = thermal_zone_of_sensor_register(&pdev->dev, 0, data, > + &sun8i_ths_thermal_ops); > + if (IS_ERR(data->tzd)) { > + ret = PTR_ERR(data->tzd); > + dev_err(&pdev->dev, "failed to register thermal zone: %d\n", > + ret); > + goto err_assert_reset; > + } You reference data->tzd in your interrupt handler, and the interrupts have been activated before initializing that field. That is likely to cause a kernel crash when you receive an interrupt between your request_irq and that call. > + > + platform_set_drvdata(pdev, data); > + return 0; > + > +err_assert_reset: > + reset_control_assert(data->reset); > +err_disable_ths: > + clk_disable_unprepare(data->clk); > +err_disable_bus: > + clk_disable_unprepare(data->busclk); > + return ret; > +} > + > +static int sun8i_ths_remove(struct platform_device *pdev) > +{ > + struct sun8i_ths_data *data = platform_get_drvdata(pdev); > + > + thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd); > + reset_control_assert(data->reset); > + clk_disable_unprepare(data->clk); > + clk_disable_unprepare(data->busclk); > + return 0; > +} > + > +static const struct of_device_id sun8i_ths_id_table[] = { > + { .compatible = "allwinner,sun8i-h3-ths", }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, sun8i_ths_id_table); > + > +static struct platform_driver sun8i_ths_driver = { > + .probe = sun8i_ths_probe, > + .remove = sun8i_ths_remove, > + .driver = { > + .name = "sun8i_ths", > + .of_match_table = sun8i_ths_id_table, > + }, > +}; > + > +module_platform_driver(sun8i_ths_driver); > + > +MODULE_AUTHOR("Ond?ej Jirman <megous@megous.com>"); > +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner H3 SoC"); > +MODULE_LICENSE("GPL v2"); Looks quite good otherwise. It looks very similar to the older touchscreen driver (without the touchscreen part). Have you tried to merge the two? Thanks, Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160625/eae212e8/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 2016-06-25 7:10 ` Maxime Ripard @ 2016-06-25 15:12 ` Ondřej Jirman 2016-06-28 11:39 ` Maxime Ripard 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-06-25 15:12 UTC (permalink / raw) To: linux-arm-kernel On 25.6.2016 09:10, Maxime Ripard wrote: > On Sat, Jun 25, 2016 at 05:44:59AM +0200, megous at megous.com wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> This patch adds support for the sun8i thermal sensor on >> Allwinner H3 SoC. >> >> Signed-off-by: Ond?ej Jirman <megous@megous.com> >> --- >> v2: >> - removed incorrect use of SID driver in sun8i_ths >> - read calibration data directly from iomem >> - better explanation for the thermal sensor driver >> - dt documentation fixes >> - dropped unncecessary macros and init code reorganization >> - moved resource aquisition from init to probe function >> - deassert reset after clock rate is set, not before >> - enable irq after all other registers are configured >> --- >> drivers/thermal/Kconfig | 7 ++ >> drivers/thermal/Makefile | 1 + >> drivers/thermal/sun8i_ths.c | 260 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 268 insertions(+) >> create mode 100644 drivers/thermal/sun8i_ths.c >> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig >> index 2d702ca..d3209d9 100644 >> --- a/drivers/thermal/Kconfig >> +++ b/drivers/thermal/Kconfig >> @@ -351,6 +351,13 @@ config MTK_THERMAL >> Enable this option if you want to have support for thermal management >> controller present in Mediatek SoCs >> >> +config SUN8I_THS >> + tristate "Thermal sensor driver for Allwinner H3" >> + depends on MACH_SUN8I >> + depends on OF >> + help >> + Enable this to support thermal reporting on some newer Allwinner SoCs. >> + >> menu "Texas Instruments thermal drivers" >> depends on ARCH_HAS_BANDGAP || COMPILE_TEST >> depends on HAS_IOMEM >> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile >> index 10b07c1..7261ee8 100644 >> --- a/drivers/thermal/Makefile >> +++ b/drivers/thermal/Makefile >> @@ -51,3 +51,4 @@ obj-$(CONFIG_TEGRA_SOCTHERM) += tegra/ >> obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o >> obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o >> obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o >> +obj-$(CONFIG_SUN8I_THS) += sun8i_ths.o >> diff --git a/drivers/thermal/sun8i_ths.c b/drivers/thermal/sun8i_ths.c >> new file mode 100644 >> index 0000000..9ba0f96 >> --- /dev/null >> +++ b/drivers/thermal/sun8i_ths.c >> @@ -0,0 +1,260 @@ >> +/* >> + * Thermal sensor driver for Allwinner H3 SoC >> + * >> + * Copyright (C) 2016 Ond?ej Jirman >> + * Based on the work of Josef Gajdusek <atx@atx.name> >> + * >> + * This software is licensed under the terms of the GNU General Public >> + * License version 2, as published by the Free Software Foundation, and >> + * may be copied, distributed, and modified under those terms. >> + * >> + * This program is distributed in the hope that 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. >> + * >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset.h> >> +#include <linux/slab.h> >> +#include <linux/thermal.h> >> +#include <linux/printk.h> >> + >> +#define THS_H3_CTRL0 0x00 >> +#define THS_H3_CTRL2 0x40 >> +#define THS_H3_INT_CTRL 0x44 >> +#define THS_H3_STAT 0x48 >> +#define THS_H3_FILTER 0x70 >> +#define THS_H3_CDATA 0x74 >> +#define THS_H3_DATA 0x80 >> + >> +#define THS_H3_CTRL0_SENSOR_ACQ0(x) (x) >> +#define THS_H3_CTRL2_SENSE_EN BIT(0) >> +#define THS_H3_CTRL2_SENSOR_ACQ1(x) ((x) << 16) >> +#define THS_H3_INT_CTRL_DATA_IRQ_EN BIT(8) >> +#define THS_H3_INT_CTRL_THERMAL_PER(x) ((x) << 12) >> +#define THS_H3_STAT_DATA_IRQ_STS BIT(8) >> +#define THS_H3_FILTER_TYPE(x) ((x) << 0) >> +#define THS_H3_FILTER_EN BIT(2) >> + >> +#define THS_H3_CLK_IN 40000000 /* Hz */ >> +#define THS_H3_DATA_PERIOD 330 /* ms */ >> + >> +#define THS_H3_FILTER_TYPE_VALUE 2 /* average over 2^(n+1) samples */ >> +#define THS_H3_FILTER_DIV (1 << (THS_H3_FILTER_TYPE_VALUE + 1)) >> +#define THS_H3_INT_CTRL_THERMAL_PER_VALUE \ >> + (THS_H3_DATA_PERIOD * (THS_H3_CLK_IN / 1000) / THS_H3_FILTER_DIV / 4096 - 1) >> +#define THS_H3_CTRL0_SENSOR_ACQ0_VALUE 0x3f /* 16us */ >> +#define THS_H3_CTRL2_SENSOR_ACQ1_VALUE 0x3f >> + >> +struct sun8i_ths_data { >> + struct reset_control *reset; >> + struct clk *clk; >> + struct clk *busclk; >> + void __iomem *regs; >> + void __iomem *calreg; >> + struct thermal_zone_device *tzd; >> + u32 temp; >> +}; >> + >> +static int sun8i_ths_get_temp(void *_data, int *out) >> +{ >> + struct sun8i_ths_data *data = _data; >> + >> + if (data->temp == 0) >> + return -EINVAL; >> + >> + /* Formula and parameters from the Allwinner 3.4 kernel */ >> + *out = 217000 - (int)((data->temp * 1000000) / 8253); >> + return 0; >> +} >> + >> +static irqreturn_t sun8i_ths_irq_thread(int irq, void *_data) >> +{ >> + struct sun8i_ths_data *data = _data; >> + >> + writel(THS_H3_STAT_DATA_IRQ_STS, data->regs + THS_H3_STAT); >> + >> + data->temp = readl(data->regs + THS_H3_DATA); >> + if (data->temp) >> + thermal_zone_device_update(data->tzd); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static void sun8i_ths_h3_init(struct sun8i_ths_data *data) >> +{ >> + u32 caldata; >> + >> + caldata = readl(data->calreg) & 0xfff; >> + if (caldata != 0) >> + writel(caldata, data->regs + THS_H3_CDATA); >> + >> + writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE), >> + data->regs + THS_H3_CTRL0); >> + writel(THS_H3_FILTER_EN | THS_H3_FILTER_TYPE(THS_H3_FILTER_TYPE_VALUE), >> + data->regs + THS_H3_FILTER); >> + writel(THS_H3_CTRL2_SENSOR_ACQ1(THS_H3_CTRL2_SENSOR_ACQ1_VALUE) | >> + THS_H3_CTRL2_SENSE_EN, >> + data->regs + THS_H3_CTRL2); >> + writel(THS_H3_INT_CTRL_THERMAL_PER(THS_H3_INT_CTRL_THERMAL_PER_VALUE) | >> + THS_H3_INT_CTRL_DATA_IRQ_EN, >> + data->regs + THS_H3_INT_CTRL); >> +} >> + >> +static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = { >> + .get_temp = sun8i_ths_get_temp, >> +}; >> + >> +static int sun8i_ths_probe(struct platform_device *pdev) >> +{ >> + struct sun8i_ths_data *data; >> + struct resource *res; >> + int ret; >> + int irq; >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_err(&pdev->dev, "no memory resources defined\n"); >> + return -EINVAL; >> + } >> + >> + data->regs = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(data->regs)) { >> + ret = PTR_ERR(data->regs); >> + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); >> + return ret; >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + if (!res) { >> + dev_err(&pdev->dev, "no calibration data memory resource defined\n"); >> + return -EINVAL; >> + } >> + >> + data->calreg = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(data->calreg)) { >> + ret = PTR_ERR(data->calreg); >> + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); >> + return ret; >> + } > > Why did you remove the SID use through the nvmem framework ?! Because it's overkill for reading a single word from memeory, the sunxi nvmem driver doesn't support H3, the sid is not documented in the datasheet, aside from some registger name/offset dump on the mailing list some time ago. Aside from that, I've yet to see H3 soc that has anything else than 0 in the calibration data memory location. So this is basically nop. Proposed solution seems simpler with no drawbacks that I can see, without resorting to dropping the thing entirely from this driver. Which I would be fine with too. Calibration is optional feature in the BSP kernel, so I assume dropping it may not do too much harm either. If anyone wants to implement sid in the future, it will be easy enough to do with backwards compatibility. The second reg will become optional, and the driver can check for nvmem. >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq); >> + return irq; >> + } >> + >> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, >> + sun8i_ths_irq_thread, IRQF_ONESHOT, >> + dev_name(&pdev->dev), data); >> + if (ret) >> + return ret; >> + >> + data->busclk = devm_clk_get(&pdev->dev, "ahb"); >> + if (IS_ERR(data->busclk)) { >> + ret = PTR_ERR(data->busclk); >> + dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret); >> + return ret; >> + } >> + >> + data->clk = devm_clk_get(&pdev->dev, "ths"); >> + if (IS_ERR(data->clk)) { >> + ret = PTR_ERR(data->clk); >> + dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret); >> + return ret; >> + } >> + >> + data->reset = devm_reset_control_get(&pdev->dev, "ahb"); >> + if (IS_ERR(data->reset)) { >> + ret = PTR_ERR(data->reset); >> + dev_err(&pdev->dev, "failed to get reset: %d\n", ret); >> + return ret; >> + } >> + >> + ret = clk_prepare_enable(data->busclk); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret); >> + return ret; >> + } >> + >> + ret = clk_prepare_enable(data->clk); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret); >> + goto err_disable_bus; >> + } >> + >> + ret = clk_set_rate(data->clk, THS_H3_CLK_IN); >> + if (ret) >> + goto err_disable_ths; >> + >> + ret = reset_control_deassert(data->reset); >> + if (ret) { >> + dev_err(&pdev->dev, "reset deassert failed: %d\n", ret); >> + goto err_disable_ths; >> + } > > Having runtime_pm support would be great. Suspend/resume handling? I would have no way of testing it, other than blindly impelementing what BSP kernel does. Other than that, I can add it quite easily. It should be rather simple. > >> + sun8i_ths_h3_init(data); >> + >> + data->tzd = thermal_zone_of_sensor_register(&pdev->dev, 0, data, >> + &sun8i_ths_thermal_ops); >> + if (IS_ERR(data->tzd)) { >> + ret = PTR_ERR(data->tzd); >> + dev_err(&pdev->dev, "failed to register thermal zone: %d\n", >> + ret); >> + goto err_assert_reset; >> + } > > You reference data->tzd in your interrupt handler, and the interrupts > have been activated before initializing that field. That is likely to > cause a kernel crash when you receive an interrupt between your > request_irq and that call. Good catch. I'll just move the above sun8i_ths_h3_init call below the tzd initialization. That should fix it. >> + >> + platform_set_drvdata(pdev, data); >> + return 0; >> + >> +err_assert_reset: >> + reset_control_assert(data->reset); >> +err_disable_ths: >> + clk_disable_unprepare(data->clk); >> +err_disable_bus: >> + clk_disable_unprepare(data->busclk); >> + return ret; >> +} >> + >> +static int sun8i_ths_remove(struct platform_device *pdev) >> +{ >> + struct sun8i_ths_data *data = platform_get_drvdata(pdev); >> + >> + thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd); >> + reset_control_assert(data->reset); >> + clk_disable_unprepare(data->clk); >> + clk_disable_unprepare(data->busclk); >> + return 0; >> +} >> + >> +static const struct of_device_id sun8i_ths_id_table[] = { >> + { .compatible = "allwinner,sun8i-h3-ths", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, sun8i_ths_id_table); >> + >> +static struct platform_driver sun8i_ths_driver = { >> + .probe = sun8i_ths_probe, >> + .remove = sun8i_ths_remove, >> + .driver = { >> + .name = "sun8i_ths", >> + .of_match_table = sun8i_ths_id_table, >> + }, >> +}; >> + >> +module_platform_driver(sun8i_ths_driver); >> + >> +MODULE_AUTHOR("Ond?ej Jirman <megous@megous.com>"); >> +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner H3 SoC"); >> +MODULE_LICENSE("GPL v2"); > > Looks quite good otherwise. It looks very similar to the older > touchscreen driver (without the touchscreen part). > > Have you tried to merge the two? What driver? Thank you very much for the review. regards, Ondrej > Thanks, > Maxime > ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 2016-06-25 15:12 ` Ondřej Jirman @ 2016-06-28 11:39 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-06-28 11:39 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:12:41PM +0200, Ond?ej Jirman wrote: > >> + data->calreg = devm_ioremap_resource(&pdev->dev, res); > >> + if (IS_ERR(data->calreg)) { > >> + ret = PTR_ERR(data->calreg); > >> + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret); > >> + return ret; > >> + } > > > > Why did you remove the SID use through the nvmem framework ?! > > Because it's overkill for reading a single word from memeory, the sunxi > nvmem driver doesn't support H3, the sid is not documented in the > datasheet, aside from some registger name/offset dump on the mailing > list some time ago. > > Aside from that, I've yet to see H3 soc that has anything else than 0 in > the calibration data memory location. So this is basically nop. > > Proposed solution seems simpler with no drawbacks that I can see, > without resorting to dropping the thing entirely from this driver. Which > I would be fine with too. Calibration is optional feature in the BSP > kernel, so I assume dropping it may not do too much harm either. > > If anyone wants to implement sid in the future, it will be easy enough > to do with backwards compatibility. The second reg will become optional, > and the driver can check for nvmem. A lot of things in drivers boil down to "reading a single word from memory". However, abstractions are here for a reason, and there's none to not use it. If that's not something we can use today, remove it entirely. And if that becomes necessary, we will add an optional nvmem property. > >> + > >> + irq = platform_get_irq(pdev, 0); > >> + if (irq < 0) { > >> + dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq); > >> + return irq; > >> + } > >> + > >> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > >> + sun8i_ths_irq_thread, IRQF_ONESHOT, > >> + dev_name(&pdev->dev), data); > >> + if (ret) > >> + return ret; > >> + > >> + data->busclk = devm_clk_get(&pdev->dev, "ahb"); > >> + if (IS_ERR(data->busclk)) { > >> + ret = PTR_ERR(data->busclk); > >> + dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + data->clk = devm_clk_get(&pdev->dev, "ths"); > >> + if (IS_ERR(data->clk)) { > >> + ret = PTR_ERR(data->clk); > >> + dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + data->reset = devm_reset_control_get(&pdev->dev, "ahb"); > >> + if (IS_ERR(data->reset)) { > >> + ret = PTR_ERR(data->reset); > >> + dev_err(&pdev->dev, "failed to get reset: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = clk_prepare_enable(data->busclk); > >> + if (ret) { > >> + dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = clk_prepare_enable(data->clk); > >> + if (ret) { > >> + dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret); > >> + goto err_disable_bus; > >> + } > >> + > >> + ret = clk_set_rate(data->clk, THS_H3_CLK_IN); > >> + if (ret) > >> + goto err_disable_ths; > >> + > >> + ret = reset_control_deassert(data->reset); > >> + if (ret) { > >> + dev_err(&pdev->dev, "reset deassert failed: %d\n", ret); > >> + goto err_disable_ths; > >> + } > > > > Having runtime_pm support would be great. > > Suspend/resume handling? I would have no way of testing it, other than > blindly impelementing what BSP kernel does. Other than that, I can add > it quite easily. It should be rather simple. No, I mean runtime_pm, with runtime_suspend and runtime_resume, to allow only powering up the device when it's used, and shut it down when not used. > >> +MODULE_AUTHOR("Ond?ej Jirman <megous@megous.com>"); > >> +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner H3 SoC"); > >> +MODULE_LICENSE("GPL v2"); > > > > Looks quite good otherwise. It looks very similar to the older > > touchscreen driver (without the touchscreen part). > > > > Have you tried to merge the two? > > What driver? drivers/input/touchscreen/sun4i-ts.c Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160628/59304ceb/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com 2016-06-25 3:44 ` [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock megous at megous.com 2016-06-25 3:44 ` [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-28 20:56 ` Rob Herring 2016-06-25 3:45 ` [PATCH v2 04/14] regulator: SY8106A regulator driver megous at megous.com ` (10 subsequent siblings) 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> This patch adds the binding documentation for the sun8i_ths driver. This is a driver for thermal sensor found in Allwinner H3 SoC. Signed-off-by: Ond?ej Jirman <megous@megous.com> --- .../devicetree/bindings/thermal/sun8i-ths.txt | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/sun8i-ths.txt diff --git a/Documentation/devicetree/bindings/thermal/sun8i-ths.txt b/Documentation/devicetree/bindings/thermal/sun8i-ths.txt new file mode 100644 index 0000000..76859d4 --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/sun8i-ths.txt @@ -0,0 +1,26 @@ +* Thermal sensor driver for Allwinner H3 SoC + +Required properties: +- compatible : "allwinner,sun8i-h3-ths" +- reg : Address range of the thermal sensor registers and of the calibration + data +- resets : Must contain phandles to reset controls matching the entries + of the names +- reset-names : Must include the name "ahb" +- clocks : Must contain phandles to clock controls matching the entries + of the names +- clock-names : Must contain "ahb" for the bus gate and "ths" for the THS + clock + +Example: +ths: ths at 01c25000 { + #thermal-sensor-cells = <0>; + compatible = "allwinner,sun8i-h3-ths"; + reg = <0x01c25000 0x400>, + <0x01c14234 0x4>; + interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>; + resets = <&bus_rst 136>; + reset-names = "ahb"; + clocks = <&bus_gates 72>, <&ths_clk>; + clock-names = "ahb", "ths"; +}; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver 2016-06-25 3:45 ` [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver megous at megous.com @ 2016-06-28 20:56 ` Rob Herring 0 siblings, 0 replies; 66+ messages in thread From: Rob Herring @ 2016-06-28 20:56 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:45:00AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > This patch adds the binding documentation for the > sun8i_ths driver. This is a driver for thermal sensor > found in Allwinner H3 SoC. > > Signed-off-by: Ond?ej Jirman <megous@megous.com> > --- > .../devicetree/bindings/thermal/sun8i-ths.txt | 26 ++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > create mode 100644 Documentation/devicetree/bindings/thermal/sun8i-ths.txt I guess the example will change for the clocks some, but otherwise: Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 04/14] regulator: SY8106A regulator driver 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (2 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-26 11:26 ` Mark Brown 2016-06-27 15:10 ` Mark Brown 2016-06-25 3:45 ` [PATCH v2 05/14] dt-bindings: document " megous at megous.com ` (9 subsequent siblings) 13 siblings, 2 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> SY8106A is I2C attached single output voltage regulator made by Silergy. Signed-off-by: Ondrej Jirman <megous@megous.com> --- v2 - added dt-bindings for the regulator - changed to use of_device_id for probing - added initialization failure checks --- drivers/regulator/Kconfig | 8 +- drivers/regulator/Makefile | 2 +- drivers/regulator/sy8106a-regulator.c | 171 ++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 drivers/regulator/sy8106a-regulator.c diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 144cbf5..93f3fe4f 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -702,6 +702,13 @@ config REGULATOR_STW481X_VMMC This driver supports the internal VMMC regulator in the STw481x PMIC chips. +config REGULATOR_SY8106A + tristate "Silergy SY8106A" + depends on I2C && (OF || COMPILE_TEST) + select REGMAP_I2C + help + This driver provides support for SY8106A voltage regulator. + config REGULATOR_TPS51632 tristate "TI TPS51632 Power Regulator" depends on I2C @@ -861,4 +868,3 @@ config REGULATOR_WM8994 WM8994 CODEC. endif - diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 85a1d44..e3f720f 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -88,6 +88,7 @@ obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o @@ -111,5 +112,4 @@ obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o - ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG diff --git a/drivers/regulator/sy8106a-regulator.c b/drivers/regulator/sy8106a-regulator.c new file mode 100644 index 0000000..98626bc --- /dev/null +++ b/drivers/regulator/sy8106a-regulator.c @@ -0,0 +1,171 @@ +/* + * sy8106a-regulator.c - Regulator device driver for SY8106A + * + * Copyright (C) 2016 Ond?ej Jirman <megous@megous.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, + * Boston, MA 02110-1301, USA. + */ + +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/of_regulator.h> + +#define SY8106A_REG_VOUT1_SEL 0x01 +#define SY8106A_REG_VOUT_COM 0x02 +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f +#define SY8106A_DISABLE_REG BIT(0) +#define SY8106A_GO_BIT BIT(7) + +struct sy8106a { + struct regulator_dev *rdev; + struct regmap *regmap; +}; + +static const struct regmap_config sy8106a_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned sel) +{ + /* We use our set_voltage_sel in order to avoid unnecessary I2C chatter, + * because the regulator_get_voltage_sel_regmap using apply_bit + * would perform 4 unnecessary transfers instead of one, increasing the + * chance of error. + */ + return regmap_write(rdev->regmap, rdev->desc->vsel_reg, + sel | SY8106A_GO_BIT); +} + +static const struct regulator_ops sy8106a_ops = { + .is_enabled = regulator_is_enabled_regmap, + .set_voltage_sel = sy8106a_set_voltage_sel, + .set_voltage_time_sel = regulator_set_voltage_time_sel, + .get_voltage_sel = regulator_get_voltage_sel_regmap, + .list_voltage = regulator_list_voltage_linear, +}; + +/* Default limits measured in millivolts and milliamps */ +#define SY8106A_MIN_MV 680 +#define SY8106A_MAX_MV 1950 +#define SY8106A_STEP_MV 10 + +static const struct regulator_desc sy8106a_reg = { + .name = "SY8106A", + .id = 0, + .ops = &sy8106a_ops, + .type = REGULATOR_VOLTAGE, + .n_voltages = ((SY8106A_MAX_MV - SY8106A_MIN_MV) / SY8106A_STEP_MV) + 1, + .min_uV = (SY8106A_MIN_MV * 1000), + .uV_step = (SY8106A_STEP_MV * 1000), + .vsel_reg = SY8106A_REG_VOUT1_SEL, + .vsel_mask = SY8106A_REG_VOUT1_SEL_MASK, + .enable_reg = SY8106A_REG_VOUT_COM, + .enable_mask = SY8106A_DISABLE_REG, + .disable_val = SY8106A_DISABLE_REG, + .enable_is_inverted = 1, + .owner = THIS_MODULE, +}; + +/* + * I2C driver interface functions + */ +static int sy8106a_i2c_probe(struct i2c_client *i2c, + const struct i2c_device_id *id) +{ + struct sy8106a *chip; + struct device *dev = &i2c->dev; + struct regulator_dev *rdev = NULL; + struct regulator_config config = { }; + unsigned int selector; + int error; + + chip = devm_kzalloc(&i2c->dev, sizeof(struct sy8106a), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->regmap = devm_regmap_init_i2c(i2c, &sy8106a_regmap_config); + if (IS_ERR(chip->regmap)) { + error = PTR_ERR(chip->regmap); + dev_err(&i2c->dev, "Failed to allocate register map: %d\n", + error); + return error; + } + + config.dev = &i2c->dev; + config.regmap = chip->regmap; + config.of_node = dev->of_node; + config.driver_data = chip; + config.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg); + if (!config.init_data) { + return -EINVAL; + } + + /* Probe regulator */ + error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector); + if (error) { + dev_err(&i2c->dev, "Failed to read voltage at probe time: %d\n", error); + return error; + } + + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK)); + + rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, &config); + if (IS_ERR(rdev)) { + error = PTR_ERR(rdev); + dev_err(&i2c->dev, "Failed to register SY8106A regulator: %d\n", error); + return error; + } + + chip->rdev = rdev; + + i2c_set_clientdata(i2c, chip); + + return 0; +} + +static const struct of_device_id sy8106a_i2c_of_match[] = { + { .compatible = "silergy,sy8106a" }, + { }, +}; +MODULE_DEVICE_TABLE(of, sy8106a_i2c_of_match); + +/* + * This is useless for OF-enabled devices, but it is needed by I2C subsystem + */ +static const struct i2c_device_id sy8106a_i2c_id[] = { + { }, +}; +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id); + +static struct i2c_driver sy8106a_regulator_driver = { + .driver = { + .name = "sy8106a", + .of_match_table = of_match_ptr(sy8106a_i2c_of_match), + }, + .probe = sy8106a_i2c_probe, + .id_table = sy8106a_i2c_id, +}; + +module_i2c_driver(sy8106a_regulator_driver); + +MODULE_AUTHOR("Ond?ej Jirman <megous@megous.com>"); +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A"); +MODULE_LICENSE("GPL v2"); -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 04/14] regulator: SY8106A regulator driver 2016-06-25 3:45 ` [PATCH v2 04/14] regulator: SY8106A regulator driver megous at megous.com @ 2016-06-26 11:26 ` Mark Brown 2016-06-26 15:07 ` Ondřej Jirman 2016-06-27 15:10 ` Mark Brown 1 sibling, 1 reply; 66+ messages in thread From: Mark Brown @ 2016-06-26 11:26 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:45:01AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > SY8106A is I2C attached single output voltage regulator > made by Silergy. I'm missing almost all of this series, I've just got this and another patch which look like a standalone driver so it's hard to see any dependencies. What's going on here? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160626/1264acd9/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 04/14] regulator: SY8106A regulator driver 2016-06-26 11:26 ` Mark Brown @ 2016-06-26 15:07 ` Ondřej Jirman 2016-06-27 14:54 ` Mark Brown 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-06-26 15:07 UTC (permalink / raw) To: linux-arm-kernel On 26.6.2016 13:26, Mark Brown wrote: > On Sat, Jun 25, 2016 at 05:45:01AM +0200, megous at megous.com wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> SY8106A is I2C attached single output voltage regulator >> made by Silergy. > > I'm missing almost all of this series, I've just got this and another > patch which look like a standalone driver so it's hard to see any > dependencies. What's going on here? > Sorry, it's this series: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html And here's v1 intro letter: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html regards, Ondrej Jirman ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 04/14] regulator: SY8106A regulator driver 2016-06-26 15:07 ` Ondřej Jirman @ 2016-06-27 14:54 ` Mark Brown 2016-06-28 16:27 ` Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Mark Brown @ 2016-06-27 14:54 UTC (permalink / raw) To: linux-arm-kernel On Sun, Jun 26, 2016 at 05:07:16PM +0200, Ond?ej Jirman wrote: > On 26.6.2016 13:26, Mark Brown wrote: > > I'm missing almost all of this series, I've just got this and another > > patch which look like a standalone driver so it's hard to see any > > dependencies. What's going on here? > Sorry, it's this series: > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html > And here's v1 intro letter: > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html These aren't really enlightening I'm afraid, there's nothing in there about why these are a single series and no dependency information. Indeed it looks like something is really confused as the thread is missing at least patches 2 and 3. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160627/63382a9b/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 04/14] regulator: SY8106A regulator driver 2016-06-27 14:54 ` Mark Brown @ 2016-06-28 16:27 ` Ondřej Jirman 0 siblings, 0 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-06-28 16:27 UTC (permalink / raw) To: linux-arm-kernel On 27.6.2016 16:54, Mark Brown wrote: > On Sun, Jun 26, 2016 at 05:07:16PM +0200, Ond?ej Jirman wrote: >> On 26.6.2016 13:26, Mark Brown wrote: > >>> I'm missing almost all of this series, I've just got this and another >>> patch which look like a standalone driver so it's hard to see any >>> dependencies. What's going on here? > >> Sorry, it's this series: >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html > >> And here's v1 intro letter: >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html > > These aren't really enlightening I'm afraid, there's nothing in there > about why these are a single series and no dependency information. > Indeed it looks like something is really confused as the thread is > missing at least patches 2 and 3. > You're right there's not much about the regulator there. Xulong Orange Pi PC is ARM SBC, that uses Allwinner H3 Soc, to which this regulator is connected via I2C bus, to regulate the main CPU supply voltage. It is used on some other SBC's from Xulong. It is fairly simple single output, fixed I2C address voltage regulator. It has just 3 registers, which allow for: slew rate control (not used in this driver), setting and reading out the voltage, switching between voltage set via I2C and via external resistor divider and enabling/disabling output. It also has status reporting for over-tempertature and over-current conditions. That's all the features it has. The patch series uses it to provide dynamic vltage scaling of the ARM cores in the SoC to be able to achieve higher CPU frequency. Datasheet is here: https://01916271185794791722.googlegroups.com/attach/93415fbd5402/SY8106A_datasheet.pdf?part=0.1&vt=ANaJVrHMej8w8XRfd-7A3XoyiryMDDhrHU2SS-tYyHCpOIXRqIaICOIlZTWAUV74vToesmic457zPvODDuvrNnRpomPOPbtV8bMMFV65VX5aYb5NciMkh8E I'll also include this information in the revised patch. thank you and regards, Ondrej Jirman -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160628/e4ec00eb/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 04/14] regulator: SY8106A regulator driver 2016-06-25 3:45 ` [PATCH v2 04/14] regulator: SY8106A regulator driver megous at megous.com 2016-06-26 11:26 ` Mark Brown @ 2016-06-27 15:10 ` Mark Brown 1 sibling, 0 replies; 66+ messages in thread From: Mark Brown @ 2016-06-27 15:10 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:45:01AM +0200, megous at megous.com wrote: > + config.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg); > + if (!config.init_data) { > + return -EINVAL; > + } This is broken, constraints are entirely optional - the driver should just instantiate no matter what (if anything) the constraints are. This means people can read the current state even if there is no need for runtime configuration. > + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + > + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK)); This is just noise, remove it - if we want to list the voltage for regulators at probe we should be doing it consistently for all regulators in the core rather than in individual drivers. > +/* > + * This is useless for OF-enabled devices, but it is needed by I2C subsystem > + */ > +static const struct i2c_device_id sy8106a_i2c_id[] = { > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id); No, this is not the case - supporting DT does not mean that other enumeration mechanisms can't be supported and there's no reason not to list a sensible ID here. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160627/1d409030/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 05/14] dt-bindings: document SY8106A regulator driver 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (3 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 04/14] regulator: SY8106A regulator driver megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-26 11:27 ` Mark Brown 2016-06-25 3:45 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method megous at megous.com ` (8 subsequent siblings) 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> This patch adds the binding documentation for the sy8106a regulator driver. Signed-off-by: Ondrej Jirman <megous@megous.com> --- .../bindings/regulator/sy8106a-regulator.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/sy8106a-regulator.txt diff --git a/Documentation/devicetree/bindings/regulator/sy8106a-regulator.txt b/Documentation/devicetree/bindings/regulator/sy8106a-regulator.txt new file mode 100644 index 0000000..1e623a34 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/sy8106a-regulator.txt @@ -0,0 +1,21 @@ +SY8106A Voltage regulator + +Required properties: +- compatible: Must be "silergy,sy8106a" +- reg: I2C slave address - must be <0x65> + +Any property defined as part of the core regulator binding, defined in +regulator.txt, can also be used. + +Example: + + sy8106a { + compatible = "silergy,sy8106a"; + reg = <0x65>; + regulator-name = "sy8106a-vdd"; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1400000>; + regulator-ramp-delay = <200>; + regulator-boot-on; + regulator-always-on; + }; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 05/14] dt-bindings: document SY8106A regulator driver 2016-06-25 3:45 ` [PATCH v2 05/14] dt-bindings: document " megous at megous.com @ 2016-06-26 11:27 ` Mark Brown 2016-06-26 15:10 ` Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Mark Brown @ 2016-06-26 11:27 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:45:02AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > This patch adds the binding documentation for the > sy8106a regulator driver. Please submit patches using subject lines reflecting the style for the subsystem. This makes it easier for people to identify relevant patches. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160626/0dcf4d06/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 05/14] dt-bindings: document SY8106A regulator driver 2016-06-26 11:27 ` Mark Brown @ 2016-06-26 15:10 ` Ondřej Jirman 2016-06-26 18:52 ` Mark Brown 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-06-26 15:10 UTC (permalink / raw) To: linux-arm-kernel On 26.6.2016 13:27, Mark Brown wrote: > On Sat, Jun 25, 2016 at 05:45:02AM +0200, megous at megous.com wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> This patch adds the binding documentation for the >> sy8106a regulator driver. > > Please submit patches using subject lines reflecting the style for the > subsystem. This makes it easier for people to identify relevant > patches. > Hello, I assume that means "regulator: dt-bindings: ..." in this case. I'll keep that in mind in the future. Thank you. ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 05/14] dt-bindings: document SY8106A regulator driver 2016-06-26 15:10 ` Ondřej Jirman @ 2016-06-26 18:52 ` Mark Brown 0 siblings, 0 replies; 66+ messages in thread From: Mark Brown @ 2016-06-26 18:52 UTC (permalink / raw) To: linux-arm-kernel On Sun, Jun 26, 2016 at 05:10:06PM +0200, Ond?ej Jirman wrote: > Hello, I assume that means "regulator: dt-bindings: ..." in this case. > I'll keep that in mind in the future. Thank you. Yes, or "regulator: sy8106a: " - as ever take a look at how other commits in the same file/directory are done. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160626/1c336117/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (4 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 05/14] dt-bindings: document " megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-30 20:40 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 07/14] ARM: dts: sun8i: Use sun8i-h3-pll1-clk for pll1 in H3 megous at megous.com ` (7 subsequent siblings) 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> PLL1 on H3 requires special factors application algorithm, when the rate is changed. This algorithm was extracted from the arisc code that handles frequency scaling in the BSP kernel. This commit adds optional apply function to struct factors_data, that can implement non-trivial factors application method, when necessary. Also struct clk_factors_config is extended with position of the PLL lock flag. Signed-off-by: Ondrej Jirman <megous@megous.com> --- Documentation/devicetree/bindings/clock/sunxi.txt | 1 + drivers/clk/sunxi/clk-factors.c | 34 +++++------ drivers/clk/sunxi/clk-factors.h | 12 ++++ drivers/clk/sunxi/clk-sunxi.c | 72 ++++++++++++++++++++++- 4 files changed, 98 insertions(+), 21 deletions(-) diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt index 5faae05..774500c 100644 --- a/Documentation/devicetree/bindings/clock/sunxi.txt +++ b/Documentation/devicetree/bindings/clock/sunxi.txt @@ -10,6 +10,7 @@ Required properties: "allwinner,sun4i-a10-pll1-clk" - for the main PLL clock and PLL4 "allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31 "allwinner,sun8i-a23-pll1-clk" - for the main PLL clock on A23 + "allwinner,sun8i-h3-pll1-clk" - for the main PLL clock on H3 "allwinner,sun4i-a10-pll3-clk" - for the video PLL clock on A10 "allwinner,sun9i-a80-pll4-clk" - for the peripheral PLLs on A80 "allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c index ddefe96..7c165db 100644 --- a/drivers/clk/sunxi/clk-factors.c +++ b/drivers/clk/sunxi/clk-factors.c @@ -34,13 +34,6 @@ #define FACTORS_MAX_PARENTS 5 -#define SETMASK(len, pos) (((1U << (len)) - 1) << (pos)) -#define CLRMASK(len, pos) (~(SETMASK(len, pos))) -#define FACTOR_GET(bit, len, reg) (((reg) & SETMASK(len, bit)) >> (bit)) - -#define FACTOR_SET(bit, len, reg, val) \ - (((reg) & CLRMASK(len, bit)) | (val << (bit))) - static unsigned long clk_factors_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) { @@ -150,20 +143,24 @@ static int clk_factors_set_rate(struct clk_hw *hw, unsigned long rate, if (factors->lock) spin_lock_irqsave(factors->lock, flags); - /* Fetch the register value */ - reg = readl(factors->reg); + if (factors->apply) { + factors->apply(factors, &req); + } else { + /* Fetch the register value */ + reg = readl(factors->reg); - /* Set up the new factors - macros do not do anything if width is 0 */ - reg = FACTOR_SET(config->nshift, config->nwidth, reg, req.n); - reg = FACTOR_SET(config->kshift, config->kwidth, reg, req.k); - reg = FACTOR_SET(config->mshift, config->mwidth, reg, req.m); - reg = FACTOR_SET(config->pshift, config->pwidth, reg, req.p); + /* Set up the new factors - macros do not do anything if width is 0 */ + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req.n); + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req.k); + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req.m); + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req.p); - /* Apply them now */ - writel(reg, factors->reg); + /* Apply them now */ + writel(reg, factors->reg); - /* delay 500us so pll stabilizes */ - __delay((rate >> 20) * 500 / 2); + /* delay 500us so pll stabilizes */ + __delay((rate >> 20) * 500 / 2); + } if (factors->lock) spin_unlock_irqrestore(factors->lock, flags); @@ -213,6 +210,7 @@ struct clk *sunxi_factors_register(struct device_node *node, factors->config = data->table; factors->get_factors = data->getter; factors->recalc = data->recalc; + factors->apply = data->apply; factors->lock = lock; /* Add a gate if this factor clock can be gated */ diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h index 1e63c5b..661a45a 100644 --- a/drivers/clk/sunxi/clk-factors.h +++ b/drivers/clk/sunxi/clk-factors.h @@ -6,6 +6,13 @@ #define SUNXI_FACTORS_NOT_APPLICABLE (0) +#define SETMASK(len, pos) (((1U << (len)) - 1) << (pos)) +#define CLRMASK(len, pos) (~(SETMASK(len, pos))) +#define FACTOR_GET(bit, len, reg) (((reg) & SETMASK(len, bit)) >> (bit)) + +#define FACTOR_SET(bit, len, reg, val) \ + (((reg) & CLRMASK(len, bit)) | (val << (bit))) + struct clk_factors_config { u8 nshift; u8 nwidth; @@ -16,6 +23,7 @@ struct clk_factors_config { u8 pshift; u8 pwidth; u8 n_start; + u8 lock; }; struct factors_request { @@ -28,6 +36,8 @@ struct factors_request { u8 p; }; +struct clk_factors; + struct factors_data { int enable; int mux; @@ -35,6 +45,7 @@ struct factors_data { const struct clk_factors_config *table; void (*getter)(struct factors_request *req); void (*recalc)(struct factors_request *req); + void (*apply)(struct clk_factors *factors, struct factors_request *req); const char *name; }; @@ -44,6 +55,7 @@ struct clk_factors { const struct clk_factors_config *config; void (*get_factors)(struct factors_request *req); void (*recalc)(struct factors_request *req); + void (*apply)(struct clk_factors *factors, struct factors_request *req); spinlock_t *lock; /* for cleanup */ struct clk_mux *mux; diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index 838b22a..e4bb908 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -23,6 +23,7 @@ #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/log2.h> +#include <linux/delay.h> #include "clk-factors.h" @@ -200,6 +201,56 @@ static void sun8i_a23_get_pll1_factors(struct factors_request *req) } /** + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the + * register using an algorithm that tries to reserve the PLL lock + */ + +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) +{ + const struct clk_factors_config *config = factors->config; + u32 reg; + + /* Fetch the register value */ + reg = readl(factors->reg); + + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); + + writel(reg, factors->reg); + __delay(2000); + } + + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); + + writel(reg, factors->reg); + __delay(2000); + } + + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); + + writel(reg, factors->reg); + __delay(20); + + while (!(readl(factors->reg) & (1 << config->lock))); + + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) { + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); + + writel(reg, factors->reg); + __delay(2000); + } + + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) { + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); + + writel(reg, factors->reg); + __delay(2000); + } +} + +/** * sun4i_get_pll5_factors() - calculates n, k factors for PLL5 * PLL5 rate is calculated as follows * rate = parent_rate * n * (k + 1) @@ -451,6 +502,7 @@ static const struct clk_factors_config sun8i_a23_pll1_config = { .pshift = 16, .pwidth = 2, .n_start = 1, + .lock = 28 }; static const struct clk_factors_config sun4i_pll5_config = { @@ -513,6 +565,13 @@ static const struct factors_data sun8i_a23_pll1_data __initconst = { .getter = sun8i_a23_get_pll1_factors, }; +static const struct factors_data sun8i_h3_pll1_data __initconst = { + .enable = 31, + .table = &sun8i_a23_pll1_config, + .getter = sun8i_a23_get_pll1_factors, + .apply = sun8i_h3_apply_pll1_factors, +}; + static const struct factors_data sun7i_a20_pll4_data __initconst = { .enable = 31, .table = &sun4i_pll5_config, @@ -590,12 +649,19 @@ static void __init sun6i_pll1_clk_setup(struct device_node *node) CLK_OF_DECLARE(sun6i_pll1, "allwinner,sun6i-a31-pll1-clk", sun6i_pll1_clk_setup); -static void __init sun8i_pll1_clk_setup(struct device_node *node) +static void __init sun8i_a23_pll1_clk_setup(struct device_node *node) { sunxi_factors_clk_setup(node, &sun8i_a23_pll1_data); } -CLK_OF_DECLARE(sun8i_pll1, "allwinner,sun8i-a23-pll1-clk", - sun8i_pll1_clk_setup); +CLK_OF_DECLARE(sun8i_a23_pll1, "allwinner,sun8i-a23-pll1-clk", + sun8i_a23_pll1_clk_setup); + +static void __init sun8i_h3_pll1_clk_setup(struct device_node *node) +{ + sunxi_factors_clk_setup(node, &sun8i_h3_pll1_data); +} +CLK_OF_DECLARE(sun8i_h3_pll1, "allwinner,sun8i-h3-pll1-clk", + sun8i_h3_pll1_clk_setup); static void __init sun7i_pll4_clk_setup(struct device_node *node) { -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-06-25 3:45 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method megous at megous.com @ 2016-06-30 20:40 ` Maxime Ripard 2016-07-01 0:50 ` Ondřej Jirman 2016-07-01 0:53 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method Ondřej Jirman 0 siblings, 2 replies; 66+ messages in thread From: Maxime Ripard @ 2016-06-30 20:40 UTC (permalink / raw) To: linux-arm-kernel Hi, On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > PLL1 on H3 requires special factors application algorithm, > when the rate is changed. This algorithm was extracted > from the arisc code that handles frequency scaling > in the BSP kernel. > > This commit adds optional apply function to > struct factors_data, that can implement non-trivial > factors application method, when necessary. > > Also struct clk_factors_config is extended with position > of the PLL lock flag. Have you tested the current implementation, and found that it was not working, or did you duplicate the arisc code directly? > /** > + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the > + * register using an algorithm that tries to reserve the PLL lock > + */ > + > +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) > +{ > + const struct clk_factors_config *config = factors->config; > + u32 reg; > + > + /* Fetch the register value */ > + reg = readl(factors->reg); > + > + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { > + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); > + > + writel(reg, factors->reg); > + __delay(2000); > + } So there was some doubts about the fact that P was being used, or at least that it was useful. > + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { > + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); > + > + writel(reg, factors->reg); > + __delay(2000); > + } > + > + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); > + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); > + > + writel(reg, factors->reg); > + __delay(20); > + > + while (!(readl(factors->reg) & (1 << config->lock))); So, they are applying the dividers first, and then applying the multipliers, and then wait for the PLL to stabilize. > + > + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) { > + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); > + > + writel(reg, factors->reg); > + __delay(2000); > + } > + > + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) { > + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); > + > + writel(reg, factors->reg); > + __delay(2000); > + } However, this is kind of weird, why would you need to re-apply the dividers? Nothing really changes. Have you tried without that part? Since this is really specific, I guess you could simply make the clk_ops for the nkmp clocks public, and just re-implement set_rate using that logic. You might also need to set an upper limit on P, since the last value (4) is not a valid one. I guess you could do that by adding a max field in the __ccu_div structure. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160630/57038018/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-06-30 20:40 ` Maxime Ripard @ 2016-07-01 0:50 ` Ondřej Jirman 2016-07-01 5:37 ` Jean-Francois Moine 2016-07-15 8:53 ` Maxime Ripard 2016-07-01 0:53 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method Ondřej Jirman 1 sibling, 2 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-07-01 0:50 UTC (permalink / raw) To: linux-arm-kernel Hi, On 30.6.2016 22:40, Maxime Ripard wrote: > Hi, > > On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous at megous.com wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> PLL1 on H3 requires special factors application algorithm, >> when the rate is changed. This algorithm was extracted >> from the arisc code that handles frequency scaling >> in the BSP kernel. >> >> This commit adds optional apply function to >> struct factors_data, that can implement non-trivial >> factors application method, when necessary. >> >> Also struct clk_factors_config is extended with position >> of the PLL lock flag. > > Have you tested the current implementation, and found that it was not > working, or did you duplicate the arisc code directly? I have tested the current implementation, and it was not working. It depended on some other factors, like the initial setup done by u-boot. It didn't work reliably. Then I reverse engineered arisc, in an effort to see what's the difference, between mainline and BSP code. >> /** >> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the >> + * register using an algorithm that tries to reserve the PLL lock >> + */ >> + >> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) >> +{ >> + const struct clk_factors_config *config = factors->config; >> + u32 reg; >> + >> + /* Fetch the register value */ >> + reg = readl(factors->reg); >> + >> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { >> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } > > So there was some doubts about the fact that P was being used, or at > least that it was useful. p is necessary to reduce frequencies below 288 MHz according to the datasheet. >> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { >> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } >> + >> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); >> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); >> + >> + writel(reg, factors->reg); >> + __delay(20); >> + >> + while (!(readl(factors->reg) & (1 << config->lock))); > > So, they are applying the dividers first, and then applying the > multipliers, and then wait for the PLL to stabilize. Not exactly, first we are increasing dividers if the new dividers are higher that that what's already set. This ensures that because application of dividers is immediate by the design of the PLL, the application of multipliers isn't. So the VCO would still run at the same frequency for a while gradually rising to a new value for example, while the dividers would be reduced immediately. Leading to crash. PLL -------------------------- PRE DIV(f0) -> VCO(f1) -> POST DIV(f2) P K,N M Example: (we set all factors at once, reducing dividers and multipliers at the same time at 0ms - this should lead to no change in the output frequency, but...) -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 2GHz - boom 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz The current code crashes exactly at boom, you don't get any more instructions to execute. See. So this patch first increases dividers (only if necessary), changes multipliers and waits for change to happen (takes around 2000 cycles), and then decreases dividers (only if necessary). So we get: -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz - no boom, multiplier reduced 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz 1.9ms: f0 = 24MHz, f1 = 1GHz, f2 = 0.5GHz - we got PLL sync 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz - and here we reduce divider at last >> + >> + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) { >> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } >> + >> + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) { >> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } > > However, this is kind of weird, why would you need to re-apply the > dividers? Nothing really changes. Have you tried without that part? See above, we either increase before PLL change, or reduce dividers after the change. Nothing is re-applied. > Since this is really specific, I guess you could simply make the > clk_ops for the nkmp clocks public, and just re-implement set_rate > using that logic. I would argue that this may be necessary for other PLL clocks too, if you can get out of bounds output frequency, by changing the dividers too early or too late. So perhaps this code should be generalized for other PLL clocks too, instead. > > You might also need to set an upper limit on P, since the last value > (4) is not a valid one. I think, that should be done by the factors calculation function already. > I guess you could do that by adding a max field in the __ccu_div > structure. > > Maxime > regards, Ondrej -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160701/76a7525e/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-01 0:50 ` Ondřej Jirman @ 2016-07-01 5:37 ` Jean-Francois Moine 2016-07-01 6:34 ` Ondřej Jirman 2016-07-15 8:53 ` Maxime Ripard 1 sibling, 1 reply; 66+ messages in thread From: Jean-Francois Moine @ 2016-07-01 5:37 UTC (permalink / raw) To: linux-arm-kernel On Fri, 1 Jul 2016 02:50:57 +0200 Ond?ej Jirman <megous@megous.com> wrote: > > Since this is really specific, I guess you could simply make the > > clk_ops for the nkmp clocks public, and just re-implement set_rate > > using that logic. > > I would argue that this may be necessary for other PLL clocks too, if > you can get out of bounds output frequency, by changing the dividers too > early or too late. So perhaps this code should be generalized for other > PLL clocks too, instead. The documentation says that only the CPU and DDR PLLs can be dynamically changed after boot. -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-01 5:37 ` Jean-Francois Moine @ 2016-07-01 6:34 ` Ondřej Jirman 2016-07-01 7:47 ` Jean-Francois Moine 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-07-01 6:34 UTC (permalink / raw) To: linux-arm-kernel On 1.7.2016 07:37, Jean-Francois Moine wrote: > On Fri, 1 Jul 2016 02:50:57 +0200 > Ond?ej Jirman <megous@megous.com> wrote: > >>> Since this is really specific, I guess you could simply make the >>> clk_ops for the nkmp clocks public, and just re-implement set_rate >>> using that logic. >> >> I would argue that this may be necessary for other PLL clocks too, if >> you can get out of bounds output frequency, by changing the dividers too >> early or too late. So perhaps this code should be generalized for other >> PLL clocks too, instead. > > The documentation says that only the CPU and DDR PLLs can be dynamically > changed after boot. The question is what exactly is meant by after boot. :) Anyway, if the kernel has no business changing some other PLLs, if there's code for changing them, should it be dropped? regards, Ondrej -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160701/729f1527/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-01 6:34 ` Ondřej Jirman @ 2016-07-01 7:47 ` Jean-Francois Moine 0 siblings, 0 replies; 66+ messages in thread From: Jean-Francois Moine @ 2016-07-01 7:47 UTC (permalink / raw) To: linux-arm-kernel On Fri, 1 Jul 2016 08:34:21 +0200 Ond?ej Jirman <megous@megous.com> wrote: > > The documentation says that only the CPU and DDR PLLs can be dynamically > > changed after boot. > > The question is what exactly is meant by after boot. :) Anyway, if the > kernel has no business changing some other PLLs, if there's code for > changing them, should it be dropped? No, because all the other PLLs may not be initialized by the U-boot (audio, video, gpu...), and also, their rate may be changed safely by stopping them (gate). -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-01 0:50 ` Ondřej Jirman 2016-07-01 5:37 ` Jean-Francois Moine @ 2016-07-15 8:53 ` Maxime Ripard 2016-07-15 10:38 ` Ondřej Jirman 1 sibling, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-07-15 8:53 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ond?ej Jirman wrote: > >> /** > >> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the > >> + * register using an algorithm that tries to reserve the PLL lock > >> + */ > >> + > >> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) > >> +{ > >> + const struct clk_factors_config *config = factors->config; > >> + u32 reg; > >> + > >> + /* Fetch the register value */ > >> + reg = readl(factors->reg); > >> + > >> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { > >> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); > >> + > >> + writel(reg, factors->reg); > >> + __delay(2000); > >> + } > > > > So there was some doubts about the fact that P was being used, or at > > least that it was useful. > > p is necessary to reduce frequencies below 288 MHz according to the > datasheet. Yes, but you could reach those frequencies without P, too, and it's not part of any OPP provided by Allwinner. > >> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { > >> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); > >> + > >> + writel(reg, factors->reg); > >> + __delay(2000); > >> + } > >> + > >> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); > >> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); > >> + > >> + writel(reg, factors->reg); > >> + __delay(20); > >> + > >> + while (!(readl(factors->reg) & (1 << config->lock))); > > > > So, they are applying the dividers first, and then applying the > > multipliers, and then wait for the PLL to stabilize. > > Not exactly, first we are increasing dividers if the new dividers are > higher that that what's already set. This ensures that because > application of dividers is immediate by the design of the PLL, the > application of multipliers isn't. So the VCO would still run at the same > frequency for a while gradually rising to a new value for example, > while the dividers would be reduced immediately. Leading to crash. > > PLL > -------------------------- > PRE DIV(f0) -> VCO(f1) -> POST DIV(f2) > P K,N M > > Example: (we set all factors at once, reducing dividers and multipliers > at the same time at 0ms - this should lead to no change in the output > frequency, but...) > > -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz > 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 2GHz - boom > 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz > 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz > > The current code crashes exactly at boom, you don't get any more > instructions to execute. > > See. > > So this patch first increases dividers (only if necessary), changes > multipliers and waits for change to happen (takes around 2000 cycles), > and then decreases dividers (only if necessary). > > So we get: > > -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz > 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz - no boom, multiplier > reduced > 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz > 1.9ms: f0 = 24MHz, f1 = 1GHz, f2 = 0.5GHz - we got PLL sync > 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz - and here we reduce divider > at last Awesome explanation, thanks! So I guess it really all boils down to the fact that the CPU is clocked way outside of it's operating frequency while the PLL stabilizes, right? If so, then yes, trying to switch to the 24MHz oscillator before applying the factors, and then switching back when the PLL is stable would be a nice solution. I just checked, and all the SoCs we've had so far have that possibility, so if it works, for now, I'd like to stick to that. > >> + > >> + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) { > >> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); > >> + > >> + writel(reg, factors->reg); > >> + __delay(2000); > >> + } > >> + > >> + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) { > >> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); > >> + > >> + writel(reg, factors->reg); > >> + __delay(2000); > >> + } > > > > However, this is kind of weird, why would you need to re-apply the > > dividers? Nothing really changes. Have you tried without that part? > > See above, we either increase before PLL change, or reduce dividers > after the change. Nothing is re-applied. > > > Since this is really specific, I guess you could simply make the > > clk_ops for the nkmp clocks public, and just re-implement set_rate > > using that logic. > > I would argue that this may be necessary for other PLL clocks too, if > you can get out of bounds output frequency, by changing the dividers too > early or too late. So perhaps this code should be generalized for other > PLL clocks too, instead. We can scale down the problem a bit. The only PLL we modify are the CPU, audio and video ones. The CPU should definitely be addressed, but what would be the outcome of an out of bounds audio pll for example? Is it just taking more time to stabilize, or would it hurt the system? In the former case, then we can just wait. In the latter, we of course need to come up with a solution. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/d7b66f8b/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 8:53 ` Maxime Ripard @ 2016-07-15 10:38 ` Ondřej Jirman 2016-07-15 13:27 ` Jean-Francois Moine 2016-07-21 9:48 ` Maxime Ripard 0 siblings, 2 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-07-15 10:38 UTC (permalink / raw) To: linux-arm-kernel On 15.7.2016 10:53, Maxime Ripard wrote: > On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ond?ej Jirman wrote: >>>> /** >>>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the >>>> + * register using an algorithm that tries to reserve the PLL lock >>>> + */ >>>> + >>>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) >>>> +{ >>>> + const struct clk_factors_config *config = factors->config; >>>> + u32 reg; >>>> + >>>> + /* Fetch the register value */ >>>> + reg = readl(factors->reg); >>>> + >>>> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { >>>> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >>>> + >>>> + writel(reg, factors->reg); >>>> + __delay(2000); >>>> + } >>> >>> So there was some doubts about the fact that P was being used, or at >>> least that it was useful. >> >> p is necessary to reduce frequencies below 288 MHz according to the >> datasheet. > > Yes, but you could reach those frequencies without P, too, and it's > not part of any OPP provided by Allwinner. The arisc firmware for H3 contains table of factors for frequences from 0 to 2GHz and, P is used below 240MHz. M is never used, BTW. (other datasheets specify M as for testing use only, whatever that means - not H3, but it seems it's the same PLL block) ... snip ... { .n = 9, .k = 0, .m = 0, .p = 2 }, // 60 => 60 MHz { .n = 10, .k = 0, .m = 0, .p = 2 }, // 66 => 66 MHz { .n = 11, .k = 0, .m = 0, .p = 2 }, // 72 => 72 MHz { .n = 12, .k = 0, .m = 0, .p = 2 }, // 78 => 78 MHz { .n = 13, .k = 0, .m = 0, .p = 2 }, // 84 => 84 MHz { .n = 14, .k = 0, .m = 0, .p = 2 }, // 90 => 90 MHz { .n = 15, .k = 0, .m = 0, .p = 2 }, // 96 => 96 MHz { .n = 16, .k = 0, .m = 0, .p = 2 }, // 102 => 102 MHz { .n = 17, .k = 0, .m = 0, .p = 2 }, // 108 => 108 MHz { .n = 18, .k = 0, .m = 0, .p = 2 }, // 114 => 114 MHz { .n = 9, .k = 0, .m = 0, .p = 1 }, // 120 => 120 MHz { .n = 10, .k = 0, .m = 0, .p = 1 }, // 126 => 132 MHz { .n = 10, .k = 0, .m = 0, .p = 1 }, // 132 => 132 MHz { .n = 11, .k = 0, .m = 0, .p = 1 }, // 138 => 144 MHz { .n = 11, .k = 0, .m = 0, .p = 1 }, // 144 => 144 MHz { .n = 12, .k = 0, .m = 0, .p = 1 }, // 150 => 156 MHz { .n = 12, .k = 0, .m = 0, .p = 1 }, // 156 => 156 MHz { .n = 13, .k = 0, .m = 0, .p = 1 }, // 162 => 168 MHz { .n = 13, .k = 0, .m = 0, .p = 1 }, // 168 => 168 MHz { .n = 14, .k = 0, .m = 0, .p = 1 }, // 174 => 180 MHz { .n = 14, .k = 0, .m = 0, .p = 1 }, // 180 => 180 MHz { .n = 15, .k = 0, .m = 0, .p = 1 }, // 186 => 192 MHz { .n = 15, .k = 0, .m = 0, .p = 1 }, // 192 => 192 MHz { .n = 16, .k = 0, .m = 0, .p = 1 }, // 198 => 204 MHz { .n = 16, .k = 0, .m = 0, .p = 1 }, // 204 => 204 MHz { .n = 17, .k = 0, .m = 0, .p = 1 }, // 210 => 216 MHz { .n = 17, .k = 0, .m = 0, .p = 1 }, // 216 => 216 MHz { .n = 18, .k = 0, .m = 0, .p = 1 }, // 222 => 228 MHz { .n = 18, .k = 0, .m = 0, .p = 1 }, // 228 => 228 MHz { .n = 9, .k = 0, .m = 0, .p = 0 }, // 234 => 240 MHz { .n = 9, .k = 0, .m = 0, .p = 0 }, // 240 => 240 MHz { .n = 10, .k = 0, .m = 0, .p = 0 }, // 246 => 264 MHz { .n = 10, .k = 0, .m = 0, .p = 0 }, // 252 => 264 MHz { .n = 10, .k = 0, .m = 0, .p = 0 }, // 258 => 264 MHz { .n = 10, .k = 0, .m = 0, .p = 0 }, // 264 => 264 MHz { .n = 11, .k = 0, .m = 0, .p = 0 }, // 270 => 288 MHz { .n = 11, .k = 0, .m = 0, .p = 0 }, // 276 => 288 MHz ... snip ... >>>> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { >>>> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >>>> + >>>> + writel(reg, factors->reg); >>>> + __delay(2000); >>>> + } >>>> + >>>> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); >>>> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); >>>> + >>>> + writel(reg, factors->reg); >>>> + __delay(20); >>>> + >>>> + while (!(readl(factors->reg) & (1 << config->lock))); >>> >>> So, they are applying the dividers first, and then applying the >>> multipliers, and then wait for the PLL to stabilize. >> >> Not exactly, first we are increasing dividers if the new dividers are >> higher that that what's already set. This ensures that because >> application of dividers is immediate by the design of the PLL, the >> application of multipliers isn't. So the VCO would still run at the same >> frequency for a while gradually rising to a new value for example, >> while the dividers would be reduced immediately. Leading to crash. >> >> PLL >> -------------------------- >> PRE DIV(f0) -> VCO(f1) -> POST DIV(f2) >> P K,N M >> >> Example: (we set all factors at once, reducing dividers and multipliers >> at the same time at 0ms - this should lead to no change in the output >> frequency, but...) >> >> -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz >> 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 2GHz - boom >> 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz >> 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz >> >> The current code crashes exactly at boom, you don't get any more >> instructions to execute. >> >> See. >> >> So this patch first increases dividers (only if necessary), changes >> multipliers and waits for change to happen (takes around 2000 cycles), >> and then decreases dividers (only if necessary). >> >> So we get: >> >> -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz >> 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz - no boom, multiplier >> reduced >> 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz >> 1.9ms: f0 = 24MHz, f1 = 1GHz, f2 = 0.5GHz - we got PLL sync >> 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz - and here we reduce divider >> at last > > Awesome explanation, thanks! > > So I guess it really all boils down to the fact that the CPU is > clocked way outside of it's operating frequency while the PLL > stabilizes, right? It may be, depending on the factors before and after change. I haven't tested what factors the mainline kernel calculates for each frequency. The arisc never uses M, and only uses P at frequencies that would not allow for this behavior. I created a test program for arisc that runs a loop on the main CPU using msgbox to send pings to the arisc CPU, and the vary the PLL1 randomly from the arisc, and haven't been able to lockup the main CPU yet with either method. There's also AXI clock, that depends on PLL1. Arisc firmware uses the same method to change it while changing CPUX clock. If the clock would rise above certain frequency, AXI divider is increased before the PLL1 change. If it would fall below certain frequency it is decreased after the PLL1 change. Though, aw kernel has axi divider set to 3 for all PLL1 frequencies, so this has no effect in practice. It's all smoke and mirrors. :D > If so, then yes, trying to switch to the 24MHz oscillator before > applying the factors, and then switching back when the PLL is stable > would be a nice solution. > > I just checked, and all the SoCs we've had so far have that > possibility, so if it works, for now, I'd like to stick to that. It would need to be tested. U-boot does the change only once, while the kernel would be doing it all the time and between various frequencies and PLL settings. So the issues may show up with this solution too. >>>> + >>>> + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) { >>>> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >>>> + >>>> + writel(reg, factors->reg); >>>> + __delay(2000); >>>> + } >>>> + >>>> + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) { >>>> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >>>> + >>>> + writel(reg, factors->reg); >>>> + __delay(2000); >>>> + } >>> >>> However, this is kind of weird, why would you need to re-apply the >>> dividers? Nothing really changes. Have you tried without that part? >> >> See above, we either increase before PLL change, or reduce dividers >> after the change. Nothing is re-applied. >> >>> Since this is really specific, I guess you could simply make the >>> clk_ops for the nkmp clocks public, and just re-implement set_rate >>> using that logic. >> >> I would argue that this may be necessary for other PLL clocks too, if >> you can get out of bounds output frequency, by changing the dividers too >> early or too late. So perhaps this code should be generalized for other >> PLL clocks too, instead. > > We can scale down the problem a bit. The only PLL we modify are the > CPU, audio and video ones. > > The CPU should definitely be addressed, but what would be the outcome > of an out of bounds audio pll for example? Is it just taking more time > to stabilize, or would it hurt the system? I have no idea. Given the low frequencies, you'll probably just get some transient audio noise. > In the former case, then we can just wait. In the latter, we of course > need to come up with a solution. > > Maxime > > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/392e824b/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 10:38 ` Ondřej Jirman @ 2016-07-15 13:27 ` Jean-Francois Moine 2016-07-15 13:48 ` Ondřej Jirman 2016-07-21 9:51 ` Maxime Ripard 2016-07-21 9:48 ` Maxime Ripard 1 sibling, 2 replies; 66+ messages in thread From: Jean-Francois Moine @ 2016-07-15 13:27 UTC (permalink / raw) To: linux-arm-kernel On Fri, 15 Jul 2016 12:38:54 +0200 Ond?ej Jirman <megous@megous.com> wrote: > > If so, then yes, trying to switch to the 24MHz oscillator before > > applying the factors, and then switching back when the PLL is stable > > would be a nice solution. > > > > I just checked, and all the SoCs we've had so far have that > > possibility, so if it works, for now, I'd like to stick to that. > > It would need to be tested. U-boot does the change only once, while the > kernel would be doing it all the time and between various frequencies > and PLL settings. So the issues may show up with this solution too. I don't think this is a good idea: the CPU clock may be changed at any time with the CPUFreq governor. I don't see the system moving from 1008MHz to 24MHz and then to 1200MHz when some computation is needed! BTW, Ond?ej, in my BPi M2+, I tried to change the CPU clock with your code at kernel start time from 792MHz to 1008MHz, but the hardware (arisc?) set an other value, and the system speed was lower than before (the PLL-CPUx register is 0x90031521 on boot, I want to set it to xxxx1410 and I read 0x91031f33 - sorry, I did not have a look at the CPU SD pattern). Do you know why? -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 13:27 ` Jean-Francois Moine @ 2016-07-15 13:48 ` Ondřej Jirman 2016-07-15 14:22 ` [linux-sunxi] " Michal Suchanek 2016-07-21 9:51 ` Maxime Ripard 1 sibling, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-07-15 13:48 UTC (permalink / raw) To: linux-arm-kernel On 15.7.2016 15:27, Jean-Francois Moine wrote: > On Fri, 15 Jul 2016 12:38:54 +0200 > Ond?ej Jirman <megous@megous.com> wrote: > >>> If so, then yes, trying to switch to the 24MHz oscillator before >>> applying the factors, and then switching back when the PLL is stable >>> would be a nice solution. >>> >>> I just checked, and all the SoCs we've had so far have that >>> possibility, so if it works, for now, I'd like to stick to that. >> >> It would need to be tested. U-boot does the change only once, while the >> kernel would be doing it all the time and between various frequencies >> and PLL settings. So the issues may show up with this solution too. > > I don't think this is a good idea: the CPU clock may be changed at any > time with the CPUFreq governor. I don't see the system moving from > 1008MHz to 24MHz and then to 1200MHz when some computation is needed! PLL lock time is around 10-20us, I'd guess based on the number of loops in the PLL lock wait loop. So unless you'll be switching frequencies many times per second, this should be barely noticeable. But I'd like a different solution too. > BTW, Ond?ej, in my BPi M2+, I tried to change the CPU clock with your > code at kernel start time from 792MHz to 1008MHz, but the hardware > (arisc?) set an other value, and the system speed was lower than before > (the PLL-CPUx register is 0x90031521 on boot, I want to set it to > xxxx1410 and I read 0x91031f33 - sorry, I did not have a look at the > CPU SD pattern). Do you know why? No idea. Arisc shouldn't do anything, unless you load some firmware into it, and release its reset line. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/55a9a2db/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 13:48 ` Ondřej Jirman @ 2016-07-15 14:22 ` Michal Suchanek 2016-07-15 16:33 ` Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Michal Suchanek @ 2016-07-15 14:22 UTC (permalink / raw) To: linux-arm-kernel Hello, On 15 July 2016 at 15:48, Ond?ej Jirman <megous@megous.com> wrote: > > > On 15.7.2016 15:27, Jean-Francois Moine wrote: >> On Fri, 15 Jul 2016 12:38:54 +0200 >> Ond?ej Jirman <megous@megous.com> wrote: >> >>>> If so, then yes, trying to switch to the 24MHz oscillator before >>>> applying the factors, and then switching back when the PLL is stable >>>> would be a nice solution. >>>> >>>> I just checked, and all the SoCs we've had so far have that >>>> possibility, so if it works, for now, I'd like to stick to that. >>> >>> It would need to be tested. U-boot does the change only once, while the >>> kernel would be doing it all the time and between various frequencies >>> and PLL settings. So the issues may show up with this solution too. >> >> I don't think this is a good idea: the CPU clock may be changed at any >> time with the CPUFreq governor. I don't see the system moving from >> 1008MHz to 24MHz and then to 1200MHz when some computation is needed! > > PLL lock time is around 10-20us, I'd guess based on the number of loops > in the PLL lock wait loop. So unless you'll be switching frequencies > many times per second, this should be barely noticeable. > > But I'd like a different solution too. Do you have a patch to test this? For me changing CPU frequency on Orange Pi One always locks up the system. I keep running it on the u-boot setup 1.08GHz at 1.1V Thanks Michal ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 14:22 ` [linux-sunxi] " Michal Suchanek @ 2016-07-15 16:33 ` Ondřej Jirman 0 siblings, 0 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-07-15 16:33 UTC (permalink / raw) To: linux-arm-kernel On 15.7.2016 16:22, Michal Suchanek wrote: > Hello, > > On 15 July 2016 at 15:48, Ond?ej Jirman <megous@megous.com> wrote: >> >> >> On 15.7.2016 15:27, Jean-Francois Moine wrote: >>> On Fri, 15 Jul 2016 12:38:54 +0200 >>> Ond?ej Jirman <megous@megous.com> wrote: >>> >>>>> If so, then yes, trying to switch to the 24MHz oscillator before >>>>> applying the factors, and then switching back when the PLL is stable >>>>> would be a nice solution. >>>>> >>>>> I just checked, and all the SoCs we've had so far have that >>>>> possibility, so if it works, for now, I'd like to stick to that. >>>> >>>> It would need to be tested. U-boot does the change only once, while the >>>> kernel would be doing it all the time and between various frequencies >>>> and PLL settings. So the issues may show up with this solution too. >>> >>> I don't think this is a good idea: the CPU clock may be changed at any >>> time with the CPUFreq governor. I don't see the system moving from >>> 1008MHz to 24MHz and then to 1200MHz when some computation is needed! >> >> PLL lock time is around 10-20us, I'd guess based on the number of loops >> in the PLL lock wait loop. So unless you'll be switching frequencies >> many times per second, this should be barely noticeable. >> >> But I'd like a different solution too. > > Do you have a patch to test this? > > For me changing CPU frequency on Orange Pi One always locks up the > system. I keep running it on the u-boot setup 1.08GHz at 1.1V Not anymore. But it's quite simple to hack it into the drivers/clk/sunxi/clk-factors.c:clk_factors_set_rate() You can look at u-boot arch/arm/mach-sunxi/clock_sun6i.c:clock_set_pll1 for how to change the CPU clock source. > Thanks > > Michal > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/4598f219/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 13:27 ` Jean-Francois Moine 2016-07-15 13:48 ` Ondřej Jirman @ 2016-07-21 9:51 ` Maxime Ripard 1 sibling, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-07-21 9:51 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 15, 2016 at 03:27:56PM +0200, Jean-Francois Moine wrote: > On Fri, 15 Jul 2016 12:38:54 +0200 > Ond?ej Jirman <megous@megous.com> wrote: > > > > If so, then yes, trying to switch to the 24MHz oscillator before > > > applying the factors, and then switching back when the PLL is stable > > > would be a nice solution. > > > > > > I just checked, and all the SoCs we've had so far have that > > > possibility, so if it works, for now, I'd like to stick to that. > > > > It would need to be tested. U-boot does the change only once, while the > > kernel would be doing it all the time and between various frequencies > > and PLL settings. So the issues may show up with this solution too. > > I don't think this is a good idea: the CPU clock may be changed at any > time with the CPUFreq governor. I don't see the system moving from > 1008MHz to 24MHz and then to 1200MHz when some computation is needed! It wouldn't happen that often. The sampling rate for the governor is 1000 times the latency, so, at most, 0.1% of the time would be spent at 24MHz. And if you're really concerned about performances, you won't enable cpufreq anyway. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160721/c73e8efe/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-15 10:38 ` Ondřej Jirman 2016-07-15 13:27 ` Jean-Francois Moine @ 2016-07-21 9:48 ` Maxime Ripard 2016-07-21 9:52 ` Ondřej Jirman 1 sibling, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-07-21 9:48 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 15, 2016 at 12:38:54PM +0200, Ond?ej Jirman wrote: > On 15.7.2016 10:53, Maxime Ripard wrote: > > On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ond?ej Jirman wrote: > >>>> /** > >>>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the > >>>> + * register using an algorithm that tries to reserve the PLL lock > >>>> + */ > >>>> + > >>>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) > >>>> +{ > >>>> + const struct clk_factors_config *config = factors->config; > >>>> + u32 reg; > >>>> + > >>>> + /* Fetch the register value */ > >>>> + reg = readl(factors->reg); > >>>> + > >>>> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { > >>>> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); > >>>> + > >>>> + writel(reg, factors->reg); > >>>> + __delay(2000); > >>>> + } > >>> > >>> So there was some doubts about the fact that P was being used, or at > >>> least that it was useful. > >> > >> p is necessary to reduce frequencies below 288 MHz according to the > >> datasheet. > > > > Yes, but you could reach those frequencies without P, too, and it's > > not part of any OPP provided by Allwinner. > > The arisc firmware for H3 contains table of factors for frequences from > 0 to 2GHz and, P is used below 240MHz. M is never used, BTW. (other > datasheets specify M as for testing use only, whatever that means - not > H3, but it seems it's the same PLL block) Interesting. Which SoCs in particular? > >>>> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { > >>>> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); > >>>> + > >>>> + writel(reg, factors->reg); > >>>> + __delay(2000); > >>>> + } > >>>> + > >>>> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); > >>>> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); > >>>> + > >>>> + writel(reg, factors->reg); > >>>> + __delay(20); > >>>> + > >>>> + while (!(readl(factors->reg) & (1 << config->lock))); > >>> > >>> So, they are applying the dividers first, and then applying the > >>> multipliers, and then wait for the PLL to stabilize. > >> > >> Not exactly, first we are increasing dividers if the new dividers are > >> higher that that what's already set. This ensures that because > >> application of dividers is immediate by the design of the PLL, the > >> application of multipliers isn't. So the VCO would still run at the same > >> frequency for a while gradually rising to a new value for example, > >> while the dividers would be reduced immediately. Leading to crash. > >> > >> PLL > >> -------------------------- > >> PRE DIV(f0) -> VCO(f1) -> POST DIV(f2) > >> P K,N M > >> > >> Example: (we set all factors at once, reducing dividers and multipliers > >> at the same time at 0ms - this should lead to no change in the output > >> frequency, but...) > >> > >> -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz > >> 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 2GHz - boom > >> 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz > >> 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz > >> > >> The current code crashes exactly at boom, you don't get any more > >> instructions to execute. > >> > >> See. > >> > >> So this patch first increases dividers (only if necessary), changes > >> multipliers and waits for change to happen (takes around 2000 cycles), > >> and then decreases dividers (only if necessary). > >> > >> So we get: > >> > >> -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz > >> 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz - no boom, multiplier > >> reduced > >> 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz > >> 1.9ms: f0 = 24MHz, f1 = 1GHz, f2 = 0.5GHz - we got PLL sync > >> 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz - and here we reduce divider > >> at last > > > > Awesome explanation, thanks! > > > > So I guess it really all boils down to the fact that the CPU is > > clocked way outside of it's operating frequency while the PLL > > stabilizes, right? > > It may be, depending on the factors before and after change. I haven't > tested what factors the mainline kernel calculates for each frequency. > The arisc never uses M, and only uses P at frequencies that would not > allow for this behavior. > > I created a test program for arisc that runs a loop on the main CPU > using msgbox to send pings to the arisc CPU, and the vary the PLL1 > randomly from the arisc, and haven't been able to lockup the main CPU > yet with either method. > > There's also AXI clock, that depends on PLL1. Arisc firmware uses the > same method to change it while changing CPUX clock. If the clock would > rise above certain frequency, AXI divider is increased before the PLL1 > change. If it would fall below certain frequency it is decreased after > the PLL1 change. If we ever need to change that, we can always rely on a clock notifier to adjust the divider before the change happen (and support all the scenarios, like the clock change has been aborted). > > If so, then yes, trying to switch to the 24MHz oscillator before > > applying the factors, and then switching back when the PLL is stable > > would be a nice solution. > > > > I just checked, and all the SoCs we've had so far have that > > possibility, so if it works, for now, I'd like to stick to that. > > It would need to be tested. U-boot does the change only once, while the > kernel would be doing it all the time and between various frequencies > and PLL settings. So the issues may show up with this solution too. That would have the benefit of being quite easy to document, not be a huge amount of code and it would work on all the CPUs PLLs we have so far, so still, a pretty big win. If it doesn't, of course, we don't really have the choice. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160721/de014597/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-21 9:48 ` Maxime Ripard @ 2016-07-21 9:52 ` Ondřej Jirman 2016-07-26 6:32 ` Maxime Ripard 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-07-21 9:52 UTC (permalink / raw) To: linux-arm-kernel On 21.7.2016 11:48, Maxime Ripard wrote: > On Fri, Jul 15, 2016 at 12:38:54PM +0200, Ond?ej Jirman wrote: >> On 15.7.2016 10:53, Maxime Ripard wrote: >>> On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ond?ej Jirman wrote: >>>>>> /** >>>>>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the >>>>>> + * register using an algorithm that tries to reserve the PLL lock >>>>>> + */ >>>>>> + >>>>>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) >>>>>> +{ >>>>>> + const struct clk_factors_config *config = factors->config; >>>>>> + u32 reg; >>>>>> + >>>>>> + /* Fetch the register value */ >>>>>> + reg = readl(factors->reg); >>>>>> + >>>>>> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { >>>>>> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >>>>>> + >>>>>> + writel(reg, factors->reg); >>>>>> + __delay(2000); >>>>>> + } >>>>> >>>>> So there was some doubts about the fact that P was being used, or at >>>>> least that it was useful. >>>> >>>> p is necessary to reduce frequencies below 288 MHz according to the >>>> datasheet. >>> >>> Yes, but you could reach those frequencies without P, too, and it's >>> not part of any OPP provided by Allwinner. >> >> The arisc firmware for H3 contains table of factors for frequences from >> 0 to 2GHz and, P is used below 240MHz. M is never used, BTW. (other >> datasheets specify M as for testing use only, whatever that means - not >> H3, but it seems it's the same PLL block) > > Interesting. Which SoCs in particular? > >>>>>> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { >>>>>> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >>>>>> + >>>>>> + writel(reg, factors->reg); >>>>>> + __delay(2000); >>>>>> + } >>>>>> + >>>>>> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); >>>>>> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); >>>>>> + >>>>>> + writel(reg, factors->reg); >>>>>> + __delay(20); >>>>>> + >>>>>> + while (!(readl(factors->reg) & (1 << config->lock))); >>>>> >>>>> So, they are applying the dividers first, and then applying the >>>>> multipliers, and then wait for the PLL to stabilize. >>>> >>>> Not exactly, first we are increasing dividers if the new dividers are >>>> higher that that what's already set. This ensures that because >>>> application of dividers is immediate by the design of the PLL, the >>>> application of multipliers isn't. So the VCO would still run at the same >>>> frequency for a while gradually rising to a new value for example, >>>> while the dividers would be reduced immediately. Leading to crash. >>>> >>>> PLL >>>> -------------------------- >>>> PRE DIV(f0) -> VCO(f1) -> POST DIV(f2) >>>> P K,N M >>>> >>>> Example: (we set all factors at once, reducing dividers and multipliers >>>> at the same time at 0ms - this should lead to no change in the output >>>> frequency, but...) >>>> >>>> -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz >>>> 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 2GHz - boom >>>> 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz >>>> 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz >>>> >>>> The current code crashes exactly at boom, you don't get any more >>>> instructions to execute. >>>> >>>> See. >>>> >>>> So this patch first increases dividers (only if necessary), changes >>>> multipliers and waits for change to happen (takes around 2000 cycles), >>>> and then decreases dividers (only if necessary). >>>> >>>> So we get: >>>> >>>> -1ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz >>>> 0ms: f0 = 24MHz, f1 = 2GHz, f2 = 1GHz - no boom, multiplier >>>> reduced >>>> 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz >>>> 1.9ms: f0 = 24MHz, f1 = 1GHz, f2 = 0.5GHz - we got PLL sync >>>> 2ms: f0 = 24MHz, f1 = 1GHz, f2 = 1GHz - and here we reduce divider >>>> at last >>> >>> Awesome explanation, thanks! >>> >>> So I guess it really all boils down to the fact that the CPU is >>> clocked way outside of it's operating frequency while the PLL >>> stabilizes, right? >> >> It may be, depending on the factors before and after change. I haven't >> tested what factors the mainline kernel calculates for each frequency. >> The arisc never uses M, and only uses P at frequencies that would not >> allow for this behavior. >> >> I created a test program for arisc that runs a loop on the main CPU >> using msgbox to send pings to the arisc CPU, and the vary the PLL1 >> randomly from the arisc, and haven't been able to lockup the main CPU >> yet with either method. >> >> There's also AXI clock, that depends on PLL1. Arisc firmware uses the >> same method to change it while changing CPUX clock. If the clock would >> rise above certain frequency, AXI divider is increased before the PLL1 >> change. If it would fall below certain frequency it is decreased after >> the PLL1 change. > > If we ever need to change that, we can always rely on a clock notifier > to adjust the divider before the change happen (and support all the > scenarios, like the clock change has been aborted). > >>> If so, then yes, trying to switch to the 24MHz oscillator before >>> applying the factors, and then switching back when the PLL is stable >>> would be a nice solution. >>> >>> I just checked, and all the SoCs we've had so far have that >>> possibility, so if it works, for now, I'd like to stick to that. >> >> It would need to be tested. U-boot does the change only once, while the >> kernel would be doing it all the time and between various frequencies >> and PLL settings. So the issues may show up with this solution too. > > That would have the benefit of being quite easy to document, not be a > huge amount of code and it would work on all the CPUs PLLs we have so > far, so still, a pretty big win. If it doesn't, of course, we don't > really have the choice. It's probably more code though. It has to access different register from the one that is already defined in dts, which would add a lot of code and require dts changes. The original patch I sent is simpler than that. regards, Ondrej > Maxime > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160721/7bc0138f/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-21 9:52 ` Ondřej Jirman @ 2016-07-26 6:32 ` Maxime Ripard 2016-07-28 11:27 ` Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-07-26 6:32 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: > >>> If so, then yes, trying to switch to the 24MHz oscillator before > >>> applying the factors, and then switching back when the PLL is stable > >>> would be a nice solution. > >>> > >>> I just checked, and all the SoCs we've had so far have that > >>> possibility, so if it works, for now, I'd like to stick to that. > >> > >> It would need to be tested. U-boot does the change only once, while the > >> kernel would be doing it all the time and between various frequencies > >> and PLL settings. So the issues may show up with this solution too. > > > > That would have the benefit of being quite easy to document, not be a > > huge amount of code and it would work on all the CPUs PLLs we have so > > far, so still, a pretty big win. If it doesn't, of course, we don't > > really have the choice. > > It's probably more code though. It has to access different register from > the one that is already defined in dts, which would add a lot of code > and require dts changes. The original patch I sent is simpler than that. Why? You can use container_of to retrieve the parent structure of the clock notifier, and then you get a ccu_common structure pointer, with the CCU base address, the clock register, its lock, etc. Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. I don't really get why anything should be changed in the DT, or why it would add a lot of code. Or maybe we're not talking about the same thing? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160726/fe36e315/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-26 6:32 ` Maxime Ripard @ 2016-07-28 11:27 ` Ondřej Jirman 2016-07-28 11:38 ` [linux-sunxi] " Chen-Yu Tsai 2016-07-28 21:00 ` Maxime Ripard 0 siblings, 2 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-07-28 11:27 UTC (permalink / raw) To: linux-arm-kernel Hi Maxime, I don't have your sunxi-ng clock patches in my mailbox, so I'm replying to this. On 26.7.2016 08:32, Maxime Ripard wrote: > On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: >>>>> If so, then yes, trying to switch to the 24MHz oscillator before >>>>> applying the factors, and then switching back when the PLL is stable >>>>> would be a nice solution. >>>>> >>>>> I just checked, and all the SoCs we've had so far have that >>>>> possibility, so if it works, for now, I'd like to stick to that. >>>> >>>> It would need to be tested. U-boot does the change only once, while the >>>> kernel would be doing it all the time and between various frequencies >>>> and PLL settings. So the issues may show up with this solution too. >>> >>> That would have the benefit of being quite easy to document, not be a >>> huge amount of code and it would work on all the CPUs PLLs we have so >>> far, so still, a pretty big win. If it doesn't, of course, we don't >>> really have the choice. >> >> It's probably more code though. It has to access different register from >> the one that is already defined in dts, which would add a lot of code >> and require dts changes. The original patch I sent is simpler than that. > > Why? > > You can use container_of to retrieve the parent structure of the clock > notifier, and then you get a ccu_common structure pointer, with the > CCU base address, the clock register, its lock, etc. > > Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. > > I don't really get why anything should be changed in the DT, or why it > would add a lot of code. Or maybe we're not talking about the same > thing? I've looked at the new CCU code, particularly ccu_nkmp.c, and found that it very liberally uses divider parameters, even in situations that are out of spec compared to the current code in the kernel. In the current code and especially in the original vendor code, divider parameters are used as last resort only. Presumably because, of the inherent trouble in changing them, as I described to you in other email. The new ccu code uses dividers often and even at very high frequencies, which goes against the spec. In the vendor code M is never anything else but 0, and P is used only for frequencies below 288MHz, which matches the H3 datasheet, which says: "The P factor only use in the condition that PLL output less than 288 MHz." Also other datasheets of similar socs from Allwinner state that M should not be used in production code. So it may be that they either forgot to state it in the H3 datasheet, or it can be used. In any case, they never use M in their code, so it may be wise to keep to that. When I boot with the new CCU code I get this: PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0 PLL_CPUX : freq = 1008MHz Mathematically it works, but it is against the spec. Also, this: analyzing CPU 0: driver: cpufreq-dt CPUs which run at the same hardware frequency: 0 1 2 3 CPUs which need to have their frequency coordinated by software: 0 1 2 3 maximum transition latency: 1.74 ms hardware limits: 120 MHz - 1.37 GHz available frequency steps: 120 MHz, 240 MHz, 480 MHz, 648 MHz, 816 MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22 GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz available cpufreq governors: conservative ondemand userspace powersave performance current policy: frequency should be within 240 MHz and 240 MHz. The governor "performance" may decide which speed to use within this range. current CPU frequency: 24.0 MHz (asserted by call to hardware) Somehow, the new CCU code sets the CPUX to 24MHz no matter what. I'm using your pen/clk-rework branch without any other patches that were later sent to the mailing list. regards, Ondrej > > Maxime > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160728/795daaa5/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-28 11:27 ` Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman @ 2016-07-28 11:38 ` Chen-Yu Tsai 2016-07-28 21:00 ` Maxime Ripard 1 sibling, 0 replies; 66+ messages in thread From: Chen-Yu Tsai @ 2016-07-28 11:38 UTC (permalink / raw) To: linux-arm-kernel Hi, On Thu, Jul 28, 2016 at 7:27 PM, Ond?ej Jirman <megous@megous.com> wrote: > Hi Maxime, > > I don't have your sunxi-ng clock patches in my mailbox, so I'm replying > to this. > > On 26.7.2016 08:32, Maxime Ripard wrote: >> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: >>>>>> If so, then yes, trying to switch to the 24MHz oscillator before >>>>>> applying the factors, and then switching back when the PLL is stable >>>>>> would be a nice solution. >>>>>> >>>>>> I just checked, and all the SoCs we've had so far have that >>>>>> possibility, so if it works, for now, I'd like to stick to that. >>>>> >>>>> It would need to be tested. U-boot does the change only once, while the >>>>> kernel would be doing it all the time and between various frequencies >>>>> and PLL settings. So the issues may show up with this solution too. >>>> >>>> That would have the benefit of being quite easy to document, not be a >>>> huge amount of code and it would work on all the CPUs PLLs we have so >>>> far, so still, a pretty big win. If it doesn't, of course, we don't >>>> really have the choice. >>> >>> It's probably more code though. It has to access different register from >>> the one that is already defined in dts, which would add a lot of code >>> and require dts changes. The original patch I sent is simpler than that. >> >> Why? >> >> You can use container_of to retrieve the parent structure of the clock >> notifier, and then you get a ccu_common structure pointer, with the >> CCU base address, the clock register, its lock, etc. >> >> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. >> >> I don't really get why anything should be changed in the DT, or why it >> would add a lot of code. Or maybe we're not talking about the same >> thing? > > I've looked at the new CCU code, particularly ccu_nkmp.c, and found that > it very liberally uses divider parameters, even in situations that are > out of spec compared to the current code in the kernel. > > In the current code and especially in the original vendor code, divider > parameters are used as last resort only. Presumably because, of the > inherent trouble in changing them, as I described to you in other email. > > The new ccu code uses dividers often and even at very high frequencies, > which goes against the spec. > > In the vendor code M is never anything else but 0, and P is used only > for frequencies below 288MHz, which matches the H3 datasheet, which says: > > "The P factor only use in the condition that PLL output less than 288 > MHz." > > Also other datasheets of similar socs from Allwinner state that M should > not be used in production code. So it may be that they either forgot to > state it in the H3 datasheet, or it can be used. In any case, they never > use M in their code, so it may be wise to keep to that. > > When I boot with the new CCU code I get this: > > PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0 > PLL_CPUX : freq = 1008MHz > > Mathematically it works, but it is against the spec. > > Also, this: > > analyzing CPU 0: > driver: cpufreq-dt > CPUs which run at the same hardware frequency: 0 1 2 3 > CPUs which need to have their frequency coordinated by software: 0 1 2 3 > maximum transition latency: 1.74 ms > hardware limits: 120 MHz - 1.37 GHz > available frequency steps: 120 MHz, 240 MHz, 480 MHz, 648 MHz, 816 > MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22 > GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz > available cpufreq governors: conservative ondemand userspace powersave > performance > current policy: frequency should be within 240 MHz and 240 MHz. > The governor "performance" may decide which speed to use > within this range. > current CPU frequency: 24.0 MHz (asserted by call to hardware) > > > Somehow, the new CCU code sets the CPUX to 24MHz no matter what. > > I'm using your pen/clk-rework branch without any other patches that were > later sent to the mailing list. The H3 CPUX clock does not have the CLK_SET_RATE_PARENT flag set, which means cpufreq's attempts to set the clk rate will, in some cases, be ignored as the change is not propagated up to the parent PLL. In the worst case it will select the 24 MHz oscillator. The fix is to add CLK_SET_RATE_PARENT. And then you'll have to deal with the PLL potentially going too high, as you stated in your other mail, and can be mitigated with my clk notifier patch. ChenYu > regards, > Ondrej > >> >> Maxime >> > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 66+ messages in thread
* Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-28 11:27 ` Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman 2016-07-28 11:38 ` [linux-sunxi] " Chen-Yu Tsai @ 2016-07-28 21:00 ` Maxime Ripard 2016-07-28 22:01 ` Ondřej Jirman 1 sibling, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-07-28 21:00 UTC (permalink / raw) To: linux-arm-kernel Hi Ondrej, On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ond?ej Jirman wrote: > Hi Maxime, > > I don't have your sunxi-ng clock patches in my mailbox, so I'm replying > to this. You can find it in the clock maintainers tree: https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng > On 26.7.2016 08:32, Maxime Ripard wrote: > > On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: > >>>>> If so, then yes, trying to switch to the 24MHz oscillator before > >>>>> applying the factors, and then switching back when the PLL is stable > >>>>> would be a nice solution. > >>>>> > >>>>> I just checked, and all the SoCs we've had so far have that > >>>>> possibility, so if it works, for now, I'd like to stick to that. > >>>> > >>>> It would need to be tested. U-boot does the change only once, while the > >>>> kernel would be doing it all the time and between various frequencies > >>>> and PLL settings. So the issues may show up with this solution too. > >>> > >>> That would have the benefit of being quite easy to document, not be a > >>> huge amount of code and it would work on all the CPUs PLLs we have so > >>> far, so still, a pretty big win. If it doesn't, of course, we don't > >>> really have the choice. > >> > >> It's probably more code though. It has to access different register from > >> the one that is already defined in dts, which would add a lot of code > >> and require dts changes. The original patch I sent is simpler than that. > > > > Why? > > > > You can use container_of to retrieve the parent structure of the clock > > notifier, and then you get a ccu_common structure pointer, with the > > CCU base address, the clock register, its lock, etc. > > > > Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. > > > > I don't really get why anything should be changed in the DT, or why it > > would add a lot of code. Or maybe we're not talking about the same > > thing? > > I've looked at the new CCU code, particularly ccu_nkmp.c, and found that > it very liberally uses divider parameters, even in situations that are > out of spec compared to the current code in the kernel. > > In the current code and especially in the original vendor code, divider > parameters are used as last resort only. Presumably because, of the > inherent trouble in changing them, as I described to you in other email. > > The new ccu code uses dividers often and even at very high frequencies, > which goes against the spec. > > In the vendor code M is never anything else but 0, and P is used only > for frequencies below 288MHz, which matches the H3 datasheet, which says: In the vendor code, P is never used either. All the boards we had so far don't go that low, so we cannot make any of these assumptions, especially since the vendor code has had the bad habit of doing something wrong and / or useless in the past. However, this implementation is a new thing, and it was using the H3 precisely because of its early stage of support to use it as a testbed for the more established. If you feel like we should use a different formula to favour the multipliers over the dividers (or want to change the class of the CPU PLL to an NKM or something else, this is totally doable. > "The P factor only use in the condition that PLL output less than 288 > MHz." And the datasheet also had some issues, either misleading or wrong comments in the past. Don't get me wrong, I'm not saying that this is wrong, just that we should not follow it religiously, and that we should trust more the experiments than the datasheet. > Also other datasheets of similar socs from Allwinner state that M should > not be used in production code. Which ones specifically? > So it may be that they either forgot to state it in the H3 > datasheet, or it can be used. In any case, they never use M in their > code, so it may be wise to keep to that. > > When I boot with the new CCU code I get this: > > PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0 > PLL_CPUX : freq = 1008MHz > > Mathematically it works, but it is against the spec. > > Also, this: > > analyzing CPU 0: > driver: cpufreq-dt > CPUs which run at the same hardware frequency: 0 1 2 3 > CPUs which need to have their frequency coordinated by software: 0 1 2 3 > maximum transition latency: 1.74 ms > hardware limits: 120 MHz - 1.37 GHz > available frequency steps: 120 MHz, 240 MHz, 480 MHz, 648 MHz, 816 > MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22 > GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz > available cpufreq governors: conservative ondemand userspace powersave > performance > current policy: frequency should be within 240 MHz and 240 MHz. > The governor "performance" may decide which speed to use > within this range. > current CPU frequency: 24.0 MHz (asserted by call to hardware) > > > Somehow, the new CCU code sets the CPUX to 24MHz no matter what. > > I'm using your pen/clk-rework branch without any other patches that were > later sent to the mailing list. It's probably because of CLK_SET_RATE_PARENT on the CPU clock missing, as Chen-Yu suggested. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160728/7cef4bb7/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-28 21:00 ` Maxime Ripard @ 2016-07-28 22:01 ` Ondřej Jirman 2016-07-31 10:31 ` Maxime Ripard 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-07-28 22:01 UTC (permalink / raw) To: linux-arm-kernel On 28.7.2016 23:00, Maxime Ripard wrote: > Hi Ondrej, > > On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ond?ej Jirman wrote: >> Hi Maxime, >> >> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying >> to this. > > You can find it in the clock maintainers tree: > https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng > >> On 26.7.2016 08:32, Maxime Ripard wrote: >>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: >>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before >>>>>>> applying the factors, and then switching back when the PLL is stable >>>>>>> would be a nice solution. >>>>>>> >>>>>>> I just checked, and all the SoCs we've had so far have that >>>>>>> possibility, so if it works, for now, I'd like to stick to that. >>>>>> >>>>>> It would need to be tested. U-boot does the change only once, while the >>>>>> kernel would be doing it all the time and between various frequencies >>>>>> and PLL settings. So the issues may show up with this solution too. >>>>> >>>>> That would have the benefit of being quite easy to document, not be a >>>>> huge amount of code and it would work on all the CPUs PLLs we have so >>>>> far, so still, a pretty big win. If it doesn't, of course, we don't >>>>> really have the choice. >>>> >>>> It's probably more code though. It has to access different register from >>>> the one that is already defined in dts, which would add a lot of code >>>> and require dts changes. The original patch I sent is simpler than that. >>> >>> Why? >>> >>> You can use container_of to retrieve the parent structure of the clock >>> notifier, and then you get a ccu_common structure pointer, with the >>> CCU base address, the clock register, its lock, etc. >>> >>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. >>> >>> I don't really get why anything should be changed in the DT, or why it >>> would add a lot of code. Or maybe we're not talking about the same >>> thing? >> >> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that >> it very liberally uses divider parameters, even in situations that are >> out of spec compared to the current code in the kernel. >> >> In the current code and especially in the original vendor code, divider >> parameters are used as last resort only. Presumably because, of the >> inherent trouble in changing them, as I described to you in other email. >> >> The new ccu code uses dividers often and even at very high frequencies, >> which goes against the spec. >> >> In the vendor code M is never anything else but 0, and P is used only >> for frequencies below 288MHz, which matches the H3 datasheet, which says: > > In the vendor code, P is never used either. All the boards we had so > far don't go that low, so we cannot make any of these assumptions, > especially since the vendor code has had the bad habit of doing > something wrong and / or useless in the past. P is used in the arisc firmware according to the spec for the lower frequencies. > However, this implementation is a new thing, and it was using the H3 > precisely because of its early stage of support to use it as a testbed > for the more established. > > If you feel like we should use a different formula to favour the > multipliers over the dividers (or want to change the class of the CPU > PLL to an NKM or something else, this is totally doable. I think the original formula that's currently in the mainline kernel is better and avoids fiddling with dividers too much. >> "The P factor only use in the condition that PLL output less than 288 >> MHz." > > And the datasheet also had some issues, either misleading or wrong > comments in the past. Don't get me wrong, I'm not saying that this is > wrong, just that we should not follow it religiously, and that we > should trust more the experiments than the datasheet. I can believe that. :) Regardless, I think the reasons given for avoiding dividers are quite reasonable. It's based on how PLL block works, not what manual says. >> Also other datasheets of similar socs from Allwinner state that M should >> not be used in production code. > > Which ones specifically? A83T for example. You can search for "only for test" phrase. https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf Those PLLs are a bit different though. regards, Ondrej >> So it may be that they either forgot to state it in the H3 >> datasheet, or it can be used. In any case, they never use M in their >> code, so it may be wise to keep to that. >> >> When I boot with the new CCU code I get this: >> >> PLL_CPUX = 0x00001B21 : M = 2, K = 3, N = 28, P = 1, EN = 0 >> PLL_CPUX : freq = 1008MHz >> >> Mathematically it works, but it is against the spec. >> >> Also, this: >> >> analyzing CPU 0: >> driver: cpufreq-dt >> CPUs which run at the same hardware frequency: 0 1 2 3 >> CPUs which need to have their frequency coordinated by software: 0 1 2 3 >> maximum transition latency: 1.74 ms >> hardware limits: 120 MHz - 1.37 GHz >> available frequency steps: 120 MHz, 240 MHz, 480 MHz, 648 MHz, 816 >> MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22 >> GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz >> available cpufreq governors: conservative ondemand userspace powersave >> performance >> current policy: frequency should be within 240 MHz and 240 MHz. >> The governor "performance" may decide which speed to use >> within this range. >> current CPU frequency: 24.0 MHz (asserted by call to hardware) >> >> >> Somehow, the new CCU code sets the CPUX to 24MHz no matter what. >> >> I'm using your pen/clk-rework branch without any other patches that were >> later sent to the mailing list. > > It's probably because of CLK_SET_RATE_PARENT on the CPU clock missing, > as Chen-Yu suggested. > > Maxime > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160729/6576f8ce/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-28 22:01 ` Ondřej Jirman @ 2016-07-31 10:31 ` Maxime Ripard 2016-07-31 22:01 ` Ondřej Jirman 0 siblings, 1 reply; 66+ messages in thread From: Maxime Ripard @ 2016-07-31 10:31 UTC (permalink / raw) To: linux-arm-kernel Hi, On Fri, Jul 29, 2016 at 12:01:09AM +0200, Ond?ej Jirman wrote: > On 28.7.2016 23:00, Maxime Ripard wrote: > > Hi Ondrej, > > > > On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ond?ej Jirman wrote: > >> Hi Maxime, > >> > >> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying > >> to this. > > > > You can find it in the clock maintainers tree: > > https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng > > > >> On 26.7.2016 08:32, Maxime Ripard wrote: > >>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: > >>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before > >>>>>>> applying the factors, and then switching back when the PLL is stable > >>>>>>> would be a nice solution. > >>>>>>> > >>>>>>> I just checked, and all the SoCs we've had so far have that > >>>>>>> possibility, so if it works, for now, I'd like to stick to that. > >>>>>> > >>>>>> It would need to be tested. U-boot does the change only once, while the > >>>>>> kernel would be doing it all the time and between various frequencies > >>>>>> and PLL settings. So the issues may show up with this solution too. > >>>>> > >>>>> That would have the benefit of being quite easy to document, not be a > >>>>> huge amount of code and it would work on all the CPUs PLLs we have so > >>>>> far, so still, a pretty big win. If it doesn't, of course, we don't > >>>>> really have the choice. > >>>> > >>>> It's probably more code though. It has to access different register from > >>>> the one that is already defined in dts, which would add a lot of code > >>>> and require dts changes. The original patch I sent is simpler than that. > >>> > >>> Why? > >>> > >>> You can use container_of to retrieve the parent structure of the clock > >>> notifier, and then you get a ccu_common structure pointer, with the > >>> CCU base address, the clock register, its lock, etc. > >>> > >>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. > >>> > >>> I don't really get why anything should be changed in the DT, or why it > >>> would add a lot of code. Or maybe we're not talking about the same > >>> thing? > >> > >> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that > >> it very liberally uses divider parameters, even in situations that are > >> out of spec compared to the current code in the kernel. > >> > >> In the current code and especially in the original vendor code, divider > >> parameters are used as last resort only. Presumably because, of the > >> inherent trouble in changing them, as I described to you in other email. > >> > >> The new ccu code uses dividers often and even at very high frequencies, > >> which goes against the spec. > >> > >> In the vendor code M is never anything else but 0, and P is used only > >> for frequencies below 288MHz, which matches the H3 datasheet, which says: > > > > In the vendor code, P is never used either. All the boards we had so > > far don't go that low, so we cannot make any of these assumptions, > > especially since the vendor code has had the bad habit of doing > > something wrong and / or useless in the past. > > P is used in the arisc firmware according to the spec for the lower > frequencies. Yes, but has anyone actually tested those frequencies? Judging from the FEX files I could gather, cpufreq never actually goes lower than 480 MHz. > > However, this implementation is a new thing, and it was using the H3 > > precisely because of its early stage of support to use it as a testbed > > for the more established. > > > > If you feel like we should use a different formula to favour the > > multipliers over the dividers (or want to change the class of the CPU > > PLL to an NKM or something else, this is totally doable. > > I think the original formula that's currently in the mainline kernel is > better and avoids fiddling with dividers too much. Yeah, but the older formula is not generic at all. The whole rework was precisely to avoid doing the whole one driver per clock that was just becoming a nightmare to maintain, and a pain to add support for new SoCs. That code will be used for A10's CPU and VE PLLs too for example. And they probably have the same constraints, but with different variations (available values of each factors for example). > >> "The P factor only use in the condition that PLL output less than 288 > >> MHz." > > > > And the datasheet also had some issues, either misleading or wrong > > comments in the past. Don't get me wrong, I'm not saying that this is > > wrong, just that we should not follow it religiously, and that we > > should trust more the experiments than the datasheet. > > I can believe that. :) Regardless, I think the reasons given for > avoiding dividers are quite reasonable. It's based on how PLL block > works, not what manual says. Yes, indeed. Would replacing the current factors computation function by something like: for (m = 1; m < max_m; m++) for (p = 1; p < max_p; p++) for (n = 1; n < max_n; n++) for (k = 1; k < max_k; k++) if rate == computed rate break; work for you? That way, we will favor the multipliers over the dividers, and we can always "blacklist" p or m (or both) by setting their maximum to 1 (this would need an extra field in _ccu_div though). > >> Also other datasheets of similar socs from Allwinner state that M should > >> not be used in production code. > > > > Which ones specifically? > > A83T for example. You can search for "only for test" phrase. > > https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf > > Those PLLs are a bit different though. Thanks, Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160731/533d46e0/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-31 10:31 ` Maxime Ripard @ 2016-07-31 22:01 ` Ondřej Jirman 2016-08-31 20:25 ` Maxime Ripard 0 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-07-31 22:01 UTC (permalink / raw) To: linux-arm-kernel Hi, On 31.7.2016 12:31, Maxime Ripard wrote: > Hi, > > On Fri, Jul 29, 2016 at 12:01:09AM +0200, Ond?ej Jirman wrote: >> On 28.7.2016 23:00, Maxime Ripard wrote: >>> Hi Ondrej, >>> >>> On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ond?ej Jirman wrote: >>>> Hi Maxime, >>>> >>>> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying >>>> to this. >>> >>> You can find it in the clock maintainers tree: >>> https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng >>> >>>> On 26.7.2016 08:32, Maxime Ripard wrote: >>>>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ond?ej Jirman wrote: >>>>>>>>> If so, then yes, trying to switch to the 24MHz oscillator before >>>>>>>>> applying the factors, and then switching back when the PLL is stable >>>>>>>>> would be a nice solution. >>>>>>>>> >>>>>>>>> I just checked, and all the SoCs we've had so far have that >>>>>>>>> possibility, so if it works, for now, I'd like to stick to that. >>>>>>>> >>>>>>>> It would need to be tested. U-boot does the change only once, while the >>>>>>>> kernel would be doing it all the time and between various frequencies >>>>>>>> and PLL settings. So the issues may show up with this solution too. >>>>>>> >>>>>>> That would have the benefit of being quite easy to document, not be a >>>>>>> huge amount of code and it would work on all the CPUs PLLs we have so >>>>>>> far, so still, a pretty big win. If it doesn't, of course, we don't >>>>>>> really have the choice. >>>>>> >>>>>> It's probably more code though. It has to access different register from >>>>>> the one that is already defined in dts, which would add a lot of code >>>>>> and require dts changes. The original patch I sent is simpler than that. >>>>> >>>>> Why? >>>>> >>>>> You can use container_of to retrieve the parent structure of the clock >>>>> notifier, and then you get a ccu_common structure pointer, with the >>>>> CCU base address, the clock register, its lock, etc. >>>>> >>>>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC. >>>>> >>>>> I don't really get why anything should be changed in the DT, or why it >>>>> would add a lot of code. Or maybe we're not talking about the same >>>>> thing? >>>> >>>> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that >>>> it very liberally uses divider parameters, even in situations that are >>>> out of spec compared to the current code in the kernel. >>>> >>>> In the current code and especially in the original vendor code, divider >>>> parameters are used as last resort only. Presumably because, of the >>>> inherent trouble in changing them, as I described to you in other email. >>>> >>>> The new ccu code uses dividers often and even at very high frequencies, >>>> which goes against the spec. >>>> >>>> In the vendor code M is never anything else but 0, and P is used only >>>> for frequencies below 288MHz, which matches the H3 datasheet, which says: >>> >>> In the vendor code, P is never used either. All the boards we had so >>> far don't go that low, so we cannot make any of these assumptions, >>> especially since the vendor code has had the bad habit of doing >>> something wrong and / or useless in the past. >> >> P is used in the arisc firmware according to the spec for the lower >> frequencies. > > Yes, but has anyone actually tested those frequencies? Judging from > the FEX files I could gather, cpufreq never actually goes lower than > 480 MHz. > I tested it. It works well down to 60MHz. You can even run on 24MHz if you run directly from 24mhz osc. It's terribly slow, but it works ok. I've rebased my working branch over the mainline kernel, which now contains the sunxi-ng, and tested it. Cpufreq seems to work on orange pi pc without any changes discussed in this thread. I didn't do any extensive testing though. But it doesn't hang on boot or cpufreq config changes. You can see it here: https://github.com/megous/linux/commits/orange-pi-4.8 >>> However, this implementation is a new thing, and it was using the H3 >>> precisely because of its early stage of support to use it as a testbed >>> for the more established. >>> >>> If you feel like we should use a different formula to favour the >>> multipliers over the dividers (or want to change the class of the CPU >>> PLL to an NKM or something else, this is totally doable. >> >> I think the original formula that's currently in the mainline kernel is >> better and avoids fiddling with dividers too much. > > Yeah, but the older formula is not generic at all. The whole rework > was precisely to avoid doing the whole one driver per clock that was > just becoming a nightmare to maintain, and a pain to add support for > new SoCs. That code will be used for A10's CPU and VE PLLs too for > example. And they probably have the same constraints, but with > different variations (available values of each factors for example). Fair enough. >>>> "The P factor only use in the condition that PLL output less than 288 >>>> MHz." >>> >>> And the datasheet also had some issues, either misleading or wrong >>> comments in the past. Don't get me wrong, I'm not saying that this is >>> wrong, just that we should not follow it religiously, and that we >>> should trust more the experiments than the datasheet. >> >> I can believe that. :) Regardless, I think the reasons given for >> avoiding dividers are quite reasonable. It's based on how PLL block >> works, not what manual says. > > Yes, indeed. > > Would replacing the current factors computation function by something > like: > > for (m = 1; m < max_m; m++) > for (p = 1; p < max_p; p++) > for (n = 1; n < max_n; n++) > for (k = 1; k < max_k; k++) > if rate == computed rate > break; > > work for you? This would be better. thank you and regards, Ondrej > That way, we will favor the multipliers over the dividers, and we can > always "blacklist" p or m (or both) by setting their maximum to 1 > (this would need an extra field in _ccu_div though). > >>>> Also other datasheets of similar socs from Allwinner state that M should >>>> not be used in production code. >>> >>> Which ones specifically? >> >> A83T for example. You can search for "only for test" phrase. >> >> https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf >> >> Those PLLs are a bit different though. > > Thanks, > Maxime > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160801/5d9638b9/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* Changed: sunxi-ng clock code - NKMP clock implementation is wrong 2016-07-31 22:01 ` Ondřej Jirman @ 2016-08-31 20:25 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-08-31 20:25 UTC (permalink / raw) To: linux-arm-kernel Hi Ondrej, Sorry for the (very) belated reply. On Mon, Aug 01, 2016 at 12:01:39AM +0200, Ond?ej Jirman wrote: > >>>> In the vendor code M is never anything else but 0, and P is used only > >>>> for frequencies below 288MHz, which matches the H3 datasheet, which says: > >>> > >>> In the vendor code, P is never used either. All the boards we had so > >>> far don't go that low, so we cannot make any of these assumptions, > >>> especially since the vendor code has had the bad habit of doing > >>> something wrong and / or useless in the past. > >> > >> P is used in the arisc firmware according to the spec for the lower > >> frequencies. > > > > Yes, but has anyone actually tested those frequencies? Judging from > > the FEX files I could gather, cpufreq never actually goes lower than > > 480 MHz. > > > > I tested it. It works well down to 60MHz. You can even run on 24MHz if > you run directly from 24mhz osc. It's terribly slow, but it works ok. Ok, it's good to know (and yeah, I wouldn't expect a CPU at 24MHz to be very fast :)) > I've rebased my working branch over the mainline kernel, which now > contains the sunxi-ng, and tested it. Cpufreq seems to work on orange pi > pc without any changes discussed in this thread. I didn't do any > extensive testing though. But it doesn't hang on boot or cpufreq config > changes. > > You can see it here: > > https://github.com/megous/linux/commits/orange-pi-4.8 Thanks a lot. That's good to know as well. > >>>> "The P factor only use in the condition that PLL output less than 288 > >>>> MHz." > >>> > >>> And the datasheet also had some issues, either misleading or wrong > >>> comments in the past. Don't get me wrong, I'm not saying that this is > >>> wrong, just that we should not follow it religiously, and that we > >>> should trust more the experiments than the datasheet. > >> > >> I can believe that. :) Regardless, I think the reasons given for > >> avoiding dividers are quite reasonable. It's based on how PLL block > >> works, not what manual says. > > > > Yes, indeed. > > > > Would replacing the current factors computation function by something > > like: > > > > for (m = 1; m < max_m; m++) > > for (p = 1; p < max_p; p++) > > for (n = 1; n < max_n; n++) > > for (k = 1; k < max_k; k++) > > if rate == computed rate > > break; > > > > work for you? > > This would be better. Ok, I'll try to cook something up when I get the time. If you feel like it's not done fast enough, feel free to do it. Note that we also ended up merging for the A31 some code that reparent CPUx temporarily while setting the PLL-CPUX rate, and it should be somewhat generic. But of course, it's not exclusive. Thanks again for all your tests, it's very appreciated. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160831/6fccb8c4/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-06-30 20:40 ` Maxime Ripard 2016-07-01 0:50 ` Ondřej Jirman @ 2016-07-01 0:53 ` Ondřej Jirman 2016-07-15 8:19 ` Maxime Ripard 1 sibling, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-07-01 0:53 UTC (permalink / raw) To: linux-arm-kernel On 30.6.2016 22:40, Maxime Ripard wrote: > Hi, > > On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous at megous.com wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> PLL1 on H3 requires special factors application algorithm, >> when the rate is changed. This algorithm was extracted >> from the arisc code that handles frequency scaling >> in the BSP kernel. >> >> This commit adds optional apply function to >> struct factors_data, that can implement non-trivial >> factors application method, when necessary. >> >> Also struct clk_factors_config is extended with position >> of the PLL lock flag. > > Have you tested the current implementation, and found that it was not > working, or did you duplicate the arisc code directly? Also of note is that similar code probably doesn't crash in u-boot, because there, before changing the PLL1 clock, the cpu is switched to 24MHz osc, so it is not overclocked, even if factors align in such a way that you'd get the behavior I described in the other email. >> /** >> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the >> + * register using an algorithm that tries to reserve the PLL lock >> + */ >> + >> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req) >> +{ >> + const struct clk_factors_config *config = factors->config; >> + u32 reg; >> + >> + /* Fetch the register value */ >> + reg = readl(factors->reg); >> + >> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) { >> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } > > So there was some doubts about the fact that P was being used, or at > least that it was useful. > >> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) { >> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } >> + >> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n); >> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k); >> + >> + writel(reg, factors->reg); >> + __delay(20); >> + >> + while (!(readl(factors->reg) & (1 << config->lock))); > > So, they are applying the dividers first, and then applying the > multipliers, and then wait for the PLL to stabilize. > >> + >> + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) { >> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } >> + >> + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) { >> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p); >> + >> + writel(reg, factors->reg); >> + __delay(2000); >> + } > > However, this is kind of weird, why would you need to re-apply the > dividers? Nothing really changes. Have you tried without that part? > > Since this is really specific, I guess you could simply make the > clk_ops for the nkmp clocks public, and just re-implement set_rate > using that logic. > > You might also need to set an upper limit on P, since the last value > (4) is not a valid one. > > I guess you could do that by adding a max field in the __ccu_div > structure. > > Maxime > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160701/e95205dd/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method 2016-07-01 0:53 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method Ondřej Jirman @ 2016-07-15 8:19 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-07-15 8:19 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 01, 2016 at 02:53:52AM +0200, Ond?ej Jirman wrote: > On 30.6.2016 22:40, Maxime Ripard wrote: > > Hi, > > > > On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous at megous.com wrote: > >> From: Ondrej Jirman <megous@megous.com> > >> > >> PLL1 on H3 requires special factors application algorithm, > >> when the rate is changed. This algorithm was extracted > >> from the arisc code that handles frequency scaling > >> in the BSP kernel. > >> > >> This commit adds optional apply function to > >> struct factors_data, that can implement non-trivial > >> factors application method, when necessary. > >> > >> Also struct clk_factors_config is extended with position > >> of the PLL lock flag. > > > > Have you tested the current implementation, and found that it was not > > working, or did you duplicate the arisc code directly? > > Also of note is that similar code probably doesn't crash in u-boot, > because there, before changing the PLL1 clock, the cpu is switched to > 24MHz osc, so it is not overclocked, even if factors align in such a way > that you'd get the behavior I described in the other email. That's also something that we can do. See Meson's clk-cpu clock notifiers for example. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/235bbbce/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 07/14] ARM: dts: sun8i: Use sun8i-h3-pll1-clk for pll1 in H3 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (5 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 3:45 ` [PATCH v2 08/14] ARM: dts: sun8i: Add thermal sensor node to the sun8i-h3.dtsi megous at megous.com ` (6 subsequent siblings) 13 siblings, 0 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> PLL1 on H3 requires special factors application algorithm, when the rate is changed. This algorithm was extracted from the arisc code that handles frequency scaling in the BSP kernel. This algorithm is implemented by sun8i-h3-pll1-clk. Signed-off-by: Ondrej Jirman <megous@megous.com> --- arch/arm/boot/dts/sun8i-h3.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index 4a4926b..b3247f4 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -106,7 +106,7 @@ pll1: clk at 01c20000 { #clock-cells = <0>; - compatible = "allwinner,sun8i-a23-pll1-clk"; + compatible = "allwinner,sun8i-h3-pll1-clk"; reg = <0x01c20000 0x4>; clocks = <&osc24M>; clock-output-names = "pll1"; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 08/14] ARM: dts: sun8i: Add thermal sensor node to the sun8i-h3.dtsi 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (6 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 07/14] ARM: dts: sun8i: Use sun8i-h3-pll1-clk for pll1 in H3 megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 3:45 ` [PATCH v2 09/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi megous at megous.com ` (5 subsequent siblings) 13 siblings, 0 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> This patch adds nodes for the thermal sensor driver and the THS clock to the Allwinner sun8i-h3.dtsi file. Signed-off-by: Ondrej Jirman <megous@megous.com> --- arch/arm/boot/dts/sun8i-h3.dtsi | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index b3247f4..d3c29cc 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -77,6 +77,14 @@ }; }; + thermal-zones { + cpu_thermal: cpu_thermal { + polling-delay-passive = <330>; + polling-delay = <1000>; + thermal-sensors = <&ths 0>; + }; + }; + timer { compatible = "arm,armv7-timer"; interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, @@ -239,6 +247,14 @@ "bus_scr", "bus_ephy", "bus_dbg"; }; + ths_clk: clk at 01c20074 { + #clock-cells = <0>; + compatible = "allwinner,sun8i-h3-ths-clk"; + reg = <0x01c20074 0x4>; + clocks = <&osc24M>; + clock-output-names = "ths"; + }; + mmc0_clk: clk at 01c20088 { #clock-cells = <1>; compatible = "allwinner,sun4i-a10-mmc-clk"; @@ -574,6 +590,18 @@ interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>; }; + ths: ths at 01c25000 { + #thermal-sensor-cells = <0>; + compatible = "allwinner,sun8i-h3-ths"; + reg = <0x01c25000 0x400>, + <0x01c14234 0x4>; + interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>; + resets = <&apb1_rst 8>; + reset-names = "ahb"; + clocks = <&bus_gates 72>, <&ths_clk>; + clock-names = "ahb", "ths"; + }; + uart0: serial at 01c28000 { compatible = "snps,dw-apb-uart"; reg = <0x01c28000 0x400>; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 09/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (7 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 08/14] ARM: dts: sun8i: Add thermal sensor node to the sun8i-h3.dtsi megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 3:45 ` [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller megous at megous.com ` (4 subsequent siblings) 13 siblings, 0 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> Add label to the first cpu so that it can be referenced from derived dts files. Signed-off-by: Ondrej Jirman <megous@megous.com> --- v2 - move clocks/clock-latency to sun8i-h3.dtsi --- arch/arm/boot/dts/sun8i-h3.dtsi | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index d3c29cc..56f211e 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -52,7 +52,9 @@ #address-cells = <1>; #size-cells = <0>; - cpu at 0 { + cpu0: cpu at 0 { + clocks = <&cpu>; + clock-latency = <244144>; /* 8 32k periods */ compatible = "arm,cortex-a7"; device_type = "cpu"; reg = <0>; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (8 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 09/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 7:16 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 11/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC megous at megous.com ` (3 subsequent siblings) 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> H3 SoC contains I2C controller optionally available on the PL0 and PL1 pins. This patch makes this controller available. Signed-off-by: Ondrej Jirman <megous@megous.com> --- arch/arm/boot/dts/sun8i-h3.dtsi | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index 56f211e..e32f211 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -322,8 +322,9 @@ reg = <0x01f01428 0x4>; #clock-cells = <1>; clocks = <&apb0>; - clock-indices = <0>, <1>; - clock-output-names = "apb0_pio", "apb0_ir"; + clock-indices = <0>, <1>, <6>; + clock-output-names = "apb0_pio", "apb0_ir", "apb0_i2c"; + }; ir_clk: ir_clk at 01f01454 { @@ -656,6 +657,20 @@ status = "disabled"; }; + r_twi: i2c at 01f02400 { + compatible = "allwinner,sun6i-a31-i2c"; + reg = <0x01f02400 0x400>; + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&r_twi_pins_a>; + clocks = <&apb0_gates 6>; + clock-frequency = <100000>; + resets = <&apb0_reset 6>; + status = "disabled"; + #address-cells = <1>; + #size-cells = <0>; + }; + gic: interrupt-controller at 01c81000 { compatible = "arm,cortex-a7-gic", "arm,cortex-a15-gic"; reg = <0x01c81000 0x1000>, @@ -707,6 +722,13 @@ allwinner,drive = <SUN4I_PINCTRL_10_MA>; allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; }; + + r_twi_pins_a: r_twi at 0 { + allwinner,pins = "PL0", "PL1"; + allwinner,function = "s_twi"; + allwinner,drive = <SUN4I_PINCTRL_10_MA>; + allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; + }; }; }; }; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller 2016-06-25 3:45 ` [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller megous at megous.com @ 2016-06-25 7:16 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-06-25 7:16 UTC (permalink / raw) To: linux-arm-kernel Hi, On Sat, Jun 25, 2016 at 05:45:07AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > H3 SoC contains I2C controller optionally available > on the PL0 and PL1 pins. This patch makes this controller > available. > > Signed-off-by: Ondrej Jirman <megous@megous.com> > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 56f211e..e32f211 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -322,8 +322,9 @@ > reg = <0x01f01428 0x4>; > #clock-cells = <1>; > clocks = <&apb0>; > - clock-indices = <0>, <1>; > - clock-output-names = "apb0_pio", "apb0_ir"; > + clock-indices = <0>, <1>, <6>; > + clock-output-names = "apb0_pio", "apb0_ir", "apb0_i2c"; > + The new line is not needed, and it should be part of a separate patch. > }; > > ir_clk: ir_clk at 01f01454 { > @@ -656,6 +657,20 @@ > status = "disabled"; > }; > > + r_twi: i2c at 01f02400 { We call it i2c everywhere, please call it that way too. > + compatible = "allwinner,sun6i-a31-i2c"; > + reg = <0x01f02400 0x400>; > + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&r_twi_pins_a>; > + clocks = <&apb0_gates 6>; > + clock-frequency = <100000>; > + resets = <&apb0_reset 6>; > + status = "disabled"; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > gic: interrupt-controller at 01c81000 { > compatible = "arm,cortex-a7-gic", "arm,cortex-a15-gic"; > reg = <0x01c81000 0x1000>, > @@ -707,6 +722,13 @@ > allwinner,drive = <SUN4I_PINCTRL_10_MA>; > allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; > }; > + > + r_twi_pins_a: r_twi at 0 { > + allwinner,pins = "PL0", "PL1"; > + allwinner,function = "s_twi"; > + allwinner,drive = <SUN4I_PINCTRL_10_MA>; > + allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; Separate patch please. Thanks! -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160625/5c48f897/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 11/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (9 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 3:45 ` [PATCH v2 12/14] ARM: dts: sun8i: Setup CPU operating points for Onrage PI PC megous at megous.com ` (2 subsequent siblings) 13 siblings, 0 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> Add sy8106a regulator to r_twi bus and enable the r_twi bus on Orange Pi PC. This regulator controls the CPUX voltage. Signed-off-by: Ondrej Jirman <megous@megous.com> --- arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts index daf50b9a6..79f0b49 100644 --- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts @@ -90,6 +90,20 @@ }; }; +&r_twi { + status = "okay"; + + vdd_cpu: regulator at 65 { + compatible = "silergy,sy8106a"; + reg = <0x65>; + regulator-min-microvolt = <1000000>; + regulator-max-microvolt = <1400000>; + regulator-ramp-delay = <200>; + regulator-boot-on; + regulator-always-on; + }; +}; + &ehci1 { status = "okay"; }; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 12/14] ARM: dts: sun8i: Setup CPU operating points for Onrage PI PC 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (10 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 11/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 3:45 ` [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One megous at megous.com 2016-06-25 3:45 ` [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS " megous at megous.com 13 siblings, 0 replies; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> Orange PI PC uses SY8106A regulator for fine grained CPUX voltage regulation. Setup appropriate operating points for the board. --- arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts index 79f0b49..1b029e9 100644 --- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts @@ -90,6 +90,54 @@ }; }; +&cpu0 { + operating-points = < + /* kHz uV */ + 1512000 1400000 + 1440000 1400000 + 1368000 1340000 + 1344000 1340000 + 1296000 1340000 + 1248000 1300000 + 1224000 1300000 + 1200000 1300000 + 1104000 1200000 + 1008000 1140000 + 960000 1100000 + 648000 1100000 + 480000 1100000 + 240000 1100000 + 120000 1100000 + >; + #cooling-cells = <2>; + cooling-min-level = <0>; + cooling-max-level = <14>; + cpu0-supply = <&vdd_cpu>; +}; + +&cpu_thermal { + cooling-maps { + map0 { + trip = <&cpu_alert0>; + cooling-device = <&cpu0 (-1) (-1)>; + }; + }; + + trips { + cpu_alert0: cpu_alert0 { + temperature = <80000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_crit: cpu_crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; +}; + &r_twi { status = "okay"; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (11 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 12/14] ARM: dts: sun8i: Setup CPU operating points for Onrage PI PC megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-25 7:18 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS " megous at megous.com 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> Xulong Orange Pi One uses GPIO based regulator that switches between two voltages: 1.1V and 1.3V. The regulator is controlled from the PL6 pin. Signed-off-by: Ondrej Jirman <megous@megous.com> --- v2 - add missing pinctrl-names for gpio-regulator --- arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts index 0adf932..b1bd6b0 100644 --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts @@ -88,6 +88,25 @@ gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>; }; }; + + vdd_soc: gpio-regulator { + compatible = "regulator-gpio"; + pinctrl-names = "default"; + pinctrl-0 = <&vdd_reg_r_opc>; + + regulator-name = "soc-vdd-supply"; + regulator-min-microvolt = <1100000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + regulator-type = "voltage"; + + gpios = <&r_pio 0 6 GPIO_ACTIVE_HIGH>; + states = <1100000 0x0 + 1300000 0x1>; + + startup-delay-us = <100000>; + enable-active-high; + }; }; &ehci1 { @@ -131,6 +150,13 @@ allwinner,drive = <SUN4I_PINCTRL_10_MA>; allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; }; + + vdd_reg_r_opc: regulator_pins at 0 { + allwinner,pins = "PL6"; + allwinner,function = "gpio_out"; + allwinner,drive = <SUN4I_PINCTRL_10_MA>; + allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; + }; }; &uart0 { -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One 2016-06-25 3:45 ` [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One megous at megous.com @ 2016-06-25 7:18 ` Maxime Ripard 0 siblings, 0 replies; 66+ messages in thread From: Maxime Ripard @ 2016-06-25 7:18 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jun 25, 2016 at 05:45:10AM +0200, megous at megous.com wrote: > From: Ondrej Jirman <megous@megous.com> > > Xulong Orange Pi One uses GPIO based regulator that > switches between two voltages: 1.1V and 1.3V. The > regulator is controlled from the PL6 pin. > > Signed-off-by: Ondrej Jirman <megous@megous.com> > --- > v2 > - add missing pinctrl-names for gpio-regulator > --- > arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > index 0adf932..b1bd6b0 100644 > --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > @@ -88,6 +88,25 @@ > gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>; > }; > }; > + > + vdd_soc: gpio-regulator { > + compatible = "regulator-gpio"; > + pinctrl-names = "default"; > + pinctrl-0 = <&vdd_reg_r_opc>; > + > + regulator-name = "soc-vdd-supply"; > + regulator-min-microvolt = <1100000>; > + regulator-max-microvolt = <1300000>; > + regulator-boot-on; > + regulator-type = "voltage"; It should be marked as always-on. Otherwise, if the cpufreq driver is not enabled, the regulator will be shutdown, which is not that great :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160625/afaa2985/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com ` (12 preceding siblings ...) 2016-06-25 3:45 ` [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One megous at megous.com @ 2016-06-25 3:45 ` megous at megous.com 2016-06-30 11:13 ` [linux-sunxi] " Michal Suchanek 13 siblings, 1 reply; 66+ messages in thread From: megous at megous.com @ 2016-06-25 3:45 UTC (permalink / raw) To: linux-arm-kernel From: Ondrej Jirman <megous@megous.com> Use Xulong Orange Pi One GPIO based regulator for passive cooling and thermal management. Signed-off-by: Ondrej Jirman <megous@megous.com> --- arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts index b1bd6b0..a38d871 100644 --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts @@ -109,6 +109,45 @@ }; }; +&cpu0 { + operating-points = < + /* kHz uV */ + 1296000 1300000 + 1200000 1300000 + 624000 1100000 + 480000 1100000 + 312000 1100000 + 240000 1100000 + >; + #cooling-cells = <2>; + cooling-min-level = <0>; + cooling-max-level = <5>; + cpu0-supply = <&vdd_soc>; +}; + +&cpu_thermal { + cooling-maps { + map0 { + trip = <&cpu_alert0>; + cooling-device = <&cpu0 (-1) (-1)>; + }; + }; + + trips { + cpu_alert0: cpu_alert0 { + temperature = <80000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_crit: cpu_crit { + temperature = <100000>; + hysteresis = <2000>; + type = "critical"; + }; + }; +}; + &ehci1 { status = "okay"; }; -- 2.9.0 ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-25 3:45 ` [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS " megous at megous.com @ 2016-06-30 11:13 ` Michal Suchanek 2016-06-30 14:19 ` Ondřej Jirman ` (2 more replies) 0 siblings, 3 replies; 66+ messages in thread From: Michal Suchanek @ 2016-06-30 11:13 UTC (permalink / raw) To: linux-arm-kernel Hello, On 25 June 2016 at 05:45, <megous@megous.com> wrote: > From: Ondrej Jirman <megous@megous.com> > > Use Xulong Orange Pi One GPIO based regulator for > passive cooling and thermal management. > > Signed-off-by: Ondrej Jirman <megous@megous.com> > --- > arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > index b1bd6b0..a38d871 100644 > --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > @@ -109,6 +109,45 @@ > }; > }; > > +&cpu0 { > + operating-points = < > + /* kHz uV */ > + 1296000 1300000 > + 1200000 1300000 First problem is that the board boots at 1008000 which is not listed and the kernel complains. Second problem is that the board locks up during boot with this enabled. Do you have some suggestion for alternate configuration to test? Thanks Michal ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 11:13 ` [linux-sunxi] " Michal Suchanek @ 2016-06-30 14:19 ` Ondřej Jirman 2016-06-30 15:16 ` Michal Suchanek 2016-06-30 14:23 ` Siarhei Siamashka 2016-07-22 0:41 ` Ondřej Jirman 2 siblings, 1 reply; 66+ messages in thread From: Ondřej Jirman @ 2016-06-30 14:19 UTC (permalink / raw) To: linux-arm-kernel Hello, On 30.6.2016 13:13, Michal Suchanek wrote: > Hello, > > On 25 June 2016 at 05:45, <megous@megous.com> wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> Use Xulong Orange Pi One GPIO based regulator for >> passive cooling and thermal management. >> >> Signed-off-by: Ondrej Jirman <megous@megous.com> >> --- >> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >> index b1bd6b0..a38d871 100644 >> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >> @@ -109,6 +109,45 @@ >> }; >> }; >> >> +&cpu0 { >> + operating-points = < >> + /* kHz uV */ >> + 1296000 1300000 >> + 1200000 1300000 > > First problem is that the board boots at 1008000 which is not listed > and the kernel complains. > > Second problem is that the board locks up during boot with this enabled. > > Do you have some suggestion for alternate configuration to test? Just to verify, did you test with the entire series applied? (especially the PLL1 clk application changes) You may try dropping the highest operating point, it's probably overly optimistic for Orange Pi One. Is the power supply/cable you're using hard enough? Where during the boot process does it lock up? regards, Ondrej > Thanks > > Michal > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160630/cf58b266/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 14:19 ` Ondřej Jirman @ 2016-06-30 15:16 ` Michal Suchanek 2016-06-30 15:32 ` Ondřej Jirman 2016-06-30 15:50 ` Michal Suchanek 0 siblings, 2 replies; 66+ messages in thread From: Michal Suchanek @ 2016-06-30 15:16 UTC (permalink / raw) To: linux-arm-kernel On 30 June 2016 at 16:19, Ond?ej Jirman <megous@megous.com> wrote: > Hello, > > On 30.6.2016 13:13, Michal Suchanek wrote: >> Hello, >> >> On 25 June 2016 at 05:45, <megous@megous.com> wrote: >>> From: Ondrej Jirman <megous@megous.com> >>> >>> Use Xulong Orange Pi One GPIO based regulator for >>> passive cooling and thermal management. >>> >>> Signed-off-by: Ondrej Jirman <megous@megous.com> >>> --- >>> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>> index b1bd6b0..a38d871 100644 >>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>> @@ -109,6 +109,45 @@ >>> }; >>> }; >>> >>> +&cpu0 { >>> + operating-points = < >>> + /* kHz uV */ >>> + 1296000 1300000 >>> + 1200000 1300000 >> >> First problem is that the board boots at 1008000 which is not listed >> and the kernel complains. >> >> Second problem is that the board locks up during boot with this enabled. >> >> Do you have some suggestion for alternate configuration to test? > > Just to verify, did you test with the entire series applied? (especially > the PLL1 clk application changes) Yes, I applied the whole series. > > You may try dropping the highest operating point, it's probably overly > optimistic for Orange Pi One. > > Is the power supply/cable you're using hard enough? I use a 7 port hub to power the board. It worked with some other small devboards. The cable is some random Chinese USB to power jack adaptor with an extra adaptor to fit the Pi socket. It looks ok but I did not test this particular combination thoroughly. > > Where during the boot process does it lock up? Usually sometime around enabling cpufreq before getty starts. Different runs and settings give slightly different results. In particular adding the 1008000 point seems to make it go further. Removing all traces of the regulator, cpufreq and thermal I can boot pretty much 100% to login prompt. I don't think the difference between 1GHz and 1.3GHz frequency on the core would put much additional stress on the supply but I can try with 35W PSU and some alternative cabling to be sure. I did some more tests and it seems 1008000 at 1.1V is fine but switching to 1.3V crashes the board. Even with only the first 1.3V state. Maybe there is need for some delay somewhere for the regulator to get stable? Thanks Michal ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 15:16 ` Michal Suchanek @ 2016-06-30 15:32 ` Ondřej Jirman 2016-06-30 15:50 ` Michal Suchanek 1 sibling, 0 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-06-30 15:32 UTC (permalink / raw) To: linux-arm-kernel On 30.6.2016 17:16, Michal Suchanek wrote: > On 30 June 2016 at 16:19, Ond?ej Jirman <megous@megous.com> wrote: >> Hello, >> >> On 30.6.2016 13:13, Michal Suchanek wrote: >>> Hello, >>> >>> On 25 June 2016 at 05:45, <megous@megous.com> wrote: >>>> From: Ondrej Jirman <megous@megous.com> >>>> >>>> Use Xulong Orange Pi One GPIO based regulator for >>>> passive cooling and thermal management. >>>> >>>> Signed-off-by: Ondrej Jirman <megous@megous.com> >>>> --- >>>> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>> index b1bd6b0..a38d871 100644 >>>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>> @@ -109,6 +109,45 @@ >>>> }; >>>> }; >>>> >>>> +&cpu0 { >>>> + operating-points = < >>>> + /* kHz uV */ >>>> + 1296000 1300000 >>>> + 1200000 1300000 >>> >>> First problem is that the board boots at 1008000 which is not listed >>> and the kernel complains. >>> >>> Second problem is that the board locks up during boot with this enabled. >>> >>> Do you have some suggestion for alternate configuration to test? >> >> Just to verify, did you test with the entire series applied? (especially >> the PLL1 clk application changes) > > Yes, I applied the whole series. > >> >> You may try dropping the highest operating point, it's probably overly >> optimistic for Orange Pi One. >> >> Is the power supply/cable you're using hard enough? > > I use a 7 port hub to power the board. It worked with some other small > devboards. > > The cable is some random Chinese USB to power jack adaptor with an > extra adaptor to fit the Pi socket. It looks ok but I did not test > this particular combination thoroughly. > >> >> Where during the boot process does it lock up? > > Usually sometime around enabling cpufreq before getty starts. > Different runs and settings give slightly different results. In > particular adding the 1008000 point seems to make it go further. > > Removing all traces of the regulator, cpufreq and thermal I can boot > pretty much 100% to login prompt. > > I don't think the difference between 1GHz and 1.3GHz frequency on the > core would put much additional stress on the supply but I can try with > 35W PSU and some alternative cabling to be sure. > > I did some more tests and it seems 1008000 at 1.1V is fine but switching > to 1.3V crashes the board. Even with only the first 1.3V state. Maybe > there is need for some delay somewhere for the regulator to get > stable? Please try adding: regulator-ramp-delay = <10> to the gpio regulator. which will allow 100ms per 1V change, which should b enough for determining, if this is the cause. regards, Ondrej > Thanks > > Michal > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160630/eb238767/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 15:16 ` Michal Suchanek 2016-06-30 15:32 ` Ondřej Jirman @ 2016-06-30 15:50 ` Michal Suchanek 2016-06-30 15:53 ` Ondřej Jirman 2016-07-01 10:54 ` Michal Suchanek 1 sibling, 2 replies; 66+ messages in thread From: Michal Suchanek @ 2016-06-30 15:50 UTC (permalink / raw) To: linux-arm-kernel On 30 June 2016 at 17:16, Michal Suchanek <hramrach@gmail.com> wrote: > On 30 June 2016 at 16:19, Ond?ej Jirman <megous@megous.com> wrote: >> Hello, >> >> On 30.6.2016 13:13, Michal Suchanek wrote: >>> Hello, >>> >>> On 25 June 2016 at 05:45, <megous@megous.com> wrote: >>>> From: Ondrej Jirman <megous@megous.com> >>>> >>>> Use Xulong Orange Pi One GPIO based regulator for >>>> passive cooling and thermal management. >>>> >>>> Signed-off-by: Ondrej Jirman <megous@megous.com> >>>> --- >>>> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>> index b1bd6b0..a38d871 100644 >>>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>> @@ -109,6 +109,45 @@ >>>> }; >>>> }; >>>> >>>> +&cpu0 { >>>> + operating-points = < >>>> + /* kHz uV */ >>>> + 1296000 1300000 >>>> + 1200000 1300000 >>> >>> First problem is that the board boots at 1008000 which is not listed >>> and the kernel complains. >>> >>> Second problem is that the board locks up during boot with this enabled. >>> >>> Do you have some suggestion for alternate configuration to test? >> >> Just to verify, did you test with the entire series applied? (especially >> the PLL1 clk application changes) > > Yes, I applied the whole series. > >> >> You may try dropping the highest operating point, it's probably overly >> optimistic for Orange Pi One. >> >> Is the power supply/cable you're using hard enough? > > I use a 7 port hub to power the board. It worked with some other small > devboards. > > The cable is some random Chinese USB to power jack adaptor with an > extra adaptor to fit the Pi socket. It looks ok but I did not test > this particular combination thoroughly. > >> >> Where during the boot process does it lock up? > > Usually sometime around enabling cpufreq before getty starts. > Different runs and settings give slightly different results. In > particular adding the 1008000 point seems to make it go further. > > Removing all traces of the regulator, cpufreq and thermal I can boot > pretty much 100% to login prompt. > > I don't think the difference between 1GHz and 1.3GHz frequency on the > core would put much additional stress on the supply but I can try with > 35W PSU and some alternative cabling to be sure. > > I did some more tests and it seems 1008000 at 1.1V is fine but switching > to 1.3V crashes the board. Even with only the first 1.3V state. Maybe > there is need for some delay somewhere for the regulator to get > stable? > Hm, the AW table shows 1008000 at 1.3V as supported state and it indeed works. So the voltage switching works or does nothing. Can I measure the regulator output somewhere? Clocking the chip higher does not work. I will try with another PSU later. Thanks Michal ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 15:50 ` Michal Suchanek @ 2016-06-30 15:53 ` Ondřej Jirman 2016-07-01 10:54 ` Michal Suchanek 1 sibling, 0 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-06-30 15:53 UTC (permalink / raw) To: linux-arm-kernel On 30.6.2016 17:50, Michal Suchanek wrote: > On 30 June 2016 at 17:16, Michal Suchanek <hramrach@gmail.com> wrote: >> On 30 June 2016 at 16:19, Ond?ej Jirman <megous@megous.com> wrote: >>> Hello, >>> >>> On 30.6.2016 13:13, Michal Suchanek wrote: >>>> Hello, >>>> >>>> On 25 June 2016 at 05:45, <megous@megous.com> wrote: >>>>> From: Ondrej Jirman <megous@megous.com> >>>>> >>>>> Use Xulong Orange Pi One GPIO based regulator for >>>>> passive cooling and thermal management. >>>>> >>>>> Signed-off-by: Ondrej Jirman <megous@megous.com> >>>>> --- >>>>> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>>> index b1bd6b0..a38d871 100644 >>>>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>>> @@ -109,6 +109,45 @@ >>>>> }; >>>>> }; >>>>> >>>>> +&cpu0 { >>>>> + operating-points = < >>>>> + /* kHz uV */ >>>>> + 1296000 1300000 >>>>> + 1200000 1300000 >>>> >>>> First problem is that the board boots at 1008000 which is not listed >>>> and the kernel complains. >>>> >>>> Second problem is that the board locks up during boot with this enabled. >>>> >>>> Do you have some suggestion for alternate configuration to test? >>> >>> Just to verify, did you test with the entire series applied? (especially >>> the PLL1 clk application changes) >> >> Yes, I applied the whole series. >> >>> >>> You may try dropping the highest operating point, it's probably overly >>> optimistic for Orange Pi One. >>> >>> Is the power supply/cable you're using hard enough? >> >> I use a 7 port hub to power the board. It worked with some other small >> devboards. >> >> The cable is some random Chinese USB to power jack adaptor with an >> extra adaptor to fit the Pi socket. It looks ok but I did not test >> this particular combination thoroughly. >> >>> >>> Where during the boot process does it lock up? >> >> Usually sometime around enabling cpufreq before getty starts. >> Different runs and settings give slightly different results. In >> particular adding the 1008000 point seems to make it go further. >> >> Removing all traces of the regulator, cpufreq and thermal I can boot >> pretty much 100% to login prompt. >> >> I don't think the difference between 1GHz and 1.3GHz frequency on the >> core would put much additional stress on the supply but I can try with >> 35W PSU and some alternative cabling to be sure. >> >> I did some more tests and it seems 1008000 at 1.1V is fine but switching >> to 1.3V crashes the board. Even with only the first 1.3V state. Maybe >> there is need for some delay somewhere for the regulator to get >> stable? >> > > Hm, the AW table shows 1008000 at 1.3V as supported state and it indeed > works. So the voltage switching works or does nothing. Can I measure > the regulator output somewhere? Clocking the chip higher does not > work. Regulator output is TP1 on the bottom of the board, next ot the CSI connector. regards, Ondrej > I will try with another PSU later. > > Thanks > > Michal > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160630/7cf02d08/attachment-0001.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 15:50 ` Michal Suchanek 2016-06-30 15:53 ` Ondřej Jirman @ 2016-07-01 10:54 ` Michal Suchanek 1 sibling, 0 replies; 66+ messages in thread From: Michal Suchanek @ 2016-07-01 10:54 UTC (permalink / raw) To: linux-arm-kernel On 30 June 2016 at 17:50, Michal Suchanek <hramrach@gmail.com> wrote: > On 30 June 2016 at 17:16, Michal Suchanek <hramrach@gmail.com> wrote: >> On 30 June 2016 at 16:19, Ond?ej Jirman <megous@megous.com> wrote: >>> Hello, >>)k, so I tried> >>> On 30.6.2016 13:13, Michal Suchanek wrote: >>>> Hello, >>>> >>>> On 25 June 2016 at 05:45, <megous@megous.com> wrote: >>>>> From: Ondrej Jirman <megous@megous.com> >>>>> >>>>> Use Xulong Orange Pi One GPIO based regulator for >>>>> passive cooling and thermal management. >>>>> >>>>> Signed-off-by: Ondrej Jirman <megous@megous.com> >>>>> --- >>>>> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>>> index b1bd6b0..a38d871 100644 >>>>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>>>> @@ -109,6 +109,45 @@ >>>>> }; >>>>> }; >>>>> >>>>> +&cpu0 { >>>>> + operating-points = < >>>>> + /* kHz uV */ >>>>> + 1296000 1300000 >>>>> + 1200000 1300000 >>>> >>>> First problem is that the board boots at 1008000 which is not listed >>>> and the kernel complains. >>>> >>>> Second problem is that the board locks up during boot with this enabled. >>>> >>>> Do you have some suggestion for alternate configuration to test? >>> >>> Just to verify, did you test with the entire series applied? (especially >>> the PLL1 clk application changes) >> >> Yes, I applied the whole series. >> >>> >>> You may try dropping the highest operating point, it's probably overly >>> optimistic for Orange Pi One. >>> >>> Is the power supply/cable you're using hard enough? >> >> I use a 7 port hub to power the board. It worked with some other small >> devboards. >> >> The cable is some random Chinese USB to power jack adaptor with an >> extra adaptor to fit the Pi socket. It looks ok but I did not test >> this particular combination thoroughly. >> >>> >>> Where during the boot process does it lock up? >> >> Usually sometime around enabling cpufreq before getty starts. >> Different runs and settings give slightly different results. In >> particular adding the 1008000 point seems to make it go further. >> >> Removing all traces of the regulator, cpufreq and thermal I can boot >> pretty much 100% to login prompt. >> >> I don't think the difference between 1GHz and 1.3GHz frequency on the >> core would put much additional stress on the supply but I can try with >> 35W PSU and some alternative cabling to be sure. >> >> I did some more tests and it seems 1008000 at 1.1V is fine but switching >> to 1.3V crashes the board. Even with only the first 1.3V state. Maybe >> there is need for some delay somewhere for the regulator to get >> stable? >> > > Hm, the AW table shows 1008000 at 1.3V as supported state and it indeed > works. So the voltage switching works or does nothing. Can I measure > the regulator output somewhere? Clocking the chip higher does not > work. > > I will try with another PSU later. > Ok, so I tried. The result is the same. A Chinese USB power meter shows voltage 5.1V which drops to like 4.95V when the board is running. The power draw is around 170mA and about 200mA when switch to 1.3V is attempted. The voltage drop is not nice and is probably due to excessive cabling used to distribute power to multiple boards. The USB hub starts at 5.2V and drops to something like 5.1V. When the top frequency is 1008000 at 1.3V and governor is performance the board keeps running 1008000 at 1.1V as set up by u-boot. Changing the top point to 1008000 at 1.1V the board locks up as soon as governor is changed to conservative. So any attempt at frequency scaling crashes even without touching the regulator. Thanks Michal ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 11:13 ` [linux-sunxi] " Michal Suchanek 2016-06-30 14:19 ` Ondřej Jirman @ 2016-06-30 14:23 ` Siarhei Siamashka 2016-07-01 1:17 ` Ondřej Jirman 2016-07-22 0:41 ` Ondřej Jirman 2 siblings, 1 reply; 66+ messages in thread From: Siarhei Siamashka @ 2016-06-30 14:23 UTC (permalink / raw) To: linux-arm-kernel On Thu, 30 Jun 2016 13:13:48 +0200 Michal Suchanek <hramrach@gmail.com> wrote: > Hello, > > On 25 June 2016 at 05:45, <megous@megous.com> wrote: > > From: Ondrej Jirman <megous@megous.com> > > > > Use Xulong Orange Pi One GPIO based regulator for > > passive cooling and thermal management. > > > > Signed-off-by: Ondrej Jirman <megous@megous.com> > > --- > > arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > > index b1bd6b0..a38d871 100644 > > --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > > +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts > > @@ -109,6 +109,45 @@ > > }; > > }; > > > > +&cpu0 { > > + operating-points = < > > + /* kHz uV */ > > + 1296000 1300000 > > + 1200000 1300000 > > First problem is that the board boots at 1008000 which is not listed > and the kernel complains. > > Second problem is that the board locks up during boot with this enabled. > > Do you have some suggestion for alternate configuration to test? Maybe try the Allwinner's original DVFS table instead of these undervolted values and see if it helps? https://linux-sunxi.org/index.php?title=Xunlong_Orange_Pi_PC&oldid=17753#CPU_clock_speed_limit While undervolting is tempting because it helps to decrease the SoC temperature and avoid throttling, different units may have different tolerances and one needs to be very careful when picking defaults that are intended to work correctly on all boards. Some safety headroom exists there for a reason. If I remember correctly, some people pushed for undervolting experiments at least twice in the past (on the Banana Pi and C.H.I.P.). In both cases this did not end up well and had to be fixed later to solve reliability problems. In order to allow individual per-unit tuning, a concept of "speed grading" may be probably introduced later. So that the board is tested for reliability and then the speed grade rating is stored somewhere on the non-removable storage (EEPROM, SPI flash, eFUSE, ...). Some SoC manufacturers, such as Samsung, are already doing this with their chips. ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 14:23 ` Siarhei Siamashka @ 2016-07-01 1:17 ` Ondřej Jirman 0 siblings, 0 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-07-01 1:17 UTC (permalink / raw) To: linux-arm-kernel On 30.6.2016 16:23, Siarhei Siamashka wrote: > On Thu, 30 Jun 2016 13:13:48 +0200 > Michal Suchanek <hramrach@gmail.com> wrote: > >> Hello, >> >> On 25 June 2016 at 05:45, <megous@megous.com> wrote: >>> From: Ondrej Jirman <megous@megous.com> >>> >>> Use Xulong Orange Pi One GPIO based regulator for >>> passive cooling and thermal management. >>> >>> Signed-off-by: Ondrej Jirman <megous@megous.com> >>> --- >>> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>> index b1bd6b0..a38d871 100644 >>> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >>> @@ -109,6 +109,45 @@ >>> }; >>> }; >>> >>> +&cpu0 { >>> + operating-points = < >>> + /* kHz uV */ >>> + 1296000 1300000 >>> + 1200000 1300000 >> >> First problem is that the board boots at 1008000 which is not listed >> and the kernel complains. >> >> Second problem is that the board locks up during boot with this enabled. >> >> Do you have some suggestion for alternate configuration to test? > > Maybe try the Allwinner's original DVFS table instead of these > undervolted values and see if it helps? > > https://linux-sunxi.org/index.php?title=Xunlong_Orange_Pi_PC&oldid=17753#CPU_clock_speed_limit Thanks, I'll use these. I'm not sure what will happen if regulator is not capable of providing the exact voltages specified in the operating points, though. Will it do the sane thing? :) Also LV6+ in the table is bellow specified minimum voltage in the datasheet. But then Orange Pi works at much lower voltages for me. Conversly LV1 is the absolute maximum. Recommended maximum is 1.4V So I would not use that. It might be nice to have something like that dram clock test, but for cpux freq/voltage, and collect some statistics. regards, Ondrej > While undervolting is tempting because it helps to decrease the SoC > temperature and avoid throttling, different units may have different > tolerances and one needs to be very careful when picking defaults > that are intended to work correctly on all boards. Some safety > headroom exists there for a reason. > > If I remember correctly, some people pushed for undervolting experiments > at least twice in the past (on the Banana Pi and C.H.I.P.). In both > cases this did not end up well and had to be fixed later to solve > reliability problems. > > In order to allow individual per-unit tuning, a concept of "speed > grading" may be probably introduced later. So that the board is tested > for reliability and then the speed grade rating is stored somewhere on > the non-removable storage (EEPROM, SPI flash, eFUSE, ...). Some SoC > manufacturers, such as Samsung, are already doing this with their chips. > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160701/e09949b2/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
* [linux-sunxi] [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One 2016-06-30 11:13 ` [linux-sunxi] " Michal Suchanek 2016-06-30 14:19 ` Ondřej Jirman 2016-06-30 14:23 ` Siarhei Siamashka @ 2016-07-22 0:41 ` Ondřej Jirman 2 siblings, 0 replies; 66+ messages in thread From: Ondřej Jirman @ 2016-07-22 0:41 UTC (permalink / raw) To: linux-arm-kernel Hello Michal, On 30.6.2016 13:13, Michal Suchanek wrote: > Hello, > > On 25 June 2016 at 05:45, <megous@megous.com> wrote: >> From: Ondrej Jirman <megous@megous.com> >> >> Use Xulong Orange Pi One GPIO based regulator for >> passive cooling and thermal management. >> >> Signed-off-by: Ondrej Jirman <megous@megous.com> >> --- >> arch/arm/boot/dts/sun8i-h3-orangepi-one.dts | 39 +++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >> index b1bd6b0..a38d871 100644 >> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts >> @@ -109,6 +109,45 @@ >> }; >> }; >> >> +&cpu0 { >> + operating-points = < >> + /* kHz uV */ >> + 1296000 1300000 >> + 1200000 1300000 > > First problem is that the board boots at 1008000 which is not listed > and the kernel complains. > > Second problem is that the board locks up during boot with this enabled. > > Do you have some suggestion for alternate configuration to test? I've identified the problem as incorrectly set up gpio-regulator. During boot, when it was probed, it would be initiualized with 1.1V for a moment. This caused random non-specific lockups/crashes afterwards. Fixed patches are in my branch on github, if you want to try again. https://github.com/megous/linux/commits/orange-pi-4.7 regards, o. > Thanks > > Michal > -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160722/bcc89923/attachment.sig> ^ permalink raw reply [flat|nested] 66+ messages in thread
end of thread, other threads:[~2016-08-31 20:25 UTC | newest] Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-25 3:44 [PATCH v2] Thermal regulation for Orange Pi PC and Orange Pi One megous at megous.com 2016-06-25 3:44 ` [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock megous at megous.com 2016-06-25 7:13 ` Maxime Ripard 2016-06-25 15:23 ` Ondřej Jirman 2016-06-28 11:52 ` Maxime Ripard 2016-06-25 3:44 ` [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 megous at megous.com 2016-06-25 7:10 ` Maxime Ripard 2016-06-25 15:12 ` Ondřej Jirman 2016-06-28 11:39 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver megous at megous.com 2016-06-28 20:56 ` Rob Herring 2016-06-25 3:45 ` [PATCH v2 04/14] regulator: SY8106A regulator driver megous at megous.com 2016-06-26 11:26 ` Mark Brown 2016-06-26 15:07 ` Ondřej Jirman 2016-06-27 14:54 ` Mark Brown 2016-06-28 16:27 ` Ondřej Jirman 2016-06-27 15:10 ` Mark Brown 2016-06-25 3:45 ` [PATCH v2 05/14] dt-bindings: document " megous at megous.com 2016-06-26 11:27 ` Mark Brown 2016-06-26 15:10 ` Ondřej Jirman 2016-06-26 18:52 ` Mark Brown 2016-06-25 3:45 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method megous at megous.com 2016-06-30 20:40 ` Maxime Ripard 2016-07-01 0:50 ` Ondřej Jirman 2016-07-01 5:37 ` Jean-Francois Moine 2016-07-01 6:34 ` Ondřej Jirman 2016-07-01 7:47 ` Jean-Francois Moine 2016-07-15 8:53 ` Maxime Ripard 2016-07-15 10:38 ` Ondřej Jirman 2016-07-15 13:27 ` Jean-Francois Moine 2016-07-15 13:48 ` Ondřej Jirman 2016-07-15 14:22 ` [linux-sunxi] " Michal Suchanek 2016-07-15 16:33 ` Ondřej Jirman 2016-07-21 9:51 ` Maxime Ripard 2016-07-21 9:48 ` Maxime Ripard 2016-07-21 9:52 ` Ondřej Jirman 2016-07-26 6:32 ` Maxime Ripard 2016-07-28 11:27 ` Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman 2016-07-28 11:38 ` [linux-sunxi] " Chen-Yu Tsai 2016-07-28 21:00 ` Maxime Ripard 2016-07-28 22:01 ` Ondřej Jirman 2016-07-31 10:31 ` Maxime Ripard 2016-07-31 22:01 ` Ondřej Jirman 2016-08-31 20:25 ` Maxime Ripard 2016-07-01 0:53 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method Ondřej Jirman 2016-07-15 8:19 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 07/14] ARM: dts: sun8i: Use sun8i-h3-pll1-clk for pll1 in H3 megous at megous.com 2016-06-25 3:45 ` [PATCH v2 08/14] ARM: dts: sun8i: Add thermal sensor node to the sun8i-h3.dtsi megous at megous.com 2016-06-25 3:45 ` [PATCH v2 09/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi megous at megous.com 2016-06-25 3:45 ` [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller megous at megous.com 2016-06-25 7:16 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 11/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC megous at megous.com 2016-06-25 3:45 ` [PATCH v2 12/14] ARM: dts: sun8i: Setup CPU operating points for Onrage PI PC megous at megous.com 2016-06-25 3:45 ` [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One megous at megous.com 2016-06-25 7:18 ` Maxime Ripard 2016-06-25 3:45 ` [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS " megous at megous.com 2016-06-30 11:13 ` [linux-sunxi] " Michal Suchanek 2016-06-30 14:19 ` Ondřej Jirman 2016-06-30 15:16 ` Michal Suchanek 2016-06-30 15:32 ` Ondřej Jirman 2016-06-30 15:50 ` Michal Suchanek 2016-06-30 15:53 ` Ondřej Jirman 2016-07-01 10:54 ` Michal Suchanek 2016-06-30 14:23 ` Siarhei Siamashka 2016-07-01 1:17 ` Ondřej Jirman 2016-07-22 0:41 ` Ondřej Jirman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).