| Message ID | 20221123021346.18136-1-yu.tu@amlogic.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add S4 SoC PLL and Peripheral clock controller | expand |
On 23/11/2022 03:13, Yu Tu wrote: > Add the peripherals clock controller found and bindings in the s4 SoC family. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + No, this is total mess now. Additionally, you received a lot of feedback but your changelog says only: "V3 -> V4: change format and clock flags." so you ignored entire feedback? That's not the way to work with patches. Best regards, Krzysztof
On 2022/11/23 18:09, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 03:13, Yu Tu wrote: >> Add the peripherals clock controller found and bindings in the s4 SoC family. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + > > No, this is total mess now. > > Additionally, you received a lot of feedback but your changelog says only: > "V3 -> V4: change format and clock flags." > so you ignored entire feedback? > > That's not the way to work with patches. Hi Krzysztof, You can check the previous email reply. Now I don't know who to follow your advice or Jerome's. I'm confused. Maybe you need to come to a conclusion. So I can change it in the next patch. > > Best regards, > Krzysztof > > .
On 23/11/2022 12:22, Yu Tu wrote: > > > On 2022/11/23 18:09, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 23/11/2022 03:13, Yu Tu wrote: >>> Add the peripherals clock controller found and bindings in the s4 SoC family. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + >> >> No, this is total mess now. >> >> Additionally, you received a lot of feedback but your changelog says only: >> "V3 -> V4: change format and clock flags." >> so you ignored entire feedback? >> >> That's not the way to work with patches. > > Hi Krzysztof, > You can check the previous email reply. Now I don't know who to follow > your advice or Jerome's. I'm confused. Maybe you need to come to a > conclusion. So I can change it in the next patch. I don't understand your comment. You received a lot of things to change for your v3. You said here "change format and clock flagS", so all other feedbacks from me were ignored? They were not contradicting to Jerome's comments, so either you implement them and mention this in changelog, or you keep discussing. Best regards, Krzysztof
Hi Krzysztof, On 2022/11/23 21:06, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 12:22, Yu Tu wrote: >> >> >> On 2022/11/23 18:09, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Add the peripherals clock controller found and bindings in the s4 SoC family. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + >>> >>> No, this is total mess now. >>> >>> Additionally, you received a lot of feedback but your changelog says only: >>> "V3 -> V4: change format and clock flags." >>> so you ignored entire feedback? >>> >>> That's not the way to work with patches. >> >> Hi Krzysztof, >> You can check the previous email reply. Now I don't know who to follow >> your advice or Jerome's. I'm confused. Maybe you need to come to a >> conclusion. So I can change it in the next patch. > > I don't understand your comment. You received a lot of things to change > for your v3. You said here "change format and clock flagS", so all other > feedbacks from me were ignored? They were not contradicting to Jerome's > comments, so either you implement them and mention this in changelog, or > you keep discussing. > I'm sorry I didn't write that clearly. The reality is that V4 I have changed some of the suggestions from Jerome. But there was a part that didn't agree before I sent V4, which Jerome chose to skip. Let's continue with V3, and then prepare this V5. > Best regards, > Krzysztof > > .
On Mon 28 Nov 2022 at 16:08, Yu Tu <yu.tu@amlogic.com> wrote: >>> + >>> +/* >>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator. >>> + * If it is used, it should be documented in using fw_name and documented in the >>> + * Bindings. Not currently in use on this board, so skip it. >>> + */ >>> +static u32 rtc_clk_sel[] = { 0, 1 }; >> No reason to do that > > I'm going to change it to static u32 rtc_clk_sel[] = { 0, 1, 2 };. > I don't know if that's okay with you? ... then there is no need to specify this table. > >> >>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = { >>> + { .hw = &s4_rtc_32k_by_oscin.hw }, >>> + { .hw = &s4_rtc_32k_by_oscin_div.hw }, >>> + { .fw_name = "ext_32k", } >>> +}; >>> + >>> +static struct clk_regmap s4_rtc_clk = { >>> + .data = &(struct clk_regmap_mux_data) { >>> + .offset = CLKCTRL_RTC_CTRL, >>> + .mask = 0x3, >>> + .shift = 0, >>> + .table = rtc_clk_sel, >>> + .flags = CLK_MUX_ROUND_CLOSEST, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "rtc_clk_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = rtc_clk_sel_parent_data, >>> + .num_parents = 2, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + [...] >>> + >>> +/* Video Clocks */ >>> +static struct clk_regmap s4_vid_pll_div = { >>> + .data = &(struct meson_vid_pll_div_data){ >>> + .val = { >>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>> + .shift = 0, >>> + .width = 15, >>> + }, >>> + .sel = { >>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>> + .shift = 16, >>> + .width = 2, >>> + }, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "vid_pll_div", >>> + /* Same to g12a */ >>> + .ops = &meson_vid_pll_div_ro_ops, >> Please add an helpful explanation. >> 'Same to g12a' is not helpful. >> > > "Because the vid_pll_div clock is a clock that does not need to change the > divisor, ops only provides meson_vid_pll_div_ro_ops." > I wonder if this description is ok for you? I understand this divider will not change with RO ops. I'm interrested why it does not change and how it is expected to be setup. > >>> + .parent_data = (const struct clk_parent_data []) { >>> + { .fw_name = "hdmi_pll", } >>> + }, >>> + .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; [...] >>> + >>> +static struct clk_regmap s4_vclk_sel = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_VID_CLK_CTRL, >>> + .mask = 0x7, >>> + .shift = 16, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "vclk_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_vclk_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_vclk_parent_data), >>> + }, >> You are stopping rate propagation here. >> It deserves an explanation. Same goes below. > > "When the driver uses this clock, needs to specify the patent clock he > wants in the dts." > Is ok for you? Then you still don't understand the clock flag usage. Preserving the parent selection (CLK_SET_RATE_NO_REPARENT) and rate propagation (CLK_SET_RATE_PARENT) is not the same thing. As it stands, your comment is not aliged with what you do. > >> >>> +};
Hi Jerome, On 2022/11/28 20:23, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Mon 28 Nov 2022 at 16:08, Yu Tu <yu.tu@amlogic.com> wrote: > >>>> + >>>> +/* >>>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator. >>>> + * If it is used, it should be documented in using fw_name and documented in the >>>> + * Bindings. Not currently in use on this board, so skip it. >>>> + */ >>>> +static u32 rtc_clk_sel[] = { 0, 1 }; >>> No reason to do that >> >> I'm going to change it to static u32 rtc_clk_sel[] = { 0, 1, 2 };. >> I don't know if that's okay with you? > > ... then there is no need to specify this table. > I got it.I'll change it as you suggest. > > >> >>> >>>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = { >>>> + { .hw = &s4_rtc_32k_by_oscin.hw }, >>>> + { .hw = &s4_rtc_32k_by_oscin_div.hw }, >>>> + { .fw_name = "ext_32k", } >>>> +}; >>>> + >>>> +static struct clk_regmap s4_rtc_clk = { >>>> + .data = &(struct clk_regmap_mux_data) { >>>> + .offset = CLKCTRL_RTC_CTRL, >>>> + .mask = 0x3, >>>> + .shift = 0, >>>> + .table = rtc_clk_sel, >>>> + .flags = CLK_MUX_ROUND_CLOSEST, >>>> + }, >>>> + .hw.init = &(struct clk_init_data){ >>>> + .name = "rtc_clk_sel", >>>> + .ops = &clk_regmap_mux_ops, >>>> + .parent_data = rtc_clk_sel_parent_data, >>>> + .num_parents = 2, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> + }, >>>> +}; >>>> + > > [...] > >>>> + >>>> +/* Video Clocks */ >>>> +static struct clk_regmap s4_vid_pll_div = { >>>> + .data = &(struct meson_vid_pll_div_data){ >>>> + .val = { >>>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>>> + .shift = 0, >>>> + .width = 15, >>>> + }, >>>> + .sel = { >>>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>>> + .shift = 16, >>>> + .width = 2, >>>> + }, >>>> + }, >>>> + .hw.init = &(struct clk_init_data) { >>>> + .name = "vid_pll_div", >>>> + /* Same to g12a */ >>>> + .ops = &meson_vid_pll_div_ro_ops, >>> Please add an helpful explanation. >>> 'Same to g12a' is not helpful. >>> >> >> "Because the vid_pll_div clock is a clock that does not need to change the >> divisor, ops only provides meson_vid_pll_div_ro_ops." >> I wonder if this description is ok for you? > > I understand this divider will not change with RO ops. > I'm interrested why it does not change and how it is expected to be setup. > Maybe you can be more specific, I don't understand, you're interested in that part of it specifically. I don't know if you have the document of chip. If not, I can provide it to you privately. You can ask specific questions in conjunction with your documentation and submission(The original submission came from you.). I can give you a specific answer or ask the chip designer to give you a reply.Do you think that's okay with you >> >>>> + .parent_data = (const struct clk_parent_data []) { >>>> + { .fw_name = "hdmi_pll", } >>>> + }, >>>> + .num_parents = 1, >>>> + .flags = CLK_SET_RATE_PARENT, >>>> + }, >>>> +}; > > [...] > >>>> + >>>> +static struct clk_regmap s4_vclk_sel = { >>>> + .data = &(struct clk_regmap_mux_data){ >>>> + .offset = CLKCTRL_VID_CLK_CTRL, >>>> + .mask = 0x7, >>>> + .shift = 16, >>>> + }, >>>> + .hw.init = &(struct clk_init_data){ >>>> + .name = "vclk_sel", >>>> + .ops = &clk_regmap_mux_ops, >>>> + .parent_data = s4_vclk_parent_data, >>>> + .num_parents = ARRAY_SIZE(s4_vclk_parent_data), >>>> + }, >>> You are stopping rate propagation here. >>> It deserves an explanation. Same goes below. >> >> "When the driver uses this clock, needs to specify the patent clock he >> wants in the dts." >> Is ok for you? > > Then you still don't understand the clock flag usage. > > Preserving the parent selection (CLK_SET_RATE_NO_REPARENT) and rate > propagation (CLK_SET_RATE_PARENT) is not the same thing. > > As it stands, your comment is not aliged with what you do. > Thanks for the explanation of flag. My goal is to have the clock user describe themselves in DTS using the parent, or using the assigned-clocks and assigned-clock-parents settings in DTS. According to your explanation, some clocks like this should use CLK_SET_RATE_NO_REPARENT, right? >> >>> >>>> +}; > > .
1. Add PLL and Peripheral clock controller driver for S4 SOC. Yu Tu (4): clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver and bindings arm64: dts: meson: add S4 Soc PLL clock controller in DT clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings arm64: dts: meson: add S4 Soc Peripheral clock controller in DT V4 -> V5: change format and clock flags and adjust the patch series as suggested by Jerome. V3 -> V4: change format and clock flags. V2 -> V3: Use two clock controller. V1 -> V2: Change format as discussed in the email. Link:https://lore.kernel.org/all/20220823022630.25007-1-yu.tu@amlogic.com/ .../clock/amlogic,s4-peripherals-clkc.yaml | 105 + .../bindings/clock/amlogic,s4-pll-clkc.yaml | 51 + MAINTAINERS | 1 + arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 34 + drivers/clk/meson/Kconfig | 25 + drivers/clk/meson/Makefile | 2 + drivers/clk/meson/s4-peripherals.c | 3783 +++++++++++++++++ drivers/clk/meson/s4-peripherals.h | 218 + drivers/clk/meson/s4-pll.c | 875 ++++ drivers/clk/meson/s4-pll.h | 88 + .../clock/amlogic,s4-peripherals-clkc.h | 131 + .../dt-bindings/clock/amlogic,s4-pll-clkc.h | 30 + 12 files changed, 5343 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-peripherals-clkc.yaml create mode 100644 Documentation/devicetree/bindings/clock/amlogic,s4-pll-clkc.yaml create mode 100644 drivers/clk/meson/s4-peripherals.c create mode 100644 drivers/clk/meson/s4-peripherals.h create mode 100644 drivers/clk/meson/s4-pll.c create mode 100644 drivers/clk/meson/s4-pll.h create mode 100644 include/dt-bindings/clock/amlogic,s4-peripherals-clkc.h create mode 100644 include/dt-bindings/clock/amlogic,s4-pll-clkc.h base-commit: 5eeec1fd8360d57899d29a607ff81d0094e6cf59