Re: [PATCH v13 1/2] dt-bindings: clock: add loongson-2 boot clock index

From: zhuyinbo
Date: Thu Mar 09 2023 - 21:07:23 EST



在 2023/3/10 上午12:12, Krzysztof Kozlowski 写道:
On 09/03/2023 13:44, zhuyinbo wrote:
在 2023/3/9 下午2:25, Krzysztof Kozlowski 写道:
On 09/03/2023 02:43, zhuyinbo wrote:
在 2023/3/8 下午6:38, Krzysztof Kozlowski 写道:
On 08/03/2023 10:24, zhuyinbo wrote:
That's an ABI break and commit msg does not explain it.
you meaning is that need add a explanation in commit msg that why
You need good explanation to break the ABI. I don't understand the
commit msg, but anyway I could not find there justification for ABI
break. If you do not have good justification, don't break the ABI,
The commit msg is the patch commit  log,  and I maybe not got it about
break the ABI.  You said about "break the ABI"

is whether is location issue about "LOONGSON2_BOOT_CLK"?   if yes,   the
LOONGSON2_BOOT_CLK was placed

after LOONGSON2_PIX1_PLL that is due to their clock parent is same.
and I whether need add this explanation

in patch commit log description?
Unfortunately I do not understand single thing from this.

Best regards,
Krzysztof
The patch commit log description is patch desription.  as follows:


commit 592bc2b4106d787ea166ba16bfde6b3101ab1a8a
Author: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
Date:   Tue Mar 7 17:18:32 2023 +0800

    dt-bindings: clock: add loongson-2 boot clock index

    The Loongson-2 boot clock was used to spi and lio peripheral and
    this patch was to add boot clock index number.
I cannot understand this either.
I will rework commit msg .
and your advice is "That's an ABI break and commit msg does not explain it."

I got it  from your advice that was to add a explanation about
LOONGSON2_BOOT_CLK's

location issue in patch description, right?
ABI break needs justification, why do you think it is fine or who
is/isn't affected etc. Your commit msg does not explain why ABI break is
okay. It doesn't even explain to me why you need it.
 #define LOONGSON2_DC_PLL                               3
 #define LOONGSON2_PIX0_PLL                             4
 #define LOONGSON2_PIX1_PLL                             5
-#define LOONGSON2_NODE_CLK                             6
-#define LOONGSON2_HDA_CLK                              7
-#define LOONGSON2_GPU_CLK                              8
-#define LOONGSON2_DDR_CLK                              9
-#define LOONGSON2_GMAC_CLK                             10
-#define LOONGSON2_DC_CLK                               11
-#define LOONGSON2_APB_CLK                              12
-#define LOONGSON2_USB_CLK                              13
-#define LOONGSON2_SATA_CLK                             14
-#define LOONGSON2_PIX0_CLK                             15
-#define LOONGSON2_PIX1_CLK                             16
-#define LOONGSON2_CLK_END                              17
+#define LOONGSON2_BOOT_CLK                             6
+#define LOONGSON2_NODE_CLK                             7

after add my patch, if dts still use above macro and not cause any
issue. but

if dts not use macro rather than use original clk number index that will
cause a uncorrect clk,

eg.

-#define LOONGSON2_NODE_CLK                             6

+#define LOONGSON2_NODE_CLK                             7

 this issue is that what you said about  "ABI break",  isn't it ?


About your advice and question and I will use following description as
patch  commit msg,  what do you think?


dt-bindings: clock: add loongson-2 boot clock index

The spi need to use boot clock and this patch is to add a boot clock
index about  LOONGSON2_BOOT_CLK

and the LOONGSON2_BOOT_CLK was placed in after LOONGSON2_PIX1_PLL that
due to

LOONGSON2_PIX1_PLL,  LOONGSON2_PIX0_PLL , LOONGSON2_DC_PLL and
LOONGSON2_BOOT_CLK

has same parent clock.  In addition, the Loongson  code of the community
is still in the development stage,

so this patch modification will  not cause uncorrect clk quote issue at
present.
So the reason is same parent clock...? That's not much. These are IDs
and parent clock do not matter. Drop the ID change.

okay,  I will add this marcro  LOONGSON2_BOOT_CLK in ending.


Thanks.


Best regards,
Krzysztof