Re: [PATCH net-next 2/6] net: ipa: kill gsi->virt_raw

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Alex Elder <elder@linaro.org>
Date: Wed, 15 Feb 2023 13:53:48 -0600

> Starting at IPA v4.5, almost all GSI registers had their offsets
> changed by a fixed amount (shifted downward by 0xd000).  Rather than
> defining offsets for all those registers dependent on version, an
> adjustment was applied for most register accesses.  This was
> implemented in commit cdeee49f3ef7f ("net: ipa: adjust GSI register
> addresses").  It was later modified to be a bit more obvious about
> the adjusment, in commit 571b1e7e58ad3 ("net: ipa: use a separate
> pointer for adjusted GSI memory").

[...]

> @@ -142,27 +127,17 @@ int gsi_reg_init(struct gsi *gsi, struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Make sure we can make our pointer adjustment if necessary */
> -	adjust = gsi->version < IPA_VERSION_4_5 ? 0 : GSI_EE_REG_ADJUST;
> -	if (res->start < adjust) {
> -		dev_err(dev, "DT memory resource \"gsi\" too low (< %u)\n",
> -			adjust);
> -		return -EINVAL;
> -	}
> -
>  	gsi->regs = gsi_regs(gsi);
>  	if (!gsi->regs) {
>  		dev_err(dev, "unsupported IPA version %u (?)\n", gsi->version);
>  		return -EINVAL;
>  	}
>  
> -	gsi->virt_raw = ioremap(res->start, size);
> -	if (!gsi->virt_raw) {
> +	gsi->virt = ioremap(res->start, size);

Now that at least one check above went away and the second one might be
or be not correct (I thought ioremap core takes care of this), can't
just devm_platform_ioremap_resource_byname() be used here for simplicity?

> +	if (!gsi->virt) {
>  		dev_err(dev, "unable to remap \"gsi\" memory\n");
>  		return -ENOMEM;
>  	}
> -	/* Most registers are accessed using an adjusted register range */
> -	gsi->virt = gsi->virt_raw - adjust;
>  
>  	return 0;
>  }
> @@ -170,7 +145,7 @@ int gsi_reg_init(struct gsi *gsi, struct platform_device *pdev)
>  /* Inverse of gsi_reg_init() */
>  void gsi_reg_exit(struct gsi *gsi)
>  {
> +	iounmap(gsi->virt);

(don't forget to remove this unmap if you decide to switch to devm_)

>  	gsi->virt = NULL;
> -	iounmap(gsi->virt_raw);
> -	gsi->virt_raw = NULL;
> +	gsi->regs = NULL;
>  }

[...]

> diff --git a/drivers/net/ipa/reg/gsi_reg-v3.1.c b/drivers/net/ipa/reg/gsi_reg-v3.1.c
> index 651c8a7ed6116..8451d3f8e421e 100644
> --- a/drivers/net/ipa/reg/gsi_reg-v3.1.c
> +++ b/drivers/net/ipa/reg/gsi_reg-v3.1.c
> @@ -8,16 +8,12 @@
>  #include "../reg.h"
>  #include "../gsi_reg.h"
>  
> -/* The inter-EE IRQ registers are relative to gsi->virt_raw (IPA v3.5+) */
> -
>  REG(INTER_EE_SRC_CH_IRQ_MSK, inter_ee_src_ch_irq_msk,
>      0x0000c020 + 0x1000 * GSI_EE_AP);
>  
>  REG(INTER_EE_SRC_EV_CH_IRQ_MSK, inter_ee_src_ev_ch_irq_msk,
>      0x0000c024 + 0x1000 * GSI_EE_AP);
>  
> -/* All other register offsets are relative to gsi->virt */
> -
>  static const u32 reg_ch_c_cntxt_0_fmask[] = {
>  	[CHTYPE_PROTOCOL]				= GENMASK(2, 0),
>  	[CHTYPE_DIR]					= BIT(3),
> @@ -66,10 +62,6 @@ static const u32 reg_error_log_fmask[] = {
>  	[ERR_EE]					= GENMASK(31, 28),
>  };
>  
> -REG_FIELDS(ERROR_LOG, error_log, 0x0001f200 + 0x4000 * GSI_EE_AP);
> -
> -REG(ERROR_LOG_CLR, error_log_clr, 0x0001f210 + 0x4000 * GSI_EE_AP);
> -
>  REG_STRIDE(CH_C_SCRATCH_0, ch_c_scratch_0,
>  	   0x0001c060 + 0x4000 * GSI_EE_AP, 0x80);
>  
> @@ -152,6 +144,7 @@ REG_FIELDS(GSI_STATUS, gsi_status, 0x0001f000 + 0x4000 * GSI_EE_AP);
>  
>  static const u32 reg_ch_cmd_fmask[] = {
>  	[CH_CHID]					= GENMASK(7, 0),
> +						/* Bits 8-23 reserved */
>  	[CH_OPCODE]					= GENMASK(31, 24),
>  };
>  
> @@ -159,6 +152,7 @@ REG_FIELDS(CH_CMD, ch_cmd, 0x0001f008 + 0x4000 * GSI_EE_AP);
>  
>  static const u32 reg_ev_ch_cmd_fmask[] = {
>  	[EV_CHID]					= GENMASK(7, 0),
> +						/* Bits 8-23 reserved */
>  	[EV_OPCODE]					= GENMASK(31, 24),
>  };
>  

[...]

(offtopic)

I hope all those gsi_reg-v*.c are autogenerated? They look pretty scary
to be written and edited manually each time :D

Thanks,
Olek