mbox series

[00/17] CAMSS: SM8250 support (and some fixes)

Message ID 20210511180728.23781-1-jonathan@marek.ca
Headers show
Series CAMSS: SM8250 support (and some fixes) | expand

Message

Jonathan Marek May 11, 2021, 6:07 p.m. UTC
This adds initial support for SM8250 and its 4 VFEs and 6 CSIPHYs.
The only big change is the added camss-vfe-480.c to support the
Titan 480 VFE.

Jonathan Marek (17):
  media: camss: csiphy-3ph: don't print HW version as an error
  media: camss: csiphy-3ph: disable interrupts
  media: camss: csiphy-3ph: add support for SM8250 CSI DPHY
  media: camss: csid-170: fix non-10bit formats
  media: camss: csid-170: don't enable unused irqs
  media: camss: csid-170: remove stray comment
  media: camss: csid-170: support more than one lite vfe
  media: camss: csid-170: set the right HALF_CMD when disabled
  media: camss: csid: allow csid to work without a regulator
  media: camss: remove vdda-csiN from sdm845 resources
  media: camss: fix VFE irq name
  media: camss: vfe-170: clean up some dead code
  media: camss: vfe-170: fix "VFE halt timeout" error
  media: camss: Add initial support for VFE hardware version Titan 480
  media: camss: add support for V4L2_PIX_FMT_GREY for sdm845 HW
  media: camss: add support for SM8250 camss
  media: dt-bindings: media: camss: Add qcom,sm8250-camss binding

 .../bindings/media/qcom,sm8250-camss.yaml     | 398 +++++++++++++
 drivers/media/platform/qcom/camss/Makefile    |   1 +
 .../platform/qcom/camss/camss-csid-170.c      |  31 +-
 .../media/platform/qcom/camss/camss-csid.c    |  39 +-
 .../qcom/camss/camss-csiphy-3ph-1-0.c         | 181 ++++--
 .../media/platform/qcom/camss/camss-csiphy.c  |   9 +-
 .../media/platform/qcom/camss/camss-vfe-170.c |  65 +-
 .../media/platform/qcom/camss/camss-vfe-480.c | 554 ++++++++++++++++++
 drivers/media/platform/qcom/camss/camss-vfe.c |  13 +-
 drivers/media/platform/qcom/camss/camss-vfe.h |   1 +
 .../media/platform/qcom/camss/camss-video.c   |   5 +-
 drivers/media/platform/qcom/camss/camss.c     | 202 ++++++-
 drivers/media/platform/qcom/camss/camss.h     |   1 +
 13 files changed, 1352 insertions(+), 148 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-480.c

Comments

Rob Herring (Arm) May 12, 2021, 6:35 p.m. UTC | #1
On Tue, 11 May 2021 14:07:24 -0400, Jonathan Marek wrote:
> Add bindings for qcom,sm8250-camss in order to support the camera
> subsystem for SM8250.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  .../bindings/media/qcom,sm8250-camss.yaml     | 398 ++++++++++++++++++
>  1 file changed, 398 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/media/qcom,sm8250-camss.example.dts:20:18: fatal error: dt-bindings/clock/qcom,camcc-sm8250.h: No such file or directory
   20 |         #include <dt-bindings/clock/qcom,camcc-sm8250.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/media/qcom,sm8250-camss.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1416: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1477274

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Robert Foss May 19, 2021, 10:16 a.m. UTC | #2
Hey Jonathan,

Thanks for submitting this series, and including some cleanups and fixes.

On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>

> Avoid unnecessary noise in normal usage (it prints every time CSIPHY is

> powered on).

>

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

> ---

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

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> 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 e318c822ab04..5948abdcd220 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

> @@ -163,7 +163,7 @@ static void csiphy_hw_version_read(struct csiphy_device *csiphy,

>         hw_version |= readl_relaxed(csiphy->base +

>                                    CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(15)) << 24;

>

> -       dev_err(dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);

> +       dev_dbg(dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);

>  }

>

>  /*


Reviewed-by: Robert Foss <robert.foss@linaro.org>
Robert Foss May 19, 2021, 4:26 p.m. UTC | #3
On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>

> Add support for CSIPHY (2PH/DPHY mode) found on SM8250 hardware.

>

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

> ---

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

>  1 file changed, 137 insertions(+), 7 deletions(-)

>

> 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 783b65295d20..61947576ddfb 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

> @@ -62,6 +62,7 @@ struct csiphy_reg_t {

>         u32 csiphy_param_type;

>  };

>

> +/* GEN2 1.0 2PH */

>  static const struct

>  csiphy_reg_t lane_regs_sdm845[5][14] = {

>         {

> @@ -146,6 +147,121 @@ csiphy_reg_t lane_regs_sdm845[5][14] = {

>         },

>  };

>

> +/* GEN2 1.2.1 2PH */

> +static const struct

> +csiphy_reg_t lane_regs_sm8250[5][20] = {

> +       {

> +               {0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0900, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0908, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0904, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0904, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0034, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0010, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x001C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

> +               {0x0000, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x000c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0028, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0024, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +       },

> +       {

> +               {0x0730, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C80, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C88, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C84, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C84, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0704, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x072C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0734, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0710, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x071C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x073C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0708, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

> +               {0x0700, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x070c, 0xA5, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0738, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0714, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0728, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0724, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +       },

> +       {

> +               {0x0230, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0A00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0A08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0A04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0A04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0204, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x022C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0234, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0210, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x021C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x023C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0208, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

> +               {0x0200, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x020c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0238, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0214, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0228, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0224, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +       },

> +       {

> +               {0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0B00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0B08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0B04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0B04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0434, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0410, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x041C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

> +               {0x0400, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x040c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0428, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0424, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +       },

> +       {

> +               {0x0630, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0C04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0604, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x062C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0634, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0610, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x061C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x063C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0608, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

> +               {0x0600, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x060c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0638, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0614, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0628, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0624, 0x00, 0x00, CSIPHY_DNP_PARAMS},

> +               {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

> +               {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

> +       },

> +};

> +

>  static void csiphy_hw_version_read(struct csiphy_device *csiphy,

>                                    struct device *dev)

>  {

> @@ -298,13 +414,23 @@ static void csiphy_gen1_config_lanes(struct csiphy_device *csiphy,

>  static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,

>                                      u8 settle_cnt)

>  {

> -       int i, l;

> -       u32 val;

> +       const struct csiphy_reg_t *r;

> +       int i, l, array_size;

> +       u32 val, lane_enable;

> +

> +       switch (csiphy->camss->version) {

> +       case CAMSS_845:

> +               r = &lane_regs_sdm845[0][0];

> +               array_size = ARRAY_SIZE(lane_regs_sdm845[0]);

> +               break;

> +       case CAMSS_8250:


CAMSS_8250 is not defined until later in the series, in "media: camss:
add support for SM8250 camss".

> +               r = &lane_regs_sm8250[0][0];

> +               array_size = ARRAY_SIZE(lane_regs_sm8250[0]);

> +               break;

> +       }

>

>         for (l = 0; l < 5; l++) {

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

> -                       const struct csiphy_reg_t *r = &lane_regs_sdm845[l][i];

> -

> +               for (i = 0; i < array_size; i++, r++) {

>                         switch (r->csiphy_param_type) {

>                         case CSIPHY_SETTLE_CNT_LOWER_BYTE:

>                                 val = settle_cnt & 0xff;

> @@ -331,7 +457,10 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,

>

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

>

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

> +       if (csiphy->camss->version == CAMSS_8250)

> +               val = BIT(7);

> +       else

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


sdm845 and sm8250 behave the same way, and I think this chunk should
reflect that. With that being said the docs for camss-sdm845 mention
that the only valid lane for the clock is #7. I don't know if it is
preferred to enforce the restriction in the driver, yaml or both.

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

>                 val |= BIT(c->data[i].pos * 2);

>

> @@ -349,7 +478,8 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,

>         if (csiphy->camss->version == CAMSS_8x16 ||

>             csiphy->camss->version == CAMSS_8x96)

>                 csiphy_gen1_config_lanes(csiphy, cfg, settle_cnt);

> -       else if (csiphy->camss->version == CAMSS_845)

> +       else if (csiphy->camss->version == CAMSS_845 ||

> +                csiphy->camss->version == CAMSS_8250)

>                 csiphy_gen2_config_lanes(csiphy, settle_cnt);

>

>         /* IRQ_MASK registers - disable all interrupts */


With the above issues fixed;

Reviewed-by: Robert Foss <robert.foss@linaro.org>
Robert Foss May 20, 2021, 10:31 a.m. UTC | #4
Hey Jonathan,

Thanks for catching this.

On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>
> Use the decode_format/data_type from the "format" struct instead of a
> hardcoded 10-bit format.
>
> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/media/platform/qcom/camss/camss-csid-170.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-170.c b/drivers/media/platform/qcom/camss/camss-csid-170.c
> index ac22ff29d2a9..a81cc94c075f 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-170.c
> @@ -366,7 +366,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>                         val |= input_format->width & 0x1fff << TPG_DT_n_CFG_0_FRAME_WIDTH;
>                         writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_0(0));
>
> -                       val = DATA_TYPE_RAW_10BIT << TPG_DT_n_CFG_1_DATA_TYPE;
> +                       val = format->data_type << TPG_DT_n_CFG_1_DATA_TYPE;
>                         writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_1(0));
>
>                         val = tg->mode << TPG_DT_n_CFG_2_PAYLOAD_MODE;
> @@ -382,8 +382,8 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>                 val = 1 << RDI_CFG0_BYTE_CNTR_EN;
>                 val |= 1 << RDI_CFG0_FORMAT_MEASURE_EN;
>                 val |= 1 << RDI_CFG0_TIMESTAMP_EN;
> -               val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
> -               val |= DATA_TYPE_RAW_10BIT << RDI_CFG0_DATA_TYPE;
> +               val |= format->decode_format << RDI_CFG0_DECODE_FORMAT;
> +               val |= format->data_type << RDI_CFG0_DATA_TYPE;
>                 val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;
>                 val |= dt_id << RDI_CFG0_DT_ID;
>                 writel_relaxed(val, csid->base + CSID_RDI_CFG0(0));

Reviewed-by: Robert Foss <robert.foss@linaro.org>
Robert Foss May 20, 2021, 11:10 a.m. UTC | #5
Hey Jonathan,

On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>
> Use the "HALT_CMD_RESUME_AT_FRAME_BOUNDARY" define instead of a "1" which
> is otherwise confusing, and add a "HALT_CMD_HALT_AT_FRAME_BOUNDARY" which
> is set when disabling.
>
> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/media/platform/qcom/camss/camss-csid-170.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-170.c b/drivers/media/platform/qcom/camss/camss-csid-170.c
> index af134ded241d..9f6334fd68fc 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-170.c
> @@ -105,7 +105,8 @@
>  #define CSID_RDI_CTRL(rdi)                     ((IS_LITE ? 0x208 : 0x308)\
>                                                 + 0x100 * (rdi))
>  #define                RDI_CTRL_HALT_CMD               0
> -#define                        ALT_CMD_RESUME_AT_FRAME_BOUNDARY        1
> +#define                        HALT_CMD_HALT_AT_FRAME_BOUNDARY         0
> +#define                        HALT_CMD_RESUME_AT_FRAME_BOUNDARY       1
>  #define                RDI_CTRL_HALT_MODE              2
>
>  #define CSID_RDI_FRM_DROP_PATTERN(rdi)                 ((IS_LITE ? 0x20C : 0x30C)\
> @@ -443,7 +444,10 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>         val |= 1 << CSI2_RX_CFG1_MISR_EN;
>         writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
>
> -       val = 1 << RDI_CTRL_HALT_CMD;
> +       if (enable)
> +               val = HALT_CMD_RESUME_AT_FRAME_BOUNDARY << RDI_CTRL_HALT_CMD;
> +       else
> +               val = HALT_CMD_HALT_AT_FRAME_BOUNDARY << RDI_CTRL_HALT_CMD;
>         writel_relaxed(val, csid->base + CSID_RDI_CTRL(0));

Reviewed-by: Robert Foss <robert.foss@linaro.org>
Andrey Konovalov May 25, 2021, 5:19 p.m. UTC | #6
Hi Jonathan,

Thank you for the patchset!

On 11.05.2021 21:07, Jonathan Marek wrote:
> Add support for CSIPHY (2PH/DPHY mode) found on SM8250 hardware.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>   .../qcom/camss/camss-csiphy-3ph-1-0.c         | 144 +++++++++++++++++-
>   1 file changed, 137 insertions(+), 7 deletions(-)
> 
> 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 783b65295d20..61947576ddfb 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
> @@ -62,6 +62,7 @@ struct csiphy_reg_t {
>   	u32 csiphy_param_type;
>   };
>   
> +/* GEN2 1.0 2PH */
>   static const struct
>   csiphy_reg_t lane_regs_sdm845[5][14] = {
>   	{
> @@ -146,6 +147,121 @@ csiphy_reg_t lane_regs_sdm845[5][14] = {
>   	},
>   };
>   
> +/* GEN2 1.2.1 2PH */
> +static const struct
> +csiphy_reg_t lane_regs_sm8250[5][20] = {
> +	{
> +		{0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0900, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0908, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0904, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0904, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0034, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0010, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x001C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0000, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x000c, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0028, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0024, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0730, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C80, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C88, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C84, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C84, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0704, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x072C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0734, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0710, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x071C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x073C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0708, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0700, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x070c, 0xA5, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0738, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0714, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0728, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0724, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0230, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0A00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0A08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0A04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0A04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0204, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x022C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0234, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0210, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x021C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x023C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0208, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0200, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x020c, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0238, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0214, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0228, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0224, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0B00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0B08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0B04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0B04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0434, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0410, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x041C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0400, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x040c, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0428, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0424, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +	{
> +		{0x0630, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0C04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0604, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x062C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0634, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0610, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x061C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x063C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0608, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +		{0x0600, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x060c, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0638, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0614, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0628, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0624, 0x00, 0x00, CSIPHY_DNP_PARAMS},
> +		{0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},
> +		{0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	},
> +};
> +
>   static void csiphy_hw_version_read(struct csiphy_device *csiphy,
>   				   struct device *dev)
>   {
> @@ -298,13 +414,23 @@ static void csiphy_gen1_config_lanes(struct csiphy_device *csiphy,
>   static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>   				     u8 settle_cnt)
>   {
> -	int i, l;
> -	u32 val;
> +	const struct csiphy_reg_t *r;
> +	int i, l, array_size;
> +	u32 val, lane_enable;
> +
> +	switch (csiphy->camss->version) {
> +	case CAMSS_845:
> +		r = &lane_regs_sdm845[0][0];
> +		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
> +		break;
> +	case CAMSS_8250:

CAMSS_8250 is only introduced in "[PATCH 16_17] media: camss: add support for SM8250 camss",
and this breaks bisecting.

Thanks,
Andrey

> +		r = &lane_regs_sm8250[0][0];
> +		array_size = ARRAY_SIZE(lane_regs_sm8250[0]);
> +		break;
> +	}
>   
>   	for (l = 0; l < 5; l++) {
> -		for (i = 0; i < 14; i++) {
> -			const struct csiphy_reg_t *r = &lane_regs_sdm845[l][i];
> -
> +		for (i = 0; i < array_size; i++, r++) {
>   			switch (r->csiphy_param_type) {
>   			case CSIPHY_SETTLE_CNT_LOWER_BYTE:
>   				val = settle_cnt & 0xff;
> @@ -331,7 +457,10 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>   
>   	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>   
> -	val = BIT(c->clk.pos);
> +	if (csiphy->camss->version == CAMSS_8250)
> +		val = BIT(7);
> +	else
> +		val = BIT(c->clk.pos);
>   	for (i = 0; i < c->num_data; i++)
>   		val |= BIT(c->data[i].pos * 2);
>   
> @@ -349,7 +478,8 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>   	if (csiphy->camss->version == CAMSS_8x16 ||
>   	    csiphy->camss->version == CAMSS_8x96)
>   		csiphy_gen1_config_lanes(csiphy, cfg, settle_cnt);
> -	else if (csiphy->camss->version == CAMSS_845)
> +	else if (csiphy->camss->version == CAMSS_845 ||
> +		 csiphy->camss->version == CAMSS_8250)
>   		csiphy_gen2_config_lanes(csiphy, settle_cnt);
>   
>   	/* IRQ_MASK registers - disable all interrupts */
>
Andrey Konovalov May 25, 2021, 5:35 p.m. UTC | #7
Hi Jonathan,

Couple more minor issues.

On 25.05.2021 20:19, Andrey Konovalov wrote:
> Hi Jonathan,

> 

> Thank you for the patchset!

> 

> On 11.05.2021 21:07, Jonathan Marek wrote:

>> Add support for CSIPHY (2PH/DPHY mode) found on SM8250 hardware.

>>

>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

>> ---

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

>>   1 file changed, 137 insertions(+), 7 deletions(-)

>>

>> 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 783b65295d20..61947576ddfb 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

>> @@ -62,6 +62,7 @@ struct csiphy_reg_t {

>>       u32 csiphy_param_type;

>>   };

>> +/* GEN2 1.0 2PH */

>>   static const struct

>>   csiphy_reg_t lane_regs_sdm845[5][14] = {

>>       {

>> @@ -146,6 +147,121 @@ csiphy_reg_t lane_regs_sdm845[5][14] = {

>>       },

>>   };

>> +/* GEN2 1.2.1 2PH */

>> +static const struct

>> +csiphy_reg_t lane_regs_sm8250[5][20] = {

>> +    {

>> +        {0x0030, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0900, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0908, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0904, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0904, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0004, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x002C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0034, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0010, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x001C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x003C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0008, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

>> +        {0x0000, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x000c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0038, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0014, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0028, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0024, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +    },

>> +    {

>> +        {0x0730, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C80, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C88, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C84, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C84, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0704, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x072C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0734, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0710, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x071C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x073C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0708, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

>> +        {0x0700, 0x80, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x070c, 0xA5, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0738, 0x1F, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0714, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0728, 0x04, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0724, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +    },

>> +    {

>> +        {0x0230, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0A00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0A08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0A04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0A04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0204, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x022C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0234, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0210, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x021C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x023C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0208, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

>> +        {0x0200, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x020c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0238, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0214, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0228, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0224, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +    },

>> +    {

>> +        {0x0430, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0B00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0B08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0B04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0B04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0404, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x042C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0434, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0410, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x041C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x043C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0408, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

>> +        {0x0400, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x040c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0438, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0414, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0428, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0424, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +    },

>> +    {

>> +        {0x0630, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C00, 0x05, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C08, 0x10, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C04, 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0C04, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0604, 0x0C, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x062C, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0634, 0x07, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0610, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x061C, 0x08, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x063C, 0xB8, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0608, 0x10, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},

>> +        {0x0600, 0x8D, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x060c, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0638, 0xFE, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0614, 0x60, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0628, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0624, 0x00, 0x00, CSIPHY_DNP_PARAMS},

>> +        {0x0800, 0x02, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +        {0x0884, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

>> +    },

>> +};

>> +

>>   static void csiphy_hw_version_read(struct csiphy_device *csiphy,

>>                      struct device *dev)

>>   {

>> @@ -298,13 +414,23 @@ static void csiphy_gen1_config_lanes(struct csiphy_device *csiphy,

>>   static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,

>>                        u8 settle_cnt)

>>   {

>> -    int i, l;

>> -    u32 val;

>> +    const struct csiphy_reg_t *r;

>> +    int i, l, array_size;

>> +    u32 val, lane_enable;


lane_enable is not used in this patch ("warning: unused variable ‘lane_enable’").

>> +

>> +    switch (csiphy->camss->version) {


This switch statement doesn't have the "default:" clause which leads to several
warnings of "enumeration value ‘CAMSS_8x16’ not handled in switch" kind.

Thanks,
Andrey

>> +    case CAMSS_845:

>> +        r = &lane_regs_sdm845[0][0];

>> +        array_size = ARRAY_SIZE(lane_regs_sdm845[0]);

>> +        break;

>> +    case CAMSS_8250:

> 

> CAMSS_8250 is only introduced in "[PATCH 16_17] media: camss: add support for SM8250 camss",

> and this breaks bisecting.

> 

> Thanks,

> Andrey

> 

>> +        r = &lane_regs_sm8250[0][0];

>> +        array_size = ARRAY_SIZE(lane_regs_sm8250[0]);

>> +        break;

>> +    }

>>       for (l = 0; l < 5; l++) {

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

>> -            const struct csiphy_reg_t *r = &lane_regs_sdm845[l][i];

>> -

>> +        for (i = 0; i < array_size; i++, r++) {

>>               switch (r->csiphy_param_type) {

>>               case CSIPHY_SETTLE_CNT_LOWER_BYTE:

>>                   val = settle_cnt & 0xff;

>> @@ -331,7 +457,10 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,

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

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

>> +    if (csiphy->camss->version == CAMSS_8250)

>> +        val = BIT(7);

>> +    else

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

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

>>           val |= BIT(c->data[i].pos * 2);

>> @@ -349,7 +478,8 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,

>>       if (csiphy->camss->version == CAMSS_8x16 ||

>>           csiphy->camss->version == CAMSS_8x96)

>>           csiphy_gen1_config_lanes(csiphy, cfg, settle_cnt);

>> -    else if (csiphy->camss->version == CAMSS_845)

>> +    else if (csiphy->camss->version == CAMSS_845 ||

>> +         csiphy->camss->version == CAMSS_8250)

>>           csiphy_gen2_config_lanes(csiphy, settle_cnt);

>>       /* IRQ_MASK registers - disable all interrupts */

>>
Andrey Konovalov May 25, 2021, 6:15 p.m. UTC | #8
Hi Jonathan,

Thank you for your patch!

On 11.05.2021 21:07, Jonathan Marek wrote:
> Use the decode_format/data_type from the "format" struct instead of a

> hardcoded 10-bit format.

> 

> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

> ---

>   drivers/media/platform/qcom/camss/camss-csid-170.c | 6 +++---

>   1 file changed, 3 insertions(+), 3 deletions(-)

> 

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

> index ac22ff29d2a9..a81cc94c075f 100644

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

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

> @@ -366,7 +366,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)

>   			val |= input_format->width & 0x1fff << TPG_DT_n_CFG_0_FRAME_WIDTH;

>   			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_0(0));

>   

> -			val = DATA_TYPE_RAW_10BIT << TPG_DT_n_CFG_1_DATA_TYPE;

> +			val = format->data_type << TPG_DT_n_CFG_1_DATA_TYPE;

>   			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_1(0));

>   

>   			val = tg->mode << TPG_DT_n_CFG_2_PAYLOAD_MODE;

> @@ -382,8 +382,8 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)

>   		val = 1 << RDI_CFG0_BYTE_CNTR_EN;

>   		val |= 1 << RDI_CFG0_FORMAT_MEASURE_EN;

>   		val |= 1 << RDI_CFG0_TIMESTAMP_EN;

> -		val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;

> -		val |= DATA_TYPE_RAW_10BIT << RDI_CFG0_DATA_TYPE;

> +		val |= format->decode_format << RDI_CFG0_DECODE_FORMAT;

> +		val |= format->data_type << RDI_CFG0_DATA_TYPE;


I've given it a try on RB3 board (aka db845c plus the navigation mezzanine), which uses ov8856 camera
sensor (its output format is SGRBG10_1X10).

The above change doesn't work for me because format->decode_format has the value of 0x02 (which is
DECODE_FORMAT_UNCOMPRESSED_10_BIT). format->data_type has the expected value of 0x2b (DATA_TYPE_RAW_10BIT).

Thanks,
Andrey

>   		val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;

>   		val |= dt_id << RDI_CFG0_DT_ID;

>   		writel_relaxed(val, csid->base + CSID_RDI_CFG0(0));

>
Jonathan Marek May 25, 2021, 6:38 p.m. UTC | #9
On 5/25/21 2:15 PM, Andrey Konovalov wrote:
> Hi Jonathan,

> 

> Thank you for your patch!

> 

> On 11.05.2021 21:07, Jonathan Marek wrote:

>> Use the decode_format/data_type from the "format" struct instead of a

>> hardcoded 10-bit format.

>>

>> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware 

>> version Titan 170")

>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

>> ---

>>   drivers/media/platform/qcom/camss/camss-csid-170.c | 6 +++---

>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>

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

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

>> index ac22ff29d2a9..a81cc94c075f 100644

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

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

>> @@ -366,7 +366,7 @@ static void csid_configure_stream(struct 

>> csid_device *csid, u8 enable)

>>               val |= input_format->width & 0x1fff << 

>> TPG_DT_n_CFG_0_FRAME_WIDTH;

>>               writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_0(0));

>> -            val = DATA_TYPE_RAW_10BIT << TPG_DT_n_CFG_1_DATA_TYPE;

>> +            val = format->data_type << TPG_DT_n_CFG_1_DATA_TYPE;

>>               writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_1(0));

>>               val = tg->mode << TPG_DT_n_CFG_2_PAYLOAD_MODE;

>> @@ -382,8 +382,8 @@ static void csid_configure_stream(struct 

>> csid_device *csid, u8 enable)

>>           val = 1 << RDI_CFG0_BYTE_CNTR_EN;

>>           val |= 1 << RDI_CFG0_FORMAT_MEASURE_EN;

>>           val |= 1 << RDI_CFG0_TIMESTAMP_EN;

>> -        val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;

>> -        val |= DATA_TYPE_RAW_10BIT << RDI_CFG0_DATA_TYPE;

>> +        val |= format->decode_format << RDI_CFG0_DECODE_FORMAT;

>> +        val |= format->data_type << RDI_CFG0_DATA_TYPE;

> 

> I've given it a try on RB3 board (aka db845c plus the navigation 

> mezzanine), which uses ov8856 camera

> sensor (its output format is SGRBG10_1X10).

> 

> The above change doesn't work for me because format->decode_format has 

> the value of 0x02 (which is

> DECODE_FORMAT_UNCOMPRESSED_10_BIT). format->data_type has the expected 

> value of 0x2b (DATA_TYPE_RAW_10BIT).

> 


I will change it back to using DECODE_FORMAT_PAYLOAD_ONLY for the v2, 
since it does seem like this is the correct value for the RDI path.

(but IIRC, using DECODE_FORMAT_UNCOMPRESSED_10_BIT worked on RB3 with 
the ov8856 camera last year when I brought it up. maybe the VFE or 
another register is configured differently)

> Thanks,

> Andrey

> 

>>           val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;

>>           val |= dt_id << RDI_CFG0_DT_ID;

>>           writel_relaxed(val, csid->base + CSID_RDI_CFG0(0));

>>
Andrey Konovalov May 26, 2021, 1:59 p.m. UTC | #10
Hi Jonathan,

On 25.05.2021 21:38, Jonathan Marek wrote:
> On 5/25/21 2:15 PM, Andrey Konovalov wrote:

>> Hi Jonathan,

>>

>> Thank you for your patch!

>>

>> On 11.05.2021 21:07, Jonathan Marek wrote:

>>> Use the decode_format/data_type from the "format" struct instead of a

>>> hardcoded 10-bit format.

>>>

>>> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")

>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

>>> ---

>>>   drivers/media/platform/qcom/camss/camss-csid-170.c | 6 +++---

>>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>>

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

>>> index ac22ff29d2a9..a81cc94c075f 100644

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

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

>>> @@ -366,7 +366,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)

>>>               val |= input_format->width & 0x1fff << TPG_DT_n_CFG_0_FRAME_WIDTH;

>>>               writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_0(0));

>>> -            val = DATA_TYPE_RAW_10BIT << TPG_DT_n_CFG_1_DATA_TYPE;

>>> +            val = format->data_type << TPG_DT_n_CFG_1_DATA_TYPE;

>>>               writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_1(0));

>>>               val = tg->mode << TPG_DT_n_CFG_2_PAYLOAD_MODE;

>>> @@ -382,8 +382,8 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)

>>>           val = 1 << RDI_CFG0_BYTE_CNTR_EN;

>>>           val |= 1 << RDI_CFG0_FORMAT_MEASURE_EN;

>>>           val |= 1 << RDI_CFG0_TIMESTAMP_EN;

>>> -        val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;

>>> -        val |= DATA_TYPE_RAW_10BIT << RDI_CFG0_DATA_TYPE;

>>> +        val |= format->decode_format << RDI_CFG0_DECODE_FORMAT;

>>> +        val |= format->data_type << RDI_CFG0_DATA_TYPE;

>>

>> I've given it a try on RB3 board (aka db845c plus the navigation mezzanine), which uses ov8856 camera

>> sensor (its output format is SGRBG10_1X10).

>>

>> The above change doesn't work for me because format->decode_format has the value of 0x02 (which is

>> DECODE_FORMAT_UNCOMPRESSED_10_BIT). format->data_type has the expected value of 0x2b (DATA_TYPE_RAW_10BIT).

>>

> 

> I will change it back to using DECODE_FORMAT_PAYLOAD_ONLY for the v2, since it does seem like this is the correct value for the 

> RDI path.


Sounds good. Just in case, I've tried

--- a/drivers/media/platform/qcom/camss/camss-csid-170.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-170.c
@@ -390,7 +390,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
                 val = 1 << RDI_CFG0_BYTE_CNTR_EN;
                 val |= 1 << RDI_CFG0_FORMAT_MEASURE_EN;
                 val |= 1 << RDI_CFG0_TIMESTAMP_EN;
-               val |= format->decode_format << RDI_CFG0_DECODE_FORMAT;
+               val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
                 val |= format->data_type << RDI_CFG0_DATA_TYPE;
                 val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;
                 val |= dt_id << RDI_CFG0_DT_ID;

on top of v1 patchset, and it worked OK on RB3 with the ov8856 camera.

I planned to check the other RB3's camera too - the "tracking" ov7251 one, but it turned out that I don't
have the information on which particular SOC's MIPI lanes are connected to this camera. I've tried a few
clock-lanes/data-lanes combinations, but none worked (ov7251 is controlled OK via the i2c bus, but no
data are captured).

> (but IIRC, using DECODE_FORMAT_UNCOMPRESSED_10_BIT worked on RB3 with the ov8856 camera last year when I brought it up. maybe 

> the VFE or another register is configured differently)


OK. I only used the current upstream camss driver, so have nothing to compare it against.

I also noticed that with ov8856 camera, only 3264x2448 and 1632x1224 modes work correctly on RB3
(used the current media_tree). But with the below change all the other modes (3280x2464 and 1640x1232)
start working OK too:

--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -1575,7 +1575,7 @@ int msm_vfe_register_entities(struct vfe_device *vfe,
                 }

                 video_out->ops = &vfe->video_ops;
-               video_out->bpl_alignment = 8;
+               video_out->bpl_alignment = 16;
                 video_out->line_based = 0;
                 if (i == VFE_LINE_PIX) {
                         video_out->bpl_alignment = 16;

And (with the increased size of the capture buffer) I did see that the data captured from camss were padded
to the next 16-byte boundary with the data from the previous 16-byte chunk. E.g. in the 3280x2464 case 12
padding bytes were added.
Personally I don't have access to the datasheet, but I've been told that as per the docs, 16-byte alignment
should not be needed in this case (RAW10P camera connected through rdi).
Does it ring any bell for you?

Thanks,
Andrey

>> Thanks,

>> Andrey

>>

>>>           val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;

>>>           val |= dt_id << RDI_CFG0_DT_ID;

>>>           writel_relaxed(val, csid->base + CSID_RDI_CFG0(0));

>>>
Robert Foss May 31, 2021, 11:02 a.m. UTC | #11
On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>

> This isn't used and only works because devm_regulator_get() returns a dummy

> regulator.

>

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

> ---

>  drivers/media/platform/qcom/camss/camss.c | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

>

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

> index ef100d5f7763..c08d6d6f6f90 100644

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

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

> @@ -542,7 +542,7 @@ static const struct resources csiphy_res_845[] = {

>  static const struct resources csid_res_845[] = {

>         /* CSID0 */

>         {

> -               .regulator = { "vdda-csi0" },

> +               .regulator = { NULL },

>                 .clock = { "cpas_ahb", "cphy_rx_src", "slow_ahb_src",

>                                 "soc_ahb", "vfe0", "vfe0_src",

>                                 "vfe0_cphy_rx", "csi0",

> @@ -562,7 +562,7 @@ static const struct resources csid_res_845[] = {

>

>         /* CSID1 */

>         {

> -               .regulator = { "vdda-csi1" },

> +               .regulator = { NULL },

>                 .clock = { "cpas_ahb", "cphy_rx_src", "slow_ahb_src",

>                                 "soc_ahb", "vfe1", "vfe1_src",

>                                 "vfe1_cphy_rx", "csi1",

> @@ -582,7 +582,7 @@ static const struct resources csid_res_845[] = {

>

>         /* CSID2 */

>         {

> -               .regulator = { "vdda-csi2" },

> +               .regulator = { NULL },

>                 .clock = { "cpas_ahb", "cphy_rx_src", "slow_ahb_src",

>                                 "soc_ahb", "vfe_lite", "vfe_lite_src",

>                                 "vfe_lite_cphy_rx", "csi2",


Reviewed-by: Robert Foss <robert.foss@linaro.org>
Robert Foss May 31, 2021, 11:11 a.m. UTC | #12
On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>

> vfe_isr_read()/vfe_isr_halt_ack()/vfe_reg_clr() are never called.

>

> vfe_isr_sof() does nothing, remove it.

>

> The only vfe_reg_set() usage can be easily replaced with a writel.

>

> Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170")

> Signed-off-by: Jonathan Marek <jonathan@marek.ca>

> ---

>  .../media/platform/qcom/camss/camss-vfe-170.c | 53 +------------------

>  1 file changed, 2 insertions(+), 51 deletions(-)

>

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

> index 8594d275b41d..076ca082e107 100644

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

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

> @@ -188,20 +188,6 @@ static void vfe_hw_version_read(struct vfe_device *vfe, struct device *dev)

>         dev_err(dev, "VFE HW Version = %u.%u.%u\n", gen, rev, step);

>  }

>

> -static inline void vfe_reg_clr(struct vfe_device *vfe, u32 reg, u32 clr_bits)

> -{

> -       u32 bits = readl_relaxed(vfe->base + reg);

> -

> -       writel_relaxed(bits & ~clr_bits, vfe->base + reg);

> -}

> -

> -static inline void vfe_reg_set(struct vfe_device *vfe, u32 reg, u32 set_bits)

> -{

> -       u32 bits = readl_relaxed(vfe->base + reg);

> -

> -       writel_relaxed(bits | set_bits, vfe->base + reg);

> -}

> -

>  static void vfe_global_reset(struct vfe_device *vfe)

>  {

>         u32 reset_bits = GLOBAL_RESET_CMD_CORE          |

> @@ -305,32 +291,14 @@ static inline void vfe_reg_update_clear(struct vfe_device *vfe,

>

>  static void vfe_enable_irq_common(struct vfe_device *vfe)

>  {

> -       vfe_reg_set(vfe, VFE_IRQ_MASK_0, ~0u);

> -       vfe_reg_set(vfe, VFE_IRQ_MASK_1, ~0u);

> +       writel_relaxed(~0u, vfe->base + VFE_IRQ_MASK_0);

> +       writel_relaxed(~0u, vfe->base + VFE_IRQ_MASK_1);

>

>         writel_relaxed(~0u, vfe->base + VFE_BUS_IRQ_MASK(0));

>         writel_relaxed(~0u, vfe->base + VFE_BUS_IRQ_MASK(1));

>         writel_relaxed(~0u, vfe->base + VFE_BUS_IRQ_MASK(2));

>  }

>

> -static void vfe_isr_halt_ack(struct vfe_device *vfe)

> -{

> -       complete(&vfe->halt_complete);

> -}

> -

> -static void vfe_isr_read(struct vfe_device *vfe, u32 *status0, u32 *status1)

> -{

> -       *status0 = readl_relaxed(vfe->base + VFE_IRQ_STATUS_0);

> -       *status1 = readl_relaxed(vfe->base + VFE_IRQ_STATUS_1);

> -

> -       writel_relaxed(*status0, vfe->base + VFE_IRQ_CLEAR_0);

> -       writel_relaxed(*status1, vfe->base + VFE_IRQ_CLEAR_1);

> -

> -       /* Enforce ordering between IRQ Clear and Global IRQ Clear */

> -       wmb();

> -       writel_relaxed(CMD_GLOBAL_CLEAR, vfe->base + VFE_IRQ_CMD);

> -}

> -

>  static void vfe_violation_read(struct vfe_device *vfe)

>  {

>         u32 violation = readl_relaxed(vfe->base + VFE_VIOLATION_STATUS);

> @@ -375,10 +343,6 @@ static irqreturn_t vfe_isr(int irq, void *dev)

>                 if (status0 & STATUS_0_RDI_REG_UPDATE(i))

>                         vfe->isr_ops.reg_update(vfe, i);

>

> -       for (i = VFE_LINE_RDI0; i <= VFE_LINE_RDI2; i++)

> -               if (status0 & STATUS_1_RDI_SOF(i))

> -                       vfe->isr_ops.sof(vfe, i);

> -

>         for (i = 0; i < MSM_VFE_COMPOSITE_IRQ_NUM; i++)

>                 if (vfe_bus_status[0] & STATUS0_COMP_BUF_DONE(i))

>                         vfe->isr_ops.comp_done(vfe, i);

> @@ -607,16 +571,6 @@ static int vfe_disable(struct vfe_line *line)

>         return 0;

>  }

>

> -/*

> - * vfe_isr_sof - Process start of frame interrupt

> - * @vfe: VFE Device

> - * @line_id: VFE line

> - */

> -static void vfe_isr_sof(struct vfe_device *vfe, enum vfe_line_id line_id)

> -{

> -       /* nop */

> -}

> -

>  /*

>   * vfe_isr_reg_update - Process reg update interrupt

>   * @vfe: VFE Device

> @@ -749,9 +703,7 @@ static int vfe_queue_buffer(struct camss_video *vid,

>

>  static const struct vfe_isr_ops vfe_isr_ops_170 = {

>         .reset_ack = vfe_isr_reset_ack,

> -       .halt_ack = vfe_isr_halt_ack,

>         .reg_update = vfe_isr_reg_update,

> -       .sof = vfe_isr_sof,

>         .comp_done = vfe_isr_comp_done,

>         .wm_done = vfe_isr_wm_done,

>  };

> @@ -772,7 +724,6 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)

>  const struct vfe_hw_ops vfe_ops_170 = {

>         .global_reset = vfe_global_reset,

>         .hw_version_read = vfe_hw_version_read,

> -       .isr_read = vfe_isr_read,

>         .isr = vfe_isr,

>         .pm_domain_off = vfe_pm_domain_off,

>         .pm_domain_on = vfe_pm_domain_on,


Gen1 & gen2 seem to diverge in terms of which ISR & helpers are
needed. Should the vfe_isr_ops & vfe_hw_ops structs have generation
based versions? I'm fine with a no here. There are diminishing returns
to this pedantism.

With ^^^ covered, feel free to add my r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>
Robert Foss May 31, 2021, 11:13 a.m. UTC | #13
On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>
> This function waits for halt_complete but doesn't do anything to cause
> it to complete, and always hits the "VFE halt timeout" error. Just delete
> this code for now.
>
> Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe-170.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> index 076ca082e107..080eef767d3b 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> @@ -363,17 +363,7 @@ static irqreturn_t vfe_isr(int irq, void *dev)
>   */
>  static int vfe_halt(struct vfe_device *vfe)
>  {
> -       unsigned long time;
> -
> -       reinit_completion(&vfe->halt_complete);
> -
> -       time = wait_for_completion_timeout(&vfe->halt_complete,
> -                                          msecs_to_jiffies(VFE_HALT_TIMEOUT_MS));
> -       if (!time) {
> -               dev_err(vfe->camss->dev, "VFE halt timeout\n");
> -               return -EIO;
> -       }
> -
> +       /* rely on vfe_disable_output() to stop the VFE */
>         return 0;
>  }


Reviewed-by: Robert Foss <robert.foss@linaro.org>
Robert Foss May 31, 2021, 12:14 p.m. UTC | #14
On Tue, 11 May 2021 at 20:08, Jonathan Marek <jonathan@marek.ca> wrote:
>
> Add this common format to the various format lists relevant to sdm845.
>
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/media/platform/qcom/camss/camss-csid-170.c | 7 +++++++
>  drivers/media/platform/qcom/camss/camss-csiphy.c   | 1 +
>  drivers/media/platform/qcom/camss/camss-vfe.c      | 1 +
>  drivers/media/platform/qcom/camss/camss-video.c    | 2 ++
>  4 files changed, 11 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-170.c b/drivers/media/platform/qcom/camss/camss-csid-170.c
> index 9f6334fd68fc..5258e2099a43 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-170.c
> @@ -262,6 +262,13 @@ static const struct csid_format csid_formats[] = {
>                 10,
>                 1,
>         },
> +       {
> +               MEDIA_BUS_FMT_Y8_1X8,
> +               DATA_TYPE_RAW_8BIT,
> +               DECODE_FORMAT_UNCOMPRESSED_8_BIT,
> +               10,
> +               1,
> +       },
>         {
>                 MEDIA_BUS_FMT_Y10_1X10,
>                 DATA_TYPE_RAW_10BIT,
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index b3c3bf19e522..f82f1e2aa688 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -94,6 +94,7 @@ static const struct csiphy_format csiphy_formats_sdm845[] = {
>         { MEDIA_BUS_FMT_SGBRG14_1X14, 14 },
>         { MEDIA_BUS_FMT_SGRBG14_1X14, 14 },
>         { MEDIA_BUS_FMT_SRGGB14_1X14, 14 },
> +       { MEDIA_BUS_FMT_Y8_1X8, 8 },
>         { MEDIA_BUS_FMT_Y10_1X10, 10 },
>  };
>
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index dec89079c6ae..e7ab2c175ac9 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -118,6 +118,7 @@ static const struct vfe_format formats_rdi_845[] = {
>         { MEDIA_BUS_FMT_SGBRG14_1X14, 14 },
>         { MEDIA_BUS_FMT_SGRBG14_1X14, 14 },
>         { MEDIA_BUS_FMT_SRGGB14_1X14, 14 },
> +       { MEDIA_BUS_FMT_Y8_1X8, 8 },
>         { MEDIA_BUS_FMT_Y10_1X10, 10 },
>         { MEDIA_BUS_FMT_Y10_2X8_PADHI_LE, 16 },
>  };
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index f282275af626..54e77d30d452 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -176,6 +176,8 @@ static const struct camss_format_info formats_rdi_845[] = {
>           { { 1, 1 } }, { { 1, 1 } }, { 14 } },
>         { MEDIA_BUS_FMT_SRGGB14_1X14, V4L2_PIX_FMT_SRGGB14P, 1,
>           { { 1, 1 } }, { { 1, 1 } }, { 14 } },
> +       { MEDIA_BUS_FMT_Y8_1X8, V4L2_PIX_FMT_GREY, 1,
> +         { { 1, 1 } }, { { 1, 1 } }, { 8 } },
>         { MEDIA_BUS_FMT_Y10_1X10, V4L2_PIX_FMT_Y10P, 1,
>           { { 1, 1 } }, { { 1, 1 } }, { 10 } },
>         { MEDIA_BUS_FMT_Y10_2X8_PADHI_LE, V4L2_PIX_FMT_Y10, 1,

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