[PATCH v1 13/24] arm: octeontx: Add headers for OcteonTX

[Simon Glass]

HI Stefan,

On Fri, 31 Jul 2020 at 08:21, Stefan Roese <sr at denx.de> wrote:
>
> Hi Simon,
>
> On 28.07.20 21:01, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr at denx.de> wrote:
> >>
> >> From: Suneel Garapati <sgarapati at marvell.com>
> >>
> >> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
> >>
> >> Signed-off-by: Stefan Roese <sr at denx.de>
> >> ---
> >>
> >> Changes in v1:
> >> - Change patch subject
> >>
> >>   arch/arm/include/asm/arch-octeontx/board.h    |  123 ++
> >>   arch/arm/include/asm/arch-octeontx/clock.h    |   25 +
> >>   .../asm/arch-octeontx/csrs/csrs-mio_emm.h     | 1193 +++++++++++++++++
> >>   .../include/asm/arch-octeontx/csrs/csrs-xcv.h |  428 ++++++
> >>   arch/arm/include/asm/arch-octeontx/gpio.h     |    6 +
> >>   arch/arm/include/asm/arch-octeontx/smc.h      |   20 +
> >>   arch/arm/include/asm/arch-octeontx/soc.h      |   33 +
> >>   7 files changed, 1828 insertions(+)
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/board.h
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/clock.h
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/csrs/csrs-mio_emm.h
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/csrs/csrs-xcv.h
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/gpio.h
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/smc.h
> >>   create mode 100644 arch/arm/include/asm/arch-octeontx/soc.h
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > Generic thoughts to consider:
> > - drop extra brackets around constants - e.g. MIO_EMM_BAR_E_MIO_EMM_PF_BAR4
> > - use #define or enum instead of inline functions, e.g. MIO_EMM_DMA
> > - lower-case hex
>
> Yes, thanks. I agree. Let me check, if I can find some time to clean
> this up a bit more.
>
> > I don't normally see bitfields in U-Boot. Is that a good idea?
>
> It is not, I agree. I've been bitten by those bitfields in the other
> Octeon drivers before (I2C, SPI etc). Bitfields make portable driver
> code quite hard. You need to add separate bitfield definitions for big-
> endian and little-endian support. So I switched on the common drivers,
> and with common I mean drivers supporting MIPS Octeon and ARM Octeon
> TX/TX2, from using bitfields to BIT_ULL() / GENMASK_ULL etc. This works
> just fine with big- and little-endian systems.
>
> But, as you probably have guessed, these header are auto-generated and
> changing this to BIT_ULL etc is really painful (time consuming) and also
> potentially error prone. So I really would like to stay with the
> bitfields for those structs / variables, that are solely used by the
> Octeon TX/TX2 (little-endian) platforms.
>
> I hope this is okay.
>

Seems OK to me.

Regards,
Simon