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

[Heiko Schocher]

Hello Miquel,

On 01.10.24 10:50, Miquel Raynal wrote:
> Hi Michael,
> 
> michael at amarulasolutions.com wrote on Tue, 1 Oct 2024 10:33:56 +0200:
> 
>> Hi Miguel
>>
>> On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal at bootlin.com> 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/
>>>   
>>
>> The clock patches are not specific at all. You need to have it working
>> for the parent for each component.
> 
> The diff shown by Heiko is explicitly enabling PLLs by naming them in
> the iMX driver. This is not the correct approach. The problem of
> having non-enabled new parents is global. Parent clocks should be
> enabled before changing muxes, and this should be enforced
> by the clock core/uclass, not the SoC drivers.

Okay, valid argument.

> 
>> This is a standard way to do it and nothing magic compared to other
>> implementations.
> 
> No, naming PLLs explicitly is not the correct approach.
> 
>> I don't see
>> in your series any addressing or reparent clock or take in account
>> that a clock should be enable before
>> reparenting.
> 
> That's exactly the link above, whose diff is pasted here for reference:
> 
> @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>   	if (!ops->set_parent)
>   		return -ENOSYS;
>   
> +	ret = clk_enable(parent);
> +	if (ret)
> +		return ret;

As I said before, I had *exact* the same patch and thought I made a big
hack :-P

But I wonder ... if this a generic "problem", why nobody had yet problems
with it...

Thanks!

bye,
Heiko

>>> My series is complimentary, even though there are some overlaps that we
>>> need to merge.
>>
>> The only collision I can see from your series is that you re-write the
>> imx approach of power domain.
>> Can you please expand here a bit your point?
> 
> Well, that's what I'd call an overlap :) There are also similar changes
> in the clock core IIRC. Well, there is a bit of merging that needs to
> happen I guess, but if you don't think there is any then my series can
> enter as-is (once the comments addressed, ofc).
> 
> Thanks,
> Miquèl
> 

-- 
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