Re: [RFC PATCH 3/3] arm64: dts: qcom: qcm2290: Add CAMSS OPE node

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 3/23/26 1:58 PM, Loic Poulain wrote:
> Add the Qualcomm CAMSS Offline Processing Engine (OPE) node for
> QCM2290. The OPE is a memory-to-memory image processing block used in
> offline imaging pipelines.
> 
> The node includes register regions, clocks, interconnects, IOMMU
> mappings, power domains, interrupts, and an associated OPP table.
> 
> At the moment we assign a fixed rate to GCC_CAMSS_AXI_CLK since this
> clock is shared across multiple CAMSS components and there is currently
> no support for dynamically scaling it.
> 
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>  arch/arm64/boot/dts/qcom/agatti.dtsi | 72 ++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/agatti.dtsi b/arch/arm64/boot/dts/qcom/agatti.dtsi
> index f9b46cf1c646..358ebfc99552 100644
> --- a/arch/arm64/boot/dts/qcom/agatti.dtsi
> +++ b/arch/arm64/boot/dts/qcom/agatti.dtsi
> @@ -1935,6 +1935,78 @@ port@1 {
>  			};
>  		};
>  
> +		isp_ope: isp@5c42400 {

"camss_ope"? Label's don't need to be generic, but they need to be
meaningful - currently one could assume that there's a non-ISP OPE
as well (and I'm intentionally stretching it a bit to prove a point)



> +			compatible = "qcom,qcm2290-camss-ope";
> +
> +			reg = <0x0 0x5c42400 0x0 0x200>,
> +			      <0x0 0x5c46c00 0x0 0x190>,
> +			      <0x0 0x5c46d90 0x0 0xa00>,
> +			      <0x0 0x5c42800 0x0 0x4400>,
> +			      <0x0 0x5c42600 0x0 0x200>;
> +			reg-names = "top",
> +				    "bus_read",
> +				    "bus_write",
> +				    "pipeline",
> +				    "qos";

This is a completely arbitrary choice, but I think it's easier to compare
against the docs if the reg entries are sorted by the 'reg' (which isn't
always easy to do since that can very between SoCs but this module is not
very common)


> +
> +			clocks = <&gcc GCC_CAMSS_AXI_CLK>,
> +				 <&gcc GCC_CAMSS_OPE_CLK>,
> +				 <&gcc GCC_CAMSS_OPE_AHB_CLK>,
> +				 <&gcc GCC_CAMSS_NRT_AXI_CLK>,
> +				 <&gcc GCC_CAMSS_TOP_AHB_CLK>;
> +			clock-names = "axi", "core", "iface", "nrt", "top";

Similarly, in the arbitrary choice of indices, I think putting "core"
first is "neat"

> +			assigned-clocks = <&gcc GCC_CAMSS_AXI_CLK>;
> +			assigned-clock-rates = <300000000>;

I really think we shouldn't be doing this here for a clock that covers
so much hw

[...]


> +
> +			interrupts = <GIC_SPI 209 IRQ_TYPE_EDGE_RISING>;
> +
> +			interconnects = <&bimc MASTER_APPSS_PROC RPM_ACTIVE_TAG
> +					 &config_noc SLAVE_CAMERA_CFG RPM_ACTIVE_TAG>,
> +					<&mmnrt_virt MASTER_CAMNOC_SF RPM_ALWAYS_TAG
> +					 &bimc SLAVE_EBI1 RPM_ALWAYS_TAG>;
> +			interconnect-names = "config",
> +					     "data";
> +
> +			iommus = <&apps_smmu 0x820 0x0>,
> +				 <&apps_smmu 0x840 0x0>;
> +
> +			operating-points-v2 = <&ope_opp_table>;
> +			power-domains = <&gcc GCC_CAMSS_TOP_GDSC>,

Moving this under camss should let you remove the TOP_GDSC and TOP_AHB (and
perhaps some other) references

> +					<&rpmpd QCM2290_VDDCX>;
> +			power-domain-names = "camss",
> +					     "cx";> +
> +			ope_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +
> +				opp-19200000 {
> +					opp-hz = /bits/ 64 <19200000>;
> +					required-opps = <&rpmpd_opp_min_svs>;
> +				};
> +
> +				opp-200000000 {
> +					opp-hz = /bits/ 64 <200000000>;
> +					required-opps = <&rpmpd_opp_svs>;
> +				};
> +
> +				opp-266600000 {
> +					opp-hz = /bits/ 64 <266600000>;
> +					required-opps = <&rpmpd_opp_svs_plus>;
> +				};
> +
> +				opp-465000000 {
> +					opp-hz = /bits/ 64 <465000000>;
> +					required-opps = <&rpmpd_opp_nom>;
> +				};
> +
> +				opp-580000000 {
> +					opp-hz = /bits/ 64 <580000000>;
> +					required-opps = <&rpmpd_opp_turbo>;
> +					turbo-mode;

Are we going to act on this property? Otherwise I think it's just a naming
collision with Qualcomm's TURBO (which may? have previously??? had some
special implications)

Konrad