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

[Alexander Dahl <ada@thorsis.com>]

Hello Alexandre,

Am Thu, Aug 22, 2024 at 10:17:46AM +0200 schrieb Alexandre Belloni:
> On 22/08/2024 08:53:59+0200, Alexander Dahl wrote:
> > 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@thorsis.com>
> > > > ---
> > > > 
> > > > 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?
> > 
> 
> You are right, I got confused because you were referring t the 32768 Hz
> crystal in your commit message and though you aimed at selected the
> parent of td_slck (and also, I didn't really work on the sam9x60).

Thanks for confirming.

Should I reword the commit message then to make it easier to
understand?

> > > 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?
> 
> 
> I guess you could have a look at how the RTC is drifting when selecting
> the RC osc as the parent but it will anyway be way less precise than the
> crystal so i'm not sure how you could get a conclusive result.

I would have to look deeper into rtc for that.
Maybe in a calm minute. ;-)

Greets
Alex

> 
> > 
> > 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
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>