On 14.04.2020 14:16, Alexandre Belloni wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 14/04/2020 08:42:08+0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote: >>> Why would one use the RTT while the RTC is far superior? >> >> I didn't enabled this for a particular use case, but: couldn't this be used >> by some user that wants to generate multiple alarms? from multiple RTCs? >> > > I very much doubt that as Linux is able to properly multiplex alarms and > basically, the only one we are interested in is actually wakeup. I think you can use the wakealarm sysfs exported file to prepare an alarm and take user space actions based on that without being suspended. > >> Moreover, this IP's counter has the possibility of being clocked at 1Hz. >> Couldn't this minimize the power consumption while being in a power saving >> mode? >> > > And that 1Hz clock is coming from the RTC so using the RTC is > definitively consuming less power. Datasheet specifies this: "Configuring the RTPRES field value to 0x8000 (default value) corresponds to feeding the real-time counter with a 1Hz signal (if the slow clock is 32.768 kHz)." So, it is not the RTC, it is the slow clock divided by 32768. > >>> >>>>> >>>>> In any case, this diff should be merge with the other at91-sam9x60ek.dts >>>>> change instead of being with the dtsi change. >>>> >>>> The changes in this patch are related to enabling the RTT. The other dts >>>> change is related to enabling gpbr. The RTT uses that enabled gpbr -> one >>>> change per patch. >>>> >>>> If you still want to merge then, I'll do it, but then it becomes mixed. >>>> >>> >>> This patch is already mixing add the gpbr in sam9x60ek and add the node >>> in sam9x60.dtsi which is worse. >> >> This patch is like this: >> >> diff --git a/arch/arm/boot/dts/at91-sam9x60ek.dts >> b/arch/arm/boot/dts/at91-sam9x60ek.dts >> index ab3d2d9a420a..4020e79a958e 100644 >> --- a/arch/arm/boot/dts/at91-sam9x60ek.dts >> +++ b/arch/arm/boot/dts/at91-sam9x60ek.dts >> @@ -617,6 +617,11 @@ >> }; >> }; >> >> +&rtt { >> + atmel,rtt-rtc-time-reg = <&gpbr 0x0>; >> + status = "okay"; >> +}; >> + >> &shutdown_controller { >> atmel,shdwc-debouncer = <976>; >> status = "okay"; >> diff --git a/arch/arm/boot/dts/sam9x60.dtsi b/arch/arm/boot/dts/sam9x60.dtsi >> index 326b39328b58..e1d8e3a4cb0b 100644 >> --- a/arch/arm/boot/dts/sam9x60.dtsi >> +++ b/arch/arm/boot/dts/sam9x60.dtsi >> @@ -661,6 +661,13 @@ >> status = "disabled"; >> }; >> >> + rtt: rtt@fffffe20 { >> + compatible = "microchip,sam9x60-rtt"; >> + reg = <0xfffffe20 0x20>; >> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>; >> + clocks = <&clk32k 0>; >> + }; >> + >> >> It doesn't adds the GPBR in sam9x60ek, it adds rtt in sam9x60ek which uses >> GPBR. >> >>> >>> Just have one patch adding the rtt node to the sam9x60.dtsi and then a >>> patch adding the RTT to sam9x60ek. >> >> Ok, I understand this. >> >>> Because the RTT uses the gpbr, it is >>> a good time to add enable the gpbr, this is a single functionnal change. >>> >>> Let's say that for some reason, the RTT patch on sam9x60ek has to be >>> reverted, then the RTT node is still defined which is good for all the >>> other eventual users. >> >> RTT node would still be defined but GPBR node will not be enabled. >> >> If RTT patch contains this change that I understand you want me to merge here: >> >> +&gpbr { >> + status = "okay"; >> +}; >> + >> >> then, theoretically, some other IPs using the GPBR (RTC have the >> possibility of doing this), may be broken by reverting the RTT patch that >> includes the GPBR enabling patch. >> > > But this is very unlikely to happen because this would be limited to a > single board device tree instead of impact every sam9x60 based boards. Very unlikely but a having a patch with diff like this: +&gpbr { + status = "okay"; +}; + +&rtt { + atmel,rtt-rtc-time-reg = <&gpbr 0x0>; + status = "okay"; +}; + and reverting it may affect the other users of gpbr in sam9x60ek.dts. > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >