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

From: Krzysztof Kozlowski
Date: Thu Oct 14 2021 - 03:16:03 EST

On 13/10/2021 20:47, Sam Protsenko wrote:
> On Wed, 13 Oct 2021 at 19:04, Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxxxxx> 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@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..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