diff mbox series

[1/2] media: camss: use v4l2_get_link_freq() to calculate the relevant clocks

Message ID 20210214213404.8373-2-andrey.konovalov@linaro.org
State Superseded
Headers show
Series media: qcom: camss: V4L2_CID_PIXEL_RATE/LINK_FREQ fixes | expand

Commit Message

Andrey Konovalov Feb. 14, 2021, 9:34 p.m. UTC 
There are places in the camss driver where camss_get_pixel_clock() is
called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to
calculate the link frequency from it. There is a case when this would
not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are
read (sampled) from the sensor's pixel array, and this rate is different
from the pixel transmission rate over the CSI link, the link frequency
value can't be calculated from the pixel rate. One needs to use
V4L2_CID_LINK_FREQ to get the link frequency in this case.

Replace such calls to camss_get_pixel_clock() with calls to a wrapper
around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ
first, and if it is not implemented by the camera sensor driver, falls
back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from.

Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates()
are left intact as it looks like this VFE clock does depend on the
rate the pixel samples comes out of the camera sensor, not on the
frequency at which the link between the sensor and the CSI receiver
operates.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 .../media/platform/qcom/camss/camss-csid.c    | 22 ++++++------
 .../qcom/camss/camss-csiphy-2ph-1-0.c         | 22 ++++++------
 .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 ++++++------
 .../media/platform/qcom/camss/camss-csiphy.c  | 36 +++++++++----------
 .../media/platform/qcom/camss/camss-csiphy.h  |  2 +-
 drivers/media/platform/qcom/camss/camss.c     | 23 ++++++++++++
 drivers/media/platform/qcom/camss/camss.h     |  2 ++
 7 files changed, 73 insertions(+), 56 deletions(-)

Comments

Robert Foss Feb. 15, 2021, 11:28 a.m. UTC | #1 
On Mon, 15 Feb 2021 at 12:27, Robert Foss <robert.foss@linaro.org> wrote:
>
> Hey Andrey,
>
> Thanks for sending out this series and picking up Vladimirs patch.
>
> With the below issues fixed feel free to add my r-b.
> Reviewed-by: Robert Foss <robert.foss@linaro.org>

Actually, make that an Acked-by.
Acked-by: Robert Foss <robert.foss@linaro.org>

>
> On Sun, 14 Feb 2021 at 22:34, Andrey Konovalov
> <andrey.konovalov@linaro.org> wrote:
> >
> > There are places in the camss driver where camss_get_pixel_clock() is
> > called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to
> > calculate the link frequency from it. There is a case when this would
> > not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are
> > read (sampled) from the sensor's pixel array, and this rate is different
> > from the pixel transmission rate over the CSI link, the link frequency
> > value can't be calculated from the pixel rate. One needs to use
> > V4L2_CID_LINK_FREQ to get the link frequency in this case.
> >
> > Replace such calls to camss_get_pixel_clock() with calls to a wrapper
> > around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ
> > first, and if it is not implemented by the camera sensor driver, falls
> > back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from.
> >
> > Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates()
> > are left intact as it looks like this VFE clock does depend on the
> > rate the pixel samples comes out of the camera sensor, not on the
> > frequency at which the link between the sensor and the CSI receiver
> > operates.
> >
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > ---
> >  .../media/platform/qcom/camss/camss-csid.c    | 22 ++++++------
> >  .../qcom/camss/camss-csiphy-2ph-1-0.c         | 22 ++++++------
> >  .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 ++++++------
> >  .../media/platform/qcom/camss/camss-csiphy.c  | 36 +++++++++----------
> >  .../media/platform/qcom/camss/camss-csiphy.h  |  2 +-
> >  drivers/media/platform/qcom/camss/camss.c     | 23 ++++++++++++
> >  drivers/media/platform/qcom/camss/camss.h     |  2 ++
> >  7 files changed, 73 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> > index be3fe76f3dc3..b2cbf4b65949 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csid.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> > @@ -462,13 +462,19 @@ static irqreturn_t csid_isr(int irq, void *dev)
> >  static int csid_set_clock_rates(struct csid_device *csid)
> >  {
> >         struct device *dev = csid->camss->dev;
> > -       u32 pixel_clock;
> > +       s64 link_freq;
> >         int i, j;
> >         int ret;
> >
> > -       ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock);
> > -       if (ret)
> > -               pixel_clock = 0;
> > +       const struct csid_format *f = csid_get_fmt_entry(
> > +               csid->formats,
> > +               csid->nformats,
> > +               csid->fmt[MSM_CSIPHY_PAD_SINK].code);
> > +       u8 num_lanes = csid->phy.lane_cnt;
> > +       link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,
> > +                                       2 * num_lanes);
>
> Checkpatch lists the following error:
>
> WARNING: Missing a blank line after declarations
> #59: FILE: drivers/media/platform/qcom/camss/camss-csid.c:474:
> +       u8 num_lanes = csid->phy.lane_cnt;
> +       link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,
>
>
> > +       if (link_freq < 0)
> > +               link_freq = 0;
> >
> >         for (i = 0; i < csid->nclocks; i++) {
> >                 struct camss_clock *clock = &csid->clock[i];
> > @@ -477,13 +483,7 @@ static int csid_set_clock_rates(struct csid_device *csid)
> >                     !strcmp(clock->name, "csi1") ||
> >                     !strcmp(clock->name, "csi2") ||
> >                     !strcmp(clock->name, "csi3")) {
> > -                       const struct csid_format *f = csid_get_fmt_entry(
> > -                               csid->formats,
> > -                               csid->nformats,
> > -                               csid->fmt[MSM_CSIPHY_PAD_SINK].code);
> > -                       u8 num_lanes = csid->phy.lane_cnt;
> > -                       u64 min_rate = pixel_clock * f->bpp /
> > -                                                       (2 * num_lanes * 4);
> > +                       u64 min_rate = link_freq / 4;
> >                         long rate;
> >
> >                         camss_add_clock_margin(&min_rate);
> > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> > index 12bce391d71f..30b454c369ab 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> > @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy)
> >   *
> >   * Helper function to calculate settle count value. This is
> >   * based on the CSI2 T_hs_settle parameter which in turn
> > - * is calculated based on the CSI2 transmitter pixel clock
> > - * frequency.
> > + * is calculated based on the CSI2 transmitter link frequency.
> >   *
> > - * Return settle count value or 0 if the CSI2 pixel clock
> > - * frequency is not available
> > + * Return settle count value or 0 if the CSI2 link frequency
> > + * is not available
> >   */
> > -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> > -                                u32 timer_clk_rate)
> > +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
> >  {
> > -       u32 mipi_clock; /* Hz */
> >         u32 ui; /* ps */
> >         u32 timer_period; /* ps */
> >         u32 t_hs_prepare_max; /* ps */
> > @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> >         u32 t_hs_settle; /* ps */
> >         u8 settle_cnt;
> >
> > -       mipi_clock = pixel_clock * bpp / (2 * num_lanes);
> > -       ui = div_u64(1000000000000LL, mipi_clock);
> > +       if (link_freq <= 0)
> > +               return 0;
> > +
> > +       ui = div_u64(1000000000000LL, link_freq);
> >         ui /= 2;
> >         t_hs_prepare_max = 85000 + 6 * ui;
> >         t_hs_prepare_zero_min = 145000 + 10 * ui;
> > @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> >
> >  static void csiphy_lanes_enable(struct csiphy_device *csiphy,
> >                                 struct csiphy_config *cfg,
> > -                               u32 pixel_clock, u8 bpp, u8 lane_mask)
> > +                               s64 link_freq, u8 lane_mask)
> >  {
> >         struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
> >         u8 settle_cnt;
> >         u8 val, l = 0;
> >         int i = 0;
> >
> > -       settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
> > -                                           csiphy->timer_clk_rate);
> > +       settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
> >
> >         writel_relaxed(0x1, csiphy->base +
> >                        CAMSS_CSI_PHY_GLBL_T_INIT_CFG0);
> > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> > index 97cb9de85031..da7c3d3f9a10 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> > @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev)
> >   *
> >   * Helper function to calculate settle count value. This is
> >   * based on the CSI2 T_hs_settle parameter which in turn
> > - * is calculated based on the CSI2 transmitter pixel clock
> > - * frequency.
> > + * is calculated based on the CSI2 transmitter link frequency.
> >   *
> > - * Return settle count value or 0 if the CSI2 pixel clock
> > - * frequency is not available
> > + * Return settle count value or 0 if the CSI2 link frequency
> > + * is not available
> >   */
> > -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> > -                                u32 timer_clk_rate)
> > +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
> >  {
> > -       u32 mipi_clock; /* Hz */
> >         u32 ui; /* ps */
> >         u32 timer_period; /* ps */
> >         u32 t_hs_prepare_max; /* ps */
> >         u32 t_hs_settle; /* ps */
> >         u8 settle_cnt;
> >
> > -       mipi_clock = pixel_clock * bpp / (2 * num_lanes);
> > -       ui = div_u64(1000000000000LL, mipi_clock);
> > +       if (link_freq <= 0)
> > +               return 0;
> > +
> > +       ui = div_u64(1000000000000LL, link_freq);
> >         ui /= 2;
> >         t_hs_prepare_max = 85000 + 6 * ui;
> >         t_hs_settle = t_hs_prepare_max;
> > @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> >
> >  static void csiphy_lanes_enable(struct csiphy_device *csiphy,
> >                                 struct csiphy_config *cfg,
> > -                               u32 pixel_clock, u8 bpp, u8 lane_mask)
> > +                               s64 link_freq, u8 lane_mask)
> >  {
> >         struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
> >         u8 settle_cnt;
> >         u8 val, l = 0;
> >         int i;
> >
> > -       settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
> > -                                           csiphy->timer_clk_rate);
> > +       settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
> >
> >         val = BIT(c->clk.pos);
> >         for (i = 0; i < c->num_data; i++)
> > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> > index 509c9a59c09c..9b5fe6fc7664 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> > @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats,
> >  static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
> >  {
> >         struct device *dev = csiphy->camss->dev;
> > -       u32 pixel_clock;
> > +       s64 link_freq;
> >         int i, j;
> >         int ret;
> >
> > -       ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
> > -       if (ret)
> > -               pixel_clock = 0;
> > +       u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
> > +                               csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> > +       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
> > +       link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
> > +                                       2 * num_lanes);
> > +       if (link_freq < 0)
> > +               link_freq  = 0;
> >
> >         for (i = 0; i < csiphy->nclocks; i++) {
> >                 struct camss_clock *clock = &csiphy->clock[i];
> >
> >                 if (csiphy->rate_set[i]) {
> > -                       u8 bpp = csiphy_get_bpp(csiphy->formats,
> > -                                       csiphy->nformats,
> > -                                       csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> > -                       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
> > -                       u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4);
> > +                       u64 min_rate = link_freq / 4;
> >                         long round_rate;
> >
> >                         camss_add_clock_margin(&min_rate);
> > @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
> >  static int csiphy_stream_on(struct csiphy_device *csiphy)
> >  {
> >         struct csiphy_config *cfg = &csiphy->cfg;
> > -       u32 pixel_clock;
> > +       s64 link_freq;
>
> Checkpatch throws the following error:
>
> WARNING: Missing a blank line after declarations
> #211: FILE: drivers/media/platform/qcom/camss/camss-csiphy.c:112:
> +       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
> +       link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>
>
>
> >         u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg);
> >         u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
> >                                 csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> > +       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
> >         u8 val;
> > -       int ret;
> >
> > -       ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
> > -       if (ret) {
> > -               dev_err(csiphy->camss->dev,
> > -                       "Cannot get CSI2 transmitter's pixel clock\n");
> > -               return -EINVAL;
> > -       }
> > -       if (!pixel_clock) {
> > +       link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
> > +                                       2 * num_lanes);
> > +       if (link_freq < 0) {
> >                 dev_err(csiphy->camss->dev,
> > -                       "Got pixel clock == 0, cannot continue\n");
> > +                       "Cannot get CSI2 transmitter's link frequency\n");
> >                 return -EINVAL;
> >         }
> >
> > @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
> >         writel_relaxed(val, csiphy->base_clk_mux);
> >         wmb();
> >
> > -       csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask);
> > +       csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask);
> >
> >         return 0;
> >  }
> > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> > index f7967ef836dc..d71b8bc6ec00 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> > @@ -50,7 +50,7 @@ struct csiphy_hw_ops {
> >         void (*reset)(struct csiphy_device *csiphy);
> >         void (*lanes_enable)(struct csiphy_device *csiphy,
> >                              struct csiphy_config *cfg,
> > -                            u32 pixel_clock, u8 bpp, u8 lane_mask);
> > +                            s64 link_freq, u8 lane_mask);
> >         void (*lanes_disable)(struct csiphy_device *csiphy,
> >                               struct csiphy_config *cfg);
> >         irqreturn_t (*isr)(int irq, void *dev);
> > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > index 7c0f669f8aa6..2888c7ef2303 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity)
> >         }
> >  }
> >
> > +/**
> > + * camss_get_link_freq - Get link frequency from sensor
> > + * @entity: Media entity in the current pipeline
> > + * @bpp: Number of bits per pixel for the current format
> > + * @lanes: Number of lanes in the link to the sensor
> > + *
> > + * Return link frequency on success or a negative error code otherwise
> > + */
> > +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
> > +                       unsigned int lanes)
> > +{
> > +       struct media_entity *sensor;
> > +       struct v4l2_subdev *subdev;
> > +
> > +       sensor = camss_find_sensor(entity);
> > +       if (!sensor)
> > +               return -ENODEV;
> > +
> > +       subdev = media_entity_to_v4l2_subdev(sensor);
> > +
> > +       return v4l2_get_link_freq(subdev->ctrl_handler, bpp, lanes);
> > +}
> > +
> >  /*
> >   * camss_get_pixel_clock - Get pixel clock rate from sensor
> >   * @entity: Media entity in the current pipeline
> > diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> > index 3a0484683cd6..86cdc25189eb 100644
> > --- a/drivers/media/platform/qcom/camss/camss.h
> > +++ b/drivers/media/platform/qcom/camss/camss.h
> > @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock,
> >                         struct device *dev);
> >  void camss_disable_clocks(int nclocks, struct camss_clock *clock);
> >  struct media_entity *camss_find_sensor(struct media_entity *entity);
> > +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
> > +                       unsigned int lanes);
> >  int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
> >  int camss_pm_domain_on(struct camss *camss, int id);
> >  void camss_pm_domain_off(struct camss *camss, int id);
> > --
> > 2.17.1
> >
Jacopo Mondi Feb. 15, 2021, noon UTC | #2 
Hi Andrey,
   nice to see progress in this direction

On Mon, Feb 15, 2021 at 12:34:03AM +0300, Andrey Konovalov wrote:
> There are places in the camss driver where camss_get_pixel_clock() is
> called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to
> calculate the link frequency from it. There is a case when this would
> not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are
> read (sampled) from the sensor's pixel array, and this rate is different
> from the pixel transmission rate over the CSI link, the link frequency
> value can't be calculated from the pixel rate. One needs to use
> V4L2_CID_LINK_FREQ to get the link frequency in this case.
>
> Replace such calls to camss_get_pixel_clock() with calls to a wrapper
> around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ
> first, and if it is not implemented by the camera sensor driver, falls
> back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from.

Is it worth warning in the core function that the subdevice should
support LINK_FREQ and if we fallback to use PIXEL_RATE the calculation
result might not be accurate ?

>
> Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates()
> are left intact as it looks like this VFE clock does depend on the
> rate the pixel samples comes out of the camera sensor, not on the
> frequency at which the link between the sensor and the CSI receiver
> operates.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    | 22 ++++++------
>  .../qcom/camss/camss-csiphy-2ph-1-0.c         | 22 ++++++------
>  .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 ++++++------
>  .../media/platform/qcom/camss/camss-csiphy.c  | 36 +++++++++----------
>  .../media/platform/qcom/camss/camss-csiphy.h  |  2 +-
>  drivers/media/platform/qcom/camss/camss.c     | 23 ++++++++++++
>  drivers/media/platform/qcom/camss/camss.h     |  2 ++
>  7 files changed, 73 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index be3fe76f3dc3..b2cbf4b65949 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -462,13 +462,19 @@ static irqreturn_t csid_isr(int irq, void *dev)
>  static int csid_set_clock_rates(struct csid_device *csid)
>  {
>  	struct device *dev = csid->camss->dev;
> -	u32 pixel_clock;
> +	s64 link_freq;
>  	int i, j;
>  	int ret;
>
> -	ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock);
> -	if (ret)
> -		pixel_clock = 0;
> +	const struct csid_format *f = csid_get_fmt_entry(
> +		csid->formats,
> +		csid->nformats,
> +		csid->fmt[MSM_CSIPHY_PAD_SINK].code);

Weird indent :/

I would either keep the arguments on one line or align after the open
( if it doesn't go past 80-cols


> +	u8 num_lanes = csid->phy.lane_cnt;
> +	link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,

Empy line maybe ?

> +					2 * num_lanes);

I see you pass in 2 * num_lanes and I assume it's for CSI-2 DDR.
Can't this be handled in camss_get_link_freq() so that you here only
pass in the actual number of lanes ?
> +	if (link_freq < 0)
> +		link_freq = 0;

I don't know this driver, but I wonder if it wouldn't be better to
fail instead of defaulting to 0, which might be dangerous if used as a
divider.

>
>  	for (i = 0; i < csid->nclocks; i++) {
>  		struct camss_clock *clock = &csid->clock[i];
> @@ -477,13 +483,7 @@ static int csid_set_clock_rates(struct csid_device *csid)
>  		    !strcmp(clock->name, "csi1") ||
>  		    !strcmp(clock->name, "csi2") ||
>  		    !strcmp(clock->name, "csi3")) {
> -			const struct csid_format *f = csid_get_fmt_entry(
> -				csid->formats,
> -				csid->nformats,
> -				csid->fmt[MSM_CSIPHY_PAD_SINK].code);
> -			u8 num_lanes = csid->phy.lane_cnt;
> -			u64 min_rate = pixel_clock * f->bpp /
> -							(2 * num_lanes * 4);
> +			u64 min_rate = link_freq / 4;

Why 4 ? :)

>  			long rate;
>
>  			camss_add_clock_margin(&min_rate);
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> index 12bce391d71f..30b454c369ab 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
> @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy)
>   *
>   * Helper function to calculate settle count value. This is
>   * based on the CSI2 T_hs_settle parameter which in turn
> - * is calculated based on the CSI2 transmitter pixel clock
> - * frequency.
> + * is calculated based on the CSI2 transmitter link frequency.
>   *
> - * Return settle count value or 0 if the CSI2 pixel clock
> - * frequency is not available
> + * Return settle count value or 0 if the CSI2 link frequency
> + * is not available
>   */
> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> -				 u32 timer_clk_rate)
> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>  {
> -	u32 mipi_clock; /* Hz */
>  	u32 ui; /* ps */
>  	u32 timer_period; /* ps */
>  	u32 t_hs_prepare_max; /* ps */
> @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>  	u32 t_hs_settle; /* ps */
>  	u8 settle_cnt;
>
> -	mipi_clock = pixel_clock * bpp / (2 * num_lanes);
> -	ui = div_u64(1000000000000LL, mipi_clock);
> +	if (link_freq <= 0)
> +		return 0;

If you error out if the link frequency cannot be calculated, can this
be skipped ?

> +
> +	ui = div_u64(1000000000000LL, link_freq);
>  	ui /= 2;
>  	t_hs_prepare_max = 85000 + 6 * ui;
>  	t_hs_prepare_zero_min = 145000 + 10 * ui;
> @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>
>  static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>  				struct csiphy_config *cfg,
> -				u32 pixel_clock, u8 bpp, u8 lane_mask)
> +				s64 link_freq, u8 lane_mask)
>  {
>  	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>  	u8 settle_cnt;
>  	u8 val, l = 0;
>  	int i = 0;
>
> -	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
> -					    csiphy->timer_clk_rate);
> +	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>
>  	writel_relaxed(0x1, csiphy->base +
>  		       CAMSS_CSI_PHY_GLBL_T_INIT_CFG0);
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index 97cb9de85031..da7c3d3f9a10 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev)
>   *
>   * Helper function to calculate settle count value. This is
>   * based on the CSI2 T_hs_settle parameter which in turn
> - * is calculated based on the CSI2 transmitter pixel clock
> - * frequency.
> + * is calculated based on the CSI2 transmitter link frequency.
>   *
> - * Return settle count value or 0 if the CSI2 pixel clock
> - * frequency is not available
> + * Return settle count value or 0 if the CSI2 link frequency
> + * is not available
>   */
> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
> -				 u32 timer_clk_rate)
> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>  {
> -	u32 mipi_clock; /* Hz */
>  	u32 ui; /* ps */
>  	u32 timer_period; /* ps */
>  	u32 t_hs_prepare_max; /* ps */
>  	u32 t_hs_settle; /* ps */
>  	u8 settle_cnt;
>
> -	mipi_clock = pixel_clock * bpp / (2 * num_lanes);
> -	ui = div_u64(1000000000000LL, mipi_clock);
> +	if (link_freq <= 0)
> +		return 0;
> +
> +	ui = div_u64(1000000000000LL, link_freq);
>  	ui /= 2;
>  	t_hs_prepare_max = 85000 + 6 * ui;
>  	t_hs_settle = t_hs_prepare_max;
> @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>
>  static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>  				struct csiphy_config *cfg,
> -				u32 pixel_clock, u8 bpp, u8 lane_mask)
> +				s64 link_freq, u8 lane_mask)
>  {
>  	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>  	u8 settle_cnt;
>  	u8 val, l = 0;
>  	int i;
>
> -	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
> -					    csiphy->timer_clk_rate);
> +	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>
>  	val = BIT(c->clk.pos);
>  	for (i = 0; i < c->num_data; i++)
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 509c9a59c09c..9b5fe6fc7664 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats,
>  static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
>  {
>  	struct device *dev = csiphy->camss->dev;
> -	u32 pixel_clock;
> +	s64 link_freq;
>  	int i, j;
>  	int ret;
>
> -	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
> -	if (ret)
> -		pixel_clock = 0;
> +	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
> +				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> +	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
> +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
> +					2 * num_lanes);
> +	if (link_freq < 0)
> +		link_freq  = 0;
>
>  	for (i = 0; i < csiphy->nclocks; i++) {
>  		struct camss_clock *clock = &csiphy->clock[i];
>
>  		if (csiphy->rate_set[i]) {
> -			u8 bpp = csiphy_get_bpp(csiphy->formats,
> -					csiphy->nformats,
> -					csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> -			u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
> -			u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4);
> +			u64 min_rate = link_freq / 4;
>  			long round_rate;
>
>  			camss_add_clock_margin(&min_rate);
> @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
>  static int csiphy_stream_on(struct csiphy_device *csiphy)
>  {
>  	struct csiphy_config *cfg = &csiphy->cfg;
> -	u32 pixel_clock;
> +	s64 link_freq;
>  	u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg);
>  	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
>  				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> +	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>  	u8 val;
> -	int ret;
>
> -	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
> -	if (ret) {
> -		dev_err(csiphy->camss->dev,
> -			"Cannot get CSI2 transmitter's pixel clock\n");
> -		return -EINVAL;
> -	}
> -	if (!pixel_clock) {
> +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
> +					2 * num_lanes);
> +	if (link_freq < 0) {
>  		dev_err(csiphy->camss->dev,
> -			"Got pixel clock == 0, cannot continue\n");
> +			"Cannot get CSI2 transmitter's link frequency\n");
>  		return -EINVAL;
>  	}
>
> @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
>  	writel_relaxed(val, csiphy->base_clk_mux);
>  	wmb();
>
> -	csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask);
> +	csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask);
>
>  	return 0;
>  }
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index f7967ef836dc..d71b8bc6ec00 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -50,7 +50,7 @@ struct csiphy_hw_ops {
>  	void (*reset)(struct csiphy_device *csiphy);
>  	void (*lanes_enable)(struct csiphy_device *csiphy,
>  			     struct csiphy_config *cfg,
> -			     u32 pixel_clock, u8 bpp, u8 lane_mask);
> +			     s64 link_freq, u8 lane_mask);
>  	void (*lanes_disable)(struct csiphy_device *csiphy,
>  			      struct csiphy_config *cfg);
>  	irqreturn_t (*isr)(int irq, void *dev);
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 7c0f669f8aa6..2888c7ef2303 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity)
>  	}
>  }
>
> +/**
> + * camss_get_link_freq - Get link frequency from sensor
> + * @entity: Media entity in the current pipeline
> + * @bpp: Number of bits per pixel for the current format
> + * @lanes: Number of lanes in the link to the sensor
> + *
> + * Return link frequency on success or a negative error code otherwise
> + */
> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
> +			unsigned int lanes)
> +{
> +	struct media_entity *sensor;
> +	struct v4l2_subdev *subdev;
> +
> +	sensor = camss_find_sensor(entity);
> +	if (!sensor)
> +		return -ENODEV;

Can this happen ?

Thanks
  j

> +
> +	subdev = media_entity_to_v4l2_subdev(sensor);
> +
> +	return v4l2_get_link_freq(subdev->ctrl_handler, bpp, lanes);
> +}
> +
>  /*
>   * camss_get_pixel_clock - Get pixel clock rate from sensor
>   * @entity: Media entity in the current pipeline
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 3a0484683cd6..86cdc25189eb 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>  			struct device *dev);
>  void camss_disable_clocks(int nclocks, struct camss_clock *clock);
>  struct media_entity *camss_find_sensor(struct media_entity *entity);
> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
> +			unsigned int lanes);
>  int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
>  int camss_pm_domain_on(struct camss *camss, int id);
>  void camss_pm_domain_off(struct camss *camss, int id);
> --
> 2.17.1
>
Andrey Konovalov Feb. 15, 2021, 4:10 p.m. UTC | #3 
Hi Robert,

Thanks for your review!

On 15.02.2021 14:28, Robert Foss wrote:
> On Mon, 15 Feb 2021 at 12:27, Robert Foss <robert.foss@linaro.org> wrote:
>>
>> Hey Andrey,
>>
>> Thanks for sending out this series and picking up Vladimirs patch.
>>
>> With the below issues fixed feel free to add my r-b.
>> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> 
> Actually, make that an Acked-by.
> Acked-by: Robert Foss <robert.foss@linaro.org>

OK, thanks!

>>
>> On Sun, 14 Feb 2021 at 22:34, Andrey Konovalov
>> <andrey.konovalov@linaro.org> wrote:
>>>
>>> There are places in the camss driver where camss_get_pixel_clock() is
>>> called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to
>>> calculate the link frequency from it. There is a case when this would
>>> not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are
>>> read (sampled) from the sensor's pixel array, and this rate is different
>>> from the pixel transmission rate over the CSI link, the link frequency
>>> value can't be calculated from the pixel rate. One needs to use
>>> V4L2_CID_LINK_FREQ to get the link frequency in this case.
>>>
>>> Replace such calls to camss_get_pixel_clock() with calls to a wrapper
>>> around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ
>>> first, and if it is not implemented by the camera sensor driver, falls
>>> back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from.
>>>
>>> Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates()
>>> are left intact as it looks like this VFE clock does depend on the
>>> rate the pixel samples comes out of the camera sensor, not on the
>>> frequency at which the link between the sensor and the CSI receiver
>>> operates.
>>>
>>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>>> ---
>>>   .../media/platform/qcom/camss/camss-csid.c    | 22 ++++++------
>>>   .../qcom/camss/camss-csiphy-2ph-1-0.c         | 22 ++++++------
>>>   .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 ++++++------
>>>   .../media/platform/qcom/camss/camss-csiphy.c  | 36 +++++++++----------
>>>   .../media/platform/qcom/camss/camss-csiphy.h  |  2 +-
>>>   drivers/media/platform/qcom/camss/camss.c     | 23 ++++++++++++
>>>   drivers/media/platform/qcom/camss/camss.h     |  2 ++
>>>   7 files changed, 73 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
>>> index be3fe76f3dc3..b2cbf4b65949 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>>> @@ -462,13 +462,19 @@ static irqreturn_t csid_isr(int irq, void *dev)
>>>   static int csid_set_clock_rates(struct csid_device *csid)
>>>   {
>>>          struct device *dev = csid->camss->dev;
>>> -       u32 pixel_clock;
>>> +       s64 link_freq;
>>>          int i, j;
>>>          int ret;
>>>
>>> -       ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock);
>>> -       if (ret)
>>> -               pixel_clock = 0;
>>> +       const struct csid_format *f = csid_get_fmt_entry(
>>> +               csid->formats,
>>> +               csid->nformats,
>>> +               csid->fmt[MSM_CSIPHY_PAD_SINK].code);
>>> +       u8 num_lanes = csid->phy.lane_cnt;
>>> +       link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,
>>> +                                       2 * num_lanes);
>>
>> Checkpatch lists the following error:

Oops... indeed.

>> WARNING: Missing a blank line after declarations
>> #59: FILE: drivers/media/platform/qcom/camss/camss-csid.c:474:
>> +       u8 num_lanes = csid->phy.lane_cnt;
>> +       link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,
>>
>>
>>> +       if (link_freq < 0)
>>> +               link_freq = 0;
>>>
>>>          for (i = 0; i < csid->nclocks; i++) {
>>>                  struct camss_clock *clock = &csid->clock[i];
>>> @@ -477,13 +483,7 @@ static int csid_set_clock_rates(struct csid_device *csid)
>>>                      !strcmp(clock->name, "csi1") ||
>>>                      !strcmp(clock->name, "csi2") ||
>>>                      !strcmp(clock->name, "csi3")) {
>>> -                       const struct csid_format *f = csid_get_fmt_entry(
>>> -                               csid->formats,
>>> -                               csid->nformats,
>>> -                               csid->fmt[MSM_CSIPHY_PAD_SINK].code);
>>> -                       u8 num_lanes = csid->phy.lane_cnt;
>>> -                       u64 min_rate = pixel_clock * f->bpp /
>>> -                                                       (2 * num_lanes * 4);
>>> +                       u64 min_rate = link_freq / 4;
>>>                          long rate;
>>>
>>>                          camss_add_clock_margin(&min_rate);
>>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
>>> index 12bce391d71f..30b454c369ab 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
>>> @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy)
>>>    *
>>>    * Helper function to calculate settle count value. This is
>>>    * based on the CSI2 T_hs_settle parameter which in turn
>>> - * is calculated based on the CSI2 transmitter pixel clock
>>> - * frequency.
>>> + * is calculated based on the CSI2 transmitter link frequency.
>>>    *
>>> - * Return settle count value or 0 if the CSI2 pixel clock
>>> - * frequency is not available
>>> + * Return settle count value or 0 if the CSI2 link frequency
>>> + * is not available
>>>    */
>>> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>> -                                u32 timer_clk_rate)
>>> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>>>   {
>>> -       u32 mipi_clock; /* Hz */
>>>          u32 ui; /* ps */
>>>          u32 timer_period; /* ps */
>>>          u32 t_hs_prepare_max; /* ps */
>>> @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>>          u32 t_hs_settle; /* ps */
>>>          u8 settle_cnt;
>>>
>>> -       mipi_clock = pixel_clock * bpp / (2 * num_lanes);
>>> -       ui = div_u64(1000000000000LL, mipi_clock);
>>> +       if (link_freq <= 0)
>>> +               return 0;
>>> +
>>> +       ui = div_u64(1000000000000LL, link_freq);
>>>          ui /= 2;
>>>          t_hs_prepare_max = 85000 + 6 * ui;
>>>          t_hs_prepare_zero_min = 145000 + 10 * ui;
>>> @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>>
>>>   static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>>>                                  struct csiphy_config *cfg,
>>> -                               u32 pixel_clock, u8 bpp, u8 lane_mask)
>>> +                               s64 link_freq, u8 lane_mask)
>>>   {
>>>          struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>>>          u8 settle_cnt;
>>>          u8 val, l = 0;
>>>          int i = 0;
>>>
>>> -       settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
>>> -                                           csiphy->timer_clk_rate);
>>> +       settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>>>
>>>          writel_relaxed(0x1, csiphy->base +
>>>                         CAMSS_CSI_PHY_GLBL_T_INIT_CFG0);
>>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> index 97cb9de85031..da7c3d3f9a10 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>>> @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev)
>>>    *
>>>    * Helper function to calculate settle count value. This is
>>>    * based on the CSI2 T_hs_settle parameter which in turn
>>> - * is calculated based on the CSI2 transmitter pixel clock
>>> - * frequency.
>>> + * is calculated based on the CSI2 transmitter link frequency.
>>>    *
>>> - * Return settle count value or 0 if the CSI2 pixel clock
>>> - * frequency is not available
>>> + * Return settle count value or 0 if the CSI2 link frequency
>>> + * is not available
>>>    */
>>> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>> -                                u32 timer_clk_rate)
>>> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>>>   {
>>> -       u32 mipi_clock; /* Hz */
>>>          u32 ui; /* ps */
>>>          u32 timer_period; /* ps */
>>>          u32 t_hs_prepare_max; /* ps */
>>>          u32 t_hs_settle; /* ps */
>>>          u8 settle_cnt;
>>>
>>> -       mipi_clock = pixel_clock * bpp / (2 * num_lanes);
>>> -       ui = div_u64(1000000000000LL, mipi_clock);
>>> +       if (link_freq <= 0)
>>> +               return 0;
>>> +
>>> +       ui = div_u64(1000000000000LL, link_freq);
>>>          ui /= 2;
>>>          t_hs_prepare_max = 85000 + 6 * ui;
>>>          t_hs_settle = t_hs_prepare_max;
>>> @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>>
>>>   static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>>>                                  struct csiphy_config *cfg,
>>> -                               u32 pixel_clock, u8 bpp, u8 lane_mask)
>>> +                               s64 link_freq, u8 lane_mask)
>>>   {
>>>          struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>>>          u8 settle_cnt;
>>>          u8 val, l = 0;
>>>          int i;
>>>
>>> -       settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
>>> -                                           csiphy->timer_clk_rate);
>>> +       settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>>>
>>>          val = BIT(c->clk.pos);
>>>          for (i = 0; i < c->num_data; i++)
>>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
>>> index 509c9a59c09c..9b5fe6fc7664 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
>>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
>>> @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats,
>>>   static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
>>>   {
>>>          struct device *dev = csiphy->camss->dev;
>>> -       u32 pixel_clock;
>>> +       s64 link_freq;
>>>          int i, j;
>>>          int ret;
>>>
>>> -       ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
>>> -       if (ret)
>>> -               pixel_clock = 0;
>>> +       u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
>>> +                               csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>>> +       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>>> +       link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>>> +                                       2 * num_lanes);
>>> +       if (link_freq < 0)
>>> +               link_freq  = 0;
>>>
>>>          for (i = 0; i < csiphy->nclocks; i++) {
>>>                  struct camss_clock *clock = &csiphy->clock[i];
>>>
>>>                  if (csiphy->rate_set[i]) {
>>> -                       u8 bpp = csiphy_get_bpp(csiphy->formats,
>>> -                                       csiphy->nformats,
>>> -                                       csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>>> -                       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>>> -                       u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4);
>>> +                       u64 min_rate = link_freq / 4;
>>>                          long round_rate;
>>>
>>>                          camss_add_clock_margin(&min_rate);
>>> @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
>>>   static int csiphy_stream_on(struct csiphy_device *csiphy)
>>>   {
>>>          struct csiphy_config *cfg = &csiphy->cfg;
>>> -       u32 pixel_clock;
>>> +       s64 link_freq;
>>
>> Checkpatch throws the following error:
>>
>> WARNING: Missing a blank line after declarations
>> #211: FILE: drivers/media/platform/qcom/camss/camss-csiphy.c:112:
>> +       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>> +       link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>>

OK, I'll fix the checkpatch warnings in v2.

Thanks,
Andrey

>>
>>>          u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg);
>>>          u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
>>>                                  csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>>> +       u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>>>          u8 val;
>>> -       int ret;
>>>
>>> -       ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
>>> -       if (ret) {
>>> -               dev_err(csiphy->camss->dev,
>>> -                       "Cannot get CSI2 transmitter's pixel clock\n");
>>> -               return -EINVAL;
>>> -       }
>>> -       if (!pixel_clock) {
>>> +       link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>>> +                                       2 * num_lanes);
>>> +       if (link_freq < 0) {
>>>                  dev_err(csiphy->camss->dev,
>>> -                       "Got pixel clock == 0, cannot continue\n");
>>> +                       "Cannot get CSI2 transmitter's link frequency\n");
>>>                  return -EINVAL;
>>>          }
>>>
>>> @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
>>>          writel_relaxed(val, csiphy->base_clk_mux);
>>>          wmb();
>>>
>>> -       csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask);
>>> +       csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask);
>>>
>>>          return 0;
>>>   }
>>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
>>> index f7967ef836dc..d71b8bc6ec00 100644
>>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
>>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
>>> @@ -50,7 +50,7 @@ struct csiphy_hw_ops {
>>>          void (*reset)(struct csiphy_device *csiphy);
>>>          void (*lanes_enable)(struct csiphy_device *csiphy,
>>>                               struct csiphy_config *cfg,
>>> -                            u32 pixel_clock, u8 bpp, u8 lane_mask);
>>> +                            s64 link_freq, u8 lane_mask);
>>>          void (*lanes_disable)(struct csiphy_device *csiphy,
>>>                                struct csiphy_config *cfg);
>>>          irqreturn_t (*isr)(int irq, void *dev);
>>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>>> index 7c0f669f8aa6..2888c7ef2303 100644
>>> --- a/drivers/media/platform/qcom/camss/camss.c
>>> +++ b/drivers/media/platform/qcom/camss/camss.c
>>> @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity)
>>>          }
>>>   }
>>>
>>> +/**
>>> + * camss_get_link_freq - Get link frequency from sensor
>>> + * @entity: Media entity in the current pipeline
>>> + * @bpp: Number of bits per pixel for the current format
>>> + * @lanes: Number of lanes in the link to the sensor
>>> + *
>>> + * Return link frequency on success or a negative error code otherwise
>>> + */
>>> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
>>> +                       unsigned int lanes)
>>> +{
>>> +       struct media_entity *sensor;
>>> +       struct v4l2_subdev *subdev;
>>> +
>>> +       sensor = camss_find_sensor(entity);
>>> +       if (!sensor)
>>> +               return -ENODEV;
>>> +
>>> +       subdev = media_entity_to_v4l2_subdev(sensor);
>>> +
>>> +       return v4l2_get_link_freq(subdev->ctrl_handler, bpp, lanes);
>>> +}
>>> +
>>>   /*
>>>    * camss_get_pixel_clock - Get pixel clock rate from sensor
>>>    * @entity: Media entity in the current pipeline
>>> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
>>> index 3a0484683cd6..86cdc25189eb 100644
>>> --- a/drivers/media/platform/qcom/camss/camss.h
>>> +++ b/drivers/media/platform/qcom/camss/camss.h
>>> @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>>>                          struct device *dev);
>>>   void camss_disable_clocks(int nclocks, struct camss_clock *clock);
>>>   struct media_entity *camss_find_sensor(struct media_entity *entity);
>>> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
>>> +                       unsigned int lanes);
>>>   int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
>>>   int camss_pm_domain_on(struct camss *camss, int id);
>>>   void camss_pm_domain_off(struct camss *camss, int id);
>>> --
>>> 2.17.1
>>>
Andrey Konovalov Feb. 15, 2021, 8:58 p.m. UTC | #4 
Hi Jacopo,

Thank you for the detailed review!

On 15.02.2021 15:00, Jacopo Mondi wrote:
> Hi Andrey,
>     nice to see progress in this direction
> 
> On Mon, Feb 15, 2021 at 12:34:03AM +0300, Andrey Konovalov wrote:
>> There are places in the camss driver where camss_get_pixel_clock() is
>> called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to
>> calculate the link frequency from it. There is a case when this would
>> not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are
>> read (sampled) from the sensor's pixel array, and this rate is different
>> from the pixel transmission rate over the CSI link, the link frequency
>> value can't be calculated from the pixel rate. One needs to use
>> V4L2_CID_LINK_FREQ to get the link frequency in this case.
>>
>> Replace such calls to camss_get_pixel_clock() with calls to a wrapper
>> around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ
>> first, and if it is not implemented by the camera sensor driver, falls
>> back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from.
> 
> Is it worth warning in the core function that the subdevice should
> support LINK_FREQ and if we fallback to use PIXEL_RATE the calculation
> result might not be accurate ?

Yes, this might be useful. I'll add a separate patch for that in v2.

>> Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates()
>> are left intact as it looks like this VFE clock does depend on the
>> rate the pixel samples comes out of the camera sensor, not on the
>> frequency at which the link between the sensor and the CSI receiver
>> operates.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   .../media/platform/qcom/camss/camss-csid.c    | 22 ++++++------
>>   .../qcom/camss/camss-csiphy-2ph-1-0.c         | 22 ++++++------
>>   .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 ++++++------
>>   .../media/platform/qcom/camss/camss-csiphy.c  | 36 +++++++++----------
>>   .../media/platform/qcom/camss/camss-csiphy.h  |  2 +-
>>   drivers/media/platform/qcom/camss/camss.c     | 23 ++++++++++++
>>   drivers/media/platform/qcom/camss/camss.h     |  2 ++
>>   7 files changed, 73 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
>> index be3fe76f3dc3..b2cbf4b65949 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -462,13 +462,19 @@ static irqreturn_t csid_isr(int irq, void *dev)
>>   static int csid_set_clock_rates(struct csid_device *csid)
>>   {
>>   	struct device *dev = csid->camss->dev;
>> -	u32 pixel_clock;
>> +	s64 link_freq;
>>   	int i, j;
>>   	int ret;
>>
>> -	ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock);
>> -	if (ret)
>> -		pixel_clock = 0;
>> +	const struct csid_format *f = csid_get_fmt_entry(
>> +		csid->formats,
>> +		csid->nformats,
>> +		csid->fmt[MSM_CSIPHY_PAD_SINK].code);
> 
> Weird indent :/

This part was in this driver before - I've just moved it a few lines up.
But it doesn't look very nice, agreed.

> I would either keep the arguments on one line or align after the open
> ( if it doesn't go past 80-cols

- as far as I can see, the "csid->fmt[MSM_CSIPHY_PAD_SINK].code);" wouldn't
fit into 80-cols either way.

I'll try to think something out in v2.

>> +	u8 num_lanes = csid->phy.lane_cnt;
>> +	link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,
> 
> Empy line maybe ?

Ack. checkpatch suggests the same.

>> +					2 * num_lanes);
> 
> I see you pass in 2 * num_lanes and I assume it's for CSI-2 DDR.

Right.

> Can't this be handled in camss_get_link_freq() so that you here only
> pass in the actual number of lanes ?

OK, this should look more clear. Will do that in v2.

>> +	if (link_freq < 0)
>> +		link_freq = 0;
> 
> I don't know this driver, but I wonder if it wouldn't be better to
> fail instead of defaulting to 0, which might be dangerous if used as a
> divider.

This is in accordance with the logic implemented in the current driver:

-----8<-----
			u64 min_rate = link_freq / 4;
<snip>
			camss_add_clock_margin(&min_rate);
<if min_rate is 0, camss_add_clock_margin() leaves min_rate as 0>
<snip>
			/* if sensor pixel clock is not available */
			/* set highest possible CSID clock rate */
			if (min_rate == 0)
				j = clock->nfreqs - 1;
-----8<-----

>>
>>   	for (i = 0; i < csid->nclocks; i++) {
>>   		struct camss_clock *clock = &csid->clock[i];
>> @@ -477,13 +483,7 @@ static int csid_set_clock_rates(struct csid_device *csid)
>>   		    !strcmp(clock->name, "csi1") ||
>>   		    !strcmp(clock->name, "csi2") ||
>>   		    !strcmp(clock->name, "csi3")) {
>> -			const struct csid_format *f = csid_get_fmt_entry(
>> -				csid->formats,
>> -				csid->nformats,
>> -				csid->fmt[MSM_CSIPHY_PAD_SINK].code);
>> -			u8 num_lanes = csid->phy.lane_cnt;
>> -			u64 min_rate = pixel_clock * f->bpp /
>> -							(2 * num_lanes * 4);
>> +			u64 min_rate = link_freq / 4;
> 
> Why 4 ? :)

min_rate = (pixel_clock * f->bpp / (2 * num_lanes)) / 4;
But (pixel_clock * f->bpp / (2 * num_lanes)) equals link_freq.
Actually the "APQ8016E Technical Reference Manual", LM80-P0436-100 Rev. D [1] at page 960
says exactly that:
	"F(GCC_CAMSS_CSIx_CLK) >= F(DDR_CLK)/4"

[1] https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf

>>   			long rate;
>>
>>   			camss_add_clock_margin(&min_rate);
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
>> index 12bce391d71f..30b454c369ab 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
>> @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy)
>>    *
>>    * Helper function to calculate settle count value. This is
>>    * based on the CSI2 T_hs_settle parameter which in turn
>> - * is calculated based on the CSI2 transmitter pixel clock
>> - * frequency.
>> + * is calculated based on the CSI2 transmitter link frequency.
>>    *
>> - * Return settle count value or 0 if the CSI2 pixel clock
>> - * frequency is not available
>> + * Return settle count value or 0 if the CSI2 link frequency
>> + * is not available
>>    */
>> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>> -				 u32 timer_clk_rate)
>> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>>   {
>> -	u32 mipi_clock; /* Hz */
>>   	u32 ui; /* ps */
>>   	u32 timer_period; /* ps */
>>   	u32 t_hs_prepare_max; /* ps */
>> @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>   	u32 t_hs_settle; /* ps */
>>   	u8 settle_cnt;
>>
>> -	mipi_clock = pixel_clock * bpp / (2 * num_lanes);
>> -	ui = div_u64(1000000000000LL, mipi_clock);
>> +	if (link_freq <= 0)
>> +		return 0;
> 
> If you error out if the link frequency cannot be calculated, can this
> be skipped ?

Do you mean removing the "if (link_freq <= 0) return 0;" from csiphy_settle_cnt_calc(), and
making the caller of csiphy_settle_cnt_calc() to do this check?

>> +
>> +	ui = div_u64(1000000000000LL, link_freq);
>>   	ui /= 2;
>>   	t_hs_prepare_max = 85000 + 6 * ui;
>>   	t_hs_prepare_zero_min = 145000 + 10 * ui;
>> @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>
>>   static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>>   				struct csiphy_config *cfg,
>> -				u32 pixel_clock, u8 bpp, u8 lane_mask)
>> +				s64 link_freq, u8 lane_mask)
>>   {
>>   	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>>   	u8 settle_cnt;
>>   	u8 val, l = 0;
>>   	int i = 0;
>>
>> -	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
>> -					    csiphy->timer_clk_rate);
>> +	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>>
>>   	writel_relaxed(0x1, csiphy->base +
>>   		       CAMSS_CSI_PHY_GLBL_T_INIT_CFG0);
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>> index 97cb9de85031..da7c3d3f9a10 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
>> @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev)
>>    *
>>    * Helper function to calculate settle count value. This is
>>    * based on the CSI2 T_hs_settle parameter which in turn
>> - * is calculated based on the CSI2 transmitter pixel clock
>> - * frequency.
>> + * is calculated based on the CSI2 transmitter link frequency.
>>    *
>> - * Return settle count value or 0 if the CSI2 pixel clock
>> - * frequency is not available
>> + * Return settle count value or 0 if the CSI2 link frequency
>> + * is not available
>>    */
>> -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>> -				 u32 timer_clk_rate)
>> +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>>   {
>> -	u32 mipi_clock; /* Hz */
>>   	u32 ui; /* ps */
>>   	u32 timer_period; /* ps */
>>   	u32 t_hs_prepare_max; /* ps */
>>   	u32 t_hs_settle; /* ps */
>>   	u8 settle_cnt;
>>
>> -	mipi_clock = pixel_clock * bpp / (2 * num_lanes);
>> -	ui = div_u64(1000000000000LL, mipi_clock);
>> +	if (link_freq <= 0)
>> +		return 0;
>> +
>> +	ui = div_u64(1000000000000LL, link_freq);
>>   	ui /= 2;
>>   	t_hs_prepare_max = 85000 + 6 * ui;
>>   	t_hs_settle = t_hs_prepare_max;
>> @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
>>
>>   static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>>   				struct csiphy_config *cfg,
>> -				u32 pixel_clock, u8 bpp, u8 lane_mask)
>> +				s64 link_freq, u8 lane_mask)
>>   {
>>   	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>>   	u8 settle_cnt;
>>   	u8 val, l = 0;
>>   	int i;
>>
>> -	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
>> -					    csiphy->timer_clk_rate);
>> +	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>>
>>   	val = BIT(c->clk.pos);
>>   	for (i = 0; i < c->num_data; i++)
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> index 509c9a59c09c..9b5fe6fc7664 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats,
>>   static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
>>   {
>>   	struct device *dev = csiphy->camss->dev;
>> -	u32 pixel_clock;
>> +	s64 link_freq;
>>   	int i, j;
>>   	int ret;
>>
>> -	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
>> -	if (ret)
>> -		pixel_clock = 0;
>> +	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
>> +				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>> +	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>> +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>> +					2 * num_lanes);
>> +	if (link_freq < 0)
>> +		link_freq  = 0;
>>
>>   	for (i = 0; i < csiphy->nclocks; i++) {
>>   		struct camss_clock *clock = &csiphy->clock[i];
>>
>>   		if (csiphy->rate_set[i]) {
>> -			u8 bpp = csiphy_get_bpp(csiphy->formats,
>> -					csiphy->nformats,
>> -					csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>> -			u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>> -			u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4);
>> +			u64 min_rate = link_freq / 4;
>>   			long round_rate;
>>
>>   			camss_add_clock_margin(&min_rate);
>> @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
>>   static int csiphy_stream_on(struct csiphy_device *csiphy)
>>   {
>>   	struct csiphy_config *cfg = &csiphy->cfg;
>> -	u32 pixel_clock;
>> +	s64 link_freq;
>>   	u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg);
>>   	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
>>   				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>> +	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
>>   	u8 val;
>> -	int ret;
>>
>> -	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
>> -	if (ret) {
>> -		dev_err(csiphy->camss->dev,
>> -			"Cannot get CSI2 transmitter's pixel clock\n");
>> -		return -EINVAL;
>> -	}
>> -	if (!pixel_clock) {
>> +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>> +					2 * num_lanes);
>> +	if (link_freq < 0) {
>>   		dev_err(csiphy->camss->dev,
>> -			"Got pixel clock == 0, cannot continue\n");
>> +			"Cannot get CSI2 transmitter's link frequency\n");
>>   		return -EINVAL;
>>   	}
>>
>> @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
>>   	writel_relaxed(val, csiphy->base_clk_mux);
>>   	wmb();
>>
>> -	csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask);
>> +	csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask);
>>
>>   	return 0;
>>   }
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
>> index f7967ef836dc..d71b8bc6ec00 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
>> @@ -50,7 +50,7 @@ struct csiphy_hw_ops {
>>   	void (*reset)(struct csiphy_device *csiphy);
>>   	void (*lanes_enable)(struct csiphy_device *csiphy,
>>   			     struct csiphy_config *cfg,
>> -			     u32 pixel_clock, u8 bpp, u8 lane_mask);
>> +			     s64 link_freq, u8 lane_mask);
>>   	void (*lanes_disable)(struct csiphy_device *csiphy,
>>   			      struct csiphy_config *cfg);
>>   	irqreturn_t (*isr)(int irq, void *dev);
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 7c0f669f8aa6..2888c7ef2303 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity)
>>   	}
>>   }
>>
>> +/**
>> + * camss_get_link_freq - Get link frequency from sensor
>> + * @entity: Media entity in the current pipeline
>> + * @bpp: Number of bits per pixel for the current format
>> + * @lanes: Number of lanes in the link to the sensor
>> + *
>> + * Return link frequency on success or a negative error code otherwise
>> + */
>> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
>> +			unsigned int lanes)
>> +{
>> +	struct media_entity *sensor;
>> +	struct v4l2_subdev *subdev;
>> +
>> +	sensor = camss_find_sensor(entity);
>> +	if (!sensor)
>> +		return -ENODEV;
> 
> Can this happen ?

In general case yes, it could.
This would definitely happen if we used camss_get_link_freq() with the vfe
entities when there is no sensor accessible from the given vfe entity via
the enabled links.
But as for the vfe entities we call camss_get_pixel_clock() instead,
it looks like camss_get_link_freq() should not fail to find the sensor.
Does it hurts, and isn't this safer to check the return value of
camss_find_sensor() in camss_get_link_freq() as well (just in case)?

Thanks,
Andrey

> Thanks
>    j
> 
>> +
>> +	subdev = media_entity_to_v4l2_subdev(sensor);
>> +
>> +	return v4l2_get_link_freq(subdev->ctrl_handler, bpp, lanes);
>> +}
>> +
>>   /*
>>    * camss_get_pixel_clock - Get pixel clock rate from sensor
>>    * @entity: Media entity in the current pipeline
>> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
>> index 3a0484683cd6..86cdc25189eb 100644
>> --- a/drivers/media/platform/qcom/camss/camss.h
>> +++ b/drivers/media/platform/qcom/camss/camss.h
>> @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>>   			struct device *dev);
>>   void camss_disable_clocks(int nclocks, struct camss_clock *clock);
>>   struct media_entity *camss_find_sensor(struct media_entity *entity);
>> +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
>> +			unsigned int lanes);
>>   int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
>>   int camss_pm_domain_on(struct camss *camss, int id);
>>   void camss_pm_domain_off(struct camss *camss, int id);
>> --
>> 2.17.1
>>
Jacopo Mondi Feb. 16, 2021, 2:44 p.m. UTC | #5 
Hi Andrey,

On Mon, Feb 15, 2021 at 11:58:27PM +0300, Andrey Konovalov wrote:
> Hi Jacopo,

>

> Thank you for the detailed review!

>

> On 15.02.2021 15:00, Jacopo Mondi wrote:

> > Hi Andrey,

> >     nice to see progress in this direction

> >

> > On Mon, Feb 15, 2021 at 12:34:03AM +0300, Andrey Konovalov wrote:

> > > There are places in the camss driver where camss_get_pixel_clock() is

> > > called to get the pixel rate (using V4L2_CID_PIXEL_RATE control) and to

> > > calculate the link frequency from it. There is a case when this would

> > > not work: when V4L2_CID_PIXEL_RATE gets the rate at which the pixels are

> > > read (sampled) from the sensor's pixel array, and this rate is different

> > > from the pixel transmission rate over the CSI link, the link frequency

> > > value can't be calculated from the pixel rate. One needs to use

> > > V4L2_CID_LINK_FREQ to get the link frequency in this case.

> > >

> > > Replace such calls to camss_get_pixel_clock() with calls to a wrapper

> > > around v4l2_get_link_freq(). v4l2_get_link_freq() tries V4L2_CID_LINK_FREQ

> > > first, and if it is not implemented by the camera sensor driver, falls

> > > back to V4L2_CID_PIXEL_RATE to calculate the link frequency value from.

> >

> > Is it worth warning in the core function that the subdevice should

> > support LINK_FREQ and if we fallback to use PIXEL_RATE the calculation

> > result might not be accurate ?

>

> Yes, this might be useful. I'll add a separate patch for that in v2.

>

> > > Calls to camss_get_pixel_clock() from vfe_[check,set]_clock_rates()

> > > are left intact as it looks like this VFE clock does depend on the

> > > rate the pixel samples comes out of the camera sensor, not on the

> > > frequency at which the link between the sensor and the CSI receiver

> > > operates.

> > >

> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> > > ---

> > >   .../media/platform/qcom/camss/camss-csid.c    | 22 ++++++------

> > >   .../qcom/camss/camss-csiphy-2ph-1-0.c         | 22 ++++++------

> > >   .../qcom/camss/camss-csiphy-3ph-1-0.c         | 22 ++++++------

> > >   .../media/platform/qcom/camss/camss-csiphy.c  | 36 +++++++++----------

> > >   .../media/platform/qcom/camss/camss-csiphy.h  |  2 +-

> > >   drivers/media/platform/qcom/camss/camss.c     | 23 ++++++++++++

> > >   drivers/media/platform/qcom/camss/camss.h     |  2 ++

> > >   7 files changed, 73 insertions(+), 56 deletions(-)

> > >

> > > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c

> > > index be3fe76f3dc3..b2cbf4b65949 100644

> > > --- a/drivers/media/platform/qcom/camss/camss-csid.c

> > > +++ b/drivers/media/platform/qcom/camss/camss-csid.c

> > > @@ -462,13 +462,19 @@ static irqreturn_t csid_isr(int irq, void *dev)

> > >   static int csid_set_clock_rates(struct csid_device *csid)

> > >   {

> > >   	struct device *dev = csid->camss->dev;

> > > -	u32 pixel_clock;

> > > +	s64 link_freq;

> > >   	int i, j;

> > >   	int ret;

> > >

> > > -	ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock);

> > > -	if (ret)

> > > -		pixel_clock = 0;

> > > +	const struct csid_format *f = csid_get_fmt_entry(

> > > +		csid->formats,

> > > +		csid->nformats,

> > > +		csid->fmt[MSM_CSIPHY_PAD_SINK].code);

> >

> > Weird indent :/

>

> This part was in this driver before - I've just moved it a few lines up.

> But it doesn't look very nice, agreed.

>

> > I would either keep the arguments on one line or align after the open

> > ( if it doesn't go past 80-cols

>

> - as far as I can see, the "csid->fmt[MSM_CSIPHY_PAD_SINK].code);" wouldn't

> fit into 80-cols either way.

>

> I'll try to think something out in v2.

>

> > > +	u8 num_lanes = csid->phy.lane_cnt;

> > > +	link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,

> >

> > Empy line maybe ?

>

> Ack. checkpatch suggests the same.

>

> > > +					2 * num_lanes);

> >

> > I see you pass in 2 * num_lanes and I assume it's for CSI-2 DDR.

>

> Right.

>

> > Can't this be handled in camss_get_link_freq() so that you here only

> > pass in the actual number of lanes ?

>

> OK, this should look more clear. Will do that in v2.

>

> > > +	if (link_freq < 0)

> > > +		link_freq = 0;

> >

> > I don't know this driver, but I wonder if it wouldn't be better to

> > fail instead of defaulting to 0, which might be dangerous if used as a

> > divider.

>

> This is in accordance with the logic implemented in the current driver:

>

> -----8<-----

> 			u64 min_rate = link_freq / 4;

> <snip>

> 			camss_add_clock_margin(&min_rate);

> <if min_rate is 0, camss_add_clock_margin() leaves min_rate as 0>

> <snip>

> 			/* if sensor pixel clock is not available */

> 			/* set highest possible CSID clock rate */


Sorry, we clarified on irc and I never replied to this email.
As I've said I'm not familiar with the driver, so if it's best to
return 0 and use the maximum available CSID rate, then please do so.

> 			if (min_rate == 0)

> 				j = clock->nfreqs - 1;

> -----8<-----

>

> > >

> > >   	for (i = 0; i < csid->nclocks; i++) {

> > >   		struct camss_clock *clock = &csid->clock[i];

> > > @@ -477,13 +483,7 @@ static int csid_set_clock_rates(struct csid_device *csid)

> > >   		    !strcmp(clock->name, "csi1") ||

> > >   		    !strcmp(clock->name, "csi2") ||

> > >   		    !strcmp(clock->name, "csi3")) {

> > > -			const struct csid_format *f = csid_get_fmt_entry(

> > > -				csid->formats,

> > > -				csid->nformats,

> > > -				csid->fmt[MSM_CSIPHY_PAD_SINK].code);

> > > -			u8 num_lanes = csid->phy.lane_cnt;

> > > -			u64 min_rate = pixel_clock * f->bpp /

> > > -							(2 * num_lanes * 4);

> > > +			u64 min_rate = link_freq / 4;

> >

> > Why 4 ? :)

>

> min_rate = (pixel_clock * f->bpp / (2 * num_lanes)) / 4;

> But (pixel_clock * f->bpp / (2 * num_lanes)) equals link_freq.

> Actually the "APQ8016E Technical Reference Manual", LM80-P0436-100 Rev. D [1] at page 960

> says exactly that:

> 	"F(GCC_CAMSS_CSIx_CLK) >= F(DDR_CLK)/4"

>

> [1] https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf

>


Ah ok, it's an HW requirement then. I felt I was missing something
obvious...

> > >   			long rate;

> > >

> > >   			camss_add_clock_margin(&min_rate);

> > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c

> > > index 12bce391d71f..30b454c369ab 100644

> > > --- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c

> > > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c

> > > @@ -51,16 +51,13 @@ static void csiphy_reset(struct csiphy_device *csiphy)

> > >    *

> > >    * Helper function to calculate settle count value. This is

> > >    * based on the CSI2 T_hs_settle parameter which in turn

> > > - * is calculated based on the CSI2 transmitter pixel clock

> > > - * frequency.

> > > + * is calculated based on the CSI2 transmitter link frequency.

> > >    *

> > > - * Return settle count value or 0 if the CSI2 pixel clock

> > > - * frequency is not available

> > > + * Return settle count value or 0 if the CSI2 link frequency

> > > + * is not available

> > >    */

> > > -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,

> > > -				 u32 timer_clk_rate)

> > > +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)

> > >   {

> > > -	u32 mipi_clock; /* Hz */

> > >   	u32 ui; /* ps */

> > >   	u32 timer_period; /* ps */

> > >   	u32 t_hs_prepare_max; /* ps */

> > > @@ -68,8 +65,10 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,

> > >   	u32 t_hs_settle; /* ps */

> > >   	u8 settle_cnt;

> > >

> > > -	mipi_clock = pixel_clock * bpp / (2 * num_lanes);

> > > -	ui = div_u64(1000000000000LL, mipi_clock);

> > > +	if (link_freq <= 0)

> > > +		return 0;

> >

> > If you error out if the link frequency cannot be calculated, can this

> > be skipped ?

>

> Do you mean removing the "if (link_freq <= 0) return 0;" from csiphy_settle_cnt_calc(), and

> making the caller of csiphy_settle_cnt_calc() to do this check?

>

> > > +

> > > +	ui = div_u64(1000000000000LL, link_freq);

> > >   	ui /= 2;

> > >   	t_hs_prepare_max = 85000 + 6 * ui;

> > >   	t_hs_prepare_zero_min = 145000 + 10 * ui;

> > > @@ -83,15 +82,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,

> > >

> > >   static void csiphy_lanes_enable(struct csiphy_device *csiphy,

> > >   				struct csiphy_config *cfg,

> > > -				u32 pixel_clock, u8 bpp, u8 lane_mask)

> > > +				s64 link_freq, u8 lane_mask)

> > >   {

> > >   	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;

> > >   	u8 settle_cnt;

> > >   	u8 val, l = 0;

> > >   	int i = 0;

> > >

> > > -	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,

> > > -					    csiphy->timer_clk_rate);

> > > +	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);

> > >

> > >   	writel_relaxed(0x1, csiphy->base +

> > >   		       CAMSS_CSI_PHY_GLBL_T_INIT_CFG0);

> > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c

> > > index 97cb9de85031..da7c3d3f9a10 100644

> > > --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c

> > > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c

> > > @@ -107,24 +107,23 @@ static irqreturn_t csiphy_isr(int irq, void *dev)

> > >    *

> > >    * Helper function to calculate settle count value. This is

> > >    * based on the CSI2 T_hs_settle parameter which in turn

> > > - * is calculated based on the CSI2 transmitter pixel clock

> > > - * frequency.

> > > + * is calculated based on the CSI2 transmitter link frequency.

> > >    *

> > > - * Return settle count value or 0 if the CSI2 pixel clock

> > > - * frequency is not available

> > > + * Return settle count value or 0 if the CSI2 link frequency

> > > + * is not available

> > >    */

> > > -static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,

> > > -				 u32 timer_clk_rate)

> > > +static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)

> > >   {

> > > -	u32 mipi_clock; /* Hz */

> > >   	u32 ui; /* ps */

> > >   	u32 timer_period; /* ps */

> > >   	u32 t_hs_prepare_max; /* ps */

> > >   	u32 t_hs_settle; /* ps */

> > >   	u8 settle_cnt;

> > >

> > > -	mipi_clock = pixel_clock * bpp / (2 * num_lanes);

> > > -	ui = div_u64(1000000000000LL, mipi_clock);

> > > +	if (link_freq <= 0)

> > > +		return 0;

> > > +

> > > +	ui = div_u64(1000000000000LL, link_freq);

> > >   	ui /= 2;

> > >   	t_hs_prepare_max = 85000 + 6 * ui;

> > >   	t_hs_settle = t_hs_prepare_max;

> > > @@ -137,15 +136,14 @@ static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,

> > >

> > >   static void csiphy_lanes_enable(struct csiphy_device *csiphy,

> > >   				struct csiphy_config *cfg,

> > > -				u32 pixel_clock, u8 bpp, u8 lane_mask)

> > > +				s64 link_freq, u8 lane_mask)

> > >   {

> > >   	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;

> > >   	u8 settle_cnt;

> > >   	u8 val, l = 0;

> > >   	int i;

> > >

> > > -	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,

> > > -					    csiphy->timer_clk_rate);

> > > +	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);

> > >

> > >   	val = BIT(c->clk.pos);

> > >   	for (i = 0; i < c->num_data; i++)

> > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c

> > > index 509c9a59c09c..9b5fe6fc7664 100644

> > > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c

> > > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c

> > > @@ -102,23 +102,23 @@ static u8 csiphy_get_bpp(const struct csiphy_format *formats,

> > >   static int csiphy_set_clock_rates(struct csiphy_device *csiphy)

> > >   {

> > >   	struct device *dev = csiphy->camss->dev;

> > > -	u32 pixel_clock;

> > > +	s64 link_freq;

> > >   	int i, j;

> > >   	int ret;

> > >

> > > -	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);

> > > -	if (ret)

> > > -		pixel_clock = 0;

> > > +	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,

> > > +				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);

> > > +	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;

> > > +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,

> > > +					2 * num_lanes);

> > > +	if (link_freq < 0)

> > > +		link_freq  = 0;

> > >

> > >   	for (i = 0; i < csiphy->nclocks; i++) {

> > >   		struct camss_clock *clock = &csiphy->clock[i];

> > >

> > >   		if (csiphy->rate_set[i]) {

> > > -			u8 bpp = csiphy_get_bpp(csiphy->formats,

> > > -					csiphy->nformats,

> > > -					csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);

> > > -			u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;

> > > -			u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4);

> > > +			u64 min_rate = link_freq / 4;

> > >   			long round_rate;

> > >

> > >   			camss_add_clock_margin(&min_rate);

> > > @@ -238,22 +238,18 @@ static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)

> > >   static int csiphy_stream_on(struct csiphy_device *csiphy)

> > >   {

> > >   	struct csiphy_config *cfg = &csiphy->cfg;

> > > -	u32 pixel_clock;

> > > +	s64 link_freq;

> > >   	u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg);

> > >   	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,

> > >   				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);

> > > +	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;

> > >   	u8 val;

> > > -	int ret;

> > >

> > > -	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);

> > > -	if (ret) {

> > > -		dev_err(csiphy->camss->dev,

> > > -			"Cannot get CSI2 transmitter's pixel clock\n");

> > > -		return -EINVAL;

> > > -	}

> > > -	if (!pixel_clock) {

> > > +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,

> > > +					2 * num_lanes);

> > > +	if (link_freq < 0) {

> > >   		dev_err(csiphy->camss->dev,

> > > -			"Got pixel clock == 0, cannot continue\n");

> > > +			"Cannot get CSI2 transmitter's link frequency\n");

> > >   		return -EINVAL;

> > >   	}

> > >

> > > @@ -268,7 +264,7 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)

> > >   	writel_relaxed(val, csiphy->base_clk_mux);

> > >   	wmb();

> > >

> > > -	csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask);

> > > +	csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask);

> > >

> > >   	return 0;

> > >   }

> > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h

> > > index f7967ef836dc..d71b8bc6ec00 100644

> > > --- a/drivers/media/platform/qcom/camss/camss-csiphy.h

> > > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h

> > > @@ -50,7 +50,7 @@ struct csiphy_hw_ops {

> > >   	void (*reset)(struct csiphy_device *csiphy);

> > >   	void (*lanes_enable)(struct csiphy_device *csiphy,

> > >   			     struct csiphy_config *cfg,

> > > -			     u32 pixel_clock, u8 bpp, u8 lane_mask);

> > > +			     s64 link_freq, u8 lane_mask);

> > >   	void (*lanes_disable)(struct csiphy_device *csiphy,

> > >   			      struct csiphy_config *cfg);

> > >   	irqreturn_t (*isr)(int irq, void *dev);

> > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c

> > > index 7c0f669f8aa6..2888c7ef2303 100644

> > > --- a/drivers/media/platform/qcom/camss/camss.c

> > > +++ b/drivers/media/platform/qcom/camss/camss.c

> > > @@ -548,6 +548,29 @@ struct media_entity *camss_find_sensor(struct media_entity *entity)

> > >   	}

> > >   }

> > >

> > > +/**

> > > + * camss_get_link_freq - Get link frequency from sensor

> > > + * @entity: Media entity in the current pipeline

> > > + * @bpp: Number of bits per pixel for the current format

> > > + * @lanes: Number of lanes in the link to the sensor

> > > + *

> > > + * Return link frequency on success or a negative error code otherwise

> > > + */

> > > +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,

> > > +			unsigned int lanes)

> > > +{

> > > +	struct media_entity *sensor;

> > > +	struct v4l2_subdev *subdev;

> > > +

> > > +	sensor = camss_find_sensor(entity);

> > > +	if (!sensor)

> > > +		return -ENODEV;

> >

> > Can this happen ?

>

> In general case yes, it could.

> This would definitely happen if we used camss_get_link_freq() with the vfe

> entities when there is no sensor accessible from the given vfe entity via

> the enabled links.

> But as for the vfe entities we call camss_get_pixel_clock() instead,

> it looks like camss_get_link_freq() should not fail to find the sensor.

> Does it hurts, and isn't this safer to check the return value of

> camss_find_sensor() in camss_get_link_freq() as well (just in case)?


I don't mind. If the fact that there might be no sensor on the other
end of the link is accepted by the driver, please keep this check
here.

Thanks
  j

>

> Thanks,

> Andrey

>

> > Thanks

> >    j

> >

> > > +

> > > +	subdev = media_entity_to_v4l2_subdev(sensor);

> > > +

> > > +	return v4l2_get_link_freq(subdev->ctrl_handler, bpp, lanes);

> > > +}

> > > +

> > >   /*

> > >    * camss_get_pixel_clock - Get pixel clock rate from sensor

> > >    * @entity: Media entity in the current pipeline

> > > diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h

> > > index 3a0484683cd6..86cdc25189eb 100644

> > > --- a/drivers/media/platform/qcom/camss/camss.h

> > > +++ b/drivers/media/platform/qcom/camss/camss.h

> > > @@ -108,6 +108,8 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock,

> > >   			struct device *dev);

> > >   void camss_disable_clocks(int nclocks, struct camss_clock *clock);

> > >   struct media_entity *camss_find_sensor(struct media_entity *entity);

> > > +s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,

> > > +			unsigned int lanes);

> > >   int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);

> > >   int camss_pm_domain_on(struct camss *camss, int id);

> > >   void camss_pm_domain_off(struct camss *camss, int id);

> > > --

> > > 2.17.1

> > >
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index be3fe76f3dc3..b2cbf4b65949 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -462,13 +462,19 @@  static irqreturn_t csid_isr(int irq, void *dev)
 static int csid_set_clock_rates(struct csid_device *csid)
 {
 	struct device *dev = csid->camss->dev;
-	u32 pixel_clock;
+	s64 link_freq;
 	int i, j;
 	int ret;
 
-	ret = camss_get_pixel_clock(&csid->subdev.entity, &pixel_clock);
-	if (ret)
-		pixel_clock = 0;
+	const struct csid_format *f = csid_get_fmt_entry(
+		csid->formats,
+		csid->nformats,
+		csid->fmt[MSM_CSIPHY_PAD_SINK].code);
+	u8 num_lanes = csid->phy.lane_cnt;
+	link_freq = camss_get_link_freq(&csid->subdev.entity, f->bpp,
+					2 * num_lanes);
+	if (link_freq < 0)
+		link_freq = 0;
 
 	for (i = 0; i < csid->nclocks; i++) {
 		struct camss_clock *clock = &csid->clock[i];
@@ -477,13 +483,7 @@  static int csid_set_clock_rates(struct csid_device *csid)
 		    !strcmp(clock->name, "csi1") ||
 		    !strcmp(clock->name, "csi2") ||
 		    !strcmp(clock->name, "csi3")) {
-			const struct csid_format *f = csid_get_fmt_entry(
-				csid->formats,
-				csid->nformats,
-				csid->fmt[MSM_CSIPHY_PAD_SINK].code);
-			u8 num_lanes = csid->phy.lane_cnt;
-			u64 min_rate = pixel_clock * f->bpp /
-							(2 * num_lanes * 4);
+			u64 min_rate = link_freq / 4;
 			long rate;
 
 			camss_add_clock_margin(&min_rate);
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
index 12bce391d71f..30b454c369ab 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-2ph-1-0.c
@@ -51,16 +51,13 @@  static void csiphy_reset(struct csiphy_device *csiphy)
  *
  * Helper function to calculate settle count value. This is
  * based on the CSI2 T_hs_settle parameter which in turn
- * is calculated based on the CSI2 transmitter pixel clock
- * frequency.
+ * is calculated based on the CSI2 transmitter link frequency.
  *
- * Return settle count value or 0 if the CSI2 pixel clock
- * frequency is not available
+ * Return settle count value or 0 if the CSI2 link frequency
+ * is not available
  */
-static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
-				 u32 timer_clk_rate)
+static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
 {
-	u32 mipi_clock; /* Hz */
 	u32 ui; /* ps */
 	u32 timer_period; /* ps */
 	u32 t_hs_prepare_max; /* ps */
@@ -68,8 +65,10 @@  static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
 	u32 t_hs_settle; /* ps */
 	u8 settle_cnt;
 
-	mipi_clock = pixel_clock * bpp / (2 * num_lanes);
-	ui = div_u64(1000000000000LL, mipi_clock);
+	if (link_freq <= 0)
+		return 0;
+
+	ui = div_u64(1000000000000LL, link_freq);
 	ui /= 2;
 	t_hs_prepare_max = 85000 + 6 * ui;
 	t_hs_prepare_zero_min = 145000 + 10 * ui;
@@ -83,15 +82,14 @@  static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
 
 static void csiphy_lanes_enable(struct csiphy_device *csiphy,
 				struct csiphy_config *cfg,
-				u32 pixel_clock, u8 bpp, u8 lane_mask)
+				s64 link_freq, u8 lane_mask)
 {
 	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
 	u8 settle_cnt;
 	u8 val, l = 0;
 	int i = 0;
 
-	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
-					    csiphy->timer_clk_rate);
+	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
 
 	writel_relaxed(0x1, csiphy->base +
 		       CAMSS_CSI_PHY_GLBL_T_INIT_CFG0);
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
index 97cb9de85031..da7c3d3f9a10 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
@@ -107,24 +107,23 @@  static irqreturn_t csiphy_isr(int irq, void *dev)
  *
  * Helper function to calculate settle count value. This is
  * based on the CSI2 T_hs_settle parameter which in turn
- * is calculated based on the CSI2 transmitter pixel clock
- * frequency.
+ * is calculated based on the CSI2 transmitter link frequency.
  *
- * Return settle count value or 0 if the CSI2 pixel clock
- * frequency is not available
+ * Return settle count value or 0 if the CSI2 link frequency
+ * is not available
  */
-static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
-				 u32 timer_clk_rate)
+static u8 csiphy_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
 {
-	u32 mipi_clock; /* Hz */
 	u32 ui; /* ps */
 	u32 timer_period; /* ps */
 	u32 t_hs_prepare_max; /* ps */
 	u32 t_hs_settle; /* ps */
 	u8 settle_cnt;
 
-	mipi_clock = pixel_clock * bpp / (2 * num_lanes);
-	ui = div_u64(1000000000000LL, mipi_clock);
+	if (link_freq <= 0)
+		return 0;
+
+	ui = div_u64(1000000000000LL, link_freq);
 	ui /= 2;
 	t_hs_prepare_max = 85000 + 6 * ui;
 	t_hs_settle = t_hs_prepare_max;
@@ -137,15 +136,14 @@  static u8 csiphy_settle_cnt_calc(u32 pixel_clock, u8 bpp, u8 num_lanes,
 
 static void csiphy_lanes_enable(struct csiphy_device *csiphy,
 				struct csiphy_config *cfg,
-				u32 pixel_clock, u8 bpp, u8 lane_mask)
+				s64 link_freq, u8 lane_mask)
 {
 	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
 	u8 settle_cnt;
 	u8 val, l = 0;
 	int i;
 
-	settle_cnt = csiphy_settle_cnt_calc(pixel_clock, bpp, c->num_data,
-					    csiphy->timer_clk_rate);
+	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
 
 	val = BIT(c->clk.pos);
 	for (i = 0; i < c->num_data; i++)
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
index 509c9a59c09c..9b5fe6fc7664 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
@@ -102,23 +102,23 @@  static u8 csiphy_get_bpp(const struct csiphy_format *formats,
 static int csiphy_set_clock_rates(struct csiphy_device *csiphy)
 {
 	struct device *dev = csiphy->camss->dev;
-	u32 pixel_clock;
+	s64 link_freq;
 	int i, j;
 	int ret;
 
-	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
-	if (ret)
-		pixel_clock = 0;
+	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
+				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
+	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
+	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
+					2 * num_lanes);
+	if (link_freq < 0)
+		link_freq  = 0;
 
 	for (i = 0; i < csiphy->nclocks; i++) {
 		struct camss_clock *clock = &csiphy->clock[i];
 
 		if (csiphy->rate_set[i]) {
-			u8 bpp = csiphy_get_bpp(csiphy->formats,
-					csiphy->nformats,
-					csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
-			u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
-			u64 min_rate = pixel_clock * bpp / (2 * num_lanes * 4);
+			u64 min_rate = link_freq / 4;
 			long round_rate;
 
 			camss_add_clock_margin(&min_rate);
@@ -238,22 +238,18 @@  static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
 static int csiphy_stream_on(struct csiphy_device *csiphy)
 {
 	struct csiphy_config *cfg = &csiphy->cfg;
-	u32 pixel_clock;
+	s64 link_freq;
 	u8 lane_mask = csiphy_get_lane_mask(&cfg->csi2->lane_cfg);
 	u8 bpp = csiphy_get_bpp(csiphy->formats, csiphy->nformats,
 				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
+	u8 num_lanes = csiphy->cfg.csi2->lane_cfg.num_data;
 	u8 val;
-	int ret;
 
-	ret = camss_get_pixel_clock(&csiphy->subdev.entity, &pixel_clock);
-	if (ret) {
-		dev_err(csiphy->camss->dev,
-			"Cannot get CSI2 transmitter's pixel clock\n");
-		return -EINVAL;
-	}
-	if (!pixel_clock) {
+	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
+					2 * num_lanes);
+	if (link_freq < 0) {
 		dev_err(csiphy->camss->dev,
-			"Got pixel clock == 0, cannot continue\n");
+			"Cannot get CSI2 transmitter's link frequency\n");
 		return -EINVAL;
 	}
 
@@ -268,7 +264,7 @@  static int csiphy_stream_on(struct csiphy_device *csiphy)
 	writel_relaxed(val, csiphy->base_clk_mux);
 	wmb();
 
-	csiphy->ops->lanes_enable(csiphy, cfg, pixel_clock, bpp, lane_mask);
+	csiphy->ops->lanes_enable(csiphy, cfg, link_freq, lane_mask);
 
 	return 0;
 }
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
index f7967ef836dc..d71b8bc6ec00 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.h
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
@@ -50,7 +50,7 @@  struct csiphy_hw_ops {
 	void (*reset)(struct csiphy_device *csiphy);
 	void (*lanes_enable)(struct csiphy_device *csiphy,
 			     struct csiphy_config *cfg,
-			     u32 pixel_clock, u8 bpp, u8 lane_mask);
+			     s64 link_freq, u8 lane_mask);
 	void (*lanes_disable)(struct csiphy_device *csiphy,
 			      struct csiphy_config *cfg);
 	irqreturn_t (*isr)(int irq, void *dev);
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 7c0f669f8aa6..2888c7ef2303 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -548,6 +548,29 @@  struct media_entity *camss_find_sensor(struct media_entity *entity)
 	}
 }
 
+/**
+ * camss_get_link_freq - Get link frequency from sensor
+ * @entity: Media entity in the current pipeline
+ * @bpp: Number of bits per pixel for the current format
+ * @lanes: Number of lanes in the link to the sensor
+ *
+ * Return link frequency on success or a negative error code otherwise
+ */
+s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
+			unsigned int lanes)
+{
+	struct media_entity *sensor;
+	struct v4l2_subdev *subdev;
+
+	sensor = camss_find_sensor(entity);
+	if (!sensor)
+		return -ENODEV;
+
+	subdev = media_entity_to_v4l2_subdev(sensor);
+
+	return v4l2_get_link_freq(subdev->ctrl_handler, bpp, lanes);
+}
+
 /*
  * camss_get_pixel_clock - Get pixel clock rate from sensor
  * @entity: Media entity in the current pipeline
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index 3a0484683cd6..86cdc25189eb 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -108,6 +108,8 @@  int camss_enable_clocks(int nclocks, struct camss_clock *clock,
 			struct device *dev);
 void camss_disable_clocks(int nclocks, struct camss_clock *clock);
 struct media_entity *camss_find_sensor(struct media_entity *entity);
+s64 camss_get_link_freq(struct media_entity *entity, unsigned int bpp,
+			unsigned int lanes);
 int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
 int camss_pm_domain_on(struct camss *camss, int id);
 void camss_pm_domain_off(struct camss *camss, int id);