> Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock
> define
>
> 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.
Apology, I can't catch your point. But I still need your guideline.
**Regarding `SCU0_CLK_UART_DIV13`:**
- This clock ID was originally defined but was never used in any in-tree device trees. (i2c-ast2700.c v1 ~ v9)
- So there should not have ABI-break am I correct?
**Regarding `SOC0_CLK_HPLL_DIV_AHB` → `SOC0_CLK_AHBMUX`:**
- The previous implementation used dividers (`mpll_div_ahb`, `hpll_div_ahb`) for AHB clock selection. (i2c-ast2700.c v1 ~ v8)
mpll->mpll_div_ahb
-> clk_ahb
hpll->hpll_div_ahb
- The new approach use `SOC0_CLK_AHBMUX`, which AHB clock sources via a mux. (i2c-ast2700.c v9)
mpll->
ahb_mux -> div_table -> clk_ahb
hpll->
- And since i2c-ast2700.c (v8) is not applied, so there should also no one use this ABI. Am I correct?
If this is not acceptable, my next patch will keep SCU0_CLK_UART_DIV13/SCU0_CLK_HPLL_DIV_AHB define.
But add new SCU0_CLK_AHBMUX define for avoid your point ABI break. Is this acceptable?
Any more clear guideline will help more clear your through. Thank your patience.
>
> Best regards,
> Krzysztof
