[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b0b2ae77-3311-34c8-d1a2-c6f30eca3f1e@intel.com>
Date: Thu, 16 Feb 2023 18:51:08 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Alex Elder <elder@...aro.org>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <caleb.connolly@...aro.org>,
<mka@...omium.org>, <evgreen@...omium.org>, <andersson@...nel.org>,
<quic_cpratapa@...cinc.com>, <quic_avuyyuru@...cinc.com>,
<quic_jponduru@...cinc.com>, <quic_subashab@...cinc.com>,
<elder@...nel.org>, <netdev@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 2/6] net: ipa: kill gsi->virt_raw
From: Alex Elder <elder@...aro.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
Powered by blists - more mailing lists