| Message ID | 20200720112401.4620-2-miguelborgesdefreitas@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | rtc: pcf8523: imx6qdl-cubox-i: Make DSM for battery switch-over configurable from DT and enable it for the cubox-i | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote: > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > This adds direct-switching mode as a configurable DT flag for > RTC modules supporting it (e.g. nxp pcf8523). > DSM switches the power source to the battery supply whenever the > VDD drops below VBAT. The option is recommended for hw designs > where VDD is always expected to be higher than VBAT. > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > --- > Changes in v2: > - Added extended commit message for git history > - Separate dt bindings documentation into a single patch > > Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++- > Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > index 0b1080c..f715a8f 100644 > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > @@ -4,10 +4,14 @@ Required properties: > - compatible: Should contain "nxp,pcf8523". > - reg: I2C address for chip. > > -Optional property: > +Optional properties: > - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), > expressed in femto Farad (fF). Valid values are 7000 and 12500. > Default value (if no value is specified) is 12500fF. > +- pm-enable-dsm: battery switch-over function is enabled in direct > + switching mode. The power failure condition happens when VDD < VBAT, > + without requiring VDD to drop below Vth(sw)bat. > + Default value (if not provided) is the standard mode. > > Example: > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 { > compatible = "nxp,pcf8523"; > reg = <0x68>; > quartz-load-femtofarads = <7000>; > + pm-enable-dsm; > }; > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml > index ee237b2..a0048f4 100644 > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml > @@ -47,4 +47,11 @@ properties: > description: > Enables wake up of host system on alarm. > > + pm-enable-dsm: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Enables the battery switch-over function in direct switching > + mode. Should be set in systems where VDD is higher than VBAT > + at all times. I'm all for common properties, but is this common across vendors? > + > ... > -- > 1.8.3.1 >
On 23/07/2020 11:49:05-0600, Rob Herring wrote: > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote: > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > This adds direct-switching mode as a configurable DT flag for > > RTC modules supporting it (e.g. nxp pcf8523). > > DSM switches the power source to the battery supply whenever the > > VDD drops below VBAT. The option is recommended for hw designs > > where VDD is always expected to be higher than VBAT. > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > --- > > Changes in v2: > > - Added extended commit message for git history > > - Separate dt bindings documentation into a single patch > > > > Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++- > > Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > index 0b1080c..f715a8f 100644 > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > @@ -4,10 +4,14 @@ Required properties: > > - compatible: Should contain "nxp,pcf8523". > > - reg: I2C address for chip. > > > > -Optional property: > > +Optional properties: > > - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), > > expressed in femto Farad (fF). Valid values are 7000 and 12500. > > Default value (if no value is specified) is 12500fF. > > +- pm-enable-dsm: battery switch-over function is enabled in direct > > + switching mode. The power failure condition happens when VDD < VBAT, > > + without requiring VDD to drop below Vth(sw)bat. > > + Default value (if not provided) is the standard mode. > > > > Example: > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 { > > compatible = "nxp,pcf8523"; > > reg = <0x68>; > > quartz-load-femtofarads = <7000>; > > + pm-enable-dsm; > > }; > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml > > index ee237b2..a0048f4 100644 > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml > > @@ -47,4 +47,11 @@ properties: > > description: > > Enables wake up of host system on alarm. > > > > + pm-enable-dsm: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: > > + Enables the battery switch-over function in direct switching > > + mode. Should be set in systems where VDD is higher than VBAT > > + at all times. > > I'm all for common properties, but is this common across vendors? > This is but this shouldn't be a DT property as it has to be changed dynamically. I'm working on an ioctl interface to change this configuration.
Hi Alexandre, Having a way to dynamically change the configuration would definitely be helpful in most cases. I decided to go with a DT property because in the case this patch tries to solve (the cubox-i) there isn't simply any other option - the default mode won't work due to the missing hw components. So, I thought that by defining it as a DT property it could somehow be locked to the hardware definition. Keep me posted Regards PS: Sorry for the second message, forgot to disable html and the message couldn't be delivered to all recipients. Alexandre Belloni <alexandre.belloni@bootlin.com> escreveu no dia quinta, 23/07/2020 à(s) 20:57: > > > > > I'm all for common properties, but is this common across vendors? > > > > This is but this shouldn't be a DT property as it has to be changed > dynamically. I'm working on an ioctl interface to change this > configuration. > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Thu, Jul 23, 2020 at 10:41 PM Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> wrote: > > Hi Alexandre, > > Having a way to dynamically change the configuration would definitely > be helpful in most cases. I decided to go with a DT property because > in the case this patch tries to solve (the cubox-i) there isn't simply > any other option - the default mode won't work due to the missing hw > components. So, I thought that by defining it as a DT property it > could somehow be locked to the hardware definition. > Keep me posted > > Regards > > PS: Sorry for the second message, forgot to disable html and the > message couldn't be delivered to all recipients. > > Alexandre Belloni <alexandre.belloni@bootlin.com> escreveu no dia > quinta, 23/07/2020 à(s) 20:57: > > > > > > > > > I'm all for common properties, but is this common across vendors? > > > > > > > This is but this shouldn't be a DT property as it has to be changed > > dynamically. I'm working on an ioctl interface to change this > > configuration. > > > > Thanks for sending this patch. I think there are two paths forward if an ioctl interface is being added. 1) Change the property to reflect that this is the default state the RTC should be initialized to. 2) Just move this configuration into the bootloader and then verify that the bootloader doesn't reset this value. -Jon
On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote: > On 23/07/2020 11:49:05-0600, Rob Herring wrote: > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote: > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > > > This adds direct-switching mode as a configurable DT flag for > > > RTC modules supporting it (e.g. nxp pcf8523). > > > DSM switches the power source to the battery supply whenever the > > > VDD drops below VBAT. The option is recommended for hw designs > > > where VDD is always expected to be higher than VBAT. > > > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > --- > > > Changes in v2: > > > - Added extended commit message for git history > > > - Separate dt bindings documentation into a single patch > > > > > > Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++- > > > Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++ > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > index 0b1080c..f715a8f 100644 > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > @@ -4,10 +4,14 @@ Required properties: > > > - compatible: Should contain "nxp,pcf8523". > > > - reg: I2C address for chip. > > > > > > -Optional property: > > > +Optional properties: > > > - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), > > > expressed in femto Farad (fF). Valid values are 7000 and 12500. > > > Default value (if no value is specified) is 12500fF. > > > +- pm-enable-dsm: battery switch-over function is enabled in direct > > > + switching mode. The power failure condition happens when VDD < VBAT, > > > + without requiring VDD to drop below Vth(sw)bat. > > > + Default value (if not provided) is the standard mode. > > > > > > Example: > > > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 { > > > compatible = "nxp,pcf8523"; > > > reg = <0x68>; > > > quartz-load-femtofarads = <7000>; > > > + pm-enable-dsm; > > > }; > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > index ee237b2..a0048f4 100644 > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > @@ -47,4 +47,11 @@ properties: > > > description: > > > Enables wake up of host system on alarm. > > > > > > + pm-enable-dsm: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: > > > + Enables the battery switch-over function in direct switching > > > + mode. Should be set in systems where VDD is higher than VBAT > > > + at all times. > > > > I'm all for common properties, but is this common across vendors? > > > > This is but this shouldn't be a DT property as it has to be changed > dynamically. I'm working on an ioctl interface to change this > configuration. Why does it need to be changed dynamically? If the hardware components are not fitted to allow the RTC to be safely used without DSM, then why should userspace be able to disable DSM?
On Mon, Jul 27, 2020 at 11:46 AM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote: > > On 23/07/2020 11:49:05-0600, Rob Herring wrote: > > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote: > > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > > > > > This adds direct-switching mode as a configurable DT flag for > > > > RTC modules supporting it (e.g. nxp pcf8523). > > > > DSM switches the power source to the battery supply whenever the > > > > VDD drops below VBAT. The option is recommended for hw designs > > > > where VDD is always expected to be higher than VBAT. > > > > > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > --- > > > > Changes in v2: > > > > - Added extended commit message for git history > > > > - Separate dt bindings documentation into a single patch > > > > > > > > Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++- > > > > Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++ > > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > index 0b1080c..f715a8f 100644 > > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > @@ -4,10 +4,14 @@ Required properties: > > > > - compatible: Should contain "nxp,pcf8523". > > > > - reg: I2C address for chip. > > > > > > > > -Optional property: > > > > +Optional properties: > > > > - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), > > > > expressed in femto Farad (fF). Valid values are 7000 and 12500. > > > > Default value (if no value is specified) is 12500fF. > > > > +- pm-enable-dsm: battery switch-over function is enabled in direct > > > > + switching mode. The power failure condition happens when VDD < VBAT, > > > > + without requiring VDD to drop below Vth(sw)bat. > > > > + Default value (if not provided) is the standard mode. > > > > > > > > Example: > > > > > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 { > > > > compatible = "nxp,pcf8523"; > > > > reg = <0x68>; > > > > quartz-load-femtofarads = <7000>; > > > > + pm-enable-dsm; > > > > }; > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > index ee237b2..a0048f4 100644 > > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > @@ -47,4 +47,11 @@ properties: > > > > description: > > > > Enables wake up of host system on alarm. > > > > > > > > + pm-enable-dsm: > > > > + $ref: /schemas/types.yaml#/definitions/flag > > > > + description: > > > > + Enables the battery switch-over function in direct switching > > > > + mode. Should be set in systems where VDD is higher than VBAT > > > > + at all times. > > > > > > I'm all for common properties, but is this common across vendors? > > > > > > > This is but this shouldn't be a DT property as it has to be changed > > dynamically. I'm working on an ioctl interface to change this > > configuration. > > Why does it need to be changed dynamically? If the hardware components > are not fitted to allow the RTC to be safely used without DSM, then > why should userspace be able to disable DSM? > My presumption would be if you had a system that ran at different system voltages depending if it is plugged in to mains or running on a battery.
On Mon, Jul 27, 2020 at 03:33:17PM +0200, Jon Nettleton wrote: > On Mon, Jul 27, 2020 at 11:46 AM Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote: > > > On 23/07/2020 11:49:05-0600, Rob Herring wrote: > > > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote: > > > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > > > > > > > This adds direct-switching mode as a configurable DT flag for > > > > > RTC modules supporting it (e.g. nxp pcf8523). > > > > > DSM switches the power source to the battery supply whenever the > > > > > VDD drops below VBAT. The option is recommended for hw designs > > > > > where VDD is always expected to be higher than VBAT. > > > > > > > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > > --- > > > > > Changes in v2: > > > > > - Added extended commit message for git history > > > > > - Separate dt bindings documentation into a single patch > > > > > > > > > > Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++- > > > > > Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++ > > > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > > index 0b1080c..f715a8f 100644 > > > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > > @@ -4,10 +4,14 @@ Required properties: > > > > > - compatible: Should contain "nxp,pcf8523". > > > > > - reg: I2C address for chip. > > > > > > > > > > -Optional property: > > > > > +Optional properties: > > > > > - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), > > > > > expressed in femto Farad (fF). Valid values are 7000 and 12500. > > > > > Default value (if no value is specified) is 12500fF. > > > > > +- pm-enable-dsm: battery switch-over function is enabled in direct > > > > > + switching mode. The power failure condition happens when VDD < VBAT, > > > > > + without requiring VDD to drop below Vth(sw)bat. > > > > > + Default value (if not provided) is the standard mode. > > > > > > > > > > Example: > > > > > > > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 { > > > > > compatible = "nxp,pcf8523"; > > > > > reg = <0x68>; > > > > > quartz-load-femtofarads = <7000>; > > > > > + pm-enable-dsm; > > > > > }; > > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > > index ee237b2..a0048f4 100644 > > > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > > @@ -47,4 +47,11 @@ properties: > > > > > description: > > > > > Enables wake up of host system on alarm. > > > > > > > > > > + pm-enable-dsm: > > > > > + $ref: /schemas/types.yaml#/definitions/flag > > > > > + description: > > > > > + Enables the battery switch-over function in direct switching > > > > > + mode. Should be set in systems where VDD is higher than VBAT > > > > > + at all times. > > > > > > > > I'm all for common properties, but is this common across vendors? > > > > > > > > > > This is but this shouldn't be a DT property as it has to be changed > > > dynamically. I'm working on an ioctl interface to change this > > > configuration. > > > > Why does it need to be changed dynamically? If the hardware components > > are not fitted to allow the RTC to be safely used without DSM, then > > why should userspace be able to disable DSM? > > > > My presumption would be if you had a system that ran at different > system voltages depending if it is plugged in to mains or running on a > battery. Yes, but we're not talking about that case with the Cubox-i. Should a platform like the Cubox-i allow the user to disable DSM? There needs to be a way to block the ability to dynamically change this mode if the hardware is not up to operating without DSM.
On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote: > > This is but this shouldn't be a DT property as it has to be changed > > dynamically. I'm working on an ioctl interface to change this > > configuration. > > Why does it need to be changed dynamically? If the hardware components > are not fitted to allow the RTC to be safely used without DSM, then > why should userspace be able to disable DSM? For RTCs with a standby mode, you want to be able to return to standby mode. That would happen for example after factory flashing in that common use case: - the board is manufactured - Vbackup is installed, the RTC switches to standby mode - the board is then booted to flash a system, Vprimary is now present, the RTC switches to DSM. At this point, if the board is simply shut down, the RTC will start draining Vbackup before leaving the factory. Instead, we want to be able to return to standby mode until the final user switches the product on for the first time.
On Mon, Jul 27, 2020 at 4:17 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Mon, Jul 27, 2020 at 03:33:17PM +0200, Jon Nettleton wrote: > > On Mon, Jul 27, 2020 at 11:46 AM Russell King - ARM Linux admin > > <linux@armlinux.org.uk> wrote: > > > > > > On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote: > > > > On 23/07/2020 11:49:05-0600, Rob Herring wrote: > > > > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote: > > > > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > > > > > > > > > This adds direct-switching mode as a configurable DT flag for > > > > > > RTC modules supporting it (e.g. nxp pcf8523). > > > > > > DSM switches the power source to the battery supply whenever the > > > > > > VDD drops below VBAT. The option is recommended for hw designs > > > > > > where VDD is always expected to be higher than VBAT. > > > > > > > > > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com> > > > > > > --- > > > > > > Changes in v2: > > > > > > - Added extended commit message for git history > > > > > > - Separate dt bindings documentation into a single patch > > > > > > > > > > > > Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++- > > > > > > Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++ > > > > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > > > index 0b1080c..f715a8f 100644 > > > > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt > > > > > > @@ -4,10 +4,14 @@ Required properties: > > > > > > - compatible: Should contain "nxp,pcf8523". > > > > > > - reg: I2C address for chip. > > > > > > > > > > > > -Optional property: > > > > > > +Optional properties: > > > > > > - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), > > > > > > expressed in femto Farad (fF). Valid values are 7000 and 12500. > > > > > > Default value (if no value is specified) is 12500fF. > > > > > > +- pm-enable-dsm: battery switch-over function is enabled in direct > > > > > > + switching mode. The power failure condition happens when VDD < VBAT, > > > > > > + without requiring VDD to drop below Vth(sw)bat. > > > > > > + Default value (if not provided) is the standard mode. > > > > > > > > > > > > Example: > > > > > > > > > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 { > > > > > > compatible = "nxp,pcf8523"; > > > > > > reg = <0x68>; > > > > > > quartz-load-femtofarads = <7000>; > > > > > > + pm-enable-dsm; > > > > > > }; > > > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > > > index ee237b2..a0048f4 100644 > > > > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml > > > > > > @@ -47,4 +47,11 @@ properties: > > > > > > description: > > > > > > Enables wake up of host system on alarm. > > > > > > > > > > > > + pm-enable-dsm: > > > > > > + $ref: /schemas/types.yaml#/definitions/flag > > > > > > + description: > > > > > > + Enables the battery switch-over function in direct switching > > > > > > + mode. Should be set in systems where VDD is higher than VBAT > > > > > > + at all times. > > > > > > > > > > I'm all for common properties, but is this common across vendors? > > > > > > > > > > > > > This is but this shouldn't be a DT property as it has to be changed > > > > dynamically. I'm working on an ioctl interface to change this > > > > configuration. > > > > > > Why does it need to be changed dynamically? If the hardware components > > > are not fitted to allow the RTC to be safely used without DSM, then > > > why should userspace be able to disable DSM? > > > > > > > My presumption would be if you had a system that ran at different > > system voltages depending if it is plugged in to mains or running on a > > battery. > > Yes, but we're not talking about that case with the Cubox-i. > > Should a platform like the Cubox-i allow the user to disable DSM? > > There needs to be a way to block the ability to dynamically change > this mode if the hardware is not up to operating without DSM. > Agreed. Do we need a modes supported device-tree property? That would actually describe the hardware as device-tree is supposed to do.
On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote: > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote: > > > This is but this shouldn't be a DT property as it has to be changed > > > dynamically. I'm working on an ioctl interface to change this > > > configuration. > > > > Why does it need to be changed dynamically? If the hardware components > > are not fitted to allow the RTC to be safely used without DSM, then > > why should userspace be able to disable DSM? > > For RTCs with a standby mode, you want to be able to return to standby > mode. > > That would happen for example after factory flashing in that common use > case: > - the board is manufactured > - Vbackup is installed, the RTC switches to standby mode > - the board is then booted to flash a system, Vprimary is now present, > the RTC switches to DSM. > > At this point, if the board is simply shut down, the RTC will start > draining Vbackup before leaving the factory. Instead, we want to be able > to return to standby mode until the final user switches the product on > for the first time. I don't think you're understanding what's going on with this proposed patch. The cubox-i does work today, and the RTC does survive most power-downs. There are situations where it doesn't. So, let's take your process above. - the board is manufactured - Vbackup is installed, the RTC switches to standby mode - the board is then booted to flash a system, Vprimary is now present - the board is powered down. the RTC _might_ switch over to battery if it notices the power failure in time, or it might not. A random sample of units leaving the factory have the RTC in standby mode. Others are draining the battery. I'm not saying what you propose isn't a good idea. I'm questioning why we should expose this in the generic kernel on platforms where it's likely to end up with the RTC being corrupted. Now, I question your idea that units should leave the factory without the RTC being programmed. We know that lovely systemd goes utterly bonkers if the system time is beyond INT_MAX. If the RTC leaves standby mode containing a date which we translate beyond INT_MAX, systemd will refuse to boot the system, and the user will have no way to set the correct time. The user returns the device to the supplier as faulty...
On 27/07/2020 16:24:39+0100, Russell King - ARM Linux admin wrote: > On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote: > > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote: > > > > This is but this shouldn't be a DT property as it has to be changed > > > > dynamically. I'm working on an ioctl interface to change this > > > > configuration. > > > > > > Why does it need to be changed dynamically? If the hardware components > > > are not fitted to allow the RTC to be safely used without DSM, then > > > why should userspace be able to disable DSM? > > > > For RTCs with a standby mode, you want to be able to return to standby > > mode. > > > > That would happen for example after factory flashing in that common use > > case: > > - the board is manufactured > > - Vbackup is installed, the RTC switches to standby mode > > - the board is then booted to flash a system, Vprimary is now present, > > the RTC switches to DSM. > > > > At this point, if the board is simply shut down, the RTC will start > > draining Vbackup before leaving the factory. Instead, we want to be able > > to return to standby mode until the final user switches the product on > > for the first time. > > I don't think you're understanding what's going on with this proposed > patch. The cubox-i does work today, and the RTC does survive most > power-downs. There are situations where it doesn't. > > So, let's take your process above. > > - the board is manufactured > - Vbackup is installed, the RTC switches to standby mode > - the board is then booted to flash a system, Vprimary is now present > - the board is powered down. the RTC _might_ switch over to battery > if it notices the power failure in time, or it might not. A random > sample of units leaving the factory have the RTC in standby mode. > Others are draining the battery. > > I'm not saying what you propose isn't a good idea. I'm questioning > why we should expose this in the generic kernel on platforms where > it's likely to end up with the RTC being corrupted. > Note that I didn't say we should expose settings that are not working but it is a different discussion. I was explaining why we need to be able to change it dynamically. > Now, I question your idea that units should leave the factory without > the RTC being programmed. We know that lovely systemd goes utterly > bonkers if the system time is beyond INT_MAX. If the RTC leaves > standby mode containing a date which we translate beyond INT_MAX, > systemd will refuse to boot the system, and the user will have no > way to set the correct time. The user returns the device to the > supplier as faulty... This is doesn't happen since v4.17.
On Mon, Jul 27, 2020 at 05:41:04PM +0200, Alexandre Belloni wrote: > On 27/07/2020 16:24:39+0100, Russell King - ARM Linux admin wrote: > > On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote: > > > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote: > > > > > This is but this shouldn't be a DT property as it has to be changed > > > > > dynamically. I'm working on an ioctl interface to change this > > > > > configuration. > > > > > > > > Why does it need to be changed dynamically? If the hardware components > > > > are not fitted to allow the RTC to be safely used without DSM, then > > > > why should userspace be able to disable DSM? > > > > > > For RTCs with a standby mode, you want to be able to return to standby > > > mode. > > > > > > That would happen for example after factory flashing in that common use > > > case: > > > - the board is manufactured > > > - Vbackup is installed, the RTC switches to standby mode > > > - the board is then booted to flash a system, Vprimary is now present, > > > the RTC switches to DSM. > > > > > > At this point, if the board is simply shut down, the RTC will start > > > draining Vbackup before leaving the factory. Instead, we want to be able > > > to return to standby mode until the final user switches the product on > > > for the first time. > > > > I don't think you're understanding what's going on with this proposed > > patch. The cubox-i does work today, and the RTC does survive most > > power-downs. There are situations where it doesn't. > > > > So, let's take your process above. > > > > - the board is manufactured > > - Vbackup is installed, the RTC switches to standby mode > > - the board is then booted to flash a system, Vprimary is now present > > - the board is powered down. the RTC _might_ switch over to battery > > if it notices the power failure in time, or it might not. A random > > sample of units leaving the factory have the RTC in standby mode. > > Others are draining the battery. > > > > I'm not saying what you propose isn't a good idea. I'm questioning > > why we should expose this in the generic kernel on platforms where > > it's likely to end up with the RTC being corrupted. > > > > Note that I didn't say we should expose settings that are not working > but it is a different discussion. It isn't a different discussion - that is exactly what the point of my emails to you all along have been! So, can we please have that discussion, it is pertinent to this patch.
On Mon, Jul 27, 2020 at 5:43 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Mon, Jul 27, 2020 at 05:41:04PM +0200, Alexandre Belloni wrote: > > On 27/07/2020 16:24:39+0100, Russell King - ARM Linux admin wrote: > > > On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote: > > > > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote: > > > > > > This is but this shouldn't be a DT property as it has to be changed > > > > > > dynamically. I'm working on an ioctl interface to change this > > > > > > configuration. > > > > > > > > > > Why does it need to be changed dynamically? If the hardware components > > > > > are not fitted to allow the RTC to be safely used without DSM, then > > > > > why should userspace be able to disable DSM? > > > > > > > > For RTCs with a standby mode, you want to be able to return to standby > > > > mode. > > > > > > > > That would happen for example after factory flashing in that common use > > > > case: > > > > - the board is manufactured > > > > - Vbackup is installed, the RTC switches to standby mode > > > > - the board is then booted to flash a system, Vprimary is now present, > > > > the RTC switches to DSM. > > > > > > > > At this point, if the board is simply shut down, the RTC will start > > > > draining Vbackup before leaving the factory. Instead, we want to be able > > > > to return to standby mode until the final user switches the product on > > > > for the first time. > > > > > > I don't think you're understanding what's going on with this proposed > > > patch. The cubox-i does work today, and the RTC does survive most > > > power-downs. There are situations where it doesn't. > > > > > > So, let's take your process above. > > > > > > - the board is manufactured > > > - Vbackup is installed, the RTC switches to standby mode > > > - the board is then booted to flash a system, Vprimary is now present > > > - the board is powered down. the RTC _might_ switch over to battery > > > if it notices the power failure in time, or it might not. A random > > > sample of units leaving the factory have the RTC in standby mode. > > > Others are draining the battery. > > > > > > I'm not saying what you propose isn't a good idea. I'm questioning > > > why we should expose this in the generic kernel on platforms where > > > it's likely to end up with the RTC being corrupted. > > > > > > > Note that I didn't say we should expose settings that are not working > > but it is a different discussion. > > It isn't a different discussion - that is exactly what the point of > my emails to you all along have been! > > So, can we please have that discussion, it is pertinent to this patch. > Thinking about this some more, I believe whether or not an IOCTL interface is in the works or needed is irrelevant. This patch describes the hardware and how it is designed and the topic of discussion is if we need a simple boolean state, or if we need something that could be used to support dynamic configuration in the future. I would rather make this decision now rather than keep tacking on boolean config options, or revisit a bunch of device-tree changes.
On 27/07/2020 17:55:50+0200, Jon Nettleton wrote: > > So, can we please have that discussion, it is pertinent to this patch. > > > > Thinking about this some more, I believe whether or not an IOCTL > interface is in the works or needed is irrelevant. This patch > describes the hardware and how it is designed and the topic of > discussion is if we need a simple boolean state, or if we need > something that could be used to support dynamic configuration in the > future. I would rather make this decision now rather than keep > tacking on boolean config options, or revisit a bunch of device-tree > changes. Something that would describe the hardware is a property telling whether the filter is present on VDD, not a property selecting DSM. The driver can then chose to avoid standard mode when needed.
On Mon, Jul 27, 2020 at 6:16 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 27/07/2020 17:55:50+0200, Jon Nettleton wrote: > > > So, can we please have that discussion, it is pertinent to this patch. > > > > > > > Thinking about this some more, I believe whether or not an IOCTL > > interface is in the works or needed is irrelevant. This patch > > describes the hardware and how it is designed and the topic of > > discussion is if we need a simple boolean state, or if we need > > something that could be used to support dynamic configuration in the > > future. I would rather make this decision now rather than keep > > tacking on boolean config options, or revisit a bunch of device-tree > > changes. > > Something that would describe the hardware is a property telling whether > the filter is present on VDD, not a property selecting DSM. The driver > can then chose to avoid standard mode when needed. > The filter being present or not is not the singular hardware requirement for making this determination. There is no reason to make this some obtuse argument. There are two states and you may want one or the other, or the ability to switch between them. This is a flag to tell the driver how the hardware should be configured based on the board preference. Let's find a simple consistent way to do this.
On Mon, Jul 27, 2020 at 06:16:32PM +0200, Alexandre Belloni wrote: > On 27/07/2020 17:55:50+0200, Jon Nettleton wrote: > > > So, can we please have that discussion, it is pertinent to this patch. > > > > > > > Thinking about this some more, I believe whether or not an IOCTL > > interface is in the works or needed is irrelevant. This patch > > describes the hardware and how it is designed and the topic of > > discussion is if we need a simple boolean state, or if we need > > something that could be used to support dynamic configuration in the > > future. I would rather make this decision now rather than keep > > tacking on boolean config options, or revisit a bunch of device-tree > > changes. > > Something that would describe the hardware is a property telling whether > the filter is present on VDD, not a property selecting DSM. The driver > can then chose to avoid standard mode when needed. Whether DSM needs to be enabled or not is _not_ just a function of whether there is a filter present or not. The requirement in the data sheet is that when the VDD supply drops below 2.5V, it does not fall more than 0.7V/ms. That can be achieved many different ways, not only by adding a resistive filter to the VDD supply to the RTC. It could also be achieved via the design of the power supply - for example, having a large enough reservoir capacitor to ensure under all loads that the VDD supply will not fall faster than 0.7V/ms. There are many ways to meet this requirement. Adding a DT property to indicate whether the filter is present or not is definitely not the right approach. Should we also add properties for every possible solution to this problem. vdd-has-filter; psu-has-large-capacitors; ... etc ...
Russell King - ARM Linux admin <linux@armlinux.org.uk> escreveu no dia segunda, 27/07/2020 à(s) 18:30: > > On Mon, Jul 27, 2020 at 06:16:32PM +0200, Alexandre Belloni wrote: > > On 27/07/2020 17:55:50+0200, Jon Nettleton wrote: > > > > So, can we please have that discussion, it is pertinent to this patch. > > > > > > > > > > Thinking about this some more, I believe whether or not an IOCTL > > > interface is in the works or needed is irrelevant. This patch > > > describes the hardware and how it is designed and the topic of > > > discussion is if we need a simple boolean state, or if we need > > > something that could be used to support dynamic configuration in the > > > future. I would rather make this decision now rather than keep > > > tacking on boolean config options, or revisit a bunch of device-tree > > > changes. For what it's worth I also tend to agree. The patchset, regardless of the property name (that I admit might be misleading), is intended at enforcing a mode that the RTC/driver should use by default. This mode is strongly related to the hardware definition/implementation and its capabilities. While I understand the need for the IOCTL interface to solve issues exactly like the aforementioned factory example, I fail to see how it can be of any help to solve the problem at hand - as it won't likely configure the driver to use a different default mode depending on the board. The IOCTL interface might also allow the userspace to change this property back to the default mode (000) and end up breaking the RTC operation, but I guess that's the cost of configurability and, in the end, the user's responsibility. Any pointers on how to proceed are appreciated. Regards, Miguel
On 27/07/2020 22:13:42+0100, Miguel Borges de Freitas wrote: > Russell King - ARM Linux admin <linux@armlinux.org.uk> escreveu no dia > segunda, 27/07/2020 à(s) 18:30: > > > > On Mon, Jul 27, 2020 at 06:16:32PM +0200, Alexandre Belloni wrote: > > > On 27/07/2020 17:55:50+0200, Jon Nettleton wrote: > > > > > So, can we please have that discussion, it is pertinent to this patch. > > > > > > > > > > > > > Thinking about this some more, I believe whether or not an IOCTL > > > > interface is in the works or needed is irrelevant. This patch > > > > describes the hardware and how it is designed and the topic of > > > > discussion is if we need a simple boolean state, or if we need > > > > something that could be used to support dynamic configuration in the > > > > future. I would rather make this decision now rather than keep > > > > tacking on boolean config options, or revisit a bunch of device-tree > > > > changes. > > For what it's worth I also tend to agree. > The patchset, regardless of the property name (that I admit might be > misleading), is intended at enforcing a mode that the RTC/driver > should use by default. This mode is strongly related to the hardware > definition/implementation and its capabilities. While I understand the > need for the IOCTL interface to solve issues exactly like the > aforementioned factory example, I fail to see how it can be of any > help to solve the problem at hand - as it won't likely configure the > driver to use a different default mode depending on the board. The > IOCTL interface might also allow the userspace to change this property > back to the default mode (000) and end up breaking the RTC operation, > but I guess that's the cost of configurability and, in the end, the > user's responsibility. > Any pointers on how to proceed are appreciated. > I would think the simpler way to proceed is to have a device specific property indicating that standard mode is not available. From the driver, you can then switch from standard to DSM when this property is present. I think it is difficult to come up with a generic property for that as most other RTCs with level/threshold switching have a fast edge detection feature that is usually enabled by default. So what they would require instead is a property indicating that the voltage is ramping down at a certain rate allowing to disable fast edge detection and saving a bit of power.
diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt index 0b1080c..f715a8f 100644 --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt @@ -4,10 +4,14 @@ Required properties: - compatible: Should contain "nxp,pcf8523". - reg: I2C address for chip. -Optional property: +Optional properties: - quartz-load-femtofarads: The capacitive load of the quartz(x-tal), expressed in femto Farad (fF). Valid values are 7000 and 12500. Default value (if no value is specified) is 12500fF. +- pm-enable-dsm: battery switch-over function is enabled in direct + switching mode. The power failure condition happens when VDD < VBAT, + without requiring VDD to drop below Vth(sw)bat. + Default value (if not provided) is the standard mode. Example: @@ -15,4 +19,5 @@ pcf8523: rtc@68 { compatible = "nxp,pcf8523"; reg = <0x68>; quartz-load-femtofarads = <7000>; + pm-enable-dsm; }; diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml index ee237b2..a0048f4 100644 --- a/Documentation/devicetree/bindings/rtc/rtc.yaml +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml @@ -47,4 +47,11 @@ properties: description: Enables wake up of host system on alarm. + pm-enable-dsm: + $ref: /schemas/types.yaml#/definitions/flag + description: + Enables the battery switch-over function in direct switching + mode. Should be set in systems where VDD is higher than VBAT + at all times. + ...