Message-ID: <573E0704.7010104@ti.com>
Date:	Thu, 19 May 2016 13:33:40 -0500
From:	Dave Gerlach <d-gerlach@...com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
CC:	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-omap@...r.kernel.org>, <linux-pm@...r.kernel.org>,
	<devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>,
	"Rafael J . Wysocki" <rjw@...ysocki.net>,
	Tony Lindgren <tony@...mide.com>,
	Mark Rutland <mark.rutland@....com>,
	Nishanth Menon <nm@...com>,
	Yegor Yefremov <yegorslists@...glemail.com>
Subject: Re: [PATCH 2/2] cpufreq: ti: Add cpufreq driver to determine
 available OPPs at runtime

On 05/18/2016 11:39 PM, Viresh Kumar wrote:
> On 18-05-16, 18:30, Dave Gerlach wrote:
>> Some TI SoCs, like those in the AM335x, AM437x, DRA7x, and AM57x families,
>> have different OPPs available for the MPU depending on which specific
>> variant of the SoC is in use. This can be determined through use of the
>> revision and an eFuse register present in the silicon. Introduce a
>> ti-cpufreq driver that can read the aformentioned values and provide
>> them as version matching data to the opp framework. Through this the
>> opp-supported-hw dt binding that is part of the operating-points-v2
>> table can be used to indicate availability of OPPs for each device.
>>
>> This driver also creates the "cpufreq-dt" platform_device after passing
>> the version matching data to the OPP framework so that the cpufreq-dt
>> handles the actual cpufreq implementation. Even without the necessary
>> data to pass the version matching data the driver will still create this
>> device to maintain backwards compatibility with operating-points v1
>> tables.
>>
>> Signed-off-by: Dave Gerlach <d-gerlach@...com>
>> ---
>>   drivers/cpufreq/Kconfig.arm  |  11 ++
>>   drivers/cpufreq/Makefile     |   1 +
>>   drivers/cpufreq/ti-cpufreq.c | 254 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 266 insertions(+)
>>   create mode 100644 drivers/cpufreq/ti-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index d89b8afe23b6..0dea6849ac3e 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -234,6 +234,17 @@ config ARM_TEGRA124_CPUFREQ
>>   	help
>>   	  This adds the CPUFreq driver support for Tegra124 SOCs.
>>
>> +config ARM_TI_CPUFREQ
>> +	tristate "Texas Instruments CPUFreq support"
>
> You sure you want to get it compiled as a module? And don't provide
> module_exit() at all?

I thought about this for a while, and I was going to make it bool but 
decided to model it after what has already been accepted (sti-cpufreq). 
Wasn't sure what preferred path is, but I will go ahead and switch this 
to bool for this driver as I think that makes the most sense.

>
>> +	depends on ARCH_OMAP2PLUS
>> +	help
>> +	  This driver enables valid OPPs on the running platform based on
>> +	  values contained within the SoC in use. Enable this in order to
>> +	  use the cpufreq-dt driver on all Texas Instruments platforms that
>> +	  provide dt based operating-points-v2 tables with opp-supported-hw
>> +	  data provided. Required for cpufreq support on AM335x, AM437x,
>> +	  DRA7x, and AM57x platforms.
>> +
>>   config ARM_PXA2xx_CPUFREQ
>>   	tristate "Intel PXA2xx CPUfreq driver"
>>   	depends on PXA27x || PXA25x
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index 0a9b6a093646..5b1b6ec0a9f0 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>>   obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>> +obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>>   obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
>>   obj-$(CONFIG_MACH_MVEBU_V7)		+= mvebu-cpufreq.o
>> diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
>> new file mode 100644
>> index 000000000000..e47b452aadd0
>> --- /dev/null
>> +++ b/drivers/cpufreq/ti-cpufreq.c
>> @@ -0,0 +1,254 @@
>> +/*
>> + * TI CPUFreq/OPP hw-supported driver
>> + *
>> + * Copyright (C) 2016 Texas Instruments, Inc.
>> + *	 Dave Gerlach <d-gerlach@...com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/regmap.h>
>> +
>> +#define REVISION_MASK				(0xF << 28)
>
> Use below shift-mask here ?

Yes, but this will change due to later change where this will actually 
be used.

>
>> +#define REVISION_SHIFT				28
>> +
>> +#define DRA7_EFUSE_HAS_OD_MPU_OPP		11
>> +#define DRA7_EFUSE_HAS_HIGH_MPU_OPP		15
>> +#define DRA7_EFUSE_HAS_ALL_MPU_OPP		23
>> +
>> +#define DRA7_EFUSE_NOM_MPU_OPP			BIT(0)
>> +#define DRA7_EFUSE_OD_MPU_OPP			BIT(1)
>> +#define DRA7_EFUSE_HIGH_MPU_OPP			BIT(2)
>> +
>> +#define VERSION_COUNT				2
>> +
>> +static struct ti_cpufreq_data {
>> +	struct device *cpu;
>> +	struct regmap *opp_efuse;
>> +	struct regmap *revision;
>> +} opp_data;
>> +
>> +static struct ti_cpufreq_soc_data {
>> +	unsigned long (*efuse_xlate)(unsigned long efuse);
>> +} *soc_data;
>> +
>> +static unsigned long amx3_efuse_xlate(unsigned long efuse)
>> +{
>> +	/* AM335x and AM437x use "OPP disable" bits, so invert */
>> +	return ~efuse;
>> +}
>> +
>> +static unsigned long dra7_efuse_xlate(unsigned long efuse)
>> +{
>> +	unsigned long calculated_efuse = DRA7_EFUSE_NOM_MPU_OPP;
>> +
>> +	/*
>> +	 * The efuse on dra7 and am57 parts contains a specific
>> +	 * value indicating the highest available OPP.
>> +	 */
>> +
>> +	switch (efuse) {
>> +	case DRA7_EFUSE_HAS_ALL_MPU_OPP:
>> +	case DRA7_EFUSE_HAS_HIGH_MPU_OPP:
>> +		calculated_efuse |= DRA7_EFUSE_HIGH_MPU_OPP;
>> +	case DRA7_EFUSE_HAS_OD_MPU_OPP:
>> +		calculated_efuse |= DRA7_EFUSE_OD_MPU_OPP;
>> +	}
>> +
>> +	return calculated_efuse;
>> +}
>> +
>> +static struct ti_cpufreq_soc_data amx3_soc_data = {
>> +	.efuse_xlate = amx3_efuse_xlate,
>> +};
>> +
>> +static struct ti_cpufreq_soc_data dra7_soc_data = {
>> +	.efuse_xlate = dra7_efuse_xlate,
>> +};
>> +
>> +/**
>> + * ti_cpufreq_get_efuse() - Parse and return efuse value present on SoC
>> + * @efuse_value: Set to the value parsed from efuse
>> + *
>> + * Returns error code if efuse not read properly.
>> + */
>> +static int ti_cpufreq_get_efuse(u32 *efuse_value)
>> +{
>> +	struct device *dev = opp_data.cpu;
>> +	struct device_node *np = dev->of_node;
>> +	unsigned int efuse_offset;
>> +	u32 efuse, efuse_mask, efuse_shift;
>> +	int ret;
>> +
>> +	ret = of_property_read_u32_index(np, "ti,syscon-efuse",
>> +					 1, &efuse_offset);
>> +	if (ret) {
>> +		dev_err(dev,
>
> Line break here isn't required.

Ok thanks for catching this.

>
>> +			"No efuse offset provided %s: %d\n",
>> +			np->full_name, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32_index(np, "ti,syscon-efuse", 2,
>> +					 &efuse_mask);
>> +	if (ret)
>> +		efuse_mask = 0xffffffff;
>> +
>> +	ret = of_property_read_u32_index(np, "ti,syscon-efuse", 3,
>> +					 &efuse_shift);
>> +	if (ret)
>> +		efuse_shift = 0;
>
> Why don't you read an array of 3 integers in one go?

To be honest, didn't know I could, will update to use 
of_property_read_u32_array.

>
>> +
>> +	ret = regmap_read(opp_data.opp_efuse, efuse_offset, &efuse);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to read the efuse value from syscon: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	efuse = (efuse & efuse_mask) >> efuse_shift;
>> +
>> +	*efuse_value = soc_data->efuse_xlate(efuse);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ti_cpufreq_get_rev() - Parse and return rev value present on SoC
>> + * @revision_value: Set to the value parsed from revision register
>> + *
>> + * Returns error code if revision not read properly.
>> + */
>> +static int ti_cpufreq_get_rev(u32 *revision_value)
>> +{
>> +	struct device *dev = opp_data.cpu;
>> +	struct device_node *np = dev->of_node;
>> +	unsigned int revision_offset;
>> +	u32 revision;
>> +	int ret;
>> +
>> +	ret = of_property_read_u32_index(np, "ti,syscon-rev",
>> +					 1, &revision_offset);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"No revision offset provided %s [%d]\n",
>> +			np->full_name, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regmap_read(opp_data.revision, revision_offset, &revision);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to read the revision number from syscon: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	*revision_value = BIT((revision & REVISION_MASK) >> REVISION_SHIFT);
>
> That's an crazy operation.
>
> So you first shifted 0xF << 27, and then right shifted everything by 27 bits :)
>
> You should rather do:
>
> #define REVISION_MASK   0xF
> (revision >> REVISION_SHIFT) & REVISION_MASK

Yeah that's much cleaner I can update.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_cpufreq_setup_syscon_registers(void)
>> +{
>> +	struct device *dev = opp_data.cpu;
>> +	struct device_node *np = dev->of_node;
>> +
>> +	opp_data.opp_efuse = syscon_regmap_lookup_by_phandle(np,
>> +							"ti,syscon-efuse");
>> +	if (IS_ERR(opp_data.opp_efuse)) {
>> +		dev_dbg(dev,  "\"ti,syscon-efuse\" is missing, cannot use OPPv2 table.\n");
>> +		return PTR_ERR(opp_data.opp_efuse);
>> +	}
>> +
>> +	opp_data.revision = syscon_regmap_lookup_by_phandle(np,
>> +							"ti,syscon-rev");
>> +	if (IS_ERR(opp_data.revision)) {
>> +		dev_dbg(dev,  "\"ti,syscon-rev\" is missing, cannot use OPPv2 table.\n");
>
> These messages are wrong as your code is going to use opp-v2 anyway.

Yes you are right for the case when these properties aren't provided 
isn't provided but opp-v2 table is, I was thinking more of when no 
properties are provided but opp-v1 table has been provided, like on 
legacy DT files. I will need to consider both for this message.

>
>> +		return PTR_ERR(opp_data.revision);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct ti_cpufreq_soc_data *ti_cpufreq_get_soc_data(void)
>> +{
>> +	if (of_machine_is_compatible("ti,am33xx") ||
>> +	    of_machine_is_compatible("ti,am4372"))
>> +		return &amx3_soc_data;
>> +	else if (of_machine_is_compatible("ti,dra7"))
>> +		return &dra7_soc_data;
>> +	else
>> +		return NULL;
>> +}
>> +
>> +static int ti_cpufreq_init(void)
>
> __init ?

Whoops, left that out.

>
>> +{
>> +	int ret;
>> +	u32 version[VERSION_COUNT];
>> +
>> +	soc_data = ti_cpufreq_get_soc_data();
>> +	if (!soc_data)
>> +		return -ENODEV;
>
> Why not use of_match_node() and an array of type struct of_device_id instead of
> above function?

Interesting idea, I like it and will do that.

Thanks for your comments.

Regards,
Dave

>
>> +
>> +	opp_data.cpu = get_cpu_device(0);
>> +	if (!opp_data.cpu) {
>> +		pr_err("%s: Failed to get device for CPU0\n", __func__);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!of_get_property(opp_data.cpu->of_node, "operating-points-v2",
>> +			     NULL)) {
>> +		dev_info(opp_data.cpu, "OPP-v2 not supported, cpufreq-dt will attempt to use legacy tables.\n");
>> +		goto register_cpufreq_dt;
>> +	}
>> +
>> +	ret = ti_cpufreq_setup_syscon_registers();
>> +	if (ret)
>> +		goto register_cpufreq_dt;
>> +
>> +	/*
>> +	 * OPPs determine whether or not they are supported based on
>> +	 * two metrics:
>> +	 *	0 - SoC Revision
>> +	 *	1 - eFuse value
>> +	 */
>> +	ret = ti_cpufreq_get_rev(&version[0]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ti_cpufreq_get_efuse(&version[1]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dev_pm_opp_set_supported_hw(opp_data.cpu, version, VERSION_COUNT);
>> +	if (ret) {
>> +		dev_err(opp_data.cpu, "Failed to set supported hardware\n");
>> +		return ret;
>> +	}
>> +
>> +register_cpufreq_dt:
>> +	platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
>> +
>> +	return 0;
>> +}
>> +module_init(ti_cpufreq_init);
>> +
>> +MODULE_DESCRIPTION("TI CPUFreq/OPP hw-supported driver");
>> +MODULE_AUTHOR("Dave Gerlach <d-gerlach@...com>");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.7.3
>

Powered by blists - more mailing lists