linux-kernel - Re: [PATCH v3 1/1] drivers/rtc: rtc-sun6i: AutoCal Internal OSC Clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240503122723.37b511e5@donnerap.manchester.arm.com>
Date: Fri, 3 May 2024 12:27:23 +0100
From: Andre Przywara <andre.przywara@...s.arm.com>
To: "Ryan Walklin" <ryan@...ttoast.com>
Cc: "Alois Fertl" <a.fertl@...nline.de>, a.zummo@...ertech.it,
 alexandre.belloni@...tlin.com, "Chen-Yu Tsai" <wens@...e.org>, "Jernej
 Skrabec" <jernej.skrabec@...il.com>, "Samuel Holland"
 <samuel@...lland.org>, linux-rtc@...r.kernel.org,
 linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] drivers/rtc: rtc-sun6i: AutoCal Internal OSC
 Clock

On Fri, 03 May 2024 23:02:13 +1200
"Ryan Walklin" <ryan@...ttoast.com> wrote:

Hi,

> On Fri, 3 May 2024, at 6:07 AM, Alois Fertl wrote:
> > I have a M98-8K PLUS Magcubic TV-Box based on the Allwinner H618 SOC.
> > On board is a Sp6330 wifi/bt module that requires a 32kHz clock to
> > operate correctly. Without this change the clock from the SOC is
> > ~29kHz and BT module does not start up. The patch enables the Internal
> > OSC Clock Auto Calibration of the H616/H618 which than provides the
> > necessary 32kHz and the BT module initializes successfully.
> > Add a flag and set it for H6 AND H616. The H618 is the same as H616
> > regarding rtc.
> >
> > Signed-off-by: Alois Fertl <a.fertl@...nline.de>
> > ---
> >
> > v1->v2
> > - add flag and activate for H6 AND H616
> >
> > v2->v3
> > - correct findings from review
> >
> > I was hoping to get some feedback regarding the situation on H6,
> > where an external 32k crystal can be present.
> > From what I understand from the H6 manual there should be no
> > conflict as one can select which souce will drive the output.
> > Should certainly be tested but I don`t have H6 hardware.
> >
> >  drivers/rtc/rtc-sun6i.c | 17 ++++++++++++++++-
> >
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index e0b85a0d5645..20e81ccdef29 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -42,6 +42,11 @@
> > 
> >  #define SUN6I_LOSC_CLK_PRESCAL			0x0008
> > 
> > +#define SUN6I_LOSC_CLK_AUTO_CAL			0x000c
> > +#define SUN6I_LOSC_CLK_AUTO_CAL_16MS		BIT(2)
> > +#define SUN6I_LOSC_CLK_AUTO_CAL_ENABLE		BIT(1)
> > +#define SUN6I_LOSC_CLK_AUTO_CAL_SEL_CAL		BIT(0)
> > +
> >  /* RTC */
> >  #define SUN6I_RTC_YMD				0x0010
> >  #define SUN6I_RTC_HMS				0x0014
> > @@ -126,7 +131,6 @@
> >   *     registers (R40, H6)
> >   *   - SYS power domain controls (R40)
> >   *   - DCXO controls (H6)
> > - *   - RC oscillator calibration (H6)
> >   *
> >   * These functions are not covered by this driver.
> >   */
> > @@ -138,6 +142,7 @@ struct sun6i_rtc_clk_data {
> >  	unsigned int has_losc_en : 1;
> >  	unsigned int has_auto_swt : 1;
> >  	unsigned int no_ext_losc : 1;
> > +	unsigned int has_auto_cal : 1;
> >  };
> > 
> >  #define RTC_LINEAR_DAY	BIT(0)
> > @@ -268,6 +273,14 @@ static void __init sun6i_rtc_clk_init(struct 
> > device_node *node,
> >  	}
> >  	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > 
> > +	if (rtc->data->has_auto_cal) {
> > +		/* Enable internal OSC clock auto calibration */
> > +		reg = SUN6I_LOSC_CLK_AUTO_CAL_16MS |
> > +			SUN6I_LOSC_CLK_AUTO_CAL_ENABLE |
> > +			SUN6I_LOSC_CLK_AUTO_CAL_SEL_CAL;
> > +		writel(reg, rtc->base + SUN6I_LOSC_CLK_AUTO_CAL);
> > +	}
> > +
> >  	/* Yes, I know, this is ugly. */
> >  	sun6i_rtc = rtc;
> > 
> > @@ -380,6 +393,7 @@ static const struct sun6i_rtc_clk_data 
> > sun50i_h6_rtc_data = {
> >  	.has_out_clk = 1,
> >  	.has_losc_en = 1,
> >  	.has_auto_swt = 1,
> > +	.has_auto_cal = 1,
> >  };
> > 
> >  static void __init sun50i_h6_rtc_clk_init(struct device_node *node)  
> 
> I tried to test this with my H700 board but this struct is not actually in any mainline kernel and looks like it is from an earlier version of the H616 RTC changes which was never actually merged?

Yes, thanks for the heads up, Ryan. Is this some Armbian branch, by any
chance?

> Talked to Andre on IRC and he thinks the H6 and H616 RTC clocks *should*
> be equivalent.

Yeah, that was my first naive idea, but it's actually not true, and the
increasing complexity of the RTC *clocks* were the trigger for the new
driver. That doesn't mean that the IOSC calibration routine cannot be the
same between the H6 and H616, though, but as it stands that would need to
live in two drivers (see my other email).

Cheers,
Andre

> 
> > @@ -395,6 +409,7 @@ static const struct sun6i_rtc_clk_data 
> > sun50i_h616_rtc_data = {
> >  	.has_prescaler = 1,
> >  	.has_out_clk = 1,
> >  	.no_ext_losc = 1,
> > +	.has_auto_cal = 1,
> >  };
> > 
> >  static void __init sun50i_h616_rtc_clk_init(struct device_node *node)
> > -- 
> > 2.39.2  
> 
> Regards,
> 
> Ryan
>