Message-ID: <2sm6owthrdao6jzsrokccw54dg4xm5c57ertzotbk3kxorbwlb@gvidv6e5o2pl>
Date: Mon, 26 Aug 2024 11:48:34 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: Eric Chanudet <echanude@...hat.com>
Cc: Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>, 
	Tero Kristo <kristo@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: ti: k3-j784s4-main: align watchdog clocks

On Tue, Aug 20, 2024 at 06:01:34PM GMT, Eric Chanudet wrote:
> On Mon, Aug 05, 2024 at 01:42:51PM GMT, Eric Chanudet wrote:
> > assigned-clock sets DEV_RTIx_RTI_CLK(id:0) whereas clocks sets
> > DEV_RTIx_RTI_CLK_PARENT_GLUELOGIC_HFOSC0_CLKOUT(id:1)[1]. This does not
> > look right, the timers in the driver assume a max frequency of 32kHz for
> > the heartbeat (HFOSC0 is 19.2MHz on j784s4-evm).
> > 
> > With this change, WDIOC_GETTIMELEFT return coherent time left
> > (DEFAULT_HEARTBEAT=60, reports 60s upon opening the cdev).
> > 
> > [1] http://downloads.ti.com/tisci/esd/latest/5_soc_doc/j784s4/clocks.html#clocks-for-rti0-device
> > 
> > Fixes: caae599de8c6 ("arm64: dts: ti: k3-j784s4-main: Add the main domain watchdog instances")
> > Suggested-by: Andrew Halaney <ahalaney@...hat.com>
> > Signed-off-by: Eric Chanudet <echanude@...hat.com>
> 
> Gentle ping and update to the test comment.
> 
> > ---
> > I could not get the watchdog to do more than reporting 0x32 in
> > RTIWDSTATUS. Setting RTIWWDRXCTRL[0:3] to generate a reset instead of an
> > interrupt (0x5) didn't trigger a reset either when the window expired.
> 
> Re-testing using u-boot from the BSP (2023.04) has the board reset as
> expected when the watchdog expires and WDIOC_GETTIMELEFT report the time
> left coherently with this patch until that happens.
> 
> I initially had a u-boot with a DT lacking:
> 	"mcu_esm: esm@...00000"
> and I could reproduce the board not resetting by commenting in its
> description:
> 	"ti,esm-pins = <95>;"
> 
> I don't understand why that is on the other hand. The TRM says ESM0
> ERR_O drives the SOC_SAFETY_ERRORn pin, which goes to the PMIC GPIO3 on
> the schematic _and_ to MCU_ESM0 as an error input event. The tps6594-esm
> module is probing successfully and it sets both ESM_SOC_EN|ESM_SOC_ENDRV
> and ESM_SOC_START, so I would expect the PMIC to reset the board without
> MCU_ESM0 being described or configured by u-boot.
> 

So, just so I understand (after writing/reading this ten times basically
I think we're saying the same thing, with mine just being 10x the words
:P )... This patch is needed to fix the clock for RTIx's else they tick at
the wrong speed and things go wrong.

Further with respect to actually resetting the board...
When an RTIx wdog trips, it sends the error to the associated
ESM (MAIN, MCU, WKUP, depending on the RTI instance). The ESM's need
to be programmed to take that as an error input ("ti,esm-pins").
The 3 ESMs are daisy chained too, and we assume that ti,esm-pins = <95>
describes the MCU_ESM0 and ESM0 connection (IIRC we couldn't find that explictly
documented, in fact we think that pin is not described in the TRM docs but
the others next to it were...).

The MAIN ESM, and the WKUP ESM, have a SOC_SAFETY_ERRORn/MCU_SAFETY_ERRORn
output from the SoC as well. That goes to tps6594's GPIO3/GPIO7. So if the
daisy chaining is setup you get ESM0 kicking all the way to WKUP_ESM0
which asserts out GPIO7/MCU_SAFETY_ERRORn. Without the daisy chaining you just
get GPIO3/SOC_SAFETY_ERRORn.

The tps6594 has a state machine (ESM again) that will reset the SoC
if those lines are asserted for long enough. That state machine needs
programming. Upstream linux programs that with
tps6594-esm + ESM_SOC_EN + ESM_SOC_START. The MCU_SOC_EN etc bits are only
referenced in upstream u-boots esm_pmic.c (using compatible = "ti,tps659413-esm").
At the moment, that's not setup for the j784s4 in upstream u-boot:
    halaney@...en2nano ~/git/u-boot (git)-[master] % git grep tps659413-esm
    arch/arm/dts/k3-j721e-r5-common-proc-board.dts:                 compatible = "ti,tps659413-esm";
    arch/arm/dts/k3-j721e-r5-sk.dts:                        compatible = "ti,tps659413-esm";
    ...
but it is in the BSP u-boot version you mentioned:
    tisdk@...a76441f0d:~/yocto-build/build/arago-tmp-default-baremetal-k3r5/work/j784s4_evm_k3r5-oe-eabi/u-boot-ti-staging/1_2023.04+gitAUTOINC+f9b966c674-r0_tisdk_3_edgeai_4/git$ git grep tps659413-esm | grep j784s4
    arch/arm/dts/k3-j784s4-r5-evm.dts:			compatible = "ti,tps659413-esm";

You (and I) were expecting to that even without the ESM daisy chaining,
the RTI0 instance if tripped would assert to pin 688 to ESM0 (which needs
to be programmed for ESM0 to pay attention to), and that would assert
SOC_SAFETY_ERRORn to tps6594's GPIO3, which Linux should have finished the
setup for to ensure the state machine is ran thru and the board is reset...
but you did not see that happen in practice, and needed to ensure MCU_ESM0
(via pin 95) was also programmed to get the board reset.

Dumb question, but there's so many pieces and domains at play I'll ask, did
you ensure that ESM0 was programmed in this test? Right now if you're using
upstream u-boot and upstream linux, nobody seems to be configured by default
to do that (u-boot needs to do that with k3_esm.c - meaning the defconfig needs
CONFIG_ESM_K3 && the dts needs ti,j721-esm with the RTI0-7 pins (688-695)
described). I think you probably did (since it sounds like adding the
daisy chaining pin 95 description back in made things reset), but just
making sure since I'm getting confused about the contexts here!

Below I grep through upstream u-boot to see if CONFIG_ESM_K3 is set
for j784s4 + to see if the devicetree used describes the ESMs:

    ahalaney@...en2nano ~/git/u-boot (git)-[master] % # Note how j784s4 isn't in the list... also note the r5 handles this usually on other socs             :(
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git grep ESM_K3                                                                                        :(
    ...
    configs/am62x_beagleplay_r5_defconfig:CONFIG_ESM_K3=y
    configs/am62x_evm_r5_defconfig:CONFIG_ESM_K3=y
    configs/am64x_evm_r5_defconfig:CONFIG_ESM_K3=y
    configs/j721e_evm_r5_defconfig:CONFIG_ESM_K3=y
    configs/phycore_am62x_r5_defconfig:CONFIG_ESM_K3=y
    configs/phycore_am64x_r5_defconfig:CONFIG_ESM_K3=y
    configs/verdin-am62_r5_defconfig:CONFIG_ESM_K3=y
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git grep DEVICE_TREE configs/j784s4_evm_r5_defconfig
    configs/j784s4_evm_r5_defconfig:CONFIG_DEFAULT_DEVICE_TREE="k3-j784s4-r5-evm"
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git grep j721e-esm | grep j784s4                                                                     :(
    dts/upstream/src/arm64/ti/k3-j784s4-main.dtsi:		compatible = "ti,j721e-esm";
    dts/upstream/src/arm64/ti/k3-j784s4-mcu-wakeup.dtsi:		compatible = "ti,j721e-esm";
    dts/upstream/src/arm64/ti/k3-j784s4-mcu-wakeup.dtsi:		compatible = "ti,j721e-esm";
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git grep k3-j784s4-mcu-wakeup.dtsi
    dts/upstream/src/arm64/ti/k3-j784s4.dtsi:#include "k3-j784s4-mcu-wakeup.dtsi"
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git grep k3-j784s4.dtsi
    dts/upstream/src/arm64/ti/k3-am69-sk.dts:#include "k3-j784s4.dtsi"
    dts/upstream/src/arm64/ti/k3-j784s4-evm.dts:#include "k3-j784s4.dtsi"
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git grep k3-j784s4-evm.dts
    arch/arm/dts/k3-j784s4-r5-evm.dts:#include "k3-j784s4-evm.dts"
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % # Just to make sure there's not another file with that name in the tree..
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % git ls-files | grep k3-j784s4-evm.dts
    dts/upstream/src/arm64/ti/k3-j784s4-evm.dts
    ahalaney@...en2nano ~/git/u-boot (git)-[master] % # Nope, ok, j784s4 r5 dts inherits the upstream one!

So upstream u-boot currently doesn't touch the ESMs on j784s4 at all as
far as I can tell due to lacking CONFIG_ESM_K3.

With all that in my scrambled mind, I feel we need to:

1. Take this patch to fix the RTI clocks and make that block work
   appropriately
2. Enable the ESM_K3 in upstream u-boot for the r5 build

The u-boot bits above are already done in the BSP u-boot, which is why I suppose things
work:
    tisdk@...a76441f0d:$ # Verify the r5 dts in the BSP describes the ESMs
    tisdk@...a76441f0d:$ git grep DEVICE_TREE configs/j784s4_evm_r5_defconfig 
    configs/j784s4_evm_r5_defconfig:CONFIG_DEFAULT_DEVICE_TREE="k3-j784s4-r5-evm"
    tisdk@...a76441f0d:$ git grep ti,j721e-esm | grep j784s4
    arch/arm/dts/k3-j784s4-main.dtsi:		compatible = "ti,j721e-esm";
    arch/arm/dts/k3-j784s4-mcu-wakeup.dtsi:		compatible = "ti,j721e-esm";
    arch/arm/dts/k3-j784s4-mcu-wakeup.dtsi:		compatible = "ti,j721e-esm";
    tisdk@...a76441f0d:$ git grep k3-j784s4-mcu-wakeup.dtsi 
    arch/arm/dts/k3-j784s4.dtsi:#include "k3-j784s4-mcu-wakeup.dtsi"
    tisdk@...a76441f0d:$ git grep k3-j784s4.dtsi
    arch/arm/dts/k3-am69-sk.dts:#include "k3-j784s4.dtsi"
    arch/arm/dts/k3-j784s4-evm.dts:#include "k3-j784s4.dtsi"
    tisdk@...a76441f0d:$ git grep k3-j784s4-evm.dts
    arch/arm/dts/k3-j784s4-r5-evm.dts:#include "k3-j784s4-evm.dts"
    # verify that the driver is built
    tisdk@...a76441f0d:$ git grep ESM_K3 | grep j784s4
    board/ti/j784s4/evm.c:	if (IS_ENABLED(CONFIG_ESM_K3)) {
    configs/j784s4_evm_r5_defconfig:CONFIG_ESM_K3=y

There's also that board file bit right above this comment, but I'm not
entirely sure if that's needed or not. Its not in upstream u-boot at least
for j784s4 (but is for j721e).

Here's how I view the connections mentally in case that garbled mess
above makes no sense. RTI0 is driven by the MAIN domain with linux,
ESMs are driven (if they are at all) by the MCU/r5 domains in u-boot,
TPS6594 is driven by linux wrt GPIO3's state machine, u-boot (if at all)
wrt GPIO7's state machine:

rti0 ---> ESM0 pin 688 --SOC_SAFETY_ERRORn--> TPS6594 GPIO3
				|
				|
				--> MCU_ESM0 pin 95 --> WKUP_ESM0 pin 63 --MCU_SAFETY_ERRORn--> TPS6584 GPIO7

Please let me know if that matches your view Eric... if so hopefully someone
from TI who's a little more intimate with the platform can comment on our view
and we can wrap this up for all the different contexts at play here!

Thanks,
Andrew

Powered by blists - more mailing lists