[PATCH v7 03/19] dt-bindings: rtc: sun6i: Add H616 compatible string

[PATCH v7 03/19] dt-bindings: rtc: sun6i: Add H616 compatible string

[Andre Przywara]

Add the obvious compatible name to the existing RTC binding.
The actual RTC part of the device uses a different day/month/year
storage scheme, so it's not compatible with the previous devices.
Also the clock part is quite different, as there is no external 32K LOSC
oscillator input.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml     | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
index b1b0ee769b71..2c3fd72e17ee 100644
--- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
@@ -26,6 +26,7 @@ properties:
           - const: allwinner,sun50i-a64-rtc
           - const: allwinner,sun8i-h3-rtc
       - const: allwinner,sun50i-h6-rtc
+      - const: allwinner,sun50i-h616-rtc
 
   reg:
     maxItems: 1
@@ -105,6 +106,20 @@ allOf:
           minItems: 3
           maxItems: 3
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: allwinner,sun50i-h616-rtc
+
+    then:
+      properties:
+        clock-output-names:
+          minItems: 3
+          maxItems: 3
+        clocks:
+          maxItems: 0
+
   - if:
       properties:
         compatible:
-- 
2.17.5

Re: [PATCH v7 03/19] dt-bindings: rtc: sun6i: Add H616 compatible string

[Rob Herring]

On Tue, Jun 15, 2021 at 12:06:20PM +0100, Andre Przywara wrote:
> Add the obvious compatible name to the existing RTC binding.
> The actual RTC part of the device uses a different day/month/year
> storage scheme, so it's not compatible with the previous devices.
> Also the clock part is quite different, as there is no external 32K LOSC
> oscillator input.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml     | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> index b1b0ee769b71..2c3fd72e17ee 100644
> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> @@ -26,6 +26,7 @@ properties:
>            - const: allwinner,sun50i-a64-rtc
>            - const: allwinner,sun8i-h3-rtc
>        - const: allwinner,sun50i-h6-rtc
> +      - const: allwinner,sun50i-h616-rtc
>  
>    reg:
>      maxItems: 1
> @@ -105,6 +106,20 @@ allOf:
>            minItems: 3
>            maxItems: 3
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: allwinner,sun50i-h616-rtc
> +
> +    then:
> +      properties:
> +        clock-output-names:
> +          minItems: 3
> +          maxItems: 3
> +        clocks:
> +          maxItems: 0

clocks: false

if forbidding clocks is what you want.

> +
>    - if:
>        properties:
>          compatible:
> -- 
> 2.17.5

Re: [PATCH v7 03/19] dt-bindings: rtc: sun6i: Add H616 compatible string

[Andre Przywara]

On Tue, 15 Jun 2021 17:35:02 -0600
Rob Herring <robh@kernel.org> wrote:

Hi,

> On Tue, Jun 15, 2021 at 12:06:20PM +0100, Andre Przywara wrote:
> > Add the obvious compatible name to the existing RTC binding.
> > The actual RTC part of the device uses a different day/month/year
> > storage scheme, so it's not compatible with the previous devices.
> > Also the clock part is quite different, as there is no external 32K LOSC
> > oscillator input.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml     | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> > index b1b0ee769b71..2c3fd72e17ee 100644
> > --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> > @@ -26,6 +26,7 @@ properties:
> >            - const: allwinner,sun50i-a64-rtc
> >            - const: allwinner,sun8i-h3-rtc
> >        - const: allwinner,sun50i-h6-rtc
> > +      - const: allwinner,sun50i-h616-rtc
> >  
> >    reg:
> >      maxItems: 1
> > @@ -105,6 +106,20 @@ allOf:
> >            minItems: 3
> >            maxItems: 3
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: allwinner,sun50i-h616-rtc
> > +
> > +    then:
> > +      properties:
> > +        clock-output-names:
> > +          minItems: 3
> > +          maxItems: 3
> > +        clocks:
> > +          maxItems: 0  
> 
> clocks: false
> 
> if forbidding clocks is what you want.

Yes, thanks for the hint!

Cheers,
Andre

> 
> > +
> >    - if:
> >        properties:
> >          compatible:
> > -- 
> > 2.17.5  
> 

[PATCH v7 04/19] rtc: sun6i: Add support for linear day storage

[Andre Przywara]

Newer versions of the Allwinner RTC, as for instance found in the H616
SoC, no longer store a broken-down day/month/year representation in the
RTC_DAY_REG, but just a linear day number.
The user manual does not give any indication about the expected epoch
time of this day count, but the BSP kernel uses the UNIX epoch, which
allows easy support due to existing conversion functions in the kernel.

Allow tagging a compatible string with a flag, and use that to mark
those new RTCs. Then convert between a UNIX day number (converted into
seconds) and the broken-down day representation using mktime64() and
time64_to_tm() in the set_time/get_time functions.

That enables support for the RTC in those new chips.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 66 +++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index adec1b14a8de..e4fc6e4f2bfb 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -110,6 +110,8 @@
 #define SUN6I_YEAR_MIN				1970
 #define SUN6I_YEAR_OFF				(SUN6I_YEAR_MIN - 1900)
 
+#define SEC_PER_DAY				(24 * 3600ULL)
+
 /*
  * There are other differences between models, including:
  *
@@ -133,12 +135,15 @@ struct sun6i_rtc_clk_data {
 	unsigned int has_auto_swt : 1;
 };
 
+#define RTC_LINEAR_DAY	BIT(0)
+
 struct sun6i_rtc_dev {
 	struct rtc_device *rtc;
 	const struct sun6i_rtc_clk_data *data;
 	void __iomem *base;
 	int irq;
 	unsigned long alarm;
+	unsigned long flags;
 
 	struct clk_hw hw;
 	struct clk_hw *int_osc;
@@ -467,22 +472,30 @@ static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 	} while ((date != readl(chip->base + SUN6I_RTC_YMD)) ||
 		 (time != readl(chip->base + SUN6I_RTC_HMS)));
 
+	if (chip->flags & RTC_LINEAR_DAY) {
+		/*
+		 * Newer chips store a linear day number, the manual
+		 * does not mandate any epoch base. The BSP driver uses
+		 * the UNIX epoch, let's just copy that, as it's the
+		 * easiest anyway.
+		 */
+		rtc_time64_to_tm((date & 0xffff) * SEC_PER_DAY, rtc_tm);
+	} else {
+		rtc_tm->tm_mday = SUN6I_DATE_GET_DAY_VALUE(date);
+		rtc_tm->tm_mon  = SUN6I_DATE_GET_MON_VALUE(date) - 1;
+		rtc_tm->tm_year = SUN6I_DATE_GET_YEAR_VALUE(date);
+
+		/*
+		 * switch from (data_year->min)-relative offset to
+		 * a (1900)-relative one
+		 */
+		rtc_tm->tm_year += SUN6I_YEAR_OFF;
+	}
+
 	rtc_tm->tm_sec  = SUN6I_TIME_GET_SEC_VALUE(time);
 	rtc_tm->tm_min  = SUN6I_TIME_GET_MIN_VALUE(time);
 	rtc_tm->tm_hour = SUN6I_TIME_GET_HOUR_VALUE(time);
 
-	rtc_tm->tm_mday = SUN6I_DATE_GET_DAY_VALUE(date);
-	rtc_tm->tm_mon  = SUN6I_DATE_GET_MON_VALUE(date);
-	rtc_tm->tm_year = SUN6I_DATE_GET_YEAR_VALUE(date);
-
-	rtc_tm->tm_mon  -= 1;
-
-	/*
-	 * switch from (data_year->min)-relative offset to
-	 * a (1900)-relative one
-	 */
-	rtc_tm->tm_year += SUN6I_YEAR_OFF;
-
 	return 0;
 }
 
@@ -571,20 +584,27 @@ static int sun6i_rtc_settime(struct device *dev, struct rtc_time *rtc_tm)
 	u32 date = 0;
 	u32 time = 0;
 
-	rtc_tm->tm_year -= SUN6I_YEAR_OFF;
-	rtc_tm->tm_mon += 1;
-
-	date = SUN6I_DATE_SET_DAY_VALUE(rtc_tm->tm_mday) |
-		SUN6I_DATE_SET_MON_VALUE(rtc_tm->tm_mon)  |
-		SUN6I_DATE_SET_YEAR_VALUE(rtc_tm->tm_year);
-
-	if (is_leap_year(rtc_tm->tm_year + SUN6I_YEAR_MIN))
-		date |= SUN6I_LEAP_SET_VALUE(1);
-
 	time = SUN6I_TIME_SET_SEC_VALUE(rtc_tm->tm_sec)  |
 		SUN6I_TIME_SET_MIN_VALUE(rtc_tm->tm_min)  |
 		SUN6I_TIME_SET_HOUR_VALUE(rtc_tm->tm_hour);
 
+	if (chip->flags & RTC_LINEAR_DAY) {
+		rtc_tm->tm_sec = 0;
+		rtc_tm->tm_min = 0;
+		rtc_tm->tm_hour = 0;
+		date = rtc_tm_to_time64(rtc_tm) / SEC_PER_DAY;
+	} else {
+		rtc_tm->tm_year -= SUN6I_YEAR_OFF;
+		rtc_tm->tm_mon += 1;
+
+		date = SUN6I_DATE_SET_DAY_VALUE(rtc_tm->tm_mday) |
+			SUN6I_DATE_SET_MON_VALUE(rtc_tm->tm_mon)  |
+			SUN6I_DATE_SET_YEAR_VALUE(rtc_tm->tm_year);
+
+		if (is_leap_year(rtc_tm->tm_year + SUN6I_YEAR_MIN))
+			date |= SUN6I_LEAP_SET_VALUE(1);
+	}
+
 	/* Check whether registers are writable */
 	if (sun6i_rtc_wait(chip, SUN6I_LOSC_CTRL,
 			   SUN6I_LOSC_CTRL_ACC_MASK, 50)) {
@@ -678,6 +698,8 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, chip);
 
+	chip->flags = (unsigned long)of_device_get_match_data(&pdev->dev);
+
 	chip->irq = platform_get_irq(pdev, 0);
 	if (chip->irq < 0)
 		return chip->irq;
-- 
2.17.5

[PATCH v7 05/19] rtc: sun6i: Add support for broken-down alarm registers

[Andre Przywara]

Newer versions of the Allwinner RTC, for instance as found in the H616
SoC, not only store the current day as a linear number, but also change
the way the alarm is handled: There are now two registers, that
explicitly store the wakeup time, in the same format as the current
time.

Add support for that variant by writing the requested wakeup time
directly into the registers, instead of programming the seconds left, as
the old SoCs required.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 60 +++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index e4fc6e4f2bfb..54bd47fb0a5f 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -48,7 +48,8 @@
 
 /* Alarm 0 (counter) */
 #define SUN6I_ALRM_COUNTER			0x0020
-#define SUN6I_ALRM_CUR_VAL			0x0024
+/* This holds the remaining alarm seconds on older SoCs (current value) */
+#define SUN6I_ALRM_COUNTER_HMS			0x0024
 #define SUN6I_ALRM_EN				0x0028
 #define SUN6I_ALRM_EN_CNT_EN			BIT(0)
 #define SUN6I_ALRM_IRQ_EN			0x002c
@@ -523,36 +524,55 @@ static int sun6i_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 	struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
 	struct rtc_time *alrm_tm = &wkalrm->time;
 	struct rtc_time tm_now;
-	unsigned long time_now = 0;
 	unsigned long time_set = 0;
-	unsigned long time_gap = 0;
+	unsigned long counter_val, counter_val_hms;
 	int ret = 0;
 
-	ret = sun6i_rtc_gettime(dev, &tm_now);
-	if (ret < 0) {
-		dev_err(dev, "Error in getting time\n");
-		return -EINVAL;
-	}
-
 	time_set = rtc_tm_to_time64(alrm_tm);
-	time_now = rtc_tm_to_time64(&tm_now);
-	if (time_set <= time_now) {
-		dev_err(dev, "Date to set in the past\n");
-		return -EINVAL;
-	}
-
-	time_gap = time_set - time_now;
 
-	if (time_gap > U32_MAX) {
-		dev_err(dev, "Date too far in the future\n");
-		return -EINVAL;
+	if (chip->flags & RTC_LINEAR_DAY) {
+		/*
+		 * The alarm registers hold the actual alarm time, encoded
+		 * in the same way (linear day + HMS) as the current time.
+		 */
+		counter_val_hms = SUN6I_TIME_SET_SEC_VALUE(alrm_tm->tm_sec)  |
+				  SUN6I_TIME_SET_MIN_VALUE(alrm_tm->tm_min)  |
+				  SUN6I_TIME_SET_HOUR_VALUE(alrm_tm->tm_hour);
+		counter_val = mktime64(alrm_tm->tm_year + 1900, alrm_tm->tm_mon,
+				       alrm_tm->tm_mday, 0, 0, 0) / SEC_PER_DAY;
+	} else {
+		/* The alarm register holds the number of seconds left. */
+		unsigned long time_now;
+
+		ret = sun6i_rtc_gettime(dev, &tm_now);
+		if (ret < 0) {
+			dev_err(dev, "Error in getting time\n");
+			return -EINVAL;
+		}
+
+		time_now = rtc_tm_to_time64(&tm_now);
+		if (time_set <= time_now) {
+			dev_err(dev, "Date to set in the past\n");
+			return -EINVAL;
+		}
+
+		counter_val = time_set - time_now;
+
+		if (counter_val > U32_MAX) {
+			dev_err(dev, "Date too far in the future\n");
+			return -EINVAL;
+		}
 	}
 
 	sun6i_rtc_setaie(0, chip);
 	writel(0, chip->base + SUN6I_ALRM_COUNTER);
+	if (chip->flags & RTC_LINEAR_DAY)
+		writel(0, chip->base + SUN6I_ALRM_COUNTER_HMS);
 	usleep_range(100, 300);
 
-	writel(time_gap, chip->base + SUN6I_ALRM_COUNTER);
+	writel(counter_val, chip->base + SUN6I_ALRM_COUNTER);
+	if (chip->flags & RTC_LINEAR_DAY)
+		writel(counter_val_hms, chip->base + SUN6I_ALRM_COUNTER_HMS);
 	chip->alarm = time_set;
 
 	sun6i_rtc_setaie(wkalrm->enabled, chip);
-- 
2.17.5

[PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Andre Przywara]

Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack
a pin for an external 32768 Hz oscillator. As a consequence, this LOSC
can't be selected as the RTC clock source, and we must rely on the
internal RC oscillator.
To allow additions of clocks to the RTC node, add a feature bit to ignore
any provided clocks for now (the current code would think this is the
external LOSC). Later DTs and code can then for instance add the PLL
based clock input, and older kernel won't get confused.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 54bd47fb0a5f..1fabb3c69041 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -134,6 +134,7 @@ struct sun6i_rtc_clk_data {
 	unsigned int export_iosc : 1;
 	unsigned int has_losc_en : 1;
 	unsigned int has_auto_swt : 1;
+	unsigned int no_ext_losc : 1;
 };
 
 #define RTC_LINEAR_DAY	BIT(0)
@@ -256,7 +257,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
 	}
 
 	/* Switch to the external, more precise, oscillator, if present */
-	if (of_get_property(node, "clocks", NULL)) {
+	if (!rtc->data->no_ext_losc && of_get_property(node, "clocks", NULL)) {
 		reg |= SUN6I_LOSC_CTRL_EXT_OSC;
 		if (rtc->data->has_losc_en)
 			reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
@@ -282,14 +283,19 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
 	}
 
 	parents[0] = clk_hw_get_name(rtc->int_osc);
-	/* If there is no external oscillator, this will be NULL and ... */
-	parents[1] = of_clk_get_parent_name(node, 0);
+	if (rtc->data->no_ext_losc) {
+		parents[1] = NULL;
+		init.num_parents = 1;
+	} else {
+		/* If there is no external oscillator, this will be NULL and */
+		parents[1] = of_clk_get_parent_name(node, 0);
+		/* ... number of clock parents will be 1. */
+		init.num_parents = of_clk_get_parent_count(node) + 1;
+	}
 
 	rtc->hw.init = &init;
 
 	init.parent_names = parents;
-	/* ... number of clock parents will be 1. */
-	init.num_parents = of_clk_get_parent_count(node) + 1;
 	of_property_read_string_index(node, "clock-output-names", 0,
 				      &init.name);
 
-- 
2.17.5

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Maxime Ripard]

Hi,

On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack
> a pin for an external 32768 Hz oscillator. As a consequence, this LOSC
> can't be selected as the RTC clock source, and we must rely on the
> internal RC oscillator.
> To allow additions of clocks to the RTC node, add a feature bit to ignore
> any provided clocks for now (the current code would think this is the
> external LOSC). Later DTs and code can then for instance add the PLL
> based clock input, and older kernel won't get confused.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Honestly, I don't really know if it's worth it at this point.

If we sums this up:

 - The RTC has 2 features that we use, mostly centered around 2
   registers set plus a global one

 - Those 2 features are programmed in a completely different way

 - Even the common part is different, given the discussion around the
   clocks that we have.

What is there to share in that driver aside from the probe, and maybe
the interrupt handling? Instead of complicating this further with more
special case that you were (rightfully) complaining about, shouldn't we
just acknowledge the fact that it's a completely separate design and
should be treated as such, with a completely separate driver?

Maxime

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Andre Przywara]

On Wed, 16 Jun 2021 11:14:31 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

Hi,

> On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> > Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack
> > a pin for an external 32768 Hz oscillator. As a consequence, this LOSC
> > can't be selected as the RTC clock source, and we must rely on the
> > internal RC oscillator.
> > To allow additions of clocks to the RTC node, add a feature bit to ignore
> > any provided clocks for now (the current code would think this is the
> > external LOSC). Later DTs and code can then for instance add the PLL
> > based clock input, and older kernel won't get confused.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Honestly, I don't really know if it's worth it at this point.
> 
> If we sums this up:
> 
>  - The RTC has 2 features that we use, mostly centered around 2
>    registers set plus a global one
> 
>  - Those 2 features are programmed in a completely different way
> 
>  - Even the common part is different, given the discussion around the
>    clocks that we have.
> 
> What is there to share in that driver aside from the probe, and maybe
> the interrupt handling? Instead of complicating this further with more
> special case that you were (rightfully) complaining about, shouldn't we
> just acknowledge the fact that it's a completely separate design and
> should be treated as such, with a completely separate driver?

If you mean to have a separate clock driver, and one RTC driver: I
agree, and IIUC Samuel has a prototype, covering the H6 and D1 as well:
https://github.com/smaeul/linux/commit/6f8f761db1d8dd4b6abf006fb7e2427da79321c2

The only problem I see that they are sharing MMIO registers. Maybe it
works because the RTC part never touches anything below 0x10, and the
clock part just needs the first four registers?
But this means we can't easily change this for the H6, as the
existing H6 RTC code adds 0x10 to the MMIO base, and also old DTs will
have the RTC base address in their RTC reg property.

If we can somehow solve this (let the clock driver point to the RTC
node to get a regmap?) I am all in, for the reasons you mentioned.

Cheers,
Andre

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

On Wed, Jun 16, 2021 at 11:14:52AM +0100, Andre Przywara wrote:
> On Wed, 16 Jun 2021 11:14:31 +0200
> Maxime Ripard <maxime@cerno.tech> wrote:
> 
> Hi,
> 
> > On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> > > Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack
> > > a pin for an external 32768 Hz oscillator. As a consequence, this LOSC
> > > can't be selected as the RTC clock source, and we must rely on the
> > > internal RC oscillator.
> > > To allow additions of clocks to the RTC node, add a feature bit to ignore
> > > any provided clocks for now (the current code would think this is the
> > > external LOSC). Later DTs and code can then for instance add the PLL
> > > based clock input, and older kernel won't get confused.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> > 
> > Honestly, I don't really know if it's worth it at this point.
> > 
> > If we sums this up:
> > 
> >  - The RTC has 2 features that we use, mostly centered around 2
> >    registers set plus a global one
> > 
> >  - Those 2 features are programmed in a completely different way
> > 
> >  - Even the common part is different, given the discussion around the
> >    clocks that we have.
> > 
> > What is there to share in that driver aside from the probe, and maybe
> > the interrupt handling? Instead of complicating this further with more
> > special case that you were (rightfully) complaining about, shouldn't we
> > just acknowledge the fact that it's a completely separate design and
> > should be treated as such, with a completely separate driver?
> 
> If you mean to have a separate clock driver, and one RTC driver: I
> agree, and IIUC Samuel has a prototype, covering the H6 and D1 as well:
> https://github.com/smaeul/linux/commit/6f8f761db1d8dd4b6abf006fb7e2427da79321c2
> 
> The only problem I see that they are sharing MMIO registers. Maybe it
> works because the RTC part never touches anything below 0x10, and the
> clock part just needs the first four registers?
> But this means we can't easily change this for the H6, as the
> existing H6 RTC code adds 0x10 to the MMIO base, and also old DTs will
> have the RTC base address in their RTC reg property.
> 
> If we can somehow solve this (let the clock driver point to the RTC
> node to get a regmap?) I am all in, for the reasons you mentioned.

I meant having a separate RTC+clocks driver. I'm not really sure why we
need to split them.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Andre Przywara]

On Wed, 16 Jun 2021 11:14:31 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

Hi Maxime,

> On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> > Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack
> > a pin for an external 32768 Hz oscillator. As a consequence, this LOSC
> > can't be selected as the RTC clock source, and we must rely on the
> > internal RC oscillator.
> > To allow additions of clocks to the RTC node, add a feature bit to ignore
> > any provided clocks for now (the current code would think this is the
> > external LOSC). Later DTs and code can then for instance add the PLL
> > based clock input, and older kernel won't get confused.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Honestly, I don't really know if it's worth it at this point.
> 
> If we sums this up:
> 
>  - The RTC has 2 features that we use, mostly centered around 2
>    registers set plus a global one
> 
>  - Those 2 features are programmed in a completely different way
> 
>  - Even the common part is different, given the discussion around the
>    clocks that we have.
> 
> What is there to share in that driver aside from the probe, and maybe
> the interrupt handling? Instead of complicating this further with more
> special case that you were (rightfully) complaining about, shouldn't we
> just acknowledge the fact that it's a completely separate design and
> should be treated as such, with a completely separate driver?

So I had a look, and I don't think it justifies a separate driver:
- Indeed it looks like the core functionality is different, but there
  are a lot of commonalities, with all the RTC and driver boilerplate,
  register offsets, and also the special access pattern (rtc_wait and
  rtc_setaie).
- The actual difference is really in the way the *date* is stored
  (the time is still in 24h H/M/S format), and the missing LOSC input
  clock - which is already optional for existing devices. The two
  patches just make this obvious, by using if() statements at the parts
  where they differ.

So we would end up with possibly some shared .c file, and two driver
front-end files, which I am not sure is really worth it.

Next I thought about providing separate rtc_class_ops, but even they
share a lot of code, so they would be possibly be calling a shared
function each. I don't think that is really better.

If you dislike the rather large if/else branches in the previous two
patches, I could move that out into separate functions, but I feel this
is more code, for no real benefit.

So for now I am tempted to keep it shared. I think Samuel had ideas for
bigger changes in the clock part, at which point we could revisit this
decision - for instance keep the RTC part (still quite similar) mostly
in a shared file, while modelling the clocks in separate files - in a
more "common clock" style for the new SoCs.

Feel free to disagree, but when I tried to actually separate the drivers
it just felt wrong.

Cheers,
Andre

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3449 bytes --]

On Fri, Jul 23, 2021 at 12:17:21AM +0100, Andre Przywara wrote:
> On Wed, 16 Jun 2021 11:14:31 +0200
> Maxime Ripard <maxime@cerno.tech> wrote:
> 
> Hi Maxime,
> 
> > On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> > > Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack
> > > a pin for an external 32768 Hz oscillator. As a consequence, this LOSC
> > > can't be selected as the RTC clock source, and we must rely on the
> > > internal RC oscillator.
> > > To allow additions of clocks to the RTC node, add a feature bit to ignore
> > > any provided clocks for now (the current code would think this is the
> > > external LOSC). Later DTs and code can then for instance add the PLL
> > > based clock input, and older kernel won't get confused.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> > 
> > Honestly, I don't really know if it's worth it at this point.
> > 
> > If we sums this up:
> > 
> >  - The RTC has 2 features that we use, mostly centered around 2
> >    registers set plus a global one
> > 
> >  - Those 2 features are programmed in a completely different way
> > 
> >  - Even the common part is different, given the discussion around the
> >    clocks that we have.
> > 
> > What is there to share in that driver aside from the probe, and maybe
> > the interrupt handling? Instead of complicating this further with more
> > special case that you were (rightfully) complaining about, shouldn't we
> > just acknowledge the fact that it's a completely separate design and
> > should be treated as such, with a completely separate driver?
> 
> So I had a look, and I don't think it justifies a separate driver:
> - Indeed it looks like the core functionality is different, but there
>   are a lot of commonalities, with all the RTC and driver boilerplate,
>   register offsets, and also the special access pattern (rtc_wait and
>   rtc_setaie).
> - The actual difference is really in the way the *date* is stored
>   (the time is still in 24h H/M/S format), and the missing LOSC input
>   clock - which is already optional for existing devices. The two
>   patches just make this obvious, by using if() statements at the parts
>   where they differ.

My point was that the code that is shared, like the driver boilerplate,
is much more complicated than it can be precisely because it's shared.

I'd take two simple-but-with-some-redundancy drivers over one big,
shared but complicated driver any day.

But fine, I guess.

> So we would end up with possibly some shared .c file, and two driver
> front-end files, which I am not sure is really worth it.
> 
> Next I thought about providing separate rtc_class_ops, but even they
> share a lot of code, so they would be possibly be calling a shared
> function each. I don't think that is really better.
> 
> If you dislike the rather large if/else branches in the previous two
> patches, I could move that out into separate functions, but I feel this
> is more code, for no real benefit.
> 
> So for now I am tempted to keep it shared. I think Samuel had ideas for
> bigger changes in the clock part, at which point we could revisit this
> decision - for instance keep the RTC part (still quite similar) mostly
> in a shared file, while modelling the clocks in separate files - in a
> more "common clock" style for the new SoCs.

What's the plan?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Icenowy Zheng]

在 2021-06-16星期三的 11:14 +0200，Maxime Ripard写道：
> Hi,
> 
> On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> > Some newer Allwinner RTCs (for instance the one in the H616 SoC)
> > lack
> > a pin for an external 32768 Hz oscillator. As a consequence, this
> > LOSC
> > can't be selected as the RTC clock source, and we must rely on the
> > internal RC oscillator.
> > To allow additions of clocks to the RTC node, add a feature bit to
> > ignore
> > any provided clocks for now (the current code would think this is
> > the
> > external LOSC). Later DTs and code can then for instance add the
> > PLL
> > based clock input, and older kernel won't get confused.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> Honestly, I don't really know if it's worth it at this point.
> 
> If we sums this up:
> 
>  - The RTC has 2 features that we use, mostly centered around 2
>    registers set plus a global one
> 
>  - Those 2 features are programmed in a completely different way
> 
>  - Even the common part is different, given the discussion around the
>    clocks that we have.
> 
> What is there to share in that driver aside from the probe, and maybe
> the interrupt handling? Instead of complicating this further with
> more
> special case that you were (rightfully) complaining about, shouldn't
> we
> just acknowledge the fact that it's a completely separate design and
> should be treated as such, with a completely separate driver?

I think our problem is just that we're having a single driver for both
functionalities (clock manager and RTC).

Personally I don't think we should have seperated driver for clock
managers, although I am fine with seperated RTC driver for linear days.

By the way, not having a LOSC is only what happens on H616, maybe
because there should never be a battery-backed H616 device. On R329,
the RTC part has linear day storage, but it still have LOSC. Because of
this, I don't think we should duplicate at least at least the current
sun6i-rtc dual-functionality driver, because the clock funtionality is
just the same with previous SoCs on R329.

> 
> Maxime

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Maxime Ripard]

On Thu, Jul 29, 2021 at 04:04:10PM +0800, Icenowy Zheng wrote:
> 在 2021-06-16星期三的 11:14 +0200，Maxime Ripard写道：
> > Hi,
> > 
> > On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
> > > Some newer Allwinner RTCs (for instance the one in the H616 SoC)
> > > lack
> > > a pin for an external 32768 Hz oscillator. As a consequence, this
> > > LOSC
> > > can't be selected as the RTC clock source, and we must rely on the
> > > internal RC oscillator.
> > > To allow additions of clocks to the RTC node, add a feature bit to
> > > ignore
> > > any provided clocks for now (the current code would think this is
> > > the
> > > external LOSC). Later DTs and code can then for instance add the
> > > PLL
> > > based clock input, and older kernel won't get confused.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > 
> > Honestly, I don't really know if it's worth it at this point.
> > 
> > If we sums this up:
> > 
> >  - The RTC has 2 features that we use, mostly centered around 2
> >    registers set plus a global one
> > 
> >  - Those 2 features are programmed in a completely different way
> > 
> >  - Even the common part is different, given the discussion around the
> >    clocks that we have.
> > 
> > What is there to share in that driver aside from the probe, and maybe
> > the interrupt handling? Instead of complicating this further with
> > more
> > special case that you were (rightfully) complaining about, shouldn't
> > we
> > just acknowledge the fact that it's a completely separate design and
> > should be treated as such, with a completely separate driver?
> 
> I think our problem is just that we're having a single driver for both
> functionalities (clock manager and RTC).
> 
> Personally I don't think we should have seperated driver for clock
> managers, although I am fine with seperated RTC driver for linear days.

Why do you think it's a bad idea to have the RTC and clocks in the same
driver?

Maxime

Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs

[Icenowy Zheng]

于 2021年7月29日 GMT+08:00 下午6:32:28, Maxime Ripard <maxime@cerno.tech> 写到:
>On Thu, Jul 29, 2021 at 04:04:10PM +0800, Icenowy Zheng wrote:
>> 在 2021-06-16星期三的 11:14 +0200，Maxime Ripard写道：
>> > Hi,
>> > 
>> > On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote:
>> > > Some newer Allwinner RTCs (for instance the one in the H616 SoC)
>> > > lack
>> > > a pin for an external 32768 Hz oscillator. As a consequence, this
>> > > LOSC
>> > > can't be selected as the RTC clock source, and we must rely on the
>> > > internal RC oscillator.
>> > > To allow additions of clocks to the RTC node, add a feature bit to
>> > > ignore
>> > > any provided clocks for now (the current code would think this is
>> > > the
>> > > external LOSC). Later DTs and code can then for instance add the
>> > > PLL
>> > > based clock input, and older kernel won't get confused.
>> > > 
>> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> > 
>> > Honestly, I don't really know if it's worth it at this point.
>> > 
>> > If we sums this up:
>> > 
>> >  - The RTC has 2 features that we use, mostly centered around 2
>> >    registers set plus a global one
>> > 
>> >  - Those 2 features are programmed in a completely different way
>> > 
>> >  - Even the common part is different, given the discussion around the
>> >    clocks that we have.
>> > 
>> > What is there to share in that driver aside from the probe, and maybe
>> > the interrupt handling? Instead of complicating this further with
>> > more
>> > special case that you were (rightfully) complaining about, shouldn't
>> > we
>> > just acknowledge the fact that it's a completely separate design and
>> > should be treated as such, with a completely separate driver?
>> 
>> I think our problem is just that we're having a single driver for both
>> functionalities (clock manager and RTC).
>> 
>> Personally I don't think we should have seperated driver for clock
>> managers, although I am fine with seperated RTC driver for linear days.
>
>Why do you think it's a bad idea to have the RTC and clocks in the same
>driver?

Well you really misread, I'm just thinking we shouldn't have a new driver
from scratch. As we're going to have a single sun6i-rtc, please allow H616
and R329 to enter it.

>
>Maxime

[PATCH v7 07/19] rtc: sun6i: Add Allwinner H616 support

[Andre Przywara]

The H616 RTC changes its day storage to the newly introduced linear day
scheme, so pair the new compatible string with this feature flag.
The clock part is missing an external 32768 Hz oscillator input pin,
for future expansion we must thus ignore any provided clock for now.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 1fabb3c69041..25dae50019af 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -392,6 +392,23 @@ static void __init sun50i_h6_rtc_clk_init(struct device_node *node)
 CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_clk, "allwinner,sun50i-h6-rtc",
 		      sun50i_h6_rtc_clk_init);
 
+static const struct sun6i_rtc_clk_data sun50i_h616_rtc_data = {
+	.rc_osc_rate = 16000000,
+	.fixed_prescaler = 32,
+	.has_prescaler = 1,
+	.has_out_clk = 1,
+	.export_iosc = 1,
+	.no_ext_losc = 1,
+};
+
+static void __init sun50i_h616_rtc_clk_init(struct device_node *node)
+{
+	sun6i_rtc_clk_init(node, &sun50i_h616_rtc_data);
+}
+
+CLK_OF_DECLARE_DRIVER(sun50i_h616_rtc_clk, "allwinner,sun50i-h616-rtc",
+		      sun50i_h616_rtc_clk_init);
+
 /*
  * The R40 user manual is self-conflicting on whether the prescaler is
  * fixed or configurable. The clock diagram shows it as fixed, but there
@@ -797,6 +814,8 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
 	{ .compatible = "allwinner,sun8i-v3-rtc" },
 	{ .compatible = "allwinner,sun50i-h5-rtc" },
 	{ .compatible = "allwinner,sun50i-h6-rtc" },
+	{ .compatible = "allwinner,sun50i-h616-rtc",
+		.data = (void *)RTC_LINEAR_DAY },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
-- 
2.17.5