Message-ID: <aPAL67Oct5yJv8/d@lizhi-Precision-Tower-5810>
Date: Wed, 15 Oct 2025 17:02:35 -0400
From: Frank Li <Frank.li@....com>
To: Rob Herring <robh@...nel.org>
Cc: Tomeu Vizoso <tomeu@...euvizoso.net>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Oded Gabbay <ogabbay@...nel.org>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Christian König <christian.koenig@....com>,
	Robin Murphy <robin.murphy@....com>,
	Steven Price <steven.price@....com>,
	Daniel Stone <daniel@...ishbar.org>,
	Sui Jingfeng <sui.jingfeng@...ux.dev>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v4 2/2] accel: Add Arm Ethos-U NPU driver

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@....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.
> > >
> > > The h/w has no MMU nor external IOMMU and is a DMA engine which can
> > > read and write anywhere in memory without h/w bounds checks. The user
> > > submitted command streams must be validated against the bounds of the
> > > GEM BOs. This is similar to the VC4 design which validates shaders.
> > >
> > > The job submit is based on the rocket driver for the Rockchip NPU
> > > utilizing the GPU scheduler. It is simpler as there's only 1 core rather
> > > than 3.
> > >
> > > Tested on i.MX93 platform (U65) and FVP (U85) with WIP Mesa Teflon
> > > support.
> > >
> > > Acked-by: Thomas Zimmermann <tzimmermann@...e.de>
> > > Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> > > ---
> >
> > How to test this driver?
>
> You need to add the DT node to i.MX93 .dts like the example, build the
> mesa ethosu branch, and then run tflite with it pointed to the mesa
> delegate.
>
> I can send an i.MX93 dts patch after this is merged.
>
> > > v4:
> > > - Use bulk clk API
> > > - Various whitespace fixes mostly due to ethos->ethosu rename
> > > - Drop error check on dma_set_mask_and_coherent()
> > > - Drop unnecessary pm_runtime_mark_last_busy() call
> > > - Move variable declarations out of switch (a riscv/clang build failure)
> > > - Use lowercase hex in all defines
> > > - Drop unused ethosu_device.coherent member
> > > - Add comments on all locks
> > >
> > ...
> > > diff --git a/drivers/accel/ethosu/ethosu_device.h b/drivers/accel/ethosu/ethosu_device.h
> > > new file mode 100644
> > > index 000000000000..69d610c5c2d7
> > > --- /dev/null
> > > +++ b/drivers/accel/ethosu/ethosu_device.h
> > > @@ -0,0 +1,190 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only or MIT */
> > > +/* Copyright 2025 Arm, Ltd. */
> > > +
> > > +#ifndef __ETHOSU_DEVICE_H__
> > > +#define __ETHOSU_DEVICE_H__
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +#include <drm/drm_device.h>
> > > +#include <drm/gpu_scheduler.h>
> > > +
> > > +#include <drm/ethosu_accel.h>
> > > +
> > > +struct clk;
> > > +struct gen_pool;
> >
> > Supposed should include clk.h instead declear a struct.
>
> Headers should only use a forward declaration if that's all they need.
> It keeps the struct opaque for starters.
>
> > ...
> > > +
> > > +static int ethosu_open(struct drm_device *ddev, struct drm_file *file)
> > > +{
> > > +     int ret = 0;
> > > +     struct ethosu_file_priv *priv;
> > > +
> > > +     if (!try_module_get(THIS_MODULE))
> > > +             return -EINVAL;
> > > +
> > > +     priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > +     if (!priv) {
> > > +             ret = -ENOMEM;
> > > +             goto err_put_mod;
> > > +     }
> > > +     priv->edev = to_ethosu_device(ddev);
> > > +
> > > +     ret = ethosu_job_open(priv);
> > > +     if (ret)
> > > +             goto err_free;
> > > +
> > > +     file->driver_priv = priv;
> >
> > slice simple.
> >
> > struct ethosu_file_priv __free(kfree) *priv = NULL;
> > ...
> > priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>
> Linus has voiced his opinion that the above should not be done. It
> should be all one line *only*. But now we allow C99 declarations, so
> we can move it down. We can't get rid of the goto for module_put(), so
> it only marginally helps here.
>
> > ...
> >
> > file->driver_priv = no_free_ptr(priv);
> >
> >
> > > +     return 0;
> > > +
> > > +err_free:
> > > +     kfree(priv);
> > > +err_put_mod:
> > > +     module_put(THIS_MODULE);
> > > +     return ret;
> > > +}
> > > +
> > ...
> > > +
> > > +
> > > +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.

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);
}

ethosu_init() only is called by ethosu_probe(). with above pattern,
needn't check CONFIG_PM and call resume hook ethosu_device_resume();

Frank

>
> Rob

Powered by blists - more mailing lists