Re: [PATCH v2 3/4] drm/nvdla: Add register head file of NVDLA

Thierry Reding
2022-04-28T14:32:01Z

[-- Attachment #1: Type: text/plain, Size: 3745 bytes --]

On Tue, Apr 26, 2022 at 02:08:00PM +0800, Cai Huoqing wrote:
> The NVIDIA Deep Learning Accelerator (NVDLA) is an open source IP
> which is integrated into NVIDIA Jetson AGX Xavier,
> so add register head file of this accelerator.
>
> Signed-off-by: Cai Huoqing <cai.huoqing@linux.dev>
> ---
> drivers/gpu/drm/nvdla/nvdla_reg.h | 6411 +++++++++++++++++++++++++++++
> 1 file changed, 6411 insertions(+)
> create mode 100644 drivers/gpu/drm/nvdla/nvdla_reg.h

You probably want to change the ordering of the patches a little because
this new header file is already being used in patch 2. With the current
ordering you'll break the build between patches 2 and 3. The same will
likely happen with patch 4 because I'm assuming (haven't looked yet)
that the UAPI structures are getting used in the driver code as well.

Other than that, not much to say about this. One note perhaps, see
below.
>
> diff --git a/drivers/gpu/drm/nvdla/nvdla_reg.h b/drivers/gpu/drm/nvdla/nvdla_reg.h
> new file mode 100644
> index 000000000000..5ca2897405bc
> --- /dev/null
> +++ b/drivers/gpu/drm/nvdla/nvdla_reg.h
> @@ -0,0 +1,6411 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/*
> + * Copyright (C) 2017-2018 NVIDIA CORPORATION.
> + * Copyright (C) 2022 Cai Huoqing
> + */
> +
> +#ifndef __NVDLA_REG_H_
> +#define __NVDLA_REG_H_
> +
> +// Register NVDLA_CFGROM_CFGROM_HW_VERSION_0
> +#define NVDLA_CFGROM_CFGROM_HW_VERSION_0 _MK_ADDR_CONST(0x0)
> +#define NVDLA_CFGROM_CFGROM_HW_VERSION_0_HW_VERSION_SHIFT _MK_SHIFT_CONST(0)
> +#define NVDLA_CFGROM_CFGROM_HW_VERSION_0_HW_VERSION_FIELD _MK_FIELD_CONST(0xffffffff, NVDLA_CFGROM_CFGROM_HW_VERSION_0_HW_VERSION_SHIFT)

I know that people have in the past expressed reluctance about the way
that these fields are defined. I personally don't like these very much
because they are very redundant (e.g. that CFGROM_ is duplicated for no
obvious reason). I also think those _MK_* macros are very difficult to
understand because they can be found nowhere else in the Linux sources
so people aren't used to the format. And in fact the Linux kernel has
its own set of macros to define fields.

I also realize that this would probably be a pain to change. Let me see
if I can find out how exactly those get generated, so perhaps there's a
way to get them generated into a format that more closely matches the
idioms used in Linux.

> +//
> +// ADDRESS SPACES
> +//
> +
> +#define BASE_ADDRESS_NVDLA_CFGROM 0x0
> +#define BASE_ADDRESS_NVDLA_GLB 0x1000
> +#define BASE_ADDRESS_NVDLA_MCIF 0x2000
> +#define BASE_ADDRESS_NVDLA_CDMA 0x3000
> +#define BASE_ADDRESS_NVDLA_CSC 0x4000
> +#define BASE_ADDRESS_NVDLA_CMAC_A 0x5000
> +#define BASE_ADDRESS_NVDLA_CMAC_B 0x6000
> +#define BASE_ADDRESS_NVDLA_CACC 0x7000
> +#define BASE_ADDRESS_NVDLA_SDP_RDMA 0x8000
> +#define BASE_ADDRESS_NVDLA_SDP 0x9000
> +#define BASE_ADDRESS_NVDLA_PDP_RDMA 0xa000
> +#define BASE_ADDRESS_NVDLA_PDP 0xb000
> +#define BASE_ADDRESS_NVDLA_CDP_RDMA 0xc000
> +#define BASE_ADDRESS_NVDLA_CDP 0xd000
> +#define BASE_ADDRESS_NVDLA_GEC 0xe000
> +#define BASE_ADDRESS_NVDLA_CVIF 0xf000
> +#define BASE_ADDRESS_NVDLA_BDMA 0x10000
> +#define BASE_ADDRESS_NVDLA_RBK 0x11000

I'm wondering if it might make sense to turn these into separate reg
entries in the device tree, though it might not be worth it. The one
concern I have is that these might change internally in a newer
revision, which might then make it a pain to adapt the driver to the
differing offsets.

That said, I'm not an expert on DLA, so I don't know how this has
evolved in newer chip. I'll try to track down our local experts so that
they can clarify a few things for us.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]