[RFC PATCH 0/2] QFPROM wrappers via syscon

[RFC PATCH 0/2] QFPROM wrappers via syscon

[Srinivas Kandagatla]

Hi All,

This discussion is triggered once again from recent chat with Stephen Boyd and
my need to access qfprom for tsens driver.

On MSM parts there are some efuses (called qfprom) these fuses store things like
calibration data, speed bins.. etc. Drivers like cpufreq, thermal sensors would
read out this data for configuring the driver. As qfprom is a mmio on QCOM parts
and is kinda shared resource, drivers which need data from qfrom can access it
by using syscon or use eeprom/efuse framework. Both of them, will do the job,
but as of today there is no eeprom, framework in the kernel, so accessing qfprom
via syscon seems to be more generic and the only option with current kernel
situation.

But there are some side effects of this approach,

1> code duplication: drivers which needs to access qfprom have to talk to syscon
and get hold of regmap, offset, size and then do regmap reads. This will be
kinda redone in every driver which needs access to qfprom.

2> handling multiple resources: with out some extra dt code in drivers its not
easy to get hold of multple qfprom resources.

3> register stride: syscon by default has a register stride of word size.

So this wrapper + "a syscon fix" addresses the above issues making syscon more
friendly for qfprom.

With this approach DT looks like:
qfprom: qfprom@00700000 {
	compatible      = "qcom,qfprom", "syscon";
 	reg             = <0x00700000 0x1000>;
	stride          = <1>;
}; 

tsens: tsens {
	#thermal-sensor-cells = <1>;
	compatible = "qcom,apq8064-tsens";

	qcom,qfprom = <&qfprom 0x404 0x10>, <&qfprom 0x414 0x10>;
	qcom,qfprom-names = "calib", "backup_calib";
};

and the driver side looks like:
	cdata = qfprom_get_data(dev, 0, &sz);
	cdata_backup = qfprom_get_data(dev, 1, &sz);


Questions:
- Should we wait for more generic eeprom/efuse interfaces? With no time line in place?
- Should we take a simplistic approach and move to using these wrappers till we
  get some eeprom/efuse apis in the kernel?
- Are there any better ways to represent qfprom via DT nodes?

Stephen had similar discussions [1] some time back in July 2014, but nothing
really changed since then. With the given situation Am proposing these patches
to get something that works on top of mainline which will unblock upstreaming
tsens and hopefully help other drivers like cpufreq an abstracted way to get
hold of qfprom data.

So here are my WIP patches for your comments/disussions.. :-)

Thanks,
srini
[1] http://comments.gmane.org/gmane.linux.ports.arm.msm/8142


Srinivas Kandagatla (2):
  WIP: mfd: syscon: Add register stride to DT bindings.
  WIP: Add wrappers for qfprom access via syscon

 Documentation/devicetree/bindings/mfd/syscon.txt   |   3 +
 .../devicetree/bindings/soc/qcom/qfprom.txt        |  29 +++++
 drivers/mfd/syscon.c                               |   9 ++
 drivers/soc/qcom/Kconfig                           |   7 ++
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/qfprom.c                          | 134 +++++++++++++++++++++
 6 files changed, 183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qfprom.txt
 create mode 100644 drivers/soc/qcom/qfprom.c

-- 
1.9.1

[RFC PATCH 1/2] WIP: mfd: syscon: Add register stride to DT bindings.

[Srinivas Kandagatla]

This patch adds register stride to dt bindings so that the consumers of
the syscon could change it to there need. One of the the use case for
this feature is Qualcomm qfprom which needs a byte access to regmap
returned from syscon.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 Documentation/devicetree/bindings/mfd/syscon.txt | 3 +++
 drivers/mfd/syscon.c                             | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index fe8150b..7f06ec1 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -13,6 +13,9 @@ Required properties:
 - compatible: Should contain "syscon".
 - reg: the register region can be accessed from syscon
 
+Optional properties:
+- stride : register address stride in bytes.
+
 Examples:
 gpr: iomuxc-gpr@020e0000 {
 	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 176bf0f..98769d5 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -48,6 +48,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	struct regmap *regmap;
 	void __iomem *base;
 	int ret;
+	u32 stride;
 	struct regmap_config syscon_config = syscon_regmap_config;
 
 	if (!of_device_is_compatible(np, "syscon"))
@@ -69,6 +70,14 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	 else if (of_property_read_bool(np, "little-endian"))
 		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
 
+	if (!of_property_read_u32(np, "stride", &stride)) {
+		if (stride > 4)
+			stride = 4;
+
+		syscon_config.reg_stride = stride;
+		syscon_config.val_bits = 8 * stride;
+	}
+
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	if (IS_ERR(regmap)) {
 		pr_err("regmap init failed\n");
-- 
1.9.1

[RFC PATCH 2/2] WIP: Add wrappers for qfprom access via syscon

[Srinivas Kandagatla]

Syscon fits very well to access qfprom. This also means drivers which needs to
access qfprom have to talk to syscon and get regmap, offset, size and then do
regmap reads. This will be kinda redone in every driver. Having a wrapper for
this would avoid lot of code duplications and also provide a higher level and
user friendly apis for qfprom. This patch attempt to provide such wrappers.
This wrappers are easy way to use syscon for qfprom purposes.

Advantages of this approch is:

- driver need not have hardcoded qfprom offsets or have soc specific
  compatible strings to determine the offset.
- access multiple qfprom resources which is kinda tricky with standard
  syscon.
- no code duplication.
- light weight, single call.
- not a platform device driver level binding.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/soc/qcom/qfprom.txt        |  29 +++++
 drivers/soc/qcom/Kconfig                           |   7 ++
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/qfprom.c                          | 134 +++++++++++++++++++++
 4 files changed, 171 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qfprom.txt
 create mode 100644 drivers/soc/qcom/qfprom.c

diff --git a/Documentation/devicetree/bindings/soc/qcom/qfprom.txt b/Documentation/devicetree/bindings/soc/qcom/qfprom.txt
new file mode 100644
index 0000000..3ed7309
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qfprom.txt
@@ -0,0 +1,29 @@
+QCOM QFPROM
+
+QFPROM is basically some efuses where things like calibration data, speed bins,
+etc are stored. This data is accessed by various drivers like the cpufreq,
+thermal, etc.
+
+Required properties:
+- compatible: must contain "qcom,qfprom" followed by "syscon"
+- reg: Address range for QFPROM
+- stride : register address stride.
+	1 for byte.
+	2 for 2 bytes
+	3 for 3 bytes
+	4 for a word.
+	
+
+Example:
+	qfprom: qfprom@00700000 {
+		compatible 	= "qcom,qfprom", "syscon";
+		reg		= <0x00700000 0x1000>;
+		stride		= <1>;
+	};
+
+	tsens@34000 {
+		compatible = "qcom,tsens-apq8064";
+		reg = <0x34000 0x1000>;
+		qcom,qfprom = <&qfprom 0x18 0x10>, <&qfprom 0x28 0x10>;
+		qcom,qfprom-names = "calib", "backup_calib";
+	};
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 012fb37..389ec3e 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -19,3 +19,10 @@ config QCOM_PM
 	  QCOM Platform specific power driver to manage cores and L2 low power
 	  modes. It interface with various system drivers to put the cores in
 	  low power modes.
+
+config QCOM_QFPROM
+	tristate "QCOM QFPROM Interface"
+        depends on ARCH_QCOM && OF
+        help
+          Say y here to enable QFPROM support. The QFPROM provides access
+          functions for QFPROM data to rest of the drivers via syscon wrappers.
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 20b329f..f5aff0a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_PM)	+=	spm.o
+obj-$(CONFIG_QCOM_QFPROM) +=	qfprom.o
 CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
 obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o
diff --git a/drivers/soc/qcom/qfprom.c b/drivers/soc/qcom/qfprom.c
new file mode 100644
index 0000000..d00ed25
--- /dev/null
+++ b/drivers/soc/qcom/qfprom.c
@@ -0,0 +1,134 @@
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/soc/qcom/qfprom.h>
+#include <linux/slab.h>
+
+#define QFPROM_MAX_ARGS	2
+
+static char *__qfprom_get_data(struct device *dev,
+				bool devm, int idx, int *len)
+{
+	struct device_node *syscon_np, *np = dev->of_node;
+	struct regmap *rm;
+	struct of_phandle_args args;
+	int rc, stride = 4;
+	u32 offset, size;
+	char *data;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	rc = of_parse_phandle_with_fixed_args(np, "qcom,qfprom",
+					      QFPROM_MAX_ARGS, idx, &args);
+	if (rc)
+		return ERR_PTR(rc);
+
+	syscon_np = args.np;
+
+	of_property_read_u32(syscon_np, "stride", &stride);
+
+	if (stride >= 4)
+		stride = 4;
+
+	if (args.args_count < QFPROM_MAX_ARGS) {
+		dev_err(dev, "Insufficient qfprom arguments %d\n",
+			args.args_count);
+		return ERR_PTR(-EINVAL);
+	}
+
+	rm = syscon_node_to_regmap(syscon_np);
+	if (IS_ERR(rm))
+		return ERR_CAST(rm);
+
+	offset = args.args[0];
+	size = args.args[1];
+
+	of_node_put(syscon_np);
+
+	if (devm)
+		data = devm_kzalloc(dev, size, GFP_KERNEL | GFP_ATOMIC);
+	else
+		data = kzalloc(size, GFP_KERNEL | GFP_ATOMIC);
+
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	rc = regmap_bulk_read(rm, offset, data, size/stride);
+	if (rc < 0) {
+		if (devm)
+			devm_kfree(dev, data);
+		else
+			kfree(data);
+
+		return ERR_PTR(rc);
+	}
+
+	*len = size;
+
+	return data;
+}
+
+static char *__qfprom_get_data_byname(struct device *dev,
+				       bool devm, const char *name, int *len)
+{
+	int index = 0;
+
+	if (name)
+		index = of_property_match_string(dev->of_node,
+						 "qcom,qfprom-names", name);
+
+	return __qfprom_get_data(dev, devm, index, len);
+}
+
+char *devm_qfprom_get_data_byname(struct device *dev,
+					  const char *name, int *len)
+{
+	return __qfprom_get_data_byname(dev, true, name, len);
+}
+EXPORT_SYMBOL_GPL(devm_qfprom_get_data_byname);
+
+char *devm_qfprom_get_data(struct device *dev,
+				   int index, int *len)
+{
+	return __qfprom_get_data(dev, true, index, len);
+}
+EXPORT_SYMBOL_GPL(devm_qfprom_get_data);
+
+/**
+ * qfprom_get_data_byname(): Reads qfprom data by name
+ *
+ * @dev: device which is requesting qfprom data
+ * @index: name of qfprom resources specified "qcom,qfprom-names" DT property.
+ * @len: length of data read from qfprom.
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a data buffer. The buffer should be freed by the user once its
+ * finished working with it kfree.
+ **/
+char *qfprom_get_data_byname(struct device *dev,
+				     const char *name, int *len)
+{
+	return __qfprom_get_data_byname(dev, false, name, len);
+}
+EXPORT_SYMBOL_GPL(qfprom_get_data_byname);
+
+/**
+ * qfprom_get_data(): Reads qfprom data from the index
+ *
+ * @dev: device which is requesting qfprom data
+ * @index: index into qfprom resources specified "qcom,qfprom" DT property.
+ * @len: length of data read from qfprom.
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a data buffer. The buffer should be freed by the user once its
+ * finished working with it kfree.
+ **/
+char *qfprom_get_data(struct device *dev,
+				   int index, int *len)
+{
+	return __qfprom_get_data(dev, false, index, len);
+}
+EXPORT_SYMBOL_GPL(qfprom_get_data);
-- 
1.9.1

RE: [RFC PATCH 2/2] WIP: Add wrappers for qfprom access via syscon

[Narendran Rajan]

Hi Srini,

The idea of abstracting qfrom reads in general is good, but looking at the 
Implementation this seems to add unwanted overhead.

Every read (if the driver is reading byte by byte especially) 
would involve a syscon node lookup,  memalloc and then regmap read.

To make it more efficient, two suggestions:

1) Instead of allocating memory in this library, let the clients allocate
memory
and provide a pointer (move memory management to client)
2) Provide one a time registration and then every read just specifies
relative offset
and length for every read

Usage pattern could be something like
struct qfprom *qfprom_register(struct device *dev, int index);
int devm_qfprom_get_data (struct qfprom *qfprom, int offset, int len, void
*data);

> -----Original Message-----
> From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-
> owner@vger.kernel.org] On Behalf Of Srinivas Kandagatla
> Sent: Monday, January 26, 2015 2:13 AM
> To: linux-arm-msm@vger.kernel.org
> Cc: patches@linaro.org; linaro-kernel@lists.linaro.org; Srinivas
Kandagatla
> Subject: [RFC PATCH 2/2] WIP: Add wrappers for qfprom access via syscon
> 
> Syscon fits very well to access qfprom. This also means drivers which
needs
> to access qfprom have to talk to syscon and get regmap, offset, size and
then
> do regmap reads. This will be kinda redone in every driver. Having a
wrapper
> for this would avoid lot of code duplications and also provide a higher
level
> and user friendly apis for qfprom. This patch attempt to provide such
> wrappers.
> This wrappers are easy way to use syscon for qfprom purposes.
> 
> Advantages of this approch is:
> 
> - driver need not have hardcoded qfprom offsets or have soc specific
>   compatible strings to determine the offset.
> - access multiple qfprom resources which is kinda tricky with standard
>   syscon.
> - no code duplication.
> - light weight, single call.
> - not a platform device driver level binding.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../devicetree/bindings/soc/qcom/qfprom.txt        |  29 +++++
>  drivers/soc/qcom/Kconfig                           |   7 ++
>  drivers/soc/qcom/Makefile                          |   1 +
>  drivers/soc/qcom/qfprom.c                          | 134
+++++++++++++++++++++
>  4 files changed, 171 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/soc/qcom/qfprom.txt
>  create mode 100644 drivers/soc/qcom/qfprom.c
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qfprom.txt
> b/Documentation/devicetree/bindings/soc/qcom/qfprom.txt
> new file mode 100644
> index 0000000..3ed7309
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qfprom.txt
> @@ -0,0 +1,29 @@
> +QCOM QFPROM
> +
> +QFPROM is basically some efuses where things like calibration data,
> +speed bins, etc are stored. This data is accessed by various drivers
> +like the cpufreq, thermal, etc.
> +
> +Required properties:
> +- compatible: must contain "qcom,qfprom" followed by "syscon"
> +- reg: Address range for QFPROM
> +- stride : register address stride.
> +	1 for byte.
> +	2 for 2 bytes
> +	3 for 3 bytes
> +	4 for a word.
> +
> +
> +Example:
> +	qfprom: qfprom@00700000 {
> +		compatible 	= "qcom,qfprom", "syscon";
> +		reg		= <0x00700000 0x1000>;
> +		stride		= <1>;
> +	};
> +
> +	tsens@34000 {
> +		compatible = "qcom,tsens-apq8064";
> +		reg = <0x34000 0x1000>;
> +		qcom,qfprom = <&qfprom 0x18 0x10>, <&qfprom 0x28
> 0x10>;
> +		qcom,qfprom-names = "calib", "backup_calib";
> +	};
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index
> 012fb37..389ec3e 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -19,3 +19,10 @@ config QCOM_PM
>  	  QCOM Platform specific power driver to manage cores and L2 low
> power
>  	  modes. It interface with various system drivers to put the cores
in
>  	  low power modes.
> +
> +config QCOM_QFPROM
> +	tristate "QCOM QFPROM Interface"
> +        depends on ARCH_QCOM && OF
> +        help
> +          Say y here to enable QFPROM support. The QFPROM provides access
> +          functions for QFPROM data to rest of the drivers via syscon
wrappers.
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index
> 20b329f..f5aff0a 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>  obj-$(CONFIG_QCOM_PM)	+=	spm.o
> +obj-$(CONFIG_QCOM_QFPROM) +=	qfprom.o
>  CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
>  obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o diff --git
> a/drivers/soc/qcom/qfprom.c b/drivers/soc/qcom/qfprom.c new file mode
> 100644 index 0000000..d00ed25
> --- /dev/null
> +++ b/drivers/soc/qcom/qfprom.c
> @@ -0,0 +1,134 @@
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/soc/qcom/qfprom.h>
> +#include <linux/slab.h>
> +
> +#define QFPROM_MAX_ARGS	2
> +
> +static char *__qfprom_get_data(struct device *dev,
> +				bool devm, int idx, int *len)
> +{
> +	struct device_node *syscon_np, *np = dev->of_node;
> +	struct regmap *rm;
> +	struct of_phandle_args args;
> +	int rc, stride = 4;
> +	u32 offset, size;
> +	char *data;
> +
> +	if (!np)
> +		return ERR_PTR(-EINVAL);
> +
> +	rc = of_parse_phandle_with_fixed_args(np, "qcom,qfprom",
> +					      QFPROM_MAX_ARGS, idx, &args);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	syscon_np = args.np;
> +
> +	of_property_read_u32(syscon_np, "stride", &stride);
> +
> +	if (stride >= 4)
> +		stride = 4;
> +
> +	if (args.args_count < QFPROM_MAX_ARGS) {
> +		dev_err(dev, "Insufficient qfprom arguments %d\n",
> +			args.args_count);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	rm = syscon_node_to_regmap(syscon_np);
> +	if (IS_ERR(rm))
> +		return ERR_CAST(rm);
> +
> +	offset = args.args[0];
> +	size = args.args[1];
> +
> +	of_node_put(syscon_np);
> +
> +	if (devm)
> +		data = devm_kzalloc(dev, size, GFP_KERNEL | GFP_ATOMIC);
> +	else
> +		data = kzalloc(size, GFP_KERNEL | GFP_ATOMIC);
> +
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = regmap_bulk_read(rm, offset, data, size/stride);
> +	if (rc < 0) {
> +		if (devm)
> +			devm_kfree(dev, data);
> +		else
> +			kfree(data);
> +
> +		return ERR_PTR(rc);
> +	}
> +
> +	*len = size;
> +
> +	return data;
> +}
> +
> +static char *__qfprom_get_data_byname(struct device *dev,
> +				       bool devm, const char *name, int
*len) {
> +	int index = 0;
> +
> +	if (name)
> +		index = of_property_match_string(dev->of_node,
> +						 "qcom,qfprom-names",
> name);
> +
> +	return __qfprom_get_data(dev, devm, index, len); }
> +
> +char *devm_qfprom_get_data_byname(struct device *dev,
> +					  const char *name, int *len)
> +{
> +	return __qfprom_get_data_byname(dev, true, name, len); }
> +EXPORT_SYMBOL_GPL(devm_qfprom_get_data_byname);
> +
> +char *devm_qfprom_get_data(struct device *dev,
> +				   int index, int *len)
> +{
> +	return __qfprom_get_data(dev, true, index, len); }
> +EXPORT_SYMBOL_GPL(devm_qfprom_get_data);
> +
> +/**
> + * qfprom_get_data_byname(): Reads qfprom data by name
> + *
> + * @dev: device which is requesting qfprom data
> + * @index: name of qfprom resources specified "qcom,qfprom-names" DT
> property.
> + * @len: length of data read from qfprom.
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a data buffer. The buffer should be freed by the user once its
> + * finished working with it kfree.
> + **/
> +char *qfprom_get_data_byname(struct device *dev,
> +				     const char *name, int *len)
> +{
> +	return __qfprom_get_data_byname(dev, false, name, len); }
> +EXPORT_SYMBOL_GPL(qfprom_get_data_byname);
> +
> +/**
> + * qfprom_get_data(): Reads qfprom data from the index
> + *
> + * @dev: device which is requesting qfprom data
> + * @index: index into qfprom resources specified "qcom,qfprom" DT
> property.
> + * @len: length of data read from qfprom.
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a data buffer. The buffer should be freed by the user once its
> + * finished working with it kfree.
> + **/
> +char *qfprom_get_data(struct device *dev,
> +				   int index, int *len)
> +{
> +	return __qfprom_get_data(dev, false, index, len); }
> +EXPORT_SYMBOL_GPL(qfprom_get_data);
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm"
in
> the body of a message to majordomo@vger.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html

--Naren