[PATCH v2 01/32] bsps/grlib: Add generated headers

[Martin Åberg]

Hello,

The headers introduced by this commit indeed have strong potential for 
replacing the
existing GRLIB peripheral register descriptions used in device drivers. 
There
are some comments below regarding the flexibility, naming and content of the
new headers. That is, the review takes into account the possibility of 
transition
the current GRLIB peripheral device drivers into using the generated 
headers.

The changes suggested here should not affect the other commits in the
patch set.
No current GRLIB drivers will break because of this commit (changed or
unchanged).

All files in the commit which are not commented explicitly below are OK 
as is.


On 2023-07-05 13:18, Sebastian Huber wrote:
> diff --git a/bsps/include/grlib/grcan-regs.h b/bsps/include/grlib/grcan-regs.h
> +  /**
> +   * @brief See @ref RTEMSDeviceGRCANCanTxRD.
> +   */
> +  uint32_t cantxrd_0;
> +
> +  /**
> +   * @brief See @ref RTEMSDeviceGRCANCanTxRD.
> +   */
> +  uint32_t cantxrd_1;
> +
> +  uint32_t reserved_218_300[ 58 ];

Offset 0x210 should be cantxrd, offset 0x214 should be cantxirq. 
(GR740-UM-DS
has the wrong register name for txirq. The GRLIB IP Core User's Manual 
got it
right.)


> diff --git a/bsps/include/grlib/grclkgate-regs.h b/bsps/include/grlib/grclkgate-regs.h

> +#define GRCLKGATE_UNLOCK_UNLOCK_MASK 0x7ffU

> +#define GRCLKGATE_CLKEN_ENABLE_MASK 0x7ffU

> +#define GRCLKGATE_RESET_RESET_MASK 0x7ffU

The width of the unlock/enable/reset fields are defined as 11 bits in the
header. This matches the GR740 hardware implementation and documentation.
However, in GR712RC the implemented width is 12 bits. In the general case it
can be up to 32 bits so it would be preferable to define it as a
mask with all 32 bits included.



> diff --git a/bsps/include/grlib/grgprbank-regs.h b/bsps/include/grlib/grgprbank-regs.h

See comments below on grgpreg-regs.h


> diff --git a/bsps/include/grlib/grgpreg-regs.h b/bsps/include/grlib/grgpreg-regs.h

The content in this file is GR740 specific. The GR712RC has a peripheral 
with
the same name but with incompatible (chip specific) meaning. The GRGPREG is
used to access chip functionality which is not part of GRLIB itself, but 
rather
the chip technology and instantiation. For example PLL control and pin 
muxing.

To avoid confusion and naming conflicts, the files grgprbank-regs.h and
grgpreg-regs.h could be renamed to indicate GR740 in its name and fields.
Or possibly moved to a BSP variant specific directory.



> diff --git a/bsps/include/grlib/grspw2-regs.h b/bsps/include/grlib/grspw2-regs.h

> diff --git a/bsps/include/grlib/grspwrouter-regs.h b/bsps/include/grlib/grspwrouter-regs.h

> diff --git a/bsps/include/grlib/spwrmap-regs.h b/bsps/include/grlib/spwrmap-regs.h

This is the review comments for grspw2-regs.h, grspwrouter-regs.h,
spwrmap-regs.h:

There seems to be a bit of overlap in these three files, and some things
missing.

grspw2-regs.h as reviewed contains register descriptions for an 
instantiation
of the GRSPW2 controller, including DMA, interrupt control, RMAP target 
config,
link status/control. It only has one DMA channel however. Possibly this is
modelled after the GRSPW2 in GR712RC, or the "SpaceWire Debug Link" in 
GR740.

grspwrouter-regs.h as reviewed contains the registers for the GR740 GRSPW2
controllers connected to the GR740 on-chip SpaceWire router. (These 
controllers
are also called AMBA ports, which is a subset of the general GRSPW2 
interface).
The register interface description has 4 DMA channels, but does not contain
link status/control.

spwrmap-regs.h as revewied appears to describe the GR740 SpaceWire router.
However, it is missing many registers for configuring links, policy and 
routing
table. The bit definitions are there, but only one node address and one 
port is
covered.


Below are recommendations for cleaning up the naming and to make the
SpaceWire register descriptions compatible with all GRLIB systems. It
eliminates one of the files as a bonus.

1.
Remove grspwrouter-regs.h from the commit.

2.
Align grspw2-regs.h with the register description in the GRSPW2 chapter of
GRLIB IP Core User's Manual (GRIP). In particular, there should be registers
for all 4 DMA channels and for link control.

The applicable GRIP tables are "GRSPW2 registers" and forward. Registers 
after
offset 0xC8 can be skipped. Also the tables for "SpaceWire Plug-and-Play
support" can be skipped because they are already in spwpnp-regs.h.

The delta can probably be extracted from the removed grspwrouter-regs.h.

3.
Rename spwrmap-regs.h to spwrouter-regs.h. This makes it clear that the file
contains register defintions for the SpaceWire router (link control, routing
tables, policy, status, etc.). It could be modelled on the GR740 router
implementation. In particular, fields such as rtpmap and rtactrl should 
be an
array of 255 uint32_t entries rather than a single uint32_t as in the 
reviewed
file.


For reference, and to illustrate the point, please have a look at the
definitions of the GRSPW2 and SPWROUTER registers in
   bsps/shared/grlib/spw/grspw_pkt.c
   bsps/shared/grlib/spw/grspw_router.c

With these updates in place, the headers will be compatible with current and
future GRLIB systems with SpaceWire. Including GR740, GR712RC, UT700, GR765,
aswell as custom FPGAs and ASICs.


-- 
Best regards,

Martin Åberg
Software Engineer
Frontgrade Gaisler
martin.aberg at gaisler.com

Frontgrade Gaisler AB, Kungsgatan 12, SE-411 19 GÖTEBORG, Sweden.
+46 (0) 31 775 8650, www.gaisler.com