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/
