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

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

On 10/29/25 4:37 PM, Bjorn Andersson wrote:
> 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@linaro.org>
>>>>>>>>> Signed-off-by: Jingyi Wang <jingyi.wang@oss.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@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@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@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@oss.qualcomm.com/
> which Konrad pointed out, which would also work with this model
> (qcom,imem fallback but no implementation).

Hm sorry I skipped this thread and started repeating similar points
in v3.

Ultimately I don't really care either way (mmio-sram vs generic node
acted upon by different drivers), but I do care about closing this
discussion..

Konrad