Re: [PATCH v5 3/4] soc: samsung: Add Exynos Adaptive Supply Voltage driver

From: Sylwester Nawrocki <s.nawrocki@samsung•com>
To: Krzysztof Kozlowski <krzk@kernel•org>
Cc: devicetree@vger•kernel.org, linux-samsung-soc@vger•kernel.org,
	b.zolnierkie@samsung•com, sboyd@kernel•org, vireshk@kernel•org,
	linux-pm@vger•kernel.org, roger.lu@mediatek•com,
	robh+dt@kernel•org, linux-arm-kernel@lists•infradead.org,
	m.szyprowski@samsung•com
Subject: Re: [PATCH v5 3/4] soc: samsung: Add Exynos Adaptive Supply Voltage driver
Date: Wed, 23 Oct 2019 12:48:34 +0200	[thread overview]
Message-ID: <ccd20b0e-9c4c-93cd-2e7e-40aef1bb336c@samsung.com> (raw)
In-Reply-To: <20191022190419.GB8305@kozik-lap>

Hi Krzysztof,

On 10/22/19 21:04, Krzysztof Kozlowski wrote:

> I wanted to apply this patch but spotted some unusual printk... 
> and then started looking for more...
> 
> Sparse complains:
> ../drivers/soc/samsung/exynos5422-asv.c:457:5: warning: symbol 
> 'exynos5422_asv_init' was not declared. Should it be static?

#include "exynos5422-asv.h" should be added to 
drivers/soc/samsung/exynos5422-asv.c.

>>  drivers/soc/samsung/Kconfig          |  10 +
>>  drivers/soc/samsung/Makefile         |   3 +
>>  drivers/soc/samsung/exynos-asv.c     | 179 ++++++++++
>>  drivers/soc/samsung/exynos-asv.h     |  82 +++++
>>  drivers/soc/samsung/exynos5422-asv.c | 509 +++++++++++++++++++++++++++
>>  drivers/soc/samsung/exynos5422-asv.h |  25 ++
>>  6 files changed, 808 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
>>

>> +++ b/drivers/soc/samsung/exynos-asv.c

>> +#include <linux/cpu.h>
>> +#include <linux/delay.h>
> 
> Do you use this header?

It can be removed now, after conversion to dev_pm_opp_adjust_voltage().

>> +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);
> 
> Since this binds to the same node as chipid, why do you need syscon for
> it? Why regular IO access does not work? Eventually, if this has to be
> regmap because of locking (does it?), then maybe simply
> device_node_to_regmap()?

We just need regmap available to any of the two drivers whichever needs it
first. device_node_to_regmap() sounds like a good idea. Then we could drop
"syscon" compatible from the binding.

>> +	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);
> 
> Is this going to scream for every Exynos matching the 4210-chipid?

Yes, it should really be dev_info() or removed entirely.

>> +		return -ENODEV;
>> +	}

>> +++ b/drivers/soc/samsung/exynos-asv.h

>> +enum {
>> +	EXYNOS_ASV_SUBSYS_ID_ARM,
>> +	EXYNOS_ASV_SUBSYS_ID_EGL = EXYNOS_ASV_SUBSYS_ID_ARM,
>> +	EXYNOS_ASV_SUBSYS_ID_KFC,
>> +	EXYNOS_ASV_SUBSYS_ID_INT,
>> +	EXYNOS_ASV_SUBSYS_ID_MIF,
>> +	EXYNOS_ASV_SUBSYS_ID_G3D,
>> +	EXYNOS_ASV_SUBSYS_ID_CAM,
>> +	EXYNOS_ASV_SUBSYS_ID_MAX
>> +};
> 
> I cannot find usage of it in generic part of ASV driver. What's the
> purpose? Isn't it specific to Exynos5422?

It was meant as generic enumeration of available subsystems, it's not
Exynos5422 specific. It could be moved to the exynos5422 part of the 
driver (limited to EXYNOS_ASV_SUBSYS_ID_ARM, EXYNOS_ASV_SUBSYS_ID_KFC)
until support for of ther subsystems than CPU clusters is added.
The CPUs are now matched by compatible.

>> +struct regmap;
>> +
>> +/* HPM, IDS values to select target group */

>> +struct exynos_asv_subsys {
>> +	struct exynos_asv *asv;
>> +	const char *cpu_dt_compat;
>> +	int id;
>> +	struct exynos_asv_table table;
>> +
>> +	unsigned int base_volt;
>> +	unsigned int offset_volt_h;
>> +	unsigned int offset_volt_l;
>> +};
>> +
>> +struct exynos_asv {
>> +	struct device *dev;
>> +	struct regmap *chipid_regmap;
>> +	struct exynos_asv_subsys subsys[2];
>> +
>> +	int (*opp_get_voltage)(struct exynos_asv_subsys *subs, int level,
>> +			       unsigned int voltage);
>> +	unsigned int group;
>> +	unsigned int table;
>> +
>> +	/* True if SG fields from PKG_ID register should be used */
>> +	bool use_sg;
>> +	/* ASV bin read from DT */
>> +	int of_bin;
>> +};
>> +
>> +static inline u32 __asv_get_table_entry(struct exynos_asv_table *table,
> 
> 'table' looks here like pointer to const.

Yes, const could be added here.

>> +					unsigned int row, unsigned int col)
>> +{
>> +	return table->buf[row * (table->num_cols) + col];
>> +}
>> +
>> +static inline u32 exynos_asv_opp_get_voltage(struct exynos_asv_subsys *subsys,
>> +					unsigned int level, unsigned int group)
>> +{
> 
> The same, for subsys.

Agreed.

>> +	return __asv_get_table_entry(&subsys->table, level, group + 1);
>> +}

>> +++ b/drivers/soc/samsung/exynos5422-asv.c

>> +#include <linux/bitrev.h>
>> +#include <linux/device.h>
> 
> Is it used?
> 
>> +#include <linux/errno.h>
>> +#include <linux/of.h>
> 
> The same?

Some might be not used now, I will check it again.

>> +#include <linux/regmap.h>
>> +#include <linux/soc/samsung/exynos-chipid.h>
>> +#include <linux/slab.h>
>> +
>> +#include "exynos-asv.h"

>> +static int exynos5422_asv_opp_get_voltage(struct exynos_asv_subsys *subsys,
>> +				int level, unsigned int volt)
> 
> subsys is not modified.

>> +static unsigned int exynos5422_asv_parse_table(struct exynos_asv *asv,
>> +				      unsigned int pkg_id)
>> +{
> 
> The same, for asv. Looks BTW unused...

Indeed the asv argument should be dropped now.

>> +	return (pkg_id >> EXYNOS5422_TABLE_OFFSET) & EXYNOS5422_TABLE_MASK;
>> +}

>> +int exynos5422_asv_init(struct exynos_asv *asv)
>> +{
>> +	struct exynos_asv_subsys *subsys;
>> +	unsigned int table_index;
>> +	unsigned int pkg_id;
>> +	bool bin2;
>> +
> 
> How about checking if asv != null? It's a header exposed function.

Do we really need it? The caller will ensure that asv is not null.

>> +	regmap_read(asv->chipid_regmap, EXYNOS_CHIPID_REG_PKG_ID, &pkg_id);

-- 
Regards,
Sylwester

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel