[PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Sam Protsenko]

Old Exynos SoCs have both Product ID and Revision ID in one single
register, while new SoCs tend to have two separate registers for those
IDs. Implement handling of both cases by passing Revision ID register
offsets in driver data.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
 include/linux/soc/samsung/exynos-chipid.h |  6 +-
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
index 5c1d0f97f766..7837331fb753 100644
--- a/drivers/soc/samsung/exynos-chipid.c
+++ b/drivers/soc/samsung/exynos-chipid.c
@@ -16,6 +16,7 @@
 #include <linux/errno.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -24,6 +25,17 @@
 
 #include "exynos-asv.h"
 
+struct exynos_chipid_variant {
+	unsigned int rev_reg;		/* revision register offset */
+	unsigned int main_rev_shift;	/* main revision offset in rev_reg */
+	unsigned int sub_rev_shift;	/* sub revision offset in rev_reg */
+};
+
+struct exynos_chipid_info {
+	u32 product_id;
+	u32 revision;
+};
+
 static const struct exynos_soc_id {
 	const char *name;
 	unsigned int id;
@@ -49,31 +61,55 @@ static const char *product_id_to_soc_id(unsigned int product_id)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(soc_ids); i++)
-		if ((product_id & EXYNOS_MASK) == soc_ids[i].id)
+		if (product_id == soc_ids[i].id)
 			return soc_ids[i].name;
 	return NULL;
 }
 
+static int exynos_chipid_get_chipid_info(struct regmap *regmap,
+		const struct exynos_chipid_variant *data,
+		struct exynos_chipid_info *soc_info)
+{
+	int ret;
+	unsigned int val, main_rev, sub_rev;
+
+	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
+	if (ret < 0)
+		return ret;
+	soc_info->product_id = val & EXYNOS_MASK;
+
+	ret = regmap_read(regmap, data->rev_reg, &val);
+	if (ret < 0)
+		return ret;
+	main_rev = (val >> data->main_rev_shift) & EXYNOS_REV_PART_MASK;
+	sub_rev = (val >> data->sub_rev_shift) & EXYNOS_REV_PART_MASK;
+	soc_info->revision = (main_rev << EXYNOS_REV_PART_SHIFT) | sub_rev;
+
+	return 0;
+}
+
 static int exynos_chipid_probe(struct platform_device *pdev)
 {
+	const struct exynos_chipid_variant *drv_data;
+	struct exynos_chipid_info soc_info;
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
 	struct device_node *root;
 	struct regmap *regmap;
-	u32 product_id;
-	u32 revision;
 	int ret;
 
+	drv_data = of_device_get_match_data(&pdev->dev);
+	if (!drv_data)
+		return -EINVAL;
+
 	regmap = device_node_to_regmap(pdev->dev.of_node);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
-	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id);
+	ret = exynos_chipid_get_chipid_info(regmap, drv_data, &soc_info);
 	if (ret < 0)
 		return ret;
 
-	revision = product_id & EXYNOS_REV_MASK;
-
 	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
 				    GFP_KERNEL);
 	if (!soc_dev_attr)
@@ -86,8 +122,8 @@ static int exynos_chipid_probe(struct platform_device *pdev)
 	of_node_put(root);
 
 	soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-						"%x", revision);
-	soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
+						"%x", soc_info.revision);
+	soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
 	if (!soc_dev_attr->soc_id) {
 		pr_err("Unknown SoC\n");
 		return -ENODEV;
@@ -106,7 +142,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
 
 	dev_info(soc_device_to_device(soc_dev),
 		 "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
-		 soc_dev_attr->soc_id, product_id, revision);
+		 soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
 
 	return 0;
 
@@ -125,9 +161,18 @@ static int exynos_chipid_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
+	.rev_reg	= 0x0,
+	.main_rev_shift	= 0,
+	.sub_rev_shift	= 4,
+};
+
 static const struct of_device_id exynos_chipid_of_device_ids[] = {
-	{ .compatible = "samsung,exynos4210-chipid" },
-	{}
+	{
+		.compatible	= "samsung,exynos4210-chipid",
+		.data		= &exynos4210_chipid_drv_data,
+	},
+	{ }
 };
 
 static struct platform_driver exynos_chipid_driver = {
diff --git a/include/linux/soc/samsung/exynos-chipid.h b/include/linux/soc/samsung/exynos-chipid.h
index 8bca6763f99c..62f0e2531068 100644
--- a/include/linux/soc/samsung/exynos-chipid.h
+++ b/include/linux/soc/samsung/exynos-chipid.h
@@ -9,10 +9,8 @@
 #define __LINUX_SOC_EXYNOS_CHIPID_H
 
 #define EXYNOS_CHIPID_REG_PRO_ID	0x00
-#define EXYNOS_SUBREV_MASK		(0xf << 4)
-#define EXYNOS_MAINREV_MASK		(0xf << 0)
-#define EXYNOS_REV_MASK			(EXYNOS_SUBREV_MASK | \
-					 EXYNOS_MAINREV_MASK)
+#define EXYNOS_REV_PART_MASK		0xf
+#define EXYNOS_REV_PART_SHIFT		4
 #define EXYNOS_MASK			0xfffff000
 
 #define EXYNOS_CHIPID_REG_PKG_ID	0x04
-- 
2.30.2

[PATCH v2 2/3] dt-bindings: samsung: exynos-chipid: Document Exynos850 compatible

[Sam Protsenko]

Add compatible string for Exynos850 chip-id. While at it, use enum
instead of items/const, to reduce further cluttering of "compatible"
list.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 .../devicetree/bindings/arm/samsung/exynos-chipid.yaml       | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-chipid.yaml b/Documentation/devicetree/bindings/arm/samsung/exynos-chipid.yaml
index f99c0c6df21b..bfc352a2fdd6 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-chipid.yaml
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-chipid.yaml
@@ -11,8 +11,9 @@ maintainers:
 
 properties:
   compatible:
-    items:
-      - const: samsung,exynos4210-chipid
+    enum:
+      - samsung,exynos4210-chipid
+      - samsung,exynos850-chipid
 
   reg:
     maxItems: 1
-- 
2.30.2

[PATCH v2 3/3] soc: samsung: exynos-chipid: Add Exynos850 support

[Sam Protsenko]

Add chip-id support for Exynos850 SoC. Despite its "E3830" ID, the
actual SoC name is Exynos850 (Exynos3830 name is internal and outdated).

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/soc/samsung/exynos-chipid.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
index 7837331fb753..fdf806e4b6ed 100644
--- a/drivers/soc/samsung/exynos-chipid.c
+++ b/drivers/soc/samsung/exynos-chipid.c
@@ -54,6 +54,7 @@ static const struct exynos_soc_id {
 	{ "EXYNOS5440", 0xE5440000 },
 	{ "EXYNOS5800", 0xE5422000 },
 	{ "EXYNOS7420", 0xE7420000 },
+	{ "EXYNOS850", 0xE3830000 },
 };
 
 static const char *product_id_to_soc_id(unsigned int product_id)
@@ -167,10 +168,19 @@ static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
 	.sub_rev_shift	= 4,
 };
 
+static const struct exynos_chipid_variant exynos850_chipid_drv_data = {
+	.rev_reg	= 0x10,
+	.main_rev_shift	= 20,
+	.sub_rev_shift	= 16,
+};
+
 static const struct of_device_id exynos_chipid_of_device_ids[] = {
 	{
 		.compatible	= "samsung,exynos4210-chipid",
 		.data		= &exynos4210_chipid_drv_data,
+	}, {
+		.compatible	= "samsung,exynos850-chipid",
+		.data		= &exynos850_chipid_drv_data,
 	},
 	{ }
 };
-- 
2.30.2

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Krzysztof Kozlowski]

On 13/10/2021 22:21, Sam Protsenko wrote:
> Old Exynos SoCs have both Product ID and Revision ID in one single
> register, while new SoCs tend to have two separate registers for those
> IDs. Implement handling of both cases by passing Revision ID register
> offsets in driver data.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
>  include/linux/soc/samsung/exynos-chipid.h |  6 +-
>  2 files changed, 58 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> index 5c1d0f97f766..7837331fb753 100644
> --- a/drivers/soc/samsung/exynos-chipid.c
> +++ b/drivers/soc/samsung/exynos-chipid.c
> @@ -16,6 +16,7 @@
>  #include <linux/errno.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> @@ -24,6 +25,17 @@

Include a changelog please. Your patch does not apply and there is no
information on tree which it was based on.


Best regards,
Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Krzysztof Kozlowski]

On 13/10/2021 22:21, Sam Protsenko wrote:
> Old Exynos SoCs have both Product ID and Revision ID in one single
> register, while new SoCs tend to have two separate registers for those
> IDs. Implement handling of both cases by passing Revision ID register
> offsets in driver data.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
>  include/linux/soc/samsung/exynos-chipid.h |  6 +-
>  2 files changed, 58 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> index 5c1d0f97f766..7837331fb753 100644
> --- a/drivers/soc/samsung/exynos-chipid.c
> +++ b/drivers/soc/samsung/exynos-chipid.c
> @@ -16,6 +16,7 @@
>  #include <linux/errno.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> @@ -24,6 +25,17 @@
>  
>  #include "exynos-asv.h"
>  
> +struct exynos_chipid_variant {
> +	unsigned int rev_reg;		/* revision register offset */
> +	unsigned int main_rev_shift;	/* main revision offset in rev_reg */
> +	unsigned int sub_rev_shift;	/* sub revision offset in rev_reg */
> +};
> +
> +struct exynos_chipid_info {
> +	u32 product_id;
> +	u32 revision;
> +};
> +
>  static const struct exynos_soc_id {
>  	const char *name;
>  	unsigned int id;
> @@ -49,31 +61,55 @@ static const char *product_id_to_soc_id(unsigned int product_id)
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(soc_ids); i++)
> -		if ((product_id & EXYNOS_MASK) == soc_ids[i].id)
> +		if (product_id == soc_ids[i].id)
>  			return soc_ids[i].name;
>  	return NULL;
>  }
>  
> +static int exynos_chipid_get_chipid_info(struct regmap *regmap,
> +		const struct exynos_chipid_variant *data,
> +		struct exynos_chipid_info *soc_info)
> +{
> +	int ret;
> +	unsigned int val, main_rev, sub_rev;
> +
> +	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
> +	if (ret < 0)
> +		return ret;
> +	soc_info->product_id = val & EXYNOS_MASK;
> +
> +	ret = regmap_read(regmap, data->rev_reg, &val);

Isn't this the same register as EXYNOS_CHIPID_REG_PRO_ID?

> +	if (ret < 0)
> +		return ret;
> +	main_rev = (val >> data->main_rev_shift) & EXYNOS_REV_PART_MASK;
> +	sub_rev = (val >> data->sub_rev_shift) & EXYNOS_REV_PART_MASK;
> +	soc_info->revision = (main_rev << EXYNOS_REV_PART_SHIFT) | sub_rev;
> +
> +	return 0;
> +}
> +
>  static int exynos_chipid_probe(struct platform_device *pdev)
>  {
> +	const struct exynos_chipid_variant *drv_data;
> +	struct exynos_chipid_info soc_info;
>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
>  	struct device_node *root;
>  	struct regmap *regmap;
> -	u32 product_id;
> -	u32 revision;
>  	int ret;
>  
> +	drv_data = of_device_get_match_data(&pdev->dev);
> +	if (!drv_data)
> +		return -EINVAL;
> +
>  	regmap = device_node_to_regmap(pdev->dev.of_node);
>  	if (IS_ERR(regmap))
>  		return PTR_ERR(regmap);
>  
> -	ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id);
> +	ret = exynos_chipid_get_chipid_info(regmap, drv_data, &soc_info);
>  	if (ret < 0)
>  		return ret;
>  
> -	revision = product_id & EXYNOS_REV_MASK;
> -
>  	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>  				    GFP_KERNEL);
>  	if (!soc_dev_attr)
> @@ -86,8 +122,8 @@ static int exynos_chipid_probe(struct platform_device *pdev)
>  	of_node_put(root);
>  
>  	soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> -						"%x", revision);
> -	soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
> +						"%x", soc_info.revision);
> +	soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
>  	if (!soc_dev_attr->soc_id) {
>  		pr_err("Unknown SoC\n");
>  		return -ENODEV;
> @@ -106,7 +142,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
>  
>  	dev_info(soc_device_to_device(soc_dev),
>  		 "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
> -		 soc_dev_attr->soc_id, product_id, revision);
> +		 soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
>  
>  	return 0;
>  
> @@ -125,9 +161,18 @@ static int exynos_chipid_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
> +	.rev_reg	= 0x0,
> +	.main_rev_shift	= 0,
> +	.sub_rev_shift	= 4,

The code does not look correct here. Subrev is at 0:3 bits, mainrev is
at 4:7.

Did you test it that it produces same result? Looks not - I gave it a
try and got wrong revision.


Best regards,
Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Sam Protsenko]

On Thu, 14 Oct 2021 at 09:41, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 13/10/2021 22:21, Sam Protsenko wrote:
> > Old Exynos SoCs have both Product ID and Revision ID in one single
> > register, while new SoCs tend to have two separate registers for those
> > IDs. Implement handling of both cases by passing Revision ID register
> > offsets in driver data.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
> >  include/linux/soc/samsung/exynos-chipid.h |  6 +-
> >  2 files changed, 58 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> > index 5c1d0f97f766..7837331fb753 100644
> > --- a/drivers/soc/samsung/exynos-chipid.c
> > +++ b/drivers/soc/samsung/exynos-chipid.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/slab.h>
> > @@ -24,6 +25,17 @@
>
> Include a changelog please. Your patch does not apply and there is no
> information on tree which it was based on.
>

Sorry, my bad. Will do in v3. As for the tree: it's based on the
latest mainline/master. I'll double check if patches apply correctly
to that before sending v3. Please let me know if you want me to rebase
this series on top of some other tree.

>
> Best regards,
> Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Sam Protsenko]

On Thu, 14 Oct 2021 at 10:11, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 13/10/2021 22:21, Sam Protsenko wrote:
> > Old Exynos SoCs have both Product ID and Revision ID in one single
> > register, while new SoCs tend to have two separate registers for those
> > IDs. Implement handling of both cases by passing Revision ID register
> > offsets in driver data.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
> >  include/linux/soc/samsung/exynos-chipid.h |  6 +-
> >  2 files changed, 58 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> > index 5c1d0f97f766..7837331fb753 100644
> > --- a/drivers/soc/samsung/exynos-chipid.c
> > +++ b/drivers/soc/samsung/exynos-chipid.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/slab.h>
> > @@ -24,6 +25,17 @@
> >
> >  #include "exynos-asv.h"
> >
> > +struct exynos_chipid_variant {
> > +     unsigned int rev_reg;           /* revision register offset */
> > +     unsigned int main_rev_shift;    /* main revision offset in rev_reg */
> > +     unsigned int sub_rev_shift;     /* sub revision offset in rev_reg */
> > +};
> > +
> > +struct exynos_chipid_info {
> > +     u32 product_id;
> > +     u32 revision;
> > +};
> > +
> >  static const struct exynos_soc_id {
> >       const char *name;
> >       unsigned int id;
> > @@ -49,31 +61,55 @@ static const char *product_id_to_soc_id(unsigned int product_id)
> >       int i;
> >
> >       for (i = 0; i < ARRAY_SIZE(soc_ids); i++)
> > -             if ((product_id & EXYNOS_MASK) == soc_ids[i].id)
> > +             if (product_id == soc_ids[i].id)
> >                       return soc_ids[i].name;
> >       return NULL;
> >  }
> >
> > +static int exynos_chipid_get_chipid_info(struct regmap *regmap,
> > +             const struct exynos_chipid_variant *data,
> > +             struct exynos_chipid_info *soc_info)
> > +{
> > +     int ret;
> > +     unsigned int val, main_rev, sub_rev;
> > +
> > +     ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
> > +     if (ret < 0)
> > +             return ret;
> > +     soc_info->product_id = val & EXYNOS_MASK;
> > +
> > +     ret = regmap_read(regmap, data->rev_reg, &val);
>
> Isn't this the same register as EXYNOS_CHIPID_REG_PRO_ID?
>

It is for Exynos4210, but it's not for Exynos850 (see PATCH 3/3), as
it's described in the commit message. I tried to keep this code
unified for all SoCs.

> > +     if (ret < 0)
> > +             return ret;
> > +     main_rev = (val >> data->main_rev_shift) & EXYNOS_REV_PART_MASK;
> > +     sub_rev = (val >> data->sub_rev_shift) & EXYNOS_REV_PART_MASK;
> > +     soc_info->revision = (main_rev << EXYNOS_REV_PART_SHIFT) | sub_rev;
> > +
> > +     return 0;
> > +}
> > +
> >  static int exynos_chipid_probe(struct platform_device *pdev)
> >  {
> > +     const struct exynos_chipid_variant *drv_data;
> > +     struct exynos_chipid_info soc_info;
> >       struct soc_device_attribute *soc_dev_attr;
> >       struct soc_device *soc_dev;
> >       struct device_node *root;
> >       struct regmap *regmap;
> > -     u32 product_id;
> > -     u32 revision;
> >       int ret;
> >
> > +     drv_data = of_device_get_match_data(&pdev->dev);
> > +     if (!drv_data)
> > +             return -EINVAL;
> > +
> >       regmap = device_node_to_regmap(pdev->dev.of_node);
> >       if (IS_ERR(regmap))
> >               return PTR_ERR(regmap);
> >
> > -     ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id);
> > +     ret = exynos_chipid_get_chipid_info(regmap, drv_data, &soc_info);
> >       if (ret < 0)
> >               return ret;
> >
> > -     revision = product_id & EXYNOS_REV_MASK;
> > -
> >       soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> >                                   GFP_KERNEL);
> >       if (!soc_dev_attr)
> > @@ -86,8 +122,8 @@ static int exynos_chipid_probe(struct platform_device *pdev)
> >       of_node_put(root);
> >
> >       soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > -                                             "%x", revision);
> > -     soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
> > +                                             "%x", soc_info.revision);
> > +     soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
> >       if (!soc_dev_attr->soc_id) {
> >               pr_err("Unknown SoC\n");
> >               return -ENODEV;
> > @@ -106,7 +142,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
> >
> >       dev_info(soc_device_to_device(soc_dev),
> >                "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
> > -              soc_dev_attr->soc_id, product_id, revision);
> > +              soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
> >
> >       return 0;
> >
> > @@ -125,9 +161,18 @@ static int exynos_chipid_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
> > +     .rev_reg        = 0x0,
> > +     .main_rev_shift = 0,
> > +     .sub_rev_shift  = 4,
>
> The code does not look correct here. Subrev is at 0:3 bits, mainrev is
> at 4:7.
>

Right. I was confused by those existing macros:

    #define EXYNOS_SUBREV_MASK        (0xf << 4)
    #define EXYNOS_MAINREV_MASK        (0xf << 0)

Those were probably wrong the whole time? Anyway, now I've found
Exynos4412 User Manual and checked it there -- you are right about
offsets. Will fix in v3.

> Did you test it that it produces same result? Looks not - I gave it a
> try and got wrong revision.
>

I only have Exynos850 based board, and that has 0x0 in Revision ID
register. But for v3 I'll try to emulate register value in the code
and make sure that the read value does not change with patch applied.

>
> Best regards,
> Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Krzysztof Kozlowski]

On 14/10/2021 13:25, Sam Protsenko wrote:
> On Thu, 14 Oct 2021 at 09:41, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 13/10/2021 22:21, Sam Protsenko wrote:
>>> Old Exynos SoCs have both Product ID and Revision ID in one single
>>> register, while new SoCs tend to have two separate registers for those
>>> IDs. Implement handling of both cases by passing Revision ID register
>>> offsets in driver data.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>>  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
>>>  include/linux/soc/samsung/exynos-chipid.h |  6 +-
>>>  2 files changed, 58 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
>>> index 5c1d0f97f766..7837331fb753 100644
>>> --- a/drivers/soc/samsung/exynos-chipid.c
>>> +++ b/drivers/soc/samsung/exynos-chipid.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/errno.h>
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/of.h>
>>> +#include <linux/of_device.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/slab.h>
>>> @@ -24,6 +25,17 @@
>>
>> Include a changelog please. Your patch does not apply and there is no
>> information on tree which it was based on.
>>
> 
> Sorry, my bad. Will do in v3. As for the tree: it's based on the
> latest mainline/master. I'll double check if patches apply correctly
> to that before sending v3. Please let me know if you want me to rebase
> this series on top of some other tree.

Mainline/master won't work in most of the cases. You need to rebase your
work on maintainer's tree. This is sometimes tricky, so usually it's
enough to base on linux-next.

In this case, either use linux-next or for-next branch of my tree:
https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git/

Best regards,
Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Krzysztof Kozlowski]

On 14/10/2021 13:34, Sam Protsenko wrote:
> On Thu, 14 Oct 2021 at 10:11, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 13/10/2021 22:21, Sam Protsenko wrote:
>>> Old Exynos SoCs have both Product ID and Revision ID in one single
>>> register, while new SoCs tend to have two separate registers for those
>>> IDs. Implement handling of both cases by passing Revision ID register
>>> offsets in driver data.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>>  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
>>>  include/linux/soc/samsung/exynos-chipid.h |  6 +-
>>>  2 files changed, 58 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
>>> index 5c1d0f97f766..7837331fb753 100644
>>> --- a/drivers/soc/samsung/exynos-chipid.c
>>> +++ b/drivers/soc/samsung/exynos-chipid.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/errno.h>
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/of.h>
>>> +#include <linux/of_device.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/slab.h>
>>> @@ -24,6 +25,17 @@
>>>
>>>  #include "exynos-asv.h"
>>>
>>> +struct exynos_chipid_variant {
>>> +     unsigned int rev_reg;           /* revision register offset */
>>> +     unsigned int main_rev_shift;    /* main revision offset in rev_reg */
>>> +     unsigned int sub_rev_shift;     /* sub revision offset in rev_reg */
>>> +};
>>> +
>>> +struct exynos_chipid_info {
>>> +     u32 product_id;
>>> +     u32 revision;
>>> +};
>>> +
>>>  static const struct exynos_soc_id {
>>>       const char *name;
>>>       unsigned int id;
>>> @@ -49,31 +61,55 @@ static const char *product_id_to_soc_id(unsigned int product_id)
>>>       int i;
>>>
>>>       for (i = 0; i < ARRAY_SIZE(soc_ids); i++)
>>> -             if ((product_id & EXYNOS_MASK) == soc_ids[i].id)
>>> +             if (product_id == soc_ids[i].id)
>>>                       return soc_ids[i].name;
>>>       return NULL;
>>>  }
>>>
>>> +static int exynos_chipid_get_chipid_info(struct regmap *regmap,
>>> +             const struct exynos_chipid_variant *data,
>>> +             struct exynos_chipid_info *soc_info)
>>> +{
>>> +     int ret;
>>> +     unsigned int val, main_rev, sub_rev;
>>> +
>>> +     ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +     soc_info->product_id = val & EXYNOS_MASK;
>>> +
>>> +     ret = regmap_read(regmap, data->rev_reg, &val);
>>
>> Isn't this the same register as EXYNOS_CHIPID_REG_PRO_ID?
>>
> 
> It is for Exynos4210, but it's not for Exynos850 (see PATCH 3/3), as
> it's described in the commit message. I tried to keep this code
> unified for all SoCs.

Yeah, but for Exynos4210 you read the same register twice. It's
confusing. Read only once. You could compare the offsets and skip second
read.

> 
>>> +     if (ret < 0)
>>> +             return ret;
>>> +     main_rev = (val >> data->main_rev_shift) & EXYNOS_REV_PART_MASK;
>>> +     sub_rev = (val >> data->sub_rev_shift) & EXYNOS_REV_PART_MASK;
>>> +     soc_info->revision = (main_rev << EXYNOS_REV_PART_SHIFT) | sub_rev;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>  static int exynos_chipid_probe(struct platform_device *pdev)
>>>  {
>>> +     const struct exynos_chipid_variant *drv_data;
>>> +     struct exynos_chipid_info soc_info;
>>>       struct soc_device_attribute *soc_dev_attr;
>>>       struct soc_device *soc_dev;
>>>       struct device_node *root;
>>>       struct regmap *regmap;
>>> -     u32 product_id;
>>> -     u32 revision;
>>>       int ret;
>>>
>>> +     drv_data = of_device_get_match_data(&pdev->dev);
>>> +     if (!drv_data)
>>> +             return -EINVAL;
>>> +
>>>       regmap = device_node_to_regmap(pdev->dev.of_node);
>>>       if (IS_ERR(regmap))
>>>               return PTR_ERR(regmap);
>>>
>>> -     ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id);
>>> +     ret = exynos_chipid_get_chipid_info(regmap, drv_data, &soc_info);
>>>       if (ret < 0)
>>>               return ret;
>>>
>>> -     revision = product_id & EXYNOS_REV_MASK;
>>> -
>>>       soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>>>                                   GFP_KERNEL);
>>>       if (!soc_dev_attr)
>>> @@ -86,8 +122,8 @@ static int exynos_chipid_probe(struct platform_device *pdev)
>>>       of_node_put(root);
>>>
>>>       soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>> -                                             "%x", revision);
>>> -     soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
>>> +                                             "%x", soc_info.revision);
>>> +     soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
>>>       if (!soc_dev_attr->soc_id) {
>>>               pr_err("Unknown SoC\n");
>>>               return -ENODEV;
>>> @@ -106,7 +142,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
>>>
>>>       dev_info(soc_device_to_device(soc_dev),
>>>                "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
>>> -              soc_dev_attr->soc_id, product_id, revision);
>>> +              soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
>>>
>>>       return 0;
>>>
>>> @@ -125,9 +161,18 @@ static int exynos_chipid_remove(struct platform_device *pdev)
>>>       return 0;
>>>  }
>>>
>>> +static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
>>> +     .rev_reg        = 0x0,
>>> +     .main_rev_shift = 0,
>>> +     .sub_rev_shift  = 4,
>>
>> The code does not look correct here. Subrev is at 0:3 bits, mainrev is
>> at 4:7.
>>
> 
> Right. I was confused by those existing macros:
> 
>     #define EXYNOS_SUBREV_MASK        (0xf << 4)
>     #define EXYNOS_MAINREV_MASK        (0xf << 0)
> 
> Those were probably wrong the whole time? Anyway, now I've found
> Exynos4412 User Manual and checked it there -- you are right about
> offsets. Will fix in v3.

They were not used, I think.

> 
>> Did you test it that it produces same result? Looks not - I gave it a
>> try and got wrong revision.
>>
> 
> I only have Exynos850 based board, and that has 0x0 in Revision ID
> register. But for v3 I'll try to emulate register value in the code
> and make sure that the read value does not change with patch applied.

You should get one of Odroid boards to test it. The MC1 is fairly cheap.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Sam Protsenko]

On Thu, 14 Oct 2021 at 14:48, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 14/10/2021 13:34, Sam Protsenko wrote:
> > On Thu, 14 Oct 2021 at 10:11, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 13/10/2021 22:21, Sam Protsenko wrote:
> >>> Old Exynos SoCs have both Product ID and Revision ID in one single
> >>> register, while new SoCs tend to have two separate registers for those
> >>> IDs. Implement handling of both cases by passing Revision ID register
> >>> offsets in driver data.
> >>>
> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> >>> ---
> >>>  drivers/soc/samsung/exynos-chipid.c       | 67 +++++++++++++++++++----
> >>>  include/linux/soc/samsung/exynos-chipid.h |  6 +-
> >>>  2 files changed, 58 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
> >>> index 5c1d0f97f766..7837331fb753 100644
> >>> --- a/drivers/soc/samsung/exynos-chipid.c
> >>> +++ b/drivers/soc/samsung/exynos-chipid.c
> >>> @@ -16,6 +16,7 @@
> >>>  #include <linux/errno.h>
> >>>  #include <linux/mfd/syscon.h>
> >>>  #include <linux/of.h>
> >>> +#include <linux/of_device.h>
> >>>  #include <linux/platform_device.h>
> >>>  #include <linux/regmap.h>
> >>>  #include <linux/slab.h>
> >>> @@ -24,6 +25,17 @@
> >>>
> >>>  #include "exynos-asv.h"
> >>>
> >>> +struct exynos_chipid_variant {
> >>> +     unsigned int rev_reg;           /* revision register offset */
> >>> +     unsigned int main_rev_shift;    /* main revision offset in rev_reg */
> >>> +     unsigned int sub_rev_shift;     /* sub revision offset in rev_reg */
> >>> +};
> >>> +
> >>> +struct exynos_chipid_info {
> >>> +     u32 product_id;
> >>> +     u32 revision;
> >>> +};
> >>> +
> >>>  static const struct exynos_soc_id {
> >>>       const char *name;
> >>>       unsigned int id;
> >>> @@ -49,31 +61,55 @@ static const char *product_id_to_soc_id(unsigned int product_id)
> >>>       int i;
> >>>
> >>>       for (i = 0; i < ARRAY_SIZE(soc_ids); i++)
> >>> -             if ((product_id & EXYNOS_MASK) == soc_ids[i].id)
> >>> +             if (product_id == soc_ids[i].id)
> >>>                       return soc_ids[i].name;
> >>>       return NULL;
> >>>  }
> >>>
> >>> +static int exynos_chipid_get_chipid_info(struct regmap *regmap,
> >>> +             const struct exynos_chipid_variant *data,
> >>> +             struct exynos_chipid_info *soc_info)
> >>> +{
> >>> +     int ret;
> >>> +     unsigned int val, main_rev, sub_rev;
> >>> +
> >>> +     ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &val);
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +     soc_info->product_id = val & EXYNOS_MASK;
> >>> +
> >>> +     ret = regmap_read(regmap, data->rev_reg, &val);
> >>
> >> Isn't this the same register as EXYNOS_CHIPID_REG_PRO_ID?
> >>
> >
> > It is for Exynos4210, but it's not for Exynos850 (see PATCH 3/3), as
> > it's described in the commit message. I tried to keep this code
> > unified for all SoCs.
>
> Yeah, but for Exynos4210 you read the same register twice. It's
> confusing. Read only once. You could compare the offsets and skip second
> read.
>

Thanks, will do in v3.

> >
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +     main_rev = (val >> data->main_rev_shift) & EXYNOS_REV_PART_MASK;
> >>> +     sub_rev = (val >> data->sub_rev_shift) & EXYNOS_REV_PART_MASK;
> >>> +     soc_info->revision = (main_rev << EXYNOS_REV_PART_SHIFT) | sub_rev;
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>>  static int exynos_chipid_probe(struct platform_device *pdev)
> >>>  {
> >>> +     const struct exynos_chipid_variant *drv_data;
> >>> +     struct exynos_chipid_info soc_info;
> >>>       struct soc_device_attribute *soc_dev_attr;
> >>>       struct soc_device *soc_dev;
> >>>       struct device_node *root;
> >>>       struct regmap *regmap;
> >>> -     u32 product_id;
> >>> -     u32 revision;
> >>>       int ret;
> >>>
> >>> +     drv_data = of_device_get_match_data(&pdev->dev);
> >>> +     if (!drv_data)
> >>> +             return -EINVAL;
> >>> +
> >>>       regmap = device_node_to_regmap(pdev->dev.of_node);
> >>>       if (IS_ERR(regmap))
> >>>               return PTR_ERR(regmap);
> >>>
> >>> -     ret = regmap_read(regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id);
> >>> +     ret = exynos_chipid_get_chipid_info(regmap, drv_data, &soc_info);
> >>>       if (ret < 0)
> >>>               return ret;
> >>>
> >>> -     revision = product_id & EXYNOS_REV_MASK;
> >>> -
> >>>       soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> >>>                                   GFP_KERNEL);
> >>>       if (!soc_dev_attr)
> >>> @@ -86,8 +122,8 @@ static int exynos_chipid_probe(struct platform_device *pdev)
> >>>       of_node_put(root);
> >>>
> >>>       soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> >>> -                                             "%x", revision);
> >>> -     soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
> >>> +                                             "%x", soc_info.revision);
> >>> +     soc_dev_attr->soc_id = product_id_to_soc_id(soc_info.product_id);
> >>>       if (!soc_dev_attr->soc_id) {
> >>>               pr_err("Unknown SoC\n");
> >>>               return -ENODEV;
> >>> @@ -106,7 +142,7 @@ static int exynos_chipid_probe(struct platform_device *pdev)
> >>>
> >>>       dev_info(soc_device_to_device(soc_dev),
> >>>                "Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x] Detected\n",
> >>> -              soc_dev_attr->soc_id, product_id, revision);
> >>> +              soc_dev_attr->soc_id, soc_info.product_id, soc_info.revision);
> >>>
> >>>       return 0;
> >>>
> >>> @@ -125,9 +161,18 @@ static int exynos_chipid_remove(struct platform_device *pdev)
> >>>       return 0;
> >>>  }
> >>>
> >>> +static const struct exynos_chipid_variant exynos4210_chipid_drv_data = {
> >>> +     .rev_reg        = 0x0,
> >>> +     .main_rev_shift = 0,
> >>> +     .sub_rev_shift  = 4,
> >>
> >> The code does not look correct here. Subrev is at 0:3 bits, mainrev is
> >> at 4:7.
> >>
> >
> > Right. I was confused by those existing macros:
> >
> >     #define EXYNOS_SUBREV_MASK        (0xf << 4)
> >     #define EXYNOS_MAINREV_MASK        (0xf << 0)
> >
> > Those were probably wrong the whole time? Anyway, now I've found
> > Exynos4412 User Manual and checked it there -- you are right about
> > offsets. Will fix in v3.
>
> They were not used, I think.
>
> >
> >> Did you test it that it produces same result? Looks not - I gave it a
> >> try and got wrong revision.
> >>
> >
> > I only have Exynos850 based board, and that has 0x0 in Revision ID
> > register. But for v3 I'll try to emulate register value in the code
> > and make sure that the read value does not change with patch applied.
>
> You should get one of Odroid boards to test it. The MC1 is fairly cheap.
>

Will do, I see how it can be useful for further work. For this series,
I'm pretty sure I can test all cases by emulating the read register
values. Would it be enough? Also, if you have some time, I'd ask you
to check v3 on your board.

> Best regards,
> Krzysztof

Re: [PATCH v2 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets

[Krzysztof Kozlowski]

On 14/10/2021 14:03, Sam Protsenko wrote:
> On Thu, 14 Oct 2021 at 14:48, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>>>
>>>> Did you test it that it produces same result? Looks not - I gave it a
>>>> try and got wrong revision.
>>>>
>>>
>>> I only have Exynos850 based board, and that has 0x0 in Revision ID
>>> register. But for v3 I'll try to emulate register value in the code
>>> and make sure that the read value does not change with patch applied.
>>
>> You should get one of Odroid boards to test it. The MC1 is fairly cheap.
>>
> 
> Will do, I see how it can be useful for further work. For this series,
> I'm pretty sure I can test all cases by emulating the read register
> values. Would it be enough? Also, if you have some time, I'd ask you
> to check v3 on your board.

Yes, it's OK. I can test it.

Best regards,
Krzysztof