* [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
@ 2021-10-12 17:16 ` Sam Protsenko
0 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-12 17:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
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..1264a18aef97 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_bit; /* main revision offset */
+ unsigned int sub_rev_bit; /* sub revision offset */
+};
+
+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_bit) & EXYNOS_REV_PART_LEN;
+ sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
+ soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
+ .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
+#define EXYNOS_REV_PART_OFF 4
#define EXYNOS_MASK 0xfffff000
#define EXYNOS_CHIPID_REG_PKG_ID 0x04
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/3] dt-bindings: samsung: exynos-chipid: Document Exynos850 compatible
2021-10-12 17:16 ` Sam Protsenko
@ 2021-10-12 17:16 ` Sam Protsenko
-1 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-12 17:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/3] dt-bindings: samsung: exynos-chipid: Document Exynos850 compatible
@ 2021-10-12 17:16 ` Sam Protsenko
0 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-12 17:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] soc: samsung: exynos-chipid: Add Exynos850 support
2021-10-12 17:16 ` Sam Protsenko
@ 2021-10-12 17:16 ` Sam Protsenko
-1 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-12 17:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
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 1264a18aef97..d160f9b082c4 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_bit = 4,
};
+static const struct exynos_chipid_variant exynos850_chipid_drv_data = {
+ .rev_reg = 0x10,
+ .main_rev_bit = 20,
+ .sub_rev_bit = 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 3/3] soc: samsung: exynos-chipid: Add Exynos850 support
@ 2021-10-12 17:16 ` Sam Protsenko
0 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-12 17:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
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 1264a18aef97..d160f9b082c4 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_bit = 4,
};
+static const struct exynos_chipid_variant exynos850_chipid_drv_data = {
+ .rev_reg = 0x10,
+ .main_rev_bit = 20,
+ .sub_rev_bit = 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
2021-10-12 17:16 ` Sam Protsenko
@ 2021-10-13 16:04 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-13 16:04 UTC (permalink / raw)
To: Sam Protsenko, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
On 12/10/2021 19:16, 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..1264a18aef97 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_bit; /* main revision offset */
I understand it is offset of a bit within register, so how about:
unsigned int main_rev_shift; /* main revision offset within rev_reg
unsigned int sub_rev_shift; /* sub revision offset within rev_reg */
> + unsigned int sub_rev_bit; /* sub revision offset */
> +};
> +
> +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_bit) & EXYNOS_REV_PART_LEN;
> + sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
> + soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
> + .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
EXYNOS_REV_PART_MASK
> +#define EXYNOS_REV_PART_OFF 4
define EXYNOS_REV_PART_SHIFT
> #define EXYNOS_MASK 0xfffff000
>
> #define EXYNOS_CHIPID_REG_PKG_ID 0x04
>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
@ 2021-10-13 16:04 ` Krzysztof Kozlowski
0 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-13 16:04 UTC (permalink / raw)
To: Sam Protsenko, Rob Herring
Cc: Sumit Semwal, linux-samsung-soc, linux-arm-kernel, devicetree,
linux-kernel
On 12/10/2021 19:16, 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..1264a18aef97 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_bit; /* main revision offset */
I understand it is offset of a bit within register, so how about:
unsigned int main_rev_shift; /* main revision offset within rev_reg
unsigned int sub_rev_shift; /* sub revision offset within rev_reg */
> + unsigned int sub_rev_bit; /* sub revision offset */
> +};
> +
> +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_bit) & EXYNOS_REV_PART_LEN;
> + sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
> + soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
> + .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
EXYNOS_REV_PART_MASK
> +#define EXYNOS_REV_PART_OFF 4
define EXYNOS_REV_PART_SHIFT
> #define EXYNOS_MASK 0xfffff000
>
> #define EXYNOS_CHIPID_REG_PKG_ID 0x04
>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
2021-10-13 16:04 ` Krzysztof Kozlowski
@ 2021-10-13 18:47 ` Sam Protsenko
-1 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-13 18:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Sumit Semwal, Linux Samsung SOC,
linux-arm Mailing List, devicetree, Linux Kernel Mailing List
On Wed, 13 Oct 2021 at 19:04, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 12/10/2021 19:16, 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..1264a18aef97 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_bit; /* main revision offset */
>
> I understand it is offset of a bit within register, so how about:
>
> unsigned int main_rev_shift; /* main revision offset within rev_reg
> unsigned int sub_rev_shift; /* sub revision offset within rev_reg */
>
> > + unsigned int sub_rev_bit; /* sub revision offset */
> > +};
> > +
> > +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_bit) & EXYNOS_REV_PART_LEN;
> > + sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
> > + soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
> > + .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
>
> EXYNOS_REV_PART_MASK
>
> > +#define EXYNOS_REV_PART_OFF 4
>
> define EXYNOS_REV_PART_SHIFT
>
Thanks, I'll fix everything you mentioned in v2.
Btw, I forgot to provide an explanation on user interface changes I
made. Those are ok from my POV, but you might disagree:
1. EXYNOS_MASK is now applied to product_id when assigning it. The
only side effect is that dev_info() in probe() will print a bit
different info. Hope it's fine. The code looks better this way, as we
clearly differentiate SoC ID and Revision ID from the very beginning.
2. "revision" sysfs node will now show the revision in this form:
"(main_rev << 4) | sub_rev". Before this patch it was "(sub_rev << 4)
| main_rev". It has to do with internal register representation: on
older Exynos SoCs it has the latter form, on newer SoCs -- the former.
For consistency I want to keep it in the same form for all platforms.
I decided to go with approach implemented in downstream Samsung
kernel, though it of course changes the output on older SoCs. Possible
design options are:
(a) Use "(sub_rev << 4) | main_rev" form instead for all SoCs. It
would preserve "revision" node output on older SoCs. On newer SoCs it
will be rotated form w.r.t. internal register representation, and it
won't be consistent with downstream implementation (not that we should
care about that much)
(b) Use "(sub_rev << 4) | main_rev" form for old SoCs and
"(main_rev << 4) | sub_rev" for for new SoCs. That will clutter the
logic for sure, making it not very elegant.
(c) Keep it as is (as I already implemented that in this patch).
Changes "revision" node output, but I'm not sure if we should care
about that, user space shouldn't probably rely on that data anyway
Please let me know if you think the current design is ok, or you want
me to change that.
> > #define EXYNOS_MASK 0xfffff000
> >
> > #define EXYNOS_CHIPID_REG_PKG_ID 0x04
> >
>
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
@ 2021-10-13 18:47 ` Sam Protsenko
0 siblings, 0 replies; 12+ messages in thread
From: Sam Protsenko @ 2021-10-13 18:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Sumit Semwal, Linux Samsung SOC,
linux-arm Mailing List, devicetree, Linux Kernel Mailing List
On Wed, 13 Oct 2021 at 19:04, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 12/10/2021 19:16, 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..1264a18aef97 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_bit; /* main revision offset */
>
> I understand it is offset of a bit within register, so how about:
>
> unsigned int main_rev_shift; /* main revision offset within rev_reg
> unsigned int sub_rev_shift; /* sub revision offset within rev_reg */
>
> > + unsigned int sub_rev_bit; /* sub revision offset */
> > +};
> > +
> > +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_bit) & EXYNOS_REV_PART_LEN;
> > + sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
> > + soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
> > + .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
>
> EXYNOS_REV_PART_MASK
>
> > +#define EXYNOS_REV_PART_OFF 4
>
> define EXYNOS_REV_PART_SHIFT
>
Thanks, I'll fix everything you mentioned in v2.
Btw, I forgot to provide an explanation on user interface changes I
made. Those are ok from my POV, but you might disagree:
1. EXYNOS_MASK is now applied to product_id when assigning it. The
only side effect is that dev_info() in probe() will print a bit
different info. Hope it's fine. The code looks better this way, as we
clearly differentiate SoC ID and Revision ID from the very beginning.
2. "revision" sysfs node will now show the revision in this form:
"(main_rev << 4) | sub_rev". Before this patch it was "(sub_rev << 4)
| main_rev". It has to do with internal register representation: on
older Exynos SoCs it has the latter form, on newer SoCs -- the former.
For consistency I want to keep it in the same form for all platforms.
I decided to go with approach implemented in downstream Samsung
kernel, though it of course changes the output on older SoCs. Possible
design options are:
(a) Use "(sub_rev << 4) | main_rev" form instead for all SoCs. It
would preserve "revision" node output on older SoCs. On newer SoCs it
will be rotated form w.r.t. internal register representation, and it
won't be consistent with downstream implementation (not that we should
care about that much)
(b) Use "(sub_rev << 4) | main_rev" form for old SoCs and
"(main_rev << 4) | sub_rev" for for new SoCs. That will clutter the
logic for sure, making it not very elegant.
(c) Keep it as is (as I already implemented that in this patch).
Changes "revision" node output, but I'm not sure if we should care
about that, user space shouldn't probably rely on that data anyway
Please let me know if you think the current design is ok, or you want
me to change that.
> > #define EXYNOS_MASK 0xfffff000
> >
> > #define EXYNOS_CHIPID_REG_PKG_ID 0x04
> >
>
>
> Best regards,
> Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
2021-10-13 18:47 ` Sam Protsenko
@ 2021-10-14 7:15 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-14 7:15 UTC (permalink / raw)
To: Sam Protsenko
Cc: Rob Herring, Sumit Semwal, Linux Samsung SOC,
linux-arm Mailing List, devicetree, Linux Kernel Mailing List
On 13/10/2021 20:47, Sam Protsenko wrote:
> On Wed, 13 Oct 2021 at 19:04, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 12/10/2021 19:16, 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..1264a18aef97 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_bit; /* main revision offset */
>>
>> I understand it is offset of a bit within register, so how about:
>>
>> unsigned int main_rev_shift; /* main revision offset within rev_reg
>> unsigned int sub_rev_shift; /* sub revision offset within rev_reg */
>>
>>> + unsigned int sub_rev_bit; /* sub revision offset */
>>> +};
>>> +
>>> +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_bit) & EXYNOS_REV_PART_LEN;
>>> + sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
>>> + soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
>>> + .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
>>
>> EXYNOS_REV_PART_MASK
>>
>>> +#define EXYNOS_REV_PART_OFF 4
>>
>> define EXYNOS_REV_PART_SHIFT
>>
>
> Thanks, I'll fix everything you mentioned in v2.
>
> Btw, I forgot to provide an explanation on user interface changes I
> made. Those are ok from my POV, but you might disagree:
>
> 1. EXYNOS_MASK is now applied to product_id when assigning it. The
> only side effect is that dev_info() in probe() will print a bit
> different info. Hope it's fine. The code looks better this way, as we
> clearly differentiate SoC ID and Revision ID from the very beginning.
That's fine.
>
> 2. "revision" sysfs node will now show the revision in this form:
> "(main_rev << 4) | sub_rev". Before this patch it was "(sub_rev << 4)
> | main_rev". It has to do with internal register representation: on
> older Exynos SoCs it has the latter form, on newer SoCs -- the former.
> For consistency I want to keep it in the same form for all platforms.
> I decided to go with approach implemented in downstream Samsung
> kernel, though it of course changes the output on older SoCs. Possible
> design options are:
I miss the point. Regardless of representation in register - whether
main_rev is shifted or sub_rev - you should always print it the same
way. Currently it was printed "(main_rev << 4) | sub_rev" and it should
not be changed.
>
> (a) Use "(sub_rev << 4) | main_rev" form instead for all SoCs. It
> would preserve "revision" node output on older SoCs. On newer SoCs it
> will be rotated form w.r.t. internal register representation, and it
> won't be consistent with downstream implementation (not that we should
> care about that much)
> (b) Use "(sub_rev << 4) | main_rev" form for old SoCs and
> "(main_rev << 4) | sub_rev" for for new SoCs. That will clutter the
> logic for sure, making it not very elegant.
> (c) Keep it as is (as I already implemented that in this patch).
> Changes "revision" node output, but I'm not sure if we should care
> about that, user space shouldn't probably rely on that data anyway
Should be always printed as "(main_rev << 4) | sub_rev" regardless how
it is written in register. That's how it works so far.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] soc: samsung: exynos-chipid: Pass revision reg offsets
@ 2021-10-14 7:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-14 7:15 UTC (permalink / raw)
To: Sam Protsenko
Cc: Rob Herring, Sumit Semwal, Linux Samsung SOC,
linux-arm Mailing List, devicetree, Linux Kernel Mailing List
On 13/10/2021 20:47, Sam Protsenko wrote:
> On Wed, 13 Oct 2021 at 19:04, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 12/10/2021 19:16, 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..1264a18aef97 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_bit; /* main revision offset */
>>
>> I understand it is offset of a bit within register, so how about:
>>
>> unsigned int main_rev_shift; /* main revision offset within rev_reg
>> unsigned int sub_rev_shift; /* sub revision offset within rev_reg */
>>
>>> + unsigned int sub_rev_bit; /* sub revision offset */
>>> +};
>>> +
>>> +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_bit) & EXYNOS_REV_PART_LEN;
>>> + sub_rev = (val >> data->sub_rev_bit) & EXYNOS_REV_PART_LEN;
>>> + soc_info->revision = (main_rev << EXYNOS_REV_PART_OFF) | 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_bit = 0,
>>> + .sub_rev_bit = 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..5270725ba408 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_LEN 0xf
>>
>> EXYNOS_REV_PART_MASK
>>
>>> +#define EXYNOS_REV_PART_OFF 4
>>
>> define EXYNOS_REV_PART_SHIFT
>>
>
> Thanks, I'll fix everything you mentioned in v2.
>
> Btw, I forgot to provide an explanation on user interface changes I
> made. Those are ok from my POV, but you might disagree:
>
> 1. EXYNOS_MASK is now applied to product_id when assigning it. The
> only side effect is that dev_info() in probe() will print a bit
> different info. Hope it's fine. The code looks better this way, as we
> clearly differentiate SoC ID and Revision ID from the very beginning.
That's fine.
>
> 2. "revision" sysfs node will now show the revision in this form:
> "(main_rev << 4) | sub_rev". Before this patch it was "(sub_rev << 4)
> | main_rev". It has to do with internal register representation: on
> older Exynos SoCs it has the latter form, on newer SoCs -- the former.
> For consistency I want to keep it in the same form for all platforms.
> I decided to go with approach implemented in downstream Samsung
> kernel, though it of course changes the output on older SoCs. Possible
> design options are:
I miss the point. Regardless of representation in register - whether
main_rev is shifted or sub_rev - you should always print it the same
way. Currently it was printed "(main_rev << 4) | sub_rev" and it should
not be changed.
>
> (a) Use "(sub_rev << 4) | main_rev" form instead for all SoCs. It
> would preserve "revision" node output on older SoCs. On newer SoCs it
> will be rotated form w.r.t. internal register representation, and it
> won't be consistent with downstream implementation (not that we should
> care about that much)
> (b) Use "(sub_rev << 4) | main_rev" form for old SoCs and
> "(main_rev << 4) | sub_rev" for for new SoCs. That will clutter the
> logic for sure, making it not very elegant.
> (c) Keep it as is (as I already implemented that in this patch).
> Changes "revision" node output, but I'm not sure if we should care
> about that, user space shouldn't probably rely on that data anyway
Should be always printed as "(main_rev << 4) | sub_rev" regardless how
it is written in register. That's how it works so far.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread