| Message ID | 20240217-clk-mv200-v2-1-b782e4eb66f7@outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clk: hisilicon: add support for Hi3798MV200 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > According to the datasheet, some clocks are missing, add their > definitions first. > > Some aliases for hi3798mv200 are also introduced. > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h > index e64e5770ada6..68a53053586a 100644 > --- a/include/dt-bindings/clock/histb-clock.h > +++ b/include/dt-bindings/clock/histb-clock.h > @@ -58,6 +58,27 @@ > #define HISTB_USB3_UTMI_CLK1 48 > #define HISTB_USB3_PIPE_CLK1 49 > #define HISTB_USB3_SUSPEND_CLK1 50 > +#define HISTB_SDIO1_BIU_CLK 51 > +#define HISTB_SDIO1_CIU_CLK 52 > +#define HISTB_SDIO1_DRV_CLK 53 > +#define HISTB_SDIO1_SAMPLE_CLK 54 > +#define HISTB_ETH0_PHY_CLK 55 > +#define HISTB_ETH1_PHY_CLK 56 > +#define HISTB_WDG0_CLK 57 > +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK Why? It's anyway placed oddly, the entries are ordered by number/value. > +#define HISTB_USB2_UTMI1_CLK 58 > +#define HISTB_USB3_REF_CLK 59 > +#define HISTB_USB3_GM_CLK 60 > +#define HISTB_USB3_GS_CLK 61 > + > +/* Hi3798MV200 specific clocks */ > + > +// reuse clocks of histb Don't mix comment styles. > +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK > +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK > +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK > +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK > +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK I don't understand what do you want to achieve here. Clock IDs start from 0 or 1. Best regards, Krzysztof
On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: > On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> According to the datasheet, some clocks are missing, add their >> definitions first. >> >> Some aliases for hi3798mv200 are also introduced. >> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >> index e64e5770ada6..68a53053586a 100644 >> --- a/include/dt-bindings/clock/histb-clock.h >> +++ b/include/dt-bindings/clock/histb-clock.h >> @@ -58,6 +58,27 @@ >> #define HISTB_USB3_UTMI_CLK1 48 >> #define HISTB_USB3_PIPE_CLK1 49 >> #define HISTB_USB3_SUSPEND_CLK1 50 >> +#define HISTB_SDIO1_BIU_CLK 51 >> +#define HISTB_SDIO1_CIU_CLK 52 >> +#define HISTB_SDIO1_DRV_CLK 53 >> +#define HISTB_SDIO1_SAMPLE_CLK 54 >> +#define HISTB_ETH0_PHY_CLK 55 >> +#define HISTB_ETH1_PHY_CLK 56 >> +#define HISTB_WDG0_CLK 57 >> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK > Why? It's anyway placed oddly, the entries are ordered by number/value. > >> +#define HISTB_USB2_UTMI1_CLK 58 >> +#define HISTB_USB3_REF_CLK 59 >> +#define HISTB_USB3_GM_CLK 60 >> +#define HISTB_USB3_GS_CLK 61 >> + >> +/* Hi3798MV200 specific clocks */ >> + >> +// reuse clocks of histb > Don't mix comment styles. > >> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK >> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK >> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK >> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK >> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK > I don't understand what do you want to achieve here. Clock IDs start > from 0 or 1. They are aliases. A friendlier name compared to ETH0/1. > > > > Best regards, > Krzysztof >
On 20/02/2024 11:12, Yang Xiwen wrote: > On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> According to the datasheet, some clocks are missing, add their >>> definitions first. >>> >>> Some aliases for hi3798mv200 are also introduced. >>> >>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>> --- >>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>> index e64e5770ada6..68a53053586a 100644 >>> --- a/include/dt-bindings/clock/histb-clock.h >>> +++ b/include/dt-bindings/clock/histb-clock.h >>> @@ -58,6 +58,27 @@ >>> #define HISTB_USB3_UTMI_CLK1 48 >>> #define HISTB_USB3_PIPE_CLK1 49 >>> #define HISTB_USB3_SUSPEND_CLK1 50 >>> +#define HISTB_SDIO1_BIU_CLK 51 >>> +#define HISTB_SDIO1_CIU_CLK 52 >>> +#define HISTB_SDIO1_DRV_CLK 53 >>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>> +#define HISTB_ETH0_PHY_CLK 55 >>> +#define HISTB_ETH1_PHY_CLK 56 >>> +#define HISTB_WDG0_CLK 57 >>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >> Why? It's anyway placed oddly, the entries are ordered by number/value. >> >>> +#define HISTB_USB2_UTMI1_CLK 58 >>> +#define HISTB_USB3_REF_CLK 59 >>> +#define HISTB_USB3_GM_CLK 60 >>> +#define HISTB_USB3_GS_CLK 61 >>> + >>> +/* Hi3798MV200 specific clocks */ >>> + >>> +// reuse clocks of histb >> Don't mix comment styles. >> >>> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK >>> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK >>> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK >>> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK >>> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK >> I don't understand what do you want to achieve here. Clock IDs start >> from 0 or 1. > They are aliases. A friendlier name compared to ETH0/1. Fix your email client, so it will not remove line breaks before/after quotes. Your email client makes it unreadable. Aliases do not bind anything, so you can drop these. Best regards, Krzysztof
On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: > On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> According to the datasheet, some clocks are missing, add their >> definitions first. >> >> Some aliases for hi3798mv200 are also introduced. >> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >> index e64e5770ada6..68a53053586a 100644 >> --- a/include/dt-bindings/clock/histb-clock.h >> +++ b/include/dt-bindings/clock/histb-clock.h >> @@ -58,6 +58,27 @@ >> #define HISTB_USB3_UTMI_CLK1 48 >> #define HISTB_USB3_PIPE_CLK1 49 >> #define HISTB_USB3_SUSPEND_CLK1 50 >> +#define HISTB_SDIO1_BIU_CLK 51 >> +#define HISTB_SDIO1_CIU_CLK 52 >> +#define HISTB_SDIO1_DRV_CLK 53 >> +#define HISTB_SDIO1_SAMPLE_CLK 54 >> +#define HISTB_ETH0_PHY_CLK 55 >> +#define HISTB_ETH1_PHY_CLK 56 >> +#define HISTB_WDG0_CLK 57 >> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK > Why? It's anyway placed oddly, the entries are ordered by number/value. So this is somewhat broken at the beginning. It named after histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK after it and increment all the indexes after it? Then the diff would be very ugly. > >> +#define HISTB_USB2_UTMI1_CLK 58 >> +#define HISTB_USB3_REF_CLK 59 >> +#define HISTB_USB3_GM_CLK 60 >> +#define HISTB_USB3_GS_CLK 61 >> + >> +/* Hi3798MV200 specific clocks */ >> + >> +// reuse clocks of histb > Don't mix comment styles. > >> +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK >> +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK >> +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK >> +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK >> +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK > I don't understand what do you want to achieve here. Clock IDs start > from 0 or 1. > > > > Best regards, > Krzysztof >
On 20/02/2024 15:06, Yang Xiwen wrote: > On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> According to the datasheet, some clocks are missing, add their >>> definitions first. >>> >>> Some aliases for hi3798mv200 are also introduced. >>> >>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>> --- >>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>> index e64e5770ada6..68a53053586a 100644 >>> --- a/include/dt-bindings/clock/histb-clock.h >>> +++ b/include/dt-bindings/clock/histb-clock.h >>> @@ -58,6 +58,27 @@ >>> #define HISTB_USB3_UTMI_CLK1 48 >>> #define HISTB_USB3_PIPE_CLK1 49 >>> #define HISTB_USB3_SUSPEND_CLK1 50 >>> +#define HISTB_SDIO1_BIU_CLK 51 >>> +#define HISTB_SDIO1_CIU_CLK 52 >>> +#define HISTB_SDIO1_DRV_CLK 53 >>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>> +#define HISTB_ETH0_PHY_CLK 55 >>> +#define HISTB_ETH1_PHY_CLK 56 >>> +#define HISTB_WDG0_CLK 57 >>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >> Why? It's anyway placed oddly, the entries are ordered by number/value. > > > So this is somewhat broken at the beginning. It named after > histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For > Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. > > > What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK > after it and increment all the indexes after it? Then the diff would be > very ugly. I still don't understand what is the problem you are trying to solve here. Your commit msg says add missing ID, but that ID - HISTB_USB2_UTMI_CLK - is already there. I also do not get why there is a need to rename anything. Best regards, Krzysztof
On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote: > On 20/02/2024 15:06, Yang Xiwen wrote: >> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>>> From: Yang Xiwen <forbidden405@outlook.com> >>>> >>>> According to the datasheet, some clocks are missing, add their >>>> definitions first. >>>> >>>> Some aliases for hi3798mv200 are also introduced. >>>> >>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>> --- >>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>>> >>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>>> index e64e5770ada6..68a53053586a 100644 >>>> --- a/include/dt-bindings/clock/histb-clock.h >>>> +++ b/include/dt-bindings/clock/histb-clock.h >>>> @@ -58,6 +58,27 @@ >>>> #define HISTB_USB3_UTMI_CLK1 48 >>>> #define HISTB_USB3_PIPE_CLK1 49 >>>> #define HISTB_USB3_SUSPEND_CLK1 50 >>>> +#define HISTB_SDIO1_BIU_CLK 51 >>>> +#define HISTB_SDIO1_CIU_CLK 52 >>>> +#define HISTB_SDIO1_DRV_CLK 53 >>>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>>> +#define HISTB_ETH0_PHY_CLK 55 >>>> +#define HISTB_ETH1_PHY_CLK 56 >>>> +#define HISTB_WDG0_CLK 57 >>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >>> Why? It's anyway placed oddly, the entries are ordered by number/value. >> >> So this is somewhat broken at the beginning. It named after >> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For >> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. >> >> >> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK >> after it and increment all the indexes after it? Then the diff would be >> very ugly. > I still don't understand what is the problem you are trying to solve > here. Your commit msg says add missing ID, but that ID - > HISTB_USB2_UTMI_CLK - is already there. > > I also do not get why there is a need to rename anything. Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200. UTMI1 is missing here. For other HiSTB SoCs, there could be even more. If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without renaming it to UTMI0. Just like all the other clocks. E.g. I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK. > > > > Best regards, > Krzysztof >
On 20/02/2024 17:19, Yang Xiwen wrote: > On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote: >> On 20/02/2024 15:06, Yang Xiwen wrote: >>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>>>> From: Yang Xiwen <forbidden405@outlook.com> >>>>> >>>>> According to the datasheet, some clocks are missing, add their >>>>> definitions first. >>>>> >>>>> Some aliases for hi3798mv200 are also introduced. >>>>> >>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>>> --- >>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>>>> 1 file changed, 21 insertions(+) >>>>> >>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>>>> index e64e5770ada6..68a53053586a 100644 >>>>> --- a/include/dt-bindings/clock/histb-clock.h >>>>> +++ b/include/dt-bindings/clock/histb-clock.h >>>>> @@ -58,6 +58,27 @@ >>>>> #define HISTB_USB3_UTMI_CLK1 48 >>>>> #define HISTB_USB3_PIPE_CLK1 49 >>>>> #define HISTB_USB3_SUSPEND_CLK1 50 >>>>> +#define HISTB_SDIO1_BIU_CLK 51 >>>>> +#define HISTB_SDIO1_CIU_CLK 52 >>>>> +#define HISTB_SDIO1_DRV_CLK 53 >>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>>>> +#define HISTB_ETH0_PHY_CLK 55 >>>>> +#define HISTB_ETH1_PHY_CLK 56 >>>>> +#define HISTB_WDG0_CLK 57 >>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >>>> Why? It's anyway placed oddly, the entries are ordered by number/value. >>> >>> So this is somewhat broken at the beginning. It named after >>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For >>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. >>> >>> >>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK >>> after it and increment all the indexes after it? Then the diff would be >>> very ugly. >> I still don't understand what is the problem you are trying to solve >> here. Your commit msg says add missing ID, but that ID - >> HISTB_USB2_UTMI_CLK - is already there. >> >> I also do not get why there is a need to rename anything. > > > Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200. > UTMI1 is missing here. For other HiSTB SoCs, there could be even more. My comment was under UTMI0. We do not talk about UTMI1... > > > If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without > renaming it to UTMI0. Just like all the other clocks. E.g. > I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK. Then place it next to old name and explain why it is deprecated with comment. Best regards, Krzysztof
On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote: > On 20/02/2024 17:19, Yang Xiwen wrote: >> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote: >>> On 20/02/2024 15:06, Yang Xiwen wrote: >>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>>>>> From: Yang Xiwen <forbidden405@outlook.com> >>>>>> >>>>>> According to the datasheet, some clocks are missing, add their >>>>>> definitions first. >>>>>> >>>>>> Some aliases for hi3798mv200 are also introduced. >>>>>> >>>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>>>> --- >>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>>>>> 1 file changed, 21 insertions(+) >>>>>> >>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>>>>> index e64e5770ada6..68a53053586a 100644 >>>>>> --- a/include/dt-bindings/clock/histb-clock.h >>>>>> +++ b/include/dt-bindings/clock/histb-clock.h >>>>>> @@ -58,6 +58,27 @@ >>>>>> #define HISTB_USB3_UTMI_CLK1 48 >>>>>> #define HISTB_USB3_PIPE_CLK1 49 >>>>>> #define HISTB_USB3_SUSPEND_CLK1 50 >>>>>> +#define HISTB_SDIO1_BIU_CLK 51 >>>>>> +#define HISTB_SDIO1_CIU_CLK 52 >>>>>> +#define HISTB_SDIO1_DRV_CLK 53 >>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>>>>> +#define HISTB_ETH0_PHY_CLK 55 >>>>>> +#define HISTB_ETH1_PHY_CLK 56 >>>>>> +#define HISTB_WDG0_CLK 57 >>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >>>>> Why? It's anyway placed oddly, the entries are ordered by number/value. >>>> So this is somewhat broken at the beginning. It named after >>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For >>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. >>>> >>>> >>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK >>>> after it and increment all the indexes after it? Then the diff would be >>>> very ugly. >>> I still don't understand what is the problem you are trying to solve >>> here. Your commit msg says add missing ID, but that ID - >>> HISTB_USB2_UTMI_CLK - is already there. >>> >>> I also do not get why there is a need to rename anything. >> >> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200. >> UTMI1 is missing here. For other HiSTB SoCs, there could be even more. > My comment was under UTMI0. We do not talk about UTMI1... > >> >> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without >> renaming it to UTMI0. Just like all the other clocks. E.g. >> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK. > Then place it next to old name and explain why it is deprecated with > comment. Do we need to keep the old name? I can fix all the users (only hi3798cv200.dtsi) in next version and drop this name directly. Is that okay? Should i insert UTMI1_CLK to the middle and re-index all the macros after it? Or simply add it to the tail? > > Best regards, > Krzysztof >
On 20/02/2024 17:31, Yang Xiwen wrote: > On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote: >> On 20/02/2024 17:19, Yang Xiwen wrote: >>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote: >>>> On 20/02/2024 15:06, Yang Xiwen wrote: >>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>>>>>> From: Yang Xiwen <forbidden405@outlook.com> >>>>>>> >>>>>>> According to the datasheet, some clocks are missing, add their >>>>>>> definitions first. >>>>>>> >>>>>>> Some aliases for hi3798mv200 are also introduced. >>>>>>> >>>>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>>>>> --- >>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>>>>>> 1 file changed, 21 insertions(+) >>>>>>> >>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>>>>>> index e64e5770ada6..68a53053586a 100644 >>>>>>> --- a/include/dt-bindings/clock/histb-clock.h >>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h >>>>>>> @@ -58,6 +58,27 @@ >>>>>>> #define HISTB_USB3_UTMI_CLK1 48 >>>>>>> #define HISTB_USB3_PIPE_CLK1 49 >>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50 >>>>>>> +#define HISTB_SDIO1_BIU_CLK 51 >>>>>>> +#define HISTB_SDIO1_CIU_CLK 52 >>>>>>> +#define HISTB_SDIO1_DRV_CLK 53 >>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>>>>>> +#define HISTB_ETH0_PHY_CLK 55 >>>>>>> +#define HISTB_ETH1_PHY_CLK 56 >>>>>>> +#define HISTB_WDG0_CLK 57 >>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value. >>>>> So this is somewhat broken at the beginning. It named after >>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For >>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. >>>>> >>>>> >>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK >>>>> after it and increment all the indexes after it? Then the diff would be >>>>> very ugly. >>>> I still don't understand what is the problem you are trying to solve >>>> here. Your commit msg says add missing ID, but that ID - >>>> HISTB_USB2_UTMI_CLK - is already there. >>>> >>>> I also do not get why there is a need to rename anything. >>> >>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200. >>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more. >> My comment was under UTMI0. We do not talk about UTMI1... >> >>> >>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without >>> renaming it to UTMI0. Just like all the other clocks. E.g. >>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK. >> Then place it next to old name and explain why it is deprecated with >> comment. > > > Do we need to keep the old name? I can fix all the users (only > hi3798cv200.dtsi) in next version and drop this name directly. Is that All users in all projects? That might be tricky. And even for Linux kernel, how can you do it in a bisectable way? Just keep old name. > okay? Should i insert UTMI1_CLK to the middle and re-index all the > macros after it? Or simply add it to the tail? Bindings and header constants are ABI, so you cannot change them. Best regards, Krzysztof
On 2/21/2024 1:06 AM, Krzysztof Kozlowski wrote: > On 20/02/2024 17:31, Yang Xiwen wrote: >> On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote: >>> On 20/02/2024 17:19, Yang Xiwen wrote: >>>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote: >>>>> On 20/02/2024 15:06, Yang Xiwen wrote: >>>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >>>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>>>>>>> From: Yang Xiwen <forbidden405@outlook.com> >>>>>>>> >>>>>>>> According to the datasheet, some clocks are missing, add their >>>>>>>> definitions first. >>>>>>>> >>>>>>>> Some aliases for hi3798mv200 are also introduced. >>>>>>>> >>>>>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>>>>>> --- >>>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>>>>>>> 1 file changed, 21 insertions(+) >>>>>>>> >>>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>>>>>>> index e64e5770ada6..68a53053586a 100644 >>>>>>>> --- a/include/dt-bindings/clock/histb-clock.h >>>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h >>>>>>>> @@ -58,6 +58,27 @@ >>>>>>>> #define HISTB_USB3_UTMI_CLK1 48 >>>>>>>> #define HISTB_USB3_PIPE_CLK1 49 >>>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50 >>>>>>>> +#define HISTB_SDIO1_BIU_CLK 51 >>>>>>>> +#define HISTB_SDIO1_CIU_CLK 52 >>>>>>>> +#define HISTB_SDIO1_DRV_CLK 53 >>>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>>>>>>> +#define HISTB_ETH0_PHY_CLK 55 >>>>>>>> +#define HISTB_ETH1_PHY_CLK 56 >>>>>>>> +#define HISTB_WDG0_CLK 57 >>>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >>>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value. >>>>>> So this is somewhat broken at the beginning. It named after >>>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For >>>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. >>>>>> >>>>>> >>>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK >>>>>> after it and increment all the indexes after it? Then the diff would be >>>>>> very ugly. >>>>> I still don't understand what is the problem you are trying to solve >>>>> here. Your commit msg says add missing ID, but that ID - >>>>> HISTB_USB2_UTMI_CLK - is already there. >>>>> >>>>> I also do not get why there is a need to rename anything. >>>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200. >>>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more. >>> My comment was under UTMI0. We do not talk about UTMI1... >>> >>>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without >>>> renaming it to UTMI0. Just like all the other clocks. E.g. >>>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK. >>> Then place it next to old name and explain why it is deprecated with >>> comment. >> >> Do we need to keep the old name? I can fix all the users (only >> hi3798cv200.dtsi) in next version and drop this name directly. Is that > All users in all projects? That might be tricky. And even for Linux > kernel, how can you do it in a bisectable way? Just keep old name. > > >> okay? Should i insert UTMI1_CLK to the middle and re-index all the >> macros after it? Or simply add it to the tail? > Bindings and header constants are ABI, so you cannot change them. This file should be renamed to hi3798cv200-clock.h, it shouldn't be called histb-clock.h from the beginning. Now I have to workaround this in a dirty way. What if another HiSTB SoC has 3 or more UTMI_CLKs? Do we need to add more definitions to the end of the file? The file is gonna to be more and more unreadable with scattered clock definitions. Do you think it's acceptable to create a new header file instead? I think we don't need a generic histb-clock.h. Each SoC should maintain their own clock indexes header file. Maybe we can rename it to hi3798cv200-clock.h and include it from a new histb-clock.h (also mark this generic header file deprecated and only for hi3798cv200). Then I'll create hi3798mv200-clock.h header file instead. So we don't have to workaround this. > > Best regards, > Krzysztof >
On 20/02/2024 18:29, Yang Xiwen wrote: > On 2/21/2024 1:06 AM, Krzysztof Kozlowski wrote: >> On 20/02/2024 17:31, Yang Xiwen wrote: >>> On 2/21/2024 12:25 AM, Krzysztof Kozlowski wrote: >>>> On 20/02/2024 17:19, Yang Xiwen wrote: >>>>> On 2/21/2024 12:13 AM, Krzysztof Kozlowski wrote: >>>>>> On 20/02/2024 15:06, Yang Xiwen wrote: >>>>>>> On 2/20/2024 6:10 PM, Krzysztof Kozlowski wrote: >>>>>>>> On 17/02/2024 13:52, Yang Xiwen via B4 Relay wrote: >>>>>>>>> From: Yang Xiwen <forbidden405@outlook.com> >>>>>>>>> >>>>>>>>> According to the datasheet, some clocks are missing, add their >>>>>>>>> definitions first. >>>>>>>>> >>>>>>>>> Some aliases for hi3798mv200 are also introduced. >>>>>>>>> >>>>>>>>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>>>>>>>> --- >>>>>>>>> include/dt-bindings/clock/histb-clock.h | 21 +++++++++++++++++++++ >>>>>>>>> 1 file changed, 21 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h >>>>>>>>> index e64e5770ada6..68a53053586a 100644 >>>>>>>>> --- a/include/dt-bindings/clock/histb-clock.h >>>>>>>>> +++ b/include/dt-bindings/clock/histb-clock.h >>>>>>>>> @@ -58,6 +58,27 @@ >>>>>>>>> #define HISTB_USB3_UTMI_CLK1 48 >>>>>>>>> #define HISTB_USB3_PIPE_CLK1 49 >>>>>>>>> #define HISTB_USB3_SUSPEND_CLK1 50 >>>>>>>>> +#define HISTB_SDIO1_BIU_CLK 51 >>>>>>>>> +#define HISTB_SDIO1_CIU_CLK 52 >>>>>>>>> +#define HISTB_SDIO1_DRV_CLK 53 >>>>>>>>> +#define HISTB_SDIO1_SAMPLE_CLK 54 >>>>>>>>> +#define HISTB_ETH0_PHY_CLK 55 >>>>>>>>> +#define HISTB_ETH1_PHY_CLK 56 >>>>>>>>> +#define HISTB_WDG0_CLK 57 >>>>>>>>> +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK >>>>>>>> Why? It's anyway placed oddly, the entries are ordered by number/value. >>>>>>> So this is somewhat broken at the beginning. It named after >>>>>>> histb-clock.h but actually they are all clocks for Hi3798CV200 SoC. For >>>>>>> Hi3798MV200(also a HiSTB SoC), there is one additional UTMI clock. >>>>>>> >>>>>>> >>>>>>> What solution do you prefer? rename UTMI_CLK to UTMI0_CLK, add UTMI1_CLK >>>>>>> after it and increment all the indexes after it? Then the diff would be >>>>>>> very ugly. >>>>>> I still don't understand what is the problem you are trying to solve >>>>>> here. Your commit msg says add missing ID, but that ID - >>>>>> HISTB_USB2_UTMI_CLK - is already there. >>>>>> >>>>>> I also do not get why there is a need to rename anything. >>>>> Because there are two USB2_UTMI_CLKs in total, at least for Hi3798MV200. >>>>> UTMI1 is missing here. For other HiSTB SoCs, there could be even more. >>>> My comment was under UTMI0. We do not talk about UTMI1... >>>> >>>>> If we add USB2_UTMI1_CLK, it looks silly to keep USB2_UTMI_CLK without >>>>> renaming it to UTMI0. Just like all the other clocks. E.g. >>>>> I2Cn_CLK(n=0,1,2,3,4) etc.., so the same for USB2_UTMI_CLK. >>>> Then place it next to old name and explain why it is deprecated with >>>> comment. >>> >>> Do we need to keep the old name? I can fix all the users (only >>> hi3798cv200.dtsi) in next version and drop this name directly. Is that >> All users in all projects? That might be tricky. And even for Linux >> kernel, how can you do it in a bisectable way? Just keep old name. >> >> >>> okay? Should i insert UTMI1_CLK to the middle and re-index all the >>> macros after it? Or simply add it to the tail? >> Bindings and header constants are ABI, so you cannot change them. > > > This file should be renamed to hi3798cv200-clock.h, it shouldn't be > called histb-clock.h from the beginning. Now I have to workaround this > in a dirty way. What if another HiSTB SoC has 3 or more UTMI_CLKs? Do we > need to add more definitions to the end of the file? The file is gonna That's not a big problem, but indeed shows poor design of the driver and bindings. > to be more and more unreadable with scattered clock definitions. > > > Do you think it's acceptable to create a new header file instead? I This depends on the purpose of it. In general every SoC follows that concept - binding headers are per given clock controller, not even per SoC. > think we don't need a generic histb-clock.h. Each SoC should maintain > their own clock indexes header file. Maybe we can rename it to > hi3798cv200-clock.h and include it from a new histb-clock.h (also mark > this generic header file deprecated and only for hi3798cv200). Then I'll > create hi3798mv200-clock.h header file instead. So we don't have to > workaround this. Best regards, Krzysztof
diff --git a/include/dt-bindings/clock/histb-clock.h b/include/dt-bindings/clock/histb-clock.h index e64e5770ada6..68a53053586a 100644 --- a/include/dt-bindings/clock/histb-clock.h +++ b/include/dt-bindings/clock/histb-clock.h @@ -58,6 +58,27 @@ #define HISTB_USB3_UTMI_CLK1 48 #define HISTB_USB3_PIPE_CLK1 49 #define HISTB_USB3_SUSPEND_CLK1 50 +#define HISTB_SDIO1_BIU_CLK 51 +#define HISTB_SDIO1_CIU_CLK 52 +#define HISTB_SDIO1_DRV_CLK 53 +#define HISTB_SDIO1_SAMPLE_CLK 54 +#define HISTB_ETH0_PHY_CLK 55 +#define HISTB_ETH1_PHY_CLK 56 +#define HISTB_WDG0_CLK 57 +#define HISTB_USB2_UTMI0_CLK HISTB_USB2_UTMI_CLK +#define HISTB_USB2_UTMI1_CLK 58 +#define HISTB_USB3_REF_CLK 59 +#define HISTB_USB3_GM_CLK 60 +#define HISTB_USB3_GS_CLK 61 + +/* Hi3798MV200 specific clocks */ + +// reuse clocks of histb +#define HI3798MV200_GMAC_CLK HISTB_ETH0_MAC_CLK +#define HI3798MV200_GMACIF_CLK HISTB_ETH0_MACIF_CLK +#define HI3798MV200_FEMAC_CLK HISTB_ETH1_MAC_CLK +#define HI3798MV200_FEMACIF_CLK HISTB_ETH1_MACIF_CLK +#define HI3798MV200_FEPHY_CLK HISTB_ETH1_PHY_CLK /* clocks provided by mcu CRG */ #define HISTB_MCE_CLK 1