Re: [PATCH 2/2] media: nxp: add driver for i.MX93 MIPI CSI-2 controller and D-PHY

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Guoniu,

thanks for the fast response.

Am Mittwoch, 5. Juli 2023, 05:52:05 CEST schrieb G.N. Zhou (OSS):
> Hi Alexander,
> 
> Thanks for your comments.
> 
> [snip]
> > > +
> > > +/* Set default high speed frequency range to 1.5Gbps  */
> > > +#define DPHY_DEFAULT_FREQRANGE               0x2c
> > > +
> > > +enum imx93_csi_clks {
> > > +     PER,
> > > +     PIXEL,
> > > +     PHY_CFG,
> > > +};
> > > +
> > > +enum model {
> > > +     DWC_CSI2RX_IMX93,
> > > +};
> > > +
> > > +enum dwc_csi2rx_intf {
> > > +     DWC_CSI2RX_INTF_IDI,
> > 
> > 
> > This is unused, what is it intented for?
> 
> 
> DesignWare Core MIPI CSI-2 support both IDI and IPI interface. For i.MX93 it
> use IPI as interface with ISI(gasket) and I reserved IDI here on the one
> hand support full features of the MIPI CSI-2 IP as more as possible, on the
> other hand, NXP i.MX95 MIPI CSI-2 remove IPI and use IDI as the interface. 

I don't know about the differences on IPI and IDI, but it looks like both 
i.MX93 and i.MX95 use the same MIPI-CSI2 IP core, but have a different glue 
layer. So IPI and IDI specifics seem to be SoC specific as well. Did I get 
something wrong?

> [snip]

> > > ------------------------------------------------------------------------
> > > ---
 -- + * Debug
> > > + */
> > > +
> > > +static void dwc_csi_clear_counters(struct dwc_csi_device *csidev)
> > > +{
> > > +     unsigned long flags;
> > > +     unsigned int i;
> > > +
> > > +     spin_lock_irqsave(&csidev->slock, flags);
> > > +
> > > +     for (i = 0; i < csidev->pdata->num_events; ++i)
> > > +             csidev->events[i].counter = 0;
> > > +
> > > +     spin_unlock_irqrestore(&csidev->slock, flags);
> > > +}
> > > +
> > > +static void dwc_csi_log_counters(struct dwc_csi_device *csidev)
> > > +{
> > > +     unsigned int num_events = csidev->pdata->num_events;
> > > +     unsigned long flags;
> > > +     unsigned int i;
> > > +
> > > +     spin_lock_irqsave(&csidev->slock, flags);
> > > +
> > > +     for (i = 0; i < num_events; ++i) {
> > > +             if (csidev->events[i].counter > 0)
> > > +                     dev_info(csidev->dev, "%s events: %d\n",
> > > +                              csidev->events[i].name,
> > > +                              csidev->events[i].counter);
> > > +     }
> > > +
> > > +     spin_unlock_irqrestore(&csidev->slock, flags);
> > > +}
> > > +
> > > +static void dwc_csi_dump_regs(struct dwc_csi_device *csidev)
> > > +{
> > > +#define DWC_MIPI_CSIS_DEBUG_REG(name)                {name,
> > 
> > #name}
> > 
> > > +     static const struct {
> > > +             u32 offset;
> > > +             const char * const name;
> > > +     } registers[] = {
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_VERSION),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_N_LANES),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_HOST_RESETN),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_INT_ST_MAIN),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_DPHY_SHUTDOWNZ),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_DPHY_RSTZ),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_DPHY_RX_STATUS),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_DPHY_STOPSTATE),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_DPHY_TEST_CTRL0),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_DPHY_TEST_CTRL1),
> > > +
> > 
> > DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_PPI_PG_PATTERN_VRES),
> > 
> > > +
> > 
> > DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_PPI_PG_PATTERN_HRES),
> > 
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_PPI_PG_CONFIG),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_PPI_PG_ENABLE),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_PPI_PG_STATUS),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_IPI_MODE),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_IPI_VCID),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_IPI_DATA_TYPE),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_IPI_MEM_FLUSH),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_IPI_SOFTRSTN),
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_IPI_ADV_FEATURES),
> > > +
> > 
> > DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_INT_ST_DPHY_FATAL),
> > 
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_INT_ST_PKT_FATAL),
> > > +
> > 
> > DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_INT_ST_DPHY_FATAL),
> > 
> > > +             DWC_MIPI_CSIS_DEBUG_REG(CSI2RX_INT_ST_IPI_FATAL),
> > > +     };
> > > +
> > > +     unsigned int i;
> > > +     u32 cfg;
> > > +
> > > +     dev_dbg(csidev->dev, "--- REGISTERS ---\n");
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(registers); i++) {
> > > +             cfg = dwc_csi_read(csidev, registers[i].offset);
> > > +             dev_dbg(csidev->dev, "%14s[0x%02x]: 0x%08x\n",
> > > +                     registers[i].name, registers[i].offset, cfg);
> > > +     }
> > > +}

These register dumps also look like a good candidate for 
v4l2_subdev_core_ops.log_status callback. VIDIOC_LOG_STATUS ioctl that is.

> [snip]

> > > +static int dwc_csi_subdev_set_fmt(struct v4l2_subdev *sd,
> > > +                                struct v4l2_subdev_state *sd_state,
> > > +                                struct v4l2_subdev_format
> > 
> > *sdformat)
> > 
> > > +{
> > > +     struct dwc_csi_device *csidev = sd_to_dwc_csi_device(sd);
> > > +     struct dwc_csi_pix_format const *csi_fmt;
> > > +     struct v4l2_mbus_framefmt *fmt;
> > > +     unsigned int align;
> > > +
> > > +     /*
> > > +      * The CSIS can't transcode in any way, the source format can't
> > > be
> > > +      * modified.
> > > +      */
> > > +     if (sdformat->pad == DWC_CSI2RX_PAD_SOURCE)
> > > +             return dwc_csi_subdev_get_fmt(sd, sd_state, sdformat);
> > > +
> > > +     if (sdformat->pad != DWC_CSI2RX_PAD_SINK)
> > > +             return -EINVAL;
> > > +
> > > +     /*
> > > +      * Validate the media bus code and clamp and align the size.
> > > +      *
> > > +      * The total number of bits per line must be a multiple of 8. We
> > 
> > thus
> > 
> > > +      * need to align the width for formats that are not multiples of
> > > 8
> > > +      * bits.
> > > +      */
> > > +     csi_fmt = find_csi_format(sdformat->format.code);
> > > +     if (!csi_fmt)
> > > +             csi_fmt = &dwc_csi_formats[0];
> > > +
> > > +     switch (csi_fmt->width % 8) {
> > > +     case 0:
> > > +             align = 0;
> > > +             break;
> > > +     case 4:
> > > +             align = 1;
> > > +             break;
> > > +     case 2:
> > > +     case 6:
> > > +             align = 2;
> > > +             break;
> > > +     default:
> > > +             /* 1, 3, 5, 7 */
> > > +             align = 3;
> > > +             break;
> > > +     }
> > 
> > 
> > Is this switch-case actually necessary? If the bits per line have to be a
> > multiple of 8, IMHO calling v4l_bound_align_image() with walign=3 should
> > be enough for all cases.
> 
> 
> Yes it's. walign=3 can cover all cases as you said but can't handle precise
> control and cause unnecessary memory waste.

Right, it's about _bits_ per line, not pixel per line. So your calculation 
seems sensible.

> [snip]

> > > +static int dwc_csi_async_register(struct dwc_csi_device *csidev)
> > > +{
> > > +     struct v4l2_fwnode_endpoint vep = {
> > > +             .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > +     };
> > > +     struct v4l2_async_subdev *asd;
> > > +     struct fwnode_handle *ep;
> > > +     unsigned int i;
> > > +     int ret;
> > > +
> > > +     v4l2_async_nf_init(&csidev->notifier);
> > > +
> > > +     ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csidev->dev), 0,
> > 
> > 0,
> > 
> > > +
> > 
> > FWNODE_GRAPH_ENDPOINT_NEXT);
> > 
> > > +     if (!ep)
> > > +             return -ENOTCONN;
> > > +
> > > +     ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > +     if (ret)
> > > +             goto err_parse;
> > > +
> > > +     for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> > > +             if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> > > +                     dev_err(csidev->dev,
> > > +                             "data lanes reordering is not
> > 
> > supported");
> > 
> > > +                     ret = -EINVAL;
> > > +                     goto err_parse;
> > > +             }
> > > +     }
> > > +
> > > +     csidev->bus = vep.bus.mipi_csi2;
> > > +
> > > +     if (fwnode_property_read_u32(ep, "fsl,hsfreqrange",
> > > +                                  &csidev->hsfreqrange))
> > > +             csidev->hsfreqrange = DPHY_DEFAULT_FREQRANGE;
> > > +
> > > +     dev_dbg(csidev->dev, "data lanes: %d\n", csidev-
> > >
> > >bus.num_data_lanes);
> > >
> > > +     dev_dbg(csidev->dev, "flags: 0x%08x\n", csidev->bus.flags);
> > > +     dev_dbg(csidev->dev, "high speed frequency range: 0x%X\n",
> > > csidev->hsfreqrange); +
> > > +     asd = v4l2_async_nf_add_fwnode_remote(&csidev->notifier, ep,
> > > +                                           struct
> > 
> > v4l2_async_subdev);
> > 
> > > +     if (IS_ERR(asd)) {
> > > +             ret = PTR_ERR(asd);
> > > +             goto err_parse;
> > > +     }
> > > +
> > > +     fwnode_handle_put(ep);
> > > +
> > > +     csidev->notifier.ops = &dwc_csi_notify_ops;
> > > +
> > > +     ret = v4l2_async_subdev_nf_register(&csidev->sd,
> > > &csidev->notifier);
> > > +     if (ret)
> > > +             return ret;
> > 
> > 
> > I'm not sure which part causes the following message:
> > 
> > > dwc-mipi-csi2 4ae00000.mipi-csi: Consider updating driver dwc-mipi-csi2
> > > to
> > 
> > match on endpoints
> > 
> > But as this is a new driver, this should be addressed.
> 
> 
> Sure. Could you help to share the steps about how to reproduce it?

Sure, I assume this depends how your OF graph looks like. This is the one I 
used, stripped some of the (irrelevant) camera properties.

&lpi2c5 {
  camera@1a {
    compatible = "sony,imx327lqr";
    reg = <0x1a>;

    port {
      sony_imx327: endpoint {
        remote-endpoint = <&mipi_to_isi>;
        data-lanes = <1 2>;
        clock-lanes = <0>;
        clock-noncontinuous = <1>;
        link-frequencies = /bits/ 64 <445500000 297000000>;
      };
    };
  };
};

/ {
  soc@0 {
    mipi_csi: mipi-csi@4ae00000 {
      compatible = "fsl,imx93-mipi-csi2";
      reg = <0x4ae00000 0x10000>;
      interrupts = <GIC_SPI 175 IRQ_TYPE_LEVEL_HIGH>;
      clocks = <&clk IMX93_CLK_MIPI_CSI_GATE>,
         <&clk IMX93_CLK_CAM_PIX>,
         <&clk IMX93_CLK_MIPI_PHY_CFG>;
      clock-names = "per", "pixel", "phy_cfg";
      power-domains = <&media_blk_ctrl IMX93_MEDIABLK_PD_MIPI_CSI>;

      ports {
        #address-cells = <1>;
        #size-cells = <0>;

        port@0 {
          reg = <0>;

          mipi_from_sensor: endpoint {
            data-lanes = <1 2>;
            fsl,hsfreqrange = <0x2c>;
          };
        };

        port@1 {
          reg = <1>;

          mipi_to_isi: endpoint {
            remote-endpoint = <&isi_in>;
          };
        };
      };
    };
  };
};

As you can see the link speed is either 445.5MHz or 297Mhz. Does this mean I 
have to set fsl,hsfreqrange to 0x16 or 0x14?

> [snip]

> > > +void dphy_rx_test_code_config(struct dwc_csi_device *csidev)
> > > +{
> > > +     u32 val;
> > > +     u8 dphy_val;
> > > +
> > > +     /* Set testclr=1'b0 */
> > > +     val = dwc_csi_read(csidev, CSI2RX_DPHY_TEST_CTRL0);
> > > +     val &= ~CSI2RX_DPHY_TEST_CTRL0_TEST_CLR;
> > > +     dwc_csi_write(csidev, CSI2RX_DPHY_TEST_CTRL0, val);
> > > +
> > > +     /* Enable hsfreqrange_ovr_en and set hsfreqrange */
> > > +     dphy_rx_write(csidev, DPHY_RX_SYS_0,
> > 
> > HSFREQRANGE_OVR_EN_RW);
> > 
> > > +     dphy_rx_write(csidev, DPHY_RX_SYS_1,
> > > +                   HSFREQRANGE_OVR_RW(csidev->hsfreqrange));
> > > +
> > > +     /* Enable timebase_ovr_en */
> > > +     dphy_val = dphy_rx_read(csidev, DPHY_RX_SYS_1);
> > > +     dphy_val |= TIMEBASE_OVR_EN_RW;
> > > +     dphy_rx_write(csidev, DPHY_RX_SYS_1, dphy_val);
> > > +
> > > +     /* Set cfgclkfreqrange */
> > > +     dphy_rx_write(csidev, DPHY_RX_SYS_2,
> > > +                   TIMEBASE_OVR_RW(csidev->cfgclkfreqrange + 0x44));
> > 
> > 
> > RM Rev 2. mentions that depending on cfgclkfreqrange another
> > configuration,
 called counter_for_des_en_config_if, also needs to be
> > set. Is this missing here?
> 
> 
> It isn't needed from my experiment result.

Okay, I'm just doing guesswork trying to figure out why I get those D-PHY 
errors.

> > 
> > 
> > > +}
> > > +
> > > +void dphy_rx_power_off(struct dwc_csi_device *csidev)
> > > +{
> > > +     dwc_csi_write(csidev, CSI2RX_DPHY_RSTZ, 0x0);
> > > +     dwc_csi_write(csidev, CSI2RX_DPHY_SHUTDOWNZ, 0x0);
> > > +}
> > > +
> > > +void dphy_rx_test_code_dump(struct dwc_csi_device *csidev)
> > > +{
> > > +#define DPHY_DEBUG_REG(name)         {name, #name}
> > > +     static const struct {
> > > +             u32 offset;
> > > +             const char * const name;
> > > +     } test_codes[] = {
> > > +             DPHY_DEBUG_REG(DPHY_RX_SYS_0),
> > > +             DPHY_DEBUG_REG(DPHY_RX_SYS_1),
> > > +             DPHY_DEBUG_REG(DPHY_RX_SYS_2),
> > > +     };
> > > +     unsigned int i;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(test_codes); i++)
> > > +             dev_dbg(csidev->dev, "%14s[0x%02x]: 0x%02x\n",
> > > +                     test_codes[i].name, test_codes[i].offset,
> > > +                     dphy_rx_read(csidev, test_codes[i].offset));
> > > +}
> > 
> > 
> > Could you also provide a complete DT configuration? I tried myself, but I
> > just ended up in getting errors while trying to use a MIPI-CSI camera
> > dwc-mipi-csi2 4ae00000.mipi-csi: IPI Interface Fatal Error events:
> > 2800064
> > dwc-mipi-csi2 4ae00000.mipi-csi: PHY Error events: 2174
> > dwc-mipi-csi2 4ae00000.mipi-csi: IPI Interface Fatal Error events:
> > 2800064
> > dwc-mipi-csi2 4ae00000.mipi-csi: PHY Error events: 2174
> 
> 
> Sure, I can provide full patches that use i.MX93 platform with AP1302 if you
> are interested.
> Will send you in another mails.

Thanks.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/