Re: [PATCH v2 13/22] thunderbolt: Add ring register accessors to tb_nhi_ops

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

Hi,

On Sun, Sep 06, 2026 at 08:36:36PM +0200, Sven Peter wrote:
> The ring descriptor and options registers are laid out differently on
> the Apple NHI. Add optional tb_nhi_ops hooks for their base addresses
> which fall back to the standard USB4 register layout when they are not
> set.
> 
> Signed-off-by: Sven Peter <sven@kernel.org>
> ---
>  drivers/thunderbolt/nhi.c | 32 ++++++++++++++++++++++++++------
>  drivers/thunderbolt/nhi.h |  6 ++++++
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index fc54ff7edfb9..1b1e4a12c744 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -179,19 +179,37 @@ void nhi_disable_interrupts(struct tb_nhi *nhi)
>  static void __iomem *ring_desc_base(struct tb_ring *ring)
>  {
>  	void __iomem *io = ring->nhi->iobase;
> +

Unintended change?

>  	io += ring->is_tx ? REG_TX_RING_BASE : REG_RX_RING_BASE;
>  	io += ring->hop * 16;
>  	return io;
>  }
>  
> +static void __iomem *nhi_ring_desc_base(struct tb_ring *ring)
> +{
> +	if (ring->nhi->ops->ring_desc_base)
> +		return ring->nhi->ops->ring_desc_base(ring);
> +
> +	return ring_desc_base(ring);
> +}
> +
>  static void __iomem *ring_options_base(struct tb_ring *ring)
>  {
>  	void __iomem *io = ring->nhi->iobase;
> +

Ditto here.

>  	io += ring->is_tx ? REG_TX_OPTIONS_BASE : REG_RX_OPTIONS_BASE;
>  	io += ring->hop * 32;
>  	return io;
>  }
>  
> +static void __iomem *nhi_ring_options_base(struct tb_ring *ring)
> +{
> +	if (ring->nhi->ops->ring_options_base)
> +		return ring->nhi->ops->ring_options_base(ring);
> +
> +	return ring_options_base(ring);

This is better IMO. Although I think even better would be to populate
nhi->ops for both the standard NHI and for Apple (and possibly others) and
then we can just do:

	ring->nhi->ops->ring_options_base(..)

or with a wrapper like this that makes it slightly more readable.

It does not need to be part of this series but wanted to mention here.

> +}
> +
>  static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
>  {
>  	/*
> @@ -199,29 +217,31 @@ static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
>  	 * are ignored by the hardware so we can save one ioread32() by
>  	 * filling the read-only bits with zeroes.
>  	 */
> -	iowrite32(cons, ring_desc_base(ring) + 8);
> +	iowrite32(cons, nhi_ring_desc_base(ring) + 8);
>  }
>  
>  static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
>  {
>  	/* See ring_iowrite_cons() above for explanation */
> -	iowrite32(prod << 16, ring_desc_base(ring) + 8);
> +	iowrite32(prod << 16, nhi_ring_desc_base(ring) + 8);
>  }
>  
>  static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
>  {
> -	iowrite32(value, ring_desc_base(ring) + offset);
> +	iowrite32(value, nhi_ring_desc_base(ring) + 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 = nhi_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)
>  {
> -	iowrite32(value, ring_options_base(ring) + offset);
> +	iowrite32(value, nhi_ring_options_base(ring) + offset);
>  }
>  
>  static bool ring_full(struct tb_ring *ring)
> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
> index 4884c3f5a2b2..3a963b0b5b46 100644
> --- a/drivers/thunderbolt/nhi.h
> +++ b/drivers/thunderbolt/nhi.h
> @@ -52,6 +52,10 @@ extern const struct dev_pm_ops nhi_pm_ops;
>   * @post_nvm_auth: hook to run after Thunderbolt 3 NVM authentication
>   * @request_ring_irq: NHI specific interrupt retrieval hook
>   * @release_ring_irq: NHI specific interrupt release hook
> + * @ring_desc_base: NHI specific hook returning the ring descriptor registers.
> + *		    If not set the standard USB4 NHI registers are used.
> + * @ring_options_base: NHI specific hook returning the ring option registers.
> + *		       If not set the standard USB4 NHI registers are used.
>   * @ring_interrupt_active: NHI specific hook to activate/deactivate the
>   *			   interrupt of a single ring. If not set the
>   *			   standard USB4 NHI registers are used.
> @@ -70,6 +74,8 @@ struct tb_nhi_ops {
>  	void (*post_nvm_auth)(struct tb_nhi *nhi);
>  	int (*request_ring_irq)(struct tb_ring *ring, bool no_suspend);
>  	void (*release_ring_irq)(struct tb_ring *ring);
> +	void __iomem *(*ring_desc_base)(struct tb_ring *ring);
> +	void __iomem *(*ring_options_base)(struct tb_ring *ring);
>  	void (*ring_interrupt_active)(struct tb_ring *ring, bool active);
>  	bool (*is_present)(struct tb_nhi *nhi);
>  	int (*init_interrupts)(struct tb_nhi *nhi);
> 
> -- 
> 2.55.0
>