Re: [PATCH v9 1/5] drm/msm/hdmi: switch to generic PHY subsystem

Re: [PATCH v9 1/5] drm/msm/hdmi: switch to generic PHY subsystem

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Resource Mapping Mismatch and DT ABI Breakage
- [High] Missing 'xo' Clock Initialization for MSM8998
- [High] Unbalanced Operations and Framework State Corruption on Early Return
- [High] Integer Overflow on 32-bit Architectures in PLL Calculation
- [High] Illegal Sleeping in Atomic Context
- [High] Missing Hardware Reset for MSM8x60
- [High] Unpowered Register Access in Clock Callbacks
- [Low] PM Callback Conditional Compilation Violation
- [Low] Missing GENERIC_PHY selection in Kconfig
--

(Note: Due to system recitation restrictions, this review uses a free-form
summary instead of inline patch quoting.)

commit 128cc7cf2eaed12c44a5fdd2ac7d194176f30165
Author: Dmitry Baryshkov <lumag@kernel.org>
Subject: drm/msm/hdmi: switch to generic PHY subsystem

This commit changes the MSM HDMI driver to use the generic PHY subsystem.
It moves the PHY drivers to the drivers/phy/qualcomm/ directory to allow
better code sharing.

In the new qmp_hdmi_probe function in phy-qcom-qmp-hdmi-base.c, memory
resources are fetched by index, using 0 for serdes and 5 for phy_reg. Since
existing device trees like qcom,hdmi-phy-8996 map hdmi_phy at index 0 and
hdmi_pll at index 1, does this swap the PHY and PLL register bases and
break the ABI?

Similarly, in phy-qcom-hdmi-preqmp.c, qcom_hdmi_preqmp_probe unconditionally
maps index 1 for pll_reg. Will this cause the probe to fail completely on
platforms like MSM8974 and MSM8x60 whose device trees only define a single
memory resource? Could devm_platform_ioremap_resource_byname be used here?

The new generic driver sets MAX_CLKS to 2 and only retrieves the iface and
ref clocks. The original hdmi_phy_8998.c explicitly retrieved and enabled a
third clock, xo. Without the board crystal oscillator enabled, won't this
lead to PLL lock timeouts and hardware hangs when powering on the MSM8998
PHY?

In msm_hdmi_bridge_atomic_pre_enable, if phy_power_on fails, the code
unlocks the mutex and returns early.

Since this skips clk_prepare_enable for extp_clk, but the disable path
unconditionally calls clk_disable_unprepare, could this cause an unbalanced
clock disable warning?

Additionally, the return value of phy_init is ignored. If phy_init fails,
the PHY framework leaves init_count at 0. Later, phy_exit in the disable
path will execute --phy->init_count, underflowing it to -1. Won't the next
modeset attempt see init_count equal to -1, skip the init callback, and then
cause a synchronous external abort by writing to unpowered registers in
phy_power_on?

In phy-qcom-qmp-hdmi-msm8998.c, qmp_hdmi_8998_pll_get_post_div calculates
rvar1 using HDMI_DEFAULT_REF_CLOCK * rng1 * HZ_PER_MHZ. On 32-bit
architectures, doesn't this overflow the 32-bit unsigned long arithmetic
since the product is roughly 307.2 trillion? The previous driver cast
HDMI_DEFAULT_REF_CLOCK to u64 to prevent this.

The patch replaces udelay loops with readl_poll_timeout in several PHY
power-on functions, and adds usleep_range in power-off functions.

Since these operations are called from the DRM bridge atomic pre-enable
and post-disable callbacks, which run in atomic context, will these sleeping
functions cause kernel warnings and system instability? Should we use
readl_poll_timeout_atomic instead?

In phy-qcom-hdmi-45nm.c, qcom_hdmi_msm8x60_phy_power_off omits the software
reset sequence (REG_HDMI_PHY_CTRL) that was present in the original driver.
Because this register resides in the HDMI controller space, the new PHY
driver does not access it. Does omitting this reset leave the PHY in a hung
or inconsistent state across power cycles?

In the clock framework callbacks like qmp_hdmi_899x_pll_recalc_rate, the
code directly reads PHY MMIO registers. Because the common clock framework
can invoke this callback at any time (for example, reading debugfs), can
this trigger an immediate bus timeout or external abort if the PHY is
runtime-suspended and unclocked?

In the platform driver definitions for the new PHY drivers, the pm field is
assigned directly to the pm_ops structures. This isn't a bug, but should
pm_ptr() be used here to avoid dead code retention when PM is disabled?

The new PHY_QCOM_QMP_HDMI configuration uses generic PHY framework APIs like
devm_phy_create, but its Kconfig definition does not select GENERIC_PHY.
If the framework is disabled, will these APIs be stubbed and cause the
driver to silently fail with -ENOSYS during probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-fd-hdmi-phy-v9-0-ca98c72f1f9f@oss.qualcomm.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy