Re: [PATCH] ARM: dts: ti/omap: gta04: fix pm issues caused by spi module

[Roger Quadros <rogerq@kernel.org>]

On 28/11/2024 00:58, Andreas Kemnade wrote:
> Am Mon, 18 Nov 2024 15:08:48 +0200
> schrieb Roger Quadros <rogerq@kernel.org>:
> 
>> On 16/11/2024 22:27, Andreas Kemnade wrote:
>>> Am Mon, 11 Nov 2024 23:46:04 +0100
>>> schrieb Andreas Kemnade <andreas@kemnade.info>:
>>>   
>>>> Am Mon, 11 Nov 2024 19:31:17 +0100
>>>> schrieb Andreas Kemnade <andreas@kemnade.info>:
>>>>  
>>>>> Am Mon, 11 Nov 2024 17:09:53 +0200
>>>>> schrieb Tony Lindgren <tony@atomide.com>:
>>>>>     
>>>>>> * Andreas Kemnade <andreas@kemnade.info> [241108 17:41]:      
>>>>>>> They are not used, if they are just disabled, kernel does not touch
>>>>>>> them, so if it is there, the kernel can handle
>>>>>>> pm. At least as long as it is not under ti,sysc.
>>>>>>>
>>>>>>> There are probably cleaner solutions for this, but for a CC: stable I
>>>>>>> would prefer something less invasive.        
>>>>>>
>>>>>> For unused devices, it's best to configure things to use ti-sysc, and
>>>>>> then set status disabled (or reserved) for the child devices only. This
>>>>>> way the parent interconnect target module is PM runtime managed by
>>>>>> Linux, and it's power domain gets properly idled for the unused devices
>>>>>> too.
>>>>>>       
>>>>> Hmm, we also have omap_hwmod_setup_all() which is still called if
>>>>> without device nodes being available.
>>>>>
>>>>> Converting mcspi to ti-sysc is more than 100 lines. So it does not
>>>>> qualify for stable.
>>>>>     
>>>>>>> I can try a ti-sysc based fix in parallel.        
>>>>>>
>>>>>> Yeah that should be trivial hopefully :)
>>>>>>       
>>>>> I played around, got pm issues too, tried to force-enable things (via
>>>>> power/control),
>>>>> watched CM_IDLEST1_CORE and CM_FCLKEN1_CORE, they behave. Bits are set
>>>>> or reset.
>>>>>
>>>>> but not CM_IDLEST_CKGEN, it is 0x209 instead of 0x1.
>>>>>
>>>>> I test from initramfs, so no mmc activity involved
>>>>>
>>>>> removing status = "disabled" from mcspi3 solves things.
>>>>> With and without ti-sysc conversion. removing status = "disabled" from
>>>>> mcspi4 seems not to help.
>>>>>
>>>>> That all cannot be... I will retry tomorrow.
>>>>>     
>>>> well, I tried a bit further:
>>>> I build the omap spi driver as module.
>>>> and booted With mcspi3 not disabled and no module autoload.
>>>>
>>>> without module loaded: pm bad, same as with mcspi3 disabled
>>>> with module loaded: core pm ok
>>>> with module loaded and unloaded: core pm ok.
>>>>
>>>> so at least a trace.
>>>>  
>>> ok, I am a bit further.
>>> mcspi is per default in slave mode, setting it to master solves issues.
>>> And that happens when the driver is probed because its default is
>>> master.
>>> Having the pins muxed as mode 7 also helps or selecting a pulldown for
>>> cs. (cs is active high per default!)
>>> switching to pullup does not harm once the spi module is off, but having
>>> active cs seems to prevent idling despite CM_IDLEST1_CORE
>>> not showing it.
>>>
>>> History: u-boot muxes McSPI3, because it can be available on an
>>> optionally fitted pin header. But there is no user known (would need
>>> a dtb overlay anyways). So I will rather mux to mode 7.  
>>
>> I'm sorry I didn't fully understand the problem.
>>
>> So, u-boot configures pinmux for McSPI3 and enables McSPI3 as well
>> but fails to disable it properly?
> 
> At least it sets Pinmux.
>  
>> And because McSPI3 is in slave mode and CS is active it fails to
>> transition to idle in Linux?
> 
> yes, slave mode is default.
>>
>> So isn't this a u-boot issue?
>>
> Just telling u-boot to not mux McSPI3 helps. So, yes u-boot should not
> set it. But. I have no clear idea how bitrot the u-boot update process
> is. I would prefer setting the pinmux also in kernel back to the
> default.
> 
That would be fine.

-- 
cheers,
-roger