[PATCH 08/26] power: Add iMX8M block ctrl driver for dispmix

[Heiko Schocher]

Hello Miquel,

On 01.10.24 10:29, Miquel Raynal wrote:
> Hi Heiko,
> 
> 
>>>>> Hmm.. unfortunately ... I had applied your 2 clock patches, which
>>>>> fixed a problem with enabling parent clocks ... but they broke booting
>>>>> on a carrier which has fec ethernet! After "Net: " output the board hang...
>>>>>
>>>>> I reverted your 2 clock patches and it bootet again ... so there is
>>>>> a problem ... try to get some more time to look into...
>>>>>   
>>>>
>>>> We have a fec, but we had I think two patches more on it. I forget to
>>>> answer Marek
>>>> about them because I don't have my board now and because he is
>>>> partially right (or maybe right).
>>>> Anyway when we boot we could have and we have clocks that are enabled
>>>> by bootrom or SPL that
>>>> we need to declare as enable like PLL2/PLL3 those clocks are parts or
>>>> could be part of reparent so,
>>>> you need to have a reference counter on them that allow to not disable
>>>> during the down chain disable
>>>> and up chain enable. I think that what happens to your ethernet it's
>>>> that you disable some clock that is
>>>> critical to the board to survive. I had a patch merged by Tom that
>>>
>>> Yep, thats what I think too! If you access registers in a block for
>>> which the clock is not enabled ... it just hang...
>>>    
>>>> prints the clock name so if you enable
>>>> the debug of the clock you will find that your board stops working
>>>> during one of this reparinting.
>>>
>>> I currently work on 2024.07... will rebase if 2024.10 is out...
>>>
>>> Ah, you mean:
>>>
>>> commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08
>>> Author: Michael Trimarchi <michael at amarulasolutions.com>
>>> Date:   Tue Jul 9 08:28:13 2024 +0200
>>>
>>>       clk: clk-uclass: Print clk name in clk_enable/clk_disable
>>>
>>>       Print clk name in clk_enable and clk_disable. Make sense to know
>>>       what clock get disabled/enabled before a system crash or system
>>>       hang.
>>>
>>>       Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
>>>
>>> I have the same change when I debug :-P
>>>
>>> IIRC I did not see the clock names ... but I will recheck!
>>
>> I see with your patch the clock names, so fine... and see [1]
>>
>> Hmm... I am on imx8mp, and I think the changes the patchset do in
>>
>> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate"
>>
>> are in clk-imx8mp already ...
>>
>> Ported the patch from patchset
>>
>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled"
>>
>> to imx8mp [2] and fec ethernet works again for me on imx8mp!
>>
>> Could you add this if you post a v2 ?
> 
> TBH I don't feel like the below change is the correct one, it is too
> specific. The clock core is recursive and thus should handle the
> reparenting situations gracefully.
> 
> I posted a series that is targeting the LVDS output on imx8mp. You
> should probably consider checking these patches as well if you work
> on imx8mp as well. I also had similar breakages with Ethernet which
> happened during the assigned-clocks handling. This patch, which is more
> future and platform agnostic, fixed it:
> 
> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/

Yes, I also had such a "fix" first, before seeing Darios patchset!

Damn, I did not see your patches...

regarding your patch ... I am unsure which version is the best one ...
I fear of side effects for other plattforms adding this change in
generic "drivers/clk/clk-uclass.c" file...

> My series is complimentary, even though there are some overlaps that we
> need to merge.

I try to find time to test them on my board!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de