Hi Joel,

Thanks for your comment. I will modify the patch and send it again.

-----Original Message----- From: Joel Stanley joel@jms.id.au Sent: Tuesday, November 16, 2021 2:19 PM To: Tommy Huang tommy_huang@aspeedtech.com Cc: David Airlie airlied@linux.ie; Daniel Vetter daniel@ffwll.ch; Rob Herring robh+dt@kernel.org; Andrew Jeffery andrew@aj.id.au; linux-aspeed linux-aspeed@lists.ozlabs.org; open list:DRM DRIVERS dri-devel@lists.freedesktop.org; devicetree devicetree@vger.kernel.org; Linux ARM linux-arm-kernel@lists.infradead.org; Linux Kernel Mailing List linux-kernel@vger.kernel.org; BMC-SW BMC-SW@aspeedtech.com Subject: Re: [PATCH 3/4] drm/aspeed: Update INTR_STS handling

On Mon, 1 Nov 2021 at 11:01, tommy-huang tommy_huang@aspeedtech.com wrote:

...

The V-sync INTR_STS is differnet on AST2600.
Change into general rule to handle it.

Signed-off-by: tommy-huang tommy_huang@aspeedtech.com

drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 ++ drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 26 ++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h b/drivers/gpu/drm/aspeed/aspeed_gfx.h index 96501152bafa..5eed9275bce7 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx.h +++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h @@ -12,6 +12,8 @@ struct aspeed_gfx { struct regmap *scu;

    u32                             dac_reg;

•   u32                             int_reg;
  

•   u32                             int_clr_reg;
    u32                             vga_scratch_reg;
    u32                             throd_val;
    u32                             scan_line_max;
  

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index b53fee6f1c17..1092060cb59c 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -60,6 +60,8 @@

struct aspeed_gfx_config { u32 dac_reg; /* DAC register in SCU */

•   u32 int_status_reg;     /* Interrupt status register */
  

This is the same for all supported chips; do you need to introduce the variable for it?

...

•   u32 int_clear_reg;      /* Interrupt clear register */
    u32 vga_scratch_reg;    /* VGA scratch register in SCU */
    u32 throd_val;          /* Default Threshold Seting */
    u32 scan_line_max;      /* Max memory size of one scan line */
  

@@ -67,6 +69,8 @@ struct aspeed_gfx_config {

static const struct aspeed_gfx_config ast2400_config = { .dac_reg = 0x2c,

•   .int_status_reg = 0x60,
  

•   .int_clear_reg = 0x60,
    .vga_scratch_reg = 0x50,
    .throd_val = CRT_THROD_LOW(0x1e) | CRT_THROD_HIGH(0x12),
    .scan_line_max = 64,
  

@@ -74,14 +78,26 @@ static const struct aspeed_gfx_config ast2400_config = {

static const struct aspeed_gfx_config ast2500_config = { .dac_reg = 0x2c,

•   .int_status_reg = 0x60,
  

•   .int_clear_reg = 0x60,
    .vga_scratch_reg = 0x50,
    .throd_val = CRT_THROD_LOW(0x24) | CRT_THROD_HIGH(0x3c),
    .scan_line_max = 128,
  

};

+static const struct aspeed_gfx_config ast2600_config = {

•   .dac_reg = 0xc0,
  

•   .int_status_reg = 0x60,
  

•   .int_clear_reg = 0x68,
  

•   .vga_scratch_reg = 0x50,
  

•   .throd_val = CRT_THROD_LOW(0x50) | CRT_THROD_HIGH(0x70),
  

•   .scan_line_max = 128,
  

+};

This patch combines adding the clear_reg functionality with support for the 2600. Can you split them out; add int_clear_reg first, and then add support for the 2600 in the following patch?

...

static const struct of_device_id aspeed_gfx_match[] = { { .compatible = "aspeed,ast2400-gfx", .data = &ast2400_config }, { .compatible = "aspeed,ast2500-gfx", .data = &ast2500_config },

•   { .compatible = "aspeed,ast2600-gfx", .data = &ast2600_config
  

• }, { },

}; MODULE_DEVICE_TABLE(of, aspeed_gfx_match); @@ -113,13 +129,15 @@ static irqreturn_t aspeed_gfx_irq_handler(int irq, void *data) { struct drm_device *drm = data; struct aspeed_gfx *priv = to_aspeed_gfx(drm);

•   u32 reg;
  

•   u32 reg, clr_reg;
  
  

•   reg = readl(priv->base + CRT_CTRL1);
  

•   reg = readl(priv->base + priv->int_reg);
  
    if (reg & CRT_CTRL_VERTICAL_INTR_STS) {
            drm_crtc_handle_vblank(&priv->pipe.crtc);
  

•           writel(reg, priv->base + CRT_CTRL1);
  

•           clr_reg = (readl(priv->base + priv->int_clr_reg) |
  

•                   CRT_CTRL_VERTICAL_INTR_STS);
  

•           writel(clr_reg, priv->base + priv->int_clr_reg);
  

You do not need to do read-modify-write.

On the 2500 you're writing back the value you read. On the 2600 all of the bits are read only except the INTR_STS bit, which is W1C, so you can use the value you read from CRT_CTRL1, so this should be enough:

writel(reg, priv->base + priv->int_clr_reg);

...

            return IRQ_HANDLED;
    }

@@ -147,6 +165,8 @@ static int aspeed_gfx_load(struct drm_device *drm) config = match->data;

    priv->dac_reg = config->dac_reg;

•   priv->int_reg = config->int_status_reg;
  

•   priv->int_clr_reg = config->int_clear_reg;
    priv->vga_scratch_reg = config->vga_scratch_reg;
    priv->throd_val = config->throd_val;
    priv->scan_line_max = config->scan_line_max;
  

-- 2.17.1

From: Joel Stanley joel@jms.id.au

Enable the GFX device with a framebuffer memory region.

Signed-off-by: Joel Stanley joel@jms.id.au Signed-off-by: tommy-huang tommy_huang@aspeedtech.com --- arch/arm/boot/dts/aspeed-ast2600-evb.dts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts index b7eb552640cb..e223dad2abd0 100644 --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts @@ -23,6 +23,19 @@ reg = <0x80000000 0x80000000>; };

+ reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + + gfx_memory: framebuffer { + size = <0x01000000>; + alignment = <0x01000000>; + compatible = "shared-dma-pool"; + reusable; + }; + }; + vcc_sdhci0: regulator-vcc-sdhci0 { compatible = "regulator-fixed"; regulator-name = "SDHCI0 Vcc"; @@ -300,3 +313,8 @@ vqmmc-supply = <&vccq_sdhci1>; clk-phase-sd-hs = <7>, <200>; }; + +&gfx { + status = "okay"; + memory-region = <&gfx_memory>; +};