On 10/27/20 7:14 PM, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <el...@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 <el...@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?
>
