Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>]

Hi Doug,

On 2020-08-18 20:42, Doug Anderson wrote:
> Hi,
> 
<snip>...

> 
> I guess to start, it wasn't obvious (to me) that there were two
> choices and we were picking one.  Mentioning that the other
> alternative was way-based allocation would help a lot.  Even if you
> can't fully explain the differences between the two, adding something
> to the commit message indicating that this is a policy decision (in
> other words, both work but each have their tradeoffs) would help.
> Something like this, if it's correct:
> 
> In general we try to enable capacity based allocation (instead of the
> default way based allocation) since that gives us better performance
> with the current software / hardware configuration.
> 

Thanks, I will add it for next version. Let me also go poke some arch 
teams
to understand if we actually do gain something with this selection, who 
knows
we might get some additional details as well.

>> >
>> > Why are you introducing a whole second table?  Shouldn't you just add
>> > a field to "struct qcom_llcc_config" ?
>> >
>> 
>> This was my 2nd option, first one was to have this based on the 
>> version
>> of LLCC
>> which are exposed by hw info registers. But that didn't turn out good
>> since I
>> couldn't find any relation of this property with LLCC version.
>> 
>> Second option was as you mentioned to have a field to 
>> qcom_llcc_config.
>> Now this is good,
>> but then I thought that if we add LLCC support for 20(random number)
>> SoCs of which
>> 10 is capable of supporting cap_based_alloc and rest 10 are not, then 
>> we
>> will still be adding
>> 20 more lines to each SoC's llcc_config if we follow this 2nd option.
> 
> If you do it right, you'd only need to add lines to the SoCs that need
> it.  Linux conventions in general are that it's OK (and encouraged) to
> rely on the fact that if you don't mention a variable in static
> initialization it's initted to 0/False/NULL.  So if the member
> variable is "need_llcc_config" then you only need to add this in
> places where you're setting it to true.  It only needs to be a boolean
> so if later someone really is worried about all the bytes that flags
> like this are taking up we can use a bitfield.  For now (presumably)
> just adding a boolean would be more efficient since (presumably) the
> extra code needed to access a bitfield would be more than the extra
> data bytes used.  In theory this could also be initdata probably, too.
> 

Yes but in this case we would better be explicit about the capable SoCs
because for someone in QCOM it would be easier to confirm if the SoC is
actually capable but it will not be very obvious for outside folks that
the particular SoC actually supports it. I will use a boolean field 
properly
initialized to indicate if a particular SoC is capable or not.

> 
>> So why not opt for a 3rd option with the table where you just need to
>> specify only the capable
>> targets which is just 10 in our sample case above.
> 
> Are you trying to save space?  ...or complexity?  Sure a good compiler
> will probably pool the constant string here so you won't need to
> allocate it twice, but IMO having a second match table is more
> complex.  You also need at least a pointer + bool per entry.  Each
> probe will now need to parse through all these strings, too.  None of
> this is a big deal, but I'd bet just adding a field to the existing
> struct is lower overhead all around.
> 

Mostly space, but I agree now that the boolean field is indeed better 
than
a separate table.

> 
>> Am I just overthinking this too much and should just go with the 2nd
>> option as you mentioned?
> 
> Someone could feel free to vote against me, but I'd just add to the
> existing config.
> 

I vote for you :)

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation