| Message ID | 20210615110636.23403-7-andre.przywara@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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
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
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
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
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
在 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
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
于 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 --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);
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(-)