Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver

[jacopo mondi]

Hi Laurent,

On Mon, Dec 18, 2017 at 05:28:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
> > On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
[snip]
> > >> +/**
> > >> + * ceu_soft_reset() - Software reset the CEU interface
> > >> + */
> > >> +static int ceu_soft_reset(struct ceu_device *ceudev)
> > >> +{
> > >> +	unsigned int reset_done;
> > >> +	unsigned int i;
> > >> +
> > >> +	ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> > >> +
> > >> +	reset_done = 0;
> > >> +	for (i = 0; i < 1000 && !reset_done; i++) {
> > >> +		udelay(1);
> > >> +		if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> > >> +			reset_done++;
> > >> +	}
> > >
> > > How many iterations does this typically require ? Wouldn't a sleep be
> > > better than a delay ? As far as I can tell the ceu_soft_reset() function
> > > is only called with interrupts disabled (in interrupt context) from
> > > ceu_capture() in an error path, and that code should be reworked to make
> > > it possible to sleep if a reset takes too long.
> >
> > The HW manual does not provide any indication about absolute timings.
> > I can empirically try and see, but that would just be a hint.
>
> That's why I asked how many iterations it typically takes :-) A hint is enough
> to start with, preferably on both SH and ARM SoCs.

I've seen only 0s when printing out how many cycles it takes to clear
both registers. This means 1usec is enough, therefore I would keep using
udelay here. Also, I would reduce the attempts to 100 here (or even
10), as if a single one is typically enough, 1000 is definitely an
overkill.

>
> > Also, the reset function is called in many places (runtime_pm
> > suspend/resume) s_stream(0) and in error path of ceu_capture().
> >
> > In ceu_capture() we reset the interface if the previous frame was bad,
> > and we do that before re-enabling the capture interrupt (so interrupts
> > are not -disabled-, just the one we care about is not enabled yet..)
>
> The ceu_capture() function is called from the driver's interrupt handler, so
> interrupts are disabled in that code path.
>

I have removed that reset call from capture and re-worked the irq
handler to manage state before calling capture().

> > But that's not big deal, as if we fail there, we are about to abort
> > capture anyhow and so if we miss some spurious capture interrupt it's
> > ok...
> >
> > >> +	if (!reset_done) {
> > >> +		v4l2_err(&ceudev->v4l2_dev, "soft reset time out\n");
> > >
> > > How about dev_err() instead ?
> >
> > Is dev_err/dev_dbg preferred over v4l2_err/v4l2_dbg? Is this because
> > of dynamic debug?
>
> Yes, and the fact that the V4L2 macros don't provide us anymore with much
> compared to the dev_* macros.
>
> > >> +
> > >> +/**
> > >> + * ceu_capture() - Trigger start of a capture sequence
> > >> + *
> > >> + * Return value doesn't reflect the success/failure to queue the new
> > >> buffer,
> > >> + * but rather the status of the previous capture.
> > >> + */
> > >> +static int ceu_capture(struct ceu_device *ceudev)
> > >> +{
> > >> +	struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> > >> +	dma_addr_t phys_addr_top;
> > >> +	u32 status;
> > >> +
> > >> +	/* Clean interrupt status and re-enable interrupts */
> > >> +	status = ceu_read(ceudev, CEU_CETCR);
> > >> +	ceu_write(ceudev, CEU_CEIER,
> > >> +		  ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> > >> +	ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> > >> +	ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> > >
> > > I wonder why there's a need to disable and reenable interrupts here.
> >
> > The original driver clearly said "The hardware is -very- picky about
> > this sequence" and I got scared and nerver touched this.
>
> How about experimenting to see how the hardware reacts ?

Turns out this was not needed at all, both on RZ and SH4. I captured
several images without any issues in both platforms just clearing the
interrupt state without disabling interrutps.

>
> > Also, I very much dislike the CEU_CETRC_MAGIC mask, but again the old driver
> > said "Acknoledge magical interrupt sources" and I was afraid to change it
> > (I can rename it though, to something lioke CEU_CETCR_ALL_INT because that's
> > what it is, a mask with all available interrupt source enabled).
>
> I think renaming it is a good idea. Additionally, regardless of whether there
> is any hidden interrupt source, the datasheet mentions for all reserved bits
> that "The write  value  should always  be 0". They should read as 0, but
> masking them would be an additional safeguard.

The HW manual is a bit confused (and confusing) on this point.
Yes, there is the statement you have cited here, but there's also
"to clear only the CPE bit to 0, write H'FFFF FFFE to CETCR" a few
lines above, which clearly contradicts the "write 0 to reserved bits"
thing.

In practice, I'm now writing to 0 only bits to be cleared, and thus
writing 1s to everything else, reserved included. I haven't seen any
issue both on RZ and SH4 platforms.

>
> Also not that on the RZ/A1 platform bit 22 is documented as reserved, so you
> might want to compute the mask based on the CEU model.

While I can use the .data pointer of 'of_device_id' for OF based
devices (RZ) to store the opportune IRQ mask, I'm not sure how to
do that for platform devices. Can I assume (platform data == SH) in
you opinion?

>
> If you have time you could add a debug print when an undocumented interrupt is
> flagged and see if that happens for real.

It's not "undocumented interrupt sources" but "magical interrupt
sources". It's very different :-D

By the way, no, never seen any!

Thanks
   j