Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422624 > unrolled thread

[PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-15 08:10 +0200
    Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving  options Rob Herring <robh@kernel.org> - 2016-06-19 16:40 +0200
      Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-19 20:20 +0200
        Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving  options Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-21 23:10 +0200
          Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-22 04:40 +0200
        Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options Andrey Smirnov <andrew.smirnov@gmail.com> - 2016-06-22 01:30 +0200
        Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving  options Rob Herring <robh@kernel.org> - 2016-06-22 05:30 +0200

#1422624 — [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

Add DS1341 specific power-saving options that allow to disable certain
functional aspects of the chip in order to minimize its power
consumption.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
 drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt

diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
new file mode 100644
index 0000000..b8be7a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
@@ -0,0 +1,23 @@
+* Dallas DS1341		I2C Serial Real-Time Clock
+
+Required properties:
+
+- compatible: Should contain "dallas,ds1341".
+
+- reg: I2C address for chip
+
+Optional properties:
+
+- disable-oscillator-stop-flag : Configure chip to disable oscillator
+  fault detection circuitry
+
+- enable-glitch-filter : Configure chip to enable crystal oscillator
+  output glitch filtering
+
+Example:
+	ds1341: rtc@68 {
+		compatible = "dallas,ds1341";
+		disable-oscillator-stop-flag;
+		enable-glitch-filter;
+		reg = <0x68>;
+	};
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index c618c22..54cc527 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -78,6 +78,7 @@ enum ds_type {
 #define DS1337_REG_CONTROL	0x0e
 #	define DS1337_BIT_nEOSC		0x80
 #	define DS1339_BIT_BBSQI		0x20
+#	define DS1341_BIT_EGFIL		0x20
 #	define DS3231_BIT_BBSQW		0x40 /* same as BBSQI */
 #	define DS1337_BIT_RS2		0x10
 #	define DS1337_BIT_RS1		0x08
@@ -93,7 +94,9 @@ enum ds_type {
 #	define DS1340_BIT_OSF		0x80
 #define DS1337_REG_STATUS	0x0f
 #	define DS1337_BIT_OSF		0x80
+#	define DS1341_BIT_DOSF		0x40
 #	define DS3231_BIT_EN32KHZ	0x08
+#	define DS1341_BIT_ECLK		0x04
 #	define DS1337_BIT_A2I		0x02
 #	define DS1337_BIT_A1I		0x01
 #define DS1339_REG_ALARM1_SECS	0x07
@@ -1319,6 +1322,31 @@ static int ds1307_probe(struct i2c_client *client,
 		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
 			ds1307->regs[0] &= ~DS1337_BIT_nEOSC;
 
+		if (ds1307->type == ds_1341) {
+			/* Make sure we are not generating square wave
+			 * output */
+			ds1307->regs[1] &= ~DS1341_BIT_ECLK;
+
+			if (of_property_read_bool(client->dev.of_node,
+						  "disable-oscillator-stop-flag"))
+				ds1307->regs[1] |= DS1341_BIT_DOSF;
+			else
+				ds1307->regs[1] &= ~DS1341_BIT_DOSF;
+
+			if (of_property_read_bool(client->dev.of_node,
+						  "enable-glitch-filter"))
+				ds1307->regs[0] |= DS1341_BIT_EGFIL;
+			else
+				ds1307->regs[0] &= ~DS1341_BIT_EGFIL;
+
+			/*
+			 * Write status register. Control register
+			 * would be set by the code below
+			 */
+			i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
+						  ds1307->regs[1]);
+		}
+
 		/*
 		 * Disable the square wave and both alarms.
 		 * For some variants, be sure alarms can trigger when we're
-- 
2.5.5

[toc] | [next] | [standalone]

#1425979 — Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote:
> Add DS1341 specific power-saving options that allow to disable certain
> functional aspects of the chip in order to minimize its power
> consumption.

This description doesn't match that you are adding a new binding. It is 
preferred that bindings are a separate patch.

> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
>  drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> new file mode 100644
> index 0000000..b8be7a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> @@ -0,0 +1,23 @@
> +* Dallas DS1341		I2C Serial Real-Time Clock
> +
> +Required properties:
> +
> +- compatible: Should contain "dallas,ds1341".
> +
> +- reg: I2C address for chip
> +
> +Optional properties:
> +
> +- disable-oscillator-stop-flag : Configure chip to disable oscillator
> +  fault detection circuitry
> +
> +- enable-glitch-filter : Configure chip to enable crystal oscillator
> +  output glitch filtering

What determines setting these properties or not?

They should have vendor prefix and be explicit that they are boolean.

Rob

[toc] | [prev] | [next] | [standalone]

#1426019

On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote:
>> Add DS1341 specific power-saving options that allow to disable certain
>> functional aspects of the chip in order to minimize its power
>> consumption.
>
> This description doesn't match that you are adding a new binding. It is
> preferred that bindings are a separate patch.

OK, will split this patch into two in v2.

>
>>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
>>  drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
>>  2 files changed, 51 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
>> new file mode 100644
>> index 0000000..b8be7a4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
>> @@ -0,0 +1,23 @@
>> +* Dallas DS1341              I2C Serial Real-Time Clock
>> +
>> +Required properties:
>> +
>> +- compatible: Should contain "dallas,ds1341".
>> +
>> +- reg: I2C address for chip
>> +
>> +Optional properties:
>> +
>> +- disable-oscillator-stop-flag : Configure chip to disable oscillator
>> +  fault detection circuitry
>> +
>> +- enable-glitch-filter : Configure chip to enable crystal oscillator
>> +  output glitch filtering
>
> What determines setting these properties or not?

Setting those properties allows drastically reduce RTC's power
consumption at the expense of reliability and quality of service. In
my use case, DS1341 is powered by a supercap and enabling those two
setting allows to increase holdup from less than a day do 2+ weeks.

>
> They should have vendor prefix and be explicit that they are boolean.

I was trying to be consistent with ds1339 and ds1390 bindings which do
not have vendor prefixes. Will fix in v2.

Thank you,
Andrey Smirnov

[toc] | [prev] | [next] | [standalone]

#1428164 — Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
> So wouldn't you want to set one mode while running and the lower power 
> mode while suspended? I'm trying to understand the frequency of changing 
> this. If it is always one setting for a board, then yes it belongs in 
> DT. If it is a user decision, then it probably shouldn't be in DT.
> 
> Seeing as these are reused, I've probably already had this discussion...
> 

I would agree with Rob here. It may be better to provide a sysfs
interface to configure that particular behavior. This is usually ok
because the use case is:
 - the RTC is not configured, time has never been set
 - time is set for the first time
 - the user can set the oscillator mode/detection/...
 - on subsequent reboots, the mode is kept alongside the time and date

I would advise against trying to set a mode automatically in the driver
because you may have unexpected power cuts and it may then let the RTC
consume more power than what you really want.

> > > They should have vendor prefix and be explicit that they are boolean.
> > 
> > I was trying to be consistent with ds1339 and ds1390 bindings which do
> > not have vendor prefixes. Will fix in v2.
> 
> Okay, then they are fine if you are using existing properties. Perhaps 
> these should all be in a common binding doc though.
> 

I'll try to collect the existing common properties and write that doc
this week.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1428357

On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote :
>> So wouldn't you want to set one mode while running and the lower power
>> mode while suspended? I'm trying to understand the frequency of changing
>> this. If it is always one setting for a board, then yes it belongs in
>> DT. If it is a user decision, then it probably shouldn't be in DT.
>>
>> Seeing as these are reused, I've probably already had this discussion...
>>
>
> I would agree with Rob here. It may be better to provide a sysfs
> interface to configure that particular behavior.

I don't see any value in doing that, could you give me a realistic
example of a scenario in which a user would want to spend some of
uptime with RTC oscillator fault detection/glitch filtering disabled
and then enable it?

> This is usually ok because the use case is:
>  - the RTC is not configured, time has never been set
>  - time is set for the first time
>  - the user can set the oscillator mode/detection/...

Unfortunately exposing that feature using sysfs gives you a leaky
abstraction and your userspace instead of using a generic RTC starts
using DS1341 RTC. So to accommodate for that a user would have to:

a) Write + integrate a userspace tool to set the mode (which IMHO is
decided upon once and doesn't change)
b) If a board design is new and there's a chance of moving this chip
to a different I2C bus, the code above would have to account for that
and not hardcore sysfs path
c) If board's BSP is intended to be used in multiple generations of a
product, not all of which would use DS1341, it would be necessary to
accommodate for that by either more code in the tool or an additional
BSP build configuration variant

>  - on subsequent reboots, the mode is kept alongside the time and date
>

This assumes that your bootloader leaves those mode bits alone.

> I would advise against trying to set a mode automatically in the driver
> because you may have unexpected power cuts and it may then let the RTC
> consume more power than what you really want.
>

I fell like I am not understanding you correctly.  Why would moving
configuration decision logic into userspace improve the situation in
case of unexpected power loss?

Andrey

[toc] | [prev] | [next] | [standalone]

#1428261

> So wouldn't you want to set one mode while running and the lower power
> mode while suspended? I'm trying to understand the frequency of changing
> this. If it is always one setting for a board, then yes it belongs in
> DT. If it is a user decision, then it probably shouldn't be in DT.

I don't really see a use-case where you'd want setting that
dynamically. I might be wrong, but IMHO, power consumption of a system
in suspended mode would dwarf that of an RTC, so there's really much
to gain by enabling this feature dynamically. Where it matters the
most is time setting retention when the system is powered off and RTC
is ticking off of a battery or a some other power storage device. So
in my opinion it is more of a system design question where one has to
choose if reliability of RTC data is more important (detection of
oscillator faults and higher oscillator glitch immunity) and bigger
power storage device is needed or higher risk of RTC "malfunction" is
acceptable and cheaper/more convenient power storage device can be
used

>
> Seeing as these are reused, I've probably already had this discussion...
>
>> > They should have vendor prefix and be explicit that they are boolean.
>>
>> I was trying to be consistent with ds1339 and ds1390 bindings which do
>> not have vendor prefixes. Will fix in v2.
>
> Okay, then they are fine if you are using existing properties. Perhaps
> these should all be in a common binding doc though.

I think we have a misunderstanding. What I meant by "trying to be
consistent" was that bindings for other DS1307 variants do not prefix
their own properties with vendor name. Your comment about properties
being reused makes me suspicious that I misled you to believe that
other chip variants use those exact properties which is not the case.

Andrey

[toc] | [prev] | [next] | [standalone]

#1428385 — Re: [PATCH 03/13] RTC: ds1307: Add DS1341 specific power-saving options

On Sun, Jun 19, 2016 at 11:12:55AM -0700, Andrey Smirnov wrote:
> On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote:
> >> Add DS1341 specific power-saving options that allow to disable certain
> >> functional aspects of the chip in order to minimize its power
> >> consumption.
> >
> > This description doesn't match that you are adding a new binding. It is
> > preferred that bindings are a separate patch.
> 
> OK, will split this patch into two in v2.
> 
> >
> >>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> ---
> >>  .../devicetree/bindings/rtc/dallas,ds1341.txt      | 23 ++++++++++++++++++
> >>  drivers/rtc/rtc-ds1307.c                           | 28 ++++++++++++++++++++++
> >>  2 files changed, 51 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> >> new file mode 100644
> >> index 0000000..b8be7a4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt
> >> @@ -0,0 +1,23 @@
> >> +* Dallas DS1341              I2C Serial Real-Time Clock
> >> +
> >> +Required properties:
> >> +
> >> +- compatible: Should contain "dallas,ds1341".
> >> +
> >> +- reg: I2C address for chip
> >> +
> >> +Optional properties:
> >> +
> >> +- disable-oscillator-stop-flag : Configure chip to disable oscillator
> >> +  fault detection circuitry
> >> +
> >> +- enable-glitch-filter : Configure chip to enable crystal oscillator
> >> +  output glitch filtering
> >
> > What determines setting these properties or not?
> 
> Setting those properties allows drastically reduce RTC's power
> consumption at the expense of reliability and quality of service. In
> my use case, DS1341 is powered by a supercap and enabling those two
> setting allows to increase holdup from less than a day do 2+ weeks.

So wouldn't you want to set one mode while running and the lower power 
mode while suspended? I'm trying to understand the frequency of changing 
this. If it is always one setting for a board, then yes it belongs in 
DT. If it is a user decision, then it probably shouldn't be in DT.

Seeing as these are reused, I've probably already had this discussion...

> > They should have vendor prefix and be explicit that they are boolean.
> 
> I was trying to be consistent with ds1339 and ds1390 bindings which do
> not have vendor prefixes. Will fix in v2.

Okay, then they are fine if you are using existing properties. Perhaps 
these should all be in a common binding doc though.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web