Re: [PATCH v2 2/2] phy: mediatek: Add support for MT8196 MIPI DSI PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Sleeping in Atomic Context
- [Low] Dead Code / Hardware Capability Restriction
--

--- Patch [2]: [PATCH v2 2/2] phy: mediatek: Add support for MT8196 MIPI DSI PHY ---
commit 290d9427c436e302c4aba5a26f5e3e1e9bb728f5
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: mediatek: Add support for MT8196 MIPI DSI PHY

Add support for the MIPI DSI PHY found in the MediaTek MT8196 SoC
and its variants. This PHY has a different register layout and provides
support for more hardware features compared to the previous generation.

> diff --git a/drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8196.c b/drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8196.c
> new file mode 100644
> index 0000000000000..273f236fa7e9e
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8196.c
> @@ -0,0 +1,196 @@
[ ... ]
> +static int mtk_mipi_tx_pll_enable(struct clk_hw *hw)
> +{
> +	struct mtk_mipi_tx *mipi_tx = mtk_mipi_tx_from_clk_hw(hw);
> +	void __iomem *base = mipi_tx->regs;
> +	u32 voltage = RG_DSI_PRD_REF_MINI;
> +	u32 pres = MIPITX_PRESERVED_MINI;
> +	unsigned long long pcw_calc;
> +	unsigned int txdiv, txdiv0;
> +	u32 pcw;
> +
> +	dev_dbg(mipi_tx->dev, "enable: %u bps\n", mipi_tx->data_rate);
> +
> +	if (mipi_tx->data_rate >= 2000000000) {
> +		/* Select higher signaling voltage for fast data rates */
> +		voltage = RG_DSI_PRD_REF_DEF;

[Severity: Low]
Is this block reachable? Later in mtk_mipi_tx_pll_determine_rate(),
req->rate is clamped to a maximum of 1,600,000,000 Hz. If the hardware
supports rates above 2.0 GHz, does the clamp limit need to be increased,
or is this dead code?

> +		pres = MIPITX_PRESERVED_DEF;
> +		txdiv = 1;
> +		txdiv0 = 0;
> +	} else if (mipi_tx->data_rate >= 1000000000) {
[ ... ]
> +	mtk_phy_set_bits(base + MIPITX_PLL_PWR, AD_DSI_PLL_SDM_PWR_ON);
> +	mtk_phy_clear_bits(base + MIPITX_PLL_CON1, RG_DSI_PLL_EN);
> +	usleep_range(30, 60);

[Severity: High]
Does this code sleep in an atomic context?

The mtk_mipi_tx_pll_enable() function is registered as the .enable
callback in mtk_mipi_tx_pll_ops below. The Common Clock Framework invokes
.enable under an enable_lock spinlock with interrupts disabled.

Calling usleep_range() here, which internally calls schedule(), will lead
to scheduling while atomic. Should this use an atomic-safe delay like
udelay(30), or should this logic be moved to the sleepable .prepare
callback?

> +
> +	mtk_phy_clear_bits(base + MIPITX_PLL_PWR, AD_DSI_PLL_SDM_ISO_EN);
> +	writel(pcw, base + MIPITX_PLL_CON0);
> +	mtk_phy_update_field(base + MIPITX_PLL_CON1, RG_DSI_PLL_POSDIV, txdiv0);
> +	usleep_range(30, 60);
> +
> +	mtk_phy_set_bits(base + MIPITX_PLL_CON1, RG_DSI_PLL_EN);
> +	usleep_range(30, 60);
> +
> +	return 0;
> +}
[ ... ]
> +static int mtk_mipi_tx_pll_determine_rate(struct clk_hw *hw,
> +					  struct clk_rate_request *req)
> +{
> +	req->rate = clamp_val(req->rate, 125000000, 1600000000);
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops mtk_mipi_tx_pll_ops = {
> +	.enable = mtk_mipi_tx_pll_enable,
> +	.disable = mtk_mipi_tx_pll_disable,
> +	.determine_rate = mtk_mipi_tx_pll_determine_rate,
> +	.set_rate = mtk_mipi_tx_pll_set_rate,
> +	.recalc_rate = mtk_mipi_tx_pll_recalc_rate,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713102322.21782-1-angelogioacchino.delregno@collabora.com?part=2