diff mbox series

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

Message ID 20210615110636.23403-7-andre.przywara@arm.com
State New
Headers show
Series None | expand

Commit Message

Andre Przywara June 15, 2021, 11:06 a.m. UTC
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(-)

Comments

Maxime Ripard June 16, 2021, 9:14 a.m. UTC | #1
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
Andre Przywara June 16, 2021, 10:14 a.m. UTC | #2
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
Maxime Ripard June 16, 2021, 1:47 p.m. UTC | #3
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
Andre Przywara July 22, 2021, 11:17 p.m. UTC | #4
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
Maxime Ripard July 26, 2021, 2:59 p.m. UTC | #5
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
Icenowy Zheng July 29, 2021, 8:04 a.m. UTC | #6
在 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
Maxime Ripard July 29, 2021, 10:32 a.m. UTC | #7
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
Icenowy Zheng July 29, 2021, 1:04 p.m. UTC | #8
于 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
diff mbox series

Patch

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);