| Message ID | 20230911-topic-mars-v2-3-3dac84b88c4b@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Venus cleanups | expand |
On 09/02/2024 21:09, 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. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > 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..ea0a7d4601e2 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 Get non-codec Venus clocks. > + * @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-2pcs). (0-2pcs) is hard for me to decode => zero to two parts? Why are we double quoting "main logic" feels a bit "Dr Evil" Suggest hyphenating which would do the same thing: 'one clock for the core-logic||main-logic' > + * > + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic" > + * domain, so this function is the only kind if clk_get necessary there. > + * > + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being > + * statically proclaimed a decoder and core1 an encoder, with both having > + * their own clock domains. "statically defined" not "statically proclaimed" > + * > + * SDM845 features two video cores, each one of which may or may not be > + * subdivided into 2 enc/dec threads. "into two encoder/decoder threads." > + * > + * Other SoCs either feature a single video core (with its own clock domain) > + * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases > + * we treat it the same (CVP only happens to live near-by Venus on the SoC). One not 1 > + * > + * Due to unfortunate developments in the past, we have to support bindings > + * (MSM8996, SDM660, SDM845) that require specifying the clocks and > + * power-domains associated with a video core domain in a bogus subnode, > + * which means that additional fluff is necessary.. "We need to support legacy bindings" "sub-node" > + * > + * Return: 0 on success, negative errno on failure. > + */ > static int venus_clks_get(struct venus_core *core) > { > const struct venus_resources *res = core->res; > With that fixed. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote: > On 09/02/2024 21:09, 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. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> 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..ea0a7d4601e2 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 > > Get non-codec Venus clocks. No, this is not necessarily the case.. these may still include vcodec clocks, that are specified under the root venus node (which is the only way we'd like to keep) I applied the rest of your suggestions, do I keep your rb? Konrad
On 26/03/2024 21:23, Konrad Dybcio wrote: > On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote: >> On 09/02/2024 21:09, 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. >>> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>> --- >>> 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..ea0a7d4601e2 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 >> >> Get non-codec Venus clocks. > > No, this is not necessarily the case.. these may still include > vcodec clocks, that are specified under the root venus node (which > is the only way we'd like to keep) > > I applied the rest of your suggestions, do I keep your rb? > > Konrad > Sure BTW, I plan to test this series when I can - do you have a working tree ? --- bod
On 27.03.2024 10:55 AM, Bryan O'Donoghue wrote: > On 26/03/2024 21:23, Konrad Dybcio wrote: >> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote: >>> On 09/02/2024 21:09, 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. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>> --- >>>> 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..ea0a7d4601e2 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 >>> >>> Get non-codec Venus clocks. >> >> No, this is not necessarily the case.. these may still include >> vcodec clocks, that are specified under the root venus node (which >> is the only way we'd like to keep) >> >> I applied the rest of your suggestions, do I keep your rb? >> >> Konrad >> > > Sure > > BTW, I plan to test this series when I can - do you have a working tree ? next + the patchset in question is a working tree.. Konrad
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c index ac7c83404c6e..ea0a7d4601e2 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-2pcs). + * + * MSM8916 (and possibly other HFIv1 users) only feature the "main logic" + * domain, so this function is the only kind if clk_get necessary there. + * + * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being + * statically proclaimed 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 + * subdivided into 2 enc/dec threads. + * + * Other SoCs either feature a single video core (with its own clock domain) + * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases + * we treat it the same (CVP only happens to live near-by Venus on the SoC). + * + * Due to unfortunate developments in the past, we have to support bindings + * (MSM8996, SDM660, SDM845) that require specifying the clocks and + * power-domains associated with a video core domain in a bogus subnode, + * which means that additional fluff is necessary.. + * + * Return: 0 on success, negative errno on failure. + */ static int venus_clks_get(struct venus_core *core) { const struct venus_resources *res = core->res;
To make it easier to understand the various clock requirements within this driver, add kerneldoc to venus_clk_get() explaining the fluff. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)