diff mbox series

[2/3] cpufreq: ti-cpufreq: Support nvmem for chip version

Message ID 20240206145721.2418893-3-msp@baylibre.com (mailing list archive)
State New, archived
Headers show
Series arm64: am62: Use nvmem for chip information in opp table | expand

Commit Message

Markus Schneider-Pargmann Feb. 6, 2024, 2:57 p.m. UTC 
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

Dhruva Gole Feb. 15, 2024, 9:13 a.m. UTC | #1 
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>
diff mbox series

Patch

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;
 	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