| Message ID | 20201027161120.5575-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: ipa: minor bug fixes | expand |
On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote: > > The minimum and maximum limits for resources assigned to a given > resource group are programmed in pairs, with the limits for two > groups set in a single register. > > If the number of supported resource groups is odd, only half of the > register that defines these limits is valid for the last group; that > group has no second group in the pair. > > Currently we ignore this constraint, and it turns out to be harmless, If nothing currently calls it with an odd number of registers, is this a bugfix or a new feature (anticipating future expansion, I guess)? > but it is not guaranteed to be. This patch addresses that, and adds > support for programming the 5th resource group's limits. > > Rework how the resource group limit registers are programmed by > having a single function program all group pairs rather than having > one function program each pair. Add the programming of the 4-5 > resource group pair limits to this function. If a resource group is > not supported, pass a null pointer to ipa_resource_config_common() > for that group and have that function write zeroes in that case. > > Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code") > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/net/ipa/ipa_main.c | 89 +++++++++++++++++++++++--------------- > 1 file changed, 53 insertions(+), 36 deletions(-) > > diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c > index 74b1e15ebd6b2..09c8a16d216df 100644 > --- a/drivers/net/ipa/ipa_main.c > +++ b/drivers/net/ipa/ipa_main.c > @@ -370,8 +370,11 @@ static bool ipa_resource_limits_valid(struct ipa *ipa, > u32 i; > u32 j; > > + /* We program at most 6 source or destination resource group limits */ > + BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6); > + > group_count = ipa_resource_group_src_count(ipa->version); > - if (!group_count) > + if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX) > return false; Perhaps more a comment to the previous patch, but _MAX usually denotes the end of an inclusive range, here 5. The previous name COUNT better reflects the number of elements in range [0, 5], which is 6. > /* Return an error if a non-zero resource limit is specified > @@ -387,7 +390,7 @@ static bool ipa_resource_limits_valid(struct ipa *ipa, > } > > group_count = ipa_resource_group_dst_count(ipa->version); > - if (!group_count) > + if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX) > return false; > > for (i = 0; i < data->resource_dst_count; i++) { > @@ -421,46 +424,64 @@ ipa_resource_config_common(struct ipa *ipa, u32 offset, > > val = u32_encode_bits(xlimits->min, X_MIN_LIM_FMASK); > val |= u32_encode_bits(xlimits->max, X_MAX_LIM_FMASK); > - val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK); > - val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK); > + if (ylimits) { > + val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK); > + val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK); > + } > > iowrite32(val, ipa->reg_virt + offset); > } > > -static void ipa_resource_config_src_01(struct ipa *ipa, > - const struct ipa_resource_src *resource) > +static void ipa_resource_config_src(struct ipa *ipa, > + const struct ipa_resource_src *resource) > { > - u32 offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type); > + u32 group_count = ipa_resource_group_src_count(ipa->version); > + const struct ipa_resource_limits *ylimits; > + u32 offset; > > - ipa_resource_config_common(ipa, offset, > - &resource->limits[0], &resource->limits[1]); > -} > + offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type); > + ylimits = group_count == 1 ? NULL : &resource->limits[1]; > + ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits); > > -static void ipa_resource_config_src_23(struct ipa *ipa, > - const struct ipa_resource_src *resource) > -{ > - u32 offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type); > + if (group_count < 2) > + return; > > - ipa_resource_config_common(ipa, offset, > - &resource->limits[2], &resource->limits[3]); > -} > + offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type); > + ylimits = group_count == 3 ? NULL : &resource->limits[3]; > + ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits); > > -static void ipa_resource_config_dst_01(struct ipa *ipa, > - const struct ipa_resource_dst *resource) > -{ > - u32 offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type); > + if (group_count < 4) > + return; > > - ipa_resource_config_common(ipa, offset, > - &resource->limits[0], &resource->limits[1]); > + offset = IPA_REG_SRC_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type); > + ylimits = group_count == 5 ? NULL : &resource->limits[5]; Due to the check > + if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX) > return false; above, group_count can never be greater than 5. Should be greater than?
On 10/27/20 7:14 PM, Willem de Bruijn wrote: > On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote: >> >> The minimum and maximum limits for resources assigned to a given >> resource group are programmed in pairs, with the limits for two >> groups set in a single register. >> >> If the number of supported resource groups is odd, only half of the >> register that defines these limits is valid for the last group; that >> group has no second group in the pair. >> >> Currently we ignore this constraint, and it turns out to be harmless, > > If nothing currently calls it with an odd number of registers, is this > a bugfix or a new feature (anticipating future expansion, I guess)? > >> but it is not guaranteed to be. This patch addresses that, and adds >> support for programming the 5th resource group's limits. >> >> Rework how the resource group limit registers are programmed by >> having a single function program all group pairs rather than having >> one function program each pair. Add the programming of the 4-5 >> resource group pair limits to this function. If a resource group is >> not supported, pass a null pointer to ipa_resource_config_common() >> for that group and have that function write zeroes in that case. >> >> Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code") >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- >> drivers/net/ipa/ipa_main.c | 89 +++++++++++++++++++++++--------------- >> 1 file changed, 53 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c >> index 74b1e15ebd6b2..09c8a16d216df 100644 >> --- a/drivers/net/ipa/ipa_main.c >> +++ b/drivers/net/ipa/ipa_main.c >> @@ -370,8 +370,11 @@ static bool ipa_resource_limits_valid(struct ipa *ipa, >> u32 i; >> u32 j; >> >> + /* We program at most 6 source or destination resource group limits */ >> + BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6); >> + >> group_count = ipa_resource_group_src_count(ipa->version); >> - if (!group_count) >> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX) >> return false; > > Perhaps more a comment to the previous patch, but _MAX usually denotes > the end of an inclusive range, here 5. The previous name COUNT better > reflects the number of elements in range [0, 5], which is 6. I agree with your point, but the max here represents something different from what you're expecting. For a given resource type (source or destination) there is some fixed number (count) of resources available based on the version of SoC. The *driver* can handle any number of them up to the maximum number (max) for any SoC it supports. In that respect, it *does* represent the largest value in an inclusive range. I could change the suffix to something like SRC_COUNT_MAX, but in general the symbol names are longer than I like in this driver and I'm trying to shorten them where possible. If you still want me to change this, please say so and tell me what you suggest I use instead. Your observation below about using ">" rather than ">=" is correct, and applies here as well. That error might have led you to make your comment about "max" representing an inclusive maximum. I will send out a new version of the series to fix that. I'll wait until later today to give some time for more feedback before I prepare them. Thanks a lot for reviewing this. -Alex >> /* Return an error if a non-zero resource limit is specified >> @@ -387,7 +390,7 @@ static bool ipa_resource_limits_valid(struct ipa *ipa, >> } >> >> group_count = ipa_resource_group_dst_count(ipa->version); >> - if (!group_count) >> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX) >> return false; >> >> for (i = 0; i < data->resource_dst_count; i++) { >> @@ -421,46 +424,64 @@ ipa_resource_config_common(struct ipa *ipa, u32 offset, >> >> val = u32_encode_bits(xlimits->min, X_MIN_LIM_FMASK); >> val |= u32_encode_bits(xlimits->max, X_MAX_LIM_FMASK); >> - val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK); >> - val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK); >> + if (ylimits) { >> + val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK); >> + val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK); >> + } >> >> iowrite32(val, ipa->reg_virt + offset); >> } >> >> -static void ipa_resource_config_src_01(struct ipa *ipa, >> - const struct ipa_resource_src *resource) >> +static void ipa_resource_config_src(struct ipa *ipa, >> + const struct ipa_resource_src *resource) >> { >> - u32 offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type); >> + u32 group_count = ipa_resource_group_src_count(ipa->version); >> + const struct ipa_resource_limits *ylimits; >> + u32 offset; >> >> - ipa_resource_config_common(ipa, offset, >> - &resource->limits[0], &resource->limits[1]); >> -} >> + offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type); >> + ylimits = group_count == 1 ? NULL : &resource->limits[1]; >> + ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits); >> >> -static void ipa_resource_config_src_23(struct ipa *ipa, >> - const struct ipa_resource_src *resource) >> -{ >> - u32 offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type); >> + if (group_count < 2) >> + return; >> >> - ipa_resource_config_common(ipa, offset, >> - &resource->limits[2], &resource->limits[3]); >> -} >> + offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type); >> + ylimits = group_count == 3 ? NULL : &resource->limits[3]; >> + ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits); >> >> -static void ipa_resource_config_dst_01(struct ipa *ipa, >> - const struct ipa_resource_dst *resource) >> -{ >> - u32 offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type); >> + if (group_count < 4) >> + return; >> >> - ipa_resource_config_common(ipa, offset, >> - &resource->limits[0], &resource->limits[1]); >> + offset = IPA_REG_SRC_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type); >> + ylimits = group_count == 5 ? NULL : &resource->limits[5]; > > Due to the check > >> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX) >> return false; > > above, group_count can never be greater than 5. Should be greater than? >
On 10/27/20 7:14 PM, Willem de Bruijn wrote: > On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote: >> >> The minimum and maximum limits for resources assigned to a given >> resource group are programmed in pairs, with the limits for two >> groups set in a single register. >> >> If the number of supported resource groups is odd, only half of the >> register that defines these limits is valid for the last group; that >> group has no second group in the pair. >> >> Currently we ignore this constraint, and it turns out to be harmless, > > If nothing currently calls it with an odd number of registers, is this > a bugfix or a new feature (anticipating future expansion, I guess)? . . . Sorry, I missed this comment. Yes, I'm working on support for an upcoming IPA hardware version that has 5 resources of each type. And it is a bug fix, though the bug doesn't bite us (because the maximum number of resources supported is even), so "it turns out to be harmless." -Alex
> >> + /* We program at most 6 source or destination resource group limits */ > >> + BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6); > >> + > >> group_count = ipa_resource_group_src_count(ipa->version); > >> - if (!group_count) > >> + if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX) > >> return false; > > > > Perhaps more a comment to the previous patch, but _MAX usually denotes > > the end of an inclusive range, here 5. The previous name COUNT better > > reflects the number of elements in range [0, 5], which is 6. > > I agree with your point, but the max here represents something different > from what you're expecting. > > For a given resource type (source or destination) there is some fixed > number (count) of resources available based on the version of SoC. > The *driver* can handle any number of them up to the maximum number > (max) for any SoC it supports. In that respect, it *does* represent > the largest value in an inclusive range. > > I could change the suffix to something like SRC_COUNT_MAX, but in > general the symbol names are longer than I like in this driver and > I'm trying to shorten them where possible. Makes sense. Can you then call this out more explicitly in the commit message? That MAX here means max count, not max element id.