| Message ID | 20240606113334.396693-2-primoz.fiser@norik.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [1/2] OPP: Introduce devm_pm_opp_set_config_regulators | expand |
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().
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
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.
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);
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(-)