Re: [PATCH 4/5] ARM: dts: sam9x60: add rtt

[Claudiu.Beznea]

On 13.04.2020 13:46, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 13/04/2020 08:51:12+0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
>>
>>
>> On 11.04.2020 01:26, Alexandre Belloni wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 10/04/2020 19:26:58+0300, Claudiu Beznea wrote:
>>>> Add RTT.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>>>> ---
>>>>  arch/arm/boot/dts/at91-sam9x60ek.dts | 5 +++++
>>>>  arch/arm/boot/dts/sam9x60.dtsi       | 7 +++++++
>>>>  2 files changed, 12 insertions(+)
>>>>
>>>> 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";
>>>
>>> Is there any point using a gpbr register while there is already a much
>>> better RTC in the system?
>>
>> Only to have it also enabled.
>>
> 
> 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?

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?

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

> 
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>