Hi Andre,
On 12/29/22 17:17, Andre Przywara wrote:
> On Thu, 29 Dec 2022 15:53:19 -0600
> Samuel Holland <samuel@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> This patch should be applied before [1] so this patch can be backported.
>> [1]: https://lore.kernel.org/linux-rtc/20221229184011.62925-2-samuel@xxxxxxxxxxxx/
>>
>> 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,
>> };
>
