linux-kernel - Re: [PATCH v2 1/4] dt-bindings: soc: qcom: Add qcom,kaanapali-imem compatible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ygqgzflpavwgd43e5zedgcijm3lz27nqlzprttalgcroedz45u@ztqkppajpyry>
Date: Mon, 27 Oct 2025 13:44:10 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Jingyi Wang <jingyi.wang@....qualcomm.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Robert Marko <robimarko@...il.com>, Das Srinagesh <quic_gurus@...cinc.com>, 
	aiqun.yu@....qualcomm.com, tingwei.zhang@....qualcomm.com, trilok.soni@....qualcomm.com, 
	yijie.yang@....qualcomm.com, linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Eugen Hristev <eugen.hristev@...aro.org>
Subject: Re: [PATCH v2 1/4] dt-bindings: soc: qcom: Add qcom,kaanapali-imem
 compatible

On Thu, Oct 23, 2025 at 03:06:00AM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 22, 2025 at 05:42:58PM -0500, Bjorn Andersson wrote:
> > On Wed, Oct 22, 2025 at 12:34:58PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, Oct 22, 2025 at 05:05:30PM +0800, Jingyi Wang wrote:
> > > > 
> > > > 
> > > > On 10/22/2025 4:49 PM, Dmitry Baryshkov wrote:
> > > > > On Wed, Oct 22, 2025 at 12:28:41AM -0700, Jingyi Wang wrote:
> > > > >> Document qcom,kaanapali-imem compatible.
> > > > >>
> > > > >> Reviewed-by: Eugen Hristev <eugen.hristev@...aro.org>
> > > > >> Signed-off-by: Jingyi Wang <jingyi.wang@....qualcomm.com>
> > > > >> ---
> > > > >>  Documentation/devicetree/bindings/sram/qcom,imem.yaml | 1 +
> > > > >>  1 file changed, 1 insertion(+)
> > > > >>
> > > > >> diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> > > > >> index 6a627c57ae2f..1e29a8ff287f 100644
> > > > >> --- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> > > > >> +++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
> > > > >> @@ -19,6 +19,7 @@ properties:
> > > > >>        - enum:
> > > > >>            - qcom,apq8064-imem
> > > > >>            - qcom,ipq5424-imem
> > > > >> +          - qcom,kaanapali-imem
> > > > > 
> > > > > Can you use mmio-sram instead?
> > > > > 
> > > > 
> > > > Here is the node: 
> > > > 
> > > > 		sram@...80000 {
> > > > 			compatible = "qcom,kaanapali-imem", "syscon", "simple-mfd";
> > > > 			reg = <0x0 0x14680000 0x0 0x1000>;
> > > > 			ranges = <0 0 0x14680000 0x1000>;
> > > > 
> > > > 			#address-cells = <1>;
> > > > 			#size-cells = <1>;
> > > > 
> > > > 			pil-reloc@94c {
> > > > 				compatible = "qcom,pil-reloc-info";
> > > > 				reg = <0x94c 0xc8>;
> > > > 			};
> > > > 		};
> > > > 
> > > > other qualcomm are also using imem, could you please give more details on why
> > > > we should use mmio-sram here?
> > > 
> > > https://lore.kernel.org/linux-arm-msm/e4c5ecc3-fd97-4b13-a057-bb1a3b7f9207@kernel.org/
> > > 
> > 
> > I considered exactly this when I wrote the binding back then...
> > 
> > But the binding defines mmio-sram as "Simple IO memory regions to be
> > managed by the genalloc API." and the Linux sram driver follows that and
> > registers a gen_pool across the sram memory region.
> > 
> > I believe IMEM is SRAM (it's at least not registers), but its memory
> > layout is fixed, so it's not a pool in any form.
> > 
> > 
> > What Krzysztof says makes sense, but rather than just throwing a yak at
> > Jingyi, it would be nice if you provided some guidance on how you would
> > like to see this turn out.
> 
> I tested, pretty same approach seems to work:
> 

Now you're shaving at random ;)

> 	sram@...80000 {
> 		compatible = "mmio-sram";

You can put "pil-reloc-sram" wherever, because it will perform a
of_find_compatible_node() to dig up some node with the compatible
"qcom,pil-reloc-info" .

In other words, this line created a genpool for something that really
isn't a genpool, but luckily that didn't have any side effects.


There are however other users of IMEM, such as the "reboot-mode", which
relies on the "sram" device probing child devices, and is implemented by
"syscon-reboot-mode".

Perhaps the solution is to not support any new users of that?


But no matter what, the definition "Simple IO memory regions to be
managed by the genalloc API" will never be true for IMEM.

And as this isn't a syscon, simple-mfd, or mmio-sram...how about making
the fallback "qcom,imem" (in this same binding) and omitting any
implementation until we need one)?

Regards,
Bjorn

> 		reg = <0x0 0x14680000 0x0 0x1000>;
> 		ranges = <0 0 0x14680000 0x1000>;
> 
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 
> 		pil-reloc-sram@94c {
> 			compatible = "qcom,pil-reloc-info";
> 			reg = <0x94c 0xc8>;
> 		};
> 	};
> 
> 
> -- 
> With best wishes
> Dmitry