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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Wed, Oct 23, 2019 at 12:48:34PM +0200, Sylwester Nawrocki wrote:
> 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.

OK, let's keep the regmap for safe access to the same region by multiple
drivers.

> 
> >> +	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.

Remove it entirely please or leave it to dev_dbg (but even then add some
calming message like "Unsupported ...., skipping ASV"). Driver binds on
many devices and lack of ASV is kind of regular (till now there was no
ASV at all).

> 
> >> +		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.

Then let's move it to exynos5422 part, and later make it generic if
needed.

> 
> >> +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.

Indeed, it's not needed. Skip my comment.

Best regards,
Krzysztof