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

[Patrick DELAUNAY]

Hi Marek,

> From: Marek Vasut <marex at denx.de>
> Sent: mercredi 5 février 2020 03:23
> 
> 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.

Not random but official ST delivery, without any restriction.

Today Mainline (Linux / U-Boot / TF-A / OP-TEE) have still still restriction
(power support or security support for example) as we don't finish our upstream of
the STM32MP15x.

But we continue to push to have reduce the difference, to be sure that customer
can use soon indifferently the Mailine or our delivery.

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

I shared this point with Linux maintainers.
 
> 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.

I  agree.

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

Thanks, it is clear now 😉

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

With STM32MP15x SOC and RCC, very few clocks need to be fixed by bootloaders
(in fact more or less the root clocks of the system = frequency of PLL1 to PLL4, 
common for many device or protected  by security feature), I think it is the case for
any platform.

All the other clocks have a initial value / source provided by bootloaders but they can 
also be modified at runtime (by device tree assigned-clock-parents for not secure clocks
and with SMC call to TF-A secure monitor for "secure" clock).

For ST boards, we are trying to don't modify the initial clock tree-source only to prove
that this clock tree is functional / correct for most of case.

But for client and after first deployment, clock tree modification must be done in Linux kernel
without any Bootloader update (and avoid all known issue for OTA).

I shared your concerns with my team and we are completely agree with you.

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

OK, thanks

> [...]

Regards

Patrick