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

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

[Andrey Smirnov]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Rob Herring]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Andrey Smirnov]

On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Rob Herring]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  .../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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Alexandre Belloni]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Andrey Smirnov]

On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Andrey Smirnov]

On Tue, Jun 21, 2016 at 7:34 PM, Andrey Smirnov
<andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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


Alexandre, what's the status of this discussion/patchset? Would it be
more acceptable if I dropped all of the refactoring and just kept the
code adding new feature + DT binding for it? I am more than happy to
drop all but essential parts of the patches if that would allow us to
make progress.

Thank you,
Andrey

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Alexandre Belloni]

On 21/06/2016 at 19:34:56 -0700, Andrey Smirnov wrote :
> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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?
> 

Well, the issue is not being dynamic, it is differentiating between
hardware description and user configuration. Configuration must not be in
DT. And this choice is definitively not hardware related (as opposed to
the trickle charging that depends on the battery that is used on the
board).

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

Well, it doesn't leak abstraction as long as all the RTC that are able
to disable the oscillator failure detection use the same ABI.

> >  - on subsequent reboots, the mode is kept alongside the time and date
> >
> 
> This assumes that your bootloader leaves those mode bits alone.
> 

Well, if that is not the case, the bootloader as to be fixed anyway and
silently changing the configuration back using DT is probably worse.

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

I was reacting to Rob's idea to running in the lower power mode when
suspend. I'm pretty sure this is not a good idea. I'd leave the whole
policy to userspace.

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Andrey Smirnov]

On Tue, Jul 19, 2016 at 3:47 PM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On 21/06/2016 at 19:34:56 -0700, Andrey Smirnov wrote :
>> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni
>> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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?
>>
>
> Well, the issue is not being dynamic, it is differentiating between
> hardware description and user configuration. Configuration must not be in
> DT.

Why? And I don't mean in a generic sense, but in this particular case.
What is gained by not having this bit of configuration, whose only
consumer is the driver, in the device tree file?

> And this choice is definitively not hardware related (as opposed to
> the trickle charging that depends on the battery that is used on the
> board).

There's most certainly plenty of precedents of non hardware-related in
device tree, first two that come to mind are "chosen" node and
"local-mac-address" property and, granted, those might be
exceptions/legacy bindings that are just there to stay, but even if
you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and
possibly rtc-st-lpc.txt are providing bindings that are not exactly
hardware related.

Rtc-cmos.txt is especially noticeable example since it literally does
what I am trying to do -- allows the user to specify initial values to
certain registers that would be initialized by the driver.

>
>> > 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
>>
>
> Well, it doesn't leak abstraction as long as all the RTC that are able
> to disable the oscillator failure detection use the same ABI.

Correct me if I am wrong, but no such, at least semi-standardized, ABI
exist as of today, correct? If that is so, then what you are saying is
that the abstraction doesn't leak as long as you put it inside of a
new abstraction that doesn't leak. I am not arguing that it is
impossible to create a new one that would allow to hide hardware
differences, I am positive it is, what I am arguing is that to do so
is a lot of work for as far as I can see for no gain.

>
>> >  - on subsequent reboots, the mode is kept alongside the time and date
>> >
>>
>> This assumes that your bootloader leaves those mode bits alone.
>>
>
> Well, if that is not the case, the bootloader as to be fixed anyway and
> silently changing the configuration back using DT is probably worse.
>

How so? Consider the following two scenarios with assumption that the
bootloader is broken:

  - Bits set wrong by bootloader, then corrected by kernel, device is
powered off RTC consumes expected amount of current

  - Bits set wrong by bootloader, kernel does nothing, device is
powered off RTC consumes more than anticipated and we drain the power
storage device and loose time.

What do you you think former is worse than latter?

Andrey

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Alexandre Belloni]

On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
> >> 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?
> >>
> >
> > Well, the issue is not being dynamic, it is differentiating between
> > hardware description and user configuration. Configuration must not be in
> > DT.
> 
> Why? And I don't mean in a generic sense, but in this particular case.
> What is gained by not having this bit of configuration, whose only
> consumer is the driver, in the device tree file?
> 

Because configuration doesn't belong to DT. DT is about hardware
description, not configuration.


> > And this choice is definitively not hardware related (as opposed to
> > the trickle charging that depends on the battery that is used on the
> > board).
> 
> There's most certainly plenty of precedents of non hardware-related in
> device tree, first two that come to mind are "chosen" node and
> "local-mac-address" property and, granted, those might be
> exceptions/legacy bindings that are just there to stay, but even if
> you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and
> possibly rtc-st-lpc.txt are providing bindings that are not exactly
> hardware related.
>
> Rtc-cmos.txt is especially noticeable example since it literally does
> what I am trying to do -- allows the user to specify initial values to
> certain registers that would be initialized by the driver.
> 

Well, the RTC subsystem has been left unmaintained for a while and it is
not because we made mistakes in the past that we have to make them
again.

rtc-st-lpc is only hardware related. The mode it is used in is board
dependant. And I have a plan to change how the gpbr register is
allocated for the at91 RTT. I agree that rtc-cmos is an example of what
not to do.

> > Well, it doesn't leak abstraction as long as all the RTC that are able
> > to disable the oscillator failure detection use the same ABI.
> 
> Correct me if I am wrong, but no such, at least semi-standardized, ABI
> exist as of today, correct? If that is so, then what you are saying is
> that the abstraction doesn't leak as long as you put it inside of a
> new abstraction that doesn't leak. I am not arguing that it is
> impossible to create a new one that would allow to hide hardware
> differences, I am positive it is, what I am arguing is that to do so
> is a lot of work for as far as I can see for no gain.
> 

You are correct, that ABI doesn't exist and I'm asking to create it.
That is how kernel development happens.

> >
> >> >  - on subsequent reboots, the mode is kept alongside the time and date
> >> >
> >>
> >> This assumes that your bootloader leaves those mode bits alone.
> >>
> >
> > Well, if that is not the case, the bootloader as to be fixed anyway and
> > silently changing the configuration back using DT is probably worse.
> >
> 
> How so? Consider the following two scenarios with assumption that the
> bootloader is broken:
> 
>   - Bits set wrong by bootloader, then corrected by kernel, device is
> powered off RTC consumes expected amount of current
> 
>   - Bits set wrong by bootloader, kernel does nothing, device is
> powered off RTC consumes more than anticipated and we drain the power
> storage device and loose time.
> 
> What do you you think former is worse than latter?
> 

Whether is is done in the kernel or in userspace doesn't change much
to that use case.



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

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

[Andrey Smirnov]

On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
>> >> 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?
>> >>
>> >
>> > Well, the issue is not being dynamic, it is differentiating between
>> > hardware description and user configuration. Configuration must not be in
>> > DT.
>>
>> Why? And I don't mean in a generic sense, but in this particular case.
>> What is gained by not having this bit of configuration, whose only
>> consumer is the driver, in the device tree file?
>>
>
> Because configuration doesn't belong to DT. DT is about hardware
> description, not configuration.

That doesn't really answer my question. You just re-iterating some
maxim without explaining what is the point behind applying it.

>
>
>> > And this choice is definitively not hardware related (as opposed to
>> > the trickle charging that depends on the battery that is used on the
>> > board).
>>
>> There's most certainly plenty of precedents of non hardware-related in
>> device tree, first two that come to mind are "chosen" node and
>> "local-mac-address" property and, granted, those might be
>> exceptions/legacy bindings that are just there to stay, but even if
>> you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and
>> possibly rtc-st-lpc.txt are providing bindings that are not exactly
>> hardware related.
>>
>> Rtc-cmos.txt is especially noticeable example since it literally does
>> what I am trying to do -- allows the user to specify initial values to
>> certain registers that would be initialized by the driver.
>>
>
> Well, the RTC subsystem has been left unmaintained for a while and it is
> not because we made mistakes in the past that we have to make them
> again.
>
> rtc-st-lpc is only hardware related. The mode it is used in is board
> dependant. And I have a plan to change how the gpbr register is
> allocated for the at91 RTT. I agree that rtc-cmos is an example of what
> not to do.
>
>> > Well, it doesn't leak abstraction as long as all the RTC that are able
>> > to disable the oscillator failure detection use the same ABI.
>>
>> Correct me if I am wrong, but no such, at least semi-standardized, ABI
>> exist as of today, correct? If that is so, then what you are saying is
>> that the abstraction doesn't leak as long as you put it inside of a
>> new abstraction that doesn't leak. I am not arguing that it is
>> impossible to create a new one that would allow to hide hardware
>> differences, I am positive it is, what I am arguing is that to do so
>> is a lot of work for as far as I can see for no gain.
>>
>
> You are correct, that ABI doesn't exist and I'm asking to create it.
> That is how kernel development happens.

OK, I don't think this is going anywhere. I am sure by now you know
very well, that I am not going to agree to that. I'll just drop this
part, gut the patchset to it's bare minimum and re-submit it.

Andrey

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Alexandre Belloni]

On 20/07/2016 at 07:36:55 -0700, Andrey Smirnov wrote :
> On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni
> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
> >> >> 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?
> >> >>
> >> >
> >> > Well, the issue is not being dynamic, it is differentiating between
> >> > hardware description and user configuration. Configuration must not be in
> >> > DT.
> >>
> >> Why? And I don't mean in a generic sense, but in this particular case.
> >> What is gained by not having this bit of configuration, whose only
> >> consumer is the driver, in the device tree file?
> >>
> >
> > Because configuration doesn't belong to DT. DT is about hardware
> > description, not configuration.
> 
> That doesn't really answer my question. You just re-iterating some
> maxim without explaining what is the point behind applying it.
> 

Well, that is from the device tree specification and how the device tree
maintainers want it...

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Andrey Smirnov]

On Wed, Jul 20, 2016 at 8:38 AM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On 20/07/2016 at 07:36:55 -0700, Andrey Smirnov wrote :
>> On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni
>> <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote :
>> >> >> 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?
>> >> >>
>> >> >
>> >> > Well, the issue is not being dynamic, it is differentiating between
>> >> > hardware description and user configuration. Configuration must not be in
>> >> > DT.
>> >>
>> >> Why? And I don't mean in a generic sense, but in this particular case.
>> >> What is gained by not having this bit of configuration, whose only
>> >> consumer is the driver, in the device tree file?
>> >>
>> >
>> > Because configuration doesn't belong to DT. DT is about hardware
>> > description, not configuration.
>>
>> That doesn't really answer my question. You just re-iterating some
>> maxim without explaining what is the point behind applying it.
>>
>
> Well, that is from the device tree specification and how the device tree
> maintainers want it...

And yet, we have whole subsystems such as "nvmem" and I am sure plenty
of other smaller examples where that maxim is being applied very lax
if at all. So it seems people can and do make compromises regarding
the "no configuration in DT" rule if pros of doing so outweighs the
cons.

Andrey

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Andrey Smirnov]

> 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