[PATCH 01/14] ARM: dts: sun8i: Add SID node

[PATCH 01/14] ARM: dts: sun8i: Add SID node

[megous]

From: Josef Gajdusek <atx@atx.name>

Add a node describing the Security ID memory to the Allwinner H3 .dtsi file.

Signed-off-by: Josef Gajdusek <atx@atx.name>
---
 arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 4a4926b..172576d 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -389,6 +389,13 @@
 			#size-cells = <0>;
 		};
 
+		sid: eeprom@01c14000 {
+			compatible = "allwinner,sun4i-a10-sid";
+			reg = <0x01c14000 0x400>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		usbphy: phy@01c19400 {
 			compatible = "allwinner,sun8i-h3-usb-phy";
 			reg = <0x01c19400 0x2c>,
-- 
2.9.0

Re: [PATCH 01/14] ARM: dts: sun8i: Add SID node

[Chen-Yu Tsai]

On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> From: Josef Gajdusek <atx@atx.name>
>
> Add a node describing the Security ID memory to the Allwinner H3 .dtsi file.
>
> Signed-off-by: Josef Gajdusek <atx@atx.name>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 4a4926b..172576d 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -389,6 +389,13 @@
>                         #size-cells = <0>;
>                 };
>
> +               sid: eeprom@01c14000 {
> +                       compatible = "allwinner,sun4i-a10-sid";

This has been discussed before. The hardware is not compatible.
The write control registers are at different offsets.

ChenYu

> +                       reg = <0x01c14000 0x400>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +               };
> +
>                 usbphy: phy@01c19400 {
>                         compatible = "allwinner,sun8i-h3-usb-phy";
>                         reg = <0x01c19400 0x2c>,
> --
> 2.9.0
>

Re: [PATCH 01/14] ARM: dts: sun8i: Add SID node

[Ondřej Jirman]

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

Hello,

thank you for the review.

On 24.6.2016 04:41, Chen-Yu Tsai wrote:
> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>> From: Josef Gajdusek <atx@atx.name>
>>
>> Add a node describing the Security ID memory to the Allwinner H3 .dtsi file.
>>
>> Signed-off-by: Josef Gajdusek <atx@atx.name>
>> ---
>>  arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
>> index 4a4926b..172576d 100644
>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>> @@ -389,6 +389,13 @@
>>                         #size-cells = <0>;
>>                 };
>>
>> +               sid: eeprom@01c14000 {
>> +                       compatible = "allwinner,sun4i-a10-sid";
> 
> This has been discussed before. The hardware is not compatible.
> The write control registers are at different offsets.

I'm not sure what you mean by write control registers. Code in
sunxi_sid.c implements only read access to the nvram. Can you pelase
elaborate?

  Ondrej

> 
> ChenYu
> 
>> +                       reg = <0x01c14000 0x400>;
>> +                       #address-cells = <1>;
>> +                       #size-cells = <1>;
>> +               };
>> +
>>                 usbphy: phy@01c19400 {
>>                         compatible = "allwinner,sun8i-h3-usb-phy";
>>                         reg = <0x01c19400 0x2c>,
>> --
>> 2.9.0
>>


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

Re: [linux-sunxi] Re: [PATCH 01/14] ARM: dts: sun8i: Add SID node

[Chen-Yu Tsai]

Hi,

On Sat, Jun 25, 2016 at 3:58 AM, Ondřej Jirman <megous@megous.com> wrote:
> Hello,
>
> thank you for the review.
>
> On 24.6.2016 04:41, Chen-Yu Tsai wrote:
>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>> From: Josef Gajdusek <atx@atx.name>
>>>
>>> Add a node describing the Security ID memory to the Allwinner H3 .dtsi file.
>>>
>>> Signed-off-by: Josef Gajdusek <atx@atx.name>
>>> ---
>>>  arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
>>> index 4a4926b..172576d 100644
>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>>> @@ -389,6 +389,13 @@
>>>                         #size-cells = <0>;
>>>                 };
>>>
>>> +               sid: eeprom@01c14000 {
>>> +                       compatible = "allwinner,sun4i-a10-sid";
>>
>> This has been discussed before. The hardware is not compatible.
>> The write control registers are at different offsets.
>
> I'm not sure what you mean by write control registers. Code in
> sunxi_sid.c implements only read access to the nvram. Can you pelase
> elaborate?

See http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/388022.html

Also, different compatibles are used for different hardware, regardless
of how close the drivers may be. The driver might only be compatible when
implementing a subset of the possible features. If one were to fully
implement it, they would become incompatible.

To put it another way, the compatible string designates the hardware,
and the driver implements support for that compatible string.

ChenYu

>   Ondrej
>
>>
>> ChenYu
>>
>>> +                       reg = <0x01c14000 0x400>;
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <1>;
>>> +               };
>>> +
>>>                 usbphy: phy@01c19400 {
>>>                         compatible = "allwinner,sun8i-h3-usb-phy";
>>>                         reg = <0x01c19400 0x2c>,
>>> --
>>> 2.9.0
>>>
>
> --
> 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@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[PATCH 02/14] ARM: clk: sunxi: Add driver for the H3 THS clock

[megous]

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

[PATCH 03/14] thermal: Add support for sun8i THS on Allwinner H3

[megous]

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>
---
 drivers/thermal/Kconfig     |   7 ++
 drivers/thermal/Makefile    |   1 +
 drivers/thermal/sun8i_ths.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 303 insertions(+)
 create mode 100644 drivers/thermal/sun8i_ths.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 2d702ca..3de0f8d 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 "sun8i THS driver"
+	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..618ccc3
--- /dev/null
+++ b/drivers/thermal/sun8i_ths.c
@@ -0,0 +1,295 @@
+/*
+ * sun8i THS driver
+ *
+ * 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/nvmem-consumer.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_OFFS   0
+#define THS_H3_CTRL0_SENSOR_ACQ0(x) \
+        ((x) << THS_H3_CTRL0_SENSOR_ACQ0_OFFS)
+#define THS_H3_CTRL2_SENSE_EN_OFFS      0
+#define THS_H3_CTRL2_SENSE_EN \
+        BIT(THS_H3_CTRL2_SENSE_EN_OFFS)
+#define THS_H3_CTRL2_SENSOR_ACQ1_OFFS   16
+#define THS_H3_CTRL2_SENSOR_ACQ1(x) \
+        ((x) << THS_H3_CTRL2_SENSOR_ACQ1_OFFS)
+
+#define THS_H3_INT_CTRL_DATA_IRQ_EN_OFFS        8
+#define THS_H3_INT_CTRL_DATA_IRQ_EN \
+		BIT(THS_H3_INT_CTRL_DATA_IRQ_EN_OFFS)
+#define THS_H3_INT_CTRL_THERMAL_PER_OFFS        12
+#define THS_H3_INT_CTRL_THERMAL_PER(x) \
+		((x) << THS_H3_INT_CTRL_THERMAL_PER_OFFS)
+
+#define THS_H3_STAT_DATA_IRQ_STS_OFFS   8
+#define THS_H3_STAT_DATA_IRQ_STS \
+        BIT(THS_H3_STAT_DATA_IRQ_STS_OFFS)
+
+#define THS_H3_FILTER_TYPE_OFFS 0
+#define THS_H3_FILTER_TYPE(x) \
+        ((x) << THS_H3_FILTER_TYPE_OFFS)
+#define THS_H3_FILTER_EN_OFFS   2
+#define THS_H3_FILTER_EN \
+        BIT(THS_H3_FILTER_EN_OFFS)
+
+#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;
+	struct nvmem_cell *calcell;
+	struct platform_device *pdev;
+	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 - (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 int sun8i_ths_h3_init(struct platform_device *pdev,
+			     struct sun8i_ths_data *data)
+{
+	int ret;
+	size_t callen;
+	s32 *caldata;
+
+	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;
+	}
+
+	if (data->calcell) {
+		caldata = nvmem_cell_read(data->calcell, &callen);
+		if (IS_ERR(caldata))
+			return PTR_ERR(caldata);
+
+		writel(be32_to_cpu(*caldata), data->regs + THS_H3_CDATA);
+		kfree(caldata);
+	}
+
+	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 = reset_control_deassert(data->reset);
+	if (ret) {
+		dev_err(&pdev->dev, "reset deassert failed: %d\n", ret);
+		goto err_disable_ths;
+	}
+
+	ret = clk_set_rate(data->clk, THS_H3_CLK_IN);
+	if (ret)
+		goto err_disable_ths;
+
+	writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE),
+		data->regs + THS_H3_CTRL0);
+	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);
+	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);
+
+	return 0;
+
+err_disable_ths:
+	clk_disable_unprepare(data->clk);
+err_disable_bus:
+	clk_disable_unprepare(data->busclk);
+
+	return ret;
+}
+
+static void sun8i_ths_h3_deinit(struct sun8i_ths_data *data)
+{
+	reset_control_assert(data->reset);
+	clk_disable_unprepare(data->clk);
+	clk_disable_unprepare(data->busclk);
+}
+
+static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = {
+	.get_temp = sun8i_ths_get_temp,
+};
+
+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 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;
+
+	data->pdev = pdev;
+
+	data->calcell = devm_nvmem_cell_get(&pdev->dev, "calibration");
+	if (IS_ERR(data->calcell)) {
+		if (PTR_ERR(data->calcell) == -EPROBE_DEFER)
+			return PTR_ERR(data->calcell);
+
+		data->calcell = NULL; /* No calibration data */
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	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;
+	}
+
+	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;
+
+	ret = sun8i_ths_h3_init(pdev, data);
+	if (ret)
+		return ret;
+
+	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_deinit;
+	}
+
+	platform_set_drvdata(pdev, data);
+	return 0;
+
+err_deinit:
+	sun8i_ths_h3_deinit(data);
+	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);
+	sun8i_ths_h3_deinit(data);
+	return 0;
+}
+
+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("sun8i THS driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.0

Re: [PATCH 03/14] thermal: Add support for sun8i THS on Allwinner H3

[Chen-Yu Tsai]

Hi,

On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> From: Ondrej Jirman <megous@megous.com>
>

The subject could read:

  thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3

> This patch adds support for the sun8i thermal sensor on
> Allwinner H3 SoC.
>
> Signed-off-by: Ondřej Jirman <megous@megous.com>
> ---
>  drivers/thermal/Kconfig     |   7 ++
>  drivers/thermal/Makefile    |   1 +
>  drivers/thermal/sun8i_ths.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 303 insertions(+)
>  create mode 100644 drivers/thermal/sun8i_ths.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 2d702ca..3de0f8d 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 "sun8i THS driver"

Explain THS.

> +       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..618ccc3
> --- /dev/null
> +++ b/drivers/thermal/sun8i_ths.c
> @@ -0,0 +1,295 @@
> +/*
> + * sun8i THS driver

Explain THS.

> + *
> + * 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/nvmem-consumer.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_OFFS   0
> +#define THS_H3_CTRL0_SENSOR_ACQ0(x) \
> +        ((x) << THS_H3_CTRL0_SENSOR_ACQ0_OFFS)
> +#define THS_H3_CTRL2_SENSE_EN_OFFS      0
> +#define THS_H3_CTRL2_SENSE_EN \
> +        BIT(THS_H3_CTRL2_SENSE_EN_OFFS)
> +#define THS_H3_CTRL2_SENSOR_ACQ1_OFFS   16
> +#define THS_H3_CTRL2_SENSOR_ACQ1(x) \
> +        ((x) << THS_H3_CTRL2_SENSOR_ACQ1_OFFS)
> +
> +#define THS_H3_INT_CTRL_DATA_IRQ_EN_OFFS        8
> +#define THS_H3_INT_CTRL_DATA_IRQ_EN \
> +               BIT(THS_H3_INT_CTRL_DATA_IRQ_EN_OFFS)
> +#define THS_H3_INT_CTRL_THERMAL_PER_OFFS        12
> +#define THS_H3_INT_CTRL_THERMAL_PER(x) \
> +               ((x) << THS_H3_INT_CTRL_THERMAL_PER_OFFS)
> +
> +#define THS_H3_STAT_DATA_IRQ_STS_OFFS   8
> +#define THS_H3_STAT_DATA_IRQ_STS \
> +        BIT(THS_H3_STAT_DATA_IRQ_STS_OFFS)
> +
> +#define THS_H3_FILTER_TYPE_OFFS 0
> +#define THS_H3_FILTER_TYPE(x) \
> +        ((x) << THS_H3_FILTER_TYPE_OFFS)
> +#define THS_H3_FILTER_EN_OFFS   2
> +#define THS_H3_FILTER_EN \
> +        BIT(THS_H3_FILTER_EN_OFFS)

Is it really necessary to split the lines of all the macros?
It makes it harder to find and read stuff.

You're also not using any of the *_OFFS macros in the actual code,
so just drop them.

> +
> +#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;
> +       struct nvmem_cell *calcell;
> +       struct platform_device *pdev;
> +       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 - (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 int sun8i_ths_h3_init(struct platform_device *pdev,
> +                            struct sun8i_ths_data *data)
> +{
> +       int ret;
> +       size_t callen;
> +       s32 *caldata;
> +
> +       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");

IIRC with the new shared reset control stuff merged, you are supposed
to specify whether you want a shared or exclusive one when you ask for
it.

Also you seem to be requesting some resources here, while others
directly in the probe function. Could you put them in the same place?
Maybe requesting all resources in the probe function, and this init
function turns everything on?

> +       if (IS_ERR(data->reset)) {
> +               ret = PTR_ERR(data->reset);
> +               dev_err(&pdev->dev, "failed to get reset: %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (data->calcell) {
> +               caldata = nvmem_cell_read(data->calcell, &callen);
> +               if (IS_ERR(caldata))
> +                       return PTR_ERR(caldata);

Check the returned length in case of a bogus cell?

> +
> +               writel(be32_to_cpu(*caldata), data->regs + THS_H3_CDATA);
> +               kfree(caldata);
> +       }
> +
> +       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 = reset_control_deassert(data->reset);
> +       if (ret) {
> +               dev_err(&pdev->dev, "reset deassert failed: %d\n", ret);
> +               goto err_disable_ths;
> +       }
> +
> +       ret = clk_set_rate(data->clk, THS_H3_CLK_IN);
> +       if (ret)
> +               goto err_disable_ths;
> +
> +       writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE),
> +               data->regs + THS_H3_CTRL0);
> +       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);

This enables the interrupts. You already requested IRQs from the kernel, which
means as soon as this line executes, interrupts will start firing.

You should do this last, after you've finishing all the configuration,
i.e. move this after the next writel calls.

> +       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);
> +
> +       return 0;
> +
> +err_disable_ths:
> +       clk_disable_unprepare(data->clk);
> +err_disable_bus:
> +       clk_disable_unprepare(data->busclk);
> +
> +       return ret;
> +}
> +
> +static void sun8i_ths_h3_deinit(struct sun8i_ths_data *data)
> +{
> +       reset_control_assert(data->reset);
> +       clk_disable_unprepare(data->clk);
> +       clk_disable_unprepare(data->busclk);
> +}
> +
> +static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = {
> +       .get_temp = sun8i_ths_get_temp,
> +};
> +
> +static const struct of_device_id sun8i_ths_id_table[] = {
> +       {
> +               .compatible = "allwinner,sun8i-h3-ths",
> +       },

Nit. You could fit it in one line.

> +       { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_ths_id_table);
> +
> +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;
> +
> +       data->pdev = pdev;
> +
> +       data->calcell = devm_nvmem_cell_get(&pdev->dev, "calibration");
> +       if (IS_ERR(data->calcell)) {
> +               if (PTR_ERR(data->calcell) == -EPROBE_DEFER)
> +                       return PTR_ERR(data->calcell);
> +
> +               data->calcell = NULL; /* No calibration data */
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       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;
> +       }
> +
> +       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);

Is a threaded irq required? The thermal core seems to use atomics,
so this shouldn't be necessary.

> +       if (ret)
> +               return ret;
> +
> +       ret = sun8i_ths_h3_init(pdev, data);
> +       if (ret)
> +               return ret;
> +
> +       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_deinit;
> +       }
> +
> +       platform_set_drvdata(pdev, data);
> +       return 0;
> +
> +err_deinit:
> +       sun8i_ths_h3_deinit(data);
> +       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);
> +       sun8i_ths_h3_deinit(data);
> +       return 0;
> +}
> +
> +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("sun8i THS driver");

Explain THS.

Regards
ChenYu

> +MODULE_LICENSE("GPL v2");
> --
> 2.9.0
>

Re: [PATCH 03/14] thermal: Add support for sun8i THS on Allwinner H3

[Ondřej Jirman]

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

Thanks, I've incorporated all the suggestions (and more :)), except for
the threaded IRQ, which si expalined below.

regards,
  Ondrej

On 24.6.2016 05:09, Chen-Yu Tsai wrote:
> Hi,
> 
> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>> From: Ondrej Jirman <megous@megous.com>
>>
> 
> The subject could read:
> 
>   thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3
> 
>> This patch adds support for the sun8i thermal sensor on
>> Allwinner H3 SoC.
>>
>> Signed-off-by: Ondřej Jirman <megous@megous.com>
>> ---
>>  drivers/thermal/Kconfig     |   7 ++
>>  drivers/thermal/Makefile    |   1 +
>>  drivers/thermal/sun8i_ths.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 303 insertions(+)
>>  create mode 100644 drivers/thermal/sun8i_ths.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 2d702ca..3de0f8d 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 "sun8i THS driver"
> 
> Explain THS.
> 
>> +       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..618ccc3
>> --- /dev/null
>> +++ b/drivers/thermal/sun8i_ths.c
>> @@ -0,0 +1,295 @@
>> +/*
>> + * sun8i THS driver
> 
> Explain THS.
> 
>> + *
>> + * 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/nvmem-consumer.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_OFFS   0
>> +#define THS_H3_CTRL0_SENSOR_ACQ0(x) \
>> +        ((x) << THS_H3_CTRL0_SENSOR_ACQ0_OFFS)
>> +#define THS_H3_CTRL2_SENSE_EN_OFFS      0
>> +#define THS_H3_CTRL2_SENSE_EN \
>> +        BIT(THS_H3_CTRL2_SENSE_EN_OFFS)
>> +#define THS_H3_CTRL2_SENSOR_ACQ1_OFFS   16
>> +#define THS_H3_CTRL2_SENSOR_ACQ1(x) \
>> +        ((x) << THS_H3_CTRL2_SENSOR_ACQ1_OFFS)
>> +
>> +#define THS_H3_INT_CTRL_DATA_IRQ_EN_OFFS        8
>> +#define THS_H3_INT_CTRL_DATA_IRQ_EN \
>> +               BIT(THS_H3_INT_CTRL_DATA_IRQ_EN_OFFS)
>> +#define THS_H3_INT_CTRL_THERMAL_PER_OFFS        12
>> +#define THS_H3_INT_CTRL_THERMAL_PER(x) \
>> +               ((x) << THS_H3_INT_CTRL_THERMAL_PER_OFFS)
>> +
>> +#define THS_H3_STAT_DATA_IRQ_STS_OFFS   8
>> +#define THS_H3_STAT_DATA_IRQ_STS \
>> +        BIT(THS_H3_STAT_DATA_IRQ_STS_OFFS)
>> +
>> +#define THS_H3_FILTER_TYPE_OFFS 0
>> +#define THS_H3_FILTER_TYPE(x) \
>> +        ((x) << THS_H3_FILTER_TYPE_OFFS)
>> +#define THS_H3_FILTER_EN_OFFS   2
>> +#define THS_H3_FILTER_EN \
>> +        BIT(THS_H3_FILTER_EN_OFFS)
> 
> Is it really necessary to split the lines of all the macros?
> It makes it harder to find and read stuff.
> 
> You're also not using any of the *_OFFS macros in the actual code,
> so just drop them.
> 
>> +
>> +#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;
>> +       struct nvmem_cell *calcell;
>> +       struct platform_device *pdev;
>> +       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 - (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 int sun8i_ths_h3_init(struct platform_device *pdev,
>> +                            struct sun8i_ths_data *data)
>> +{
>> +       int ret;
>> +       size_t callen;
>> +       s32 *caldata;
>> +
>> +       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");
> 
> IIRC with the new shared reset control stuff merged, you are supposed
> to specify whether you want a shared or exclusive one when you ask for
> it.
> 
> Also you seem to be requesting some resources here, while others
> directly in the probe function. Could you put them in the same place?
> Maybe requesting all resources in the probe function, and this init
> function turns everything on?
> 
>> +       if (IS_ERR(data->reset)) {
>> +               ret = PTR_ERR(data->reset);
>> +               dev_err(&pdev->dev, "failed to get reset: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       if (data->calcell) {
>> +               caldata = nvmem_cell_read(data->calcell, &callen);
>> +               if (IS_ERR(caldata))
>> +                       return PTR_ERR(caldata);
> 
> Check the returned length in case of a bogus cell?
> 
>> +
>> +               writel(be32_to_cpu(*caldata), data->regs + THS_H3_CDATA);
>> +               kfree(caldata);
>> +       }
>> +
>> +       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 = reset_control_deassert(data->reset);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "reset deassert failed: %d\n", ret);
>> +               goto err_disable_ths;
>> +       }
>> +
>> +       ret = clk_set_rate(data->clk, THS_H3_CLK_IN);
>> +       if (ret)
>> +               goto err_disable_ths;
>> +
>> +       writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE),
>> +               data->regs + THS_H3_CTRL0);
>> +       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);
> 
> This enables the interrupts. You already requested IRQs from the kernel, which
> means as soon as this line executes, interrupts will start firing.
> 
> You should do this last, after you've finishing all the configuration,
> i.e. move this after the next writel calls.
> 
>> +       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);
>> +
>> +       return 0;
>> +
>> +err_disable_ths:
>> +       clk_disable_unprepare(data->clk);
>> +err_disable_bus:
>> +       clk_disable_unprepare(data->busclk);
>> +
>> +       return ret;
>> +}
>> +
>> +static void sun8i_ths_h3_deinit(struct sun8i_ths_data *data)
>> +{
>> +       reset_control_assert(data->reset);
>> +       clk_disable_unprepare(data->clk);
>> +       clk_disable_unprepare(data->busclk);
>> +}
>> +
>> +static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = {
>> +       .get_temp = sun8i_ths_get_temp,
>> +};
>> +
>> +static const struct of_device_id sun8i_ths_id_table[] = {
>> +       {
>> +               .compatible = "allwinner,sun8i-h3-ths",
>> +       },
> 
> Nit. You could fit it in one line.
> 
>> +       { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, sun8i_ths_id_table);
>> +
>> +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;
>> +
>> +       data->pdev = pdev;
>> +
>> +       data->calcell = devm_nvmem_cell_get(&pdev->dev, "calibration");
>> +       if (IS_ERR(data->calcell)) {
>> +               if (PTR_ERR(data->calcell) == -EPROBE_DEFER)
>> +                       return PTR_ERR(data->calcell);
>> +
>> +               data->calcell = NULL; /* No calibration data */
>> +       }
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       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;
>> +       }
>> +
>> +       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);
> 
> Is a threaded irq required? The thermal core seems to use atomics,
> so this shouldn't be necessary.

As I understand it, thermal_zone_device_update(data->tzd) can do quite a
lot of work and all other thermal drivers defer this call from the hard
irq handler. So, yes, it is necessary.

>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = sun8i_ths_h3_init(pdev, data);
>> +       if (ret)
>> +               return ret;
>> +
>> +       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_deinit;
>> +       }
>> +
>> +       platform_set_drvdata(pdev, data);
>> +       return 0;
>> +
>> +err_deinit:
>> +       sun8i_ths_h3_deinit(data);
>> +       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);
>> +       sun8i_ths_h3_deinit(data);
>> +       return 0;
>> +}
>> +
>> +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("sun8i THS driver");
> 
> Explain THS.
> 
> Regards
> ChenYu
> 
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.9.0
>>


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

Re: [PATCH 03/14] thermal: Add support for sun8i THS on Allwinner H3

[Ondřej Jirman]

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

On 24.6.2016 05:09, Chen-Yu Tsai wrote:
>> +static int sun8i_ths_h3_init(struct platform_device *pdev,
>> +                            struct sun8i_ths_data *data)
>> +{
>> +       int ret;
>> +       size_t callen;
>> +       s32 *caldata;
>> +
>> +       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");
> 
> IIRC with the new shared reset control stuff merged, you are supposed
> to specify whether you want a shared or exclusive one when you ask for
> it.

Here devm_reset_control_get will get the exclusive reference. So this
should be ok.

regards,
  Ondrej

> 
> Regards
> ChenYu
> 
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.9.0
>>


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

Re: [PATCH 03/14] thermal: Add support for sun8i THS on Allwinner H3

[Chen-Yu Tsai]

On Sat, Jun 25, 2016 at 8:35 AM, Ondřej Jirman <megous@megous.com> wrote:
> On 24.6.2016 05:09, Chen-Yu Tsai wrote:
>>> +static int sun8i_ths_h3_init(struct platform_device *pdev,
>>> +                            struct sun8i_ths_data *data)
>>> +{
>>> +       int ret;
>>> +       size_t callen;
>>> +       s32 *caldata;
>>> +
>>> +       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");
>>
>> IIRC with the new shared reset control stuff merged, you are supposed
>> to specify whether you want a shared or exclusive one when you ask for
>> it.
>
> Here devm_reset_control_get will get the exclusive reference. So this
> should be ok.

See https://patchwork.kernel.org/patch/9158691/

The generic ones might be removed later on. I remember in another thread
it was asked that new users should use the explicit API, and avoid having
to be converted.

ChenYu

>
> regards,
>   Ondrej
>
>>
>> Regards
>> ChenYu
>>
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> 2.9.0
>>>
>

Re: [PATCH 03/14] thermal: Add support for sun8i THS on Allwinner H3

[Ondřej Jirman]

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

On 25.6.2016 02:54, Chen-Yu Tsai wrote:
> On Sat, Jun 25, 2016 at 8:35 AM, Ondřej Jirman <megous@megous.com> wrote:
>> On 24.6.2016 05:09, Chen-Yu Tsai wrote:
>>>> +static int sun8i_ths_h3_init(struct platform_device *pdev,
>>>> +                            struct sun8i_ths_data *data)
>>>> +{
>>>> +       int ret;
>>>> +       size_t callen;
>>>> +       s32 *caldata;
>>>> +
>>>> +       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");
>>>
>>> IIRC with the new shared reset control stuff merged, you are supposed
>>> to specify whether you want a shared or exclusive one when you ask for
>>> it.
>>
>> Here devm_reset_control_get will get the exclusive reference. So this
>> should be ok.
> 
> See https://patchwork.kernel.org/patch/9158691/
> 
> The generic ones might be removed later on. I remember in another thread
> it was asked that new users should use the explicit API, and avoid having
> to be converted.

I see, thank you, I'll change that.

regards,
  Ondrej

> ChenYu
> 
>>
>> regards,
>>   Ondrej
>>
>>>
>>> Regards
>>> ChenYu
>>>
>>>> +MODULE_LICENSE("GPL v2");
>>>> --
>>>> 2.9.0
>>>>
>>


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

[PATCH 04/14] dt-bindings: document sun8i_ths

[megous]

From: Ondrej Jirman <megous@megous.com>

This patch adds the binding documentation for the sun8i_ths driver

Signed-off-by: Ondřej Jirman <megous@megous.com>
---
 .../devicetree/bindings/thermal/sun8i-ths.txt      | 31 ++++++++++++++++++++++
 1 file changed, 31 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..826cd57
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/sun8i-ths.txt
@@ -0,0 +1,31 @@
+* sun8i THS
+
+Required properties:
+- compatible : "allwinner,sun8i-h3-ths"
+- reg : Address range of the thermal registers and location of the calibration
+        value
+- resets : Must contain an entry for each entry in reset-names.
+           see ../reset/reset.txt for details
+- reset-names : Must include the name "ahb"
+- clocks : Must contain an entry for each entry in clock-names.
+- clock-names : Must contain "ahb" for the bus gate and "ths" for the THS
+  clock
+
+Optional properties:
+- nvmem-cells : Must contain an entry for each entry in nvmem-cell-names
+- nvmem-cell-names : Must contain "calibration" for the cell containing the
+  temperature calibration cell, if available
+
+Example:
+ths: ths@01c25000 {
+	#thermal-sensor-cells = <0>;
+	compatible = "allwinner,sun8i-h3-ths";
+	reg = <0x01c25000 0x400>;
+	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";
+	nvmem-cells = <&ths_calibration>;
+	nvmem-cell-names = "calibration";
+};
-- 
2.9.0

Re: [PATCH 04/14] dt-bindings: document sun8i_ths

[Chen-Yu Tsai]

On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> This patch adds the binding documentation for the sun8i_ths driver
>
> Signed-off-by: Ondřej Jirman <megous@megous.com>
> ---
>  .../devicetree/bindings/thermal/sun8i-ths.txt      | 31 ++++++++++++++++++++++
>  1 file changed, 31 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..826cd57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/sun8i-ths.txt
> @@ -0,0 +1,31 @@
> +* sun8i THS

An explanation of the acronym would be nice, both in the docs,
and in the commit message.

> +
> +Required properties:
> +- compatible : "allwinner,sun8i-h3-ths"
> +- reg : Address range of the thermal registers and location of the calibration

                                        *sensor*

Also you only specify one address range in the example. The "location
of the calibration value" is handled by the nvram-* properties. Please
remove the description.

> +        value
> +- resets : Must contain an entry for each entry in reset-names.

This, and clocks below, should probably read "must contain phandles
to reset/clock controls matching the entries of the names".

Regards
ChenYu

> +           see ../reset/reset.txt for details
> +- reset-names : Must include the name "ahb"
> +- clocks : Must contain an entry for each entry in clock-names.
> +- clock-names : Must contain "ahb" for the bus gate and "ths" for the THS
> +  clock
> +
> +Optional properties:
> +- nvmem-cells : Must contain an entry for each entry in nvmem-cell-names
> +- nvmem-cell-names : Must contain "calibration" for the cell containing the
> +  temperature calibration cell, if available
> +
> +Example:
> +ths: ths@01c25000 {
> +       #thermal-sensor-cells = <0>;
> +       compatible = "allwinner,sun8i-h3-ths";
> +       reg = <0x01c25000 0x400>;
> +       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";
> +       nvmem-cells = <&ths_calibration>;
> +       nvmem-cell-names = "calibration";
> +};
> --
> 2.9.0
>

[PATCH 05/14] ARM: dts: sun8i: Add THS node to the sun8i-h3.dtsi

[megous]

From: Ondrej Jirman <megous@megous.com>

This patch adds nodes for the THS 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 | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 172576d..9938972 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@01c20074 {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun8i-h3-ths-clk";
+			reg = <0x01c20074 0x4>;
+			clocks = <&osc24M>;
+			clock-output-names = "ths";
+		};
+
 		mmc0_clk: clk@01c20088 {
 			#clock-cells = <1>;
 			compatible = "allwinner,sun4i-a10-mmc-clk";
@@ -394,6 +410,10 @@
 			reg = <0x01c14000 0x400>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			ths_calibration: calib@234 {
+				reg = <0x234 0x4>;
+			};
 		};
 
 		usbphy: phy@01c19400 {
@@ -581,6 +601,19 @@
 			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		ths: ths@01c25000 {
+			#thermal-sensor-cells = <0>;
+			compatible = "allwinner,sun8i-h3-ths";
+			reg = <0x01c25000 0x400>;
+			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";
+			nvmem-cells = <&ths_calibration>;
+			nvmem-cell-names = "calibration";
+		};
+
 		uart0: serial@01c28000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28000 0x400>;
-- 
2.9.0

[PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[megous]

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>
---
 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 9938972..82faefc 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -52,7 +52,7 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		cpu@0 {
+		cpu0: cpu@0 {
 			compatible = "arm,cortex-a7";
 			device_type = "cpu";
 			reg = <0>;
-- 
2.9.0

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Chen-Yu Tsai]

On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> 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>
> ---
>  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 9938972..82faefc 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -52,7 +52,7 @@
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
> -               cpu@0 {
> +               cpu0: cpu@0 {
>                         compatible = "arm,cortex-a7";
>                         device_type = "cpu";
>                         reg = <0>;

Can you also set the cpu clock here? It is part of the SoC
and does not belong in the board DTS files.

Otherwise this one looks good.

ChenYu

> --
> 2.9.0
>

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Ondřej Jirman]

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

Hello,

comments below.

On 24.6.2016 05:48, Chen-Yu Tsai wrote:
> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>> 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>
>> ---
>>  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 9938972..82faefc 100644
>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>> @@ -52,7 +52,7 @@
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>> -               cpu@0 {
>> +               cpu0: cpu@0 {
>>                         compatible = "arm,cortex-a7";
>>                         device_type = "cpu";
>>                         reg = <0>;
> 
> Can you also set the cpu clock here? It is part of the SoC
> and does not belong in the board DTS files.

Do you mean operating-points, or something else? Different SBCs will
probably require different combinations of operating points just for
safety's sake, because they have different regulators and [some have
botched] thermal designs, so it might make sense to customize it for
differnt boards, and I don't feel adventurous enough setting it for all
H3 boards out there.

Or is this comment related to the missing cpu clock rate message I see
on every boot?

[    0.058912] /cpus/cpu@0 missing clock-frequency property

regards,
  Ondrej

> Otherwise this one looks good.
> 
> ChenYu
> 
>> --
>> 2.9.0
>>


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

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Chen-Yu Tsai]

On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
> Hello,
>
> comments below.
>
> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>> 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>
>>> ---
>>>  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 9938972..82faefc 100644
>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>>> @@ -52,7 +52,7 @@
>>>                 #address-cells = <1>;
>>>                 #size-cells = <0>;
>>>
>>> -               cpu@0 {
>>> +               cpu0: cpu@0 {
>>>                         compatible = "arm,cortex-a7";
>>>                         device_type = "cpu";
>>>                         reg = <0>;
>>
>> Can you also set the cpu clock here? It is part of the SoC
>> and does not belong in the board DTS files.
>
> Do you mean operating-points, or something else? Different SBCs will
> probably require different combinations of operating points just for
> safety's sake, because they have different regulators and [some have
> botched] thermal designs, so it might make sense to customize it for
> differnt boards, and I don't feel adventurous enough setting it for all
> H3 boards out there.

I meant clocks = <...> and clock-latency = <...>.

These 2 are part of the SoC.

The OPP can stay in the board files. It's a pity there's no standard
OPP table for H3 though. :(

ChenYu

>
> Or is this comment related to the missing cpu clock rate message I see
> on every boot?
>
> [    0.058912] /cpus/cpu@0 missing clock-frequency property
>
> regards,
>   Ondrej
>
>> Otherwise this one looks good.
>>
>> ChenYu
>>
>>> --
>>> 2.9.0
>>>
>

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Maxime Ripard]

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

On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
> > Hello,
> >
> > comments below.
> >
> > On 24.6.2016 05:48, Chen-Yu Tsai wrote:
> >> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> >>> 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>
> >>> ---
> >>>  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 9938972..82faefc 100644
> >>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> >>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> >>> @@ -52,7 +52,7 @@
> >>>                 #address-cells = <1>;
> >>>                 #size-cells = <0>;
> >>>
> >>> -               cpu@0 {
> >>> +               cpu0: cpu@0 {
> >>>                         compatible = "arm,cortex-a7";
> >>>                         device_type = "cpu";
> >>>                         reg = <0>;
> >>
> >> Can you also set the cpu clock here? It is part of the SoC
> >> and does not belong in the board DTS files.
> >
> > Do you mean operating-points, or something else? Different SBCs will
> > probably require different combinations of operating points just for
> > safety's sake, because they have different regulators and [some have
> > botched] thermal designs, so it might make sense to customize it for
> > differnt boards, and I don't feel adventurous enough setting it for all
> > H3 boards out there.
> 
> I meant clocks = <...> and clock-latency = <...>.
> 
> These 2 are part of the SoC.
> 
> The OPP can stay in the board files. It's a pity there's no standard
> OPP table for H3 though. :(

This has never been the case, and we always had some deviation in the
FEX files for all the SoCs.

If we could come up with standard OPPs that work for every one,
there's no reason it can't happen here.

I don't really see why the thermal design should change anything. If a
boards heats faster, it will throttle down to a lower OPP faster, but
those OPPs are not going to change.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Ondřej Jirman]

On 25.6.2016 09:02, Maxime Ripard wrote:
> On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
>> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
>>> Hello,
>>>
>>> comments below.
>>>
>>> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
>>>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>>>> 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>
>>>>> ---
>>>>>  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 9938972..82faefc 100644
>>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>> @@ -52,7 +52,7 @@
>>>>>                 #address-cells = <1>;
>>>>>                 #size-cells = <0>;
>>>>>
>>>>> -               cpu@0 {
>>>>> +               cpu0: cpu@0 {
>>>>>                         compatible = "arm,cortex-a7";
>>>>>                         device_type = "cpu";
>>>>>                         reg = <0>;
>>>>
>>>> Can you also set the cpu clock here? It is part of the SoC
>>>> and does not belong in the board DTS files.
>>>
>>> Do you mean operating-points, or something else? Different SBCs will
>>> probably require different combinations of operating points just for
>>> safety's sake, because they have different regulators and [some have
>>> botched] thermal designs, so it might make sense to customize it for
>>> differnt boards, and I don't feel adventurous enough setting it for all
>>> H3 boards out there.
>>
>> I meant clocks = <...> and clock-latency = <...>.
>>
>> These 2 are part of the SoC.
>>
>> The OPP can stay in the board files. It's a pity there's no standard
>> OPP table for H3 though. :(
> 
> This has never been the case, and we always had some deviation in the
> FEX files for all the SoCs.
> 
> If we could come up with standard OPPs that work for every one,
> there's no reason it can't happen here.
> 
> I don't really see why the thermal design should change anything. If a
> boards heats faster, it will throttle down to a lower OPP faster, but
> those OPPs are not going to change.

I've no way to test, but I've been told some Sinovoip boards are really
bad in this regard (SoC is not even well thermally connected to the
PCB/PCB not having copper layer to spread the heat). Thermal sensor
readings happen at fixed intervals, so the question is if you can heat
up the soc from say 80°C (first trip point) to over 110°C in less than
that period (330ms currently).

I say it shouldn't be a problem, if that small thing is drawing say 2W
at max load. It will burn or trigger a second trip point before the
first one has a chance to trigger and the kernel will shut down. I
remember tkaiser saying that he has to run that board at 240MHz max. But
perhaps I'm misremembering.

I'm just speculating.

regards,
  Ondrej



> Maxime
> 

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Maxime Ripard]

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

Hi,

On Sat, Jun 25, 2016 at 04:50:24PM +0200, Ondřej Jirman wrote:
> On 25.6.2016 09:02, Maxime Ripard wrote:
> > On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
> >> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
> >>> Hello,
> >>>
> >>> comments below.
> >>>
> >>> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
> >>>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> >>>>> 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>
> >>>>> ---
> >>>>>  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 9938972..82faefc 100644
> >>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> >>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> >>>>> @@ -52,7 +52,7 @@
> >>>>>                 #address-cells = <1>;
> >>>>>                 #size-cells = <0>;
> >>>>>
> >>>>> -               cpu@0 {
> >>>>> +               cpu0: cpu@0 {
> >>>>>                         compatible = "arm,cortex-a7";
> >>>>>                         device_type = "cpu";
> >>>>>                         reg = <0>;
> >>>>
> >>>> Can you also set the cpu clock here? It is part of the SoC
> >>>> and does not belong in the board DTS files.
> >>>
> >>> Do you mean operating-points, or something else? Different SBCs will
> >>> probably require different combinations of operating points just for
> >>> safety's sake, because they have different regulators and [some have
> >>> botched] thermal designs, so it might make sense to customize it for
> >>> differnt boards, and I don't feel adventurous enough setting it for all
> >>> H3 boards out there.
> >>
> >> I meant clocks = <...> and clock-latency = <...>.
> >>
> >> These 2 are part of the SoC.
> >>
> >> The OPP can stay in the board files. It's a pity there's no standard
> >> OPP table for H3 though. :(
> > 
> > This has never been the case, and we always had some deviation in the
> > FEX files for all the SoCs.
> > 
> > If we could come up with standard OPPs that work for every one,
> > there's no reason it can't happen here.
> > 
> > I don't really see why the thermal design should change anything. If a
> > boards heats faster, it will throttle down to a lower OPP faster, but
> > those OPPs are not going to change.
> 
> I've no way to test, but I've been told some Sinovoip boards are really
> bad in this regard (SoC is not even well thermally connected to the
> PCB/PCB not having copper layer to spread the heat). Thermal sensor
> readings happen at fixed intervals, so the question is if you can heat
> up the soc from say 80°C (first trip point) to over 110°C in less than
> that period (330ms currently).
> 
> I say it shouldn't be a problem, if that small thing is drawing say 2W
> at max load. It will burn or trigger a second trip point before the
> first one has a chance to trigger and the kernel will shut down. I
> remember tkaiser saying that he has to run that board at 240MHz max. But
> perhaps I'm misremembering.
> 
> I'm just speculating.

Yes, but that's just poor thermal design. What I was saying is that
even if we really need to throttle the SoC to 240 MHz on that board
because it heats too much, the couple of the frequency and the voltage
will likely be the same across all boards. It's just the amount of
time we'll spend using it that will differ.

But that's just my understanding, I might be speaking non-sense :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Ondřej Jirman]

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

On 29.6.2016 22:45, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Jun 25, 2016 at 04:50:24PM +0200, Ondřej Jirman wrote:
>> On 25.6.2016 09:02, Maxime Ripard wrote:
>>> On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
>>>> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
>>>>> Hello,
>>>>>
>>>>> comments below.
>>>>>
>>>>> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
>>>>>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>>>>>> 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>
>>>>>>> ---
>>>>>>>  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 9938972..82faefc 100644
>>>>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>>>> @@ -52,7 +52,7 @@
>>>>>>>                 #address-cells = <1>;
>>>>>>>                 #size-cells = <0>;
>>>>>>>
>>>>>>> -               cpu@0 {
>>>>>>> +               cpu0: cpu@0 {
>>>>>>>                         compatible = "arm,cortex-a7";
>>>>>>>                         device_type = "cpu";
>>>>>>>                         reg = <0>;
>>>>>>
>>>>>> Can you also set the cpu clock here? It is part of the SoC
>>>>>> and does not belong in the board DTS files.
>>>>>
>>>>> Do you mean operating-points, or something else? Different SBCs will
>>>>> probably require different combinations of operating points just for
>>>>> safety's sake, because they have different regulators and [some have
>>>>> botched] thermal designs, so it might make sense to customize it for
>>>>> differnt boards, and I don't feel adventurous enough setting it for all
>>>>> H3 boards out there.
>>>>
>>>> I meant clocks = <...> and clock-latency = <...>.
>>>>
>>>> These 2 are part of the SoC.
>>>>
>>>> The OPP can stay in the board files. It's a pity there's no standard
>>>> OPP table for H3 though. :(
>>>
>>> This has never been the case, and we always had some deviation in the
>>> FEX files for all the SoCs.
>>>
>>> If we could come up with standard OPPs that work for every one,
>>> there's no reason it can't happen here.
>>>
>>> I don't really see why the thermal design should change anything. If a
>>> boards heats faster, it will throttle down to a lower OPP faster, but
>>> those OPPs are not going to change.
>>
>> I've no way to test, but I've been told some Sinovoip boards are really
>> bad in this regard (SoC is not even well thermally connected to the
>> PCB/PCB not having copper layer to spread the heat). Thermal sensor
>> readings happen at fixed intervals, so the question is if you can heat
>> up the soc from say 80°C (first trip point) to over 110°C in less than
>> that period (330ms currently).
>>
>> I say it shouldn't be a problem, if that small thing is drawing say 2W
>> at max load. It will burn or trigger a second trip point before the
>> first one has a chance to trigger and the kernel will shut down. I
>> remember tkaiser saying that he has to run that board at 240MHz max. But
>> perhaps I'm misremembering.
>>
>> I'm just speculating.
> 
> Yes, but that's just poor thermal design. What I was saying is that
> even if we really need to throttle the SoC to 240 MHz on that board
> because it heats too much, the couple of the frequency and the voltage
> will likely be the same across all boards. It's just the amount of
> time we'll spend using it that will differ.
> 
> But that's just my understanding, I might be speaking non-sense :)
> 
> Maxime
> 

You're probably right. Operating points should be part of h3.dtsi, and
if some board is particularly bad, and can't handle being above certain
frequency safely, due to thermal design issues, we can override
operating points in its dts file.

regards,
  Ondrej


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

Re: [linux-sunxi] Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Michal Suchanek]

Hello,

On 29 June 2016 at 23:11, Ondřej Jirman <megous@megous.com> wrote:
> On 29.6.2016 22:45, Maxime Ripard wrote:
>> Hi,
>>
>> On Sat, Jun 25, 2016 at 04:50:24PM +0200, Ondřej Jirman wrote:
>>> On 25.6.2016 09:02, Maxime Ripard wrote:
>>>> On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
>>>>> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> comments below.
>>>>>>
>>>>>> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
>>>>>>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>>>>>>> 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>
>>>>>>>> ---
>>>>>>>>  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 9938972..82faefc 100644
>>>>>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>>>>> @@ -52,7 +52,7 @@
>>>>>>>>                 #address-cells = <1>;
>>>>>>>>                 #size-cells = <0>;
>>>>>>>>
>>>>>>>> -               cpu@0 {
>>>>>>>> +               cpu0: cpu@0 {
>>>>>>>>                         compatible = "arm,cortex-a7";
>>>>>>>>                         device_type = "cpu";
>>>>>>>>                         reg = <0>;
>>>>>>>
>>>>>>> Can you also set the cpu clock here? It is part of the SoC
>>>>>>> and does not belong in the board DTS files.
>>>>>>
>>>>>> Do you mean operating-points, or something else? Different SBCs will
>>>>>> probably require different combinations of operating points just for
>>>>>> safety's sake, because they have different regulators and [some have
>>>>>> botched] thermal designs, so it might make sense to customize it for
>>>>>> differnt boards, and I don't feel adventurous enough setting it for all
>>>>>> H3 boards out there.
>>>>>
>>>>> I meant clocks = <...> and clock-latency = <...>.
>>>>>
>>>>> These 2 are part of the SoC.
>>>>>
>>>>> The OPP can stay in the board files. It's a pity there's no standard
>>>>> OPP table for H3 though. :(
>>>>
>>>> This has never been the case, and we always had some deviation in the
>>>> FEX files for all the SoCs.
>>>>
>>>> If we could come up with standard OPPs that work for every one,
>>>> there's no reason it can't happen here.
>>>>
>>>> I don't really see why the thermal design should change anything. If a
>>>> boards heats faster, it will throttle down to a lower OPP faster, but
>>>> those OPPs are not going to change.
>>>
>>> I've no way to test, but I've been told some Sinovoip boards are really
>>> bad in this regard (SoC is not even well thermally connected to the
>>> PCB/PCB not having copper layer to spread the heat). Thermal sensor
>>> readings happen at fixed intervals, so the question is if you can heat
>>> up the soc from say 80°C (first trip point) to over 110°C in less than
>>> that period (330ms currently).
>>>
>>> I say it shouldn't be a problem, if that small thing is drawing say 2W
>>> at max load. It will burn or trigger a second trip point before the
>>> first one has a chance to trigger and the kernel will shut down. I
>>> remember tkaiser saying that he has to run that board at 240MHz max. But
>>> perhaps I'm misremembering.
>>>
>>> I'm just speculating.
>>
>> Yes, but that's just poor thermal design. What I was saying is that
>> even if we really need to throttle the SoC to 240 MHz on that board
>> because it heats too much, the couple of the frequency and the voltage
>> will likely be the same across all boards. It's just the amount of
>> time we'll spend using it that will differ.
>>
>> But that's just my understanding, I might be speaking non-sense :)
>>
>> Maxime
>>
>
> You're probably right. Operating points should be part of h3.dtsi, and
> if some board is particularly bad, and can't handle being above certain
> frequency safely, due to thermal design issues, we can override
> operating points in its dts file.
>

Can you override them?

AFAIK you cannot replace a property set in SoC file in a board file.
If you can keep the operating point list and just add option which
selects acceptable subset for the board that should work. On some
boards this subset would be determined by regulator voltage
constraints but not sure how you would select the subset for the
boards with thermal problems.

Thanks

Michal

Re: [linux-sunxi] Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Maxime Ripard]

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

On Thu, Jun 30, 2016 at 01:04:40PM +0200, Michal Suchanek wrote:
> > You're probably right. Operating points should be part of h3.dtsi, and
> > if some board is particularly bad, and can't handle being above certain
> > frequency safely, due to thermal design issues, we can override
> > operating points in its dts file.
> >
> 
> Can you override them?
> 
> AFAIK you cannot replace a property set in SoC file in a board file.

You totally can, we have litterally dozens of examples of that already.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Ondřej Jirman]

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



On 25.6.2016 09:02, Maxime Ripard wrote:
> On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
>> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
>>> Hello,
>>>
>>> comments below.
>>>
>>> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
>>>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>>>> 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>
>>>>> ---
>>>>>  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 9938972..82faefc 100644
>>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>>>>> @@ -52,7 +52,7 @@
>>>>>                 #address-cells = <1>;
>>>>>                 #size-cells = <0>;
>>>>>
>>>>> -               cpu@0 {
>>>>> +               cpu0: cpu@0 {
>>>>>                         compatible = "arm,cortex-a7";
>>>>>                         device_type = "cpu";
>>>>>                         reg = <0>;
>>>>
>>>> Can you also set the cpu clock here? It is part of the SoC
>>>> and does not belong in the board DTS files.
>>>
>>> Do you mean operating-points, or something else? Different SBCs will
>>> probably require different combinations of operating points just for
>>> safety's sake, because they have different regulators and [some have
>>> botched] thermal designs, so it might make sense to customize it for
>>> differnt boards, and I don't feel adventurous enough setting it for all
>>> H3 boards out there.
>>
>> I meant clocks = <...> and clock-latency = <...>.
>>
>> These 2 are part of the SoC.
>>
>> The OPP can stay in the board files. It's a pity there's no standard
>> OPP table for H3 though. :(
> 
> This has never been the case, and we always had some deviation in the
> FEX files for all the SoCs.
> 
> If we could come up with standard OPPs that work for every one,
> there's no reason it can't happen here.
> 
> I don't really see why the thermal design should change anything. If a
> boards heats faster, it will throttle down to a lower OPP faster, but
> those OPPs are not going to change.

So I tried, and found out that it will not be so easy. Different boards
have different regulators, and linux doesn't deal well with voltages
that are not supported by the regulator.

So even if the board can run at certain frequency if you round the
voltage to the next higher voltage supported by the regulator, opp
implementation doesn't do the rounding and just drops the operating
points that have no support in the voltage regulator.

We have boards that have 1.1/1.3V switching, only 1.3V, fine tuned
voltage regulation and every such board will need it's own set of
operating points.

I'd leave the OPP definitions in the board files for now.

cpu cpu0: _opp_add: OPP not supported by regulators (1368000000)
core: _opp_supported_by_regulators: OPP minuV: 1340000 maxuV: 1340000,
not supported by regulator
cpu cpu0: _opp_add: OPP not supported by regulators (1344000000)
core: _opp_supported_by_regulators: OPP minuV: 1340000 maxuV: 1340000,
not supported by regulator
cpu cpu0: _opp_add: OPP not supported by regulators (1296000000)
core: _opp_supported_by_regulators: OPP minuV: 1200000 maxuV: 1200000,
not supported by regulator
cpu cpu0: _opp_add: OPP not supported by regulators (1104000000)
core: _opp_supported_by_regulators: OPP minuV: 1140000 maxuV: 1140000,
not supported by regulator
cpu cpu0: _opp_add: OPP not supported by regulators (1008000000)

regards,
  o.

> Maxime
> 


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

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Maxime Ripard]

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

On Sun, Jul 17, 2016 at 04:39:27PM +0200, Ondřej Jirman wrote:
> 
> 
> On 25.6.2016 09:02, Maxime Ripard wrote:
> > On Sat, Jun 25, 2016 at 09:02:48AM +0800, Chen-Yu Tsai wrote:
> >> On Sat, Jun 25, 2016 at 6:51 AM, Ondřej Jirman <megous@megous.com> wrote:
> >>> Hello,
> >>>
> >>> comments below.
> >>>
> >>> On 24.6.2016 05:48, Chen-Yu Tsai wrote:
> >>>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> >>>>> 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>
> >>>>> ---
> >>>>>  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 9938972..82faefc 100644
> >>>>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> >>>>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> >>>>> @@ -52,7 +52,7 @@
> >>>>>                 #address-cells = <1>;
> >>>>>                 #size-cells = <0>;
> >>>>>
> >>>>> -               cpu@0 {
> >>>>> +               cpu0: cpu@0 {
> >>>>>                         compatible = "arm,cortex-a7";
> >>>>>                         device_type = "cpu";
> >>>>>                         reg = <0>;
> >>>>
> >>>> Can you also set the cpu clock here? It is part of the SoC
> >>>> and does not belong in the board DTS files.
> >>>
> >>> Do you mean operating-points, or something else? Different SBCs will
> >>> probably require different combinations of operating points just for
> >>> safety's sake, because they have different regulators and [some have
> >>> botched] thermal designs, so it might make sense to customize it for
> >>> differnt boards, and I don't feel adventurous enough setting it for all
> >>> H3 boards out there.
> >>
> >> I meant clocks = <...> and clock-latency = <...>.
> >>
> >> These 2 are part of the SoC.
> >>
> >> The OPP can stay in the board files. It's a pity there's no standard
> >> OPP table for H3 though. :(
> > 
> > This has never been the case, and we always had some deviation in the
> > FEX files for all the SoCs.
> > 
> > If we could come up with standard OPPs that work for every one,
> > there's no reason it can't happen here.
> > 
> > I don't really see why the thermal design should change anything. If a
> > boards heats faster, it will throttle down to a lower OPP faster, but
> > those OPPs are not going to change.
> 
> So I tried, and found out that it will not be so easy. Different boards
> have different regulators, and linux doesn't deal well with voltages
> that are not supported by the regulator.
> 
> So even if the board can run at certain frequency if you round the
> voltage to the next higher voltage supported by the regulator, opp
> implementation doesn't do the rounding and just drops the operating
> points that have no support in the voltage regulator.
> 
> We have boards that have 1.1/1.3V switching, only 1.3V, fine tuned
> voltage regulation and every such board will need it's own set of
> operating points.
> 
> I'd leave the OPP definitions in the board files for now.

Works for me.

Maxime


-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 06/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi

[Maxime Ripard]

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

On Tue, Jul 19, 2016 at 07:10:54AM -0700, Thomas Kaiser wrote:
> Hi,
> 
> Ondřej Jirman wrote:
> >
> > We have boards that have 1.1/1.3V switching, only 1.3V, fine tuned 
> > voltage regulation and every such board will need it's own set of 
> > operating points. 
> >
> 
> Yes, and Allwinner's current BSP kernel code might encourage board makers 
> to implement a forth variant: switching between 4 different voltages 
> through GPIOs.
> 
> Currently we have 4 boards that rely on the simple '2 voltage regulation' 
> all using 1.1V/1.3V: Orange Pi One and Lite and NanoPi M1 and NEO. Then 
> there are 2 devices with (legacy) Linux support existing that use no 
> voltage regulation at all: Banana Pi M2+ (according to schematic using 1.2V 
> but in reality it's 1.3V VDD_CPUX) and Beelink X2. And according to Tsvetan 
> if/when Olimex will release their 2 H3 boards we have two more with fixed 
> but yet unknown VDD_CPUX voltage (since olimex fears overheating maybe they 
> use 1.1V or 1.2V limiting max cpufreq to 816 or 1008 MHz). And all the 
> bigger H3 based Orange Pi use the SY8106A voltage regulator being able to 
> adjust VDD_CPUX in steps of 20mV allowing VDD_CPUX to exceed 1200 MHz (a 
> reasonable value seems to be 1296 MHz since above throttling will be an 
> issue without active cooling)

Ok, good to know. I'm not sure overclocking is ever a reasonable
solution, but that's a separate topic.

> Things get even worse since Xunlong uses copper layers inside the PCB to 
> spread the heat away from H3 so Orange Pi One/Lite do not overheat that 
> much like eg. NanoPi M1 (and maybe NEO -- can tell next week when I get dev 
> samples to play with). So while eg. Orange Pi One and NanoPi M1 switch 
> between the same voltages in the same way we (Armbian) found that we have 
> to allow M1 to downclock to even 240 MHz since when testing with legacy 
> kernel really heavy workloads led to throttling that low (even CPU cores 
> were killed at this low clockspeed -- same applies to BPi M2+ and Beelink 
> X2)

And that's what I really want to avoid. Even though that board
absolutely requires the 240MHz OPP to run properly, nothing prevents
from using that OPP on other boards as well, that will also benefit
from it. Thermal throttling is something that needs to be handled, but
power management is also something we should consider, and I see no
reason why not to have a consistent set of operating frequencies, even
though the voltage might differ depending on the regulator
capabilities.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 07/14] regulator: SY8106A regulator driver

[megous]

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>
---
 drivers/regulator/Kconfig             |   8 +-
 drivers/regulator/Makefile            |   2 +-
 drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 drivers/regulator/sy8106a-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 144cbf5..fc3fae2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -860,5 +860,11 @@ config REGULATOR_WM8994
 	  This driver provides support for the voltage regulators on the
 	  WM8994 CODEC.
 
-endif
+config REGULATOR_SY8106A
+	tristate "Silergy SY8106A"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This driver provides support for the voltage regulator SY8106A.
 
+endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 85a1d44..f382095 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
 obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
 obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
 obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
-
+obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-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..34bd69c
--- /dev/null
+++ b/drivers/regulator/sy8106a-regulator.c
@@ -0,0 +1,153 @@
+/*
+ * 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/init.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regmap.h>
+
+#define SY8106A_REG_VOUT1_SEL 		0x01
+#define SY8106A_REG_VOUT_COM 		0x02
+#define SY8106A_REG_VOUT1_SEL_MASK 	0x7f
+#define SY8106A_DISABLE_REG 		0x01
+
+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)
+{
+	return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+				  0xff, sel | 0x80);
+}
+
+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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
+	config.driver_data = chip;
+	config.regmap = chip->regmap;
+	config.of_node = dev->of_node;
+
+	/* Probe regulator */
+	error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
+	dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
+	if (error) {
+		dev_err(&i2c->dev, "Failed to read voltage at probe time: %d\n", error);
+		return error;
+	}
+
+	rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
+		return PTR_ERR(rdev);
+	}
+
+	chip->rdev = rdev;
+
+	i2c_set_clientdata(i2c, chip);
+
+	return 0;
+}
+
+static const struct i2c_device_id sy8106a_i2c_id[] = {
+	{"sy8106a", 0},
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);
+
+static struct i2c_driver sy8106a_regulator_driver = {
+	.driver = {
+		.name = "sy8106a",
+	},
+	.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

Re: [PATCH 07/14] regulator: SY8106A regulator driver

[Chen-Yu Tsai]

On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
> 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>
> ---
>  drivers/regulator/Kconfig             |   8 +-
>  drivers/regulator/Makefile            |   2 +-
>  drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
>  3 files changed, 161 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/regulator/sy8106a-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 144cbf5..fc3fae2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -860,5 +860,11 @@ config REGULATOR_WM8994
>           This driver provides support for the voltage regulators on the
>           WM8994 CODEC.
>
> -endif
> +config REGULATOR_SY8106A
> +       tristate "Silergy SY8106A"
> +       depends on I2C

Maybe you should also depend on OF since the driver is going to crippled
without any constraints set, or (OF || COMPILE_TEST) if you want some
compile test coverage.

> +       select REGMAP_I2C
> +       help
> +         This driver provides support for the voltage regulator SY8106A.
>
> +endif
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 85a1d44..f382095 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> -
> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o

Follow the existing ordering in the Makefile.

>
>  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..34bd69c
> --- /dev/null
> +++ b/drivers/regulator/sy8106a-regulator.c
> @@ -0,0 +1,153 @@
> +/*
> + * 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/init.h>

Do you need this one?

> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>

And this one?

> +#include <linux/regulator/of_regulator.h>
> +#include <linux/regmap.h>

Sort alphabetically please.

> +
> +#define SY8106A_REG_VOUT1_SEL          0x01
> +#define SY8106A_REG_VOUT_COM           0x02
> +#define SY8106A_REG_VOUT1_SEL_MASK     0x7f
> +#define SY8106A_DISABLE_REG            0x01

BIT(0) would be clearer.

> +
> +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)
> +{
> +       return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
> +                                 0xff, sel | 0x80);

Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?

> +}
> +
> +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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);

Check for possible failures?

> +       config.driver_data = chip;
> +       config.regmap = chip->regmap;
> +       config.of_node = dev->of_node;
> +
> +       /* Probe regulator */
> +       error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
> +       dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
> +       if (error) {
> +               dev_err(&i2c->dev, "Failed to read voltage at probe time: %d\n", error);
> +               return error;
> +       }
> +
> +       rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, &config);
> +       if (IS_ERR(rdev)) {
> +               dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
> +               return PTR_ERR(rdev);
> +       }
> +
> +       chip->rdev = rdev;
> +
> +       i2c_set_clientdata(i2c, chip);
> +
> +       return 0;
> +}
> +
> +static const struct i2c_device_id sy8106a_i2c_id[] = {
> +       {"sy8106a", 0},
> +       {},
> +};
> +

Extra line.

> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);

IMHO probing explicitly through DT (of_device_id) would be better.


Regards
ChenYu

> +
> +static struct i2c_driver sy8106a_regulator_driver = {
> +       .driver = {
> +               .name = "sy8106a",
> +       },
> +       .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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/14] regulator: SY8106A regulator driver

[Ondřej Jirman]

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

Hi,

thank you for the review. I've resolved most of the issues. Some more
comments below.

On 24.6.2016 05:41, Chen-Yu Tsai wrote:
> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>> 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>
>> ---
>>  drivers/regulator/Kconfig             |   8 +-
>>  drivers/regulator/Makefile            |   2 +-
>>  drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 161 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/regulator/sy8106a-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 144cbf5..fc3fae2 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -860,5 +860,11 @@ config REGULATOR_WM8994
>>           This driver provides support for the voltage regulators on the
>>           WM8994 CODEC.
>>
>> -endif
>> +config REGULATOR_SY8106A
>> +       tristate "Silergy SY8106A"
>> +       depends on I2C
> 
> Maybe you should also depend on OF since the driver is going to crippled
> without any constraints set, or (OF || COMPILE_TEST) if you want some
> compile test coverage.
> 
>> +       select REGMAP_I2C
>> +       help
>> +         This driver provides support for the voltage regulator SY8106A.
>>
>> +endif
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index 85a1d44..f382095 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>> -
>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
> 
> Follow the existing ordering in the Makefile.
> 
>>
>>  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..34bd69c
>> --- /dev/null
>> +++ b/drivers/regulator/sy8106a-regulator.c
>> @@ -0,0 +1,153 @@
>> +/*
>> + * 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/init.h>
> 
> Do you need this one?
> 
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
> 
> And this one?
> 
>> +#include <linux/regulator/of_regulator.h>
>> +#include <linux/regmap.h>
> 
> Sort alphabetically please.
> 
>> +
>> +#define SY8106A_REG_VOUT1_SEL          0x01
>> +#define SY8106A_REG_VOUT_COM           0x02
>> +#define SY8106A_REG_VOUT1_SEL_MASK     0x7f
>> +#define SY8106A_DISABLE_REG            0x01
> 
> BIT(0) would be clearer.
> 
>> +
>> +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)
>> +{
>> +       return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>> +                                 0xff, sel | 0x80);
> 
> Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?

I understand the code savings, but I'd rather avoid that, because it
would involve 2 needless readouts and rewrites of the voltage setting
register. I'd rather change this to regmap_write, so that the regulator
only receives writes over I2C, to minimize possibility of bit flip error
by minimizing the communication over the I2C bus.

>> +}
>> +
>> +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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
> 
> Check for possible failures?
> 
>> +       config.driver_data = chip;
>> +       config.regmap = chip->regmap;
>> +       config.of_node = dev->of_node;
>> +
>> +       /* Probe regulator */
>> +       error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
>> +       dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
>> +       if (error) {
>> +               dev_err(&i2c->dev, "Failed to read voltage at probe time: %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, &config);
>> +       if (IS_ERR(rdev)) {
>> +               dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
>> +               return PTR_ERR(rdev);
>> +       }
>> +
>> +       chip->rdev = rdev;
>> +
>> +       i2c_set_clientdata(i2c, chip);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct i2c_device_id sy8106a_i2c_id[] = {
>> +       {"sy8106a", 0},
>> +       {},
>> +};
>> +
> 
> Extra line.
> 
>> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);
> 
> IMHO probing explicitly through DT (of_device_id) would be better.

I checked out a few recently added i2c regulator drivers, and this is
the way they're written. I'd rather not differ, especially since I'm a
beginner with this thing, atm. Also, I'm not sure what chanhge in
particular you're suggesting.

thanks,
  Ondrej

> 
> Regards
> ChenYu
> 
>> +
>> +static struct i2c_driver sy8106a_regulator_driver = {
>> +       .driver = {
>> +               .name = "sy8106a",
>> +       },
>> +       .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
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


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

Re: [PATCH 07/14] regulator: SY8106A regulator driver

[Chen-Yu Tsai]

On Sat, Jun 25, 2016 at 8:11 AM, Ondřej Jirman <megous@megous.com> wrote:
> Hi,
>
> thank you for the review. I've resolved most of the issues. Some more
> comments below.
>
> On 24.6.2016 05:41, Chen-Yu Tsai wrote:
>> On Fri, Jun 24, 2016 at 3:20 AM,  <megous@megous.com> wrote:
>>> 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>
>>> ---
>>>  drivers/regulator/Kconfig             |   8 +-
>>>  drivers/regulator/Makefile            |   2 +-
>>>  drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 161 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/regulator/sy8106a-regulator.c
>>>
>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>>> index 144cbf5..fc3fae2 100644
>>> --- a/drivers/regulator/Kconfig
>>> +++ b/drivers/regulator/Kconfig
>>> @@ -860,5 +860,11 @@ config REGULATOR_WM8994
>>>           This driver provides support for the voltage regulators on the
>>>           WM8994 CODEC.
>>>
>>> -endif
>>> +config REGULATOR_SY8106A
>>> +       tristate "Silergy SY8106A"
>>> +       depends on I2C
>>
>> Maybe you should also depend on OF since the driver is going to crippled
>> without any constraints set, or (OF || COMPILE_TEST) if you want some
>> compile test coverage.
>>
>>> +       select REGMAP_I2C
>>> +       help
>>> +         This driver provides support for the voltage regulator SY8106A.
>>>
>>> +endif
>>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>>> index 85a1d44..f382095 100644
>>> --- a/drivers/regulator/Makefile
>>> +++ b/drivers/regulator/Makefile
>>> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>>>  obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>>>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>>>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>>> -
>>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
>>
>> Follow the existing ordering in the Makefile.
>>
>>>
>>>  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..34bd69c
>>> --- /dev/null
>>> +++ b/drivers/regulator/sy8106a-regulator.c
>>> @@ -0,0 +1,153 @@
>>> +/*
>>> + * 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/init.h>
>>
>> Do you need this one?
>>
>>> +#include <linux/regulator/driver.h>
>>> +#include <linux/regulator/machine.h>
>>
>> And this one?
>>
>>> +#include <linux/regulator/of_regulator.h>
>>> +#include <linux/regmap.h>
>>
>> Sort alphabetically please.
>>
>>> +
>>> +#define SY8106A_REG_VOUT1_SEL          0x01
>>> +#define SY8106A_REG_VOUT_COM           0x02
>>> +#define SY8106A_REG_VOUT1_SEL_MASK     0x7f
>>> +#define SY8106A_DISABLE_REG            0x01
>>
>> BIT(0) would be clearer.
>>
>>> +
>>> +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)
>>> +{
>>> +       return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>>> +                                 0xff, sel | 0x80);
>>
>> Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?
>
> I understand the code savings, but I'd rather avoid that, because it
> would involve 2 needless readouts and rewrites of the voltage setting
> register. I'd rather change this to regmap_write, so that the regulator
> only receives writes over I2C, to minimize possibility of bit flip error
> by minimizing the communication over the I2C bus.

OK. It's best to add a comment then, in case someone comes in and refactors it.

>
>>> +}
>>> +
>>> +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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
>>
>> Check for possible failures?
>>
>>> +       config.driver_data = chip;
>>> +       config.regmap = chip->regmap;
>>> +       config.of_node = dev->of_node;
>>> +
>>> +       /* Probe regulator */
>>> +       error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
>>> +       dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
>>> +       if (error) {
>>> +               dev_err(&i2c->dev, "Failed to read voltage at probe time: %d\n", error);
>>> +               return error;
>>> +       }
>>> +
>>> +       rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, &config);
>>> +       if (IS_ERR(rdev)) {
>>> +               dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
>>> +               return PTR_ERR(rdev);
>>> +       }
>>> +
>>> +       chip->rdev = rdev;
>>> +
>>> +       i2c_set_clientdata(i2c, chip);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id sy8106a_i2c_id[] = {
>>> +       {"sy8106a", 0},
>>> +       {},
>>> +};
>>> +
>>
>> Extra line.
>>
>>> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);
>>
>> IMHO probing explicitly through DT (of_device_id) would be better.
>
> I checked out a few recently added i2c regulator drivers, and this is
> the way they're written. I'd rather not differ, especially since I'm a
> beginner with this thing, atm. Also, I'm not sure what chanhge in
> particular you're suggesting.

See drivers/mfd/axp20x-i2c.c , the last 50 lines or so.

ChenYu

>
> thanks,
>   Ondrej
>
>>
>> Regards
>> ChenYu
>>
>>> +
>>> +static struct i2c_driver sy8106a_regulator_driver = {
>>> +       .driver = {
>>> +               .name = "sy8106a",
>>> +       },
>>> +       .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
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH 08/14] ARM: dts: sun8i: Add r_twi I2C controller

[megous]

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 82faefc..8d86f57 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -320,8 +320,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@01f01454 {
@@ -666,6 +667,20 @@
 			status = "disabled";
 		};
 
+		r_twi: i2c@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@01c81000 {
 			compatible = "arm,cortex-a7-gic", "arm,cortex-a15-gic";
 			reg = <0x01c81000 0x1000>,
@@ -717,6 +732,13 @@
 				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
 				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
 			};
+
+			r_twi_pins_a: r_twi@0 {
+				allwinner,pins = "PL0", "PL1";
+				allwinner,function = "s_twi";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
 		};
 	};
 };
-- 
2.9.0

[PATCH 09/14] ARM: dts: sun8i: Enable r_twi on Orange Pi PC

[megous]

From: Ondrej Jirman <megous@megous.com>

Enable I2C controller where the SY8106A regulator for
CPUX voltage is attached on Orange Pi PC SBC.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 4 ++++
 1 file changed, 4 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..e5991da 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
@@ -90,6 +90,10 @@
 	};
 };
 
+&r_twi {
+	status = "okay";
+};
+
 &ehci1 {
 	status = "okay";
 };
-- 
2.9.0

[PATCH 10/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC

[megous]

From: Ondrej Jirman <megous@megous.com>

Add sy8106a regulator to 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
index e5991da..7e04017 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
@@ -92,6 +92,16 @@
 
 &r_twi {
 	status = "okay";
+
+	vdd_cpu: regulator@65 {
+		compatible = "sy8106a";
+		reg = <0x65>;
+		regulator-min-microvolt = <1000000>;
+		regulator-max-microvolt = <1400000>;
+		regulator-ramp-delay = <200>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
 };
 
 &ehci1 {
-- 
2.9.0

Re: [PATCH 10/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC

[Chen-Yu Tsai]

On Fri, Jun 24, 2016 at 3:21 AM,  <megous@megous.com> wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> Add sy8106a regulator to 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 | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
> index e5991da..7e04017 100644
> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
> @@ -92,6 +92,16 @@
>
>  &r_twi {
>         status = "okay";
> +
> +       vdd_cpu: regulator@65 {
> +               compatible = "sy8106a";
> +               reg = <0x65>;
> +               regulator-min-microvolt = <1000000>;
> +               regulator-max-microvolt = <1400000>;
> +               regulator-ramp-delay = <200>;
> +               regulator-boot-on;
> +               regulator-always-on;
> +       };

This should be merged with the previous "enable r_twi" patch.

ChenYu

>  };
>
>  &ehci1 {
> --
> 2.9.0
>

[PATCH 11/14] ARM: sun8i: clk: Add clk-factor rate application method

[megous]

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>
---
 arch/arm/boot/dts/sun8i-h3.dtsi |  2 +-
 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(+), 22 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 8d86f57..58a49db 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -114,7 +114,7 @@
 
 		pll1: clk@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";
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

Re: [linux-sunxi] [PATCH 11/14] ARM: sun8i: clk: Add clk-factor rate application method

[Julian Calaby]

Hi Ondrej,

On Fri, Jun 24, 2016 at 5:21 AM,  <megous@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.
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi |  2 +-
>  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(+), 22 deletions(-)

Shouldn't the .dtsi changes be in a separate patch?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH 12/14] ARM: dts: sun8i: Setup CPU operating points for Onrage PI PC

[megous]

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 | 50 ++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
index 7e04017..e85fe91 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
@@ -90,6 +90,56 @@
 	};
 };
 
+&cpu0 {
+	clocks = <&cpu>;
+	clock-latency = <244144>; /* 8 32k periods */
+	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

[PATCH 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One

[megous]

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>
---
 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..ce4ba91 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";
+
+		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>;
 	};
+
+	soc_reg0: soc_reg@0 {
+		allwinner,pins = "PL6";
+		allwinner,function = "gpio_out";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+	};
 };
 
 &uart0 {
-- 
2.9.0

Re: [linux-sunxi] [PATCH 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One

[Julian Calaby]

Hi Ondrej,

On Fri, Jun 24, 2016 at 5:21 AM,  <megous@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>
> ---
>  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..ce4ba91 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";
> +
> +               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;

Don't you need to reference the new pinctl node in this one?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One

[Ondřej Jirman]

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

Hi Julian,

thank you for the review. You're right. I added the pinctrl client
nodes. Also the patches were split incorrectly, so I fixed that too.

regards,
  Ondrej

On 24.6.2016 04:51, Julian Calaby wrote:
> Hi Ondrej,
> 
> On Fri, Jun 24, 2016 at 5:21 AM,  <megous@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>
>> ---
>>  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..ce4ba91 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";
>> +
>> +               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;
> 
> Don't you need to reference the new pinctl node in this one?
> 
> Thanks,
> 


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

Re: [linux-sunxi] [PATCH 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One

[Julian Calaby]

Hi Ondrej,

On Fri, Jun 24, 2016 at 5:21 AM,  <megous@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>
> ---
>  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..ce4ba91 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";
> +
> +               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;
> +       };
> +};
> +

Also, isn't adding another closing bracket here a syntax error?

>  };

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

[PATCH 14/14] ARM: dts: sun8i: Enable DVFS on Orange Pi One

[megous]

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 ce4ba91..6bd4367 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts
@@ -107,6 +107,45 @@
 	};
 };
 
+&cpu0 {
+	clocks = <&cpu>;
+	clock-latency = <244144>; /* 8 32k periods */
+	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 {
-- 
2.9.0