[2/3] cpufreq: ti-cpufreq: Support nvmem for chip version
Commit Message
Support using nvmem-cells 'chipvariant' and 'chipspeed' instead of
syscon. This makes it more flexible and moves more configuration into
the devicetree.
If nvmem-cells are present, probing will fail if the configuration of
these cells is broken. If nvmem-cells is not present syscon will be
used.
Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---
drivers/cpufreq/ti-cpufreq.c | 105 ++++++++++++++++++++++-------------
1 file changed, 66 insertions(+), 39 deletions(-)
Comments
Hi,
On Feb 06, 2024 at 15:57:20 +0100, Markus Schneider-Pargmann wrote:
> Support using nvmem-cells 'chipvariant' and 'chipspeed' instead of
> syscon. This makes it more flexible and moves more configuration into
> the devicetree.
>
> If nvmem-cells are present, probing will fail if the configuration of
> these cells is broken. If nvmem-cells is not present syscon will be
> used.
>
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
> drivers/cpufreq/ti-cpufreq.c | 105 ++++++++++++++++++++++-------------
> 1 file changed, 66 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
> index 46c41e2ca727..3ee72b1309f0 100644
> --- a/drivers/cpufreq/ti-cpufreq.c
> +++ b/drivers/cpufreq/ti-cpufreq.c
> @@ -10,6 +10,7 @@
> #include <linux/io.h>
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> #include <linux/init.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> @@ -65,6 +66,7 @@ struct ti_cpufreq_soc_data {
>
> struct ti_cpufreq_data {
> struct device *cpu_dev;
> + struct device *dev;
> struct device_node *opp_node;
> struct regmap *syscon;
> const struct ti_cpufreq_soc_data *soc_data;
> @@ -244,31 +246,40 @@ static struct ti_cpufreq_soc_data am625_soc_data = {
> static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
> u32 *efuse_value)
> {
> + struct device_node *np = opp_data->opp_node;
Umm.. slightly confused, where is *np being used?
> struct device *dev = opp_data->cpu_dev;
> u32 efuse;
> int ret;
>
> - ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
> - &efuse);
> - if (ret == -EIO) {
> - /* not a syscon register! */
> - void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> - opp_data->soc_data->efuse_offset, 4);
> -
> - if (!regs)
> - return -ENOMEM;
> - efuse = readl(regs);
> - iounmap(regs);
> + ret = nvmem_cell_read_u32(opp_data->dev, "chipspeed", &efuse);
> + if (ret && (ret != -ENOENT || !opp_data->syscon))
> + return dev_err_probe(dev, ret,
> + "Failed to read nvmem cell 'chipspeed': %pe",
> + ERR_PTR(ret));
> +
> + if (ret) {
> + ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
> + &efuse);
> + if (ret == -EIO) {
> + /* not a syscon register! */
> + void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> + opp_data->soc_data->efuse_offset, 4);
> +
> + if (!regs)
> + return -ENOMEM;
> + efuse = readl(regs);
> + iounmap(regs);
> + }
> + else if (ret) {
else should be enough I guess, no need of elif?
> + dev_err(dev,
> + "Failed to read the efuse value from syscon: %d\n",
> + ret);
> + return ret;
> }
> - else if (ret) {
> - dev_err(dev,
> - "Failed to read the efuse value from syscon: %d\n",
> - ret);
> - return ret;
> - }
>
> - efuse = (efuse & opp_data->soc_data->efuse_mask);
> - efuse >>= opp_data->soc_data->efuse_shift;
> + efuse = (efuse & opp_data->soc_data->efuse_mask);
> + efuse >>= opp_data->soc_data->efuse_shift;
> + }
>
> *efuse_value = opp_data->soc_data->efuse_xlate(opp_data, efuse);
>
> @@ -285,30 +296,41 @@ static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
> static int ti_cpufreq_get_rev(struct ti_cpufreq_data *opp_data,
> u32 *revision_value)
> {
> + struct device_node *np = opp_data->opp_node;
where is this used? Atleast, in this patch I don't see it...
> struct device *dev = opp_data->cpu_dev;
> u32 revision;
> int ret;
>
> - ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
> - &revision);
> - if (ret == -EIO) {
> - /* not a syscon register! */
> - void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> - opp_data->soc_data->rev_offset, 4);
> -
> - if (!regs)
> - return -ENOMEM;
> - revision = readl(regs);
> - iounmap(regs);
> + ret = nvmem_cell_read_u32(opp_data->dev, "chipvariant", &revision);
> + if (ret && (ret != -ENOENT || !opp_data->syscon))
> + return dev_err_probe(dev, ret,
> + "Failed to read nvmem cell 'chipvariant': %pe",
> + ERR_PTR(ret));
> +
> + if (ret) {
> + ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
> + &revision);
> + if (ret == -EIO) {
> + /* not a syscon register! */
> + void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> + opp_data->soc_data->rev_offset, 4);
> +
> + if (!regs)
> + return -ENOMEM;
> + revision = readl(regs);
> + iounmap(regs);
> + }
> + else if (ret) {
Do you really have to? This code will reach only if(ret) is satisfied,
the elif feels redundant. Else should be fine
> + dev_err(dev,
> + "Failed to read the revision number from syscon: %d\n",
> + ret);
> + return ret;
> }
> - else if (ret) {
> - dev_err(dev,
> - "Failed to read the revision number from syscon: %d\n",
> - ret);
> - return ret;
> +
> + revision = (revision >> REVISION_SHIFT) & REVISION_MASK;
> }
>
> - *revision_value = BIT((revision >> REVISION_SHIFT) & REVISION_MASK);
> + *revision_value = BIT(revision);
>
> return 0;
> }
> @@ -392,9 +414,14 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
> goto register_cpufreq_dt;
> }
>
> - ret = ti_cpufreq_setup_syscon_register(opp_data);
> - if (ret)
> - goto fail_put_node;
> + opp_data->dev = &pdev->dev;
> + opp_data->dev->of_node = opp_data->opp_node;
> +
> + if (!of_property_read_bool(opp_data->opp_node, "nvmem-cells")) {
> + ret = ti_cpufreq_setup_syscon_register(opp_data);
> + if (ret)
> + goto fail_put_node;
> + }
Mostly looks okay, with above comments addressed:
Reviewed-by: Dhruva Gole <d-gole@ti.com>
@@ -10,6 +10,7 @@
#include <linux/io.h>
#include <linux/mfd/syscon.h>
#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
#include <linux/init.h>
#include <linux/of.h>
#include <linux/platform_device.h>
@@ -65,6 +66,7 @@ struct ti_cpufreq_soc_data {
struct ti_cpufreq_data {
struct device *cpu_dev;
+ struct device *dev;
struct device_node *opp_node;
struct regmap *syscon;
const struct ti_cpufreq_soc_data *soc_data;
@@ -244,31 +246,40 @@ static struct ti_cpufreq_soc_data am625_soc_data = {
static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
u32 *efuse_value)
{
+ struct device_node *np = opp_data->opp_node;
struct device *dev = opp_data->cpu_dev;
u32 efuse;
int ret;
- ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
- &efuse);
- if (ret == -EIO) {
- /* not a syscon register! */
- void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
- opp_data->soc_data->efuse_offset, 4);
-
- if (!regs)
- return -ENOMEM;
- efuse = readl(regs);
- iounmap(regs);
+ ret = nvmem_cell_read_u32(opp_data->dev, "chipspeed", &efuse);
+ if (ret && (ret != -ENOENT || !opp_data->syscon))
+ return dev_err_probe(dev, ret,
+ "Failed to read nvmem cell 'chipspeed': %pe",
+ ERR_PTR(ret));
+
+ if (ret) {
+ ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
+ &efuse);
+ if (ret == -EIO) {
+ /* not a syscon register! */
+ void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
+ opp_data->soc_data->efuse_offset, 4);
+
+ if (!regs)
+ return -ENOMEM;
+ efuse = readl(regs);
+ iounmap(regs);
+ }
+ else if (ret) {
+ dev_err(dev,
+ "Failed to read the efuse value from syscon: %d\n",
+ ret);
+ return ret;
}
- else if (ret) {
- dev_err(dev,
- "Failed to read the efuse value from syscon: %d\n",
- ret);
- return ret;
- }
- efuse = (efuse & opp_data->soc_data->efuse_mask);
- efuse >>= opp_data->soc_data->efuse_shift;
+ efuse = (efuse & opp_data->soc_data->efuse_mask);
+ efuse >>= opp_data->soc_data->efuse_shift;
+ }
*efuse_value = opp_data->soc_data->efuse_xlate(opp_data, efuse);
@@ -285,30 +296,41 @@ static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
static int ti_cpufreq_get_rev(struct ti_cpufreq_data *opp_data,
u32 *revision_value)
{
+ struct device_node *np = opp_data->opp_node;
struct device *dev = opp_data->cpu_dev;
u32 revision;
int ret;
- ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
- &revision);
- if (ret == -EIO) {
- /* not a syscon register! */
- void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
- opp_data->soc_data->rev_offset, 4);
-
- if (!regs)
- return -ENOMEM;
- revision = readl(regs);
- iounmap(regs);
+ ret = nvmem_cell_read_u32(opp_data->dev, "chipvariant", &revision);
+ if (ret && (ret != -ENOENT || !opp_data->syscon))
+ return dev_err_probe(dev, ret,
+ "Failed to read nvmem cell 'chipvariant': %pe",
+ ERR_PTR(ret));
+
+ if (ret) {
+ ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
+ &revision);
+ if (ret == -EIO) {
+ /* not a syscon register! */
+ void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
+ opp_data->soc_data->rev_offset, 4);
+
+ if (!regs)
+ return -ENOMEM;
+ revision = readl(regs);
+ iounmap(regs);
+ }
+ else if (ret) {
+ dev_err(dev,
+ "Failed to read the revision number from syscon: %d\n",
+ ret);
+ return ret;
}
- else if (ret) {
- dev_err(dev,
- "Failed to read the revision number from syscon: %d\n",
- ret);
- return ret;
+
+ revision = (revision >> REVISION_SHIFT) & REVISION_MASK;
}
- *revision_value = BIT((revision >> REVISION_SHIFT) & REVISION_MASK);
+ *revision_value = BIT(revision);
return 0;
}
@@ -392,9 +414,14 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
goto register_cpufreq_dt;
}
- ret = ti_cpufreq_setup_syscon_register(opp_data);
- if (ret)
- goto fail_put_node;
+ opp_data->dev = &pdev->dev;
+ opp_data->dev->of_node = opp_data->opp_node;
+
+ if (!of_property_read_bool(opp_data->opp_node, "nvmem-cells")) {
+ ret = ti_cpufreq_setup_syscon_register(opp_data);
+ if (ret)
+ goto fail_put_node;
+ }
/*
* OPPs determine whether or not they are supported based on