[v6,2/8] thunderbolt: Updating the register definitions

Adding more Thunderbolt(TM) register definitions
and some helper macros.

Signed-off-by: Amir Levy <amir.jer.levy@intel.com>
---
 drivers/thunderbolt/nhi_regs.h | 109 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

On Mon, Aug 1, 2016 at 2:23 PM, Amir Levy <amir.jer.levy@intel.com> wrote:
> Adding more Thunderbolt(TM) register definitions
> and some helper macros.

Thinking about this again I would prefer it if you would put your
definitions into a separate file under icm/ (even if there is some
duplication). The style (bitfields vs. genmask) is different between
the drivers and for a reader it is difficult to find out what is
actually supposed to be used by the two drivers (ring_desc vs
tbt_buf_desc or the ring RING_INT_EN/DISABLE macros in the header file
vs. ring_interrupt_active in nhi.c).

This would also completely separate the two drivers.

Andreas


> Signed-off-by: Amir Levy <amir.jer.levy@intel.com>
> ---
>  drivers/thunderbolt/nhi_regs.h | 109 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 109 insertions(+)
>
> diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
> index 75cf069..b8e961f 100644
> --- a/drivers/thunderbolt/nhi_regs.h
> +++ b/drivers/thunderbolt/nhi_regs.h
> @@ -9,6 +9,11 @@
>
>  #include <linux/types.h>
>
> +#define NHI_MMIO_BAR 0
> +
> +#define TBT_RING_MIN_NUM_BUFFERS       2
> +#define TBT_RING_MAX_FRAME_SIZE                (4 * 1024)
> +
>  enum ring_flags {
>         RING_FLAG_ISOCH_ENABLE = 1 << 27, /* TX only? */
>         RING_FLAG_E2E_FLOW_CONTROL = 1 << 28,
> @@ -39,6 +44,33 @@ struct ring_desc {
>         u32 time; /* write zero */
>  } __packed;
>
> +/**
> + * struct tbt_buf_desc - TX/RX ring buffer descriptor.
> + * This is same as struct ring_desc, but without the use of bitfields and
> + * with explicit endianity.
> + */
> +struct tbt_buf_desc {
> +       __le64 phys;
> +       __le32 attributes;
> +       __le32 time;
> +};
> +
> +#define DESC_ATTR_LEN_SHIFT            0
> +#define DESC_ATTR_LEN_MASK             GENMASK(11, DESC_ATTR_LEN_SHIFT)
> +#define DESC_ATTR_EOF_SHIFT            12
> +#define DESC_ATTR_EOF_MASK             GENMASK(15, DESC_ATTR_EOF_SHIFT)
> +#define DESC_ATTR_SOF_SHIFT            16
> +#define DESC_ATTR_SOF_MASK             GENMASK(19, DESC_ATTR_SOF_SHIFT)
> +#define DESC_ATTR_TX_ISOCH_DMA_EN      BIT(20) /* TX */
> +#define DESC_ATTR_RX_CRC_ERR           BIT(20) /* RX after use */
> +#define DESC_ATTR_DESC_DONE            BIT(21)
> +#define DESC_ATTR_REQ_STS              BIT(22) /* TX and RX before use */
> +#define DESC_ATTR_RX_BUF_OVRN_ERR      BIT(22) /* RX after use */
> +#define DESC_ATTR_INT_EN               BIT(23)
> +#define DESC_ATTR_OFFSET_SHIFT         24
> +#define DESC_ATTR_OFFSET_MASK          GENMASK(31, DESC_ATTR_OFFSET_SHIFT)
> +
> +
>  /* NHI registers in bar 0 */
>
>  /*
> @@ -60,6 +92,30 @@ struct ring_desc {
>   */
>  #define REG_RX_RING_BASE       0x08000
>
> +#define REG_RING_STEP                  16
> +#define REG_RING_PHYS_LO_OFFSET                0
> +#define REG_RING_PHYS_HI_OFFSET                4
> +#define REG_RING_CONS_PROD_OFFSET      8       /* cons - RO, prod - RW */
> +#define REG_RING_CONS_SHIFT            0
> +#define REG_RING_CONS_MASK             GENMASK(15, REG_RING_CONS_SHIFT)
> +#define REG_RING_PROD_SHIFT            16
> +#define REG_RING_PROD_MASK             GENMASK(31, REG_RING_PROD_SHIFT)
> +#define REG_RING_SIZE_OFFSET           12
> +#define REG_RING_SIZE_SHIFT            0
> +#define REG_RING_SIZE_MASK             GENMASK(15, REG_RING_SIZE_SHIFT)
> +#define REG_RING_BUF_SIZE_SHIFT                16
> +#define REG_RING_BUF_SIZE_MASK         GENMASK(27, REG_RING_BUF_SIZE_SHIFT)
> +
> +#define TBT_RING_CONS_PROD_REG(iobase, ringbase, ringnumber) \
> +                             ((iobase) + (ringbase) + \
> +                             ((ringnumber) * REG_RING_STEP) + \
> +                             REG_RING_CONS_PROD_OFFSET)
> +
> +#define TBT_REG_RING_PROD_EXTRACT(val) (((val) & REG_RING_PROD_MASK) >> \
> +                                      REG_RING_PROD_SHIFT)
> +
> +#define TBT_REG_RING_CONS_EXTRACT(val) (((val) & REG_RING_CONS_MASK) >> \
> +                                      REG_RING_CONS_SHIFT)
>  /*
>   * 32 bytes per entry, one entry for every hop (REG_HOP_COUNT)
>   * 00: enum_ring_flags
> @@ -77,6 +133,19 @@ struct ring_desc {
>   * ..: unknown
>   */
>  #define REG_RX_OPTIONS_BASE    0x29800
> +#define REG_RX_OPTS_TX_E2E_HOP_ID_SHIFT        12
> +#define REG_RX_OPTS_TX_E2E_HOP_ID_MASK \
> +                               GENMASK(22, REG_RX_OPTS_TX_E2E_HOP_ID_SHIFT)
> +#define REG_RX_OPTS_MASK_OFFSET                4
> +#define REG_RX_OPTS_MASK_EOF_SHIFT     0
> +#define REG_RX_OPTS_MASK_EOF_MASK      GENMASK(15, REG_RX_OPTS_MASK_EOF_SHIFT)
> +#define REG_RX_OPTS_MASK_SOF_SHIFT     16
> +#define REG_RX_OPTS_MASK_SOF_MASK      GENMASK(31, REG_RX_OPTS_MASK_SOF_SHIFT)
> +
> +#define REG_OPTS_STEP                  32
> +#define REG_OPTS_E2E_EN                        BIT(28)
> +#define REG_OPTS_RAW                   BIT(30)
> +#define REG_OPTS_VALID                 BIT(31)
>
>  /*
>   * three bitfields: tx, rx, rx overflow
> @@ -86,6 +155,7 @@ struct ring_desc {
>   */
>  #define REG_RING_NOTIFY_BASE   0x37800
>  #define RING_NOTIFY_REG_COUNT(nhi) ((31 + 3 * nhi->hop_count) / 32)
> +#define REG_RING_NOTIFY_STEP   4
>
>  /*
>   * two bitfields: rx, tx
> @@ -94,8 +164,47 @@ struct ring_desc {
>   */
>  #define REG_RING_INTERRUPT_BASE        0x38200
>  #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
> +#define REG_RING_INT_TX_PROCESSED(ring_num)            BIT(ring_num)
> +#define REG_RING_INT_RX_PROCESSED(ring_num, num_paths) BIT((ring_num) + \
> +                                                           (num_paths))
> +#define RING_INT_DISABLE(base, val) iowrite32( \
> +                       ioread32((base) + REG_RING_INTERRUPT_BASE) & ~(val), \
> +                       (base) + REG_RING_INTERRUPT_BASE)
> +#define RING_INT_ENABLE(base, val) iowrite32( \
> +                       ioread32((base) + REG_RING_INTERRUPT_BASE) | (val), \
> +                       (base) + REG_RING_INTERRUPT_BASE)
> +#define RING_INT_DISABLE_TX(base, ring_num) \
> +       RING_INT_DISABLE(base, REG_RING_INT_TX_PROCESSED(ring_num))
> +#define RING_INT_DISABLE_RX(base, ring_num, num_paths) \
> +       RING_INT_DISABLE(base, REG_RING_INT_RX_PROCESSED(ring_num, num_paths))
> +#define RING_INT_ENABLE_TX(base, ring_num) \
> +       RING_INT_ENABLE(base, REG_RING_INT_TX_PROCESSED(ring_num))
> +#define RING_INT_ENABLE_RX(base, ring_num, num_paths) \
> +       RING_INT_ENABLE(base, REG_RING_INT_RX_PROCESSED(ring_num, num_paths))
> +#define RING_INT_DISABLE_TX_RX(base, ring_num, num_paths) \
> +       RING_INT_DISABLE(base, REG_RING_INT_TX_PROCESSED(ring_num) | \
> +                              REG_RING_INT_RX_PROCESSED(ring_num, num_paths))
> +
> +#define REG_RING_INTERRUPT_STEP        4
> +
> +#define REG_INT_THROTTLING_RATE        0x38c00
> +#define REG_INT_THROTTLING_RATE_STEP   4
> +#define NUM_INT_VECTORS                        16
> +
> +#define REG_INT_VEC_ALLOC_BASE 0x38c40
> +#define REG_INT_VEC_ALLOC_STEP         4
> +#define REG_INT_VEC_ALLOC_FIELD_BITS   4
> +#define REG_INT_VEC_ALLOC_FIELD_MASK   (BIT(REG_INT_VEC_ALLOC_FIELD_BITS) - 1)
> +#define REG_INT_VEC_ALLOC_PER_REG      ((BITS_PER_BYTE * sizeof(u32)) / \
> +                                        REG_INT_VEC_ALLOC_FIELD_BITS)
>
>  /* The last 11 bits contain the number of hops supported by the NHI port. */
>  #define REG_HOP_COUNT          0x39640
> +#define REG_HOP_COUNT_TOTAL_PATHS_MASK GENMASK(10, 0)
> +
> +#define REG_HOST_INTERFACE_RST 0x39858
> +
> +#define REG_DMA_MISC           0x39864
> +#define REG_DMA_MISC_INT_AUTO_CLEAR    BIT(2)
>
>  #endif
> --
> 2.7.4
>

On Sun, Sep 11 2016, 03:02 AM, Andreas Noever wrote:
> On Mon, Aug 1, 2016 at 2:23 PM, Amir Levy <amir.jer.levy@intel.com> wrote:

> > Adding more Thunderbolt(TM) register definitions and some helper

> > macros.

> 

> Thinking about this again I would prefer it if you would put your definitions

> into a separate file under icm/ (even if there is some duplication). The style

> (bitfields vs. genmask) is different between the drivers and for a reader it is

> difficult to find out what is actually supposed to be used by the two drivers

> (ring_desc vs tbt_buf_desc or the ring RING_INT_EN/DISABLE macros in the

> header file vs. ring_interrupt_active in nhi.c).

> 

> This would also completely separate the two drivers.

> 

> Andreas

> 


I'm also in favor of completely separating the drivers, but is it the right thing to do with the register definitions
when the underlying registers layout is exactly the same?

Note that bitfields are not so recommended when you care about the format/order of bits, like in the ring descriptor.

Amir