Re: [PATCH v4 3/5] media: platform: mediatek: isp_30: add mediatek ISP3.0 sensor interface

[Julien Stephan <jstephan@baylibre.com>]

Hi Angelo,

Thank you for your comments. Few questions below.

Cheers
Julien

Le jeu. 11 janv. 2024 à 13:04, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> a écrit :
 .. snip ..
>
> > +             .code = MEDIA_BUS_FMT_S5C_UYVY_JPEG_1X8,
> > +             .flags = MTK_SENINF_FORMAT_JPEG,
> > +     },
> > +     /* Keep the input-only formats last. */
>
> Your comment doesn't make me understand why input-only formats shall be
> placed last - and makes me think that having two arrays (one for both
> and one for input only) would be easier and less error prone, other than
> making you able to drop the MTK_SENINF_FORMAT_INPUT_ONLY flag entirely.
>

Not sure I agree with you on this... If I want to split into two I
will have to duplicate
the whole mtk_seninf_formats[] which is quite big because the first 26
formats are
for both input and source pad. Moreover it will introduce some check on the pad
before choosing the correct format structure..  or maybe I am missing
your point?

> > +     {
> > +             .code = MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8,
> > +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
> > +     }, {
> > +             .code = MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8,
> > +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
> > +     }, {
> > +             .code = MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8,
> > +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
> > +     }, {
> > +             .code = MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8,
> > +             .flags = MTK_SENINF_FORMAT_DPCM | MTK_SENINF_FORMAT_INPUT_ONLY,
> > +     }
> > +};
> > +
> > +static const struct mtk_seninf_format_info *mtk_seninf_format_info(u32 code)
> > +{
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(mtk_seninf_formats); ++i) {
> > +             if (mtk_seninf_formats[i].code == code)
> > +                     return &mtk_seninf_formats[i];
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
>
> ..snip..
>
> > +
> > +static void mtk_seninf_input_setup_csi2(struct mtk_seninf_input *input,
> > +                                     struct v4l2_subdev_state *state)
> > +{
> > +     const struct mtk_seninf_format_info *fmtinfo;
> > +     const struct v4l2_mbus_framefmt *format;
> > +     unsigned int num_data_lanes = input->bus.num_data_lanes;
> > +     unsigned int val = 0;
> > +
> > +     format = v4l2_subdev_state_get_stream_format(state, input->pad, 0);
> > +     fmtinfo = mtk_seninf_format_info(format->code);
> > +
> > +     /* Configure timestamp */
> > +     writel(SENINF_TIMESTAMP_STEP, input->base + SENINF_TG1_TM_STP);
> > +
> > +     /* HQ */
> > +     writel(0x0, input->base + SENINF_TG1_PH_CNT);
>
> Zero means:
>   - Sensor master clock: ISP_CLK
>   - Sensor clock polarity: Rising edge
>   - Sensor reset deasserted
>   - Sensor powered up
>   - Pixel clock inversion disabled
>   - Sensor master clock polarity disabled
>   - Phase counter disabled
>
> > +     writel(0x10001, input->base + SENINF_TG1_SEN_CK);
>
> Unroll this one... this is the TG1 sensor clock divider.
>
> CLKFL GENMASK(5, 0)
> CLKRS GENMASK(13, 8)
> CLKCNT GENMASK(21,16)
>
> Like this, I don't get what you're trying to set, because you're using a fixed
> sensor clock rate, meaning that only a handful of camera sensors will be usable.
>
> Is this 8Mhz? 16? 24? what? :-)
>
> Two hints:
>   - sensor_clk = clk_get_rate(isp_clk) / (tg1_sen_ck_clkcnt + 1);
>   - int mtk_seninf_set_sensor_clk(u8 rate_mhz);
>
> Please :-)
>
> > +
> > +     /* First Enable Sensor interface and select pad (0x1a04_0200) */
> > +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_EN, 1);
> > +     mtk_seninf_input_update(input, SENINF_CTRL, PAD2CAM_DATA_SEL, SENINF_PAD_10BIT);
> > +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_SRC_SEL, 0);
> > +     mtk_seninf_input_update(input, SENINF_CTRL_EXT, SENINF_CSI2_IP_EN, 1);
> > +     mtk_seninf_input_update(input, SENINF_CTRL_EXT, SENINF_NCSI2_IP_EN, 0);
> > +
> > +     /* DPCM Enable */
> > +     if (fmtinfo->flags & MTK_SENINF_FORMAT_DPCM)
> > +             val = SENINF_CSI2_DPCM_DI_2A_DPCM_EN;
> > +     else
> > +             val = SENINF_CSI2_DPCM_DI_30_DPCM_EN;
> > +     writel(val, input->base + SENINF_CSI2_DPCM);
> > +
> > +     /* Settle delay */
> > +     mtk_seninf_input_update(input, SENINF_CSI2_LNRD_TIMING,
> > +                             DATA_SETTLE_PARAMETER, SENINF_SETTLE_DELAY);
> > +
> > +     /* HQ */
> > +     writel(0x10, input->base + SENINF_CSI2_LNRC_FSM);
>
> As far as I know, SENINF_CSI2_LNRC_FSM is a read-only register: this write will do
> exactly nothing...
>
> > +
> > +     /* CSI2 control */
> > +     val = readl(input->base + SENINF_CSI2_CTL)
> > +         | (FIELD_PREP(SENINF_CSI2_CTL_ED_SEL, DATA_HEADER_ORDER_DI_WCL_WCH)
> > +         | SENINF_CSI2_CTL_CLOCK_LANE_EN | (BIT(num_data_lanes) - 1));
> > +     writel(val, input->base + SENINF_CSI2_CTL);
> > +
> > +     mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL,
> > +                             BYPASS_LANE_RESYNC, 0);
>
> 93 columns: fits in one line (not only this one!).
>
> > +     mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL, CDPHY_SEL, 0);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_RESYNC_MERGE_CTL,
> > +                             CPHY_LANE_RESYNC_CNT, 3);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_MODE, CSR_CSI2_MODE, 0);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_MODE, CSR_CSI2_HEADER_LEN, 0);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_DPHY_SYNC, SYNC_SEQ_MASK_0, 0xff00);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_DPHY_SYNC, SYNC_SEQ_PAT_0, 0x001d);
> > +
> > +     mtk_seninf_input_update(input, SENINF_CSI2_CTL, CLOCK_HS_OPTION, 0);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_CTL, HSRX_DET_EN, 0);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_CTL, HS_TRAIL_EN, 1);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_HS_TRAIL, HS_TRAIL_PARAMETER,
> > +                             SENINF_HS_TRAIL_PARAMETER);
> > +
> > +     /* Set debug port to output packet number */
> > +     mtk_seninf_input_update(input, SENINF_CSI2_DGB_SEL, DEBUG_EN, 1);
> > +     mtk_seninf_input_update(input, SENINF_CSI2_DGB_SEL, DEBUG_SEL, 0x1a);
> > +
> > +     /* HQ */
> > +     writel(0xfffffffe, input->base + SENINF_CSI2_SPARE0);
>
> I have no idea what this SPARE0 does, but I think that this is something that you
> want to get from platform_data, as I guess this would be different on various SoCs?
>
> > +
> > +     /* Enable CSI2 IRQ mask */
> > +     /* Turn on all interrupt */
> > +     writel(0xffffffff, input->base + SENINF_CSI2_INT_EN);
> > +     /* Write clear CSI2 IRQ */
> > +     writel(0xffffffff, input->base + SENINF_CSI2_INT_STATUS);
> > +     /* Enable CSI2 Extend IRQ mask */
>
> You missed:
>         writel(0xffffffff, input->base + SENINF_CSI2_INT_EN_EXT);
>
> P.S.: #define SENINF_CSI2_INT_EN_EXT 0x0b10
>
>
> > +     /* Turn on all interrupt */
>
> /* Reset the CSI2 to commit changes */ <-- makes more sense, doesn't it?
>
> > +     mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 1);
> > +     udelay(1);
> > +     mtk_seninf_input_update(input, SENINF_CTRL, CSI2_SW_RST, 0);
> > +}
> > +
> > +static void mtk_seninf_mux_setup(struct mtk_seninf_mux *mux,
> > +                              struct mtk_seninf_input *input,
> > +                              struct v4l2_subdev_state *state)
> > +{
> > +     const struct mtk_seninf_format_info *fmtinfo;
> > +     const struct v4l2_mbus_framefmt *format;
> > +     unsigned int pix_sel_ext;
> > +     unsigned int pix_sel;
> > +     unsigned int hs_pol = 0;
> > +     unsigned int vs_pol = 0;
> > +     unsigned int val;
> > +     u32 rst_mask;
> > +
> > +     format = v4l2_subdev_state_get_stream_format(state, input->pad, 0);
> > +     fmtinfo = mtk_seninf_format_info(format->code);
> > +
> > +     /* Enable mux */
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_MUX_EN, 1);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_SRC_SEL, SENINF_MIPI_SENSOR);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT, SENINF_SRC_SEL_EXT, SENINF_NORMAL_MODEL);
> > +
> > +     pix_sel_ext = 0;
> > +     pix_sel = 1;
>
>
>         pixels_per_cycle = 1;
>         bus_width = pixels_per_cycle >> 1;
>
> because:  0 == 1pix/cyc, 1 == 2pix/cyc, 2 == 4pix/cyc, 3 == 8pix... etc
> ...but the width of this register depends on the SoC, so you also want to set
> constraints to the bus width on a per-soc basis (platform data again, or at
> least leave a comment here).>
>         mtk_seninf_mux_update(....  PIX_SEL_EXT, bus_width);
>         mtk_seninf_mux_update(....  PIX_SEL, bus_width);
>

Again, not sure to get your point here. PIX_SEL_EXT and PIX_SEL are 1 bit each
and according to the datasheet 1pix/cycle is 0 in PIX_SEL_EXT and 0 in PIX_SEL,
2 pix/cycles is 0 in PIX_SEL_EXT and 1 in PIX_SEL
and 4 pix/cycle is 1 in PIX_SEL_EXT and 0 in PIX_SEL


> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT, SENINF_PIX_SEL_EXT, pix_sel_ext);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_PIX_SEL, pix_sel);
> > +
> > +     if (fmtinfo->flags & MTK_SENINF_FORMAT_JPEG) {
> > +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 0);
> > +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN,
> > +                                   FIFO_FLUSH_EN_JPEG_2_PIXEL_MODE);
> > +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN,
> > +                             FIFO_PUSH_EN_JPEG_2_PIXEL_MODE);
> > +     } else {
> > +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 2);
> > +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN,
> > +                                  FIFO_FLUSH_EN_NORMAL_MODE);
> > +             mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN,
> > +                             FIFO_PUSH_EN_NORMAL_MODE);
> > +     }
> > +
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_HSYNC_POL, hs_pol);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_VSYNC_POL, vs_pol);
> > +
> > +     val = mtk_seninf_mux_read(mux, SENINF_MUX_CTRL);
> > +     rst_mask = SENINF_MUX_CTRL_SENINF_IRQ_SW_RST | SENINF_MUX_CTRL_SENINF_MUX_SW_RST;
> > +
> > +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val | rst_mask);
>
> Are you sure that you don't need any wait between assertion and deassertion of RST?
> Looks strange, but I don't really know then.
>
> > +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL, val & ~rst_mask);
> > +
> > +     /* HQ */
> > +     mtk_seninf_mux_write(mux, SENINF_MUX_SPARE, 0xc2000);
>
> val = SENINF_FIFO_FULL_SEL;
>
> /* SPARE field meaning is unknown */
> val |= 0xc0000;
>
>         mtk_seninf_mux_write(mux, SENINF_MUX_SPARE, val);
>
> > +}
> > +
> > +static void mtk_seninf_top_mux_setup(struct mtk_seninf *priv,
> > +                                  enum mtk_seninf_id seninf_id,
> > +                                  struct mtk_seninf_mux *mux)
> > +{
> > +     unsigned int val;
> > +
> > +     /*
> > +      * Use the top mux (from SENINF input to MUX) to configure routing, and
> > +      * hardcode a 1:1 mapping from the MUX instances to the SENINF outputs.
> > +      */
> > +     val = readl(priv->base + SENINF_TOP_MUX_CTRL)
> > +             & ~(0xf << (mux->mux_id * 4));
> > +     val |= (seninf_id & 0xf) << (mux->mux_id * 4);
> > +     writel(val, priv->base + SENINF_TOP_MUX_CTRL);
> > +
> > +     writel(0x76541010, priv->base + SENINF_TOP_CAM_MUX_CTRL);
>
> Each four bits of TOP_CAM_MUX_CTRL selects between seninf1 to seninf8 muxes, and
> TOP_MUX_CTRL is laid out in the very same way.
>
> This means that if you're calculating a value for TOP_MUX_CTRL, you can do exactly
> the same for TOP_CAM_MUX_CTRL.
>
> > +}
> > +
> > +static void seninf_enable_test_pattern(struct mtk_seninf *priv,
> > +                                    struct v4l2_subdev_state *state)
> > +{
> > +     struct mtk_seninf_input *input = &priv->inputs[CSI_PORT_0];
> > +     struct mtk_seninf_mux *mux = &priv->muxes[0];
> > +     const struct mtk_seninf_format_info *fmtinfo;
> > +     const struct v4l2_mbus_framefmt *format;
> > +     unsigned int val;
> > +     unsigned int pix_sel_ext;
> > +     unsigned int pix_sel;
> > +     unsigned int hs_pol = 0;
> > +     unsigned int vs_pol = 0;
> > +     unsigned int seninf = 0;
> > +     unsigned int tm_size = 0;
> > +     unsigned int mux_id = mux->mux_id;
> > +
> > +     format = v4l2_subdev_state_get_stream_format(state, priv->conf->nb_inputs, 0);
> > +     fmtinfo = mtk_seninf_format_info(format->code);
> > +
> > +     mtk_seninf_update(priv, SENINF_TOP_CTRL, MUX_LP_MODE, 0);
> > +
> > +     mtk_seninf_update(priv, SENINF_TOP_CTRL, SENINF_PCLK_EN, 1);
> > +     mtk_seninf_update(priv, SENINF_TOP_CTRL, SENINF2_PCLK_EN, 1);
> > +
> > +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_EN, 1);
> > +     mtk_seninf_input_update(input, SENINF_CTRL, SENINF_SRC_SEL, 1);
> > +     mtk_seninf_input_update(input, SENINF_CTRL_EXT,
> > +                             SENINF_TESTMDL_IP_EN, 1);
> > +
> > +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_EN, 1);
> > +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_PAT, 0xc);
> > +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_VSYNC, 4);
> > +     mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_DUMMYPXL, 0x28);
> > +
> > +     if (fmtinfo->flags & MTK_SENINF_FORMAT_BAYER)
> > +             mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_FMT, 0x0);
> > +     else
> > +             mtk_seninf_input_update(input, SENINF_TG1_TM_CTL, TM_FMT, 0x1);
> > +
> > +     tm_size = FIELD_PREP(SENINF_TG1_TM_SIZE_TM_LINE, format->height + 8);
> > +     switch (format->code) {
> > +     case MEDIA_BUS_FMT_UYVY8_1X16:
> > +     case MEDIA_BUS_FMT_VYUY8_1X16:
> > +     case MEDIA_BUS_FMT_YUYV8_1X16:
> > +     case MEDIA_BUS_FMT_YVYU8_1X16:
> > +             tm_size |= FIELD_PREP(SENINF_TG1_TM_SIZE_TM_PXL, format->width * 2);
> > +             break;
> > +     default:
> > +             tm_size |= FIELD_PREP(SENINF_TG1_TM_SIZE_TM_PXL, format->width);
> > +             break;
> > +     }
> > +     writel(tm_size, input->base + SENINF_TG1_TM_SIZE);
> > +
> > +     writel(TEST_MODEL_CLK_DIVIDED_CNT, input->base + SENINF_TG1_TM_CLK);
> > +     writel(TIME_STAMP_DIVIDER, input->base + SENINF_TG1_TM_STP);
> > +
> > +     /* Set top mux */
> > +     val = (readl(priv->base + SENINF_TOP_MUX_CTRL) & (~(0xf << (mux_id * 4)))) |
> > +             ((seninf & 0xf) << (mux_id * 4));
> > +     writel(val, priv->base + SENINF_TOP_MUX_CTRL);
>
> This is duplicated, and it is the same that you have in mtk_seninf_top_mux_setup()
>
> > +
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_MUX_EN, 1);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT,
> > +                           SENINF_SRC_SEL_EXT, SENINF_TEST_MODEL);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_SRC_SEL, 1);
> > +
> > +     pix_sel_ext = 0;
> > +     pix_sel = 1;
> > +
>
> This is in mtk_seninf_mux_setup(), but if you apply my suggestion, it won't be in
> there anymore, so you'll call a function here to set the right value :-)
>
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL_EXT,
> > +                           SENINF_PIX_SEL_EXT, pix_sel_ext);
> > +
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_PIX_SEL, pix_sel);
> > +
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_PUSH_EN, 0x1f);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FLUSH_EN, 0x1b);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, FIFO_FULL_WR_EN, 2);
> > +
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_HSYNC_POL, hs_pol);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_VSYNC_POL, vs_pol);
> > +     mtk_seninf_mux_update(mux, SENINF_MUX_CTRL, SENINF_HSYNC_MASK, 1);
> > +
> > +     mtk_seninf_mux_write(mux, SENINF_MUX_INTEN,
> > +                          SENINF_IRQ_CLR_SEL | SENINF_ALL_ERR_IRQ_EN);
> > +
> > +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL,
> > +                          mtk_seninf_mux_read(mux, SENINF_MUX_CTRL) |
> > +                          SENINF_MUX_CTRL_SENINF_IRQ_SW_RST |
> > +                          SENINF_MUX_CTRL_SENINF_MUX_SW_RST);
> > +     udelay(1);
> > +     mtk_seninf_mux_write(mux, SENINF_MUX_CTRL,
> > +                          mtk_seninf_mux_read(mux, SENINF_MUX_CTRL) &
> > +                          ~(SENINF_MUX_CTRL_SENINF_IRQ_SW_RST |
> > +                            SENINF_MUX_CTRL_SENINF_MUX_SW_RST));
> > +
> > +     //check this
> > +     writel(0x76540010, priv->base + SENINF_TOP_CAM_MUX_CTRL);
> > +
> > +     dev_dbg(priv->dev, "%s: OK\n", __func__);
> > +}
> > +
>
> Cheers,
> Angelo
>