Hi Jacopo,
On Thu, 2020-03-19 at 16:20:05 -0700, Hyun Kwon wrote:
> 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!
>
And I got a chance to try.
- used the mbus config for sync between devices. Ex, vsync inversion in [1]
- made the overlap window of max9286 as a control in [2]
- some other configs using the mbus config in [3]
Let me know if this aligns with your thought.
Thanks,
-hyun
[1] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/a1d812c0452905a644d83f715c43e91ade11b266
[2] https://github.com/xlnx-hyunkwon/linux-xlnx/commit/c3d55a9e0a8d2b67f27996529582bb7cfa551b6a
[3] https://github.com/xlnx-hyunkwon/linux-xlnx/commits/hyunk/vision-wip-5.4-next
> 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
> > > > >
