Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 2/6] thermal: amlogic: Add thermal driver to support G12 SoCs
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Amlogic G12A and G12B SoCs integrate two thermal sensors
with the same design.
One is located close to the DDR controller and the other one is
located close to the PLLs (between the CPU and GPU).

The calibration data for each of the thermal sensors instance is
stored in a different location within the AO region.

Implement reading the temperature from each thermal sensor.

The IP block has more functionality, which may be added to this driver
in the future:
- chip reset when the temperature exceeds a configurable threshold
- up to four interrupts when the temperature has risen above a
configurable threshold
- up to four interrupts when the temperature has fallen below a
configurable threshold

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 drivers/thermal/Kconfig           |  11 +
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/amlogic_thermal.c | 335 ++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 drivers/thermal/amlogic_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 9966364a6deb..0f31bb4bc372 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -348,6 +348,17 @@ config MTK_THERMAL
 	  Enable this option if you want to have support for thermal management
 	  controller present in Mediatek SoCs
 
+config AMLOGIC_THERMAL
+	tristate "Amlogic Thermal Support"
+	default ARCH_MESON
+	depends on OF && ARCH_MESON
+	help
+	  If you say yes here you get support for Amlogic Thermal
+	  for G12 SoC Family.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called amlogic_thermal.
+
 menu "Intel thermal drivers"
 depends on X86 || X86_INTEL_QUARK || COMPILE_TEST
 source "drivers/thermal/intel/Kconfig"
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 74a37c7f847a..baeb70bf0568 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_MTK_THERMAL)	+= mtk_thermal.o
 obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
 obj-$(CONFIG_ZX2967_THERMAL)	+= zx2967_thermal.o
 obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
+obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
new file mode 100644
index 000000000000..173d808048d2
--- /dev/null
+++ b/drivers/thermal/amlogic_thermal.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic Thermal Sensor Driver
+ *
+ * Copyright (C) 2017 Huan Biao <huan.biao@amlogic.com>
+ * Copyright (C) 2019 Guillaume La Roque <glaroque@baylibre.com>
+ *
+ * Register value to celsius temperature formulas:
+ *	Read_Val	    m * U
+ * U = ---------, Uptat = ---------
+ *	2^16		  1 + n * U
+ *
+ * Temperature = A * ( Uptat + u_efuse / 2^16 )- B
+ *
+ *  A B m n : calibration parameters
+ *  u_efuse : fused calibration value, it's a signed 16 bits value
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+
+#include "thermal_core.h"
+
+#define TSENSOR_CFG_REG1			0x4
+	#define TSENSOR_CFG_REG1_RSET_VBG	BIT(12)
+	#define TSENSOR_CFG_REG1_RSET_ADC	BIT(11)
+	#define TSENSOR_CFG_REG1_VCM_EN		BIT(10)
+	#define TSENSOR_CFG_REG1_VBG_EN		BIT(9)
+	#define TSENSOR_CFG_REG1_OUT_CTL	BIT(6)
+	#define TSENSOR_CFG_REG1_FILTER_EN	BIT(5)
+	#define TSENSOR_CFG_REG1_DEM_EN		BIT(3)
+	#define TSENSOR_CFG_REG1_CH_SEL		GENMASK(1, 0)
+	#define TSENSOR_CFG_REG1_ENABLE		\
+		(TSENSOR_CFG_REG1_FILTER_EN |	\
+		 TSENSOR_CFG_REG1_VCM_EN |	\
+		 TSENSOR_CFG_REG1_VBG_EN |	\
+		 TSENSOR_CFG_REG1_DEM_EN |	\
+		 TSENSOR_CFG_REG1_CH_SEL)
+
+#define TSENSOR_STAT0			0x40
+
+#define TSENSOR_STAT9			0x64
+
+#define TSENSOR_READ_TEMP_MASK		GENMASK(15, 0)
+#define TSENSOR_TEMP_MASK		GENMASK(11, 0)
+
+#define TSENSOR_TRIM_SIGN_MASK		BIT(15)
+#define TSENSOR_TRIM_TEMP_MASK		GENMASK(14, 0)
+#define TSENSOR_TRIM_VERSION_MASK	GENMASK(31, 24)
+
+#define TSENSOR_TRIM_VERSION(_version)	\
+	FIELD_GET(TSENSOR_TRIM_VERSION_MASK, _version)
+
+#define TSENSOR_TRIM_CALIB_VALID_MASK	(GENMASK(3, 2) | BIT(7))
+
+#define TSENSOR_CALIB_OFFSET	1
+#define TSENSOR_CALIB_SHIFT	4
+
+/**
+ * struct amlogic_thermal_soc_calib_data
+ * @A, B, m, n: calibration parameters
+ * This structure is required for configuration of amlogic thermal driver.
+ */
+struct amlogic_thermal_soc_calib_data {
+	int A;
+	int B;
+	int m;
+	int n;
+};
+
+/**
+ * struct amlogic_thermal_data
+ * @u_efuse_off: register offset to read fused calibration value
+ * @soc: calibration parameters structure pointer
+ * @regmap_config: regmap config for the device
+ * This structure is required for configuration of amlogic thermal driver.
+ */
+struct amlogic_thermal_data {
+	int u_efuse_off;
+	const struct amlogic_thermal_soc_calib_data *calibration_parameters;
+	const struct regmap_config *regmap_config;
+};
+
+struct amlogic_thermal {
+	struct platform_device *pdev;
+	const struct amlogic_thermal_data *data;
+	struct regmap *regmap;
+	struct regmap *sec_ao_map;
+	struct clk *clk;
+	struct thermal_zone_device *tzd;
+	u32 trim_info;
+};
+
+/*
+ * Calculate a temperature value from a temperature code.
+ * The unit of the temperature is degree milliCelsius.
+ */
+static int amlogic_thermal_code_to_millicelsius(struct amlogic_thermal *pdata,
+						int temp_code)
+{
+	const struct amlogic_thermal_soc_calib_data *param =
+					pdata->data->calibration_parameters;
+	int temp;
+	s64 factor, Uptat, uefuse;
+
+	uefuse = pdata->trim_info & TSENSOR_TRIM_SIGN_MASK ?
+			     ~(pdata->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
+			     (pdata->trim_info & TSENSOR_TRIM_TEMP_MASK);
+
+	factor = param->n * temp_code;
+	factor = div_s64(factor, 100);
+
+	Uptat = temp_code * param->m;
+	Uptat = div_s64(Uptat, 100);
+	Uptat = Uptat * BIT(16);
+	Uptat = div_s64(Uptat, BIT(16) + factor);
+
+	temp = (Uptat + uefuse) * param->A;
+	temp = div_s64(temp, BIT(16));
+	temp = (temp - param->B) * 100;
+
+	return temp;
+}
+
+static int amlogic_thermal_initialize(struct amlogic_thermal *pdata)
+{
+	int ret = 0;
+	int ver;
+
+	regmap_read(pdata->sec_ao_map, pdata->data->u_efuse_off,
+		    &pdata->trim_info);
+
+	ver = TSENSOR_TRIM_VERSION(pdata->trim_info);
+
+	if ((ver & TSENSOR_TRIM_CALIB_VALID_MASK) == 0) {
+		ret = -EINVAL;
+		dev_err(&pdata->pdev->dev,
+			"tsensor thermal calibration not supported: 0x%x!\n",
+			ver);
+	}
+
+	return ret;
+}
+
+static int amlogic_thermal_enable(struct amlogic_thermal *data)
+{
+	int ret;
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
+	regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
+			   TSENSOR_CFG_REG1_ENABLE, TSENSOR_CFG_REG1_ENABLE);
+
+	return 0;
+}
+
+static int amlogic_thermal_disable(struct amlogic_thermal *data)
+{
+	regmap_update_bits(data->regmap, TSENSOR_CFG_REG1,
+			   TSENSOR_CFG_REG1_ENABLE, 0);
+	clk_disable_unprepare(data->clk);
+
+	return 0;
+}
+
+static int amlogic_thermal_get_temp(void *data, int *temp)
+{
+	unsigned int tval;
+	struct amlogic_thermal *pdata = data;
+
+	if (!data)
+		return -EINVAL;
+
+	regmap_read(pdata->regmap, TSENSOR_STAT0, &tval);
+	*temp =
+	   amlogic_thermal_code_to_millicelsius(pdata,
+						tval & TSENSOR_READ_TEMP_MASK);
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops amlogic_thermal_ops = {
+	.get_temp	= amlogic_thermal_get_temp,
+};
+
+static const struct regmap_config amlogic_thermal_regmap_config_g12a = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = TSENSOR_STAT9,
+};
+
+static const struct amlogic_thermal_soc_calib_data amlogic_thermal_g12a = {
+	.A = 9411,
+	.B = 3159,
+	.m = 424,
+	.n = 324,
+};
+
+static const struct amlogic_thermal_data amlogic_thermal_g12a_cpu_param = {
+	.u_efuse_off = 0x128,
+	.calibration_parameters = &amlogic_thermal_g12a,
+	.regmap_config = &amlogic_thermal_regmap_config_g12a,
+};
+
+static const struct amlogic_thermal_data amlogic_thermal_g12a_ddr_param = {
+	.u_efuse_off = 0xf0,
+	.calibration_parameters = &amlogic_thermal_g12a,
+	.regmap_config = &amlogic_thermal_regmap_config_g12a,
+};
+
+static const struct of_device_id of_amlogic_thermal_match[] = {
+	{
+		.compatible = "amlogic,g12a-ddr-thermal",
+		.data = &amlogic_thermal_g12a_ddr_param,
+	},
+	{
+		.compatible = "amlogic,g12a-cpu-thermal",
+		.data = &amlogic_thermal_g12a_cpu_param,
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_amlogic_thermal_match);
+
+static int amlogic_thermal_probe(struct platform_device *pdev)
+{
+	struct amlogic_thermal *pdata;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *base;
+	int ret;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->data = of_device_get_match_data(dev);
+	pdata->pdev = pdev;
+	platform_set_drvdata(pdev, pdata);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base)) {
+		dev_err(dev, "failed to get io address\n");
+		return PTR_ERR(base);
+	}
+
+	pdata->regmap = devm_regmap_init_mmio(dev, base,
+					      pdata->data->regmap_config);
+	if (IS_ERR(pdata->regmap))
+		return PTR_ERR(pdata->regmap);
+
+	pdata->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pdata->clk)) {
+		if (PTR_ERR(pdata->clk) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get clock\n");
+		return PTR_ERR(pdata->clk);
+	}
+
+	pdata->sec_ao_map = syscon_regmap_lookup_by_phandle
+		(pdev->dev.of_node, "amlogic,ao-secure");
+	if (IS_ERR(pdata->sec_ao_map)) {
+		dev_err(dev, "syscon regmap lookup failed.\n");
+		return PTR_ERR(pdata->sec_ao_map);
+	}
+
+	pdata->tzd = devm_thermal_zone_of_sensor_register(&pdev->dev,
+							  0,
+							  pdata,
+							  &amlogic_thermal_ops);
+	if (IS_ERR(pdata->tzd)) {
+		ret = PTR_ERR(pdata->tzd);
+		dev_err(dev, "Failed to register tsensor: %d\n", ret);
+		return PTR_ERR(pdata->tzd);
+	}
+
+	ret = amlogic_thermal_initialize(pdata);
+	if (ret)
+		return ret;
+
+	ret = amlogic_thermal_enable(pdata);
+
+	return ret;
+}
+
+static int amlogic_thermal_remove(struct platform_device *pdev)
+{
+	struct amlogic_thermal *data = platform_get_drvdata(pdev);
+
+	return amlogic_thermal_disable(data);
+}
+
+static int __maybe_unused amlogic_thermal_suspend(struct device *dev)
+{
+	struct amlogic_thermal *data = dev_get_drvdata(dev);
+
+	return amlogic_thermal_disable(data);
+}
+
+static int __maybe_unused amlogic_thermal_resume(struct device *dev)
+{
+	struct amlogic_thermal *data = dev_get_drvdata(dev);
+
+	return amlogic_thermal_enable(data);
+}
+
+static SIMPLE_DEV_PM_OPS(amlogic_thermal_pm_ops,
+			 amlogic_thermal_suspend, amlogic_thermal_resume);
+
+static struct platform_driver amlogic_thermal_driver = {
+	.driver = {
+		.name		= "amlogic_thermal",
+		.pm		= &amlogic_thermal_pm_ops,
+		.of_match_table = of_amlogic_thermal_match,
+	},
+	.probe	= amlogic_thermal_probe,
+	.remove	= amlogic_thermal_remove,
+};
+
+module_platform_driver(amlogic_thermal_driver);
+
+MODULE_AUTHOR("Guillaume La Roque <glaroque@baylibre.com>");
+MODULE_DESCRIPTION("Amlogic thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v4 0/6] Add support of New Amlogic temperature sensor for G12 SoCs
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm

This patchs series add support of New Amlogic temperature sensor and minimal
thermal zone for SEI510 and ODROID-N2 boards.

First implementation was doing on IIO[1] but after comments i move on thermal framework.
Formulas and calibration values come from amlogic.

Changes since v3:
  - Add cooling map and trip point for hot type
  - move compatible on g12a instead of g12 to be aligned with others
  - add all reviewer, sorry for this mistake

Changes since v2:
  - fix yaml documention
  - remove unneeded status variable for temperature-sensor node
  - rework driver after Martin review
  - add some information in commit message

Changes since v1:
  - fix enum vs const in documentation
  - fix error with thermal-sensor-cells value set to 1 instead of 0
  - add some dependencies needed to add cooling-maps

Dependencies :
- patch 3,4 & 5: depends on Neil's patch and series :
              - missing dwc2 phy-names[2]
              - patchsets to add DVFS on G12a[3] which have deps on [4] and [5]

[1] https://lore.kernel.org/linux-amlogic/20190604144714.2009-1-glaroque@baylibre.com/
[2] https://lore.kernel.org/linux-amlogic/20190625123647.26117-1-narmstrong@baylibre.com/
[3] https://lore.kernel.org/linux-amlogic/20190729132622.7566-1-narmstrong@baylibre.com/
[4] https://lore.kernel.org/linux-amlogic/20190731084019.8451-5-narmstrong@baylibre.com/
[5] https://lore.kernel.org/linux-amlogic/20190729132622.7566-3-narmstrong@baylibre.com/


Tested-by: Christian Hewitt <christianshewitt@gmail.com>
Tested-by: Kevin Hilman <khilman@baylibre.com>

Guillaume La Roque (6):
  dt-bindings: thermal: Add DT bindings documentation for Amlogic
    Thermal
  thermal: amlogic: Add thermal driver to support G12 SoCs
  arm64: dts: amlogic: g12: add temperature sensor
  arm64: dts: meson: sei510: Add minimal thermal zone
  arm64: dts: amlogic: odroid-n2: add minimal thermal zone
  MAINTAINERS: add entry for Amlogic Thermal driver

 .../bindings/thermal/amlogic,thermal.yaml     |  54 +++
 MAINTAINERS                                   |   9 +
 .../boot/dts/amlogic/meson-g12-common.dtsi    |  20 ++
 .../boot/dts/amlogic/meson-g12a-sei510.dts    |  70 ++++
 .../boot/dts/amlogic/meson-g12b-odroid-n2.dts |  76 ++++
 drivers/thermal/Kconfig                       |  11 +
 drivers/thermal/Makefile                      |   1 +
 drivers/thermal/amlogic_thermal.c             | 335 ++++++++++++++++++
 8 files changed, 576 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
 create mode 100644 drivers/thermal/amlogic_thermal.c

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH v4 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Adding the devicetree binding documentation for the Amlogic temperature
sensor found in the Amlogic Meson G12 SoCs.
the G12A  and G12B SoCs are supported.

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/thermal/amlogic,thermal.yaml     | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml

diff --git a/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
new file mode 100644
index 000000000000..f761681e4c0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/amlogic,thermal.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic Thermal
+
+maintainers:
+  - Guillaume La Roque <glaroque@baylibre.com>
+
+description: Binding for Amlogic Thermal
+
+properties:
+  compatible:
+      items:
+        - enum:
+            - amlogic,g12a-cpu-thermal
+            - amlogic,g12a-ddr-thermal
+        - const: amlogic,g12a-thermal
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  amlogic,ao-secure:
+    description: phandle to the ao-secure syscon
+    $ref: '/schemas/types.yaml#/definitions/phandle'
+
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - amlogic,ao-secure
+
+examples:
+  - |
+        cpu_temp: temperature-sensor@ff634800 {
+                compatible = "amlogic,g12a-cpu-thermal",
+                             "amlogic,g12a-thermal";
+                reg = <0xff634800 0x50>;
+                interrupts = <0x0 0x24 0x0>;
+                clocks = <&clk 164>;
+                #thermal-sensor-cells = <0>;
+                amlogic,ao-secure = <&sec_AO>;
+        };
+...
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v4 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Add minimal thermal zone for two temperature sensor
One is located close to the DDR and the other one is
located close to the PLLs (between the CPU and GPU)

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../boot/dts/amlogic/meson-g12a-sei510.dts    | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
index c9fa23a56562..35d2ebbd6d4e 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
@@ -10,6 +10,7 @@
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/gpio/meson-g12a-gpio.h>
 #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	compatible = "seirobotics,sei510", "amlogic,g12a";
@@ -33,6 +34,67 @@
 		ethernet0 = &ethmac;
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&cpu_temp>;
+
+			trips {
+				cpu_hot: cpu-hot {
+					temperature = <85000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "hot";
+				};
+
+				cpu_critical: cpu-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_hot>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+
+				map1 {
+					trip = <&cpu_critical>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		ddr-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&ddr_temp>;
+
+			trips {
+				ddr_critical: ddr-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&ddr_critical>;
+					cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+	};
+
 	mono_dac: audio-codec-0 {
 		compatible = "maxim,max98357a";
 		#sound-dai-cells = <0>;
@@ -321,6 +383,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu1 {
@@ -328,6 +391,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu2 {
@@ -335,6 +399,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu3 {
@@ -342,6 +407,7 @@
 	operating-points-v2 = <&cpu_opp_table>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cvbs_vdac_port {
@@ -368,6 +434,10 @@
 	status = "okay";
 };
 
+&mali {
+	#cooling-cells = <2>;
+};
+
 &hdmi_tx {
 	status = "okay";
 	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v4 3/6] arm64: dts: amlogic: g12: add temperature sensor
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Add cpu and ddr temperature sensors for G12 Socs

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../boot/dts/amlogic/meson-g12-common.dtsi    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index 06e186ca41e3..ce13c7c2e454 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -1353,6 +1353,26 @@
 				};
 			};
 
+			cpu_temp: temperature-sensor@34800 {
+				compatible = "amlogic,g12a-cpu-thermal",
+					     "amlogic,g12a-thermal";
+				reg = <0x0 0x34800 0x0 0x50>;
+				interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
+				clocks = <&clkc CLKID_TS>;
+				#thermal-sensor-cells = <0>;
+				amlogic,ao-secure = <&sec_AO>;
+			};
+
+			ddr_temp: temperature-sensor@34c00 {
+				compatible = "amlogic,g12a-ddr-thermal",
+					     "amlogic,g12a-thermal";
+				reg = <0x0 0x34c00 0x0 0x50>;
+				interrupts = <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>;
+				clocks = <&clkc CLKID_TS>;
+				#thermal-sensor-cells = <0>;
+				amlogic,ao-secure = <&sec_AO>;
+			};
+
 			usb2_phy0: phy@36000 {
 				compatible = "amlogic,g12a-usb2-phy";
 				reg = <0x0 0x36000 0x0 0x2000>;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v4 5/6] arm64: dts: amlogic: odroid-n2: add minimal thermal zone
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Add minimal thermal zone for two temperature sensor
One is located close to the DDR and the other one is
located close to the PLLs (between the CPU and GPU)

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../boot/dts/amlogic/meson-g12b-odroid-n2.dts | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
index 777bfb938854..8d7c73bad4aa 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
@@ -10,6 +10,7 @@
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/gpio/meson-g12a-gpio.h>
 #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	compatible = "hardkernel,odroid-n2", "amlogic,g12b";
@@ -20,6 +21,71 @@
 		ethernet0 = &ethmac;
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&cpu_temp>;
+
+			trips {
+				cpu_hot: cpu-hot {
+					temperature = <85000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "hot";
+				};
+
+				cpu_critical: cpu-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_hot>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu100 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu101 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu102 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu103 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+
+				map1 {
+					trip = <&cpu_critical>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu100 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu101 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu102 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu103 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		ddr-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <100>;
+			thermal-sensors = <&ddr_temp>;
+
+			trips {
+				ddr_critical: ddr-critical {
+					temperature = <110000>; /* millicelsius */
+					hysteresis = <2000>; /* millicelsius */
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map {
+					trip = <&ddr_critical>;
+					cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -289,6 +355,7 @@
 	operating-points-v2 = <&cpu_opp_table_0>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu1 {
@@ -296,6 +363,7 @@
 	operating-points-v2 = <&cpu_opp_table_0>;
 	clocks = <&clkc CLKID_CPU_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu100 {
@@ -303,6 +371,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu101 {
@@ -310,6 +379,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu102 {
@@ -317,6 +387,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &cpu103 {
@@ -324,6 +395,7 @@
 	operating-points-v2 = <&cpub_opp_table_1>;
 	clocks = <&clkc CLKID_CPUB_CLK>;
 	clock-latency = <50000>;
+	#cooling-cells = <2>;
 };
 
 &ext_mdio {
@@ -378,6 +450,10 @@
 	};
 };
 
+&mali {
+	#cooling-cells = <2>;
+};
+
 &hdmi_tx {
 	status = "okay";
 	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH v4 6/6] MAINTAINERS: add entry for Amlogic Thermal driver
From: Guillaume La Roque @ 2019-08-21 22:24 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Add myself as maintainer for Amlogic Thermal driver.

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fb2b12f75c37..299f27d11058 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15910,6 +15910,15 @@ F:	Documentation/driver-api/thermal/cpu-cooling-api.rst
 F:	drivers/thermal/cpu_cooling.c
 F:	include/linux/cpu_cooling.h
 
+THERMAL DRIVER FOR AMLOGIC SOCS
+M:	Guillaume La Roque <glaroque@baylibre.com>
+L:	linux-pm@vger.kernel.org
+L:	linux-amlogic@lists.infradead.org
+W:	http://linux-meson.com/
+S:	Supported
+F:	drivers/thermal/amlogic_thermal.c
+F:	Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml
+
 THINKPAD ACPI EXTRAS DRIVER
 M:	Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>
 L:	ibm-acpi-devel@lists.sourceforge.net
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller
From: Kevin Hilman @ 2019-08-21 23:16 UTC (permalink / raw)
  To: Neil Armstrong, ulf.hansson
  Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
	Neil Armstrong
In-Reply-To: <20190821114121.10430-3-narmstrong@baylibre.com>

Neil Armstrong <narmstrong@baylibre.com> writes:

> Add support for the General Purpose Amlogic Everything-Else Power controller,
> with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
> USB, NNA, GE2D and Ethernet Power Domains.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Nice!  Thanks for generalizing this.

A few comments/concerns below, but this is mostly ready.

> ---
>  drivers/soc/amlogic/Kconfig         |  11 +
>  drivers/soc/amlogic/Makefile        |   1 +
>  drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++++++++++++
>  3 files changed, 572 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index 23bfb8ef4fdb..bc2c912949bd 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -37,6 +37,17 @@ config MESON_GX_PM_DOMAINS
>  	  Say yes to expose Amlogic Meson GX Power Domains as
>  	  Generic Power Domains.
>  
> +config MESON_EE_PM_DOMAINS
> +	bool "Amlogic Meson Everything-Else Power Domains driver"
> +	depends on ARCH_MESON || COMPILE_TEST
> +	depends on PM && OF
> +	default ARCH_MESON
> +	select PM_GENERIC_DOMAINS
> +	select PM_GENERIC_DOMAINS_OF
> +	help
> +	  Say yes to expose Amlogic Meson Everything-Else Power Domains as
> +	  Generic Power Domains.
> +
>  config MESON_MX_SOCINFO
>  	bool "Amlogic Meson MX SoC Information driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index f2e4ed171297..de79d044b545 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> +obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
> diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
> new file mode 100644
> index 000000000000..7159888c850b
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-ee-pwrc.c
> @@ -0,0 +1,560 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + */
> +
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset.h>
> +#include <linux/clk.h>
> +#include <dt-bindings/power/meson-g12a-power.h>
> +#include <dt-bindings/power/meson-sm1-power.h>
> +
> +/* AO Offsets */
> +
> +#define AO_RTI_GEN_PWR_SLEEP0		(0x3a << 2)
> +#define AO_RTI_GEN_PWR_ISO0		(0x3b << 2)
> +
> +/* HHI Offsets */
> +
> +#define HHI_MEM_PD_REG0			(0x40 << 2)
> +#define HHI_VPU_MEM_PD_REG0		(0x41 << 2)
> +#define HHI_VPU_MEM_PD_REG1		(0x42 << 2)
> +#define HHI_VPU_MEM_PD_REG3		(0x43 << 2)
> +#define HHI_VPU_MEM_PD_REG4		(0x44 << 2)
> +#define HHI_AUDIO_MEM_PD_REG0		(0x45 << 2)
> +#define HHI_NANOQ_MEM_PD_REG0		(0x46 << 2)
> +#define HHI_NANOQ_MEM_PD_REG1		(0x47 << 2)
> +#define HHI_VPU_MEM_PD_REG2		(0x4d << 2)
> +
> +struct meson_ee_pwrc;
> +struct meson_ee_pwrc_domain;
> +
> +struct meson_ee_pwrc_mem_domain {
> +	unsigned int reg;
> +	unsigned int mask;
> +};
> +
> +struct meson_ee_pwrc_top_domain {
> +	unsigned int sleep_reg;
> +	unsigned int sleep_mask;
> +	unsigned int iso_reg;
> +	unsigned int iso_mask;
> +};
> +
> +struct meson_ee_pwrc_domain_desc {
> +	char *name;
> +	char **reset_names;
> +	unsigned int reset_names_count;
> +	char **clk_names;
> +	unsigned int clk_names_count;
> +	struct meson_ee_pwrc_top_domain *top_pd;
> +	unsigned int mem_pd_count;
> +	struct meson_ee_pwrc_mem_domain *mem_pd;
> +	bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
> +};
> +
> +struct meson_ee_pwrc_domain_data {
> +	unsigned int count;
> +	struct meson_ee_pwrc_domain_desc *domains;
> +};
> +
> +/* Clock and Resets lists */
> +
> +static char *g12a_pwrc_vpu_resets[] = {
> +	"viu", "venc", "vcbus", "bt656",
> +	"rdma", "venci", "vencp", "vdac",
> +	"vdi6", "vencl", "vid_lock",
> +};
> +
> +static char *g12a_pwrc_vpu_clks[] = {
> +	"vpu", "vapb",
> +};
> +
> +/* TOP Power Domains */
> +
> +static struct meson_ee_pwrc_top_domain g12a_pwrc_vpu = {
> +	.sleep_reg = AO_RTI_GEN_PWR_SLEEP0,
> +	.sleep_mask = BIT(8),
> +	.iso_reg = AO_RTI_GEN_PWR_SLEEP0,
> +	.iso_mask = BIT(9),
> +};
> +
> +#define SM1_EE_PD(__bit)					\
> +	{							\
> +		.sleep_reg = AO_RTI_GEN_PWR_SLEEP0, 		\
> +		.sleep_mask = BIT(__bit), 			\
> +		.iso_reg = AO_RTI_GEN_PWR_ISO0, 		\
> +		.iso_mask = BIT(__bit), 			\
> +	}
> +
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_nna = SM1_EE_PD(16);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_usb = SM1_EE_PD(17);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_pci = SM1_EE_PD(18);
> +static struct meson_ee_pwrc_top_domain sm1_pwrc_ge2d = SM1_EE_PD(19);
> +
> +/* Memory PD Domains */
> +
> +#define VPU_MEMPD(__reg)					\
> +	{ __reg, GENMASK(1, 0) },				\
> +	{ __reg, GENMASK(3, 2) },				\
> +	{ __reg, GENMASK(5, 4) },				\
> +	{ __reg, GENMASK(7, 6) },				\
> +	{ __reg, GENMASK(9, 8) },				\
> +	{ __reg, GENMASK(11, 10) },				\
> +	{ __reg, GENMASK(13, 12) },				\
> +	{ __reg, GENMASK(15, 14) },				\
> +	{ __reg, GENMASK(17, 16) },				\
> +	{ __reg, GENMASK(19, 18) },				\
> +	{ __reg, GENMASK(21, 20) },				\
> +	{ __reg, GENMASK(23, 22) },				\
> +	{ __reg, GENMASK(25, 24) },				\
> +	{ __reg, GENMASK(27, 26) },				\
> +	{ __reg, GENMASK(29, 28) },				\
> +	{ __reg, GENMASK(31, 30) }
> +
> +#define VPU_HHI_MEMPD(__reg)					\
> +	{ __reg, BIT(8) },					\
> +	{ __reg, BIT(9) },					\
> +	{ __reg, BIT(10) },					\
> +	{ __reg, BIT(11) },					\
> +	{ __reg, BIT(12) },					\
> +	{ __reg, BIT(13) },					\
> +	{ __reg, BIT(14) },					\
> +	{ __reg, BIT(15) }
> +
> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_vpu[] = {
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
> +	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
> +};
> +
> +static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_eth[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(3, 2) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_vpu[] = {
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
> +	VPU_MEMPD(HHI_VPU_MEM_PD_REG3),
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(1, 0) },
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(3, 2) },
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(5, 4) },
> +	{ HHI_VPU_MEM_PD_REG4, GENMASK(7, 6) },
> +	VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_nna[] = {
> +	{ HHI_NANOQ_MEM_PD_REG0, 0xff },
> +	{ HHI_NANOQ_MEM_PD_REG1, 0xff },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_usb[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(31, 30) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_pcie[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(29, 26) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_ge2d[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(25, 18) },
> +};
> +
> +static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
> +	{ HHI_MEM_PD_REG0, GENMASK(5, 4) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(1, 0) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(3, 2) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(5, 4) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(7, 6) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(13, 12) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(15, 14) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(17, 16) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(19, 18) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(21, 20) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(23, 22) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(25, 24) },
> +	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
> +};
> +
> +#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power)	\
> +	{								\
> +		.name = __name,						\
> +		.reset_names_count = ARRAY_SIZE(__resets),		\
> +		.reset_names = __resets,				\
> +		.clk_names_count = ARRAY_SIZE(__clks),			\
> +		.clk_names = __clks,					\
> +		.top_pd = __top_pd,					\
> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
> +		.mem_pd = __mem,					\
> +		.get_power = __get_power,				\
> +	}
> +
> +#define TOP_PD(__name, __top_pd, __mem)					\
> +	{								\
> +		.name = __name,						\
> +		.top_pd = __top_pd,					\
> +		.mem_pd_count = ARRAY_SIZE(__mem),			\
> +		.mem_pd = __mem,					\
> +	}

Why can't the TOP_PD domains also have a __get_power().  Shouldn't we
just be able to check the sleep_reg/sleep_mask like in the VPU case?

Also, for readability, I think the arguments to VPU_PD and TOP_PD to
have the same order, at least for the common ones.  IOW, __name,
__top_pd, __mem should be first.

> +#define MEM_PD(__name, __mem)						\
> +	TOP_PD(__name, NULL, __mem)
> +
> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
> +
> +static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
> +	[PWRC_G12A_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
> +				     g12a_pwrc_vpu_clks, &g12a_pwrc_vpu,
> +				     g12a_pwrc_mem_vpu,
> +				     pwrc_vpu_get_power),
> +	[PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
> +
> +static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
> +	[PWRC_SM1_VPU_ID]  = VPU_PD("VPU", g12a_pwrc_vpu_resets,
> +				    g12a_pwrc_vpu_clks, &sm1_pwrc_vpu,
> +				    sm1_pwrc_mem_vpu,
> +				    pwrc_vpu_get_power),
> +	[PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna),
> +	[PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb),
> +	[PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie),
> +	[PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d),
> +	[PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
> +	[PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
> +};
> +
> +struct meson_ee_pwrc_domain {
> +	struct generic_pm_domain base;
> +	bool enabled;
> +	struct meson_ee_pwrc *pwrc;
> +	struct meson_ee_pwrc_domain_desc desc;
> +	struct clk **clks;
> +	int num_clks;
> +	struct reset_control **rstc;
> +	int num_rstc;
> +};
> +
> +struct meson_ee_pwrc {
> +	struct regmap *regmap_ao;
> +	struct regmap *regmap_hhi;
> +	struct meson_ee_pwrc_domain *domains;
> +	struct genpd_onecell_data xlate;
> +};
> +
> +static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	u32 reg;
> +
> +	regmap_read(pwrc_domain->pwrc->regmap_ao,
> +		    pwrc_domain->desc.top_pd->sleep_reg, &reg);
> +
> +	return (reg & pwrc_domain->desc.top_pd->sleep_mask);
> +}
> +
> +static int meson_ee_reset_assert(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
> +		ret = reset_control_assert(pwrc_domain->rstc[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int meson_ee_reset_deassert(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
> +		ret = reset_control_deassert(pwrc_domain->rstc[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

You should use the reset_array functions, then you don't need these helpers.

> +static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i;
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
> +		clk_disable(pwrc_domain->clks[i]);
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
> +		clk_unprepare(pwrc_domain->clks[i]);
> +
> +	return 0;
> +}
> +
> +static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
> +		ret = clk_prepare(pwrc_domain->clks[i]);
> +		if (ret)
> +			goto fail_prepare;
> +	}
> +
> +	for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
> +		ret = clk_enable(pwrc_domain->clks[i]);
> +		if (ret)
> +			goto fail_enable;
> +	}
> +
> +	return 0;
> +fail_enable:
> +	while (--i)
> +		clk_disable(pwrc_domain->clks[i]);
> +
> +	/* Unprepare all clocks */
> +	i = pwrc_domain->num_clks;
> +
> +fail_prepare:
> +	while (--i)
> +		clk_unprepare(pwrc_domain->clks[i]);
> +
> +	return ret;
> +}

Both the clk enable and disable functions above are just open-coding of
the clk_bulk equivalents.  Please use clk_bulk_*, then you don't need
these helpers.  (c.f. the RFT patch I did to convert the old driver to
clk_bulk[1])

> +static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
> +{
> +	struct meson_ee_pwrc_domain *pwrc_domain =
> +		container_of(domain, struct meson_ee_pwrc_domain, base);
> +	int i;
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->sleep_reg,
> +				   pwrc_domain->desc.top_pd->sleep_mask,
> +				   pwrc_domain->desc.top_pd->sleep_mask);
> +	udelay(20);
> +
> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
> +				   pwrc_domain->desc.mem_pd[i].reg,
> +				   pwrc_domain->desc.mem_pd[i].mask,
> +				   pwrc_domain->desc.mem_pd[i].mask);
> +
> +	udelay(20);
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->iso_reg,
> +				   pwrc_domain->desc.top_pd->iso_mask,
> +				   pwrc_domain->desc.top_pd->iso_mask);
> +
> +	if (pwrc_domain->num_clks) {
> +		msleep(20);
> +		meson_ee_clk_disable(pwrc_domain);
> +	}
> +
> +	return 0;
> +}
> +
> +static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
> +{
> +	struct meson_ee_pwrc_domain *pwrc_domain =
> +		container_of(domain, struct meson_ee_pwrc_domain, base);
> +	int i, ret;
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->sleep_reg,
> +				   pwrc_domain->desc.top_pd->sleep_mask, 0);
> +	udelay(20);
> +
> +	for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
> +				   pwrc_domain->desc.mem_pd[i].reg,
> +				   pwrc_domain->desc.mem_pd[i].mask, 0);
> +
> +	udelay(20);
> +
> +	ret = meson_ee_reset_assert(pwrc_domain);
> +	if (ret)
> +		return ret;
> +
> +	if (pwrc_domain->desc.top_pd)
> +		regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
> +				   pwrc_domain->desc.top_pd->iso_reg,
> +				   pwrc_domain->desc.top_pd->iso_mask, 0);
> +
> +	ret = meson_ee_reset_deassert(pwrc_domain);
> +	if (ret)
> +		return ret;
> +
> +	return meson_ee_clk_enable(pwrc_domain);
> +}
> +
> +static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
> +				     struct meson_ee_pwrc *sm1_pwrc,
> +				     struct meson_ee_pwrc_domain *dom)
> +{
> +	dom->pwrc = sm1_pwrc;
> +	dom->num_rstc = dom->desc.reset_names_count;
> +	dom->num_clks = dom->desc.clk_names_count;
> +
> +	if (dom->num_rstc) {
> +		int rst;
> +
> +		dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
> +				sizeof(struct reset_control *),	GFP_KERNEL);
> +		if (!dom->rstc)
> +			return -ENOMEM;
> +
> +		for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
> +			dom->rstc[rst] = devm_reset_control_get_exclusive(
> +					&pdev->dev,
> +					dom->desc.reset_names[rst]);
> +			if (IS_ERR(dom->rstc[rst]))
> +				return PTR_ERR(dom->rstc[rst]);
> +		}

Why not simplify and use the helpers that get multiple reset lines (like
devm_reset_control_array_get() used in meson-gx-pwrc-vpu.c)?

You could also use reset_control_get_count() and compare to the expected
number (dom->num_rstc).

> +	}
> +
> +	if (dom->num_clks) {
> +		int clk;
> +
> +		dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
> +				sizeof(struct clk *), GFP_KERNEL);
> +		if (!dom->clks)
> +			return -ENOMEM;
> +
> +		for (clk = 0 ; clk < dom->num_clks ; ++clk) {
> +			dom->clks[clk] = devm_clk_get(&pdev->dev,
> +					dom->desc.clk_names[clk]);
> +			if (IS_ERR(dom->clks[clk]))
> +				return PTR_ERR(dom->clks[clk]);
> +		}
> +	}

Please use clk_bulk API, and then just double-check that the number of
clocks found matches the expected number.

> +	dom->base.name = dom->desc.name;
> +	dom->base.power_on = meson_ee_pwrc_on;
> +	dom->base.power_off = meson_ee_pwrc_off;
> +
> +	if (dom->desc.get_power) {
> +		bool powered_off = dom->desc.get_power(dom);

nit: insert blank line here

More importantly, we defintely will have problem here in the
!powered_off case.  TL;DR; the driver's state does not match the actual
hardware state.

When powered_off = false, you're telling the genpd core that this domain
is already turned on.  However, you haven't called _pwrc_on() yet for
the domain, which means internal state of the driver for this domain
(e.g. clock enables, resets, etc.) is not in sync with the HW.  On
SEI610 this case is happending for the VPU, which seems to be enabled by
u-boot, so this driver detects it as already on, which is fine.  But...

Remember that the ->power_off() function will be called during suspend,
and will lead to the clk unprepare/disable calls.  However, for domains
that are detected as on (!powered_off), clk prepare/enable will never
have been called, so that when suspend happens, you'll get "already
unprepared" errors from the clock core

IOW, I think you need something like this here:

		if (!powered_off)
			meson_ee_pwrc_on(&dom->base);

so that the internal state of clock fwk etc. matches the detected state
of the HW.  The problem with that simple fix, at least for the VPU is
that it might cause us to lose any existing display or framebuffer
console that's on-going.  Probably needs some testing.

Anyways, to see what I mean, try suspend/resume (you can test this
series on my integ branch with "rtcwake -d rtc0 -m mem -s4") and you'll
see error splats from the clock core during suspend.



> +		pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
> +			      powered_off);

> +	} else
> +		pm_genpd_init(&dom->base, NULL, true);

nit: the else clause should also have {} to match the if
(c.f. CodingStyle)

Why do you force the always-on governor in the case where ->get_power()
exists, but not the other?

If you force that, then for any devices connected to these domains that
use runtime PM, they will never turn off the domain when it's idle.
IOW, these domains will only ever be turned off on system-wide
suspend/resume.

IMO, unless there's a good reason not to, you should pass NULL for the
governor.

> +	return 0;
> +}
> +
> +static int meson_ee_pwrc_probe(struct platform_device *pdev)
> +{
> +	const struct meson_ee_pwrc_domain_data *match;
> +	struct regmap *regmap_ao, *regmap_hhi;
> +	struct meson_ee_pwrc *sm1_pwrc;

Why the sm1_ prefix throughout this code?  This is now generalized for
more SoCs, so shouldn't be sm1.  I'm assuming this is just left-over
from the original version.  IMO, just called it pwrc.

> +	int i, ret;
> +
> +	match = of_device_get_match_data(&pdev->dev);
> +	if (!match) {
> +		dev_err(&pdev->dev, "failed to get match data\n");
> +		return -ENODEV;
> +	}
> +
> +	sm1_pwrc = devm_kzalloc(&pdev->dev, sizeof(*sm1_pwrc), GFP_KERNEL);
> +	if (!sm1_pwrc)
> +		return -ENOMEM;
> +
> +	sm1_pwrc->xlate.domains =
> +		devm_kcalloc(&pdev->dev,
> +			     match->count,
> +			     sizeof(*sm1_pwrc->xlate.domains),
> +			     GFP_KERNEL);
> +	if (!sm1_pwrc->xlate.domains)
> +		return -ENOMEM;
> +
> +	sm1_pwrc->domains =
> +		devm_kcalloc(&pdev->dev,

nit: this line probably doesn't need to be wrapped.

> +			     match->count,
> +			     sizeof(*sm1_pwrc->domains),
> +			     GFP_KERNEL);
> +	if (!sm1_pwrc->domains)
> +		return -ENOMEM;
> +
> +	sm1_pwrc->xlate.num_domains = match->count;
> +
> +	regmap_hhi = syscon_node_to_regmap(of_get_parent(pdev->dev.of_node));
> +	if (IS_ERR(regmap_hhi)) {
> +		dev_err(&pdev->dev, "failed to get HHI regmap\n");
> +		return PTR_ERR(regmap_hhi);
> +	}
> +
> +	regmap_ao = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +						    "amlogic,ao-sysctrl");
> +	if (IS_ERR(regmap_ao)) {
> +		dev_err(&pdev->dev, "failed to get AO regmap\n");
> +		return PTR_ERR(regmap_ao);
> +	}
> +
> +	sm1_pwrc->regmap_ao = regmap_ao;
> +	sm1_pwrc->regmap_hhi = regmap_hhi;
> +
> +	platform_set_drvdata(pdev, sm1_pwrc);
> +
> +	for (i = 0 ; i < match->count ; ++i) {
> +		struct meson_ee_pwrc_domain *dom = &sm1_pwrc->domains[i];
> +
> +		memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
> +
> +		ret = meson_ee_pwrc_init_domain(pdev, sm1_pwrc, dom);
> +		if (ret)
> +			return ret;
> +
> +		sm1_pwrc->xlate.domains[i] = &dom->base;
> +	}
> +
> +	of_genpd_add_provider_onecell(pdev->dev.of_node, &sm1_pwrc->xlate);
> +
> +	return 0;
> +}
> +
> +static struct meson_ee_pwrc_domain_data meson_ee_g12a_pwrc_data = {
> +	.count = ARRAY_SIZE(g12a_pwrc_domains),
> +	.domains = g12a_pwrc_domains,
> +};
> +
> +static struct meson_ee_pwrc_domain_data meson_ee_sm1_pwrc_data = {
> +	.count = ARRAY_SIZE(sm1_pwrc_domains),
> +	.domains = sm1_pwrc_domains,
> +};
> +
> +static const struct of_device_id meson_ee_pwrc_match_table[] = {
> +	{
> +		.compatible = "amlogic,meson-g12a-pwrc",
> +		.data = &meson_ee_g12a_pwrc_data,
> +	},
> +	{
> +		.compatible = "amlogic,meson-sm1-pwrc",
> +		.data = &meson_ee_sm1_pwrc_data,
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver meson_ee_pwrc_driver = {
> +	.probe	= meson_ee_pwrc_probe,
> +	.driver = {
> +		.name		= "meson_ee_pwrc",
> +		.of_match_table	= meson_ee_pwrc_match_table,
> +	},
> +};
> +builtin_platform_driver(meson_ee_pwrc_driver);
> -- 
> 2.22.0

Kevin

[1] https://lore.kernel.org/linux-amlogic/20190809230904.28747-1-khilman@baylibre.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 4/6] arm64: dts: meson: sei510: Add minimal thermal zone
From: Kevin Hilman @ 2019-08-21 23:29 UTC (permalink / raw)
  To: Guillaume La Roque, rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-5-glaroque@baylibre.com>

Guillaume La Roque <glaroque@baylibre.com> writes:

> Add minimal thermal zone for two temperature sensor
> One is located close to the DDR and the other one is
> located close to the PLLs (between the CPU and GPU)
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../boot/dts/amlogic/meson-g12a-sei510.dts    | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> index c9fa23a56562..35d2ebbd6d4e 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts
> @@ -10,6 +10,7 @@
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/gpio/meson-g12a-gpio.h>
>  #include <dt-bindings/sound/meson-g12a-tohdmitx.h>
> +#include <dt-bindings/thermal/thermal.h>
>  
>  / {
>  	compatible = "seirobotics,sei510", "amlogic,g12a";
> @@ -33,6 +34,67 @@
>  		ethernet0 = &ethmac;
>  	};
>  
> +	thermal-zones {
> +		cpu-thermal {
> +			polling-delay = <1000>;
> +			polling-delay-passive = <100>;
> +			thermal-sensors = <&cpu_temp>;
> +
> +			trips {
> +				cpu_hot: cpu-hot {
> +					temperature = <85000>; /* millicelsius */
> +					hysteresis = <2000>; /* millicelsius */
> +					type = "hot";
> +				};
> +
> +				cpu_critical: cpu-critical {
> +					temperature = <110000>; /* millicelsius */
> +					hysteresis = <2000>; /* millicelsius */
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu_hot>;
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +
> +				map1 {
> +					trip = <&cpu_critical>;
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
> +		};
> +
> +		ddr-thermal {
> +			polling-delay = <1000>;
> +			polling-delay-passive = <100>;
> +			thermal-sensors = <&ddr_temp>;
> +
> +			trips {
> +				ddr_critical: ddr-critical {
> +					temperature = <110000>; /* millicelsius */
> +					hysteresis = <2000>; /* millicelsius */
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map {
> +					trip = <&ddr_critical>;
> +					cooling-device = <&mali THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
> +		};
> +	};
> +
>  	mono_dac: audio-codec-0 {
>  		compatible = "maxim,max98357a";
>  		#sound-dai-cells = <0>;
> @@ -321,6 +383,7 @@
>  	operating-points-v2 = <&cpu_opp_table>;
>  	clocks = <&clkc CLKID_CPU_CLK>;
>  	clock-latency = <50000>;
> +	#cooling-cells = <2>;
>  };
>  
>  &cpu1 {
> @@ -328,6 +391,7 @@
>  	operating-points-v2 = <&cpu_opp_table>;
>  	clocks = <&clkc CLKID_CPU_CLK>;
>  	clock-latency = <50000>;
> +	#cooling-cells = <2>;
>  };
>  
>  &cpu2 {
> @@ -335,6 +399,7 @@
>  	operating-points-v2 = <&cpu_opp_table>;
>  	clocks = <&clkc CLKID_CPU_CLK>;
>  	clock-latency = <50000>;
> +	#cooling-cells = <2>;
>  };
>  
>  &cpu3 {
> @@ -342,6 +407,7 @@
>  	operating-points-v2 = <&cpu_opp_table>;
>  	clocks = <&clkc CLKID_CPU_CLK>;
>  	clock-latency = <50000>;
> +	#cooling-cells = <2>;
>  };
>  
>  &cvbs_vdac_port {
> @@ -368,6 +434,10 @@
>  	status = "okay";
>  };
>  
> +&mali {
> +	#cooling-cells = <2>;
> +};
> +

Is there a reason these #cooling-cells properties belong in the SoC
.dtsi and not the board .dts.  Seems like you'll have to repeat this in
every board .dts which doesn't seem necessary.

Same comment for patch 5/6

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support
From: Kevin Hilman @ 2019-08-21 23:31 UTC (permalink / raw)
  To: Neil Armstrong, ulf.hansson
  Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
	Neil Armstrong
In-Reply-To: <20190821114121.10430-5-narmstrong@baylibre.com>

Neil Armstrong <narmstrong@baylibre.com> writes:

> Update compatible of the pwc-vpu node and add the HDMI support nodes
> for the Amlogic SM1 Based SEI610 Board.

I think this changelog is out of date.  It's not doing anything with the
VPU pwrc node.

Kevin

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  .../boot/dts/amlogic/meson-sm1-sei610.dts     | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> index 12dab0ba2f26..66bd3bfbaf91 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
> @@ -51,6 +51,17 @@
>  		};
>  	};
>  
> +	hdmi-connector {
> +		compatible = "hdmi-connector";
> +		type = "a";
> +
> +		port {
> +			hdmi_connector_in: endpoint {
> +				remote-endpoint = <&hdmi_tx_tmds_out>;
> +			};
> +		};
> +	};
> +
>  	leds {
>  		compatible = "gpio-leds";
>  
> @@ -177,6 +188,18 @@
>  	phy-mode = "rmii";
>  };
>  
> +&hdmi_tx {
> +	status = "okay";
> +	pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
> +	pinctrl-names = "default";
> +};
> +
> +&hdmi_tx_tmds_port {
> +	hdmi_tx_tmds_out: endpoint {
> +		remote-endpoint = <&hdmi_connector_in>;
> +	};
> +};
> +
>  &i2c3 {
>  	status = "okay";
>  	pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
> -- 
> 2.22.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 0/6] Add support of New Amlogic temperature sensor for G12 SoCs
From: Kevin Hilman @ 2019-08-21 23:39 UTC (permalink / raw)
  To: Guillaume La Roque, rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-1-glaroque@baylibre.com>

Guillaume La Roque <glaroque@baylibre.com> writes:

> This patchs series add support of New Amlogic temperature sensor and minimal
> thermal zone for SEI510 and ODROID-N2 boards.
>
> First implementation was doing on IIO[1] but after comments i move on thermal framework.
> Formulas and calibration values come from amlogic.
>
> Changes since v3:
>   - Add cooling map and trip point for hot type
>   - move compatible on g12a instead of g12 to be aligned with others
>   - add all reviewer, sorry for this mistake
>
> Changes since v2:
>   - fix yaml documention
>   - remove unneeded status variable for temperature-sensor node
>   - rework driver after Martin review
>   - add some information in commit message
>
> Changes since v1:
>   - fix enum vs const in documentation
>   - fix error with thermal-sensor-cells value set to 1 instead of 0
>   - add some dependencies needed to add cooling-maps
>
> Dependencies :
> - patch 3,4 & 5: depends on Neil's patch and series :
>               - missing dwc2 phy-names[2]
>               - patchsets to add DVFS on G12a[3] which have deps on [4] and [5]
>
> [1] https://lore.kernel.org/linux-amlogic/20190604144714.2009-1-glaroque@baylibre.com/
> [2] https://lore.kernel.org/linux-amlogic/20190625123647.26117-1-narmstrong@baylibre.com/
> [3] https://lore.kernel.org/linux-amlogic/20190729132622.7566-1-narmstrong@baylibre.com/
> [4] https://lore.kernel.org/linux-amlogic/20190731084019.8451-5-narmstrong@baylibre.com/
> [5] https://lore.kernel.org/linux-amlogic/20190729132622.7566-3-narmstrong@baylibre.com/
>
>
> Tested-by: Christian Hewitt <christianshewitt@gmail.com>
> Tested-by: Kevin Hilman <khilman@baylibre.com>

nit: you should put these on the individual patches, since the cover
letter does not get applied to any tree, any tags here get lost.

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4 1/6] dt-bindings: thermal: Add DT bindings documentation for Amlogic Thermal
From: Kevin Hilman @ 2019-08-21 23:40 UTC (permalink / raw)
  To: Guillaume La Roque, rui.zhang, edubezval, daniel.lezcano
  Cc: devicetree, linux-amlogic, linux-kernel, linux-arm-kernel,
	linux-pm
In-Reply-To: <20190821222421.30242-2-glaroque@baylibre.com>

Guillaume La Roque <glaroque@baylibre.com> writes:

> Adding the devicetree binding documentation for the Amlogic temperature
> sensor found in the Amlogic Meson G12 SoCs.
> the G12A  and G12B SoCs are supported.
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

nit: put your sign-off at the end.  The tags you collect from
reviewers/testers should go first.

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 5/9] block: support diskcipher
From: boojin.kim @ 2019-08-22  0:54 UTC (permalink / raw)
  To: axboe, linux-block, linux-kernel
  Cc: 'Ulf Hansson', 'Mike Snitzer', dm-devel,
	'Andreas Dilger', 'Alasdair Kergon',
	'Eric Biggers', linux-samsung-soc, 'Herbert Xu',
	'Krzysztof Kozlowski', 'Jaehoon Chung',
	'Kukjin Kim', linux-ext4, 'Chao Yu',
	linux-fscrypt, 'Jaegeuk Kim', linux-arm-kernel,
	'Jens Axboe', 'Theodore Ts'o', linux-mmc,
	linux-f2fs-devel, linux-crypto, linux-fsdevel,
	'David S. Miller'
In-Reply-To: <CGME20190822005438epcas2p337aba06b328cdcdd1549395f0bbcfdbc@epcas2p3.samsung.com>

On 8/21/19 21:09 AM, Jens Axboe wrote:
> This isn't going to happen. With this, and the inline encryption
> proposed by Google, we'll bloat the bio even more. At least the Google
> approach didn't include bio iter changes as well.

> Please work it out between yourselves so we can have a single, clean
> abstraction that works for both.

I'm looking at inline encryption by Google. 
I will find compatibility with inline encryption to avoid conflicts
in BIO/F2FS.
And changing bio iter has a benefit for diskcipher.
Without changing bio iter, diskcipher should control(alloc/free) a buffer
to hold a 'bi_dun' variable for every bio.
But changing bio iter is difficult to accept for block layer,
I will modify the diskcipher.
And, as you mentioned, inline encryption by Google has this control.
So I might be able to use it.

Thanks.
Boojin Kim.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v4] kasan: add memory corruption identification for software tag-based mode
From: Walter Wu @ 2019-08-22  1:22 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: wsd_upstream, Vasily Gorbik, Arnd Bergmann, linux-mm,
	Andrey Konovalov, linux-kernel, kasan-dev, Martin Schwidefsky,
	Miles Chen, Alexander Potapenko, linux-arm-kernel,
	Matthias Brugger, linux-mediatek, Andrew Morton, Thomas Gleixner,
	Dmitry Vyukov
In-Reply-To: <3318f9d7-a760-3cc8-b700-f06108ae745f@virtuozzo.com>

On Wed, 2019-08-21 at 20:52 +0300, Andrey Ryabinin wrote:
> 
> On 8/20/19 8:37 AM, Walter Wu wrote:
> > On Tue, 2019-08-06 at 13:43 +0800, Walter Wu wrote:
> >> This patch adds memory corruption identification at bug report for
> >> software tag-based mode, the report show whether it is "use-after-free"
> >> or "out-of-bound" error instead of "invalid-access" error. This will make
> >> it easier for programmers to see the memory corruption problem.
> >>
> >> We extend the slab to store five old free pointer tag and free backtrace,
> >> we can check if the tagged address is in the slab record and make a
> >> good guess if the object is more like "use-after-free" or "out-of-bound".
> >> therefore every slab memory corruption can be identified whether it's
> >> "use-after-free" or "out-of-bound".
> >>
> >> ====== Changes
> >> Change since v1:
> >> - add feature option CONFIG_KASAN_SW_TAGS_IDENTIFY.
> >> - change QUARANTINE_FRACTION to reduce quarantine size.
> >> - change the qlist order in order to find the newest object in quarantine
> >> - reduce the number of calling kmalloc() from 2 to 1 time.
> >> - remove global variable to use argument to pass it.
> >> - correct the amount of qobject cache->size into the byes of qlist_head.
> >> - only use kasan_cache_shrink() to shink memory.
> >>
> >> Change since v2:
> >> - remove the shinking memory function kasan_cache_shrink()
> >> - modify the description of the CONFIG_KASAN_SW_TAGS_IDENTIFY
> >> - optimize the quarantine_find_object() and qobject_free()
> >> - fix the duplicating function name 3 times in the header.
> >> - modify the function name set_track() to kasan_set_track()
> >>
> >> Change since v3:
> >> - change tag-based quarantine to extend slab to identify memory corruption
> > 
> > Hi,Andrey,
> > 
> > Would you review the patch,please?
> 
> 
> I didn't notice anything fundamentally wrong, but I find there are some
> questionable implementation choices that makes code look weirder than necessary
> and harder to understand. So I ended up with cleaning it up, see the diff bellow.
> I'll send v5 with that diff folded.
> 

Thanks your review and suggestion.

Walter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 2/7] ARM: aspeed: Select timer in each SoC
From: Andrew Jeffery @ 2019-08-22  1:27 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
  Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
	Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-3-joel@jms.id.au>



On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> In preparation for adding the ast2600 which does not use this timer.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  arch/arm/mach-aspeed/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index a15c3a291386..2979aa4daeea 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -5,7 +5,6 @@ menuconfig ARCH_ASPEED
>  	select SRAM
>  	select WATCHDOG
>  	select ASPEED_WATCHDOG
> -	select FTTMR010_TIMER
>  	select MFD_SYSCON
>  	select PINCTRL
>  	help
> @@ -18,6 +17,7 @@ config MACH_ASPEED_G4
>  	depends on ARCH_MULTI_V5
>  	select CPU_ARM926T
>  	select PINCTRL_ASPEED_G4
> +	select FTTMR010_TIMER
>  	help
>  	 Say yes if you intend to run on an Aspeed ast2400 or similar
>  	 fourth generation BMCs, such as those used by OpenPower Power8
> @@ -28,6 +28,7 @@ config MACH_ASPEED_G5
>  	depends on ARCH_MULTI_V6
>  	select CPU_V6
>  	select PINCTRL_ASPEED_G5
> +	select FTTMR010_TIMER
>  	help
>  	 Say yes if you intend to run on an Aspeed ast2500 or similar
>  	 fifth generation Aspeed BMCs.
> -- 
> 2.23.0.rc1
> 
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 3/7] ARM: aspeed: Add ASPEED AST2600 architecture
From: Andrew Jeffery @ 2019-08-22  1:28 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
  Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
	Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-4-joel@jms.id.au>



On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> The AST2600 is a Cortex A7 dual core CPU that uses the ARM GIC for
> interrupts and ARM timer as a clocksource.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  arch/arm/mach-aspeed/Kconfig | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index 2979aa4daeea..56007b0b6120 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  menuconfig ARCH_ASPEED
>  	bool "Aspeed BMC architectures"
> -	depends on ARCH_MULTI_V5 || ARCH_MULTI_V6
> +	depends on ARCH_MULTI_V5 || ARCH_MULTI_V6 || ARCH_MULTI_V7
>  	select SRAM
>  	select WATCHDOG
>  	select ASPEED_WATCHDOG
> @@ -33,4 +33,16 @@ config MACH_ASPEED_G5
>  	 Say yes if you intend to run on an Aspeed ast2500 or similar
>  	 fifth generation Aspeed BMCs.
>  
> +config MACH_ASPEED_G6
> +	bool "Aspeed SoC 6th Generation"
> +	depends on ARCH_MULTI_V7
> +	select CPU_V7
> +	select PINCTRL_ASPEED_G6
> +	select ARM_GIC
> +	select HAVE_ARM_ARCH_TIMER
> +	select HAVE_SMP
> +	help
> +	 Say yes if you intend to run on an Aspeed ast2600 or similar
> +	 sixth generation Aspeed BMCs.
> +
>  endif
> -- 
> 2.23.0.rc1
> 
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 5/7] ARM: dts: aspeed: Add AST2600 and EVB
From: Andrew Jeffery @ 2019-08-22  1:58 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
  Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
	Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-6-joel@jms.id.au>



On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> The AST2600 is a new SoC by ASPEED. It contains a dual core Cortex A7
> CPU and shares many periperhals with the existing AST2400 and AST2500.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  arch/arm/boot/dts/Makefile               |   1 +
>  arch/arm/boot/dts/aspeed-ast2600-evb.dts |  44 ++++
>  arch/arm/boot/dts/aspeed-g6.dtsi         | 266 +++++++++++++++++++++++
>  3 files changed, 311 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-ast2600-evb.dts
>  create mode 100644 arch/arm/boot/dts/aspeed-g6.dtsi
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 247e556de48e..2d8d29e5686d 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1276,6 +1276,7 @@ dtb-$(CONFIG_ARCH_MILBEAUT) += 
> milbeaut-m10v-evb.dtb
>  dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
>  dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-ast2500-evb.dtb \
> +	aspeed-ast2600-evb.dtb \
>  	aspeed-bmc-arm-centriq2400-rep.dtb \
>  	aspeed-bmc-arm-stardragon4800-rep2.dtb \
>  	aspeed-bmc-facebook-cmm.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> new file mode 100644
> index 000000000000..7f2528e084b5
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2019 IBM Corp.
> +
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +
> +/ {
> +	model = "AST2600 EVB";
> +	compatible = "aspeed,ast2600";
> +
> +	aliases {
> +		serial4 = &uart5;
> +	};
> +
> +	chosen {
> +		bootargs = "console=ttyS4,115200n8";
> +	};
> +
> +	memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0x80000000>;
> +	};
> +};
> +
> +&mdio1 {
> +	status = "okay";
> +
> +	ethphy1: ethernet-phy@0 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0>;
> +	};
> +};
> +
> +&mac1 {
> +	status = "okay";
> +
> +	phy-mode = "rgmii";
> +	phy-handle = <&ethphy1>;
> +};
> +
> +&emmc {
> +	status = "okay";
> +};
> diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi 
> b/arch/arm/boot/dts/aspeed-g6.dtsi
> new file mode 100644
> index 000000000000..9f9931541060
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-g6.dtsi
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2019 IBM Corp.
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/ast2600-clock.h>
> +
> +/ {
> +	model = "Aspeed BMC";
> +	compatible = "aspeed,ast2600";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	interrupt-parent = <&gic>;
> +
> +	aliases {
> +		serial4 = &uart5;
> +	};
> +
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		enable-method = "aspeed,ast2600-smp";
> +
> +		cpu@f00 {
> +			compatible = "arm,cortex-a7";
> +			device_type = "cpu";
> +			reg = <0xf00>;
> +		};
> +
> +		cpu@f01 {
> +			compatible = "arm,cortex-a7";
> +			device_type = "cpu";
> +			reg = <0xf01>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv7-timer";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | 
> IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>;
> +		clocks = <&syscon ASPEED_CLK_HPLL>;
> +		arm,cpu-registers-not-fw-configured;
> +	};
> +
> +	ahb {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		device_type = "soc";
> +		ranges;
> +
> +		gic: interrupt-controller@40461000 {
> +			compatible = "arm,cortex-a7-gic";
> +			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | 
> IRQ_TYPE_LEVEL_HIGH)>;
> +			#interrupt-cells = <3>;
> +			interrupt-controller;
> +			interrupt-parent = <&gic>;
> +			reg = <0x40461000 0x1000>,
> +			    <0x40462000 0x1000>,
> +			    <0x40464000 0x2000>,
> +			    <0x40466000 0x2000>;
> +			};
> +
> +		mdio0: mdio@1e650000 {
> +			compatible = "aspeed,ast2600-mdio";
> +			reg = <0x1e650000 0x8>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		mdio1: mdio@1e650008 {
> +			compatible = "aspeed,ast2600-mdio";
> +			reg = <0x1e650008 0x8>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		mdio2: mdio@1e650010 {
> +			compatible = "aspeed,ast2600-mdio";
> +			reg = <0x1e650010 0x8>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		mdio3: mdio@1e650018 {
> +			compatible = "aspeed,ast2600-mdio";
> +			reg = <0x1e650018 0x8>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		mac0: ftgmac@1e660000 {
> +			compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> +			reg = <0x1e660000 0x180>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>;
> +			status = "disabled";
> +		};
> +
> +		mac1: ftgmac@1e680000 {
> +			compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> +			reg = <0x1e680000 0x180>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&syscon ASPEED_CLK_GATE_MAC2CLK>;
> +			status = "disabled";
> +		};
> +
> +		mac2: ftgmac@1e670000 {
> +			compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> +			reg = <0x1e670000 0x180>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&syscon ASPEED_CLK_GATE_MAC3CLK>;
> +			status = "disabled";
> +		};
> +
> +		mac3: ftgmac@1e690000 {
> +			compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> +			reg = <0x1e690000 0x180>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&syscon ASPEED_CLK_GATE_MAC4CLK>;
> +			status = "disabled";
> +		};
> +
> +		apb {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			syscon: syscon@1e6e2000 {
> +				compatible = "aspeed,ast2600-scu", "syscon", "simple-mfd";
> +				reg = <0x1e6e2000 0x1000>;
> +				ranges = <0 0x1e6e2000 0x1000>;
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				#clock-cells = <1>;
> +				#reset-cells = <1>;
> +
> +				pinctrl: pinctrl {
> +					compatible = "aspeed,ast2600-pinctrl";
> +				};
> +
> +				smp-memram@180 {
> +					compatible = "aspeed,ast2600-smpmem";
> +					reg = <0x180 0x40>;
> +				};
> +			};
> +
> +			rng: hwrng@1e6e2524 {
> +				compatible = "timeriomem_rng";
> +				reg = <0x1e6e2524 0x4>;
> +				period = <1>;
> +				quality = <100>;
> +			};
> +
> +			rtc: rtc@1e781000 {
> +				compatible = "aspeed,ast2600-rtc";
> +				reg = <0x1e781000 0x18>;
> +				interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			uart5: serial@1e784000 {
> +				compatible = "ns16550a";
> +				reg = <0x1e784000 0x1000>;
> +				reg-shift = <2>;
> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +				clocks = <&syscon ASPEED_CLK_GATE_UART5CLK>;
> +				no-loopback-test;
> +			};
> +
> +			wdt1: watchdog@1e785000 {
> +				compatible = "aspeed,ast2600-wdt";
> +				reg = <0x1e785000 0x40>;
> +			};
> +
> +			wdt2: watchdog@1e785040 {
> +				compatible = "aspeed,ast2600-wdt";
> +				reg = <0x1e785040 0x40>;
> +				status = "disabled";
> +			};
> +
> +			wdt3: watchdog@1e785080 {
> +				compatible = "aspeed,ast2600-wdt";
> +				reg = <0x1e785080 0x40>;
> +				status = "disabled";
> +			};
> +
> +			wdt4: watchdog@1e7850C0 {
> +				compatible = "aspeed,ast2600-wdt";
> +				reg = <0x1e7850C0 0x40>;
> +				status = "disabled";
> +			};
> +
> +			sdc: sdc@1e740000 {
> +				compatible = "aspeed,ast2600-sd-controller";
> +				reg = <0x1e740000 0x100>;
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0 0x1e740000 0x10000>;
> +				clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> +				status = "disabled";
> +
> +				sdhci0: sdhci@1e740100 {
> +					compatible = "aspeed,ast2600-sdhci", "sdhci";
> +					reg = <0x100 0x100>;
> +					interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> +					sdhci,auto-cmd12;
> +					clocks = <&syscon ASPEED_CLK_SDIO>;
> +					status = "disabled";
> +				};
> +
> +				sdhci1: sdhci@1e740200 {
> +					compatible = "aspeed,ast2600-sdhci", "sdhci";
> +					reg = <0x200 0x100>;
> +					interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> +					sdhci,auto-cmd12;
> +					clocks = <&syscon ASPEED_CLK_SDIO>;
> +					status = "disabled";
> +				};
> +			};
> +
> +			emmc: sdc@1e750000 {
> +				compatible = "aspeed,ast2600-sd-controller";
> +				reg = <0x1e750000 0x100>;
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0 0x1e750000 0x10000>;
> +				clocks = <&syscon ASPEED_CLK_GATE_EMMCCLK>;
> +				status = "disabled";
> +
> +				sdhci@1e750100 {
> +					compatible = "aspeed,ast2600-sdhci";
> +					reg = <0x100 0x100>;
> +					sdhci,auto-cmd12;
> +					interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +					clocks = <&syscon ASPEED_CLK_EMMC>;
> +					pinctrl-names = "default";
> +					pinctrl-0 = <&pinctrl_emmc_default>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +&pinctrl {
> +	pinctrl_emmc_default: emmc_default {
> +		function = "SD3";
> +		groups = "SD3";
> +	};

I need to send some fixes for pinmux along with the dt patche, but this
will do for the moment.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 6/7] ARM: configs: multi_v7: Add ASPEED G6
From: Andrew Jeffery @ 2019-08-22  2:04 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
  Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
	Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-7-joel@jms.id.au>



On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> This adds the ASPEED AST2600 system and associated ASPEED devices so we
> get build coverage.
> 
> The changes to the UART configuration to ensure the default console
> (UART5) works.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Acked-by: Andrew Jeffery <andrew@aj.id.au>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 7/7] ARM: configs: aspeed_g5: Enable AST2600
From: Andrew Jeffery @ 2019-08-22  2:07 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
  Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
	Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-8-joel@jms.id.au>



On Wed, 21 Aug 2019, at 15:26, Joel Stanley wrote:
> CONFIG_STRICT_KERNEL_RWX is enabled by default with ARMv7.
> 
> Turn on HIGHMEM as the EVB has 2GB of RAM, and not all is usable without
> hihgmem.
> 
> The SoC contains Cortex A7 supporting VFP and has two CPUs.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  arch/arm/configs/aspeed_g5_defconfig | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/configs/aspeed_g5_defconfig 
> b/arch/arm/configs/aspeed_g5_defconfig
> index 426d8e0c9890..597536cc9573 100644
> --- a/arch/arm/configs/aspeed_g5_defconfig
> +++ b/arch/arm/configs/aspeed_g5_defconfig
> @@ -21,21 +21,26 @@ CONFIG_PERF_EVENTS=y
>  CONFIG_SLAB=y
>  CONFIG_SLAB_FREELIST_RANDOM=y
>  CONFIG_ARCH_MULTI_V6=y
> -# CONFIG_ARCH_MULTI_V7 is not set
>  CONFIG_ARCH_ASPEED=y
>  CONFIG_MACH_ASPEED_G5=y
> +CONFIG_MACH_ASPEED_G6=y
>  # CONFIG_CACHE_L2X0 is not set
> +CONFIG_SMP=y
> +# CONFIG_ARM_CPU_TOPOLOGY is not set
>  CONFIG_VMSPLIT_2G=y
> +CONFIG_NR_CPUS=2
> +CONFIG_HIGHMEM=y
>  CONFIG_UACCESS_WITH_MEMCPY=y
>  CONFIG_SECCOMP=y
>  # CONFIG_ATAGS is not set
>  CONFIG_ZBOOT_ROM_TEXT=0x0
>  CONFIG_ZBOOT_ROM_BSS=0x0
>  CONFIG_KEXEC=y
> -# CONFIG_SUSPEND is not set
> +CONFIG_VFP=y
> +CONFIG_NEON=y
> +CONFIG_KERNEL_MODE_NEON=y
>  CONFIG_FIRMWARE_MEMMAP=y
>  CONFIG_JUMP_LABEL=y
> -CONFIG_STRICT_KERNEL_RWX=y
>  # CONFIG_BLK_DEV_BSG is not set
>  # CONFIG_BLK_DEBUG_FS is not set
>  # CONFIG_MQ_IOSCHED_DEADLINE is not set
> @@ -140,10 +145,12 @@ CONFIG_ASPEED_BT_IPMI_BMC=y
>  CONFIG_HW_RANDOM_TIMERIOMEM=y
>  # CONFIG_I2C_COMPAT is not set
>  CONFIG_I2C_CHARDEV=y
> +CONFIG_I2C_MUX=y
>  CONFIG_I2C_MUX_PCA9541=y
>  CONFIG_I2C_MUX_PCA954x=y
>  CONFIG_I2C_ASPEED=y
>  CONFIG_I2C_FSI=y
> +CONFIG_SPI=y
>  CONFIG_GPIOLIB=y
>  CONFIG_GPIO_SYSFS=y
>  CONFIG_GPIO_ASPEED=y
> @@ -194,6 +201,10 @@ CONFIG_USB_CONFIGFS_F_LB_SS=y
>  CONFIG_USB_CONFIGFS_F_FS=y
>  CONFIG_USB_CONFIGFS_F_HID=y
>  CONFIG_USB_CONFIGFS_F_PRINTER=y
> +CONFIG_MMC=y
> +CONFIG_MMC_SDHCI=y
> +CONFIG_MMC_SDHCI_PLTFM=y
> +CONFIG_MMC_SDHCI_OF_ASPEED=y

The patches haven't yet been applied to the MMC tree, maybe we should
add this later?

Anyway,

Acked-by: Andrew Jeffery <andrew@aj.id.au>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 1/7] dt-bindings: arm: cpus: Add ASPEED SMP
From: Andrew Jeffery @ 2019-08-22  2:08 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Arnd Bergmann, Olof Johansson
  Cc: Mark Rutland, devicetree, Ryan Chen, linux-aspeed,
	Cédric Le Goater, linux-arm-kernel
In-Reply-To: <20190821055530.8720-2-joel@jms.id.au>



On Wed, 21 Aug 2019, at 15:25, Joel Stanley wrote:
> The AST2600 SoC contains two CPUs and requires the operating system to
> bring the second one out of firmware.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH v8 04/14] media: rkisp1: add Rockchip MIPI Synopsys DPHY driver
From: Laurent Pinchart @ 2019-08-22  2:32 UTC (permalink / raw)
  To: Helen Koike
  Cc: devicetree, eddie.cai.linux, kernel, heiko, jacob2.chen,
	jeffy.chen, zyc, linux-kernel, tfiga, linux-rockchip,
	Sakari Ailus, sakari.ailus, zhengsq, hans.verkuil, mchehab,
	ezequiel, linux-arm-kernel, linux-media
In-Reply-To: <28b05c0e-2f68-aecb-536a-c543bcd43de1@collabora.com>

Hi Helen,

On Wed, Aug 21, 2019 at 06:46:15PM -0300, Helen Koike wrote:
> On 8/15/19 2:54 PM, Laurent Pinchart wrote:
> > On Wed, Aug 07, 2019 at 10:37:55AM -0300, Helen Koike wrote:
> >> On 8/7/19 10:05 AM, Sakari Ailus wrote:
> >>> On Tue, Jul 30, 2019 at 03:42:46PM -0300, Helen Koike wrote:
> >>>> From: Jacob Chen <jacob2.chen@rock-chips.com>
> >>>>
> >>>> This commit adds a subdev driver for Rockchip MIPI Synopsys DPHY driver
> >>>>
> >>>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> >>>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> >>>> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> >>>> [migrate to phy framework]
> >>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> >>>> [update for upstream]
> >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>
> >>>> ---
> >>>>
> >>>> Changes in v8:
> >>>> - Remove boiler plate license text
> >>>>
> >>>> Changes in v7:
> >>>> - Migrate dphy specific code from
> >>>> drivers/media/platform/rockchip/isp1/mipi_dphy_sy.c
> >>>> to drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>> - Drop support for rk3288
> >>>> - Drop support for dphy txrx
> >>>> - code styling and checkpatch fixes
> >>>>
> >>>>  drivers/phy/rockchip/Kconfig             |   8 +
> >>>>  drivers/phy/rockchip/Makefile            |   1 +
> >>>>  drivers/phy/rockchip/phy-rockchip-dphy.c | 408 +++++++++++++++++++++++
> >>>>  3 files changed, 417 insertions(+)
> >>>>  create mode 100644 drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>>
> >>>> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> >>>> index c454c90cd99e..afd072f135e6 100644
> >>>> --- a/drivers/phy/rockchip/Kconfig
> >>>> +++ b/drivers/phy/rockchip/Kconfig
> >>>> @@ -9,6 +9,14 @@ config PHY_ROCKCHIP_DP
> >>>>  	help
> >>>>  	  Enable this to support the Rockchip Display Port PHY.
> >>>>  
> >>>> +config PHY_ROCKCHIP_DPHY
> >>>> +	tristate "Rockchip MIPI Synopsys DPHY driver"
> > 
> > How much of this PHY is Rockchip-specific ? Would it make sense to turn
> > it into a Synopsys DPHY driver, with some Rockchip glue ? I suppose this
> > could always be done later, if needed (and I also suppose there's no
> > existing driver in drivers/phy/ that support the same Synopsys IP).
> > 
> >>>> +	depends on ARCH_ROCKCHIP && OF
> >>>
> >>> How about (...) || COMPILE_TEST ?
> >>>
> >>>> +	select GENERIC_PHY_MIPI_DPHY
> >>>> +	select GENERIC_PHY
> >>>> +	help
> >>>> +	  Enable this to support the Rockchip MIPI Synopsys DPHY.
> >>>> +
> >>>>  config PHY_ROCKCHIP_EMMC
> >>>>  	tristate "Rockchip EMMC PHY Driver"
> >>>>  	depends on ARCH_ROCKCHIP && OF
> >>>> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
> >>>> index fd21cbaf40dd..f62e9010bcaf 100644
> >>>> --- a/drivers/phy/rockchip/Makefile
> >>>> +++ b/drivers/phy/rockchip/Makefile
> >>>> @@ -1,5 +1,6 @@
> >>>>  # SPDX-License-Identifier: GPL-2.0
> >>>>  obj-$(CONFIG_PHY_ROCKCHIP_DP)		+= phy-rockchip-dp.o
> >>>> +obj-$(CONFIG_PHY_ROCKCHIP_DPHY)		+= phy-rockchip-dphy.o
> >>>>  obj-$(CONFIG_PHY_ROCKCHIP_EMMC)		+= phy-rockchip-emmc.o
> >>>>  obj-$(CONFIG_PHY_ROCKCHIP_INNO_HDMI)	+= phy-rockchip-inno-hdmi.o
> >>>>  obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2)	+= phy-rockchip-inno-usb2.o
> >>>> diff --git a/drivers/phy/rockchip/phy-rockchip-dphy.c b/drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>> new file mode 100644
> >>>> index 000000000000..3a29976c2dff
> >>>> --- /dev/null
> >>>> +++ b/drivers/phy/rockchip/phy-rockchip-dphy.c
> >>>> @@ -0,0 +1,408 @@
> >>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >>>> +/*
> >>>> + * Rockchip MIPI Synopsys DPHY driver
> >>>> + *
> >>>> + * Based on:
> >>>> + *
> >>>> + * Copyright (C) 2016 FuZhou Rockchip Co., Ltd.
> >>>> + * Author: Yakir Yang <ykk@@rock-chips.com>
> >>>> + */
> >>>> +
> >>>> +#include <linux/clk.h>
> >>>> +#include <linux/io.h>
> >>>> +#include <linux/mfd/syscon.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_device.h>
> >>>> +#include <linux/phy/phy.h>
> >>>> +#include <linux/phy/phy-mipi-dphy.h>
> >>>> +#include <linux/platform_device.h>
> >>>> +#include <linux/regmap.h>
> >>>> +
> >>>> +#define RK3399_GRF_SOC_CON9	0x6224
> >>>> +#define RK3399_GRF_SOC_CON21	0x6254
> >>>> +#define RK3399_GRF_SOC_CON22	0x6258
> >>>> +#define RK3399_GRF_SOC_CON23	0x625c
> >>>> +#define RK3399_GRF_SOC_CON24	0x6260
> >>>> +#define RK3399_GRF_SOC_CON25	0x6264
> >>>> +#define RK3399_GRF_SOC_STATUS1	0xe2a4
> >>>> +
> >>>> +#define CLOCK_LANE_HS_RX_CONTROL		0x34
> >>>> +#define LANE0_HS_RX_CONTROL			0x44
> >>>> +#define LANE1_HS_RX_CONTROL			0x54
> >>>> +#define LANE2_HS_RX_CONTROL			0x84
> >>>> +#define LANE3_HS_RX_CONTROL			0x94
> >>>> +#define HS_RX_DATA_LANES_THS_SETTLE_CONTROL	0x75
> >>>> +
> >>>> +#define MAX_DPHY_CLK 8
> >>>> +
> >>>> +#define PHY_TESTEN_ADDR			(0x1 << 16)
> >>>> +#define PHY_TESTEN_DATA			(0x0 << 16)
> >>>> +#define PHY_TESTCLK			(0x1 << 1)
> >>>> +#define PHY_TESTCLR			(0x1 << 0)
> > 
> > Maybe s/0x// for the previous four lines ?
> > 
> >>>> +#define THS_SETTLE_COUNTER_THRESHOLD	0x04
> >>>> +
> >>>> +#define HIWORD_UPDATE(val, mask, shift) \
> >>>> +	((val) << (shift) | (mask) << ((shift) + 16))
> > 
> > As you use this in a single place, I would inline it, possibly with a
> > small comment that explains what's happening.
> > 
> >>>> +
> >>>> +#define GRF_SOC_CON12                           0x0274
> >>>> +
> >>>> +#define GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK   BIT(20)
> >>>> +#define GRF_EDP_REF_CLK_SEL_INTER               BIT(4)
> >>>> +
> >>>> +#define GRF_EDP_PHY_SIDDQ_HIWORD_MASK           BIT(21)
> >>>> +#define GRF_EDP_PHY_SIDDQ_ON                    0
> >>>> +#define GRF_EDP_PHY_SIDDQ_OFF                   BIT(5)
> > 
> > I would recommend aligning the value of of all macros in the same way.
> > 
> >>>> +
> >>>> +struct hsfreq_range {
> >>>> +	u32 range_h;
> > 
> > The structure would be more compact if you turned this into a u16.
> > 
> >>>> +	u8 cfg_bit;
> >>>> +};
> >>>> +
> >>>> +static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
> >>>> +	{  89, 0x00}, {  99, 0x10}, { 109, 0x20}, { 129, 0x01},
> >>>> +	{ 139, 0x11}, { 149, 0x21}, { 169, 0x02}, { 179, 0x12},
> >>>> +	{ 199, 0x22}, { 219, 0x03}, { 239, 0x13}, { 249, 0x23},
> >>>> +	{ 269, 0x04}, { 299, 0x14}, { 329, 0x05}, { 359, 0x15},
> >>>> +	{ 399, 0x25}, { 449, 0x06}, { 499, 0x16}, { 549, 0x07},
> >>>> +	{ 599, 0x17}, { 649, 0x08}, { 699, 0x18}, { 749, 0x09},
> >>>> +	{ 799, 0x19}, { 849, 0x29}, { 899, 0x39}, { 949, 0x0a},
> >>>> +	{ 999, 0x1a}, {1049, 0x2a}, {1099, 0x3a}, {1149, 0x0b},
> >>>> +	{1199, 0x1b}, {1249, 0x2b}, {1299, 0x3b}, {1349, 0x0c},
> >>>> +	{1399, 0x1c}, {1449, 0x2c}, {1500, 0x3c}
> > 
> > Maybe s/{/{ / and s/}/ }/ to give it a bit more air ? :-)
> > 
> >>>> +};
> >>>> +
> >>>> +static const char * const rk3399_mipidphy_clks[] = {
> >>>> +	"dphy-ref",
> >>>> +	"dphy-cfg",
> >>>> +	"grf",
> >>>> +};
> >>>> +
> >>>> +enum dphy_reg_id {
> >>>> +	GRF_DPHY_RX0_TURNDISABLE = 0,
> >>>> +	GRF_DPHY_RX0_FORCERXMODE,
> >>>> +	GRF_DPHY_RX0_FORCETXSTOPMODE,
> >>>> +	GRF_DPHY_RX0_ENABLE,
> >>>> +	GRF_DPHY_RX0_TESTCLR,
> >>>> +	GRF_DPHY_RX0_TESTCLK,
> >>>> +	GRF_DPHY_RX0_TESTEN,
> >>>> +	GRF_DPHY_RX0_TESTDIN,
> >>>> +	GRF_DPHY_RX0_TURNREQUEST,
> >>>> +	GRF_DPHY_RX0_TESTDOUT,
> >>>> +	GRF_DPHY_TX0_TURNDISABLE,
> >>>> +	GRF_DPHY_TX0_FORCERXMODE,
> >>>> +	GRF_DPHY_TX0_FORCETXSTOPMODE,
> >>>> +	GRF_DPHY_TX0_TURNREQUEST,
> >>>> +	GRF_DPHY_TX1RX1_TURNDISABLE,
> >>>> +	GRF_DPHY_TX1RX1_FORCERXMODE,
> >>>> +	GRF_DPHY_TX1RX1_FORCETXSTOPMODE,
> >>>> +	GRF_DPHY_TX1RX1_ENABLE,
> >>>> +	GRF_DPHY_TX1RX1_MASTERSLAVEZ,
> >>>> +	GRF_DPHY_TX1RX1_BASEDIR,
> >>>> +	GRF_DPHY_TX1RX1_ENABLECLK,
> >>>> +	GRF_DPHY_TX1RX1_TURNREQUEST,
> >>>> +	GRF_DPHY_RX1_SRC_SEL,
> >>>> +	/* rk3288 only */
> >>>> +	GRF_CON_DISABLE_ISP,
> >>>> +	GRF_CON_ISP_DPHY_SEL,
> >>>> +	GRF_DSI_CSI_TESTBUS_SEL,
> >>>> +	GRF_DVP_V18SEL,
> >>>> +	/* below is for rk3399 only */
> >>>> +	GRF_DPHY_RX0_CLK_INV_SEL,
> >>>> +	GRF_DPHY_RX1_CLK_INV_SEL,
> >>>> +};
> >>>> +
> >>>> +struct dphy_reg {
> >>>> +	u32 offset;
> >>>> +	u32 mask;
> >>>> +	u32 shift;
> > 
> > The offset should hold in 16 bits and the mask and shift in 8 bits. That
> > would save space in the table below.
> > 
> >>>> +};
> >>>> +
> >>>> +#define PHY_REG(_offset, _width, _shift) \
> >>>> +	{ .offset = _offset, .mask = BIT(_width) - 1, .shift = _shift, }
> >>>> +
> >>>> +static const struct dphy_reg rk3399_grf_dphy_regs[] = {
> >>>> +	[GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON9, 4, 0),
> >>>> +	[GRF_DPHY_RX0_CLK_INV_SEL] = PHY_REG(RK3399_GRF_SOC_CON9, 1, 10),
> >>>> +	[GRF_DPHY_RX1_CLK_INV_SEL] = PHY_REG(RK3399_GRF_SOC_CON9, 1, 11),
> >>>> +	[GRF_DPHY_RX0_ENABLE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 0),
> >>>> +	[GRF_DPHY_RX0_FORCERXMODE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 4),
> >>>> +	[GRF_DPHY_RX0_FORCETXSTOPMODE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 8),
> >>>> +	[GRF_DPHY_RX0_TURNDISABLE] = PHY_REG(RK3399_GRF_SOC_CON21, 4, 12),
> >>>> +	[GRF_DPHY_TX0_FORCERXMODE] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 0),
> >>>> +	[GRF_DPHY_TX0_FORCETXSTOPMODE] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 4),
> >>>> +	[GRF_DPHY_TX0_TURNDISABLE] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 8),
> >>>> +	[GRF_DPHY_TX0_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON22, 4, 12),
> >>>> +	[GRF_DPHY_TX1RX1_ENABLE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 0),
> >>>> +	[GRF_DPHY_TX1RX1_FORCERXMODE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 4),
> >>>> +	[GRF_DPHY_TX1RX1_FORCETXSTOPMODE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 8),
> >>>> +	[GRF_DPHY_TX1RX1_TURNDISABLE] = PHY_REG(RK3399_GRF_SOC_CON23, 4, 12),
> >>>> +	[GRF_DPHY_TX1RX1_TURNREQUEST] = PHY_REG(RK3399_GRF_SOC_CON24, 4, 0),
> >>>> +	[GRF_DPHY_RX1_SRC_SEL] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 4),
> >>>> +	[GRF_DPHY_TX1RX1_BASEDIR] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 5),
> >>>> +	[GRF_DPHY_TX1RX1_ENABLECLK] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 6),
> >>>> +	[GRF_DPHY_TX1RX1_MASTERSLAVEZ] = PHY_REG(RK3399_GRF_SOC_CON24, 1, 7),
> >>>> +	[GRF_DPHY_RX0_TESTDIN] = PHY_REG(RK3399_GRF_SOC_CON25, 8, 0),
> >>>> +	[GRF_DPHY_RX0_TESTEN] = PHY_REG(RK3399_GRF_SOC_CON25, 1, 8),
> >>>> +	[GRF_DPHY_RX0_TESTCLK] = PHY_REG(RK3399_GRF_SOC_CON25, 1, 9),
> >>>> +	[GRF_DPHY_RX0_TESTCLR] = PHY_REG(RK3399_GRF_SOC_CON25, 1, 10),
> >>>> +	[GRF_DPHY_RX0_TESTDOUT] = PHY_REG(RK3399_GRF_SOC_STATUS1, 8, 0),
> > 
> > The annoying part with such an indirection is that you can't really
> > write multiple fields in a single register with a single operation.
> > Is the register mapping completely different between the rk3288 and the
> > rk3399, or are the fields grouped in registers in a similar way ? In the
> > latter case we could possibly optimise it.
> 
> This would be the rk3288 version:
> 
> +static const struct dphy_reg rk3288_grf_dphy_regs[] = {
> +	[GRF_CON_DISABLE_ISP] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 0),
> +	[GRF_CON_ISP_DPHY_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 1),
> +	[GRF_DSI_CSI_TESTBUS_SEL] = PHY_REG(RK3288_GRF_SOC_CON6, 1, 14),
> +	[GRF_DPHY_TX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 0),
> +	[GRF_DPHY_TX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 4),
> +	[GRF_DPHY_TX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON8, 4, 8),
> +	[GRF_DPHY_TX1RX1_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 0),
> +	[GRF_DPHY_TX1RX1_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 4),
> +	[GRF_DPHY_TX1RX1_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 8),
> +	[GRF_DPHY_TX1RX1_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON9, 4, 12),
> +	[GRF_DPHY_RX0_TURNDISABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 0),
> +	[GRF_DPHY_RX0_FORCERXMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 4),
> +	[GRF_DPHY_RX0_FORCETXSTOPMODE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 8),
> +	[GRF_DPHY_RX0_ENABLE] = PHY_REG(RK3288_GRF_SOC_CON10, 4, 12),
> +	[GRF_DPHY_RX0_TESTCLR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 0),
> +	[GRF_DPHY_RX0_TESTCLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 1),
> +	[GRF_DPHY_RX0_TESTEN] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 2),
> +	[GRF_DPHY_RX0_TESTDIN] = PHY_REG(RK3288_GRF_SOC_CON14, 8, 3),
> +	[GRF_DPHY_TX1RX1_ENABLECLK] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 12),
> +	[GRF_DPHY_RX1_SRC_SEL] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 13),
> +	[GRF_DPHY_TX1RX1_MASTERSLAVEZ] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 14),
> +	[GRF_DPHY_TX1RX1_BASEDIR] = PHY_REG(RK3288_GRF_SOC_CON14, 1, 15),
> +	[GRF_DPHY_RX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 0),
> +	[GRF_DPHY_TX1RX1_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 4, 4),
> +	[GRF_DPHY_TX0_TURNREQUEST] = PHY_REG(RK3288_GRF_SOC_CON15, 3, 8),
> +	[GRF_DVP_V18SEL] = PHY_REG(RK3288_GRF_IO_VSEL, 1, 1),
> +	[GRF_DPHY_RX0_TESTDOUT] = PHY_REG(RK3288_GRF_SOC_STATUS21, 8, 0),
> +};
> 
> Which seems different mask and shifts from rk3399. If you have any ideas in
> how to optimize this I would appreciate it.

It would be tricky indeed :-( Nevermind for now.

> >>>> +};
> >>>> +
> >>>> +struct dphy_drv_data {
> >>>> +	const char * const *clks;
> >>>> +	int num_clks;
> > 
> > This is never negative, you can make it an unsigned int.
> > 
> >>>> +	const struct hsfreq_range *hsfreq_ranges;
> >>>> +	int num_hsfreq_ranges;
> > 
> > Same here.
> > 
> >>>> +	const struct dphy_reg *regs;
> >>>> +};
> >>>> +
> >>>> +struct rockchip_dphy {
> >>>> +	struct device *dev;
> >>>> +	struct regmap *grf;
> >>>> +	const struct dphy_reg *grf_regs;
> >>>> +	struct clk_bulk_data clks[MAX_DPHY_CLK];
> >>>> +
> >>>> +	const struct dphy_drv_data *drv_data;
> >>>> +	struct phy_configure_opts_mipi_dphy config;
> >>>> +};
> >>>> +
> >>>> +static inline void write_grf_reg(struct rockchip_dphy *priv,
> >>>> +				 int index, u8 value)
> > 
> > Maybe unsigned int index ?
> > 
> >>>> +{
> >>>> +	const struct dphy_reg *reg = &priv->grf_regs[index];
> >>>> +	unsigned int val = HIWORD_UPDATE(value, reg->mask, reg->shift);
> >>>> +
> >>>> +	WARN_ON(!reg->offset);
> >>>> +	regmap_write(priv->grf, reg->offset, val);
> >>>> +}
> >>>> +
> >>>> +static void mipidphy0_wr_reg(struct rockchip_dphy *priv,
> >>>> +			     u8 test_code, u8 test_data)
> > 
> > Function (and structure) names have different prefixes, would it make
> > sense to standardise them ? Maybe rockchip_dphy_ ? Or rk_dphy_ for a
> > shorter version ? This could become rk_dphy_write_dphy(), and the
> > previous function rk_dphy_write_grf().
> > 
> >>>> +{
> >>>> +	/*
> >>>> +	 * With the falling edge on TESTCLK, the TESTDIN[7:0] signal content
> >>>> +	 * is latched internally as the current test code. Test data is
> >>>> +	 * programmed internally by rising edge on TESTCLK.
> >>>> +	 */
> > 
> > I've never understood why PHYs tend to have a register named TEST that
> > contains way more than test data :-)
> > 
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 1);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTDIN, test_code);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTEN, 1);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 0);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTEN, 0);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTDIN, test_data);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 1);
> >>>> +}
> >>>> +
> >>>> +/* should be move to power_on */
> > 
> > s/move/moved/
> > 
> > Do you mean merging the two functions together ? What prevents from
> > doing so ? 
> 
> Nothing really, this is a left over command as mipidphy_rx_stream_on() is already
> being called from power_on, and I don't think we should merge it because
> in the future we'll probably going to have mipidphy_txrx_stream_on() for dphy1.

Fine with me, let's just remove the comment then.

> >>>> +static int mipidphy_rx_stream_on(struct rockchip_dphy *priv)
> >>>> +{
> >>>> +	const struct dphy_drv_data *drv_data = priv->drv_data;
> >>>> +	const struct hsfreq_range *hsfreq_ranges = drv_data->hsfreq_ranges;
> >>>> +	struct phy_configure_opts_mipi_dphy *config = &priv->config;
> >>>> +	unsigned int i, hsfreq = 0, data_rate_mbps = config->hs_clk_rate;
> >>>> +	int num_hsfreq_ranges = drv_data->num_hsfreq_ranges;
> >>>> +
> >>>> +	do_div(data_rate_mbps, 1000 * 1000);
> >>>> +
> >>>> +	dev_dbg(priv->dev, "%s: lanes %d - data_rate_mbps %u\n",
> >>>> +		__func__, config->lanes, data_rate_mbps);
> >>>> +
> >>>> +	for (i = 0; i < num_hsfreq_ranges; i++) {
> >>>> +		if (hsfreq_ranges[i].range_h >= data_rate_mbps) {
> >>>> +			hsfreq = hsfreq_ranges[i].cfg_bit;
> >>>> +			break;
> >>>> +		}
> >>>> +	}
> > 
> > As num_hsfreq_ranges and hsfreq_ranges are only used in this loop, I
> > would remove the local variables.
> > 
> >>>> +
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_FORCERXMODE, 0);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_FORCETXSTOPMODE, 0);
> >>>> +
> >>>> +	/* Disable lan turn around, which is ignored in receive mode */
> > 
> > Is it "lan turn around", or "lane turn around" ?
> > 
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TURNREQUEST, 0);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TURNDISABLE, 0xf);
> >>>> +
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_ENABLE, GENMASK(config->lanes - 1, 0));
> >>>> +
> >>>> +	/* dphy start */
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTCLK, 1);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTCLR, 1);
> >>>> +	usleep_range(100, 150);
> >>>> +	write_grf_reg(priv, GRF_DPHY_RX0_TESTCLR, 0);
> >>>> +	usleep_range(100, 150);
> >>>> +
> >>>> +	/* set clock lane */
> >>>> +	/* HS hsfreq_range & lane 0  settle bypass */
> >>>> +	mipidphy0_wr_reg(priv, CLOCK_LANE_HS_RX_CONTROL, 0);
> >>>> +	/* HS RX Control of lane0 */
> >>>> +	mipidphy0_wr_reg(priv, LANE0_HS_RX_CONTROL, hsfreq << 1);
> >>>> +	/* HS RX Control of lane1 */
> >>>> +	mipidphy0_wr_reg(priv, LANE1_HS_RX_CONTROL, 0);
> >>>> +	/* HS RX Control of lane2 */
> >>>> +	mipidphy0_wr_reg(priv, LANE2_HS_RX_CONTROL, 0);
> >>>> +	/* HS RX Control of lane3 */
> >>>> +	mipidphy0_wr_reg(priv, LANE3_HS_RX_CONTROL, 0);
> > 
> > Does this hardcode usage of a single lane ?
> 
> Rockchip seems to uses TEST* registers to set the hsfreqrange.
> It mentions the test code 0x44 (which is LANE0_HS_RX_CONTROL)
> but it doesn't mention the others lanes.
> 
> Replacing those call by
> mipidphy0_wr_reg(priv, LANEx_HS_RX_CONTROL, hsfreq << 1);
> seems to be working.
> 
> I can check if this changes the datarate (I just need to figure a proper
> way to test this or get some docs).
> 
> Thanks for spotting this.

We've discussed this on IRC, it's not clear if the above code is
incorrect or not. Let's add this to a list of open issues.

> >>>> +	/* HS RX Data Lanes Settle State Time Control */
> >>>> +	mipidphy0_wr_reg(priv, HS_RX_DATA_LANES_THS_SETTLE_CONTROL,
> >>>> +			 THS_SETTLE_COUNTER_THRESHOLD);
> >>>> +
> >>>> +	/* Normal operation */
> >>>> +	mipidphy0_wr_reg(priv, 0x0, 0);
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> >>>> +{
> >>>> +	struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +	int ret;
> >>>> +
> >>>> +	/* pass with phy_mipi_dphy_get_default_config (with pixel rate?) */
> > 
> > I'm not sure to understand what this means.
> 
> iirc, the question is if we should fail when phy_mipi_dphy_config_validate() fails,
> or if we should use a default config.
> 
> Looking at other examples, is seems that only two drivers call
> phy_mipi_dphy_get_default_config() in a totally diferent context, not in mipi path.
> So I guess I would just remove this comment if this is ok with you.

OK.

> >>>> +	ret = phy_mipi_dphy_config_validate(&opts->mipi_dphy);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	memcpy(&priv->config, opts, sizeof(priv->config));
> >>>
> >>> You could to:
> >>>
> >>> 	priv->config = *opts;
> >>>
> >>> Up to you. Some people like memcpy(). :-)
> >>
> >> your way is better thanks!
> >>
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_power_on(struct phy *phy)
> >>>> +{
> >>>> +	struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = clk_bulk_enable(priv->drv_data->num_clks, priv->clks);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	return mipidphy_rx_stream_on(priv);
> > 
> > Should you call clk_bulk_disable() if mipidphy_rx_stream_on() fails ?
> > Actually that function never fails, so I'd make it a void function, and
> > return 0 here.
> 
> Ack, I made it void, I'll send it in the next version.
> 
> > What happens if the clock rate is higher than the maximum supported by
> > the PHY ? Shouldn't rockchip_dphy_configure() fail in that case ?
> 
> This is checked in function mipidphy_rx_stream_on(), if it is higher we just
> configure the maximum supported rate. Is this ok?

I think it would be better to reject that in rockchip_dphy_configure()
in order to let the user of the PHY handle the error as early as
possible.

> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_power_off(struct phy *phy)
> >>>> +{
> >>>> +	struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +
> > 
> > No need to write any register ? That's scary, what will happen on the
> > next power on, when the clocks gets enabled ?
> 
> Just for testing, I hacked the code to only call mipidphy_rx_stream_on() once,
> when streaming for the first time, then I don't call it anymore and starting/stopping
> streaming always works, so I guess it keeps the previous configuration when clocks
> get enabled.
> I wonder if this can be a problem when switching from dphy rx to txrx, but for now
> we just support rx.
> 
> Maybe just calling rk_dphy_write_grf(priv, GRF_DPHY_RX0_ENABLE, 0) is enough.

If that works with disable/enable sequences I think it would be good to
include it.

> >>>> +	clk_bulk_disable(priv->drv_data->num_clks, priv->clks);
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_init(struct phy *phy)
> >>>> +{
> >>>> +	struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = clk_bulk_prepare(priv->drv_data->num_clks, priv->clks);
> >>>
> >>> return ...;
> >>>
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int rockchip_dphy_exit(struct phy *phy)
> >>>> +{
> >>>> +	struct rockchip_dphy *priv = phy_get_drvdata(phy);
> >>>> +
> >>>> +	clk_bulk_unprepare(priv->drv_data->num_clks, priv->clks);
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static const struct phy_ops rockchip_dphy_ops = {
> >>>> +	.power_on	= rockchip_dphy_power_on,
> >>>> +	.power_off	= rockchip_dphy_power_off,
> >>>> +	.init		= rockchip_dphy_init,
> >>>> +	.exit		= rockchip_dphy_exit,
> >>>> +	.configure	= rockchip_dphy_configure,
> >>>> +	.owner		= THIS_MODULE,
> >>>> +};
> >>>> +
> >>>> +static const struct dphy_drv_data rk3399_mipidphy_drv_data = {
> >>>> +	.clks = rk3399_mipidphy_clks,
> >>>> +	.num_clks = ARRAY_SIZE(rk3399_mipidphy_clks),
> >>>> +	.hsfreq_ranges = rk3399_mipidphy_hsfreq_ranges,
> >>>> +	.num_hsfreq_ranges = ARRAY_SIZE(rk3399_mipidphy_hsfreq_ranges),
> >>>> +	.regs = rk3399_grf_dphy_regs,
> >>>
> >>> Do you expect to support more of the similar PHY(s) --- are there such? If
> >>> not, you could put these in the code that uses them.
> >>
> >> Yes, for rk3288 in the future.
> >>
> >>>> +};
> >>>> +
> >>>> +static const struct of_device_id rockchip_dphy_dt_ids[] = {
> >>>> +	{
> >>>> +		.compatible = "rockchip,rk3399-mipi-dphy",
> >>>> +		.data = &rk3399_mipidphy_drv_data,
> >>>> +	},
> >>>> +	{}
> >>>> +};
> >>>> +MODULE_DEVICE_TABLE(of, rockchip_dphy_dt_ids);
> >>>> +
> >>>> +static int rockchip_dphy_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct device *dev = &pdev->dev;
> >>>> +	struct device_node *np = dev->of_node;
> >>>> +	const struct dphy_drv_data *drv_data;
> >>>> +	struct phy_provider *phy_provider;
> >>>> +	const struct of_device_id *of_id;
> >>>> +	struct rockchip_dphy *priv;
> >>>> +	struct regmap *grf;
> >>>> +	struct phy *phy;
> >>>> +	unsigned int i;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (!dev->parent || !dev->parent->of_node)
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	if (platform_get_resource(pdev, IORESOURCE_MEM, 0)) {
> >>>> +		dev_err(&pdev->dev, "Rockchip DPHY driver only suports rx\n");
> > 
> > You can replace pdev->dev with dev here and below.
> > 
> > s/rx/RX mode/ ?
> > 
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>>> +	if (!priv)
> >>>> +		return -ENOMEM;
> >>>> +	priv->dev = dev;
> >>>> +
> >>>> +	grf = syscon_node_to_regmap(dev->parent->of_node);
> >>>> +	if (IS_ERR(grf)) {
> >>>> +		grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>>> +						      "rockchip,grf");
> >>>> +		if (IS_ERR(grf)) {
> >>>> +			dev_err(dev, "Can't find GRF syscon\n");
> >>>> +			return -ENODEV;
> >>>> +		}
> >>>> +	}
> >>>> +	priv->grf = grf;
> >>>> +
> >>>> +	of_id = of_match_device(rockchip_dphy_dt_ids, dev);
> >>>> +	if (!of_id)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	drv_data = of_id->data;
> >>>> +	priv->grf_regs = drv_data->regs;
> > 
> > Do you have to store grf_regs in priv, or could it be accessed through
> > priv->drv_data->regs ?
> > 
> >>>> +	priv->drv_data = drv_data;
> >>>> +	for (i = 0; i < drv_data->num_clks; i++)
> >>>> +		priv->clks[i].id = drv_data->clks[i];
> >>>> +	ret = devm_clk_bulk_get(&pdev->dev, drv_data->num_clks, priv->clks);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	phy = devm_phy_create(dev, np, &rockchip_dphy_ops);
> >>>> +	if (IS_ERR(phy)) {
> >>>> +		dev_err(dev, "failed to create phy\n");
> >>>> +		return PTR_ERR(phy);
> >>>> +	}
> >>>> +	phy_set_drvdata(phy, priv);
> >>>> +
> >>>> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> >>>> +
> >>>> +	return PTR_ERR_OR_ZERO(phy_provider);
> >>>> +}
> >>>> +
> >>>> +static struct platform_driver rockchip_dphy_driver = {
> >>>> +	.probe = rockchip_dphy_probe,
> >>>> +	.driver = {
> >>>> +		.name	= "rockchip-mipi-dphy",
> >>>> +		.of_match_table = rockchip_dphy_dt_ids,
> >>>> +	},
> >>>> +};
> >>>> +module_platform_driver(rockchip_dphy_driver);
> >>>> +
> >>>> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel@collabora.com>");
> >>>> +MODULE_DESCRIPTION("Rockchip MIPI Synopsys DPHY driver");
> >>>> +MODULE_LICENSE("Dual MIT/GPL");
> > 
> > Overall this is quite good, there are only small issues.
> 
> Thank you a lot for your review

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH] ARM: UNWINDER_FRAME_POINTER implementation for Clang
From: Nick Desaulniers @ 2019-08-22  2:33 UTC (permalink / raw)
  To: Nathan Huckleberry, Catalin Marinas
  Cc: Tri Vo, Arnd Bergmann, Ard Biesheuvel, Russell King, LKML,
	clang-built-linux, Miles Chen (陳民樺),
	Linux ARM
In-Reply-To: <CAJkfWY4cHz+i8kYg2i1Krs-32nh7-WQU+psT=DRGYnTje6yj4Q@mail.gmail.com>

On Wed, Aug 21, 2019 at 10:43 AM Nathan Huckleberry <nhuck@google.com> wrote:
>
> On Tue, Aug 20, 2019 at 2:39 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Tue, Aug 20, 2019 at 12:44 PM Nathan Huckleberry <nhuck@google.com> wrote:
...snip...
> > > +tst    r1, #0x10               @ 26 or 32-bit mode?
> > > +moveq  mask, #0xfc000003
> >
> > Should we be using different masks for ARM vs THUMB as per the
> > reference implementation?
> The change that introduces the arm/thumb code looked like a script
> that was run over all arm in the kernel. Neither this code nor the
> reference solution is compatible with arm, so there's no need for the
> change.

Looks like you're referring to commit 8b592783a2e8 ("Thumb-2:
Implement the unified arch/arm/lib functions").

Currently, arch/arm/Kconfig.debug has:
  57 config UNWINDER_FRAME_POINTER
  58   bool "Frame pointer unwinder"
  59   depends on !THUMB2_KERNEL && !CC_IS_CLANG

So it looks like UNWINDER_FRAME_POINTER and THUMB2_KERNEL are mutually
exclusive.  Probably could send a patch cleaning that up. (ie.
removing the different masks; essentially removing the hunk from
arch/arm/lib/backtrace.S from 8b592783a2e8).

> > > +for_each_frame:        tst     frame, mask             @ Check for address exceptions
> > > +               bne     no_frame
> > > +
> > > +/*
> > > + * sv_fp is the stack frame with the locals for the current considered
> > > + *     function.
> > > + * sv_pc is the saved lr frame the frame above. This is a pointer to a
> > > + *     code address within the current considered function, but
> > > + *     it is not the function start. This value gets updated to be
> > > + *     the function start later if it is possible.
> > > + */
> > > +1001:          ldr     sv_pc, [frame, #4]      @ get saved 'pc'
> > > +1002:          ldr     sv_fp, [frame, #0]      @ get saved fp
> >
> > The reference implementation applies the mask to sv_pc and sv_fp.  I
> > assume we want to, too?
> The mask is already applied to both. See for_each_frame:

ah, under the finished_setup label.
-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* RE: [PATCH V3 2/4] watchdog: Add i.MX7ULP watchdog support
From: Anson Huang @ 2019-08-22  2:44 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, Leonard Crestez,
	schnitzeltony@gmail.com, linux-watchdog@vger.kernel.org,
	otavio@ossystems.com.br, festevam@gmail.com,
	s.hauer@pengutronix.de, jan.tuerk@emtrion.com,
	linux@armlinux.org.uk, linux-kernel@vger.kernel.org,
	robh+dt@kernel.org, dl-linux-imx, kernel@pengutronix.de,
	u.kleine-koenig@pengutronix.de, wim@linux-watchdog.org,
	shawnguo@kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190821141355.GA10463@roeck-us.net>

Hi, Guenter

> On Tue, Aug 20, 2019 at 10:07:56PM -0400, Anson Huang wrote:
> > The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
> that
> > is available for system use.
> > It provides a safety feature to ensure that software is executing as
> > planned and that the CPU is not stuck in an infinite loop or executing
> > unintended code. If the WDOG module is not serviced
> > (refreshed) within a certain period, it resets the MCU.
> >
> > Add driver support for i.MX7ULP watchdog.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V2:
> > 	- add devm_add_action_or_reset to disable clk for remove action.
> > ---
> >  drivers/watchdog/Kconfig       |  13 +++
> >  drivers/watchdog/Makefile      |   1 +
> >  drivers/watchdog/imx7ulp_wdt.c | 246
> > +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 260 insertions(+)
> >  create mode 100644 drivers/watchdog/imx7ulp_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > a8f5c81..d68e5b5 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -724,6 +724,19 @@ config IMX_SC_WDT
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called imx_sc_wdt.
> >
> > +config IMX7ULP_WDT
> > +	tristate "IMX7ULP Watchdog"
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	select WATCHDOG_CORE
> > +	help
> > +	  This is the driver for the hardware watchdog on the Freescale
> > +	  IMX7ULP and later processors. If you have one of these
> > +	  processors and wish to have watchdog support enabled,
> > +	  say Y, otherwise say N.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called imx7ulp_wdt.
> > +
> >  config UX500_WATCHDOG
> >  	tristate "ST-Ericsson Ux500 watchdog"
> >  	depends on MFD_DB8500_PRCMU
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index b5a0aed..2ee352b 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -67,6 +67,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> >  obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> >  obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> >  obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> > +obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
> >  obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> >  obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> >  obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o diff --git
> > a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> new
> > file mode 100644 index 0000000..5d37957
> > --- /dev/null
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDOG_CS			0x0
> > +#define WDOG_CS_CMD32EN		BIT(13)
> > +#define WDOG_CS_ULK		BIT(11)
> > +#define WDOG_CS_RCS		BIT(10)
> > +#define WDOG_CS_EN		BIT(7)
> > +#define WDOG_CS_UPDATE		BIT(5)
> > +
> > +#define WDOG_CNT	0x4
> > +#define WDOG_TOVAL	0x8
> > +
> > +#define REFRESH_SEQ0	0xA602
> > +#define REFRESH_SEQ1	0xB480
> > +#define REFRESH		((REFRESH_SEQ1 << 16) | REFRESH_SEQ0)
> > +
> > +#define UNLOCK_SEQ0	0xC520
> > +#define UNLOCK_SEQ1	0xD928
> > +#define UNLOCK		((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0)
> > +
> > +#define DEFAULT_TIMEOUT	60
> > +#define MAX_TIMEOUT	128
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> > +bool, 0000); MODULE_PARM_DESC(nowayout, "Watchdog cannot be
> stopped
> > +once started (default="
> > +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +struct imx7ulp_wdt_device {
> > +	struct notifier_block restart_handler;
> > +	struct watchdog_device wdd;
> > +	void __iomem *base;
> > +	struct clk *clk;
> > +	int rate;
> > +};
> > +
> > +static inline void imx7ulp_wdt_enable(void __iomem *base, bool
> > +enable) {
> > +	u32 val = readl(base + WDOG_CS);
> > +
> > +	writel(UNLOCK, base + WDOG_CNT);
> > +	if (enable)
> > +		writel(val | WDOG_CS_EN, base + WDOG_CS);
> > +	else
> > +		writel(val & ~WDOG_CS_EN, base + WDOG_CS); }
> > +
> > +static inline bool imx7ulp_wdt_is_enabled(void __iomem *base) {
> > +	u32 val = readl(base + WDOG_CS);
> > +
> > +	return val & WDOG_CS_EN;
> > +}
> > +
> > +static int imx7ulp_wdt_ping(struct watchdog_device *wdog) {
> > +	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > +	writel(REFRESH, wdt->base + WDOG_CNT);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_start(struct watchdog_device *wdog) {
> > +	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > +	imx7ulp_wdt_enable(wdt->base, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_stop(struct watchdog_device *wdog) {
> > +	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > +	imx7ulp_wdt_enable(wdt->base, false);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
> > +				   unsigned int timeout)
> > +{
> > +	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +	u32 val = wdt->rate * timeout;
> > +
> > +	writel(UNLOCK, wdt->base + WDOG_CNT);
> > +	writel(val, wdt->base + WDOG_TOVAL);
> > +
> > +	wdog->timeout = timeout;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_ops imx7ulp_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = imx7ulp_wdt_start,
> > +	.stop  = imx7ulp_wdt_stop,
> > +	.ping  = imx7ulp_wdt_ping,
> > +	.set_timeout = imx7ulp_wdt_set_timeout, };
> > +
> > +static const struct watchdog_info imx7ulp_wdt_info = {
> > +	.identity = "i.MX7ULP watchdog timer",
> > +	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > +		    WDIOF_MAGICCLOSE,
> > +};
> > +
> > +static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int
> > +timeout) {
> > +	u32 val;
> > +
> > +	/* unlock the wdog for reconfiguration */
> > +	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > +	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > +
> > +	/* set an initial timeout value in TOVAL */
> > +	writel(timeout, base + WDOG_TOVAL);
> > +	/* enable 32bit command sequence and reconfigure */
> > +	val = BIT(13) | BIT(8) | BIT(5);
> > +	writel(val, base + WDOG_CS);
> > +}
> > +
> > +static void imx7ulp_wdt_action(void *data) {
> > +	struct imx7ulp_wdt_device *imx7ulp_wdt = data;
> > +
> > +	clk_disable_unprepare(imx7ulp_wdt->clk);
> 
> If you had passed imx7ulp_wdt->clk as parameter, the dereference here
> would not be necessary.

Yup, I thought we may need other data later, so I just passed whole structure.
I agree with your idea, for now, we ONLY need the clk, so I will ONLY pass the clk
to save the dereference operation. Will send out V4.

> 
> > +}
> > +
> > +static int imx7ulp_wdt_probe(struct platform_device *pdev) {
> > +	struct imx7ulp_wdt_device *imx7ulp_wdt;
> > +	struct device *dev = &pdev->dev;
> > +	struct watchdog_device *wdog;
> > +	int ret;
> > +
> > +	imx7ulp_wdt = devm_kzalloc(dev, sizeof(*imx7ulp_wdt),
> GFP_KERNEL);
> > +	if (!imx7ulp_wdt)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, imx7ulp_wdt);
> > +
> > +	imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(imx7ulp_wdt->base))
> > +		return PTR_ERR(imx7ulp_wdt->base);
> > +
> > +	imx7ulp_wdt->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(imx7ulp_wdt->clk)) {
> > +		dev_err(dev, "Failed to get watchdog clock\n");
> > +		return PTR_ERR(imx7ulp_wdt->clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(imx7ulp_wdt->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_add_action_or_reset(dev, imx7ulp_wdt_action,
> imx7ulp_wdt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	imx7ulp_wdt->rate = 1000;
> 
> I forgot to ask (sorry, I had noticed, but I guess I forgot).
> 
> Why not clk_get_rate() ? If the clock rate is fixed, why bother with a 'rate'
> variable ? You could use a constant instead whereever it is used.

OK, the clock rate is fixed on 7ULP, I will use a constant instead of the variable.

Thanks,
Anson

> 
> Thanks
> Guenter
> 
> > +	wdog = &imx7ulp_wdt->wdd;
> > +	wdog->info = &imx7ulp_wdt_info;
> > +	wdog->ops = &imx7ulp_wdt_ops;
> > +	wdog->min_timeout = 1;
> > +	wdog->max_timeout = MAX_TIMEOUT;
> > +	wdog->parent = dev;
> > +	wdog->timeout = DEFAULT_TIMEOUT;
> > +
> > +	watchdog_init_timeout(wdog, 0, dev);
> > +	watchdog_stop_on_reboot(wdog);
> > +	watchdog_stop_on_unregister(wdog);
> > +	watchdog_set_drvdata(wdog, imx7ulp_wdt);
> > +	imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout *
> > +imx7ulp_wdt->rate);
> > +
> > +	return devm_watchdog_register_device(dev, wdog); }
> > +
> > +static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev) {
> > +	struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> > +
> > +	if (watchdog_active(&imx7ulp_wdt->wdd))
> > +		imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
> > +
> > +	clk_disable_unprepare(imx7ulp_wdt->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused imx7ulp_wdt_resume(struct device *dev) {
> > +	struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> > +	u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->rate;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx7ulp_wdt->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
> > +		imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
> > +
> > +	if (watchdog_active(&imx7ulp_wdt->wdd))
> > +		imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
> > +
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops,
> imx7ulp_wdt_suspend,
> > +			 imx7ulp_wdt_resume);
> > +
> > +static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
> > +	{ .compatible = "fsl,imx7ulp-wdt", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
> > +
> > +static struct platform_driver imx7ulp_wdt_driver = {
> > +	.probe		= imx7ulp_wdt_probe,
> > +	.driver		= {
> > +		.name	= "imx7ulp-wdt",
> > +		.pm	= &imx7ulp_wdt_pm_ops,
> > +		.of_match_table = imx7ulp_wdt_dt_ids,
> > +	},
> > +};
> > +module_platform_driver(imx7ulp_wdt_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [PATCH V4 3/4] ARM: imx_v6_v7_defconfig: Enable CONFIG_IMX7ULP_WDT by default
From: Anson Huang @ 2019-08-22  2:37 UTC (permalink / raw)
  To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, linux, otavio, leonard.crestez, schnitzeltony,
	u.kleine-koenig, jan.tuerk, linux-watchdog, devicetree,
	linux-arm-kernel, linux-kernel
  Cc: Linux-imx
In-Reply-To: <1566441463-11911-1-git-send-email-Anson.Huang@nxp.com>

Select CONFIG_IMX7ULP_WDT by default to support i.MX7ULP watchdog.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 9bfffbe..be2a694 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -236,6 +236,7 @@ CONFIG_DA9062_WATCHDOG=y
 CONFIG_DA9063_WATCHDOG=m
 CONFIG_RN5T618_WATCHDOG=y
 CONFIG_IMX2_WDT=y
+CONFIG_IMX7ULP_WDT=y
 CONFIG_MFD_DA9052_I2C=y
 CONFIG_MFD_DA9062=y
 CONFIG_MFD_DA9063=y
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* [PATCH V4 2/4] watchdog: Add i.MX7ULP watchdog support
From: Anson Huang @ 2019-08-22  2:37 UTC (permalink / raw)
  To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, linux, otavio, leonard.crestez, schnitzeltony,
	u.kleine-koenig, jan.tuerk, linux-watchdog, devicetree,
	linux-arm-kernel, linux-kernel
  Cc: Linux-imx
In-Reply-To: <1566441463-11911-1-git-send-email-Anson.Huang@nxp.com>

The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
that is available for system use.
It provides a safety feature to ensure that software is executing
as planned and that the CPU is not stuck in an infinite loop or
executing unintended code. If the WDOG module is not serviced
(refreshed) within a certain period, it resets the MCU.

Add driver support for i.MX7ULP watchdog.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V3:
	- pass clk directly for reset action to avoid dereference from structure;
	- use constant instead of variable for wdog clock rate, as it is fixed as 1KHz.
---
 drivers/watchdog/Kconfig       |  13 +++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/imx7ulp_wdt.c | 243 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/watchdog/imx7ulp_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index a8f5c81..d68e5b5 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -724,6 +724,19 @@ config IMX_SC_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx_sc_wdt.
 
+config IMX7ULP_WDT
+	tristate "IMX7ULP Watchdog"
+	depends on ARCH_MXC || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  This is the driver for the hardware watchdog on the Freescale
+	  IMX7ULP and later processors. If you have one of these
+	  processors and wish to have watchdog support enabled,
+	  say Y, otherwise say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called imx7ulp_wdt.
+
 config UX500_WATCHDOG
 	tristate "ST-Ericsson Ux500 watchdog"
 	depends on MFD_DB8500_PRCMU
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b5a0aed..2ee352b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
 obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
+obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
 obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
 obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
 obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
new file mode 100644
index 0000000..5ce5102
--- /dev/null
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define WDOG_CS			0x0
+#define WDOG_CS_CMD32EN		BIT(13)
+#define WDOG_CS_ULK		BIT(11)
+#define WDOG_CS_RCS		BIT(10)
+#define WDOG_CS_EN		BIT(7)
+#define WDOG_CS_UPDATE		BIT(5)
+
+#define WDOG_CNT	0x4
+#define WDOG_TOVAL	0x8
+
+#define REFRESH_SEQ0	0xA602
+#define REFRESH_SEQ1	0xB480
+#define REFRESH		((REFRESH_SEQ1 << 16) | REFRESH_SEQ0)
+
+#define UNLOCK_SEQ0	0xC520
+#define UNLOCK_SEQ1	0xD928
+#define UNLOCK		((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0)
+
+#define DEFAULT_TIMEOUT	60
+#define MAX_TIMEOUT	128
+#define WDOG_CLOCK_RATE	1000
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct imx7ulp_wdt_device {
+	struct notifier_block restart_handler;
+	struct watchdog_device wdd;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
+{
+	u32 val = readl(base + WDOG_CS);
+
+	writel(UNLOCK, base + WDOG_CNT);
+	if (enable)
+		writel(val | WDOG_CS_EN, base + WDOG_CS);
+	else
+		writel(val & ~WDOG_CS_EN, base + WDOG_CS);
+}
+
+static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
+{
+	u32 val = readl(base + WDOG_CS);
+
+	return val & WDOG_CS_EN;
+}
+
+static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
+{
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+	writel(REFRESH, wdt->base + WDOG_CNT);
+
+	return 0;
+}
+
+static int imx7ulp_wdt_start(struct watchdog_device *wdog)
+{
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+	imx7ulp_wdt_enable(wdt->base, true);
+
+	return 0;
+}
+
+static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
+{
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+	imx7ulp_wdt_enable(wdt->base, false);
+
+	return 0;
+}
+
+static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
+				   unsigned int timeout)
+{
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+	u32 val = WDOG_CLOCK_RATE * timeout;
+
+	writel(UNLOCK, wdt->base + WDOG_CNT);
+	writel(val, wdt->base + WDOG_TOVAL);
+
+	wdog->timeout = timeout;
+
+	return 0;
+}
+
+static const struct watchdog_ops imx7ulp_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = imx7ulp_wdt_start,
+	.stop  = imx7ulp_wdt_stop,
+	.ping  = imx7ulp_wdt_ping,
+	.set_timeout = imx7ulp_wdt_set_timeout,
+};
+
+static const struct watchdog_info imx7ulp_wdt_info = {
+	.identity = "i.MX7ULP watchdog timer",
+	.options  = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+		    WDIOF_MAGICCLOSE,
+};
+
+static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
+{
+	u32 val;
+
+	/* unlock the wdog for reconfiguration */
+	writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
+	writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+
+	/* set an initial timeout value in TOVAL */
+	writel(timeout, base + WDOG_TOVAL);
+	/* enable 32bit command sequence and reconfigure */
+	val = BIT(13) | BIT(8) | BIT(5);
+	writel(val, base + WDOG_CS);
+}
+
+static void imx7ulp_wdt_action(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static int imx7ulp_wdt_probe(struct platform_device *pdev)
+{
+	struct imx7ulp_wdt_device *imx7ulp_wdt;
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdog;
+	int ret;
+
+	imx7ulp_wdt = devm_kzalloc(dev, sizeof(*imx7ulp_wdt), GFP_KERNEL);
+	if (!imx7ulp_wdt)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, imx7ulp_wdt);
+
+	imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(imx7ulp_wdt->base))
+		return PTR_ERR(imx7ulp_wdt->base);
+
+	imx7ulp_wdt->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(imx7ulp_wdt->clk)) {
+		dev_err(dev, "Failed to get watchdog clock\n");
+		return PTR_ERR(imx7ulp_wdt->clk);
+	}
+
+	ret = clk_prepare_enable(imx7ulp_wdt->clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, imx7ulp_wdt_action, imx7ulp_wdt->clk);
+	if (ret)
+		return ret;
+
+	wdog = &imx7ulp_wdt->wdd;
+	wdog->info = &imx7ulp_wdt_info;
+	wdog->ops = &imx7ulp_wdt_ops;
+	wdog->min_timeout = 1;
+	wdog->max_timeout = MAX_TIMEOUT;
+	wdog->parent = dev;
+	wdog->timeout = DEFAULT_TIMEOUT;
+
+	watchdog_init_timeout(wdog, 0, dev);
+	watchdog_stop_on_reboot(wdog);
+	watchdog_stop_on_unregister(wdog);
+	watchdog_set_drvdata(wdog, imx7ulp_wdt);
+	imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout * WDOG_CLOCK_RATE);
+
+	return devm_watchdog_register_device(dev, wdog);
+}
+
+static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
+{
+	struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&imx7ulp_wdt->wdd))
+		imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
+
+	clk_disable_unprepare(imx7ulp_wdt->clk);
+
+	return 0;
+}
+
+static int __maybe_unused imx7ulp_wdt_resume(struct device *dev)
+{
+	struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+	u32 timeout = imx7ulp_wdt->wdd.timeout * WDOG_CLOCK_RATE;
+	int ret;
+
+	ret = clk_prepare_enable(imx7ulp_wdt->clk);
+	if (ret)
+		return ret;
+
+	if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
+		imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
+
+	if (watchdog_active(&imx7ulp_wdt->wdd))
+		imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops, imx7ulp_wdt_suspend,
+			 imx7ulp_wdt_resume);
+
+static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
+	{ .compatible = "fsl,imx7ulp-wdt", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
+
+static struct platform_driver imx7ulp_wdt_driver = {
+	.probe		= imx7ulp_wdt_probe,
+	.driver		= {
+		.name	= "imx7ulp-wdt",
+		.pm	= &imx7ulp_wdt_pm_ops,
+		.of_match_table = imx7ulp_wdt_dt_ids,
+	},
+};
+module_platform_driver(imx7ulp_wdt_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox