Re: [PATCH v6 3/3] arm64: dts: qcom: sm8250: Add JPEG encoder node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Unmanaged Interconnect Bandwidth / DT Misconfiguration
- [High] Incomplete / Ambiguous OPP Performance State Mapping

Pre-existing issues:
- [Critical] Missing Power Domain Attachment Logic (Driver)
- [High] Clock Ordering and Unconfigured Clocks
--

--- Patch [3]: [PATCH v6 3/3] arm64: dts: qcom: sm8250: Add JPEG encoder node ---
commit 78bc4a1419802a831ed1f485f610899cf6a592a5
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

arm64: dts: qcom: sm8250: Add JPEG encoder node

Add the JPEG encoder hardware node to the SM8250 device tree so the
qcom-jpeg V4L2 encoder driver can bind and operate on this platform.

> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index f11da2ca41818..d42b294193202 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi

[ ... ]

> +		jpeg-encoder@ac53000 {
> +			compatible = "qcom,sm8250-jpeg-enc";
> +			status = "disabled";
> +
> +			reg = <0x0 0x0ac53000 0x0 0x1000>;
> +
> +			interrupts = <GIC_SPI 474 IRQ_TYPE_EDGE_RISING>;
> +
> +			power-domains = <&camcc TITAN_TOP_GDSC>,
> +					<&rpmhpd RPMHPD_MMCX>,
> +					<&rpmhpd RPMHPD_MX>;

[Severity: High]
Will the OPP framework map performance states to the correct domain here?

The device node lists three power domains, but the jpeg_opp_table
provides only a single required-opps entry per OPP. The OPP framework
maps required-opps entries sequentially to attached genpd domains.

It looks like this maps the single entry to the first domain
(TITAN_TOP_GDSC), which does not support performance states, while the
scalable MMCX and MX domains receive no performance scaling mapping. Could
this lead to voltage scaling silently failing?

> +			power-domain-names = "titan_top", "mmcx", "mx";

[ ... ]

> +			jpeg_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +
> +				opp-100000000 {
> +					opp-hz = /bits/ 64 <400000000>,
> +						 /bits/ 64 <100000000>;
> +					required-opps = <&rpmhpd_opp_min_svs>;
> +				};

[Severity: Critical]
Are we missing the opp-peak-kBps arrays required for interconnect bandwidth
voting?

The DT node defines interconnects, but the OPP table omits opp-peak-kBps.
If the driver relies entirely on the OPP framework for interconnect management,
without opp-peak-kBps the framework votes for 0 kBps when an OPP is set.

Could this cause a synchronous external abort when the JPEG hardware accesses
memory through unclocked or unpowered NoC paths?

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but does the qcom-jpeg driver
have the necessary logic to attach and manage multiple power domains?

The new device tree node specifies three power domains:

power-domains = <&camcc TITAN_TOP_GDSC>,
		<&rpmhpd RPMHPD_MMCX>,
		<&rpmhpd RPMHPD_MX>;

Looking at the driver's probe function:

drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c:qcom_jpeg_probe() {
    ...
    rc = devm_pm_runtime_enable(jenc->dev);
    if (rc)
        goto err_free_jenc;
    ...
}

The driver relies solely on the driver core, which automatically attaches
only the first domain (TITAN_TOP_GDSC). Because the probe function does
not explicitly call devm_pm_domain_attach_list(), the MMCX and MX domains
are never powered on.

Will this result in unhandled page faults when accessing unpowered hardware
registers?

[Severity: High]
This is a pre-existing issue, but is the clock ordering correct when resuming
or during active transitions?

Looking at the driver's clock enablement logic:

drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c:qcom_jpeg_clk_on() {
    ...
    rc = clk_bulk_prepare_enable(jenc->num_clks, jenc->clks);
    if (rc)
        return rc;

    /* setup the OPP according to the calculated optimal frequency */
    opp = dev_pm_opp_find_freq_ceil_indexed(jenc->dev, &jenc->opt_freq, JPEG_OPP_CORE_IDX);
    ...
    rc = dev_pm_opp_set_opp(jenc->dev, opp);
    ...
}

Clocks are explicitly enabled before the OPP framework handles the required
voltage scaling constraints via dev_pm_opp_set_opp(). If the bootloader left
clocks at a high frequency, enabling them before voltage is scaled up could
violate hardware voltage constraints.

Additionally, three of the five provided clocks (sf_axi, core_ahb, cpas_ahb)
are never rate-configured by the driver, leaving them at arbitrary unmanaged
rates. Could this lead to hardware instability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730150524.2659577-1-atanas.filipov@oss.qualcomm.com?part=3