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

From: Antonio Borneo <antonio.borneo@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.

Signed-off-by: Antonio Borneo <antonio.borneo@st.com>
Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 arch/arm/dts/stm32mp157a-avenger96-u-boot.dtsi | 2 +-
 arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi       | 2 +-
 arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi       | 2 +-
 arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

On 1/28/20 10:11 AM, Patrick Delaunay wrote:
> From: Antonio Borneo <antonio.borneo@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 ?

Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 28 janvier 2020 13:16
> 
> On 1/28/20 10:11 AM, Patrick Delaunay wrote:
> > From: Antonio Borneo <antonio.borneo@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/d62f14dece32e41c2854b9ff44aca7b8384aa8a0
+ For TF-A = https://github.com/STMicroelectronics/arm-trusted-firmware/commit/9a24ceda6c3ba060d9acf2b26d069fedde9f0807

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.

We could add protection in FDCAN driver (don't assume fixed clock in probe for example) 
but anyway that don't protect for any issue (pending FDCAN transfer when pixel clock change).

The main issue is that we try to share a clock source between 2 IP that are not compatible:
1/ FDCAN => clock source configurated by CLK_FDCAN_PLL4Q
2/ pixel clocl for LTDC and DSI = LTDC_PX or DSI_PX  => _PLL4_Q  (hardcoded in RCC)

The clock source for pixel clock PLL4_Q need only managed only by LDTC as it can modify the source clock.

It is why we decide to change the reference clock tree used on ST Microelectronic boards.
And unfortunately that impacts the first stage bootloader.

For information in our solution the clock tree is fixed and configurated at boot by first stage bootloader 
(TF-A normally for trusted boot chain / SPL for basic boot chain) as this configuration is  done in secured
registers with information provided by device-tree.

See https://wiki.st.com/stm32mpu/wiki/STM32MP15_clock_tree for details

Regards

Patrick

On 1/29/20 5:51 PM, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

>> From: Marek Vasut <marex@denx.de>
>> Sent: mardi 28 janvier 2020 13:16
>>
>> On 1/28/20 10:11 AM, Patrick Delaunay wrote:
>>> From: Antonio Borneo <antonio.borneo@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/d62f14dece32e41c2854b9ff44aca7b8384aa8a0
> + For TF-A = https://github.com/STMicroelectronics/arm-trusted-firmware/commit/9a24ceda6c3ba060d9acf2b26d069fedde9f0807
> 
> 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.

Hi Marek,

> From: Marek Vasut <marex@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@denx.de>
> >> Sent: mardi 28 janvier 2020 13:16
> >>
> >> On 1/28/20 10:11 AM, Patrick Delaunay wrote:
> >>> From: Antonio Borneo <antonio.borneo@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/d62f14dece32e41c
> > + 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).

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.

Regards

Patrick

On 1/31/20 9:15 AM, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

>> From: Marek Vasut <marex@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@denx.de>
>>>> Sent: mardi 28 janvier 2020 13:16
>>>>
>>>> On 1/28/20 10:11 AM, Patrick Delaunay wrote:
>>>>> From: Antonio Borneo <antonio.borneo@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/d62f14dece32e41c
>>> + 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.

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

Hi Marek

> From: Marek Vasut <marex@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@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@denx.de>
> >>>> Sent: mardi 28 janvier 2020 13:16
> >>>>
> >>>> On 1/28/20 10:11 AM, Patrick Delaunay wrote:
> >>>>> From: Antonio Borneo <antonio.borneo@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

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.

[...]

Hi Marek,

> From: Marek Vasut <marex@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

On 2/6/20 9:59 AM, Patrick DELAUNAY wrote:
> Hi Marek,

Hello Patrick

[...]

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

So, shall we expect a proper fix for the Linux kernel too ?

[...]

On 1/28/20 10:11 AM, Patrick Delaunay wrote:
> From: Antonio Borneo <antonio.borneo@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.
>
> Signed-off-by: Antonio Borneo <antonio.borneo@st.com>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  arch/arm/dts/stm32mp157a-avenger96-u-boot.dtsi | 2 +-
>  arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi       | 2 +-
>  arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi       | 2 +-
>  arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi     | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/dts/stm32mp157a-avenger96-u-boot.dtsi b/arch/arm/dts/stm32mp157a-avenger96-u-boot.dtsi
> index 1104a70a65..d8a4617d90 100644
> --- a/arch/arm/dts/stm32mp157a-avenger96-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp157a-avenger96-u-boot.dtsi
> @@ -91,7 +91,7 @@
>  		CLK_UART6_HSI
>  		CLK_UART78_HSI
>  		CLK_SPDIF_PLL4P
> -		CLK_FDCAN_PLL4Q
> +		CLK_FDCAN_PLL4R
>  		CLK_SAI1_PLL3Q
>  		CLK_SAI2_PLL3Q
>  		CLK_SAI3_PLL3Q
> diff --git a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> index 4045a6e731..a7a125c087 100644
> --- a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> @@ -110,7 +110,7 @@
>  		CLK_UART6_HSI
>  		CLK_UART78_HSI
>  		CLK_SPDIF_PLL4P
> -		CLK_FDCAN_PLL4Q
> +		CLK_FDCAN_PLL4R
>  		CLK_SAI1_PLL3Q
>  		CLK_SAI2_PLL3Q
>  		CLK_SAI3_PLL3Q
> diff --git a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
> index b2ac49472a..32d95b84e7 100644
> --- a/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp157c-ed1-u-boot.dtsi
> @@ -107,7 +107,7 @@
>  		CLK_UART6_HSI
>  		CLK_UART78_HSI
>  		CLK_SPDIF_PLL4P
> -		CLK_FDCAN_PLL4Q
> +		CLK_FDCAN_PLL4R
>  		CLK_SAI1_PLL3Q
>  		CLK_SAI2_PLL3Q
>  		CLK_SAI3_PLL3Q
> diff --git a/arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi b/arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi
> index 320912edd8..21aa4bfb86 100644
> --- a/arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi
> @@ -142,7 +142,7 @@
>  		CLK_UART6_HSI
>  		CLK_UART78_HSI
>  		CLK_SPDIF_PLL4P
> -		CLK_FDCAN_PLL4Q
> +		CLK_FDCAN_PLL4R
>  		CLK_SAI1_PLL3Q
>  		CLK_SAI2_PLL3Q
>  		CLK_SAI3_PLL3Q

Reviewed-by: Patrice Chotard <patrice.chotard@st.com>

Thanks

Hi,

> From: Patrick DELAUNAY <patrick.delaunay@st.com>
> Sent: mardi 28 janvier 2020 10:11
> 
> From: Antonio Borneo <antonio.borneo@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.
> 
> Signed-off-by: Antonio Borneo <antonio.borneo@st.com>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---

Applied to u-boot-stm32/master, thanks!
 
 Regards
 Patrick