mbox series

[0/5] rtc: ingenic: various updates

Message ID 20220418184933.13172-1-paul@crapouillou.net
Headers show
Series rtc: ingenic: various updates | expand

Message

Paul Cercueil April 18, 2022, 6:49 p.m. UTC 
Hi,

Here's a set of patches for the Ingenic RTC driver (jz4740-rtc).

Patch [1/5] and [4/5] update the DT binding documentation and update the
driver to support the CLK32K pin. This pin optionally supplies a 32 kHz
clock, which is required on the MIPS CI20 board for the WiFi/Bluetooth
chip to work.

Patch [2/5] is a code cleanup. Patch [3/5] fixes the RTC time yielding
an impossible value after a power loss.

Finally, patch [5/5] is *RFC*. I do not know if it works, as I have
absolutely no idea about how to test it.

Cheers,
-Paul

Paul Cercueil (5):
  dt-bindings: rtc: Rework compatible strings and add #clock-cells
  rtc: jz4740: Use readl_poll_timeout
  rtc: jz4740: Reset scratchpad register on power loss
  rtc: jz4740: Register clock provider for the CLK32K pin
  rtc: jz4740: Support for fine-tuning the RTC clock

 .../devicetree/bindings/rtc/ingenic,rtc.yaml  |   7 +-
 drivers/rtc/rtc-jz4740.c                      | 137 +++++++++++++++---
 2 files changed, 125 insertions(+), 19 deletions(-)

Comments

Alexandre Belloni April 19, 2022, 7:35 p.m. UTC | #1 
On 18/04/2022 19:49:31+0100, Paul Cercueil wrote:
> On power loss, reading the RTC value would fail as the scratchpad lost
> its magic value, until the hardware clock was set once again.
> 
> To avoid that, reset the RTC value to Epoch in the probe if we detect
> that the scratchpad lost its magic value.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/rtc/rtc-jz4740.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
> index 119baf168b32..aac5f68bf626 100644
> --- a/drivers/rtc/rtc-jz4740.c
> +++ b/drivers/rtc/rtc-jz4740.c
> @@ -42,6 +42,9 @@
>  /* Magic value to enable writes on jz4780 */
>  #define JZ_RTC_WENR_MAGIC	0xA55A
>  
> +/* Value written to the scratchpad to detect power losses */
> +#define JZ_RTC_SCRATCHPAD_MAGIC	0x12345678
> +
>  #define JZ_RTC_WAKEUP_FILTER_MASK	0x0000FFE0
>  #define JZ_RTC_RESET_COUNTER_MASK	0x00000FE0
>  
> @@ -134,10 +137,11 @@ static int jz4740_rtc_ctrl_set_bits(struct jz4740_rtc *rtc, uint32_t mask,
>  static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
>  {
>  	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
> -	uint32_t secs, secs2;
> +	uint32_t secs, secs2, magic;
>  	int timeout = 5;
>  
> -	if (jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD) != 0x12345678)
> +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
> +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC)
>  		return -EINVAL;
>  
>  	/* If the seconds register is read while it is updated, it can contain a
> @@ -169,7 +173,8 @@ static int jz4740_rtc_set_time(struct device *dev, struct rtc_time *time)
>  	if (ret)
>  		return ret;
>  
> -	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD, 0x12345678);
> +	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
> +				    JZ_RTC_SCRATCHPAD_MAGIC);
>  }
>  
>  static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> @@ -307,6 +312,7 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
>  	struct jz4740_rtc *rtc;
>  	unsigned long rate;
>  	struct clk *clk;
> +	uint32_t magic;
>  	int ret, irq;
>  
>  	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> @@ -369,6 +375,18 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
>  	/* Each 1 Hz pulse should happen after (rate) ticks */
>  	jz4740_rtc_reg_write(rtc, JZ_REG_RTC_REGULATOR, rate - 1);
>  
> +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
> +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC) {
> +		/*
> +		 * If the scratchpad doesn't hold our magic value, then a
> +		 * power loss occurred. Reset to Epoch.
> +		 */
> +		struct rtc_time time;
> +
> +		rtc_time64_to_tm(0, &time);
> +		jz4740_rtc_set_time(dev, &time);

Don't do that, this defeats the purpose of detecting when the power is
lost. Returning a known bogus time is the worst thing you can do here.

> +	}
> +
>  	ret = devm_rtc_register_device(rtc->rtc);
>  	if (ret)
>  		return ret;
> -- 
> 2.35.1
>
Paul Cercueil April 19, 2022, 7:48 p.m. UTC | #2 
Hi Alexandre,

Le mar., avril 19 2022 at 21:35:35 +0200, Alexandre Belloni 
<alexandre.belloni@bootlin.com> a écrit :
> On 18/04/2022 19:49:31+0100, Paul Cercueil wrote:
>>  On power loss, reading the RTC value would fail as the scratchpad 
>> lost
>>  its magic value, until the hardware clock was set once again.
>> 
>>  To avoid that, reset the RTC value to Epoch in the probe if we 
>> detect
>>  that the scratchpad lost its magic value.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/rtc/rtc-jz4740.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>> 
>>  diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
>>  index 119baf168b32..aac5f68bf626 100644
>>  --- a/drivers/rtc/rtc-jz4740.c
>>  +++ b/drivers/rtc/rtc-jz4740.c
>>  @@ -42,6 +42,9 @@
>>   /* Magic value to enable writes on jz4780 */
>>   #define JZ_RTC_WENR_MAGIC	0xA55A
>> 
>>  +/* Value written to the scratchpad to detect power losses */
>>  +#define JZ_RTC_SCRATCHPAD_MAGIC	0x12345678
>>  +
>>   #define JZ_RTC_WAKEUP_FILTER_MASK	0x0000FFE0
>>   #define JZ_RTC_RESET_COUNTER_MASK	0x00000FE0
>> 
>>  @@ -134,10 +137,11 @@ static int jz4740_rtc_ctrl_set_bits(struct 
>> jz4740_rtc *rtc, uint32_t mask,
>>   static int jz4740_rtc_read_time(struct device *dev, struct 
>> rtc_time *time)
>>   {
>>   	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>  -	uint32_t secs, secs2;
>>  +	uint32_t secs, secs2, magic;
>>   	int timeout = 5;
>> 
>>  -	if (jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD) != 0x12345678)
>>  +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
>>  +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC)
>>   		return -EINVAL;
>> 
>>   	/* If the seconds register is read while it is updated, it can 
>> contain a
>>  @@ -169,7 +173,8 @@ static int jz4740_rtc_set_time(struct device 
>> *dev, struct rtc_time *time)
>>   	if (ret)
>>   		return ret;
>> 
>>  -	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD, 
>> 0x12345678);
>>  +	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
>>  +				    JZ_RTC_SCRATCHPAD_MAGIC);
>>   }
>> 
>>   static int jz4740_rtc_read_alarm(struct device *dev, struct 
>> rtc_wkalrm *alrm)
>>  @@ -307,6 +312,7 @@ static int jz4740_rtc_probe(struct 
>> platform_device *pdev)
>>   	struct jz4740_rtc *rtc;
>>   	unsigned long rate;
>>   	struct clk *clk;
>>  +	uint32_t magic;
>>   	int ret, irq;
>> 
>>   	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
>>  @@ -369,6 +375,18 @@ static int jz4740_rtc_probe(struct 
>> platform_device *pdev)
>>   	/* Each 1 Hz pulse should happen after (rate) ticks */
>>   	jz4740_rtc_reg_write(rtc, JZ_REG_RTC_REGULATOR, rate - 1);
>> 
>>  +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
>>  +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC) {
>>  +		/*
>>  +		 * If the scratchpad doesn't hold our magic value, then a
>>  +		 * power loss occurred. Reset to Epoch.
>>  +		 */
>>  +		struct rtc_time time;
>>  +
>>  +		rtc_time64_to_tm(0, &time);
>>  +		jz4740_rtc_set_time(dev, &time);
> 
> Don't do that, this defeats the purpose of detecting when the power is
> lost. Returning a known bogus time is the worst thing you can do here.

So what is the best thing to do then?

Cheers,
-Paul

>>  +	}
>>  +
>>   	ret = devm_rtc_register_device(rtc->rtc);
>>   	if (ret)
>>   		return ret;
>>  --
>>  2.35.1
>> 
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Alexandre Belloni April 19, 2022, 8 p.m. UTC | #3 
On 19/04/2022 20:48:54+0100, Paul Cercueil wrote:
> Hi Alexandre,
> 
> Le mar., avril 19 2022 at 21:35:35 +0200, Alexandre Belloni
> <alexandre.belloni@bootlin.com> a écrit :
> > On 18/04/2022 19:49:31+0100, Paul Cercueil wrote:
> > >  On power loss, reading the RTC value would fail as the scratchpad
> > > lost
> > >  its magic value, until the hardware clock was set once again.
> > > 
> > >  To avoid that, reset the RTC value to Epoch in the probe if we
> > > detect
> > >  that the scratchpad lost its magic value.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   drivers/rtc/rtc-jz4740.c | 24 +++++++++++++++++++++---
> > >   1 file changed, 21 insertions(+), 3 deletions(-)
> > > 
> > >  diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
> > >  index 119baf168b32..aac5f68bf626 100644
> > >  --- a/drivers/rtc/rtc-jz4740.c
> > >  +++ b/drivers/rtc/rtc-jz4740.c
> > >  @@ -42,6 +42,9 @@
> > >   /* Magic value to enable writes on jz4780 */
> > >   #define JZ_RTC_WENR_MAGIC	0xA55A
> > > 
> > >  +/* Value written to the scratchpad to detect power losses */
> > >  +#define JZ_RTC_SCRATCHPAD_MAGIC	0x12345678
> > >  +
> > >   #define JZ_RTC_WAKEUP_FILTER_MASK	0x0000FFE0
> > >   #define JZ_RTC_RESET_COUNTER_MASK	0x00000FE0
> > > 
> > >  @@ -134,10 +137,11 @@ static int jz4740_rtc_ctrl_set_bits(struct
> > > jz4740_rtc *rtc, uint32_t mask,
> > >   static int jz4740_rtc_read_time(struct device *dev, struct
> > > rtc_time *time)
> > >   {
> > >   	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
> > >  -	uint32_t secs, secs2;
> > >  +	uint32_t secs, secs2, magic;
> > >   	int timeout = 5;
> > > 
> > >  -	if (jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD) != 0x12345678)
> > >  +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
> > >  +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC)
> > >   		return -EINVAL;
> > > 
> > >   	/* If the seconds register is read while it is updated, it can
> > > contain a
> > >  @@ -169,7 +173,8 @@ static int jz4740_rtc_set_time(struct device
> > > *dev, struct rtc_time *time)
> > >   	if (ret)
> > >   		return ret;
> > > 
> > >  -	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
> > > 0x12345678);
> > >  +	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
> > >  +				    JZ_RTC_SCRATCHPAD_MAGIC);
> > >   }
> > > 
> > >   static int jz4740_rtc_read_alarm(struct device *dev, struct
> > > rtc_wkalrm *alrm)
> > >  @@ -307,6 +312,7 @@ static int jz4740_rtc_probe(struct
> > > platform_device *pdev)
> > >   	struct jz4740_rtc *rtc;
> > >   	unsigned long rate;
> > >   	struct clk *clk;
> > >  +	uint32_t magic;
> > >   	int ret, irq;
> > > 
> > >   	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> > >  @@ -369,6 +375,18 @@ static int jz4740_rtc_probe(struct
> > > platform_device *pdev)
> > >   	/* Each 1 Hz pulse should happen after (rate) ticks */
> > >   	jz4740_rtc_reg_write(rtc, JZ_REG_RTC_REGULATOR, rate - 1);
> > > 
> > >  +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
> > >  +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC) {
> > >  +		/*
> > >  +		 * If the scratchpad doesn't hold our magic value, then a
> > >  +		 * power loss occurred. Reset to Epoch.
> > >  +		 */
> > >  +		struct rtc_time time;
> > >  +
> > >  +		rtc_time64_to_tm(0, &time);
> > >  +		jz4740_rtc_set_time(dev, &time);
> > 
> > Don't do that, this defeats the purpose of detecting when the power is
> > lost. Returning a known bogus time is the worst thing you can do here.
> 
> So what is the best thing to do then?
> 

Well, -EINVAL is returned when the time is invalid, this should be
enough. I'm not actually sure what is the issue you are trying to fix
here.

> Cheers,
> -Paul
> 
> > >  +	}
> > >  +
> > >   	ret = devm_rtc_register_device(rtc->rtc);
> > >   	if (ret)
> > >   		return ret;
> > >  --
> > >  2.35.1
> > > 
> > 
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> 
>
Paul Cercueil April 19, 2022, 8:53 p.m. UTC | #4 
Le mar., avril 19 2022 at 22:00:32 +0200, Alexandre Belloni 
<alexandre.belloni@bootlin.com> a écrit :
> On 19/04/2022 20:48:54+0100, Paul Cercueil wrote:
>>  Hi Alexandre,
>> 
>>  Le mar., avril 19 2022 at 21:35:35 +0200, Alexandre Belloni
>>  <alexandre.belloni@bootlin.com> a écrit :
>>  > On 18/04/2022 19:49:31+0100, Paul Cercueil wrote:
>>  > >  On power loss, reading the RTC value would fail as the 
>> scratchpad
>>  > > lost
>>  > >  its magic value, until the hardware clock was set once again.
>>  > >
>>  > >  To avoid that, reset the RTC value to Epoch in the probe if we
>>  > > detect
>>  > >  that the scratchpad lost its magic value.
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  ---
>>  > >   drivers/rtc/rtc-jz4740.c | 24 +++++++++++++++++++++---
>>  > >   1 file changed, 21 insertions(+), 3 deletions(-)
>>  > >
>>  > >  diff --git a/drivers/rtc/rtc-jz4740.c 
>> b/drivers/rtc/rtc-jz4740.c
>>  > >  index 119baf168b32..aac5f68bf626 100644
>>  > >  --- a/drivers/rtc/rtc-jz4740.c
>>  > >  +++ b/drivers/rtc/rtc-jz4740.c
>>  > >  @@ -42,6 +42,9 @@
>>  > >   /* Magic value to enable writes on jz4780 */
>>  > >   #define JZ_RTC_WENR_MAGIC	0xA55A
>>  > >
>>  > >  +/* Value written to the scratchpad to detect power losses */
>>  > >  +#define JZ_RTC_SCRATCHPAD_MAGIC	0x12345678
>>  > >  +
>>  > >   #define JZ_RTC_WAKEUP_FILTER_MASK	0x0000FFE0
>>  > >   #define JZ_RTC_RESET_COUNTER_MASK	0x00000FE0
>>  > >
>>  > >  @@ -134,10 +137,11 @@ static int 
>> jz4740_rtc_ctrl_set_bits(struct
>>  > > jz4740_rtc *rtc, uint32_t mask,
>>  > >   static int jz4740_rtc_read_time(struct device *dev, struct
>>  > > rtc_time *time)
>>  > >   {
>>  > >   	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>  > >  -	uint32_t secs, secs2;
>>  > >  +	uint32_t secs, secs2, magic;
>>  > >   	int timeout = 5;
>>  > >
>>  > >  -	if (jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD) != 
>> 0x12345678)
>>  > >  +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
>>  > >  +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC)
>>  > >   		return -EINVAL;
>>  > >
>>  > >   	/* If the seconds register is read while it is updated, it 
>> can
>>  > > contain a
>>  > >  @@ -169,7 +173,8 @@ static int jz4740_rtc_set_time(struct 
>> device
>>  > > *dev, struct rtc_time *time)
>>  > >   	if (ret)
>>  > >   		return ret;
>>  > >
>>  > >  -	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
>>  > > 0x12345678);
>>  > >  +	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
>>  > >  +				    JZ_RTC_SCRATCHPAD_MAGIC);
>>  > >   }
>>  > >
>>  > >   static int jz4740_rtc_read_alarm(struct device *dev, struct
>>  > > rtc_wkalrm *alrm)
>>  > >  @@ -307,6 +312,7 @@ static int jz4740_rtc_probe(struct
>>  > > platform_device *pdev)
>>  > >   	struct jz4740_rtc *rtc;
>>  > >   	unsigned long rate;
>>  > >   	struct clk *clk;
>>  > >  +	uint32_t magic;
>>  > >   	int ret, irq;
>>  > >
>>  > >   	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
>>  > >  @@ -369,6 +375,18 @@ static int jz4740_rtc_probe(struct
>>  > > platform_device *pdev)
>>  > >   	/* Each 1 Hz pulse should happen after (rate) ticks */
>>  > >   	jz4740_rtc_reg_write(rtc, JZ_REG_RTC_REGULATOR, rate - 1);
>>  > >
>>  > >  +	magic = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
>>  > >  +	if (magic != JZ_RTC_SCRATCHPAD_MAGIC) {
>>  > >  +		/*
>>  > >  +		 * If the scratchpad doesn't hold our magic value, then a
>>  > >  +		 * power loss occurred. Reset to Epoch.
>>  > >  +		 */
>>  > >  +		struct rtc_time time;
>>  > >  +
>>  > >  +		rtc_time64_to_tm(0, &time);
>>  > >  +		jz4740_rtc_set_time(dev, &time);
>>  >
>>  > Don't do that, this defeats the purpose of detecting when the 
>> power is
>>  > lost. Returning a known bogus time is the worst thing you can do 
>> here.
>> 
>>  So what is the best thing to do then?
>> 
> 
> Well, -EINVAL is returned when the time is invalid, this should be
> enough. I'm not actually sure what is the issue you are trying to fix
> here.

htop fails to start and tells me:
"No btime in /proc/stat: No such file or directory"

until the date is reset. So I was assuming it was a case of the jz4740 
driver not being correct and breaking userspace.

Cheers,
-Paul


> 
>>  Cheers,
>>  -Paul
>> 
>>  > >  +	}
>>  > >  +
>>  > >   	ret = devm_rtc_register_device(rtc->rtc);
>>  > >   	if (ret)
>>  > >   		return ret;
>>  > >  --
>>  > >  2.35.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
Alexandre Belloni May 16, 2022, 1:56 p.m. UTC | #5 
On 19/04/2022 21:53:17+0100, Paul Cercueil wrote:
> > >  So what is the best thing to do then?
> > > 
> > 
> > Well, -EINVAL is returned when the time is invalid, this should be
> > enough. I'm not actually sure what is the issue you are trying to fix
> > here.
> 
> htop fails to start and tells me:
> "No btime in /proc/stat: No such file or directory"
> 
> until the date is reset. So I was assuming it was a case of the jz4740
> driver not being correct and breaking userspace.
> 

I guess either /proc/stat or htop needs fixing then ;)

> Cheers,
> -Paul
> 
> 
> > 
> > >  Cheers,
> > >  -Paul
> > > 
> > >  > >  +	}
> > >  > >  +
> > >  > >   	ret = devm_rtc_register_device(rtc->rtc);
> > >  > >   	if (ret)
> > >  > >   		return ret;
> > >  > >  --
> > >  > >  2.35.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
> 
>