Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
From: Maxime Ripard <maxime@cerno.tech>
To: "Jernej Škrabec" <jernej.skrabec@siol.net>
Cc: wens@csie.org, robh+dt@kernel.org, mark.rutland@arm.com,
	a.zummo@towertech.it, alexandre.belloni@bootlin.com,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-rtc@vger.kernel.org
Subject: Re: [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional
Date: Thu, 20 Feb 2020 18:47:49 +0100
Message-ID: <20200220174749.ih3pcj3oxiwvuurz@gilmour.lan> (raw)
In-Reply-To: <5326350.DvuYhMxLoT@jernej-laptop>

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

On Fri, Feb 14, 2020 at 05:42:13PM +0100, Jernej Škrabec wrote:
> Hi Maxime,
>
> Dne petek, 14. februar 2020 ob 09:14:43 CET je Maxime Ripard napisal(a):
> > Hi Jernej,
> >
> > Thanks for taking care of this
> >
> > On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote:
> > > Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix TX6
> > > (H6) don't have external 32kHz oscillator. Till H6, it didn't really
> > > matter if external oscillator was enabled because HW detected error and
> > > fall back to internal one. H6 has same functionality but it's the first
> > > SoC which have "auto switch bypass" bit documented and always enabled in
> > > driver. This prevents RTC to work correctly if external crystal is not
> > > present on board. There are other side effects - all peripherals which
> > > depends on this clock also don't work (HDMI CEC for example).
> > >
> > > Make clocks property optional. If it is present, select external
> > > oscillator. If not, stay on internal.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > >
> > >  drivers/rtc/rtc-sun6i.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > index 852f5f3b3592..538cf7e19034 100644
> > > --- a/drivers/rtc/rtc-sun6i.c
> > > +++ b/drivers/rtc/rtc-sun6i.c
> > > @@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct
> > > device_node *node,>
> > >  		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > >
> > >  	}
> > >
> > > -	/* Switch to the external, more precise, oscillator */
> > > -	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
> > > -	if (rtc->data->has_losc_en)
> > > -		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > +	/* Switch to the external, more precise, oscillator, if present */
> > > +	if (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;
> > > +	}
> > >
> > >  	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > >
> > >  	/* Yes, I know, this is ugly. */
> > >  	sun6i_rtc = rtc;
> > >
> > > -	/* Deal with old DTs */
> > > -	if (!of_get_property(node, "clocks", NULL))
> > > -		goto err;
> > > -
> >
> > Doesn't that prevent the parents to be properly set if there's an
> > external crystal?
>
> No, why?
>
> Check these two clk_summary:
> http://ix.io/2bHY Tanix TX6 (no external crystal)
> http://ix.io/2bI2 OrangePi 3 (external crystal present)

I was concerned about the "other" parent. In the case where you don't
have a clocks property (so the check that you are removing), the code
then registers a clock with two parents: the one that we create (the
internal oscillator) and the one coming from the clocks property.

clk_summary only shows the current parent, which is going to be right
with your patch, but in the case where you have no clocks property,
you still (attempts to) register two parents, the second one being
non-functional.

Further looking at it, we might be good because we allocate an array
of two clocks, but only register of_clk_get_parent_count(node) + 1
clocks, so 1 if clocks is missing.

Still, I think this should be more obvious, through a comment or
shuffling a bit the parent registration maybe?

Maxime

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

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13 21:14 [PATCH 0/2] rtc: sun6i: Make external " Jernej Skrabec
2020-02-13 21:14 ` [PATCH 1/2] rtc: sun6i: Make external 32k " Jernej Skrabec
2020-02-14  8:14   ` Maxime Ripard
2020-02-14 16:42     ` Jernej Škrabec
2020-02-20 17:47       ` Maxime Ripard [this message]
2020-02-20 17:59         ` Jernej Škrabec
2020-02-13 21:14 ` [PATCH 2/2] arm64: dts: allwinner: h6: Move ext. oscillator to board DTs Jernej Skrabec

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200220174749.ih3pcj3oxiwvuurz@gilmour.lan \
    --to=maxime@cerno.tech \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jernej.skrabec@siol.net \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git