Received-SPF: pass (google.com: domain of linux-kernel+bounces-195711-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 30 May 2024 18:10:22 +0100
From: Andre Przywara <andre.przywara@arm.com>
To: Alois Fertl <a.fertl@t-online.de>
Cc: a.zummo@towertech.it, alexandre.belloni@bootlin.com, wens@csie.org,
 jernej.skrabec@gmail.com, samuel@sholland.org, linux-rtc@vger.kernel.org,
 linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/1] drivers/rtc: rtc-sun6i: AutoCal Internal OSC
Message-ID: <20240530181022.6fbc5a7d@donnerap.manchester.arm.com>
In-Reply-To: <20240522182826.6824-1-a.fertl@t-online.de>
References: <20240522182826.6824-1-a.fertl@t-online.de>
Organization: ARM
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 22 May 2024 20:28:26 +0200
Alois Fertl <a.fertl@t-online.de> 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.
> Also the code is developed on the H618 board it only modifies the H6 as
> there is no support for H616/H618 in the current code.

I am a bit confused: so this patch doesn't fix your problem then, because
the code you touch is not used on the H616/H618?
Actually I would have expected your patch to only change
drivers/clk/sunxi-ng/ccu-sun6i-rtc.c, since that's the only RTC clock
driver relevant for the H616.

> Signed-off-by: Alois Fertl <a.fertl@t-online.de>
> ---
> 
> v1->v2
> - add flag and activate for H6 AND H616
> 
> v2->v3
> - correct findings from review
> 
> v3->v4
> - adjust to mainline tree
> 
> I have also tried to test this using the new driver in sunxi-ng
> manually injecting the reverted patch
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=60d9f050da63b

So this was done on a H6 device? Because out of the box rtc-sun6i.c is
used on the H6 only, and ccu-sun6i-rtc.c is only used on the H616.

Maybe I am missing something here ...

> The code in drivers/clk/sunxi-ng/ccu-sun6i-rtc.c is being called and it
> initializes the relevant registers to the same values as the old driver,
> but the change ends up with a system that often hangs during booting and
> only ocasionally reaches the login state (one out of 10).
> The main difference I see adhoc is that the old drivers init is done
> using CLK_OF_DECLARE_DRIVER so initialization is done very early.
> The new driver does the initialisation via probe which is quite some
> time later.
> Can't tell if this is the cause for the problems.

That sounds odd, can you post your changes somewhere?

Generally, without a proper problem and without further testing, I would
not like to touch the H6 RTC code needlessly.
For the H616 we have a concrete problem at hand, that justifies a change,
also it's the proper driver for new devices, so that's where the change
should happen.

Cheers,
Andre

> 
> ---
>  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 8e0c66906..57aa52d3b 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.
>   */
> @@ -137,6 +141,7 @@ struct sun6i_rtc_clk_data {
>  	unsigned int has_out_clk : 1;
>  	unsigned int has_losc_en : 1;
>  	unsigned int has_auto_swt : 1;
> +	unsigned int has_auto_cal : 1;
>  };
>  
>  #define RTC_LINEAR_DAY	BIT(0)
> @@ -267,6 +272,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;
>  
> @@ -374,6 +387,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)