Re: [PATCH 1/3] i2c: adv748x: store number of CSI-2 lanes described in device tree

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dave,

On Friday, 21 September 2018 12:33:39 EEST Dave Stevenson wrote:
> On Tue, 18 Sep 2018 at 12:13, Laurent Pinchart wrote:
> > On Tuesday, 18 September 2018 13:51:34 EEST Kieran Bingham wrote:
> >> On 18/09/18 11:46, Laurent Pinchart wrote:
> >>> On Tuesday, 18 September 2018 13:37:55 EEST Kieran Bingham wrote:
> >>>> On 18/09/18 11:28, Laurent Pinchart wrote:
> >>>>> On Tuesday, 18 September 2018 13:19:39 EEST Kieran Bingham wrote:
> >>>>>> On 18/09/18 02:45, Niklas Söderlund wrote:
> >>>>>>> The adv748x CSI-2 transmitters TXA and TXB can use different
> >>>>>>> number of lines to transmit data on. In order to be able configure
> >>>>>>> the device correctly this information need to be parsed from
> >>>>>>> device tree and stored in each TX private data structure.
> >>>>>>> 
> >>>>>>> TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.
> >>>>>> 
> >>>>>> Am I right in assuming that it is the CSI device which specifies
> >>>>>> the number of lanes in their DT?
> >>>>> 
> >>>>> Do you mean the CSI-2 receiver ? Both the receiver and the
> >>>>> transmitter should specify the data lanes in their DT node.
> >>>> 
> >>>> Yes, I should have said CSI-2 receiver.
> >>>> 
> >>>> Aha - so *both* sides of the link have to specify the lanes and
> >>>> presumably match with each other?
> >>> 
> >>> Yes, they should certainly match :-)
> >> 
> >> I assumed so :) - do we need to validate that at a framework level?
> >> (or perhaps it already is, all I've only looked at this morning is
> >> e-mails :D )
> > 
> > It's not done yet as far as I know. CC'ing Sakari who may have a comment
> > regarding whether this should be added.
> 
> (Interested party here due to CSI2 interfacing on the Pi, and I'd like
> to try and get adv748x working on the Pi once I can get some hardware)
> 
> Do they need to match? DT is supposedly describing the hardware. Where
> are you drawing the boundary between the two devices from the
> hardware/devicetree perspective?
> 
> As an example, the CSI2 receiver can support 4 lanes, whilst the
> source only needs 2, or only has 2 connected. As long as the two ends
> agree on the value in use (which will typically match the source),
> then there is no issue.
> It does require the CSI2 receiver driver to validate the remote
> endpoint settings to ensure that the source doesn't try to use more
> lanes than the receiver can cope with. That isn't a big deal as you
> already have the link to the remote end-point. Presumably you're
> already checking it to determine settings such as if the source is
> using continuous clocks or not, unless you expect that to be
> duplicated on both endpoints or your hardware doesn't care.

At least for the data-lanes property, the value is meant to describe how lanes 
are physically connected on the board from the transmitter to the receiver. It 
does not tell the maximum of lanes that the device supports, which is 
intrinsic information known to the driver already.

Regarding other parameters, such as the clock mode you mentioned, they should 
usually be queried and negotiated at runtime, especially given that both the 
transmitting and receiving devices may very well support multiple options. We 
usually don't involve DT in those cases. Parsing DT properties of a remote 
node is frowned upon, because those properties are qualified by the compatible 
string of the node they sit in. A driver matching "foo,abc" knows what 
properties are defined for the corresponding DT node, but when that driver 
crosses links to DT node "bar,xyz", it has no a priori knowledge of what to 
expect there.

Following the OF graph through endpoints and ports is still fine, as if a DT 
node uses the OF graph bindings, the nodes it is connected to are assumed to 
use the same bindings. No assumption can usually be made on other properties 
though, including the ones describing the bus configuration. For that reason 
the properties for the CSI-2 source should be parsed by the CSI-2 source 
driver, and the configuration of the source should be queried by the CSI-2 
receiver at runtime using the v4l2_subdev API (which is routinely extended 
with additional configuration information when the need arises).

> I'm genuinely interested on views here. On the Pi there will be a
> variety of CSI2 devices connected, generally configured using
> dtoverlays, and they will have varying requirements on number of
> lanes. Standard Pis only have 2 CSI-2 lanes exposed out of a possible
> 4 for the peripheral. The Compute Module is the exception where one
> CSI2 interface has all 4 lanes brought out, the other only supports 2
> lanes anyway.
> I'm expecting the CSI2 receiver endpoint data-lanes property to match
> that exposed by the Pi board, whilst the source endpoint data-lanes
> property defines what the source uses. That allows easy validation by
> the driver that the configuration can work. Otherwise an overlay would
> have to write the number of lanes used on both the CSI endpoints and
> potentially configuring it to use more lanes than can be supported.

As explained above, we currently expect the latter, with the overlay modifying 
the data-lanes property of the receiver as well. This is partly due to the 
fact that receivers can support data lanes remapping, so the data-lanes 
property of the receiver needs not only to specify the number of lanes, but 
also how they are connected.

If you want to validate the data-lanes property to ensure the overlay doesn't 
try to use more lanes than available, you can do so either against a hardcoded 
value in the receiver driver (when the maximum is fixed for a given compatible 
string), or against a value read from DT (when the maximum depends on the 
board).

> There is also the oddball one of the TC358743 which dynamically
> switches the number of lanes in use based on the data rate required.
> That's probably a separate discussion, but is currently dealt with via
> g_mbus_config as amended back in Sept 2017 [1].

This falls into the case of dynamic configuration discovery and negotiation I 
mentioned above, and we clearly need to make sure the v4l2_subdev API supports 
this use case.

> [1] Discussion https://www.spinics.net/lists/linux-media/msg122287.html
> and patch https://www.spinics.net/lists/linux-media/msg122435.html
> 
> >>>>>> Could we make this clear in the commit log (and possibly an extra
> >>>>>> comment in the code). At first I was assuming we would have to
> >>>>>> declare the number of lanes in the ADV748x TX DT node, but I don't
> >>>>>> think that's the case.
> >>>>>> 
> >>>>>>> Signed-off-by: Niklas Söderlund
> >>>>>>> <niklas.soderlund+renesas@ragnatech.se>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>  drivers/media/i2c/adv748x/adv748x-core.c | 49 +++++++++++++++++++++
> >>>>>>>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
> >>>>>>>  2 files changed, 50 insertions(+)

[snip]

-- 
Regards,

Laurent Pinchart