On Thu, 14 Oct 2021 at 14:48, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> wrote:
>
> On 14/10/2021 13:34, Sam Protsenko wrote:
> > On Thu, 14 Oct 2021 at 10:11, Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> 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
