Re: [PATCH v14 1/2] drm: add kms driver for loongson display controller

[Sui Jingfeng]

Hi,

On 2023/5/21 20:21, WANG Xuerui wrote:
> > +#ifndef __LSDC_REGS_H__
> > +#define __LSDC_REGS_H__
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/types.h>
> > +
> > +/*
> > + * PIXEL PLL Reference clock
> > + */
> > +#define LSDC_PLL_REF_CLK                100000           /* kHz */
>
> Consider naming it like "LSDC_PLL_REF_CLK_KHZ" for it to be 
> self-documenting?
Indeed, this is really reasonable.  Can be self-documenting.

thanks.

> > +
> > +/*
> > + * Those PLL registers are relative to LSxxxxx_CFG_REG_BASE. xxxxx = 
> > 7A1000,
> > + * 7A2000, 2K2000, 2K1000 etc.
> > + */
> > +
> > +/* LS7A1000 */
> > +
> > +#define LS7A1000_PIXPLL0_REG            0x04B0
> > +#define LS7A1000_PIXPLL1_REG            0x04C0
> > +
> > +/* The DC, GPU, Graphic Memory Controller share the single gfxpll */
> > +#define LS7A1000_PLL_GFX_REG            0x0490
> > +
> > +#define LS7A1000_CONF_REG_BASE          0x10010000
> > +
> > +/* LS7A2000 */
> > +
> > +#define LS7A2000_PIXPLL0_REG            0x04B0
> > +#define LS7A2000_PIXPLL1_REG            0x04C0
> > +
> > +/* The DC, GPU, Graphic Memory Controller share the single gfxpll */
> > +#define LS7A2000_PLL_GFX_REG            0x0490
> > +
> > +#define LS7A2000_CONF_REG_BASE          0x10010000
> > +
> > +/* For LSDC_CRTCx_CFG_REG */
> > +#define CFG_PIX_FMT_MASK                GENMASK(2, 0)
> > +
> > +enum lsdc_pixel_format {
> > +    LSDC_PF_NONE = 0,
> > +    LSDC_PF_XRGB444 = 1,    /* [12 bits] */
> > +    LSDC_PF_XRGB555 = 2,    /* [15 bits] */
> > +    LSDC_PF_XRGB565 = 3,    /* RGB [16 bits] */
> > +    LSDC_PF_XRGB8888 = 4,   /* XRGB [32 bits] */
> > +};
> > +
> > +/*
> > + * Each crtc has two set fb address registers usable, 
> > FB_REG_IN_USING bit of
> > + * LSDC_CRTCx_CFG_REG indicate which fb address register is in using 
> > by the
> > + * CRTC currently. CFG_PAGE_FLIP is used to trigger the switch, the 
> > switching
> > + * will be finished at the very next vblank. Trigger it again if you 
> > want to
> > + * switch back.
> > + *
> > + * If FB0_ADDR_REG is in using, we write the address to FB0_ADDR_REG,
> > + * if FB1_ADDR_REG is in using, we write the address to FB1_ADDR_REG.
> > + */
> > +#define CFG_PAGE_FLIP                   BIT(7)
> > +#define CFG_OUTPUT_ENABLE               BIT(8)
> > +#define CFG_HW_CLONE                    BIT(9)
> > +/* Indicate witch fb addr reg is in using, currently. read only */
> > +#define FB_REG_IN_USING                 BIT(11)
> > +#define CFG_GAMMA_EN                    BIT(12)
> > +
> > +/* The DC get soft reset if this bit changed from "1" to "0", active 
> > low */
> > +#define CFG_RESET_N                     BIT(20)
> > +/* If this bit is set, it say that the CRTC stop working anymore, 
> > anchored. */
> > +#define CRTC_ANCHORED                   BIT(24)
> > +
> > +/*
> > + * The DMA step of the DC in LS7A2000/LS2K2000 is configurable,
> > + * setting those bits on ls7a1000 platform make no effect.
> > + */
> > +#define CFG_DMA_STEP_MASK              GENMASK(17, 16)
> > +#define CFG_DMA_STEP_SHIFT             16
> > +enum lsdc_dma_steps {
> > +    LSDC_DMA_STEP_256_BYTES = 0,
> > +    LSDC_DMA_STEP_128_BYTES = 1,
> > +    LSDC_DMA_STEP_64_BYTES = 2,
> > +    LSDC_DMA_STEP_32_BYTES = 3,
> > +};
> > +
> > +#define CFG_VALID_BITS_MASK             GENMASK(20, 0)
> > +
> > +/* For LSDC_CRTCx_PANEL_CONF_REG */
> > +#define PHY_CLOCK_POL                   BIT(9)
> > +#define PHY_CLOCK_EN                    BIT(8)
> > +#define PHY_DE_POL                      BIT(1)
> > +#define PHY_DATA_EN                     BIT(0)
> > +
> > +/* For LSDC_CRTCx_HSYNC_REG */
> > +#define HSYNC_INV                       BIT(31)
> > +#define HSYNC_EN                        BIT(30)
> > +#define HSYNC_END_MASK                  GENMASK(28, 16)
> > +#define HSYNC_END_SHIFT                 16
> > +#define HSYNC_START_MASK                GENMASK(12, 0)
> > +#define HSYNC_START_SHIFT               0
> > +
> > +/* For LSDC_CRTCx_VSYNC_REG */
> > +#define VSYNC_INV                       BIT(31)
> > +#define VSYNC_EN                        BIT(30)
> > +#define VSYNC_END_MASK                  GENMASK(27, 16)
> > +#define VSYNC_END_SHIFT                 16
> > +#define VSYNC_START_MASK                GENMASK(11, 0)
> > +#define VSYNC_START_SHIFT               0
> > +
> > +/*********** CRTC0 & DISPLAY PIPE0 ***********/
> > +#define LSDC_CRTC0_CFG_REG              0x1240
> > +#define LSDC_CRTC0_FB0_ADDR_LO_REG      0x1260
> > +#define LSDC_CRTC0_FB0_ADDR_HI_REG      0x15A0
> > +#define LSDC_CRTC0_STRIDE_REG           0x1280
> > +#define LSDC_CRTC0_FB_ORIGIN_REG        0x1300
> > +#define LSDC_CRTC0_PANEL_CONF_REG       0x13C0
> > +#define LSDC_CRTC0_HDISPLAY_REG         0x1400
> > +#define LSDC_CRTC0_HSYNC_REG            0x1420
> > +#define LSDC_CRTC0_VDISPLAY_REG         0x1480
> > +#define LSDC_CRTC0_VSYNC_REG            0x14A0
> > +#define LSDC_CRTC0_GAMMA_INDEX_REG      0x14E0
> > +#define LSDC_CRTC0_GAMMA_DATA_REG       0x1500
> > +#define LSDC_CRTC0_FB1_ADDR_LO_REG      0x1580
> > +#define LSDC_CRTC0_FB1_ADDR_HI_REG      0x15C0
> > +
> > +/*********** CTRC1 & DISPLAY PIPE1 ***********/
>
> "CRTC1"

Indeed, thanks for your sharpen eyes.

I will try to solve all other problems you mentioned at next version.

I don't notice this.

Great thanks.