| Message ID | 20220910011131.1431934-4-elder@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | net: ipa: a mix of cleanups | expand |
On Fri, 2022-09-09 at 20:11 -0500, Alex Elder wrote: > Move the definition of ipa_version_valid(), making it a static > inline function defined together with the enumerated type in > "ipa_version.h". Define a new count value in the type. > > Rename the function to be ipa_version_supported(), and have it > return true only if the IPA version supplied is explicitly supported > by the driver. I'm wondering if the above is going to cause regressions with some IPA versions suddenly not probed anymore by the module? Additionally there are a few places checking for the now unsupported version[s], I guess that check could/should be removed? e.g. ipa_reg_irq_suspend_en_ee_n_offset(), ipa_reg_irq_suspend_info_ee_n_offset() ... Thanks, Paolo
On 9/20/22 3:29 AM, Paolo Abeni wrote: > On Fri, 2022-09-09 at 20:11 -0500, Alex Elder wrote: >> Move the definition of ipa_version_valid(), making it a static >> inline function defined together with the enumerated type in >> "ipa_version.h". Define a new count value in the type. >> >> Rename the function to be ipa_version_supported(), and have it >> return true only if the IPA version supplied is explicitly supported >> by the driver. > > I'm wondering if the above is going to cause regressions with some IPA > versions suddenly not probed anymore by the module? That is a really good observation. The way versions are handled is a little bit inconsistent. The code is generally written in such a way that *any* version could be used (between a certain minimum and maximum, currently 3.0-4.11). In other words, the *intent* in the code is to make it so that quirks and features that are version-specific are handled the right way, even if we do not (yet) support it. So for example the inline macro rsrc_grp_encoded() returns the mask to use to specify an endpoint's assigned resource group. IPA v4.7 uses one bit, whereas others use two or three bits. We don't "formally" support IPA v4.7, because I (or someone else) haven't set up a Device Tree file and "IPA config data" to test it on real hardware. Still, rsrc_grp_encoded() returns the right value for IPA v4.7, even though it won't be needed until IPA v4.7 is tested and declared supported. The intent is to facilitate adding support for IPA v4.7 (and others). In principle one could simply try it out and it should work, but in reality it is unlikely to be that easy. Finally, as mentioned, to support a version (such as 4.7) we need to create "ipa_data-v4.7.c", which defines a bunch of things that are version-specific. Because those definitions are missing, no IPA v4.7 hardware will be matched by the ipa_match[] table. So the answer to your question is that currently none of the unsupported versions will successfully probe anyway. > Additionally there are a few places checking for the now unsupported > version[s], I guess that check could/should be removed? e.g. > ipa_reg_irq_suspend_en_ee_n_offset(), > ipa_reg_irq_suspend_info_ee_n_offset() > ... I'm a fan of removing unused code like this, but I really would like to actually support these other IPA versions, and I hope the code is close to ready for that. I would just need to get some hardware to test it with (and it needs to rise to the top of my priority list...). Does this make sense to you? Thank you very much for taking the time to review this. -Alex > Thanks, > > Paolo >
On Tue, 2022-09-20 at 07:50 -0500, Alex Elder wrote: > On 9/20/22 3:29 AM, Paolo Abeni wrote: > > On Fri, 2022-09-09 at 20:11 -0500, Alex Elder wrote: > > > Move the definition of ipa_version_valid(), making it a static > > > inline function defined together with the enumerated type in > > > "ipa_version.h". Define a new count value in the type. > > > > > > Rename the function to be ipa_version_supported(), and have it > > > return true only if the IPA version supplied is explicitly supported > > > by the driver. > > > > I'm wondering if the above is going to cause regressions with some IPA > > versions suddenly not probed anymore by the module? > > That is a really good observation. > > The way versions are handled is a little bit inconsistent. The > code is generally written in such a way that *any* version could > be used (between a certain minimum and maximum, currently 3.0-4.11). > In other words, the *intent* in the code is to make it so that > quirks and features that are version-specific are handled the right > way, even if we do not (yet) support it. > > So for example the inline macro rsrc_grp_encoded() returns the > mask to use to specify an endpoint's assigned resource group. > IPA v4.7 uses one bit, whereas others use two or three bits. > We don't "formally" support IPA v4.7, because I (or someone > else) haven't set up a Device Tree file and "IPA config data" > to test it on real hardware. Still, rsrc_grp_encoded() returns > the right value for IPA v4.7, even though it won't be needed > until IPA v4.7 is tested and declared supported. > > The intent is to facilitate adding support for IPA v4.7 (and > others). In principle one could simply try it out and it should > work, but in reality it is unlikely to be that easy. > > Finally, as mentioned, to support a version (such as 4.7) we > need to create "ipa_data-v4.7.c", which defines a bunch of > things that are version-specific. Because those definitions > are missing, no IPA v4.7 hardware will be matched by the > ipa_match[] table. > > So the answer to your question is that currently none of the > unsupported versions will successfully probe anyway. > > > Additionally there are a few places checking for the now unsupported > > version[s], I guess that check could/should be removed? e.g. > > ipa_reg_irq_suspend_en_ee_n_offset(), > > ipa_reg_irq_suspend_info_ee_n_offset() > > ... > > I'm a fan of removing unused code like this, but I really would > like to actually support these other IPA versions, and I hope > the code is close to ready for that. I would just need to get > some hardware to test it with (and it needs to rise to the top > of my priority list...). > > Does this make sense to you? Yes, very clear and detailed explaination, thanks! I'm ok with the series in the current form. Cheers, Paolo
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c index 32962d885acd5..9dfa7f58a207f 100644 --- a/drivers/net/ipa/ipa_main.c +++ b/drivers/net/ipa/ipa_main.c @@ -616,27 +616,6 @@ static void ipa_validate_build(void) field_max(AGGR_GRANULARITY_FMASK)); } -static bool ipa_version_valid(enum ipa_version version) -{ - switch (version) { - case IPA_VERSION_3_0: - case IPA_VERSION_3_1: - case IPA_VERSION_3_5: - case IPA_VERSION_3_5_1: - case IPA_VERSION_4_0: - case IPA_VERSION_4_1: - case IPA_VERSION_4_2: - case IPA_VERSION_4_5: - case IPA_VERSION_4_7: - case IPA_VERSION_4_9: - case IPA_VERSION_4_11: - return true; - - default: - return false; - } -} - /** * ipa_probe() - IPA platform driver probe function * @pdev: Platform device pointer @@ -678,8 +657,8 @@ static int ipa_probe(struct platform_device *pdev) return -ENODEV; } - if (!ipa_version_valid(data->version)) { - dev_err(dev, "invalid IPA version\n"); + if (!ipa_version_supported(data->version)) { + dev_err(dev, "unsupported IPA version %u\n", data->version); return -EINVAL; } diff --git a/drivers/net/ipa/ipa_version.h b/drivers/net/ipa/ipa_version.h index 852d6cbc87758..58f7b43b4db3b 100644 --- a/drivers/net/ipa/ipa_version.h +++ b/drivers/net/ipa/ipa_version.h @@ -19,10 +19,10 @@ * @IPA_VERSION_4_7: IPA version 4.7/GSI version 2.7 * @IPA_VERSION_4_9: IPA version 4.9/GSI version 2.9 * @IPA_VERSION_4_11: IPA version 4.11/GSI version 2.11 (2.1.1) + * @IPA_VERSION_COUNT: Number of defined IPA versions * * Defines the version of IPA (and GSI) hardware present on the platform. - * Please update ipa_version_valid() and ipa_version_string() whenever a - * new version is added. + * Please update ipa_version_string() whenever a new version is added. */ enum ipa_version { IPA_VERSION_3_0, @@ -36,8 +36,24 @@ enum ipa_version { IPA_VERSION_4_7, IPA_VERSION_4_9, IPA_VERSION_4_11, + IPA_VERSION_COUNT, /* Last; not a version */ }; +static inline bool ipa_version_supported(enum ipa_version version) +{ + switch (version) { + case IPA_VERSION_3_1: + case IPA_VERSION_3_5_1: + case IPA_VERSION_4_2: + case IPA_VERSION_4_5: + case IPA_VERSION_4_9: + case IPA_VERSION_4_11: + return true; + default: + return false; + } +} + /* Execution environment IDs */ enum gsi_ee_id { GSI_EE_AP = 0x0,
Move the definition of ipa_version_valid(), making it a static inline function defined together with the enumerated type in "ipa_version.h". Define a new count value in the type. Rename the function to be ipa_version_supported(), and have it return true only if the IPA version supplied is explicitly supported by the driver. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa_main.c | 25 ++----------------------- drivers/net/ipa/ipa_version.h | 20 ++++++++++++++++++-- 2 files changed, 20 insertions(+), 25 deletions(-)