[v3] soc: at91: Add Atmel SFR SN (Serial Number) support
diff mbox series

Message ID 20191003114606.7846-1-kamel.bouhara@bootlin.com
State New
Headers show
Series
  • [v3] soc: at91: Add Atmel SFR SN (Serial Number) support
Related show

Commit Message

Kamel Bouhara Oct. 3, 2019, 11:46 a.m. UTC
Add support to read SFR's read-only registers providing the SoC
Serial Numbers (SN0+SN1) to userspace.

~ #  hexdump -n 8  -e'"%d\n"' /sys/bus/nvmem/devices/atmel-sfr0/nvmem
959527243
371539274

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
Changes in v3:
 - Fixed typo: processor in Kconfig
 - Renamed private struct sfr_priv to atmel_sfr_priv
 - Dropped the drvdata structure as we have same size for both SN
   registers in SAMA5D2/4, just hardcoded it for now.
 - Cleaned up private struct from unused members
 - Fixed misusage of devm_kzalloc

 drivers/soc/atmel/Kconfig  | 11 +++++
 drivers/soc/atmel/Makefile |  1 +
 drivers/soc/atmel/sfr.c    | 93 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+)
 create mode 100644 drivers/soc/atmel/sfr.c

--
2.23.0

Comments

Tudor Ambarus Oct. 4, 2019, 7:29 a.m. UTC | #1
Hi, Kamel,

On 10/03/2019 02:46 PM, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Add support to read SFR's read-only registers providing the SoC
> Serial Numbers (SN0+SN1) to userspace.
> 
> ~ #  hexdump -n 8  -e'"%d\n"' /sys/bus/nvmem/devices/atmel-sfr0/nvmem

nitpick probably it's better to print the SN in hex, so %08x instead of %d

> 959527243
> 371539274
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>

Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Is the SN unique for each device or it is unique per SoC? For example, for
sama5d2, I get the following (in hex):

root@sama5d2-xplained-sd:~#  hexdump -n 8  -e '"%08x\n"'
/sys/bus/nvmem/devices/atmel-sfr0/nvmem
4643524b
02010657

If this is unique for each device, then maybe is it worth to add the serial
number in the entropy pool (with add_device_randomness()?)

What was your use case, why do you need to print the SN?

And you have a blank line at EOF, but probably the maintainers can remove it, if
the patch will remain as is.

Cheers,
ta

> ---
> Changes in v3:
>  - Fixed typo: processor in Kconfig
>  - Renamed private struct sfr_priv to atmel_sfr_priv
>  - Dropped the drvdata structure as we have same size for both SN
>    registers in SAMA5D2/4, just hardcoded it for now.
>  - Cleaned up private struct from unused members
>  - Fixed misusage of devm_kzalloc
> 
>  drivers/soc/atmel/Kconfig  | 11 +++++
>  drivers/soc/atmel/Makefile |  1 +
>  drivers/soc/atmel/sfr.c    | 93 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 105 insertions(+)
>  create mode 100644 drivers/soc/atmel/sfr.c
> 
> diff --git a/drivers/soc/atmel/Kconfig b/drivers/soc/atmel/Kconfig
> index 05528139b023..50caf6db9c0e 100644
> --- a/drivers/soc/atmel/Kconfig
> +++ b/drivers/soc/atmel/Kconfig
> @@ -5,3 +5,14 @@ config AT91_SOC_ID
>  	default ARCH_AT91
>  	help
>  	  Include support for the SoC bus on the Atmel ARM SoCs.
> +
> +config AT91_SOC_SFR
> +	tristate "Special Function Registers support"
> +	depends on ARCH_AT91 || COMPILE_TEST
> +	help
> +	  This is a driver for the Special Function Registers available on
> +	  Atmel SAMA5Dx SoCs, providing access to specific aspects of the
> +	  integrated memory, bridge implementations, processor etc.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called sfr.
> diff --git a/drivers/soc/atmel/Makefile b/drivers/soc/atmel/Makefile
> index 7ca355d10553..d849a897cd77 100644
> --- a/drivers/soc/atmel/Makefile
> +++ b/drivers/soc/atmel/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_AT91_SOC_ID) += soc.o
> +obj-$(CONFIG_AT91_SOC_SFR) += sfr.o
> diff --git a/drivers/soc/atmel/sfr.c b/drivers/soc/atmel/sfr.c
> new file mode 100644
> index 000000000000..e92050b4c09e
> --- /dev/null
> +++ b/drivers/soc/atmel/sfr.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sfr.c - driver for special function registers
> + *
> + * Copyright (C) 2019 Bootlin.
> + *
> + */
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define SFR_SN0		0x4c
> +#define SFR_SN_SIZE	8
> +
> +struct atmel_sfr_priv {
> +	struct regmap			*regmap;
> +};
> +
> +static int atmel_sfr_read(void *context, unsigned int offset,
> +			  void *buf, size_t bytes)
> +{
> +	struct atmel_sfr_priv *priv = context;
> +
> +	return regmap_bulk_read(priv->regmap, SFR_SN0 + offset,
> +				buf, bytes / 4);
> +}
> +
> +static struct nvmem_config atmel_sfr_nvmem_config = {
> +	.name = "atmel-sfr",
> +	.read_only = true,
> +	.word_size = 4,
> +	.stride = 4,
> +	.size = SFR_SN_SIZE,
> +	.reg_read = atmel_sfr_read,
> +};
> +
> +static int atmel_sfr_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct nvmem_device *nvmem;
> +	struct atmel_sfr_priv *priv;
> +
> +	priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = syscon_node_to_regmap(np);
> +	if (IS_ERR(priv->regmap)) {
> +		dev_err(dev, "cannot get parent's regmap\n");
> +		return PTR_ERR(priv->regmap);
> +	}
> +
> +	atmel_sfr_nvmem_config.dev = dev;
> +	atmel_sfr_nvmem_config.priv = priv;
> +
> +	nvmem = devm_nvmem_register(dev, &atmel_sfr_nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(dev, "error registering nvmem config\n");
> +		return PTR_ERR(nvmem);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id atmel_sfr_dt_ids[] = {
> +	{
> +		.compatible = "atmel,sama5d2-sfr",
> +	}, {
> +		.compatible = "atmel,sama5d4-sfr",
> +	}, {
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, atmel_sfr_dt_ids);
> +
> +static struct platform_driver atmel_sfr_driver = {
> +	.probe = atmel_sfr_probe,
> +	.driver = {
> +		.name = "atmel-sfr",
> +		.of_match_table = atmel_sfr_dt_ids,
> +	},
> +};
> +module_platform_driver(atmel_sfr_driver);
> +
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("Atmel SFR SN driver for SAMA5D2/4 SoC family");
> +MODULE_LICENSE("GPL v2");
> +
> --
> 2.23.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
>
Kamel Bouhara Oct. 4, 2019, 9:31 a.m. UTC | #2
Hi Tudor,

On 10/4/19 9:29 AM, Tudor.Ambarus@microchip.com wrote:
> Hi, Kamel,
>
> On 10/03/2019 02:46 PM, Kamel Bouhara wrote:
>> External E-Mail
>>
>>
>> Add support to read SFR's read-only registers providing the SoC
>> Serial Numbers (SN0+SN1) to userspace.
>>
>> ~ #  hexdump -n 8  -e'"%d\n"' /sys/bus/nvmem/devices/atmel-sfr0/nvmem
> nitpick probably it's better to print the SN in hex, so %08x instead of %d
Of course, it is just an example of user space command, this is 
completely up to user to choose the relevant format.
>> 959527243
>> 371539274
>>
>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> Is the SN unique for each device or it is unique per SoC? For example, for
> sama5d2, I get the following (in hex):
>
> root@sama5d2-xplained-sd:~#  hexdump -n 8  -e '"%08x\n"'
> /sys/bus/nvmem/devices/atmel-sfr0/nvmem
> 4643524b
> 02010657
>
> If this is unique for each device, then maybe is it worth to add the serial
> number in the entropy pool (with add_device_randomness()?)

Yes it is. Maybe it is not suited to throw a non random 8 bytes (only) 
number to the entropy pool ?

By the way, some SoC drivers do it already.

Shall I propose a v4 for this ?

>
> What was your use case, why do you need to print the SN?	
It can be used in a factory process to associate a mac address for 
instance... but I'm sure there is lot of use case using a unique identifier.
>
> And you have a blank line at EOF, but probably the maintainers can remove it, if
> the patch will remain as is.

OK, thanks for you review, If so, it will be fixed in v4.

Kamel
Tudor Ambarus Oct. 4, 2019, 10:41 a.m. UTC | #3
On 10/04/2019 12:31 PM, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Hi Tudor,

Hi, Kamel,

> 
> On 10/4/19 9:29 AM, Tudor.Ambarus@microchip.com wrote:
>> Hi, Kamel,
>>
>> On 10/03/2019 02:46 PM, Kamel Bouhara wrote:
>>> External E-Mail
>>>
>>>
>>> Add support to read SFR's read-only registers providing the SoC
>>> Serial Numbers (SN0+SN1) to userspace.
>>>
>>> ~ #  hexdump -n 8  -e'"%d\n"' /sys/bus/nvmem/devices/atmel-sfr0/nvmem
>> nitpick probably it's better to print the SN in hex, so %08x instead of %d
> Of course, it is just an example of user space command, this is completely up to user to choose the relevant format.
>>> 959527243
>>> 371539274
>>>
>>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
>> Tested-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Is the SN unique for each device or it is unique per SoC? For example, for
>> sama5d2, I get the following (in hex):
>>
>> root@sama5d2-xplained-sd:~#  hexdump -n 8  -e '"%08x\n"'
>> /sys/bus/nvmem/devices/atmel-sfr0/nvmem
>> 4643524b
>> 02010657
>>
>> If this is unique for each device, then maybe is it worth to add the serial
>> number in the entropy pool (with add_device_randomness()?)
> 
> Yes it is. Maybe it is not suited to throw a non random 8 bytes (only) number to the entropy pool ?
> 
> By the way, some SoC drivers do it already.
> 
> Shall I propose a v4 for this ?

I would. This is more like an improvement, it's not mandatory, it's your call.
The code looks good. I have some doubts on the use cases though, but the at91
maintainers will probably decide on this. Let me know if you are sending v4, so
I can know where to add my R-b tag :)
> 
>>
>> What was your use case, why do you need to print the SN?   
> It can be used in a factory process to associate a mac address for instance... but I'm sure there is lot of use case using a unique identifier.

I don't know the strategy in regards to serial numbers. I guess there are some
IPs that have fields for serial numbers in their registers, should we expose
them all?

Cheers,
ta

>>
>> And you have a blank line at EOF, but probably the maintainers can remove it, if
>> the patch will remain as is.
> 
> OK, thanks for you review, If so, it will be fixed in v4.
> 
> Kamel
>

Patch
diff mbox series

diff --git a/drivers/soc/atmel/Kconfig b/drivers/soc/atmel/Kconfig
index 05528139b023..50caf6db9c0e 100644
--- a/drivers/soc/atmel/Kconfig
+++ b/drivers/soc/atmel/Kconfig
@@ -5,3 +5,14 @@  config AT91_SOC_ID
 	default ARCH_AT91
 	help
 	  Include support for the SoC bus on the Atmel ARM SoCs.
+
+config AT91_SOC_SFR
+	tristate "Special Function Registers support"
+	depends on ARCH_AT91 || COMPILE_TEST
+	help
+	  This is a driver for the Special Function Registers available on
+	  Atmel SAMA5Dx SoCs, providing access to specific aspects of the
+	  integrated memory, bridge implementations, processor etc.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called sfr.
diff --git a/drivers/soc/atmel/Makefile b/drivers/soc/atmel/Makefile
index 7ca355d10553..d849a897cd77 100644
--- a/drivers/soc/atmel/Makefile
+++ b/drivers/soc/atmel/Makefile
@@ -1,2 +1,3 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_AT91_SOC_ID) += soc.o
+obj-$(CONFIG_AT91_SOC_SFR) += sfr.o
diff --git a/drivers/soc/atmel/sfr.c b/drivers/soc/atmel/sfr.c
new file mode 100644
index 000000000000..e92050b4c09e
--- /dev/null
+++ b/drivers/soc/atmel/sfr.c
@@ -0,0 +1,93 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sfr.c - driver for special function registers
+ *
+ * Copyright (C) 2019 Bootlin.
+ *
+ */
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define SFR_SN0		0x4c
+#define SFR_SN_SIZE	8
+
+struct atmel_sfr_priv {
+	struct regmap			*regmap;
+};
+
+static int atmel_sfr_read(void *context, unsigned int offset,
+			  void *buf, size_t bytes)
+{
+	struct atmel_sfr_priv *priv = context;
+
+	return regmap_bulk_read(priv->regmap, SFR_SN0 + offset,
+				buf, bytes / 4);
+}
+
+static struct nvmem_config atmel_sfr_nvmem_config = {
+	.name = "atmel-sfr",
+	.read_only = true,
+	.word_size = 4,
+	.stride = 4,
+	.size = SFR_SN_SIZE,
+	.reg_read = atmel_sfr_read,
+};
+
+static int atmel_sfr_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct nvmem_device *nvmem;
+	struct atmel_sfr_priv *priv;
+
+	priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = syscon_node_to_regmap(np);
+	if (IS_ERR(priv->regmap)) {
+		dev_err(dev, "cannot get parent's regmap\n");
+		return PTR_ERR(priv->regmap);
+	}
+
+	atmel_sfr_nvmem_config.dev = dev;
+	atmel_sfr_nvmem_config.priv = priv;
+
+	nvmem = devm_nvmem_register(dev, &atmel_sfr_nvmem_config);
+	if (IS_ERR(nvmem)) {
+		dev_err(dev, "error registering nvmem config\n");
+		return PTR_ERR(nvmem);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id atmel_sfr_dt_ids[] = {
+	{
+		.compatible = "atmel,sama5d2-sfr",
+	}, {
+		.compatible = "atmel,sama5d4-sfr",
+	}, {
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, atmel_sfr_dt_ids);
+
+static struct platform_driver atmel_sfr_driver = {
+	.probe = atmel_sfr_probe,
+	.driver = {
+		.name = "atmel-sfr",
+		.of_match_table = atmel_sfr_dt_ids,
+	},
+};
+module_platform_driver(atmel_sfr_driver);
+
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_DESCRIPTION("Atmel SFR SN driver for SAMA5D2/4 SoC family");
+MODULE_LICENSE("GPL v2");
+