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

[Ryan Chen <ryan_chen@aspeedtech.com>]

> Subject: Re: [PATCH v9 1/3] dt-binding: clock: ast2700: modify soc0/1 clock
> define
> 
> On 26/02/2025 06:10, Ryan Chen wrote:
> >> 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?
> 
> 
> No, because there are other users of bindings. All forks, out of tree DTS, other
> projects etc. You defined ABI for them.
> 
> >
> > **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->
> 
> Your formatting is one of the problems I have troubles understanding it.
> Above is not wrapped or wrapped oddly. You keep using bold * but double **,
> which is not a standard markup. Please switch to standard mailing list
> formatting - proper wrapping, only text mode and use client which actually can
> parse and create that.
> 
> What I don't understand is how clocks could change in hardware. You described
> implementation, but the clock IDS do not describe implementation but the the
> mapping to real hardware clocks. So how SOC0_CLK_HPLL_DIV_AHB clock
> disappeared from hardware?

Sorry, It's not disappeared, it is replaced by implement.

- The previous implementation hardcoded `mpll_div_ahb` and `hpll_div_ahb` as the only AHB clock sources:
     mpll -> mpll_div_ahb 
						-> clk_ahb
     hpll -> hpll_div_ahb> 

- The new approach introduces `SOC0_CLK_AHBMUX`, clear clock tree selection of AHB clock sources via a mux:
 Than through div_table to get ahb clk.
     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 binding was not applied, then what are you changing here?

My point is only i2c-ast2700.c (v8) used, which not be applied in Linux. 
And i2c-ast2700.c (v9) no need anymore.

> 
> Does it mean header described clock which was in the hardware but its
> handling was not yet implemented in the driver?
> 
> 
> >
> > If this is not acceptable, my next patch will keep
> SCU0_CLK_UART_DIV13/SCU0_CLK_HPLL_DIV_AHB define.
> 
> Maybe my last message was not clear, so: you need to explain why you are
> breaking ABI and/or explain the ABI impact in the commit msg.
Understood your concern about this.
I think add new SCU0_CLK_AHBMUX to avoid ABI impact will be the better way.

Thank your guideline. 
> 
> > But add new SCU0_CLK_AHBMUX define for avoid your point ABI break. Is
> this acceptable?
> 
> 
> Best regards,
> Krzysztof