On imx6sx accessing the ocotp memory area directly is wrong because the
ocotp clock needs to be enabled first. Fix this by reinterpreting the
fsl,tempmon-data phandle as a reference to a nvmem_device and doing all
the read through that.
This clock requirement does not apply to older imx6qdl chips because
there the ocotp access clock (clk_ipg_s) is always enabled.
This is visible by comparing the "System Clocks, Gating, and Override"
tables (OCOTP rows) in the 6DQ and 6SX manuals:
http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6SXRM.pdf
http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf
This happens to work right now without this patch because the ocotp
clock might be enabled for some other reason. In particular it might be
enabled from the bootloader and it only gets disabled late during boot
in clk_disable_unused, after imx-thermal has completed probing.
If imx-thermal is compiled as a module then the system can hang on
probe.
This makes IMX_THERMAL depend on NVMEM_IMX_OCOTP but that driver seems
be already available for all chips that contain tempmon so it's
acceptable.
Reported-by: Lothar Waßmann <[email protected]>
Signed-off-by: Leonard Crestez <[email protected]>
---
This was reported as a comment to a patch adding tempmon support for
imx6ul (which is very similar to imx6sx). Since it already affects a
supported chip this patch is sent as a separate bugfix.
Link: https://lkml.org/lkml/2017/6/9/578
There are various other ways to fix this problem. The main advantage of
this solution is that it does not add a new binding but rather preserves
compatibility with old DTBs. It also aligns with the idea that
devicetree describes hardware relationships rather than a specific linux
implementation.
An alternative would have been to add a nvmem-cells binding to imx-thermal and
use that if available instead of fsl,tempmon-data. It might not be good to
sidestep the official nvmem bindings, the devicetree people were added so that
they have an opportunity to object.
In theory the "thermal grade" is a two-bit quantity and might be a
candidate for using a cell with a "bits" binding. However this causes
the nvmem core to issue reads of length and alignment less than 4 to the
imx-ocotp driver so additional fixes might be required.
drivers/nvmem/core.c | 15 ++++++++++++
drivers/thermal/Kconfig | 2 +-
drivers/thermal/imx_thermal.c | 53 ++++++++++++++++++++++++++----------------
include/linux/nvmem-consumer.h | 6 +++++
4 files changed, 55 insertions(+), 21 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 8c830a8..66502ca 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -630,6 +630,21 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
return __nvmem_device_get(nvmem_np, NULL, NULL);
}
EXPORT_SYMBOL_GPL(of_nvmem_device_get);
+
+/**
+ * of_nvmem_device_phandle_get() - Get nvmem device from a given phandle
+ *
+ * @nvmem_np: Device tree node for the nvmem device
+ *
+ * Return: ERR_PTR() on error or a valid pointer to a struct nvmem_device
+ * on success.
+ */
+struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np)
+{
+
+ return __nvmem_device_get(nvmem_np, NULL, NULL);
+}
+EXPORT_SYMBOL_GPL(of_nvmem_device_phandle_get);
#endif
/**
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index b5b5fac..a427936 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -206,7 +206,7 @@ config HISI_THERMAL
config IMX_THERMAL
tristate "Temperature sensor driver for Freescale i.MX SoCs"
depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
- depends on MFD_SYSCON
+ depends on NVMEM_IMX_OCOTP
depends on OF
help
Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs.
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index fb648a4..1cf35bd 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -24,6 +24,7 @@
#include <linux/slab.h>
#include <linux/thermal.h>
#include <linux/types.h>
+#include <linux/nvmem-consumer.h>
#define REG_SET 0x4
#define REG_CLR 0x8
@@ -55,8 +56,8 @@
#define TEMPSENSE2_PANIC_VALUE_SHIFT 16
#define TEMPSENSE2_PANIC_VALUE_MASK 0xfff0000
-#define OCOTP_MEM0 0x0480
-#define OCOTP_ANA1 0x04e0
+#define OCOTP_MEM0_OFFSET 32
+#define OCOTP_ANA1_OFFSET 56
/* The driver supports 1 passive trip point and 1 critical trip point */
enum imx_thermal_trip {
@@ -347,29 +348,39 @@ static struct thermal_zone_device_ops imx_tz_ops = {
static int imx_get_sensor_data(struct platform_device *pdev)
{
struct imx_thermal_data *data = platform_get_drvdata(pdev);
- struct regmap *map;
+ struct device_node *ocotp_np;
+ struct nvmem_device *ocotp;
int t1, n1;
int ret;
u32 val;
u64 temp64;
- map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
- "fsl,tempmon-data");
- if (IS_ERR(map)) {
- ret = PTR_ERR(map);
- dev_err(&pdev->dev, "failed to get sensor regmap: %d\n", ret);
+ ocotp_np = of_parse_phandle(pdev->dev.of_node, "fsl,tempmon-data", 0);
+ if (IS_ERR(ocotp_np)) {
+ ret = PTR_ERR(ocotp_np);
+ dev_err(&pdev->dev, "failed to parse fsl,tempmon-data phandle: %d\n", ret);
+ return ret;
+ }
+ ocotp = of_nvmem_device_phandle_get(ocotp_np);
+ of_node_put(ocotp_np);
+ if (IS_ERR(ocotp)) {
+ ret = PTR_ERR(ocotp);
+ if (ret != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "failed to get fsl,tempmon-data nvmem device: %d\n", ret);
return ret;
}
- ret = regmap_read(map, OCOTP_ANA1, &val);
- if (ret) {
+ ret = nvmem_device_read(ocotp, OCOTP_ANA1_OFFSET, sizeof(val), &val);
+ if (ret != sizeof(val)) {
dev_err(&pdev->dev, "failed to read sensor data: %d\n", ret);
- return ret;
+ ret = -EIO;
+ goto out;
}
if (val == 0 || val == ~0) {
dev_err(&pdev->dev, "invalid sensor calibration data\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
/*
@@ -404,10 +415,11 @@ static int imx_get_sensor_data(struct platform_device *pdev)
data->c2 = n1 * data->c1 + 1000 * t1;
/* use OTP for thermal grade */
- ret = regmap_read(map, OCOTP_MEM0, &val);
- if (ret) {
- dev_err(&pdev->dev, "failed to read temp grade: %d\n", ret);
- return ret;
+ ret = nvmem_device_read(ocotp, OCOTP_MEM0_OFFSET, sizeof(val), &val);
+ if (ret != sizeof(val)) {
+ dev_err(&pdev->dev, "failed to read sensor data: %d\n", ret);
+ ret = -EIO;
+ goto out;
}
/* The maximum die temp is specified by the Temperature Grade */
@@ -437,7 +449,10 @@ static int imx_get_sensor_data(struct platform_device *pdev)
data->temp_critical = data->temp_max - (1000 * 5);
data->temp_passive = data->temp_max - (1000 * 10);
- return 0;
+ ret = 0;
+out:
+ nvmem_device_put(ocotp);
+ return ret;
}
static irqreturn_t imx_thermal_alarm_irq(int irq, void *dev)
@@ -513,10 +528,8 @@ static int imx_thermal_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
ret = imx_get_sensor_data(pdev);
- if (ret) {
- dev_err(&pdev->dev, "failed to get sensor data\n");
+ if (ret)
return ret;
- }
/* Make sure sensor is in known good state for measurements */
regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index c2256d7..3606167 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -140,6 +140,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
const char *name);
struct nvmem_device *of_nvmem_device_get(struct device_node *np,
const char *name);
+struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np);
#else
static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
const char *name)
@@ -152,6 +153,11 @@ static inline struct nvmem_device *of_nvmem_device_get(struct device_node *np,
{
return ERR_PTR(-ENOSYS);
}
+
+static inline struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np)
+{
+ return ERR_PTR(-ENOSYS);
+}
#endif /* CONFIG_NVMEM && CONFIG_OF */
#endif /* ifndef _LINUX_NVMEM_CONSUMER_H */
--
2.7.4
On Mon, 2017-06-19 at 16:40 +0300, Leonard Crestez wrote:
> On imx6sx accessing the ocotp memory area directly is wrong because the
> ocotp clock needs to be enabled first. Fix this by reinterpreting the
> fsl,tempmon-data phandle as a reference to a nvmem_device and doing all
> the read through that.
>
> This clock requirement does not apply to older imx6qdl chips because
> there the ocotp access clock (clk_ipg_s) is always enabled.
>
> This is visible by comparing the "System Clocks, Gating, and Override"
> tables (OCOTP rows) in the 6DQ and 6SX manuals:
> http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6SXRM.pdf
> http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf
>
> This happens to work right now without this patch because the ocotp
> clock might be enabled for some other reason. In particular it might be
> enabled from the bootloader and it only gets disabled late during boot
> in clk_disable_unused, after imx-thermal has completed probing.
>
> If imx-thermal is compiled as a module then the system can hang on
> probe.
>
> This makes IMX_THERMAL depend on NVMEM_IMX_OCOTP but that driver seems
> be already available for all chips that contain tempmon so it's
> acceptable.
>
> Reported-by: Lothar Waßmann <[email protected]>
> Signed-off-by: Leonard Crestez <[email protected]>
>
> ---
>
> This was reported as a comment to a patch adding tempmon support for
> imx6ul (which is very similar to imx6sx). Since it already affects a
> supported chip this patch is sent as a separate bugfix.
>
> Link: https://lkml.org/lkml/2017/6/9/578
>
> There are various other ways to fix this problem. The main advantage of
> this solution is that it does not add a new binding but rather preserves
> compatibility with old DTBs. It also aligns with the idea that
> devicetree describes hardware relationships rather than a specific linux
> implementation.
>
> An alternative would have been to add a nvmem-cells binding to imx-thermal and
> use that if available instead of fsl,tempmon-data. It might not be good to
> sidestep the official nvmem bindings, the devicetree people were added so that
> they have an opportunity to object.
>
> In theory the "thermal grade" is a two-bit quantity and might be a
> candidate for using a cell with a "bits" binding. However this causes
> the nvmem core to issue reads of length and alignment less than 4 to the
> imx-ocotp driver so additional fixes might be required.
>
It's been two weeks without a reply, any advice on how to proceed?
Since the original message is quite long I will summarize the main
question:
Is it OK to reinterpret the fsl,tempmon-data property as a nvmem
reference instead of a "raw" syscon regmap?
--
Regards,
Leonard
On Mon, Jun 19, 2017 at 04:40:43PM +0300, Leonard Crestez wrote:
> On imx6sx accessing the ocotp memory area directly is wrong because the
> ocotp clock needs to be enabled first. Fix this by reinterpreting the
> fsl,tempmon-data phandle as a reference to a nvmem_device and doing all
> the read through that.
>
> This clock requirement does not apply to older imx6qdl chips because
> there the ocotp access clock (clk_ipg_s) is always enabled.
>
> This is visible by comparing the "System Clocks, Gating, and Override"
> tables (OCOTP rows) in the 6DQ and 6SX manuals:
> http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6SXRM.pdf
> http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf
>
> This happens to work right now without this patch because the ocotp
> clock might be enabled for some other reason. In particular it might be
> enabled from the bootloader and it only gets disabled late during boot
> in clk_disable_unused, after imx-thermal has completed probing.
>
> If imx-thermal is compiled as a module then the system can hang on
> probe.
>
> This makes IMX_THERMAL depend on NVMEM_IMX_OCOTP but that driver seems
> be already available for all chips that contain tempmon so it's
> acceptable.
>
> Reported-by: Lothar Wa?mann <[email protected]>
> Signed-off-by: Leonard Crestez <[email protected]>
>
> ---
>
> This was reported as a comment to a patch adding tempmon support for
> imx6ul (which is very similar to imx6sx). Since it already affects a
> supported chip this patch is sent as a separate bugfix.
>
> Link: https://lkml.org/lkml/2017/6/9/578
>
> There are various other ways to fix this problem. The main advantage of
> this solution is that it does not add a new binding but rather preserves
> compatibility with old DTBs. It also aligns with the idea that
> devicetree describes hardware relationships rather than a specific linux
> implementation.
>
> An alternative would have been to add a nvmem-cells binding to imx-thermal and
> use that if available instead of fsl,tempmon-data. It might not be good to
> sidestep the official nvmem bindings, the devicetree people were added so that
> they have an opportunity to object.
>
> In theory the "thermal grade" is a two-bit quantity and might be a
> candidate for using a cell with a "bits" binding. However this causes
> the nvmem core to issue reads of length and alignment less than 4 to the
> imx-ocotp driver so additional fixes might be required.
>
> drivers/nvmem/core.c | 15 ++++++++++++
> drivers/thermal/Kconfig | 2 +-
> drivers/thermal/imx_thermal.c | 53 ++++++++++++++++++++++++++----------------
> include/linux/nvmem-consumer.h | 6 +++++
> 4 files changed, 55 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 8c830a8..66502ca 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -630,6 +630,21 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
> return __nvmem_device_get(nvmem_np, NULL, NULL);
> }
> EXPORT_SYMBOL_GPL(of_nvmem_device_get);
> +
> +/**
> + * of_nvmem_device_phandle_get() - Get nvmem device from a given phandle
> + *
> + * @nvmem_np: Device tree node for the nvmem device
> + *
> + * Return: ERR_PTR() on error or a valid pointer to a struct nvmem_device
> + * on success.
> + */
> +struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np)
> +{
> +
> + return __nvmem_device_get(nvmem_np, NULL, NULL);
> +}
> +EXPORT_SYMBOL_GPL(of_nvmem_device_phandle_get);
> #endif
>
> /**
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index b5b5fac..a427936 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -206,7 +206,7 @@ config HISI_THERMAL
> config IMX_THERMAL
> tristate "Temperature sensor driver for Freescale i.MX SoCs"
> depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
> - depends on MFD_SYSCON
> + depends on NVMEM_IMX_OCOTP
> depends on OF
> help
> Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs.
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index fb648a4..1cf35bd 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -24,6 +24,7 @@
> #include <linux/slab.h>
> #include <linux/thermal.h>
> #include <linux/types.h>
> +#include <linux/nvmem-consumer.h>
>
> #define REG_SET 0x4
> #define REG_CLR 0x8
> @@ -55,8 +56,8 @@
> #define TEMPSENSE2_PANIC_VALUE_SHIFT 16
> #define TEMPSENSE2_PANIC_VALUE_MASK 0xfff0000
>
> -#define OCOTP_MEM0 0x0480
> -#define OCOTP_ANA1 0x04e0
> +#define OCOTP_MEM0_OFFSET 32
> +#define OCOTP_ANA1_OFFSET 56
>
> /* The driver supports 1 passive trip point and 1 critical trip point */
> enum imx_thermal_trip {
> @@ -347,29 +348,39 @@ static struct thermal_zone_device_ops imx_tz_ops = {
> static int imx_get_sensor_data(struct platform_device *pdev)
> {
> struct imx_thermal_data *data = platform_get_drvdata(pdev);
> - struct regmap *map;
> + struct device_node *ocotp_np;
> + struct nvmem_device *ocotp;
> int t1, n1;
> int ret;
> u32 val;
> u64 temp64;
>
> - map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> - "fsl,tempmon-data");
> - if (IS_ERR(map)) {
> - ret = PTR_ERR(map);
> - dev_err(&pdev->dev, "failed to get sensor regmap: %d\n", ret);
> + ocotp_np = of_parse_phandle(pdev->dev.of_node, "fsl,tempmon-data", 0);
> + if (IS_ERR(ocotp_np)) {
> + ret = PTR_ERR(ocotp_np);
> + dev_err(&pdev->dev, "failed to parse fsl,tempmon-data phandle: %d\n", ret);
> + return ret;
> + }
> + ocotp = of_nvmem_device_phandle_get(ocotp_np);
> + of_node_put(ocotp_np);
> + if (IS_ERR(ocotp)) {
> + ret = PTR_ERR(ocotp);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to get fsl,tempmon-data nvmem device: %d\n", ret);
> return ret;
> }
>
> - ret = regmap_read(map, OCOTP_ANA1, &val);
> - if (ret) {
> + ret = nvmem_device_read(ocotp, OCOTP_ANA1_OFFSET, sizeof(val), &val);
> + if (ret != sizeof(val)) {
> dev_err(&pdev->dev, "failed to read sensor data: %d\n", ret);
> - return ret;
> + ret = -EIO;
> + goto out;
> }
>
> if (val == 0 || val == ~0) {
> dev_err(&pdev->dev, "invalid sensor calibration data\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> /*
> @@ -404,10 +415,11 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> data->c2 = n1 * data->c1 + 1000 * t1;
>
> /* use OTP for thermal grade */
> - ret = regmap_read(map, OCOTP_MEM0, &val);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to read temp grade: %d\n", ret);
> - return ret;
I see a few other occurences of regmap_read() in this driver, for
example, inside imx_get_temp(). Do they also get affect by the reported
bug? Should they be replaced with nvmem_device_read() too?
> + ret = nvmem_device_read(ocotp, OCOTP_MEM0_OFFSET, sizeof(val), &val);
> + if (ret != sizeof(val)) {
> + dev_err(&pdev->dev, "failed to read sensor data: %d\n", ret);
> + ret = -EIO;
> + goto out;
> }
>
> /* The maximum die temp is specified by the Temperature Grade */
> @@ -437,7 +449,10 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> data->temp_critical = data->temp_max - (1000 * 5);
> data->temp_passive = data->temp_max - (1000 * 10);
>
> - return 0;
> + ret = 0;
> +out:
> + nvmem_device_put(ocotp);
> + return ret;
> }
>
> static irqreturn_t imx_thermal_alarm_irq(int irq, void *dev)
> @@ -513,10 +528,8 @@ static int imx_thermal_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, data);
>
> ret = imx_get_sensor_data(pdev);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to get sensor data\n");
> + if (ret)
> return ret;
> - }
>
> /* Make sure sensor is in known good state for measurements */
> regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index c2256d7..3606167 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -140,6 +140,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> const char *name);
> struct nvmem_device *of_nvmem_device_get(struct device_node *np,
> const char *name);
> +struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np);
> #else
> static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> const char *name)
> @@ -152,6 +153,11 @@ static inline struct nvmem_device *of_nvmem_device_get(struct device_node *np,
> {
> return ERR_PTR(-ENOSYS);
> }
> +
> +static inline struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> #endif /* CONFIG_NVMEM && CONFIG_OF */
>
> #endif /* ifndef _LINUX_NVMEM_CONSUMER_H */
> --
> 2.7.4
>
On 19/06/17 14:40, Leonard Crestez wrote:
> On imx6sx accessing the ocotp memory area directly is wrong because the
> ocotp clock needs to be enabled first. Fix this by reinterpreting the
> fsl,tempmon-data phandle as a reference to a nvmem_device and doing all
> the read through that.
This looks like a clear workaround to the issue, You should follow NVMEM
bindings, not add new API to bypass these.
Am against this!
I would expect imx tempmon to use the nvmem cell to refer to the data.
You should probably fix the bindings for imx tempmon driver to address
this issue.
>
> This clock requirement does not apply to older imx6qdl chips because
> there the ocotp access clock (clk_ipg_s) is always enabled.
>
> This is visible by comparing the "System Clocks, Gating, and Override"
> tables (OCOTP rows) in the 6DQ and 6SX manuals:
> http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6SXRM.pdf
> http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf
>
> This happens to work right now without this patch because the ocotp
> clock might be enabled for some other reason. In particular it might be
> enabled from the bootloader and it only gets disabled late during boot
> in clk_disable_unused, after imx-thermal has completed probing.
>
> If imx-thermal is compiled as a module then the system can hang on
> probe.
>
> This makes IMX_THERMAL depend on NVMEM_IMX_OCOTP but that driver seems
> be already available for all chips that contain tempmon so it's
> acceptable.
>
> Reported-by: Lothar Waßmann <[email protected]>
> Signed-off-by: Leonard Crestez <[email protected]>
>
> ---
>
> This was reported as a comment to a patch adding tempmon support for
> imx6ul (which is very similar to imx6sx). Since it already affects a
> supported chip this patch is sent as a separate bugfix.
>
> Link: https://lkml.org/lkml/2017/6/9/578
>
> There are various other ways to fix this problem. The main advantage of
> this solution is that it does not add a new binding but rather preserves
> compatibility with old DTBs. It also aligns with the idea that
> devicetree describes hardware relationships rather than a specific linux
> implementation.
>
> An alternative would have been to add a nvmem-cells binding to imx-thermal and
> use that if available instead of fsl,tempmon-data. It might not be good to
> sidestep the official nvmem bindings, the devicetree people were added so that
> they have an opportunity to object.
Not sure why you think nvmem bindings would change in this case, Only
the tempmon bindings would be fixed to address the issue.
>
> In theory the "thermal grade" is a two-bit quantity and might be a
> candidate for using a cell with a "bits" binding. However this causes
> the nvmem core to issue reads of length and alignment less than 4 to the
> imx-ocotp driver so additional fixes might be required.
This would depend on what sride and wordsize is set in the nvmem provider.
Thanks,
srini
>
> drivers/nvmem/core.c | 15 ++++++++++++
> drivers/thermal/Kconfig | 2 +-
> drivers/thermal/imx_thermal.c | 53 ++++++++++++++++++++++++++----------------
> include/linux/nvmem-consumer.h | 6 +++++
> 4 files changed, 55 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 8c830a8..66502ca 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -630,6 +630,21 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
> return __nvmem_device_get(nvmem_np, NULL, NULL);
> }
> EXPORT_SYMBOL_GPL(of_nvmem_device_get);
> +
> +/**
> + * of_nvmem_device_phandle_get() - Get nvmem device from a given phandle
> + *
> + * @nvmem_np: Device tree node for the nvmem device
> + *
> + * Return: ERR_PTR() on error or a valid pointer to a struct nvmem_device
> + * on success.
> + */
> +struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np)
> +{
> +
> + return __nvmem_device_get(nvmem_np, NULL, NULL);
> +}
> +EXPORT_SYMBOL_GPL(of_nvmem_device_phandle_get);
> #endif
>
> /**
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index b5b5fac..a427936 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -206,7 +206,7 @@ config HISI_THERMAL
> config IMX_THERMAL
> tristate "Temperature sensor driver for Freescale i.MX SoCs"
> depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
> - depends on MFD_SYSCON
> + depends on NVMEM_IMX_OCOTP
> depends on OF
> help
> Support for Temperature Monitor (TEMPMON) found on Freescale i.MX SoCs.
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index fb648a4..1cf35bd 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -24,6 +24,7 @@
> #include <linux/slab.h>
> #include <linux/thermal.h>
> #include <linux/types.h>
> +#include <linux/nvmem-consumer.h>
>
> #define REG_SET 0x4
> #define REG_CLR 0x8
> @@ -55,8 +56,8 @@
> #define TEMPSENSE2_PANIC_VALUE_SHIFT 16
> #define TEMPSENSE2_PANIC_VALUE_MASK 0xfff0000
>
> -#define OCOTP_MEM0 0x0480
> -#define OCOTP_ANA1 0x04e0
> +#define OCOTP_MEM0_OFFSET 32
> +#define OCOTP_ANA1_OFFSET 56
>
> /* The driver supports 1 passive trip point and 1 critical trip point */
> enum imx_thermal_trip {
> @@ -347,29 +348,39 @@ static struct thermal_zone_device_ops imx_tz_ops = {
> static int imx_get_sensor_data(struct platform_device *pdev)
> {
> struct imx_thermal_data *data = platform_get_drvdata(pdev);
> - struct regmap *map;
> + struct device_node *ocotp_np;
> + struct nvmem_device *ocotp;
> int t1, n1;
> int ret;
> u32 val;
> u64 temp64;
>
> - map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> - "fsl,tempmon-data");
> - if (IS_ERR(map)) {
> - ret = PTR_ERR(map);
> - dev_err(&pdev->dev, "failed to get sensor regmap: %d\n", ret);
> + ocotp_np = of_parse_phandle(pdev->dev.of_node, "fsl,tempmon-data", 0);
> + if (IS_ERR(ocotp_np)) {
> + ret = PTR_ERR(ocotp_np);
> + dev_err(&pdev->dev, "failed to parse fsl,tempmon-data phandle: %d\n", ret);
> + return ret;
> + }
> + ocotp = of_nvmem_device_phandle_get(ocotp_np);
> + of_node_put(ocotp_np);
> + if (IS_ERR(ocotp)) {
> + ret = PTR_ERR(ocotp);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to get fsl,tempmon-data nvmem device: %d\n", ret);
> return ret;
> }
>
> - ret = regmap_read(map, OCOTP_ANA1, &val);
> - if (ret) {
> + ret = nvmem_device_read(ocotp, OCOTP_ANA1_OFFSET, sizeof(val), &val);
> + if (ret != sizeof(val)) {
> dev_err(&pdev->dev, "failed to read sensor data: %d\n", ret);
> - return ret;
> + ret = -EIO;
> + goto out;
> }
>
> if (val == 0 || val == ~0) {
> dev_err(&pdev->dev, "invalid sensor calibration data\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> /*
> @@ -404,10 +415,11 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> data->c2 = n1 * data->c1 + 1000 * t1;
>
> /* use OTP for thermal grade */
> - ret = regmap_read(map, OCOTP_MEM0, &val);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to read temp grade: %d\n", ret);
> - return ret;
> + ret = nvmem_device_read(ocotp, OCOTP_MEM0_OFFSET, sizeof(val), &val);
> + if (ret != sizeof(val)) {
> + dev_err(&pdev->dev, "failed to read sensor data: %d\n", ret);
> + ret = -EIO;
> + goto out;
> }
>
> /* The maximum die temp is specified by the Temperature Grade */
> @@ -437,7 +449,10 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> data->temp_critical = data->temp_max - (1000 * 5);
> data->temp_passive = data->temp_max - (1000 * 10);
>
> - return 0;
> + ret = 0;
> +out:
> + nvmem_device_put(ocotp);
> + return ret;
> }
>
> static irqreturn_t imx_thermal_alarm_irq(int irq, void *dev)
> @@ -513,10 +528,8 @@ static int imx_thermal_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, data);
>
> ret = imx_get_sensor_data(pdev);
> - if (ret) {
> - dev_err(&pdev->dev, "failed to get sensor data\n");
> + if (ret)
> return ret;
> - }
>
> /* Make sure sensor is in known good state for measurements */
> regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_POWER_DOWN);
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index c2256d7..3606167 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -140,6 +140,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> const char *name);
> struct nvmem_device *of_nvmem_device_get(struct device_node *np,
> const char *name);
> +struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np);
> #else
> static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> const char *name)
> @@ -152,6 +153,11 @@ static inline struct nvmem_device *of_nvmem_device_get(struct device_node *np,
> {
> return ERR_PTR(-ENOSYS);
> }
> +
> +static inline struct nvmem_device *of_nvmem_device_phandle_get(struct device_node *nvmem_np)
> +{
> + return ERR_PTR(-ENOSYS);
> +}
> #endif /* CONFIG_NVMEM && CONFIG_OF */
>
> #endif /* ifndef _LINUX_NVMEM_CONSUMER_H */
>
On Tue, 2017-07-04 at 11:54 -0700, Eduardo Valentin wrote:
> On Mon, Jun 19, 2017 at 04:40:43PM +0300, Leonard Crestez wrote:
> >
> > On imx6sx accessing the ocotp memory area directly is wrong because the
> > ocotp clock needs to be enabled first. Fix this by reinterpreting the
> > fsl,tempmon-data phandle as a reference to a nvmem_device and doing all
> > the read through that.
> >
> > @@ -347,29 +348,39 @@ static struct thermal_zone_device_ops imx_tz_ops = {
> > static int imx_get_sensor_data(struct platform_device *pdev)
> > {
> > struct imx_thermal_data *data = platform_get_drvdata(pdev);
> > - struct regmap *map;
> > + struct device_node *ocotp_np;
> > + struct nvmem_device *ocotp;
> > int t1, n1;
> > int ret;
> > u32 val;
> > u64 temp64;
> >
> > - map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > - "fsl,tempmon-data");
> > - if (IS_ERR(map)) {
> > - ret = PTR_ERR(map);
> > - dev_err(&pdev->dev, "failed to get sensor regmap: %d\n", ret);
> > + ocotp_np = of_parse_phandle(pdev->dev.of_node, "fsl,tempmon-data", 0);
> > + if (IS_ERR(ocotp_np)) {
> > + ret = PTR_ERR(ocotp_np);
> > + dev_err(&pdev->dev, "failed to parse fsl,tempmon-data phandle: %d\n", ret);
> > + return ret;
> > + }
> > + ocotp = of_nvmem_device_phandle_get(ocotp_np);
> > + of_node_put(ocotp_np);
> > + if (IS_ERR(ocotp)) {
> > + ret = PTR_ERR(ocotp);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "failed to get fsl,tempmon-data nvmem device: %d\n", ret);
> > return ret;
> > }
> >
> > @@ -404,10 +415,11 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> > data->c2 = n1 * data->c1 + 1000 * t1;
> >
> > /* use OTP for thermal grade */
> > - ret = regmap_read(map, OCOTP_MEM0, &val);
> > - if (ret) {
> > - dev_err(&pdev->dev, "failed to read temp grade: %d\n", ret);
> > - return ret;
> I see a few other occurences of regmap_read() in this driver, for
> example, inside imx_get_temp(). Do they also get affect by the reported
> bug? Should they be replaced with nvmem_device_read() too?
No, most of the code uses regmap to read from the tempmon device
itself.
It is only imx_get_sensor_data which uses syscon_regmap_lookup to fetch
the regmap for another device (ocotp) in order to read calibration data
and thermal grading at probe time. Perhaps this function should be
renamed to something like imx_tempmon_init_calib.
The problem is that on newer imx SOCs reading directly from OCOTP like
that doesn't work if the ocotp clock is not enabled.
--
Regards,
Leonard