Hi Niklas, Thank you for the patch. On Sunday, 13 May 2018 22:19:17 EEST Niklas Söderlund wrote: > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > connected between the video sources and the video grabbers (VIN). > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > > --- > > * Changes since v14 > - Data sheet update changed init sequence for PHY forcing a restructure > of the driver. The restructure was so big I felt compel to drop all > review tags :-( > - The change was that the Renesas H3 procedure was aligned with other > SoC in the Gen3 family procedure. I had kept the rework as separate > patches and was planing to post once original driver with H3 and M3-W > support where merged. As review tags are dropped I chosen to squash > those patches into 2/2. > - Add support for Gen3 V3M. > - Add support for Gen3 M3-N. > - Set PHTC_TESTCLR when stopping the PHY. > - Revert back to the v12 and earlier phypll calculation as it turns out > it was correct after all. > > * Changes since v13 > - Change return rcar_csi2_formats + i to return &rcar_csi2_formats[i]. > - Add define for PHCLM_STOPSTATECKL. > - Update spelling in comments. > - Update calculation in rcar_csi2_calc_phypll() according to > https://linuxtv.org/downloads/v4l-dvb-apis/kapi/csi2.html. The one > before v14 did not take into account that 2 bits per sample is > transmitted. > - Use Geert's suggestion of (1 << priv->lanes) - 1 instead of switch > statement to set correct number of lanes to enable. > - Change hex constants in hsfreqrange_m3w_h3es1[] to lower case to match > style of rest of file. > - Switch to %u instead of 0x%x when printing bus type. > - Switch to %u instead of %d for priv->lanes which is unsigned. > - Add MEDIA_BUS_FMT_YUYV8_1X16 to the list of supported formats in > rcar_csi2_formats[]. > - Fixed bps for MEDIA_BUS_FMT_YUYV10_2X10 to 20 and not 16. > - Set INTSTATE after PL-11 is confirmed to match flow chart in > datasheet. > - Change priv->notifier.subdevs == NULL to !priv->notifier.subdevs. > - Add Maxime's and laurent's tags. > --- > drivers/media/platform/rcar-vin/Kconfig | 12 + > drivers/media/platform/rcar-vin/Makefile | 1 + > drivers/media/platform/rcar-vin/rcar-csi2.c | 1101 +++++++++++++++++++ > 3 files changed, 1114 insertions(+) > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c [snip] > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644 > index 0000000000000000..b19374f1516464dc > --- /dev/null > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c [snip] > +struct phtw_value { > + u16 data; > + u16 code; > +}; > + > +struct phtw_mbps { > + u16 mbps; > + u16 data; > +}; [snip] > +struct phypll_hsfreqrange { > + u16 mbps; > + u16 reg; > +}; Would it make sense to merge the phypll_hsfreqrange and phtw_mbps structures (not the data tables themselves, just the structure definitions) ? They both map a frequency to a register value. [snip] > +static int rcsi2_wait_phy_start(struct rcar_csi2 *priv) > +{ > + int timeout; > + > + /* Wait for the clock and data lanes to enter LP-11 state. */ > + for (timeout = 100; timeout > 0; timeout--) { > + const u32 lane_mask = (1 << priv->lanes) - 1; > + > + if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > + (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > + return 0; > + > + msleep(20); > + } Could you check how long this typically takes ? I would expect the lanes to all be in LP-11 already, so this should be a matter if getting the PHY to initialize properly to detect the lane state, which shouldn't take very long. > + > + dev_err(priv->dev, "Timeout waiting for LP-11 state\n"); > + > + return -ETIMEDOUT; > +} > + > +static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps) > +{ > + const struct phypll_hsfreqrange *hsfreq; > + > + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) > + if (hsfreq->mbps >= mbps) > + break; > + > + if (!hsfreq->mbps) { > + dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps); > + return -ERANGE; > + } > + > + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %u got %u Mbps\n", mbps, > + hsfreq->mbps); I think you can drop this message. > + rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg)); > + > + return 0; > +} > + > +static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp) > +{ > + struct v4l2_subdev *source; > + struct v4l2_ctrl *ctrl; > + u64 mbps; > + > + if (!priv->remote) > + return -ENODEV; > + > + source = priv->remote; > + > + /* Read the pixel rate control from remote. */ > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + dev_err(priv->dev, "no pixel rate control in subdev %s\n", > + source->name); > + return -EINVAL; > + } > + > + /* > + * Calculate the phypll in mbps (from v4l2 documentation). I'd say from the CSI-2 specification, as this isn't V4L2-specific (or I'd just drop the part in parentheses). > + * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes) > + * bps = link_freq * 2 > + */ > + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > + do_div(mbps, priv->lanes * 1000000); > + > + return mbps; > +} [snip] > +/* ------------------------------------------------------------------------ > + * PHTW unitizing sequences. Unitizing ? > + * > + * NOTE: Magic values are from the datasheet and lack documentation. > + */ > + > +static int rcsi2_phtw_write(struct rcar_csi2 *priv, u16 data, u16 code) > +{ > + unsigned int timeout; > + > + rcsi2_write(priv, PHTW_REG, > + PHTW_DWEN | PHTW_TESTDIN_DATA(data) | > + PHTW_CWEN | PHTW_TESTDIN_CODE(code)); > + > + /* Wait for DWEN and CWEN to be cleared by hardware. */ > + for (timeout = 100; timeout > 0; timeout--) { > + if (!(rcsi2_read(priv, PHTW_REG) & (PHTW_DWEN | PHTW_CWEN))) > + return 0; > + msleep(20); That's a very long sleep. I don't expect the hardware to need 20ms, I assume that if the condition is false at the first iteration you will only need to wait for a very short time. Could you experiment with smaller delays and see how long is typically needed ? > + } > + > + dev_err(priv->dev, "Timeout waiting for PHTW_DWEN and/or PHTW_CWEN\n"); > + > + return -ETIMEDOUT; > +} > + > +static int rcsi2_phtw_write_array(struct rcar_csi2 *priv, > + const struct phtw_value *values) > +{ > + const struct phtw_value *value; > + int ret; > + > + for (value = values; (value->data || value->code); value++) { No need for the inner parentheses. You could also operate on the values argument directly without using a value local variable. Up to you. > + ret = rcsi2_phtw_write(priv, value->data, value->code); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps, > + const struct phtw_mbps *values, u16 code) > +{ > + const struct phtw_mbps *value; > + > + for (value = values; value->mbps; value++) > + if (value->mbps >= mbps) > + break; > + > + if (!value->mbps) { > + dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps); > + return -ERANGE; > + } > + > + dev_dbg(priv->dev, "PHTW requested %u got %u Mbps\n", mbps, > + value->mbps); I think you can drop this debug statement. > + return rcsi2_phtw_write(priv, value->data, code); > +} > + > +static int rcsi2_init_phtw_h3_v3h_m3n(struct rcar_csi2 *priv, unsigned int > mbps) > +{ > + static const struct phtw_value step1[] = { > + { .data = 0xcc, .code = 0xe2 }, > + { .data = 0x01, .code = 0xe3 }, > + { .data = 0x11, .code = 0xe4 }, > + { .data = 0x01, .code = 0xe5 }, > + { .data = 0x10, .code = 0x04 }, > + { /* sentinel */ }, > + }; > + > + static const struct phtw_value step2[] = { > + { .data = 0x38, .code = 0x08 }, > + { .data = 0x01, .code = 0x00 }, > + { .data = 0x4b, .code = 0xac }, > + { .data = 0x03, .code = 0x00 }, > + { .data = 0x80, .code = 0x07 }, > + { /* sentinel */ }, > + }; > + > + int ret; > + > + ret = rcsi2_phtw_write_array(priv, step1); > + if (ret) > + return ret; > + > + if (mbps <= 250) { This worries me. I wonder what will happen if we use the CSI-2 receiver with a frequency below 250 MHz, and then with a frequency above. Is there a risk that the PHTW settings for the first run will be retained ? You're following the datasheet so I have no objection, but I would appreciate if you could double- check this with Renesas. Update: following our IRC conversation, the rcsi2_write(priv, PHTC_REG, PHTC_TESTCLR) call in rcsi2_stop() should reset the PHY, so there should be no issue here. A brief comment in this function to explain that could be nice. > + ret = rcsi2_phtw_write(priv, 0x39, 0x05); > + if (ret) > + return ret; > + > + ret = rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_h3_v3h_m3n, > + 0xf1); > + if (ret) > + return ret; > + } > + > + return rcsi2_phtw_write_array(priv, step2); > +} [snip] > +static struct platform_driver __refdata rcar_csi2_pdrv = { Do you need __refdata ? > + .remove = rcsi2_remove, > + .probe = rcsi2_probe, > + .driver = { > + .name = "rcar-csi2", > + .of_match_table = rcar_csi2_of_table, > + }, > +}; > + > +module_platform_driver(rcar_csi2_pdrv); > + > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver"); Nitpicking, should this be "Renesas R-Car MIPI CSI-2 receiver driver" ? > +MODULE_LICENSE("GPL"); All these are small issues, they're not blocking. Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> -- Regards, Laurent Pinchart
