Re: [PATCH v7 2/2] media: i2c: Add MAX9286 driver

[Hyun Kwon <hyun.kwon@xilinx.com>]

Hi Jacopo,

On Thu, 2020-03-19 at 01:45:57 -0700, Jacopo Mondi wrote:
> Hi Hyun,
> 
> On Wed, Mar 18, 2020 at 06:07:35PM -0700, Hyun Kwon wrote:
> > Hi Jacobo,
> >
> > On Sun, 2020-03-15 at 16:15:17 -0700, Jacopo Mondi wrote:
> > > Hello Hyun, Kieran,
> > >    it's great you are looking into this!
> > >
> > > On Fri, Feb 28, 2020 at 10:13:04AM -0800, Hyun Kwon wrote:
> > > > Hi Kieran,
> > > >
> > > > Thanks for sharing a patch.
> > > >
> > > [snip]
> > >
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Set the I2C bus speed.
> > > > > > +	 *
> > > > > > +	 * Enable I2C Local Acknowledge during the probe sequences of the camera
> > > > > > +	 * only. This should be disabled after the mux is initialised.
> > > > > > +	 */
> > > > > > +	max9286_configure_i2c(priv, true);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Reverse channel setup.
> > > > > > +	 *
> > > > > > +	 * - Enable custom reverse channel configuration (through register 0x3f)
> > > > > > +	 *   and set the first pulse length to 35 clock cycles.
> > > > > > +	 * - Increase the reverse channel amplitude to 170mV to accommodate the
> > > > > > +	 *   high threshold enabled by the serializer driver.
> > > > > > +	 */
> > > > > > +	max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> > > > > > +	max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> > > > > > +		      MAX9286_REV_AMP_X);
> > > > > > +	usleep_range(2000, 2500);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Enable GMSL links, mask unused ones and autodetect link
> > > > > > +	 * used as CSI clock source.
> > > > > > +	 */
> > > > > > +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > > > > > +	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> > > > > > +	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Video format setup:
> > > > > > +	 * Disable CSI output, VC is set according to Link number.
> > > > > > +	 */
> > > > > > +	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> > > > > > +
> > > > > > +	/* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
> > > > > > +	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> > > > > > +		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > > > > > +		      MAX9286_DATATYPE_YUV422_8BIT);
> > > > > > +
> > > > > > +	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > > > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > > > +		      MAX9286_FSYNCMETH_AUTO);
> > > > > > +
> > > > > > +	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > > > > > +	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> > > > > > +		      MAX9286_HVSRC_D14);
> > > >
> > >
> > > I agree we need to make some of these configurable, we need that too
> > > to handle both rdacm20 and 21.
> > >
> > > > Some of these configs in fact need some handshake between serializer and
> > > > de-serializer. For example, I had to invert vsync in serializer [3] to make
> > > > it work with this patch.
> > >
> > > Quickly skamming through the datasheet I'm surprised there is no way
> > > to control the vsync input polarity and you have to go through the
> > > crossbar :) Oh well, I think this could be well controlled through a
> > > device property of the serializer, what do you think?
> > >
> > > We have standard properties to control vsync and hsync polarities, but
> > > they're usually used for output signals, and I would reserve them for
> > > that future usage.. maybe a custom property to control the input vsync
> > > and hsync polarities would do...
> >
> > Thanks for sharing pointers. These are not really hardened - static properties
> > so I'm not sure if device tree is the right place. I was thinking something
> > more similar to phy_configure_opts_mipi_dphy in phy subsystem.
> 
> Let's take a step back, as it seems I was confused as well.
> 
> Not knowing the device, I can only guess on why you need to invert
> the input VSYNC signal in the cross-bar. I see two options:
> 
> 1) Looking at Figure 1 (Functional block diagram) the sensor vsync signal
> is fed to the video timing generation circtuit. The cross-bar switch
> comes after the timing generation circuit, and inverting vsync there
> is then equivalent to invert the vsync output of the timing generation
> block. If that's the case, instead of going through the crossbar,
> the same result can be obtained by setting the VSYNC_INV bit of
> register cxtp (0x4d[3]). If that's the case, I agree this setting
> should be negotiated between ser/desr, as the VS_OUT signal in Figure
> 18 is inverted in the serialized byte stream. Is this the case ?
> 
> 2) Alternatively, you need to invert the input vsync polarity to trigger
> the timing generation circuit. This mean vsync is inverted -before-
> being fed to timing generation, and this was my first understanding,
> as I assumed the crossbar switch come -before- the timing generation
> circtuitry. If this is the case, this setting should not be negotiated
> between ser/deser but between the serializer and the connected camera
> sensor.
> 
> Which case are you trying to address ?
> 

My case is simpler in fact :-), hence the executive summary is,
The sensor vsync signal is active high, and it passes through the serializer.
Since the vsync is already inverted in de-serializer by this patch, expecting
active low, I'm inverting it again in serializer to match.

	sensor -- (vsync active high) --> serializer -- (vsync active low) --> de-serializer
    
If the vsync inversion becomes a device tree property of max9286, the property
value will have to align with polarity of vsync source. In my case, I can
disable the inversion knowing the sensor vsync is active high. But in other
cases, such chain can be quite deep and may be inconvinient for users to check.

Another one is the BWS setting, which is just between ser and de-ser.

With mbus_get_config() operation, the problem can be isolated nicely in
my opinion, and the solution handles all above and scales better.
- The de-serializer checks the vsync polarity of all channels using GMSL
  config. If all are active low, invert the vsync (if it can)

	vsync_bitmap = 0;
	for(chan : channels) {
		config = get_mbus_config(chan);
		if (config->type != gmsl)
			error;

		if (config->gmsl.vsync == +)
			vsync_bitmap |= << chan->chan_id;
	}

	if (vsync_bitmap == (1 << channels.size() - 1))
		nop(); // all active high. don't invert
	else if (vsync_bitmap == 0)
		invert_vsync(ser);
	else
		error;

- The serializer checks vsync polarity in the parallel port config, and
  if it's active low (and if it can), invert to make it active high.
  Otherwise mark it in GMSL config, so the de-serializer can hanle.

	max96705_get_mbus_config()
	{
		config = get_mbus_config(sensor);
		if (config->type != parallel)
			error;

		if (config->parallel.vsync == -) {
			if (invert_vsync(ser))
				ser_config->gmsl.vsync = +;
			else
				ser_config->gmsl.vsync = -;
		}

		return ser_config;
	}

The same can be used for bandwidth setting. The max96705 driver sets 24 bit
mode only as supported bandwidth. The deserializer driver can pick it up from
mbus_get_config(), and adjust its own config accordingly. It will need some
remote node handling in each driver, but seems worth to me.

This became too lengthy! Hope it explains better and aligns with your thought,
described in other thread. I will give it a try too!

Thanks,
-hyun

> >
> > >
> > > >
> > > > In addition to that, I need a couple of additional programmings on max9286
> > > > (registers 0x0 to 0x63/0x64- disable overlap window and 0xf4 to 0x1c which
> > > > oddly change reserved bits) to get frames. The datasheet doesn't explain
> > > > enough for me to understand. I'm talking to Maxim to get more details and
> > > > see if those can be handled by serilizer driver.
> > >
> > > I would be really interested in knowing what's the overlap window control
> > > about... it's very little detailed in the manual, I agree :) 0xf4 is
> > > not even documented in my version. I assume it's something related to
> > > fsync sync locking (Fig. 46) as I failed to achieve it without that
> > > setting. How did it fail for you ?
> > >
> >
> > I received one doc "Frame Synchronization Block" that explains the overlap
> > window in more details. It's essentially a window between camera vsync and
> > frame sync. If those 2 don't happen within the window, it errors out. So it
> > gives a validation check, but may not work depending on the vsync patterm from
> > camera or the window should be bigger, which makes the validation less
> > useful in my opinion.
> 
> Thanks for the detailed info!
> 
> This reinforces the idea this setting should be disabled by default.
> If we want to enable it, a value should be provided explicitly. I
> still think DTS are the right place for this setting, as this is a
> deserializer-only configuration parameter..
> 
> >
> > I believe 0x1c has something to do with BWS as mentioned in my previous reply.
> > The max9286 on my board sets BWS pin as open, and it makes the bandwidth
> > to be 27 bit mode by default. So writing 0xf4 to 0x1c register sets to 24 bit
> > mode (while 0xf6 = 27 bit mode). But I didn't hear back from Maxim regarding
> > this yet. And unfortunately, I couldn't make it work with 27 bit mode on both
> > max9286 and max96705.
> >
> > So this and similar properties may also be something that can be handled by
> > the negotiating call mentioned above, while the configuraton through external
> > pins can be modeled as device tree properties?
> 
> Indeed external pin configuration fits well as DTS property. Would you
> like to have a go ?
> 
> Thanks
>    j
> 
> >
> > Thanks,
> > -hyun
> >
> > > On how to control overlap window a integer would do ? Setting it to 0
> > > disables it, so we could use a boolean property for convenience..
> > >
> > > Not knowing what it does I would be careful.. I think we should
> > > actualy require a mandatory property, so all current dts select their
> > > behaviour explicitly. If we later find out what it does we could
> > > define a default behaviour by defining a boolean property. New dts
> > > simpler and old dts still happy. What do you think ?
> > >
> > > >
> > > > In a longer term, it'd be nice if such alignment between serializer and
> > > > de-serializer is handled in more scalable way.
> > > >
> > >
> > > Indeed :)
> > >
> > > Thanks
> > >   j
> > >
> > > > Thanks,
> > > > -hyun
> > > >
> > > > [1] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/3bd2dded834492f4ee89e370c22877b97c2e106e
> > > > [2] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/fb0ad7fd699d90d6bbc78fc55dd98639389cfc5b
> > > > [3] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/fe0b33b174b2850bf0bb25b3a367319127ae12ee
> > > >