[RFC 0/5] media: Implement negotiation of CSI-2 data lanes

[RFC 0/5] media: Implement negotiation of CSI-2 data lanes

[Jacopo Mondi]

Hello,
   this RFC series implements negotiation of CSI-2 data lanes number and
position by extending the v4l2_mbus_frame_desc structure with a 'phy' field
that describes the media bus configuration.

The use case this series cover is the following one:
the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
connected to its CSI-2 receivers. The ADV748x chip has recently gained support
for routing both HDMI and analogue video streams through its 4 lanes TXA
transmitter, specifically to support the Ebisu board that has a single CSI-2
receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
to different CSI-2 receivers, and where analogue video is streamed out from the
ADV748x single lane TXB transmitter.

To properly support transmission of analogue video through TXA, the number of
data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
CSI-2 minimum clock frequency requirements.

So far, the number of data lanes has always come from DT as a static parameter,
preventing its run-time modifications. This series moves the adv748x and
the R-Car CSI-2 one to use the DT property as an indication of the number of
physically available lanes instead, and to negotiate the number of lanes in
use based on the transmitter requirements, in this case the selected
analogue video routing path.

Sending as RFC as this series is based on the in-review v4l2-multiplexed support
which extends the frame descriptor with CSI-2 specific informations:
[PATCH v3 00/31] v4l: add support for multiplexed streams

In detail on the patches:
1/5 expands the frame descriptor with D-PHY (and a TODO C-PHY) configurations
2/5 is possibly for inclusion as it addresses the same issue tackled by the
    not-so-welcome "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup"
3/5 moves the adv748x to dynamically select the number of data lanes to
    use based on the selected routing
4/5 adds to the adv748x frame descriptor the D-PHY bus configuration parameters
5/5 makes the R-Car CSI-2 receiver configure itself using the bus configuration
    reported by the remote subdevice

Tested on Ebisu E3 board capturing HDMI and analogue video from TXA output,
and on Salvator-X M3-W capturing analogue video from TXA and making sure the
most canonical use case of capturing HDMI through TXA and analogue through TXB
still works. The image quality on E3 is the expected one, while on Salvator-X
the AFE->TXA routing produces images with visible artifacts and mangled colors,
but for an RFC I consider this good enough as a proof of concept.

Sending to renesas-soc and linux-media with Dave in Cc has I recall he expressed
interest for this feature during review of some adv748x patch series.

For the interested ones, the series is available at:
git://jmondi.org/linux v4l2-mux/media-master/v3-/data-lanes-negotiation

Thanks
  j

Jacopo Mondi (5):
  v4l: subdev: Add MIPI CSI-2 PHY to frame desc
  media: adv748x: Post-pone IO10 write to power up
  media: adv748x: Make lanes number depend on routing
  media: adv748x: Report D-PHY configuration
  media: rcar-csi2: Configure CSI-2 with frame desc

 drivers/media/i2c/adv748x/adv748x-core.c    | 72 ++++++++++++++-------
 drivers/media/i2c/adv748x/adv748x-csi2.c    | 21 ++++--
 drivers/media/i2c/adv748x/adv748x.h         |  3 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 71 ++++++++++++--------
 include/media/v4l2-subdev.h                 | 42 ++++++++++--
 5 files changed, 147 insertions(+), 62 deletions(-)

--
2.21.0

[RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Jacopo Mondi]

Add PHY-specific parameters to MIPI CSI-2 frame descriptor.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 6311f670de3c..eca9633c83bf 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
 	int (*s_stream)(struct v4l2_subdev *sd, int enable);
 };
 
+#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES	4
+
+/**
+ * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
+ *
+ * @clock_lane:		physical lane index of the clock lane
+ * @data_lanes:		an array of physical data lane indexes
+ * @num_data_lanes:	number of data lanes
+ */
+struct v4l2_mbus_frame_desc_entry_csi2_dphy {
+	u8 clock_lane;
+	u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
+	u8 num_data_lanes;
+};
+
+/**
+ * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
+ */
+struct v4l2_mbus_frame_desc_entry_csi2_cphy {
+	/* TODO */
+};
+
 /**
  * struct v4l2_mbus_frame_desc_entry_csi2
  *
- * @channel: CSI-2 virtual channel
- * @data_type: CSI-2 data type ID
+ * @channel:	CSI-2 virtual channel
+ * @data_type:	CSI-2 data type ID
  */
 struct v4l2_mbus_frame_desc_entry_csi2 {
 	u8 channel;
@@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
 	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
 	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
 	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
-	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
+	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
+	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
 };
 
 /**
  * struct v4l2_mbus_frame_desc - media bus data frame description
- * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
- * @entry: frame descriptors array
- * @num_entries: number of entries in @entry array
+ * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
+ * @entry:		frame descriptors array
+ * @phy:		PHY specific parameters
+ * @phy.dphy:		MIPI D-PHY specific bus configurations
+ * @phy.cphy:		MIPI C-PHY specific bus configurations
+ * @num_entries:	number of entries in @entry array
  */
 struct v4l2_mbus_frame_desc {
 	enum v4l2_mbus_frame_desc_type type;
 	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
+	union {
+		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
+		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
+	} phy;
 	unsigned short num_entries;
 };
 
-- 
2.21.0

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Sergei Shtylyov]

On 03/16/2019 06:47 PM, Jacopo Mondi wrote:

> Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6311f670de3c..eca9633c83bf 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
[...]
> @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
>  	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
>  	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
>  	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> -	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
>  };
>  
>  /**
>   * struct v4l2_mbus_frame_desc - media bus data frame description
> - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> - * @entry: frame descriptors array
> - * @num_entries: number of entries in @entry array
> + * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
> + * @entry:		frame descriptors array
> + * @phy:		PHY specific parameters
> + * @phy.dphy:		MIPI D-PHY specific bus configurations
> + * @phy.cphy:		MIPI C-PHY specific bus configurations

   The union members have csi2_ prefix in their names, no? 

> + * @num_entries:	number of entries in @entry array
>   */
>  struct v4l2_mbus_frame_desc {
>  	enum v4l2_mbus_frame_desc_type type;
>  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> +	union {
> +		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> +		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> +	} phy;
>  	unsigned short num_entries;
>  };
>  

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

Hello,

On Sat, Mar 16, 2019 at 07:20:28PM +0300, Sergei Shtylyov wrote:
> On 03/16/2019 06:47 PM, Jacopo Mondi wrote:
>
> > Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 6311f670de3c..eca9633c83bf 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> [...]
> > @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
> >  	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> >  	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> >  	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > -	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
> >  };
> >
> >  /**
> >   * struct v4l2_mbus_frame_desc - media bus data frame description
> > - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> > - * @entry: frame descriptors array
> > - * @num_entries: number of entries in @entry array
> > + * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
> > + * @entry:		frame descriptors array
> > + * @phy:		PHY specific parameters
> > + * @phy.dphy:		MIPI D-PHY specific bus configurations
> > + * @phy.cphy:		MIPI C-PHY specific bus configurations
>
>    The union members have csi2_ prefix in their names, no?
>

Correct! Thanks Sergei for noticing this!

> > + * @num_entries:	number of entries in @entry array
> >   */
> >  struct v4l2_mbus_frame_desc {
> >  	enum v4l2_mbus_frame_desc_type type;
> >  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> > +	union {
> > +		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> > +		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> > +	} phy;
> >  	unsigned short num_entries;
> >  };
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Sakari Ailus]

Hi Jacopo,

On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6311f670de3c..eca9633c83bf 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>  };
>  
> +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES	4
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> + *
> + * @clock_lane:		physical lane index of the clock lane
> + * @data_lanes:		an array of physical data lane indexes
> + * @num_data_lanes:	number of data lanes
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> +	u8 clock_lane;
> +	u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> +	u8 num_data_lanes;

Do you need more than the number of the data lanes? I'd expect few devices
to be able to do more than that. The PHY type comes already from the
firmware but I guess it's good to do the separation here as well.

Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I
think it'd be good to stick to a single definition.

> +};
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> +	/* TODO */
> +};
> +
>  /**
>   * struct v4l2_mbus_frame_desc_entry_csi2
>   *
> - * @channel: CSI-2 virtual channel
> - * @data_type: CSI-2 data type ID
> + * @channel:	CSI-2 virtual channel
> + * @data_type:	CSI-2 data type ID
>   */
>  struct v4l2_mbus_frame_desc_entry_csi2 {
>  	u8 channel;
> @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
>  	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
>  	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
>  	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> -	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
>  };
>  
>  /**
>   * struct v4l2_mbus_frame_desc - media bus data frame description
> - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> - * @entry: frame descriptors array
> - * @num_entries: number of entries in @entry array
> + * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
> + * @entry:		frame descriptors array
> + * @phy:		PHY specific parameters
> + * @phy.dphy:		MIPI D-PHY specific bus configurations
> + * @phy.cphy:		MIPI C-PHY specific bus configurations
> + * @num_entries:	number of entries in @entry array
>   */
>  struct v4l2_mbus_frame_desc {
>  	enum v4l2_mbus_frame_desc_type type;
>  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> +	union {
> +		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> +		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> +	} phy;
>  	unsigned short num_entries;
>  };
>  

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4014 bytes --]

Hi Sakari,
+Maxime because of it's D-PHY work in the phy framework.

On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> > Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 6311f670de3c..eca9633c83bf 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
> >  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >  };
> >
> > +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES	4
> > +
> > +/**
> > + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> > + *
> > + * @clock_lane:		physical lane index of the clock lane
> > + * @data_lanes:		an array of physical data lane indexes
> > + * @num_data_lanes:	number of data lanes
> > + */
> > +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> > +	u8 clock_lane;
> > +	u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> > +	u8 num_data_lanes;
>
> Do you need more than the number of the data lanes? I'd expect few devices
> to be able to do more than that. The PHY type comes already from the
> firmware but I guess it's good to do the separation here as well.

Indeed lane reordering at run time seems like a quite unusual
operation. I would say I could drop that, but then, a structure and a
new field in v4l2_mbus_frame_desc for just an u8, isn't it an
overkill (unless we know it could be expanded, with say, D-PHY timings
as in Maxime's D-PHY phy support implementation. Again, not sure they
should be run-time negotiated, but...)

>
> Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I
> think it'd be good to stick to a single definition.
>

I initially moved and renamed that define, then went back and added a
new one as I was not sure where to put this new global and D-PHY
specific define. I'll look into unifying them.

Thanks
  j


> > +};
> > +
> > +/**
> > + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> > + */
> > +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> > +	/* TODO */
> > +};
> > +
> >  /**
> >   * struct v4l2_mbus_frame_desc_entry_csi2
> >   *
> > - * @channel: CSI-2 virtual channel
> > - * @data_type: CSI-2 data type ID
> > + * @channel:	CSI-2 virtual channel
> > + * @data_type:	CSI-2 data type ID
> >   */
> >  struct v4l2_mbus_frame_desc_entry_csi2 {
> >  	u8 channel;
> > @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
> >  	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> >  	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> >  	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > -	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> > +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
> >  };
> >
> >  /**
> >   * struct v4l2_mbus_frame_desc - media bus data frame description
> > - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> > - * @entry: frame descriptors array
> > - * @num_entries: number of entries in @entry array
> > + * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
> > + * @entry:		frame descriptors array
> > + * @phy:		PHY specific parameters
> > + * @phy.dphy:		MIPI D-PHY specific bus configurations
> > + * @phy.cphy:		MIPI C-PHY specific bus configurations
> > + * @num_entries:	number of entries in @entry array
> >   */
> >  struct v4l2_mbus_frame_desc {
> >  	enum v4l2_mbus_frame_desc_type type;
> >  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> > +	union {
> > +		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> > +		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> > +	} phy;
> >  	unsigned short num_entries;
> >  };
> >
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Dave Stevenson]

Hi Jacopo.

Sorry, for some reason linux-media messages aren't coming through to
me at the moment.

I'm interested mainly for tc358743 rather than adv748x, but they want
the very similar functionality.
I'll try to create patches for that source as well.

On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Sakari,
> +Maxime because of it's D-PHY work in the phy framework.
>
> On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> > > Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> > >  1 file changed, 36 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 6311f670de3c..eca9633c83bf 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
> > >     int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > >  };
> > >
> > > +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES      4
> > > +
> > > +/**
> > > + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> > > + *
> > > + * @clock_lane:            physical lane index of the clock lane
> > > + * @data_lanes:            an array of physical data lane indexes
> > > + * @num_data_lanes:        number of data lanes
> > > + */
> > > +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> > > +   u8 clock_lane;
> > > +   u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> > > +   u8 num_data_lanes;
> >
> > Do you need more than the number of the data lanes? I'd expect few devices
> > to be able to do more than that. The PHY type comes already from the
> > firmware but I guess it's good to do the separation here as well.
>
> Indeed lane reordering at run time seems like a quite unusual
> operation. I would say I could drop that, but then, a structure and a
> new field in v4l2_mbus_frame_desc for just an u8, isn't it an
> overkill (unless we know it could be expanded, with say, D-PHY timings
> as in Maxime's D-PHY phy support implementation. Again, not sure they
> should be run-time negotiated, but...)

If we're adding extra information, then can I suggest that the
clock-noncontinuous flag is also added?
If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial
CSI2 switch), then there is nothing stopping one source wanting
continuous clocks, and one not. Encoding it in the receiver's DT node
therefore doesn't work for one of the sources. Duplication of that
definition between source and receiver has always seemed a likely
source of errors to me, but I'm the relative newcomer here.

Cheers
  Dave

[1] https://www.onsemi.com/PowerSolutions/product.do?id=FSA642
available on a board such as
http://www.ivmech.com/magaza/en/development-modules-c-4/ivport-dual-v2-raspberry-pi-camera-module-v2-multiplexer-p-109

> >
> > Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I
> > think it'd be good to stick to a single definition.
> >
>
> I initially moved and renamed that define, then went back and added a
> new one as I was not sure where to put this new global and D-PHY
> specific define. I'll look into unifying them.
>
> Thanks
>   j
>
>
> > > +};
> > > +
> > > +/**
> > > + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> > > + */
> > > +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> > > +   /* TODO */
> > > +};
> > > +
> > >  /**
> > >   * struct v4l2_mbus_frame_desc_entry_csi2
> > >   *
> > > - * @channel: CSI-2 virtual channel
> > > - * @data_type: CSI-2 data type ID
> > > + * @channel:       CSI-2 virtual channel
> > > + * @data_type:     CSI-2 data type ID
> > >   */
> > >  struct v4l2_mbus_frame_desc_entry_csi2 {
> > >     u8 channel;
> > > @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
> > >     V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> > >     V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> > >     V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > > -   V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > > +   V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> > > +   V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
> > >  };
> > >
> > >  /**
> > >   * struct v4l2_mbus_frame_desc - media bus data frame description
> > > - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> > > - * @entry: frame descriptors array
> > > - * @num_entries: number of entries in @entry array
> > > + * @type:          type of the bus (enum v4l2_mbus_frame_desc_type)
> > > + * @entry:         frame descriptors array
> > > + * @phy:           PHY specific parameters
> > > + * @phy.dphy:              MIPI D-PHY specific bus configurations
> > > + * @phy.cphy:              MIPI C-PHY specific bus configurations
> > > + * @num_entries:   number of entries in @entry array
> > >   */
> > >  struct v4l2_mbus_frame_desc {
> > >     enum v4l2_mbus_frame_desc_type type;
> > >     struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> > > +   union {
> > > +           struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> > > +           struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> > > +   } phy;
> > >     unsigned short num_entries;
> > >  };
> > >
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
> -----BEGIN PGP SIGNATURE-----
>
> iQIzBAABCAAdFiEEtcQ9SICaIIqPWDjAcjQGjxahVjwFAlyPV1EACgkQcjQGjxah
> VjxkUxAAkdhZTDN90GEkYfaiIIhthYaaz3Hd2ByI51aTqbNR4wq/6+WeqqmCUVJP
> wSB4cD3NQp6ZfJLbw97+XZ1oIZj7n4IRNDD050a3z4MFmVkJiz7dJ/yetBZhaqvA
> eutDDqk+OM6GJc0d2IUTOuiX69JSA9ToUXJrcMbkCp8TjzD5g7Kt7bwbQv4oaG44
> VBzTaMJpgGWzP1Lxv78mnWeOtH+WIuufw4vtjF5UHvRO8EC6f3kdqilem76+Ffn2
> N+n3ajhvbPyHk398wyxmcNhm29DZ6Y8CehqWw3AlzMHVbCeEEeEqsQ2MmRxrBlYx
> +U8R0Nw9JHJ1BqsrjkWWWMVCrTNzoeAGIu4Dbd/81JpxAG+FowNaVDI0Xlm0r9wG
> psLhQr6ZFaEcxE07VU7E8jfoGvjbRfmZkrtdxr34tUoBAE/YXfZ6rVXVCjTqABIf
> dcGNVMtVDV7cMvqjqiJa+9uHx467b0QZEaYn3CwW2V7qoa4D7iLf4WchsW9gvjq4
> mUEnzQFbd63qFuBaTrE4paq9hkYcoSAjrrDtL12l3FHop4wTjJLCJXLSO7khd97w
> qMQOKnWKI8edPrakz3nBx8lexgOWcoe/tMRYDF4dgYJLMm+ZNS0R90Xu9x5DMOvG
> cYBKeQk72lr0znYNTowGdc+xZcuO2BjyU83SnyZuRsmAo6kg3nA=
> =BYda
> -----END PGP SIGNATURE-----

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 7492 bytes --]

Hi Dave,
   thanks for the feedback!

On Mon, Mar 18, 2019 at 03:29:55PM +0000, Dave Stevenson wrote:
> Hi Jacopo.
>
> Sorry, for some reason linux-media messages aren't coming through to
> me at the moment.
>
> I'm interested mainly for tc358743 rather than adv748x, but they want
> the very similar functionality.
> I'll try to create patches for that source as well.
>
> On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Sakari,
> > +Maxime because of it's D-PHY work in the phy framework.
> >
> > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> > > > Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> > > >  1 file changed, 36 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index 6311f670de3c..eca9633c83bf 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
> > > >     int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > > >  };
> > > >
> > > > +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES      4
> > > > +
> > > > +/**
> > > > + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> > > > + *
> > > > + * @clock_lane:            physical lane index of the clock lane
> > > > + * @data_lanes:            an array of physical data lane indexes
> > > > + * @num_data_lanes:        number of data lanes
> > > > + */
> > > > +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> > > > +   u8 clock_lane;
> > > > +   u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> > > > +   u8 num_data_lanes;
> > >
> > > Do you need more than the number of the data lanes? I'd expect few devices
> > > to be able to do more than that. The PHY type comes already from the
> > > firmware but I guess it's good to do the separation here as well.
> >
> > Indeed lane reordering at run time seems like a quite unusual
> > operation. I would say I could drop that, but then, a structure and a
> > new field in v4l2_mbus_frame_desc for just an u8, isn't it an
> > overkill (unless we know it could be expanded, with say, D-PHY timings
> > as in Maxime's D-PHY phy support implementation. Again, not sure they
> > should be run-time negotiated, but...)
>
> If we're adding extra information, then can I suggest that the
> clock-noncontinuous flag is also added?

Great, this is a good point. I'll add this flag to the next version.

> If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial
> CSI2 switch), then there is nothing stopping one source wanting
> continuous clocks, and one not. Encoding it in the receiver's DT node
> therefore doesn't work for one of the sources. Duplication of that
> definition between source and receiver has always seemed a likely
> source of errors to me, but I'm the relative newcomer here.
>

The duplication of the bus configuration parameters seems to be a
notable source of confusion :) Apart from this, if drivers move to
support fetching some parameters from the frame descriptor, at the
same time they should make sure they retain compatibility with "legacy"
implementations, where these informations come from DT only. I don't
think that's a big deal, as one should not exclude the other, but
establishing a policy for this going forward might be beneficial. I
would say, as long as your receiver does not mandate the source to
provide bus parameters in the frame descriptor, DT has always
precedence, as this would also make sure older DT still work as
intended on your platform.

Thanks
   j

> Cheers
>   Dave
>
> [1] https://www.onsemi.com/PowerSolutions/product.do?id=FSA642
> available on a board such as
> http://www.ivmech.com/magaza/en/development-modules-c-4/ivport-dual-v2-raspberry-pi-camera-module-v2-multiplexer-p-109
>
> > >
> > > Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I
> > > think it'd be good to stick to a single definition.
> > >
> >
> > I initially moved and renamed that define, then went back and added a
> > new one as I was not sure where to put this new global and D-PHY
> > specific define. I'll look into unifying them.
> >
> > Thanks
> >   j
> >
> >
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> > > > + */
> > > > +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> > > > +   /* TODO */
> > > > +};
> > > > +
> > > >  /**
> > > >   * struct v4l2_mbus_frame_desc_entry_csi2
> > > >   *
> > > > - * @channel: CSI-2 virtual channel
> > > > - * @data_type: CSI-2 data type ID
> > > > + * @channel:       CSI-2 virtual channel
> > > > + * @data_type:     CSI-2 data type ID
> > > >   */
> > > >  struct v4l2_mbus_frame_desc_entry_csi2 {
> > > >     u8 channel;
> > > > @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
> > > >     V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> > > >     V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> > > >     V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> > > > -   V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> > > > +   V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> > > > +   V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
> > > >  };
> > > >
> > > >  /**
> > > >   * struct v4l2_mbus_frame_desc - media bus data frame description
> > > > - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> > > > - * @entry: frame descriptors array
> > > > - * @num_entries: number of entries in @entry array
> > > > + * @type:          type of the bus (enum v4l2_mbus_frame_desc_type)
> > > > + * @entry:         frame descriptors array
> > > > + * @phy:           PHY specific parameters
> > > > + * @phy.dphy:              MIPI D-PHY specific bus configurations
> > > > + * @phy.cphy:              MIPI C-PHY specific bus configurations
> > > > + * @num_entries:   number of entries in @entry array
> > > >   */
> > > >  struct v4l2_mbus_frame_desc {
> > > >     enum v4l2_mbus_frame_desc_type type;
> > > >     struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> > > > +   union {
> > > > +           struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> > > > +           struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> > > > +   } phy;
> > > >     unsigned short num_entries;
> > > >  };
> > > >
> > >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus
> > > sakari.ailus@linux.intel.com
> > -----BEGIN PGP SIGNATURE-----
> >
> > iQIzBAABCAAdFiEEtcQ9SICaIIqPWDjAcjQGjxahVjwFAlyPV1EACgkQcjQGjxah
> > VjxkUxAAkdhZTDN90GEkYfaiIIhthYaaz3Hd2ByI51aTqbNR4wq/6+WeqqmCUVJP
> > wSB4cD3NQp6ZfJLbw97+XZ1oIZj7n4IRNDD050a3z4MFmVkJiz7dJ/yetBZhaqvA
> > eutDDqk+OM6GJc0d2IUTOuiX69JSA9ToUXJrcMbkCp8TjzD5g7Kt7bwbQv4oaG44
> > VBzTaMJpgGWzP1Lxv78mnWeOtH+WIuufw4vtjF5UHvRO8EC6f3kdqilem76+Ffn2
> > N+n3ajhvbPyHk398wyxmcNhm29DZ6Y8CehqWw3AlzMHVbCeEEeEqsQ2MmRxrBlYx
> > +U8R0Nw9JHJ1BqsrjkWWWMVCrTNzoeAGIu4Dbd/81JpxAG+FowNaVDI0Xlm0r9wG
> > psLhQr6ZFaEcxE07VU7E8jfoGvjbRfmZkrtdxr34tUoBAE/YXfZ6rVXVCjTqABIf
> > dcGNVMtVDV7cMvqjqiJa+9uHx467b0QZEaYn3CwW2V7qoa4D7iLf4WchsW9gvjq4
> > mUEnzQFbd63qFuBaTrE4paq9hkYcoSAjrrDtL12l3FHop4wTjJLCJXLSO7khd97w
> > qMQOKnWKI8edPrakz3nBx8lexgOWcoe/tMRYDF4dgYJLMm+ZNS0R90Xu9x5DMOvG
> > cYBKeQk72lr0znYNTowGdc+xZcuO2BjyU83SnyZuRsmAo6kg3nA=
> > =BYda
> > -----END PGP SIGNATURE-----

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Sakari Ailus]

Hi Dave,

On Mon, Mar 18, 2019 at 03:29:55PM +0000, Dave Stevenson wrote:
> Hi Jacopo.
> 
> Sorry, for some reason linux-media messages aren't coming through to
> me at the moment.
> 
> I'm interested mainly for tc358743 rather than adv748x, but they want
> the very similar functionality.
> I'll try to create patches for that source as well.
> 
> On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Sakari,
> > +Maxime because of it's D-PHY work in the phy framework.
> >
> > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> > > > Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> > > >  1 file changed, 36 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index 6311f670de3c..eca9633c83bf 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
> > > >     int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > > >  };
> > > >
> > > > +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES      4
> > > > +
> > > > +/**
> > > > + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> > > > + *
> > > > + * @clock_lane:            physical lane index of the clock lane
> > > > + * @data_lanes:            an array of physical data lane indexes
> > > > + * @num_data_lanes:        number of data lanes
> > > > + */
> > > > +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> > > > +   u8 clock_lane;
> > > > +   u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> > > > +   u8 num_data_lanes;
> > >
> > > Do you need more than the number of the data lanes? I'd expect few devices
> > > to be able to do more than that. The PHY type comes already from the
> > > firmware but I guess it's good to do the separation here as well.
> >
> > Indeed lane reordering at run time seems like a quite unusual
> > operation. I would say I could drop that, but then, a structure and a
> > new field in v4l2_mbus_frame_desc for just an u8, isn't it an
> > overkill (unless we know it could be expanded, with say, D-PHY timings
> > as in Maxime's D-PHY phy support implementation. Again, not sure they
> > should be run-time negotiated, but...)
> 
> If we're adding extra information, then can I suggest that the
> clock-noncontinuous flag is also added?
> If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial
> CSI2 switch), then there is nothing stopping one source wanting
> continuous clocks, and one not. Encoding it in the receiver's DT node
> therefore doesn't work for one of the sources. Duplication of that
> definition between source and receiver has always seemed a likely
> source of errors to me, but I'm the relative newcomer here.

Do you have such a case somewhere?

As this is typically up to a device configuration (for those that support
it), it should be possible to use the same setting for both the sensors.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Dave Stevenson]

Hi Sakari

On Thu, 4 Apr 2019 at 09:49, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Dave,
>
> On Mon, Mar 18, 2019 at 03:29:55PM +0000, Dave Stevenson wrote:
> > Hi Jacopo.
> >
> > Sorry, for some reason linux-media messages aren't coming through to
> > me at the moment.
> >
> > I'm interested mainly for tc358743 rather than adv748x, but they want
> > the very similar functionality.
> > I'll try to create patches for that source as well.
> >
> > On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Sakari,
> > > +Maxime because of it's D-PHY work in the phy framework.
> > >
> > > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> > > > > Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> > > > >  1 file changed, 36 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > index 6311f670de3c..eca9633c83bf 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
> > > > >     int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > > > >  };
> > > > >
> > > > > +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES      4
> > > > > +
> > > > > +/**
> > > > > + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> > > > > + *
> > > > > + * @clock_lane:            physical lane index of the clock lane
> > > > > + * @data_lanes:            an array of physical data lane indexes
> > > > > + * @num_data_lanes:        number of data lanes
> > > > > + */
> > > > > +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> > > > > +   u8 clock_lane;
> > > > > +   u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> > > > > +   u8 num_data_lanes;
> > > >
> > > > Do you need more than the number of the data lanes? I'd expect few devices
> > > > to be able to do more than that. The PHY type comes already from the
> > > > firmware but I guess it's good to do the separation here as well.
> > >
> > > Indeed lane reordering at run time seems like a quite unusual
> > > operation. I would say I could drop that, but then, a structure and a
> > > new field in v4l2_mbus_frame_desc for just an u8, isn't it an
> > > overkill (unless we know it could be expanded, with say, D-PHY timings
> > > as in Maxime's D-PHY phy support implementation. Again, not sure they
> > > should be run-time negotiated, but...)
> >
> > If we're adding extra information, then can I suggest that the
> > clock-noncontinuous flag is also added?
> > If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial
> > CSI2 switch), then there is nothing stopping one source wanting
> > continuous clocks, and one not. Encoding it in the receiver's DT node
> > therefore doesn't work for one of the sources. Duplication of that
> > definition between source and receiver has always seemed a likely
> > source of errors to me, but I'm the relative newcomer here.
>
> Do you have such a case somewhere?

There are boards available for the Pi that use the FSA642 to switch
between multiple camera modules [1].
They haven't written any clever drivers as their intended use is
against the (closed) firmware, therefore they currently have to be
identical sensors on each input.
The use of those boards seems to fit pretty cleanly into this
multiplexed CSI bus framework, and there needn't be the limitation of
them being the same sensor.

I acknowledge that the Pi CSI2 receiver driver isn't upstream yet. I
put that on hold as the TC358743 was one of the main devices to be
supported, and without the rest of this patch set passing the number
of lanes in use through then that was pretty dead in the water. It was
less effort to keep the driver in the Pi vendor tree using the
g_mbus_config mechanism that was rejected, rather than holding patches
on an upstreamed driver. Once this set is merged and I can therefore
support TC358743 cleanly I'll be looking again at upstreaming it.

[1] http://www.ivmech.com/magaza/en/development-modules-c-4/ivport-dual-raspberry-pi-camera-module-multiplexer-p-104

> As this is typically up to a device configuration (for those that support
> it), it should be possible to use the same setting for both the sensors.

Are you meaning CSI transmitter or receiver for device configuration?
Most receivers support either, but often have to be told which mode to expect.
TC358743 seems to be one of the few CSI drivers that actually supports
both. None of the others appear to quote the use of
clock-noncontinuous in their DT bindings, but that doesn't guarantee
that that is the actual truth.

So there's no issue at the moment, but there will be issues when you
get the first driver that only supports clock-noncontinuous and
someone wants to mux it.

As I said, having the potential for the two ends to be set differently
in device tree and no way to check at runtime seems like an
unnecessary trap for the unwary. You're now adding a config path from
source to receiver, therefore adding that information removes that
confusion. Just a suggestion.

  Dave

Re: [RFC 1/5] v4l: subdev: Add MIPI CSI-2 PHY to frame desc

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4730 bytes --]

Hi Sakari, Laurent, all..

The more I look at this patch, the less I like it, to be honest...

On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6311f670de3c..eca9633c83bf 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>  };
>
> +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES	4
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> + *
> + * @clock_lane:		physical lane index of the clock lane
> + * @data_lanes:		an array of physical data lane indexes
> + * @num_data_lanes:	number of data lanes
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> +	u8 clock_lane;
> +	u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> +	u8 num_data_lanes;
> +};
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> +	/* TODO */
> +};
> +
>  /**
>   * struct v4l2_mbus_frame_desc_entry_csi2
>   *
> - * @channel: CSI-2 virtual channel
> - * @data_type: CSI-2 data type ID
> + * @channel:	CSI-2 virtual channel
> + * @data_type:	CSI-2 data type ID
>   */
>  struct v4l2_mbus_frame_desc_entry_csi2 {
>  	u8 channel;
> @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
>  	V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
>  	V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
>  	V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> -	V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> +	V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
>  };
>
>  /**
>   * struct v4l2_mbus_frame_desc - media bus data frame description
> - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> - * @entry: frame descriptors array
> - * @num_entries: number of entries in @entry array
> + * @type:		type of the bus (enum v4l2_mbus_frame_desc_type)
> + * @entry:		frame descriptors array
> + * @phy:		PHY specific parameters
> + * @phy.dphy:		MIPI D-PHY specific bus configurations
> + * @phy.cphy:		MIPI C-PHY specific bus configurations
> + * @num_entries:	number of entries in @entry array
>   */
>  struct v4l2_mbus_frame_desc {
>  	enum v4l2_mbus_frame_desc_type type;
>  	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> +	union {
> +		struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> +		struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> +	} phy;

I had a brief look at how v4l2_mbus_frame_desc evolved, and how Sakari
has extended it with stream and CSI-2 specific information in the
v4l-multiplexed series[1] before I've thrown the phy configuration part
in the mix with this patch, and my feeling is that we're maybe bending
the original v4l2_mbus_frame_desc purpose to our needs a bit...

What I'm questioning is if stream and media bus configuration actually
belong here. Sure we could bend this to match our purposes, but
'struct v4l2_mbus_frame_desc' is actually only used in mainline to
transport informations about binary streams, and it fits the purpose,
which is, by the way, also intended in the structure name.

What I think would be more appropriate would be a 'streaming session'
descriptor, that contains entries for up to 4 streams and one phy
configuration part. I would even say that, as this changes are intended
to make frame_desc usable to negotiate multiplexed streams parameters
that are only available on CSI-2 (VC and data type at the protocol level,
and since this patch, a few D-PHY parameters at the phy level) this
could actually be (at least initially) a new CSI-2 specific
structure with a new set of operations associated, so that we can leave
frame_desc alone to be used for binary blob description?

What do you think? Are you comfortable with this change?

Thanks
   j


[1]
https://patchwork.kernel.org/patch/10875871/
https://patchwork.kernel.org/patch/10875873/
https://patchwork.kernel.org/patch/10875879/

From the same series, here it is how frame_desc is now used:
https://patchwork.kernel.org/patch/10875911/
https://patchwork.kernel.org/patch/10875911/

And from this RFC series how it would be used to expose phy
parameters:
https://patchwork.kernel.org/patch/10855937/



>  	unsigned short num_entries;
>  };
>
> --
> 2.21.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Jacopo Mondi]

Post-pone the write of the ADV748X_IO_10 register that controls the active
routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
power-up time.

While at there, use the 'csi4_in_sel' name, which matches the register
field description in the manual, in place of 'io_10' which is the full
register name.

Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
 drivers/media/i2c/adv748x/adv748x.h      |  2 +
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 0e5a75eb6d75..02135741b1a6 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)

 int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
 {
-	int val;
+	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
+		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
+	struct adv748x_state *state = tx->state;
+	int ret;

 	if (!is_tx_enabled(tx))
 		return 0;

-	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
-	if (val < 0)
-		return val;
+	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
+	if (ret < 0)
+		return ret;

 	/*
 	 * This test against BIT(6) is not documented by the datasheet, but was
 	 * specified in the downstream driver.
 	 * Track with a WARN_ONCE to determine if it is ever set by HW.
 	 */
-	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
+	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
 			"Enabling with unknown bit set");

+	/* Configure TXA routing. */
+	if (on) {
+		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
+				state->csi4_in_sel);
+		if (ret)
+			return ret;
+	}
+
+
 	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
 }

@@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
 	struct adv748x_state *state = v4l2_get_subdevdata(sd);
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
 	bool enable = flags & MEDIA_LNK_FL_ENABLED;
-	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
-		       ADV748X_IO_10_CSI4_EN |
-		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
-	u8 io10 = 0;
+	u8 csi4_in_sel = 0;

 	/* Refuse to enable multiple links to the same TX at the same time. */
 	if (enable && tx->src)
@@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,

 	if (state->afe.tx) {
 		/* AFE Requires TXA enabled, even when output to TXB */
-		io10 |= ADV748X_IO_10_CSI4_EN;
+		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
 		if (is_txa(tx))
-			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
+			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
 		else
-			io10 |= ADV748X_IO_10_CSI1_EN;
+			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
 	}

 	if (state->hdmi.tx)
-		io10 |= ADV748X_IO_10_CSI4_EN;
+		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;

-	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
+	state->csi4_in_sel = csi4_in_sel;
+
+	return 0;
 }

 static const struct media_entity_operations adv748x_tx_media_ops = {
@@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
 static int adv748x_reset(struct adv748x_state *state)
 {
 	int ret;
-	u8 regval = 0;

 	ret = adv748x_sw_reset(state);
 	if (ret < 0)
@@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
 	if (ret)
 		return ret;

+	/* Conditionally enable TXa and TXb. */
+	if (is_tx_enabled(&state->txa))
+		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
+	if (is_tx_enabled(&state->txb))
+		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
+
 	/* Reset TXA and TXB */
 	adv748x_tx_power(&state->txa, 1);
 	adv748x_tx_power(&state->txa, 0);
@@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
 	/* Disable chip powerdown & Enable HDMI Rx block */
 	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);

-	/* Conditionally enable TXa and TXb. */
-	if (is_tx_enabled(&state->txa))
-		regval |= ADV748X_IO_10_CSI4_EN;
-	if (is_tx_enabled(&state->txb))
-		regval |= ADV748X_IO_10_CSI1_EN;
-	io_write(state, ADV748X_IO_10, regval);
-
 	/* Use vid_std and v_freq as freerun resolution for CP */
 	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
 					      ADV748X_CP_CLMP_POS_DIS_AUTO);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 4a5a6445604f..27c116d09284 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -196,6 +196,8 @@ struct adv748x_state {
 	struct adv748x_afe afe;
 	struct adv748x_csi2 txa;
 	struct adv748x_csi2 txb;
+
+	unsigned int csi4_in_sel;
 };

 #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
--
2.21.0

Re: [RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Kieran Bingham]

Hi Jacopo,

On 16/03/2019 15:47, Jacopo Mondi wrote:
> Post-pone the write of the ADV748X_IO_10 register that controls the active
> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
> power-up time.

"by caching its configuration in the driver state."

> 
> While at there, use the 'csi4_in_sel' name, which matches the register

'While at it, use the...'

Except I'm not sure csi4_in_sel is the right name for the cached values
as below...


> field description in the manual, in place of 'io_10' which is the full
> register name.
> 

This has a fixes tag, but doesn't state what the actual problem is?

Can I assume that the problem is that the configuration here is being
written to the hardware before it is powered up or such?

Or perhaps reading through the patch again, is it that the call to
link_setup can affect running streams?



> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 0e5a75eb6d75..02135741b1a6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> 
>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  {
> -	int val;
> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +	struct adv748x_state *state = tx->state;
> +	int ret;
> 
>  	if (!is_tx_enabled(tx))
>  		return 0;
> 
> -	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> -	if (val < 0)
> -		return val;
> +	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> +	if (ret < 0)
> +		return ret;
> 
>  	/*
>  	 * This test against BIT(6) is not documented by the datasheet, but was
>  	 * specified in the downstream driver.
>  	 * Track with a WARN_ONCE to determine if it is ever set by HW.
>  	 */
> -	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> +	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>  			"Enabling with unknown bit set");
> 
> +	/* Configure TXA routing. */

Should TXA routing be configured even on TXB power up? This function
handles both TX code paths. (Edit: possibly yes)

Can the logic that determines state->csi4_in_sel value simply be moved
here (or to an independent adv748x_configure_routing() function)?

I think this patch means that changes to routing will now only take
effect when starting or stopping a stream, is that right? (If so - could
that go into the commit message please?)




> +	if (on) {
> +		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
> +				state->csi4_in_sel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +
>  	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>  }
> 
> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
>  	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> -	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
> -		       ADV748X_IO_10_CSI4_EN |
> -		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> -	u8 io10 = 0;
> +	u8 csi4_in_sel = 0;
> 
>  	/* Refuse to enable multiple links to the same TX at the same time. */
>  	if (enable && tx->src)
> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,
> 
>  	if (state->afe.tx) {
>  		/* AFE Requires TXA enabled, even when output to TXB */
> -		io10 |= ADV748X_IO_10_CSI4_EN;
> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>  		if (is_txa(tx))
> -			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;

Hrm ... this is the one part that I think can't be determined without
caching some sort of value to state the routing.

>  		else
> -			io10 |= ADV748X_IO_10_CSI1_EN;
> +			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>  	}
> 
>  	if (state->hdmi.tx)
> -		io10 |= ADV748X_IO_10_CSI4_EN;
> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> 
> -	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
> +	state->csi4_in_sel = csi4_in_sel;
> +
> +	return 0;
>  }
> 
>  static const struct media_entity_operations adv748x_tx_media_ops = {
> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
>  static int adv748x_reset(struct adv748x_state *state)
>  {
>  	int ret;
> -	u8 regval = 0;
> 
>  	ret = adv748x_sw_reset(state);
>  	if (ret < 0)
> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret)
>  		return ret;
> 

Should adv748x_reset() reset the state->csi4_in_sel cached value to 0
before setting it?


> +	/* Conditionally enable TXa and TXb. */
> +	if (is_tx_enabled(&state->txa))
> +		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> +	if (is_tx_enabled(&state->txb))
> +		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;

This makes it looks like the naming "csi4_in_sel" is less appropriate,
as it covers both CSI4 and CSI1...

Also, this is confusing two terms, between the 'enable' and the 'select'

The _EN bits looks like they control the activation of the CSI
transmitter, where as the 'select' bits control the routing.

As the is_tx_enabled($TX) state is constant, perhaps that bit could be
inferred later when the register is written, and doesn't need to be
cached here?


> +
>  	/* Reset TXA and TXB */
>  	adv748x_tx_power(&state->txa, 1);
>  	adv748x_tx_power(&state->txa, 0);
> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
>  	/* Disable chip powerdown & Enable HDMI Rx block */
>  	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
> 
> -	/* Conditionally enable TXa and TXb. */
> -	if (is_tx_enabled(&state->txa))
> -		regval |= ADV748X_IO_10_CSI4_EN;
> -	if (is_tx_enabled(&state->txb))
> -		regval |= ADV748X_IO_10_CSI1_EN;
> -	io_write(state, ADV748X_IO_10, regval);
> -
>  	/* Use vid_std and v_freq as freerun resolution for CP */
>  	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
>  					      ADV748X_CP_CLMP_POS_DIS_AUTO);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 4a5a6445604f..27c116d09284 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -196,6 +196,8 @@ struct adv748x_state {
>  	struct adv748x_afe afe;
>  	struct adv748x_csi2 txa;
>  	struct adv748x_csi2 txb;
> +
> +	unsigned int csi4_in_sel;
>  };
> 
>  #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
> --
> 2.21.0
> 

-- 
Regards
--
Kieran

Re: [RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 8964 bytes --]

Hi Kieran,

On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 16/03/2019 15:47, Jacopo Mondi wrote:
> > Post-pone the write of the ADV748X_IO_10 register that controls the active
> > routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
> > power-up time.
>
> "by caching its configuration in the driver state."
>
> >
> > While at there, use the 'csi4_in_sel' name, which matches the register
>
> 'While at it, use the...'
>
> Except I'm not sure csi4_in_sel is the right name for the cached values
> as below...
>
>
> > field description in the manual, in place of 'io_10' which is the full
> > register name.
> >
>
> This has a fixes tag, but doesn't state what the actual problem is?
>

As reported in the cover letter, please see:
"[PATCH] media: adv748x: Don't disable CSI-2 on link_setup"

> Can I assume that the problem is that the configuration here is being
> written to the hardware before it is powered up or such?
>
> Or perhaps reading through the patch again, is it that the call to
> link_setup can affect running streams?

The issue I was trying to solve, even with the first patch is that
going through a link disable (eg. at media graph reset time) wrote to
the csi4_in_sel register a 0 value, when both TXA and TXB routing
where disabled and this causes issues on some HDMI transmiters, for
reason Ian and Hans tried to expand but about which I'm not yet sure
about.

The idea here is to cache the routing settings at link_setup time
(upon activation or deactivation of a link) and apply them to hardware
at tx power up time, which happens at s_stream(1).

In this way, if streaming is started, we know at least one link is
enabled and we can write to csi4_in_sel.

>
>
>
> > Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 0e5a75eb6d75..02135741b1a6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> >
> >  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  {
> > -	int val;
> > +	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
> > +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +	struct adv748x_state *state = tx->state;
> > +	int ret;
> >
> >  	if (!is_tx_enabled(tx))
> >  		return 0;
> >
> > -	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> > -	if (val < 0)
> > -		return val;
> > +	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> > +	if (ret < 0)
> > +		return ret;
> >
> >  	/*
> >  	 * This test against BIT(6) is not documented by the datasheet, but was
> >  	 * specified in the downstream driver.
> >  	 * Track with a WARN_ONCE to determine if it is ever set by HW.
> >  	 */
> > -	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> > +	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> >  			"Enabling with unknown bit set");
> >
> > +	/* Configure TXA routing. */
>
> Should TXA routing be configured even on TXB power up? This function
> handles both TX code paths. (Edit: possibly yes)
>

The comment is wrong, thanks for noticing.

> Can the logic that determines state->csi4_in_sel value simply be moved
> here (or to an independent adv748x_configure_routing() function)?
>

It has to be changed at link_setup time in rensponse to a media link
activation or deactivation.

> I think this patch means that changes to routing will now only take
> effect when starting or stopping a stream, is that right? (If so - could
> that go into the commit message please?)
>

Well...

 Post-pone the write of the ADV748X_IO_10 register that controls the active
 routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
 power-up time.

Doesn't it say that? Or what confused you is that s_stream->s_power(1)
and I should mention streaming instead of power up?

>
>
>
> > +	if (on) {
> > +		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
> > +				state->csi4_in_sel);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +
> >  	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
> >  }
> >
> > @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
> >  	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> > -	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
> > -		       ADV748X_IO_10_CSI4_EN |
> > -		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > -	u8 io10 = 0;
> > +	u8 csi4_in_sel = 0;
> >
> >  	/* Refuse to enable multiple links to the same TX at the same time. */
> >  	if (enable && tx->src)
> > @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,
> >
> >  	if (state->afe.tx) {
> >  		/* AFE Requires TXA enabled, even when output to TXB */
> > -		io10 |= ADV748X_IO_10_CSI4_EN;
> > +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> >  		if (is_txa(tx))
> > -			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> Hrm ... this is the one part that I think can't be determined without
> caching some sort of value to state the routing.
>

Yes

> >  		else
> > -			io10 |= ADV748X_IO_10_CSI1_EN;
> > +			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
> >  	}
> >
> >  	if (state->hdmi.tx)
> > -		io10 |= ADV748X_IO_10_CSI4_EN;
> > +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> >
> > -	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
> > +	state->csi4_in_sel = csi4_in_sel;
> > +
> > +	return 0;
> >  }
> >
> >  static const struct media_entity_operations adv748x_tx_media_ops = {
> > @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
> >  static int adv748x_reset(struct adv748x_state *state)
> >  {
> >  	int ret;
> > -	u8 regval = 0;
> >
> >  	ret = adv748x_sw_reset(state);
> >  	if (ret < 0)
> > @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
> >  	if (ret)
> >  		return ret;
> >
>
> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0
> before setting it?

I should better check when reset happens, and if it happens only when
links have been disabled or not.
If links are disabled, the variable gets zeroed by the link_setup
callback. If reset happens with links active, we should not zero it
otherwise we lose the configuration

>
>
> > +	/* Conditionally enable TXa and TXb. */
> > +	if (is_tx_enabled(&state->txa))
> > +		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> > +	if (is_tx_enabled(&state->txb))
> > +		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>
> This makes it looks like the naming "csi4_in_sel" is less appropriate,
> as it covers both CSI4 and CSI1...
>

Blame the hw designers :)

> Also, this is confusing two terms, between the 'enable' and the 'select'
>
> The _EN bits looks like they control the activation of the CSI
> transmitter, where as the 'select' bits control the routing.
>

You are righ. csi4_in_sel should only control the 3 bits used for
routing, while enabling and disabling of the TXes is controlled by
other bits of the io_10 register.
I'll look into changing the name back

> As the is_tx_enabled($TX) state is constant, perhaps that bit could be
> inferred later when the register is written, and doesn't need to be
> cached here?

I'll consider that, thanks

Thanks
  j

>
>
> > +
> >  	/* Reset TXA and TXB */
> >  	adv748x_tx_power(&state->txa, 1);
> >  	adv748x_tx_power(&state->txa, 0);
> > @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
> >  	/* Disable chip powerdown & Enable HDMI Rx block */
> >  	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
> >
> > -	/* Conditionally enable TXa and TXb. */
> > -	if (is_tx_enabled(&state->txa))
> > -		regval |= ADV748X_IO_10_CSI4_EN;
> > -	if (is_tx_enabled(&state->txb))
> > -		regval |= ADV748X_IO_10_CSI1_EN;
> > -	io_write(state, ADV748X_IO_10, regval);
> > -
> >  	/* Use vid_std and v_freq as freerun resolution for CP */
> >  	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
> >  					      ADV748X_CP_CLMP_POS_DIS_AUTO);
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 4a5a6445604f..27c116d09284 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -196,6 +196,8 @@ struct adv748x_state {
> >  	struct adv748x_afe afe;
> >  	struct adv748x_csi2 txa;
> >  	struct adv748x_csi2 txb;
> > +
> > +	unsigned int csi4_in_sel;
> >  };
> >
> >  #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
> > --
> > 2.21.0
> >
>
> --
> Regards
> --
> Kieran

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Kieran Bingham]

Hi Jacopo,

On 12/04/2019 15:45, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 16/03/2019 15:47, Jacopo Mondi wrote:
>>> Post-pone the write of the ADV748X_IO_10 register that controls the active
>>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
>>> power-up time.
>>
>> "by caching its configuration in the driver state."
>>
>>>
>>> While at there, use the 'csi4_in_sel' name, which matches the register
>>
>> 'While at it, use the...'
>>
>> Except I'm not sure csi4_in_sel is the right name for the cached values
>> as below...
>>
>>
>>> field description in the manual, in place of 'io_10' which is the full
>>> register name.
>>>
>>
>> This has a fixes tag, but doesn't state what the actual problem is?
>>
> 
> As reported in the cover letter, please see:
> "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup"

Ah I see, I had missed that.

The patch itself should still describe the problem if it's fixing
something. The cover letter will not be available in the git-log.

Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable
CSI-2 on link_setup" ?

>> Can I assume that the problem is that the configuration here is being
>> written to the hardware before it is powered up or such?
>>
>> Or perhaps reading through the patch again, is it that the call to
>> link_setup can affect running streams?
> 
> The issue I was trying to solve, even with the first patch is that
> going through a link disable (eg. at media graph reset time) wrote to
> the csi4_in_sel register a 0 value, when both TXA and TXB routing
> where disabled and this causes issues on some HDMI transmiters, for
> reason Ian and Hans tried to expand but about which I'm not yet sure
> about.

Ok, I found that patch and read their comments. So disabling the CSI2
might trigger a hot-plug event to the transmitter which makes them think
they have been (physically) disconnected in some way, or triggers them
to re-read the EDID which will fail. But we don't really know what the
fault is yet.


> The idea here is to cache the routing settings at link_setup time
> (upon activation or deactivation of a link) and apply them to hardware
> at tx power up time, which happens at s_stream(1).
> 
> In this way, if streaming is started, we know at least one link is
> enabled and we can write to csi4_in_sel.

Overall this seems reasonable, only making a change to io10 when either
of the stream states are changed.


>>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
>>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>>>  2 files changed, 33 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>> index 0e5a75eb6d75..02135741b1a6 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
>>>
>>>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>>>  {
>>> -	int val;
>>> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
>>> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +	struct adv748x_state *state = tx->state;
>>> +	int ret;
>>>
>>>  	if (!is_tx_enabled(tx))
>>>  		return 0;
>>>
>>> -	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
>>> -	if (val < 0)
>>> -		return val;
>>> +	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
>>> +	if (ret < 0)
>>> +		return ret;
>>>
>>>  	/*
>>>  	 * This test against BIT(6) is not documented by the datasheet, but was
>>>  	 * specified in the downstream driver.
>>>  	 * Track with a WARN_ONCE to determine if it is ever set by HW.
>>>  	 */
>>> -	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>>> +	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>>>  			"Enabling with unknown bit set");
>>>
>>> +	/* Configure TXA routing. */
>>
>> Should TXA routing be configured even on TXB power up? This function
>> handles both TX code paths. (Edit: possibly yes)
>>
> 
> The comment is wrong, thanks for noticing.
> 
>> Can the logic that determines state->csi4_in_sel value simply be moved
>> here (or to an independent adv748x_configure_routing() function)?
>>
> 
> It has to be changed at link_setup time in rensponse to a media link
> activation or deactivation.
> 
>> I think this patch means that changes to routing will now only take
>> effect when starting or stopping a stream, is that right? (If so - could
>> that go into the commit message please?)
>>
> 
> Well...
> 
>  Post-pone the write of the ADV748X_IO_10 register that controls the active
>  routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
>  power-up time.
> 
> Doesn't it say that? Or what confused you is that s_stream->s_power(1)
> and I should mention streaming instead of power up?

I think of "Power up" meaning at device probe time (or possibly some
runtime-pm event time). But yes, I think the distinction that it now
happens at stream_on/stream_off is important.



> 
>>
>>
>>
>>> +	if (on) {
>>> +		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
>>> +				state->csi4_in_sel);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +
>>>  	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>>>  }
>>>
>>> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
>>>  	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>>>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>>  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
>>> -	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
>>> -		       ADV748X_IO_10_CSI4_EN |
>>> -		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> -	u8 io10 = 0;
>>> +	u8 csi4_in_sel = 0;
>>>
>>>  	/* Refuse to enable multiple links to the same TX at the same time. */
>>>  	if (enable && tx->src)
>>> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,
>>>
>>>  	if (state->afe.tx) {
>>>  		/* AFE Requires TXA enabled, even when output to TXB */
>>> -		io10 |= ADV748X_IO_10_CSI4_EN;
>>> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>>>  		if (is_txa(tx))
>>> -			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>
>> Hrm ... this is the one part that I think can't be determined without
>> caching some sort of value to state the routing.
>>
> 
> Yes

But the actual hardware shouldn't be updated if the stream is running
right? (I wonder if the media-controller would prevent that anyway).



> 
>>>  		else
>>> -			io10 |= ADV748X_IO_10_CSI1_EN;
>>> +			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>>>  	}
>>>
>>>  	if (state->hdmi.tx)
>>> -		io10 |= ADV748X_IO_10_CSI4_EN;
>>> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>>>
>>> -	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
>>> +	state->csi4_in_sel = csi4_in_sel;
>>> +
>>> +	return 0;
>>>  }
>>>
>>>  static const struct media_entity_operations adv748x_tx_media_ops = {
>>> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
>>>  static int adv748x_reset(struct adv748x_state *state)
>>>  {
>>>  	int ret;
>>> -	u8 regval = 0;
>>>
>>>  	ret = adv748x_sw_reset(state);
>>>  	if (ret < 0)
>>> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
>>>  	if (ret)
>>>  		return ret;
>>>
>>
>> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0
>> before setting it?
> 
> I should better check when reset happens, and if it happens only when
> links have been disabled or not.
> If links are disabled, the variable gets zeroed by the link_setup
> callback. If reset happens with links active, we should not zero it
> otherwise we lose the configuration


Ah yes, I missed that the local variable is initialised to zero, and
this state variable is set to the result at the end of the call...

It does mean that the function ordering will be important here.

It becomes irrelevant if these conditionals move to the same point
anyway though.



> 
>>
>>
>>> +	/* Conditionally enable TXa and TXb. */
>>> +	if (is_tx_enabled(&state->txa))
>>> +		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>>> +	if (is_tx_enabled(&state->txb))
>>> +		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>>
>> This makes it looks like the naming "csi4_in_sel" is less appropriate,
>> as it covers both CSI4 and CSI1...
>>
> 
> Blame the hw designers :)

Always. ... of course they keep blaming us back :D

> 
>> Also, this is confusing two terms, between the 'enable' and the 'select'
>>
>> The _EN bits looks like they control the activation of the CSI
>> transmitter, where as the 'select' bits control the routing.
>>
> 
> You are righ. csi4_in_sel should only control the 3 bits used for
> routing, while enabling and disabling of the TXes is controlled by
> other bits of the io_10 register.
> I'll look into changing the name back
> 
>> As the is_tx_enabled($TX) state is constant, perhaps that bit could be
>> inferred later when the register is written, and doesn't need to be
>> cached here?
> 
> I'll consider that, thanks

I think it's only the routing choice that needs to be stored in the
state. That would minimise being stored 'globally', and the values could
be determined at the time of programming the register.

> 
> Thanks
>   j
> 
>>
>>
>>> +
>>>  	/* Reset TXA and TXB */
>>>  	adv748x_tx_power(&state->txa, 1);
>>>  	adv748x_tx_power(&state->txa, 0);
>>> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
>>>  	/* Disable chip powerdown & Enable HDMI Rx block */
>>>  	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
>>>
>>> -	/* Conditionally enable TXa and TXb. */
>>> -	if (is_tx_enabled(&state->txa))
>>> -		regval |= ADV748X_IO_10_CSI4_EN;
>>> -	if (is_tx_enabled(&state->txb))
>>> -		regval |= ADV748X_IO_10_CSI1_EN;
>>> -	io_write(state, ADV748X_IO_10, regval);
>>> -
>>>  	/* Use vid_std and v_freq as freerun resolution for CP */
>>>  	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
>>>  					      ADV748X_CP_CLMP_POS_DIS_AUTO);
>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>>> index 4a5a6445604f..27c116d09284 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>> @@ -196,6 +196,8 @@ struct adv748x_state {
>>>  	struct adv748x_afe afe;
>>>  	struct adv748x_csi2 txa;
>>>  	struct adv748x_csi2 txb;
>>> +
>>> +	unsigned int csi4_in_sel;
>>>  };
>>>
>>>  #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
>>> --
>>> 2.21.0
>>>

-- 
Regards
--
Kieran

Re: [RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 11739 bytes --]

Hi Kieran,

On Fri, Apr 12, 2019 at 04:57:45PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 12/04/2019 15:45, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote:
> >> Hi Jacopo,
> >>
> >> On 16/03/2019 15:47, Jacopo Mondi wrote:
> >>> Post-pone the write of the ADV748X_IO_10 register that controls the active
> >>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
> >>> power-up time.
> >>
> >> "by caching its configuration in the driver state."
> >>
> >>>
> >>> While at there, use the 'csi4_in_sel' name, which matches the register
> >>
> >> 'While at it, use the...'
> >>
> >> Except I'm not sure csi4_in_sel is the right name for the cached values
> >> as below...
> >>
> >>
> >>> field description in the manual, in place of 'io_10' which is the full
> >>> register name.
> >>>
> >>
> >> This has a fixes tag, but doesn't state what the actual problem is?
> >>
> >
> > As reported in the cover letter, please see:
> > "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup"
>
> Ah I see, I had missed that.
>
> The patch itself should still describe the problem if it's fixing
> something. The cover letter will not be available in the git-log.
>

You're right, I'll expand the commit message.

> Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable
> CSI-2 on link_setup" ?
>

Yes

> >> Can I assume that the problem is that the configuration here is being
> >> written to the hardware before it is powered up or such?
> >>
> >> Or perhaps reading through the patch again, is it that the call to
> >> link_setup can affect running streams?
> >
> > The issue I was trying to solve, even with the first patch is that
> > going through a link disable (eg. at media graph reset time) wrote to
> > the csi4_in_sel register a 0 value, when both TXA and TXB routing
> > where disabled and this causes issues on some HDMI transmiters, for
> > reason Ian and Hans tried to expand but about which I'm not yet sure
> > about.
>
> Ok, I found that patch and read their comments. So disabling the CSI2
> might trigger a hot-plug event to the transmitter which makes them think
> they have been (physically) disconnected in some way, or triggers them
> to re-read the EDID which will fail. But we don't really know what the
> fault is yet.
>

Correct. Please note that I've seen this happening with one HDMI
transmitter (a chromecast device), while if HDMI source is a laptop
everything's fine...

>
> > The idea here is to cache the routing settings at link_setup time
> > (upon activation or deactivation of a link) and apply them to hardware
> > at tx power up time, which happens at s_stream(1).
> >
> > In this way, if streaming is started, we know at least one link is
> > enabled and we can write to csi4_in_sel.
>
> Overall this seems reasonable, only making a change to io10 when either
> of the stream states are changed.
>

Thanks, I'll re-send (maybe even out of this series if it gets stale).
Could I have your tag on the next iteration?

Thanks
  j

>
> >>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
> >>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >>>  2 files changed, 33 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> index 0e5a75eb6d75..02135741b1a6 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> >>>
> >>>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >>>  {
> >>> -	int val;
> >>> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
> >>> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> +	struct adv748x_state *state = tx->state;
> >>> +	int ret;
> >>>
> >>>  	if (!is_tx_enabled(tx))
> >>>  		return 0;
> >>>
> >>> -	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> >>> -	if (val < 0)
> >>> -		return val;
> >>> +	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>>
> >>>  	/*
> >>>  	 * This test against BIT(6) is not documented by the datasheet, but was
> >>>  	 * specified in the downstream driver.
> >>>  	 * Track with a WARN_ONCE to determine if it is ever set by HW.
> >>>  	 */
> >>> -	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> >>> +	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> >>>  			"Enabling with unknown bit set");
> >>>
> >>> +	/* Configure TXA routing. */
> >>
> >> Should TXA routing be configured even on TXB power up? This function
> >> handles both TX code paths. (Edit: possibly yes)
> >>
> >
> > The comment is wrong, thanks for noticing.
> >
> >> Can the logic that determines state->csi4_in_sel value simply be moved
> >> here (or to an independent adv748x_configure_routing() function)?
> >>
> >
> > It has to be changed at link_setup time in rensponse to a media link
> > activation or deactivation.
> >
> >> I think this patch means that changes to routing will now only take
> >> effect when starting or stopping a stream, is that right? (If so - could
> >> that go into the commit message please?)
> >>
> >
> > Well...
> >
> >  Post-pone the write of the ADV748X_IO_10 register that controls the active
> >  routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
> >  power-up time.
> >
> > Doesn't it say that? Or what confused you is that s_stream->s_power(1)
> > and I should mention streaming instead of power up?
>
> I think of "Power up" meaning at device probe time (or possibly some
> runtime-pm event time). But yes, I think the distinction that it now
> happens at stream_on/stream_off is important.
>
>
>
> >
> >>
> >>
> >>
> >>> +	if (on) {
> >>> +		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
> >>> +				state->csi4_in_sel);
> >>> +		if (ret)
> >>> +			return ret;
> >>> +	}
> >>> +
> >>> +
> >>>  	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
> >>>  }
> >>>
> >>> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
> >>>  	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> >>>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >>>  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> >>> -	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
> >>> -		       ADV748X_IO_10_CSI4_EN |
> >>> -		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> -	u8 io10 = 0;
> >>> +	u8 csi4_in_sel = 0;
> >>>
> >>>  	/* Refuse to enable multiple links to the same TX at the same time. */
> >>>  	if (enable && tx->src)
> >>> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,
> >>>
> >>>  	if (state->afe.tx) {
> >>>  		/* AFE Requires TXA enabled, even when output to TXB */
> >>> -		io10 |= ADV748X_IO_10_CSI4_EN;
> >>> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> >>>  		if (is_txa(tx))
> >>> -			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> +			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>
> >> Hrm ... this is the one part that I think can't be determined without
> >> caching some sort of value to state the routing.
> >>
> >
> > Yes
>
> But the actual hardware shouldn't be updated if the stream is running
> right? (I wonder if the media-controller would prevent that anyway).
>
>
>
> >
> >>>  		else
> >>> -			io10 |= ADV748X_IO_10_CSI1_EN;
> >>> +			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
> >>>  	}
> >>>
> >>>  	if (state->hdmi.tx)
> >>> -		io10 |= ADV748X_IO_10_CSI4_EN;
> >>> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> >>>
> >>> -	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
> >>> +	state->csi4_in_sel = csi4_in_sel;
> >>> +
> >>> +	return 0;
> >>>  }
> >>>
> >>>  static const struct media_entity_operations adv748x_tx_media_ops = {
> >>> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
> >>>  static int adv748x_reset(struct adv748x_state *state)
> >>>  {
> >>>  	int ret;
> >>> -	u8 regval = 0;
> >>>
> >>>  	ret = adv748x_sw_reset(state);
> >>>  	if (ret < 0)
> >>> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
> >>>  	if (ret)
> >>>  		return ret;
> >>>
> >>
> >> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0
> >> before setting it?
> >
> > I should better check when reset happens, and if it happens only when
> > links have been disabled or not.
> > If links are disabled, the variable gets zeroed by the link_setup
> > callback. If reset happens with links active, we should not zero it
> > otherwise we lose the configuration
>
>
> Ah yes, I missed that the local variable is initialised to zero, and
> this state variable is set to the result at the end of the call...
>
> It does mean that the function ordering will be important here.
>
> It becomes irrelevant if these conditionals move to the same point
> anyway though.
>
>
>
> >
> >>
> >>
> >>> +	/* Conditionally enable TXa and TXb. */
> >>> +	if (is_tx_enabled(&state->txa))
> >>> +		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> >>> +	if (is_tx_enabled(&state->txb))
> >>> +		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
> >>
> >> This makes it looks like the naming "csi4_in_sel" is less appropriate,
> >> as it covers both CSI4 and CSI1...
> >>
> >
> > Blame the hw designers :)
>
> Always. ... of course they keep blaming us back :D
>
> >
> >> Also, this is confusing two terms, between the 'enable' and the 'select'
> >>
> >> The _EN bits looks like they control the activation of the CSI
> >> transmitter, where as the 'select' bits control the routing.
> >>
> >
> > You are righ. csi4_in_sel should only control the 3 bits used for
> > routing, while enabling and disabling of the TXes is controlled by
> > other bits of the io_10 register.
> > I'll look into changing the name back
> >
> >> As the is_tx_enabled($TX) state is constant, perhaps that bit could be
> >> inferred later when the register is written, and doesn't need to be
> >> cached here?
> >
> > I'll consider that, thanks
>
> I think it's only the routing choice that needs to be stored in the
> state. That would minimise being stored 'globally', and the values could
> be determined at the time of programming the register.
>
> >
> > Thanks
> >   j
> >
> >>
> >>
> >>> +
> >>>  	/* Reset TXA and TXB */
> >>>  	adv748x_tx_power(&state->txa, 1);
> >>>  	adv748x_tx_power(&state->txa, 0);
> >>> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
> >>>  	/* Disable chip powerdown & Enable HDMI Rx block */
> >>>  	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
> >>>
> >>> -	/* Conditionally enable TXa and TXb. */
> >>> -	if (is_tx_enabled(&state->txa))
> >>> -		regval |= ADV748X_IO_10_CSI4_EN;
> >>> -	if (is_tx_enabled(&state->txb))
> >>> -		regval |= ADV748X_IO_10_CSI1_EN;
> >>> -	io_write(state, ADV748X_IO_10, regval);
> >>> -
> >>>  	/* Use vid_std and v_freq as freerun resolution for CP */
> >>>  	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
> >>>  					      ADV748X_CP_CLMP_POS_DIS_AUTO);
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> >>> index 4a5a6445604f..27c116d09284 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x.h
> >>> +++ b/drivers/media/i2c/adv748x/adv748x.h
> >>> @@ -196,6 +196,8 @@ struct adv748x_state {
> >>>  	struct adv748x_afe afe;
> >>>  	struct adv748x_csi2 txa;
> >>>  	struct adv748x_csi2 txb;
> >>> +
> >>> +	unsigned int csi4_in_sel;
> >>>  };
> >>>
> >>>  #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
> >>> --
> >>> 2.21.0
> >>>
>
> --
> Regards
> --
> Kieran

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Kieran Bingham]

Hi Jacopo,

Sorry for dragging up an old thread...

On 15/04/2019 08:00, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Apr 12, 2019 at 04:57:45PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 12/04/2019 15:45, Jacopo Mondi wrote:
>>> Hi Kieran,
>>>
>>> On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote:
>>>> Hi Jacopo,
>>>>
>>>> On 16/03/2019 15:47, Jacopo Mondi wrote:
>>>>> Post-pone the write of the ADV748X_IO_10 register that controls the active
>>>>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
>>>>> power-up time.
>>>>
>>>> "by caching its configuration in the driver state."
>>>>
>>>>>
>>>>> While at there, use the 'csi4_in_sel' name, which matches the register
>>>>
>>>> 'While at it, use the...'
>>>>
>>>> Except I'm not sure csi4_in_sel is the right name for the cached values
>>>> as below...
>>>>
>>>>
>>>>> field description in the manual, in place of 'io_10' which is the full
>>>>> register name.
>>>>>
>>>>
>>>> This has a fixes tag, but doesn't state what the actual problem is?
>>>>
>>>
>>> As reported in the cover letter, please see:
>>> "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup"
>>
>> Ah I see, I had missed that.
>>
>> The patch itself should still describe the problem if it's fixing
>> something. The cover letter will not be available in the git-log.
>>
> 
> You're right, I'll expand the commit message.
> 
>> Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable
>> CSI-2 on link_setup" ?
>>
> 
> Yes
> 
>>>> Can I assume that the problem is that the configuration here is being
>>>> written to the hardware before it is powered up or such?
>>>>
>>>> Or perhaps reading through the patch again, is it that the call to
>>>> link_setup can affect running streams?
>>>
>>> The issue I was trying to solve, even with the first patch is that
>>> going through a link disable (eg. at media graph reset time) wrote to
>>> the csi4_in_sel register a 0 value, when both TXA and TXB routing
>>> where disabled and this causes issues on some HDMI transmiters, for
>>> reason Ian and Hans tried to expand but about which I'm not yet sure
>>> about.
>>
>> Ok, I found that patch and read their comments. So disabling the CSI2
>> might trigger a hot-plug event to the transmitter which makes them think
>> they have been (physically) disconnected in some way, or triggers them
>> to re-read the EDID which will fail. But we don't really know what the
>> fault is yet.
>>
> 
> Correct. Please note that I've seen this happening with one HDMI
> transmitter (a chromecast device), while if HDMI source is a laptop
> everything's fine...
> 
>>
>>> The idea here is to cache the routing settings at link_setup time
>>> (upon activation or deactivation of a link) and apply them to hardware
>>> at tx power up time, which happens at s_stream(1).
>>>
>>> In this way, if streaming is started, we know at least one link is
>>> enabled and we can write to csi4_in_sel.
>>
>> Overall this seems reasonable, only making a change to io10 when either
>> of the stream states are changed.
>>
> 
> Thanks, I'll re-send (maybe even out of this series if it gets stale).
> Could I have your tag on the next iteration?

I found this patch on a branch while rebasing old patches to latest.

It still applies, and seems to make sense - and it looks like I agreed
with you back when you posted it.

It seems it only needed me to say:

"Yes,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
"

To unblock this particular patch (separate from the rest of the series
it currently resides in.

So there you have it ;-)

With the only action remaining being to try to briefly describe the
problem it fixes in the commit message.


Thanks,
--
Kieran



> 
> Thanks
>   j
> 
>>
>>>>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>>> ---
>>>>>  drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
>>>>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>>>>>  2 files changed, 33 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> index 0e5a75eb6d75..02135741b1a6 100644
>>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
>>>>>
>>>>>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>>>>>  {
>>>>> -	int val;
>>>>> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
>>>>> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> +	struct adv748x_state *state = tx->state;
>>>>> +	int ret;
>>>>>
>>>>>  	if (!is_tx_enabled(tx))
>>>>>  		return 0;
>>>>>
>>>>> -	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
>>>>> -	if (val < 0)
>>>>> -		return val;
>>>>> +	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>>
>>>>>  	/*
>>>>>  	 * This test against BIT(6) is not documented by the datasheet, but was
>>>>>  	 * specified in the downstream driver.
>>>>>  	 * Track with a WARN_ONCE to determine if it is ever set by HW.
>>>>>  	 */
>>>>> -	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>>>>> +	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>>>>>  			"Enabling with unknown bit set");
>>>>>
>>>>> +	/* Configure TXA routing. */
>>>>
>>>> Should TXA routing be configured even on TXB power up? This function
>>>> handles both TX code paths. (Edit: possibly yes)
>>>>
>>>
>>> The comment is wrong, thanks for noticing.
>>>
>>>> Can the logic that determines state->csi4_in_sel value simply be moved
>>>> here (or to an independent adv748x_configure_routing() function)?
>>>>
>>>
>>> It has to be changed at link_setup time in rensponse to a media link
>>> activation or deactivation.
>>>
>>>> I think this patch means that changes to routing will now only take
>>>> effect when starting or stopping a stream, is that right? (If so - could
>>>> that go into the commit message please?)
>>>>
>>>
>>> Well...
>>>
>>>  Post-pone the write of the ADV748X_IO_10 register that controls the active
>>>  routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
>>>  power-up time.
>>>
>>> Doesn't it say that? Or what confused you is that s_stream->s_power(1)
>>> and I should mention streaming instead of power up?
>>
>> I think of "Power up" meaning at device probe time (or possibly some
>> runtime-pm event time). But yes, I think the distinction that it now
>> happens at stream_on/stream_off is important.
>>
>>
>>
>>>
>>>>
>>>>
>>>>
>>>>> +	if (on) {
>>>>> +		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
>>>>> +				state->csi4_in_sel);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +
>>>>> +
>>>>>  	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>>>>>  }
>>>>>
>>>>> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
>>>>>  	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>>>>>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>>>>  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
>>>>> -	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
>>>>> -		       ADV748X_IO_10_CSI4_EN |
>>>>> -		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> -	u8 io10 = 0;
>>>>> +	u8 csi4_in_sel = 0;
>>>>>
>>>>>  	/* Refuse to enable multiple links to the same TX at the same time. */
>>>>>  	if (enable && tx->src)
>>>>> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,
>>>>>
>>>>>  	if (state->afe.tx) {
>>>>>  		/* AFE Requires TXA enabled, even when output to TXB */
>>>>> -		io10 |= ADV748X_IO_10_CSI4_EN;
>>>>> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>>>>>  		if (is_txa(tx))
>>>>> -			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> +			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>
>>>> Hrm ... this is the one part that I think can't be determined without
>>>> caching some sort of value to state the routing.
>>>>
>>>
>>> Yes
>>
>> But the actual hardware shouldn't be updated if the stream is running
>> right? (I wonder if the media-controller would prevent that anyway).
>>
>>
>>
>>>
>>>>>  		else
>>>>> -			io10 |= ADV748X_IO_10_CSI1_EN;
>>>>> +			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>>>>>  	}
>>>>>
>>>>>  	if (state->hdmi.tx)
>>>>> -		io10 |= ADV748X_IO_10_CSI4_EN;
>>>>> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>>>>>
>>>>> -	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
>>>>> +	state->csi4_in_sel = csi4_in_sel;
>>>>> +
>>>>> +	return 0;
>>>>>  }
>>>>>
>>>>>  static const struct media_entity_operations adv748x_tx_media_ops = {
>>>>> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
>>>>>  static int adv748x_reset(struct adv748x_state *state)
>>>>>  {
>>>>>  	int ret;
>>>>> -	u8 regval = 0;
>>>>>
>>>>>  	ret = adv748x_sw_reset(state);
>>>>>  	if (ret < 0)
>>>>> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>
>>>>
>>>> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0
>>>> before setting it?
>>>
>>> I should better check when reset happens, and if it happens only when
>>> links have been disabled or not.
>>> If links are disabled, the variable gets zeroed by the link_setup
>>> callback. If reset happens with links active, we should not zero it
>>> otherwise we lose the configuration
>>
>>
>> Ah yes, I missed that the local variable is initialised to zero, and
>> this state variable is set to the result at the end of the call...
>>
>> It does mean that the function ordering will be important here.
>>
>> It becomes irrelevant if these conditionals move to the same point
>> anyway though.
>>
>>
>>
>>>
>>>>
>>>>
>>>>> +	/* Conditionally enable TXa and TXb. */
>>>>> +	if (is_tx_enabled(&state->txa))
>>>>> +		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>>>>> +	if (is_tx_enabled(&state->txb))
>>>>> +		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>>>>
>>>> This makes it looks like the naming "csi4_in_sel" is less appropriate,
>>>> as it covers both CSI4 and CSI1...
>>>>
>>>
>>> Blame the hw designers :)
>>
>> Always. ... of course they keep blaming us back :D
>>
>>>
>>>> Also, this is confusing two terms, between the 'enable' and the 'select'
>>>>
>>>> The _EN bits looks like they control the activation of the CSI
>>>> transmitter, where as the 'select' bits control the routing.
>>>>
>>>
>>> You are righ. csi4_in_sel should only control the 3 bits used for
>>> routing, while enabling and disabling of the TXes is controlled by
>>> other bits of the io_10 register.
>>> I'll look into changing the name back
>>>
>>>> As the is_tx_enabled($TX) state is constant, perhaps that bit could be
>>>> inferred later when the register is written, and doesn't need to be
>>>> cached here?
>>>
>>> I'll consider that, thanks
>>
>> I think it's only the routing choice that needs to be stored in the
>> state. That would minimise being stored 'globally', and the values could
>> be determined at the time of programming the register.
>>
>>>
>>> Thanks
>>>   j
>>>
>>>>
>>>>
>>>>> +
>>>>>  	/* Reset TXA and TXB */
>>>>>  	adv748x_tx_power(&state->txa, 1);
>>>>>  	adv748x_tx_power(&state->txa, 0);
>>>>> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
>>>>>  	/* Disable chip powerdown & Enable HDMI Rx block */
>>>>>  	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
>>>>>
>>>>> -	/* Conditionally enable TXa and TXb. */
>>>>> -	if (is_tx_enabled(&state->txa))
>>>>> -		regval |= ADV748X_IO_10_CSI4_EN;
>>>>> -	if (is_tx_enabled(&state->txb))
>>>>> -		regval |= ADV748X_IO_10_CSI1_EN;
>>>>> -	io_write(state, ADV748X_IO_10, regval);
>>>>> -
>>>>>  	/* Use vid_std and v_freq as freerun resolution for CP */
>>>>>  	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
>>>>>  					      ADV748X_CP_CLMP_POS_DIS_AUTO);
>>>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>>>>> index 4a5a6445604f..27c116d09284 100644
>>>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>>>> @@ -196,6 +196,8 @@ struct adv748x_state {
>>>>>  	struct adv748x_afe afe;
>>>>>  	struct adv748x_csi2 txa;
>>>>>  	struct adv748x_csi2 txb;
>>>>> +
>>>>> +	unsigned int csi4_in_sel;
>>>>>  };
>>>>>
>>>>>  #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
>>>>> --
>>>>> 2.21.0
>>>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran

[RFC 3/5] media: adv748x: Make lanes number depend on routing

[Jacopo Mondi]

Use the TXA routing information to configure the number of active CSI-2
data lanes. When routing AFE through TXA limit the number of data lanes
to 1, while in the canonical HDMI->TXA routing use all the physically
available ones.

The number of lanes collected from the 'data-lanes' DT property is now
used as a the number of physically available data lanes, while the
'num_lanes' variable contains the number of active ones.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 19 ++++++++++++++++++-
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 02135741b1a6..f91c7b83f1bf 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -350,6 +350,8 @@ static int adv748x_link_setup(struct media_entity *entity,
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
 	bool enable = flags & MEDIA_LNK_FL_ENABLED;
 	u8 csi4_in_sel = 0;
+	u8 num_lanes;
+	int ret;
 
 	/* Refuse to enable multiple links to the same TX at the same time. */
 	if (enable && tx->src)
@@ -373,10 +375,23 @@ static int adv748x_link_setup(struct media_entity *entity,
 			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
 		else
 			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
+
+		num_lanes = 1;
 	}
 
-	if (state->hdmi.tx)
+	if (state->hdmi.tx) {
 		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
+		num_lanes = tx->available_lanes;
+	}
+
+	/* Update the number of active lanes if it has changed. */
+	if (num_lanes != tx->num_lanes) {
+		tx->num_lanes = num_lanes;
+		ret = adv748x_write(state, tx->page, 0x00,
+				    0x80 | tx->num_lanes);
+		if (ret)
+			return ret;
+	}
 
 	state->csi4_in_sel = csi4_in_sel;
 
@@ -608,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txa.num_lanes = num_lanes;
+		state->txa.available_lanes = num_lanes;
 		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
 	}
 
@@ -619,6 +635,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txb.num_lanes = num_lanes;
+		state->txb.available_lanes = num_lanes;
 		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
 	}
 
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 27c116d09284..6e5c2cb421fe 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -80,6 +80,7 @@ struct adv748x_csi2 {
 	unsigned int page;
 	unsigned int port;
 	unsigned int num_lanes;
+	unsigned int available_lanes;
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
-- 
2.21.0

Re: [RFC 3/5] media: adv748x: Make lanes number depend on routing

[Kieran Bingham]

Hi Jacopo,

On 16/03/2019 15:47, Jacopo Mondi wrote:
> Use the TXA routing information to configure the number of active CSI-2
> data lanes. When routing AFE through TXA limit the number of data lanes
> to 1, while in the canonical HDMI->TXA routing use all the physically
> available ones.
> 
> The number of lanes collected from the 'data-lanes' DT property is now
> used as a the number of physically available data lanes, while the
> 'num_lanes' variable contains the number of active ones.

Could the variable perhaps be called active_lanes rather than num_lanes
if it represents how many are active rather than how many there are?


> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 19 ++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  1 +
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 02135741b1a6..f91c7b83f1bf 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -350,6 +350,8 @@ static int adv748x_link_setup(struct media_entity *entity,
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
>  	u8 csi4_in_sel = 0;
> +	u8 num_lanes;
> +	int ret;
>  
>  	/* Refuse to enable multiple links to the same TX at the same time. */
>  	if (enable && tx->src)
> @@ -373,10 +375,23 @@ static int adv748x_link_setup(struct media_entity *entity,
>  			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>  		else
>  			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
> +
> +		num_lanes = 1;
>  	}
>  
> -	if (state->hdmi.tx)
> +	if (state->hdmi.tx) {
>  		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> +		num_lanes = tx->available_lanes;
> +	}
> +
> +	/* Update the number of active lanes if it has changed. */
> +	if (num_lanes != tx->num_lanes) {
> +		tx->num_lanes = num_lanes;
> +		ret = adv748x_write(state, tx->page, 0x00,
> +				    0x80 | tx->num_lanes);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	state->csi4_in_sel = csi4_in_sel;
>  
> @@ -608,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>  		}
>  
>  		state->txa.num_lanes = num_lanes;
> +		state->txa.available_lanes = num_lanes;
>  		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
>  	}
>  
> @@ -619,6 +635,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>  		}
>  
>  		state->txb.num_lanes = num_lanes;
> +		state->txb.available_lanes = num_lanes;
>  		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
>  	}
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 27c116d09284..6e5c2cb421fe 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -80,6 +80,7 @@ struct adv748x_csi2 {
>  	unsigned int page;
>  	unsigned int port;
>  	unsigned int num_lanes;

Aha, except num_lanes looks like its' from an earlier patch...

I wonder if it's worth renaming...

> +	unsigned int available_lanes;
>  
>  	struct media_pad pads[ADV748X_CSI2_NR_PADS];
>  	struct v4l2_ctrl_handler ctrl_hdl;
> 

-- 
Regards
--
Kieran

[RFC 4/5] media: adv748x: Report D-PHY configuration

[Jacopo Mondi]

Extend the media bus frame description to report the D-PHY bus
configuration.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 13454af72c6e..c733c7ab8247 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -231,8 +231,12 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 				       struct v4l2_mbus_frame_desc *fd)
 {
+	struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy;
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+	struct v4l2_mbus_frame_desc_entry_csi2 *csi2;
 	struct v4l2_mbus_framefmt *mbusformat;
+	unsigned int i;
+
 
 	memset(fd, 0, sizeof(*fd));
 
@@ -244,13 +248,20 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	if (!mbusformat)
 		return -EINVAL;
 
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY;
+	fd->num_entries = 1;
+
 	fd->entry->stream = tx->vc;
-	fd->entry->bus.csi2.channel = tx->vc;
-	fd->entry->bus.csi2.data_type =
-		adv748x_csi2_code_to_datatype(mbusformat->code);
 
-	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
-	fd->num_entries = 1;
+	csi2 = &fd->entry->bus.csi2;
+	csi2->channel = tx->vc;
+	csi2->data_type = adv748x_csi2_code_to_datatype(mbusformat->code);
+
+	dphy = &fd->phy.csi2_dphy;
+	dphy->clock_lane = 0;
+	for (i = 0; i < tx->num_lanes; ++i)
+		dphy->data_lanes[i] = i + 1;
+	dphy->num_data_lanes = tx->num_lanes;
 
 	return 0;
 }
-- 
2.21.0

[RFC 5/5] media: rcar-csi2: Configure CSI-2 with frame desc

[Jacopo Mondi]

Use the D-PHY configuration reported by the remote subdevice in its
frame descriptor to configure the interface.

Store the number of lanes reported through the 'data-lanes' DT property
as the number of phyisically available lanes, which might not correspond
to the number of lanes actually in use.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++--------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 6c46bcc0ee83..70b9a8165a6e 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -375,8 +375,8 @@ struct rcar_csi2 {
 	struct mutex lock;
 	int stream_count;
 
-	unsigned short lanes;
-	unsigned char lane_swap[4];
+	unsigned short available_data_lanes;
+	unsigned short num_data_lanes;
 };
 
 static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
@@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
 	if (ret)
 		return -ENODEV;
 
-	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
+	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) {
 		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
 		return -EINVAL;
 	}
@@ -438,7 +438,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 
 	/* Wait for the clock and data lanes to enter LP-11 state. */
 	for (timeout = 0; timeout <= 20; timeout++) {
-		const u32 lane_mask = (1 << priv->lanes) - 1;
+		const u32 lane_mask = (1 << priv->num_data_lanes) - 1;
 
 		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
 		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
@@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
 	 * bps = link_freq * 2
 	 */
 	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
-	do_div(mbps, priv->lanes * 1000000);
+	do_div(mbps, priv->num_data_lanes * 1000000);
 
 	return mbps;
 }
 
 static int rcsi2_start(struct rcar_csi2 *priv)
 {
-	u32 phycnt, vcdt = 0, vcdt2 = 0;
+	struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy;
+	u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0;
 	struct v4l2_mbus_frame_desc fd;
 	unsigned int i;
 	int mbps, ret;
@@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv)
 			entry->bus.csi2.channel, entry->bus.csi2.data_type);
 	}
 
+	/* Get description of the D-PHY media bus configuration. */
+	dphy = &fd.phy.csi2_dphy;
+	if (dphy->clock_lane != 0) {
+		dev_err(priv->dev,
+			"CSI-2 configuration not supported: clock at index %u",
+			dphy->clock_lane);
+		return -EINVAL;
+	}
+
+	if (dphy->num_data_lanes > priv->available_data_lanes ||
+	    dphy->num_data_lanes == 3) {
+		dev_err(priv->dev,
+			"Number of CSI-2 data lanes not supported: %u",
+			dphy->num_data_lanes);
+		return -EINVAL;
+	}
+	priv->num_data_lanes = dphy->num_data_lanes;
+
 	phycnt = PHYCNT_ENABLECLK;
-	phycnt |= (1 << priv->lanes) - 1;
+	phycnt |= (1 << priv->num_data_lanes) - 1;
 
 	mbps = rcsi2_calc_mbps(priv, &fd);
 	if (mbps < 0)
@@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv)
 	rcsi2_write(priv, VCDT_REG, vcdt);
 	if (vcdt2)
 		rcsi2_write(priv, VCDT2_REG, vcdt2);
+
 	/* Lanes are zero indexed. */
-	rcsi2_write(priv, LSWAP_REG,
-		    LSWAP_L0SEL(priv->lane_swap[0] - 1) |
-		    LSWAP_L1SEL(priv->lane_swap[1] - 1) |
-		    LSWAP_L2SEL(priv->lane_swap[2] - 1) |
-		    LSWAP_L3SEL(priv->lane_swap[3] - 1));
+	for (i = 0; i < priv->num_data_lanes; ++i)
+		lswap |= (dphy->data_lanes[i] - 1) << (i * 2);
+	rcsi2_write(priv, LSWAP_REG, lswap);
 
 	/* Start */
 	if (priv->info->init_phtw) {
@@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
 static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 			    struct v4l2_fwnode_endpoint *vep)
 {
-	unsigned int i;
+	unsigned int num_data_lanes;
 
 	/* Only port 0 endpoint 0 is valid. */
 	if (vep->base.port || vep->base.id)
@@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 		return -EINVAL;
 	}
 
-	priv->lanes = vep->bus.mipi_csi2.num_data_lanes;
-	if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) {
+	num_data_lanes = vep->bus.mipi_csi2.num_data_lanes;
+	switch (num_data_lanes) {
+	case 1:
+		/* fallthrough */
+	case 2:
+		/* fallthrough */
+	case 4:
+		priv->available_data_lanes = num_data_lanes;
+		break;
+	default:
 		dev_err(priv->dev, "Unsupported number of data-lanes: %u\n",
-			priv->lanes);
+			num_data_lanes);
 		return -EINVAL;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
-		priv->lane_swap[i] = i < priv->lanes ?
-			vep->bus.mipi_csi2.data_lanes[i] : i;
-
-		/* Check for valid lane number. */
-		if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) {
-			dev_err(priv->dev, "data-lanes must be in 1-4 range\n");
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }
 
@@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error;
 
-	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
+	dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes);
 
 	return 0;
 
-- 
2.21.0

Re: [RFC 5/5] media: rcar-csi2: Configure CSI-2 with frame desc

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2019-03-16 16:48:01 +0100, Jacopo Mondi wrote:
> Use the D-PHY configuration reported by the remote subdevice in its
> frame descriptor to configure the interface.
> 
> Store the number of lanes reported through the 'data-lanes' DT property
> as the number of phyisically available lanes, which might not correspond
> to the number of lanes actually in use.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++--------
>  1 file changed, 43 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 6c46bcc0ee83..70b9a8165a6e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -375,8 +375,8 @@ struct rcar_csi2 {
>  	struct mutex lock;
>  	int stream_count;
>  
> -	unsigned short lanes;
> -	unsigned char lane_swap[4];
> +	unsigned short available_data_lanes;
> +	unsigned short num_data_lanes;

I don't like this, I'm sorry :-(

I think you should keep lanes and lane_swap and most code touching them 
intact. And drop num_data_lanes as it's only used when starting and only 
valid for one 'session' and should not be cached in the data structure 
unnecessary.

Furthermore I think involving lane swapping in the runtime configurable 
parameters is a bad idea. Or do you see a scenario where lanes could be 
swapped in runtime? In my view DT should describe which physical lanes 
are connected and how. And the runtime configuration part should only 
deal with how many lanes are used for the particular capture session.

>  };
>  
>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> @@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
>  	if (ret)
>  		return -ENODEV;
>  
> -	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) {
>  		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
>  		return -EINVAL;
>  	}
> @@ -438,7 +438,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  
>  	/* Wait for the clock and data lanes to enter LP-11 state. */
>  	for (timeout = 0; timeout <= 20; timeout++) {
> -		const u32 lane_mask = (1 << priv->lanes) - 1;
> +		const u32 lane_mask = (1 << priv->num_data_lanes) - 1;
>  
>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> @@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>  	 * bps = link_freq * 2
>  	 */
>  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> -	do_div(mbps, priv->lanes * 1000000);
> +	do_div(mbps, priv->num_data_lanes * 1000000);
>  
>  	return mbps;
>  }
>  
>  static int rcsi2_start(struct rcar_csi2 *priv)
>  {
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0;
>  	struct v4l2_mbus_frame_desc fd;
>  	unsigned int i;
>  	int mbps, ret;
> @@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>  			entry->bus.csi2.channel, entry->bus.csi2.data_type);
>  	}
>  
> +	/* Get description of the D-PHY media bus configuration. */
> +	dphy = &fd.phy.csi2_dphy;
> +	if (dphy->clock_lane != 0) {
> +		dev_err(priv->dev,
> +			"CSI-2 configuration not supported: clock at index %u",
> +			dphy->clock_lane);
> +		return -EINVAL;
> +	}
> +
> +	if (dphy->num_data_lanes > priv->available_data_lanes ||
> +	    dphy->num_data_lanes == 3) {
> +		dev_err(priv->dev,
> +			"Number of CSI-2 data lanes not supported: %u",
> +			dphy->num_data_lanes);
> +		return -EINVAL;
> +	}
> +	priv->num_data_lanes = dphy->num_data_lanes;
> +
>  	phycnt = PHYCNT_ENABLECLK;
> -	phycnt |= (1 << priv->lanes) - 1;
> +	phycnt |= (1 << priv->num_data_lanes) - 1;
>  
>  	mbps = rcsi2_calc_mbps(priv, &fd);
>  	if (mbps < 0)
> @@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>  	rcsi2_write(priv, VCDT_REG, vcdt);
>  	if (vcdt2)
>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> +
>  	/* Lanes are zero indexed. */
> -	rcsi2_write(priv, LSWAP_REG,
> -		    LSWAP_L0SEL(priv->lane_swap[0] - 1) |
> -		    LSWAP_L1SEL(priv->lane_swap[1] - 1) |
> -		    LSWAP_L2SEL(priv->lane_swap[2] - 1) |
> -		    LSWAP_L3SEL(priv->lane_swap[3] - 1));
> +	for (i = 0; i < priv->num_data_lanes; ++i)
> +		lswap |= (dphy->data_lanes[i] - 1) << (i * 2);
> +	rcsi2_write(priv, LSWAP_REG, lswap);
>  
>  	/* Start */
>  	if (priv->info->init_phtw) {
> @@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
>  static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  			    struct v4l2_fwnode_endpoint *vep)
>  {
> -	unsigned int i;
> +	unsigned int num_data_lanes;
>  
>  	/* Only port 0 endpoint 0 is valid. */
>  	if (vep->base.port || vep->base.id)
> @@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  		return -EINVAL;
>  	}
>  
> -	priv->lanes = vep->bus.mipi_csi2.num_data_lanes;
> -	if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) {
> +	num_data_lanes = vep->bus.mipi_csi2.num_data_lanes;
> +	switch (num_data_lanes) {
> +	case 1:
> +		/* fallthrough */
> +	case 2:
> +		/* fallthrough */
> +	case 4:
> +		priv->available_data_lanes = num_data_lanes;
> +		break;
> +	default:

Nit pick, I don't like this switch statement and would prefer the 
original construct with an if.

>  		dev_err(priv->dev, "Unsupported number of data-lanes: %u\n",
> -			priv->lanes);
> +			num_data_lanes);
>  		return -EINVAL;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> -		priv->lane_swap[i] = i < priv->lanes ?
> -			vep->bus.mipi_csi2.data_lanes[i] : i;
> -
> -		/* Check for valid lane number. */
> -		if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) {
> -			dev_err(priv->dev, "data-lanes must be in 1-4 range\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error;
>  
> -	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> +	dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes);
>  
>  	return 0;
>  
> -- 
> 2.21.0
> 

-- 
Regards,
Niklas Söderlund

Re: [RFC 5/5] media: rcar-csi2: Configure CSI-2 with frame desc

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 7248 bytes --]

Hi Niklas,

On Wed, Mar 20, 2019 at 08:50:15PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-03-16 16:48:01 +0100, Jacopo Mondi wrote:
> > Use the D-PHY configuration reported by the remote subdevice in its
> > frame descriptor to configure the interface.
> >
> > Store the number of lanes reported through the 'data-lanes' DT property
> > as the number of phyisically available lanes, which might not correspond
> > to the number of lanes actually in use.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++--------
> >  1 file changed, 43 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 6c46bcc0ee83..70b9a8165a6e 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -375,8 +375,8 @@ struct rcar_csi2 {
> >  	struct mutex lock;
> >  	int stream_count;
> >
> > -	unsigned short lanes;
> > -	unsigned char lane_swap[4];
> > +	unsigned short available_data_lanes;
> > +	unsigned short num_data_lanes;
>
> I don't like this, I'm sorry :-(
>
> I think you should keep lanes and lane_swap and most code touching them
> intact. And drop num_data_lanes as it's only used when starting and only
> valid for one 'session' and should not be cached in the data structure
> unnecessary.
>
> Furthermore I think involving lane swapping in the runtime configurable
> parameters is a bad idea. Or do you see a scenario where lanes could be
> swapped in runtime? In my view DT should describe which physical lanes
> are connected and how. And the runtime configuration part should only
> deal with how many lanes are used for the particular capture session.
>

Yeah, it's dumb, it was noted in the review of the framework
side as well..

I'll drop anything related to lane re-ordering, so I should not need
to touch 'lanes' and 'lane_swap'. I need 'num_data_lanes' in 'struct
rcar_csi2' though, as it is used in a few function called by
rcsi2_start().

Thanks
  j

> >  };
> >
> >  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > @@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
> >  	if (ret)
> >  		return -ENODEV;
> >
> > -	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) {
> >  		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
> >  		return -EINVAL;
> >  	}
> > @@ -438,7 +438,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >
> >  	/* Wait for the clock and data lanes to enter LP-11 state. */
> >  	for (timeout = 0; timeout <= 20; timeout++) {
> > -		const u32 lane_mask = (1 << priv->lanes) - 1;
> > +		const u32 lane_mask = (1 << priv->num_data_lanes) - 1;
> >
> >  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> >  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > @@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
> >  	 * bps = link_freq * 2
> >  	 */
> >  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > -	do_div(mbps, priv->lanes * 1000000);
> > +	do_div(mbps, priv->num_data_lanes * 1000000);
> >
> >  	return mbps;
> >  }
> >
> >  static int rcsi2_start(struct rcar_csi2 *priv)
> >  {
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0;
> >  	struct v4l2_mbus_frame_desc fd;
> >  	unsigned int i;
> >  	int mbps, ret;
> > @@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >  			entry->bus.csi2.channel, entry->bus.csi2.data_type);
> >  	}
> >
> > +	/* Get description of the D-PHY media bus configuration. */
> > +	dphy = &fd.phy.csi2_dphy;
> > +	if (dphy->clock_lane != 0) {
> > +		dev_err(priv->dev,
> > +			"CSI-2 configuration not supported: clock at index %u",
> > +			dphy->clock_lane);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (dphy->num_data_lanes > priv->available_data_lanes ||
> > +	    dphy->num_data_lanes == 3) {
> > +		dev_err(priv->dev,
> > +			"Number of CSI-2 data lanes not supported: %u",
> > +			dphy->num_data_lanes);
> > +		return -EINVAL;
> > +	}
> > +	priv->num_data_lanes = dphy->num_data_lanes;
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> > -	phycnt |= (1 << priv->lanes) - 1;
> > +	phycnt |= (1 << priv->num_data_lanes) - 1;
> >
> >  	mbps = rcsi2_calc_mbps(priv, &fd);
> >  	if (mbps < 0)
> > @@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, VCDT_REG, vcdt);
> >  	if (vcdt2)
> >  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> > +
> >  	/* Lanes are zero indexed. */
> > -	rcsi2_write(priv, LSWAP_REG,
> > -		    LSWAP_L0SEL(priv->lane_swap[0] - 1) |
> > -		    LSWAP_L1SEL(priv->lane_swap[1] - 1) |
> > -		    LSWAP_L2SEL(priv->lane_swap[2] - 1) |
> > -		    LSWAP_L3SEL(priv->lane_swap[3] - 1));
> > +	for (i = 0; i < priv->num_data_lanes; ++i)
> > +		lswap |= (dphy->data_lanes[i] - 1) << (i * 2);
> > +	rcsi2_write(priv, LSWAP_REG, lswap);
> >
> >  	/* Start */
> >  	if (priv->info->init_phtw) {
> > @@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = {
> >  static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  			    struct v4l2_fwnode_endpoint *vep)
> >  {
> > -	unsigned int i;
> > +	unsigned int num_data_lanes;
> >
> >  	/* Only port 0 endpoint 0 is valid. */
> >  	if (vep->base.port || vep->base.id)
> > @@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  		return -EINVAL;
> >  	}
> >
> > -	priv->lanes = vep->bus.mipi_csi2.num_data_lanes;
> > -	if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) {
> > +	num_data_lanes = vep->bus.mipi_csi2.num_data_lanes;
> > +	switch (num_data_lanes) {
> > +	case 1:
> > +		/* fallthrough */
> > +	case 2:
> > +		/* fallthrough */
> > +	case 4:
> > +		priv->available_data_lanes = num_data_lanes;
> > +		break;
> > +	default:
>
> Nit pick, I don't like this switch statement and would prefer the
> original construct with an if.
>
> >  		dev_err(priv->dev, "Unsupported number of data-lanes: %u\n",
> > -			priv->lanes);
> > +			num_data_lanes);
> >  		return -EINVAL;
> >  	}
> >
> > -	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> > -		priv->lane_swap[i] = i < priv->lanes ?
> > -			vep->bus.mipi_csi2.data_lanes[i] : i;
> > -
> > -		/* Check for valid lane number. */
> > -		if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) {
> > -			dev_err(priv->dev, "data-lanes must be in 1-4 range\n");
> > -			return -EINVAL;
> > -		}
> > -	}
> > -
> >  	return 0;
> >  }
> >
> > @@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto error;
> >
> > -	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> > +	dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes);
> >
> >  	return 0;
> >
> > --
> > 2.21.0
> >
>
> --
> Regards,
> Niklas Söderlund

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]