Re: [PATCH v2] ARM: dts: microchip: sam9x60: Fix rtc/rtt clocks

[Alexander Dahl]

Hello Alexandre,

Am Thu, Aug 22, 2024 at 01:52:05AM +0200 schrieb Alexandre Belloni:
> On 21/08/2024 07:51:36+0200, Alexander Dahl wrote:
> > The RTC and RTT peripherals use the "timing domain slow clock (TD_SLCK),
> > sourced from the 32.768 kHz crystal oscillator.
> > 
> > (The previously used Monitoring domain slow clock (MD_SLCK) is sourced
> > from an internal RC oscillator which is most probably not precise enough
> > for real time clock purposes.)
> > 
> > Fixes: 1e5f532c2737 ("ARM: dts: at91: sam9x60: add device tree for soc and board")
> > Fixes: 5f6b33f46346 ("ARM: dts: sam9x60: add rtt")
> > Signed-off-by: Alexander Dahl <ada@xxxxxxxxxxx>
> > ---
> > 
> > Notes:
> >     Picked the wrong patch in the first try.  This v2 one has a slightly
> >     adapted commit message and more context below.
> >     
> >     This obviously requires a 32.768 kHz crystal oscillator to be present,
> >     but the sam9x60.dtsi does contain that, and the clock-controllers
> >     reference that, so I assume it's always present.
> 
> The crystal is optional so this is going to break the boards that don't
> have one. I don't really mind but this should probably be part of the
> commit message.

Okay right, according to the datasheet (Figure 27.1 SCKC Block
Diagram) you don't need that crystal, you can clear TD_OSCSEL and
td_slck runs from the internal rc then.  However, td_slck is always
present, it either sources from the internal slow rc oscillator or the
crystal oscillator.  And the datasheet says in section 29.1 (PMC):

    "The Slow Clock Controller (SCKC) selects the source of TD_SLCK
    (drives the real-time part (RTT/RTC)).  The source of MD_SLCK
    (drives the rest of the system controller: wake-up logic,
    watchdog, PMC, etc.) is always the slow RC oscillator."

md_slck and td_slck are both registered by the at91 sckc driver, and
the td_slck gets two parents in of_sam9x60_sckc_setup() when
registered by at91_clk_register_sam9x5_slow().  The parent can be
switched by clk_sam9x5_slow_set_parent() from sam9x5_slow_ops then,
correctly setting the OSCSEL bit.

The whole idea of the patch is giving the rtc/rtt td_slck as a parent
as documented in the datasheet.  I don't see how this should be
affected by the parents of td_slck?  Am I missing something?

> This makes me realise that we always assumed the RC oscillator was
> running at 32768 while the sam9x60 datasheet refers to it has a 32kHz
> oscillator. However the RTC only has a 32768 divider...

When sourced from the internal rc oscillator, this would mean the
output would be incorrect, right?  How could one prove this?

Greets
Alex

> 
> >     
> >     /sys/kernel/debug/clk/clk_summary content excerpt before:
> >     
> >          slow_rc_osc                         1       1        0        32768       93750000   0     50000      Y   deviceless                      no_connection_id
> >             md_slck                          4       4        0        32768       0          0     50000      Y      fffffea8.rtc                    no_connection_id
> >                                                                                                                       fffffe20.rtc                    no_connection_id
> >                                                                                                                       fffffe10.poweroff               no_connection_id
> >                                                                                                                       fffffe00.reset-controller       no_connection_id
> >                                                                                                                       timer@f8008000                  slow_clk
> >                                                                                                                       deviceless                      no_connection_id
> >     …
> >          slow_xtal                           0       0        0        32768       0          0     50000      Y   deviceless                      no_connection_id
> >             slow_osc                         0       0        0        32768       0          0     50000      Y      deviceless                      no_connection_id
> >                td_slck                       0       0        0        32768       0          0     50000      Y         deviceless                      no_connection_id
> >     
> >     And after:
> >     
> >          slow_rc_osc                         1       1        0        32768       93750000   0     50000      Y   deviceless                      no_connection_id
> >             md_slck                          2       2        0        32768       0          0     50000      Y      fffffe10.poweroff               no_connection_id
> >                                                                                                                       fffffe00.reset-controller       no_connection_id
> >                                                                                                                       timer@f8008000                  slow_clk
> >                                                                                                                       deviceless                      no_connection_id
> >     …
> >          slow_xtal                           1       1        0        32768       0          0     50000      Y   deviceless                      no_connection_id
> >             slow_osc                         1       1        0        32768       0          0     50000      Y      deviceless                      no_connection_id
> >                td_slck                       2       2        0        32768       0          0     50000      Y         fffffea8.rtc                    no_connection_id
> >                                                                                                                          fffffe20.rtc                    no_connection_id
> >                                                                                                                          deviceless                      no_connection_id
> > 
> >  arch/arm/boot/dts/microchip/sam9x60.dtsi | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> > index 291540e5d81e..d077afd5024d 100644
> > --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi
> > +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi
> > @@ -1312,7 +1312,7 @@ rtt: rtc@fffffe20 {
> >  				compatible = "microchip,sam9x60-rtt", "atmel,at91sam9260-rtt";
> >  				reg = <0xfffffe20 0x20>;
> >  				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> > -				clocks = <&clk32k 0>;
> > +				clocks = <&clk32k 1>;
> >  			};
> >  
> >  			pit: timer@fffffe40 {
> > @@ -1338,7 +1338,7 @@ rtc: rtc@fffffea8 {
> >  				compatible = "microchip,sam9x60-rtc", "atmel,at91sam9x5-rtc";
> >  				reg = <0xfffffea8 0x100>;
> >  				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> > -				clocks = <&clk32k 0>;
> > +				clocks = <&clk32k 1>;
> >  			};
> >  
> >  			watchdog: watchdog@ffffff80 {
> > 
> > base-commit: 47ac09b91befbb6a235ab620c32af719f8208399
> > -- 
> > 2.39.2
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com