Re: [PATCH 1/4] dt-bindings: mtd: qcom,nandc: Add MDM9607 QPIC NAND controller

[Konrad Dybcio]

On 6/9/26 12:02 PM, Stephan Gerhold wrote:
> On Tue, Jun 09, 2026 at 11:30:54AM +0200, Miquel Raynal wrote:
>> On 09/06/2026 at 11:08:03 +02, Stephan Gerhold <stephan.gerhold@xxxxxxxxxx> wrote:
>>
>>> On Tue, Jun 09, 2026 at 11:01:18AM +0200, Konrad Dybcio wrote:
>>>> On 6/9/26 10:55 AM, Konrad Dybcio wrote:
>>>>> On 6/9/26 10:10 AM, Stephan Gerhold wrote:
>>>>>> On Tue, Jun 09, 2026 at 09:52:51AM +0200, Miquel Raynal wrote:
>>>>>>>>> On MDM9607, there is only a single controllable clock for the NAND
>>>>>>>>> controller (RPM_SMD_QPIC_CLK). The same situation also applies e.g. for
>>>>>>>>> qcom,sdx55-nand, but the corresponding device tree (qcom-sdx55.dtsi) works
>>>>>>>>> around that by assigning a dummy clock (&nand_clk_dummy) to the second
>>>>>>>>> clock ("aon") that is required by the dt-bindings. This is not really
>>>>>>>>> useful, so avoid doing that for new platforms by excluding the second "aon"
>>>>>>>>> clock entry in the dt-bindings.
>>>>>>>>
>>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxxxxx>
>>>>>>>
>>>>>>> What is the problem in giving twice the same clock? If this is what is
>>>>>>> done in the hardware routing, I do not see the reason for more
>>>>>>> complexity in the binding?
>>>>>>>
>>>>>>
>>>>>> I had that in my first draft for this series, but this would be wrong
>>>>>> IMO. I suspect there is no QPIC/NAND related "aon" (always-on) clock on
>>>>>> this platform at all. I'm not sure about MDM9607 in particular (maybe
>>>>>> someone from Qualcomm can confirm), but a similar platform I was looking
>>>>>> into at some point actually had *3* separate clocks for QPIC in the
>>>>>> hardware and none of them were called "aon" ...
>>>>>
>>>>> gcc_qpic_ahb_clk (50/100/133.(3) MHz sourced from PCNoC_bfdcd_clk_src)
>>>>> gcc_qpic_clk (likewise, sourced from qpic_clk_src which is sourced
>>>>> from GPLLs)
>>>>> gcc_qpic_system_clk (32 KHz)
>>>>>
>>>>> No clock containing the substring 'aon' in its name on this platform
>>>>
>>>> Looking at SDX65, perhaps the 32 Khz clock is the "aon" one after all..
>>>> The NAND documentation says
>>>>
>>>> CC_QPIC_SYSTEM_CLK - Always-on timeout clock (32 KHz)
>>>>
>>>
>>> Thanks for looking this up.
>>>
>>> IMO, if we want to describe the actual hardware routing, we should
>>> describe all 3 clocks and assign all of them to RPM_SMD_QPIC_CLK for
>>> MDM9607).
>>
>> Sounds more accurate to me.
>>
>>> The resulting diff would be basically the same as this patch just
>>> inversed (3 clocks for MDM9607+SDX(?) and 2 clocks for the IPQ* SoCs.
>>
>> Diff would not be simpler but more accurate. So if we go for a
>> modification of the bindings, I would prefer that path.
>>
> 
> IMO the result wouldn't be much more accurate from the perspective of
> the kernel. If we assign RPM_SMD_QPIC_CLK to all 3 clocks we would be
> effectively saying "there is a single clock with a single rate that is
> sourcing 'core', 'ahb' and 'system'(/'aon')". But in reality, these are
> 3 separate clock domains with separate rates, as shown by Konrad above.
> 
> We could try defining dummy clocks like the &nand_clk_dummy in
> qcom-sdx55.dtsi, but this isn't very accurate either. Presumably, all of
> these clocks are toggled by RPM_SMD_QPIC_CLK. So if we define a dummy
> clock for 'ahb', then enabling that clock without also enabling the
> non-dummy 'core' (RPM_SMD_QPIC_CLK) will do nothing.

I can't find a good answer for what RPM_SMD_QPIC_CLK controls, maybe
+Mani or +Kathiravan know where to look

Konrad

> 
> At the end, the truth for the OS/kernel running on this hardware is that
> it can only see the 'core' clock (with the option to change its rate).
> All others are invisible, with no way to influence or check the status,
> so pretending that we have separate resources for them doesn't really
> make things more accurate in my opinion.
> 
> But yeah, let's leave the decision up to Krzysztof. I'm happy to change
> this patch as needed as long it works at the end. :-)