Hi,
this series adds support for hardware-triggered thermal reset to the PMC
driver. Namely, it adds device tree properties for specifying the I2C
command to be sent when thermtrip is triggered.
The soctherm driver has now been acked and will be taken into 3.19, so we
can move forward with this. Note that there are no compile time dependencies
between the two series; it's just that this series is no-op without the
soctherm driver being present.
Mikko Perttunen (5):
of: Add descriptions of thermtrip properties to Tegra PMC bindings
of: Add nvidia,controller-id property to Tegra I2C bindings
ARM: tegra124: Add I2C controller ids to device tree
ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree
ARM: tegra: Add thermal reset (thermtrip) support to PMC
.../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24 ++
.../devicetree/bindings/i2c/nvidia,tegra20-i2c.txt | 5 +
arch/arm/boot/dts/tegra124-jetson-tk1.dts | 9 +-
arch/arm/boot/dts/tegra124.dtsi | 6 +
drivers/soc/tegra/pmc.c | 379 ++++++++++++++-------
5 files changed, 290 insertions(+), 133 deletions(-)
--
2.1.3
From: Mikko Perttunen <[email protected]>
Hardware-triggered thermal reset requires configuring the I2C
reset procedure. This configuration is read from the device tree,
so document the relevant properties in the binding documentation.
Signed-off-by: Mikko Perttunen <[email protected]>
Reviewed-by: Wei Ni <[email protected]>
Tested-by: Wei Ni <[email protected]>
---
.../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
index 68ac65f..dc13fb0 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
@@ -47,6 +47,21 @@ Required properties when nvidia,suspend-mode=<0>:
sleep mode, the warm boot code will restore some PLLs, clocks and then
bring up CPU0 for resuming the system.
+Hardware-triggered thermal reset:
+On Tegra30, Tegra114 and Tegra124, if the 'i2c-thermtrip' subnode exists,
+hardware-triggered thermal reset will be enabled.
+
+Required properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'):
+- nvidia,i2c-bus : Phandle to I2C bus containing the PMU
+- nvidia,bus-addr : Bus address of the PMU on the I2C bus
+- nvidia,reg-addr : I2C register address to write poweroff command to
+- nvidia,reg-data : Poweroff command to write to PMU
+
+Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'):
+- nvidia,pinmux-id : Pinmux used by the hardware when issuing poweroff command.
+ Defaults to 0. Valid values are described in section 12.5.2
+ "Pinmux Support" of the Tegra4 Technical Reference Manual.
+
Example:
/ SoC dts including file
@@ -69,6 +84,15 @@ pmc@7000f400 {
/ Tegra board dts file
{
...
+ pmc@7000f400 {
+ i2c-thermtrip {
+ nvidia,i2c-bus = <&pmic_bus>;
+ nvidia,bus-addr = <0x40>;
+ nvidia,reg-addr = <0x36>;
+ nvidia,reg-data = <0x2>;
+ };
+ };
+ ...
clocks {
compatible = "simple-bus";
#address-cells = <1>;
--
2.1.3
From: Mikko Perttunen <[email protected]>
I2C controller ids are required when programming hardware blocks
that send messages to devices connected to an I2C bus, such as
when the PMC sends a poweroff message to the PMIC. Add ids
to all I2C controllers in Tegra124.
Signed-off-by: Mikko Perttunen <[email protected]>
Reviewed-by: Wei Ni <[email protected]>
Tested-by: Wei Ni <[email protected]>
---
arch/arm/boot/dts/tegra124.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 41f8b27..0f09e93 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -361,6 +361,7 @@
reset-names = "i2c";
dmas = <&apbdma 21>, <&apbdma 21>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <0>;
status = "disabled";
};
@@ -376,6 +377,7 @@
reset-names = "i2c";
dmas = <&apbdma 22>, <&apbdma 22>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <1>;
status = "disabled";
};
@@ -391,6 +393,7 @@
reset-names = "i2c";
dmas = <&apbdma 23>, <&apbdma 23>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <2>;
status = "disabled";
};
@@ -406,6 +409,7 @@
reset-names = "i2c";
dmas = <&apbdma 26>, <&apbdma 26>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <3>;
status = "disabled";
};
@@ -421,6 +425,7 @@
reset-names = "i2c";
dmas = <&apbdma 24>, <&apbdma 24>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <4>;
status = "disabled";
};
@@ -436,6 +441,7 @@
reset-names = "i2c";
dmas = <&apbdma 30>, <&apbdma 30>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <5>;
status = "disabled";
};
--
2.1.3
From: Mikko Perttunen <[email protected]>
This adds the required information to reset the board during an overheating
situation to the Jetson TK1 device tree. The thermal reset is handled by the
PMC by sending an I2C message to the PMIC. The entries specify the I2C
message to be sent.
Signed-off-by: Mikko Perttunen <[email protected]>
Reviewed-by: Wei Ni <[email protected]>
Tested-by: Wei Ni <[email protected]>
---
arch/arm/boot/dts/tegra124-jetson-tk1.dts | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index f46a789..57ead69 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1457,7 +1457,7 @@
};
/* Expansion PWR_I2C_*, on-board components */
- i2c@0,7000d000 {
+ pmic_bus: i2c@0,7000d000 {
status = "okay";
clock-frequency = <400000>;
@@ -1672,6 +1672,13 @@
nvidia,core-pwr-off-time = <61036>;
nvidia,core-power-req-active-high;
nvidia,sys-clock-req-active-high;
+
+ i2c-thermtrip {
+ nvidia,i2c-bus = <&pmic_bus>;
+ nvidia,bus-addr = <0x40>;
+ nvidia,reg-addr = <0x36>;
+ nvidia,reg-data = <0x2>;
+ };
};
/* Serial ATA */
--
2.1.3
From: Mikko Perttunen <[email protected]>
Sometimes, hardware blocks want to issue requests to devices
connected to I2C buses by itself. In such case, the bus the
target device resides on must be configured into a register.
For this purpose, each I2C controller has a defined ID known
by the hardware. Add a property for these IDs to the device tree
bindings, so that drivers can know what ID to write to a hardware
register when configuring a block that sends I2C messages autonomously.
Signed-off-by: Mikko Perttunen <[email protected]>
Reviewed-by: Wei Ni <[email protected]>
Tested-by: Wei Ni <[email protected]>
---
Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt
index 87507e9..e9e5994 100644
--- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt
@@ -57,6 +57,10 @@ Required properties:
- rx
- tx
+Optional properties:
+- nvidia,controller-id: ID of controller when referred to in
+ hardware registers.
+
Example:
i2c@7000c000 {
@@ -71,5 +75,6 @@ Example:
reset-names = "i2c";
dmas = <&apbdma 16>, <&apbdma 16>;
dma-names = "rx", "tx";
+ nvidia,controller-id = <0>;
status = "disabled";
};
--
2.1.3
From: Mikko Perttunen <[email protected]>
This adds a device tree controlled option to enable PMC-based
thermal reset in overheating situations. Thermtrip is supported on
Tegra30, Tegra114 and Tegra124. The thermal reset only works when
the thermal sensors are calibrated, so a soctherm driver is also
required.
The thermtrip event is triggered by the soctherm block, and all
soctherm sensors default to showing a temperature of zero Celsius
before they are initialized. Because of this, it is safe to initialize
thermtrip and soctherm in any order.
Signed-off-by: Mikko Perttunen <[email protected]>
Reviewed-by: Wei Ni <[email protected]>
Tested-by: Wei Ni <[email protected]>
---
drivers/soc/tegra/pmc.c | 379 +++++++++++++++++++++++++++++++-----------------
1 file changed, 247 insertions(+), 132 deletions(-)
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index a2c0ceb..de514fe 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -83,11 +83,28 @@
#define GPU_RG_CNTRL 0x2d4
+#define PMC_SENSOR_CTRL 0x1b0
+#define PMC_SENSOR_CTRL_SCRATCH_WRITE (1 << 2)
+#define PMC_SENSOR_CTRL_ENABLE_RST (1 << 1)
+
+#define PMC_SCRATCH54 0x258
+#define PMC_SCRATCH54_DATA_SHIFT 8
+#define PMC_SCRATCH54_ADDR_SHIFT 0
+
+#define PMC_SCRATCH55 0x25c
+#define PMC_SCRATCH55_RESET_TEGRA (1 << 31)
+#define PMC_SCRATCH55_CNTRL_ID_SHIFT 27
+#define PMC_SCRATCH55_PINMUX_SHIFT 24
+#define PMC_SCRATCH55_16BITOP (1 << 15)
+#define PMC_SCRATCH55_CHECKSUM_SHIFT 16
+#define PMC_SCRATCH55_I2CSLV1_SHIFT 0
+
struct tegra_pmc_soc {
unsigned int num_powergates;
const char *const *powergates;
unsigned int num_cpu_powergates;
const u8 *cpu_powergates;
+ bool has_tsense_reset;
};
/**
@@ -606,6 +623,234 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
}
#endif
+static const char * const tegra20_powergates[] = {
+ [TEGRA_POWERGATE_CPU] = "cpu",
+ [TEGRA_POWERGATE_3D] = "3d",
+ [TEGRA_POWERGATE_VENC] = "venc",
+ [TEGRA_POWERGATE_VDEC] = "vdec",
+ [TEGRA_POWERGATE_PCIE] = "pcie",
+ [TEGRA_POWERGATE_L2] = "l2",
+ [TEGRA_POWERGATE_MPE] = "mpe",
+};
+
+static const struct tegra_pmc_soc tegra20_pmc_soc = {
+ .num_powergates = ARRAY_SIZE(tegra20_powergates),
+ .powergates = tegra20_powergates,
+ .num_cpu_powergates = 0,
+ .cpu_powergates = NULL,
+};
+
+static const char * const tegra30_powergates[] = {
+ [TEGRA_POWERGATE_CPU] = "cpu0",
+ [TEGRA_POWERGATE_3D] = "3d0",
+ [TEGRA_POWERGATE_VENC] = "venc",
+ [TEGRA_POWERGATE_VDEC] = "vdec",
+ [TEGRA_POWERGATE_PCIE] = "pcie",
+ [TEGRA_POWERGATE_L2] = "l2",
+ [TEGRA_POWERGATE_MPE] = "mpe",
+ [TEGRA_POWERGATE_HEG] = "heg",
+ [TEGRA_POWERGATE_SATA] = "sata",
+ [TEGRA_POWERGATE_CPU1] = "cpu1",
+ [TEGRA_POWERGATE_CPU2] = "cpu2",
+ [TEGRA_POWERGATE_CPU3] = "cpu3",
+ [TEGRA_POWERGATE_CELP] = "celp",
+ [TEGRA_POWERGATE_3D1] = "3d1",
+};
+
+static const u8 tegra30_cpu_powergates[] = {
+ TEGRA_POWERGATE_CPU,
+ TEGRA_POWERGATE_CPU1,
+ TEGRA_POWERGATE_CPU2,
+ TEGRA_POWERGATE_CPU3,
+};
+
+static const struct tegra_pmc_soc tegra30_pmc_soc = {
+ .num_powergates = ARRAY_SIZE(tegra30_powergates),
+ .powergates = tegra30_powergates,
+ .num_cpu_powergates = ARRAY_SIZE(tegra30_cpu_powergates),
+ .cpu_powergates = tegra30_cpu_powergates,
+ .has_tsense_reset = true,
+};
+
+static const char * const tegra114_powergates[] = {
+ [TEGRA_POWERGATE_CPU] = "crail",
+ [TEGRA_POWERGATE_3D] = "3d",
+ [TEGRA_POWERGATE_VENC] = "venc",
+ [TEGRA_POWERGATE_VDEC] = "vdec",
+ [TEGRA_POWERGATE_MPE] = "mpe",
+ [TEGRA_POWERGATE_HEG] = "heg",
+ [TEGRA_POWERGATE_CPU1] = "cpu1",
+ [TEGRA_POWERGATE_CPU2] = "cpu2",
+ [TEGRA_POWERGATE_CPU3] = "cpu3",
+ [TEGRA_POWERGATE_CELP] = "celp",
+ [TEGRA_POWERGATE_CPU0] = "cpu0",
+ [TEGRA_POWERGATE_C0NC] = "c0nc",
+ [TEGRA_POWERGATE_C1NC] = "c1nc",
+ [TEGRA_POWERGATE_DIS] = "dis",
+ [TEGRA_POWERGATE_DISB] = "disb",
+ [TEGRA_POWERGATE_XUSBA] = "xusba",
+ [TEGRA_POWERGATE_XUSBB] = "xusbb",
+ [TEGRA_POWERGATE_XUSBC] = "xusbc",
+};
+
+static const u8 tegra114_cpu_powergates[] = {
+ TEGRA_POWERGATE_CPU0,
+ TEGRA_POWERGATE_CPU1,
+ TEGRA_POWERGATE_CPU2,
+ TEGRA_POWERGATE_CPU3,
+};
+
+static const struct tegra_pmc_soc tegra114_pmc_soc = {
+ .num_powergates = ARRAY_SIZE(tegra114_powergates),
+ .powergates = tegra114_powergates,
+ .num_cpu_powergates = ARRAY_SIZE(tegra114_cpu_powergates),
+ .cpu_powergates = tegra114_cpu_powergates,
+ .has_tsense_reset = true,
+};
+
+static const char * const tegra124_powergates[] = {
+ [TEGRA_POWERGATE_CPU] = "crail",
+ [TEGRA_POWERGATE_3D] = "3d",
+ [TEGRA_POWERGATE_VENC] = "venc",
+ [TEGRA_POWERGATE_PCIE] = "pcie",
+ [TEGRA_POWERGATE_VDEC] = "vdec",
+ [TEGRA_POWERGATE_L2] = "l2",
+ [TEGRA_POWERGATE_MPE] = "mpe",
+ [TEGRA_POWERGATE_HEG] = "heg",
+ [TEGRA_POWERGATE_SATA] = "sata",
+ [TEGRA_POWERGATE_CPU1] = "cpu1",
+ [TEGRA_POWERGATE_CPU2] = "cpu2",
+ [TEGRA_POWERGATE_CPU3] = "cpu3",
+ [TEGRA_POWERGATE_CELP] = "celp",
+ [TEGRA_POWERGATE_CPU0] = "cpu0",
+ [TEGRA_POWERGATE_C0NC] = "c0nc",
+ [TEGRA_POWERGATE_C1NC] = "c1nc",
+ [TEGRA_POWERGATE_SOR] = "sor",
+ [TEGRA_POWERGATE_DIS] = "dis",
+ [TEGRA_POWERGATE_DISB] = "disb",
+ [TEGRA_POWERGATE_XUSBA] = "xusba",
+ [TEGRA_POWERGATE_XUSBB] = "xusbb",
+ [TEGRA_POWERGATE_XUSBC] = "xusbc",
+ [TEGRA_POWERGATE_VIC] = "vic",
+ [TEGRA_POWERGATE_IRAM] = "iram",
+};
+
+static const u8 tegra124_cpu_powergates[] = {
+ TEGRA_POWERGATE_CPU0,
+ TEGRA_POWERGATE_CPU1,
+ TEGRA_POWERGATE_CPU2,
+ TEGRA_POWERGATE_CPU3,
+};
+
+static const struct tegra_pmc_soc tegra124_pmc_soc = {
+ .num_powergates = ARRAY_SIZE(tegra124_powergates),
+ .powergates = tegra124_powergates,
+ .num_cpu_powergates = ARRAY_SIZE(tegra124_cpu_powergates),
+ .cpu_powergates = tegra124_cpu_powergates,
+ .has_tsense_reset = true,
+};
+
+static const struct of_device_id tegra_pmc_match[] = {
+ { .compatible = "nvidia,tegra124-pmc", .data = &tegra124_pmc_soc },
+ { .compatible = "nvidia,tegra114-pmc", .data = &tegra114_pmc_soc },
+ { .compatible = "nvidia,tegra30-pmc", .data = &tegra30_pmc_soc },
+ { .compatible = "nvidia,tegra20-pmc", .data = &tegra20_pmc_soc },
+ { }
+};
+
+void tegra_pmc_init_tsense_reset(struct device *dev)
+{
+ u32 pmu_i2c_addr, i2c_ctrl_id, reg_addr, reg_data, pinmux;
+ u32 value, checksum;
+ struct device_node *np = dev->of_node;
+ struct device_node *i2c_np, *tt_np;
+ const struct of_device_id *match = of_match_node(tegra_pmc_match, np);
+ const struct tegra_pmc_soc *data = match->data;
+
+ if (!data->has_tsense_reset)
+ return;
+
+ tt_np = of_find_node_by_name(np, "i2c-thermtrip");
+ if (!tt_np) {
+ dev_warn(dev, "no i2c-thermtrip node found, disabling emergency thermal reset\n");
+ return;
+ }
+
+ i2c_np = of_parse_phandle(tt_np, "nvidia,i2c-bus", 0);
+ if (!i2c_np) {
+ dev_err(dev, "I2C bus reference missing, disabling emergency thermal reset\n");
+ goto put_tt;
+ }
+
+ if (of_property_read_u32(i2c_np, "nvidia,controller-id", &i2c_ctrl_id)) {
+ dev_err(dev, "I2C bus controller id missing, disabling emergency thermal reset\n");
+ goto put_i2c;
+ }
+
+ of_node_put(i2c_np);
+
+ if (of_property_read_u32(tt_np, "nvidia,bus-addr", &pmu_i2c_addr)) {
+ dev_err(dev, "nvidia,bus-addr missing, disabling emergency thermal reset\n");
+ goto put_tt;
+ }
+
+ if (of_property_read_u32(tt_np, "nvidia,reg-addr", ®_addr)) {
+ dev_err(dev, "nvidia,reg-addr missing, disabling emergency thermal reset\n");
+ goto put_tt;
+ }
+
+ if (of_property_read_u32(tt_np, "nvidia,reg-data", ®_data)) {
+ dev_err(dev, "nvidia,reg-data missing, disabling emergency thermal reset\n");
+ goto put_tt;
+ }
+
+ if (of_property_read_u32(tt_np, "nvidia,pinmux-id", &pinmux))
+ pinmux = 0;
+
+ of_node_put(tt_np);
+
+ value = tegra_pmc_readl(PMC_SENSOR_CTRL);
+ value |= PMC_SENSOR_CTRL_SCRATCH_WRITE;
+ tegra_pmc_writel(value, PMC_SENSOR_CTRL);
+
+ value = (reg_data << PMC_SCRATCH54_DATA_SHIFT) |
+ (reg_addr << PMC_SCRATCH54_ADDR_SHIFT);
+ tegra_pmc_writel(value, PMC_SCRATCH54);
+
+ value = 0;
+ value |= PMC_SCRATCH55_RESET_TEGRA;
+ value |= i2c_ctrl_id << PMC_SCRATCH55_CNTRL_ID_SHIFT;
+ value |= pinmux << PMC_SCRATCH55_PINMUX_SHIFT;
+ value |= pmu_i2c_addr << PMC_SCRATCH55_I2CSLV1_SHIFT;
+
+ /* Calculate checksum of SCRATCH54, SCRATCH55 fields.
+ * Bits 23:16 will contain the checksum and are currently zero,
+ * so they are not added.
+ */
+ checksum = reg_addr + reg_data + (value & 0xff) + ((value >> 8) & 0xff)
+ + ((value >> 24) & 0xff);
+ checksum &= 0xff;
+ checksum = 0x100 - checksum;
+
+ value |= checksum << PMC_SCRATCH55_CHECKSUM_SHIFT;
+
+ tegra_pmc_writel(value, PMC_SCRATCH55);
+
+ value = tegra_pmc_readl(PMC_SENSOR_CTRL);
+ value |= PMC_SENSOR_CTRL_ENABLE_RST;
+ tegra_pmc_writel(value, PMC_SENSOR_CTRL);
+
+ dev_info(dev, "PMC emergency thermal reset enabled\n");
+
+ return;
+
+put_i2c:
+ of_node_put(i2c_np);
+
+put_tt:
+ of_node_put(tt_np);
+}
+
static int tegra_pmc_parse_dt(struct tegra_pmc *pmc, struct device_node *np)
{
u32 value, values[2];
@@ -730,6 +975,8 @@ static int tegra_pmc_probe(struct platform_device *pdev)
tegra_pmc_init(pmc);
+ tegra_pmc_init_tsense_reset(&pdev->dev);
+
if (IS_ENABLED(CONFIG_DEBUG_FS)) {
err = tegra_powergate_debugfs_init();
if (err < 0)
@@ -757,138 +1004,6 @@ static int tegra_pmc_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(tegra_pmc_pm_ops, tegra_pmc_suspend, tegra_pmc_resume);
-static const char * const tegra20_powergates[] = {
- [TEGRA_POWERGATE_CPU] = "cpu",
- [TEGRA_POWERGATE_3D] = "3d",
- [TEGRA_POWERGATE_VENC] = "venc",
- [TEGRA_POWERGATE_VDEC] = "vdec",
- [TEGRA_POWERGATE_PCIE] = "pcie",
- [TEGRA_POWERGATE_L2] = "l2",
- [TEGRA_POWERGATE_MPE] = "mpe",
-};
-
-static const struct tegra_pmc_soc tegra20_pmc_soc = {
- .num_powergates = ARRAY_SIZE(tegra20_powergates),
- .powergates = tegra20_powergates,
- .num_cpu_powergates = 0,
- .cpu_powergates = NULL,
-};
-
-static const char * const tegra30_powergates[] = {
- [TEGRA_POWERGATE_CPU] = "cpu0",
- [TEGRA_POWERGATE_3D] = "3d0",
- [TEGRA_POWERGATE_VENC] = "venc",
- [TEGRA_POWERGATE_VDEC] = "vdec",
- [TEGRA_POWERGATE_PCIE] = "pcie",
- [TEGRA_POWERGATE_L2] = "l2",
- [TEGRA_POWERGATE_MPE] = "mpe",
- [TEGRA_POWERGATE_HEG] = "heg",
- [TEGRA_POWERGATE_SATA] = "sata",
- [TEGRA_POWERGATE_CPU1] = "cpu1",
- [TEGRA_POWERGATE_CPU2] = "cpu2",
- [TEGRA_POWERGATE_CPU3] = "cpu3",
- [TEGRA_POWERGATE_CELP] = "celp",
- [TEGRA_POWERGATE_3D1] = "3d1",
-};
-
-static const u8 tegra30_cpu_powergates[] = {
- TEGRA_POWERGATE_CPU,
- TEGRA_POWERGATE_CPU1,
- TEGRA_POWERGATE_CPU2,
- TEGRA_POWERGATE_CPU3,
-};
-
-static const struct tegra_pmc_soc tegra30_pmc_soc = {
- .num_powergates = ARRAY_SIZE(tegra30_powergates),
- .powergates = tegra30_powergates,
- .num_cpu_powergates = ARRAY_SIZE(tegra30_cpu_powergates),
- .cpu_powergates = tegra30_cpu_powergates,
-};
-
-static const char * const tegra114_powergates[] = {
- [TEGRA_POWERGATE_CPU] = "crail",
- [TEGRA_POWERGATE_3D] = "3d",
- [TEGRA_POWERGATE_VENC] = "venc",
- [TEGRA_POWERGATE_VDEC] = "vdec",
- [TEGRA_POWERGATE_MPE] = "mpe",
- [TEGRA_POWERGATE_HEG] = "heg",
- [TEGRA_POWERGATE_CPU1] = "cpu1",
- [TEGRA_POWERGATE_CPU2] = "cpu2",
- [TEGRA_POWERGATE_CPU3] = "cpu3",
- [TEGRA_POWERGATE_CELP] = "celp",
- [TEGRA_POWERGATE_CPU0] = "cpu0",
- [TEGRA_POWERGATE_C0NC] = "c0nc",
- [TEGRA_POWERGATE_C1NC] = "c1nc",
- [TEGRA_POWERGATE_DIS] = "dis",
- [TEGRA_POWERGATE_DISB] = "disb",
- [TEGRA_POWERGATE_XUSBA] = "xusba",
- [TEGRA_POWERGATE_XUSBB] = "xusbb",
- [TEGRA_POWERGATE_XUSBC] = "xusbc",
-};
-
-static const u8 tegra114_cpu_powergates[] = {
- TEGRA_POWERGATE_CPU0,
- TEGRA_POWERGATE_CPU1,
- TEGRA_POWERGATE_CPU2,
- TEGRA_POWERGATE_CPU3,
-};
-
-static const struct tegra_pmc_soc tegra114_pmc_soc = {
- .num_powergates = ARRAY_SIZE(tegra114_powergates),
- .powergates = tegra114_powergates,
- .num_cpu_powergates = ARRAY_SIZE(tegra114_cpu_powergates),
- .cpu_powergates = tegra114_cpu_powergates,
-};
-
-static const char * const tegra124_powergates[] = {
- [TEGRA_POWERGATE_CPU] = "crail",
- [TEGRA_POWERGATE_3D] = "3d",
- [TEGRA_POWERGATE_VENC] = "venc",
- [TEGRA_POWERGATE_PCIE] = "pcie",
- [TEGRA_POWERGATE_VDEC] = "vdec",
- [TEGRA_POWERGATE_L2] = "l2",
- [TEGRA_POWERGATE_MPE] = "mpe",
- [TEGRA_POWERGATE_HEG] = "heg",
- [TEGRA_POWERGATE_SATA] = "sata",
- [TEGRA_POWERGATE_CPU1] = "cpu1",
- [TEGRA_POWERGATE_CPU2] = "cpu2",
- [TEGRA_POWERGATE_CPU3] = "cpu3",
- [TEGRA_POWERGATE_CELP] = "celp",
- [TEGRA_POWERGATE_CPU0] = "cpu0",
- [TEGRA_POWERGATE_C0NC] = "c0nc",
- [TEGRA_POWERGATE_C1NC] = "c1nc",
- [TEGRA_POWERGATE_SOR] = "sor",
- [TEGRA_POWERGATE_DIS] = "dis",
- [TEGRA_POWERGATE_DISB] = "disb",
- [TEGRA_POWERGATE_XUSBA] = "xusba",
- [TEGRA_POWERGATE_XUSBB] = "xusbb",
- [TEGRA_POWERGATE_XUSBC] = "xusbc",
- [TEGRA_POWERGATE_VIC] = "vic",
- [TEGRA_POWERGATE_IRAM] = "iram",
-};
-
-static const u8 tegra124_cpu_powergates[] = {
- TEGRA_POWERGATE_CPU0,
- TEGRA_POWERGATE_CPU1,
- TEGRA_POWERGATE_CPU2,
- TEGRA_POWERGATE_CPU3,
-};
-
-static const struct tegra_pmc_soc tegra124_pmc_soc = {
- .num_powergates = ARRAY_SIZE(tegra124_powergates),
- .powergates = tegra124_powergates,
- .num_cpu_powergates = ARRAY_SIZE(tegra124_cpu_powergates),
- .cpu_powergates = tegra124_cpu_powergates,
-};
-
-static const struct of_device_id tegra_pmc_match[] = {
- { .compatible = "nvidia,tegra124-pmc", .data = &tegra124_pmc_soc },
- { .compatible = "nvidia,tegra114-pmc", .data = &tegra114_pmc_soc },
- { .compatible = "nvidia,tegra30-pmc", .data = &tegra30_pmc_soc },
- { .compatible = "nvidia,tegra20-pmc", .data = &tegra20_pmc_soc },
- { }
-};
-
static struct platform_driver tegra_pmc_driver = {
.driver = {
.name = "tegra-pmc",
--
2.1.3
On 11/10/2014 10:12 PM, Mikko Perttunen wrote:
> From: Mikko Perttunen <[email protected]>
>
> Hardware-triggered thermal reset requires configuring the I2C
> reset procedure. This configuration is read from the device tree,
> so document the relevant properties in the binding documentation.
>
> Signed-off-by: Mikko Perttunen <[email protected]>
> Reviewed-by: Wei Ni <[email protected]>
> Tested-by: Wei Ni <[email protected]>
> ---
> .../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24 ++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 68ac65f..dc13fb0 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -47,6 +47,21 @@ Required properties when nvidia,suspend-mode=<0>:
> sleep mode, the warm boot code will restore some PLLs, clocks and then
> bring up CPU0 for resuming the system.
>
> +Hardware-triggered thermal reset:
> +On Tegra30, Tegra114 and Tegra124, if the 'i2c-thermtrip' subnode exists,
> +hardware-triggered thermal reset will be enabled.
> +
> +Required properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'):
> +- nvidia,i2c-bus : Phandle to I2C bus containing the PMU
> +- nvidia,bus-addr : Bus address of the PMU on the I2C bus
> +- nvidia,reg-addr : I2C register address to write poweroff command to
> +- nvidia,reg-data : Poweroff command to write to PMU
This binding is taking two different routes to provide values to the driver:
1) It uses a phandle for i2c-bus (which must then be provided by another
binding implemented in the two following patches)
2) It uses direct values for bus-addr, reg-addr and reg-data.
Do we need to use both approaches? bus-addr could just as well be
obtained through a phandle to the i2c device and reading its reg
property. From this phandle you could also go back up to the bus, making
the i2c-bus property unnecessary. reg-addr and reg-data cannot be
specified that way, obviously.
Actually I think I'd prefer to see i2c-bus become an integer property
instead of a phandle, because at the end of the day it is a value field
of a particular register and the reference is only used to retrieve that
value. It is not like we are actually going to call functions on the bus
instance or change its state. And for the single purpose of retrieving
that value, this binding requires the addition of a new property on the
bus node that will probably never be used for something else.
We can also imagine that some design may not have a PMIC device in the
DT but still want to use the thermal reset feature. In this case, it is
again preferable to use direct values.
On 11/10/2014 10:12 PM, Mikko Perttunen wrote:
> @@ -606,6 +623,234 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
> }
> #endif
>
> +static const char * const tegra20_powergates[] = {
> + [TEGRA_POWERGATE_CPU] = "cpu",
> + [TEGRA_POWERGATE_3D] = "3d",
> + [TEGRA_POWERGATE_VENC] = "venc",
> + [TEGRA_POWERGATE_VDEC] = "vdec",
> + [TEGRA_POWERGATE_PCIE] = "pcie",
> + [TEGRA_POWERGATE_L2] = "l2",
> + [TEGRA_POWERGATE_MPE] = "mpe",
> +};
> +
> +static const struct tegra_pmc_soc tegra20_pmc_soc = {
> + .num_powergates = ARRAY_SIZE(tegra20_powergates),
> + .powergates = tegra20_powergates,
> + .num_cpu_powergates = 0,
> + .cpu_powergates = NULL,
> +};
> +
> +static const char * const tegra30_powergates[] = {
> + [TEGRA_POWERGATE_CPU] = "cpu0",
> + [TEGRA_POWERGATE_3D] = "3d0",
> + [TEGRA_POWERGATE_VENC] = "venc",
> + [TEGRA_POWERGATE_VDEC] = "vdec",
> + [TEGRA_POWERGATE_PCIE] = "pcie",
> + [TEGRA_POWERGATE_L2] = "l2",
> + [TEGRA_POWERGATE_MPE] = "mpe",
> + [TEGRA_POWERGATE_HEG] = "heg",
> + [TEGRA_POWERGATE_SATA] = "sata",
> + [TEGRA_POWERGATE_CPU1] = "cpu1",
> + [TEGRA_POWERGATE_CPU2] = "cpu2",
> + [TEGRA_POWERGATE_CPU3] = "cpu3",
> + [TEGRA_POWERGATE_CELP] = "celp",
> + [TEGRA_POWERGATE_3D1] = "3d1",
> +};
> +
> +static const u8 tegra30_cpu_powergates[] = {
> + TEGRA_POWERGATE_CPU,
> + TEGRA_POWERGATE_CPU1,
> + TEGRA_POWERGATE_CPU2,
> + TEGRA_POWERGATE_CPU3,
> +};
> +
> +static const struct tegra_pmc_soc tegra30_pmc_soc = {
> + .num_powergates = ARRAY_SIZE(tegra30_powergates),
> + .powergates = tegra30_powergates,
> + .num_cpu_powergates = ARRAY_SIZE(tegra30_cpu_powergates),
> + .cpu_powergates = tegra30_cpu_powergates,
> + .has_tsense_reset = true,
> +};
> +
> +static const char * const tegra114_powergates[] = {
> + [TEGRA_POWERGATE_CPU] = "crail",
> + [TEGRA_POWERGATE_3D] = "3d",
> + [TEGRA_POWERGATE_VENC] = "venc",
> + [TEGRA_POWERGATE_VDEC] = "vdec",
> + [TEGRA_POWERGATE_MPE] = "mpe",
> + [TEGRA_POWERGATE_HEG] = "heg",
> + [TEGRA_POWERGATE_CPU1] = "cpu1",
> + [TEGRA_POWERGATE_CPU2] = "cpu2",
> + [TEGRA_POWERGATE_CPU3] = "cpu3",
> + [TEGRA_POWERGATE_CELP] = "celp",
> + [TEGRA_POWERGATE_CPU0] = "cpu0",
> + [TEGRA_POWERGATE_C0NC] = "c0nc",
> + [TEGRA_POWERGATE_C1NC] = "c1nc",
> + [TEGRA_POWERGATE_DIS] = "dis",
> + [TEGRA_POWERGATE_DISB] = "disb",
> + [TEGRA_POWERGATE_XUSBA] = "xusba",
> + [TEGRA_POWERGATE_XUSBB] = "xusbb",
> + [TEGRA_POWERGATE_XUSBC] = "xusbc",
> +};
> +
> +static const u8 tegra114_cpu_powergates[] = {
> + TEGRA_POWERGATE_CPU0,
> + TEGRA_POWERGATE_CPU1,
> + TEGRA_POWERGATE_CPU2,
> + TEGRA_POWERGATE_CPU3,
> +};
> +
> +static const struct tegra_pmc_soc tegra114_pmc_soc = {
> + .num_powergates = ARRAY_SIZE(tegra114_powergates),
> + .powergates = tegra114_powergates,
> + .num_cpu_powergates = ARRAY_SIZE(tegra114_cpu_powergates),
> + .cpu_powergates = tegra114_cpu_powergates,
> + .has_tsense_reset = true,
> +};
> +
> +static const char * const tegra124_powergates[] = {
> + [TEGRA_POWERGATE_CPU] = "crail",
> + [TEGRA_POWERGATE_3D] = "3d",
> + [TEGRA_POWERGATE_VENC] = "venc",
> + [TEGRA_POWERGATE_PCIE] = "pcie",
> + [TEGRA_POWERGATE_VDEC] = "vdec",
> + [TEGRA_POWERGATE_L2] = "l2",
> + [TEGRA_POWERGATE_MPE] = "mpe",
> + [TEGRA_POWERGATE_HEG] = "heg",
> + [TEGRA_POWERGATE_SATA] = "sata",
> + [TEGRA_POWERGATE_CPU1] = "cpu1",
> + [TEGRA_POWERGATE_CPU2] = "cpu2",
> + [TEGRA_POWERGATE_CPU3] = "cpu3",
> + [TEGRA_POWERGATE_CELP] = "celp",
> + [TEGRA_POWERGATE_CPU0] = "cpu0",
> + [TEGRA_POWERGATE_C0NC] = "c0nc",
> + [TEGRA_POWERGATE_C1NC] = "c1nc",
> + [TEGRA_POWERGATE_SOR] = "sor",
> + [TEGRA_POWERGATE_DIS] = "dis",
> + [TEGRA_POWERGATE_DISB] = "disb",
> + [TEGRA_POWERGATE_XUSBA] = "xusba",
> + [TEGRA_POWERGATE_XUSBB] = "xusbb",
> + [TEGRA_POWERGATE_XUSBC] = "xusbc",
> + [TEGRA_POWERGATE_VIC] = "vic",
> + [TEGRA_POWERGATE_IRAM] = "iram",
> +};
> +
> +static const u8 tegra124_cpu_powergates[] = {
> + TEGRA_POWERGATE_CPU0,
> + TEGRA_POWERGATE_CPU1,
> + TEGRA_POWERGATE_CPU2,
> + TEGRA_POWERGATE_CPU3,
> +};
> +
> +static const struct tegra_pmc_soc tegra124_pmc_soc = {
> + .num_powergates = ARRAY_SIZE(tegra124_powergates),
> + .powergates = tegra124_powergates,
> + .num_cpu_powergates = ARRAY_SIZE(tegra124_cpu_powergates),
> + .cpu_powergates = tegra124_cpu_powergates,
> + .has_tsense_reset = true,
> +};
> +
> +static const struct of_device_id tegra_pmc_match[] = {
> + { .compatible = "nvidia,tegra124-pmc", .data = &tegra124_pmc_soc },
> + { .compatible = "nvidia,tegra114-pmc", .data = &tegra114_pmc_soc },
> + { .compatible = "nvidia,tegra30-pmc", .data = &tegra30_pmc_soc },
> + { .compatible = "nvidia,tegra20-pmc", .data = &tegra20_pmc_soc },
> + { }
> +};
Is there a need for moving this huge block of declarations? AFAICT you
are only using tegra_pmc_match in your function, maybe you can just do a
forward declaration so does not get longer and more complex than it
needs to be?
This detail aside, patch looks ok to me.
On 11/11/2014 08:37 AM, Alexandre Courbot wrote:
> On 11/10/2014 10:12 PM, Mikko Perttunen wrote:
>> From: Mikko Perttunen <[email protected]>
>>
>> Hardware-triggered thermal reset requires configuring the I2C
>> reset procedure. This configuration is read from the device tree,
>> so document the relevant properties in the binding documentation.
>>
>> Signed-off-by: Mikko Perttunen <[email protected]>
>> Reviewed-by: Wei Ni <[email protected]>
>> Tested-by: Wei Ni <[email protected]>
>> ---
>> .../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24
>> ++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 68ac65f..dc13fb0 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -47,6 +47,21 @@ Required properties when nvidia,suspend-mode=<0>:
>> sleep mode, the warm boot code will restore some PLLs, clocks and
>> then
>> bring up CPU0 for resuming the system.
>>
>> +Hardware-triggered thermal reset:
>> +On Tegra30, Tegra114 and Tegra124, if the 'i2c-thermtrip' subnode
>> exists,
>> +hardware-triggered thermal reset will be enabled.
>> +
>> +Required properties for hardware-triggered thermal reset (inside
>> 'i2c-thermtrip'):
>> +- nvidia,i2c-bus : Phandle to I2C bus containing the PMU
>> +- nvidia,bus-addr : Bus address of the PMU on the I2C bus
>> +- nvidia,reg-addr : I2C register address to write poweroff command to
>> +- nvidia,reg-data : Poweroff command to write to PMU
>
> This binding is taking two different routes to provide values to the
> driver:
>
> 1) It uses a phandle for i2c-bus (which must then be provided by another
> binding implemented in the two following patches)
>
> 2) It uses direct values for bus-addr, reg-addr and reg-data.
>
> Do we need to use both approaches? bus-addr could just as well be
> obtained through a phandle to the i2c device and reading its reg
> property. From this phandle you could also go back up to the bus, making
> the i2c-bus property unnecessary. reg-addr and reg-data cannot be
> specified that way, obviously.
This was in fact how I used to implement this, but Stephen or Thierry
pointed out that the reg property actually might not contain the correct
address (I think because the PMIC could have multiple addresses, and the
one in DT might not be the one that accepts the reset command).
The workaround for that was to either add this integer property for
bus-addr or add a new PMIC API for querying. I went for this as it is
much simpler.
>
> Actually I think I'd prefer to see i2c-bus become an integer property
> instead of a phandle, because at the end of the day it is a value field
> of a particular register and the reference is only used to retrieve that
> value. It is not like we are actually going to call functions on the bus
> instance or change its state. And for the single purpose of retrieving
> that value, this binding requires the addition of a new property on the
> bus node that will probably never be used for something else.
And this was how I used to implement this even earlier, but Thierry
objected to that since it was duplicating information :)
>
> We can also imagine that some design may not have a PMIC device in the
> DT but still want to use the thermal reset feature. In this case, it is
> again preferable to use direct values.
>
I don't see how that'd change anything; the hardware feature still
requires an I2C device to send a message to. If there's an I2C device on
one of the SoC buses, you probably should have it in the DT..
Cheers,
Mikko
On Wed, Nov 12, 2014 at 02:07:51PM +0200, Mikko Perttunen wrote:
> On 11/11/2014 08:37 AM, Alexandre Courbot wrote:
> >On 11/10/2014 10:12 PM, Mikko Perttunen wrote:
> >>From: Mikko Perttunen <[email protected]>
> >>
> >>Hardware-triggered thermal reset requires configuring the I2C
> >>reset procedure. This configuration is read from the device tree,
> >>so document the relevant properties in the binding documentation.
> >>
> >>Signed-off-by: Mikko Perttunen <[email protected]>
> >>Reviewed-by: Wei Ni <[email protected]>
> >>Tested-by: Wei Ni <[email protected]>
> >>---
> >> .../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24
> >>++++++++++++++++++++++
> >> 1 file changed, 24 insertions(+)
> >>
> >>diff --git
> >>a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> >>b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> >>index 68ac65f..dc13fb0 100644
> >>--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> >>+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> >>@@ -47,6 +47,21 @@ Required properties when nvidia,suspend-mode=<0>:
> >> sleep mode, the warm boot code will restore some PLLs, clocks and
> >>then
> >> bring up CPU0 for resuming the system.
> >>
> >>+Hardware-triggered thermal reset:
> >>+On Tegra30, Tegra114 and Tegra124, if the 'i2c-thermtrip' subnode
> >>exists,
> >>+hardware-triggered thermal reset will be enabled.
> >>+
> >>+Required properties for hardware-triggered thermal reset (inside
> >>'i2c-thermtrip'):
> >>+- nvidia,i2c-bus : Phandle to I2C bus containing the PMU
> >>+- nvidia,bus-addr : Bus address of the PMU on the I2C bus
> >>+- nvidia,reg-addr : I2C register address to write poweroff command to
> >>+- nvidia,reg-data : Poweroff command to write to PMU
> >
> >This binding is taking two different routes to provide values to the
> >driver:
> >
> >1) It uses a phandle for i2c-bus (which must then be provided by another
> >binding implemented in the two following patches)
> >
> >2) It uses direct values for bus-addr, reg-addr and reg-data.
> >
> >Do we need to use both approaches? bus-addr could just as well be
> >obtained through a phandle to the i2c device and reading its reg
> >property. From this phandle you could also go back up to the bus, making
> >the i2c-bus property unnecessary. reg-addr and reg-data cannot be
> >specified that way, obviously.
>
> This was in fact how I used to implement this, but Stephen or Thierry
> pointed out that the reg property actually might not contain the correct
> address (I think because the PMIC could have multiple addresses, and the one
> in DT might not be the one that accepts the reset command).
>
> The workaround for that was to either add this integer property for bus-addr
> or add a new PMIC API for querying. I went for this as it is much simpler.
>
> >
> >Actually I think I'd prefer to see i2c-bus become an integer property
> >instead of a phandle, because at the end of the day it is a value field
> >of a particular register and the reference is only used to retrieve that
> >value. It is not like we are actually going to call functions on the bus
> >instance or change its state. And for the single purpose of retrieving
> >that value, this binding requires the addition of a new property on the
> >bus node that will probably never be used for something else.
>
> And this was how I used to implement this even earlier, but Thierry objected
> to that since it was duplicating information :)
If I remember correctly what I was asking for was to derive as much as
possible from simply a phandle. That is, what I was after is a phandle
to the PMU and ideally a way for the PMU to pass back information about
the register and value for the power off command.
Given the lack of a PMU abstraction and how this is probably very Tegra
specific I was okay with leaving reg-addr and reg-data in the DT. But if
we can't derive even the slave address from a phandle along with the I2C
bus master, then I think there remains little point in doing it this way
at all.
If we're going to duplicate three properties, adding a fourth isn't
going to make it much worse.
Thierry
On 11/12/2014 02:29 PM, Thierry Reding wrote:
> On Wed, Nov 12, 2014 at 02:07:51PM +0200, Mikko Perttunen wrote:
>> On 11/11/2014 08:37 AM, Alexandre Courbot wrote:
>>> On 11/10/2014 10:12 PM, Mikko Perttunen wrote:
>>>> From: Mikko Perttunen <[email protected]>
>>>>
>>>> Hardware-triggered thermal reset requires configuring the I2C
>>>> reset procedure. This configuration is read from the device tree,
>>>> so document the relevant properties in the binding documentation.
>>>>
>>>> Signed-off-by: Mikko Perttunen <[email protected]>
>>>> Reviewed-by: Wei Ni <[email protected]>
>>>> Tested-by: Wei Ni <[email protected]>
>>>> ---
>>>> .../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24
>>>> ++++++++++++++++++++++
>>>> 1 file changed, 24 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> index 68ac65f..dc13fb0 100644
>>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> @@ -47,6 +47,21 @@ Required properties when nvidia,suspend-mode=<0>:
>>>> sleep mode, the warm boot code will restore some PLLs, clocks and
>>>> then
>>>> bring up CPU0 for resuming the system.
>>>>
>>>> +Hardware-triggered thermal reset:
>>>> +On Tegra30, Tegra114 and Tegra124, if the 'i2c-thermtrip' subnode
>>>> exists,
>>>> +hardware-triggered thermal reset will be enabled.
>>>> +
>>>> +Required properties for hardware-triggered thermal reset (inside
>>>> 'i2c-thermtrip'):
>>>> +- nvidia,i2c-bus : Phandle to I2C bus containing the PMU
>>>> +- nvidia,bus-addr : Bus address of the PMU on the I2C bus
>>>> +- nvidia,reg-addr : I2C register address to write poweroff command to
>>>> +- nvidia,reg-data : Poweroff command to write to PMU
>>>
>>> This binding is taking two different routes to provide values to the
>>> driver:
>>>
>>> 1) It uses a phandle for i2c-bus (which must then be provided by another
>>> binding implemented in the two following patches)
>>>
>>> 2) It uses direct values for bus-addr, reg-addr and reg-data.
>>>
>>> Do we need to use both approaches? bus-addr could just as well be
>>> obtained through a phandle to the i2c device and reading its reg
>>> property. From this phandle you could also go back up to the bus, making
>>> the i2c-bus property unnecessary. reg-addr and reg-data cannot be
>>> specified that way, obviously.
>>
>> This was in fact how I used to implement this, but Stephen or Thierry
>> pointed out that the reg property actually might not contain the correct
>> address (I think because the PMIC could have multiple addresses, and the one
>> in DT might not be the one that accepts the reset command).
>>
>> The workaround for that was to either add this integer property for bus-addr
>> or add a new PMIC API for querying. I went for this as it is much simpler.
>>
>>>
>>> Actually I think I'd prefer to see i2c-bus become an integer property
>>> instead of a phandle, because at the end of the day it is a value field
>>> of a particular register and the reference is only used to retrieve that
>>> value. It is not like we are actually going to call functions on the bus
>>> instance or change its state. And for the single purpose of retrieving
>>> that value, this binding requires the addition of a new property on the
>>> bus node that will probably never be used for something else.
>>
>> And this was how I used to implement this even earlier, but Thierry objected
>> to that since it was duplicating information :)
>
> If I remember correctly what I was asking for was to derive as much as
> possible from simply a phandle. That is, what I was after is a phandle
> to the PMU and ideally a way for the PMU to pass back information about
> the register and value for the power off command.
>
> Given the lack of a PMU abstraction and how this is probably very Tegra
> specific I was okay with leaving reg-addr and reg-data in the DT. But if
> we can't derive even the slave address from a phandle along with the I2C
> bus master, then I think there remains little point in doing it this way
> at all.
>
> If we're going to duplicate three properties, adding a fourth isn't
> going to make it much worse.
>
> Thierry
>
Yeah, I guess that's sensible. I'll change the phandle to an integer if
that's preferred.
Mikko
On 11/12/2014 10:07 PM, Mikko Perttunen wrote:
> On 11/12/2014 02:29 PM, Thierry Reding wrote:
>> On Wed, Nov 12, 2014 at 02:07:51PM +0200, Mikko Perttunen wrote:
>>> On 11/11/2014 08:37 AM, Alexandre Courbot wrote:
>>>> On 11/10/2014 10:12 PM, Mikko Perttunen wrote:
>>>>> From: Mikko Perttunen <[email protected]>
>>>>>
>>>>> Hardware-triggered thermal reset requires configuring the I2C
>>>>> reset procedure. This configuration is read from the device tree,
>>>>> so document the relevant properties in the binding documentation.
>>>>>
>>>>> Signed-off-by: Mikko Perttunen <[email protected]>
>>>>> Reviewed-by: Wei Ni <[email protected]>
>>>>> Tested-by: Wei Ni <[email protected]>
>>>>> ---
>>>>> .../bindings/arm/tegra/nvidia,tegra20-pmc.txt | 24
>>>>> ++++++++++++++++++++++
>>>>> 1 file changed, 24 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> index 68ac65f..dc13fb0 100644
>>>>> ---
>>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> +++
>>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> @@ -47,6 +47,21 @@ Required properties when nvidia,suspend-mode=<0>:
>>>>> sleep mode, the warm boot code will restore some PLLs, clocks and
>>>>> then
>>>>> bring up CPU0 for resuming the system.
>>>>>
>>>>> +Hardware-triggered thermal reset:
>>>>> +On Tegra30, Tegra114 and Tegra124, if the 'i2c-thermtrip' subnode
>>>>> exists,
>>>>> +hardware-triggered thermal reset will be enabled.
>>>>> +
>>>>> +Required properties for hardware-triggered thermal reset (inside
>>>>> 'i2c-thermtrip'):
>>>>> +- nvidia,i2c-bus : Phandle to I2C bus containing the PMU
>>>>> +- nvidia,bus-addr : Bus address of the PMU on the I2C bus
>>>>> +- nvidia,reg-addr : I2C register address to write poweroff command to
>>>>> +- nvidia,reg-data : Poweroff command to write to PMU
>>>>
>>>> This binding is taking two different routes to provide values to the
>>>> driver:
>>>>
>>>> 1) It uses a phandle for i2c-bus (which must then be provided by
>>>> another
>>>> binding implemented in the two following patches)
>>>>
>>>> 2) It uses direct values for bus-addr, reg-addr and reg-data.
>>>>
>>>> Do we need to use both approaches? bus-addr could just as well be
>>>> obtained through a phandle to the i2c device and reading its reg
>>>> property. From this phandle you could also go back up to the bus,
>>>> making
>>>> the i2c-bus property unnecessary. reg-addr and reg-data cannot be
>>>> specified that way, obviously.
>>>
>>> This was in fact how I used to implement this, but Stephen or Thierry
>>> pointed out that the reg property actually might not contain the correct
>>> address (I think because the PMIC could have multiple addresses, and
>>> the one
>>> in DT might not be the one that accepts the reset command).
>>>
>>> The workaround for that was to either add this integer property for
>>> bus-addr
>>> or add a new PMIC API for querying. I went for this as it is much
>>> simpler.
>>>
>>>>
>>>> Actually I think I'd prefer to see i2c-bus become an integer property
>>>> instead of a phandle, because at the end of the day it is a value field
>>>> of a particular register and the reference is only used to retrieve
>>>> that
>>>> value. It is not like we are actually going to call functions on the
>>>> bus
>>>> instance or change its state. And for the single purpose of retrieving
>>>> that value, this binding requires the addition of a new property on the
>>>> bus node that will probably never be used for something else.
>>>
>>> And this was how I used to implement this even earlier, but Thierry
>>> objected
>>> to that since it was duplicating information :)
>>
>> If I remember correctly what I was asking for was to derive as much as
>> possible from simply a phandle. That is, what I was after is a phandle
>> to the PMU and ideally a way for the PMU to pass back information about
>> the register and value for the power off command.
>>
>> Given the lack of a PMU abstraction and how this is probably very Tegra
>> specific I was okay with leaving reg-addr and reg-data in the DT. But if
>> we can't derive even the slave address from a phandle along with the I2C
>> bus master, then I think there remains little point in doing it this way
>> at all.
>>
>> If we're going to duplicate three properties, adding a fourth isn't
>> going to make it much worse.
>>
>> Thierry
>>
>
> Yeah, I guess that's sensible. I'll change the phandle to an integer if
> that's preferred.
As far as I'm concerned, definitely - at the end of the day these values
are really here just to be written into a register, and not because we
plan to do fancy things with the PMU. So to me it is really not an issue
if the DT bindings reflect that fact (not to mention doing otherwise
would be uselessly cumbersome).