On Mon, Sep 29, 2025 at 2:22 PM Frank Li <[email protected]> wrote:
>
> On Fri, Sep 26, 2025 at 03:00:49PM -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);
>
> pm_runtime_use_autosuspend() should be after last register read
> readl_relaxed(ethosudev->regs + NPU_REG_CONFIG);
>
> incase schedule happen between
> pm_runtime_use_autosuspend(ethosudev->base.dev);
> and readl().
All the call does is enable autosuspend. I don't think it matters
exactly when we enable it. We already did a get preventing autosuspend
until we do a put.
> > + /* 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;
> > + }
> > +
> > + ethosudev->npu_info.id = id = readl_relaxed(ethosudev->regs +
> > NPU_REG_ID);
> > + ethosudev->npu_info.config = config = readl_relaxed(ethosudev->regs +
> > NPU_REG_CONFIG);
> ...
> > +
> > +/**
> > + * ethosu_gem_create_with_handle() - Create a GEM object and attach it to
> > a handle.
> > + * @file: DRM file.
> > + * @ddev: DRM device.
> > + * @size: Size of the GEM object to allocate.
> > + * @flags: Combination of drm_ethosu_bo_flags flags.
> > + * @handle: Pointer holding the handle pointing to the new GEM object.
> > + *
> > + * Return: Zero on success
> > + */
> > +int ethosu_gem_create_with_handle(struct drm_file *file,
> > + struct drm_device *ddev,
> > + u64 *size, u32 flags, u32 *handle)
> > +{
> > + int ret;
> > + struct drm_gem_dma_object *mem;
> > + struct ethosu_gem_object *bo;
>
> move 'ret' here to keep reverise christmas tree order.
Is that the order DRM likes? It's got to be the dumbest coding standard we have.
> > +
> > + mem = drm_gem_dma_create(ddev, *size);
> > + if (IS_ERR(mem))
> > + return PTR_ERR(mem);
> > +
> > + bo = to_ethosu_bo(&mem->base);
> > + bo->flags = flags;
> > +
> > + /*
> > + * Allocate an id of idr table where the obj is registered
> > + * and handle has the id what user can see.
> > + */
> > + ret = drm_gem_handle_create(file, &mem->base, handle);
> > + if (!ret)
> > + *size = bo->base.base.size;
> > +
> > + /* drop reference from allocate - handle holds it now. */
> > + drm_gem_object_put(&mem->base);
> > +
> > + return ret;
> > +}
> > +
> ...
> > +
> > +static void cmd_state_init(struct cmd_state *st)
> > +{
> > + /* Initialize to all 1s to detect missing setup */
> > + memset(st, 0xff, sizeof(*st));
> > +}
> > +
> > +static u64 cmd_to_addr(u32 *cmd)
> > +{
> > + return ((u64)((cmd[0] & 0xff0000) << 16)) | cmd[1];
>
> will FIELD_PREP helpful?
Like this?:
return ((u64)FIELD_PREP(GENMASK(23, 16), cmd[0]) << 32) | cmd[1];
Questionable to me if that's better...
>
> > +}
> > +
> > +static u64 dma_length(struct ethosu_validated_cmdstream_info *info,
> > + struct dma_state *dma_st, struct dma *dma)
> > +{
> > + s8 mode = dma_st->mode;
> > + u64 len = dma->len;
> > +
> > + if (mode >= 1) {
> > + len += dma->stride[0];
> > + len *= dma_st->size0;
> > + }
> > + if (mode == 2) {
> > + len += dma->stride[1];
> > + len *= dma_st->size1;
> > + }
> > + if (dma->region >= 0)
> > + info->region_size[dma->region] =
> > max(info->region_size[dma->region],
> > + len + dma->offset);
> > +
> > + return len;
> > +}
> > +
> ...
>
> > +
> > +static void ethosu_job_handle_irq(struct ethosu_device *dev)
> > +{
> > + u32 status;
> > +
> > + pm_runtime_mark_last_busy(dev->base.dev);
>
> I think don't need pm_runtime_mark_last_busy() here because
> pm_runtime_put_autosuspend() already call pm_runtime_mark_last_busy().
>
> only mark last busy without pm_runtime_put() can't affect run time pm
> state, still in active state.
Yes, agreed. Copied from rocket, so Tomeu, you may want to look at that.
>
> > +
> > + status = readl_relaxed(dev->regs + NPU_REG_STATUS);
> > +
> > + if (status & (STATUS_BUS_STATUS | STATUS_CMD_PARSE_ERR)) {
> > + dev_err(dev->base.dev, "Error IRQ - %x\n", status);
> > + drm_sched_fault(&dev->sched);
> > + return;
> > + }
> > +
> > + scoped_guard(mutex, &dev->job_lock) {
> > + if (dev->in_flight_job) {
> > + dma_fence_signal(dev->in_flight_job->done_fence);
> > + pm_runtime_put_autosuspend(dev->base.dev);
> > + dev->in_flight_job = NULL;
> > + }
> > + }
> > +}
> > +
> ...
> > +
> > +int ethosu_job_init(struct ethosu_device *dev)
> > +{
> > + struct drm_sched_init_args args = {
> > + .ops = ðosu_sched_ops,
> > + .num_rqs = DRM_SCHED_PRIORITY_COUNT,
> > + .credit_limit = 1,
> > + .timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
> > + .name = dev_name(dev->base.dev),
> > + .dev = dev->base.dev,
> > + };
> > + int ret;
> > +
> > + spin_lock_init(&dev->fence_lock);
> > + mutex_init(&dev->job_lock);
> > + mutex_init(&dev->sched_lock);
>
> now perfer use dev_mutex_init().
$ git grep dev_mutex_init
(END)
Huh??
