Re: [PATCH v4 2/2] accel: Add Arm Ethos-U NPU driver

[Frank Li <Frank.li@nxp.com>]

On Thu, Oct 16, 2025 at 07:35:06AM -0500, Rob Herring wrote:
> On Wed, Oct 15, 2025 at 4:02 PM Frank Li <Frank.li@nxp.com> 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 <Frank.li@nxp.com> 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);

Look good.

Frank
>
>
> Rob