Re: [PATCH v2 1/1] drivers/rtc: rtc-sun6i: AutoCal Internal OSC Clock

[Alois Fertl]

On Sun, 2024-04-21 at 21:57 +0200, Jernej Škrabec wrote:
> Dne nedelja, 21. april 2024 ob 20:36:33 GMT +2 je Alois Fertl
> napisal(a):
> > 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.
> > 
> > v1->v2
> > - add flag and activate for H6 AND H616
> 
> Please move changelog below --- line.
Thanks for reviewing, will move the changelog
> 
> > 
> > Signed-off-by: Alois Fertl <a.fertl@xxxxxxxxxxx>
> > ---
> >  drivers/rtc/rtc-sun6i.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index e0b85a0d5645..5d0c917b2099 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_EANABLE                BIT(1)
> 
> EANABLE -> ENABLE
yes sure
> 
> > +#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,13 @@ 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_EANABLE |
> > +                               SUN6I_LOSC_CLK_AUTO_CAL_SEL_CAL);
> 
> Remove parenthesis and fix indentation. Since macro names are pretty
> long,
> maybe put one per line.
will do so
> 
> Is this safe to be done even on the boards with external 32k crystal?
Can't tell for sure. I don't have a H6 board. But I would think that
it's even required because H6 has a feature to autoswitch clock source
if the external is detected to have failed.
> 
> Best regards,
> Jernej
Thanks and regards,
Alois
> 
> > +               writel(reg, rtc->base + SUN6I_LOSC_CLK_AUTO_CAL);
> > +       }
> > +
> >         /* Yes, I know, this is ugly. */
> >         sun6i_rtc = rtc;
> >  
> > @@ -380,6 +392,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)
> > @@ -395,6 +408,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)
> > 
> 
> 
> 
>