Re: [PATCH v2 17/19] media: camss: Add SM8250 bandwdith configuration support

[Robert Foss <robert.foss@linaro.org>]

On Wed, 24 Nov 2021 at 18:57, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> Downstream makes some pretty explicit comments about voting for bus
> bandwidth prior to camcc_camnoc_axi_clk_src. Working with camx downstream
> also shows that the bandwidth vote is required to get that root clock
> working.
>
> Add a simple mechanism to declare set and unset named NOCs. Whereas the
> objective is to enable the sm8250 specifically the code has been
> implemented to allow setting of whatever NOCs different SoCs using this
> driver may require.
>
> Tested-by: Julian Grahsl <jgrahsl@snap.com>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss.c | 81 +++++++++++++++++++++++
>  drivers/media/platform/qcom/camss/camss.h | 17 +++++
>  2 files changed, 98 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 066639db9f18..d9905e737d88 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2015-2018 Linaro Ltd.
>   */
>  #include <linux/clk.h>
> +#include <linux/interconnect.h>
>  #include <linux/media-bus-format.h>
>  #include <linux/media.h>
>  #include <linux/module.h>
> @@ -841,6 +842,29 @@ static const struct resources vfe_res_8250[] = {
>         },
>  };
>
> +static const struct resources_icc icc_res_sm8250[] = {
> +       {
> +               .name = "cam_ahb",
> +               .icc_bw_tbl.avg = 38400,
> +               .icc_bw_tbl.peak = 76800,
> +       },
> +       {
> +               .name = "cam_hf_0_mnoc",
> +               .icc_bw_tbl.avg = 2097152,
> +               .icc_bw_tbl.peak = 2097152,
> +       },
> +       {
> +               .name = "cam_sf_0_mnoc",
> +               .icc_bw_tbl.avg = 0,
> +               .icc_bw_tbl.peak = 2097152,
> +       },
> +       {
> +               .name = "cam_sf_icp_mnoc",
> +               .icc_bw_tbl.avg = 2097152,
> +               .icc_bw_tbl.peak = 2097152,
> +       },
> +};
> +
>  /*
>   * camss_add_clock_margin - Add margin to clock frequency rate
>   * @rate: Clock frequency rate
> @@ -1470,6 +1494,29 @@ static int camss_configure_pd(struct camss *camss)
>         return ret;
>  }
>
> +static int camss_icc_get(struct camss *camss)
> +{
> +       const struct resources_icc *icc_res;
> +       int nbr_icc_paths = 0;
> +       int i;
> +
> +       if (camss->version == CAMSS_8250) {
> +               icc_res = &icc_res_sm8250[0];
> +               nbr_icc_paths = ICC_SM8250_COUNT;
> +       }
> +
> +       for (i = 0; i < nbr_icc_paths; i++) {
> +               camss->icc_path[i] = devm_of_icc_get(camss->dev,
> +                                                    icc_res[i].name);
> +               if (IS_ERR(camss->icc_path[i]))
> +                       return PTR_ERR(camss->icc_path[i]);
> +
> +               camss->icc_bw_tbl[i] = icc_res[i].icc_bw_tbl;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * camss_probe - Probe CAMSS platform device
>   * @pdev: Pointer to CAMSS platform device
> @@ -1562,6 +1609,10 @@ static int camss_probe(struct platform_device *pdev)
>                 goto err_cleanup;
>         }
>
> +       ret = camss_icc_get(camss);
> +       if (ret < 0)
> +               goto err_cleanup;
> +
>         ret = camss_init_subdevices(camss);
>         if (ret < 0)
>                 goto err_cleanup;
> @@ -1695,11 +1746,41 @@ MODULE_DEVICE_TABLE(of, camss_dt_match);
>
>  static int __maybe_unused camss_runtime_suspend(struct device *dev)
>  {
> +       struct camss *camss = dev_get_drvdata(dev);
> +       int nbr_icc_paths = 0;
> +       int i;
> +       int ret;
> +
> +       if (camss->version == CAMSS_8250)
> +               nbr_icc_paths = ICC_SM8250_COUNT;
> +
> +       for (i = 0; i < nbr_icc_paths; i++) {
> +               ret = icc_set_bw(camss->icc_path[i], 0, 0);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         return 0;
>  }
>
>  static int __maybe_unused camss_runtime_resume(struct device *dev)
>  {
> +       struct camss *camss = dev_get_drvdata(dev);
> +       int nbr_icc_paths = 0;
> +       int i;
> +       int ret;
> +
> +       if (camss->version == CAMSS_8250)
> +               nbr_icc_paths = ICC_SM8250_COUNT;
> +
> +       for (i = 0; i < nbr_icc_paths; i++) {
> +               ret = icc_set_bw(camss->icc_path[i],
> +                                camss->icc_bw_tbl[i].avg,
> +                                camss->icc_bw_tbl[i].peak);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         return 0;
>  }
>
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 377e2474a485..9c644e638a94 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -56,6 +56,16 @@ struct resources_ispif {
>         char *interrupt;
>  };
>
> +struct icc_bw_tbl {
> +       u32 avg;
> +       u32 peak;
> +};
> +
> +struct resources_icc {
> +       char *name;
> +       struct icc_bw_tbl icc_bw_tbl;
> +};
> +
>  enum pm_domain {
>         PM_DOMAIN_VFE0 = 0,
>         PM_DOMAIN_VFE1 = 1,
> @@ -72,6 +82,11 @@ enum camss_version {
>         CAMSS_8250,
>  };
>
> +enum icc_count {
> +       ICC_DEFAULT_COUNT = 0,
> +       ICC_SM8250_COUNT = 4,
> +};
> +
>  struct camss {
>         enum camss_version version;
>         struct v4l2_device v4l2_dev;
> @@ -88,6 +103,8 @@ struct camss {
>         atomic_t ref_count;
>         struct device *genpd[PM_DOMAIN_GEN2_COUNT];
>         struct device_link *genpd_link[PM_DOMAIN_GEN2_COUNT];
> +       struct icc_path *icc_path[ICC_SM8250_COUNT];
> +       struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
>  };
>
>  struct camss_camera_interface {
> --
> 2.33.0
>

Looks good!

Reviewed-by: Robert Foss <robert.foss@linaro.org>