MIME-Version: 1.0
References: <20220107145515.613009-1-narmstrong@baylibre.com> <20220107145515.613009-4-narmstrong@baylibre.com>
In-Reply-To: <20220107145515.613009-4-narmstrong@baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Fri, 7 Jan 2022 23:33:14 +0100
Message-ID: <CAFBinCA-df8Sedqh8Arh_BsMCHYv6-Kb3owrkFBd=W_EY2qxSA@mail.gmail.com>
Subject: Re: [PATCH 3/6] drm/meson: venc: add ENCL encoder setup for MIPI-DSI output
To:     Neil Armstrong <narmstrong@baylibre.com>
Cc:     dri-devel@lists.freedesktop.org, linux-amlogic@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

 Hi Neil,

On Fri, Jan 7, 2022 at 3:57 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> This adds supports for the ENCL encoder connected to a MIPI-DSI transceiver on the
> Amlogic AXG SoCs.
Should this be "AXG and newer SoCs" or is this really AXG specific?

[...]
> +#define GAMMA_VCOM_POL    7     /* RW */
> +#define GAMMA_RVS_OUT     6     /* RW */
> +#define ADR_RDY           5     /* Read Only */
> +#define WR_RDY            4     /* Read Only */
> +#define RD_RDY            3     /* Read Only */
> +#define GAMMA_TR          2     /* RW */
> +#define GAMMA_SET         1     /* RW */
> +#define GAMMA_EN          0     /* RW */
> +
> +#define H_RD              12
> +#define H_AUTO_INC        11
> +#define H_SEL_R           10
> +#define H_SEL_G           9
> +#define H_SEL_B           8
I think all values above can be wrapped in the BIT() macro, then you
don't need that below.

> +#define HADR_MSB          7            /* 7:0 */
> +#define HADR              0            /* 7:0 */
Here GENMASK(7, 0) can be used for HADR

Also I think prefixing all macros above with their register name
(L_GAMMA_CNTL_PORT_ or L_GAMMA_ADDR_PORT_) will make the code easier
to read.

[...]
> +       writel_relaxed(0x8000, priv->io_base + _REG(ENCL_VIDEO_MODE));
The public S905 datasheet calls 0x8000 ENCL_PX_LN_CNT_SHADOW_EN

> +       writel_relaxed(0x0418, priv->io_base + _REG(ENCL_VIDEO_MODE_ADV));
According to the public S905 datasheet this is:
- BIT(3): ENCL_VIDEO_MODE_ADV_VFIFO_EN
- BIT(4): ENCL_VIDEO_MODE_ADV_GAIN_HDTV
- BIT(10): ENCL_SEL_GAMMA_RGB_IN

> +       writel_relaxed(0x1000, priv->io_base + _REG(ENCL_VIDEO_FILT_CTRL));
I don't know the exact name but the 32-bit vendor kernel sources have
a comment [0] saying that 0x1000 is "bypass filter"
But maybe we can simply call it ENCL_VIDEO_FILT_CTRL_BYPASS_FILTER

[...]
> +       writel_relaxed(3, priv->io_base + _REG(ENCL_VIDEO_RGBIN_CTRL));
The public S905 datasheet says:
- BIT(0): USE RGB data from VIU, furthermore a comment in the 3.10
kernel sources make this more clear: bit[0] 1:RGB, 0:YUV
- BIT(1): CFG_VIDEO_RGBIN_ZBLK

> +       /* default black pattern */
> +       writel_relaxed(0, priv->io_base + _REG(ENCL_TST_MDSEL));
> +       writel_relaxed(0, priv->io_base + _REG(ENCL_TST_Y));
> +       writel_relaxed(0, priv->io_base + _REG(ENCL_TST_CB));
> +       writel_relaxed(0, priv->io_base + _REG(ENCL_TST_CR));
> +       writel_relaxed(1, priv->io_base + _REG(ENCL_TST_EN));
> +       writel_bits_relaxed(BIT(3), 0, priv->io_base + _REG(ENCL_VIDEO_MODE_ADV));
same as above: ENCL_VIDEO_MODE_ADV_VFIFO_EN

> +
> +       writel_relaxed(1, priv->io_base + _REG(ENCL_VIDEO_EN));
> +
> +       writel_relaxed(0, priv->io_base + _REG(L_RGB_BASE_ADDR));
> +       writel_relaxed(0x400, priv->io_base + _REG(L_RGB_COEFF_ADDR));
note to self: L_RGB_COEFF_ADDR seems to contain some "magic" value,
there's no further info in the 3.10 kernel sources or datasheet

> +       writel_relaxed(0x400, priv->io_base + _REG(L_DITH_CNTL_ADDR));
According to the public S905 datasheet BIT(10) is DITH10_EN (10-bits
Dithering to 8 Bits Enable).
I am not sure if this would belong to the selected video mode/bit depth.
I'll let other reviewers decide if this is relevant or not because I don't know.

[...]
> +       writel_relaxed(0, priv->io_base + _REG(L_INV_CNT_ADDR));
> +       writel_relaxed(BIT(4) | BIT(5),
> +                      priv->io_base + _REG(L_TCON_MISC_SEL_ADDR));
the public S905 datasheet states:
- BIT(4): STV1_SEL (STV1 is frame Signal)
- BIT(5): STV2_SEL (STV2 is frame Signal)
This doesn't seem helpful to me though, but maybe you can still create
preprocessor macros for this (for consistency)?

[...]
> +       switch (priv->venc.current_mode) {
> +       case MESON_VENC_MODE_MIPI_DSI:
> +               writel_relaxed(0x200,
> +                              priv->io_base + _REG(VENC_INTCTRL));
the public S905 datasheet documents this as:
- BIT(9): ENCP_LNRST_INT_EN (Progressive encoder filed change interrupt enable)
Please add a preprocessor macro to make it consistent with
VENC_INTCTRL_ENCI_LNRST_INT_EN which already exists and is used below.


Best regards,
Martin