| Message ID | 20230215195352.755744-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: final GSI register updates | expand |
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
On 2/16/23 11:51 AM, Alexander Lobakin wrote: > 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? Previously, virt_raw would be the "real" re-mapped pointer, and then virt would be adjusted downward from that. It was a weird thing to do, because the result pointed to a non-mapped address. But all uses of the virt pointer added an offset that was enough to put the result into the mapped range. The new code updates all offsets to account for what the adjustment previously did. The test that got removed isn't necessary any more. > >> + 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_) As far as devm_*() calls, I don't use those anywhere in the driver currently. If I were going to use them in one place I'd want do it consistently, everywhere. I don't want to do that. >> 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 I know they look scary, but no, they're manually generated and it's a real pain to review them. I try to be consistent enough that a "diff" is revealing and helpful. For the GSI registers, most of them don't change (until IPA v5.0). I intend to modify this a bit further so that registers that are the same as the previous version don't have to be re-stated (so each new version only has to highlight the differences). All that said, once created, they don't change. Thanks. -Alex > > Thanks, > Olek
From: Alex Elder <elder@linaro.org> Date: Thu, 16 Feb 2023 12:11:11 -0600 > On 2/16/23 11:51 AM, Alexander Lobakin wrote: >> From: Alex Elder <elder@linaro.org> >> Date: Wed, 15 Feb 2023 13:53:48 -0600 [...] >>> 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? > > Previously, virt_raw would be the "real" re-mapped pointer, and then > virt would be adjusted downward from that. It was a weird thing to > do, because the result pointed to a non-mapped address. But all uses > of the virt pointer added an offset that was enough to put the result > into the mapped range. > > The new code updates all offsets to account for what the adjustment > previously did. The test that got removed isn't necessary any more. Yeah I got it, just asked that maybe you can now use platform_ioremap_resource_byname() instead of platform_get_resource_byname() + ioremap() :) > >> >>> + 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_) > > As far as devm_*() calls, I don't use those anywhere in the driver > currently. If I were going to use them in one place I'd want do > it consistently, everywhere. I don't want to do that. + > >>> gsi->virt = NULL; >>> - iounmap(gsi->virt_raw); >>> - gsi->virt_raw = NULL; >>> + gsi->regs = NULL; [...] >> (offtopic) >> >> I hope all those gsi_reg-v*.c are autogenerated? They look pretty scary >> to be written and edited manually each time :D > > I know they look scary, but no, they're manually generated and > it's a real pain to review them. I try to be consistent enough > that a "diff" is revealing and helpful. For the GSI registers, > most of them don't change (until IPA v5.0). I intend to modify > this a bit further so that registers that are the same as the > previous version don't have to be re-stated (so each new version > only has to highlight the differences). No, it's +/- okay to review, as you say, they're pretty consistent in terms of code. > > All that said, once created, they don't change. > > Thanks. > > -Alex Thanks, Olek
On 2/17/23 5:57 AM, Alexander Lobakin wrote: >>> just devm_platform_ioremap_resource_byname() be used here for simplicity? >> Previously, virt_raw would be the "real" re-mapped pointer, and then >> virt would be adjusted downward from that. It was a weird thing to >> do, because the result pointed to a non-mapped address. But all uses >> of the virt pointer added an offset that was enough to put the result >> into the mapped range. >> >> The new code updates all offsets to account for what the adjustment >> previously did. The test that got removed isn't necessary any more. > Yeah I got it, just asked that maybe you can now use > platform_ioremap_resource_byname() instead of > platform_get_resource_byname() + ioremap() :) Sorry, I focused on the "devm" part and not this part. Yes I like that, but let me do that as a follow-on patch, and I think I can do it in more than this spot (possibly three, but I have to look closely). Thanks. -Alex
Hello: This series was applied to netdev/net-next.git (master) by Paolo Abeni <pabeni@redhat.com>: On Wed, 15 Feb 2023 13:53:46 -0600 you wrote: > I believe this is the last set of changes required to allow IPA v5.0 > to be supported. There is a little cleanup work remaining, but that > can happen in the next Linux release cycle. Otherwise we just need > config data and register definitions for IPA v5.0 (and DTS updates). > These are ready but won't be posted without further testing. > > The first patch in this series fixes a minor bug in a patch just > posted, which I found too late. The second eliminates the GSI > memory "adjustment"; this was done previously to avoid/delay the > need to implement a more general way to define GSI register offsets. > Note that this patch causes "checkpatch" warnings due to indentation > that aligns with an open parenthesis. > > [...] Here is the summary with links: - [net-next,1/6] net: ipa: fix an incorrect assignment https://git.kernel.org/netdev/net-next/c/ecfa80ce3b87 - [net-next,2/6] net: ipa: kill gsi->virt_raw https://git.kernel.org/netdev/net-next/c/59b12b1d27f3 - [net-next,3/6] net: ipa: kill ev_ch_e_cntxt_1_length_encode() https://git.kernel.org/netdev/net-next/c/f75f44ddd4cb - [net-next,4/6] net: ipa: avoid setting an undefined field https://git.kernel.org/netdev/net-next/c/62747512ebe6 - [net-next,5/6] net: ipa: support different event ring encoding https://git.kernel.org/netdev/net-next/c/37cd29ec8401 - [net-next,6/6] net: ipa: add HW_PARAM_4 GSI register https://git.kernel.org/netdev/net-next/c/f651334e1ef5 You are awesome, thank you!
On 2/17/23 7:04 AM, Alex Elder wrote: > On 2/17/23 5:57 AM, Alexander Lobakin wrote: >>>> just devm_platform_ioremap_resource_byname() be used here for >>>> simplicity? >>> Previously, virt_raw would be the "real" re-mapped pointer, and then >>> virt would be adjusted downward from that. It was a weird thing to >>> do, because the result pointed to a non-mapped address. But all uses >>> of the virt pointer added an offset that was enough to put the result >>> into the mapped range. >>> >>> The new code updates all offsets to account for what the adjustment >>> previously did. The test that got removed isn't necessary any more. >> Yeah I got it, just asked that maybe you can now use >> platform_ioremap_resource_byname() instead of >> platform_get_resource_byname() + ioremap() :) > > Sorry, I focused on the "devm" part and not this part. > Yes I like that, but let me do that as a follow-on > patch, and I think I can do it in more than this > spot (possibly three, but I have to look closely). Looking at this today, the only OF functions that look up a resource and I/O remap it in one call are devm_*() variants. There is no platform_ioremap_resource_byname() function. One that's available is devm_platform_ioremap_resource_byname(), which could possibly be used in the two locations that call platform_get_resource_byname() followed by ioremap(). As I said earlier, if I were to use any devm_*() function calls the driver, I would want to convert *everything* to use devm_*() variants, and I have no plans to do that at this time. So I will not be implementing your suggestion. -Alex > Thanks. > > -Alex > >
From: Alex Elder <elder@ieee.org> Date: Sun, 5 Mar 2023 10:58:19 -0600 > On 2/17/23 7:04 AM, Alex Elder wrote: >> On 2/17/23 5:57 AM, Alexander Lobakin wrote: >>>>> just devm_platform_ioremap_resource_byname() be used here for >>>>> simplicity? >>>> Previously, virt_raw would be the "real" re-mapped pointer, and then >>>> virt would be adjusted downward from that. It was a weird thing to >>>> do, because the result pointed to a non-mapped address. But all uses >>>> of the virt pointer added an offset that was enough to put the result >>>> into the mapped range. >>>> >>>> The new code updates all offsets to account for what the adjustment >>>> previously did. The test that got removed isn't necessary any more. >>> Yeah I got it, just asked that maybe you can now use >>> platform_ioremap_resource_byname() instead of >>> platform_get_resource_byname() + ioremap() :) >> >> Sorry, I focused on the "devm" part and not this part. >> Yes I like that, but let me do that as a follow-on >> patch, and I think I can do it in more than this >> spot (possibly three, but I have to look closely). > > Looking at this today, the only OF functions that look up a > resource and I/O remap it in one call are devm_*() variants. > There is no platform_ioremap_resource_byname() function. > > One that's available is devm_platform_ioremap_resource_byname(), > which could possibly be used in the two locations that call > platform_get_resource_byname() followed by ioremap(). > > As I said earlier, if I were to use any devm_*() function > calls the driver, I would want to convert *everything* to > use devm_*() variants, and I have no plans to do that at > this time. > > So I will not be implementing your suggestion. Sure. It's fully up to the developers as it doesn't make the code worse in any way. > > -Alex > >> Thanks. >> >> -Alex >> >> > Thanks, Olek