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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

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