Re: [PATCH 10/19] thunderbolt: Make the ring register layout configurable

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Sun, Aug 30, 2026 at 10:19:28PM +0200, Sven Peter wrote:
> The ring descriptor and options registers are laid out differently on
> the Apple NHI. Describe their offsets and strides with a
> tb_nhi_ring_layout struct that NHI drivers can override.

This is odd because Apple is sitting in USB-IF and they are behind the
original register layout so why they do not follow it themselves?

> Signed-off-by: Sven Peter <sven@kernel.org>
> ---
>  drivers/thunderbolt/nhi.c   | 30 ++++++++++++++++++++++++------
>  drivers/thunderbolt/nhi.h   | 18 ++++++++++++++++++
>  include/linux/thunderbolt.h |  3 +++
>  3 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index fc54ff7edfb9..3a3d334e69fd 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -176,19 +176,32 @@ void nhi_disable_interrupts(struct tb_nhi *nhi)
>  
>  /* ring helper methods */
>  
> +static const struct tb_nhi_ring_layout nhi_default_ring_layout = {
> +	.tx_desc_base = REG_TX_RING_BASE,
> +	.rx_desc_base = REG_RX_RING_BASE,
> +	.desc_stride = 16,
> +	.tx_options_base = REG_TX_OPTIONS_BASE,
> +	.rx_options_base = REG_RX_OPTIONS_BASE,
> +	.options_stride = 32,
> +};

I'm not fan of these to be honest.

I think either adding ring_ops that includes hooks that can be overridden
by non-standard HI or not sure if we can take advantage of regmap here?

> +
>  static void __iomem *ring_desc_base(struct tb_ring *ring)
>  {
> +	const struct tb_nhi_ring_layout *layout = ring->nhi->ring_layout;
>  	void __iomem *io = ring->nhi->iobase;
> -	io += ring->is_tx ? REG_TX_RING_BASE : REG_RX_RING_BASE;
> -	io += ring->hop * 16;
> +
> +	io += ring->is_tx ? layout->tx_desc_base : layout->rx_desc_base;
> +	io += ring->hop * layout->desc_stride;
>  	return io;
>  }
>  
>  static void __iomem *ring_options_base(struct tb_ring *ring)
>  {
> +	const struct tb_nhi_ring_layout *layout = ring->nhi->ring_layout;
>  	void __iomem *io = ring->nhi->iobase;
> -	io += ring->is_tx ? REG_TX_OPTIONS_BASE : REG_RX_OPTIONS_BASE;
> -	io += ring->hop * 32;
> +
> +	io += ring->is_tx ? layout->tx_options_base : layout->rx_options_base;
> +	io += ring->hop * layout->options_stride;
>  	return io;
>  }
>  
> @@ -215,8 +228,10 @@ static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
>  
>  static void ring_iowrite64desc(struct tb_ring *ring, u64 value, u32 offset)
>  {
> -	iowrite32(value, ring_desc_base(ring) + offset);
> -	iowrite32(value >> 32, ring_desc_base(ring) + offset + 4);
> +	void __iomem *base = ring_desc_base(ring);
> +
> +	iowrite32(value, base + offset);
> +	iowrite32(value >> 32, base + offset + 4);
>  }
>  
>  static void ring_iowrite32options(struct tb_ring *ring, u32 value, u32 offset)
> @@ -1253,6 +1268,9 @@ int nhi_probe(struct tb_nhi *nhi)
>  	if (!nhi->ops->init_interrupts)
>  		return dev_err_probe(dev, -EINVAL, "missing required NHI ops\n");
>  
> +	if (!nhi->ring_layout)
> +		nhi->ring_layout = &nhi_default_ring_layout;
> +
>  	nhi->hop_count = ioread32(nhi->iobase + REG_CAPS) & 0x3ff;
>  	dev_dbg(dev, "total paths: %d\n", nhi->hop_count);
>  
> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
> index 4884c3f5a2b2..035cee433a3c 100644
> --- a/drivers/thunderbolt/nhi.h
> +++ b/drivers/thunderbolt/nhi.h
> @@ -40,6 +40,24 @@ void nhi_reset_interface(struct tb_nhi *nhi);
>  
>  extern const struct dev_pm_ops nhi_pm_ops;
>  
> +/**
> + * struct tb_nhi_ring_layout - Layout of the ring registers in the NHI MMIO space
> + * @tx_desc_base: Offset of the descriptor registers of the first TX ring
> + * @rx_desc_base: Offset of the descriptor registers of the first RX ring
> + * @desc_stride: Stride between the descriptor registers of neighboring rings
> + * @tx_options_base: Offset of the options registers of the first TX ring
> + * @rx_options_base: Offset of the options registers of the first RX ring
> + * @options_stride: Stride between the options registers of neighboring rings
> + */
> +struct tb_nhi_ring_layout {
> +	u32 tx_desc_base;
> +	u32 rx_desc_base;
> +	u32 desc_stride;
> +	u32 tx_options_base;
> +	u32 rx_options_base;
> +	u32 options_stride;
> +};
> +
>  /**
>   * struct tb_nhi_ops - NHI specific optional operations
>   * @init: NHI specific initialization
> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h
> index d48623fda79b..7c2717bd2945 100644
> --- a/include/linux/thunderbolt.h
> +++ b/include/linux/thunderbolt.h
> @@ -503,6 +503,8 @@ void tb_service_properties_changed(struct tb_service *svc);
>   *	  interrupt_work when dispatching interrupts to individual rings.
>   * @dev: Device associated with this NHI instance
>   * @ops: NHI specific optional ops
> + * @ring_layout: Layout of the ring registers inside @iobase. If not set
> + *		 the default layout from the USB4 specification is used.
>   * @iobase: MMIO space of the NHI
>   * @tx_rings: All Tx rings available on this host controller
>   * @rx_rings: All Rx rings available on this host controller
> @@ -524,6 +526,7 @@ struct tb_nhi {
>  	spinlock_t lock;
>  	struct device *dev;
>  	const struct tb_nhi_ops *ops;
> +	const struct tb_nhi_ring_layout *ring_layout;
>  	void __iomem *iobase;
>  	struct tb_ring **tx_rings;
>  	struct tb_ring **rx_rings;
> 
> -- 
> 2.55.0
>