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

Tue Feb 4 14:16:55 CET 2020

Hi Marek

> From: Marek Vasut <marex at denx.de>
> Sent: dimanche 2 fÃ©vrier 2020 18:28
> 
> On 1/31/20 9:15 AM, Patrick DELAUNAY wrote:
> > Hi Marek,
> 
> Hi,
> 
> >> From: Marek Vasut <marex at denx.de>
> >> Sent: jeudi 30 janvier 2020 03:23
> >>
> >> On 1/29/20 5:51 PM, Patrick DELAUNAY wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>>> From: Marek Vasut <marex at denx.de>
> >>>> Sent: mardi 28 janvier 2020 13:16
> >>>>
> >>>> On 1/28/20 10:11 AM, Patrick Delaunay wrote:
> >>>>> From: Antonio Borneo <antonio.borneo at st.com>
> >>>>>
> >>>>> LTDC modifies the clock frequency to adapt it to the display. Such
> >>>>> frequency change is not detected by the FDCAN driver that instead
> >>>>> cache the value at probe and pretend to use it later.
> >>>>>
> >>>>> Keep the LTDC alone on PLL4_Q by moving the FDCAN to PLL4_R.
> >>>>
> >>>> Now this looks like a grisly workaround. Can you fix the LTDC
> >>>> driver to do something sane on boards which didn't update bootloader yet ?
> >>>
> >>> In fact it more a issue in the initial clock-tree used when I
> >>> upstream the ST board the first time... based on our delivery v1.0.0
> >>>
> >>> It is already corrected in downstream on v1.1.0:
> >>> + For U-Boot =
> >>> + https://github.com/STMicroelectronics/u-boot/commit/d62f14dece32e4
> >>> + 1c
> >>> + 2854b9ff44aca7b8384aa8a0 For TF-A =
> >>> + https://github.com/STMicroelectronics/arm-trusted-firmware/commit/
> >>> + 9a
> >>> + 24ceda6c3ba060d9acf2b26d069fedde9f0807
> >>>
> >>> The LTDC/DSI need to set the pixel clock according the panel
> >>> configuration and
> >> settings: it is normal and needed.
> >>>
> >>> But If the pixel clock is shared with FDCAN, which expects that its
> >>> input clock is
> >> fixed, an issue occur when the pixel clock change.
> >>
> >> I understand the problem you are trying to solve.
> >>
> >> 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.

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.

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.

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.

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)

Regards

Patrick