On 02/02/2017 03:50 AM, Philipp Zabel wrote:

+       struct v4l2_subdev     *src_sd;
+       struct v4l2_subdev     *sink_sd[CSI2_NUM_SRC_PADS];
I see no reason to store pointers to the remote v4l2_subdevs.

+       int                    input_pad;
+       struct clk             *dphy_clk;
+       struct clk             *cfg_clk;
+       struct clk             *pix_clk; /* what is this? */
+       void __iomem           *base;
+       int                     intr1;
+       int                     intr2;
The interrupts are not used, I'd remove them and the dead code in
_probe.

done.

+
+static inline u32 imxcsi2_read(struct imxcsi2_dev *csi2, unsigned int regoff)
+{
+       return readl(csi2->base + regoff);
+}
+
+static inline void imxcsi2_write(struct imxcsi2_dev *csi2, u32 val,
+                                unsigned int regoff)
+{
+       writel(val, csi2->base + regoff);
+}
Do those two wrappers really make the code more readable?

It doesn't really matter to me either way, I removed these
macros.

+
+static void imxcsi2_enable(struct imxcsi2_dev *csi2, bool enable)
+{
+       if (enable) {
+               imxcsi2_write(csi2, 0xffffffff, CSI2_PHY_SHUTDOWNZ);
+               imxcsi2_write(csi2, 0xffffffff, CSI2_DPHY_RSTZ);
+               imxcsi2_write(csi2, 0xffffffff, CSI2_RESETN);
Given that these registers only contain a single bit, and bits 31:1 are
documented as reserved, 0, I think these should write 1 instead of
0xffffffff.

Yes, these lines are lifted from the FSL BSP's mipi csi-2 driver. I
did notice this but left it in place because I was worried about
possible undocumented bits. I tried writing 1 to these registers
and the behavior is the same as before (still works).

+       } else {
+               imxcsi2_write(csi2, 0x0, CSI2_PHY_SHUTDOWNZ);
+               imxcsi2_write(csi2, 0x0, CSI2_DPHY_RSTZ);
+               imxcsi2_write(csi2, 0x0, CSI2_RESETN);
+       }
+}
+
+static void imxcsi2_reset(struct imxcsi2_dev *csi2)
+{
+       imxcsi2_enable(csi2, false);
+
+       imxcsi2_write(csi2, 0x00000001, CSI2_PHY_TST_CTRL0);
+       imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL1);
+       imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
+       imxcsi2_write(csi2, 0x00000002, CSI2_PHY_TST_CTRL0);
+       imxcsi2_write(csi2, 0x00010044, CSI2_PHY_TST_CTRL1);
+       imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
+       imxcsi2_write(csi2, 0x00000014, CSI2_PHY_TST_CTRL1);
+       imxcsi2_write(csi2, 0x00000002, CSI2_PHY_TST_CTRL0);
+       imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
These magic constants should be replaced with proper defines for the
documented bitfields, if available.

#define PHY_TESTCLR             BIT(0)
#define PHY_TESTCLK             BIT(1)

#define PHY_TESTEN              BIT(16)

        /* Clear PHY test interface */
        imxcsi2_write(csi2, PHY_TESTCLR, CSI2_PHY_TST_CTRL0);
        imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL1);
        imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL0);

        /* Raise test interface strobe signal */
        imxcsi2_write(csi2, PHY_TESTCLK, CSI2_PHY_TST_CTRL0);

        /* Configure address write on falling edge and lower strobe signal */
        u8 addr = 0x44;
        imxcsi2_write(csi2, PHY_TESTEN | addr, CSI2_PHY_TST_CTRL1);
        imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL0);

        /* Configure data write on rising edge and raise strobe signal */
        u8 data = 0x14;
        imxcsi2_write(csi2, data, CSI2_PHY_TST_CTRL1);
        imxcsi2_write(csi2, PHY_TESTCLK, CSI2_PHY_TST_CTRL0);

        /* Clear strobe signal */
        imxcsi2_write(csi2, 0, CSI2_PHY_TST_CTRL0);

The whole sequence should probably be encapsulated in a
dw_mipi_dphy_write function.

Actually, this exact function already exists as dw_mipi_dsi_phy_write in
drivers/gpu/drm/rockchip/dw-mipi-dsi.c, and it looks like the D-PHY
register 0x44 might contain a field called HSFREQRANGE_SEL.

Thanks for pointing out drivers/gpu/drm/rockchip/dw-mipi-dsi.c.
It's clear from that driver that there probably needs to be a fuller
treatment of the D-PHY programming here, but I don't know where
to find the MIPI CSI-2 D-PHY documentation for the i.MX6. The code
in imxcsi2_reset() was also pulled from FSL, and that's all I really have
to go on for the D-PHY programming. I assume the D-PHY is also a
Synopsys core, like the host controller, but the i.MX6 manual doesn't
cover it.

In any case I've created dw_mipi_csi2_phy_write(), modeled after
dw_mipi_dsi_phy_write(). The "0x14" value is a value derived for
a target max bandwidth per lane of 300 Mbps, at least that is what
drivers/gpu/drm/rockchip/dw-mipi-dsi.c suggests. I've added a FIXME
note that effect, that this value should be derived based on the D-PHY
PLL clock rate and the desired max lane bandwidth.


+       imxcsi2_enable(csi2, true);
+}
+
+static int imxcsi2_dphy_wait(struct imxcsi2_dev *csi2)
+{
+       u32 reg;
+       int i;
+
+       /* wait for mipi sensor ready */
More specifically, wait for the clock lane module to leave ULP state.

I've split this function in two:

imxcsi2_dphy_wait_lp_11()

which waits for !PHY_RXULPSCLKNOT, and stable/error-free
CSI-2 bus (CSI2_ERR1 == 0),

and

 imxcsi2_dphy_wait_clock_lane()

which waits for PHY_RXCLKACTIVEHS.

The former is called during s_power(1), the latter in s_stream(1).

+       for (i = 0; i < 50; i++) {
+               reg = imxcsi2_read(csi2, CSI2_PHY_STATE);
+               if (reg != 0x200)
Magic constants are bad. This is PHY_RXULPSCLKNOT (clock lane module in
ultra low power state).

Fixed.


+                       break;
+               usleep_range(10000, 20000);
+       }
How about breaking this out into a wait function, or even better, using
readl_poll_timeout instead of open coding these loops multiple times?

Cool, thanks for pointing out that macro. I've switched to readl_poll_timeout()
everywhere.


+
+       if (i >= 50) {
+               v4l2_err(&csi2->sd,
+                        "wait for clock lane timeout, phy_state = 0x%08x\n",
+                        reg);
+               return -ETIME;
+       }
+
+       /* wait for mipi stable */
Wait for error free transmission?

Changed to /* wait until no errors on bus */.


+
+       if (i >= 50) {
+               v4l2_err(&csi2->sd,
+                        "wait for controller timeout, err1 = 0x%08x\n",
+                        reg);
+               return -ETIME;
+       }
+
+       /* finally let's wait for active clock on the clock lane */
+       for (i = 0; i < 50; i++) {
+               reg = imxcsi2_read(csi2, CSI2_PHY_STATE);
+               if (reg & (1 << 8))
Yes, and that is PHY_RXCLKACTIVEHS.

done.


+       return 0;
+}
+
+/*
+ * V4L2 subdev operations
+ */
+
+static int imxcsi2_link_setup(struct media_entity *entity,
+                             const struct media_pad *local,
+                             const struct media_pad *remote, u32 flags)
+{
+       struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+       struct imxcsi2_dev *csi2 = sd_to_dev(sd);
+       struct v4l2_subdev *remote_sd;
+
+       dev_dbg(csi2->dev, "link setup %s -> %s", remote->entity->name,
+               local->entity->name);
+
+       remote_sd = media_entity_to_v4l2_subdev(remote->entity);
+
+       if (local->flags & MEDIA_PAD_FL_SOURCE) {
+               if (flags & MEDIA_LNK_FL_ENABLED) {
+                       if (csi2->sink_sd[local->index])
+                               return -EBUSY;
+                       csi2->sink_sd[local->index] = remote_sd;
+               } else {
+                       csi2->sink_sd[local->index] = NULL;
+               }
+       } else {
+               if (flags & MEDIA_LNK_FL_ENABLED) {
+                       if (csi2->src_sd)
+                               return -EBUSY;
+                       csi2->src_sd = remote_sd;
+               } else {
+                       csi2->src_sd = NULL;
+               }
+       }
This whole code block is just to check if there is another link already
active on the given pad. This could be stored in a boolean or a bit, no
need to store pointers to remote subdevices.

I converted these to true booleans.


+
+static int imxcsi2_get_fmt(struct v4l2_subdev *sd,
+                          struct v4l2_subdev_pad_config *cfg,
+                          struct v4l2_subdev_format *sdformat)
+{
+       struct imxcsi2_dev *csi2 = sd_to_dev(sd);
+
+       sdformat->format = csi2->format_mbus;
The output formats are different from the input formats, see the media
bus format discussion in the other thread. The input pad is the MIPI
CSI-2 bus, but the four output pads are connected to the muxes / CSIs
via a 16-bit parallel bus.

So if the input format is UYVY8_1X16, for example, the output should be
set to UYVY8_2X8.

Since the output buses from the CSI2IPU gasket are 16-bit
parallel buses, shouldn't an input format UYVY8_1X16 also be
the same at the output?

Steve

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to