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

[Bjorn Andersson]

On Wed, Oct 29, 2025 at 07:47:11PM +0800, Aiqun(Maria) Yu wrote:
> On 10/28/2025 2:44 AM, Bjorn Andersson wrote:
> > 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@xxxxxxxxxx>
> >>>>>>> Signed-off-by: Jingyi Wang <jingyi.wang@xxxxxxxxxxxxxxxx>
> >>>>>>> ---
> >>>>>>>  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@14680000 {
> >>>>> 			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@xxxxxxxxxx/
> >>>>
> >>>
> >>> 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@14680000 {
> >> 		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)?
> 
> 
> Totally agree. We can remove the "syscon" and "simple-mfd" compatibles
> for Kaanapali.
> For Kaanapali, the reboot reason does not rely on imem at all—it uses
> nvmem cells instead.
> Previously, the syscon-reboot-mode required "syscon" and "simple-mfd"
> compatibles for older targets like APQ8064, which used imem as the
> reboot mode solution.
> 

And there's
https://lore.kernel.org/lkml/20250527-topic-ipa_imem-v2-0-6d1aad91b841@xxxxxxxxxxxxxxxx/
which Konrad pointed out, which would also work with this model
(qcom,imem fallback but no implementation).


This also does leave the door open for a future qcom,imem
implementation, if we need to associate some logic to the imem device
itself.

Regards,
Bjorn

> 
> > 
> > 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
> 
> 
> -- 
> Thx and BRs,
> Aiqun(Maria) Yu