[PATCH 4/9] ARM: dts: stm32mp1: move FDCAN to PLL4_R

[Marek Vasut]

On 2/4/20 2:16 PM, Patrick DELAUNAY wrote:
> Hi Marek

Hello Patrick,

[...]

>>>> What I think you are missing is that not everyone will update
>>>> ATF/U-Boot/Linux in lockstep. That is the problem you need to deal with here.
>>>
>>> I understood the possible issue and I hope that I will be not the case
>>> (at least for the ST Microelectronics boards).
>>
>> Do I understand it correctly that you expect the customers who buy the ST chip to
>> update bootloader in lockstep with the kernel in systems which are deployed today
>> ?
>>
>> No, this does not work. If you have a working bootloader and your kernel fails to
>> start, that is something you can recover from, If your bootloader fails to start and
>> you need to dig an embedded system buried who-knows-where or recall a lot of
>> systems because of a failed bootloader update, that would be a disaster.
> 
> No, we don't expect a bootloader updater for all the current customers.
>  
> We found this weakness in the clock tree configuration used in ST board 
> after our first delivery and we have already provide the patch (in downstream)
> to clients. So we hope they already use this correction in the  bootloaders
> used in current products.

Since it's not in mainline, they do not. Unless you expect that users of
the STM32MP1 will use some random downstream fork of vendoruboot.

> However this clock issue only occur for few case, when FDCAN and LTDC are 
> used in parallel on the boards and only if LTDC change the pixel clock.
> 
> So it should be occurs only for few customer and the issue is not blocking for
> most of the cases.
> 
>>> We are aware of the possible issue to fixe these clocks in first stage
>>> bootloader but after a long debate, we don't found a better solution, with the
>> constraints:
>>> - M4 firmware require clock initialization before start and it can be
>>> loaded by U-Boot
>>> - clock tree is generated by CubeMX tools which generate device tree
>>> (for bootloaders and kernel)
>>> - "assigned-clock" management in Linux kernel could lead us to a race
>>> condition for shared clock
>>>
>>> We spent a long time to found a clock tree which respect all the
>>> constraints of the SOC before to our first release v1.0 and before I upstream the
>> stm32mp1 support in U-Boot.
>>>
>>> Then I wait a year before to upstream the patches on clock tree
>>> initialization (and the second release v1.2) to avoid too many iteration.
>>>  And this patch on FDCAN is the only issue found on the initial clock tree.
>>>
>>> Today I think (hope?) it will be the last patch on this part.
>>
>> You will keep finding clock issues and no , this will not be the last patch which
>> fixes a clock issue.
>>
>> So what solution is there for those who can only update the kernel ?
> 
> Yes, this issue can also solved by a patch in Linux DT to change the clock tree:
> 
> &m_can1 {
> 	assigned-clocks = <&rcc FDCAN_K >;
> 	assigned-clock-parents = <&rcc PLL4_R>;
>  };
> 
> It is the solution recommended for any customer which can't/wan't update the bootloaders.

But this should be part of mainline Linux either way and possibly Linux
should print a BIG WARNING if such "weakness" is detected and fix it up,
otherwise some systems will become dependent on bootloader behavior and
once the behaviors diverge sufficiently, you will end up with a platform
which fails to boot.

If you want a physically embedded system to be deployed for 10+ years
somewhere and you want to keep updating it with latest kernel versions
(because security fixes and so on), then you can be sure a bootloader
update is not an option, because if the system stops working after such
an update, someone will have to go there and physically retrieve the
device and fix it, and that might be very expensive or impossible. If
you only update the kernel, then the bootloader can still be used to
recover even a failed kernel update.

> And I agree that this issue also highlight a issue in the FDCAN driver, which should use
> the API 'clk_rate_exclusive_get(clk)' to prevent modification by LTDC clock.
> 
> The current patch is only to provide a better "official" clock tree in U-Boot upstream, 
> as we known the ST board is used as reference by many client and also to align the clocks 
> used in downstream (https://github.com/STMicroelectronics/u-boot) and in upstream.

I am _NOT_ opposed to this patch.

My problem is with the bootloader-Linux clock tree dependency. That
dependency should not exist or be minimized, otherwise you end up with a
very poor long-term experience, see above. And if you want for this
platform to be successful in the industrial/automotive deployments, then
you want to make sure the long-term experience is a good one.

> But if you prefer kept the current clock tree for DH PKD2 board (with potential issue on FDCAN), 
> I can revert my modification on stm32mp15xx-dhcom-pdk2-u-boot.dtsi.

This has nothing to do with this board, or any other board, see above.
Patching this board is fine.

> I propose this modification only because it seems you have the same clock-tree than ST boards
>  (except CLK_ETH_PLL4P vs  CLK_ETH_DISABLED but is is probably linked to the ethernet
> PHY configuration)

No, keep the clock trees in sync as much as possible.

[...]