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
