On Wed, Oct 15, 2025 at 4:02 PM Frank Li <[email protected]> wrote:
>
> On Wed, Oct 15, 2025 at 03:36:05PM -0500, Rob Herring wrote:
> > On Wed, Oct 15, 2025 at 2:39 PM Frank Li <[email protected]> wrote:
> > >
> > > On Wed, Oct 15, 2025 at 12:47:40PM -0500, Rob Herring (Arm) wrote:
> > > > Add a driver for Arm Ethos-U65/U85 NPUs. The Ethos-U NPU has a
> > > > relatively simple interface with single command stream to describe
> > > > buffers, operation settings, and network operations. It supports up to 8
> > > > memory regions (though no h/w bounds on a region). The Ethos NPUs
> > > > are designed to use an SRAM for scratch memory. Region 2 is reserved
> > > > for SRAM (like the downstream driver stack and compiler). Userspace
> > > > doesn't need access to the SRAM.
> > > > +static int ethosu_init(struct ethosu_device *ethosudev)
> > > > +{
> > > > + int ret;
> > > > + u32 id, config;
> > > > +
> > > > + ret = devm_pm_runtime_enable(ethosudev->base.dev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = pm_runtime_resume_and_get(ethosudev->base.dev);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
> > > > + pm_runtime_use_autosuspend(ethosudev->base.dev);
> > > > +
> > > > + /* If PM is disabled, we need to call ethosu_device_resume()
> > > > manually. */
> > > > + if (!IS_ENABLED(CONFIG_PM)) {
> > > > + ret = ethosu_device_resume(ethosudev->base.dev);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > >
> > > I think it should call ethosu_device_resume() unconditional before
> > > devm_pm_runtime_enable();
> > >
> > > ethosu_device_resume();
> > > pm_runtime_set_active();
> > > pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
> > > devm_pm_runtime_enable();
> >
> > Why do you think this? Does this do a get?
> >
> > I don't think it is good to call the resume hook on our own, but we
> > have no choice with !CONFIG_PM. With CONFIG_PM, we should only use the
> > pm_runtime API.
>
> Enable clock and do some init work at probe() is quite common. But I never
> seen IS_ENABLED(CONFIG_PM) check. It is quite weird and not necessary to
> check CONFIG_PM flags. The most CONFIG_PM is enabled, so the branch !CONFIG_PM
> almost never tested.
Okay, I get what you meant.
>
> probe()
> {
> devm_clk_bulk_get_all_enabled();
>
> ... did some init work
>
> pm_runtime_set_active();
> devm_pm_runtime_enable();
>
> ...
> pm_runtime_put_autosuspend(ethosudev->base.dev);
> }
I think we still need a pm_runtime_get_noresume() in here since we do
a put later on. Here's what I have now:
ret = ethosu_device_resume(ethosudev->base.dev);
if (ret)
return ret;
pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
pm_runtime_use_autosuspend(ethosudev->base.dev);
ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
if (ret)
return ret;
pm_runtime_get_noresume(ethosudev->base.dev);
Rob
