Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC

["Ondřej Jirman" <megous@megous.com>]

On Sat, Aug 24, 2019 at 02:51:54PM +0200, Jernej Škrabec wrote:
> Dne sobota, 24. avgust 2019 ob 14:46:54 CEST je Ondřej Jirman napisal(a):
> > Hi,
> > 
> > On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> > > Hi!
> > > 
> > > Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com 
> napisal(a):
> > > > From: Ondrej Jirman <megous@megous.com>
> > > > 
> > > > RTC on H6 is mostly the same as on H5 and H3. It has slight differences
> > > > mostly in features that are not yet supported by this driver.
> > > > 
> > > > Some differences are already stated in the comments in existing code.
> > > > One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
> > > > EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> > > > 
> > > > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
> > > > external low speed oscillator is working correctly.
> > > > 
> > > > This patch adds support for enabling LOSC when necessary:
> > > > 
> > > > - during reparenting
> > > > - when probing the clock
> > > > 
> > > > H6 also has capacbility to automatically reparent RTC clock from
> > > > external crystal oscillator, to internal RC oscillator, if external
> > > > oscillator fails. This is enabled by default. Disable it during
> > > > probe.
> > > > 
> > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > > ---
> > > > 
> > > >  drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > > index d50ee023b559..b0c3752bed3f 100644
> > > > --- a/drivers/rtc/rtc-sun6i.c
> > > > +++ b/drivers/rtc/rtc-sun6i.c
> > > > @@ -32,9 +32,11 @@
> > > > 
> > > >  /* Control register */
> > > >  #define SUN6I_LOSC_CTRL				0x0000
> > > >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
> > > > 
> > > > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> > > 
> > > User manual says that above field is bit 14.
> > 
> > See the previous discussion, this is from BSP.
> 
> I have two versions of BSP (don't ask me which) which have this set as bit 14 
> and changing this to 14 actually solves all my problems with LOSC (no more 
> issues with setting RTC and HDMI-CEC works now - it uses LOSC as parent) on 
> Tanix TX6 box.

Interesting. Is LOSC fed externally generated clock, or is it setup as a crystal
oscillator on your board?

Anyway, see here:

https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.h?h=h6-4.9-bsp#n649
https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n652

It would be nice to know what's really happening.

My output is:

[    0.832252] sun6i-rtc 7000000.rtc: registered as rtc0
[    0.832257] sun6i-rtc 7000000.rtc: RTC enabled
[    1.728968] sun6i-rtc 7000000.rtc: setting system clock to 1970-01-01T00:00:07 UTC (7)

I think, you may have just enabled the auto switch feature, and running the
clock from low precision RC oscillator with your patch.

The real issue probably is that the mainline driver is missing this:

https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n650

regards,
	o.

> Best regards,
> Jernej
> 
> > 
> > regards,
> > 	o.
> > 
> > > Best regards,
> > > Jernej
> > > 
> > > >  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
> > > >  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
> > > >  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
> > > > 
> > > > +#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
> > > > 
> > > >  #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
> > > >  #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
> > > > 
> > > > @@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
> > > > 
> > > >  	unsigned int has_prescaler : 1;
> > > >  	unsigned int has_out_clk : 1;
> > > >  	unsigned int export_iosc : 1;
> > > > 
> > > > +	unsigned int has_losc_en : 1;
> > > > +	unsigned int has_auto_swt : 1;
> > > > 
> > > >  };
> > > >  
> > > >  struct sun6i_rtc_dev {
> > > > 
> > > > @@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw
> > > > *hw,
> > > > u8 index) val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
> > > > 
> > > >  	val |= SUN6I_LOSC_CTRL_KEY;
> > > >  	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> > > > 
> > > > +	if (rtc->data->has_losc_en) {
> > > > +		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > > +		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
> > > > +	}
> > > > 
> > > >  	writel(val, rtc->base + SUN6I_LOSC_CTRL);
> > > >  	spin_unlock_irqrestore(&rtc->lock, flags);
> > > > 
> > > > @@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct
> > > > device_node *node, const char *iosc_name = "rtc-int-osc";
> > > > 
> > > >  	const char *clkout_name = "osc32k-out";
> > > >  	const char *parents[2];
> > > > 
> > > > +	u32 reg;
> > > > 
> > > >  	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> > > >  	if (!rtc)
> > > > 
> > > > @@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct
> > > > device_node *node, goto err;
> > > > 
> > > >  	}
> > > > 
> > > > +	reg = SUN6I_LOSC_CTRL_KEY;
> > > > +	if (rtc->data->has_auto_swt) {
> > > > +		/* Bypass auto-switch to int osc, on ext losc failure 
> */
> > > > +		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
> > > > +		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > > +	}
> > > > +
> > > > 
> > > >  	/* Switch to the external, more precise, oscillator */
> > > > 
> > > > -	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
> > > > -	       rtc->base + SUN6I_LOSC_CTRL);
> > > > +	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;
> > > > 
> > > > @@ -345,6 +363,23 @@ CLK_OF_DECLARE_DRIVER(sun8i_h3_rtc_clk,
> > > > "allwinner,sun8i-h3-rtc", CLK_OF_DECLARE_DRIVER(sun50i_h5_rtc_clk,
> > > > "allwinner,sun50i-h5-rtc", sun8i_h3_rtc_clk_init);
> > > > 
> > > > +static const struct sun6i_rtc_clk_data sun50i_h6_rtc_data = {
> > > > +	.rc_osc_rate = 16000000,
> > > > +	.fixed_prescaler = 32,
> > > > +	.has_prescaler = 1,
> > > > +	.has_out_clk = 1,
> > > > +	.export_iosc = 1,
> > > > +	.has_losc_en = 1,
> > > > +	.has_auto_swt = 1,
> > > > +};
> > > > +
> > > > +static void __init sun50i_h6_rtc_clk_init(struct device_node *node)
> > > > +{
> > > > +	sun6i_rtc_clk_init(node, &sun50i_h6_rtc_data);
> > > > +}
> > > > +CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_clk, "allwinner,sun50i-h6-rtc",
> > > > +		      sun50i_h6_rtc_clk_init);
> > > > +
> > > > 
> > > >  static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
> > > >  
> > > >  	.rc_osc_rate = 32000,
> > > >  	.has_out_clk = 1,
> > > > 
> > > > @@ -675,6 +710,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[]
> > > > = {
> > > > 
> > > >  	{ .compatible = "allwinner,sun8i-r40-rtc" },
> > > >  	{ .compatible = "allwinner,sun8i-v3-rtc" },
> > > >  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> > > > 
> > > > +	{ .compatible = "allwinner,sun50i-h6-rtc" },
> > > > 
> > > >  	{ /* sentinel */ },
> > > >  
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel