Re: [PATCH v2 2/2] rcar-csi2: Use standby mode instead of resetting

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

Hi Niklas,

On Tue, Mar 12, 2019 at 07:10:07PM +0100, Niklas Söderlund wrote:
> On 2019-03-11 11:53:51 +0200, Laurent Pinchart wrote:
> > On Sat, Mar 09, 2019 at 12:47:22AM +0100, Niklas Söderlund wrote:
> >> Later versions of the datasheet updates the reset procedure to more
> >> closely resemble the standby mode. Update the driver to enter and exit
> >> the standby mode instead of resetting the hardware before and after
> >> streaming is started and stopped.
> > 
> > I was mislead thinking this added support for module reset in addition
> > to software reset (SRST_SRST-, but it actually removes the software
> > reset completely. Should the commit message make this clear ?
> 
> Good point, will add a clarification in next version.
> 
> >> While at it break out the full start and stop procedures from
> >> rcsi2_s_stream() into the existing helper functions.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>  drivers/media/platform/rcar-vin/Kconfig     |  1 +
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 69 +++++++++++++--------
> >>  2 files changed, 43 insertions(+), 27 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/Kconfig b/drivers/media/platform/rcar-vin/Kconfig
> >> index e3eb8fee253658da..f26f47e3bcf44825 100644
> >> --- a/drivers/media/platform/rcar-vin/Kconfig
> >> +++ b/drivers/media/platform/rcar-vin/Kconfig
> >> @@ -3,6 +3,7 @@ config VIDEO_RCAR_CSI2
> >>  	tristate "R-Car MIPI CSI-2 Receiver"
> >>  	depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
> >>  	depends on ARCH_RENESAS || COMPILE_TEST
> >> +	select RESET_CONTROLLER
> >>  	select V4L2_FWNODE
> >>  	help
> >>  	  Support for Renesas R-Car MIPI CSI-2 receiver.
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> index f64528d2be3c95dd..7a1c9b549e0fffc6 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -14,6 +14,7 @@
> >>  #include <linux/of_graph.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >>  #include <linux/sys_soc.h>
> >>  
> >>  #include <media/v4l2-ctrls.h>
> >> @@ -350,6 +351,7 @@ struct rcar_csi2 {
> >>  	struct device *dev;
> >>  	void __iomem *base;
> >>  	const struct rcar_csi2_info *info;
> >> +	struct reset_control *rstc;
> >>  
> >>  	struct v4l2_subdev subdev;
> >>  	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> >> @@ -387,11 +389,19 @@ static void rcsi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data)
> >>  	iowrite32(data, priv->base + reg);
> >>  }
> >>  
> >> -static void rcsi2_reset(struct rcar_csi2 *priv)
> >> +static void rcsi2_enter_standby(struct rcar_csi2 *priv)
> >>  {
> >> -	rcsi2_write(priv, SRST_REG, SRST_SRST);
> >> +	rcsi2_write(priv, PHYCNT_REG, 0);
> >> +	rcsi2_write(priv, PHTC_REG, PHTC_TESTCLR);
> >> +	reset_control_assert(priv->rstc);
> >>  	usleep_range(100, 150);
> >> -	rcsi2_write(priv, SRST_REG, 0);
> >> +	pm_runtime_put(priv->dev);
> >> +}
> >> +
> >> +static void rcsi2_exit_standby(struct rcar_csi2 *priv)
> >> +{
> >> +	pm_runtime_get_sync(priv->dev);
> >> +	reset_control_deassert(priv->rstc);
> >>  }
> >>  
> >>  static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >> @@ -462,7 +472,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >>  	return mbps;
> >>  }
> >>  
> >> -static int rcsi2_start(struct rcar_csi2 *priv)
> >> +static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  {
> >>  	const struct rcar_csi2_format *format;
> >>  	u32 phycnt, vcdt = 0, vcdt2 = 0;
> >> @@ -506,7 +516,6 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >>  
> >>  	/* Init */
> >>  	rcsi2_write(priv, TREF_REG, TREF_TREF);
> >> -	rcsi2_reset(priv);
> >>  	rcsi2_write(priv, PHTC_REG, 0);
> >>  
> >>  	/* Configure */
> >> @@ -564,19 +573,36 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >>  	return 0;
> >>  }
> >>  
> >> +static int rcsi2_start(struct rcar_csi2 *priv)
> >> +{
> >> +	int ret;
> >> +
> >> +	rcsi2_exit_standby(priv);
> >> +
> >> +	ret = rcsi2_start_receiver(priv);
> > 
> > The rcsi2_start_receiver() function should be split in two, with
> > rcsi2_wait_phy_start() and below being performed after starting the
> > source. Alternatively you could start the source first, but I think a
> > split would be better.
> 
> I don't think that would work nor is correct. In Figure 25.16.1.1 data 
> transfer should be started after confirmation of PHY start. If we call 
> s_stream on the source before confirming PHY start we will never be able 
> to detect LP-11.
> 
> I interpret the diagram as the CSI-2 transmitter should be powered on 
> before conforming PHY start, something that should happen in s_power not 
> s_stream. We handle s_power correctly today using the v4l2-mc link 
> handling so I believe the current code is correct.
> 
> Would you agree with my reasoning?

Yes, I agree with you, I was wrong.

> >> +	if (ret) {
> >> +		rcsi2_enter_standby(priv);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = v4l2_subdev_call(priv->remote, video, s_stream, 1);
> >> +	if (ret) {
> >> +		rcsi2_enter_standby(priv);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static void rcsi2_stop(struct rcar_csi2 *priv)
> >>  {
> >> -	rcsi2_write(priv, PHYCNT_REG, 0);
> >> -
> >> -	rcsi2_reset(priv);
> >> -
> >> -	rcsi2_write(priv, PHTC_REG, PHTC_TESTCLR);
> >> +	v4l2_subdev_call(priv->remote, video, s_stream, 0);
> >> +	rcsi2_enter_standby(priv);
> > 
> > Shouldn't you enter standby before stopping the source ? Otherwise
> > there's a risk of CSI errors being detected. Figures 25.23.1 and 25.23.2
> > seem to agree with me.
> 
> I agree with you on this, it should enter standby before stopping the 
> source. Will change this for next version.
> 
> >>  }
> >>  
> >>  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> >>  {
> >>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> >> -	struct v4l2_subdev *nextsd;
> >>  	int ret = 0;
> >>  
> >>  	mutex_lock(&priv->lock);
> >> @@ -586,27 +612,12 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> >>  		goto out;
> >>  	}
> >>  
> >> -	nextsd = priv->remote;
> >> -
> >>  	if (enable && priv->stream_count == 0) {
> >> -		pm_runtime_get_sync(priv->dev);
> >> -
> >>  		ret = rcsi2_start(priv);
> >> -		if (ret) {
> >> -			pm_runtime_put(priv->dev);
> >> +		if (ret)
> >>  			goto out;
> >> -		}
> >> -
> >> -		ret = v4l2_subdev_call(nextsd, video, s_stream, 1);
> >> -		if (ret) {
> >> -			rcsi2_stop(priv);
> >> -			pm_runtime_put(priv->dev);
> >> -			goto out;
> >> -		}
> >>  	} else if (!enable && priv->stream_count == 1) {
> >>  		rcsi2_stop(priv);
> >> -		v4l2_subdev_call(nextsd, video, s_stream, 0);
> >> -		pm_runtime_put(priv->dev);
> >>  	}
> >>  
> >>  	priv->stream_count += enable ? 1 : -1;
> >> @@ -936,6 +947,10 @@ static int rcsi2_probe_resources(struct rcar_csi2 *priv,
> >>  	if (irq < 0)
> >>  		return irq;
> >>  
> >> +	priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(priv->rstc))
> >> +		return PTR_ERR(priv->rstc);
> >> +
> >>  	return 0;
> >>  }
> >>  

-- 
Regards,

Laurent Pinchart