Re: [PATCH] dt-bindings: mfd: qcom,tcsr: Add compatible for Kaanapali

[Krzysztof Kozlowski <krzk@kernel.org>]

On 13/11/2025 11:03, Aiqun(Maria) Yu wrote:
> On 11/12/2025 12:05 AM, Bjorn Andersson wrote:
>> On Tue, Nov 11, 2025 at 08:27:17PM +0800, Aiqun(Maria) Yu wrote:
>>> On 11/7/2025 12:24 AM, Konrad Dybcio wrote:
>>>> On 11/6/25 11:16 AM, Aiqun(Maria) Yu wrote:
>>>>> On 11/6/2025 5:06 AM, Bjorn Andersson wrote:
>>>>>> On Tue, Nov 04, 2025 at 01:35:01PM +0800, Jingyi Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 11/4/2025 12:02 PM, Bjorn Andersson wrote:
>>>>>>>> On Tue, Nov 04, 2025 at 11:34:25AM +0800, Aiqun(Maria) Yu wrote:
>>>>>>>>> On 9/25/2025 7:23 AM, Jingyi Wang wrote:
>>>>>>>>>> Document the qcom,tcsr-kaanapali compatible, tcsr will provide various
>>>>>>>>>> control and status functions for their peripherals.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
>>>>>>>>>> ---
>>>>>>>>>>  Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml | 1 +
>>>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml b/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
>>>>>>>>>> index 14ae3f00ef7e..ae55b0a70766 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.yaml
>>>>>>>>>> @@ -48,6 +48,7 @@ properties:
>>>>>>>>>>            - qcom,tcsr-ipq8064
>>>>>>>>>>            - qcom,tcsr-ipq8074
>>>>>>>>>>            - qcom,tcsr-ipq9574
>>>>>>>>>> +          - qcom,tcsr-kaanapali
>>>>>>>>>
>>>>>>>>> It looks good to me. Glymur didn't have this functionality verified yet.
>>>>>>>>
>>>>>>>> You spelled Reviewed-by: Aiqun Yu <..> wrong.
>>>>>>>>
>>>>>>>>> Remind for review.
>>>>>>>>
>>>>>>>> No need for that, reviewers will review when they have time.
>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>> Hi Bjorn,
>>>>>>>
>>>>>>>>
>>>>>>>> But that said, most modern additions to this binding follow the common
>>>>>>>> format of qcom,<soc>-<block>.
>>>>>>>>
>>>>>>>> So I would prefer this to be qcom,kaanapali-tcsr.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Bjorn
>>>>>>>>
>>>>>>>
>>>>>>> qcom,tcsr-kaanapali is used to distinguish with binding for GCC:
>>>>>>> https://lore.kernel.org/all/20251030-gcc_kaanapali-v2-v2-2-a774a587af6f@oss.qualcomm.com/
>>>>>>>
>>>>>>
>>>>>> So, qcom,kaanapali-tcsr is the clock controller region of TCSR and
>>>>>> qcom,tcsr-kaanapali is the non-clock controller region of TCSR?
>>>>>>
>>>>>> Sorry for not understanding that earlier, but this doesn't work for me.
>>>>>>
>>>>>> It's a bit of a lie that TCSR_MUTEX is a separate node in devicetree,
>>>>>> but it's always an nice chunk of 256K in the beginning (or end in some
>>>>>> cases?) of TCSR. But for the rest, there should be a single tcsr node in
>>>>>> DeviceTree and that one node should be a syscon and a clock controller.
>>>>>
>>>>> I've been dive deeply on this tcsr block. And actually the tcsr clock
>>>>> controller part is a very small trunk size(0x1c) of the tcsr block. And
>>>>> this block have contain other multiple purposed sys registers. So maybe
>>>>> we can have a more discussion on how to have device tree node describe
>>>>> this situation? It is not straight forward that to have a non-tcsrcc
>>>>> related area being described in tcsrcc.
>>>>>
>>>>> What about option 1 (tcsr_mutex + tcsr_dload_syscon + tcsrcc):>> tcsr_mutex: hwlock@1f40000 {
>>>>> 	compatible = "qcom,tcsr-mutex";
>>>>> 	reg = <0x0 0x01f40000 0x0 0x20000>;
>>>>> 	#hwlock-cells = <1>;
>>>>> };
>>>>>
>>>>> tcsr_dload: syscon@1fc0000 {
>>>>> 	compatible = "qcom,tcsr-kaanapali", "syscon";
>>>>> 	reg = <0x0 0x1fc0000 0x0 0x30000>;
>>>>> };
>>>>>
>>>>> tcsrcc: clock-controller@1fd5044 {
>>>>> 	compatible = "qcom,kaanapali-tcsr", "syscon";
>>>
>>> Remove "syscon" here. Not need for tcsrcc fallback to "syscon".
>>>
>>>>> 	reg = <0x0 0x01fd5044 0x0 0x1c>;
>>>>> ...
>>>>> };
>>>>>
>>>>> What about option 2 (tcsr whole block + tcsr_mutex  + tcsrcc):
>>>>>
>>>>> tcsr: syscon@1f40000 {
>>>>> 	compatible = "qcom,tcsr-kaanapali", "syscon";
>>>>> 	reg = <0x0 0x1f40000 0x0 0xC0000>; //align with the whole hardware
>>>>> block design.
>>>>> };
>>>>>
>>>>> tcsr_mutex: hwlock@1f40000 {
>>>>> 	compatible = "qcom,tcsr-mutex";
>>>>> 	reg = <0x0 0x01f40000 0x0 0x20000>;
>>>>> 	#hwlock-cells = <1>;
>>>>> };
>>>>>
>>>>> tcsrcc: clock-controller@1fd5044 {
>>>>> 	compatible = "qcom,kaanapali-tcsr", "syscon";
>>>
>>> Same here, don't need to have "syscon" here.
>>>
>>>>> 	reg = <0x0 0x01fd5044 0x0 0x1c>;
>>>>> ...
>>>>> };
>>>>
>>>> Is there anything wrong with what we have done for x1e80100?
>>>> ______________________
>>>> |             |       |
>>>> | TCSR_MUTEX  | mutex |
>>>> |_____________|_______|
>>>> |	      |       |
>>>> | RANDOM_REGS |       |
>>>> |_____________|       |
>>>> |	      |       |
>>>> | TCSR_CLKS   | tcsr  |
>>>> |_____________|       |
>>>> |	      |       |
>>>> | RANDOM_REGS |       |
>>>> |_____________|_______|
>>>>
>>>
>>> Second you! We can firstly have a option selected for kaanapali, and
>>> then other platform can be followed or fixed afterwards.
>>>
>>> Here suggest to have option 2 which is remove "syscon" from tcsr clocks,
>>> and only add the whole "syscon" to "tcsr" whole block.
>>>
>>
>> I think you misunderstood Konrad, or perhaps I misunderstand you.
> 
> Maybe let Konrad help to explain more here. I thought the chart below is
> very clearly indicate the tcsr_clks is only part of the tcsr block.
> 
>>
>> This is what we have for Hamoa:
>>
>> tcsr_mutex: hwlock@1f40000 {
>>         compatible = "qcom,tcsr-mutex";
>>         reg = <0 0x01f40000 0 0x20000>;
>>         #hwlock-cells = <1>;
>> };
>>
>> tcsr: clock-controller@1fc0000 {
> 
> 
> It is not a clock-controller start from 0x1fc0000.
> 
>>         compatible = "qcom,x1e80100-tcsr", "syscon";
>>         reg = <0 0x01fc0000 0 0x30000>;
> 
> This is what we have a discussion initialized here:
> "qcom,<platform>-tcsr" is only a tcsr clock controller binder, reference
> from [1].
> "qcom,tcsr-<platform>" is a common tcsr block. reference current binding
> patch.
> 
> For below hardware block information, is it really a recommendation to
> combine the tscr block with tcsr clock controller all together?
> ______________________
> |             |       |
> | TCSR_MUTEX  | mutex |
> |_____________|_______|
> |	      |       |
> | RANDOM_REGS |       |
> |_____________|       |
> |	      |       |
> | TCSR_CLKS   | tcsr  |
> |_____________|       |
> |	      |       |
> | RANDOM_REGS |       |
> |_____________|_______|
> 
> [1]https://lore.kernel.org/all/20251030-gcc_kaanapali-v2-v2-2-a774a587af6f@oss.qualcomm.com/
> 
> 
>>         clocks = <&rpmhcc RPMH_CXO_CLK>;
>>         #clock-cells = <1>;
>>         #reset-cells = <1>;
>> };
>>
>> This is exactly what I suggested above and Konrad is asking you why
>> this doesn't work for Kaanapali. The addresses are even the same, what
>> is the problem?
> 
> The problem is the current patchset document a separate tcsr block as a
> mfd. While the suggestion here is to use the tcsr clock controller

There is no MFD. Don't use that term in context of supporting a change.
But regardless, this documents only random regs.

> binding to document the whole tcsr block which is not belonged to tcsr
> clock controller.

I don't understand whether you claim this patch as "this suggestion" or
something else.


Best regards,
Krzysztof