[PATCH] disable clkout for rv3028 by default

[PATCH] disable clkout for rv3028 by default

[Johannes Kirchmair]

The rv3028 chip is kind of strange.
The chip has two inputs one for the buffer battery
(V_backup) and one for the main power supply (V_dd).
By default a clk out of the chip is enabled, drawing a big amount of
current, draining the buffer battery of our board in 3 days.
There is a mode that would shut down the clk out if powered from
V_backup, but that would have to be configured as well. In our
application the battery is connected via V_dd. So disabling the clk by
default is the way to go for us.

Signed-off-by: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
---
 drivers/rtc/rtc-rv3028.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
index 12c807306893..9e2aaa7a533e 100644
--- a/drivers/rtc/rtc-rv3028.c
+++ b/drivers/rtc/rtc-rv3028.c
@@ -787,7 +787,7 @@ static const struct regmap_config regmap_config = {
 static int rv3028_probe(struct i2c_client *client)
 {
 	struct rv3028_data *rv3028;
-	int ret, status;
+	int ret, status, buf;
 	u32 ohms;
 	struct nvmem_config nvmem_cfg = {
 		.name = "rv3028_nvram",
@@ -826,6 +826,16 @@ static int rv3028_probe(struct i2c_client *client)
 	if (status & RV3028_STATUS_AF)
 		dev_warn(&client->dev, "An alarm may have been missed.\n");
 
+	ret = regmap_read(rv3028->regmap, RV3028_CLKOUT, &buf);
+	if (ret < 0)
+		return ret;
+
+	if (buf != RV3028_CLKOUT_FD_MASK) {
+		ret = rv3028_update_cfg(rv3028, RV3028_CLKOUT, 0xff, RV3028_CLKOUT_FD_MASK); // disable clk out
+		if (ret < 0)
+			return ret;
+	}
+
 	rv3028->rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(rv3028->rtc))
 		return PTR_ERR(rv3028->rtc);
-- 
2.25.1

Re: [PATCH] disable clkout for rv3028 by default

[Alexandre Belloni]

On 04/05/2023 10:32:17+0200, Johannes Kirchmair wrote:
> The rv3028 chip is kind of strange.
> The chip has two inputs one for the buffer battery
> (V_backup) and one for the main power supply (V_dd).
> By default a clk out of the chip is enabled, drawing a big amount of
> current, draining the buffer battery of our board in 3 days.
> There is a mode that would shut down the clk out if powered from
> V_backup, but that would have to be configured as well. In our
> application the battery is connected via V_dd. So disabling the clk by
> default is the way to go for us.
> 

You can't do that, this introduces a glitch in the clock output and will
break existing users. The clock should be disabled automatically by the
CCF when there are no users. Is your kernel built without
CONFIG_COMMON_CLK?

> Signed-off-by: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
> ---
>  drivers/rtc/rtc-rv3028.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> index 12c807306893..9e2aaa7a533e 100644
> --- a/drivers/rtc/rtc-rv3028.c
> +++ b/drivers/rtc/rtc-rv3028.c
> @@ -787,7 +787,7 @@ static const struct regmap_config regmap_config = {
>  static int rv3028_probe(struct i2c_client *client)
>  {
>  	struct rv3028_data *rv3028;
> -	int ret, status;
> +	int ret, status, buf;
>  	u32 ohms;
>  	struct nvmem_config nvmem_cfg = {
>  		.name = "rv3028_nvram",
> @@ -826,6 +826,16 @@ static int rv3028_probe(struct i2c_client *client)
>  	if (status & RV3028_STATUS_AF)
>  		dev_warn(&client->dev, "An alarm may have been missed.\n");
>  
> +	ret = regmap_read(rv3028->regmap, RV3028_CLKOUT, &buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (buf != RV3028_CLKOUT_FD_MASK) {
> +		ret = rv3028_update_cfg(rv3028, RV3028_CLKOUT, 0xff, RV3028_CLKOUT_FD_MASK); // disable clk out
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	rv3028->rtc = devm_rtc_allocate_device(&client->dev);
>  	if (IS_ERR(rv3028->rtc))
>  		return PTR_ERR(rv3028->rtc);
> -- 
> 2.25.1
> 

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

RE: [PATCH] disable clkout for rv3028 by default

[Johannes Kirchmair]

Hello Alex,

> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Donnerstag, 4. Mai 2023 11:13
> To: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
> Cc: linux-rtc@vger.kernel.org
> Subject: Re: [PATCH] disable clkout for rv3028 by default
> 
> CAUTION: External E-Mail !
> 
> On 04/05/2023 10:32:17+0200, Johannes Kirchmair wrote:
> > The rv3028 chip is kind of strange.
> > The chip has two inputs one for the buffer battery
> > (V_backup) and one for the main power supply (V_dd).
> > By default a clk out of the chip is enabled, drawing a big amount of
> > current, draining the buffer battery of our board in 3 days.
> > There is a mode that would shut down the clk out if powered from
> > V_backup, but that would have to be configured as well. In our
> > application the battery is connected via V_dd. So disabling the clk by
> > default is the way to go for us.
> >
> 
> You can't do that, this introduces a glitch in the clock output and will
> break existing users. The clock should be disabled automatically by the
> CCF when there are no users. Is your kernel built without
> CONFIG_COMMON_CLK
Now I am a little confused.
I tested on an x86 board where the clock was not disabled by the kernel. 
Than I tested on an arm imx8 board where it was disabled by default.

In both cases I used an 5.15 kernel.

Difference between x86 and arm is, that in x86 we probe from user space after boot process and on the arm board the rtc is probed while booting. 

Do you know at what point the clock is disabled? Is it part of registering the clock?

Best regards Johannes

> 
> > Signed-off-by: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
> > ---
> >  drivers/rtc/rtc-rv3028.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> > index 12c807306893..9e2aaa7a533e 100644
> > --- a/drivers/rtc/rtc-rv3028.c
> > +++ b/drivers/rtc/rtc-rv3028.c
> > @@ -787,7 +787,7 @@ static const struct regmap_config regmap_config = {
> >  static int rv3028_probe(struct i2c_client *client)
> >  {
> >       struct rv3028_data *rv3028;
> > -     int ret, status;
> > +     int ret, status, buf;
> >       u32 ohms;
> >       struct nvmem_config nvmem_cfg = {
> >               .name = "rv3028_nvram",
> > @@ -826,6 +826,16 @@ static int rv3028_probe(struct i2c_client *client)
> >       if (status & RV3028_STATUS_AF)
> >               dev_warn(&client->dev, "An alarm may have been missed.\n");
> >
> > +     ret = regmap_read(rv3028->regmap, RV3028_CLKOUT, &buf);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (buf != RV3028_CLKOUT_FD_MASK) {
> > +             ret = rv3028_update_cfg(rv3028, RV3028_CLKOUT, 0xff,
> RV3028_CLKOUT_FD_MASK); // disable clk out
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> >       rv3028->rtc = devm_rtc_allocate_device(&client->dev);
> >       if (IS_ERR(rv3028->rtc))
> >               return PTR_ERR(rv3028->rtc);
> > --
> > 2.25.1
> >
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH] disable clkout for rv3028 by default

[Alexandre Belloni]

On 05/05/2023 10:31:38+0000, Johannes Kirchmair wrote:
> Hello Alex,
> 
> > -----Original Message-----
> > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Sent: Donnerstag, 4. Mai 2023 11:13
> > To: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
> > Cc: linux-rtc@vger.kernel.org
> > Subject: Re: [PATCH] disable clkout for rv3028 by default
> > 
> > CAUTION: External E-Mail !
> > 
> > On 04/05/2023 10:32:17+0200, Johannes Kirchmair wrote:
> > > The rv3028 chip is kind of strange.
> > > The chip has two inputs one for the buffer battery
> > > (V_backup) and one for the main power supply (V_dd).
> > > By default a clk out of the chip is enabled, drawing a big amount of
> > > current, draining the buffer battery of our board in 3 days.
> > > There is a mode that would shut down the clk out if powered from
> > > V_backup, but that would have to be configured as well. In our
> > > application the battery is connected via V_dd. So disabling the clk by
> > > default is the way to go for us.
> > >
> > 
> > You can't do that, this introduces a glitch in the clock output and will
> > break existing users. The clock should be disabled automatically by the
> > CCF when there are no users. Is your kernel built without
> > CONFIG_COMMON_CLK
> Now I am a little confused.
> I tested on an x86 board where the clock was not disabled by the kernel. 
> Than I tested on an arm imx8 board where it was disabled by default.
> 
> In both cases I used an 5.15 kernel.
> 
> Difference between x86 and arm is, that in x86 we probe from user space after boot process and on the arm board the rtc is probed while booting. 
> 
> Do you know at what point the clock is disabled? Is it part of registering the clock?
> 

This is clk_disable_unused which is a late_initcall_sync()

> Best regards Johannes
> 
> > 
> > > Signed-off-by: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
> > > ---
> > >  drivers/rtc/rtc-rv3028.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> > > index 12c807306893..9e2aaa7a533e 100644
> > > --- a/drivers/rtc/rtc-rv3028.c
> > > +++ b/drivers/rtc/rtc-rv3028.c
> > > @@ -787,7 +787,7 @@ static const struct regmap_config regmap_config = {
> > >  static int rv3028_probe(struct i2c_client *client)
> > >  {
> > >       struct rv3028_data *rv3028;
> > > -     int ret, status;
> > > +     int ret, status, buf;
> > >       u32 ohms;
> > >       struct nvmem_config nvmem_cfg = {
> > >               .name = "rv3028_nvram",
> > > @@ -826,6 +826,16 @@ static int rv3028_probe(struct i2c_client *client)
> > >       if (status & RV3028_STATUS_AF)
> > >               dev_warn(&client->dev, "An alarm may have been missed.\n");
> > >
> > > +     ret = regmap_read(rv3028->regmap, RV3028_CLKOUT, &buf);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     if (buf != RV3028_CLKOUT_FD_MASK) {
> > > +             ret = rv3028_update_cfg(rv3028, RV3028_CLKOUT, 0xff,
> > RV3028_CLKOUT_FD_MASK); // disable clk out
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +     }
> > > +
> > >       rv3028->rtc = devm_rtc_allocate_device(&client->dev);
> > >       if (IS_ERR(rv3028->rtc))
> > >               return PTR_ERR(rv3028->rtc);
> > > --
> > > 2.25.1
> > >
> > 
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com

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