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

[OndÅej Jirman]

On Sat, Aug 24, 2019 at 11:36:26PM +0200, Jernej Åkrabec wrote:
> 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

Well, I don't know. There has to be some way to make it work, since the code
deals with it here:

https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L270

Number of parents for LOSC is calculated from the DT somehow. Maybne something
to do with the #clock-cells property?

Sorry I can't be of more help here.

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

Internal RC is thought to be part of the RTC module, so it's not defined as an
input clock to the RTC module.

regards,
	Ondrej

> 
> 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.
> 
> 
> 
>