From: "gabriel.fernandez@foss.st.com" <gabriel.fernandez@foss.st.com> To: Marek Vasut <marex@denx.de>, Alexandre TORGUE <alexandre.torgue@foss.st.com>, <linux-arm-kernel@lists.infradead.org> Cc: Christophe Roullier <christophe.roullier@foss.st.com>, Patrice Chotard <patrice.chotard@foss.st.com>, Patrick Delaunay <patrick.delaunay@foss.st.com>, Stephen Boyd <swboyd@chromium.org>, <linux-clk@vger.kernel.org>, <linux-stm32@st-md-mailman.stormreply.com>, Stephen Boyd <sboyd@kernel.org> Subject: Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock Date: Mon, 19 Apr 2021 09:46:27 +0200 [thread overview] Message-ID: <8481872c-9ee0-c759-3ab0-5209165ad9b2@foss.st.com> (raw) In-Reply-To: <2281af74-33a0-df45-968b-baa1ddd9d6e0@denx.de> 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. > > [...]
WARNING: multiple messages have this Message-ID (diff)
From: "gabriel.fernandez@foss.st.com" <gabriel.fernandez@foss.st.com> To: Marek Vasut <marex@denx.de>, Alexandre TORGUE <alexandre.torgue@foss.st.com>, <linux-arm-kernel@lists.infradead.org> Cc: Christophe Roullier <christophe.roullier@foss.st.com>, Patrice Chotard <patrice.chotard@foss.st.com>, Patrick Delaunay <patrick.delaunay@foss.st.com>, Stephen Boyd <swboyd@chromium.org>, <linux-clk@vger.kernel.org>, <linux-stm32@st-md-mailman.stormreply.com>, Stephen Boyd <sboyd@kernel.org> Subject: Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock Date: Mon, 19 Apr 2021 09:46:27 +0200 [thread overview] Message-ID: <8481872c-9ee0-c759-3ab0-5209165ad9b2@foss.st.com> (raw) In-Reply-To: <2281af74-33a0-df45-968b-baa1ddd9d6e0@denx.de> 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. > > [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-04-19 7:46 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-08 18:57 [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-08 18:57 ` [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-14 13:03 ` gabriel.fernandez 2021-04-14 13:03 ` gabriel.fernandez 2021-04-14 14:04 ` Marek Vasut 2021-04-14 14:04 ` Marek Vasut 2021-04-16 6:44 ` gabriel.fernandez 2021-04-16 6:44 ` gabriel.fernandez 2021-04-16 13:47 ` Marek Vasut 2021-04-16 13:47 ` Marek Vasut 2021-04-16 15:23 ` Alexandre TORGUE 2021-04-16 15:23 ` Alexandre TORGUE 2021-04-16 15:31 ` Marek Vasut 2021-04-16 15:31 ` Marek Vasut 2021-04-19 7:46 ` gabriel.fernandez [this message] 2021-04-19 7:46 ` gabriel.fernandez 2022-01-18 22:11 ` Marek Vasut 2022-01-18 22:11 ` Marek Vasut 2021-04-08 18:57 ` [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-16 6:44 ` gabriel.fernandez 2021-04-16 6:44 ` gabriel.fernandez 2021-04-16 13:39 ` Marek Vasut 2021-04-16 13:39 ` Marek Vasut 2021-04-16 14:39 ` Alexandre TORGUE 2021-04-16 14:39 ` Alexandre TORGUE 2021-04-16 14:54 ` Marek Vasut 2021-04-16 14:54 ` Marek Vasut 2021-04-16 15:01 ` Alexandre TORGUE 2021-04-16 15:01 ` Alexandre TORGUE 2021-04-08 18:57 ` [PATCH 3/7] clk: stm32mp1: Register clock with device_node pointer Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-08 18:57 ` [PATCH 4/7] clk: stm32mp1: Add parent_data to ETHRX clock Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-08 18:57 ` [PATCH 5/7] ARM: dts: stm32: Add alternate pinmux for ethernet0 pins Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-08 18:57 ` [PATCH 6/7] ARM: dts: stm32: Add alternate pinmux for mco2 pins Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-08 18:57 ` [PATCH 7/7] ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM Marek Vasut 2021-04-08 18:57 ` Marek Vasut 2021-04-08 20:32 ` [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM Stephen Boyd 2021-04-08 20:32 ` Stephen Boyd 2021-04-12 8:09 ` Alexandre TORGUE 2021-04-12 8:09 ` Alexandre TORGUE 2021-04-12 18:44 ` Marek Vasut 2021-04-12 18:44 ` Marek Vasut 2021-04-13 7:48 ` Alexandre TORGUE 2021-04-13 7:48 ` Alexandre TORGUE 2021-04-13 12:05 ` Marek Vasut 2021-04-13 12:05 ` Marek Vasut
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8481872c-9ee0-c759-3ab0-5209165ad9b2@foss.st.com \ --to=gabriel.fernandez@foss.st.com \ --cc=alexandre.torgue@foss.st.com \ --cc=christophe.roullier@foss.st.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=marex@denx.de \ --cc=patrice.chotard@foss.st.com \ --cc=patrick.delaunay@foss.st.com \ --cc=sboyd@kernel.org \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.