From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH] rcar-csi2: Use standby mode instead of resetting
Date: Thu, 7 Mar 2019 01:03:38 +0100 [thread overview]
Message-ID: <20190307000338.GA10014@bigcity.dyn.berto.se> (raw)
In-Reply-To: <20190217194124.fziv3yfr2xlvvcib@uno.localdomain>
Hi Jacopo,
Thanks for your feedback.
On 2019-02-17 20:41:24 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> ah, ups, this was maybe the patch the other one I just reviewed was
> based on... sorry, I missed this one :)
:-)
>
> On Sat, Feb 16, 2019 at 11:56:38PM +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.
> >
> > 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/rcar-csi2.c | 69 +++++++++++++--------
> > 1 file changed, 42 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index f64528d2be3c95dd..f3099f3e536d808a 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_standby_mode(struct rcar_csi2 *priv, int on)
> > {
> > - rcsi2_write(priv, SRST_REG, SRST_SRST);
> > + if (!on) {
>
> minor thing: if (!on) { "wakeup"; } is confusing. What if you call the
> variable "standby" or just "off" ?
I agree this was a bad design, I will split this function in two.
rcsi2_enter_standby()
rcsi2_exit_standby()
>
> > + pm_runtime_get_sync(priv->dev);
> > + reset_control_deassert(priv->rstc);
> > + return;
> > + }
> > +
> > + 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 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_standby_mode(priv, 0);
> > +
> > + ret = rcsi2_start_receiver(priv);
> > + if (ret) {
> > + rcsi2_standby_mode(priv, 1);
> > + return ret;
> > + }
> > +
> > + ret = v4l2_subdev_call(priv->remote, video, s_stream, 1);
>
> minor thing as well, but I feel this one was better where it was, so
> that "rcsi2_start()" only handles the hardware, while s_stream handles
> the pipeline. But then _start() and _stop() becomes very short... so
> yeah, feel free to keep it the way it is.
Do not agree, I like this :-)
>
> > + if (ret) {
> > + rcsi2_standby_mode(priv, 1);
> > + 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_standby_mode(priv, 1);
> > }
> >
> > 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);
> > +
>
> I don't see 'resets' listed as a mandatory property of the rcar-csi2
> bindings, shouldn't you fallback to software reset if not 'reset'
> is specified? True that all mainline users have a reset property specified,
> so you could also add 'resets' among the mandatory properties, could
> that break out of tree implementations in your opinion?
Nice catch! I will add a patch to this series listing it as mandatory.
It's a good thing the resets property always have been part of the dts
sources so it will not create any regressions.
>
> Thanks
> j
>
> > return 0;
> > }
> >
> > --
> > 2.20.1
> >
--
Regards,
Niklas Söderlund
next prev parent reply other threads:[~2019-03-07 0:03 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-16 22:56 [PATCH] rcar-csi2: Use standby mode instead of resetting Niklas Söderlund
2019-02-17 19:41 ` Jacopo Mondi
2019-03-07 0:03 ` Niklas Söderlund [this message]
2019-02-18 9:07 ` Geert Uytterhoeven
2019-03-07 0:12 ` Niklas Söderlund
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190307000338.GA10014@bigcity.dyn.berto.se \
--to=niklas.soderlund@ragnatech.se \
--cc=jacopo@jmondi.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).