Re: [PATCH v4 4/6] drm/verisilicon: add DC8000 (DCUltraLite) display controller support

[Icenowy Zheng]

(CC'ed clk maintainers for weird clock gate bit)

在 2026-06-17三的 18:35 +0800，Joey Lu写道：
> 
> On 6/15/2026 4:51 PM, Icenowy Zheng wrote:
> > 在 2026-06-15一的 14:50 +0800，Joey Lu写道：
> > > The Nuvoton MA35D1 SoC integrates a Verisilicon DCUltraLite
> > > display
> > > controller whose register layout differs from the DC8200 in
> > > several
> > > important ways:
> > > 
> > > 1. No CONFIG_EX commit path: framebuffer updates use the enable
> > > (bit
> > > 0)
> > >     and reset (bit 4) bits in FB_CONFIG instead of the DC8200
> > > staging
> > >     registers (FB_CONFIG_EX, FB_TOP_LEFT, FB_BOTTOM_RIGHT,
> > >     FB_BLEND_CONFIG, PANEL_CONFIG_EX).
> > > 
> > > 2. No PANEL_START register: panel output starts when
> > >     PANEL_CONFIG.RUNNING is set; there is no multi-display sync
> > > start
> > >     register.
> > > 
> > > 3. Different IRQ registers: DCUltraLite uses DISP_IRQ_STA
> > > (0x147C) /
> > >     DISP_IRQ_EN (0x1480) versus DC8200's TOP_IRQ_ACK (0x0010) /
> > >     TOP_IRQ_EN (0x0014).
> > > 
> > > 4. Per-frame commit cycle: DCUltraLite requires the VALID bit in
> > >     FB_CONFIG to be set at the start of each atomic commit
> > > (crtc_begin)
> > >     and cleared after (crtc_flush).
> > > 
> > > 5. Simpler clock topology: only 'core' (bus gate) and 'pix0'
> > > (pixel
> > >     divider) clocks; no axi or ahb clocks required.  Make axi_clk
> > > and
> > >     ahb_clk optional (devm_clk_get_optional_enabled) so DC8000
> > > nodes
> > >     without those clocks are handled gracefully.
> > > 
> > > Add vs_dc8000.c implementing the vs_dc_funcs vtable for the above
> > > differences.  The probe now selects vs_dc8000_funcs when the
> > > identified
> > > generation is VSDC_GEN_DC8000 (DCUltraLite reads model 0x0,
> > > revision 0x5560, customer_id 0x305).
> > > 
> > > Signed-off-by: Joey Lu <a0987203069@xxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/verisilicon/Makefile    |  2 +-
> > >   drivers/gpu/drm/verisilicon/vs_dc.c     |  9 ++-
> > >   drivers/gpu/drm/verisilicon/vs_dc.h     |  1 +
> > >   drivers/gpu/drm/verisilicon/vs_dc8000.c | 78
> > > +++++++++++++++++++++++++
> > >   4 files changed, 86 insertions(+), 4 deletions(-)
> > >   create mode 100644 drivers/gpu/drm/verisilicon/vs_dc8000.c
> > > 
> > > diff --git a/drivers/gpu/drm/verisilicon/Makefile
> > > b/drivers/gpu/drm/verisilicon/Makefile
> > > index 9d4cd16452fa..d2fd8e4dff24 100644
> > > --- a/drivers/gpu/drm/verisilicon/Makefile
> > > +++ b/drivers/gpu/drm/verisilicon/Makefile
> > > @@ -1,6 +1,6 @@
> > >   # SPDX-License-Identifier: GPL-2.0-only
> > >   
> > > -verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o
> > > vs_drm.o vs_hwdb.o \
> > > +verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_dc8200.o
> > > vs_dc8000.o vs_drm.o vs_hwdb.o \
> > >   	vs_plane.o vs_primary_plane.o vs_cursor_plane.o
> > >   
> > >   obj-$(CONFIG_DRM_VERISILICON_DC) += verisilicon-dc.o
> > > diff --git a/drivers/gpu/drm/verisilicon/vs_dc.c
> > > b/drivers/gpu/drm/verisilicon/vs_dc.c
> > > index 9729b693d360..9499fffbca58 100644
> > > --- a/drivers/gpu/drm/verisilicon/vs_dc.c
> > > +++ b/drivers/gpu/drm/verisilicon/vs_dc.c
> > > @@ -90,13 +90,13 @@ static int vs_dc_probe(struct platform_device
> > > *pdev)
> > >   		return PTR_ERR(dc->core_clk);
> > >   	}
> > >   
> > > -	dc->axi_clk = devm_clk_get_enabled(dev, "axi");
> > > +	dc->axi_clk = devm_clk_get_optional_enabled(dev, "axi");
> > >   	if (IS_ERR(dc->axi_clk)) {
> > >   		dev_err(dev, "can't get axi clock\n");
> > >   		return PTR_ERR(dc->axi_clk);
> > >   	}
> > >   
> > > -	dc->ahb_clk = devm_clk_get_enabled(dev, "ahb");
> > > +	dc->ahb_clk = devm_clk_get_optional_enabled(dev, "ahb");
> > Please make the clock change a separated patch for atomicity.
> > 
> > BTW the MA35D1 manual's clock tree shows that DCUltra appears on
> > AXI2
> > ACLK, AHB_HCLK2, behind a mux of SYS-PLL/EPLL-DIV2 (which seems to
> > be
> > the core clock), and behind a divider (which seems to be the pixel
> > clock).
> > 
> > However it's weird that only one DCUltra Clock Enable Bit exists
> > despite both bus clocks have "ICG" (I think it means "Integrated
> > Clock
> > Gating"). In addition the linux clk-ma35d1 driver assigns
> > "dcu_gate" as
> > a downstream of "dcu_mux", although the Figure 6.5-2 in the TRM
> > shows
> > no ICG after the "Display core CLK" mux.
> > 
> > Is the two bus clocks controlled by a single gate bit, and is the
> > bit
> > also gating DC core clock?
> > 
> > Thanks,
> > Icenowy
> I will split the axi/ahb optional-clock change into its own patch in
> v5 
> for atomicity.
> Regarding the MA35D1 clock tree: from the TRM, the single "dcu_gate"
> bit 
> gates both bus clocks (AXI ACLK and AHB HCLK) together with the
> display 
> core clock through the same ICG cell. The clk-ma35d1 driver exposes
> only 
> "dcu_gate" (downstream of "dcu_mux") and does not provide separate 

Then it's one of the case that the clock tree doesn't properly
represent the hardware, which is bad. However, as three gates share the
same bit, I am not sure how to represent such kind of thing in the
common clk framework.

> axi/ahb clock entries. Therefore the MA35D1 DT binding will use only
> two 
> clocks ("core" and "pix0"); making axi and ahb optional in the driver
> is the correct approach, and this will be stated clearly in the
>  split-out patch.

I agree to make them optional, although these two clocks do exist in
the hardware of MA35D1.

Thanks,
Icenowy

> > >   	if (IS_ERR(dc->ahb_clk)) {
> > >   		dev_err(dev, "can't get ahb clock\n");
> > >   		return PTR_ERR(dc->ahb_clk);