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

[Thierry Reding]

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@xxxxxxxxx>
> ---
>  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: signature.asc
Description: PGP signature