Message-ID: <b0b2ae77-3311-34c8-d1a2-c6f30eca3f1e@intel.com>
Date:   Thu, 16 Feb 2023 18:51:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next 2/6] net: ipa: kill gsi->virt_raw
Content-Language: en-US
To:     Alex Elder <elder@linaro.org>
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <caleb.connolly@linaro.org>,
        <mka@chromium.org>, <evgreen@chromium.org>, <andersson@kernel.org>,
        <quic_cpratapa@quicinc.com>, <quic_avuyyuru@quicinc.com>,
        <quic_jponduru@quicinc.com>, <quic_subashab@quicinc.com>,
        <elder@kernel.org>, <netdev@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230215195352.755744-1-elder@linaro.org>
 <20230215195352.755744-3-elder@linaro.org>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230215195352.755744-3-elder@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WDl5dFhZM1dyM2ZLN2xBeEVHTWVLVFc3eXdCWXNEMGpGRE4zcytMYVFsdFJt?=
 =?utf-8?B?d2hWdTRmY1dSRFB2N1ZaZFJzaG56QXRxVjY1a01wckxwWFFMR1NCUnVwMUNk?=
 =?utf-8?B?R1BxSnZHR2pNRE9mdE5tTWNYa2MyK2UzRWF1c1RsYnR2aXYxSEZualFoWGE4?=
 =?utf-8?B?Z0ppNDBSQXVpTFl4bkRhSDE4Sm1ONUZnemZmWmVFc2swZE1XYkkrSWttSnM5?=
 =?utf-8?B?cGs1aXIvY09MbXZRMFhvUXo3QWJQcWN5blJqR3RDTnIySFFkNk1mUTdPK0p0?=
 =?utf-8?B?bWVMOUFubENSN3VIS1lqS3BXSmI1QVQ3d2ZuNFVGaGZVWmU0aGhWaEJLRmpD?=
 =?utf-8?B?aXhHOHo5SGc1Smw0UEVRZkxXL0ZmZmFWWXlldlRZRWIwcDlXazE2Q3BoV0Q5?=
 =?utf-8?B?akVqMUMzNG9sTnlKNy9HQTV4VXQ4REY5RXBMRXhsNzlzSFdxL0ovN2s1MU1o?=
 =?utf-8?B?TDZSMXpuOW0zd3lyR01XNDlJbFlyQXYva3J4aU43OXRNL3M0QnZKbnRURkJy?=
 =?utf-8?B?N0diL0xxN2I0ZTZaQ0pRcjdDOGJxTXBlVWsxVENlRWErcUplMW83YzJjSkxh?=
 =?utf-8?B?VVZDTGFqQmd5MUZaZ0FGaDB6dW5rdE9ITGVjNTdMSFlJeDB1cXdwMzR4akZ3?=
 =?utf-8?B?c0NxZDErbCs4aWlWaDh1d2s4T0ZoVnkrQ1JQaDg2cUo3QmFQa0FiMHlTUTVt?=
 =?utf-8?B?eTZxZklabUZQa1oyWUt6dzUzbTRJbkVTeWowZmdtdE1OVHFzdWdFdG00OFg2?=
 =?utf-8?B?RnNITW1ybUcwN3IxZjVTK2pmc016WVBGOGhDQXB5UEQwT2FIMTM1enBhbDNv?=
 =?utf-8?B?alg4V1VicFFTNlAyd3BCZks3amJLVVIwbTJJb1Q4OFRWQno4N0JJWnU0Q3Zu?=
 =?utf-8?B?RWd3V2VJRnc2eXlGZE1QeGZqVGhpdG54Zzg2WWtDcmsrOUw2bm1nMzV4bHJ1?=
 =?utf-8?B?MjFVTmNmbWZXeHRXdEFPZGZaM1VhVTF4VENWMk91N0dzZFpPMnRSNVZIZVUr?=
 =?utf-8?B?SExYZURySUFveHNPVTduWHlLOGFiQWJpVi9zYXFWaS96MGRoM0hBMFBIbUho?=
 =?utf-8?B?a0FKN2IvaGNxVkdKOGliZ3RMMHpxYWtWeElBYzVYN01QQUNiUEZYeVhRdE4z?=
 =?utf-8?B?NjlLdXVRVVdQRi9UUGJIWi9jWFNobDZMbnNRSy85Z2Jtc1B6MkxlcDJGaDRI?=
 =?utf-8?B?WHA4R0dBQmtWS1NQY1VBMXJRTmJ5MG1BU09pd0RyME1TNHJRbzZMZWFUNE95?=
 =?utf-8?B?YUhaWVExY01JZ1k3VWlUeWcrOUpSV2NUNGdGeHZQb2NaNGxDanlaOExuQ1RM?=
 =?utf-8?B?YTF0ZGVnZjU1SXhTcWVBNENMVkwyZXZ5by9sWEYyYy8wQ2VYNWhCV29ISW56?=
 =?utf-8?B?VGh5T1NvQnA2RWxHYVNtOW9FL3kwbVRZWllnRlpnWkZqT2IvTEQvU0tqeXVv?=
 =?utf-8?B?M2psVEJCNHBJSGRBUWd6b3k4VzU0aUo1RVNUUGFjbkR3SkN5czBEOWZZdVE1?=
 =?utf-8?B?TWgxNlNEdS9meWt2YTE1bm51SCtGdHRIdzRUVVQrNCtqSVFrcU5tL0dLT0g0?=
 =?utf-8?B?M0p3eWNoTTBMTGNpYkpGQWFrd0RPdktOUTlJUGFFU0JwM05PZ2EwVnlud0hK?=
 =?utf-8?B?Q1lUOEN0L3k4K2gwTmsrR0JIZTM5eE1XUFlpMGFCK29NMlVycmtJNnhlZTVu?=
 =?utf-8?B?U1BiTFFpeFhuMHZBeEU5R1pNTEtJNUVSRU0zT3pEc0NaM3ViZjYvVThIY1hX?=
 =?utf-8?B?U3Z3cVk4Z0l2M3hrWGJGSFlseXYyOVpURjAyNGhxOERQQ0FubG42M1lvWHg0?=
 =?utf-8?B?NWc5em1iOWhscW5EY3RvQUIzc2lLeWYxSkRWVVQrbzFhK2tqR2E5anhwWFZR?=
 =?utf-8?B?ZnZLVkw5OU9UTXJ3bndrTVNqZ0JCdjlQck5yWGs5MHlJVVc5Zld1SHJYY3Rq?=
 =?utf-8?B?Mnc2b1FpejkrQnlSb0NLMDJWR2M1THF4WXFKWDluM1RyQ2U4VTgyOEFRZnBY?=
 =?utf-8?B?YkxCNFZ4MkxyT3NETFR4TzE1UUFqeThEYjVmbFRBR2xzbjhZS3NDVkJCZmVI?=
 =?utf-8?B?cGU3ZnBpUWdrSXFyYUhjNzRNbXlmMjlsVlBwYVl3dVBBbkR0WTFyWWE0d2cy?=
 =?utf-8?B?UWlIcEcwL1gyQ29HQ1VRTTM5STNERjZlSUxocGJvNjQ3Z0ltRSt1NFdZQ3NT?=
 =?utf-8?Q?wzqArLW2i7y4foYwKLQj8hI=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 18accf48-a269-4e84-7566-08db104697d5
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2023 17:52:38.6817
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QFlpS9j428cfcA7dsoZLiS403UsCjzDuTRUQLUX/blr8YFQ/vnG5F6z8pSYahhYcpo7ZFiFdvONviDxXsd21abEHc51WjuN6Ml6TlXJqCwM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4901
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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