Re: [PATCH v3 14/19] drm/bridge: starfive: Add JH7110 HDMI controller driver

[Michal Wilczynski]

On 9/4/26 15:39, Icenowy Zheng wrote:
> 在 2026-09-04五的 15:27 +0200，Michal Wilczynski写道：
>> Add the HDMI controller (bridge) driver for the StarFive JH7110.
>>
>> This driver binds to the starfive,jh7110-inno-hdmi-controller node.
>> It gets its shared regmap from its parent and its register access,
>> module and bus clocks from voutcrg. It consumes the pixel clock and
>> the
>> PHY from its hdmi_phy sibling.
>>
>> The driver calls the generic inno_hdmi_probe function and passes the
>> shared regmap to it, registering as a DRM bridge. The .enable hook is
>> responsible for setting the PHY's pixel clock rate via clk_set_rate()
>> and powering on the PHY via phy_power_on().
>>
>> The DC8200 has two panels, each exposing a DP and a DPI interface,
>> and a
>> mux in the video output system controller picks which of them drives
>> the
>> HDMI transmitter. Program that mux from the port graph rather than
>> relying on whatever the bootloader left behind, taking the panel from
>> the
>> remote port number and the interface from the remote endpoint number.
>>
>> The generic driver holds the clock it looks up as the register access
>> clock enabled for its lifetime, and derives the DDC divider from that
>> clock's rate, so point it at the system clock. Naming the pixel clock
>> there instead would keep the PHY pre-PLL powered from probe onwards
>> and
>> size the divider from the wrong rate.
>>
>> The PHY can only generate the discrete set of pixel clocks described
>> by
>> its pre-PLL table, so .mode_valid rejects any mode clk_round_rate()
>> cannot satisfy. Without it such a mode would be advertised to
>> userspace
>> and the modeset would appear to succeed while the display stayed
>> blank.
>>
>> .enable returns early when the rate is unsupported or the PHY fails
>> to
>> power on, so track whether the pixel clock was actually enabled and
>> let
>> .disable tear down only what was brought up, otherwise the clock
>> refcount underflows.
>>
>> The clocks and the reset are torn down through devm rather than from
>> .remove, so that they outlive the bridge that inno_hdmi_probe() adds
>> with
>> devm_drm_bridge_add(). Releasing them in .remove runs before devres
>> unwinds and would leave the bridge registered with its clocks already
>> gated.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig            |  11 ++
>>  drivers/gpu/drm/bridge/Makefile           |   1 +
>>  drivers/gpu/drm/bridge/jh7110-inno-hdmi.c | 318
>> ++++++++++++++++++++++++++++++
>>  3 files changed, 330 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>> b/drivers/gpu/drm/bridge/Kconfig
>> index
>> 4a57d49b4c6d3ab4b965228835b372d191647197..75b1cf6727d5a32310dcf9fe573
>> 4d95e14eea8fe 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig

[snip]

>> +
>> +	/* Data mapping: 8-bit RGB on whichever interface is in use.
>> */
>> +	mask = VOUT_HDMI_DPI_DP_SEL | VOUT_HDMI_DP_BIT_DEPTH |
>> +	       VOUT_HDMI_DP_YUV_MODE | VOUT_HDMI_DPI_BIT_DEPTH;
>> +	val = FIELD_PREP(VOUT_HDMI_DPI_DP_SEL, endpoint.id) |
>> +	      FIELD_PREP(VOUT_HDMI_DP_YUV_MODE,
>> VOUT_HDMI_DP_YUV_MODE_RGB) |
>> +	      FIELD_PREP(VOUT_HDMI_DPI_BIT_DEPTH,
>> VOUT_HDMI_DPI_BIT_DEPTH_8BIT);
> 
> Well it looks like the vendor driver never sets DP interface, is it
> tested? I doubt whether the SoC designer messed it up.

The DP interface is documented in the TRM and I've also tested it so it
does work indeed. Here is my test branch if you would like to see for
youself.

[1] - https://github.com/mwilczy/linux/commit/7efcba3b0be30734087c79f5773cae19245924db


[snip]

> 
>> +}
>> +
>> +/*
>> + * This table is now only used for the generic .mode_valid check.
>> + * The real validation happens in the PHY driver's .round_rate.
>> + */
>> +static struct inno_hdmi_phy_config stf_hdmi_phy_configs[] = {
>> +	{ 297000000, 0x00, 0x00 },
>> +	{ ~0UL, 0x00, 0x00 }, /* Sentinel */
>> +};
> 
> If it's just such a upper bound, why don't just override the function
> as a bound check?

Yeah actually patch 10 implements mode_valid callback, so it's better to
remove this stub altogether, previously this was done satisfy the probe
check reasons - v4 will add patch that makes this table optional in
inno-hdmi driver.

> 
> Or... should the real table be used here? I start to wonder whether
> this is related to Maud's failure on the Framework panel.

Maud panel needed another entry in PHY pre-PLL table. Patch 17 in v3
adds this entry.

There is a new issue reported by Maud, I will answer that in a separate
thread.

> 
> Thanks,
> Icenowy
> 
>> +
>> +static const struct inno_hdmi_plat_ops stf_inno_hdmi_plat_ops = {
>> +	.enable = inno_hdmi_starfive_enable,
>> +	.disable = inno_hdmi_starfive_disable,
>> +	.mode_valid = inno_hdmi_starfive_mode_valid,
>> +};
>> +
>> +static const struct inno_hdmi_plat_data stf_inno_hdmi_plat_data = {
>> +	.ops = &stf_inno_hdmi_plat_ops,
>> +	.phy_configs = stf_hdmi_phy_configs,
>> +	.default_phy_config = &stf_hdmi_phy_configs[0],
>> +};
>> +
>> +static const struct of_device_id starfive_hdmi_controller_dt_ids[] =
>> {
>> +	{ .compatible = "starfive,jh7110-inno-hdmi-controller",
>> +	  .data = &stf_inno_hdmi_plat_data },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, starfive_hdmi_controller_dt_ids);
>> +
>> +struct platform_driver starfive_inno_hdmi_controller_driver = {
>> +	.probe = starfive_inno_hdmi_controller_probe,
>> +	.driver = {
>> +		.name = "starfive-inno-hdmi-controller",
>> +		.of_match_table = starfive_hdmi_controller_dt_ids,
>> +	},
>> +};
>> +module_platform_driver(starfive_inno_hdmi_controller_driver);
>> +
>> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("StarFive INNO HDMI Controller Driver");
>> +MODULE_LICENSE("GPL");
> 
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>