Re: [PATCH 08/25] hw/i2c: add support for flexcomm i2c

[Corey Minyard <corey@minyard.net>]

Thanks, this all looks good to me.  And FIELD() is the right way to
go, as Peter said.

-corey

On Fri, Sep 20, 2024 at 1:03 PM Octavian Purdila <tavip@google.com> wrote:
>
> On Thu, Sep 19, 2024 at 2:36 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 18 Sept 2024 at 22:31, Corey Minyard <corey@minyard.net> wrote:
> > > Generally it's frowned upon to have a ton of extra stuff that's not
> > > used.  I would think some is ok, like defining bits in registers that
> > > aren't used yet, but I have no idea how all the enums below here
> > > actually tie into anything.  I'm guessing these are just bitmasks into
> > > registers, but really, it's a lot easier to read if you have something
> > > like:
> > >
> > > /*
> > >  * The I2C Master function enable. When disabled, the Master
> > >  * configuration settings are not changed, but the Master function is
> > >  * internally reset.
> > >  */
> > > #define FLEXCOMM_I2C_CFG_MSTEN (1 << 4)
> >
> > The FIELD macro already gives you that:
> >   FIELD(FLEXCOMM_I2C_CFG, MSTEN, startbit, len);
> > will define an R_FLEXCOMM_I2C_CFG_MSTEN_MASK (which is
> > (1 << startbit) for the 'len == 1' case).
> >
> > You can also set and read a 1 bit field the same as
> > any other, with the FIELD_DP32/FIELD_EX32 macros, so
> > you don't often need to directly use the MASK macro:
> >   s->cfg = FIELD_DP32(s->cfg, CFG, MSTEN, 1);
> > and
> >   if (FIELD_EX32(s->cfg, CFG, MSTEN)) {
> >      ...
> >   }
> >
> > The FIELD() macros are a bit unwieldy sometimes but the
> > advantage over ad-hoc #defines is that they're consistent
> > for every field in every register.
> >
> > I agree that providing enums for the possible values for 1-bit
> > fields is a bit superfluous.
> >
>
> I went ahead and removed those 1-bit enum values and added support to
> filter register/fields when generating the code. Also converted the
> enums to defines to make these a little bit more compact as I don't
> think we have any advantage over the enums?
>
> So with the following invocation:
>
>   run_target('svd-flexcomm-i2c', command: svd_gen_header +
>     [ '-i', rt595, '-o', '@SOURCE_ROOT@/include/hw/arm/svd/flexcomm_i2c.h',
>       '-p', 'I2C0', '-t', 'FLEXCOMM_I2C',
>       '--fields', 'CFG TIMEOUT:TOMIN MSTCTL MSTDAT
> STAT:MSTPENDING,MSTSTATE INT*:MSTPENDING* SLV*:'])
>
> I am getting the below generated file. Note that the register info is
> generated for all registers because this information is used to
> initialize the reset values, mask writes appropriately in registers
> and trace register access.
>
> Please let me know if this looks good or if there are any other tweaks
> I could make.
>
> /*
>  * Copyright 2016-2023 NXP SPDX-License-Identifier: BSD-3-Clause
>  *
>  * Automatically generated by svd-gen-header.py from MIMXRT595S_cm33.xml
>  */
> #pragma once
>
> #include "hw/register.h"
>
> /* I2C Bus Interface */
> #define FLEXCOMM_I2C_REGS_NO (1024)
>
> /* Configuration Register */
> REG32(FLEXCOMM_I2C_CFG, 0x800);
> /* Master Enable */
> FIELD(FLEXCOMM_I2C_CFG, MSTEN, 0, 1);
> /* Slave Enable */
> FIELD(FLEXCOMM_I2C_CFG, SLVEN, 1, 1);
> /* Monitor Enable */
> FIELD(FLEXCOMM_I2C_CFG, MONEN, 2, 1);
> /* I2C bus Time-out Enable */
> FIELD(FLEXCOMM_I2C_CFG, TIMEOUTEN, 3, 1);
> /* Monitor function Clock Stretching */
> FIELD(FLEXCOMM_I2C_CFG, MONCLKSTR, 4, 1);
> /* High Speed mode Capable enable */
> FIELD(FLEXCOMM_I2C_CFG, HSCAPABLE, 5, 1);
>
> /* Status Register */
> REG32(FLEXCOMM_I2C_STAT, 0x804);
> /* Master Pending */
> FIELD(FLEXCOMM_I2C_STAT, MSTPENDING, 0, 1);
> /* Master State code */
> FIELD(FLEXCOMM_I2C_STAT, MSTSTATE, 1, 3);
> /* Idle. The Master function is available to be used for a new transaction. */
> #define FLEXCOMM_I2C_STAT_MSTSTATE_IDLE 0
> /*
>  * Receive ready. Received data is available (in Master Receiver mode). Address
>  * plus Read was previously sent and Acknowledged by a slave.
>  */
> #define FLEXCOMM_I2C_STAT_MSTSTATE_RECEIVE_READY 1
> /*
>  * Transmit ready. Data can be transmitted (in Master Transmitter mode).
>  * Address plus Write was previously sent and Acknowledged by a slave.
>  */
> #define FLEXCOMM_I2C_STAT_MSTSTATE_TRANSMIT_READY 2
> /* NACK Address. Slave NACKed address. */
> #define FLEXCOMM_I2C_STAT_MSTSTATE_NACK_ADDRESS 3
> /* NACK Data. Slave NACKed transmitted data. */
> #define FLEXCOMM_I2C_STAT_MSTSTATE_NACK_DATA 4
>
> /* Interrupt Enable Set Register */
> REG32(FLEXCOMM_I2C_INTENSET, 0x808);
> /* Master Pending interrupt Enable */
> FIELD(FLEXCOMM_I2C_INTENSET, MSTPENDINGEN, 0, 1);
>
> /* Interrupt Enable Clear Register */
> REG32(FLEXCOMM_I2C_INTENCLR, 0x80C);
> /* Master Pending interrupt clear */
> FIELD(FLEXCOMM_I2C_INTENCLR, MSTPENDINGCLR, 0, 1);
>
> /* Time-out Register */
> REG32(FLEXCOMM_I2C_TIMEOUT, 0x810);
> /* Time-out time value, the bottom 4 bits */
> FIELD(FLEXCOMM_I2C_TIMEOUT, TOMIN, 0, 4);
>
> /* Interrupt Status Register */
> REG32(FLEXCOMM_I2C_INTSTAT, 0x818);
> /* Master Pending */
> FIELD(FLEXCOMM_I2C_INTSTAT, MSTPENDING, 0, 1);
>
> /* Master Control Register */
> REG32(FLEXCOMM_I2C_MSTCTL, 0x820);
> /* Master Continue(write-only) */
> FIELD(FLEXCOMM_I2C_MSTCTL, MSTCONTINUE, 0, 1);
> /* Master Start control(write-only) */
> FIELD(FLEXCOMM_I2C_MSTCTL, MSTSTART, 1, 1);
> /* Master Stop control(write-only) */
> FIELD(FLEXCOMM_I2C_MSTCTL, MSTSTOP, 2, 1);
> /* Master DMA enable */
> FIELD(FLEXCOMM_I2C_MSTCTL, MSTDMA, 3, 1);
>
> /* Master Data Register */
> REG32(FLEXCOMM_I2C_MSTDAT, 0x828);
> /* Master function data register */
> FIELD(FLEXCOMM_I2C_MSTDAT, DATA, 0, 8);
>
> /* Slave Control Register */
> REG32(FLEXCOMM_I2C_SLVCTL, 0x840);
>
> /* Slave Data Register */
> REG32(FLEXCOMM_I2C_SLVDAT, 0x844);
>
> /* Slave Address Register */
> REG32(FLEXCOMM_I2C_SLVADR0, 0x848);
>
> /* Slave Address Register */
> REG32(FLEXCOMM_I2C_SLVADR1, 0x84C);
>
> /* Slave Address Register */
> REG32(FLEXCOMM_I2C_SLVADR2, 0x850);
>
> /* Slave Address Register */
> REG32(FLEXCOMM_I2C_SLVADR3, 0x854);
>
> /* Slave Qualification for Address 0 Register */
> REG32(FLEXCOMM_I2C_SLVQUAL0, 0x858);
>
>
> #define FLEXCOMM_I2C_REGISTER_ACCESS_INFO_ARRAY(_name) \
>     struct RegisterAccessInfo _name[FLEXCOMM_I2C_REGS_NO] = { \
>         [0 ... FLEXCOMM_I2C_REGS_NO - 1] = { \
>             .name = "", \
>             .addr = -1, \
>         }, \
>         [0x200] = { \
>             .name = "CFG", \
>             .addr = 0x800, \
>             .ro = 0xFFFFFFC0, \
>             .reset = 0x0, \
>         }, \
>         [0x201] = { \
>             .name = "STAT", \
>             .addr = 0x804, \
>             .ro = 0xFCF57FAF, \
>             .reset = 0x801, \
>         }, \
>         [0x202] = { \
>             .name = "INTENSET", \
>             .addr = 0x808, \
>             .ro = 0xFCF476AE, \
>             .reset = 0x0, \
>         }, \
>         [0x203] = { \
>             .name = "INTENCLR", \
>             .addr = 0x80C, \
>             .ro = 0xFCF476AE, \
>             .reset = 0x0, \
>         }, \
>         [0x204] = { \
>             .name = "TIMEOUT", \
>             .addr = 0x810, \
>             .ro = 0xFFFF0000, \
>             .reset = 0xFFFF, \
>         }, \
>         [0x205] = { \
>             .name = "CLKDIV", \
>             .addr = 0x814, \
>             .ro = 0xFFFF0000, \
>             .reset = 0x0, \
>         }, \
>         [0x206] = { \
>             .name = "INTSTAT", \
>             .addr = 0x818, \
>             .ro = 0xFFFFFFFF, \
>             .reset = 0x801, \
>         }, \
>         [0x208] = { \
>             .name = "MSTCTL", \
>             .addr = 0x820, \
>             .ro = 0xFFFFFFF0, \
>             .reset = 0x0, \
>         }, \
>         [0x209] = { \
>             .name = "MSTTIME", \
>             .addr = 0x824, \
>             .ro = 0xFFFFFF88, \
>             .reset = 0x77, \
>         }, \
>         [0x20A] = { \
>             .name = "MSTDAT", \
>             .addr = 0x828, \
>             .ro = 0xFFFFFF00, \
>             .reset = 0x0, \
>         }, \
>         [0x210] = { \
>             .name = "SLVCTL", \
>             .addr = 0x840, \
>             .ro = 0xFFFFFCF4, \
>             .reset = 0x0, \
>         }, \
>         [0x211] = { \
>             .name = "SLVDAT", \
>             .addr = 0x844, \
>             .ro = 0xFFFFFF00, \
>             .reset = 0x0, \
>         }, \
>         [0x212] = { \
>             .name = "SLVADR0", \
>             .addr = 0x848, \
>             .ro = 0xFFFF7F00, \
>             .reset = 0x1, \
>         }, \
>         [0x213] = { \
>             .name = "SLVADR1", \
>             .addr = 0x84C, \
>             .ro = 0xFFFF7F00, \
>             .reset = 0x1, \
>         }, \
>         [0x214] = { \
>             .name = "SLVADR2", \
>             .addr = 0x850, \
>             .ro = 0xFFFF7F00, \
>             .reset = 0x1, \
>         }, \
>         [0x215] = { \
>             .name = "SLVADR3", \
>             .addr = 0x854, \
>             .ro = 0xFFFF7F00, \
>             .reset = 0x1, \
>         }, \
>         [0x216] = { \
>             .name = "SLVQUAL0", \
>             .addr = 0x858, \
>             .ro = 0xFFFFFF00, \
>             .reset = 0x0, \
>         }, \
>         [0x220] = { \
>             .name = "MONRXDAT", \
>             .addr = 0x880, \
>             .ro = 0xFFFFFFFF, \
>             .reset = 0x0, \
>         }, \
>         [0x3FF] = { \
>             .name = "ID", \
>             .addr = 0xFFC, \
>             .ro = 0xFFFFFFFF, \
>             .reset = 0xE0301300, \
>         }, \
>     }