Hi Adam,
On Sat, Jul 16, 2022 at 12:59:35PM -0500, Adam Ford wrote:
> On Mon, Jul 11, 2022 at 7:06 PM Laurent Pinchart wrote:
> >
> > The Image Sensing Interface (ISI) combines image processing pipelines
> > with DMA engines to process and capture frames originating from a
> > variety of sources. The inputs to the ISI go through Pixel Link
> > interfaces, and their number and nature is SoC-dependent. They cover
> > both capture interfaces (MIPI CSI-2 RX, HDMI RX) and memory inputs.
> >
> > Signed-off-by: Christian Hemp <c.hemp@xxxxxxxxx>
> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx>
> > Signed-off-by: Guoniu Zhou <guoniu.zhou@xxxxxxx>
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Stefan Riedmueller <s.riedmueller@xxxxxxxxx>
> > ---
> > MAINTAINERS | 7 +
> > drivers/media/platform/nxp/Kconfig | 2 +
> > drivers/media/platform/nxp/Makefile | 1 +
> > drivers/media/platform/nxp/imx8-isi/Kconfig | 22 +
> > drivers/media/platform/nxp/imx8-isi/Makefile | 9 +
> > .../platform/nxp/imx8-isi/imx8-isi-core.c | 646 +++++++
> > .../platform/nxp/imx8-isi/imx8-isi-core.h | 394 +++++
> > .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 529 ++++++
> > .../platform/nxp/imx8-isi/imx8-isi-debug.c | 109 ++
> > .../media/platform/nxp/imx8-isi/imx8-isi-hw.c | 651 +++++++
> > .../platform/nxp/imx8-isi/imx8-isi-m2m.c | 858 ++++++++++
> > .../platform/nxp/imx8-isi/imx8-isi-pipe.c | 867 ++++++++++
> > .../platform/nxp/imx8-isi/imx8-isi-regs.h | 418 +++++
> > .../platform/nxp/imx8-isi/imx8-isi-video.c | 1513 +++++++++++++++++
> > 14 files changed, 6026 insertions(+)
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/Kconfig
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/Makefile
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-debug.c
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-regs.h
> > create mode 100644 drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
>
> (snip)
>
> Laurent,
>
> Thank you for this series.
>
> > +
> > +/* -----------------------------------------------------------------------------
> > + * IRQ
> > + */
> > +
> > +u32 mxc_isi_channel_irq_status(struct mxc_isi_pipe *pipe, bool clear)
> > +{
> > + u32 status;
> > +
> > + status = mxc_isi_read(pipe, CHNL_STS);
> > + if (clear)
> > + mxc_isi_write(pipe, CHNL_STS, status);
> > +
> > + return status;
> > +}
> > +
> > +void mxc_isi_channel_irq_clear(struct mxc_isi_pipe *pipe)
> > +{
> > + mxc_isi_write(pipe, CHNL_STS, 0xffffffff);
> > +}
> > +
>
> I was reading through the TRM for both Nano and Plus, and it seems
> like there are a few bits in CHNL_STS that are always 0, but we're
> setting them 1. Bit 0-7, 11-15 an 24 all show 0. The TRM says to
> write a 1 to any bit that's set in order to clear it, and
> mxc_isi_channel_irq_status can do this. Why not just have
> mxc_isi_channel_irq_clear call mxc_isi_channel_irq_status(pipe, true)?
> It seems clearer to me than writing a hard-coded hex value. This
> way, it's only clearing any set bits and not arbitrarily writing 1's
> to bit locations that might not be desired..
This is the usual question of how to handle reserved bits, and if taking
shortcuts that simplify the driver implementation is acceptable. I've
heard various arguments related to this topic over time, ranging from
strict compliance with the datasheet for fear of unwanted behaviour, to
forward-compatibility with newer IP versions that may use those reserved
bits.
In practice, true reserved bits are very likely to be ignored by the
hardware when written, so it shouldn't matter much, and
forward-compatibility is often more of a theoretical argument as a newer
IP version that includes changes to registers will most likely need
driver updates anyway. However, bits documented as reserved may do
something undocumented, so without feedback from the vendor that
reserved bits are safe to be written, it's hard to be 100% sure.
In this specific case, I don't think a simple write(read()) would be the
best idea, as it will be more costly due to the additional read. It's
also not clear what happens when the read-only bits of the CHNL_STS
register do when written, I assume writes are just ignored, but if
you're concerned about writing to arbitrary bits, one could also argue
that writing 1's to read-only locations could be a problem.
We could replace the above with
mxc_isi_write(pipe, CHNL_STS, 0xfeff0000);
Would that handle your concern ? Note that the i.MX8 DualX, DualXPlus
and QuadXPlus include an ISI that has a different layout for the
CHNL_STS register, with "clear on write" interrupt bits in [31:14], so
the function would need to be adapted when supporting that SoC. One
option would be to write
mxc_isi_write(pipe, CHNL_STS, 0xffffc000);
on the assumption that bits 14, 15 and 24 are really not implemented in
i.MX8MM and i.MX8MP.
Xavier, do you have any preference ? Is there any rule followed by NXP
for this ?
> (snip)
--
Regards,
Laurent Pinchart
