[PATCH 1/2] OPP: Introduce devm_pm_opp_set_config_regulators

[PATCH 1/2] OPP: Introduce devm_pm_opp_set_config_regulators

[Primoz Fiser]

Introduce devm_pm_opp_set_config_regulators() function. This is device
managed version of dev_pm_opp_set_config_regulators so caller don't need
to worry about managing returned tokens.

Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
 include/linux/pm_opp.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index dd7c8441af42..fa515aab47c9 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -664,6 +664,16 @@ static inline int dev_pm_opp_set_config_regulators(struct device *dev,
 	return dev_pm_opp_set_config(dev, &config);
 }
 
+static inline int devm_pm_opp_set_config_regulators(struct device *dev,
+						   config_regulators_t helper)
+{
+	struct dev_pm_opp_config config = {
+		.config_regulators = helper,
+	};
+
+	return devm_pm_opp_set_config(dev, &config);
+}
+
 static inline void dev_pm_opp_put_config_regulators(int token)
 {
 	dev_pm_opp_clear_config(token);
-- 
2.25.1

[PATCH 2/2] OPP: ti: Use devm_pm_opp_set_config_regulators

[Primoz Fiser]

Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti:
Migrate to dev_pm_opp_set_config_regulators()") returns wrong values
when all goes well and hence driver probing eventually fails.

Switch to using devm_pm_opp_set_config_regulators() function that
correctly handles return values and doesn't require us to handle
returned tokens.

Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()")
Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
 drivers/opp/ti-opp-supply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
index e3b97cd1fbbf..8a4bcc5fb9dc 100644
--- a/drivers/opp/ti-opp-supply.c
+++ b/drivers/opp/ti-opp-supply.c
@@ -392,7 +392,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators);
+	ret = devm_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators);
 	if (ret < 0)
 		_free_optimized_voltages(dev, &opp_data);
 
-- 
2.25.1

Re: [PATCH 2/2] OPP: ti: Use devm_pm_opp_set_config_regulators

[Viresh Kumar]

Hi Primoz,

Thanks for your changes, they look exactly as we discussed earlier, but .. 

On 06-06-24, 13:33, Primoz Fiser wrote:
> Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti:
> Migrate to dev_pm_opp_set_config_regulators()") returns wrong values
> when all goes well and hence driver probing eventually fails.
> 
> Switch to using devm_pm_opp_set_config_regulators() function that
> correctly handles return values and doesn't require us to handle
> returned tokens.
> 
> Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()")
> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
> ---
>  drivers/opp/ti-opp-supply.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
> index e3b97cd1fbbf..8a4bcc5fb9dc 100644
> --- a/drivers/opp/ti-opp-supply.c
> +++ b/drivers/opp/ti-opp-supply.c
> @@ -392,7 +392,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> -	ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators);
> +	ret = devm_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators);
>  	if (ret < 0)
>  		_free_optimized_voltages(dev, &opp_data);

-- I made a mistake.

The driver gets probed with a platform device, while
devm_pm_opp_set_config_regulators() works with cpu device. And so the
issue related to module insertion/removal/insertion will still be
there :(.

Did you try that though ?

The only way to get this solved is probably by introducing a remove()
method, which clears the OPP config and stores the token returned by
dev_pm_opp_set_config_regulators().

-- 
viresh

Re: [PATCH 2/2] OPP: ti: Use devm_pm_opp_set_config_regulators

[Primoz Fiser]

Hi Viresh,

On 10. 06. 24 06:22, Viresh Kumar wrote:
> Hi Primoz,
> 
> Thanks for your changes, they look exactly as we discussed earlier, but .. 
> 
> On 06-06-24, 13:33, Primoz Fiser wrote:
>> Function ti_opp_supply_probe() since commit 6baee034cb55 ("OPP: ti:
>> Migrate to dev_pm_opp_set_config_regulators()") returns wrong values
>> when all goes well and hence driver probing eventually fails.
>>
>> Switch to using devm_pm_opp_set_config_regulators() function that
>> correctly handles return values and doesn't require us to handle
>> returned tokens.
>>
>> Fixes: 6baee034cb55 ("OPP: ti: Migrate to dev_pm_opp_set_config_regulators()")
>> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
>> ---
>>  drivers/opp/ti-opp-supply.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
>> index e3b97cd1fbbf..8a4bcc5fb9dc 100644
>> --- a/drivers/opp/ti-opp-supply.c
>> +++ b/drivers/opp/ti-opp-supply.c
>> @@ -392,7 +392,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev)
>>  			return ret;
>>  	}
>>  
>> -	ret = dev_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators);
>> +	ret = devm_pm_opp_set_config_regulators(cpu_dev, ti_opp_config_regulators);
>>  	if (ret < 0)
>>  		_free_optimized_voltages(dev, &opp_data);
> 
> -- I made a mistake.

:(

> 
> The driver gets probed with a platform device, while
> devm_pm_opp_set_config_regulators() works with cpu device. And so the
> issue related to module insertion/removal/insertion will still be
> there :(.
> 
> Did you try that though ?

I didn't because of:

config ARM_TI_CPUFREQ

        bool "Texas Instruments CPUFreq support"


is a built-in driver.

Anyway, I guess one could trigger this also with:

$ cd /sys/devices/platform/ocp/4a003b20.opp-supply/driver
$ echo 4a003b20.opp-supply > unbind
$ echo 4a003b20.opp-supply > bind

[  164.231781] ------------[ cut here ]------------
[  164.236450] WARNING: CPU: 1 PID: 230 at drivers/opp/core.c:2474
dev_pm_opp_set_config+0x384/0x634
[  164.245422] Modules linked in: sha256_generic libsha256 sha256_arm
cfg80211 uas usb_storage xhci_plat_hcd xhci_hcd usbcore dwc3 roles
udc_core pru_rproc irq_pruss_intc usb_common rpmsg_ctrl rpmsg_char etnaviv
 ti_vpe ti_vip snd_soc_simple_card gpu_sched ti_sc bq27xxx_battery_hdq
bq27xxx_battery snd_soc_simple_card_utils pvrsrvkm(O) snd_soc_omap_hdmi
ahci_dwc ti_csc libahci_platform v4l2_mem2mem ti_vpdma libahci omap_
aes_driver videobuf2_dma_contig libaes videobuf2_memops videobuf2_v4l2
c_can_platform pruss libata videobuf2_common c_can omap_wdt
phy_omap_usb2 can_dev omap_hdq dwc3_omap omap_des
snd_soc_tlv320aic3x_i2c snd_so
c_tlv320aic3x libdes rtc_omap wire at24 omap_crypto palmas_pwrbutton
extcon_palmas rtc_palmas omap_sham crypto_engine omap_remoteproc
virtio_rpmsg_bus rpmsg_ns sch_fq_codel cryptodev(O) cmemk(O)
[  164.318298] CPU: 1 PID: 230 Comm: sh Tainted: G           O
6.1.80-bsp-yocto-ampliphy-am57x-kirkstone #1
[  164.328369] Hardware name: Generic DRA74X (Flattened Device Tree)
[  164.334472]  unwind_backtrace from show_stack+0x10/0x14
[  164.339752]  show_stack from dump_stack_lvl+0x40/0x4c
[  164.344818]  dump_stack_lvl from __warn+0x94/0xc0
[  164.349578]  __warn from warn_slowpath_fmt+0x1a4/0x1ac
[  164.354736]  warn_slowpath_fmt from dev_pm_opp_set_config+0x384/0x634
[  164.361236]  dev_pm_opp_set_config from devm_pm_opp_set_config+0xc/0x44
[  164.367889]  devm_pm_opp_set_config from ti_opp_supply_probe+0x1c8/0x2f4
[  164.374633]  ti_opp_supply_probe from platform_probe+0x5c/0xbc
[  164.380493]  platform_probe from really_probe+0xc8/0x2ec
[  164.385833]  really_probe from __driver_probe_device+0x88/0x1a0
[  164.391815]  __driver_probe_device from device_driver_attach+0x40/0x98
[  164.398376]  device_driver_attach from bind_store+0x80/0xec
[  164.403991]  bind_store from kernfs_fop_write_iter+0x10c/0x1cc
[  164.409851]  kernfs_fop_write_iter from vfs_write+0x2a0/0x3c8
[  164.415649]  vfs_write from ksys_write+0x5c/0xd4
[  164.420288]  ksys_write from ret_fast_syscall+0x0/0x4c
[  164.425445] Exception stack(0xf2359fa8 to 0xf2359ff0)
[  164.430511] 9fa0:                   00000014 00539568 00000001
00539568 00000014 00000000
[  164.438751] 9fc0: 00000014 00539568 b6f5c5a0 00000004 00000014
0050cc08 00000000 00000000
[  164.446960] 9fe0: 00000004 bed839d8 b6e80827 b6e01ae6
[  164.452056] ---[ end trace 0000000000000000 ]---
[  164.456726] ti_opp_supply: probe of 4a003b20.opp-supply failed with
error -16
-sh: echo: write error: Device or resource busy

so the error comes from drivers/opp/core.c block:

	/* This should be called before OPPs are initialized */
	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
		ret = -EBUSY;
		goto err;
	}


> 
> The only way to get this solved is probably by introducing a remove()
> method, which clears the OPP config and stores the token returned by
> dev_pm_opp_set_config_regulators().
> 

I did some additional experiments today by adding .remove_new callback
and calling dev_pm_opp_clear_config() in it.

However I still get the same error as above.

Unforunatelly, I cannot test on latest master since it doesn't boot yet
on my board (still using linux-ti 6.1).

Shall we store token and not call dev_pm_opp_set_config_regulators() in
probe() if token has been already aquired once?

BR,
Primoz

Re: [PATCH 2/2] OPP: ti: Use devm_pm_opp_set_config_regulators

[Viresh Kumar]

On 10-06-24, 13:39, Primoz Fiser wrote:
> I didn't because of:
> 
> config ARM_TI_CPUFREQ
> 
>         bool "Texas Instruments CPUFreq support"
> 
> 
> is a built-in driver.

This driver has confused me so many times.. The driver looks like a module,
since it declares all module properties but is builtin only :(

> Anyway, I guess one could trigger this also with:
> 
> $ cd /sys/devices/platform/ocp/4a003b20.opp-supply/driver
> $ echo 4a003b20.opp-supply > unbind
> $ echo 4a003b20.opp-supply > bind

> -sh: echo: write error: Device or resource busy
> 
> so the error comes from drivers/opp/core.c block:
> 
> 	/* This should be called before OPPs are initialized */
> 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> 		ret = -EBUSY;
> 		goto err;
> 	}

This is a different issue, which is unrelated to what we are discussing here. It
happens as the driver has registered the cpufreq device from init() callback
instead of probe() and remove() doesn't undo that.
 
> > The only way to get this solved is probably by introducing a remove()
> > method, which clears the OPP config and stores the token returned by
> > dev_pm_opp_set_config_regulators().

I will apply the V1 patch itself now. That is probably the best we can do for
now.

-- 
viresh