RE: [PATCH v2 4/4] soc: samsung: exynos-chipid: convert to driver and merge exynos-asv
From: Pankaj Dubey
Date: Tue Dec 08 2020 - 02:02:36 EST
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Tuesday, December 8, 2020 12:35 AM
> To: Krzysztof Kozlowski <krzk@xxxxxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: Sylwester Nawrocki <snawrocki@xxxxxxxxxx>; Marek Szyprowski
> <m.szyprowski@xxxxxxxxxxx>; Bartlomiej Zolnierkiewicz
> <b.zolnierkie@xxxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Chanwoo
> Choi <cw00.choi@xxxxxxxxxxx>; Alim Akhtar <alim.akhtar@xxxxxxxxxxx>;
> Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> Subject: [PATCH v2 4/4] soc: samsung: exynos-chipid: convert to driver and
> merge exynos-asv
>
> The Exynos Chip ID driver on Exynos SoCs has so far only informational
> purpose - to expose the SoC device in sysfs. No other drivers depend on
it
> so there is really no benefit of initializing it early.
>
One of the intention behind initializing Exynos Chip ID driver in early
stage was to simplify code in arch/arm/mach-exynos specifically calls such
as soc_is_exynosXXXX.
But there were lot of resistance from community to add any such calls (or
exported function) from mach-exynos files to the driver file. Whereas some
other SoC code is using the same, e.g. tegra_get_chip_id being called from
mach-tegra files to drivers/soc/tegra/. Unfortunately we could not accept
similar solution for Exynos SoC and hence could not get rid of
soc_is_exynosxXXX and similar macros from various file in mach-exynos and
eventually converting those files into a full-fledged driver files.
Any opinion how can we achieve this if we convert Exynos Chip ID driver to a
regular driver?
Thanks,
Pankaj Dubey
> The code would be the most flexible if converted to a regular driver.
> However there is already another driver - Exynos ASV (Adaptive Supply
> Voltage) - which binds to the device node of Chip ID.
>
> The solution is to convert the Exynos Chip ID to a built in driver and
merge
> the Exynos ASV into it.
>
> This has several benefits:
> 1. Although the Exynos ASV driver binds to a device node present in all
> Exynos DTS (generic compatible), it fails to probe except on the
> supported ones (only Exynos5422). This means that the regular boot
> process has a planned/expected device probe failure.
>
> Merging the ASV into Chip ID will remove this probe failure because
> the final driver will always bind, just with disabled ASV features.
>
> 2. Allows to use dev_info() as the SoC bus is present (since
> core_initcall).
>
> 3. Could speed things up because of execution of Chip ID code in a SMP
> environment (after bringing up secondary CPUs, unlike early_initcall),
> This reduces the amount of work to be done early, when the kernel has
> to bring up critical devices.
>
> 5. Makes the Chip ID code defer-probe friendly,
>
> Signed-off-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> ---
> arch/arm/mach-exynos/Kconfig | 1 -
> drivers/soc/samsung/Kconfig | 12 ++---
> drivers/soc/samsung/Makefile | 3 +-
> drivers/soc/samsung/exynos-asv.c | 45 +++++------------
> drivers/soc/samsung/exynos-asv.h | 2 +
> drivers/soc/samsung/exynos-chipid.c | 75 ++++++++++++++++++++---------
> 6 files changed, 70 insertions(+), 68 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-
> exynos/Kconfig index 56d272967fc0..5a48abac6af4 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -13,7 +13,6 @@ menuconfig ARCH_EXYNOS
> select ARM_GIC
> select EXYNOS_IRQ_COMBINER
> select COMMON_CLK_SAMSUNG
> - select EXYNOS_ASV
> select EXYNOS_CHIPID
> select EXYNOS_THERMAL
> select EXYNOS_PMU
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index fc7f48a92288..5745d7e5908e 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -7,21 +7,19 @@ menuconfig SOC_SAMSUNG
>
> if SOC_SAMSUNG
>
> -config EXYNOS_ASV
> - bool "Exynos Adaptive Supply Voltage support" if COMPILE_TEST
> - depends on (ARCH_EXYNOS && EXYNOS_CHIPID) || COMPILE_TEST
> - select EXYNOS_ASV_ARM if ARM && ARCH_EXYNOS
> -
> # There is no need to enable these drivers for ARMv8 config
> EXYNOS_ASV_ARM
> bool "Exynos ASV ARMv7-specific driver extensions" if
> COMPILE_TEST
> - depends on EXYNOS_ASV
> + depends on EXYNOS_CHIPID
>
> config EXYNOS_CHIPID
> - bool "Exynos Chipid controller driver" if COMPILE_TEST
> + bool "Exynos ChipID controller and ASV driver" if COMPILE_TEST
> depends on ARCH_EXYNOS || COMPILE_TEST
> + select EXYNOS_ASV_ARM if ARM && ARCH_EXYNOS
> select MFD_SYSCON
> select SOC_BUS
> + help
> + Support for Samsung Exynos SoC ChipID and Adaptive Supply
> Voltage.
>
> config EXYNOS_PMU
> bool "Exynos PMU controller driver" if COMPILE_TEST diff --git
> a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile index
> 59e8e9453f27..0c523a8de4eb 100644
> --- a/drivers/soc/samsung/Makefile
> +++ b/drivers/soc/samsung/Makefile
> @@ -1,9 +1,8 @@
> # SPDX-License-Identifier: GPL-2.0
>
> -obj-$(CONFIG_EXYNOS_ASV) += exynos-asv.o
> obj-$(CONFIG_EXYNOS_ASV_ARM) += exynos5422-asv.o
>
> -obj-$(CONFIG_EXYNOS_CHIPID) += exynos-chipid.o
> +obj-$(CONFIG_EXYNOS_CHIPID) += exynos-chipid.o exynos-asv.o
> obj-$(CONFIG_EXYNOS_PMU) += exynos-pmu.o
>
> obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS) += exynos3250-
> pmu.o exynos4-pmu.o \
> diff --git a/drivers/soc/samsung/exynos-asv.c
> b/drivers/soc/samsung/exynos-asv.c
> index 5daeadc36382..d60af8acc391 100644
> --- a/drivers/soc/samsung/exynos-asv.c
> +++ b/drivers/soc/samsung/exynos-asv.c
> @@ -2,7 +2,9 @@
> /*
> * Copyright (c) 2019 Samsung Electronics Co., Ltd.
> * http://www.samsung.com/
> + * Copyright (c) 2020 Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> * Author: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> + * Author: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> *
> * Samsung Exynos SoC Adaptive Supply Voltage support
> */
> @@ -10,12 +12,7 @@
> #include <linux/cpu.h>
> #include <linux/device.h>
> #include <linux/errno.h>
> -#include <linux/init.h>
> -#include <linux/mfd/syscon.h>
> -#include <linux/module.h>
> #include <linux/of.h>
> -#include <linux/of_device.h>
> -#include <linux/platform_device.h>
> #include <linux/pm_opp.h>
> #include <linux/regmap.h>
> #include <linux/soc/samsung/exynos-chipid.h>
> @@ -111,7 +108,7 @@ static int exynos_asv_update_opps(struct
> exynos_asv *asv)
> return 0;
> }
>
> -static int exynos_asv_probe(struct platform_device *pdev)
> +int exynos_asv_init(struct device *dev, struct regmap *regmap)
> {
> int (*probe_func)(struct exynos_asv *asv);
> struct exynos_asv *asv;
> @@ -119,21 +116,16 @@ static int exynos_asv_probe(struct platform_device
> *pdev)
> u32 product_id = 0;
> int ret, i;
>
> - asv = devm_kzalloc(&pdev->dev, sizeof(*asv), GFP_KERNEL);
> + asv = devm_kzalloc(dev, sizeof(*asv), GFP_KERNEL);
> if (!asv)
> return -ENOMEM;
>
> - asv->chipid_regmap = device_node_to_regmap(pdev-
> >dev.of_node);
> - if (IS_ERR(asv->chipid_regmap)) {
> - dev_err(&pdev->dev, "Could not find syscon regmap\n");
> - return PTR_ERR(asv->chipid_regmap);
> - }
> -
> + asv->chipid_regmap = regmap;
> + asv->dev = dev;
> ret = regmap_read(asv->chipid_regmap,
> EXYNOS_CHIPID_REG_PRO_ID,
> &product_id);
> if (ret < 0) {
> - dev_err(&pdev->dev, "Cannot read revision from
> ChipID: %d\n",
> - ret);
> + dev_err(dev, "Cannot read revision from ChipID: %d\n", ret);
> return -ENODEV;
> }
>
> @@ -142,7 +134,9 @@ static int exynos_asv_probe(struct platform_device
> *pdev)
> probe_func = exynos5422_asv_init;
> break;
> default:
> - return -ENODEV;
> + dev_dbg(dev, "No ASV support for this SoC\n");
> + devm_kfree(dev, asv);
> + return 0;
> }
>
> cpu_dev = get_cpu_device(0);
> @@ -150,14 +144,11 @@ static int exynos_asv_probe(struct platform_device
> *pdev)
> if (ret < 0)
> return -EPROBE_DEFER;
>
> - ret = of_property_read_u32(pdev->dev.of_node, "samsung,asv-
> bin",
> + ret = of_property_read_u32(dev->of_node, "samsung,asv-bin",
> &asv->of_bin);
> if (ret < 0)
> asv->of_bin = -EINVAL;
>
> - asv->dev = &pdev->dev;
> - dev_set_drvdata(&pdev->dev, asv);
> -
> for (i = 0; i < ARRAY_SIZE(asv->subsys); i++)
> asv->subsys[i].asv = asv;
>
> @@ -167,17 +158,3 @@ static int exynos_asv_probe(struct platform_device
> *pdev)
>
> return exynos_asv_update_opps(asv);
> }
> -
> -static const struct of_device_id exynos_asv_of_device_ids[] = {
> - { .compatible = "samsung,exynos4210-chipid" },
> - {}
> -};
> -
> -static struct platform_driver exynos_asv_driver = {
> - .driver = {
> - .name = "exynos-asv",
> - .of_match_table = exynos_asv_of_device_ids,
> - },
> - .probe = exynos_asv_probe,
> -};
> -module_platform_driver(exynos_asv_driver);
> diff --git a/drivers/soc/samsung/exynos-asv.h
> b/drivers/soc/samsung/exynos-asv.h
> index 3fd1f2acd999..dcbe154db31e 100644
> --- a/drivers/soc/samsung/exynos-asv.h
> +++ b/drivers/soc/samsung/exynos-asv.h
> @@ -68,4 +68,6 @@ static inline u32 exynos_asv_opp_get_frequency(const
> struct exynos_asv_subsys *s
> return __asv_get_table_entry(&subsys->table, level, 0); }
>
> +int exynos_asv_init(struct device *dev, struct regmap *regmap);
> +
> #endif /* __LINUX_SOC_EXYNOS_ASV_H */
> diff --git a/drivers/soc/samsung/exynos-chipid.c
> b/drivers/soc/samsung/exynos-chipid.c
> index b4cd0cc00f45..fa6a9b9f6d70 100644
> --- a/drivers/soc/samsung/exynos-chipid.c
> +++ b/drivers/soc/samsung/exynos-chipid.c
> @@ -2,20 +2,28 @@
> /*
> * Copyright (c) 2019 Samsung Electronics Co., Ltd.
> * http://www.samsung.com/
> + * Copyright (c) 2020 Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> *
> * Exynos - CHIP ID support
> * Author: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> * Author: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> + * Author: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> + *
> + * Samsung Exynos SoC Adaptive Supply Voltage and Chip ID support
> */
>
> -#include <linux/io.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> #include <linux/mfd/syscon.h>
> #include <linux/of.h>
> +#include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/soc/samsung/exynos-chipid.h>
> #include <linux/sys_soc.h>
>
> +#include "exynos-asv.h"
> +
> static const struct exynos_soc_id {
> const char *name;
> unsigned int id;
> @@ -46,25 +54,17 @@ static const char * __init
> product_id_to_soc_id(unsigned int product_id)
> return NULL;
> }
>
> -static int __init exynos_chipid_early_init(void)
> +static int exynos_chipid_probe(struct platform_device *pdev)
> {
> struct soc_device_attribute *soc_dev_attr;
> struct soc_device *soc_dev;
> struct device_node *root;
> - struct device_node *syscon;
> struct regmap *regmap;
> u32 product_id;
> u32 revision;
> int ret;
>
> - syscon = of_find_compatible_node(NULL, NULL,
> - "samsung,exynos4210-chipid");
> - if (!syscon)
> - return -ENODEV;
> -
> - regmap = device_node_to_regmap(syscon);
> - of_node_put(syscon);
> -
> + regmap = device_node_to_regmap(pdev->dev.of_node);
> if (IS_ERR(regmap))
> return PTR_ERR(regmap);
>
> @@ -74,7 +74,8 @@ static int __init exynos_chipid_early_init(void)
>
> revision = product_id & EXYNOS_REV_MASK;
>
> - soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> + GFP_KERNEL);
> if (!soc_dev_attr)
> return -ENOMEM;
>
> @@ -84,31 +85,57 @@ static int __init exynos_chipid_early_init(void)
> of_property_read_string(root, "model", &soc_dev_attr->machine);
> of_node_put(root);
>
> - soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x", revision);
> + soc_dev_attr->revision = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "%x", revision);
> soc_dev_attr->soc_id = product_id_to_soc_id(product_id);
> if (!soc_dev_attr->soc_id) {
> pr_err("Unknown SoC\n");
> - ret = -ENODEV;
> - goto err;
> + return -ENODEV;
> }
>
> /* please note that the actual registration will be deferred */
> soc_dev = soc_device_register(soc_dev_attr);
> - if (IS_ERR(soc_dev)) {
> - ret = PTR_ERR(soc_dev);
> + if (IS_ERR(soc_dev))
> + return PTR_ERR(soc_dev);
> +
> + ret = exynos_asv_init(&pdev->dev, regmap);
> + if (ret)
> goto err;
> - }
>
> - /* it is too early to use dev_info() here (soc_dev is NULL) */
> - pr_info("soc soc0: Exynos: CPU[%s] PRO_ID[0x%x] REV[0x%x]
> Detected\n",
> - soc_dev_attr->soc_id, product_id, revision);
> + platform_set_drvdata(pdev, soc_dev);
> +
> + 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);
>
> return 0;
>
> err:
> - kfree(soc_dev_attr->revision);
> - kfree(soc_dev_attr);
> + soc_device_unregister(soc_dev);
> +
> return ret;
> }
>
> -early_initcall(exynos_chipid_early_init);
> +static int exynos_chipid_remove(struct platform_device *pdev) {
> + struct soc_device *soc_dev = platform_get_drvdata(pdev);
> +
> + soc_device_unregister(soc_dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_chipid_of_device_ids[] = {
> + { .compatible = "samsung,exynos4210-chipid" },
> + {}
> +};
> +
> +static struct platform_driver exynos_chipid_driver = {
> + .driver = {
> + .name = "exynos-chipid",
> + .of_match_table = exynos_chipid_of_device_ids,
> + },
> + .probe = exynos_chipid_probe,
> + .remove = exynos_chipid_remove,
> +};
> +builtin_platform_driver(exynos_chipid_driver);
> --
> 2.25.1