mbox series

[0/3] arm64: am62: Use nvmem for chip information in opp table

Message ID 20240206145721.2418893-1-msp@baylibre.com
Headers show
Series arm64: am62: Use nvmem for chip information in opp table | expand

Message

Markus Schneider-Pargmann Feb. 6, 2024, 2:57 p.m. UTC 
Hi everyone,

the OPP table on am625 currently uses a syscon node to get required
information from efuse registers. As efuse registers contain many
different information, this series adds nvmem support for the TI OPP
table and cpufreq driver. This way just the specific information can be
referenced in the devicetree without the need to use a syscon reference.

The nvmem layout is added in my previous series, links are below.

This series is based on
  https://lore.kernel.org/linux-arm-kernel/20240206143711.2410135-1-msp@baylibre.com/
Which is also available on my public git:
  https://gitlab.baylibre.com/msp8/linux/-/tree/topic/ti-chipid-nvmem/v6.8?ref_type=heads

This series is available on git as well:
  https://gitlab.baylibre.com/msp8/linux/-/tree/topic/ti-cpufreq-nvmem/v6.8?ref_type=heads

Best,
Markus

Markus Schneider-Pargmann (3):
  dt-bindings: cpufreq: Add nvmem-cells for chip information
  cpufreq: ti-cpufreq: Support nvmem for chip version
  arm64: dts: ti: k3-am625: Use nvmem-cells for opp

 .../opp/operating-points-v2-ti-cpu.yaml       |  16 ++-
 arch/arm64/boot/dts/ti/k3-am625.dtsi          |   2 +
 drivers/cpufreq/ti-cpufreq.c                  | 105 +++++++++++-------
 3 files changed, 83 insertions(+), 40 deletions(-)

Comments

Krzysztof Kozlowski Feb. 15, 2024, 8:24 a.m. UTC | #1 
On 06/02/2024 15:57, Markus Schneider-Pargmann wrote:
> Use nvmem cells referring to chip variant and speed grade for the
> operating points.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> ---
>  arch/arm64/boot/dts/ti/k3-am625.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am625.dtsi b/arch/arm64/boot/dts/ti/k3-am625.dtsi
> index 4193c2b3eed6..d60e1be9eb89 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am625.dtsi
> @@ -105,6 +105,8 @@ a53_opp_table: opp-table {
>  		compatible = "operating-points-v2-ti-cpu";
>  		opp-shared;
>  		syscon = <&wkup_conf>;
> +		nvmem-cells = <&chip_variant>, <&chip_speed>;
> +		nvmem-cell-names = "chipvariant", "chipspeed";

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof
Dhruva Gole Feb. 15, 2024, 9:13 a.m. UTC | #2 
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>