rtc: sun6i: Always export the internal oscillator

  On all variants of the hardware, the internal oscillator is one possible
parent for the AR100 clock. It needs to be exported so we can model that
relationship correctly in the devicetree.

Fixes: c56afc1844d6 ("rtc: sun6i: Expose internal oscillator through device tree")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
This patch should be applied before [1] so this patch can be backported.
[1]: https://lore.kernel.org/linux-rtc/20221229184011.62925-2-samuel@sholland.org/

 drivers/rtc/rtc-sun6i.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)
  

On Thu, 29 Dec 2022 15:53:19 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> On all variants of the hardware, the internal oscillator is one possible
> parent for the AR100 clock. It needs to be exported so we can model that
> relationship correctly in the devicetree.

So do you plan to use this third clock on any SoCs that don't export it
yet, like the R40 or V3s, or the older SoCs? This would then create a
non-compatible DT change, wouldn't it? Since existing/older kernels
cannot resolve clock index 2?
Or would that not be used by kernels, or would not be fatal?

Cheers,
Andre

> Fixes: c56afc1844d6 ("rtc: sun6i: Expose internal oscillator through device tree")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> This patch should be applied before [1] so this patch can be backported.
> [1]: https://lore.kernel.org/linux-rtc/20221229184011.62925-2-samuel@sholland.org/
> 
>  drivers/rtc/rtc-sun6i.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index ed5516089e9a..7038f47d77ff 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -136,7 +136,6 @@ struct sun6i_rtc_clk_data {
>  	unsigned int fixed_prescaler : 16;
>  	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;
>  };
> @@ -271,10 +270,8 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
>  	/* Yes, I know, this is ugly. */
>  	sun6i_rtc = rtc;
>  
> -	/* Only read IOSC name from device tree if it is exported */
> -	if (rtc->data->export_iosc)
> -		of_property_read_string_index(node, "clock-output-names", 2,
> -					      &iosc_name);
> +	of_property_read_string_index(node, "clock-output-names", 2,
> +				      &iosc_name);
>  
>  	rtc->int_osc = clk_hw_register_fixed_rate_with_accuracy(NULL,
>  								iosc_name,
> @@ -315,13 +312,10 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
>  		goto err_register;
>  	}
>  
> -	clk_data->num = 2;
> +	clk_data->num = 3;
>  	clk_data->hws[0] = &rtc->hw;
>  	clk_data->hws[1] = __clk_get_hw(rtc->ext_losc);
> -	if (rtc->data->export_iosc) {
> -		clk_data->hws[2] = rtc->int_osc;
> -		clk_data->num = 3;
> -	}
> +	clk_data->hws[2] = rtc->int_osc;
>  	of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
>  	return;
>  
> @@ -361,7 +355,6 @@ static const struct sun6i_rtc_clk_data sun8i_h3_rtc_data = {
>  	.fixed_prescaler = 32,
>  	.has_prescaler = 1,
>  	.has_out_clk = 1,
> -	.export_iosc = 1,
>  };
>  
>  static void __init sun8i_h3_rtc_clk_init(struct device_node *node)
> @@ -379,7 +372,6 @@ static const struct sun6i_rtc_clk_data sun50i_h6_rtc_data = {
>  	.fixed_prescaler = 32,
>  	.has_prescaler = 1,
>  	.has_out_clk = 1,
> -	.export_iosc = 1,
>  	.has_losc_en = 1,
>  	.has_auto_swt = 1,
>  };
  

Hi Andre,

On 12/29/22 17:17, Andre Przywara wrote:
> On Thu, 29 Dec 2022 15:53:19 -0600
> Samuel Holland <samuel@sholland.org> wrote:
> 
> Hi Samuel,
> 
>> On all variants of the hardware, the internal oscillator is one possible
>> parent for the AR100 clock. It needs to be exported so we can model that
>> relationship correctly in the devicetree.
> 
> So do you plan to use this third clock on any SoCs that don't export it
> yet, like the R40 or V3s, or the older SoCs? This would then create a

I am targeting A31 and A23/A33 with this change, so I can correct those
devicetrees. Currently A31 has the wrong fourth parent for its AR100
clock, and A23/A33 models it completely wrong as a fixed-factor clock.

I have ported Crust to A23/A33, and it sets the AR100 parent to IOSC at
boot (just like on other SoCs), so it doesn't have to change the parent
during suspend/resume. But because the AR100 clock is modeled wrong in
Linux, Linux thinks the APB0 clock is running much faster than it really
is, and sets a totally wrong divider in the RSB controller, which breaks
the RSB bus.

> non-compatible DT change, wouldn't it? Since existing/older kernels
> cannot resolve clock index 2?

With the current patch contents, I expect this change to be backported
as far as 5.4. If I set ".export_iosc = 1" for A31 and A23/A33 instead
of entirely removing the flag, it could be backported further. So that
somewhat mitigates the issue.

The other option would be to add IOSC as a fixed clock in the DT, and
use that as the AR100 parent. We would still want to make this change
now, so we could eventually clean up the DT.

> Or would that not be used by kernels, or would not be fatal?

It might not be fatal if the AR100 clock isn't set to its IOSC parent. I
would have to test this.

Regards,
Samuel

> 
> Cheers,
> Andre
> 
>> Fixes: c56afc1844d6 ("rtc: sun6i: Expose internal oscillator through device tree")
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>> This patch should be applied before [1] so this patch can be backported.
>> [1]: https://lore.kernel.org/linux-rtc/20221229184011.62925-2-samuel@sholland.org/
>>
>>  drivers/rtc/rtc-sun6i.c | 16 ++++------------
>>  1 file changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
>> index ed5516089e9a..7038f47d77ff 100644
>> --- a/drivers/rtc/rtc-sun6i.c
>> +++ b/drivers/rtc/rtc-sun6i.c
>> @@ -136,7 +136,6 @@ struct sun6i_rtc_clk_data {
>>  	unsigned int fixed_prescaler : 16;
>>  	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;
>>  };
>> @@ -271,10 +270,8 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
>>  	/* Yes, I know, this is ugly. */
>>  	sun6i_rtc = rtc;
>>  
>> -	/* Only read IOSC name from device tree if it is exported */
>> -	if (rtc->data->export_iosc)
>> -		of_property_read_string_index(node, "clock-output-names", 2,
>> -					      &iosc_name);
>> +	of_property_read_string_index(node, "clock-output-names", 2,
>> +				      &iosc_name);
>>  
>>  	rtc->int_osc = clk_hw_register_fixed_rate_with_accuracy(NULL,
>>  								iosc_name,
>> @@ -315,13 +312,10 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
>>  		goto err_register;
>>  	}
>>  
>> -	clk_data->num = 2;
>> +	clk_data->num = 3;
>>  	clk_data->hws[0] = &rtc->hw;
>>  	clk_data->hws[1] = __clk_get_hw(rtc->ext_losc);
>> -	if (rtc->data->export_iosc) {
>> -		clk_data->hws[2] = rtc->int_osc;
>> -		clk_data->num = 3;
>> -	}
>> +	clk_data->hws[2] = rtc->int_osc;
>>  	of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
>>  	return;
>>  
>> @@ -361,7 +355,6 @@ static const struct sun6i_rtc_clk_data sun8i_h3_rtc_data = {
>>  	.fixed_prescaler = 32,
>>  	.has_prescaler = 1,
>>  	.has_out_clk = 1,
>> -	.export_iosc = 1,
>>  };
>>  
>>  static void __init sun8i_h3_rtc_clk_init(struct device_node *node)
>> @@ -379,7 +372,6 @@ static const struct sun6i_rtc_clk_data sun50i_h6_rtc_data = {
>>  	.fixed_prescaler = 32,
>>  	.has_prescaler = 1,
>>  	.has_out_clk = 1,
>> -	.export_iosc = 1,
>>  	.has_losc_en = 1,
>>  	.has_auto_swt = 1,
>>  };
>
  

Dne četrtek, 29. december 2022 ob 22:53:19 CET je Samuel Holland napisal(a):
> On all variants of the hardware, the internal oscillator is one possible
> parent for the AR100 clock. It needs to be exported so we can model that
> relationship correctly in the devicetree.
> 
> Fixes: c56afc1844d6 ("rtc: sun6i: Expose internal oscillator through device
> tree") Signed-off-by: Samuel Holland <samuel@sholland.org>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej
  

On Thu, 29 Dec 2022 15:53:19 -0600, Samuel Holland wrote:
> On all variants of the hardware, the internal oscillator is one possible
> parent for the AR100 clock. It needs to be exported so we can model that
> relationship correctly in the devicetree.
> 
> 

Applied, thanks!

[1/1] rtc: sun6i: Always export the internal oscillator
      commit: 344f4030f6c50a9db2d03021884c4bf36191b53a

Best regards,