Message-ID: <CAL_Jsq+L2RHgP9FaEpxzzVRybyjeNr84xgEBbU4KEyZtrz63FA@mail.gmail.com>
Date: Wed, 15 Oct 2025 15:36:05 -0500
From: Rob Herring <robh@...nel.org>
To: Frank Li <Frank.li@....com>
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 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.

Rob

Powered by blists - more mailing lists