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

[Jernej Åkrabec]

Dne sobota, 24. avgust 2019 ob 23:27:46 CEST je OndÅej Jirman napisal(a):
> Hello Jernej,
> 
> On Sat, Aug 24, 2019 at 11:09:49PM +0200, Jernej Åkrabec wrote:
> > > Visually?
> > > 
> > > That would explain why it doesn't work for you. The mainline RTC driver
> > > disables auto-switch feature, and if your board doesn't have a crystal
> > > for
> > > LOSC, RTC will not generate a clock for the RTC.
> > > 
> > > H6's dtsi describes by default a situatiuon with external 32k crystal
> > > oscillator. See ext_osc32k node. That's incorrect for your board if it
> > > doesn't have the crystal. You need to fix this in the DTS for your board
> > > instead of patching the driver.
> > 
> > I see that reparenting is supported, but I'm not sure how to fix that in
> > DT. Any suggestion?
> 
> You may try removing the clocks property from rtc node.

I don't think this would work:
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L246

> 
> > > The driver has parent clock selection logic in case the LOSC crystal is
> > > not
> > > used.
> > > 
> > > Your patch enables automatic detection of LOSC failure and RTC changes
> > > clock to LOSC automatically, despite what's described in the DTS. That
> > > may fix the issue, but is not the correct solution.
> > > 
> > > Registers on my board look like this (external 32k osc is used) for
> > > reference:
> > > 
> > > LOSC_CTRL_REG[7000000]: 8011
> > > 
> > > 	KEY_FIELD                      ???                  (0)
> > > 	LOSC_AUTO_SWT_BYPASS           EN                   (1)
> > > 	LOSC_AUTO_SWT_EN               DIS                  (0)
> > > 	EXT_LOSC_EN                    EN                   (1)
> > > 	EXT_LOSC_GSM                   LOW                  (0)
> > > 	BATTERY_DIR                    DISCHARGE            (0)
> > > 	LOSC_SRC_SEL                   EXT32k               (1)
> > > 
> > > LOSC_AUTO_SWT_STA_REG[7000004]: 1
> > > 
> > > 	EXT_LOSC_STA                   OK                   (0)
> > > 	LOSC_AUTO_SWT_PEND             NOEFF                (0)
> > > 	LOSC_SRC_SEL_STA               EXT32K               (1)
> > 
> > In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
> > has value 0x4, so there is issue with external crystal (it's missing) and
> > RTC switched to internal one.
> > 
> > BTW, what's wrong with automatic switching? Why is it disabled?
> 
> It always was disabled on mainline (bit 14 was set to 0 even before my
> patch). H6 just probably has another extra undocummented bit, that's needed
> to disables it properly.
> 
> You probably don't want a glitch to switch your RTC from high-precision
> clock to a low precision one possibly without any indication in the
> userspace or a kernel log.
> 
> Regardless of all this, DTS needs to have a correct description of the HW,
> which means if RTC module is not connected to the 32.757kHz crystal/clock,
> clocks property should be empty.

If we are talking about correct HW description, then clock property should 
actually have possibility that two clocks are defined - one for internal RC 
(always present) and one external crystal (optional). In such case I could 
really just omit external clock and be done with it. But I'm not sure if such 
solution is acceptable at this point.

Best regards,
Jernej

> 
> regards,
> 	o.
> 
> > Best regards,
> > Jernej
> > 
> > > regards,
> > > 
> > > 	o.
> > > 	
> > > > > 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-b
> > > > > sp#n
> > > > > 650
> > > > 
> > > > Not sure what you mean by that. ext vs. int source selection?
> > > > 
> > > > 
> > > > 
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > > regards,
> > > > > 
> > > > > 	o.