[v2,20/20] media: venus: pm_helpers: Use reset_bulk API
Commit Message
All of the resets are toggled together. Use the bulk api to save on some
code complexity.
The delay between resets is now correctly determined by the reset
framework.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
drivers/media/platform/qcom/venus/core.c | 15 ++++++++++-----
drivers/media/platform/qcom/venus/core.h | 4 ++--
drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
3 files changed, 15 insertions(+), 19 deletions(-)
Comments
Hi Konrad,
On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
> All of the resets are toggled together. Use the bulk api to save on some
> code complexity.
>
> The delay between resets is now correctly determined by the reset
> framework.
If this is a recent change, could you reference the commit?
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> drivers/media/platform/qcom/venus/core.c | 15 ++++++++++-----
> drivers/media/platform/qcom/venus/core.h | 4 ++--
> drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
> 3 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 873affe17537..ff5601a5ce77 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - for (i = 0; i < res->resets_num; i++) {
> - core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
> - if (IS_ERR(core->resets[i]))
> - return PTR_ERR(core->resets[i]);
> - }
> + core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);
Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
allocation is worth it.
> + if (res->resets_num && !core->resets)
> + return -ENOMEM;
> +
> + for (i = 0; i < res->resets_num; i++)
> + core->resets[i].id = res->resets[i];
> +
> + ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to get resets\n");
>
> ret = venus_get_resources(core);
> if (ret)
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 6ecaa3e38cac..2376b9cbdf2c 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -130,7 +130,7 @@ struct venus_format {
> * @pmdomains: a pointer to a list of pmdomains
> * @opp_dl_venus: an device-link for device OPP
> * @opp_pmdomain: an OPP power-domain
> - * @resets: an array of reset signals
> + * @resets: a reset_control_bulk_data array of hardware reset signals
> * @vdev_dec: a reference to video device structure for decoder instances
> * @vdev_enc: a reference to video device structure for encoder instances
> * @v4l2_dev: a holder for v4l2 device structure
> @@ -183,7 +183,7 @@ struct venus_core {
> struct dev_pm_domain_list *pmdomains;
> struct device_link *opp_dl_venus;
> struct device *opp_pmdomain;
> - struct reset_control *resets[VIDC_RESETS_NUM_MAX];
> + struct reset_control_bulk_data *resets;
Any reason not to just keep this as an array[VIDC_RESETS_NUM_MAX]?
> struct video_device *vdev_dec;
> struct video_device *vdev_enc;
> struct v4l2_device v4l2_dev;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 9df8f2292c17..170fb131cb1e 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
> static int core_resets_reset(struct venus_core *core)
> {
> const struct venus_resources *res = core->res;
> - unsigned int i;
> int ret;
>
> - for (i = 0; i < res->resets_num; i++) {
> - ret = reset_control_assert(core->resets[i]);
> - if (ret)
> - goto err;
> -
> - usleep_range(150, 250);
> - ret = reset_control_deassert(core->resets[i]);
> - if (ret)
> - goto err;
> - }
> + ret = reset_control_bulk_reset(res->resets_num, core->resets);
> + if (ret)
> + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
>
> -err:
> return ret;
Could be simplified to:
return reset_control_bulk_reset(res->resets_num, core-
>resets);
regards
Philipp
On 14.02.2024 14:31, Philipp Zabel wrote:
> Hi Konrad,
>
> On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
>> All of the resets are toggled together. Use the bulk api to save on some
>> code complexity.
>>
>> The delay between resets is now correctly determined by the reset
>> framework.
>
> If this is a recent change, could you reference the commit?
It's a series that recently landed in -next [1]
[...]
>
> Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
> allocation is worth it.
It's 2 today, anyway. I wanted to keep it flexible
[...]
>> + ret = reset_control_bulk_reset(res->resets_num, core->resets);
>> + if (ret)
>> + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
>>
>> -err:
>> return ret;
>
> Could be simplified to:
>
> return reset_control_bulk_reset(res->resets_num, core-
>> resets);
I intentionally kept the if (ret) to print a specific error message
in case the call fails, this driver doesn't go a good job of telling
the user/developer what went wrong.
Konrad
On Mi, 2024-02-14 at 22:20 +0100, Konrad Dybcio wrote:
> On 14.02.2024 14:31, Philipp Zabel wrote:
> > Hi Konrad,
> >
> > On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
> > > All of the resets are toggled together. Use the bulk api to save on some
> > > code complexity.
> > >
> > > The delay between resets is now correctly determined by the reset
> > > framework.
> >
> > If this is a recent change, could you reference the commit?
>
> It's a series that recently landed in -next [1]
Missing link?
> [...]
>
> >
> > Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
> > allocation is worth it.
>
> It's 2 today, anyway. I wanted to keep it flexible
If this is expected to grow, fine.
> [...]
>
> > > + ret = reset_control_bulk_reset(res->resets_num, core->resets);
> > > + if (ret)
> > > + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
> > >
> > > -err:
> > > return ret;
> >
> > Could be simplified to:
> >
> > return reset_control_bulk_reset(res->resets_num, core-
> > > resets);
>
> I intentionally kept the if (ret) to print a specific error message
> in case the call fails, this driver doesn't go a good job of telling
> the user/developer what went wrong.
Oh, ok.
regards
Philipp
On 21.02.2024 14:34, Philipp Zabel wrote:
> On Mi, 2024-02-14 at 22:20 +0100, Konrad Dybcio wrote:
>> On 14.02.2024 14:31, Philipp Zabel wrote:
>>> Hi Konrad,
>>>
>>> On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
>>>> All of the resets are toggled together. Use the bulk api to save on some
>>>> code complexity.
>>>>
>>>> The delay between resets is now correctly determined by the reset
>>>> framework.
>>>
>>> If this is a recent change, could you reference the commit?
>>
>> It's a series that recently landed in -next [1]
>
> Missing link?
Yes, sorry!
Konrad
[1] https://lore.kernel.org/linux-arm-msm/20240105-topic-venus_reset-v2-0-c37eba13b5ce@linaro.org/
On Mi, 2024-02-21 at 14:37 +0100, Konrad Dybcio wrote:
> On 21.02.2024 14:34, Philipp Zabel wrote:
> > On Mi, 2024-02-14 at 22:20 +0100, Konrad Dybcio wrote:
> > > On 14.02.2024 14:31, Philipp Zabel wrote:
> > > > Hi Konrad,
> > > >
> > > > On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
> > > > > All of the resets are toggled together. Use the bulk api to save on some
> > > > > code complexity.
> > > > >
> > > > > The delay between resets is now correctly determined by the reset
> > > > > framework.
> > > >
> > > > If this is a recent change, could you reference the commit?
> > >
> > > It's a series that recently landed in -next [1]
> >
> > Missing link?
>
> Yes, sorry!
>
> Konrad
>
> [1] https://lore.kernel.org/linux-arm-msm/20240105-topic-venus_reset-v2-0-c37eba13b5ce@linaro.org/
Thank you. With that,
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
regards
Philipp
@@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
return ret;
- for (i = 0; i < res->resets_num; i++) {
- core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
- if (IS_ERR(core->resets[i]))
- return PTR_ERR(core->resets[i]);
- }
+ core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);
+ if (res->resets_num && !core->resets)
+ return -ENOMEM;
+
+ for (i = 0; i < res->resets_num; i++)
+ core->resets[i].id = res->resets[i];
+
+ ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get resets\n");
ret = venus_get_resources(core);
if (ret)
@@ -130,7 +130,7 @@ struct venus_format {
* @pmdomains: a pointer to a list of pmdomains
* @opp_dl_venus: an device-link for device OPP
* @opp_pmdomain: an OPP power-domain
- * @resets: an array of reset signals
+ * @resets: a reset_control_bulk_data array of hardware reset signals
* @vdev_dec: a reference to video device structure for decoder instances
* @vdev_enc: a reference to video device structure for encoder instances
* @v4l2_dev: a holder for v4l2 device structure
@@ -183,7 +183,7 @@ struct venus_core {
struct dev_pm_domain_list *pmdomains;
struct device_link *opp_dl_venus;
struct device *opp_pmdomain;
- struct reset_control *resets[VIDC_RESETS_NUM_MAX];
+ struct reset_control_bulk_data *resets;
struct video_device *vdev_dec;
struct video_device *vdev_enc;
struct v4l2_device v4l2_dev;
@@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
static int core_resets_reset(struct venus_core *core)
{
const struct venus_resources *res = core->res;
- unsigned int i;
int ret;
- for (i = 0; i < res->resets_num; i++) {
- ret = reset_control_assert(core->resets[i]);
- if (ret)
- goto err;
-
- usleep_range(150, 250);
- ret = reset_control_deassert(core->resets[i]);
- if (ret)
- goto err;
- }
+ ret = reset_control_bulk_reset(res->resets_num, core->resets);
+ if (ret)
+ dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
-err:
return ret;
}