Re: [PATCH v4 07/22] media: camss: Add support for VFE hardware version Titan 170

[Robert Foss <robert.foss@linaro.org>]

On Wed, 10 Feb 2021 at 20:36, Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> Hi Robert,
>
> Thank you for your patch!
>
> In the patchset summary email you are saying that camss does not support
> the PIX interface for this generation of ISPs. But this patch still
> carries quite a lot of code handling the PIX interface. Like:
>
> -----8<-----
> #define         REG_UPDATE_line_n(n)            \
>                         ((n) == VFE_LINE_PIX ? 1 : REG_UPDATE_RDIn(n))
> -----8<-----
>         for (i = VFE_LINE_RDI0; i <= VFE_LINE_PIX; i++)
>                 if (status0 & STATUS_0_line_n_REG_UPDATE(i))
>                         vfe->isr_ops.reg_update(vfe, i);

Thanks, I'll rework these defines for v5.

> -----8<-----
>         switch (f->fmt.pix_mp.pixelformat) {
>         case V4L2_PIX_FMT_NV12:
>         case V4L2_PIX_FMT_NV21:
>         case V4L2_PIX_FMT_NV16:
>         case V4L2_PIX_FMT_NV61:
> -----8<-----
> - the NV pixel formats are only supported for PIX

Ack, cutting this section out for v5.

>
> -----8<-----
>                         if (i == VFE_LINE_PIX) {
>                                 l->formats = formats_pix_845;
>                                 l->nformats = ARRAY_SIZE(formats_pix_845);
>                         } else {
> -----8<-----

Yep, removing this for v5.

> static const struct camss_format_info formats_pix_845[] = {
>         { MEDIA_BUS_FMT_YUYV8_1_5X8, V4L2_PIX_FMT_NV12, 1,
>           { { 1, 1 } }, { { 2, 3 } }, { 8 } },
>         { MEDIA_BUS_FMT_YVYU8_1_5X8, V4L2_PIX_FMT_NV12, 1,
>           { { 1, 1 } }, { { 2, 3 } }, { 8 } },
> ...
> -----8<-----

Removing this for v5.

>
> Guess clearing the PIX related interrupt status bits is correct (provided that
> the driver doesn't try to really process them, and just clears these bits if
> they happen to be set for some reason). But the rest should be removed.
>
> One more comment inline below.
>
> On 05.02.2021 13:43, Robert Foss wrote:
> > Add register definitions for version 170 of the Titan architecture
> > and implement support for the RDI output mode.
> >
> > The RDI mode as opposed to the PIX output mode for the VFE unit does
> > not support any ISP functionality. This means essentially only
> > supporting dumping the output of the whatever the CSI decoder receives
> > from the sensor.
> >
> > For example will a sensor outputting YUV pixel format frames, only
> > allow the VFE to dump those frames as they are received by the ISP
> > to memory through the RDI interface.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > Changes since v1:
> >   - Andrey: Removed commented out chunk
> >   - Remove left over WIP comments
> >
> >
> >   drivers/media/platform/qcom/camss/Makefile    |   1 +
> >   .../media/platform/qcom/camss/camss-vfe-170.c | 805 ++++++++++++++++++
> >   drivers/media/platform/qcom/camss/camss-vfe.c |  59 +-
> >   drivers/media/platform/qcom/camss/camss-vfe.h |  25 +-
> >   .../media/platform/qcom/camss/camss-video.c   | 100 +++
> >   drivers/media/platform/qcom/camss/camss.c     |  61 ++
> >   6 files changed, 1031 insertions(+), 20 deletions(-)
> >   create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-170.c
> >
> > diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> > index 940c0ae3e003..052c4f405fa3 100644
> > --- a/drivers/media/platform/qcom/camss/Makefile
> > +++ b/drivers/media/platform/qcom/camss/Makefile
> > @@ -11,6 +11,7 @@ qcom-camss-objs += \
> >               camss-vfe-4-1.o \
> >               camss-vfe-4-7.o \
> >               camss-vfe-4-8.o \
> > +             camss-vfe-170.o \
> >               camss-vfe-gen1.o \
> >               camss-vfe.o \
> >               camss-video.o \
> > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> > new file mode 100644
> > index 000000000000..b8ac3a137c8a
> > --- /dev/null
> > +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> > @@ -0,0 +1,805 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * camss-vfe-4-7.c
> > + *
> > + * Qualcomm MSM Camera Subsystem - VFE (Video Front End) Module v4.7
> > + *
> > + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> > + * Copyright (C) 2015-2018 Linaro Ltd.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +
> > +#include "camss.h"
> > +#include "camss-vfe.h"
> > +
> > +#define VFE_HW_VERSION                               (0x000)
> > +
> > +#define VFE_GLOBAL_RESET_CMD                 (0x018)
> > +#define              GLOBAL_RESET_CMD_CORE           BIT(0)
> > +#define              GLOBAL_RESET_CMD_CAMIF          BIT(1)
> > +#define              GLOBAL_RESET_CMD_BUS            BIT(2)
> > +#define              GLOBAL_RESET_CMD_BUS_BDG        BIT(3)
> > +#define              GLOBAL_RESET_CMD_REGISTER       BIT(4)
> > +#define              GLOBAL_RESET_CMD_PM             BIT(5)
> > +#define              GLOBAL_RESET_CMD_BUS_MISR       BIT(6)
> > +#define              GLOBAL_RESET_CMD_TESTGEN        BIT(7)
> > +#define              GLOBAL_RESET_CMD_DSP            BIT(8)
> > +#define              GLOBAL_RESET_CMD_IDLE_CGC       BIT(9)
> > +
> > +#define VFE_CORE_CFG                         (0x050)
> > +#define              CFG_PIXEL_PATTERN_YCBYCR        (0x4)
> > +#define              CFG_PIXEL_PATTERN_YCRYCB        (0x5)
> > +#define              CFG_PIXEL_PATTERN_CBYCRY        (0x6)
> > +#define              CFG_PIXEL_PATTERN_CRYCBY        (0x7)
> > +#define              CFG_COMPOSITE_REG_UPDATE_EN     BIT(4)
> > +
> > +#define VFE_IRQ_CMD                          (0x058)
> > +#define              CMD_GLOBAL_CLEAR                BIT(0)
> > +
> > +#define VFE_IRQ_MASK_0                                       (0x05c)
> > +#define              MASK_0_CAMIF_SOF                        BIT(0)
> > +#define              MASK_0_CAMIF_EOF                        BIT(1)
> > +#define              MASK_0_RDIn_REG_UPDATE(n)               BIT((n) + 5)
> > +#define              MASK_0_line_n_REG_UPDATE(n)             \
> > +                     ((n) == VFE_LINE_PIX ? \
> > +                             BIT(4) : MASK_0_RDIn_REG_UPDATE(n))
> > +#define              MASK_0_IMAGE_MASTER_n_PING_PONG(n)      BIT((n) + 8)
> > +#define              MASK_0_IMAGE_COMPOSITE_DONE_n(n)        BIT((n) + 25)
> > +#define              MASK_0_RESET_ACK                        BIT(31)
> > +
> > +#define VFE_IRQ_MASK_1                                       (0x060)
> > +#define              MASK_1_CAMIF_ERROR                      BIT(0)
> > +#define              MASK_1_VIOLATION                        BIT(7)
> > +#define              MASK_1_BUS_BDG_HALT_ACK                 BIT(8)
> > +#define              MASK_1_IMAGE_MASTER_n_BUS_OVERFLOW(n)   BIT((n) + 9)
> > +#define              MASK_1_RDIn_SOF(n)                      BIT((n) + 29)
> > +
> > +#define VFE_IRQ_CLEAR_0                                      (0x064)
> > +#define VFE_IRQ_CLEAR_1                                      (0x068)
> > +
> > +#define VFE_IRQ_STATUS_0                             (0x06c)
> > +#define              STATUS_0_CAMIF_SOF                      BIT(0)
> > +#define              STATUS_0_RDIn_REG_UPDATE(n)             BIT((n) + 5)
> > +#define              STATUS_0_line_n_REG_UPDATE(n)           \
> > +                     ((n) == VFE_LINE_PIX ? \
> > +                             BIT(4) : STATUS_0_RDIn_REG_UPDATE(n))
> > +#define              STATUS_0_IMAGE_MASTER_n_PING_PONG(n)    BIT((n) + 8)
> > +#define              STATUS_0_IMAGE_COMPOSITE_DONE_n(n)      BIT((n) + 25)
> > +#define              STATUS_0_RESET_ACK                      BIT(31)
> > +
> > +#define VFE_IRQ_STATUS_1                             (0x070)
> > +#define              STATUS_1_VIOLATION                      BIT(7)
> > +#define              STATUS_1_BUS_BDG_HALT_ACK               BIT(8)
> > +#define              STATUS_1_RDIn_SOF(n)                    BIT((n) + 27)
> > +
> > +#define VFE_VIOLATION_STATUS                 (0x07c)
> > +
> > +#define VFE_CAMIF_CMD                                (0x478)
> > +#define              CMD_CLEAR_CAMIF_STATUS          BIT(2)
> > +
> > +#define VFE_CAMIF_CFG                                (0x47c)
> > +#define              CFG_VSYNC_SYNC_EDGE             (0)
> > +#define                      VSYNC_ACTIVE_HIGH       (0)
> > +#define                      VSYNC_ACTIVE_LOW        (1)
> > +#define              CFG_HSYNC_SYNC_EDGE             (1)
> > +#define                      HSYNC_ACTIVE_HIGH       (0)
> > +#define                      HSYNC_ACTIVE_LOW        (1)
> > +#define              CFG_VFE_SUBSAMPLE_ENABLE        BIT(4)
> > +#define              CFG_BUS_SUBSAMPLE_ENABLE        BIT(5)
> > +#define              CFG_VFE_OUTPUT_EN               BIT(6)
> > +#define              CFG_BUS_OUTPUT_EN               BIT(7)
> > +#define              CFG_BINNING_EN                  BIT(9)
> > +#define              CFG_FRAME_BASED_EN              BIT(10)
> > +#define              CFG_RAW_CROP_EN                 BIT(22)
> > +
> > +// XXX different, don't exist in TITAN register docs
> > +#define VFE_0_CAMIF_FRAME_CFG                        0x484
> > +#define VFE_0_CAMIF_WINDOW_WIDTH_CFG         0x488
> > +#define VFE_0_CAMIF_WINDOW_HEIGHT_CFG                0x48c
> > +#define VFE_0_CAMIF_SUBSAMPLE_CFG            0x490
> > +#define VFE_0_CAMIF_IRQ_FRAMEDROP_PATTERN    0x498
> > +#define VFE_0_CAMIF_IRQ_SUBSAMPLE_PATTERN    0x49c
> > +#define VFE_0_CAMIF_STATUS                   0x4a4
> > +#define VFE_0_CAMIF_STATUS_HALT                      BIT(31)
> > +#define CAMIF_TIMEOUT_SLEEP_US 1000
> > +#define CAMIF_TIMEOUT_ALL_US 1000000
> > +
> > +#define VFE_REG_UPDATE_CMD                   (0x4ac)
> > +#define              REG_UPDATE_RDIn(n)              BIT(1 + (n))
> > +#define              REG_UPDATE_line_n(n)            \
> > +                     ((n) == VFE_LINE_PIX ? 1 : REG_UPDATE_RDIn(n))
> > +
> > +
> > +#define VFE_BUS_IRQ_MASK(n)          (0x2044 + (n) * 4)
> > +#define VFE_BUS_IRQ_CLEAR(n)         (0x2050 + (n) * 4)
> > +
> > +#define VFE_BUS_IRQ_STATUS(n)                (0x205c + (n) * 4)
> > +#define              STATUS0_COMP_RESET_DONE         BIT(0)
> > +#define              STATUS0_COMP_REG_UPDATE0_DONE   BIT(1)
> > +#define              STATUS0_COMP_REG_UPDATE1_DONE   BIT(2)
> > +#define              STATUS0_COMP_REG_UPDATE2_DONE   BIT(3)
> > +#define              STATUS0_COMP_REG_UPDATE3_DONE   BIT(4)
> > +#define              STATUS0_COMP_REG_UPDATE_DONE(n) BIT(n + 1)
> > +#define              STATUS0_COMP0_BUF_DONE          BIT(5)
> > +#define              STATUS0_COMP1_BUF_DONE          BIT(6)
> > +#define              STATUS0_COMP2_BUF_DONE          BIT(7)
> > +#define              STATUS0_COMP3_BUF_DONE          BIT(8)
> > +#define              STATUS0_COMP4_BUF_DONE          BIT(9)
> > +#define              STATUS0_COMP5_BUF_DONE          BIT(10)
> > +#define              STATUS0_COMP_BUF_DONE(n)        BIT(n + 5)
> > +#define              STATUS0_COMP_ERROR              BIT(11)
> > +#define              STATUS0_COMP_OVERWRITE          BIT(12)
> > +#define              STATUS0_OVERFLOW                BIT(13)
> > +#define              STATUS0_VIOLATION               BIT(14)
> > +/* WM_CLIENT_BUF_DONE defined for buffers 0:19 */
> > +#define              STATUS1_WM_CLIENT_BUF_DONE(n)           BIT(n)
> > +#define              STATUS1_EARLY_DONE                      BIT(24)
> > +#define              STATUS2_DUAL_COMP0_BUF_DONE             BIT(0)
> > +#define              STATUS2_DUAL_COMP1_BUF_DONE             BIT(1)
> > +#define              STATUS2_DUAL_COMP2_BUF_DONE             BIT(2)
> > +#define              STATUS2_DUAL_COMP3_BUF_DONE             BIT(3)
> > +#define              STATUS2_DUAL_COMP4_BUF_DONE             BIT(4)
> > +#define              STATUS2_DUAL_COMP5_BUF_DONE             BIT(5)
> > +#define              STATUS2_DUAL_COMP_BUF_DONE(n)           BIT(n)
> > +#define              STATUS2_DUAL_COMP_ERROR                 BIT(6)
> > +#define              STATUS2_DUAL_COMP_OVERWRITE             BIT(7)
> > +
> > +#define VFE_BUS_IRQ_CLEAR_GLOBAL             (0x2068)
> > +
> > +#define VFE_BUS_WM_DEBUG_STATUS_CFG          (0x226c)
> > +#define              DEBUG_STATUS_CFG_STATUS0(n)     BIT(n)
> > +#define              DEBUG_STATUS_CFG_STATUS1(n)     BIT(8+n)
> > +
> > +#define VFE_BUS_WM_ADDR_SYNC_FRAME_HEADER    (0x2080)
> > +
> > +#define VFE_BUS_WM_ADDR_SYNC_NO_SYNC         (0x2084)
> > +#define              BUS_VER2_MAX_CLIENTS (24)
> > +#define              WM_ADDR_NO_SYNC_DEFAULT_VAL \
> > +                             ((1 << BUS_VER2_MAX_CLIENTS) - 1)
> > +
> > +#define VFE_BUS_WM_CGC_OVERRIDE                      (0x200c)
> > +#define              WM_CGC_OVERRIDE_ALL             (0xFFFFF)
> > +
> > +#define VFE_BUS_WM_TEST_BUS_CTRL             (0x211c)
> > +
> > +#define VFE_BUS_WM_STATUS0(n)                        (0x2200 + (n) * 0x100)
> > +#define VFE_BUS_WM_STATUS1(n)                        (0x2204 + (n) * 0x100)
> > +#define VFE_BUS_WM_CFG(n)                    (0x2208 + (n) * 0x100)
> > +#define              WM_CFG_EN                       (0)
> > +#define              WM_CFG_MODE                     (1)
> > +#define                      MODE_QCOM_PLAIN (0)
> > +#define                      MODE_MIPI_RAW   (1)
> > +#define              WM_CFG_VIRTUALFRAME             (2)
> > +#define VFE_BUS_WM_HEADER_ADDR(n)            (0x220c + (n) * 0x100)
> > +#define VFE_BUS_WM_HEADER_CFG(n)             (0x2210 + (n) * 0x100)
> > +#define VFE_BUS_WM_IMAGE_ADDR(n)             (0x2214 + (n) * 0x100)
> > +#define VFE_BUS_WM_IMAGE_ADDR_OFFSET(n)              (0x2218 + (n) * 0x100)
> > +#define VFE_BUS_WM_BUFFER_WIDTH_CFG(n)               (0x221c + (n) * 0x100)
> > +#define              WM_BUFFER_DEFAULT_WIDTH         (0xFF01)
> > +
> > +#define VFE_BUS_WM_BUFFER_HEIGHT_CFG(n)              (0x2220 + (n) * 0x100)
> > +#define VFE_BUS_WM_PACKER_CFG(n)             (0x2224 + (n) * 0x100)
> > +
> > +#define VFE_BUS_WM_STRIDE(n)                 (0x2228 + (n) * 0x100)
> > +#define              WM_STRIDE_DEFAULT_STRIDE        (0xFF01)
> > +
> > +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(n)   (0x2248 + (n) * 0x100)
> > +#define VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(n)  (0x224c + (n) * 0x100)
> > +#define VFE_BUS_WM_FRAMEDROP_PERIOD(n)               (0x2250 + (n) * 0x100)
> > +#define VFE_BUS_WM_FRAMEDROP_PATTERN(n)              (0x2254 + (n) * 0x100)
> > +#define VFE_BUS_WM_FRAME_INC(n)                      (0x2258 + (n) * 0x100)
> > +#define VFE_BUS_WM_BURST_LIMIT(n)            (0x225c + (n) * 0x100)
> > +
> > +
> > +static void vfe_hw_version_read(struct vfe_device *vfe, struct device *dev)
> > +{
> > +     u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
> > +
> > +     u32 gen = (hw_version >> 28) & 0xF;
> > +     u32 rev = (hw_version >> 16) & 0xFFF;
> > +     u32 step = hw_version & 0xFFFF;
> > +
> > +     dev_err(dev, "VFE HW Version = %u.%u.%u\n", gen, rev, step);
> > +}
> > +
> > +static inline void vfe_reg_clr(struct vfe_device *vfe, u32 reg, u32 clr_bits)
> > +{
> > +     u32 bits = readl_relaxed(vfe->base + reg);
> > +
> > +     writel_relaxed(bits & ~clr_bits, vfe->base + reg);
> > +}
> > +
> > +static inline void vfe_reg_set(struct vfe_device *vfe, u32 reg, u32 set_bits)
> > +{
> > +     u32 bits = readl_relaxed(vfe->base + reg);
> > +
> > +     writel_relaxed(bits | set_bits, vfe->base + reg);
> > +}
> > +
> > +static void vfe_global_reset(struct vfe_device *vfe)
> > +{
>
> reset_bits are written to twice, the second write overwriting the first written value:
>
> > +     u32 reset_bits = GLOBAL_RESET_CMD_IDLE_CGC      |
> > +                      GLOBAL_RESET_CMD_DSP           |
> > +                      GLOBAL_RESET_CMD_TESTGEN       |
> > +                      GLOBAL_RESET_CMD_BUS_MISR      |
> > +                      GLOBAL_RESET_CMD_PM            |
> > +                      GLOBAL_RESET_CMD_REGISTER      |
> > +                      GLOBAL_RESET_CMD_BUS_BDG       |
> > +                      GLOBAL_RESET_CMD_BUS           |
> > +                      GLOBAL_RESET_CMD_CAMIF         |
> > +                      GLOBAL_RESET_CMD_CORE;
>
> - 1st write
>
> > +     reset_bits = 0x00003F9F;
>
> - 2nd write

I found the documentation for these fields, and replaced the 2nd write
by setting all of its bits in the 1st write.
Nice catch!

>
> Thanks,
> Andrey
>
> <snip>