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

From: Maxime Ripard <maxime@cerno.tech>
To: Andre Przywara <andre.przywara@arm.com>
Cc: Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Rob Herring <robh@kernel.org>, Icenowy Zheng <icenowy@aosc.io>,
	Samuel Holland <samuel@sholland.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev,
	linux-kernel@vger.kernel.org, Ondrej Jirman <megous@megous.com>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-rtc@vger.kernel.org
Subject: Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs
Date: Wed, 16 Jun 2021 15:47:49 +0200	[thread overview]
Message-ID: <20210616134749.mzp52dvbjmiabgl3@gilmour> (raw)
In-Reply-To: <20210616111452.1d7f2423@slackpad.fritz.box>

[-- 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 --]