| Message ID | 20190813150827.31972-1-s.nawrocki@samsung.com |
|---|---|
| Headers | show |
| Series | Exynos Adaptive Supply Voltage support | expand |
On Tue, 13 Aug 2019 at 17:08, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > > The Adaptive Supply Voltage (ASV) driver adjusts CPU cluster operating > points depending on exact revision of an SoC retrieved from the CHIPID > block or the OTP memory. This allows for some power saving as for some > CPU clock frequencies we can lower CPU cluster supply voltage comparing > to safe values common to the all chip revisions. > > This patch adds support for Exynos5422/5800 SoC, it is partially based > on code from https://github.com/hardkernel/linux repository, > branch odroidxu4-4.14.y, files: arch/arm/mach-exynos/exynos5422-asv.[ch]. > > Tested on Odroid XU3, XU4, XU3 Lite. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v2: > - Use devm_kzalloc() in probe() to avoid memory leak, > - removed leading spaces in exynos-chipid.h, > - removed unneeded <linux/init.h> header inclusion, > - dropped parentheses from exynos542_asv_parse_sg(), > - updated Kconfig entry, > - added const attribute to struct exynos_asv_susbsys::cpu_dt_compat. > > Changes since v1 (RFC): > - removed code for parsing the ASV OPP tables from DT, the ASV OPP tables > moved to the driver; > - converted to use the regmap API; > - converted to normal platform driver. > --- > drivers/soc/samsung/Kconfig | 10 + > drivers/soc/samsung/Makefile | 3 + > drivers/soc/samsung/exynos-asv.c | 184 ++++++++++ > drivers/soc/samsung/exynos-asv.h | 82 +++++ > drivers/soc/samsung/exynos5422-asv.c | 498 +++++++++++++++++++++++++++ > drivers/soc/samsung/exynos5422-asv.h | 25 ++ > 6 files changed, 802 insertions(+) > create mode 100644 drivers/soc/samsung/exynos-asv.c > create mode 100644 drivers/soc/samsung/exynos-asv.h > create mode 100644 drivers/soc/samsung/exynos5422-asv.c > create mode 100644 drivers/soc/samsung/exynos5422-asv.h > > diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig > index 2905f5262197..73ccf59676a1 100644 > --- a/drivers/soc/samsung/Kconfig > +++ b/drivers/soc/samsung/Kconfig > @@ -7,6 +7,16 @@ 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 > + > config EXYNOS_CHIPID > bool "Exynos Chipid controller driver" if COMPILE_TEST > depends on ARCH_EXYNOS || COMPILE_TEST > diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile > index 3b6a8797416c..edd1d6ea064d 100644 > --- a/drivers/soc/samsung/Makefile > +++ b/drivers/soc/samsung/Makefile > @@ -1,5 +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_PMU) += exynos-pmu.o > > diff --git a/drivers/soc/samsung/exynos-asv.c b/drivers/soc/samsung/exynos-asv.c > new file mode 100644 > index 000000000000..481deb600afc > --- /dev/null > +++ b/drivers/soc/samsung/exynos-asv.c > @@ -0,0 +1,184 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019 Samsung Electronics Co., Ltd. > + * http://www.samsung.com/ > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> > + * > + * Samsung Exynos SoC Adaptive Supply Voltage support > + */ > + > +#include <linux/cpu.h> > +#include <linux/delay.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> > + > +#include "exynos-asv.h" > +#include "exynos5422-asv.h" > + > +#define MHZ 1000000U > + > +static int exynos_asv_update_cpu_opps(struct exynos_asv *asv, > + struct device *cpu) > +{ > + struct exynos_asv_subsys *subsys = NULL; > + struct dev_pm_opp *opp; > + unsigned int opp_freq; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(asv->subsys); i++) { > + if (of_device_is_compatible(cpu->of_node, > + asv->subsys[i].cpu_dt_compat)) { > + subsys = &asv->subsys[i]; > + break; > + } > + } > + if (!subsys) > + return -EINVAL; > + > + for (i = 0; i < subsys->table.num_rows; i++) { > + unsigned int new_voltage; > + unsigned int voltage; > + int timeout = 1000; > + int err; > + > + opp_freq = exynos_asv_opp_get_frequency(subsys, i); > + > + opp = dev_pm_opp_find_freq_exact(cpu, opp_freq * MHZ, true); > + if (IS_ERR(opp)) { > + dev_info(asv->dev, "cpu%d opp%d, freq: %u missing\n", > + cpu->id, i, opp_freq); > + > + continue; > + } > + > + voltage = dev_pm_opp_get_voltage(opp); > + new_voltage = asv->opp_get_voltage(subsys, i, voltage); > + dev_pm_opp_put(opp); > + > + opp_freq *= MHZ; > + dev_pm_opp_remove(cpu, opp_freq); > + > + while (--timeout) { > + opp = dev_pm_opp_find_freq_exact(cpu, opp_freq, true); > + if (IS_ERR(opp)) > + break; > + dev_pm_opp_put(opp); > + msleep(1); > + } > + > + err = dev_pm_opp_add(cpu, opp_freq, new_voltage); > + if (err < 0) > + dev_err(asv->dev, > + "Failed to add OPP %u Hz/%u uV for cpu%d\n", > + opp_freq, new_voltage, cpu->id); > + } > + > + return 0; > +} > + > +static int exynos_asv_update_opps(struct exynos_asv *asv) > +{ > + struct opp_table *last_opp_table = NULL; > + struct device *cpu; > + int ret, cpuid; > + > + for_each_possible_cpu(cpuid) { > + struct opp_table *opp_table; > + > + cpu = get_cpu_device(cpuid); > + if (!cpu) > + continue; > + > + opp_table = dev_pm_opp_get_opp_table(cpu); > + if (IS_ERR(opp_table)) > + continue; > + > + if (!last_opp_table || opp_table != last_opp_table) { > + last_opp_table = opp_table; > + > + ret = exynos_asv_update_cpu_opps(asv, cpu); > + if (ret < 0) > + dev_err(asv->dev, "Couldn't udate OPPs for cpu%d\n", > + cpuid); > + } > + > + dev_pm_opp_put_opp_table(opp_table); > + } > + > + return 0; > +} > + > +static int exynos_asv_probe(struct platform_device *pdev) > +{ > + int (*probe_func)(struct exynos_asv *asv); > + struct exynos_asv *asv; > + struct device *cpu_dev; > + u32 product_id = 0; > + int ret, i; > + > + cpu_dev = get_cpu_device(0); > + ret = dev_pm_opp_get_opp_count(cpu_dev); > + if (ret < 0) > + return -EPROBE_DEFER; > + > + asv = devm_kzalloc(&pdev->dev, sizeof(*asv), GFP_KERNEL); > + if (!asv) > + return -ENOMEM; > + > + asv->chipid_regmap = syscon_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); > + } > + > + regmap_read(asv->chipid_regmap, EXYNOS_CHIPID_REG_PRO_ID, &product_id); > + > + switch (product_id & EXYNOS_MASK) { > + case 0xE5422000: > + probe_func = exynos5422_asv_init; > + break; > + default: > + dev_err(&pdev->dev, "Unsupported product ID: %#x", product_id); > + return -ENODEV; > + } > + > + ret = of_property_read_u32(pdev->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; > + > + ret = probe_func(asv); > + if (ret < 0) > + return ret; > + > + 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 > new file mode 100644 > index 000000000000..14b4fedf2ddd > --- /dev/null > +++ b/drivers/soc/samsung/exynos-asv.h > @@ -0,0 +1,82 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2019 Samsung Electronics Co., Ltd. > + * http://www.samsung.com/ > + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> > + * > + * Samsung Exynos SoC Adaptive Supply Voltage support > + */ > +#ifndef __LINUX_SOC_EXYNOS_ASV_H > +#define __LINUX_SOC_EXYNOS_ASV_H Yikes, that was my mistake. The file is in drivers/soc, not include, so this could stay as previous one. Or make more path dependend - __DRIVERS_SOC... Now it is inconsistent with drivers/soc/samsung/exynos5422-asv.h. I can fixup these two files while applying but if there is going to be a resend, then change both to __DRIVERS_SOC_..._H. Best regards, Krzysztof
On Tue, 13 Aug 2019 at 17:08, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > > This is third iteration of my patch series adding ASV (Adaptive Supply > Voltage) support for Exynos SoCs. The previous one can be found at: > https://lore.kernel.org/lkml/20190718143044.25066-1-s.nawrocki@samsung.com > > There is no major changes in this series comparing to v2, only minor > corrections addressing review comments. > > I was not sure it was a good idea to try to extend the OPP binding > so as to include the ASV data tables in DT, so the tables are left > in the driver. > > This patch set includes Exynos CHIPID driver posted by Pankaj Dubey and > futher improved by Bartłomiej Żołnierkiewicz [1]. > > Tested on Odroid XU3, XU3 Lite, XU4. > > One of the things on TODO list is support for the Adaptive Body Bias. > This will require modifications on the cpufreq driver side in order to > support multiple voltage regulators and changes in the OPP framework > to support adding OPPs with multiple voltages. > > [1] https://lkml.org/lkml/2018/11/15/908 > > Pankaj Dubey (3): > soc: samsung: Add exynos chipid driver support > ARM: EXYNOS: enable exynos_chipid for ARCH_EXYNOS > ARM64: EXYNOS: enable exynos_chipid for ARCH_EXYNOS > > Sylwester Nawrocki (6): > soc: samsung: Convert exynos-chipid driver to use the regmap API > soc: samsung: Add Exynos Adaptive Supply Voltage driver > ARM: EXYNOS: Enable exynos-asv driver for ARCH_EXYNOS > soc: samsung: Update the CHIP ID DT binding documentation > ARM: dts: Add "syscon" compatible string to chipid node > ARM: dts: Add samsung,asv-bin property for odroidxu3-lite All look good to me but I need acks for bindings before applying. Best regards, Krzysztof
On Tue, Aug 13, 2019 at 05:08:19PM +0200, Sylwester Nawrocki wrote: > From: Pankaj Dubey <pankaj.dubey@samsung.com> > > Exynos SoCs have Chipid, for identification of product IDs and SoC > revisions. This patch intends to provide initialization code for all > these functionalities, at the same time it provides some sysfs entries > for accessing these information to user-space. > Thanks, applied. Best regards, Krzysztof
On Tue, Aug 13, 2019 at 05:08:20PM +0200, Sylwester Nawrocki wrote: > Convert the driver to use regmap API in order to allow other > drivers, like ASV, to access the CHIPID registers. > > This patch adds definition of selected CHIPID register offsets > and register bit fields for Exynos5422 SoC. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v2: > - s/_EXYNOS_ASV_H/__LINU_SOC_EXYNOS_ASV_H, > - removed __func__ from error log, > - removed unneeded <linux/of_address.h> header inclusion. > > Changes since v1 (RFC): > - new patch > --- > drivers/soc/samsung/exynos-chipid.c | 34 ++++++--------- > include/linux/soc/samsung/exynos-chipid.h | 52 +++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 21 deletions(-) Thanks, applied. Best regards, Krzysztof
On Tue, Aug 13, 2019 at 05:08:22PM +0200, Sylwester Nawrocki wrote: > From: Pankaj Dubey <pankaj.dubey@samsung.com> > > As now we have chipid driver to initialize SoC related information > let's include it in build by default. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v1 (RFC): Thanks, applied. Best regards, Krzysztof
On Tue, Aug 13, 2019 at 05:08:23PM +0200, Sylwester Nawrocki wrote: > From: Pankaj Dubey <pankaj.dubey@samsung.com> > > This patch enables exynos_chipid driver for ARCH_EXYNOS > based SoC. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v1 (RFC): > - none > --- > arch/arm64/Kconfig.platforms | 1 + Thanks, applied. Best regards, Krzysztof
On 13/08/2019 16:08, Sylwester Nawrocki wrote: > Convert the driver to use regmap API in order to allow other > drivers, like ASV, to access the CHIPID registers. > > This patch adds definition of selected CHIPID register offsets > and register bit fields for Exynos5422 SoC. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v2: > - s/_EXYNOS_ASV_H/__LINU_SOC_EXYNOS_ASV_H, > - removed __func__ from error log, > - removed unneeded <linux/of_address.h> header inclusion. > > Changes since v1 (RFC): > - new patch > --- > drivers/soc/samsung/exynos-chipid.c | 34 ++++++--------- > include/linux/soc/samsung/exynos-chipid.h | 52 +++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 21 deletions(-) > create mode 100644 include/linux/soc/samsung/exynos-chipid.h > > diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c > index bcf691f2b650..006a95feb618 100644 > --- a/drivers/soc/samsung/exynos-chipid.c > +++ b/drivers/soc/samsung/exynos-chipid.c > @@ -9,16 +9,13 @@ > */ > > #include <linux/io.h> > +#include <linux/mfd/syscon.h> > #include <linux/of.h> > -#include <linux/of_address.h> > +#include <linux/regmap.h> > #include <linux/slab.h> > +#include <linux/soc/samsung/exynos-chipid.h> > #include <linux/sys_soc.h> > > -#define EXYNOS_SUBREV_MASK (0xF << 4) > -#define EXYNOS_MAINREV_MASK (0xF << 0) > -#define EXYNOS_REV_MASK (EXYNOS_SUBREV_MASK | EXYNOS_MAINREV_MASK) > -#define EXYNOS_MASK 0xFFFFF000 > - > static const struct exynos_soc_id { > const char *name; > unsigned int id; > @@ -51,29 +48,24 @@ static const char * __init product_id_to_soc_id(unsigned int product_id) > int __init exynos_chipid_early_init(void) > { > struct soc_device_attribute *soc_dev_attr; > - void __iomem *exynos_chipid_base; > struct soc_device *soc_dev; > struct device_node *root; > - struct device_node *np; > + struct regmap *regmap; > u32 product_id; > u32 revision; > + int ret; > > - /* look up for chipid node */ > - np = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-chipid"); > - if (!np) > - return -ENODEV; > - > - exynos_chipid_base = of_iomap(np, 0); > - of_node_put(np); > - > - if (!exynos_chipid_base) { > - pr_err("Failed to map SoC chipid\n"); > - return -ENXIO; > + regmap = syscon_regmap_lookup_by_compatible("samsung,exynos4210-chipid"); > + if (IS_ERR(regmap)) { > + pr_err("Failed to get CHIPID regmap\n"); > + return PTR_ERR(regmap); > } Following this change, I am now seeing the above error on our Tegra boards where this driver is enabled. This is triggering a kernel warnings test we have to fail. Hence, I don't think that you can remove the compatible node test here, unless you have a better way to determine if this is a samsung device. Cheers Jon
On Tue, 20 Aug 2019 at 21:24, Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 13/08/2019 16:08, Sylwester Nawrocki wrote: > > Convert the driver to use regmap API in order to allow other > > drivers, like ASV, to access the CHIPID registers. > > > > This patch adds definition of selected CHIPID register offsets > > and register bit fields for Exynos5422 SoC. > > > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > --- > > Changes since v2: > > - s/_EXYNOS_ASV_H/__LINU_SOC_EXYNOS_ASV_H, > > - removed __func__ from error log, > > - removed unneeded <linux/of_address.h> header inclusion. > > > > Changes since v1 (RFC): > > - new patch > > --- > > drivers/soc/samsung/exynos-chipid.c | 34 ++++++--------- > > include/linux/soc/samsung/exynos-chipid.h | 52 +++++++++++++++++++++++ > > 2 files changed, 65 insertions(+), 21 deletions(-) > > create mode 100644 include/linux/soc/samsung/exynos-chipid.h > > > > diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c > > index bcf691f2b650..006a95feb618 100644 > > --- a/drivers/soc/samsung/exynos-chipid.c > > +++ b/drivers/soc/samsung/exynos-chipid.c > > @@ -9,16 +9,13 @@ > > */ > > > > #include <linux/io.h> > > +#include <linux/mfd/syscon.h> > > #include <linux/of.h> > > -#include <linux/of_address.h> > > +#include <linux/regmap.h> > > #include <linux/slab.h> > > +#include <linux/soc/samsung/exynos-chipid.h> > > #include <linux/sys_soc.h> > > > > -#define EXYNOS_SUBREV_MASK (0xF << 4) > > -#define EXYNOS_MAINREV_MASK (0xF << 0) > > -#define EXYNOS_REV_MASK (EXYNOS_SUBREV_MASK | EXYNOS_MAINREV_MASK) > > -#define EXYNOS_MASK 0xFFFFF000 > > - > > static const struct exynos_soc_id { > > const char *name; > > unsigned int id; > > @@ -51,29 +48,24 @@ static const char * __init product_id_to_soc_id(unsigned int product_id) > > int __init exynos_chipid_early_init(void) > > { > > struct soc_device_attribute *soc_dev_attr; > > - void __iomem *exynos_chipid_base; > > struct soc_device *soc_dev; > > struct device_node *root; > > - struct device_node *np; > > + struct regmap *regmap; > > u32 product_id; > > u32 revision; > > + int ret; > > > > - /* look up for chipid node */ > > - np = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-chipid"); > > - if (!np) > > - return -ENODEV; > > - > > - exynos_chipid_base = of_iomap(np, 0); > > - of_node_put(np); > > - > > - if (!exynos_chipid_base) { > > - pr_err("Failed to map SoC chipid\n"); > > - return -ENXIO; > > + regmap = syscon_regmap_lookup_by_compatible("samsung,exynos4210-chipid"); > > + if (IS_ERR(regmap)) { > > + pr_err("Failed to get CHIPID regmap\n"); > > + return PTR_ERR(regmap); > > } > > Following this change, I am now seeing the above error on our Tegra > boards where this driver is enabled. This is triggering a kernel > warnings test we have to fail. Hence, I don't think that you can remove > the compatible node test here, unless you have a better way to determine > if this is a samsung device. Right, this is really wrong... I missed that it is not a probe but early init. And this init will be called on every board... Probably it should be converted to a regular driver. This is very old patchset, revived recently. I see that in v6 it was a platform driver: https://patchwork.kernel.org/patch/9134949/ Pankaj, apparently based on these comments, made it initcall... but why? Another point is that Arnd complained there about exposing global header and no change here - we still expose the global header, but not with soc revisions but register internals... although it has its purpose - other Exynos-specific drivers need to access through regmap. Best regards, Krzysztof
On 8/20/19 21:37, Krzysztof Kozlowski wrote: >>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c >>> @@ -51,29 +48,24 @@ static const char * __init product_id_to_soc_id(unsigned int product_id) >>> int __init exynos_chipid_early_init(void) >>> { >>> struct soc_device_attribute *soc_dev_attr; >>> - void __iomem *exynos_chipid_base; >>> struct soc_device *soc_dev; >>> struct device_node *root; >>> - struct device_node *np; >>> + struct regmap *regmap; >>> u32 product_id; >>> u32 revision; >>> + int ret; >>> >>> - /* look up for chipid node */ >>> - np = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-chipid"); >>> - if (!np) >>> - return -ENODEV; >>> - >>> - exynos_chipid_base = of_iomap(np, 0); >>> - of_node_put(np); >>> - >>> - if (!exynos_chipid_base) { >>> - pr_err("Failed to map SoC chipid\n"); >>> - return -ENXIO; >>> + regmap = syscon_regmap_lookup_by_compatible("samsung,exynos4210-chipid"); >>> + if (IS_ERR(regmap)) { >>> + pr_err("Failed to get CHIPID regmap\n"); >>> + return PTR_ERR(regmap); >>> } >> Following this change, I am now seeing the above error on our Tegra >> boards where this driver is enabled. This is triggering a kernel >> warnings test we have to fail. Hence, I don't think that you can remove >> the compatible node test here, unless you have a better way to determine >> if this is a samsung device. > > Right, this is really wrong... I missed that it is not a probe but > early init. And this init will be called on every board... Probably it > should be converted to a regular driver. I'm also inclined to have it converted to a regular driver. We already have "exynos-asv" driver matching on the chipid node (patch 3/9). The ASV patches will not be merged soon anyway, all this needs some more thought. Krzysztof, can we abandon the chipid patches for now? Your pull request doesn't appear to be merged to arm-soc yet. Sorry about that. -- Regards, Sylwester
On Tue, 20 Aug 2019 at 23:38, Sylwester Nawrocki <snawrocki@kernel.org> wrote: > > On 8/20/19 21:37, Krzysztof Kozlowski wrote: > >>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c > > >>> @@ -51,29 +48,24 @@ static const char * __init product_id_to_soc_id(unsigned int product_id) > >>> int __init exynos_chipid_early_init(void) > >>> { > >>> struct soc_device_attribute *soc_dev_attr; > >>> - void __iomem *exynos_chipid_base; > >>> struct soc_device *soc_dev; > >>> struct device_node *root; > >>> - struct device_node *np; > >>> + struct regmap *regmap; > >>> u32 product_id; > >>> u32 revision; > >>> + int ret; > >>> > >>> - /* look up for chipid node */ > >>> - np = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-chipid"); > >>> - if (!np) > >>> - return -ENODEV; > >>> - > >>> - exynos_chipid_base = of_iomap(np, 0); > >>> - of_node_put(np); > >>> - > >>> - if (!exynos_chipid_base) { > >>> - pr_err("Failed to map SoC chipid\n"); > >>> - return -ENXIO; > >>> + regmap = syscon_regmap_lookup_by_compatible("samsung,exynos4210-chipid"); > >>> + if (IS_ERR(regmap)) { > >>> + pr_err("Failed to get CHIPID regmap\n"); > >>> + return PTR_ERR(regmap); > >>> } > >> Following this change, I am now seeing the above error on our Tegra > >> boards where this driver is enabled. This is triggering a kernel > >> warnings test we have to fail. Hence, I don't think that you can remove > >> the compatible node test here, unless you have a better way to determine > >> if this is a samsung device. > > > > Right, this is really wrong... I missed that it is not a probe but > > early init. And this init will be called on every board... Probably it > > should be converted to a regular driver. > > I'm also inclined to have it converted to a regular driver. We already > have "exynos-asv" driver matching on the chipid node (patch 3/9). > The ASV patches will not be merged soon anyway, all this needs some more > thought. Krzysztof, can we abandon the chipid patches for now? Your > pull request doesn't appear to be merged to arm-soc yet. Sorry about > that. Yes, let's abandon the pull request and rework the concept. Best regards, Krzysztof
Hi, On 8/20/19 11:38 PM, Sylwester Nawrocki wrote: > On 8/20/19 21:37, Krzysztof Kozlowski wrote: >>>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c > >>>> @@ -51,29 +48,24 @@ static const char * __init product_id_to_soc_id(unsigned int product_id) >>>> int __init exynos_chipid_early_init(void) >>>> { >>>> struct soc_device_attribute *soc_dev_attr; >>>> - void __iomem *exynos_chipid_base; >>>> struct soc_device *soc_dev; >>>> struct device_node *root; >>>> - struct device_node *np; >>>> + struct regmap *regmap; >>>> u32 product_id; >>>> u32 revision; >>>> + int ret; >>>> >>>> - /* look up for chipid node */ >>>> - np = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-chipid"); >>>> - if (!np) >>>> - return -ENODEV; >>>> - >>>> - exynos_chipid_base = of_iomap(np, 0); >>>> - of_node_put(np); >>>> - >>>> - if (!exynos_chipid_base) { >>>> - pr_err("Failed to map SoC chipid\n"); >>>> - return -ENXIO; >>>> + regmap = syscon_regmap_lookup_by_compatible("samsung,exynos4210-chipid"); >>>> + if (IS_ERR(regmap)) { >>>> + pr_err("Failed to get CHIPID regmap\n"); >>>> + return PTR_ERR(regmap); >>>> } >>> Following this change, I am now seeing the above error on our Tegra >>> boards where this driver is enabled. This is triggering a kernel >>> warnings test we have to fail. Hence, I don't think that you can remove >>> the compatible node test here, unless you have a better way to determine >>> if this is a samsung device. >> >> Right, this is really wrong... I missed that it is not a probe but >> early init. And this init will be called on every board... Probably it >> should be converted to a regular driver. Early initialization is needed for SoC driver to be used from within arch/arm/mach-exynos/ and _initcall() usage is the usual way for SoC drivers to be initialized: drivers/soc/amlogic/meson-gx-socinfo.c drivers/soc/amlogic/meson-mx-socinfo.c drivers/soc/atmel/soc.c drivers/soc/bcm/brcmstb/common.c drivers/soc/imx/soc-imx-scu.c drivers/soc/imx/soc-imx8.c drivers/soc/renesas/renesas-soc.c drivers/soc/tegra/fuse/fuse-tegra.c drivers/soc/ux500/ux500-soc-id.c drivers/soc/versatile/soc-integrator.c drivers/soc/versatile/soc-integrator.c The only SoC drivers that are regular drivers are: drivers/soc/fsl/guts.c drivers/soc/versatile/soc-realview.c > I'm also inclined to have it converted to a regular driver. We already > have "exynos-asv" driver matching on the chipid node (patch 3/9). > The ASV patches will not be merged soon anyway, all this needs some more > thought. Krzysztof, can we abandon the chipid patches for now? Your chipid driver is good and useful on its own. The preferred solution IMHO would be to just revert "soc: samsung: Convert exynos-chipid driver to use the regmap API" commit. > pull request doesn't appear to be merged to arm-soc yet. Sorry about > that. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Wed, 21 Aug 2019 at 13:51, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > >>> Following this change, I am now seeing the above error on our Tegra > >>> boards where this driver is enabled. This is triggering a kernel > >>> warnings test we have to fail. Hence, I don't think that you can remove > >>> the compatible node test here, unless you have a better way to determine > >>> if this is a samsung device. > >> > >> Right, this is really wrong... I missed that it is not a probe but > >> early init. And this init will be called on every board... Probably it > >> should be converted to a regular driver. > > Early initialization is needed for SoC driver to be used from within > arch/arm/mach-exynos/ and _initcall() usage is the usual way for SoC > drivers to be initialized: > > drivers/soc/amlogic/meson-gx-socinfo.c > drivers/soc/amlogic/meson-mx-socinfo.c > drivers/soc/atmel/soc.c > drivers/soc/bcm/brcmstb/common.c > drivers/soc/imx/soc-imx-scu.c > drivers/soc/imx/soc-imx8.c > drivers/soc/renesas/renesas-soc.c > drivers/soc/tegra/fuse/fuse-tegra.c > drivers/soc/ux500/ux500-soc-id.c > drivers/soc/versatile/soc-integrator.c > drivers/soc/versatile/soc-integrator.c > > The only SoC drivers that are regular drivers are: > > drivers/soc/fsl/guts.c > drivers/soc/versatile/soc-realview.c Thanks for pointing it out. Indeed, the initcall was needed in your set of patches here: https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=43565&state=* but this work was not continued here. Maybe it will be later resubmitted... maybe not... who knows? Therefore I would prefer proper solution for current case (driver), unless patches for mach are being brought back to life now. > > I'm also inclined to have it converted to a regular driver. We already > > have "exynos-asv" driver matching on the chipid node (patch 3/9). > > The ASV patches will not be merged soon anyway, all this needs some more > > thought. Krzysztof, can we abandon the chipid patches for now? Your > > chipid driver is good and useful on its own. The preferred solution > IMHO would be to just revert "soc: samsung: Convert exynos-chipid > driver to use the regmap API" commit. I queued the chipid as a dependency for ASV but ASV requires the regmap. What would be left after reverting the regmap part? Simple unused printk driver? No need for such. If reverting, then let's drop entire driver and rework it offline. Best regards, Krzysztof
On 8/21/19 14:16, Krzysztof Kozlowski wrote: >>> I'm also inclined to have it converted to a regular driver. We already >>> have "exynos-asv" driver matching on the chipid node (patch 3/9). >>> The ASV patches will not be merged soon anyway, all this needs some more >>> thought. Krzysztof, can we abandon the chipid patches for now? Your >> >> chipid driver is good and useful on its own. The preferred solution >> IMHO would be to just revert "soc: samsung: Convert exynos-chipid >> driver to use the regmap API" commit. > > I queued the chipid as a dependency for ASV but ASV requires the > regmap. What would be left after reverting the regmap part? Simple > unused printk driver? No need for such. If reverting, then let's drop > entire driver and rework it offline. In fact there is now no dependency between the chipid and the ASV driver (patch 3/9), the regmap is provided by the syscon driver/API. I should have added "depends on REGMAP && MFD_SYSCON" to Kconfig. Both drivers (chipid, ASV) share the registers region so the regmap API seemed appropriate here. Converting the chipid code to platform driver wouldn't make sense as it wouldn't be useful early in arch/arm/mach-exynos and we can't have two drivers for same device (the ASV driver matches on the chipid compatible now).
On Wed, 21 Aug 2019 at 14:41, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > > On 8/21/19 14:16, Krzysztof Kozlowski wrote: > >>> I'm also inclined to have it converted to a regular driver. We already > >>> have "exynos-asv" driver matching on the chipid node (patch 3/9). > >>> The ASV patches will not be merged soon anyway, all this needs some more > >>> thought. Krzysztof, can we abandon the chipid patches for now? Your > >> > >> chipid driver is good and useful on its own. The preferred solution > >> IMHO would be to just revert "soc: samsung: Convert exynos-chipid > >> driver to use the regmap API" commit. > > > > I queued the chipid as a dependency for ASV but ASV requires the > > regmap. What would be left after reverting the regmap part? Simple > > unused printk driver? No need for such. If reverting, then let's drop > > entire driver and rework it offline. > > In fact there is now no dependency between the chipid and the ASV > driver (patch 3/9), the regmap is provided by the syscon driver/API. > I should have added "depends on REGMAP && MFD_SYSCON" to Kconfig. > Both drivers (chipid, ASV) share the registers region so the regmap > API seemed appropriate here. Indeed, ASV needs only the header + DT change... Then actually we do not need chipid driver at all. Just to print the SoC and provide sysfs entry? If this is the only purpose, then it should be a driver. > Converting the chipid code to platform driver wouldn't make sense as > it wouldn't be useful early in arch/arm/mach-exynos and we can't have > two drivers for same device (the ASV driver matches on the chipid > compatible now). There is no use case for arm/mach-exynos. This code was not resubmitted and I doubt it will be (unless now someone wants to prove I am wrong and sends it again :) ). The two-device case is indeed a problem but it is possible. Clocks are doing it with PMU driver. See CLK_OF_DECLARE_DRIVER(), although I do not remember whether it is maybe obsolete pattern (discouraged). Best regards, Krzysztof