Re: [PATCH 2/7] phy: mediatek: hdmi: Add support for mt8167

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: determine_rate callback fails to propagate best_parent_rate and mutates persistent state
- [Medium] clk: phy-mtk-hdmi: recalc_rate returns internal rate instead of logical TMDS clock
--

commit c9a6701a365f2268fb32f51141821ccc937de091
Author: Luca Leonardo Scorcia <l.scorcia@gmail.com>

phy: mediatek: hdmi: Add support for mt8167
    
Add support for the HDMI PHY found in MediaTek mt8167.

> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8167.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8167.c
> new file mode 100644
> index 0000000000000..2435054d4e176
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8167.c

[ ... ]

> +static int mtk_hdmi_pll_determine_rate(struct clk_hw *hw,
> +				       struct clk_rate_request *req)
> +{
> +	struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw);
> +
> +	hdmi_phy->pll_rate = req->rate;
> +
> +	return 0;
> +}

[Severity: High]
Does this callback fail to calculate and set req->best_parent_rate?

This clock is registered with the CLK_SET_RATE_PARENT flag set. Failing to
update req->best_parent_rate means the parent clock's rate might never be
adjusted to support required pixel clock frequencies, such as 148.5MHz or
108MHz. The hardware dividers configured in set_rate would be applied to an
unscaled parent frequency, yielding an incorrect HDMI pixel clock.

Also, since determine_rate is a dry-run CCF query callback (e.g., invoked by
clk_round_rate), is it safe to mutate persistent driver state
(hdmi_phy->pll_rate) here?

[ ... ]

> +static unsigned long mtk_hdmi_pll_recalc_rate(struct clk_hw *hw,
> +					      unsigned long parent_rate)
> +{
> +	struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw);
> +
> +	unsigned long out_rate, val;
> +	u32 tmp;

[ ... ]

> +	tmp = readl(hdmi_phy->regs + HDMI_CON2);
> +	val = FIELD_GET(RG_HDMITX_TX_POSDIV, tmp);
> +	out_rate >>= val;
> +
> +	if (tmp & RG_HDMITX_EN_TX_POSDIV)
> +		out_rate /= 5;
> +
> +	return out_rate;
> +}

[Severity: Medium]
Does this mathematically compute the internal PHY output rate instead of the
logical TMDS pixel clock requested by the HDMI controller?

It appears this calculates a fraction of the TMDS clock (e.g., 37.125MHz for a
74.25MHz clock). Since the HDMI controller driver requests the TMDS pixel clock
rate, returning the internal rate could lead to CCF rate mismatches and
warnings like "Want PLL 74250000 Hz, got 37125000 Hz".

Combined with mtk_hdmi_pll_determine_rate() failing to configure the
appropriate scaled rate for the parent clock, could this cement the framework
mismatch and violate the clock framework API contract?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010115539.9237-1-l.scorcia@gmail.com?part=2