| Message ID | 20210408185731.135511-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM | expand |
Hi Marek, Thanks for the patchset On 4/8/21 8:57 PM, Marek Vasut wrote: > The ETHCK_K are modeled as composite clock of MUX and GATE, however per > STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral > clock distribution for Ethernet, ETHPTPDIV divider is attached past the > ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate. > Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are > in use, ETHCKEN gate can be turned off. Current driver does not permit > that, fix it. I don"t understand, it's already the case. ETHCK_K it's a composite with a MUX and a GATE. ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no gate) If you use only ETHPTPDIV, ETHCKEN gate can be turned off. > This patch converts ETHCK_K from composite clock into a ETHCKEN gate, > ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another > NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock > to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and > ETHPTP_K remain functional as before. > > [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574, > Figure 83. Peripheral clock distribution for Ethernet > https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf > > Signed-off-by: Marek Vasut<marex@denx.de> > Cc: Alexandre Torgue<alexandre.torgue@foss.st.com> > Cc: Christophe Roullier<christophe.roullier@foss.st.com> > Cc: Gabriel Fernandez<gabriel.fernandez@foss.st.com> > Cc: Patrice Chotard<patrice.chotard@foss.st.com> > Cc: Patrick Delaunay<patrick.delaunay@foss.st.com> > Cc: Stephen Boyd<swboyd@chromium.org> > Cc:linux-clk@vger.kernel.org > Cc:linux-stm32@st-md-mailman.stormreply.com > To:linux-arm-kernel@lists.infradead.org > --- > drivers/clk/clk-stm32mp1.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c > index a875649df8b8..a7c7f544ee5d 100644 > --- a/drivers/clk/clk-stm32mp1.c > +++ b/drivers/clk/clk-stm32mp1.c > @@ -1949,7 +1949,6 @@ static const struct clock_config stm32mp1_clock_cfg[] = { > KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI), > KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1), > KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO), > - KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK), > > /* Particulary Kernel Clocks (no mux or no gate) */ > MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM), > @@ -1958,11 +1957,16 @@ static const struct clock_config stm32mp1_clock_cfg[] = { > MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU), > MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12), > > - COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE | > + COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE | > CLK_SET_RATE_NO_REPARENT, > _NO_GATE, > _MMUX(M_ETHCK), > - _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), > + _NO_DIV), > + > + MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK), assigned parent with ETHCK_K will not work > + > + DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE | CLK_OPS_PARENT_ENABLE flags not useful with a divider. best regards Gabriel > + CLK_SET_RATE_NO_REPARENT, RCC_ETHCKSELR, 4, 4, 0), > > /* RTC clock */ > DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0),
On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote: > Hi Marek, Hello Gabriel, > Thanks for the patchset > > On 4/8/21 8:57 PM, Marek Vasut wrote: >> The ETHCK_K are modeled as composite clock of MUX and GATE, however per >> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral >> clock distribution for Ethernet, ETHPTPDIV divider is attached past the >> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate. >> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are >> in use, ETHCKEN gate can be turned off. Current driver does not permit >> that, fix it. > > I don"t understand, it's already the case. > > ETHCK_K it's a composite with a MUX and a GATE. But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the datasheet again and schematic below. > ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no gate) But ETHPTP_K shouldn't control any mux, it is only a divider. > If you use only ETHPTPDIV, ETHCKEN gate can be turned off. Look, this is what you have today: .------------ ETHCK_K -----------. |_______ _______ | pll4_p_ck--|M_ETHCK\ |G_ETHCK\ | | MUX |------+-----| GATE |-------------[x] ETH_CLK pll3_q_ck--|_______/ | |_______/ eth_clk_fb | | | '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref | | '------------ ETHPTP_K --------------------' And this is what you should have, to avoid having two composite clock which control the same mux using the same register bit, i.e. what this patch implements: .- ck_ker_eth -. .--- ETHCK_K --. |_______ | | _______ | pll4_p_ck--|M_ETHCK\ | | |G_ETHCK\ | | MUX |------+-----| GATE |-------------[x] ETH_CLK pll3_q_ck--|_______/ | |_______/ eth_clk_fb | '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref | | '---- ETHPTP_K -----------' >> This patch converts ETHCK_K from composite clock into a ETHCKEN gate, >> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another >> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock >> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and >> ETHPTP_K remain functional as before. >> >> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574, >> Figure 83. Peripheral clock distribution for Ethernet >> >> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf >> [...] >> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c >> index a875649df8b8..a7c7f544ee5d 100644 >> --- a/drivers/clk/clk-stm32mp1.c >> +++ b/drivers/clk/clk-stm32mp1.c >> @@ -1949,7 +1949,6 @@ static const struct clock_config >> stm32mp1_clock_cfg[] = { >> KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI), >> KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1), >> KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO), >> - KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK), >> /* Particulary Kernel Clocks (no mux or no gate) */ >> MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM), >> @@ -1958,11 +1957,16 @@ static const struct clock_config >> stm32mp1_clock_cfg[] = { >> MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU), >> MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12), >> - COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE | >> + COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE | >> CLK_SET_RATE_NO_REPARENT, >> _NO_GATE, >> _MMUX(M_ETHCK), >> - _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), >> + _NO_DIV), >> + >> + MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK), > assigned parent with ETHCK_K will not work >> + >> + DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE | > > CLK_OPS_PARENT_ENABLE flags not useful with a divider. How so ?
Hi Marek On 4/14/21 4:04 PM, Marek Vasut wrote: > On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote: >> Hi Marek, > > Hello Gabriel, > >> Thanks for the patchset >> >> On 4/8/21 8:57 PM, Marek Vasut wrote: >>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per >>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral >>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the >>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate. >>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are >>> in use, ETHCKEN gate can be turned off. Current driver does not permit >>> that, fix it. >> >> I don"t understand, it's already the case. >> >> ETHCK_K it's a composite with a MUX and a GATE. > > But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the > datasheet again and schematic below. > >> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no >> gate) > > But ETHPTP_K shouldn't control any mux, it is only a divider. > >> If you use only ETHPTPDIV, ETHCKEN gate can be turned off. > > Look, this is what you have today: > > .------------ ETHCK_K -----------. > |_______ _______ | > pll4_p_ck--|M_ETHCK\ |G_ETHCK\ | > | MUX |------+-----| GATE |-------------[x] ETH_CLK > pll3_q_ck--|_______/ | |_______/ eth_clk_fb > | | > | '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref > | | > '------------ ETHPTP_K --------------------' > > And this is what you should have, to avoid having two composite clock > which control the same mux using the same register bit, i.e. what this > patch implements: > > .- ck_ker_eth -. .--- ETHCK_K --. > |_______ | | _______ | > pll4_p_ck--|M_ETHCK\ | | |G_ETHCK\ | > | MUX |------+-----| GATE |-------------[x] ETH_CLK > pll3_q_ck--|_______/ | |_______/ eth_clk_fb > | > '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref > | | > '---- ETHPTP_K -----------' > These 2 solutions are valid. I made the choice to implement the first one to be able to change parent with the kernel clock of the IP (no need to add an intermediate binding). It's the same principle for all kernel of this soc. I can ask to Alexandre to comeback of this principle, but i 'm not favorable. >>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate, >>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another >>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock >>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and >>> ETHPTP_K remain functional as before. >>> >>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574, >>> Figure 83. Peripheral clock distribution for Ethernet >>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf >>> > > [...] > >>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c >>> index a875649df8b8..a7c7f544ee5d 100644 >>> --- a/drivers/clk/clk-stm32mp1.c >>> +++ b/drivers/clk/clk-stm32mp1.c >>> @@ -1949,7 +1949,6 @@ static const struct clock_config >>> stm32mp1_clock_cfg[] = { >>> KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI), >>> KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1), >>> KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO), >>> - KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK), >>> /* Particulary Kernel Clocks (no mux or no gate) */ >>> MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM), >>> @@ -1958,11 +1957,16 @@ static const struct clock_config >>> stm32mp1_clock_cfg[] = { >>> MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU), >>> MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12), >>> - COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE | >>> + COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE | >>> CLK_SET_RATE_NO_REPARENT, >>> _NO_GATE, >>> _MMUX(M_ETHCK), >>> - _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), >>> + _NO_DIV), >>> + >>> + MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK), >> assigned parent with ETHCK_K will not work >>> + >>> + DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE | >> >> CLK_OPS_PARENT_ENABLE flags not useful with a divider. > > How so ?
On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote: > Hi Marek Hello Gabriel, > On 4/14/21 4:04 PM, Marek Vasut wrote: >> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote: >>> Hi Marek, >> >> Hello Gabriel, >> >>> Thanks for the patchset >>> >>> On 4/8/21 8:57 PM, Marek Vasut wrote: >>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per >>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral >>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the >>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate. >>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are >>>> in use, ETHCKEN gate can be turned off. Current driver does not permit >>>> that, fix it. >>> >>> I don"t understand, it's already the case. >>> >>> ETHCK_K it's a composite with a MUX and a GATE. >> >> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the >> datasheet again and schematic below. >> >>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no >>> gate) >> >> But ETHPTP_K shouldn't control any mux, it is only a divider. >> >>> If you use only ETHPTPDIV, ETHCKEN gate can be turned off. >> >> Look, this is what you have today: >> >> .------------ ETHCK_K -----------. >> |_______ _______ | >> pll4_p_ck--|M_ETHCK\ |G_ETHCK\ | >> | MUX |------+-----| GATE |-------------[x] ETH_CLK >> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >> | | >> | '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref >> | | >> '------------ ETHPTP_K --------------------' >> >> And this is what you should have, to avoid having two composite clock >> which control the same mux using the same register bit, i.e. what this >> patch implements: >> >> .- ck_ker_eth -. .--- ETHCK_K --. >> |_______ | | _______ | >> pll4_p_ck--|M_ETHCK\ | | |G_ETHCK\ | >> | MUX |------+-----| GATE |-------------[x] ETH_CLK >> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >> | >> '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref >> | | >> '---- ETHPTP_K -----------' >> > > These 2 solutions are valid. I made the choice to implement the first > one to be able to change parent with the kernel clock of the IP (no need > to add an intermediate binding). Which IP are you talking about in here ? > It's the same principle for all kernel > of this soc. The first option is wrong, because in that model, you have two composite clock which control the same one mux bit in the same register. Basically you register two distinct clock which operate the same hardware knob. > I can ask to Alexandre to comeback of this principle, but i 'm not > favorable. Please ask. >>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate, >>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another >>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent >>>> clock >>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and >>>> ETHPTP_K remain functional as before. >>>> >>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574, >>>> Figure 83. Peripheral clock distribution for Ethernet >>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf >>>> >> >> [...] >> >>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c >>>> index a875649df8b8..a7c7f544ee5d 100644 >>>> --- a/drivers/clk/clk-stm32mp1.c >>>> +++ b/drivers/clk/clk-stm32mp1.c >>>> @@ -1949,7 +1949,6 @@ static const struct clock_config >>>> stm32mp1_clock_cfg[] = { >>>> KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI), >>>> KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1), >>>> KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO), >>>> - KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK), >>>> /* Particulary Kernel Clocks (no mux or no gate) */ >>>> MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM), >>>> @@ -1958,11 +1957,16 @@ static const struct clock_config >>>> stm32mp1_clock_cfg[] = { >>>> MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU), >>>> MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12), >>>> - COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE | >>>> + COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE | >>>> CLK_SET_RATE_NO_REPARENT, >>>> _NO_GATE, >>>> _MMUX(M_ETHCK), >>>> - _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), >>>> + _NO_DIV), >>>> + >>>> + MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK), >>> assigned parent with ETHCK_K will not work >>>> + >>>> + DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE | >>> >>> CLK_OPS_PARENT_ENABLE flags not useful with a divider. >> >> How so ?
On 4/16/21 3:47 PM, Marek Vasut wrote: > On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote: >> Hi Marek > > Hello Gabriel, > >> On 4/14/21 4:04 PM, Marek Vasut wrote: >>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote: >>>> Hi Marek, >>> >>> Hello Gabriel, >>> >>>> Thanks for the patchset >>>> >>>> On 4/8/21 8:57 PM, Marek Vasut wrote: >>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however >>>>> per >>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. >>>>> Peripheral >>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past >>>>> the >>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN >>>>> gate. >>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are >>>>> in use, ETHCKEN gate can be turned off. Current driver does not permit >>>>> that, fix it. >>>> >>>> I don"t understand, it's already the case. >>>> >>>> ETHCK_K it's a composite with a MUX and a GATE. >>> >>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the >>> datasheet again and schematic below. >>> >>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV >>>> (no gate) >>> >>> But ETHPTP_K shouldn't control any mux, it is only a divider. >>> >>>> If you use only ETHPTPDIV, ETHCKEN gate can be turned off. >>> >>> Look, this is what you have today: >>> >>> .------------ ETHCK_K -----------. >>> |_______ _______ | >>> pll4_p_ck--|M_ETHCK\ |G_ETHCK\ | >>> | MUX |------+-----| GATE |-------------[x] ETH_CLK >>> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >>> | | >>> | '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref >>> | | >>> '------------ ETHPTP_K --------------------' >>> >>> And this is what you should have, to avoid having two composite clock >>> which control the same mux using the same register bit, i.e. what >>> this patch implements: >>> >>> .- ck_ker_eth -. .--- ETHCK_K --. >>> |_______ | | _______ | >>> pll4_p_ck--|M_ETHCK\ | | |G_ETHCK\ | >>> | MUX |------+-----| GATE |-------------[x] ETH_CLK >>> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >>> | >>> '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref >>> | | >>> '---- ETHPTP_K -----------' >>> >> >> These 2 solutions are valid. I made the choice to implement the first >> one to be able to change parent with the kernel clock of the IP (no >> need to add an intermediate binding). > > Which IP are you talking about in here ? > >> It's the same principle for all kernel of this soc. > > The first option is wrong, because in that model, you have two composite > clock which control the same one mux bit in the same register. Basically > you register two distinct clock which operate the same hardware knob. > >> I can ask to Alexandre to comeback of this principle, but i 'm not >> favorable. > The only discussing thing is how the clock is shown. I mean either two composites or one mux plus two gates. Gabriel made a choice to abstract the mux in two composite clocks. But it seems that at the end we have the same behaviour, isn't ? Adding "ck_ker_eth" would impose a new clock to take in DT ? regards alex > Please ask. > >>>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate, >>>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another >>>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent >>>>> clock >>>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and >>>>> ETHPTP_K remain functional as before. >>>>> >>>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574, >>>>> Figure 83. Peripheral clock distribution for Ethernet >>>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf >>>>> >>> >>> [...] >>> >>>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c >>>>> index a875649df8b8..a7c7f544ee5d 100644 >>>>> --- a/drivers/clk/clk-stm32mp1.c >>>>> +++ b/drivers/clk/clk-stm32mp1.c >>>>> @@ -1949,7 +1949,6 @@ static const struct clock_config >>>>> stm32mp1_clock_cfg[] = { >>>>> KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI), >>>>> KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1), >>>>> KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO), >>>>> - KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK), >>>>> /* Particulary Kernel Clocks (no mux or no gate) */ >>>>> MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM), >>>>> @@ -1958,11 +1957,16 @@ static const struct clock_config >>>>> stm32mp1_clock_cfg[] = { >>>>> MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU), >>>>> MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12), >>>>> - COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE | >>>>> + COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE | >>>>> CLK_SET_RATE_NO_REPARENT, >>>>> _NO_GATE, >>>>> _MMUX(M_ETHCK), >>>>> - _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), >>>>> + _NO_DIV), >>>>> + >>>>> + MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK), >>>> assigned parent with ETHCK_K will not work >>>>> + >>>>> + DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE | >>>> >>>> CLK_OPS_PARENT_ENABLE flags not useful with a divider. >>> >>> How so ?
On 4/16/21 5:23 PM, Alexandre TORGUE wrote: Hello Alexandre, > On 4/16/21 3:47 PM, Marek Vasut wrote: >> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote: >>> Hi Marek >> >> Hello Gabriel, >> >>> On 4/14/21 4:04 PM, Marek Vasut wrote: >>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote: >>>>> Hi Marek, >>>> >>>> Hello Gabriel, >>>> >>>>> Thanks for the patchset >>>>> >>>>> On 4/8/21 8:57 PM, Marek Vasut wrote: >>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, >>>>>> however per >>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. >>>>>> Peripheral >>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached >>>>>> past the >>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN >>>>>> gate. >>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock >>>>>> are >>>>>> in use, ETHCKEN gate can be turned off. Current driver does not >>>>>> permit >>>>>> that, fix it. >>>>> >>>>> I don"t understand, it's already the case. >>>>> >>>>> ETHCK_K it's a composite with a MUX and a GATE. >>>> >>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the >>>> datasheet again and schematic below. >>>> >>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV >>>>> (no gate) >>>> >>>> But ETHPTP_K shouldn't control any mux, it is only a divider. >>>> >>>>> If you use only ETHPTPDIV, ETHCKEN gate can be turned off. >>>> >>>> Look, this is what you have today: >>>> >>>> .------------ ETHCK_K -----------. >>>> |_______ _______ | >>>> pll4_p_ck--|M_ETHCK\ |G_ETHCK\ | >>>> | MUX |------+-----| GATE |-------------[x] ETH_CLK >>>> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >>>> | | >>>> | '--(ETHCKSELR[7:4] divider)--[x] >>>> clk_ptp_ref >>>> | | >>>> '------------ ETHPTP_K --------------------' >>>> >>>> And this is what you should have, to avoid having two composite >>>> clock which control the same mux using the same register bit, i.e. >>>> what this patch implements: >>>> >>>> .- ck_ker_eth -. .--- ETHCK_K --. >>>> |_______ | | _______ | >>>> pll4_p_ck--|M_ETHCK\ | | |G_ETHCK\ | >>>> | MUX |------+-----| GATE |-------------[x] ETH_CLK >>>> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >>>> | >>>> '--(ETHCKSELR[7:4] divider)--[x] >>>> clk_ptp_ref >>>> | | >>>> '---- ETHPTP_K -----------' >>>> >>> >>> These 2 solutions are valid. I made the choice to implement the first >>> one to be able to change parent with the kernel clock of the IP (no >>> need to add an intermediate binding). >> >> Which IP are you talking about in here ? >> >>> It's the same principle for all kernel of this soc. >> >> The first option is wrong, because in that model, you have two >> composite clock which control the same one mux bit in the same >> register. Basically you register two distinct clock which operate the >> same hardware knob. >> >>> I can ask to Alexandre to comeback of this principle, but i 'm not >>> favorable. >> > > The only discussing thing is how the clock is shown. I mean either two > composites or one mux plus two gates. Gabriel made a choice to abstract > the mux in two composite clocks. But it seems that at the end we have > the same behaviour, isn't ? Not really. Since the two composite clock control the same mux bit, consider what would happen if you were to select pll4_p_ck as parent for one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. ETHPTP_K), what would be the result ? I guess the result would depend on when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I don't think that's how it should work. With a single mux controlling that one single bit, such situation wouldn't happen. > Adding "ck_ker_eth" would impose a new clock to take in DT ? Nope, the ck_ker_eth is without ID and internal to the driver. They exist only to describe the clock tree correctly. [...]
Hi Marek, On 4/16/21 5:31 PM, Marek Vasut wrote: > On 4/16/21 5:23 PM, Alexandre TORGUE wrote: > > Hello Alexandre, > >> On 4/16/21 3:47 PM, Marek Vasut wrote: >>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote: >>>> Hi Marek >>> >>> Hello Gabriel, >>> >>>> On 4/14/21 4:04 PM, Marek Vasut wrote: >>>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote: >>>>>> Hi Marek, >>>>> >>>>> Hello Gabriel, >>>>> >>>>>> Thanks for the patchset >>>>>> >>>>>> On 4/8/21 8:57 PM, Marek Vasut wrote: >>>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, >>>>>>> however per >>>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. >>>>>>> Peripheral >>>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached >>>>>>> past the >>>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN >>>>>>> gate. >>>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP >>>>>>> clock are >>>>>>> in use, ETHCKEN gate can be turned off. Current driver does not >>>>>>> permit >>>>>>> that, fix it. >>>>>> >>>>>> I don"t understand, it's already the case. >>>>>> >>>>>> ETHCK_K it's a composite with a MUX and a GATE. >>>>> >>>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in >>>>> the datasheet again and schematic below. >>>>> >>>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV >>>>>> (no gate) >>>>> >>>>> But ETHPTP_K shouldn't control any mux, it is only a divider. >>>>> >>>>>> If you use only ETHPTPDIV, ETHCKEN gate can be turned off. >>>>> >>>>> Look, this is what you have today: >>>>> >>>>> .------------ ETHCK_K -----------. >>>>> |_______ _______ | >>>>> pll4_p_ck--|M_ETHCK\ |G_ETHCK\ | >>>>> | MUX |------+-----| GATE |-------------[x] ETH_CLK >>>>> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >>>>> | | >>>>> | '--(ETHCKSELR[7:4] divider)--[x] >>>>> clk_ptp_ref >>>>> | | >>>>> '------------ ETHPTP_K --------------------' >>>>> >>>>> And this is what you should have, to avoid having two composite >>>>> clock which control the same mux using the same register bit, i.e. >>>>> what this patch implements: >>>>> >>>>> .- ck_ker_eth -. .--- ETHCK_K --. >>>>> |_______ | | _______ | >>>>> pll4_p_ck--|M_ETHCK\ | | |G_ETHCK\ | >>>>> | MUX |------+-----| GATE |-------------[x] ETH_CLK >>>>> pll3_q_ck--|_______/ | |_______/ eth_clk_fb >>>>> | >>>>> '--(ETHCKSELR[7:4] divider)--[x] >>>>> clk_ptp_ref >>>>> | | >>>>> '---- ETHPTP_K -----------' >>>>> >>>> >>>> These 2 solutions are valid. I made the choice to implement the >>>> first one to be able to change parent with the kernel clock of the >>>> IP (no need to add an intermediate binding). >>> >>> Which IP are you talking about in here ? >>> >>>> It's the same principle for all kernel of this soc. >>> >>> The first option is wrong, because in that model, you have two >>> composite clock which control the same one mux bit in the same >>> register. Basically you register two distinct clock which operate the >>> same hardware knob. >>> >>>> I can ask to Alexandre to comeback of this principle, but i 'm not >>>> favorable. >>> >> >> The only discussing thing is how the clock is shown. I mean either two >> composites or one mux plus two gates. Gabriel made a choice to >> abstract the mux in two composite clocks. But it seems that at the end >> we have the same behaviour, isn't ? > > Not really. Since the two composite clock control the same mux bit, > consider what would happen if you were to select pll4_p_ck as parent for > one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. > ETHPTP_K), what would be the result ? I guess the result would depend on > when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I > don't think that's how it should work. With a single mux controlling > that one single bit, such situation wouldn't happen. The reparenting is managed. This mux has specific ops. root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent && cat /sys/kernel/debug/clk/ethptp_k/clk_parent pll4_p pll4_p root@stm32mp1-disco-oss:~# echo pll3_q > /sys/kernel/debug/clk/ethptp_k/clk_set_parent root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent && cat /sys/kernel/debug/clk/ethptp_k/clk_parent pll3_q pll3_q > >> Adding "ck_ker_eth" would impose a new clock to take in DT ? > Nope, the ck_ker_eth is without ID and internal to the driver. They > exist only to describe the clock tree correctly. > > [...]
On 4/19/21 09:46, gabriel.fernandez@foss.st.com wrote: Hello again, [...] I sent out an rebased (and much shorter) patch series now: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=606380
diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c index a875649df8b8..a7c7f544ee5d 100644 --- a/drivers/clk/clk-stm32mp1.c +++ b/drivers/clk/clk-stm32mp1.c @@ -1949,7 +1949,6 @@ static const struct clock_config stm32mp1_clock_cfg[] = { KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI), KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1), KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO), - KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK), /* Particulary Kernel Clocks (no mux or no gate) */ MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM), @@ -1958,11 +1957,16 @@ static const struct clock_config stm32mp1_clock_cfg[] = { MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU), MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12), - COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE | + COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_NO_REPARENT, _NO_GATE, _MMUX(M_ETHCK), - _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)), + _NO_DIV), + + MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK), + + DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE | + CLK_SET_RATE_NO_REPARENT, RCC_ETHCKSELR, 4, 4, 0), /* RTC clock */ DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0),
The ETHCK_K are modeled as composite clock of MUX and GATE, however per STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral clock distribution for Ethernet, ETHPTPDIV divider is attached past the ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate. Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are in use, ETHCKEN gate can be turned off. Current driver does not permit that, fix it. This patch converts ETHCK_K from composite clock into a ETHCKEN gate, ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and ETHPTP_K remain functional as before. [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral clock distribution for Ethernet https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Christophe Roullier <christophe.roullier@foss.st.com> Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com> Cc: Patrice Chotard <patrice.chotard@foss.st.com> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> Cc: Stephen Boyd <swboyd@chromium.org> Cc: linux-clk@vger.kernel.org Cc: linux-stm32@st-md-mailman.stormreply.com To: linux-arm-kernel@lists.infradead.org --- drivers/clk/clk-stm32mp1.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)