Re: [PATCH v3 03/19] media: venus: pm_helpers: Add kerneldoc to venus_clks_get()

[Vikash Garodia]

Hi Konrad,

On 4/5/2024 1:56 PM, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> To make it easier to understand the various clock requirements within
>> this driver, add kerneldoc to venus_clk_get() explaining the fluff.
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>> ---
>>  drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index ac7c83404c6e..cf91f50a33aa 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -23,6 +23,34 @@
>>  
>>  static bool legacy_binding;
>>  
>> +/**
>> + * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
>> + * @core: A pointer to the venus core resource
>> + *
>> + * The Venus block (depending on the generation) can be split into a couple
>> + * of clock domains: one for main logic and one for each video core (0-2 instances).
s/main logic/controller. Applies to below places as well.

>> + *
>> + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
>> + * domain, so this function is the only kind if clk_get necessary there.
To be checked, unable to get the clock document to see why only core clock
(VENUS0_VCODEC0_CLK). Will update.

>> + *
>> + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
>> + * statically defined a decoder and core1 an encoder, with both having
>> + * their own clock domains.
>> + *
>> + * SDM845 features two video cores, each one of which may or may not be
s/two video cores/two identical video cores
>> + * subdivided into two encoder/decoder threads.
decoder cannot be split into core threads. you can keep it like "each of which
is capable to do any encode or decode"

>> + *
>> + * Other SoCs either feature a single video core (with its own clock domain)
>> + * or one video core and one CVP (Computer Vision Processor) core. In both cases
>> + * we treat it the same way (CVP only happens to live near-by Venus on the SoC).
>> + *
>> + * Due to unfortunate developments in the past, we need to support legacy
> why unfortunate? please re-phrase this.
>> + * bindings (MSM8996, SDM660, SDM845) that require specifying the clocks and
>> + * power-domains associated with a video core domain in a bogus sub-node,
>> + * which means that additional fluff is necessary.
Some background:
It was done that way to support decoder core with specific clocks and similarly
for encoder. Earlier architectures use to have different clock source for these
specific decoder/encoder core clocks, now there is a common clock source for
both the cores. Hence if any one is enabled, others gets enabled as it is
derived from same source.
So if we see the later bindings, the clocks were moved out of sub node to main
venus node.

Regards,
Vikash
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>>  static int venus_clks_get(struct venus_core *core)
>>  {
>>  	const struct venus_resources *res = core->res;
>>
> 
> Thanks,
> Dikshita