[PATCH 0/3] rtc: armada38x: Few improvement and cleanup

[PATCH 0/3] rtc: armada38x: Few improvement and cleanup

[Gregory CLEMENT]

Hi,

this series brings some improvement and cleanup for the armada 38x
RTC.

The errata for this RTC gave us more information on how to work around
it. The first patch implement it.

The second patch convert the driver to the time64_t usage.

And the last patch make the driver really usable on 64 bits
architecture.

These last two patches are a preliminary step allowing using this
driver on the Armada 7K/8K which are 64 bits ARM SoCs which use the
same RTC IP.

Thanks,

Gregory

Gregory CLEMENT (2):
  rtc: armada38x: Convert to time64_t
  rtc: armada38x: Prepare for being use on 64 bits

Shaker Daibes (1):
  rtc: armada38x: improve RTC errata implementation

 drivers/rtc/rtc-armada38x.c | 145 ++++++++++++++++++++++++++++++--------------
 1 file changed, 100 insertions(+), 45 deletions(-)

-- 
2.10.2

[PATCH 1/3] rtc: armada38x: improve RTC errata implementation

[Gregory CLEMENT]

From: Shaker Daibes <shaker@marvell.com>

According to FE-3124064:
The device supports CPU write and read access to the RTC time register.
However, due to this errata, read from RTC TIME register may fail.

Workaround:
General configuration:
1. Configure the RTC Mbus Bridge Timing Control register (offset 0x184A0)
   to value 0xFD4D4FFF
   Write RTC WRCLK Period to its maximum value (0x3FF)
   Write RTC WRCLK setup to 0x53 (default value )
   Write RTC WRCLK High Time to 0x53 (default value )
   Write RTC Read Output Delay to its maximum value (0x1F)
   Mbus - Read All Byte Enable to 0x1 (default value )
2. Configure the RTC Test Configuration Register (offset 0xA381C) bit3
   to '1' (Reserved, Marvell internal)

For any RTC register read operation:
1. Read the requested register 100 times.
2. Find the result that appears most frequently and use this result
   as the correct value.

For any RTC register write operation:
1. Issue two dummy writes of 0x0 to the RTC Status register (offset
   0xA3800).
2. Write the time to the RTC Time register (offset 0xA380C).

[gregory.clement at free-electrons.com: cosmetic changes and fix issues for
interrupt in original patch]

Reviewed-by: Lior Amsalem <alior@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/rtc/rtc-armada38x.c | 109 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 85 insertions(+), 24 deletions(-)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index 9a3f2a6f512e..a0859286a4c4 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -29,12 +29,21 @@
 #define RTC_TIME	    0xC
 #define RTC_ALARM1	    0x10
 
+#define SOC_RTC_BRIDGE_TIMING_CTL   0x0
+#define SOC_RTC_PERIOD_OFFS		0
+#define SOC_RTC_PERIOD_MASK		(0x3FF << SOC_RTC_PERIOD_OFFS)
+#define SOC_RTC_READ_DELAY_OFFS		26
+#define SOC_RTC_READ_DELAY_MASK		(0x1F << SOC_RTC_READ_DELAY_OFFS)
+
 #define SOC_RTC_INTERRUPT   0x8
 #define SOC_RTC_ALARM1		BIT(0)
 #define SOC_RTC_ALARM2		BIT(1)
 #define SOC_RTC_ALARM1_MASK	BIT(2)
 #define SOC_RTC_ALARM2_MASK	BIT(3)
 
+
+#define SAMPLE_NR 100
+
 struct armada38x_rtc {
 	struct rtc_device   *rtc_dev;
 	void __iomem	    *regs;
@@ -47,32 +56,85 @@ struct armada38x_rtc {
  * According to the datasheet, the OS should wait 5us after every
  * register write to the RTC hard macro so that the required update
  * can occur without holding off the system bus
+ * According to errata FE-3124064, Write to any RTC register
+ * may fail. As a workaround, before writing to RTC
+ * register, issue a dummy write of 0x0 twice to RTC Status
+ * register.
  */
+
 static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
 {
+	writel(0, rtc->regs + RTC_STATUS);
+	writel(0, rtc->regs + RTC_STATUS);
 	writel(val, rtc->regs + offset);
 	udelay(5);
 }
 
+/* Update RTC-MBUS bridge timing parameters */
+static void rtc_update_mbus_timing_params(struct armada38x_rtc *rtc)
+{
+	uint32_t reg;
+
+	reg = readl(rtc->regs_soc + SOC_RTC_BRIDGE_TIMING_CTL);
+	reg &= ~SOC_RTC_PERIOD_MASK;
+	reg |= 0x3FF << SOC_RTC_PERIOD_OFFS; /* Maximum value */
+	reg &= ~SOC_RTC_READ_DELAY_MASK;
+	reg |= 0x1F << SOC_RTC_READ_DELAY_OFFS; /* Maximum value */
+	writel(reg, rtc->regs_soc + SOC_RTC_BRIDGE_TIMING_CTL);
+}
+
+struct str_value_to_freq {
+	unsigned long value;
+	u8 freq;
+} __packed;
+
+static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
+{
+	unsigned long value_array[SAMPLE_NR], i, j, value;
+	unsigned long max = 0, index_max = SAMPLE_NR - 1;
+	struct str_value_to_freq value_to_freq[SAMPLE_NR];
+
+	for (i = 0; i < SAMPLE_NR; i++) {
+		value_to_freq[i].freq = 0;
+		value_array[i] = readl(rtc->regs + rtc_reg);
+	}
+	for (i = 0; i < SAMPLE_NR; i++) {
+		value = value_array[i];
+		/*
+		 * if value appears in value_to_freq array so add the
+		 * counter of value, if didn't appear yet in counters
+		 * array then allocate new member of value_to_freq
+		 * array with counter = 1
+		 */
+		for (j = 0; j < SAMPLE_NR; j++) {
+			if (value_to_freq[j].freq == 0 ||
+					value_to_freq[j].value == value)
+				break;
+			if (j == (SAMPLE_NR - 1))
+				break;
+		}
+		if (value_to_freq[j].freq == 0)
+			value_to_freq[j].value = value;
+		value_to_freq[j].freq++;
+		/* find the most common result */
+		if (max < value_to_freq[j].freq) {
+			index_max = j;
+			max = value_to_freq[j].freq;
+		}
+	}
+	return value_to_freq[index_max].value;
+}
+
 static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	unsigned long time, time_check, flags;
+	unsigned long time, flags;
 
 	spin_lock_irqsave(&rtc->lock, flags);
-	time = readl(rtc->regs + RTC_TIME);
-	/*
-	 * WA for failing time set attempts. As stated in HW ERRATA if
-	 * more than one second between two time reads is detected
-	 * then read once again.
-	 */
-	time_check = readl(rtc->regs + RTC_TIME);
-	if ((time_check - time) > 1)
-		time_check = readl(rtc->regs + RTC_TIME);
-
+	time = read_rtc_register_wa(rtc, RTC_TIME);
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
-	rtc_time_to_tm(time_check, tm);
+	rtc_time_to_tm(time, tm);
 
 	return 0;
 }
@@ -87,16 +149,9 @@ static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 	if (ret)
 		goto out;
-	/*
-	 * According to errata FE-3124064, Write to RTC TIME register
-	 * may fail. As a workaround, after writing to RTC TIME
-	 * register, issue a dummy write of 0x0 twice to RTC Status
-	 * register.
-	 */
+
 	spin_lock_irqsave(&rtc->lock, flags);
 	rtc_delayed_write(time, rtc, RTC_TIME);
-	rtc_delayed_write(0, rtc, RTC_STATUS);
-	rtc_delayed_write(0, rtc, RTC_STATUS);
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
 out:
@@ -111,8 +166,8 @@ static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	spin_lock_irqsave(&rtc->lock, flags);
 
-	time = readl(rtc->regs + RTC_ALARM1);
-	val = readl(rtc->regs + RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
+	time = read_rtc_register_wa(rtc, RTC_ALARM1);
+	val = read_rtc_register_wa(rtc, RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
 
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
@@ -182,7 +237,7 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
 	val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
 
 	writel(val & ~SOC_RTC_ALARM1, rtc->regs_soc + SOC_RTC_INTERRUPT);
-	val = readl(rtc->regs + RTC_IRQ1_CONF);
+	val = read_rtc_register_wa(rtc, RTC_IRQ1_CONF);
 	/* disable all the interrupts for alarm 1 */
 	rtc_delayed_write(0, rtc, RTC_IRQ1_CONF);
 	/* Ack the event */
@@ -196,7 +251,6 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
 		else
 			event |= RTC_PF;
 	}
-
 	rtc_update_irq(rtc->rtc_dev, 1, event);
 
 	return IRQ_HANDLED;
@@ -253,6 +307,9 @@ static __init int armada38x_rtc_probe(struct platform_device *pdev)
 	if (rtc->irq != -1)
 		device_init_wakeup(&pdev->dev, 1);
 
+	/* Update RTC-MBUS bridge timing parameters */
+	rtc_update_mbus_timing_params(rtc);
+
 	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
 					&armada38x_rtc_ops, THIS_MODULE);
 	if (IS_ERR(rtc->rtc_dev)) {
@@ -260,6 +317,7 @@ static __init int armada38x_rtc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
 		return ret;
 	}
+
 	return 0;
 }
 
@@ -280,6 +338,9 @@ static int armada38x_rtc_resume(struct device *dev)
 	if (device_may_wakeup(dev)) {
 		struct armada38x_rtc *rtc = dev_get_drvdata(dev);
 
+		/* Update RTC-MBUS bridge timing parameters */
+		rtc_update_mbus_timing_params(rtc);
+
 		return disable_irq_wake(rtc->irq);
 	}
 
-- 
2.10.2

[PATCH 1/3] rtc: armada38x: improve RTC errata implementation

[Andrew Lunn]

> +struct str_value_to_freq {
> +	unsigned long value;
> +	u8 freq;
> +} __packed;
> +
> +static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
> +{
> +	unsigned long value_array[SAMPLE_NR], i, j, value;
> +	unsigned long max = 0, index_max = SAMPLE_NR - 1;
> +	struct str_value_to_freq value_to_freq[SAMPLE_NR];

Hi Gregory

This appears to be putting over 900 bytes on the stack. Is there any
danger of overflowing the stack? Would it be safer to make these
arrays part of armada38x_rtc?

       Andrew

[PATCH 1/3] rtc: armada38x: improve RTC errata implementation

[Gregory CLEMENT]

Hi Andrew,
 
 On jeu., d?c. 08 2016, Andrew Lunn <andrew@lunn.ch> wrote:

>> +struct str_value_to_freq {
>> +	unsigned long value;
>> +	u8 freq;
>> +} __packed;
>> +
>> +static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
>> +{
>> +	unsigned long value_array[SAMPLE_NR], i, j, value;
>> +	unsigned long max = 0, index_max = SAMPLE_NR - 1;
>> +	struct str_value_to_freq value_to_freq[SAMPLE_NR];
>
> Hi Gregory
>
> This appears to be putting over 900 bytes on the stack. Is there any

Actually the structure being packed it is 500 bytes.

> danger of overflowing the stack? Would it be safer to make these
> arrays part of armada38x_rtc?

We could do this if you fear a stack overflow.

Gregory

>
>        Andrew

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] rtc: armada38x: improve RTC errata implementation

[Andrew Lunn]

On Fri, Dec 09, 2016 at 05:19:07PM +0100, Gregory CLEMENT wrote:
> Hi Andrew,
>  
>  On jeu., d?c. 08 2016, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> +struct str_value_to_freq {
> >> +	unsigned long value;
> >> +	u8 freq;
> >> +} __packed;
> >> +
> >> +static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
> >> +{
> >> +	unsigned long value_array[SAMPLE_NR], i, j, value;
> >> +	unsigned long max = 0, index_max = SAMPLE_NR - 1;
> >> +	struct str_value_to_freq value_to_freq[SAMPLE_NR];
> >
> > Hi Gregory
> >
> > This appears to be putting over 900 bytes on the stack. Is there any
> 
> Actually the structure being packed it is 500 bytes.

Did you verify this? I never remember where the __packed needs to go.
You clearly have a packed structure, but is the array of structures
packed?

And the long value_array[SAMPLE_NR] is another 400 bytes, totalling
900. And as Russell pointed out, this is on 32 bit systems. Until your
third patch, 64 bit systems probably have double that. I would also
suggest squashing patch #3 into #1.

> > danger of overflowing the stack? Would it be safer to make these
> > arrays part of armada38x_rtc?
> 
> We could do this if you fear a stack overflow.

It is generally consider not a good idea to put > $BIG structures on
the stack, but the value of $BIG is not clearly defined. Stack
overflow seems to be an issue with lots of layering going on, swap on
NFS etc. But it seems unlikely to me reading the RTC will happen with
an already deep stack. So this is probably O.K.

   Andrew

[PATCH 1/3] rtc: armada38x: improve RTC errata implementation

[Russell King - ARM Linux]

On Thu, Dec 08, 2016 at 06:10:08PM +0100, Gregory CLEMENT wrote:
> From: Shaker Daibes <shaker@marvell.com>
> 
> According to FE-3124064:
> The device supports CPU write and read access to the RTC time register.
> However, due to this errata, read from RTC TIME register may fail.
> 
> Workaround:
> General configuration:
> 1. Configure the RTC Mbus Bridge Timing Control register (offset 0x184A0)
>    to value 0xFD4D4FFF
>    Write RTC WRCLK Period to its maximum value (0x3FF)
>    Write RTC WRCLK setup to 0x53 (default value )
>    Write RTC WRCLK High Time to 0x53 (default value )
>    Write RTC Read Output Delay to its maximum value (0x1F)
>    Mbus - Read All Byte Enable to 0x1 (default value )
> 2. Configure the RTC Test Configuration Register (offset 0xA381C) bit3
>    to '1' (Reserved, Marvell internal)
> 
> For any RTC register read operation:
> 1. Read the requested register 100 times.
> 2. Find the result that appears most frequently and use this result
>    as the correct value.
> 
> For any RTC register write operation:
> 1. Issue two dummy writes of 0x0 to the RTC Status register (offset
>    0xA3800).
> 2. Write the time to the RTC Time register (offset 0xA380C).
> 
> [gregory.clement at free-electrons.com: cosmetic changes and fix issues for
> interrupt in original patch]

A particularly interesting question here is whether the above description
came first or whether the code below came first, and which was derived
from which.

Given that both readl() writel() involves a barrier operation, which is
not mentioned in the above workaround text, is it appropriate to use the
barriered operations?

> 
> Reviewed-by: Lior Amsalem <alior@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/rtc/rtc-armada38x.c | 109 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 85 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> index 9a3f2a6f512e..a0859286a4c4 100644
> --- a/drivers/rtc/rtc-armada38x.c
> +++ b/drivers/rtc/rtc-armada38x.c
> @@ -29,12 +29,21 @@
>  #define RTC_TIME	    0xC
>  #define RTC_ALARM1	    0x10
>  
> +#define SOC_RTC_BRIDGE_TIMING_CTL   0x0
> +#define SOC_RTC_PERIOD_OFFS		0
> +#define SOC_RTC_PERIOD_MASK		(0x3FF << SOC_RTC_PERIOD_OFFS)
> +#define SOC_RTC_READ_DELAY_OFFS		26
> +#define SOC_RTC_READ_DELAY_MASK		(0x1F << SOC_RTC_READ_DELAY_OFFS)
> +
>  #define SOC_RTC_INTERRUPT   0x8
>  #define SOC_RTC_ALARM1		BIT(0)
>  #define SOC_RTC_ALARM2		BIT(1)
>  #define SOC_RTC_ALARM1_MASK	BIT(2)
>  #define SOC_RTC_ALARM2_MASK	BIT(3)
>  
> +
> +#define SAMPLE_NR 100
> +
>  struct armada38x_rtc {
>  	struct rtc_device   *rtc_dev;
>  	void __iomem	    *regs;
> @@ -47,32 +56,85 @@ struct armada38x_rtc {
>   * According to the datasheet, the OS should wait 5us after every
>   * register write to the RTC hard macro so that the required update
>   * can occur without holding off the system bus
> + * According to errata FE-3124064, Write to any RTC register
> + * may fail. As a workaround, before writing to RTC
> + * register, issue a dummy write of 0x0 twice to RTC Status
> + * register.
>   */
> +
>  static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
>  {
> +	writel(0, rtc->regs + RTC_STATUS);
> +	writel(0, rtc->regs + RTC_STATUS);
>  	writel(val, rtc->regs + offset);
>  	udelay(5);
>  }
>  
> +/* Update RTC-MBUS bridge timing parameters */
> +static void rtc_update_mbus_timing_params(struct armada38x_rtc *rtc)
> +{
> +	uint32_t reg;
> +
> +	reg = readl(rtc->regs_soc + SOC_RTC_BRIDGE_TIMING_CTL);
> +	reg &= ~SOC_RTC_PERIOD_MASK;
> +	reg |= 0x3FF << SOC_RTC_PERIOD_OFFS; /* Maximum value */
> +	reg &= ~SOC_RTC_READ_DELAY_MASK;
> +	reg |= 0x1F << SOC_RTC_READ_DELAY_OFFS; /* Maximum value */
> +	writel(reg, rtc->regs_soc + SOC_RTC_BRIDGE_TIMING_CTL);
> +}
> +
> +struct str_value_to_freq {
> +	unsigned long value;
> +	u8 freq;
> +} __packed;

Why packed?  This makes accesses to this structure very inefficient -
the compiler for some CPUs will load "value" by bytes.

As you're targetting 32-bit and 64-bit, why the use of "unsigned long"
here - readl() returns a u32, guaranteed to be 32-bit.  "unsigned long"
will be 64-bit on ARM64, so wastes 32-bits per sample, completely
negating any advantage of that __packed attribute.

> +
> +static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
> +{
> +	unsigned long value_array[SAMPLE_NR], i, j, value;
> +	unsigned long max = 0, index_max = SAMPLE_NR - 1;
> +	struct str_value_to_freq value_to_freq[SAMPLE_NR];

Consider using int or unsigned int for loop variables and indexes, but
something other than unsigned long to store the results from reading
hardware registers (same reason as above.)

> +
> +	for (i = 0; i < SAMPLE_NR; i++) {
> +		value_to_freq[i].freq = 0;
> +		value_array[i] = readl(rtc->regs + rtc_reg);
> +	}
> +	for (i = 0; i < SAMPLE_NR; i++) {
> +		value = value_array[i];
> +		/*
> +		 * if value appears in value_to_freq array so add the
> +		 * counter of value, if didn't appear yet in counters
> +		 * array then allocate new member of value_to_freq
> +		 * array with counter = 1
> +		 */
> +		for (j = 0; j < SAMPLE_NR; j++) {
> +			if (value_to_freq[j].freq == 0 ||
> +					value_to_freq[j].value == value)
> +				break;
> +			if (j == (SAMPLE_NR - 1))
> +				break;
> +		}
> +		if (value_to_freq[j].freq == 0)
> +			value_to_freq[j].value = value;
> +		value_to_freq[j].freq++;
> +		/* find the most common result */
> +		if (max < value_to_freq[j].freq) {
> +			index_max = j;
> +			max = value_to_freq[j].freq;
> +		}
> +	}
> +	return value_to_freq[index_max].value;
> +}
> +
>  static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
> -	unsigned long time, time_check, flags;
> +	unsigned long time, flags;
>  
>  	spin_lock_irqsave(&rtc->lock, flags);
> -	time = readl(rtc->regs + RTC_TIME);
> -	/*
> -	 * WA for failing time set attempts. As stated in HW ERRATA if
> -	 * more than one second between two time reads is detected
> -	 * then read once again.
> -	 */
> -	time_check = readl(rtc->regs + RTC_TIME);
> -	if ((time_check - time) > 1)
> -		time_check = readl(rtc->regs + RTC_TIME);
> -
> +	time = read_rtc_register_wa(rtc, RTC_TIME);
>  	spin_unlock_irqrestore(&rtc->lock, flags);
>  
> -	rtc_time_to_tm(time_check, tm);
> +	rtc_time_to_tm(time, tm);
>  
>  	return 0;
>  }
> @@ -87,16 +149,9 @@ static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  
>  	if (ret)
>  		goto out;
> -	/*
> -	 * According to errata FE-3124064, Write to RTC TIME register
> -	 * may fail. As a workaround, after writing to RTC TIME
> -	 * register, issue a dummy write of 0x0 twice to RTC Status
> -	 * register.
> -	 */
> +
>  	spin_lock_irqsave(&rtc->lock, flags);
>  	rtc_delayed_write(time, rtc, RTC_TIME);
> -	rtc_delayed_write(0, rtc, RTC_STATUS);
> -	rtc_delayed_write(0, rtc, RTC_STATUS);
>  	spin_unlock_irqrestore(&rtc->lock, flags);
>  
>  out:
> @@ -111,8 +166,8 @@ static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  
>  	spin_lock_irqsave(&rtc->lock, flags);
>  
> -	time = readl(rtc->regs + RTC_ALARM1);
> -	val = readl(rtc->regs + RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
> +	time = read_rtc_register_wa(rtc, RTC_ALARM1);
> +	val = read_rtc_register_wa(rtc, RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
>  
>  	spin_unlock_irqrestore(&rtc->lock, flags);
>  
> @@ -182,7 +237,7 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
>  	val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
>  
>  	writel(val & ~SOC_RTC_ALARM1, rtc->regs_soc + SOC_RTC_INTERRUPT);
> -	val = readl(rtc->regs + RTC_IRQ1_CONF);
> +	val = read_rtc_register_wa(rtc, RTC_IRQ1_CONF);
>  	/* disable all the interrupts for alarm 1 */
>  	rtc_delayed_write(0, rtc, RTC_IRQ1_CONF);
>  	/* Ack the event */
> @@ -196,7 +251,6 @@ static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
>  		else
>  			event |= RTC_PF;
>  	}
> -
>  	rtc_update_irq(rtc->rtc_dev, 1, event);
>  
>  	return IRQ_HANDLED;
> @@ -253,6 +307,9 @@ static __init int armada38x_rtc_probe(struct platform_device *pdev)
>  	if (rtc->irq != -1)
>  		device_init_wakeup(&pdev->dev, 1);
>  
> +	/* Update RTC-MBUS bridge timing parameters */
> +	rtc_update_mbus_timing_params(rtc);
> +
>  	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
>  					&armada38x_rtc_ops, THIS_MODULE);
>  	if (IS_ERR(rtc->rtc_dev)) {
> @@ -260,6 +317,7 @@ static __init int armada38x_rtc_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
>  		return ret;
>  	}
> +
>  	return 0;
>  }
>  
> @@ -280,6 +338,9 @@ static int armada38x_rtc_resume(struct device *dev)
>  	if (device_may_wakeup(dev)) {
>  		struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>  
> +		/* Update RTC-MBUS bridge timing parameters */
> +		rtc_update_mbus_timing_params(rtc);
> +
>  		return disable_irq_wake(rtc->irq);
>  	}
>  
> -- 
> 2.10.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/3] rtc: armada38x: improve RTC errata implementation

[Gregory CLEMENT]

Hi Russell King,
 
 On jeu., d?c. 08 2016, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Thu, Dec 08, 2016 at 06:10:08PM +0100, Gregory CLEMENT wrote:
>> From: Shaker Daibes <shaker@marvell.com>
>> 
>> According to FE-3124064:
>> The device supports CPU write and read access to the RTC time register.
>> However, due to this errata, read from RTC TIME register may fail.
>> 
>> Workaround:
>> General configuration:
>> 1. Configure the RTC Mbus Bridge Timing Control register (offset 0x184A0)
>>    to value 0xFD4D4FFF
>>    Write RTC WRCLK Period to its maximum value (0x3FF)
>>    Write RTC WRCLK setup to 0x53 (default value )
>>    Write RTC WRCLK High Time to 0x53 (default value )
>>    Write RTC Read Output Delay to its maximum value (0x1F)
>>    Mbus - Read All Byte Enable to 0x1 (default value )
>> 2. Configure the RTC Test Configuration Register (offset 0xA381C) bit3
>>    to '1' (Reserved, Marvell internal)
>> 
>> For any RTC register read operation:
>> 1. Read the requested register 100 times.
>> 2. Find the result that appears most frequently and use this result
>>    as the correct value.
>> 
>> For any RTC register write operation:
>> 1. Issue two dummy writes of 0x0 to the RTC Status register (offset
>>    0xA3800).
>> 2. Write the time to the RTC Time register (offset 0xA380C).
>> 
>> [gregory.clement at free-electrons.com: cosmetic changes and fix issues for
>> interrupt in original patch]
>
> A particularly interesting question here is whether the above description
> came first or whether the code below came first, and which was derived
> from which.

Well, it is difficult to say. the above description comes from the
errata datasheet (since rev B). But the errata sheet itself mentioned
the software implementation in one of the Marvell release. 


>
> Given that both readl() writel() involves a barrier operation, which is
> not mentioned in the above workaround text, is it appropriate to use the
> barriered operations?

Do you suggest to use the the relaxed version of readl() and writel()?

>
>> 
>> Reviewed-by: Lior Amsalem <alior@marvell.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/rtc/rtc-armada38x.c | 109 ++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 85 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
>> index 9a3f2a6f512e..a0859286a4c4 100644
>> --- a/drivers/rtc/rtc-armada38x.c
>> +++ b/drivers/rtc/rtc-armada38x.c
>> @@ -29,12 +29,21 @@
>>  #define RTC_TIME	    0xC
>>  #define RTC_ALARM1	    0x10
>>  
>> +#define SOC_RTC_BRIDGE_TIMING_CTL   0x0
>> +#define SOC_RTC_PERIOD_OFFS		0
>> +#define SOC_RTC_PERIOD_MASK		(0x3FF << SOC_RTC_PERIOD_OFFS)
>> +#define SOC_RTC_READ_DELAY_OFFS		26
>> +#define SOC_RTC_READ_DELAY_MASK		(0x1F << SOC_RTC_READ_DELAY_OFFS)
>> +
>>  #define SOC_RTC_INTERRUPT   0x8
>>  #define SOC_RTC_ALARM1		BIT(0)
>>  #define SOC_RTC_ALARM2		BIT(1)
>>  #define SOC_RTC_ALARM1_MASK	BIT(2)
>>  #define SOC_RTC_ALARM2_MASK	BIT(3)
>>  
>> +
>> +#define SAMPLE_NR 100
>> +
>>  struct armada38x_rtc {
>>  	struct rtc_device   *rtc_dev;
>>  	void __iomem	    *regs;
>> @@ -47,32 +56,85 @@ struct armada38x_rtc {
>>   * According to the datasheet, the OS should wait 5us after every
>>   * register write to the RTC hard macro so that the required update
>>   * can occur without holding off the system bus
>> + * According to errata FE-3124064, Write to any RTC register
>> + * may fail. As a workaround, before writing to RTC
>> + * register, issue a dummy write of 0x0 twice to RTC Status
>> + * register.
>>   */
>> +
>>  static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
>>  {
>> +	writel(0, rtc->regs + RTC_STATUS);
>> +	writel(0, rtc->regs + RTC_STATUS);
>>  	writel(val, rtc->regs + offset);
>>  	udelay(5);
>>  }
>>  
>> +/* Update RTC-MBUS bridge timing parameters */
>> +static void rtc_update_mbus_timing_params(struct armada38x_rtc *rtc)
>> +{
>> +	uint32_t reg;
>> +
>> +	reg = readl(rtc->regs_soc + SOC_RTC_BRIDGE_TIMING_CTL);
>> +	reg &= ~SOC_RTC_PERIOD_MASK;
>> +	reg |= 0x3FF << SOC_RTC_PERIOD_OFFS; /* Maximum value */
>> +	reg &= ~SOC_RTC_READ_DELAY_MASK;
>> +	reg |= 0x1F << SOC_RTC_READ_DELAY_OFFS; /* Maximum value */
>> +	writel(reg, rtc->regs_soc + SOC_RTC_BRIDGE_TIMING_CTL);
>> +}
>> +
>> +struct str_value_to_freq {
>> +	unsigned long value;
>> +	u8 freq;
>> +} __packed;
>
> Why packed?  This makes accesses to this structure very inefficient -
> the compiler for some CPUs will load "value" by bytes.

As pointed by Andrew, I think the intent was to reduce the amount of
memory used from the stack.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/3] rtc: armada38x: Convert to time64_t

[Gregory CLEMENT]

It is one more step to remove the deprecated functions rtc_time_to_tm
and rtc_tm_to_time.

And as bonus it simplifies a little the driver.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/rtc/rtc-armada38x.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index a0859286a4c4..b8a74ffaae80 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -128,13 +128,14 @@ static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
 static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	unsigned long time, flags;
+	unsigned long flags;
+	time64_t time;
 
 	spin_lock_irqsave(&rtc->lock, flags);
-	time = read_rtc_register_wa(rtc, RTC_TIME);
+	time = (time64_t)read_rtc_register_wa(rtc, RTC_TIME);
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
-	rtc_time_to_tm(time, tm);
+	rtc_time64_to_tm(time, tm);
 
 	return 0;
 }
@@ -142,37 +143,34 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	int ret = 0;
-	unsigned long time, flags;
-
-	ret = rtc_tm_to_time(tm, &time);
+	unsigned long flags;
+	time64_t time;
 
-	if (ret)
-		goto out;
+	time = rtc_tm_to_time64(tm);
 
 	spin_lock_irqsave(&rtc->lock, flags);
-	rtc_delayed_write(time, rtc, RTC_TIME);
+	rtc_delayed_write((u32)time, rtc, RTC_TIME);
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
-out:
-	return ret;
+	return 0;
 }
 
 static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	unsigned long time, flags;
+	unsigned long flags;
+	time64_t time;
 	u32 val;
 
 	spin_lock_irqsave(&rtc->lock, flags);
 
-	time = read_rtc_register_wa(rtc, RTC_ALARM1);
+	time = (time64_t)read_rtc_register_wa(rtc, RTC_ALARM1);
 	val = read_rtc_register_wa(rtc, RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
 
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
 	alrm->enabled = val ? 1 : 0;
-	rtc_time_to_tm(time,  &alrm->time);
+	rtc_time64_to_tm(time,  &alrm->time);
 
 	return 0;
 }
@@ -180,18 +178,15 @@ static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	unsigned long time, flags;
-	int ret = 0;
+	unsigned long flags;
+	time64_t time;
 	u32 val;
 
-	ret = rtc_tm_to_time(&alrm->time, &time);
-
-	if (ret)
-		goto out;
+	time = rtc_tm_to_time64(&alrm->time);
 
 	spin_lock_irqsave(&rtc->lock, flags);
 
-	rtc_delayed_write(time, rtc, RTC_ALARM1);
+	rtc_delayed_write((u32)time, rtc, RTC_ALARM1);
 
 	if (alrm->enabled) {
 			rtc_delayed_write(RTC_IRQ1_AL_EN, rtc, RTC_IRQ1_CONF);
@@ -202,8 +197,7 @@ static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
-out:
-	return ret;
+	return 0;
 }
 
 static int armada38x_rtc_alarm_irq_enable(struct device *dev,
-- 
2.10.2

[PATCH 2/3] rtc: armada38x: Convert to time64_t

[Russell King - ARM Linux]

On Thu, Dec 08, 2016 at 06:10:09PM +0100, Gregory CLEMENT wrote:
> It is one more step to remove the deprecated functions rtc_time_to_tm
> and rtc_tm_to_time.

I fail to see any advantage to this patch, or in fact the y2038 patch
merged into rtc.

Let's first look at the original rtc_time_to_tm():

+void rtc_time_to_tm(unsigned long time, struct rtc_time *tm)
+{
+       days = time / 86400;
+       year = 1970 + days / 365;

time is an unsigned long, so is 32-bit or 64-bit.  Let's assume 32-bit.
The maximum value it can have is 2^32-1 - which gives a maximum days
value of 49710 (it always fits in an int or unsigned int as a positive
value on any arch supported by Linux.)

That gives a maximum value of 'year' as 2106.  So, actually
rtc_time_to_tm() doesn't have a y2038 problem, it has a y2106 problem.
So, the commentary in the original commit is actually wrong!


Okay, that aside, let's look at what your patch actually achieves.

Old code to set the time:

	unsigned long time;

	ret = rtc_tm_to_time(tm, &time);
	if (ret)
		goto out;
	/* write time to a 32-bit register */

New code:

	time64_t time;

	time = rtc_tm_to_time64(tm);
	/* write time to a 32-bit register */

I fail to see how this is any kind of improvement, or aids to solve the
y2106 problem (not y2038) here.

A better approach would have been to also provide a rtc_tm_to_time32()
following the function signature of rtc_tm_to_time(), but replacing the
unsigned long pointer with a u32 pointer, and (importantly) making it
fail if the time was unrepresentable as a u32 value.

Why u32 and not time_t?  time_t is a signed value, which suffers from
the y2038 problem.  u32 suffers from a y2106 problem, and is what we
have with the original rtc_tm_to_time() on 32-bit platforms.  So it's
safe _not_ to needlessly chop off a bit there which wasn't done in the
original code.

This would mean that drivers (such as armada38x) which are only capable
of storing a 32-bit time fail gracefully to set times beyond y2106.

The problem as I see it right now is that the y2038 patch which
deprecates rtc_time_to_tm() etc wasn't thought out thoroughly enough,
and is making the situation much worse by pushing the problem in many
drivers in a way that results in it being less visible.

I think this needs to be reconsidered, and it needs a much better
implementation - one which at least tells the user that their hardware
has broken when trying to set the time to something the hardware can't
support.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 3/3] rtc: armada38x: Prepare for being use on 64 bits

[Gregory CLEMENT]

The drivers are supposed to be portable, however there are few assumption
done here about the unsigned long size. Make sure we use the accurate
width for the variable.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/rtc/rtc-armada38x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index b8a74ffaae80..c4138130febf 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -84,14 +84,14 @@ static void rtc_update_mbus_timing_params(struct armada38x_rtc *rtc)
 }
 
 struct str_value_to_freq {
-	unsigned long value;
+	u32 value;
 	u8 freq;
 } __packed;
 
-static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
+static u32 read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
 {
-	unsigned long value_array[SAMPLE_NR], i, j, value;
-	unsigned long max = 0, index_max = SAMPLE_NR - 1;
+	int i, j, index_max = SAMPLE_NR - 1;
+	u32 value_array[SAMPLE_NR], value, max = 0;
 	struct str_value_to_freq value_to_freq[SAMPLE_NR];
 
 	for (i = 0; i < SAMPLE_NR; i++) {
-- 
2.10.2

[PATCH 3/3] rtc: armada38x: Prepare for being use on 64 bits

[Russell King - ARM Linux]

On Thu, Dec 08, 2016 at 06:10:10PM +0100, Gregory CLEMENT wrote:
> The drivers are supposed to be portable, however there are few assumption
> done here about the unsigned long size. Make sure we use the accurate
> width for the variable.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/rtc/rtc-armada38x.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> index b8a74ffaae80..c4138130febf 100644
> --- a/drivers/rtc/rtc-armada38x.c
> +++ b/drivers/rtc/rtc-armada38x.c
> @@ -84,14 +84,14 @@ static void rtc_update_mbus_timing_params(struct armada38x_rtc *rtc)
>  }
>  
>  struct str_value_to_freq {
> -	unsigned long value;
> +	u32 value;
>  	u8 freq;
>  } __packed;
>  
> -static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
> +static u32 read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg)
>  {
> -	unsigned long value_array[SAMPLE_NR], i, j, value;
> -	unsigned long max = 0, index_max = SAMPLE_NR - 1;
> +	int i, j, index_max = SAMPLE_NR - 1;
> +	u32 value_array[SAMPLE_NR], value, max = 0;

Ah, I see my comments on patch 1 got addressed in patch 3... :)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.