Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock define

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/02/2025 10:49, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1
>>>> clock define
>>>>
>>>> On 24/02/2025 10:55, Ryan Chen wrote:
>>>>> -remove redundant SOC0_CLK_UART_DIV13:
>>>>> SOC0_CLK_UART_DIV13 is not use at clk-ast2700.c, the clock source
>>>>> tree is uart clk src -> uart_div_table -> uart clk.
>>>>>
>>>>> -Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX:
>>>>> modify clock tree implement.
>>>>> older CLK_AHB use mpll_div_ahb/hpll_div_ahb to be ahb clock source.
>>>>> mpll->mpll_div_ahb
>>>>>                   -> clk_ahb
>>>>> hpll->hpll_div_ahb
>>>>
>>>>
>>>> I can barely understand it and from the pieces I got, it does not
>>>> explain need for ABI break.
>>>>
>>>
>>> #1. SCU0_CLK_UART_DIV13 is redundant, it does not impact ABI break
>>
>> You did not explain how it does not impact. Clock was exported, there was a
>> user and now there is no clock. User stops working. ABI break.
>>
> 
> Sorry, SCU0_CLK_UART_DIV13 was defined, but was never referenced in any upstream device trees.


That's incomplete definition of ABI

> Since there is no in-tree usage of `SCU0_CLK_UART_DIV13`, its removal does not cause an ABI break.


You ignored out of tree users. Please read carefully ABI docs.


> 
>>> #2. Change SOC0_CLK_HPLL_DIV_AHB to SOC0_CLK_AHBMUX Older
>> implement
>>> where `mpll_div_ahb` and `hpll_div_ahb` were **hardcoded dividers** for
>> AHB.
>>> In **the new approach (v8)**, I refactored the clock tree to clock tree.
>>
>> I still cannot parse sentences like "refactoring A to A". It's meaningless to me.
>>
>>> It should be ABI-safe change
>>
>> No, you do not understand the ABI. You removed a clock ID, that's the ABI
>> change.
>>
>> Otherwise explain how this is not changing ABI.
>>
>>
>>>
>>> Or you want to keep original SOC0_CLK_HPLL_DIV_AHB define and then add
>> SOC0_CLK_AHBMUX.
>>> To be 1st patch, then 2n patch remove redundant
>> SOC0_CLK_HPLL_DIV_AHB?
>>
>> If you break the ABI you need to clearly explain why. We have long
>> conversations and you still did not say why.
>>
> Sorry, my point will be following steps to avoid potential ABI issues, 
> I can modify the patch series as follows:
> 1. **Patch 1:** Add `SOC0_CLK_AHBMUX` without removing `SOC0_CLK_HPLL_DIV_AHB`.
> 2. **Patch 2:** Finally remove `SOC0_CLK_HPLL_DIV_AHB`.


I do not understand what changed here. You remove exported clock which
is ABI, so how is this answering my question.

You keep dodging my questions. Here I asked "why". I do not see any
answer why.

Best regards,
Krzysztof